From a9aedc843e02d6a4c0899d61fafda382d8718f24 Mon Sep 17 00:00:00 2001 From: viettranx Date: Wed, 25 Mar 2026 10:21:39 +0700 Subject: [PATCH] fix(security): add tenant membership check to tenant-config endpoints Prevent cross-tenant config manipulation by verifying caller has owner/admin role within the target tenant before allowing set/delete operations. Also adds MaxBytesReader, audit trail, and fixes middleware consistency between skills and builtin_tools endpoints. --- cmd/gateway_http_handlers.go | 4 +-- internal/http/builtin_tools.go | 23 ++++++------- internal/http/skills.go | 28 ++++++++-------- internal/http/tenant_auth_helpers.go | 49 ++++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 26 deletions(-) create mode 100644 internal/http/tenant_auth_helpers.go diff --git a/cmd/gateway_http_handlers.go b/cmd/gateway_http_handlers.go index e92d9161..373c0ed6 100644 --- a/cmd/gateway_http_handlers.go +++ b/cmd/gateway_http_handlers.go @@ -33,7 +33,7 @@ func wireHTTP(stores *store.Stores, defaultWorkspace, dataDir, bundledSkillsDir if pgSkills, ok := stores.Skills.(*pg.PGSkillStore); ok { dirs := pgSkills.Dirs() if len(dirs) > 0 { - skillsH = httpapi.NewSkillsHandler(pgSkills, dirs[0], dataDir, bundledSkillsDir, msgBus, stores.SkillTenantCfgs) + skillsH = httpapi.NewSkillsHandler(pgSkills, dirs[0], dataDir, bundledSkillsDir, msgBus, stores.SkillTenantCfgs, stores.Tenants) } } } @@ -72,7 +72,7 @@ func wireHTTP(stores *store.Stores, defaultWorkspace, dataDir, bundledSkillsDir } if stores != nil && stores.BuiltinTools != nil { - builtinToolsH = httpapi.NewBuiltinToolsHandler(stores.BuiltinTools, stores.BuiltinToolTenantCfgs, msgBus) + builtinToolsH = httpapi.NewBuiltinToolsHandler(stores.BuiltinTools, stores.BuiltinToolTenantCfgs, stores.Tenants, msgBus) } if stores != nil && stores.PendingMessages != nil { diff --git a/internal/http/builtin_tools.go b/internal/http/builtin_tools.go index 146efb74..351b0dfb 100644 --- a/internal/http/builtin_tools.go +++ b/internal/http/builtin_tools.go @@ -18,12 +18,13 @@ import ( type BuiltinToolsHandler struct { store store.BuiltinToolStore tenantCfgStore store.BuiltinToolTenantConfigStore + tenantStore store.TenantStore msgBus *bus.MessageBus } // NewBuiltinToolsHandler creates a handler for built-in tool management endpoints. -func NewBuiltinToolsHandler(s store.BuiltinToolStore, tenantCfgs store.BuiltinToolTenantConfigStore, msgBus *bus.MessageBus) *BuiltinToolsHandler { - return &BuiltinToolsHandler{store: s, tenantCfgStore: tenantCfgs, msgBus: msgBus} +func NewBuiltinToolsHandler(s store.BuiltinToolStore, tenantCfgs store.BuiltinToolTenantConfigStore, tenantStore store.TenantStore, msgBus *bus.MessageBus) *BuiltinToolsHandler { + return &BuiltinToolsHandler{store: s, tenantCfgStore: tenantCfgs, tenantStore: tenantStore, msgBus: msgBus} } // RegisterRoutes registers all built-in tool routes on the given mux. @@ -144,17 +145,16 @@ func (h *BuiltinToolsHandler) handleSetTenantConfig(w http.ResponseWriter, r *ht writeJSON(w, http.StatusNotImplemented, map[string]string{"error": "tenant config not available"}) return } - name := r.PathValue("name") - tid := store.TenantIDFromContext(r.Context()) - if tid == uuid.Nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": "tenant context required"}) + if !requireTenantAdmin(w, r, h.tenantStore) { return } + name := r.PathValue("name") + tid := store.TenantIDFromContext(r.Context()) var body struct { Enabled bool `json:"enabled"` } - if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + if err := json.NewDecoder(http.MaxBytesReader(w, r.Body, 1<<10)).Decode(&body); err != nil { writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid request body"}) return } @@ -165,6 +165,7 @@ func (h *BuiltinToolsHandler) handleSetTenantConfig(w http.ResponseWriter, r *ht return } + emitAudit(h.msgBus, r, "builtin_tool.tenant_config.set", "builtin_tool", name) writeJSON(w, http.StatusOK, map[string]string{"status": "updated"}) } @@ -174,17 +175,17 @@ func (h *BuiltinToolsHandler) handleDeleteTenantConfig(w http.ResponseWriter, r writeJSON(w, http.StatusNotImplemented, map[string]string{"error": "tenant config not available"}) return } - name := r.PathValue("name") - tid := store.TenantIDFromContext(r.Context()) - if tid == uuid.Nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": "tenant context required"}) + if !requireTenantAdmin(w, r, h.tenantStore) { return } + name := r.PathValue("name") + tid := store.TenantIDFromContext(r.Context()) if err := h.tenantCfgStore.Delete(r.Context(), tid, name); err != nil { writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) return } + emitAudit(h.msgBus, r, "builtin_tool.tenant_config.deleted", "builtin_tool", name) writeJSON(w, http.StatusOK, map[string]string{"status": "deleted"}) } diff --git a/internal/http/skills.go b/internal/http/skills.go index ee943748..bd0c70ea 100644 --- a/internal/http/skills.go +++ b/internal/http/skills.go @@ -30,11 +30,12 @@ type SkillsHandler struct { bundledDir string // original bundled skills dir (fallback for broken managed copies) msgBus *bus.MessageBus tenantCfgStore store.SkillTenantConfigStore + tenantStore store.TenantStore } // NewSkillsHandler creates a handler for skill management endpoints. -func NewSkillsHandler(skills *pg.PGSkillStore, baseDir, dataDir, bundledDir string, msgBus *bus.MessageBus, tenantCfgStore store.SkillTenantConfigStore) *SkillsHandler { - return &SkillsHandler{skills: skills, baseDir: baseDir, dataDir: dataDir, bundledDir: bundledDir, msgBus: msgBus, tenantCfgStore: tenantCfgStore} +func NewSkillsHandler(skills *pg.PGSkillStore, baseDir, dataDir, bundledDir string, msgBus *bus.MessageBus, tenantCfgStore store.SkillTenantConfigStore, tenantStore store.TenantStore) *SkillsHandler { + return &SkillsHandler{skills: skills, baseDir: baseDir, dataDir: dataDir, bundledDir: bundledDir, msgBus: msgBus, tenantCfgStore: tenantCfgStore, tenantStore: tenantStore} } // tenantSkillsDir returns the skills-store directory scoped to the requesting tenant. @@ -78,8 +79,9 @@ func (h *SkillsHandler) RegisterRoutes(mux *http.ServeMux) { mux.HandleFunc("POST /v1/skills/install-dep", h.adminMiddleware(h.handleInstallDep)) mux.HandleFunc("GET /v1/skills/runtimes", h.adminMiddleware(h.handleRuntimes)) mux.HandleFunc("POST /v1/skills/{id}/toggle", h.adminMiddleware(h.handleToggle)) - mux.HandleFunc("PUT /v1/skills/{id}/tenant-config", h.adminMiddleware(h.handleSetTenantConfig)) - mux.HandleFunc("DELETE /v1/skills/{id}/tenant-config", h.adminMiddleware(h.handleDeleteTenantConfig)) + // Per-tenant overrides: tenant-level admin check inside handler (not system admin). + mux.HandleFunc("PUT /v1/skills/{id}/tenant-config", h.authMiddleware(h.handleSetTenantConfig)) + mux.HandleFunc("DELETE /v1/skills/{id}/tenant-config", h.authMiddleware(h.handleDeleteTenantConfig)) } func (h *SkillsHandler) authMiddleware(next http.HandlerFunc) http.HandlerFunc { @@ -525,12 +527,11 @@ func (h *SkillsHandler) handleSetTenantConfig(w http.ResponseWriter, r *http.Req writeJSON(w, http.StatusNotImplemented, map[string]string{"error": "tenant config not available"}) return } - locale := store.LocaleFromContext(r.Context()) - tid := store.TenantIDFromContext(r.Context()) - if tid == uuid.Nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": "tenant context required"}) + if !requireTenantAdmin(w, r, h.tenantStore) { return } + locale := store.LocaleFromContext(r.Context()) + tid := store.TenantIDFromContext(r.Context()) idStr := r.PathValue("id") skillID, err := uuid.Parse(idStr) @@ -542,7 +543,7 @@ func (h *SkillsHandler) handleSetTenantConfig(w http.ResponseWriter, r *http.Req var body struct { Enabled bool `json:"enabled"` } - if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + if err := json.NewDecoder(http.MaxBytesReader(w, r.Body, 1<<10)).Decode(&body); err != nil { writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidJSON)}) return } @@ -553,6 +554,7 @@ func (h *SkillsHandler) handleSetTenantConfig(w http.ResponseWriter, r *http.Req return } + emitAudit(h.msgBus, r, "skill.tenant_config.set", "skill", idStr) writeJSON(w, http.StatusOK, map[string]string{"status": "updated"}) } @@ -562,12 +564,11 @@ func (h *SkillsHandler) handleDeleteTenantConfig(w http.ResponseWriter, r *http. writeJSON(w, http.StatusNotImplemented, map[string]string{"error": "tenant config not available"}) return } - locale := store.LocaleFromContext(r.Context()) - tid := store.TenantIDFromContext(r.Context()) - if tid == uuid.Nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": "tenant context required"}) + if !requireTenantAdmin(w, r, h.tenantStore) { return } + locale := store.LocaleFromContext(r.Context()) + tid := store.TenantIDFromContext(r.Context()) idStr := r.PathValue("id") skillID, err := uuid.Parse(idStr) @@ -582,5 +583,6 @@ func (h *SkillsHandler) handleDeleteTenantConfig(w http.ResponseWriter, r *http. return } + emitAudit(h.msgBus, r, "skill.tenant_config.deleted", "skill", idStr) writeJSON(w, http.StatusOK, map[string]string{"status": "deleted"}) } diff --git a/internal/http/tenant_auth_helpers.go b/internal/http/tenant_auth_helpers.go new file mode 100644 index 00000000..785c96c9 --- /dev/null +++ b/internal/http/tenant_auth_helpers.go @@ -0,0 +1,49 @@ +package http + +import ( + "net/http" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/i18n" + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +// requireTenantAdmin verifies the caller has owner or admin role within the +// specified tenant. System-wide owners (IsOwnerRole) bypass the check. +// Returns true if authorized, false if an error response was written. +func requireTenantAdmin(w http.ResponseWriter, r *http.Request, ts store.TenantStore) bool { + ctx := r.Context() + + // System-wide owner bypasses tenant membership check. + if store.IsOwnerRole(ctx) { + return true + } + + if ts == nil { + writeJSON(w, http.StatusNotImplemented, map[string]string{"error": "tenant store not available"}) + return false + } + + tid := store.TenantIDFromContext(ctx) + if tid == uuid.Nil { + locale := store.LocaleFromContext(ctx) + writeJSON(w, http.StatusBadRequest, map[string]string{ + "error": i18n.T(locale, i18n.MsgPermissionDenied, "tenant config"), + }) + return false + } + + userID := store.UserIDFromContext(ctx) + // GetUserRole returns ("", nil) when the user has no membership in this tenant, + // which correctly falls through to the role check below (denied). + role, err := ts.GetUserRole(ctx, tid, userID) + if err != nil || (role != store.TenantRoleOwner && role != store.TenantRoleAdmin) { + locale := store.LocaleFromContext(ctx) + writeJSON(w, http.StatusForbidden, map[string]string{ + "error": i18n.T(locale, i18n.MsgPermissionDenied, "tenant config"), + }) + return false + } + return true +}