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.

Sign up for GitHub

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

Jump to bottom

jQuery.ajax uncaught Exception for jsonp calls to 404 error page even though response header Content-Type: text/html is present #5034

Open
codefactor opened this issue Apr 13, 2022 · 6 comments
Open

jQuery.ajax uncaught Exception for jsonp calls to 404 error page even though response header Content-Type: text/html is present #5034

codefactor opened this issue Apr 13, 2022 · 6 comments
Assignees
@mgol
Labels
Ajax Behavior Change
Milestone
5.0.0

Comments

@codefactor
Copy link

Description

Upgrading from version 2 to version 3 of jQuery we see uncaught exceptions being thrown in unit test cases that cause certain unit tests to fail due to these uncaught exceptions. This causes is a backwards incompatibility to adopt version 3 we need to hack our unit test to ignore certain uncaught exceptions or otherwise detect the test environment to avoid requests to URLs that don't work in this environment. It would be good if we can fix that incompatibility since it seems easily avoidable.

Steps to Reproduce:

Run something like this:

jQuery.ajax({ url: "./someInvalidUrl", dataType: "jsonp" });

Assume the URL goes to some place which responds with 404 status and a Content-Type: text/html response header and responseText with HTML content.

Expected Behavior:

No uncaught exceptions should be observed.

Actual Behavior:

An uncaught exception is thrown attempting to evaluate the response text, which is not a script but HTML content.

Uncaught SyntaxError: Unexpected token '<'
    at b (jquery.min.js:2:866)
    at Function.globalEval (jquery.min.js:2:2905)
    at text script (jquery.min.js:2:83080)
    at jquery.min.js:2:79470
    at l (jquery.min.js:2:79587)
    at XMLHttpRequest.<anonymous> (jquery.min.js:2:82355)

Suggested Solution:

Do not attempt to DOMEval a 404 response during jsonp evaluation, an additional clue that this response is not a script is the content type response header. Either one should be sufficient.

Link to test case

Failure in jQuery 3.X: https://jsfiddle.net/5o94ujsL/
Same thin in jQuery 2.X: https://jsfiddle.net/m3dp2zbt/

In both cases the global error handler should not be invoked, but we see in jquery 3 that the global error handler is invoked because of the following stack trace:

Uncaught SyntaxError: Unexpected token '<'
    at b (jquery.min.js:2:866)
    at Function.globalEval (jquery.min.js:2:2905)
    at text script (jquery.min.js:2:83080)
    at jquery.min.js:2:79470
    at l (jquery.min.js:2:79587)
    at XMLHttpRequest.<anonymous> (jquery.min.js:2:82355)
@codefactor codefactor changed the title jQuery.ajax uncaught Exception for jsonp calls text/html Content-Type on a 404 response code jQuery.ajax uncaught Exception for jsonp calls to 404 error page even though response header Content-Type: text/html is present Apr 13, 2022
@codefactor
Copy link
Author

codefactor commented Apr 13, 2022 •
edited
Loading

Looking at the code, I might suggest some changes to the following lines:

jquery/src/ajax.js

Lines 272 to 279 in 5d5ea01

try {
response = conv( response );
} catch ( e ) {
return {
state: "parsererror",
error: conv ? e : "No conversion from " + prev + " to " + current
};
}

At this place I think there is enough information in the response and the isSuccess (which is false) to avoid calling conv() which we know is only going to result in an exception, if you ignore the unlikely scenario that the server is returning Javascript with an incorrectly typed Content-Type header on an unsuccessful response (such as 404 or 500) which was meant to be interpretted as application/javascript and passed along to the failure callbacks. I think that if the server meant to do that it should be responsible to set the content type header correctly.

One other thought - I haven't looked too much into the jQuery 2.0 version, but it might be a good idea to check what was going on there to see what was the solution there that would prevent this kind of error from being uncaught.

@dmethvin
Copy link
Member

This is reminiscent of #4655

@codefactor
Copy link
Author

@dmethvin ,
Thanks for the link - it does look very similar to that one. In this case, however, the dataType is "jsonp" and not "script" - so I wonder if that is a key difference.

Also the other errors says that the errorHandler is NOT called, and I'm fairly sure that in this test case the error handler is called; however, the issue is that there is still an uncaught exception that will fail most default QUnit test cases that attempt.

@mgol
Copy link
Member

mgol commented Oct 6, 2022

@codefactor Thanks for the report. There's actually no difference in converter code; converters also run in jQuery 2.x. This is a result of one of fundamental assumptions of jQuery.ajax - if you pass dataType, you decide to ignore the content type the server returns and you decide to always parse the response using the provided dataType.

What's different is that in jQuery 2.x jQuery.globalEval only uses script tags for scripts with "use strict" being at the index 1 - this was needed due to differences in how indirect eval works in strict mode (see #1449 for more info) - and in other cases an indirect eval is used. That indirect eval then throws an error because the response is not a valid script and the code gets to the catch from your snippet.

In jQuery 3.0.0 we decided to simplify the logic and always use script injection, with the caveat errors can only be detected via window.onerror. Again, see #1449 for more info.

Interestingly, we didn't cover that in the upgrade guide: https://jquery.com/upgrade-guide/3.0/. We should address that part.

@dmethvin #4655 is tangential to this issue; see also the followup in #4773 (and also #4778 but that one will only apply to >=4.0.0).

@mgol mgol added the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Oct 6, 2022
@mgol mgol self-assigned this Oct 6, 2022
@timmywil
Copy link
Member

timmywil commented Oct 10, 2022 •
edited
Loading

if you pass dataType, you decide to ignore the content type the server returns and you decide to always parse the response using the provided dataType.

I'm fine with this for successful responses, but it seems like this should not be the case for error responses. Problem is I don't yet know the implications of that opinion.

@timmywil timmywil removed the Discuss in Meeting Reserved for Issues and PRs that anyone would like to discuss in the weekly meeting. label Oct 17, 2022
@timmywil timmywil added this to the 5.0.0 milestone Oct 17, 2022
@mgol
Copy link
Member

mgol commented Oct 17, 2022

We'll re-evaluate this for 5.0.0. It has a potential of breaking existing code so it needs to land in a major version update and we're trying to not add more stuff to 4.0.0 as we want to release a beta in some predictable future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mgol mgol

Labels
Ajax Behavior Change
Milestone
5.0.0
Development

No branches or pull requests
4 participants
@dmethvin @timmywil @codefactor @mgol