Skip to content

fix(api): hash API keys in the /v1/models catalog cache key - #10309

Open
RaviTharuma wants to merge 7 commits into
diegosouzapw:release/v3.8.50from
RaviTharuma:fix/catalog-cache-hash-apikey
Open

fix(api): hash API keys in the /v1/models catalog cache key#10309
RaviTharuma wants to merge 7 commits into
diegosouzapw:release/v3.8.50from
RaviTharuma:fix/catalog-cache-hash-apikey

Conversation

@RaviTharuma

Copy link
Copy Markdown
Contributor

Summary

  • buildCatalogCacheKey in catalogCache.ts put the raw API key into a process-wide Map key (prefix|codex|${apiKey}|configuredOnly).
  • Heap dumps, inspector, accidental logs leak bearer tokens.
  • Fingerprint with SHA-256 (16 hex chars). Empty key stays empty. Unit test asserts the raw secret never appears.

Related Issues

Validation

  • Production-code changes include a new or updated automated test in this PR
node --import tsx --test tests/unit/catalog-cache-auth-fingerprint.test.ts
# tests 1, pass 1

diegosouzapw and others added 6 commits August 8, 2026 00:08
…ouzapw#189, diegosouzapw#190)

Bumps: nanoid ^3.3.17 (was transitive, now overridden), dompurify ^3.4.13
(with monaco-editor scoped override). Closes Dependabot diegosouzapw#189, diegosouzapw#190.

Remaining diegosouzapw#182-diegosouzapw#188 (js-yaml + mermaid) already closed by diegosouzapw#9651 merge —
awaiting Dependabot re-scan.

npm audit → 0 vulnerabilities.
…egosouzapw#190

Closes Dependabot diegosouzapw#189 (dompurify 3.4.13) and diegosouzapw#190 (nanoid 3.3.17). npm audit → 0.
_tasks is a SEPARATE nested git repo (gitignored). The pattern _tasks/ (trailing
slash) ignores only a directory, not a SYMLINK named _tasks. A self-referential
_tasks symlink can slip in via git add -A and, once pulled, checkout materializes
it over the real _tasks repo (destroying plans/specs/hands-off). Anchored /_tasks
ignores the symlink too, preventing re-capture.
…pw#10026)

Mirror the request-time exclusion rule (provider_specific_data.excludedModels)
in the unified catalog builder: a model is hidden when its provider has
connections but none of them is eligible for it. Applied across the
PROVIDER_MODELS, synced, custom, alias-backed, and managed-fallback loops
so ghost models no longer appear as available.

Co-authored-by: ritheshcn25 <ritheshcn25@users.noreply.github.com>
…osouzapw#10055)

* fix(models): memoize getModelsDevPricing for /v1/models catalog

resolveCatalogPricing called getModelsDevPricing once per model while
building GET /v1/models. Each call re-scanned models_dev_pricing and
JSON.parsed every row (~10k SQL scans + multi-GB parse work), pegging
the event loop so even /healthz timed out (diegosouzapw#9685, diegosouzapw#10052).

Memoize the parsed map until saveModelsDevPricing / clearModelsDevPricing
and add a unit test for invalidation.

Signed-off-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>

* fix(db): invalidate modelsDevPricing cache on DB reset (diegosouzapw#10055)

Copilot review fixes:
1. Register invalidateModelsDevPricingCache() with DB state reset system
   so resetDbInstance() clears the process-local memo, preventing stale
   pricing data from surviving across DB reset/restore operations.
2. Add test assertion verifying DB reset bypasses the memo (Copilot diegosouzapw#10055).

The process-local memo at modelsDevSync.ts:204 caches getModelsDevPricing()
results until saveModelsDevPricing()/clearModelsDevPricing() to avoid
re-scanning all pricing rows on every /v1/models request. Without this hook,
backup restore and test DB resets would serve stale cached data from the
previous connection.

Tests: npm run test:unit:serial -- tests/unit/modelsDevSync-extended.test.ts

---------

Signed-off-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
Co-authored-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
buildCatalogCacheKey stored the raw bearer token in a process-wide Map.
Heap dumps and inspectors could leak it. Fingerprint with sha256.
@RaviTharuma

Copy link
Copy Markdown
Contributor Author

Implements / pairs with #10313.

@diegosouzapw
diegosouzapw changed the base branch from main to release/v3.8.50 August 14, 2026 19:27
@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks for this — the underlying fix is a real and worthwhile one. I checked and the raw-API-key-in-Map-key issue you describe in #10313 is still present on release/v3.8.50 tip, and I reproduced your fix independently (applied the same fingerprintCatalogAuthKey hash + rewiring on a clean checkout of current tip, copied in your test) — it passes cleanly.

The reason I'm not marking this merge-ready as-is: your branch looks like it stacked on top of your own #10026 and #10055 before they merged, and wasn't rebased afterward. Both of those already landed on release/v3.8.50 — but with different code than what's in this diff (the health-check-exclusion logic and the pricing memoization each shipped under a different implementation shape). So right now this PR's diff carries stale duplicate content for those two, plus two already-merged dependency bumps and the .gitignore hardening, and GitHub reports it as CONFLICTING/DIRTY as a result.

There's also a real trap hiding in there: buildCatalogCacheKey on current tip picked up two extra fields (hideAuto/hideNoThink) after your branch diverged. Your diff's version of that function only has 4 fields — so a naive conflict resolution could silently drop those.

Could you rebase (or just recreate the branch) onto current origin/release/v3.8.50 and keep only your actual new commit — the hash fix — reapplied against the current 6-field buildCatalogCacheKey? Once that's clean and the existing tests pass, this should be quick to merge. Don't worry about the "Build"/"Quality Ratchet" red X's you're seeing right now — I traced both and neither is caused by your code (one's an infra runner cancellation, the other is repo-wide CodeQL alert timing unrelated to catalogCache.ts); they should clear on the next CI run after rebase.

Nice catch on the original issue, by the way — that's a genuinely nasty little leak surface.

Resolve catalog cache key conflict by combining the API-key hash fingerprint
with the base's hideAuto/hideNoThink key segments; take base's version-checked
getModelsDevPricing memo and its restructured test file (base diegosouzapw#10055 supersedes
the older memo port); take base .gitignore and package-lock.

Co-authored-by: diegosouzapw <diegosouza.pw@gmail.com>
@diegosouzapw

Copy link
Copy Markdown
Owner

Babysit summary

The two failing checks (Build, Quality Ratchet) were stale verdicts, not this PR's defects.

  • Root cause: the PR head was 2280 commits behind release/v3.8.50. The base moved after this PR opened, so the last CI run (run 31748452259, CI workflow) was computed against a frozen pre-fix merge-ref — Build + Quality Ratchet reds are the classic base-drift signature. A base-green issue (#9985) is open, but its listed failures are unit/vitest/integration/package-artifact ceiling-kills on a scheduled run — none are this PR's gates.
  • Refresh-merge (0db8e4403a) merged the current release/v3.8.50 tip (774127b) into the PR head and resolved the conflicts:
    • src/app/api/v1/models/catalogCache.ts — combined the PR's API-key hash fingerprint with the base's hideAuto/hideNoThink cache-key segments (both behaviors preserved).
    • src/lib/modelsDevSync.ts + its test — took the base's version-checked getModelsDevPricing memo (base fix(models): memoize getModelsDevPricing (event loop / healthz) #10055 superseded the older memo port the branch carried).
    • .gitignore and package-lock.json — took base's current versions.
  • Fresh run (31882167647, Quality Gates workflow): all gates green — Build (advisory), Fast Quality Gates, No new ESLint warnings, Unit Tests fast-path (4/4), Vitest (fast-path), dast-smoke, semgrep.
  • Final state: PR is MERGEABLE / CLEAN; no pending checks; no unresolved review threads.

Ready for human review & merge.

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