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

fix: escape closing script tag #6822

Closed
wants to merge 2 commits into from

Conversation

noslouch
Copy link

@noslouch noslouch commented Oct 8, 2021

Hello! Big fan of svelte; thanks for all that you do.

If a svelte app is inlined to a browser (i.e. stuffed in between <script>...</script> tags instead of loaded remotely), the string in this patch is parsed as a valid closing script tag.

This is similar to, but not quite the same as #3039 / #3840 / #4406 / #4996 / #5024 / #5237 / sveltejs/rollup-plugin-svelte#133 / #5810 / sveltejs/rollup-plugin-svelte#182 / #6203 / #6366 / maybe others.

In those issues, the errant script tag is in user-land code. In this case it's in svelte core.

tests pass locally for me with this change.

if a svelte app is inlined to a browser (i.e. stuffed in between `<script>...</script>` tags instead of loaded remotely), this tag is parsed as a valid closing script tag.
@benmccann
Copy link
Member

Since it took me awhile to see what was different: </script> was changed to <\/script>

@Conduitry
Copy link
Member

I'm of two minds about whether this is Svelte's responsibility. Ultimately, it probably should be the responsibility of whoever's turning something into an inline script to make sure the appropriate things are escaped.

By making this change, we're hoping/assuming that no one else is going to unescape this again (during bundling, say, or some other process), and we're assuming that no other instances of this are going to be introduced. On the other hand, unless the component author were doing something unusual somewhere, this would likely be the only instance of </script> in the script. I don't know.

Any effort to escape this within Svelte would likely be completely clobbered during minification - unless the appropriate option in Terser were enabled (https://github.com/terser/terser#format-options) which would take care of this for us anyway.

In short, this seems to be a somewhat ineffective fix for something that ought to be a consumer's problem, but I'm not sure how bad this would actually be to change, or how much of a false sense of security this would provide.

@noslouch
Copy link
Author

Hiya, thanks for responding (sorry for being late getting back to you).

Your instinct about terser is correct: I only see this in unminified output. Terser escapes the slash on its own. Minimal repro here.

(For anyone following along at home, you can see this in your output if you use any of the DOM dimension bindings.)

I hear you on wondering where the responsibility should land. It's certainly a little unusual to inline a svelte app within an HTML document, but I wouldn't be surprised if this has bitten others before (I seem to recall svelte being pretty popular at a certain large media company 🙃 )

I guess I just came here to see if there's something we can do to help future travelers avoid the grief of encountering this and digging their way out to a fix. Maybe an FAQ or troubleshooting docs update?

@tanhauhau
Copy link
Member

closed this PR since we are not going to fix it in Svelte's core.

i've opened an issue #7935 for documentation

@tanhauhau tanhauhau closed this Oct 13, 2022
@noslouch noslouch deleted the fix/escape-closing-tag branch October 13, 2022 13:50
@noslouch
Copy link
Author

Thanks for adding a tracking issue. Putting something in the docs is perfectly fine!

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 this pull request may close these issues.

None yet

4 participants