feat(user): add POST /v1/users/batch with whitelist projection

[boris-clark]

Summary

Adds a new POST /v1/users/batch endpoint to batch-fetch users by uid, returning only a whitelist projection (uid, name, avatar). This supports clients that need to resolve many collaborators at once (e.g. document collaborator lists / presence) without N round-trips, while keeping sensitive fields off the wire.

What it does

• New route POST /v1/users/batch mounted in the existing authenticated /v1 group (under AuthMiddleware).
• Request body: { "uids": ["...", "..."] }, capped at 200 uids per call.
• Reuses the existing (*Service).GetUsers service method — no change to it or any existing handler.
• Returns a dedicated whitelist DTO with exactly three fields: uid, name, avatar. The full user record (which carries phone/email) is never serialized — only those three fields are hand-mapped, so sensitive fields cannot reach the response.
• avatar is the per-uid avatar path, consistent with how the existing user endpoints emit it.

Behavior

• Empty uids → empty array.
• Unknown uids → silently skipped (the underlying IN query returns only matched rows); partial results are not an error.
• Over the 200 cap or malformed body → 400 invalid-request error.
• DB/service failure → 500 (internal), logged.

Scope / safety

• Only adds a new file (modules/user/api_batch.go), one route-registration line, and tests. No existing endpoint, the GetUsers service, or the user response struct is modified.
• New tests assert the response contains only uid/name/avatar (no phone/email) and that the 200 cap is enforced.

Testing

• go build ./... — pass
• go vet ./modules/user/... — pass
• New unit tests (api_batch_test.go) — pass
• make i18n-lint — pass (no new error responses, no unregistered codes)

[lml2468]

✅ APPROVE — 批量取用户端点,白名单投影设计到位(手工拷 3 字段 + 反射测试锁定 + 数量上限)。无 blocking,两条 should-fix(均非阻塞)。

安全核查通过(逐项亲核)

• 认证:路由挂在 auth Group(AuthMiddleware),需登录态,非公开端点。
• 白名单投影密不透风:handler 手工只拷 uid/name/avatar 到 batchUserItem(仅 3 个显式字段、无嵌入),c.JSON 序列化的是 batchUserItem 而非 service 层 Resp → Resp 的 Phone/Email 不会上线。TestBatchUserItemWhitelist 用反射锁死「恰好 3 字段且不含 phone/email」,未来加字段/改回序列化 Resp 都会红测。
• SQL 安全:queryByUIDs 用参数化 WHERE uid in ?,精确匹配,无注入/通配枚举。
• 数量上限:>200 直接 request_invalid 拒(非静默截断),防批量枚举/DoS;空列表短路返回空数组。
• avatar 由 GetAvatarPath(uid) 按 uid 拼稳定 URL,无 secret/token 泄露。
• 越权面:任意登录用户可传任意 uid 拿 uid/name/avatar —— 但这三者是协作场景(文档协作者/presence)的公开展示信息,本就需跨成员解析,且仅此 3 个公开字段,符合 body 声明的用途,非越权泄露。
• vet 干净;反射白名单测试 + 超量拒绝测试本地实跑 PASS。

should-fix(非阻塞)

1. service 层 GetUsers 用 Select("*") 取全列(db.go:queryByUIDs):虽然本端点 wire 投影只输出 3 字段、当前无泄露,但 Select("*") 把 phone/email 等敏感列全捞进内存再丢弃。深度防御角度建议 GetUsers(或新增一个专用投影 query)只 SELECT 必要列——既减少敏感数据驻留,也防未来有人误改 handler 直接序列化 Resp 就泄露。注:这是既有 GetUsers 的行为、非本 PR 引入,本 PR 只是复用,故非 blocker;但本 PR 新增了一个"对外只暴露公开字段"的端点,正是收敛投影查询的好时机。
2. 返回裸 JSON 数组 c.JSON(200, items),与本仓其他列表端点的 {list:[...]}/{count,list} 风格不一致,建议统一封装(API 一致性 nit)。

安全主判断(投影、注入、上限、认证)全过。批准。

—— Octo-PR (lml2468)

[Jerry-Xin]

This PR is relevant to octo-server and the implementation is small, scoped, and aligned with the stated whitelist projection goal.

🔴 Blocking
None.

💬 Non-blocking
🟡 Warning: modules/user/api.go mounts POST /v1/users/batch under AuthMiddleware, but not under SharedUIDRateLimiter. Given this endpoint can resolve up to 200 users per request, consider adding the shared per-UID limiter to this route or documenting why the existing global/IP limits are sufficient.

🔵 Suggestion: modules/user/api_batch_test.go tests the oversized request and DTO shape, but not the success response body. A small success-path test with a fake IService would lock in [] for empty input and exact uid/name/avatar JSON for matched users.

✅ Highlights
The response DTO in modules/user/api_batch.go is explicitly hand-mapped, so Phone and Email from Resp are not serialized.

The 200 UID cap is enforced before service access in modules/user/api_batch.go, and error paths use the module’s localized helpers.

The legacy error-response guard was updated for the new file in modules/user/api_i18n_test.go.

Verification run: go test ./modules/user -run 'TestBatch|TestMigratedUserFilesNoLegacyResponseError' passed.

[lml2468]

认同 Jerry-Xin 第 1 条(限流),补认为 should-fix —— 我上轮 review 漏提了这条。

should-fix(我漏提):POST /v1/users/batch 未加 per-UID 限流

本端点挂在 auth Group(仅 AuthMiddleware),无 SharedUIDRateLimiter。批量端点 + 单次 200 uid 上限,在无限流下任意登录用户可高频调用,放大枚举全站用户 name/avatar 的效率(一次 200)。

严重性:非 blocking,但建议加。理由:

• 暴露的只是 name/avatar(协作场景公开展示字段,非敏感 PII),且需登录态;
• 同 Group 的 GET /users/:uid 本就无 per-UID 限流,批量端点没引入比"循环单查"更糟的面,200 上限反而比无限单查可控;
• 但批量端点天然是"放大枚举效率"的入口,加 SharedUIDRateLimiter 是合理纵深防御 —— 与 modules/space members/search 端点已采用的同款限流一致(那里也是 admin-only 列表 + per-UID 限流防枚举)。建议比照加上。

我上轮提了 Select("*") 深度防御 + 裸数组风格两条,漏了限流这条 —— 批量/列表端点应条件反射地核「有没有 per-UID 限流防枚举」,我没联想到 space 那边的先例。感谢 Jerry-Xin 补。

第 2 条(补成功响应体的端到端测试)同意,非阻塞:现有测试覆盖了白名单字段反射 + 超量拒绝,可再补一条成功返回 body 形状(仅 uid/name/avatar、值正确)的用例。

白名单投影、SQL 参数化、数量上限、认证这些主判断不变,无 blocking。建议合并前加上 per-UID 限流。

—— Octo-PR (lml2468)

[mochashanyao]

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

---

octo-server PR#401 Review Report

Reviewer: Octo-Q (automated review)
PR: #401
Head SHA: 751f7df2a86aec818c2e63c1449d26392fdf64a7
Title: feat(user): add POST /v1/users/batch with whitelist projection

---

1. Verification Summary

Item	Status	Evidence
Whitelist projection (uid/name/avatar only)	✅	api_batch.go:27-31 — batchUserItem has exactly 3 fields; Phone/Email never copied
Auth gate	✅	api.go:207 — route mounted under auth := r.Group("/v1", u.ctx.AuthMiddleware(r))
200-uid cap	✅	api_batch.go:48-51 — len(req.UIDs) > maxBatchUserUIDs → 400
Empty input → empty array	✅	service.go:1027-1029 — GetUsers short-circuits nil, nil; handler returns [] (non-nil empty slice)
Malformed JSON → 400	✅	api_batch.go:43-46 — BindJSON error → respondUserRequestInvalid
DB failure → 500 + log	✅	api_batch.go:54-57 — zap.Error(err) + respondUserError(c, errcode.ErrUserQueryFailed)
SQL injection safety	✅	db.go:159 — parameterized Where("uid in ?", uids)
Avatar URL consistency	✅	api_batch.go:64 — GetAvatarPath(resp.UID) matches pattern in api.go:2271, app_bot.go:269, etc.
Test: whitelist contract	✅	api_batch_test.go:22-40 — reflection asserts exactly 3 fields, bans phone/email
Test: oversized batch	✅	api_batch_test.go:43-65 — 201 uids → err.server.user.request_invalid
i18n test coverage	✅	api_i18n_test.go:85 — api_batch.go added to migrated-files list

2. Findings

F1 — P2: No per-UID rate limiting on batch endpoint

Diff-scope: new (this PR introduces the endpoint without rate limiting)

The batch endpoint can return up to 200 user records per request but has no per-UID rate limiter. By comparison, the search endpoint in the same route group applies searchLimit (api.go:205). An authenticated client could enumerate the user table at scale (200 users/request × unlimited requests).

Other modules consistently apply SharedUIDRateLimiter on their auth groups (e.g., group/api.go:133, space/api.go:100, usersecret/api.go:97). The user module's own /v1 group is one of the few that doesn't carry it, but the search endpoint at least has a dedicated limiter.

Recommendation: Add a per-UID rate limiter to the batch route, e.g.:

batchLimit := appwkhttp.SharedUIDRateLimiter(r, u.ctx)
auth.POST("/users/batch", batchLimit, u.batchGetUsers)

F2 — P2 (pre-existing): Destroyed/banned users included in results

Diff-scope: pre-existing — the underlying queryByUIDs (db.go:157-161) does SELECT * FROM user WHERE uid IN ? with no is_destroy or status filter. The existing GET /users/:uid endpoint also returns destroyed users (via GetUserDetail → same table). This PR does not amplify or change that behavior.

Note: If the product intent is that destroyed users should be invisible, this should be addressed at the service/DB layer (not this PR).

F3 — P2/nit (pre-existing): Response envelope inconsistency

Diff-scope: pre-existing pattern — the batch endpoint uses c.JSON(http.StatusOK, items) while the single-user get handler uses c.Response(). However, c.JSON() is an established pattern for array responses in this module (api_online.go:69, api_space_setting.go:42). Not a regression.

3. Suggestions

1. Rate limiting (F1): Add SharedUIDRateLimiter or a dedicated batch limiter before landing. This is the only actionable item.
2. Duplicate UIDs: Consider deduplicating req.UIDs before passing to GetUsers to avoid redundant DB lookups. Minor optimization, not a correctness issue (SQL IN naturally deduplicates).
3. Content-Length guard: BindJSON will reject bodies exceeding gin's default max body size, but consider an explicit Content-Length check if you want to reject very large payloads early (defense-in-depth against memory pressure from 200 very long UID strings).

4. Additional Observations

• The queryByUIDs (lowercase, db.go:157) used by GetUsers lacks the empty-slice guard that QueryByUIDs (uppercase, db.go:186) has. The service layer compensates (service.go:1027), but the asymmetry is a latent risk if a new caller bypasses the service.
• The test TestBatchGetUsersRejectsOversizedBatch uses a zero-value *User{} which would nil-panic if userService were called. This is fine because the cap check runs first, but the test name could clarify it only exercises the cap-guard path.

5. Data Flow Trace

Consumed Data	Upstream Source	Reaches Consumer?
req.UIDs	c.BindJSON → JSON body uids field	✅ Validated (type + length) before use
resps (from GetUsers)	db.queryByUIDs → SELECT * FROM user WHERE uid IN ? → []*Model → newResp() → []*Resp	✅ Flows correctly; empty input short-circuits to nil, nil
resp.UID	Model.UID → Resp.UID (via newResp)	✅ Copied to batchUserItem.UID
resp.Name	Model.Name → Resp.Name (via newResp)	✅ Copied to batchUserItem.Name
resp.Phone	Model.Phone → Resp.Phone	✅ Dropped — not copied to batchUserItem
resp.Email	Model.Email → Resp.Email	✅ Dropped — not copied to batchUserItem
Avatar URL	Config.GetAvatarPath(resp.UID)	✅ Constructed at response time, consistent with other endpoints

6. Blind-spot Checklist (R5)

• C1 — Dual-path parity: N/A. This is a new read-only endpoint with no symmetric create/delete pair.
• C2 — Control-flow ordering / nesting reuse: Clear. The handler is a linear sequence: bind → validate → query → project → respond. No nested/reused security controls.
• C3 — Authorization boundary ≠ capability boundary: Clear. The endpoint is behind AuthMiddleware (same as GET /users/:uid). Any authenticated user can look up any UID — consistent with the existing single-user endpoint. No new capability is exposed beyond what GET /users/:uid already provides.
• C4 — Authorization lifecycle / container-member cascade: N/A. This is a flat user lookup with no container/org/space hierarchy involved.

7. Cross-round Blocker Recheck (R6)

N/A — first review of this PR.

---

[Octo-Q] verdict: APPROVE

No P0/P1 findings. The whitelist projection is correctly implemented, auth is in place, error handling follows project conventions, and tests lock the privacy contract. The rate-limiting gap (F1, P2) is worth addressing but not blocking — it's a defense-in-depth improvement consistent with how other endpoints in the codebase are protected.

[lml2468]

✅ 基于最新 commit 3c71a48 重审,维持 APPROVE。限流 should-fix 已修;mochashanyao 的「已注销/禁用用户未过滤」我深挖后认为是该修的 should-fix(偏重),但非本 PR 独创的泄露路径,故不独自升 blocking——把事实摆清供作者/maintainer 定夺。

已修:per-uid 限流(我和 Jerry-Xin/mochashanyao 都提的)

新 commit 给 /users/batch 加了 SharedUIDRateLimiter(api.go:216),批量端点的枚举放大面被限流收住。✅

should-fix(偏重,建议合并前修):批量结果未过滤已注销/禁用用户

mochashanyao 🔴#2 指向真实隐私问题,我深挖确认:

• queryByUIDs 是 WHERE uid in ?,无 is_destroy / status 过滤。
• 关键:账号注销流程(finalizeDestroy/destroyAccount)只匿名化 phone/username,不动 name(api_destroy.go:191 注释明写"匿名化 phone/username",name 不在内)。所以 is_destroy=2 的已注销用户其 name 仍是真实姓名 → 批量端点会泄露已注销用户的真实姓名;status=0 的禁用用户也会返回真实 name/avatar。

严重性平衡(为何我标 should-fix 偏重、未独自升 blocking):

• 这不是批量端点独创的泄露:单查 GET /users/:uid 走的 QueryDetailByUID 同样 WHERE user.uid=? 无 is_destroy/status 过滤——任意登录用户单查一个已注销 uid 也能拿到其真实 name。即既有全局行为,批量只是放大规模(200/call)。
• 需登录态。
  但批量端点是新的规模化暴露点,且"已注销用户真实姓名泄露"是实打实的隐私问题,理应在这个新端点上过滤。建议:GetUsers/queryByUIDs(或本端点专用 query)加 is_destroy <> 2 AND status = 1 过滤(注销/禁用用户不出现在批量结果)。既有单查的同类问题建议单独 issue 跟进(超本 PR 范围)。

我上轮的判断更新

我上轮把"GetUsers 无 status 过滤"判为"只暴露公开字段、可接受"——偏轻。挖出"name 未匿名化"后我认同这值得修(mochashanyao 促我挖深,谢)。但鉴于单查同样行为、非本 PR 引入,我标 should-fix 偏重而非 blocking;是否 block 由 maintainer 按"批量新端点是否必须先堵这个既有泄露"定。

其余

• 白名单投影(手工 3 字段+反射锁)、SQL 参数化、200 上限、认证、限流——主判断都过。
• 仍建议(上轮):GetUsers 用 Select("*") 取全列,深度防御应只取必要列;返回裸数组与 {list} 风格不一致。

限流已修,无 blocking。强烈建议合并前补 is_destroy/status 过滤。

—— Octo-PR (lml2468)

[Jerry-Xin]

Re-reviewed on head 3c71a48 after the rate-limit commit. APPROVE.

What changed since my (now-dismissed) prior review

• The author added per-uid rate limiting to the batch route: auth.POST("/users/batch", appwkhttp.SharedUIDRateLimiter(r, u.ctx), u.batchGetUsers) (modules/user/api.go:216), with a comment acknowledging it is an amplification endpoint fanning out to up to 200 uids. This resolves the strongest hardening concern (mass enumeration rate amplification) raised across reviews.

Verified at this head (receipts)

• ✅ Auth gate: route is under auth group (AuthMiddleware) — login required.
• ✅ Rate limit: SharedUIDRateLimiter now wraps the handler (api.go:216).
• ✅ Whitelist projection is airtight: batchUserItem has exactly 3 explicit fields (uid/name/avatar), no embeds; handler hand-copies those 3 and serializes batchUserItem (not the service Resp), so phone/email never reach the wire. The reflection test locks "exactly 3 fields, no phone/email."
• ✅ Batch cap = 200, validated before the service call, 400 on overflow (not silent truncation), tested.
• ✅ SQL parameterized: queryByUIDs uses Select(...).Where("uid in ?", uids) (gocraft/dbr bound params) — no injection / no wildcard.

On the two 🔴 raised by another reviewer

1. No per-uid rate limit → enumeration risk — RESOLVED by 3c71a48 (SharedUIDRateLimiter added). No longer open.
2. Results include destroyed/banned users (no status filter in queryByUIDs) — I assess this as a non-blocking should-fix / product decision, not a security blocker. The endpoint returns only uid/name/avatar (public collaboration display fields). For a collaborator/presence-resolve endpoint, resolving the display name/avatar of historical or deactivated users is typically required (you still render authorship/presence on existing content). No sensitive field is exposed. If product intent is to exclude deactivated users, add a status filter in a dedicated query — but that is a product choice, not a vulnerability. queryByUIDs is a pre-existing shared helper (not introduced by this PR).

Non-blocking should-fix (🟡)

• queryByUIDs uses Select("*") (db.go:157), pulling sensitive columns (phone/email/etc.) into memory even though the wire projection only outputs 3 fields. The wire output is safe today, but defense-in-depth favors selecting only the needed columns in a dedicated batch query — both to keep sensitive data out of memory and to prevent a future edit that serializes Resp from leaking. Pre-existing behavior, but this PR is a good point to converge it.
• Response is a bare JSON array rather than the repo's {list:[...]} convention — minor API-consistency nit.
• Consider de-duplicating input uids before the IN query (minor efficiency).

Net: security design is sound (strict whitelist projection + reflection-test lock + batch cap + now per-uid rate limit), the one sharp concern (rate limit) is fixed, and the remaining items are non-blocking. APPROVE.

[yujiawei]

Code Review — PR #401 (octo-server)

Reviewed at head 751f7df2a86aec818c2e63c1449d26392fdf64a7 against merge-base dcc84ef. Built and ran the package locally: go build ./modules/user/..., go vet ./modules/user/..., and the new unit tests all pass.

Verdict: Approve. The core privacy contract is implemented correctly and is well-tested. The findings below are non-blocking improvements — none introduce a regression versus the existing user endpoints, and the most security-relevant one (rate limiting) is a pre-existing, group-wide gap rather than something this PR introduces. I'd recommend addressing the rate-limiter item soon (it's a one-liner), but it doesn't block merge.

1. What's correct (verified)

• PII whitelist projection is sound. batchUserItem exposes exactly uid/name/avatar; the handler hand-copies those three fields instead of serializing the service-layer Resp (which carries Phone/Email at modules/user/service.go:1268). phone/email cannot reach the wire, and api_batch_test.go:TestBatchUserItemWhitelist locks this via reflection. Good defensive design.
• The 200 cap is enforced and tested, and rejecting over-cap with 400 (rather than silently truncating) is the right call — it's documented at api_batch.go:11-14.
• Empty / missing / malformed input is safe. A bad body is caught by BindJSON → request_invalid; null/empty uids short-circuit in GetUsers (service.go:1026, len<=0 → nil,nil), and ranging a nil slice is a no-op — no nil-deref, the DB never sees an empty IN ().
• No SQL injection. queryByUIDs (db.go:157) uses dbr Where("uid in ?", uids), which expands to bound placeholders. With the 200 cap it stays far under MySQL placeholder/packet limits even with duplicate uids.
• Error mapping / i18n is correct. Both request_invalid (400) and ErrUserQueryFailed (500, Internal=true, logged) are registered and translated, and api_batch.go is added to the migrated-files i18n guard test.
• Avatar shape is consistent with its true peers. GetAvatarPath(uid) returns a relative path, matching in-app endpoints (auth-code login api.go:2271, conversation api_conversation.go:1172). Only the external openapi surface prepends APIBaseURL.

2. Findings (all P2 — non-blocking)

P2-1 — New authenticated endpoint is missing the documented default per-user rate limiter

modules/user/api.go:207-211 — the route is mounted on the bare r.Group("/v1", AuthMiddleware) group, which carries no SharedUIDRateLimiter. Per this repo's contributor docs, SharedUIDRateLimiter is the default for authenticated endpoints, and most peer modules mount it (e.g. modules/message/api_conversation.go:100, modules/category/api.go:39, modules/app_bot/app_bot.go:142, modules/incomingwebhook/api.go:197). With a 200-uid cap and no per-uid quota, a single token can bulk-resolve uid → name/avatar at line rate.

Why this is not a blocker: it introduces no new data exposure (the single GET /v1/users/:uid already returns name/avatar to any authenticated user with no limiter), uids are non-guessable UUIDs (util.GenerUUID()), a global per-IP floor already covers the route, and the closest comparator POST /v1/user/online shares the same posture while being less defensive (no cap, no dedup). So this PR does not regress the security posture — but it's a good moment to close the gap.

Recommendation (one line, per-route so it doesn't change the rest of the legacy group):

auth.POST("/users/batch", appwkhttp.SharedUIDRateLimiter(r, u.ctx), u.batchGetUsers)

P2-2 — Bot/robot rows are returned with no way to distinguish them

db.go:157 selects all matching users with no robot filter, and the DTO (api_batch.go:25-29) drops the robot flag that newResp populates (service.go:1284). The single-GET path does extra robot-table enrichment/gating (service.go:526-558, WHERE robot_id=? AND status=1); batch bypasses all of it and will happily return bot uids (including disabled bots) as plain {uid,name,avatar}. Decide intentionally: either surface the robot flag in the projection (so clients can tell bots from humans, as the single-GET response does) or filter robot=0.

P2-3 — Destroyed / disabled users are returned (latent, pre-existing)

queryByUIDs applies no is_destroy/status filter, so destroyed (is_destroy=2, which keeps name) and disabled accounts come back as normal-looking entries. This mirrors the existing single-GET behavior (QueryDetailByUID is equally unfiltered), so it's not introduced here — but batch makes bulk discovery of such accounts cheap. Worth a conscious decision on whether the projection should filter them.

P2-4 — Happy-path handler test gap

The two new tests cover the struct shape (reflection) and the over-cap guard (which returns before touching userService/ctx). Nothing exercises the success path: partial results for unknown uids, the actual avatar value, and that phone/email truly don't appear in the rendered JSON of a populated row. A testutil.NewTestServer()-backed test seeding 2-3 users (plus one unknown uid) and asserting the raw JSON keys would close the gap between the struct-shape lock and real behavior.

3. Minor / nits

• Per-uid input hardening: the cap bounds the slice length but not individual string sizes, and BindJSON has no MaxBytesReader (this is true of all /v1 JSON routes, not just this one). Negligible given auth + the global per-IP floor, but trimming/length-checking uids and dropping blanks before the query would be a cheap robustness win.
• Dedup: de-duplicating uids before the IN query is harmless but not required — IN is set-membership, so duplicates resolve to the same rows with no amplification.

4. Coverage note / out of scope

The /v1 user group also has no SpaceMiddleware, so uid → name/avatar is resolvable across spaces. This matches the pre-existing single-GET posture, so it's not a regression from this PR; if cross-space name/avatar exposure is undesirable, that's an architecture-level decision spanning both endpoints and should be handled separately, not in this PR.

[lml2468]

QA Verdict: COMMENT

Context read

• PR body + diff (4 files, +141/-1): new route + handler + 2 tests
• No Fixes #/Closes # linked issue (greenfield endpoint, not a bug fix)
• CI: Build/Vet/Lint/i18n PASS. Test job FAILED — root cause is --- FAIL: TestGroupSettingUpdate_AllowNoMentionRangeIsRequestInvalid with panic: Error 1040: Too many connections in modules/group. Unrelated to this PR — pre-existing MySQL connection flake in a different package. user module not exercised in the failed assertions.

Test coverage analysis

Strong:

• TestBatchUserItemWhitelist — reflect-based contract test locks exactly 3 fields and bans phone/email/Phone/Email. This is the privacy-critical contract; locking it via reflection is the right call (future struct edits can't silently leak fields).
• TestBatchGetUsersRejectsOversizedBatch — exercises the >200 cap, asserts error code err.server.user.request_invalid.
• api_batch.go added to TestMigratedUserFilesNoLegacyResponseError migration list — drift-guards i18n envelope use.

Gaps (non-blocking, recommend adding):

1. No happy-path test. The projection loop (resp.UID/Name/Avatar → batchUserItem) is unexercised. A future refactor swapping field names could regress without detection — the whitelist test only checks the DTO shape, not that the handler populates it correctly. Adding a fake userService and a 2-uid case asserting the returned JSON shape would close this.
2. Boundary not tested. Only maxBatchUserUIDs+1 (201) is asserted to fail. The 200 case (boundary should accept) is not asserted. If >= slips in instead of >, current tests pass.
3. Empty uids claim unasserted. PR body says "Empty uids → empty array" and the doc comment claims GetUsers short-circuits to nil,nil. Neither is exercised. A 0-uid request should be asserted to return [] (not null) — important for clients parsing JSON arrays.
4. Unknown uid silent-skip claim unasserted. Body says "Unknown uids → silently skipped". No test confirms partial-result semantics. Worth a 2-uid test where 1 exists and 1 doesn't.

Verify

CI Build/Vet/Lint green for the user package. Local re-run skipped — modules/group flake masks an unrelated MySQL connection limit; not actionable from this PR. Recommend re-running CI once modules/group flake is fixed to get a clean signal, but not blocking this review.

Verdict

COMMENT — privacy-critical contract is locked; cap boundary is exercised on the upper side; CI failure is unrelated flake. Coverage gaps are quality improvements, not safety issues.

[lml2468]

Security Verdict: COMMENT (CLEARED-WITH-RISK)

STRIDE / OWASP pass on the new surface

• Spoofing / AuthN: route mounted inside auth := r.Group("/v1", u.ctx.AuthMiddleware(r)) group — caller must be an authenticated user. ✓
• Tampering: request is parsed via BindJSON into a typed struct with one []string field; no SQL templating downstream. GetUsers uses an IN query with parameter binding (per the comment). ✓
• Repudiation: failure path logs 批量获取用户失败！ with zap.Error(err). Acceptable.
• Information disclosure (privacy-critical for this endpoint): response DTO batchUserItem exposes exactly uid / name / avatar. Sensitive Phone / Email fields on the service-layer Resp are dropped by hand-mapping (never serialized). TestBatchUserItemWhitelist locks this via reflection — strong defense against accidental field add. ✓
• DoS / amplification: this is a fan-out endpoint (1 request → up to 200 DB rows). Mitigations layered:
  • Per-request cap: maxBatchUserUIDs = 200, rejected as request_invalid (no truncation). ✓
  • Per-route per-uid limiter: appwkhttp.SharedUIDRateLimiter(r, u.ctx) registered alongside the handler. ✓
  • AuthMiddleware runs first (group middleware), so the caller uid is populated before the limiter reads it — registration order in api.go:211 is correct. ✓
• Elevation of privilege: no role gate; this is a generic authenticated read of public profile fields. Matches the existing GET /users/:uid posture, which is similarly open to any authenticated user. ✓
• SBOM / dep diff: no go.mod / go.sum changes in the diff. ✓

Residual risks (non-blocking)

1. Existence oracle, cheaper by ~200×. An authenticated client can iterate uid candidates and observe which exist via partial results. The same capability already exists via GET /users/:uid — the batch endpoint just makes it cheaper. The 200-cap + per-uid limiter bounds the rate, but not the cumulative discovery if many sessions cooperate. If uid is enumerable (sequential / guessable), this widens the oracle. Recommend documenting the per-uid limiter's bucket size and refill rate so the bound on enumeration rate is auditable.
2. No de-duplication of input. A caller can pass ["u1","u1",...,"u1"] 200 times; GetUsers likely de-dups via DB IN, but worth confirming the rate-limit bucket charges per request (not per resolved row).
3. Response item count is not capped separately. Cap is on input size. If GetUsers ever changes to return inflated rows (e.g. joins), the response could exceed len(req.UIDs). Currently safe — IN returns ≤ N rows — but a defensive len(items) <= maxBatchUserUIDs post-condition would harden against future service changes.

Verdict

COMMENT (CLEARED-WITH-RISK) — no blocking vuln. Whitelist contract is locked, AuthN/rate-limit/size-cap defenses are present and ordered correctly. The enumeration-oracle concern is pre-existing for the auth'd surface; this endpoint only widens cost economics, not the attack class.

[lml2468]

Code Verdict: APPROVE

Correctness

• BindJSON → cap check → service call → projection loop. Each early-return path emits an appropriate i18n error envelope (respondUserRequestInvalid for parse / cap, respondUserError(errcode.ErrUserQueryFailed) for service error). ✓
• Cap check uses > not >=, so the boundary value 200 is accepted as intended (len(req.UIDs) > maxBatchUserUIDs). ✓
• Whitelist projection copies UID / Name / Avatar explicitly — does not pass resp through; Phone / Email cannot leak. ✓
• Empty req.UIDs → GetUsers returns nil,nil (per doc comment) → items stays make([]*batchUserItem, 0, 0) → c.JSON writes [] not null. Correct JSON contract for clients parsing arrays.

Style / maintainability

• Avatar emission u.ctx.GetConfig().GetAvatarPath(resp.UID) matches the existing user-module pattern at modules/user/api.go:2288 ("avatar": u.ctx.GetConfig().GetAvatarPath(userModel.UID)). Consistent. ✓
• Slice pre-allocated with capacity hint (make([]*batchUserItem, 0, len(resps))). ✓
• New file kept narrowly scoped (handler + DTO + cap constant). No drive-by edits to Service.GetUsers or other handlers — surface area is minimal as PR body claims.
• Comments explain the non-obvious "why" (whitelist motivation, amplification rate-limit motivation, registration order, partial-result semantics, avatar field choice). Exactly the right kind of comment — explains intent, not what the code does.

Design

• Reusing (*Service).GetUsers rather than adding a new service method is the right call — avoids surface duplication.
• Putting the route inside the existing auth group keeps AuthN consistent; layering SharedUIDRateLimiter per-route (not group-wide) targets the amplification cost precisely.
• i18n migration list updated (TestMigratedUserFilesNoLegacyResponseError) so the drift-guard covers this file — meets the established convention for new handlers in this package.

Nits (optional)

• The two respondUserRequestInvalid(c, "uids") calls could carry slightly different reason hints ("uids: invalid" vs "uids: too many") for easier client debugging, but the i18n envelope is the contract — caller shouldn't depend on free-text. Leaving as-is is fine.
• Could add a // nolint or assertion that len(items) <= maxBatchUserUIDs as a forward-defense post-condition; not required given IN semantics.

Verdict

APPROVE — tight scope, correct, consistent with existing patterns, well-commented. Test coverage gaps flagged by QA are quality-of-life additions; nothing here blocks merge.

[lml2468]

Aggregate Verdict: RISKED — needs-human-review

Three reviewer verdicts are in. No blocking changes were requested, but two carry explicit risk markers, so this PR routes to a human maintainer rather than auto-clearing for merge.

Role	Verdict	One-line summary
qa	COMMENT (PASS-WITH-RISK)	Privacy-critical whitelist contract is locked via reflection test + cap upper-bound asserted; 4 coverage gaps (no happy-path, 200-boundary, empty-uids, partial-result semantics) are quality-of-life, not safety. CI Test failure on modules/group is a pre-existing MySQL Too many connections flake unrelated to this PR.
security	COMMENT (CLEARED-WITH-RISK)	STRIDE/OWASP pass: AuthN gate, IN-binding, whitelist DTO, 200-cap + per-uid SharedUIDRateLimiter ordered after AuthMiddleware. Residual risks: (1) cheaper existence-oracle on enumerable uids, (2) no input de-dup (check limiter charges per request, not per resolved row), (3) no defensive response-count post-condition. All non-blocking, pre-existing class.
code	APPROVE	Tight scope, correct early-returns, explicit projection, consistent with existing user-module avatar pattern, i18n drift-guard list updated. Two optional nits (richer reason hints, post-condition assertion).

What "RISKED" means here

• Privacy contract is sound and test-locked (reflection guard against accidental phone/email leak).
• Defenses against the amplification fan-out are in place (200-cap + per-uid limiter, correct middleware order).
• The remaining flags are information-only: enumeration-oracle widens cost economics (not the attack class), and QA gaps reduce future regression coverage but don't impair current safety.

Next step

Human maintainer review for merge. Suggested checks before merging:

1. Confirm SharedUIDRateLimiter bucket size/refill rate is documented or auditable (security residual risk fix: register app_bot and bot_api modules in startup import list #1).
2. Decide whether to require any of QA's 4 coverage additions in-PR or as a follow-up issue.
3. The unrelated modules/group CI flake is the reason the Test job is red — independently track / re-run; it does not gate this PR's correctness.

Do not auto-merge. Awaiting human decision.

— review-lead (loop)