fix(mcp): board_escalate options bypass + escape decision-comment markdown (#300) - #301
Merged
Merged
Conversation
…kdown (#300) board_escalate pipe-joined its options array and escalate.mjs re-split on '|', so an option element with an embedded '|' reconstituted into extra options downstream — defeating the maxItems:20 bound and injecting fabricated options into the human-facing decision comment. - Pass options as a structured array end-to-end. The MCP handler hands args.options straight to runEscalate; normalizeOptions only splits the string form, preserving the CLI --options "a|b|c" contract. An array element's embedded '|' now stays literal and cannot fabricate options. - Escape caller-supplied reason/context/options with escapeMd before they land in the trusted Markdown decision comment, so they cannot forge block structure, a fake "recommended" tag, or a <!-- forge:... --> marker. AC300.1 embedded-'|' element cannot inflate option count (MCP + engine tests). AC300.2 CLI --options "a|b|c" still parses to [a,b,c] (regression). AC300.3 injected Markdown/HTML-comment structure is neutralized in the comment. Closes #300 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SATRHKa6mDHDuirhP6QuwL
…ollow-up) escapeMd escaped '-' (blocking a forged Setext H2) but not '=', so an embedded-newline payload like "text\n===" still forged a Setext H1 heading in the human-facing decision comment. Add '=' to the escaped class so neither setext underline survives. From the branch security pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SATRHKa6mDHDuirhP6QuwL
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #300
Follow-up from #296's security review (parent epic #174). Fixes both findings on the halt-and-ask escalation flow.
Findings fixed
board_escalate.options'|'-delimiter bypass. The MCP handler pipe-joined itsoptionsarray andescalate.mjsre-split on'|', so an option element containing an embedded'|'reconstituted into extra options downstream — defeating themaxItems: 20bound and injecting fabricated options into the human-facing GitHub decision comment.reason/context/optionswere interpolated unescaped into the trusted Markdown decision comment.Approach
board_escalatehandler now passesargs.optionsstraight through torunEscalate(no pipe-join). NewnormalizeOptionssplits only the string form, so the CLI--options "a|b|c"contract is preserved while an array element's embedded'|'stays literal and cannot fabricate options. The schema'smaxItems: 20bound now genuinely holds.escapeMdescapes backslash + Markdown/HTML control punctuation onreason/context/optionsbefore they render, so caller text cannot forge block structure, a fake "recommended" tag, or a<!-- forge:... -->marker. Recommend-matching stays on the raw option values.Acceptance criteria
'|'cannot fabricate extra downstream options — structured pass-through makes it impossible (MCP-handler test + engine test assert the count holds).--options "a|b|c"form still parses into exactly[a, b, c](regression test).reason/context/optionsare escaped in the decision comment; injected Markdown/HTML-comment structure is neutralized (test).Verification
pnpm verifygreen locally: 547 passed (50 files).tests/escalate.test.mjs(AC300.1/.2/.3 +escapeMdunit) and updatedtests/mcp-forge/forge-core.test.mjs(structured pass-through + AC300.1 at the MCP surface).🤖 Generated with Claude Code