Repository navigation
test(deployment): rehearse OIDC upgrade on a persistent preview - #7424
Draft
snehasaisneha wants to merge 33 commits into
Draft
snehasaisneha wants to merge 33 commits into
snehasaisneha wants to merge 33 commits into
Conversation
… key (#7368) `consumeAuthRequest` looks up the stored login transaction with `store[state]`. `store` is built by `Object.fromEntries(...)` so it inherits from `Object.prototype`, and `state` is the raw value handed back on the OIDC callback, never validated. A callback carrying `state=constructor` — or `__proto__`, `toString`, `hasOwnProperty`, `valueOf` — therefore resolves to an inherited property, `transaction === undefined` is never true, and the server-side `state` check is skipped for those values. Nothing downstream catches it. The object that comes back has no `nonce` and no `codeVerifier`, and openid-client skips the nonce comparison when both the expected nonce and the ID token claim are absent, and omits `code_verifier` from the token request when it is undefined. So a callback with one of those state values is accepted as though this browser had started the login, with none of state, nonce or PKCE actually gating it. Whether that is reachable depends on the realm. A freshly imported realm sets `pkce.code.challenge.method: S256` on `backend-client`, and Keycloak then refuses to issue a code for an authorization request that carries no `code_challenge`, so the path is blocked. A realm upgraded in place is not protected: Keycloak skips re-importing a realm that already exists, so `backend-client` keeps `redirectUris: https://<host>/*` and gains no PKCE requirement. On those instances the chain can be walked end to end and an attacker-supplied callback URL makes the visitor's browser adopt the attacker's session — the login CSRF that `state` exists to prevent. Because the callback branch now runs before the cookie check, it can also replace the session of a user who is already signed in. The guard is `Object.prototype.hasOwnProperty.call(store, state)` rather than the more idiomatic `Object.hasOwn`, because `Object.hasOwn` is ES2022 and `website/tsconfig.json` pins `"lib": ["ES2021", "DOM"]`, so it fails `npm run check-types`. Raising that pin is worth doing separately; it is not needed here. This is the only place the transaction store is indexed by attacker-controlled input — `activeTransactions` iterates with `Object.entries`, which sees own enumerable keys only, and `addAuthRequest` indexes with a state it generated itself — so the single guard is sufficient. `delete store[state]` is left as it is, since deleting a key that is not an own property is a no-op. The added tests fail against the unguarded lookup (six failures) and pass with it, so they genuinely pin the behaviour rather than restating it. Other test gaps on this branch are untouched here: there is still no coverage for an oversized `returnTo`, for two logins started concurrently in different tabs, or for an already-authenticated visitor hitting the callback with an unknown state. https://claude.ai/code/session_01DWz25xM6t728H59XpfV2TB 🚀 Preview: Add `preview` label to enable --------- Co-authored-by: corneliusroemer-agent <cornelius.roemer+githubagent@gmail.com> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Sneha S <github@saisneha.com>
claude
Bot
added
the
deployment
10 tasks
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Temporary upgrade-test preview for #6994. Do not merge this PR into main.
The baseline starts from main at 87f6dc1 and changes only developmentDatabasePersistence to true. This disables values-driven recreation of the development databases; it does not attach persistent volumes or guarantee survival of pod replacement.
We will first verify the baseline Keycloak realm and browser login/logout, recording local screenshots and a test user. Only after that will state_nonce be merged into this preview branch, retaining the same database. We will then check that the original user and realm settings survive, apply the documented client-setting updates, and test login/logout and callback protections again.
This PR is for deployment verification, not a change to production defaults.
🚀 Preview: https://state-nonce-preview.loculus.org