diff --git a/internal/obsidian/exporter.go b/internal/obsidian/exporter.go index e41a3389..bcb79376 100644 --- a/internal/obsidian/exporter.go +++ b/internal/obsidian/exporter.go @@ -38,6 +38,32 @@ func NewExporter(s StoreReader, cfg ExportConfig) *Exporter { return &Exporter{store: s, config: cfg} } +// sanitizePathComponent strips path separators and dot-dot sequences from a +// single path component (project name or observation type), preventing path +// traversal attacks when the value is used inside filepath.Join. +// Any slash, backslash, or ".." element is replaced with "_". +func sanitizePathComponent(s string) string { + // Replace OS separators with underscore. + s = strings.ReplaceAll(s, string(filepath.Separator), "_") + s = strings.ReplaceAll(s, "/", "_") + s = strings.ReplaceAll(s, "\\", "_") + + // Replace ".." with "__" to neutralise traversal. + s = strings.ReplaceAll(s, "..", "__") + + // filepath.Clean on a single component that contains no separators is a + // no-op, but run it anyway to normalise any remaining oddities (e.g. trailing + // dots on Windows). We re-apply the separator guard afterwards because Clean + // can introduce a leading "/" on absolute-looking inputs on Unix. + clean := filepath.Clean(s) + clean = strings.ReplaceAll(clean, string(filepath.Separator), "_") + clean = strings.ReplaceAll(clean, "/", "_") + if clean == "" || clean == "." { + clean = "_" + } + return clean +} + // SetGraphConfig sets the GraphConfig mode on this exporter's config. // This is used by Watcher to force GraphConfigSkip on subsequent cycles (REQ-WATCH-06). func (e *Exporter) SetGraphConfig(mode GraphConfigMode) { @@ -189,16 +215,27 @@ func (e *Exporter) Export() (*ExportResult, error) { } } - // Determine target file path + // Determine target file path — sanitize project and type to prevent + // path traversal (issue #180): strip separators and dot-dot sequences, + // then verify the resolved absolute path stays inside engRoot. project := "unknown" if obs.Project != nil && *obs.Project != "" { - project = *obs.Project + project = sanitizePathComponent(*obs.Project) } + obsType := sanitizePathComponent(obs.Type) slug := Slugify(obs.Title, obs.ID) - relPath := filepath.Join(project, obs.Type, slug+".md") - absDir := filepath.Join(engRoot, project, obs.Type) + relPath := filepath.Join(project, obsType, slug+".md") + absDir := filepath.Join(engRoot, project, obsType) absPath := filepath.Join(engRoot, relPath) + // Containment check: reject any path that escapes engRoot after cleaning. + cleanRoot := filepath.Clean(engRoot) + cleanPath := filepath.Clean(absPath) + if !strings.HasPrefix(cleanPath, cleanRoot+string(filepath.Separator)) { + result.Errors = append(result.Errors, fmt.Errorf("unsafe path rejected (would escape export root): %s", cleanPath)) + continue + } + // Create directory if err := os.MkdirAll(absDir, 0755); err != nil { result.Errors = append(result.Errors, fmt.Errorf("mkdir %s: %w", absDir, err)) diff --git a/internal/obsidian/exporter_test.go b/internal/obsidian/exporter_test.go index cbbd05a5..cb95b1e9 100644 --- a/internal/obsidian/exporter_test.go +++ b/internal/obsidian/exporter_test.go @@ -1,6 +1,8 @@ package obsidian import ( + "path/filepath" + "strings" "testing" "github.com/Gentleman-Programming/engram/internal/store" @@ -546,6 +548,120 @@ func TestExporterCallsGraphConfig(t *testing.T) { }) } +// ─── Security: TestPathTraversalPrevention (Issue #180) ────────────────────── + +// TestPathTraversalPrevention asserts that malicious Project or Type values +// containing path traversal sequences (e.g. "../../etc") or absolute paths +// cannot produce output files that escape the {vault}/engram/ export root. +func TestPathTraversalPrevention(t *testing.T) { + traversalCases := []struct { + name string + project string + obsType string + }{ + { + name: "dotdot in project", + project: "../../etc", + obsType: "bugfix", + }, + { + name: "dotdot in type", + project: "myproject", + obsType: "../../etc", + }, + { + name: "dotdot in both", + project: "../../tmp", + obsType: "../../../var", + }, + { + name: "absolute path in project", + project: "/etc", + obsType: "bugfix", + }, + { + name: "absolute path in type", + project: "myproject", + obsType: "/etc", + }, + { + name: "nested traversal", + project: "foo/../../../secret", + obsType: "bugfix", + }, + } + + for _, tc := range traversalCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + + ms := &mockStore{ + exportData: &store.ExportData{ + Sessions: []store.Session{ + {ID: "sess-1", Project: tc.project}, + }, + Observations: []store.Observation{ + { + ID: 1, + SessionID: "sess-1", + Type: tc.obsType, + Title: "Traversal Test", + Content: "malicious content", + Scope: "project", + CreatedAt: "2026-01-01T10:00:00Z", + UpdatedAt: "2026-01-01T10:00:00Z", + Project: strPtr(tc.project), + }, + }, + Prompts: []store.Prompt{}, + }, + } + + cfg := ExportConfig{VaultPath: dir} + exp := NewExporter(ms, cfg) + result, err := exp.Export() + if err != nil { + t.Fatalf("Export() returned unexpected error: %v", err) + } + + // The observation must either be sanitized (created inside engRoot) + // or skipped due to invalid path — it must NOT escape the export root. + engRoot := dir + "/engram" + + // Walk everything that was written and assert containment. + err = walkDir(dir, func(path string) { + // engRoot itself and state/hub files are fine — only check obs files. + if !isContainedIn(path, engRoot) { + t.Errorf("file written OUTSIDE export root: %s (engRoot=%s)", path, engRoot) + } + }) + if err != nil { + t.Fatalf("walkDir: %v", err) + } + + // If a file was created, verify its absolute clean path is under engRoot. + if result != nil && result.Created > 0 { + // Retrieve the state to find which relative path was recorded. + stateFile := engRoot + "/.engram-sync-state.json" + state, readErr := ReadState(stateFile) + if readErr != nil { + t.Fatalf("ReadState: %v", readErr) + } + for id, relPath := range state.Files { + absPath := filepath.Join(engRoot, relPath) + cleanAbs := filepath.Clean(absPath) + cleanRoot := filepath.Clean(engRoot) + if !strings.HasPrefix(cleanAbs, cleanRoot+string(filepath.Separator)) && + cleanAbs != cleanRoot { + t.Errorf("obs %d: state path escapes export root: %s (engRoot=%s)", id, cleanAbs, cleanRoot) + } + } + } + }) + } +} + // ─── Helpers ───────────────────────────────────────────────────────────────── // buildPipelineFixtures creates a fixture ExportData with 3 projects, 5 sessions, diff --git a/internal/obsidian/testhelpers_test.go b/internal/obsidian/testhelpers_test.go index d9076ad0..63e60ac1 100644 --- a/internal/obsidian/testhelpers_test.go +++ b/internal/obsidian/testhelpers_test.go @@ -3,6 +3,7 @@ package obsidian import ( "os" "path/filepath" + "strings" ) // mkdirAll creates the given directory path (all parents), used in tests. @@ -48,3 +49,23 @@ func countFilesInDir(t interface { } return count } + +// walkDir walks all regular files under root and calls fn for each absolute path. +func walkDir(root string, fn func(path string)) error { + return filepath.Walk(root, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if !info.IsDir() { + fn(path) + } + return nil + }) +} + +// isContainedIn reports whether path is inside (or equal to) root after cleaning. +func isContainedIn(path, root string) bool { + cleanPath := filepath.Clean(path) + cleanRoot := filepath.Clean(root) + return strings.HasPrefix(cleanPath, cleanRoot+string(filepath.Separator)) || cleanPath == cleanRoot +}