Last Comment Bug 605296 - CORS XHR fails if username/password arguments are supplied as "undefined"
: CORS XHR fails if username/password arguments are supplied as "undefined"
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
: P1 normal (vote)
: mozilla2.0b8
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
Depends on:
Blocks: 609865
  Show dependency treegraph
 
Reported: 2010-10-18 15:14 PDT by Thomas Bachem
Modified: 2013-04-17 18:17 PDT (History)
13 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+


Attachments
Test Case (1.34 KB, text/html)
2010-10-18 15:15 PDT, Thomas Bachem
no flags Details
part 1. Infrastructure for supporting AUTF8String in quickstubs. (13.62 KB, patch)
2010-11-12 15:11 PST, Boris Zbarsky [:bz] (still a bit busy)
jorendorff: review+
Details | Diff | Splinter Review
part 2. Make username/password stringify undefined to empty string. (7.49 KB, patch)
2010-11-12 15:13 PST, Boris Zbarsky [:bz] (still a bit busy)
jonas: review+
Details | Diff | Splinter Review

Description Thomas Bachem 2010-10-18 15:14:23 PDT
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 2010-10-18 15:15:14 PDT
Created attachment 484119 [details]
Test Case
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2010-10-18 18:39:20 PDT
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 2010-10-18 23:12:24 PDT
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 [:bz] (still a bit busy) 2010-10-18 23:17:53 PDT
> 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 [:bz] (still a bit busy) 2010-10-18 23:20:43 PDT
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 2010-10-18 23:26:18 PDT
> 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) 2010-10-22 10:50:38 PDT
This is the way undefined behaves in almost all places. Is it really worth special casing it here?
Comment 8 Thomas Bachem 2010-10-22 10:54:19 PDT
But it worked in Firefox 3.6?
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2010-10-22 11:20:24 PDT
> 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 [:bz] (still a bit busy) 2010-11-11 23:04:28 PST
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 2010-11-11 23:18:38 PST
I guess we should go with the solution in comment 4 then.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2010-11-12 15:11:31 PST
Created attachment 490255 [details] [diff] [review]
part 1.  Infrastructure for supporting AUTF8String in quickstubs.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2010-11-12 15:13:28 PST
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 [:bz] (still a bit busy) 2010-11-12 15:13:45 PST
Created attachment 490257 [details] [diff] [review]
part 2.  Make username/password stringify undefined to empty string.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2010-11-12 15:15:51 PST
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 2010-11-12 15:39:24 PST
Comment on attachment 490257 [details] [diff] [review]
part 2.  Make username/password stringify undefined to empty string.

Nice!
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2010-11-13 19:23:42 PST
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, jst@mozilla.com) 2010-11-17 18:27:28 PST
jorendorff, review ping?
Comment 19 Jason Orendorff [:jorendorff] 2010-11-24 13:14:20 PST
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 [:bz] (still a bit busy) 2010-11-24 13:31:03 PST
>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 22 Dave Townsend [:mossop] 2010-12-01 11:05:26 PST
Backed out the patch for being in the range of bug 615736
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2010-12-02 06:18:21 PST
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 2013-04-17 17:39:02 PDT
(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 2013-04-17 17:41:54 PDT
Gah, ignore me. That's not XHR2, no idea how I got there. :(
Comment 26 Olli Pettay [:smaug] 2013-04-17 18:17:14 PDT
Note, the spec we try to implement is http://xhr.spec.whatwg.org/

Note You need to log in before you can comment on or make changes to this bug.