Skip to content

Commit ed45f6f

Browse files
authored
feat(app_bot): delegate platform bot management to admin + localize the error/response surface (#375)
- platform /v1/admin/app_bot handlers: superAdmin → admin∪superAdmin (operations) - platform routes now reject space-scoped bots (botInRouteScope) — closes a cross-tenant token-exposure IDOR that the gate widening would otherwise have exposed (admin reaching any space's bot token by global id); updateBot also gains its missing existence check - localize the entire app_bot error/response surface via registered errcode + the i18n envelope, with correct semantics and status preservation (400/404/409/403/500, Internal hidden+logged); apply-flow not-found pinned to wire-400 (D14) - apply-flow success messages localized via an msgtmpl catalog - tests: source guard, responder status/code matrix, sqlmock route gate tests, a pure scope-guard test, and a full-server + real-MySQL e2e proving the cross-tenant guard - direct-error-response lint baseline for app_bot.go ratcheted 9 → 0 https://claude.ai/code/session_01G8ocbvm4BTcTUfyehwYB12
1 parent 0ac26c1 commit ed45f6f

12 files changed

Lines changed: 778 additions & 90 deletions

File tree

modules/app_bot/api_i18n.go

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
package app_bot
2+
3+
import (
4+
"github.com/Mininglamp-OSS/octo-lib/pkg/wkhttp"
5+
"github.com/Mininglamp-OSS/octo-server/pkg/errcode"
6+
"github.com/Mininglamp-OSS/octo-server/pkg/httperr"
7+
"github.com/Mininglamp-OSS/octo-server/pkg/i18n"
8+
)
9+
10+
// This file localizes the app_bot error responses. Every handler used to return
11+
// raw, unlocalized strings — c.ResponseError(errors.New("...")),
12+
// c.AbortWithStatusJSON(403, err.Error()) and c.JSON(40x, {"msg": "..."}) —
13+
// which leaked English/Chinese framework text straight onto the wire and could
14+
// not be language-negotiated. They now route through these helpers onto a
15+
// registered errcode + the i18n envelope. Status-preserving codes (404/409/403/
16+
// 500) use ResponseErrorLWithStatus so the console keeps branching on the real
17+
// wire status; validation stays at 400.
18+
19+
// respondAppBotForbidden renders the localized shared 403 for every app_bot
20+
// authorization guard: the platform /v1/admin/app_bot system-role gates, the
21+
// space-scoped checkSpaceAdmin gates, and the apply-flow space-membership check.
22+
// All collapse to one generic forbidden code (anti-enumeration) — the specific
23+
// role/membership reason stays in logs, never on the client.
24+
func respondAppBotForbidden(c *wkhttp.Context) {
25+
httperr.ResponseErrorLWithStatus(c, errcode.ErrSharedForbidden, nil, nil)
26+
}
27+
28+
// respondAppBotRequestInvalid covers malformed / empty request input (BindJSON
29+
// failure, invalid robot_uid, empty update). An empty field is omitted so the
30+
// renderer does not surface a noisy empty key.
31+
func respondAppBotRequestInvalid(c *wkhttp.Context, field string) {
32+
details := i18n.Details{}
33+
if field != "" {
34+
details["field"] = field
35+
}
36+
httperr.ResponseErrorL(c, errcode.ErrAppBotRequestInvalid, nil, details)
37+
}
38+
39+
// respondAppBotIDInvalid covers a bot id failing the format rule or colliding
40+
// with a reserved id.
41+
func respondAppBotIDInvalid(c *wkhttp.Context) {
42+
httperr.ResponseErrorL(c, errcode.ErrAppBotIDInvalid, nil, nil)
43+
}
44+
45+
// respondAppBotNotFound renders the status-preserving 404 for a missing or
46+
// scope-mismatched bot.
47+
func respondAppBotNotFound(c *wkhttp.Context) {
48+
httperr.ResponseErrorLWithStatus(c, errcode.ErrAppBotNotFound, nil, nil)
49+
}
50+
51+
// respondAppBotNotFoundPinned renders the same not-found code at the legacy
52+
// fixed-400 wire status (D14), for the user-facing /v1/app_bot/apply endpoint
53+
// whose SDK clients may branch on 400. The management-console paths use
54+
// respondAppBotNotFound (real 404) instead.
55+
func respondAppBotNotFoundPinned(c *wkhttp.Context) {
56+
httperr.ResponseErrorL(c, errcode.ErrAppBotNotFound, nil, nil)
57+
}
58+
59+
// respondAppBotIDConflict renders the 409 for a create colliding with an in-use id.
60+
func respondAppBotIDConflict(c *wkhttp.Context) {
61+
httperr.ResponseErrorLWithStatus(c, errcode.ErrAppBotIDConflict, nil, nil)
62+
}
63+
64+
// respondAppBotTokenRotationConflict renders the 409 for a lost token-rotation race.
65+
func respondAppBotTokenRotationConflict(c *wkhttp.Context) {
66+
httperr.ResponseErrorLWithStatus(c, errcode.ErrAppBotTokenRotationConflict, nil, nil)
67+
}
68+
69+
// respondAppBotQueryFailed / StoreFailed / IMTokenFailed / Internal render the
70+
// status-preserving 500. Internal=true hides the message — callers MUST log the
71+
// underlying err (zap.Error) with context before calling these.
72+
func respondAppBotQueryFailed(c *wkhttp.Context) {
73+
httperr.ResponseErrorLWithStatus(c, errcode.ErrAppBotQueryFailed, nil, nil)
74+
}
75+
76+
func respondAppBotStoreFailed(c *wkhttp.Context) {
77+
httperr.ResponseErrorLWithStatus(c, errcode.ErrAppBotStoreFailed, nil, nil)
78+
}
79+
80+
func respondAppBotIMTokenFailed(c *wkhttp.Context) {
81+
httperr.ResponseErrorLWithStatus(c, errcode.ErrAppBotIMTokenFailed, nil, nil)
82+
}
83+
84+
func respondAppBotInternal(c *wkhttp.Context) {
85+
httperr.ResponseErrorLWithStatus(c, errcode.ErrAppBotInternal, nil, nil)
86+
}

modules/app_bot/api_i18n_test.go

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
package app_bot
2+
3+
import (
4+
"encoding/json"
5+
"net/http"
6+
"net/http/httptest"
7+
"os"
8+
"strings"
9+
"testing"
10+
11+
"github.com/Mininglamp-OSS/octo-lib/pkg/wkhttp"
12+
"github.com/Mininglamp-OSS/octo-server/pkg/i18n"
13+
)
14+
15+
// TestAppBotNoLegacyResponseError pins that the module's HTTP surface renders
16+
// every error through the i18n envelope (httperr.ResponseErrorL* +
17+
// errcode.ErrAppBot* / shared codes) and never regresses to octo-lib raw
18+
// responses. Comments are stripped first so the migration breadcrumbs in
19+
// api_i18n.go (which name the old c.ResponseError / c.AbortWithStatusJSON forms)
20+
// don't trip the guard. Add any new handler file to the list below.
21+
func TestAppBotNoLegacyResponseError(t *testing.T) {
22+
files := []string{"app_bot.go", "api_i18n.go", "messages.go"}
23+
banned := []string{
24+
".ResponseError(",
25+
".ResponseErrorf(",
26+
".ResponseErrorWithStatus(",
27+
".AbortWithStatusJSON(",
28+
".AbortWithStatus(",
29+
"c.Response(\"",
30+
}
31+
for _, f := range files {
32+
t.Run(f, func(t *testing.T) {
33+
data, err := os.ReadFile(f)
34+
if err != nil {
35+
t.Fatalf("read %s: %v", f, err)
36+
}
37+
var clean strings.Builder
38+
for _, line := range strings.Split(string(data), "\n") {
39+
if idx := strings.Index(line, "//"); idx >= 0 {
40+
line = line[:idx]
41+
}
42+
clean.WriteString(line)
43+
clean.WriteByte('\n')
44+
}
45+
cleaned := clean.String()
46+
for _, b := range banned {
47+
if strings.Contains(cleaned, b) {
48+
t.Fatalf("modules/app_bot/%s must render errors via httperr.ResponseErrorL* / errcode.ErrAppBot*, not legacy %s", f, b)
49+
}
50+
}
51+
})
52+
}
53+
}
54+
55+
// TestAppBotNotFoundPinnedIsWire400 pins the apply-path responder: it carries the
56+
// real 404 in the envelope but keeps the legacy fixed-400 wire status (D14), so
57+
// existing /v1/app_bot/apply SDK clients that branch on 400 don't break.
58+
func TestAppBotNotFoundPinnedIsWire400(t *testing.T) {
59+
r := appBotHelperHarness(respondAppBotNotFoundPinned)
60+
req := httptest.NewRequest(http.MethodGet, "/probe", nil)
61+
rec := httptest.NewRecorder()
62+
r.ServeHTTP(rec, req)
63+
64+
if rec.Code != http.StatusBadRequest {
65+
t.Fatalf("wire status = %d, want 400 (D14 pinned for the apply path); body=%s", rec.Code, rec.Body.String())
66+
}
67+
var env appBotEnvelope
68+
if err := json.Unmarshal(rec.Body.Bytes(), &env); err != nil {
69+
t.Fatalf("decode envelope: %v; body=%s", err, rec.Body.String())
70+
}
71+
if env.Error.Code != "err.server.app_bot.not_found" {
72+
t.Fatalf("error.code = %q, want err.server.app_bot.not_found", env.Error.Code)
73+
}
74+
if env.Error.HTTPStatus != http.StatusNotFound {
75+
t.Fatalf("error.http_status = %d, want 404 (envelope keeps the real status)", env.Error.HTTPStatus)
76+
}
77+
}
78+
79+
// appBotEnvelope is the partial shape of an httperr.ResponseErrorL* response.
80+
type appBotEnvelope struct {
81+
Error struct {
82+
Code string `json:"code"`
83+
HTTPStatus int `json:"http_status"`
84+
} `json:"error"`
85+
}
86+
87+
func appBotHelperHarness(probe func(c *wkhttp.Context)) *wkhttp.WKHttp {
88+
r := wkhttp.New()
89+
r.SetErrorRenderer(i18n.NewErrorRenderer(i18n.NewLocalizer(i18n.DefaultLanguage)))
90+
r.GET("/probe", probe)
91+
return r
92+
}
93+
94+
// TestAppBotRespondHelpers asserts each responder renders its registered code at
95+
// the correct wire status: validation pins 400 (D14), while not-found/conflict/
96+
// forbidden/internal preserve the code's real status via ResponseErrorLWithStatus.
97+
// No DB/Redis needed — it only exercises the renderer.
98+
func TestAppBotRespondHelpers(t *testing.T) {
99+
cases := []struct {
100+
name string
101+
probe func(c *wkhttp.Context)
102+
wantStatus int
103+
wantCodeID string
104+
}{
105+
{"requestInvalid", func(c *wkhttp.Context) { respondAppBotRequestInvalid(c, "") }, http.StatusBadRequest, "err.server.app_bot.request_invalid"},
106+
{"idInvalid", respondAppBotIDInvalid, http.StatusBadRequest, "err.server.app_bot.id_invalid"},
107+
{"notFound", respondAppBotNotFound, http.StatusNotFound, "err.server.app_bot.not_found"},
108+
{"idConflict", respondAppBotIDConflict, http.StatusConflict, "err.server.app_bot.id_conflict"},
109+
{"tokenRotationConflict", respondAppBotTokenRotationConflict, http.StatusConflict, "err.server.app_bot.token_rotation_conflict"},
110+
{"queryFailed", respondAppBotQueryFailed, http.StatusInternalServerError, "err.server.app_bot.query_failed"},
111+
{"storeFailed", respondAppBotStoreFailed, http.StatusInternalServerError, "err.server.app_bot.store_failed"},
112+
{"imTokenFailed", respondAppBotIMTokenFailed, http.StatusInternalServerError, "err.server.app_bot.im_token_failed"},
113+
{"internal", respondAppBotInternal, http.StatusInternalServerError, "err.server.app_bot.internal"},
114+
{"forbidden", respondAppBotForbidden, http.StatusForbidden, "err.shared.auth.forbidden"},
115+
}
116+
for _, tc := range cases {
117+
t.Run(tc.name, func(t *testing.T) {
118+
r := appBotHelperHarness(tc.probe)
119+
req := httptest.NewRequest(http.MethodGet, "/probe", nil)
120+
rec := httptest.NewRecorder()
121+
r.ServeHTTP(rec, req)
122+
123+
if rec.Code != tc.wantStatus {
124+
t.Fatalf("status = %d, want %d; body=%s", rec.Code, tc.wantStatus, rec.Body.String())
125+
}
126+
var env appBotEnvelope
127+
if err := json.Unmarshal(rec.Body.Bytes(), &env); err != nil {
128+
t.Fatalf("decode envelope: %v; body=%s", err, rec.Body.String())
129+
}
130+
if env.Error.Code != tc.wantCodeID {
131+
t.Fatalf("error.code = %q, want %q", env.Error.Code, tc.wantCodeID)
132+
}
133+
if env.Error.HTTPStatus != tc.wantStatus {
134+
t.Fatalf("error.http_status = %d, want %d", env.Error.HTTPStatus, tc.wantStatus)
135+
}
136+
})
137+
}
138+
}

0 commit comments

Comments
 (0)