-
Notifications
You must be signed in to change notification settings - Fork 109
Conversation
this is for brave/browser-laptop#12223 btw |
|
||
void URLRequestHttpJob::NotifyHeadersComplete() { | ||
- DCHECK(!response_info_); | ||
+ std::string first_party_host = request_->site_for_cookies().host(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we care about this warning? https://cs.chromium.org/chromium/src/net/url_request/url_request.h?q=site_for_cookies&sq=package:chromium&l=270
site_for_cookies
is used for firstPartyUrl in webRequest also; i don't exactly remember why we decided it wasn't an issue there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even in this case we are using site_for_cookies
to figure out the first party url.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to https://tools.ietf.org/html/draft-ietf-httpbis-cookie-same-site-00 section 2.1, site for cookies is not the same as top-level site in a few cases:
- in a nested cross-origin iframe, the site-for-cookies is ''
- for shared workers belonging to documents with different origins, the site-for-cookies is ''
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess it doesn't matter in this case since we only use the first party host to check if it is the same as the request host (which would not be empty in the cases above): https://github.com/brave/muon/pull/527/files#diff-85ff0d997b1a2f089546f7b2823b0ad7R1869
+ } | ||
+ | ||
ProcessPublicKeyPinsHeader(); | ||
ProcessExpectCTHeader(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should also move ProcessPublicKeyPinsHeader
and ProcessExpectCTHeader
into the if (first_party_host == request_host) {...
block since they can be used for similar tracking
nice work! this looks like it'll work for new HSTS requests, but not HSTS state that the browser has already saved. can we delete the existing HSTS database when the user updates to this version of muon? (maybe can be done in browser-laptop) |
looks like |
this looks like something we could do in filtering in browser-laptop and add/remove the header as needed |
Closing in favor of: brave/browser-laptop#13437 |
Don't apply HSTS on 3rd party subresource loads, unless that subresource has been visited as a first party
Testing instructions:
HTTPS Everywhere
in preferences.safebrowsing-proxy.brave.com
(ping @jumde if the domain is not working)safebrowsing-proxy.brave.com
and confirm that the HTTPS upgrade happens with a 307 redirect instead of 301.