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 3 commits into
Conversation
🦋 Changeset detectedLatest commit: ecc7752 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 docstrings
🧪 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 ecc7752
💡 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.98%) 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.98% +3.91%
==========================================
Files 155 156 +1
Lines 24398 25168 +770
Branches 1203 1478 +275
==========================================
+ Hits 4410 5534 +1124
+ Misses 19988 19634 -354
🚀 New features to boost your workflow:
|
|
Deployed bc18b2a to https://ForgeRock.github.io/ping-javascript-sdk/pr-682/bc18b2affbeabda1cba92a5f8bf30c5c7730bb91 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/oidc-client - 35.3 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
| : await iFrameManager().getParamsByRedirect({ | ||
| url: req.url, | ||
| resolveOnRedirectUri: new URL(req.url).searchParams.get( | ||
| 'redirect_uri', | ||
| ) as string, | ||
| errorParams, | ||
| successParams: [], | ||
| timeout, | ||
| }); |
There was a problem hiding this comment.
Add runtime validation for redirect_uri parameter.
The cast to string on Line 213-215 is unsafe because searchParams.get() returns string | null. While buildNoneUrl always includes redirect_uri, type safety isn't guaranteed.
🛡️ Proposed fix to add validation
: await iFrameManager().getParamsByRedirect({
url: req.url,
- resolveOnRedirectUri: new URL(req.url).searchParams.get(
- 'redirect_uri',
- ) as string,
+ resolveOnRedirectUri:
+ new URL(req.url).searchParams.get('redirect_uri') ??
+ (() => {
+ throw new Error('redirect_uri missing from session check URL');
+ })(),
errorParams,
successParams: [],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| : await iFrameManager().getParamsByRedirect({ | |
| url: req.url, | |
| resolveOnRedirectUri: new URL(req.url).searchParams.get( | |
| 'redirect_uri', | |
| ) as string, | |
| errorParams, | |
| successParams: [], | |
| timeout, | |
| }); | |
| : await iFrameManager().getParamsByRedirect({ | |
| url: req.url, | |
| resolveOnRedirectUri: | |
| new URL(req.url).searchParams.get('redirect_uri') ?? | |
| (() => { | |
| throw new Error('redirect_uri missing from session check URL'); | |
| })(), | |
| errorParams, | |
| successParams: [], | |
| 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/oidc.api.ts` around lines 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.
| storageClient: StorageClient<OauthTokens>, | ||
| ): Micro.Micro<string | null, GenericError, never> => { | ||
| return Micro.tryPromise({ | ||
| try: async () => { |
There was a problem hiding this comment.
We shouldn't need the async/await here. Let's break this down granularly, handle the get in tryPromise
then map over the tokens && idToken.
psuedocode:
Micro.tryPromise({ try: () => storageClient.get(), catch: failed to get storage client }).pipe(
Micro.map(tokens => tokens && 'idToken' in tokens ? tokens.idToken : null)
Writing it out this way breaks out another issue, we have null as a possible value. We can now consider, do we want null to be success? do we want it to be failure? do we want to lift the predicate out and evaluate it there?
Something to think about - while it doesn't need to be addressed because it's not a concrete solution, breaking out the pieces like this will allow us to compose this and refactor easily. it also allows us to create granular errors for the step in the micro we are at.
| 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.
| const redirectUri = options?.redirectUri ?? config.redirectUri; | ||
| // none mode resolves by recognising when the iframe lands on the redirect URI. | ||
| // An empty redirect_uri means the iframe never matches and silently times out instead of failing fast. | ||
| if (!redirectUri) { | ||
| return yield* Micro.fail<GenericError>({ | ||
| error: 'missing_redirect_uri', | ||
| message: 'redirect_uri is required for session check', | ||
| type: 'argument_error', | ||
| }); | ||
| } |
There was a problem hiding this comment.
semantics - but I think it may be nice to handle the error state above at the top of the function so that way we short-circuit before running any code if not necessary.
| const url = buildNoneUrl(wellknown.authorization_endpoint, config, storedIdToken, options); | ||
| log.debug('Session check (none) URL built'); | ||
|
|
||
| yield* dispatchSessionCheckµ(store, url, SessionCheckResponseType.None); | ||
| log.debug('Session check (none) completed successfully'); | ||
|
|
||
| return {} satisfies SessionCheckSuccess; | ||
| }); |
There was a problem hiding this comment.
if we do the above comment - we can create a singular pipeline for this now.
| const storedIdToken = yield* readStoredIdTokenµ(storageClient); | ||
| const { url, nonce, state } = buildIdTokenUrl( | ||
| wellknown.authorization_endpoint, | ||
| config, | ||
| storedIdToken, | ||
| options, | ||
| ); | ||
| log.debug('Session check (id_token) URL built'); | ||
|
|
||
| const iframeParams = yield* dispatchSessionCheckµ(store, url, SessionCheckResponseType.IdToken); | ||
| const claims = yield* validateSessionCheckResponseµ( | ||
| iframeParams, | ||
| state, | ||
| nonce, | ||
| options?.subject, | ||
| ); | ||
| log.debug('Session check (id_token) completed successfully'); | ||
|
|
||
| return { claims } satisfies SessionCheckSuccess; |
There was a problem hiding this comment.
This is fine - i have no issues with it personally. However, I think as a team, we tend to prefer pipe syntax where it's cleaner. I'd ask if you write it in a pipe if you feel it's cleaner or harder to read. and then just weigh that decision. I think this can be done pretty cleanly as a pipe.
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.
| /** | ||
| * An object containing methods for OIDC session management | ||
| */ | ||
| session: { |
There was a problem hiding this comment.
I'm not sure how I feel about adding a session property here. Since we are checking the user's current session, I feel like we could move this into the user property. user.logout, for example, terminates the current user session.
| // Both a const object (for runtime value access: SessionCheckResponseType.IdToken) and a type | ||
| // (for annotations: param: SessionCheckResponseType) are declared under the same name. | ||
| // This is the TypeScript const-object + union-type pattern — a tree-shakeable alternative to enums | ||
| // that preserves the string literals ('id_token' | 'none') in the compiled output. | ||
| export const SessionCheckResponseType = { | ||
| IdToken: 'id_token', | ||
| None: 'none', | ||
| } as const; | ||
|
|
||
| export type SessionCheckResponseType = | ||
| (typeof SessionCheckResponseType)[keyof typeof SessionCheckResponseType]; |
There was a problem hiding this comment.
I have to admit, this really confused me for longer than a care to admit. The fact that they are both named the same, but serve different purposes ... the const is actual runtime code, where the type is compile-time only diverges away from our patterns. We don't really want runtime code in our .type.ts files as they should be completely compiled away.
I would be more open to this if we had to derive the type dynamically from something external, or something that is highly volatile or unknown at compile time. Since this is a static config property that is very simple and will likely never change, this feels over-engineered.
This pattern essentially gets reduced down to a string union: 'id_token' | 'none', why don't we just directly write it as such? When the type is directly declared as a union of strings, you get the same amount of type-ahead/autocomplete/spellcheck as a string enum. So, I'm wondering why the additional complexity.
There was a problem hiding this comment.
I think this is good feedback for this given type / data. It is simple enough that it makes sense to keep it as a literal union of strings.
| ? await iFrameManager().getParamsByRedirect({ | ||
| url: req.url, | ||
| successParams: ['id_token'], | ||
| errorParams, | ||
| includeHashParams: true, | ||
| timeout, | ||
| }) | ||
| : await iFrameManager().getParamsByRedirect({ | ||
| url: req.url, | ||
| resolveOnRedirectUri: new URL(req.url).searchParams.get( | ||
| 'redirect_uri', | ||
| ) as string, | ||
| errorParams, | ||
| successParams: [], | ||
| timeout, |
There was a problem hiding this comment.
Since this ternary calls the same function, but with slightly different configs, can we just construct the config within a condition, and call the function once with whatever config is built? By splitting the actual function calls between the condition, it reads more complex than it really is.
| // With resolveOnRedirectUri (response_type=none), success is detected via URI landing so | ||
| // successParams:[] is intentional — but errorParams is still required since errors arrive as query params. |
There was a problem hiding this comment.
I'm not sure I understand these changes. Can elaborate on this case where there are neither success or error params?
There was a problem hiding this comment.
Error params will always exist.
The case is for response type none. For response type none, success params naturally don't exist because the response type is none. For example, I experimented with state by adding state to success params, expecting AM to echo it back but it did not.
So response type none by its nature returns no query params in success case.
There was a problem hiding this comment.
Ah, okay, that makes sense ... interesting. Okay, I'll re-review this bit of code with that in mind.
| if (exitIsSuccess(result)) { | ||
| return result.value; | ||
| } else if (exitIsFail(result)) { | ||
| return result.cause.error; | ||
| } else { | ||
| const defect = causeIsDie(result.cause) ? result.cause.defect : undefined; | ||
| return { | ||
| error: 'Session check failure', | ||
| message: defect instanceof Error ? defect.message : String(defect ?? 'Unknown defect'), | ||
| type: 'unknown_error', | ||
| }; | ||
| } |
There was a problem hiding this comment.
I think this is common enough we may want to abstract this out into a reusable function to make this composable.
| 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.
| scope?: string; | ||
| } | ||
|
|
||
| export interface SessionCheckSuccess { |
There was a problem hiding this comment.
I'm not sure I understand this. This type represents the data structure of an ID Token, yeah? An ID Token's encoded JSON data should just be a flat map. Something like this:
{
"iss": "https://server.example.com",
"sub": "24400320",
"aud": "s6BhdRkqt3",
"nonce": "n-0S6_WzA2Mj",
"exp": 1311281970,
"iat": 1311280970,
"auth_time": 1311280969,
"acr": "urn:mace:incommon:iap:silver"
}
So, I'm a bit thrown when I see a claims property that has an object with unknown values. I feel like I'm missing where the claims property is coming from.
| yield* dispatchSessionCheckµ(store, url, SessionCheckResponseType.None); | ||
| log.debug('Session check (none) completed successfully'); | ||
|
|
||
| return {} satisfies SessionCheckSuccess; |
There was a problem hiding this comment.
This feels like we are circumventing the type system. I think we should just be more explicit that the session check return value could be 3 things, rather than just two: 1) an object with an optional property of claims or 2) an error. If we move to 3 return values, it would be something like this:
{ result: 'token', data: IdToken }{ result: 'boolean', data: true }GenericError
Does that make sense?
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
| export interface SessionCheckOptions { | ||
| /** The response type for the session check. Defaults to 'none'. */ | ||
| responseType?: SessionCheckResponseType; | ||
| /** Overrides OidcConfig.redirectUri for the session check request. */ | ||
| redirectUri?: string; | ||
| /** If provided, the sub claim in the returned id_token must match. id_token mode only. */ | ||
| subject?: string; | ||
| /** OAuth scope. Default: 'openid'. */ | ||
| scope?: string; |
There was a problem hiding this comment.
Make subject invalid unless responseType is 'id_token'.
subject is documented as id_token-only, but this type still allows { subject: '...' } when responseType is omitted or 'none'. In packages/oidc-client/src/lib/client.store.ts, that path falls through to sessionCheckNoneµ, so the option is silently ignored instead of enforced.
Suggested contract tightening
-export interface SessionCheckOptions {
- /** The response type for the session check. Defaults to 'none'. */
- responseType?: SessionCheckResponseType;
- /** Overrides OidcConfig.redirectUri for the session check request. */
- redirectUri?: string;
- /** If provided, the sub claim in the returned id_token must match. id_token mode only. */
- subject?: string;
- /** OAuth scope. Default: 'openid'. */
- scope?: string;
-}
+type SessionCheckBaseOptions = {
+ /** Overrides OidcConfig.redirectUri for the session check request. */
+ redirectUri?: string;
+ /** OAuth scope. Default: 'openid'. */
+ scope?: string;
+};
+
+export type SessionCheckOptions =
+ | (SessionCheckBaseOptions & {
+ /** The response type for the session check. Defaults to 'none'. */
+ responseType?: 'none';
+ subject?: never;
+ })
+ | (SessionCheckBaseOptions & {
+ responseType: 'id_token';
+ /** If provided, the sub claim in the returned id_token must match. */
+ subject?: string;
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface SessionCheckOptions { | |
| /** The response type for the session check. Defaults to 'none'. */ | |
| responseType?: SessionCheckResponseType; | |
| /** Overrides OidcConfig.redirectUri for the session check request. */ | |
| redirectUri?: string; | |
| /** If provided, the sub claim in the returned id_token must match. id_token mode only. */ | |
| subject?: string; | |
| /** OAuth scope. Default: 'openid'. */ | |
| scope?: string; | |
| type SessionCheckBaseOptions = { | |
| /** Overrides OidcConfig.redirectUri for the session check request. */ | |
| redirectUri?: string; | |
| /** OAuth scope. Default: 'openid'. */ | |
| scope?: string; | |
| }; | |
| export type SessionCheckOptions = | |
| | (SessionCheckBaseOptions & { | |
| /** The response type for the session check. Defaults to 'none'. */ | |
| responseType?: 'none'; | |
| subject?: never; | |
| }) | |
| | (SessionCheckBaseOptions & { | |
| responseType: 'id_token'; | |
| /** If provided, the sub claim in the returned id_token must match. */ | |
| subject?: string; | |
| }); |
🤖 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.types.ts` around lines 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.
| scope?: string; | ||
| } | ||
|
|
||
| export type SessionCheckSuccess = { mode: 'none' } | { mode: 'id_token'; claims: JWTPayload }; |
There was a problem hiding this comment.
Do not expose these as ordinary verified claims.
The runtime path in packages/oidc-client/src/lib/session.micros.ts only decodeJwt()s the iframe token and checks state/nonce/optional sub; it does not verify signature, issuer, audience, or expiry, and the PR notes that an invalid token can still produce a valid session. Returning claims: JWTPayload here makes the payload look safe to use for identity or authorization decisions when it is not.
🤖 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.types.ts` at 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.
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!
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