mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-13 18:32:12 +03:00
fix(api): explicit sweep scope, audit log and atomic delete for completed batches
Follow-up to #12969 (GHSA-wvxc-jp3v-5mg5) from the omni-code-review battery (LEDGER-1/2/3/4/7/8/9/10): - deleteCompletedBatches takes an explicit scope `{ apiKeyId } | { allTenants: true }`; a missing/empty id throws instead of silently sweeping the whole instance - route branches on the dashboard session explicitly; keys only sweep their own batches; batches with no owner stay out of a key-scoped sweep on purpose (JSDoc) - every sweep is logged (warn for instance-wide, info for key-scoped); a failing sweep returns a sanitized 500 via buildErrorBody - the file soft-deletes, checkpoint delete and batch delete run in one transaction; the empty catch around deleteFile now logs the failure - route-level regression test through the real handler; test files self-isolate their DATA_DIR so the single-file command never touches ~/.omniroute - changelog.d fragment
This commit is contained in:
diegosouzapw
@@ -0,0 +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)
|
||||
@@ -1,7 +1,11 @@
|
||||
import { CORS_HEADERS, handleCorsOptions } from "@/shared/utils/cors";
|
||||
import { deleteCompletedBatches } from "@/lib/db/batches";
|
||||
import { deleteCompletedBatches, type DeleteCompletedBatchesScope } from "@/lib/db/batches";
|
||||
import { NextResponse } from "next/server";
|
||||
import { getApiKeyRequestScope } from "@/app/api/v1/_helpers/apiKeyScope";
|
||||
import { buildErrorBody } from "@omniroute/open-sse/utils/error";
|
||||
import * as log from "@/sse/utils/logger";
|
||||
|
||||
const LOG_ROUTE = "batches/delete-completed";
|
||||
|
||||
export async function OPTIONS() {
|
||||
return handleCorsOptions();
|
||||
@@ -11,19 +15,53 @@ export async function DELETE(request: Request) {
|
||||
const scope = await getApiKeyRequestScope(request);
|
||||
if (scope.rejection) return scope.rejection;
|
||||
|
||||
// Allow session-authenticated (dashboard) requests; for API-key requests, require a key
|
||||
if (!scope.isSessionAuth && !scope.apiKeyId) {
|
||||
// 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
|
||||
// (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) {
|
||||
sweepScope = { apiKeyId: scope.apiKeyId };
|
||||
} 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";
|
||||
|
||||
// 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);
|
||||
let result: ReturnType<typeof deleteCompletedBatches>;
|
||||
try {
|
||||
result = deleteCompletedBatches(sweepScope);
|
||||
} catch (err) {
|
||||
log.error("BATCHES", "delete-completed sweep failed", {
|
||||
route: LOG_ROUTE,
|
||||
mode,
|
||||
apiKeyId: scope.apiKeyId,
|
||||
error: err instanceof Error ? err.message : String(err),
|
||||
});
|
||||
return NextResponse.json(buildErrorBody(500, "Failed to delete completed batches"), {
|
||||
status: 500,
|
||||
headers: CORS_HEADERS,
|
||||
});
|
||||
}
|
||||
|
||||
const audit = {
|
||||
route: LOG_ROUTE,
|
||||
mode,
|
||||
apiKeyId: scope.apiKeyId,
|
||||
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);
|
||||
}
|
||||
|
||||
return NextResponse.json(
|
||||
{ deleted: true, deletedBatches: result.deletedBatches, deletedFiles: result.deletedFiles },
|
||||
|
||||
@@ -1,6 +1,9 @@
|
||||
import { getDbInstance, rowToCamel, objToSnake } from "./core";
|
||||
import { deleteFile } from "./files";
|
||||
import { v4 as uuidv4 } from "uuid";
|
||||
import { logger } from "../../../open-sse/utils/logger.ts";
|
||||
|
||||
const log = logger("DB_BATCHES");
|
||||
|
||||
function parseBatchRow(row: any): BatchRecord {
|
||||
const camel = rowToCamel(row) as any;
|
||||
@@ -411,57 +414,90 @@ export function deleteBatch(id: string): boolean {
|
||||
return result.changes > 0;
|
||||
}
|
||||
|
||||
/**
|
||||
* Scope of a `deleteCompletedBatches` sweep. The intent is explicit on purpose:
|
||||
* a caller either names the API key whose batches it may sweep, or states
|
||||
* `allTenants: true` — there is no default that widens to the whole instance.
|
||||
*/
|
||||
export type DeleteCompletedBatchesScope = { apiKeyId: string } | { allTenants: true };
|
||||
|
||||
/**
|
||||
* 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).
|
||||
* `{ apiKeyId }` scopes the sweep to that key's own batches, exactly like
|
||||
* `listBatches`/`countBatches`. `{ allTenants: true }` 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). A
|
||||
* missing/empty `apiKeyId` without `allTenants` throws instead of silently
|
||||
* widening the sweep.
|
||||
*
|
||||
* Batches whose `api_key_id` IS NULL are intentionally OUT of a key-scoped sweep.
|
||||
* This diverges from `scopeCheck` in `src/app/api/v1/batches/[id]/route.ts`,
|
||||
* which lets any key read/delete a single unowned batch by id: a bulk destructive
|
||||
* sweep must never reach records the key does not own, so unowned batches are
|
||||
* only swept by `{ allTenants: true }`.
|
||||
*
|
||||
* The file soft-deletes, the checkpoint DELETE and the batches DELETE run in one
|
||||
* transaction, so a mid-sweep failure rolls everything back — no batch row is
|
||||
* left pointing at a file whose content was already nulled.
|
||||
*/
|
||||
export function deleteCompletedBatches(apiKeyId?: string): {
|
||||
export function deleteCompletedBatches(scope: DeleteCompletedBatchesScope): {
|
||||
deletedBatches: number;
|
||||
deletedFiles: number;
|
||||
} {
|
||||
const scopeObj = scope && typeof scope === "object" ? scope : {};
|
||||
const allTenants = "allTenants" in scopeObj && scopeObj.allTenants === true;
|
||||
const apiKeyId = "apiKeyId" in scopeObj ? scopeObj.apiKeyId : undefined;
|
||||
if (!allTenants && !apiKeyId) {
|
||||
throw new Error("deleteCompletedBatches: apiKeyId required unless allTenants");
|
||||
}
|
||||
|
||||
const db = getDbInstance();
|
||||
|
||||
const ownershipClause = apiKeyId ? " AND api_key_id = ?" : "";
|
||||
const ownershipArgs = apiKeyId ? [apiKeyId] : [];
|
||||
const ownershipClause = allTenants ? "" : " AND api_key_id = ?";
|
||||
const ownershipArgs = allTenants ? [] : [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'${ownershipClause}`
|
||||
)
|
||||
.all(...ownershipArgs) as Array<{
|
||||
input_file_id: string | null;
|
||||
output_file_id: string | null;
|
||||
error_file_id: string | null;
|
||||
}>;
|
||||
const sweep = db.transaction(() => {
|
||||
// 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'${ownershipClause}`
|
||||
)
|
||||
.all(...ownershipArgs) as Array<{
|
||||
input_file_id: string | null;
|
||||
output_file_id: string | null;
|
||||
error_file_id: string | null;
|
||||
}>;
|
||||
|
||||
const fileIds = new Set<string>();
|
||||
for (const row of rows) {
|
||||
if (row.input_file_id) fileIds.add(row.input_file_id);
|
||||
if (row.output_file_id) fileIds.add(row.output_file_id);
|
||||
if (row.error_file_id) fileIds.add(row.error_file_id);
|
||||
}
|
||||
|
||||
let deletedFiles = 0;
|
||||
for (const fid of fileIds) {
|
||||
try {
|
||||
if (deleteFile(fid)) deletedFiles++;
|
||||
} catch {
|
||||
/* ignore */
|
||||
const fileIds = new Set<string>();
|
||||
for (const row of rows) {
|
||||
if (row.input_file_id) fileIds.add(row.input_file_id);
|
||||
if (row.output_file_id) fileIds.add(row.output_file_id);
|
||||
if (row.error_file_id) fileIds.add(row.error_file_id);
|
||||
}
|
||||
}
|
||||
|
||||
db.prepare(
|
||||
`DELETE FROM batch_item_checkpoints WHERE batch_id IN (SELECT id FROM batches WHERE status = 'completed'${ownershipClause})`
|
||||
).run(...ownershipArgs);
|
||||
let deletedFiles = 0;
|
||||
for (const fid of fileIds) {
|
||||
try {
|
||||
if (deleteFile(fid)) deletedFiles++;
|
||||
} catch (err) {
|
||||
log.warn("deleteCompletedBatches: file soft-delete failed", {
|
||||
fid,
|
||||
err: err instanceof Error ? err.message : String(err),
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
const result = db
|
||||
.prepare(`DELETE FROM batches WHERE status = 'completed'${ownershipClause}`)
|
||||
.run(...ownershipArgs);
|
||||
return { deletedBatches: result.changes, deletedFiles };
|
||||
db.prepare(
|
||||
`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'${ownershipClause}`)
|
||||
.run(...ownershipArgs);
|
||||
return { deletedBatches: result.changes, deletedFiles };
|
||||
});
|
||||
|
||||
return sweep();
|
||||
}
|
||||
|
||||
@@ -180,7 +180,7 @@ describe("deleteCompletedBatches", () => {
|
||||
assert.ok(getFile(liveInput.id));
|
||||
|
||||
// Delete all completed (may include pre-existing ones from other tests)
|
||||
const result = deleteCompletedBatches();
|
||||
const result = deleteCompletedBatches({ allTenants: true });
|
||||
assert.ok(result.deletedBatches >= 3, `expected >=3, got ${result.deletedBatches}`);
|
||||
assert.ok(result.deletedFiles >= 3, `expected >=3, got ${result.deletedFiles}`);
|
||||
|
||||
@@ -194,7 +194,7 @@ describe("deleteCompletedBatches", () => {
|
||||
});
|
||||
|
||||
it("should return zero counts when no completed batches exist", () => {
|
||||
const result = deleteCompletedBatches();
|
||||
const result = deleteCompletedBatches({ allTenants: true });
|
||||
assert.strictEqual(result.deletedBatches, 0);
|
||||
assert.strictEqual(result.deletedFiles, 0);
|
||||
});
|
||||
@@ -224,7 +224,7 @@ describe("deleteCompletedBatches", () => {
|
||||
assert.ok(getBatch(batchB.id));
|
||||
assert.ok(getFile(sharedFile.id));
|
||||
|
||||
const result = deleteCompletedBatches();
|
||||
const result = deleteCompletedBatches({ allTenants: true });
|
||||
assert.ok(result.deletedBatches >= 2);
|
||||
assert.ok(result.deletedFiles >= 1, "shared file should be counted once");
|
||||
|
||||
|
||||
@@ -4,19 +4,39 @@
|
||||
*
|
||||
* `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.
|
||||
* caller is an authenticated dashboard session. `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.
|
||||
* These tests pin the DB-level contract:
|
||||
* - `{ apiKeyId }` sweeps only that key's completed batches;
|
||||
* - `{ allTenants: true }` is the ONLY way to sweep the whole instance — an
|
||||
* empty/missing apiKeyId throws instead of silently widening the sweep;
|
||||
* - batches with `api_key_id IS NULL` are deliberately OUT of a key-scoped
|
||||
* sweep (strict — diverges from the single-batch `scopeCheck` on purpose);
|
||||
* - the sweep is atomic: a failure after the file soft-deletes rolls the file
|
||||
* content back, so no batch row is left pointing at a nulled file;
|
||||
* - a file soft-delete failure is logged, not swallowed, and the batch rows
|
||||
* are still swept.
|
||||
*
|
||||
* Self-isolating: DATA_DIR is pointed at a fresh temp dir BEFORE any `@/lib/db/*`
|
||||
* module loads (dynamic imports below), so this file never touches ~/.omniroute
|
||||
* even when run without tests/_setup/isolateDataDir.ts.
|
||||
*/
|
||||
import { describe, it, after } from "node:test";
|
||||
import { describe, it, after, mock } 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";
|
||||
import fs from "node:fs";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
|
||||
const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "wvxc-"));
|
||||
process.env.DATA_DIR = TEST_DATA_DIR;
|
||||
|
||||
const { createFile, getFile, getFileContent } = await import("../../src/lib/db/files.ts");
|
||||
const { createBatch, getBatch, deleteCompletedBatches } =
|
||||
await import("../../src/lib/db/batches.ts");
|
||||
const { getDbInstance, resetDbInstance } = await import("../../src/lib/db/core.ts");
|
||||
|
||||
/** One completed batch owned by `apiKeyId`, with its input file. */
|
||||
function seedCompletedBatch(apiKeyId: string | null, label: string) {
|
||||
@@ -40,13 +60,14 @@ function seedCompletedBatch(apiKeyId: string | null, label: string) {
|
||||
describe("deleteCompletedBatches — ownership boundary (GHSA-wvxc-jp3v-5mg5)", () => {
|
||||
after(() => {
|
||||
resetDbInstance();
|
||||
fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 });
|
||||
});
|
||||
|
||||
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");
|
||||
const result = deleteCompletedBatches({ apiKeyId: "key_attacker_wvxc" });
|
||||
|
||||
assert.strictEqual(
|
||||
getBatch(attacker.batch.id),
|
||||
@@ -81,19 +102,138 @@ describe("deleteCompletedBatches — ownership boundary (GHSA-wvxc-jp3v-5mg5)",
|
||||
apiKeyId: "key_owner_wvxc",
|
||||
});
|
||||
|
||||
deleteCompletedBatches("key_owner_wvxc");
|
||||
deleteCompletedBatches({ apiKeyId: "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)", () => {
|
||||
it("keeps the instance-wide sweep only for the explicit allTenants scope", () => {
|
||||
const a = seedCompletedBatch("key_a_wvxc_global", "wvxc-global-a");
|
||||
const b = seedCompletedBatch("key_b_wvxc_global", "wvxc-global-b");
|
||||
|
||||
deleteCompletedBatches();
|
||||
deleteCompletedBatches({ allTenants: true });
|
||||
|
||||
assert.strictEqual(getBatch(a.batch.id), null, "session sweep clears every key");
|
||||
assert.strictEqual(getBatch(b.batch.id), null, "session sweep clears every key");
|
||||
});
|
||||
|
||||
it("throws on a missing/empty apiKeyId instead of silently sweeping the instance", () => {
|
||||
const survivor = seedCompletedBatch("key_survivor_wvxc", "wvxc-survivor");
|
||||
|
||||
assert.throws(
|
||||
() => deleteCompletedBatches({ apiKeyId: "" }),
|
||||
/apiKeyId required unless allTenants/
|
||||
);
|
||||
assert.throws(
|
||||
// A caller that forgot the scope entirely (JS caller / `any` cast) must not
|
||||
// fall through to an instance-wide sweep either.
|
||||
() => (deleteCompletedBatches as unknown as (s?: unknown) => unknown)(),
|
||||
/apiKeyId required unless allTenants/
|
||||
);
|
||||
assert.throws(
|
||||
() =>
|
||||
(deleteCompletedBatches as unknown as (s: unknown) => unknown)({
|
||||
apiKeyId: null,
|
||||
allTenants: false,
|
||||
}),
|
||||
/apiKeyId required unless allTenants/
|
||||
);
|
||||
|
||||
assert.ok(getBatch(survivor.batch.id), "a rejected call must not delete anything");
|
||||
assert.strictEqual(
|
||||
getFileContent(survivor.file.id)?.toString(),
|
||||
"wvxc-survivor",
|
||||
"a rejected call must not null file content"
|
||||
);
|
||||
|
||||
deleteCompletedBatches({ apiKeyId: "key_survivor_wvxc" });
|
||||
});
|
||||
|
||||
it("STRICT: a batch with api_key_id NULL stays out of a key-scoped sweep (diverges from scopeCheck on purpose)", () => {
|
||||
const unowned = seedCompletedBatch(null, "wvxc-unowned");
|
||||
const own = seedCompletedBatch("key_strict_wvxc", "wvxc-strict-own");
|
||||
|
||||
const scoped = deleteCompletedBatches({ apiKeyId: "key_strict_wvxc" });
|
||||
|
||||
assert.strictEqual(scoped.deletedBatches, 1, "only the key's own batch is counted");
|
||||
assert.strictEqual(getBatch(own.batch.id), null, "the key's own batch goes");
|
||||
assert.ok(getBatch(unowned.batch.id), "the NULL-owned batch survives a key-scoped sweep");
|
||||
assert.ok(getFile(unowned.file.id), "the NULL-owned batch's file survives");
|
||||
assert.strictEqual(
|
||||
getFileContent(unowned.file.id)?.toString(),
|
||||
"wvxc-unowned",
|
||||
"the NULL-owned batch's file content is intact"
|
||||
);
|
||||
|
||||
const instanceWide = deleteCompletedBatches({ allTenants: true });
|
||||
|
||||
assert.ok(instanceWide.deletedBatches >= 1, "the allTenants sweep reaches unowned batches");
|
||||
assert.strictEqual(getBatch(unowned.batch.id), null, "allTenants removes the NULL-owned batch");
|
||||
assert.strictEqual(getFile(unowned.file.id), null, "allTenants soft-deletes its file too");
|
||||
});
|
||||
|
||||
it("ATOMIC: a failure after the file soft-deletes rolls the file content back", () => {
|
||||
const db = getDbInstance();
|
||||
const own = seedCompletedBatch("key_atomic_wvxc", "wvxc-atomic");
|
||||
|
||||
// Test-side failure injection: make the final `DELETE FROM batches` abort. The
|
||||
// file soft-deletes and the checkpoint DELETE run before it, so without a
|
||||
// transaction the batch row would survive pointing at a nulled file.
|
||||
db.exec(
|
||||
"CREATE TRIGGER wvxc_abort_batch_delete BEFORE DELETE ON batches BEGIN SELECT RAISE(ABORT, 'wvxc injected failure'); END"
|
||||
);
|
||||
try {
|
||||
assert.throws(
|
||||
() => deleteCompletedBatches({ apiKeyId: "key_atomic_wvxc" }),
|
||||
/wvxc injected failure/
|
||||
);
|
||||
} finally {
|
||||
db.exec("DROP TRIGGER IF EXISTS wvxc_abort_batch_delete");
|
||||
}
|
||||
|
||||
const batch = getBatch(own.batch.id);
|
||||
assert.ok(batch, "the batch row is still there after the failed sweep");
|
||||
assert.ok(
|
||||
getFile(own.file.id),
|
||||
"the referenced file is not soft-deleted after the failed sweep"
|
||||
);
|
||||
assert.strictEqual(
|
||||
getFileContent(own.file.id)?.toString(),
|
||||
"wvxc-atomic",
|
||||
"the file content was rolled back — no batch row points at a nulled file"
|
||||
);
|
||||
|
||||
// Sanity: without the injected failure the sweep completes normally.
|
||||
const result = deleteCompletedBatches({ apiKeyId: "key_atomic_wvxc" });
|
||||
assert.strictEqual(result.deletedBatches, 1);
|
||||
assert.strictEqual(result.deletedFiles, 1);
|
||||
assert.strictEqual(getFile(own.file.id), null);
|
||||
});
|
||||
|
||||
it("logs (does not swallow) a file soft-delete failure and still sweeps the batch rows", () => {
|
||||
const db = getDbInstance();
|
||||
const own = seedCompletedBatch("key_filefail_wvxc", "wvxc-filefail");
|
||||
const warn = mock.method(console, "warn", () => {});
|
||||
|
||||
db.exec(
|
||||
"CREATE TRIGGER wvxc_abort_file_update BEFORE UPDATE ON files BEGIN SELECT RAISE(ABORT, 'wvxc file failure'); END"
|
||||
);
|
||||
let result: { deletedBatches: number; deletedFiles: number };
|
||||
try {
|
||||
result = deleteCompletedBatches({ apiKeyId: "key_filefail_wvxc" });
|
||||
} finally {
|
||||
db.exec("DROP TRIGGER IF EXISTS wvxc_abort_file_update");
|
||||
warn.mock.restore();
|
||||
}
|
||||
|
||||
assert.strictEqual(result.deletedBatches, 1, "batch rows are swept even when a file fails");
|
||||
assert.strictEqual(result.deletedFiles, 0, "a failed soft-delete is not counted");
|
||||
assert.strictEqual(getBatch(own.batch.id), null);
|
||||
const logged = warn.mock.calls
|
||||
.map((c) => c.arguments.map((a) => String(a)).join(" "))
|
||||
.find((line) => line.includes("deleteCompletedBatches: file soft-delete failed"));
|
||||
assert.ok(logged, "the failure is logged at warn level");
|
||||
assert.ok(logged!.includes(own.file.id), "the log line names the file id");
|
||||
});
|
||||
});
|
||||
|
||||
175
tests/unit/batches-delete-completed-route-scope.test.ts
Normal file
175
tests/unit/batches-delete-completed-route-scope.test.ts
Normal file
@@ -0,0 +1,175 @@
|
||||
/**
|
||||
* GHSA-wvxc-jp3v-5mg5 — route-level regression guard for
|
||||
* `DELETE /api/v1/batches/delete-completed`.
|
||||
*
|
||||
* The DB-level contract lives in batches-delete-completed-ownership-wvxc.test.ts.
|
||||
* This file drives the REAL route handler with REAL credentials (API keys created
|
||||
* through `createApiKey`, a dashboard session via a signed `auth_token` cookie) so
|
||||
* it fails whenever the route stops translating the caller's scope into the
|
||||
* matching `deleteCompletedBatches` scope:
|
||||
*
|
||||
* - an inference key must only sweep its own completed batches and the response
|
||||
* must report 0 deletions when it owns none — proven against an un-scoped
|
||||
* `{ allTenants: true }` regression (the "flip" recorded in the PR notes);
|
||||
* - an authenticated dashboard session sweeps the whole instance, even when the
|
||||
* request ALSO carries an API key (isSessionAuth wins);
|
||||
* - 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).
|
||||
*
|
||||
* Self-isolating: DATA_DIR points at a fresh temp dir BEFORE any `@/lib/db/*`
|
||||
* module loads (dynamic imports below), so this file never touches ~/.omniroute.
|
||||
*/
|
||||
import { describe, it, after } from "node:test";
|
||||
import assert from "node:assert";
|
||||
import fs from "node:fs";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { SignJWT } from "jose";
|
||||
|
||||
const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "wvxc-route-"));
|
||||
process.env.DATA_DIR = TEST_DATA_DIR;
|
||||
process.env.API_KEY_SECRET = process.env.API_KEY_SECRET || "wvxc-route-api-secret";
|
||||
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 { 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");
|
||||
|
||||
const ROUTE_URL = "http://localhost/api/v1/batches/delete-completed";
|
||||
|
||||
async function sessionCookie(): Promise<string> {
|
||||
const secret = new TextEncoder().encode(process.env.JWT_SECRET);
|
||||
const jwt = await new SignJWT({ sub: "admin" })
|
||||
.setProtectedHeader({ alg: "HS256" })
|
||||
.setExpirationTime("1h")
|
||||
.sign(secret);
|
||||
return `auth_token=${jwt}`;
|
||||
}
|
||||
|
||||
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 };
|
||||
}
|
||||
|
||||
async function callDelete(headers: Record<string, string>) {
|
||||
const res = await DELETE(new Request(ROUTE_URL, { method: "DELETE", headers }));
|
||||
const body = (await res.json()) as {
|
||||
deleted?: boolean;
|
||||
deletedBatches?: number;
|
||||
deletedFiles?: number;
|
||||
error?: { message: string; type?: string; code?: string };
|
||||
};
|
||||
return { res, body };
|
||||
}
|
||||
|
||||
describe("DELETE /api/v1/batches/delete-completed — caller scope (GHSA-wvxc-jp3v-5mg5)", () => {
|
||||
after(() => {
|
||||
resetDbInstance();
|
||||
fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 });
|
||||
});
|
||||
|
||||
it("an inference key sweeps only its own batches: another key's batch survives and the response reports 0", async () => {
|
||||
const keyA = await createApiKey("wvxc-route-key-a", "machine-wvxc-a", []);
|
||||
const keyB = await createApiKey("wvxc-route-key-b", "machine-wvxc-b", []);
|
||||
const victim = seedCompletedBatch(keyB.id, "wvxc-route-victim");
|
||||
|
||||
const { res, body } = await callDelete({ Authorization: `Bearer ${keyA.key}` });
|
||||
|
||||
assert.strictEqual(res.status, 200);
|
||||
assert.strictEqual(body.deleted, true);
|
||||
assert.strictEqual(body.deletedBatches, 0, "key A owns no completed batch — nothing to sweep");
|
||||
assert.strictEqual(body.deletedFiles, 0);
|
||||
assert.ok(getBatch(victim.batch.id), "key B's completed batch must survive key A's sweep");
|
||||
assert.strictEqual(
|
||||
getFileContent(victim.file.id)?.toString(),
|
||||
"wvxc-route-victim",
|
||||
"key B's file content must not be nulled by key A"
|
||||
);
|
||||
|
||||
// And key A's own batch IS swept by key A.
|
||||
const own = seedCompletedBatch(keyA.id, "wvxc-route-own");
|
||||
const second = await callDelete({ Authorization: `Bearer ${keyA.key}` });
|
||||
assert.strictEqual(second.res.status, 200);
|
||||
assert.strictEqual(second.body.deletedBatches, 1);
|
||||
assert.strictEqual(second.body.deletedFiles, 1);
|
||||
assert.strictEqual(getBatch(own.batch.id), null, "key A's own completed batch is swept");
|
||||
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", []);
|
||||
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(),
|
||||
});
|
||||
|
||||
assert.strictEqual(res.status, 200);
|
||||
assert.ok(
|
||||
body.deletedBatches! >= 2,
|
||||
`session sweep must reach every tenant, got ${body.deletedBatches}`
|
||||
);
|
||||
assert.strictEqual(getBatch(other.batch.id), null, "session sweep removes another key's batch");
|
||||
assert.strictEqual(getBatch(unowned.batch.id), null, "session sweep removes the unowned batch");
|
||||
assert.strictEqual(getFile(other.file.id), null, "session sweep soft-deletes the files too");
|
||||
});
|
||||
|
||||
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");
|
||||
|
||||
const { res, body } = await callDelete({});
|
||||
|
||||
assert.strictEqual(res.status, 401);
|
||||
assert.strictEqual(body.error?.message, "Authentication required");
|
||||
assert.ok(getBatch(seeded.batch.id), "nothing is swept without credentials");
|
||||
});
|
||||
|
||||
it("returns a sanitized 500 (no stack trace, no raw SQLite message) when the sweep throws, and deletes nothing", async () => {
|
||||
const keyA = await createApiKey("wvxc-route-500-a", "machine-wvxc-500", []);
|
||||
const own = seedCompletedBatch(keyA.id, "wvxc-route-500");
|
||||
const db = getDbInstance();
|
||||
|
||||
db.exec(
|
||||
"CREATE TRIGGER wvxc_route_abort_batch_delete BEFORE DELETE ON batches BEGIN SELECT RAISE(ABORT, 'wvxc route injected failure at /secret/path.ts:1'); END"
|
||||
);
|
||||
let outcome: Awaited<ReturnType<typeof callDelete>>;
|
||||
try {
|
||||
outcome = await callDelete({ Authorization: `Bearer ${keyA.key}` });
|
||||
} finally {
|
||||
db.exec("DROP TRIGGER IF EXISTS wvxc_route_abort_batch_delete");
|
||||
}
|
||||
|
||||
assert.strictEqual(outcome.res.status, 500);
|
||||
const message = outcome.body.error?.message ?? "";
|
||||
assert.ok(message.length > 0, "a 500 still carries a human-readable message");
|
||||
assert.ok(!message.includes("at /"), `stack trace leaked: ${message}`);
|
||||
assert.ok(!message.includes("wvxc route injected failure"), `raw DB error leaked: ${message}`);
|
||||
assert.ok(!message.includes("SQLITE"), `raw SQLite code leaked: ${message}`);
|
||||
assert.ok(getBatch(own.batch.id), "a failed sweep leaves the batch row in place");
|
||||
assert.strictEqual(
|
||||
getFileContent(own.file.id)?.toString(),
|
||||
"wvxc-route-500",
|
||||
"a failed sweep rolls the file content back"
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user