Fix safe-output inline-backtick neutralization - #54103
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: b591adab-6031-4a22-ac49-0755015a5014
There was a problem hiding this comment.
Pull request overview
Hardens safe-output Markdown sanitization against attacker-controlled backticks.
Changes:
- Adds render-safe code-span delimiters and boundary handling.
- Aligns fenced-code detection with CommonMark.
- Adds regression coverage and a patch changeset.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/sanitize_content.cjs |
Applies safe mention neutralization after truncation. |
actions/setup/js/sanitize_content_core.cjs |
Implements safe delimiters and code-region handling. |
actions/setup/js/sanitize_content.test.cjs |
Adds sanitizer regression tests. |
actions/setup/js/markdown_code_region_balancer.cjs |
Refines fence recognition. |
actions/setup/js/markdown_code_region_balancer.test.cjs |
Tests malformed fence handling. |
.changeset/patch-fix-safe-output-inline-backticks.md |
Adds the patch release note. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Balanced
| const usedDelimiterLengths = new Set(Array.from(s.matchAll(/`+/g), match => match[0].length)); | ||
| let delimiterLength = 1; | ||
| while (usedDelimiterLengths.has(delimiterLength)) { | ||
| delimiterLength++; | ||
| } | ||
| const delimiter = "`".repeat(delimiterLength); |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #54103 does not have the implementation label and has 0 new lines of code in business logic directories (threshold: 100). The 6 changed files are infrastructure/config files outside src/, lib/, pkg/, internal/, app/, core/, domain/, services/, or api/.
|
|
✅ Ponytail Reviewer completed successfully!
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
REQUEST_CHANGES — the sanitizer fix is headed in the right direction, but it introduces a fresh denial-of-service vector by making wrapper size attacker-controlled.
The blocking theme
createRenderSafeCodeSpanWrapper()picks the first backtick-run length absent from the entire input.- An attacker can force that missing length to be arbitrarily large by including runs of length 1..N in otherwise valid input.
- Every neutralized mention / command / bot trigger / repo reference then emits that huge delimiter twice, after the final truncation pass, so the output can grow far past the configured max length and trigger avoidable memory / payload blowups in the exact path that is supposed to make hostile content safe.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 10.2 AIC · ⌖ 8.02 AIC · ⊞ 7K
Comment /review to run again
| while (usedDelimiterLengths.has(delimiterLength)) { | ||
| delimiterLength++; | ||
| } | ||
| const delimiter = "`".repeat(delimiterLength); |
There was a problem hiding this comment.
createRenderSafeCodeSpanWrapper() makes the escape delimiter length attacker-controlled, so a crafted input containing backtick runs of length 1..N can force every later neutralization to append O(N) bytes twice and blow past the post-truncation size cap.
💡 Why this blocks the change
This helper scans the whole input, picks the first missing backtick-run length, and then reuses that delimiter for every wrapped mention / command / bot trigger / GitHub reference. Because mention neutralization now runs after applyTruncation(), the extra delimiter bytes are no longer capped by maxLength.
A hostile body can cheaply include `, , ``` `` ... up to some large N, making the chosen delimiter length N+1. From there, each sanitized token grows by 2 * (N + 1) bytes plus any added separators. A single comment with many mentions can therefore expand dramatically after the supposed final length check.
Please bound the generated delimiter length to a small constant and fall back to a non-markdown escape when that bound is exhausted, or re-run truncation after all wrapper insertions.
There was a problem hiding this comment.
Review: Fix safe-output inline-backtick neutralization
The approach is architecturally sound: choosing a delimiter length absent from the full input, deferring mention neutralization until after truncation, and threading before/after context through applyFnOutsideInlineCode are all correct solutions to the described attack vectors.
The two existing review comments identify the main remaining issues:
- Attacker-inflatable
delimiterLength— an input seeded with backtick runs of lengths 1...N forces N+1 delimiter length, emitted twice per token. A bounded cap (e.g. max delimiter length of 10) would prevent a malicious payload from bloating output size. - Changeset missing
---frontmatter — the.changeset/patch-fix-safe-output-inline-backticks.mdfile lacks the required---delimiters, so Changesets will silently ignore the version bump (release blocker).
Beyond those, the implementation looks correct: the before/after separator logic handles adjacent matched and unmatched code spans, count correctly accumulates across segments for neutralizeBotTriggers, and matchFenceLine correctly excludes the backtick-in-info-string edge case. The new tests cover the attack scenarios well.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 31.7 AIC · ⌖ 8.87 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Ponytail review: one over-engineering finding — duplicated boundary-char logic across four neutralize* functions. net: -18 lines possible.
Generated by ✂️ Ponytail Reviewer for #54103 · auto · 34.8 AIC · ⌖ 4.33 AIC · ⊞ 7.3K
Comment /ponytail to run again
Comments that could not be inline-anchored
actions/setup/js/sanitize_content_core.cjs:346
yagni: identical before/after boundary-char derivation (prefix || (offset === 0 ? regionBefore : "") / segment[offset+match.length] || (... regionAfter : "")) repeated in neutralizeMentions, neutralizeAllMentions, neutralizeBotTriggers, and neutralizeGitHubReferences. One shared helper, e.g. boundaryChars(segment, offset, matchLen, prefix, regionBefore, regionAfter), replaces all 4 copies.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness issues and a missing regression test.
📋 Key Themes & Highlights
Key Themes
- Unbounded delimiter loop (
createRenderSafeCodeSpanWrapper): an attacker who controls large input can force thedelimiterLengthloop to grow indefinitely, causing output amplification. A cap (e.g.<= 10) and a test would close this. - Mention-regex inconsistency:
neutralizeAllMentionsuses[^A-Za-z0-9]whileneutralizeMentionsuses[^\w]—_@useris treated differently in the two paths. Aligning them closes a quiet divergence. - Missing edge-case test: the
{0,3}indentation fix inmatchFenceLineis a spec-correct improvement, but a 4-space indent test is absent; without it the guard could silently regress.
Positive Highlights
- ✅ Excellent root-cause analysis in the PR description — the re-pairing attack is clearly explained
- ✅
applyToNonCodeRegionsboundary-context threading (before/after) is a clean, well-scoped change - ✅ Moving mention neutralization after truncation correctly eliminates the delimiter-splitting window
- ✅ Strong test coverage across mentions, bot triggers, references, truncation, and code-span boundaries
- ✅
matchFenceLineextraction removes duplicated regex and adds the CommonMark backtick-in-info-string check in one place
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 119.3 AIC · ⌖ 10.2 AIC · ⊞ 7.8K
Comment /matt to run again
| return applyToNonCodeRegions(s, (segment, regionBefore = "", regionAfter = "") => { | ||
| // No filtering - all mentions outside matched code regions are neutralized. | ||
| // Use an explicit ASCII class so underscores before mentions remain covered. | ||
| return segment.replace(/(^|[^A-Za-z0-9])@([A-Za-z0-9](?:[A-Za-z0-9_-]{0,37}[A-Za-z0-9])?(?:\/[A-Za-z0-9._-]+)?)/g, (match, prefix, alias, offset) => { |
There was a problem hiding this comment.
[/diagnosing-bugs] Regex inconsistency: neutralizeAllMentions (core) uses [^A-Za-z0-9] as the non-word guard, but neutralizeMentions in sanitize_content.cjs uses [^\w]. Because \w includes _, an underscore-prefixed mention like _@octocat is treated as a live mention in the filtered path but not in the core path — a silent, hard-to-spot divergence.
💡 Suggested fix
Decide on one policy (e.g. [^A-Za-z0-9_] or [^\w]) and apply it in both places. Then add a regression test:
it('should neutralize a mention preceded by an underscore', () => {
expect(sanitizeContent('foo_@octocat bar')).toBe('foo_ ``@octocat`` bar');
});This removes the inconsistency and documents the intended boundary rule.
@copilot please address this.
| * @returns {(text: string, before?: string, after?: string) => string} A render-safe inline-code wrapper | ||
| */ | ||
| function createRenderSafeCodeSpanWrapper(s) { | ||
| const usedDelimiterLengths = new Set(Array.from(s.matchAll(/`+/g), match => match[0].length)); |
There was a problem hiding this comment.
[/diagnosing-bugs] createRenderSafeCodeSpanWrapper scans the pre-mutation string s for backtick run lengths. If s contains attacker-controlled content that uses every short length (e.g. \`` and ``and```), the function enters an unbounded loop until it finds an absent length. An adversary who controls large inputs can make delimiterLength` grow arbitrarily, producing extremely long delimiters in every neutralized token — this is a potential output-amplification vector.
💡 Suggested fix
Cap delimiterLength to a reasonable maximum (e.g. 5 or 10):
while (usedDelimiterLengths.has(delimiterLength) && delimiterLength <= 10) {
delimiterLength++;
}Add a regression test:
it('should cap delimiter length when all short delimiters are present in input', () => {
const input = '` `` ``` ```` @octocat';
const result = sanitizeContent(input);
// delimiter should not exceed a reasonable max
expect(result).not.toMatch(/`{6,}/);
});@copilot please address this.
| * @param {string} line | ||
| * @returns {RegExpMatchArray | null} | ||
| */ | ||
| function matchFenceLine(line) { |
There was a problem hiding this comment.
[/tdd] matchFenceLine now correctly limits leading indentation to {0,3} spaces per CommonMark spec (the old \s* was over-permissive), but there is no regression test covering the 4-space case. A line indented by 4+ spaces should not be treated as a fence and should be left untouched.
💡 Suggested test
it('should not treat a 4-space indented line as a fence', () => {
const input = ' ```\ncode\n ```';
expect(balancer.isBalanced(input)).toBe(true);
expect(balancer.balanceCodeRegions(input)).toBe(input);
});@copilot please address this.
|
@copilot This PR still needs forward progress before maintainer pickup. Open maintainer-visible blockers:
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the sanitizer review blockers in db2f9bc: delimiter growth now falls back to escaped HTML code elements, the changeset has valid frontmatter, and parser regressions cover delimiter exhaustion and four-space-indented fences. Targeted sanitizer tests pass; the reported CI shard failure was an unrelated git-helper test failure. |
|
@copilot This PR still needs a forward-progress pass.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Completed the forward-progress pass in 9966059. The remaining mention-boundary inconsistency is aligned with the core sanitizer, four-space-indented fence coverage was added, and the bounded delimiter fallback plus valid changeset frontmatter remain in place. Targeted JS validation and repository lint/format checks pass; the prior CI shard failure is unrelated git-helper fixture corruption and needs a maintainer-triggered rerun. |
|
@copilot quick triage for this PR:
|
Forward-progress pass complete. The branch is already refreshed through |
|
@copilot the current head is already code-complete, but maintainer-visible forward progress is still blocked by workflow state.
|
PR finisher pass complete. Branch refresh is present at |
Summary
Safe-output sanitization neutralized mentions, GitHub references, bot triggers, and commands by wrapping them in fixed single-backtick code spans. Attacker-controlled backticks could re-pair with those inserted delimiters during GitHub Markdown rendering and leave the supposedly inert token live.
Severity
Low. Exploitation requires attacker-controlled content to be reproduced by an agent into a write-enabled safe output. Demonstrated impact is unwanted mentions, cross-references, or trigger text; this does not enable privilege escalation, secret disclosure, or arbitrary code execution.
Solution
Validation
/markdownrenderer keeps the reported payload variants inertAddresses githubnext/gh-aw-security#3081.
Run: https://github.com/github/gh-aw/actions/runs/32334226502> Generated by 👨🍳 PR Sous Chef · gpt54 · 27.9 AIC · ⌖ 8.81 AIC · ⊞ 9.3K · ◷