mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-21 22:32:22 +03:00
fix(api): scope batch bulk-delete to the calling API key
`DELETE /api/v1/batches/delete-completed` accepted any valid ordinary
inference key — including one with `scopes: []` — and then called
`deleteCompletedBatches()` with no ownership predicate. The helper ran
DELETE FROM batches WHERE status = 'completed'
instance-wide, and passed every referenced file through `deleteFile()`,
which nulls `content`. One tenant could therefore destroy every other
tenant's completed batches and their stored file contents, with no victim
batch id, file id or key id needed (GHSA-wvxc-jp3v-5mg5, CWE-862).
Every sibling operation already keeps this boundary: `listBatches` and
`countBatches` take an optional `apiKeyId` and scope the SQL to
`api_key_id = ?`, falling back to instance-wide only when the caller is an
authenticated dashboard session. `deleteCompletedBatches` was the one
operation that dropped it.
The fix follows that same shape rather than inventing a new one: the helper
takes an optional `apiKeyId` and appends `AND api_key_id = ?` to the file
collection, the checkpoint delete and the batch delete; the route passes
`scope.apiKeyId || undefined`, so a dashboard session keeps the
instance-wide sweep the UI relies on and an API key only ever clears its
own batches.
Regression guard: tests/unit/batches-delete-completed-ownership-wvxc.test.ts
pins all three halves of the contract — a foreign key's batch and file
survive, a non-completed batch is never swept, and the session-wide sweep
still clears every key. The first assertion fails on the pre-fix helper.
This commit is contained in:
@@ -19,7 +19,11 @@ export async function DELETE(request: Request) {
|
||||
);
|
||||
}
|
||||
|
||||
const result = deleteCompletedBatches();
|
||||
// Scope the sweep to the caller's own batches, like the list/count siblings do.
|
||||
// Only a dashboard session (apiKeyId === null) sweeps the whole instance —
|
||||
// otherwise an ordinary inference key would delete every tenant's completed
|
||||
// batches and null out their file contents (GHSA-wvxc-jp3v-5mg5).
|
||||
const result = deleteCompletedBatches(scope.apiKeyId || undefined);
|
||||
|
||||
return NextResponse.json(
|
||||
{ deleted: true, deletedBatches: result.deletedBatches, deletedFiles: result.deletedFiles },
|
||||
|
||||
@@ -411,15 +411,30 @@ export function deleteBatch(id: string): boolean {
|
||||
return result.changes > 0;
|
||||
}
|
||||
|
||||
export function deleteCompletedBatches(): { deletedBatches: number; deletedFiles: number } {
|
||||
/**
|
||||
* Delete completed batches and the files they reference.
|
||||
*
|
||||
* `apiKeyId` scopes the sweep to that key's own batches, exactly like
|
||||
* `listBatches`/`countBatches`. Omitting it sweeps the whole instance and is
|
||||
* reserved for an authenticated dashboard session — an ordinary inference key
|
||||
* that reached this without its own id would otherwise delete every tenant's
|
||||
* completed batches and null out their file contents (GHSA-wvxc-jp3v-5mg5).
|
||||
*/
|
||||
export function deleteCompletedBatches(apiKeyId?: string): {
|
||||
deletedBatches: number;
|
||||
deletedFiles: number;
|
||||
} {
|
||||
const db = getDbInstance();
|
||||
|
||||
// Collect unique file IDs from all completed batches
|
||||
const ownershipClause = apiKeyId ? " AND api_key_id = ?" : "";
|
||||
const ownershipArgs = apiKeyId ? [apiKeyId] : [];
|
||||
|
||||
// Collect unique file IDs from the completed batches in scope
|
||||
const rows = db
|
||||
.prepare(
|
||||
"SELECT input_file_id, output_file_id, error_file_id FROM batches WHERE status = 'completed'"
|
||||
`SELECT input_file_id, output_file_id, error_file_id FROM batches WHERE status = 'completed'${ownershipClause}`
|
||||
)
|
||||
.all() as Array<{
|
||||
.all(...ownershipArgs) as Array<{
|
||||
input_file_id: string | null;
|
||||
output_file_id: string | null;
|
||||
error_file_id: string | null;
|
||||
@@ -442,9 +457,11 @@ export function deleteCompletedBatches(): { deletedBatches: number; deletedFiles
|
||||
}
|
||||
|
||||
db.prepare(
|
||||
"DELETE FROM batch_item_checkpoints WHERE batch_id IN (SELECT id FROM batches WHERE status = 'completed')"
|
||||
).run();
|
||||
`DELETE FROM batch_item_checkpoints WHERE batch_id IN (SELECT id FROM batches WHERE status = 'completed'${ownershipClause})`
|
||||
).run(...ownershipArgs);
|
||||
|
||||
const result = db.prepare("DELETE FROM batches WHERE status = 'completed'").run();
|
||||
const result = db
|
||||
.prepare(`DELETE FROM batches WHERE status = 'completed'${ownershipClause}`)
|
||||
.run(...ownershipArgs);
|
||||
return { deletedBatches: result.changes, deletedFiles };
|
||||
}
|
||||
|
||||
99
tests/unit/batches-delete-completed-ownership-wvxc.test.ts
Normal file
99
tests/unit/batches-delete-completed-ownership-wvxc.test.ts
Normal file
@@ -0,0 +1,99 @@
|
||||
/**
|
||||
* GHSA-wvxc-jp3v-5mg5 — DELETE /api/v1/batches/delete-completed dropped the
|
||||
* ownership predicate that every sibling batch operation keeps.
|
||||
*
|
||||
* `listBatches(apiKeyId?)` / `countBatches(apiKeyId?)` take the caller's key and
|
||||
* scope the SQL to `api_key_id = ?`, falling back to instance-wide only when the
|
||||
* caller is an authenticated dashboard session (which passes `undefined`).
|
||||
* `deleteCompletedBatches()` took no such argument, so any valid inference key —
|
||||
* including one with `scopes: []` — deleted every completed batch on the instance
|
||||
* and nulled the content of the files those batches referenced.
|
||||
*
|
||||
* These tests pin both halves of the contract: scoped deletion for a key, and the
|
||||
* unchanged instance-wide sweep for the dashboard session.
|
||||
*/
|
||||
import { describe, it, after } from "node:test";
|
||||
import assert from "node:assert";
|
||||
import { createFile, getFile } from "@/lib/db/files";
|
||||
import { createBatch, getBatch, deleteCompletedBatches } from "@/lib/db/batches";
|
||||
import { resetDbInstance } from "@/lib/db/core";
|
||||
|
||||
/** One completed batch owned by `apiKeyId`, with its input file. */
|
||||
function seedCompletedBatch(apiKeyId: string | null, label: string) {
|
||||
const file = createFile({
|
||||
bytes: 8,
|
||||
filename: `${label}.jsonl`,
|
||||
purpose: "batch",
|
||||
content: Buffer.from(label),
|
||||
apiKeyId,
|
||||
});
|
||||
const batch = createBatch({
|
||||
endpoint: "/v1/chat/completions",
|
||||
completionWindow: "24h",
|
||||
inputFileId: file.id,
|
||||
status: "completed",
|
||||
apiKeyId,
|
||||
});
|
||||
return { file, batch };
|
||||
}
|
||||
|
||||
describe("deleteCompletedBatches — ownership boundary (GHSA-wvxc-jp3v-5mg5)", () => {
|
||||
after(() => {
|
||||
resetDbInstance();
|
||||
});
|
||||
|
||||
it("deletes only the caller's completed batches, never another key's", () => {
|
||||
const attacker = seedCompletedBatch("key_attacker_wvxc", "wvxc-attacker");
|
||||
const victim = seedCompletedBatch("key_victim_wvxc", "wvxc-victim");
|
||||
|
||||
const result = deleteCompletedBatches("key_attacker_wvxc");
|
||||
|
||||
assert.strictEqual(
|
||||
getBatch(attacker.batch.id),
|
||||
null,
|
||||
"the caller's own completed batch should be deleted"
|
||||
);
|
||||
assert.ok(
|
||||
getBatch(victim.batch.id),
|
||||
"another key's completed batch must survive — this is the vulnerability"
|
||||
);
|
||||
assert.ok(
|
||||
getFile(victim.file.id),
|
||||
"another key's file content must not be cleared by a foreign caller"
|
||||
);
|
||||
assert.strictEqual(result.deletedBatches, 1, "only one batch belonged to the caller");
|
||||
});
|
||||
|
||||
it("leaves a batch that is not completed alone, even when the caller owns it", () => {
|
||||
const own = seedCompletedBatch("key_owner_wvxc", "wvxc-owner");
|
||||
const inProgressFile = createFile({
|
||||
bytes: 8,
|
||||
filename: "wvxc-inprogress.jsonl",
|
||||
purpose: "batch",
|
||||
content: Buffer.from("running"),
|
||||
apiKeyId: "key_owner_wvxc",
|
||||
});
|
||||
const inProgress = createBatch({
|
||||
endpoint: "/v1/chat/completions",
|
||||
completionWindow: "24h",
|
||||
inputFileId: inProgressFile.id,
|
||||
status: "in_progress",
|
||||
apiKeyId: "key_owner_wvxc",
|
||||
});
|
||||
|
||||
deleteCompletedBatches("key_owner_wvxc");
|
||||
|
||||
assert.strictEqual(getBatch(own.batch.id), null, "completed batch of the caller goes");
|
||||
assert.ok(getBatch(inProgress.id), "an in-progress batch is never swept");
|
||||
});
|
||||
|
||||
it("keeps the instance-wide sweep for a dashboard session (no apiKeyId)", () => {
|
||||
const a = seedCompletedBatch("key_a_wvxc_global", "wvxc-global-a");
|
||||
const b = seedCompletedBatch("key_b_wvxc_global", "wvxc-global-b");
|
||||
|
||||
deleteCompletedBatches();
|
||||
|
||||
assert.strictEqual(getBatch(a.batch.id), null, "session sweep clears every key");
|
||||
assert.strictEqual(getBatch(b.batch.id), null, "session sweep clears every key");
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user