From f7a537ccdc2905dc0f2ac2092d7757a799f077f3 Mon Sep 17 00:00:00 2001 From: ding113 Date: Mon, 4 May 2026 10:30:19 +0000 Subject: [PATCH 1/2] fix(keys): register :reveal/:enable/:renew before generic CRUD to fix 400 NaN Hono's RegExpRouter resolves overlapping route matches in registration order. The generic `GET /keys/{keyId}` registered via `keysRouter.openapi(...)` auto-installs a Zod-validating middleware; with the more specific `/keys/:keyId{[0-9]+:reveal}` registered AFTER it, every reveal request matched the generic route first, captured `keyId="155:reveal"`, and was rejected as `expected number, received NaN` before ever reaching `revealKey`. The handler-level suffix strip in `parseKeyParams` was correct but never ran. Move the three custom-method routes (`:enable`, `:renew`, `:reveal`) ahead of the generic GET/PATCH/DELETE block. Add a route-level integration test for `:reveal` (the gap that let this regression slip through) and an OpenAPI doc assertion. Regenerated `openapi-types.gen.ts` reflects only the path-ordering change. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/app/api/v1/resources/keys/router.ts | 130 ++++++----- src/lib/api-client/v1/openapi-types.gen.ts | 254 ++++++++++----------- tests/api/v1/keys/keys.test.ts | 17 ++ 3 files changed, 211 insertions(+), 190 deletions(-) diff --git a/src/app/api/v1/resources/keys/router.ts b/src/app/api/v1/resources/keys/router.ts index fbe05095a..a2166d3e9 100644 --- a/src/app/api/v1/resources/keys/router.ts +++ b/src/app/api/v1/resources/keys/router.ts @@ -111,69 +111,10 @@ keysRouter.openapi( createUserKey as never ); -keysRouter.openapi( - createRoute({ - method: "get", - path: "/keys/{keyId}", - middleware: requireAuth("admin"), - tags: ["Keys"], - summary: "Get key", - description: "Gets one key view through the existing key limit usage guard.", - "x-required-access": "admin", - security, - request: { params: KeyIdParamSchema }, - responses: { - 200: { - description: "Key detail.", - content: { "application/json": { schema: GenericKeyResponseSchema } }, - }, - ...problemResponses, - }, - }), - getKey as never -); - -keysRouter.openapi( - createRoute({ - method: "patch", - path: "/keys/{keyId}", - middleware: requireAuth("admin"), - tags: ["Keys"], - summary: "Update key", - description: "Updates one key.", - "x-required-access": "admin", - security, - request: { - params: KeyIdParamSchema, - body: { required: true, content: { "application/json": { schema: KeyUpdateSchema } } }, - }, - responses: { - 200: { - description: "Update result.", - content: { "application/json": { schema: GenericKeyResponseSchema } }, - }, - ...problemResponses, - }, - }), - updateKey as never -); - -keysRouter.openapi( - createRoute({ - method: "delete", - path: "/keys/{keyId}", - middleware: requireAuth("admin"), - tags: ["Keys"], - summary: "Delete key", - description: "Deletes one key.", - "x-required-access": "admin", - security, - request: { params: KeyIdParamSchema }, - responses: { 204: { description: "Key deleted." }, ...problemResponses }, - }), - deleteKey as never -); - +// Custom-method routes (`/keys/{id}:reveal` etc.) must register before the +// generic `/keys/{keyId}` CRUD routes — Hono's RegExpRouter resolves +// overlapping matches in registration order, and the generic GET would +// otherwise capture `keyId="155:reveal"` and fail Zod coercion as NaN. const enableKeyRoute = createRoute({ method: "post", path: "/keys/{keyId}:enable", @@ -246,6 +187,69 @@ keysRouter.openAPIRegistry.registerPath(revealKeyRoute); // apply (admin OR key owner). Non-owners are rejected at the action layer. keysRouter.get("/keys/:keyId{[0-9]+:reveal}", requireAuth("read"), revealKey); +keysRouter.openapi( + createRoute({ + method: "get", + path: "/keys/{keyId}", + middleware: requireAuth("admin"), + tags: ["Keys"], + summary: "Get key", + description: "Gets one key view through the existing key limit usage guard.", + "x-required-access": "admin", + security, + request: { params: KeyIdParamSchema }, + responses: { + 200: { + description: "Key detail.", + content: { "application/json": { schema: GenericKeyResponseSchema } }, + }, + ...problemResponses, + }, + }), + getKey as never +); + +keysRouter.openapi( + createRoute({ + method: "patch", + path: "/keys/{keyId}", + middleware: requireAuth("admin"), + tags: ["Keys"], + summary: "Update key", + description: "Updates one key.", + "x-required-access": "admin", + security, + request: { + params: KeyIdParamSchema, + body: { required: true, content: { "application/json": { schema: KeyUpdateSchema } } }, + }, + responses: { + 200: { + description: "Update result.", + content: { "application/json": { schema: GenericKeyResponseSchema } }, + }, + ...problemResponses, + }, + }), + updateKey as never +); + +keysRouter.openapi( + createRoute({ + method: "delete", + path: "/keys/{keyId}", + middleware: requireAuth("admin"), + tags: ["Keys"], + summary: "Delete key", + description: "Deletes one key.", + "x-required-access": "admin", + security, + request: { params: KeyIdParamSchema }, + responses: { 204: { description: "Key deleted." }, ...problemResponses }, + }), + deleteKey as never +); + keysRouter.openapi( createRoute({ method: "post", diff --git a/src/lib/api-client/v1/openapi-types.gen.ts b/src/lib/api-client/v1/openapi-types.gen.ts index 5f257dbfa..f8d4eeee5 100644 --- a/src/lib/api-client/v1/openapi-types.gen.ts +++ b/src/lib/api-client/v1/openapi-types.gen.ts @@ -2468,34 +2468,6 @@ export interface paths { patch?: never; trace?: never; }; - "/api/v1/keys/{keyId}": { - parameters: { - query?: never; - header?: never; - path?: never; - cookie?: never; - }; - /** - * Get key - * @description Gets one key view through the existing key limit usage guard. - */ - get: operations["getKeysByKeyid"]; - put?: never; - post?: never; - /** - * Delete key - * @description Deletes one key. - */ - delete: operations["deleteKeysByKeyid"]; - options?: never; - head?: never; - /** - * Update key - * @description Updates one key. - */ - patch: operations["patchKeysByKeyid"]; - trace?: never; - }; "/api/v1/keys/{keyId}:enable": { parameters: { query?: never; @@ -2556,6 +2528,34 @@ export interface paths { patch?: never; trace?: never; }; + "/api/v1/keys/{keyId}": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + /** + * Get key + * @description Gets one key view through the existing key limit usage guard. + */ + get: operations["getKeysByKeyid"]; + put?: never; + post?: never; + /** + * Delete key + * @description Deletes one key. + */ + delete: operations["deleteKeysByKeyid"]; + options?: never; + head?: never; + /** + * Update key + * @description Updates one key. + */ + patch: operations["patchKeysByKeyid"]; + trace?: never; + }; "/api/v1/keys/{keyId}/limits:reset": { parameters: { query?: never; @@ -32806,19 +32806,29 @@ export interface operations { }; }; }; - getKeysByKeyid: { + postKeysByKeyidEnable: { parameters: { query?: never; - header?: never; + header?: { + /** @description Required only when authenticating with the auth-token cookie on mutation requests. */ + "X-CCH-CSRF"?: string; + }; path: { /** @description Key id. */ keyId: number; }; cookie?: never; }; - requestBody?: never; + requestBody: { + content: { + "application/json": { + /** @description Target enabled state. */ + enabled: boolean; + }; + }; + }; responses: { - /** @description Key detail. */ + /** @description Toggle result. */ 200: { headers: { [name: string]: unknown; @@ -32979,7 +32989,7 @@ export interface operations { }; }; }; - deleteKeysByKeyid: { + postKeysByKeyidRenew: { parameters: { query?: never; header?: { @@ -32992,14 +33002,27 @@ export interface operations { }; cookie?: never; }; - requestBody?: never; + requestBody: { + content: { + "application/json": { + /** @description New expiration timestamp. */ + expiresAt: string; + /** @description Enable the key while renewing. */ + enableKey?: boolean; + }; + }; + }; responses: { - /** @description Key deleted. */ - 204: { + /** @description Renew result. */ + 200: { headers: { [name: string]: unknown; }; - content?: never; + content: { + "application/json": { + [key: string]: unknown; + }; + }; }; /** @description Invalid request. */ 400: { @@ -33151,73 +33174,27 @@ export interface operations { }; }; }; - patchKeysByKeyid: { + getKeysByKeyidReveal: { parameters: { query?: never; - header?: { - /** @description Required only when authenticating with the auth-token cookie on mutation requests. */ - "X-CCH-CSRF"?: string; - }; + header?: never; path: { /** @description Key id. */ keyId: number; }; cookie?: never; }; - requestBody: { - content: { - "application/json": { - /** @description Key name. */ - name: string; - /** @description Expiration date or null. */ - expiresAt?: string | unknown; - /** @description Whether the key is enabled. */ - isEnabled?: boolean; - /** @description Whether this key can login to the Web UI. */ - canLoginWebUi?: boolean; - /** @description Five-hour USD quota. */ - limit5hUsd?: number | null; - /** - * @description Five-hour reset mode. - * @enum {string} - */ - limit5hResetMode?: "fixed" | "rolling"; - /** @description Daily USD quota. */ - limitDailyUsd?: number | null; - /** - * @description Daily reset mode. - * @enum {string} - */ - dailyResetMode?: "fixed" | "rolling"; - /** @description Daily reset time in HH:mm. */ - dailyResetTime?: string; - /** @description Weekly USD quota. */ - limitWeeklyUsd?: number | null; - /** @description Monthly USD quota. */ - limitMonthlyUsd?: number | null; - /** @description Total USD quota. */ - limitTotalUsd?: number | null; - /** @description Concurrent session limit. */ - limitConcurrentSessions?: number; - /** @description Provider group expression. */ - providerGroup?: string | null; - /** - * @description Cache TTL preference. - * @enum {string} - */ - cacheTtlPreference?: "inherit" | "5m" | "1h"; - }; - }; - }; + requestBody?: never; responses: { - /** @description Update result. */ + /** @description Unmasked key. */ 200: { headers: { [name: string]: unknown; }; content: { "application/json": { - [key: string]: unknown; + /** @description Unmasked key value. Returned only to admin callers. */ + key: string; }; }; }; @@ -33371,29 +33348,19 @@ export interface operations { }; }; }; - postKeysByKeyidEnable: { + getKeysByKeyid: { parameters: { query?: never; - header?: { - /** @description Required only when authenticating with the auth-token cookie on mutation requests. */ - "X-CCH-CSRF"?: string; - }; + header?: never; path: { /** @description Key id. */ keyId: number; }; cookie?: never; }; - requestBody: { - content: { - "application/json": { - /** @description Target enabled state. */ - enabled: boolean; - }; - }; - }; + requestBody?: never; responses: { - /** @description Toggle result. */ + /** @description Key detail. */ 200: { headers: { [name: string]: unknown; @@ -33554,7 +33521,7 @@ export interface operations { }; }; }; - postKeysByKeyidRenew: { + deleteKeysByKeyid: { parameters: { query?: never; header?: { @@ -33567,27 +33534,14 @@ export interface operations { }; cookie?: never; }; - requestBody: { - content: { - "application/json": { - /** @description New expiration timestamp. */ - expiresAt: string; - /** @description Enable the key while renewing. */ - enableKey?: boolean; - }; - }; - }; + requestBody?: never; responses: { - /** @description Renew result. */ - 200: { + /** @description Key deleted. */ + 204: { headers: { [name: string]: unknown; }; - content: { - "application/json": { - [key: string]: unknown; - }; - }; + content?: never; }; /** @description Invalid request. */ 400: { @@ -33739,27 +33693,73 @@ export interface operations { }; }; }; - getKeysByKeyidReveal: { + patchKeysByKeyid: { parameters: { query?: never; - header?: never; + header?: { + /** @description Required only when authenticating with the auth-token cookie on mutation requests. */ + "X-CCH-CSRF"?: string; + }; path: { /** @description Key id. */ keyId: number; }; cookie?: never; }; - requestBody?: never; + requestBody: { + content: { + "application/json": { + /** @description Key name. */ + name: string; + /** @description Expiration date or null. */ + expiresAt?: string | unknown; + /** @description Whether the key is enabled. */ + isEnabled?: boolean; + /** @description Whether this key can login to the Web UI. */ + canLoginWebUi?: boolean; + /** @description Five-hour USD quota. */ + limit5hUsd?: number | null; + /** + * @description Five-hour reset mode. + * @enum {string} + */ + limit5hResetMode?: "fixed" | "rolling"; + /** @description Daily USD quota. */ + limitDailyUsd?: number | null; + /** + * @description Daily reset mode. + * @enum {string} + */ + dailyResetMode?: "fixed" | "rolling"; + /** @description Daily reset time in HH:mm. */ + dailyResetTime?: string; + /** @description Weekly USD quota. */ + limitWeeklyUsd?: number | null; + /** @description Monthly USD quota. */ + limitMonthlyUsd?: number | null; + /** @description Total USD quota. */ + limitTotalUsd?: number | null; + /** @description Concurrent session limit. */ + limitConcurrentSessions?: number; + /** @description Provider group expression. */ + providerGroup?: string | null; + /** + * @description Cache TTL preference. + * @enum {string} + */ + cacheTtlPreference?: "inherit" | "5m" | "1h"; + }; + }; + }; responses: { - /** @description Unmasked key. */ + /** @description Update result. */ 200: { headers: { [name: string]: unknown; }; content: { "application/json": { - /** @description Unmasked key value. Returned only to admin callers. */ - key: string; + [key: string]: unknown; }; }; }; diff --git a/tests/api/v1/keys/keys.test.ts b/tests/api/v1/keys/keys.test.ts index b75255e58..d55bbf387 100644 --- a/tests/api/v1/keys/keys.test.ts +++ b/tests/api/v1/keys/keys.test.ts @@ -14,6 +14,7 @@ const toggleKeyEnabledMock = vi.hoisted(() => vi.fn()); const renewKeyExpiresAtMock = vi.hoisted(() => vi.fn()); const patchKeyLimitMock = vi.hoisted(() => vi.fn()); const batchUpdateKeysMock = vi.hoisted(() => vi.fn()); +const getUnmaskedKeyMock = vi.hoisted(() => vi.fn()); vi.mock("@/lib/auth", async (importOriginal) => { const actual = await importOriginal(); @@ -32,6 +33,7 @@ vi.mock("@/actions/keys", () => ({ renewKeyExpiresAt: renewKeyExpiresAtMock, patchKeyLimit: patchKeyLimitMock, batchUpdateKeys: batchUpdateKeysMock, + getUnmaskedKey: getUnmaskedKeyMock, })); vi.mock("@/actions/key-quota", () => ({ @@ -86,6 +88,7 @@ describe("v1 key endpoints", () => { ok: true, data: { requestedCount: 1, updatedCount: 1, updatedIds: [10] }, }); + getUnmaskedKeyMock.mockResolvedValue({ ok: true, data: { key: "sk-revealed" } }); }); test("lists and creates user keys", async () => { @@ -266,6 +269,19 @@ describe("v1 key endpoints", () => { expect(removeKeyMock).toHaveBeenCalledWith(10); }); + test("reveals unmasked key value with no-store headers", async () => { + const revealed = await callV1Route({ + method: "GET", + pathname: "/api/v1/keys/10:reveal", + headers, + }); + expect(revealed.response.status).toBe(200); + expect(revealed.json).toEqual({ key: "sk-revealed" }); + expect(revealed.response.headers.get("Cache-Control")).toContain("no-store"); + expect(revealed.response.headers.get("Pragma")).toBe("no-cache"); + expect(getUnmaskedKeyMock).toHaveBeenCalledWith(10); + }); + test("reads and mutates key limit quota endpoints", async () => { const limitUsage = await callV1Route({ method: "GET", @@ -375,6 +391,7 @@ describe("v1 key endpoints", () => { expect(doc.paths).toHaveProperty("/api/v1/keys/{keyId}"); expect(doc.paths).toHaveProperty("/api/v1/keys/{keyId}:enable"); expect(doc.paths).toHaveProperty("/api/v1/keys/{keyId}:renew"); + expect(doc.paths).toHaveProperty("/api/v1/keys/{keyId}:reveal"); expect(doc.paths).toHaveProperty("/api/v1/keys/{keyId}/limits:reset"); expect(doc.paths).toHaveProperty("/api/v1/keys/{keyId}/limit-usage"); expect(doc.paths).toHaveProperty("/api/v1/keys/{keyId}/quota"); From d74992983d6b6dd00ba0dd72fc04d5ca1d6ff50a Mon Sep 17 00:00:00 2001 From: ding113 Date: Mon, 4 May 2026 11:13:49 +0000 Subject: [PATCH 2/2] fix(keys): address PR review on schema accuracy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - expiresAt: switch from `z.union([z.string(), z.null()])` to `z.string().nullable()` so the generated TS type is `string | null` instead of `string | unknown` (which collapses to `unknown` and silently disables type checking on the field). - KeyRevealResponseSchema.key: correct the description — admins AND the key's owner can reveal; non-owners receive 403. The previous "Returned only to admin callers" was misleading consumers. Regenerated openapi-types.gen.ts to reflect both source changes. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/api-client/v1/openapi-types.gen.ts | 6 +++--- src/lib/api/v1/schemas/keys.ts | 8 ++++++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/lib/api-client/v1/openapi-types.gen.ts b/src/lib/api-client/v1/openapi-types.gen.ts index f8d4eeee5..bb01049dd 100644 --- a/src/lib/api-client/v1/openapi-types.gen.ts +++ b/src/lib/api-client/v1/openapi-types.gen.ts @@ -32605,7 +32605,7 @@ export interface operations { /** @description Key name. */ name: string; /** @description Expiration date or null. */ - expiresAt?: string | unknown; + expiresAt?: string | null; /** @description Whether the key is enabled. */ isEnabled?: boolean; /** @description Whether this key can login to the Web UI. */ @@ -33193,7 +33193,7 @@ export interface operations { }; content: { "application/json": { - /** @description Unmasked key value. Returned only to admin callers. */ + /** @description Unmasked key value. Returned to admin callers and to the key owner; non-owners receive 403. */ key: string; }; }; @@ -33712,7 +33712,7 @@ export interface operations { /** @description Key name. */ name: string; /** @description Expiration date or null. */ - expiresAt?: string | unknown; + expiresAt?: string | null; /** @description Whether the key is enabled. */ isEnabled?: boolean; /** @description Whether this key can login to the Web UI. */ diff --git a/src/lib/api/v1/schemas/keys.ts b/src/lib/api/v1/schemas/keys.ts index c22aa619b..a8c47a8b9 100644 --- a/src/lib/api/v1/schemas/keys.ts +++ b/src/lib/api/v1/schemas/keys.ts @@ -17,7 +17,7 @@ export const KeyListQuerySchema = z.object({ const KeyMutationFields = { name: z.string().trim().min(1).max(64).describe("Key name."), - expiresAt: z.union([z.string(), z.null()]).optional().describe("Expiration date or null."), + expiresAt: z.string().nullable().optional().describe("Expiration date or null."), isEnabled: z.boolean().optional().describe("Whether the key is enabled."), canLoginWebUi: z.boolean().optional().describe("Whether this key can login to the Web UI."), limit5hUsd: z.number().min(0).max(10_000).nullable().optional().describe("Five-hour USD quota."), @@ -130,7 +130,11 @@ export const KeyListResponseSchema = z.object({ }); export const KeyRevealResponseSchema = z.object({ - key: z.string().describe("Unmasked key value. Returned only to admin callers."), + key: z + .string() + .describe( + "Unmasked key value. Returned to admin callers and to the key owner; non-owners receive 403." + ), }); export type KeyCreateInput = z.infer;