Skip to content

fix(batches): let session auth cancel any batch, not just apiKeyId-null ones - #13683

Closed
hartmark wants to merge 1 commit into
diegosouzapw:release/v3.8.51from
hartmark:fix/batch-cancel-session-auth-scope
Closed

hartmark wants to merge 1 commit into
diegosouzapw:release/v3.8.51from
hartmark:fix/batch-cancel-session-auth-scope

Conversation

@hartmark

Copy link
Copy Markdown
Contributor

Problem

POST /api/v1/batches/[id]/cancel rejected the dashboard's own session-authenticated
caller as "Batch not found" (404) for any batch owned by a non-null api_key_id --
which in practice is every batch created through the default env-key. Clicking
Cancel in the dashboard on /dashboard/batch silently did nothing; the batch just
sat there. Confirmed live: two real stuck batches (validating, in_progress)
both have api_key_id: "env-key" and cancelling_at: null, proving every cancel
click against them had been rejected the whole time.

Root cause

The route carried its own inline ownership check:

if (!batch || (batch.apiKeyId !== null && batch.apiKeyId !== apiKeyId)) { ... 404 ... }

This never exempts session auth (apiKeyId === null, the dashboard) from the
mismatch check the moment batch.apiKeyId is non-null. Two sibling
implementations in the same codebase already get this right:

  • batches/[id]/route.ts (GET/DELETE) — its own local scopeCheck()
  • deleteCompletedBatches() (GHSA-wvxc-jp3v-5mg5) — scope.isSessionAuth ? undefined : scope.apiKeyId

Both treat session auth as the instance-wide operator, able to act on any
record regardless of which API key owns it. cancel/route.ts was the one
endpoint that never got that exemption.

Fix

  • Promoted [id]/route.ts's local scopeCheck() to the shared
    _helpers/apiKeyScope.ts (both routes now import the same function instead
    of carrying independent copies of the same logic).
  • Switched cancel/route.ts to use !scopeCheck(scope, batch.apiKeyId)
    instead of its buggy inline predicate.

Evidence

  • New regression test (tests/unit/batch-cancel-session-auth-scope.test.ts)
    proves session auth can now cancel a batch owned by "env-key", proves a
    foreign API key still cannot cancel someone else's batch, and proves the
    owning API key still can cancel its own -- plus a source-level assertion
    that the old buggy inline check is gone. Confirmed it fails for the
    intended reason against the pre-fix route (old inline check present) and
    passes against the fix.
  • Full sibling batch/file/cancel unit suite re-run (70 tests across 6 files,
    including the GHSA-wvxc-jp3v-5mg5 ownership test and the existing "Batch
    Cancel API" test) — all green, no regressions.
  • tsc -p tsconfig.typecheck-core.json clean.

…ll ones

POST /api/v1/batches/[id]/cancel rejected the dashboard's own
session-authenticated caller as "Batch not found" (404) for any batch owned
by a non-null api_key_id -- which in practice is every batch created through
the default env-key, i.e. every real batch on the instance. Clicking cancel
in the dashboard silently did nothing.

The route carried its own inline ownership check
(`batch.apiKeyId !== null && batch.apiKeyId !== apiKeyId`) instead of the
canonical scopeCheck() that two sibling implementations already get right:
batches/[id]/route.ts (GET/DELETE) and deleteCompletedBatches()
(GHSA-wvxc-jp3v-5mg5) both treat session auth as the instance-wide operator,
able to act on any record regardless of which API key owns it. The inline
check never granted that exemption.

Promoted [id]/route.ts's local scopeCheck() to the shared
_helpers/apiKeyScope.ts (both routes now import the same function instead of
carrying independent copies), and switched cancel/route.ts to use it.

Regression test proves the fix and proves the old inline check would have
rejected the exact shape of the two batches actually stuck in production
(api_key_id: "env-key").
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 14, 2026
…y batch, not just apiKeyId-null ones) into dev/omniroute-dev-combined
hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 14, 2026
…y batch, not just apiKeyId-null ones) into dev/omniroute-dev-combined
@diegosouzapw

Copy link
Copy Markdown
Owner

Confirmed and reproduced: the inline ownership check in cancel/route.ts never exempted
session auth from the apiKeyId mismatch, so clicking Cancel in the dashboard 404'd on any
batch owned by a non-null key (i.e. almost every real batch, since they're created via the
default env-key). The fix correctly mirrors the sibling [id]/route.ts's scopeCheck() —
promoted to the shared helper instead of duplicated — and the new regression test (5/5,
verified against the PR head) proves both the fix and that a foreign key still can't touch
someone else's batch. Clean, minimal, ready to merge.

hartmark added a commit to hartmark/OmniRoute that referenced this pull request Sep 15, 2026
…y batch, not just apiKeyId-null ones) into dev/omniroute-dev-combined
diegosouzapw added a commit that referenced this pull request Sep 15, 2026
… records and anonymous listing (#13749)

GHSA-2jm2-mpx8-6523 and GHSA-m3hp-hq9g-fpmv, one root cause.

`getApiKeyRequestScope()` never rejects: with the default REQUIRE_API_KEY=false
the client-api policy admits both a missing and an invalid bearer as anonymous,
and the scope comes back `{ apiKeyId: null, isSessionAuth: false }`. The
`/v1/files` and `/v1/batches` routes then treated "null" as permissive in two
different ways:

- GHSA-m3hp — the list routes coerced `apiKeyId || undefined`, and the DB layer
  reads `undefined` as "no owner filter", so an anonymous or invalid-bearer
  caller got every tenant's file and batch metadata, the same unfiltered view as
  the operator's dashboard.
- GHSA-2jm2 — the single-record checks were `record.apiKeyId !== null && …`, so
  a record with no owner short-circuited to "allowed" for any caller: read,
  download raw content, delete, cancel, or use as a batch input. Null-owner
  records are common — every dashboard-session upload, and every batch output
  file inheriting a session batch's owner, which carries model responses.

`api_key_id` has existed since the table was created (migration 028), so a null
owner is not a legacy row; it is an unattributable write. No doc described it
as shared — API_REFERENCE says files are scoped per key — and batch_api.test.ts
pinned the by-id exposure as expected behaviour.

One rule now, in `_helpers/apiKeyScope.ts`:

- `canAccessOwnedRecord(scope, owner)`: a dashboard session is the instance
  operator and may act on any record; an API key acts on its own records only;
  a null owner is denied to every non-session caller. Applied to files
  GET / DELETE / content, batches GET / DELETE / cancel, and the batch-create
  input-file check.
- `resolveListScope(scope)`: an explicit union for list/count reads — scoped to
  the presented key (a key wins even alongside a session cookie), instance-wide
  only for a session without a key, and 401 otherwise, including for a bearer
  that does not resolve to a key. There is no default that widens a read.

This follows the GHSA-wvxc shape already used by the delete-completed sweep.

Behaviour change: the anonymous upload → batch → download flow no longer works
without an API key, because a null owner cannot be attributed.

Subsumes #13683: it moved `scopeCheck` into the shared helper so a session can
cancel any batch — kept, and its test ported — but it also kept null-owner
records open on the premise they predate ownership tracking, which migration
028 contradicts.

Tests are red-first. batch_api's by-id case is flipped to 404 with a negative
assertion; batch-deletion-route-logic now imports the real helper instead of a
local copy that had silently diverged from production; the two integration
tests present a real key, since their subject is limits and rate logging, not
auth.

Co-authored-by: Markus Hartung <mail@hartmark.se>
@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks for this, @hartmark — the core of it has landed in #13749, with you credited as co-author on the commit.

Some context on why it went in through a different PR. While triaging two private security advisories (GHSA-2jm2-mpx8-6523 and GHSA-m3hp-hq9g-fpmv), we found that null-owner files and batches were readable, downloadable, deletable and cancellable by any API key, and by anonymous callers under the default REQUIRE_API_KEY=false. The fix needed exactly the shared helper you introduced here, so #13749 builds on your structure:

  • canAccessOwnedRecord() in _helpers/apiKeyScope.ts — a dashboard session can act on any record (your "session can cancel any batch" behaviour, kept), an API key acts on its own records only, and a null owner is denied to every non-session caller;
  • your batch-cancel-session-auth-scope.test.ts is ported as-is.

The one part that could not carry over is keeping null-owner records open. The rationale that those rows predate ownership tracking doesn't hold: api_key_id has existed since the table was created (migration 028), so a null owner is an unattributable write — every dashboard-session upload, and batch output files that inherit it — rather than a legacy row.

Since #13749 covers this PR's behaviour, it can probably be closed; I'm leaving that call to the maintainer.

@hartmark

Copy link
Copy Markdown
Contributor Author

Thanks for the context and the credit — closing this in favor of #13749, which is the correct fix. Appreciate the null-owner rationale being corrected too (api_key_id has existed since migration 028, so that's an unattributable-write risk, not a legacy-row exemption).

@hartmark hartmark closed this Sep 15, 2026
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
… records and anonymous listing (diegosouzapw#13749)

GHSA-2jm2-mpx8-6523 and GHSA-m3hp-hq9g-fpmv, one root cause.

`getApiKeyRequestScope()` never rejects: with the default REQUIRE_API_KEY=false
the client-api policy admits both a missing and an invalid bearer as anonymous,
and the scope comes back `{ apiKeyId: null, isSessionAuth: false }`. The
`/v1/files` and `/v1/batches` routes then treated "null" as permissive in two
different ways:

- GHSA-m3hp — the list routes coerced `apiKeyId || undefined`, and the DB layer
  reads `undefined` as "no owner filter", so an anonymous or invalid-bearer
  caller got every tenant's file and batch metadata, the same unfiltered view as
  the operator's dashboard.
- GHSA-2jm2 — the single-record checks were `record.apiKeyId !== null && …`, so
  a record with no owner short-circuited to "allowed" for any caller: read,
  download raw content, delete, cancel, or use as a batch input. Null-owner
  records are common — every dashboard-session upload, and every batch output
  file inheriting a session batch's owner, which carries model responses.

`api_key_id` has existed since the table was created (migration 028), so a null
owner is not a legacy row; it is an unattributable write. No doc described it
as shared — API_REFERENCE says files are scoped per key — and batch_api.test.ts
pinned the by-id exposure as expected behaviour.

One rule now, in `_helpers/apiKeyScope.ts`:

- `canAccessOwnedRecord(scope, owner)`: a dashboard session is the instance
  operator and may act on any record; an API key acts on its own records only;
  a null owner is denied to every non-session caller. Applied to files
  GET / DELETE / content, batches GET / DELETE / cancel, and the batch-create
  input-file check.
- `resolveListScope(scope)`: an explicit union for list/count reads — scoped to
  the presented key (a key wins even alongside a session cookie), instance-wide
  only for a session without a key, and 401 otherwise, including for a bearer
  that does not resolve to a key. There is no default that widens a read.

This follows the GHSA-wvxc shape already used by the delete-completed sweep.

Behaviour change: the anonymous upload → batch → download flow no longer works
without an API key, because a null owner cannot be attributed.

Subsumes diegosouzapw#13683: it moved `scopeCheck` into the shared helper so a session can
cancel any batch — kept, and its test ported — but it also kept null-owner
records open on the premise they predate ownership tracking, which migration
028 contradicts.

Tests are red-first. batch_api's by-id case is flipped to 404 with a negative
assertion; batch-deletion-route-logic now imports the real helper instead of a
local copy that had silently diverged from production; the two integration
tests present a real key, since their subject is limits and rate logging, not
auth.

Co-authored-by: Markus Hartung <mail@hartmark.se>

@tieng1344 tieng1344 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants