Closed Bug 1478539 Opened Last year Closed Last year

doubleclick.net cookies are added to subresource requests after visiting a Google property serving doubleclick.net ads

Categories

(Core :: Networking: HTTP, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox63 --- verified

People

(Reporter: Ehsan, Assigned: Ehsan)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-triaged])

Attachments

(3 files, 2 obsolete files)

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.  :-(
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.
I've been working on it since a few hours ago.  :-)
Assignee: nobody → ehsan
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)
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+
Flags: needinfo?(jduell.mcbugs)
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...
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 - Attachment is obsolete: true
Attachment #8995353 - Flags: review?(amarchesini) → review+
(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
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)
Attachment #8995661 - Attachment is obsolete: true
Attachment #8999224 - Flags: review?(hurley)
Attachment #8999223 - Flags: review?(hurley) → review+
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+
(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... :-)
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
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
Depends on: 1485182
Depends on: 1485673

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.