605296 - CORS XHR fails if username/password arguments are supplied as "undefined"

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Thomas Bachem]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6) Gecko/20100101 Firefox/4.0b6
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6) Gecko/20100101 Firefox/4.0b6

Firefox 4.0b7 has a different behaviour compared to 3.6 when it comes to the use of "undefined" as an argument to the XMLHttpRequest.open() method.

The Dojo Toolkit (tested using v1.5.0) contains the following line in its dojo.xhr() method:

---
   xhr.open(method, ioArgs.url, args.sync !== true, args.user || undefined, args.password || undefined);
---

This will always fail with a NS_ERROR_DOM_BAD_URI "Access to restricted URI denied" error if done cross-domain, as FF 4.0b7 doesn't ignore the "undefined" arguments.

Reproducible: Always

Steps to Reproduce:
See attached test case.
Actual Results:  
Works! - FAIL - Works!

Expected Results:  
Works! - Works! - Works!

Comment 1

[Thomas Bachem]

Attachment: Test Case

Comment 2

[Boris Zbarsky [:bzbarsky]]

Hmm.  We should just be ignoring username/password for cross-origin requests, not throwing, no?

Note that passing undefined is the same as passing the string "undefined" per http://www.w3.org/TR/XMLHttpRequest/#the-open-method step 15 and step 16.  So this code is actually trying to make the request with the username "undefined" and the password "undefined".  Anne, that seems ... suboptimal.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, the behavior for undefined is suboptimal, but it's something that we've inherited from ECMAScript so not much we can do.

Throwing is the right thing to do per spec. See
http://dev.w3.org/2006/webapi/XMLHttpRequest-2/Overview.html#the-open-method

Dojo should be changed to do

args.user || ""

instead. Unfortunately :(

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Throwing is the right thing to do per spec.

I assume step 13.1 and 14.1?

There's another option here; change the spec so that for this API undefined stringifies to "".  See http://www.w3.org/TR/WebIDL/#Undefined

Comment 5

[Boris Zbarsky [:bzbarsky]]

Then again, we apparently can't quickstub open() because it uses the JS stack (just for the IsCallerTrustedForRead crud?  Do we care?), so we wouldn't be able to easily implement that anyway...

But why is using the js stack a bar to being quickstubbed?

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> There's another option here; change the spec so that for this API undefined
> stringifies to "".  See http://www.w3.org/TR/WebIDL/#Undefined

I would be fine with that yes.

Anne?

Comment 7

[Anne (:annevk)]

This is the way undefined behaves in almost all places. Is it really worth special casing it here?

Comment 8

[Thomas Bachem]

But it worked in Firefox 3.6?

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Is it really worth special casing it here?

Given that the other option breaks a widely deployed and popular JS library?  Perhaps yes.

> But it worked in Firefox 3.6?

Firefox 3.6 doesn't implement the parts of the XHR2 spec dealing with username/password.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Jonas, I'm reopening this.  We have at least two more independent reports of this behavior causing probles with existing sites; see bug 609865.  I think the spec needs to change here, personally....

I won't reopen it again if you still think this is invalid, but in that case we need to document/evang/outreach, etc.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I guess we should go with the solution in comment 4 then.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Infrastructure for supporting AUTF8String in quickstubs.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 490255 [details] [diff] [review]
part 1.  Infrastructure for supporting AUTF8String in quickstubs.

Note that this keeps our old behavior, but this may or may not be compatible with what WebIDL calls for for the URI bit of XMLHttpRequest...  We can deal with that later, once there are test suites.

Jason, let me know if you'd really prefer the separate code to combining the strings together like this, but the additional copy/paste for utf8 string was really bothering me.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make username/password stringify undefined to empty string.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 490257 [details] [diff] [review]
part 2.  Make username/password stringify undefined to empty string.

This is sorta a minimal change needed to fix this.

This stuff is SO not per spec, by the way.  Nonempty passwords are ignored as long as username is empty (instead of throwing, say).  Empty string is allowed for both user and password and treated like the spec says to treat null (I think this should change in the spec: it should treat undefined, null, empty string as all the same for username+password, probably by having the prose talk about empty string and having the IDL specify that null/undefined become empty).

This should fix the dojo issue, though.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 490257 [details] [diff] [review]
part 2.  Make username/password stringify undefined to empty string.

Nice!

Comment 17

[Boris Zbarsky [:bzbarsky]]

Note that per comments in bug 609865 this is also an issue for mootools, not just dojo.  See also https://mootools.lighthouseapp.com/projects/2706/tickets/1086-mootools-13-requestjs-basic-http-authentication-firefox-4-beta-7

So yeah, I think the spec needs to change here, unless you expect every single site using mootools and dojo to get updated to fixed versions of those libraries before any UAs ship this aspect of the spec.

Comment 18

[Johnny Stenback (:jst)]

jorendorff, review ping?

Comment 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 490255 [details] [diff] [review]
part 1.  Infrastructure for supporting AUTF8String in quickstubs.

Nice patch.

In xpcquickstubs.h:
>+    template<class traits>
>+    JSString* TryStringifyValue(JSContext* cx, jsval v, jsval* pval,
>+                                StringificationBehavior nullBehavior,
>+                                StringificationBehavior undefinedBehavior,
>+                                JSBool* initComplete) {

I think this can just be an ordinary method, with
    typedef typename implementation_type::char_traits traits;
inside.

Also: The initComplete out-param isn't necessary; this can just return nsnull
to indicate that initialization is complete. Code calling this method can just
do:

        if (JSString *s = TryStringifyValue(cx, ...))
        {
            size_t len = s->length();
            ...blah blah...
            mValid = JS_TRUE;
        }

It could be called InitOrStringify.

>+                *initComplete = JS_TRUE;
>+                return s;
>+            }
>+
>+            s = JS_ValueToString(cx, v);

s is returned without having been initialized. The suggestion above would have
this return nsnull instead.

Comment 20

[Boris Zbarsky [:bzbarsky]]

>I think this can just be an ordinary method, with
>    typedef typename implementation_type::char_traits traits;
>inside.

It can't, actually.  For the UTF8 case, implementation_type is NS_ConvertUTF16toUTF8, which is a CString and hence has char_traits == nsCharTraits<char>.  But I need to use nsCharTraits<PRUnichar> in this method for that case.

I did try what you suggest first; it just doesn't compile.  ;)

> this can just return nsnull to indicate that initialization is complete.

Hmm.  And rely on the JS_ValueToString failure setting mValid to false, I guess?  I can do that.

> It could be called InitOrStringify.

Good idea.  Will do.

> s is returned without having been initialized

Good catch.  Yeah, I'll switch to null.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Pushed:

  http://hg.mozilla.org/mozilla-central/rev/ebaf0b1da9e6
  http://hg.mozilla.org/mozilla-central/rev/edf0c4a6a10c

and a followup to fix test orange:

  http://hg.mozilla.org/mozilla-central/rev/8777ed85c5ac

Comment 22

[Dave Townsend [:mossop]]

Backed out the patch for being in the range of bug 615736

Comment 23

[Boris Zbarsky [:bzbarsky]]

Relanded:

  http://hg.mozilla.org/mozilla-central/rev/43437276f9f1
  http://hg.mozilla.org/mozilla-central/rev/251cd89364a8

(rolled the orange fix into the second changeset).  Tree is green.

Comment 24

[Robert Kaiser]

(In reply to Jonas Sicking (:sicking) from comment #3)
> Throwing is the right thing to do per spec.

I realize this is an old bug, I just stumbled over it while investigating why username/password don't work with CORS and it looks like that throwing behavior has been removed from the spec, see http://www.w3.org/TR/XMLHttpRequest/#the-open%28%29-method - should we file a new bug on allowing it now? Or does someone from our side need to push for getting this back into the spec?

Comment 25

[Robert Kaiser]

Gah, ignore me. That's not XHR2, no idea how I got there. :(

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Note, the spec we try to implement is http://xhr.spec.whatwg.org/