Repository navigation
🔐 fix: Keep the Admin OpenID State So PKCE Holds When the IdP Omits S256 - #17478
Open
danny-avila wants to merge 1 commit into
Open
danny-avila wants to merge 1 commit into
danny-avila wants to merge 1 commit into
Conversation
openid-client's Passport Strategy replaces the authorization request's state with a random one when the IdP's discovery metadata does not list S256 in code_challenge_methods_supported and the request has no nonce. Azure Entra ID omits that field, so the admin callback could not find the admin panel's PKCE challenge under pkce:<state> and issued an exchange code that any code_verifier, or none, could redeem. The admin strategy now adds a nonce in that case, so openid-client keeps the admin route's state and checks the nonce against the ID token on the callback. The chat OpenID flow is unchanged.
Collaborator
Author
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
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
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.
Pull Request
Summary
The admin panel's OpenID sign-in silently drops its PKCE check when the IdP's discovery document does not list
S256incode_challenge_methods_supported. Azure Entra ID is the common case: its discovery document omits the field.GET /api/admin/oauth/openidstores the panel'scode_challengeunderpkce:<state>and passes thatstateto theopenidAdminstrategy, but openid-client v6's Passport Strategy replaces the state with a random one wheneverserverMetadata().supportsPKCE()is false and the request carries nononce. The IdP returns the replaced state, so the callback finds no challenge, logsState present but no PKCE challenge found; PKCE will not be enforced for this request(the warning in LibreChat-AI/admin-panel#79's logs), and issues an exchange code thatPOST /api/admin/oauth/exchangeredeems with anycode_verifieror with none. Sign-in still succeeds, so the only sign of the problem is the warning.This PR gives admin OpenID sign-in its own strategy class. When the IdP does not advertise
S256and the request has no nonce yet, it adds a fresh nonce to the authorization request. openid-client then keeps the admin route's state, stores the nonce with the session's PKCE verifier and checks it against the ID token on the callback. The callback finds the panel's challenge, and the exchange code only redeems with the panel'scode_verifier. Nothing changes for an IdP that advertisesS256, for a request that already has a nonce (OPENID_GENERATE_NONCE=true, the existing workaround), or for the chat OpenID sign-in (/oauth/openid).Related to LibreChat-AI/admin-panel#79
How it works
openid-client's
authorizationRequest(openid-client/build/passport.js) only replaces the state when there is no nonce:setupOpenIdAdminnow builds anAdminOpenIDStrategy, a subclass ofCustomOpenIDStrategy. It adds the nonce throughpreserveAdminOAuthStateinpackages/api/src/auth/exchange.ts, next to the helper that stores the challenge:Type of change
Testing
Tested environments/configuration:
code_challenge_methods_supportedleft out (like Entra ID) and with['S256'](like Keycloak)OPENID_GENERATE_NONCEandOPENID_USE_PKCEunsetmain, served against that backendEach row is three admin sign-ins driven the way the panel drives them: start with a
code_challenge, follow the IdP, take the exchange code from the panel callback URL, then exchange it with a wrong verifier, without a verifier, and with the right one:code_challenge_methods_supportedcode_challenge_methods_supportedINVALID_OR_EXPIRED_CODEINVALID_OR_EXPIRED_CODE['S256']INVALID_OR_EXPIRED_CODEINVALID_OR_EXPIRED_CODEIn the admin panel, "Sign in with SSO" against the Entra-like provider on this branch goes from the backend to the IdP, back to the backend callback and on to the panel, which lands on its home page signed in as the OpenID user. The backend logs no warning. With either metadata, this branch's chat sign-in (
GET /oauth/openid) still sends the IdP no nonce.Automated tests:
api/server/routes/admin/auth.openid.test.js(new): runs the real admin router, Passport and the installed openid-client against a stub IdP behindundici.fetch, with metadata that has and lackscode_challenge_methods_supported. It checks that the state the admin challenge is stored under reaches the IdP, that the callback finds and consumes the challenge without the warning, that the exchange refuses a missing or wrongcode_verifierand issues the token for the right one, that a nonce is added only withoutS256, and that the chat sign-in request is unchanged. An Entra callback whose ID token leaves the nonce out, or carries another request's nonce, hands the admin panel no exchange code. WithsetupOpenIdAdminback onCustomOpenIDStrategy, 4 of its 10 tests fail.packages/api/src/auth/adminPkce.spec.ts:preserveAdminOAuthStateadds a nonce when the metadata lacksS256or lists onlyplain, and a fresh one for each request. It adds nothing when the metadata advertisesS256or the request has no state, and it keeps a nonce the request already has.npm run buildfor the packages,tscfor data-schemas and api, the admin and app suites inpackages/api, the app and config suites inpackages/data-schemas, the legacy admin routes,openidStrategy.spec.jsand the Config service suites, and the static checks pass.npm run lighthouse, the gate AGENTS.md asks for on auth changes, passes on this branch: median LCP 3,422 ms (budget 4,500), CLS 0.018 (0.1), TBT 129 ms (500). The change adds no database read, and its code only runs when an admin starts OpenID sign-in, which the Lighthouse scenario does not load.Screenshots / recordings
No user-facing change.
Risk / compatibility
Security-sensitive: this restores the PKCE binding between the admin panel's session and the exchange code for IdPs that do not advertise
S256. On those IdPs, admin sign-in now also needs the ID token to carry the nonce it was sent. OpenID Connect requires the IdP to return it, and Entra ID does. An IdP that omitsS256and also drops the nonce signed in on dev, without the PKCE check. It now fails the admin callback, as it already does withOPENID_GENERATE_NONCE=true. The admin sees the backend's generic error page (HTTP 500, "An unknown error occurred.", the existing handling for any openid-client callback error) rather than the admin panel'sauth_failedpage, and no setting turns this nonce off. Admin sign-in with IdPs that advertiseS256, and all chat OpenID sign-ins, behave as before.Known limit, kept for a follow-up: the callback still lets the exchange code through without a challenge when no challenge is cached for some other reason, such as an admin panel that sends no
code_challengeor a stay at the IdP longer thanPKCE_CHALLENGE_TTL(5 minutes). It logs the same warning, as on dev.Checklist