Skip to content

Add regression coverage for script close variants - #219

Open
TorMatzAndren wants to merge 2 commits into
yahoo:mainfrom
TorMatzAndren:security/script-close-variant-regressions
Open

TorMatzAndren wants to merge 2 commits into
yahoo:mainfrom
TorMatzAndren:security/script-close-variant-regressions

Conversation

@TorMatzAndren

Copy link
Copy Markdown

This adds regression coverage for several script-closing variants inside serialized function bodies.

The existing XSS tests already cover common </script> cases. This expands coverage to mixed-case tags, whitespace variants, and attribute-like variants while also verifying that the serialized function still deserializes to the original payload.

Tested locally with:

npm test

@TorMatzAndren TorMatzAndren changed the title Add regression coveraAdd regression coverage for script close variantsge for script close variants Add regression coverage for script close variantsge for script close variants May 15, 2026
@TorMatzAndren TorMatzAndren changed the title Add regression coverage for script close variantsge for script close variants Add regression coverage for script close variants May 15, 2026
@redonkulus

Copy link
Copy Markdown
Collaborator

Thanks for this — the extra coverage (mixed-case tags, </script/x>, and the round-trip check through deserialization) is still useful, and all of these assertions pass on current main. Two things before we can merge:

1. Please rebase onto main. Several script-close test blocks have landed since this was opened, so the branch currently conflicts in test/unit/serialize.js. Placing the new test alongside the other </script cases in that describe block is fine.

2. The whitespace payloads aren't testing whitespace. In a JS string literal, '</script\\t>' is a literal backslash followed by t, not a tab — same for \\n, \\r, and \\f. They still pass, but they're exercising a backslash rather than the TAB/LF/CR/FF characters the HTML tokenizer treats as ending the tag name, which are the cases that matter. Could you change them to single backslashes:

'</script\t>',
'</script\n>',
'</script\r>',
'</script\f>',

Optional: the three includes() checks could be collapsed into one case-insensitive assertion, which also covers other casings like </sCrIpT:

strictEqual(/<\/script/i.test(serialized), false);

Happy to merge once those are in. Thanks again!

@redonkulus redonkulus closed this Oct 23, 2026
@TorMatzAndren

Copy link
Copy Markdown
Author

Follow-up to #219, addressing the maintainer's October 22 review.

Rebased the regression coverage onto current main (v7.1.4) and retained the useful cases not already explicitly covered by existing tests.

The updated test covers mixed-case script-closing tags, slash-delimited variants, and actual TAB/LF/CR/FF whitespace characters. It also uses a case-insensitive assertion to verify that serialized output contains no unsafe script-closing sequence, while checking that deserialization preserves the original payload.

Only test/unit/serialize.js is modified.

Verification: npm test — 119 passed, 0 failed.

Thanks for the review and guidance!

@redonkulus redonkulus reopened this Oct 23, 2026
@redonkulus

Copy link
Copy Markdown
Collaborator

@TorMatzAndren Do you have an updated commit to push?

@TorMatzAndren
TorMatzAndren force-pushed the security/script-close-variant-regressions branch from 6e5b830 to 2c3bae1 Compare October 24, 2026 08:14
@TorMatzAndren

Copy link
Copy Markdown
Author

Thanks for reopening #219!

I've now updated the original PR branch against current main, corrected the whitespace payloads to use actual TAB/LF/CR/FF characters, and simplified the case-insensitive assertion.

The full test suite passes (119/119). The redundant coverage introduced upstream has also been removed.

Thanks again for taking another look!

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.

2 participants