Closed Bug 1064378 Opened 6 years ago Closed 4 years ago

XMLHttpRequest Authorization header overwritten by cache

Categories

(Core :: General, defect)

32 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: ray.navarette, Assigned: mayhemer)

References

Details

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140825202822

Steps to reproduce:

I'm submitting an AJAX request via AngularJS $http method.  This method sets the Authorization header using values entered from a form.  When the user has not entered these credentials manually in the browser basic auth dialog, this works fine.

However, if the user has entered credentials into the browsers authentication dialog, these entered credentials overwrite those manually set on the request.


Actual results:

The browser overwrites the Authorization header with a cached value.


Expected results:

The browser should respect the value configured for the request.
Flags: needinfo?(honzab.moz)
Yes, we definitely overwrite any added Authorization headers pre-set on a channel.  We can add some simple logic to leave it and only "add cached credentials" when first auth attempt failed.  I think it makes sense, we do something similar for custom cached conditional request headers.
Assignee: nobody → honzab.moz
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(honzab.moz)
Thanks for looking at this.  I'm just wondering, why would you ever want to overwrite the manually set header?  I would think that if the app developer (JS code) was setting the header, that is the value that should be used, no matter what the cache contains.
Hi there, having the same issue (using AngularJS, too). Agree with Ray, it's really strange that browser overwrites header that is set by application. AFAIK other browsers don't do that.
I think I'm seeing this problem on a staging site that uses Token auth for XHR, but also a layer of Basic auth for the entire site. On Chrome, it works, but on Firefox, the Token auth header gets clobbered with the Basic auth one.

Is there any workaround for this, other than avoiding Basic auth?
I'm affected by the same issue. Would love to see it resolved
Same here. I' working on an angular application that used to rely on basic authentication header and now is using a Bearer token added programmatically. Instead of keeping the explicitly defined header it keeps using an old value from cache and of course turns to old "Basic" authentications. 

I can only advice users to clean caches when using firefox. Or, i'll have to implement other ways of sending the Bearer token (form field etc.). Both options are bad.
Attached patch v1 (obsolete) — Splinter Review
- we check for Authorization http header in AsyncOpen, remember it's present
- when present, we don't add cached credentials in AddAuthorizationHeaders
- when 401 is hit, we simply fail (do not try cached creds or prompt user) ; this is IMO simplest and also most desired behavior for when a custom header is set on the channel

https://treeherder.mozilla.org/#/jobs?repo=try&revision=722486d3cf83
Attachment #8708411 - Flags: review?(jduell.mcbugs)
(In reply to Honza Bambas (:mayhemer) from comment #7)
> Created attachment 8708411 [details] [diff] [review]
> v1

> https://treeherder.mozilla.org/#/jobs?repo=try&revision=722486d3cf83

TEST-UNEXPECTED-PASS | /XMLHttpRequest/send-authentication-basic-setrequestheader-existing-session.htm | XMLHttpRequest: send() - "Basic" authenticated request using setRequestHeader() when there is an existing session - expected FAIL
TEST-UNEXPECTED-PASS | /XMLHttpRequest/send-authentication-basic.htm | XMLHttpRequest: send() - "Basic" authenticated requests with user name and password passed to open() - expected FAIL
PROCESS | 284 | [Child 4084] ###!!! ABORT: Aborting on channel error.: file c:/builds/moz2_slave/try-w32-0000000000000000000000/build/src/ipc/glue/MessageChannel.cpp, line 1762
Return code: 1
So, seems like this patch breaks web-platform tests (it's expected to take cached credentials instead of what has been set on XHR manually.)

James, as you are the one who imported the tests, do you think this bug should be closed as WONTFIX?  

However, for me this bug and the behavior suggested here makes more sense.  Also looking at the reasonable comments in this bug, it seems developers expect what's suggested here.

Should we propose a spec change on W3C side?  Are there any security implications?  I somehow sense the current behavior is more dangerous than the suggested one, since we leak the cached credentials.
Flags: needinfo?(james)
Did some more tests with Edge and Chrome Dev using [1]:
Edge - test FAILS
Chrome - test PASS

I cannot find any docs on whether this test is expected to pass or fail, but I presume it is written to pass.

Based on that, I propose to go with this bug.


[1] https://w3c-test.org/XMLHttpRequest/send-authentication-basic-setrequestheader-existing-session.htm
Attached patch v1.1 (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d99db7bdc4de
Attachment #8708411 - Attachment is obsolete: true
Attachment #8708411 - Flags: review?(jduell.mcbugs)
Attachment #8708579 - Flags: review?(jduell.mcbugs)
I feel like annevk is a more useful person to talk to about this.
Flags: needinfo?(james) → needinfo?(annevk)
Hallvord created the test in question. It is certainly possible https://xhr.spec.whatwg.org/ and https://fetch.spec.whatwg.org/ need changes around this and the currently specified behavior is not exactly great, although it does not match this test either as I think per the specification you'd end up with two authorization headers (assuming this is same-origin).

I like what you suggest in comment 7 though how do you account for credentials found in the URL or set through XMLHttpRequest's open() method? Do they also have a precedence dance with the Authorization header?
Flags: needinfo?(annevk) → needinfo?(hsteen)
(In reply to Anne (:annevk) from comment #13)
> Hallvord created the test in question. It is certainly possible
> https://xhr.spec.whatwg.org/ and https://fetch.spec.whatwg.org/ need changes
> around this and the currently specified behavior is not exactly great,
> although it does not match this test either as I think per the specification
> you'd end up with two authorization headers (assuming this is same-origin).

How can you end up with two auth headers?  Is that something that comes out of the spec?  Note that this is not possible in Gecko.

The Gecko impl is built on experience with how web servers behave, to maximize compatibility (old times way).

> 
> I like what you suggest in comment 7 though how do you account for
> credentials found in the URL or set through XMLHttpRequest's open() method?
> Do they also have a precedence dance with the Authorization header?

Since the auth header itself is a result of either URL added creds or those added via open(), having the header set on the channel directly has the absolute priority.  That seems to me to be the most logical from the process point of view.  I think we just have to specify this somewhere to make it official.  I believe there will be exceptions on how developers expect the priority be here, but we have to do it somehow.

The order is, with my patch: Authorization request header overrides XHR.open() creds override (actually rewrites) URL creds override cached creds.  If all of it fails (or, obviously, is missing on the first 401/7), we ask the user.  On 200 we cache the used creds.
Well, TEST-UNEXPECTED-PASS results imply that your changes made a test that used to fail pass - so it sounds like you're doing a good job here :D
These tests should be written to conform carefully with the spec but it's entirely possible that I have failed to consider whether the spec prose as-is might result in an implementation sending *two* auth headers..

I'm trying to understand what the spec *actually* says. Since this behaviour basically spans three different specs (XHR, URL, Fetch) it's complicated.. :-/

XHR passes username and password to URL.

When it gets to fetching, as far as I can tell, we end up here:
https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch

> If there's an authentication entry for httpRequest and either httpRequest's 
> use-URL-credentials flag is unset or httpRequest's current url does not 
> include credentials, set authorizationValue to authentication entry.

So "authentication entry" (cached credentials) is used if 
 - use-URL-credentials is unset
or 
 - the URL doesn't have credentials

At least when invoked by XHR those two conditions express the same thing: use-URL-credentials is set if the URL has credentials.

So per spec, XHR's open() arguments *must* override any cached credentials. So, I believe, will user:pass@ string in a URL passed to open().

I think the specs collectively need to fix these issues:

* I *think* it's not clear based on the specs if user:pass@ or method arguments "win" when both are provided. We have a test expecting open() arguments to win if these include a value for username (send-authentication-competing-names-passwords.htm).

* There's nothing in "HTTP or cache fetch" step 11 that ensures there will only be one Authorization header. One option (which seems reasonable) is to update the text to "set" the header instead of "append"ing it to the headers list. This will make sure we don't send two headers if a script has created one with setRequestHeader(). 


Doing so also defines that user/pass in URLs or as arguments to open() override a setRequestHeader()-provided value - we have a test called send-authentication-basic-setrequestheader-existing-session.htm which asserts the opposite for the similar case of cached credentials but it fails on some browsers (Firefox, Safari). We don't have a test for the case that simultaneously uses setRequestHeader() and passes user,pass to open(). I reported web-platform-tests issue 2502 on adding such a test.
* Come to think of it, what should happen if a script does setRequestHeader('Authorization', ..), it's a CORS request and .withCredentials is false? Should the implementation remove Authorization header, terminate the request, or let it pass through? Issue 2503 on testing that.

To sum up: while there are some corner cases to handle the spec is more explicit than I thought at first sight, it seems the tests are pretty well aligned and the changes in this bug takes us in the right direction. Life's good :)
Flags: needinfo?(hsteen)
(And it's *especially* good that per this bug the spec'ed approach supports existing content and isn't likely to break the web!)
Hallvord, I think the behavior Honza advocates is that if you set a "custom" Authorization header, that wins from everything else. So I think what we want to do instead is terminate 11 early with a new step 2 or so that checks if header list already contains an Authorization header. I filed https://github.com/whatwg/fetch/issues/198 on that.
Note, this will affect fetch() in addition to XHR.  We should test both paths.
Duplicate of this bug: 1240755
Per the test coming in here https://github.com/w3c/web-platform-tests/commit/d70588e5a49d what Honza is suggesting is what browsers already do (right now only tested in Firefox and Safari on a Mac though) so +1 :) Makes it a bit harder to spec step 11 I guess.. "Set an Authorize header if it does not already exist in the headers list"?
(In reply to Hallvord R. M. Steen [:hallvors] from comment #15)
> Well, TEST-UNEXPECTED-PASS results imply that your changes made a test that
> used to fail pass - so it sounds like you're doing a good job here :D
> These tests should be written to conform carefully with the spec but it's
> entirely possible that I have failed to consider whether the spec prose
> as-is might result in an implementation sending *two* auth headers..

I think that it's something we never should do - to send more than just a single Authorization header.  I don't think there are servers capable of handling more than one.  Though I was not able to find a definitive answer to this, I believe there can be just one Authorization request header.

> 
> I'm trying to understand what the spec *actually* says. Since this behaviour
> basically spans three different specs (XHR, URL, Fetch) it's complicated..
> :-/
> 
> XHR passes username and password to URL.

Yes, and that is what current Gecko implementation does too.

> 
> When it gets to fetching, as far as I can tell, we end up here:
> https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch
> 
> > If there's an authentication entry for httpRequest and either httpRequest's 
> > use-URL-credentials flag is unset or httpRequest's current url does not 
> > include credentials, set authorizationValue to authentication entry.
> 
> So "authentication entry" (cached credentials) is used if 
>  - use-URL-credentials is unset
> or 
>  - the URL doesn't have credentials
> 
> At least when invoked by XHR those two conditions express the same thing:
> use-URL-credentials is set if the URL has credentials.
> 
> So per spec, XHR's open() arguments *must* override any cached credentials.
> So, I believe, will user:pass@ string in a URL passed to open().

Yes, and that is what we do as well.

> 
> I think the specs collectively need to fix these issues:
> 
> * I *think* it's not clear based on the specs if user:pass@ or method
> arguments "win" when both are provided. We have a test expecting open()
> arguments to win if these include a value for username
> (send-authentication-competing-names-passwords.htm).

open() _replaces_ user:pass@ in the URL in Gecko.

> 
> * There's nothing in "HTTP or cache fetch" step 11 that ensures there will
> only be one Authorization header. One option (which seems reasonable) is to
> update the text to "set" the header instead of "append"ing it to the headers
> list. This will make sure we don't send two headers if a script has created
> one with setRequestHeader(). 

Yes (change to "set") since that is what Gecko does and works for years.

> 
> 
> Doing so also defines that user/pass in URLs or as arguments to open()
> override a setRequestHeader()-provided value 

OK, that would make the patch for this bug a bit more complicated, but it's doable.
Based on what you say the priority would be:

- open("user", "pass")
- user:pass@
- setRequestHeader("Authorization", "...")
- session cached credentials
- user input

Is this really something we should end up with??

> - we have a test called
> send-authentication-basic-setrequestheader-existing-session.htm which
> asserts the opposite for the similar case of cached credentials but it fails
> on some browsers (Firefox, Safari). 

Hmm.. so that expects this:
- setRequestHeader("Authorization", "...")
- session cached credentials
- user input

and if open or URL creds involved it would be:
- setRequestHeader("Authorization", "...")
- open("user", "pass")
- user:pass@
- session cached credentials
- user input

That is what this patch v1 exactly leads to!

> We don't have a test for the case that
> simultaneously uses setRequestHeader() and passes user,pass to open(). I
> reported web-platform-tests issue 2502 on adding such a test.
> * Come to think of it, what should happen if a script does
> setRequestHeader('Authorization', ..), it's a CORS request and
> .withCredentials is false? 

I'm not 100% sure what we do in Gecko.  I *think* (bug 654348) we send URL credentials out.

> Should the implementation remove Authorization
> header, terminate the request, or let it pass through? Issue 2503 on testing
> that.

Hard to say.  But I can imagine developers may use some combination of setHeader("Authorization") and .withCredentials = false to bypass session creds cache of the browser.  That is IMO hack and we should provide better ways to achieve that.  Like no need to set .withCredentials = false and just use the header and not check for cache (nor prompt the user).  That is what mt patch v1 does.  Can send() throw when there is some inappropriate combination of properties?

> 
> To sum up: while there are some corner cases to handle the spec is more
> explicit than I thought at first sight, it seems the tests are pretty well
> aligned and the changes in this bug takes us in the right direction. Life's
> good :)

OK, so let's land it ;)


Thanks for this exhausting answer!
Comment on attachment 8708579 [details] [diff] [review]
v1.1

Review of attachment 8708579 [details] [diff] [review]:
-----------------------------------------------------------------

> Note, this will affect fetch() in addition to XHR.  We should test both paths.

Are we testing fetch()?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1591,5 @@
> +        if (MOZ_UNLIKELY(mCustomAuthHeader) && httpStatus == 401) {
> +            // When a custom auth header fails, we don't want to try
> +            // any cached credentials, nor we want to ask the user.
> +            // It's up to the consumer to re-try w/o setting a custom
> +            // auth hreader if cached credentials should be attempted.

header, not hreader

@@ +1596,5 @@
> +            rv = NS_ERROR_FAILURE;
> +        } else {
> +            rv = mAuthProvider->ProcessAuthentication(
> +                httpStatus, mConnectionInfo->EndToEndSSL() &&
> +                            mTransaction->ProxyConnectFailed());

weird indenting:  choose an indent level and use it for both lines

::: netwerk/protocol/http/nsHttpChannelAuthProvider.cpp
@@ +214,5 @@
>          return NS_OK;
>      }
>  
> +    if (aDontUseCachedWWWCreds) {
> +        LOG(("Adding a cached Authorization header prevented from above\n"));

prevented from above?

How about "Authorization header already present: skipping adding auth header from cache\n"?
Attachment #8708579 - Flags: review?(jduell.mcbugs) → review+
Note that even if we give precedent to a custom set "Authorization" header, there's nothing preventing someone from setting the "Authorization" header more than once. So developers can still shoot themselves in the foot (or implement some new scheme that requires multiple headers).
Argh. I agree with Honza the browser should only ever generate one Authorize header, but Anne is right - we have only two types of headers so far, one that can occur multiple times and one where values get comma-concatenated if it's set many times. We don't have any non-comma concatenated header that's guaranteed to only occur once in a HTTP request and I don't think the specific corner case of Authorize being set several times warrant special measures.

We should however probably have a test for that.. :-]

> * Come to think of it, what should happen if a script does
> setRequestHeader('Authorization', ..), it's a CORS request and
> .withCredentials is false? 

I did add a test for this, the header is just passed through if the CORS checks pass. It's reasonable.
(In reply to Anne (:annevk) from comment #23)
> Note that even if we give precedent to a custom set "Authorization" header,
> there's nothing preventing someone from setting the "Authorization" header
> more than once. So developers can still shoot themselves in the foot (or
> implement some new scheme that requires multiple headers).

I think goes beyond this bug.  Feel free to open a new bug for that.
Attached patch v1.2Splinter Review
(Just cosmetic changes, no need for another try run)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d99db7bdc4de
Attachment #8708579 - Attachment is obsolete: true
Attachment #8710542 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/075be35157dd
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.