fix(api): scope batch bulk-delete to the calling API key - #12969
Merged
diegosouzapw merged 11 commits intoSep 14, 2026
Merged
Conversation
`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.
…atches/delete-completed (omni-code-review battery on #12969) (#13262) * 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 * 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 * test(batches): pin APP_LOG_LEVEL in the sweep-ownership test so the log assertion is environment-independent * fix(api): fail closed on an unresolvable API key in the completed-batch sweep; no-op key sweeps log at info
This was referenced Sep 10, 2026
…NCE_SWEEP_CHUNK ids per statement The B3 restructure collected ALL of a key's completed batch ids into one IN (…) list; past SQLite's 32766 bound-parameter ceiling the sweep would throw and the tenant's only bulk-cleanup path would stay dead. Key mode now runs the same 200-id unit inside ONE outer transaction (nested calls are savepoints), so it stays all-or-nothing and never exceeds the ceiling. Also: whitespace-only apiKeyId rejected at the top; JSDoc qualifies the nulled-file guarantee as per-chunk in instance mode. Refs #12969
14 tasks
jonlwheat2-gif
added a commit
to jonlwheat2-gif/OmniRoute
that referenced
this pull request
Sep 13, 2026
…pw#13377) Auth (from PR diegosouzapw#13375): - Delete isStaleDashboardJwtError + staleDashboardJwtWarningEmitted from pipeline.ts; emit log in !payload branch - Minters use getDashboardJwtSecret() (trimmed) in login/route.ts, oidc/callback/route.ts - ws/handshake.ts uses getDashboardJwtSecret() instead of raw env - verifyDashboardSessionToken: pin {algorithms:['HS256']}, reject empty Uint8Array - Use DASHBOARD_SESSION_COOKIE/CLAIM constants in 7 consumers - Source guard: allowlist dashboardSessionToken.ts + oidc/callback for jwtVerify imports - Add try/finally around env mutation in dashboard-session-token-13298.test.ts - AUTHZ_GUIDE.md: 'route guard' -> 'dashboard route guard (isDashboardSessionAuthenticated())' Tests: 28/28 auth tests pass, typecheck clean Scope note: the Batches half of diegosouzapw#13377 (forward-progress guard for deleteCompletedBatches) is intentionally NOT in this PR. The issue's wording ("both for (;;) loops") targets PR diegosouzapw#13374's chunked implementation, which is stacked on PR diegosouzapw#12969 — not this release/v3.8.51 base. It is tracked separately.
…-authz # Conflicts: # src/app/api/v1/batches/delete-completed/route.ts # src/lib/db/batches.ts
This was referenced Sep 14, 2026
diegosouzapw
added a commit
that referenced
this pull request
Sep 15, 2026
…es-delete-completed-authz) (#13684) Batch sweep enforces the caller's API-key policy (allowedEndpoints/schedule/usage/rate limit; the /api/v1 pathname now resolves its endpoint category for every /v1 route), commits per 200-batch chunk in key mode, guards against no-progress loops, rejects a scope naming both a key and allTenants; 8 covering tests registered for the mutation gate. Remaining CI reds are release base-reds (#12732), reproduced identically on the base tip. Refs #12969, #13680, #13681, #13685, #13377
muhamadgalihsaputra
pushed a commit
to niyatna/NiyatnaRoute
that referenced
this pull request
Sep 27, 2026
…w#12969) Merged after reconciling the whole stack onto the tip, operator-reviewed before merge. The core ownership fix for GHSA-wvxc-jp3v-5mg5 had already landed via diegosouzapw#13211 with a different implementation of the same endpoint. This branch carries a stricter one, built up in layers: this PR, diegosouzapw#13262 (explicit scope, audit log, atomic sweep), diegosouzapw#13297 (revoked, deactivated, banned or expired keys rejected) and diegosouzapw#13374 (owner-scoped file half, chunked instance sweep — SEC-C/SEC-D). The stack's implementation was kept over diegosouzapw#13211's because it is stricter on every point: - **route:** with diegosouzapw#13211, a request carrying BOTH a dashboard session cookie and an API key swept the whole instance. Here a presented key always scopes the sweep to that key; only a session without a key sweeps all tenants; neither returns 401. Instance-wide and row-deleting sweeps log at warn as an audit trail, and a failing sweep returns a sanitized 500. - **`deleteCompletedBatches`:** takes an explicit `{ apiKeyId } | { allTenants: true }`. An omitted or blank key throws instead of widening, and passing both throws. - **files:** a key sweep only soft-deletes files the caller owns (`deleteFileOwnedBy`), so a batch referencing another tenant's or an unowned file never nulls its content. - **transactions:** key mode is all-or-nothing across chunks; instance mode commits per 200-id chunk so a large sweep never holds one write lock on the table. diegosouzapw#13211's own test was aligned to the explicit-scope API with its assertions unchanged. Its seed now creates the file with the batch's owner, as an upload through that key does in production — without that, SEC-C correctly leaves the unowned file intact. - 51/51 across the six batch suites (diegosouzapw#13211's test, this stack's ownership and route-scope tests, `batch-deletion`, `batch-deletion-route-logic`, `files-delete-owned-by`) - ESLint, `typecheck:core`, complexity, cognitive-complexity, changelog integrity: clean⚠️ base-red inherited: diegosouzapw#12732
muhamadgalihsaputra
pushed a commit
to niyatna/NiyatnaRoute
that referenced
this pull request
Sep 27, 2026
…es-delete-completed-authz) (diegosouzapw#13684) Batch sweep enforces the caller's API-key policy (allowedEndpoints/schedule/usage/rate limit; the /api/v1 pathname now resolves its endpoint category for every /v1 route), commits per 200-batch chunk in key mode, guards against no-progress loops, rejects a scope naming both a key and allTenants; 8 covering tests registered for the mutation gate. Remaining CI reds are release base-reds (diegosouzapw#12732), reproduced identically on the base tip. Refs diegosouzapw#12969, diegosouzapw#13680, diegosouzapw#13681, diegosouzapw#13685, diegosouzapw#13377
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the missing authorization reported privately as GHSA-wvxc-jp3v-5mg5
(CWE-862, High, CVSS 3.1 8.1). Confirmed still present on the
release/v3.8.51tip (
d6f3150), not only on the reportedv3.8.50.The defect
DELETE /api/v1/batches/delete-completedaccepts any valid ordinary inferencekey — including one with
scopes: []— and then calleddeleteCompletedBatches()with no ownership predicate:
Every file referenced by those batches was passed through
deleteFile(), whichsets
content = NULL. One tenant could therefore destroy every other tenant'scompleted batches and their stored file contents. No victim batch id, file id or
key id was needed.
Why this shape of fix
Every sibling operation already keeps the boundary —
listBatches(apiKeyId?)andcountBatches(apiKeyId?)scope the SQL toapi_key_id = ?and fall back toinstance-wide only when the caller is an authenticated dashboard session (which
passes
undefined).deleteCompletedBatcheswas the single operation thatdropped it, so the fix restores the existing convention rather than inventing a
new authorization layer:
apiKeyIdand appendsAND api_key_id = ?to thefile collection, the checkpoint delete and the batch delete;
scope.apiKeyId || undefined.Net effect: a dashboard session keeps the instance-wide sweep the UI relies on,
and an API key only ever clears its own batches. The route is the only production
call site (verified by grep across
src/,open-sse/,bin/,electron/).Validation (Hard Rule #18 — TDD)
tests/unit/batches-delete-completed-ownership-wvxc.test.tspins all three halvesof the contract. On the pre-fix helper the first case fails exactly as the
report describes:
After the fix:
batch-deletion+batch-deletion-route-logic+batch_api+batches-f9-helpersnpm run typecheck:coreeslint+prettier --checkon the touched filesFollow-up (not in this PR)
The reporter notes that Threat Model §3.13 documents this path as
POSTwhilethe implemented route and the dashboard use
DELETE. That is a docs/route namingmismatch, not an authorization defect, and is left for a separate docs pass.
release/v3.8.51is not green. Every failingcheck on this PR is inherited, none is a defect of this branch:
Unit Tests fast-path (1..4/4)— failures are inchatCore,HuggingChat,Claude/Responsesstreaming,catalog build (#9147)andcombo.ts. Thisbranch touches only
src/lib/db/batches.tsand thedelete-completedroute;no batch test fails in any of the four shards.
Fast Quality Gates/Docs Gates/Merge integrity/API Route Typecheck—same shape as the tip, listed in the base-red issue.
The change's own gates are green locally: the new regression guard (3/3), the four
existing batch suites (60/60),
typecheck:core, eslint and prettier.Review batteries (omni-code-review / omni-code-sec)