From 20e98998352a50e17b274defdc95b353c1425a75 Mon Sep 17 00:00:00 2001
From: Rodin <rodin@forgedthought.ai>
Date: Fri, 15 May 2026 14:54:13 +0000
Subject: [PATCH] =?UTF-8?q?docs(#150):=20fix=20stale=20comments=20in=20val?=
 =?UTF-8?q?idateDocmapPath=20=E2=80=94=20reflect=20new=20in-repo-symlink?=
 =?UTF-8?q?=20semantic?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Findings 1-3 from self-review (4dce8e4):

Finding 1 [NIT]: remove dead ModeSymlink check and its misleading
'defense-in-depth' comment. After filepath.EvalSymlinks, resolvedPath
is guaranteed symlink-free; fi.Mode()&os.ModeSymlink can never be set.
Dropped the unreachable branch; updated Lstat comment to say so.

Finding 2 [MINOR]: update validateDocmapPath godoc — invariant #2 now
reads 'The resolved path is within resolvedRoot' instead of 'The path
is not a symlink'. In-repo file-level symlinks whose resolved target
stays within the root are allowed; the confinement check enforces the
actual invariant.

Finding 3 [MINOR]: update inline comment in runValidateDocmap — the
bulleted list item now says 'Resolved target stays within the root
(in-repo symlinks allowed...)' instead of 'Is not a symlink'.
---
 cmd/review-bot/validatedocmap.go | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/cmd/review-bot/validatedocmap.go b/cmd/review-bot/validatedocmap.go
index e29fa25..d8f2547 100644
--- a/cmd/review-bot/validatedocmap.go
+++ b/cmd/review-bot/validatedocmap.go
@@ -23,8 +23,9 @@ const maxDocmapBytes int64 = 10 * 1024 * 1024 // 10 MB
 //  1. The path resolves to a regular file within resolvedRoot (path
 //     confinement): prevents a PR-controlled --docmap from reading arbitrary
 //     host files via absolute paths or ".." traversal.
-//  2. The path is not a symlink: prevents denial-of-service via /dev/zero or
-//     information disclosure via symlinks that point outside the workspace.
+//  2. The resolved path is within resolvedRoot: in-repo file-level symlinks
+//     are allowed when their resolved target is still inside the root;
+//     symlinks that escape the root are rejected by the confinement check.
 //  3. The file does not exceed maxDocmapBytes: prevents memory exhaustion
 //     from an oversized but legitimately committed doc-map file.
 //
@@ -49,18 +50,13 @@ func validateDocmapPath(localPath, resolvedRoot string) error {
 		return fmt.Errorf("cannot resolve path (symlink): %w", err)
 	}
 
-	// Lstat the resolved path — at this point resolvedPath is symlink-free, so
-	// ModeSymlink will never be set. We keep the check as defense-in-depth.
+	// 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)
 	}
 
-	// Defense-in-depth: reject any remaining symlink indicator.
-	if fi.Mode()&os.ModeSymlink != 0 {
-		return fmt.Errorf("symlinks are not allowed")
-	}
-
 	// Reject anything that is not a regular file (directories, FIFOs, device
 	// nodes, etc.) — ParseDocMapConfig expects a plain YAML file and would
 	// produce a confusing error on non-regular entries.
@@ -144,7 +140,8 @@ func runValidateDocmap(args []string) int {
 	// may reference a PR-controlled file (e.g. .review-bot/doc-map.yml).
 	// Validate that it:
 	//   1. Resolves within resolvedRoot (prevent reading arbitrary host files).
-	//   2. Is not a symlink (prevent /dev/zero or symlink-based host probing).
+	//   2. Resolved target stays within the root (in-repo symlinks are allowed
+	//      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 {