fix: fido-errors #503
fix: fido-errors #503
Conversation
🦋 Changeset detectedLatest commit: b975a8b 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 |
WalkthroughAdds FIDO/WebAuthn error-code mapping and propagation, a per-call FIDO client logger config, replaces console.error with SDK logging, and ensures action requests include an explicit empty Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 b975a8b
☁️ 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: 3
🧹 Nitpick comments (1)
packages/davinci-client/src/lib/fido/fido.test.ts (1)
34-36: Consider stricter type guard for GenericError detection.The current
isGenericErroronly checks for anerrorproperty. For stronger validation, consider also checking for thetypeorcodeproperties that are part of theGenericErrorstructure.function isGenericError(result: unknown): result is GenericError { - return typeof result === 'object' && result !== null && 'error' in result; + return typeof result === 'object' && result !== null && 'error' in result && 'type' in result; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/fido-error-codes.md(1 hunks)packages/davinci-client/src/lib/davinci.utils.test.ts(1 hunks)packages/davinci-client/src/lib/davinci.utils.ts(1 hunks)packages/davinci-client/src/lib/fido/fido.test.ts(1 hunks)packages/davinci-client/src/lib/fido/fido.ts(7 hunks)packages/davinci-client/src/lib/fido/fido.types.test.ts(1 hunks)packages/davinci-client/src/lib/fido/fido.types.ts(1 hunks)packages/davinci-client/src/types.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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.
📚 Learning: 2025-09-23T20:50:26.537Z
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.
Applied to files:
.changeset/fido-error-codes.md
📚 Learning: 2025-11-07T21:35:14.799Z
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.
Applied to files:
.changeset/fido-error-codes.md
🧬 Code graph analysis (4)
packages/davinci-client/src/lib/fido/fido.types.test.ts (1)
packages/davinci-client/src/lib/fido/fido.types.ts (1)
toFidoErrorCode(51-56)
packages/davinci-client/src/lib/fido/fido.test.ts (2)
packages/davinci-client/src/lib/fido/fido.ts (2)
FidoClientConfig(29-35)fido(65-162)packages/davinci-client/src/lib/davinci.types.ts (2)
FidoRegistrationOptions(163-183)FidoAuthenticationOptions(195-203)
packages/davinci-client/src/lib/davinci.utils.test.ts (2)
packages/davinci-client/src/types.ts (1)
ContinueNode(28-28)packages/davinci-client/src/lib/node.types.ts (1)
ContinueNode(57-80)
packages/davinci-client/src/lib/fido/fido.ts (1)
packages/davinci-client/src/lib/fido/fido.types.ts (2)
toFidoErrorCode(51-56)createFidoError(65-76)
⏰ 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 (14)
packages/davinci-client/src/lib/davinci.utils.test.ts (1)
150-183: LGTM! Test coverage for formData in action requests.The new test case verifies that
formData: {}is included for FIDO error codes, and the existing test is updated to expect this field. This aligns with the API contract changes intransformActionRequest.packages/davinci-client/src/lib/fido/fido.types.test.ts (1)
10-57: Excellent test coverage for error code mapping.The test suite comprehensively validates all supported
FidoErrorCodevalues and edge cases including non-Error inputs and unrecognized DOMException names. Good defensive testing for the type guard logic.packages/davinci-client/src/types.ts (1)
8-9: LGTM! Public type exports properly extended.The new
FidoClientConfigandFidoErrorCodetypes are correctly exported for consumer access, enabling proper typing for FIDO error handling in downstream applications.Also applies to: 59-59
packages/davinci-client/src/lib/davinci.utils.ts (1)
99-104: LGTM! API contract consistency improvement.Adding
formData: {}to action requests ensures the payload structure is consistent with submit requests and meets API expectations. The change is minimal and well-tested..changeset/fido-error-codes.md (1)
1-13: Well-documented changeset with clear consumer guidance.The changeset correctly identifies this as a patch release and provides clear documentation of all changes, including the recommended pattern for propagating FIDO errors to DaVinci.
packages/davinci-client/src/lib/fido/fido.test.ts (2)
53-239: Thorough test coverage for registration error scenarios.The register tests comprehensively cover all
FidoErrorCodevalues including the null credential edge case, and properly validate theGenericErrorstructure (code, error, type, message). Good use of theisGenericErrorguard for narrowing.
242-429: Complete test coverage for authentication error scenarios.Mirror of registration tests for the authenticate flow. All error codes and edge cases are properly tested with consistent assertions.
packages/davinci-client/src/lib/fido/fido.types.ts (3)
15-31: Well-designed error code type system.The
FidoErrorCodeunion type andVALID_FIDO_ERROR_CODESset are correctly structured - the set intentionally excludesUnknownErrorsince it's the fallback for unrecognized errors. Clean implementation of the WebAuthn error mapping.
33-56: Robust type guards with proper null-safety.The
isErrorWithNameguard safely handles null/undefined and validates thenameproperty type. ThetoFidoErrorCodefunction provides a safe mapping withUnknownErrorfallback for unrecognized errors.
65-76: Clean GenericError factory function.
createFidoErrorprovides a consistent way to construct FIDO errors with the standardizedtype: 'fido_error'discriminator, enabling consumers to identify FIDO-specific errors.packages/davinci-client/src/lib/fido/fido.ts (4)
29-35: Well-designed configuration interface.The
FidoClientConfiginterface provides optional logger configuration with support for custom loggers, maintaining backward compatibility by making the entire config optional.
65-66: Sensible default log level.Defaulting to
'error'log level when not configured ensures production deployments aren't noisy while still capturing failure scenarios.
82-86: Proper integration of error code mapping.The catch handler correctly uses
toFidoErrorCodeto map WebAuthn errors andcreateFidoErrorto construct the standardized error object with logging.
129-133: Authentication error handling follows the same pattern.Consistent with registration error handling, properly maps WebAuthn errors and logs them.
![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.
ryanbas21
force-pushed
the
fido-errors
branch
from
ad510b6 to
9198a76
Compare
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 reran your CI automatically.
🎓 Learn more about Self-Healing CI on nx.dev
ryanbas21
force-pushed
the
fido-errors
branch
from
e4810fc to
b975a8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/davinci-client/src/lib/fido/fido.test.ts (2)
38-240: Consider adding a test for environments without WebAuthn supportRight now all tests assume
navigator.credentialsexists and is overridable. It might be worth adding one extra test that simulates a runtime with nonavigator.credentials(or missingcreate/get) and asserts that FIDO returns an appropriateGenericError(likely a not-supported flavor), to lock in behavior for non-WebAuthn-capable environments.
53-429: Optional: DRY up repeatednavigator.credentialsmockingMany tests repeat the same
Object.defineProperty(navigator, 'credentials', { value: { create|get: mockFn }, ... })setup. If this suite grows further, you may want to factor that into small helpers (e.g.,mockNavigatorCreate(mock)/mockNavigatorGet(mock)) to reduce duplication and make individual test intent even clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/fido-error-codes.md(1 hunks)packages/davinci-client/src/lib/davinci.utils.test.ts(1 hunks)packages/davinci-client/src/lib/davinci.utils.ts(1 hunks)packages/davinci-client/src/lib/fido/fido.test.ts(1 hunks)packages/davinci-client/src/lib/fido/fido.ts(7 hunks)packages/davinci-client/src/lib/fido/fido.types.test.ts(1 hunks)packages/davinci-client/src/lib/fido/fido.types.ts(1 hunks)packages/davinci-client/src/types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/davinci-client/src/lib/davinci.utils.test.ts
- packages/davinci-client/src/lib/fido/fido.ts
- packages/davinci-client/src/lib/fido/fido.types.ts
- packages/davinci-client/src/lib/davinci.utils.ts
- packages/davinci-client/src/lib/fido/fido.types.test.ts
- .changeset/fido-error-codes.md
🧰 Additional context used
🧠 Learnings (1)
📓 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.
⏰ 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). (3)
- GitHub Check: pr
- GitHub Check: Mend Code Security Check
- GitHub Check: Mend Security Check
🔇 Additional comments (7)
packages/davinci-client/src/types.ts (1)
8-9: FIDO type imports/exports align with public API designRe-exporting
FidoClient,FidoClientConfig, andFidoErrorCodevia the roottypesentrypoint is consistent with the rest of this module and cleanly exposes the new FIDO surface to consumers. No issues from a typing/API standpoint.Also applies to: 59-59
packages/davinci-client/src/lib/fido/fido.test.ts (6)
14-36: Helper setup and type guard are appropriate
silentConfig, the shared FIDO options fixtures, andisGenericErrorprovide a clear structure for the suite and correctly model theGenericErrorshape for these tests. This keeps individual test bodies focused on behavior instead of boilerplate.
38-52: Navigator credentials mocking/restoration is safe and containedCapturing
originalCredentialsand restoring it inafterEachviaObject.definePropertykeeps thenavigator.credentialsmocking isolated per test and avoids cross-test leakage. This is a solid pattern for these WebAuthn-style tests.
53-240: Registration error/success coverage looks completeThe
registersuite exercises all key WebAuthn error names plus unknown-error and null-credential cases, and verifies both thecodeand FIDO-specificerror/typefields, along with the success path (attestationValue). Behavior matches the intendedFidoErrorCodemapping and union return type.
242-429: Authentication error/success coverage is symmetrical and consistentThe
authenticatetests mirror the registration coverage for the same WebAuthn error names, unknown errors, null assertions, and success (assertionValue), which should keep the two flows in sync as the error surface evolves.
431-455: Error-detection pattern test nicely encodes the GenericError union contractThe
"error" in resultpattern plus the comments about TypeScript narrowing give consumers a clear, simple way to work with theGenericError | successunion returned by FIDO operations, reducing the risk of ignored errors we’ve seen with other APIs returningGenericError | null. Based on learnings, this is exactly the kind of explicit pattern that helps downstream call sites handle errors correctly.
457-490: Logger configuration tests validate both explicit and default pathsThe two logger tests confirm that:
- providing
silentConfigdoesn’t throw and still yields aGenericError, and- calling
fido()with no config also works and returns aGenericErrorfor WebAuthn failures.This correctly exercises both the optional logger configuration and default behavior.
Codecov Report❌ Patch coverage is
❌ Your project status has failed because the head coverage (19.23%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #503 +/- ##
==========================================
+ Coverage 18.79% 19.23% +0.43%
==========================================
Files 140 141 +1
Lines 27640 27674 +34
Branches 980 1011 +31
==========================================
+ Hits 5195 5322 +127
+ Misses 22445 22352 -93
🚀 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 2b97c43 to https://ForgeRock.github.io/ping-javascript-sdk/pr-503/2b97c43ee5fb09303a6eabae7d888eb76f0184ba branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/journey-client - 0.0 KB (-82.4 KB, -100.0%) 📊 Minor Changes📉 @forgerock/journey-client - 82.4 KB (-0.0 KB) ➖ No Changes➖ @forgerock/device-client - 9.2 KB 13 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4480
Description
Adds WebAuthn error code propagation for FIDO operations, allowing consumers to send standardized error codes back to DaVinci when FIDO registration or authentication fails.
One note: explicitly not auto-submitting the error, and leaving that to the developer
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.