From d720132c8c7088d50358d052685b2fa68bbe27ac Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Sat, 10 Oct 2026 10:21:40 +0000 Subject: [PATCH] fix(apiserver): stop ControlHost after validateHost rejects the request (bug-1961) Cause: validateHost (controllers/hosts.go) wrote a 403 via ReturnError without aborting, so ControlHost's ctx.IsAborted() check never fired and it went on to write a second (202) response; the permission was also checked once per host and not at all for an empty host list. Fix: validateHost writes no response; it checks host:control once up front and returns an error, on which ControlHost writes a single 403 and returns. For "all", the host list is resolved before the one validateHost call. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/frontend/apiserver/controllers/hosts.go | 43 ++++++------ .../controllers/hosts_handler_test.go | 68 +++++++++++++++++++ .../apiserver/controllers/integration_test.go | 5 +- 3 files changed, 94 insertions(+), 22 deletions(-) create mode 100644 src/frontend/apiserver/controllers/hosts_handler_test.go diff --git a/src/frontend/apiserver/controllers/hosts.go b/src/frontend/apiserver/controllers/hosts.go index 152014f2..a15975ab 100644 --- a/src/frontend/apiserver/controllers/hosts.go +++ b/src/frontend/apiserver/controllers/hosts.go @@ -422,17 +422,17 @@ func (h *HostHandler) ControlHost(ctx *gin.Context) { successHosts := []map[string]interface{}{} failedHosts := []map[string]interface{}{} - validatedSuccessHosts, validatedFailedHosts := validateHost(ctx, actionType, req.Hostname) - if ctx.IsAborted() { + hostnames := req.Hostname + if allFlag { + hostnames = store.GetAllHostNames() + } + validatedSuccessHosts, validatedFailedHosts, err := validateHost(ctx, actionType, hostnames) + if err != nil { + // validateHost rejected the request: answer once and stop (bug-1961). ReturnError(ctx, common.ERROR_AUTH_CODE, common.ERROR_ACCESS_FORBIDDEN_MSG, nil) return } - if allFlag { - hostnames := store.GetAllHostNames() - validatedSuccessHosts, validatedFailedHosts = validateHost(ctx, actionType, hostnames) - } - for _, hostname := range validatedSuccessHosts { successHosts = append(successHosts, map[string]interface{}{ "host_name": hostname, @@ -471,16 +471,27 @@ func (h *HostHandler) ControlHost(ctx *gin.Context) { return } -// validateHost validates the hostname in the request -func validateHost(ctx *gin.Context, control int, hostnames []string) (successHosts []string, failedHosts map[string]string) { +// validateHost validates the hostnames in the request. It writes no +// response: a non-nil error means the caller lacks host:control (or its +// token is invalid) and the caller must reject the whole request (bug-1961). +func validateHost(ctx *gin.Context, control int, hostnames []string) (successHosts []string, failedHosts map[string]string, err error) { logger.Debug("Validating hostnames: %v", zap.Any("hostnames", hostnames)) // get logon user token := ctx.GetHeader("Authorization") logonUser, err := GetUsernameFromToken(token) if err != nil { - ReturnError(ctx, common.ERROR_CODE, err.Error(), nil) - return + logger.Error(err.Error()) + return nil, nil, err + } + + hostPermission, err := GetRequireResourcePermission(logonUser, "host:control") + if err != nil { + logger.Error(err.Error(), zap.String("user", logonUser)) + return nil, nil, err + } + if hostPermission.DomainType == models.DomainTypeHostGroup { + // TODO: check hostgroup values } successHosts = []string{} @@ -497,16 +508,6 @@ func validateHost(ctx *gin.Context, control int, hostnames []string) (successHos continue } - hostPermission, err := GetRequireResourcePermission(logonUser, "host:control") - if err != nil { - logger.Error(err.Error()) - ReturnError(ctx, common.ERROR_AUTH_CODE, common.ERROR_ACCESS_FORBIDDEN_MSG, nil) - return - } - if hostPermission.DomainType == models.DomainTypeHostGroup { - // TODO: check hostgroup values - } - if store.Hosts.Has(hostname) { logger.Debug("Found hostname in store", zap.String("hostname", hostname)) successHosts = append(successHosts, hostname) diff --git a/src/frontend/apiserver/controllers/hosts_handler_test.go b/src/frontend/apiserver/controllers/hosts_handler_test.go new file mode 100644 index 00000000..8a2e8f3c --- /dev/null +++ b/src/frontend/apiserver/controllers/hosts_handler_test.go @@ -0,0 +1,68 @@ +//go:build !cgo + +package controllers + +import ( + "encoding/json" + "io" + "net/http" + "strings" + "testing" +) + +// RBAC fixture for bug-1961: viewer has no host:control grant at all. +const hostControlRBAC = ` +permissions: + - host:control + - host:view +roles: + host_viewer: + permissions: + - action: host:view + domain: + - type: all +users: + viewer: + roles: + - name: host_viewer +` + +// countJSONValues returns how many JSON documents body holds back to back: +// a handler that writes a second response after an error shows up as 2. +func countJSONValues(t *testing.T, body string) int { + t.Helper() + dec := json.NewDecoder(strings.NewReader(body)) + n := 0 + for { + var v interface{} + if err := dec.Decode(&v); err == io.EOF { + return n + } else if err != nil { + t.Fatalf("decoding response body %q: %v", body, err) + } + n++ + } +} + +// bug-1961: when validateHost rejects the request (no host:control grant), +// ControlHost must stop with a single 403 response. The route is mounted +// without RequirePermission so the handler's own check is what is tested. +func TestControlHost_RejectedWritesSingleResponse(t *testing.T) { + for _, hosts := range []string{`["node01"]`, `["all"]`} { + t.Run(hosts, func(t *testing.T) { + setupRBACTest(t, hostControlRBAC) + r := newTestRouter() + h := &HostHandler{} + r.POST("/xce/v1/hosts/control", h.ControlHost) + + w := doRoleRequest(r, http.MethodPost, "/xce/v1/hosts/control", "viewer", + `{"action":"close","host_name":`+hosts+`}`) + if n := countJSONValues(t, w.Body.String()); n != 1 { + t.Errorf("handler wrote %d responses, want 1: %s", n, w.Body.String()) + } + if w.Code != http.StatusForbidden { + t.Errorf("status = %d, want 403 (%s)", w.Code, w.Body.String()) + } + }) + } +} diff --git a/src/frontend/apiserver/controllers/integration_test.go b/src/frontend/apiserver/controllers/integration_test.go index 1d965315..05cac6e9 100644 --- a/src/frontend/apiserver/controllers/integration_test.go +++ b/src/frontend/apiserver/controllers/integration_test.go @@ -189,7 +189,10 @@ users: r := gin.New() r.POST("/test", func(ctx *gin.Context) { - successHosts, failedHosts := validateHost(ctx, 1, []string{"node01", "nonexistent"}) + successHosts, failedHosts, err := validateHost(ctx, 1, []string{"node01", "nonexistent"}) + if err != nil { + t.Fatalf("validateHost: %v", err) + } foundNode01 := false for _, h := range successHosts { if h == "node01" {