fix(api): scope batch bulk-delete to the calling API key

changelog.d/fixes/12969-batches-delete-completed-ownership.md

@@ -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, a 401 for a presented key that is unknown, revoked, deactivated, banned or expired (never falling through to the session branch), audit logging of every sweep, a sanitized 500 on failure, an owner-scoped file half (a key-scoped sweep never nulls a file another tenant owns) and an atomic sweep — chunked in 200-batch transactions in instance mode — 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))

src/app/api/v1/batches/delete-completed/route.ts

@@ -1,7 +1,12 @@
 import { CORS_HEADERS, handleCorsOptions } from "@/shared/utils/cors";
-import { deleteCompletedBatches } from "@/lib/db/batches";
+import { deleteCompletedBatches, type DeleteCompletedBatchesScope } from "@/lib/db/batches";
+import { validateApiKey } from "@/lib/db/apiKeys";
 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,18 +16,83 @@ 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) {
-    return NextResponse.json(
-      { error: { message: "Authentication required", type: "invalid_request_error" } },
-      { status: 401, headers: CORS_HEADERS }
-    );
+  // Fail closed on an unresolvable OR invalid credential. `getApiKeyRequestScope`
+  // resolves the key by row EXISTENCE (so the list/count siblings can still
+  // attribute reads); existence is not authorization for a destructive sweep:
+  // a revoked, deactivated, banned or expired key still has a row and would
+  // otherwise run the sweep (CWE-613). `validateApiKey` is the one lifecycle
+  // gate (is_active, revoked_at, is_banned, expires_at) — and neither case may
+  // fall through to the session branch and widen the sweep to the whole instance.
+  if (scope.apiKey && (!scope.apiKeyId || !(await validateApiKey(scope.apiKey)))) {
+    log.warn("BATCHES", "delete-completed: presented API key rejected", {
+      route: LOG_ROUTE,
+      reason: scope.apiKeyId ? "invalid" : "unresolved",
+      apiKeyId: scope.apiKeyId,
+      isSessionAuth: scope.isSessionAuth,
+    });
+    return NextResponse.json(buildErrorBody(401, "Invalid API key"), {
+      status: 401,
+      headers: CORS_HEADERS,
+    });
+  }
+
+  // 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;
+  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(buildErrorBody(401, "Authentication required"), {
+      status: 401,
+      headers: CORS_HEADERS,
+    });
   }
 
-  // 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);
+  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 ? { message: err.message, stack: err.stack } : 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,
+  };
+  // A bulk delete is an audit event, not routine chatter: an instance-wide sweep
+  // and any key-scoped sweep that actually removed rows log at warn so the trail
+  // survives APP_LOG_LEVEL=warn; a no-op key-scoped sweep stays at info so a
+  // caller looping on the endpoint cannot flood the warn log.
+  if (mode === "instance") {
+    log.warn("BATCHES", "instance-wide completed-batch sweep", audit);
+  } else if (result.deletedBatches > 0) {
+    log.warn("BATCHES", "completed-batch sweep", audit);
+  } else {
+    log.info("BATCHES", "completed-batch sweep (no-op)", audit);
+  }
 
   return NextResponse.json(
     { deleted: true, deletedBatches: result.deletedBatches, deletedFiles: result.deletedFiles },

src/lib/db/batches.ts

@@ -1,6 +1,9 @@
 import { getDbInstance, rowToCamel, objToSnake } from "./core";
-import { deleteFile } from "./files";
+import { deleteFile, deleteFileOwnedBy } 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;
@@ -412,71 +415,155 @@ export function deleteBatch(id: string): boolean {
 }
 
 /**
- * Bulk-delete completed batches and the files they reference.
+ * 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 };
+
+/** Instance-wide sweeps commit in chunks of this many batches (SEC-D). */
+export const INSTANCE_SWEEP_CHUNK = 200;
+
+/**
+ * Delete completed batches and the files they reference.
+ *
+ * `{ 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, and a scope carrying BOTH `apiKeyId` and `allTenants` is
+ * rejected rather than widened.
+ *
+ * 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 }`.
  *
- * `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.
+ * In key mode the file half is owner-scoped too: only files whose api_key_id is
+ * the caller's are soft-deleted; a referenced file another tenant owns (or an
+ * unowned one) is left intact and is not counted in deletedFiles.
+ *
+ * The file soft-deletes, the checkpoint DELETE and the batches DELETE for a set
+ * of batch ids run in one transaction, so a mid-sweep failure rolls that set back
+ * — within a chunk, no batch row is left pointing at a file whose content was
+ * already nulled. Both modes walk the key's/instance's completed batches in
+ * chunks of `INSTANCE_SWEEP_CHUNK` ids and run that unit once per chunk.
+ * Key mode wraps the whole chunk loop in ONE outer transaction (a nested
+ * transaction call is a savepoint on every adapter), so the key sweep stays
+ * all-or-nothing: a failure in any chunk rolls every earlier chunk back too.
+ * Instance mode (`allTenants`) commits per chunk (SEC-D): a large sweep never
+ * holds one write lock over the whole table, each chunk stays atomic, and a
+ * failure inside chunk N leaves chunks < N committed, chunk N fully rolled
+ * back, and rethrows. Inherent to per-chunk commits: a file shared by batches
+ * in two different chunks can be nulled by chunk 1 before chunk 2 fails; the
+ * surviving batch row is swept by the next run. The returned totals sum the
+ * chunks.
+ *
+ * The ids of a unit are bound as `IN (?, …)` placeholders. No statement ever
+ * binds more than `INSTANCE_SWEEP_CHUNK` ids in either mode, so a tenant with
+ * tens of thousands of completed batches never hits SQLite's default
+ * SQLITE_MAX_VARIABLE_NUMBER (32766 since 3.32).
  */
-export function deleteCompletedBatches(apiKeyId?: string | null): {
+export function deleteCompletedBatches(scope: DeleteCompletedBatchesScope): {
   deletedBatches: number;
   deletedFiles: number;
 } {
-  const db = getDbInstance();
-  const scoped = typeof apiKeyId === "string" && apiKeyId.length > 0;
-
-  // 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;
-  }>;
-
-  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);
+  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 && (typeof apiKeyId !== "string" || apiKeyId.trim() === "")) {
+    throw new Error("deleteCompletedBatches: apiKeyId required unless allTenants");
+  }
+  if (allTenants && apiKeyId) {
+    throw new Error("deleteCompletedBatches: apiKeyId and allTenants are mutually exclusive");
   }
 
-  let deletedFiles = 0;
-  for (const fid of fileIds) {
-    try {
-      if (deleteFile(fid)) deletedFiles++;
-    } catch {
-      /* ignore */
+  const db = getDbInstance();
+
+  // One consistent unit: file soft-deletes → checkpoints → batch rows for a
+  // given set of batch ids. Both modes run it per chunk of INSTANCE_SWEEP_CHUNK
+  // ids; key mode nests the chunks in one outer transaction, instance mode
+  // commits each chunk so a large sweep never holds one write-lock for the
+  // whole table (SEC-D) while each chunk stays atomic.
+  const sweepIds = db.transaction((ids: string[]) => {
+    if (ids.length === 0) return { deletedBatches: 0, deletedFiles: 0 };
+    const marks = ids.map(() => "?").join(",");
+    const rows = db
+      .prepare(
+        `SELECT input_file_id, output_file_id, error_file_id FROM batches WHERE id IN (${marks})`
+      )
+      .all(...ids) 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);
     }
-  }
 
-  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);
+    let deletedFiles = 0;
+    for (const fid of fileIds) {
+      try {
+        // Key mode: only the key's OWN files. A batch may reference a file
+        // another tenant (or nobody) owns; a bulk destructive sweep must not
+        // reach it (SEC-C). Instance mode keeps the unconditional soft delete.
+        const removed = allTenants ? deleteFile(fid) : deleteFileOwnedBy(fid, apiKeyId as string);
+        if (removed) deletedFiles++;
+      } catch (err) {
+        log.warn("deleteCompletedBatches: file soft-delete failed", {
+          fid,
+          err: err instanceof Error ? err.message : String(err),
+        });
+      }
+    }
+
+    db.prepare(`DELETE FROM batch_item_checkpoints WHERE batch_id IN (${marks})`).run(...ids);
+    const result = db.prepare(`DELETE FROM batches WHERE id IN (${marks})`).run(...ids);
     return { deletedBatches: result.changes, deletedFiles };
-  }
+  });
 
-  db.prepare(
-    "DELETE FROM batch_item_checkpoints WHERE batch_id IN (SELECT id FROM batches WHERE status = 'completed')"
-  ).run();
+  if (!allTenants) {
+    // One outer transaction so the key sweep stays all-or-nothing; inside it,
+    // the same 200-id unit as instance mode (nested transaction calls become
+    // savepoints on every adapter), so no statement ever binds more than
+    // INSTANCE_SWEEP_CHUNK ids — a tenant with tens of thousands of completed
+    // batches must not hit SQLite's 32766 bound-parameter ceiling.
+    const keyChunk = db.prepare(
+      "SELECT id FROM batches WHERE status = 'completed' AND api_key_id = ? ORDER BY rowid LIMIT ?"
+    );
+    const sweepKey = db.transaction(() => {
+      const totals = { deletedBatches: 0, deletedFiles: 0 };
+      for (;;) {
+        const ids = (keyChunk.all(apiKeyId, INSTANCE_SWEEP_CHUNK) as Array<{ id: string }>).map(
+          (r) => r.id
+        );
+        if (ids.length === 0) break;
+        const part = sweepIds(ids);
+        totals.deletedBatches += part.deletedBatches;
+        totals.deletedFiles += part.deletedFiles;
+      }
+      return totals;
+    });
+    return sweepKey();
+  }
 
-  const result = db.prepare("DELETE FROM batches WHERE status = 'completed'").run();
-  return { deletedBatches: result.changes, deletedFiles };
+  const totals = { deletedBatches: 0, deletedFiles: 0 };
+  const nextChunk = db.prepare(
+    "SELECT id FROM batches WHERE status = 'completed' ORDER BY rowid LIMIT ?"
+  );
+  for (;;) {
+    const ids = (nextChunk.all(INSTANCE_SWEEP_CHUNK) as Array<{ id: string }>).map((r) => r.id);
+    if (ids.length === 0) break;
+    const part = sweepIds(ids);
+    totals.deletedBatches += part.deletedBatches;
+    totals.deletedFiles += part.deletedFiles;
+  }
+  return totals;
 }

src/lib/db/files.ts

@@ -168,3 +168,22 @@ export function deleteFile(id: string): boolean {
     .run(Math.floor(Date.now() / 1000), id);
   return result.changes > 0;
 }
+
+/**
+ * Owner-scoped soft delete: same effect as `deleteFile`, but only when the
+ * file belongs to `apiKeyId`. Used by the key-scoped completed-batch sweep so
+ * a batch that references another tenant's (or an unowned) file never nulls
+ * that file's content (GHSA-wvxc-jp3v-5mg5, SEC-C). Returns false when the
+ * row is not the caller's; throws on an empty owner so a caller cannot widen
+ * the delete by passing a blank id.
+ */
+export function deleteFileOwnedBy(id: string, apiKeyId: string): boolean {
+  if (typeof apiKeyId !== "string" || apiKeyId.trim() === "") {
+    throw new Error("deleteFileOwnedBy: apiKeyId is required");
+  }
+  const db = getDbInstance();
+  const result = db
+    .prepare("UPDATE files SET deleted_at = ?, content = NULL WHERE id = ? AND api_key_id = ?")
+    .run(Math.floor(Date.now() / 1000), id, apiKeyId);
+  return result.changes > 0;
+}