From b6d2f0f82b9210a206e9b3daef4edd21328bca25 Mon Sep 17 00:00:00 2001 From: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Date: Thu, 10 Sep 2026 19:06:29 -0300 Subject: [PATCH] fix(api): reject revoked, deactivated, banned or expired keys on DELETE /v1/batches/delete-completed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fail-closed gate added in #13262 authorized a presented key by row EXISTENCE (getApiKeyMetadata); a revoked/deactivated/banned/expired key still has a row and ran the sweep (CWE-613). The route now also requires validateApiKey() — the one lifecycle gate — before choosing a scope, and neither an unresolved nor an invalid key falls through to the session branch. Both 401 bodies now go through buildErrorBody() (Hard Rule #12). Found by the omni-code-sec battery on #12969 (SEC-B, SEC-E, SEC-F); 4 negative route tests added (revoked, deactivated, banned, expired — the last one alongside a session cookie). Refs #12969 --- ...2969-batches-delete-completed-ownership.md | 2 +- .../api/v1/batches/delete-completed/route.ts | 33 +++++---- ...tches-delete-completed-route-scope.test.ts | 72 ++++++++++++++++++- 3 files changed, 92 insertions(+), 15 deletions(-) diff --git a/changelog.d/fixes/12969-batches-delete-completed-ownership.md b/changelog.d/fixes/12969-batches-delete-completed-ownership.md index 29824bf729..da4081d555 100644 --- a/changelog.d/fixes/12969-batches-delete-completed-ownership.md +++ b/changelog.d/fixes/12969-batches-delete-completed-ownership.md @@ -1 +1 @@ -- **fix(api):** `DELETE /v1/batches/delete-completed` now sweeps only the calling API key's own completed batches (batches with no owner stay out of a key-scoped sweep on purpose), with an explicit instance-wide mode reserved for authenticated dashboard sessions, audit logging of every sweep, a sanitized 500 on failure and an atomic sweep so a mid-way error never leaves a batch pointing at a nulled file (GHSA-wvxc-jp3v-5mg5) ([#12969](https://github.com/diegosouzapw/OmniRoute/pull/12969)) +- **fix(api):** `DELETE /v1/batches/delete-completed` now sweeps only the calling API key's own completed batches (batches with no owner stay out of a key-scoped sweep on purpose), with an explicit instance-wide mode reserved for authenticated dashboard sessions, a 401 for a presented key that is unknown, revoked, deactivated, banned or expired (never falling through to the session branch), audit logging of every sweep, a sanitized 500 on failure and an atomic sweep so a mid-way error never leaves a batch pointing at a nulled file (GHSA-wvxc-jp3v-5mg5) ([#12969](https://github.com/diegosouzapw/OmniRoute/pull/12969)) diff --git a/src/app/api/v1/batches/delete-completed/route.ts b/src/app/api/v1/batches/delete-completed/route.ts index fbd0f2bbb5..6f6448f92a 100644 --- a/src/app/api/v1/batches/delete-completed/route.ts +++ b/src/app/api/v1/batches/delete-completed/route.ts @@ -1,5 +1,6 @@ import { CORS_HEADERS, handleCorsOptions } from "@/shared/utils/cors"; import { deleteCompletedBatches, type DeleteCompletedBatchesScope } from "@/lib/db/batches"; +import { validateApiKey } from "@/lib/db/apiKeys"; import { NextResponse } from "next/server"; import { getApiKeyRequestScope } from "@/app/api/v1/_helpers/apiKeyScope"; import { buildErrorBody } from "@omniroute/open-sse/utils/error"; @@ -15,18 +16,24 @@ export async function DELETE(request: Request) { const scope = await getApiKeyRequestScope(request); if (scope.rejection) return scope.rejection; - // Fail closed on an unresolvable credential: a presented key that the DB does - // not know (deleted, rotated, mistyped) must never fall through to the session - // branch and widen a destructive sweep to the whole instance. - if (scope.apiKey && !scope.apiKeyId) { - log.warn("BATCHES", "delete-completed: presented API key did not resolve", { + // Fail closed on an unresolvable OR invalid credential. `getApiKeyRequestScope` + // resolves the key by row EXISTENCE (so the list/count siblings can still + // attribute reads); existence is not authorization for a destructive sweep: + // a revoked, deactivated, banned or expired key still has a row and would + // otherwise run the sweep (CWE-613). `validateApiKey` is the one lifecycle + // gate (is_active, revoked_at, is_banned, expires_at) — and neither case may + // fall through to the session branch and widen the sweep to the whole instance. + if (scope.apiKey && (!scope.apiKeyId || !(await validateApiKey(scope.apiKey)))) { + log.warn("BATCHES", "delete-completed: presented API key rejected", { route: LOG_ROUTE, + reason: scope.apiKeyId ? "invalid" : "unresolved", + apiKeyId: scope.apiKeyId, isSessionAuth: scope.isSessionAuth, }); - return NextResponse.json( - { error: { message: "Invalid API key", type: "invalid_request_error" } }, - { status: 401, headers: CORS_HEADERS } - ); + return NextResponse.json(buildErrorBody(401, "Invalid API key"), { + status: 401, + headers: CORS_HEADERS, + }); } // A presented API key always scopes the sweep to that key — even when the @@ -46,10 +53,10 @@ export async function DELETE(request: Request) { sweepScope = { allTenants: true }; mode = "instance"; } else { - return NextResponse.json( - { error: { message: "Authentication required", type: "invalid_request_error" } }, - { status: 401, headers: CORS_HEADERS } - ); + return NextResponse.json(buildErrorBody(401, "Authentication required"), { + status: 401, + headers: CORS_HEADERS, + }); } let result: ReturnType; diff --git a/tests/unit/batches-delete-completed-route-scope.test.ts b/tests/unit/batches-delete-completed-route-scope.test.ts index c66edb90b5..5fdf9c7755 100644 --- a/tests/unit/batches-delete-completed-route-scope.test.ts +++ b/tests/unit/batches-delete-completed-route-scope.test.ts @@ -16,6 +16,9 @@ * session WITHOUT a key sweeps the whole instance; * - a presented key that does not resolve (deleted/rotated/mistyped) is rejected * with 401 even when a session cookie is also present (fail closed); + * - a presented key that resolves but is no longer VALID (revoked, deactivated, + * banned or expired) is rejected with 401 too — existence of the row is not + * authorization (CWE-613); the 401 body is the `buildErrorBody()` shape; * - no credentials at all → 401; * - a sweep that throws → sanitized 500 (no stack trace, no raw SQLite message) * and nothing deleted (the sweep is atomic). @@ -36,7 +39,8 @@ process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "wvxc-route-api-secre process.env.JWT_SECRET = "wvxc-route-jwt-secret"; const { getDbInstance, resetDbInstance } = await import("../../src/lib/db/core.ts"); -const { createApiKey } = await import("../../src/lib/db/apiKeys.ts"); +const { createApiKey, revokeApiKey, updateApiKeyPermissions, setApiKeyExpiry } = + await import("../../src/lib/db/apiKeys.ts"); const { createFile, getFile, getFileContent } = await import("../../src/lib/db/files.ts"); const { createBatch, getBatch } = await import("../../src/lib/db/batches.ts"); const { DELETE } = await import("../../src/app/api/v1/batches/delete-completed/route.ts"); @@ -182,6 +186,70 @@ describe("DELETE /api/v1/batches/delete-completed — caller scope (GHSA-wvxc-jp ); }); + it("rejects a REVOKED key with 401 — the row still exists but is no longer valid — and deletes nothing", async () => { + const keyA = await createApiKey("wvxc-route-revoked-a", "machine-wvxc-ra", []); + const own = seedCompletedBatch(keyA.id, "wvxc-route-revoked-own"); + assert.strictEqual(await revokeApiKey(keyA.id), true); + + const { res, body } = await callDelete({ Authorization: `Bearer ${keyA.key}` }); + + assert.strictEqual(res.status, 401, "a revoked key must not run the sweep"); + assert.strictEqual(body.error?.message, "Invalid API key"); + assert.strictEqual(body.error?.type, "authentication_error"); + assert.strictEqual(body.error?.code, "invalid_api_key"); + assert.ok(getBatch(own.batch.id), "nothing is swept with a revoked key"); + assert.strictEqual( + getFileContent(own.file.id)?.toString(), + "wvxc-route-revoked-own", + "file content is intact with a revoked key" + ); + }); + + it("rejects a DEACTIVATED key (is_active = 0) with 401 and deletes nothing", async () => { + const keyA = await createApiKey("wvxc-route-inactive-a", "machine-wvxc-ia", []); + const own = seedCompletedBatch(keyA.id, "wvxc-route-inactive-own"); + await updateApiKeyPermissions(keyA.id, { isActive: false }); + + const { res, body } = await callDelete({ Authorization: `Bearer ${keyA.key}` }); + + assert.strictEqual(res.status, 401, "a deactivated key must not run the sweep"); + assert.strictEqual(body.error?.message, "Invalid API key"); + assert.ok(getBatch(own.batch.id), "nothing is swept with a deactivated key"); + }); + + it("rejects a BANNED key with 401 and deletes nothing", async () => { + const keyA = await createApiKey("wvxc-route-banned-a", "machine-wvxc-ba2", []); + const own = seedCompletedBatch(keyA.id, "wvxc-route-banned-own"); + await updateApiKeyPermissions(keyA.id, { isBanned: true }); + + const { res, body } = await callDelete({ Authorization: `Bearer ${keyA.key}` }); + + assert.strictEqual(res.status, 401, "a banned key must not run the sweep"); + assert.strictEqual(body.error?.message, "Invalid API key"); + assert.ok(getBatch(own.batch.id), "nothing is swept with a banned key"); + }); + + it("rejects an EXPIRED key with 401 — even alongside a session cookie — and deletes nothing", async () => { + const keyA = await createApiKey("wvxc-route-expired-a", "machine-wvxc-ea", []); + const own = seedCompletedBatch(keyA.id, "wvxc-route-expired-own"); + const unowned = seedCompletedBatch(null, "wvxc-route-expired-unowned"); + await setApiKeyExpiry(keyA.id, new Date(Date.now() - 60_000).toISOString()); + + const { res, body } = await callDelete({ + Authorization: `Bearer ${keyA.key}`, + cookie: await sessionCookie(), + }); + + assert.strictEqual( + res.status, + 401, + "an expired key must fail closed, not fall through to the session" + ); + assert.strictEqual(body.error?.message, "Invalid API key"); + assert.ok(getBatch(own.batch.id), "nothing is swept with an expired key"); + assert.ok(getBatch(unowned.batch.id), "the session branch is never reached"); + }); + it("rejects an unauthenticated request with 401 and deletes nothing", async () => { const keyB = await createApiKey("wvxc-route-401-b", "machine-wvxc-401", []); const seeded = seedCompletedBatch(keyB.id, "wvxc-route-401"); @@ -190,6 +258,8 @@ describe("DELETE /api/v1/batches/delete-completed — caller scope (GHSA-wvxc-jp assert.strictEqual(res.status, 401); assert.strictEqual(body.error?.message, "Authentication required"); + assert.strictEqual(body.error?.type, "authentication_error", "401 body uses buildErrorBody()"); + assert.strictEqual(body.error?.code, "invalid_api_key"); assert.ok(getBatch(seeded.batch.id), "nothing is swept without credentials"); });