Skip to content

Commit 2e1bbcb

Browse files
easonLiangWorldedtecheasonLiangWorldedtech
committed
fix(webview): reject encoded traversal separators and pin mention/link overlap
Thread triage on #1763 (edelauna @02:18:43Z): 1. The "fully inside or outside" claim is real: the mention regex's path branch runs to the next whitespace, so @/src/a.ts[link](x) matches [0,19] while the link node masks only [10,19] - a partial overlap where some() and every() disagree (some discards; every keeps and would swallow the link syntax into the placeholder). The Stryker directive claiming equivalence is removed, the comment states the touch rule, and a new test pins the shipped behavior (link preserved, mention inert) so the some->every mutant is killed (NC verified). 2. Guard test inputs: added src/../../.env (kills a first-segment-only check), ../x.ts:12 (pins traversal refusal after the line anchor is stripped), and a backslash case. The backslash case exposed a real gap: react-markdown's URL transform percent-encodes the backslash to %5C before the guard runs, so a backslash traversal reached openFile as ./..%5C..%5C.env. The guard now also splits on the encoded separator (case-insensitive); NC verified a slash-only split reddens the test. Proofs: MarkdownBlock.spec 54/54; NC some->every reddens the overlap test, slash-only split reddens the backslash test, first-segment-only reddens the mid-path test (restored byte-exact); eslint --max-warnings=0 clean on both files; prettier clean.
1 parent 3ab830a commit 2e1bbcb

2 files changed

Lines changed: 67 additions & 4 deletions

File tree

‎webview-ui/src/components/common/MarkdownBlock.tsx‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -124,8 +124,11 @@ function prepareMentions(markdown: string): { preparedMarkdown: string; mentions
124124
// The raw string (not a masked copy) is what the shared regex's boundary
125125
// rules must see: masking would turn a preceding `)` or backtick into a
126126
// space and make a non-mention actionable (e.g. `[file](/src/a.ts)`@problems``).
127-
// Discard a match only when its range lands inside a masked literal region.
128-
// Stryker disable next-line MethodExpression: equivalent: mention matches are word-boundary delimited while masked regions begin and end on non-word syntax characters, so an overlapping range is fully inside or outside and some/every agree
127+
// Discard a match as soon as its range touches a masked literal region: the
128+
// path branch runs to the next whitespace, so a match can extend past a link
129+
// it starts before (`@/src/a.ts[link](x)` masks [10,19] but matches [0,19]);
130+
// keeping a partial overlap would replace the range with a placeholder and
131+
// swallow the link syntax with it.
129132
if (isMasked.slice(start, end).some(Boolean)) {
130133
continue
131134
}
@@ -585,8 +588,10 @@ const MarkdownBlock = memo(({ markdown, mentions = false, breaks = false }: Mark
585588
// Reject path traversal: task markdown is untrusted, so a
586589
// `..` segment (e.g. `[x](../../.env)`) must never reach the
587590
// extension's openFile. The extension re-checks workspace
588-
// containment as a second line of defense.
589-
if (filePath.split(/[\\/]/).includes("..")) {
591+
// containment as a second line of defense. Windows separators
592+
// arrive percent-encoded (react-markdown's URL transform turns
593+
// `\` into `%5C`), so the encoded spelling is scanned too.
594+
if (filePath.split(/%5C|[\\/]/i).includes("..")) {
590595
return
591596
}
592597

‎webview-ui/src/components/common/__tests__/MarkdownBlock.spec.tsx‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -491,6 +491,23 @@ describe("MarkdownBlock", () => {
491491
expect(container.querySelector("p")?.textContent).toBe("file@problems and x@problems")
492492
})
493493

494+
it("keeps a mention whose range only partially overlaps a link inert and preserves the link", async () => {
495+
// The path branch runs to the next whitespace, so here the match spans
496+
// the whole string while the link node masks only [10,19]: the overlap
497+
// is partial and some/every disagree. Discarding on any overlap is the
498+
// shipped rule - keeping the match would replace the range with a
499+
// placeholder and swallow the link syntax with it.
500+
const markdown = "@/src/a.ts[link](x)"
501+
const { container } = render(<MarkdownBlock markdown={markdown} mentions />)
502+
503+
await screen.findByText("link")
504+
505+
expect(container.querySelectorAll("span.mention-context-highlight").length).toBe(0)
506+
const anchor = container.querySelector("a")!
507+
expect(anchor.textContent).toBe("link")
508+
expect(anchor.getAttribute("href")).toBe("x")
509+
})
510+
494511
it("keeps a whitespace-separated mention after a link or inline code actionable", async () => {
495512
// The space is a legitimate boundary for the shared regex, so these
496513
// mentions stay clickable: the masked regions end before the spaces and
@@ -695,6 +712,47 @@ describe("MarkdownBlock", () => {
695712
expect(mockPostMessage).not.toHaveBeenCalledWith(expect.objectContaining({ type: "openFile" }))
696713
})
697714

715+
it("does not post openFile when the traversal starts mid-path", async () => {
716+
// A first-segment-only check would wave this through: the guard must
717+
// scan every separator-delimited segment, not just the leading one.
718+
const markdown = "open [the secret](src/../../.env) now"
719+
const { container } = render(<MarkdownBlock markdown={markdown} />)
720+
721+
await screen.findByText(/the secret/)
722+
723+
const anchor = container.querySelector("a")!
724+
expect(fireEvent.click(anchor)).toBe(false)
725+
expect(mockPostMessage).not.toHaveBeenCalledWith(expect.objectContaining({ type: "openFile" }))
726+
})
727+
728+
it("does not post openFile for backslash-separated traversal", async () => {
729+
// Windows-style separators are untrusted input too: a guard that split
730+
// on "/" alone sees one opaque segment and posts it. The angle brackets
731+
// keep the backslashes literal (outside them, markdown consumes them
732+
// as escape characters before the guard ever runs).
733+
const markdown = "open [the secret](<..\\\\..\\\\.env>) now"
734+
const { container } = render(<MarkdownBlock markdown={markdown} />)
735+
736+
await screen.findByText(/the secret/)
737+
738+
const anchor = container.querySelector("a")!
739+
expect(fireEvent.click(anchor)).toBe(false)
740+
expect(mockPostMessage).not.toHaveBeenCalledWith(expect.objectContaining({ type: "openFile" }))
741+
})
742+
743+
it("does not post openFile for a traversal target carrying a line anchor", async () => {
744+
// The line anchor is stripped before the guard runs, so the traversal
745+
// check must apply to the path that would actually be posted.
746+
const markdown = "open [the secret](../x.ts:12) now"
747+
const { container } = render(<MarkdownBlock markdown={markdown} />)
748+
749+
await screen.findByText(/the secret/)
750+
751+
const anchor = container.querySelector("a")!
752+
expect(fireEvent.click(anchor)).toBe(false)
753+
expect(mockPostMessage).not.toHaveBeenCalledWith(expect.objectContaining({ type: "openFile" }))
754+
})
755+
698756
it("leaves non-file links to the browser without posting openFile", async () => {
699757
// External schemes are not local files: the anchor keeps its default
700758
// navigation behavior (no preventDefault) and posts nothing.

0 commit comments

Comments
 (0)