Repository navigation
script_safe: fix escaping of ", \ and control characters - #1146
Merged
Merged
Conversation
Since 889c06a, `search_script_safe_escape` returns the raw table value (`ESCAPE_LEN_1`) and leaves flushing to the escape function, but `escape_UTF8_char` still matched `case 1` and never flushed. With `script_safe: true` (without `ascii_only`), escaped characters hit no case, and the cursor skipped over both them and any pending unescaped bytes, silently dropping data: JSON.generate(["\""], script_safe: true) # => "[\"\"]" JSON.generate("a\"b\\c\nd", script_safe: true) # => "\"d\"" Match `ESCAPE_LEN_1` and flush before appending the escape, like `full_escape_UTF8_char` does. Co-Authored-By: Claude Opus 5.5 <[email protected]>
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
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.
889c06a (#1145) changed
search_script_safe_escapeto return the raw table value (ESCAPE_LEN_1) and leave flushing to the escape function.full_escape_UTF8_char(theascii_onlypath) was updated for this, butescape_UTF8_char(thescript_safe-only path) still matchescase 1and never callssearch_flush.As a result, with
script_safe: trueand withoutascii_only, characters that need escaping (",\, control characters) match no case. They are dropped, and so is any pending unescaped text before them:Fix
In
escape_UTF8_char, matchESCAPE_LEN_1and flush before appending the escape sequence, the same wayfull_escape_UTF8_chardoes.Tests
run_script_safe_testnow also covers strings with",\,\n,\tand a control character, and a string that mixes regular escapes with a</scriptescape. It runs withascii_onlyboth off and on. The existing tests had noscript_safe-only case with such characters, which is why the regression went unnoticed.I ran the full test suite against the C extension: 702 tests, 0 failures. I also round-trip fuzzed 20,000 random strings built from
<,/,script,!-, quotes, backslashes, newlines, control and multibyte characters, withascii_onlyboth off and on. Every output parsed back to the original string and contained no<script,</scriptor<!--. I have not run the tests on JRuby.🤖 Generated with Claude Code