From c76e7dcd2ee7393fb169752b3304baebbbc292cd Mon Sep 17 00:00:00 2001
From: Rodin <rodin@forgedthought.ai>
Date: Fri, 15 May 2026 16:23:07 -0700
Subject: [PATCH] =?UTF-8?q?fix(#150):=20add=20os.SameFile=20check=20after?=
 =?UTF-8?q?=20open=20to=20close=20Lstat=E2=86=92open=20TOCTOU=20window?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

After getting the resolved path from validateDocmapPath, Lstat the path
immediately before os.Open, then compare with f.Stat() after open using
os.SameFile. If the file was swapped between validation and open (e.g.,
replaced with a symlink pointing outside the repo), the inode comparison
catches it and returns an error.

Also changes defer f.Close() // nolint:errcheck to
defer func() { _ = f.Close() }() to follow the project convention of
explicit ignores over suppressor comments.

Addresses security bot finding (review 4812) against d6bab7a9.
---
 cmd/review-bot/validatedocmap.go | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/cmd/review-bot/validatedocmap.go b/cmd/review-bot/validatedocmap.go
index 2c1f364..8fc19bb 100644
--- a/cmd/review-bot/validatedocmap.go
+++ b/cmd/review-bot/validatedocmap.go
@@ -156,12 +156,33 @@ func runValidateDocmap(args []string) int {
 	// window between the Lstat size check in validateDocmapPath and the file open
 	// here. The limit is maxDocmapBytes+1 so we can detect a file that grew past
 	// the cap after the stat without reading unbounded bytes.
+	//
+	// Defense-in-depth: stat the path immediately before and after open so we can
+	// detect a file swap between validateDocmapPath's validation and this open via
+	// os.SameFile. An attacker with workspace write access could otherwise replace
+	// the validated file with a symlink in the gap between validation and use.
+	preStat, err := os.Lstat(resolvedDocmap)
+	if err != nil {
+		fmt.Fprintf(errWriter, "Error: failed to stat docmap before open %q: %v\n", *docmapFlag, err)
+		return 2
+	}
 	f, err := os.Open(resolvedDocmap)
 	if err != nil {
 		fmt.Fprintf(errWriter, "Error: failed to open docmap %q: %v\n", *docmapFlag, err)
 		return 2
 	}
-	defer f.Close() // nolint:errcheck
+	defer func() { _ = f.Close() }()
+	// Verify we opened the same file that was validated — rejects a swap between
+	// the pre-open Lstat and the open call.
+	postStat, err := f.Stat()
+	if err != nil {
+		fmt.Fprintf(errWriter, "Error: failed to stat open docmap %q: %v\n", *docmapFlag, err)
+		return 2
+	}
+	if !os.SameFile(preStat, postStat) {
+		fmt.Fprintf(errWriter, "Error: --docmap %q changed between validation and open\n", *docmapFlag)
+		return 2
+	}
 	docmapData, err := io.ReadAll(io.LimitReader(f, maxDocmapBytes+1))
 	if err != nil {
 		fmt.Fprintf(errWriter, "Error: failed to read docmap %q: %v\n", *docmapFlag, err)