From f3d6c99dda929d0c18347c419d2b889b383fd8a9 Mon Sep 17 00:00:00 2001 From: viettranx Date: Thu, 16 Apr 2026 07:33:33 +0700 Subject: [PATCH] fix(hooks): close source-tier forge + i18n + concurrency hardening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code-review findings from Wave 1 audit (commits 097776c5..f76a681e). Critical (security): - C1 internal/gateway/methods/hooks.go: parseHookConfigParams strips caller-supplied source/id/created_by/version. Without this a tenant admin could POST {"source":"builtin"} and escalate their UI hook into the dispatcher's builtin capability tier (which is allowed to mutate event input). Default Source resolves to "ui" via Validate. - C2 internal/gateway/methods/hooks.go: handleUpdate also strips source + created_by from the patch map. Same forge surface via PATCH instead of POST. High: - H1 internal/gateway/methods/hooks.go: Update + Delete handlers wrap hooks.ErrBuiltinReadOnly into the i18n key MsgHookBuiltinReadOnly so users get a localized "builtin hooks are read-only" message instead of the raw English sentinel string. - H3 internal/hooks/handlers/script.go: bound tenantSems map with opportunistic sweep — once map crosses 64 entries, idle slots (no in-flight grants AND last-used > 1h ago) are reclaimed under the same lock the acquire path already takes. Goroutine-free; common-path cost unchanged. Medium: - M1 internal/hooks/dispatcher.go: builtinAllowlistLookup uses atomic.Pointer instead of plain package var. Fixes the parallel-test race where one test installs a lookup while another reads. - M3 ui/web/index.html: drop http: https: from CSP connect-src — keep 'self' + ws:/wss: only. Same-origin XHR/WS still works; cross-origin HTTP is no longer wildcard-permitted. Tests: - internal/gateway/methods/hooks_source_strip_test.go pins the C1+C2 strip behavior at the parse layer with a regression case that posts every forge field and asserts each is zeroed. --- internal/gateway/methods/hooks.go | 25 ++++++++++ .../methods/hooks_source_strip_test.go | 47 +++++++++++++++++++ internal/hooks/dispatcher.go | 19 ++++++-- internal/hooks/handlers/script.go | 27 +++++++++++ ui/web/index.html | 7 +-- 5 files changed, 117 insertions(+), 8 deletions(-) create mode 100644 internal/gateway/methods/hooks_source_strip_test.go diff --git a/internal/gateway/methods/hooks.go b/internal/gateway/methods/hooks.go index 5622964c..4a3f28c2 100644 --- a/internal/gateway/methods/hooks.go +++ b/internal/gateway/methods/hooks.go @@ -195,9 +195,15 @@ func (m *HookMethods) handleUpdate(ctx context.Context, client *gateway.Client, } // Strip protected columns before any validation so callers cannot bypass // the scope/edition/matcher/timeout gate by sneaking them into updates. + // `source` strip closes the C2 forge: without it a tenant admin could + // PATCH {"source":"builtin"} on an existing UI row to escalate it into a + // builtin-tier hook (which the dispatcher allows to mutate event input). + // `created_by` strip prevents lying about provenance. delete(params.Updates, "id") delete(params.Updates, "tenant_id") delete(params.Updates, "version") + delete(params.Updates, "source") + delete(params.Updates, "created_by") // Re-validate the merged config: fetch current → apply patch → Validate. // Without this, admin-role callers could bypass edition gate, timeout @@ -215,6 +221,11 @@ func (m *HookMethods) handleUpdate(ctx context.Context, client *gateway.Client, } if err := m.store.Update(ctx, id, params.Updates); err != nil { + if errors.Is(err, hooks.ErrBuiltinReadOnly) { + client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, + i18n.T(locale, i18n.MsgHookBuiltinReadOnly))) + return + } client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, i18n.T(locale, i18n.MsgFailedToUpdate, "hook", err.Error()))) return @@ -285,6 +296,11 @@ func (m *HookMethods) handleDelete(ctx context.Context, client *gateway.Client, return } if err := m.store.Delete(ctx, id); err != nil { + if errors.Is(err, hooks.ErrBuiltinReadOnly) { + client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, + i18n.T(locale, i18n.MsgHookBuiltinReadOnly))) + return + } client.SendResponse(protocol.NewErrorResponse(req.ID, protocol.ErrInvalidRequest, i18n.T(locale, i18n.MsgFailedToDelete, "hook", err.Error()))) return @@ -393,6 +409,15 @@ func parseHookConfigParams(raw json.RawMessage) (*hooks.HookConfig, error) { if cfg.HandlerType == "" || cfg.Event == "" || cfg.Scope == "" { return nil, errors.New("handler_type, event, and scope are required") } + // Strip caller-controlled identity / provenance fields. The dispatcher's + // source-tier gate trusts `Source == "builtin"` to permit input mutation; + // allowing a tenant admin to forge that here would let any script bypass + // the readonly capability tier and rewrite event input. ID stripping keeps + // the H9 cfg.ID-honor path (Phase 03) restricted to internal seeders only. + cfg.Source = "" + cfg.ID = uuid.Nil + cfg.CreatedBy = nil + cfg.Version = 0 return &cfg, nil } diff --git a/internal/gateway/methods/hooks_source_strip_test.go b/internal/gateway/methods/hooks_source_strip_test.go new file mode 100644 index 00000000..50cb77b8 --- /dev/null +++ b/internal/gateway/methods/hooks_source_strip_test.go @@ -0,0 +1,47 @@ +package methods + +import ( + "encoding/json" + "testing" + + "github.com/google/uuid" +) + +// Regression tests for code-review findings C1 + C2 (260415): +// the WS RPC must NOT trust caller-supplied `source`, `id`, `created_by`, +// or `version` when creating a hook. Without this strip, a tenant admin can +// POST {"source":"builtin"} and escalate their UI hook into the builtin +// capability tier (which the dispatcher allows to mutate event input). + +func TestParseHookConfigParams_StripsForgeFields(t *testing.T) { + caller := []byte(`{ + "event": "pre_tool_use", + "scope": "tenant", + "handler_type": "http", + "source": "builtin", + "id": "11111111-2222-3333-4444-555555555555", + "created_by": "99999999-9999-9999-9999-999999999999", + "version": 42 + }`) + + cfg, err := parseHookConfigParams(json.RawMessage(caller)) + if err != nil { + t.Fatalf("parse: %v", err) + } + if cfg.Source != "" { + t.Errorf("Source not stripped: got %q (caller can forge builtin tier)", cfg.Source) + } + if cfg.ID != uuid.Nil { + t.Errorf("ID not stripped: got %s (caller can collide with seeded UUIDv5)", cfg.ID) + } + if cfg.CreatedBy != nil { + t.Errorf("CreatedBy not stripped: got %v (caller can lie about provenance)", *cfg.CreatedBy) + } + if cfg.Version != 0 { + t.Errorf("Version not stripped: got %d", cfg.Version) + } + // Sanity: the legitimate fields should round-trip. + if cfg.HandlerType != "http" || cfg.Scope != "tenant" { + t.Errorf("legitimate fields lost: %+v", cfg) + } +} diff --git a/internal/hooks/dispatcher.go b/internal/hooks/dispatcher.go index a80b4703..8953e694 100644 --- a/internal/hooks/dispatcher.go +++ b/internal/hooks/dispatcher.go @@ -6,6 +6,7 @@ import ( "fmt" "log/slog" "sync" + "sync/atomic" "time" "github.com/google/uuid" @@ -322,13 +323,21 @@ func applyBuiltinMutation(ev *Event, updated map[string]any, allowlist []string) // (cmd/gateway_managed.go calls SetBuiltinAllowlistLookup(builtin.AllowlistFor)). // When nil (tests that don't wire the registry) we fall back to the Phase 03 // permissive default so legacy mutation tests stay green. -var builtinAllowlistLookup func(id uuid.UUID) []string +// +// atomic.Pointer guards against the data race that fires when parallel tests +// install/clear the lookup while the dispatcher reads it on another goroutine. +var builtinAllowlistLookup atomic.Pointer[func(uuid.UUID) []string] // SetBuiltinAllowlistLookup installs the per-id allowlist function. Called // once at startup from cmd/gateway_managed.go. Safe to leave unset in tests — -// callers get the permissive default (rawInput + toolInput) below. +// callers get the permissive default (rawInput + toolInput) below. Pass nil +// to clear (used by `defer` cleanup in tests). func SetBuiltinAllowlistLookup(f func(uuid.UUID) []string) { - builtinAllowlistLookup = f + if f == nil { + builtinAllowlistLookup.Store(nil) + return + } + builtinAllowlistLookup.Store(&f) } // builtinAllowlistFor returns the field allowlist for a builtin hook row. @@ -336,8 +345,8 @@ func SetBuiltinAllowlistLookup(f func(uuid.UUID) []string) { // → empty allowlist (mutations stripped, defense-in-depth). Unset lookup // (unit tests) → Phase 03 permissive default. func builtinAllowlistFor(id uuid.UUID) []string { - if builtinAllowlistLookup != nil { - return builtinAllowlistLookup(id) + if fp := builtinAllowlistLookup.Load(); fp != nil { + return (*fp)(id) } return []string{"rawInput", "toolInput"} } diff --git a/internal/hooks/handlers/script.go b/internal/hooks/handlers/script.go index 5d6985b0..a72241cf 100644 --- a/internal/hooks/handlers/script.go +++ b/internal/hooks/handlers/script.go @@ -211,13 +211,40 @@ func (h *ScriptHandler) Execute(ctx context.Context, cfg hooks.HookConfig, ev ho return dec, nil } +// tenantSlotIdleTimeout is how long a per-tenant semaphore can sit unused +// before it becomes eligible for sweep. Picked at 1h: long enough to cover +// idle gaps between user sessions; short enough that long-running multi-tenant +// gateways with churning tenants don't accumulate dead entries forever. +const tenantSlotIdleTimeout = 1 * time.Hour + +// tenantSemSweepThreshold defers sweep cost until the map is large enough to +// matter. Below this, the O(N) scan per acquire is wasted work. +const tenantSemSweepThreshold = 64 + // acquireTenantSem returns the per-tenant slot channel, lazily allocating on // first use. Zero-UUID tenants (system calls) share the same slot keyed by // uuid.Nil, which is acceptable because system-source hooks are still bounded // by the global cap. +// +// Sweep: when the map crosses tenantSemSweepThreshold entries, idle slots +// (no in-flight grants AND last-used > tenantSlotIdleTimeout ago) are dropped +// opportunistically. Goroutine-free design — sweep happens under the same +// lock the acquire path already takes, so steady-state cost stays one +// CompareAndSwap-equivalent for the common path. func (h *ScriptHandler) acquireTenantSem(tid uuid.UUID) chan struct{} { h.tenantMu.Lock() defer h.tenantMu.Unlock() + if len(h.tenantSems) >= tenantSemSweepThreshold { + now := time.Now() + for k, v := range h.tenantSems { + if k == tid { + continue + } + if len(v.sem) == 0 && now.Sub(v.lastUsed) > tenantSlotIdleTimeout { + delete(h.tenantSems, k) + } + } + } s, ok := h.tenantSems[tid] if !ok { s = &tenantSlot{sem: make(chan struct{}, h.perTenantCap)} diff --git a/ui/web/index.html b/ui/web/index.html index 1326be00..0a3f4294 100644 --- a/ui/web/index.html +++ b/ui/web/index.html @@ -14,10 +14,11 @@ Google Fonts stylesheet require inline styles. - font-src self + gstatic — Google Fonts woff2 files. - img-src self + data: — base64 avatars/thumbnails. - - connect-src self + ws:/wss: — websocket to gateway. http(s): for CORS - calls to backend on a different origin in dev. + - connect-src self + ws:/wss: — websocket to gateway. Same-origin only; + if the gateway is hosted on a different domain in production, add the + explicit origin here rather than wildcard-allowing all http(s):. --> - +