Skip to content

Commit e26ddc7

Browse files
okuryuCopilot
andcommitted
Fix quadratic scan and regex-literal quote misclassification
Address two Copilot review comments on PR #226: - Replace the O(n*m) `.some()` linear scan over string/comment spans (run once per script-close match) with a forward-moving cursor. Both matches and spans are processed in increasing source-offset order, so a single cursor is enough to classify every match in O(n) total instead of re-scanning all spans for every match. - Add a regex-literal alternative to STRING_OR_COMMENT_REGEXP. Without it, a quote character inside a genuine regex literal (e.g. `/'/`) could be mistaken for the start of a string, misaligning the span for a real subsequent string literal and causing its content to be incorrectly space-inserted instead of unicode-escaped -- silently changing the serialized value (e.g. `</script ` became `< /script ` for `/'/.test(x) ? '</script ' : 'ok'`). Add regression test for the regex-literal quote case; 92/92 tests pass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 0122baf commit e26ddc7

2 files changed

Lines changed: 48 additions & 16 deletions

File tree

index.js

Lines changed: 34 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -54,36 +54,54 @@ function escapeUnsafeChars(unsafeChar) {
5454
return ESCAPED_CHARS[unsafeChar];
5555
}
5656

57-
// Matches string literals, template literals, and comments so that
58-
// `escapeFunctionBody` can tell them apart from plain code (see below).
59-
// This is a lightweight heuristic, not a full parser: a whole template
60-
// literal (backtick to backtick) is treated as one opaque span, including
61-
// any `${...}` substitutions inside it. Known limitation: if a `</script`
62-
// sequence appears *inside* such a substitution (which is actual code, e.g.
63-
// `` `${ x</script/.test(x) }` ``), it will be misidentified as
64-
// string/template content and unicode-escaped, which can still produce a
65-
// SyntaxError. This is considered an acceptable trade-off for keeping this
66-
// scan simple; none of our tests hit this narrower case.
67-
var STRING_OR_COMMENT_REGEXP = /\/\*[\s\S]*?\*\/|\/\/[^\n]*|'(?:\\.|[^'\\])*'|"(?:\\.|[^"\\])*"|`(?:\\.|[^`\\])*`/g;
57+
// Matches string literals, template literals, regex literals, and comments
58+
// so that `escapeFunctionBody` can tell them apart from plain code (see
59+
// below). This is a lightweight heuristic, not a full parser:
60+
// - A whole template literal (backtick to backtick) is treated as one
61+
// opaque span, including any `${...}` substitutions inside it. Known
62+
// limitation: if a `</script` sequence appears *inside* such a
63+
// substitution (which is actual code, e.g. `` `${ x</script/.test(x) }` ``),
64+
// it will be misidentified as string/template content and
65+
// unicode-escaped, which can still produce a SyntaxError. This is
66+
// considered an acceptable trade-off for keeping this scan simple; none
67+
// of our tests hit this narrower case.
68+
// - The regex-literal alternative can't reliably distinguish a real regex
69+
// literal from a division expression (e.g. `a / b / c`), since that
70+
// requires knowing the preceding token. It's included primarily so a
71+
// quote character inside a genuine regex literal (e.g. `/'/`) isn't
72+
// mistaken for the start of a string, which would misalign every
73+
// subsequent string match; a division expression that happens to match
74+
// this pattern is merely treated as opaque, which only risks an
75+
// unnecessary (but still valid) unicode-escape rather than a
76+
// miscalculated string boundary.
77+
var STRING_OR_COMMENT_REGEXP = /\/\*[\s\S]*?\*\/|\/\/[^\n]*|'(?:\\.|[^'\\])*'|"(?:\\.|[^"\\])*"|`(?:\\.|[^`\\])*`|\/(?:\\.|[^\/\\\n])+\//g;
6878

6979
// Escape function body for XSS protection while preserving arrow function
7080
// syntax (=>), comparison operators, and regex literals: only script end
7181
// tags and line terminators are escaped.
7282
function escapeFunctionBody(str) {
7383
// Record the [start, end) span of every string literal, template
74-
// literal, and comment so matches inside them can be treated
75-
// differently from matches in plain code (see below).
84+
// literal, regex literal, and comment so matches inside them can be
85+
// treated differently from matches in plain code (see below).
7686
var stringAndCommentSpans = [];
7787
var match;
7888
STRING_OR_COMMENT_REGEXP.lastIndex = 0;
7989
while ((match = STRING_OR_COMMENT_REGEXP.exec(str))) {
8090
stringAndCommentSpans.push([match.index, match.index + match[0].length]);
8191
}
8292

93+
// Both the script-close matches (found below, in source order via
94+
// `replace`) and `stringAndCommentSpans` are ordered by offset, so a
95+
// single forward-moving cursor is enough to classify every match in
96+
// O(n) total instead of re-scanning every span for every match.
97+
var spanCursor = 0;
98+
8399
str = str.replace(SCRIPT_CLOSE_REGEXP, function(scriptCloseMatch, offset) {
84-
var inStringOrComment = stringAndCommentSpans.some(function(span) {
85-
return offset >= span[0] && offset < span[1];
86-
});
100+
while (spanCursor < stringAndCommentSpans.length && stringAndCommentSpans[spanCursor][1] <= offset) {
101+
spanCursor++;
102+
}
103+
var span = stringAndCommentSpans[spanCursor];
104+
var inStringOrComment = !!span && offset >= span[0] && offset < span[1];
87105
if (!inStringOrComment) {
88106
// Outside of strings/templates/comments, `<` and `/` are real
89107
// JavaScript tokens (a comparison operator, a regex literal

test/unit/serialize.js

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -696,6 +696,20 @@ describe('serialize( obj )', function () {
696696
strictEqual(deserialized('script'), fn('script'));
697697
strictEqual(deserialized('other'), fn('other'));
698698
});
699+
700+
it('should not let a quote inside a regex literal misalign a later string literal', function () {
701+
// The quote in `/'/` must not be mistaken for the start of a
702+
// string; otherwise the real string below is misidentified and
703+
// its `</script ` payload is left unescaped as plain code.
704+
function fn(x) { return /'/.test(x) ? '</script ' : 'ok'; }
705+
var serialized = serialize(fn);
706+
707+
strictEqual(/<\/script[\t\n\f\r \/>]/i.test(serialized), false);
708+
709+
var deserialized; eval('deserialized = ' + serialized);
710+
strictEqual(deserialized("'"), fn("'"));
711+
strictEqual(deserialized('x'), fn('x'));
712+
});
699713
});
700714

701715
describe('options', function () {

0 commit comments

Comments
 (0)