Do not execute scripts for unsuccessful http responses #4250 #4379

skrobinson · 2019-04-30T13:48:01Z

Summary

Fix for #4250.

Checklist

All authors have signed the CLA at https://cla.js.foundation/jquery/jquery
New tests have been added to show the fix or feature works
Grunt build and unit tests pass locally with these changes
If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com

timmywil · 2019-05-06T16:43:34Z

I'd approve without the wip commit. Travis will run on PRs.

mgol · 2019-05-06T17:20:08Z

Don't we want converters to be applied for unsuccessful non-script responses?

.travis.yml

timmywil · 2019-05-06T19:10:03Z

Good point. This should be limited to scripts.

skrobinson · 2019-05-07T14:38:07Z

Should converters be run on error responses? For text and html, probably yes. For scripts, no. What about json, jsonp, and xml?

dmethvin · 2019-05-07T16:40:53Z

I know of APIs that return non-200 status with JSON. The only case we want to prevent here is script.

mgol · 2019-05-07T18:06:31Z

I did some attempts in mgol@0cf3869 some time ago but I struggled with how to actually detect scripts. This is because a script may have dataType === "script" but it may also have none when it was determined to be a script via content-type matching.

skrobinson · 2019-05-08T14:28:41Z

@dmethvin Good point. Should we consider APIs which intentionally return non-200 status scripts?

Is it correct to replace anything other than the default converter? My current patch would also replace any custom "text script" converter.

mgol · 2019-05-08T14:47:59Z

@skrobinson Returning data on non-200 responses definitely makes sense. Consider a JSON API that sends a 400 with JSON data describing an error in body; that's quite common. Scripts are special in that its response is not really just "converted" but executed as a script, that's why we want to avoid it in this specific example. I think that answers your question:

Is it correct to replace anything other than the default converter?

with: no, that's not correct. We need to stop applying converters for scripts only.

skrobinson · 2019-05-08T15:31:15Z

Is it correct to replace anything other than the default converter?
with: no, that's not correct. We need to stop applying converters for scripts only.

Yes, I'm convinced about data. But, should we be careful not to disrupt a non-default "text script" converter? If the dev-user has defined a "text script" converter with ajaxSetup, should we still replace that converter with a noop?

mgol · 2019-05-08T16:28:25Z

@skrobinson A good question. I think I'm fine with ignoring user-provided script converters for failed HTTP status codes.

skrobinson · 2019-05-10T13:26:08Z

@mgol I'm still learning the GH way and should have brought commit 3e3fc0d to your attention. It think this incorporates all current feeback. Please let me know about other edits to be done.

mgol

I'd like to see a test for a request without dataType but with a response with a JS Content-Type, plus maybe one for jsonp? You can incorporate a test from my attempt at mgol@0cf3869.

I also left one comment.

Other than that, this seems to work, nice job!

test/unit/ajax.js

skrobinson · 2019-05-13T13:27:05Z

We've just started a post-semester remodeling at work and I'll be unavailable for up to two weeks. I will return to this PR, but likely not until end of May.

mgol · 2019-05-13T16:27:28Z

@skrobinson No problem, we're not in a hurry. Let us know when you're ready to continue, I'll be happy to keep reviewing new changes (just ping me directly when you update the PR, please :)).

gibson042 · 2019-05-13T16:43:00Z

I think this means that jQuery._evalUrl can also be simplified.

mgol · 2019-05-13T16:48:40Z

I think this means that jQuery._evalUrl can also be simplified.

@gibson042 I was thinking about that but it now also supports the options object that's used to pass nonce in domManip...

But, I guess, we could move the logic back to the converter now since it won't be executed for bad requests. Is that what you had in mind?

mgol

One more comment (but see the previous ones as well).

test/unit/ajax.js

gibson042 · 2019-05-13T17:27:22Z

But, I guess, we could move the logic back to the converter now since it won't be executed for bad requests. Is that what you had in mind?

Yeah, it should no longer need the dataFilter kludge AFAICT.

skrobinson · 2019-06-13T13:39:31Z

@mgol I have pushed updates to my PR branch.

You made a good call asking for more tests. I found my first if was not sufficient.

P.S. I'll look at jQuery._evalUrl after this PR lands.

mgol

Sorry for the delay. I like the way you generated many test cases, it's easy to see what's going on. I left a few comments.

test/unit/ajax.js

skrobinson · 2019-08-30T13:11:59Z

@mgol This PR is ready for another review.

RE: the many test cases. I copied the pattern used for the "jQuery.ajax() - cache" tests, once I understood how those tests worked.

mgol

A few more minor comments but it looks almost done to me, thanks!

test/data/mock.php

test/unit/ajax.js

test/data/mock.php

mgol · 2019-09-05T21:29:28Z

@skrobinson before applying further updates, could you rebase this PR over current master? The current base is from April this year & a lot has changed in the setup since then.

Signed-off-by: Sean Robinson <[email protected]>

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 <[email protected]>

Signed-off-by: Sean Robinson <[email protected]>

The exact error message is platform dependent, so just check that a message is present. Firefox says "File not found" while Chrome says "Not Found". Signed-off-by: Sean Robinson <[email protected]>

The original change was meant to prompt Travis tests before submitting a Pull Request. With a PR in, this is no longer necessary. This reverts commit 1146399. Signed-off-by: Sean Robinson <[email protected]>

Signed-off-by: Sean Robinson <[email protected]>

s.dataType is optional and may be undefined. Look for "script" in the chain of convertible types. Expands tests by building on mgol's previous work mocking failed services. Signed-off-by: Sean Robinson <[email protected]>

The original flow was an attempt to keep a pristine copy of the default options. But, it doesn't account for how JavaScript works. The object literal is created anew on each call to request and does not need to be protected. Signed-off-by: Sean Robinson <[email protected]>

The jQuery project's standard missing file is 404.txt. Signed-off-by: Sean Robinson <[email protected]>

Use a common object which fails any test attempting to convert the response as a script. Signed-off-by: Sean Robinson <[email protected]>

Signed-off-by: Sean Robinson <[email protected]>

skrobinson · 2019-09-06T13:58:18Z

@mgol These latest changes are complete and I've rebased onto today's master.

mgol

Looks good, thanks!

timmywil

LGTM

mgol · 2019-09-26T01:52:02Z

Landed, thanks @skrobinson for keeping up with our review comments!

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 · 2020-04-06T20:46:29Z

Backported to 3.x-stable in da3dd85.

mgol added Needs review Ajax labels Apr 30, 2019

timmywil changed the title ~~Fix for #4250~~ Do not execute scripts for unsuccessful http responses #4250 May 6, 2019

mgol reviewed May 6, 2019

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

mgol requested changes May 12, 2019

View reviewed changes

test/unit/ajax.js Outdated Show resolved Hide resolved

timmywil removed the Needs review label May 13, 2019

timmywil added this to the 4.0.0 milestone May 13, 2019

mgol requested changes May 13, 2019

View reviewed changes

test/unit/ajax.js Outdated Show resolved Hide resolved

mgol requested changes Jul 18, 2019

View reviewed changes

mgol added the Behavior Change label Aug 21, 2019

This comment has been minimized.

Sign in to view

skrobinson force-pushed the wip-gh-4250-v2 branch from cb9e30e to c312bf3 Compare August 28, 2019 13:26

This comment has been minimized.

Sign in to view

mgol requested changes Sep 5, 2019

View reviewed changes

test/data/mock.php Outdated Show resolved Hide resolved

test/unit/ajax.js Outdated Show resolved Hide resolved

test/data/mock.php Outdated Show resolved Hide resolved

skrobinson added 13 commits September 6, 2019 06:42

Add wip- branches to Travis builds

692e2bb

Signed-off-by: Sean Robinson <[email protected]>

Improve test message

9e6e84c

Signed-off-by: Sean Robinson <[email protected]>

Fix error message test

f2c7619

The exact error message is platform dependent, so just check that a message is present. Firefox says "File not found" while Chrome says "Not Found". Signed-off-by: Sean Robinson <[email protected]>

Revert "Add wip- branches to Travis builds"

21803c6

The original change was meant to prompt Travis tests before submitting a Pull Request. With a PR in, this is no longer necessary. This reverts commit 1146399. Signed-off-by: Sean Robinson <[email protected]>

Limit noop converter to script type

dd3f7c8

Signed-off-by: Sean Robinson <[email protected]>

Improve conditional for script type and unit tests

7353e4a

s.dataType is optional and may be undefined. Look for "script" in the chain of convertible types. Expands tests by building on mgol's previous work mocking failed services. Signed-off-by: Sean Robinson <[email protected]>

Change URL for missing script file

71109f6

The jQuery project's standard missing file is 404.txt. Signed-off-by: Sean Robinson <[email protected]>

Expand tests to include script and non-script mock server responses

49c414e

Use a common object which fails any test attempting to convert the response as a script. Signed-off-by: Sean Robinson <[email protected]>

Fix whitespace in mock.php: spaces to tabs

301e1b3

Signed-off-by: Sean Robinson <[email protected]>

Use camelCase with fail converters variable name

dce55ba

Signed-off-by: Sean Robinson <[email protected]>

Rename script type parameter

db906f0

Signed-off-by: Sean Robinson <[email protected]>

skrobinson force-pushed the wip-gh-4250-v2 branch from f701a76 to db906f0 Compare September 6, 2019 13:44

mgol approved these changes Sep 7, 2019

View reviewed changes

timmywil approved these changes Sep 23, 2019

View reviewed changes

mgol closed this in 50871a5 Sep 26, 2019

lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020

mgol modified the milestones: 4.0.0, 3.5.0 Apr 6, 2020

Do not execute scripts for unsuccessful http responses #4250 #4379

Do not execute scripts for unsuccessful http responses #4250 #4379

Uh oh!

Conversation

skrobinson commented Apr 30, 2019

Summary

Checklist

Uh oh!

timmywil commented May 6, 2019

Uh oh!

mgol commented May 6, 2019

Uh oh!

Uh oh!

timmywil commented May 6, 2019

Uh oh!

skrobinson commented May 7, 2019

Uh oh!

dmethvin commented May 7, 2019

Uh oh!

mgol commented May 7, 2019

Uh oh!

skrobinson commented May 8, 2019

Uh oh!

mgol commented May 8, 2019

Uh oh!

skrobinson commented May 8, 2019

Uh oh!

mgol commented May 8, 2019

Uh oh!

skrobinson commented May 10, 2019

Uh oh!

mgol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skrobinson commented May 13, 2019 via email

Uh oh!

mgol commented May 13, 2019

Uh oh!

gibson042 commented May 13, 2019

Uh oh!

mgol commented May 13, 2019

Uh oh!

mgol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gibson042 commented May 13, 2019

Uh oh!

skrobinson commented Jun 13, 2019

Uh oh!

mgol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

skrobinson commented Aug 30, 2019

Uh oh!

mgol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mgol commented Sep 5, 2019

Uh oh!

skrobinson commented Sep 6, 2019

Uh oh!

mgol left a comment

Choose a reason for hiding this comment

Uh oh!

timmywil left a comment

Choose a reason for hiding this comment