Skip to content

Conversation

@markelog
Copy link
Member

Fixes gh-2432

@markelog
Copy link
Member Author

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 commented Oct 9, 2015

I think we decided to land it. :)

@jaubourg
Copy link
Member

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 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 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 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 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

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 pull request Oct 12, 2015
@markelog markelog mentioned this pull request Nov 16, 2015
@reedloden
Copy link
Contributor

Please see #2432 (comment)

};
} );

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

Choose a reason for hiding this comment

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

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

davideschiera pushed a commit to draios/jquery that referenced this pull request Mar 7, 2018
Proposed by @jaubourg

Fixes jquerygh-2432
Closes jquerygh-2588

(cherry picked from commit b078a62)

# Conflicts:
#	test/unit/ajax.js
davideschiera pushed a commit to draios/jquery that referenced this pull request 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 pushed a commit to acronisscs/jquery that referenced this pull request Jan 29, 2021
tamcy pushed a commit to tamcy/jquery that referenced this pull request Mar 17, 2022
drosen0 pushed a commit to draios/jquery that referenced this pull request Mar 14, 2024
Proposed by @jaubourg

Fixes jquerygh-2432
Closes jquerygh-2588

(cherry picked from commit b078a62)
(cherry picked from commit 3493060)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

7 participants