Skip to content

Commit 9349da8

Browse files
aaron-tsarclaude
andcommitted
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>
1 parent 4e29ba0 commit 9349da8

2 files changed

Lines changed: 391 additions & 13 deletions

File tree

internal/http/vault_handler_documents.go

Lines changed: 50 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -23,22 +23,52 @@ import (
2323
// pipeline (which reads files by path) can later compute summaries, embeddings
2424
// and links.
2525
func (h *VaultHandler) writeDocumentContent(workspace, relPath string, content []byte) (string, error) {
26-
target := filepath.Join(workspace, filepath.Clean(relPath))
27-
// Symlink/escape protection: the cleaned target must stay inside workspace.
28-
absTarget, err := filepath.Abs(target)
26+
// Resolve the workspace root through any symlinks so all containment checks
27+
// compare canonical paths.
28+
realWS, err := filepath.EvalSymlinks(workspace)
2929
if err != nil {
3030
return "", err
3131
}
32-
absWS, err := filepath.Abs(workspace)
33-
if err != nil {
34-
return "", err
35-
}
36-
if !strings.HasPrefix(absTarget, absWS+string(os.PathSeparator)) && absTarget != absWS {
32+
target := filepath.Join(realWS, filepath.Clean(relPath))
33+
34+
// Lexical containment: blocks "../" and absolute-path escapes.
35+
if target != realWS && !strings.HasPrefix(target, realWS+string(os.PathSeparator)) {
3736
return "", os.ErrInvalid
3837
}
38+
39+
// Symlink containment: a lexical prefix check is not enough — a symlink that
40+
// lives *inside* the workspace but points outside it (e.g. workspace/link ->
41+
// /etc) would otherwise let os.WriteFile follow it and clobber a file beyond
42+
// the workspace. Resolve the deepest already-existing ancestor of the target
43+
// and confirm it still canonicalises to a path inside the workspace, before
44+
// any directory is created or byte written.
45+
for ancestor := filepath.Dir(target); ; {
46+
resolved, rerr := filepath.EvalSymlinks(ancestor)
47+
if rerr == nil {
48+
if resolved != realWS && !strings.HasPrefix(resolved, realWS+string(os.PathSeparator)) {
49+
return "", os.ErrInvalid
50+
}
51+
break
52+
}
53+
if !os.IsNotExist(rerr) {
54+
return "", rerr
55+
}
56+
parent := filepath.Dir(ancestor)
57+
if parent == ancestor {
58+
break // walked to the filesystem root without an existing ancestor
59+
}
60+
ancestor = parent
61+
}
62+
3963
if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil {
4064
return "", err
4165
}
66+
67+
// Refuse to follow a symlink planted at the final path component itself.
68+
if fi, lerr := os.Lstat(target); lerr == nil && fi.Mode()&os.ModeSymlink != 0 {
69+
return "", os.ErrInvalid
70+
}
71+
4272
if err := os.WriteFile(target, content, 0o644); err != nil {
4373
return "", err
4474
}
@@ -48,20 +78,27 @@ func (h *VaultHandler) writeDocumentContent(workspace, relPath string, content [
4878
// publishDocUpserted enqueues the standard EventVaultDocUpserted so the
4979
// enrichment worker re-runs summary/embedding/link extraction for this doc.
5080
// No-op when the event bus is not wired (some test setups).
51-
func (h *VaultHandler) publishDocUpserted(tenantID, agentID, docID, relPath, hash, workspace string) {
81+
func (h *VaultHandler) publishDocUpserted(tenantID uuid.UUID, agentID, docID, relPath, hash, workspace string) {
5282
if h.eventBus == nil {
5383
return
5484
}
85+
tenantIDStr := tenantID.String()
86+
// Start enrichment progress before publishing so the worker pool can't drain
87+
// the event and finish before the progress tracker registers it — same
88+
// ordering handleUpload relies on to avoid that race.
89+
if h.enrichProgress != nil {
90+
h.enrichProgress.Start(1, tenantID)
91+
}
5592
h.eventBus.Publish(eventbus.DomainEvent{
5693
ID: uuid.Must(uuid.NewV7()).String(),
5794
Type: eventbus.EventVaultDocUpserted,
5895
SourceID: docID + ":" + hash,
59-
TenantID: tenantID,
96+
TenantID: tenantIDStr,
6097
AgentID: agentID,
6198
Timestamp: time.Now(),
6299
Payload: eventbus.VaultDocUpsertedPayload{
63100
DocID: docID,
64-
TenantID: tenantID,
101+
TenantID: tenantIDStr,
65102
AgentID: agentID,
66103
Path: relPath,
67104
ContentHash: hash,
@@ -284,7 +321,7 @@ func (h *VaultHandler) handleCreateDocument(w http.ResponseWriter, r *http.Reque
284321
if doc.AgentID != nil {
285322
evAgent = *doc.AgentID
286323
}
287-
h.publishDocUpserted(tenantID.String(), evAgent, doc.ID, body.Path, writtenHash, wsPath)
324+
h.publishDocUpserted(tenantID, evAgent, doc.ID, body.Path, writtenHash, wsPath)
288325
}
289326

290327
// 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
391428
if existing.AgentID != nil {
392429
evAgent = *existing.AgentID
393430
}
394-
h.publishDocUpserted(tenantID.String(), evAgent, existing.ID, existing.Path, writtenHash, wsPath)
431+
h.publishDocUpserted(tenantID, evAgent, existing.ID, existing.Path, writtenHash, wsPath)
395432
}
396433

397434
updated, _ := h.store.GetDocumentByID(r.Context(), tenantID.String(), docID)

0 commit comments

Comments
 (0)