fix(links): stop mangling percent-encoding when opening relative + external links

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

package.json

@@ -27,7 +27,7 @@
     "gfm",
     "MEO"
   ],
-  "version": "0.1.58",
+  "version": "0.1.59",
   "publisher": "mtskf",
   "type": "module",
   "engines": {

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);
+}

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<boolean>;
   /** 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.
   //

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

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,
         }),
     });

src/extension/test-harness.ts

@@ -157,13 +157,14 @@ interface TestOverrides {
   webviewPostMessage: ((message: HostToWebview) => Thenable<boolean>) | 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<boolean>) | 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<boolean>) | 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<boolean>) | null {
+   *  `TestOverrides.openExternal`. Receives the built `vscode.Uri`. */
+  get openExternalOverride(): ((uri: Uri) => Thenable<boolean>) | null {
     return this._overrides.openExternal;
   }
 
-  set openExternalOverride(override: ((url: string) => Thenable<boolean>) | null) {
+  set openExternalOverride(override: ((uri: Uri) => Thenable<boolean>) | null) {
     this._overrides.openExternal = override;
   }
 

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;