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

Status: VERIFIED FIXED

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

Description

[(no longer active)]

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.  :-(

Comment 1

[(no longer active)]

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.

Comment 2

[u408661]

This seems relatively important, but not 100% sure how quickly someone needs to jump on this. ni?jduell so he can figure that out.

Comment 3

[(no longer active)]

I've been working on it since a few hours ago.  :-)

Comment 4

[(no longer active)]

Attachment: 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

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.

Comment 5

[(no longer active)]

Attachment: Part 2: Add a test case to ensure that we don't send existing cookies in restricted third-party storage contexts

Comment 6

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a595fe8141cf1bd2fc9a55289e7377262121822

Comment 7

[u408661]

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.

Comment 8

[(no longer active)]

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...

Comment 9

[(no longer active)]

Attachment: 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

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.

Comment 10

[(no longer active)]

(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.

Comment 11

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6073d5eca60beaa1b2aa23cbfb4d8194dcd597c

Comment 12

[(no longer active)]

With an extra bug fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ad8f59e6bd272d06987ba296f9ee79a1b7232cc

Comment 13

[(no longer active)]

Attachment: 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

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.

Comment 14

[(no longer active)]

Attachment: Part 3: Remove some dead code

Comment 15

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4517acadeba0ffbd12d79b811354e80fd358c75a

Comment 16

[u408661]

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 :)

Comment 17

[(no longer active)]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Pulsebot]

Pushed by [e-mail]:
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

[Eliza Balazs [:ebalazs_]]

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

Comment 21

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

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.