feat(oidc-client): add oidc session check with response type of id_token and none#682
feat(oidc-client): add oidc session check with response type of id_token and none#682vatsalparikh wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: 086ce08 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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:
📝 WalkthroughWalkthroughAdds OIDC session status checking via a new user.session() method supporting prompt=none flows (response_type=none and response_type=id_token), enhances iframe-manager for hash/redirect matching, implements effect-based orchestration and RTK mutations, integrates into the OIDC client, and adds unit/E2E tests plus test-app UI buttons. ChangesSession Check Feature Implementation
Sequence DiagramssequenceDiagram
participant App
participant OidcClient as oidc() client
participant SessionMicro as sessionCheckNoneµ/IdTokenµ
participant StorageClient
participant IFrameDispatch as sessionCheckIframe RTK
participant IFrameManager
participant AuthorizationServer as IdP
App->>OidcClient: user.session({responseType: 'none'})
OidcClient->>SessionMicro: call with config + options
SessionMicro->>StorageClient: read stored idToken
StorageClient-->>SessionMicro: idToken (or null)
SessionMicro->>SessionMicro: build authorize URL (prompt=none)
SessionMicro->>IFrameDispatch: dispatch iframe/fetch mutation with url
IFrameDispatch->>IFrameManager: getParamsByRedirect()
IFrameManager->>AuthorizationServer: load authorize endpoint in iframe
AuthorizationServer-->>IFrameManager: redirect to redirectUri
IFrameManager-->>IFrameDispatch: extracted params
IFrameDispatch-->>SessionMicro: return params
SessionMicro-->>OidcClient: SessionCheckSuccess {mode:'none'} or GenericError
OidcClient-->>App: return result
sequenceDiagram
participant App
participant OidcClient as oidc() client
participant SessionMicro as sessionCheckIdTokenµ
participant StorageClient
participant IFrameDispatch
participant IFrameManager
participant AuthorizationServer as IdP
participant JWTDecoder as jose
App->>OidcClient: user.session({responseType: 'id_token'})
OidcClient->>SessionMicro: call (generates nonce,state)
SessionMicro->>StorageClient: read stored idToken (hint)
SessionMicro->>SessionMicro: build authorize URL with nonce,state
SessionMicro->>IFrameDispatch: dispatch iframe mutation
IFrameDispatch->>IFrameManager: getParamsByRedirect()
IFrameManager->>AuthorizationServer: load authorize endpoint
AuthorizationServer-->>IFrameManager: redirect with id_token
IFrameManager-->>IFrameDispatch: return id_token + state
SessionMicro->>JWTDecoder: decode id_token
JWTDecoder-->>SessionMicro: claims
SessionMicro->>OidcClient: SessionCheckSuccess {mode:'id_token', claims}
OidcClient-->>App: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
|
View your CI Pipeline Execution ↗ for commit 086ce08
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
vatsalparikh
changed the title
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (21.97%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #682 +/- ##
==========================================
+ Coverage 18.07% 21.97% +3.89%
==========================================
Files 155 157 +2
Lines 24398 25188 +790
Branches 1203 1475 +272
==========================================
+ Hits 4410 5534 +1124
+ Misses 19988 19654 -334
🚀 New features to boost your workflow:
|
|
Deployed 0ccb932 to https://ForgeRock.github.io/ping-javascript-sdk/pr-682/0ccb9320451cfe01cecbf9e4879fb6b8db73cd90 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/oidc-client - 35.2 KB (new) 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
vatsalparikh
requested review from
SteinGabriel,
ancheetah,
cerebrl and
ryanbas21
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
e2e/oidc-suites/src/session.spec.ts (1)
15-30: 💤 Low valueConsider base64url-encoding the fake signature for structural correctness.
The
makeFakeJwthelper uses a plain string"fakesignature"for the third segment. While this may work when the SDK only validates the payload (nonce/sub/iat), a structurally correct JWT should have a base64url-encoded signature segment. Consider encoding it for better test fidelity.♻️ Proposed refinement
const body = btoa(JSON.stringify(payload)) .replace(/\+/g, '-') .replace(/\//g, '_') .replace(/=/g, ''); - return `${header}.${body}.fakesignature`; + const signature = btoa('fake-signature-bytes') + .replace(/\+/g, '-') + .replace(/\//g, '_') + .replace(/=/g, ''); + return `${header}.${body}.${signature}`; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/oidc-suites/src/session.spec.ts` around lines 15 - 30, The fake JWT's signature segment is a plain string; update the makeFakeJwt helper to produce a structurally correct base64url-encoded signature instead of "fakesignature". Locate the makeFakeJwt function and replace the static third segment with a base64url-encoded value (e.g., encode a constant like "fakesignature" using the same btoa + replace(/\+/g,'-')/replace(/\//g,'_')/replace(/=/g,'') pattern used for header and body) so the JWT has three properly encoded segments.packages/oidc-client/src/lib/session.micros.ts (2)
25-39: ⚡ Quick winReconsider the error
typefor storage read failures.The error mapping uses
type: 'argument_error'for storage access failures. Storage read failures are typically infrastructure or I/O issues, not argument validation problems. Consider usingtype: 'unknown_error'or a more appropriate type that reflects the nature of storage access failures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oidc-client/src/lib/session.micros.ts` around lines 25 - 39, The GenericError returned by readStoredIdTokenµ currently uses type: 'argument_error' which is misleading for storage/I/O failures; update the catch mapper in readStoredIdTokenµ to use a more appropriate error type such as 'unknown_error' (or an I/O/infrastructure-specific type) so storageClient.get() failures are classified correctly, keeping the same error and message fields; ensure the change is applied to the catch block that constructs the GenericError (referencing readStoredIdTokenµ, StorageClient<OauthTokens>, and GenericError).
206-235: ⚡ Quick winConsider validating
redirectUripresence in id_token mode.While id_token mode resolves by detecting the
id_tokenparameter rather than matching the redirect URI, theredirectUriis still included in the authorization request (line 161) and must be valid. Consider adding an early validation check similar to the one insessionCheckNoneµ(lines 186-192) to fail fast ifredirectUriis missing, preventing a confusing AS error or timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oidc-client/src/lib/session.micros.ts` around lines 206 - 235, sessionCheckIdTokenµ currently proceeds to build the id_token URL without ensuring a redirectUri is present, which can cause confusing AS errors; add the same early validation used in sessionCheckNoneµ to fail fast: before calling buildIdTokenUrl (or immediately after entering sessionCheckIdTokenµ) check for a valid redirectUri (e.g. options?.redirectUri or the config field your code expects) and throw/return a GenericError if missing, mirroring the validation logic from sessionCheckNoneµ so callers get a clear, early error instead of an AS timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/oidc-app/src/utils/oidc-app.ts`:
- Around line 216-231: Replace the use of innerHTML in the session check button
handlers with safe DOM text insertion: instead of setting el.innerHTML use
createElement/textContent to build the nodes and set the JSON string into a text
node (or set el.textContent / the pre element's textContent) so untrusted values
from oidcClient.session?.check() are not interpreted as HTML; update both the
handler for 'session-check-btn' (that currently creates a div and sets innerHTML
with "Session Check (none)" and the JSON) and the handler for
'session-check-id-token-btn' (that sets innerHTML with "Session Check
(id_token)" and the JSON) to construct elements and assign textContent for the
JSON output before appending to the app element.
In `@packages/oidc-client/src/lib/oidc.api.ts`:
- Around line 211-219: The code unsafely casts
URL(req.url).searchParams.get('redirect_uri') to string when calling
iFrameManager().getParamsByRedirect; add a runtime null-check for the
redirect_uri value (derived from URL(req.url).searchParams.get('redirect_uri'))
before invoking iFrameManager().getParamsByRedirect (the resolveOnRedirectUri
parameter), and if it's missing handle it explicitly (throw an error, return a
failure response, or log and reject) so the call only receives a guaranteed
string; update the surrounding code path that builds/consumes redirect_uri
(e.g., buildNoneUrl usages) to maintain type safety and avoid the direct `as
string` cast.
---
Nitpick comments:
In `@e2e/oidc-suites/src/session.spec.ts`:
- Around line 15-30: The fake JWT's signature segment is a plain string; update
the makeFakeJwt helper to produce a structurally correct base64url-encoded
signature instead of "fakesignature". Locate the makeFakeJwt function and
replace the static third segment with a base64url-encoded value (e.g., encode a
constant like "fakesignature" using the same btoa +
replace(/\+/g,'-')/replace(/\//g,'_')/replace(/=/g,'') pattern used for header
and body) so the JWT has three properly encoded segments.
In `@packages/oidc-client/src/lib/session.micros.ts`:
- Around line 25-39: The GenericError returned by readStoredIdTokenµ currently
uses type: 'argument_error' which is misleading for storage/I/O failures; update
the catch mapper in readStoredIdTokenµ to use a more appropriate error type such
as 'unknown_error' (or an I/O/infrastructure-specific type) so
storageClient.get() failures are classified correctly, keeping the same error
and message fields; ensure the change is applied to the catch block that
constructs the GenericError (referencing readStoredIdTokenµ,
StorageClient<OauthTokens>, and GenericError).
- Around line 206-235: sessionCheckIdTokenµ currently proceeds to build the
id_token URL without ensuring a redirectUri is present, which can cause
confusing AS errors; add the same early validation used in sessionCheckNoneµ to
fail fast: before calling buildIdTokenUrl (or immediately after entering
sessionCheckIdTokenµ) check for a valid redirectUri (e.g. options?.redirectUri
or the config field your code expects) and throw/return a GenericError if
missing, mirroring the validation logic from sessionCheckNoneµ so callers get a
clear, early error instead of an AS timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f1fa359c-8961-4f12-9227-9e66f943afc1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.changeset/brave-foxes-dance.mde2e/oidc-app/src/ping-am/index.htmle2e/oidc-app/src/ping-one/index.htmle2e/oidc-app/src/utils/oidc-app.tse2e/oidc-suites/src/session.spec.tse2e/oidc-suites/src/utils/login.tspackages/oidc-client/api-report/oidc-client.api.mdpackages/oidc-client/api-report/oidc-client.types.api.mdpackages/oidc-client/package.jsonpackages/oidc-client/src/lib/client.store.test.tspackages/oidc-client/src/lib/client.store.tspackages/oidc-client/src/lib/oidc.api.tspackages/oidc-client/src/lib/session.micros.test.tspackages/oidc-client/src/lib/session.micros.tspackages/oidc-client/src/lib/session.types.tspackages/oidc-client/src/types.tspackages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.test.tspackages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.tspnpm-workspace.yaml
| if ('error' in result && result.error) { | ||
| const errData = result.error as { | ||
| data?: { error?: string; message?: string; type?: string }; | ||
| }; |
There was a problem hiding this comment.
Why have this check if we can just handle it in the catch above? seems like we're duplicating error handling? We handle it as a Generic error and then update it as errData? This seems like it can be a singular pipe,
Micro.tryPromise({
try: store-dispatch,
catch: error
}).pipe(
Micro.mapError(fail),
Micro.succeed(success)
);
We can also consider, do we want to "fail" here in session.micro.ts or do we want to allow the failure to be consumed later on in the pipeline? That way it's possible the session check here can be composed and handled uniquely when it needs to be? Again - similarly this is just me thinking in my head, and not something to specifically be addressed but i'm trying to break down my thought process in why I think to write the effectful pipelines as i'm suggesting.
There was a problem hiding this comment.
Thanks, using the pipe here!
Doing this with flatMap instead of mapError. RTK Query always resolves the promise successfully, even when the request fails, it just puts the failure inside the result as an error property { error: } . So mapError wouldn't catch it.
In response to failing here, I believe we are doing all error handling for generic error in respective functions instead of bubbling up. When we do work on unifying errors, we'll have something else than Generic Error and we can think about returning the exact error types then. I think this looks good for now.
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
Left some comments after an initial review. I'll take a deeper look at your .micro.ts file soon.
| export interface SessionCheckSuccess { | ||
| /** Decoded id_token payload. Present only in id_token mode. */ | ||
| claims?: Record<string, unknown>; | ||
| } |
There was a problem hiding this comment.
This may be a nitpick but I'm not a huge fan of types like this because {} checks as SessionCheckSuccess.
We should probably declare a specific type for when an id_token is valid so that we can drive through the type system.
Because the other member of the union is essentially an empty object, I wonder if we should declare a discriminator { success: boolean } | { success: boolean; claims: Record<string, unknown> }
This way we can tell if we have claims or not. it alleviates the {} type from bottom type check from passing by mistake
We probably could even type claims more strictly.
I just fear having {} as a type alias as a structured return type.
There was a problem hiding this comment.
Okay, it can seem a bit empty, I agree. I am using a shape like below now
export type SessionCheckSuccess = {
responseType: SessionCheckResponseType;
claims?: JWTPayload;
};
So the return will be something like
{
responseType: 'none'
}
About using a discriminator, I don't think it is necessary. SessionCheckSuccess should be sufficient. Let me know if you feel otherwise.
I am using JWTPayload for claims now, so that should be good!
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/oidc-app/src/utils/oidc-app.ts`:
- Around line 22-25: The displayError function currently injects JSON into the
DOM via errorEl.innerHTML which allows HTML injection; change it to create safe
text nodes instead by setting the element's textContent (or creating a child
text node) for the serialized error payload and avoid using innerHTML — update
the displayError function and the errorEl creation (referenced as displayError
and errorEl) so the error JSON is rendered via textContent to prevent XSS.
In `@packages/oidc-client/src/lib/session.types.ts`:
- Line 30: The SessionCheckSuccess type exposes decoded JWT payload as
JWTPayload which misleads callers into treating it as verified; change the type
in SessionCheckSuccess (and any exports referencing it) to a clearly untrusted
shape (e.g., claims: UnverifiedJwtPayload or claims: Record<string, unknown>)
and update usages in session.micros.ts (the code path that only calls
decodeJwt() and checks state/nonce/sub) so callers must explicitly verify
signatures/claims before making auth/identity decisions; ensure the new type
name and shape make it obvious these claims are unverified.
- Around line 19-27: Update the SessionCheckOptions type to a discriminated
union so that `subject` is only allowed when `responseType` is `'id_token'`:
replace the open interface `SessionCheckOptions` with two variants (one for
`responseType?: 'none' | undefined` that forbids `subject`, and one for
`responseType: 'id_token'` that permits `subject?: string`) and keep
`SessionCheckResponseType` as the discriminant; adjust any call sites (e.g.,
code paths in `client.store.ts` that call `sessionCheckNoneµ`) to satisfy the
new types if necessary. Ensure JSDoc/comments still reflect that `subject` is
id_token-only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b25862b5-6334-4768-979b-c19377454717
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
.changeset/brave-foxes-dance.mde2e/oidc-app/src/ping-am/index.htmle2e/oidc-app/src/ping-one/index.htmle2e/oidc-app/src/utils/oidc-app.tse2e/oidc-suites/src/session.spec.tse2e/oidc-suites/src/utils/login.tspackages/oidc-client/api-report/oidc-client.api.mdpackages/oidc-client/api-report/oidc-client.types.api.mdpackages/oidc-client/package.jsonpackages/oidc-client/src/lib/client.store.test.tspackages/oidc-client/src/lib/client.store.tspackages/oidc-client/src/lib/client.store.utils.tspackages/oidc-client/src/lib/oidc.api.tspackages/oidc-client/src/lib/session.micros.test.tspackages/oidc-client/src/lib/session.micros.tspackages/oidc-client/src/lib/session.types.tspackages/oidc-client/src/types.tspackages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.test.tspackages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.tspnpm-workspace.yaml
✅ Files skipped from review due to trivial changes (3)
- .changeset/brave-foxes-dance.md
- packages/oidc-client/src/types.ts
- pnpm-workspace.yaml
🚧 Files skipped from review as they are similar to previous changes (10)
- e2e/oidc-app/src/ping-one/index.html
- packages/oidc-client/src/lib/client.store.test.ts
- e2e/oidc-suites/src/utils/login.ts
- e2e/oidc-app/src/ping-am/index.html
- packages/oidc-client/src/lib/oidc.api.ts
- packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.test.ts
- e2e/oidc-suites/src/session.spec.ts
- packages/oidc-client/src/lib/session.micros.test.ts
- packages/oidc-client/src/lib/session.micros.ts
- packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/oidc-client/src/lib/oidc.api.ts`:
- Around line 295-323: sessionCheckFetch currently collapses all response.error
cases into an auth error and assumes any success means 204; change it to: if
response.error exists and represents a transport/fetch-level failure (i.e., no
response available on response.meta or error.status is one of
FETCH_ERROR/TIMEOUT_ERROR/PARSING_ERROR) then return the original response.error
unchanged to preserve RTK Query transport semantics, otherwise (HTTP error
present) map the HTTP error to the existing login_required structure; on
success, inspect response.meta?.response?.status and only return { data: {
status: 204 } } when that status === 204, otherwise return a suitable
FetchBaseQueryError indicating unexpected session-check status. Ensure you
update the logic around response, response.error, and
response.meta.response.status inside sessionCheckFetch to implement these
branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c0aef36-836e-4308-b6dc-9201b9327344
📒 Files selected for processing (8)
e2e/oidc-app/src/ping-am/index.htmle2e/oidc-app/src/ping-one/index.htmle2e/oidc-app/src/utils/oidc-app.tspackages/oidc-client/api-report/oidc-client.api.mdpackages/oidc-client/api-report/oidc-client.types.api.mdpackages/oidc-client/src/lib/oidc.api.tspackages/oidc-client/src/lib/session.micros.test.tspackages/oidc-client/src/lib/session.micros.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/oidc-app/src/ping-am/index.html
- packages/oidc-client/api-report/oidc-client.api.md
- packages/oidc-client/api-report/oidc-client.types.api.md
There was a problem hiding this comment.
Nx Cloud has identified a flaky task in your failed CI:
🔂 Since the failure was identified as flaky, we triggered a CI rerun by adding an empty commit to this branch.
🎓 Learn more about Self-Healing CI on nx.dev
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
e2e/oidc-app/src/utils/oidc-app.ts (1)
22-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop injecting session/error payloads via
innerHTML; render withtextContentinstead.Line 24 and session-check render paths (Lines 221, 230, 239) inject serialized payloads into HTML sinks. These values can include server-controlled fields and should be treated as untrusted.
Proposed patch
function displayError(error: unknown) { const errorEl = document.createElement('div'); - errorEl.innerHTML = `<p><strong>Error:</strong> <span class="error">${JSON.stringify(error, null, 2)}</span></p>`; + const p = document.createElement('p'); + const strong = document.createElement('strong'); + strong.textContent = 'Error:'; + const span = document.createElement('span'); + span.className = 'error'; + span.textContent = JSON.stringify(error, null, 2); + p.append(strong, document.createTextNode(' '), span); + errorEl.appendChild(p); document.body.appendChild(errorEl); } @@ - el.innerHTML = `<p><strong>Session Check (none, iframe):</strong></p><pre id="session-check-result">${JSON.stringify(result, null, 2)}</pre>`; + const title = document.createElement('p'); + const strong = document.createElement('strong'); + strong.textContent = 'Session Check (none, iframe):'; + title.appendChild(strong); + const pre = document.createElement('pre'); + pre.id = 'session-check-result'; + pre.textContent = JSON.stringify(result, null, 2); + el.append(title, pre); @@ - el.innerHTML = `<p><strong>Session Check (none, no redirect):</strong></p><pre id="session-check-no-redirect-result">${JSON.stringify(result, null, 2)}</pre>`; + const title = document.createElement('p'); + const strong = document.createElement('strong'); + strong.textContent = 'Session Check (none, no redirect):'; + title.appendChild(strong); + const pre = document.createElement('pre'); + pre.id = 'session-check-no-redirect-result'; + pre.textContent = JSON.stringify(result, null, 2); + el.append(title, pre); @@ - el.innerHTML = `<p><strong>Session Check (id_token):</strong></p><pre id="session-check-id-token-result">${JSON.stringify(result, null, 2)}</pre>`; + const title = document.createElement('p'); + const strong = document.createElement('strong'); + strong.textContent = 'Session Check (id_token):'; + title.appendChild(strong); + const pre = document.createElement('pre'); + pre.id = 'session-check-id-token-result'; + pre.textContent = JSON.stringify(result, null, 2); + el.append(title, pre);Also applies to: 221-223, 229-231, 239-240
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/oidc-app/src/utils/oidc-app.ts` around lines 22 - 25, The UI currently injects untrusted payloads via innerHTML (e.g., function displayError) which risks XSS; update displayError and the session-check render paths referenced in the comment to build DOM nodes and assign data using textContent (or setAttribute) instead of innerHTML: create the surrounding elements (p, strong, span) via document.createElement, set their textContent to the safe JSON.stringify(output, null, 2) or individual fields, and append them to document.body (or the target container); ensure no raw HTML strings are passed into innerHTML anywhere in the displayError function or the session rendering code paths.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/oidc-client/src/lib/oidc.api.ts`:
- Around line 297-320: sessionCheckFetch currently treats numeric HTTP failures
the same as transport/network errors; update the response mapping to distinguish
network/transport errors (when typeof responseError.status === 'string') from
HTTP errors (when status is a number). In the error return block (variables:
responseError, isNetworkError, errorData, message) set status to
responseError.status for HTTP errors and to 0 or a non-HTTP sentinel for network
errors; set data.error to 'session_check_error' and type to 'network_error' for
network/transport failures, but to 'login_required' and 'auth_error' for HTTP
401/403 auth failures (use responseError.status to decide), and derive message
from errorData.error_description or responseError.statusText accordingly so
outages aren’t reported as “not logged in” (preserve
GenericError/FetchBaseQueryError shapes). Ensure logger.error still logs
responseError.
---
Duplicate comments:
In `@e2e/oidc-app/src/utils/oidc-app.ts`:
- Around line 22-25: The UI currently injects untrusted payloads via innerHTML
(e.g., function displayError) which risks XSS; update displayError and the
session-check render paths referenced in the comment to build DOM nodes and
assign data using textContent (or setAttribute) instead of innerHTML: create the
surrounding elements (p, strong, span) via document.createElement, set their
textContent to the safe JSON.stringify(output, null, 2) or individual fields,
and append them to document.body (or the target container); ensure no raw HTML
strings are passed into innerHTML anywhere in the displayError function or the
session rendering code paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bccc04db-80fc-42ce-bba5-f5696bcae4ac
📒 Files selected for processing (10)
e2e/oidc-app/src/ping-am/index.htmle2e/oidc-app/src/ping-one/index.htmle2e/oidc-app/src/utils/oidc-app.tspackages/oidc-client/README.mdpackages/oidc-client/api-report/oidc-client.api.mdpackages/oidc-client/api-report/oidc-client.types.api.mdpackages/oidc-client/src/lib/client.store.test.tspackages/oidc-client/src/lib/oidc.api.tspackages/oidc-client/src/lib/session.micros.test.tspackages/oidc-client/src/lib/session.micros.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- e2e/oidc-app/src/ping-one/index.html
- e2e/oidc-app/src/ping-am/index.html
- packages/oidc-client/api-report/oidc-client.types.api.md
- packages/oidc-client/api-report/oidc-client.api.md
- packages/oidc-client/src/lib/session.micros.test.ts
vatsalparikh
force-pushed
the
sdks-5003
branch
3 times, most recently
from
be134fd to
bc03162
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/oidc-client/src/lib/session.micros.test.ts`:
- Around line 179-189: The iframe-path test for sessionCheckNoneµ currently only
asserts success and dispatch calls but misses verifying the returned
SessionCheckSuccess payload; update the test for sessionCheckNoneµ to, after
confirming Micro.exitIsSuccess(exit), assert exit.value equals the expected
SessionCheckSuccess object ({ mode: 'none' })—use the same pattern as the
fetch-path test (check Micro.exitIsSuccess(exit) then
expect(exit.value).toStrictEqual({ mode: 'none' })) to ensure the iframe branch
returns the correct value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0639c678-93eb-4d1e-be3a-105a0e10a815
📒 Files selected for processing (10)
e2e/oidc-app/src/ping-am/index.htmle2e/oidc-app/src/ping-one/index.htmle2e/oidc-app/src/utils/oidc-app.tspackages/oidc-client/README.mdpackages/oidc-client/api-report/oidc-client.api.mdpackages/oidc-client/api-report/oidc-client.types.api.mdpackages/oidc-client/src/lib/client.store.test.tspackages/oidc-client/src/lib/oidc.api.tspackages/oidc-client/src/lib/session.micros.test.tspackages/oidc-client/src/lib/session.micros.ts
✅ Files skipped from review due to trivial changes (2)
- e2e/oidc-app/src/ping-one/index.html
- packages/oidc-client/README.md
🚧 Files skipped from review as they are similar to previous changes (6)
- e2e/oidc-app/src/ping-am/index.html
- packages/oidc-client/api-report/oidc-client.types.api.md
- packages/oidc-client/src/lib/client.store.test.ts
- packages/oidc-client/src/lib/session.micros.ts
- packages/oidc-client/src/lib/oidc.api.ts
- e2e/oidc-app/src/utils/oidc-app.ts
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
This looks good to me. Great job, Vatsal!
vatsalparikh
force-pushed
the
sdks-5003
branch
6 times, most recently
from
4bd0fed to
8628489
Compare
vatsalparikh
force-pushed
the
sdks-5003
branch
2 times, most recently
from
05d53ad to
8c0043b
Compare
4 participants
https://pingidentity.atlassian.net/browse/SDKS-5003
Implementation Decisions
How to test
pnpm nx build oidc-app && pnpm nx serve oidc-apphttp://localhost:8443/ping-ampage and Login (Background)Recording
Screen.Recording.2026-06-09.at.7.32.41.AM.mov
Summary by CodeRabbit
New Features
Enhancements
Tests
Documentation