Conversation
🦋 Changeset detectedLatest commit: e012bed The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 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 |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (33)
📝 WalkthroughWalkthroughAdds end-to-end PAR (Pushed Authorization Request) support: centralizes authorize-parameter building, adds effects/micros for PAR/dispatch, integrates PAR into authorize flow, updates SDK entry exports, and adds unit and E2E tests and fixtures. ChangesPushed Authorization Request (PAR) Support
Sequence Diagram(s)sequenceDiagram
participant Client
participant OidcLib
participant RTK_Par
participant AuthServer
Client->>OidcLib: authorize.url (useParFlow = true)
OidcLib->>OidcLib: build auth params + PKCE (buildAuthorizeParams / generatePkceChallenge)
OidcLib->>RTK_Par: POST { endpoint, body: URLSearchParams } (dispatchParRequest)
RTK_Par->>AuthServer: HTTP POST /par
AuthServer-->>RTK_Par: { request_uri, expires_in }
RTK_Par-->>OidcLib: PushAuthorizationResponse
OidcLib-->>Client: slim authorize URL (client_id + request_uri)
Client->>AuthServer: Redirect with slim URL
AuthServer-->>Client: Redirect with code & state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
|
View your CI Pipeline Execution ↗ for commit 08f8d74
☁️ Nx Cloud last updated this comment at |
ryanbas21
force-pushed
the
par
branch
2 times, most recently
from
1ee2b2c to
5526f30
Compare
@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 (19.84%) 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 #631 +/- ##
==========================================
+ Coverage 18.07% 19.84% +1.77%
==========================================
Files 155 158 +3
Lines 24398 24788 +390
Branches 1203 1331 +128
==========================================
+ Hits 4410 4920 +510
+ Misses 19988 19868 -120
🚀 New features to boost your workflow:
|
Contributor
|
Deployed 673dc21 to https://ForgeRock.github.io/ping-javascript-sdk/pr-631/673dc21a2cb934f8955a39f975afb93a94249602 branch gh-pages in ForgeRock/ping-javascript-sdk |
Contributor
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔺 @forgerock/sdk-oidc - 5.7 KB (+0.9 KB, +18.9%) 📊 Minor Changes📈 @forgerock/sdk-request-middleware - 4.6 KB (+0.0 KB) ➖ No Changes➖ @forgerock/storage - 1.5 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
![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.
Contributor
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud is proposing a fix for your failed CI:
We updated the PAR background login test to match the two-step flow introduced alongside it in this PR: the journey form must be submitted first to establish an AM session and enable the Login (Background) button before background auth can succeed. The original test skipped that step, causing Playwright to time out waiting for the disabled button to become actionable, and incorrectly expected a redirect to the AM login page which does not occur in the prompt=none silent-auth happy path.
Warning
❌ We could not verify this fix.
Suggested Fix changes
diff --git a/e2e/oidc-suites/src/par.spec.ts b/e2e/oidc-suites/src/par.spec.ts
index d680f95..08afd89 100644
--- a/e2e/oidc-suites/src/par.spec.ts
+++ b/e2e/oidc-suites/src/par.spec.ts
@@ -12,7 +12,7 @@ import { asyncEvents } from './utils/async-events.js';
test.describe('PAR (Pushed Authorization Request) login tests', () => {
test('background login with PAR enabled (ParClient) obtains access token', async ({ page }) => {
- const { clickWithRedirect, navigate } = asyncEvents(page);
+ const { navigate } = asyncEvents(page);
const parRequests: string[] = [];
@@ -24,14 +24,14 @@ test.describe('PAR (Pushed Authorization Request) login tests', () => {
await navigate('/par/');
- await clickWithRedirect('Login (Background)', '**/am/XUI/**');
-
+ // Step 1: Establish AM session via journey login to enable the background button
await page.getByLabel('User Name').fill(pingAmUsername);
- await page.getByRole('textbox', { name: 'Password' }).fill(pingAmPassword);
- await clickWithRedirect('Next', 'http://localhost:8443/par/**');
+ await page.getByLabel('Password').fill(pingAmPassword);
+ await page.getByRole('button', { name: 'Login (Journey)' }).click();
+ await expect(page.locator('#journey-status')).toContainText('Session established');
- expect(page.url()).toContain('code');
- expect(page.url()).toContain('state');
+ // Step 2: Background PAR auth with prompt=none succeeds silently using existing session
+ await page.getByRole('button', { name: 'Login (Background)' }).click();
await expect(page.locator('#accessToken-0')).not.toBeEmpty();
Or Apply changes locally with:
npx nx-cloud apply-locally JZaG-dtfX
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
e2e/oidc-app/src/par/main.ts (2)
59-61: ⚡ Quick winError detection may not catch all failure cases.
The success check
if (!submitJson.tokenId && !submitJson.successUrl)only throws when both fields are absent. However, AM may return error responses with different structures (e.g.,{ code: 401, message: 'Authentication Failed' }).🛡️ More robust error detection
-if (!submitJson.tokenId && !submitJson.successUrl) { - throw new Error(submitJson.message || 'Login failed'); -} +if (submitJson.code && submitJson.code >= 400) { + throw new Error(submitJson.message || `Login failed with code ${submitJson.code}`); +} +if (!submitJson.tokenId && !submitJson.successUrl) { + throw new Error(submitJson.message || 'Login failed'); +}🤖 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/par/main.ts` around lines 59 - 61, The current check in main.ts using submitJson (if (!submitJson.tokenId && !submitJson.successUrl)) misses alternate error shapes; update the validation around submitJson (the PAR response handling) to treat any response that lacks tokenId and lacks successUrl as an error and also detect common error shapes like presence of a numeric code or a message string (e.g., submitJson.code, submitJson.message) — throw an Error that includes submitJson.message when present otherwise include a safe serialization of submitJson (JSON.stringify(submitJson)) so callers get meaningful diagnostics; ensure this change is applied where submitJson is handled in the PAR response block.
45-48: ⚡ Quick winConsider defensive callback validation.
The code mutates
initJson.callbackswithout first checking whether the array exists or contains the expected callback types. If the journey response structure changes or the Login journey is misconfigured, this could throw a runtime error.🛡️ Add defensive checks
// Fill NameCallback + PasswordCallback -for (const cb of initJson.callbacks ?? []) { +const callbacks = initJson.callbacks ?? []; +const nameCallback = callbacks.find((cb) => cb.type === 'NameCallback'); +const passwordCallback = callbacks.find((cb) => cb.type === 'PasswordCallback'); + +if (!nameCallback || !passwordCallback) { + throw new Error('Expected NameCallback and PasswordCallback not found in journey'); +} + +nameCallback.input[0].value = username; +passwordCallback.input[0].value = password; + +for (const cb of callbacks) { if (cb.type === 'NameCallback') cb.input[0].value = username; if (cb.type === 'PasswordCallback') cb.input[0].value = password; }Alternatively, simpler version that just validates presence:
+if (!initJson.callbacks || initJson.callbacks.length === 0) { + throw new Error('No callbacks returned from journey'); +} + for (const cb of initJson.callbacks ?? []) { if (cb.type === 'NameCallback') cb.input[0].value = username; if (cb.type === 'PasswordCallback') cb.input[0].value = password; }🤖 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/par/main.ts` around lines 45 - 48, The loop over initJson.callbacks in main.ts blindly mutates callback objects and can throw if callbacks is missing or items lack expected shape; update the code that iterates initJson.callbacks to first verify initJson.callbacks is an array and for each cb confirm cb.type is defined and cb.input is an array with at least one element before setting cb.input[0].value, and skip or log a warning for entries that don’t match expected "NameCallback" / "PasswordCallback" shapes so the login flow fails gracefully instead of raising a runtime error.e2e/oidc-suites/src/par.spec.ts (1)
13-13: ⚡ Quick winAdd type annotation for the
pageparameter.The
pageparameter should have an explicit type annotation for consistency with TypeScript best practices.📝 Proposed fix to add type annotation
+import type { Page } from '@playwright/test'; + -async function loginJourney(page, username: string, password: string) { +async function loginJourney(page: Page, username: string, password: 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 `@e2e/oidc-suites/src/par.spec.ts` at line 13, The function loginJourney is missing a TypeScript type for the page parameter; add an explicit Playwright Page type by updating the signature to accept page: Page and ensure you import Page from your Playwright test package (e.g. import { Page } from '@playwright/test' or 'playwright' if not already imported), then update the function declaration loginJourney(page: Page, username: string, password: string) accordingly.packages/oidc-client/src/lib/authorize.request.utils.test.ts (1)
109-224: ⚡ Quick winAdd one PAR regression case for request-level overrides.
All of the new PAR cases use the base config, so they won't catch a mismatch when a call overrides
scope,redirectUri, orresponseType. One assertion against the dispatched PAR body for an overridden value would lock that contract down.🤖 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/authorize.request.utils.test.ts` around lines 109 - 224, Add a PAR regression test that verifies request-level overrides are propagated into the PAR POST body: create a test that calls parAuthorizeµ with par: true and passes overrides (e.g., scope: 'openid profile', redirectUri: 'https://app/callback', responseType: 'code') and stub sessionStorage as other tests do; spy on sdkOidc.buildAuthorizeParams or inspect the first argument passed to mockStore.dispatch to assert the PAR body includes those overridden fields (scope, redirect_uri, response_type) instead of the values from the base config, and keep existing assertions about the returned slim URL.
🤖 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-suites/src/par.spec.ts`:
- Line 64: The selector string passed to clickWithRedirect is missing a closing
parenthesis causing an incomplete button text match; update the call to
clickWithRedirect (in par.spec.ts) to use the full button label "Login
(Redirect)" so it matches the actual button text (consistent with the usage on
line 16) and ensures the selector finds the correct element.
- Line 37: The button selector uses an unterminated regex (/Login \(Background/)
which is inconsistent with the full button text used elsewhere (e.g., 'Login
(Journey)'); update the selector in the call to page.getByRole('button', { name:
... }) to match the complete label — either supply the exact string "Login
(Background)" or a properly escaped/closed regex like /Login \(Background\)/ so
it only targets the intended button.
In `@packages/oidc-client/src/lib/authorize.request.ts`:
- Around line 83-87: The current log.debug call is emitting the raw
authorization payload (data / data.authorizeResponse) which may contain
sensitive fields like code and state; update the logging inside the authorize
response handling (where variables data and data.authorizeResponse are used) to
remove any payload contents and instead log only metadata (e.g., "Received
success response" plus non-sensitive context such as endpoint path, request id,
or status) and do the same for the second occurrence later in the file (the
block around the other log.debug at lines ~139-143) so no authorization payload
fields are ever logged.
In `@packages/oidc-client/src/lib/authorize.request.utils.ts`:
- Around line 157-170: The toAuthorizationError helper currently only checks for
the presence of error and then casts the object to AuthorizationError, which can
yield missing error_description or type; update toAuthorizationError to validate
that data is a non-null object and that the properties error (string),
error_description (string), and type (string) all exist and have correct types
before casting to AuthorizationError (use the Record<string, unknown> shape
check already present as basis); if any of those fields are missing or wrong
type, return the canonical fallback AuthorizationError with error:
'Unknown_Error', error_description: 'Unexpected error response shape', and type:
'unknown_error' so downstream code never sees partial/undefined fields.
- Around line 200-225: generate a single normalized request-level overrides
object (e.g., requestOverrides) that merges config defaults (clientId,
redirectUri, scope, responseType) with the per-call options, then pass that same
requestOverrides into generateAndStoreAuthUrlValues(...) and into
buildAuthorizeParams(...) instead of re-reading config values; specifically,
stop re-setting redirectUri/ scope/responseType from config when building the
PAR body (in the block using authUrlOptions, createChallenge, and
buildAuthorizeParams) and use requestOverrides for those fields so the stored
authUrlValues and the pushed PAR body cannot diverge.
In `@packages/oidc-client/src/lib/client.store.test.ts`:
- Around line 289-603: Tests are missing coverage for error branches in
dispatchAuthorizeµ; add unit tests in client.store.test.ts that trigger and
assert each error-path: (1) simulate the PAR endpoint returning a
CONFIGURATION_ERROR-shaped response and assert dispatchAuthorizeµ returns/passes
through a configuration_error result, (2) simulate non-configuration failure
shapes so createAuthorizeErrorµ is used (e.g., server returns a 400 with
standard OAuth error JSON) and assert the resulting error type
(network_error/argument_error as appropriate), and (3) feed malformed/unexpected
RTK error shapes from the PAR endpoint and assert the fallback error handling
path is exercised; use the existing test helpers (server.use/http.post/http.get
and OIDC client via oidc({ config: ..., storage: ... })) and target
dispatchAuthorizeµ/createAuthorizeErrorµ behavior by calling authorize.url() or
authorize.background() and asserting the returned error objects' types and
payloads.
In `@packages/oidc-client/src/lib/oidc.api.ts`:
- Around line 152-163: The current check "if (!response.data || !('request_uri'
in response.data))" can throw when response.data is a non-object (e.g.,
string/number); update the guard to ensure response.data is an object (e.g.,
typeof response.data === 'object' && response.data !== null) before testing for
the 'request_uri' property so malformed PAR responses return the existing
PAR_ERROR object; locate and change the conditional around
response.data/request_uri in the function that handles the PAR response
(references: response.data and 'request_uri') to perform this type/object check
and keep the same error return path when the guard fails.
---
Nitpick comments:
In `@e2e/oidc-app/src/par/main.ts`:
- Around line 59-61: The current check in main.ts using submitJson (if
(!submitJson.tokenId && !submitJson.successUrl)) misses alternate error shapes;
update the validation around submitJson (the PAR response handling) to treat any
response that lacks tokenId and lacks successUrl as an error and also detect
common error shapes like presence of a numeric code or a message string (e.g.,
submitJson.code, submitJson.message) — throw an Error that includes
submitJson.message when present otherwise include a safe serialization of
submitJson (JSON.stringify(submitJson)) so callers get meaningful diagnostics;
ensure this change is applied where submitJson is handled in the PAR response
block.
- Around line 45-48: The loop over initJson.callbacks in main.ts blindly mutates
callback objects and can throw if callbacks is missing or items lack expected
shape; update the code that iterates initJson.callbacks to first verify
initJson.callbacks is an array and for each cb confirm cb.type is defined and
cb.input is an array with at least one element before setting cb.input[0].value,
and skip or log a warning for entries that don’t match expected "NameCallback" /
"PasswordCallback" shapes so the login flow fails gracefully instead of raising
a runtime error.
In `@e2e/oidc-suites/src/par.spec.ts`:
- Line 13: The function loginJourney is missing a TypeScript type for the page
parameter; add an explicit Playwright Page type by updating the signature to
accept page: Page and ensure you import Page from your Playwright test package
(e.g. import { Page } from '@playwright/test' or 'playwright' if not already
imported), then update the function declaration loginJourney(page: Page,
username: string, password: string) accordingly.
In `@packages/oidc-client/src/lib/authorize.request.utils.test.ts`:
- Around line 109-224: Add a PAR regression test that verifies request-level
overrides are propagated into the PAR POST body: create a test that calls
parAuthorizeµ with par: true and passes overrides (e.g., scope: 'openid
profile', redirectUri: 'https://app/callback', responseType: 'code') and stub
sessionStorage as other tests do; spy on sdkOidc.buildAuthorizeParams or inspect
the first argument passed to mockStore.dispatch to assert the PAR body includes
those overridden fields (scope, redirect_uri, response_type) instead of the
values from the base config, and keep existing assertions about the returned
slim URL.
🪄 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: 382a985d-e9cd-4735-b948-7172731959aa
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
.changeset/some-shirts-joke.mde2e/am-mock-api/src/app/constants.jse2e/am-mock-api/src/app/responses.jse2e/am-mock-api/src/app/routes.auth.jse2e/oidc-app/src/index.htmle2e/oidc-app/src/par/index.htmle2e/oidc-app/src/par/main.tse2e/oidc-app/src/utils/oidc-app.tse2e/oidc-app/vite.config.tse2e/oidc-suites/src/par.spec.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/authorize.request.tspackages/oidc-client/src/lib/authorize.request.utils.test.tspackages/oidc-client/src/lib/authorize.request.utils.tspackages/oidc-client/src/lib/client.store.test.tspackages/oidc-client/src/lib/client.store.tspackages/oidc-client/src/lib/config.types.tspackages/oidc-client/src/lib/oidc.api.tspackages/oidc-client/src/lib/par.types.tspackages/oidc-client/src/types.tspackages/oidc-client/tsconfig.lib.jsonpackages/sdk-effects/oidc/src/lib/authorize.effects.tspackages/sdk-effects/oidc/src/lib/authorize.test.tspackages/sdk-effects/sdk-request-middleware/src/lib/request-mware.derived.ts
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/authorize.request.utils.test.ts`:
- Around line 56-59: The afterEach hook currently calls vi.resetAllMocks(),
which only resets mocks but does not restore spied original implementations
(e.g., the spy created with vi.spyOn(sdkOidc, 'buildAuthorizeParams')); replace
vi.resetAllMocks() with vi.restoreAllMocks() in the afterEach block so all spies
and mocked implementations are restored to their originals while keeping
vi.unstubAllGlobals() as-is.
🪄 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: 7be9d2af-1669-4699-91c5-aa8c866fc957
📒 Files selected for processing (7)
e2e/oidc-suites/src/login.spec.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/oidc-client/src/lib/authorize.request.tspackages/oidc-client/src/lib/authorize.request.utils.test.tspackages/oidc-client/src/lib/authorize.request.utils.tspackages/oidc-client/src/lib/client.store.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/oidc-client/src/lib/authorize.request.ts
- packages/oidc-client/src/lib/client.store.ts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/oidc-client/src/lib/authorize.request.ts (1)
50-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLogging the raw authorize success payload still leaks
code/state.
resulthere is the resolvedAuthorizationSuccessreturned fromdispatchAuthorizeFetch(data.authorizeResponse) ordispatchAuthorizeIframe(data) — both carry the authorizationcode,state, and related artifacts. The earlier reviewed call sites were refactored into this helper, but the same payload-in-debug-log pattern was reintroduced on both branches. Log metadata only.🛡️ Proposed fix
if (options.responseMode === 'pi.flow') { const { responseMode: _, ...redirectOptions } = options; return dispatchAuthorizeFetch(store, url, wellknown, redirectOptions).pipe( - Micro.tap((result) => log.debug('Received success response', result)), + Micro.tap(() => log.debug('Received success response from authorize endpoint (pi.flow)')), ); } return dispatchAuthorizeIframe(store, url, wellknown, options).pipe( - Micro.tap((result) => log.debug('Received success response', result)), + Micro.tap(() => log.debug('Received success response from authorize endpoint (iframe)')), );🤖 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/authorize.request.ts` around lines 50 - 60, The debug taps currently log the full AuthorizationSuccess payload (which contains sensitive fields like code and state) in the Micro.tap callbacks; change those log.debug calls inside the branch that calls dispatchAuthorizeFetch(...) and the branch that calls dispatchAuthorizeIframe(...) to log only non-sensitive metadata (e.g., event type, method used, timestamps, or success status) rather than the raw result object — update the Micro.tap((result) => log.debug('Received success response', result)) invocations to pass a metadata object or formatted message that excludes result.code/result.state while preserving context for debugging.
🧹 Nitpick comments (1)
packages/oidc-client/src/lib/authorize.request.micros.test.ts (1)
337-339: 💤 Low valueConsider simplifying the mock return type cast.
The double cast
as unknown as ReturnType<typeof mockStore.dispatch>works but is complex. Consider defining an explicit type for the mock return value to improve clarity, though the current approach is acceptable for test code.♻️ Optional refactoring approach
Define a helper type for the dispatch result:
type MockDispatchResult<T> = Promise<{ data?: T; error?: unknown }>;Then use it in the mock:
vi.mocked(mockStore.dispatch).mockResolvedValueOnce({ data: { authorizeResponse }, } as MockDispatchResult<{ authorizeResponse: typeof authorizeResponse }>);However, given that this is test code and the current pattern is common for mocking Redux Toolkit types, the existing approach is acceptable.
Also applies to: 355-357
🤖 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/authorize.request.micros.test.ts` around lines 337 - 339, Replace the double cast on the mock return value for vi.mocked(mockStore.dispatch).mockResolvedValueOnce by introducing a small explicit helper type (e.g., MockDispatchResult<T> = Promise<{ data?: T; error?: unknown }>) and use it to type the resolved value for the authorizeResponse mocks; update both occurrences that currently use "as unknown as ReturnType<typeof mockStore.dispatch>" so the mock call reads like mockResolvedValueOnce({...} as MockDispatchResult<{ authorizeResponse: typeof authorizeResponse }>) referencing mockStore.dispatch and the authorizeResponse variable.
🤖 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/par/index.html`:
- Around line 1-5: Add standard meta tags for character encoding and responsive
viewport inside the existing <head> element (before or after the <title>) in the
HTML file: include a meta charset="utf-8" and a meta name="viewport"
content="width=device-width, initial-scale=1". Update the head block that
currently contains the <title> to insert those two meta tags so the page uses
UTF-8 encoding and proper mobile scaling.
In `@packages/oidc-client/src/lib/authorize.request.ts`:
- Around line 78-84: The current Micro.fail call when parEndpoint is missing
uses a sentence for the error field; change it to a machine-readable
SCREAMING_SNAKE code (e.g., PAR_ENDPOINT_NOT_CONFIGURED) so it matches other PAR
errors; modify the Micro.fail invocation in authorize.request.ts where
parEndpoint is checked (the block that currently returns Micro.fail({ error:
'PAR endpoint not configured', ... })) to use the new constant-style error
string for error while keeping the existing error_description and type fields
unchanged.
In `@packages/oidc-client/src/lib/authorize.request.utils.ts`:
- Around line 200-217: The validateParResponse function currently maps any
result.error to toAuthorizationError passing undefined when
isFetchBaseQueryError(result.error) is false, which drops SerializedError
diagnostics; update validateParResponse to mirror
handleDispatchError/toDispatchError behavior: detect if result.error is a
FetchBaseQueryError and convert via toAuthorizationError, otherwise convert the
SerializedError (preserving code/message/name) using toDispatchError (or the
existing toDispatchError helper) before returning Micro.fail; keep the existing
PAR missing-field Micro.fail branch and return Micro.succeed(result.data)
unchanged.
---
Duplicate comments:
In `@packages/oidc-client/src/lib/authorize.request.ts`:
- Around line 50-60: The debug taps currently log the full AuthorizationSuccess
payload (which contains sensitive fields like code and state) in the Micro.tap
callbacks; change those log.debug calls inside the branch that calls
dispatchAuthorizeFetch(...) and the branch that calls
dispatchAuthorizeIframe(...) to log only non-sensitive metadata (e.g., event
type, method used, timestamps, or success status) rather than the raw result
object — update the Micro.tap((result) => log.debug('Received success response',
result)) invocations to pass a metadata object or formatted message that
excludes result.code/result.state while preserving context for debugging.
---
Nitpick comments:
In `@packages/oidc-client/src/lib/authorize.request.micros.test.ts`:
- Around line 337-339: Replace the double cast on the mock return value for
vi.mocked(mockStore.dispatch).mockResolvedValueOnce by introducing a small
explicit helper type (e.g., MockDispatchResult<T> = Promise<{ data?: T; error?:
unknown }>) and use it to type the resolved value for the authorizeResponse
mocks; update both occurrences that currently use "as unknown as
ReturnType<typeof mockStore.dispatch>" so the mock call reads like
mockResolvedValueOnce({...} as MockDispatchResult<{ authorizeResponse: typeof
authorizeResponse }>) referencing mockStore.dispatch and the authorizeResponse
variable.
🪄 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: 578574e5-b6ca-43a5-847b-b6bdd13b3d05
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
.changeset/some-shirts-joke.mde2e/am-mock-api/src/app/constants.jse2e/am-mock-api/src/app/responses.jse2e/am-mock-api/src/app/routes.auth.jse2e/oidc-app/src/index.htmle2e/oidc-app/src/par/index.htmle2e/oidc-app/src/par/main.tse2e/oidc-app/src/utils/oidc-app.tse2e/oidc-app/vite.config.tse2e/oidc-suites/src/login.spec.tse2e/oidc-suites/src/par.spec.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/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/authorize.request.effects.tspackages/oidc-client/src/lib/authorize.request.micros.test.tspackages/oidc-client/src/lib/authorize.request.micros.tspackages/oidc-client/src/lib/authorize.request.tspackages/oidc-client/src/lib/authorize.request.utils.test.tspackages/oidc-client/src/lib/authorize.request.utils.tspackages/oidc-client/src/lib/client.store.test.tspackages/oidc-client/src/lib/client.store.tspackages/oidc-client/src/lib/config.types.tspackages/oidc-client/src/lib/oidc.api.tspackages/oidc-client/src/lib/par.types.tspackages/oidc-client/src/types.tspackages/oidc-client/tsconfig.lib.jsonpackages/sdk-effects/oidc/src/index.tspackages/sdk-effects/oidc/src/lib/authorize.effects.tspackages/sdk-effects/oidc/src/lib/authorize.test.tspackages/sdk-effects/oidc/src/lib/authorize.utils.tspackages/sdk-effects/sdk-request-middleware/src/lib/request-mware.derived.ts
✅ Files skipped from review due to trivial changes (6)
- e2e/oidc-app/vite.config.ts
- packages/oidc-client/src/types.ts
- packages/oidc-client/src/lib/par.types.ts
- e2e/oidc-app/src/index.html
- .changeset/some-shirts-joke.md
- e2e/oidc-suites/src/login.spec.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/sdk-effects/sdk-request-middleware/src/lib/request-mware.derived.ts
- packages/oidc-client/src/lib/config.types.ts
- packages/oidc-client/tsconfig.lib.json
- e2e/am-mock-api/src/app/responses.js
- packages/oidc-client/api-report/oidc-client.types.api.md
- packages/oidc-client/src/lib/oidc.api.ts
- packages/sdk-effects/oidc/src/lib/authorize.test.ts
- packages/oidc-client/src/lib/client.store.test.ts
- e2e/am-mock-api/src/app/routes.auth.js
- e2e/oidc-app/src/utils/oidc-app.ts
- e2e/oidc-app/src/par/main.ts
- packages/oidc-client/api-report/oidc-client.api.md
- e2e/oidc-suites/src/par.spec.ts
- packages/oidc-client/package.json
- packages/oidc-client/src/lib/client.store.ts
- packages/oidc-client/src/lib/authorize.request.utils.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/oidc-client/src/lib/authorize.request.utils.test.ts (1)
67-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore spies instead of resetting them in test cleanup.
On Line 69,
vi.resetAllMocks()does not restore original implementations forvi.spyOn(...), which can leak altered behavior across tests. Usevi.restoreAllMocks()in this hook.Proposed fix
afterEach(() => { vi.unstubAllGlobals(); - vi.resetAllMocks(); + vi.restoreAllMocks(); });🤖 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/authorize.request.utils.test.ts` around lines 67 - 70, The test cleanup currently calls vi.resetAllMocks() which does not restore original implementations for spies created with vi.spyOn; in the afterEach hook replace vi.resetAllMocks() with vi.restoreAllMocks() so that spies are properly restored while keeping vi.unstubAllGlobals() intact; update the afterEach closure (the one containing vi.unstubAllGlobals() and vi.resetAllMocks()) to call vi.restoreAllMocks() instead of vi.resetAllMocks() to prevent spy leakage across tests.
🧹 Nitpick comments (1)
e2e/oidc-suites/src/par.spec.ts (1)
63-66: ⚡ Quick winAssert no-redirect behavior explicitly in the PAR 400 test.
The test name promises "without redirecting", but current checks only assert the error UI. Add a URL assertion to lock that contract.
Proposed test assertion update
// The SDK should surface an error in the UI instead of redirecting away + await expect(page).not.toHaveURL(/\/am\/XUI\//); + await expect(page).toHaveURL(/\/par\/(?:\?.*)?$/); await expect(page.locator('.error')).toBeVisible({ timeout: 10000 }); await expect(page.locator('.error')).toContainText('PAR_ERROR');🤖 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/par.spec.ts` around lines 63 - 66, The test currently checks only the error UI (page.locator('.error')) but doesn't assert the "no-redirect" promise; capture the page URL before the action that triggers PAR and after the error appears and assert they are identical to ensure there was no navigation. Specifically, in par.spec.ts around the PAR 400 test, record the URL via page.url() (or use expect(page).toHaveURL(...) semantics) before invoking the flow that produces the PAR error, then after the existing checks (await expect(page.locator('.error')).toBeVisible... and toContainText('PAR_ERROR')) add an assertion that the current URL equals the captured URL to lock the no-redirect contract.
🤖 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.
Duplicate comments:
In `@packages/oidc-client/src/lib/authorize.request.utils.test.ts`:
- Around line 67-70: The test cleanup currently calls vi.resetAllMocks() which
does not restore original implementations for spies created with vi.spyOn; in
the afterEach hook replace vi.resetAllMocks() with vi.restoreAllMocks() so that
spies are properly restored while keeping vi.unstubAllGlobals() intact; update
the afterEach closure (the one containing vi.unstubAllGlobals() and
vi.resetAllMocks()) to call vi.restoreAllMocks() instead of vi.resetAllMocks()
to prevent spy leakage across tests.
---
Nitpick comments:
In `@e2e/oidc-suites/src/par.spec.ts`:
- Around line 63-66: The test currently checks only the error UI
(page.locator('.error')) but doesn't assert the "no-redirect" promise; capture
the page URL before the action that triggers PAR and after the error appears and
assert they are identical to ensure there was no navigation. Specifically, in
par.spec.ts around the PAR 400 test, record the URL via page.url() (or use
expect(page).toHaveURL(...) semantics) before invoking the flow that produces
the PAR error, then after the existing checks (await
expect(page.locator('.error')).toBeVisible... and toContainText('PAR_ERROR'))
add an assertion that the current URL equals the captured URL to lock the
no-redirect contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ad87a678-4a47-4351-bd39-8c7c811ce636
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (34)
.changeset/some-shirts-joke.mde2e/am-mock-api/src/app/constants.jse2e/am-mock-api/src/app/responses.jse2e/am-mock-api/src/app/routes.auth.jse2e/oidc-app/src/index.htmle2e/oidc-app/src/par/index.htmle2e/oidc-app/src/par/main.tse2e/oidc-app/src/utils/oidc-app.tse2e/oidc-app/vite.config.tse2e/oidc-suites/src/login.spec.tse2e/oidc-suites/src/par.spec.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/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/authorize.request.effects.tspackages/oidc-client/src/lib/authorize.request.micros.test.tspackages/oidc-client/src/lib/authorize.request.micros.tspackages/oidc-client/src/lib/authorize.request.tspackages/oidc-client/src/lib/authorize.request.utils.test.tspackages/oidc-client/src/lib/authorize.request.utils.tspackages/oidc-client/src/lib/client.store.test.tspackages/oidc-client/src/lib/client.store.tspackages/oidc-client/src/lib/config.types.tspackages/oidc-client/src/lib/oidc.api.tspackages/oidc-client/src/lib/par.types.tspackages/oidc-client/src/types.tspackages/oidc-client/tsconfig.lib.jsonpackages/sdk-effects/oidc/src/index.tspackages/sdk-effects/oidc/src/lib/authorize.effects.tspackages/sdk-effects/oidc/src/lib/authorize.test.tspackages/sdk-effects/oidc/src/lib/authorize.utils.tspackages/sdk-effects/sdk-request-middleware/src/lib/request-mware.derived.ts
✅ Files skipped from review due to trivial changes (5)
- packages/oidc-client/tsconfig.lib.json
- e2e/oidc-suites/src/login.spec.ts
- packages/oidc-client/package.json
- packages/oidc-client/api-report/oidc-client.api.md
- .changeset/some-shirts-joke.md
🚧 Files skipped from review as they are similar to previous changes (21)
- e2e/oidc-app/vite.config.ts
- e2e/am-mock-api/src/app/responses.js
- packages/oidc-client/src/types.ts
- packages/oidc-client/src/lib/par.types.ts
- packages/oidc-client/src/lib/config.types.ts
- e2e/oidc-app/src/index.html
- e2e/oidc-app/src/utils/oidc-app.ts
- e2e/am-mock-api/src/app/routes.auth.js
- packages/oidc-client/src/lib/oidc.api.ts
- e2e/oidc-app/src/par/index.html
- packages/sdk-effects/oidc/src/lib/authorize.test.ts
- packages/sdk-effects/oidc/src/lib/authorize.utils.ts
- e2e/am-mock-api/src/app/constants.js
- packages/oidc-client/src/lib/authorize.request.ts
- packages/oidc-client/src/lib/authorize.request.effects.ts
- packages/oidc-client/api-report/oidc-client.types.api.md
- packages/oidc-client/src/lib/authorize.request.micros.test.ts
- e2e/oidc-app/src/par/main.ts
- packages/oidc-client/src/lib/authorize.request.micros.ts
- packages/oidc-client/src/lib/authorize.request.utils.ts
- packages/oidc-client/src/lib/client.store.ts
Implements RFC 9126 PAR flow: client POSTs authorization parameters to the server's pushed_authorization_request_endpoint, receives a request_uri, and uses only that in the redirect — keeping sensitive parameters out of browser history and server logs. - New parAuthorizeµ pipeline: builds PAR body, dispatches POST, validates response, stores PKCE state, builds slim authorize URL - authorizeµ selects PAR vs standard flow via config.par or wellknown.require_pushed_authorization_requests - New par.types.ts, authorize.request.micros.ts, authorize.request.effects.ts layer split - PAR endpoint added to oidc.api.ts (RTK Query mutation) - e2e suite and app added (e2e/oidc-app/src/par/, oidc-suites/par.spec.ts)
cerebrl
requested changes
May 20, 2026
Collaborator
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
I'm still going through it, but I thought I'd provide what I have so far.
|
|
||
| const response = await initQuery(request, 'par') | ||
| .applyMiddleware(requestMiddleware) | ||
| .applyQuery(async (req: FetchArgs) => await baseQuery(req)); |
Collaborator
There was a problem hiding this comment.
Can we get a debug log printing out the raw response? I want to have one for the request just before we use initQuery, and one for the response right after, before we have any logic.
| }; | ||
| } | ||
|
|
||
| logger.debug('OIDC PAR API response', response); |
Collaborator
There was a problem hiding this comment.
Ah, can we just move this up?
| wellknown: WellknownResponse, | ||
| options?: OptionalAuthorizeOptions, | ||
| ): Micro.Micro<ReturnType<typeof generateAndStoreAuthUrlValues>, AuthorizationError, never> => | ||
| Micro.try({ |
Collaborator
There was a problem hiding this comment.
It took me an embarrassing long time to realize this Micro was being returned, so it was a bit confusing for me. I then noticed the missing braces. Anyway, maybe I'm a boomer, so take this with a grain of salt, but for larger function bodies, I really prefer the use of braces and a return statement.
Collaborator
There was a problem hiding this comment.
How do you feel about merging .micro.ts with .effect.ts? Let's just say get rid of the Effect Micro-Pattern, and replace it with "Micro" Micro-Pattern. Micros can be both effectful and non-effectful. So, for internal patterns, it would look like this:
- Store: factory function that returns public API
- API: network orchestration and request-response mgmt
- Reducers/Slices: state transformation/transition
- Micro: functions that return a
Micro(optionally effectful; non-network effects) - Utilities: pure, stateless functions
- Types: interfaces and types
The Effect Macro-Pattern will still be a thing, but let's swap out Micro for Effect at the Micro level since there's really not a difference, and we can expose Micro internally, just not externally.
Comment on lines
+120
to
+124
| /** | ||
| * Background authorization flow. Uses PAR when the server mandates it | ||
| * (`require_pushed_authorization_requests`) or `config.par` is set; otherwise | ||
| * builds a full authorize URL and dispatches via iframe or pi.flow fetch. | ||
| */ |
Collaborator
There was a problem hiding this comment.
Why did we remove the detailed JSDoc comment? Are we no longer generating API docs from these comments?
| } as const; | ||
| }, | ||
| ): Micro.Micro<never, AuthorizationError, never> => | ||
| Micro.tryPromise({ |
Collaborator
There was a problem hiding this comment.
Okay, I see what you're saying in the other thread we have. So, my vision is that Micro, the pattern and library, can use Utilities, but Utilities can't use Micro.
With that said, let's remove the use of Micro, or move these functions to the micro.ts file.
Collaborator
There was a problem hiding this comment.
Plus, if a function returns a Micro, it needs to communicate that with the use of the µ symbol.
| challenge: string, | ||
| state: string, | ||
| prompt?: PromptValue, | ||
| ): Micro.Micro<URLSearchParams, AuthorizationError, never> => |
Collaborator
There was a problem hiding this comment.
Let's just create a clear delineation with a Utility from a Micro, so let's have the rule that if Micro is used, it's a Micro.
ryanbas21
force-pushed
the
par
branch
2 times, most recently
from
f6de759 to
2f3564a
Compare
…ror handling - Fix: spurious await on Micro value in client.store authorize path - Fix: validateParResponse uses toDispatchError for SerializedError, preserving diagnostic info instead of silently dropping it - Fix: optional chaining on wellknown?.require_pushed_authorization_requests in authorizeµ default param (prevents synchronous throw outside Micro) - Fix: scope PAR dispatch tapError inside flatMap to prevent double-logging with misleading "Error dispatching PAR authorize request" message - Fix: dispatchAuthorizeIframe validates code+state shape before succeeding - Fix: add credentials: include to PAR endpoint FetchArgs for SSO support - Fix: correct misleading log in authorizeIframe error path - Feat: surface expires_in from PAR response; warn when TTL < 30s - Test: add PAR 400 error e2e scenario to mock server and par.spec.ts - Chore: remove console.log debug artifacts from client.store.test.ts fix(oidc-app): add charset and viewport meta tags to PAR demo page fix(oidc-suites): tighten PAR button selectors with anchored regex fix(oidc-client): use PAR_NOT_CONFIGURED error code for missing PAR endpoint fix(oidc-client): redact authorization payload from authorize success debug logs docs(oidc-client): restore JSDoc on authorizeµ and parAuthorizeµ fix(oidc-client): guard PAR response shape before reading request_uri fix(oidc-client): bracket PAR queryFn with raw request and raw response debug logs test(oidc-client): use vi.restoreAllMocks so spyOn originals are restored between tests test(oidc-client): cover dispatchAuthorizeµ error branches via authorize.background() refactor(oidc-client): merge effects.ts into micros.ts and µ-suffix all Micro exports Consolidate authorize.request.effects.ts and existing micros into a single file. Every function returning Micro is now µ-suffixed and uses braces + explicit return for clarity. refactor(oidc-client): move Micro helpers into micros.ts; utils.ts holds only pure utilities Establish the convention: any function returning a Micro lives in authorize.request.micros.ts and ends in µ. authorize.request.utils.ts now contains only pure stateless utilities (type guards, error converters, URL builders) with no dependency on the Effect library.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add support for par in oidc client.
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4236
Description
Adds support for PAR for redirect based flows in oidc-client
Summary by CodeRabbit
New Features
User Interface
Tests
Chores