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

Unhandled http error while appending an html with script element #4126

Closed
HolgerJeromin opened this issue Jul 12, 2018 · 16 comments · Fixed by #4243
Closed

Unhandled http error while appending an html with script element #4126

HolgerJeromin opened this issue Jul 12, 2018 · 16 comments · Fixed by #4243

Comments

@HolgerJeromin
Copy link

Description

I have an application where the customers can load arbitrary HTML code which is then loaded into the DOM via jQuery.
A customer had a copy and paste error which linked to an non existing js file. This lead to an 404 but nevertheless jQuery pushes the return value (error HTML page) into a HTMLScriptElement text property. This leads to an exception:

Uncaught SyntaxError: Unexpected token <
at DOMEval (jquery-3.3.1.js:111)
at Function.globalEval (jquery-3.3.1.js:345)
at text script (jquery-3.3.1.js:9640)
at ajaxConvert (jquery-3.3.1.js:8784)
at done (jquery-3.3.1.js:9255)
at XMLHttpRequest.<anonymous> (jquery-3.3.1.js:9548)
at Object.send (jquery-3.3.1.js:9600)
at Function.ajax (jquery-3.3.1.js:9206)
at Function.jQuery._evalUrl (jquery-3.3.1.js:9367)
at domManip (jquery-3.3.1.js:5759)

Minimal test case

Put this HTML onto an webserver:

<!DOCTYPE html>
<html>
  <head>
    <script type="text/javascript" src="https://code.jquery.com/jquery-3.3.1.js"></script>
    <title>Title</title>
	<script>
	function clickme() {
			var jBody = $(document.body);
			jBody.append('<script type="text/javascript" src="notexisting.js"><'+'/script>');
	}
	</script>
  </head>
  <body>
        <button onclick="clickme()">Click</button>
  </body>
</html>
@timmywil
Copy link
Member

Thanks for opening an issue. However, you can find support on stackoverflow.com or the jquery forums.

@HolgerJeromin
Copy link
Author

Why was this report closed?
I am using the jQuery API and jQuery is pushing knowingly (http code !== 200) garbage into an HTMLScriptElement text content.

@HolgerJeromin HolgerJeromin changed the title Unexpected exception while appending an html with script element Unhandled http error while appending an html with script element Jul 13, 2018
@mgol
Copy link
Member

mgol commented Jul 13, 2018

I can't confirm your issue, this works as expected in my JS Bin: http://jsbin.com/laqumev/edit?html,js,console,output

Something else must be going in your code.

@HolgerJeromin
Copy link
Author

@mgol
You changed my code to have a cross domain request. This changes the callstack and therefor prevents this error. (Could have mentioned it, sorry for that)
Changing it back results in
DOMException: Only secure origins are allowed
so jsbin is no valid hoster for this bug.
I have hosted it on my personal webspace: https://www.katur.de/bug.php (i don't know long this link will be valid)

@mgol
Copy link
Member

mgol commented Jul 13, 2018

@HolgerJeromin Thank you, I can confirm it locally on a same-origin request. In the future, please try to prepare as small an example as possible, here without any event handlers and especially without onclick which is not supported by jQuery.

@timmywil I think it's a valid issue, we shouldn't try to eval a valid error response. Let's reopen for a discussion.

@mgol mgol reopened this Jul 13, 2018
@timmywil
Copy link
Member

I see. Thank you @mgol for taking the time to make a test case.

However, I'm not sure this is necessarily wrong. jQuery is appending a script tag. The content is actually HTML. Whether it's eval'd by jQuery or executed directly by the browser, you'd get the same error. Right?

@HolgerJeromin
Copy link
Author

Whether it's eval'd by jQuery or executed directly by the browser, you'd get the same error. Right?

I am pretty sure no sane browser will try to interprete an 404 response of an script element as JavaScript.

I wrote an php code which has an 404 error and an alert as content. This script is referenced direct via script-element and loaded with jQuery. So this alert should be fired twice. But the alert is only fired when loaded via jQuery. Updated testcase: http://www.katur.de/bug.php

@mgol
Copy link
Member

mgol commented Jul 13, 2018

@timmywil We're not doing what the browser does. As @HolgerJeromin mentioned, if a browser hits an error page when downloading a script it will display an error message in the console but will not try to evaluate the error page as a script. Meanwhile, we invoke the converter even for error responses and the converter calls globalEval on the response.

I think it's a bug we should fix. Moreover, if the 404 page returns something parseable as JavaScript this may influence the page behavior randomly. Granted, such a response is unlikely but possible so in edge cases, this bug may have important consequences.

@mgol mgol added this to the 3.4.0 milestone Jul 13, 2018
@timmywil
Copy link
Member

timmywil commented Jul 13, 2018

Yup, that makes sense. I guess the only way the browser would try to execute is if the response code did not match the response content.

@gibson042
Copy link
Member

The HTML Standard requires an "ok status" as defined by Fetch to be "in the range 200 to 299, inclusive".

@dmethvin
Copy link
Member

In the case where we are trying to emulate script tags, it makes sense that we would match browser behavior and only execute responses that matched HTTP 2xx. Ideally, people wouldn't try to inject strings that are script tags and we've agonized about the problems these cause for years, including the synchronous execution issue.

@timmywil
Copy link
Member

Discussed in the meeting. We'll likely limit the eval to 2xx responses.

@gibson042
Copy link
Member

I wasn't sure about what changes were necessary and had been exploring extensions to ajax converter and dataFilter arguments, but in the end came to a much simpler conclusion: rather than relying upon them, we should update jQuery._evalUrl to evaluate responses manually:

return jQuery.ajax( {
	url: url,

	// Make this explicit, since user can override this through ajaxSetup (#11264)
	type: "GET",
	dataType: "text",
	cache: true,
	async: false,
	global: false,
	"throws": true,

	// Only evaluate the response if it is successful (gh-4126)
	success: function( text ) { jQuery.globalEval( text ); }
} );

@timmywil
Copy link
Member

@gibson042 So simple and clever. Love it.

@mgol
Copy link
Member

mgol commented Sep 5, 2018

@gibson042 Since you found out how to fix the issue, do you plan to prepare a PR?

@gibson042 gibson042 self-assigned this Sep 5, 2018
@gonzalez1
Copy link

Gracias

gibson042 added a commit to gibson042/jquery that referenced this issue Nov 26, 2018
gibson042 added a commit to gibson042/jquery that referenced this issue Nov 26, 2018
gibson042 added a commit to gibson042/jquery that referenced this issue Nov 26, 2018
gibson042 added a commit to gibson042/jquery that referenced this issue Nov 26, 2018
gibson042 added a commit to gibson042/jquery that referenced this issue Nov 26, 2018
gibson042 added a commit to gibson042/jquery that referenced this issue Nov 26, 2018
gibson042 added a commit to gibson042/jquery that referenced this issue Dec 13, 2018
IE and iOS <10 XHR transport does not succeed on data: URIs
Ref jquerygh-4243
Ref jquerygh-4126
gibson042 added a commit that referenced this issue Dec 13, 2018
IE and iOS <10 XHR transport does not succeed on data: URIs
Ref gh-4243
Ref gh-4126
Closes gh-4258
@lock lock bot locked as resolved and limited conversation to collaborators Jun 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging a pull request may close this issue.

6 participants