Skip to content

fix: address review findings on OIDC LLM forwarding (PR-2 follow-up) - #20

Open
imryao wants to merge 6 commits into
mainfrom
fix/oidc-followup
Open

fix: address review findings on OIDC LLM forwarding (PR-2 follow-up)#20
imryao wants to merge 6 commits into
mainfrom
fix/oidc-followup

Conversation

@imryao

@imryao imryao commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to #19 (OIDC LLM forwarding). Post-merge review identified 5 Critical findings and several Important test gaps. This PR addresses all 4 Critical findings that can be fixed at the server layer (the 5th, client-side AUTH_FAILED rendering, requires a separate frontend PR) plus the 2 critical test gaps that lock current correct behavior against future regression.

Commits

Fixes

Commit Finding What
9514399 C-5: DI fail-loud The 3 TS initializers (openai/anthropic/custom) used to silently skip OIDC override when refreshOIDCAccessToken was undefined. Any future caller dropping the DI param would silently leak env service key for OIDC users. Now throws.
7bbcb9a C-3: refresh error log fidelity refreshOIDCAccessToken's catch logged only error.message, collapsing invalid_grant / network / JWKS / 5xx into indistinguishable lines. Now logs structured discriminator fields (name/code/status/error_description/cause) while still avoiding error.response.data (would leak tokenset).
d819ab9 C-1: cookie de-sync OIDC_FORWARD_TO_LLM=true + OPENID_REUSE_TOKENS=true silently de-syncs the openid_user_id cookie on every LLM-side refresh (bridge passes res=null -> cookie writes skipped -> cookie expires while session still alive -> images break silently). Pragmatic fix: assertOIDCForwardingCompatible() now rejects the combination at boot. Structural fix (thread req.res through the bridge) deferred.
25feeec C-2: summarization OIDC bypass resolveSummarizationProvider in agents/run.ts built its own apiKey from customEndpointConfig.apiKey, bypassing initializeCustom and therefore the OIDC bearer override. With OIDC_FORWARD_TO_LLM=true, summarization sub-calls leaked env service key instead of user's OIDC JWT - gateway saw mixed identity for one conversation. Threads refreshOIDCAccessToken through createRun -> buildAgentContext -> resolveSummarizationProvider with same fail-loud + userProvidesURL guard pattern as F1. Also fixes a silent catch-block that was swallowing the new AUTH_FAILED throws.

Tests

Commit Finding What
c57e457 T-3 + T-4 T-3: inflightRefresh map keyed by user._id - locks in that two concurrent calls for DIFFERENT users trigger 2 refreshes producing 2 distinct tokens (no cross-contamination). T-4: req.session.openidTokens.expiresAt (ms, REFRESH_TOKEN_EXPIRY-relative) vs req.user.federatedTokens.expires_at (seconds, IdP exp claim) are deliberately different fields with different units - locks the asymmetry in with explicit assertions on both units and magnitude. A future "cleanup" PR that "unifies" them would silently break refresh leeway.
6ced954 T-1 + T-2 T-1: azureAssistants initializer had ZERO direct tests - its OIDC override updates BOTH apiKey AND opts.defaultHeaders['api-key'] (different SDK path from openai's clientOptions.azure.azureOpenAIApiKey). T-2: assistants initializer had ZERO direct tests despite carrying a security-critical userProvidesURL throw. Both now covered.

Deferred to PR-4 (frontend + polish)

  • C-4: client-side AUTH_FAILED rendering - touches client/ i18n keys; deserves its own PR
  • T-5: parameterize startup guard provider matrix (currently only google + local tested directly)
  • T-6: end-to-end nock test asserting bearer reaches the wire
  • N-1 through N-7: comment/documentation polish (stale line numbers, missing rationale comments, etc.)

Verification

  • All targeted tests pass:
    • packages/api PR-3 specs: 5 suites, 106 tests passed
    • api PR-3 specs: 5 suites, 85 tests passed
  • npm run lint: 93 errors / 53 warnings total, ZERO in any of the 17 files touched by this PR (all pre-existing in upstream-only files; baseline matches PR-1/PR-2)
  • npm run test:api (full suite): 155 passed / 3 failed suites - all 3 failures are infrastructure-dependent (Redis ECONNREFUSED on 127.0.0.1:6379, S3) and are pre-existing baseline failures in environments without Redis/S3
  • packages/api full suite: 228 passed / 21 failed suites - same root cause (cache_integration / stream_integration / redis_integration / s3 / mcp_integration tests all require Redis or S3 services); none touch PR-3 files

Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com

Generated with Claude Code

mryao and others added 6 commits June 17, 2026 17:27
mryao @claude
fix(endpoints): fail loud when refreshOIDCAccessToken DI missing in O…
9514399 
…IDC mode

Previously, the 3 TypeScript initializers (openai/anthropic/custom)
gated their OIDC override on '&& refreshOIDCAccessToken', so any future
caller that dropped the DI param would silently send the env service
key for OIDC users — a latent privilege boundary inversion with no log
signal. Changed to: when isLLMOIDCForwardingEnabled() and user is OIDC,
missing DI is an explicit error.

Adds 3 regression tests, one per initializer, asserting the throw.
Existing tests already inject the dep where required, so no other
test changes needed.

The 2 JS initializers (azureAssistants, assistants) require the bridge
directly via require() so this issue doesn't apply to them.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mryao @claude
fix(auth): preserve OIDC refresh error category in log output
7bbcb9a 
The previous logger.error call captured only error.message, collapsing
invalid_grant / invalid_client / network errors / JWKS failures / IdP
5xx into indistinguishable log lines. openid-client carries category
info on error.code / cause / response.status / error_description —
preserve them so production operators can distinguish 'IdP outage'
from 'expired refresh_token' from 'JWKS rotation' without re-deriving
context from request timestamps.

error.response.data is deliberately NOT logged — that field carries
the failed tokenset and would leak secrets.

Adds a regression test asserting the structured fields are present
AND that response.data contents never leak through any logger call.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mryao @claude
fix(startup): reject OIDC_FORWARD_TO_LLM + OPENID_REUSE_TOKENS combo
d819ab9 
The combination silently de-syncs the openid_user_id cookie: every
LLM-side token refresh (via refreshOIDCAccessToken bridge) passes
res=null to setOpenIDAuthTokens, which (correctly, per C2) skips
all res.cookie writes. The session-side expiry updates but the
cookie stays at the OLD expiry. Once the cookie expires, image
fetches silently fail while the user's session is still active.

Pragmatic fix: extend assertOIDCForwardingCompatible() to reject the
combination at boot, with a message pointing operators at the design
doc. The structural alternative (threading req.res through the
bridge so cookies CAN be written during refresh) is a larger change
deferred to a future PR.

Adds 2 regression tests + .env.example warning.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mryao @claude
fix(agents): forward OIDC bearer in cross-endpoint summarization
25feeec 
resolveSummarizationProvider built its own apiKey from
customEndpointConfig.apiKey (env-expanded) and passed it to
getOpenAIConfig directly, bypassing initializeCustom and therefore
the OIDC bearer override added in C4. With OIDC_FORWARD_TO_LLM=true,
summarization sub-calls leaked the env service key instead of the
user's OIDC JWT — gateway saw mixed identities for one conversation.

Plumbs refreshOIDCAccessToken through createRun → buildAgentContext →
resolveSummarizationProvider, gates on the same flag/provider check
the initializers use, and overrides apiKey with the OIDC accessToken.
Same fail-loud pattern as F1: missing DI in OIDC mode throws.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mryao @claude
test(auth): lock cross-user dedup isolation + session/user expiry fie…
c57e457 
…ld asymmetry

T-3: ensureLLMBearer's inflightRefresh map is keyed by String(req.user._id).
Add a regression test asserting two concurrent calls for DIFFERENT users
trigger 2 refreshes (not 1) and produce 2 distinct tokens (no cross-
contamination). A future regression that keys the map on shared input
(e.g., openidId truncation) would silently swap tokens between users —
a real security issue.

T-4: req.session.openidTokens.expiresAt (ms, REFRESH_TOKEN_EXPIRY relative)
vs req.user.federatedTokens.expires_at (seconds, IdP-issued exp claim) are
deliberately different fields with different units. A future cleanup PR
that 'unifies' them would silently break the 60s pre-refresh leeway check
in ensureLLMBearer. Lock the asymmetry in with explicit assertions on
units and magnitude.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mryao @claude
test(endpoints): add OIDC override specs for assistants + azureAssist…
6ced954 
…ants initializers

T-1 (azureAssistants): the JS initializer updates BOTH apiKey AND
opts.defaultHeaders['api-key'] — a different SDK path from the TS
openai initializer's clientOptions.azure.azureOpenAIApiKey. Without
a direct spec, regressions on either rewrite would be invisible.

T-2 (assistants): adds tests for the userProvidesURL security guard
(prevents OIDC bearer exfiltration to user-controlled baseURL). The
custom initializer has the same guard with its own test; this file
needs its own coverage because copy-paste regressions in either
direction would not surface in the other spec.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

This branch has not been deployed

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@imryao