SDKS-4377: Implement FIDO module in DaVinci client #468
Conversation
🦋 Changeset detectedLatest commit: fd99414 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 a new FIDO/WebAuthn feature module to Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Davinci as DaVinci Client
participant Fido as FIDO Module
participant WA as WebAuthn API
rect rgba(220,240,255,0.8)
note over App,Davinci: Registration Flow
App->>Davinci: trigger registration
Davinci->>Fido: fido.register(davinciOptions)
Fido->>Fido: transformRegistrationOptions()
Fido->>WA: navigator.credentials.create(...)
WA->>Fido: PublicKeyCredential
Fido->>Fido: transformPublicKeyCredential()
Fido-->>Davinci: FidoRegistrationInputValue
Davinci-->>App: registration result
end
rect rgba(220,255,220,0.8)
note over App,Davinci: Authentication Flow
App->>Davinci: trigger authentication
Davinci->>Fido: fido.authenticate(davinciOptions)
Fido->>Fido: transformAuthenticationOptions()
Fido->>WA: navigator.credentials.get(...)
WA->>Fido: PublicKeyCredential (assertion)
Fido->>Fido: transformAssertion()
Fido-->>Davinci: FidoAuthenticationInputValue
Davinci-->>App: authentication result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 5d5c2b6
☁️ Nx Cloud last updated this comment at |
ancheetah
force-pushed
the
SDKS-4377-dv-mfa
branch
from
38a4b64 to
939fe08
Compare
![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.
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
This looks good. I think the patterns here are great, and the code reads well. What is left for this to be converted from a draft to mergeable?
@cerebrl a few things, the main one being input validation if we even want to do that. I'll message you with more details in Slack. TODO:
|
ancheetah
force-pushed
the
SDKS-4377-dv-mfa
branch
from
939fe08 to
5d5c2b6
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
packages/davinci-client/src/lib/client.store.ts (1)
48-48: LGTM! Good optimization for bundle size.Converting to a type-only import is correct since
ContinueNodeandStartNodeare only used for type annotations and assertions (lines 72, 181, 199) without any runtime usage. This follows TypeScript best practices and ensures these symbols are stripped during build.packages/davinci-client/src/lib/fido/README.md (2)
28-28: Consider using proper Markdown heading syntax.Line 28 uses bold emphasis instead of a proper heading. For better document structure and accessibility, consider using heading syntax.
Apply this change:
-**register(options: FidoRegistrationOptions) => Promise<FidoRegistrationInputValue | GenericError>** +#### register(options: FidoRegistrationOptions) => Promise<FidoRegistrationInputValue | GenericError>
34-34: Consider using proper Markdown heading syntax.Line 34 uses bold emphasis instead of a proper heading. For better document structure and accessibility, consider using heading syntax.
Apply this change:
-**authenticate: (options: FidoAuthenticationOptions) => Promise<FidoAuthenticationInputValue | GenericError>** +#### authenticate(options: FidoAuthenticationOptions) => Promise<FidoAuthenticationInputValue | GenericError>packages/davinci-client/src/lib/fido/fido.utils.ts (1)
13-15: UseUint8Arrayto preserve WebAuthn byte semantics.Switching from
Int8ArraytoUint8Arrayavoids negative intermediate values and matches the unsigned expectations of WebAuthn challenges and IDs. It keeps the underlying bytes identical but removes confusion and future bugs when debugging or extending the helper.Apply this diff:
-function convertArrayToBuffer(arr: number[]): ArrayBuffer { - return new Int8Array(arr).buffer; -} +function convertArrayToBuffer(arr: number[]): ArrayBuffer { + return new Uint8Array(arr).buffer; +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
.changeset/sparkly-dragons-lay.md(1 hunks)package.json(1 hunks)packages/davinci-client/package.json(1 hunks)packages/davinci-client/src/index.ts(1 hunks)packages/davinci-client/src/lib/client.store.ts(1 hunks)packages/davinci-client/src/lib/client.types.test-d.ts(2 hunks)packages/davinci-client/src/lib/client.types.ts(2 hunks)packages/davinci-client/src/lib/collector.types.ts(4 hunks)packages/davinci-client/src/lib/davinci.types.ts(1 hunks)packages/davinci-client/src/lib/davinci.utils.ts(1 hunks)packages/davinci-client/src/lib/fido/README.md(1 hunks)packages/davinci-client/src/lib/fido/fido.ts(1 hunks)packages/davinci-client/src/lib/fido/fido.utils.test.ts(1 hunks)packages/davinci-client/src/lib/fido/fido.utils.ts(1 hunks)packages/davinci-client/src/types.ts(2 hunks)packages/sdk-types/src/lib/error.types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
packages/davinci-client/src/lib/fido/fido.utils.test.ts (3)
packages/davinci-client/src/lib/davinci.types.ts (2)
FidoRegistrationOptions(163-183)FidoAuthenticationOptions(195-203)packages/davinci-client/src/lib/fido/fido.utils.ts (4)
transformRegistrationOptions(32-55)transformPublicKeyCredential(63-82)transformAuthenticationOptions(90-105)transformAssertion(113-136)packages/davinci-client/src/lib/collector.types.ts (2)
FidoRegistrationInputValue(555-557)FidoAuthenticationInputValue(576-578)
packages/davinci-client/src/lib/fido/fido.ts (4)
packages/davinci-client/src/lib/davinci.types.ts (2)
FidoRegistrationOptions(163-183)FidoAuthenticationOptions(195-203)packages/davinci-client/src/lib/collector.types.ts (2)
FidoRegistrationInputValue(555-557)FidoAuthenticationInputValue(576-578)packages/sdk-types/src/lib/error.types.ts (1)
GenericError(7-24)packages/davinci-client/src/lib/fido/fido.utils.ts (4)
transformRegistrationOptions(32-55)transformPublicKeyCredential(63-82)transformAuthenticationOptions(90-105)transformAssertion(113-136)
packages/davinci-client/src/lib/client.types.ts (1)
packages/davinci-client/src/lib/collector.types.ts (3)
PhoneNumberInputValue(297-300)FidoRegistrationInputValue(555-557)FidoAuthenticationInputValue(576-578)
packages/davinci-client/src/lib/fido/fido.utils.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (2)
FidoRegistrationOptions(163-183)FidoAuthenticationOptions(195-203)packages/davinci-client/src/lib/collector.types.ts (2)
FidoRegistrationInputValue(555-557)FidoAuthenticationInputValue(576-578)
packages/davinci-client/src/lib/client.types.test-d.ts (1)
packages/davinci-client/src/lib/collector.types.ts (3)
PhoneNumberInputValue(297-300)FidoRegistrationInputValue(555-557)FidoAuthenticationInputValue(576-578)
packages/davinci-client/src/lib/collector.types.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (2)
FidoRegistrationOptions(163-183)FidoAuthenticationOptions(195-203)packages/davinci-client/src/types.ts (3)
ProtectCollector(52-52)FidoRegistrationCollector(53-53)FidoAuthenticationCollector(54-54)
🪛 Gitleaks (8.28.0)
packages/davinci-client/src/lib/fido/fido.utils.test.ts
[high] 178-178: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
packages/davinci-client/src/lib/fido/README.md
[grammar] ~5-~5: Use a hyphen to join words.
Context: ...sform options from DaVinci into WebAuthn compatible options, then call `navigator...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
packages/davinci-client/src/lib/fido/README.md
28-28: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
34-34: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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). (1)
- GitHub Check: pr
🔇 Additional comments (12)
.changeset/sparkly-dragons-lay.md (1)
1-6: Changeset summary matches the scope of the FIDO work.Both affected packages receive the expected minor bump and the note captures the new module addition cleanly.
packages/sdk-types/src/lib/error.types.ts (1)
13-17: Union update keeps error typing aligned.Including
'fido_error'ensures downstream handling can discriminate FIDO-specific failures without resorting to catch-alls.packages/davinci-client/package.json (1)
32-32: Dependency addition is appropriate.Bringing in
effectvia the catalog aligns with the new module’s functional abstractions, so the runtime dependency list stays consistent with actual usage.package.json (1)
21-21: Clean script tweak is sensible.Explicitly clearing
./e2e/node_modulesremoves a common leftover directory and keeps the workspace reset thorough.packages/davinci-client/src/lib/collector.types.ts (1)
541-635: Typed collector refinements look solid.The new IV/OV generics keep existing collectors working while giving FIDO and Protect flows precise input/output contracts, so consumers get safer access to WebAuthn payloads without breaking changes.
packages/davinci-client/src/lib/davinci.utils.ts (1)
47-48: LGTM! Collector categories expanded appropriately.The addition of
SingleValueAutoCollectorandObjectValueAutoCollectorto the form submission filter is consistent with the existing pattern and correctly broadens the set of collectors that contribute to form data.packages/davinci-client/src/index.ts (1)
8-10: LGTM! FIDO module exported correctly.The import and export of the
fidomodule is clean and follows the existing pattern, successfully expanding the public API surface.packages/davinci-client/src/types.ts (1)
9-9: LGTM! FidoClient type imported and exported correctly.The type import and export are properly structured, exposing the FidoClient type as part of the public API surface.
packages/davinci-client/src/lib/client.types.ts (2)
9-13: LGTM! FIDO input value types imported correctly.The new imports for FIDO-related input value types are properly structured and align with the broader FIDO integration.
25-31: LGTM! Updater signature expanded appropriately.The
Updatersignature has been correctly expanded to accept the new FIDO input value types, maintaining type safety while supporting the new FIDO functionality.packages/davinci-client/src/lib/fido/fido.utils.test.ts (1)
1-210: LGTM! Comprehensive FIDO utility tests.The test coverage is thorough, validating all transformation functions with proper type assertions and expected value checks. The tests correctly verify ArrayBuffer conversions and base64 encoding/decoding for FIDO credentials.
Note: The Gitleaks warning on line 178 is a false positive—
authenticatorDataBase64is a test fixture string, not an actual API key.packages/davinci-client/src/lib/davinci.types.ts (1)
163-183: Review comment appears to be flagging a valid gap, but verification is inconclusive.The
transformRegistrationOptionsfunction currently convertschallenge,user.id, andexcludeCredentials[].idarrays toArrayBuffer, but does not handle the inheritedextensionsproperty. The function spreads options as-is, leaving extensions unchanged. Test data shows extensions as{credProps: true, hmacCreateSecret: true}(boolean values only), with no array properties requiring conversion evident in the codebase.However, standard WebAuthn extensions are typically boolean or string values, not arrays. Whether DaVinci-specific extensions contain array properties that must be converted to
BufferSourcecannot be definitively determined from the current code. Please manually verify whether your extensions can contain array-typed properties requiring buffer conversion, and implement conversion logic if needed.
| function convertBufferToBase64(buffer: ArrayBuffer): string { | ||
| const byteArray = new Uint8Array(buffer); | ||
| let binaryString = ''; | ||
| for (let i = 0; i < byteArray.byteLength; i++) { | ||
| binaryString += String.fromCharCode(byteArray[i]); | ||
| } | ||
| return btoa(binaryString); | ||
| } |
There was a problem hiding this comment.
Encode response buffers as base64url, not standard base64.
The current helper emits RFC 4648 base64 (+, /, =). WebAuthn JSON exchanges (and DaVinci’s APIs) expect base64url; leaving the characters unconverted causes assertion/attestation uploads to be rejected whenever the binary data contains those bytes. This breaks registration/auth flows for a significant subset of credentials.
Apply this diff:
let binaryString = '';
for (let i = 0; i < byteArray.byteLength; i++) {
binaryString += String.fromCharCode(byteArray[i]);
}
- return btoa(binaryString);
+ const base64 = btoa(binaryString);
+ return base64.replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/g, '');📝 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.
| function convertBufferToBase64(buffer: ArrayBuffer): string { | |
| const byteArray = new Uint8Array(buffer); | |
| let binaryString = ''; | |
| for (let i = 0; i < byteArray.byteLength; i++) { | |
| binaryString += String.fromCharCode(byteArray[i]); | |
| } | |
| return btoa(binaryString); | |
| } | |
| function convertBufferToBase64(buffer: ArrayBuffer): string { | |
| const byteArray = new Uint8Array(buffer); | |
| let binaryString = ''; | |
| for (let i = 0; i < byteArray.byteLength; i++) { | |
| binaryString += String.fromCharCode(byteArray[i]); | |
| } | |
| const base64 = btoa(binaryString); | |
| return base64.replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/g, ''); | |
| } |
🤖 Prompt for AI Agents
In packages/davinci-client/src/lib/fido/fido.utils.ts around lines 17 to 24, the
helper currently returns RFC‑4648 standard base64 which uses '+', '/', '=';
WebAuthn expects base64url. Update the function so after producing the base64
string it is converted to base64url by replacing '+' → '-', '/' → '_' and
removing any trailing '=' padding (or rename function to indicate base64url if
desired); keep the same ArrayBuffer→binary→btoa flow but post-process the btoa
result to emit base64url.
There was a problem hiding this comment.
Thank you, I will look into RFC 4648 tomorrow
There was a problem hiding this comment.
@ancheetah, sounds good! Feel free to reach out if you have any questions when you're looking into it. The key point is that WebAuthn specification requires base64url encoding (RFC 4648 §5) for all binary fields to ensure URL-safe transport without special character escaping.
There was a problem hiding this comment.
I don't believe this is true. The DaVinci sample payloads given to us use base64.
e.g.
{
"data": {
"formData": {
"fido2": {
"assertionValue": {
"id": "p_DyLMDrLOpMbuDLA-wnFA",
"rawId": "p/DyLMDrLOpMbuDLA+wnFA==",
"type": "public-key",
"response": {
"clientDataJSON": "eyJ0eXBlIjoid2ViYXV0aG4uZ2V0IiwiY2hhbGxlbmdlIjoiTklWc2ZTM2VqaXZzQ29HT2JZNXRTZGVrekxHcElwXzEwcVJZM1NMcVZoTSIsIm9yaWdpbiI6Imh0dHBzOi8vYXV0aC50ZXN0LW9uZS1waW5nb25lLmNvbSIsImNyb3NzT3JpZ2luIjpmYWxzZX0=",
"authenticatorData": "noTurEqraz6u+ayaXHxYtpg5+6QTa/glcysxIzEUSAEdAAAAAA==",
"signature": "MEUCIDlIH5nDKPMz1jlbwetA3VlGZdG5obGnKmlErO8d3GQjAiEApkrYhorHM6aqpRsHoUUEpPCttp0vRUGLBdtxCJlbH2k=",
"userHandle": "Yf6bZeO7hy1QVGsd8CjHVVXceG/q5zzDFEVJrcHZ5HA="
}
}
}
}
}
}
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PingOne DaVinci FIDO WebAuthn base64 encoding requirements
💡 Result:
Short answer: use base64url (URL-safe base64, RFC 4648 §5 — i.e., '-' and '_' instead of '+' and '/', typically without '=' padding) for WebAuthn fields and PingOne DaVinci/FIDO challenges. The challenge must decode to at least 32 bytes. Key points and sources:
- PingOne: custom FIDO2 challenge must be a valid Base64URL string that decodes to a data array of at least 32 bytes. (apidocs.pingidentity.com) [PingOne MFA API].
- Ping connector docs also reference Base64URL for FIDO/WebAuthn challenge usage. (docs.pingidentity.com).
- WebAuthn / FIDO specs require base64url encoding for buffer fields (clientDataJSON, attestationObject, authenticatorData, signature, id/rawId/userHandle when transported as JSON). Use base64url (no padding) per the spec. (w3c.github.io).
If you want, tell me which field(s) or example data you need (challenge, clientDataJSON, rawId, etc.) and I’ll give exact encode/decode examples.
Citations:
- 1: https://apidocs.pingidentity.com/pingone/mfa/v1/api/?utm_source=openai
- 2: https://docs.pingidentity.com/connectors/p1_mfa_connector.html?utm_source=openai
- 3: https://w3c.github.io/webauthn/?utm_source=openai
🌐 Web query:
DaVinci FIDO2 assertionValue format specification
💡 Result:
I couldn't find any public specification named "DaVinci FIDO2 assertionValue." If you mean the standard FIDO2 / WebAuthn assertion format (authenticatorData + signature over clientDataHash), see the WebAuthn spec and libfido2 docs for the exact byte/CBOR layouts and signature rules. (w3.org)
Sources:
- W3C WebAuthn spec (assertion signature / authenticatorData format). [1]. (w3.org)
- libfido2 fido2-assert / input-output format (practical examples). [2]. (developers.yubico.com)
If "DaVinci" is a vendor/product name, please tell me the vendor or provide a link or sample assertionValue and I’ll search for that specific format.
Citations:
- 1: https://www.w3.org/TR/webauthn-3/?utm_source=openai
- 2: https://www.w3.org/TR/webauthn-3/?utm_source=openai
- 3: https://developers.yubico.com/libfido2/Manuals/fido2-assert.html?utm_source=openai
@ancheetah, thank you for the correction and the sample payload! You're absolutely right. Looking at your example, I can see that while the id field uses base64url (- instead of +), the rawId and all the response fields (clientDataJSON, authenticatorData, signature, userHandle) use standard base64 with = padding and +// characters.
While the WebAuthn specification technically requires base64url encoding, your sample shows that DaVinci's actual implementation accepts (and expects) standard base64 for those response fields. Since you have the actual sample payloads from the DaVinci team, that's the authoritative source for what the API expects.
The current implementation using standard btoa() is correct for DaVinci. My initial suggestion to convert to base64url would have broken compatibility with DaVinci's API. I apologize for the confusion!
✏️ Learnings added
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.779Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Thank you for bringing this up. I will double check with DaVinci team next week. It seems the samples for registration may use base64url encoding while the samples provided for authentication use base64.
There was a problem hiding this comment.
@ancheetah, that's a great catch! An inconsistency between registration and authentication encoding would definitely need clarification. It's good that you're verifying with the DaVinci team—they'll be able to confirm whether:
- Both flows should use the same encoding (either base64 or base64url)
- They intentionally use different encodings for different flows
- The samples you have reflect the actual API behavior
Once you hear back from them, feel free to ping me and I can help review the code to ensure it matches whatever encoding format they confirm. In the meantime, the current implementation uses standard base64 for both flows, so if registration needs base64url, we can update convertBufferToBase64 or add a separate helper function as needed.
🧠 Learnings used
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.779Z
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.
| return { | ||
| ...options, | ||
| challenge: convertArrayToBuffer(options.challenge), | ||
| user: { | ||
| ...options.user, | ||
| id: convertArrayToBuffer(options.user.id), | ||
| }, | ||
| pubKeyCredParams, | ||
| excludeCredentials, | ||
| }; |
There was a problem hiding this comment.
Convert extension byte arrays before calling navigator.credentials.create.
DaVinci can send registration extensions (e.g., large blob, prf) as number arrays. Leaving them untouched means we hand number[] to WebAuthn, which throws a TypeError because the spec requires BufferSource. That blocks registration whenever those extensions are enabled—this is a release stopper.
Apply this diff:
- return {
- ...options,
+ const extensions = options.extensions
+ ? Object.fromEntries(
+ Object.entries(options.extensions).map(([name, value]) => {
+ if (Array.isArray(value) && value.every((item) => typeof item === 'number')) {
+ return [name, convertArrayToBuffer(value as number[])];
+ }
+ return [name, value];
+ }),
+ )
+ : undefined;
+
+ return {
+ ...options,
challenge: convertArrayToBuffer(options.challenge),
user: {
...options.user,
id: convertArrayToBuffer(options.user.id),
},
pubKeyCredParams,
excludeCredentials,
+ extensions,
};📝 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.
| return { | |
| ...options, | |
| challenge: convertArrayToBuffer(options.challenge), | |
| user: { | |
| ...options.user, | |
| id: convertArrayToBuffer(options.user.id), | |
| }, | |
| pubKeyCredParams, | |
| excludeCredentials, | |
| }; | |
| const extensions = options.extensions | |
| ? Object.fromEntries( | |
| Object.entries(options.extensions).map(([name, value]) => { | |
| if (Array.isArray(value) && value.every((item) => typeof item === 'number')) { | |
| return [name, convertArrayToBuffer(value as number[])]; | |
| } | |
| return [name, value]; | |
| }), | |
| ) | |
| : undefined; | |
| return { | |
| ...options, | |
| challenge: convertArrayToBuffer(options.challenge), | |
| user: { | |
| ...options.user, | |
| id: convertArrayToBuffer(options.user.id), | |
| }, | |
| pubKeyCredParams, | |
| excludeCredentials, | |
| extensions, | |
| }; |
🤖 Prompt for AI Agents
In packages/davinci-client/src/lib/fido/fido.utils.ts around lines 45 to 54, the
returned options don't convert registration extension byte arrays to
BufferSource, so pass-through number[] values to navigator.credentials.create
will throw a TypeError; fix this by mapping options.extensions into a new object
where any extension value that is a number[] (or Array of numbers) is replaced
with convertArrayToBuffer(value) (handle nested/typed arrays if present), leave
non-byte-array values unchanged, and include that as the extensions property in
the returned object (preserve all other fields: challenge, user.id,
pubKeyCredParams, excludeCredentials).
| return { | ||
| ...options, | ||
| challenge, | ||
| allowCredentials, | ||
| }; |
There was a problem hiding this comment.
Do the same extension conversion for authentication options.
Authentication extensions (e.g., largeBlob, prf) suffer from the same number[] → BufferSource mismatch. Without conversion navigator.credentials.get rejects the request, so relying flows fail outright.
Apply this diff:
- return {
- ...options,
+ const extensions = options.extensions
+ ? Object.fromEntries(
+ Object.entries(options.extensions).map(([name, value]) => {
+ if (Array.isArray(value) && value.every((item) => typeof item === 'number')) {
+ return [name, convertArrayToBuffer(value as number[])];
+ }
+ return [name, value];
+ }),
+ )
+ : undefined;
+
+ return {
+ ...options,
challenge,
allowCredentials,
+ extensions,
};🤖 Prompt for AI Agents
In packages/davinci-client/src/lib/fido/fido.utils.ts around lines 100-104, the
returned authentication options currently include raw extension values that may
be number[] and cause navigator.credentials.get to reject; convert any extension
values that are number[] into BufferSource (e.g., new Uint8Array(array).buffer)
the same way we do for registration options before returning. Detect
options.extensions, map over its properties, replace number[] values with
Uint8Array(...).buffer (and leave other types untouched), then return the
modified options including challenge and allowCredentials.
ryanbas21
left a comment
ryanbas21
left a comment
There was a problem hiding this comment.
Nothing blocking but outstanding questions and one np
| }, | ||
| }), | ||
| ), | ||
| Micro.flatMap((assertion) => { |
There was a problem hiding this comment.
nitpick but i think this should be map
There was a problem hiding this comment.
I thought that as well but then the type of the result of running getAssertionu didn't quite seem right.
Type 'Micro<never, GenericError, never> | Micro<FidoAuthenticationInputValue, never, never>' is not assignable to type 'GenericError | FidoAuthenticationInputValue'
| } | ||
|
|
||
| export interface AttestationValue | ||
| extends Omit<PublicKeyCredential, 'rawId' | 'response' | 'getClientExtensionResults' | 'toJSON'> { |
There was a problem hiding this comment.
So are we omitting them because we want a complete override below? it seems odd to omit them but then redefine many of the same fields?
There was a problem hiding this comment.
Honestly, I wasn't sure what the best way was to do this. This was the only way I could get the properties to override. Open to other suggestions.
| } | ||
|
|
||
| export interface AssertionValue | ||
| extends Omit<PublicKeyCredential, 'rawId' | 'response' | 'getClientExtensionResults' | 'toJSON'> { |
There was a problem hiding this comment.
I suppose same question, maybe i'm not understanding something regarding this
ancheetah
force-pushed
the
SDKS-4377-dv-mfa
branch
from
5d5c2b6 to
fd99414
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/davinci-client/src/lib/fido/README.md (2)
5-5: Consider hyphenating compound adjective."WebAuthn compatible" should be "WebAuthn-compatible" when used as a compound adjective modifying "options".
As per static analysis hints.
28-28: Use proper markdown headings instead of bold text.Lines 28 and 34 use bold emphasis for section titles. Convert to proper markdown headings for better structure.
Apply this diff:
-**register(options: FidoRegistrationOptions) => Promise<FidoRegistrationInputValue | GenericError>** +#### register(options: FidoRegistrationOptions) => Promise<FidoRegistrationInputValue | GenericError> -**authenticate: (options: FidoAuthenticationOptions) => Promise<FidoAuthenticationInputValue | GenericError>** +#### authenticate(options: FidoAuthenticationOptions) => Promise<FidoAuthenticationInputValue | GenericError>As per static analysis hints.
Also applies to: 34-34
packages/davinci-client/src/lib/fido/fido.ts (1)
66-66: Consider using structured logging instead of console.error.Both registration and authentication methods use
console.errorfor logging failures. Consider using the@forgerock/sdk-logger(already a dependency) for consistent structured logging across the SDK.Also applies to: 119-119
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
.changeset/sparkly-dragons-lay.md(1 hunks)package.json(1 hunks)packages/davinci-client/package.json(1 hunks)packages/davinci-client/src/index.ts(1 hunks)packages/davinci-client/src/lib/client.store.ts(1 hunks)packages/davinci-client/src/lib/client.types.test-d.ts(2 hunks)packages/davinci-client/src/lib/client.types.ts(2 hunks)packages/davinci-client/src/lib/collector.types.ts(4 hunks)packages/davinci-client/src/lib/davinci.types.ts(1 hunks)packages/davinci-client/src/lib/davinci.utils.ts(1 hunks)packages/davinci-client/src/lib/fido/README.md(1 hunks)packages/davinci-client/src/lib/fido/fido.ts(1 hunks)packages/davinci-client/src/lib/fido/fido.utils.test.ts(1 hunks)packages/davinci-client/src/lib/fido/fido.utils.ts(1 hunks)packages/davinci-client/src/types.ts(2 hunks)packages/sdk-types/src/lib/error.types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/sdk-types/src/lib/error.types.ts
- package.json
- packages/davinci-client/src/lib/client.types.test-d.ts
- .changeset/sparkly-dragons-lay.md
- packages/davinci-client/src/lib/fido/fido.utils.test.ts
- packages/davinci-client/src/types.ts
- packages/davinci-client/src/index.ts
- packages/davinci-client/src/lib/client.types.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ancheetah
Repo: ForgeRock/ping-javascript-sdk PR: 417
File: packages/sdk-effects/storage/src/lib/storage.effects.ts:11-12
Timestamp: 2025-09-23T20:50:26.537Z
Learning: When the storage client API was changed from returning void to GenericError | null in SDKS-4361, call sites in DaVinci client and OIDC client were not updated to handle the returned errors, creating a follow-up task for proper error handling implementation.
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.779Z
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.
📚 Learning: 2025-11-07T21:35:14.779Z
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.779Z
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.
Applied to files:
packages/davinci-client/src/lib/fido/fido.utils.tspackages/davinci-client/src/lib/fido/README.md
🧬 Code graph analysis (3)
packages/davinci-client/src/lib/fido/fido.ts (4)
packages/davinci-client/src/lib/davinci.types.ts (2)
FidoRegistrationOptions(163-183)FidoAuthenticationOptions(195-203)packages/davinci-client/src/lib/collector.types.ts (2)
FidoRegistrationInputValue(555-557)FidoAuthenticationInputValue(576-578)packages/sdk-types/src/lib/error.types.ts (1)
GenericError(7-24)packages/davinci-client/src/lib/fido/fido.utils.ts (4)
transformRegistrationOptions(32-55)transformPublicKeyCredential(63-85)transformAuthenticationOptions(93-108)transformAssertion(116-141)
packages/davinci-client/src/lib/fido/fido.utils.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (2)
FidoRegistrationOptions(163-183)FidoAuthenticationOptions(195-203)packages/davinci-client/src/lib/collector.types.ts (2)
FidoRegistrationInputValue(555-557)FidoAuthenticationInputValue(576-578)
packages/davinci-client/src/lib/collector.types.ts (2)
packages/davinci-client/src/lib/davinci.types.ts (2)
FidoRegistrationOptions(163-183)FidoAuthenticationOptions(195-203)packages/davinci-client/src/types.ts (3)
ProtectCollector(52-52)FidoRegistrationCollector(53-53)FidoAuthenticationCollector(54-54)
🪛 LanguageTool
packages/davinci-client/src/lib/fido/README.md
[grammar] ~5-~5: Use a hyphen to join words.
Context: ...sform options from DaVinci into WebAuthn compatible options, then call `navigator...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
packages/davinci-client/src/lib/fido/README.md
28-28: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
34-34: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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). (1)
- GitHub Check: Mend Code Security Check
🔇 Additional comments (19)
packages/davinci-client/src/lib/client.store.ts (1)
48-48: LGTM! Type-only import is appropriate here.Converting to
import typeis correct sinceContinueNodeandStartNodeare used exclusively in type positions (type parameters, assertions, and casts). This improves tree-shaking and makes the intent explicit.packages/davinci-client/src/lib/davinci.utils.ts (1)
47-48: Code is correct—no additional auto-collector categories exist.The filter at lines 47-48 includes all valid auto-collector categories. The
AutoCollectorCategoriestype defines exactly two categories:'SingleValueAutoCollector'and'ObjectValueAutoCollector'. Both are already present in the filter, making it complete.packages/davinci-client/package.json (1)
32-32: LGTM!The Effect library dependency is appropriate for the new FIDO module's async flow control using
Micro.packages/davinci-client/src/lib/fido/README.md (1)
40-70: LGTM!The usage examples accurately demonstrate registration and authentication flows with proper error handling and collector integration.
packages/davinci-client/src/lib/collector.types.ts (6)
8-8: LGTM!The import of FIDO options types is necessary for the new FIDO collector output value interfaces.
541-544: LGTM!The ProtectOutputValue interface clearly defines the configuration for protect collectors.
546-563: LGTM!The Omit pattern is appropriate here. It removes properties from PublicKeyCredential that are being overridden (rawId, response) or are methods not needed for serialization (getClientExtensionResults, toJSON). This allows type-safe redefinition of rawId and response as base64 strings for JSON transport to DaVinci.
565-584: LGTM!AssertionValue and related authentication types follow the same pattern as the registration types, providing consistent serialization for WebAuthn credentials.
594-616: LGTM!The generic extension to AutoCollector enables type-safe output configuration while maintaining backward compatibility with the default
Record<string, unknown>.
618-635: LGTM!The collector type definitions properly utilize the new generic parameters, creating fully type-safe FIDO registration and authentication collectors.
packages/davinci-client/src/lib/davinci.types.ts (1)
163-183: LGTM!The extension of the Omit clause and redefinition of
pubKeyCredParamsandexcludeCredentialsis necessary to match DaVinci's JSON serialization format, where IDs are transmitted asnumber[]arrays and algorithm identifiers can be either string or number. This is consistent with the existing pattern forchallengeanduser.id.packages/davinci-client/src/lib/fido/fido.ts (3)
23-42: LGTM!The FidoClient interface is well-documented with complete JSDoc annotations and proper TypeScript signatures.
55-104: LGTM!The register implementation properly handles the WebAuthn flow with appropriate error handling and null checks. The Micro pipeline structure provides clear error boundaries.
108-155: LGTM!The authenticate implementation follows the same pattern as register with proper error handling and null checks for the returned credential.
packages/davinci-client/src/lib/fido/fido.utils.ts (5)
17-24: LGTM!The base64 encoding implementation is correct for DaVinci's API requirements, which accepts standard base64 encoding (RFC 4648 §4).
Based on learnings.
45-54: Verify extension field conversion requirements.The function currently does not convert extension fields that may contain byte arrays. The PR objectives list "convert extensions arrays to BufferSources" as a TODO. If DaVinci sends extensions with
number[]values (e.g., forlargeBlob,prf), these will need to be converted toArrayBufferbefore passing tonavigator.credentials.create(), otherwise a TypeError will occur.Confirm with the DaVinci team whether extensions are sent in the current API and if they contain byte arrays. If yes, implement conversion before release:
const extensions = options.extensions ? Object.fromEntries( Object.entries(options.extensions).map(([name, value]) => { if (Array.isArray(value) && value.every((item) => typeof item === 'number')) { return [name, convertArrayToBuffer(value as number[])]; } return [name, value]; }), ) : undefined; return { ...options, challenge: convertArrayToBuffer(options.challenge), user: { ...options.user, id: convertArrayToBuffer(options.user.id), }, pubKeyCredParams, excludeCredentials, extensions, };
63-85: LGTM!The credential transformation properly converts ArrayBuffer fields to base64 strings for DaVinci transport while preserving all other credential properties.
103-107: Same extension conversion consideration applies here.Authentication options may also contain extensions with byte arrays. If DaVinci sends authentication extensions (e.g.,
largeBlob,prf), apply the same conversion logic as suggested for registration options.
116-141: LGTM!The assertion transformation properly handles all AuthenticatorAssertionResponse fields, including the optional
userHandle, converting them to base64 strings for DaVinci transport.
| function convertArrayToBuffer(arr: number[]): ArrayBuffer { | ||
| return new Int8Array(arr).buffer; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use Uint8Array instead of Int8Array for byte array conversion.
The current implementation uses Int8Array, which interprets values as signed (-128 to 127). Since WebAuthn binary data represents unsigned byte arrays, use Uint8Array to avoid potential issues with values in the 128-255 range.
Apply this diff:
function convertArrayToBuffer(arr: number[]): ArrayBuffer {
- return new Int8Array(arr).buffer;
+ return new Uint8Array(arr).buffer;
}📝 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.
| function convertArrayToBuffer(arr: number[]): ArrayBuffer { | |
| return new Int8Array(arr).buffer; | |
| } | |
| function convertArrayToBuffer(arr: number[]): ArrayBuffer { | |
| return new Uint8Array(arr).buffer; | |
| } |
🤖 Prompt for AI Agents
In packages/davinci-client/src/lib/fido/fido.utils.ts around lines 13 to 15, the
function converting a number array to ArrayBuffer uses Int8Array which treats
bytes as signed; update it to use Uint8Array so values 0-255 are preserved as
unsigned when producing the ArrayBuffer. Replace the Int8Array creation with a
Uint8Array created from the input array and return its buffer, leaving the
function signature and return type unchanged.
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4377
Description
TODO:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores