From 0240a36b05127013c002ebfcee1cd280141a7cee Mon Sep 17 00:00:00 2001 From: Bruno Clermont Date: Wed, 24 Jun 2026 22:53:28 -0400 Subject: [PATCH] fix(hooks): allow system tenant to create tenant/agent scoped hooks (#1241) (#1280) Co-authored-by: Bruno Clermont --- internal/gateway/methods/hooks.go | 6 +- internal/hooks/config.go | 26 +++-- internal/hooks/config_test.go | 98 ++++++++++++++----- tests/integration/hooks_ws_end_to_end_test.go | 6 +- 4 files changed, 98 insertions(+), 38 deletions(-) diff --git a/internal/gateway/methods/hooks.go b/internal/gateway/methods/hooks.go index 2fe59783..2bacfb25 100644 --- a/internal/gateway/methods/hooks.go +++ b/internal/gateway/methods/hooks.go @@ -153,7 +153,7 @@ func (m *HookMethods) handleCreate(ctx context.Context, client *gateway.Client, cfg.TenantID = hooks.SentinelTenantID } - if err := cfg.Validate(m.edition); err != nil { + if err := cfg.Validate(m.edition, store.IsMasterScope(ctx)); err != nil { client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, err.Error())) return } @@ -215,7 +215,7 @@ func (m *HookMethods) handleUpdate(ctx context.Context, client *gateway.Client, return } merged := applyHookPatch(*current, params.Updates) - if verr := merged.Validate(m.edition); verr != nil { + if verr := merged.Validate(m.edition, store.IsMasterScope(ctx)); verr != nil { client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, verr.Error())) return } @@ -377,7 +377,7 @@ func (m *HookMethods) handleTest(ctx context.Context, client *gateway.Client, re if cfg.Scope == hooks.ScopeGlobal { cfg.TenantID = hooks.SentinelTenantID } - if err := cfg.Validate(m.edition); err != nil { + if err := cfg.Validate(m.edition, store.IsMasterScope(ctx)); err != nil { client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, err.Error())) return } diff --git a/internal/hooks/config.go b/internal/hooks/config.go index 64a6a8b1..0d1eaa3e 100644 --- a/internal/hooks/config.go +++ b/internal/hooks/config.go @@ -42,17 +42,23 @@ var knownEvents = map[HookEvent]struct{}{ // the DB or the dispatcher. Side effects (on success): TimeoutMS, OnTimeout, // Version, and Source may be populated with their defaults. // +// isMasterScope must be true when the caller is master/owner scope (e.g. +// store.IsMasterScope(ctx)). Master callers may legitimately create +// ScopeTenant and ScopeAgent hooks whose TenantID equals MasterTenantID +// (the system tenant). Regular callers cannot use MasterTenantID / the +// sentinel UUID for tenant-or-agent-scoped hooks. +// // Validation order matters — cheap checks run first so we fail fast: // 1. Event enum — zero-cost map lookup. // 2. Scope / tenant / agent invariants — no I/O. // 3. Edition gate — pure-function policy. // 4. Matcher + CEL compile — most expensive; done last. -func (h *HookConfig) Validate(ed edition.Edition) error { +func (h *HookConfig) Validate(ed edition.Edition, isMasterScope bool) error { if _, ok := knownEvents[h.Event]; !ok { return fmt.Errorf("hook: unknown event %q", h.Event) } - if err := h.validateScope(); err != nil { + if err := h.validateScope(isMasterScope); err != nil { return err } @@ -80,20 +86,26 @@ func (h *HookConfig) Validate(ed edition.Edition) error { // validateScope enforces (scope, tenant_id, agent_id) invariants: // - global → tenant_id MUST be the sentinel UUID. -// - tenant → tenant_id MUST be non-sentinel. -// - agent → tenant_id MUST be non-sentinel AND agent_id non-nil. -func (h *HookConfig) validateScope() error { +// - tenant → tenant_id MUST be non-sentinel (or MasterTenantID when isMasterScope). +// - agent → same tenant_id rule AND at least one agent_id non-nil. +// +// isMasterScope relaxes the sentinel check for tenant/agent scope: master/owner +// callers creating hooks for the system tenant legitimately supply MasterTenantID +// (which equals SentinelTenantID), so we must not reject them. +func (h *HookConfig) validateScope(isMasterScope bool) error { switch h.Scope { case ScopeGlobal: if h.TenantID != SentinelTenantID { return fmt.Errorf("hook: global scope requires sentinel tenant_id, got %s", h.TenantID) } case ScopeTenant: - if h.TenantID == SentinelTenantID || h.TenantID == uuid.Nil { + // Master-scope callers may use MasterTenantID (== SentinelTenantID) because + // they are scoping the hook to the real system tenant row. + if h.TenantID == uuid.Nil || (!isMasterScope && h.TenantID == SentinelTenantID) { return fmt.Errorf("hook: tenant scope requires a real tenant_id") } case ScopeAgent: - if h.TenantID == SentinelTenantID || h.TenantID == uuid.Nil { + if h.TenantID == uuid.Nil || (!isMasterScope && h.TenantID == SentinelTenantID) { return fmt.Errorf("hook: agent scope requires a real tenant_id") } if len(h.AgentIDs) == 0 { diff --git a/internal/hooks/config_test.go b/internal/hooks/config_test.go index d364efc1..74bad5a0 100644 --- a/internal/hooks/config_test.go +++ b/internal/hooks/config_test.go @@ -33,7 +33,7 @@ func baseValidCommandHook() hooks.HookConfig { func TestValidate_AcceptsValidCommandHook(t *testing.T) { h := baseValidCommandHook() - if err := h.Validate(edition.Lite); err != nil { + if err := h.Validate(edition.Lite, false); err != nil { t.Fatalf("expected nil error; got %v", err) } } @@ -44,7 +44,7 @@ func TestValidate_AcceptsValidHTTPHook(t *testing.T) { h.Config = map[string]any{"url": "https://example.com/hook"} h.Matcher = "" // http hook without matcher is allowed (matches all tools) h.IfExpr = "" - if err := h.Validate(edition.Standard); err != nil { + if err := h.Validate(edition.Standard, false); err != nil { t.Fatalf("expected nil error; got %v", err) } } @@ -55,7 +55,7 @@ func TestValidate_AcceptsValidPromptHook(t *testing.T) { h.Config = map[string]any{"prompt_template": "Review this action"} h.Matcher = "^Write$" h.IfExpr = "" - if err := h.Validate(edition.Standard); err != nil { + if err := h.Validate(edition.Standard, false); err != nil { t.Fatalf("expected nil error; got %v", err) } } @@ -68,7 +68,7 @@ func TestValidate_RejectsPromptWithoutFilter(t *testing.T) { h.Config = map[string]any{"template": "x"} h.Matcher = "" h.IfExpr = "" - err := h.Validate(edition.Standard) + err := h.Validate(edition.Standard, false) if err == nil { t.Fatal("expected validation error for prompt without matcher/if_expr") } @@ -80,7 +80,7 @@ func TestValidate_RejectsPromptWithoutFilter(t *testing.T) { func TestValidate_RejectsInvalidRegex(t *testing.T) { h := baseValidCommandHook() h.Matcher = "[unclosed" - err := h.Validate(edition.Lite) + err := h.Validate(edition.Lite, false) if err == nil { t.Fatal("expected error for malformed regex") } @@ -92,7 +92,7 @@ func TestValidate_RejectsInvalidRegex(t *testing.T) { func TestValidate_RejectsMalformedCEL(t *testing.T) { h := baseValidCommandHook() h.IfExpr = `tool_name ===== "bad"` // CEL syntax error - err := h.Validate(edition.Lite) + err := h.Validate(edition.Lite, false) if err == nil { t.Fatal("expected error for malformed CEL") } @@ -107,7 +107,7 @@ func TestValidate_RejectsCommandOnStandardTenant(t *testing.T) { h := baseValidCommandHook() h.Scope = hooks.ScopeTenant h.TenantID = uuid.New() - err := h.Validate(edition.Standard) + err := h.Validate(edition.Standard, false) if err == nil { t.Fatal("expected error for command on Standard+tenant") } @@ -122,7 +122,7 @@ func TestValidate_RejectsCommandOnStandardAgent(t *testing.T) { h.TenantID = uuid.New() agentID := uuid.New() h.AgentID = &agentID - err := h.Validate(edition.Standard) + err := h.Validate(edition.Standard, false) if err == nil { t.Fatal("expected error for command on Standard+agent") } @@ -139,7 +139,7 @@ func TestValidate_AcceptsCommandOnLiteAnyScope(t *testing.T) { agentID := uuid.New() h.AgentID = &agentID } - if err := h.Validate(edition.Lite); err != nil { + if err := h.Validate(edition.Lite, false); err != nil { t.Errorf("Lite+command+%s should be valid; got %v", scope, err) } } @@ -150,7 +150,7 @@ func TestValidate_AcceptsCommandOnLiteAnyScope(t *testing.T) { func TestValidate_AppliesTimeoutDefault(t *testing.T) { h := baseValidCommandHook() h.TimeoutMS = 0 - if err := h.Validate(edition.Lite); err != nil { + if err := h.Validate(edition.Lite, false); err != nil { t.Fatalf("Validate: %v", err) } if h.TimeoutMS != hooks.DefaultTimeoutMS { @@ -164,7 +164,7 @@ func TestValidate_AppliesOnTimeoutDefault(t *testing.T) { h := baseValidCommandHook() h.OnTimeout = "" h.Event = hooks.EventPreToolUse // blocking - if err := h.Validate(edition.Lite); err != nil { + if err := h.Validate(edition.Lite, false); err != nil { t.Fatalf("Validate: %v", err) } if h.OnTimeout != hooks.DecisionBlock { @@ -174,7 +174,7 @@ func TestValidate_AppliesOnTimeoutDefault(t *testing.T) { h2 := baseValidCommandHook() h2.OnTimeout = "" h2.Event = hooks.EventPostToolUse // non-blocking - if err := h2.Validate(edition.Lite); err != nil { + if err := h2.Validate(edition.Lite, false); err != nil { t.Fatalf("Validate: %v", err) } if h2.OnTimeout != hooks.DecisionAllow { @@ -186,7 +186,7 @@ func TestValidate_AppliesOnTimeoutDefault(t *testing.T) { func TestValidate_AppliesVersionDefault(t *testing.T) { h := baseValidCommandHook() h.Version = 0 - if err := h.Validate(edition.Lite); err != nil { + if err := h.Validate(edition.Lite, false); err != nil { t.Fatalf("Validate: %v", err) } if h.Version != 1 { @@ -199,7 +199,7 @@ func TestValidate_AppliesVersionDefault(t *testing.T) { func TestValidate_AppliesSourceDefault(t *testing.T) { h := baseValidCommandHook() h.Source = "" - if err := h.Validate(edition.Lite); err != nil { + if err := h.Validate(edition.Lite, false); err != nil { t.Fatalf("Validate: %v", err) } if h.Source != "ui" { @@ -210,7 +210,7 @@ func TestValidate_AppliesSourceDefault(t *testing.T) { func TestValidate_RejectsUnknownEvent(t *testing.T) { h := baseValidCommandHook() h.Event = hooks.HookEvent("webhook_magic") - err := h.Validate(edition.Lite) + err := h.Validate(edition.Lite, false) if err == nil { t.Fatal("expected error for unknown event") } @@ -220,7 +220,7 @@ func TestValidate_RejectsGlobalScopeWithNonSentinelTenant(t *testing.T) { h := baseValidCommandHook() h.Scope = hooks.ScopeGlobal h.TenantID = uuid.New() // must be sentinel for global - err := h.Validate(edition.Lite) + err := h.Validate(edition.Lite, false) if err == nil { t.Fatal("expected error for global scope with non-sentinel tenant") } @@ -231,7 +231,7 @@ func TestValidate_RejectsAgentScopeWithoutAgentID(t *testing.T) { h.Scope = hooks.ScopeAgent h.TenantID = uuid.New() h.AgentID = nil - err := h.Validate(edition.Lite) + err := h.Validate(edition.Lite, false) if err == nil { t.Fatal("expected error for agent scope without agent_id") } @@ -240,7 +240,7 @@ func TestValidate_RejectsAgentScopeWithoutAgentID(t *testing.T) { func TestValidate_RejectsNegativeTimeout(t *testing.T) { h := baseValidCommandHook() h.TimeoutMS = -1 - err := h.Validate(edition.Lite) + err := h.Validate(edition.Lite, false) if err == nil { t.Fatal("expected error for negative timeout") } @@ -249,7 +249,7 @@ func TestValidate_RejectsNegativeTimeout(t *testing.T) { func TestValidate_CapsTimeoutToMax(t *testing.T) { h := baseValidCommandHook() h.TimeoutMS = 60 * 1000 // 60s — exceeds chain wall-time budget - err := h.Validate(edition.Lite) + err := h.Validate(edition.Lite, false) if err == nil { t.Fatal("expected error for timeout > MaxTimeoutMS") } @@ -266,7 +266,7 @@ func baseValidScriptHook() hooks.HookConfig { func TestValidate_AcceptsValidScriptHook(t *testing.T) { h := baseValidScriptHook() - if err := h.Validate(edition.Standard); err != nil { + if err := h.Validate(edition.Standard, false); err != nil { t.Fatalf("expected nil error; got %v", err) } } @@ -274,7 +274,7 @@ func TestValidate_AcceptsValidScriptHook(t *testing.T) { func TestValidate_RejectsEmptyScriptSource(t *testing.T) { h := baseValidScriptHook() h.Config = map[string]any{"source": ""} - err := h.Validate(edition.Standard) + err := h.Validate(edition.Standard, false) if err == nil || !strings.Contains(err.Error(), "non-empty") { t.Fatalf("expected non-empty source error; got %v", err) } @@ -284,7 +284,7 @@ func TestValidate_RejectsOversizedScriptSource(t *testing.T) { h := baseValidScriptHook() big := strings.Repeat("// pad\n", 6000) // >32 KiB h.Config = map[string]any{"source": big} - err := h.Validate(edition.Standard) + err := h.Validate(edition.Standard, false) if err == nil || !strings.Contains(err.Error(), "exceeds") { t.Fatalf("expected exceeds-bytes error; got %v", err) } @@ -294,7 +294,7 @@ func TestValidate_RejectsScriptCompileError(t *testing.T) { h := baseValidScriptHook() // Missing closing brace → parser error with line:col. h.Config = map[string]any{"source": `function handle(e) { return {`} - err := h.Validate(edition.Standard) + err := h.Validate(edition.Standard, false) if err == nil || !strings.Contains(err.Error(), "compile error") { t.Fatalf("expected compile error; got %v", err) } @@ -303,7 +303,7 @@ func TestValidate_RejectsScriptCompileError(t *testing.T) { func TestValidate_RejectsOnTimeoutAsk(t *testing.T) { h := baseValidScriptHook() h.OnTimeout = hooks.DecisionAsk - err := h.Validate(edition.Standard) + err := h.Validate(edition.Standard, false) if err == nil || !strings.Contains(err.Error(), "on_timeout") { t.Fatalf("expected on_timeout rejection; got %v", err) } @@ -312,8 +312,56 @@ func TestValidate_RejectsOnTimeoutAsk(t *testing.T) { func TestValidate_RejectsOnTimeoutDefer(t *testing.T) { h := baseValidScriptHook() h.OnTimeout = hooks.DecisionDefer - err := h.Validate(edition.Standard) + err := h.Validate(edition.Standard, false) if err == nil || !strings.Contains(err.Error(), "on_timeout") { t.Fatalf("expected on_timeout rejection; got %v", err) } } + +// TestValidate_MasterScopeAllowsSystemTenantHooks verifies that master/owner +// callers can create ScopeTenant and ScopeAgent hooks whose TenantID equals +// MasterTenantID (the real system tenant row). Before this fix, validateScope +// rejected such hooks because SentinelTenantID == MasterTenantID. +func TestValidate_MasterScopeAllowsSystemTenantHooks(t *testing.T) { + systemTenantID := hooks.SentinelTenantID // same UUID as MasterTenantID + + t.Run("ScopeTenant with MasterTenantID as master", func(t *testing.T) { + h := baseValidCommandHook() + h.Scope = hooks.ScopeTenant + h.TenantID = systemTenantID + if err := h.Validate(edition.Lite, true); err != nil { + t.Errorf("master scope should accept ScopeTenant+MasterTenantID; got %v", err) + } + }) + + t.Run("ScopeAgent with MasterTenantID as master", func(t *testing.T) { + h := baseValidCommandHook() + h.Scope = hooks.ScopeAgent + h.TenantID = systemTenantID + agentID := uuid.New() + h.AgentID = &agentID + if err := h.Validate(edition.Lite, true); err != nil { + t.Errorf("master scope should accept ScopeAgent+MasterTenantID; got %v", err) + } + }) + + t.Run("ScopeTenant with MasterTenantID as non-master is rejected", func(t *testing.T) { + h := baseValidCommandHook() + h.Scope = hooks.ScopeTenant + h.TenantID = systemTenantID + if err := h.Validate(edition.Lite, false); err == nil { + t.Error("non-master scope should reject ScopeTenant+MasterTenantID") + } + }) + + t.Run("ScopeAgent with uuid.Nil is always rejected", func(t *testing.T) { + h := baseValidCommandHook() + h.Scope = hooks.ScopeAgent + h.TenantID = uuid.Nil + agentID := uuid.New() + h.AgentID = &agentID + if err := h.Validate(edition.Lite, true); err == nil { + t.Error("uuid.Nil tenant_id should always be rejected for agent scope") + } + }) +} diff --git a/tests/integration/hooks_ws_end_to_end_test.go b/tests/integration/hooks_ws_end_to_end_test.go index de771c96..97633c72 100644 --- a/tests/integration/hooks_ws_end_to_end_test.go +++ b/tests/integration/hooks_ws_end_to_end_test.go @@ -87,7 +87,7 @@ func TestHooksF4_TenantAdminCreatesScriptHook(t *testing.T) { cfg := hookCfgScript(tenantA, hooks.EventUserPromptSubmit, "ui", `function handle(e) { return {decision: "allow", reason: "ok"}; }`) - if err := cfg.Validate(edition.Standard); err != nil { + if err := cfg.Validate(edition.Standard, false); err != nil { t.Fatalf("Validate: %v", err) } id, err := hs.Create(tenantCtx(tenantA), cfg) @@ -144,7 +144,7 @@ func TestHooksF5_CommandHandlerRejectedOnStandard(t *testing.T) { Enabled: true, Source: "ui", } - err := cfg.Validate(edition.Standard) + err := cfg.Validate(edition.Standard, false) if err == nil { t.Fatal("Standard Validate accepted command handler; want rejection") } @@ -153,7 +153,7 @@ func TestHooksF5_CommandHandlerRejectedOnStandard(t *testing.T) { } // Sanity: Lite accepts the same config (regression guard). - if err := cfg.Validate(edition.Lite); err != nil { + if err := cfg.Validate(edition.Lite, false); err != nil { t.Errorf("Lite Validate rejected command handler: %v", err) } }