From d3e0f0a238ed66a05d50ab6f627eb55874f0f57e Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Sat, 26 Sep 2026 00:48:14 -0700 Subject: [PATCH] fix(server): validate CLI auth challenge IDs (#14095) Reject malformed CLI challenge UUIDs before database access while preserving secret and authentication precedence. Document the HTTP responses and pin the supervisor test fixture to the CI-selected Node executable. Validated by 21 focused tests, root typecheck/build, and full CI. Mac broad-suite baseline limitations are documented in the PR. Greptile 5/5. Co-Authored-By: Paperclip --- doc/CLI.md | 6 ++ server/src/__tests__/cli-auth-routes.test.ts | 73 ++++++++++++++++--- .../local-service-supervisor.test.ts | 10 ++- server/src/routes/access.ts | 3 + server/src/routes/openapi.ts | 12 ++- 5 files changed, 87 insertions(+), 17 deletions(-) diff --git a/doc/CLI.md b/doc/CLI.md index 04264ddf112..c1a6da5659b 100644 --- a/doc/CLI.md +++ b/doc/CLI.md @@ -932,6 +932,12 @@ npx paperclipai auth revoke-current `--token ` is still supported for compatibility, but `--token-env` avoids putting challenge secrets in shell history or process arguments. +Use the challenge UUID returned by `auth challenge create` for get, approve, and +cancel. With the required secret and approval authentication present, malformed +IDs return HTTP 400 before database access. A status request without a secret +returns HTTP 404. Unknown challenges or incorrect challenge secrets still return +HTTP 404. Approval requires board authentication, checked before ID validation. + ## Instance Settings Commands ```sh diff --git a/server/src/__tests__/cli-auth-routes.test.ts b/server/src/__tests__/cli-auth-routes.test.ts index 391510856b8..0531f713ca4 100644 --- a/server/src/__tests__/cli-auth-routes.test.ts +++ b/server/src/__tests__/cli-auth-routes.test.ts @@ -104,7 +104,7 @@ describe.sequential("cli auth routes", () => { it.sequential("creates a CLI auth challenge with approval metadata", async () => { mockBoardAuthService.createCliAuthChallenge.mockResolvedValue({ challenge: { - id: "challenge-1", + id: "12345678-1234-4123-8123-123456789abc", expiresAt: new Date("2026-03-23T13:00:00.000Z"), }, challengeSecret: "pcp_cli_auth_secret", @@ -122,14 +122,14 @@ describe.sequential("cli auth routes", () => { expect(res.status, res.text || JSON.stringify(res.body)).toBe(201); expect(res.body).toMatchObject({ - id: "challenge-1", + id: "12345678-1234-4123-8123-123456789abc", token: "pcp_cli_auth_secret", - approvalPath: "/cli-auth/challenge-1?token=pcp_cli_auth_secret", - pollPath: "/cli-auth/challenges/challenge-1", + approvalPath: "/cli-auth/12345678-1234-4123-8123-123456789abc?token=pcp_cli_auth_secret", + pollPath: "/cli-auth/challenges/12345678-1234-4123-8123-123456789abc", expiresAt: "2026-03-23T13:00:00.000Z", }); expect(res.body.boardApiToken).toBe("pcp_board_token"); - expect(res.body.approvalUrl).toContain("/cli-auth/challenge-1?token=pcp_cli_auth_secret"); + expect(res.body.approvalUrl).toContain("/cli-auth/12345678-1234-4123-8123-123456789abc?token=pcp_cli_auth_secret"); }); it.sequential("rejects anonymous access to generic skill documents", async () => { @@ -176,7 +176,7 @@ describe.sequential("cli auth routes", () => { it.sequential("marks challenge status as requiring sign-in for anonymous viewers", async () => { mockBoardAuthService.describeCliAuthChallenge.mockResolvedValue({ - id: "challenge-1", + id: "12345678-1234-4123-8123-123456789abc", status: "pending", command: "paperclipai company import", clientName: "paperclipai cli", @@ -190,18 +190,67 @@ describe.sequential("cli auth routes", () => { }); const app = await createApp({ type: "none", source: "none" }); - const res = await request(app).get("/api/cli-auth/challenges/challenge-1?token=pcp_cli_auth_secret"); + const res = await request(app).get("/api/cli-auth/challenges/12345678-1234-4123-8123-123456789abc?token=pcp_cli_auth_secret"); expect(res.status).toBe(200); expect(res.body.requiresSignIn).toBe(true); expect(res.body.canApprove).toBe(false); }); + it.each(["PASTE_ID_HERE", "not-a-uuid"])("rejects malformed challenge ID %s before calling the service", async (id) => { + const app = await createApp({ type: "board", userId: "user-1", source: "session" }); + const responses = [ + await request(app).get(`/api/cli-auth/challenges/${id}?token=pcp_cli_auth_secret`), + await request(app).post(`/api/cli-auth/challenges/${id}/approve`).send({ token: "pcp_cli_auth_secret" }), + await request(app).post(`/api/cli-auth/challenges/${id}/cancel`).send({ token: "pcp_cli_auth_secret" }), + ]; + for (const response of responses) { + expect(response.status).toBe(400); + expect(response.body.error).toBe("Invalid CLI auth challenge ID"); + } + expect(mockBoardAuthService.describeCliAuthChallenge).not.toHaveBeenCalled(); + expect(mockBoardAuthService.approveCliAuthChallenge).not.toHaveBeenCalled(); + expect(mockBoardAuthService.cancelCliAuthChallenge).not.toHaveBeenCalled(); + expect(mockLogActivity).not.toHaveBeenCalled(); + }); + + it("requires authentication before approving even a malformed challenge ID", async () => { + const app = await createApp({ type: "none", source: "none" }); + const res = await request(app) + .post("/api/cli-auth/challenges/PASTE_ID_HERE/approve") + .send({ token: "pcp_cli_auth_secret" }); + expect(res.status).toBe(401); + expect(mockBoardAuthService.approveCliAuthChallenge).not.toHaveBeenCalled(); + }); + + it("preserves missing-secret and unknown-challenge responses", async () => { + const app = await createApp({ type: "none", source: "none" }); + const path = "/api/cli-auth/challenges/12345678-1234-4123-8123-123456789abc"; + const missing = await request(app).get(path); + expect(missing.status).toBe(404); + expect(mockBoardAuthService.describeCliAuthChallenge).not.toHaveBeenCalled(); + mockBoardAuthService.describeCliAuthChallenge.mockResolvedValue(null); + const unknown = await request(app).get(`${path}?token=wrong-secret`); + expect(unknown.status).toBe(404); + }); + + it("cancels a valid challenge and preserves path whitespace normalization", async () => { + mockBoardAuthService.cancelCliAuthChallenge.mockResolvedValue({ status: "cancelled" }); + const app = await createApp({ type: "none", source: "none" }); + const id = "12345678-1234-4123-8123-123456789ABC"; + const res = await request(app) + .post(`/api/cli-auth/challenges/${encodeURIComponent(` ${id} `)}/cancel`) + .send({ token: "pcp_cli_auth_secret" }); + expect(res.status).toBe(200); + expect(res.body).toEqual({ status: "cancelled", cancelled: true }); + expect(mockBoardAuthService.cancelCliAuthChallenge).toHaveBeenCalledWith(id, "pcp_cli_auth_secret"); + }); + it.sequential("approves a CLI auth challenge for a signed-in board user", async () => { mockBoardAuthService.approveCliAuthChallenge.mockResolvedValue({ status: "approved", challenge: { - id: "challenge-1", + id: "12345678-1234-4123-8123-123456789abc", boardApiKeyId: "board-key-1", requestedAccess: "board", requestedCompanyId: "company-1", @@ -223,12 +272,12 @@ describe.sequential("cli auth routes", () => { companyIds: ["company-1"], }); const res = await request(app) - .post("/api/cli-auth/challenges/challenge-1/approve") + .post("/api/cli-auth/challenges/12345678-1234-4123-8123-123456789abc/approve") .send({ token: "pcp_cli_auth_secret" }); expect(res.status).toBe(200); expect(mockBoardAuthService.approveCliAuthChallenge).toHaveBeenCalledWith( - "challenge-1", + "12345678-1234-4123-8123-123456789abc", "pcp_cli_auth_secret", "user-1", ); @@ -246,7 +295,7 @@ describe.sequential("cli auth routes", () => { mockBoardAuthService.approveCliAuthChallenge.mockResolvedValue({ status: "approved", challenge: { - id: "challenge-2", + id: "12345678-1234-4123-8123-123456789def", boardApiKeyId: "board-key-2", requestedAccess: "instance_admin_required", requestedCompanyId: null, @@ -263,7 +312,7 @@ describe.sequential("cli auth routes", () => { companyIds: [], }); const res = await request(app) - .post("/api/cli-auth/challenges/challenge-2/approve") + .post("/api/cli-auth/challenges/12345678-1234-4123-8123-123456789def/approve") .send({ token: "pcp_cli_auth_secret" }); expect(res.status).toBe(200); diff --git a/server/src/__tests__/local-service-supervisor.test.ts b/server/src/__tests__/local-service-supervisor.test.ts index bd891224793..afe37b14c4e 100644 --- a/server/src/__tests__/local-service-supervisor.test.ts +++ b/server/src/__tests__/local-service-supervisor.test.ts @@ -31,6 +31,13 @@ describe("local service supervision", () => { process.env.PAPERCLIP_HOME = paperclipHome; process.env.PAPERCLIP_INSTANCE_ID = `service-stdio-${randomUUID()}`; + // The managed service uses a login shell, whose PATH can select a different + // Node installation from the one CI configured for this test process. + // Git Bash accepts Windows drive paths with forward slashes. + const nodeExecutable = process.platform === "win32" + ? process.execPath.replace(/\\/g, "/") + : process.execPath; + const nodeCommand = "'" + nodeExecutable.replace(/'/g, "'\\''") + "'"; let registryRecord: Awaited>[number] | null = null; try { const [service] = await startRuntimeServicesForWorkspaceControl({ @@ -54,7 +61,7 @@ describe("local service supervision", () => { workspaceRuntime: { services: [{ name: "web", - command: "command -v node; node --version; node -e \"const http=require('node:http'); console.log('fixture starting', process.execPath, process.env.PORT); process.on('SIGTERM',()=>{}); http.createServer((req,res)=>{ process.stdout.write('request '+req.url+'\\\\n',(error)=>{ if (!error) res.end('ok'); }); }).listen(Number(process.env.PORT), '127.0.0.1',()=>console.log('fixture listening', process.env.PORT))\"", + command: nodeCommand + " -e \"const http=require('node:http'); console.log('fixture starting', process.execPath, process.env.PORT); process.on('SIGTERM',()=>{}); http.createServer((req,res)=>{ process.stdout.write('request '+req.url+'\\\\n',(error)=>{ if (!error) res.end('ok'); }); }).listen(Number(process.env.PORT), '127.0.0.1',()=>console.log('fixture listening', process.env.PORT))\"", port: { type: "auto" }, readiness: { type: "http", @@ -82,6 +89,7 @@ describe("local service supervision", () => { await expect(fetch(`${service!.url}/after-restart`)).resolves.toMatchObject({ ok: true }); const log = await fs.readFile(resolveLocalServiceLogPath(registryRecord!.serviceKey), "utf8"); expect(log).toContain("request /after-restart"); + expect(log).toContain(process.execPath); await terminateLocalService(registryRecord!); expect(await readLocalServicePortOwner(service!.port!)).toBeNull(); diff --git a/server/src/routes/access.ts b/server/src/routes/access.ts index a6052861ccc..475843cf303 100644 --- a/server/src/routes/access.ts +++ b/server/src/routes/access.ts @@ -2792,6 +2792,7 @@ export function accessRoutes( const token = typeof req.query.token === "string" ? req.query.token.trim() : ""; if (!id || !token) throw notFound("CLI auth challenge not found"); + if (!isUuidLike(id)) throw badRequest("Invalid CLI auth challenge ID"); const challenge = await boardAuth.describeCliAuthChallenge(id, token); if (!challenge) throw notFound("CLI auth challenge not found"); @@ -2824,6 +2825,7 @@ export function accessRoutes( ) { throw unauthorized("Sign in before approving CLI access"); } + if (!isUuidLike(id)) throw badRequest("Invalid CLI auth challenge ID"); const userId = req.actor.userId ?? "local-board"; const approved = await boardAuth.approveCliAuthChallenge( @@ -2871,6 +2873,7 @@ export function accessRoutes( validate(resolveCliAuthChallengeSchema), async (req, res) => { const id = (req.params.id as string).trim(); + if (!isUuidLike(id)) throw badRequest("Invalid CLI auth challenge ID"); const cancelled = await boardAuth.cancelCliAuthChallenge(id, req.body.token); res.json({ status: cancelled.status, diff --git a/server/src/routes/openapi.ts b/server/src/routes/openapi.ts index c7d44d0aa5d..62866294e96 100644 --- a/server/src/routes/openapi.ts +++ b/server/src/routes/openapi.ts @@ -636,6 +636,10 @@ const heartbeatRunIdParamSchema = z.string() .regex(/^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[1-5][0-9a-fA-F]{3}-[89aAbB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}$/) .describe("Heartbeat run UUID; malformed values return 400"); +const cliAuthChallengeIdParamSchema = z.string().trim() + .regex(/^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[1-5][0-9a-fA-F]{3}-[89aAbB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}$/) + .describe("CLI auth challenge UUID; malformed values return 400"); + const ErrorSchema = registry.register("Error", z.object({ error: z.string() })); const responses = { @@ -6468,7 +6472,7 @@ registry.registerPath({ tags: ["access"], summary: "Approve a CLI auth challenge", request: { - params: z.object({ id: z.string() }), + params: z.object({ id: cliAuthChallengeIdParamSchema }), body: jsonBody(resolveCliAuthChallengeSchema), }, responses: { @@ -6485,7 +6489,7 @@ registry.registerPath({ tags: ["access"], summary: "Cancel a CLI auth challenge", request: { - params: z.object({ id: z.string() }), + params: z.object({ id: cliAuthChallengeIdParamSchema }), body: jsonBody(resolveCliAuthChallengeSchema), }, responses: { 200: r.ok(), 400: r.badRequest, 404: r.notFound }, @@ -9201,8 +9205,8 @@ registry.registerPath({ path: "/api/cli-auth/challenges/{id}", tags: ["access"], summary: "Get a CLI auth challenge", - request: { params: z.object({ id: z.string() }) }, - responses: { 200: r.ok(), 404: r.notFound }, + request: { params: z.object({ id: cliAuthChallengeIdParamSchema }) }, + responses: { 200: r.ok(), 400: r.badRequest, 404: r.notFound }, }); // ─── Invite onboarding ────────────────────────────────────────────────────────