feat(journey-client): webauthn conditional mediation autofill passkey support …#581
feat(journey-client): webauthn conditional mediation autofill passkey support …#581vatsalparikh merged 4 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 8d314c6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds conditional WebAuthn mediation support: API gains optional AbortSignal and mediation fields; runtime chooses conditional flow when step metadata requests it and the browser supports it (with cancellation and error semantics). E2E inputs get Changes
Sequence DiagramsequenceDiagram
participant App as Journey App
participant Handler as WebAuthn Handler
participant Browser as Browser Capability\n(PublicKeyCredential.isConditionalMediationAvailable)
participant Creds as navigator.credentials
participant Server as Backend
App->>Handler: handleWebAuthnStep(step)
Handler->>App: render callbacks (input[autocomplete="webauthn"])
Handler->>Browser: isConditionalMediationSupported()
alt unsupported
Browser-->>Handler: false
Handler->>App: fall back to prompted flow (webauthnComponent) -> submitForm()
else supported
Browser-->>Handler: true
Handler->>Creds: get({mediation:'conditional', signal, ...})
alt credential returned
Creds-->>Handler: PublicKeyCredential
Handler->>Server: submitForm()
Server-->>Handler: response
Handler-->>App: didSubmit: true
else abort or error
Creds-->>Handler: AbortError / Error
Handler-->>App: setError(...), didSubmit: false
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
vatsalparikh
requested review from
SteinGabriel,
ancheetah,
cerebrl and
ryanbas21
|
View your CI Pipeline Execution ↗ for commit 72550da
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
e2e/journey-app/components/webauthn-step.ts (1)
68-69: Redundant conditional-mediation support check.
WebAuthn.authenticate(..., 'conditional', signal)already callsisConditionalMediationSupported()internally and throwsNotSupportedErrorif unsupported. Calling it here too means two async feature-detection round-trips per auth step. Consider caching the result, or relying onauthenticate()'s internal check and falling back on theNotSupportedErrorrejection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-app/components/webauthn-step.ts` around lines 68 - 69, The extra await WebAuthn.isConditionalMediationSupported() + if (isConditionalSupported && conditionalInput) is redundant and causes double feature-detection; remove that pre-check and always call WebAuthn.authenticate(..., 'conditional', signal) when conditionalInput is available, then catch the rejection and detect a NotSupportedError (or exception.name === 'NotSupportedError') to fall back to the non-conditional path. If you care about avoiding repeated detection calls, add a small cache (e.g., a module-level cachedConditionalSupported flag checked/updated when WebAuthn.isConditionalMediationSupported() is first called) and use it instead of awaiting every time. Ensure references to WebAuthn.authenticate, WebAuthn.isConditionalMediationSupported, and the conditionalInput handling are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/journey-app/components/webauthn-step.ts`:
- Around line 70-78: handleWebAuthnStep currently launches
WebAuthn.authenticate(step, 'conditional', controller.signal) with a local
AbortController that is never aborted, so the in-flight promise can later call
submitForm() or setError() against a stale step; fix by exposing and using an
abort mechanism (e.g., return or register the controller via a module-level
hook) so callers (main.ts and the submit handler) can call controller.abort()
whenever renderForm()/step changes, the form is submitted, or the component is
torn down; specifically ensure the AbortController created in handleWebAuthnStep
(and used in WebAuthn.authenticate) is aborted from main.ts before calling
journeyClient.next(...) and when swapping steps to prevent stale
submitForm()/setError() invocations.
- Around line 71-75: The catch block on WebAuthn.authenticate currently treats
every rejection as a user-facing error; change it so AbortError is ignored: in
the promise rejection handler for WebAuthn.authenticate(step, 'conditional',
controller.signal) check the thrown error (e.g., if (err?.name === 'AbortError')
return;), and only call setError('WebAuthn failed or was cancelled. Please try
again or use a different method.') for non-AbortError failures so cancellations
from the conditional flow (controller.signal/step changes) are not surfaced to
the user; keep successful flow calling submitForm() unchanged.
---
Nitpick comments:
In `@e2e/journey-app/components/webauthn-step.ts`:
- Around line 68-69: The extra await WebAuthn.isConditionalMediationSupported()
+ if (isConditionalSupported && conditionalInput) is redundant and causes double
feature-detection; remove that pre-check and always call
WebAuthn.authenticate(..., 'conditional', signal) when conditionalInput is
available, then catch the rejection and detect a NotSupportedError (or
exception.name === 'NotSupportedError') to fall back to the non-conditional
path. If you care about avoiding repeated detection calls, add a small cache
(e.g., a module-level cachedConditionalSupported flag checked/updated when
WebAuthn.isConditionalMediationSupported() is first called) and use it instead
of awaiting every time. Ensure references to WebAuthn.authenticate,
WebAuthn.isConditionalMediationSupported, and the conditionalInput handling are
updated accordingly.
🪄 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: 8f68f8e4-60dd-4731-ad4c-0ede19ab8556
📒 Files selected for processing (8)
.changeset/ready-snakes-sell.mde2e/journey-app/components/text-input.tse2e/journey-app/components/validated-username.tse2e/journey-app/components/webauthn-step.tse2e/journey-app/main.tspackages/journey-client/api-report/journey-client.webauthn.api.mdpackages/journey-client/src/lib/webauthn/webauthn.test.tspackages/journey-client/src/lib/webauthn/webauthn.ts
| const controller = new AbortController(); | ||
| void WebAuthn.authenticate(step, 'conditional', controller.signal) | ||
| .then(() => submitForm()) | ||
| .catch(() => { | ||
| setError('WebAuthn failed or was cancelled. Please try again or use a different method.'); | ||
| }); | ||
|
|
||
| return { callbacksRendered: true, didSubmit: false }; | ||
| } |
There was a problem hiding this comment.
Stale in-flight conditional authenticate can submit or error against a newer step.
handleWebAuthnStep returns { callbacksRendered: true, didSubmit: false } while the WebAuthn.authenticate(step, 'conditional', controller.signal) promise keeps running in the background. The AbortController is created but never aborted, so if the user submits via a different method (e.g., username/password on the same step) or renderForm() is invoked again for a new step, the in-flight conditional request can still resolve later and call submitForm() on a now-stale step, or call setError() over the new UI. You should abort the controller when the step changes / form is submitted / component is torn down.
Suggested direction
Expose an abort hook (or module-level controller) so main.ts can cancel the in-flight conditional request before navigating to the next step, and abort it inside the submit handler in main.ts before calling journeyClient.next(...).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/journey-app/components/webauthn-step.ts` around lines 70 - 78,
handleWebAuthnStep currently launches WebAuthn.authenticate(step, 'conditional',
controller.signal) with a local AbortController that is never aborted, so the
in-flight promise can later call submitForm() or setError() against a stale
step; fix by exposing and using an abort mechanism (e.g., return or register the
controller via a module-level hook) so callers (main.ts and the submit handler)
can call controller.abort() whenever renderForm()/step changes, the form is
submitted, or the component is torn down; specifically ensure the
AbortController created in handleWebAuthnStep (and used in
WebAuthn.authenticate) is aborted from main.ts before calling
journeyClient.next(...) and when swapping steps to prevent stale
submitForm()/setError() invocations.
| void WebAuthn.authenticate(step, 'conditional', controller.signal) | ||
| .then(() => submitForm()) | ||
| .catch(() => { | ||
| setError('WebAuthn failed or was cancelled. Please try again or use a different method.'); | ||
| }); |
There was a problem hiding this comment.
Don't surface an error for AbortError in the conditional flow.
With conditional mediation, authenticate() intentionally rethrows AbortError without mutating the hidden outcome because cancellations are expected (user picked a different method, step changed, controller aborted). The blanket .catch(() => setError(...)) here turns every cancellation into a user-visible "WebAuthn failed or was cancelled" message, which will be noisy during normal autofill UX.
- void WebAuthn.authenticate(step, 'conditional', controller.signal)
- .then(() => submitForm())
- .catch(() => {
- setError('WebAuthn failed or was cancelled. Please try again or use a different method.');
- });
+ void WebAuthn.authenticate(step, 'conditional', controller.signal)
+ .then(() => submitForm())
+ .catch((err: unknown) => {
+ if (err instanceof Error && err.name === 'AbortError') return;
+ setError('WebAuthn failed or was cancelled. Please try again or use a different method.');
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/journey-app/components/webauthn-step.ts` around lines 71 - 75, The catch
block on WebAuthn.authenticate currently treats every rejection as a user-facing
error; change it so AbortError is ignored: in the promise rejection handler for
WebAuthn.authenticate(step, 'conditional', controller.signal) check the thrown
error (e.g., if (err?.name === 'AbortError') return;), and only call
setError('WebAuthn failed or was cancelled. Please try again or use a different
method.') for non-AbortError failures so cancellations from the conditional flow
(controller.signal/step changes) are not surfaced to the user; keep successful
flow calling submitForm() unchanged.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (5.88%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #581 +/- ##
===========================================
- Coverage 70.90% 17.48% -53.42%
===========================================
Files 53 154 +101
Lines 2021 24195 +22174
Branches 377 1146 +769
===========================================
+ Hits 1433 4231 +2798
- Misses 588 19964 +19376
🚀 New features to boost your workflow:
|
@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: |
|
Deployed 4cdb392 to https://ForgeRock.github.io/ping-javascript-sdk/pr-581/4cdb39250bff1828e3c5c84449aa3255763af1b8 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/device-client - 0.0 KB (-10.0 KB, -100.0%) ➖ No Changes➖ @forgerock/device-client - 10.0 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/journey-client/src/lib/webauthn/webauthn.ts (1)
355-371: Optional: mirror the signal guard insidegetAuthenticationCredentialfor defense-in-depth.
authenticate()enforces thatmediation === 'conditional'requires anAbortSignal, butgetAuthenticationCredentialis apublic staticmethod on an exported class (surfaced injourney-client.webauthn.api.md). A caller invoking it directly withmediation: 'conditional'and no signal will get a browser-level rejection, which is acceptable, but a thin guard here would make the public API self-consistent and keep error messages uniform. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journey-client/src/lib/webauthn/webauthn.ts` around lines 355 - 371, Add a defensive guard in getAuthenticationCredential: before calling navigator.credentials.get, check if mediation === 'conditional' and signal is falsy, and if so throw an Error with a clear message like "AbortSignal required when mediation === 'conditional'" and set the error name to WebAuthnOutcomeType.InvalidStateError (mirroring the enforcement in authenticate); place this check in the getAuthenticationCredential method just after the PublicKeyCredential feature check and before the call to navigator.credentials.get so callers who invoke getAuthenticationCredential directly get a consistent, descriptive error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/journey-client/src/lib/webauthn/webauthn.ts`:
- Around line 355-371: Add a defensive guard in getAuthenticationCredential:
before calling navigator.credentials.get, check if mediation === 'conditional'
and signal is falsy, and if so throw an Error with a clear message like
"AbortSignal required when mediation === 'conditional'" and set the error name
to WebAuthnOutcomeType.InvalidStateError (mirroring the enforcement in
authenticate); place this check in the getAuthenticationCredential method just
after the PublicKeyCredential feature check and before the call to
navigator.credentials.get so callers who invoke getAuthenticationCredential
directly get a consistent, descriptive error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f1390bc-a634-4fc6-9b70-94c33f98dafb
📒 Files selected for processing (8)
.changeset/ready-snakes-sell.mde2e/journey-app/components/text-input.tse2e/journey-app/components/validated-username.tse2e/journey-app/components/webauthn-step.tse2e/journey-app/main.tspackages/journey-client/api-report/journey-client.webauthn.api.mdpackages/journey-client/src/lib/webauthn/webauthn.test.tspackages/journey-client/src/lib/webauthn/webauthn.ts
✅ Files skipped from review due to trivial changes (2)
- e2e/journey-app/components/text-input.ts
- e2e/journey-app/components/validated-username.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/ready-snakes-sell.md
- e2e/journey-app/main.ts
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
Before further review, I needed to ask a question.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/journey-client/api-report/journey-client.webauthn.api.md (1)
95-95: ⚡ Quick winAdd JSDoc documentation for new APIs.
The modified and new APIs are marked
// (undocumented), indicating missing JSDoc comments in the source. Since these are public APIs involving a new feature (conditional mediation), documentation would help consumers understand:
- When
signalis required (conditional mediation) vs. optional- What conditional mediation is and when to use it
- Browser compatibility considerations
- The relationship between
metadata.mediationand thesignalparameterConsider adding JSDoc comments in the source TypeScript files to explain these aspects.
Also applies to: 98-98, 107-107, 120-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journey-client/api-report/journey-client.webauthn.api.md` at line 95, Add JSDoc comments to the public WebAuthn APIs — specifically the static method authenticate(step: JourneyStep, signal?: AbortSignal) and the related methods at the other marked locations — to explain when the AbortSignal is required versus optional (i.e., conditional mediation), define what “conditional mediation” means and when callers should use it, call out browser compatibility caveats, and describe how the optional signal parameter interacts with metadata.mediation on JourneyStep; update the TypeScript source (where authenticate and the other undocumented APIs are declared) to include these JSDoc blocks so the generated api-report no longer shows “(undocumented)”.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/journey-client/api-report/journey-client.webauthn.api.md`:
- Line 95: Add JSDoc comments to the public WebAuthn APIs — specifically the
static method authenticate(step: JourneyStep, signal?: AbortSignal) and the
related methods at the other marked locations — to explain when the AbortSignal
is required versus optional (i.e., conditional mediation), define what
“conditional mediation” means and when callers should use it, call out browser
compatibility caveats, and describe how the optional signal parameter interacts
with metadata.mediation on JourneyStep; update the TypeScript source (where
authenticate and the other undocumented APIs are declared) to include these
JSDoc blocks so the generated api-report no longer shows “(undocumented)”.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c382bd37-9d7b-4d0c-a238-d24ea34559a3
📒 Files selected for processing (6)
.changeset/ready-snakes-sell.mde2e/journey-app/components/webauthn-step.tspackages/journey-client/api-report/journey-client.webauthn.api.mdpackages/journey-client/src/lib/webauthn/interfaces.tspackages/journey-client/src/lib/webauthn/webauthn.test.tspackages/journey-client/src/lib/webauthn/webauthn.ts
✅ Files skipped from review due to trivial changes (1)
- packages/journey-client/src/lib/webauthn/interfaces.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- .changeset/ready-snakes-sell.md
- packages/journey-client/src/lib/webauthn/webauthn.test.ts
- e2e/journey-app/components/webauthn-step.ts
- packages/journey-client/src/lib/webauthn/webauthn.ts
vatsalparikh
force-pushed
the
sdks-4612
branch
2 times, most recently
from
14892f6 to
1028da7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
e2e/journey-app/components/webauthn-step.ts (2)
81-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIgnore
AbortErrorin conditional mediation catch path.At Line 81, cancellation is expected behavior in conditional flows; surfacing it as an error message will create false-negative UX noise.
Suggested fix
- void WebAuthn.authenticate(step, controller.signal) - .then(() => submitForm()) - .catch(() => { - setError('WebAuthn failed or was cancelled. Please try again or use a different method.'); - }); + void WebAuthn.authenticate(step, controller.signal) + .then(() => submitForm()) + .catch((err: unknown) => { + if (err instanceof Error && err.name === 'AbortError') return; + setError('WebAuthn failed or was cancelled. Please try again or use a different method.'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-app/components/webauthn-step.ts` around lines 81 - 83, The catch handler currently always sets an error message on any rejection; change the catch to accept the error (e.g., catch((err) => ...)) and if err.name === 'AbortError' simply return/ignore (no call to setError) to avoid surfacing expected cancellation in the conditional mediation flow, otherwise call setError('WebAuthn failed or was cancelled. Please try again or use a different method.') as before; update the catch associated with the conditional mediation / navigator.credentials flow so it references the caught error and checks its name before calling setError.
77-85:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftConditional flow needs lifecycle cancellation to prevent stale submissions.
At Line 78, the
AbortControlleris local and never aborted when the step changes or another submit path wins, so the in-flight conditional request can later callsubmitForm()/setError()on stale UI state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/journey-suites/src/webauthn-device.test.ts`:
- Around line 139-176: The PR is blocked by missing unit tests for the changed
branches in packages/journey-client/src/lib/webauthn/webauthn.ts — add focused
unit tests that exercise the module's conditional-mediation path, the
AbortSignal/abort handling, and the error paths: mock
navigator.credentials.get/create to return a conditional mediation response,
simulate an AbortController abort to hit the abort branch, and force navigator
failures to hit error propagation; write tests that import the exported
functions from webauthn.ts and assert expected results/throws and any cleanup
behavior so the conditional/abort/error branches are covered.
---
Duplicate comments:
In `@e2e/journey-app/components/webauthn-step.ts`:
- Around line 81-83: The catch handler currently always sets an error message on
any rejection; change the catch to accept the error (e.g., catch((err) => ...))
and if err.name === 'AbortError' simply return/ignore (no call to setError) to
avoid surfacing expected cancellation in the conditional mediation flow,
otherwise call setError('WebAuthn failed or was cancelled. Please try again or
use a different method.') as before; update the catch associated with the
conditional mediation / navigator.credentials flow so it references the caught
error and checks its name before calling setError.
🪄 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: e43e7be9-8da2-4b20-b9e0-14ae70d36356
📒 Files selected for processing (7)
.changeset/ready-snakes-sell.mde2e/journey-app/components/webauthn-step.tse2e/journey-suites/src/webauthn-device.test.tspackages/journey-client/api-report/journey-client.webauthn.api.mdpackages/journey-client/src/lib/webauthn/interfaces.tspackages/journey-client/src/lib/webauthn/webauthn.test.tspackages/journey-client/src/lib/webauthn/webauthn.ts
✅ Files skipped from review due to trivial changes (1)
- packages/journey-client/src/lib/webauthn/webauthn.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/journey-client/src/lib/webauthn/interfaces.ts
- .changeset/ready-snakes-sell.md
- packages/journey-client/src/lib/webauthn/webauthn.ts
| test('registers a passkey then authenticates via conditional autofill', async ({ page }) => { | ||
| const { clickButton, navigate } = asyncEvents(page); | ||
|
|
||
| await test.step('Register a WebAuthn credential', async () => { | ||
| // Start with an empty virtual authenticator. | ||
| const { credentials: initialCredentials } = await cdp.send('WebAuthn.getCredentials', { | ||
| authenticatorId, | ||
| }); | ||
| expect(initialCredentials).toHaveLength(0); | ||
|
|
||
| // Run a registration journey that creates a credential in the authenticator. | ||
| await navigate('/?clientId=tenant&journey=TEST_WebAuthn-Registration'); | ||
| await expect(page.getByLabel('User Name')).toBeVisible(); | ||
| await page.getByLabel('User Name').fill(username); | ||
| await page.getByLabel('Password').fill(password); | ||
| await clickButton('Submit', '/authenticate'); | ||
| await expect(page.getByRole('button', { name: 'Logout' })).toBeVisible(); | ||
|
|
||
| const { credentials } = await cdp.send('WebAuthn.getCredentials', { authenticatorId }); | ||
| expect(credentials.length).toBeGreaterThan(0); | ||
| }); | ||
|
|
||
| await test.step('Authenticate using conditional UI / passkey autofill', async () => { | ||
| // Ensure we are not reusing an existing AM session. | ||
| // This makes the test exercise passkey auth, not cookie auth. | ||
| await page.context().clearCookies(); | ||
|
|
||
| // This journey emits conditional mediation metadata and should complete via background | ||
| // WebAuthn (journey-app triggers the request and submits when a credential is returned). | ||
| await navigate('/?clientId=tenant&journey=TEST_AutofillPasskeyWebAuthn'); | ||
|
|
||
| // With a virtual authenticator configured for automatic presence simulation, this should | ||
| // complete without any manual click. | ||
| await expect(page.getByRole('button', { name: 'Logout' })).toBeVisible(); | ||
| await expect(page.getByRole('heading', { name: 'Complete' })).toBeVisible(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Coverage gate is still failing for changed webauthn.ts logic.
This E2E test helps, but the PR remains blocked by low patch coverage on packages/journey-client/src/lib/webauthn/webauthn.ts (notably conditional/abort/error branches). Please add targeted unit tests for those branches to satisfy the required threshold.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/journey-suites/src/webauthn-device.test.ts` around lines 139 - 176, The
PR is blocked by missing unit tests for the changed branches in
packages/journey-client/src/lib/webauthn/webauthn.ts — add focused unit tests
that exercise the module's conditional-mediation path, the AbortSignal/abort
handling, and the error paths: mock navigator.credentials.get/create to return a
conditional mediation response, simulate an AbortController abort to hit the
abort branch, and force navigator failures to hit error propagation; write tests
that import the exported functions from webauthn.ts and assert expected
results/throws and any cleanup behavior so the conditional/abort/error branches
are covered.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/journey-client/src/lib/webauthn/webauthn.ts (1)
195-209:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't persist
AbortErrorintowebAuthnOutcomefor caller-cancelled requests.
authenticate()now accepts a caller-suppliedAbortSignalfor every auth request, but this catch only treatsAbortErroras benign whenmediation === 'conditional'. If a consumer aborts a promptedauthenticate(step, signal), Line 208 writesERROR::AbortError...into the hidden callback even though the cancellation was intentional, which can leak stale error state into a later submit.Suggested fix
- if (mediation === 'conditional' && error.name === 'AbortError') { + if ( + error.name === 'AbortError' && + (mediation === 'conditional' || signal?.aborted) + ) { throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/journey-client/src/lib/webauthn/webauthn.ts` around lines 195 - 209, The catch block in authenticate() currently only skips persisting AbortError when mediation === 'conditional', causing caller-triggered AbortErrors to be written into hiddenCallback (webAuthnOutcome); change the logic so any AbortError is treated as a benign cancellation: if error.name === 'AbortError' then rethrow immediately without calling hiddenCallback.setInputValue, regardless of mediation. Keep the existing NotSupportedError handling intact and continue to set hiddenCallback for other errors.
♻️ Duplicate comments (1)
e2e/journey-app/components/webauthn-step.ts (1)
77-85:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftExpose and abort the conditional-flow controller on step changes / alternate submits.
This branch returns immediately while
WebAuthn.authenticate(step, controller.signal)keeps running, but the controller never leaves this scope. That means a later step render or different submit path cannot cancel the in-flight conditional request, so the stale promise can still callsubmitForm()or hit this catch against newer UI. Sincepackages/journey-client/src/lib/webauthn/webauthn.tsalready treats conditionalAbortErroras expected cancellation, the rejection handler here should also stop surfacingAbortErroronce the controller is wired up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/journey-app/components/webauthn-step.ts` around lines 77 - 85, The in-flight WebAuthn conditional controller is scoped locally and cannot be aborted by later renders or alternate submits, allowing a stale promise to call submitForm() or setError(); fix by exposing the AbortController (e.g., attach it to the step object or component-level ref) so other code paths and the step cleanup can abort it, and update the rejection handler of WebAuthn.authenticate(step, controller.signal) to ignore AbortError (check error.name === 'AbortError' or similar) so aborted attempts do not trigger submitForm() or setError; ensure any previous controller is aborted before creating a new one and clear the attached controller after resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/journey-client/src/lib/webauthn/webauthn.ts`:
- Around line 195-209: The catch block in authenticate() currently only skips
persisting AbortError when mediation === 'conditional', causing caller-triggered
AbortErrors to be written into hiddenCallback (webAuthnOutcome); change the
logic so any AbortError is treated as a benign cancellation: if error.name ===
'AbortError' then rethrow immediately without calling
hiddenCallback.setInputValue, regardless of mediation. Keep the existing
NotSupportedError handling intact and continue to set hiddenCallback for other
errors.
---
Duplicate comments:
In `@e2e/journey-app/components/webauthn-step.ts`:
- Around line 77-85: The in-flight WebAuthn conditional controller is scoped
locally and cannot be aborted by later renders or alternate submits, allowing a
stale promise to call submitForm() or setError(); fix by exposing the
AbortController (e.g., attach it to the step object or component-level ref) so
other code paths and the step cleanup can abort it, and update the rejection
handler of WebAuthn.authenticate(step, controller.signal) to ignore AbortError
(check error.name === 'AbortError' or similar) so aborted attempts do not
trigger submitForm() or setError; ensure any previous controller is aborted
before creating a new one and clear the attached controller after resolution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a6794a1-e797-4c94-9419-3dc1d428e06d
📒 Files selected for processing (7)
.changeset/ready-snakes-sell.mde2e/journey-app/components/webauthn-step.tse2e/journey-suites/src/webauthn-device.test.tspackages/journey-client/api-report/journey-client.webauthn.api.mdpackages/journey-client/src/lib/webauthn/interfaces.tspackages/journey-client/src/lib/webauthn/webauthn.test.tspackages/journey-client/src/lib/webauthn/webauthn.ts
✅ Files skipped from review due to trivial changes (1)
- packages/journey-client/src/lib/webauthn/webauthn.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .changeset/ready-snakes-sell.md
…roller [Self-Healing CI Rerun]
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…roller [Self-Healing CI Rerun]
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Nx Cloud has identified a flaky task in your failed CI:
🔂 Since the failure was identified as flaky, we triggered a CI rerun by adding an empty commit to this branch.
🎓 Learn more about Self-Healing CI on nx.dev
4 participants
JIRA Ticket
pingidentity.atlassian.net/browse/SDKS-4612
Description
Add WebAuthn conditional mediation (passkey autofill) support to @forgerock/journey-client.
This enables consumers to opt-in to conditional UI by passing mediation: 'conditional' and an AbortSignal through to navigator.credentials.get(...) with a helper to feature-detect browser support.
What changed
How to test
e2e/journey-appfolder and with commandspnpm buildfollowed bypnpm serveping-javascript-sdk/packages/journey-client/src/lib/webauthn/webauthn.ts
Line 151 in a935fc4
Recording
Screen.Recording.2026-04-23.at.11.53.07.AM.mov
Did you add a changeset?
Yes
Summary by CodeRabbit
New Features
Bug Fixes / UX
Tests