From 524ba9188ee49405ea39bc752010d58219c616c7 Mon Sep 17 00:00:00 2001 From: Yash Datta Date: Sat, 5 Sep 2026 23:31:19 +0800 Subject: [PATCH] fix(web-ui): serve /ui on Windows by comparing against the platform separator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The path-traversal guard in the web-ui frontend tested containment with a hardcoded `/`: if (target !== DIST && !target.startsWith(`${DIST}/`)) `normalize()` emits the PLATFORM separator, so on Windows every legitimate asset resolves to `C:\...\web\dist\index.html`, which does not start with `C:\...\web\dist/`. The guard fired on valid files and the entire UI returned 403 Forbidden. Fixes #299. Worth recording what the guard was actually doing: that 403 branch is unreachable over HTTP. The WHATWG URL parser resolves `..` segments (and folds `\` to `/` for http) before `handleFetch` sees the path, so `/ui/../../etc/passwd` arrives as pathname `/etc/passwd` and falls out of the `/ui/` prefix check entirely. The guard's only observable effect in production was this Windows false positive. It is still correct defense-in-depth, so it is fixed rather than dropped. Rather than only swapping in `sep` — which no test on a POSIX CI machine can verify — the mapping moves into a pure `resolveUiAsset(distRoot, urlPath, pathFlavor)`, following the precedent already set by `buildBootScript` in this module. The path flavor is injectable, so `path.win32` reproduces the Windows behaviour from Linux and the regression is covered where it actually runs. Tests assert both flavors: legitimate nested assets, the mount root with and without a trailing slash, SPA deep links, `..` traversal, and a sibling directory that merely shares the root's string prefix (`web/dist-secrets`). Confirmed to catch the regression — reverting the guard to `${distRoot}/` fails four win32 cases; restoring `${distRoot}${p.sep}` passes. Verified end to end against a live local-mode daemon: `/ui` and `/ui/` 200, both hashed bundle assets 200, SPA deep link 200, missing asset 404, four traversal payloads (including `%2f`-encoded) never escaping dist, and the injected local token driving a real WebSocket through auth.ok to a session list. Reported by @yl-dev-tmtc, who also identified the cause and the `sep` fix. Co-Authored-By: Claude Opus 5 (1M context) --- src/frontends/web-ui/index.ts | 42 +++++++++-- src/tests/web-ui-asset-path.test.ts | 106 ++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 7 deletions(-) create mode 100644 src/tests/web-ui-asset-path.test.ts diff --git a/src/frontends/web-ui/index.ts b/src/frontends/web-ui/index.ts index 3ee0644..e6d8187 100644 --- a/src/frontends/web-ui/index.ts +++ b/src/frontends/web-ui/index.ts @@ -26,7 +26,7 @@ */ import { existsSync } from "node:fs"; -import { join, normalize, resolve } from "node:path"; +import { join, normalize, resolve, sep } from "node:path"; import { isLoopbackHost } from "../../daemon/local-auth.js"; import type { Frontend, FrontendContext } from "../types.js"; @@ -65,6 +65,37 @@ export function buildBootScript(opts: { return ``; } +/** + * The slice of `node:path` the asset resolver needs. Injectable so the + * resolver can be exercised under Windows semantics (`path.win32`) from a + * POSIX host — the separator bug this guards against is invisible otherwise. + */ +export type PathFlavor = { join: typeof join; normalize: typeof normalize; sep: string }; + +const NATIVE_PATH: PathFlavor = { join, normalize, sep }; + +/** + * Map a `/ui/...` request path to a file under `distRoot`, or `null` when it + * escapes that root. + * + * The containment check compares against `distRoot + sep`, NOT a hardcoded + * `/`. `normalize()` emits the platform separator, so on Windows every legit + * asset resolves to `C:\...\web\dist\index.html` — which does not start with + * `C:\...\web\dist/`, and the whole UI 403s (issue #299). The separator has to + * come from the same path flavor that produced the string being tested. + */ +export function resolveUiAsset( + distRoot: string, + urlPath: string, + p: PathFlavor = NATIVE_PATH, +): { rel: string; target: string } | null { + const rel = + urlPath === "/ui" || urlPath === "/ui/" ? "index.html" : urlPath.slice("/ui/".length); + const target = p.normalize(p.join(distRoot, rel)); + if (target !== distRoot && !target.startsWith(`${distRoot}${p.sep}`)) return null; + return { rel, target }; +} + /** Does this request's `Host` name a loopback address? Fails closed. */ function hostIsLoopback(requestUrl: string): boolean { try { @@ -136,13 +167,10 @@ export class WebUiFrontend implements Frontend { const path = new URL(req.url).pathname; if (path !== "/ui" && !path.startsWith("/ui/")) return null; - const rel = - path === "/ui" || path === "/ui/" ? "index.html" : path.slice("/ui/".length); - const target = normalize(join(DIST, rel)); // Path-traversal guard — never serve outside the dist root. - if (target !== DIST && !target.startsWith(`${DIST}/`)) { - return new Response("Forbidden", { status: 403 }); - } + const asset = resolveUiAsset(DIST, path); + if (!asset) return new Response("Forbidden", { status: 403 }); + const { rel, target } = asset; let file = target; if (!existsSync(file)) { diff --git a/src/tests/web-ui-asset-path.test.ts b/src/tests/web-ui-asset-path.test.ts new file mode 100644 index 0000000..b2b0cd4 --- /dev/null +++ b/src/tests/web-ui-asset-path.test.ts @@ -0,0 +1,106 @@ +/** + * The `/ui/*` → file mapping and its path-traversal guard. + * + * The guard used to compare against a hardcoded `${DIST}/`. `normalize()` + * emits the PLATFORM separator, so on Windows every legitimate asset resolved + * to `C:\...\web\dist\index.html` — which does not start with + * `C:\...\web\dist/` — and the entire UI returned 403 (issue #299). + * + * That bug is invisible to a POSIX-only test, so the resolver takes its path + * flavor as an argument and both flavors are asserted here: `path.win32` + * reproduces the Windows failure from Linux CI, `path.posix` proves the fix + * did not loosen containment on the platform that already worked. + */ + +import { describe, expect, it } from "bun:test"; +import path from "node:path"; +import { type PathFlavor, resolveUiAsset } from "../frontends/web-ui/index.js"; + +const FLAVORS: ReadonlyArray<{ name: string; p: PathFlavor; dist: string; outside: string }> = [ + { + name: "posix", + p: path.posix, + dist: "/home/dev/codeoid/web/dist", + outside: "/home/dev/codeoid/web/dist/../../../etc/passwd", + }, + { + name: "win32", + p: path.win32, + dist: "C:\\Users\\dev\\codeoid\\web\\dist", + outside: "C:\\Users\\dev\\.ssh\\id_rsa", + }, +]; + +for (const { name, p, dist } of FLAVORS) { + describe(`resolveUiAsset — ${name}`, () => { + it("serves index.html for the mount root, with and without a trailing slash", () => { + for (const url of ["/ui", "/ui/"]) { + const asset = resolveUiAsset(dist, url, p); + expect(asset).not.toBeNull(); + expect(asset?.rel).toBe("index.html"); + expect(asset?.target).toBe(p.join(dist, "index.html")); + } + }); + + it("resolves a nested asset inside the dist root", () => { + // The regression: on win32 this used to be rejected as a traversal. + const asset = resolveUiAsset(dist, "/ui/assets/index-a1b2c3.js", p); + expect(asset).not.toBeNull(); + expect(asset?.rel).toBe("assets/index-a1b2c3.js"); + expect(asset?.target).toBe(p.join(dist, "assets", "index-a1b2c3.js")); + }); + + it("keeps an SPA deep-link inside the root (no extension → index fallback)", () => { + const asset = resolveUiAsset(dist, "/ui/sessions/abc123", p); + expect(asset).not.toBeNull(); + expect(asset?.rel).toBe("sessions/abc123"); + expect(asset?.rel.includes(".")).toBe(false); + }); + + it("rejects `..` traversal out of the dist root", () => { + for (const url of [ + "/ui/../secret.txt", + "/ui/../../../../etc/passwd", + "/ui/assets/../../../.env", + ]) { + expect(resolveUiAsset(dist, url, p)).toBeNull(); + } + }); + + it("rejects a sibling directory that merely shares the root's prefix", () => { + // `.../web/dist-secrets` starts with `.../web/dist` but is NOT inside it. + expect(resolveUiAsset(dist, "/ui/../dist-secrets/keys.json", p)).toBeNull(); + }); + }); +} + +describe("resolveUiAsset — win32 separator handling", () => { + const dist = "C:\\Users\\dev\\codeoid\\web\\dist"; + + it("produces backslash-separated targets under the dist root", () => { + const asset = resolveUiAsset(dist, "/ui/assets/app.css", path.win32); + expect(asset?.target).toBe("C:\\Users\\dev\\codeoid\\web\\dist\\assets\\app.css"); + expect(asset?.target.startsWith(`${dist}\\`)).toBe(true); + // The pre-fix check — proof this test would catch the regression. + expect(asset?.target.startsWith(`${dist}/`)).toBe(false); + }); + + it("still refuses to escape when the URL smuggles backslashes", () => { + // A raw client can send `\` unencoded; the WHATWG parser folds it to `/` + // for http(s), but assert the resolver holds either way. + expect(resolveUiAsset(dist, "/ui/..\\..\\id_rsa", path.win32)).toBeNull(); + }); +}); + +describe("resolveUiAsset — native flavor", () => { + it("defaults to node:path and admits a real asset under the root", () => { + const dist = path.resolve("/tmp/codeoid-test/web/dist"); + const asset = resolveUiAsset(dist, "/ui/assets/index.js"); + expect(asset?.target).toBe(path.join(dist, "assets", "index.js")); + }); + + it("defaults to node:path and rejects a real traversal", () => { + const dist = path.resolve("/tmp/codeoid-test/web/dist"); + expect(resolveUiAsset(dist, "/ui/../../../etc/passwd")).toBeNull(); + }); +});