diff --git a/CHANGELOG.md b/CHANGELOG.md index f03eb54a..a92ea984 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ All notable changes to Quoll are documented here. +## 0.1.59 — 2026-07-29 + +### Fixed + +- Links with percent-encoded characters now open correctly: a relative link like `[notes](my%20notes.md)` opens the space-named file, and external links containing `%2F` or `+` (GitLab-style API URLs, search queries) reach your browser unchanged instead of being silently altered. + ## 0.1.58 — 2026-07-28 ### Fixed diff --git a/package.json b/package.json index bac05482..de78c2eb 100644 --- a/package.json +++ b/package.json @@ -27,7 +27,7 @@ "gfm", "MEO" ], - "version": "0.1.58", + "version": "0.1.59", "publisher": "mtskf", "type": "module", "engines": { diff --git a/src/extension/links/build-external-uri.ts b/src/extension/links/build-external-uri.ts new file mode 100644 index 00000000..a94aaba8 --- /dev/null +++ b/src/extension/links/build-external-uri.ts @@ -0,0 +1,85 @@ +// Encoding-preserving external-URL → vscode.Uri builder for env.openExternal. +// +// WHY NOT Uri.parse(url): Uri.parse DECODES percent-escapes into the stored +// components (`%2F` in the path becomes `/`, unrecoverable) and re-encodes on +// serialisation, silently changing GitLab-style `%2F` API paths and `+`-bearing +// search queries. The WHATWG URL parser PRESERVES percent-encoding in +// pathname/search/hash — in particular the reported `%2F` (path) and `+` +// (query) — so splitting with it and rebuilding via Uri.from(...) keeps those +// bytes intact through to the browser: VS Code's OpenerService +// delivers the href as `encodeURI(uri.toString(true))` (skipEncoding, then an +// encodeURI that leaves %, %2F, +, %20, &, =, # untouched — read from the +// installed VS Code 1.130.0 source, `workbench.desktop.main.js`). +// +// SCOPE OF THE GUARANTEE (deliberately NOT a byte-for-byte echo of arbitrary +// input): what is preserved is the percent-encoding of path/query/fragment — +// notably `%2F` and `+`, the reported bug. `new URL()` (and Uri.from's own +// serialisation) apply destination-PRESERVING canonicalisation a browser would +// apply anyway: lower-cased scheme/host, punycoded IDN host, dropped default +// port, resolved dot-segments, `?`/`#` present only when non-empty, trailing `/` +// on a bare authority. We accept that over hand-rolling a lexical URL splitter: +// a bespoke splitter feeding an external-open is a security risk (a mis-split +// authority opens a different HOST), and Uri.from/toString(true) normalises +// regardless, so a splitter would not buy true byte-identity either. `new URL()` +// DROPS userinfo from `.host`, so the authority is rebuilt below. +// +// VERSION NOTE: the encodeURI(toString(true)) opener behaviour is a long-stable +// VS Code trait (query strings would break universally otherwise) but is an +// implementation detail, not a documented API contract. It was read from VS Code +// 1.130.0 source, while the extension's engines.vscode floor is older (~1.94), so +// the exact opener internals on the floor were not source-verified — the E2E +// asserts the Uri-level contract (which is version-independent), and the opener's +// encodeURI(toString(true)) step is the residual assumption. If external links +// ever start arriving mangled after a VS Code upgrade, re-verify this path first. + +import { Uri } from "vscode"; + +export type ExternalUrlParts = { + scheme: string; + authority: string; + path: string; + query: string; + fragment: string; +}; + +/** Split an http/https/mailto URL into still-percent-encoded Uri components. + * Returns null when the WHATWG parser rejects the input. Pure (no vscode). */ +export function splitExternalUrl(href: string): ExternalUrlParts | null { + let url: URL; + try { + url = new URL(href); + } catch { + return null; + } + // `url.host` is host + optional :port but DROPS userinfo — rebuild the full + // authority so `user:pw@host` links are not silently stripped. Guard on + // username OR password: `https://:pw@host` is valid (username === "", + // password === "pw") and a username-only guard would drop the `:pw@`. + const userinfo = + url.username || url.password ? `${url.username}${url.password ? `:${url.password}` : ""}@` : ""; + return { + scheme: url.protocol.replace(/:$/, ""), + authority: userinfo + url.host, + path: url.pathname, // WHATWG preserves %-encoding here + query: url.search.replace(/^\?/, ""), + fragment: url.hash.replace(/^#/, ""), + }; +} + +/** Build a vscode.Uri for env.openExternal that preserves path/query/fragment + * percent-encoding (notably %2F and +). + * + * FALLBACK: `isAllowedUrl` gates only the scheme, not full URL syntax, so a + * degenerate-but-allowlisted href like `https://` (no host) passes the gate yet + * makes `new URL()` throw → splitExternalUrl returns null. For those unparseable + * inputs we fall back to `Uri.parse(href)` — the pre-fix path. The + * encoding-preserving guarantee does NOT extend to the fallback: it is lossy + * exactly like the old code, and a WHATWG-rejected href could still contain + * `%2F`/`+` (they would be lost here). We accept that over rejecting the click: + * the input is already malformed enough that the WHATWG parser refused it, and + * falling back keeps the click working (env.openExternal + handle-open-external's + * toast still cover any open failure) rather than silently dropping it. */ +export function buildExternalUri(href: string): Uri { + const parts = splitExternalUrl(href); + return parts ? Uri.from(parts) : Uri.parse(href); +} diff --git a/src/extension/links/handle-open-external.ts b/src/extension/links/handle-open-external.ts index e6accc2e..a88c760a 100644 --- a/src/extension/links/handle-open-external.ts +++ b/src/extension/links/handle-open-external.ts @@ -39,8 +39,8 @@ const OPENABLE_SCHEMES = new Set(["http", "https", "mailto"]); /** User-facing toast shown when a gated, launchable URL still fails to open. * Covers all three post-gate failure modes symmetrically — a fulfilled * `false` (the OS has no handler for the scheme), an async rejection - * (system browser missing, OS denial), and a synchronous throw (Uri.parse - * on malformed input). The specific cause goes to the host log; the toast + * (system browser missing, OS denial), and a synchronous throw (buildExternalUri's + * `Uri.parse` fallback on malformed input). The specific cause goes to the host log; the toast * just tells the user the click did something. */ const OPEN_EXTERNAL_FAILURE_MESSAGE = "Quoll: couldn't open the link. See the extension host log for details."; @@ -65,9 +65,10 @@ function sanitizeForLog(href: string): string { export type HandleOpenExternalDeps = { /** vscode.env.openExternal binding (or a test mock). Returns a Thenable * per VS Code's API contract. The QuollEditorPanel wire-up wraps - * `(url) => env.openExternal(Uri.parse(url))`; Uri.parse can throw - * synchronously on malformed input that isAllowedUrl missed (review - * fix #6) — handleOpenExternal absorbs that throw. */ + * `(url) => env.openExternal(buildExternalUri(url))`; `buildExternalUri` + * (or its `Uri.parse` fallback) can throw synchronously on malformed input + * that isAllowedUrl missed (review fix #6) — handleOpenExternal absorbs + * that throw. */ openExternal: (url: string) => Thenable; /** Host showError (a window.showErrorMessage wrapper). Surfaces a * user-visible toast when a launch fails so a failed link click is not @@ -104,11 +105,11 @@ export function handleOpenExternal(href: string, deps: HandleOpenExternalDeps): }); return; } - // Synchronous-throw guard (review fix #6): Uri.parse — called by the - // production deps closure `(url) => env.openExternal(Uri.parse(url))` — - // throws synchronously on inputs even non-strict mode rejects (rare - // post-isAllowedUrl, but defense in depth). Without try/catch, that - // throw escapes handleOpenExternal, breaks + // Synchronous-throw guard (review fix #6): the production deps closure + // `(url) => env.openExternal(buildExternalUri(url))` can throw synchronously + // — buildExternalUri's `Uri.parse` fallback arm rejects inputs even + // non-strict mode refuses (rare post-isAllowedUrl, but defense in depth). + // Without try/catch, that throw escapes handleOpenExternal, breaks // QuollEditorPanel.handleInbound's switch, and corrupts subsequent // inbound-message handling. // diff --git a/src/extension/links/handle-open-link.ts b/src/extension/links/handle-open-link.ts index e627b8d9..c0aaadb2 100644 --- a/src/extension/links/handle-open-link.ts +++ b/src/extension/links/handle-open-link.ts @@ -9,12 +9,19 @@ // rationale. In order it: // - re-applies isAllowedUrl (rejects C0/DEL + protocol-relative //host — the // SAME host-side re-validation handle-open-external.ts applies), -// - strips a trailing #fragment, -// - rejects a scheme-bearing / absolute / non-.md destination, +// - strips a trailing #fragment on the ENCODED form (a literal `#` in a name +// is `%23`, so URL-structural splitting must precede decode), +// - percent-decodes the destination ONCE (so `my%20notes.md` opens the real +// space-named file; malformed escapes fall back to the raw form), then +// re-applies isAllowedUrl to the DECODED form, +// - rejects a scheme-bearing / absolute / non-.md destination (on the decoded +// path), // - resolves the remainder against THIS document's directory (host owns // document.uri), // - requires the resolved target to be inside a workspace folder OR (no -// workspace / escaped) inside the document's own directory subtree, +// workspace / escaped) inside the document's own directory subtree — +// containment on the resolved, decoded target is authoritative regardless +// of encoding, so decoding does not reopen the traversal hole, // - opens it via the injected openWith (production: openInQuollEditor + // QuollEditorPanel.viewType). // @@ -66,14 +73,42 @@ export function handleOpenLink(href: string, deps: HandleOpenLinkDeps): void { } const hashIdx = href.indexOf("#"); - const pathPart = hashIdx >= 0 ? href.slice(0, hashIdx) : href; + const encodedPath = hashIdx >= 0 ? href.slice(0, hashIdx) : href; - if (pathPart.length === 0) { + if (encodedPath.length === 0) { console.warn("[quoll] open-link dropped: empty path (fragment-only)", { hrefPreview: sanitizeForLog(href), }); return; } + + // Percent-decode ONCE so the standard `[notes](my%20notes.md)` form (what VS + // Code preview / GitHub / Obsidian emit) resolves to the real space-named + // file rather than a literal `%20`-named one. decodeURIComponent throws on a + // malformed escape (e.g. `50%off.md`) — fall back to the raw form so such a + // link still opens its literal-named file (no regression). decodeURIComponent + // is all-or-nothing, so a path mixing a valid and an invalid escape + // (`sub%20dir/50%off.md`) decodes to neither (whole-string fallback to raw), + // which merely surfaces the existing not-found toast — no security impact. + // Every structural guard below AND the containment check run on the DECODED + // path, so a decoded `../` traversal (`..%2f..%2f…`) resolves outside scope + // and is rejected by containment — decoding does not reopen the traversal hole. + let pathPart: string; + try { + pathPart = decodeURIComponent(encodedPath); + } catch { + pathPart = encodedPath; + } + + // Re-apply the allowlist to the decoded form: catches C0/DEL bytes and + // protocol-relative `//host` that were hidden behind percent-escapes + // (`%01`, `%2f%2f…`) in the raw href. + if (!isAllowedUrl(pathPart)) { + console.warn("[quoll] open-link rejected: decoded path not in allowlist", { + hrefPreview: sanitizeForLog(href), + }); + return; + } if (/^[a-z][a-z0-9+.-]*:/i.test(pathPart)) { // isAllowedUrl accepts mailto:/http: — but open-link targets are schemeless. console.warn("[quoll] open-link dropped: destination has a scheme", { @@ -103,15 +138,11 @@ export function handleOpenLink(href: string, deps: HandleOpenLinkDeps): void { // Fail-closed containment: inside a workspace folder, OR (no workspace / // escaped the workspace) inside the document's own directory subtree. // - // Encoded segments do NOT traverse: decodeMarkdownDestination does not - // percent-decode and Uri.joinPath does not treat a literal `%2f`/`%5c` as a - // separator, so `..%2f..%2fx.md` resolves to a single literal (non-existent) - // filename INSIDE `dir` — never an escape. Containment is asserted on the - // resolved `target`, authoritative regardless of encoding. (The image-write - // gate in url-allowlist.ts decodes per segment because it validates absolute - // resolved URLs; this handler only joins relative segments onto `dir`, so the - // literal-separator property suffices.) A literal `%` is deliberately NOT - // rejected so a legitimate `%20`-in-filename link still opens. + // Containment runs on the DECODED, resolved `target`, so it is authoritative + // regardless of encoding. A decoded `../` traversal (`..%2f..%2fx.md` → + // `../../x.md`) resolves OUTSIDE `dir`/workspace and is rejected here — + // decoding before the join does not reopen the traversal hole, because the + // boundary is asserted on the resolved target, not on the raw string. // // A rejection here is log-only by design: the webview cannot evaluate // containment (it owns no path), so a containment-refused click is a normal diff --git a/src/extension/session/quoll-editor-panel.ts b/src/extension/session/quoll-editor-panel.ts index fbc05d37..aa00633b 100644 --- a/src/extension/session/quoll-editor-panel.ts +++ b/src/extension/session/quoll-editor-panel.ts @@ -73,6 +73,7 @@ import { createContextHandoffWiring } from "../handoff/context-handoff-wiring.js import { takeSwitchCaret } from "../handoff/editor-switch-caret.js"; import { createRevealCaretSuppression } from "../handoff/reveal-caret-suppression.js"; import { createImageWriteWiring } from "../image/image-write-wiring.js"; +import { buildExternalUri } from "../links/build-external-uri.js"; import { handleOpenCodeReference } from "../links/handle-open-code-reference.js"; import { handleOpenExternal } from "../links/handle-open-external.js"; import { handleOpenLink } from "../links/handle-open-link.js"; @@ -468,15 +469,23 @@ export class QuollEditorPanel implements CustomTextEditorProvider { ? this.harness.applyEditOverride(edit as WorkspaceEdit) : workspace.applyEdit(edit as WorkspaceEdit), }, - // `openExternalOverride` (when set) bypasses Uri.parse so the open-external - // E2E test can pin the delegation contract without depending on the real - // `env` binding — the test process cannot spy on `env.openExternal` through - // the vscode module namespace. The override sees the gated href as a plain - // string; same surface as the production closure. + // The production closure builds the encoding-preserving `Uri` via + // `buildExternalUri` (WHATWG split + `Uri.from`, preserving `%2F`/`+`) + // and routes it to the override (tests) or the real `env.openExternal`. + // `openExternalOverride` (when set) receives that built `Uri`, so the + // open-external E2E pins the real handoff — the test process cannot spy on + // `env.openExternal` through the vscode module namespace. openExternal: (href) => handleOpenExternal(href, { - openExternal: - this.harness?.openExternalOverride ?? ((url) => env.openExternal(Uri.parse(url))), + // Build the encoding-preserving Uri ONCE, then route it to the override + // (tests) or the real env.openExternal. Building BEFORE the override + // boundary means the E2E observes the exact Uri production opens — the + // old code built via Uri.parse INSIDE the fallback arm, so the override + // bypassed it and %2F/+ mangling went untested. + openExternal: (url) => + (this.harness?.openExternalOverride ?? ((uri) => env.openExternal(uri)))( + buildExternalUri(url) + ), showError, }), }); diff --git a/src/extension/test-harness.ts b/src/extension/test-harness.ts index 80d27e1c..43c8a7ba 100644 --- a/src/extension/test-harness.ts +++ b/src/extension/test-harness.ts @@ -157,13 +157,14 @@ interface TestOverrides { webviewPostMessage: ((message: HostToWebview) => Thenable) | null; /** When non-null, the panel's `case "open-external"` arm routes * `handleOpenExternal`'s injected `openExternal` dep through this - * instead of `(url) => env.openExternal(Uri.parse(url))`. The override - * sees the already-allowlist-gated, post-decode href as a plain string - * and is used by the open-external E2E test to pin the case arm's - * delegation contract without depending on the real `env` binding - * (which the test process cannot spy on through the vscode module - * namespace). */ - openExternal: ((url: string) => Thenable) | null; + * instead of `(uri) => env.openExternal(uri)`. The production closure + * builds the encoding-preserving `Uri` via `buildExternalUri(href)` + * BEFORE this override boundary, so the override sees the fully-built + * `vscode.Uri` env.openExternal would open — letting the open-external + * E2E assert the exact Uri (and its byte-exact `%2F`/`+` preservation) + * without depending on the real `env` binding (which the test process + * cannot spy on through the vscode module namespace). */ + openExternal: ((uri: Uri) => Thenable) | null; /** When non-null, the panel's `case "open-link"` arm routes * `handleOpenLink`'s injected `openWith` dep through this instead of * `(uri) => openInQuollEditor(uri, QuollEditorPanel.viewType)`. The @@ -285,12 +286,12 @@ export class TestHarness { /** Read by the panel's `case "open-external"` arm to route the * `openExternal` dep through a test override — see - * `TestOverrides.openExternal`. */ - get openExternalOverride(): ((url: string) => Thenable) | null { + * `TestOverrides.openExternal`. Receives the built `vscode.Uri`. */ + get openExternalOverride(): ((uri: Uri) => Thenable) | null { return this._overrides.openExternal; } - set openExternalOverride(override: ((url: string) => Thenable) | null) { + set openExternalOverride(override: ((uri: Uri) => Thenable) | null) { this._overrides.openExternal = override; } diff --git a/src/shared/protocol.ts b/src/shared/protocol.ts index c3de9b48..92ed2f62 100644 --- a/src/shared/protocol.ts +++ b/src/shared/protocol.ts @@ -409,7 +409,9 @@ export type EditMessage = Envelope & { * * `href` is the already-decoded URL string (post * decodeMarkdownDestination) — NOT raw Markdown source bytes. The host - * feeds it straight to isAllowedUrl + Uri.parse with no further decode. */ + * feeds it to isAllowedUrl, then rebuilds the URL via `buildExternalUri` + * (WHATWG split + `Uri.from`) so path/query percent-encoding (`%2F`/`+`) + * reaches the browser intact — no `Uri.parse` round-trip, no further decode. */ export type OpenExternalMessage = Envelope & { type: "open-external"; href: string; diff --git a/test/extension/e2e/open-external.test.ts b/test/extension/e2e/open-external.test.ts index 21e1a2e1..c878e9f5 100644 --- a/test/extension/e2e/open-external.test.ts +++ b/test/extension/e2e/open-external.test.ts @@ -1,4 +1,5 @@ import * as assert from "node:assert"; +import type { Uri } from "vscode"; import { PROTOCOL_VERSION } from "./constants"; import { cleanupBetweenTests, getHarness, isDocumentEvent, openFixtureWithQuoll } from "./harness"; @@ -9,21 +10,17 @@ import { cleanupBetweenTests, getHarness, isDocumentEvent, openFixtureWithQuoll * `handleOpenExternal` in isolation, but the case-arm's wiring — label * spelling, fallthrough behaviour, delegate target — has zero coverage. * A typo on the case label, an accidental fallthrough, or a swapped - * delegate target would be invisible to CI. (The `Uri.parse` wrap - * inside the production closure is intentionally NOT pinned here — - * `openExternalOverride` replaces the entire closure, so the override - * sees the post-allowlist href as a plain string; `quoll-editor-panel.ts` - * documents this bypass on the case arm itself, and TypeScript's - * `env.openExternal: (uri: Uri)` signature would catch a removed wrap - * at compile time.) + * delegate target would be invisible to CI. * - * The harness exposes `openExternalOverride` so the test can pin the - * delegation contract without depending on `env.openExternal` (which the - * test process cannot spy on through the vscode module namespace). The - * override sees the post-allowlist href as a plain string; production - * routes through `(url) => env.openExternal(Uri.parse(url))`, but the - * pre-Uri.parse string is exactly what `handleOpenExternal`'s injected - * dep contract requires. + * The production closure builds the encoding-preserving `Uri` via + * `buildExternalUri` (WHATWG split + `Uri.from`) BEFORE the override + * boundary, so `openExternalOverride` now receives the fully-built + * `vscode.Uri` env.openExternal would open — NOT a pre-Uri string. These + * tests therefore pin the full production handoff + * `closure → buildExternalUri → Uri → env.openExternal`, and the byte-exact + * test below proves `%2F`/`+` survive that path. The harness seam is used + * because the test process cannot spy on `env.openExternal` through the + * vscode module namespace. */ describe("open-external", function () { this.timeout(15000); @@ -43,8 +40,8 @@ describe("open-external", function () { await harness.waitForEvent(isDocumentEvent, 8000); const calls: string[] = []; - harness.openExternalOverride = async (url: string): Promise => { - calls.push(url); + harness.openExternalOverride = async (uri: Uri): Promise => { + calls.push(uri.toString(true)); return true; }; @@ -64,8 +61,8 @@ describe("open-external", function () { assert.deepStrictEqual( calls, - ["https://example.com"], - "expected env.openExternal to be invoked once with the safe URL" + ["https://example.com/"], + "expected env.openExternal to be invoked once with the safe URL (new URL() adds the trailing / on a bare authority)" ); }); @@ -75,8 +72,8 @@ describe("open-external", function () { await harness.waitForEvent(isDocumentEvent, 8000); const calls: string[] = []; - harness.openExternalOverride = async (url: string): Promise => { - calls.push(url); + harness.openExternalOverride = async (uri: Uri): Promise => { + calls.push(uri.toString(true)); return true; }; @@ -109,8 +106,8 @@ describe("open-external", function () { await harness.waitForEvent(isDocumentEvent, 8000); const calls: string[] = []; - harness.openExternalOverride = async (url: string): Promise => { - calls.push(url); + harness.openExternalOverride = async (uri: Uri): Promise => { + calls.push(uri.toString(true)); return true; }; @@ -131,4 +128,59 @@ describe("open-external", function () { "expected OPENABLE_SCHEMES fallthrough to drop the fragment-only URL — openExternal must not be reached" ); }); + + it("hands env.openExternal a Uri that preserves %2F and + byte-exactly", async () => { + const harness = await getHarness(); + await openFixtureWithQuoll("sample.md"); + await harness.waitForEvent(isDocumentEvent, 8000); + + const calls: string[] = []; + harness.openExternalOverride = async (uri: Uri): Promise => { + calls.push(uri.toString(true)); + return true; + }; + + const raw = "https://gitlab.com/api/v4/projects/foo%2Fbar/pipelines?q=a+b"; + const panel = harness.activePanel; + assert.ok(panel); + panel.simulateInbound({ protocol: PROTOCOL_VERSION, type: "open-external", href: raw }); + + await Promise.resolve(); + await Promise.resolve(); + + // toString(true) is the skipEncoding form VS Code's opener feeds to + // encodeURI(...) (which leaves %2F / + untouched) — so this is what reaches + // the browser. The old Uri.parse path would show `.../foo/bar/...?q=a+b`. + assert.deepStrictEqual(calls, [raw], "expected the built Uri to preserve %2F and +"); + }); + + it("still reaches openExternal via the Uri.parse fallback when WHATWG rejects the href", async () => { + // "https://" passes isAllowedUrl (scheme-only check) but has no host, so + // `new URL()` throws inside splitExternalUrl — buildExternalUri falls back + // to `Uri.parse(href)` (see build-external-uri.ts's FALLBACK doc). This + // pins that the fallback does not throw synchronously and still reaches + // env.openExternal through handleOpenExternal's try/catch (review fix #6). + const harness = await getHarness(); + await openFixtureWithQuoll("sample.md"); + await harness.waitForEvent(isDocumentEvent, 8000); + + const calls: string[] = []; + harness.openExternalOverride = async (uri: Uri): Promise => { + calls.push(uri.toString(true)); + return true; + }; + + const panel = harness.activePanel; + assert.ok(panel); + panel.simulateInbound({ protocol: PROTOCOL_VERSION, type: "open-external", href: "https://" }); + + await Promise.resolve(); + await Promise.resolve(); + + assert.strictEqual( + calls.length, + 1, + "expected the Uri.parse fallback to reach env.openExternal exactly once, without throwing" + ); + }); }); diff --git a/test/extension/e2e/open-link.test.ts b/test/extension/e2e/open-link.test.ts index 65da6248..b4ba5f66 100644 --- a/test/extension/e2e/open-link.test.ts +++ b/test/extension/e2e/open-link.test.ts @@ -71,4 +71,54 @@ describe("open-link", function () { "expected the host containment gate to drop an out-of-scope open-link" ); }); + + it("decodes %20 in a relative link and resolves the space-named target", async () => { + const harness = await getHarness(); + await openFixtureWithQuoll("link-source.md"); + await harness.waitForEvent(isDocumentEvent, 8000); + + const opened: string[] = []; + harness.openLinkOverride = (uri): Promise => { + opened.push(uri.path); + return Promise.resolve(undefined); + }; + + const panel = harness.activePanel; + assert.ok(panel); + panel.simulateInbound({ + protocol: PROTOCOL_VERSION, + type: "open-link", + href: "./my%20notes.md", + }); + await Promise.resolve(); + + assert.strictEqual(opened.length, 1); + assert.ok( + opened[0].endsWith("/my notes.md"), + `expected the decoded space-named target, got ${opened[0]}` + ); + }); + + it("rejects a percent-encoded traversal after decoding (real Uri.joinPath)", async () => { + const harness = await getHarness(); + await openFixtureWithQuoll("link-source.md"); + await harness.waitForEvent(isDocumentEvent, 8000); + + const opened: string[] = []; + harness.openLinkOverride = (uri): Promise => { + opened.push(uri.path); + return Promise.resolve(undefined); + }; + + const panel = harness.activePanel; + assert.ok(panel); + panel.simulateInbound({ + protocol: PROTOCOL_VERSION, + type: "open-link", + href: "..%2f..%2f..%2f..%2f..%2f..%2fetc%2fpasswd.md", + }); + await Promise.resolve(); + + assert.deepStrictEqual(opened, [], "decoded ../ traversal must be dropped by containment"); + }); }); diff --git a/test/extension/e2e/types.ts b/test/extension/e2e/types.ts index 7a6fb002..ec063e11 100644 --- a/test/extension/e2e/types.ts +++ b/test/extension/e2e/types.ts @@ -246,7 +246,7 @@ export interface TestHarnessShape { webviewPostMessageOverride: | ((message: { type: string } & Record) => Thenable) | null; - openExternalOverride: ((url: string) => Thenable) | null; + openExternalOverride: ((uri: Uri) => Thenable) | null; openLinkOverride: ((uri: Uri) => Thenable) | null; openCodeReferenceOverride: | ((uri: Uri, line: number | undefined, col: number | undefined) => Thenable) diff --git a/test/extension/links/build-external-uri.test.ts b/test/extension/links/build-external-uri.test.ts new file mode 100644 index 00000000..52c9ab97 --- /dev/null +++ b/test/extension/links/build-external-uri.test.ts @@ -0,0 +1,44 @@ +import { describe, expect, it } from "vitest"; +import { splitExternalUrl } from "../../../src/extension/links/build-external-uri.js"; + +describe("splitExternalUrl", () => { + it("preserves %2F in the path (does not collapse to /)", () => { + const parts = splitExternalUrl("https://gitlab.com/api/v4/projects/foo%2Fbar/pipelines"); + expect(parts).not.toBeNull(); + expect(parts?.path).toBe("/api/v4/projects/foo%2Fbar/pipelines"); + expect(parts?.scheme).toBe("https"); + expect(parts?.authority).toBe("gitlab.com"); + }); + + it("preserves + and %2B verbatim in the query", () => { + const parts = splitExternalUrl("https://example.com/search?q=a+b&x=1%2B2"); + expect(parts?.query).toBe("q=a+b&x=1%2B2"); + }); + + it("preserves %20 in the fragment", () => { + const parts = splitExternalUrl("https://example.com/p#frag%20ment"); + expect(parts?.fragment).toBe("frag%20ment"); + }); + + it("splits a mailto: URL preserving encoded query", () => { + const parts = splitExternalUrl("mailto:foo@example.com?subject=a%2Fb"); + expect(parts?.scheme).toBe("mailto"); + expect(parts?.path).toBe("foo@example.com"); + expect(parts?.query).toBe("subject=a%2Fb"); + }); + + it("preserves userinfo in the authority (new URL drops it from .host)", () => { + const parts = splitExternalUrl("https://user:pw@example.com/x%2Fy"); + expect(parts?.authority).toBe("user:pw@example.com"); + expect(parts?.path).toBe("/x%2Fy"); + }); + + it("preserves password-only userinfo (username-only guard would drop :pw@)", () => { + const parts = splitExternalUrl("https://:pw@example.com/x"); + expect(parts?.authority).toBe(":pw@example.com"); + }); + + it("returns null on a URL the WHATWG parser rejects", () => { + expect(splitExternalUrl("https://")).toBeNull(); + }); +}); diff --git a/test/extension/links/handle-open-link.test.ts b/test/extension/links/handle-open-link.test.ts index 5cc755ba..8428e65b 100644 --- a/test/extension/links/handle-open-link.test.ts +++ b/test/extension/links/handle-open-link.test.ts @@ -106,6 +106,15 @@ describe("handleOpenLink", () => { expect(opened).toEqual([]); }); + it("rejects a percent-encoded control byte that decodes to C0 (%01)", () => { + // Raw `./oth%01er.md` passes the FIRST isAllowedUrl (literal %,0,1); only the + // post-decode re-validation catches the decoded U+0001. Pins that guard as + // non-vacuous — removing it must turn this test red. + const { deps, opened } = makeDeps(); + handleOpenLink("./oth%01er.md", deps); + expect(opened).toEqual([]); + }); + it("rejects a sibling dir that shares the doc-dir name as a prefix (no workspace)", () => { // /ws/notes vs /ws/notes-evil must NOT match on a bare prefix — guards the // trailing-slash normalisation in isWithinDir (without it, startsWith would @@ -115,13 +124,41 @@ describe("handleOpenLink", () => { expect(opened).toEqual([]); }); - it("does not traverse via percent-encoded separators (stays contained)", () => { - // `%2f` is literal (not a separator) through Uri.joinPath, so this resolves - // to a single literal filename INSIDE the doc dir — contained, never an - // escape. Pins the threat-model claim that encoded segments don't traverse. + it("decodes a percent-encoded space so my%20notes.md opens my notes.md", () => { + const { deps, opened } = makeDeps(); + handleOpenLink("./my%20notes.md", deps); + expect(opened).toEqual(["/ws/notes/my notes.md"]); + }); + + it("decodes a percent-encoded space inside a subdirectory segment", () => { + const { deps, opened } = makeDeps(); + handleOpenLink("./sub%20dir/my%20notes.md", deps); + expect(opened).toEqual(["/ws/notes/sub dir/my notes.md"]); + }); + + it("leaves a malformed percent-escape literal (no decode, still opens)", () => { + // decodeURIComponent throws on `%of`; we fall back to the raw form so a real + // file literally named `50%off.md` still opens (no regression vs pre-fix). + const { deps, opened } = makeDeps(); + handleOpenLink("./50%off.md", deps); + expect(opened).toEqual(["/ws/notes/50%off.md"]); + }); + + it("rejects a decoded percent-encoded traversal (containment holds after decode)", () => { + // `..%2f..%2f` now decodes to `../../` and resolves to /secret.md — OUTSIDE + // the workspace — so containment (asserted on the resolved target AFTER + // decoding) rejects it. Pins that decoding does not reopen the traversal hole. const { deps, opened } = makeDeps(); handleOpenLink("..%2f..%2fsecret.md", deps); - expect(opened).toEqual(["/ws/notes/..%2f..%2fsecret.md"]); + expect(opened).toEqual([]); + }); + + it("falls back to the raw form when a segment mixes a valid and invalid escape", () => { + // decodeURIComponent is all-or-nothing: %20 is NOT partially decoded when a + // later %off is malformed. Whole-string raw fallback → literal-named target. + const { deps, opened } = makeDeps(); + handleOpenLink("./sub%20dir/50%off.md", deps); + expect(opened).toEqual(["/ws/notes/sub%20dir/50%off.md"]); }); it("shows the failure toast when openWith rejects asynchronously", async () => { diff --git a/test/markdown/url-choke-point.test.ts b/test/markdown/url-choke-point.test.ts index e0a40ded..de3037f2 100644 --- a/test/markdown/url-choke-point.test.ts +++ b/test/markdown/url-choke-point.test.ts @@ -133,9 +133,14 @@ const HTML_TABLE_PASTE_PARSE = [ "src/webview/cm/paste/html-to-markdown.ts", ] as const; +// - build-external-uri.ts: splits an ALREADY-`isAllowedUrl`-gated external URL +// with `new URL` to preserve path/query percent-encoding (`%2F`/`+`) before +// rebuilding a `vscode.Uri` for `env.openExternal`. The parse re-derives +// components from a gated href; it mints no new destination. const URL_PARSE_ENDPOINTS = [ "src/markdown/url-allowlist.ts", "src/webview/cm/image/resource-base.ts", + "src/extension/links/build-external-uri.ts", ] as const; const CHOKE_POINTS: readonly ChokePoint[] = [