mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-13 18:32:12 +03:00
fix(api): key-first sweep scope, audit at warn, mixed-scope guard for completed batches
Round-2 findings of the omni-code-review battery on the previous commit: - a presented API key always scopes the sweep to that key, even when the request also carries a dashboard session cookie (parity with GET /v1/batches; a leaked or over-shared key can never widen a destructive sweep); only a session without a key sweeps the whole instance - both sweep modes log at warn so the audit trail survives APP_LOG_LEVEL=warn; the failure log carries the error stack - a scope carrying both apiKeyId and allTenants is rejected instead of widening - changelog fragment links the PR
This commit is contained in:
@@ -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))
|
||||
|
||||
@@ -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<typeof deleteCompletedBatches>;
|
||||
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 },
|
||||
|
||||
@@ -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();
|
||||
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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");
|
||||
|
||||
Reference in New Issue
Block a user