diff --git a/src/app/api/v1/batches/delete-completed/route.ts b/src/app/api/v1/batches/delete-completed/route.ts index 0659253e09..9bffc499e6 100644 --- a/src/app/api/v1/batches/delete-completed/route.ts +++ b/src/app/api/v1/batches/delete-completed/route.ts @@ -19,7 +19,10 @@ export async function DELETE(request: Request) { ); } - const result = deleteCompletedBatches(); + // Scope the sweep to the caller. Only the operator's own dashboard (session + // auth) may clear the whole instance; an API key clears only its own + // completed batches (GHSA-wvxc-jp3v-5mg5). + const result = deleteCompletedBatches(scope.isSessionAuth ? undefined : scope.apiKeyId); 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 268f8aa4a9..18cccf3058 100644 --- a/src/lib/db/batches.ts +++ b/src/lib/db/batches.ts @@ -411,15 +411,37 @@ export function deleteBatch(id: string): boolean { return result.changes > 0; } -export function deleteCompletedBatches(): { deletedBatches: number; deletedFiles: number } { +/** + * Bulk-delete completed batches and the files they reference. + * + * `apiKeyId` scopes EVERY statement to that owner. Omitting it keeps the + * instance-wide sweep, which is legitimate for the operator's own dashboard + * (session auth) and for nothing else: without the predicate, an ordinary + * inference key could wipe every tenant's completed batches and null out their + * file contents (GHSA-wvxc-jp3v-5mg5). Same ownership shape as `listBatches` + * and `countBatches` above. + */ +export function deleteCompletedBatches(apiKeyId?: string | null): { + deletedBatches: number; + deletedFiles: number; +} { const db = getDbInstance(); + const scoped = typeof apiKeyId === "string" && apiKeyId.length > 0; - // Collect unique file IDs from all completed batches - const rows = db - .prepare( - "SELECT input_file_id, output_file_id, error_file_id FROM batches WHERE status = 'completed'" - ) - .all() as Array<{ + // Collect unique file IDs from the completed batches in scope + const rows = ( + scoped + ? db + .prepare( + "SELECT input_file_id, output_file_id, error_file_id FROM batches WHERE status = 'completed' AND api_key_id = ?" + ) + .all(apiKeyId) + : db + .prepare( + "SELECT input_file_id, output_file_id, error_file_id FROM batches WHERE status = 'completed'" + ) + .all() + ) as Array<{ input_file_id: string | null; output_file_id: string | null; error_file_id: string | null; @@ -441,6 +463,16 @@ export function deleteCompletedBatches(): { deletedBatches: number; deletedFiles } } + if (scoped) { + db.prepare( + "DELETE FROM batch_item_checkpoints WHERE batch_id IN (SELECT id FROM batches WHERE status = 'completed' AND api_key_id = ?)" + ).run(apiKeyId); + const result = db + .prepare("DELETE FROM batches WHERE status = 'completed' AND api_key_id = ?") + .run(apiKeyId); + return { deletedBatches: result.changes, deletedFiles }; + } + db.prepare( "DELETE FROM batch_item_checkpoints WHERE batch_id IN (SELECT id FROM batches WHERE status = 'completed')" ).run(); diff --git a/tests/unit/batch-delete-completed-ownership-wvxc.test.ts b/tests/unit/batch-delete-completed-ownership-wvxc.test.ts new file mode 100644 index 0000000000..b6685a330b --- /dev/null +++ b/tests/unit/batch-delete-completed-ownership-wvxc.test.ts @@ -0,0 +1,114 @@ +/** + * GHSA-wvxc-jp3v-5mg5 — `DELETE /api/v1/batches/delete-completed` deleted the + * completed batches of EVERY api key on the instance, and nulled the contents of + * every file those batches referenced. + * + * Two defects in one endpoint: + * 1. `deleteCompletedBatches()` carried no `api_key_id` predicate — the file + * SELECT, the checkpoint DELETE and the batch DELETE were all instance-wide. + * 2. The route only checked that SOME key was present (`!scope.apiKeyId` → + * 401), never that the caller owned anything. A key with `scopes: []` — + * an ordinary inference key — could wipe another tenant's batches. + * + * The ownership pattern this restores is not new: `listBatches(apiKeyId?)` and + * `countBatches(apiKeyId?)` in the same module already scope by `api_key_id`, + * and `batches/[id]/route.ts` already gates per-record access with `scopeCheck` + * (session auth sees everything, a key sees only its own). This helper was the + * one that never got it. + * + * Run with: + * node --import tsx/esm --test tests/unit/batch-delete-completed-ownership-wvxc.test.ts + */ + +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { createFile, getFile } from "@/lib/db/files"; +import { createBatch, getBatch, deleteCompletedBatches } from "@/lib/db/batches"; + +const KEY_A = "key-wvxc-aaaa"; +const KEY_B = "key-wvxc-bbbb"; + +function seedCompletedBatch(apiKeyId: string | null, tag: string) { + const file = createFile({ + bytes: 10, + filename: `wvxc-${tag}.jsonl`, + purpose: "batch", + content: Buffer.from("{}"), + }); + const batch = createBatch({ + endpoint: "/v1/chat/completions", + completionWindow: "24h", + inputFileId: file.id, + status: "completed", + apiKeyId, + }); + return { file, batch }; +} + +describe("deleteCompletedBatches — ownership scoping (GHSA-wvxc-jp3v-5mg5)", () => { + it("scoped to one key deletes ONLY that key's completed batches", () => { + const a = seedCompletedBatch(KEY_A, "a1"); + const b = seedCompletedBatch(KEY_B, "b1"); + + const result = deleteCompletedBatches(KEY_A); + + assert.equal(getBatch(a.batch.id), null, "the caller's own batch should be gone"); + assert.ok(getBatch(b.batch.id), "another key's batch must survive"); + assert.equal(result.deletedBatches, 1, "must report only what it actually deleted"); + }); + + it("scoped deletion does not touch another key's file contents", () => { + const a = seedCompletedBatch(KEY_A, "a2"); + const b = seedCompletedBatch(KEY_B, "b2"); + + deleteCompletedBatches(KEY_A); + + assert.equal(getFile(a.file.id), null, "the caller's own file should be gone"); + assert.ok(getFile(b.file.id), "another key's file must survive with its content intact"); + }); + + it("a key with no completed batches deletes nothing at all", () => { + const b = seedCompletedBatch(KEY_B, "b3"); + + const result = deleteCompletedBatches("key-wvxc-with-nothing"); + + assert.equal(result.deletedBatches, 0); + assert.equal(result.deletedFiles, 0); + assert.ok(getBatch(b.batch.id), "an unrelated key's batch must survive"); + }); + + it("unscoped (dashboard session) still clears the whole instance", () => { + // The operator's own dashboard legitimately cleans up everything; that is + // the ONLY caller allowed to omit the key. Preserved deliberately. + seedCompletedBatch(KEY_A, "a4"); + seedCompletedBatch(KEY_B, "b4"); + + const result = deleteCompletedBatches(); + + assert.ok( + result.deletedBatches >= 2, + `expected an instance-wide sweep, got ${result.deletedBatches}` + ); + }); +}); + +describe("the route passes the caller's key through", () => { + it("delete-completed scopes by api key instead of calling the helper bare", async () => { + const { readFileSync } = await import("node:fs"); + const { fileURLToPath } = await import("node:url"); + const src = readFileSync( + fileURLToPath( + new URL("../../src/app/api/v1/batches/delete-completed/route.ts", import.meta.url) + ), + "utf8" + ); + assert.ok( + !/deleteCompletedBatches\(\s*\)/.test(src), + "the route still calls deleteCompletedBatches() with no owner — every tenant's batches go" + ); + assert.ok( + /deleteCompletedBatches\(\s*scope\./.test(src), + "the route must pass the caller's scope into the helper" + ); + }); +});