Codex/new api skill market - #4357
Conversation
Record SMS verification code login feature added in this fork: - Support Aliyun, Aliyun PNVS, Tencent Cloud SMS providers - Rate limiting: 1/60s per phone, 5/hour per IP - Auto-register new users with phone number - Support 2FA and Turnstile verification Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Support Aliyun, Aliyun PNVS, Tencent Cloud SMS providers - Add rate limiting: 1/60s per phone, 5/hour per IP - Auto-register new users with phone number - Support 2FA and Turnstile verification - Add Phone field to User model Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add /api/client/login_sms: SMS code login with auto-register and per-user reusable API token (returns full key, no cookie session) - Add /api/client/self: API key authenticated user info endpoint - Add /api/client/subscription/plans: public subscription plans list - Add /api/client/subscription/epay/pay: subscription order via API key Currency defaults switched from USD to CNY: - SubscriptionPlan.Currency default - subscription_plans table DDL default - AdminCreate/UpdateSubscriptionPlan force CNY - General quota_display_type default
- sync-db-pull.sh: download server one-api.db via SSH, auto-backup local - sync-db-push.sh: upload local db to server with confirmation + rollback
WalkthroughThis PR adds SMS-based authentication with configurable providers (Aliyun, Tencent), introduces client-specific API endpoints for SMS login and token management, and implements a skill market admin interface for managing downloadable skills. Supporting changes include SMS rate limiting, CORS refinement, user phone field storage, database migrations, UI components for SMS login forms, internationalization keys, and database sync utility scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Client
participant Server as New-API Server
participant SmsProvider as SMS Provider<br/>(Aliyun/Tencent)
participant DB as Database
User->>Server: POST /api/sms/send<br/>{phone}
activate Server
Server->>Server: Validate phone
Server->>Server: Generate 6-digit code
Server->>DB: Store verification code<br/>(phone, code, purpose)
DB-->>Server: ✓
Server->>SmsProvider: SendCode(phone, code)
activate SmsProvider
SmsProvider-->>Server: ✓ Success
deactivate SmsProvider
Server-->>User: {success: true}
deactivate Server
User->>Server: POST /api/user/login/sms<br/>{phone, code}
activate Server
Server->>DB: Fetch verification code
DB-->>Server: {code, timestamp}
Server->>Server: Verify code match
Server->>DB: Delete verification record
Server->>DB: Load user by phone
alt User not found
Server->>Server: Auto-register enabled?
Server->>DB: Create new user<br/>(phone, username, etc.)
DB-->>Server: {userId}
else User exists
DB-->>Server: {userId, status}
end
Server->>Server: Check 2FA required?
alt 2FA needed
Server->>Server: Store pending login in session
Server-->>User: {require_2fa: true}
else No 2FA
Server->>DB: Create/fetch dedicated token
DB-->>Server: {apiKey}
Server->>DB: Log login event
Server-->>User: {success: true, user, apiKey}
end
deactivate Server
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 4
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
model/option.go (1)
267-276:⚠️ Potential issue | 🟡 MinorStale comment and loss of USD path via legacy toggle.
Two concerns in this block:
- The comment on line 269 still says
true -> USD, false -> TOKENS, but the code now mapstrueto"CNY". Please update the comment to match reality.- The legacy
DisplayInCurrencyEnabled=truepath previously let operators choose USD display; after this change they can no longer reachQuotaDisplayTypeUSDthrough this compatibility branch. Any client or admin UI still togglingDisplayInCurrencyEnabledwill now silently coerce display to CNY. If non-CNY deployments are still supported, consider either (a) preserving the previousUSDmapping here and relying onquota_display_typesetters for the CNY default, or (b) documenting clearly that legacy toggling now forces CNY.✏️ Minimal comment fix
case "DisplayInCurrencyEnabled": // 兼容旧字段：同步到新配置 general_setting.quota_display_type（运行时生效） - // true -> USD, false -> TOKENS + // true -> CNY, false -> TOKENS newVal := "CNY"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/option.go` around lines 267 - 276, The comment in the switch case for "DisplayInCurrencyEnabled" is stale and the compatibility mapping changed: update the inline comment in the case to reflect that true now maps to "CNY" (not USD), and address the behavior change so legacy toggles don't silently force CNY — either restore the previous USD mapping for backward compatibility (set newVal to "USD" when boolValue is true) or add a clear comment and/or documentation note that this branch now coerces legacy true to "CNY"; modify the code around the DisplayInCurrencyEnabled case (variable newVal, the boolValue check, and the call to config.UpdateConfigFromMap via config.GlobalConfig.Get("general_setting")) to implement your chosen approach and ensure QuotaDisplayTypeUSD remains reachable if you opt to preserve USD mapping.web/src/components/settings/SystemSetting.jsx (1)
182-199:⚠️ Potential issue | 🟠 MajorNormalize
sms.enabledbefore binding it to the checkbox.Line 1128 binds
sms.enabledto aForm.Checkbox, but this switch does not convert the option value from"true"/"false"to a boolean. A stored"false"string can be treated as truthy by the form control and show SMS login enabled when it is not.Proposed fix
case 'LinuxDOOAuthEnabled': case 'discord.enabled': case 'oidc.enabled': + case 'sms.enabled': case 'passkey.enabled':🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/settings/SystemSetting.jsx` around lines 182 - 199, The sms.enabled option isn't normalized to a boolean before binding to the Form.Checkbox, so add the 'sms.enabled' case to the switch that converts item.value via toBoolean; specifically update the block handling option keys (cases like 'PasswordLoginEnabled', 'discord.enabled', etc.) to include 'sms.enabled' so item.value = toBoolean(item.value) runs for it, ensuring the Form.Checkbox receives a real boolean rather than the string "true"/"false".
🟠 Major comments (24)
scripts/sync-db-push.sh-7-9 (1)
7-9:⚠️ Potential issue | 🟠 MajorDefault service name
myclaw-newapidoes not match the systemd unit shipped in this repo.The repository ships
new-api.service(unit namenew-api), but this script defaultsREMOTE_SERVICEtomyclaw-newapi. Any contributor who runs this script without settingNEWAPI_REMOTE_SERVICEwill get asystemctl restart myclaw-newapithat fails on a fresh install — and because the failure happens after the DB has already been overwritten (line 61), the rollback branch at line 66‑70 will also run against the wrong service and leave the server in a broken state.Either default to
new-api(matching the shipped unit) or makeNEWAPI_REMOTE_SERVICEmandatory with:?.-REMOTE_SERVICE="${NEWAPI_REMOTE_SERVICE:-myclaw-newapi}" +REMOTE_SERVICE="${NEWAPI_REMOTE_SERVICE:-new-api}"Same concern about
REMOTE_HOST="001"applies as insync-db-pull.sh— it's a personal SSH alias.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sync-db-push.sh` around lines 7 - 9, Change the unsafe defaults: replace the current fallback for REMOTE_SERVICE (NEWAPI_REMOTE_SERVICE) so it either defaults to "new-api" (the shipped systemd unit) or is made mandatory with the parameter expansion form that fails if unset (use :?); likewise remove the personal SSH alias default for REMOTE_HOST ("001") by either defaulting to empty or making NEWAPI_REMOTE_HOST mandatory with :? so contributors cannot run the script against the wrong host/service; update references to REMOTE_SERVICE and REMOTE_HOST in this script (the REMOTE_SERVICE variable and systemctl restart/rollback logic) to rely on the new behavior.model/client_skill_market.go-1-63 (1)
1-63: 🛠️ Refactor suggestion | 🟠 MajorCoding guideline violation: direct
encoding/jsonusage.Per repository guidelines, all JSON marshal/unmarshal operations in Go business code must go through the wrappers in
common/json.go(common.Marshal,common.Unmarshal,common.UnmarshalJsonStr,common.DecodeJson,common.GetJsonType). This file importsencoding/json(line 4) and callsjson.UnmarshalinTagList(line 59).As per coding guidelines: "All JSON marshal/unmarshal operations MUST use wrapper functions in
common/json.go... Do NOT directly import or callencoding/jsonin business code."♻️ Proposed fix
import ( - "encoding/json" - "github.com/QuantumNous/new-api/common" "gorm.io/gorm" ) @@ func (s *ClientSkillMarketItem) TagList() []string { if len(s.Tags) == 0 { return []string{} } var tags []string - if err := json.Unmarshal(s.Tags, &tags); err != nil { + if err := common.Unmarshal(s.Tags, &tags); err != nil { return []string{} } return tags }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/client_skill_market.go` around lines 1 - 63, The file imports encoding/json and calls json.Unmarshal in ClientSkillMarketItem.TagList, violating the guideline to use common/json.go wrappers; remove the encoding/json import and replace json.Unmarshal(s.Tags, &tags) with the repository wrapper (e.g. common.Unmarshal or common.UnmarshalJsonStr as appropriate) so TagList uses common.Unmarshal to decode s.Tags into []string, returning an empty slice on error as before; update imports accordingly and keep the function name TagList and type JSONValue unchanged.controller/client_skillhub.go-26-33 (1)
26-33:⚠️ Potential issue | 🟠 Major
slugis concatenated into the upstream URL without URL-encoding.
slugcomes straight fromc.Query("slug")and is appended toskillHubDownloadBaseURL + slug. A slug containing&,#,?, a space, or non-ASCII bytes will change the upstream query semantics or break the request (e.g.slug=foo&admin=1would inject a second query parameter into the SkillHub call). Percent-encode it before concatenation.🛡️ Proposed fix
- "net/http" - "path" - "strings" + "net/http" + "net/url" + "path" + "strings" @@ - upstreamURL := skillHubDownloadBaseURL + slug + upstreamURL := skillHubDownloadBaseURL + url.QueryEscape(slug)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/client_skillhub.go` around lines 26 - 33, The slug from c.Query("slug") is used raw when building upstreamURL (skillHubDownloadBaseURL + slug) which allows injection of special chars; before concatenation, percent-encode the slug (e.g. use url.PathEscape or equivalent) and assign the escaped value back to slug (or a new variable) so upstreamURL is built as skillHubDownloadBaseURL + escapedSlug; keep the rest of the flow (resp, err := service.DoDownloadRequest(upstreamURL, "skillhub skill download proxy", slug)) but pass the original slug where semantic logging/identification is needed and the escapedSlug into upstreamURL construction.controller/subscription.go-129-132 (1)
129-132:⚠️ Potential issue | 🟠 MajorDon’t overwrite explicit plan currencies.
These unconditional assignments force every created or updated plan to
CNY, discarding admin-provided values. KeepCNYas the default only when the request leaves currency empty.🐛 Proposed fix
if req.Plan.Currency == "" { req.Plan.Currency = "CNY" } - req.Plan.Currency = "CNY"if req.Plan.Currency == "" { req.Plan.Currency = "CNY" } - req.Plan.Currency = "CNY"Also applies to: 192-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/subscription.go` around lines 129 - 132, The code currently unconditionally sets req.Plan.Currency = "CNY" (lines around the first block and again around lines 192-195), which overwrites admin-provided currencies; change both places to only assign the default when the incoming value is empty (e.g., if req.Plan.Currency == "" { req.Plan.Currency = "CNY" }) and remove the extra unconditional assignment so explicit currencies are preserved while still defaulting to CNY when not provided.middleware/cors.go-18-28 (1)
18-28:⚠️ Potential issue | 🟠 MajorTighten the credentialed CORS origin check.
With
AllowCredentials = true,strings.HasPrefix(origin, "https://")still allows any HTTPS site to issue credentialed cross-origin requests. The localhost prefix checks also match hostnames likehttp://localhost.evil.example. Parse the origin and allow only exact dev hosts plus a configured production allowlist.🔒 Proposed direction
+ allowedOrigins := common.GetEnvOrDefaultString("CORS_ALLOWED_ORIGINS", "") + allowed := map[string]struct{}{} + for _, item := range strings.Split(allowedOrigins, ",") { + if origin := strings.TrimSpace(item); origin != "" { + allowed[origin] = struct{}{} + } + } config.AllowOriginFunc = func(origin string) bool { - // Allow localhost for development - if strings.HasPrefix(origin, "http://localhost") || strings.HasPrefix(origin, "http://127.0.0.1") { + u, err := url.Parse(origin) + if err != nil { + return false + } + if u.Scheme == "http" && (u.Hostname() == "localhost" || u.Hostname() == "127.0.0.1") { return true } - // Allow all HTTPS origins (production) - if strings.HasPrefix(origin, "https://") { + if _, ok := allowed[origin]; ok { return true } return false }This also requires adding
net/urlto imports and wiring the production origins through configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/cors.go` around lines 18 - 28, The current AllowOriginFunc (config.AllowOriginFunc) is too permissive for credentialed CORS and matches substrings like "http://localhost.evil.example"; change it to parse the origin with net/url (url.Parse) and then validate scheme == "http" or "https" and exact host equality (e.g., "localhost", "127.0.0.1" with optional port) for dev, and check the parsed origin (Scheme + "://" + Host) against a configured production allowlist (passed via config) for production; use the parsed Host (not HasPrefix) to avoid subdomain spoofing and ensure AllowCredentials remains safe by only returning true when the origin exactly matches an allowed entry.web/src/i18n/locales/en.json-3384-3390 (1)
3384-3390:⚠️ Potential issue | 🟠 MajorRemove duplicate locale keys before they override existing translations.
The keys
请输入验证码,获取验证码, and两步验证already exist earlier in thistranslationobject. JSON object keys must be unique; the last duplicate value overwrites earlier ones, silently changing existing UI copy globally, including the 2FA label. Use distinct Chinese source strings such as获取短信验证码or短信两步验证instead.🌐 Suggested cleanup
- "请输入验证码": "Please enter verification code", "请输入手机号和验证码": "Please enter phone number and verification code", - "获取验证码": "Get Code", + "获取短信验证码": "Get SMS Code", "秒后重新获取": "s to resend", "验证码发送成功": "Verification code sent", "验证码发送失败": "Failed to send verification code", - "两步验证": "Two-Step Verification",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/i18n/locales/en.json` around lines 3384 - 3390, The JSON contains duplicate Chinese keys "请输入验证码", "获取验证码", and "两步验证" which override earlier translations; remove or rename these duplicates (for example rename to "请输入短信验证码", "获取短信验证码", or "短信两步验证") and update their English values accordingly so each source key is unique; ensure you keep the original earlier entries intact (or merge meanings consistently) and adjust any code references if they rely on the renamed keys.web/src/hooks/common/useSidebar.js-51-51 (1)
51-51:⚠️ Potential issue | 🟠 MajorRemove
skill-marketfromDEFAULT_ADMIN_CONFIGor gate it properly viaisModuleVisible().The sidebar explicitly bypasses the
isModuleVisible()check forskill-market, only checkingisAdmin()(line 204 inSiderBar.jsx). Adding this entry to the default config is misleading because administrators cannot disable it throughSidebarModulesAdmin—the sidebar will always show it for all admins regardless of configuration. Either:
- Remove the config entry and document why skill-market is intentionally outside the permission system, or
- Update the sidebar to use
isModuleVisible('admin', 'skill-market')like other admin modules.Per the sidebar architecture, admin access should be governed by the
SidebarModulesAdminconfiguration, not hardcoded allowlists. The current implementation bypasses that system.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/hooks/common/useSidebar.js` at line 51, DEFAULT_ADMIN_CONFIG currently contains 'skill-market' but SiderBar.jsx bypasses isModuleVisible() for that module (it only checks isAdmin()), so remove 'skill-market' from DEFAULT_ADMIN_CONFIG or make the sidebar obey the admin config; specifically either delete the 'skill-market' entry from DEFAULT_ADMIN_CONFIG (and add a comment explaining it's intentionally outside the permission system) or modify SiderBar.jsx to call isModuleVisible('admin', 'skill-market') where other admin modules are checked so SidebarModulesAdmin controls its visibility instead of the hardcoded allowlist.middleware/rate-limit.go-91-101 (1)
91-101:⚠️ Potential issue | 🟠 MajorDo not bypass the global web limiter based on loopback
ClientIP().In typical reverse-proxy deployments where the proxy connects from
127.0.0.1, this bypass disables rate limiting for all traffic. Thelocalhostprefix check is ineffective dead code (IP addresses never match this pattern). Remove the bypass or gate it behind an explicit development-only configuration flag.🛡️ Safer default
func GlobalWebRateLimit() func(c *gin.Context) { if common.GlobalWebRateLimitEnable { - limiter := rateLimitFactory(common.GlobalWebRateLimitNum, common.GlobalWebRateLimitDuration, "GW") - return func(c *gin.Context) { - clientIP := c.ClientIP() - if clientIP == "127.0.0.1" || clientIP == "::1" || strings.HasPrefix(clientIP, "localhost") { - c.Next() - return - } - limiter(c) - } + return rateLimitFactory(common.GlobalWebRateLimitNum, common.GlobalWebRateLimitDuration, "GW") } return defNext }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/rate-limit.go` around lines 91 - 101, The GlobalWebRateLimit middleware currently bypasses the limiter for loopback IPs using ClientIP() (checks for "127.0.0.1", "::1", "localhost"), which is unsafe in reverse-proxy deployments; remove that bypass so the limiter(c) is always invoked, or if you need a bypass for local development, replace the IP checks with an explicit development-only feature flag (e.g., common.AllowLocalhostBypass) and gate the bypass on that flag only; update the function GlobalWebRateLimit and its use of rateLimitFactory to either delete the conditional IP-check block or change it to check the new explicit flag (and document that the flag must be true only in dev).model/user.go-630-635 (1)
630-635:⚠️ Potential issue | 🟠 MajorReturn the phone lookup error.
FillUserByPhonecurrently returnsnileven when no user exists or the query fails, which can make SMS login paths treat an unfilledUseras valid.Proposed fix
func (user *User) FillUserByPhone() error { - if user.Phone == "" { + user.Phone = strings.TrimSpace(user.Phone) + if user.Phone == "" { return errors.New("phone 为空！") } - DB.Where("phone = ?", user.Phone).First(user) - return nil + return DB.Where("phone = ?", user.Phone).First(user).Error }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/user.go` around lines 630 - 635, The FillUserByPhone function currently ignores the DB query result and always returns nil; change it to capture the GORM result (e.g. r := DB.Where("phone = ?", user.Phone).First(user)) and return r.Error so callers receive the lookup error (including not-found or other DB errors); keep the existing empty-phone check and return early if user.Phone == "".web/src/components/layout/SiderBar.jsx-203-205 (1)
203-205:⚠️ Potential issue | 🟠 MajorRespect sidebar module visibility for the admin section.
skill-marketis already part of the admin sidebar config, so hardcoding it to show for every admin bypasses the configured module visibility. The section render should also keep thehasSectionVisibleModules('admin')gate to avoid showing an admin group when all admin modules are disabled.Proposed fix
- // 根据配置过滤项目 + // 根据配置过滤项目 const filteredItems = items.filter((item) => { - // 技能管理是本地扩展核心入口，管理员下始终显示，避免被配置误隐藏。 - if (item.itemKey === 'skill-market') return isAdmin(); const configVisible = isModuleVisible('admin', item.itemKey); return configVisible; });- {/* 管理员区域 - 管理员始终显示，避免配置误隐藏 */} - {isAdmin() && ( + {/* 管理员区域 */} + {isAdmin() && hasSectionVisibleModules('admin') && (Based on learnings, the sidebar management system uses SidebarModulesAdmin configuration to control admin user permissions, and admin access to console modules should not be bypassed with hardcoded allowlists.
Also applies to: 487-488
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/layout/SiderBar.jsx` around lines 203 - 205, Remove the hardcoded always-show for the 'skill-market' item and instead use the normal visibility checks: rely on isModuleVisible('admin', item.itemKey) to decide per-item visibility (including 'skill-market') and ensure the admin group rendering is guarded by hasSectionVisibleModules('admin') so the entire admin section is hidden when no admin modules are enabled; update the logic around item.itemKey === 'skill-market' and the admin group render to remove the special-case and use isModuleVisible and hasSectionVisibleModules respectively.model/user.go-37-37 (1)
37-37:⚠️ Potential issue | 🟠 MajorMake phone ownership unambiguous.
phoneis only indexed, andIsPhoneAlreadyTakenreturnsfalsewhen duplicate rows already exist becauseRowsAffectedwould be greater than 1. SMS login should treat any existing phone as taken and the write path should enforce one active owner per phone.Minimum local fix
func IsPhoneAlreadyTaken(phone string) bool { - return DB.Unscoped().Where("phone = ?", phone).Find(&User{}).RowsAffected == 1 + phone = strings.TrimSpace(phone) + if phone == "" { + return false + } + return DB.Unscoped().Where("phone = ?", phone).Find(&User{}).RowsAffected > 0 }Also add a cross-database-safe uniqueness strategy for non-empty phone bindings during registration/binding; avoid a plain unique index while unbound users are stored as
"", because that would block multiple unbound users.Also applies to: 638-640
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/user.go` at line 37, The Phone field and IsPhoneAlreadyTaken logic make ownership ambiguous—update IsPhoneAlreadyTaken to treat any existing non-empty phone row as taken (RowsAffected > 0), and in all registration/binding write paths (the functions that assign/validate Phone during registration/binding) enforce one active owner by performing a transactional check-and-set (e.g., SELECT ... FOR UPDATE or an equivalent DB lock) before inserting/updating; also change the storage/uniqueness strategy so unbound users do not use the empty string (use NULL or a marker) and add a cross-database-safe uniqueness constraint for non-empty phones (e.g., partial/filtered unique index where phone IS NOT NULL / phone <> '' or enforce uniqueness at the application level within the same transaction) to ensure no two active owners can exist for the same phone.web/src/components/auth/SmsLoginForm.jsx-85-87 (1)
85-87:⚠️ Potential issue | 🟠 MajorSend the Turnstile token under the parameter name the middleware reads.
The routes use
TurnstileCheck(), and the provided middleware readsturnstile_token, but this component sendsturnstile. SMS login will fail whenever Turnstile is enabled.Proposed fix
const res = await API.post( - `/api/sms/send?turnstile=${turnstileToken}`, + `/api/sms/send?turnstile_token=${turnstileToken}`, { phone }, ); @@ const res = await API.post( - `/api/user/login/sms?turnstile=${turnstileToken}`, + `/api/user/login/sms?turnstile_token=${turnstileToken}`, { phone, code }, );Also applies to: 119-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/auth/SmsLoginForm.jsx` around lines 85 - 87, In SmsLoginForm.jsx update the API.post calls that currently send the Turnstile token as turnstile to use the parameter name the middleware expects (turnstile_token); locate the two occurrences around the API.post calls in the SmsLoginForm component and change the query param from turnstile=${turnstileToken} to turnstile_token=${turnstileToken} so TurnstileCheck() can read the token correctly.common/sms/tencent.go-118-126 (1)
118-126:⚠️ Potential issue | 🟠 MajorTreat an empty Tencent send status as a failed send.
If Tencent returns no top-level
Errorbut also noSendStatusSet, this returnsnileven though no phone delivery status was confirmed.Proposed fix
if result.Response.Error != nil { return fmt.Errorf("tencent SMS error: %s - %s", result.Response.Error.Code, result.Response.Error.Message) } - if len(result.Response.SendStatusSet) > 0 && result.Response.SendStatusSet[0].Code != "Ok" { + if len(result.Response.SendStatusSet) == 0 { + return fmt.Errorf("tencent SMS send error: empty SendStatusSet") + } + if result.Response.SendStatusSet[0].Code != "Ok" { return fmt.Errorf("tencent SMS send error: %s - %s", result.Response.SendStatusSet[0].Code, result.Response.SendStatusSet[0].Message) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/sms/tencent.go` around lines 118 - 126, The current return path treats a response with no top-level Error and an empty SendStatusSet as success; update the check around result.Response.SendStatusSet so that if len(result.Response.SendStatusSet) == 0 you return an error indicating "tencent SMS send error: empty SendStatusSet" (or similar) before the existing check that inspects SendStatusSet[0].Code, ensuring you still handle result.Response.Error first and then treat missing send status as a failed send (references: result.Response.Error and result.Response.SendStatusSet).common/sms/aliyun.go-69-73 (1)
69-73:⚠️ Potential issue | 🟠 MajorSet a timeout for Aliyun SMS requests.
Both
http.Getcalls in the SMS sender modules use the default client with no timeout, allowing stalled Aliyun connections to block login/send-code requests indefinitely. Apply the same fix to bothcommon/sms/aliyun.go(line 69) andcommon/sms/aliyun_pnvs.go(line 82).Proposed fix
- resp, err := http.Get(reqURL) + client := &http.Client{Timeout: 10 * time.Second} + resp, err := client.Get(reqURL)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/sms/aliyun.go` around lines 69 - 73, Replace use of the default http.Get (which has no timeout) with an HTTP request that enforces a timeout: create a local http.Client with a reasonable Timeout (e.g., a few seconds) and call client.Get instead of http.Get, or perform the request with a context with deadline and http.NewRequest; update the call in common/sms/aliyun.go (the resp, err := http.Get(reqURL) site) and apply the identical change to common/sms/aliyun_pnvs.go (the similar http.Get usage) so stalled Aliyun connections cannot block processing indefinitely.common/sms/tencent.go-7-7 (1)
7-7:⚠️ Potential issue | 🟠 MajorUse
common.Marshal/common.Unmarshalhere.This file directly imports and calls
encoding/jsonin business code. Use the project wrappers instead. As per coding guidelines, "All JSON marshal/unmarshal operations MUST use wrapper functions fromcommon/json.go(common.Marshal,common.Unmarshal,common.UnmarshalJsonStr,common.DecodeJson,common.GetJsonType). Do NOT directly import or callencoding/jsonin business code."Proposed fix
- "encoding/json" "fmt" "io" "net/http" @@ "time" + + "github.com/QuantumNous/new-api/common" ) @@ - payloadBytes, err := json.Marshal(payload) + payloadBytes, err := common.Marshal(payload) @@ - if err := json.Unmarshal(body, &result); err != nil { + if err := common.Unmarshal(body, &result); err != nil { return fmt.Errorf("tencent SMS parse response failed: %w", err) }Also applies to: 45-48, 114-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/sms/tencent.go` at line 7, Replace direct use of encoding/json in this file: remove the "encoding/json" import and replace all json.Marshal/json.Unmarshal calls used when building the SMS request payload and when parsing Tencent API responses with the project wrappers common.Marshal and common.Unmarshal; specifically update the code paths that serialize the request body (where the payload is constructed) and the code that decodes the response into structs to call common.Marshal(...) and common.Unmarshal(..., &target) respectively, and ensure the import for the common package is added.common/sms/aliyun.go-7-7 (1)
7-7:⚠️ Potential issue | 🟠 MajorReplace direct
encoding/jsonwith wrapper function fromcommon/json.go.This file imports
encoding/jsondirectly and callsjson.Unmarshalat line 84. Usecommon.Unmarshalinstead to maintain consistency with the project's JSON handling. The coding guidelines require all JSON operations to use wrapper functions fromcommon/json.go.Proposed fix
- "encoding/json" "fmt" "io" "net/http" @@ "time" + "github.com/QuantumNous/new-api/common" "github.com/google/uuid" @@ - if err := json.Unmarshal(body, &result); err != nil { + if err := common.Unmarshal(body, &result); err != nil { return fmt.Errorf("aliyun SMS parse response failed: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/sms/aliyun.go` at line 7, The file currently imports "encoding/json" and calls json.Unmarshal (the call at json.Unmarshal around line 84); replace that usage with the project's JSON wrapper by removing the "encoding/json" import and importing the project's common package, then call common.Unmarshal instead of json.Unmarshal (preserving the same arguments and error handling). Ensure any variable names and error checks around the Unmarshal call remain unchanged and that import grouping/aliases are adjusted so the file compiles with common.Unmarshal from common/json.go.controller/client_skills.go-202-205 (1)
202-205:⚠️ Potential issue | 🟠 MajorCheck
RowsAffectedbefore reporting a download as recorded.The model helper returns only
error, so GORM can update zero rows for a missing/disabled/non-public skill and this handler still returnsrecorded: true. Return rows affected from the model helper or perform the update here and requireRowsAffected > 0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/client_skills.go` around lines 202 - 205, The handler currently treats any nil error from model.IncrementClientSkillMarketDownload(id) as a successful recorded download even if GORM updated zero rows; change the model helper IncrementClientSkillMarketDownload to return (int64, error) (or return the gorm.Result), or perform the DB update in this handler, and check the returned RowsAffected > 0 before calling common.ApiSuccess(c, gin.H{"recorded": true}); if RowsAffected == 0 return recorded: false or an appropriate error response instead. Ensure you adjust the call site in this handler (the code using IncrementClientSkillMarketDownload) to handle the new return signature.middleware/sms_rate_limit.go-74-87 (1)
74-87:⚠️ Potential issue | 🟠 MajorKeep the phone-based SMS limit in the in-memory fallback too.
When Redis is disabled, this path only limits by IP, so a single phone can receive up to the per-IP burst and distributed IPs bypass the per-phone protection entirely. Mirror the phone key limit here as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/sms_rate_limit.go` around lines 74 - 87, memorySmsRateLimiter currently only enforces IP-based in-memory limits; also enforce the per-phone in-memory limit by reading the phone number from the request (e.g., the same param used in your send-SMS handler), build a phone key using SmsRateLimitMark + ":phone:" + phone, and call inMemoryRateLimiter.Request with the phone-specific limits (SmsMaxRequestsPerPhone, SmsPhoneDuration); if that request returns false, respond with the same 429 JSON and abort just like the IP branch. Ensure you perform the phone check before c.Next() and keep the same error response shape.controller/sms_login.go-119-143 (1)
119-143:⚠️ Potential issue | 🟠 MajorAvoid deriving usernames from
GetMaxUserId()+1.Concurrent SMS auto-registrations can compute the same
nextId, causing duplicatesms_<id>usernames and failed registration. Use a collision-resistant username derived from the phone hash/random suffix, or retry on duplicate-key errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/sms_login.go` around lines 119 - 143, The current registration derives username from GetMaxUserId()+1 which can collide under concurrent SMS auto-registrations; change User creation to generate a collision-resistant username (e.g., hash of req.Phone plus short random suffix or a UUID) instead of fmt.Sprintf("sms_%d", nextId), or implement a retry-on-duplicate-key loop around newUser.Insert that regenerates the username and retries until Insert succeeds; update references in this block (GetMaxUserId usage, the username creation logic, and the call to newUser.Insert) and ensure GenerateKey/DisplayName logic remains unchanged while properly handling and logging duplicate-key errors returned by model.User.Insert.controller/client_auth.go-104-122 (1)
104-122:⚠️ Potential issue | 🟠 MajorAvoid
GetMaxUserId()+1for auto-generated usernames.Concurrent client SMS registrations can compute the same next id and collide on
sms_<id>. Generate a unique phone-derived/random username or retry on duplicate-key errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/client_auth.go` around lines 104 - 122, The current username generation using GetMaxUserId()+1 (username := fmt.Sprintf("sms_%d", nextId)) can collide under concurrent SMS registrations; change username creation to produce a unique value (for example derive from the phone plus a random/UUID suffix or use a secure short random token from common.GenerateKey instead of nextId) and update the flow around model.User.Insert to handle duplicate-key errors by retrying username generation a few times (identify duplicate-key error returned by newUser.Insert and regenerate username and call Insert again, failing after N attempts). Ensure you reference and update the GetMaxUserId usage, the username construction, common.GenerateKey usage, and the newUser.Insert error handling to implement the retry/unique-username strategy.middleware/sms_rate_limit.go-26-52 (1)
26-52:⚠️ Potential issue | 🟠 MajorParse the JSON body before applying the phone limiter.
SendSmsVerificationaccepts JSON, but this middleware only readsc.PostForm("phone"), so normal JSON requests skip theSmsMaxRequestsPerPhonelimit and only hit the coarser IP limit. Read and restore the request body, then decodephone, or make the client submit form data consistently.🛡️ Proposed direction
+// Read JSON body without consuming it for the controller, then apply the same +// phone limiter to both JSON and form requests. phone := c.PostForm("phone") +if phone == "" && strings.Contains(c.GetHeader("Content-Type"), "application/json") { + body, _ := io.ReadAll(c.Request.Body) + c.Request.Body = io.NopCloser(bytes.NewReader(body)) + var req struct { + Phone string `json:"phone"` + } + if err := common.Unmarshal(body, &req); err == nil { + phone = req.Phone + } + c.Request.Body = io.NopCloser(bytes.NewReader(body)) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/sms_rate_limit.go` around lines 26 - 52, The phone-based limiter in middleware/sms_rate_limit.go currently only reads c.PostForm("phone") so JSON requests bypass SmsMaxRequestsPerPhone; before using phone in the phone limiter block, read and buffer c.Request.Body, decode the JSON body to extract the "phone" field (falling back to c.PostForm("phone") if not present), then restore the request body for downstream handlers and continue using the existing phoneKey/Incr/Expire/TTL logic with SmsRateLimitMark, SmsPhoneDuration and SmsMaxRequestsPerPhone; ensure any body read errors fall back to IP-only limiting and do not disrupt the request flow.controller/sms_login.go-54-66 (1)
54-66:⚠️ Potential issue | 🟠 MajorOnly register the verification code after the SMS is accepted for sending.
The code is stored before
sms.NewSender()andsender.SendCode(). If provider config or delivery fails, an unsent code remains valid and may also replace a previously deliverable code.🛡️ Proposed fix
code := common.GenerateVerificationCode(6) -common.RegisterVerificationCodeWithKey(req.Phone, code, common.SmsVerificationPurpose) sender, err := sms.NewSender() if err != nil { common.ApiErrorI18n(c, i18n.MsgSmsProviderNotConfig) return } if err := sender.SendCode(req.Phone, code); err != nil { common.SysLog("SMS send failed: " + err.Error()) common.ApiErrorI18n(c, i18n.MsgSmsSendFailed) return } +common.RegisterVerificationCodeWithKey(req.Phone, code, common.SmsVerificationPurpose)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/sms_login.go` around lines 54 - 66, The verification code is registered before ensuring the SMS provider is configured and the message is accepted; move the call to common.RegisterVerificationCodeWithKey so it only happens after sms.NewSender() succeeds and sender.SendCode(req.Phone, code) returns nil. In other words: call common.GenerateVerificationCode(6), create the sender via sms.NewSender(), call sender.SendCode(...), and only when SendCode returns no error invoke common.RegisterVerificationCodeWithKey(req.Phone, code, common.SmsVerificationPurpose); keep existing error handling for sms.NewSender and sender.SendCode unchanged.controller/client_skills.go-148-192 (1)
148-192:⚠️ Potential issue | 🟠 MajorDo not fall back to bundled public skills after the DB returns empty or not-found.
If an admin disables/removes all public skills,
loadClientPublicSkills()still returnsdefaultClientPublicSkills; similarly, a disabled/missing id can be served from the fallback. That makes admin visibility controls ineffective. Prefer seeding defaults into the DB, or only use fallback on actual DB errors.🛡️ Proposed fix
if err != nil { common.SysError("load client public skills from db failed: " + err.Error()) return defaultClientPublicSkills } -if len(items) == 0 { - return defaultClientPublicSkills -} @@ -if err != nil && !errors.Is(err, gorm.ErrRecordNotFound) { +if err != nil && !errors.Is(err, gorm.ErrRecordNotFound) { common.SysError("get client public skill from db failed: " + err.Error()) + for _, skill := range defaultClientPublicSkills { + if skill.ID == id { + common.ApiSuccess(c, skill) + return + } + } } - -for _, skill := range defaultClientPublicSkills { - if skill.ID == id { - common.ApiSuccess(c, skill) - return - } -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/client_skills.go` around lines 148 - 192, loadClientPublicSkills and ClientGetPublicSkill currently fall back to defaultClientPublicSkills when the DB returns an empty list or a not-found id, which bypasses admin removals; change logic so that loadClientPublicSkills returns an empty slice when model.ListPublicClientSkillMarketItems succeeds but returns zero items (only use defaultClientPublicSkills when ListPublicClientSkillMarketItems returns an actual error), and change ClientGetPublicSkill to only use defaultClientPublicSkills when model.GetPublicClientSkillMarketItemByID returns an error indicative of a DB failure (not when it returns gorm.ErrRecordNotFound); reference functions/vars: loadClientPublicSkills, ClientGetPublicSkill, defaultClientPublicSkills, model.ListPublicClientSkillMarketItems, model.GetPublicClientSkillMarketItemByID.common/sms/aliyun_pnvs.go-82-86 (1)
82-86:⚠️ Potential issue | 🟠 MajorAdd HTTP timeout and status code validation to the Aliyun PNVS request.
The
http.Get()call uses the default client which has no timeout, allowing a stalled provider to block the request handler indefinitely. Additionally, the response body is parsed without validating the HTTP status code, so 4xx/5xx responses with invalid JSON will produce misleading error messages.Add a timeout to the client and validate the status code before decoding the response:
Proposed fix
- resp, err := http.Get(reqURL) + client := &http.Client{Timeout: 10 * time.Second} + resp, err := client.Get(reqURL) if err != nil { return fmt.Errorf("aliyun PNVS SMS request failed: %w", err) } defer resp.Body.Close() + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return fmt.Errorf("aliyun PNVS SMS HTTP error: %d", resp.StatusCode) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/sms/aliyun_pnvs.go` around lines 82 - 86, Replace the use of the default http.Get with an explicit http.Client that has a sensible Timeout (e.g., 5-10s) and use client.Get(reqURL) instead of http.Get(reqURL); after receiving resp check resp.StatusCode and if it is not in the 2xx range read/limit the resp.Body to capture a short error snippet and return a clear error including the status code and snippet, then only proceed to decode the body when the status is OK; ensure resp.Body.Close() is still deferred.
🟡 Minor comments (11)
scripts/sync-db-push.sh-47-51 (1)
47-51:⚠️ Potential issue | 🟡 MinorUse
read -rto avoid backslash mangling.Shellcheck SC2162. Trivial fix:
-read -p "确认上传并重启服务？(输入 yes 继续): " answer +read -r -p "确认上传并重启服务？(输入 yes 继续): " answer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sync-db-push.sh` around lines 47 - 51, The prompt uses plain read which allows backslash interpretation; change the interactive prompt to use read -r (i.e., replace the read invocation shown in the prompt line with read -r) so backslashes are not mangled and satisfy ShellCheck SC2162 while keeping the existing variable name (answer) and prompt text unchanged.setting/operation_setting/general_setting.go-30-30 (1)
30-30:⚠️ Potential issue | 🟡 MinorDefault currency flip (USD → CNY) is an observable behavior change for fresh installs.
For brand-new deployments and for any installation that hasn't persisted
quota_display_typeexplicitly, the admin panel, billing UI, logs, and API responses will now render quotas in CNY and applyConvertQuotaToCNY. For non-Chinese operators this will be surprising. Consider documenting this flip in the changelog/migration notes (and inCHANGELOG_FORK.mdwhich is part of this PR), and double-check thatcontroller/topup.go,controller/billing.go, andlogger/logger.goall produce sensible output under the new default.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setting/operation_setting/general_setting.go` at line 30, The default QuotaDisplayType is being set to QuotaDisplayTypeCNY which causes a behavioral change for fresh installs; revert the default back to QuotaDisplayTypeUSD in the general setting (replace QuotaDisplayTypeCNY with QuotaDisplayTypeUSD in the QuotaDisplayType default), and add a clear migration/changelog entry in CHANGELOG_FORK.md documenting the intentional currency default change if you decide to keep CNY; also review and update usages in controller/topup.go, controller/billing.go, and logger/logger.go to ensure they format and convert quotas correctly (run a quick test of API responses, admin UI, and logs to confirm no regressions).CHANGELOG_FORK.md-85-92 (1)
85-92:⚠️ Potential issue | 🟡 MinorAdd languages to the fenced code blocks.
markdownlintflags these fences; marking the HTTP examples and flow block keeps the new changelog lint-clean.🧹 Proposed documentation fix
-``` +```http POST /api/sms/send?turnstile={token} Content-Type: application/json { "phone": "+8613800138000" }...
-+http
POST /api/user/login/sms?turnstile={token}
Content-Type: application/json{
"phone": "+8613800138000",
"code": "123456"
}... -``` +```text 1. AionUi 打开弹窗： https://new-api.example.com/login?mode=popup&callback_origin=https://aionui.example.comAlso applies to: 104-112, 226-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG_FORK.md` around lines 85 - 92, The fenced code blocks in CHANGELOG_FORK.md are missing language specifiers; update each fence shown (the HTTP example starting with "POST /api/sms/send?turnstile={token}", the second block starting "POST /api/user/login/sms?turnstile={token}", and the flow block beginning "1. AionUi 打开弹窗：") to include appropriate languages (e.g., ```http for HTTP requests and ```text for the numbered flow) so markdownlint no longer flags them; keep the original block contents unchanged and only add the language tokens to the opening backtick fences.controller/option.go-191-198 (1)
191-198:⚠️ Potential issue | 🟡 MinorUse the existing SMS i18n key for this response.
This new SMS-specific message is hardcoded, while the SMS controllers already use
i18n.MsgSmsProviderNotConfig(controller/sms_login.go:59-71,i18n/keys.go:320-327). Keeping the same key avoids returning Chinese text in non-Chinese admin sessions.🌐 Proposed fix
+ "github.com/QuantumNous/new-api/i18n" "github.com/QuantumNous/new-api/model"case "sms.enabled": if option.Value == "true" && system_setting.GetSmsSettings().Provider == "" { c.JSON(http.StatusOK, gin.H{ "success": false, - "message": "无法启用短信登录，请先配置短信服务商！", + "message": i18n.T(c, i18n.MsgSmsProviderNotConfig), }) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/option.go` around lines 191 - 198, Replace the hardcoded Chinese message in the "sms.enabled" case with the existing SMS i18n key: use i18n.MsgSmsProviderNotConfig for the "message" field so the response follows the same localization used in controller/sms_login.go; update the block that checks option.Value == "true" and system_setting.GetSmsSettings().Provider == "" to return the i18n key rather than the literal string.middleware/cache.go-12-21 (1)
12-21:⚠️ Potential issue | 🟡 MinorDon’t attach no-cache legacy headers to immutable assets.
The
/assets/branch advertises long immutable caching, but lines 20-21 still addPragma: no-cacheandExpires: 0to those same responses. Keep those headers only on the non-asset branch.Proposed fix
if strings.HasPrefix(uri, "/assets/") { // 构建产物文件名带 hash，允许长缓存。 c.Header("Cache-Control", "public, max-age=604800, immutable") } else { // 页面路由（如 /console/skill-market）和其他入口统一禁用缓存， // 避免 IAB 持续命中旧前端代码导致“编辑空白”。 c.Header("Cache-Control", "no-store, no-cache, must-revalidate") + c.Header("Pragma", "no-cache") + c.Header("Expires", "0") } - c.Header("Pragma", "no-cache") - c.Header("Expires", "0") c.Header("Cache-Version", "skill-market-hotfix-20260419")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/cache.go` around lines 12 - 21, The current middleware sets long immutable caching for asset paths (strings.HasPrefix(uri, "/assets/")) but still unconditionally adds legacy no-cache headers via c.Header("Pragma", "no-cache") and c.Header("Expires", "0"); move those two header calls into the non-asset branch (the else block) so that only non-asset responses get "Pragma: no-cache" and "Expires: 0" while the assets branch only sets the immutable Cache-Control header using c.Header("Cache-Control", "public, max-age=604800, immutable").web/src/i18n/locales/zh-CN.json-2988-2994 (1)
2988-2994:⚠️ Potential issue | 🟡 MinorRemove duplicate locale keys.
请输入验证码,获取验证码, and两步验证already exist earlier in this locale file, so re-adding them creates duplicate JSON members.Proposed fix
- "请输入验证码": "请输入验证码", "请输入手机号和验证码": "请输入手机号和验证码", - "获取验证码": "获取验证码", "秒后重新获取": "秒后重新获取", "验证码发送成功": "验证码发送成功", "验证码发送失败": "验证码发送失败", - "两步验证": "两步验证", "允许通过短信验证码登录": "允许通过短信验证码登录",As per coding guidelines, use CLI tools:
bun run i18n:extract,bun run i18n:sync,bun run i18n:lint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/i18n/locales/zh-CN.json` around lines 2988 - 2994, Remove the duplicate JSON members by deleting the repeated locale keys "请输入验证码", "获取验证码", and "两步验证" from the added block so each key appears only once in the zh-CN locale; after removing those entries, run the i18n tooling to re-extract/sync/lint (bun run i18n:extract, bun run i18n:sync, bun run i18n:lint) to ensure no other duplicates or extraction issues remain.web/src/pages/SkillMarket/index.jsx-365-623 (1)
365-623:⚠️ Potential issue | 🟡 MinorWrap the new Skill Market UI strings with
t(...).This new page hard-codes table titles, button labels, filter labels, status text, and empty-state text, but it does not use
useTranslation(). These strings will be skipped by the i18n flow. As per coding guidelines, “Translation files inweb/src/i18n/locales/{lang}.jsonmust be flat JSON with Chinese source strings as keys. UseuseTranslation()hook and callt('中文key')in components.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/SkillMarket/index.jsx` around lines 365 - 623, The page hard-codes UI text in the columns constant and JSX (e.g., table column titles in columns, Tag labels, Button text, select option labels, Input placeholder, statusSummary/filteredSkills labels, Empty description) and must use i18n: import and call useTranslation() in this component and wrap each user-visible Chinese string with t('中文原文') (use the Chinese text itself as the key per locale guidelines). Update occurrences in the columns renderers (columns const), Button labels (openCreateEditor, loadSkills buttons, action Buttons), Switch labels, select option texts (status/category/page size options), Tag texts (statusSummary tags and status render), Empty.description, and any hard-coded tooltips or Popconfirm titles/content to use t('...'); keep dynamic values (numbers) unchanged and ensure keys match the flat locales JSON convention.web/src/pages/SkillMarket/EditSkillModal.jsx-13-289 (1)
13-289:⚠️ Potential issue | 🟡 MinorWrap user-facing copy with
useTranslation()andt(...).This component hardcodes all labels, buttons, status text, and error copy. Please use
react-i18nextso the skill market admin UI remains localizable. As per coding guidelines,web/src/**/*.{ts,tsx,js,jsx}must useuseTranslation()and callt('中文key')in components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/SkillMarket/EditSkillModal.jsx` around lines 13 - 289, The component EditSkillModal currently hardcodes all user-facing strings; update it to use react-i18next by calling useTranslation() inside EditSkillModal and replacing each literal label/button/error text with t('key') calls (e.g. title tags, Tag text, Typography.Title, all Input labels like "原始名称", "分类", placeholders, debug panel strings, buttons "保存"/"取消"/"预览技能页"/"立即上架"/"立即下架", error message wrapping editError, and the getMylclawNavLabel display). Ensure you import useTranslation from 'react-i18next', create meaningful i18n keys for these strings, and keep existing handlers (setEditingSkill, onSubmit, onCancel, updateSkillStatus, getPreviewUrl, statusLoadingMap) unchanged while swapping literals for t('...') calls so the UI remains localizable.web/src/pages/SkillMarket/Editor.jsx-92-490 (1)
92-490:⚠️ Potential issue | 🟡 MinorLocalize the editor page copy via
react-i18next.All labels, button text, loading/error messages, and toasts are hardcoded in Chinese. Please add
useTranslation()and wrap Chinese source strings witht(...). As per coding guidelines,web/src/**/*.{ts,tsx,js,jsx}must useuseTranslation()and callt('中文key')in components.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/SkillMarket/Editor.jsx` around lines 92 - 490, The component SkillMarketEditor currently contains hardcoded Chinese UI strings; import and use useTranslation from 'react-i18next' (e.g. const { t } = useTranslation()) at the top of the SkillMarketEditor component and replace all hardcoded Chinese literals in JSX, button labels, placeholders, status/loading/error messages, toast calls (showError/showSuccess), and the static build banner with t('...') keys; keep existing logic and identifiers (SkillMarketEditor, previewUrl, submitSkill, loadSkill, updateSkillStatus, updateField, isCreate, editingSkill) and use interpolation for dynamic strings (e.g. t('skillMarket.editor.titleWithId', { id: editingSkill.id || skillId })) and consistent key names like 'skillMarket.editor.save', 'skillMarket.editor.loading', 'skillMarket.editor.pageError', etc., so the UI strings are localized via react-i18next.common/sms/aliyun_pnvs.go-7-7 (1)
7-7:⚠️ Potential issue | 🟡 MinorUse the project JSON wrappers instead of
encoding/json.Replace the direct import and
json.Unmarshalcall withcommon.Unmarshalto keep behavior consistent with the rest of the codebase. All Go JSON marshal/unmarshal operations must use wrapper functions fromcommon/json.go.♻️ Proposed fix
import ( "crypto/hmac" "crypto/sha1" "encoding/base64" - "encoding/json" "fmt" "io" "net/http" @@ + "github.com/QuantumNous/new-api/common" "github.com/google/uuid" ) @@ - if err := json.Unmarshal(body, &result); err != nil { + if err := common.Unmarshal(body, &result); err != nil { return fmt.Errorf("aliyun PNVS SMS parse response failed: %w", err) }Also applies to: 101-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/sms/aliyun_pnvs.go` at line 7, Replace direct use of the standard library JSON package with the project's JSON wrapper: remove the "encoding/json" import and import the project's common JSON wrapper package (the package that exposes Unmarshal), then replace calls to json.Unmarshal (and any json.Marshal usages) with common.Unmarshal (and common.Marshal) in this file—specifically update the json.Unmarshal usages around the current occurrences (including the ones referenced at lines ~101-103) so they call common.Unmarshal and match the wrapper's signature.controller/client_skills.go-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorReplace
json.Marshalwithcommon.Marshalwrapper function.Line 96 in
tagsToJSONdirectly callsjson.Marshal. Per project guidelines, all JSON marshal/unmarshal operations in Go files must use wrapper functions fromcommon/json.go. Remove the directencoding/jsonimport at line 4 and usecommon.Marshalinstead.Proposed fix
import ( - "encoding/json" "errors" "strconv" "strings" "github.com/QuantumNous/new-api/common" "github.com/QuantumNous/new-api/model" "github.com/gin-gonic/gin" "gorm.io/gorm" )func tagsToJSON(tags []string) (model.JSONValue, error) { if tags == nil { tags = []string{} } - tagBytes, err := json.Marshal(tags) + tagBytes, err := common.Marshal(tags) if err != nil { return nil, err } return model.JSONValue(tagBytes), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/client_skills.go` at line 4, The function tagsToJSON currently uses json.Marshal directly and imports "encoding/json"; replace that call with the project wrapper common.Marshal and remove the direct "encoding/json" import. Update tagsToJSON to call common.Marshal(tags) (handle the returned ([]byte, error) exactly as before) and import the package that provides common.Marshal (or qualify it with the existing common package) instead of using json.Marshal; ensure error handling remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15532ba0-f599-43ae-9073-67f368e8fdf2
⛔ Files ignored due to path filters (1)
web/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
CHANGELOG_FORK.mdcommon/sms/aliyun.gocommon/sms/aliyun_pnvs.gocommon/sms/sms.gocommon/sms/tencent.gocommon/verification.gocontroller/client_auth.gocontroller/client_skillhub.gocontroller/client_skills.gocontroller/misc.gocontroller/option.gocontroller/sms_login.gocontroller/subscription.gocontroller/user.goi18n/keys.goi18n/locales/en.yamli18n/locales/zh-CN.yamli18n/locales/zh-TW.yamlmiddleware/auth.gomiddleware/cache.gomiddleware/cors.gomiddleware/rate-limit.gomiddleware/sms_rate_limit.gomodel/client_skill_market.gomodel/main.gomodel/option.gomodel/subscription.gomodel/user.gorouter/api-router.goscripts/sync-db-pull.shscripts/sync-db-push.shsetting/operation_setting/general_setting.gosetting/system_setting/sms.goweb/src/App.jsxweb/src/components/auth/LoginForm.jsxweb/src/components/auth/SmsLoginForm.jsxweb/src/components/common/logo/PhoneIcon.jsxweb/src/components/layout/SiderBar.jsxweb/src/components/settings/SystemSetting.jsxweb/src/helpers/auth.jsxweb/src/helpers/render.jsxweb/src/hooks/common/useSidebar.jsweb/src/i18n/locales/en.jsonweb/src/i18n/locales/zh-CN.jsonweb/src/pages/SkillMarket/EditSkillModal.jsxweb/src/pages/SkillMarket/Editor.jsxweb/src/pages/SkillMarket/index.jsx
| // 与 SmsLogin 的区别: | ||
| // - 不创建 cookie session（客户端不需要） | ||
| // - 自动确保用户拥有一个名为 "MyClaw Client" 的 Token，并返回完整 Key | ||
| // - 不支持 2FA 流程（客户端登录场景简化） | ||
| func ClientSmsLogin(c *gin.Context) { |
There was a problem hiding this comment.
Do not bypass 2FA when issuing a client API key.
This endpoint skips the 2FA flow but returns a long-lived API key. Accounts with 2FA enabled can therefore authenticate with only SMS here, unlike SmsLogin, which checks model.IsTwoFAEnabled(user.Id) before completing login.
🛡️ Proposed direction
if user.Status != common.UserStatusEnabled {
common.ApiErrorI18n(c, i18n.MsgUserDisabled)
return
}
+
+if model.IsTwoFAEnabled(user.Id) {
+ common.ApiErrorI18n(c, i18n.MsgUserRequire2FA)
+ return
+}
// 6. 查找或创建客户端专用 Token
apiKey, err := ensureClientToken(user.Id, user.Username)Also applies to: 135-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/client_auth.go` around lines 55 - 59, ClientSmsLogin currently
skips the 2FA check and issues a long‑lived API key; modify ClientSmsLogin (and
the similar logic around lines 135-147) to call model.IsTwoFAEnabled(user.Id)
after verifying the SMS code and before creating or returning the "MyClaw
Client" token, and if 2FA is enabled reject the flow (return an appropriate
2FA-required error) instead of issuing the API key; mirror the same 2FA gate
used in SmsLogin to ensure users with 2FA enabled cannot obtain the client API
key via SMS-only login.
| # 2. 上传 | ||
| echo "==> 上传中..." | ||
| scp -q "$LOCAL_PATH" "${REMOTE_HOST}:${REMOTE_PATH}" | ||
| echo "✓ 上传完成" | ||
|
|
||
| # 3. 重启服务 | ||
| echo "==> 重启服务 ${REMOTE_SERVICE}..." | ||
| ssh "$REMOTE_HOST" "systemctl restart ${REMOTE_SERVICE} && sleep 2 && systemctl is-active ${REMOTE_SERVICE}" || { | ||
| echo "✗ 服务重启失败，尝试回滚..." | ||
| ssh "$REMOTE_HOST" "cp ${REMOTE_PATH}.bak.${TIMESTAMP} ${REMOTE_PATH} && systemctl restart ${REMOTE_SERVICE}" | ||
| exit 1 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
SQLite WAL mode corruption when database file is replaced while process is open
💡 Result:
Replacing the main SQLite database file (e.g., db.sqlite) while a process has it open in WAL mode will likely cause corruption or data loss. SQLite connections maintain open file handles to the main database, -wal, and -shm files, using a shared memory region (-shm) and reading changes from the WAL file. Directly overwriting the main file bypasses these mechanisms, leading to inconsistencies between the process's in-memory state, WAL content, and the new main file. Key risks from official documentation: - WAL is part of the database's persistent state; separating the main file from its WAL can cause committed transactions to be lost or the database to corrupt. - File overwrite by external processes is a primary corruption vector. - Processes detect WAL mode via the database header (version bytes 18-19 set to 2); a mismatched replacement file may trigger errors like "file is encrypted or is not a database" or malformed database. Supported safe alternatives: - Use the Online Backup API (sqlite3_backup_init etc.) to create a consistent snapshot without locking the source for extended periods. This copies main + WAL content atomically into a new file. - VACUUM INTO 'newfile'; (SQLite 3.27+) for a vacuumed live copy. - Close all connections, replace files (main + -wal + -shm if present), then reopen. Unsupported hacks (e.g., overwriting -wal/-shm or PRAGMA journal_mode=DELETE tricks) risk corruption and are not recommended, as confirmed in SQLite forums. To replace while processes are open, signal them to close/reopen connections after preparing a consistent replacement via backup API.
Citations:
- 1: https://sqlite.cn/howtocorrupt.html
- 2: https://www.sqlite.org/wal.html
- 3: https://sqlite.org/forum/info/6884c63086e0528e
- 4: https://www.sqlite.org/backup.html
- 5: https://sqlite.org/forum/info/f7e12e17fc74dfa0
🏁 Script executed:
# Verify the file exists and check the relevant section
if [ -f scripts/sync-db-push.sh ]; then
echo "File exists. Showing lines 1-80 with line numbers:"
head -80 scripts/sync-db-push.sh | cat -n
else
echo "File not found at scripts/sync-db-push.sh"
# Try to find it
fd sync-db-push.sh
fiRepository: QuantumNous/new-api
Length of output: 2998
🏁 Script executed:
# Search for SQLite configuration or WAL mode settings in the codebase
rg -i "pragma.*journal_mode|wal|synchronous" --type-add 'config:*.{toml,yaml,yml,conf,ini}' -t go -t js -t python -t configRepository: QuantumNous/new-api
Length of output: 93
🏁 Script executed:
# Search for SQLite configuration or WAL mode settings
rg -i "pragma.*journal_mode|wal|synchronous" --type go --type jsRepository: QuantumNous/new-api
Length of output: 6030
🏁 Script executed:
# Search for database initialization or SQLite pragma settings
rg "gorm\.|db\.|sqlite" --type go -A 3 -B 1 | grep -i "pragma\|journal\|wal\|gorm.Open\|sqlite" | head -40Repository: QuantumNous/new-api
Length of output: 1257
🏁 Script executed:
# Check the database initialization file for SQLite pragma settings
sed -n '1,100p' model/main.go | cat -nRepository: QuantumNous/new-api
Length of output: 2976
🏁 Script executed:
# Search more thoroughly for SQLite pragma settings or DSN configuration
rg "sqlite.Open|DSN|ParseTime|pragma|journal_mode" model/main.go -A 5 -B 2Repository: QuantumNous/new-api
Length of output: 1637
🏁 Script executed:
# Search for SQLite connection string or initialization with parameters
rg "SQLitePath|cache=|mode=" --type go -A 1 -B 1 | head -40Repository: QuantumNous/new-api
Length of output: 842
🏁 Script executed:
# Check the common/database.go file to see the full SQLite configuration
cat -n common/database.go | grep -A 5 -B 5 SQLitePathRepository: QuantumNous/new-api
Length of output: 315
Critical data-corruption risk: Database is overwritten while the service is still running.
The script uploads one-api.db (line 61) while the remote service continues running, which is unsafe for SQLite:
- If WAL mode is enabled (indicated by
.db-shm/.db-walreferences in the codebase), the running process maintains open file handles to those WAL/SHM files. Overwriting the main DB file creates a mismatch between the file and the WAL, leading to corruption or silent data loss on the next write. scptruncates-and-writes; concurrent writes from the running process during upload create indeterminate file state.- Line 39's remote query against a running server may also hold locks and read inconsistent state.
The service must be stopped before upload to release all DB locks and flush any WAL data:
Recommended fix
+# Stop service first to release DB locks and flush WAL
+ssh "$REMOTE_HOST" "systemctl stop ${REMOTE_SERVICE}"
+
# 1. 在服务器上备份
TIMESTAMP=$(date +%Y%m%d-%H%M%S)
echo "==> 服务器端备份现有 db..."
-ssh "$REMOTE_HOST" "cp ${REMOTE_PATH} ${REMOTE_PATH}.bak.${TIMESTAMP}"
+ssh "$REMOTE_HOST" "cp '${REMOTE_PATH}' '${REMOTE_PATH}.bak.${TIMESTAMP}' \
+ && rm -f '${REMOTE_PATH}-wal' '${REMOTE_PATH}-shm'"
echo "✓ 备份完成: ${REMOTE_PATH}.bak.${TIMESTAMP}"
# 2. 上传
echo "==> 上传中..."
scp -q "$LOCAL_PATH" "${REMOTE_HOST}:${REMOTE_PATH}"
echo "✓ 上传完成"
# 3. 重启服务
-echo "==> 重启服务 ${REMOTE_SERVICE}..."
-ssh "$REMOTE_HOST" "systemctl restart ${REMOTE_SERVICE} && sleep 2 && systemctl is-active ${REMOTE_SERVICE}" || {
+echo "==> 启动服务 ${REMOTE_SERVICE}..."
+ssh "$REMOTE_HOST" "systemctl start ${REMOTE_SERVICE} && sleep 2 && systemctl is-active ${REMOTE_SERVICE}" || {
echo "✗ 服务重启失败，尝试回滚..."
- ssh "$REMOTE_HOST" "cp ${REMOTE_PATH}.bak.${TIMESTAMP} ${REMOTE_PATH} && systemctl restart ${REMOTE_SERVICE}"
+ ssh "$REMOTE_HOST" "cp '${REMOTE_PATH}.bak.${TIMESTAMP}' '${REMOTE_PATH}' && systemctl start ${REMOTE_SERVICE}"
exit 1
}Also add a trap to handle Ctrl-C between stop and start, which would otherwise leave the service down.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 66-66: Note that, unescaped, this expands on the client side.
(SC2029)
[info] 68-68: Note that, unescaped, this expands on the client side.
(SC2029)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/sync-db-push.sh` around lines 59 - 70, The script uploads the
database while the remote service is still running, risking SQLite WAL/SHM
corruption; modify the flow in the block using REMOTE_SERVICE, REMOTE_HOST,
REMOTE_PATH and scp so you first stop the remote service (systemctl stop
${REMOTE_SERVICE}), verify it is inactive (systemctl is-active), create a backup
of the remote DB (e.g. cp ${REMOTE_PATH} ${REMOTE_PATH}.bak.${TIMESTAMP}), then
perform scp of LOCAL_PATH to REMOTE_PATH, and only then start the service
(systemctl start/restart ${REMOTE_SERVICE}) and verify it became active; add a
trap on the local script to handle interrupts between stop and start that will
attempt to restore the backup and restart the service on failure or Ctrl-C, and
in the existing ssh failure branch ensure you restore the .bak.${TIMESTAMP}
backup before attempting to restart.
| // 弹窗模式：登录成功后获取 token 并回传给 opener | ||
| const handlePopupCallback = async (userData) => { | ||
| if (!window.opener || !callbackOrigin) { | ||
| showSuccess('登录成功！'); | ||
| navigate('/console'); | ||
| return; | ||
| } | ||
| try { | ||
| // 1. 获取 Access Token | ||
| const tokenRes = await API.get('/api/user/token'); | ||
| const accessToken = tokenRes.data.success ? tokenRes.data.data : ''; | ||
|
|
||
| // 2. 获取 API Token 列表 | ||
| const tokensRes = await API.get('/api/token/'); | ||
| let apiTokenKey = ''; | ||
| let tokens = tokensRes.data?.data?.items || tokensRes.data?.data || []; | ||
| if (tokens.length === 0) { | ||
| // 没有 Token，创建一个 | ||
| const createRes = await API.post('/api/token/', { | ||
| name: 'AionUi Default', | ||
| remain_quota: 0, | ||
| expired_time: -1, | ||
| unlimited_quota: true, | ||
| }); | ||
| if (createRes.data.success) { | ||
| const newTokensRes = await API.get('/api/token/'); | ||
| tokens = newTokensRes.data?.data?.items || newTokensRes.data?.data || []; | ||
| } | ||
| } | ||
| if (tokens.length > 0) { | ||
| // 获取第一个 token 的 key | ||
| const keyRes = await API.post(`/api/token/${tokens[0].id}/key`); | ||
| if (keyRes.data.success) { | ||
| apiTokenKey = keyRes.data.data; | ||
| } | ||
| } | ||
|
|
||
| // 3. postMessage 回传 | ||
| window.opener.postMessage({ | ||
| type: 'aionui-auth', | ||
| accessToken, | ||
| apiToken: apiTokenKey, | ||
| user: userData, | ||
| }, callbackOrigin); |
There was a problem hiding this comment.
Do not send tokens to an untrusted callback_origin, and fix token response parsing.
Line 525 trusts callback_origin from the URL, so any site can open the login popup with its own origin and receive accessToken / apiToken after the user logs in. Validate the target origin against a server-controlled allowlist or a signed login state before calling postMessage.
Also, lines 547 and 555 expect { success, data }, but the provided token handlers return { id, key, data } for token creation and the token object directly for /api/token/:id/key; this can send an empty apiToken to the opener.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/auth/LoginForm.jsx` around lines 523 - 566, The
handlePopupCallback flow currently trusts callbackOrigin and mis-parses token
responses; before calling window.opener.postMessage validate callbackOrigin
against a server-controlled allowlist or a server-provided signed state (e.g.,
compare callbackOrigin to a validated origin returned from your login/session
endpoint or verify a signed callback token stored in state) and abort sending
any tokens if validation fails, and only send a minimal user payload when
untrusted. Also fix token parsing: treat the /api/token/ POST response as
returning {id, key, data} (use the returned key if present) and treat the
/api/token/:id/key response as returning the token string directly (or check
both shapes), so set apiTokenKey from keyRes.data.key or keyRes.data.data or
keyRes.data as appropriate, and similarly extract tokens from
tokensRes.data?.data?.items or tokensRes.data?.data ensuring you handle both
array and object shapes; only call /api/token/:id/key when you have a valid
token id and include robust checks before attaching apiToken to the postMessage.
| // Secret field: only send if non-empty | ||
| if (inputs['sms.access_key_secret'] !== '') { | ||
| options.push({ | ||
| key: 'sms.access_key_secret', | ||
| value: inputs['sms.access_key_secret'], | ||
| }); | ||
| } |
There was a problem hiding this comment.
Avoid sending an undefined SMS secret.
Because setInputs(newInputs) replaces the initial defaults, inputs['sms.access_key_secret'] can be undefined when the backend does not return secrets. Line 613 then evaluates undefined !== '' as true and sends the secret option with an undefined/missing value; the option endpoint persists whatever is sent, so this can overwrite the existing SMS credential.
Proposed fix
}
// Secret field: only send if non-empty
- if (inputs['sms.access_key_secret'] !== '') {
+ const accessKeySecret = inputs['sms.access_key_secret'] ?? '';
+ if (
+ accessKeySecret !== '' &&
+ originInputs['sms.access_key_secret'] !== accessKeySecret
+ ) {
options.push({
key: 'sms.access_key_secret',
- value: inputs['sms.access_key_secret'],
+ value: accessKeySecret,
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/settings/SystemSetting.jsx` around lines 612 - 618, The
current check only compares inputs['sms.access_key_secret'] !== '' which treats
undefined as "not empty" and causes sending an undefined secret; update the
condition in the block that pushes into options (the code that references inputs
and options in SystemSetting.jsx) to only push when the value is both defined
(not null/undefined) and not an empty string (e.g., check
inputs['sms.access_key_secret'] != null && inputs['sms.access_key_secret'] !==
''), so the secret option is only sent when a real value exists.
1 participant
Important
📝 变更描述 / Description
(简述：做了什么？为什么这样改能生效？请基于你对代码逻辑的理解来写，避免粘贴未经整理的内容)
🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
(请在此粘贴截图、关键日志或测试报告，以证明变更生效)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation