feat: add PasskeyModule for WebAuthn P-256 session key registration a…#948
Merged
Conversation
…nd signing Implements ancore-org#859. Adds packages/account-abstraction/src/passkey/passkeyModule.ts with three public functions: - registerPasskey(): calls navigator.credentials.create() with ES256 (alg: -7), parses the SubjectPublicKeyInfo DER to extract P-256 x/y coordinates, and returns the credentialId plus key coordinates ready for on-chain registration - buildAddSessionKeyInvocation(): builds an add_session_key contract invocation using the P-256 x-coordinate (32 bytes) as BytesN<32> with PERMISSION_EXECUTE - signRelayPayload(): SHA-256 hashes the canonical payload JSON as the WebAuthn challenge, converts the DER ECDSA response to compact r‖s (128 hex chars), and returns authenticatorData + clientDataJSON for relay server verification Also adds typed PasskeyError hierarchy, 27 unit tests with mocked navigator.credentials, DOM lib to the package tsconfig, and barrel exports. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@Hahfyeex Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughAdds ChangesPasskeyModule WebAuthn P-256 Registration and Signing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/account-abstraction/src/__tests__/passkey-module.test.ts (1)
244-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
expiresAtandPERMISSION_EXECUTEto match the test's intent.The test name claims "x-coordinate and PERMISSION_EXECUTE", and
argslength is asserted as 3, but onlyargs[0](the x-coordinate) is validated. The expiry (args[1]) and permission (args[2]) encodings are unchecked, so a regression in either would pass silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/account-abstraction/src/__tests__/passkey-module.test.ts` around lines 244 - 251, The test for buildAddSessionKeyInvocation only verifies the x-coordinate, so it can miss regressions in the other encoded arguments. Update the assertions in passkey-module.test.ts to also validate inv.args[1] for the expiresAt encoding and inv.args[2] for PERMISSION_EXECUTE, using the same invocation object from buildAddSessionKeyInvocation so the test matches its stated intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/account-abstraction/src/__tests__/passkey-module.test.ts`:
- Around line 286-295: The current test only verifies DER padding removal in
signRelayPayload and does not cover signature normalization for a high-s
signature. Update the passkey-module test by introducing a high-s vector for
mockS, then assert that the compact signature output uses n - s instead of the
original high-s value; keep the existing mockGet/makeAssertionCredential flow
and use signRelayPayload plus the returned signature to validate both padding
stripping and low-s normalization.
In `@packages/account-abstraction/src/passkey/passkeyModule.ts`:
- Around line 289-292: The WebAuthn awaits in passkeyModule should not leak DOM
exceptions; wrap the navigator.credentials.create flow in the registration path
and the corresponding signing path in try/catch, then rethrow as
PasskeyRegistrationError or PasskeySigningError respectively. Update the logic
around the credential creation/assertion calls so browser cancellation and
authenticator failures are converted into the module’s promised error types,
while keeping the existing null/type checks for non-exception failures.
- Around line 232-237: The challenge canonicalization in passkeyModule.ts only
normalizes the top-level payload shape, so nested parameters can still hash
differently across equivalent requests. Update the canonicalization used in the
WebAuthn challenge flow around the payload JSON.stringify in the passkeyModule
code to recursively normalize nested payload data, especially
payload.parameters, before hashing so the same logical request always produces
the same challenge.
- Around line 211-220: The DER-to-compact signature parsing logic currently
strips padding for r and s but does not normalize s to low-s, so update the
signature helper in passkeyModule.ts to canonicalize s before building the
fixed-width output. Detect high-s values, convert them to the low-s equivalent
using the curve order, then continue returning the 32-byte rFixed and sFixed
pair so the compact signature is non-malleable and verifier-friendly.
- Around line 121-129: The WebAuthn guard in assertWebAuthnSupport() only
validates navigator.credentials.create, but signRelayPayload() also relies on
navigator.credentials.get. Update the guard to check that
navigator.credentials.get is available as a function as well, and keep throwing
PasskeyNotSupportedError when either WebAuthn method is missing so partial mocks
fail consistently.
---
Nitpick comments:
In `@packages/account-abstraction/src/__tests__/passkey-module.test.ts`:
- Around line 244-251: The test for buildAddSessionKeyInvocation only verifies
the x-coordinate, so it can miss regressions in the other encoded arguments.
Update the assertions in passkey-module.test.ts to also validate inv.args[1] for
the expiresAt encoding and inv.args[2] for PERMISSION_EXECUTE, using the same
invocation object from buildAddSessionKeyInvocation so the test matches its
stated intent.
🪄 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: 098b6555-d030-4eb6-a028-c374ae56199f
📒 Files selected for processing (4)
packages/account-abstraction/src/__tests__/passkey-module.test.tspackages/account-abstraction/src/index.tspackages/account-abstraction/src/passkey/passkeyModule.tspackages/account-abstraction/tsconfig.json
Comment on lines
+286
to
+295
| it('strips the leading 0x00 DER padding byte from r and s', async () => { | ||
| const paddedDer = buildDerSig(mockR, mockS, true, true); | ||
| mockGet.mockResolvedValue(makeAssertionCredential({ sig: paddedDer })); | ||
|
|
||
| const { signature } = await signRelayPayload(unsignedPayload, 'AQID'); | ||
|
|
||
| expect(signature).toHaveLength(128); | ||
| const rHex = mockR.reduce((acc, b) => acc + b.toString(16).padStart(2, '0'), ''); | ||
| expect(signature.slice(0, 64)).toBe(rHex); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'parseDerSignature|low[-_ ]?s|n\s*-\s*s|SECP256R1|0xFFFFFFFF00000000' packages/account-abstraction/src/passkey/passkeyModule.ts -C3Repository: ancore-org/ancore
Length of output: 1136
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the signing path around parseDerSignature and signature assembly.
sed -n '160,420p' packages/account-abstraction/src/passkey/passkeyModule.ts
# Search for any low-s normalization logic anywhere in the passkey module/package.
rg -n --hidden -S 'low[-_ ]?s|n\s*-\s*s|canonical low[-_ ]?s|SECP256R1|P-256|signature.*normalize|normalize.*signature' packages/account-abstraction/src -C 3
# Inspect the related test file for existing high-s coverage.
sed -n '1,380p' packages/account-abstraction/src/__tests__/passkey-module.test.tsRepository: ancore-org/ancore
Length of output: 33276
Exercise the high-s path too. This case only strips DER padding; mockS is already below n/2, so it doesn’t cover signature normalization. Add a high-s vector and assert the compact result uses n - s, since signRelayPayload currently just returns r || s unchanged.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/account-abstraction/src/__tests__/passkey-module.test.ts` around
lines 286 - 295, The current test only verifies DER padding removal in
signRelayPayload and does not cover signature normalization for a high-s
signature. Update the passkey-module test by introducing a high-s vector for
mockS, then assert that the compact signature output uses n - s instead of the
original high-s value; keep the existing mockGet/makeAssertionCredential flow
and use signRelayPayload plus the returned signature to validate both padding
stripping and low-s normalization.
Comment on lines
+121
to
+129
| function assertWebAuthnSupport(): void { | ||
| if ( | ||
| typeof navigator === 'undefined' || | ||
| typeof navigator.credentials === 'undefined' || | ||
| typeof navigator.credentials.create !== 'function' | ||
| ) { | ||
| throw new PasskeyNotSupportedError(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline packages/account-abstraction/src/passkey/passkeyModule.ts --view expanded || true
printf '\n== relevant lines ==\n'
sed -n '1,260p' packages/account-abstraction/src/passkey/passkeyModule.ts | cat -n
printf '\n== usages ==\n'
rg -n "assertWebAuthnSupport|signRelayPayload|navigator\.credentials\.(create|get)" packages/account-abstraction/src/passkey -S || trueRepository: ancore-org/ancore
Length of output: 14427
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '340,390p' packages/account-abstraction/src/passkey/passkeyModule.ts | cat -nRepository: ancore-org/ancore
Length of output: 2169
Check navigator.credentials.get() in the WebAuthn guard. signRelayPayload() calls navigator.credentials.get(), but assertWebAuthnSupport() only checks create(). In partial mocks or incomplete WebAuthn implementations, signing can throw a raw TypeError instead of PasskeyNotSupportedError.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/account-abstraction/src/passkey/passkeyModule.ts` around lines 121 -
129, The WebAuthn guard in assertWebAuthnSupport() only validates
navigator.credentials.create, but signRelayPayload() also relies on
navigator.credentials.get. Update the guard to check that
navigator.credentials.get is available as a function as well, and keep throwing
PasskeyNotSupportedError when either WebAuthn method is missing so partial mocks
fail consistently.
Comment on lines
+211
to
+220
| // Strip DER sign-padding zero. | ||
| if (r.length === 33 && r[0] === 0x00) r = r.slice(1); | ||
| if (s.length === 33 && s[0] === 0x00) s = s.slice(1); | ||
|
|
||
| const rFixed = new Uint8Array(32); | ||
| const sFixed = new Uint8Array(32); | ||
| rFixed.set(r, 32 - r.length); | ||
| sFixed.set(s, 32 - s.length); | ||
|
|
||
| return { r: rFixed, s: sFixed }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Normalize s to low-s before returning the compact signature.
The DER parser strips padding but returns high-s signatures unchanged, which misses the linked requirement and can produce malleable or verifier-rejected signatures.
Suggested fix
+const P256_N = BigInt('0xffffffff00000000ffffffffffffffffbce6faada7179e84f3b9cac2fc632551');
+const P256_HALF_N = P256_N >> 1n;
+
+function bytesToBigInt(bytes: Uint8Array): bigint {
+ return BigInt(`0x${toHex(bytes)}`);
+}
+
+function bigIntTo32Bytes(value: bigint): Uint8Array {
+ const hex = value.toString(16).padStart(64, '0');
+ const out = new Uint8Array(32);
+ for (let i = 0; i < 32; i++) {
+ out[i] = Number.parseInt(hex.slice(i * 2, i * 2 + 2), 16);
+ }
+ return out;
+}
+
// Strip DER sign-padding zero.
if (r.length === 33 && r[0] === 0x00) r = r.slice(1);
if (s.length === 33 && s[0] === 0x00) s = s.slice(1);
+
+ if (r.length > 32 || s.length > 32) {
+ throw new PasskeySigningError('DER signature: invalid P-256 integer length');
+ }
const rFixed = new Uint8Array(32);
const sFixed = new Uint8Array(32);
rFixed.set(r, 32 - r.length);
sFixed.set(s, 32 - s.length);
- return { r: rFixed, s: sFixed };
+ const sValue = bytesToBigInt(sFixed);
+ return { r: rFixed, s: sValue > P256_HALF_N ? bigIntTo32Bytes(P256_N - sValue) : sFixed };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Strip DER sign-padding zero. | |
| if (r.length === 33 && r[0] === 0x00) r = r.slice(1); | |
| if (s.length === 33 && s[0] === 0x00) s = s.slice(1); | |
| const rFixed = new Uint8Array(32); | |
| const sFixed = new Uint8Array(32); | |
| rFixed.set(r, 32 - r.length); | |
| sFixed.set(s, 32 - s.length); | |
| return { r: rFixed, s: sFixed }; | |
| const P256_N = BigInt('0xffffffff00000000ffffffffffffffffbce6faada7179e84f3b9cac2fc632551'); | |
| const P256_HALF_N = P256_N >> 1n; | |
| function bytesToBigInt(bytes: Uint8Array): bigint { | |
| return BigInt(`0x${toHex(bytes)}`); | |
| } | |
| function bigIntTo32Bytes(value: bigint): Uint8Array { | |
| const hex = value.toString(16).padStart(64, '0'); | |
| const out = new Uint8Array(32); | |
| for (let i = 0; i < 32; i++) { | |
| out[i] = Number.parseInt(hex.slice(i * 2, i * 2 + 2), 16); | |
| } | |
| return out; | |
| } | |
| // Strip DER sign-padding zero. | |
| if (r.length === 33 && r[0] === 0x00) r = r.slice(1); | |
| if (s.length === 33 && s[0] === 0x00) s = s.slice(1); | |
| if (r.length > 32 || s.length > 32) { | |
| throw new PasskeySigningError('DER signature: invalid P-256 integer length'); | |
| } | |
| const rFixed = new Uint8Array(32); | |
| const sFixed = new Uint8Array(32); | |
| rFixed.set(r, 32 - r.length); | |
| sFixed.set(s, 32 - s.length); | |
| const sValue = bytesToBigInt(sFixed); | |
| return { r: rFixed, s: sValue > P256_HALF_N ? bigIntTo32Bytes(P256_N - sValue) : sFixed }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/account-abstraction/src/passkey/passkeyModule.ts` around lines 211 -
220, The DER-to-compact signature parsing logic currently strips padding for r
and s but does not normalize s to low-s, so update the signature helper in
passkeyModule.ts to canonicalize s before building the fixed-width output.
Detect high-s values, convert them to the low-s equivalent using the curve
order, then continue returning the 32-byte rFixed and sFixed pair so the compact
signature is non-malleable and verifier-friendly.
Comment on lines
+232
to
+237
| const canonical = JSON.stringify({ | ||
| nonce: payload.nonce, | ||
| operation: payload.operation, | ||
| parameters: payload.parameters, | ||
| sessionKey: payload.sessionKey, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Canonicalize nested parameters before hashing.
This only fixes top-level key order. If the relay/server canonicalizes nested parameters differently, equivalent payloads can hash to different WebAuthn challenges and fail verification.
Suggested fix
+function canonicalize(value: unknown): unknown {
+ if (Array.isArray(value)) {
+ return value.map(canonicalize);
+ }
+
+ if (value !== null && typeof value === 'object') {
+ return Object.fromEntries(
+ Object.entries(value as Record<string, unknown>)
+ .sort(([a], [b]) => a.localeCompare(b))
+ .map(([key, nested]) => [key, canonicalize(nested)])
+ );
+ }
+
+ return value;
+}
+
async function computePayloadChallenge(payload: UnsignedRelayPayload): Promise<Uint8Array> {
- const canonical = JSON.stringify({
+ const canonical = JSON.stringify(canonicalize({
nonce: payload.nonce,
operation: payload.operation,
parameters: payload.parameters,
sessionKey: payload.sessionKey,
- });
+ }));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const canonical = JSON.stringify({ | |
| nonce: payload.nonce, | |
| operation: payload.operation, | |
| parameters: payload.parameters, | |
| sessionKey: payload.sessionKey, | |
| }); | |
| function canonicalize(value: unknown): unknown { | |
| if (Array.isArray(value)) { | |
| return value.map(canonicalize); | |
| } | |
| if (value !== null && typeof value === 'object') { | |
| return Object.fromEntries( | |
| Object.entries(value as Record<string, unknown>) | |
| .sort(([a], [b]) => a.localeCompare(b)) | |
| .map(([key, nested]) => [key, canonicalize(nested)]) | |
| ); | |
| } | |
| return value; | |
| } | |
| async function computePayloadChallenge(payload: UnsignedRelayPayload): Promise<Uint8Array> { | |
| const canonical = JSON.stringify(canonicalize({ | |
| nonce: payload.nonce, | |
| operation: payload.operation, | |
| parameters: payload.parameters, | |
| sessionKey: payload.sessionKey, | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/account-abstraction/src/passkey/passkeyModule.ts` around lines 232 -
237, The challenge canonicalization in passkeyModule.ts only normalizes the
top-level payload shape, so nested parameters can still hash differently across
equivalent requests. Update the canonicalization used in the WebAuthn challenge
flow around the payload JSON.stringify in the passkeyModule code to recursively
normalize nested payload data, especially payload.parameters, before hashing so
the same logical request always produces the same challenge.
Comment on lines
+289
to
+292
| const credential = await navigator.credentials.create(creationOptions); | ||
|
|
||
| if (!credential || credential.type !== 'public-key') { | ||
| throw new PasskeyRegistrationError('Passkey creation was cancelled or failed'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wrap WebAuthn rejections in the module error types.
Browser cancellation and authenticator failures reject with DOM exceptions, so callers will not receive the promised PasskeyRegistrationError / PasskeySigningError unless these awaits are wrapped.
Suggested fix
- const credential = await navigator.credentials.create(creationOptions);
+ let credential: Credential | null;
+ try {
+ credential = await navigator.credentials.create(creationOptions);
+ } catch (error) {
+ const detail = error instanceof Error ? `: ${error.message}` : '';
+ throw new PasskeyRegistrationError(`Passkey creation failed${detail}`);
+ }
...
- const assertion = await navigator.credentials.get(requestOptions);
+ let assertion: Credential | null;
+ try {
+ assertion = await navigator.credentials.get(requestOptions);
+ } catch (error) {
+ const detail = error instanceof Error ? `: ${error.message}` : '';
+ throw new PasskeySigningError(`Passkey assertion failed${detail}`);
+ }Also applies to: 372-375
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/account-abstraction/src/passkey/passkeyModule.ts` around lines 289 -
292, The WebAuthn awaits in passkeyModule should not leak DOM exceptions; wrap
the navigator.credentials.create flow in the registration path and the
corresponding signing path in try/catch, then rethrow as
PasskeyRegistrationError or PasskeySigningError respectively. Update the logic
around the credential creation/assertion calls so browser cancellation and
authenticator failures are converted into the module’s promised error types,
while keeping the existing null/type checks for non-exception failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #859
#859 [ACCOUNT-ABSTRACTION] Add PasskeyModule for WebAuthn P-256 session key registration and signing
Repo Avatar
ancore-org/ancore
Feature Description
Add a PasskeyModule to packages/account-abstraction/ that wraps the WebAuthn API to register P-256 public keys as session keys on the smart account and sign relay payloads using the authenticator.
Problem Statement
Ancore's AA value proposition is passwordless UX via WebAuthn — users authenticate with Touch ID or Face ID instead of managing seed phrases. This flow is not implemented anywhere in the codebase.