[ES-2881] Added PKCE implementation in mock relying party. - #555
Conversation
Signed-off-by: Sajid Mannikeri <sajid.mannikeri@ad.infosys.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds PKCE support: environment flag, frontend PKCE utilities and storage, UI changes to include code_challenge, backend forwarding of PKCE params (request URI and token exchange), locale entries for errors, and a React guard to prevent duplicate API calls. Changes
Sequence Diagram(s)sequenceDiagram
participant 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=...&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: redirect back with authorization code
Browser->>FE: Callback with code
FE->>FE: retrieve code_verifier from sessionStorage
FE->>BE: POST /fetchUserInfo (include code, code_verifier)
BE->>Token: POST /token (include code_verifier)
Token->>Token: verify code_verifier vs code_challenge
Token-->>BE: token / success
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 |
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 <sajid.mannikeri@ad.infosys.com>
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
zesu22
left a comment
There was a problem hiding this comment.
please resolve review comment
Signed-off-by: Sajid Mannikeri <sajid.mannikeri@ad.infosys.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
mock-relying-party-ui/src/services/relyingPartyService.js (1)
43-45:⚠️ Potential issue | 🟠 MajorConstrain PKCE fallback to known methods only
If
S256is absent, returningsupportedMethods[0]can propagate an unsupportedcode_challenge_methodand break auth. Preferplainonly when explicitly advertised; otherwise default toS256.💡 Proposed fix
- // If method contains S256, use S256; otherwise fallback to first supported method. - return supportedMethods.includes('S256') ? 'S256' : supportedMethods[0]; + // Prefer S256; fallback to plain only if explicitly supported. + if (supportedMethods.includes('S256')) return 'S256'; + if (supportedMethods.includes('plain')) return 'plain'; + 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 43 - 45, The PKCE method selection currently returns supportedMethods[0] when S256 isn't present, which can pick an unsupported value; update the selection in the function that computes code_challenge_method (the block that currently does return supportedMethods.includes('S256') ? 'S256' : supportedMethods[0];) so it returns 'plain' only if supportedMethods.includes('plain'), otherwise default to 'S256'; adjust the catch/return path accordingly so unknown methods are not forwarded as the code_challenge_method.
🧹 Nitpick comments (1)
mock-relying-party-ui/src/services/relyingPartyService.js (1)
60-63: Optional: simplify unreachable null-check
get_code_challenge_methodcurrently always resolves to a non-empty string, so this branch is effectively dead code.🤖 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 60 - 63, The branch guarding against a falsy "method" in the code that calls get_code_challenge_method (the if (!method) { console.warn('PKCE disabled: no supported method found'); return null; } block) is unreachable because get_code_challenge_method always returns a non-empty string; remove the dead branch and the console.warn and instead directly use/return the method value (or adjust the caller to assume a non-null string), ensuring references to get_code_challenge_method remain consistent.
🤖 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 165-169: The PKCE verifier is being removed from sessionStorage
before the token POST, which makes retries impossible if the exchange fails;
change the logic in relyingPartyService.js so that you only call
sessionStorage.removeItem(`pkce_${client_id}_${state}`) after the token exchange
completes successfully (e.g., after a successful fetch/POST response and tokens
are stored), and keep assigning request.code_verifier = codeVerifier beforehand
so retries still include it; update the code path that handles successful token
responses (where tokens are validated/stored) to perform the removal.
---
Duplicate comments:
In `@mock-relying-party-ui/src/services/relyingPartyService.js`:
- Around line 43-45: The PKCE method selection currently returns
supportedMethods[0] when S256 isn't present, which can pick an unsupported
value; update the selection in the function that computes code_challenge_method
(the block that currently does return supportedMethods.includes('S256') ? 'S256'
: supportedMethods[0];) so it returns 'plain' only if
supportedMethods.includes('plain'), otherwise default to 'S256'; adjust the
catch/return path accordingly so unknown methods are not forwarded as the
code_challenge_method.
---
Nitpick comments:
In `@mock-relying-party-ui/src/services/relyingPartyService.js`:
- Around line 60-63: The branch guarding against a falsy "method" in the code
that calls get_code_challenge_method (the if (!method) { console.warn('PKCE
disabled: no supported method found'); return null; } block) is unreachable
because get_code_challenge_method always returns a non-empty string; remove the
dead branch and the console.warn and instead directly use/return the method
value (or adjust the caller to assume a non-null string), ensuring references to
get_code_challenge_method remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 74a2c0d1-dd8c-4e5a-8b4b-47103686e108
📒 Files selected for processing (1)
mock-relying-party-ui/src/services/relyingPartyService.js
Signed-off-by: Sajid Mannikeri <sajid.mannikeri@ad.infosys.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
mock-relying-party-ui/src/services/relyingPartyService.js (1)
43-45:⚠️ Potential issue | 🟠 MajorConstrain PKCE method to known values to avoid method/hash mismatch.
Line 44 can still return an arbitrary method value. Downstream (Line 68-Line 75), non-
plainmethods are hashed with SHA-256, so an unknown method (e.g.,"S512") would send a mismatchedcode_challenge_methodand fail exchange.💡 Proposed fix
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 supportedMethods = response.data?.code_challenge_methods_supported || []; // Default to S256 if (!supportedMethods || supportedMethods.length === 0) return 'S256'; - // If method contains S256, use S256; otherwise fallback to first supported method. - return supportedMethods.includes('S256') ? 'S256' : supportedMethods[0]; + // Use only methods implemented by this client. + if (supportedMethods.includes('S256')) return 'S256'; + if (supportedMethods.includes('plain')) return 'plain'; + return 'S256'; } catch (error) { console.warn('Failed to fetch PKCE methods, defaulting to S256:', error); return 'S256'; } };Also applies to: 68-75
🤖 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 43 - 45, The PKCE selection currently returns supportedMethods[0] which can be an unsupported/unknown value; change the selection logic so it only returns 'S256' or 'plain' (e.g., return 'S256' if supportedMethods includes 'S256', otherwise return 'plain'), and update the downstream code that computes the code_challenge to only treat 'S256' as the hashed case and treat any other value as 'plain' (ensure the block that hashes non-'plain' methods checks specifically for 'S256' rather than "not plain"). Reference the PKCE selection expression (the ternary that returns 'S256' or supportedMethods[0] ) and the code_challenge computation block that branches on non-'plain' methods to locate where to make these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mock-relying-party-ui/src/services/relyingPartyService.js`:
- Around line 43-45: The PKCE selection currently returns supportedMethods[0]
which can be an unsupported/unknown value; change the selection logic so it only
returns 'S256' or 'plain' (e.g., return 'S256' if supportedMethods includes
'S256', otherwise return 'plain'), and update the downstream code that computes
the code_challenge to only treat 'S256' as the hashed case and treat any other
value as 'plain' (ensure the block that hashes non-'plain' methods checks
specifically for 'S256' rather than "not plain"). Reference the PKCE selection
expression (the ternary that returns 'S256' or supportedMethods[0] ) and the
code_challenge computation block that branches on non-'plain' methods to locate
where to make these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d2123fa3-ea14-46db-9568-ebd73f3b41ce
📒 Files selected for processing (1)
mock-relying-party-ui/src/services/relyingPartyService.js
Signed-off-by: Sajid Mannikeri <sajid.mannikeri@ad.infosys.com>
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 68-72: Wrap the crypto.subtle.digest call in a try-catch inside
the PKCE branch where method === 'S256' (the block that uses TextEncoder,
crypto.subtle.digest, base64UrlEncode and assigns codeChallenge); in the catch
log or attach the original error and fail gracefully by either returning a
rejected Promise or throwing a new Error with a clear message that includes the
original error detail, or alternatively set codeChallenge to null and return a
controlled error value—ensure the error handling preserves the original error
information for debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 013d494d-ecfe-4e60-b372-4495b7624292
📒 Files selected for processing (1)
mock-relying-party-ui/src/services/relyingPartyService.js
[ES-2881] Added PKCE implementation in mock relying party.
[ES-2881] Added PKCE implementation in mock relying party. Signed-off-by: SajidMannikeri17 <sajid.mannikeri@infosys.com>
* [ES-2881] Merge pull request #555 from Infosys/ES-2881 [ES-2881] Added PKCE implementation in mock relying party. Signed-off-by: SajidMannikeri17 <sajid.mannikeri@infosys.com> * Added endpoint to fetch identity schema (#564) * Added endpoint to fetch identity schema Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> * Added endpoint to fetch identity schema Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> * Fixed testcase Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> --------- Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> Signed-off-by: SajidMannikeri17 <sajid.mannikeri@infosys.com> * Fixed the required field validation error (#565) * Added endpoint to fetch identity schema Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> * Added endpoint to fetch identity schema Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> * Fixed required fields validation error Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> --------- Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> Signed-off-by: SajidMannikeri17 <sajid.mannikeri@infosys.com> * ES-2799 Merge pull request #557 from ase-101/develop ES-2799 Corrected the logic to full dynamic identity schema Signed-off-by: SajidMannikeri17 <sajid.mannikeri@infosys.com> * updated readme Signed-off-by: sajid.mannikeri <sajid.mannikeri@infosys.com> Signed-off-by: SajidMannikeri17 <sajid.mannikeri@infosys.com> * readme update Signed-off-by: SajidMannikeri17 <sajid.mannikeri@infosys.com> --------- Signed-off-by: SajidMannikeri17 <sajid.mannikeri@infosys.com> Signed-off-by: ase-101 <sunkadaeanusha@gmail.com> Signed-off-by: sajid.mannikeri <sajid.mannikeri@infosys.com> Co-authored-by: ase-101 <sunkadaeanusha@gmail.com> Co-authored-by: Zeeshan Mehboob <82993262+zesu22@users.noreply.github.com>



Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Localization