diff --git a/src/cli.ts b/src/cli.ts index 3ad71b9..7cc17a8 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -11,10 +11,11 @@ export const KNOWN_COMMANDS = [ const args = process.argv.slice(2); const command = args[0]; -// Global flags — take priority over subcommands -if (args.includes("--help") || args.includes("-h")) { +// Global flags — only when in the command position, so a subcommand argument +// named `--help`/`-v` (or a flag passed to a subcommand) isn't hijacked. +if (command === "--help" || command === "-h") { await import("./commands/help.js"); -} else if (args.includes("--version") || args.includes("-v")) { +} else if (command === "--version" || command === "-v") { await import("./commands/version.js"); } else { switch (command) { diff --git a/src/cli/config-io.test.ts b/src/cli/config-io.test.ts index 4082f50..340175f 100644 --- a/src/cli/config-io.test.ts +++ b/src/cli/config-io.test.ts @@ -134,9 +134,9 @@ describe("getServicePort", () => { expect(getServicePort(entry)).toBe(3000); }); - it("returns first port when service not found", () => { + it("returns null when a named service is not found (no silent fallback)", () => { const entry = { ports: { web: 3000 } }; - expect(getServicePort(entry, "missing")).toBe(3000); + expect(getServicePort(entry, "missing")).toBeNull(); }); it("returns port from legacy entry regardless of service param", () => { diff --git a/src/cli/config-io.ts b/src/cli/config-io.ts index a46f14f..d3a31e1 100644 --- a/src/cli/config-io.ts +++ b/src/cli/config-io.ts @@ -98,8 +98,10 @@ export function getEntryPorts(entry: WorktreeEntry): number[] { /** Get port for a specific service, with legacy fallback */ export function getServicePort(entry: WorktreeEntry, service?: string): number | null { if ("ports" in entry) { - if (service && service in entry.ports) return entry.ports[service] ?? null; - // Fallback: first port + // A named service that isn't registered returns null rather than + // silently misrouting to another service's port. Only fall back to the + // first port when no service is requested. + if (service) return entry.ports[service] ?? null; const values = Object.values(entry.ports); return values[0] ?? null; } diff --git a/src/commands/doctor.test.ts b/src/commands/doctor.test.ts index c28b2b1..bb7d615 100644 --- a/src/commands/doctor.test.ts +++ b/src/commands/doctor.test.ts @@ -259,21 +259,23 @@ describe("checkWorktreeConfig", () => { }); it("validates portRange (min < max)", () => { + // worktrees come from the instance file (readProjectConfig); worktreeConfig + // and routes are resolved from dev-proxy.config.mjs into the ProjectConfig. readProjectConfigMock.mockReturnValue({ worktrees: { feat1: { port: 4001 } }, - worktreeConfig: { - portRange: [4000, 5000], - directory: "../{branch}", - }, }); const projects: ProjectConfig[] = [ { path: "/p1", - configPath: "/p1/.dev-proxy.json", - configType: "json", + configPath: "/p1/dev-proxy.config.mjs", + configType: "js", routes: {}, worktrees: {}, + worktreeConfig: { + portRange: [4000, 5000], + directory: "../{branch}", + }, }, ]; @@ -286,19 +288,19 @@ describe("checkWorktreeConfig", () => { it("reports invalid portRange when min >= max", () => { readProjectConfigMock.mockReturnValue({ worktrees: { feat1: { port: 4001 } }, - worktreeConfig: { - portRange: [5000, 4000], - directory: "../{branch}", - }, }); const projects: ProjectConfig[] = [ { path: "/p1", - configPath: "/p1/.dev-proxy.json", - configType: "json", + configPath: "/p1/dev-proxy.config.mjs", + configType: "js", routes: {}, worktrees: {}, + worktreeConfig: { + portRange: [5000, 4000], + directory: "../{branch}", + }, }, ]; @@ -310,19 +312,19 @@ describe("checkWorktreeConfig", () => { it("reports invalid portRange when min === max", () => { readProjectConfigMock.mockReturnValue({ worktrees: { feat1: { port: 4001 } }, - worktreeConfig: { - portRange: [5000, 5000], - directory: "../{branch}", - }, }); const projects: ProjectConfig[] = [ { path: "/p1", - configPath: "/p1/.dev-proxy.json", - configType: "json", + configPath: "/p1/dev-proxy.config.mjs", + configType: "js", routes: {}, worktrees: {}, + worktreeConfig: { + portRange: [5000, 5000], + directory: "../{branch}", + }, }, ]; @@ -333,25 +335,24 @@ describe("checkWorktreeConfig", () => { it("cross-checks services against routes — warns for service not in routes", () => { readProjectConfigMock.mockReturnValue({ - routes: { app: "http://localhost:3000" }, worktrees: { feat1: { ports: { app: 4001, api: 4002 } } }, - worktreeConfig: { - portRange: [4000, 5000], - directory: "../{branch}", - services: { - app: { env: "PORT_APP" }, - api: { env: "PORT_API" }, - }, - }, }); const projects: ProjectConfig[] = [ { path: "/p1", - configPath: "/p1/.dev-proxy.json", - configType: "json", - routes: {}, + configPath: "/p1/dev-proxy.config.mjs", + configType: "js", + routes: { app: "http://localhost:3000" }, worktrees: {}, + worktreeConfig: { + portRange: [4000, 5000], + directory: "../{branch}", + services: { + app: { env: "PORT_APP" }, + api: { env: "PORT_API" }, + }, + }, }, ]; @@ -364,25 +365,24 @@ describe("checkWorktreeConfig", () => { it("reports valid when all services are in routes", () => { readProjectConfigMock.mockReturnValue({ - routes: { app: "http://localhost:3000", api: "http://localhost:4000" }, worktrees: { feat1: { ports: { app: 4001, api: 4002 } } }, - worktreeConfig: { - portRange: [4000, 5000], - directory: "../{branch}", - services: { - app: { env: "PORT_APP" }, - api: { env: "PORT_API" }, - }, - }, }); const projects: ProjectConfig[] = [ { path: "/p1", - configPath: "/p1/.dev-proxy.json", - configType: "json", - routes: {}, + configPath: "/p1/dev-proxy.config.mjs", + configType: "js", + routes: { app: "http://localhost:3000", api: "http://localhost:4000" }, worktrees: {}, + worktreeConfig: { + portRange: [4000, 5000], + directory: "../{branch}", + services: { + app: { env: "PORT_APP" }, + api: { env: "PORT_API" }, + }, + }, }, ]; @@ -467,25 +467,24 @@ describe("checkWorktreeConfig", () => { it("handles wildcard service in cross-check (does not warn for '*')", () => { readProjectConfigMock.mockReturnValue({ - routes: { app: "http://localhost:3000" }, worktrees: { feat1: { ports: { app: 4001 } } }, - worktreeConfig: { - portRange: [4000, 5000], - directory: "../{branch}", - services: { - app: { env: "PORT_APP" }, - "*": { env: "PORT_DEFAULT" }, - }, - }, }); const projects: ProjectConfig[] = [ { path: "/p1", - configPath: "/p1/.dev-proxy.json", - configType: "json", - routes: {}, + configPath: "/p1/dev-proxy.config.mjs", + configType: "js", + routes: { app: "http://localhost:3000" }, worktrees: {}, + worktreeConfig: { + portRange: [4000, 5000], + directory: "../{branch}", + services: { + app: { env: "PORT_APP" }, + "*": { env: "PORT_DEFAULT" }, + }, + }, }, ]; diff --git a/src/commands/doctor.tsx b/src/commands/doctor.tsx index cef0b78..0d8b588 100644 --- a/src/commands/doctor.tsx +++ b/src/commands/doctor.tsx @@ -7,7 +7,11 @@ import * as net from "node:net"; import { Box, Text, render, useApp } from "ink"; import { config, CONFIG_DIR, GLOBAL_CONFIG_PATH } from "../proxy/config.js"; import type { ProjectConfig } from "../proxy/config.js"; -import { getEntryPorts, readProjectConfig } from "../cli/config-io.js"; +import { + getEntryPorts, + readProjectConfig, + type WorktreeConfig, +} from "../cli/config-io.js"; import { Header, Check, Section } from "../cli/output.js"; interface CheckResult { @@ -182,7 +186,11 @@ function checkWorktreeConfig(projects: ProjectConfig[]): CheckResult[] { for (const project of projects) { const cfg = readProjectConfig(project.path); const worktrees = cfg.worktrees ?? {}; - const wtConfig = cfg.worktreeConfig; + // worktreeConfig and routes come from the resolved singleton, which merges + // dev-proxy.config.mjs (the standard format). readProjectConfig reads JSON + // only, so cfg.worktreeConfig is always undefined for mjs-based projects. + const wtConfig = project.worktreeConfig as WorktreeConfig | undefined; + const routes = project.routes; const entries = Object.entries(worktrees); if (entries.length === 0) continue; @@ -222,7 +230,7 @@ function checkWorktreeConfig(projects: ProjectConfig[]): CheckResult[] { // services vs routes cross-check if (wtConfig.services) { - const routeKeys = new Set(Object.keys(cfg.routes ?? {})); + const routeKeys = new Set(Object.keys(routes)); for (const svc of Object.keys(wtConfig.services)) { if (!routeKeys.has(svc) && svc !== "*") { results.push({ diff --git a/src/commands/init.tsx b/src/commands/init.tsx index a283e9a..d7b1fb1 100644 --- a/src/commands/init.tsx +++ b/src/commands/init.tsx @@ -39,8 +39,11 @@ type Step = // ── Validators ────────────────────────────────────────────── function validatePort(value: string): string | null { - const num = parseInt(value, 10); - if (!isValidPort(num)) return `Invalid port "${value}" — must be 1-65535`; + // Number() rejects trailing garbage ("4000abc" → NaN); parseInt would not. + const num = Number(value.trim()); + if (!value.trim() || !isValidPort(num)) { + return `Invalid port "${value}" — must be 1-65535`; + } return null; } @@ -78,8 +81,8 @@ function parseRouteInput(value: string, existing: Route[]): ParseRouteResult { } const portStr = trimmed.slice(eq + 1).trim(); - const portNum = parseInt(portStr, 10); - if (!isValidPort(portNum)) { + const portNum = Number(portStr); + if (!portStr || !isValidPort(portNum)) { return { ok: false, error: `Invalid port "${portStr}" — must be 1-65535` }; } diff --git a/src/commands/migrate.test.ts b/src/commands/migrate.test.ts index 88ebc77..175c704 100644 --- a/src/commands/migrate.test.ts +++ b/src/commands/migrate.test.ts @@ -128,6 +128,22 @@ describe("migrateProject", () => { expect(mockUnlinkSync).toHaveBeenCalled(); }); + it('returns "cleanup-failed" when the legacy file cannot be deleted', () => { + resolveProjectConfigFileMock.mockReturnValue(null); + mockExistsSync.mockReturnValue(true); + readProjectConfigMock.mockReturnValue({ + routes: { app: "http://localhost:3000" }, + }); + mockUnlinkSync.mockImplementation(() => { + throw new Error("EACCES"); + }); + + const result = migrateProject("/p"); + // Contents were migrated, but the stale legacy file remains — surface it. + expect(result).toEqual({ path: "/p", status: "cleanup-failed" }); + expect(writeJsConfigMock).toHaveBeenCalled(); + }); + it("moves worktreeConfig into mjs as the third argument", () => { resolveProjectConfigFileMock.mockReturnValue(null); mockExistsSync.mockReturnValue(true); diff --git a/src/commands/migrate.tsx b/src/commands/migrate.tsx index 23cab74..5d00126 100644 --- a/src/commands/migrate.tsx +++ b/src/commands/migrate.tsx @@ -17,6 +17,7 @@ interface MigrateResult { status: | "migrated" | "cleaned-legacy" + | "cleanup-failed" | "skipped-no-legacy" | "skipped-no-json" | "skipped-no-routes"; @@ -39,8 +40,8 @@ function migrateProject(projectPath: string): MigrateResult { const hasLegacyWtConfig = cfg.worktreeConfig !== undefined; if (!hasWorktrees && !hasLegacyWtConfig) { - cleanupLegacyFile(legacyPath); - return { path: projectPath, status: "cleaned-legacy" }; + const cleaned = cleanupLegacyFile(legacyPath); + return { path: projectPath, status: cleaned ? "cleaned-legacy" : "cleanup-failed" }; } if (hasWorktrees) { @@ -54,8 +55,8 @@ function migrateProject(projectPath: string): MigrateResult { writeJsConfig(projectPath, routes, cfg.worktreeConfig); } - cleanupLegacyFile(legacyPath); - return { path: projectPath, status: "cleaned-legacy" }; + const cleaned = cleanupLegacyFile(legacyPath); + return { path: projectPath, status: cleaned ? "cleaned-legacy" : "cleanup-failed" }; } if (!existsSync(legacyPath)) { @@ -78,17 +79,19 @@ function migrateProject(projectPath: string): MigrateResult { } // .dev-proxy.json no longer holds anything — delete it. - cleanupLegacyFile(legacyPath); + const cleaned = cleanupLegacyFile(legacyPath); - return { path: projectPath, status: "migrated" }; + return { path: projectPath, status: cleaned ? "migrated" : "cleanup-failed" }; } -function cleanupLegacyFile(legacyPath: string): void { - if (!existsSync(legacyPath)) return; +/** Delete the legacy file. Returns false if it still exists afterwards. */ +function cleanupLegacyFile(legacyPath: string): boolean { + if (!existsSync(legacyPath)) return true; try { unlinkSync(legacyPath); + return true; } catch { - /* best-effort cleanup */ + return false; } } @@ -113,8 +116,12 @@ function Migrate() { const migrated = results.filter( (r) => r.status === "migrated" || r.status === "cleaned-legacy", ); + const failed = results.filter((r) => r.status === "cleanup-failed"); const skipped = results.filter( - (r) => r.status !== "migrated" && r.status !== "cleaned-legacy", + (r) => + r.status !== "migrated" && + r.status !== "cleaned-legacy" && + r.status !== "cleanup-failed", ); return ( @@ -133,6 +140,14 @@ function Migrate() { /> ))} + {failed.map((r) => ( + + ))} + {skipped.map((r) => ( {" "} diff --git a/src/commands/worktree.test.ts b/src/commands/worktree.test.ts index c4f48a9..c8f298a 100644 --- a/src/commands/worktree.test.ts +++ b/src/commands/worktree.test.ts @@ -31,6 +31,7 @@ vi.mock("../cli/output.js", () => ({ vi.mock("node:child_process", () => ({ execSync: vi.fn(), + execFileSync: vi.fn(), })); vi.mock("node:fs", () => ({ @@ -108,6 +109,24 @@ describe("findOwningProject", () => { expect(findOwningProject("/home/projectX")).toBeNull(); expect(findOwningProject("/home/projectX/src")).toBeNull(); }); + + it("prefers the longest (most specific) matching prefix for nested projects", () => { + readGlobalConfigMock.mockReturnValue({ + projects: ["/home/user/mono", "/home/user/mono/packages/app"], + }); + + expect(findOwningProject("/home/user/mono/packages/app/src")).toBe( + "/home/user/mono/packages/app", + ); + }); + + it("normalizes trailing slashes and '..' segments before matching", () => { + readGlobalConfigMock.mockReturnValue({ + projects: ["/home/user/project/"], + }); + + expect(findOwningProject("/home/user/project/sub/..")).toBe("/home/user/project/"); + }); }); // ── formatPorts ──────────────────────────────────────────── diff --git a/src/commands/worktree.tsx b/src/commands/worktree.tsx index ee3ab58..704e0ad 100644 --- a/src/commands/worktree.tsx +++ b/src/commands/worktree.tsx @@ -1,4 +1,4 @@ -import { execSync } from "node:child_process"; +import { execFileSync, execSync } from "node:child_process"; import { writeFileSync } from "node:fs"; import { resolve } from "node:path"; import { Box, Text, render } from "ink"; @@ -26,12 +26,20 @@ import { config } from "../proxy/config.js"; function findOwningProject(cwd: string): string | null { const cfg = readGlobalConfig(); const projects = cfg.projects ?? []; + const normalizedCwd = resolve(cwd); + // Normalize both sides (trailing slashes, `..`, symlink-free relative parts) + // and prefer the longest matching prefix so a nested project wins over its + // parent when both are registered. + let best: string | null = null; for (const p of projects) { - if (cwd === p || cwd.startsWith(p + "/")) { - return p; + const normalized = resolve(p); + if (normalizedCwd === normalized || normalizedCwd.startsWith(normalized + "/")) { + if (best === null || normalized.length > resolve(best).length) { + best = p; + } } } - return null; + return best; } /** @@ -114,9 +122,10 @@ function WorktreeAdd({ name, port }: { name: string; port: number }) { } const cfg = readProjectConfig(projectPath); - cfg.worktrees = cfg.worktrees ?? {}; - cfg.worktrees[name] = { port }; - writeProjectConfig(projectPath, cfg); + const worktrees = { ...(cfg.worktrees ?? {}), [name]: { port } }; + // Write only the worktrees file — passing the full merged cfg would + // resurrect a legacy .dev-proxy.json from routes/worktreeConfig read from it. + writeProjectConfig(projectPath, { worktrees }); return ( @@ -160,8 +169,7 @@ function WorktreeRemove({ name }: { name: string }) { } const { [name]: _, ...remaining } = worktrees; - cfg.worktrees = remaining; - writeProjectConfig(projectPath, cfg); + writeProjectConfig(projectPath, { worktrees: remaining }); return ( @@ -263,9 +271,10 @@ function WorktreeCreate({ branch }: { branch: string }) { const messages: string[] = []; const warnings: string[] = []; - // git worktree add + // git worktree add — execFileSync avoids the shell entirely, so the branch + // name can never be interpreted as shell syntax regardless of validation. try { - execSync(`git worktree add ${JSON.stringify(worktreeDir)} ${branch}`, { + execFileSync("git", ["worktree", "add", worktreeDir, branch], { cwd: projectPath, stdio: "pipe", }); @@ -282,12 +291,26 @@ function WorktreeCreate({ branch }: { branch: string }) { ); } - // Update config - cfg.worktrees = { ...worktrees, [branch]: worktreeEntry }; - try { - writeProjectConfig(projectPath, cfg); - } catch (err) { - warnings.push(`Failed to update config: ${(err as Error).message}`); + // Re-read worktrees immediately before writing to narrow the TOCTOU window + // against a concurrent `worktree create`. Detect collisions on either the + // branch or any allocated port, and rebuild on the freshest state. + const fresh = readProjectConfig(projectPath).worktrees ?? {}; + const freshUsedPorts = new Set(Object.values(fresh).flatMap((w) => getEntryPorts(w))); + const collidingPort = getEntryPorts(worktreeEntry).find((p) => freshUsedPorts.has(p)); + if (branch in fresh) { + warnings.push(`Config entry for "${branch}" already added by another process`); + } else if (collidingPort !== undefined) { + warnings.push( + `Port ${collidingPort} was taken by another process — config not updated`, + ); + } else { + try { + writeProjectConfig(projectPath, { + worktrees: { ...fresh, [branch]: worktreeEntry }, + }); + } catch (err) { + warnings.push(`Failed to update config: ${(err as Error).message}`); + } } if (portsMap) { @@ -396,9 +419,9 @@ function WorktreeDestroy({ branch }: { branch: string }) { } } - // git worktree remove + // git worktree remove — execFileSync avoids the shell (no injection via dir) try { - execSync(`git worktree remove ${JSON.stringify(worktreeDir)} --force`, { + execFileSync("git", ["worktree", "remove", worktreeDir, "--force"], { cwd: projectPath, stdio: "pipe", }); @@ -407,12 +430,12 @@ function WorktreeDestroy({ branch }: { branch: string }) { warnings.push(`git worktree remove failed (config entry still removed)`); } - // Update config + // Update config — write only the worktrees file (re-read for freshness) const removed = worktrees[branch] as WorktreeEntry; - const { [branch]: _, ...remaining } = worktrees; - cfg.worktrees = remaining; + const fresh = readProjectConfig(projectPath).worktrees ?? worktrees; + const { [branch]: _, ...remaining } = fresh; try { - writeProjectConfig(projectPath, cfg); + writeProjectConfig(projectPath, { worktrees: remaining }); } catch (err) { warnings.push(`Failed to update config: ${(err as Error).message}`); } @@ -466,6 +489,15 @@ if (subcommand === "create") { const branch = args[1]; if (!branch) { render(); + } else if (!isValidSubdomain(branch)) { + // Validate before any directory resolution / hook execution — an + // unvalidated branch flows into `{branch}` substitution and the hook cwd. + render( + , + ); } else { render(); } diff --git a/src/proxy/server.test.ts b/src/proxy/server.test.ts index a58de29..f8a1d7d 100644 --- a/src/proxy/server.test.ts +++ b/src/proxy/server.test.ts @@ -105,6 +105,47 @@ describe("escapeHtml", () => { "<script>alert("xss")</script>", ); }); + + it("escapes single quotes", () => { + expect(escapeHtml("it's a 'test'")).toBe("it's a 'test'"); + }); +}); + +describe("stripHopByHopHeaders", () => { + const { stripHopByHopHeaders } = __testing; + + it("removes standard hop-by-hop headers", () => { + const headers = { + host: "example.com", + connection: "keep-alive", + "keep-alive": "timeout=5", + "transfer-encoding": "chunked", + upgrade: "h2c", + te: "trailers", + "content-type": "application/json", + }; + stripHopByHopHeaders(headers); + expect(headers).toEqual({ + host: "example.com", + "content-type": "application/json", + }); + }); + + it("removes headers named in the Connection header", () => { + const headers = { + connection: "x-custom, close", + "x-custom": "value", + "x-keep": "keep", + }; + stripHopByHopHeaders(headers); + expect(headers).toEqual({ "x-keep": "keep" }); + }); + + it("leaves a header object without hop-by-hop entries untouched", () => { + const headers = { host: "example.com", accept: "*/*" }; + stripHopByHopHeaders(headers); + expect(headers).toEqual({ host: "example.com", accept: "*/*" }); + }); }); describe("parseCookies", () => { diff --git a/src/proxy/server.ts b/src/proxy/server.ts index 2b4eed2..07a5877 100644 --- a/src/proxy/server.ts +++ b/src/proxy/server.ts @@ -30,7 +30,33 @@ function escapeHtml(s: string): string { .replace(/&/g, "&") .replace(//g, ">") - .replace(/"/g, """); + .replace(/"/g, """) + .replace(/'/g, "'"); +} + +// Hop-by-hop headers must not be forwarded by a proxy (RFC 7230 §6.1). +// Forwarding them can poison the keep-alive agent's pooled connections. +const HOP_BY_HOP_HEADERS = [ + "connection", + "keep-alive", + "proxy-authenticate", + "proxy-authorization", + "te", + "trailer", + "transfer-encoding", + "upgrade", +]; + +/** Remove hop-by-hop headers (and any listed in `Connection`) in place. */ +function stripHopByHopHeaders(headers: http.IncomingHttpHeaders): void { + const connection = headers.connection; + if (typeof connection === "string") { + for (const name of connection.split(",")) { + const key = name.trim().toLowerCase(); + if (key) Reflect.deleteProperty(headers, key); + } + } + for (const key of HOP_BY_HOP_HEADERS) Reflect.deleteProperty(headers, key); } function worktreeErrorPage(worktree: string, target: URL, error: string): string { @@ -196,8 +222,9 @@ function createRequestHandler( emitter.emit("request", event); - // Strip any client-supplied forwarding headers, then set ours + // Strip hop-by-hop and any client-supplied forwarding headers, then set ours const headers = clientReq.headers; + stripHopByHopHeaders(headers); delete headers["x-forwarded-for"]; delete headers["x-forwarded-host"]; delete headers["x-forwarded-proto"]; @@ -209,6 +236,12 @@ function createRequestHandler( headers.host = host.replace(`${worktree}--`, ""); } + // A single request can surface a terminal event from multiple sources + // (proxyRes error, proxyReq error, normal completion). Guard so only the + // first one emits and touches clientRes — otherwise a late error fires a + // second writeHead/end and throws ERR_HTTP_HEADERS_SENT. + let settled = false; + const transport = requestTransport(target); const proxyReq = transport.request( { @@ -226,6 +259,8 @@ function createRequestHandler( responseSize += chunk.length; }); proxyRes.on("end", () => { + if (settled) return; + settled = true; event.statusCode = proxyRes.statusCode; event.duration = Math.round(performance.now() - start); event.responseHeaders = collectDetail ? headersToRecord(proxyRes.headers) : {}; @@ -233,13 +268,20 @@ function createRequestHandler( emitter.emit("request:complete", event); }); proxyRes.on("error", (err) => { + if (settled) return; + settled = true; event.error = err.message; event.duration = Math.round(performance.now() - start); emitter.emit("request:error", event); if (!clientRes.headersSent) clientRes.writeHead(502); - clientRes.end(); + if (!clientRes.writableEnded) clientRes.end(); }); + // Client aborted mid-response — tear down the upstream stream so the + // keep-alive socket isn't left draining bytes with nowhere to go. + clientRes.on("close", () => { + if (!proxyRes.complete) proxyRes.destroy(); + }); clientRes.on("error", () => { // Client disconnected — best-effort, nothing to do proxyRes.destroy(); @@ -251,6 +293,8 @@ function createRequestHandler( ); proxyReq.on("error", (err) => { + if (settled) return; + settled = true; event.error = err.message; event.duration = Math.round(performance.now() - start); emitter.emit("request:error", event); @@ -265,7 +309,7 @@ function createRequestHandler( clientRes.writeHead(502, { "Content-Type": "text/plain" }); clientRes.end(`Dev proxy: target not ready (${err.message})`); } - } else { + } else if (!clientRes.writableEnded) { clientRes.end(); } }); @@ -465,6 +509,7 @@ function resetNextId(): void { export const __testing = { escapeHtml, + stripHopByHopHeaders, parseCookies, parseQuery, headersToRecord, diff --git a/src/proxy/worktrees.test.ts b/src/proxy/worktrees.test.ts index 058a89b..f2d8120 100644 --- a/src/proxy/worktrees.test.ts +++ b/src/proxy/worktrees.test.ts @@ -1,5 +1,6 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { ProjectConfig } from "./config.js"; +import type { WorktreeEntry } from "./worktrees.js"; // ── Mocks ─────────────────────────────────────────────────── // Must be set up before importing the module under test. @@ -96,6 +97,34 @@ describe("isValidEntry", () => { // @ts-expect-error — testing runtime validation expect(isValidEntry({ port: "4000" })).toBe(false); }); + + it("rejects multi-service entry with a non-numeric port value", () => { + // @ts-expect-error — testing runtime validation + expect(isValidEntry({ ports: { web: "abc" } })).toBe(false); + }); + + it("rejects ports that are not positive integers", () => { + expect(isValidEntry({ ports: { web: 0 } })).toBe(false); + expect(isValidEntry({ ports: { web: -1 } })).toBe(false); + expect(isValidEntry({ ports: { web: 3000.5 } })).toBe(false); + expect(isValidEntry({ ports: { web: 70000 } })).toBe(false); + expect(isValidEntry({ port: 0 })).toBe(false); + }); +}); + +describe("getWorktreeTarget — port validation", () => { + it("returns null when an entry's port is out of range", () => { + // Bypass isValidEntry by setting the map directly + __testing.worktreeMap = new Map([["feat", { port: 99999 }]]); + expect(getWorktreeTarget("feat")).toBeNull(); + }); + + it("returns null when a multi-service port is invalid", () => { + // Bypass isValidEntry by casting a deliberately malformed entry + const malformed = { ports: { web: "nope" } } as unknown as WorktreeEntry; + __testing.worktreeMap = new Map([["feat", malformed]]); + expect(getWorktreeTarget("feat", "web")).toBeNull(); + }); }); describe("getWorktreeTarget", () => { @@ -301,10 +330,20 @@ describe("getWorktreeTarget — multi-service", () => { expect(url?.href).toBe("http://localhost:5001/"); }); - it("falls back to first port for unknown service name", () => { + it("returns null for an unknown service name (no silent fallback)", () => { __testing.worktreeMap = new Map([["feat", { ports: { web: 5000, api: 5001 } }]]); + // A named service that isn't registered must not route to another + // service's port — that masks misconfiguration. Per the routing invariant, + // an unresolved worktree service yields the offline error page, not a guess. const url = getWorktreeTarget("feat", "nonexistent"); + expect(url).toBeNull(); + }); + + it("falls back to the first port only when no service is given", () => { + __testing.worktreeMap = new Map([["feat", { ports: { web: 5000, api: 5001 } }]]); + + const url = getWorktreeTarget("feat"); expect(url).toBeInstanceOf(URL); expect(url?.href).toBe("http://localhost:5000/"); }); diff --git a/src/proxy/worktrees.ts b/src/proxy/worktrees.ts index 1fe05c4..35beecd 100644 --- a/src/proxy/worktrees.ts +++ b/src/proxy/worktrees.ts @@ -36,11 +36,16 @@ function notify() { // ── Core API ───────────────────────────────────────────────── +function isValidPort(port: unknown): port is number { + return typeof port === "number" && Number.isInteger(port) && port > 0 && port < 65536; +} + function isValidEntry(entry: WorktreeEntry): boolean { if ("ports" in entry) { - return Object.keys(entry.ports).length > 0; + const values = Object.values(entry.ports); + return values.length > 0 && values.every(isValidPort); } - return typeof entry.port === "number"; + return isValidPort(entry.port); } function readRegistry(): void { @@ -120,14 +125,15 @@ export function getWorktreeTarget(branch: string, service?: string): URL | null if (!entry) return null; if ("ports" in entry) { - const port = - service && service in entry.ports - ? entry.ports[service] - : Object.values(entry.ports)[0]; - if (port === undefined) return null; + // A named service that isn't registered must not silently fall back to + // another service's port — that masks misconfiguration and can route to + // the wrong app. Only default to the first port when no service is given. + const port = service ? entry.ports[service] : Object.values(entry.ports)[0]; + if (!isValidPort(port)) return null; return new URL(`http://localhost:${port}`); } + if (!isValidPort(entry.port)) return null; return new URL(`http://localhost:${entry.port}`); } @@ -152,6 +158,7 @@ export function useWorktrees(): Map { } export const __testing = { + isValidPort, isValidEntry, readRegistry, readProjectWorktrees, diff --git a/src/store.test.ts b/src/store.test.ts index e842398..e132e75 100644 --- a/src/store.test.ts +++ b/src/store.test.ts @@ -196,6 +196,47 @@ describe("dev-proxy store", () => { expect(getSelectedDetail()).not.toBeNull(); }); + it("merges response detail collected idle when the request started while active", () => { + setInspectActive(true); + + // Request starts while active — request detail is captured. + pushHttp( + makeHttpEvent({ + id: "req-1", + url: "/slow", + requestHeaders: { host: "www.example.dev:3000", "x-trace": "abc" }, + responseHeaders: {}, + }), + ); + expect(getSelectedDetail()?.requestHeaders).toMatchObject({ "x-trace": "abc" }); + + // Detail collection goes idle before the response arrives. + setInspectActive(false); + + // request:complete arrives while idle — the existing detail entry must be + // updated with the response headers rather than dropped. + pushHttp( + makeHttpEvent({ + id: "req-1", + url: "/slow", + statusCode: 200, + requestHeaders: { host: "www.example.dev:3000", "x-trace": "abc" }, + responseHeaders: { "content-type": "application/json" }, + }), + ); + + const detail = getSelectedDetail(); + expect(detail).not.toBeNull(); + expect(detail?.responseHeaders).toMatchObject({ "content-type": "application/json" }); + }); + + it("does not start collecting detail for a brand-new request while idle", () => { + setInspectActive(false); + pushHttp(makeHttpEvent({ id: "req-idle", url: "/idle" })); + expect(getSelected()?.id).toBe("req-idle"); + expect(getSelectedDetail()).toBeNull(); + }); + it("tracks active websocket count without truncating long-running sessions", () => { for (let i = 0; i < 35; i++) { pushWs(makeWsEvent({ id: `ws-${i}`, status: "open", url: `/socket/${i}` })); diff --git a/src/store.ts b/src/store.ts index 335bbf3..52e3954 100644 --- a/src/store.ts +++ b/src/store.ts @@ -185,6 +185,9 @@ function evictExcess() { } } events.length = writeIdx; + // The events array was compacted in place — invalidate the filtered cache + // explicitly rather than relying on the caller's revision-bump ordering. + cachedFiltered = null; } function shellQuote(value: string): string { @@ -195,7 +198,10 @@ export function pushHttp(event: ProxyRequestEvent) { eventsRevision++; let slim: SlimRequestEvent; let detail: RequestDetail | null; - if (detailActive) { + // Collect detail when active, OR when this id already has a detail entry — + // otherwise a request that started while active loses its response headers + // when its `request:complete` arrives after the idle timeout. + if (detailActive || detailMap.has(event.id)) { const split = splitEvent(event); slim = split.slim; detail = split.detail;