diff --git a/cmd/review-bot/validatedocmap.go b/cmd/review-bot/validatedocmap.go index 0545138..00e8cc1 100644 --- a/cmd/review-bot/validatedocmap.go +++ b/cmd/review-bot/validatedocmap.go @@ -83,10 +83,22 @@ func runValidateDocmap(args []string) int { } // --- Check 2: Stale docs --- + // Resolve repoRoot to an absolute, symlink-free path before any Rel checks + // so that a symlinked --repo-root cannot be used to bypass the escape + // guard in checkStaleDocs. + absRoot, err := filepath.Abs(*repoRootFlag) + if err != nil { + fmt.Fprintf(errWriter, "Error: failed to resolve --repo-root %q: %v\n", *repoRootFlag, err) + return 2 + } + resolvedRoot, err := filepath.EvalSymlinks(absRoot) + if err != nil { + fmt.Fprintf(errWriter, "Error: failed to resolve --repo-root %q: %v\n", *repoRootFlag, err) + return 2 + } // 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) + staleDocs := checkStaleDocs(cfg, resolvedRoot) if len(staleDocs) > 0 { failed = true fmt.Fprintln(errWriter, "ERROR: stale docmap docs: entries (paths do not exist):") @@ -108,9 +120,11 @@ func runValidateDocmap(args []string) int { // // 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. +// confined to repoRoot via filepath.Clean + filepath.Rel before os.Lstat is +// called. Symlinks are treated as stale — a CI tool running against +// PR-controlled content must not follow symlinks that could probe arbitrary +// host paths. Paths that fail any check are treated as invalid (reported as +// stale) without following any symlinks. func checkStaleDocs(cfg *review.DocMapConfig, repoRoot string) []string { seen := make(map[string]struct{}) var stale []string @@ -142,9 +156,15 @@ func checkStaleDocs(cfg *review.DocMapConfig, repoRoot string) []string { continue } - // Safe to stat: path is relative, contains no "..", and is - // confined within repoRoot. - if _, err := os.Stat(fullPath); err != nil { + // Use Lstat (not Stat) so symlinks are never followed. A symlink + // under repoRoot could point anywhere on the host, allowing a + // malicious PR to probe file existence. Treat symlinks as stale. + fi, err := os.Lstat(fullPath) + if err != nil { + stale = append(stale, docPath) + continue + } + if fi.Mode()&os.ModeSymlink != 0 { stale = append(stale, docPath) } } diff --git a/cmd/review-bot/validatedocmap_test.go b/cmd/review-bot/validatedocmap_test.go index b9a0a45..b2aa704 100644 --- a/cmd/review-bot/validatedocmap_test.go +++ b/cmd/review-bot/validatedocmap_test.go @@ -335,3 +335,104 @@ mappings: }) } } + +// TestCheckStaleDocs_SymlinkOutside verifies that a symlink under repoRoot +// pointing outside the repo is treated as stale (not followed). +func TestCheckStaleDocs_SymlinkOutside(t *testing.T) { + dir := t.TempDir() + + // Create a symlink inside repoRoot pointing to a file outside the repo. + // We point at /etc/hostname (exists on Linux CI) but the test does not + // depend on that file existing — Lstat must reject the symlink itself. + linkPath := filepath.Join(dir, "docs", "secret.md") + if err := os.MkdirAll(filepath.Dir(linkPath), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.Symlink("/etc/hostname", linkPath); err != nil { + t.Fatalf("Symlink: %v", err) + } + + docmap := makeDocmapYAML(t, ` +mappings: + - paths: + - "lib/**" + docs: + - docs/secret.md +`) + + code, _, stderr := stdinValidateDocmap(t, + "", + []string{"--docmap", docmap, "--repo-root", dir}, + ) + if code != 1 { + t.Errorf("expected exit 1 for symlink doc, got %d; stderr: %q", code, stderr) + } + if !strings.Contains(stderr, "docs/secret.md") { + t.Errorf("expected stale path in stderr, got %q", stderr) + } +} + +// TestCheckStaleDocs_SymlinkInsideRepo verifies that a symlink pointing to +// another file *within* the repo is also treated as stale. We refuse all +// symlinks regardless of target to keep the check simple and safe. +func TestCheckStaleDocs_SymlinkInsideRepo(t *testing.T) { + dir := t.TempDir() + + // Real doc file. + makeDocFile(t, dir, "docs/real.md") + + // Symlink inside repo pointing at the real file. + linkPath := filepath.Join(dir, "docs", "link.md") + if err := os.Symlink(filepath.Join(dir, "docs", "real.md"), linkPath); err != nil { + t.Fatalf("Symlink: %v", err) + } + + docmap := makeDocmapYAML(t, ` +mappings: + - paths: + - "lib/**" + docs: + - docs/link.md +`) + + code, _, stderr := stdinValidateDocmap(t, + "", + []string{"--docmap", docmap, "--repo-root", dir}, + ) + if code != 1 { + t.Errorf("expected exit 1 for symlink doc (even intra-repo), got %d; stderr: %q", code, stderr) + } +} + +// TestRunValidateDocmap_SymlinkRepoRoot verifies that a --repo-root that is +// itself a symlink to a valid directory resolves correctly (Finding #2). +func TestRunValidateDocmap_SymlinkRepoRoot(t *testing.T) { + realDir := t.TempDir() + makeDocFile(t, realDir, "docs/foo.md") + + // Create a symlink pointing at realDir. + symlinkDir := filepath.Join(t.TempDir(), "link-root") + if err := os.Symlink(realDir, symlinkDir); err != nil { + t.Fatalf("Symlink: %v", err) + } + + docmap := makeDocmapYAML(t, ` +mappings: + - paths: + - "lib/**" + docs: + - docs/foo.md +`) + + // Using the symlinked repo-root: the real doc exists → should be clean. + code, stdout, stderr := stdinValidateDocmap(t, + "lib/foo.go\n", + []string{"--docmap", docmap, "--repo-root", symlinkDir}, + ) + if code != 0 { + t.Errorf("expected exit 0 for symlinked repo-root with existing doc, got %d; stderr: %q", code, stderr) + } + if !strings.Contains(stdout, "OK") { + t.Errorf("expected 'OK' in stdout, got %q", stdout) + } +} diff --git a/review/docmap.go b/review/docmap.go index c629167..761dabf 100644 --- a/review/docmap.go +++ b/review/docmap.go @@ -311,10 +311,14 @@ func readFileBytes(path string) ([]byte, error) { } // 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. +// VCS API (absolute paths, any ".." segment, backslashes). 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. Backslashes are rejected +// explicitly to prevent Windows platform edge cases. func ValidateDocPath(p string) error { + if strings.Contains(p, "\\") { + return fmt.Errorf("backslashes not allowed in doc paths") + } if filepath.IsAbs(p) { return fmt.Errorf("absolute paths not allowed") } diff --git a/review/docmap_test.go b/review/docmap_test.go index 0ae6778..2674d15 100644 --- a/review/docmap_test.go +++ b/review/docmap_test.go @@ -395,6 +395,10 @@ func TestValidateDocPath(t *testing.T) { "docs/../../../etc/passwd", "../sibling-repo/file.md", "a/b/../c", + // Backslashes must be rejected (Finding #3 — Windows platform edge cases). + `docs\foo.md`, + `docs\..\secret`, + `\absolute`, } for _, p := range invalid { if err := ValidateDocPath(p); err == nil { @@ -420,6 +424,27 @@ func TestLoadMatchingDocs_PathTraversalRejected(t *testing.T) { } } +// TestValidateDocPath_Backslash verifies that backslash-bearing paths are +// rejected to prevent Windows platform edge cases where a path separator +// could be normalised differently by the host OS or VCS backend. +func TestValidateDocPath_Backslash(t *testing.T) { + backslashPaths := []string{ + `docs\foo.md`, + `docs\subdir\file.md`, + `\absolute`, + } + for _, p := range backslashPaths { + if err := ValidateDocPath(p); err == nil { + t.Errorf("expected backslash path %q to be rejected, but it was accepted", p) + } + } + + // Sanity: forward-slash path must still be accepted. + if err := ValidateDocPath("docs/foo.md"); err != nil { + t.Errorf("expected forward-slash path to be accepted, got: %v", err) + } +} + // ============================================================ // Helpers // ============================================================