Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for broken cross domain behaviour in jQuery >= 1.5 #167

Closed
wants to merge 1 commit into from

Conversation

ciaranlee
Copy link
Contributor

Hi,

This is following up on the discussion here: a284dd7#comments

I have implemented antidis's fix. I'm sorry but I couldn't manage to write a test for it, I tried but couldn't manage to get it to work, so I deployed a simple app to illustrate the problem and show that the patched version fixes it.

If you check the app here: http://deep-autumn-373.heroku.com/home/index you will see that neither the link or the form will submit properly, as the browser will refuse to send a cross domain request with the X-CSRF-Token header. Then try the patched rails ujs (by clicking on the link at the top of the page) and it will work.

Again, apologies for the lack of a test.

Ciaran

@JangoSteve
Copy link
Member

Is there any sort of documentation as to why a cross-domain request fails with the X-CSRF-Token header? I thought headers that begin with "X-" are largely meant for personal use in websites (analogous to the "data-" attributes in HTML5), as in they have no official semantic meaning. Is this some convention that browsers have taken it upon themselves to look at?

I'm just trying to get a better understanding of what's actually happening.

@ciaranlee
Copy link
Contributor Author

Hey Steve,

There is a good explanation of preflighted cross domain requests here , and of cross domain requests in general here

If a cross domain request requires a preflight check, then unless the preflight response explicitly includes X-CSRF-Token in it's Access-Control-Allow-Headers header, then the browser will refuse to make the cross domain request. You can see this happen by looking at the JS console in your browser while trying my demo.

I think it's a mistake to include the X-CSRF-Token header in cross-domain requests as they are not relevant to the cross domain request. Also, it's probably unrealistic to expect third party services to modify their preflight response in order to accomodate the X-CSRF-Token header.

I hope this helps explain things!

It's not a problem for us if you guys don't want to merge this in, as we have worked around it, but somebody else will probably run in to the same problem in future and it would be nice for them not to have to go through the same process of discovery again.

@JangoSteve
Copy link
Member

Thanks, Ciaran. The preflighted link, and also this link it contained were exactly what I was looking for. I didn't realize that custom HTTP headers trigger a preflighted request which includes Access-Control-Request-Headers: X-CSRF-Token to the target domain; I was under the misconception that they were silently ignored.

For a test-case, I'm thinking we could probably do something as simple as check to make sure the X-CSRF-Token is undefined in the request header. E.g. see this test.

Thinking through it a little more, would it make sense to turn off CSRF by default if the host in the remote url is different from the current host? Maybe something like:

function crossDomain(element) {
  var crossDomain = element.data('cross-domain');

  if (crossDomain === undefined) {
    // To parse the host from the target url string...
    var url = document.createElement('a');
    url.href = element.attr( (element.is('form') ? 'action' : 'href') );

    return url.host != location.host;
  } else {
    return crossDomain;
  }
}

This would still follow the data-cross-domain attribute if present, but would also give a handy default when the request is obviously cross-domain.

@@ -116,7 +117,7 @@
}

rails.ajax({
url: url, type: method || 'GET', data: data, dataType: dataType,
url: url, type: method || 'GET', data: data, dataType: dataType, crossDomain: crossDomain,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be noted that the crossDomain parameter in jQuery's ajax function was only added in 1.5. This caused some funky behavior in the test case I created when testing with jquery >1.5.

I read through the jquery source, since it's not really documented anywhere, but among other things, setting crossDomain: true causes jquery to not set headers[ "X-Requested-With" ] = "XMLHttpRequest";, and so, Rails would return false forrequest.xhr?`.

According to the comments in the jquery source:

// X-Requested-With header
// For cross-domain requests, seeing as conditions for a preflight are
// akin to a jigsaw puzzle, we simply never set it to be sure.
// (it can always be set on a per-request basis or even using ajaxSetup)
// For same-domain requests, won't change header if already provided.
if ( !s.crossDomain && !headers["X-Requested-With"] ) {
  headers[ "X-Requested-With" ] = "XMLHttpRequest";
}

@JangoSteve
Copy link
Member

FYI, I am pulling this in, albeit with some modifications as discussed above in the line-comments. Also, I created a test-case for this, which I'm currently trying to get working. There's a little weirdness in jquery >= 1.5, due to the fact that jquery skips setting the X-Requested-With header.

JangoSteve added a commit that referenced this pull request Jun 15, 2011
Fixed token-header-exclusion for jquery 1.4.4.

Also created test for intelligently guessing cross-domain behavior when target
url is on a different domain, and fixed behavior for this case.

See Issue #167 for all relevant discussion.
@JangoSteve
Copy link
Member

Pulled in 4188a64, tests created and implementation modified in 616e4fe.

@JangoSteve JangoSteve closed this Jun 15, 2011
@ciaranlee
Copy link
Contributor Author

Thanks Steve. I tried to write tests for this a bunch of times but couldn't get them to work. Sorry I couldn't be of more help.

@JangoSteve
Copy link
Member

Hey no prob, you were lots of help with educating me as to what was going on. It took me a while to figure out the test suite as well :-P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants