diff --git a/docs/operations/runbook.md b/docs/operations/runbook.md index f9ec24bbbd..cf4f0f66f2 100644 --- a/docs/operations/runbook.md +++ b/docs/operations/runbook.md @@ -36,8 +36,10 @@ If started without key: ### Dashboard and file access boundaries - Dashboard mutation requests to `/api/*`, `/health`, and `/ready` reject hostile browser origins. Treat `Sec-Fetch-Site: cross-site`, malformed `Origin`, cross-host `Origin`, malformed `Referer`, and cross-host `Referer` on mutations as blocked browser requests; CLI/API clients without browser origin headers remain allowed. - Local directory browsing only lists canonical paths inside the allowed local roots. Encoded traversal, Windows-style separator traversal, absolute paths outside the roots, and symlink escapes must not expose directory listings or leak requested paths in error responses. -- Sprint file-browser file and diff reads accept normalized relative paths only. Encoded traversal, `..` traversal, Windows drive paths, and absolute paths are rejected before provider or Docker-backed file-browser dependencies are invoked. -- MCP approval prompts are one-time, correlation-id-bound decisions. Expired, mismatched, duplicate, blank, or malformed correlation IDs must not return a pending approval. +- Sprint file-browser file and diff reads accept normalized relative paths only. Encoded traversal, malformed percent encoding, `..` traversal, Windows drive paths, and absolute paths are rejected as malformed client input before provider or Docker-backed file-browser dependencies are invoked. +- Provider login terminal requests reject provider configuration IDs with path separators, traversal sequences, absolute-path syntax, encoded separators, control characters, leading hyphens, or characters outside the filesystem-safe ID set before credential directories are removed, created, or copied. +- Malformed dashboard route inputs should return client errors (`400`, `403`, or `404` depending on the failure). Unexpected server failures should return only `{ "error": "Internal Server Error" }` to callers while still flowing to Express error handling and structured logs. +- MCP approval prompts are one-time, correlation-id-bound decisions. Expired, mismatched, duplicate, blank, or malformed correlation IDs must not return a pending approval, and destructive settings approvals are bound to the exact action and payload that was queued. - Structured logs and invocation output pass through redaction helpers before storage or display. Secret-like environment assignments, authorization headers, hosted Git tokens, and URL credentials should appear only as `[REDACTED]` in logs and provider output. ### Emergency stop diff --git a/src/server/file-browser-routes.ts b/src/server/file-browser-routes.ts index 1b2b5709db..d6af5612e7 100644 --- a/src/server/file-browser-routes.ts +++ b/src/server/file-browser-routes.ts @@ -5,7 +5,14 @@ import { requireTrimmedString } from "./request-parsers.js"; import { normalizeAndValidatePath } from "../services/file-browser-scan-policy.js"; function requireValidatedFileBrowserPath(value: unknown): string { - return normalizeAndValidatePath(requireTrimmedString(value, "path")); + try { + return normalizeAndValidatePath(requireTrimmedString(value, "path")); + } catch (error) { + if (error instanceof URIError) { + throw new Error("Invalid file path: malformed encoding"); + } + throw error; + } } export function registerFileBrowserRoutes(app: Express, deps: DashboardDependencies): void { diff --git a/tests/backend/mcp/management-payload-parsers.test.ts b/tests/backend/mcp/management-payload-parsers.test.ts index 71c6cf511a..91dd75be23 100644 --- a/tests/backend/mcp/management-payload-parsers.test.ts +++ b/tests/backend/mcp/management-payload-parsers.test.ts @@ -21,6 +21,11 @@ describe("Payload Parsers", () => { expect(() => parseRequiredString({}, "foo", "Custom error!")).toThrow("Custom error!"); }); + it("parseRequiredString treats null and non-string values as validation errors", () => { + expect(() => parseRequiredString({ foo: null }, "foo")).toThrow("foo is required"); + expect(() => parseRequiredString({ foo: 42 }, "foo")).toThrow("foo is required"); + }); + it("parseOptionalString", () => { expect(parseOptionalString({ foo: " bar " }, "foo")).toBe("bar"); expect(parseOptionalString({ foo: " " }, "foo")).toBeUndefined(); @@ -49,6 +54,12 @@ describe("Payload Parsers", () => { expect(parseOptionalBoolean({}, "foo")).toBeUndefined(); }); + it("parseOptionalBoolean does not coerce string or numeric booleans", () => { + expect(parseOptionalBoolean({ foo: "false" }, "foo")).toBeUndefined(); + expect(parseOptionalBoolean({ foo: 0 }, "foo")).toBeUndefined(); + expect(parseOptionalBoolean({ foo: 1 }, "foo")).toBeUndefined(); + }); + it("parseOptionalObject", () => { expect(parseOptionalObject({ foo: { a: 1 } }, "foo")).toEqual({ a: 1 }); expect(parseOptionalObject({ foo: [1, 2] }, "foo")).toBeUndefined(); @@ -78,6 +89,10 @@ describe("Payload Parsers", () => { .toThrow("Invalid value for count. Must be a valid integer."); expect(() => parseOptionalIntegerStrict({ count: "0" }, "count", { min: 1 })) .toThrow("Invalid value for count. Must be at least 1."); + expect(() => parseOptionalIntegerStrict({ count: "11" }, "count", { max: 10 })) + .toThrow("Invalid value for count. Must be at most 10."); + expect(() => parseOptionalIntegerStrict({ count: "" }, "count")) + .toThrow("Invalid value for count. Must be a valid integer."); }); it("formats validation and runtime error envelopes consistently", () => { diff --git a/tests/backend/mcp/mcp-management.test.ts b/tests/backend/mcp/mcp-management.test.ts index 44b0e37ff9..9a489fd443 100644 --- a/tests/backend/mcp/mcp-management.test.ts +++ b/tests/backend/mcp/mcp-management.test.ts @@ -67,6 +67,8 @@ describe("ManagementToolHandler", () => { }, settingsRepository: { getGlobalSettings: vi.fn(), + getSystemSettings: vi.fn(() => ({ defaults: { automationLevel: "FULL" } })), + saveSystemSettings: vi.fn((settings: unknown) => settings), }, agentPresetSyncService: { syncPresets: vi.fn(), @@ -215,6 +217,53 @@ describe("ManagementToolHandler", () => { }); }); + it("does not let a destructive settings approval be reused", async () => { + const payload = { path: "defaults.automationLevel", value: "SEMI_AUTO" }; + + let response = await handler.handleManageSettings({ + action: "patch_system_setting", + ...payload, + }); + let parsed = JSON.parse(response.content[0].text); + expect(parsed.approvalRequired).toBe(true); + + response = await handler.handleManageSettings({ + action: "patch_system_setting", + ...payload, + approval: { confirmed: true }, + }); + parsed = JSON.parse(response.content[0].text); + expect(parsed.result.settings.defaults.automationLevel).toBe("SEMI_AUTO"); + + response = await handler.handleManageSettings({ + action: "patch_system_setting", + ...payload, + approval: { confirmed: true }, + }); + parsed = JSON.parse(response.content[0].text); + expect(parsed.approvalRequired).toBe(true); + expect(deps.settingsRepository.saveSystemSettings).toHaveBeenCalledTimes(1); + }); + + it("does not let a destructive settings approval execute a mismatched payload", async () => { + await handler.handleManageSettings({ + action: "patch_system_setting", + path: "defaults.automationLevel", + value: "SEMI_AUTO", + }); + + const response = await handler.handleManageSettings({ + action: "patch_system_setting", + path: "defaults.automationLevel", + value: "MANUAL", + approval: { confirmed: true }, + }); + const parsed = JSON.parse(response.content[0].text); + + expect(parsed.approvalRequired).toBe(true); + expect(deps.settingsRepository.saveSystemSettings).not.toHaveBeenCalled(); + }); + it("should return approvalRequired for destructive actions without approval in handleManageCodeUx", async () => { const response = await handler.handleManageCodeUx({ domain: "unknown", action: "delete_something", payload: {} }); const parsed = JSON.parse(response.content[0].text); diff --git a/tests/backend/server/dashboard-routes-error.test.ts b/tests/backend/server/dashboard-routes-error.test.ts index 58595bfc2a..3616824552 100644 --- a/tests/backend/server/dashboard-routes-error.test.ts +++ b/tests/backend/server/dashboard-routes-error.test.ts @@ -83,6 +83,18 @@ describe("dashboard route handlers", () => { body: { error: "Route conflict" }, expectedNextError: null, }, + { + label: "explicit forbidden HttpRouteError", + route: "/api/status", + deps: { + getStatus: () => { + throw new HttpRouteError(403, "Forbidden request"); + }, + }, + status: 403, + body: { error: "Forbidden request" }, + expectedNextError: null, + }, { label: "unexpected sync error", route: "/api/status", diff --git a/tests/backend/server/file-browser-routes.test.ts b/tests/backend/server/file-browser-routes.test.ts index 4df3109364..acef896f25 100644 --- a/tests/backend/server/file-browser-routes.test.ts +++ b/tests/backend/server/file-browser-routes.test.ts @@ -34,6 +34,8 @@ describe("file browser routes", () => { ["encoded traversal", "src/%2e%2e/package.json"], ["Windows-style traversal", "src\\..\\package.json"], ["Unix traversal", "../package.json"], + ["encoded slash traversal", "src%2f..%2fpackage.json"], + ["malformed percent encoding", "src/%E0%A4%A"], ["Unix absolute path", "/etc/passwd"], ["Windows absolute path", "C:\\Windows\\System32\\drivers\\etc\\hosts"], ])("rejects %s before reading a file", async (_label, hostilePath) => { @@ -52,6 +54,8 @@ describe("file browser routes", () => { ["encoded traversal", "src/%2e%2e/package.json"], ["Windows-style traversal", "src\\..\\package.json"], ["Unix traversal", "../package.json"], + ["encoded slash traversal", "src%2f..%2fpackage.json"], + ["malformed percent encoding", "src/%E0%A4%A"], ["Unix absolute path", "/etc/passwd"], ["Windows absolute path", "C:\\Windows\\System32\\drivers\\etc\\hosts"], ])("rejects %s before reading a diff", async (_label, hostilePath) => { diff --git a/tests/backend/server/local-directory-routes.test.ts b/tests/backend/server/local-directory-routes.test.ts index 717073b147..250661d8b9 100644 --- a/tests/backend/server/local-directory-routes.test.ts +++ b/tests/backend/server/local-directory-routes.test.ts @@ -3,7 +3,7 @@ import * as fs from "fs/promises"; import * as os from "os"; import * as path from "path"; import request from "supertest"; -import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { registerLocalDirectoryRoutes } from "../../../src/server/local-directory-routes.js"; const tempDirs: string[] = []; @@ -13,6 +13,7 @@ beforeEach(() => { }); afterEach(async () => { + vi.restoreAllMocks(); await Promise.all(tempDirs.splice(0).map((dir) => fs.rm(dir, { recursive: true, force: true }))); delete process.env.CODE_UX_DIRECTORY_BROWSER_ROOTS; }); @@ -72,22 +73,30 @@ describe("local directory routes", () => { it("rejects path traversal outside allowed roots", async () => { const rootDir = path.parse(process.cwd()).root; + const statSpy = vi.spyOn(fs, "stat"); + const readdirSpy = vi.spyOn(fs, "readdir"); // Assuming rootDir is not an allowed root const response = await request(createApp()).get("/api/local-directories").query({ path: rootDir }); expect(response.status).toBe(403); expect(response.body.error).toBe("Access denied"); + expect(statSpy).not.toHaveBeenCalled(); + expect(readdirSpy).not.toHaveBeenCalled(); }); it("rejects encoded traversal that resolves outside allowed roots", async () => { const dir = await fs.mkdtemp(path.join(os.tmpdir(), "code-ux-local-directories-")); tempDirs.push(dir); const encodedTraversal = `${dir}/%2e%2e/%2e%2e`; + const statSpy = vi.spyOn(fs, "stat"); + const readdirSpy = vi.spyOn(fs, "readdir"); const response = await request(createApp()).get(`/api/local-directories?path=${encodedTraversal}`); expect(response.status).toBe(403); expect(response.body.error).toBe("Access denied"); expect(response.text).not.toContain(dir); + expect(statSpy).not.toHaveBeenCalled(); + expect(readdirSpy).not.toHaveBeenCalled(); }); it("rejects Windows-style separator traversal attempts without listing directories", async () => { @@ -103,11 +112,15 @@ describe("local directory routes", () => { it("rejects absolute paths outside the allowed roots", async () => { const outsideRoot = path.parse(process.cwd()).root; + const statSpy = vi.spyOn(fs, "stat"); + const readdirSpy = vi.spyOn(fs, "readdir"); const response = await request(createApp()).get("/api/local-directories").query({ path: outsideRoot }); expect(response.status).toBe(403); expect(response.body.error).toBe("Access denied"); + expect(statSpy).not.toHaveBeenCalled(); + expect(readdirSpy).not.toHaveBeenCalled(); }); it("rejects symlink escapes outside allowed roots", async () => { diff --git a/tests/backend/server/request-parsers.test.ts b/tests/backend/server/request-parsers.test.ts index 2511b9061f..d7693e4775 100644 --- a/tests/backend/server/request-parsers.test.ts +++ b/tests/backend/server/request-parsers.test.ts @@ -47,6 +47,12 @@ describe("Request Parsers", () => { expect(parseOptionalBoolean(null)).toBeUndefined(); expect(parseOptionalBoolean(undefined)).toBeUndefined(); }); + + it("parseOptionalBoolean rejects empty strings instead of silently coercing", () => { + expect(() => parseOptionalBoolean("")).toThrow("Invalid boolean value for field."); + expect(() => parseOptionalBoolean(" ", "enabled")).toThrow("Invalid boolean value for enabled."); + }); + it("parseOptionalInteger handles strings, numbers, flooring, and throws on invalid boundaries and formats", () => { expect(parseOptionalInteger(2.8)).toBe(2); expect(parseOptionalInteger("5")).toBe(5); @@ -59,6 +65,15 @@ describe("Request Parsers", () => { expect(() => parseOptionalInteger(Infinity)).toThrow(/valid integer/); expect(() => parseOptionalInteger({})).toThrow(/valid integer/); }); + + it("parseOptionalInteger preserves nullable fields and enforces inclusive integer bounds", () => { + expect(parseOptionalInteger(null, 1, 5, "count")).toBeUndefined(); + expect(parseOptionalInteger(undefined, 1, 5, "count")).toBeUndefined(); + expect(parseOptionalInteger("1", 1, 5, "count")).toBe(1); + expect(parseOptionalInteger("5", 1, 5, "count")).toBe(5); + expect(() => parseOptionalInteger("0", 1, 5, "count")).toThrow("Invalid value for count. Must be between 1 and 5."); + expect(() => parseOptionalInteger("6", 1, 5, "count")).toThrow("Invalid value for count. Must be between 1 and 5."); + }); }); describe("requireTrimmedString", () => { @@ -245,6 +260,8 @@ describe("Request Parsers", () => { it("rejects invalid enum values", () => { expect(() => parseCreateProjectInput({ name: "n", sourceType: "ftp", sourceRef: "r" })).toThrow(/sourceType/); + expect(() => parseCreateProjectInput({ name: "n", sourceType: null, sourceRef: "r" })).toThrow(/sourceType/); + expect(() => parseCreateProjectInput({ name: "n", sourceType: "", sourceRef: "r" })).toThrow(/sourceType/); }); }); @@ -255,6 +272,20 @@ describe("Request Parsers", () => { expect(result.featureBranchPrefix).toBe("feat/"); }); + it("drops empty optional strings while preserving nullable project fields", () => { + const result = parseUpdateProjectInput({ + name: " ", + sourceRef: "", + defaultBranch: null, + featureBranchPrefix: null, + }); + + expect(result.name).toBe(""); + expect(result.sourceRef).toBe(""); + expect(result.defaultBranch).toBeNull(); + expect(result.featureBranchPrefix).toBeNull(); + }); + it("rejects a non-object body", () => { expect(() => parseUpdateProjectInput(42)).toThrow(/body must be an object/); }); @@ -277,6 +308,13 @@ describe("Request Parsers", () => { expect(result.number).toBe(7); expect(result.showcasePinned).toBe(true); }); + + it("rejects invalid sprint enums and out-of-range numbers", () => { + expect(() => parseCreateSprintInput({ status: "blocked" })).toThrow(/status/); + expect(() => parseUpdateSprintInput({ status: "blocked" })).toThrow(/status/); + expect(() => parseCreateSprintInput({ number: 1000001 })).toThrow(/between -1000000 and 1000000/); + expect(() => parseUpdateSprintInput({ number: -1000001 })).toThrow(/between -1000000 and 1000000/); + }); }); describe("task parsers", () => { @@ -300,6 +338,7 @@ describe("Request Parsers", () => { it("rejects an invalid task priority", () => { expect(() => parseCreateTaskInput({ sprintId: "s", title: "t", priority: "urgent" })).toThrow(/priority/); + expect(() => parseUpdateTaskInput({ executorType: "manual" })).toThrow(/executorType/); }); it("parses update task optional fields", () => { @@ -333,6 +372,10 @@ describe("Request Parsers", () => { expect(parseQuicksprintExecutionInput({ templateId: "t", taskCount: "2", submitMode: "plan_and_start" })).toMatchObject({ taskCount: 2, submitMode: "plan_and_start" }); expect(parseQuicksprintExecutionInput({ templateId: "t", submitMode: "plan_only", noTaskLimit: true })).toMatchObject({ taskCount: 5, noTaskLimit: true, submitMode: "plan_only" }); }); + + it("does not let noTaskLimit bypass invalid boolean coercion", () => { + expect(() => parseQuicksprintExecutionInput({ templateId: "t", submitMode: "plan_only", noTaskLimit: "yes" })).toThrow(/Invalid boolean value/); + }); }); describe("parseThreadRouteInput", () => { diff --git a/tests/backend/server/route-utils.test.ts b/tests/backend/server/route-utils.test.ts index 15bd0e9d79..6385f24c3c 100644 --- a/tests/backend/server/route-utils.test.ts +++ b/tests/backend/server/route-utils.test.ts @@ -70,6 +70,13 @@ describe("route-utils", () => { body: { error: "Conflict while updating route" }, delegatesToNext: false, }, + { + label: "explicit forbidden HttpRouteError", + error: new HttpRouteError(403, "Forbidden path"), + status: 403, + body: { error: "Forbidden path" }, + delegatesToNext: false, + }, { label: "unexpected error", error: new Error("Database password leaked in stack"), diff --git a/tests/backend/server/terminal-routes.test.ts b/tests/backend/server/terminal-routes.test.ts index 9f9bc565fe..c347ddffa0 100644 --- a/tests/backend/server/terminal-routes.test.ts +++ b/tests/backend/server/terminal-routes.test.ts @@ -3,6 +3,7 @@ import request from "supertest"; import express from "express"; import { EventEmitter } from "events"; import { spawn } from "child_process"; +import * as fs from "fs/promises"; import { registerTerminalRoutes, bootDashboardTerminalWebSocketServer, @@ -116,6 +117,7 @@ describe("Terminal Routes", () => { mockStderr.removeAllListeners(); mockLoginImageState.holdBuild = false; mockLoginImageState.finishBuild = null; + vi.restoreAllMocks(); }); it("should reject websocket upgrades from hostile origins", async () => { @@ -154,21 +156,36 @@ describe("Terminal Routes", () => { // A directly-supplied (valid) providerId bypasses the providerConfigId // lookup, so without validation the traversal value would reach the // destructive credential fs.rm/mkdir/cp. The handler must 400 first. + const rmSpy = vi.spyOn(fs, "rm"); + const mkdirSpy = vi.spyOn(fs, "mkdir"); + const response = await request(app) .post("/api/terminal/start") .send({ providerId: "codex", providerConfigId: "../../../../tmp/evil" }); expect(response.status).toBe(400); expect(String(response.body.error)).toMatch(/providerConfigId/i); + expect(rmSpy).not.toHaveBeenCalled(); + expect(mkdirSpy).not.toHaveBeenCalled(); }); - it("rejects a providerConfigId containing path separators", async () => { + it.each([ + ["Unix separator", "codex/../../secrets"], + ["Windows separator", "codex\\..\\secrets"], + ["absolute path", "/tmp/secrets"], + ["encoded separator", "codex%2fsecrets"], + ])("rejects a providerConfigId containing %s", async (_label, providerConfigId) => { + const rmSpy = vi.spyOn(fs, "rm"); + const mkdirSpy = vi.spyOn(fs, "mkdir"); + const response = await request(app) .post("/api/terminal/start") - .send({ providerId: "claude-code", providerConfigId: "codex/../../secrets" }); + .send({ providerId: "claude-code", providerConfigId }); expect(response.status).toBe(400); expect(String(response.body.error)).toMatch(/providerConfigId/i); + expect(rmSpy).not.toHaveBeenCalled(); + expect(mkdirSpy).not.toHaveBeenCalled(); }); it("should close the socket when receiving oversized frames", async () => {