Strengthen route and MCP validation tests (codex)

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

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 {

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", () => {

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

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",

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) => {

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 () => {

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", () => {

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"),

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 () => {