From 0fedefad3fa2ab6433c63d639a855bcda5988614 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 15 May 2026 14:55:06 +0000 Subject: [PATCH] fix(#150): return resolved path from validateDocmapPath to close TOCTOU gap Finding 4 [MINOR] from self-review: Previously, validateDocmapPath validated *docmapFlag then returned error only, leaving the caller to re-open the original (unresolved) path via ParseDocMapConfig. In theory, the path could change between validation and use (check-then-use race). Change validateDocmapPath to return (string, error): on success it returns the filepath.EvalSymlinks-resolved absolute path. The caller now passes resolvedDocmap to ParseDocMapConfig instead of the original *docmapFlag string, eliminating any check-then-use window. Also update the test for TestValidateDocmapPath_DirSymlinkBypass to use the new two-value return: _ for the resolved path, err for the error. Low-risk in ephemeral CI but correct by construction. --- cmd/review-bot/validatedocmap.go | 26 +++++++++++++++----------- cmd/review-bot/validatedocmap_test.go | 2 +- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/cmd/review-bot/validatedocmap.go b/cmd/review-bot/validatedocmap.go index d8f2547..7ea7bd6 100644 --- a/cmd/review-bot/validatedocmap.go +++ b/cmd/review-bot/validatedocmap.go @@ -31,11 +31,11 @@ const maxDocmapBytes int64 = 10 * 1024 * 1024 // 10 MB // // resolvedRoot must already be an absolute, symlink-free path (obtained from // filepath.Abs + filepath.EvalSymlinks). -func validateDocmapPath(localPath, resolvedRoot string) error { +func validateDocmapPath(localPath, resolvedRoot string) (string, error) { // Resolve the docmap path to an absolute path. absPath, err := filepath.Abs(localPath) if err != nil { - return fmt.Errorf("cannot resolve path: %w", err) + return "", fmt.Errorf("cannot resolve path: %w", err) } // Resolve ALL symlink components, not just the final one. @@ -47,14 +47,14 @@ func validateDocmapPath(localPath, resolvedRoot string) error { // path is inside the root while the actual destination is not. resolvedPath, err := filepath.EvalSymlinks(absPath) if err != nil { - return fmt.Errorf("cannot resolve path (symlink): %w", err) + return "", fmt.Errorf("cannot resolve path (symlink): %w", err) } // Lstat the resolved path — EvalSymlinks guarantees resolvedPath is // symlink-free, so ModeSymlink can never be set here; this is unreachable. fi, err := os.Lstat(resolvedPath) if err != nil { - return fmt.Errorf("cannot stat file: %w", err) + return "", fmt.Errorf("cannot stat file: %w", err) } // Reject anything that is not a regular file (directories, FIFOs, device @@ -68,15 +68,15 @@ func validateDocmapPath(localPath, resolvedRoot string) error { // symlink inside the repo cannot carry the path outside the root. rel, err := filepath.Rel(resolvedRoot, resolvedPath) if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { - return fmt.Errorf("path must be within --repo-root") + return "", fmt.Errorf("path must be within --repo-root") } // Enforce size cap before reading to prevent memory exhaustion. if fi.Size() > maxDocmapBytes { - return fmt.Errorf("file size %d bytes exceeds %d-byte limit", fi.Size(), maxDocmapBytes) + return "", fmt.Errorf("file size %d bytes exceeds %d-byte limit", fi.Size(), maxDocmapBytes) } - return nil + return resolvedPath, nil } // runValidateDocmap implements the `review-bot validate-docmap` subcommand. @@ -144,15 +144,19 @@ func runValidateDocmap(args []string) int { // if they resolve to a path inside the root). // 3. Does not exceed maxDocmapBytes (prevent memory exhaustion from an // oversized committed file). - if err := validateDocmapPath(*docmapFlag, resolvedRoot); err != nil { + // validateDocmapPath returns the resolved path; use it directly to + // eliminate any TOCTOU race between validation and use. + resolvedDocmap, err := validateDocmapPath(*docmapFlag, resolvedRoot) + if err != nil { fmt.Fprintf(errWriter, "Error: --docmap %q is invalid: %v\n", *docmapFlag, err) return 2 } - // Parse docmap YAML. - cfg, err := review.ParseDocMapConfig(*docmapFlag) + // Parse docmap YAML using the resolved path — eliminates any TOCTOU race + // between validation and use. + cfg, err := review.ParseDocMapConfig(resolvedDocmap) if err != nil { - fmt.Fprintf(errWriter, "Error: failed to parse docmap %q: %v\n", *docmapFlag, err) + fmt.Fprintf(errWriter, "Error: failed to parse docmap %q: %v\n", resolvedDocmap, err) return 2 } diff --git a/cmd/review-bot/validatedocmap_test.go b/cmd/review-bot/validatedocmap_test.go index f30a08b..3d4e6d4 100644 --- a/cmd/review-bot/validatedocmap_test.go +++ b/cmd/review-bot/validatedocmap_test.go @@ -595,7 +595,7 @@ func TestValidateDocmapPath_DirSymlinkBypass(t *testing.T) { t.Fatalf("EvalSymlinks(repoDir): %v", err) } - if err := validateDocmapPath(attackPath, resolvedRoot); err == nil { + if _, err := validateDocmapPath(attackPath, resolvedRoot); err == nil { t.Error("expected rejection of dir-symlink bypass, got nil error") } }