search

emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.46k stars 4.21k forks source link

[Bug] Ember silently allows malformed HTML :bear: #19703

Closed sophie-gg closed 2 years ago

sophie-gg commented 2 years ago

🐞 Describe the Bug

Ember accepts malformed closing tags in hbs templates. For example <div>Hello</<div> or <div>Hello</$$/////</<<( ✈️🏝🍜 £190/🐻 )<<}}}}}<div>. (Behaviour is the same for both HTML tags and Glimmer components)

🔬 Minimal Reproduction

https://ember-twiddle.com/26826bed641649699d942a91c01e64f5?openFiles=templates.application%5C.hbs%2C

😕 Actual Behavior

Ember silently ignores he malformed closing tag; builds the app without errors and warnings. In the browser, the characters that were part of the malformed tag (like "✈️🏝🍜 £190/🐻 " in the example above) disappear.

This is somewhat consistent with plain HTML behaviour. If you open a Chrome console and type document.body.innerHTML = '<div>Welcome to this app</$$/////</<<( ✈️🏝:ramen: £190/:bear: )<<}}}}}, Chrome also ignores the malformed bits and inserts a correct closing tag instead. But in this case, Chrome at least keeps the malformed bits but surrounds them with comments. Ember does not seem to do that.

Browsers are very forgiving with malformed HTML, and make an effort to display something. In another example, they let you omit a closing tag: document.body.innerHTML = '<div>This works;'. Chrome handles this, and display a div. But if you try <div>This works in an Ember hbs template, you get a build-time error.

🤔 Expected Behavior

Just like the case of a missing closing tag, a malformed closing tag should produce a build-time error and prevent compilation of the template.

🌍 Environment

➕ Additional Context

Discovered this during a code review, when something that looked like a syntax error in a template (</<MyComponent>) did not fail the build, nor did it produce a runtime error.

NullVoxPopuli commented 2 years ago

Maybe looks related to https://github.com/glimmerjs/glimmer-vm/issues/1309 ? using the underlying package: https://www.npmjs.com/package/simple-html-tokenizer (https://github.com/tildeio/simple-html-tokenizer) ?

MelSumner commented 2 years ago

@sophie-gg - @NullVoxPopuli is correct; a good path forward would be to submit a PR to simple-html-tokenizer with a test that you would expect to fail (but doesn't). 👍

jfdnc commented 2 years ago

Fwiw, the simple-html-tokenizer README vaguely acknowledges this behavior:

'It also passes through character references, instead of trying to tokenize and process them, because the preprocessed templates will ultimately be parsed by a real browser context.'

Any character reference that's encountered after the end tag is opened just gets ignored, as in the several funky cases in the OP here. It looks like this was a practical decision (due to similar built-in browser flexibility as mentioned) and has always acted this way for character references in closing tags.

jfdnc commented 2 years ago

Another update: There is already an open PR in simple-html-tokenizer to address error end tag error handling. Shoulda seen that yesterday but I just started looking back into this and the PR is there with no issue. Will ping there with a link to this issue!

MelSumner commented 2 years ago

Thanks! Closing this issue for now, please reopen if you feel it is necessary. 👍