Skip to content

Commit 2bdbaaf

Browse files
redonkulusclaude
andauthored
Merge commit from fork
* fix: prevent script close tag from being swallowed by a single match `SCRIPT_CLOSE_REGEXP` matched `<\/script[^>]*>`, whose wildcard could run from one `</script` to the next `>` anywhere in the function source. When a second, complete `</script>` fell inside that span, the whole thing collapsed into one match, and since the plain-code branch neutralizes only the leading `<`, the swallowed tag was re-emitted verbatim. A function body reaches that shape whenever `</script` appears in code position -- `x</script=+/` parses as `x < /script=+/`, a comparison against a regex literal -- followed by a `</script>` in a later string. The serialized output then carries a live `</script>`, which terminates the script element when embedded the way the README documents, so the rest of the payload is parsed as HTML. Confirmed in headless Chromium: the injected `onerror` runs. This regressed in v7.1.0. v7.0.7 used the same wildcard but escaped the entire match, so nothing survived. Excluding `<` from the character class fixes it: a match can no longer reach past a second `<`, so every `</script` in the source either starts its own match or is followed by a non-delimiter -- and the HTML tokenizer only ends the tag name on TAB, LF, FF, CR, SPACE, `/` or `>`, emitting anything else as text. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: reject spoofed function toString() and make native-code check stateless Two defects in `serializeFunc`, found while investigating the script close tag escaping. `fn.toString()` was trusted to return a string. It is attacker-controlled in the same way `URL.prototype.toString` and `RegExp.prototype.source` were before they were hardened. Returning an object with its own `replace()` is enough to defeat the escaping outright, because `escapeFunctionBody()` is built entirely from `str.replace(...)` calls -- the object's `replace` simply returns itself, and the payload reaches the output untouched: var f = function () {}; f.toString = () => ({ replace: function () { return this; }, toString: () => 'function(){}</script><img src=x onerror=alert(1)>' }); serialize({ f: f }); // {"f":function(){}</script><img src=x onerror=alert(1)>} A primitive string from a spoofed `toString()` was already safe; only the non-string case bypasses escaping. Rejecting it matches how the URL and RegExp spoofing cases are already handled. Separately, `IS_NATIVE_CODE_REGEXP` carried a `/g` flag. `.test()` on a global regexp advances `lastIndex`, so after one rejection the next call began its scan past the `[native code]` match and returned false, letting every other native function through: try { serialize(Math.max); } catch (e) {} // correctly throws serialize(Math.min); // 'function min() { [native code] }' That output is a syntax error rather than an injection, so the impact is limited to an unreliable guard, but the flag serves no purpose here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 8c8caa7 commit 2bdbaaf

2 files changed

Lines changed: 115 additions & 10 deletions

File tree

‎index.js‎

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,20 +11,34 @@ var UID_LENGTH = 16;
1111
var UID = generateUID();
1212
var PLACE_HOLDER_REGEXP = new RegExp('(\\\\)?"@__(F|R|D|M|S|A|U|I|B|L)-' + UID + '-(\\d+)__@"', 'g');
1313

14-
var IS_NATIVE_CODE_REGEXP = /\{\s*\[native code\]\s*\}/g;
14+
// Not global: `.test()` on a `/g` regexp advances `lastIndex`, so a second
15+
// call could start past a `[native code]` match and wrongly report a native
16+
// function as safe to serialize.
17+
var IS_NATIVE_CODE_REGEXP = /\{\s*\[native code\]\s*\}/;
1518
var IS_PURE_FUNCTION = /function.*?\(/;
1619
var IS_ARROW_FUNCTION = /.*?=>.*?/;
1720
var UNSAFE_CHARS_REGEXP = /[<>\/\u2028\u2029]/g;
1821
// Matches a script end tag (case-insensitive) for XSS protection: either a
19-
// full `</script...>` tag, or a bare `</script` followed by one of the
20-
// characters (TAB, LF, FF, CR, SPACE, `/`, `>`) that the HTML tokenizer
21-
// treats as ending the tag name (see the WHATWG "script data end tag name
22-
// state"). The bare-prefix form matters because the matching `>` could be
23-
// supplied by a different serialized value later in the output, so escaping
24-
// stops there without waiting for a closing `>`. A trailing backslash is not
25-
// a delimiter here (that's a JS-level concern, not an HTML one), so this
26-
// doesn't affect tagged template literals like `String.raw`.
27-
var SCRIPT_CLOSE_REGEXP = /<\/script[^>]*>|<\/script(?=[\t\n\f\r \/>])/gi;
22+
// literal `</script>`, or a bare `</script` followed by one of the other
23+
// characters (TAB, LF, FF, CR, SPACE, `/`) that the HTML tokenizer treats as
24+
// ending the tag name (see the WHATWG "script data end tag name state"). Any
25+
// other following character means the tokenizer emits `</script` as text and
26+
// stays in script data, so only these need neutralizing. The bare-prefix form
27+
// matters because the matching `>` could be supplied by a different
28+
// serialized value later in the output, so escaping stops there without
29+
// waiting for a closing `>`. A trailing backslash is not a delimiter here
30+
// (that's a JS-level concern, not an HTML one), so this doesn't affect tagged
31+
// template literals like `String.raw`.
32+
//
33+
// The `[^<>]*` in the first alternative excludes `<`, which is load-bearing:
34+
// it was previously `[^>]*`, letting a single match run from one `</script`
35+
// to the next `>` anywhere in the source and swallow a complete `</script>`
36+
// in between. Only one replacement is emitted per match, so the swallowed tag
37+
// survived verbatim in the output (PSECBUGS-117112). Because a match can no
38+
// longer contain a second `<`, every `</script` in the source is either the
39+
// start of its own match or is followed by a non-delimiter — and the latter
40+
// the tokenizer never treats as an end tag.
41+
var SCRIPT_CLOSE_REGEXP = /<\/script[^<>]*>|<\/script(?=[\t\n\f\r \/>])/gi;
2842

2943
var RESERVED_SYMBOLS = ['*', 'async'];
3044

@@ -206,6 +220,14 @@ module.exports = function serialize(obj, options) {
206220

207221
function serializeFunc(fn, options) {
208222
var serializedFn = fn.toString();
223+
224+
// A spoofed `toString()` can return a non-string whose `replace()` is
225+
// attacker-controlled, which would let it pass through
226+
// `escapeFunctionBody()` unescaped and inject markup into the output.
227+
if (typeof serializedFn !== 'string') {
228+
throw new TypeError('Function.toString() must return a string');
229+
}
230+
209231
if (IS_NATIVE_CODE_REGEXP.test(serializedFn)) {
210232
throw new TypeError('Serializing native function: ' + fn.name);
211233
}

‎test/unit/serialize.js‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -121,6 +121,45 @@ describe('serialize( obj )', function () {
121121
strictEqual(err instanceof TypeError, true);
122122
});
123123

124+
it('should keep rejecting native built-ins after an earlier rejection', function () {
125+
// The native-code check must not carry `lastIndex` state between
126+
// calls, or every other native function would slip through.
127+
throws(function () { serialize(Math.max); }, TypeError);
128+
throws(function () { serialize(Math.min); }, TypeError);
129+
throws(function () { serialize(Math.max); }, TypeError);
130+
});
131+
132+
it('should throw when serializing a function with a spoofed non-string toString()', function () {
133+
var fn = function () {};
134+
fn.toString = function () {
135+
return {
136+
toString: function () { return 'function(){}'; }
137+
};
138+
};
139+
throws(function () { serialize({ fn: fn }); }, TypeError);
140+
});
141+
142+
it('should throw when a spoofed toString() returns an object with its own replace()', function () {
143+
// Without a type check, `escapeFunctionBody()` would call this
144+
// object's `replace()`, which returns the payload unescaped.
145+
var fn = function () {};
146+
fn.toString = function () {
147+
return {
148+
replace: function () { return this; },
149+
toString: function () {
150+
return 'function(){}</script><img src=x onerror=alert(1)>';
151+
}
152+
};
153+
};
154+
throws(function () { serialize({ fn: fn }); }, TypeError);
155+
});
156+
157+
it('should escape script close tags from a spoofed string toString()', function () {
158+
var fn = function () {};
159+
fn.toString = function () { return 'function(){}</script>'; };
160+
strictEqual(serialize({ fn: fn }).indexOf('</script>'), -1);
161+
});
162+
124163
it('should serialize enhanced literal objects', function () {
125164
var obj = {
126165
foo() { return true; },
@@ -604,6 +643,50 @@ describe('serialize( obj )', function () {
604643
strictEqual(deserialized(), '</script>');
605644
});
606645

646+
// A `</script` only ends the script element when the next character
647+
// is one the HTML tokenizer accepts as ending the tag name. Followed
648+
// by anything else it is emitted as text, so that is what must be
649+
// absent from the output -- not every literal `</script` substring.
650+
var CLOSES_SCRIPT = /<\/script[\t\n\f\r \/>]/i;
651+
652+
it('should not leak a second script close tag swallowed by the first match (PSECBUGS-117112)', function () {
653+
// `x</script=+/` parses as `x < /script=+/` (a comparison against
654+
// a regex literal), so `</script` appears in plain-code position
655+
// with no `>` after it until the one inside the later string. The
656+
// first regex alternative used to match across that whole span in
657+
// one go, emitting the inner `</script>` raw.
658+
var src = "function f(x){ return x</script=+/ + '</script><img src=x onerror=alert(1)>' }";
659+
var fn = new Function('return ' + src)();
660+
var serialized = serialize({ h: fn });
661+
662+
strictEqual(CLOSES_SCRIPT.test(serialized), false);
663+
// The inner tag is inside a string literal, so it unicode-escapes.
664+
strictEqual(serialized.includes('\\u003C\\u002Fscript\\u003E'), true);
665+
});
666+
667+
it('should escape every script close tag when several appear in one function', function () {
668+
var src = "function f(x){ return x</script=+/ + '</script>' + '</script>' + '</script >' }";
669+
var fn = new Function('return ' + src)();
670+
var serialized = serialize({ h: fn });
671+
672+
strictEqual(CLOSES_SCRIPT.test(serialized), false);
673+
});
674+
675+
it('should not let a script close tag straddle plain code and a string', function () {
676+
// The `</script` in code position and the one in the string must
677+
// be classified separately, since only the latter can safely be
678+
// rewritten as unicode escapes.
679+
var src = "function f(x){ return x</script/ + '</script>' }";
680+
var fn = new Function('return ' + src)();
681+
var serialized = serialize({ h: fn });
682+
683+
strictEqual(CLOSES_SCRIPT.test(serialized), false);
684+
// Code position: neutralized with a space, which parses the same.
685+
strictEqual(serialized.includes('< /script/'), true);
686+
// String position: unicode-escaped, preserving the runtime value.
687+
strictEqual(serialized.includes('\\u003C\\u002Fscript\\u003E'), true);
688+
});
689+
607690
it('should encode unsafe HTML chars in arrow function bodies', function () {
608691
var fn = () => { return '</script>'; };
609692
var serialized = serialize(fn);

0 commit comments

Comments
 (0)