🔐 fix: Strip code_challenge from Admin OAuth requests before Passport#12534
Merged
Conversation
openid-client v6's Passport Strategy uses `currentUrl.searchParams.size === 0` to distinguish initial authorization requests from OAuth callbacks. The admin-panel-specific `code_challenge` query parameter caused the strategy to misclassify the request as a callback and return 401 Unauthorized.
danny-avila
changed the title
code_challenge from Admin OAuth requests before Passport
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes admin OAuth initiation requests being misclassified as OAuth callbacks by Passport/openid-client v6 when the admin panel adds a code_challenge query parameter.
Changes:
- Added a helper to strip
code_challengefrom the Express request query/URL before invoking Passport strategies. - Applied the helper across all admin OAuth initiation routes (OpenID, SAML, Google, GitHub, Discord, Facebook, Apple).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+91
to
+93
| delete req.query.code_challenge; | ||
| req.originalUrl = req.originalUrl.replace(/[?&]code_challenge=[^&]+/, ''); | ||
| req.url = req.url.replace(/[?&]code_challenge=[^&]+/, ''); |
There was a problem hiding this comment.
This replace() can yield an invalid URL when code_challenge is the first query param but others follow (e.g. ?code_challenge=...&foo=1 becomes &foo=1). Prefer rebuilding req.originalUrl via URL/URLSearchParams (delete the param, then serialize) so separators (?/&) stay correct.
Suggested change
| delete req.query.code_challenge; | |
| req.originalUrl = req.originalUrl.replace(/[?&]code_challenge=[^&]+/, ''); | |
| req.url = req.url.replace(/[?&]code_challenge=[^&]+/, ''); | |
| const stripCodeChallengeFromUrl = (value) => { | |
| const parsedUrl = new URL(value, 'http://localhost'); | |
| parsedUrl.searchParams.delete('code_challenge'); | |
| return `${parsedUrl.pathname}${parsedUrl.search}${parsedUrl.hash}`; | |
| }; | |
| delete req.query.code_challenge; | |
| req.originalUrl = stripCodeChallengeFromUrl(req.originalUrl); | |
| req.url = stripCodeChallengeFromUrl(req.url); |
Comment on lines
+91
to
+93
| delete req.query.code_challenge; | ||
| req.originalUrl = req.originalUrl.replace(/[?&]code_challenge=[^&]+/, ''); | ||
| req.url = req.url.replace(/[?&]code_challenge=[^&]+/, ''); |
There was a problem hiding this comment.
Same issue as above for req.url: stripping ?code_challenge=... via regex can leave a leading & when other params follow, producing a malformed URL string. Use URLSearchParams deletion + re-serialization (or equivalent) rather than a single regex replace.
Suggested change
| delete req.query.code_challenge; | |
| req.originalUrl = req.originalUrl.replace(/[?&]code_challenge=[^&]+/, ''); | |
| req.url = req.url.replace(/[?&]code_challenge=[^&]+/, ''); | |
| const removeCodeChallengeFromUrl = (url) => { | |
| const hashIndex = url.indexOf('#'); | |
| const beforeHash = hashIndex === -1 ? url : url.slice(0, hashIndex); | |
| const hash = hashIndex === -1 ? '' : url.slice(hashIndex); | |
| const queryIndex = beforeHash.indexOf('?'); | |
| if (queryIndex === -1) { | |
| return url; | |
| } | |
| const path = beforeHash.slice(0, queryIndex); | |
| const search = beforeHash.slice(queryIndex + 1); | |
| const searchParams = new URLSearchParams(search); | |
| searchParams.delete('code_challenge'); | |
| const serializedSearch = searchParams.toString(); | |
| return `${path}${serializedSearch ? `?${serializedSearch}` : ''}${hash}`; | |
| }; | |
| delete req.query.code_challenge; | |
| req.originalUrl = removeCodeChallengeFromUrl(req.originalUrl); | |
| req.url = removeCodeChallengeFromUrl(req.url); |
openid-client v6's Passport Strategy uses `currentUrl.searchParams.size === 0` to distinguish initial authorization requests from OAuth callbacks. The admin-panel-specific `code_challenge` query parameter caused the strategy to misclassify the request as a callback and return 401 Unauthorized. - Fix regex to handle `code_challenge` in any query position without producing malformed URLs, and handle empty `code_challenge=` values (`[^&]*` vs `[^&]+`) - Combine `storePkceChallenge` + `stripCodeChallenge` into a single `storeAndStripChallenge` helper to enforce read-store-strip ordering - Apply defensively to all 7 admin OAuth providers - Add 12 unit tests covering stripCodeChallenge and storeAndStripChallenge
- Move stripCodeChallenge and storeAndStripChallenge to api/server/utils/adminPkce.js — eliminates _test production export and avoids loading the full auth.js module tree in tests - Add missing req.originalUrl/req.url assertions to invalid-challenge and no-challenge test branches (regression blind spots) - Hoist cache reference to module scope in tests (was redundantly re-acquired from mock factory on every beforeEach)
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ 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". |
- Fix import order in auth.js (longest-to-shortest per CLAUDE.md) - Remove unused PKCE_CHALLENGE_TTL/PKCE_CHALLENGE_PATTERN exports - Hoist strip arrow to module-scope stripChallengeFromUrl - Rename auth.test.js → auth.spec.js (project convention) - Tighten cache-failure test: toBe instead of toContain, add req.url
Move stripCodeChallenge and storeAndStripChallenge from api/server/utils into packages/api/src/auth/exchange.ts alongside the existing PKCE verification logic. Cache is now injected as a Keyv parameter, matching the dependency-injection pattern used throughout packages/api/. - Add PkceStrippableRequest interface for minimal req typing - auth.js imports storeAndStripChallenge from @librechat/api - Delete api/server/utils/adminPkce.js - Move tests to packages/api/src/auth/adminPkce.spec.ts (TypeScript, real Keyv instances, no getLogStores mock needed)
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ 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". |
jcbartle
pushed a commit
to jcbartle/LibreChat
that referenced
this pull request
May 11, 2026
…rt (danny-avila#12534) * 🔐 fix: Strip code_challenge from admin OAuth requests before Passport openid-client v6's Passport Strategy uses `currentUrl.searchParams.size === 0` to distinguish initial authorization requests from OAuth callbacks. The admin-panel-specific `code_challenge` query parameter caused the strategy to misclassify the request as a callback and return 401 Unauthorized. * 🔐 fix: Strip code_challenge from admin OAuth requests before Passport openid-client v6's Passport Strategy uses `currentUrl.searchParams.size === 0` to distinguish initial authorization requests from OAuth callbacks. The admin-panel-specific `code_challenge` query parameter caused the strategy to misclassify the request as a callback and return 401 Unauthorized. - Fix regex to handle `code_challenge` in any query position without producing malformed URLs, and handle empty `code_challenge=` values (`[^&]*` vs `[^&]+`) - Combine `storePkceChallenge` + `stripCodeChallenge` into a single `storeAndStripChallenge` helper to enforce read-store-strip ordering - Apply defensively to all 7 admin OAuth providers - Add 12 unit tests covering stripCodeChallenge and storeAndStripChallenge * refactor: Extract PKCE helpers to utility file, harden tests - Move stripCodeChallenge and storeAndStripChallenge to api/server/utils/adminPkce.js — eliminates _test production export and avoids loading the full auth.js module tree in tests - Add missing req.originalUrl/req.url assertions to invalid-challenge and no-challenge test branches (regression blind spots) - Hoist cache reference to module scope in tests (was redundantly re-acquired from mock factory on every beforeEach) * chore: Address review NITs — imports, exports, naming, assertions - Fix import order in auth.js (longest-to-shortest per CLAUDE.md) - Remove unused PKCE_CHALLENGE_TTL/PKCE_CHALLENGE_PATTERN exports - Hoist strip arrow to module-scope stripChallengeFromUrl - Rename auth.test.js → auth.spec.js (project convention) - Tighten cache-failure test: toBe instead of toContain, add req.url * refactor: Move PKCE helpers to packages/api with dependency injection Move stripCodeChallenge and storeAndStripChallenge from api/server/utils into packages/api/src/auth/exchange.ts alongside the existing PKCE verification logic. Cache is now injected as a Keyv parameter, matching the dependency-injection pattern used throughout packages/api/. - Add PkceStrippableRequest interface for minimal req typing - auth.js imports storeAndStripChallenge from @librechat/api - Delete api/server/utils/adminPkce.js - Move tests to packages/api/src/auth/adminPkce.spec.ts (TypeScript, real Keyv instances, no getLogStores mock needed)
ThomasVuNguyen
pushed a commit
to ThomasVuNguyen/LibreChat
that referenced
this pull request
Jul 15, 2026
…rt (danny-avila#12534) * 🔐 fix: Strip code_challenge from admin OAuth requests before Passport openid-client v6's Passport Strategy uses `currentUrl.searchParams.size === 0` to distinguish initial authorization requests from OAuth callbacks. The admin-panel-specific `code_challenge` query parameter caused the strategy to misclassify the request as a callback and return 401 Unauthorized. * 🔐 fix: Strip code_challenge from admin OAuth requests before Passport openid-client v6's Passport Strategy uses `currentUrl.searchParams.size === 0` to distinguish initial authorization requests from OAuth callbacks. The admin-panel-specific `code_challenge` query parameter caused the strategy to misclassify the request as a callback and return 401 Unauthorized. - Fix regex to handle `code_challenge` in any query position without producing malformed URLs, and handle empty `code_challenge=` values (`[^&]*` vs `[^&]+`) - Combine `storePkceChallenge` + `stripCodeChallenge` into a single `storeAndStripChallenge` helper to enforce read-store-strip ordering - Apply defensively to all 7 admin OAuth providers - Add 12 unit tests covering stripCodeChallenge and storeAndStripChallenge * refactor: Extract PKCE helpers to utility file, harden tests - Move stripCodeChallenge and storeAndStripChallenge to api/server/utils/adminPkce.js — eliminates _test production export and avoids loading the full auth.js module tree in tests - Add missing req.originalUrl/req.url assertions to invalid-challenge and no-challenge test branches (regression blind spots) - Hoist cache reference to module scope in tests (was redundantly re-acquired from mock factory on every beforeEach) * chore: Address review NITs — imports, exports, naming, assertions - Fix import order in auth.js (longest-to-shortest per CLAUDE.md) - Remove unused PKCE_CHALLENGE_TTL/PKCE_CHALLENGE_PATTERN exports - Hoist strip arrow to module-scope stripChallengeFromUrl - Rename auth.test.js → auth.spec.js (project convention) - Tighten cache-failure test: toBe instead of toContain, add req.url * refactor: Move PKCE helpers to packages/api with dependency injection Move stripCodeChallenge and storeAndStripChallenge from api/server/utils into packages/api/src/auth/exchange.ts alongside the existing PKCE verification logic. Cache is now injected as a Keyv parameter, matching the dependency-injection pattern used throughout packages/api/. - Add PkceStrippableRequest interface for minimal req typing - auth.js imports storeAndStripChallenge from @librechat/api - Delete api/server/utils/adminPkce.js - Move tests to packages/api/src/auth/adminPkce.spec.ts (TypeScript, real Keyv instances, no getLogStores mock needed)
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.
openid-client v6's Passport Strategy uses
currentUrl.searchParams.size === 0to distinguish initial authorization requests from OAuth callbacks. The admin-panel-specificcode_challengequery parameter caused the strategy to misclassify the request as a callback and return 401 Unauthorized.