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

jQuery.ajax: make dataType: "script" not execute scripts on unsuccessful HTTP status #4250

Closed
mgol opened this issue Dec 3, 2018 · 5 comments
Closed

Comments

@mgol
Copy link
Member

@mgol mgol commented Dec 3, 2018

Description

A followup of PR #4243 and an extension of issue #4126. We should stop evaluating scripts returned with an unsuccessful HTTP status when loaded via jQuery.ajax with dataType: "script". The fix in #4243 only targets DOM manipulation.

Link to test case

@mgol mgol added this to the 4.0.0 milestone Dec 3, 2018
@mgol mgol changed the title jQuery.ajax: make dataType: "script" not execute scripts on unsuccessfull HTTP status jQuery.ajax: make dataType: "script" not execute scripts on unsuccessful HTTP status Dec 3, 2018
@mgol mgol removed the Needs review label Dec 3, 2018
skrobinson added a commit to skrobinson/jquery that referenced this issue Apr 19, 2019
The content returned in an error response may not be valid Javascript.  I
first saw this when trying to retrieve a missing script file, the server
replied with HTTP 404 and an HTML page detailing the error.  So the error
which appeared on the console was "SyntaxError: expected expression,
got '<'" because the browser was attempting to parse the error page as
a script file.

Fixes jquerygh-4250

Signed-off-by: Sean Robinson <sean.robinson@scottsdalecc.edu>
skrobinson added a commit to skrobinson/jquery that referenced this issue Apr 19, 2019
The content returned in an error response may not be valid Javascript.  I
first saw this when trying to retrieve a missing script file, the server
replied with HTTP 404 and an HTML page detailing the error.  So the error
which appeared on the console was "SyntaxError: expected expression,
got '<'" because the browser was attempting to parse the error page as
a script file.

Fixes jquerygh-4250

Signed-off-by: Sean Robinson <sean.robinson@scottsdalecc.edu>
@skrobinson
Copy link
Contributor

@skrobinson skrobinson commented Apr 19, 2019

I have a possible fix and a test case in commit 6716b75.

@mgol
Copy link
Member Author

@mgol mgol commented Apr 23, 2019

@skrobinson Your patch skips converters completely for unsuccessful responses; this is not right, it doesn't pass our test suite.

The proper fix will most likely require generalizing what was done in #4243 so that the logic is applied to all requests with dataType: "script".

skrobinson added a commit to skrobinson/jquery that referenced this issue Apr 26, 2019
Unsuccessful responses may not contain text convertible to the intended
type (e.g. json, script, etc.) using the normal converter.  The two added
test cases only check for `dataType: "script"`.

Fixes jquerygh-4250

Signed-off-by: Sean Robinson <sean.robinson@scottsdalecc.edu>
@skrobinson
Copy link
Contributor

@skrobinson skrobinson commented Apr 26, 2019

@mgol Thank you for your feedback. I've re-read #4243 and #4258 and tried again using the pattern in #4258. I've also improved my testing and have a new attempt at a fix.

@mgol
Copy link
Member Author

@mgol mgol commented Apr 27, 2019

@skrobinson Let us know if you need any help/direction! (or if you don't have time to submit a PR so that we know we should look at it ourselves)

@mgol mgol removed the Needs review label Apr 27, 2019
skrobinson added a commit to skrobinson/jquery that referenced this issue Jun 12, 2019
While trying to improve my fix for jquerygh-4250, I needed a failing service.
This will be parked on a side branch until it is ever needed.  Which may
be never.

Signed-off-by: Sean Robinson <sean.robinson@scottsdalecc.edu>
skrobinson added a commit to skrobinson/jquery that referenced this issue Sep 6, 2019
Unsuccessful responses may not contain text convertible to the intended
type (e.g. json, script, etc.) using the normal converter.  The two added
test cases only check for `dataType: "script"`.

Fixes jquerygh-4250

Signed-off-by: Sean Robinson <sean.robinson@scottsdalecc.edu>
skrobinson added a commit to skrobinson/jquery that referenced this issue Sep 6, 2019
Unsuccessful responses may not contain text convertible to the intended
type (e.g. json, script, etc.) using the normal converter.  The two added
test cases only check for `dataType: "script"`.

Fixes jquerygh-4250

Signed-off-by: Sean Robinson <sean.robinson@scottsdalecc.edu>
@mgol mgol closed this as completed in 50871a5 Sep 26, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020
mgol pushed a commit that referenced this issue Apr 6, 2020
The script transport used to evaluate fetched script sources which is
undesirable for unsuccessful HTTP responses. This is different to other data
types where such a convention was fine (e.g. in case of JSON).

(cherry picked from 50871a5)

Fixes gh-4250
Fixes gh-4655
Closes gh-4379
@mgol mgol removed this from the 4.0.0 milestone Apr 6, 2020
@mgol mgol added this to the 3.5.0 milestone Apr 6, 2020
@mgol
Copy link
Member Author

@mgol mgol commented Apr 6, 2020

Landed on master in 50871a5 last year & now backported to 3.x-stable in da3dd85.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

2 participants