[ES-2881] Added PKCE implementation in mock relying party.#555
Open
SajidMannikeri17 wants to merge 2 commits intomosip:developfrom
Open
[ES-2881] Added PKCE implementation in mock relying party.#555SajidMannikeri17 wants to merge 2 commits intomosip:developfrom
SajidMannikeri17 wants to merge 2 commits intomosip:developfrom
Conversation
Signed-off-by: Sajid Mannikeri <[email protected]>
WalkthroughAdds PKCE (code_challenge/code_verifier) support across the mock relying-party stack: environment flags, frontend PKCE utilities and storage, backend request-uri and token parameter forwarding, locale strings, and a React duplicate-call guard. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant FE as Frontend
participant BE as Backend
participant AS as AuthorizationServer
participant Token as TokenEndpoint
Browser->>FE: Click "Sign in"
FE->>FE: generateCodeVerifier()\nget_code_challenge()\nsessionStorage.set(code_verifier)
FE->>BE: get_requestUri(clientId, ..., code_challenge, method)
BE->>AS: POST /requestUri (includes code_challenge & method)
AS-->>BE: return request_uri
BE-->>FE: return request_uri
FE->>AS: Redirect user to authorization endpoint (request_uri)
AS-->>Browser: Authorization code (after user consent)
Browser->>FE: Callback with code
FE->>FE: retrieve code_verifier from sessionStorage
FE->>BE: post_fetchUserInfo(code, code_verifier)
BE->>Token: POST /token (include code_verifier)
Token->>Token: verify code_verifier vs code_challenge
Token-->>BE: access token
BE-->>FE: user info / token result
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
mock-relying-party-ui/src/constants/clientDetails.js (1)
62-62:code_challengestores a callback function name, not the PKCE value — naming diverges from the established convention.
par_callback_nameanddpop_callback_nameboth use the_callback_namesuffix to signal that the value is a function reference. The field stores"get_code_challenge"(a callback name), but is namedcode_challenge— the same term used in the OAuth/OIDC spec for the actualBASE64URL(SHA256(code_verifier))hash. This divergence invites confusion.Consider renaming to
code_challenge_callback_namefor consistency, unless the eSignet sign-in button plugin contract specifically requires the property to be namedcode_challenge.♻️ Proposed rename if plugin API allows
-const code_challenge = window._env_.CODE_CHALLENGE; +const code_challenge_callback_name = window._env_.CODE_CHALLENGE;- code_challenge: code_challenge, + code_challenge_callback_name: code_challenge_callback_name,Also applies to: 112-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mock-relying-party-ui/src/constants/clientDetails.js` at line 62, The constant code_challenge is actually holding a callback function name (e.g., "get_code_challenge") which conflicts with OAuth terminology; rename it to code_challenge_callback_name for consistency with par_callback_name and dpop_callback_name (and update any usages that read window._env_.CODE_CHALLENGE to use the new identifier). If the external eSignet plugin API requires the exact property name, instead add an inline comment clarifying that code_challenge contains a callback name and mirror the par/dpop naming where possible; ensure you update references in clientDetails.js and any components that consume code_challenge/code_challenge_callback_name (search for code_challenge) to avoid breakage.mock-relying-party-ui/src/services/relyingPartyService.js (2)
52-58: Dead code:get_code_challenge_methodalways returns a truthy string.The
if (!method)check on line 55 can never be true becauseget_code_challenge_methodreturns either a method from the array or the fallback'S256'. This block is unreachable.♻️ Proposed cleanup
const get_code_challenge = async (clientId, state) => { const method = await get_code_challenge_method(); - - if (!method) { - console.warn('PKCE disabled: no supported method found'); - return null; - } - const codeVerifier = generateCodeVerifier();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mock-relying-party-ui/src/services/relyingPartyService.js` around lines 52 - 58, The if-statement checking method truthiness in get_code_challenge is dead because get_code_challenge_method always returns a non-empty string; remove the unreachable block (the console.warn and return null) and simplify get_code_challenge to assume a valid method from get_code_challenge_method, keeping logic that uses method thereafter (reference functions: get_code_challenge and get_code_challenge_method).
34-44: Blindly pickingmethods[0]may selectplainoverS256.RFC 7636 Section 4.2 recommends
S256unless the client cannot perform SHA-256. If the server returns["plain", "S256"], this code would pickplain. PreferS256when available.♻️ Proposed fix
const methods = response.data?.code_challenge_methods_supported; - return methods && methods.length > 0 ? methods[0] : 'S256'; + if (methods && methods.length > 0) { + return methods.includes('S256') ? 'S256' : methods[0]; + } + return 'S256';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mock-relying-party-ui/src/services/relyingPartyService.js` around lines 34 - 44, The current get_code_challenge_method implementation may return methods[0] which can pick "plain" when the server lists ["plain","S256"]; update get_code_challenge_method to prefer "S256" when present in the returned methods array and only fall back to "plain" if S256 is not available (and finally default to 'S256' on errors); locate the function get_code_challenge_method and the variable methods (response.data?.code_challenge_methods_supported) and change the selection logic to check for methods.includes('S256') first, then methods.includes('plain'), then default to 'S256'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mock-relying-party-service/esignetService.js`:
- Around line 118-121: The current block appends code_challenge_method even when
it may be undefined causing "undefined" to appear; update the logic around
params.append to require both values: when code_challenge is present ensure
code_challenge_method is non-null/undefined (or set a safe default like "S256")
before calling params.append("code_challenge_method", ...), and if it's missing
either omit appending the method or throw/return an explicit error; locate and
change the check that references code_challenge, code_challenge_method, and
params.append in esignetService.js.
In `@mock-relying-party-ui/README.md`:
- Line 59: The docker run command at the end incorrectly includes a stray "-e"
before the image placeholder ("<dockerImageName>:<tag>"); remove that "-e" so
the image name is passed as the final argument to docker run rather than being
treated as an environment variable flag, leaving the rest of the -e environment
variables (ESIGNET_UI_BASE_URL, MOCK_RELYING_PARTY_BASE_URL, REDIRECT_URI,
CLIENT_ID, ACRS, MAX_AGE, DISPLAY, PROMPT, GRANT_TYPE,
SIGN_IN_BUTTON_PLUGIN_URL, SCOPE_USER_PROFILE, PAR_CALLBACK_NAME,
DPOP_CALLBACK_NAME, CODE_CHALLENGE) unchanged.
In `@mock-relying-party-ui/src/services/relyingPartyService.js`:
- Around line 113-121: The get_requestUri function currently builds
URLSearchParams with possibly undefined values which become the string
"undefined"; change the params construction to add keys only when their values
are non-null/defined (e.g. push state and ui_locales always, but only append
dpop_jkt, code_challenge, and code_challenge_method when they are
truthy/non-undefined). Update any existing guard logic that checks dpop_jkt to
rely on actual presence (not string checks) so the backend won't receive
"undefined" strings; locate and modify the get_requestUri function and the
URLSearchParams usage to conditionally set parameters instead of passing
potentially undefined values.
---
Nitpick comments:
In `@mock-relying-party-ui/src/constants/clientDetails.js`:
- Line 62: The constant code_challenge is actually holding a callback function
name (e.g., "get_code_challenge") which conflicts with OAuth terminology; rename
it to code_challenge_callback_name for consistency with par_callback_name and
dpop_callback_name (and update any usages that read window._env_.CODE_CHALLENGE
to use the new identifier). If the external eSignet plugin API requires the
exact property name, instead add an inline comment clarifying that
code_challenge contains a callback name and mirror the par/dpop naming where
possible; ensure you update references in clientDetails.js and any components
that consume code_challenge/code_challenge_callback_name (search for
code_challenge) to avoid breakage.
In `@mock-relying-party-ui/src/services/relyingPartyService.js`:
- Around line 52-58: The if-statement checking method truthiness in
get_code_challenge is dead because get_code_challenge_method always returns a
non-empty string; remove the unreachable block (the console.warn and return
null) and simplify get_code_challenge to assume a valid method from
get_code_challenge_method, keeping logic that uses method thereafter (reference
functions: get_code_challenge and get_code_challenge_method).
- Around line 34-44: The current get_code_challenge_method implementation may
return methods[0] which can pick "plain" when the server lists ["plain","S256"];
update get_code_challenge_method to prefer "S256" when present in the returned
methods array and only fall back to "plain" if S256 is not available (and
finally default to 'S256' on errors); locate the function
get_code_challenge_method and the variable methods
(response.data?.code_challenge_methods_supported) and change the selection logic
to check for methods.includes('S256') first, then methods.includes('plain'),
then default to 'S256'.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
docker-compose/mock-relying-party-portal-fapi2-docker-compose.ymlmock-relying-party-service/app.jsmock-relying-party-service/esignetService.jsmock-relying-party-ui/Dockerfilemock-relying-party-ui/README.mdmock-relying-party-ui/public/env-config.jsmock-relying-party-ui/public/locales/ar.jsonmock-relying-party-ui/public/locales/en.jsonmock-relying-party-ui/public/locales/hi.jsonmock-relying-party-ui/public/locales/km.jsonmock-relying-party-ui/public/locales/kn.jsonmock-relying-party-ui/public/locales/ta.jsonmock-relying-party-ui/src/components/Login.jsmock-relying-party-ui/src/components/Sidenav.jsmock-relying-party-ui/src/constants/clientDetails.jsmock-relying-party-ui/src/services/relyingPartyService.js
Signed-off-by: Sajid Mannikeri <[email protected]>
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mock-relying-party-ui/src/services/relyingPartyService.js (1)
55-58: Dead code: the!methodguard is unreachable.
get_code_challenge_methodalways returns a non-empty string (methods[0]or'S256'as fallback), somethodcan never be falsy here and this early return is never executed. The comment "PKCE disabled" is also misleading.♻️ Proposed cleanup
- if (!method) { - console.warn('PKCE disabled: no supported method found'); - return null; - } - const codeVerifier = generateCodeVerifier();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mock-relying-party-ui/src/services/relyingPartyService.js` around lines 55 - 58, Remove the unreachable PKCE-disabled guard and misleading log: the check if (!method) { console.warn('PKCE disabled: no supported method found'); return null; } is dead because get_code_challenge_method always returns a non-empty string. Delete that conditional (the if (!method) block) in relyingPartyService.js and rely on the method value returned by get_code_challenge_method; keep any downstream logic that assumes a valid method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mock-relying-party-ui/src/services/relyingPartyService.js`:
- Around line 34-44: The get_code_challenge_method function currently returns
methods[0], which can silently pick 'plain' if the server lists it first;
instead, inspect response.data?.code_challenge_methods_supported and return
'S256' if that array includes 'S256', otherwise fall back to 'plain' only if
explicitly present, and finally default to 'S256' on error or if no known
methods; update the logic in get_code_challenge_method to prefer 'S256' first,
then 'plain' if present, using the existing axios call and
response.data?.code_challenge_methods_supported reference.
---
Nitpick comments:
In `@mock-relying-party-ui/src/services/relyingPartyService.js`:
- Around line 55-58: Remove the unreachable PKCE-disabled guard and misleading
log: the check if (!method) { console.warn('PKCE disabled: no supported method
found'); return null; } is dead because get_code_challenge_method always returns
a non-empty string. Delete that conditional (the if (!method) block) in
relyingPartyService.js and rely on the method value returned by
get_code_challenge_method; keep any downstream logic that assumes a valid
method.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
mock-relying-party-service/esignetService.jsmock-relying-party-ui/README.mdmock-relying-party-ui/src/services/relyingPartyService.js
🚧 Files skipped from review as they are similar to previous changes (1)
- mock-relying-party-service/esignetService.js
Comment on lines
+34
to
+44
| const get_code_challenge_method = async () => { | ||
| try { | ||
| const baseUrl = window._env_.ESIGNET_UI_BASE_URL; | ||
| const response = await axios.get(`${baseUrl}/.well-known/openid-configuration`); | ||
| const methods = response.data?.code_challenge_methods_supported; | ||
| return methods && methods.length > 0 ? methods[0] : 'S256'; | ||
| } catch (error) { | ||
| console.warn('Failed to fetch PKCE methods, defaulting to S256:', error); | ||
| return 'S256'; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Insecure: methods[0] is order-dependent and can silently downgrade to 'plain'.
RFC 7636 §7 mandates that "If the client is capable of using 'S256', it MUST use 'S256'"; clients are permitted to use 'plain' only if they cannot support 'S256' for some technical reason. If the authorization server advertises ['plain', 'S256'], the current code blindly returns 'plain'. Servers that support PKCE are required to support 'S256', so S256 should always be tried first.
🔒 Proposed fix: prefer S256 when available
- return methods && methods.length > 0 ? methods[0] : 'S256';
+ if (!methods || methods.length === 0) return 'S256';
+ return methods.includes('S256') ? 'S256' : methods[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mock-relying-party-ui/src/services/relyingPartyService.js` around lines 34 -
44, The get_code_challenge_method function currently returns methods[0], which
can silently pick 'plain' if the server lists it first; instead, inspect
response.data?.code_challenge_methods_supported and return 'S256' if that array
includes 'S256', otherwise fall back to 'plain' only if explicitly present, and
finally default to 'S256' on error or if no known methods; update the logic in
get_code_challenge_method to prefer 'S256' first, then 'plain' if present, using
the existing axios call and response.data?.code_challenge_methods_supported
reference.
Contributor
There was a problem hiding this comment.
@SajidMannikeri17 do one thing in this point, check if S256 is present then use the same otherwise use plain then
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
zesu22
requested changes
Feb 24, 2026
Contributor
zesu22
left a comment
zesu22
left a comment
There was a problem hiding this comment.
please resolve review comment
| const getSearchParams = async () => { | ||
| // Prevent duplicate API calls during React.StrictMode double mounting | ||
| if (hasFetchedRef.current) return; | ||
| hasFetchedRef.current = true; |
Comment on lines
+34
to
+44
| const get_code_challenge_method = async () => { | ||
| try { | ||
| const baseUrl = window._env_.ESIGNET_UI_BASE_URL; | ||
| const response = await axios.get(`${baseUrl}/.well-known/openid-configuration`); | ||
| const methods = response.data?.code_challenge_methods_supported; | ||
| return methods && methods.length > 0 ? methods[0] : 'S256'; | ||
| } catch (error) { | ||
| console.warn('Failed to fetch PKCE methods, defaulting to S256:', error); | ||
| return 'S256'; | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
@SajidMannikeri17 do one thing in this point, check if S256 is present then use the same otherwise use plain then
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
2 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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation