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

RESOLVED FIXED in mozilla2.0b8

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Thomas Bachem, Assigned: bz)

Tracking

({regression})

unspecified
mozilla2.0b8
x86
Mac OS X
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(3 attachments)

(Reporter)

Description

7 years ago
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!
(Reporter)

Comment 1

7 years ago
Created attachment 484119 [details]
Test Case
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.
Status: UNCONFIRMED → NEW
blocking2.0: --- → ?
Ever confirmed: true
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 :(
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → INVALID
> 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
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?
> 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

7 years ago
This is the way undefined behaves in almost all places. Is it really worth special casing it here?
(Reporter)

Comment 8

7 years ago
But it worked in Firefox 3.6?
> 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.
Blocks: 609865
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.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
I guess we should go with the solution in comment 4 then.
Created attachment 490255 [details] [diff] [review]
part 1.  Infrastructure for supporting AUTF8String in quickstubs.
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.
Attachment #490255 - Flags: review?(jorendorff)
Created attachment 490257 [details] [diff] [review]
part 2.  Make username/password stringify undefined to empty string.
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.
Attachment #490257 - Flags: review?(jonas)
Assignee: nobody → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
Comment on attachment 490257 [details] [diff] [review]
part 2.  Make username/password stringify undefined to empty string.

Nice!
Attachment #490257 - Flags: review?(jonas) → review+
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.
jorendorff, review ping?
blocking2.0: ? → betaN+

Updated

7 years ago
Keywords: regression
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.
Attachment #490255 - Flags: review?(jorendorff) → review+
>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.
Whiteboard: [need review] → [need landing]
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
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [need landing]
Target Milestone: --- → mozilla2.0b8
Backed out the patch for being in the range of bug 615736
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [need landing]
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.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [need landing]
Component: DOM: Other → DOM
Product: Core → Core

Comment 24

4 years ago
(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

4 years ago
Gah, ignore me. That's not XHR2, no idea how I got there. :(
Note, the spec we try to implement is http://xhr.spec.whatwg.org/
You need to log in before you can comment on or make changes to this bug.