Fix/pr 2900 - #2998
Conversation
WalkthroughThese changes introduce support for custom usernames in OAuth flows by adding a new username length validation constant and conditionally applying the provided OAuth username during user creation instead of using the auto-generated provider-prefixed default. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/oauth.go`:
- Around line 241-248: The username length check uses len() (bytes) so
multi-byte UTF-8 names can be wrongly rejected and DB errors from
CheckUserExistOrDeleted are swallowed; replace len(oauthUser.Username) <=
model.UserNameMaxLength with utf8.RuneCountInString(oauthUser.Username) <=
model.UserNameMaxLength to count runes consistently with the validator, and when
calling model.CheckUserExistOrDeleted(...) propagate or at minimum log the
returned error (use the existing logger) instead of ignoring it so transient DB
errors are visible; keep the surrounding logic that assigns user.Username =
oauthUser.Username when the name is valid and not existing/deleted.
- Around line 241-248: Replace the byte-length check with a rune-length check
and surface DB errors: call utf8.RuneCountInString(oauthUser.Username) instead
of len(oauthUser.Username) when comparing to model.UserNameMaxLength, and when
invoking model.CheckUserExistOrDeleted(oauthUser.Username, "") handle the
returned error by logging it via common.SysLog(...) before proceeding (only set
user.Username = oauthUser.Username when no error and exists==false and the
rune-count check passes).
| if oauthUser.Username != "" { | ||
| if exists, err := model.CheckUserExistOrDeleted(oauthUser.Username, ""); err == nil && !exists { | ||
| // 防止索引退化 | ||
| if len(oauthUser.Username) <= model.UserNameMaxLength { | ||
| user.Username = oauthUser.Username | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
go-playground validator v10 max tag strings rune count utf8
💡 Result:
In go-playground/validator/v10, the max tag (and min/len/gt/gte/lt/lte on strings) measures string length in Unicode code points (runes), not bytes:
maxis implemented ashasMaxOf→isLte[1].- For
reflect.String,isLtecomparesint64(utf8.RuneCountInString(field.String())) <= p[1]. - The public docs also describe
min(and by extensionmax) for strings as “number of characters” [2].
Notes:
- This is rune count, not grapheme-cluster/user-perceived “characters” (some emojis/combining sequences can count as >1 rune).
- It does not enforce “valid UTF-8”; it just counts using
utf8.RuneCountInStringsemantics.
Sources: [1] baked-in implementation (hasMaxOf/isLte uses utf8.RuneCountInString) https://docs-go.hexacode.org/src/github.com/go-playground/validator/v10/baked_in.go, [2] pkg.go.dev docs for min on strings (“characters”) https://pkg.go.dev/github.com/go-playground/validator/v10
🏁 Script executed:
# Find the oauth.go file and examine the context around lines 241-248
find . -name "oauth.go" -type fRepository: QuantumNous/new-api
Length of output: 84
🏁 Script executed:
# Search for model.UserNameMaxLength definition
rg "UserNameMaxLength" --type goRepository: QuantumNous/new-api
Length of output: 184
🏁 Script executed:
# Check imports in controller/oauth.go to see current JSON handling patterns
head -50 controller/oauth.goRepository: QuantumNous/new-api
Length of output: 1316
🏁 Script executed:
# Search for oauthUser struct definition and its validator tags
rg "type.*OAuthUser" --type go -A 10Repository: QuantumNous/new-api
Length of output: 1138
🏁 Script executed:
# Get more context around lines 241-248 in controller/oauth.go
sed -n '230,260p' controller/oauth.goRepository: QuantumNous/new-api
Length of output: 894
🏁 Script executed:
# Find CheckUserExistOrDeleted function to understand what errors it returns
rg "func.*CheckUserExistOrDeleted" --type go -A 15Repository: QuantumNous/new-api
Length of output: 816
🏁 Script executed:
# Check User struct definition and validator tags
rg "type User struct" --type go -A 30 model/user.goRepository: QuantumNous/new-api
Length of output: 2899
🏁 Script executed:
# Check if validator tags reference UserNameMaxLength elsewhere
rg "validate.*UserNameMaxLength|UserNameMaxLength.*validate" --type goRepository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
# Check the full context - where does oauthUser get validated before reaching this code?
sed -n '220,245p' controller/oauth.goRepository: QuantumNous/new-api
Length of output: 1035
Use utf8.RuneCountInString() instead of len() to count username length consistently with the validator constraint.
The User.Username field has a validate:"max=20" constraint that counts UTF-8 runes (not bytes), but this code checks len(oauthUser.Username) <= model.UserNameMaxLength which counts bytes. A 15-character CJK username (45 bytes but only 15 runes) would pass validation in a normal registration flow but get silently rejected here, falling back to an auto-generated username.
Additionally, when CheckUserExistOrDeleted returns an error, it is currently silently swallowed without logging. This makes transient database failures invisible.
Proposed fix
+import "unicode/utf8"
if oauthUser.Username != "" {
if exists, err := model.CheckUserExistOrDeleted(oauthUser.Username, ""); err == nil && !exists {
- if len(oauthUser.Username) <= model.UserNameMaxLength {
+ if utf8.RuneCountInString(oauthUser.Username) <= model.UserNameMaxLength {
user.Username = oauthUser.Username
}
+ } else if err != nil {
+ common.SysLog(fmt.Sprintf("[OAuth] failed to check username existence for %q: %v", oauthUser.Username, err))
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if oauthUser.Username != "" { | |
| if exists, err := model.CheckUserExistOrDeleted(oauthUser.Username, ""); err == nil && !exists { | |
| // 防止索引退化 | |
| if len(oauthUser.Username) <= model.UserNameMaxLength { | |
| user.Username = oauthUser.Username | |
| } | |
| } | |
| } | |
| if oauthUser.Username != "" { | |
| if exists, err := model.CheckUserExistOrDeleted(oauthUser.Username, ""); err == nil && !exists { | |
| // 防止索引退化 | |
| if utf8.RuneCountInString(oauthUser.Username) <= model.UserNameMaxLength { | |
| user.Username = oauthUser.Username | |
| } | |
| } else if err != nil { | |
| common.SysLog(fmt.Sprintf("[OAuth] failed to check username existence for %q: %v", oauthUser.Username, err)) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/oauth.go` around lines 241 - 248, The username length check uses
len() (bytes) so multi-byte UTF-8 names can be wrongly rejected and DB errors
from CheckUserExistOrDeleted are swallowed; replace len(oauthUser.Username) <=
model.UserNameMaxLength with utf8.RuneCountInString(oauthUser.Username) <=
model.UserNameMaxLength to count runes consistently with the validator, and when
calling model.CheckUserExistOrDeleted(...) propagate or at minimum log the
returned error (use the existing logger) instead of ignoring it so transient DB
errors are visible; keep the surrounding logic that assigns user.Username =
oauthUser.Username when the name is valid and not existing/deleted.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the actual code in controller/oauth.go
fd -type f -name "oauth.go" -path "*/controller/*" --exec cat -n {} \; | head -300Repository: QuantumNous/new-api
Length of output: 233
🏁 Script executed:
# Search for UserNameMaxLength definition
rg "UserNameMaxLength" -B 2 -A 2Repository: QuantumNous/new-api
Length of output: 571
🏁 Script executed:
# Check if go-playground/validator is used in the codebase
rg "github.com/go-playground/validator" -nRepository: QuantumNous/new-api
Length of output: 638
🌐 Web query:
go-playground/validator v10 max constraint string rune count implementation
💡 Result:
In go-playground/validator/v10 (e.g., v10.30.1), the max tag for strings is implemented as hasMaxOf, which simply delegates to isLte (“less-than-or-equal”). isLte measures string length using utf8.RuneCountInString(field.String()), i.e. rune/code-point count, not byte length (and not grapheme-cluster/user-perceived character count). max and lte therefore behave the same for strings. [1]
The package docs also describe max for strings as “string length is at most that number of characters.” [2]
Sources
[1] baked_in.go (hasMaxOf → isLte; isLte uses utf8.RuneCountInString for strings) (raw.githubusercontent.com)
[2] Package documentation for max (pkg.go.dev)
Citations:
- 1: https://raw.githubusercontent.com/go-playground/validator/v10.30.1/baked_in.go
- 2: https://pkg.go.dev/github.com/go-playground/validator/v10?utm_source=openai
🏁 Script executed:
# Find User struct definition and Username field tag
rg "type User struct" -A 50 | head -80Repository: QuantumNous/new-api
Length of output: 3986
🏁 Script executed:
# Search for Username field with validation tags
rg "Username.*validate" -nRepository: QuantumNous/new-api
Length of output: 168
🏁 Script executed:
# Check CheckUserExistOrDeleted function and error handling patterns
rg "CheckUserExistOrDeleted" -B 2 -A 3Repository: QuantumNous/new-api
Length of output: 1253
🏁 Script executed:
# Search for other uses of len() on username fields in oauth.go
rg "len\(.*Username\)" -nRepository: QuantumNous/new-api
Length of output: 198
Use utf8.RuneCountInString() instead of len() to match validator v10's max constraint, and log errors from CheckUserExistOrDeleted().
The Username field has validate:"max=20" which measures rune count via utf8.RuneCountInString(). However, the guard at line 244 uses len(), which counts bytes. For multi-byte usernames (e.g., 15 CJK characters = 45 bytes), this incorrectly rejects valid usernames that would pass validation.
Additionally, errors from CheckUserExistOrDeleted() are silently discarded, making DB failures invisible. Other parts of the codebase (e.g., controller/user.go) properly log these errors with common.SysLog().
Proposed fix
+import "unicode/utf8"
if oauthUser.Username != "" {
- if exists, err := model.CheckUserExistOrDeleted(oauthUser.Username, ""); err == nil && !exists {
- // 防止索引退化
- if len(oauthUser.Username) <= model.UserNameMaxLength {
- user.Username = oauthUser.Username
- }
- }
+ if exists, err := model.CheckUserExistOrDeleted(oauthUser.Username, ""); err != nil {
+ common.SysLog(fmt.Sprintf("[OAuth] failed to check username %q: %v", oauthUser.Username, err))
+ } else if !exists && utf8.RuneCountInString(oauthUser.Username) <= model.UserNameMaxLength {
+ user.Username = oauthUser.Username
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/oauth.go` around lines 241 - 248, Replace the byte-length check
with a rune-length check and surface DB errors: call
utf8.RuneCountInString(oauthUser.Username) instead of len(oauthUser.Username)
when comparing to model.UserNameMaxLength, and when invoking
model.CheckUserExistOrDeleted(oauthUser.Username, "") handle the returned error
by logging it via common.SysLog(...) before proceeding (only set user.Username =
oauthUser.Username when no error and exists==false and the rune-count check
passes).
3 participants
Summary by CodeRabbit
Release Notes