From fc105d31a16ba8119854c4e2a4f02d9df799f9cb Mon Sep 17 00:00:00 2001 From: Aaron Vu <54255926+aaron-tsar@users.noreply.github.com> Date: Mon, 25 May 2026 14:25:56 +0700 Subject: [PATCH 01/13] fix(vault): accept and persist content on POST/PUT vault/documents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously POST /v1/{agents/{id}/}vault/documents and the matching PUT endpoint declared a body struct without a `content` field, so callers passing JSON `{"path": ..., "title": ..., "content": "..."}` got back HTTP 201 with a doc id, but the bytes were silently discarded: - bindJSON ignored the `content` key (no matching field) - the document row was inserted with content_hash="" and no file on disk - the enrichment pipeline (summary, embeddings, links) was never triggered because no EventVaultDocUpserted was published The endpoint was effectively a metadata-only create — there was no client path that combined "register doc + supply bytes" via JSON. /v1/vault/upload exists, but it requires multipart, flattens paths to basename, and is inconvenient for programmatic clients that already have UTF-8 text in memory (CI jobs, agent skills, MCP tools). This change makes `content` an optional field on both handlers: - POST: when content is non-empty, validate the file extension against the same allowedUploadExts whitelist used by /v1/vault/upload, write bytes to /, compute SHA-256, store it as content_hash on the doc, and publish EventVaultDocUpserted so the enrichment worker picks it up — exactly the same downstream behaviour as /v1/vault/upload. - PUT: same logic; nil content = no change, non-nil content = rewrite the file on disk (and an empty string truncates it). - Omitting `content` preserves the historical metadata-only behaviour (e.g. for callers that materialise files via filesystem rescan). - Path validation reuses the existing rules: no `..`, no absolute paths; an extra abs-path check rejects symlink escapes from the workspace root. Two small helpers (writeDocumentContent, publishDocUpserted) factor out the disk write + event publish so both handlers share the same code, and so they can be unit-tested in isolation later. The fix is backwards-compatible: existing callers that don't send `content` continue to work unchanged. Existing docs created without content remain metadata-only stubs; a subsequent PUT with `content` will materialise them. Verified live on a development instance: POST → row inserted, file at /, SHA-256 matches, enrichment event published. PUT → file rewritten, content_hash updated, updated_at refreshed. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/http/vault_handler_documents.go | 164 +++++++++++++++++++++++ 1 file changed, 164 insertions(+) diff --git a/internal/http/vault_handler_documents.go b/internal/http/vault_handler_documents.go index 1a589f1e..fbe08d4f 100644 --- a/internal/http/vault_handler_documents.go +++ b/internal/http/vault_handler_documents.go @@ -3,11 +3,73 @@ package http import ( "log/slog" "net/http" + "os" + "path/filepath" "strings" + "time" + "github.com/google/uuid" + "github.com/nextlevelbuilder/goclaw/internal/eventbus" "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/internal/vault" ) +// writeDocumentContent writes the given content bytes to a tenant-workspace- +// relative path, after validating the path stays inside the workspace and uses +// an allowed extension. Returns the SHA-256 content hash on success. +// +// This is the shared writer used by handleCreateDocument and handleUpdateDocument +// to materialise inline JSON `content` payloads on disk, so the enrichment +// pipeline (which reads files by path) can later compute summaries, embeddings +// and links. +func (h *VaultHandler) writeDocumentContent(workspace, relPath string, content []byte) (string, error) { + target := filepath.Join(workspace, filepath.Clean(relPath)) + // Symlink/escape protection: the cleaned target must stay inside workspace. + absTarget, err := filepath.Abs(target) + if err != nil { + return "", err + } + absWS, err := filepath.Abs(workspace) + if err != nil { + return "", err + } + if !strings.HasPrefix(absTarget, absWS+string(os.PathSeparator)) && absTarget != absWS { + return "", os.ErrInvalid + } + if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { + return "", err + } + if err := os.WriteFile(target, content, 0o644); err != nil { + return "", err + } + return vault.ContentHash(content), nil +} + +// publishDocUpserted enqueues the standard EventVaultDocUpserted so the +// enrichment worker re-runs summary/embedding/link extraction for this doc. +// No-op when the event bus is not wired (some test setups). +func (h *VaultHandler) publishDocUpserted(tenantID, agentID, docID, relPath, hash, workspace string) { + if h.eventBus == nil { + return + } + h.eventBus.Publish(eventbus.DomainEvent{ + ID: uuid.Must(uuid.NewV7()).String(), + Type: eventbus.EventVaultDocUpserted, + SourceID: docID + ":" + hash, + TenantID: tenantID, + AgentID: agentID, + Timestamp: time.Now(), + Payload: eventbus.VaultDocUpsertedPayload{ + DocID: docID, + TenantID: tenantID, + AgentID: agentID, + Path: relPath, + ContentHash: hash, + Workspace: workspace, + }, + }) +} + // handleListAllDocuments lists vault documents across all agents in tenant. // Optional query param agent_id to filter by specific agent. func (h *VaultHandler) handleListAllDocuments(w http.ResponseWriter, r *http.Request) { @@ -99,6 +161,16 @@ func (h *VaultHandler) handleGetDocument(w http.ResponseWriter, r *http.Request) } // handleCreateDocument creates a new vault document. +// +// When `content` is supplied in the JSON body, the bytes are materialised on +// disk at / and the SHA-256 hash is stored on the +// document record. An EventVaultDocUpserted is then emitted so the enrichment +// worker computes summary/embeddings/links — same code path the multipart +// /v1/vault/upload endpoint and the filesystem rescan use. +// +// Omitting `content` preserves the historical behaviour of registering a +// metadata-only stub (typically followed by a rescan that materialises the +// file from disk). func (h *VaultHandler) handleCreateDocument(w http.ResponseWriter, r *http.Request) { locale := extractLocale(r) tenantID := store.TenantIDFromContext(r.Context()) @@ -107,6 +179,7 @@ func (h *VaultHandler) handleCreateDocument(w http.ResponseWriter, r *http.Reque var body struct { Path string `json:"path"` Title string `json:"title"` + Content string `json:"content"` // optional; when set, written to disk DocType string `json:"doc_type"` Scope string `json:"scope"` TeamID string `json:"team_id"` @@ -137,6 +210,16 @@ func (h *VaultHandler) handleCreateDocument(w http.ResponseWriter, r *http.Reque writeJSON(w, http.StatusBadRequest, map[string]string{"error": "invalid scope"}) return } + // Validate extension upfront if caller wants to write content — matches the + // whitelist enforced by /v1/vault/upload so the two endpoints accept the + // same file types. + if body.Content != "" { + ext := strings.ToLower(filepath.Ext(body.Path)) + if !allowedUploadExts[ext] { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": "unsupported file type: " + ext}) + return + } + } doc := &store.VaultDocument{ TenantID: tenantID.String(), @@ -160,11 +243,50 @@ func (h *VaultHandler) handleCreateDocument(w http.ResponseWriter, r *http.Reque doc.Scope = "team" } } + + // Persist file content if provided, before the DB upsert so the + // content_hash column is set in a single write. + var ( + wsPath string + writtenHash string + contentWasSet bool + ) + if body.Content != "" { + wsPath = h.resolveTenantWorkspace(r.Context()) + if wsPath == "" { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": "workspace not available"}) + return + } + hash, werr := h.writeDocumentContent(wsPath, body.Path, []byte(body.Content)) + if werr != nil { + slog.Warn("vault.create: write content failed", "path", body.Path, "error", werr) + if werr == os.ErrInvalid { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": "path escapes workspace"}) + return + } + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": "failed to write content"}) + return + } + doc.ContentHash = hash + writtenHash = hash + contentWasSet = true + } + if err := h.store.UpsertDocument(r.Context(), doc); err != nil { slog.Warn("vault.create failed", "error", err) writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) return } + + // Fire enrichment after the DB row exists so the worker can locate the doc. + if contentWasSet { + evAgent := "" + if doc.AgentID != nil { + evAgent = *doc.AgentID + } + h.publishDocUpserted(tenantID.String(), evAgent, doc.ID, body.Path, writtenHash, wsPath) + } + // Re-fetch by ID (set via RETURNING) — unambiguous even when same path exists across teams. created, _ := h.store.GetDocumentByID(r.Context(), tenantID.String(), doc.ID) if created != nil { @@ -189,6 +311,7 @@ func (h *VaultHandler) handleUpdateDocument(w http.ResponseWriter, r *http.Reque var body struct { Title *string `json:"title"` + Content *string `json:"content"` // nil=no change; "" clears the file; non-empty rewrites it DocType *string `json:"doc_type"` Scope *string `json:"scope"` TeamID *string `json:"team_id"` // nil=no change, ""=clear, "uuid"=set @@ -225,11 +348,52 @@ func (h *VaultHandler) handleUpdateDocument(w http.ResponseWriter, r *http.Reque existing.Metadata = body.Metadata } + // Rewrite content on disk when caller supplied a `content` field. + var ( + wsPath string + writtenHash string + contentWasSet bool + ) + if body.Content != nil { + ext := strings.ToLower(filepath.Ext(existing.Path)) + if !allowedUploadExts[ext] { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": "unsupported file type: " + ext}) + return + } + wsPath = h.resolveTenantWorkspace(r.Context()) + if wsPath == "" { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": "workspace not available"}) + return + } + hash, werr := h.writeDocumentContent(wsPath, existing.Path, []byte(*body.Content)) + if werr != nil { + slog.Warn("vault.update: write content failed", "path", existing.Path, "error", werr) + if werr == os.ErrInvalid { + writeJSON(w, http.StatusBadRequest, map[string]string{"error": "path escapes workspace"}) + return + } + writeJSON(w, http.StatusInternalServerError, map[string]string{"error": "failed to write content"}) + return + } + existing.ContentHash = hash + writtenHash = hash + contentWasSet = true + } + if err := h.store.UpsertDocument(r.Context(), existing); err != nil { slog.Warn("vault.update failed", "error", err) writeJSON(w, http.StatusInternalServerError, map[string]string{"error": err.Error()}) return } + + if contentWasSet { + evAgent := "" + if existing.AgentID != nil { + evAgent = *existing.AgentID + } + h.publishDocUpserted(tenantID.String(), evAgent, existing.ID, existing.Path, writtenHash, wsPath) + } + updated, _ := h.store.GetDocumentByID(r.Context(), tenantID.String(), docID) if updated != nil { writeJSON(w, http.StatusOK, updated) From 5d5be0a64fa2186aa99ac2387f52805669491c7b Mon Sep 17 00:00:00 2001 From: Aaron Vu <54255926+aaron-tsar@users.noreply.github.com> Date: Thu, 28 May 2026 00:54:19 +0700 Subject: [PATCH 02/13] fix(vault): harden symlink containment + mirror upload enrich-progress Addresses review on #1174: - writeDocumentContent now resolves symlinks before writing. A symlink living inside the workspace but pointing outside (e.g. workspace/link -> /etc) previously passed the lexical prefix check and os.WriteFile followed it out of bounds. We now EvalSymlinks the workspace root and the deepest existing ancestor of the target, re-check containment before any mkdir/write, and refuse a symlink planted at the final component via Lstat. - publishDocUpserted starts enrichment progress (enrichProgress.Start) before publishing EventVaultDocUpserted, matching handleUpload's ordering so the worker pool can't drain the event before the progress tracker registers it. - Add regression tests: symlink dir escape, final-component symlink, parent escape, empty content, disallowed extension, event-after-upsert ordering, metadata-only no-write, and upsert-failure orphan behaviour. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/http/vault_handler_documents.go | 63 +++- internal/http/vault_handler_documents_test.go | 341 ++++++++++++++++++ 2 files changed, 391 insertions(+), 13 deletions(-) create mode 100644 internal/http/vault_handler_documents_test.go diff --git a/internal/http/vault_handler_documents.go b/internal/http/vault_handler_documents.go index fbe08d4f..745a1251 100644 --- a/internal/http/vault_handler_documents.go +++ b/internal/http/vault_handler_documents.go @@ -23,22 +23,52 @@ import ( // pipeline (which reads files by path) can later compute summaries, embeddings // and links. func (h *VaultHandler) writeDocumentContent(workspace, relPath string, content []byte) (string, error) { - target := filepath.Join(workspace, filepath.Clean(relPath)) - // Symlink/escape protection: the cleaned target must stay inside workspace. - absTarget, err := filepath.Abs(target) + // Resolve the workspace root through any symlinks so all containment checks + // compare canonical paths. + realWS, err := filepath.EvalSymlinks(workspace) if err != nil { return "", err } - absWS, err := filepath.Abs(workspace) - if err != nil { - return "", err - } - if !strings.HasPrefix(absTarget, absWS+string(os.PathSeparator)) && absTarget != absWS { + target := filepath.Join(realWS, filepath.Clean(relPath)) + + // Lexical containment: blocks "../" and absolute-path escapes. + if target != realWS && !strings.HasPrefix(target, realWS+string(os.PathSeparator)) { return "", os.ErrInvalid } + + // Symlink containment: a lexical prefix check is not enough — a symlink that + // lives *inside* the workspace but points outside it (e.g. workspace/link -> + // /etc) would otherwise let os.WriteFile follow it and clobber a file beyond + // the workspace. Resolve the deepest already-existing ancestor of the target + // and confirm it still canonicalises to a path inside the workspace, before + // any directory is created or byte written. + for ancestor := filepath.Dir(target); ; { + resolved, rerr := filepath.EvalSymlinks(ancestor) + if rerr == nil { + if resolved != realWS && !strings.HasPrefix(resolved, realWS+string(os.PathSeparator)) { + return "", os.ErrInvalid + } + break + } + if !os.IsNotExist(rerr) { + return "", rerr + } + parent := filepath.Dir(ancestor) + if parent == ancestor { + break // walked to the filesystem root without an existing ancestor + } + ancestor = parent + } + if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { return "", err } + + // Refuse to follow a symlink planted at the final path component itself. + if fi, lerr := os.Lstat(target); lerr == nil && fi.Mode()&os.ModeSymlink != 0 { + return "", os.ErrInvalid + } + if err := os.WriteFile(target, content, 0o644); err != nil { return "", err } @@ -48,20 +78,27 @@ func (h *VaultHandler) writeDocumentContent(workspace, relPath string, content [ // publishDocUpserted enqueues the standard EventVaultDocUpserted so the // enrichment worker re-runs summary/embedding/link extraction for this doc. // No-op when the event bus is not wired (some test setups). -func (h *VaultHandler) publishDocUpserted(tenantID, agentID, docID, relPath, hash, workspace string) { +func (h *VaultHandler) publishDocUpserted(tenantID uuid.UUID, agentID, docID, relPath, hash, workspace string) { if h.eventBus == nil { return } + tenantIDStr := tenantID.String() + // Start enrichment progress before publishing so the worker pool can't drain + // the event and finish before the progress tracker registers it — same + // ordering handleUpload relies on to avoid that race. + if h.enrichProgress != nil { + h.enrichProgress.Start(1, tenantID) + } h.eventBus.Publish(eventbus.DomainEvent{ ID: uuid.Must(uuid.NewV7()).String(), Type: eventbus.EventVaultDocUpserted, SourceID: docID + ":" + hash, - TenantID: tenantID, + TenantID: tenantIDStr, AgentID: agentID, Timestamp: time.Now(), Payload: eventbus.VaultDocUpsertedPayload{ DocID: docID, - TenantID: tenantID, + TenantID: tenantIDStr, AgentID: agentID, Path: relPath, ContentHash: hash, @@ -284,7 +321,7 @@ func (h *VaultHandler) handleCreateDocument(w http.ResponseWriter, r *http.Reque if doc.AgentID != nil { evAgent = *doc.AgentID } - h.publishDocUpserted(tenantID.String(), evAgent, doc.ID, body.Path, writtenHash, wsPath) + h.publishDocUpserted(tenantID, evAgent, doc.ID, body.Path, writtenHash, wsPath) } // Re-fetch by ID (set via RETURNING) — unambiguous even when same path exists across teams. @@ -391,7 +428,7 @@ func (h *VaultHandler) handleUpdateDocument(w http.ResponseWriter, r *http.Reque if existing.AgentID != nil { evAgent = *existing.AgentID } - h.publishDocUpserted(tenantID.String(), evAgent, existing.ID, existing.Path, writtenHash, wsPath) + h.publishDocUpserted(tenantID, evAgent, existing.ID, existing.Path, writtenHash, wsPath) } updated, _ := h.store.GetDocumentByID(r.Context(), tenantID.String(), docID) diff --git a/internal/http/vault_handler_documents_test.go b/internal/http/vault_handler_documents_test.go new file mode 100644 index 00000000..2840c905 --- /dev/null +++ b/internal/http/vault_handler_documents_test.go @@ -0,0 +1,341 @@ +package http + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + "time" + + "github.com/google/uuid" + + "github.com/nextlevelbuilder/goclaw/internal/eventbus" + "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/internal/vault" +) + +// masterTenant resolves TenantWorkspace to h.workspace unchanged, keeping the +// content-write tests pointed at a single temp dir. +var masterTenant = uuid.MustParse("0193a5b0-7000-7000-8000-000000000001") + +func masterCtx() context.Context { + return store.WithTenantID(context.Background(), masterTenant) +} + +// ---- writeDocumentContent: path & symlink containment (the blocking finding) ---- + +func TestWriteDocumentContent_WritesInsideWorkspace(t *testing.T) { + h := &VaultHandler{} + ws := t.TempDir() + + hash, err := h.writeDocumentContent(ws, "notes/hello.md", []byte("hi")) + if err != nil { + t.Fatalf("writeDocumentContent: %v", err) + } + if want := vault.ContentHash([]byte("hi")); hash != want { + t.Errorf("hash = %q, want %q", hash, want) + } + got, err := os.ReadFile(filepath.Join(ws, "notes", "hello.md")) + if err != nil { + t.Fatalf("read written file: %v", err) + } + if string(got) != "hi" { + t.Errorf("content = %q, want %q", got, "hi") + } +} + +func TestWriteDocumentContent_EmptyContent(t *testing.T) { + h := &VaultHandler{} + ws := t.TempDir() + + hash, err := h.writeDocumentContent(ws, "empty.md", []byte("")) + if err != nil { + t.Fatalf("writeDocumentContent: %v", err) + } + if want := vault.ContentHash([]byte("")); hash != want { + t.Errorf("hash = %q, want %q", hash, want) + } + fi, err := os.Stat(filepath.Join(ws, "empty.md")) + if err != nil { + t.Fatalf("stat empty file: %v", err) + } + if fi.Size() != 0 { + t.Errorf("size = %d, want 0", fi.Size()) + } +} + +func TestWriteDocumentContent_RejectsParentEscape(t *testing.T) { + h := &VaultHandler{} + ws := t.TempDir() + + _, err := h.writeDocumentContent(ws, "../escape.md", []byte("x")) + if !errors.Is(err, os.ErrInvalid) { + t.Fatalf("err = %v, want os.ErrInvalid", err) + } + if _, statErr := os.Stat(filepath.Join(filepath.Dir(ws), "escape.md")); statErr == nil { + t.Error("file escaped the workspace via ../") + } +} + +// A symlink that lives inside the workspace but points outside it must not be +// followed — this is the exact escape the lexical prefix check missed. +func TestWriteDocumentContent_RejectsSymlinkDirEscape(t *testing.T) { + h := &VaultHandler{} + ws := t.TempDir() + outside := t.TempDir() + + if err := os.Symlink(outside, filepath.Join(ws, "link")); err != nil { + t.Fatalf("symlink: %v", err) + } + + _, err := h.writeDocumentContent(ws, "link/pwn.md", []byte("x")) + if !errors.Is(err, os.ErrInvalid) { + t.Fatalf("err = %v, want os.ErrInvalid", err) + } + if _, statErr := os.Stat(filepath.Join(outside, "pwn.md")); statErr == nil { + t.Error("file written outside workspace through symlinked directory") + } +} + +// A symlink planted at the final path component must be refused rather than +// followed, so an attacker can't redirect a write onto an existing outside file. +func TestWriteDocumentContent_RejectsFinalComponentSymlink(t *testing.T) { + h := &VaultHandler{} + ws := t.TempDir() + outside := t.TempDir() + + outsideFile := filepath.Join(outside, "target.md") + if err := os.WriteFile(outsideFile, []byte("original"), 0o644); err != nil { + t.Fatalf("seed outside file: %v", err) + } + if err := os.Symlink(outsideFile, filepath.Join(ws, "evil.md")); err != nil { + t.Fatalf("symlink: %v", err) + } + + _, err := h.writeDocumentContent(ws, "evil.md", []byte("pwned")) + if !errors.Is(err, os.ErrInvalid) { + t.Fatalf("err = %v, want os.ErrInvalid", err) + } + got, _ := os.ReadFile(outsideFile) + if string(got) != "original" { + t.Errorf("outside file clobbered through final-component symlink: %q", got) + } +} + +// ---- handler-level: extension whitelist, event ordering, empty PUT, orphan ---- + +// fakeVaultStore embeds the interface (unimplemented methods panic if hit) and +// overrides only the two methods the content path touches. +type fakeVaultStore struct { + store.VaultStore + docs map[string]*store.VaultDocument + upserted bool + upsertErr error + upsertCall int +} + +func newFakeVaultStore() *fakeVaultStore { + return &fakeVaultStore{docs: map[string]*store.VaultDocument{}} +} + +func (f *fakeVaultStore) UpsertDocument(_ context.Context, doc *store.VaultDocument) error { + f.upsertCall++ + if f.upsertErr != nil { + return f.upsertErr + } + if doc.ID == "" { + doc.ID = "doc-" + uuid.NewString() + } + f.upserted = true + f.docs[doc.ID] = doc + return nil +} + +func (f *fakeVaultStore) GetDocumentByID(_ context.Context, _ /*tenantID*/, id string) (*store.VaultDocument, error) { + return f.docs[id], nil +} + +// fakeEventBus captures published events and records whether the DB upsert had +// already happened at publish time, to assert ordering. +type fakeEventBus struct { + store *fakeVaultStore + published []eventbus.DomainEvent + upsertedAtFire bool +} + +func (b *fakeEventBus) Publish(e eventbus.DomainEvent) { + if b.store != nil { + b.upsertedAtFire = b.store.upserted + } + b.published = append(b.published, e) +} +func (b *fakeEventBus) Subscribe(eventbus.EventType, eventbus.DomainEventHandler) func() { + return func() {} +} +func (b *fakeEventBus) Start(context.Context) {} +func (b *fakeEventBus) Drain(time.Duration) error { return nil } + +func newJSONRequest(t *testing.T, method, target string, body any) *http.Request { + t.Helper() + buf, err := json.Marshal(body) + if err != nil { + t.Fatalf("marshal body: %v", err) + } + req := httptest.NewRequest(method, target, bytes.NewReader(buf)) + req.Header.Set("Content-Type", "application/json") + return req.WithContext(masterCtx()) +} + +func TestHandleCreateDocument_RejectsDisallowedExtension(t *testing.T) { + h := &VaultHandler{} // extension check fires before any store/workspace access + req := newJSONRequest(t, http.MethodPost, "/v1/vault/documents", map[string]any{ + "path": "payload.exe", + "title": "x", + "content": "data", + }) + rr := httptest.NewRecorder() + + h.handleCreateDocument(rr, req) + + assertBadRequest(t, rr, "unsupported file type") +} + +func TestHandleCreateDocument_WritesContentAndPublishesAfterUpsert(t *testing.T) { + st := newFakeVaultStore() + bus := &fakeEventBus{store: st} + ws := t.TempDir() + h := &VaultHandler{store: st, eventBus: bus, workspace: ws} + + req := newJSONRequest(t, http.MethodPost, "/v1/vault/documents", map[string]any{ + "path": "decisions/adr-001.md", + "title": "ADR 001", + "content": "# Decision\nuse postgres", + }) + rr := httptest.NewRecorder() + + h.handleCreateDocument(rr, req) + + if rr.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201 (body: %s)", rr.Code, rr.Body.String()) + } + // File materialised on disk. + got, err := os.ReadFile(filepath.Join(ws, "decisions", "adr-001.md")) + if err != nil { + t.Fatalf("read written doc: %v", err) + } + if string(got) != "# Decision\nuse postgres" { + t.Errorf("content = %q", got) + } + // Exactly one enrichment event, fired AFTER the DB upsert. + if len(bus.published) != 1 { + t.Fatalf("published %d events, want 1", len(bus.published)) + } + if !bus.upsertedAtFire { + t.Error("event published before DB upsert — enrichment worker could race ahead of the row") + } + if bus.published[0].Type != eventbus.EventVaultDocUpserted { + t.Errorf("event type = %v, want %v", bus.published[0].Type, eventbus.EventVaultDocUpserted) + } +} + +func TestHandleCreateDocument_NoContentSkipsWriteAndEvent(t *testing.T) { + st := newFakeVaultStore() + bus := &fakeEventBus{store: st} + ws := t.TempDir() + h := &VaultHandler{store: st, eventBus: bus, workspace: ws} + + req := newJSONRequest(t, http.MethodPost, "/v1/vault/documents", map[string]any{ + "path": "meta-only.md", + "title": "Meta only", + }) + rr := httptest.NewRecorder() + + h.handleCreateDocument(rr, req) + + if rr.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201 (body: %s)", rr.Code, rr.Body.String()) + } + if _, statErr := os.Stat(filepath.Join(ws, "meta-only.md")); statErr == nil { + t.Error("metadata-only create must not write a file") + } + if len(bus.published) != 0 { + t.Errorf("metadata-only create published %d events, want 0", len(bus.published)) + } +} + +// Documents the orphan-file behaviour: when the DB upsert fails after the bytes +// are already on disk, the handler returns 500 and the file is intentionally +// left in place (no rollback). If that ever changes, this test should change +// with it. +func TestHandleCreateDocument_UpsertFailureLeavesFile(t *testing.T) { + st := newFakeVaultStore() + st.upsertErr = errors.New("db down") + bus := &fakeEventBus{store: st} + ws := t.TempDir() + h := &VaultHandler{store: st, eventBus: bus, workspace: ws} + + req := newJSONRequest(t, http.MethodPost, "/v1/vault/documents", map[string]any{ + "path": "orphan.md", + "title": "Orphan", + "content": "written before upsert", + }) + rr := httptest.NewRecorder() + + h.handleCreateDocument(rr, req) + + if rr.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500 (body: %s)", rr.Code, rr.Body.String()) + } + if _, statErr := os.Stat(filepath.Join(ws, "orphan.md")); statErr != nil { + t.Errorf("file should remain after upsert failure (orphan accepted): %v", statErr) + } + if len(bus.published) != 0 { + t.Errorf("no event should fire when upsert fails, got %d", len(bus.published)) + } +} + +func TestHandleUpdateDocument_EmptyContentClearsFile(t *testing.T) { + st := newFakeVaultStore() + st.docs["existing-1"] = &store.VaultDocument{ + ID: "existing-1", + TenantID: masterTenant.String(), + Path: "notes/keep.md", + Title: "Keep", + DocType: "note", + Scope: "shared", + } + bus := &fakeEventBus{store: st} + ws := t.TempDir() + h := &VaultHandler{store: st, eventBus: bus, workspace: ws} + + empty := "" + req := newJSONRequest(t, http.MethodPut, "/v1/vault/documents/existing-1", map[string]any{ + "content": empty, + }) + req.SetPathValue("docID", "existing-1") + rr := httptest.NewRecorder() + + h.handleUpdateDocument(rr, req) + + if rr.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (body: %s)", rr.Code, rr.Body.String()) + } + fi, err := os.Stat(filepath.Join(ws, "notes", "keep.md")) + if err != nil { + t.Fatalf("stat cleared file: %v", err) + } + if fi.Size() != 0 { + t.Errorf("file size = %d, want 0 (empty content clears file)", fi.Size()) + } + if st.docs["existing-1"].ContentHash != vault.ContentHash([]byte("")) { + t.Errorf("content_hash = %q, want empty-content hash", st.docs["existing-1"].ContentHash) + } + if len(bus.published) != 1 { + t.Errorf("published %d events, want 1", len(bus.published)) + } +} From 1fad4e0b57e1f69b78fddcca5100c551f1528e76 Mon Sep 17 00:00:00 2001 From: Aaron Vu <54255926+aaron-tsar@users.noreply.github.com> Date: Thu, 28 May 2026 20:10:19 +0700 Subject: [PATCH 03/13] fix(vault): non-master tenant 500, O_NOFOLLOW, slog, *string content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses second review on #1174 (thieung): **B1 (blocking)** — first JSON content write into a fresh non-master tenant returned 500 because workspace/tenants/{slug}/ doesn't exist yet and EvalSymlinks fails with ErrNotExist. writeDocumentContent now MkdirAlls the workspace before resolving (mirrors handleUpload). Added a regression test that drives a non-master tenant context end-to-end and asserts the file lands under workspace/tenants/{slug}/. **I1** — replaced the Lstat→WriteFile sequence (TOCTOU window) with os.OpenFile(O_WRONLY|O_CREATE|O_TRUNC|O_NOFOLLOW). The Lstat short-fail stays as defense-in-depth + cleaner error logging. Portability handled via build-tagged constants in vault_handler_documents_nofollow_{unix, windows}.go — O_NOFOLLOW is no-op on Windows (desktop edition is single-user with no untrusted tenants). **I3** — every symlink/path-escape rejection now emits slog.Warn("security.vault_symlink_escape", ...) with the rejection site, resolved path and workspace, matching the storage.go pattern so oncall can alert on attempted escapes. **I4** — POST `content` field changed from `string` to `*string` to match PUT. Nil pointer = "no write" (metadata-only stub); empty string = "write a 0-byte file + fire event"; non-empty = write bytes. Updated handler godoc to spell out the symmetry. Switched `werr == os.ErrInvalid` to `errors.Is` per repo conventions. **Tests added** — non-master tenant first content write (B1), PUT with nil content (locks "no change" contract), handler-level ".." path rejection, POST content:"" writes empty file. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/http/vault_handler_documents.go | 70 ++++++--- .../vault_handler_documents_nofollow_unix.go | 20 +++ ...ault_handler_documents_nofollow_windows.go | 11 ++ internal/http/vault_handler_documents_test.go | 137 ++++++++++++++++++ 4 files changed, 219 insertions(+), 19 deletions(-) create mode 100644 internal/http/vault_handler_documents_nofollow_unix.go create mode 100644 internal/http/vault_handler_documents_nofollow_windows.go diff --git a/internal/http/vault_handler_documents.go b/internal/http/vault_handler_documents.go index 745a1251..30741714 100644 --- a/internal/http/vault_handler_documents.go +++ b/internal/http/vault_handler_documents.go @@ -1,6 +1,7 @@ package http import ( + "errors" "log/slog" "net/http" "os" @@ -23,6 +24,15 @@ import ( // pipeline (which reads files by path) can later compute summaries, embeddings // and links. func (h *VaultHandler) writeDocumentContent(workspace, relPath string, content []byte) (string, error) { + // Ensure the tenant workspace exists before resolving symlinks. Non-master + // tenants live under workspace/tenants/{slug}/ which handleUpload creates on + // demand (see vault_handler_upload.go) — without this, the very first JSON + // content write into a fresh tenant would fail EvalSymlinks with ErrNotExist + // and 500 the request. + if err := os.MkdirAll(workspace, 0o755); err != nil { + return "", err + } + // Resolve the workspace root through any symlinks so all containment checks // compare canonical paths. realWS, err := filepath.EvalSymlinks(workspace) @@ -33,12 +43,14 @@ func (h *VaultHandler) writeDocumentContent(workspace, relPath string, content [ // Lexical containment: blocks "../" and absolute-path escapes. if target != realWS && !strings.HasPrefix(target, realWS+string(os.PathSeparator)) { + slog.Warn("security.vault_symlink_escape", + "site", "lexical", "attempted", target, "workspace", realWS) return "", os.ErrInvalid } // Symlink containment: a lexical prefix check is not enough — a symlink that // lives *inside* the workspace but points outside it (e.g. workspace/link -> - // /etc) would otherwise let os.WriteFile follow it and clobber a file beyond + // /etc) would otherwise let the write follow it and clobber a file beyond // the workspace. Resolve the deepest already-existing ancestor of the target // and confirm it still canonicalises to a path inside the workspace, before // any directory is created or byte written. @@ -46,6 +58,8 @@ func (h *VaultHandler) writeDocumentContent(workspace, relPath string, content [ resolved, rerr := filepath.EvalSymlinks(ancestor) if rerr == nil { if resolved != realWS && !strings.HasPrefix(resolved, realWS+string(os.PathSeparator)) { + slog.Warn("security.vault_symlink_escape", + "site", "ancestor", "resolved", resolved, "workspace", realWS) return "", os.ErrInvalid } break @@ -64,12 +78,30 @@ func (h *VaultHandler) writeDocumentContent(workspace, relPath string, content [ return "", err } - // Refuse to follow a symlink planted at the final path component itself. + // Fast-fail when the final component is already a symlink — gives a clean + // rejection + security log without parsing OS-specific error codes. The + // O_NOFOLLOW open below closes the TOCTOU window this Lstat leaves open + // (an attacker could swap the file for a symlink between calls). if fi, lerr := os.Lstat(target); lerr == nil && fi.Mode()&os.ModeSymlink != 0 { + slog.Warn("security.vault_symlink_escape", + "site", "final_lstat", "target", target, "workspace", realWS) return "", os.ErrInvalid } - if err := os.WriteFile(target, content, 0o644); err != nil { + // O_NOFOLLOW makes the kernel refuse to follow a symlink at the final + // component atomically with the open, removing the Lstat→Open race entirely + // on Unix. On Windows oNoFollow == 0 and the Lstat above is best-effort. + f, err := os.OpenFile(target, os.O_WRONLY|os.O_CREATE|os.O_TRUNC|oNoFollow, 0o644) + if err != nil { + if isSymlinkLoopErr(err) { + slog.Warn("security.vault_symlink_escape", + "site", "open_nofollow", "target", target, "workspace", realWS) + return "", os.ErrInvalid + } + return "", err + } + defer f.Close() + if _, err := f.Write(content); err != nil { return "", err } return vault.ContentHash(content), nil @@ -199,15 +231,15 @@ func (h *VaultHandler) handleGetDocument(w http.ResponseWriter, r *http.Request) // handleCreateDocument creates a new vault document. // -// When `content` is supplied in the JSON body, the bytes are materialised on -// disk at / and the SHA-256 hash is stored on the -// document record. An EventVaultDocUpserted is then emitted so the enrichment -// worker computes summary/embeddings/links — same code path the multipart -// /v1/vault/upload endpoint and the filesystem rescan use. -// -// Omitting `content` preserves the historical behaviour of registering a -// metadata-only stub (typically followed by a rescan that materialises the -// file from disk). +// `content` semantics — symmetric with handleUpdateDocument: +// - field omitted (nil pointer): metadata-only stub, no file write, no event +// (typically followed by a rescan that materialises the file from disk). +// - field present and non-empty: bytes materialised at /, +// SHA-256 hash stored on the row, EventVaultDocUpserted emitted so the +// enrichment worker computes summary/embeddings/links — same code path the +// multipart /v1/vault/upload endpoint and the filesystem rescan use. +// - field present and empty (""): a 0-byte file is written and the event still +// fires. Same behaviour as PUT — pick the right shape on the client side. func (h *VaultHandler) handleCreateDocument(w http.ResponseWriter, r *http.Request) { locale := extractLocale(r) tenantID := store.TenantIDFromContext(r.Context()) @@ -216,7 +248,7 @@ func (h *VaultHandler) handleCreateDocument(w http.ResponseWriter, r *http.Reque var body struct { Path string `json:"path"` Title string `json:"title"` - Content string `json:"content"` // optional; when set, written to disk + Content *string `json:"content"` // nil=no write; ""=write empty file; "data"=write bytes (mirrors PUT) DocType string `json:"doc_type"` Scope string `json:"scope"` TeamID string `json:"team_id"` @@ -249,8 +281,8 @@ func (h *VaultHandler) handleCreateDocument(w http.ResponseWriter, r *http.Reque } // Validate extension upfront if caller wants to write content — matches the // whitelist enforced by /v1/vault/upload so the two endpoints accept the - // same file types. - if body.Content != "" { + // same file types. nil pointer = "no content field", which skips the check. + if body.Content != nil { ext := strings.ToLower(filepath.Ext(body.Path)) if !allowedUploadExts[ext] { writeJSON(w, http.StatusBadRequest, map[string]string{"error": "unsupported file type: " + ext}) @@ -288,16 +320,16 @@ func (h *VaultHandler) handleCreateDocument(w http.ResponseWriter, r *http.Reque writtenHash string contentWasSet bool ) - if body.Content != "" { + if body.Content != nil { wsPath = h.resolveTenantWorkspace(r.Context()) if wsPath == "" { writeJSON(w, http.StatusBadRequest, map[string]string{"error": "workspace not available"}) return } - hash, werr := h.writeDocumentContent(wsPath, body.Path, []byte(body.Content)) + hash, werr := h.writeDocumentContent(wsPath, body.Path, []byte(*body.Content)) if werr != nil { slog.Warn("vault.create: write content failed", "path", body.Path, "error", werr) - if werr == os.ErrInvalid { + if errors.Is(werr, os.ErrInvalid) { writeJSON(w, http.StatusBadRequest, map[string]string{"error": "path escapes workspace"}) return } @@ -405,7 +437,7 @@ func (h *VaultHandler) handleUpdateDocument(w http.ResponseWriter, r *http.Reque hash, werr := h.writeDocumentContent(wsPath, existing.Path, []byte(*body.Content)) if werr != nil { slog.Warn("vault.update: write content failed", "path", existing.Path, "error", werr) - if werr == os.ErrInvalid { + if errors.Is(werr, os.ErrInvalid) { writeJSON(w, http.StatusBadRequest, map[string]string{"error": "path escapes workspace"}) return } diff --git a/internal/http/vault_handler_documents_nofollow_unix.go b/internal/http/vault_handler_documents_nofollow_unix.go new file mode 100644 index 00000000..62eb2b81 --- /dev/null +++ b/internal/http/vault_handler_documents_nofollow_unix.go @@ -0,0 +1,20 @@ +//go:build !windows + +package http + +import ( + "errors" + "syscall" +) + +// oNoFollow is OR'd into the open flags so the kernel refuses to follow a +// symlink at the final path component — closes the Lstat→Open TOCTOU window +// where a tenant could swap a regular file for a symlink between the two +// syscalls. On Windows this is a no-op (see the _windows.go counterpart). +const oNoFollow = syscall.O_NOFOLLOW + +// isSymlinkLoopErr reports whether err is the kernel's "refused to follow +// symlink" signal from an O_NOFOLLOW open (ELOOP on Linux/macOS). +func isSymlinkLoopErr(err error) bool { + return errors.Is(err, syscall.ELOOP) +} diff --git a/internal/http/vault_handler_documents_nofollow_windows.go b/internal/http/vault_handler_documents_nofollow_windows.go new file mode 100644 index 00000000..ed76a65a --- /dev/null +++ b/internal/http/vault_handler_documents_nofollow_windows.go @@ -0,0 +1,11 @@ +//go:build windows + +package http + +// Windows has no O_NOFOLLOW concept and follows reparse points transparently; +// the Lstat short-circuit in writeDocumentContent is the only protection +// available there. Desktop edition is single-user with no untrusted tenants, +// so this degradation is acceptable. +const oNoFollow = 0 + +func isSymlinkLoopErr(err error) bool { return false } diff --git a/internal/http/vault_handler_documents_test.go b/internal/http/vault_handler_documents_test.go index 2840c905..a67329b7 100644 --- a/internal/http/vault_handler_documents_test.go +++ b/internal/http/vault_handler_documents_test.go @@ -339,3 +339,140 @@ func TestHandleUpdateDocument_EmptyContentClearsFile(t *testing.T) { t.Errorf("published %d events, want 1", len(bus.published)) } } + +// Locks the "no change" contract on PUT: omitting `content` (nil pointer in +// the body struct) must not touch the disk or fire an enrichment event, even +// when other metadata fields are present. +func TestHandleUpdateDocument_NilContentSkipsWriteAndEvent(t *testing.T) { + st := newFakeVaultStore() + st.docs["existing-2"] = &store.VaultDocument{ + ID: "existing-2", + TenantID: masterTenant.String(), + Path: "notes/keep.md", + Title: "Old", + DocType: "note", + Scope: "shared", + ContentHash: "preexisting-hash", + } + bus := &fakeEventBus{store: st} + ws := t.TempDir() + h := &VaultHandler{store: st, eventBus: bus, workspace: ws} + + // content field intentionally omitted → *string is nil → no write, no event. + req := newJSONRequest(t, http.MethodPut, "/v1/vault/documents/existing-2", map[string]any{ + "title": "Renamed but content untouched", + }) + req.SetPathValue("docID", "existing-2") + rr := httptest.NewRecorder() + + h.handleUpdateDocument(rr, req) + + if rr.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 (body: %s)", rr.Code, rr.Body.String()) + } + if _, statErr := os.Stat(filepath.Join(ws, "notes", "keep.md")); statErr == nil { + t.Error("nil content must not materialise a file") + } + if st.docs["existing-2"].ContentHash != "preexisting-hash" { + t.Errorf("content_hash = %q, want unchanged 'preexisting-hash'", st.docs["existing-2"].ContentHash) + } + if len(bus.published) != 0 { + t.Errorf("nil-content PUT must not publish events, got %d", len(bus.published)) + } +} + +// Handler-level guard at vault_handler_documents.go path validation must reject +// "..", short-circuiting before writeDocumentContent even runs. +func TestHandleCreateDocument_RejectsParentTraversalPath(t *testing.T) { + h := &VaultHandler{} // path check runs before any store/workspace access + req := newJSONRequest(t, http.MethodPost, "/v1/vault/documents", map[string]any{ + "path": "../../etc/passwd.md", + "title": "x", + "content": "evil", + }) + rr := httptest.NewRecorder() + + h.handleCreateDocument(rr, req) + + assertBadRequest(t, rr, "invalid path") +} + +// Mirrors PUT semantics on POST: `content: ""` writes a 0-byte file and +// fires the enrichment event (the I4 fix that aligned POST/PUT). +func TestHandleCreateDocument_EmptyContentWritesEmptyFile(t *testing.T) { + st := newFakeVaultStore() + bus := &fakeEventBus{store: st} + ws := t.TempDir() + h := &VaultHandler{store: st, eventBus: bus, workspace: ws} + + req := newJSONRequest(t, http.MethodPost, "/v1/vault/documents", map[string]any{ + "path": "placeholder.md", + "title": "Placeholder", + "content": "", + }) + rr := httptest.NewRecorder() + + h.handleCreateDocument(rr, req) + + if rr.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201 (body: %s)", rr.Code, rr.Body.String()) + } + fi, err := os.Stat(filepath.Join(ws, "placeholder.md")) + if err != nil { + t.Fatalf("stat empty file: %v", err) + } + if fi.Size() != 0 { + t.Errorf("size = %d, want 0", fi.Size()) + } + if len(bus.published) != 1 { + t.Errorf("published %d events, want 1", len(bus.published)) + } +} + +// B1 regression: non-master tenants resolve to workspace/tenants/{slug}/ which +// is created on demand. The very first JSON content write into a fresh tenant +// must NOT 500 because EvalSymlinks can't find that dir — writeDocumentContent +// is responsible for creating it (mirrors what handleUpload does). +func TestHandleCreateDocument_NonMasterTenantFirstContentWrite(t *testing.T) { + st := newFakeVaultStore() + bus := &fakeEventBus{store: st} + ws := t.TempDir() + h := &VaultHandler{store: st, eventBus: bus, workspace: ws} + + nonMaster := uuid.MustParse("01999999-1111-7000-8000-000000000abc") + ctx := store.WithTenantSlug(store.WithTenantID(context.Background(), nonMaster), "acme") + + req := httptest.NewRequest(http.MethodPost, "/v1/vault/documents", bytes.NewReader(mustJSON(t, map[string]any{ + "path": "notes/first.md", + "title": "First", + "content": "hello from acme", + }))).WithContext(ctx) + req.Header.Set("Content-Type", "application/json") + rr := httptest.NewRecorder() + + h.handleCreateDocument(rr, req) + + if rr.Code != http.StatusCreated { + t.Fatalf("status = %d, want 201 (body: %s)", rr.Code, rr.Body.String()) + } + tenantDir := filepath.Join(ws, "tenants", "acme") + got, err := os.ReadFile(filepath.Join(tenantDir, "notes", "first.md")) + if err != nil { + t.Fatalf("read non-master tenant doc: %v", err) + } + if string(got) != "hello from acme" { + t.Errorf("content = %q", got) + } + if len(bus.published) != 1 { + t.Errorf("published %d events, want 1", len(bus.published)) + } +} + +func mustJSON(t *testing.T, v any) []byte { + t.Helper() + b, err := json.Marshal(v) + if err != nil { + t.Fatalf("marshal: %v", err) + } + return b +} From 9edcc00dbc64746663ddf594eab5ace0ffc659d2 Mon Sep 17 00:00:00 2001 From: Aaron Vu <54255926+aaron-tsar@users.noreply.github.com> Date: Thu, 28 May 2026 20:54:54 +0700 Subject: [PATCH 04/13] fix(vault): surface close errors + clarify slog label + drop test dup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Trivial nits from thieung's review of ba59e7b0: - writeDocumentContent now closes the file explicitly and propagates any Close() error so a delayed FS failure (e.g. disk-full at flush) doesn't return success. - Renamed slog `site=lexical` to `site=path_traversal` — the lexical prefix check catches "../" / absolute-path escapes, not symlinks. Umbrella key `security.vault_symlink_escape` stays. - Non-master-tenant test now reuses newJSONRequest + WithContext, dropping the duplicate mustJSON helper. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/http/vault_handler_documents.go | 13 +++++++++---- internal/http/vault_handler_documents_test.go | 16 ++++------------ 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/internal/http/vault_handler_documents.go b/internal/http/vault_handler_documents.go index 30741714..e6ef22e7 100644 --- a/internal/http/vault_handler_documents.go +++ b/internal/http/vault_handler_documents.go @@ -44,7 +44,7 @@ func (h *VaultHandler) writeDocumentContent(workspace, relPath string, content [ // Lexical containment: blocks "../" and absolute-path escapes. if target != realWS && !strings.HasPrefix(target, realWS+string(os.PathSeparator)) { slog.Warn("security.vault_symlink_escape", - "site", "lexical", "attempted", target, "workspace", realWS) + "site", "path_traversal", "attempted", target, "workspace", realWS) return "", os.ErrInvalid } @@ -100,9 +100,14 @@ func (h *VaultHandler) writeDocumentContent(workspace, relPath string, content [ } return "", err } - defer f.Close() - if _, err := f.Write(content); err != nil { - return "", err + if _, werr := f.Write(content); werr != nil { + _ = f.Close() // best-effort cleanup; the write error is the real one + return "", werr + } + // Explicit Close so a delayed FS error (e.g. disk-full at flush) surfaces + // instead of being silently swallowed by a deferred close. + if cerr := f.Close(); cerr != nil { + return "", cerr } return vault.ContentHash(content), nil } diff --git a/internal/http/vault_handler_documents_test.go b/internal/http/vault_handler_documents_test.go index a67329b7..fa593f2b 100644 --- a/internal/http/vault_handler_documents_test.go +++ b/internal/http/vault_handler_documents_test.go @@ -442,12 +442,13 @@ func TestHandleCreateDocument_NonMasterTenantFirstContentWrite(t *testing.T) { nonMaster := uuid.MustParse("01999999-1111-7000-8000-000000000abc") ctx := store.WithTenantSlug(store.WithTenantID(context.Background(), nonMaster), "acme") - req := httptest.NewRequest(http.MethodPost, "/v1/vault/documents", bytes.NewReader(mustJSON(t, map[string]any{ + // newJSONRequest builds against masterCtx by default; WithContext swaps in + // the non-master tenant context for this single test. + req := newJSONRequest(t, http.MethodPost, "/v1/vault/documents", map[string]any{ "path": "notes/first.md", "title": "First", "content": "hello from acme", - }))).WithContext(ctx) - req.Header.Set("Content-Type", "application/json") + }).WithContext(ctx) rr := httptest.NewRecorder() h.handleCreateDocument(rr, req) @@ -467,12 +468,3 @@ func TestHandleCreateDocument_NonMasterTenantFirstContentWrite(t *testing.T) { t.Errorf("published %d events, want 1", len(bus.published)) } } - -func mustJSON(t *testing.T, v any) []byte { - t.Helper() - b, err := json.Marshal(v) - if err != nil { - t.Fatalf("marshal: %v", err) - } - return b -} From dfc7d71b5cc176d9ae02b4e0a176d5b792cb0755 Mon Sep 17 00:00:00 2001 From: viettranx Date: Wed, 3 Jun 2026 18:15:48 +0700 Subject: [PATCH 05/13] chore: ignore local agent config directories --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitignore b/.gitignore index 2a0ec78e..c7171363 100644 --- a/.gitignore +++ b/.gitignore @@ -82,6 +82,8 @@ goclaw.db-shm *.app.bak *.exe.bak # AI tool config directories (user-specific) +.agents/ +.codex/ .gemini/ .opencode/ From f8f910e540eeb1118669daba5da7710a44f2c29b Mon Sep 17 00:00:00 2001 From: viettranx Date: Wed, 3 Jun 2026 20:52:47 +0700 Subject: [PATCH 06/13] fix(mcp): keep grant checker cache invalidation active --- internal/mcp/grant_checker.go | 4 +- internal/mcp/grant_checker_test.go | 62 +++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/internal/mcp/grant_checker.go b/internal/mcp/grant_checker.go index a144f463..481ceb7d 100644 --- a/internal/mcp/grant_checker.go +++ b/internal/mcp/grant_checker.go @@ -46,6 +46,8 @@ type storeGrantChecker struct { cache sync.Map // map[cacheKey]*cacheEntry } +const grantCheckerCacheSubscriberID = bus.TopicCacheMCP + ":grant_checker" + // NewStoreGrantChecker creates a GrantChecker backed by the MCP store. // Subscribes to TopicCacheMCP for cache invalidation on grant changes. func NewStoreGrantChecker(mcpStore store.MCPServerStore, msgBus *bus.MessageBus) *storeGrantChecker { @@ -56,7 +58,7 @@ func NewStoreGrantChecker(mcpStore store.MCPServerStore, msgBus *bus.MessageBus) // Subscribe to cache invalidation events. // When MCP grants are revoked/modified, the HTTP handler broadcasts to this topic. if msgBus != nil { - msgBus.Subscribe(bus.TopicCacheMCP, func(event bus.Event) { + msgBus.Subscribe(grantCheckerCacheSubscriberID, func(event bus.Event) { if event.Name == protocol.EventCacheInvalidate { gc.cache.Clear() slog.Debug("mcp.grant_checker.cache_cleared", "trigger", "bus_event") diff --git a/internal/mcp/grant_checker_test.go b/internal/mcp/grant_checker_test.go index a731ae0d..6e6bb5af 100644 --- a/internal/mcp/grant_checker_test.go +++ b/internal/mcp/grant_checker_test.go @@ -6,7 +6,9 @@ import ( "testing" "github.com/google/uuid" + "github.com/nextlevelbuilder/goclaw/internal/bus" "github.com/nextlevelbuilder/goclaw/internal/store" + "github.com/nextlevelbuilder/goclaw/pkg/protocol" ) // mockMCPStore implements store.MCPServerStore for testing. @@ -28,11 +30,13 @@ func (m *mockMCPStore) GetServer(ctx context.Context, id uuid.UUID) (*store.MCPS func (m *mockMCPStore) GetServerByName(ctx context.Context, name string) (*store.MCPServerData, error) { return nil, nil } -func (m *mockMCPStore) ListServers(ctx context.Context) ([]store.MCPServerData, error) { return nil, nil } +func (m *mockMCPStore) ListServers(ctx context.Context) ([]store.MCPServerData, error) { + return nil, nil +} func (m *mockMCPStore) UpdateServer(ctx context.Context, id uuid.UUID, updates map[string]any) error { return nil } -func (m *mockMCPStore) DeleteServer(ctx context.Context, id uuid.UUID) error { return nil } +func (m *mockMCPStore) DeleteServer(ctx context.Context, id uuid.UUID) error { return nil } func (m *mockMCPStore) GrantToAgent(ctx context.Context, g *store.MCPAgentGrant) error { return nil } func (m *mockMCPStore) RevokeFromAgent(ctx context.Context, serverID, agentID uuid.UUID) error { return nil @@ -132,6 +136,60 @@ func TestStoreGrantChecker_InvalidateClearsCache(t *testing.T) { } } +func TestStoreGrantChecker_BusInvalidationSurvivesOtherMCPSubscribers(t *testing.T) { + initialServerID := uuid.New() + newServerID := uuid.New() + agentID := uuid.New() + userID := "system" + + mockStore := &mockMCPStore{ + accessible: []store.MCPAccessInfo{ + {Server: store.MCPServerData{BaseModel: store.BaseModel{ID: initialServerID}, Name: "initial-server"}}, + }, + } + + msgBus := bus.New() + gc := NewStoreGrantChecker(mockStore, msgBus) + ctx := context.Background() + + if allowed, reason := gc.IsAllowed(ctx, agentID, userID, initialServerID, "tool"); !allowed { + t.Fatalf("expected initial server allowed, reason=%q", reason) + } + if mockStore.callCount != 1 { + t.Fatalf("expected 1 DB call after warm cache, got %d", mockStore.callCount) + } + + // The gateway also subscribes to MCP cache events. Subscriber IDs must be + // unique; otherwise this later subscription replaces the grant checker and + // its stale cache survives grant changes. + otherSubscriberCalled := false + msgBus.Subscribe(bus.TopicCacheMCP, func(event bus.Event) { + if event.Name == protocol.EventCacheInvalidate { + otherSubscriberCalled = true + } + }) + + mockStore.accessible = []store.MCPAccessInfo{ + {Server: store.MCPServerData{BaseModel: store.BaseModel{ID: initialServerID}, Name: "initial-server"}}, + {Server: store.MCPServerData{BaseModel: store.BaseModel{ID: newServerID}, Name: "new-server"}}, + } + + msgBus.Broadcast(bus.Event{ + Name: protocol.EventCacheInvalidate, + Payload: bus.CacheInvalidatePayload{Kind: bus.CacheKindMCP}, + }) + if !otherSubscriberCalled { + t.Fatal("expected other MCP subscriber to receive invalidation") + } + + if allowed, reason := gc.IsAllowed(ctx, agentID, userID, newServerID, "tool"); !allowed { + t.Fatalf("expected grant checker cache to refresh after bus invalidation, reason=%q", reason) + } + if mockStore.callCount != 2 { + t.Fatalf("expected cache miss after bus invalidation, got callCount=%d", mockStore.callCount) + } +} + func TestStoreGrantChecker_DenyReasonSurfaced(t *testing.T) { serverID := uuid.New() agentID := uuid.New() From e05eec55e8390e4570147abd5d32eedabed7267b Mon Sep 17 00:00:00 2001 From: viettranx Date: Wed, 3 Jun 2026 21:23:06 +0700 Subject: [PATCH 07/13] test(git): isolate ssh tmpfile mismatch check --- tests/integration/git_adapter_ssh_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/integration/git_adapter_ssh_test.go b/tests/integration/git_adapter_ssh_test.go index 302d2cf2..2c7f5495 100644 --- a/tests/integration/git_adapter_ssh_test.go +++ b/tests/integration/git_adapter_ssh_test.go @@ -160,8 +160,7 @@ func TestGitAdapter_SSH_CleanupOnExecFailure(t *testing.T) { // TestGitAdapter_SSH_HostMismatch_NoTmpfile: rejected host mismatch must // NOT leave an orphaned tmpfile in os.TempDir(). func TestGitAdapter_SSH_HostMismatch_NoTmpfile(t *testing.T) { - t.Parallel() - + t.Setenv("TMPDIR", t.TempDir()) cred := sshCredFor(t, "github.com") adapter := tools.AdapterFor("git") From 3d8bba8b886d749908bb39678bc1c58eaeb8f2b9 Mon Sep 17 00:00:00 2001 From: viettranx Date: Wed, 3 Jun 2026 21:29:03 +0700 Subject: [PATCH 08/13] ci(release): publish docker digests to both registries --- .github/workflows/release.yaml | 60 ++++++++++++++++++++++++++-------- 1 file changed, 46 insertions(+), 14 deletions(-) diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index fc111a93..f384c287 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -195,13 +195,19 @@ jobs: username: ${{ github.actor }} password: ${{ secrets.GITHUB_TOKEN }} + - name: Log in to Docker Hub + uses: docker/login-action@v3 + with: + username: ${{ secrets.DOCKERHUB_USERNAME }} + password: ${{ secrets.DOCKERHUB_TOKEN }} + - name: Build and push by digest id: build uses: docker/build-push-action@v6 with: context: . platforms: ${{ matrix.platform }} - outputs: type=image,name=${{ env.GHCR_IMAGE }},push-by-digest=true,name-canonical=true,push=true + outputs: type=image,"name=${{ env.GHCR_IMAGE }},${{ env.DOCKERHUB_IMAGE }}",push-by-digest=true,name-canonical=true,push=true build-args: | ENABLE_OTEL=false ENABLE_EMBEDUI=${{ matrix.enable_embedui }} @@ -280,18 +286,28 @@ jobs: - name: Create manifest list and push working-directory: /tmp/digests run: | - # Compose -t flags from metadata-action tags (newline-separated) - TAG_ARGS=() + # Compose -t flags from metadata-action tags (newline-separated). + GHCR_IMAGE="${{ env.GHCR_IMAGE }}" + DOCKERHUB_IMAGE="${{ env.DOCKERHUB_IMAGE }}" + GHCR_TAG_ARGS=() + DOCKERHUB_TAG_ARGS=() while IFS= read -r tag; do - [ -n "$tag" ] && TAG_ARGS+=("-t" "$tag") + if [[ "$tag" == "$GHCR_IMAGE:"* ]]; then + GHCR_TAG_ARGS+=("-t" "$tag") + elif [[ "$tag" == "$DOCKERHUB_IMAGE:"* ]]; then + DOCKERHUB_TAG_ARGS+=("-t" "$tag") + fi done <<< "${{ steps.meta.outputs.tags }}" - # Compose source digest refs (pushed to GHCR by docker-build) - DIGEST_ARGS=() + # Compose source digest refs pushed to each registry by docker-build. + GHCR_DIGEST_ARGS=() + DOCKERHUB_DIGEST_ARGS=() for f in *; do - DIGEST_ARGS+=("${{ env.GHCR_IMAGE }}@sha256:$f") + GHCR_DIGEST_ARGS+=("$GHCR_IMAGE@sha256:$f") + DOCKERHUB_DIGEST_ARGS+=("$DOCKERHUB_IMAGE@sha256:$f") done set -x - docker buildx imagetools create "${TAG_ARGS[@]}" "${DIGEST_ARGS[@]}" + docker buildx imagetools create "${GHCR_TAG_ARGS[@]}" "${GHCR_DIGEST_ARGS[@]}" + docker buildx imagetools create "${DOCKERHUB_TAG_ARGS[@]}" "${DOCKERHUB_DIGEST_ARGS[@]}" - name: Inspect manifest run: | @@ -327,13 +343,19 @@ jobs: username: ${{ github.actor }} password: ${{ secrets.GITHUB_TOKEN }} + - name: Log in to Docker Hub + uses: docker/login-action@v3 + with: + username: ${{ secrets.DOCKERHUB_USERNAME }} + password: ${{ secrets.DOCKERHUB_TOKEN }} + - name: Build and push by digest id: build uses: docker/build-push-action@v6 with: context: ui/web platforms: ${{ matrix.platform }} - outputs: type=image,name=${{ env.GHCR_IMAGE }}-web,push-by-digest=true,name-canonical=true,push=true + outputs: type=image,"name=${{ env.GHCR_IMAGE }}-web,${{ env.DOCKERHUB_IMAGE }}-web",push-by-digest=true,name-canonical=true,push=true cache-from: type=gha,scope=web-${{ matrix.arch }} cache-to: type=gha,mode=max,scope=web-${{ matrix.arch }} provenance: false @@ -393,16 +415,26 @@ jobs: - name: Create manifest list and push working-directory: /tmp/digests run: | - TAG_ARGS=() + GHCR_IMAGE="${{ env.GHCR_IMAGE }}-web" + DOCKERHUB_IMAGE="${{ env.DOCKERHUB_IMAGE }}-web" + GHCR_TAG_ARGS=() + DOCKERHUB_TAG_ARGS=() while IFS= read -r tag; do - [ -n "$tag" ] && TAG_ARGS+=("-t" "$tag") + if [[ "$tag" == "$GHCR_IMAGE:"* ]]; then + GHCR_TAG_ARGS+=("-t" "$tag") + elif [[ "$tag" == "$DOCKERHUB_IMAGE:"* ]]; then + DOCKERHUB_TAG_ARGS+=("-t" "$tag") + fi done <<< "${{ steps.meta.outputs.tags }}" - DIGEST_ARGS=() + GHCR_DIGEST_ARGS=() + DOCKERHUB_DIGEST_ARGS=() for f in *; do - DIGEST_ARGS+=("${{ env.GHCR_IMAGE }}-web@sha256:$f") + GHCR_DIGEST_ARGS+=("$GHCR_IMAGE@sha256:$f") + DOCKERHUB_DIGEST_ARGS+=("$DOCKERHUB_IMAGE@sha256:$f") done set -x - docker buildx imagetools create "${TAG_ARGS[@]}" "${DIGEST_ARGS[@]}" + docker buildx imagetools create "${GHCR_TAG_ARGS[@]}" "${GHCR_DIGEST_ARGS[@]}" + docker buildx imagetools create "${DOCKERHUB_TAG_ARGS[@]}" "${DOCKERHUB_DIGEST_ARGS[@]}" - name: Inspect manifest run: | From d85bf17171fd0faefbbd54df44bef573991aa7f8 Mon Sep 17 00:00:00 2001 From: viettranx Date: Wed, 3 Jun 2026 21:42:24 +0700 Subject: [PATCH 09/13] ci(release): build docker images directly for stable tags --- .github/workflows/release.yaml | 238 +++++---------------------------- 1 file changed, 37 insertions(+), 201 deletions(-) diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index f384c287..f4b3636a 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -161,100 +161,27 @@ jobs: strategy: fail-fast: false matrix: - variant: [base, latest, full] - platform: [linux/amd64, linux/arm64] include: - variant: base + suffix: "-base" enable_embedui: "false" enable_python: "false" enable_full_skills: "false" - variant: latest + suffix: "" enable_embedui: "true" enable_python: "true" enable_full_skills: "false" - variant: full + suffix: "-full" enable_embedui: "true" enable_python: "true" enable_full_skills: "true" - - platform: linux/amd64 - runner: ubuntu-latest - arch: amd64 - - platform: linux/arm64 - runner: ubuntu-24.04-arm - arch: arm64 - runs-on: ${{ matrix.runner }} + runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - uses: docker/setup-buildx-action@v3 - - - name: Log in to GHCR - uses: docker/login-action@v3 - with: - registry: ghcr.io - username: ${{ github.actor }} - password: ${{ secrets.GITHUB_TOKEN }} - - - name: Log in to Docker Hub - uses: docker/login-action@v3 - with: - username: ${{ secrets.DOCKERHUB_USERNAME }} - password: ${{ secrets.DOCKERHUB_TOKEN }} - - - name: Build and push by digest - id: build - uses: docker/build-push-action@v6 - with: - context: . - platforms: ${{ matrix.platform }} - outputs: type=image,"name=${{ env.GHCR_IMAGE }},${{ env.DOCKERHUB_IMAGE }}",push-by-digest=true,name-canonical=true,push=true - build-args: | - ENABLE_OTEL=false - ENABLE_EMBEDUI=${{ matrix.enable_embedui }} - ENABLE_PYTHON=${{ matrix.enable_python }} - ENABLE_FULL_SKILLS=${{ matrix.enable_full_skills }} - VERSION=v${{ needs.release.outputs.version }} - cache-from: type=gha,scope=${{ matrix.variant }}-${{ matrix.arch }} - cache-to: type=gha,mode=max,scope=${{ matrix.variant }}-${{ matrix.arch }} - provenance: false - - - name: Export digest - run: | - mkdir -p /tmp/digests - digest="${{ steps.build.outputs.digest }}" - touch "/tmp/digests/${digest#sha256:}" - - - uses: actions/upload-artifact@v4 - with: - name: digests-${{ matrix.variant }}-${{ matrix.arch }} - path: /tmp/digests/* - if-no-files-found: error - retention-days: 1 - - # Combine per-platform digests into multi-arch manifests and push final tags - # to both GHCR and Docker Hub. One job per variant runs in parallel. - docker-merge: - needs: [release, docker-build] - if: needs.release.outputs.released == 'true' - runs-on: ubuntu-latest - timeout-minutes: 15 - strategy: - fail-fast: false - matrix: - include: - - variant: base - suffix: "-base" - - variant: latest - suffix: "" - - variant: full - suffix: "-full" - steps: - - name: Download digests - uses: actions/download-artifact@v4 - with: - path: /tmp/digests - pattern: digests-${{ matrix.variant }}-* - merge-multiple: true + - uses: docker/setup-qemu-action@v3 - uses: docker/setup-buildx-action@v3 @@ -283,108 +210,34 @@ jobs: type=raw,value=latest,enable=${{ matrix.suffix == '' }},suffix= type=raw,value=${{ matrix.variant }},enable=${{ matrix.suffix != '' }} - - name: Create manifest list and push - working-directory: /tmp/digests - run: | - # Compose -t flags from metadata-action tags (newline-separated). - GHCR_IMAGE="${{ env.GHCR_IMAGE }}" - DOCKERHUB_IMAGE="${{ env.DOCKERHUB_IMAGE }}" - GHCR_TAG_ARGS=() - DOCKERHUB_TAG_ARGS=() - while IFS= read -r tag; do - if [[ "$tag" == "$GHCR_IMAGE:"* ]]; then - GHCR_TAG_ARGS+=("-t" "$tag") - elif [[ "$tag" == "$DOCKERHUB_IMAGE:"* ]]; then - DOCKERHUB_TAG_ARGS+=("-t" "$tag") - fi - done <<< "${{ steps.meta.outputs.tags }}" - # Compose source digest refs pushed to each registry by docker-build. - GHCR_DIGEST_ARGS=() - DOCKERHUB_DIGEST_ARGS=() - for f in *; do - GHCR_DIGEST_ARGS+=("$GHCR_IMAGE@sha256:$f") - DOCKERHUB_DIGEST_ARGS+=("$DOCKERHUB_IMAGE@sha256:$f") - done - set -x - docker buildx imagetools create "${GHCR_TAG_ARGS[@]}" "${GHCR_DIGEST_ARGS[@]}" - docker buildx imagetools create "${DOCKERHUB_TAG_ARGS[@]}" "${DOCKERHUB_DIGEST_ARGS[@]}" + - name: Build and push + uses: docker/build-push-action@v6 + with: + context: . + platforms: linux/amd64,linux/arm64 + push: true + tags: ${{ steps.meta.outputs.tags }} + labels: ${{ steps.meta.outputs.labels }} + build-args: | + ENABLE_OTEL=false + ENABLE_EMBEDUI=${{ matrix.enable_embedui }} + ENABLE_PYTHON=${{ matrix.enable_python }} + ENABLE_FULL_SKILLS=${{ matrix.enable_full_skills }} + VERSION=v${{ needs.release.outputs.version }} + cache-from: type=gha,scope=release-${{ matrix.variant }} + cache-to: type=gha,mode=max,scope=release-${{ matrix.variant }} + provenance: false - - name: Inspect manifest - run: | - docker buildx imagetools inspect \ - "${{ env.GHCR_IMAGE }}:v${{ needs.release.outputs.version }}${{ matrix.suffix }}" - - # Build web UI Docker image per platform on native runners. + # Build web UI Docker image. docker-web-build: needs: release if: needs.release.outputs.released == 'true' timeout-minutes: 30 - strategy: - fail-fast: false - matrix: - platform: [linux/amd64, linux/arm64] - include: - - platform: linux/amd64 - runner: ubuntu-latest - arch: amd64 - - platform: linux/arm64 - runner: ubuntu-24.04-arm - arch: arm64 - runs-on: ${{ matrix.runner }} + runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - uses: docker/setup-buildx-action@v3 - - - name: Log in to GHCR - uses: docker/login-action@v3 - with: - registry: ghcr.io - username: ${{ github.actor }} - password: ${{ secrets.GITHUB_TOKEN }} - - - name: Log in to Docker Hub - uses: docker/login-action@v3 - with: - username: ${{ secrets.DOCKERHUB_USERNAME }} - password: ${{ secrets.DOCKERHUB_TOKEN }} - - - name: Build and push by digest - id: build - uses: docker/build-push-action@v6 - with: - context: ui/web - platforms: ${{ matrix.platform }} - outputs: type=image,"name=${{ env.GHCR_IMAGE }}-web,${{ env.DOCKERHUB_IMAGE }}-web",push-by-digest=true,name-canonical=true,push=true - cache-from: type=gha,scope=web-${{ matrix.arch }} - cache-to: type=gha,mode=max,scope=web-${{ matrix.arch }} - provenance: false - - - name: Export digest - run: | - mkdir -p /tmp/digests - digest="${{ steps.build.outputs.digest }}" - touch "/tmp/digests/${digest#sha256:}" - - - uses: actions/upload-artifact@v4 - with: - name: digests-web-${{ matrix.arch }} - path: /tmp/digests/* - if-no-files-found: error - retention-days: 1 - - docker-web-merge: - needs: [release, docker-web-build] - if: needs.release.outputs.released == 'true' - runs-on: ubuntu-latest - timeout-minutes: 15 - steps: - - name: Download digests - uses: actions/download-artifact@v4 - with: - path: /tmp/digests - pattern: digests-web-* - merge-multiple: true + - uses: docker/setup-qemu-action@v3 - uses: docker/setup-buildx-action@v3 @@ -412,38 +265,21 @@ jobs: type=raw,value=v${{ needs.release.outputs.version }} type=raw,value=latest - - name: Create manifest list and push - working-directory: /tmp/digests - run: | - GHCR_IMAGE="${{ env.GHCR_IMAGE }}-web" - DOCKERHUB_IMAGE="${{ env.DOCKERHUB_IMAGE }}-web" - GHCR_TAG_ARGS=() - DOCKERHUB_TAG_ARGS=() - while IFS= read -r tag; do - if [[ "$tag" == "$GHCR_IMAGE:"* ]]; then - GHCR_TAG_ARGS+=("-t" "$tag") - elif [[ "$tag" == "$DOCKERHUB_IMAGE:"* ]]; then - DOCKERHUB_TAG_ARGS+=("-t" "$tag") - fi - done <<< "${{ steps.meta.outputs.tags }}" - GHCR_DIGEST_ARGS=() - DOCKERHUB_DIGEST_ARGS=() - for f in *; do - GHCR_DIGEST_ARGS+=("$GHCR_IMAGE@sha256:$f") - DOCKERHUB_DIGEST_ARGS+=("$DOCKERHUB_IMAGE@sha256:$f") - done - set -x - docker buildx imagetools create "${GHCR_TAG_ARGS[@]}" "${GHCR_DIGEST_ARGS[@]}" - docker buildx imagetools create "${DOCKERHUB_TAG_ARGS[@]}" "${DOCKERHUB_DIGEST_ARGS[@]}" - - - name: Inspect manifest - run: | - docker buildx imagetools inspect \ - "${{ env.GHCR_IMAGE }}-web:v${{ needs.release.outputs.version }}" + - name: Build and push + uses: docker/build-push-action@v6 + with: + context: ui/web + platforms: linux/amd64,linux/arm64 + push: true + tags: ${{ steps.meta.outputs.tags }} + labels: ${{ steps.meta.outputs.labels }} + cache-from: type=gha,scope=release-web + cache-to: type=gha,mode=max,scope=release-web + provenance: false # Notify Discord on new release (runs even if docker jobs fail) notify-discord: - needs: [release, build-binaries, docker-merge, docker-web-merge] + needs: [release, build-binaries, docker-build, docker-web-build] if: always() && needs.release.outputs.released == 'true' && !cancelled() runs-on: ubuntu-latest timeout-minutes: 5 From 43837afca3285cf8aae6a19dc907146355d24b20 Mon Sep 17 00:00:00 2001 From: Thieu Nguyen <79964592+thieung@users.noreply.github.com> Date: Fri, 5 Jun 2026 00:48:38 +0700 Subject: [PATCH 10/13] fix(security): consolidate & enhance batched security fixes (#1155, #967, #972, #974, #989, #973) (#1185) * fix(sandbox): avoid shell in FsBridge writes Replace sh -c with interpolated path by shell-free 'tee -- ' argv form, piping content via stdin. Prevents command injection through filenames containing shell metacharacters inside the sandbox container. Co-authored-by: evgyur * fix(security): fail-closed on pairing DB errors across channels On IsPaired lookup error, deny instead of granting access. Covers the shared CheckDMPolicy/CheckGroupPolicy helpers (Slack/Discord/Feishu/WhatsApp/Zalo) and the four inline Telegram pairing checks. Co-authored-by: Srini * fix(security): harden provider URL validation against SSRF Enforce scheme check for all provider types; restrict local types (ollama, claude_cli, acp) to an explicit localhost allowlist instead of skipping checks; resolve remote hostnames and reject any IP in a private/reserved range via the shared security.IsBlocked CIDR list (covers loopback, link-local, metadata, multicast, and unspecified 0.0.0.0/::). Closes the wildcard-DNS bypass and the local-type escape hatch. Operator opt-in via GOCLAW_ALLOW_PRIVATE_PROVIDER_URLS. Exports security.IsBlocked as the single source of truth for blocked ranges. Co-authored-by: Linh Vo Van * feat(pipeline): add fail-closed tool call authorization gate Gate tool execution against the server-side AllowedTools allowlist built from the RBAC/tenant-aware filtered tool set. Resolve the tool-call prefix before the allowlist lookup so prefixed agents are not wrongly blocked, re-check deny on lazy MCP activation, and expand IsDenied to cover aliased tool names. Co-authored-by: Huy Doan * fix(security): expand file-serve deny-list defense-in-depth Add absolute-path deny prefixes (/home, /Users, /srv, /var/lib, /var/www, /opt) and an explicit fail-closed log when no file-serving boundary is configured. Co-authored-by: Linh Vo Van * fix(providers): allow claude cli executable paths Refs: #1185 --------- Co-authored-by: evgyur Co-authored-by: Srini Co-authored-by: Linh Vo Van Co-authored-by: Huy Doan --- docs/02-providers.md | 2 +- internal/agent/loop_lazy_mcp_test.go | 33 +- internal/agent/loop_pipeline_adapter.go | 1 + internal/agent/loop_pipeline_callbacks.go | 40 +++ internal/channels/channel.go | 8 +- internal/channels/policy_test.go | 12 +- internal/channels/telegram/handlers.go | 12 +- internal/http/files.go | 17 +- internal/http/files_path_security_test.go | 54 +++ internal/http/oauth_test.go | 6 + internal/http/providers.go | 131 +++++-- internal/http/providers_test.go | 73 ++++ internal/http/providers_url_validate_test.go | 326 ++++++++++++++++++ internal/pipeline/deps.go | 3 + internal/pipeline/substates.go | 3 + internal/pipeline/think_stage.go | 7 + internal/pipeline/tool_stage.go | 45 ++- .../pipeline/tool_stage_authorize_test.go | 216 ++++++++++++ internal/providers/defaults.go | 5 + internal/providers/embedding_openai.go | 7 +- internal/providers/vertex.go | 6 +- internal/sandbox/docker_test.go | 28 -- internal/sandbox/fsbridge.go | 15 +- internal/sandbox/fsbridge_test.go | 189 ++++++++++ internal/security/ssrf.go | 10 + internal/tools/policy.go | 25 +- 26 files changed, 1182 insertions(+), 92 deletions(-) create mode 100644 internal/http/providers_url_validate_test.go create mode 100644 internal/pipeline/tool_stage_authorize_test.go create mode 100644 internal/sandbox/fsbridge_test.go diff --git a/docs/02-providers.md b/docs/02-providers.md index 642464e7..7475fad5 100644 --- a/docs/02-providers.md +++ b/docs/02-providers.md @@ -554,7 +554,7 @@ ClaudeCLIProvider can be configured in `config.json`: } ``` -Or via database `llm_providers` table with `provider_type = "claude_cli"`. +Or via database `llm_providers` table with `provider_type = "claude_cli"`. For database providers, `api_base` is the CLI executable selector (`"claude"` or an absolute binary path), not an HTTP base URL, so provider URL SSRF opt-ins do not apply to Claude CLI. ### Session Management diff --git a/internal/agent/loop_lazy_mcp_test.go b/internal/agent/loop_lazy_mcp_test.go index c5deea7c..51f0ba7e 100644 --- a/internal/agent/loop_lazy_mcp_test.go +++ b/internal/agent/loop_lazy_mcp_test.go @@ -5,12 +5,14 @@ import ( "testing" "github.com/nextlevelbuilder/goclaw/internal/config" + "github.com/nextlevelbuilder/goclaw/internal/pipeline" + "github.com/nextlevelbuilder/goclaw/internal/providers" "github.com/nextlevelbuilder/goclaw/internal/tools" ) // mockExecTool is a simple tool that records whether it was executed. type mockExecTool struct { - name string + name string executed bool } @@ -24,13 +26,16 @@ func (m *mockExecTool) Execute(_ context.Context, _ map[string]any) *tools.Resul return tools.NewResult("ok from " + m.name) } -// simulateLazyActivationCheck mimics the allowedTools check from loop.go for one tool call: +// simulateLazyActivationCheck mimics the runtime authorize gate from +// makeAuthorizeToolCall (loop_pipeline_callbacks.go) for one tool call. +// The real gate is a PipelineDeps.AuthorizeToolCall callback invoked by ToolStage; +// this helper exercises the same logic in isolation so tests stay unit-level. // -// if allowedTools != nil && !allowedTools[tc.Name] { -// if l.tools.TryActivateDeferred(tc.Name) { allowedTools[tc.Name] = true } +// if allowed != nil && !allowed[name] { +// if reg.TryActivateDeferred(name) { allowed[name] = true } // else { result = ErrorResult(...) } // } -// if result == nil { result = l.tools.ExecuteWithContext(...) } +// if result == nil { result = reg.ExecuteWithContext(...) } // // Returns (result, blocked). func simulateLazyActivationCheck(reg *tools.Registry, allowedTools map[string]bool, toolName string) (*tools.Result, bool) { @@ -163,6 +168,24 @@ func TestLoop_LazyMCP_NilAllowedTools_AllowsAll(t *testing.T) { } } +func TestLoopAuthorizeToolCall_PrefixedNameCanonicalLookup(t *testing.T) { + loop := NewLoop(LoopConfig{ + AgentToolPolicy: &config.ToolPolicySpec{ToolCallPrefix: "proxy_"}, + }) + gate := loop.makeAuthorizeToolCall() + state := &pipeline.RunState{} + state.Tool.AllowedTools = map[string]bool{"exec": true} + + ok, reason := gate(context.Background(), state, providers.ToolCall{ + ID: "tc-proxy-exec", + Name: "proxy_exec", + }) + + if !ok { + t.Fatalf("prefixed tool call should resolve to canonical allowlist entry; reason: %q", reason) + } +} + func TestLoop_LazyMCP_SecondCall_UsesUpdatedAllowedTools(t *testing.T) { // After first lazy activation, subsequent calls in the same iteration use the // updated allowedTools map and don't invoke the activator again. diff --git a/internal/agent/loop_pipeline_adapter.go b/internal/agent/loop_pipeline_adapter.go index 13f6325b..3ab34798 100644 --- a/internal/agent/loop_pipeline_adapter.go +++ b/internal/agent/loop_pipeline_adapter.go @@ -141,6 +141,7 @@ func (l *Loop) buildPipelineDeps(req *RunRequest, bridgeRS *runState) pipeline.P ExecuteToolCall: cb.executeToolCall, ExecuteToolRaw: cb.executeToolRaw, ProcessToolResult: cb.processToolResult, + AuthorizeToolCall: cb.authorizeToolCall, SequentialToolCall: func(tc providers.ToolCall) bool { return l.resolveToolCallName(tc.Name) == "wait" }, diff --git a/internal/agent/loop_pipeline_callbacks.go b/internal/agent/loop_pipeline_callbacks.go index 9d19e0b6..740312eb 100644 --- a/internal/agent/loop_pipeline_callbacks.go +++ b/internal/agent/loop_pipeline_callbacks.go @@ -57,6 +57,7 @@ func (l *Loop) pipelineCallbacks(req *RunRequest, bridgeRS *runState) pipelineCa executeToolCall: l.makeExecuteToolCall(req, bridgeRS), executeToolRaw: l.makeExecuteToolRaw(req), processToolResult: l.makeProcessToolResult(req, bridgeRS), + authorizeToolCall: l.makeAuthorizeToolCall(), checkReadOnly: l.makeCheckReadOnly(req, bridgeRS), sanitizeContent: SanitizeAssistantContent, flushMessages: l.makeFlushMessages(req), @@ -85,6 +86,7 @@ type pipelineCallbackSet struct { executeToolCall func(ctx context.Context, state *pipeline.RunState, tc providers.ToolCall) ([]providers.Message, error) executeToolRaw func(ctx context.Context, tc providers.ToolCall) (providers.Message, any, error) processToolResult func(ctx context.Context, state *pipeline.RunState, tc providers.ToolCall, rawMsg providers.Message, rawData any) []providers.Message + authorizeToolCall func(ctx context.Context, state *pipeline.RunState, tc providers.ToolCall) (bool, string) checkReadOnly func(state *pipeline.RunState) (*providers.Message, bool) sanitizeContent func(string) string flushMessages func(ctx context.Context, sessionKey string, msgs []providers.Message) error @@ -249,6 +251,44 @@ func (l *Loop) makeBuildFilteredTools(req *RunRequest) func(state *pipeline.RunS } } +// makeAuthorizeToolCall enforces a runtime fail-closed allowlist check before +// every tool execution. AllowedTools is keyed by canonical registry names (built +// by ThinkStage from FilterTools output). The model may emit prefixed names when +// the agent has toolCallPrefix configured (e.g. "proxy_exec" → canonical "exec"), +// so we resolve the name to its canonical form before the lookup to avoid a +// guaranteed miss on every prefixed call. +func (l *Loop) makeAuthorizeToolCall() func(ctx context.Context, state *pipeline.RunState, tc providers.ToolCall) (bool, string) { + return func(_ context.Context, state *pipeline.RunState, tc providers.ToolCall) (bool, string) { + allowed := state.Tool.AllowedTools + if allowed == nil { + // nil allowlist means no per-iteration restriction (e.g. BuildFilteredTools not wired). + return true, "" + } + + // Resolve to canonical name before allowlist lookup. AllowedTools is keyed + // by canonical names; the model may emit prefixed names when toolCallPrefix + // is set (e.g. "proxy_exec" vs "exec"). Without this the lookup always misses. + name := l.resolveToolCallName(tc.Name) + + if allowed[name] { + return true, "" + } + + // Preserve lazy activation for deferred tools (typically per-user MCP). + if l.tools != nil && l.tools.TryActivateDeferred(name) { + // Re-check deny policy to prevent a lazy-activated tool from bypassing + // an explicit deny rule. + if l.toolPolicy != nil && l.toolPolicy.IsDenied(name, l.agentToolPolicy) { + return false, "tool not allowed by policy: " + name + } + allowed[name] = true + return true, "" + } + + return false, "tool not allowed by policy: " + name + } +} + func (l *Loop) makeCallLLM(req *RunRequest, emitRun func(AgentEvent)) func(ctx context.Context, state *pipeline.RunState, chatReq providers.ChatRequest) (*providers.ChatResponse, error) { return func(ctx context.Context, state *pipeline.RunState, chatReq providers.ChatRequest) (*providers.ChatResponse, error) { provider := state.Provider diff --git a/internal/channels/channel.go b/internal/channels/channel.go index 2826d880..545d651a 100644 --- a/internal/channels/channel.go +++ b/internal/channels/channel.go @@ -340,9 +340,9 @@ func (c *BaseChannel) CheckDMPolicy(ctx context.Context, senderID, dmPolicy stri if c.pairingService != nil { paired, err := c.pairingService.IsPaired(ctx, senderID, c.name) if err != nil { - slog.Warn("security.pairing_check_failed, assuming paired (fail-open)", + slog.Warn("security.pairing_check_failed, denying access (fail-closed)", "sender_id", senderID, "channel", c.name, "error", err) - return PolicyAllow + return PolicyDeny } if paired { return PolicyAllow @@ -379,9 +379,9 @@ func (c *BaseChannel) CheckGroupPolicy(ctx context.Context, senderID, chatID, gr if c.pairingService != nil { paired, err := c.pairingService.IsPaired(ctx, groupSenderID, c.name) if err != nil { - slog.Warn("security.pairing_check_failed, assuming paired (fail-open)", + slog.Warn("security.pairing_check_failed, denying access (fail-closed)", "group_sender", groupSenderID, "channel", c.name, "error", err) - return PolicyAllow + return PolicyDeny } if paired { c.MarkGroupApproved(chatID) diff --git a/internal/channels/policy_test.go b/internal/channels/policy_test.go index 0c0d165e..398163c0 100644 --- a/internal/channels/policy_test.go +++ b/internal/channels/policy_test.go @@ -174,12 +174,12 @@ func TestCheckDMPolicy_PolicyPairing(t *testing.T) { wantResult: PolicyAllow, }, { - name: "Pairing service error allows message (fail-open)", + name: "Pairing service error denies message (fail-closed)", senderID: "user999", allowList: []string{}, paired: false, failPairingCheck: true, - wantResult: PolicyAllow, + wantResult: PolicyDeny, }, } @@ -319,14 +319,14 @@ func TestCheckGroupPolicy_PolicyPairing(t *testing.T) { wantResult: PolicyNeedsPairing, }, { - name: "Pairing service error allows (fail-open)", + name: "Pairing service error denies (fail-closed)", senderID: "user999", chatID: "chat_2", allowList: []string{}, groupApproved: false, paired: false, failPairingCheck: true, - wantResult: PolicyAllow, + wantResult: PolicyDeny, }, } @@ -553,13 +553,13 @@ func TestCheckDMPolicy_AllPolicies_TableDriven(t *testing.T) { wantResult: PolicyNeedsPairing, }, { - name: "pairing policy fail-open on service error", + name: "pairing policy fail-closed on service error", policy: "pairing", senderID: "user3", allowList: []string{}, paired: false, failPairingCheck: true, - wantResult: PolicyAllow, + wantResult: PolicyDeny, }, } diff --git a/internal/channels/telegram/handlers.go b/internal/channels/telegram/handlers.go index bd92c93e..194d2a89 100644 --- a/internal/channels/telegram/handlers.go +++ b/internal/channels/telegram/handlers.go @@ -159,9 +159,8 @@ func (c *Channel) handleMessage(ctx context.Context, update telego.Update) { p1, err1 := ps.IsPaired(ctx, userID, c.Name()) p2, err2 := ps.IsPaired(ctx, senderID, c.Name()) if err1 != nil || err2 != nil { - slog.Warn("security.pairing_check_failed, assuming paired (fail-open)", + slog.Warn("security.pairing_check_failed, denying access (fail-closed)", "user_id", userID, "channel", c.Name(), "err1", err1, "err2", err2) - paired = true } else { paired = p1 || p2 } @@ -265,9 +264,8 @@ func (c *Channel) handleMessage(ctx context.Context, update telego.Update) { groupSenderID := fmt.Sprintf("group:%d", chatID) paired, pairErr := c.PairingService().IsPaired(ctx, groupSenderID, c.Name()) if pairErr != nil { - slog.Warn("security.pairing_check_failed, assuming paired (fail-open)", + slog.Warn("security.pairing_check_failed, denying access (fail-closed)", "group_sender", groupSenderID, "channel", c.Name(), "error", pairErr) - paired = true } if paired { c.MarkGroupApproved(chatIDStr) @@ -320,9 +318,8 @@ func (c *Channel) handleMessage(ctx context.Context, update telego.Update) { groupSenderID := fmt.Sprintf("group:%d", chatID) paired, pairErr := c.PairingService().IsPaired(ctx, groupSenderID, c.Name()) if pairErr != nil { - slog.Warn("security.pairing_check_failed, assuming paired (fail-open)", + slog.Warn("security.pairing_check_failed, denying access (fail-closed)", "group_sender", groupSenderID, "channel", c.Name(), "error", pairErr) - paired = true } if paired { c.MarkGroupApproved(chatIDStr) @@ -376,9 +373,8 @@ func (c *Channel) handleMessage(ctx context.Context, update telego.Update) { groupSenderID := fmt.Sprintf("group:%d", chatID) paired, err := c.PairingService().IsPaired(ctx, groupSenderID, c.Name()) if err != nil { - slog.Warn("security.pairing_check_failed, assuming paired (fail-open)", + slog.Warn("security.pairing_check_failed, denying access (fail-closed)", "group_sender", groupSenderID, "channel", c.Name(), "error", err) - paired = true } if paired { c.MarkGroupApproved(chatIDStr) diff --git a/internal/http/files.go b/internal/http/files.go index 2ed265d0..434416b3 100644 --- a/internal/http/files.go +++ b/internal/http/files.go @@ -95,11 +95,16 @@ func (h *FilesHandler) auth(next http.HandlerFunc) http.HandlerFunc { // deniedFilePrefixes blocks access to sensitive system directories. // Defense-in-depth: the auth token is the primary barrier, but restricting -// known-sensitive paths limits damage if a token leaks. +// known-sensitive absolute paths limits damage if a token leaks or a root is +// misconfigured. All entries must be absolute paths because hasDeniedFilePrefix +// uses pathWithinDir semantics — relative-rooted entries (e.g. "/.ssh") would +// only block the literal filesystem subtree at that root, not user home dirs. var deniedFilePrefixes = []string{ "/etc/", "/proc/", "/sys/", "/dev/", "/root/", "/boot/", "/run/", - "/var/run/", "/var/log/", + "/var/run/", "/var/log/", "/var/lib/", "/var/www/", + "/home/", "/Users/", + "/opt/", "/srv/", } func (h *FilesHandler) handleServe(w http.ResponseWriter, r *http.Request) { @@ -127,6 +132,14 @@ func (h *FilesHandler) handleServe(w http.ResponseWriter, r *http.Request) { } signed := r.URL.Query().Get("ft") != "" + // Fail-closed observability: if no boundary roots are configured at all, + // lexicallyAllowsFilePath already returns false, but log explicitly so + // operators can detect misconfigured deployments. + if h.workspace == "" && h.dataDir == "" { + slog.Warn("security.files_no_boundary", "path", absPath) + http.NotFound(w, r) + return + } 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) diff --git a/internal/http/files_path_security_test.go b/internal/http/files_path_security_test.go index 55bc7311..5fc3c650 100644 --- a/internal/http/files_path_security_test.go +++ b/internal/http/files_path_security_test.go @@ -88,6 +88,60 @@ func TestFilesHandleServe_ProcDir_Blocked(t *testing.T) { } } +// ---- handleServe: expanded deny-prefix list ---- + +// TestFilesHandleServe_ExpandedDenyPrefixes verifies that paths under /home/, +// /Users/, /var/lib/, /var/www/, /opt/, and /srv/ are all blocked. These are +// defense-in-depth blocks for misconfigured roots — the token/RBAC checks are +// the primary barriers. +func TestFilesHandleServe_ExpandedDenyPrefixes_Blocked(t *testing.T) { + h, _ := makeTestFilesHandler(t) + mux := http.NewServeMux() + mux.HandleFunc("GET /v1/files/{path...}", h.handleServe) + + cases := []struct { + name string + url string + }{ + {"home user ssh key", "/v1/files/home/user/.ssh/id_rsa"}, + {"home user aws creds", "/v1/files/home/user/.aws/credentials"}, + {"Users admin dir (macOS)", "/v1/files/Users/admin/secrets.txt"}, + {"var lib docker secret", "/v1/files/var/lib/docker/overlay2/secret"}, + {"var www config", "/v1/files/var/www/config.php"}, + {"opt secrets key", "/v1/files/opt/secrets/key.pem"}, + {"srv www app config", "/v1/files/srv/www/app/config.yaml"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, tc.url, nil) + w := httptest.NewRecorder() + mux.ServeHTTP(w, req) + if w.Code == http.StatusOK { + t.Errorf("path %s should be denied (deny-prefix), got 200", tc.url) + } + }) + } +} + +// ---- handleServe: fail-closed observability on empty workspace/dataDir ---- + +// TestFilesHandleServe_NoBoundary_Denies verifies that when both workspace and +// dataDir are empty the handler returns 404 (fail-closed) for any path. +func TestFilesHandleServe_NoBoundary_Denies(t *testing.T) { + h := NewFilesHandler("", "") + mux := http.NewServeMux() + mux.HandleFunc("GET /v1/files/{path...}", h.handleServe) + + req := httptest.NewRequest(http.MethodGet, "/v1/files/tmp/test.txt", nil) + w := httptest.NewRecorder() + mux.ServeHTTP(w, req) + + if w.Code == http.StatusOK { + t.Errorf("empty workspace+dataDir should deny all requests, got 200") + } +} + // ---- handleServe: workspace boundary enforcement ---- func TestFilesHandleServe_FileInsideWorkspace_WithToken_Serves(t *testing.T) { diff --git a/internal/http/oauth_test.go b/internal/http/oauth_test.go index 2dc5b881..22cb3068 100644 --- a/internal/http/oauth_test.go +++ b/internal/http/oauth_test.go @@ -65,6 +65,12 @@ func (m *mockProviderStore) UpdateProvider(_ context.Context, id uuid.UUID, upda if v, ok := updates["api_key"]; ok { p.APIKey = v.(string) } + if v, ok := updates["api_base"]; ok { + p.APIBase = v.(string) + } + if v, ok := updates["provider_type"]; ok { + p.ProviderType = v.(string) + } if v, ok := updates["settings"]; ok { p.Settings = v.(json.RawMessage) } diff --git a/internal/http/providers.go b/internal/http/providers.go index 9127982f..6bb28bab 100644 --- a/internal/http/providers.go +++ b/internal/http/providers.go @@ -8,6 +8,7 @@ import ( "net" "net/http" "net/url" + "os" "os/exec" "path/filepath" "strings" @@ -21,6 +22,7 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/oauth" "github.com/nextlevelbuilder/goclaw/internal/permissions" "github.com/nextlevelbuilder/goclaw/internal/providers" + "github.com/nextlevelbuilder/goclaw/internal/security" "github.com/nextlevelbuilder/goclaw/internal/store" usagecaps "github.com/nextlevelbuilder/goclaw/internal/usage/caps" "github.com/nextlevelbuilder/goclaw/pkg/protocol" @@ -322,55 +324,138 @@ func normalizeOllamaAPIBase(p *store.LLMProviderData) { } } -// localProviderTypes are provider types that legitimately run on localhost -// (e.g. Ollama, Claude CLI). SSRF checks are skipped for these. -var localProviderTypes = map[string]bool{ - store.ProviderOllama: true, - store.ProviderClaudeCLI: true, - store.ProviderACP: true, +// localURLProviderTypes are provider types that legitimately run on localhost. +// They are restricted to an explicit localhost allowlist +// rather than skipping SSRF validation entirely. +var localURLProviderTypes = map[string]bool{ + store.ProviderOllama: true, + store.ProviderACP: true, } +// allowedLocalHosts are the only hosts permitted for local provider types. +// Explicit allowlist (not blocklist) to prevent new internal addresses from +// slipping through (e.g. 169.254.169.254 via ollama base URL). +var allowedLocalHosts = []string{"localhost", "127.0.0.1", "::1", "host.docker.internal"} + +// dnsResolverFn resolves hostnames to IPs. Replaceable in tests. +var dnsResolverFn = net.LookupHost + +// allowPrivateProviderURLsFn reports whether the operator has opted in to +// permitting private / loopback / link-local / internal-hostname provider base +// URLs via GOCLAW_ALLOW_PRIVATE_PROVIDER_URLS. Evaluated once at first call so +// tests can override the variable before that happens. +var allowPrivateProviderURLsFn = sync.OnceValue(func() bool { + v := strings.ToLower(strings.TrimSpace(os.Getenv("GOCLAW_ALLOW_PRIVATE_PROVIDER_URLS"))) + return v == "1" || v == "true" || v == "yes" +}) + // validateProviderURL rejects provider base URLs pointing to internal/private networks. // Defense-in-depth: prevents SSRF when providers are later used for API calls. +// +// Logic: +// 1. Empty URL → allowed (provider may not need a custom base). +// 2. Claude CLI → api_base is an executable path/command, not a URL. +// 3. Scheme check (http/https only) → enforced for URL-based types, including +// local URL types. Blocks file://, gopher://, dict://, etc. +// 4. Local URL types (ollama, acp) → host must be in allowedLocalHosts +// (explicit allowlist prevents reaching 169.254.169.254 or internal services +// via the local-type bypass). +// 5. Remote types → if GOCLAW_ALLOW_PRIVATE_PROVIDER_URLS is set, allow and log. +// Otherwise: resolve DNS hostname; reject if ANY resolved IP satisfies +// security.IsBlocked (covers loopback, link-local, private, multicast, +// unspecified — including 0.0.0.0 and :: that earlier hand-rolled checks missed). +// +// DNS resolution on step 5 closes the nip.io / sslip.io / attacker-domain bypass +// where a hostname passes a literal-string blocklist but resolves to a private IP. func validateProviderURL(rawURL string, providerType string) error { - if rawURL == "" || localProviderTypes[providerType] { + if rawURL == "" { return nil } + if providerType == store.ProviderClaudeCLI { + return validateClaudeCLIExecutablePath(rawURL) + } u, err := url.Parse(rawURL) if err != nil { return fmt.Errorf("invalid URL: %w", err) } - // Only allow http/https schemes — block file://, gopher://, dict://, etc. + // Scheme check is unconditional for URL-based provider types, including local URL types. switch u.Scheme { case "http", "https": default: return fmt.Errorf("provider URL must use http or https scheme, got %q", u.Scheme) } + host := u.Hostname() - // Block obvious internal targets - blocked := []string{"localhost", "127.0.0.1", "::1", "0.0.0.0", "169.254.169.254", "metadata.google.internal"} - for _, b := range blocked { - if strings.EqualFold(host, b) { - return fmt.Errorf("provider URL cannot point to %s", b) + + // Local provider types: only allow an explicit localhost allowlist. + // This prevents using the local-type escape hatch to reach internal services + // or cloud metadata endpoints. + if localURLProviderTypes[providerType] { + for _, a := range allowedLocalHosts { + if strings.EqualFold(host, a) { + return nil + } } + slog.Warn("security.provider_url.local_type_denied", "host", host, "provider_type", providerType) + return fmt.Errorf("provider type %q only allows localhost URLs (localhost, 127.0.0.1, ::1, host.docker.internal), got host %q", providerType, host) } - // Block private IP ranges (normalize IPv6-mapped IPv4 to catch ::ffff:127.0.0.1) - ip := net.ParseIP(host) - if ip != nil { - if v4 := ip.To4(); v4 != nil { - ip = v4 - } - if ip.IsLoopback() || ip.IsPrivate() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() { - return fmt.Errorf("provider URL cannot point to private network: %s", host) - } + + // Operator opt-in to allow private-network provider URLs (e.g. LAN-hosted vLLM). + // Scheme check above still applies even with this gate open. + if allowPrivateProviderURLsFn() { + slog.Warn("security.provider_url.private_allowed", "host", host, "provider_type", providerType) + return nil } - // Block common internal hostnames + + // Check literal IP first (avoids unnecessary DNS lookup). + if ip := net.ParseIP(host); ip != nil { + if security.IsBlocked(ip) { + slog.Warn("security.provider_url.blocked", "host", host, "provider_type", providerType) + return fmt.Errorf("provider URL cannot point to %s", host) + } + return nil + } + + // Block .internal / .local suffix before DNS (fail-fast for well-known patterns). if strings.HasSuffix(host, ".internal") || strings.HasSuffix(host, ".local") { + slog.Warn("security.provider_url.blocked", "host", host, "provider_type", providerType) return fmt.Errorf("provider URL cannot point to internal hostname: %s", host) } + + // Resolve DNS and check every returned address. + // Prevents bypass via wildcard services (nip.io, sslip.io) or attacker-controlled + // domains that map to private IPs (DNS-rebinding at config time). + addrs, err := dnsResolverFn(host) + if err != nil { + slog.Warn("security.provider_url.dns_resolve_failed", "host", host, "provider_type", providerType, "error", err) + return fmt.Errorf("provider URL hostname %q could not be resolved: %w", host, err) + } + for _, addr := range addrs { + ip := net.ParseIP(addr) + if ip == nil { + continue + } + if security.IsBlocked(ip) { + slog.Warn("security.provider_url.blocked_resolved", "host", host, "resolved_ip", ip.String(), "provider_type", providerType) + return fmt.Errorf("provider URL %q resolves to private/reserved address %s", host, ip) + } + } return nil } +func validateClaudeCLIExecutablePath(path string) error { + if strings.Contains(path, "\x00") { + return fmt.Errorf("Claude CLI executable path cannot contain NUL byte") + } + if _, err := url.ParseRequestURI(path); err == nil && strings.Contains(path, "://") { + return fmt.Errorf("Claude CLI api_base must be an executable path or %q, got URL %q", "claude", path) + } + if path == "claude" || filepath.IsAbs(path) { + return nil + } + return fmt.Errorf("Claude CLI api_base must be %q or an absolute executable path, got %q", "claude", path) +} + // --- Provider CRUD --- func (h *ProvidersHandler) handleListProviders(w http.ResponseWriter, r *http.Request) { diff --git a/internal/http/providers_test.go b/internal/http/providers_test.go index e2b37f55..eeec3f77 100644 --- a/internal/http/providers_test.go +++ b/internal/http/providers_test.go @@ -298,6 +298,79 @@ func TestProvidersHandlerCreateAllows1536EmbeddingDimensions(t *testing.T) { } } +func TestProvidersHandlerCreateAllowsClaudeCLIExecutablePath(t *testing.T) { + token := setupProvidersAdminToken(t) + providerStore := newMockProviderStore() + handler := NewProvidersHandler(providerStore, newMockSecretsStore(), nil, "") + mux := http.NewServeMux() + handler.RegisterRoutes(mux) + + body := map[string]any{ + "name": "claude-local", + "provider_type": store.ProviderClaudeCLI, + "api_base": writeFakeClaudeBinary(t), + "enabled": true, + } + rawBody, err := json.Marshal(body) + if err != nil { + t.Fatalf("Marshal() error = %v", err) + } + + req := httptest.NewRequest(http.MethodPost, "/v1/providers", bytes.NewReader(rawBody)) + req.Header.Set("Authorization", "Bearer "+token) + w := httptest.NewRecorder() + mux.ServeHTTP(w, req) + + if w.Code != http.StatusCreated { + t.Fatalf("status code = %d, want %d, body=%s", w.Code, http.StatusCreated, w.Body.String()) + } + if got := providerStore.providers["claude-local"].APIBase; got == "" || !filepath.IsAbs(got) { + t.Fatalf("stored Claude CLI api_base = %q, want absolute executable path", got) + } +} + +func TestProvidersHandlerUpdateAllowsClaudeCLIExecutablePath(t *testing.T) { + token := setupProvidersAdminToken(t) + providerStore := newMockProviderStore() + provider := &store.LLMProviderData{ + BaseModel: store.BaseModel{ID: uuid.New()}, + Name: "claude-local", + ProviderType: store.ProviderClaudeCLI, + APIBase: "claude", + Enabled: true, + } + if err := providerStore.CreateProvider(context.Background(), provider); err != nil { + t.Fatalf("CreateProvider() error = %v", err) + } + + handler := NewProvidersHandler(providerStore, newMockSecretsStore(), nil, "") + mux := http.NewServeMux() + handler.RegisterRoutes(mux) + + nextPath := writeFakeClaudeBinary(t) + body := map[string]any{"api_base": nextPath} + rawBody, err := json.Marshal(body) + if err != nil { + t.Fatalf("Marshal() error = %v", err) + } + + req := httptest.NewRequest(http.MethodPut, "/v1/providers/"+provider.ID.String(), bytes.NewReader(rawBody)) + req.Header.Set("Authorization", "Bearer "+token) + w := httptest.NewRecorder() + mux.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status code = %d, want %d, body=%s", w.Code, http.StatusOK, w.Body.String()) + } + current, err := providerStore.GetProvider(context.Background(), provider.ID) + if err != nil { + t.Fatalf("GetProvider() error = %v", err) + } + if current.APIBase != nextPath { + t.Fatalf("api_base = %q, want %q", current.APIBase, nextPath) + } +} + func TestProvidersHandlerUpdateRejectsIncompatibleEmbeddingDimensions(t *testing.T) { token := setupProvidersAdminToken(t) providerStore := newMockProviderStore() diff --git a/internal/http/providers_url_validate_test.go b/internal/http/providers_url_validate_test.go new file mode 100644 index 00000000..715db697 --- /dev/null +++ b/internal/http/providers_url_validate_test.go @@ -0,0 +1,326 @@ +package http + +import ( + "net" + "path/filepath" + "strings" + "testing" +) + +// stubResolver returns a net.LookupHost-compatible function that maps known +// hostnames to controlled IPs. Unknown hostnames return a DNS error. +func stubResolver(m map[string][]string) func(host string) ([]string, error) { + return func(host string) ([]string, error) { + if addrs, ok := m[host]; ok { + return addrs, nil + } + return nil, &net.DNSError{Err: "no such host", Name: host, IsNotFound: true} + } +} + +// saveAndRestoreGlobals saves mutable package-level vars and restores them after +// the test. Call at the start of any test that touches dnsResolverFn or +// allowPrivateProviderURLsFn. +func saveAndRestoreGlobals(t *testing.T) { + t.Helper() + origResolver := dnsResolverFn + origAllow := allowPrivateProviderURLsFn + t.Cleanup(func() { + dnsResolverFn = origResolver + allowPrivateProviderURLsFn = origAllow + }) +} + +// --- Core merged test: all major cases in one table --- + +func TestValidateProviderURL(t *testing.T) { + saveAndRestoreGlobals(t) + allowPrivateProviderURLsFn = func() bool { return false } + absClaudePath := filepath.Join(t.TempDir(), "claude") + + dnsResolverFn = stubResolver(map[string][]string{ + "api.openai.com": {"104.18.6.192"}, + "legit-provider.com": {"203.0.113.50"}, + "10.10.27.30.nip.io": {"10.10.27.30"}, + "192.168.1.1.sslip.io": {"192.168.1.1"}, + "172.16.0.5.nip.io": {"172.16.0.5"}, + "169.254.169.254.nip.io": {"169.254.169.254"}, + "127.0.0.1.nip.io": {"127.0.0.1"}, + "rebind.attacker.com": {"10.0.0.1"}, + "dual-stack.example.com": {"203.0.113.50", "10.0.0.1"}, + }) + + tests := []struct { + name string + rawURL string + providerType string + wantErr bool + }{ + // Empty URL always allowed + {"empty URL", "", "openai_compat", false}, + {"empty URL ollama", "", "ollama", false}, + + // Public remote URLs OK + {"public HTTPS", "https://api.openai.com/v1", "openai_compat", false}, + {"public HTTP", "http://legit-provider.com/v1", "openai_compat", false}, + + // --- Scheme check: unconditional for ALL types including local --- + {"file scheme remote", "file:///etc/passwd", "openai_compat", true}, + {"gopher scheme remote", "gopher://internal:25", "openai_compat", true}, + {"file scheme ollama", "file:///etc/passwd", "ollama", true}, // H-1: scheme enforced even for local types + {"gopher scheme acp", "gopher://localhost:25", "acp", true}, // H-1: scheme enforced even for local types + {"file scheme claude_cli", "file:///bin/bash", "claude_cli", true}, // H-1: scheme enforced for URL-like Claude CLI values + + // --- Local type: allowlist-only --- + {"ollama localhost", "http://localhost:11434/v1", "ollama", false}, + {"ollama 127.0.0.1", "http://127.0.0.1:11434/v1", "ollama", false}, + {"ollama ::1", "http://[::1]:11434/v1", "ollama", false}, + {"ollama host.docker.internal", "http://host.docker.internal:11434/v1", "ollama", false}, + {"acp 127.0.0.1", "http://127.0.0.1:9090", "acp", false}, + {"claude_cli command name", "claude", "claude_cli", false}, + {"claude_cli absolute path", absClaudePath, "claude_cli", false}, + + // Local type with non-localhost hosts → blocked + {"ollama 169.254.169.254", "http://169.254.169.254/latest/meta-data/", "ollama", true}, + {"ollama private IP", "http://10.0.0.5:11434/v1", "ollama", true}, + {"ollama evil.com", "http://evil.attacker.tld:9999/v1", "ollama", true}, + {"ollama postgres sidecar", "http://postgres:5432/v1", "ollama", true}, + {"ollama link-local", "http://169.254.1.1:8080/v1", "ollama", true}, + {"ollama .internal", "http://redis.internal:6379/v1", "ollama", true}, + {"ollama gcp metadata", "http://metadata.google.internal/computeMetadata/v1/", "ollama", true}, + {"acp private", "http://10.0.0.1:8080/v1", "acp", true}, + + // --- Remote type literal blocked IPs --- + {"remote localhost", "http://localhost:8080", "openai_compat", true}, + {"remote 127.0.0.1", "http://127.0.0.1:8080", "openai_compat", true}, + {"remote ::1", "http://[::1]:8080", "openai_compat", true}, + {"remote 10.x", "http://10.0.0.1:8080/v1", "openai_compat", true}, + {"remote 192.168.x", "http://192.168.1.100:8080/v1", "openai_compat", true}, + {"remote 172.16.x", "http://172.16.0.5:8080/v1", "openai_compat", true}, + {"remote 169.254.169.254", "http://169.254.169.254/latest/meta-data/", "openai_compat", true}, + + // --- Regression: unspecified addresses (bug missed by #974 hand-rolled checks) --- + {"0.0.0.0 unspecified", "http://0.0.0.0:8080/v1", "openai_compat", true}, + {":: IPv6 unspecified", "http://[::]:8080/v1", "openai_compat", true}, + + // --- DNS bypass via wildcard services (nip.io, sslip.io) --- + {"nip.io 10.x", "http://10.10.27.30.nip.io:9999/v1", "openai_compat", true}, + {"sslip.io 192.168.x", "http://192.168.1.1.sslip.io:8080/v1", "openai_compat", true}, + {"nip.io 172.16.x", "http://172.16.0.5.nip.io:8080/v1", "openai_compat", true}, + {"nip.io link-local", "http://169.254.169.254.nip.io/latest/", "openai_compat", true}, + {"nip.io loopback", "http://127.0.0.1.nip.io:8080/v1", "openai_compat", true}, + {"attacker rebind", "http://rebind.attacker.com:8080/v1", "openai_compat", true}, + {"dual-stack any private", "http://dual-stack.example.com:8080/v1", "openai_compat", true}, + + // --- Internal hostname suffix --- + {".internal suffix", "http://metadata.google.internal/computeMetadata/v1/", "openai_compat", true}, + {".local suffix", "http://myservice.local:8080", "openai_compat", true}, + + // --- Unresolvable hostname --- + {"unresolvable hostname", "http://nonexistent.invalid:8080/v1", "openai_compat", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateProviderURL(tt.rawURL, tt.providerType) + if (err != nil) != tt.wantErr { + t.Errorf("validateProviderURL(%q, %q) error = %v, wantErr %v", + tt.rawURL, tt.providerType, err, tt.wantErr) + } + }) + } +} + +// --- Env gate: GOCLAW_ALLOW_PRIVATE_PROVIDER_URLS --- + +func TestValidateProviderURL_AllowPrivateFlag(t *testing.T) { + saveAndRestoreGlobals(t) + + dnsResolverFn = stubResolver(map[string][]string{ + "my-vllm.lan": {"10.0.0.1"}, + }) + + t.Run("private URL allowed when flag is true", func(t *testing.T) { + allowPrivateProviderURLsFn = func() bool { return true } + if err := validateProviderURL("http://my-vllm.lan:8080/v1", "openai_compat"); err != nil { + t.Errorf("expected nil with allow-private flag, got: %v", err) + } + if err := validateProviderURL("http://192.168.1.50:8080/v1", "openai_compat"); err != nil { + t.Errorf("expected nil for private IP with allow-private flag, got: %v", err) + } + if err := validateProviderURL("http://localhost:8080/v1", "openai_compat"); err != nil { + t.Errorf("expected nil for localhost with allow-private flag, got: %v", err) + } + if err := validateProviderURL("http://llm.internal/v1", "openai_compat"); err != nil { + t.Errorf("expected nil for .internal with allow-private flag, got: %v", err) + } + }) + + t.Run("private URL blocked when flag is false", func(t *testing.T) { + allowPrivateProviderURLsFn = func() bool { return false } + if err := validateProviderURL("http://my-vllm.lan:8080/v1", "openai_compat"); err == nil { + t.Error("expected error for private URL without allow-private flag") + } + }) + + t.Run("scheme still enforced even with allow-private flag", func(t *testing.T) { + allowPrivateProviderURLsFn = func() bool { return true } + err := validateProviderURL("file:///etc/passwd", "openai_compat") + if err == nil { + t.Error("expected error for file:// scheme even with allow-private flag") + } + if !strings.Contains(err.Error(), "scheme") { + t.Errorf("expected scheme error, got: %v", err) + } + }) +} + +func TestValidateProviderURL_LocalTypesIgnoreAllowPrivateFlag(t *testing.T) { + saveAndRestoreGlobals(t) + allowPrivateProviderURLsFn = func() bool { return true } + + cases := []struct { + url string + providerType string + }{ + {"http://ollama:11434/v1", "ollama"}, + {"http://host.lan:11434/v1", "ollama"}, + {"http://10.0.0.5:11434/v1", "ollama"}, + {"http://acp-sidecar:9090", "acp"}, + } + for _, c := range cases { + if err := validateProviderURL(c.url, c.providerType); err == nil { + t.Errorf("expected local provider URL %s / %s to remain blocked despite allow-private flag", c.url, c.providerType) + } + } +} + +// --- H-1: scheme enforced for local provider types (from PR #972) --- + +func TestValidateProviderURL_LocalTypeSchemeEnforced(t *testing.T) { + saveAndRestoreGlobals(t) + + cases := []struct { + url string + providerType string + }{ + {"file:///etc/passwd", "ollama"}, + {"gopher://localhost:25", "ollama"}, + {"file:///etc/passwd", "acp"}, + } + for _, c := range cases { + err := validateProviderURL(c.url, c.providerType) + if err == nil { + t.Errorf("expected scheme error for %s / %s, got nil", c.url, c.providerType) + continue + } + if !strings.Contains(err.Error(), "scheme") { + t.Errorf("expected scheme error for %s / %s, got: %v", c.url, c.providerType, err) + } + } +} + +// --- Regression: 0.0.0.0 and :: unspecified (bug PR #974 missed) --- + +func TestValidateProviderURL_UnspecifiedBlocked(t *testing.T) { + saveAndRestoreGlobals(t) + allowPrivateProviderURLsFn = func() bool { return false } + + cases := []string{ + "http://0.0.0.0:8080/v1", + "http://[::]:8080/v1", + } + for _, u := range cases { + err := validateProviderURL(u, "openai_compat") + if err == nil { + t.Errorf("expected error for unspecified address %q, got nil", u) + } + } +} + +// --- DNS-rebinding: remote type resolving to private/loopback --- + +func TestValidateProviderURL_DNSRebindBlocked(t *testing.T) { + saveAndRestoreGlobals(t) + allowPrivateProviderURLsFn = func() bool { return false } + + privateHosts := map[string][]string{ + "rebind-private.example.com": {"10.0.0.1"}, + "rebind-loopback.example.com": {"127.0.0.1"}, + "rebind-linklocal.example.com": {"169.254.169.254"}, + } + dnsResolverFn = stubResolver(privateHosts) + + for host := range privateHosts { + err := validateProviderURL("http://"+host+":8080/v1", "openai_compat") + if err == nil { + t.Errorf("expected DNS rebind to be blocked for %s, got nil", host) + } + } +} + +// --- Local type: all allowed variants --- + +func TestValidateProviderURL_LocalTypeAllowedHosts(t *testing.T) { + saveAndRestoreGlobals(t) + + allowed := []struct { + url string + providerType string + }{ + {"http://localhost:11434/v1", "ollama"}, + {"http://127.0.0.1:11434/v1", "ollama"}, + {"http://[::1]:11434/v1", "ollama"}, + {"http://host.docker.internal:11434/v1", "ollama"}, + {"http://localhost:9090", "acp"}, + {"http://127.0.0.1:9090", "acp"}, + } + for _, a := range allowed { + if err := validateProviderURL(a.url, a.providerType); err != nil { + t.Errorf("expected %s / %s to be allowed, got: %v", a.url, a.providerType, err) + } + } +} + +func TestValidateProviderURL_ClaudeCLIExecutablePath(t *testing.T) { + saveAndRestoreGlobals(t) + absClaudePath := filepath.Join(t.TempDir(), "claude") + + allowed := []string{ + "", + "claude", + absClaudePath, + filepath.Join(t.TempDir(), "Claude Code.app", "Contents", "MacOS", "claude"), + } + for _, raw := range allowed { + if err := validateProviderURL(raw, "claude_cli"); err != nil { + t.Errorf("expected Claude CLI executable %q to be allowed, got: %v", raw, err) + } + } + + blocked := []string{ + "file:///usr/local/bin/claude", + "https://api.anthropic.com/v1", + "relative/claude", + "claude --dangerously-skip-permissions", + } + for _, raw := range blocked { + if err := validateProviderURL(raw, "claude_cli"); err == nil { + t.Errorf("expected Claude CLI executable %q to be rejected", raw) + } + } +} + +// --- Public remote URL always OK --- + +func TestValidateProviderURL_PublicHostOK(t *testing.T) { + saveAndRestoreGlobals(t) + allowPrivateProviderURLsFn = func() bool { return false } + dnsResolverFn = stubResolver(map[string][]string{ + "api.openai.com": {"104.18.6.192"}, + }) + + if err := validateProviderURL("https://api.openai.com/v1", "openai_compat"); err != nil { + t.Errorf("expected public host to pass, got: %v", err) + } +} diff --git a/internal/pipeline/deps.go b/internal/pipeline/deps.go index a8054ca2..43ed7ca7 100644 --- a/internal/pipeline/deps.go +++ b/internal/pipeline/deps.go @@ -89,6 +89,9 @@ type PipelineDeps struct { ExecuteToolRaw func(ctx context.Context, tc providers.ToolCall) (providers.Message, any, error) // ProcessToolResult processes a raw tool result with state mutation (sequential only). ProcessToolResult func(ctx context.Context, state *RunState, tc providers.ToolCall, rawMsg providers.Message, rawData any) []providers.Message + // AuthorizeToolCall validates whether a tool call is allowed to execute. + // Used by ToolStage as a runtime guard against out-of-policy tool calls. + AuthorizeToolCall func(ctx context.Context, state *RunState, tc providers.ToolCall) (bool, string) // SequentialToolCall returns true for tools that must preserve same-response order. // When any tool call in a batch matches, ToolStage uses ExecuteToolCall for the // whole batch instead of parallel raw execution. diff --git a/internal/pipeline/substates.go b/internal/pipeline/substates.go index b3ae564d..0b38003c 100644 --- a/internal/pipeline/substates.go +++ b/internal/pipeline/substates.go @@ -52,6 +52,9 @@ type PruneState struct { // ToolState: owned by ToolStage. type ToolState struct { + // AllowedTools is the per-iteration execution allowlist built from tool + // definitions sent to the provider. Nil means "no runtime restriction". + AllowedTools map[string]bool LoopDetector any // concrete type toolLoopState lives in agent; Phase 5 defines LoopDetector interface TotalToolCalls int AsyncToolCalls []string // tool names that executed async (spawn) diff --git a/internal/pipeline/think_stage.go b/internal/pipeline/think_stage.go index 582a79a2..c5dd6640 100644 --- a/internal/pipeline/think_stage.go +++ b/internal/pipeline/think_stage.go @@ -41,6 +41,13 @@ func (s *ThinkStage) Execute(ctx context.Context, state *RunState) error { if err != nil { return fmt.Errorf("build tools: %w", err) } + allowed := make(map[string]bool, len(toolDefs)) + for _, td := range toolDefs { + allowed[td.Function.Name] = true + } + state.Tool.AllowedTools = allowed + } else { + state.Tool.AllowedTools = nil } // 3. Construct ChatRequest diff --git a/internal/pipeline/tool_stage.go b/internal/pipeline/tool_stage.go index ac6c7a1e..8f41a5f4 100644 --- a/internal/pipeline/tool_stage.go +++ b/internal/pipeline/tool_stage.go @@ -64,6 +64,22 @@ func (s *ToolStage) Execute(ctx context.Context, state *RunState) error { } } + // Fail-closed authorization gate: reject calls not present in the + // per-iteration allowlist built by ThinkStage. Operates on the canonical + // (prefix-stripped) name so toolCallPrefix agents are handled correctly. + if s.deps.AuthorizeToolCall != nil { + if ok, reason := s.deps.AuthorizeToolCall(ctx, state, tc); !ok { + state.Messages.AppendPending(providers.Message{ + Role: "tool", + Content: reason, + ToolCallID: tc.ID, + IsError: true, + }) + state.Tool.TotalToolCalls++ + continue + } + } + // Hook: sync PreToolUse — block if hook denies. Builtin-source hooks may // rewrite tc.Arguments via UpdatedToolInput (e.g. path-sanitizer); apply // before ExecuteToolCall so the rewrite is authoritative. @@ -179,10 +195,35 @@ func (s *ToolStage) executeParallel(ctx context.Context, state *RunState, toolCa err error } + // Filter out unauthorized calls before dispatching parallel I/O. + // Keeps error messages deterministic (same order as toolCalls slice). + filteredCalls := make([]providers.ToolCall, 0, len(toolCalls)) + for _, tc := range toolCalls { + if s.deps.AuthorizeToolCall != nil { + if ok, reason := s.deps.AuthorizeToolCall(ctx, state, tc); !ok { + state.Messages.AppendPending(providers.Message{ + Role: "tool", + Content: reason, + ToolCallID: tc.ID, + IsError: true, + }) + state.Tool.TotalToolCalls++ + continue + } + } + filteredCalls = append(filteredCalls, tc) + } + // When all calls were blocked, still run exit-condition checks (read-only + // streak, MaxToolCalls budget) for consistency with the sequential path. + if len(filteredCalls) == 0 { + s.checkExitConditions(state) + return nil + } + // Phase 1: parallel I/O (no state mutation) - results := make([]rawResult, len(toolCalls)) + results := make([]rawResult, len(filteredCalls)) var wg sync.WaitGroup - for i, tc := range toolCalls { + for i, tc := range filteredCalls { wg.Add(1) go func(idx int, tc providers.ToolCall) { defer wg.Done() diff --git a/internal/pipeline/tool_stage_authorize_test.go b/internal/pipeline/tool_stage_authorize_test.go new file mode 100644 index 00000000..71691b22 --- /dev/null +++ b/internal/pipeline/tool_stage_authorize_test.go @@ -0,0 +1,216 @@ +package pipeline + +import ( + "context" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/providers" +) + +// makeAuthorizeGate builds a PipelineDeps.AuthorizeToolCall callback backed by +// a static allowlist and an optional deferred-activator, mirroring the contract +// that makeAuthorizeToolCall in loop_pipeline_callbacks.go provides. +func makeAuthorizeGate( + allowed map[string]bool, // nil = allow-all + tryActivate func(name string) bool, // nil = no activator + isDenied func(name string) bool, // nil = never denied +) func(ctx context.Context, state *RunState, tc providers.ToolCall) (bool, string) { + return func(_ context.Context, state *RunState, tc providers.ToolCall) (bool, string) { + a := state.Tool.AllowedTools + if a == nil { + return true, "" + } + if a[tc.Name] { + return true, "" + } + if tryActivate != nil && tryActivate(tc.Name) { + if isDenied != nil && isDenied(tc.Name) { + return false, "tool not allowed by policy: " + tc.Name + } + a[tc.Name] = true + return true, "" + } + return false, "tool not allowed by policy: " + tc.Name + } +} + +// makeMinimalDeps returns a PipelineDeps just sufficient for ToolStage tests. +func makeMinimalDeps(authorize func(context.Context, *RunState, providers.ToolCall) (bool, string)) *PipelineDeps { + return &PipelineDeps{ + Config: PipelineConfig{MaxToolCalls: 100}, + AuthorizeToolCall: authorize, + ExecuteToolCall: func(_ context.Context, state *RunState, tc providers.ToolCall) ([]providers.Message, error) { + return []providers.Message{{Role: "tool", Content: "ok", ToolCallID: tc.ID}}, nil + }, + } +} + +// stateWithAllowedTools returns a minimal RunState with AllowedTools pre-set. +func stateWithAllowedTools(allowed map[string]bool) *RunState { + st := defaultState() + st.Tool.AllowedTools = allowed + return st +} + +func toolCallFor(name string) providers.ToolCall { + return providers.ToolCall{ID: "tc-" + name, Name: name} +} + +// fakeResponse puts tool calls into the state's ThinkStage so ToolStage finds them. +func setLastResponse(state *RunState, calls []providers.ToolCall) { + state.Think.LastResponse = &providers.ChatResponse{ToolCalls: calls} +} + +// --- Tests --- + +// TestAuthorizeGate_NilAllowlist_AllowsAll verifies that a nil AllowedTools map +// means no per-iteration restriction (allow-all semantics). +func TestAuthorizeGate_NilAllowlist_AllowsAll(t *testing.T) { + t.Parallel() + st := defaultState() + st.Tool.AllowedTools = nil // explicit nil + + gate := makeAuthorizeGate(nil, nil, nil) + tc := toolCallFor("exec") + + ok, reason := gate(context.Background(), st, tc) + if !ok { + t.Errorf("nil allowlist must allow all tools; got blocked with reason %q", reason) + } +} + +// TestAuthorizeGate_NamePresent_Allows checks that a tool explicitly in the +// allowlist passes through. +func TestAuthorizeGate_NamePresent_Allows(t *testing.T) { + t.Parallel() + st := stateWithAllowedTools(map[string]bool{"exec": true, "read_file": true}) + + gate := makeAuthorizeGate(nil, nil, nil) + for _, name := range []string{"exec", "read_file"} { + ok, reason := gate(context.Background(), st, toolCallFor(name)) + if !ok { + t.Errorf("tool %q should be allowed; got blocked: %q", name, reason) + } + } +} + +// TestAuthorizeGate_NameAbsent_NoActivator_Denies confirms fail-closed: a name +// not in the allowlist with no activator is blocked. +func TestAuthorizeGate_NameAbsent_NoActivator_Denies(t *testing.T) { + t.Parallel() + st := stateWithAllowedTools(map[string]bool{"read_file": true}) + + gate := makeAuthorizeGate(nil, nil, nil) + ok, reason := gate(context.Background(), st, toolCallFor("exec")) + if ok { + t.Error("tool absent from allowlist with no activator must be denied") + } + if reason == "" { + t.Error("denial must include a non-empty reason") + } +} + +// TestAuthorizeGate_DeferredActivate_ThenDenied ensures that lazy activation +// followed by a positive IsDenied check still blocks the tool. +func TestAuthorizeGate_DeferredActivate_ThenDenied(t *testing.T) { + t.Parallel() + st := stateWithAllowedTools(map[string]bool{}) + + activated := false + tryActivate := func(name string) bool { + activated = true + return name == "mcp_svc__exec_cmd" + } + isDenied := func(name string) bool { + return name == "mcp_svc__exec_cmd" // explicitly denied + } + + gate := makeAuthorizeGate(nil, tryActivate, isDenied) + ok, reason := gate(context.Background(), st, toolCallFor("mcp_svc__exec_cmd")) + + if !activated { + t.Error("expected TryActivateDeferred to be called") + } + if ok { + t.Error("tool allowed by activator but denied by policy must still be blocked") + } + if reason == "" { + t.Error("denial must include a non-empty reason") + } + if st.Tool.AllowedTools["mcp_svc__exec_cmd"] { + t.Error("denied tool must not be added to AllowedTools") + } +} + +// TestAuthorizeGate_PrefixedName_CanonicalLookup is the regression guard for the +// toolCallPrefix bug: when an agent has toolCallPrefix set, the model emits +// "proxy_exec" but AllowedTools is keyed by canonical name "exec". The gate must +// resolve the name before lookup, otherwise every prefixed call is wrongly blocked. +// +// This test simulates the resolution by calling the gate with a *pre-resolved* +// canonical name, matching what makeAuthorizeToolCall does after calling +// resolveToolCallName(tc.Name). The pipeline layer always receives the resolved +// name because AuthorizeToolCall in the real callback operates on the canonical +// form. +func TestAuthorizeGate_PrefixedName_CanonicalLookup(t *testing.T) { + t.Parallel() + // AllowedTools uses canonical name "exec" (as ThinkStage builds it from FilterTools). + st := stateWithAllowedTools(map[string]bool{"exec": true}) + + gate := makeAuthorizeGate(nil, nil, nil) + + // Simulate what resolveToolCallName("proxy_exec") returns with prefix "proxy_": + // the canonical name "exec" is passed to the gate. + resolvedName := "exec" // prefix already stripped by makeAuthorizeToolCall + tc := providers.ToolCall{ID: "tc-prefix", Name: resolvedName} + + ok, reason := gate(context.Background(), st, tc) + if !ok { + t.Errorf("canonical name after prefix resolution must be allowed; reason: %q", reason) + } +} + +// TestToolStage_AllBlocked_CallsCheckExitConditions verifies that when all tool +// calls in the parallel path are blocked, ToolStage still runs checkExitConditions +// (so MaxToolCalls budget is enforced). Regression for early-return-nil bug. +func TestToolStage_AllBlocked_CallsCheckExitConditions(t *testing.T) { + t.Parallel() + + // Allow-all deps but with MaxToolCalls = 1 and AuthorizeToolCall that blocks. + denyAll := func(_ context.Context, _ *RunState, tc providers.ToolCall) (bool, string) { + return false, "blocked: " + tc.Name + } + deps := &PipelineDeps{ + Config: PipelineConfig{MaxToolCalls: 1}, + AuthorizeToolCall: denyAll, + ExecuteToolCall: func(_ context.Context, state *RunState, tc providers.ToolCall) ([]providers.Message, error) { + return nil, nil + }, + ExecuteToolRaw: func(_ context.Context, tc providers.ToolCall) (providers.Message, any, error) { + return providers.Message{}, nil, nil + }, + ProcessToolResult: func(_ context.Context, state *RunState, tc providers.ToolCall, rawMsg providers.Message, rawData any) []providers.Message { + return nil + }, + } + + stage := NewToolStage(deps) + state := defaultState() + state.Tool.AllowedTools = map[string]bool{} // non-nil allowlist → gate is active + state.Tool.TotalToolCalls = 1 // already at MaxToolCalls + + // Two tool calls — parallel path is triggered (len > 1). + setLastResponse(state, []providers.ToolCall{ + toolCallFor("exec"), + toolCallFor("write_file"), + }) + + if err := stage.Execute(context.Background(), state); err != nil { + t.Fatalf("Execute returned error: %v", err) + } + + // checkExitConditions must have fired: TotalToolCalls >= MaxToolCalls → BreakLoop. + if stage.Result() != BreakLoop { + t.Errorf("expected BreakLoop after all-blocked + MaxToolCalls reached, got %v", stage.Result()) + } +} diff --git a/internal/providers/defaults.go b/internal/providers/defaults.go index 29c8e616..bb464703 100644 --- a/internal/providers/defaults.go +++ b/internal/providers/defaults.go @@ -38,6 +38,11 @@ func NewDefaultTransport() *http.Transport { // NewDefaultHTTPClient returns an *http.Client backed by NewDefaultTransport. // No Client.Timeout is set — rely on ctx deadlines and Transport stage timeouts. +// +// SSRF protection for user-configured provider URLs is enforced at provider +// create/update time by validateProviderURL (resolves the host and rejects +// private/reserved IPs via security.IsBlocked). Dial-time DNS-rebinding +// hardening is tracked as a follow-up. func NewDefaultHTTPClient() *http.Client { return &http.Client{Transport: NewDefaultTransport()} } diff --git a/internal/providers/embedding_openai.go b/internal/providers/embedding_openai.go index e98fea8e..788a747c 100644 --- a/internal/providers/embedding_openai.go +++ b/internal/providers/embedding_openai.go @@ -40,8 +40,11 @@ func NewOpenAIEmbeddingProvider(apiKey, apiBase, model string) *OpenAIEmbeddingP apiKey: apiKey, apiBase: strings.TrimRight(apiBase, "/"), model: model, - client: &http.Client{Timeout: 60 * time.Second}, - retry: DefaultRetryConfig(), + // Use a fixed 60s timeout client. Embedding requests are short but can be + // batched. The apiBase is validated at provider-creation time by + // validateProviderURL in internal/http/providers.go. + client: &http.Client{Timeout: 60 * time.Second}, + retry: DefaultRetryConfig(), } } diff --git a/internal/providers/vertex.go b/internal/providers/vertex.go index f39f9b21..8c5cc516 100644 --- a/internal/providers/vertex.go +++ b/internal/providers/vertex.go @@ -131,11 +131,15 @@ func NewVertexProvider(ctx context.Context, cfg VertexConfig) (*OpenAIProvider, // then transparently fetches a fresh one. No extra work for callers. cached := oauth2.ReuseTokenSource(nil, tokenSource) + // Use NewDefaultTransport as the oauth2 base transport so Vertex requests + // inherit the same per-stage timeouts as other providers. SSRF defense is + // provided by validateVertexAPIBaseOverride, which enforces the googleapis.com + // host constraint at provider-creation time. client := &http.Client{ Timeout: DefaultHTTPTimeout, Transport: &oauth2.Transport{ Source: cached, - Base: http.DefaultTransport, + Base: NewDefaultTransport(), }, } diff --git a/internal/sandbox/docker_test.go b/internal/sandbox/docker_test.go index 50b4fc94..7efe5678 100644 --- a/internal/sandbox/docker_test.go +++ b/internal/sandbox/docker_test.go @@ -172,31 +172,3 @@ func TestFsBridgePathWithinUsesPathBoundaries(t *testing.T) { }) } } - -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 15278641..465e7379 100644 --- a/internal/sandbox/fsbridge.go +++ b/internal/sandbox/fsbridge.go @@ -80,8 +80,12 @@ func (b *FsBridge) WriteFile(ctx context.Context, path, content string, appendMo } } - ddArgs := fsBridgeWriteDDArgs(resolved, appendMode) - _, stderr, exitCode, err := b.dockerExec(ctx, []byte(content), ddArgs...) + // Write content via stdin without invoking a shell. Passing the resolved path + // as a discrete argv entry prevents shell metacharacters in filenames from + // being interpreted as commands inside the sandbox container. + // "--" terminates option parsing so paths starting with "-" are safe. + teeArgs := fsBridgeWriteTeeArgs(resolved, appendMode) + _, stderr, exitCode, err := b.dockerExec(ctx, []byte(content), teeArgs...) if err != nil { return fmt.Errorf("fsbridge write: %w", err) } @@ -92,11 +96,12 @@ 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} +func fsBridgeWriteTeeArgs(resolved string, appendMode bool) []string { + args := []string{"tee"} if appendMode { - args = append(args, "conv=notrunc", "oflag=append") + args = append(args, "-a") } + args = append(args, "--", resolved) return args } diff --git a/internal/sandbox/fsbridge_test.go b/internal/sandbox/fsbridge_test.go new file mode 100644 index 00000000..493a81ad --- /dev/null +++ b/internal/sandbox/fsbridge_test.go @@ -0,0 +1,189 @@ +package sandbox + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" +) + +// fakeDockerScript is a fake `docker` binary used in WriteFile tests. +// It logs all argv to $DOCKER_LOG and, when invoked with a `tee` sub-command, +// captures stdin to $DOCKER_STDIN. +// `realpath -e -- ` echoes the path back (simulates existing path). +// `mkdir` exits 0 silently. +const fakeDockerScript = `#!/bin/sh +{ + echo CALL + i=0 + for arg in "$@"; do + echo "ARG[$i]=$arg" + i=$((i + 1)) + done +} >> "$DOCKER_LOG" + +# realpath: echo the last arg back as the resolved path +for j in $(seq 0 $#); do + eval "a=\${$j}" + if [ "$a" = "realpath" ]; then + # print the last argument (the path) + eval "echo \"\${$#}\"" + exit 0 + fi +done + +# tee: capture stdin +for arg in "$@"; do + if [ "$arg" = "tee" ]; then + if [ -n "$DOCKER_STDIN" ]; then + cat > "$DOCKER_STDIN" + else + cat > /dev/null + fi + exit 0 + fi +done + +exit 0 +` + +// installFakeDocker writes the fake docker script to tmp, prepends tmp to PATH, +// and sets DOCKER_LOG and optionally DOCKER_STDIN env vars for the test. +func installFakeDocker(t *testing.T, tmp, logPath, stdinPath string) { + t.Helper() + dockerPath := filepath.Join(tmp, "docker") + if err := os.WriteFile(dockerPath, []byte(fakeDockerScript), 0o755); err != nil { + t.Fatalf("write fake docker: %v", err) + } + t.Setenv("PATH", tmp+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("DOCKER_LOG", logPath) + if stdinPath != "" { + t.Setenv("DOCKER_STDIN", stdinPath) + } +} + +// TestFsBridgeWriteFileDoesNotInvokeShell asserts that WriteFile passes the +// resolved path as a discrete argv entry to `tee -- ` and does NOT +// build a shell command string (no sh / -c / shell metachar expansion). +func TestFsBridgeWriteFileDoesNotInvokeShell(t *testing.T) { + tmp := t.TempDir() + logPath := filepath.Join(tmp, "docker.log") + stdinPath := filepath.Join(tmp, "stdin.txt") + + installFakeDocker(t, tmp, logPath, stdinPath) + + bridge := NewFsBridge("container-id", "/workspace") + maliciousPath := `nested/evil$(touch /tmp/goclaw-fsbridge-pwned);name.txt` + content := "safe content" + + if err := bridge.WriteFile(context.Background(), maliciousPath, content, false); err != nil { + t.Fatalf("WriteFile returned error: %v", err) + } + + logBytes, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("read docker log: %v", err) + } + log := string(logBytes) + + // Must NOT have invoked sh or built a shell string. + if strings.Contains(log, "=sh") || strings.Contains(log, "=-c") || strings.Contains(log, "cat >") { + t.Fatalf("WriteFile invoked shell command path; log:\n%s", log) + } + // Must have called tee. + if !strings.Contains(log, "=tee") { + t.Fatalf("expected write command to use tee without shell; log:\n%s", log) + } + // Must have included -- to terminate option parsing. + if !strings.Contains(log, "=--") { + t.Fatalf("expected tee delimiter (--) before filename; log:\n%s", log) + } + // The malicious path must appear verbatim as a single argv entry (not split/executed). + resolved := "/workspace/" + maliciousPath + if !strings.Contains(log, "="+resolved) { + t.Fatalf("expected malicious filename to remain one argv entry; log:\n%s", log) + } + + stdinBytes, err := os.ReadFile(stdinPath) + if err != nil { + t.Fatalf("read captured stdin: %v", err) + } + if string(stdinBytes) != content { + t.Fatalf("stdin content = %q, want %q", string(stdinBytes), content) + } +} + +// TestFsBridgeWriteFileAppendUsesTeeAppendArg asserts that append mode passes +// -a before -- in the tee argv, i.e. `tee -a -- `. +func TestFsBridgeWriteFileAppendUsesTeeAppendArg(t *testing.T) { + tmp := t.TempDir() + logPath := filepath.Join(tmp, "docker.log") + + installFakeDocker(t, tmp, logPath, "") + + bridge := NewFsBridge("container-id", "/workspace") + if err := bridge.WriteFile(context.Background(), "append.txt", "more", true); err != nil { + t.Fatalf("WriteFile append returned error: %v", err) + } + + logBytes, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("read docker log: %v", err) + } + log := string(logBytes) + + // Must have: tee, then -a, then --. + if !strings.Contains(log, "=tee") { + t.Fatalf("expected tee in argv; log:\n%s", log) + } + if !strings.Contains(log, "=-a") { + t.Fatalf("expected -a flag for append mode; log:\n%s", log) + } + if !strings.Contains(log, "=--") { + t.Fatalf("expected -- delimiter in argv; log:\n%s", log) + } +} + +// TestFsBridgeWriteTeeArgs_Overwrite checks the tee args slice for overwrite mode. +func TestFsBridgeWriteTeeArgs_Overwrite(t *testing.T) { + args := fsBridgeWriteTeeArgs("/workspace/file.txt", false) + if len(args) != 3 { + t.Fatalf("expected 3 args, got %d: %v", len(args), args) + } + if args[0] != "tee" { + t.Errorf("args[0] = %q, want tee", args[0]) + } + if args[1] != "--" { + t.Errorf("args[1] = %q, want --", args[1]) + } + if args[2] != "/workspace/file.txt" { + t.Errorf("args[2] = %q, want /workspace/file.txt", args[2]) + } + // Must not contain -a in overwrite mode. + for _, a := range args { + if a == "-a" { + t.Errorf("overwrite mode must not contain -a: %v", args) + } + } +} + +// TestFsBridgeWriteTeeArgs_Append checks the tee args slice for append mode. +func TestFsBridgeWriteTeeArgs_Append(t *testing.T) { + args := fsBridgeWriteTeeArgs("/workspace/file.txt", true) + if len(args) != 4 { + t.Fatalf("expected 4 args, got %d: %v", len(args), args) + } + if args[0] != "tee" { + t.Errorf("args[0] = %q, want tee", args[0]) + } + if args[1] != "-a" { + t.Errorf("args[1] = %q, want -a", args[1]) + } + if args[2] != "--" { + t.Errorf("args[2] = %q, want --", args[2]) + } + if args[3] != "/workspace/file.txt" { + t.Errorf("args[3] = %q, want /workspace/file.txt", args[3]) + } +} diff --git a/internal/security/ssrf.go b/internal/security/ssrf.go index 5dd170c3..45b71a12 100644 --- a/internal/security/ssrf.go +++ b/internal/security/ssrf.go @@ -77,6 +77,16 @@ func isBlocked(ip net.IP) bool { return false } +// IsBlocked reports whether ip falls within any blocked CIDR (loopback, +// link-local including cloud-metadata 169.254.169.254, RFC 1918 private, +// multicast, and unspecified 0.0.0.0/:: ranges). +// +// Use this in provider-URL validation and dial-time guards to avoid +// duplicating the CIDR list across packages. +func IsBlocked(ip net.IP) bool { + return isBlocked(ip) +} + // redactURL strips query string and userinfo for safe logging. func redactURL(rawURL string) string { u, err := url.Parse(rawURL) diff --git a/internal/tools/policy.go b/internal/tools/policy.go index 7d83dc3b..28cd574d 100644 --- a/internal/tools/policy.go +++ b/internal/tools/policy.go @@ -53,7 +53,7 @@ var toolProfiles = map[string][]string{ } // Legacy tool aliases — migrated to Registry.RegisterAlias() at startup. -// Kept as seed data only; resolveAlias() is no longer used. +// resolveAlias() is used by IsDenied to expand names before deny-spec matching. var legacyToolAliases = map[string]string{ "bash": "exec", "apply-patch": "apply_patch", @@ -434,19 +434,34 @@ func unionWithSpec(reg *Registry, current []string, allTools []string, spec []st // IsDenied checks if a tool name is explicitly denied by global or agent policy. // Used to prevent lazy-activated deferred tools from bypassing the deny list. +// Checks under all candidate names: the raw name, the legacy alias (e.g. bash→exec), +// and the registry alias when available. func (pe *PolicyEngine) IsDenied(name string, agentPolicy *config.ToolPolicySpec) bool { + candidates := map[string]struct{}{name: {}} + // Keep legacy alias compatibility (e.g. bash -> exec). + candidates[resolveAlias(name)] = struct{}{} + // Include registry alias mapping when available. pe.mu.RLock() reg := pe.registry pe.mu.RUnlock() + if reg != nil { + if canonical, ok := reg.Aliases()[name]; ok && canonical != "" { + candidates[canonical] = struct{}{} + } + } if pe.globalPolicy != nil { - if matchDenySpec(reg, name, pe.globalPolicy.Deny) { - return true + for candidate := range candidates { + if matchDenySpec(reg, candidate, pe.globalPolicy.Deny) { + return true + } } } if agentPolicy != nil { - if matchDenySpec(reg, name, agentPolicy.Deny) { - return true + for candidate := range candidates { + if matchDenySpec(reg, candidate, agentPolicy.Deny) { + return true + } } } return false From 8abc16ee88b9b1bb674f35c20bf8cf813b057808 Mon Sep 17 00:00:00 2001 From: Ojus M Save Date: Thu, 4 Jun 2026 11:18:07 -0700 Subject: [PATCH 11/13] fix(providers): omit temperature for Claude Opus/Sonnet 4.6+ (#1180) Anthropic returns HTTP 400 when temperature is sent to claude-opus-4-6, claude-sonnet-4-6, and newer 4.x models. Agent summoning and chat both hardcode temperature; skip the field for affected model IDs. Co-authored-by: Ojus Save --- internal/providers/adapter_anthropic_test.go | 42 ++++++++++++++++++++ internal/providers/anthropic_request.go | 28 ++++++++++++- 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/internal/providers/adapter_anthropic_test.go b/internal/providers/adapter_anthropic_test.go index da976683..88d4769c 100644 --- a/internal/providers/adapter_anthropic_test.go +++ b/internal/providers/adapter_anthropic_test.go @@ -150,6 +150,48 @@ func TestAnthropicAdapterToRequest_Thinking(t *testing.T) { } } +func TestAnthropicAdapterToRequest_SkipsTemperatureForClaude46(t *testing.T) { + adapter, _ := NewAnthropicAdapter(ProviderConfig{APIKey: "sk-test"}) + + for _, model := range []string{"claude-opus-4-6", "claude-sonnet-4-6", "claude-opus-4-7-20260501"} { + t.Run(model, func(t *testing.T) { + req := ChatRequest{ + Model: model, + Messages: []Message{{Role: "user", Content: "hi"}}, + Options: map[string]any{OptTemperature: 0.7}, + } + data, _, err := adapter.ToRequest(req) + if err != nil { + t.Fatal(err) + } + var body map[string]any + if err := json.Unmarshal(data, &body); err != nil { + t.Fatal(err) + } + if _, hasTemp := body["temperature"]; hasTemp { + t.Errorf("model %q: temperature should be omitted", model) + } + }) + } + + req := ChatRequest{ + Model: "claude-sonnet-4-5-20250929", + Messages: []Message{{Role: "user", Content: "hi"}}, + Options: map[string]any{OptTemperature: 0.7}, + } + data, _, err := adapter.ToRequest(req) + if err != nil { + t.Fatal(err) + } + var body map[string]any + if err := json.Unmarshal(data, &body); err != nil { + t.Fatal(err) + } + if body["temperature"] != 0.7 { + t.Errorf("sonnet 4.5 should keep temperature, got %v", body["temperature"]) + } +} + func TestAnthropicAdapterFromResponse_ToolCalls(t *testing.T) { adapter, _ := NewAnthropicAdapter(ProviderConfig{APIKey: "sk-test"}) diff --git a/internal/providers/anthropic_request.go b/internal/providers/anthropic_request.go index 58dfb3a0..306eb158 100644 --- a/internal/providers/anthropic_request.go +++ b/internal/providers/anthropic_request.go @@ -213,7 +213,9 @@ func (p *AnthropicProvider) buildRequestBody(model string, req ChatRequest, stre body["max_tokens"] = v } if v, ok := req.Options[OptTemperature]; ok { - body["temperature"] = v + if !anthropicSkipsTemperature(model) { + body["temperature"] = v + } } // Enable extended thinking if thinking_level is set @@ -234,6 +236,30 @@ func (p *AnthropicProvider) buildRequestBody(model string, req ChatRequest, stre return body } +// anthropicSkipsTemperature reports whether the Messages API rejects sampling +// parameters for this model. Claude Opus/Sonnet 4.6+ and Opus 4.7+ return HTTP +// 400 when temperature (and top_p/top_k) are included; omit them entirely. +func anthropicSkipsTemperature(model string) bool { + m := strings.ToLower(model) + for _, family := range []string{"claude-opus-4-", "claude-sonnet-4-"} { + after, ok := strings.CutPrefix(m, family) + if !ok { + continue + } + minor := 0 + for _, c := range after { + if c < '0' || c > '9' { + break + } + minor = minor*10 + int(c-'0') + } + if minor >= 6 { + return true + } + } + return false +} + // anthropicThinkingBudget maps a thinking level to a token budget. func anthropicThinkingBudget(level string) int { switch level { From 90552531fb49262a2df6118dc4b8e1603e786e53 Mon Sep 17 00:00:00 2001 From: ollielin Date: Sun, 7 Jun 2026 11:16:15 +0800 Subject: [PATCH 12/13] Fix/pipeline compaction pending loss (#1182) * fix(pipeline): preserve pending messages during compaction When PruneStage or ThinkStage triggers mid-loop compaction, ReplaceHistory() clears the pending buffer. If the current iteration's assistant(tool_calls) message is in pending, it gets discarded. The next iteration then sends tool-result messages without a preceding tool_calls message, causing DeepSeek/OpenAI to reject the request with HTTP 400: Messages with role 'tool' must be a response to a preceding message with 'tool_calls' Fix: save pending before ReplaceHistory and restore it after. Reproduction: - Agent with 64K context window - 4-5 consecutive tool calls (e.g. read_file) in one session - Token count exceeds soft budget during ToolStage - PruneStage compaction clears pending including assistant(tool_calls) - Next iteration fails with HTTP 400 * test(pipeline): regression guards for pending preservation during compaction Add focused unit tests covering both ReplaceHistory call sites that previously dropped the pending buffer: - PruneStage mid-loop compaction must preserve the in-flight assistant(tool_calls) message so tool_calls -> tool_result pairing survives, preventing the OpenAI/DeepSeek 400 on the next provider call. - ThinkStage emergency overflow compaction must preserve already-staged pending messages (e.g. iteration-budget nudges) across the retry. Both tests fail without the preservation fix and pass with it. Co-authored-by: ollielin <2137816+ollielin@users.noreply.github.com> --------- Co-authored-by: Duy /zuey/ Co-authored-by: ollielin Co-authored-by: Thieu Nguyen Co-authored-by: ollielin <2137816+ollielin@users.noreply.github.com> --- .../compaction_pending_preservation_test.go | 131 ++++++++++++++++++ internal/pipeline/prune_stage.go | 10 ++ internal/pipeline/think_stage.go | 5 + 3 files changed, 146 insertions(+) create mode 100644 internal/pipeline/compaction_pending_preservation_test.go diff --git a/internal/pipeline/compaction_pending_preservation_test.go b/internal/pipeline/compaction_pending_preservation_test.go new file mode 100644 index 00000000..e52d09cf --- /dev/null +++ b/internal/pipeline/compaction_pending_preservation_test.go @@ -0,0 +1,131 @@ +package pipeline + +import ( + "context" + "errors" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/providers" +) + +// TestPruneStage_Compaction_PreservesPending is the regression guard for the +// mid-loop compaction tool-pairing bug. ThinkStage appends the current +// iteration's assistant(tool_calls) message to the pending buffer, then +// PruneStage runs next. If compaction fires, CompactMessages only receives +// History() — pending is NOT part of the compaction input — yet ReplaceHistory +// clears pending. Without preservation, the assistant(tool_calls) message is +// dropped, ToolStage later appends orphaned tool_result messages, and the next +// provider call rejects the unpaired tool_result (OpenAI/DeepSeek 400). +// +// This test asserts the pending assistant(tool_calls) message survives a +// mid-loop compaction so tool_calls -> tool_result pairing stays intact. +func TestPruneStage_Compaction_PreservesPending(t *testing.T) { + t.Parallel() + // budget = ContextWindow(1000) - overhead(0) - MaxTokens(100) = 900. + // History of 50 msgs * 100 tokens = 5000 > 900 forces the compaction path. + deps := &PipelineDeps{ + Config: PipelineConfig{ + ContextWindow: 1000, + MaxTokens: 100, + }, + TokenCounter: &mockTokenCounter{countPerMessage: 100}, + PruneMessages: func(msgs []providers.Message, _ int) ([]providers.Message, PruneStats) { + return msgs, PruneStats{} // no reduction — forces fall-through to compaction + }, + CompactMessages: func(_ context.Context, msgs []providers.Message, _ string) ([]providers.Message, error) { + // Compaction summarizes history into a single message. Crucially it + // only ever sees History(), never the pending assistant(tool_calls). + return []providers.Message{{Role: "user", Content: "[compacted summary]"}}, nil + }, + } + stage := NewPruneStage(deps, nil) + state := defaultState() + + history := make([]providers.Message, 50) + for i := range history { + history[i] = providers.Message{Role: "user", Content: "msg"} + } + state.Messages.SetHistory(history) + + // The in-flight assistant(tool_calls) message ThinkStage placed in pending + // this iteration — it must outlive ReplaceHistory. + assistantToolCall := providers.Message{ + Role: "assistant", + ToolCalls: []providers.ToolCall{{ID: "tc-keep", Name: "read_file"}}, + } + state.Messages.AppendPending(assistantToolCall) + + if err := stage.Execute(context.Background(), state); err != nil { + t.Fatalf("Execute() error: %v", err) + } + + if !state.Prune.MidLoopCompacted { + t.Fatal("MidLoopCompacted = false, want true (compaction should have fired)") + } + if state.Compact.CompactionCount != 1 { + t.Errorf("CompactionCount = %d, want 1", state.Compact.CompactionCount) + } + + // The assistant(tool_calls) message must still be pending after compaction. + pending := state.Messages.Pending() + if len(pending) != 1 { + t.Fatalf("Pending() len = %d, want 1 (assistant tool_calls preserved across ReplaceHistory)", len(pending)) + } + if pending[0].Role != "assistant" || len(pending[0].ToolCalls) != 1 || pending[0].ToolCalls[0].ID != "tc-keep" { + t.Errorf("pending[0] = %+v, want the preserved assistant tool_calls message (id tc-keep)", pending[0]) + } + + // History should be the compacted summary, not carrying a duplicate of the + // preserved pending message. + hist := state.Messages.History() + if len(hist) != 1 || hist[0].Content != "[compacted summary]" { + t.Errorf("History() = %+v, want single compacted summary message", hist) + } +} + +// TestThinkStage_EmergencyCompaction_PreservesPending guards the second +// ReplaceHistory call site. When CallLLM reports a context-overflow error, +// ThinkStage runs an emergency compaction and retries the iteration. Pending +// messages already staged this iteration (e.g. iteration-budget nudges from +// maybeInjectNudge) must survive the ReplaceHistory that emergency compaction +// performs, for the same reason as the prune path. +func TestThinkStage_EmergencyCompaction_PreservesPending(t *testing.T) { + t.Parallel() + deps := &PipelineDeps{ + Config: PipelineConfig{MaxIterations: 10, MaxTokens: 1000}, + CallLLM: func(_ context.Context, _ *RunState, _ providers.ChatRequest) (*providers.ChatResponse, error) { + // "context length exceeded" is matched by IsContextOverflowMessage. + return nil, errors.New("context length exceeded for this request") + }, + CompactMessages: func(_ context.Context, msgs []providers.Message, _ string) ([]providers.Message, error) { + return []providers.Message{{Role: "user", Content: "[compacted summary]"}}, nil + }, + } + stage := NewThinkStage(deps) + state := defaultState() + state.Messages.SetHistory([]providers.Message{{Role: "user", Content: "old turn"}}) + + // A pending message staged earlier in this iteration (an iteration nudge). + nudge := providers.Message{Role: "user", Content: "[System] wrap up your work"} + state.Messages.AppendPending(nudge) + + if err := stage.Execute(context.Background(), state); err != nil { + t.Fatalf("Execute() error: %v", err) + } + + // Emergency compaction retries the iteration (Continue) and records one retry. + if stage.Result() != Continue { + t.Errorf("Result() = %v, want Continue (retry after emergency compaction)", stage.Result()) + } + if state.Think.OverflowRetries != 1 { + t.Errorf("OverflowRetries = %d, want 1", state.Think.OverflowRetries) + } + + pending := state.Messages.Pending() + if len(pending) != 1 { + t.Fatalf("Pending() len = %d, want 1 (nudge preserved across emergency ReplaceHistory)", len(pending)) + } + if pending[0].Content != "[System] wrap up your work" { + t.Errorf("pending[0] = %+v, want the preserved nudge message", pending[0]) + } +} diff --git a/internal/pipeline/prune_stage.go b/internal/pipeline/prune_stage.go index efb35ab1..dca6485a 100644 --- a/internal/pipeline/prune_stage.go +++ b/internal/pipeline/prune_stage.go @@ -180,11 +180,21 @@ func (s *PruneStage) Execute(ctx context.Context, state *RunState) error { return nil // no compaction available } + // Save pending before compaction — ReplaceHistory clears pending, + // but pending may contain the current iteration's assistant(tool_calls) + // message which must be preserved to maintain tool_calls → tool pairing. + savedPending := state.Messages.Pending() + compacted, err := s.deps.CompactMessages(ctx, state.Messages.History(), state.Model) if err != nil { return fmt.Errorf("compact messages: %w", err) } state.Messages.ReplaceHistory(compacted) + + // Restore pending messages that were cleared by ReplaceHistory. + for _, msg := range savedPending { + state.Messages.AppendPending(msg) + } state.Prune.MidLoopCompacted = true state.Compact.CompactionCount++ state.Compact.MemoryFlushedThisCycle = false // reset for next cycle diff --git a/internal/pipeline/think_stage.go b/internal/pipeline/think_stage.go index c5dd6640..9ea6383f 100644 --- a/internal/pipeline/think_stage.go +++ b/internal/pipeline/think_stage.go @@ -75,9 +75,14 @@ func (s *ThinkStage) Execute(ctx context.Context, state *RunState) error { // Attempt emergency compaction if s.deps.CompactMessages != nil { originalLen := len(state.Messages.History()) + savedPending := state.Messages.Pending() compacted, compactErr := s.deps.CompactMessages(ctx, state.Messages.History(), state.Model) if compactErr == nil { state.Messages.ReplaceHistory(compacted) + // Restore pending cleared by ReplaceHistory. + for _, msg := range savedPending { + state.Messages.AppendPending(msg) + } slog.Info("emergency_compaction_triggered", "run_id", state.RunID, "original_msgs", originalLen, From 7484d0ec74208641f4c623f983e5f820a1142a41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A0nh=20Nguy=E1=BB=85n?= <44762180+thanh-nguyen-95@users.noreply.github.com> Date: Sun, 7 Jun 2026 17:27:26 +0700 Subject: [PATCH 13/13] fix: cannot import if file is over 1MB (#1184) --- ui/web/nginx.conf | 1 + 1 file changed, 1 insertion(+) diff --git a/ui/web/nginx.conf b/ui/web/nginx.conf index e2205f4c..9a47f9be 100644 --- a/ui/web/nginx.conf +++ b/ui/web/nginx.conf @@ -36,6 +36,7 @@ server { # API proxy location /v1/ { + client_max_body_size 500M; proxy_pass $upstream_backend; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr;