fix: 使用GitHub数字ID代替可变的login作为唯一标识 - #2609
Conversation
fix: root page does not have analytic code
feat: add claude-opus-4-5-20251101
…ini-integration-011nJGemhrPUdqwg3qDvmqVB feat: enable thoughtSignature for non-function-call messages
fix: volcengine && baidu claude adapter
fix: volcengine claude DoResponse
fix: volcengine claude DoResponse
…pro-image-preview-oai OAI生图接口支持gemini 3 pro image preview
…ageConfig fix: gemini image correct generationConfig
…ith i18n - Add SSEViewer component for interactive SSE message inspection * Display SSE data stream with collapsible panels * Show parsed JSON with syntax highlighting * Display key information badges (content, tokens, finish reason) * Support copy individual or all SSE messages * Show error messages with detailed information - Support Ctrl+V to paste images in chat input * Enable image paste in CustomInputRender component * Auto-detect and add pasted images to image list * Show toast notifications for paste results - Add complete i18n support for 6 languages * Chinese (zh): Complete translations * English (en): Complete translations * Japanese (ja): Add 28 new translations * French (fr): Add 28 new translations * Russian (ru): Add 28 new translations * Vietnamese (vi): Add 32 new translations - Update .gitignore to exclude data directory
…-i2v Gemini Veo3.1[AI Studio]增加图生视频支持
Ensure image file is closed using defer after opening.
…edit Gemini Image系列支持图像编辑
…d-oai feat: 视频下载和界面预览统一使用OAI标准接口
fix: fix the proxyURL is empty, not using the default HTTP client configuration && the AWS calling side did not apply the relay timeout.
fix: add tips for model management and channel testing
问题描述： - 使用 auto 分组的令牌调用 /v1/videos 等 Task 接口时，虽然任务能成功创建， 但使用日志不显示记录，且不会扣费 根本原因： - Distribute 中间件在选择渠道后，会将实际选中的分组存储在 ContextKeyAutoGroup 中 - 但 RelayTaskSubmit 函数没有从 context 中读取这个值来更新 info.UsingGroup - 导致 info.UsingGroup 始终是 "auto" 而不是实际选中的分组（如 "sora2逆"） - 当 auto 分组的倍率配置为 0 时，quota 计算结果为 0 - 日志记录条件 "if quota != 0" 不满足，导致日志不记录、不扣费 修复方案： - 在 RelayTaskSubmit 函数中计算分组倍率之前，添加从 ContextKeyAutoGroup 获取实际分组的逻辑 - 使用安全的类型断言，避免潜在的 panic 风险 影响范围： - 仅影响 Task Relay 流程（/v1/videos, /suno, /kling 等接口） - 不影响使用具体分组令牌的调用 - 不影响其他 Relay 类型（chat/completions 等已有类似处理逻辑）
…task-logging fix(task): 修复使用 auto 分组时 Task Relay 不记录日志和不扣费的问题
fix: 设置默认max req body 为128MB
WalkthroughSwitched GitHub identity handling from login strings to numeric GitHub IDs: added Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Server
participant GitHubAPI
participant DB
Client->>Server: OAuth callback (code)
Server->>GitHubAPI: Exchange code -> access token
GitHubAPI->>Server: Return access token
Server->>GitHubAPI: Request user info (includes numeric id)
GitHubAPI->>Server: Return user info (id, login, email)
Server->>DB: Lookup user by github_id (stringified numeric id)
alt user exists
DB-->>Server: return user
Server->>DB: update user fields as needed (ensure github_id numeric)
else new user
Server->>DB: create user (github_id = numeric id)
DB-->>Server: success or unique-constraint error
alt duplicate username error
Server->>Server: retry username generation (up to 5) and re-attempt insert
end
end
Server->>Client: Issue session / response
sequenceDiagram
actor Client
participant Server
participant GitHubAPI
participant DB
Client->>Server: Bind request (OAuth code)
Server->>GitHubAPI: Exchange code -> user info (numeric id)
GitHubAPI->>Server: Return user info
Server->>DB: Check if numeric github_id already bound
alt bound to another account
DB-->>Server: bound -> Server returns error
else not bound
Server->>DB: update current user set github_id = numeric id
DB-->>Server: updated
Server->>Client: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controller/github.go
🔇 Additional comments (2)
controller/github.go (2)
26-26: LGTM - Proper field addition for GitHub numeric ID.The
Id int64field correctly captures GitHub's immutable numeric user identifier, which is the appropriate choice for unique user identification per OAuth best practices.
77-79: Validation change is safe and reliable.The check
githubUser.Id == 0correctly validates against invalid responses. GitHub API always returns theidfield in user objects, and since GitHub IDs are always positive integers, treating 0 as invalid is a proper safeguard against malformed or incomplete API responses. The change is appropriate.
|
好家伙，你是完全不管兼容的吗 |
9ce1074 to
2c54162
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controller/github.go (1)
113-163: Architectural recommendation: Use database migration script instead of runtime migration.Given the security vulnerabilities and complexity of runtime migration, consider this approach:
Before deployment:
- Create a database migration script that updates all existing
GitHubIdvalues- The script should query GitHub API for each user's current numeric ID based on their stored login
- Update the database records with the numeric IDs
- Notify all users of the maintenance window
After migration:
- Deploy the simplified code that only looks up by numeric ID
- Remove all compatibility fallback logic
- Clean, secure, and maintainable
For users who can't be migrated:
- Those whose GitHub accounts were deleted or whose logins can't be resolved
- Provide a manual account recovery process through support
This approach eliminates the security risks, race conditions, and complexity of runtime migration while properly addressing the compatibility concerns raised by seefs001.
🤖 Fix all issues with AI agents
In @controller/github.go:
- Line 168: The assignment user.Username =
"github_"+strconv.Itoa(model.GetMaxUserId()+1) in the GitHub OAuth flow is racy
because concurrent calls to model.GetMaxUserId() can produce duplicate
usernames; replace this with a safe strategy such as deriving the username from
a database-generated unique value (e.g., user.ID or a DB sequence) or using a
generator that appends a random/UUID suffix, or implement
retry-on-unique-violation logic around the create/insert operation (loop:
generate candidate username, try insert, if unique constraint error retry a
bounded number of times and log/fail if exhausted); update the code path that
sets user.Username and the create/insert logic to use one of these
atomic/transactional approaches instead of GetMaxUserId().
- Around line 224-228: The duplicate-check incorrectly rejects binds when either
the numeric ID or the old login string is taken; change the logic so only the
immutable numeric ID is checked: replace the isAlreadyBound assignment that
calls model.IsGitHubIdAlreadyTaken(githubUser.Login) with a single check using
numericId (i.e., isAlreadyBound := model.IsGitHubIdAlreadyTaken(numericId)), and
remove or adjust any code paths, messages or variables that assumed the
login-based check (e.g., any branches using githubUser.Login) so binding is
allowed when numericId is free even if the old login string exists.
- Around line 113-129: The runtime fallback that checks githubUser.Login (the
model.IsGitHubIdAlreadyTaken(...) branch that sets user.GitHubId to
githubUser.Login and calls common.SysLog) creates an account-takeover risk;
remove that fallback and any in-code migration/update logic so the controller
only looks up users by immutable numericId (numericId and
model.IsGitHubIdAlreadyTaken(numericId)) and then calls FillUserByGitHubId with
the numeric ID; migrate existing login-based records with an offline DB
migration script prior to deployment. If you must keep runtime migration, only
allow a login-based match after verifying the stored numeric ID equals
githubUser.Id (store/compare the numeric ID) before updating user.GitHubId,
otherwise reject the match.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controller/github.go
🔇 Additional comments (3)
controller/github.go (3)
26-26: LGTM: Added numeric ID field.The addition of the
Idfield correctly captures GitHub's immutable numeric identifier, which is the right approach for stable user identification.
77-77: LGTM: Updated validation to use numeric ID.The validation correctly checks for the presence of the numeric ID field.
244-244: LGTM: Correctly uses numeric ID for binding.The bind operation properly stores the numeric ID as a string, which is the correct approach for new bindings.
2c54162 to
1573853
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
controller/github.go (2)
176-182: Bug: Wrong error variable used in error response.Line 179 uses
err.Error()buterrwas set fromgetGitHubUserInfoByCodeat line 109. The actual insertion error is ininsertErr, so this will display the wrong error message to users.🐛 Proposed fix
if insertErr != nil { c.JSON(http.StatusOK, gin.H{ "success": false, - "message": err.Error(), + "message": insertErr.Error(), }) return }
228-237: Compilation error:uservariable is undefined.The
uservariable is referenced on lines 231, 232, 237, and 238, but it is never declared in theGitHubBindfunction. This code will not compile.🐛 Proposed fix: Add user variable declaration
session := sessions.Default(c) id := session.Get("id") // id := c.GetInt("id") // critical bug! + user := model.User{} user.Id = id.(int) err = user.FillUserById()
🤖 Fix all issues with AI agents
In @controller/github.go:
- Around line 269-282: The isDuplicateKeyError function is too broad (matching
"constraint") and can misclassify non-duplicate errors; update
isDuplicateKeyError to detect duplicate-key errors more precisely by checking
concrete indicators: for MySQL detect error number 1062 (or
mysql.MySQLError.Number == 1062), for PostgreSQL detect SQLSTATE "23505" or
pq.Error.Code == "23505", for SQLite match the exact message "UNIQUE constraint
failed" or sqlite3.ErrConstraintUnique, and keep explicit string matches like
"duplicate entry" and "duplicate key value violates unique constraint"; remove
the generic "constraint" substring check and prefer driver-specific error
type/code checks where possible (reference function isDuplicateKeyError).
In @docs/migrate_github_ids.go:
- Around line 154-167: The counting logic uses result[0] which indexes bytes not
runes, so replace the byte-based checks with Unicode-safe checks (e.g., use
strings.HasPrefix(result, "✓ ") / strings.HasPrefix(result, "✓") and
strings.HasPrefix(result, "✗") or parse the first rune via []rune(result) ) to
correctly detect success/skip/fail and increment successCount, skipCount, and
failCount accordingly; update the switch/if on result[0] to operate on the first
rune or string prefix and keep the existing result loop and counter variables
(result, successCount, skipCount, failCount).
- Around line 21-44: getGitHubNumericId currently makes unauthenticated requests
and will hit GitHub's 60/hr rate limit; update it to accept/use an authenticated
request (read a token from env/config and add an Authorization: bearer <TOKEN>
header) and add rate-limit handling: check resp.StatusCode for 401/403 and
inspect X-RateLimit-Remaining and X-RateLimit-Reset headers to either back off
and retry with exponential backoff or surface a clear error; also consider
making the HTTP client injectable (pass an *http.Client or context) so callers
can control timeout/retries and add retry logic around getGitHubNumericId to
handle transient 429/403 due to rate limiting.
🧹 Nitpick comments (2)
docs/migrate_github_ids.go (2)
3-12: Missingstringsimport needed for proper string comparison.If you adopt the
strings.HasPrefixfix for result counting, add"strings"to imports.
50-54: Numeric detection heuristic is acceptable but could be more robust.Checking only the first character works because GitHub usernames cannot start with digits, but a full numeric validation would be safer.
♻️ Optional: Use strconv for complete validation
// 跳过已经是数字ID的用户 - if len(user.GitHubId) > 0 && user.GitHubId[0] >= '0' && user.GitHubId[0] <= '9' { + if _, err := strconv.ParseInt(user.GitHubId, 10, 64); err == nil { results <- fmt.Sprintf("✓ 跳过: %s (ID: %d) - github_id已经是数字格式: %s", user.Username, user.Id, user.GitHubId) return }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controller/github.godocs/migrate_github_ids.godocs/migrate_github_ids.sql
✅ Files skipped from review due to trivial changes (1)
- docs/migrate_github_ids.sql
🧰 Additional context used
🧬 Code graph analysis (1)
docs/migrate_github_ids.go (2)
controller/github.go (1)
GitHubUser(26-31)model/main.go (1)
DB(64-64)
🔇 Additional comments (5)
docs/migrate_github_ids.go (2)
125-143: Concurrency implementation looks correct.The semaphore pattern properly limits concurrent API calls, the goroutine correctly captures the user by value, and the results channel is appropriately sized.
80-104: Good UX: dry-run mode and confirmation before destructive operation.The confirmation prompt and clear warning about database modification is a good practice for migration tools.
controller/github.go (3)
26-31: Good change: Using immutable numeric ID for validation.Switching from mutable
Loginto immutableIdfor identity validation is the correct approach for security.Also applies to: 78-81
142-174: Retry mechanism for username collision is appropriate.The retry loop with 5 attempts and logging is a reasonable approach for handling race conditions during concurrent user registration.
113-119: Security improvement: Using only numeric ID prevents account hijacking.Good security practice. However, this requires all existing users to be migrated first using the migration tool, or they won't be able to log in. Ensure the migration tool is run before deploying this change.
Confirm that the deployment plan includes running the migration tool before deploying this controller change.
1573853 to
fb52a9b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
controller/github.go (2)
176-182: Critical bug: Wrong error variable used in error message.Line 179 uses
err.Error()buterris from the earliergetGitHubUserInfoByCodecall (which succeeded if we reached this point). The correct variable isinsertErr. This will either show an incorrect message or cause a nil pointer panic.Proposed fix
if insertErr != nil { c.JSON(http.StatusOK, gin.H{ "success": false, - "message": err.Error(), + "message": insertErr.Error(), }) return }
228-232: Critical bug:uservariable is used but not declared.
user.Idanduser.FillUserById()are referenced, butuseris never declared inGitHubBind. This will cause a compile error. Auservariable declaration is missing.Proposed fix
session := sessions.Default(c) id := session.Get("id") // id := c.GetInt("id") // critical bug! + user := model.User{} user.Id = id.(int) err = user.FillUserById()
🤖 Fix all issues with AI agents
In @docs/migrate_github_ids.go:
- Around line 167-171: The current check only inspects user.GitHubId[0], which
can falsely treat values like "123abc" as numeric; update the logic that skips
processing (the block referencing user.GitHubId and writing to results) to
validate the entire string is numeric by attempting to parse user.GitHubId as an
integer (e.g., with strconv.ParseInt/strconv.Atoi) or by verifying all runes are
digits, and only skip when the parse/validation succeeds; keep the same results
message and return behavior when the value is confirmed numeric.
- Around line 86-87: The fetchGitHubUser method interpolates raw username into
the GitHub API path, enabling path traversal; sanitize by URL-encoding the
username (e.g., via url.PathEscape) or validate it against an allowed username
pattern before building the URL, update fetchGitHubUser to use the
encoded/validated value when formatting the URL, and add "net/url" to the
imports.
In @docs/migrate_github_ids.sql:
- Around line 63-69: The UPDATE uses a self-referencing subquery on the users
table which can fail on some MySQL versions; change the update to avoid
referencing users in a subquery by either (a) performing a two-step update
(SELECT the target id first, then UPDATE by id), or (b) rewrite the statement to
use a derived filter/CTE or a direct WHERE clause (e.g., match github_id =
'alice' with deleted_at IS NULL and LIMIT 1) so the UPDATE on the users table
does not contain a subquery that reads the same table.
- Around line 91-104: The current SELECT uses MySQL-only REGEXP in the CASE
expressing migration_status (REGEXP '^[0-9]+$' on github_id), which breaks
Postgres/SQLite; update the docs to show database-specific alternatives instead
of a single REGEXP expression: for Postgres use the ~ operator on github_id, and
for SQLite use a combination of GLOB checks (e.g., numeric-only pattern and a
negative match for non-digits), and include these alternative CASE expressions
alongside the original so the users can run the same users -> github_id ->
migration_status query on MySQL, PostgreSQL, and SQLite.
🧹 Nitpick comments (4)
docs/migrate_github_ids.go (4)
17-21: GitHubUser struct is duplicated and missing theThis struct duplicates
controller/github.go:26-31but omits
201-224:tokenFromFileis set but never used.The variable is assigned on lines 216-217 and 221-222 but has no subsequent usage. Remove it or use it for logging/diagnostics.
Proposed fix
- tokenFromFile := false // ... for i, arg := range os.Args { if arg == "--token" && i+1 < len(os.Args) { token = os.Args[i+1] - tokenFromFile = true break } if strings.HasPrefix(arg, "--token=") { token = strings.TrimPrefix(arg, "--token=") - tokenFromFile = true break } }
316-329: Result counting logic using Unicode prefix matching is fragile and error-prone.Distinguishing "success" (
"✓ ") from "skip" ("✓"without trailing space) is brittle. A typo or formatting change will silently break the counts. Use a structured result type instead.Proposed fix using a struct
type MigrationResult struct { Status string // "success", "skip", "fail" Message string } // In migrateUser, return the struct via the channel instead of a formatted string. // Then count by Status field for reliable tallying.
184-191: Redundant assignment:user.GitHubIdis set twice.Line 186 assigns
user.GitHubId = newGitHubId, but theUpdatecall on line 187 also explicitly sets"github_id", newGitHubId. One assignment suffices; remove the struct field mutation.Proposed fix
if !dryRun { // 更新数据库 - user.GitHubId = newGitHubId if err := model.DB.Model(&user).Update("github_id", newGitHubId).Error; err != nil {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controller/github.godocs/migrate_github_ids.godocs/migrate_github_ids.sql
🧰 Additional context used
🧬 Code graph analysis (2)
docs/migrate_github_ids.go (2)
controller/github.go (1)
GitHubUser(26-31)model/main.go (1)
DB(64-64)
controller/github.go (3)
model/user.go (4)
User(20-50)IsGitHubIdAlreadyTaken(586-588)GetMaxUserId(182-186)GetUserIdByAffCode(303-310)common/constants.go (2)
RoleCommonUser(138-138)UserStatusEnabled(179-179)common/sys_log.go (1)
SysLog(11-14)
🔇 Additional comments (4)
docs/migrate_github_ids.sql (1)
1-142: Good documentation coverage for the migration process.The guide includes backup instructions, verification queries, rollback steps, and important notes about timing and idempotence. This is well-structured for operators.
controller/github.go (3)
26-31: LGTM: Added immutable numeric ID field to GitHubUser.Using
Id int64from GitHub's API as the canonical identifier is the correct approach—it never changes even if users rename their accounts.
269-300: Good: Database-agnostic duplicate key detection.The
isDuplicateKeyErrorhelper correctly handles MySQL, PostgreSQL, and SQLite error patterns. This enables the retry logic to work across database backends.
113-140: Security improvement: Using numeric ID prevents account hijacking.The switch from mutable
Loginto immutableIdcloses the vulnerability where a malicious user could rename their GitHub account to a previously-used username and hijack an existing account. Well done.
问题： - GitHub OAuth使用可变的login字段作为唯一标识 - 当用户修改GitHub用户名后，会导致无法登录原账号 - 可能创建重复账号，用户失去原有数据和额度 解决方案： - 在GitHubUser结构体添加Id字段(int64) - 使用GitHub API返回的数字ID作为唯一标识 - 修改所有使用login的地方改用数字ID 修改内容： 1. controller/github.go:26 - 添加Id字段 2. controller/github.go:77 - 验证逻辑检查Id而非Login 3. controller/github.go:113 - GitHubOAuth函数使用数字ID 4. controller/github.go:192 - GitHubBind函数检查数字ID 5. controller/github.go:210 - GitHubBind更新逻辑使用数字ID 参考其他OAuth实现： - Discord使用UID (数字ID) - OIDC使用sub (不可变标识符) - Telegram使用数字ID - 现GitHub也统一使用数字ID 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
fb52a9b to
76fde98
Compare
|
建议你还是先别弄了，你都被CodeRabbit和你的Claude Code带偏到哪儿去了 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controller/github.go (1)
176-182: Critical bug: Wrong error variable returned.Line 179 returns
err.Error()but should returninsertErr.Error(). The variableerris from the earlier GitHub API call (line 109), whileinsertErrcontains the actual insertion error. This will either return the wrong error message or cause a nil pointer dereference iferris nil.🐛 Fix the error variable
if insertErr != nil { c.JSON(http.StatusOK, gin.H{ "success": false, - "message": err.Error(), + "message": insertErr.Error(), }) return }
🤖 Fix all issues with AI agents
In @controller/github.go:
- Around line 142-174: The current username generation uses
model.GetMaxUserId()+1 (in the loop constructing user.Username) which causes a
race under concurrent OAuth logins; replace this with a deterministic unique
suffix such as the GitHub numeric ID or a timestamp/UUID to guarantee uniqueness
(e.g., set user.Username to "github_"+<github numeric id> or a
timestamp+random/UUID), update the code that reads githubUser to use its numeric
ID field when building user.Username, and remove reliance on
model.GetMaxUserId()/retry for uniqueness so user.Insert (and the
isDuplicateKeyError check) no longer depends on fragile max-id logic.
- Around line 270-300: The current string-matching duplicate-key detection is
fragile; enable GORM's TranslateError on all gorm.Config instances used to open
DB connections (set TranslateError: true alongside existing PrepareStmt: true)
and replace the isDuplicateKeyError implementation to use type-safe checking via
errors.Is(err, gorm.ErrDuplicatedKey) instead of string inspection; update
imports accordingly so the function uses the standard errors package and
gorm.ErrDuplicatedKey and remove the old string checks.
🧹 Nitpick comments (2)
docs/migrate_github_ids.go (2)
292-310: Consider reducing the results channel buffer size.Line 296 allocates a buffer equal to the number of users, which could consume significant memory for large user counts (e.g., 10,000+ users). Since the goroutines are waited on before reading results (line 309), consider using an unbuffered channel or a smaller fixed buffer.
♻️ Suggested refactor
- results := make(chan string, len(users)) + results := make(chan string, 100) // Fixed smaller bufferOr use an unbuffered channel:
- results := make(chan string, len(users)) + results := make(chan string)
321-334: Fragile result counting logic.The string-prefix matching for Unicode symbols (lines 324-333) is brittle and assumes exact formatting. The distinction between "✓ " (with space, line 325) meaning success and "✓" alone (line 328) meaning skip is error-prone.
♻️ More robust approach
Consider using structured result types instead of string formatting:
type MigrationResult struct { Status string // "success", "skip", "fail" Message string } // Then modify migrateUser to send MigrationResult instead of string // and count based on result.Status instead of string prefix parsing
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controller/github.godocs/migrate_github_ids.godocs/migrate_github_ids.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/migrate_github_ids.sql
🧰 Additional context used
🧬 Code graph analysis (2)
docs/migrate_github_ids.go (2)
controller/github.go (1)
GitHubUser(26-31)model/main.go (1)
DB(64-64)
controller/github.go (3)
model/user.go (4)
User(20-50)IsGitHubIdAlreadyTaken(586-588)GetMaxUserId(182-186)GetUserIdByAffCode(303-310)common/constants.go (2)
RoleCommonUser(138-138)UserStatusEnabled(179-179)common/sys_log.go (1)
SysLog(11-14)
🔇 Additional comments (8)
docs/migrate_github_ids.go (5)
24-40: LGTM: Clean API client design.The client initialization is well-structured with a reasonable 30s timeout and descriptive user agent.
42-84: LGTM: Robust retry mechanism.The exponential backoff with rate-limit handling and appropriate retry conditions (skipping 404s) follows best practices for GitHub API interaction.
86-144: LGTM: Secure API interaction.The implementation properly escapes the username (line 89), handles authentication, and covers all relevant HTTP status codes.
146-164: LGTM: Useful rate-limit monitoring.The rate-limit checks provide helpful visibility during migration without blocking execution.
166-200: LGTM: Clean migration logic per user.The function correctly skips already-migrated users (line 171), handles both dry-run and live modes, and reports results appropriately.
controller/github.go (3)
26-31: LGTM: Correctly adds numeric ID field.The
Id int64field enables using GitHub's immutable numeric identifier instead of the mutable login name.
78-80: LGTM: Validation switched to immutable ID.Using
Id == 0as the validation check correctly leverages the immutable numeric identifier.
216-237: LGTM: Bind logic correctly uses numeric IDs.The binding flow consistently uses
numericIdfor duplicate checks and assignment, preventing account hijacking via username changes.
| // isDuplicateKeyError 检查错误是否是数据库唯一约束冲突 | ||
| // 通过检测数据库特定的错误码或消息来精确识别 | ||
| func isDuplicateKeyError(err error) bool { | ||
| if err == nil { | ||
| return false | ||
| } | ||
|
|
||
| errMsg := strings.ToLower(err.Error()) | ||
|
|
||
| // MySQL: Error 1062: Duplicate entry | ||
| // 检查 "Duplicate entry" 或 "ER_DUP_ENTRY" | ||
| if strings.Contains(errMsg, "duplicate entry") || strings.Contains(errMsg, "er_dup_entry") { | ||
| return true | ||
| } | ||
|
|
||
| // PostgreSQL: SQLSTATE 23505 - unique_violation | ||
| // 检查 "duplicate key value violates unique constraint" 或 "23505" | ||
| if strings.Contains(errMsg, "duplicate key value violates unique constraint") || | ||
| strings.Contains(errMsg, "sqlstate 23505") || | ||
| strings.Contains(errMsg, "23505") { | ||
| return true | ||
| } | ||
|
|
||
| // SQLite: "UNIQUE constraint failed" | ||
| // 精确匹配 "UNIQUE constraint failed" | ||
| if strings.Contains(errMsg, "unique constraint failed") { | ||
| return true | ||
| } | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the function in controller/github.go
cat -n controller/github.go | sed -n '270,300p'Repository: QuantumNous/new-api
Length of output: 1127
🏁 Script executed:
# Search for where isDuplicateKeyError is used
rg -n "isDuplicateKeyError" -A 3 -B 3Repository: QuantumNous/new-api
Length of output: 784
🏁 Script executed:
# Look for test cases related to duplicate key error detection
rg -n "isDuplicateKeyError|duplicate.*key|UNIQUE.*constraint" -t goRepository: QuantumNous/new-api
Length of output: 604
🏁 Script executed:
# Search for GORM error handling patterns
rg -n "gorm|errors\.Is|errors\.As" controller/github.go -B 2 -A 2Repository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
# Check if there's any GORM-specific error type handling
rg -n "gorm\..*[Ee]rror" -t goRepository: QuantumNous/new-api
Length of output: 2223
🏁 Script executed:
# Find User.Insert implementation
rg -n "func.*Insert" -A 10 model/user.go | head -50Repository: QuantumNous/new-api
Length of output: 413
🏁 Script executed:
# Look for how errors are handled in inserts
rg -n "Insert|insert" controller/github.go -B 5 -A 5Repository: QuantumNous/new-api
Length of output: 1068
🏁 Script executed:
# Check if there are any database/sql or driver error handling patterns
rg -n "database/sql|driver\." -t go | head -20Repository: QuantumNous/new-api
Length of output: 612
🏁 Script executed:
# Search for any existing error wrapping or error type checking
rg -n "errors\.Is|errors\.As|errors\.Unwrap" -t goRepository: QuantumNous/new-api
Length of output: 2202
🏁 Script executed:
# Check GORM version and documentation context
fd -t f "go.mod|go.sum" -x catRepository: QuantumNous/new-api
Length of output: 40223
🏁 Script executed:
# Check if there are any GORM-specific error types we can use
rg -n "gorm\.Err|clause\.|Driver" -t go | head -20Repository: QuantumNous/new-api
Length of output: 844
🌐 Web query:
GORM v1.25.2 duplicate key error handling type-safe approach
💡 Result:
Short answer — two safe options:
- Use GORM’s translator (recommended for DB-agnostic code)
- Enable TranslateError when opening DB so GORM maps driver errors to gorm.ErrDuplicatedKey:
db, _ := gorm.Open(dialector, &gorm.Config{ TranslateError: true }) - Then check:
if err != nil && errors.Is(err, gorm.ErrDuplicatedKey) { /* handle duplicate */ }
Reference: GORM error-handling / TranslateError. [1]
- Driver-specific, type-safe check (Postgres example)
- Inspect the concrete error with errors.As and compare the SQLSTATE (23505) via pgconn/pgerrcode:
var pgErr *pgconn.PgError
if errors.As(err, &pgErr) && pgErr.Code == pgerrcode.UniqueViolation {
// handle duplicate (23505)
}
Notes: import the exact pgconn package used by your driver (e.g. github.com/jackc/pgx/v5/pgconn) to make errors.As succeed. [2][3]
Context: v1.25.x introduced gorm.ErrDuplicatedKey and the TranslateError option — without TranslateError the ErrDuplicatedKey mapping will not be produced, so errors.Is will not match. Use option (1) for a portable approach; use option (2) when you need exact DB-driver details. [2][1][3]
🌐 Web query:
GORM database/sql driver error wrapping PostgreSQL MySQL SQLite unique constraint
💡 Result:
Short answer — database drivers wrap errors, so use errors.As to unwrap and check driver-specific codes (or enable GORM's TranslateError to get gorm.ErrDuplicatedKey).
Examples
-
GORM TranslateError (unified): set gorm.Config{TranslateError: true} and check errors.Is(err, gorm.ErrDuplicatedKey). [1]
-
PostgreSQL (lib/pq or pgconn): unwrap to *pq.Error or *pgconn.PgError and compare Code == "23505" (unique_violation). Use errors.As to handle wrapped errors. [2][3]
Example:
var pgErr pq.Error
if errors.As(err, &pgErr) && pgErr.Code == "23505" { / duplicate */ } -
MySQL (go-sql-driver/mysql): unwrap to *mysql.MySQLError and check Number == 1062 (ER_DUP_ENTRY). [4]
Example:
var myErr mysql.MySQLError
if errors.As(err, &myErr) && myErr.Number == 1062 { / duplicate */ } -
SQLite (mattn/go-sqlite3): driver exposes sqlite3.ErrConstraint and extended result codes; unwrap and inspect the driver error (or use errors.Is(err, sqlite3.ErrConstraint)). The sqlite3 driver/SQL returns constraint errors for UNIQUE violations. [5]
Notes
- Always use errors.As (not string matching) because drivers (and GORM) commonly wrap errors. [2][4]
- If you want DB-agnostic handling, enable GORM TranslateError and test gorm.ErrDuplicatedKey. [1]
Sources
[1] GORM error handling / TranslateError docs.
[2] lib/pq / Postgres unique_violation (SQLSTATE 23505) examples.
[3] Discussion/examples using pgconn/pq with errors.As (StackOverflow/blog).
[4] go-sql-driver/mysql duplicate-entry handling (error Number 1062).
[5] mattn/go-sqlite3 driver README / constraint error behavior.
🏁 Script executed:
# Find database initialization and configuration
rg -n "gorm.Open|gorm.Config" -A 5 -B 2 model/main.go | head -40Repository: QuantumNous/new-api
Length of output: 1215
🏁 Script executed:
# Look for TranslateError setting
rg -n "TranslateError" -iRepository: QuantumNous/new-api
Length of output: 45
Enable GORM's TranslateError and use type-safe error checking instead of string matching.
The string-based approach is fragile and will break if database drivers change error messages across versions. GORM v1.25.2 provides gorm.ErrDuplicatedKey, but it requires enabling TranslateError in the database configuration.
To fix this:
- Enable
TranslateError: trueingorm.Configfor all database connections inmodel/main.go(lines 135, 146, 165, 172):
&gorm.Config{
PrepareStmt: true,
TranslateError: true, // Add this
}- Replace the
isDuplicateKeyErrorfunction with:
func isDuplicateKeyError(err error) bool {
return errors.Is(err, gorm.ErrDuplicatedKey)
}This aligns with the codebase's existing error handling pattern (used with gorm.ErrRecordNotFound throughout) and eliminates fragility from string matching.
🤖 Prompt for AI Agents
In @controller/github.go around lines 270 - 300, The current string-matching
duplicate-key detection is fragile; enable GORM's TranslateError on all
gorm.Config instances used to open DB connections (set TranslateError: true
alongside existing PrepareStmt: true) and replace the isDuplicateKeyError
implementation to use type-safe checking via errors.Is(err,
gorm.ErrDuplicatedKey) instead of string inspection; update imports accordingly
so the function uses the standard errors package and gorm.ErrDuplicatedKey and
remove the old string checks.
Remove the retry mechanism in username generation since we now use GitHub numeric ID directly as the username suffix, which eliminates race conditions. Also remove the isDuplicateKeyError function as it's no longer needed. Changes: - controller/github.go: - Remove isDuplicateKeyError function (no longer needed) - Remove unused imports (strings, gorm.io/gorm) - Fix missing user variable declaration in GitHubBind function - docs/migrate_github_ids.go: - Remove unused tokenFromFile variable - Change common.Setup() to model.InitDB() - Remove unused common import The username generation now directly uses GitHub numeric ID as suffix: user.Username = "github_" + numericId This is deterministic, race-condition-free, and doesn't require retry logic. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
controller/github.go (2)
199-209: Duplicate binding risk: Ensure GitHub ID migration is completed before deployment.The binding check validates only against numeric IDs. If a GitHub account was previously bound using login-based format (from before the numeric ID requirement),
IsGitHubIdAlreadyTaken(numericId)will not detect the existing binding, allowing the same GitHub account to be bound to multiple user accounts.The migration script (
docs/migrate_github_ids.go) exists to convert old login-based GitHub IDs to numeric format, but it is a manual, optional tool—not an automatic migration. If not executed before deploying this code, old bindings remain undetected and the duplicate binding vulnerability persists.Required action: Run the migration script to convert all existing
github_idvalues to numeric format before deploying this change.
113-172: Critical: Incomplete safeguards for GitHub ID migration — existing users will lose login access if migration is skipped.While migration tools exist (
docs/migrate_github_ids.goanddocs/migrate_github_ids.sql), they are entirely optional and manual. The code will immediately break login for any users with non-numericgithub_idvalues:
- Existing users have
github_idas login names (e.g.,"alice")- New code only checks for numeric IDs via
IsGitHubIdAlreadyTaken(numericId)- Without migration, the lookup fails and the system attempts to create a duplicate account
- Result: Login failures for all existing GitHub OAuth users
The vulnerability: Migration is not enforced or validated at startup. There is no:
- Deployment check to prevent starting without migration
- Startup validation to warn about unmigrated users
- Documentation in README requiring migration as a prerequisite
This creates a critical deployment risk—a forgotten migration will silently break production login without triggering any warnings.
Required fixes:
- Add startup validation to detect and reject deployment if unmigrated users exist, OR
- Add explicit documentation in deployment guide marking migration as mandatory before deploying this code, AND
- Reference the migration tools in prominent deployment prerequisites
🧹 Nitpick comments (3)
controller/github.go (3)
138-139: Remove redundant assignment.
user.GitHubId = numericIdis already set on line 115. This assignment on line 139 is redundant.♻️ Remove redundant assignment
- // 创建新账号,使用数字ID作为GitHub ID - user.GitHubId = numericId - // 使用GitHub数字ID作为用户名后缀,确保唯一性且无竞态条件
141-143: Consider more user-friendly username generation.While
"github_" + numericIdis deterministic and avoids race conditions, usernames likegithub_12345678are not user-friendly. Consider either:
- Use GitHub login with numeric ID fallback:
githubUser.Login(or"github_" + numericIdif login is unavailable/taken), OR- Allow users to customize their username after first login, OR
- Document that users should update their username via account settings after registration
The current approach is functional but creates poor initial UX.
83-231: Deployment requires strict sequencing to prevent user lockout.This is a breaking change that fundamentally alters the
github_idstorage format. To deploy safely:
- Pre-deployment: Run the migration scripts (
docs/migrate_github_ids.goand SQL migrations) to convert ALL existing login-basedgithub_idvalues to numeric IDs- Verify: Confirm zero users remain with non-numeric
github_idvalues- Deploy: Only after successful migration, deploy this application code
- Monitor: Watch for login failures immediately after deployment
Missing safeguard: Consider adding a startup validation check that scans for any remaining non-numeric
github_idvalues and refuses to start if found. This prevents accidental deployment before migration is complete.Example validation:
// On application startup func ValidateGitHubIdMigration() error { var count int64 // Check for any github_id that doesn't parse as a valid int64 // This would indicate unmigrated login-based IDs DB.Model(&User{}).Where("github_id != '' AND github_id NOT REGEXP '^[0-9]+$'").Count(&count) if count > 0 { return fmt.Errorf("Found %d users with unmigrated github_id values. Run migration first.", count) } return nil }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controller/github.godocs/migrate_github_ids.go
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/migrate_github_ids.go
🧰 Additional context used
🧬 Code graph analysis (1)
controller/github.go (2)
model/user.go (2)
User(20-50)IsGitHubIdAlreadyTaken(586-588)common/gin.go (1)
ApiError(141-146)
🔇 Additional comments (3)
controller/github.go (3)
26-26: Good addition of immutable GitHub ID field.Adding the numeric
Idfield is the correct approach, as GitHub's numeric IDs are immutable and prevent account hijacking risks associated with mutable login names.
77-79: Validation correctly updated to check numeric ID.The change from validating
Loginto validatingId == 0is appropriate for ensuring GitHub's numeric identifier is present.
220-220: Correct assignment of numeric ID for binding.The assignment correctly stores GitHub's numeric ID when binding accounts.
唉……好吧…… |
|
感谢您的贡献，这个确实是一个比较严重的问题，我们看了一下pr，代码实现的方式不是很符合我们的想法，后面我们会开一个新的pr，到时候把您设置为合作贡献者，您看可以吗 |
好的，谢谢您 |
20 participants
🔒 安全修复：GitHub OAuth使用可变login字段导致用户数据丢失
🚨 严重安全漏洞
main分支存在多个严重的安全和架构问题：
login字段（用户名）作为唯一标识GetMaxUserId()+1导致用户名冲突📝 与main分支的详细对比
核心修改：
controller/github.gomain分支的问题代码：