Conversation
🦋 Changeset detectedLatest commit: a3bcb7a 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 selected for processing (6)
📝 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 f38ce4e
☁️ 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.49%) 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.49% +1.42%
==========================================
Files 155 160 +5
Lines 24398 24782 +384
Branches 1203 1318 +115
==========================================
+ Hits 4410 4832 +422
+ Misses 19988 19950 -38
🚀 New features to boost your workflow:
|
Contributor
|
Deployed 2b472c8 to https://ForgeRock.github.io/ping-javascript-sdk/pr-631/2b472c8311de8949d0d040cfbd9125047022a3c8 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
| await loginJourney(page, pingAmUsername, pingAmPassword); | ||
|
|
||
| // Background button is now enabled — click and wait for the iframe to return a code | ||
| await page.getByRole('button', { name: /Login \(Background/ }).click(); |
There was a problem hiding this comment.
Incomplete button text selector pattern.
The button selector uses /Login \(Background/ without a closing parenthesis, which differs from the complete button text used on line 16 ('Login (Journey)'). While this partial match may work, it's inconsistent and could match unintended buttons.
🔧 Proposed fix for consistent button selector
- await page.getByRole('button', { name: /Login \(Background/ }).click();
+ await page.getByRole('button', { name: /Login \(Background\)/ }).click();📝 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.
Suggested change
| await page.getByRole('button', { name: /Login \(Background/ }).click(); | |
| await page.getByRole('button', { name: /Login \(Background\)/ }).click(); |
🤖 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 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.
|
|
||
| await navigate('/par/'); | ||
|
|
||
| await clickWithRedirect('Login (Redirect', '**/am/XUI/**'); |
There was a problem hiding this comment.
Incomplete button text selector.
The button text 'Login (Redirect' is missing the closing parenthesis. For consistency with line 16 and clarity, the complete button text should be used.
🔧 Proposed fix for complete button text
- await clickWithRedirect('Login (Redirect', '**/am/XUI/**');
+ await clickWithRedirect('Login (Redirect)', '**/am/XUI/**');📝 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.
Suggested change
| await clickWithRedirect('Login (Redirect', '**/am/XUI/**'); | |
| await clickWithRedirect('Login (Redirect)', '**/am/XUI/**'); |
🤖 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 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.
Comment on lines
+152
to
+163
| if (!response.data || !('request_uri' in response.data)) { | ||
| return { | ||
| error: { | ||
| status: 'CUSTOM_ERROR', | ||
| error: 'PAR_ERROR', | ||
| data: { | ||
| error: 'PAR_ERROR', | ||
| error_description: "PAR response missing required 'request_uri' field", | ||
| type: 'network_error', | ||
| }, | ||
| } as FetchBaseQueryError, | ||
| }; |
There was a problem hiding this comment.
Guard PAR success payload shape before checking request_uri.
Line 152 can throw if response.data is a non-object JSON value. Add an object/type guard so malformed PAR responses return PAR_ERROR instead of crashing this path.
💡 Proposed fix
- if (!response.data || !('request_uri' in response.data)) {
+ const data = response.data;
+ if (
+ !data ||
+ typeof data !== 'object' ||
+ !('request_uri' in data) ||
+ typeof data.request_uri !== 'string' ||
+ data.request_uri.length === 0
+ ) {
return {
error: {
status: 'CUSTOM_ERROR',
error: 'PAR_ERROR',
data: {
error: 'PAR_ERROR',
error_description: "PAR response missing required 'request_uri' field",
type: 'network_error',
},
} as FetchBaseQueryError,
};
}
logger.debug('OIDC PAR API response', response);
- return response as { data: PushAuthorizationResponse };
+ return response as { data: PushAuthorizationResponse };📝 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.
Suggested change
| if (!response.data || !('request_uri' in response.data)) { | |
| return { | |
| error: { | |
| status: 'CUSTOM_ERROR', | |
| error: 'PAR_ERROR', | |
| data: { | |
| error: 'PAR_ERROR', | |
| error_description: "PAR response missing required 'request_uri' field", | |
| type: 'network_error', | |
| }, | |
| } as FetchBaseQueryError, | |
| }; | |
| const data = response.data; | |
| if ( | |
| !data || | |
| typeof data !== 'object' || | |
| !('request_uri' in data) || | |
| typeof data.request_uri !== 'string' || | |
| data.request_uri.length === 0 | |
| ) { | |
| return { | |
| error: { | |
| status: 'CUSTOM_ERROR', | |
| error: 'PAR_ERROR', | |
| data: { | |
| error: 'PAR_ERROR', | |
| error_description: "PAR response missing required 'request_uri' field", | |
| type: 'network_error', | |
| }, | |
| } as FetchBaseQueryError, | |
| }; | |
| } |
🤖 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 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.
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
Comment on lines
+56
to
+59
| afterEach(() => { | ||
| vi.unstubAllGlobals(); | ||
| vi.resetAllMocks(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does vitest's vi.resetAllMocks restore vi.spyOn original implementations, or is vi.restoreAllMocks required?
💡 Result:
vi.resetAllMocks does not restore vi.spyOn spies to their original implementations; it calls.mockReset on all spies, which clears history and resets each mock’s implementation (i.e., to an “empty”/reset implementation), not to the original function implementation. [1][2] If you want spies created with vi.spyOn to go back to the original implementations, you must use vi.restoreAllMocks (it calls.mockRestore on all spies, restoring the original implementations and descriptors of spied-on objects). [1][2] Recommended cleanup pattern: - afterEach( => { vi.restoreAllMocks }) to restore spies after each test. [3]
Citations:
- 1: https://vitest.dev/api/vi
- 2: https://vitest.dev/api/vi.html
- 3: https://v3.vitest.dev/guide/mocking
🏁 Script executed:
cd packages/oidc-client/src/lib && wc -l authorize.request.utils.test.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 109
🏁 Script executed:
cd packages/oidc-client/src/lib && sed -n '50,65p' authorize.request.utils.test.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 526
🏁 Script executed:
cd packages/oidc-client/src/lib && sed -n '210,220p' authorize.request.utils.test.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 562
🏁 Script executed:
cd packages/oidc-client/src/lib && grep -n "^it\|^describe" authorize.request.utils.test.ts | wc -lRepository: ForgeRock/ping-javascript-sdk
Length of output: 76
🏁 Script executed:
cd packages/oidc-client/src/lib && grep -n "sdkOidc" authorize.request.utils.test.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 200
🏁 Script executed:
cd packages/oidc-client/src/lib && sed -n '1,20p' authorize.request.utils.test.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 783
🏁 Script executed:
cd packages/oidc-client/src/lib && grep -n "vi.spyOn\|buildAuthorizeParams" authorize.request.utils.test.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 149
🏁 Script executed:
cd packages/oidc-client/src/lib && sed -n '210,230p' authorize.request.utils.test.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 880
Switch vi.resetAllMocks() to vi.restoreAllMocks() in the afterEach hook.
vi.resetAllMocks() calls .mockReset() on all mocked functions, which clears mock history and replaces the implementation with a default mock value, but does not restore the original implementation. The spy at line 216 (vi.spyOn(sdkOidc, 'buildAuthorizeParams')) will therefore remain in a reset state after each test, not restored to the original module export. This breaks any subsequent test (in this file or across files with shared module instances) that depends on the real implementation.
Use vi.restoreAllMocks() instead, which calls .mockRestore() on all spies to properly restore original implementations and descriptors.
♻️ 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 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.
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
Comment on lines
+1
to
+5
| <!doctype html> | ||
| <html> | ||
| <head> | ||
| <title>E2E Test | Ping Identity JavaScript SDK</title> | ||
|
|
There was a problem hiding this comment.
Add charset and viewport meta tags.
The HTML is missing standard meta tags that ensure proper character encoding and responsive rendering. These are essential even for E2E test pages to prevent rendering issues.
🌐 Proposed fix to add meta tags
<html>
<head>
+ <meta charset="utf-8" />
+ <meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>E2E Test | Ping Identity JavaScript SDK</title>📝 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.
Suggested change
| <!doctype html> | |
| <html> | |
| <head> | |
| <title>E2E Test | Ping Identity JavaScript SDK</title> | |
| <!doctype html> | |
| <html> | |
| <head> | |
| <meta charset="utf-8" /> | |
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | |
| <title>E2E Test | Ping Identity JavaScript SDK</title> | |
🤖 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/index.html` around lines 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.
Comment on lines
+78
to
+84
| if (!parEndpoint) { | ||
| return Micro.fail({ | ||
| error: 'PAR endpoint not configured', | ||
| error_description: 'PAR endpoint not found in server configuration', | ||
| type: 'wellknown_error', | ||
| } as const); | ||
| } |
There was a problem hiding this comment.
Use a machine-readable error code for the missing PAR endpoint case.
error: 'PAR endpoint not configured' is a sentence, not a stable identifier. Every other PAR failure in this stack uses a SCREAMING_SNAKE code (PAR_DISPATCH_ERROR, PAR_URL_BUILD_ERROR, PAR_PARAM_BUILD_ERROR, PAR_STORAGE_ERROR, PAR_CHALLENGE_ERROR). Consumers programmatically discriminating on error will break for this case.
♻️ Proposed fix
if (!parEndpoint) {
return Micro.fail({
- error: 'PAR endpoint not configured',
+ error: 'PAR_ENDPOINT_NOT_CONFIGURED',
error_description: 'PAR endpoint not found in server configuration',
type: 'wellknown_error',
} as const);
}📝 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.
Suggested change
| if (!parEndpoint) { | |
| return Micro.fail({ | |
| error: 'PAR endpoint not configured', | |
| error_description: 'PAR endpoint not found in server configuration', | |
| type: 'wellknown_error', | |
| } as const); | |
| } | |
| if (!parEndpoint) { | |
| return Micro.fail({ | |
| error: 'PAR_ENDPOINT_NOT_CONFIGURED', | |
| error_description: 'PAR endpoint not found in server configuration', | |
| type: 'wellknown_error', | |
| } as const); | |
| } |
🤖 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 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.
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)
…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
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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