From 42a402a5b42c37587a212ef49275e47b7f1fd89f Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 10 Oct 2026 08:32:04 +0000 Subject: [PATCH] fix(apiserver): abort the request on every middleware failure (bug-1949) Cause: ReturnError only writes the response and never aborts the gin chain; RequirePermission (token error) and WhitelistIPMiddleware (invalid client IP) returned after ReturnError without ctx.Abort(), so the route handler still ran after the rejection. Fix: ctx.Abort() after ReturnError at both sites (and, for clarity, on the ValidateHandler body-check path, which validateRequestBody already aborted). RequirePermission now answers a bad token with 401 instead of 500. ReturnError stays non-aborting (documented): handlers such as ControlHost call it from helpers and then test ctx.IsAborted(), so a global abort would change handler flows, and it adds nothing in handlers. Tests: TestRequirePermission_AbortsOnInvalidToken, TestWhitelistIPMiddleware_AbortsOnFailure. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../apiserver/controllers/rbac_abort_test.go | 48 +++++++++++++++++ .../apiserver/controllers/rbac_check.go | 3 +- .../apiserver/controllers/response.go | 3 ++ .../apiserver/middleware/validatehandler.go | 1 + .../apiserver/middleware/whitelist.go | 1 + .../apiserver/middleware/whitelist_test.go | 51 ++++++++++++++++++- 6 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 src/frontend/apiserver/controllers/rbac_abort_test.go diff --git a/src/frontend/apiserver/controllers/rbac_abort_test.go b/src/frontend/apiserver/controllers/rbac_abort_test.go new file mode 100644 index 00000000..1174545a --- /dev/null +++ b/src/frontend/apiserver/controllers/rbac_abort_test.go @@ -0,0 +1,48 @@ +package controllers + +import ( + "apiserver/utils" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "go.uber.org/zap" +) + +// bug-1949: a RequirePermission failure must stop the request before the +// handler runs. +func TestRequirePermission_AbortsOnInvalidToken(t *testing.T) { + resetRBACState() + prev := utils.AuthenticationYamlConfig + utils.AuthenticationYamlConfig = &utils.Authentication{ + Token: utils.TokenConfig{Type: "jwt", SecretKey: "rbac-abort-test-secret", TokenExpireTime: 3600}, + } + utils.Log = zap.NewNop() + t.Cleanup(func() { utils.AuthenticationYamlConfig = prev }) + + gin.SetMode(gin.TestMode) + r := gin.New() + handlerRan := false + r.GET("/protected", RequirePermission("host:view"), func(ctx *gin.Context) { + handlerRan = true + ReturnSuccess(ctx, http.StatusOK, "success", nil) + }) + + for _, header := range []string{"", "Bearer not-a-jwt"} { + handlerRan = false + req := httptest.NewRequest(http.MethodGet, "/protected", nil) + if header != "" { + req.Header.Set("Authorization", header) + } + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + + if handlerRan { + t.Errorf("Authorization %q: handler ran after RequirePermission failed (status %d)", header, w.Code) + } + if w.Code != http.StatusUnauthorized { + t.Errorf("Authorization %q: status = %d, want %d", header, w.Code, http.StatusUnauthorized) + } + } +} diff --git a/src/frontend/apiserver/controllers/rbac_check.go b/src/frontend/apiserver/controllers/rbac_check.go index ecc107e2..091bb6b3 100644 --- a/src/frontend/apiserver/controllers/rbac_check.go +++ b/src/frontend/apiserver/controllers/rbac_check.go @@ -117,7 +117,8 @@ func RequirePermission(perm string) gin.HandlerFunc { token := ctx.GetHeader("Authorization") logonUser, err := GetUsernameFromToken(token) if err != nil { - ReturnError(ctx, common.ERROR_CODE, err.Error(), nil) + ReturnError(ctx, common.ERROR_AUTH_CHECK_TOKEN_CODE, err.Error(), nil) + ctx.Abort() return } logger.Debug("Logon user is ", zap.String("logonUser", logonUser)) diff --git a/src/frontend/apiserver/controllers/response.go b/src/frontend/apiserver/controllers/response.go index a05d579f..9444f6e6 100644 --- a/src/frontend/apiserver/controllers/response.go +++ b/src/frontend/apiserver/controllers/response.go @@ -27,6 +27,9 @@ func ReturnSuccess(ctx *gin.Context, code int, msg interface{}, data interface{} ctx.JSON(200, json) } +// ReturnError writes the error envelope. It does not abort the gin chain: +// a middleware that rejects a request must call ctx.Abort() after it +// (bug-1949); handlers just return. func ReturnError(ctx *gin.Context, code int, msg interface{}, details interface{}) { json := &JsonErrStruct{ Code: code, diff --git a/src/frontend/apiserver/middleware/validatehandler.go b/src/frontend/apiserver/middleware/validatehandler.go index 5720a376..4b61c316 100644 --- a/src/frontend/apiserver/middleware/validatehandler.go +++ b/src/frontend/apiserver/middleware/validatehandler.go @@ -35,6 +35,7 @@ func ValidateHandler() gin.HandlerFunc { if err != nil || !isValidBody { controllers.ReturnError(ctx, common.INVALID_PARAMS_CODE, common.ERROR_READ_REQUEST_BODY_MSG+": "+err.Error(), nil) + ctx.Abort() return } } diff --git a/src/frontend/apiserver/middleware/whitelist.go b/src/frontend/apiserver/middleware/whitelist.go index 63ee1684..c7636f5c 100644 --- a/src/frontend/apiserver/middleware/whitelist.go +++ b/src/frontend/apiserver/middleware/whitelist.go @@ -129,6 +129,7 @@ func WhitelistIPMiddleware(allowedIPs []string) gin.HandlerFunc { if clientIP == nil { logger.Error("invalid client IP", zap.String("clientIP", clientIPStr)) controllers.ReturnError(ctx, common.INVALID_PARAMS_CODE, common.ERROR_INVALID_CLIENT_IP_MSG, nil) + ctx.Abort() return } diff --git a/src/frontend/apiserver/middleware/whitelist_test.go b/src/frontend/apiserver/middleware/whitelist_test.go index afc26613..35cf2ea1 100644 --- a/src/frontend/apiserver/middleware/whitelist_test.go +++ b/src/frontend/apiserver/middleware/whitelist_test.go @@ -1,6 +1,55 @@ package middleware -import "testing" +import ( + "apiserver/utils" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "go.uber.org/zap" +) + +// bug-1949: every IP-whitelist failure must stop the request before the +// handler runs. +func TestWhitelistIPMiddleware_AbortsOnFailure(t *testing.T) { + prevLog := utils.Log + utils.Log = zap.NewNop() + t.Cleanup(func() { utils.Log = prevLog }) + + tests := []struct { + name string + remoteAddr string + wantStatus int + }{ + {"unparsable client IP", "not-an-address", http.StatusBadRequest}, + {"client IP not whitelisted", "192.0.2.10:4000", http.StatusForbidden}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gin.SetMode(gin.TestMode) + r := gin.New() + handlerRan := false + r.Use(WhitelistIPMiddleware([]string{"10.0.0.0/8"})) + r.GET("/ping", func(ctx *gin.Context) { + handlerRan = true + ctx.String(http.StatusOK, "pong") + }) + + req := httptest.NewRequest(http.MethodGet, "/ping", nil) + req.RemoteAddr = tt.remoteAddr + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + + if handlerRan { + t.Errorf("handler ran after the IP whitelist rejected %q (status %d)", tt.remoteAddr, w.Code) + } + if w.Code != tt.wantStatus { + t.Errorf("status = %d, want %d", w.Code, tt.wantStatus) + } + }) + } +} func TestPathMatchesPattern(t *testing.T) { cases := []struct {