diff --git a/cmd/review-bot/validatedocmap.go b/cmd/review-bot/validatedocmap.go index 41fac7b..0545138 100644 --- a/cmd/review-bot/validatedocmap.go +++ b/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) } diff --git a/cmd/review-bot/validatedocmap_test.go b/cmd/review-bot/validatedocmap_test.go index 55da6f7..b9a0a45 100644 --- a/cmd/review-bot/validatedocmap_test.go +++ b/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) + } + } + }) + } +} diff --git a/review/docmap.go b/review/docmap.go index 7317936..c629167 100644 --- a/review/docmap.go +++ b/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") } diff --git a/review/docmap_test.go b/review/docmap_test.go index 5e42be7..0ae6778 100644 --- a/review/docmap_test.go +++ b/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) } }