diff --git a/changelog.d/fixes/12969-batches-delete-completed-ownership.md b/changelog.d/fixes/12969-batches-delete-completed-ownership.md index 77f6015eba..29824bf729 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) +- **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)) diff --git a/src/app/api/v1/batches/delete-completed/route.ts b/src/app/api/v1/batches/delete-completed/route.ts index fc9fac6de0..8340687235 100644 --- a/src/app/api/v1/batches/delete-completed/route.ts +++ b/src/app/api/v1/batches/delete-completed/route.ts @@ -15,24 +15,28 @@ export async function DELETE(request: Request) { const scope = await getApiKeyRequestScope(request); if (scope.rejection) return scope.rejection; - // Only an authenticated dashboard session sweeps the whole instance. Every - // other caller is an inference key and only sweeps its own completed batches, - // like the list/count siblings do — otherwise an ordinary key would delete - // every tenant's completed batches and null out their file contents + // A presented API key always scopes the sweep to that key — even when the + // request also carries a dashboard session cookie — exactly like the + // list/count siblings (`apiKeyId || undefined`), so a leaked or over-shared + // key can never widen a destructive sweep. Only a dashboard session WITHOUT a + // key sweeps the whole instance; otherwise an ordinary key would delete every + // tenant's completed batches and null out their file contents // (GHSA-wvxc-jp3v-5mg5). A caller that is neither gets 401; there is no // fallback that silently widens the sweep. let sweepScope: DeleteCompletedBatchesScope; - if (scope.isSessionAuth) { - sweepScope = { allTenants: true }; - } else if (scope.apiKeyId) { + let mode: "instance" | "api_key"; + if (scope.apiKeyId) { sweepScope = { apiKeyId: scope.apiKeyId }; + mode = "api_key"; + } else if (scope.isSessionAuth) { + sweepScope = { allTenants: true }; + mode = "instance"; } else { return NextResponse.json( { error: { message: "Authentication required", type: "invalid_request_error" } }, { status: 401, headers: CORS_HEADERS } ); } - const mode: "instance" | "api_key" = scope.isSessionAuth ? "instance" : "api_key"; let result: ReturnType; try { @@ -42,7 +46,7 @@ export async function DELETE(request: Request) { route: LOG_ROUTE, mode, apiKeyId: scope.apiKeyId, - error: err instanceof Error ? err.message : String(err), + error: err instanceof Error ? { message: err.message, stack: err.stack } : String(err), }); return NextResponse.json(buildErrorBody(500, "Failed to delete completed batches"), { status: 500, @@ -57,11 +61,13 @@ export async function DELETE(request: Request) { deletedBatches: result.deletedBatches, deletedFiles: result.deletedFiles, }; - if (mode === "instance") { - log.warn("BATCHES", "instance-wide completed-batch sweep", audit); - } else { - log.info("BATCHES", "completed-batch sweep", audit); - } + // A bulk delete is an audit event, not routine chatter: both modes log at + // warn so the trail survives APP_LOG_LEVEL=warn. + log.warn( + "BATCHES", + mode === "instance" ? "instance-wide completed-batch sweep" : "completed-batch sweep", + audit + ); return NextResponse.json( { deleted: true, deletedBatches: result.deletedBatches, deletedFiles: result.deletedFiles }, diff --git a/src/lib/db/batches.ts b/src/lib/db/batches.ts index 3f0aedbe64..b0e3225a10 100644 --- a/src/lib/db/batches.ts +++ b/src/lib/db/batches.ts @@ -452,6 +452,9 @@ export function deleteCompletedBatches(scope: DeleteCompletedBatchesScope): { if (!allTenants && !apiKeyId) { throw new Error("deleteCompletedBatches: apiKeyId required unless allTenants"); } + if (allTenants && apiKeyId) { + throw new Error("deleteCompletedBatches: apiKeyId and allTenants are mutually exclusive"); + } const db = getDbInstance(); diff --git a/tests/unit/batches-delete-completed-ownership-wvxc.test.ts b/tests/unit/batches-delete-completed-ownership-wvxc.test.ts index 7fdfa7a857..3a5e4d46a6 100644 --- a/tests/unit/batches-delete-completed-ownership-wvxc.test.ts +++ b/tests/unit/batches-delete-completed-ownership-wvxc.test.ts @@ -139,6 +139,15 @@ describe("deleteCompletedBatches — ownership boundary (GHSA-wvxc-jp3v-5mg5)", }), /apiKeyId required unless allTenants/ ); + assert.throws( + // A mixed scope must be rejected, never silently widened to the instance. + () => + (deleteCompletedBatches as unknown as (s: unknown) => unknown)({ + apiKeyId: "key_survivor_wvxc", + allTenants: true, + }), + /mutually exclusive/ + ); assert.ok(getBatch(survivor.batch.id), "a rejected call must not delete anything"); assert.strictEqual( diff --git a/tests/unit/batches-delete-completed-route-scope.test.ts b/tests/unit/batches-delete-completed-route-scope.test.ts index 6f7d4620c1..c4345fad88 100644 --- a/tests/unit/batches-delete-completed-route-scope.test.ts +++ b/tests/unit/batches-delete-completed-route-scope.test.ts @@ -112,16 +112,12 @@ describe("DELETE /api/v1/batches/delete-completed — caller scope (GHSA-wvxc-jp assert.ok(getBatch(victim.batch.id), "key B's batch still survives"); }); - it("a dashboard session sweeps the whole instance — even when the request also carries an API key", async () => { - const keyA = await createApiKey("wvxc-route-session-a", "machine-wvxc-sa", []); + it("a dashboard session WITHOUT a key sweeps the whole instance", async () => { const keyB = await createApiKey("wvxc-route-session-b", "machine-wvxc-sb", []); const other = seedCompletedBatch(keyB.id, "wvxc-route-session-other"); const unowned = seedCompletedBatch(null, "wvxc-route-session-unowned"); - const { res, body } = await callDelete({ - Authorization: `Bearer ${keyA.key}`, - cookie: await sessionCookie(), - }); + const { res, body } = await callDelete({ cookie: await sessionCookie() }); assert.strictEqual(res.status, 200); assert.ok( @@ -133,6 +129,33 @@ describe("DELETE /api/v1/batches/delete-completed — caller scope (GHSA-wvxc-jp assert.strictEqual(getFile(other.file.id), null, "session sweep soft-deletes the files too"); }); + it("a request carrying BOTH a session cookie and an API key is scoped to the key (the key wins, like GET /v1/batches)", async () => { + const keyA = await createApiKey("wvxc-route-both-a", "machine-wvxc-ba", []); + const keyB = await createApiKey("wvxc-route-both-b", "machine-wvxc-bb", []); + const own = seedCompletedBatch(keyA.id, "wvxc-route-both-own"); + const other = seedCompletedBatch(keyB.id, "wvxc-route-both-other"); + const unowned = seedCompletedBatch(null, "wvxc-route-both-unowned"); + + const { res, body } = await callDelete({ + Authorization: `Bearer ${keyA.key}`, + cookie: await sessionCookie(), + }); + + assert.strictEqual(res.status, 200); + assert.strictEqual(body.deletedBatches, 1, "only key A's own completed batch is swept"); + assert.strictEqual(getBatch(own.batch.id), null, "key A's own batch is swept"); + assert.ok( + getBatch(other.batch.id), + "key B's batch survives — a presented key never widens the sweep" + ); + assert.ok(getBatch(unowned.batch.id), "the unowned batch survives a key-scoped sweep"); + assert.strictEqual( + getFileContent(other.file.id)?.toString(), + "wvxc-route-both-other", + "key B's file content is intact" + ); + }); + 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");