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

@@ -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,29 +47,29 @@ 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)
 	}
 
 	// Confine to resolvedRoot: use the fully-resolved path so that a directory
 	// 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.
@@ -137,15 +137,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.
+	// Parse docmap YAML using the resolved path — eliminates any TOCTOU race
-	cfg, err := review.ParseDocMapConfig(*docmapFlag)
+	// 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
 	}
 

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")
 	}
 }