From 78879b55f844bc2e33f64dcc417bb5ec58d9e48c Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Tue, 7 Jul 2026 16:41:09 -0700 Subject: [PATCH] fix(review): harden impact map path markdown --- src/review/unified-comment-bridge.ts | 22 ++++++++-------- test/unit/impact-map-collapsible.test.ts | 32 ++++++++++++++++++------ 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index 78337f99f3..1f975dbf21 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -571,17 +571,19 @@ export type ImpactMapSummaryInput = { changedModule: string; affectedModules: st * as a trailing "+N more" instead of silently truncating with no indication more exist. */ const MAX_RENDERED_AFFECTED_MODULES = 5; -/** Public-safe inline-code escaping for a file path cell — mirrors `buildBeforeAfterCollapsible`'s - * `markdownCode`: backtick/backslash/pipe/angle-bracket neutralized so an adversarial path can't break out of - * the table or the inline-code span. Impact-map paths originate from the repo's own RAG-indexed file tree - * (never raw user input), but this is defense-in-depth, matching the discipline every other path-rendering - * helper in this file already applies. */ +/** Public-safe inline-code rendering for a file path table cell. Impact-map paths can include + * contributor-controlled filenames, so choose a code-span delimiter longer than any run inside the + * value instead of trying to backslash-escape backticks (Markdown does not honor that inside code spans). + * Normalize row-breaking controls and entity-escape table/HTML metacharacters before wrapping. */ function markdownPathCode(value: string): string { - return `\`${value - .replace(/\\/g, "\\\\") - .replace(/`/g, "\\`") - .replace(/\|/g, "\\|") - .replace(/[<>]/g, (char) => (char === "<" ? "<" : ">"))}\``; + const safeValue = value + .replace(/[\r\n]+/g, " ") + .replace(/[\u0000-\u0008\u000B\u000C\u000E-\u001F\u007F]/g, " ") + .replace(/\|/g, "|") + .replace(/[<>]/g, (char) => (char === "<" ? "<" : ">")); + const longestBacktickRun = Math.max(0, ...Array.from(safeValue.matchAll(/`+/g), (match) => match[0].length)); + const delimiter = "`".repeat(longestBacktickRun + 1); + return `${delimiter} ${safeValue} ${delimiter}`; } /** diff --git a/test/unit/impact-map-collapsible.test.ts b/test/unit/impact-map-collapsible.test.ts index d3721d1f19..7f61431bb7 100644 --- a/test/unit/impact-map-collapsible.test.ts +++ b/test/unit/impact-map-collapsible.test.ts @@ -30,24 +30,24 @@ describe("buildImpactMapCollapsible (#2185)", () => { expect(c).not.toBeNull(); expect(c?.title).toBe("Impact map"); expect(c?.body).toContain("| Changed module | Symbols | Plausibly affected |"); - expect(c?.body).toContain("`src/review/impact-map.ts`"); + expect(c?.body).toContain("` src/review/impact-map.ts `"); expect(c?.body).toContain("computeImpactMap"); - expect(c?.body).toContain("`src/review/impact-map-wire.ts`"); - expect(c?.body).toContain("`src/queue/processors.ts`"); + expect(c?.body).toContain("` src/review/impact-map-wire.ts `"); + expect(c?.body).toContain("` src/queue/processors.ts `"); }); it("renders a dash for callers when a row somehow has none", () => { const c = buildImpactMapCollapsible([{ changedModule: "src/a.ts", affectedModules: ["src/b.ts"], callers: [] }]); - expect(c?.body).toContain("| `src/a.ts` | — |"); + expect(c?.body).toContain("| ` src/a.ts ` | — |"); }); it("caps rendered affected modules with a '+N more' overflow line", () => { const many = Array.from({ length: 8 }, (_, i) => `src/caller${i}.ts`); const c = buildImpactMapCollapsible([{ changedModule: "src/a.ts", affectedModules: many, callers: ["a"] }]); const body = c?.body ?? ""; - expect(body).toContain("`src/caller0.ts`"); - expect(body).toContain("`src/caller4.ts`"); - expect(body).not.toContain("`src/caller5.ts`"); + expect(body).toContain("` src/caller0.ts `"); + expect(body).toContain("` src/caller4.ts `"); + expect(body).not.toContain("src/caller5.ts"); expect(body).toContain("(+3 more)"); }); @@ -58,7 +58,23 @@ describe("buildImpactMapCollapsible (#2185)", () => { it("escapes a hostile-looking path so it can't break out of the table/inline-code span", () => { const c = buildImpactMapCollapsible([{ changedModule: "src/`weird|.ts", affectedModules: ["src/b.ts"], callers: ["a"] }]); - expect(c?.body).toContain("src/\\`weird\\|<path>.ts"); + expect(c?.body).toContain("`` src/`weird|<path>.ts ``"); + }); + + it("normalizes path newlines so attacker markdown stays in the impact-map table row", () => { + const c = buildImpactMapCollapsible([ + { + changedModule: "src/safe`\r\n\n### fake bot guidance\n[run patch](https://evil.example/pwn).ts", + affectedModules: ["src/also`unsafe\n|spoof.ts"], + callers: ["a"], + }, + ]); + const body = c?.body ?? ""; + + expect(body).toContain("`` src/safe` ### fake bot guidance [run patch](https://evil.example/pwn).ts ``"); + expect(body).toContain("`` src/also`unsafe |spoof.ts ``"); + expect(body).not.toContain("\n### fake bot guidance"); + expect(body).not.toContain("\n|spoof.ts"); }); it("returns null for an empty entry list (no empty table)", () => {