Prevent caching OIDC redirects#1344
Conversation
2dcdbcf to
09b6c2b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09b6c2ba8d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .get_all(header::CACHE_CONTROL) | ||
| .filter_map(|value| value.to_str().ok()) |
There was a problem hiding this comment.
Call into_iter before filtering header values
When compiling this new OIDC test, HeaderMap::get_all(...) returns a GetAll collection rather than an Iterator, so chaining .filter_map(...) here fails with E0599. Add .iter() or .into_iter() before filter_map; otherwise the OIDC test target and CI cannot build.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65f2d5cefc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Fixes #1341.
Reproduction
The supplied Chrome HAR makes the loop deterministic without Android:
GET /is a303to Entra containing stateS. It hasCache-Control: max-age=86400and anExpiresvalue one day later.Ssucceeds, SQLPage removessqlpage_oidc_state_Sand redirects to/.303for/, rather than requesting SQLPage. It follows the old Entra authorization URL, which returns a fresh code but the same stateS./, returning to step 3.The HAR proves the cache reuse: entries at 22:52:41, 22:52:43, 22:52:44, and 22:52:45 all redirect to the identical authorization URL/state
Ohcyc0-izJdjD6uGl0zFkg, while their responseDateis 22:51:03. The final navigation sendsCache-Control: max-age=0, bypasses the stale redirect, and succeeds. The HAR is from desktop Chrome 150.I also reproduced this with a local app/IdP and a real Chrome 150 persistent profile: marking the state-bearing
303asmax-age=86400producesnet::ERR_TOO_MANY_REDIRECTS; changing only that header tono-storecompletes authentication normally.Fix
Send
Cache-Control: no-storeon OIDC authorization and post-login redirects. Deployments should also ensure Apache/CDN configuration does not overwrite this directive or cache redirects / responses withSet-Cookie.test_oidc_cached_authorization_redirect_cannot_replay_consumed_stateis an integration-level reproduction of the HAR sequence. It models the observed proxy-added freshness directive and Chrome private-cache behavior: it completes a valid login, reuses the cached state-bearing authorization redirect without replaying itsSet-Cookie, and obtains a callback with the now-consumed state. Before this change the test follows that callback to SQLPage's retry redirect and fails; withno-store, the browser model re-requests/with its auth cookie and receives200.Validation
cargo fmt --allcargo test --test mod oidc::cargo clippy --all-targets --all-features -- -D warnings