Conversation
* feat(auth): add OAuth authentication system with plugin support - Add OAuth module with dynamic provider loading mechanism - Implement OAuth service with plugin and npm package loading support - Add OAuth types and base provider class for extensibility - Create user OAuth connection model for account linking - Add OAuth routes: providers list, login redirect, callback handling - Implement user synchronization logic (existing user binding, email matching, new user creation) - Add comprehensive unit tests for OAuth service and user login methods - Add end-to-end tests covering full OAuth flow and error scenarios - Include example OAuth provider implementation and documentation - Update environment configuration with OAuth settings 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(auth): resolve TypeScript compilation errors in OAuth implementation - Fix unknown error type handling in OAuth service methods - Update controller method return types from Response to void - Add proper type guards for error instanceof Error checks - Ensure consistent error handling patterns 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * fix(tests): resolve OAuth test failures and type errors - Fix cookie handling in e2e tests with proper array type checking - Update OAuth service test mocks to use correct environment variable names - Fix nullable field handling in OAuthUserInfo type definitions - Update test expectations to match actual OAuth service method signatures - Resolve private method access issues in users service tests * feat(migrations): add user_o_auth_connection table and related indexes * test(oauth): improve unit test coverage and add comprehensive test suites * refactor(users): modularize OAuth connection handling * test(oauth): improve unit test coverage --------- Co-authored-by: Claude <noreply@anthropic.com>
WalkthroughThis update introduces a modular OAuth authentication system to the backend. It adds a new OAuth module, service, provider interface, DTOs, and comprehensive documentation. The user service and controller are extended to support OAuth login, user binding, and registration. Corresponding database models, migrations, environment variables, and thorough unit and integration tests are included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant OAuthProvider
User->>Frontend: Click "Login with OAuth"
Frontend->>Backend: GET /users/auth/oauth/login/:providerId
Backend->>OAuthProvider: Redirect to provider's authorization URL
User->>OAuthProvider: Authorize and grant access
OAuthProvider->>Backend: Redirect with code (callback)
Backend->>OAuthProvider: Exchange code for access token
Backend->>OAuthProvider: Fetch user info
Backend->>Backend: Find or create/bind user
Backend->>Frontend: Redirect with JWT token, user info
Frontend->>User: User is logged in
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (12)
sample.env (1)
90-108: Comprehensive OAuth configuration with security considerationsThe OAuth configuration is well-structured and comprehensive. However, consider these security improvements:
- Sensitive credentials exposure: The example shows credentials in plain text
- Plugin security: Loading from npm packages could introduce security risks
Consider adding security warnings in the comments:
# OAuth provider credentials (example for 'ruc' provider) # Replace 'RUC' with your provider ID in uppercase +# WARNING: Keep these credentials secure and never commit them to version control # OAUTH_RUC_CLIENT_ID=your-client-id # OAUTH_RUC_CLIENT_SECRET=your-client-secret # OAUTH_RUC_REDIRECT_URL=http://localhost:3000/users/auth/oauth/callback/ruc # Allow loading OAuth providers from npm packages +# WARNING: Only enable this for trusted npm packages to avoid security risks # OAUTH_ALLOW_NPM_LOADING=falsesrc/users/DTO/oauth.dto.ts (1)
22-37: Comprehensive OAuth callback validationThe
OAuthCallbackQueryDtoincludes all standard OAuth callback parameters with proper validation. However, consider adding validation for thecodeparameter length to prevent excessively long values.export class OAuthCallbackQueryDto { @IsString() + @Length(1, 2048) // Reasonable limit for OAuth authorization codes code: string;This adds protection against potential DoS attacks through extremely long authorization codes.
src/auth/oauth/oauth.types.spec.ts (1)
17-19: Remove unnecessary constructor.The constructor only calls
superwithout any additional logic. TypeScript will automatically provide this behavior.Apply this diff to remove the unnecessary constructor:
// Mock implementation for testing BaseOAuthProvider class TestOAuthProvider extends BaseOAuthProvider { - constructor(config: OAuthProviderConfig) { - super(config); - } - async handleCallback(code: string, state?: string): Promise<string> {src/auth/oauth/oauth.service.ts (1)
89-119: Use English for code comments for consistency.The codebase should maintain English comments for international collaboration.
Apply this diff to translate the Chinese comments:
): Promise<void> { - // 检查是否有必要的配置 + // Check if necessary configuration exists const config = this.getProviderConfig(providerId); if (!config) { this.logger.warn( `Missing configuration for OAuth provider '${providerId}', skipping`, ); return; } - // 尝试从插件路径加载 + // Try to load from plugin paths let provider: OAuthProvider | null = null; for (const pluginPath of pluginPaths) { provider = await this.tryLoadFromPath(providerId, pluginPath, config); if (provider) { break; } } - // 如果本地未找到且允许 npm 加载，尝试从 npm 包加载 + // If not found locally and npm loading is allowed, try to load from npm package if (!provider && allowNpmLoading) { provider = await this.tryLoadFromNpm(providerId, config); }test/user.e2e-spec.ts (2)
1395-1396: Consider removing debug console.log statements.Debug logging statements should be removed from production test code to keep test output clean.
- // Add debug logging to verify email matching - console.log(`Test: regularUser email = "${regularUser.email}"`); - console.log(`Test: regularUser userId = ${regularUser.userId}`);
1412-1415: Remove debug console.log statement.This debug logging should be removed.
- console.log( - `Mock: OAuth getUserInfo returning email = "${userInfo.email}"`, - ); return userInfo;src/auth/oauth/oauth.types.ts (1)
8-14: Consider documenting the purpose of dual username fields.The interface has both
usernameandpreferredUsernamefields. Consider adding JSDoc comments to clarify when each should be used.export interface OAuthUserInfo { id: string; email?: string; name?: string; + /** The actual username from the OAuth provider */ username?: string; + /** The user's preferred display username */ preferredUsername?: string; }docs/oauth-implementation-guide.md (1)
38-39: Consider adding security warning about npm package loading.Since
OAUTH_ALLOW_NPM_LOADINGcan be a security risk if enabled in production, consider adding a warning.# 是否允许从 npm 包加载提供商 +# ⚠️ 警告：在生产环境中启用此选项可能存在安全风险 OAUTH_ALLOW_NPM_LOADING=falsesrc/users/users.service.ts (3)
754-758: Use optional chaining for cleaner code.As suggested by static analysis, this can be simplified.
- const email = - user.email && user.email.endsWith('@placeholder.internal') - ? null - : user.email; + const email = user.email?.endsWith('@placeholder.internal') + ? null + : user.email;
1718-1720: Use optional chaining for profile check.As suggested by static analysis, this can be simplified.
- if (!existingConnection.user.userProfile) { + if (!existingConnection.user?.userProfile) {
1854-1858: Consider validating nickname length earlier.While truncating to 255 characters works, consider validating this limit in the OAuth user info to provide better error messages.
You might want to add validation in the OAuthUserInfo interface or document this length limitation.
docs/oauth.md (1)
187-187: Minor grammar improvement for clarity.The sentence structure could be clearer. Consider rephrasing for better readability.
Apply this diff to improve the sentence:
-当然，这需要权衡：把实现代码纳入仓库便于版本管理，但需确保不包含敏感信息（比如具体的学校OAuth端点可能是公开协议，无妨）。 +当然，这需要权衡：将实现代码纳入仓库虽然便于版本管理，但需确保不包含敏感信息（比如具体的学校OAuth端点可能是公开协议，无妨）。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yamlprisma/migrations/migration_lock.tomlis excluded by!**/*.tomltsconfig.build.jsonis excluded by!**/*.json
📒 Files selected for processing (20)
.gitignore(1 hunks)docs/oauth-implementation-guide.md(1 hunks)docs/oauth.md(1 hunks)prisma/migrations/20250625052726_add_oauth_tables/migration.sql(1 hunks)prisma/schema.prisma(2 hunks)sample.env(1 hunks)src/auth/auth.module.ts(2 hunks)src/auth/oauth/oauth.module.spec.ts(1 hunks)src/auth/oauth/oauth.module.ts(1 hunks)src/auth/oauth/oauth.prisma(1 hunks)src/auth/oauth/oauth.service.spec.ts(1 hunks)src/auth/oauth/oauth.service.ts(1 hunks)src/auth/oauth/oauth.types.spec.ts(1 hunks)src/auth/oauth/oauth.types.ts(1 hunks)src/users/DTO/oauth.dto.ts(1 hunks)src/users/users.controller.ts(6 hunks)src/users/users.prisma(2 hunks)src/users/users.service.spec.ts(1 hunks)src/users/users.service.ts(4 hunks)test/user.e2e-spec.ts(7 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/auth/oauth/oauth.service.spec.ts
[error] 887-887: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/auth/oauth/oauth.types.spec.ts
[error] 17-19: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
src/users/users.service.ts
[error] 759-764: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 1713-1716: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 LanguageTool
docs/oauth.md
[uncategorized] ~91-~91: 您的意思是“"不"建”？
Context: ...录存在且用户存在，但发现用户缺少用户档案（profile）（理论上不应发生），则补建默认档案以保持数据一致性。 2. 按邮箱匹配现有用户： 如果没有现成...
(BU)
[uncategorized] ~95-~95: 您的意思是“"账"号”？
Context: ...，update 分支会更新绑定到当前用户ID。这一操作将日志记录输出，表示发生了帐号关联。 * 之后复用找到的本地用户记录，跳至步骤4。 3. **创建新...
(ZHANG7_ZHANG8)
[uncategorized] ~100-~100: 数词与名词之间一般应存在量词，可能缺少量词。
Context: ...ID}`。规范化该用户名：去除特殊字符、转为小写，并确保长度在合适范围内（如不足4字符则补前缀，超长则截断）。然后检查是否与现有用户重名，若是则在末尾追加递增数字后缀...
(wa5)
[uncategorized] ~100-~100: 您的意思是“"不"前缀”？
Context: ...。规范化该用户名：去除特殊字符、转为小写，并确保长度在合适范围内（如不足4字符则补前缀，超长则截断）。然后检查是否与现有用户重名，若是则在末尾追加递增数字后缀确保...
(BU)
[uncategorized] ~134-~134: 动词的修饰一般为‘形容词（副词）+地+动词’。您的意思是否是：相同"地"会话
Context: ...in 方法所做的记录和 token 发放逻辑），将 OAuth 登录结果接入相同的会话管理流程，实现统一的用户体验。 ## 模块集成与兼容性 有了上述组件，实...
(wb4)
[uncategorized] ~147-~147: 您的意思是“"既"是”吗？
Context: ...sController 构造函数即可通过 DI 拿到它。Cheese-Auth 即是在 UsersController 中注入了 OAuthService，因此我...
(JI11_JI2)
[uncategorized] ~147-~147: 动词的修饰一般为‘形容词（副词）+地+动词’。您的意思是否是：相同"地"依赖
Context: ...ersController 中注入了 OAuthService，因此我们保持相同的依赖关系即可。 * 路由集成： 将上文列出的 `/users/auth...
(wb4)
[uncategorized] ~155-~155: 您的意思是“"不"验证”？
Context: ...ontroller 增加调用它的入口。因此，原有邮箱验证、密码重置、TOTP 两步验证等功能都与 OAuth 登录互不冲突，各自按需执行。 * **前端配合：...
(BU)
[uncategorized] ~187-~187: 能愿动词不能成为‘把’字句、‘被’字句的谓语动词。应该是:"便于把……实现"。
Context: ...现作为私有文件部署（通过文档指导运维放置），而不直接提交代码。当然，这需要权衡：把实现代码纳入仓库便于版本管理，但需确保不包含敏感信息（比如具体的学校OAuth端点可能是公开协议，无...
(wa3)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: test (16, 8.0.0, 20.x)
- GitHub Check: test (latest, 8.12.2, 22.x)
- GitHub Check: test (16, 8.12.2, 20.x)
- GitHub Check: test (16, 8.12.2, 22.x)
- GitHub Check: test (latest, 8.0.0, 20.x)
- GitHub Check: test (latest, 8.12.2, 20.x)
- GitHub Check: test (16, 8.0.0, 22.x)
- GitHub Check: test (latest, 8.0.0, 22.x)
- GitHub Check: test (latest, 8.12.2, 22.x)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (30)
sample.env (1)
88-88: LGTM: OAuth integration alignmentUncommenting
DISABLE_EMAIL_VERIFICATIONmakes sense for OAuth flows where email verification might be handled by the OAuth provider.src/auth/auth.module.ts (1)
16-16: LGTM: Proper NestJS module integrationThe OAuth module integration follows correct NestJS patterns:
- Clean import statement
- Dynamic module registration with
.register()- Proper re-export for downstream modules
The integration maintains module encapsulation while making OAuth functionality available to the auth system.
Also applies to: 34-34, 38-38
.gitignore (1)
49-53: LGTM: Appropriate security exclusionsThe gitignore additions properly protect sensitive OAuth data:
oauth-providers/andplugins/directories contain sensitive provider configurations- The clear comment explains the security rationale
uploads/is a standard exclusion for runtime dataThese exclusions align with OAuth security best practices.
src/users/users.prisma (1)
12-12: LGTM: Proper OAuth relationship establishmentThe database schema changes correctly establish the OAuth functionality:
- Clean import of
UserOAuthConnectionmodel- Proper one-to-many relationship (
oauthConnections UserOAuthConnection[])- Follows Prisma conventions and integrates well with existing User model
This enables users to have multiple OAuth provider connections as expected.
Also applies to: 58-58
src/users/DTO/oauth.dto.ts (2)
12-20: LGTM: Well-structured provider response DTOThe
GetOAuthProvidersResponseDtoproperly extends the base response pattern and includes all necessary provider information (id, name, scope). The structure aligns with typical OAuth provider discovery patterns.
39-42: Clean extension of existing UserDtoThe
OAuthUserDtoproperly extends the existingUserDtowith the optional email field, which is appropriate for OAuth flows where email might not always be available or verified.The nullable email type (
string | null) correctly handles OAuth providers that might not provide email information.src/auth/oauth/oauth.module.ts (1)
1-40: LGTM! Well-structured OAuth module implementation.The module follows NestJS best practices with proper dynamic module registration, lifecycle management, and service export. The indexed provider token strategy (
OAUTH_PROVIDER_${index}) is a clean approach for supporting multiple OAuth providers dynamically.Key strengths:
- Proper implementation of
OnModuleInitfor service initialization- Clean dynamic module registration with configurable providers
- Appropriate exports for module integration
- Consistent import/export structure
src/auth/oauth/oauth.module.spec.ts (1)
1-195: Excellent test coverage with comprehensive scenarios.The test suite thoroughly covers all aspects of the OAuth module including:
- Module initialization and service provision
- Lifecycle hook execution with proper error handling
- Dynamic module registration with multiple providers
- Edge cases (empty providers, no providers)
- Module configuration validation
The mock OAuth providers are well-structured and the setup/teardown properly manages module lifecycle. This provides strong confidence in the module's robustness.
prisma/migrations/20250625052726_add_oauth_tables/migration.sql (1)
1-24: Well-designed database migration for OAuth functionality.The migration properly establishes the OAuth connection table with:
- Appropriate data types (JSONB for profiles, TIMESTAMPTZ for timestamps)
- Foreign key with CASCADE for data integrity
- Unique constraint preventing duplicate provider connections
- Performance indexes for efficient querying
The table structure supports the core OAuth requirements while maintaining referential integrity.
src/auth/oauth/oauth.prisma (1)
1-19: Solid Prisma model design for OAuth connections.The model properly defines the OAuth connection structure with:
- Correct field mappings matching the database migration
- Appropriate relation to User model with cascade delete
- Unique constraint on
(providerId, providerUserId)preventing duplicates- Performance index on
userIdfor efficient user-based queries- Flexible JSON storage for raw OAuth profiles
- Optional fields supporting various OAuth token lifecycles
The model aligns well with the database migration and OAuth requirements.
prisma/schema.prisma (2)
195-216: Clean integration of OAuth model into main schema.The OAuth model is properly integrated with consistent field definitions matching the separate model file. The structure follows existing schema conventions and maintains proper relationships.
677-677: Proper User model relation addition.The
oauthConnectionsrelation field correctly establishes the one-to-many relationship between users and OAuth connections, enabling efficient querying of user OAuth associations.src/auth/oauth/oauth.types.spec.ts (1)
42-245: Excellent test coverage!The test suite provides comprehensive coverage for OAuth types including:
- All BaseOAuthProvider methods with various parameter combinations
- Edge cases like empty scope arrays and special characters in URLs
- Complete OAuthError class testing including inheritance verification
src/users/users.service.spec.ts (1)
1-732: Excellent OAuth integration test coverage!The test suite thoroughly covers the
loginWithOAuthmethod with comprehensive scenarios including:
- Existing user authentication flows
- New user registration with proper profile creation
- Edge cases (missing email, deleted users, special characters)
- Transaction error handling
- Proper mocking of all dependencies
src/auth/oauth/oauth.service.spec.ts (1)
1-1591: Outstanding test coverage for OAuth service!This test suite is exceptionally comprehensive, covering:
- Provider initialization and loading from multiple sources
- Security validations (path traversal, XSS attempts)
- NPM package loading scenarios
- Error handling at all levels
- Edge cases and configuration parsing
- Provider method error handling
The security testing in particular is excellent, validating against various attack vectors.
src/auth/oauth/oauth.service.ts (1)
149-195: Excellent security implementation!The path traversal security check (lines 165-172) is well-implemented, ensuring that resolved paths remain within the expected base directory. This prevents malicious provider IDs from accessing files outside the plugin directory.
test/user.e2e-spec.ts (4)
16-18: LGTM! OAuth imports added correctly.The OAuth-related imports are properly placed and necessary for the new OAuth authentication tests.
183-183: Good fix for getting the last verification code.Using
slice(-1)[0]ensures you get the most recent verification code even when multiple calls have been made, which is more reliable than using index[0].
225-229: Correct placement of environment variables.Setting JWT secret and OAuth-related environment variables before module creation ensures they are available during application initialization. This is the right approach.
1212-1545: Comprehensive OAuth authentication test suite!The test suite thoroughly covers OAuth authentication scenarios including:
- Provider configuration retrieval
- OAuth login redirection
- Error handling for invalid providers and callbacks
- User binding by email
- Handling existing OAuth connections
- Creating new users without email
The mocking strategy and test structure are well-designed.
src/auth/oauth/oauth.types.ts (1)
1-82: Well-structured OAuth type definitions!The OAuth types and interfaces are properly designed:
- Clear separation of concerns with distinct interfaces
- Good use of abstract class for common OAuth provider functionality
- Proper error class with categorized error types
- All necessary OAuth flow parameters are included
docs/oauth-implementation-guide.md (1)
1-286: Excellent OAuth implementation documentation!The guide is comprehensive and well-organized, covering:
- Clear architectural overview
- Step-by-step implementation instructions
- Security best practices
- Practical examples for both JavaScript and TypeScript
- Troubleshooting guidance
This will be very helpful for developers implementing OAuth providers.
src/users/users.service.ts (5)
44-44: OAuth imports added correctly.The OAuth type imports are properly placed and necessary for the OAuth functionality.
Also applies to: 54-54
741-764: Good addition of OAuth-specific user DTO method.The method correctly extends the base user DTO with email information while handling placeholder emails appropriately.
1642-1707: Well-structured OAuth login implementation!The
loginWithOAuthmethod properly handles all OAuth login scenarios:
- Existing OAuth connections
- Email-based user matching
- New user creation
The logging and error handling are appropriate.
1836-1841: Good approach for handling users without email.Using a placeholder email format
oauth-{providerId}-{providerUserId}@placeholder.internalis a clever solution for OAuth providers that don't provide email addresses.
1914-1941: Robust username generation logic.The method handles various scenarios well:
- Prefers preferredUsername over username
- Cleans up invalid characters
- Falls back to a default format
Good defensive programming!
src/users/users.controller.ts (3)
20-20: LGTM! OAuth module integration looks clean.The imports, logger initialization, and service injection follow NestJS best practices. The OAuth service is properly injected through the constructor for dependency injection.
Also applies to: 43-44, 66-69, 137-138, 150-150
1219-1231: Clean implementation for listing OAuth providers.The method correctly exposes available OAuth providers as a public endpoint. Good use of the service layer for business logic.
1232-1259: Well-structured OAuth login flow with proper error handling.The method correctly generates authorization URLs and handles errors by redirecting to the frontend error page. The non-null assertion on
res!is safe in this context since NestJS always provides the Response object.
| return { | ||
| id: providerId, | ||
| name: providerId, // 可以通过配置覆盖显示名称 | ||
| clientId, | ||
| clientSecret, | ||
| authorizationUrl: '', // 由具体实现提供 | ||
| tokenUrl: '', // 由具体实现提供 | ||
| redirectUrl, | ||
| scope: [], // 由具体实现提供 | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix comment language and reconsider empty URL configuration.
Two issues to address:
- Chinese comment should be in English
- Setting
authorizationUrlandtokenUrlto empty strings could cause issues
Apply this diff:
return {
id: providerId,
- name: providerId, // 可以通过配置覆盖显示名称
+ name: providerId, // Can be overridden via configuration
clientId,
clientSecret,
- authorizationUrl: '', // 由具体实现提供
- tokenUrl: '', // 由具体实现提供
+ authorizationUrl: '', // TODO: Consider making these optional in the config type
+ tokenUrl: '', // TODO: Consider making these optional in the config type
redirectUrl,
- scope: [], // 由具体实现提供
+ scope: [], // Provided by concrete implementation
};Consider updating the OAuthProviderConfig type to make authorizationUrl and tokenUrl optional since they're provided by the concrete implementation.
📝 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.
| return { | |
| id: providerId, | |
| name: providerId, // 可以通过配置覆盖显示名称 | |
| clientId, | |
| clientSecret, | |
| authorizationUrl: '', // 由具体实现提供 | |
| tokenUrl: '', // 由具体实现提供 | |
| redirectUrl, | |
| scope: [], // 由具体实现提供 | |
| }; | |
| } | |
| return { | |
| id: providerId, | |
| name: providerId, // Can be overridden via configuration | |
| clientId, | |
| clientSecret, | |
| authorizationUrl: '', // TODO: Consider making these optional in the config type | |
| tokenUrl: '', // TODO: Consider making these optional in the config type | |
| redirectUrl, | |
| scope: [], // Provided by concrete implementation | |
| }; | |
| } |
🤖 Prompt for AI Agents
In src/auth/oauth/oauth.service.ts around lines 137 to 147, replace the Chinese
comment with English and avoid setting authorizationUrl and tokenUrl to empty
strings as this may cause issues. Instead, update the OAuthProviderConfig type
to make authorizationUrl and tokenUrl optional, and remove these properties or
set them to undefined here, allowing concrete implementations to provide them.
| const successPath = | ||
| this.configService.get('FRONTEND_OAUTH_SUCCESS_PATH') || | ||
| '/oauth-success'; | ||
| const frontendUrl = `${frontendBaseUrl}${successPath}?token=${encodeURIComponent(jwtAccessToken)}&email=${encodeURIComponent(userDto.email || userDto.username)}&provider=${providerId}`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Security concern: Avoid passing JWT tokens in URL parameters.
Passing sensitive tokens in URLs can lead to security issues as they may be logged in server logs, stored in browser history, or leaked through referrer headers. Consider alternative approaches like:
- Setting the token in a secure, httpOnly cookie
- Using a temporary session-based approach
- Implementing a token exchange endpoint that the frontend can call
🤖 Prompt for AI Agents
In src/users/users.controller.ts at line 1318, avoid passing the JWT access
token in the URL query parameters due to security risks. Instead, modify the
code to send the token via a secure, httpOnly cookie or implement a backend
endpoint that the frontend can call to retrieve the token after authentication.
Remove the token from the URL construction and ensure the frontend receives the
token through a safer channel like cookies or a token exchange API.
| sameSite: 'lax', // 允许第三方跳转携带 | ||
| path: path.posix.join(cookieBasePath, 'users/auth'), | ||
| expires: newRefreshTokenExpire, | ||
| secure: process.env.NODE_ENV === 'production', // 生产环境使用 HTTPS |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance cookie security settings for better protection.
Two security improvements to consider:
- Use
sameSite: 'strict'instead of 'lax' for better CSRF protection, unless cross-site requests are specifically needed - Enable the
secureflag in all HTTPS environments (staging, UAT), not just production
Apply this diff to improve security:
- sameSite: 'lax', // 允许第三方跳转携带
+ sameSite: 'strict', // Better CSRF protection
path: path.posix.join(cookieBasePath, 'users/auth'),
expires: newRefreshTokenExpire,
- secure: process.env.NODE_ENV === 'production', // 生产环境使用 HTTPS
+ secure: this.configService.get('USE_HTTPS') === 'true' || process.env.NODE_ENV === 'production',Note: If cross-origin OAuth redirects require the cookie, you may need to keep 'lax', but document this security trade-off.
📝 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.
| sameSite: 'lax', // 允许第三方跳转携带 | |
| path: path.posix.join(cookieBasePath, 'users/auth'), | |
| expires: newRefreshTokenExpire, | |
| secure: process.env.NODE_ENV === 'production', // 生产环境使用 HTTPS | |
| sameSite: 'strict', // Better CSRF protection | |
| path: path.posix.join(cookieBasePath, 'users/auth'), | |
| expires: newRefreshTokenExpire, | |
| secure: this.configService.get('USE_HTTPS') === 'true' || process.env.NODE_ENV === 'production', |
🤖 Prompt for AI Agents
In src/users/users.controller.ts around lines 1324 to 1327, update the cookie
settings to enhance security by changing sameSite from 'lax' to 'strict' unless
cross-site requests are required, and modify the secure flag to be true in all
HTTPS environments including staging and UAT, not just production. Adjust the
environment check accordingly and document any exceptions if cross-origin OAuth
redirects need 'lax'.
1 participant
feat(auth): add OAuth authentication system with plugin support
fix(auth): resolve TypeScript compilation errors in OAuth implementation
fix(tests): resolve OAuth test failures and type errors
feat(migrations): add user_o_auth_connection table and related indexes
test(oauth): improve unit test coverage and add comprehensive test suites
refactor(users): modularize OAuth connection handling
test(oauth): improve unit test coverage