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