fix(#141): harden checkStaleDocs against path traversal

Export review.ValidateDocPath and use it in checkStaleDocs before
calling os.Stat. Add filepath.Clean + filepath.Rel confinement check
as defense-in-depth to ensure doc paths from PR-controlled YAML
cannot probe filesystem locations outside repoRoot.

Also add tests covering: ../../etc/passwd, /etc/passwd, ../outside,
a valid present path, and a valid missing path.

Addresses security finding from security-review-bot on PR #142.

cmd/review-bot/validatedocmap.go

@@ -83,6 +83,8 @@ func runValidateDocmap(args []string) int {
 	}
 
 	// --- Check 2: Stale docs ---
+	// checkStaleDocs validates each path before touching the filesystem; see
+	// its documentation for the path-traversal hardening applied.
 	repoRoot := filepath.Clean(*repoRootFlag)
 	staleDocs := checkStaleDocs(cfg, repoRoot)
 	if len(staleDocs) > 0 {
@@ -101,7 +103,14 @@ func runValidateDocmap(args []string) int {
 	return 0
 }
 
-// checkStaleDocs returns deduplicated docs: entries that do not exist under repoRoot.
+// checkStaleDocs returns deduplicated docs: entries that do not exist under
+// repoRoot.
+//
+// Path-traversal hardening: each docPath is validated with
+// review.ValidateDocPath (rejects absolute paths and ".." segments) and then
+// confined to repoRoot via filepath.Clean + filepath.Rel before os.Stat is
+// called. Paths that fail either check are treated as invalid (reported as
+// stale) without touching the host filesystem.
 func checkStaleDocs(cfg *review.DocMapConfig, repoRoot string) []string {
 	seen := make(map[string]struct{})
 	var stale []string
@@ -116,7 +125,25 @@ func checkStaleDocs(cfg *review.DocMapConfig, repoRoot string) []string {
 			}
 			seen[docPath] = struct{}{}
 
-			fullPath := filepath.Join(repoRoot, filepath.FromSlash(docPath))
+			// Guard 1: reject absolute paths and ".." segments sourced from
+			// PR-controlled YAML before joining with repoRoot.
+			if err := review.ValidateDocPath(docPath); err != nil {
+				stale = append(stale, docPath)
+				continue
+			}
+
+			// Guard 2: verify the cleaned joined path does not escape repoRoot.
+			// filepath.Clean resolves any remaining ".." after the join; the
+			// filepath.Rel check confirms the path is still under repoRoot.
+			fullPath := filepath.Clean(filepath.Join(repoRoot, filepath.FromSlash(docPath)))
+			rel, err := filepath.Rel(repoRoot, fullPath)
+			if err != nil || strings.HasPrefix(rel, "..") {
+				stale = append(stale, docPath)
+				continue
+			}
+
+			// Safe to stat: path is relative, contains no "..", and is
+			// confined within repoRoot.
 			if _, err := os.Stat(fullPath); err != nil {
 				stale = append(stale, docPath)
 			}

cmd/review-bot/validatedocmap_test.go

@@ -288,3 +288,50 @@ mappings:
 		t.Errorf("expected docs/shared.md to appear exactly once in stderr (deduplicated), got %d occurrences: %q", count, stderr)
 	}
 }
+
+// TestCheckStaleDocs_PathTraversal verifies that checkStaleDocs rejects
+// traversal and absolute paths without touching the host filesystem.
+func TestCheckStaleDocs_PathTraversal(t *testing.T) {
+	dir := t.TempDir()
+
+	// Baseline: a valid doc that exists.
+	makeDocFile(t, dir, "docs/valid.md")
+
+	tests := []struct {
+		name      string
+		docPath   string
+		wantStale bool
+	}{
+		{"dot-dot traversal", "../../etc/passwd", true},
+		{"dot-dot single", "../outside", true},
+		{"absolute path", "/etc/passwd", true},
+		{"valid present path", "docs/valid.md", false},
+		{"valid missing path", "docs/missing.md", true},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			docmap := makeDocmapYAML(t, `
+mappings:
+  - paths:
+      - "lib/**"
+    docs:
+      - `+tc.docPath+`
+`)
+			code, _, stderr := stdinValidateDocmap(t,
+				"",
+				[]string{"--docmap", docmap, "--repo-root", dir},
+			)
+
+			if tc.wantStale {
+				if code != 1 {
+					t.Errorf("path %q: expected exit 1 (stale/invalid), got %d; stderr: %q", tc.docPath, code, stderr)
+				}
+			} else {
+				if code != 0 {
+					t.Errorf("path %q: expected exit 0 (valid), got %d; stderr: %q", tc.docPath, code, stderr)
+				}
+			}
+		})
+	}
+}

review/docmap.go

@@ -257,7 +257,7 @@ type docEntry struct {
 // If the path is a directory, all .md files under it are returned.
 // If it's a file, a single entry is returned.
 func loadDocEntries(ctx context.Context, fetcher DocFetcher, owner, repo, docPath string) ([]docEntry, error) {
-	if err := validateDocPath(docPath); err != nil {
+	if err := ValidateDocPath(docPath); err != nil {
 		return nil, fmt.Errorf("doc path %q rejected: %w", docPath, err)
 	}
 
@@ -310,11 +310,11 @@ func readFileBytes(path string) ([]byte, error) {
 	return os.ReadFile(path)
 }
 
-// validateDocPath rejects doc paths that could cause path traversal via the
+// ValidateDocPath rejects doc paths that could cause path traversal via the
 // VCS API (absolute paths, any ".." segment). Defense-in-depth: the VCS API
 // should already scope paths to the repo, but we validate locally to avoid
 // any quirk in backend path handling.
-func validateDocPath(p string) error {
+func ValidateDocPath(p string) error {
 	if filepath.IsAbs(p) {
 		return fmt.Errorf("absolute paths not allowed")
 	}

review/docmap_test.go

@@ -11,7 +11,7 @@ import (
 
 // fakeDocFetcher is a mock DocFetcher for tests.
 type fakeDocFetcher struct {
-	files map[string]string // path -> content
+	files map[string]string            // path -> content
 	dirs  map[string]map[string]string // dir path -> (file path -> content)
 }
 
@@ -384,7 +384,7 @@ func TestValidateDocPath(t *testing.T) {
 		"a/b/c",
 	}
 	for _, p := range valid {
-		if err := validateDocPath(p); err != nil {
+		if err := ValidateDocPath(p); err != nil {
 			t.Errorf("expected valid path %q to pass, got error: %v", p, err)
 		}
 	}
@@ -397,7 +397,7 @@ func TestValidateDocPath(t *testing.T) {
 		"a/b/../c",
 	}
 	for _, p := range invalid {
-		if err := validateDocPath(p); err == nil {
+		if err := ValidateDocPath(p); err == nil {
 			t.Errorf("expected path %q to be rejected, but it was accepted", p)
 		}
 	}