test(davinci-client): virtual authenticator e2e tests #504
Conversation
🦋 Changeset detectedLatest commit: 7834cde The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 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 |
WalkthroughAdds FIDO/WebAuthn support: a UI component for registration/authentication, wiring into the main app, E2E tests using a Chrome CDP fake authenticator (with tests ignored by default), a changeset for a patch bump, and early-exit validation in FIDO client methods when no options are provided. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Form
participant FidoComponent
participant FidoAPI
participant Updater
participant Server
Note over FidoComponent: FIDO Registration Flow
User->>Form: Click "FIDO Register"
Form->>FidoComponent: Button event
FidoComponent->>FidoAPI: register(publicKeyCredentialCreationOptions)
FidoAPI-->>FidoComponent: credential / error
alt Registration Success
FidoComponent->>Updater: updater(credential, collector)
Updater->>Server: send credential
alt Updater Success
Server-->>Updater: accepted
Updater-->>FidoComponent: success
FidoComponent->>Form: submitForm()
else Updater Error
Updater-->>FidoComponent: error (logged)
end
else Registration Error
FidoComponent->>FidoComponent: log error (halt)
end
sequenceDiagram
participant User
participant Form
participant FidoComponent
participant FidoAPI
participant Updater
participant Server
Note over FidoComponent: FIDO Authentication Flow
User->>Form: Click "FIDO Authenticate"
Form->>FidoComponent: Button event
FidoComponent->>FidoAPI: authenticate(publicKeyCredentialRequestOptions)
FidoAPI-->>FidoComponent: assertion / error
alt Authentication Success
FidoComponent->>Updater: updater(assertion, collector)
Updater->>Server: send assertion
alt Updater Success
Server-->>Updater: verified
Updater-->>FidoComponent: success
FidoComponent->>Form: submitForm()
else Updater Error
Updater-->>FidoComponent: error (logged)
end
else Authentication Error
FidoComponent->>FidoComponent: log error (halt)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit faa2963
☁️ Nx Cloud last updated this comment at |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nx Cloud has identified a possible root cause for your failed CI:
Our new FIDO WebAuthn E2E test is failing due to a DNS resolution error (net::ERR_NAME_NOT_RESOLVED) for the external test domain "aj-test.pi.scrd.run:5829". The test code is properly structured, but the external test environment is currently unreachable, which requires infrastructure recovery before the test can execute successfully.
No code changes were suggested for this issue.
If the issue was transient, you can trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
e2e/davinci-suites/src/fido.test.ts
Outdated
| test('Register and authenticate with webauthn device', async ({ page }) => { | ||
| const { navigate } = asyncEvents(page); | ||
|
|
||
| await navigate('https://aj-test.pi.scrd.run:5829/?acr_values=ccff5c09002042bd86104da45cd7102e'); |
There was a problem hiding this comment.
Are we going to need a fully secured domain even for virtual authenticators?
There was a problem hiding this comment.
You do not on Chrome. You do on Firefox though I believe, but these will only work on Chromium.
e2e/davinci-suites/src/fido.test.ts
Outdated
| await page.getByRole('button', { name: 'DEVICE_REGISTRATION' }).click(); | ||
| await page.getByRole('button', { name: 'Biometrics/Security Key' }).click(); | ||
| await page.getByRole('button', { name: 'FIDO Register' }).click(); |
There was a problem hiding this comment.
Is this just a highly complex tree you're currently using for testing? I'm asking because of all the button clicks.
There was a problem hiding this comment.
Yeah it's very similar to the flow for phone number registration test. The last button isn't really necessary, it can be an automatic registration/authentication, but I added it anyways to trigger when you want to start FIDO.
There was a problem hiding this comment.
Got it. The extra button click is okay. I just get nervous when big, complex flows/journeys are used in testing :)
There was a problem hiding this comment.
Do we know if we are going to have the same issue with DaVinci as we did with AM? The too many credentials issue, that is.
There was a problem hiding this comment.
Responded to this via slack, but for posterity
Technically yes to Justin's question, but our solution we discussed was to not include these tests as part of our ci directly and to use the playwright grep configuration so we can run virtual Authenticator tests manually
Like playwright test —grep “*.webauthn” and playwright only picks up those file types and we can alias it into a command
This circumvents the idea of ci failing consistently
We of course eventually could hit the error because it's a browser restriction and we would have to change the user or delete the devices but the tests themselves can hold value and not run all the time until we have a better way to delete devices automatically
e2e/davinci-suites/src/fido.test.ts
Outdated
| await expect(credentialsAfterAuth.credentials).toHaveLength(1); | ||
|
|
||
| // Signature counter should have incremented after successful authentication/assertion | ||
| await expect(credentialsAfterAuth.credentials[0].signCount).toBeGreaterThan(initialSignCount); |
There was a problem hiding this comment.
So this is verifying that from an SDK perspective, the authentication actually worked, and the credential was signed?
I'm not totally against it, but it feels a bit low-level.
There was a problem hiding this comment.
Yes, I wasn't sure what you were going for with WebAuthn.setUserVerified. From my understanding this just sets UV to always succeed so I wanted some other way to test that authentication actually happened with the SDK.
There was a problem hiding this comment.
So if User Verification is required by the node, we would need UV to work on the authenticator. if it's not required, the setting probably is more no-op because there is no user verification requirement. So depends on the UV requirement
There was a problem hiding this comment.
I like this approach. The dangers about using mock things is you can accidentally assert something that will always be true because it is mocked. One of the most important parts of testing is that what you assert is actually fail'able.
ancheetah
force-pushed
the
SDKS-4522-virtual-auth-e2e
branch
from
faa2963 to
6fed17f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/full-bikes-boil.md(1 hunks)e2e/davinci-app/components/fido.ts(1 hunks)e2e/davinci-app/main.ts(5 hunks)e2e/davinci-suites/playwright.config.ts(1 hunks)e2e/davinci-suites/src/fido.test.ts(1 hunks)packages/davinci-client/src/lib/fido/fido.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: ancheetah
Repo: ForgeRock/ping-javascript-sdk PR: 468
File: packages/davinci-client/src/lib/fido/fido.utils.ts:17-24
Timestamp: 2025-11-07T21:35:14.799Z
Learning: DaVinci FIDO API accepts standard base64 encoding (RFC 4648 §4 with `+`, `/`, `=`) for WebAuthn response fields (rawId, clientDataJSON, authenticatorData, signature, userHandle) in packages/davinci-client, not base64url. This is confirmed by sample payloads from the DaVinci team showing standard base64 format.
🧬 Code graph analysis (3)
e2e/davinci-app/components/fido.ts (1)
packages/davinci-client/src/lib/node.slice.ts (1)
error(72-110)
packages/davinci-client/src/lib/fido/fido.ts (1)
packages/sdk-types/src/lib/error.types.ts (1)
GenericError(7-24)
e2e/davinci-app/main.ts (2)
packages/protect/src/lib/protect.ts (1)
protect(30-96)packages/davinci-client/src/index.ts (1)
fido(10-10)
🪛 ast-grep (0.40.0)
e2e/davinci-app/components/fido.ts
[warning] 24-24: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: button.innerHTML = 'FIDO Register'
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 46-46: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: button.innerHTML = 'FIDO Authenticate'
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 24-24: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: button.innerHTML = 'FIDO Register'
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 46-46: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: button.innerHTML = 'FIDO Authenticate'
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🔇 Additional comments (13)
.changeset/full-bikes-boil.md (1)
1-5: LGTM!The changeset correctly documents the patch version bump for improved FIDO module error handling.
packages/davinci-client/src/lib/fido/fido.ts (2)
58-64: LGTM! Good defensive validation.The early-exit validation properly handles the case when registration options are missing, preventing downstream errors in the transformation pipeline.
119-125: LGTM! Consistent error handling.The authentication validation mirrors the registration pattern and provides clear error messaging when options are unavailable.
e2e/davinci-suites/playwright.config.ts (1)
44-44: LGTM! Aligns with team's testing strategy.Excluding FIDO tests from standard runs prevents CI failures due to browser credential limits, as discussed in past comments. These tests can still be executed manually when needed.
e2e/davinci-suites/src/fido.test.ts (3)
11-27: LGTM! Proper virtual authenticator setup.The beforeEach hook correctly configures a platform authenticator with UV+RK for passkey testing, and the CDP session setup is appropriate for Chromium-based testing.
72-84: Excellent verification approach.Using signCount to verify that authentication actually occurred is a robust testing strategy that ensures the WebAuthn flow is working correctly, not just mocked.
90-143: LGTM! Good coverage of usernameless flow.The second test properly exercises the usernameless authentication scenario, providing comprehensive coverage of FIDO authentication patterns.
e2e/davinci-app/main.ts (3)
10-10: LGTM! Clean FIDO integration.The imports and initialization of the FIDO API are properly structured and follow the existing patterns in the application.
Also applies to: 34-34, 86-86
85-85: LGTM! Consistent naming improvement.Renaming to
protectApiimproves consistency across the codebase.Also applies to: 92-92, 294-294
256-266: LGTM! Proper FIDO collector handling.The rendering logic for FIDO collectors correctly passes the fidoApi instance and submitForm callback, mirroring the patterns used for other collector types.
e2e/davinci-app/components/fido.ts (3)
21-42: LGTM! Clear registration flow.The registration flow properly orchestrates the WebAuthn API call, handles both success and error cases, and integrates with the updater/submitForm pattern.
43-64: LGTM! Consistent authentication flow.The authentication flow mirrors the registration pattern, maintaining consistency across the component.
24-24: Static analysis false positive - safe innerHTML usage.The static analysis warnings about innerHTML usage on lines 24 and 46 are false positives. Both assignments use hardcoded string literals ('FIDO Register' and 'FIDO Authenticate') with no dynamic content, posing no XSS risk. Using
textContentwould be semantically equivalent here but not necessary for security.Also applies to: 46-46
ancheetah
force-pushed
the
SDKS-4522-virtual-auth-e2e
branch
from
6fed17f to
f674545
Compare
e2e/davinci-app/main.ts
Outdated
| const davinciClient: DavinciClient = await davinci({ config, logger, requestMiddleware }); | ||
| const protectAPI = protect({ envId: '02fb4743-189a-4bc7-9d6c-a919edfe6447' }); | ||
| const protectApi = protect({ envId: '02fb4743-189a-4bc7-9d6c-a919edfe6447' }); | ||
| const fidoApi = fido(); |
There was a problem hiding this comment.
Does anything other than the fido.js component use this API? If no, could we just move this into the component, rather than initializing it here to just pass it into the component?
There was a problem hiding this comment.
good point, moved it there
|
|
||
| // Initialize Protect | ||
| const error = await protectAPI.start(); | ||
| const error = await protectApi.start(); |
There was a problem hiding this comment.
Thank you for finding these ❤️
| if (!options) { | ||
| return { | ||
| error: 'registration_error', | ||
| message: 'FIDO registration failed: No options available', | ||
| type: 'fido_error', | ||
| } as GenericError; | ||
| } | ||
|
|
There was a problem hiding this comment.
Nice! It's good we caught this during our testing phase.
ancheetah
force-pushed
the
SDKS-4522-virtual-auth-e2e
branch
from
f674545 to
7834cde
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/davinci-app/components/fido.ts (1)
24-24: Static analysis false positives for hardcoded strings.The static analysis warnings about
innerHTMLusage are false positives here since both assignments use hardcoded string literals ('FIDO Register'and'FIDO Authenticate'), not user input. However, as an optional refinement, you could usetextContentinstead ofinnerHTMLfor simple text assignments to follow best practices.If you'd like to make this change:
- button.innerHTML = 'FIDO Register'; + button.textContent = 'FIDO Register';And:
- button.innerHTML = 'FIDO Authenticate'; + button.textContent = 'FIDO Authenticate';Also applies to: 46-46
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/full-bikes-boil.md(1 hunks)e2e/davinci-app/components/fido.ts(1 hunks)e2e/davinci-app/main.ts(4 hunks)e2e/davinci-suites/src/fido.test.ts(1 hunks)packages/davinci-client/src/lib/fido/fido.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/davinci-client/src/lib/fido/fido.ts
🧰 Additional context used
🧬 Code graph analysis (2)
e2e/davinci-app/main.ts (2)
packages/protect/src/lib/protect.ts (1)
protect(30-96)e2e/davinci-app/components/fido.ts (1)
fidoComponent(14-66)
e2e/davinci-app/components/fido.ts (2)
packages/davinci-client/src/lib/fido/fido.ts (1)
fido(50-173)packages/davinci-client/src/lib/node.slice.ts (1)
error(72-110)
🪛 ast-grep (0.40.0)
e2e/davinci-app/components/fido.ts
[warning] 24-24: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: button.innerHTML = 'FIDO Register'
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 46-46: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: button.innerHTML = 'FIDO Authenticate'
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 24-24: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: button.innerHTML = 'FIDO Register'
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 46-46: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: button.innerHTML = 'FIDO Authenticate'
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pr
- GitHub Check: Mend Code Security Check
🔇 Additional comments (9)
.changeset/full-bikes-boil.md (1)
1-5: LGTM!The changeset correctly documents the FIDO error handling improvement with an appropriate patch-level version bump.
e2e/davinci-app/main.ts (3)
34-34: LGTM!The import of
fidoComponentand the consistent renaming fromprotectAPItoprotectApiimprove code consistency and readability.Also applies to: 85-85, 91-91
255-264: LGTM!The FIDO collector handling follows the established pattern used for other collectors in the application, maintaining consistency in the codebase.
292-292: LGTM!Consistent with the
protectAPItoprotectApirename throughout the file.e2e/davinci-suites/src/fido.test.ts (3)
1-32: LGTM!The test setup properly configures a Chrome CDP virtual authenticator with appropriate settings for passkey testing (platform authenticator with UV and RK support). The beforeEach/afterEach hooks ensure proper test isolation.
35-88: LGTM!The test comprehensively validates the FIDO registration and authentication flow. The verification of signature counter increments after authentication is a particularly good assertion that confirms actual cryptographic operations occurred rather than just mocked behavior.
90-145: LGTM!Appropriately skipped with a clear explanation of the upstream DaVinci issue. The test structure is preserved for future enablement once the authentication options issue is resolved.
e2e/davinci-app/components/fido.ts (2)
20-42: LGTM!The registration flow is well-structured with proper error handling. The component correctly retrieves credential options, calls the FIDO API, handles both API and updater errors, and only proceeds with form submission on success.
43-65: LGTM!The authentication flow mirrors the registration flow with consistent error handling and form submission logic. The implementation maintains good symmetry between the two FIDO operations.
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4522
Description
Adds virtual authenticator tests for DaVinci client FIDO collectors and FIDO module.
Included changeset for improved error handling
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.