Skip to content

fix(security): validate OIDC id_token claims and bind the login nonce - #431

Draft
mptfire wants to merge 1 commit into
LNVPS:masterfrom
mptfire:audit/oauth-idtoken-validation
Draft

mptfire wants to merge 1 commit into
LNVPS:masterfrom
mptfire:audit/oauth-idtoken-validation

Conversation

@mptfire

@mptfire mptfire commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Found by an independent security audit of the tree at e5d95b0 (2026-11-01).

Problem

The id_token was trusted straight after base64-decoding (decode_id_token): no exp, aud or iss checks. The OIDC nonce generated at login was carried inside the signed state but never sent to the provider and never checked on return. OIDC Core §3.1.3.7 keeps several claim checks mandatory even when the token arrives over the TLS-protected token-endpoint back channel (which is the documented reason the JWS signature is not re-verified here): issuer identifier match, audience check, expiry check — and the nonce check binds the response to the request.

Without these, a misconfigured or multi-tenant IdP (generic oidc providers) could hand back a token minted for another client/issuer, and a code-injected login could not be distinguished; exp-stale tokens from a provider glitch would be accepted.

Fix

  • validate_id_token_claims (in the IdToken subject-source path): exp must be in the future, aud must contain our client id (string or single-element array form), iss must equal the provider's expected issuer, and the nonce sent with the authorization request must be echoed by the token.
  • Expected issuers: canonical values for google / apple; new optional per-provider issuer setting for generic oidc providers (documented in docs/config.md). Providers without an id_token identity source (GitHub/Facebook userinfo flow) skip the iss check by design.
  • The login request now sends the OIDC nonce; verify_state_token returns it (alongside the provider tag and redirect) so the callback can compare.

Validation

  • 7 new unit tests: well-formed Apple token passes; expired / foreign-audience / wrong-issuer / missing-or-mismatched nonce each rejected; audience-array containing our client id passes; a provider with no known issuer and no nonce expectation is not over-constrained.
  • Existing oauth + session suites green (lnvps_api lib 16 oauth tests, lnvps_api_common 638).
  • Not included (deliberate follow-up): PKCE (S256) on the authorization-code flow — worth doing, but it changes the login state shape for every provider and deserves its own PR.

Audit reference: LNVM-013 (Low; hardening per OIDC Core §3.1.3.7 / RFC 9700).

The id_token was trusted after base64-decoding: no exp, aud or iss checks,
and the OIDC nonce generated at login was carried in the signed state but
never sent to the provider nor checked on return. OIDC Core 3.1.3.7 keeps
several claim checks mandatory even when the token arrives over the
token-endpoint back channel (which is why the JWS signature is not
re-verified here).

- id_token: exp must be current, aud must carry our client id (string or
  single-element array), iss must match the provider's issuer (canonical
  for google/apple; new optional per-provider issuer setting for generic
  oidc), and the nonce sent with the authorization request must come back.
- login now sends the nonce; verify_state_token returns it alongside the
  provider tag and redirect.
- docs/config.md documents the issuer setting; API_CHANGELOG.md records
  the behavior change.

Follow-up worth considering: PKCE (S256) on the authorization-code flow.
Found by the 2026-11-01 security audit.
@mptfire
mptfire force-pushed the audit/oauth-idtoken-validation branch from 6cc759d to e59a2aa Compare November 1, 2026 22:50
mptfire added a commit to mptfire/api that referenced this pull request Nov 2, 2026
Follow-up to the id_token hardening (LNVPS#431). Every authorization request now
carries a code_challenge and the token exchange presents the code_verifier
(RFC 7636, S256). The verifier rides the signed, short-lived state token,
so the flow stays stateless; states minted before PKCE simply carry no
verifier and the token request omits it.

Mitigates authorization-code interception (RFC 9700 section 4.6): a code
stolen from redirects, referrers or logs is useless without the verifier,
which never leaves the server and the provider's browser session.

docs/config.md documents the behavior; API_CHANGELOG.md records it.
Found by the 2026-11-01 security audit (LNVM-013 follow-up).

@v0l v0l left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validation logic and tests look right. A few things before merge:

  1. Google is not covered. expected_iss returns https://accounts.google.com for Google, but Google's subject_source() is userinfo, so validate_id_token_claims never runs for it and the nonce is never checked. Google also issues iss: accounts.google.com (no scheme) in some tokens; if you later route Google through the id_token path, accept both. Either drop Google from expected_iss or validate the id_token on the userinfo path too.
  2. aud array with azp. OIDC Core 3.1.3.7 step 4/5: when aud has multiple entries, azp should be present and equal the client id. Cheap to add while you're here.
  3. No clock skew allowance on exp. Apple tokens are 10 minutes; a worker a few seconds fast will reject at the edge. Allow ~60s.
  4. A login that fails validation returns 500 (ApiError::internal). That's a client/provider fault, so 400/401 would be more accurate and keeps it out of 5xx alerting.
  5. Doc comments on new items: we keep comments out of new code (docs/agents/code-style.md), please trim them.

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