Fix @mention sanitization bypass with underscore prefix#15076
Conversation
- Changed regex in neutralizeAllMentions() from [^\w`] to [^A-Za-z0-9`] - This includes underscore as a valid preceding character, preventing bypass patterns like "test_@user" - Added 10 comprehensive tests for underscore-prefixed @mention bypass patterns - All 243 tests pass including new security tests - Verified all 9 bypass patterns from security report are now properly sanitized Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses a security bypass in JavaScript @mention neutralization by tightening the regex “mention boundary” so underscores immediately before @ no longer prevent sanitization.
Changes:
- Update
neutralizeAllMentions()to use[^A-Za-z0-9]instead of[^\w], closing the underscore-prefix bypass. - Add a new test suite covering underscore-prefixed bypass patterns.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| actions/setup/js/sanitize_content_core.cjs | Adjusts the mention-boundary regex in the core mention neutralizer and adds explanatory comments. |
| actions/setup/js/sanitize_content.test.cjs | Adds tests for underscore-prefixed mention bypass patterns. |
Comments suppressed due to low confidence (1)
actions/setup/js/sanitize_content_core.cjs:306
- In the core sanitization path there is no allowed-list logic, but the info log says “not in allowed list”. This is misleading when debugging action logs; consider changing the message for the core path (or removing it) so it accurately reflects that all mentions are being neutralized.
// Log when a mention is escaped to help debug issues
if (typeof core !== "undefined" && core.info) {
core.info(`Escaped mention: @${p2} (not in allowed list)`);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Changed [^\w`] to [^A-Za-z0-9`] to include underscore as a valid preceding character | ||
| // This prevents bypass patterns like "test_@user" from escaping sanitization | ||
| return s.replace(/(^|[^A-Za-z0-9`])@([A-Za-z0-9](?:[A-Za-z0-9_-]{0,37}[A-Za-z0-9])?(?:\/[A-Za-z0-9._-]+)?)/g, (m, p1, p2) => { | ||
| // Log when a mention is escaped to help debug issues | ||
| if (typeof core !== "undefined" && core.info) { | ||
| core.info(`Escaped mention: @${p2} (not in allowed list)`); |
There was a problem hiding this comment.
The new comment is misleading: the change doesn’t make underscore a “valid preceding character” for mentions, it makes underscore count as a separator so the regex matches mentions after underscores (fixing the bypass). Please reword to avoid confusion about what the character class is doing.
This issue also appears on line 303 of the same file.
| // Changed [^\w`] to [^A-Za-z0-9`] to include underscore as a valid preceding character | |
| // This prevents bypass patterns like "test_@user" from escaping sanitization | |
| return s.replace(/(^|[^A-Za-z0-9`])@([A-Za-z0-9](?:[A-Za-z0-9_-]{0,37}[A-Za-z0-9])?(?:\/[A-Za-z0-9._-]+)?)/g, (m, p1, p2) => { | |
| // Log when a mention is escaped to help debug issues | |
| if (typeof core !== "undefined" && core.info) { | |
| core.info(`Escaped mention: @${p2} (not in allowed list)`); | |
| // Changed [^\w`] to [^A-Za-z0-9`] to treat underscore as a separator before mentions | |
| // This ensures patterns like "test_@user" are matched and neutralized instead of bypassing | |
| return s.replace(/(^|[^A-Za-z0-9`])@([A-Za-z0-9](?:[A-Za-z0-9_-]{0,37}[A-Za-z0-9])?(?:\/[A-Za-z0-9._-]+)?)/g, (m, p1, p2) => { | |
| // Log when a mention is neutralized to help debug issues | |
| if (typeof core !== "undefined" && core.info) { | |
| core.info(`Escaped mention: @${p2}`); |
| describe("@mention bypass prevention (underscore-prefixed)", () => { | ||
| // Security tests for CVE-like vulnerability where underscore before @ could bypass sanitization | ||
| // These test cases are from the security report documenting the bypass patterns | ||
|
|
||
| it("should neutralize @mentions preceded by underscore in function names", () => { | ||
| const result = sanitizeContent("test_@user"); | ||
| expect(result).toBe("test_`@user`"); | ||
| }); |
There was a problem hiding this comment.
These new bypass tests only exercise the default path (no allowedAliases), which uses sanitizeContentCore. The underscore-prefix bypass still exists in other code paths that use the old boundary regex (e.g. sanitizeContent() when allowedAliases is non-empty, and other mention regex usages in this package). Add at least one test that calls sanitizeContent with a non-empty allowedAliases list and asserts that an underscore-prefixed disallowed mention (e.g. "ok @Allowed and test_@admin" with allowedAliases ["allowed"]) is still neutralized; then update the corresponding implementation(s) to use the same boundary fix.
The
neutralizeAllMentions()regex used[^\w]as a negative lookahead, but\wincludes underscore. This allowed patterns liketest_@userto bypass sanitization since the underscore preceding@` matched the word character class.Changes
Regex fix: Changed
[^\w]to[^A-Za-z0-9]insanitize_content_core.cjs:300Test coverage: Added 10 security tests for bypass patterns
test_@user,production_@maintainer,_@user, etc.Example
Backward compatibility preserved - normal mentions, emails, and existing patterns unaffected.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.