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