528292 - XHR should observe forceAllowThirdPartyCookie across redirected requests

Status: RESOLVED FIXED

(Core :: Networking: File, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

When an XHR follows an HTTP 302 redirect, the second request ignores nsIHttpChannelInternal::forceAllowThirdPartyCookie.  It should observe that flag and behave accordingly, so chrome code can correctly load resources that require cookies across a redirect.

An example of this is found in the Personas extension, which uses XHR to load http://www.getpersonas.com/gallery/All/Favorites?json=1, setting forceAllowThirdPartyCookie so the user's auth cookies are sent with the request.  The server redirects that request to a locale-specific URL, in my case http://www.getpersonas.com/en-US/gallery/All/Favorites?json=1, and XHR then loads that URL, but it doesn't include the Cookie header with it (in my tests using Live HTTP Headers to introspect the requests).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Ah, yeah.  nsHttpChannel::SetUpReplacementChannel should copy it over.  Myk, want to patch?

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: seems to fix bug

Sure, I can do that.

Here's a patch that appears to fix the problem in my manual tests against the Personas server.  I've included an xpcshell unit test in the patch, but for some reason nsIHTTPChannelInternal::forceAllowThirdPartyCookie doesn't seem to work at all in the test, not even for the non-redirect case (although it works fine if I enable third-party cookies), so the test doesn't actually test the functionality at this point.

I'm not sure what's wrong with it; perhaps something simple I'm missing?

Comment 3

[Myk Melez [:myk] [@mykmelez]]

s/although it works fine/although the test works fine/

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 412171 [details] [diff] [review]
patch v1: seems to fix bug

Sorry for the lag here; I put it in the "figure out what's going on here" bin, and that got a bit full...

In the test, you set the cookie for the pre-redirect URI, no?  So I wouldn't expect the cookie to be there on the post-redirect channel.  You need to set it for the post-redirect URI.

With that, looks good.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: sets cookie on post-redirect URI

Here's a patch whose test sets the cookie on the post-redirect URI.  Unfortunately, however, the test continues to fail, and I still don't see why.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm.  Odd.  Does it pas if you don't change the pref?  Is postRedirectHandler getting called?

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #6)
> Hmm.  Odd.  Does it pas if you don't change the pref?  Is postRedirectHandler
> getting called?

Yes, it passes if I don't change the pref (or I explicitly set it to "0"). postRedirectHandler is definitely getting called, as setting receivedCookieVal to "foo=bar" (the value of the cookie) instead of metadata.getHeader("Cookie") inside that function also causes the test to pass.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Hmm.  If you add a printf to the C++ at your change point, does that get hit?  What's mForceAllowThirdPartyCookie at that point?

Comment 9

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #8)
> Hmm.  If you add a printf to the C++ at your change point, does that get hit? 
> What's mForceAllowThirdPartyCookie at that point?

Yes, it gets hit, and mForceAllowThirdPartyCookie is true, as it should be.

In further testing, metadata.getHeader("Cookie") throws NS_ERROR_NOT_AVAILABLE in both preRedirectHandler and postRedirectHandler when third-party cookies are disabled, while it returns "foo=bar" in both handlers when they're enabled.

That suggests the problem is not in my change point but in whatever code decides whether or not to send the cookie in the first place, for the original channel.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Huh.  Odd.  Can you take a look at what happens in nsCookieService::CheckPrefs down in the "mCookiesPermissions == BEHAVIOR_REJECTFOREIGN" block?

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: with a working test

Instrumenting nsCookieService::CheckPrefs revealed that it was getting called three times, and it was the first call that was failing.

It turns out that nsCookieService::SetCookieString, which the test calls to set the cookie before trying the load, entrains a call to CheckPrefs, which depends on a call to nsCookiePermission::GetOriginatingURI, which fails because I didn't pass SetCookieString a channel, and GetOriginatingURI needs a channel (and, furthermore, the channel has to either have forceAllowThirdPartyCookie set or be associated with a window from which the originating URI can be derived).

The XPIDL description of SetCookieString says the channel parameter "may be null, but it is strongly recommended that a non-null value be provided to ensure that the cookie privacy preferences are honored," which I interpreted to mean that passing a null channel would bypass those privacy preferences, but I guess that's not the case.

I also assumed that SetCookieString was succeeding since it didn't throw, although it turns out that it signals failure by broadcasting a "cookie-rejected" notification.

That documentation and behavior seems a bit funky, but in any case the fix for this test is to pass SetCookieString a channel with forceAllowThirdPartyCookie set to true so that the cookie string really does get set.  Here's the updated patch.

Comment 12

[[e-mail]]

(In reply to comment #11)
> The XPIDL description of SetCookieString says the channel parameter "may be
> null, but it is strongly recommended that a non-null value be provided to
> ensure that the cookie privacy preferences are honored," which I interpreted to
> mean that passing a null channel would bypass those privacy preferences, but I
> guess that's not the case.

You're right, that should be more explicit. Patches accepted, or I can roll one once I get back into work.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 416736 [details] [diff] [review]
patch v3: with a working test

Excellent.  Thanks for digging into this!

Feel free to update the cookie docs as desired.  rs=me

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Committed in changeset http://hg.mozilla.org/mozilla-central/rev/961c37d6fc04.

I'm struggling to come up with better verbiage to describe the channel parameter to SetCookieString and SetCookieStringFromHTTP, however.

Digging through the cookie code, it looks like the channel doesn't actually make much difference at all.  nsCookieService::CheckPrefs doesn't use it, nor does nsPermissionService::CanAccess.

nsPermissionService::CanSetCookie uses it to get the parent window, which it then passes to nsCookiePromptService::CookieDialog, but that method simply uses the active window if the parent window isn't available.

In fact, it looks like the only time the channel matters is when the browser is configured to reject third-party cookies and the site hosting the given URI is not on the permission list.  In that case, the cookie service needs the channel to get the originating URI to determine whether or not this is a third-party cookie.  Without a channel, it simply fails (which is the problem that vexed the earlier patches for this bug).

So here's a possible update to the comment:

   * @param aChannel
   *        the channel used to load the document.  this parameter may be null,
   *        but it is strongly recommended that a non-null value be provided to
   *        ensure that the cookie privacy preferences are honored.  otherwise,
   *        the call might not succeed (f.e. if third-party cookies are
   *        disabled, and the cookie service can't determine whether or not
   *        this is a third-party cookie because it doesn't have the channel
   *        from which to get the originating URI).

But I wanted to check with y'all before checking this in because I wasn't sure if I was missing something about the way this parameter is being used.

dwitte: how does this update to the comments for SetCookieString and SetCookieStringFromHTTP sound to you?

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Erm, marking this fixed, as I had intended to do after committing the patch.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

This affects the Personas extension's "My Favorites" feature when third-party cookies are disabled, and it may well affect other extensions too (any extension that uses forceAllowThirdPartyCookie could be affected if the website from which they request a resource redirects the request).

Granted, third-party cookies are enabled by default, so this isn't a blocker, but it would be good to have it fixed in Firefox 3.6, so requesting wanted-1.9.2.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 416736 [details] [diff] [review]
patch v3: with a working test

Requesting approval to land this one-line fix (plus many-line test) on 1.9.2.  The fix addresses a bug in the implementation of the newish forceAllowThirdPartyCookie boolean attribute of nsIHTTPChannelInternal that caused that flag not to be conveyed across redirects.

In other words, if chrome code loaded http://example.com/page, and example.com redirected the request to http://example.com/en-US/page, then cookies for example.com would be sent with the first request but not with the second if third-party cookies were disabled, even if the chrome code had set forceAllowThirdPartyCookie to true.  With the fix, cookies for example.com are sent with both requests.

The fix landed on the trunk on Sunday, December 13 around noon and has caused no known regressions.

Comment 18

[[e-mail]]

(In reply to comment #14)
> But I wanted to check with y'all before checking this in because I wasn't sure
> if I was missing something about the way this parameter is being used.

Your analysis is right, this is the only case we need to worry about.

> dwitte: how does this update to the comments for SetCookieString and
> SetCookieStringFromHTTP sound to you?

I'd be a bit stronger with the language, to reduce the chance the casual reader misses it: maybe something like

* @param aChannel
*        the channel used to load the document.  this parameter should not be null, otherwise the cookie will not be set if third-party cookies have been disabled by the user. (the channel is used to determine the originating URI of the document; if it is not provided, the cookie will be assumed third-party.)

I think we might also need a variant of this comment for getCookieString{FromHttp}, right? Please land the comment change on 1.9.2 also, if you can; no need for approval since this is NPOB?

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Attachment: followup patch w/comment changes

(In reply to comment #18)
> I'd be a bit stronger with the language, to reduce the chance the casual
> reader misses it: maybe something like
> 
> * @param aChannel
> *        the channel used to load the document.  this parameter should not be
> null, otherwise the cookie will not be set if third-party cookies have been
> disabled by the user. (the channel is used to determine the originating URI of
> the document; if it is not provided, the cookie will be assumed third-party.)

Sounds good, this patch uses that language, except that it pluralizes "cookie" since the provided string can contain multiple cookies.


> I think we might also need a variant of this comment for
> getCookieString{FromHttp}, right?

Yup, this patch does that too.


> Please land the comment change on 1.9.2 also, if you can; no need for
> approval since this is NPOB?

That's a reasonable interpretation, of which Sheriff Gavin approved on IRC, so I'll go ahead and do that.

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Comment-only change landed on trunk and 3.6 branch:

http://hg.mozilla.org/mozilla-central/rev/b78d999d215d
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/46b278942b94

Comment 21

[[e-mail]]

Awesome, thanks!

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 416736 [details] [diff] [review]
patch v3: with a working test

Moving approval flag to 1.9.2.3 - do we need a new patch since the comment parts have already landed?

Comment 23

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #22)
> (From update of attachment 416736 [details] [diff] [review])
> Moving approval flag to 1.9.2.3 - do we need a new patch since the comment
> parts have already landed?

Nope, the comment parts are a separate patch (attachment 417583 [details] [diff] [review]) for both trunk and branch, so this patch doesn't need to be updated.  I also just confirmed that there have been no other changes on the branch that would necessitate a new patch:

myk@myk:~/Mozilla/source-1.9.2$ hg pull -u
...
myk@myk:~/Mozilla/source-1.9.2$ patch -p1 --dry-run < /home/myk/528292v3.diff 
patching file netwerk/protocol/http/src/nsHttpChannel.cpp
Hunk #1 succeeded at 2831 (offset 2 lines).
patching file netwerk/test/unit/test_bug528292.js

So this patch is ready to go once approved.

Comment 24

[LegNeato]

Comment on attachment 416736 [details] [diff] [review]
patch v3: with a working test

a=LegNeato for 1.9.2.5. Please ONLY land this on mozilla-1.9.2 default, as we
are still working on 1.9.2.4 on the relbranch

Comment 25

[Myk Melez [:myk] [@mykmelez]]

Landed on mozilla-1.9.2 default only: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/18aa7476e6de.