fix: check the OIDC callback state against the session only #21

Merged

jfro merged 1 commit from jtk/oidc-state into main

2026-10-18 17:02:08 +00:00

jfro commented

2026-10-18 16:00:48 +00:00

Owner

Fixes #9 (high): when the session had no oidc_state, the callback fell back to params["state"] with a nil nonce, so Assent's state check compared the attacker's value with itself. That's login CSRF: a victim's browser can be signed in as the attacker's account.

Changes (`oidc_controller.ex`)

authorize/2 stores Assent's session_params once, under :oidc_session_params. (The old :oidc_state/:oidc_nonce writes read string keys from an atom-keyed map, so they were always nil, which is why the fallbacks ever ran.)
callback/2 reads and deletes :oidc_session_params. No stored state → warning log and "Authentication session expired" flash, never a fallback to the request.
Only the keys Assent uses (state, nonce, PKCE code_verifier/code_challenge/code_challenge_method) are mapped to atoms, through a fixed table. String.to_atom on session data is gone.

Tests

New test/fuzzy_catalog_web/controllers/oidc_controller_test.exs with a fixed openid_configuration, so no IdP is called: callback with no session state is refused; callback whose state doesn't match the session fails with Assent.CallbackCSRFError and clears the stored params; authorize stores the state that ends up in the redirect URL. mix precommit: 165 tests pass.

Worth a manual sign-in against the real IdP after deploy, since the session key changed. Sign-ins started before the deploy will get "session expired" once.

Fixes #9 (high): when the session had no `oidc_state`, the callback fell back to `params["state"]` with a nil nonce, so Assent's state check compared the attacker's value with itself. That's login CSRF: a victim's browser can be signed in as the attacker's account. ## Changes (`oidc_controller.ex`) - `authorize/2` stores Assent's `session_params` once, under `:oidc_session_params`. (The old `:oidc_state`/`:oidc_nonce` writes read string keys from an atom-keyed map, so they were always nil, which is why the fallbacks ever ran.) - `callback/2` reads and deletes `:oidc_session_params`. No stored state → warning log and "Authentication session expired" flash, never a fallback to the request. - Only the keys Assent uses (`state`, `nonce`, PKCE `code_verifier`/`code_challenge`/`code_challenge_method`) are mapped to atoms, through a fixed table. `String.to_atom` on session data is gone. ## Tests New `test/fuzzy_catalog_web/controllers/oidc_controller_test.exs` with a fixed `openid_configuration`, so no IdP is called: callback with no session state is refused; callback whose state doesn't match the session fails with `Assent.CallbackCSRFError` and clears the stored params; authorize stores the state that ends up in the redirect URL. `mix precommit`: 165 tests pass. Worth a manual sign-in against the real IdP after deploy, since the session key changed. Sign-ins started before the deploy will get "session expired" once.

jfro added 1 commit

2026-10-18 16:00:48 +00:00

fix: check the OIDC callback state against the session only

CI / test (pull_request) Successful in 35s

Details

c8ae0c304f

When the session had no stored state, the callback used the state from
the query string with a nil nonce, so Assent compared the attacker's
value with itself (login CSRF: a victim gets signed in as the attacker).

- authorize/2 stores Assent's session params once, in oidc_session_params
- callback/2 reads and clears them; no stored state means 'session
  expired', never a fallback to request params
- only known keys are turned into atoms (no String.to_atom on session
  data)

Fixes #9

jfro reviewed

2026-10-18 17:02:03 +00:00

jfro left a comment

Review by Nye (PR review / audit) — APPROVE @c8ae0c3

Closes #9: callback only reads :oidc_session_params from our session, deletes it on use, no params fallback; a fixed atom whitelist replaces String.to_atom. Verified against Assent 0.3.1 source: OAuth2.callback verify_state does a constant-time compare of session state vs param state and config doesn't set state: false, so a mismatch returns CallbackCSRFError before any token call. Also fixes a latent bug: Assent returns atom keys, so the old session_params["state"] was always nil. No conflicts with #17, #19 or #20. CI green.

Nits (non-blocking): lines ~147-148 still delete legacy :oidc_state/:oidc_nonce (harmless); no happy-path test (needs a token mock), fine for now.

**Review by Nye (PR review / audit) — APPROVE** @c8ae0c3 Closes #9: callback only reads `:oidc_session_params` from our session, deletes it on use, no params fallback; a fixed atom whitelist replaces `String.to_atom`. Verified against Assent 0.3.1 source: `OAuth2.callback` `verify_state` does a constant-time compare of session state vs param state and config doesn't set `state: false`, so a mismatch returns `CallbackCSRFError` before any token call. Also fixes a latent bug: Assent returns atom keys, so the old `session_params["state"]` was always nil. No conflicts with #17, #19 or #20. CI green. Nits (non-blocking): lines ~147-148 still delete legacy `:oidc_state`/`:oidc_nonce` (harmless); no happy-path test (needs a token mock), fine for now.

jfro merged commit 82c7cda67c into main

2026-10-18 17:02:08 +00:00

jfro deleted branch jtk/oidc-state

2026-10-18 17:02:08 +00:00

jfro referenced this pull request from a commit

2026-10-18 17:02:09 +00:00

fix: check the OIDC callback state against the session only (#21)

jfro referenced this pull request

2026-10-18 17:12:26 +00:00

fix: log disabled users out and keep them out #34