feat(web): implement Passkey authentication #109
Conversation
# Users can authenticate with a simple biometric.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes add comprehensive Passkey API documentation and implement new endpoints for passkey registration and authentication. Each endpoint’s route includes a dedicated asynchronous POST function with robust error handling. In addition, new modules and utility functions are introduced to manage challenges, credentials, and cryptographic operations using Redis and Stellar. Environment variable validation is established via a schema using zod. A custom error handling mechanism is also incorporated along with a deployment utility for Stellar contracts. Finally, package dependencies are updated to support these functionalities. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as User Device
participant API as Passkey API
participant Handler as Registration Handler
Client->>API: POST /api/passkey/generate-registration-options<br>(identifier, origin)
API->>Handler: Call getRegistrationOptions
Handler-->>API: Return registration options
API-->>Client: Return registration options
Client->>API: POST /api/passkey/verify-registration<br>(registration response)
API->>Handler: Call verifyRegistration
Handler-->>API: Return verification result
API-->>Client: Return verification result
sequenceDiagram
participant Client as User Device
participant API as Passkey API
participant Handler as Authentication Handler
Client->>API: POST /api/passkey/generate-authentication-options<br>(identifier, origin)
API->>Handler: Call getAuthenticationOptions
Handler-->>API: Return authentication options
API-->>Client: Return authentication options
Client->>API: POST /api/passkey/verify-authentication<br>(authentication response)
API->>Handler: Call verifyAuthentication
Handler-->>API: Return verification result
API-->>Client: Return verification result
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
bot
added
documentation
espaciofuturoio
force-pushed
the
feat-passkey-backend
branch
from
effe717 to
0279408
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 31
🛑 Comments failed to post (31)
apps/web/lib/passkey/deploy.ts (5)
16-40: 🧹 Nitpick (assertive)
Team-based code clarity.
The generation ofdeployeeinvolves constructing a unique contract address usingFACTORY_CONTRACT_IDand a salt. This logic is concise and clear. However, it may be beneficial to add inline comments explaining how theContractIdPreimageis assembled to assist new contributors.
48-79: 🧹 Nitpick (assertive)
Consider broader simulation error details.
After simulating the transaction (simulateTransaction), the code checks if there's an error or restore, then immediately throws. In some cases, providing more context or user-friendly messages (e.g., “Insufficient funds” or “Invalid sequence number”) can help troubleshooting.
80-93: 🧹 Nitpick (assertive)
Robust Horizon response handling.
Currently, the code checkstxResp.successfuland otherwise throwstxResp. For improved user experience, consider examiningtxResp.extrasor other response fields to provide clearer error handling messages (e.g., insufficient fee, bad sequence, etc.).
1-14: 💡 Codebase verification
Action Required: Implement Environment Variable Validations in deploy.ts
The grep output confirms that the file only destructures values from
ENVwithout any explicit validation checks. Please add logic to verify thatRPC_URL,FACTORY_CONTRACT_ID,HORIZON_URL, andNETWORK_PASSPHRASEare defined (or non-empty) and throw a clear error if any are missing.
- File:
apps/web/lib/passkey/deploy.ts- Issue: Lack of validations for critical environment variables
🔗 Analysis chain
Ensure environment variables are validated.
The code relies onRPC_URL,FACTORY_CONTRACT_ID,HORIZON_URL, andNETWORK_PASSPHRASEfrom the environment. Consider verifying these values to handle missing or misconfigured environment variables gracefully (e.g., throw an explicit error if any is undefined).A quick check script might look like this:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash rg -A 3 'ENV.' | grep -E 'FACTORY_CONTRACT_ID|HORIZON_URL|RPC_URL|NETWORK_PASSPHRASE'Length of output: 194
41-47: 🧹 Nitpick (assertive)
Handle potential exceptions from
getContractData.
Callingrpc.getContractData(...)can fail if the contract data is not found or if there's a connection error. Currently, there's no explicit error handling or fallback logic. Consider catching exceptions or checking the status to avoid letting an unhandled rejection bubble up.if (!publicKey) { - await rpc.getContractData(deployee, xdr.ScVal.scvLedgerKeyContractInstance()) - return deployee + try { + await rpc.getContractData(deployee, xdr.ScVal.scvLedgerKeyContractInstance()) + return deployee + } catch (error) { + // Handle or re-throw with context + throw new Error(`Contract data retrieval failed: ${error}`) + } }📝 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.if (!publicKey) { try { await rpc.getContractData(deployee, xdr.ScVal.scvLedgerKeyContractInstance()) return deployee } catch (error) { // Handle or re-throw with context throw new Error(`Contract data retrieval failed: ${error}`) } }apps/web/lib/passkey/stellar.ts (3)
108-184: 🧹 Nitpick (assertive)
Consider using a well-tested library for ECDSA signature parsing.
convertEcdsaSignatureAsnToCompactmanually parses ASN.1 sequences and manipulates buffers. While instructive, it can be error-prone. If a widely adopted library exists (e.g., from a crypto library), it can reduce the risk of subtle bugs and make future maintenance easier.
1-13: 🧹 Nitpick (assertive)
Prevent unexpected truncation issues.
IntruncateAccount(), slicing a string assumes a minimum length. Consider a defensive check for very short strings or adding a comment to clarify assumptions about the account's expected format or length.export const truncateAccount = (account: string) => { - return `${account.slice(0, 5)}...${account.slice(-5)}` + if (account.length <= 10) { + return account + } + return `${account.slice(0, 5)}...${account.slice(-5)}` }📝 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.import type { AuthenticationResponseJSON, RegistrationResponseJSON, } from '@simplewebauthn/browser' import { hash } from '@stellar/stellar-sdk' import base64url from 'base64url' import { bigintToBuf, bufToBigint } from 'bigint-conversion' import * as CBOR from 'cbor-x/decode' export const truncateAccount = (account: string) => { if (account.length <= 10) { return account } return `${account.slice(0, 5)}...${account.slice(-5)}` }
80-106:
⚠️ Potential issueAvoid logging full registration data in production.
console.log(JSON.stringify(registration, null, 2))can leak sensitive PII or cryptographic details. Use an environment-based conditional or remove these logs in production builds.- console.log(JSON.stringify(registration, null, 2)) + // console.debug('Received registration payload (truncated)', registration.id)📝 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.export const getPublicKeys = async ( registration: RegistrationResponseJSON | AuthenticationResponseJSON, ) => { const contractSalt = hash(base64url.toBuffer(registration.id)) // console.debug('Received registration payload (truncated)', registration.id) if ('attestationObject' in registration.response) { // Handle RegistrationResponseJSON const { publicKeyObject } = getPublicKeyObject( registration.response.attestationObject, ) const publicKey = Buffer.from([ 4, // (0x04 prefix) https://en.bitcoin.it/wiki/Elliptic_Curve_Digital_Signature_Algorithm ...(publicKeyObject.get('-2') as Buffer), ...(publicKeyObject.get('-3') as Buffer), ]) return { contractSalt, publicKey, } } return { contractSalt, } }apps/web/lib/passkey/passkey.ts (5)
51-101: 🧹 Nitpick (assertive)
Avoid partial user creation errors.
When generating registration options, ifgetUserorsaveChallengefails, it may leave the system in an inconsistent state. Although these operations might rarely fail, consider wrapping them in a try-catch or adding transactional guarantees to handle partial failures gracefully.
103-164: 🧹 Nitpick (assertive)
Note concurrency or race conditions in challenge storage.
verifyRegistrationfetches a challenge from Redis and deletes it afterward. If multiple parallel requests are made, one might unintentionally invalidate the other's challenge. If this is undesired, consider a locking mechanism or a single-flight pattern to prevent collisions.
166-202: 🧹 Nitpick (assertive)
Check for unregistered users in
getAuthenticationOptions.
IfuserResponseis undefined, anUNEXPECTED_ERRORis thrown, but consider adding more specific debugging or user-friendly messages. This helps differentiate between a non-existent user vs. a database or caching issue.
204-261: 🧹 Nitpick (assertive)
Potential multi-credential concurrency edge cases.
verifyAuthenticationupdates the credential’s counter. If multiple authentications happen near-simultaneously, the counter might incorrectly update. Although rare, consider if the DB handles concurrency well or if a check for older counters is needed.
1-49: 💡 Codebase verification
Add validation to guarantee consistent lengths among ENV arrays
- The arrays
EXPECTED_ORIGIN,RP_NAME, andRP_IDare defined separately inapps/web/lib/passkey/env.tswith default values that are consistent (each default is a one-element array). However, there is no mechanism to validate their lengths at runtime.- Without additional checks, if an environment variable is misconfigured (i.e., set with arrays of different lengths), the
getRpInfofunction may attempt to access a non-existent index, leading to runtime errors.- Consider implementing a validation step—either during the environment parsing or within
getRpInfo—to ensure that all three arrays always have matching lengths.🔗 Analysis chain
Ensure consistent lengths for environment arrays.
ENV.EXPECTED_ORIGIN,ENV.RP_NAME, andENV.RP_IDshould have matching lengths. Consider adding a validation step ensuring each index is safe to use for all.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the arrays in env have the same length by searching their definitions: rg -A 5 'EXPECTED_ORIGIN|RP_NAME|RP_ID'Length of output: 3386
apps/web/app/api/passkey/generate-registration-options/route.ts (2)
15-22: 🛠️ Refactor suggestion
Differentiate error status codes.
Using status code 500 for all errors doesn't properly communicate the nature of the error to clients.
Consider differentiating status codes:
if (error instanceof InAppError) { - return NextResponse.json({ error: error.message }, { status: 500 }) + const status = error.code === ErrorCode.INVALID_INPUT ? 400 : 500 + return NextResponse.json({ error: error.message }, { status }) } return NextResponse.json( { error: 'Failed to generate registration options' }, { status: 500 }, )📝 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.if (error instanceof InAppError) { const status = error.code === ErrorCode.INVALID_INPUT ? 400 : 500 return NextResponse.json({ error: error.message }, { status }) } return NextResponse.json( { error: 'Failed to generate registration options' }, { status: 500 }, ) }
8-11:
⚠️ Potential issueAdd input validation for identifier and origin.
The current implementation lacks input validation for critical security parameters:
identifiershould be validated for format and length- Empty origin fallback could pose security risks in WebAuthn implementation
Consider adding validation:
const input = { + identifier: validateIdentifier(body.identifier), - origin: req.headers.get('origin') || body.origin || '', + origin: validateOrigin(req.headers.get('origin') || body.origin), } + function validateIdentifier(identifier: unknown): string { + if (!identifier || typeof identifier !== 'string' || identifier.length > 64) { + throw new InAppError(ErrorCode.INVALID_INPUT, 'Invalid identifier format') + } + return identifier + } + function validateOrigin(origin: string | null): string { + if (!origin || !origin.startsWith('https://')) { + throw new InAppError(ErrorCode.INVALID_INPUT, 'Invalid origin') + } + return origin + }Committable suggestion skipped: line range outside the PR's diff.
apps/web/app/api/passkey/generate-authentication-options/route.ts (1)
8-12: 🛠️ Refactor suggestion
⚠️ Potential issueAdd input validation and consider extracting common code.
Similar to the registration endpoint, input validation is missing. Additionally, there's code duplication between authentication and registration handlers.
- Add validation:
const input = { + identifier: validateIdentifier(body.identifier), + origin: validateOrigin(req.headers.get('origin') || body.origin), + challenge: validateChallenge(body.challenge), - identifier: body.identifier, - origin: req.headers.get('origin') || body.origin || '', - challenge: body.challenge, } + function validateChallenge(challenge: unknown): string { + if (!challenge || typeof challenge !== 'string' || !/^[A-Za-z0-9+/=]+$/.test(challenge)) { + throw new InAppError(ErrorCode.INVALID_INPUT, 'Invalid challenge format') + } + return challenge + }
- Consider creating a shared middleware for common validation logic between registration and authentication handlers.
📝 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.const input = { identifier: validateIdentifier(body.identifier), origin: validateOrigin(req.headers.get('origin') || body.origin), challenge: validateChallenge(body.challenge), } function validateChallenge(challenge: unknown): string { if (!challenge || typeof challenge !== 'string' || !/^[A-Za-z0-9+/=]+$/.test(challenge)) { throw new InAppError(ErrorCode.INVALID_INPUT, 'Invalid challenge format') } return challenge }apps/web/lib/passkey/errors.ts (2)
1-6: 🛠️ Refactor suggestion
Add error codes for input validation and rate limiting.
The current error codes don't cover common WebAuthn implementation needs.
export enum ErrorCode { NO_MATCHING_RP_ID = 'NO_MATCHING_RP_ID', CHALLENGE_NOT_FOUND = 'CHALLENGE_NOT_FOUND', AUTHENTICATOR_NOT_REGISTERED = 'AUTHENTICATOR_NOT_REGISTERED', UNEXPECTED_ERROR = 'UNEXPECTED_ERROR', + INVALID_INPUT = 'INVALID_INPUT', + RATE_LIMIT_EXCEEDED = 'RATE_LIMIT_EXCEEDED', + INVALID_CREDENTIAL = 'INVALID_CREDENTIAL', }📝 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.export enum ErrorCode { NO_MATCHING_RP_ID = 'NO_MATCHING_RP_ID', CHALLENGE_NOT_FOUND = 'CHALLENGE_NOT_FOUND', AUTHENTICATOR_NOT_REGISTERED = 'AUTHENTICATOR_NOT_REGISTERED', UNEXPECTED_ERROR = 'UNEXPECTED_ERROR', INVALID_INPUT = 'INVALID_INPUT', RATE_LIMIT_EXCEEDED = 'RATE_LIMIT_EXCEEDED', INVALID_CREDENTIAL = 'INVALID_CREDENTIAL', }
16-23: 🧹 Nitpick (assertive)
Add HTTP status code mapping to error codes.
To ensure consistent error responses across endpoints, add status code mapping.
export class InAppError extends Error { code: ErrorCode + status: number constructor(code: ErrorCode, message?: string) { super(message || ERROR_MESSAGES[code]) this.name = 'InAppError' this.code = code + this.status = this.getStatusCode() } + + private getStatusCode(): number { + switch (this.code) { + case ErrorCode.INVALID_INPUT: + return 400 + case ErrorCode.RATE_LIMIT_EXCEEDED: + return 429 + case ErrorCode.NO_MATCHING_RP_ID: + case ErrorCode.AUTHENTICATOR_NOT_REGISTERED: + return 403 + default: + return 500 + } + } }📝 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.export class InAppError extends Error { code: ErrorCode status: number constructor(code: ErrorCode, message?: string) { super(message || ERROR_MESSAGES[code]) this.name = 'InAppError' this.code = code this.status = this.getStatusCode() } private getStatusCode(): number { switch (this.code) { case ErrorCode.INVALID_INPUT: return 400 case ErrorCode.RATE_LIMIT_EXCEEDED: return 429 case ErrorCode.NO_MATCHING_RP_ID: case ErrorCode.AUTHENTICATOR_NOT_REGISTERED: return 403 default: return 500 } } }apps/web/app/api/passkey/verify-registration/route.ts (2)
19-19:
⚠️ Potential issueAvoid logging sensitive error details.
The current error logging could expose sensitive information.
- console.error('Error verifying registration', error) + console.error('Error verifying registration:', { + code: error instanceof InAppError ? error.code : 'UNEXPECTED_ERROR', + message: error instanceof InAppError ? error.message : 'An unexpected error occurred', + // Add request context but exclude sensitive data + context: { + hasIdentifier: !!body?.identifier, + hasOrigin: !!body?.origin, + hasResponse: !!body?.registrationResponse, + } + })📝 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.console.error('Error verifying registration:', { code: error instanceof InAppError ? error.code : 'UNEXPECTED_ERROR', message: error instanceof InAppError ? error.message : 'An unexpected error occurred', // Add request context but exclude sensitive data context: { hasIdentifier: !!body?.identifier, hasOrigin: !!body?.origin, hasResponse: !!body?.registrationResponse, } })
8-12:
⚠️ Potential issueAdd validation for registrationResponse.
The registration response should be validated before processing.
const input = { + identifier: validateIdentifier(body.identifier), + origin: validateOrigin(req.headers.get('origin') || body.origin), + registrationResponse: validateRegistrationResponse(body.registrationResponse), - identifier: body.identifier, - origin: req.headers.get('origin') || body.origin || '', - registrationResponse: body.registrationResponse, } + function validateRegistrationResponse(response: unknown): WebAuthnRegistrationResponse { + if (!response || typeof response !== 'object' || !response.clientDataJSON || !response.attestationObject) { + throw new InAppError(ErrorCode.INVALID_INPUT, 'Invalid registration response format') + } + return response as WebAuthnRegistrationResponse + }📝 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.const input = { identifier: validateIdentifier(body.identifier), origin: validateOrigin(req.headers.get('origin') || body.origin), registrationResponse: validateRegistrationResponse(body.registrationResponse), } function validateRegistrationResponse(response: unknown): WebAuthnRegistrationResponse { if ( !response || typeof response !== 'object' || !response.clientDataJSON || !response.attestationObject ) { throw new InAppError(ErrorCode.INVALID_INPUT, 'Invalid registration response format') } return response as WebAuthnRegistrationResponse }apps/web/app/api/passkey/verify-authentication/route.ts (2)
15-24:
⚠️ Potential issueImprove error handling.
The current implementation has several issues:
- Uses generic 500 status code for all errors
- Exposes error details in production
- Logs sensitive information to console
Apply this diff to improve error handling:
} catch (error) { - console.error('Error verifying authentication', error) + console.error('Error verifying authentication') if (error instanceof InAppError) { return NextResponse.json({ error: error.message }, { status: 500 }) } + if (error instanceof Error && error.message.includes('Missing')) { + return NextResponse.json({ error: 'Invalid request' }, { status: 400 }) + } return NextResponse.json( { error: 'Failed to verify authentication' }, { status: 500 }, ) }📝 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.} catch (error) { console.error('Error verifying authentication') if (error instanceof InAppError) { return NextResponse.json({ error: error.message }, { status: 500 }) } if (error instanceof Error && error.message.includes('Missing')) { return NextResponse.json({ error: 'Invalid request' }, { status: 400 }) } return NextResponse.json( { error: 'Failed to verify authentication' }, { status: 500 }, ) }
8-12:
⚠️ Potential issueAdd input validation for required fields.
The code should validate that all required fields (
identifier,authenticationResponse) are present in the request body before proceeding.Apply this diff to add input validation:
const body = await req.json() +if (!body.identifier || !body.authenticationResponse) { + return NextResponse.json( + { error: 'Missing required fields' }, + { status: 400 } + ) +} const input = { identifier: body.identifier, origin: req.headers.get('origin') || body.origin || '', authenticationResponse: body.authenticationResponse, }📝 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.const body = await req.json() if (!body.identifier || !body.authenticationResponse) { return NextResponse.json( { error: 'Missing required fields' }, { status: 400 } ) } const input = { identifier: body.identifier, origin: req.headers.get('origin') || body.origin || '', authenticationResponse: body.authenticationResponse, }apps/web/lib/passkey/env.ts (2)
65-70: 🛠️ Refactor suggestion
Improve error handling for environment validation.
The current implementation exits the process on validation failure, which might not be suitable for all environments. Consider implementing a more graceful failure mode.
Apply this diff to improve error handling:
} catch (err) { if (err instanceof z.ZodError) { - console.error(err.issues) + console.error('Invalid environment configuration') + if (process.env.NODE_ENV !== 'production') { + console.error(err.issues) + } } - process.exit(1) + throw new Error('Failed to load environment configuration') }📝 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.} catch (err) { if (err instanceof z.ZodError) { console.error('Invalid environment configuration') if (process.env.NODE_ENV !== 'production') { console.error(err.issues) } } throw new Error('Failed to load environment configuration') }
4-50: 💡 Codebase verification
Review default values in passkey/env.ts may be unsuitable for production.
The investigation indicates that while most production modules appropriately source environment variables (e.g. via process.env), the passkey configuration in
apps/web/lib/passkey/env.tsstill defines hardcoded defaults (for values likeREDIS_URL,RP_ID, etc.) that could inadvertently be used if the production environment does not override them.
- apps/web/lib/passkey/env.ts: Uses Zod to validate and transform values but falls back to defaults that seem better suited for local/testing environments rather than production.
- Other parts of the codebase: Production files (e.g. in
services/ai,apps/kyc-server, etc.) rely on explicit environment variables, confirming that production settings are expected to be set externally.Please ensure that for production deployments the required passkey environment variables are provided so that these defaults do not inadvertently take effect.
🔗 Analysis chain
Review default values for production readiness.
The schema includes hardcoded default values that might not be suitable for production:
- Empty Redis URL
- Default contract IDs and URLs pointing to test networks
- Short challenge TTL
Run this script to check if these defaults are used in production:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if production environment variables are properly set # Test: Verify that production doesn't rely on default values # Check environment variables in production deployment echo "Checking environment variables..." rg -l 'process\.env\.' | while read -r file; do echo "=== $file ===" rg 'process\.env\.' "$file" -A 2 -B 2 doneLength of output: 3984
apps/web/lib/passkey/redis.ts (2)
76-108: 🧹 Nitpick (assertive)
Consider data persistence and security improvements.
The TODO comment indicates this is a temporary solution. Additionally:
- User data is stored without encryption
- No key prefix for multi-tenant isolation
- Error handling returns null instead of throwing
Consider implementing:
- Data encryption for sensitive user data
- Key prefixing for multi-tenant support
- Proper error propagation
- Migration to a persistent database as mentioned in the TODO
Would you like me to create an issue to track these improvements?
10-19:
⚠️ Potential issueAdd Redis connection error handling and pooling.
The current Redis connection implementation lacks error handling and connection pooling.
Apply this diff to improve Redis connection handling:
+const REDIS_RECONNECT_ATTEMPTS = 10 +const REDIS_RECONNECT_DELAY = 1000 const getRedis = () => { if (!ENV.REDIS_URL) { console.log('Using Redis Mock') return new RedisMock() } console.log('Using Cloud Redis') - return new Redis(ENV.REDIS_URL) + const client = new Redis(ENV.REDIS_URL, { + retryStrategy: (times) => { + if (times > REDIS_RECONNECT_ATTEMPTS) { + return null + } + return REDIS_RECONNECT_DELAY + } + }) + client.on('error', (err) => { + console.error('Redis connection error:', err) + }) + return client } -const redis = getRedis() +const redis = getRedis() +process.on('SIGTERM', () => { + redis.disconnect() +})📝 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.const REDIS_RECONNECT_ATTEMPTS = 10 const REDIS_RECONNECT_DELAY = 1000 const getRedis = () => { if (!ENV.REDIS_URL) { console.log('Using Redis Mock') return new RedisMock() } console.log('Using Cloud Redis') const client = new Redis(ENV.REDIS_URL, { retryStrategy: (times) => { if (times > REDIS_RECONNECT_ATTEMPTS) { return null } return REDIS_RECONNECT_DELAY } }) client.on('error', (err) => { console.error('Redis connection error:', err) }) return client } const redis = getRedis() process.on('SIGTERM', () => { redis.disconnect() })apps/web/app/api/passkey/README.md (5)
1-2: 🧹 Nitpick (assertive)
Top-Level Heading Recommendation:
The file currently begins with "## Usage". According to Markdown lint MD041, the first line should be a top-level heading. Consider updating this line to use a single hash (e.g., "# Usage") to adhere to common markdown style guidelines.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
1-1: First line in a file should be a top-level heading
null(MD041, first-line-heading, first-line-h1)
3-11: 🧹 Nitpick (assertive)
Clear API Endpoint Definition for Generate Registration Options:
The "Generate Registration Options" endpoint is clearly documented with its HTTP method, URL, request body, and a brief description of the expected WebAuthn registration options response. For even greater clarity, consider adding a complete example JSON response in future documentation updates.
12-18: 🧹 Nitpick (assertive)
Detailed Documentation for Verify Registration Endpoint:
The POST endpoint for verifying registration is well described, including both the required request body structure and the expected verification result. Adding an example response here might further assist developers in integrating with the API.
26-32: 🧹 Nitpick (assertive)
Clear Presentation for Verify Authentication Endpoint:
Documentation for this endpoint correctly outlines the POST request details and the structure of the request body. Providing a sample response could be beneficial for developers when implementing client-side logic.
35-87: 🧹 Nitpick (assertive)
ASCII Flow Diagram Review:
The diagram effectively illustrates the sequential interactions between the user device and the API for both registration and authentication flows. To improve its readability and ensure consistent formatting across various markdown viewers, consider enclosing the diagram within a fenced code block (e.g., using triple backticks with a language hint likeplaintext).
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/api/passkey/README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
apps/web/app/api/passkey/README.md
1-1: First line in a file should be a top-level heading
null
(MD041, first-line-heading, first-line-h1)
🔇 Additional comments (4)
apps/web/app/api/passkey/README.md (4)
5-12: Generate Registration Options Documentation Clarity
The documentation for the "Generate Registration Options" endpoint is clear and provides all necessary details such as the HTTP method, endpoint URL, request body, and the expected response.
12-18: Verify Registration Endpoint Documentation
The description for the "Verify Registration" endpoint is well-detailed. The inclusion of both request payload and response information ensures clarity. Consider providing an explanation for the ellipsis ({...}) in theregistrationResponsefield elsewhere in the documentation if not already done.
19-25: Generate Authentication Options Documentation Clarity
The documentation for the "Generate Authentication Options" endpoint is consistent and clear, with a concise explanation of the expected request and response.
26-32: Verify Authentication Endpoint Documentation
The endpoint description for "Verify Authentication" is clearly outlined with the necessary details in the request body and the expected response format.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/web/app/api/passkey/README.md (2)
35-65: 🧹 Nitpick (assertive)Registration Flow Diagram – Enhance Accessibility
The ASCII diagram effectively visualizes the registration process. However, to improve accessibility for users (especially those using screen readers), consider adding a brief textual description or alt-text caption immediately below the diagram. This will help users who might find ASCII diagrams less accessible.
73-103: 🧹 Nitpick (assertive)Authentication Flow Diagram – Accessibility Improvement Suggested
The Authentication Flow diagram is well-structured and clear. To further enhance this section, consider adding a caption or brief textual summary below the diagram to describe the flow for those who might have difficulty interpreting ASCII art.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/web/app/api/passkey/README.md(1 hunks)
🔇 Additional comments (3)
apps/web/app/api/passkey/README.md (3)
1-3: Header and Section Titles are Well-Formatted
The use of a top-level header (# Usage) followed by structured section headings (e.g.,## API Endpoints) complies with markdownlint standards and improves document readability.
66-72: Registration Flow Summary is Concise and Clear
The summary provided after the registration flow diagram succinctly captures the sequence of actions. The steps are numbered and clearly explain the process, which aids comprehension.
104-109: Authentication Flow Summary is Well-Structured
The summary clearly outlines the steps of the authentication process, paralleling the registration flow summary well. This clarity aids users in understanding the complete authentication cycle.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/web/.env.sample(1 hunks)apps/web/app/api/passkey/README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
apps/web/app/api/passkey/README.md
11-11: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
15-15: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
17-17: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
29-29: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
33-33: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
35-35: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
47-47: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
51-51: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
53-53: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
65-65: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
69-69: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
71-71: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
🔇 Additional comments (9)
apps/web/.env.sample (1)
7-11: New Environment Variables for Passkey Authentication Added
The addition ofREDIS_URL,RP_ID,RP_NAME,EXPECTED_ORIGIN, andCHALLENGE_TTL_SECONDSis essential for configuring Passkey authentication. Verify that these default values are appropriately updated for your local and production environments.apps/web/app/api/passkey/README.md (8)
1-4: Documentation Header and Structure are Well-Defined
The document starts with a top-level# Usageheader and properly segments the API documentation. This establishes a clear organizational context for the reader.
5-22: Clear Documentation for Generating Registration Options
This section clearly explains the POST endpoint/api/passkey/generate-registration-optionsalong with its request body, success response, and error response. It might be beneficial to include a brief note on when the error"No matching RP ID found for the given origin"is triggered.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
11-11: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
15-15: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
17-17: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
23-40: Comprehensive Verify Registration Endpoint Documentation
The Verify Registration endpoint is well documented with its request structure and sample responses. Consider adding a short explanation of the underlying verification process or any prerequisites that must be met for successful verification.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
29-29: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
33-33: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
35-35: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
41-58: Generate Authentication Options Endpoint is Well-Documented
The POST endpoint/api/passkey/generate-authentication-optionsis clearly detailed with both the expected request body and the sample responses. The error response"Authenticator not registered"provides a clear example of potential failure modes.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
47-47: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
51-51: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
53-53: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
59-76: Clear Documentation of the Verify Authentication Endpoint
This section details the request and response for the Verify Authentication endpoint precisely. Ensure that the error conditions described here consistently reflect the API’s implementation.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
65-65: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
69-69: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
71-71: Fenced code blocks should be surrounded by blank lines
null(MD031, blanks-around-fences)
77-83: Accurate and Useful Listing of Common Error Messages
Listing common error messages in a dedicated section improves troubleshooting. Optionally, you might consider linking these errors to the specific endpoints where they may occur.
110-125: Authentication Flow Diagram Provides Clear Process Overview
The mermaid sequence diagram effectively visualizes the authentication process, detailing interactions among the client, verification API, passkey service, and Redis. It is important to ensure this diagram remains consistent with any future backend changes.
127-132: Concise Summary of Authentication Steps
The summary following the authentication diagram succinctly outlines the key steps in the authentication process. This clear enumeration reinforces the overall documentation.
espaciofuturoio
force-pushed
the
feat-passkey-backend
branch
from
f4322a2 to
fd7091e
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/web/app/api/passkey/README.md(1 hunks)apps/web/lib/passkey/stellar.ts(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
apps/web/app/api/passkey/README.md
11-11: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
15-15: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
17-17: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
29-29: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
33-33: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
35-35: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
47-47: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
51-51: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
53-53: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
65-65: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
69-69: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
71-71: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
🔇 Additional comments (2)
apps/web/app/api/passkey/README.md (2)
88-101: Add alt-text or a textual summary for the registration flow diagram.
This reiterates a past accessibility recommendation to help screen-reader users and those unfamiliar with ASCII/mermaid diagrams.
112-125: Add alt-text or a textual summary for the authentication flow diagram.
Again, repeating a past request for improved accessibility for diagrams.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
apps/web/lib/passkey/stellar.ts(1 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/web/lib/passkey/stellar.ts (1)
Learnt from: AndlerRL
PR: kindfi-org/kindfi#109
File: apps/web/lib/passkey/stellar.ts:52-54
Timestamp: 2025-02-02T10:35:42.632Z
Learning: WebAuthn credential public keys can have varying lengths depending on the authenticator, and should not be assumed to be 77 bytes. The length should be parsed from the COSE structure.
🔇 Additional comments (7)
apps/web/lib/passkey/stellar.ts (7)
52-54: Validate credential public key length instead of assuming 77 bytes.
Relying on a fixed length can cause errors for certain authenticators yielding differently sized credential public keys.
90-94: Check for missing public key components.
If-2or-3keys are missing inpublicKeyObject, calling.get('-2') as Buffercould throw an error. Validate these keys and handle potential undefined values.
107-112: Mismatch between declared curve (secp256k1) and the provided order (P-256).
The comment references secp256k1, but the specified hex corresponds to P-256 (secp256r1). This discrepancy may break signature operations.
117-117: Usethrow new Error(...)instead of throwing raw strings.
Throwing strings creates less informative error objects.
125-125: Usethrow new Error(...)instead of throwing raw strings.
Same reasoning as above.
146-146: Usethrow new Error(...)instead of throwing raw strings.
Same reasoning as above.
157-157: Usethrow new Error(...)instead of throwing raw strings.
Same reasoning as above.
There was a problem hiding this comment.
Hello @espaciofuturoio everything looks great, but the build is showing the following error:
Can you please take a look at it?
|
@Bran18 those errors are not related to my PR |
Already fix the issues, let me double check your branch and merge |
There was a problem hiding this comment.
Hi @espaciofuturoio,
Great job! Thank you so much for assisting us with these changes. We’re really pleased to have you on board and are very impressed by your performance as a new contributor in the Only Dust space and kindFi.
Let’s merge this code that closed issue #64
* feat(web): add passkey authentication # Users can authenticate with a simple biometric. * feat(web): add passkey authentication. close kindfi-org#64 * feat(wip): add passkey authentication * feat(wip): add passkey authentication * chore(docs): improve passkey documentation * chore(docs): improve passkey documentation * chore(docs): add api errors example * chore: restore (api-routes) folder * chore: use @ts-expect-error * chore: fix ts error --------- Co-authored-by: Ruben Abarca <[email protected]> Co-authored-by: Brandon Fernández <[email protected]>
Notes
Remenber to use a valid redis url or implement another DB.
Registration Flow
Authentication Flow
You should add some env vars to your CI. This is an example:
Summary by CodeRabbit
New Features
Documentation