feat(auth): passkey, TOTP and sudo mode #382
Conversation
This commit adds comprehensive passkey authentication and management features: - Added WebAuthn passkey registration and authentication endpoints - Created new Prisma model for storing passkey credentials - Added Redis-based challenge management for WebAuthn flows
Add sudo mode verification with enhanced security: - Introduce new authorization mechanism with sudo token expiration - Add endpoint and service methods for sudo mode verification - Support password and passkey-based sudo authentication
…ication Expand test coverage for authentication features: - Add unit tests for sudo mode verification with password and passkey - Implement E2E tests for sudo mode and passkey authentication endpoints - Mock WebAuthn server methods for consistent testing - Cover various authentication scenarios including edge cases
WalkthroughThis pull request introduces comprehensive support for passkey-based authentication and sudo mode. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as UsersController
participant S as UsersService
participant DB as Database
U->>C: Request Passkey Registration Options
C->>S: generatePasskeyRegistrationOptions(userId)
S->>DB: Retrieve user data
DB-->>S: Return user info
S->>C: Return registration options
C->>U: Send options
U->>C: Submit Registration Response
C->>S: verifyPasskeyRegistration(userId, response)
S->>DB: Save new passkey
DB-->>S: Confirm save
S->>C: Acknowledge registration
sequenceDiagram
participant U as User
participant C as Controller
participant S as AuthService/UsersService
U->>C: Request Sudo Mode Verification
C->>S: verifySudo(token, method, credentials)
S->>S: Check sudo mode (checkSudoMode) or issue new token
S-->>C: Return sudo token or error
C->>U: Provide sudo verification response
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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| session({ | ||
| store: redisStore, | ||
| secret: process.env.SESSION_SECRET ?? 'secret', | ||
| resave: false, | ||
| saveUninitialized: false, | ||
| cookie: { | ||
| httpOnly: true, | ||
| sameSite: 'strict', | ||
| }, | ||
| }), |
Check warning
Code scanning / CodeQL
Clear text transmission of sensitive cookie Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 14 days ago
To fix the problem, we need to ensure that the secure attribute is set on the session cookie. This can be done by adding the secure attribute to the cookie configuration within the session middleware setup. Additionally, we should conditionally set the secure attribute based on the environment to allow for development flexibility.
- Modify the session cookie configuration to include the
secureattribute. - Use the
IS_DEVvariable to conditionally set thesecureattribute totrueonly if the application is not in development mode.
| @@ -44,2 +44,3 @@ | ||
| sameSite: 'strict', | ||
| secure: !IS_DEV, | ||
| }, |
| session({ | ||
| secret: 'testSecret', | ||
| resave: false, | ||
| saveUninitialized: false, | ||
| }), |
Check warning
Code scanning / CodeQL
Clear text transmission of sensitive cookie Medium test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 14 days ago
To fix the problem, we need to ensure that the session cookie is only transmitted over an encrypted connection by setting the secure attribute to true. This can be done by modifying the session configuration to include the secure attribute. Additionally, we should ensure that the secure attribute is only set when the application is running in a production environment to avoid issues during local development or testing.
- Modify the session configuration in the
beforeAllhook to include thesecureattribute. - Use a condition to set the
secureattribute based on the environment.
| @@ -62,4 +62,4 @@ | ||
| saveUninitialized: false, | ||
| cookie: { secure: process.env.NODE_ENV === 'production' }, | ||
| }), | ||
| ); | ||
| await app.init(); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (15)
src/main.ts (2)
22-29: Consider adding error handling when connecting to Redis.
Ifredis.ping()fails, the application might crash or silently fail. Wrap the connection logic in a try-catch block or handle connection errors gracefully.try { + await redis.ping(); } catch (err) { + console.error('Redis connection failed:', err); + process.exit(1); }
31-34: Use 'const' instead of 'let' forredisStore.
redisStoreis never reassigned; usingconstinstead ofletcan improve clarity.- let redisStore = new RedisStore({ + const redisStore = new RedisStore({ client: redis, prefix: 'cheese-session:', });src/users/users.controller.ts (2)
541-557: Consider stronger type enforcement foroptions.
Currently, the return data is type-asserted toany. Provide a proper interface or rely on the type from@simplewebauthn/typesto avoid losing type-safety.return { code: 200, message: 'Generated registration options successfully.', data: { - options: options as any, + options, }, };
574-589: Session-based challenge handling is good, but verify session availability.
If sessions are disabled or misconfigured,req.sessionmay be undefined. Consider gracefully handling or logging session initialization failures.src/users/users.service.ts (2)
202-221:savePasskeyCredentialis straightforward.
Consider validating for existing credential collision if that scenario arises. Otherwise, fine for a first pass.
1101-1134:verifySudosupports both password and passkey.
Well done implementing multi-factor re-auth. Add explicit logging for successful and unsuccessful attempts as needed.test/user.e2e-spec.ts (1)
933-1032: Enable and expand Sudo Mode tests
Your Sudo Mode tests cover success and wrong-password failure, which is good. Consider enabling/commenting-in the test for passkey-based sudo verification (lines 968-989) and “sensitive operations require sudo” (lines 1006-1031) to achieve broader coverage.src/users/user-challenge.repository.ts (1)
1-40: Repository logic looks solid
This repository cleanly encapsulates Redis interactions. Consider adding defensive checks (e.g., handle unexpectedttlSecondsvalues) or dedicated error handling if Redis operations fail, to provide clearer feedback upstream.src/users/DTO/passkey.dto.ts (1)
17-19: Add validation decorators for request DTOs.Consider adding class-validator decorators to validate the request DTOs:
export class PasskeyRegistrationVerifyRequestDto { + @IsNotEmpty() + @ValidateNested() + @Type(() => RegistrationResponseJSON) response: RegistrationResponseJSON; } export class PasskeyAuthenticationVerifyRequestDto { + @IsNotEmpty() + @ValidateNested() + @Type(() => AuthenticationResponseJSON) response: AuthenticationResponseJSON; }Don't forget to import:
import { IsNotEmpty, ValidateNested } from 'class-validator'; import { Type } from 'class-transformer';Also applies to: 30-32
src/auth/definitions.ts (1)
72-76: Consider adding timestamp validation for sudoUntil.While the interface change is good, the
sudoUntiltimestamp should be validated to:
- Ensure it's a future timestamp
- Prevent potential timing attacks by validating against server time
Example validation in the auth service:
private validateSudoTimestamp(timestamp: number): boolean { const now = Date.now(); return timestamp > now && timestamp <= now + MAX_SUDO_DURATION; }src/auth/guard.decorator.ts (1)
92-96: Add JSDoc documentation for the Guard decorator.The new
requireSudoparameter needs documentation to explain:
- Its purpose and default behavior
- When to use it
- Security implications
Add this documentation:
/** * Protects a resource by requiring authentication and optional sudo mode. * @param action - The authorized action to perform * @param resourceType - The type of resource being accessed * @param requireSudo - If true, requires active sudo mode for access */src/auth/auth.service.ts (1)
233-238: Consider adding a minimum time buffer for sudo expiration checks.While the current implementation is correct, it might be vulnerable to clock skew issues.
Add a small buffer to the expiration check:
- return ( - authorization.sudoUntil !== undefined && authorization.sudoUntil > now - ); + const CLOCK_SKEW_BUFFER = 30000; // 30 seconds + return ( + authorization.sudoUntil !== undefined && + authorization.sudoUntil > (now - CLOCK_SKEW_BUFFER) + );src/users/users.spec.ts (1)
208-343: Consider adding tests for additional edge cases.While the current test coverage is good, consider adding tests for:
- Concurrent passkey operations
- Expired challenges
- Malformed passkey responses
Example test case for expired challenges:
it('should handle expired challenges', async () => { // Set up an expired challenge await usersService['userChallengeRepository'].setChallenge( 1, 'expired-challenge', -1 // Negative TTL to ensure expiration ); const fakeRegistrationResponse = { id: 'cred-id', rawId: 'raw-cred-id', response: {}, type: 'public-key', clientExtensionResults: {}, }; await expect( usersService.verifyPasskeyRegistration( 1, fakeRegistrationResponse as any ) ).rejects.toThrow('Challenge expired'); });src/users/users.prisma (1)
163-178: Add constraints to the Passkey model for better data integrity.Consider the following improvements:
- Make credentialId unique to prevent duplicate registrations
- Add maximum size for publicKey
- Use enum for deviceType
Apply these changes to improve the schema:
model Passkey { id Int @id(map: "PK_passkey") @default(autoincrement()) - credentialId String // 凭证ID + credentialId String @unique // 凭证ID - publicKey Bytes // 存储公钥（二进制数据） + publicKey Bytes @db.ByteA(max: 1024) // 存储公钥（二进制数据） - deviceType String // 'singleDevice' 或 'multiDevice' + deviceType PasskeyDeviceType // 'singleDevice' 或 'multiDevice' // ... rest of the fields } +enum PasskeyDeviceType { + singleDevice + multiDevice +}prisma/schema.prisma (1)
761-776: Introduce the new Passkey model.
The newPasskeymodel is well-defined with fields covering the credential details necessary for passkey authentication. Key points include:
- Primary Key and Identifiers: The
idfield is correctly set as an auto-increment primary key with a custom map"PK_passkey".- Credential Details: Fields like
credentialIdandpublicKeyare appropriately typed (withpublicKeyasBytesfor binary data) and include inline comments for clarity.- Counters and Device Information: The
counterfield supports verification counting, anddeviceType(with a comment specifying'singleDevice'or'multiDevice') captures the type of device.- Backup and Transports: The boolean
backedUpfield indicates backup status, and the optionaltransportsfield (as a string) is intended to store transport methods in a JSON array format.- Relation to User: The relation to the
Usermodel viauserIdand the correspondinguserfield is correctly established.- Timestamps and Indexing: Automated timestamping (
createdAtandupdatedAt) and the index onuserIdare appropriate for efficient querying.One suggestion: Consider using an enum for
deviceTypeto enforce valid values instead of a raw string. For example:+enum DeviceType { + singleDevice + multiDevice +} ... - deviceType String @map("device_type") // 'singleDevice' 或 'multiDevice' + deviceType DeviceType @map("device_type") // 'singleDevice' 或 'multiDevice'This would enhance type safety and clarity in the schema.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
design/API.ymlis excluded by!**/*.ymlpackage.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yamlsrc/auth/token-payload.schema.jsonis excluded by!**/*.json
📒 Files selected for processing (20)
prisma/schema.prisma(2 hunks)src/app.module.ts(3 hunks)src/auth/auth.error.ts(1 hunks)src/auth/auth.service.ts(3 hunks)src/auth/definitions.ts(1 hunks)src/auth/guard.decorator.ts(2 hunks)src/common/config/configuration.ts(1 hunks)src/main.ts(2 hunks)src/users/DTO/passkey.dto.ts(1 hunks)src/users/DTO/sudo.dto.ts(1 hunks)src/users/role-permission.service.ts(1 hunks)src/users/user-challenge.repository.ts(1 hunks)src/users/users-permission.service.ts(1 hunks)src/users/users.controller.ts(6 hunks)src/users/users.error.ts(1 hunks)src/users/users.module.ts(2 hunks)src/users/users.prisma(2 hunks)src/users/users.service.ts(7 hunks)src/users/users.spec.ts(2 hunks)test/user.e2e-spec.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/users/users-permission.service.ts
🧰 Additional context used
🪛 GitHub Check: CodeQL
src/main.ts
[warning] 37-46: Clear text transmission of sensitive cookie
Sensitive cookie sent without enforcing SSL encryption.
test/user.e2e-spec.ts
[warning] 59-63: Clear text transmission of sensitive cookie
Sensitive cookie sent without enforcing SSL encryption.
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: test (12, 8.0.0, 20.x, 8, 2)
- GitHub Check: test (12, 8.0.0, 20.x, 8, 1)
- GitHub Check: test (12, 8.0.0, 18.x, 8, 1)
- GitHub Check: test (12, 8.12.2, 20.x, 8, 1)
- GitHub Check: test (12, 8.12.2, 18.x, 8, 2)
- GitHub Check: test (12, 8.12.2, 18.x, 8, 1)
- GitHub Check: test (latest, 8.0.0, 20.x, 8, 2)
- GitHub Check: test (latest, 8.0.0, 18.x, 8, 2)
- GitHub Check: test (latest, 8.0.0, 18.x, 8, 1)
- GitHub Check: test (latest, 8.12.2, 20.x, 8, 2)
- GitHub Check: test (latest, 8.12.2, 20.x, 8, 1)
- GitHub Check: test (latest, 8.12.2, 18.x, 8, 2)
- GitHub Check: test (latest, 8.12.2, 18.x, 8, 1)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: build-and-push-image
- GitHub Check: test (latest, 8.12.2, 18.x, 8)
🔇 Additional comments (31)
src/main.ts (2)
2-4: No issues with the new imports.
These added imports for Redis-based session management look good and follow standard practices.
36-47: Secure cookies for production environments.
This was flagged previously (lines 37-46) for sending sensitive cookies in clear text. To address this, enable thesecureflag (and possiblysameSite: 'none') when in production.cookie: { httpOnly: true, sameSite: 'strict', + secure: !IS_DEV, },🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 37-46: Clear text transmission of sensitive cookie
Sensitive cookie sent without enforcing SSL encryption.src/users/users.controller.ts (6)
23-28: Imports and injection look good.
The addition ofRequestandResponseobjects, along withPrismaServiceinjection, is appropriate for passkey and session management.
559-571: Passkey registration flow appears correct.
This endpoint correctly delegates verification logic to the service layer and returns an appropriate response code.
591-650: Good integration of passkey verification and login flow.
Ensure you handle edge cases, such as missing or invalid passkey IDs, to avoid potential exceptions. Otherwise, this flow looks solid.
653-672: Passkey listing is straightforward.
Code is minimal and relies on the service for retrieval. No issues spotted.
674-686: Passkey deletion logic is clear.
Ensure that partial deletions (e.g., multiple passkeys) are accounted for, but otherwise this is fine.
688-710: Validatemethodin the request body.
Currently, only'password' | 'passkey'is assumed. Consider default or error handling if the client sends an invalid method.if (!['password', 'passkey'].includes(body.method)) { + throw new BadRequestException('Invalid sudo method'); }src/users/users.service.ts (9)
11-11: ConfigService injection is appropriate.
No concerns with obtaining configuration from environment variables for webauthn details.
35-39: Captured errors appear relevant, but watch for collisions.
Merging multiple error classes in one file is fine, but ensure they don’t overshadow each other when thrown. Review usage to confirm consistent error handling.
104-114: Config getters provide fallback defaults, which is good.
Using environment-based defaults forrpName,rpID, andoriginensures local dev functionality. No issues spotted.
128-165:generatePasskeyRegistrationOptionscovers standard WebAuthn flows.
Ensure no additional user attributes are required (e.g., displayName). The rest is well-structured.
167-200:verifyPasskeyRegistrationis logically sound.
Throwing errors if verification fails or if no challenge is found is correct. The code handles cleanup of the user challenge as well.
223-235:generatePasskeyAuthenticationOptionsis standard.
Storing challenge in session is typical. Good usage of@simplewebauthn/server.
237-290:verifyPasskeyAuthenticationhas robust checks.
You verify existence of challenge, retrieve authenticator from DB, and finally update the counter. Looks correct.
291-307:handlePasskeyLoginis consistent with existing login flow.
You record a login log and return a session. This is aligned with the password-based approach.
309-324:getUserPasskeysanddeletePasskeylogic is well-defined.
Double-check thatdeletePasskeyonly affects the given credential and user. So far, no issues.test/user.e2e-spec.ts (4)
11-11: Importing session at test level
Importingexpress-sessiondirectly in the e2e test is acceptable, though typically you'd mock or stub session behavior for integration tests. Nonetheless, this is workable for quickly verifying session-dependent flows.
16-39: Mocking @simplewebauthn/server
Good approach to isolate external WebAuthn logic. You might also test negative or error scenarios to ensure the mock properly simulates failures (e.g., throw an error inverifyRegistrationResponse).
58-63: Enforce secure session cookies
This portion of code sets up a session without SSL or secure flags, which may expose sensitive cookies in clear text over non-HTTPS connections. This mirrors the past report from GitHub Advanced Security.
[duplicate_comment, architecture_advice]
Consider applying a secure cookie policy:app.use( session({ secret: 'testSecret', resave: false, saveUninitialized: false, + cookie: { + secure: process.env.NODE_ENV === 'production', + httpOnly: true, + sameSite: 'strict' + } }), );🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 59-63: Clear text transmission of sensitive cookie
Sensitive cookie sent without enforcing SSL encryption.
1033-1144: Comprehensive passkey endpoint tests
These tests are well-structured, covering registration, verification, and deletion flows. As a future enhancement, consider adding negative scenarios (e.g., invalid passkey, tampered response) to solidify error-handling coverage.src/users/DTO/sudo.dto.ts (1)
1-7: Structured response DTO
ExtendingBaseResponseDtois a clean approach. If you plan to expand the sudo response in the future, ensure thedataproperty can accommodate additional fields or nested structures.src/users/users.module.ts (1)
10-10: LGTM! Module configuration aligns with passkey authentication requirements.The addition of
UserChallengeRepositoryto the providers array and the necessary imports are correctly configured, following NestJS best practices.Also applies to: 17-18, 40-40
src/users/DTO/passkey.dto.ts (1)
1-56: LGTM! Well-structured DTOs for passkey operations.The DTOs are logically organized into registration, authentication, and management categories, with proper type definitions and inheritance from
BaseResponseDto.src/common/config/configuration.ts (1)
33-37:✅ Verification successful
Ensure secure WebAuthn configuration in production.
The current default values are suitable for development but need attention for production:
rpIDshould be your actual domain (e.g., "example.com")originmust use HTTPS in production as WebAuthn requires secure contexts
🌐 Web query:
Does WebAuthn require HTTPS in production?💡 Result:
WebAuthn does require HTTPS in production environments for secure operation[1][2][5][7]. This requirement ensures end-to-end security by preventing man-in-the-middle attacks and verifying the Relying Party's authenticity through domain validation[1][5].
Key reasons for this mandate:
- Protocol Specification: The W3C WebAuthn standard explicitly states that requests must use HTTPS as part of the secure communication channel between components[1].
- Phishing Prevention: Passkeys bind to domains via Relying Party ID (rpID), requiring TLS encryption to validate domain ownership and prevent credential theft[5].
- Security Constraints:
- Challenge URLs must use HTTPS[2]
- Browser APIs block WebAuthn operations on non-HTTPS origins[7]
- Prevents cross-origin credential misuse[5]
Exceptions exist for:
localhostduring development[6]- Embedded/trusted apps using platform-specific association files (e.g., iOS/Android apps)[5]
Production deployments must configure:
- Valid TLS certificates
- Correct rpID matching the domain
- Proper Content Security Policies[2]
- Secure credential storage[3]
A GitHub issue demonstrates real-world consequences when these requirements aren't met - an "Unrecognized name" error occurred due to SNI misconfiguration in HTTPS requests[8].
Citations:
- 1: https://www.w3.org/TR/webauthn-2/
- 2: https://github.com/w3c/webauthn/wiki/Explainer:-WebAuthn-challengeURL
- 3: https://developers.yubico.com/WebAuthn/WebAuthn_Walk-Through.html
- 4: https://curity.io/resources/learn/webauthn-overview/
- 5: https://www.corbado.com/blog/webauthn-relying-party-id-rpid-passkeys
- 6: https://www.herbie.dev/blog/webauthn-basic-web-client-server/
- 7: https://css.csail.mit.edu/6.5660/2023/labs/lab5.html
- 8: WebAuthn works locally, but not in production MasterKale/SimpleWebAuthn#345
Ensure secure WebAuthn configuration in production.
The current WebAuthn config in
src/common/config/configuration.tsuses defaults that are acceptable for development but must be replaced when deploying to production:
- rpID: Default is set to
'localhost'. For production, this should match your actual domain (e.g.,"example.com").- origin: Default is
'http://localhost:7777'. In production, this must be updated to an HTTPS URL since WebAuthn operations require secure contexts.Make sure that the production environment provides these secure values through the respective environment variables to prevent security vulnerabilities.
src/auth/auth.error.ts (1)
78-96: LGTM! Well-structured error classes with appropriate status codes.The new error classes follow established patterns and use correct HTTP status codes:
- 403 for
SudoRequiredErrorindicates insufficient privileges- 401 for
InvalidCredentialsErrorindicates authentication failuresrc/users/role-permission.service.ts (1)
42-61: LGTM! Well-structured permission setup for passkey and sudo features.The permission structure is well-designed:
- Sudo verification is accessible to all users (undefined ownership)
- Passkey operations are properly restricted to the owner (ownedByUser: userId)
src/auth/auth.service.ts (2)
104-111: LGTM! Secure implementation of sudo mode verification.The audit method correctly verifies sudo mode when required, throwing appropriate errors.
Also applies to: 114-119
241-261: LGTM! Secure implementation of sudo token generation.The implementation includes several security best practices:
- Verifies existing token before issuing new one
- Uses random offset to prevent timing attacks
- Preserves original token validity
src/users/users.spec.ts (1)
108-206: LGTM! Comprehensive test coverage for sudo mode authentication.The tests cover all major sudo mode scenarios including successful verification with both password and passkey methods.
prisma/schema.prisma (1)
645-645: Add 'passkeys' field to the User model.
The addition of thepasskeysfield (of typePasskey[]) in theUsermodel properly establishes a one-to-many relationship with the new Passkey model, supporting the passkey authentication feature. This change integrates well with your PR objectives by enabling users to have multiple passkeys registered.
src/app.module.ts
Outdated
| RedisModule.forRoot({ | ||
| config: { | ||
| host: process.env.REDIS_HOST, | ||
| port: parseInt(process.env.REDIS_PORT || '6379'), | ||
| username: process.env.REDIS_USERNAME, | ||
| password: process.env.REDIS_PASSWORD, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance Redis configuration for security and reliability.
Consider the following improvements:
- Add TLS configuration for secure Redis connections
- Add connection retry and timeout settings
- Provide a default value for
REDIS_HOST
RedisModule.forRoot({
config: {
- host: process.env.REDIS_HOST,
+ host: process.env.REDIS_HOST || 'localhost',
port: parseInt(process.env.REDIS_PORT || '6379'),
username: process.env.REDIS_USERNAME,
password: process.env.REDIS_PASSWORD,
+ tls: process.env.REDIS_TLS === 'true' ? {} : undefined,
+ retryStrategy: (times) => Math.min(times * 50, 2000),
+ connectTimeout: 10000,
},
}),📝 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.
| RedisModule.forRoot({ | |
| config: { | |
| host: process.env.REDIS_HOST, | |
| port: parseInt(process.env.REDIS_PORT || '6379'), | |
| username: process.env.REDIS_USERNAME, | |
| password: process.env.REDIS_PASSWORD, | |
| }, | |
| }), | |
| RedisModule.forRoot({ | |
| config: { | |
| host: process.env.REDIS_HOST || 'localhost', | |
| port: parseInt(process.env.REDIS_PORT || '6379'), | |
| username: process.env.REDIS_USERNAME, | |
| password: process.env.REDIS_PASSWORD, | |
| tls: process.env.REDIS_TLS === 'true' ? {} : undefined, | |
| retryStrategy: (times) => Math.min(times * 50, 2000), | |
| connectTimeout: 10000, | |
| }, | |
| }), |
src/users/users.error.ts
Outdated
| export class ChallengeNotFoundError extends BaseError { | ||
| constructor() { | ||
| super('ChallengeNotFoundError', 'Challenge not found', 404); | ||
| } | ||
| } | ||
|
|
||
| export class PasskeyVerificationFailedError extends BaseError { | ||
| constructor() { | ||
| super('PasskeyVerificationFailedError', 'Passkey verification failed', 400); | ||
| } | ||
| } | ||
|
|
||
| export class PasskeyNotFoundError extends BaseError { | ||
| constructor(credentialId: string) { | ||
| super( | ||
| 'PasskeyNotFoundError', | ||
| `Passkey with credential ID '${credentialId}' not found.`, | ||
| 404, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Review credentialId exposure in error messages.
The error classes are well-structured, but exposing the full credentialId in error messages could leak sensitive information. Consider:
- Truncating or hashing the credentialId
- Using a generic message in production
Apply this diff to enhance security:
- `Passkey with credential ID '${credentialId}' not found.`,
+ `Passkey not found. ID: ${credentialId.substring(0, 8)}...`,📝 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 ChallengeNotFoundError extends BaseError { | |
| constructor() { | |
| super('ChallengeNotFoundError', 'Challenge not found', 404); | |
| } | |
| } | |
| export class PasskeyVerificationFailedError extends BaseError { | |
| constructor() { | |
| super('PasskeyVerificationFailedError', 'Passkey verification failed', 400); | |
| } | |
| } | |
| export class PasskeyNotFoundError extends BaseError { | |
| constructor(credentialId: string) { | |
| super( | |
| 'PasskeyNotFoundError', | |
| `Passkey with credential ID '${credentialId}' not found.`, | |
| 404, | |
| ); | |
| } | |
| } | |
| export class ChallengeNotFoundError extends BaseError { | |
| constructor() { | |
| super('ChallengeNotFoundError', 'Challenge not found', 404); | |
| } | |
| } | |
| export class PasskeyVerificationFailedError extends BaseError { | |
| constructor() { | |
| super('PasskeyVerificationFailedError', 'Passkey verification failed', 400); | |
| } | |
| } | |
| export class PasskeyNotFoundError extends BaseError { | |
| constructor(credentialId: string) { | |
| super( | |
| 'PasskeyNotFoundError', | |
| `Passkey not found. ID: ${credentialId.substring(0, 8)}...`, | |
| 404, | |
| ); | |
| } | |
| } |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #382 +/- ##
==========================================
- Coverage 91.49% 89.10% -2.39%
==========================================
Files 71 73 +2
Lines 2739 3102 +363
Branches 379 437 +58
==========================================
+ Hits 2506 2764 +258
- Misses 233 318 +85
- Partials 0 20 +20 ☔ View full report in Codecov by Sentry. |
Add comprehensive two-factor authentication features: - Implement TOTP (Time-based One-Time Password) authentication - Implement risk-based 2FA verification with configurable settings - Support backup codes for account recovery - Enhance login flow with 2FA verification
HuanCheng65
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
test/user.e2e-spec.ts (1)
58-64:⚠️ Potential issueEnhance session security configuration.
The current session configuration lacks essential security measures:
- Missing
secureflag for HTTPS-only cookies- Missing
sameSiteattribute to prevent CSRF- Missing cookie expiration
Apply this diff to enhance security:
app.use( session({ secret: 'testSecret', resave: false, saveUninitialized: false, + cookie: { + secure: true, // Enforce HTTPS + sameSite: 'strict', + maxAge: 24 * 60 * 60 * 1000, // 24 hours + httpOnly: true + } }), );🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 59-63: Clear text transmission of sensitive cookie
Sensitive cookie sent without enforcing SSL encryption.prisma/schema.prisma (1)
777-792: 🛠️ Refactor suggestionAdd unique constraint to Passkey.
The model should have a unique constraint on
credentialIdto prevent duplicate registrations of the same credential.Apply this diff:
model Passkey { id Int @id(map: "PK_passkey") @default(autoincrement()) - credentialId String @map("credential_id") // 凭证ID + credentialId String @unique @map("credential_id") // 凭证ID publicKey Bytes @map("public_key") // 存储公钥（二进制数据） counter Int // 验证计数器 deviceType String @map("device_type") // 'singleDevice' 或 'multiDevice' backedUp Boolean @map("backed_up") // 是否已备份 transports String? // 可选，存储传输方式（JSON 数组字符串） userId Int @map("user_id") // 关联用户ID user User @relation(fields: [userId], references: [id]) createdAt DateTime @default(now()) @map("created_at") @db.Timestamptz(6) updatedAt DateTime @default(now()) @updatedAt @map("updated_at") @db.Timestamptz(6) @@index([userId], map: "IDX_passkey_user_id") @@map("passkey") }
🧹 Nitpick comments (11)
src/users/totp.service.ts (4)
66-94: Bcrypt usage is appropriate but consider asynchronous calls.
bcrypt.hashSyncandbcrypt.compareSyncblock the event loop, potentially impacting performance under load. Consider introducing asynchronous alternatives (bcrypt.hash/bcrypt.compare) if performance becomes an issue.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 66-67: src/users/totp.service.ts#L66-L67
Added lines #L66 - L67 were not covered by tests
[warning] 70-72: src/users/totp.service.ts#L70-L72
Added lines #L70 - L72 were not covered by tests
[warning] 74-74: src/users/totp.service.ts#L74
Added line #L74 was not covered by tests
[warning] 79-79: src/users/totp.service.ts#L79
Added line #L79 was not covered by tests
[warning] 81-81: src/users/totp.service.ts#L81
Added line #L81 was not covered by tests
[warning] 87-88: src/users/totp.service.ts#L87-L88
Added lines #L87 - L88 were not covered by tests
[warning] 91-92: src/users/totp.service.ts#L91-L92
Added lines #L91 - L92 were not covered by tests
103-111: Backup code generation is correct.
Generating hex-based random strings is generally safe, though consider indicating length or format requirements in documentation.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 103-107: src/users/totp.service.ts#L103-L107
Added lines #L103 - L107 were not covered by tests
[warning] 109-109: src/users/totp.service.ts#L109
Added line #L109 was not covered by tests
225-265: Comprehensive 2FA verification with backup codes.
Logic to handle backup codes is robust. Consider logging or metering attempts to thwart brute force on backup codes.
267-290: Sudo token generation.
Be mindful that automatically grantingsudoUntilfor 15 minutes is significant. Ensure it’s short enough and that there’s a re-verification path for extremely sensitive operations.src/common/config/configuration.ts (2)
45-45: Translate the Chinese comment to English.The comment should be in English for consistency and better maintainability.
- window: parseInt(process.env.TOTP_WINDOW || '1', 10), // 验证窗口，默认前后1个时间窗口 + window: parseInt(process.env.TOTP_WINDOW || '1', 10), // Validation window, defaults to 1 window before and after
40-40: Review the TOTP encryption key fallback.The TOTP encryption key falls back to
APP_KEYwhich might not be secure enough for TOTP encryption. Consider:
- Requiring a dedicated encryption key for TOTP
- Using a stronger key derivation mechanism
src/users/users.controller.ts (2)
589-589: Remove type assertions by properly typing the WebAuthn options.The type assertions (
as any) are used as workarounds for type compatibility issues. Consider:
- Importing the correct types from
@simplewebauthn/server- Creating proper interfaces that match the expected types
- options: options as any, // Type assertion to fix compatibility issue + options: options,Also applies to: 621-621
723-745: Enhance sudo mode verification with rate limiting.The sudo mode verification endpoint could benefit from rate limiting to prevent brute force attacks.
Consider implementing rate limiting using Redis:
@Post('/auth/sudo') @Guard('verify-sudo', 'user') @UseGuards(ThrottlerGuard) @Throttle(5, 60) // 5 attempts per minute async verifySudoMode(...)src/users/users.service.ts (1)
740-778: Extract time windows to configuration.The hardcoded time windows in the risk assessment logic should be moved to the configuration for better maintainability.
+ private readonly knownDeviceWindow = 30 * 24 * 60 * 60 * 1000; // 30 days + private readonly sensitiveOperationWindow = 24 * 60 * 60 * 1000; // 24 hours private async shouldRequire2FA(...): Promise<boolean> { - createdAt: { gte: new Date(Date.now() - 30 * 24 * 60 * 60 * 1000) } + createdAt: { gte: new Date(Date.now() - this.knownDeviceWindow) } ... - createdAt: { gte: new Date(Date.now() - 24 * 60 * 60 * 1000) } + createdAt: { gte: new Date(Date.now() - this.sensitiveOperationWindow) }test/user.e2e-spec.ts (1)
16-39: Enhance WebAuthn mock implementation.The current mock implementation provides basic test coverage but could be enhanced to:
- Test error scenarios (e.g., verification failures)
- Validate input parameters
- Cover edge cases with different credential types
jest.mock('@simplewebauthn/server', () => ({ generateRegistrationOptions: jest.fn(() => Promise.resolve({ challenge: 'fake-challenge' }), ), verifyRegistrationResponse: jest.fn(() => Promise.resolve({ verified: true, registrationInfo: { credential: { id: 'cred-id', publicKey: 'fake-public-key', counter: 1 }, credentialBackedUp: false, credentialDeviceType: 'singleDevice', }, }), ), generateAuthenticationOptions: jest.fn(() => Promise.resolve({ challenge: 'fake-auth-challenge', allowCredentials: [] }), ), verifyAuthenticationResponse: jest.fn(() => Promise.resolve({ verified: true, authenticationInfo: { newCounter: 2 }, }), ), + // Add error scenarios + verifyRegistrationResponseError: jest.fn(() => + Promise.resolve({ + verified: false, + error: 'Invalid attestation', + }), + ), + verifyAuthenticationResponseError: jest.fn(() => + Promise.resolve({ + verified: false, + error: 'Invalid signature', + }), + ), + // Add different credential types + generateRegistrationOptionsWithPlatform: jest.fn(() => + Promise.resolve({ + challenge: 'fake-challenge', + authenticatorSelection: { + authenticatorAttachment: 'platform', + }, + }), + ), }));src/users/users.prisma (1)
167-177: Add unique constraint to UserBackupCode.While the model is well-structured, consider adding a unique constraint on
codeHashto prevent duplicate backup codes.Apply this diff:
model UserBackupCode { id Int @id @default(autoincrement()) userId Int @map("user_id") - codeHash String @map("code_hash") @db.VarChar(128) // 加盐哈希存储 + codeHash String @unique @map("code_hash") @db.VarChar(128) // 加盐哈希存储 used Boolean @default(false) createdAt DateTime @default(now()) @map("created_at") @db.Timestamptz(6) user User @relation(fields: [userId], references: [id]) @@index([userId], map: "IDX_user_backup_code_user_id") @@map("user_backup_code") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
.github/workflows/test-coverage.ymlis excluded by!**/*.yml.github/workflows/test.ymlis excluded by!**/*.ymldesign/API.ymlis excluded by!**/*.ymldocker-compose.ymlis excluded by!**/*.ymlpackage.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
📒 Files selected for processing (13)
prisma/schema.prisma(2 hunks)src/app.module.ts(3 hunks)src/common/config/configuration.ts(1 hunks)src/users/DTO/login.dto.ts(1 hunks)src/users/DTO/totp.dto.ts(1 hunks)src/users/role-permission.service.ts(1 hunks)src/users/totp.service.ts(1 hunks)src/users/users.controller.ts(7 hunks)src/users/users.error.ts(1 hunks)src/users/users.module.ts(2 hunks)src/users/users.prisma(2 hunks)src/users/users.service.ts(8 hunks)test/user.e2e-spec.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/users/users.module.ts
- src/app.module.ts
- src/users/role-permission.service.ts
🧰 Additional context used
🪛 GitHub Check: CodeQL
test/user.e2e-spec.ts
[warning] 59-63: Clear text transmission of sensitive cookie
Sensitive cookie sent without enforcing SSL encryption.
🪛 GitHub Check: codecov/patch
src/users/totp.service.ts
[warning] 51-53: src/users/totp.service.ts#L51-L53
Added lines #L51 - L53 were not covered by tests
[warning] 55-55: src/users/totp.service.ts#L55
Added line #L55 was not covered by tests
[warning] 60-60: src/users/totp.service.ts#L60
Added line #L60 was not covered by tests
[warning] 63-63: src/users/totp.service.ts#L63
Added line #L63 was not covered by tests
[warning] 66-67: src/users/totp.service.ts#L66-L67
Added lines #L66 - L67 were not covered by tests
[warning] 70-72: src/users/totp.service.ts#L70-L72
Added lines #L70 - L72 were not covered by tests
[warning] 74-74: src/users/totp.service.ts#L74
Added line #L74 was not covered by tests
[warning] 79-79: src/users/totp.service.ts#L79
Added line #L79 was not covered by tests
[warning] 81-81: src/users/totp.service.ts#L81
Added line #L81 was not covered by tests
[warning] 87-88: src/users/totp.service.ts#L87-L88
Added lines #L87 - L88 were not covered by tests
[warning] 91-92: src/users/totp.service.ts#L91-L92
Added lines #L91 - L92 were not covered by tests
[warning] 95-96: src/users/totp.service.ts#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 99-100: src/users/totp.service.ts#L99-L100
Added lines #L99 - L100 were not covered by tests
[warning] 103-107: src/users/totp.service.ts#L103-L107
Added lines #L103 - L107 were not covered by tests
[warning] 109-109: src/users/totp.service.ts#L109
Added line #L109 was not covered by tests
[warning] 115-115: src/users/totp.service.ts#L115
Added line #L115 was not covered by tests
[warning] 117-117: src/users/totp.service.ts#L117
Added line #L117 was not covered by tests
[warning] 122-122: src/users/totp.service.ts#L122
Added line #L122 was not covered by tests
[warning] 126-126: src/users/totp.service.ts#L126
Added line #L126 was not covered by tests
[warning] 131-131: src/users/totp.service.ts#L131
Added line #L131 was not covered by tests
[warning] 135-137: src/users/totp.service.ts#L135-L137
Added lines #L135 - L137 were not covered by tests
[warning] 141-141: src/users/totp.service.ts#L141
Added line #L141 was not covered by tests
[warning] 143-143: src/users/totp.service.ts#L143
Added line #L143 was not covered by tests
[warning] 154-154: src/users/totp.service.ts#L154
Added line #L154 was not covered by tests
[warning] 162-162: src/users/totp.service.ts#L162
Added line #L162 was not covered by tests
[warning] 165-165: src/users/totp.service.ts#L165
Added line #L165 was not covered by tests
[warning] 167-169: src/users/totp.service.ts#L167-L169
Added lines #L167 - L169 were not covered by tests
[warning] 173-173: src/users/totp.service.ts#L173
Added line #L173 was not covered by tests
🪛 GitHub Actions: Build and Test Dev Docker
prisma/schema.prisma
[error] 1-1: Process completed with exit code 2.
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: test (12, 8.0.0, 20.x, 8, 1)
- GitHub Check: test (12, 8.0.0, 18.x, 8, 2)
- GitHub Check: test (12, 8.0.0, 18.x, 8, 1)
- GitHub Check: test (12, 8.0.0, 20.x, 8, 2)
- GitHub Check: test (12, 8.0.0, 20.x, 8, 1)
- GitHub Check: test (12, 8.0.0, 18.x, 8, 2)
- GitHub Check: test (12, 8.0.0, 18.x, 8, 1)
- GitHub Check: test (12, 8.12.2, 20.x, 8, 2)
- GitHub Check: test (12, 8.12.2, 20.x, 8, 1)
- GitHub Check: test (12, 8.12.2, 18.x, 8, 2)
- GitHub Check: test (12, 8.12.2, 18.x, 8, 1)
- GitHub Check: test (latest, 8.0.0, 20.x, 8, 2)
- GitHub Check: test (latest, 8.0.0, 20.x, 8, 1)
- GitHub Check: test (latest, 8.0.0, 18.x, 8, 2)
- GitHub Check: test (latest, 8.0.0, 18.x, 8, 1)
- GitHub Check: test (latest, 8.12.2, 20.x, 8, 2)
- GitHub Check: test (latest, 8.12.2, 20.x, 8, 1)
- GitHub Check: test (latest, 8.12.2, 18.x, 8, 2)
- GitHub Check: test (latest, 8.12.2, 18.x, 8, 1)
🔇 Additional comments (14)
src/users/totp.service.ts (7)
1-14: Class structure and imports look good.
All necessary dependencies are imported, and the file is well-organized overall.
51-65: Add test coverage for the encryption/decryption logic.
Encryption routines (and especially the use of IV and auth tags) are critical. Consider adding thorough unit tests to validate correctness, error handling (e.g., corrupted data), and ensure coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 51-53: src/users/totp.service.ts#L51-L53
Added lines #L51 - L53 were not covered by tests
[warning] 55-55: src/users/totp.service.ts#L55
Added line #L55 was not covered by tests
[warning] 60-60: src/users/totp.service.ts#L60
Added line #L60 was not covered by tests
[warning] 63-63: src/users/totp.service.ts#L63
Added line #L63 was not covered by tests
95-102: TOTP secret generation is straightforward.
The usage ofauthenticator.generateSecret()is aligned with best practices. Consider adding test coverage to ensure secrets are generated consistently.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 95-96: src/users/totp.service.ts#L95-L96
Added lines #L95 - L96 were not covered by tests
[warning] 99-100: src/users/totp.service.ts#L99-L100
Added lines #L99 - L100 were not covered by tests
112-163: Validate concurrency in enable2FA.
When multiple processes or requests try to enable 2FA concurrently, ensure the final state is consistent (i.e., no race condition if two tokens are verified in parallel). Consider adding tests to check concurrency scenarios.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 115-115: src/users/totp.service.ts#L115
Added line #L115 was not covered by tests
[warning] 117-117: src/users/totp.service.ts#L117
Added line #L117 was not covered by tests
[warning] 122-122: src/users/totp.service.ts#L122
Added line #L122 was not covered by tests
[warning] 126-126: src/users/totp.service.ts#L126
Added line #L126 was not covered by tests
[warning] 131-131: src/users/totp.service.ts#L131
Added line #L131 was not covered by tests
[warning] 135-137: src/users/totp.service.ts#L135-L137
Added lines #L135 - L137 were not covered by tests
[warning] 141-141: src/users/totp.service.ts#L141
Added line #L141 was not covered by tests
[warning] 143-143: src/users/totp.service.ts#L143
Added line #L143 was not covered by tests
[warning] 154-154: src/users/totp.service.ts#L154
Added line #L154 was not covered by tests
[warning] 162-162: src/users/totp.service.ts#L162
Added line #L162 was not covered by tests
165-191: Backup code regeneration is handled correctly.
This function properly deletes old codes before creating new ones. Ensure that any relevant user notifications are sent so they know their old codes are invalid.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 165-165: src/users/totp.service.ts#L165
Added line #L165 was not covered by tests
[warning] 167-169: src/users/totp.service.ts#L167-L169
Added lines #L167 - L169 were not covered by tests
[warning] 173-173: src/users/totp.service.ts#L173
Added line #L173 was not covered by tests
193-219: disable2FA logic is solid, but confirm no leftover states.
The code setstotpSecretto null and disables 2FA. Ensure logs or events track this change in case of suspicious account activity.
221-224: Straightforward TOTP verification.
The usage ofauthenticator.verify({ token, secret })matches the otplib recommendations. Ensure test coverage for edge cases (time drift, invalid tokens).src/users/DTO/login.dto.ts (1)
15-19: Flexible login response structure is appropriate.
MakingaccessTokenanduseroptional allows more nuanced handling of partial login states (e.g., 2FA steps). Ensure front-end clients handle these optional fields gracefully.src/users/DTO/totp.dto.ts (1)
1-74: DTOs are well-structured and comprehensive.
Each class clearly defines request and response data for 2FA operations. Ensure usage of consistent naming conventions and thorough tests to confirm serialization/deserialization.src/users/users.error.ts (1)
166-215: LGTM! Well-structured error classes with appropriate error handling.The implementation:
- Uses appropriate HTTP status codes for each error type
- Provides clear and informative error messages
- Implements secure handling of sensitive data (e.g., truncating credentialId)
src/users/users.service.ts (1)
133-294: LGTM! Secure implementation of WebAuthn.The WebAuthn implementation:
- Properly validates challenges
- Securely stores credentials
- Implements correct verification flow
- Handles errors appropriately
src/users/users.prisma (1)
48-52: LGTM! Well-structured schema additions for 2FA support.The new fields are well-designed with:
- Appropriate relationships (
Passkey[],UserBackupCode[])- Proper column types (
VarChar(64)for TOTP secret)- Sensible defaults for boolean flags
prisma/schema.prisma (2)
646-650: LGTM! Well-structured schema additions for 2FA support.The new fields are well-designed with:
- Appropriate relationships (
Passkey[],UserBackupCode[])- Proper column types (
VarChar(64)for TOTP secret)- Sensible defaults for boolean flags
765-775: Add unique constraint to UserBackupCode.While the model is well-structured, consider adding a unique constraint on
codeHashto prevent duplicate backup codes.Apply this diff:
model UserBackupCode { id Int @id @default(autoincrement()) userId Int @map("user_id") - codeHash String @map("code_hash") @db.VarChar(128) // 加盐哈希存储 + codeHash String @unique @map("code_hash") @db.VarChar(128) // 加盐哈希存储 used Boolean @default(false) createdAt DateTime @default(now()) @map("created_at") @db.Timestamptz(6) user User @relation(fields: [userId], references: [id]) @@index([userId], map: "IDX_user_backup_code_user_id") @@map("user_backup_code") }
| const ENCRYPTION_KEY_SALT = 'cheese-totp-secret-'; | ||
| const ENCRYPTION_ALGORITHM = 'aes-256-gcm'; | ||
|
|
There was a problem hiding this comment.
Potential security concern in fallback encryption key usage.
You rely on a fallback encryption key ('your-fallback-key-at-least-32-chars-long') if none is provided. This can weaken security if left unmodified. Consider ensuring that a secure, randomly generated key is mandatory and validated at startup.
Also applies to: 30-49
| if (method === 'password') { | ||
| const user = await this.prismaService.user.findUnique({ | ||
| where: { id: userId }, | ||
| }); | ||
| verified = await bcrypt.compare( | ||
| credentials.password!, | ||
| user!.hashedPassword, | ||
| ); | ||
| } else if (method === 'passkey') { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use timing-safe comparison for password verification.
The password comparison in sudo mode verification should use a timing-safe comparison to prevent timing attacks.
- verified = await bcrypt.compare(credentials.password!, user!.hashedPassword);
+ verified = await this.authService.timingSafeEqual(
+ await bcrypt.hash(credentials.password!, user!.hashedPassword.slice(0, 29)),
+ user!.hashedPassword
+ );📝 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 (method === 'password') { | |
| const user = await this.prismaService.user.findUnique({ | |
| where: { id: userId }, | |
| }); | |
| verified = await bcrypt.compare( | |
| credentials.password!, | |
| user!.hashedPassword, | |
| ); | |
| } else if (method === 'passkey') { | |
| if (method === 'password') { | |
| const user = await this.prismaService.user.findUnique({ | |
| where: { id: userId }, | |
| }); | |
| verified = await this.authService.timingSafeEqual( | |
| await bcrypt.hash(credentials.password!, user!.hashedPassword.slice(0, 29)), | |
| user!.hashedPassword | |
| ); | |
| } else if (method === 'passkey') { |
| .expect(201); | ||
|
|
||
| validToken = loginRes.body.data.accessToken; | ||
| testUserId = loginRes.body.data.user.id; | ||
|
|
||
| const verifyRes = await request(app.getHttpServer()) | ||
| .post('/users/auth/sudo') | ||
| .set('Authorization', `Bearer ${validToken}`) | ||
| .send({ | ||
| method: 'password', | ||
| credentials: { | ||
| password: 'ABC^^^666', | ||
| }, | ||
| }) | ||
| .expect(201); | ||
| validToken = verifyRes.body.data.accessToken; | ||
| }); | ||
|
|
||
| it('POST /users/{userId}/passkeys/options should return registration options', async () => { | ||
| const res = await request(app.getHttpServer()) | ||
| .post(`/users/${testUserId}/passkeys/options`) | ||
| .set('Authorization', `Bearer ${validToken}`) | ||
| .send() | ||
| .expect(201); | ||
| expect(res.body.data.options).toBeDefined(); | ||
| }); | ||
|
|
||
| it('POST /users/{userId}/passkeys should register a new passkey', async () => { | ||
| // 先获取注册选项 | ||
| const resOptions = await request(app.getHttpServer()) | ||
| .post(`/users/${testUserId}/passkeys/options`) | ||
| .set('Authorization', `Bearer ${validToken}`) | ||
| .send() | ||
| .expect(201); | ||
|
|
||
| const fakeRegistrationResponse = { | ||
| id: 'cred-id', | ||
| rawId: 'raw-cred-id', | ||
| response: {}, | ||
| type: 'public-key', | ||
| clientExtensionResults: {}, | ||
| }; | ||
|
|
||
| const res = await request(app.getHttpServer()) | ||
| .post(`/users/${testUserId}/passkeys`) | ||
| .set('Authorization', `Bearer ${validToken}`) | ||
| .send({ response: fakeRegistrationResponse }) | ||
| .expect(201); | ||
| expect(res.body.message).toBe('Passkey registered successfully.'); | ||
| }); | ||
|
|
||
| it('POST /users/auth/passkey/options should return authentication options', async () => { | ||
| const res = await request(app.getHttpServer()) | ||
| .post('/users/auth/passkey/options') | ||
| .send() | ||
| .expect(201); | ||
| expect(res.body.data.options).toBeDefined(); | ||
| }); | ||
|
|
||
| it('POST /users/auth/passkey/verify should verify passkey login and return tokens', async () => { | ||
| // 先获取认证选项 | ||
| const resOptions = await request(app.getHttpServer()) | ||
| .post('/users/auth/passkey/options') | ||
| .send() | ||
| .expect(201); | ||
| const cookies = resOptions.headers['set-cookie']; | ||
|
|
||
| const fakeAuthResponse = { | ||
| id: 'cred-id', | ||
| rawId: 'raw-cred-id', | ||
| response: {}, | ||
| type: 'public-key', | ||
| clientExtensionResults: {}, | ||
| }; | ||
|
|
||
| const res = await request(app.getHttpServer()) | ||
| .post('/users/auth/passkey/verify') | ||
| .set('Cookie', cookies) | ||
| .send({ response: fakeAuthResponse }) | ||
| .expect(201); | ||
| expect(res.body.data.user).toBeDefined(); | ||
| expect(res.body.data.accessToken).toBeDefined(); | ||
| }); | ||
|
|
||
| it('GET /users/{userId}/passkeys should return user passkeys list', async () => { | ||
| const res = await request(app.getHttpServer()) | ||
| .get(`/users/${testUserId}/passkeys`) | ||
| .set('Authorization', `Bearer ${validToken}`) | ||
| .expect(200); | ||
| expect(Array.isArray(res.body.data.passkeys)).toBe(true); | ||
| }); | ||
|
|
||
| it('DELETE /users/{userId}/passkeys/{credentialId} should delete passkey', async () => { | ||
| const res = await request(app.getHttpServer()) | ||
| .delete(`/users/${testUserId}/passkeys/cred-id`) | ||
| .set('Authorization', `Bearer ${validToken}`) | ||
| .expect(200); | ||
| expect(res.body.message).toBe('Delete passkey successfully.'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance Passkey Authentication test coverage.
The test suite should be expanded to include:
- Error cases for invalid credentials
- Validation of passkey properties
- Tests for concurrent passkey operations
Apply this diff to add missing test cases:
describe('Passkey Authentication Endpoints', () => {
// ... existing tests ...
+ it('should reject registration with invalid registration response', async () => {
+ const resOptions = await request(app.getHttpServer())
+ .post(`/users/${testUserId}/passkeys/options`)
+ .set('Authorization', `Bearer ${validToken}`)
+ .send()
+ .expect(201);
+
+ const invalidResponse = {
+ id: '',
+ rawId: '',
+ response: {},
+ type: 'invalid-type',
+ clientExtensionResults: {},
+ };
+
+ await request(app.getHttpServer())
+ .post(`/users/${testUserId}/passkeys`)
+ .set('Authorization', `Bearer ${validToken}`)
+ .send({ response: invalidResponse })
+ .expect(400);
+ });
+
+ it('should validate passkey properties', async () => {
+ const res = await request(app.getHttpServer())
+ .get(`/users/${testUserId}/passkeys`)
+ .set('Authorization', `Bearer ${validToken}`)
+ .expect(200);
+
+ const passkey = res.body.data.passkeys[0];
+ expect(passkey).toHaveProperty('credentialId');
+ expect(passkey).toHaveProperty('deviceType');
+ expect(passkey.deviceType).toMatch(/^(singleDevice|multiDevice)$/);
+ expect(passkey).toHaveProperty('backedUp');
+ expect(typeof passkey.backedUp).toBe('boolean');
+ });
+
+ it('should handle concurrent passkey operations correctly', async () => {
+ const fakeRegistrationResponse = {
+ id: 'cred-id-concurrent',
+ rawId: 'raw-cred-id-concurrent',
+ response: {},
+ type: 'public-key',
+ clientExtensionResults: {},
+ };
+
+ // Attempt concurrent registrations
+ await Promise.all([
+ request(app.getHttpServer())
+ .post(`/users/${testUserId}/passkeys`)
+ .set('Authorization', `Bearer ${validToken}`)
+ .send({ response: fakeRegistrationResponse }),
+ request(app.getHttpServer())
+ .post(`/users/${testUserId}/passkeys`)
+ .set('Authorization', `Bearer ${validToken}`)
+ .send({ response: fakeRegistrationResponse }),
+ ]);
+
+ // Verify only one passkey was registered
+ const res = await request(app.getHttpServer())
+ .get(`/users/${testUserId}/passkeys`)
+ .set('Authorization', `Bearer ${validToken}`)
+ .expect(200);
+
+ const matchingPasskeys = res.body.data.passkeys.filter(
+ (p) => p.credentialId === 'cred-id-concurrent'
+ );
+ expect(matchingPasskeys.length).toBe(1);
+ });
});📝 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.
| describe('Passkey Authentication Endpoints', () => { | |
| let testUserId: number; | |
| let validToken: string; | |
| beforeAll(async () => { | |
| // 使用已注册的测试账号登录 | |
| const loginRes = await request(app.getHttpServer()) | |
| .post('/users/auth/login') | |
| .send({ | |
| username: TestUsername, | |
| password: 'ABC^^^666', | |
| }) | |
| .expect(201); | |
| validToken = loginRes.body.data.accessToken; | |
| testUserId = loginRes.body.data.user.id; | |
| const verifyRes = await request(app.getHttpServer()) | |
| .post('/users/auth/sudo') | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send({ | |
| method: 'password', | |
| credentials: { | |
| password: 'ABC^^^666', | |
| }, | |
| }) | |
| .expect(201); | |
| validToken = verifyRes.body.data.accessToken; | |
| }); | |
| it('POST /users/{userId}/passkeys/options should return registration options', async () => { | |
| const res = await request(app.getHttpServer()) | |
| .post(`/users/${testUserId}/passkeys/options`) | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send() | |
| .expect(201); | |
| expect(res.body.data.options).toBeDefined(); | |
| }); | |
| it('POST /users/{userId}/passkeys should register a new passkey', async () => { | |
| // 先获取注册选项 | |
| const resOptions = await request(app.getHttpServer()) | |
| .post(`/users/${testUserId}/passkeys/options`) | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send() | |
| .expect(201); | |
| const fakeRegistrationResponse = { | |
| id: 'cred-id', | |
| rawId: 'raw-cred-id', | |
| response: {}, | |
| type: 'public-key', | |
| clientExtensionResults: {}, | |
| }; | |
| const res = await request(app.getHttpServer()) | |
| .post(`/users/${testUserId}/passkeys`) | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send({ response: fakeRegistrationResponse }) | |
| .expect(201); | |
| expect(res.body.message).toBe('Passkey registered successfully.'); | |
| }); | |
| it('POST /users/auth/passkey/options should return authentication options', async () => { | |
| const res = await request(app.getHttpServer()) | |
| .post('/users/auth/passkey/options') | |
| .send() | |
| .expect(201); | |
| expect(res.body.data.options).toBeDefined(); | |
| }); | |
| it('POST /users/auth/passkey/verify should verify passkey login and return tokens', async () => { | |
| // 先获取认证选项 | |
| const resOptions = await request(app.getHttpServer()) | |
| .post('/users/auth/passkey/options') | |
| .send() | |
| .expect(201); | |
| const cookies = resOptions.headers['set-cookie']; | |
| const fakeAuthResponse = { | |
| id: 'cred-id', | |
| rawId: 'raw-cred-id', | |
| response: {}, | |
| type: 'public-key', | |
| clientExtensionResults: {}, | |
| }; | |
| const res = await request(app.getHttpServer()) | |
| .post('/users/auth/passkey/verify') | |
| .set('Cookie', cookies) | |
| .send({ response: fakeAuthResponse }) | |
| .expect(201); | |
| expect(res.body.data.user).toBeDefined(); | |
| expect(res.body.data.accessToken).toBeDefined(); | |
| }); | |
| it('GET /users/{userId}/passkeys should return user passkeys list', async () => { | |
| const res = await request(app.getHttpServer()) | |
| .get(`/users/${testUserId}/passkeys`) | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .expect(200); | |
| expect(Array.isArray(res.body.data.passkeys)).toBe(true); | |
| }); | |
| it('DELETE /users/{userId}/passkeys/{credentialId} should delete passkey', async () => { | |
| const res = await request(app.getHttpServer()) | |
| .delete(`/users/${testUserId}/passkeys/cred-id`) | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .expect(200); | |
| expect(res.body.message).toBe('Delete passkey successfully.'); | |
| }); | |
| }); | |
| describe('Passkey Authentication Endpoints', () => { | |
| let testUserId: number; | |
| let validToken: string; | |
| beforeAll(async () => { | |
| // 使用已注册的测试账号登录 | |
| const loginRes = await request(app.getHttpServer()) | |
| .post('/users/auth/login') | |
| .send({ | |
| username: TestUsername, | |
| password: 'ABC^^^666', | |
| }) | |
| .expect(201); | |
| validToken = loginRes.body.data.accessToken; | |
| testUserId = loginRes.body.data.user.id; | |
| const verifyRes = await request(app.getHttpServer()) | |
| .post('/users/auth/sudo') | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send({ | |
| method: 'password', | |
| credentials: { | |
| password: 'ABC^^^666', | |
| }, | |
| }) | |
| .expect(201); | |
| validToken = verifyRes.body.data.accessToken; | |
| }); | |
| it('POST /users/{userId}/passkeys/options should return registration options', async () => { | |
| const res = await request(app.getHttpServer()) | |
| .post(`/users/${testUserId}/passkeys/options`) | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send() | |
| .expect(201); | |
| expect(res.body.data.options).toBeDefined(); | |
| }); | |
| it('POST /users/{userId}/passkeys should register a new passkey', async () => { | |
| // 先获取注册选项 | |
| const resOptions = await request(app.getHttpServer()) | |
| .post(`/users/${testUserId}/passkeys/options`) | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send() | |
| .expect(201); | |
| const fakeRegistrationResponse = { | |
| id: 'cred-id', | |
| rawId: 'raw-cred-id', | |
| response: {}, | |
| type: 'public-key', | |
| clientExtensionResults: {}, | |
| }; | |
| const res = await request(app.getHttpServer()) | |
| .post(`/users/${testUserId}/passkeys`) | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send({ response: fakeRegistrationResponse }) | |
| .expect(201); | |
| expect(res.body.message).toBe('Passkey registered successfully.'); | |
| }); | |
| it('POST /users/auth/passkey/options should return authentication options', async () => { | |
| const res = await request(app.getHttpServer()) | |
| .post('/users/auth/passkey/options') | |
| .send() | |
| .expect(201); | |
| expect(res.body.data.options).toBeDefined(); | |
| }); | |
| it('POST /users/auth/passkey/verify should verify passkey login and return tokens', async () => { | |
| // 先获取认证选项 | |
| const resOptions = await request(app.getHttpServer()) | |
| .post('/users/auth/passkey/options') | |
| .send() | |
| .expect(201); | |
| const cookies = resOptions.headers['set-cookie']; | |
| const fakeAuthResponse = { | |
| id: 'cred-id', | |
| rawId: 'raw-cred-id', | |
| response: {}, | |
| type: 'public-key', | |
| clientExtensionResults: {}, | |
| }; | |
| const res = await request(app.getHttpServer()) | |
| .post('/users/auth/passkey/verify') | |
| .set('Cookie', cookies) | |
| .send({ response: fakeAuthResponse }) | |
| .expect(201); | |
| expect(res.body.data.user).toBeDefined(); | |
| expect(res.body.data.accessToken).toBeDefined(); | |
| }); | |
| it('GET /users/{userId}/passkeys should return user passkeys list', async () => { | |
| const res = await request(app.getHttpServer()) | |
| .get(`/users/${testUserId}/passkeys`) | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .expect(200); | |
| expect(Array.isArray(res.body.data.passkeys)).toBe(true); | |
| }); | |
| it('DELETE /users/{userId}/passkeys/{credentialId} should delete passkey', async () => { | |
| const res = await request(app.getHttpServer()) | |
| .delete(`/users/${testUserId}/passkeys/cred-id`) | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .expect(200); | |
| expect(res.body.message).toBe('Delete passkey successfully.'); | |
| }); | |
| it('should reject registration with invalid registration response', async () => { | |
| const resOptions = await request(app.getHttpServer()) | |
| .post(`/users/${testUserId}/passkeys/options`) | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send() | |
| .expect(201); | |
| const invalidResponse = { | |
| id: '', | |
| rawId: '', | |
| response: {}, | |
| type: 'invalid-type', | |
| clientExtensionResults: {}, | |
| }; | |
| await request(app.getHttpServer()) | |
| .post(`/users/${testUserId}/passkeys`) | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send({ response: invalidResponse }) | |
| .expect(400); | |
| }); | |
| it('should validate passkey properties', async () => { | |
| const res = await request(app.getHttpServer()) | |
| .get(`/users/${testUserId}/passkeys`) | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .expect(200); | |
| const passkey = res.body.data.passkeys[0]; | |
| expect(passkey).toHaveProperty('credentialId'); | |
| expect(passkey).toHaveProperty('deviceType'); | |
| expect(passkey.deviceType).toMatch(/^(singleDevice|multiDevice)$/); | |
| expect(passkey).toHaveProperty('backedUp'); | |
| expect(typeof passkey.backedUp).toBe('boolean'); | |
| }); | |
| it('should handle concurrent passkey operations correctly', async () => { | |
| const fakeRegistrationResponse = { | |
| id: 'cred-id-concurrent', | |
| rawId: 'raw-cred-id-concurrent', | |
| response: {}, | |
| type: 'public-key', | |
| clientExtensionResults: {}, | |
| }; | |
| // Attempt concurrent registrations | |
| await Promise.all([ | |
| request(app.getHttpServer()) | |
| .post(`/users/${testUserId}/passkeys`) | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send({ response: fakeRegistrationResponse }), | |
| request(app.getHttpServer()) | |
| .post(`/users/${testUserId}/passkeys`) | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send({ response: fakeRegistrationResponse }), | |
| ]); | |
| // Verify only one passkey was registered | |
| const res = await request(app.getHttpServer()) | |
| .get(`/users/${testUserId}/passkeys`) | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .expect(200); | |
| const matchingPasskeys = res.body.data.passkeys.filter( | |
| (p) => p.credentialId === 'cred-id-concurrent' | |
| ); | |
| expect(matchingPasskeys.length).toBe(1); | |
| }); | |
| }); |
| describe('Sudo Mode Authentication', () => { | ||
| let testUserId: number; | ||
| let validToken: string; | ||
|
|
||
| beforeAll(async () => { | ||
| // 使用已注册的测试账号登录 | ||
| const loginRes = await request(app.getHttpServer()) | ||
| .post('/users/auth/login') | ||
| .send({ | ||
| username: TestUsername, | ||
| password: 'ABC^^^666', | ||
| }) | ||
| .expect(201); | ||
|
|
||
| validToken = loginRes.body.data.accessToken; | ||
| testUserId = loginRes.body.data.user.id; | ||
| }); | ||
|
|
||
| it('should verify sudo mode with password', async () => { | ||
| const res = await request(app.getHttpServer()) | ||
| .post('/users/auth/sudo') | ||
| .set('Authorization', `Bearer ${validToken}`) | ||
| .send({ | ||
| method: 'password', | ||
| credentials: { | ||
| password: 'ABC^^^666', | ||
| }, | ||
| }) | ||
| .expect(201); | ||
|
|
||
| expect(res.body.data.accessToken).toBeDefined(); | ||
| // 保存sudo token用于后续测试 | ||
| validToken = res.body.data.accessToken; | ||
| }); | ||
|
|
||
| // it('should verify sudo mode with passkey', async () => { | ||
| // const fakeAuthResponse = { | ||
| // id: 'cred-id', | ||
| // rawId: 'raw-cred-id', | ||
| // response: {}, | ||
| // type: 'public-key', | ||
| // clientExtensionResults: {}, | ||
| // }; | ||
|
|
||
| // const res = await request(app.getHttpServer()) | ||
| // .post('/users/auth/sudo') | ||
| // .set('Authorization', `Bearer ${validToken}`) | ||
| // .send({ | ||
| // method: 'passkey', | ||
| // credentials: { | ||
| // passkeyResponse: fakeAuthResponse, | ||
| // }, | ||
| // }) | ||
| // .expect(200); | ||
|
|
||
| // expect(res.body.data.accessToken).toBeDefined(); | ||
| // }); | ||
|
|
||
| it('should reject sudo verification with wrong password', async () => { | ||
| const res = await request(app.getHttpServer()) | ||
| .post('/users/auth/sudo') | ||
| .set('Authorization', `Bearer ${validToken}`) | ||
| .send({ | ||
| method: 'password', | ||
| credentials: { | ||
| password: 'wrong-password', | ||
| }, | ||
| }) | ||
| .expect(401); | ||
|
|
||
| expect(res.body.message).toMatch(/InvalidCredentialsError/); | ||
| }); | ||
|
|
||
| // 测试需要 sudo 权限的操作 | ||
| // it('should require sudo mode for sensitive operations', async () => { | ||
| // // 使用非 sudo token | ||
| // const loginRes = await request(app.getHttpServer()) | ||
| // .post('/users/auth/login') | ||
| // .send({ | ||
| // username: TestUsername, | ||
| // password: 'ABC^^^666', | ||
| // }) | ||
| // .expect(201); | ||
|
|
||
| // const normalToken = loginRes.body.data.accessToken; | ||
|
|
||
| // // 尝试执行需要 sudo 的操作（例如修改密码） | ||
| // const res = await request(app.getHttpServer()) | ||
| // .post(`/users/${testUserId}/password`) | ||
| // .set('Authorization', `Bearer ${normalToken}`) | ||
| // .send({ | ||
| // oldPassword: 'ABC^^^666', | ||
| // newPassword: 'NewABC^^^666', | ||
| // }) | ||
| // .expect(403); | ||
|
|
||
| // expect(res.body.message).toMatch(/SudoRequiredError/); | ||
| // }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand Sudo Mode test coverage.
The test suite should be enhanced to cover:
- Sudo token expiration
- Uncomment and implement passkey verification tests
- Test sudo mode with invalid passkey credentials
Apply this diff to add missing test cases:
describe('Sudo Mode Authentication', () => {
// ... existing tests ...
+
+ it('should expire sudo token after timeout', async () => {
+ jest.useFakeTimers();
+ // Get sudo token
+ const res = await request(app.getHttpServer())
+ .post('/users/auth/sudo')
+ .set('Authorization', `Bearer ${validToken}`)
+ .send({
+ method: 'password',
+ credentials: {
+ password: 'ABC^^^666',
+ },
+ })
+ .expect(201);
+
+ const sudoToken = res.body.data.accessToken;
+
+ // Advance time by 20 minutes (max sudo duration)
+ jest.advanceTimersByTime(20 * 60 * 1000);
+
+ // Try using expired sudo token
+ await request(app.getHttpServer())
+ .post(`/users/${testUserId}/password`)
+ .set('Authorization', `Bearer ${sudoToken}`)
+ .send({
+ oldPassword: 'ABC^^^666',
+ newPassword: 'NewABC^^^666',
+ })
+ .expect(401);
+
+ jest.useRealTimers();
+ });
+
+ // Uncomment and implement passkey verification tests
- // it('should verify sudo mode with passkey', async () => {
+ it('should verify sudo mode with passkey', async () => {
const fakeAuthResponse = {
id: 'cred-id',
rawId: 'raw-cred-id',
response: {},
type: 'public-key',
clientExtensionResults: {},
};
const res = await request(app.getHttpServer())
.post('/users/auth/sudo')
.set('Authorization', `Bearer ${validToken}`)
.send({
method: 'passkey',
credentials: {
passkeyResponse: fakeAuthResponse,
},
})
.expect(201);
expect(res.body.data.accessToken).toBeDefined();
- });
+ });
+
+ it('should reject sudo verification with invalid passkey', async () => {
+ const fakeAuthResponse = {
+ id: 'invalid-cred-id',
+ rawId: 'invalid-raw-cred-id',
+ response: {},
+ type: 'public-key',
+ clientExtensionResults: {},
+ };
+
+ await request(app.getHttpServer())
+ .post('/users/auth/sudo')
+ .set('Authorization', `Bearer ${validToken}`)
+ .send({
+ method: 'passkey',
+ credentials: {
+ passkeyResponse: fakeAuthResponse,
+ },
+ })
+ .expect(401);
+ });
});📝 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.
| describe('Sudo Mode Authentication', () => { | |
| let testUserId: number; | |
| let validToken: string; | |
| beforeAll(async () => { | |
| // 使用已注册的测试账号登录 | |
| const loginRes = await request(app.getHttpServer()) | |
| .post('/users/auth/login') | |
| .send({ | |
| username: TestUsername, | |
| password: 'ABC^^^666', | |
| }) | |
| .expect(201); | |
| validToken = loginRes.body.data.accessToken; | |
| testUserId = loginRes.body.data.user.id; | |
| }); | |
| it('should verify sudo mode with password', async () => { | |
| const res = await request(app.getHttpServer()) | |
| .post('/users/auth/sudo') | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send({ | |
| method: 'password', | |
| credentials: { | |
| password: 'ABC^^^666', | |
| }, | |
| }) | |
| .expect(201); | |
| expect(res.body.data.accessToken).toBeDefined(); | |
| // 保存sudo token用于后续测试 | |
| validToken = res.body.data.accessToken; | |
| }); | |
| // it('should verify sudo mode with passkey', async () => { | |
| // const fakeAuthResponse = { | |
| // id: 'cred-id', | |
| // rawId: 'raw-cred-id', | |
| // response: {}, | |
| // type: 'public-key', | |
| // clientExtensionResults: {}, | |
| // }; | |
| // const res = await request(app.getHttpServer()) | |
| // .post('/users/auth/sudo') | |
| // .set('Authorization', `Bearer ${validToken}`) | |
| // .send({ | |
| // method: 'passkey', | |
| // credentials: { | |
| // passkeyResponse: fakeAuthResponse, | |
| // }, | |
| // }) | |
| // .expect(200); | |
| // expect(res.body.data.accessToken).toBeDefined(); | |
| // }); | |
| it('should reject sudo verification with wrong password', async () => { | |
| const res = await request(app.getHttpServer()) | |
| .post('/users/auth/sudo') | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send({ | |
| method: 'password', | |
| credentials: { | |
| password: 'wrong-password', | |
| }, | |
| }) | |
| .expect(401); | |
| expect(res.body.message).toMatch(/InvalidCredentialsError/); | |
| }); | |
| // 测试需要 sudo 权限的操作 | |
| // it('should require sudo mode for sensitive operations', async () => { | |
| // // 使用非 sudo token | |
| // const loginRes = await request(app.getHttpServer()) | |
| // .post('/users/auth/login') | |
| // .send({ | |
| // username: TestUsername, | |
| // password: 'ABC^^^666', | |
| // }) | |
| // .expect(201); | |
| // const normalToken = loginRes.body.data.accessToken; | |
| // // 尝试执行需要 sudo 的操作（例如修改密码） | |
| // const res = await request(app.getHttpServer()) | |
| // .post(`/users/${testUserId}/password`) | |
| // .set('Authorization', `Bearer ${normalToken}`) | |
| // .send({ | |
| // oldPassword: 'ABC^^^666', | |
| // newPassword: 'NewABC^^^666', | |
| // }) | |
| // .expect(403); | |
| // expect(res.body.message).toMatch(/SudoRequiredError/); | |
| // }); | |
| }); | |
| describe('Sudo Mode Authentication', () => { | |
| let testUserId: number; | |
| let validToken: string; | |
| beforeAll(async () => { | |
| // 使用已注册的测试账号登录 | |
| const loginRes = await request(app.getHttpServer()) | |
| .post('/users/auth/login') | |
| .send({ | |
| username: TestUsername, | |
| password: 'ABC^^^666', | |
| }) | |
| .expect(201); | |
| validToken = loginRes.body.data.accessToken; | |
| testUserId = loginRes.body.data.user.id; | |
| }); | |
| it('should verify sudo mode with password', async () => { | |
| const res = await request(app.getHttpServer()) | |
| .post('/users/auth/sudo') | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send({ | |
| method: 'password', | |
| credentials: { | |
| password: 'ABC^^^666', | |
| }, | |
| }) | |
| .expect(201); | |
| expect(res.body.data.accessToken).toBeDefined(); | |
| // 保存sudo token用于后续测试 | |
| validToken = res.body.data.accessToken; | |
| }); | |
| it('should expire sudo token after timeout', async () => { | |
| jest.useFakeTimers(); | |
| // Get sudo token | |
| const res = await request(app.getHttpServer()) | |
| .post('/users/auth/sudo') | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send({ | |
| method: 'password', | |
| credentials: { | |
| password: 'ABC^^^666', | |
| }, | |
| }) | |
| .expect(201); | |
| const sudoToken = res.body.data.accessToken; | |
| // Advance time by 20 minutes (max sudo duration) | |
| jest.advanceTimersByTime(20 * 60 * 1000); | |
| // Try using expired sudo token | |
| await request(app.getHttpServer()) | |
| .post(`/users/${testUserId}/password`) | |
| .set('Authorization', `Bearer ${sudoToken}`) | |
| .send({ | |
| oldPassword: 'ABC^^^666', | |
| newPassword: 'NewABC^^^666', | |
| }) | |
| .expect(401); | |
| jest.useRealTimers(); | |
| }); | |
| it('should verify sudo mode with passkey', async () => { | |
| const fakeAuthResponse = { | |
| id: 'cred-id', | |
| rawId: 'raw-cred-id', | |
| response: {}, | |
| type: 'public-key', | |
| clientExtensionResults: {}, | |
| }; | |
| const res = await request(app.getHttpServer()) | |
| .post('/users/auth/sudo') | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send({ | |
| method: 'passkey', | |
| credentials: { | |
| passkeyResponse: fakeAuthResponse, | |
| }, | |
| }) | |
| .expect(201); | |
| expect(res.body.data.accessToken).toBeDefined(); | |
| }); | |
| it('should reject sudo verification with invalid passkey', async () => { | |
| const fakeAuthResponse = { | |
| id: 'invalid-cred-id', | |
| rawId: 'invalid-raw-cred-id', | |
| response: {}, | |
| type: 'public-key', | |
| clientExtensionResults: {}, | |
| }; | |
| await request(app.getHttpServer()) | |
| .post('/users/auth/sudo') | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send({ | |
| method: 'passkey', | |
| credentials: { | |
| passkeyResponse: fakeAuthResponse, | |
| }, | |
| }) | |
| .expect(401); | |
| }); | |
| it('should reject sudo verification with wrong password', async () => { | |
| const res = await request(app.getHttpServer()) | |
| .post('/users/auth/sudo') | |
| .set('Authorization', `Bearer ${validToken}`) | |
| .send({ | |
| method: 'password', | |
| credentials: { | |
| password: 'wrong-password', | |
| }, | |
| }) | |
| .expect(401); | |
| expect(res.body.message).toMatch(/InvalidCredentialsError/); | |
| }); | |
| // 测试需要 sudo 权限的操作 | |
| // it('should require sudo mode for sensitive operations', async () => { | |
| // // 使用非 sudo token | |
| // const loginRes = await request(app.getHttpServer()) | |
| // .post('/users/auth/login') | |
| // .send({ | |
| // username: TestUsername, | |
| // password: 'ABC^^^666', | |
| // }) | |
| // .expect(201); | |
| // | |
| // const normalToken = loginRes.body.data.accessToken; | |
| // | |
| // // 尝试执行需要 sudo 的操作（例如修改密码） | |
| // const res = await request(app.getHttpServer()) | |
| // .post(`/users/${testUserId}/password`) | |
| // .set('Authorization', `Bearer ${normalToken}`) | |
| // .send({ | |
| // oldPassword: 'ABC^^^666', | |
| // newPassword: 'NewABC^^^666', | |
| // }) | |
| // .expect(403); | |
| // | |
| // expect(res.body.message).toMatch(/SudoRequiredError/); | |
| // }); | |
| }); |
| model Passkey { | ||
| id Int @id(map: "PK_passkey") @default(autoincrement()) | ||
| credentialId String // 凭证ID | ||
| publicKey Bytes // 存储公钥（二进制数据） | ||
| counter Int // 验证计数器 | ||
| deviceType String // 'singleDevice' 或 'multiDevice' | ||
| backedUp Boolean // 是否已备份 | ||
| transports String? // 可选，存储传输方式（JSON 数组字符串） | ||
| userId Int // 关联用户ID | ||
| user User @relation(fields: [userId], references: [id]) | ||
| createdAt DateTime @default(now()) @map("created_at") @db.Timestamptz(6) | ||
| updatedAt DateTime @default(now()) @updatedAt @map("updated_at") @db.Timestamptz(6) | ||
|
|
||
| @@map("passkey") | ||
| @@index([userId], map: "IDX_passkey_user_id") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add unique constraint to Passkey.
The model should have a unique constraint on credentialId to prevent duplicate registrations of the same credential.
Apply this diff:
model Passkey {
id Int @id(map: "PK_passkey") @default(autoincrement())
- credentialId String // 凭证ID
+ credentialId String @unique // 凭证ID
publicKey Bytes // 存储公钥（二进制数据）
counter Int // 验证计数器
deviceType String // 'singleDevice' 或 'multiDevice'
backedUp Boolean // 是否已备份
transports String? // 可选，存储传输方式（JSON 数组字符串）
userId Int // 关联用户ID
user User @relation(fields: [userId], references: [id])
createdAt DateTime @default(now()) @map("created_at") @db.Timestamptz(6)
updatedAt DateTime @default(now()) @updatedAt @map("updated_at") @db.Timestamptz(6)
@@map("passkey")
@@index([userId], map: "IDX_passkey_user_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.
| model Passkey { | |
| id Int @id(map: "PK_passkey") @default(autoincrement()) | |
| credentialId String // 凭证ID | |
| publicKey Bytes // 存储公钥（二进制数据） | |
| counter Int // 验证计数器 | |
| deviceType String // 'singleDevice' 或 'multiDevice' | |
| backedUp Boolean // 是否已备份 | |
| transports String? // 可选，存储传输方式（JSON 数组字符串） | |
| userId Int // 关联用户ID | |
| user User @relation(fields: [userId], references: [id]) | |
| createdAt DateTime @default(now()) @map("created_at") @db.Timestamptz(6) | |
| updatedAt DateTime @default(now()) @updatedAt @map("updated_at") @db.Timestamptz(6) | |
| @@map("passkey") | |
| @@index([userId], map: "IDX_passkey_user_id") | |
| } | |
| model Passkey { | |
| id Int @id(map: "PK_passkey") @default(autoincrement()) | |
| credentialId String @unique // 凭证ID | |
| publicKey Bytes // 存储公钥（二进制数据） | |
| counter Int // 验证计数器 | |
| deviceType String // 'singleDevice' 或 'multiDevice' | |
| backedUp Boolean // 是否已备份 | |
| transports String? // 可选，存储传输方式（JSON 数组字符串） | |
| userId Int // 关联用户ID | |
| user User @relation(fields: [userId], references: [id]) | |
| createdAt DateTime @default(now()) @map("created_at") @db.Timestamptz(6) | |
| updatedAt DateTime @default(now()) @updatedAt @map("updated_at") @db.Timestamptz(6) | |
| @@map("passkey") | |
| @@index([userId], map: "IDX_passkey_user_id") | |
| } |
Add detailed unit tests for the TOTPService covering: - TOTP secret generation and URI creation - 2FA lifecycle (enable, disable) - Verification process for TOTP and backup codes - Edge cases and token generation
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/users/totp.service.spec.ts (5)
73-77: Enhance TOTP URI test coverage.The URI test should verify all TOTP parameters to ensure complete compatibility with authenticator apps.
Add assertions for algorithm, digits, and period parameters:
it('should generate valid TOTP URI', () => { const uri = service.generateTOTPUri('SECRET123', 'testuser'); expect(uri).toContain('otpauth://totp/TestApp:testuser'); expect(uri).toContain('secret=SECRET123'); + expect(uri).toContain('algorithm=SHA1'); + expect(uri).toContain('digits=6'); + expect(uri).toContain('period=30'); });
97-116: Add edge cases to 2FA enablement tests.Consider adding tests for the following scenarios:
- Already enabled 2FA
- Rate limiting of attempts
- Concurrent enable/disable operations
Example test case:
it('should prevent enabling 2FA when already enabled', async () => { mockUser.totpEnabled = true; const testSecret = authenticator.generateSecret(); const testToken = authenticator.generate(testSecret); await expect( service.enable2FA(1, testSecret, testToken) ).rejects.toThrow('2FA is already enabled'); });
181-186: Enhance backup codes security testing.Add tests to verify:
- Minimum length and complexity of backup codes
- Proper salting of backup code hashes
- Prevention of duplicate codes
Example test:
it('should generate secure backup codes', async () => { const codes = await service.generateAndSaveBackupCodes(1); codes.forEach(code => { expect(code).toMatch(/^[A-Z0-9]{16}$/); // Verify format expect(new Set(codes).size).toBe(codes.length); // Verify uniqueness }); });
189-204: Add security-focused edge cases.Consider adding tests for:
- Account lockout after multiple failed attempts
- Expired TOTP tokens
- Cross-account verification attempts
Example test:
it('should prevent cross-account verification', async () => { const userId = 1; const differentUserId = 2; const token = 'valid-token'; // Setup mock to return different user mockPrisma.user.findUnique .mockResolvedValueOnce({ id: differentUserId, totpEnabled: true }); await expect( service.verify2FA(userId, token) ).rejects.toThrow('User mismatch'); });
206-231: Enhance token generation tests.Add assertions to verify:
- Token expiration time
- Token format and contents
- Token invalidation on 2FA disable
Example test:
it('should generate token with correct expiration', async () => { const token = await service.verify2FAAndGenerateToken(1, 'valid-token'); const decoded = jwt.decode(token); expect(decoded).toHaveProperty('exp'); expect(decoded.exp - decoded.iat).toBe(15 * 60); // 15 minutes });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/users/totp.service.spec.ts(1 hunks)
🧰 Additional context used
🪛 Gitleaks (8.21.2)
src/users/totp.service.spec.ts
47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: test (12, 8.0.0, 20.x, 8, 2)
- GitHub Check: test (12, 8.0.0, 20.x, 8, 1)
- GitHub Check: test (12, 8.0.0, 18.x, 8, 2)
- GitHub Check: test (12, 8.0.0, 18.x, 8, 1)
- GitHub Check: test (12, 8.12.2, 20.x, 8, 2)
- GitHub Check: test (12, 8.12.2, 20.x, 8, 1)
- GitHub Check: test (12, 8.12.2, 18.x, 8, 2)
- GitHub Check: test (12, 8.12.2, 18.x, 8, 1)
- GitHub Check: test (latest, 8.0.0, 20.x, 8, 2)
- GitHub Check: test (latest, 8.0.0, 20.x, 8, 1)
- GitHub Check: test (latest, 8.0.0, 18.x, 8, 2)
- GitHub Check: test (latest, 8.0.0, 18.x, 8, 1)
- GitHub Check: test (latest, 8.12.2, 20.x, 8, 1)
- GitHub Check: test (latest, 8.12.2, 18.x, 8, 2)
- GitHub Check: test (latest, 8.12.2, 18.x, 8, 1)
- GitHub Check: test (latest, 8.12.2, 18.x, 8)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
src/users/totp.service.spec.ts (1)
1-232: Well-structured and comprehensive test suite!The test implementation thoroughly covers the TOTP service functionality and aligns well with the PR objectives. The suggested improvements will further enhance security testing, but the current implementation provides a solid foundation.
🧰 Tools
🪛 Gitleaks (8.21.2)
47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| appName: 'TestApp', | ||
| backupCodesCount: 5, | ||
| window: 1, | ||
| encryptionKey: 'test-encryption-key-1234567890abc', |
There was a problem hiding this comment.
Secure the test encryption key.
The test encryption key should be replaced with a more secure mock value to prevent potential security risks, even in test environments.
Apply this diff to improve security:
- encryptionKey: 'test-encryption-key-1234567890abc',
+ encryptionKey: '[MOCK_ENCRYPTION_KEY]',📝 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.
| encryptionKey: 'test-encryption-key-1234567890abc', | |
| encryptionKey: '[MOCK_ENCRYPTION_KEY]', |
🧰 Tools
🪛 Gitleaks (8.21.2)
47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| it('should verify valid TOTP code', async () => { | ||
| const testToken = authenticator.generate('decrypted-secret'); | ||
| const result = await service.verify2FA(1, testToken); | ||
|
|
||
| expect(result.isValid).toBe(true); | ||
| expect(result.usedBackupCode).toBe(false); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify constant-time comparison for TOTP codes.
To prevent timing attacks, ensure that TOTP code comparison is done in constant time.
🏁 Script executed:
#!/bin/bash
# Search for constant-time comparison usage in TOTP verification
rg -A 5 "verify.*TOTP|compare.*token"Length of output: 1776
Action Required: Ensure constant-time comparison in TOTP verification.
The current implementation in src/users/totp.service.ts, where verifyTOTP directly calls authenticator.verify({ token, secret }), does not explicitly show constant-time comparison, which is critical to mitigate timing attacks. Please verify if the underlying library function performs constant-time checks. If not, update the comparison—possibly leveraging Node.js’s crypto.timingSafeEqual—to guarantee that the TOTP code is compared in constant time.
- Location:
src/users/totp.service.ts(functionverifyTOTP) - Suggestion: Confirm the security properties of
authenticator.verify()or modify the implementation to include a constant-time comparison.
Implement Passkey, Sudo Mode, and TOTP Two-Factor Authentication
Background
To enhance security and user experience, we're introducing three major authentication features:
Changes
Passkey Authentication
Sudo Mode
TOTP Two-Factor Authentication
API Changes
/users/auth/passkey/*/users/auth/sudo/users/{userId}/2fa/*/users/auth/verify-2faSecurity Considerations