Skip to content

fix(api): enforce image generation API key auth - #8306

Merged
diegosouzapw merged 2 commits into
diegosouzapw:release/v3.8.49from
fenix007:fix/image-generation-api-key-attribution-v3849
Jul 28, 2026
Merged

diegosouzapw merged 2 commits into
diegosouzapw:release/v3.8.49from
fenix007:fix/image-generation-api-key-attribution-v3849

Conversation

@fenix007

@fenix007 fenix007 commented Jul 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Enforce REQUIRE_API_KEY and reject invalid presented keys on the canonical and provider-scoped image generation routes.
  • Propagate validated API-key identity through request-scoped storage so provider-specific image call logs record api_key_id and api_key_name.
  • Restore per-key no-log behavior for image generation calls.
  • Share one route-level auth guard (shared/utils/clientApiRouteAuth) that mirrors the clientApiPolicy contract, so the handler check can never be stricter than the authz middleware already fronting /api/v1/*.

Related Issues

Validation

  • tests/unit/image-generation-route*.test.ts: 24/24 passing
  • call-log* + tests/unit/usage/**: 68/68 passing
  • npm run typecheck:core
  • eslint on all changed files
  • npm run check:file-size — [test-file-size] OK
  • pre-commit gates: check-docs-sync, check:any-budget:t11, check-tracked-artifacts

Tests Added Or Updated

  • tests/unit/image-generation-route-auth.test.ts (new — split out of the main image-route suite to stay under the 800-line new-test-file cap)
    • rejects missing keys when REQUIRE_API_KEY=true
    • rejects invalid presented keys when REQUIRE_API_KEY=true
    • ignores invalid presented keys when REQUIRE_API_KEY=false (matches clientApiPolicy, [BUG] invalid api key in Codex Desktop auto config #2257)
    • accepts a cookie-authenticated dashboard session when REQUIRE_API_KEY=true
    • persists API-key id/name for canonical and provider-scoped image generation
  • tests/unit/image-generation-route.test.ts — keeps the CORS preflight coverage for the three image-route OPTIONS handlers.

Coverage Notes

The tests cover every branch of the shared guard (valid key / invalid key × enforcement on-off / dashboard session / anonymous), successful call-log attribution on both routes, and the OPTIONS handlers that keep the function-coverage ratchet from regressing. The coverage baseline was not changed.

Reviewer Notes

Scope is the image routes, the shared client-API route guard, call-log attribution, and their tests — 7 files. The earlier release-repair commits are dropped; that set lives in #8307 and the base has since resolved most of it. This branch is rebased on the current release/v3.8.49 head and merges cleanly.

Image provider handlers emit call logs in multiple provider-specific branches. AsyncLocalStorage keeps validated key identity request-scoped and concurrency-safe without threading attribution arguments through every handler. Explicit call-log API-key fields still take precedence. No schema or migration changes are required.

check:complexity-ratchets and two check:file-size freezes (dashboard/providers/page.tsx, lib/tokenHealthCheck.ts) fail identically on a clean origin/release/v3.8.49 checkout — inherited base-red, deliberately not touched here.

@fenix007
fenix007 requested a review from diegosouzapw as a code owner July 23, 2026 15:37
diegosouzapw added a commit to fenix007/OmniRoute that referenced this pull request Jul 23, 2026
…8306

The complexity/cognitive-complexity baseline bumps (2130->2168, 951->956)
reconcile pre-existing release-branch drift unrelated to this PR's
image-generation API-key auth fix. Ratchet rebaselines are reserved for
the release captain (owner-approved, see prior _rebaseline_* entries in
these files) and are out of scope for a contributor branch. Reverting to
the recorded baseline; the underlying drift is real (measured 2167/956 on
origin/release/v3.8.49) and already tracked by the owner's open base-red
slice PRs (diegosouzapw#8254, diegosouzapw#8256).

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
@diegosouzapw
diegosouzapw changed the base branch from release/v3.8.49 to main July 23, 2026 23:07
@diegosouzapw
diegosouzapw changed the base branch from main to release/v3.8.49 July 23, 2026 23:07
alexey.nazarov@softmg.ru and others added 2 commits July 25, 2026 12:37
The route-level guard added for image generation was stricter than the
authz middleware that already fronts /api/v1/* (src/proxy.ts →
clientApiPolicy), so requests the pipeline admits were 401'd by the
handler:

- A cookie-authenticated dashboard session was rejected under
  REQUIRE_API_KEY=true. The dashboard Media page
  (dashboard/cache/media) and the Playground call these routes with a
  session and no Bearer — the same mismatch already fixed for
  /api/playground/presets.
- A presented invalid key was rejected even with REQUIRE_API_KEY=false,
  where clientApiPolicy (diegosouzapw#2257) and the sibling /v1/embeddings and
  /v1/web/fetch routes degrade a stale CLI key to anonymous instead.

Extract the shared guard into shared/utils/clientApiRouteAuth so both
image routes (and future /v1 handlers) mirror the middleware contract
instead of re-deriving it, and drop the now-dead auth imports.

Also switch the call-log attribution fallback back to `||`: with `??`,
an empty-string apiKeyId/apiKeyName would be persisted verbatim and
would block the request-scoped context, which the previous
`entry.apiKeyId || null` never did.

Tests: cover the dashboard-session and keyless-mode-invalid-key
branches, and split the auth/attribution cases into
image-generation-route-auth.test.ts to stay under the 800-line
new-test-file cap.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@fenix007
fenix007 force-pushed the fix/image-generation-api-key-attribution-v3849 branch from d110762 to cf5535b Compare July 25, 2026 09:46
@fenix007

Copy link
Copy Markdown
Contributor Author

Rebased onto current release/v3.8.49 + self-review fixes

The branch had drifted 91 commits behind its base and was CONFLICTING in 19 files — all of them from the inherited release-repair commits, none from the image-auth change itself. Rebuilt the branch as the feature commit alone, cherry-picked onto the current base head (30709255) with zero conflicts, then pushed a second commit addressing three issues I found reviewing my own diff.

Scope: 38 files → 7. The release-repair set is dropped entirely; it lives in #8307, and the base has since fixed most of it on its own.

Fixed in cf5535b

1. Dashboard 401 under REQUIRE_API_KEY=true (regression).
The route-level guard was stricter than the authz middleware that already fronts /api/v1/* (src/proxy.ts → clientApiPolicy). clientApiPolicy admits a cookie-authenticated dashboard session with no Bearer; the handler then rejected it. That breaks the dashboard Media page (src/app/(dashboard)/dashboard/cache/media/MediaPageClient.tsx posts to /api/v1/images/generations with no Authorization header) and the Playground's "test with key <id>" path, which enforceApiKeyPolicy resolves via resolvePlaygroundTestKey() after the point where the guard had already returned 401. Same mismatch that was fixed for /api/playground/presets ("QA P1: preset auth mismatch").

2. Invalid key rejected in keyless mode (inconsistency).
if (apiKeyRaw && !(await isValidApiKey(apiKeyRaw))) had no isRequireApiKeyEnabled() guard, so a stale key in a CLI config 401'd even with enforcement off. clientApiPolicy (#2257) and the sibling /v1/embeddings + /v1/web/fetch routes degrade to anonymous there. The inline comment claimed keyless local mode was allowed; the code did the opposite.

Both are now one shared guard — src/shared/utils/clientApiRouteAuth.ts — that mirrors the clientApiPolicy contract instead of each route re-deriving it. This also removes the 4th/5th copy of the same block. /v1/embeddings and /v1/web/fetch can migrate to it in a follow-up; left out here to keep the diff focused.

3. ?? → || in saveCallLog.

const apiKeyId = entry.apiKeyId ?? apiKeyContext?.apiKeyId ?? null;

Before this PR the line was entry.apiKeyId || null. With ??, an empty-string apiKeyId/apiKeyName is persisted verbatim instead of null, and additionally blocks the context fallback. Restored ||.

Tests

  • tests/unit/image-generation-route-auth.test.ts (new) — the auth/attribution cases, split out to stay under the 800-line new-test-file cap that the combined file would have crossed.
  • Added: dashboard session accepted under REQUIRE_API_KEY=true; invalid key ignored while REQUIRE_API_KEY=false.
  • The existing "rejects an invalid presented API key" case now sets REQUIRE_API_KEY=true, which is the condition under which rejection is correct.

Local verification (on the rebased head)

  • image-generation-route*.test.ts — 24/24
  • call-log* + tests/unit/usage/** — 68/68
  • npm run typecheck:core — clean
  • eslint on all changed files — clean
  • check:file-size — [test-file-size] OK
  • pre-commit gates: check-docs-sync, check:any-budget:t11, check-tracked-artifacts — all pass

Known base-red, deliberately not touched

check:complexity-ratchets (2169 > 2130, cognitive 956 > 951) and two check:file-size freezes (dashboard/providers/page.tsx 1990 > 1927, lib/tokenHealthCheck.ts 843 > 841) fail identically on a clean origin/release/v3.8.49 checkout — verified by running both gates on the untouched base. They are inherited, not introduced here, and fixing them in this PR is exactly what produced the 19 conflicts last time.

@fenix007

Copy link
Copy Markdown
Contributor Author

CI on the rebased head: 4 red checks, all reproduced on an untouched base

Every failure on cf5535b also fails on a clean origin/release/v3.8.49 (30709255) checkout with no changes applied. Verified in a separate worktree off the base commit, same Node, same commands:

Unit Tests fast-path (1/4) + (2/4) — 4 failures, identical on base:

test file base this PR
502 transient: exponential backoff doubles until the configured max backoff step tests/unit/error-classification.test.ts ✖ ✖
high transient backoff levels clamp to the configured maxBackoffSteps tests/unit/error-classification.test.ts ✖ ✖
Exponential backoff clamps to the configured maxBackoffLevel tests/unit/thundering-herd.test.ts ✖ ✖
live repo: no NEW unexported db modules beyond the frozen allowlist tests/unit/check-db-rules.test.ts ✖ ✖
base   (30709255, clean): ℹ tests 55 · pass 51 · fail 4
this PR (cf5535b):        ℹ tests 55 · pass 51 · fail 4   ← same 4

These are the same three resilience regressions the dropped fix(resilience,translator) commit was patching; the base has not fixed them in the 91 commits since this branch forked.

No new ESLint warnings — base fails identically:

$ npm run lint:json -- --max-warnings 0      # on clean base
There are suppressions left that do not occur anymore.
Consider re-running the command with `--prune-suppressions`.
exit 2

config/quality/eslint-suppressions.json has no entry for any file this PR touches, so the staleness is not introduced here.

Fast Quality Gates — check:file-size, base fails identically:

✗ src/app/(dashboard)/dashboard/providers/page.tsx: 1990 > congelado 1927
✗ src/lib/tokenHealthCheck.ts: 843 > congelado 841
[test-file-size] OK

What this PR's own scope reports

  • tests/unit/image-generation-route*.test.ts — 24/24
  • call-log* + tests/unit/usage/** — 68/68
  • typecheck:core, eslint on changed files, check-docs-sync, check:any-budget:t11, check-tracked-artifacts — all clean
  • [test-file-size] OK (the 800-line cap that the unsplit test file would have crossed)

Ask

The branch is now MERGEABLE and its scope is green. Making these four checks green requires repairing the release base, which is what produced the 19-file conflict last time and is what #8307 covers — that PR is itself CONFLICTING right now. I'd rather not re-bundle the repair set here.

Happy to do either, your call: (a) merge/queue this on the understanding that the four reds are inherited, or (b) land the base repair first (as its own PR against release/v3.8.49) and I'll rebase this on top.

@fenix007

Copy link
Copy Markdown
Contributor Author

Opened #8561 — a dedicated repair PR against release/v3.8.49 for the inherited failures documented above, so they stop being this PR's problem.

It fixes three of the four reds at the source: the 3 stale backoff assertions (#8396 capped the cooldown and the tests were never updated), compressionDetailNormalizers missing from the db-rules allowlist, and the one stale ESLint suppression. A fourth, self-contained commit rebaselines the two inherited check:file-size growths (#8426, #8349 — both already merged, no branch left to fix), matching the precedent already recorded in that baseline today; drop that commit if you'd rather move the frozen values yourself.

The complexity ratchet is deliberately untouched — per c539d37b2 that's the captain's call and it's tracked by #8254 / #8256.

No production code is modified there: 2 test files, 1 allowlist entry, 2 quality baselines.

Once #8561 lands I'll rebase this PR on top; nothing here changes in the meantime.

@diegosouzapw
diegosouzapw merged commit 53f8284 into diegosouzapw:release/v3.8.49 Jul 28, 2026
5 checks passed
HouMinXi pushed a commit to HouMinXi/OmniRoute that referenced this pull request Aug 2, 2026
* fix(api): enforce image generation API key auth

* fix(api): align image route auth guard with clientApiPolicy

The route-level guard added for image generation was stricter than the
authz middleware that already fronts /api/v1/* (src/proxy.ts →
clientApiPolicy), so requests the pipeline admits were 401'd by the
handler:

- A cookie-authenticated dashboard session was rejected under
  REQUIRE_API_KEY=true. The dashboard Media page
  (dashboard/cache/media) and the Playground call these routes with a
  session and no Bearer — the same mismatch already fixed for
  /api/playground/presets.
- A presented invalid key was rejected even with REQUIRE_API_KEY=false,
  where clientApiPolicy (diegosouzapw#2257) and the sibling /v1/embeddings and
  /v1/web/fetch routes degrade a stale CLI key to anonymous instead.

Extract the shared guard into shared/utils/clientApiRouteAuth so both
image routes (and future /v1 handlers) mirror the middleware contract
instead of re-deriving it, and drop the now-dead auth imports.

Also switch the call-log attribution fallback back to `||`: with `??`,
an empty-string apiKeyId/apiKeyName would be persisted verbatim and
would block the request-scoped context, which the previous
`entry.apiKeyId || null` never did.

Tests: cover the dashboard-session and keyless-mode-invalid-key
branches, and split the auth/attribution cases into
image-generation-route-auth.test.ts to stay under the 800-line
new-test-file cap.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: alexey.nazarov@softmg.ru <alexey.nazarov@softmg.ru>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
* fix(api): enforce image generation API key auth

* fix(api): align image route auth guard with clientApiPolicy

The route-level guard added for image generation was stricter than the
authz middleware that already fronts /api/v1/* (src/proxy.ts →
clientApiPolicy), so requests the pipeline admits were 401'd by the
handler:

- A cookie-authenticated dashboard session was rejected under
  REQUIRE_API_KEY=true. The dashboard Media page
  (dashboard/cache/media) and the Playground call these routes with a
  session and no Bearer — the same mismatch already fixed for
  /api/playground/presets.
- A presented invalid key was rejected even with REQUIRE_API_KEY=false,
  where clientApiPolicy (diegosouzapw#2257) and the sibling /v1/embeddings and
  /v1/web/fetch routes degrade a stale CLI key to anonymous instead.

Extract the shared guard into shared/utils/clientApiRouteAuth so both
image routes (and future /v1 handlers) mirror the middleware contract
instead of re-deriving it, and drop the now-dead auth imports.

Also switch the call-log attribution fallback back to `||`: with `??`,
an empty-string apiKeyId/apiKeyName would be persisted verbatim and
would block the request-scoped context, which the previous
`entry.apiKeyId || null` never did.

Tests: cover the dashboard-session and keyless-mode-invalid-key
branches, and split the auth/attribution cases into
image-generation-route-auth.test.ts to stay under the 800-line
new-test-file cap.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: alexey.nazarov@softmg.ru <alexey.nazarov@softmg.ru>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.

2 participants