inlined JS still breaks out of <script> for </SCRIPT>, </script >, </script/> variants - #498
Open
goingforstudying-ctrl wants to merge 1 commit into
Conversation
the current escape only catches the literal lowercase </script>, but HTML parsers close a script element on any case-insensitive match of </script followed by whitespace, a solidus, or ">". bundles with </SCRIPT>, </script >, or </script/> in string literals still broke out of the wrapper element and corrupted the saved page. scan for the full end tag syntax instead of a single literal, and leave longer tag names (</scripts>) alone.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ran into the same symptom as #497 while archiving a page the other day, big chunk of minified JS showing up as visible text in the saved file. Dug into it and the escape that was added in b2002c1 only covers the exact lowercase
</script>. HTML parsers are a lot more liberal than that: the tag name matches case-insensitively and just has to be followed by whitespace, a solidus, or>to count as an end tag (script data end tag name state). So a bundle with</SCRIPT>,</script >, or</script/>sitting in a string literal still breaks out of the wrapper element and everything after it gets parsed as page content.Repro'd against master with a tiny page + script to make sure I wasn't chasing ghosts:
the
</SCRIPT>goes through verbatim and the script element terminates early. Same story for the whitespace and solidus variants.This swaps the single-literal
.replace()for a small scanner (escape_script_end_tagin js.rs) that catches every form an HTML tokenizer would treat as a script end tag and escapes it the same way as before (<\/...). Longer tag names like</scripts>are left alone since they're not end tags, escaping those would just be churn (and technically changes the string's bytes for no reason).Tests: unit tests for the scanner itself (case variants, tab/space/newline after the tag name, solidus, tag at end of input, multibyte chars right before the tag, plus guards that
</scripts>and lone<stay untouched), and a CLI test with a local fixture covering all the variants end to end.cargo test --releaseis green, 296 passed / 0 failed. Also ran the built binary over a local http server with the repro payloads and the archived page comes out clean now.One thing I'm not 100% sure about is the double-escape corner (
<!--<script>...</script>-->inside an inline script). The scanner already catches the</scriptthat would exit double-escaped state so I think it's covered, but I didn't add explicit handling for literal<!--or<scriptin the payload. Can look at that separately if you think it's worth it.