Ajax: Mitigate possible XSS vulnerability #2588
Conversation
|
@timmywil remind me please, did we decided to do it in this or next version? In other words, should we close or land it? |
|
I think we decided to land it. :) |
|
Still not a fan of this hack. It looks like old ajax code with tests and special cases hidden deep into the code. |
|
@jaubourg You're thinking from the code perspective, I (others mostly agreed IIRC) just think it's really bad that |
|
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. |
|
OK, I had an impression that you don't want to prevent |
|
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. |
|
Thanks @jaubourg. I think that's a good suggestion. |
|
Please see #2432 (comment) |
| @@ -71,6 +71,54 @@ QUnit.module( "ajax", { | |||
| }; | |||
| } ); | |||
|
|
|||
| ajaxTest( "jQuery.ajax() - do not execute js (crossOrigin)", 2, function( assert ) { | |||
There was a problem hiding this comment.
Ain't you using assert.expect(2) anymore? xD
Proposed by @jaubourg Fixes jquerygh-2432 Closes jquerygh-2588 (cherry picked from commit b078a62) # Conflicts: # test/unit/ajax.js
Proposed by @jaubourg Fixes jquerygh-2432 Closes jquerygh-2588 (cherry picked from commit b078a62) # Conflicts: # test/unit/ajax.js (cherry picked from commit 3493060)
Proposed by @jaubourg Cherry-picked from b078a62 Fixes jquerygh-2432 Closes jquerygh-2588
Proposed by @jaubourg Fixes jquerygh-2432 Closes jquerygh-2588
Proposed by @jaubourg Fixes jquerygh-2432 Closes jquerygh-2588 (cherry picked from commit b078a62) (cherry picked from commit 3493060)
Fixes gh-2432