-
Notifications
You must be signed in to change notification settings - Fork 0
20 oci discourse verification #30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Warning Rate limit exceeded@Behzad-rabiei has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 39 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces a comprehensive set of changes across multiple files in the project, primarily focusing on code formatting, specifically the addition of semicolons, and a significant refactoring of JWT (JSON Web Token) handling. The changes involve introducing a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller
participant JwtService
participant ConfigService
Client->>Controller: Request JWT
Controller->>JwtService: Generate JWT
JwtService->>ConfigService: Get JWT Configuration
ConfigService-->>JwtService: Return Configuration
JwtService->>JwtService: Create JWT Payload
JwtService-->>Controller: Return Signed JWT
Controller-->>Client: Return JWT
Possibly Related PRs
Poem
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 16
🔭 Outside diff range comments (2)
src/config/index.ts (1)
Line range hint
33-36: Remove duplicate loggerConfigSchema entriesThe
loggerConfigSchemais repeated three times in the validation schema, which is redundant and potentially confusing....authConfigSchema, - ...loggerConfigSchema, - ...loggerConfigSchema, ...loggerConfigSchema, ...walletConfigSchema,src/eas/eas.service.spec.ts (1)
Remove outdated tests and implement new ones matching the current architecture
The commented tests are outdated and don't match the current implementation:
- The service now manages multiple EAS contracts using a Map instead of a single instance
- The
getDomainmethod no longer exists- The architecture has changed significantly to support multiple chains
Please remove these commented tests and implement new ones that verify:
- Contract initialization for multiple chains
- Proper contract and attester management
- Current functionality like attestation signing and secret handling
🔗 Analysis chain
Line range hint
48-66: Uncomment and update the EAS contract testsSeveral tests are commented out. These should either be:
- Uncommented and updated to work with the new JwtService
- Removed if no longer relevant
- Marked with a TODO if pending implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other commented tests in the codebase rg -A 5 "^\s*//\s*it\(" src/Length of output: 2504
Script:
#!/bin/bash # Check EAS service implementation ast-grep --pattern 'class EasService { $$$ }' # Also check for domain-related code rg "getDomain|domain" src/eas/Length of output: 14355
🧹 Nitpick comments (23)
src/utils/crypto-utils.service.ts (1)
29-42: Consider improving random number generation and adding input validation.While the implementation is generally secure using
crypto.randomBytes(), there are two potential improvements:
- The modulo operation (
randomBytes[i] % charsLength) can introduce a slight bias in the distribution. Consider using a more uniform distribution method.- Add input validation for the length parameter to prevent potential issues with negative or extremely large values.
Here's a suggested improvement:
private generateRandomCode( length: number, allowedCharacters: string ): string { + if (length <= 0 || length > 1000) { + throw new Error('Length must be between 1 and 1000'); + } const charsLength = allowedCharacters.length; - const randomBytes = crypto.randomBytes(length); let code = ''; - for (let i = 0; i < length; i++) { - const randomIndex = randomBytes[i] % charsLength; + // Use rejection sampling for uniform distribution + while (code.length < length) { + const randomBytes = crypto.randomBytes(1); + const randomValue = randomBytes[0]; + // Only use values that don't introduce modulo bias + if (randomValue < 256 - (256 % charsLength)) { + const randomIndex = randomValue % charsLength; code += allowedCharacters.charAt(randomIndex); - } + } } return code; }src/jwt/jwt.service.ts (1)
53-60: Ensure 'wallet.publicKey' is defined in configurationThe
issclaim useswallet.publicKeyfrom the configuration. If it's undefined, the JWT may have incorrect issuer information. Add a check to ensure it's defined.Apply this diff to add a check:
const issuer = this.configService.get<string>('wallet.publicKey'); +if (!issuer) { + this.logger.error('wallet.publicKey is not defined in the configuration'); + throw new Error('Issuer is not configured'); +} const payload: AuthJwtPayload = { sub: identifier, provider, iat: now, exp, iss: issuer, metadata, };src/discourse-verification/discourse-verification.service.ts (4)
65-71: Handle specific HTTP errors when fetching topic dataIn
fetchTopicData, all HTTP errors result in a genericBadRequestException. Consider handling specific HTTP status codes to provide more informative error messages.Apply this diff to improve error handling:
async fetchTopicData(url: string): Promise<any> { try { const response = await lastValueFrom(this.httpService.get(url)); return response.data; } catch (error) { + if (error.response) { + const status = error.response.status; + if (status === 404) { + throw new BadRequestException('Discourse topic not found (404)'); + } + // Handle other specific statuses as needed + } throw new BadRequestException( 'Failed to retrieve Discourse topic JSON' ); } }
74-90: Define interfaces for external data to enhance type safetyUsing
anyfortopicDatareduces type safety. Define interfaces for the expected structure of external data to improve reliability.For example, define an interface:
interface Post { user_id?: number; cooked?: string; } interface TopicData { post_stream?: { posts?: Post[]; }; } private validateTopicData( topicData: TopicData, jwtPayload: VerificationJwtPayload ): void { // Existing validation logic }
30-54: Validate 'topicUrl' to ensure it's a valid URLBefore using
new URL()withtopicUrl, validate that it's a properly formatted URL to prevent exceptions and handle errors gracefully.Apply this diff to add URL validation:
async verifyDiscourseTopic(verifyDto: VerifyDto): Promise<any> { + let urlObj: URL; const topicUrl = this.normalizeTopicUrl(verifyDto.topicUrl); const payload = (await this.jwtService.validateToken( verifyDto.verificationJwt )) as VerificationJwtPayload; const topicData = await this.fetchTopicData(topicUrl); this.validateTopicData(topicData, payload); const firstPost = topicData?.post_stream?.posts?.[0]; if (!firstPost?.user_id) { throw new BadRequestException( 'Could not determine user ID from the topic' ); } - const urlObj = new URL(verifyDto.topicUrl); + try { + urlObj = new URL(verifyDto.topicUrl); + } catch (error) { + throw new BadRequestException('Invalid topic URL'); + } const baseURL = urlObj.origin; const discourseJwt = await this.jwtService.generateAuthJwt( firstPost?.user_id, 'discourse', { baseURL } ); return discourseJwt; }
48-52: Ensure 'user_id' is a string when generating JWTThe
user_idmay be a number, but JWTsubclaims are typically strings. Convertuser_idto a string to prevent type issues.Apply this diff to convert
user_id:const discourseJwt = await this.jwtService.generateAuthJwt( - firstPost?.user_id, + firstPost?.user_id.toString(), 'discourse', { baseURL } );src/auth/types/jwt-payload.type.ts (1)
2-7: Add documentation comments for JWT payload fieldsAdding JSDoc comments to the
JwtPayloadfields improves code readability and helps developers understand the purpose of each property.Apply this diff to add comments:
export type JwtPayload = { + /** Subject (user identifier) */ sub: string; + /** Authentication provider */ provider: string; + /** Issuer */ iss: string; + /** Issued at timestamp */ iat: number; + /** Expiration timestamp */ exp: number; + /** Additional metadata */ metadata?: Record<string, any>; };src/discourse-verification/dto/generate-verification-token-response.dto.ts (1)
4-9: Enhance API documentation clarity.Consider providing a complete example JWT token to avoid confusion, or explicitly mention that the example is truncated.
example: - 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJjb2RlIjoiMjMxMjQ... (truncated)', + 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJjb2RlIjoiMTIzNDU2Nzg5MCIsInN1YiI6InVzZXItaWQiLCJpYXQiOjE1MTYyMzkwMjJ9.4Adcj3UFYzPUVaVF43FmMab6RlaQD6AuXvnvP0QiFAk',src/jwt/types/jwt-payload.type.ts (1)
12-15: Consider using a more specific type for metadata.The
Record<string, any>type for metadata could lead to type safety issues. Consider defining a more specific interface for the metadata structure if the possible keys and value types are known.export interface AuthJwtPayload extends BaseJwtPayload { provider: string; - metadata?: Record<string, any>; + metadata?: { + // Define specific metadata fields here + userId?: string; + roles?: string[]; + // Add other known metadata fields + }; }src/discourse-verification/dto/verify-response.dto.ts (1)
7-8: Consider using a more generic JWT example.While the current JWT example is properly structured, consider using a more generic example to avoid any potential confusion or security concerns. For instance, you could use placeholder values like
{user_id: 1}in the payload.- 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VyX2lkIjoxMjMsImJhc2VVUkwiOiJodHRwczovL2ZvcnVtcy5leGFtcGxlLmNvbSIsImlhdCI6MTY5MTMxMjAwMH0.x4dF-KO9drgBbLj197UQVHev4dgd0DcNEkN8bsuEPnY', + 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VyX2lkIjoxLCJiYXNlVVJMIjoiaHR0cHM6Ly9leGFtcGxlLmNvbSJ9.signature',🧰 Tools
🪛 Gitleaks (8.21.2)
8-8: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
src/discourse-verification/dto/generate-verification-token.dto.ts (1)
11-13: Consider using a more generic JWT example.Similar to the previous DTO, consider using a more generic JWT example to avoid any potential confusion.
- 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6', + 'eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxIiwibmFtZSI6ImV4YW1wbGUifQ.signature',src/discourse-verification/dto/verify.dto.ts (2)
7-10: Update example URL in swagger docsThe example URL is incomplete. Provide a complete URL that includes the topic path.
@ApiProperty({ description: 'the discourse topic url.', - example: 'https://forum.arbitrum.foundation', + example: 'https://forum.arbitrum.foundation/t/example-topic/123', required: true, })
21-23: Consider adding JWT format validationWhile
@IsString()and@IsNotEmpty()provide basic validation, consider adding a custom validator or regex pattern to ensure the JWT follows the correct format.@IsString() @IsNotEmpty() + @Matches(/^[A-Za-z0-9-_]+\.[A-Za-z0-9-_]+\.[A-Za-z0-9-_]*$/, { + message: 'Invalid JWT format' + }) verificationJwt: string;src/jwt/config/jwt.config.ts (1)
7-9: Improve number parsing and validationThe current implementation uses
parseIntwithout error handling. Consider using a more robust approach.- authExpiresIn: parseInt(process.env.JWT_AUTH_EXPIRATION_MINUTES, 10) || 60, + authExpiresIn: Number(process.env.JWT_AUTH_EXPIRATION_MINUTES) || 60, discourseVerificationExpiresIn: - parseInt(process.env.JWT_VERIFICATION_MINUTES, 10) || 10, + Number(process.env.JWT_VERIFICATION_MINUTES) || 10,src/jwt/jwt.service.spec.ts (1)
12-17: Enhance ConfigService mock implementationThe current mock only handles 'jwt.secret'. Add cases for other JWT configuration values and error scenarios.
const mockConfigService = { get: jest.fn((key: string) => { if (key === 'jwt.secret') return 'test-value'; + if (key === 'jwt.authExpiresIn') return 60; + if (key === 'jwt.discourseVerificationExpiresIn') return 10; return null; }), + getOrThrow: jest.fn((key: string) => { + const value = mockConfigService.get(key); + if (!value) throw new Error(`Config ${key} not found`); + return value; + }), };src/discourse-verification/discourse-verification.controller.ts (3)
34-43: Method name should follow camelCase conventionThe method
GenerateVerificationTokenshould be renamed togenerateVerificationTokento follow TypeScript naming conventions for methods.- async GenerateVerificationToken( + async generateVerificationToken(
22-43: Consider adding error handlingWhile the endpoint is well-structured, it would benefit from explicit error handling to catch and properly format potential errors from the service layer.
@HttpCode(HttpStatus.OK) async generateVerificationToken( @Body() generateVerificationTokenDto: GenerateVerificationTokenDto ): Promise<GenerateVerificationTokenResponseDto> { + try { return { verificationJwt: await this.discourseVerificationService.generateVerificationToken( generateVerificationTokenDto ), }; + } catch (error) { + // Re-throw as NestJS HttpException with appropriate status + throw new BadRequestException('Failed to generate verification token'); + } }
45-62: Add consistent error handlingFor consistency with the token endpoint, add similar error handling here.
@HttpCode(HttpStatus.OK) async verify(@Body() verifyDto: VerifyDto): Promise<VerifyResponseDto> { + try { return { discourseJwt: await this.discourseVerificationService.verifyDiscourseTopic( verifyDto ), }; + } catch (error) { + throw new BadRequestException('Discourse verification failed'); + } }src/eas/eas.service.spec.ts (1)
3-13: Group related imports togetherConsider organizing imports into groups:
- External libraries
- NestJS imports
- Local imports
This improves code readability and maintainability.
+ // External libraries import { LoggerModule, PinoLogger } from 'nestjs-pino'; import { generatePrivateKey } from 'viem/accounts'; + // NestJS imports import { ConfigService } from '@nestjs/config'; import { Test, TestingModule } from '@nestjs/testing'; + // Local imports import { JwtService } from '../jwt/jwt.service'; import { LitService } from '../lit/lit.service'; import { DataUtilsService } from '../utils/data-utils.service'; import { EthersUtilsService } from '../utils/ethers.utils.service'; import { EasService } from './eas.service';src/eas/eas.service.ts (3)
145-150: Add type assertion for JWT validation results.The JWT validation results should be explicitly typed to ensure type safety.
- const siweJwtPayload = (await this.jwtService.validateToken( - siweJwt - )) as AuthJwtPayload; - const anyJwtPayload = (await this.jwtService.validateToken( - anyJwt - )) as AuthJwtPayload; + const siweJwtPayload = await this.jwtService.validateToken<AuthJwtPayload>(siweJwt); + const anyJwtPayload = await this.jwtService.validateToken<AuthJwtPayload>(anyJwt);
202-204: Add type assertion for SIWE JWT payload.The SIWE JWT payload should be explicitly typed as
AuthJwtPayload.- const siweJwtPayload = await this.jwtService.validateToken(siweJwt); + const siweJwtPayload = await this.jwtService.validateToken<AuthJwtPayload>(siweJwt);
246-248: Add type assertion for SIWE JWT payload in decryptAttestationSecret.The SIWE JWT payload should be explicitly typed as
AuthJwtPayload.- const siweJwtPayload = await this.jwtService.validateToken(siweJwt); + const siweJwtPayload = await this.jwtService.validateToken<AuthJwtPayload>(siweJwt);src/auth-siwe/siwe.service.ts (1)
16-18: Consider adding error handling and documentation.While the implementation is correct, consider these improvements:
- Add error handling for potential generateSiweNonce failures
- Add JSDoc comments explaining the nonce's purpose, format, and usage in the SIWE flow
+ /** + * Generates a random nonce for SIWE message signing. + * @returns A random string to be used as a nonce in the SIWE message + * @throws Error if nonce generation fails + */ getNonce(): string { + try { return generateSiweNonce(); + } catch (error) { + this.logger.error(error, 'Failed to generate SIWE nonce'); + throw new HttpException( + 'Failed to generate authentication nonce', + HttpStatus.INTERNAL_SERVER_ERROR + ); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (82)
.prettierrc(1 hunks)src/app.module.ts(2 hunks)src/auth-discord/auth-discord-controller.spec.ts(1 hunks)src/auth-discord/auth-discord.controller.ts(3 hunks)src/auth-discord/auth-discord.module.ts(1 hunks)src/auth-discord/config/auth-discord.config.ts(3 hunks)src/auth-discord/dto/handle-oauth-callback-dto.ts(1 hunks)src/auth-google/auth-google.controller.spec.ts(2 hunks)src/auth-google/auth-google.controller.ts(3 hunks)src/auth-google/auth-google.module.ts(1 hunks)src/auth-google/config/google.config.ts(3 hunks)src/auth-google/dto/handle-oauth-callback-dto.ts(1 hunks)src/auth-siwe/auth-siwe.controller.spec.ts(2 hunks)src/auth-siwe/auth-siwe.controller.ts(3 hunks)src/auth-siwe/auth-siwe.module.ts(1 hunks)src/auth-siwe/dto/nonce.dto.ts(1 hunks)src/auth-siwe/dto/verify-siwe.dto.ts(3 hunks)src/auth-siwe/siwe.service.spec.ts(2 hunks)src/auth-siwe/siwe.service.ts(2 hunks)src/auth/auth.module.ts(1 hunks)src/auth/auth.service.spec.ts(0 hunks)src/auth/auth.service.ts(0 hunks)src/auth/config/auth.config.ts(1 hunks)src/auth/constants/oAuth.constants.ts(1 hunks)src/auth/constants/provider.constants.ts(1 hunks)src/auth/dto/jwt-response.dto.ts(1 hunks)src/auth/oAuth.service.spec.ts(2 hunks)src/auth/oAuth.service.ts(3 hunks)src/auth/types/auth-provider.type.ts(1 hunks)src/auth/types/jwt-payload.type.ts(1 hunks)src/config/app.config.ts(2 hunks)src/config/index.ts(3 hunks)src/config/logger.config.ts(1 hunks)src/config/pino.config.ts(2 hunks)src/config/wallet.config.ts(1 hunks)src/discourse-verification/discourse-verification.controller.spec.ts(1 hunks)src/discourse-verification/discourse-verification.controller.ts(1 hunks)src/discourse-verification/discourse-verification.module.ts(1 hunks)src/discourse-verification/discourse-verification.service.spec.ts(1 hunks)src/discourse-verification/discourse-verification.service.ts(1 hunks)src/discourse-verification/dto/generate-verification-token-response.dto.ts(1 hunks)src/discourse-verification/dto/generate-verification-token.dto.ts(1 hunks)src/discourse-verification/dto/verify-response.dto.ts(1 hunks)src/discourse-verification/dto/verify.dto.ts(1 hunks)src/doc/index.ts(1 hunks)src/doc/swagger.constants.ts(1 hunks)src/eas/constants/attestation.constants.ts(2 hunks)src/eas/constants/sepolia.constants.ts(2 hunks)src/eas/dto/decrypt-attestation-secret.dto.ts(2 hunks)src/eas/dto/sign-delegated-attestation.dto.ts(4 hunks)src/eas/dto/sign-delegated-revocation.dto.ts(2 hunks)src/eas/eas.controller.ts(4 hunks)src/eas/eas.module.ts(1 hunks)src/eas/eas.service.spec.ts(2 hunks)src/eas/eas.service.ts(3 hunks)src/eas/interfaces/attestation.interfaces.ts(2 hunks)src/jwt/config/jwt.config.ts(1 hunks)src/jwt/jwt.module.ts(1 hunks)src/jwt/jwt.service.spec.ts(1 hunks)src/jwt/jwt.service.ts(1 hunks)src/jwt/types/jwt-payload.type.ts(1 hunks)src/lit/config/lit.config.ts(1 hunks)src/lit/constants/network.constants.ts(2 hunks)src/lit/lit.module.ts(1 hunks)src/lit/lit.service.ts(6 hunks)src/main.ts(1 hunks)src/shared/constants/chain.constants.ts(4 hunks)src/shared/decorators/jwt-provider.decorator.ts(2 hunks)src/shared/filters/http-exception.filter.ts(2 hunks)src/shared/types/chain.type.ts(1 hunks)src/utils/config/crypto-utils.config.ts(1 hunks)src/utils/crypto-utils.service.spec.ts(1 hunks)src/utils/crypto-utils.service.ts(1 hunks)src/utils/data-utils.service.spec.ts(1 hunks)src/utils/data-utils.service.ts(1 hunks)src/utils/encode-utils.service.spec.ts(1 hunks)src/utils/encode-utils.service.ts(2 hunks)src/utils/ethers.utils.service.spec.ts(1 hunks)src/utils/ethers.utils.service.ts(1 hunks)src/utils/utils.module.ts(1 hunks)src/utils/viem.service.spec.ts(1 hunks)src/utils/viem.utils.service.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/auth/auth.service.ts
- src/auth/auth.service.spec.ts
✅ Files skipped from review due to trivial changes (51)
- src/auth/dto/jwt-response.dto.ts
- .prettierrc
- src/shared/types/chain.type.ts
- src/auth/types/auth-provider.type.ts
- src/utils/ethers.utils.service.spec.ts
- src/config/logger.config.ts
- src/auth/constants/provider.constants.ts
- src/auth-siwe/siwe.service.spec.ts
- src/utils/utils.module.ts
- src/auth-google/auth-google.module.ts
- src/config/wallet.config.ts
- src/auth/constants/oAuth.constants.ts
- src/jwt/jwt.module.ts
- src/auth-discord/config/auth-discord.config.ts
- src/auth-google/auth-google.controller.spec.ts
- src/eas/constants/sepolia.constants.ts
- src/utils/encode-utils.service.spec.ts
- src/config/app.config.ts
- src/lit/constants/network.constants.ts
- src/doc/swagger.constants.ts
- src/lit/lit.module.ts
- src/auth-discord/dto/handle-oauth-callback-dto.ts
- src/eas/eas.controller.ts
- src/eas/dto/decrypt-attestation-secret.dto.ts
- src/eas/interfaces/attestation.interfaces.ts
- src/main.ts
- src/shared/filters/http-exception.filter.ts
- src/utils/data-utils.service.ts
- src/utils/crypto-utils.service.spec.ts
- src/auth-google/config/google.config.ts
- src/config/pino.config.ts
- src/auth-discord/auth-discord-controller.spec.ts
- src/utils/encode-utils.service.ts
- src/lit/config/lit.config.ts
- src/auth-siwe/dto/nonce.dto.ts
- src/utils/config/crypto-utils.config.ts
- src/doc/index.ts
- src/auth/config/auth.config.ts
- src/shared/decorators/jwt-provider.decorator.ts
- src/utils/data-utils.service.spec.ts
- src/utils/viem.service.spec.ts
- src/auth-discord/auth-discord.controller.ts
- src/eas/dto/sign-delegated-revocation.dto.ts
- src/auth-google/dto/handle-oauth-callback-dto.ts
- src/lit/lit.service.ts
- src/utils/ethers.utils.service.ts
- src/utils/viem.utils.service.ts
- src/auth-siwe/dto/verify-siwe.dto.ts
- src/eas/constants/attestation.constants.ts
- src/eas/dto/sign-delegated-attestation.dto.ts
- src/shared/constants/chain.constants.ts
🧰 Additional context used
🪛 Gitleaks (8.21.2)
src/discourse-verification/dto/verify-response.dto.ts
8-8: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🔇 Additional comments (22)
src/utils/crypto-utils.service.ts (3)
1-6: LGTM! Imports are well-organized.The imports are appropriate and the new
verificationConfigimport aligns with the added functionality.
13-27: LGTM! Cryptographic operations are secure.The implementation uses appropriate cryptographic primitives:
- Random bytes generation for state and verifier
- SHA-256 for code challenge
- Proper encoding through a dedicated service
44-46: LGTM! Well-structured verification code generation.The method properly utilizes configuration values and encapsulates the random code generation logic.
src/jwt/jwt.service.ts (1)
2-2: Consider replacing 'moment' with a modern date-time libraryThe 'moment' library is in maintenance mode and is no longer recommended for new projects. Consider using a modern date-time library like 'Luxon', 'Day.js', or 'date-fns' for better performance and ongoing support.
Would you like me to check for suitable alternatives to 'moment' and their compatibility with your project?
src/discourse-verification/dto/generate-verification-token-response.dto.ts (1)
3-11: LGTM! The DTO structure is clean and follows best practices.The class correctly encapsulates the verification token response with proper type definition.
src/auth-siwe/auth-siwe.module.ts (1)
9-9: Verify the removal of AuthModule dependency.The AuthModule has been removed from imports. Please ensure that all functionality previously provided by AuthModule is now handled by JwtModule.
✅ Verification successful
AuthModule removal is correctly implemented
The AuthModule dependency has been safely removed, with its JWT functionality now properly handled by the dedicated JwtService. The authentication flow is maintained through a clean separation between SIWE verification (SiweService) and JWT generation (JwtService).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining AuthModule dependencies in the auth-siwe directory rg "AuthModule|AuthService" src/auth-siwe/Length of output: 44
Script:
#!/bin/bash # First, verify the file structure fd . src/auth-siwe/ # Then, examine the auth-siwe module implementation ast-grep --pattern 'export class AuthSiweModule { $$$ }'Length of output: 349
Script:
#!/bin/bash # Check service implementation cat src/auth-siwe/siwe.service.ts # Check controller implementation cat src/auth-siwe/auth-siwe.controller.tsLength of output: 3110
src/jwt/types/jwt-payload.type.ts (1)
1-6: LGTM! Well-structured base JWT payload interface.The BaseJwtPayload interface correctly defines standard JWT claims.
src/auth-discord/auth-discord.module.ts (1)
9-9: Verify the necessity of both AuthModule and JwtModule.The module imports both AuthModule and JwtModule. Since JwtModule is being introduced to replace authentication functionality, please verify if AuthModule is still needed.
src/eas/eas.module.ts (1)
3-3: Verify the JWT functionality migration.The replacement of
AuthModulewithJwtModulelooks good and aligns with the JWT handling refactoring. Let's verify that all JWT-related functionality previously handled byAuthModuleis now properly covered byJwtModule.Also applies to: 10-10
✅ Verification successful
JWT functionality migration is correctly implemented
The replacement of
AuthModulewithJwtModuleineas.module.tsis correct. The codebase shows a clear separation of concerns whereAuthModulehandles OAuth functionality whileJwtModulemanages JWT operations. Other modules that need both OAuth and JWT capabilities (likeauth-discord.module.ts) import both modules explicitly.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining AuthModule references that might need updating rg -l "AuthModule|AuthService" src/Length of output: 396
Script:
#!/bin/bash # Check the actual usage context of AuthModule/AuthService echo "=== app.module.ts ===" rg "AuthModule|AuthService" src/app.module.ts -C 2 echo -e "\n=== auth-google.module.ts ===" rg "AuthModule|AuthService" src/auth-google/auth-google.module.ts -C 2 echo -e "\n=== auth-discord.module.ts ===" rg "AuthModule|AuthService" src/auth-discord/auth-discord.module.ts -C 2 echo -e "\n=== Checking imports in auth modules ===" rg "import.*Auth" src/auth-*/auth-*.module.tsLength of output: 2344
Script:
#!/bin/bash # Check the current state of AuthModule echo "=== auth.module.ts content ===" cat src/auth/auth.module.ts echo -e "\n=== Check AuthModule exports ===" ast-grep --pattern 'export class AuthModule { $$$ }'Length of output: 998
src/discourse-verification/dto/verify-response.dto.ts (1)
3-11: LGTM! Clean DTO implementation.The DTO structure follows NestJS best practices with proper use of decorators and type definitions.
🧰 Tools
🪛 Gitleaks (8.21.2)
8-8: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
src/discourse-verification/discourse-verification.module.ts (1)
9-14: LGTM! Well-structured module.The module follows NestJS best practices with appropriate imports and clear separation of concerns.
src/discourse-verification/dto/generate-verification-token.dto.ts (1)
8-19: LGTM! Well-structured DTO with proper validation.The DTO implementation follows best practices with:
- Proper use of class-validator decorators
- Clear API documentation
- Custom JWT provider validation
src/config/index.ts (1)
10-10: LGTM: JWT configuration integrationThe addition of JWT configuration and its schema is well-structured and properly integrated into both the config modules and validation schema.
Also applies to: 24-24, 37-37
src/app.module.ts (1)
12-13: LGTM: Module integrationThe integration of
JwtModuleandDiscourseVerificationModulefollows NestJS best practices and aligns with the PR objectives.Also applies to: 33-34
src/auth-siwe/auth-siwe.controller.ts (2)
54-57: LGTM: JWT generation refactorThe switch to
JwtServicefor token generation is a good architectural improvement that centralizes JWT handling.
25-26: Verify impact of route changesThe controller route has been simplified from dynamic to static. Ensure this change doesn't break any existing client integrations.
✅ Verification successful
Route change verification completed
The simplified route change from dynamic to static in
auth-siwe.controller.tsappears to be safe. No other files in the codebase depend on or reference this route pattern, and the tests are properly abstracted to focus on functionality rather than specific route paths.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any references to the old dynamic route pattern rg -l "auth/\$\{AUTH_PROVIDERS\.SIWE\}" || rg -l "auth/siwe"Length of output: 96
Script:
#!/bin/bash # Search for test files related to SIWE authentication fd -e spec.ts -e test.ts -e e2e-spec.ts | grep -i "siwe" # Search for configuration files that might reference the route fd -e config.ts -e yaml -e json | xargs rg -l "siwe" || trueLength of output: 218
Script:
#!/bin/bash # Check test files for route patterns echo "=== auth-siwe.controller.spec.ts ===" cat src/auth-siwe/auth-siwe.controller.spec.ts echo -e "\n=== siwe.service.spec.ts ===" cat src/auth-siwe/siwe.service.spec.tsLength of output: 5605
src/auth-google/auth-google.controller.ts (2)
35-41: LGTM: OAuth flow implementationThe OAuth flow implementation with state management is secure and follows best practices.
22-23: Verify impact of route changesSimilar to the SIWE controller, the route has been simplified from dynamic to static. Ensure this change doesn't break existing integrations.
✅ Verification successful
Route change is safe to proceed ✅
The static route
auth/googleis already in use across docker compose configurations, and no dynamic route patterns were found in the codebase. This change aligns with existing implementations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any references to the old dynamic route pattern rg -l "auth/\$\{AUTH_PROVIDERS\.GOOGLE\}" || rg -l "auth/google"Length of output: 175
src/auth-siwe/auth-siwe.controller.spec.ts (1)
71-71: LGTM! Test mock implementation is correct.The mock implementation for
generateAuthJwtinJwtServiceis properly set up.src/auth/oAuth.service.spec.ts (1)
87-90: Uncomment and implement error handling tests.Several error handling test cases are commented out. These should be implemented to ensure proper error handling coverage.
Consider implementing the following test cases:
- Token exchange failure
- User information retrieval failure
- OAuth2 callback processing failure
Also applies to: 100-104, 132-136
src/auth-siwe/siwe.service.ts (2)
1-6: LGTM! Well-organized imports with appropriate dependencies.The imports are properly structured and include all necessary dependencies for SIWE authentication functionality.
Line range hint
8-16: LGTM! Well-structured service class following NestJS best practices.The class follows dependency injection patterns and includes proper logging setup.
| } catch (error) { | ||
| this.logger.error(error, 'Failed to validate token'); | ||
| throw new UnauthorizedException(error.message); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent leaking internal error details in exceptions
Returning the raw error message in UnauthorizedException might expose sensitive information. Consider using a generic error message for the client.
Apply this diff to provide a generic error message:
} catch (error) {
this.logger.error(error, 'Failed to validate token');
- throw new UnauthorizedException(error.message);
+ throw new UnauthorizedException('Invalid token');
}📝 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) { | |
| this.logger.error(error, 'Failed to validate token'); | |
| throw new UnauthorizedException(error.message); | |
| } | |
| } catch (error) { | |
| this.logger.error(error, 'Failed to validate token'); | |
| throw new UnauthorizedException('Invalid token'); | |
| } |
| async signPayload(payload: JwtPayload): Promise<string> { | ||
| return jwt.sign(payload, this.configService.get<string>('jwt.secret'), { | ||
| algorithm: 'HS256', | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Ensure 'jwt.secret' is defined and handle missing secret appropriately
In the signPayload method, jwt.secret is retrieved from the configuration. If it's undefined, jwt.sign may throw an error. Consider adding a check to ensure the secret is defined and provide a meaningful error if it's missing.
Apply this diff to add a check:
async signPayload(payload: JwtPayload): Promise<string> {
+ const secret = this.configService.get<string>('jwt.secret');
+ if (!secret) {
+ this.logger.error('JWT secret is not defined in the configuration');
+ throw new Error('JWT secret is not configured');
+ }
return jwt.sign(payload, secret, {
algorithm: 'HS256',
});
}Committable suggestion skipped: line range outside the PR's diff.
| async generateAuthJwt( | ||
| identifier: string, | ||
| provider: string, | ||
| metadata?: Record<string, any> | ||
| ): Promise<string> { | ||
| const now = moment().unix(); | ||
| const exp = moment() | ||
| .add(this.configService.get<string>('jwt.authExpiresIn'), 'minutes') | ||
| .unix(); | ||
|
|
||
| const payload: AuthJwtPayload = { | ||
| sub: identifier, | ||
| provider, | ||
| iat: now, | ||
| exp, | ||
| iss: this.configService.get<string>('wallet.publicKey'), | ||
| metadata, | ||
| }; | ||
|
|
||
| return this.signPayload(payload); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Validate and parse 'jwt.authExpiresIn' configuration value
The authExpiresIn value is retrieved as a string and used in date calculations. Ensure it's properly parsed as a number and handle cases where it's not set or invalid.
Apply this diff to ensure proper handling:
async generateAuthJwt(
identifier: string,
provider: string,
metadata?: Record<string, any>
): Promise<string> {
const now = moment().unix();
+ const expiresInStr = this.configService.get<string>('jwt.authExpiresIn');
+ const authExpiresIn = parseInt(expiresInStr, 10);
+ if (isNaN(authExpiresIn)) {
+ this.logger.error('Invalid jwt.authExpiresIn configuration');
+ throw new Error('JWT authExpiresIn configuration is invalid');
+ }
const exp = moment()
- .add(this.configService.get<string>('jwt.authExpiresIn'), 'minutes')
+ .add(authExpiresIn, 'minutes')
.unix();
const payload: AuthJwtPayload = {
sub: identifier,
provider,
iat: now,
exp,
iss: this.configService.get<string>('wallet.publicKey'),
metadata,
};
return this.signPayload(payload);
}Committable suggestion skipped: line range outside the PR's diff.
| async generateVerificationToken(verificationCode: string): Promise<string> { | ||
| const now = moment().unix(); | ||
| const exp = moment() | ||
| .add(this.configService.get<string>('jwt.authExpiresIn'), 'minutes') | ||
| .unix(); | ||
|
|
||
| const payload: VerificationJwtPayload = { | ||
| sub: 'Verification', | ||
| code: verificationCode, | ||
| iat: now, | ||
| exp, | ||
| iss: this.configService.get<string>('wallet.publicKey'), | ||
| }; | ||
|
|
||
| return this.signPayload(payload); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Validate and parse 'jwt.authExpiresIn' configuration value
In generateVerificationToken, ensure that jwt.authExpiresIn is parsed correctly and handle invalid configurations.
Apply this diff to ensure proper handling:
async generateVerificationToken(verificationCode: string): Promise<string> {
const now = moment().unix();
+ const expiresInStr = this.configService.get<string>('jwt.authExpiresIn');
+ const authExpiresIn = parseInt(expiresInStr, 10);
+ if (isNaN(authExpiresIn)) {
+ this.logger.error('Invalid jwt.authExpiresIn configuration');
+ throw new Error('JWT authExpiresIn configuration is invalid');
+ }
const exp = moment()
- .add(this.configService.get<string>('jwt.authExpiresIn'), 'minutes')
+ .add(authExpiresIn, 'minutes')
.unix();
const payload: VerificationJwtPayload = {
sub: 'Verification',
code: verificationCode,
iat: now,
exp,
iss: this.configService.get<string>('wallet.publicKey'),
};
return this.signPayload(payload);
}Committable suggestion skipped: line range outside the PR's diff.
| async validateToken(token: string): Promise<JwtPayload> { | ||
| try { | ||
| return jwt.verify( | ||
| token, | ||
| this.configService.get<string>('jwt.secret'), | ||
| { | ||
| algorithms: ['HS256'], | ||
| } | ||
| ) as JwtPayload; | ||
| } catch (error) { | ||
| this.logger.error(error, 'Failed to validate token'); | ||
| throw new UnauthorizedException(error.message); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Ensure 'jwt.secret' is defined and handle missing secret appropriately
In the validateToken method, if jwt.secret is undefined, token verification may fail unexpectedly. Add a check to ensure the secret is defined before verification.
Apply this diff to add a check:
async validateToken(token: string): Promise<JwtPayload> {
try {
+ const secret = this.configService.get<string>('jwt.secret');
+ if (!secret) {
+ this.logger.error('JWT secret is not defined in the configuration');
+ throw new UnauthorizedException('JWT secret is not configured');
+ }
return jwt.verify(
token,
secret,
{
algorithms: ['HS256'],
}
) as JwtPayload;
} catch (error) {
this.logger.error(error, 'Failed to validate token');
throw new UnauthorizedException(error.message);
}
}📝 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.
| async validateToken(token: string): Promise<JwtPayload> { | |
| try { | |
| return jwt.verify( | |
| token, | |
| this.configService.get<string>('jwt.secret'), | |
| { | |
| algorithms: ['HS256'], | |
| } | |
| ) as JwtPayload; | |
| } catch (error) { | |
| this.logger.error(error, 'Failed to validate token'); | |
| throw new UnauthorizedException(error.message); | |
| } | |
| } | |
| async validateToken(token: string): Promise<JwtPayload> { | |
| try { | |
| const secret = this.configService.get<string>('jwt.secret'); | |
| if (!secret) { | |
| this.logger.error('JWT secret is not defined in the configuration'); | |
| throw new UnauthorizedException('JWT secret is not configured'); | |
| } | |
| return jwt.verify( | |
| token, | |
| secret, | |
| { | |
| algorithms: ['HS256'], | |
| } | |
| ) as JwtPayload; | |
| } catch (error) { | |
| this.logger.error(error, 'Failed to validate token'); | |
| throw new UnauthorizedException(error.message); | |
| } | |
| } |
| it('should be defined', () => { | ||
| expect(controller).toBeDefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Add test coverage for controller endpoints
Current test coverage only verifies controller instantiation. Add tests for:
- POST /token endpoint:
- Successful token generation with valid GenerateVerificationTokenDto
- Error handling for invalid DTO
- POST /verify endpoint:
- Successful verification with valid VerifyDto
- Error handling for invalid SIWE verification
All necessary mocks (HttpService, JwtService, CryptoUtilsService) are already set up in the test file, making it straightforward to add these test cases.
🔗 Analysis chain
Add test coverage for controller endpoints
Current test coverage only verifies controller instantiation. Add tests for:
- Token generation endpoint:
- Successful token generation
- Error handling
- Verify endpoint:
- Successful verification
- Error handling
Here's a script to check current test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for discourse verification endpoints
rg -A 5 "describe\('.*token'" src/discourse-verification/
rg -A 5 "describe\('.*verify'" src/discourse-verification/
Length of output: 132
Script:
#!/bin/bash
# Find all test files in discourse-verification
echo "=== Test files ==="
fd "\.spec\.(ts|js)$" src/discourse-verification/
# Find controller implementation
echo -e "\n=== Controller implementation ==="
ast-grep --pattern 'export class DiscourseVerificationController {
$$$
}'
Length of output: 394
Script:
#!/bin/bash
# Check controller implementation
echo "=== Controller Implementation ==="
rg -A 10 "@Controller" src/discourse-verification/discourse-verification.controller.ts
# Check current test content
echo -e "\n=== Controller Test Content ==="
cat src/discourse-verification/discourse-verification.controller.spec.ts
echo -e "\n=== Service Test Content ==="
cat src/discourse-verification/discourse-verification.service.spec.ts
Length of output: 4824
Script:
#!/bin/bash
# Get complete controller implementation
echo "=== Complete Controller Implementation ==="
cat src/discourse-verification/discourse-verification.controller.ts
Length of output: 2360
| it('should be defined', () => { | ||
| expect(service).toBeDefined(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Enhance test coverage for service methods
Current test coverage only verifies service instantiation. Based on the implementation, add tests for:
-
generateVerificationToken:- Success case with valid SIWE JWT
- Invalid SIWE JWT handling
- Verify verification code generation
- Verify JWT token generation
-
verifyDiscourseTopic:- Success case with valid topic and verification code
- URL normalization (with/without .json suffix)
- Error cases:
- Invalid topic URL
- Failed HTTP request
- Missing posts
- Missing user ID
- Verification code not found in content
- Invalid verification JWT
The test file already has the necessary mocks set up for HttpService, JwtService, and CryptoUtilsService, making it straightforward to add these test cases.
🔗 Analysis chain
Enhance test coverage for service methods
Current test coverage only verifies service instantiation. Add tests for:
generateVerificationToken:- Success case
- Invalid input handling
verifyDiscourseTopic:- Success case
- Invalid state
- HTTP error handling
Here's a script to check current test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for discourse verification service
rg -A 5 "describe\('.*generateVerificationToken'" src/discourse-verification/
rg -A 5 "describe\('.*verifyDiscourseTopic'" src/discourse-verification/
Length of output: 166
Script:
#!/bin/bash
# Verify service implementation
echo "=== Service Implementation ==="
ast-grep --pattern 'class DiscourseVerificationService {
$$$
generateVerificationToken($$$) {
$$$
}
$$$
}'
echo -e "\n=== Method Tests ==="
# Search for any tests containing these method names
rg -l "generateVerificationToken|verifyDiscourseTopic" src/discourse-verification/
# Get full test file content for analysis
echo -e "\n=== Test File Content ==="
cat src/discourse-verification/discourse-verification.service.spec.ts
Length of output: 2527
Script:
#!/bin/bash
# Get service implementation
echo "=== Service Implementation ==="
cat src/discourse-verification/discourse-verification.service.ts
# Search for method usage in controller
echo -e "\n=== Controller Usage ==="
rg -A 5 "generateVerificationToken|verifyDiscourseTopic" src/discourse-verification/discourse-verification.controller.ts
Length of output: 3992
| const jwt = await this.jwtService.generateAuthJwt( | ||
| userInfo.id, | ||
| provider | ||
| ); | ||
| const frontendUrl = this.configService.get<string>('app.frontEndURL'); | ||
| const params = querystring.stringify({ jwt }); | ||
| return `${frontendUrl}/callback?${params}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider adding error handling for JWT generation.
The JWT generation call should be wrapped in a try-catch block to handle potential errors gracefully.
- const jwt = await this.jwtService.generateAuthJwt(
- userInfo.id,
- provider
- );
- const frontendUrl = this.configService.get<string>('app.frontEndURL');
- const params = querystring.stringify({ jwt });
- return `${frontendUrl}/callback?${params}`;
+ try {
+ const jwt = await this.jwtService.generateAuthJwt(
+ userInfo.id,
+ provider
+ );
+ const frontendUrl = this.configService.get<string>('app.frontEndURL');
+ const params = querystring.stringify({ jwt });
+ return `${frontendUrl}/callback?${params}`;
+ } catch (error) {
+ this.logger.error(error, 'Failed to generate JWT');
+ throw new InternalServerErrorException('Failed to generate JWT');
+ }Committable suggestion skipped: line range outside the PR's diff.
| const decodedData = this.decodeAttestationData(attestation.data); | ||
| const secret = decodedData[2].value.value; | ||
| return await this.litService.decryptFromJson(chainId, secret); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add error handling for decryption operation.
The decryption operation should be wrapped in a try-catch block to handle potential errors.
- const decodedData = this.decodeAttestationData(attestation.data);
- const secret = decodedData[2].value.value;
- return await this.litService.decryptFromJson(chainId, secret);
+ try {
+ const decodedData = this.decodeAttestationData(attestation.data);
+ const secret = decodedData[2].value.value;
+ return await this.litService.decryptFromJson(chainId, secret);
+ } catch (error) {
+ this.logger.error(error, 'Failed to decrypt attestation secret');
+ throw new InternalServerErrorException('Failed to decrypt attestation secret');
+ }Committable suggestion skipped: line range outside the PR's diff.
| async verifySiweMessage(message: string, signature: Hex, chainId: number) { | ||
| try { | ||
| const publicClient = this.viemUtilsService.getPublicClient(chainId) | ||
| const publicClient = this.viemUtilsService.getPublicClient(chainId); | ||
| const isValid = await publicClient.verifySiweMessage({ | ||
| message, | ||
| signature, | ||
| }) | ||
| }); | ||
| if (!isValid) { | ||
| throw new Error() | ||
| throw new Error(); | ||
| } | ||
| } catch (error) { | ||
| this.logger.error(error, `Siwe Verification Failed`) | ||
| this.logger.error(error, `Siwe Verification Failed`); | ||
| throw new HttpException( | ||
| `${AUTH_PROVIDERS.SIWE} verification Failed`, | ||
| HttpStatus.BAD_REQUEST | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance error handling and type safety.
The error handling could be more specific and informative. Consider these improvements:
- async verifySiweMessage(message: string, signature: Hex, chainId: number) {
+ async verifySiweMessage(
+ message: string,
+ signature: Hex,
+ chainId: number
+ ): Promise<void> {
+ if (!message || !signature || !chainId) {
+ throw new HttpException(
+ 'Missing required parameters for SIWE verification',
+ HttpStatus.BAD_REQUEST
+ );
+ }
try {
const publicClient = this.viemUtilsService.getPublicClient(chainId);
const isValid = await publicClient.verifySiweMessage({
message,
signature,
});
if (!isValid) {
- throw new Error();
+ throw new Error('SIWE message verification returned false');
}
} catch (error) {
- this.logger.error(error, `Siwe Verification Failed`);
+ const errorMessage = error instanceof Error ? error.message : 'Unknown error';
+ this.logger.error(
+ { error, chainId, messageLength: message.length },
+ `SIWE Verification Failed: ${errorMessage}`
+ );
throw new HttpException(
- `${AUTH_PROVIDERS.SIWE} verification Failed`,
+ `${AUTH_PROVIDERS.SIWE} verification failed: ${errorMessage}`,
HttpStatus.BAD_REQUEST
);
}
}Additionally, consider adding JSDoc documentation to describe the method's purpose and parameters.
/**
* Verifies a SIWE (Sign-In with Ethereum) message signature.
* @param message - The SIWE message to verify
* @param signature - The Ethereum signature (hex string)
* @param chainId - The blockchain network ID
* @throws HttpException if verification fails or parameters are invalid
*/
Summary by CodeRabbit
Based on the comprehensive summary, here are the high-level release notes:
New Features
Improvements
Changes
AuthServiceAuthServicewithJwtServicein multiple modulesTechnical Updates