feat(i18n): complete modules/user migration to ResponseErrorL (manager + friend + login + …)#197
Conversation
…andlers
Add 17 codes covering modules/user/api_manager.go and the friend / online /
setting / maillist / device handlers, with zh-CN runtime translations and the
regenerated extractor marker:
- manager (10): manager_permission_required, password_too_short,
password_mismatch, old_password_incorrect, new_password_same_as_old,
not_admin_account, cannot_delete_super_admin, list_filter_conflict,
token_cache_failed, short_no_gen_failed
- friend (6): cannot_add_self, already_friend, bot_not_in_space,
friend_apply_not_found, friend_not_found, friend_apply_invalid
- device (1): device_not_found
All other migrated sites reuse the existing generic codes (query_failed,
store_failed, im_call_failed, token_cache_failed, decode_failed, not_found,
current_not_found, code_invalid) and the shared auth.forbidden code.
- respondManagerForbidden: collapses the wkhttp CheckLoginRole /
CheckLoginRoleIsSuperAdmin role guards onto err.shared.auth.forbidden
instead of leaking the raw framework string.
- respondUserListFilterConflict: reports a pair of mutually-exclusive list
filters with filter / conflicts_with details.
Migrate all 86 legacy response sites (84 c.ResponseError + 2 c.Response("...")
bugs that returned HTTP 200 with a bare string body) to the localized
httperr.ResponseErrorL envelope via the respond* helpers.
Following the api.go (#188) playbook: role guards map to auth.forbidden,
validation / BindJSON to request_invalid, service / DB errors are logged and
collapsed to generic codes (never passing err.Error() to the wire), and the
manager login failure path is unified onto invalid_credentials to avoid account
enumeration. HTTP status codes become semantic (401/403/404/409/500), matching
the existing /v1/user/* migration.
Migrate 111 legacy response sites across five handlers to ResponseErrorL. Most internal DB / IM / cache failures collapse to the existing generic codes; friend-specific business guards use the new friend codes. The formatted c.ResponseErrorf sites (which logged internally) are migrated with an explicit zap.Error log preserved. Unused github.com/pkg/errors imports are dropped in favour of the errcode import where applicable.
Add TestRespondUserHelpers cases for the new helpers and codes (v2 envelope, semantic status, zh-CN copy, details, Internal-collapse), and a TestManagerAPINoLegacyResponseError source guard mirroring the api.go guard so api_manager.go cannot regress to legacy .ResponseError / .ResponseErrorf.
lml2468
left a comment
lml2468
left a comment
There was a problem hiding this comment.
[APPROVE]
Clean Phase 2.1 migration — 197 response sites across 6 handler files, consistent patterns, no blocking issues.
✅ Highlights
- Bug fix: two
c.Response("...错误")sites inapi_manager.go(resetUserPasswordandupdatePwd) were returning HTTP 200 with a bare string body on error paths. Now proper error envelopes. Good catch. - Anti-enumeration: manager login failures (user-not-found vs bad-password) unify onto
ErrUserInvalidCredentials— consistent with the existing/v1/userlogin anti-enum pattern. - Leak prevention: several sites that forwarded raw
errtoc.ResponseError(err)(e.g.m.db.queryUserListWithPageinlist(),m.friendDB.QueryFriendsinfriends()) now log the error server-side and return generic codes. Clean separation. ResponseErrorfmigration: all 5 sites inapi_online.gonow have explicitu.Error(..., zap.Error(err))+respondUserError— preserves the logging behavior that was implicit inResponseErrorf.- Source guards (
TestManagerAPINoLegacyResponseError) prevent regressions. Helper tests cover v2 envelope, semantic status, zh-CN copy, details, and Internal-collapse. - Verified:
grep -c 'c\.ResponseError\|c\.ResponseErrorf' modules/user/api_{manager,friend,online,setting,maillist,device}.go= 0 across all 6 files.
🔵 Minor observations (non-blocking)
- Several
zap.Error(err)additions on previously-bare log calls (e.g.api_maillist.go:31,api_friend.go:752) are quality-of-life improvements beyond the strict i18n scope — good to have, just noting they're not purely mechanical. ErrUserListFilterConflict.SafeDetailKeysis a nice touch — surfacingfilter/conflicts_withwithout leaking arbitrary detail payloads.
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
This PR is in scope for octo-server, but it leaves transaction-leak paths in changed handlers.
🔴 Blocking
🔴 Critical: modules/user/api_friend.go:812-835 opens a DB transaction, then returns on GenSeq failure and source.CheckSource(vercode) failure without rolling back. These paths can leave the transaction/connection open until cleanup, and repeated invalid friend-confirm requests or sequence failures can exhaust the DB pool. Move CheckSource before Begin, or call tx.Rollback() before each early return after Begin.
🔴 Critical: modules/user/api_manager.go:495-519 opens a DB transaction before hashing the new user password, then returns on HashPassword failure without rollback. A too-long bcrypt password can trigger this path, causing the same connection leak. Hash before opening the transaction, or rollback before returning.
💬 Non-blocking
🟡 Warning: gofmt -l reports modules/user/api_manager_test.go, modules/user/api_online.go, and modules/user/api_setting.go. Please run gofmt on the touched Go files before merge.
🟡 Warning: Some internal error responses now hide details from clients but still do not log the underlying error, for example modules/user/api_friend.go:849-858 and modules/user/api_friend.go:881-890. Since ErrUserStoreFailed is Internal=true, server-side logs are the only useful diagnostic trail.
✅ Highlights
✅ The PR is clearly relevant to the repository and aligns with the ongoing i18n error-envelope migration.
✅ The new helper tests cover important envelope behavior, semantic statuses, localized copy, safe details, and internal-message collapse.
lml2468
left a comment
lml2468
left a comment
There was a problem hiding this comment.
Thorough Phase 2.1 migration — 197 response sites across 6 handler files, clean and complete.
Verification (at head 98a6962):
grep -c 'ResponseError\|ResponseErrorf' modules/user/api_{manager,friend,online,setting,maillist,device}.go= 0 across all 6 files. NoAbortWithStatusJSONin any migrated file. Migration is complete.- All 31
errcode.ErrUser*codes used in handlers are registered inpkg/errcode/user.go. - Registered IDs, en-US markers, and zh-CN translations are fully aligned (cross-checked via
comm). - D23 baseline unchanged (no
AbortWithStatusJSONsites removed, onlyResponseError→respondUser*). - CI: Build ✅ Lint ✅ Vet ✅ Test ✅ i18n Extract Check ✅ i18n Lint ✅.
On Jerry-Xin's blocking findings (tx leak):
The transaction-leak paths in friendSure (GenSeq/CheckSource after Begin without Rollback) and api_manager.go (HashPassword after Begin) are pre-existing control flow — this PR only changed the error response calls (c.ResponseError(err) → respondUserError), not the transaction handling. The missing rollbacks existed before and after the PR identically. Blocking an i18n migration for pre-existing tx bugs is scope creep; recommend filing a follow-up issue instead.
💬 Non-blocking:
- Agree with Jerry-Xin's
gofmtobservation — rungofmt -won the 3 flagged files before merge.
✅ Highlights:
- Bug fix: two
c.Response("...错误")sites inapi_manager.go(resetUserPassword/updatePwd) were returning HTTP 200 with a bare string body on error. Now proper error envelopes. - Anti-enumeration: manager login (user-not-found vs bad-password) unifies onto
ErrUserInvalidCredentials. - Leak prevention: raw
errthat used to be forwarded viac.ResponseError(err)now logs server-side and returns generic codes. - Source guards (
TestManagerAPINoLegacyResponseError,TestUserAPINoLegacyResponseError) prevent regressions. - Helper test suite covers v2 envelope, semantic status, zh-CN copy, details, and Internal collapse.
No blocking issues from the i18n migration itself. LGTM.
… handlers
Add 11 codes with zh-CN translations and the regenerated marker:
- destroy (4): password_not_set, password_incorrect, account_state_changed,
not_destroying
- pinned (4): pinned_already_exists, pinned_limit_exceeded (max detail),
channel_access_denied, pinned_sort_invalid
- oauth (3): oauth_state_expired, oauth_exchange_failed (502 Internal),
oauth_profile_failed (502 Internal)
Carries the per-space pinned-channel cap as a max detail so the client can render a localized limit hint.
…onseErrorL Migrate ~80 legacy response sites across four handlers. destroy reuses the account_destroying/destroyed/destroy_failed codes and maps its loginGuard.Check guard onto login_locked; pinned collapses the validateChannelAccess rejections onto channel_access_denied (403, internal failures still logged); the gitee/github OAuth flows reuse registration_closed / register_failed and the new oauth_* codes. All internal failures are logged and never pass err.Error() to the wire.
The testutil server has no ErrorRenderer, so migrated handlers fell back to the English DefaultMessage and broke the existing zh-CN assertions. Wire the renderer (DefaultLanguage=zh-CN) so the localized envelope is produced, matching the #188 pattern.
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #197 (octo-server)
Verdict: APPROVED — no P0/P1 blockers. The findings below are non-blocking observability and test-coverage suggestions; none of them gate merge.
Scope verified
Phase 2.1 of the i18n rollout: migrates modules/user management console + friend/online/setting/maillist/device handlers from legacy c.ResponseError(errors.New("中文")) / c.ResponseErrorf / c.Response("<string>") onto the localized httperr.ResponseErrorL envelope, with 17 new err.server.user.* codes.
Checks performed locally at head SHA 98a696211b095938da5864042f7980745e88d5ca:
- ✅
go build ./...— clean. - ✅
make i18n-extract-check— clean (markers in sync). - ✅
make i18n-lint— clean (lint-direct-error-response+lint-unregistered-code). - ✅
go test ./modules/user/non-DB cases (TestRespondUserHelpers,TestManagerAPINoLegacyResponseError,TestUserAPINoLegacyResponseError) — pass. DB-backed cases require MySQL and were not run here. - ✅ All 17 new codes have matching zh-CN translations and en-US extractor markers.
- ✅ All 6 migrated handler files contain zero live
c.ResponseError/c.ResponseErrorf/c.Response("<errstring>"). Remaining legacy sites inmodules/userare confined to the explicitly-declared follow-up files (api_destroy.go,api_pinned.go,api_gitee.go,api_github.go,api_emaillogin.go,api_usernamelogin.go);api.go's only remaining references are commented-out breadcrumbs.
Security review (this PR is security-sensitive)
- ✅ Account-enumeration defense is complete.
Manager.loginnow collapses both "user not found" and "wrong password" toErrUserInvalidCredentials(401), with no remaining response-shape difference between the two branches. Consistent with the/v1/userlogin playbook. - ✅ No raw-error leakage on
Internal=truecodes.ResponseErrorLsuppressesDefaultMessage/details forInternal=trueand the renderer keepsTransportStatus=400+SemanticStatusfor legacy client compat. Migrated 5xx sites no longer puterr.Error()/ DB strings / framework strings on the wire — a net improvement over the legacyc.ResponseError(err)path, which echoed raw strings (octo-lib wkhttp/http.go:83). - ✅
respondManagerForbidden403 collapse is correct. All role-guarded routes sit behindAuthMiddleware, so unauthenticated requests are already rejected upstream; an authenticated-but-unauthorized caller (role==""or wrong role) collapsing toerr.shared.auth.forbidden(403) is semantically right, not a 401-vs-403 mistake. - ✅ Two genuine bug-fixes.
resetUserPasswordandupdatePwdpreviously returnedc.Response("…错误")— HTTP 200 with a bare string on a password-update DB failure, i.e. the client believed the password changed when it had not. Both now correctly return a 500 error envelope (ErrUserLoginPwdUpdateFailed) with azap.Errorlog. Good catch by the author. - ✅
list_filter_conflictdetails (filter/conflicts_with) carry only filter names — no sensitive data.
Non-blocking findings (P2)
These are observability / test-hardening nits. They do not block merge and several are pre-existing; consider as a small follow-up or fold into the planned login-files migration.
P2-1 — api_friend.go:392-396 & :833-837: vercode-check errors collapse to 400 code_invalid with no log.
source.CheckRequestAddFriendCode / source.CheckSource return both user-input validation errors and internal faults (wrapped DB errors, provider not initialized). Both are mapped to a non-Internal 400 ErrUserCodeInvalid and the err is discarded with no f.Error(...). These are the only err-bearing branches in the whole diff that skip a handler-level log. DB-layer faults are still logged one level down (e.g. modules/user/source.go:58), so this is debuggability, not a silent outage — but an internal fault surfacing as "invalid code" misdirects triage.
Suggestion: f.Error("好友验证码校验失败", zap.Error(err), zap.String("uid", fromUID)) before each site (f.Warn is arguably better given the predominantly user-input path); ideally distinguish internal vs validation errors so infra faults map to 500.
P2-2 — api_friend.go friendSure store branches (~lines 851, 858, 883, 890): ErrUserStoreFailed (Internal=500) responses drop err with no log.
The InsertTx / updateRelationshipTx failure branches tx.Rollback(); respondUserError(c, ErrUserStoreFailed); return with no zap.Error. For Internal=true codes the wire carries only a generic message, so a failing friend INSERT/UPDATE becomes invisible in logs — a regression vs the legacy path, which at least put a distinguishing string on the wire. The same function logs at the GenSeq/commit sites, so these four are inconsistent standouts.
Suggestion: add f.Error("添加好友失败"/"修改好友关系失败", zap.Error(err), zap.String("uid", loginUID), zap.String("toUid", applyUID)) before each, matching the existing pattern.
P2-3 (minor) — api_friend.go friendApply query/store branches (e.g. lines 371, 446, 453, 480, 492, 509, 519): log a static message but omit zap.Error(err).
These f.Error(...) lines are unchanged by this PR (pre-existing), but now that the responses are Internal=true the dropped err is the only place the real cause would live. Cheap to append zap.Error(err) while the file is already being edited.
P2-4 — api_i18n_test.go: source guard covers only 2 of the 6 migrated files.
TestUserAPINoLegacyResponseError guards api.go; TestManagerAPINoLegacyResponseError guards api_manager.go. The other 5 migrated files (api_friend.go, api_online.go, api_setting.go, api_maillist.go, api_device.go) have no legacy-ResponseError guard, so a future regression there would pass CI.
Suggestion: make the guard table-driven over all 6 migrated filenames against the same banned-substring list.
P2-5 — api_i18n_test.go:50-52 vs :67: comment overstates the guard.
The TestManagerAPINoLegacyResponseError doc comment claims it "forbids the c.Response("<string>") shape", but the banned list is only {".ResponseError(", ".ResponseErrorf("}. Harmless today (no such call remains), but a future c.Response("some error") would pass green.
Suggestion: either add a c.Response(" literal check, or correct the comment.
For a human to glance at
- Status-code behavior change. Statuses move from a uniform 400 to semantic 401/403/404/409/500. The dual envelope keeps
TransportStatus=400+ legacy{msg,status}, so body-error.codeclients are safe; any admin/friend client keying off the raw HTTP status would see new codes. Per the PR description this matches the existing/v1/usermigration and is intended — worth a quick confirmation that the management-console frontend readserror.code, not the transport status.
Nice, careful migration overall — anti-enumeration handled correctly, Internal suppression respected, the two HTTP-200-on-failure password bugs fixed, and i18n markers/translations complete. None of the above blocks merge.
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review (revised) — PR #197 (octo-server)
Revised verdict: CHANGES_REQUESTED. My earlier pass approved this on the i18n mapping (which is correct and complete), but it missed two confirmed transaction-leak paths in handlers this PR edits. After independently reproducing them, I'm aligning with the blocking finding. Correcting my own verdict rather than leave a misleading approval on a security-sensitive PR.
Blocking (P0/P1)
🔴 P0 — friendSure leaks a DB transaction on early return after Begin() — reachable by any authenticated user.
modules/user/api_friend.go:813 opens tx, err := f.ctx.DB().Begin(). The defer only rolls back inside recover() (the panic path) — it does not roll back on a normal return. Two branches then return without tx.Rollback():
:825-829—GenSeq(common.FriendSeqKey)failure.:833-837—source.CheckSource(vercode)failure (the exact branch this PR rewrites:c.ResponseError(err)→respondUserError(c, errcode.ErrUserCodeInvalid)).
friendSure is a normal user endpoint (auth, not admin). CheckSource fails on any invalid/malformed vercode when no prior friend relationship exists, so an authenticated client can repeatedly trigger the leak; abandoned *sql.Tx holds its pooled connection, and enough of them exhaust the pool → service-wide DoS. That reachability is why I rate this P0 rather than P1.
Fix: move CheckSource before Begin(), or add tx.Rollback() (consistent with RollbackUnlessCommitted used elsewhere in the file) before each early return after Begin().
🔴 P1 — addUser leaks a DB transaction on HashPassword failure after Begin().
modules/user/api_manager.go:495 opens the tx; :515-519 returns on HashPassword(req.Password) failure (also a branch this PR rewrites) with no tx.Rollback(). Lower reach than the above (super-admin-only, and bcrypt only errors on >72-byte input), but the same leak pattern.
Fix: hash before Begin(), or roll back before returning.
These are pre-existing control-flow bugs — the diff changed only the response call on these lines, not the rollback handling, so they predate this PR. I'd normally weigh that toward "follow-up issue, don't block." Two things tip me to blocking here: (1) this is the classified security-sensitive PR and one path is a user-reachable DoS, and (2) the PR is already editing these exact if err != nil { … return } blocks, so the fix is a one-line addition in code that's already open. Given that, fixing in place is cheaper than a follow-up ticket and closes a live DoS.
Also fix before merge (non-blocking but flagged by multiple reviewers)
🟡 gofmt -l is dirty on touched files: modules/user/api_online.go (import ordering — within this PR's edited import block), modules/user/api_setting.go (switch-body indentation), modules/user/api_manager_test.go. Run gofmt -w on the touched files. (Note: api_manager_test.go also shows a smart-quote ” replacing '' at line ~536 in a comment — gofmt will surface it; please correct.)
P2 (observability — fold in while you're in these files)
api_friend.go:392-396&:833-837—CheckRequestAddFriendCode/CheckSourceerrors collapse to a non-Internal400code_invalidwith theerrdiscarded and nof.Errorlog. These functions also return internal faults (wrapped DB errors,provider not initialized); a fault then surfaces as "invalid code" and misdirects triage. The only err-bearing branches in the whole diff with no handler log. Addf.Error("好友验证码校验失败", zap.Error(err), zap.String("uid", fromUID))(orf.Warn), and ideally map true internal faults to 500.api_friend.gofriendSurestore branches (~851, 858, 883, 890) —respondUserError(c, ErrUserStoreFailed)(Internal=500, message suppressed on the wire) with nozap.Error. A failing friend INSERT/UPDATE becomes invisible in logs — regression vs the legacy string-on-wire path. Addf.Error("添加好友失败"/"修改好友关系失败", zap.Error(err), …), matching the 867/906/929 pattern.
P2 (test hardening)
api_i18n_test.go— the legacy-ResponseErrorsource guard covers onlyapi.goandapi_manager.go; the other 5 migrated files (api_friend.go,api_online.go,api_setting.go,api_maillist.go,api_device.go) are unguarded, so a future regression there passes CI. Make it table-driven over all 6 filenames.api_i18n_test.go:50-52vs:67— the doc comment claims it bans thec.Response("<string>")error shape, but the banned list is only{".ResponseError(", ".ResponseErrorf("}. Harmless today; correct the comment or add the literal check.
What is correct in this PR (verified at head 98a696211b095938da5864042f7980745e88d5ca)
- ✅
go build ./...,make i18n-extract-check,make i18n-lintall clean; non-DB user tests pass. - ✅ All 17 new
err.server.user.*codes have matching zh-CN translations and en-US markers; details/SafeDetailKeysconsistent. - ✅ All 6 migrated handler files: zero live
c.ResponseError/c.ResponseErrorf/c.Response("<errstring>"). Remaining legacy sites are confined to the declared follow-up files. - ✅ Anti-enumeration complete —
Manager.logincollapses user-not-found and wrong-password toErrUserInvalidCredentials(401), no response-shape difference. - ✅ No raw-error leakage on
Internal=true—ResponseErrorLsuppresses message/details;TransportStatus=400+SemanticStatuspreserves legacy client compat. - ✅
respondManagerForbidden403 collapse is correct — role guards sit behindAuthMiddleware, so arole==""/wrong-role caller is authenticated-but-unauthorized (403), not a 401 miss. - ✅ Two real bug-fixes —
resetUserPassword/updatePwdpreviously returned HTTP 200 + bare string on a password-update DB failure (client believed the change succeeded); now proper 500 envelopes with logs.
The i18n migration itself is clean, complete, and well-tested. The blocker is the two transaction-leak paths in the changed handlers; closing those (plus gofmt) clears the gate.
…login
Add 12 codes with zh-CN translations and the regenerated marker:
- email (5): email_invalid, email_register_disabled, email_login_disabled,
account_unavailable, email_send_failed (500 Internal)
- username / web3 (7): username_register_disabled, username_format_invalid,
public_key_not_found, public_key_already_exists, signature_not_found,
signature_invalid, verify_type_invalid
Reuse the existing registration_closed / local_login_disabled /
invalid_credentials / login_locked / code_invalid / not_found / already_exists /
password_* / register_failed / query_failed / store_failed / token_cache_failed
codes for the rest.
…seErrorL Migrate the final ~119 legacy response sites in the security-sensitive login handlers (incl. 2 c.Response(errors.New(...)) bugs that returned HTTP 200 with a serialized error body). Following the api.go playbook: login failures unify onto invalid_credentials to avoid account enumeration, loginGuard.Check maps to login_locked (429), the Web3 signature flow uses the new public_key_* / signature_* codes, and all service / DB / third-party failures are logged and collapsed to generic codes. This completes the modules/user migration.
Wire the i18n renderer into the email/username block tests (testutil server has no ErrorRenderer) and assert error.code where the migrated zh-CN wording differs from the legacy literal. Add TestRespondUserHelpers cases for the new pinned / oauth / web3 / email codes and a table-driven TestMigratedUserFilesNoLegacyResponseError guard over all Phase 2.1 migrated user handlers.
an9xyz
changed the title
lml2468
left a comment
lml2468
left a comment
There was a problem hiding this comment.
Incremental re-review on 370a98c — extends the Phase 2.1 migration from the original 6 files to all remaining user module handlers.
New files migrated in this delta: api_destroy.go, api_emaillogin.go, api_gitee.go, api_github.go, api_usernamelogin.go, api_pinned.go + corresponding test updates.
Verification:
- Zero residual
ResponseError/ResponseErrorfin all 6 newly migrated files (verified at head SHA viagh api). TestMigratedUserFilesNoLegacyResponseErrorsource guard now covers all 11 handler files — prevents regressions.- New errcode registrations follow the established pattern (proper ID naming, HTTPStatus, Internal flags, SafeDetailKeys where appropriate).
- zh-CN translations and en-US markers both present for all new codes.
- i18n Lint ✅ already passed; Build/Lint/Vet/Test still running.
Notable quality improvements in the delta:
api_usernamelogin.go:registerWithUsernamehadc.Response(errors.New("密码不能为空！"))— returned HTTP 200 with an error object as body. Now properlyrespondUserRequestInvalid.- OAuth flows (gitee/github) that forwarded raw
errtoc.ResponseError(err)now log server-side and return generic codes. - Destroy handlers get proper semantic status codes (409 for state conflicts, 429 for rate limiting).
- New
respondUserPinnedLimitExceededhelper surfaces themaxdetail for client rendering.
No blocking issues. Previous approval reinstated. LGTM.
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
The PR is in scope, but two migrated paths now classify normal client-facing states as internal server errors.
🔴 Blocking
🔴 Critical: Expired or missing friend-approval tokens now return an internal decode error. In modules/user/api_friend.go:724-734, Cache().Get and JsonToMap(tokenVaule) failures both map to ErrUserDecodeFailed, which is Internal=true and semantic HTTP 500 in pkg/errcode/user.go:260-265. A missing/expired token is an expected invalid friend-apply state and should use ErrUserFriendApplyInvalid or a token-specific 4xx, while only Redis/cache infrastructure failures should be internal.
🔴 Critical: Email-code resend throttling is now reported as a generic internal server error. modules/user/api_emaillogin.go:66-70 maps every SendVerifyCode error to ErrUserEmailSendFailed, which is Internal=true / 500 in pkg/errcode/user.go:515-520. But EmailService.SendVerifyCode returns a normal user-facing rate-limit error when the one-minute resend key exists (modules/base/common/service_email.go:72-80). That path should produce a 429 or other explicit business code, not “internal server error.”
💬 Non-blocking
🟡 Warning: I could not complete the requested go test ./modules/user ./pkg/i18n/... ./pkg/errcode/... locally because modules/user tests require MySQL on 127.0.0.1:3306; the i18n and errcode packages did pass before the user tests failed.
✅ Highlights
The migration is broadly consistent with the existing ResponseErrorL pattern, the source guards for legacy ResponseError usage are useful, and the role/login anti-enumeration mappings are directionally sound.
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #197 (octo-server)
Summary
This PR migrates the modules/user manager, friend, online, setting, maillist, device, destroy, pinned, gitee/github OAuth, email-login and username/web3-login handlers from legacy c.ResponseError(errors.New("中文")) to the localized httperr.ResponseErrorL facade backed by registered error codes in pkg/errcode/user.go.
The migration is mechanically clean and well-executed. I verified the structural invariants, the code→message→locale mapping, anti-enumeration behavior on the auth paths, and wire-level compatibility. No correctness, security-correctness, or build-breaking issues found. The findings below are all non-blocking quality/UX notes.
Verified ✅
- No residual legacy calls. No
c.ResponseError(/c.ResponseErrorf(/c.Response("<err-string>")remain in any of the 12 migrated handler files. Two priorc.Response("...错误")bugs (HTTP 200 with a bare error string) inapi_manager.goare now proper error envelopes — a genuine fix. - Wire status unchanged.
ResponseErrorLhard-codesTransportStatus = 400and the renderer emits that as the HTTP status (pkg/httperr/respond.go:36, octo-liberror_renderer.go:53-61). The new semantic statuses (401/403/404/409/500/502) ride only in the envelope body, so existing clients that key off HTTP 400 +msgkeep working. - Control flow preserved. Every migrated site still
returns immediately after responding;ResponseErrorLdoes not abort, and no branch dropped itsreturn. - Locale parity. All 83 registered
err.server.user.*codes have both a zh-CN entry (active.zh-CN.toml) and an en-US marker (active.en-US.toml). No code renders a raw English fallback. - Anti-enumeration held/improved. Both password-login paths (
emailLogin,usernameLogin) and the manager login correctly unify user-not-found / disabled / wrong-password onto a singleErrUserInvalidCredentials, andmanager.loginadds a new comment + unification that closes a prior "用户不存在" vs "密码错误" oracle. - Internal=true suppression. 5xx codes (
token_cache_failed,oauth_exchange_failed,email_send_failed,short_no_gen_failed, …) collapse to the shared internal message on the wire and keep the real error in zap logs only — confirmed viaTestRespondUserHelpers(wantNotContains) which passes. - Tests.
pkg/errcode,pkg/httperr,pkg/i18n*, and the i18n guard/helper tests inmodules/userpass. (The DB-touching tests panic ondial tcp 127.0.0.1:3306— pre-existing environmental requirement, not caused by this PR.)
Findings (all non-blocking)
P2 — password_too_short zh-CN copy contradicts the rule and the en-US text
pkg/i18n/locales/active.zh-CN.toml: "密码长度必须大于 6 位。" (i.e. ≥7), but the en-US marker and DefaultMessage say "Password must be at least 6 characters" (≥6), and every call site validates len(password) < 6 — a 6-char password is accepted. The Chinese copy is therefore wrong for the boundary case. (This wording predates the PR in api_manager.go, but the migration centralizes it into one canonical string, so this is the right moment to fix it — e.g. "密码长度不能少于 6 位。")
P2 — Email send rate-limit hint is lost (collapsed into an internal error)
api_emaillogin.go:69 (emailSendCode) maps every error from EmailService.SendVerifyCode to ErrUserEmailSendFailed, which is Internal: true. The renderer wipes the message for Internal codes, so the user-actionable rate-limit string from service_email.go:80 ("发送过于频繁，请1分钟后再试") now surfaces as the generic "服务器内部错误。". Pre-migration (c.ResponseError(err)) returned that hint verbatim. Net effect: a legitimately rate-limited user sees a "server error" and keeps retrying. Suggest branching the rate-limit sentinel to a dedicated non-Internal 4xx/429 code (carrying retry_seconds) instead of collapsing onto the Internal email_send_failed.
P2 — Email verify lockout hint is lost (collapsed into "验证码错误")
The three emailService.Verify(...) sites (emailLogin ~:293, emailRegister ~:135, emailForgetPwd ~:355) all map to ErrUserCodeInvalid ("验证码错误。"). But Verify multiplexes lockout/limit states ("验证失败次数过多，请10分钟后再试" / "已锁定10分钟", service_email.go:367,403) through the same return. After the migration a locked-out user is told only "invalid code" and keeps retrying through the 10-minute lock window. The code still renders a real (non-Internal) message and the server-side lock still protects the system, so this is UX degradation only. Suggest a distinct lockout code carrying retry_minutes.
nit — Email code-login path is a (pre-existing, intentional) account/status oracle, now distinguishable by HTTP status
In emailLogin's code branch (req.Code != ""), the existence/status checks run before code verification and return three different codes: not-found → ErrUserNotFound (404), disabled/destroyed → ErrUserAccountUnavailable (403), valid+bad-code → ErrUserCodeInvalid (400). An attacker can probe an email's registration/active state without a valid code. The code comment at api_emaillogin.go:269 shows this hint is deliberately preserved from the legacy behavior (and the password branch is correctly unified), so this is not a regression in intent — but the migration newly differentiates the three states by HTTP status (not just body text), which is more trivially observable. Flagging for awareness; if the passwordless-status hint isn't a product requirement, consider collapsing the code-path not-found/unavailable onto ErrUserCodeInvalid.
Notes for a human reviewer (security-sensitive PR)
- The
respondManagerForbiddenchange intentionally raises the manager role-guard failures (CheckLoginRole/CheckLoginRoleIsSuperAdmin, which only ever return role-related errors) from legacy 400 to a semantic 403 and drops the raw framework string from the wire — correct and an improvement. validateChannelAccess(api_pinned.go) still mixes internal check failures ("校验频道权限失败" / "系统配置错误") with genuine 403 denials, all collapsed toErrUserChannelAccessDenied(403). The author explicitly documents splitting the internal failures into a 5xx as a follow-up — acceptable for this PR.
None of the above blocks merge. Approving; the P2 copy/UX items are worth a quick follow-up (the rate-limit/lockout hint loss is the most user-visible).
…gs, gofmt
Blocking (Jerry-Xin / yujiawei):
- Add tx.Rollback() before the early returns that ran after Begin() without
rolling back: api_friend.go delete (GenSeq) and friendSure (GenSeq +
source.CheckSource), api_manager.go addUser (HashPassword). These could
leak a DB connection on repeated failures. (Pre-existing control flow; fixed
here since the PR already edits these handlers.)
Non-blocking (yujiawei P2):
- P2-1: log source.CheckRequestAddFriendCode / CheckSource failures before the
code_invalid response.
- P2-2: log the four friendSure store-failure branches (ErrUserStoreFailed is
Internal=true, so logs are the only diagnostic trail).
- P2-3: append zap.Error(err) to the friendApply query/store log calls that
only carried zap.String.
- P2-5: add c.Response("<string>") to the legacy-response source guards so the
comment matches the check (and the fixed c.Response-as-error bugs can't regress).
- gofmt the touched files (api_destroy/api_online/api_setting/api_manager_test).
P2-4 (table-driven guard over all migrated files) was already addressed in the
TestMigratedUserFilesNoLegacyResponseError commit.
|
Thanks for the careful reviews. Two things in this update: 1. Scope expanded — please re-reviewThe reviews above were at
The PR title/description are updated accordingly. 2. Review findings addressed (
|
lml2468
left a comment
lml2468
left a comment
There was a problem hiding this comment.
Incremental re-review on 45555c1 — addresses all feedback from previous rounds.
Previous blockers resolved:
- 🟢 TX leak (Jerry-Xin round 1):
tx.Rollback()added before early returns inapi_friend.go(delete/GenSeq, friendSure/GenSeq, friendSure/CheckSource) andapi_manager.go(addUser/HashPassword). All 4 paths now properly clean up the transaction. - 🟢 gofmt (yujiawei/Jerry-Xin):
api_setting.goindentation fixed (9 case blocks re-indented),api_online.goimport ordering,api_manager_test.goformatting. - 🟢 Logging gaps:
zap.Error(err)added to 7 log calls that previously omitted the error, improving server-side diagnostics.
Other changes in this delta:
- Test source guards now also ban
c.Response("(bare string error responses) — catches the pattern that was fixed inresetUserPassword/updatePwd. api_destroy.go: cosmetic alignment, trailing newline.
No new issues. Previous approval reinstated. LGTM.
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
The PR is in scope for octo-server, but two migrated error paths now report normal client/business failures as internal server errors.
🔴 Blocking
🔴 Critical — Expired or malformed friend-accept tokens now return an internal decode failure. In modules/user/api_friend.go, cache read errors and JSON parse failures both map to ErrUserDecodeFailed; that code is Internal=true and semantic 500 in pkg/errcode/user.go. An expired/missing token commonly yields an empty or invalid payload and should use ErrUserFriendApplyInvalid, while actual cache backend errors should use ErrUserTokenCacheFailed.
🔴 Critical — Email verification resend throttling is now surfaced as an internal send failure. EmailService.SendVerifyCode returns a deliberate rate-limit error when the one-minute key exists in modules/base/common/service_email.go, but modules/user/api_emaillogin.go collapses every error to ErrUserEmailSendFailed, which is Internal=true and semantic 500 in pkg/errcode/user.go. Split rate-limit/client errors from SMTP/Redis failures, preferably with sentinel errors and a non-internal localized code.
✅ Highlights
The scope and direction are correct: the PR migrates project-owned user handlers to the localized envelope and adds regression guards against legacy response helpers.
The anti-enumeration mappings in username/email password login paths look preserved.
Test note: I ran go test ./modules/user/ ./pkg/i18n/... ./pkg/errcode/...; pkg/i18n and pkg/errcode passed, while modules/user could not run in this environment because MySQL on 127.0.0.1:3306 was unavailable.
lml2468
left a comment
lml2468
left a comment
There was a problem hiding this comment.
[REQUEST_CHANGES]
Correcting my earlier approval — agreeing with @Jerry-Xin's two outstanding findings. Both are regressions where normal client-facing states moved from HTTP 400 to HTTP 500 (Internal=true).
🔴 Critical — Friend-accept token decode mapped to internal 500
api_friend.go:726-735: both Cache().Get() failure and util.JsonToMap() failure collapse to ErrUserDecodeFailed (Internal=true, 500). An expired or missing friend-apply token is a normal client state — Cache().Get() returns empty string on TTL expiry, then JsonToMap("") fails with a parse error. Before the migration, both paths returned 400 via c.ResponseError(err). Now they return 500 with the message suppressed by Internal=true.
Suggested split:
- Cache backend error →
ErrUserTokenCacheFailed(500, Internal — genuine infra failure) - Empty/malformed token value →
ErrUserFriendApplyInvalid(400, client-facing — "Friend request is invalid or has expired")
🔴 Critical — Email rate-limit error mapped to internal 500
api_emaillogin.go:67-69: EmailService.SendVerifyCode returns errors.New("发送过于频繁，请1分钟后再试") for the one-minute throttle (service_email.go:80), which is a deliberate rate limit. The handler collapses all errors to ErrUserEmailSendFailed (Internal=true, 500). Before the migration this was 400 with the throttle message visible to the client.
Suggested fix: introduce a sentinel error in EmailService for rate limiting (or wrap with a typed error), then branch in the handler — rate limit → a new ErrUserEmailRateLimited (429, non-internal) and actual SMTP/Redis failures → ErrUserEmailSendFailed (500, Internal).
✅ Everything else looks good — the 5 newly-migrated files (destroy, pinned, gitee, github, usernamelogin) follow the same clean patterns, the TestMigratedUserFilesNoLegacyResponseError guard is well-structured, and the tx-leak rollbacks from the fix commit are correctly placed.
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #197 (octo-server)
Independent review at head 45555c1 (security-sensitive: auth / login / migration). The i18n mapping itself is correct and complete, the previously-flagged transaction-leak rollbacks have landed, gofmt is now clean, and the three c.Response(...)-as-error bugs are genuinely fixed. The blocker is a recurring status-semantics regression: several expected client errors are now collapsed onto HTTPStatus 500 / Internal=true codes. Because the legacy c.ResponseError(err) returned HTTP 400 with an actionable message, these paths regress from 400 + guidance to 500 + generic "internal error" — the opposite of this PR's stated "semantic status codes" goal, and they make normal client states look like server outages.
✅ Verified correct
- Anti-enumeration preserved in
api_usernamelogin.go— account-not-found, destroyed/disabled, and wrong-password all unify toErrUserInvalidCredentialswithloginGuard.RecordFailureLogged(api_usernamelogin.go:113-131). - Transaction rollbacks now present on all early returns after
Begin()inapi_friend.go(sureFriend/auto-approve) andapi_manager.go:495-572(hash-before/rollback). The prior tx-leak finding is resolved. gofmt -lon all changed.gofiles is clean;go build ./modules/user/... ./pkg/errcode/...passes.- 3
c.Response(...)-as-error bugs (HTTP 200 + bare string body) fixed (api_manager.goresetUserPassword/updatePwd;api_usernamelogin.go). pkg/errcode/user.goregistry is consistent (no duplicate IDs;Internal/status flags sane).api_destroy.go,api_online.go,api_setting.go,api_maillist.go,api_device.goare clean migrations.api_gitee.go/api_github.gocorrectly addzap.Error(err)to several previously-unlogged paths — net observability improvement.
🔴 P1 — blocking (must change before merge)
1. Email verify-code rate-limit reported as 500 Internal — api_emaillogin.go:67-70
SendVerifyCode returns errors.New("发送过于频繁，请1分钟后再试") on the 1-minute resend cooldown (modules/base/common/service_email.go:79-81), an expected client condition. The call site maps every error — including this rate-limit — to ErrUserEmailSendFailed (pkg/errcode/user.go:515, 500 / Internal=true). Clients lose the actionable "try again in 1 minute" message and a normal resend looks like a server failure (was HTTP 400 + the text on main).
Fix: return a typed sentinel from the rate-limit branch (e.g. ErrEmailRateLimited), branch on errors.Is at the call site, and map it to a new 429 code (Internal=false); keep ErrUserEmailSendFailed (500) only for genuine Redis/SMTP failures. The identical pattern in service_sms.go:54 should reuse the same sentinel when those sites migrate.
2. Expired/missing friend-apply token returns 500 ErrUserDecodeFailed — api_friend.go:732-736
On an expired/missing token, Cache().Get returns ("", nil) (the redis client signals miss via empty value, not error — same convention used at api.go:3770, message/api.go:371). The empty value then falls into util.JsonToMap(""), which errors, mapping to ErrUserDecodeFailed (pkg/errcode/user.go:260, 500 / Internal=true). An expired friend-apply link is a normal client state, not a server fault. Notably the PR already registered ErrUserFriendApplyInvalid (400, pkg/errcode/user.go:405) and uses it for the payload-shape checks just below (api_friend.go:751-760) — the empty-token path simply isn't routed to it.
Fix: after Cache().Get, if tokenVaule == "" (miss) → ErrUserFriendApplyInvalid (400). Reserve ErrUserDecodeFailed (500) for the real-err Redis-infra branch only. The JsonToMap failure on a non-empty value can stay 500.
3. OAuth login: banned/disabled account & missing-device collapse to 500 — api_gitee.go:144-148, api_github.go:80-84
execLogin returns expected business errors — "该用户已被禁用" (api.go:1390-1391) and, since config.APP == 0 makes the device-lock branch live with device == nil, "登录设备信息不能为空！" (api.go:1398-1400). Both collapse to ErrUserStoreFailed (500 / Internal). A banned/disabled account should be 403 (ErrUserAccountBanned already exists at user.go:64) and missing-device a 4xx; these were HTTP 400 + message on main. No security leak (OAuth principal already authenticated, Chinese text is suppressed), but the status + actionable semantics regress.
Fix: give execLogin typed sentinels for the account-state / device branches and map them to ErrUserAccountBanned (403) / an appropriate 4xx at both OAuth call sites; let only genuine infra failures fall through to ErrUserStoreFailed (500). Keep the existing zap.Error(err) logs.
4. liftBanUser Internal-collapsed query error not logged — api_manager.go:932
m.userDB.QueryByUID(uid) failure maps to ErrUserQueryFailed (500 / Internal=true) but the log line omits zap.Error(err) — so the real error is neither on the wire nor in the logs, leaving a 500 with no diagnostic trail. Sibling sites in the same function (lines 946, 962, 968) all pass zap.Error(err). (Note: this gap pre-dates the PR — main also didn't log the raw err here — so if you scope strictly to "no new regressions" this is arguably P2; I'm flagging it P1 under the migration rule that every Internal-collapsed path must log the underlying error, and it's a one-line fix.)
Fix: add zap.Error(err) to the m.Error("查询用户信息失败！", zap.String("uid", uid)) call.
🟡 P2 — non-blocking (suggestions / pre-existing)
api_manager.go:410(addAdminUser): query error not logged withzap.Error(err)for parity with lines 446/471. Pre-existing (legacy sent a hardcoded string, never the real err), so no diagnostics regression — consistency cleanup only.api_manager.go:465(addUser):checkAddUserReqmulti-field validation collapses torespondUserRequestInvalid(c, ""), dropping which field was empty (vsaddAdminUserat 392-407 which tags the field). Status unchanged (400). Optional: return the offending field name.api_usernamelogin.go:86-88(usernameLogin):req.Check()error →respondUserRequestInvalid(c, "")likewise drops the empty-field name. Non-blocking; status (400) and no-leak behavior unchanged.api_usernamelogin.go:192-201andapi_github.go:157-172(pre-existing, not introduced here): the txcommitCallbackwrites a response andreturn nilonCommit()failure, socreateUserWithRespAndTxproceeds past it (post-rollback side effects + a second HTTP write). The PR only swapped the envelope; flagging for a follow-up — the callback should propagate the error and let a single outer sink respond.api_pinned.go:106-112(validateChannelAccess): genuine internal DB-check failures collapse to the 403channel_access_deniedrather than a 5xx. Fail-closed and logged in-helper; explicitly documented as a deferred follow-up — acceptable.api_gitee.go:252: pre-existingc.String(200, "登录失败！")defensive dead-end (frontend polls Redis); outside the error-envelope migration scope.
Note for the human reviewer (security-sensitive)
The P1 items are status-classification regressions, not auth-bypass or info-leak — anti-enumeration and the Internal=true wire-suppression are sound. The one path worth a manual glance is the friend-apply token (#2): confirm that routing expired tokens to a 400 doesn't change any client retry/abuse behavior on that endpoint.
…o 500 Addresses #197 review round 2 (status-semantics regressions where expected client states reported as HTTP 500 Internal): - execLogin: introduce ErrUserDisabled / ErrUserDeviceInfoRequired sentinels and a single respondExecLoginError classifier used by ALL login entry points (main execLoginAndRespose + gitee/github/email/username). Disabled account → 403 (ErrUserAccountBanned), missing device info → 400, ErrUserNeedVerification keeps its 110 response, genuine internal failures → 500. Previously the OAuth/ email/username paths collapsed all of these onto 500; the main path also only special-cased NeedVerification. Now uniform. - api_emaillogin.go: email verify-code resend cooldown is a client 429, not a 500. Add commonapi.ErrEmailSendRateLimited sentinel in service_email.go, branch on it, map to new ErrUserEmailRateLimited (429, non-Internal); genuine SMTP/Redis failures still map to ErrUserEmailSendFailed (500). - api_friend.go friendSure token: Cache().Get error (real Redis fault) → 500 ErrUserTokenCacheFailed; JsonToMap failure (expired/missing token, the ("", nil) cache-miss path) → 400 ErrUserFriendApplyInvalid. Was a blanket 500 ErrUserDecodeFailed. - api_manager.go liftBanUser: add zap.Error(err) to the Internal-collapsed query-failure log. Tests: TestRespondExecLoginError covers the 4-way classification; helper case added for ErrUserEmailRateLimited.
|
Round-2 findings addressed in 🔴 execLogin disabled/device → 500 (@yujiawei #3) — fixed uniformly. Added 🔴 Email resend cooldown → 500 (@Jerry-Xin / @lml2468 / @yujiawei #1) — fixed with a sentinel as suggested: 🔴 Friend-apply token expired → 500 (@Jerry-Xin / @lml2468 #2) — fixed: 🟡 P1-4 liftBanUser — added 🟡 P2 items: field-name in New test |
lml2468
left a comment
lml2468
left a comment
There was a problem hiding this comment.
Incremental re-review on ecc998f — addresses both blocking issues from Jerry-Xin/yujiawei/Allen rounds.
Previous blockers resolved:
- 🟢 Friend token 4xx (Jerry-Xin/yujiawei/Allen):
api_friend.go:726now splits:Cache.Getfailure staysErrUserTokenCacheFailed(500, genuine Redis failure);JsonToMapfailure (expired/invalid token = empty string) moved toErrUserFriendApplyInvalid(4xx, client-actionable). Comments document the rationale. - 🟢 Email rate-limiting 429 (Jerry-Xin/yujiawei/Allen): New
ErrEmailSendRateLimitedsentinel inservice_email.go;api_emaillogin.go:67branches onerrors.Is→ newErrUserEmailRateLimited(429, not Internal). Remaining send failures (Redis/SMTP) stay onErrUserEmailSendFailed(500, Internal). Test covers the 429 envelope.
Bonus improvement in this delta:
respondExecLoginErrorcentralizes execLogin error classification across ALL login paths (main/OAuth/email/username). Before: disabled accounts and missing device info were collapsed to 500 in OAuth/email/username paths. Now:ErrUserDisabled→ 403,ErrUserDeviceInfoRequired→ 400,ErrUserNeedVerification→ bespoke 110, others → logged+500.TestRespondExecLoginErrorpins all 4 branches.
No new issues. LGTM.
lml2468
left a comment
lml2468
left a comment
There was a problem hiding this comment.
[APPROVE]
Previous blockers resolved — both client-vs-internal classification issues are fixed, plus a well-designed execLogin error centralizer.
🟢 Blocker 1 resolved — Friend-accept token decode
- Cache backend failure →
ErrUserTokenCacheFailed(500, Internal) — correct, genuine infra issue - Empty/malformed token →
ErrUserFriendApplyInvalid(400, client-facing) — correct, expired link - Log level downgraded from
ErrortoWarnfor the client-facing case ✅
🟢 Blocker 2 resolved — Email rate-limit
ErrEmailSendRateLimitedsentinel error introduced inservice_email.gowith doc explaining the contract- Handler branches on
errors.Is: rate limit →ErrUserEmailRateLimited(429, non-Internal), SMTP/Redis →ErrUserEmailSendFailed(500, Internal) ✅
✅ Bonus: respondExecLoginError centralizer
execLogin returns 4 distinct sentinels (ErrUserNeedVerification, ErrUserDisabled, ErrUserDeviceInfoRequired, plus the catch-all). Before this commit, OAuth/email/username callers collapsed all of these onto generic 500. Now all 4 login paths (main, gitee, github, email, username) share one classifier that maps disabled-account → 403, missing-device → 400, verification-needed → bespoke 110. Clean single-point-of-truth.
Jerry-Xin
left a comment
Jerry-Xin
left a comment
There was a problem hiding this comment.
Summary: The PR is in scope for octo-server and the i18n migration is broadly consistent with the existing localized error envelope pattern; I found no blocking issues.
💬 Non-blocking
🟡 Warning: Several registration callbacks still write an error response on tx.Commit() failure but return nil, allowing createUserWithRespAndTx to continue as if the commit succeeded. This is adjacent to the migration because the response call was changed, but the control-flow bug appears pre-existing. See modules/user/api.go:3224, modules/user/api_usernamelogin.go:192, modules/user/api_gitee.go:222, and modules/user/api_github.go:157. modules/user/api_emaillogin.go:202 uses the safer pattern by returning the commit error.
🟡 Warning: TestRespondExecLoginError only tests the error classifier but uses testutil.NewTestServer(), which requires a MySQL connection. It could use the lightweight helper harness pattern from api_i18n_test.go instead. See modules/user/api_usernamelogin_test.go:23.
✅ Highlights
🔵 Suggestion: The centralized respondExecLoginError classifier is a good cleanup and removes divergent login-path semantics.
🔵 Suggestion: The regression guards in api_i18n_test.go are useful for preventing future fallback to legacy response APIs.
Verification: go test ./modules/user -run 'Test(UserAPINoLegacyResponseError|ManagerAPINoLegacyResponseError|MigratedUserFilesNoLegacyResponseError|RespondUserHelpers)$' passed. Full targeted tests could not complete locally because MySQL on 127.0.0.1:3306 was unavailable.
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #197 (octo-server)
Verdict: APPROVED — no P0/P1 blockers. This completes the modules/user i18n migration cleanly; the remaining items below are non-blocking (P2/nit) and most are pre-existing or already tracked as documented follow-ups.
Reviewed independently at head ecc998f94ff8fba6a50e83b9f01f63ac4f7c7223 (security-sensitive: auth / login / migration). I traced all 12 migrated handler files against pkg/errcode/user.go and the i18n locale files, with go build ./modules/user/... ./pkg/errcode/... ./pkg/i18n/... and go vet ./modules/user/... both green locally.
Verification (✅ confirmed)
- Migration completeness ✅ — zero residual
c.ResponseError/c.ResponseErrorf/c.Response("<string>")across all 11 handler files; the source guards (TestMigratedUserFilesNoLegacyResponseError,TestManagerAPINoLegacyResponseError) pin against regression. - Status semantics ✅ — no 4xx→5xx regression. Every client/business condition (expired token, invalid input, not-found, conflict, rate-limit, permission, bad credentials) maps to a 4xx code with
Internal=false;Internal=true/5xx codes are used only on genuine DB / cache / IM / 3rd-party / hashing failures. The two paths flagged in earlier rounds are correctly fixed:api_friend.go:727-741— friend-accept token: expired/malformed payload →ErrUserFriendApplyInvalid(400); genuine Redis read error →ErrUserTokenCacheFailed(500). Correctly split.api_emaillogin.go:71-78— 1-minute resend cooldown →ErrUserEmailRateLimited(429); SMTP/Redis failure →ErrUserEmailSendFailed(500). NewErrEmailSendRateLimitedsentinel inservice_email.godrives the split.
- Transaction safety ✅ — every
Begin()in the migrated files (api_manager.goaddUser,api_friend.godelete/addFriend/autoApproveFriend/friendSure,api_emaillogin.go,api_usernamelogin.go,api_maillist.go, OAuth files) rolls back on every early return. The prior-flaggedfriendSureGenSeq/CheckSource bare-returns now bothtx.Rollback();addUserHashPassword path now rolls back. The defer blocks are correctly panic-only safety, not the sole rollback mechanism. - Error-detail leak ✅ —
respondUserError/ResponseErrorLtake a registered code + structured details only; the renderer forcesInternal=truecodes to the generic localized message and drops details. Raw DB / Redis / OAuth / SMTP errors arezap-logged server-side only. The PR actively closes prior leaks where legacyc.ResponseError(err)forwarded lockout strings and raw Redis errors to the client. - Anti-enumeration ✅ — all password/credential paths (
api_manager.gologin,api_usernamelogin.go,api_emaillogin.gopassword branch) collapse user-not-found + account-disabled + wrong-password toErrUserInvalidCredentials(401); lockout →ErrUserLoginLocked(429). - Code↔i18n mapping ✅ — all 84
errcode.ErrUser*symbols referenced in handlers are registered; every registered ID has a matching en-US marker and a non-empty zh-CN translation (zero missing, zero orphaned in either locale). Spot-checked ~18 branches for semantically correct status selection — all correct.
Non-blocking findings (P2)
-
api_pinned.govalidateChannelAccesscollapses internal failures onto a 403. (api_pinned.go:106-112,264-330) DB errors (IsFriend,QueryPeerRobotInfo,AreSpaceMembers) and an unregistered group checker all surface asErrUserChannelAccessDenied(403) alongside genuine access denials. This is the opposite direction of a client-error regression (it masks a 5xx as a 4xx) — errors are still logged, so it's observable — but a DB outage on the add-pinned endpoint will read as a permission denial to monitoring. Already documented as a deliberate follow-up atapi_pinned.go:107-109andpkg/errcode/user.go:452-455. Suggest splitting the internal branches ontoErrUserQueryFailed(500) when the follow-up lands. -
Pre-existing (not introduced by this PR): a few Commit-failure / GenSeq paths lack an explicit Rollback.
api_friend.go:940(friendSure Commit failure) andapi_manager.go:1127-1132(addSystemFriendGenSeq) rely only on the panic-recover defer. The diff only swapped the response call in these spots (andaddSystemFriend's body is not in the diff at all), so these are out of scope here, but worth a consistency pass later to match thedeletehandler'stx.RollbackUnlessCommitted(). -
Pre-existing (not introduced by this PR): two non-password enumeration oracles remain. Pure-code email login (
api_emaillogin.go:276-294) distinguishes user-not-found (404) / account-unavailable (403) / code-invalid (400) without a password; Web3 challenge/reset (api_usernamelogin.go:247-252, 407-414, 462-468) distinguishes user-not-found vs no-public-key. The legacy copy was already distinguishable, so this is not a regression — and this PR actually fixes a raw-Redis-error leak in the same Web3 functions. Consider unifying these states in a future hardening pass.
Nits
ErrUserPublicKeyNotFound/ErrUserSignatureNotFound(HTTP 400, not 404) andErrUserListFilterConflict(HTTP 400, not 409) have names implying a different status than registered. The status is correct for each branch (login-flow preconditions / mutually-exclusive query filters = bad request); only the symbol name could mislead. A one-line comment on each definition would document the intentional divergence.api_manager.gologin has nologinGuardlockout (anti-enumeration is correct, but no brute-force counter for admin accounts). The code comments explain this preserves a SuperAdmin emergency channel — acceptable trade-off, noted only for awareness.
Recommendation for the human reviewer (security-sensitive)
The migration is mechanically clean and the previously-blocking issues are resolved. Before merge, a human may want to confirm the deliberate behavior change is acceptable to clients: HTTP status codes shift from a uniform 400 to semantic values (401/403/404/409/429/500). The dual envelope still emits {msg,status} for legacy compatibility and clients are expected to read error.code, so impact should be contained — but any client branching on the literal HTTP status of these /v1/user/* endpoints would observe the change.
## Summary Phase 2.1 i18n migration of `modules/group` — the next hot module after `thread` (#176) and `user` (#188 / #197). All **386** legacy error-response sites across `api.go` (296), `api_manager.go` (46) and `invite.go` (44) are migrated from the legacy `c.ResponseError(...)` / `c.ResponseErrorWithStatus(...)` envelope to the localized `httperr.ResponseErrorL` facade, reusing the established thread/user playbook. Organized as 9 reviewable commits: code registry → respond helpers → per-file migration → tests → business-error classification fixes. ## What changed - **39 new `err.server.group.*` codes** (`pkg/errcode/group.go`) + zh-CN runtime translations + regenerated AST marker. Codes are grouped by class: validation (400), permission (401/403), not-found (404), conflict (409), rate-limit (429), and `Internal=true` query/store/notify (500). - **`modules/group/api_i18n.go`** respond helpers (`respondGroupRequestInvalid` with field detail, `respondGroupMdContentTooLarge`, shared `respondGroupForbidden` / `respondGroupNotLoggedIn`). `getGroupInfo` now returns sentinel errors so its ~14 call sites map to 404-vs-500 via `respondGroupInfoError` instead of leaking a raw string at a flat 400. - Internal failures (DB read/write, transaction/event, IM send) collapse to `Internal=true` codes; every such site keeps (or gains) a `g.Error(..., zap.Error(err))` log so the underlying error is never put on the wire. - Business rejections returned by the service layer (`AddGroupMembers` / `RemoveGroupMembers`) are classified to user-facing codes — external-join forbidden (403), blank-members (400), member-not-in-group (404), group-disbanded TOCTOU (404) — rather than collapsing to a 500. ## Behavior change (frontend, please note) Following the i18n design (D14), every migrated error response now returns **HTTP wire status 400** during the compatibility window; the semantic status lives in `error.http_status` and clients should branch on `error.code`. Notably, four previously-`403` paths (bot-ownership / external-member rejections) now return a `400` envelope with `error.http_status: 403`. Web/admin clients consuming these endpoints must read `error.code` / `error.http_status` rather than the HTTP status. This is consistent with the thread/user migrations already merged. ## Test plan - [x] `go build ./...` - [x] `go vet ./modules/group/` - [x] `make i18n-lint` (D23 direct-error-response ratchet + unregistered-code) - [x] `make i18n-extract-check` (AST marker recall) - [x] `go test ./modules/group ./pkg/errcode ./pkg/i18n/...` - [x] New tests: `TestGroupNoLegacyResponseError` (source guard over all 3 files), `TestRespondGroupHelpers` (helper / sentinel / Internal-no-leak / dual-envelope), and regression tests for the business-error classifications (group-exit not-found, expired invite auth code, blank members, admin-remove non-member). ## Follow-ups (non-blocking) - Service-layer sentinel errors (`errors.Is`) to replace the err-string matching used for business-vs-internal classification — the same shared follow-up tracked for thread/user. - `groupSettingUpdate` action errors are collapsed to `store_failed`; distinguishing invalid-value (400) / no-permission (403) needs the same service sentinels. --------- Co-authored-by: an9xyz <an9xyz@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
4 participants
Summary
Completes the
modules/useri18n migration (Phase 2.1): every handler inmodules/useris moved off the legacyc.ResponseError/c.ResponseErrorf/c.Response("<string>")paths onto the localizedhttperr.ResponseErrorLenvelope.api.golanded in #188; this PR covers the remaining 11 handler files (~385 sites) and touchesapi.goonly to add the shared execLogin error classifier.TestMigratedUserFilesNoLegacyResponseError+ the api.go / api_manager guards pin the module against regressing to legacy response calls (including thec.Response("<string>")-as-error shape).Files
api_manager.goc.Response("...")bug fixes)api_friend.goapi_online.go/api_setting.go/api_maillist.go/api_device.goapi_destroy.goapi_pinned.goapi_gitee.go/api_github.goapi_emaillogin.goapi_usernamelogin.goc.Response(errors.New(...))bug fix)err.server.user.*codes (+ reuse of the feat(i18n): migrate modules/user/api.go to ResponseErrorL (Phase 2.1) #188 set and sharedauth.forbidden).respondManagerForbidden,respondUserListFilterConflict,respondUserPinnedLimitExceeded.Error classification (status semantics)
The migration's goal is semantic status codes, so client-actionable states stay 4xx and only genuine internal faults are 5xx:
403; validation /BindJSON→400; service/DB/third-party errors are logged server-side and never puterr.Error()on the wire.invalid_credentials, code path → original semantics) is preserved unchanged;loginGuard.Check→login_locked(429), closing the 3 sites @yujiawei flagged (destroy/email/username).execLoginerrors are now classified in one place (respondExecLoginError, shared by main / OAuth / email / username): disabled account →403, missing device info →400,ErrUserNeedVerificationkeeps its110response, genuine internal failure →500. This also fixes the main login path (execLoginAndRespose) which previously collapsed disabled/device onto500.429(ErrUserEmailRateLimited, via a typedErrEmailSendRateLimitedsentinel), not a500send-failure.400(ErrUserFriendApplyInvalid); only a real Redis fault →500.Statuses move from a uniform 400 to semantic values (401/403/404/409/429/500), consistent with the
/v1/user/*migration in #188. The dual envelope still emits{msg,status}for legacy compatibility; clients readerror.code. The execLogin classifier additionally changes the main login's disabled-account response from 500 → 403 (a correctness improvement, aligned with #188's stated goal).Tests & gates
TestRespondUserHelpers(helper/code coverage),TestRespondExecLoginError(4-way execLogin classification),TestMigratedUserFilesNoLegacyResponseError(legacy-call guard over all 11 files).go build ./...,go vet,make i18n-extract-check,make i18n-lint(D23 + unregistered-code) green;go test ./modules/user/ ./modules/base/common/ ./pkg/i18n/... ./pkg/errcode/...passes (DB-backed included).Review rounds resolved
tx.Rollback()added to all early returns afterBegin()infriendSure/delete/addUser; gofmt on touched files;zap.Erroradded to internal-error logs.Follow-up (not blocking)
EmailService.Verify/source.Check*paths (internal-vs-validation), and the SMS resend cooldown (service_sms.go) reusing the same sentinel pattern.addSystemFriendhelper tx-rollback (untouched by this PR; returnserror, not a response).api_pinned.govalidateChannelAccessinternal failures collapse to 403 (logged) — splitting to 5xx deferred.