mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-16 10:15:12 +00:00
fix(hooks): close source-tier forge + i18n + concurrency hardening
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.
This commit is contained in:
1 parent
31eb953508
commit
f3d6c99dda
5 files changed
+117
-8
No files matched your search
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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"}
|
||||
}
|
||||
|
||||
@@ -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)}
|
||||
|
||||
+4
-3
@@ -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):.
|
||||
-->
|
||||
<meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline' https://fonts.googleapis.com https://cdn.jsdelivr.net; font-src 'self' data: https://fonts.gstatic.com https://cdn.jsdelivr.net; img-src 'self' data: blob:; connect-src 'self' ws: wss: http: https:; frame-ancestors 'none'; base-uri 'self';" />
|
||||
<meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'self'; style-src 'self' 'unsafe-inline' https://fonts.googleapis.com https://cdn.jsdelivr.net; font-src 'self' data: https://fonts.gstatic.com https://cdn.jsdelivr.net; img-src 'self' data: blob:; connect-src 'self' ws: wss:; frame-ancestors 'none'; base-uri 'self';" />
|
||||
<link rel="preconnect" href="https://fonts.googleapis.com" />
|
||||
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin />
|
||||
<link href="https://fonts.googleapis.com/css2?family=Inter:wght@400;500;600;700;800&display=swap" rel="stylesheet" />
|
||||
|
||||
Reference in new issue
Block a user