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

Incorrect escaping inside STYLE and SCRIPT elements. #6

Closed
mbostock opened this issue Dec 19, 2019 · 3 comments · Fixed by #22
Closed

Incorrect escaping inside STYLE and SCRIPT elements. #6

mbostock opened this issue Dec 19, 2019 · 3 comments · Fixed by #22

Comments

@mbostock
Copy link
Member

STYLE and SCRIPT elements have special behavior regarding escaping: they use “RAWTEXT” rather than “DATA” mode. For example, this is not an ampersand character reference:

html`<script>hello&amp;</script>`

I think this means we’ll need to track the element name when we enter the DATA state, and if it’s STYLE or SCRIPT, enter the RAWTEXT state instead. And then likewise we’ll have to handle the “appropriate end tag” to determine when we exit the RAWTEXT state.

@mbostock
Copy link
Member Author

As another example, the ampersand should not be encoded as &#38; here:

html`<style>

p {
  background-image: url(${"foo.png?bar=1&baz=2"});
}

</style>`

@clarkevans
Copy link

clarkevans commented Dec 17, 2020

We implemented this within a fork of this project for Julia, this turned out to be relatively easy. Most of the state machine already tracks what you need to keep the current element name, and you only have to match on style/script tags (case independently). Note that besides not permitting <style> or <script> literal values, additionally <script> doesn't like comments <!--. There's a design choice to try and save the user from injecting these values, or, letting them do it (since including <script> within Javascript string is a gotya that you can find on stackoverflow). At this time, our fork is trying to detect this error, but there's a speed cost, so we may rip it out.

@mbostock
Copy link
Member Author

mbostock commented Jan 9, 2021

@clarkevans Thanks for the pointer! I’d like to fix this together with #18; see #21 (comment).

mbostock added a commit that referenced this issue Jan 9, 2021
mbostock added a commit that referenced this issue Jan 9, 2021
@mbostock mbostock mentioned this issue Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants