mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-07-16 06:16:55 +00:00
viettranx
f3d6c99dda
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.
48 lines
1.4 KiB
Go
48 lines
1.4 KiB
Go
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)
|
|
}
|
|
}
|