fix(vault): non-master tenant 500, O_NOFOLLOW, slog, *string content

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

Author: aaron-tsar

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,19 +43,23 @@ 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.
 	for ancestor := filepath.Dir(target); ; {
 		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 <tenant-workspace>/<path> 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 <tenant-workspace>/<path>,
+//     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
 			}

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

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 }

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