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

Ajax: Mitigate possible XSS vulnerability #2588

Closed
wants to merge 1 commit into from
Closed

Conversation

@markelog
Copy link
Member

@markelog markelog commented Sep 10, 2015

Fixes gh-2432

@markelog
Copy link
Member Author

@markelog markelog commented Oct 8, 2015

@timmywil remind me please, did we decided to do it in this or next version? In other words, should we close or land it?

@mgol
Copy link
Member

@mgol mgol commented Oct 9, 2015

I think we decided to land it. :)

@jaubourg
Copy link
Member

@jaubourg jaubourg commented Oct 9, 2015

Still not a fan of this hack. It looks like old ajax code with tests and special cases hidden deep into the code.

@mgol
Copy link
Member

@mgol mgol commented Oct 9, 2015

@jaubourg You're thinking from the code perspective, I (others mostly agreed IIRC) just think it's really bad that $.get(SOME_URL) can execute code, I think most people doesn't expect it. Methods should be as secure by default as they can be.

@jaubourg
Copy link
Member

@jaubourg jaubourg commented Oct 9, 2015

I'm thinking about code perspective because there are better, more consistent ways to handle this than hack into the conversion logic like that. Like providing the options object to converters so that they can handle special cases properly and report sensible errors.

Now, from a behavior point of view, this will effectively prevent valid cross-domain script execution because the xhr transport will provide a text response and will need to use the converter whether the dataType was provided explicitely or not.

@mgol
Copy link
Member

@mgol mgol commented Oct 9, 2015

OK, I had an impression that you don't want to prevent $.get(CROSS_DOMAIN_URL) from executing script and not that you're just against this specific way of achieving that.

@jaubourg
Copy link
Member

@jaubourg jaubourg commented Oct 9, 2015

OK, so I've been thinking about this and the best way to handle this is to inhibit auto detection of the script dataType in the context of a cross-domain request which can be achieved by adding the following prefilter before the existing one in src/ajax/script:

// Prevent auto-execution of scripts when no explicit dataType was provided
jQuery.ajaxPrefilter( function( s ) {
    if ( s.crossDomain ) {
        s.contents.script = false;
    }
} );

I still think this should be done userland but, anyway, that's the way to achieve this properly.

@timmywil
Copy link
Member

@timmywil timmywil commented Oct 10, 2015

Thanks @jaubourg. I think that's a good suggestion.

@markelog markelog closed this in b078a62 Oct 12, 2015
markelog added a commit that referenced this issue Oct 12, 2015
Proposed by @jaubourg

Cherry-picked from b078a62
Fixes gh-2432
Closes gh-2588
@markelog markelog mentioned this pull request Nov 16, 2015
@reedloden
Copy link
Contributor

@reedloden reedloden commented Mar 21, 2016

Please see #2432 (comment)

@@ -71,6 +71,54 @@ QUnit.module( "ajax", {
};
} );

ajaxTest( "jQuery.ajax() - do not execute js (crossOrigin)", 2, function( assert ) {

Ain't you using assert.expect(2) anymore? xD

davideschiera added a commit to draios/jquery that referenced this issue Mar 7, 2018
Proposed by @jaubourg

Fixes jquerygh-2432
Closes jquerygh-2588

(cherry picked from commit b078a62)

# Conflicts:
#	test/unit/ajax.js
davideschiera added a commit to draios/jquery that referenced this issue Mar 8, 2018
Proposed by @jaubourg

Fixes jquerygh-2432
Closes jquerygh-2588

(cherry picked from commit b078a62)

# Conflicts:
#	test/unit/ajax.js
(cherry picked from commit 3493060)
@jquery jquery locked as resolved and limited conversation to collaborators May 8, 2018
scs-ahkr added a commit to acronisscs/jquery that referenced this issue Jan 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants