Skip to content

fix(api): enforce API-key ownership on files and batches — null-owner records and anonymous listing - #13749

Merged
diegosouzapw merged 1 commit into
release/v3.8.51from
fix/sec-files-batches-ownership-2jm2-m3hp
Sep 15, 2026
Merged

diegosouzapw merged 1 commit into
release/v3.8.51from
fix/sec-files-batches-ownership-2jm2-m3hp

Conversation

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Fixes two privately reported advisories with one root cause: GHSA-2jm2-mpx8-6523 and
GHSA-m3hp-hq9g-fpmv. Subsumes #13683 (credit to @hartmark in the commit trailer).

Root cause

getApiKeyRequestScope() never rejects. Under 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 read that
null as permissive in two ways:

  • GHSA-m3hp (listing): listFiles / listBatches got apiKeyId || undefined, and the DB layer
    treats undefined as "no owner filter". An anonymous or invalid-bearer caller received every
    tenant's
    file and batch metadata — the same view as the operator's dashboard.
  • GHSA-2jm2 (null-owner records): single-record checks were record.apiKeyId !== null && …, so a
    record without an owner was open to any caller.
Route Before, for a foreign key or anonymous caller
GET /v1/files/[id], /content null-owner metadata and raw bytes readable
DELETE /v1/files/[id] null-owner file deletable (content nulled)
POST /v1/batches foreign key could run a batch over a null-owner input file
GET / DELETE /v1/batches/[id], POST …/cancel null-owner batch readable / deletable / cancellable
GET /v1/files, GET /v1/batches every tenant's records

Null-owner records are not rare: every dashboard-session upload is one, and batch output files
inherit the batch's owner, so a session batch produces null-owner files carrying model responses.
api_key_id has existed since migration 028, so these are not legacy rows. Nothing documented them
as shared — API_REFERENCE.md says files are scoped per key — while batch_api.test.ts pinned the
by-id exposure as expected.

Change — one rule, in _helpers/apiKeyScope.ts

  • canAccessOwnedRecord(scope, owner) — dashboard session: any record; API key: own records
    only; null owner: denied to every non-session caller. Used by all seven single-record paths above.
  • resolveListScope(scope) — an explicit union for list reads: scoped to the presented key (a
    key wins even alongside a session cookie), instance-wide only for a session without a key, 401
    otherwise — including a bearer that does not resolve to a key. No default widens a read.

Same shape as the GHSA-wvxc fix already used by batches/delete-completed.

Behaviour change: the anonymous upload → batch → download flow stops working without an API key,
because a null owner cannot be attributed to anyone.

About #13683

It moved scopeCheck into the shared helper so a session can cancel any batch. That part is kept
here and its test (batch-cancel-session-auth-scope.test.ts) is ported. It also kept null-owner
records open on the premise that they predate ownership tracking, which migration 028 contradicts —
that is the GHSA-2jm2 hole.

Validation (TDD)

Check Result
New files-batches-ownership-2jm2-m3hp.test.ts (anonymous / invalid bearer → 401, key-scoped lists, session sees all, key+cookie stays key-scoped, null-owner 404, foreign key cannot batch a null-owner input) — on the tip red
Same file + ported #13683 test + flipped batch_api + batch-deletion-route-logic + delete-completed scope 62 / 62
Files/batches unit suite (522 tests) 522 / 522
integration/files-api-limit-validation 6 / 6
typecheck:core, check:docs-sync clean
eslint (repo suppressions), prettier clean

Test adaptations: batch_api's by-id case is flipped to 404 with a negative assertion (it was
pinning the bug); 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 API key, since
their subject is limits and rate logging, not auth.

integration/batch-e2e-rate-limit could not be validated locally: the Next server did not become
ready within the harness timeout on a box at load average 45 (Timed out waiting for server, before
any files/batches code runs). CI should settle it.

⚠️ base-red inherited: #12732 — failures outside the files/batches suites come from the tip.

… records and anonymous listing

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
diegosouzapw merged commit e7f9fec into release/v3.8.51 Sep 15, 2026
15 of 21 checks passed
diegosouzapw added a commit that referenced this pull request Sep 15, 2026
…ile-size caps

PR-mode check:file-size relaxes source files against the base but not
testFrozen, so image-generation-handler.test.ts (2133->2235, #13748) and
batch_api.test.ts (1345->1348, #13749) failed Fast Quality Gates on every PR,
this one included. Caps set to the merged LOC, with the justification entry.

Refs #12732
diegosouzapw added a commit that referenced this pull request Sep 16, 2026
…(stryker, CLI i18n, paid-target fixture, call-log traceId, Jina prefix, callLogStats import, gitleaks) (#13747)

* fix(ci): clear the release/v3.8.51 base-reds left by the 09-15 batch — stryker coverage, CLI ready_timeout key, paid-target fixture, call-log traceId, Jina custom prefix

Every PR into release/v3.8.51 pushed after #13635/#13678 still failed Fast
Quality Gates and all four Unit fast-path shards on the same 16 tests. Each one
reproduces on the pure tip; none is a product defect:

- mutation-test-coverage: noauth-model-lockout and
  local-token-budget-429-skips-cooldown (#13606) were missing from
  stryker.conf.json tap.testFiles.
- cli-i18n-catalog: --ready-timeout calls t("serve.ready_timeout") with no
  catalog entry; added to en, zh-CN and zh-TW (the parity-checked locales).
- paid-model-target(-routes)-6540: #13407 removed Together's one-time credit
  from the free catalog, so "together/..." classifies as unknown and the
  save-time guard correctly lets it through. Fixture is now
  gemini/gemini-3.1-pro-preview, plus a precondition test on the fixtures.
- attempt-logging-early-keepalive-merge / video-bridge-log-redaction: #13546
  keys the call-log row on traceId; baseCtx now defaults traceId to
  pendingRequestId (same pattern as chatcore-attempt-logging). The keepalive
  test also moves to the 30s wall-clock poll deadline video-bridge uses.
- models-catalog-route: custom Jina rows keep the jina-ai/ prefix; #13403
  changed the custom assertion to jina/ (only synced rows use the alias).

Refs #12732

* fix(ci): clear the four reds the first r4 CI run surfaced — callLogStats duplicate import, Uzbek gitleaks false positive, redaction probe traceId, file-size

- src/lib/db/callLogStats.ts: the #13641 merge left ERROR_TYPE_CONTRACT
  imported twice (TS2300), failing API Route Typecheck and
  check:dashboard-typecheck on every PR.
- .gitleaks.toml: the Uzbek catalog from #13727 translates outputTokenDesc as
  "Yakunlash/javob tokenlari"; generic-api-key reads it as a token value.
- dashboard-request-failed-redaction-probe: reads the persisted row by
  traceId (#13546); with pendingRequestId it asserts null.
- models-catalog-route: drop the explanatory comment, which pushed the frozen
  file over its size cap; the rationale lives in the changelog fragment.

Refs #12732

* fix(ci): re-freeze the two test files #13748/#13749 grew past their file-size caps

PR-mode check:file-size relaxes source files against the base but not
testFrozen, so image-generation-handler.test.ts (2133->2235, #13748) and
batch_api.test.ts (1345->1348, #13749) failed Fast Quality Gates on every PR,
this one included. Caps set to the merged LOC, with the justification entry.

Refs #12732

* fix(ci): register free-badge-provider-gate (#13645) in stryker tap.testFiles

#13645 landed a covering test for src/sse/services/auth.ts without the
stryker entry, so the strict mutation-test-coverage gate went red again.

Refs #12732

* fix(ci): clear two more base-reds the #13440/#13439 merges added

- stryker.conf.json: register daily-reset-tz-threading (#13440), which covers
  accountFallback.ts and rrState.ts.
- .gitleaks.toml: allowlist the PROTECTED_PRIORITY_INFRA_502_ENABLED flag id
  (#13439); generic-api-key reads its key: as a token (secrets ratchet 0 -> 1).

Refs #12732

* docs(changelog): tidy the stryker base-red fragment wording

Refs #12732
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
… records and anonymous listing (cheaper-inference#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 cheaper-inference#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>
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…(stryker, CLI i18n, paid-target fixture, call-log traceId, Jina prefix, callLogStats import, gitleaks) (cheaper-inference#13747)

* fix(ci): clear the release/v3.8.51 base-reds left by the 09-15 batch — stryker coverage, CLI ready_timeout key, paid-target fixture, call-log traceId, Jina custom prefix

Every PR into release/v3.8.51 pushed after cheaper-inference#13635/cheaper-inference#13678 still failed Fast
Quality Gates and all four Unit fast-path shards on the same 16 tests. Each one
reproduces on the pure tip; none is a product defect:

- mutation-test-coverage: noauth-model-lockout and
  local-token-budget-429-skips-cooldown (cheaper-inference#13606) were missing from
  stryker.conf.json tap.testFiles.
- cli-i18n-catalog: --ready-timeout calls t("serve.ready_timeout") with no
  catalog entry; added to en, zh-CN and zh-TW (the parity-checked locales).
- paid-model-target(-routes)-6540: cheaper-inference#13407 removed Together's one-time credit
  from the free catalog, so "together/..." classifies as unknown and the
  save-time guard correctly lets it through. Fixture is now
  gemini/gemini-3.1-pro-preview, plus a precondition test on the fixtures.
- attempt-logging-early-keepalive-merge / video-bridge-log-redaction: cheaper-inference#13546
  keys the call-log row on traceId; baseCtx now defaults traceId to
  pendingRequestId (same pattern as chatcore-attempt-logging). The keepalive
  test also moves to the 30s wall-clock poll deadline video-bridge uses.
- models-catalog-route: custom Jina rows keep the jina-ai/ prefix; cheaper-inference#13403
  changed the custom assertion to jina/ (only synced rows use the alias).

Refs cheaper-inference#12732

* fix(ci): clear the four reds the first r4 CI run surfaced — callLogStats duplicate import, Uzbek gitleaks false positive, redaction probe traceId, file-size

- src/lib/db/callLogStats.ts: the cheaper-inference#13641 merge left ERROR_TYPE_CONTRACT
  imported twice (TS2300), failing API Route Typecheck and
  check:dashboard-typecheck on every PR.
- .gitleaks.toml: the Uzbek catalog from cheaper-inference#13727 translates outputTokenDesc as
  "Yakunlash/javob tokenlari"; generic-api-key reads it as a token value.
- dashboard-request-failed-redaction-probe: reads the persisted row by
  traceId (cheaper-inference#13546); with pendingRequestId it asserts null.
- models-catalog-route: drop the explanatory comment, which pushed the frozen
  file over its size cap; the rationale lives in the changelog fragment.

Refs cheaper-inference#12732

* fix(ci): re-freeze the two test files cheaper-inference#13748/cheaper-inference#13749 grew past their file-size caps

PR-mode check:file-size relaxes source files against the base but not
testFrozen, so image-generation-handler.test.ts (2133->2235, cheaper-inference#13748) and
batch_api.test.ts (1345->1348, cheaper-inference#13749) failed Fast Quality Gates on every PR,
this one included. Caps set to the merged LOC, with the justification entry.

Refs cheaper-inference#12732

* fix(ci): register free-badge-provider-gate (cheaper-inference#13645) in stryker tap.testFiles

cheaper-inference#13645 landed a covering test for src/sse/services/auth.ts without the
stryker entry, so the strict mutation-test-coverage gate went red again.

Refs cheaper-inference#12732

* fix(ci): clear two more base-reds the cheaper-inference#13440/cheaper-inference#13439 merges added

- stryker.conf.json: register daily-reset-tz-threading (cheaper-inference#13440), which covers
  accountFallback.ts and rrState.ts.
- .gitleaks.toml: allowlist the PROTECTED_PRIORITY_INFRA_502_ENABLED flag id
  (cheaper-inference#13439); generic-api-key reads its key: as a token (secrets ratchet 0 -> 1).

Refs cheaper-inference#12732

* docs(changelog): tidy the stryker base-red fragment wording

Refs cheaper-inference#12732
@diegosouzapw
diegosouzapw deleted the fix/sec-files-batches-ownership-2jm2-m3hp branch October 18, 2026 23:51
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.

1 participant