From 532ff91d8eee7c3dc62f8052d6509146d2b9bb07 Mon Sep 17 00:00:00 2001 From: Duy /zuey/ Date: Wed, 20 May 2026 16:33:49 +0700 Subject: [PATCH] fix(security): harden upstream critical surfaces (#32) * fix(security): harden upstream critical surfaces Refs #30 * fix(security): close pre-landing review gaps Refs #30 * fix(security): close official release blockers --- .env.example | 3 + CHANGELOG.md | 4 + cmd/gateway.go | 17 +- cmd/gateway_http_wiring.go | 4 +- docker-compose.yml | 2 +- docs/18-http-api.md | 6 +- docs/20-api-keys-auth.md | 19 +- docs/23-multi-tenant-architecture.md | 2 +- docs/codebase-summary.md | 4 +- internal/channels/feishu/larkevents.go | 45 ++- internal/channels/feishu/larkevents_test.go | 105 ++++++- internal/channels/pancake/pancake.go | 3 +- .../pancake/pancake_loop_regression_test.go | 16 +- internal/channels/pancake/pancake_test.go | 111 +++++++- internal/channels/pancake/webhook_handler.go | 34 ++- internal/config/config_load.go | 51 +++- internal/config/config_load_test.go | 33 +++ internal/gateway/router.go | 5 +- internal/gateway/router_test.go | 64 +++++ internal/http/auth.go | 11 +- internal/http/auth_test.go | 21 ++ internal/http/files.go | 256 +++++++++++------- internal/http/files_path_security_test.go | 89 ++++++ internal/http/openapi_spec.json | 2 +- internal/http/storage.go | 172 +++++++++--- internal/http/storage_test.go | 211 +++++++++++++++ internal/http/tts_config.go | 30 +- internal/http/tts_config_test.go | 54 ++++ internal/http/webhooks_admin.go | 63 +++-- internal/http/webhooks_admin_test.go | 54 ++++ internal/http/webhooks_auth.go | 23 +- internal/http/webhooks_auth_test.go | 33 ++- internal/http/webhooks_context.go | 14 + internal/http/webhooks_idempotency.go | 161 +++++++++++ internal/http/webhooks_idempotency_test.go | 49 ++++ internal/http/webhooks_llm.go | 120 ++++---- internal/http/webhooks_message.go | 40 +-- internal/sandbox/docker_test.go | 74 +++++ internal/sandbox/fsbridge.go | 151 +++++++++-- internal/store/pg/webhook_calls.go | 11 +- internal/store/sqlitestore/schema.go | 9 +- internal/store/sqlitestore/schema.sql | 2 + .../sqlitestore/schema_migration_test.go | 36 +++ internal/store/sqlitestore/webhook_calls.go | 11 +- internal/store/sqlitestore/webhooks_test.go | 78 ++++++ .../store/workstation_permission_store.go | 2 +- internal/tools/edit.go | 6 +- internal/tools/filesystem.go | 32 +-- internal/tools/filesystem_list.go | 13 +- internal/tools/filesystem_write.go | 17 +- internal/tools/sandbox_utils.go | 21 +- internal/tools/sandbox_utils_test.go | 14 +- internal/tools/workstation_exec.go | 16 ++ internal/tools/workstation_exec_test.go | 85 ++++++ internal/webhooks/worker.go | 20 +- internal/webhooks/worker_test.go | 29 ++ internal/workstation/security/allowlist.go | 14 + .../workstation/security/allowlist_test.go | 15 + .../workstation-create-dialog.tsx | 4 +- 59 files changed, 2194 insertions(+), 397 deletions(-) create mode 100644 internal/gateway/router_test.go create mode 100644 internal/tools/workstation_exec_test.go create mode 100644 internal/workstation/security/allowlist_test.go diff --git a/.env.example b/.env.example index e615d566..3b4506d6 100644 --- a/.env.example +++ b/.env.example @@ -5,6 +5,9 @@ # LLM provider API keys: configure via the web dashboard setup wizard. # --- Gateway (required) --- +# Required for Docker/external binds. Run ./prepare-env.sh to generate. +# Local loopback-only development may opt into empty-token mode with: +# GOCLAW_ALLOW_INSECURE_NO_AUTH=1 GOCLAW_GATEWAY_TOKEN= GOCLAW_ENCRYPTION_KEY= POSTGRES_PASSWORD= diff --git a/CHANGELOG.md b/CHANGELOG.md index 83e4dcb5..e0d36019 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,10 @@ All notable changes to GoClaw are documented here. For full documentation, see [ ### Fixed +- **Upstream critical security remediation** — hardens gateway no-token fallback, + Feishu/Lark and Pancake webhooks, sandbox path/write handling, tenant-admin + checks for mutable HTTP surfaces, and Lite hook schema migration verification. + - **SecureCLI runtime npm binaries** — binary discovery and credentialed exec now resolve tools installed under the GoClaw runtime directories, including `{runtimeDir}/npm-global/bin`, and support single-binary npm package aliases diff --git a/cmd/gateway.go b/cmd/gateway.go index 498a739f..e4136788 100644 --- a/cmd/gateway.go +++ b/cmd/gateway.go @@ -17,24 +17,24 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/bus" "github.com/nextlevelbuilder/goclaw/internal/cache" "github.com/nextlevelbuilder/goclaw/internal/channels" - "github.com/nextlevelbuilder/goclaw/internal/consolidation" - "github.com/nextlevelbuilder/goclaw/internal/eventbus" - kg "github.com/nextlevelbuilder/goclaw/internal/knowledgegraph" "github.com/nextlevelbuilder/goclaw/internal/channels/discord" "github.com/nextlevelbuilder/goclaw/internal/channels/facebook" - "github.com/nextlevelbuilder/goclaw/internal/channels/pancake" "github.com/nextlevelbuilder/goclaw/internal/channels/feishu" + "github.com/nextlevelbuilder/goclaw/internal/channels/pancake" slackchannel "github.com/nextlevelbuilder/goclaw/internal/channels/slack" "github.com/nextlevelbuilder/goclaw/internal/channels/telegram" "github.com/nextlevelbuilder/goclaw/internal/channels/whatsapp" "github.com/nextlevelbuilder/goclaw/internal/channels/zalo" zalopersonal "github.com/nextlevelbuilder/goclaw/internal/channels/zalo/personal" "github.com/nextlevelbuilder/goclaw/internal/config" + "github.com/nextlevelbuilder/goclaw/internal/consolidation" "github.com/nextlevelbuilder/goclaw/internal/edition" + "github.com/nextlevelbuilder/goclaw/internal/eventbus" "github.com/nextlevelbuilder/goclaw/internal/gateway" "github.com/nextlevelbuilder/goclaw/internal/gateway/methods" "github.com/nextlevelbuilder/goclaw/internal/hooks" httpapi "github.com/nextlevelbuilder/goclaw/internal/http" + kg "github.com/nextlevelbuilder/goclaw/internal/knowledgegraph" mcpbridge "github.com/nextlevelbuilder/goclaw/internal/mcp" "github.com/nextlevelbuilder/goclaw/internal/media" "github.com/nextlevelbuilder/goclaw/internal/providers" @@ -84,6 +84,10 @@ func runGateway() { slog.Error("failed to load config", "error", err) os.Exit(1) } + if err := config.ValidateGatewayAuth(cfg.Gateway); err != nil { + slog.Error("unsafe gateway auth configuration", "error", err) + os.Exit(1) + } // Edition override: explicit GOCLAW_EDITION takes precedence over auto-detection. // Auto-detection happens later in setupStoresAndTracing (sqlite → lite). @@ -325,8 +329,8 @@ func runGateway() { agentRouter: agentRouter, toolsReg: toolsReg, skillsLoader: skillsLoader, - enrichProgress: enrichProgress, - enrichWorker: enrichWorker, + enrichProgress: enrichProgress, + enrichWorker: enrichWorker, workspace: workspace, dataDir: dataDir, domainBus: domainBus, @@ -339,6 +343,7 @@ func runGateway() { mcpToolLister = mcpMgr } httpapi.InitGatewayToken(cfg.Gateway.Token) + httpapi.InitGatewayNoAuthFallbackAllowed(config.GatewayNoAuthFallbackAllowed(cfg.Gateway)) exportTokenStore := httpapi.InitExportTokenStore() defer exportTokenStore.Stop() agentsH, skillsH, tracesH, mcpH, channelInstancesH, providersH, builtinToolsH, pendingMessagesH, teamEventsH, secureCLIH, secureCLIGrantH, mcpUserCredsH := wireHTTP(pgStores, cfg.Agents.Defaults.Workspace, dataDir, bundledSkillsDir, msgBus, toolsReg, providerRegistry, modelReg, permPE.IsOwner, gatewayAddr, mcpToolLister) diff --git a/cmd/gateway_http_wiring.go b/cmd/gateway_http_wiring.go index 1bc06ade..7ac461a9 100644 --- a/cmd/gateway_http_wiring.go +++ b/cmd/gateway_http_wiring.go @@ -286,7 +286,7 @@ func (d *gatewayDeps) wireHTTPHandlersOnServer( d.server.SetFilesHandler(httpapi.NewFilesHandler(d.workspace, d.dataDir)) // Storage file management — browse/delete files under the resolved workspace directory. - d.server.SetStorageHandler(httpapi.NewStorageHandler(d.workspace)) + d.server.SetStorageHandler(httpapi.NewStorageHandler(d.workspace, d.pgStores.Tenants)) // Media upload endpoint — accepts multipart file uploads, returns temp path + MIME type. d.server.SetMediaUploadHandler(httpapi.NewMediaUploadHandler()) @@ -336,7 +336,7 @@ func (d *gatewayDeps) wireHTTPHandlersOnServer( // Per-tenant TTS config endpoint — allows tenant admins to configure TTS. if d.pgStores.SystemConfigs != nil && d.pgStores.ConfigSecrets != nil { - d.server.SetTTSConfigHandler(httpapi.NewTTSConfigHandler(d.pgStores.SystemConfigs, d.pgStores.ConfigSecrets)) + d.server.SetTTSConfigHandler(httpapi.NewTTSConfigHandler(d.pgStores.SystemConfigs, d.pgStores.ConfigSecrets, d.pgStores.Tenants)) } // Workstations API — Standard edition only. diff --git a/docker-compose.yml b/docker-compose.yml index aed3e2a8..73596d65 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -44,7 +44,7 @@ services: - GOCLAW_HOST=0.0.0.0 - GOCLAW_PORT=18790 - GOCLAW_CONFIG=/app/data/config.json - - GOCLAW_GATEWAY_TOKEN=${GOCLAW_GATEWAY_TOKEN:-} + - GOCLAW_GATEWAY_TOKEN=${GOCLAW_GATEWAY_TOKEN:?run ./prepare-env.sh or set GOCLAW_GATEWAY_TOKEN} - GOCLAW_ENCRYPTION_KEY=${GOCLAW_ENCRYPTION_KEY:-} - GOCLAW_SKILLS_DIR=/app/data/skills # Debug diff --git a/docs/18-http-api.md b/docs/18-http-api.md index ca4aabea..7b07ccc0 100644 --- a/docs/18-http-api.md +++ b/docs/18-http-api.md @@ -978,7 +978,7 @@ Team activity and audit trail. ## 20. Secure CLI Credentials -CLI authentication credentials for secure command execution. Requires **admin role** (full gateway token or empty gateway token in dev/single-user mode). +CLI authentication credentials for secure command execution. Requires **admin role** (gateway token or empty-token local/dev fallback). | Method | Path | Description | |--------|------|-------------| @@ -1014,7 +1014,7 @@ CLI authentication credentials for secure command execution. Requires **admin ro ## 21. Runtime & Packages Management -Manage system (apk), Python (pip), and Node (npm) package installation in the GoClaw runtime container. These endpoints do not inspect host-level runtimes. Requires authentication. When `GOCLAW_GATEWAY_TOKEN` is empty (dev/single-user mode), all users get admin role and can manage packages. +Manage system (apk), Python (pip), and Node (npm) package installation in the GoClaw runtime container. These endpoints do not inspect host-level runtimes. Requires authentication. Empty-token admin access is limited to loopback local development or explicit `GOCLAW_ALLOW_INSECURE_NO_AUTH=1`; external binds require `GOCLAW_GATEWAY_TOKEN`. ### List Installed Packages @@ -1178,7 +1178,7 @@ Workspace file management. | `GET` | `/v1/storage/files` | List files with depth limiting | | `GET` | `/v1/storage/files/{path...}` | Read file (JSON or raw) | | `POST` | `/v1/storage/files` | Upload file (admin) | -| `DELETE` | `/v1/storage/files/{path...}` | Delete file/directory | +| `DELETE` | `/v1/storage/files/{path...}` | Delete file/directory (admin) | | `PUT` | `/v1/storage/move` | Move/rename file (admin) | | `GET` | `/v1/storage/size` | Stream storage size (Server-Sent Events, cached 60 min) | diff --git a/docs/20-api-keys-auth.md b/docs/20-api-keys-auth.md index 7ebb133e..a0b28c42 100644 --- a/docs/20-api-keys-auth.md +++ b/docs/20-api-keys-auth.md @@ -32,6 +32,15 @@ Or in WebSocket `connect`: The gateway token is compared using **constant-time comparison** (`crypto/subtle.ConstantTimeCompare`) in both HTTP and WebSocket auth paths to prevent timing attacks. The comparison reveals no information about where the provided token first differs from the expected token. +Externally reachable deployments must configure a gateway token. If `gateway.token` / `GOCLAW_GATEWAY_TOKEN` is empty while the gateway binds to `0.0.0.0`, `::`, or a non-loopback address, startup fails before the health endpoint reports ready. + +Empty-token compatibility is only for local development: + +- bind `GOCLAW_HOST` to loopback (`127.0.0.1`, `localhost`, or `::1`), or +- set `GOCLAW_ALLOW_INSECURE_NO_AUTH=1` explicitly. + +The explicit opt-in applies to both HTTP and WebSocket. Do not use it on shared hosts, Docker ports exposed outside the machine, or production deployments. + --- ## 2. API Keys @@ -100,7 +109,7 @@ GoClaw tries authentication methods in this priority order: 1. **Gateway token** (exact match via constant-time comparison) → `RoleAdmin` or `RoleOwner` for configured owner IDs 2. **API key** (SHA-256 hash lookup in `api_keys` table) → role from scopes 3. **Browser pairing** (sender ID must be paired with "browser" device type) → `RoleOperator` (HTTP only; requires `X-GoClaw-Sender-Id` header) -4. **No auth configured** (backward compatibility: if no gateway token is set) → full-access dev mode +4. **No auth configured and local/dev mode explicitly allowed** → full-access dev mode 5. **No valid auth found** → `401 Unauthorized` ### HTTP Request Flow @@ -116,13 +125,15 @@ flowchart TD G -->|Yes| H[Derive role from scopes] G -->|No| I{Gateway token configured?} I -->|Yes| J[401 Unauthorized] - I -->|No| K[Full-access backward compat] + I -->|No| K{Local/dev fallback allowed?} + K -->|No| J + K -->|Yes| O[Full-access backward compat] C -->|Check paired device| L{Device paired?} L -->|Yes| M[RoleOperator] L -->|No| J E --> N[Authenticate request] H --> N - K --> N + O --> N M --> N ``` @@ -164,7 +175,7 @@ On successful API key authentication, `last_used_at` is updated asynchronously ( ### Backward Compatibility -If no gateway token is configured (`gateway.token` is empty in `config.json`), unauthenticated requests run in backward-compatibility full-access mode. This enables self-hosted deployments without strict authentication. Once a gateway token is configured, all requests must authenticate or use browser pairing. +If no gateway token is configured (`gateway.token` is empty in `config.json`), unauthenticated requests run in backward-compatibility full-access mode only for loopback local development or when `GOCLAW_ALLOW_INSECURE_NO_AUTH=1` is set. Once a gateway token is configured, all requests must authenticate or use browser pairing. --- diff --git a/docs/23-multi-tenant-architecture.md b/docs/23-multi-tenant-architecture.md index f5e33a28..48cfacf2 100644 --- a/docs/23-multi-tenant-architecture.md +++ b/docs/23-multi-tenant-architecture.md @@ -158,7 +158,7 @@ GoClaw determines the tenant from the credentials used to connect: | **API key** (tenant-bound) | Auto from key's `tenant_id` | Normal SaaS integration | | **API key** (system-level) + `X-GoClaw-Tenant-Id` | Header value (UUID or slug), while keeping the key's original role | Cross-tenant tools | | **Browser pairing** | Master tenant by default, or a membership-validated tenant hint | Dashboard operators | -| **No credentials** | Master tenant | Dev/single-user mode | +| **No credentials** | Master tenant | Loopback local development or explicit `GOCLAW_ALLOW_INSECURE_NO_AUTH=1` only | **Owner IDs:** Configured via `GOCLAW_OWNER_IDS` env var (comma-separated). Only owners get cross-tenant access with the gateway token. Default: `system`. diff --git a/docs/codebase-summary.md b/docs/codebase-summary.md index ca176757..87073669 100644 --- a/docs/codebase-summary.md +++ b/docs/codebase-summary.md @@ -119,7 +119,6 @@ Parity enforced by `ui/web/src/__tests__/i18n-tts-key-parity.test.ts` (vitest). --- -<<<<<<< HEAD ## Image Generation Native `image_generation` support in the Codex provider (`POST /codex/responses`) + passthrough in the OpenAI-compat path. @@ -138,7 +137,7 @@ Native `image_generation` support in the Codex provider (`POST /codex/responses` **Persistence:** `internal/agent/media.go persistAssistantImages()` writes final images to `{workspace}/media/{sha256}.{ext}`, returns `MediaRef` entries, clears inline `Images[]`. Idempotent on hash. Invoked from `pipeline.FinalizeStage` via `Deps.PersistAssistantImages` callback. **Web UI:** Download filename resolver (`imageGenDownloadName`) in `ui/web/src/components/chat/media-gallery.tsx`. Image generation works automatically when the agent has the `create_image` tool — no user-facing toggle. -======= + ## Webhook Subsystem External systems invoke agents or send channel messages via webhooks without gateway tokens. @@ -188,7 +187,6 @@ Raw webhook secret encrypted at rest via AES-256-GCM using `GOCLAW_ENCRYPTION_KE All webhook calls logged with canonical `{"body_hash":"","meta":{...}}` shape in `webhook_calls.request_payload` (JSON). Used by idempotency checker to detect body mismatches on replay. ->>>>>>> a83f4090 (fix(webhooks): address post-review findings (K1-K10)) --- diff --git a/internal/channels/feishu/larkevents.go b/internal/channels/feishu/larkevents.go index 9b1f0f2b..fc2e4cd3 100644 --- a/internal/channels/feishu/larkevents.go +++ b/internal/channels/feishu/larkevents.go @@ -13,6 +13,8 @@ import ( "strings" ) +const maxWebhookBodyBytes = 1 << 20 + // --- Event types (replacing larkim.P2MessageReceiveV1) --- // MessageEvent is the parsed structure of a Feishu im.message.receive_v1 event. @@ -42,9 +44,9 @@ type EventSender struct { } type EventMessage struct { - MessageID string `json:"message_id"` - RootID string `json:"root_id"` - ParentID string `json:"parent_id"` + MessageID string `json:"message_id"` + RootID string `json:"root_id"` + ParentID string `json:"parent_id"` // ThreadID is the definitive "this message lives inside a thread" signal // per Lark docs. Unlike RootID (which is populated on ANY reply — including // plain quote replies), ThreadID is only present when the message is in an @@ -59,8 +61,8 @@ type EventMessage struct { } type EventMention struct { - Key string `json:"key"` - ID struct { + Key string `json:"key"` + ID struct { OpenID string `json:"open_id"` UserID string `json:"user_id"` UnionID string `json:"union_id"` @@ -75,9 +77,9 @@ type EventMention struct { // Schema v1.0 uses flat structure, v2.0 uses header+event. type webhookEvent struct { // v2.0 fields - Schema string `json:"schema"` - Header json.RawMessage `json:"header"` - Event json.RawMessage `json:"event"` + Schema string `json:"schema"` + Header json.RawMessage `json:"header"` + Event json.RawMessage `json:"event"` // v1.0 fields (also used for URL verification challenge) Type string `json:"type"` @@ -97,11 +99,15 @@ func NewWebhookHandler(verificationToken, encryptKey string, onMessage func(even return } - body, err := io.ReadAll(r.Body) + body, err := io.ReadAll(io.LimitReader(r.Body, maxWebhookBodyBytes+1)) if err != nil { http.Error(w, "read body failed", http.StatusBadRequest) return } + if len(body) > maxWebhookBodyBytes { + http.Error(w, "body too large", http.StatusRequestEntityTooLarge) + return + } // Try to decrypt if encrypted var envelope webhookEvent @@ -129,11 +135,22 @@ func NewWebhookHandler(verificationToken, encryptKey string, onMessage func(even // URL verification challenge if envelope.Type == "url_verification" { + if verificationToken == "" || envelope.Token != verificationToken { + slog.Warn("security.feishu_webhook_url_verification_rejected") + w.WriteHeader(http.StatusOK) + return + } w.Header().Set("Content-Type", "application/json") json.NewEncoder(w).Encode(map[string]string{"challenge": envelope.Challenge}) return } + if encryptKey != "" && envelope.Encrypt == "" { + slog.Warn("security.feishu_webhook_plaintext_rejected") + w.WriteHeader(http.StatusOK) + return + } + // Parse as message event var event MessageEvent @@ -144,8 +161,13 @@ func NewWebhookHandler(verificationToken, encryptKey string, onMessage func(even } // Verify token if configured + if verificationToken == "" && encryptKey == "" { + slog.Warn("security.feishu_webhook_missing_verification") + w.WriteHeader(http.StatusOK) + return + } if verificationToken != "" && event.Header.Token != verificationToken { - slog.Warn("feishu webhook token mismatch") + slog.Warn("security.feishu_webhook_token_mismatch") w.WriteHeader(http.StatusOK) return } @@ -181,6 +203,9 @@ func decryptEvent(encryptedBase64, key string) ([]byte, error) { // IV is first 16 bytes iv := ciphertext[:aes.BlockSize] ciphertext = ciphertext[aes.BlockSize:] + if len(ciphertext) == 0 || len(ciphertext)%aes.BlockSize != 0 { + return nil, fmt.Errorf("ciphertext length not block aligned") + } mode := cipher.NewCBCDecrypter(block, iv) mode.CryptBlocks(ciphertext, ciphertext) diff --git a/internal/channels/feishu/larkevents_test.go b/internal/channels/feishu/larkevents_test.go index da63cbe5..e5fef48a 100644 --- a/internal/channels/feishu/larkevents_test.go +++ b/internal/channels/feishu/larkevents_test.go @@ -63,7 +63,7 @@ func buildWebhookRequest(body string) *http.Request { func TestWebhookHandler_URLVerification(t *testing.T) { called := false - h := NewWebhookHandler("", "", func(_ *MessageEvent) { called = true }) + h := NewWebhookHandler("test-tok", "", func(_ *MessageEvent) { called = true }) body := `{"type":"url_verification","token":"test-tok","challenge":"abc123"}` w := httptest.NewRecorder() @@ -84,6 +84,24 @@ func TestWebhookHandler_URLVerification(t *testing.T) { } } +func TestWebhookHandler_URLVerificationRequiresMatchingToken(t *testing.T) { + h := NewWebhookHandler("expected-token", "", func(_ *MessageEvent) { + t.Fatal("onMessage must not be called for url_verification") + }) + + body := `{"type":"url_verification","token":"wrong-token","challenge":"abc123"}` + w := httptest.NewRecorder() + h.ServeHTTP(w, buildWebhookRequest(body)) + + if w.Code != http.StatusOK { + t.Errorf("status: got %d, want 200", w.Code) + } + var resp map[string]string + if err := json.NewDecoder(w.Body).Decode(&resp); err == nil && resp["challenge"] != "" { + t.Fatalf("must not return challenge for mismatched token, got %q", resp["challenge"]) + } +} + // --- Method not allowed --- func TestWebhookHandler_MethodNotAllowed(t *testing.T) { @@ -179,6 +197,72 @@ func TestWebhookHandler_TokenMatch_Dispatches(t *testing.T) { } } +func TestWebhookHandler_MissingVerificationTokenDoesNotDispatchMessage(t *testing.T) { + dispatched := make(chan *MessageEvent, 1) + h := NewWebhookHandler("", "", func(e *MessageEvent) { dispatched <- e }) + + env := map[string]any{ + "schema": "2.0", + "header": map[string]any{ + "event_id": "evt_missing_token", + "event_type": "im.message.receive_v1", + "token": "", + "app_id": "cli_test", + "tenant_key": "test-tenant-1", + }, + "event": map[string]any{ + "sender": map[string]any{}, + "message": map[string]any{"message_id": "om_1", "chat_id": "oc_1"}, + }, + } + body, _ := json.Marshal(env) + + w := httptest.NewRecorder() + h.ServeHTTP(w, buildWebhookRequest(string(body))) + + if w.Code != http.StatusOK { + t.Errorf("status: got %d, want 200", w.Code) + } + select { + case <-dispatched: + t.Fatal("onMessage must not be called when verification token is missing") + case <-time.After(100 * time.Millisecond): + } +} + +func TestWebhookHandler_EncryptKeyRejectsPlaintextEvent(t *testing.T) { + dispatched := make(chan *MessageEvent, 1) + h := NewWebhookHandler("", "encrypt-key", func(e *MessageEvent) { dispatched <- e }) + + env := map[string]any{ + "schema": "2.0", + "header": map[string]any{ + "event_id": "evt_plaintext", + "event_type": "im.message.receive_v1", + "token": "", + "app_id": "cli_test", + "tenant_key": "test-tenant-1", + }, + "event": map[string]any{ + "sender": map[string]any{}, + "message": map[string]any{"message_id": "om_1", "chat_id": "oc_1"}, + }, + } + body, _ := json.Marshal(env) + + w := httptest.NewRecorder() + h.ServeHTTP(w, buildWebhookRequest(string(body))) + + if w.Code != http.StatusOK { + t.Errorf("status: got %d, want 200", w.Code) + } + select { + case <-dispatched: + t.Fatal("onMessage must not be called for plaintext event when encrypt key is configured") + case <-time.After(100 * time.Millisecond): + } +} + // --- Non-message event type --- func TestWebhookHandler_NonMessageEvent_Ignored(t *testing.T) { @@ -217,6 +301,18 @@ func TestWebhookHandler_InvalidJSON(t *testing.T) { } } +func TestWebhookHandler_RejectsOversizedBody(t *testing.T) { + h := NewWebhookHandler("", "", func(_ *MessageEvent) { + t.Fatal("onMessage must not be called for oversized body") + }) + w := httptest.NewRecorder() + h.ServeHTTP(w, buildWebhookRequest(strings.Repeat("x", maxWebhookBodyBytes+1))) + + if w.Code != http.StatusRequestEntityTooLarge { + t.Errorf("status: got %d, want 413", w.Code) + } +} + // --- Encrypted event --- func TestWebhookHandler_EncryptedEvent_Decrypted(t *testing.T) { @@ -281,6 +377,13 @@ func TestDecryptEvent_InvalidBase64(t *testing.T) { } } +func TestDecryptEvent_RejectsNonBlockMultipleCiphertext(t *testing.T) { + payload := base64.StdEncoding.EncodeToString([]byte("12345678901234567")) + if _, err := decryptEvent(payload, "key"); err == nil { + t.Fatal("expected error for non-block-multiple ciphertext") + } +} + func TestDecryptEvent_TooShort(t *testing.T) { // Valid base64 but shorter than AES block size (16 bytes) short := base64.StdEncoding.EncodeToString([]byte("short")) diff --git a/internal/channels/pancake/pancake.go b/internal/channels/pancake/pancake.go index 968bd24b..def64d0f 100644 --- a/internal/channels/pancake/pancake.go +++ b/internal/channels/pancake/pancake.go @@ -150,7 +150,7 @@ func (ch *Channel) Start(ctx context.Context) error { if ch.webhookSecret == "" { slog.Warn("security.pancake_webhook_no_secret", "page_id", ch.pageID, - "note", "webhook_secret not configured; incoming webhook requests will not be authenticated") + "note", "webhook_secret not configured; incoming webhook requests will be ignored until configured") } // Without HMAC, any actor reaching the webhook endpoint can trigger Pancake API calls. @@ -370,4 +370,3 @@ func (ch *Channel) maxMessageLength() int { return 2000 } } - diff --git a/internal/channels/pancake/pancake_loop_regression_test.go b/internal/channels/pancake/pancake_loop_regression_test.go index 4a9e579a..ee76c0b4 100644 --- a/internal/channels/pancake/pancake_loop_regression_test.go +++ b/internal/channels/pancake/pancake_loop_regression_test.go @@ -44,9 +44,10 @@ func TestMessageHandlerSkipsRecentOutboundEchoWithHTMLFormatting(t *testing.T) { func TestWebhookRouterSkipsNonInboxConversationEvents(t *testing.T) { msgBus := bus.New() target := &Channel{ - BaseChannel: channels.NewBaseChannel(channels.TypePancake, msgBus, nil), - pageID: "page-123", - platform: "facebook", + BaseChannel: channels.NewBaseChannel(channels.TypePancake, msgBus, nil), + pageID: "page-123", + platform: "facebook", + webhookSecret: "test-secret", } router := &webhookRouter{ instances: map[string]*Channel{ @@ -74,6 +75,7 @@ func TestWebhookRouterSkipsNonInboxConversationEvents(t *testing.T) { }` req := httptest.NewRequest(http.MethodPost, "/channels/pancake/webhook", strings.NewReader(body)) + signTestPancakeRequest(req, body, target.webhookSecret) w := httptest.NewRecorder() router.ServeHTTP(w, req) @@ -92,9 +94,10 @@ func TestWebhookRouterSkipsNonInboxConversationEvents(t *testing.T) { func TestWebhookRouterPrefersMessageSenderOverConversationSender(t *testing.T) { msgBus := bus.New() target := &Channel{ - BaseChannel: channels.NewBaseChannel(channels.TypePancake, msgBus, nil), - pageID: "page-123", - platform: "facebook", + BaseChannel: channels.NewBaseChannel(channels.TypePancake, msgBus, nil), + pageID: "page-123", + platform: "facebook", + webhookSecret: "test-secret", } router := &webhookRouter{ instances: map[string]*Channel{ @@ -127,6 +130,7 @@ func TestWebhookRouterPrefersMessageSenderOverConversationSender(t *testing.T) { }` req := httptest.NewRequest(http.MethodPost, "/channels/pancake/webhook", strings.NewReader(body)) + signTestPancakeRequest(req, body, target.webhookSecret) w := httptest.NewRecorder() router.ServeHTTP(w, req) diff --git a/internal/channels/pancake/pancake_test.go b/internal/channels/pancake/pancake_test.go index 0f073ad7..de0e54be 100644 --- a/internal/channels/pancake/pancake_test.go +++ b/internal/channels/pancake/pancake_test.go @@ -3,6 +3,9 @@ package pancake import ( "bytes" "context" + "crypto/hmac" + "crypto/sha256" + "encoding/hex" "encoding/json" "fmt" "io" @@ -537,12 +540,18 @@ func buildWebhookBody(pageID, convID, convType, senderID, msgID, content, postID pageID, conv, msgID, content) } +func signTestPancakeRequest(req *http.Request, body, secret string) { + mac := hmac.New(sha256.New, []byte(secret)) + mac.Write([]byte(body)) + req.Header.Set("X-Pancake-Signature", "sha256="+hex.EncodeToString(mac.Sum(nil))) +} + // newTestRouter creates an isolated webhookRouter with a registered channel. func newTestRouter(t *testing.T, cfg pancakeInstanceConfig) (*webhookRouter, *Channel, *bus.MessageBus) { t.Helper() msgBus := bus.New() cfg.PageID = "page-test" - creds := pancakeCreds{APIKey: "k", PageAccessToken: "t"} + creds := pancakeCreds{APIKey: "k", PageAccessToken: "t", WebhookSecret: "test-secret"} ch, err := New(cfg, creds, msgBus, nil) if err != nil { t.Fatalf("New: %v", err) @@ -557,10 +566,12 @@ func newTestRouter(t *testing.T, cfg pancakeInstanceConfig) (*webhookRouter, *Ch func TestWebhookRouterRoutesCommentEvent(t *testing.T) { cfg := pancakeInstanceConfig{} cfg.Features.CommentReply = true - router, _, msgBus := newTestRouter(t, cfg) + router, ch, msgBus := newTestRouter(t, cfg) + ch.webhookSecret = "test-secret" body := buildWebhookBody("page-test", "conv-1", "COMMENT", "user-1", "msg-1", "hello", "") req := httptest.NewRequest(http.MethodPost, webhookPath, strings.NewReader(body)) + signTestPancakeRequest(req, body, ch.webhookSecret) w := httptest.NewRecorder() router.ServeHTTP(w, req) @@ -590,7 +601,7 @@ func TestWebhookRouterRoutesWebhookPageID(t *testing.T) { msgBus := bus.New() cfg.PageID = "pancake-internal-id" - creds := pancakeCreds{APIKey: "k", PageAccessToken: "t"} + creds := pancakeCreds{APIKey: "k", PageAccessToken: "t", WebhookSecret: "test-secret"} ch, err := New(cfg, creds, msgBus, nil) if err != nil { t.Fatalf("New: %v", err) @@ -611,6 +622,7 @@ func TestWebhookRouterRoutesWebhookPageID(t *testing.T) { // Webhook arrives with Facebook native page ID — must route to the channel. body := buildWebhookBody("fb-native-id", "conv-1", "COMMENT", "user-1", "msg-1", "hello", "") req := httptest.NewRequest(http.MethodPost, webhookPath, strings.NewReader(body)) + signTestPancakeRequest(req, body, ch.webhookSecret) w := httptest.NewRecorder() router.ServeHTTP(w, req) @@ -632,10 +644,12 @@ func TestWebhookRouterRoutesWebhookPageID(t *testing.T) { func TestWebhookRouterRoutesInboxEvent(t *testing.T) { cfg := pancakeInstanceConfig{} cfg.Features.InboxReply = true - router, _, msgBus := newTestRouter(t, cfg) + router, ch, msgBus := newTestRouter(t, cfg) + ch.webhookSecret = "test-secret" body := buildWebhookBody("page-test", "conv-1", "INBOX", "user-1", "msg-2", "inbox msg", "") req := httptest.NewRequest(http.MethodPost, webhookPath, strings.NewReader(body)) + signTestPancakeRequest(req, body, ch.webhookSecret) w := httptest.NewRecorder() router.ServeHTTP(w, req) @@ -651,6 +665,77 @@ func TestWebhookRouterRoutesInboxEvent(t *testing.T) { } } +func TestWebhookRouterMissingSecretDoesNotDispatch(t *testing.T) { + cfg := pancakeInstanceConfig{} + cfg.Features.InboxReply = true + router, ch, msgBus := newTestRouter(t, cfg) + ch.webhookSecret = "" + + body := buildWebhookBody("page-test", "conv-1", "INBOX", "user-1", "msg-2", "inbox msg", "") + req := httptest.NewRequest(http.MethodPost, webhookPath, strings.NewReader(body)) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected provider-safe 200, got %d", w.Code) + } + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + if _, ok := msgBus.ConsumeInbound(ctx); ok { + t.Fatal("expected no dispatch when webhook secret is missing") + } +} + +func TestWebhookRouterSignatureMismatchDoesNotDispatch(t *testing.T) { + cfg := pancakeInstanceConfig{} + cfg.Features.InboxReply = true + router, ch, msgBus := newTestRouter(t, cfg) + + body := buildWebhookBody("page-test", "conv-1", "INBOX", "user-1", "msg-2", "inbox msg", "") + req := httptest.NewRequest(http.MethodPost, webhookPath, strings.NewReader(body)) + signTestPancakeRequest(req, body, "wrong-secret") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected provider-safe 200, got %d", w.Code) + } + if ch.webhookSecret == "" { + t.Fatal("test setup error: expected configured webhook secret") + } + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + if _, ok := msgBus.ConsumeInbound(ctx); ok { + t.Fatal("expected no dispatch on signature mismatch") + } +} + +func TestWebhookRouterDuplicateSignedBodyDoesNotDispatchTwice(t *testing.T) { + cfg := pancakeInstanceConfig{} + cfg.Features.InboxReply = true + router, ch, msgBus := newTestRouter(t, cfg) + + body := buildWebhookBody("page-test", "conv-1", "INBOX", "user-1", "", "inbox msg", "") + for i := 0; i < 2; i++ { + req := httptest.NewRequest(http.MethodPost, webhookPath, strings.NewReader(body)) + signTestPancakeRequest(req, body, ch.webhookSecret) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Fatalf("request %d status = %d, want 200", i+1, w.Code) + } + } + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + if _, ok := msgBus.ConsumeInbound(ctx); !ok { + t.Fatal("expected first signed webhook to dispatch") + } + if _, ok := msgBus.ConsumeInbound(ctx); ok { + t.Fatal("expected duplicate signed webhook body to be skipped") + } +} + func TestWebhookRouterSkipsUnknownType(t *testing.T) { cfg := pancakeInstanceConfig{} cfg.Features.CommentReply = true @@ -675,10 +760,12 @@ func TestWebhookRouterSkipsUnknownType(t *testing.T) { func TestWebhookRouterCommentNormalizesPostID(t *testing.T) { cfg := pancakeInstanceConfig{} cfg.Features.CommentReply = true - router, _, msgBus := newTestRouter(t, cfg) + router, ch, msgBus := newTestRouter(t, cfg) + ch.webhookSecret = "test-secret" body := buildWebhookBody("page-test", "conv-1", "COMMENT", "user-1", "msg-4", "hello", "post-123") req := httptest.NewRequest(http.MethodPost, webhookPath, strings.NewReader(body)) + signTestPancakeRequest(req, body, ch.webhookSecret) w := httptest.NewRecorder() router.ServeHTTP(w, req) @@ -697,9 +784,9 @@ func TestWebhookRouterCommentNormalizesPostID(t *testing.T) { // multiCaptureTransport records multiple requests (for first-inbox tests). type multiCaptureTransport struct { - reqs []*http.Request + reqs []*http.Request bodies [][]byte - mu sync.Mutex + mu sync.Mutex } func (t *multiCaptureTransport) RoundTrip(req *http.Request) (*http.Response, error) { @@ -726,7 +813,7 @@ func newChannelWithMultiCapture(t *testing.T, cfg pancakeInstanceConfig) (*Chann transport := &multiCaptureTransport{} msgBus := bus.New() cfg.PageID = "page-123" - creds := pancakeCreds{APIKey: "k", PageAccessToken: "t"} + creds := pancakeCreds{APIKey: "k", PageAccessToken: "t", WebhookSecret: "test-secret"} ch, err := New(cfg, creds, msgBus, nil) if err != nil { t.Fatalf("New: %v", err) @@ -1054,7 +1141,7 @@ func TestCommentFlowEndToEnd(t *testing.T) { transport := &multiCaptureTransport{} msgBus := bus.New() cfg.PageID = "page-e2e" - creds := pancakeCreds{APIKey: "k", PageAccessToken: "t"} + creds := pancakeCreds{APIKey: "k", PageAccessToken: "t", WebhookSecret: "test-secret"} ch, err := New(cfg, creds, msgBus, nil) if err != nil { t.Fatalf("New: %v", err) @@ -1067,6 +1154,7 @@ func TestCommentFlowEndToEnd(t *testing.T) { // Step 1: POST comment webhook. body := buildWebhookBody("page-e2e", "conv-e2e", "COMMENT", "user-e2e", "msg-e2e", "great product!", "") req := httptest.NewRequest(http.MethodPost, webhookPath, strings.NewReader(body)) + signTestPancakeRequest(req, body, ch.webhookSecret) w := httptest.NewRecorder() router.ServeHTTP(w, req) @@ -1088,8 +1176,8 @@ func TestCommentFlowEndToEnd(t *testing.T) { // Step 4: Send outbound reply. outMsg := bus.OutboundMessage{ - ChatID: inMsg.ChatID, - Content: "thank you!", + ChatID: inMsg.ChatID, + Content: "thank you!", Metadata: inMsg.Metadata, } if err := ch.Send(context.Background(), outMsg); err != nil { @@ -1122,6 +1210,7 @@ func TestCommentFlowEndToEnd(t *testing.T) { // Step 6: Second comment from same sender — stateless: another DM fires. body2 := buildWebhookBody("page-e2e", "conv-e2e", "COMMENT", "user-e2e", "msg-e2e-2", "another comment", "") req2 := httptest.NewRequest(http.MethodPost, webhookPath, strings.NewReader(body2)) + signTestPancakeRequest(req2, body2, ch.webhookSecret) w2 := httptest.NewRecorder() router.ServeHTTP(w2, req2) diff --git a/internal/channels/pancake/webhook_handler.go b/internal/channels/pancake/webhook_handler.go index ca7f03f1..cbc05ce9 100644 --- a/internal/channels/pancake/webhook_handler.go +++ b/internal/channels/pancake/webhook_handler.go @@ -34,6 +34,11 @@ func verifyHMAC(body []byte, secret, signature string) bool { return hmac.Equal(got, expected) } +func webhookReplayKey(body []byte) string { + sum := sha256.Sum256(body) + return "webhook:" + hex.EncodeToString(sum[:]) +} + // --- Global webhook router for multi-page support --- // webhookRouter routes incoming Pancake webhook events to the correct channel instance by page_id. @@ -176,16 +181,25 @@ func (r *webhookRouter) ServeHTTP(w http.ResponseWriter, req *http.Request) { return } - // HMAC signature verification — skip if webhook_secret not configured. - if target.webhookSecret != "" { - sig := req.Header.Get("X-Pancake-Signature") - if !verifyHMAC(body, target.webhookSecret, sig) { - slog.Warn("security.pancake_webhook_signature_mismatch", - "page_id", pageID, - "remote_addr", req.RemoteAddr) - w.WriteHeader(http.StatusOK) - return - } + if target.webhookSecret == "" { + slog.Warn("security.pancake_webhook_missing_secret", + "page_id", pageID, + "remote_addr", req.RemoteAddr) + w.WriteHeader(http.StatusOK) + return + } + sig := req.Header.Get("X-Pancake-Signature") + if !verifyHMAC(body, target.webhookSecret, sig) { + slog.Warn("security.pancake_webhook_signature_mismatch", + "page_id", pageID, + "remote_addr", req.RemoteAddr) + w.WriteHeader(http.StatusOK) + return + } + if target.isDup(webhookReplayKey(body)) { + slog.Info("pancake: duplicate webhook skipped", "page_id", pageID) + w.WriteHeader(http.StatusOK) + return } // Build normalized MessagingData from actual Pancake payload. diff --git a/internal/config/config_load.go b/internal/config/config_load.go index d12fdece..83afe346 100644 --- a/internal/config/config_load.go +++ b/internal/config/config_load.go @@ -5,6 +5,8 @@ import ( "encoding/json" "fmt" "log/slog" + "net" + "net/netip" "os" "path/filepath" "strconv" @@ -13,6 +15,54 @@ import ( "github.com/titanous/json5" ) +const GatewayAllowInsecureNoAuthEnv = "GOCLAW_ALLOW_INSECURE_NO_AUTH" + +// GatewayNoAuthFallbackAllowed reports whether empty-token gateway auth may +// run in local/dev compatibility mode. +func GatewayNoAuthFallbackAllowed(g GatewayConfig) bool { + if strings.TrimSpace(g.Token) != "" { + return false + } + if insecureNoAuthOptIn() { + return true + } + return isLoopbackGatewayHost(g.Host) +} + +// ValidateGatewayAuth fails configurations that would expose the gateway +// without any bearer token. +func ValidateGatewayAuth(g GatewayConfig) error { + if strings.TrimSpace(g.Token) != "" || GatewayNoAuthFallbackAllowed(g) { + return nil + } + return fmt.Errorf("gateway token is required when GOCLAW_HOST=%q; set GOCLAW_GATEWAY_TOKEN or explicit %s=1 for local development only", g.Host, GatewayAllowInsecureNoAuthEnv) +} + +func insecureNoAuthOptIn() bool { + switch strings.ToLower(strings.TrimSpace(os.Getenv(GatewayAllowInsecureNoAuthEnv))) { + case "1", "true", "yes", "on": + return true + default: + return false + } +} + +func isLoopbackGatewayHost(host string) bool { + host = strings.TrimSpace(host) + if host == "" { + return false + } + if h, _, err := net.SplitHostPort(host); err == nil { + host = h + } + host = strings.Trim(host, "[]") + if strings.EqualFold(host, "localhost") { + return true + } + addr, err := netip.ParseAddr(host) + return err == nil && addr.IsLoopback() +} + // Default returns a Config with sensible defaults. func Default() *Config { return &Config{ @@ -285,7 +335,6 @@ func (c *Config) applyEnvOverrides() { } } - // Save writes the config to a JSON file. func Save(path string, cfg *Config) error { cfg.mu.RLock() diff --git a/internal/config/config_load_test.go b/internal/config/config_load_test.go index 09081520..14923e73 100644 --- a/internal/config/config_load_test.go +++ b/internal/config/config_load_test.go @@ -116,6 +116,39 @@ func TestLoad_EnvVarOverrides_InvalidPort(t *testing.T) { } } +func TestValidateGatewayAuthRejectsExternalNoToken(t *testing.T) { + cfg := Default() + cfg.Gateway.Host = "0.0.0.0" + cfg.Gateway.Token = "" + t.Setenv(GatewayAllowInsecureNoAuthEnv, "") + + if err := ValidateGatewayAuth(cfg.Gateway); err == nil { + t.Fatal("expected external bind with empty gateway token to fail") + } +} + +func TestValidateGatewayAuthAllowsLoopbackNoToken(t *testing.T) { + cfg := Default() + cfg.Gateway.Host = "127.0.0.1" + cfg.Gateway.Token = "" + t.Setenv(GatewayAllowInsecureNoAuthEnv, "") + + if err := ValidateGatewayAuth(cfg.Gateway); err != nil { + t.Fatalf("loopback no-token mode should be allowed: %v", err) + } +} + +func TestValidateGatewayAuthAllowsExplicitInsecureOptIn(t *testing.T) { + cfg := Default() + cfg.Gateway.Host = "0.0.0.0" + cfg.Gateway.Token = "" + t.Setenv(GatewayAllowInsecureNoAuthEnv, "1") + + if err := ValidateGatewayAuth(cfg.Gateway); err != nil { + t.Fatalf("explicit insecure opt-in should allow no-token mode: %v", err) + } +} + // --- Env var for API keys --- func TestLoad_EnvVarAPIKeys(t *testing.T) { diff --git a/internal/gateway/router.go b/internal/gateway/router.go index 15eb6928..f3adf908 100644 --- a/internal/gateway/router.go +++ b/internal/gateway/router.go @@ -10,6 +10,7 @@ import ( "github.com/google/uuid" "github.com/nextlevelbuilder/goclaw/internal/cache" + "github.com/nextlevelbuilder/goclaw/internal/config" "github.com/nextlevelbuilder/goclaw/internal/edition" httpapi "github.com/nextlevelbuilder/goclaw/internal/http" "github.com/nextlevelbuilder/goclaw/internal/i18n" @@ -233,7 +234,7 @@ func (r *MethodRouter) handleConnect(ctx context.Context, client *Client, req *p } // Path 2: No token configured → operator (backward compat) - if configToken == "" { + if configToken == "" && config.GatewayNoAuthFallbackAllowed(r.server.cfg.Gateway) { client.role = permissions.RoleOperator client.authenticated = true client.userID = params.UserID @@ -260,7 +261,7 @@ func (r *MethodRouter) handleConnect(ctx context.Context, client *Client, req *p if paired { client.role = permissions.RoleOperator client.authenticated = true - client.userID = params.UserID + client.userID = params.UserID client.pairedSenderID = params.SenderID client.pairedChannel = "browser" tid, errCode := r.resolveTenantHint(ctx, params.TenantHint, params.UserID) diff --git a/internal/gateway/router_test.go b/internal/gateway/router_test.go new file mode 100644 index 00000000..f3d3d7fb --- /dev/null +++ b/internal/gateway/router_test.go @@ -0,0 +1,64 @@ +package gateway + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/nextlevelbuilder/goclaw/internal/config" + "github.com/nextlevelbuilder/goclaw/internal/permissions" + "github.com/nextlevelbuilder/goclaw/pkg/protocol" +) + +func TestHandleConnectRejectsNoTokenExternalBind(t *testing.T) { + cfg := config.Default() + cfg.Gateway.Host = "0.0.0.0" + cfg.Gateway.Token = "" + t.Setenv(config.GatewayAllowInsecureNoAuthEnv, "") + + server := NewServer(cfg, nil, nil, nil) + client := NewClient(nil, server, "203.0.113.10") + req := &protocol.RequestFrame{ID: "req-1", Method: protocol.MethodConnect} + + server.router.Handle(context.Background(), client, req) + + if client.authenticated { + t.Fatal("expected unauthenticated client for external no-token connect") + } + if client.role != "" { + t.Fatalf("role = %q, want empty", client.role) + } + select { + case raw := <-client.send: + var resp protocol.ResponseFrame + if err := json.Unmarshal(raw, &resp); err != nil { + t.Fatalf("unmarshal response: %v", err) + } + if resp.Error == nil || resp.Error.Code != protocol.ErrUnauthorized { + t.Fatalf("response error = %#v, want unauthorized", resp.Error) + } + case <-time.After(500 * time.Millisecond): + t.Fatal("expected unauthorized response") + } +} + +func TestHandleConnectAllowsExplicitInsecureNoTokenOptIn(t *testing.T) { + cfg := config.Default() + cfg.Gateway.Host = "0.0.0.0" + cfg.Gateway.Token = "" + t.Setenv(config.GatewayAllowInsecureNoAuthEnv, "1") + + server := NewServer(cfg, nil, nil, nil) + client := NewClient(nil, server, "127.0.0.1") + req := &protocol.RequestFrame{ID: "req-1", Method: protocol.MethodConnect} + + server.router.Handle(context.Background(), client, req) + + if !client.authenticated { + t.Fatal("expected authenticated client with explicit insecure opt-in") + } + if client.role != permissions.RoleOperator { + t.Fatalf("role = %q, want operator", client.role) + } +} diff --git a/internal/http/auth.go b/internal/http/auth.go index d6edcd5c..bdec54e8 100644 --- a/internal/http/auth.go +++ b/internal/http/auth.go @@ -79,6 +79,7 @@ func extractAgentID(r *http.Request, model string) string { // --- Package-level API key cache for shared auth --- var pkgGatewayToken string +var pkgNoAuthFallbackAllowed = true var pkgAPIKeyCache *apiKeyCache var pkgPairingStore store.PairingStore var pkgTenantCache *tenantCache @@ -90,6 +91,12 @@ func InitGatewayToken(token string) { pkgGatewayToken = token } +// InitGatewayNoAuthFallbackAllowed controls the legacy empty-token local/dev +// fallback after startup config validation. +func InitGatewayNoAuthFallbackAllowed(allowed bool) { + pkgNoAuthFallbackAllowed = allowed +} + // InitAPIKeyCache initializes the shared API key cache with TTL and pubsub invalidation. // Must be called once during server startup before handling requests. func InitAPIKeyCache(s store.APIKeyStore, mb *bus.MessageBus) { @@ -233,8 +240,8 @@ func resolveAuthWithBearer(r *http.Request, bearer string) authResult { slog.Warn("security.http_pairing_auth_failed", "sender_id", senderID, "ip", r.RemoteAddr) } } - // No auth configured → admin (no token = dev/single-user mode, full access) - if pkgGatewayToken == "" { + // No auth configured → admin only when startup allowed local/dev fallback. + if pkgGatewayToken == "" && pkgNoAuthFallbackAllowed { return authResult{Role: permissions.RoleAdmin, Authenticated: true, TenantID: store.MasterTenantID} } return authResult{} diff --git a/internal/http/auth_test.go b/internal/http/auth_test.go index bc2869e8..d305da64 100644 --- a/internal/http/auth_test.go +++ b/internal/http/auth_test.go @@ -36,6 +36,13 @@ func setupTestToken(t *testing.T, token string) { t.Cleanup(func() { pkgGatewayToken = old }) } +func setupTestNoAuthFallback(t *testing.T, allowed bool) { + t.Helper() + old := pkgNoAuthFallbackAllowed + pkgNoAuthFallbackAllowed = allowed + t.Cleanup(func() { pkgNoAuthFallbackAllowed = old }) +} + func setupTestTenantStore(t *testing.T, ts store.TenantStore) { t.Helper() old := pkgTenantCache @@ -221,6 +228,7 @@ func TestResolveAuth_WrongToken(t *testing.T) { func TestResolveAuth_NoAuthConfigured(t *testing.T) { setupTestCache(t, nil) + setupTestNoAuthFallback(t, true) r := httptest.NewRequest("GET", "/v1/agents", nil) @@ -233,6 +241,19 @@ func TestResolveAuth_NoAuthConfigured(t *testing.T) { } } +func TestResolveAuth_NoAuthConfiguredDisallowed(t *testing.T) { + setupTestCache(t, nil) + setupTestToken(t, "") + setupTestNoAuthFallback(t, false) + + r := httptest.NewRequest("GET", "/v1/agents", nil) + + auth := resolveAuth(r) + if auth.Authenticated { + t.Fatal("expected unauthenticated when no-token fallback is disabled") + } +} + func TestResolveAuth_APIKeyReadScope(t *testing.T) { // We need to hash the token the same way crypto.HashAPIKey does // For testing, we'll inject directly into the cache diff --git a/internal/http/files.go b/internal/http/files.go index 4bb860dc..2ed265d0 100644 --- a/internal/http/files.go +++ b/internal/http/files.go @@ -25,6 +25,8 @@ type FilesHandler struct { dataDir string // data directory root for tenant path validation } +var filesAfterOpenHookForTest func(string) + // NewFilesHandler creates a handler that serves files by absolute path. // workspace is the root directory used for fallback generated file search. // dataDir is used for tenant path validation (files must be within tenant's dirs). @@ -54,39 +56,15 @@ func (h *FilesHandler) handleSign(w http.ResponseWriter, r *http.Request) { return } - // Validate path is within workspace or dataDir before signing. - // Defense-in-depth: prevents signing tokens for arbitrary system files. - absPath := filepath.Clean(body.Path) - if !filepath.IsAbs(absPath) { - // Windows drive letter path (e.g. "C:\...") — keep as-is, consistent with handleServe. - if len(absPath) >= 2 && absPath[1] == ':' { - // already absolute on Windows - } else { - absPath = filepath.Clean("/" + absPath) - } - } - sep := string(filepath.Separator) - if (h.workspace == "" || (!strings.HasPrefix(absPath, h.workspace+sep) && absPath != h.workspace)) && - (h.dataDir == "" || (!strings.HasPrefix(absPath, h.dataDir+sep) && absPath != h.dataDir)) { - slog.Warn("security.files_sign_path_denied", "path", absPath, "workspace", h.workspace, "data_dir", h.dataDir) + absPath := absoluteFilePath(body.Path) + file, _, _, ok := h.openValidatedFile(authedReq, absPath, false) + if !ok { http.Error(w, `{"error":"path outside allowed directories"}`, http.StatusForbidden) return } + _ = file.Close() - // Multi-tenant (RBAC): additionally restrict to the requesting tenant's dirs. - // Prevents tenant A from signing a URL for tenant B's files. - if edition.Current().RBACEnabled { - tenantData := config.TenantDataDir(h.dataDir, store.TenantIDFromContext(authedReq.Context()), store.TenantSlugFromContext(authedReq.Context())) - tenantWs := config.TenantWorkspace(h.workspace, store.TenantIDFromContext(authedReq.Context()), store.TenantSlugFromContext(authedReq.Context())) - if (!strings.HasPrefix(absPath, tenantData+sep) && absPath != tenantData) && - (!strings.HasPrefix(absPath, tenantWs+sep) && absPath != tenantWs) { - slog.Warn("security.files_sign_tenant_denied", "path", absPath, "tenant_data", tenantData, "tenant_ws", tenantWs) - http.Error(w, `{"error":"path outside allowed directories"}`, http.StatusForbidden) - return - } - } - - urlPath := "/v1/files/" + strings.TrimPrefix(filepath.Clean(body.Path), "/") + urlPath := fileURLPath(absPath) ft := SignFileToken(urlPath, FileSigningKey(), FileTokenTTL) writeJSON(w, http.StatusOK, map[string]string{ "url": urlPath + "?ft=" + ft, @@ -139,69 +117,20 @@ func (h *FilesHandler) handleServe(w http.ResponseWriter, r *http.Request) { return } - // URL path is the absolute path with leading "/" stripped (e.g. "app/.goclaw/workspace/file.png") - // Windows drive letter: "C:/Users/..." → use directly without prepending "/" - var absPath string - if len(urlPath) >= 2 && urlPath[1] == ':' { - absPath = filepath.Clean(urlPath) - } else { - absPath = filepath.Clean("/" + urlPath) - } + absPath := absoluteFilePath(urlPath) // Block access to sensitive system directories - for _, prefix := range deniedFilePrefixes { - if strings.HasPrefix(absPath, prefix) { - slog.Warn("security.files_denied_path", "path", absPath) - http.Error(w, i18n.T(locale, i18n.MsgInvalidPath), http.StatusForbidden) - return - } + if hasDeniedFilePrefix(absPath) { + slog.Warn("security.files_denied_path", "path", absPath) + http.Error(w, i18n.T(locale, i18n.MsgInvalidPath), http.StatusForbidden) + return } - // Defense-in-depth: validate workspace/dataDir boundary even for signed file tokens. - // The token cryptographically binds the URL path, but we also verify the resolved - // absolute path stays within allowed directories to limit blast radius of any - // bug in the signing flow. - if r.URL.Query().Get("ft") != "" { - sep := string(filepath.Separator) - inWorkspace := h.workspace != "" && (strings.HasPrefix(absPath, h.workspace+sep) || absPath == h.workspace) - inDataDir := h.dataDir != "" && (strings.HasPrefix(absPath, h.dataDir+sep) || absPath == h.dataDir) - if !inWorkspace && !inDataDir { - slog.Warn("security.files_ft_path_denied", "path", absPath, "workspace", h.workspace, "data_dir", h.dataDir) - http.NotFound(w, r) - return - } - } - - // Path isolation: validate file path is within allowed directories. - if r.URL.Query().Get("ft") == "" { - allowed := false - - // Always allow files within workspace root and data dir root. - // These are the two top-level directories that contain all user files. - sep := string(filepath.Separator) - if h.workspace != "" && (strings.HasPrefix(absPath, h.workspace+sep) || absPath == h.workspace) { - allowed = true - } - if !allowed && h.dataDir != "" && (strings.HasPrefix(absPath, h.dataDir+sep) || absPath == h.dataDir) { - allowed = true - } - - // Multi-tenant (standard edition): additionally restrict to tenant-scoped subdirectories. - if allowed && edition.Current().RBACEnabled { - tenantData := config.TenantDataDir(h.dataDir, store.TenantIDFromContext(r.Context()), store.TenantSlugFromContext(r.Context())) - tenantWs := h.tenantWorkspace(r) - if !strings.HasPrefix(absPath, tenantData+sep) && - !strings.HasPrefix(absPath, tenantWs+sep) && - absPath != tenantData && absPath != tenantWs { - allowed = false - } - } - - if !allowed { - slog.Warn("security.files_path_denied", "path", absPath, "workspace", h.workspace, "data_dir", h.dataDir) - http.NotFound(w, r) - return - } + signed := r.URL.Query().Get("ft") != "" + if !h.lexicallyAllowsFilePath(r, absPath, signed) { + slog.Warn("security.files_path_denied", "path", absPath, "workspace", h.workspace, "data_dir", h.dataDir) + http.NotFound(w, r) + return } info, err := os.Stat(absPath) @@ -221,7 +150,7 @@ func (h *FilesHandler) handleServe(w http.ResponseWriter, r *http.Request) { // For ft= signed requests, the path is cryptographically bound — no fallback search. // Searching the global workspace could cross tenant boundaries if a same-basename // file exists in another tenant's directory. - if r.URL.Query().Get("ft") != "" { + if signed { http.NotFound(w, r) return } @@ -237,6 +166,13 @@ func (h *FilesHandler) handleServe(w http.ResponseWriter, r *http.Request) { return } } + file, realPath, fileInfo, ok := h.openValidatedFile(r, absPath, signed) + if !ok { + http.NotFound(w, r) + return + } + defer file.Close() + absPath = realPath // Set Content-Type from extension ext := filepath.Ext(absPath) @@ -250,7 +186,146 @@ func (h *FilesHandler) handleServe(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename="%s"`, filepath.Base(absPath))) } - http.ServeFile(w, r, absPath) + http.ServeContent(w, r, filepath.Base(absPath), fileInfo.ModTime(), file) +} + +func absoluteFilePath(path string) string { + absPath := filepath.Clean(path) + if filepath.IsAbs(absPath) { + return absPath + } + // Windows drive letter path (e.g. "C:\...") is absolute to this handler. + if len(absPath) >= 2 && absPath[1] == ':' { + return absPath + } + return filepath.Clean(string(filepath.Separator) + absPath) +} + +func fileURLPath(absPath string) string { + return "/v1/files/" + strings.TrimPrefix(filepath.Clean(absPath), string(filepath.Separator)) +} + +func hasDeniedFilePrefix(path string) bool { + cleaned := filepath.Clean(path) + for _, prefix := range deniedFilePrefixes { + root := filepath.Clean(prefix) + if pathWithinDir(cleaned, root) { + return true + } + } + return false +} + +func configuredFileRoot(root string) string { + if root == "" { + return "" + } + return filepath.Clean(root) +} + +func canonicalFileRoots(roots []string) []string { + out := make([]string, 0, len(roots)) + for _, root := range roots { + if root = configuredFileRoot(root); root != "" { + out = append(out, evalSymlinkOrClean(root)) + } + } + return out +} + +func (h *FilesHandler) requestFileRoots(r *http.Request, signed bool, absPath string) []string { + if signed { + return []string{ + inferredScopedFileRoot(h.workspace, absPath), + inferredScopedFileRoot(h.dataDir, absPath), + } + } + if edition.Current().RBACEnabled { + return []string{ + config.TenantWorkspace(h.workspace, store.TenantIDFromContext(r.Context()), store.TenantSlugFromContext(r.Context())), + config.TenantDataDir(h.dataDir, store.TenantIDFromContext(r.Context()), store.TenantSlugFromContext(r.Context())), + } + } + return []string{h.workspace, h.dataDir} +} + +func inferredScopedFileRoot(base, absPath string) string { + base = configuredFileRoot(base) + if base == "" || !pathWithinDir(filepath.Clean(absPath), base) { + return "" + } + tenantsRoot := filepath.Join(base, "tenants") + if !pathWithinDir(filepath.Clean(absPath), tenantsRoot) || filepath.Clean(absPath) == tenantsRoot { + return base + } + rel, err := filepath.Rel(tenantsRoot, filepath.Clean(absPath)) + if err != nil { + return "" + } + first, _, _ := strings.Cut(rel, string(filepath.Separator)) + if first == "" || first == "." || first == ".." { + return "" + } + return filepath.Join(tenantsRoot, first) +} + +func filePathWithinAnyRoot(path string, roots []string) bool { + for _, root := range roots { + if root != "" && pathWithinDir(filepath.Clean(path), filepath.Clean(root)) { + return true + } + } + return false +} + +func (h *FilesHandler) lexicallyAllowsFilePath(r *http.Request, absPath string, signed bool) bool { + return filePathWithinAnyRoot(absPath, h.requestFileRoots(r, signed, absPath)) +} + +func (h *FilesHandler) openValidatedFile(r *http.Request, absPath string, signed bool) (*os.File, string, os.FileInfo, bool) { + file, err := os.Open(absPath) + if err != nil { + return nil, "", nil, false + } + if filesAfterOpenHookForTest != nil { + filesAfterOpenHookForTest(absPath) + } + + realPath, err := filepath.EvalSymlinks(absPath) + if err != nil { + slog.Warn("security.files_path_unresolved", "path", absPath, "error", err) + _ = file.Close() + return nil, "", nil, false + } + realPath = filepath.Clean(realPath) + if hasDeniedFilePrefix(realPath) { + slog.Warn("security.files_realpath_denied", "path", absPath, "resolved", realPath) + _ = file.Close() + return nil, "", nil, false + } + roots := canonicalFileRoots(h.requestFileRoots(r, signed, absPath)) + if !filePathWithinAnyRoot(realPath, roots) { + slog.Warn("security.files_realpath_escape", "path", absPath, "resolved", realPath, "roots", roots) + _ = file.Close() + return nil, "", nil, false + } + realInfo, err := os.Stat(realPath) + if err != nil { + _ = file.Close() + return nil, "", nil, false + } + fileInfo, err := file.Stat() + if err != nil { + _ = file.Close() + slog.Warn("security.files_open_race", "path", realPath, "error", err) + return nil, "", nil, false + } + if fileInfo.IsDir() || realInfo.IsDir() || !os.SameFile(realInfo, fileInfo) { + _ = file.Close() + slog.Warn("security.files_open_race", "path", realPath) + return nil, "", nil, false + } + return file, realPath, fileInfo, true } // tenantWorkspace resolves the workspace scoped to the requesting tenant. @@ -339,4 +414,3 @@ func isNumeric(s string) bool { } return len(s) > 0 } - diff --git a/internal/http/files_path_security_test.go b/internal/http/files_path_security_test.go index fed192ab..55bc7311 100644 --- a/internal/http/files_path_security_test.go +++ b/internal/http/files_path_security_test.go @@ -5,6 +5,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "strconv" "strings" "testing" ) @@ -141,6 +142,94 @@ func TestFilesHandleServe_FileOutsideAllDirs_WithToken_Returns404(t *testing.T) } } +func TestFilesHandleServe_SignedSymlinkEscape_Returns404(t *testing.T) { + h, workspace := makeTestFilesHandler(t) + outsideDir := t.TempDir() + target := filepath.Join(outsideDir, "secret.txt") + if err := os.WriteFile(target, []byte("secret"), 0644); err != nil { + t.Fatal(err) + } + linkPath := filepath.Join(workspace, "link.txt") + if err := os.Symlink(target, linkPath); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + + urlPath := "/v1/files/" + strings.TrimPrefix(filepath.Clean(linkPath), "/") + ft := SignFileToken(urlPath, FileSigningKey(), FileTokenTTL) + + mux := http.NewServeMux() + mux.HandleFunc("GET /v1/files/{path...}", h.handleServe) + + req := httptest.NewRequest(http.MethodGet, urlPath+"?ft="+ft, nil) + w := httptest.NewRecorder() + mux.ServeHTTP(w, req) + + if w.Code == http.StatusOK { + t.Fatal("signed symlink escaping workspace should not be served") + } +} + +func TestFilesHandleServe_OpenThenSwapToSymlinkEscape_Returns404(t *testing.T) { + h, workspace := makeTestFilesHandler(t) + outsideDir := t.TempDir() + secretPath := filepath.Join(outsideDir, "secret.txt") + if err := os.WriteFile(secretPath, []byte("secret"), 0644); err != nil { + t.Fatal(err) + } + filePath := filepath.Join(workspace, "race.txt") + if err := os.WriteFile(filePath, []byte("allowed"), 0644); err != nil { + t.Fatal(err) + } + filesAfterOpenHookForTest = func(opened string) { + if opened != filePath { + return + } + _ = os.Remove(filePath) + _ = os.Symlink(secretPath, filePath) + } + defer func() { filesAfterOpenHookForTest = nil }() + + urlPath := "/v1/files/" + strings.TrimPrefix(filepath.Clean(filePath), "/") + ft := SignFileToken(urlPath, FileSigningKey(), FileTokenTTL) + + mux := http.NewServeMux() + mux.HandleFunc("GET /v1/files/{path...}", h.handleServe) + + req := httptest.NewRequest(http.MethodGet, urlPath+"?ft="+ft, nil) + w := httptest.NewRecorder() + mux.ServeHTTP(w, req) + + if w.Code == http.StatusOK { + t.Fatal("file swapped to escaping symlink after open should not be served") + } + if strings.Contains(w.Body.String(), "secret") { + t.Fatal("response leaked swapped outside file content") + } +} + +func TestFilesHandleSign_SymlinkEscape_ReturnsForbidden(t *testing.T) { + setupTestToken(t, "") + setupTestNoAuthFallback(t, true) + h, workspace := makeTestFilesHandler(t) + outsideDir := t.TempDir() + target := filepath.Join(outsideDir, "secret.txt") + if err := os.WriteFile(target, []byte("secret"), 0644); err != nil { + t.Fatal(err) + } + linkPath := filepath.Join(workspace, "link.txt") + if err := os.Symlink(target, linkPath); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + + req := httptest.NewRequest(http.MethodPost, "/v1/files/sign", strings.NewReader(`{"path":`+strconv.Quote(linkPath)+`}`)) + w := httptest.NewRecorder() + h.handleSign(w, req) + + if w.Code == http.StatusOK { + t.Fatal("sign endpoint should reject symlinks escaping allowed roots") + } +} + // ---- handleServe: empty path ---- func TestFilesHandleServe_EmptyPath_Returns400(t *testing.T) { diff --git a/internal/http/openapi_spec.json b/internal/http/openapi_spec.json index bbe02cea..bf57607a 100644 --- a/internal/http/openapi_spec.json +++ b/internal/http/openapi_spec.json @@ -2,7 +2,7 @@ "openapi": "3.0.3", "info": { "title": "GoClaw Gateway API", - "description": "PostgreSQL multi-tenant AI agent gateway with WebSocket RPC + HTTP API.\n\n## Authentication\n\nAll endpoints require a Bearer token in the `Authorization` header:\n\n```\nAuthorization: Bearer \n```\n\nYou can use either the **gateway token** (grants admin access) or an **API key** created via the API Keys endpoints (grants scoped access).\n\nIf no token is configured on the server, authentication is disabled (backward compatibility).\n\n## Common Headers\n\n| Header | Description |\n|--------|-------------|\n| `X-GoClaw-User-Id` | External user ID for multi-tenant context |\n| `X-GoClaw-Agent-Id` | Target agent ID (alternative to model prefix) |\n| `X-GoClaw-Tenant-Id` | Tenant scope — UUID or slug (gateway token / cross-tenant API keys) |\n| `Accept-Language` | Locale for error messages (`en`, `vi`, `zh`) |\n\n## WebSocket Protocol\n\nConnect via `POST /ws` (upgrade). Protocol v3 uses frame types: `req`, `res`, `event`.\nFirst request must be `connect` with `{\"token\": \"...\", \"user_id\": \"...\", \"locale\": \"en\"}`.", + "description": "PostgreSQL multi-tenant AI agent gateway with WebSocket RPC + HTTP API.\n\n## Authentication\n\nAll endpoints require a Bearer token in the `Authorization` header:\n\n```\nAuthorization: Bearer \n```\n\nYou can use either the **gateway token** (grants admin access) or an **API key** created via the API Keys endpoints (grants scoped access).\n\nIf no gateway token is configured, empty-token admin access is limited to loopback local development or explicit `GOCLAW_ALLOW_INSECURE_NO_AUTH=1`; externally reachable deployments require `GOCLAW_GATEWAY_TOKEN`.\n\n## Common Headers\n\n| Header | Description |\n|--------|-------------|\n| `X-GoClaw-User-Id` | External user ID for multi-tenant context |\n| `X-GoClaw-Agent-Id` | Target agent ID (alternative to model prefix) |\n| `X-GoClaw-Tenant-Id` | Tenant scope — UUID or slug (gateway token / cross-tenant API keys) |\n| `Accept-Language` | Locale for error messages (`en`, `vi`, `zh`) |\n\n## WebSocket Protocol\n\nConnect via `POST /ws` (upgrade). Protocol v3 uses frame types: `req`, `res`, `event`.\nFirst request must be `connect` with `{\"token\": \"...\", \"user_id\": \"...\", \"locale\": \"en\"}`.", "version": "0.2.0", "contact": { "name": "GoClaw", diff --git a/internal/http/storage.go b/internal/http/storage.go index 0014e666..e5f86927 100644 --- a/internal/http/storage.go +++ b/internal/http/storage.go @@ -34,30 +34,48 @@ type sizeCacheEntry struct { type StorageHandler struct { baseDir string // global data dir (resolved absolute path to ~/.goclaw/) + tenants store.TenantStore // sizeCache caches the total storage size per tenant for 60 minutes. sizeCache sync.Map // tenantBaseDir (string) → *sizeCacheEntry } // NewStorageHandler creates a handler for workspace storage management. -func NewStorageHandler(baseDir string) *StorageHandler { - return &StorageHandler{baseDir: baseDir} +func NewStorageHandler(baseDir string, tenants ...store.TenantStore) *StorageHandler { + h := &StorageHandler{baseDir: baseDir} + if len(tenants) > 0 { + h.tenants = tenants[0] + } + return h } // RegisterRoutes registers storage management routes on the given mux. func (h *StorageHandler) RegisterRoutes(mux *http.ServeMux) { mux.HandleFunc("GET /v1/storage/files", h.auth(h.handleList)) mux.HandleFunc("GET /v1/storage/files/{path...}", h.auth(h.handleRead)) - mux.HandleFunc("DELETE /v1/storage/files/{path...}", h.auth(h.handleDelete)) + mux.HandleFunc("DELETE /v1/storage/files/{path...}", requireAuth(permissions.RoleAdmin, h.requireTenantAdmin(h.handleDelete))) mux.HandleFunc("GET /v1/storage/size", h.auth(h.handleSize)) - mux.HandleFunc("POST /v1/storage/files", requireAuth(permissions.RoleAdmin, h.handleUpload)) - mux.HandleFunc("PUT /v1/storage/move", requireAuth(permissions.RoleAdmin, h.handleMove)) + mux.HandleFunc("POST /v1/storage/files", requireAuth(permissions.RoleAdmin, h.requireTenantAdmin(h.handleUpload))) + mux.HandleFunc("PUT /v1/storage/move", requireAuth(permissions.RoleAdmin, h.requireTenantAdmin(h.handleMove))) } func (h *StorageHandler) auth(next http.HandlerFunc) http.HandlerFunc { return requireAuth("", next) } +func (h *StorageHandler) requireTenantAdmin(next http.HandlerFunc) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + if pkgGatewayToken == "" && store.TenantIDFromContext(r.Context()) == store.MasterTenantID { + next(w, r) + return + } + if !requireTenantAdmin(w, r, h.tenants) { + return + } + next(w, r) + } +} + // tenantBaseDir resolves the data directory scoped to the requesting tenant. // Master tenant returns the global baseDir (backward compat). func (h *StorageHandler) tenantBaseDir(r *http.Request) string { @@ -102,6 +120,75 @@ func (h *StorageHandler) isHiddenPath(r *http.Request, rel string) bool { return strings.EqualFold(topLevelPath(rel), "tenants") } +func pathWithinDir(path, dir string) bool { + rel, err := filepath.Rel(dir, path) + if err != nil { + return false + } + return rel == "." || (rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator))) +} + +func evalSymlinkOrClean(path string) string { + realPath, err := filepath.EvalSymlinks(path) + if err == nil { + return filepath.Clean(realPath) + } + return filepath.Clean(path) +} + +func (h *StorageHandler) isHiddenRealPath(r *http.Request, base, realPath string) bool { + if store.TenantIDFromContext(r.Context()) != store.MasterTenantID { + return false + } + realTenantRoot, err := filepath.EvalSymlinks(filepath.Join(base, "tenants")) + if err != nil { + return false + } + return pathWithinDir(filepath.Clean(realPath), filepath.Clean(realTenantRoot)) +} + +func (h *StorageHandler) validateExistingStoragePath(r *http.Request, base, absPath string) bool { + realBase := evalSymlinkOrClean(base) + realPath, err := filepath.EvalSymlinks(absPath) + if err != nil { + return false + } + realPath = filepath.Clean(realPath) + if !pathWithinDir(realPath, realBase) { + slog.Warn("security.storage_symlink_escape", "resolved", realPath, "base", realBase) + return false + } + if h.isHiddenRealPath(r, base, realPath) { + slog.Warn("security.storage_hidden_symlink_path", "resolved", realPath, "base", realBase) + return false + } + return true +} + +func (h *StorageHandler) validateStorageParent(r *http.Request, base, parent string) bool { + realBase := evalSymlinkOrClean(base) + current := filepath.Clean(parent) + for { + if realParent, err := filepath.EvalSymlinks(current); err == nil { + realParent = filepath.Clean(realParent) + if !pathWithinDir(realParent, realBase) { + slog.Warn("security.storage_parent_escape", "resolved", realParent, "base", realBase) + return false + } + if h.isHiddenRealPath(r, base, realParent) { + slog.Warn("security.storage_hidden_parent", "resolved", realParent, "base", realBase) + return false + } + return true + } + next := filepath.Dir(current) + if next == current { + return false + } + current = next + } +} + // handleList lists files and directories under ~/.goclaw/ with depth limiting. // Query params: // - ?path= scopes the listing to a subtree @@ -349,6 +436,10 @@ func (h *StorageHandler) handleRead(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidPath)}) return } + if !h.validateExistingStoragePath(r, readBase, absPath) { + writeJSON(w, http.StatusNotFound, map[string]string{"error": i18n.T(locale, i18n.MsgFileNotFound)}) + return + } data, err := os.ReadFile(absPath) if err != nil { @@ -413,6 +504,10 @@ func (h *StorageHandler) handleDelete(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusNotFound, map[string]string{"error": i18n.T(locale, i18n.MsgNotFound, "path", relPath)}) return } + if !h.validateExistingStoragePath(r, delBase, absPath) { + writeJSON(w, http.StatusNotFound, map[string]string{"error": i18n.T(locale, i18n.MsgNotFound, "path", relPath)}) + return + } if info.Mode()&os.ModeSymlink != 0 { // Remove symlink itself, not target @@ -494,41 +589,46 @@ func (h *StorageHandler) handleUpload(w http.ResponseWriter, r *http.Request) { } } + if !h.validateStorageParent(r, base, targetDir) { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidPath)}) + return + } if err := os.MkdirAll(targetDir, 0750); err != nil { slog.Error("storage.upload_mkdir_failed", "dir", targetDir, "error", err) writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, "failed to create directory")}) return } - - diskPath := filepath.Join(targetDir, origName) - - // Symlink escape check on resolved path. - realTarget, _ := filepath.EvalSymlinks(targetDir) - if realTarget == "" { - realTarget = targetDir - } - realBase, _ := filepath.EvalSymlinks(base) - if realBase == "" { - realBase = base - } - if !strings.HasPrefix(realTarget, realBase) { - slog.Warn("security.storage_upload_symlink_escape", "target", realTarget, "base", realBase) + if !h.validateStorageParent(r, base, targetDir) { writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidPath)}) return } - // Write file. - out, err := os.Create(diskPath) + diskPath := filepath.Join(targetDir, origName) + + out, err := os.CreateTemp(targetDir, ".upload-*") if err != nil { - slog.Error("storage.upload_create_failed", "path", diskPath, "error", err) + slog.Error("storage.upload_create_failed", "dir", targetDir, "error", err) writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, "failed to save file")}) return } - defer out.Close() + tmpPath := out.Name() + defer os.Remove(tmpPath) written, err := io.Copy(out, file) if err != nil { - os.Remove(diskPath) + out.Close() + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, "failed to save file")}) + return + } + if err := out.Close(); err != nil { + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, "failed to save file")}) + return + } + if !h.validateStorageParent(r, base, targetDir) { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidPath)}) + return + } + if err := os.Rename(tmpPath, diskPath); err != nil { writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, "failed to save file")}) return } @@ -588,15 +688,18 @@ func (h *StorageHandler) handleMove(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusNotFound, map[string]string{"error": i18n.T(locale, i18n.MsgFileNotFound)}) return } - baseReal, _ := filepath.EvalSymlinks(base) - if baseReal == "" { - baseReal = base - } - if !strings.HasPrefix(srcReal, baseReal+string(filepath.Separator)) { + baseReal := evalSymlinkOrClean(base) + srcReal = filepath.Clean(srcReal) + if !pathWithinDir(srcReal, baseReal) { slog.Warn("security.storage_move_src_escape", "resolved", srcReal, "base", baseReal) writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidPath)}) return } + if h.isHiddenRealPath(r, base, srcReal) { + slog.Warn("security.storage_move_hidden_src", "resolved", srcReal, "base", baseReal) + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidPath)}) + return + } // Resolve and validate destination path. destAbs := filepath.Join(base, filepath.Clean(toRel)) @@ -606,12 +709,7 @@ func (h *StorageHandler) handleMove(w http.ResponseWriter, r *http.Request) { } // Ensure destination parent exists. destDir := filepath.Dir(destAbs) - destDirReal, _ := filepath.EvalSymlinks(destDir) - if destDirReal == "" { - destDirReal = destDir - } - if !strings.HasPrefix(destDirReal+string(filepath.Separator), baseReal+string(filepath.Separator)) { - slog.Warn("security.storage_move_dest_escape", "resolved", destDirReal, "base", baseReal) + if !h.validateStorageParent(r, base, destDir) { writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidPath)}) return } @@ -620,6 +718,10 @@ func (h *StorageHandler) handleMove(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusInternalServerError, map[string]string{"error": i18n.T(locale, i18n.MsgInternalError, "failed to create directory")}) return } + if !h.validateStorageParent(r, base, destDir) { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": i18n.T(locale, i18n.MsgInvalidPath)}) + return + } // Prevent overwriting existing file. if _, err := os.Stat(destAbs); err == nil { diff --git a/internal/http/storage_test.go b/internal/http/storage_test.go index 905ff709..4e758c25 100644 --- a/internal/http/storage_test.go +++ b/internal/http/storage_test.go @@ -3,6 +3,7 @@ package http import ( "context" "encoding/json" + "mime/multipart" "net/http" "net/http/httptest" "os" @@ -88,6 +89,52 @@ func TestStorageReadTenantRootReturnsNotFoundForMaster(t *testing.T) { } } +func TestStorageReadRejectsSymlinkedTenantParentForMaster(t *testing.T) { + baseDir := t.TempDir() + tenantSecret := filepath.Join(baseDir, "tenants", "tenant-a", "secret.txt") + writeStorageTestFile(t, tenantSecret, "tenant-secret") + if err := os.Symlink(filepath.Join(baseDir, "tenants"), filepath.Join(baseDir, "tenant-link")); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + + handler := NewStorageHandler(baseDir) + req := httptest.NewRequest("GET", "/v1/storage/files/tenant-link/tenant-a/secret.txt", nil) + req = req.WithContext(store.WithTenantID(context.Background(), store.MasterTenantID)) + req.SetPathValue("path", "tenant-link/tenant-a/secret.txt") + w := httptest.NewRecorder() + + handler.handleRead(w, req) + if w.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404", w.Code) + } + if strings.Contains(w.Body.String(), "tenant-secret") { + t.Fatal("response leaked tenant secret through symlinked parent") + } +} + +func TestStorageDeleteRejectsSymlinkedTenantParentForMaster(t *testing.T) { + baseDir := t.TempDir() + tenantSecret := filepath.Join(baseDir, "tenants", "tenant-a", "secret.txt") + writeStorageTestFile(t, tenantSecret, "tenant-secret") + if err := os.Symlink(filepath.Join(baseDir, "tenants"), filepath.Join(baseDir, "tenant-link")); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + + handler := NewStorageHandler(baseDir) + req := httptest.NewRequest(http.MethodDelete, "/v1/storage/files/tenant-link/tenant-a/secret.txt", nil) + req = req.WithContext(store.WithTenantID(context.Background(), store.MasterTenantID)) + req.SetPathValue("path", "tenant-link/tenant-a/secret.txt") + w := httptest.NewRecorder() + + handler.handleDelete(w, req) + if w.Code != http.StatusNotFound { + t.Fatalf("status = %d, want 404", w.Code) + } + if _, err := os.Stat(tenantSecret); err != nil { + t.Fatalf("tenant secret should not be deleted through symlinked parent: %v", err) + } +} + func TestStorageSizeExcludesTenantRootForMaster(t *testing.T) { baseDir := t.TempDir() writeStorageTestFile(t, filepath.Join(baseDir, "master.txt"), "12345") @@ -221,3 +268,167 @@ func TestStorageMoveInvalidatesSizeCache(t *testing.T) { t.Fatal("expected size cache entry to be invalidated after move") } } + +func TestStorageMoveRejectsSymlinkedTenantDestinationParent(t *testing.T) { + baseDir := t.TempDir() + writeStorageTestFile(t, filepath.Join(baseDir, "from.txt"), "abc") + writeStorageTestFile(t, filepath.Join(baseDir, "tenants", "tenant-a", ".keep"), "") + if err := os.Symlink(filepath.Join(baseDir, "tenants", "tenant-a"), filepath.Join(baseDir, "tenant-link")); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + + handler := NewStorageHandler(baseDir) + req := httptest.NewRequest(http.MethodPut, "/v1/storage/move?from=from.txt&to=tenant-link/moved.txt", nil) + req = req.WithContext(store.WithTenantID(context.Background(), store.MasterTenantID)) + w := httptest.NewRecorder() + + handler.handleMove(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400", w.Code) + } + if _, err := os.Stat(filepath.Join(baseDir, "from.txt")); err != nil { + t.Fatalf("source should remain after rejected move: %v", err) + } + if _, err := os.Stat(filepath.Join(baseDir, "tenants", "tenant-a", "moved.txt")); !os.IsNotExist(err) { + t.Fatalf("destination should not be created through symlinked parent, err=%v", err) + } +} + +func TestStorageUploadRejectsSymlinkedTenantDestinationParent(t *testing.T) { + baseDir := t.TempDir() + writeStorageTestFile(t, filepath.Join(baseDir, "tenants", "tenant-a", ".keep"), "") + if err := os.Symlink(filepath.Join(baseDir, "tenants", "tenant-a"), filepath.Join(baseDir, "tenant-link")); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + + handler := NewStorageHandler(baseDir) + req := newStorageUploadRequest(t, "/v1/storage/files?path=tenant-link", "file", "x.txt", "data") + req = req.WithContext(store.WithTenantID(context.Background(), store.MasterTenantID)) + w := httptest.NewRecorder() + + handler.handleUpload(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400", w.Code) + } + if _, err := os.Stat(filepath.Join(baseDir, "tenants", "tenant-a", "x.txt")); !os.IsNotExist(err) { + t.Fatalf("upload should not write through symlinked parent, err=%v", err) + } +} + +func TestStorageUploadReplacesLeafSymlinkWithoutFollowingTarget(t *testing.T) { + baseDir := t.TempDir() + tenantSecret := filepath.Join(baseDir, "tenants", "tenant-a", "secret.txt") + writeStorageTestFile(t, tenantSecret, "tenant-secret") + leaf := filepath.Join(baseDir, "x.txt") + if err := os.Symlink(tenantSecret, leaf); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + + handler := NewStorageHandler(baseDir) + req := newStorageUploadRequest(t, "/v1/storage/files", "file", "x.txt", "replacement") + req = req.WithContext(store.WithTenantID(context.Background(), store.MasterTenantID)) + w := httptest.NewRecorder() + + handler.handleUpload(w, req) + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", w.Code) + } + tenantData, err := os.ReadFile(tenantSecret) + if err != nil { + t.Fatalf("read tenant secret: %v", err) + } + if string(tenantData) != "tenant-secret" { + t.Fatalf("tenant secret overwritten through leaf symlink: %q", tenantData) + } + info, err := os.Lstat(leaf) + if err != nil { + t.Fatalf("lstat uploaded leaf: %v", err) + } + if info.Mode()&os.ModeSymlink != 0 { + t.Fatal("upload should replace the leaf symlink itself") + } + uploaded, err := os.ReadFile(leaf) + if err != nil { + t.Fatalf("read uploaded file: %v", err) + } + if string(uploaded) != "replacement" { + t.Fatalf("uploaded content = %q, want replacement", uploaded) + } +} + +func TestStorageMutationsRequireTenantAdmin(t *testing.T) { + setupTestToken(t, "gateway-token") + setupTestNoAuthFallback(t, false) + ts := newMockTenantStore() + tenantID := uuid.New() + ts.addTenant(tenantID, "acme") + ts.setUserRole(tenantID, "viewer-user", store.TenantRoleViewer) + ts.setUserRole(tenantID, "admin-user", store.TenantRoleAdmin) + setupTestTenantStore(t, ts) + + baseDir := t.TempDir() + writeStorageTestFile(t, filepath.Join(baseDir, "tenants", "acme", "from.txt"), "abc") + + handler := NewStorageHandler(baseDir, ts) + mux := http.NewServeMux() + handler.RegisterRoutes(mux) + + viewerUpload := newStorageUploadRequest(t, "/v1/storage/files", "file", "x.txt", "data") + viewerUpload.Header.Set("Authorization", "Bearer gateway-token") + viewerUpload.Header.Set("X-GoClaw-User-Id", "viewer-user") + viewerUpload.Header.Set("X-GoClaw-Tenant-Id", "acme") + viewerUploadRR := httptest.NewRecorder() + mux.ServeHTTP(viewerUploadRR, viewerUpload) + if viewerUploadRR.Code != http.StatusForbidden { + t.Fatalf("viewer upload status = %d, want 403", viewerUploadRR.Code) + } + + viewerMove := httptest.NewRequest(http.MethodPut, "/v1/storage/move?from=from.txt&to=to.txt", nil) + viewerMove.Header.Set("Authorization", "Bearer gateway-token") + viewerMove.Header.Set("X-GoClaw-User-Id", "viewer-user") + viewerMove.Header.Set("X-GoClaw-Tenant-Id", "acme") + viewerMoveRR := httptest.NewRecorder() + mux.ServeHTTP(viewerMoveRR, viewerMove) + if viewerMoveRR.Code != http.StatusForbidden { + t.Fatalf("viewer move status = %d, want 403", viewerMoveRR.Code) + } + + viewerDelete := httptest.NewRequest(http.MethodDelete, "/v1/storage/files/from.txt", nil) + viewerDelete.Header.Set("Authorization", "Bearer gateway-token") + viewerDelete.Header.Set("X-GoClaw-User-Id", "viewer-user") + viewerDelete.Header.Set("X-GoClaw-Tenant-Id", "acme") + viewerDeleteRR := httptest.NewRecorder() + mux.ServeHTTP(viewerDeleteRR, viewerDelete) + if viewerDeleteRR.Code != http.StatusForbidden { + t.Fatalf("viewer delete status = %d, want 403", viewerDeleteRR.Code) + } + + adminUpload := newStorageUploadRequest(t, "/v1/storage/files", "file", "admin.txt", "data") + adminUpload.Header.Set("Authorization", "Bearer gateway-token") + adminUpload.Header.Set("X-GoClaw-User-Id", "admin-user") + adminUpload.Header.Set("X-GoClaw-Tenant-Id", "acme") + adminUploadRR := httptest.NewRecorder() + mux.ServeHTTP(adminUploadRR, adminUpload) + if adminUploadRR.Code != http.StatusOK { + t.Fatalf("tenant admin upload status = %d, want 200: %s", adminUploadRR.Code, adminUploadRR.Body.String()) + } +} + +func newStorageUploadRequest(t *testing.T, target, field, filename, content string) *http.Request { + t.Helper() + var body strings.Builder + writer := multipart.NewWriter(&body) + part, err := writer.CreateFormFile(field, filename) + if err != nil { + t.Fatalf("CreateFormFile: %v", err) + } + if _, err := part.Write([]byte(content)); err != nil { + t.Fatalf("write multipart content: %v", err) + } + if err := writer.Close(); err != nil { + t.Fatalf("close multipart writer: %v", err) + } + req := httptest.NewRequest(http.MethodPost, target, strings.NewReader(body.String())) + req.Header.Set("Content-Type", writer.FormDataContentType()) + return req +} diff --git a/internal/http/tts_config.go b/internal/http/tts_config.go index a576cc54..621b388f 100644 --- a/internal/http/tts_config.go +++ b/internal/http/tts_config.go @@ -22,17 +22,35 @@ import ( type TTSConfigHandler struct { systemConfigs store.SystemConfigStore configSecrets store.ConfigSecretsStore + tenants store.TenantStore } // NewTTSConfigHandler creates a handler for per-tenant TTS config. -func NewTTSConfigHandler(sc store.SystemConfigStore, cs store.ConfigSecretsStore) *TTSConfigHandler { - return &TTSConfigHandler{systemConfigs: sc, configSecrets: cs} +func NewTTSConfigHandler(sc store.SystemConfigStore, cs store.ConfigSecretsStore, tenants ...store.TenantStore) *TTSConfigHandler { + h := &TTSConfigHandler{systemConfigs: sc, configSecrets: cs} + if len(tenants) > 0 { + h.tenants = tenants[0] + } + return h } // RegisterRoutes wires TTS config endpoints onto mux with RoleAdmin auth. func (h *TTSConfigHandler) RegisterRoutes(mux *http.ServeMux) { - mux.HandleFunc("GET /v1/tts/config", requireAuth(permissions.RoleAdmin, h.handleGet)) - mux.HandleFunc("POST /v1/tts/config", requireAuth(permissions.RoleAdmin, h.handleSave)) + mux.HandleFunc("GET /v1/tts/config", requireAuth(permissions.RoleAdmin, h.requireTenantAdmin(h.handleGet))) + mux.HandleFunc("POST /v1/tts/config", requireAuth(permissions.RoleAdmin, h.requireTenantAdmin(h.handleSave))) +} + +func (h *TTSConfigHandler) requireTenantAdmin(next http.HandlerFunc) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + if pkgGatewayToken == "" && store.TenantIDFromContext(r.Context()) == store.MasterTenantID { + next(w, r) + return + } + if !requireTenantAdmin(w, r, h.tenants) { + return + } + next(w, r) + } } // ttsConfigResponse is the response for GET /v1/tts/config. @@ -60,8 +78,8 @@ type ttsProviderConfigResponse struct { GroupID string `json:"group_id,omitempty"` Enabled *bool `json:"enabled,omitempty"` Rate string `json:"rate,omitempty"` - Speakers string `json:"speakers,omitempty"` // JSON-encoded []SpeakerVoice (Gemini multi-speaker) - Params map[string]any `json:"params,omitempty"` // provider-specific params blob + Speakers string `json:"speakers,omitempty"` // JSON-encoded []SpeakerVoice (Gemini multi-speaker) + Params map[string]any `json:"params,omitempty"` // provider-specific params blob } // handleGet returns TTS config for the current tenant. diff --git a/internal/http/tts_config_test.go b/internal/http/tts_config_test.go index f50ebe63..53881327 100644 --- a/internal/http/tts_config_test.go +++ b/internal/http/tts_config_test.go @@ -7,9 +7,11 @@ import ( "maps" "net/http" "net/http/httptest" + "strings" "sync" "testing" + "github.com/google/uuid" "github.com/nextlevelbuilder/goclaw/internal/store" ) @@ -98,6 +100,58 @@ func newValidationTTSConfigMux(sc store.SystemConfigStore, cs store.ConfigSecret return mux } +func newValidationTTSConfigMuxWithTenants(sc store.SystemConfigStore, cs store.ConfigSecretsStore, ts store.TenantStore) *http.ServeMux { + h := NewTTSConfigHandler(sc, cs, ts) + mux := http.NewServeMux() + h.RegisterRoutes(mux) + return mux +} + +func TestTTSConfigRequiresTenantAdminForReadAndWrite(t *testing.T) { + setupTestToken(t, "gateway-token") + setupTestNoAuthFallback(t, false) + ts := newMockTenantStore() + tenantID := uuid.New() + ts.addTenant(tenantID, "acme") + ts.setUserRole(tenantID, "viewer-user", store.TenantRoleViewer) + ts.setUserRole(tenantID, "admin-user", store.TenantRoleAdmin) + setupTestTenantStore(t, ts) + + sc := &validationSystemConfigStore{data: map[string]string{}} + cs := &validationSecretsStore{data: map[string]string{}} + mux := newValidationTTSConfigMuxWithTenants(sc, cs, ts) + + viewerGet := httptest.NewRequest("GET", "/v1/tts/config", nil) + viewerGet.Header.Set("Authorization", "Bearer gateway-token") + viewerGet.Header.Set("X-GoClaw-User-Id", "viewer-user") + viewerGet.Header.Set("X-GoClaw-Tenant-Id", "acme") + viewerGetRR := httptest.NewRecorder() + mux.ServeHTTP(viewerGetRR, viewerGet) + if viewerGetRR.Code != http.StatusForbidden { + t.Fatalf("viewer GET status = %d, want 403", viewerGetRR.Code) + } + + viewerPost := httptest.NewRequest("POST", "/v1/tts/config", strings.NewReader(`{"provider":"edge"}`)) + viewerPost.Header.Set("Authorization", "Bearer gateway-token") + viewerPost.Header.Set("X-GoClaw-User-Id", "viewer-user") + viewerPost.Header.Set("X-GoClaw-Tenant-Id", "acme") + viewerPostRR := httptest.NewRecorder() + mux.ServeHTTP(viewerPostRR, viewerPost) + if viewerPostRR.Code != http.StatusForbidden { + t.Fatalf("viewer POST status = %d, want 403", viewerPostRR.Code) + } + + adminPost := httptest.NewRequest("POST", "/v1/tts/config", strings.NewReader(`{"provider":"edge"}`)) + adminPost.Header.Set("Authorization", "Bearer gateway-token") + adminPost.Header.Set("X-GoClaw-User-Id", "admin-user") + adminPost.Header.Set("X-GoClaw-Tenant-Id", "acme") + adminPostRR := httptest.NewRecorder() + mux.ServeHTTP(adminPostRR, adminPost) + if adminPostRR.Code != http.StatusOK { + t.Fatalf("tenant admin POST status = %d, want 200: %s", adminPostRR.Code, adminPostRR.Body.String()) + } +} + func TestTTSConfigSave_AcceptsLegacyAndUISchemaAliases(t *testing.T) { setupTestToken(t, "") diff --git a/internal/http/webhooks_admin.go b/internal/http/webhooks_admin.go index 9694abdd..5c63e9ae 100644 --- a/internal/http/webhooks_admin.go +++ b/internal/http/webhooks_admin.go @@ -15,6 +15,7 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/crypto" "github.com/nextlevelbuilder/goclaw/internal/edition" "github.com/nextlevelbuilder/goclaw/internal/i18n" + "github.com/nextlevelbuilder/goclaw/internal/permissions" "github.com/nextlevelbuilder/goclaw/internal/store" "github.com/nextlevelbuilder/goclaw/pkg/protocol" ) @@ -60,12 +61,28 @@ func (h *WebhooksAdminHandler) SetEncKey(encKey string) { // Runtime routes (/v1/webhooks/message, /v1/webhooks/llm) are mounted by phases 05/06 // conditionally: message-kind only if edition.Current().AllowsChannels(). func (h *WebhooksAdminHandler) RegisterRoutes(mux *http.ServeMux) { - mux.HandleFunc("POST /v1/webhooks", h.handleCreate) - mux.HandleFunc("GET /v1/webhooks", h.handleList) - mux.HandleFunc("GET /v1/webhooks/{id}", h.handleGet) - mux.HandleFunc("PATCH /v1/webhooks/{id}", h.handleUpdate) - mux.HandleFunc("POST /v1/webhooks/{id}/rotate", h.handleRotate) - mux.HandleFunc("DELETE /v1/webhooks/{id}", h.handleRevoke) + mux.HandleFunc("POST /v1/webhooks", h.requireAdmin(h.handleCreate)) + mux.HandleFunc("GET /v1/webhooks", h.requireAdmin(h.handleList)) + mux.HandleFunc("GET /v1/webhooks/{id}", h.requireAdmin(h.handleGet)) + mux.HandleFunc("PATCH /v1/webhooks/{id}", h.requireAdmin(h.handleUpdate)) + mux.HandleFunc("POST /v1/webhooks/{id}/rotate", h.requireAdmin(h.handleRotate)) + mux.HandleFunc("DELETE /v1/webhooks/{id}", h.requireAdmin(h.handleRevoke)) +} + +func (h *WebhooksAdminHandler) requireAdmin(next http.HandlerFunc) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + if role := permissions.Role(store.RoleFromContext(r.Context())); role != "" { + if !permissions.HasMinRole(role, permissions.RoleAdmin) { + writeJSON(w, http.StatusForbidden, map[string]string{ + "error": i18n.T(store.LocaleFromContext(r.Context()), i18n.MsgPermissionDenied, r.URL.Path+" requires "+string(permissions.RoleAdmin)+" role"), + }) + return + } + next(w, r) + return + } + requireAuth(permissions.RoleAdmin, next)(w, r) + } } // --- Create --- @@ -87,21 +104,21 @@ type createWebhookReq struct { // hmac_signing_key = raw secret itself — callers sign HMAC requests using raw secret bytes. // The raw secret is encrypted at rest; secret_hash is kept only for bearer-token lookup. type webhookCreateResp struct { - ID uuid.UUID `json:"id"` - TenantID uuid.UUID `json:"tenant_id"` - AgentID *uuid.UUID `json:"agent_id,omitempty"` - Name string `json:"name"` - Kind string `json:"kind"` - SecretPrefix string `json:"secret_prefix"` - Secret string `json:"secret"` // raw secret — shown ONCE; use this as HMAC key - HMACSigningKey string `json:"hmac_signing_key"` // same as Secret — raw bytes for X-GoClaw-Signature - Scopes []string `json:"scopes"` - ChannelID *uuid.UUID `json:"channel_id,omitempty"` - RateLimitPerMin int `json:"rate_limit_per_min"` - IPAllowlist []string `json:"ip_allowlist"` - RequireHMAC bool `json:"require_hmac"` - LocalhostOnly bool `json:"localhost_only"` - CreatedAt time.Time `json:"created_at"` + ID uuid.UUID `json:"id"` + TenantID uuid.UUID `json:"tenant_id"` + AgentID *uuid.UUID `json:"agent_id,omitempty"` + Name string `json:"name"` + Kind string `json:"kind"` + SecretPrefix string `json:"secret_prefix"` + Secret string `json:"secret"` // raw secret — shown ONCE; use this as HMAC key + HMACSigningKey string `json:"hmac_signing_key"` // same as Secret — raw bytes for X-GoClaw-Signature + Scopes []string `json:"scopes"` + ChannelID *uuid.UUID `json:"channel_id,omitempty"` + RateLimitPerMin int `json:"rate_limit_per_min"` + IPAllowlist []string `json:"ip_allowlist"` + RequireHMAC bool `json:"require_hmac"` + LocalhostOnly bool `json:"localhost_only"` + CreatedAt time.Time `json:"created_at"` } func (h *WebhooksAdminHandler) handleCreate(w http.ResponseWriter, r *http.Request) { @@ -465,8 +482,8 @@ func (h *WebhooksAdminHandler) handleRotate(w http.ResponseWriter, r *http.Reque writeJSON(w, http.StatusOK, map[string]any{ "id": id, - "secret": raw, // new raw secret — shown ONCE; use as HMAC key - "hmac_signing_key": raw, // same as secret; raw bytes are HMAC key (encrypted at rest) + "secret": raw, // new raw secret — shown ONCE; use as HMAC key + "hmac_signing_key": raw, // same as secret; raw bytes are HMAC key (encrypted at rest) "secret_prefix": newPrefix, }) } diff --git a/internal/http/webhooks_admin_test.go b/internal/http/webhooks_admin_test.go index 96d2b819..dcb92c25 100644 --- a/internal/http/webhooks_admin_test.go +++ b/internal/http/webhooks_admin_test.go @@ -206,6 +206,15 @@ func webhookTenantAdminCtx(tenantID uuid.UUID, userID string) context.Context { ctx := context.Background() ctx = store.WithTenantID(ctx, tenantID) ctx = store.WithUserID(ctx, userID) + ctx = store.WithRole(ctx, "admin") + return ctx +} + +func webhookTenantCtxWithRole(tenantID uuid.UUID, userID, role string) context.Context { + ctx := context.Background() + ctx = store.WithTenantID(ctx, tenantID) + ctx = store.WithUserID(ctx, userID) + ctx = store.WithRole(ctx, role) return ctx } @@ -239,6 +248,28 @@ func doRequest(t *testing.T, h *WebhooksAdminHandler, method, path string, body // ---- tests ---- +func TestWebhookAdmin_RouteRequiresHTTPAuth(t *testing.T) { + oldToken := pkgGatewayToken + oldFallback := pkgNoAuthFallbackAllowed + InitGatewayToken("required-token") + InitGatewayNoAuthFallbackAllowed(false) + defer func() { + InitGatewayToken(oldToken) + InitGatewayNoAuthFallbackAllowed(oldFallback) + }() + + h := newAdminHandler(newAdminWebhookStore(), &adminTenantStore{}) + r := httptest.NewRequest(http.MethodGet, "/v1/webhooks", nil) + w := httptest.NewRecorder() + mux := http.NewServeMux() + h.RegisterRoutes(mux) + mux.ServeHTTP(w, r) + + if w.Code != http.StatusUnauthorized { + t.Fatalf("expected 401 for unauthenticated admin route, got %d", w.Code) + } +} + // TestWebhookAdmin_Create_HappyPath verifies POST /v1/webhooks returns secret once. func TestWebhookAdmin_Create_HappyPath(t *testing.T) { tenantID := uuid.New() @@ -310,6 +341,29 @@ func TestWebhookAdmin_Create_NonAdmin_403(t *testing.T) { } } +func TestWebhookAdmin_Create_ContextOperatorRoleDeniedBeforeTenantAdmin(t *testing.T) { + tenantID := uuid.New() + userID := "operator-context" + + ts := &adminTenantStore{ + roles: map[string]string{ + tenantID.String() + ":" + userID: store.TenantRoleAdmin, + }, + } + ws := newAdminWebhookStore() + h := newAdminHandler(ws, ts) + + ctx := webhookTenantCtxWithRole(tenantID, userID, "operator") + w := doRequest(t, h, http.MethodPost, "/v1/webhooks", map[string]any{ + "name": "x", + "kind": "llm", + }, ctx) + + if w.Code != http.StatusForbidden { + t.Fatalf("want 403, got %d: %s", w.Code, w.Body.String()) + } +} + // TestWebhookAdmin_Create_InvalidKind_400 verifies unknown kind is rejected. func TestWebhookAdmin_Create_InvalidKind_400(t *testing.T) { tenantID := uuid.New() diff --git a/internal/http/webhooks_auth.go b/internal/http/webhooks_auth.go index 6a25b38c..dde6ff28 100644 --- a/internal/http/webhooks_auth.go +++ b/internal/http/webhooks_auth.go @@ -41,7 +41,7 @@ const ( // WebhookAuthMiddleware is the composed middleware chain for all /v1/webhooks/* // runtime endpoints. Order: body cap → bearer/HMAC auth → localhost gate → -// IP allowlist → rate limit → idempotency guard → inject context → next. +// IP allowlist → rate limit → inject context → idempotency guard → next. // // Parameters: // - ws: WebhookStore for secret + row lookup. @@ -185,25 +185,28 @@ func WebhookAuthMiddleware( return } - // 7. Idempotency check. - proceed, _ := checkIdempotency(w, r, body, webhook.ID, calls) - if !proceed { - return - } - - // 8. Inject webhook + tenant into context; propagate to stores. + // 7. Inject webhook + tenant into context; propagate to stores. // K1: tenant injected HERE so all store calls below are tenant-scoped. ctx = WithWebhookData(ctx, webhook) + ctx = WithWebhookRawBody(ctx, body) ctx = store.WithTenantID(ctx, webhook.TenantID) if webhook.AgentID != nil { ctx = store.WithAgentID(ctx, *webhook.AgentID) } + scopedReq := r.WithContext(ctx) + + // 8. Idempotency check. This must run after tenant injection because + // WebhookCallStore lookups are tenant scoped. + proceed, _ := checkIdempotency(w, scopedReq, body, webhook.ID, calls) + if !proceed { + return + } // Best-effort touch — don't block on failure. Use WithoutCancel so // the DB write is not cancelled when the HTTP response completes. - go func() { _ = ws.TouchLastUsed(context.WithoutCancel(r.Context()), webhook.ID) }() + go func() { _ = ws.TouchLastUsed(context.WithoutCancel(scopedReq.Context()), webhook.ID) }() - next.ServeHTTP(w, r.WithContext(ctx)) + next.ServeHTTP(w, scopedReq) }) } } diff --git a/internal/http/webhooks_auth_test.go b/internal/http/webhooks_auth_test.go index ebeadcea..bb9a6a0b 100644 --- a/internal/http/webhooks_auth_test.go +++ b/internal/http/webhooks_auth_test.go @@ -84,11 +84,12 @@ func (s *stubWebhookStore) Update(_ context.Context, _ uuid.UUID, _ map[string]a func (s *stubWebhookStore) RotateSecret(_ context.Context, _ uuid.UUID, _, _, _ string) error { return nil } -func (s *stubWebhookStore) Revoke(_ context.Context, _ uuid.UUID) error { return nil } +func (s *stubWebhookStore) Revoke(_ context.Context, _ uuid.UUID) error { return nil } func (s *stubWebhookStore) TouchLastUsed(_ context.Context, _ uuid.UUID) error { return nil } type stubWebhookCallStore struct { - calls map[string]*store.WebhookCallData // key = idempotency_key + calls map[string]*store.WebhookCallData // key = idempotency_key + lastTenant uuid.UUID } func newStubCallStore(calls ...*store.WebhookCallData) *stubWebhookCallStore { @@ -101,7 +102,8 @@ func newStubCallStore(calls ...*store.WebhookCallData) *stubWebhookCallStore { return s } -func (s *stubWebhookCallStore) GetByIdempotency(_ context.Context, _ uuid.UUID, key string) (*store.WebhookCallData, error) { +func (s *stubWebhookCallStore) GetByIdempotency(ctx context.Context, _ uuid.UUID, key string) (*store.WebhookCallData, error) { + s.lastTenant = store.TenantIDFromContext(ctx) c, ok := s.calls[key] if !ok { return nil, sql.ErrNoRows @@ -182,8 +184,8 @@ func makeWebhook(kind string, opts ...func(*store.WebhookData)) *store.WebhookDa return w } -func withRevoked(w *store.WebhookData) { w.Revoked = true } -func withRequireHMAC(w *store.WebhookData) { w.RequireHMAC = true } +func withRevoked(w *store.WebhookData) { w.Revoked = true } +func withRequireHMAC(w *store.WebhookData) { w.RequireHMAC = true } func withLocalhostOnly(w *store.WebhookData) { w.LocalhostOnly = true } func withRPM(rpm int) func(*store.WebhookData) { return func(w *store.WebhookData) { w.RateLimitPerMin = rpm } @@ -483,6 +485,27 @@ func TestWebhookAuth_IdempotencyReplay(t *testing.T) { } } +func TestWebhookAuth_IdempotencyRunsWithTenantContext(t *testing.T) { + raw, hashHex := makeSecret() + wh := makeWebhook("llm") + wh.SecretHash = hashHex + ws := newStubWebhookStore(wh) + calls := newStubCallStore() + + handler := makeMiddleware(ws, calls, "llm", WebhookMaxBodyLLM) + w := httptest.NewRecorder() + r := bearerReq(raw, `{"input":"hi"}`) + r.Header.Set("Idempotency-Key", "tenant-context-key") + handler.ServeHTTP(w, r) + + if w.Code != http.StatusOK { + t.Fatalf("expected middleware to proceed, got %d", w.Code) + } + if calls.lastTenant != wh.TenantID { + t.Fatalf("idempotency lookup tenant = %s, want %s", calls.lastTenant, wh.TenantID) + } +} + func TestWebhookAuth_NoAuthHeader(t *testing.T) { wh := makeWebhook("llm") ws := newStubWebhookStore(wh) diff --git a/internal/http/webhooks_context.go b/internal/http/webhooks_context.go index f2deedbd..1c85cb9a 100644 --- a/internal/http/webhooks_context.go +++ b/internal/http/webhooks_context.go @@ -10,6 +10,7 @@ import ( // Uses a distinct struct type (not contextKey string) to avoid collision with // store-layer keys while following the same struct-key pattern. type webhookCtxKey struct{} +type webhookRawBodyCtxKey struct{} // WithWebhookData returns a new context carrying the resolved WebhookData. // Call store.WithTenantID separately to propagate tenant to downstream stores. @@ -23,3 +24,16 @@ func WebhookDataFromContext(ctx context.Context) *store.WebhookData { v, _ := ctx.Value(webhookCtxKey{}).(*store.WebhookData) return v } + +func WithWebhookRawBody(ctx context.Context, body []byte) context.Context { + cp := append([]byte(nil), body...) + return context.WithValue(ctx, webhookRawBodyCtxKey{}, cp) +} + +func WebhookRawBodyFromContext(ctx context.Context) []byte { + v, _ := ctx.Value(webhookRawBodyCtxKey{}).([]byte) + if v == nil { + return nil + } + return append([]byte(nil), v...) +} diff --git a/internal/http/webhooks_idempotency.go b/internal/http/webhooks_idempotency.go index 7f6e83e0..e1d86c65 100644 --- a/internal/http/webhooks_idempotency.go +++ b/internal/http/webhooks_idempotency.go @@ -1,18 +1,26 @@ package http import ( + "context" "crypto/sha256" "database/sql" "encoding/hex" "encoding/json" "errors" + "log/slog" "net/http" + "time" "github.com/google/uuid" "github.com/nextlevelbuilder/goclaw/internal/i18n" "github.com/nextlevelbuilder/goclaw/internal/store" ) +// webhookSyncReservationTTL must exceed the longest legitimate sync webhook path. +// Telegram media sends can run for 3 minutes on slow uploads; keep enough margin +// so a duplicate idempotency request cannot mark an active send as expired. +const webhookSyncReservationTTL = 10 * time.Minute + // checkIdempotency inspects the Idempotency-Key header and resolves prior calls. // // Returns: @@ -66,6 +74,8 @@ func checkIdempotency( return false, errors.New("idempotency conflict") } + expireStaleSyncReservation(ctx, calls, existing, time.Now()) + // Same key + matching body → replay last stored response. if len(existing.Response) > 0 { w.Header().Set("Content-Type", "application/json") @@ -116,3 +126,154 @@ func extractBodyHash(payload []byte) string { } return p.BodyHash } + +func optionalIdempotencyKey(r *http.Request) *string { + if key := r.Header.Get("Idempotency-Key"); key != "" { + return &key + } + return nil +} + +func reserveIdempotentCall( + w http.ResponseWriter, + r *http.Request, + calls store.WebhookCallStore, + call *store.WebhookCallData, +) (reserved bool, handled bool) { + if call.IdempotencyKey == nil { + return false, false + } + if err := calls.Create(r.Context(), call); err != nil { + if errors.Is(err, store.ErrIdempotencyConflict) { + if replayStoredIdempotencyFromPayload(w, r, calls, call.WebhookID, *call.IdempotencyKey, call.RequestPayload) { + return false, true + } + } + slog.Error("webhook.idempotency_reserve_failed", "error", err, "call_id", call.ID) + writeJSON(w, http.StatusInternalServerError, map[string]string{ + "error": i18n.T(store.LocaleFromContext(r.Context()), i18n.MsgInternalError, "failed to reserve idempotency key"), + }) + return false, true + } + return true, false +} + +func persistWebhookCall( + ctx context.Context, + calls store.WebhookCallStore, + call *store.WebhookCallData, + reserved bool, + logName string, +) { + ctx = context.WithoutCancel(ctx) + var err error + if reserved { + updates := map[string]any{ + "status": call.Status, + "attempts": call.Attempts, + "response": call.Response, + "last_error": call.LastError, + "completed_at": call.CompletedAt, + } + err = calls.UpdateStatus(ctx, call.ID, updates) + } else { + err = calls.Create(ctx, call) + } + if err != nil { + slog.Warn(logName, "error", err, "call_id", call.ID) + } +} + +func replayStoredIdempotencyFromPayload( + w http.ResponseWriter, + r *http.Request, + calls store.WebhookCallStore, + webhookID uuid.UUID, + key string, + requestPayload []byte, +) bool { + existing, err := calls.GetByIdempotency(r.Context(), webhookID, key) + if err != nil { + return false + } + locale := store.LocaleFromContext(r.Context()) + if extractBodyHash(existing.RequestPayload) != extractBodyHash(requestPayload) { + writeJSON(w, http.StatusConflict, map[string]string{ + "error": i18n.T(locale, i18n.MsgWebhookIdempotencyConflict), + }) + return true + } + expireStaleSyncReservation(r.Context(), calls, existing, time.Now()) + if len(existing.Response) > 0 { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("X-Idempotency-Replayed", "true") + w.WriteHeader(http.StatusOK) + _, _ = w.Write(existing.Response) + return true + } + writeJSON(w, http.StatusAccepted, map[string]string{ + "status": existing.Status, + "call_id": existing.ID.String(), + }) + return true +} + +func expireStaleSyncReservation( + ctx context.Context, + calls store.WebhookCallStore, + existing *store.WebhookCallData, + now time.Time, +) bool { + if !isStaleSyncReservation(existing, now) { + return false + } + + reason := "sync idempotency reservation expired" + resp, err := json.Marshal(map[string]string{ + "call_id": existing.ID.String(), + "status": "failed", + "error": reason, + }) + if err != nil { + slog.Warn("webhook.idempotency_expire_response_failed", "error", err, "call_id", existing.ID) + return false + } + + completedAt := now + attempts := existing.Attempts + if attempts == 0 { + attempts = 1 + } + updates := map[string]any{ + "status": "failed", + "attempts": attempts, + "response": resp, + "last_error": reason, + "completed_at": completedAt, + } + if err := calls.UpdateStatus(context.WithoutCancel(ctx), existing.ID, updates); err != nil { + slog.Warn("webhook.idempotency_expire_failed", "error", err, "call_id", existing.ID) + return false + } + + existing.Status = "failed" + existing.Attempts = attempts + existing.Response = resp + existing.LastError = &reason + existing.CompletedAt = &completedAt + return true +} + +func isStaleSyncReservation(existing *store.WebhookCallData, now time.Time) bool { + if existing == nil || existing.Mode != "sync" || existing.Status != "running" { + return false + } + startedAt := existing.CreatedAt + if existing.StartedAt != nil { + startedAt = *existing.StartedAt + } + if startedAt.IsZero() { + return false + } + return now.Sub(startedAt) > webhookSyncReservationTTL +} diff --git a/internal/http/webhooks_idempotency_test.go b/internal/http/webhooks_idempotency_test.go index fc25117d..ca973cbf 100644 --- a/internal/http/webhooks_idempotency_test.go +++ b/internal/http/webhooks_idempotency_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" "strings" "testing" + "time" "github.com/google/uuid" "github.com/nextlevelbuilder/goclaw/internal/store" @@ -144,6 +145,54 @@ func TestCheckIdempotency_malformedStoredHash(t *testing.T) { } } +func TestCheckIdempotency_StaleSyncReservationExpires(t *testing.T) { + webhookID := uuid.New() + body := []byte(`{"input":"hello"}`) + payload, err := buildAuditPayload(body, map[string]string{"input": "hello"}) + if err != nil { + t.Fatalf("buildAuditPayload: %v", err) + } + + key := "idem-stale-sync" + startedAt := time.Now().Add(-(webhookSyncReservationTTL + time.Second)) + existing := &store.WebhookCallData{ + ID: uuid.New(), + WebhookID: webhookID, + IdempotencyKey: &key, + Mode: "sync", + Status: "running", + RequestPayload: payload, + StartedAt: &startedAt, + CreatedAt: startedAt, + } + calls := newStubCallStore(existing) + + req := httptest.NewRequest(http.MethodPost, "/v1/webhooks/llm", strings.NewReader(string(body))) + req.Header.Set("Idempotency-Key", key) + rec := httptest.NewRecorder() + + proceed, err := checkIdempotency(rec, req, body, webhookID, calls) + + if proceed { + t.Fatal("expected stale idempotency row to be handled, got proceed=true") + } + if err != nil { + t.Fatalf("expected nil error for expired replay response, got %v", err) + } + if rec.Code != http.StatusOK { + t.Fatalf("expected 200 replay for expired row, got %d: %s", rec.Code, rec.Body.String()) + } + if rec.Header().Get("X-Idempotency-Replayed") != "true" { + t.Fatal("expected X-Idempotency-Replayed header") + } + if existing.Status != "failed" { + t.Fatalf("expected stale row status failed, got %q", existing.Status) + } + if len(existing.Response) == 0 || !strings.Contains(string(existing.Response), "sync idempotency reservation expired") { + t.Fatalf("expected stored expiry response, got %s", string(existing.Response)) + } +} + // strPtr is a test helper returning a pointer to s. func strPtr(s string) *string { return &s } diff --git a/internal/http/webhooks_llm.go b/internal/http/webhooks_llm.go index 89678636..86026dff 100644 --- a/internal/http/webhooks_llm.go +++ b/internal/http/webhooks_llm.go @@ -233,19 +233,23 @@ func (h *WebhookLLMHandler) handle(w http.ResponseWriter, r *http.Request) { deliveryID := store.GenNewID() now := time.Now() - // Capture raw body bytes for body_hash computation. - // req was decoded from the HTTP body; re-marshal to get canonical bytes. + // Capture raw body bytes for body_hash computation when middleware supplied them. + // Direct handler tests fall back to canonical JSON bytes from the decoded request. // The audit payload uses the canonical JSON shape {"body_hash":"...","meta":{...}} // so PG jsonb insert never triggers error 22P02. - reqBytes, _ := json.Marshal(req) + reqBytes := WebhookRawBodyFromContext(ctx) + if reqBytes == nil { + reqBytes, _ = json.Marshal(req) + } requestPayload, _ := buildAuditPayload(reqBytes, req) + idempotencyKey := optionalIdempotencyKey(r) // Dispatch based on mode. switch mode { case "async": - h.handleAsync(w, r, ctx, locale, webhook, ag, agentID, req, callID, deliveryID, now, requestPayload, userMessage, extraSystemPrompt) + h.handleAsync(w, r, ctx, locale, webhook, ag, agentID, req, callID, deliveryID, now, requestPayload, idempotencyKey, userMessage, extraSystemPrompt) default: // "sync" - h.handleSync(w, r, ctx, locale, webhook, ag, agentID, req, callID, deliveryID, now, requestPayload, userMessage, extraSystemPrompt) + h.handleSync(w, r, ctx, locale, webhook, ag, agentID, req, callID, deliveryID, now, requestPayload, idempotencyKey, userMessage, extraSystemPrompt) } } @@ -262,10 +266,29 @@ func (h *WebhookLLMHandler) handleSync( callID, deliveryID uuid.UUID, now time.Time, requestPayload []byte, + idempotencyKey *string, userMessage, extraSystemPrompt string, ) { runID := uuid.NewString() sessionKey := resolveWebhookSessionKey(req.SessionKey, agentID, webhook.ID, runID) + callRecord := &store.WebhookCallData{ + ID: callID, + TenantID: webhook.TenantID, + WebhookID: webhook.ID, + AgentID: webhook.AgentID, + DeliveryID: deliveryID, + IdempotencyKey: idempotencyKey, + Mode: "sync", + Status: "running", + Attempts: 0, + RequestPayload: requestPayload, + CreatedAt: now, + StartedAt: &now, + } + callReserved, handled := reserveIdempotentCall(w, r, h.callStore, callRecord) + if handled { + return + } rr := agent.RunRequest{ SessionKey: sessionKey, @@ -318,6 +341,13 @@ func (h *WebhookLLMHandler) handleSync( }) if submitErr != nil { + completedAt := time.Now() + errMsg := submitErr.Error() + callRecord.Status = "failed" + callRecord.Attempts = 1 + callRecord.CompletedAt = &completedAt + callRecord.LastError = &errMsg + persistWebhookCall(ctx, h.callStore, callRecord, callReserved, "webhook.llm.audit_write_failed") // Lane at capacity or ctx cancelled before slot acquired. slog.Warn("webhook.lane_saturated", "webhook_id", webhook.ID, @@ -345,21 +375,11 @@ func (h *WebhookLLMHandler) handleSync( if errors.Is(out.err, context.DeadlineExceeded) { // Write audit row as failed/timeout. errMsg := "context deadline exceeded" - h.writeCallRecord(ctx, &store.WebhookCallData{ - ID: callID, - TenantID: webhook.TenantID, - WebhookID: webhook.ID, - AgentID: webhook.AgentID, - DeliveryID: deliveryID, - Mode: "sync", - Status: "failed", - Attempts: 1, - RequestPayload: requestPayload, - LastError: &errMsg, - CreatedAt: now, - CompletedAt: &completedAt, - StartedAt: &now, - }) + callRecord.Status = "failed" + callRecord.Attempts = 1 + callRecord.LastError = &errMsg + callRecord.CompletedAt = &completedAt + persistWebhookCall(ctx, h.callStore, callRecord, callReserved, "webhook.llm.audit_write_failed") writeError(w, http.StatusGatewayTimeout, protocol.ErrInternal, i18n.T(locale, i18n.MsgWebhookLLMTimeout)) return @@ -367,21 +387,11 @@ func (h *WebhookLLMHandler) handleSync( // Other error. errMsg := out.err.Error() - h.writeCallRecord(ctx, &store.WebhookCallData{ - ID: callID, - TenantID: webhook.TenantID, - WebhookID: webhook.ID, - AgentID: webhook.AgentID, - DeliveryID: deliveryID, - Mode: "sync", - Status: "failed", - Attempts: 1, - RequestPayload: requestPayload, - LastError: &errMsg, - CreatedAt: now, - CompletedAt: &completedAt, - StartedAt: &now, - }) + callRecord.Status = "failed" + callRecord.Attempts = 1 + callRecord.LastError = &errMsg + callRecord.CompletedAt = &completedAt + persistWebhookCall(ctx, h.callStore, callRecord, callReserved, "webhook.llm.audit_write_failed") writeError(w, http.StatusInternalServerError, protocol.ErrInternal, i18n.T(locale, i18n.MsgInternalError, out.err.Error())) return @@ -409,21 +419,11 @@ func (h *WebhookLLMHandler) handleSync( } completedAt := time.Now() - h.writeCallRecord(ctx, &store.WebhookCallData{ - ID: callID, - TenantID: webhook.TenantID, - WebhookID: webhook.ID, - AgentID: webhook.AgentID, - DeliveryID: deliveryID, - Mode: "sync", - Status: "done", - Attempts: 1, - RequestPayload: requestPayload, - Response: respBytes, - CreatedAt: now, - CompletedAt: &completedAt, - StartedAt: &now, - }) + callRecord.Status = "done" + callRecord.Attempts = 1 + callRecord.Response = respBytes + callRecord.CompletedAt = &completedAt + persistWebhookCall(ctx, h.callStore, callRecord, callReserved, "webhook.llm.audit_write_failed") slog.Info("webhook.llm.sync", "call_id", callID, @@ -438,7 +438,7 @@ func (h *WebhookLLMHandler) handleSync( // handleAsync enqueues a webhook_calls row and returns 202 immediately. func (h *WebhookLLMHandler) handleAsync( w http.ResponseWriter, - _ *http.Request, + r *http.Request, ctx context.Context, locale string, webhook *store.WebhookData, @@ -448,6 +448,7 @@ func (h *WebhookLLMHandler) handleAsync( callID, deliveryID uuid.UUID, now time.Time, requestPayload []byte, + idempotencyKey *string, _, _ string, // userMessage, extraSystemPrompt — stored in requestPayload, not used here ) { // SSRF validation on callback_url — defense against DNS rebinding. @@ -471,6 +472,7 @@ func (h *WebhookLLMHandler) handleAsync( WebhookID: webhook.ID, AgentID: webhook.AgentID, DeliveryID: deliveryID, + IdempotencyKey: idempotencyKey, Mode: "async", Status: "queued", CallbackURL: &cbURL, @@ -481,6 +483,11 @@ func (h *WebhookLLMHandler) handleAsync( } if err := h.callStore.Create(ctx, call); err != nil { + if idempotencyKey != nil && errors.Is(err, store.ErrIdempotencyConflict) { + if replayStoredIdempotencyFromPayload(w, r, h.callStore, webhook.ID, *idempotencyKey, requestPayload) { + return + } + } slog.Error("webhook.llm.async_enqueue_failed", "error", err, "call_id", callID, @@ -504,16 +511,6 @@ func (h *WebhookLLMHandler) handleAsync( }) } -// writeCallRecord persists an audit call record. Best-effort — failures are logged but not fatal. -func (h *WebhookLLMHandler) writeCallRecord(ctx context.Context, call *store.WebhookCallData) { - if err := h.callStore.Create(ctx, call); err != nil { - slog.Warn("webhook.llm.audit_write_failed", - "error", err, - "call_id", call.ID, - ) - } -} - // buildInput parses the raw JSON input into a user message and optional extra system prompt. // // Two formats are accepted: @@ -561,4 +558,3 @@ func resolveWebhookSessionKey(reqSessionKey, agentID string, webhookID uuid.UUID } return fmt.Sprintf("webhook:%s:%s:%s", agentID, webhookID.String(), runID[:8]) } - diff --git a/internal/http/webhooks_message.go b/internal/http/webhooks_message.go index eeb27002..130b8f02 100644 --- a/internal/http/webhooks_message.go +++ b/internal/http/webhooks_message.go @@ -105,7 +105,7 @@ type webhookMessageReq struct { // webhookMessageResp is the success response envelope. type webhookMessageResp struct { CallID string `json:"call_id"` - Status string `json:"status"` // always "sent" + Status string `json:"status"` // always "sent" ChannelName string `json:"channel_name"` ChatID string `json:"chat_id"` Warning string `json:"warning,omitempty"` // set when media was dropped on fallback @@ -163,9 +163,13 @@ func (h *WebhookMessageHandler) handle(w http.ResponseWriter, r *http.Request) { deliveryID := store.GenNewID() now := time.Now() callRecord := h.newCallRecord(r, webhook, callID, deliveryID, now, channelName, req) + callReserved, handled := reserveIdempotentCall(w, r, h.callStore, callRecord) + if handled { + return + } // Dispatch — media or text-only path. - warning, sendErr := h.dispatch(ctx, w, r, webhook, req, channelName, callRecord, locale) + warning, sendErr := h.dispatch(ctx, w, r, webhook, req, channelName, callRecord, callReserved, locale) if sendErr != nil { return // error response already written by dispatch } @@ -186,13 +190,7 @@ func (h *WebhookMessageHandler) handle(w http.ResponseWriter, r *http.Request) { respBytes, _ := json.Marshal(respBody) callRecord.Response = respBytes - if err := h.callStore.Create(ctx, callRecord); err != nil { - // Non-fatal: audit failure must not fail a delivered message. - slog.Warn("webhook.message.audit_write_failed", - "error", err, - "call_id", callID, - ) - } + persistWebhookCall(ctx, h.callStore, callRecord, callReserved, "webhook.message.audit_write_failed") slog.Info("webhook.message.delivered", "tenant_id", webhook.TenantID, @@ -215,12 +213,13 @@ func (h *WebhookMessageHandler) dispatch( req webhookMessageReq, channelName string, callRecord *store.WebhookCallData, + callReserved bool, locale string, ) (warning string, _ error) { if req.MediaURL == "" { // Text-only path. if err := h.channelMgr.SendToChannel(ctx, channelName, req.ChatID, req.Content); err != nil { - h.failCall(ctx, callRecord, err.Error()) + h.failCall(ctx, callRecord, callReserved, err.Error()) slog.Error("webhook.message.dispatch_failed", "error", err, "channel_name", channelName, @@ -238,7 +237,7 @@ func (h *WebhookMessageHandler) dispatch( if probeErr != nil { var mve *mediaValidateError if errors.As(probeErr, &mve) { - h.failCall(ctx, callRecord, mve.message) + h.failCall(ctx, callRecord, callReserved, mve.message) switch mve.code { case "ssrf": slog.Warn("security.webhook.ssrf_blocked", @@ -258,7 +257,7 @@ func (h *WebhookMessageHandler) dispatch( i18n.T(locale, i18n.MsgWebhookMediaSSRFBlocked)) } } else { - h.failCall(ctx, callRecord, probeErr.Error()) + h.failCall(ctx, callRecord, callReserved, probeErr.Error()) writeError(w, http.StatusBadRequest, protocol.ErrInvalidRequest, i18n.T(locale, i18n.MsgWebhookMediaSSRFBlocked)) } @@ -274,7 +273,7 @@ func (h *WebhookMessageHandler) dispatch( Caption: req.MediaCaption, }} if err := h.channelMgr.SendMediaToChannel(ctx, channelName, req.ChatID, req.Content, media); err != nil { - h.failCall(ctx, callRecord, err.Error()) + h.failCall(ctx, callRecord, callReserved, err.Error()) slog.Error("webhook.message.dispatch_failed", "error", err, "channel_name", channelName, @@ -295,7 +294,7 @@ func (h *WebhookMessageHandler) dispatch( "webhook_id", webhook.ID, ) if err := h.channelMgr.SendToChannel(ctx, channelName, req.ChatID, req.Content); err != nil { - h.failCall(ctx, callRecord, err.Error()) + h.failCall(ctx, callRecord, callReserved, err.Error()) slog.Error("webhook.message.dispatch_failed", "error", err, "channel_name", channelName, @@ -310,7 +309,7 @@ func (h *WebhookMessageHandler) dispatch( // Media unsupported + no fallback → 501. const reason = "channel does not support media and fallback_to_text is false" - h.failCall(ctx, callRecord, reason) + h.failCall(ctx, callRecord, callReserved, reason) writeError(w, http.StatusNotImplemented, protocol.ErrInvalidRequest, i18n.T(locale, i18n.MsgWebhookMediaChannelUnsupported)) return "", errors.New(reason) @@ -385,7 +384,10 @@ func (h *WebhookMessageHandler) newCallRecord( ) *store.WebhookCallData { // Encode canonical audit payload: {"body_hash": "", "meta": {...}}. // PG jsonb rejects non-JSON bytes; this shape is valid JSON on both PG and SQLite. - bodyBytes, _ := json.Marshal(req) + bodyBytes := WebhookRawBodyFromContext(r.Context()) + if bodyBytes == nil { + bodyBytes, _ = json.Marshal(req) + } requestPayload, _ := buildAuditPayload(bodyBytes, map[string]any{ "channel_name": channelName, "chat_id": req.ChatID, @@ -413,15 +415,13 @@ func (h *WebhookMessageHandler) newCallRecord( } // failCall mutates call to status=failed and records it in the store. Best-effort. -func (h *WebhookMessageHandler) failCall(ctx context.Context, call *store.WebhookCallData, reason string) { +func (h *WebhookMessageHandler) failCall(ctx context.Context, call *store.WebhookCallData, reserved bool, reason string) { now := time.Now() call.Status = "failed" call.CompletedAt = &now call.LastError = &reason call.Attempts = 1 - if err := h.callStore.Create(ctx, call); err != nil { - slog.Warn("webhook.message.audit_write_failed", "error", err, "call_id", call.ID) - } + persistWebhookCall(ctx, h.callStore, call, reserved, "webhook.message.audit_write_failed") } // redactedHost extracts the hostname from a URL string for safe (no-path) log output. diff --git a/internal/sandbox/docker_test.go b/internal/sandbox/docker_test.go index c402e22b..50b4fc94 100644 --- a/internal/sandbox/docker_test.go +++ b/internal/sandbox/docker_test.go @@ -126,3 +126,77 @@ func TestResolveScopeKey(t *testing.T) { } } } + +func TestFsBridgeResolvePathRejectsWorkspaceEscapes(t *testing.T) { + bridge := NewFsBridge("container-test", "/workspace/agent-a") + + tests := []struct { + name string + path string + want string + }{ + {name: "inside relative", path: "notes/a.txt", want: "/workspace/agent-a/notes/a.txt"}, + {name: "inside absolute", path: "/workspace/agent-a/notes/a.txt", want: "/workspace/agent-a/notes/a.txt"}, + {name: "relative parent escape", path: "../agent-b/secret.txt", want: "/workspace/agent-a"}, + {name: "absolute sibling escape", path: "/workspace/agent-b/secret.txt", want: "/workspace/agent-a"}, + {name: "root escape", path: "/etc/passwd", want: "/workspace/agent-a"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := bridge.resolvePath(tt.path); got != tt.want { + t.Fatalf("resolvePath(%q) = %q, want %q", tt.path, got, tt.want) + } + }) + } +} + +func TestFsBridgePathWithinUsesPathBoundaries(t *testing.T) { + tests := []struct { + name string + root string + target string + want bool + }{ + {name: "root itself", root: "/workspace/agent-a", target: "/workspace/agent-a", want: true}, + {name: "child path", root: "/workspace/agent-a", target: "/workspace/agent-a/file.txt", want: true}, + {name: "sibling with shared prefix", root: "/workspace/agent-a", target: "/workspace/agent-a-b/file.txt", want: false}, + {name: "parent path", root: "/workspace/agent-a", target: "/workspace", want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := fsBridgePathWithin(tt.root, tt.target); got != tt.want { + t.Fatalf("fsBridgePathWithin(%q, %q) = %v, want %v", tt.root, tt.target, got, tt.want) + } + }) + } +} + +func TestFsBridgeWriteFileCommandPreservesOverwriteTruncation(t *testing.T) { + args := fsBridgeWriteDDArgs("/workspace/file.txt", false) + for _, arg := range args { + if arg == "conv=notrunc" || arg == "oflag=append" { + t.Fatalf("overwrite command must truncate, got append-only arg %q in %v", arg, args) + } + } +} + +func TestFsBridgeWriteFileCommandUsesNoTruncOnlyForAppend(t *testing.T) { + args := fsBridgeWriteDDArgs("/workspace/file.txt", true) + if !containsString(args, "conv=notrunc") { + t.Fatalf("append command missing conv=notrunc: %v", args) + } + if !containsString(args, "oflag=append") { + t.Fatalf("append command missing oflag=append: %v", args) + } +} + +func containsString(values []string, target string) bool { + for _, value := range values { + if value == target { + return true + } + } + return false +} diff --git a/internal/sandbox/fsbridge.go b/internal/sandbox/fsbridge.go index 6188f78b..15278641 100644 --- a/internal/sandbox/fsbridge.go +++ b/internal/sandbox/fsbridge.go @@ -37,8 +37,12 @@ func NewFsBridge(containerID, workdir string) *FsBridge { // Matching TS FsBridge.readFile(). func (b *FsBridge) ReadFile(ctx context.Context, path string) (string, error) { resolved := b.resolvePath(path) + realPath, err := b.resolveExistingPath(ctx, resolved) + if err != nil { + return "", err + } - stdout, stderr, exitCode, err := b.dockerExec(ctx, nil, "cat", "--", resolved) + stdout, stderr, exitCode, err := b.dockerExec(ctx, nil, "cat", "--", realPath) if err != nil { return "", fmt.Errorf("fsbridge read: %w", err) } @@ -50,23 +54,34 @@ func (b *FsBridge) ReadFile(ctx context.Context, path string) (string, error) { } // WriteFile writes content to a file inside the container, creating directories as needed. -// When append is true, content is appended (shell >>); otherwise the file is overwritten (shell >). +// When append is true, content is appended; otherwise the file is overwritten. // Matching TS FsBridge.writeFile(). func (b *FsBridge) WriteFile(ctx context.Context, path, content string, appendMode bool) error { resolved := b.resolvePath(path) - // Create parent directory - dir := resolved[:strings.LastIndex(resolved, "/")] - if dir != "" && dir != "/" { - _, _, _, _ = b.dockerExec(ctx, nil, "mkdir", "-p", dir) + if err := b.validateExistingTargetIfPresent(ctx, resolved); err != nil { + return err } - redir := ">" - if appendMode { - redir = ">>" + dir := resolved[:strings.LastIndex(resolved, "/")] + if dir != "" && dir != "/" { + if err := b.validateParentBeforeCreate(ctx, dir); err != nil { + return err + } + _, stderr, exitCode, err := b.dockerExec(ctx, nil, "mkdir", "-p", "--", dir) + if err != nil { + return fmt.Errorf("fsbridge mkdir: %w", err) + } + if exitCode != 0 { + return fmt.Errorf("mkdir failed: %s", strings.TrimSpace(stderr)) + } + if err := b.validateParentBeforeCreate(ctx, dir); err != nil { + return err + } } - // Write content via stdin pipe - _, stderr, exitCode, err := b.dockerExec(ctx, []byte(content), "sh", "-c", fmt.Sprintf("cat %s %q", redir, resolved)) + + ddArgs := fsBridgeWriteDDArgs(resolved, appendMode) + _, stderr, exitCode, err := b.dockerExec(ctx, []byte(content), ddArgs...) if err != nil { return fmt.Errorf("fsbridge write: %w", err) } @@ -77,13 +92,25 @@ func (b *FsBridge) WriteFile(ctx context.Context, path, content string, appendMo return nil } +func fsBridgeWriteDDArgs(resolved string, appendMode bool) []string { + args := []string{"dd", "bs=1048576", "status=none", "of=" + resolved} + if appendMode { + args = append(args, "conv=notrunc", "oflag=append") + } + return args +} + // ListDir lists files and directories inside the container. // Matching TS FsBridge.readdir(). func (b *FsBridge) ListDir(ctx context.Context, path string) (string, error) { resolved := b.resolvePath(path) + realPath, err := b.resolveExistingPath(ctx, resolved) + if err != nil { + return "", err + } // Use ls -la for detailed listing - stdout, stderr, exitCode, err := b.dockerExec(ctx, nil, "ls", "-la", "--", resolved) + stdout, stderr, exitCode, err := b.dockerExec(ctx, nil, "ls", "-la", "--", realPath) if err != nil { return "", fmt.Errorf("fsbridge list: %w", err) } @@ -97,8 +124,12 @@ func (b *FsBridge) ListDir(ctx context.Context, path string) (string, error) { // Stat checks if a path exists and returns basic info. func (b *FsBridge) Stat(ctx context.Context, path string) (string, error) { resolved := b.resolvePath(path) + realPath, err := b.resolveExistingPath(ctx, resolved) + if err != nil { + return "", err + } - stdout, stderr, exitCode, err := b.dockerExec(ctx, nil, "stat", "--", resolved) + stdout, stderr, exitCode, err := b.dockerExec(ctx, nil, "stat", "--", realPath) if err != nil { return "", fmt.Errorf("fsbridge stat: %w", err) } @@ -112,20 +143,96 @@ func (b *FsBridge) Stat(ctx context.Context, path string) (string, error) { // resolvePath resolves a path relative to the container workdir. // Validates that absolute paths stay within the workdir (defense in depth). func (b *FsBridge) resolvePath(path string) string { + workdir := filepath.Clean(b.workdir) if path == "" || path == "." { - return b.workdir + return workdir } + var cleaned string if strings.HasPrefix(path, "/") { - // Validate absolute paths stay within workdir (defense in depth, - // container is already sandboxed with read-only FS + cap-drop ALL). - cleaned := filepath.Clean(path) - if cleaned == b.workdir || strings.HasPrefix(cleaned, b.workdir+"/") { - return cleaned + cleaned = filepath.Clean(path) + } else { + cleaned = filepath.Clean(filepath.Join(workdir, path)) + } + if cleaned == workdir || strings.HasPrefix(cleaned, workdir+"/") { + return cleaned + } + return workdir +} + +func fsBridgePathWithin(root, target string) bool { + root = filepath.Clean(root) + target = filepath.Clean(target) + if target == root { + return true + } + return strings.HasPrefix(target, root+"/") +} + +func (b *FsBridge) containerRealPath(ctx context.Context, path string) (string, error) { + stdout, stderr, exitCode, err := b.dockerExec(ctx, nil, "realpath", "-e", "--", path) + if err != nil { + return "", fmt.Errorf("fsbridge realpath: %w", err) + } + if exitCode != 0 { + return "", fmt.Errorf("realpath failed: %s", strings.TrimSpace(stderr)) + } + return strings.TrimSpace(stdout), nil +} + +func (b *FsBridge) containerRealWorkdir(ctx context.Context) (string, error) { + return b.containerRealPath(ctx, filepath.Clean(b.workdir)) +} + +func (b *FsBridge) resolveExistingPath(ctx context.Context, resolved string) (string, error) { + realWorkdir, err := b.containerRealWorkdir(ctx) + if err != nil { + return "", err + } + realPath, err := b.containerRealPath(ctx, resolved) + if err != nil { + return "", err + } + if !fsBridgePathWithin(realWorkdir, realPath) { + return "", fmt.Errorf("path escapes sandbox workdir") + } + return realPath, nil +} + +func (b *FsBridge) validateExistingTargetIfPresent(ctx context.Context, resolved string) error { + realWorkdir, err := b.containerRealWorkdir(ctx) + if err != nil { + return err + } + realPath, err := b.containerRealPath(ctx, resolved) + if err != nil { + return nil + } + if !fsBridgePathWithin(realWorkdir, realPath) { + return fmt.Errorf("path escapes sandbox workdir") + } + return nil +} + +func (b *FsBridge) validateParentBeforeCreate(ctx context.Context, dir string) error { + realWorkdir, err := b.containerRealWorkdir(ctx) + if err != nil { + return err + } + current := filepath.Clean(dir) + for { + realParent, err := b.containerRealPath(ctx, current) + if err == nil { + if !fsBridgePathWithin(realWorkdir, realParent) { + return fmt.Errorf("path parent escapes sandbox workdir") + } + return nil } - return b.workdir // fallback to workdir for escapes + next := filepath.Dir(current) + if next == current { + return fmt.Errorf("path parent does not exist inside sandbox workdir") + } + current = next } - // Relative paths: use filepath.Join for proper normalization - return filepath.Clean(filepath.Join(b.workdir, path)) } // dockerExec runs a command inside the container and returns stdout, stderr, exit code. diff --git a/internal/store/pg/webhook_calls.go b/internal/store/pg/webhook_calls.go index 329425bb..7cdb996d 100644 --- a/internal/store/pg/webhook_calls.go +++ b/internal/store/pg/webhook_calls.go @@ -56,11 +56,13 @@ func (s *PGWebhookCallStore) Create(ctx context.Context, call *store.WebhookCall `INSERT INTO webhook_calls (id, tenant_id, webhook_id, agent_id, delivery_id, idempotency_key, mode, status, callback_url, attempts, - next_attempt_at, request_payload, created_at) - VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13)`, + next_attempt_at, started_at, request_payload, response, last_error, + created_at, completed_at) + VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$10,$11,$12,$13,$14,$15,$16,$17)`, call.ID, call.TenantID, call.WebhookID, nilUUID(call.AgentID), call.DeliveryID, call.IdempotencyKey, call.Mode, call.Status, call.CallbackURL, call.Attempts, - call.NextAttemptAt, call.RequestPayload, call.CreatedAt, + call.NextAttemptAt, call.StartedAt, call.RequestPayload, call.Response, call.LastError, + call.CreatedAt, call.CompletedAt, ) if err != nil { // Map partial unique index violation (webhook_id, idempotency_key) → typed sentinel. @@ -141,6 +143,7 @@ func (s *PGWebhookCallStore) ClaimNext(ctx context.Context, tenantID uuid.UUID, err = tx.QueryRowContext(ctx, `SELECT id FROM webhook_calls WHERE tenant_id = $1 + AND mode = 'async' AND status = 'queued' AND (next_attempt_at IS NULL OR next_attempt_at <= $2) ORDER BY next_attempt_at ASC NULLS FIRST @@ -251,7 +254,7 @@ func (s *PGWebhookCallStore) ReclaimStale(ctx context.Context, staleThreshold ti res, err := s.db.ExecContext(ctx, `UPDATE webhook_calls SET status = 'queued', started_at = NULL, lease_token = NULL - WHERE status = 'running' AND started_at < $1`, + WHERE mode = 'async' AND status = 'running' AND started_at < $1`, staleThreshold, ) if err != nil { diff --git a/internal/store/sqlitestore/schema.go b/internal/store/sqlitestore/schema.go index 0266d667..ebdc8c3c 100644 --- a/internal/store/sqlitestore/schema.go +++ b/internal/store/sqlitestore/schema.go @@ -16,7 +16,7 @@ var schemaSQL string // SchemaVersion is the current SQLite schema version. // Bump this when adding new migration steps below. -const SchemaVersion = 36 +const SchemaVersion = 37 // migrations maps version → SQL to apply when upgrading FROM that version. // schema.sql always represents the LATEST full schema (for fresh DBs). @@ -559,6 +559,8 @@ CREATE TABLE IF NOT EXISTS agent_workstation_links ( created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), PRIMARY KEY (agent_id, workstation_id) ); +CREATE UNIQUE INDEX IF NOT EXISTS idx_agent_workstation_default + ON agent_workstation_links(agent_id) WHERE is_default = 1; CREATE INDEX IF NOT EXISTS idx_agent_workstation_tenant ON agent_workstation_links(tenant_id);`, // Version 31 → 32: workstation_permissions allowlist table. Mirrors PG migration 000063. @@ -610,6 +612,11 @@ WHERE id IN ( OR (s.is_system = 0 AND sag.tenant_id <> s.tenant_id) );`, + // Version 36 → 37: enforce one default workstation link per agent. + // Mirrors PG migration 000062 partial unique index. + 36: `CREATE UNIQUE INDEX IF NOT EXISTS idx_agent_workstation_default + ON agent_workstation_links(agent_id) WHERE is_default = 1;`, + // Version 23 → 24: vault_documents scope/ownership consistency triggers. // Mirrors PG migration 000055 CHECK constraint; SQLite cannot add CHECK via // ALTER TABLE so we use BEFORE INSERT + BEFORE UPDATE triggers instead. diff --git a/internal/store/sqlitestore/schema.sql b/internal/store/sqlitestore/schema.sql index 553e71ee..866627b3 100644 --- a/internal/store/sqlitestore/schema.sql +++ b/internal/store/sqlitestore/schema.sql @@ -1774,6 +1774,8 @@ CREATE TABLE IF NOT EXISTS agent_workstation_links ( created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ', 'now')), PRIMARY KEY (agent_id, workstation_id) ); +CREATE UNIQUE INDEX IF NOT EXISTS idx_agent_workstation_default + ON agent_workstation_links(agent_id) WHERE is_default = 1; CREATE INDEX IF NOT EXISTS idx_agent_workstation_tenant ON agent_workstation_links(tenant_id); -- ============================================================ diff --git a/internal/store/sqlitestore/schema_migration_test.go b/internal/store/sqlitestore/schema_migration_test.go index 2260ea46..3701a2e7 100644 --- a/internal/store/sqlitestore/schema_migration_test.go +++ b/internal/store/sqlitestore/schema_migration_test.go @@ -48,6 +48,42 @@ func TestEnsureSchema_FreshDB(t *testing.T) { t.Errorf("vault_documents missing column %q", want) } } + + for _, table := range []string{"hooks", "hook_agents"} { + var count int + if err := db.QueryRow(`SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name=?`, table).Scan(&count); err != nil { + t.Fatalf("lookup %s table: %v", table, err) + } + if count != 1 { + t.Errorf("fresh schema missing %q table", table) + } + } +} + +func TestEnsureSchema_PreHooksUpgradeCreatesHookTables(t *testing.T) { + db := openTestDBAtVersion(t, 19) + for _, table := range []string{"tenant_hook_budget", "hook_executions", "hook_agents", "hooks"} { + if _, err := db.Exec(`DROP TABLE IF EXISTS ` + table); err != nil { + t.Fatalf("drop %s: %v", table, err) + } + } + if _, err := db.Exec(`UPDATE schema_version SET version = 19`); err != nil { + t.Fatalf("set pre-hooks schema version: %v", err) + } + + if err := EnsureSchema(db); err != nil { + t.Fatalf("EnsureSchema (pre-hooks to current) failed: %v", err) + } + + for _, table := range []string{"hooks", "hook_agents"} { + var count int + if err := db.QueryRow(`SELECT COUNT(*) FROM sqlite_master WHERE type='table' AND name=?`, table).Scan(&count); err != nil { + t.Fatalf("lookup %s table: %v", table, err) + } + if count != 1 { + t.Errorf("upgrade schema missing %q table", table) + } + } } // TestEnsureSchema_MigrationV11Only verifies migrations from v11 onward diff --git a/internal/store/sqlitestore/webhook_calls.go b/internal/store/sqlitestore/webhook_calls.go index 4b736a41..dddcf5ea 100644 --- a/internal/store/sqlitestore/webhook_calls.go +++ b/internal/store/sqlitestore/webhook_calls.go @@ -70,11 +70,13 @@ func (s *SQLiteWebhookCallStore) Create(ctx context.Context, call *store.Webhook `INSERT INTO webhook_calls (id, tenant_id, webhook_id, agent_id, delivery_id, idempotency_key, mode, status, callback_url, attempts, - next_attempt_at, request_payload, created_at) - VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?)`, + next_attempt_at, started_at, request_payload, response, last_error, + created_at, completed_at) + VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)`, call.ID, call.TenantID, call.WebhookID, nilUUID(call.AgentID), call.DeliveryID, call.IdempotencyKey, call.Mode, call.Status, call.CallbackURL, call.Attempts, - call.NextAttemptAt, call.RequestPayload, call.CreatedAt, + call.NextAttemptAt, call.StartedAt, call.RequestPayload, call.Response, call.LastError, + call.CreatedAt, call.CompletedAt, ) if err != nil { // Map partial unique index violation (webhook_id, idempotency_key) → typed sentinel. @@ -154,6 +156,7 @@ func (s *SQLiteWebhookCallStore) ClaimNext(ctx context.Context, tenantID uuid.UU err = tx.QueryRowContext(ctx, `SELECT id FROM webhook_calls WHERE tenant_id = ? + AND mode = 'async' AND status = 'queued' AND (next_attempt_at IS NULL OR next_attempt_at <= ?) ORDER BY next_attempt_at ASC @@ -265,7 +268,7 @@ func (s *SQLiteWebhookCallStore) ReclaimStale(ctx context.Context, staleThreshol res, err := s.db.ExecContext(ctx, `UPDATE webhook_calls SET status = 'queued', started_at = NULL, lease_token = NULL - WHERE status = 'running' AND started_at < ?`, + WHERE mode = 'async' AND status = 'running' AND started_at < ?`, staleThreshold, ) if err != nil { diff --git a/internal/store/sqlitestore/webhooks_test.go b/internal/store/sqlitestore/webhooks_test.go index 675632ad..bff57aa8 100644 --- a/internal/store/sqlitestore/webhooks_test.go +++ b/internal/store/sqlitestore/webhooks_test.go @@ -160,6 +160,25 @@ func TestWebhookCallClaimNextSkipsRunningAndDone(t *testing.T) { t.Errorf("expected ErrNoRows when no queued rows, got: %v", err) } + // A queued sync audit row is not worker-owned and must not be claimed. + syncQueued := &store.WebhookCallData{ + ID: uuid.New(), + TenantID: tenantID, + WebhookID: wh.ID, + DeliveryID: uuid.New(), + Mode: "sync", + Status: "queued", + Attempts: 0, + CreatedAt: now, + } + if err := cs.Create(ctx, syncQueued); err != nil { + t.Fatalf("Create queued sync call: %v", err) + } + _, err = cs.ClaimNext(ctx, tenantID, now) + if err != sql.ErrNoRows { + t.Errorf("expected ErrNoRows for queued sync row, got: %v", err) + } + // Insert a queued call due now. queued := &store.WebhookCallData{ ID: uuid.New(), @@ -194,6 +213,65 @@ func TestWebhookCallClaimNextSkipsRunningAndDone(t *testing.T) { } } +func TestWebhookCallReclaimStaleOnlyAsync(t *testing.T) { + db := openTestWebhookDB(t) + ws := NewSQLiteWebhookStore(db) + cs := NewSQLiteWebhookCallStore(db) + + tenantID := uuid.New() + ctx := testTenantCtx(tenantID) + wh := &store.WebhookData{ + ID: uuid.New(), TenantID: tenantID, Name: "wh-reclaim", Kind: "llm", + SecretHash: "h-reclaim", Scopes: []string{}, IPAllowlist: []string{}, + RateLimitPerMin: 60, CreatedAt: time.Now().UTC(), UpdatedAt: time.Now().UTC(), + } + if err := ws.Create(ctx, wh); err != nil { + t.Fatalf("Create webhook: %v", err) + } + + stale := time.Now().UTC().Add(-time.Hour) + rows := []struct { + mode string + id uuid.UUID + }{ + {mode: "sync", id: uuid.New()}, + {mode: "async", id: uuid.New()}, + } + for _, row := range rows { + _, err := db.ExecContext(ctx, + `INSERT INTO webhook_calls (id,tenant_id,webhook_id,delivery_id,mode,status,attempts,created_at,started_at) + VALUES (?,?,?,?,?,?,?,?,?)`, + row.id, tenantID, wh.ID, uuid.New(), row.mode, "running", 0, stale, stale, + ) + if err != nil { + t.Fatalf("insert %s row: %v", row.mode, err) + } + } + + n, err := cs.ReclaimStale(ctx, time.Now().UTC()) + if err != nil { + t.Fatalf("ReclaimStale: %v", err) + } + if n != 1 { + t.Fatalf("reclaimed %d rows, want 1", n) + } + + var syncStatus string + if err := db.QueryRowContext(ctx, `SELECT status FROM webhook_calls WHERE id = ?`, rows[0].id).Scan(&syncStatus); err != nil { + t.Fatalf("select sync row: %v", err) + } + if syncStatus != "running" { + t.Fatalf("sync row status = %q, want running", syncStatus) + } + var asyncStatus string + if err := db.QueryRowContext(ctx, `SELECT status FROM webhook_calls WHERE id = ?`, rows[1].id).Scan(&asyncStatus); err != nil { + t.Fatalf("select async row: %v", err) + } + if asyncStatus != "queued" { + t.Fatalf("async row status = %q, want queued", asyncStatus) + } +} + // TestWebhookCallIdempotencyConflict verifies duplicate (webhook_id, idempotency_key) // returns ErrIdempotencyConflict. func TestWebhookCallIdempotencyConflict(t *testing.T) { diff --git a/internal/store/workstation_permission_store.go b/internal/store/workstation_permission_store.go index 18c8f06e..22909fc5 100644 --- a/internal/store/workstation_permission_store.go +++ b/internal/store/workstation_permission_store.go @@ -50,6 +50,6 @@ type WorkstationPermissionStore interface { // NOTE: shells (bash, sh, zsh) are intentionally excluded — adding a shell binary // bypasses all protection by allowing arbitrary commands as arguments. var DefaultAllowedBinaries = []string{ - "echo", "pwd", "ls", "cat", "git", "env", + "echo", "pwd", "ls", "cat", "git", "whoami", "hostname", "date", "uname", "claude", } diff --git a/internal/tools/edit.go b/internal/tools/edit.go index a08c5b44..c9a5ac90 100644 --- a/internal/tools/edit.go +++ b/internal/tools/edit.go @@ -16,8 +16,8 @@ import ( type EditTool struct { workspace string restrict bool - allowedPrefixes []string // extra allowed path prefixes (cross-drive on Windows) - deniedPrefixes []string // path prefixes to deny access to (e.g. .goclaw) + allowedPrefixes []string // extra allowed path prefixes (cross-drive on Windows) + deniedPrefixes []string // path prefixes to deny access to (e.g. .goclaw) sandboxMgr sandbox.Manager contextFileIntc *ContextFileInterceptor memIntc *MemoryInterceptor @@ -218,7 +218,7 @@ func (t *EditTool) executeInSandbox(ctx context.Context, path, oldStr, newStr st } containerPath := ResolveSandboxPath(path, containerCwd) - bridge := sandbox.NewFsBridge(sb.ID(), sandbox.DefaultContainerWorkdir) + bridge := sandbox.NewFsBridge(sb.ID(), containerCwd) content, err := bridge.ReadFile(ctx, containerPath) if err != nil { return ErrorResult(fmt.Sprintf("failed to read file: %v", err) + MaybeFsBridgeHint(err)) diff --git a/internal/tools/filesystem.go b/internal/tools/filesystem.go index ea7b0435..20e2fe2b 100644 --- a/internal/tools/filesystem.go +++ b/internal/tools/filesystem.go @@ -23,15 +23,15 @@ var virtualSystemFiles = map[string]string{ // ReadFileTool reads file contents, optionally through a sandbox container. type ReadFileTool struct { - workspace string - restrict bool - allowedPrefixes []string // extra allowed path prefixes (e.g. skills dirs) - deniedPrefixes []string // path prefixes to deny access to (e.g. .goclaw) - sandboxMgr sandbox.Manager // nil = direct host access - contextFileIntc *ContextFileInterceptor // nil = no virtual FS routing - memIntc *MemoryInterceptor // nil = no memory routing - permStore store.ConfigPermissionStore // nil = no group read restriction - vaultIntc *VaultInterceptor // nil = no vault lazy sync + workspace string + restrict bool + allowedPrefixes []string // extra allowed path prefixes (e.g. skills dirs) + deniedPrefixes []string // path prefixes to deny access to (e.g. .goclaw) + sandboxMgr sandbox.Manager // nil = direct host access + contextFileIntc *ContextFileInterceptor // nil = no virtual FS routing + memIntc *MemoryInterceptor // nil = no memory routing + permStore store.ConfigPermissionStore // nil = no group read restriction + vaultIntc *VaultInterceptor // nil = no vault lazy sync } // SetContextFileInterceptor enables virtual FS routing for context files. @@ -196,15 +196,14 @@ func (t *ReadFileTool) Execute(ctx context.Context, args map[string]any) *Result } func (t *ReadFileTool) executeInSandbox(ctx context.Context, path, sandboxKey string, args map[string]any) *Result { - bridge, err := t.getFsBridge(ctx, sandboxKey) - if err != nil { - return ErrorResult(fmt.Sprintf("sandbox error: %v", err)) - } - containerCwd, cwdErr := SandboxCwd(ctx, t.workspace, sandbox.DefaultContainerWorkdir) if cwdErr != nil { return ErrorResult(fmt.Sprintf("sandbox path mapping: %v", cwdErr)) } + bridge, err := t.getFsBridge(ctx, sandboxKey, containerCwd) + if err != nil { + return ErrorResult(fmt.Sprintf("sandbox error: %v", err)) + } containerPath := ResolveSandboxPath(path, containerCwd) data, err := bridge.ReadFile(ctx, containerPath) @@ -215,12 +214,12 @@ func (t *ReadFileTool) executeInSandbox(ctx context.Context, path, sandboxKey st return t.paginateOutput(data, args) } -func (t *ReadFileTool) getFsBridge(ctx context.Context, sandboxKey string) (*sandbox.FsBridge, error) { +func (t *ReadFileTool) getFsBridge(ctx context.Context, sandboxKey, containerCwd string) (*sandbox.FsBridge, error) { sb, err := t.sandboxMgr.Get(ctx, sandboxKey, t.workspace, SandboxConfigFromCtx(ctx)) if err != nil { return nil, err } - return sandbox.NewFsBridge(sb.ID(), sandbox.DefaultContainerWorkdir), nil + return sandbox.NewFsBridge(sb.ID(), containerCwd), nil } // readFileMaxChars is the output cap for read_file. Large files require offset/limit pagination. @@ -582,4 +581,3 @@ func resolveThroughExistingAncestors(target string) (string, error) { } return filepath.Clean(target), nil } - diff --git a/internal/tools/filesystem_list.go b/internal/tools/filesystem_list.go index 45a94266..02c20969 100644 --- a/internal/tools/filesystem_list.go +++ b/internal/tools/filesystem_list.go @@ -140,15 +140,14 @@ func (t *ListFilesTool) Execute(ctx context.Context, args map[string]any) *Resul } func (t *ListFilesTool) executeInSandbox(ctx context.Context, path, sandboxKey string) *Result { - bridge, err := t.getFsBridge(ctx, sandboxKey) - if err != nil { - return ErrorResult(fmt.Sprintf("sandbox error: %v", err)) - } - containerCwd, cwdErr := SandboxCwd(ctx, t.workspace, sandbox.DefaultContainerWorkdir) if cwdErr != nil { return ErrorResult(fmt.Sprintf("sandbox path mapping: %v", cwdErr)) } + bridge, err := t.getFsBridge(ctx, sandboxKey, containerCwd) + if err != nil { + return ErrorResult(fmt.Sprintf("sandbox error: %v", err)) + } containerPath := ResolveSandboxPath(path, containerCwd) output, err := bridge.ListDir(ctx, containerPath) @@ -159,10 +158,10 @@ func (t *ListFilesTool) executeInSandbox(ctx context.Context, path, sandboxKey s return SilentResult(output) } -func (t *ListFilesTool) getFsBridge(ctx context.Context, sandboxKey string) (*sandbox.FsBridge, error) { +func (t *ListFilesTool) getFsBridge(ctx context.Context, sandboxKey, containerCwd string) (*sandbox.FsBridge, error) { sb, err := t.sandboxMgr.Get(ctx, sandboxKey, t.workspace, SandboxConfigFromCtx(ctx)) if err != nil { return nil, err } - return sandbox.NewFsBridge(sb.ID(), sandbox.DefaultContainerWorkdir), nil + return sandbox.NewFsBridge(sb.ID(), containerCwd), nil } diff --git a/internal/tools/filesystem_write.go b/internal/tools/filesystem_write.go index f5afa1a8..8f0dff2b 100644 --- a/internal/tools/filesystem_write.go +++ b/internal/tools/filesystem_write.go @@ -15,8 +15,8 @@ import ( type WriteFileTool struct { workspace string restrict bool - allowedPrefixes []string // extra allowed path prefixes (cross-drive on Windows) - deniedPrefixes []string // path prefixes to deny access to (e.g. .goclaw) + allowedPrefixes []string // extra allowed path prefixes (cross-drive on Windows) + deniedPrefixes []string // path prefixes to deny access to (e.g. .goclaw) sandboxMgr sandbox.Manager contextFileIntc *ContextFileInterceptor // nil = no virtual FS routing memIntc *MemoryInterceptor // nil = no memory routing @@ -240,15 +240,14 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]any) *Resul } func (t *WriteFileTool) executeInSandbox(ctx context.Context, path, content, sandboxKey string, deliver, appendMode bool) *Result { - bridge, err := t.getFsBridge(ctx, sandboxKey) - if err != nil { - return ErrorResult(fmt.Sprintf("sandbox error: %v", err)) - } - containerCwd, cwdErr := SandboxCwd(ctx, t.workspace, sandbox.DefaultContainerWorkdir) if cwdErr != nil { return ErrorResult(fmt.Sprintf("sandbox path mapping: %v", cwdErr)) } + bridge, err := t.getFsBridge(ctx, sandboxKey, containerCwd) + if err != nil { + return ErrorResult(fmt.Sprintf("sandbox error: %v", err)) + } containerPath := ResolveSandboxPath(path, containerCwd) if err := bridge.WriteFile(ctx, containerPath, content, appendMode); err != nil { @@ -284,10 +283,10 @@ func (t *WriteFileTool) executeInSandbox(ctx context.Context, path, content, san return result } -func (t *WriteFileTool) getFsBridge(ctx context.Context, sandboxKey string) (*sandbox.FsBridge, error) { +func (t *WriteFileTool) getFsBridge(ctx context.Context, sandboxKey, containerCwd string) (*sandbox.FsBridge, error) { sb, err := t.sandboxMgr.Get(ctx, sandboxKey, t.workspace, SandboxConfigFromCtx(ctx)) if err != nil { return nil, err } - return sandbox.NewFsBridge(sb.ID(), sandbox.DefaultContainerWorkdir), nil + return sandbox.NewFsBridge(sb.ID(), containerCwd), nil } diff --git a/internal/tools/sandbox_utils.go b/internal/tools/sandbox_utils.go index 94e45dbb..18885326 100644 --- a/internal/tools/sandbox_utils.go +++ b/internal/tools/sandbox_utils.go @@ -35,12 +35,21 @@ func SandboxCwd(ctx context.Context, globalWorkspace, containerBase string) (str } // ResolveSandboxPath resolves a tool-provided path (relative or absolute) -// against the sandbox container CWD. If the path is relative, it is joined -// with containerCwd. Absolute paths are returned as-is (the sandbox -// filesystem already restricts access to the mounted volume). +// against the sandbox container CWD. Escapes are rejected to containerCwd so a +// tool scoped to /workspace/agent-a cannot address /workspace/agent-b. func ResolveSandboxPath(filePath, containerCwd string) string { - if strings.HasPrefix(filePath, "/") { - return filePath + cwd := path.Clean(containerCwd) + if cwd == "." || cwd == "/" { + cwd = "/workspace" } - return path.Join(containerCwd, filePath) + var resolved string + if strings.HasPrefix(filePath, "/") { + resolved = path.Clean(filePath) + } else { + resolved = path.Clean(path.Join(cwd, filePath)) + } + if resolved == cwd || strings.HasPrefix(resolved, cwd+"/") { + return resolved + } + return cwd } diff --git a/internal/tools/sandbox_utils_test.go b/internal/tools/sandbox_utils_test.go index 0fb9d4f9..a1b49009 100644 --- a/internal/tools/sandbox_utils_test.go +++ b/internal/tools/sandbox_utils_test.go @@ -109,11 +109,23 @@ func TestResolveSandboxPath(t *testing.T) { want: "/workspace/agent-a/subdir/file.txt", }, { - name: "absolute path passed through", + name: "absolute sibling workspace path is rejected to cwd", path: "/workspace/agent-a/file.txt", containerCwd: "/workspace/agent-b", + want: "/workspace/agent-b", + }, + { + name: "absolute path inside cwd stays absolute", + path: "/workspace/agent-a/file.txt", + containerCwd: "/workspace/agent-a", want: "/workspace/agent-a/file.txt", }, + { + name: "relative parent escape is rejected to cwd", + path: "../agent-b/file.txt", + containerCwd: "/workspace/agent-a", + want: "/workspace/agent-a", + }, { name: "dot path", path: ".", diff --git a/internal/tools/workstation_exec.go b/internal/tools/workstation_exec.go index 2c8567c2..d64f2e7b 100644 --- a/internal/tools/workstation_exec.go +++ b/internal/tools/workstation_exec.go @@ -372,10 +372,26 @@ func (t *WorkstationExecTool) streamAndCollect( wg.Add(2) go readStream(stream.Stdout(), "stdout", &stdoutTail) go readStream(stream.Stderr(), "stderr", &stderrTail) + readersDone := make(chan struct{}) + var killOnce sync.Once + go func() { + select { + case <-ctx.Done(): + killOnce.Do(func() { _ = stream.Kill() }) + case <-readersDone: + } + }() wg.Wait() + close(readersDone) exitCode, waitErr := stream.Wait() durationMs := time.Since(startTime).Milliseconds() + if ctx.Err() != nil { + killOnce.Do(func() { _ = stream.Kill() }) + if waitErr == nil { + waitErr = ctx.Err() + } + } // Emit done event. if t.eventBus != nil { diff --git a/internal/tools/workstation_exec_test.go b/internal/tools/workstation_exec_test.go new file mode 100644 index 00000000..7d36092a --- /dev/null +++ b/internal/tools/workstation_exec_test.go @@ -0,0 +1,85 @@ +package tools + +import ( + "context" + "errors" + "io" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/store" +) + +type blockingWorkstationStream struct { + stdoutR *io.PipeReader + stdoutW *io.PipeWriter + stderrR *io.PipeReader + stderrW *io.PipeWriter + killN atomic.Int64 + once sync.Once + done chan struct{} +} + +func newBlockingWorkstationStream() *blockingWorkstationStream { + stdoutR, stdoutW := io.Pipe() + stderrR, stderrW := io.Pipe() + return &blockingWorkstationStream{ + stdoutR: stdoutR, + stdoutW: stdoutW, + stderrR: stderrR, + stderrW: stderrW, + done: make(chan struct{}), + } +} + +func (s *blockingWorkstationStream) Stdout() io.Reader { return s.stdoutR } + +func (s *blockingWorkstationStream) Stderr() io.Reader { return s.stderrR } + +func (s *blockingWorkstationStream) Wait() (int, error) { + <-s.done + return 137, errors.New("killed") +} + +func (s *blockingWorkstationStream) Kill() error { + s.killN.Add(1) + s.once.Do(func() { + _ = s.stdoutW.CloseWithError(context.Canceled) + _ = s.stderrW.CloseWithError(context.Canceled) + close(s.done) + }) + return nil +} + +func TestStreamAndCollectTimeoutKillsBlockedReaders(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Millisecond) + defer cancel() + + stream := newBlockingWorkstationStream() + tool := &WorkstationExecTool{} + ws := &store.Workstation{ + ID: uuid.New(), + TenantID: uuid.New(), + } + + done := make(chan *Result, 1) + go func() { + done <- tool.streamAndCollect(ctx, stream, ws, uuid.NewString(), "session-timeout", "sleep 60") + }() + + select { + case result := <-done: + if !result.IsError { + t.Fatalf("expected timeout result to be an error, got %#v", result) + } + if stream.killN.Load() == 0 { + t.Fatal("expected timed-out stream to be killed") + } + case <-time.After(time.Second): + t.Fatal("streamAndCollect did not return after context timeout") + } +} diff --git a/internal/webhooks/worker.go b/internal/webhooks/worker.go index dfd88df3..1d6913ff 100644 --- a/internal/webhooks/worker.go +++ b/internal/webhooks/worker.go @@ -73,6 +73,22 @@ type asyncPayload struct { Metadata json.RawMessage `json:"metadata,omitempty"` } +func decodeAsyncPayload(payload []byte) (asyncPayload, error) { + var envelope struct { + BodyHash string `json:"body_hash"` + Meta json.RawMessage `json:"meta"` + } + if err := json.Unmarshal(payload, &envelope); err == nil && envelope.BodyHash != "" && len(envelope.Meta) > 0 { + payload = envelope.Meta + } + + var req asyncPayload + if err := json.Unmarshal(payload, &req); err != nil { + return asyncPayload{}, err + } + return req, nil +} + // callbackPayload is the JSON body POSTed to the receiver's callback_url. type callbackPayload struct { CallID string `json:"call_id"` @@ -300,8 +316,8 @@ func (w *WebhookWorker) execute(ctx context.Context, call *store.WebhookCallData }() // Decode stored request payload. - var req asyncPayload - if err := json.Unmarshal(call.RequestPayload, &req); err != nil { + req, err := decodeAsyncPayload(call.RequestPayload) + if err != nil { slog.Error("webhook.worker.payload_decode_failed", "call_id", call.ID, "error", err, diff --git a/internal/webhooks/worker_test.go b/internal/webhooks/worker_test.go index 2ee4dc48..3bc9c5d1 100644 --- a/internal/webhooks/worker_test.go +++ b/internal/webhooks/worker_test.go @@ -169,6 +169,35 @@ func newTestCall(callbackURL string, agentID *uuid.UUID) *store.WebhookCallData return call } +func TestDecodeAsyncPayload_UnwrapsAuditEnvelope(t *testing.T) { + meta := asyncPayload{ + Input: json.RawMessage(`"hello"`), + CallbackURL: "https://example.com/callback", + } + metaBytes, err := json.Marshal(meta) + if err != nil { + t.Fatalf("marshal meta: %v", err) + } + envelope, err := json.Marshal(map[string]any{ + "body_hash": "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", + "meta": json.RawMessage(metaBytes), + }) + if err != nil { + t.Fatalf("marshal envelope: %v", err) + } + + got, err := decodeAsyncPayload(envelope) + if err != nil { + t.Fatalf("decodeAsyncPayload: %v", err) + } + if string(got.Input) != `"hello"` { + t.Fatalf("input = %s, want %s", got.Input, `"hello"`) + } + if got.CallbackURL != meta.CallbackURL { + t.Fatalf("callback_url = %q, want %q", got.CallbackURL, meta.CallbackURL) + } +} + // newTestWebhook creates a webhook with an encrypted raw secret. // Returns the webhook and the raw secret bytes for signature verification. // encKey is the AES-256-GCM key (same as testEncKey). diff --git a/internal/workstation/security/allowlist.go b/internal/workstation/security/allowlist.go index 2db2fde8..97fc5955 100644 --- a/internal/workstation/security/allowlist.go +++ b/internal/workstation/security/allowlist.go @@ -112,6 +112,10 @@ func (c *AllowlistChecker) Check( c.auditDeny(ws, cmd, "empty_binary_name") return errors.New(i18n.T(locale, i18n.MsgWorkstationCmdDenied, "empty binary name")) } + if reason := validateLauncherArgs(binaryName, args); reason != "" { + c.auditDeny(ws, cmd, reason) + return errors.New(i18n.T(locale, i18n.MsgWorkstationCmdDenied, reason)) + } patterns, err := c.loadAllowlist(ctx, ws.ID) if err != nil { @@ -186,6 +190,16 @@ func isBlockedEnvKey(k string) bool { return strings.HasPrefix(k, "GOCLAW_") } +func validateLauncherArgs(binaryName string, args []string) string { + switch binaryName { + case "env", "nohup", "setsid", "timeout", "nice", "stdbuf", "xargs": + if len(args) > 0 { + return "launcher command with arguments denied: " + binaryName + } + } + return "" +} + // loadAllowlist returns the enabled binary name patterns for workstationID. // Results are cached for cacheTTL; evicted by Invalidate(). func (c *AllowlistChecker) loadAllowlist(ctx context.Context, workstationID uuid.UUID) ([]string, error) { diff --git a/internal/workstation/security/allowlist_test.go b/internal/workstation/security/allowlist_test.go new file mode 100644 index 00000000..b295f74e --- /dev/null +++ b/internal/workstation/security/allowlist_test.go @@ -0,0 +1,15 @@ +package security + +import "testing" + +func TestValidateLauncherArgsDeniesEnvCommandLaunch(t *testing.T) { + if reason := validateLauncherArgs("env", []string{"bash", "-lc", "id"}); reason == "" { + t.Fatal("expected env with command args to be denied") + } +} + +func TestValidateLauncherArgsAllowsPlainNonLauncherCommand(t *testing.T) { + if reason := validateLauncherArgs("git", []string{"status"}); reason != "" { + t.Fatalf("expected git args to be allowed, got %q", reason) + } +} diff --git a/ui/web/src/pages/workstations/workstation-create-dialog.tsx b/ui/web/src/pages/workstations/workstation-create-dialog.tsx index e66b1bb8..563821a8 100644 --- a/ui/web/src/pages/workstations/workstation-create-dialog.tsx +++ b/ui/web/src/pages/workstations/workstation-create-dialog.tsx @@ -154,8 +154,8 @@ export function WorkstationCreateDialog({ {backend === "ssh" && ( <> -
-
+
+