Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add option to trigger xhr.withCredentials without http auth #2486

Merged
merged 4 commits into from

5 participants

@ccampbell

Firefox sends the username for auth even though a password was not supplied
which causes Firefox to abort before sending a preflight request for Cross-Origin
requests.

This pull request adds a withCredentials request option that allows you to set xhr.withCredentials without sending http authentication.

See http://stackoverflow.com/questions/11255173/firefox-wont-send-cross-origin-resource-sharing-pre-flight

@dwbuiten

Ping. It's been three months without so much as a reply.

@reednj

Its sad this hasn't been merged in. Such a small changed :/

If you need to support CORS and don't want to change the source, you can easily create a new class that supports withCredentials like so:

Request.CORS = new Class({
    Extends: Request.JSON,

    initialize: function(options) {
        this.parent(options);

        if(options && options.withCredentials === true && this.xhr && 'withCredentials' in this.xhr) {
            this.xhr.withCredentials = true;
        }
    }
});
@ibolmo ibolmo modified the milestone: 1.6, 1.5
@ibolmo ibolmo modified the milestone: 1.5.1, 1.6
@SergioCrisostomo
Collaborator

LGTM.

@arian @ibolmo Can I merge this with a docs update on the options with:

withCredentials - (boolean: defaults to false) allows you to set xhr.withCredentials without sending http authentication.

Source/Request/Request.js
@@ -197,7 +198,7 @@ var Request = this.Request = new Class({
}
xhr.open(method.toUpperCase(), url, this.options.async, this.options.user, this.options.password);
- if (this.options.user && 'withCredentials' in xhr) xhr.withCredentials = true;
+ if ((this.options.user || this.options.withCredentials) && 'withCredentials' in xhr) xhr.withCredentials = true;
@ibolmo Owner
ibolmo added a note

Why not just:

if (this.options.user && this.options.password && 'withCredentials' in xhr) ...

@ibolmo I think you are missing the point of this pull request. xhr.withCredentials does not require http authentication (as the current Mootools code suggests). You should be able to specify this flag without providing a username or password.

From the spec

The term user credentials for the purposes of this specification means cookies, HTTP authentication, and client-side SSL certificates. Specifically it does not refer to proxy authentication or the Origin header.

@ibolmo Owner
ibolmo added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ibolmo
Owner

After reading https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest#open()

Looks like we should deprecate the if (this.options.user and just have a withCredentials option. Could you modify your PR so that it reads:

 if ((/*<1.4compat>*/this.options.user || /*</1.4compat>*/this.options.withCredentials) && 'withCredentials' in xhr) xhr.withCredentials = true;

We'll also need a spec to ensure that:

  1. withCredentials is set for 1.4 compliance for this.options.user usage
  2. withCredentials is not set for 1.5 for this.options.user usage
  3. withCredentials is set for this.options.withCredentials is true
@ccampbell

@ibolmo should be good to go

@ibolmo
Owner

Awesome. Just waiting on Travis CI, but afterwards will merge.

@ibolmo
Owner

Ahh, and just nit picking but could you add a note in the Request.md documentation?

@ccampbell

Okay all good!

@SergioCrisostomo
Collaborator

@ccampbell awesome work indeed.
Wish we would have such complete PRs more often.
Thank you!

@ibolmo ibolmo merged commit ccc36c2 into from
@SergioCrisostomo SergioCrisostomo commented on the diff
Specs/Request/Request.js
((22 lines not shown))
+ var dit = /*<1.4compat>*/xit || /*</1.4compat>*/it; // don't run unless no compat
+ dit('should not set xhr.withCredentials flag in 1.5 for this.options.user', function(){
+ var request = new Request({
+ url: '/something/or/other',
+ user: 'someone'
+ }).send();
+
+ expect(request.xhr.withCredentials).toBe(false);
@SergioCrisostomo Collaborator

@ibolmo this is failing in some browsers with Expected undefined to be false.

Can we cast to boolean and use expect(!!request.xhr.withCredentials).toBe(false); ?

Ah, sorry, I was actually thinking about that, but all the tests passed in the build (from using the fake XHR object I suspect)

Could also wrap the expect in

if (request.xhr.hasOwnProperty('withCredentials')) { ...
@SergioCrisostomo Collaborator

@ccampbell we have encrypted user/pass to sauce labs testing. That means pull requests are only tested against PhantomJS on Travis. We should maybe change that so that pull requests actually get tested in browsers and not only after merge.

Anyway, this is a detail, I am really happy you proposed this fix.

@SergioCrisostomo Collaborator

@ccampbell Olmo just fixed it with .toBeFalsy()
Thanks again, cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 30, 2014
  1. @ccampbell

    Add option to set withCredentials to true without setting username fo…

    ccampbell authored
    …r auth
    
    Firefox sends the username for auth even though a password was not supplied
    which causes Firefox to abort before sending a preflight request for Cross-Origin
    requests.
    
    See http://stackoverflow.com/questions/11255173/firefox-wont-send-cross-origin-resource-sharing-pre-flight
  2. @ccampbell
  3. @ccampbell
  4. @ccampbell
This page is out of date. Refresh to see the latest.
View
3  Docs/Request/Request.md
@@ -35,8 +35,9 @@ An XMLHttpRequest Wrapper.
* isSuccess - (*function*) Overrides the built-in isSuccess function.
* evalScripts - (*boolean*: defaults to *false*) If set to true, `script` tags inside the response will be evaluated.
* evalResponse - (*boolean*: defaults to *false*) If set to true, the entire response will be evaluated. Responses with javascript content-type will be evaluated automatically.
-* user - (*string*: defaults to *null*) When username is set the Request will open with credentials and try to authenticate.
+* user - (*string*: defaults to *null*) The username to use for http basic authentication.
* password - (*string*: defaults to *null*) You can use this option together with the `user` option to set authentication credentials when necessary. Note that the password will be passed as plain text and is therefore readable by anyone through the source code. It is therefore encouraged to use this option carefully
+* withCredentials - (*boolean*: defaults to *false*) If set to true, xhr.withCredentials will be set to true allowing cookies/auth to be passed for cross origin requests
### Events:
View
5 Source/Request/Request.js
@@ -34,7 +34,8 @@ var Request = this.Request = new Class({
onException: function(headerName, value){},
onTimeout: function(){},
user: '',
- password: '',*/
+ password: '',
+ withCredentials: false,*/
url: '',
data: '',
headers: {
@@ -197,7 +198,7 @@ var Request = this.Request = new Class({
}
xhr.open(method.toUpperCase(), url, this.options.async, this.options.user, this.options.password);
- if (this.options.user && 'withCredentials' in xhr) xhr.withCredentials = true;
+ if ((/*<1.4compat>*/this.options.user || /*</1.4compat>*/this.options.withCredentials) && 'withCredentials' in xhr) xhr.withCredentials = true;
xhr.onreadystatechange = this.onStateChange.bind(this);
View
36 Specs/Request/Request.js
@@ -135,5 +135,41 @@ describe('Request', function(){
});
+ it('should not set xhr.withCredentials flag by default', function(){
+ var request = new Request({
+ url: '/something/or/other'
+ }).send();
+
+ expect(request.xhr.withCredentials).toBe(false);
+ });
+
+ /*<1.4compat>*/
+ it('should set xhr.withCredentials flag in 1.4 for this.options.user', function(){
+ var request = new Request({
+ url: '/something/or/other',
+ user: 'someone'
+ }).send();
+
+ expect(request.xhr.withCredentials).toBe(true);
+ });
+ /*</1.4compat>*/
+ var dit = /*<1.4compat>*/xit || /*</1.4compat>*/it; // don't run unless no compat
+ dit('should not set xhr.withCredentials flag in 1.5 for this.options.user', function(){
+ var request = new Request({
+ url: '/something/or/other',
+ user: 'someone'
+ }).send();
+
+ expect(request.xhr.withCredentials).toBe(false);
@SergioCrisostomo Collaborator

@ibolmo this is failing in some browsers with Expected undefined to be false.

Can we cast to boolean and use expect(!!request.xhr.withCredentials).toBe(false); ?

Ah, sorry, I was actually thinking about that, but all the tests passed in the build (from using the fake XHR object I suspect)

Could also wrap the expect in

if (request.xhr.hasOwnProperty('withCredentials')) { ...
@SergioCrisostomo Collaborator

@ccampbell we have encrypted user/pass to sauce labs testing. That means pull requests are only tested against PhantomJS on Travis. We should maybe change that so that pull requests actually get tested in browsers and not only after merge.

Anyway, this is a detail, I am really happy you proposed this fix.

@SergioCrisostomo Collaborator

@ccampbell Olmo just fixed it with .toBeFalsy()
Thanks again, cheers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ });
+
+ dit('should set xhr.withCredentials flag if options.withCredentials is set', function(){
+ var request = new Request({
+ url: '/something/or/other',
+ withCredentials: true
+ }).send();
+
+ expect(request.xhr.withCredentials).toBe(true);
+ });
});
Something went wrong with that request. Please try again.