XMLHttpRequest with mozAnon=true should still send explicitly passed username + password

RESOLVED FIXED in mozilla19

Status

()

Core
Security
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: philikon, Assigned: mayhemer)

Tracking

Other Branch
mozilla19
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:-)

Details

(Whiteboard: [WebAPI:P3][LOE:M])

Attachments

(1 attachment, 3 obsolete attachments)

In bug 692677 we added the mozAnon flag to XHR which tells the User Agent not to pass cookies and other auth information along with the request. We use the nsIRequest::LOAD_ANONYMOUS flag for that. However, an explicit username and password as passed into the xhr.open() method isn't sent along either, but it really should.
This is going to be very important for all the protocols the email/calendar app intend to use with this api.
blocking-basecamp: --- → +
blocking-kilimanjaro: --- → +
Alright, I think I finally have a grasp of what's going on:

(1) The username, password parameters are equivalent to using http://username:password/... in the URI, as is evident from nsXMLHttpRequest::Open(): https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1888

(2) The URI credentials are used in nsHttpChannelAuthProvider::SetAuthorizationHeader(): https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#1199

(3) However, that is skipped if the channel has the nsIRequest::LOAD_ANONYMOUS load flag set: https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#176

While I don't feel inclined to mess with Necko too much, it seems error-prone to manually set the 'Authorization' header in nsXMLHttpRequest or something like that. My suggestion would be to invent a new flag, e.g. nsIRequest::LOAD_ANONYMOUS_WITH_URI_IDENTITY that we can check for in nsHttpChannelAuthProvider::AddAuthorizationHeaders().

Thoughts?
Assignee: nobody → philipp
CCing bz for some guidance.
What's the actual desired behavior here?  Why are we trying to send some kinds of authentication information but not others?
(In reply to Boris Zbarsky (:bz) from comment #4)
> What's the actual desired behavior here?  Why are we trying to send some
> kinds of authentication information but not others?

Basically, yes. The behaviour that we expect (Sicking and I, at least) would be that for the following request

  xhr = new XmlHttpRequest({mozAnon: true});
  xhr.open("GET", url, true, username, password)

the server would still receive the explicitly passed username and password values, whereas any username + password values from the auth cache as well as cookies would not be sent along.
Would user certificates be sent?  What about other authentication mechanisms that XHR doesn't really expose at the moment but might in the future?
(In reply to Boris Zbarsky (:bz) from comment #6)
> Would user certificates be sent?

I would not expect them to be sent. But I do not know whether they will. I should check and maybe also write a unit test for this.

> What about other authentication mechanisms
> that XHR doesn't really expose at the moment but might in the future?

If they're explicitly set on the XHR, I would expect them to be sent. AFAIUI, mozAnon = true tells the User Agent something like "don't add anything *you* know about this host from previous encounters, but let *me* add stuff explicitly".
Yeah, then it seems like we need a new request flag value to indicate that...

Comment 9

6 years ago
How are we doing here?

Updated

6 years ago
Priority: -- → P1
Whiteboard: [WebAPI:P3]
(Reporter)

Updated

6 years ago
Whiteboard: [WebAPI:P3] → [WebAPI:P3][LOE:M]
Assignee: philipp → amarchesini
Removing this as a blocker. If someone thinks it should be a blocker, please let me know exactly how we are planning on using this in a v1 app.

As far as I can tell we wouldn't be using this for the email app since it's not using XHR at all. And I'm not aware that calendar stuff is commonly behind http-auth based security.
blocking-basecamp: + → -
Created attachment 661767 [details] [diff] [review]
patch 1

I don't know if this is the right way to fix it.
Attachment #661767 - Flags: review?(bzbarsky)
Comment on attachment 661767 [details] [diff] [review]
patch 1

This is so not my thing.  Let's try Honza, I guess?
Attachment #661767 - Flags: review?(bzbarsky) → review?(honzab.moz)
(Assignee)

Comment 13

6 years ago
Comment on attachment 661767 [details] [diff] [review]
patch 1

This is a first sight feedback only:
- nsHttpChannelAuthProvider::GetCredentialsFromURI duplicates some code we already have
- this probably won't work for NTLM or any state-full auth scheme

I'll do a proper review, latest the next week.  

If you have a new version of the patch, please don't obsolete this one (it would kill my splinter comments).  Thanks.
(Assignee)

Comment 14

6 years ago
How urgent is this bug to get fixed?
(Assignee)

Comment 15

6 years ago
Created attachment 667192 [details] [diff] [review]
WIP1

A bit different approach.  Basic tests are passing but this one needs a new, more specific test yet.
can you use the mochitest from my patch?
(Assignee)

Comment 17

6 years ago
(In reply to Andrea Marchesini (:baku) from comment #16)
> can you use the mochitest from my patch?

I did, my patch didn't pass (got stuck on a prompt).  I'll look after the cause.
(Assignee)

Comment 18

6 years ago
(In reply to Honza Bambas (:mayhemer) from comment #17)
> (In reply to Andrea Marchesini (:baku) from comment #16)
> > can you use the mochitest from my patch?
> 
> I did, my patch didn't pass (got stuck on a prompt).  I'll look after the
> cause.

So, I retried and now it passes (maybe it was just a rebuild problem).  I pushed the patch to try as well along with patch for bug 776171:

https://tbpl.mozilla.org/?tree=Try&rev=3c489d152066

We'll see..
(Assignee)

Comment 19

6 years ago
Created attachment 669529 [details] [diff] [review]
v2 + test by Andrea Marchesini

- check for the ANON flag moved down to nsHttpChannelAuthProvider::GetCredentialsForChallenge
- includes a test from Andrea that I r+'ed
Assignee: amarchesini → honzab.moz
Attachment #661767 - Attachment is obsolete: true
Attachment #667192 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #661767 - Flags: review?(honzab.moz)
Attachment #669529 - Flags: review?(cbiesinger)
(Assignee)

Comment 20

6 years ago
Comment on attachment 669529 [details] [diff] [review]
v2 + test by Andrea Marchesini

r=honzab for the test parts.
Attachment #669529 - Flags: review+
Attachment #669529 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 22

6 years ago
Created attachment 673901 [details] [diff] [review]
v3 + updated test originally by Andrea Marchesini

I found an issue with my patch.  When there are cached credentials with the same username but different password then those are prioritized over the credentials in the URL.  When LOAD_ANONYMOUS is set we have to bypass this cache override to prevent leak.  Though, we still need to use cache here to make state-full schemes work (NTLM and Digest).

This is an updated patch, changes:
- the "taking identity from auth cache" block is not executed also when LOAD_ANONYMOUS is engaged
- test updated to include check for this code path
Attachment #669529 - Attachment is obsolete: true
Attachment #673901 - Flags: review?(cbiesinger)
Attachment #673901 - Flags: review?(cbiesinger) → review+
https://hg.mozilla.org/mozilla-central/rev/d1a526f4d7b8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

5 years ago
Depends on: 856978

Updated

5 years ago
No longer depends on: 856978
You need to log in before you can comment on or make changes to this bug.