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) <noreply@anthropic.com>

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)

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))
+	}
+}