Closed
Bug 1478539
Opened 6 years ago
Closed 6 years ago
doubleclick.net cookies are added to subresource requests after visiting a Google property serving doubleclick.net ads
Categories
(Core :: Networking: HTTP, defect, P2)
Core
Networking: HTTP
Tracking
()
VERIFIED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | verified |
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
(Blocks 1 open bug)
Details
(Whiteboard: [necko-triaged])
Attachments
(3 files, 2 obsolete files)
7.88 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
9.13 KB,
patch
|
u408661
:
review+
|
Details | Diff | Splinter Review |
3.18 KB,
patch
|
u408661
:
review+
|
Details | Diff | Splinter Review |
STR: 1. Turn on the cookie restrictions pref in a new profile. 2. Go to https://www.bbc.co.uk/news/science-environment-44952710. Note that the subresources from googleads.g.doubleclick.net try to set cookies but the cookies aren't accepted. 3. Go to https://www.youtube.com/. Note that subresources from pubads.g.doubleclick.net try to set cookies which are accepted. 4. Now go back to https://www.bbc.co.uk/news/science-environment-44952710. Subresources loaded from googleads.g.doubleclick.net are loaded with the Cookie HTTP header set. :-(
Assignee | ||
Comment 1•6 years ago
|
||
This happens because HttpBaseChannel::AddCookiesToRequest() is being called before the channel classification is completed. That function calls GetCookiesStringFromHttp <https://searchfox.org/mozilla-central/rev/943a6cf31a96eb439db8f241ab4df25c14003bb8/netwerk/protocol/http/HttpBaseChannel.cpp#3442> which relies on knowing the tracking status of a channel <https://searchfox.org/mozilla-central/rev/bdfd20ef30d521b57d5b6feeda71325e8b4cad66/netwerk/cookie/nsCookieService.cpp#2047> but that information isn't available yet. As a solution, AddCookiesToRequest() could be moved to be called from nsHttpChannel::OnBeforeConnect() which gets called after the classification is done, if needed. We have to be careful to dispatch http-on-modify-request after the Cookie header has been set though.
Component: Tracking Protection → Networking: HTTP
Product: Firefox → Core
Flags: needinfo?(jduell.mcbugs)
Priority: -- → P2
Whiteboard: [necko-triaged]
This seems relatively important, but not 100% sure how quickly someone needs to jump on this. ni?jduell so he can figure that out.
Assignee | ||
Comment 3•6 years ago
|
||
I've been working on it since a few hours ago. :-)
Assignee: nobody → ehsan
Assignee | ||
Comment 4•6 years ago
|
||
Besides this, we also need to do two extra things here: * Call the http-on-modify-request observers after the Cookie header is set, to ensure the Cookie header will be visible to them. * Move the call to SetLoadGroupUserAgentOverride() to happen after that, in order to preserve the existing respective ordering of that with regards to when the http-on-modify-request observers run. Please note that as things are right now, the http-on-modify-request observers are unable to see the User-Agent header overrides through the loadgroup, which seems suboptimal, but this patch aims to preserve the existing behavior of these observers.
Attachment #8995352 -
Flags: review?(hurley)
Assignee | ||
Comment 5•6 years ago
|
||
Attachment #8995353 -
Flags: review?(amarchesini)
Assignee | ||
Comment 6•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a595fe8141cf1bd2fc9a55289e7377262121822
Comment on attachment 8995352 [details] [diff] [review] Part 1: Add the Cookie header to HTTP requests only after a potential classification has been completed on the channel to ensure the tracking state is up to date Review of attachment 8995352 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +446,5 @@ > > + AddCookiesToRequest(); > + > + // notify "http-on-modify-request" observers > + CallOnModifyRequestObservers(); This looks reasonable, but you may want to update the comment for the check of mCanceled a bit later in this method, since it's not just http-on-before-connect that could cause that check to fire any more.
Attachment #8995352 -
Flags: review?(hurley) → review+
Assignee | ||
Comment 8•6 years ago
|
||
So it turns out that I forgot to handle dealing with the case where one of these callbacks suspends the channels, and thankfully try server failures reminded me. Also, the handling of cancellation and suspension in nsHttpChannel::BeginConnect() originally was just due to http-on-user-agent and http-on-modify-request, and it can now be removed. My patch still has one more failure (test_chrome_ext_webrequest_host_permissions.html) which unfortunately I didn't have time to drill into today...
Assignee | ||
Comment 9•6 years ago
|
||
Besides this, we also need to do two extra things here: * Call the http-on-modify-request observers after the Cookie header is set, to ensure the Cookie header will be visible to them. * Move the call to SetLoadGroupUserAgentOverride() to happen after that, in order to preserve the existing respective ordering of that with regards to when the http-on-modify-request observers run. Please note that as things are right now, the http-on-modify-request observers are unable to see the User-Agent header overrides through the loadgroup, which seems suboptimal, but this patch aims to preserve the existing behavior of these observers.
Assignee | ||
Updated•6 years ago
|
Attachment #8995352 -
Attachment is obsolete: true
Updated•6 years ago
|
Attachment #8995353 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to :Ehsan Akhgari from comment #8) > My patch still has one more failure > (test_chrome_ext_webrequest_host_permissions.html) which unfortunately I > didn't have time to drill into today... After banging my head against the desk for many hours, I discovered that the test failure here seems to be due to a bug in the test, bug 1482277 fixes that... Sending this to try before a final round of review.
Depends on: 1482277
Assignee | ||
Comment 11•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6073d5eca60beaa1b2aa23cbfb4d8194dcd597c
Assignee | ||
Comment 12•6 years ago
|
||
With an extra bug fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ad8f59e6bd272d06987ba296f9ee79a1b7232cc
Assignee | ||
Comment 13•6 years ago
|
||
Besides this, we also need to do two extra things here: * Call the http-on-modify-request observers after the Cookie header is set, to ensure the Cookie header will be visible to them. * Move the call to SetLoadGroupUserAgentOverride() to happen after that, in order to preserve the existing respective ordering of that with regards to when the http-on-modify-request observers run. Please note that as things are right now, the http-on-modify-request observers are unable to see the User-Agent header overrides through the loadgroup, which seems suboptimal, but this patch aims to preserve the existing behavior of these observers.
Attachment #8999223 -
Flags: review?(hurley)
Assignee | ||
Updated•6 years ago
|
Attachment #8995661 -
Attachment is obsolete: true
Assignee | ||
Comment 14•6 years ago
|
||
Attachment #8999224 -
Flags: review?(hurley)
Assignee | ||
Comment 15•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4517acadeba0ffbd12d79b811354e80fd358c75a
Attachment #8999223 -
Flags: review?(hurley) → review+
Comment 16•6 years ago
|
||
Comment on attachment 8999224 [details] [diff] [review] Part 3: Remove some dead code Review of attachment 8999224 [details] [diff] [review]: ----------------------------------------------------------------- I'm assuming this is actually dead. I'm sure the compiler will say if it isn't :)
Attachment #8999224 -
Flags: review?(hurley) → review+
Assignee | ||
Comment 17•6 years ago
|
||
(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #16) > Comment on attachment 8999224 [details] [diff] [review] > Part 3: Remove some dead code > > Review of attachment 8999224 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm assuming this is actually dead. I'm sure the compiler will say if it > isn't :) It's really dead. Moment of silence... :-)
Comment 18•6 years ago
|
||
Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/434ed946ad61 Part 1: Add the Cookie header to HTTP requests only after a potential classification has been completed on the channel to ensure the tracking state is up to date; r=nwgh https://hg.mozilla.org/integration/mozilla-inbound/rev/237c1635b0e9 Part 2: Add a test case to ensure that we don't send existing cookies in restricted third-party storage contexts; r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/acf7da1e9687 Part 3: Remove some dead code; r=nwgh
Comment 19•6 years ago
|
||
Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e8fc15190a98 Part 4: Update the test to use the new pref after the landing of bug 1480780
Comment 20•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/434ed946ad61 https://hg.mozilla.org/mozilla-central/rev/237c1635b0e9 https://hg.mozilla.org/mozilla-central/rev/acf7da1e9687 https://hg.mozilla.org/mozilla-central/rev/e8fc15190a98
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•6 years ago
|
Flags: qe-verify+
Updated•5 years ago
|
Flags: qe-verify+
Comment 21•5 years ago
|
||
I can confirm this issue is fixed. I verified using Fx 65.0.3 on Windows 10 x64, macOS 10.13.6 and Ubuntu 16.04 LTS.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•