fix(oidc): fix OIDCAdapter broken flows

[enlightened88]

Fixed critical bugs in MemoryAdapter that broke grant revocation, device flow, and token consumption, missing grantId/userCode indexes in upsert, unsafe null …Fixed critical bugs in MemoryAdapter that broke grant revocation, device flow, and token consumption, missing grantId/userCode indexes in upsert, unsafe null assertions in destroy and consume, and a stale userCode mapping leak.

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Fix critical bugs in OIDCAdapter memory storage flows

🐞 Bug fix

Walkthroughs

Description

• Fixed unsafe null assertions in destroy() and consume() methods
• Added missing grantId and userCode index management in upsert()
• Fixed stale userCode mapping leak by deleting entries in destroy()
• Improved type safety and code formatting throughout adapter

Diagram

flowchart LR
  A["destroy method"] -->|"safely handle null"| B["Check grantId existence"]
  B -->|"delete userCode mapping"| C["Clean up storage"]
  D["consume method"] -->|"validate payload exists"| E["Update consumed timestamp"]
  F["upsert method"] -->|"index grantId"| G["Track grant tokens"]
  F -->|"index userCode"| H["Map userCode to id"]
  G -->|"enable"| I["revokeByGrantId"]
  H -->|"enable"| J["findByUserCode"]

Loading

File Changes

1. src/node/security/OIDCAdapter.ts 🐞 Bug fix +41/-40

Fix null safety and missing index management

• Fixed unsafe non-null assertions (!) in destroy() and consume() methods by adding proper
 null checks
• Added missing grantId index management in upsert() to track tokens by grant
• Added missing userCode index management in upsert() to enable findByUserCode() lookups
• Fixed stale userCode mapping leak by deleting entries when destroying tokens
• Improved code formatting and type annotations for consistency

src/node/security/OIDCAdapter.ts

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. No regression test added 📘 Rule violation ☼ Reliability

Description

This PR fixes multiple MemoryAdapter behaviors (grant revocation, userCode mapping, token
consumption) but does not include any new or updated automated test to prevent regression. Without a
regression test, these critical OIDC flows can break again unnoticed.

Code

src/node/security/OIDCAdapter.ts[R35-92]

Evidence

PR Compliance ID 1 requires a regression test for bug fixes. The diff shows substantive bug-fix
logic changes in destroy(), consume(), and upsert() but no accompanying test changes are
present in the provided PR diff.

src/node/security/OIDCAdapter.ts[35-92]
Best Practice: Repository guidelines

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR changes `MemoryAdapter` behavior to fix broken OIDC flows, but it does not add/update automated tests that would fail if these fixes were reverted.
## Issue Context
The changes modify `destroy()` (grant + userCode cleanup), `consume()` (null-safe consumed marking), and `upsert()` (grantId/userCode indexing) which are core to grant revocation, device flow userCode lookup, and token consumption correctness.
## Fix Focus Areas
- src/node/security/OIDCAdapter.ts[35-92]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Storage type too narrow ✓ Resolved 🐞 Bug ≡ Correctness

Description

storage is typed as AdapterPayload | string[], but upsert() stores a string id for the
userCode→id index, which will fail TypeScript checking (tsc --noEmit) and obscures the real stored
value types. This is introduced by narrowing the cache type union while leaving string writes in
place.

Code

src/node/security/OIDCAdapter.ts[15]

Evidence

The adapter stores a string id under the userCode: key, but the cache is no longer typed to
accept strings; this conflicts with the project’s TypeScript build gate (tsc --noEmit).

src/node/security/OIDCAdapter.ts[14-16]
src/node/security/OIDCAdapter.ts[78-93]
src/package.json[150-158]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`storage` is declared as `LRUCache<string, AdapterPayload | string[]>`, but the adapter stores string values for the userCode index (`storage.set(userCodeKeyFor(...), id)`). This creates a compile-time type error (and hides legitimate runtime values).
## Issue Context
The repo runs `ts-check` via `tsc --noEmit`, so this mismatch can block builds.
## Fix Focus Areas
- src/node/security/OIDCAdapter.ts[14-16]
- src/node/security/OIDCAdapter.ts[88-90]
- src/package.json[150-158]
## Suggested fix
- Update the cache value type to include `string` (e.g., `AdapterPayload | string[] | string`) and adjust `get()` casts accordingly.
- Optionally, tighten the API by creating separate caches (payload vs. indexes) to avoid widening unions, but the minimal change is adding `string` back.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Consume resets token TTL 🐞 Bug ⛨ Security

Description

consume() re-inserts the payload via storage.set(key, payload) without preserving the per-entry
TTL established by upsert(), so consumed records can fall back to the cache default TTL (5
minutes) instead of the configured token TTL (e.g., 10 minutes/1 hour). This can break replay
protection (consumed codes can disappear early) and cause OIDC flows to fail intermittently.

Code

src/node/security/OIDCAdapter.ts[R56-63]

Evidence

upsert() explicitly sets the payload TTL from expiresIn, but consume() now writes the same
entry without TTL while the cache has a 5-minute default TTL; the repo’s configured OIDC TTLs are
longer by default.

src/node/security/OIDCAdapter.ts[4-12]
src/node/security/OIDCAdapter.ts[56-63]
src/node/security/OIDCAdapter.ts[78-93]
src/node/utils/Settings.ts[427-433]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`consume()` updates `payload.consumed` and then calls `storage.set(key, payload)` without a TTL, even though `upsert()` sets a per-entry TTL (`expiresIn * 1000`). This can change the entry lifetime to the cache’s default TTL and shorten the window during which a consumed code/token remains marked as consumed.
## Issue Context
- Cache default TTL is 5 minutes.
- OIDC TTL defaults (Settings) are longer (AuthorizationCode 10 minutes, AccessToken 1 hour, etc.).
## Fix Focus Areas
- src/node/security/OIDCAdapter.ts[4-12]
- src/node/security/OIDCAdapter.ts[56-63]
- src/node/security/OIDCAdapter.ts[78-93]
- src/node/utils/Settings.ts[427-433]
## Suggested fix
- Avoid resetting the cache entry without TTL. Either:
1) Mutate in-place (as the old code did) and do not call `storage.set()` after updating `consumed`, or
2) Re-set with a TTL derived from the payload’s absolute expiry, e.g. `ttlMs = (payload.exp - epochTime()) * 1000` and `storage.set(key, payload, {ttl: ttlMs})`.
- If `ttlMs <= 0`, delete the entry instead of extending/shortening incorrectly.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (1)

4. Index entries expire early 🐞 Bug ≡ Correctness

Description

upsert() stores grantKey and userCode: index entries without a TTL, so they inherit the cache
default TTL (5 minutes) rather than the token/code expiresIn. This can make revokeByGrantId()
and findByUserCode() fail for still-valid records (default token TTLs are 10 minutes to 1 day).

Code

src/node/security/OIDCAdapter.ts[R81-90]

Evidence

The adapter’s index entries are stored without TTL while the payload uses expiresIn; because the
cache has a 5-minute default TTL and configured OIDC TTLs are longer, indexes can disappear while
tokens/codes remain valid, breaking lookup and revocation paths.

src/node/security/OIDCAdapter.ts[4-12]
src/node/security/OIDCAdapter.ts[78-93]
src/node/security/OIDCAdapter.ts[105-112]
src/node/utils/Settings.ts[427-433]
src/node/security/OAuth2Provider.ts[13-63]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new grantId and userCode indexes are written with `storage.set(grantKey, grant)` and `storage.set(userCodeKeyFor(...), id)` but without TTL, while the underlying payload is stored with `{ttl: expiresIn * 1000}`. This causes index entries to expire earlier than the payload, breaking lookup and revocation.
## Issue Context
- Cache default TTL is 5 minutes.
- OIDC TTL defaults are longer (AuthorizationCode 10 min, AccessToken 1h, RefreshToken 1 day).
## Fix Focus Areas
- src/node/security/OIDCAdapter.ts[4-12]
- src/node/security/OIDCAdapter.ts[81-93]
- src/node/security/OIDCAdapter.ts[105-112]
- src/node/utils/Settings.ts[427-433]
## Suggested fix
- Set TTL for index entries based on the same expiry as the payload (prefer using `payload.exp` to compute remaining TTL, or use `expiresIn * 1000`):
- `storage.set(grantKey, grant, {ttl: ttlMs})`
- `storage.set(userCodeKeyFor(payload.userCode), id, {ttl: ttlMs})`
- For `grantKey`, ensure the TTL does not shrink when additional tokens for the same grant are upserted (e.g., track max `exp` across tokens, or store `{tokens, exp}` under `grantKey`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[JohnMcLear]

Thanks for picking this up — the core fix is correct and overdue. The pre-fix revokeByGrantId was effectively a no-op (no grant index was ever written), meaning any "log out everywhere" / consent revocation flowing through this adapter silently did nothing. Device flow was similarly broken (findByUserCode had nothing to look up).

Strong points

• Writing the grant:<grantId> and userCode:<userCode> indexes in upsert is the load-bearing fix.
• Widening the upsert payload type to AdapterPayload is correct — the prior hand-written subset excluded grantId, userCode, consumed, etc.
• destroy now cleans the userCode mapping and tolerates a missing grant list.
• No more ! non-null assertions throwing on missing entries.

Requested changes before merge

1. consume resets the TTL to the cache default (5 min).

storage.set(key, payload);   // no ttl arg → falls back to constructor ttl

This silently overrides the expiresIn * 1000 TTL set by upsert. Either drop the storage.set entirely (LRUCache holds by reference, so the in-place payload.consumed = epochTime() is already persisted), or preserve the original expiry, e.g. storage.set(key, payload, {ttl: storage.getRemainingTTL(key)}).

2. Add regression tests.
The four bugs being fixed are exactly the kind that silently regress. Even a small spec covering:

• upsert → revokeByGrantId actually clears tokens
• upsert → findByUserCode returns the payload
• destroy clears the userCode mapping
• consume on a missing id doesn't throw

…would lock these in. Given this is OIDC, I'd really like to see this before merge.

Notes (not blockers)

• The grant list and userCode index are written without an explicit TTL, so they fall back to the 5-min default while the underlying token entry uses expiresIn * 1000. Matches the upstream panva/node-oidc-provider reference adapter, but worth a one-line comment noting the trade-off — a late revokeByGrantId can miss tokens whose grant index has already expired.
• findByUid is still an O(n) scan over the whole cache — not in scope here, but a uid:<uid> secondary index (mirroring what this PR just added for grantId) would be a worthwhile follow-up.
• Cosmetic whitespace / type-annotation cleanup is bundled with the fix. Fine, just noting it.

Nice catch on a real security-relevant bug. Approving in spirit pending the consume TTL nit and at least minimal test coverage.

[SamTV12345]

Thanks for the mr. That really fixed an issue as described by John ^^

[enlightened88]

Thanks for the mr. That really fixed an issue as described by John ^^

I am glad that I was useful to you