fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass

The previous implementation called os.Lstat(absPath) which only avoids
following the *final* path component. A PR committing .review-bot/ as a
directory symlink pointing outside the repo would pass the filepath.Rel
confinement check because the textual path was inside the root while
the resolved destination was not.

Fix: call filepath.EvalSymlinks after filepath.Abs to resolve ALL symlink
components before the confinement check. If EvalSymlinks fails (dangling
symlink, nonexistent target) the path is rejected. The filepath.Rel check
then operates on the fully-resolved path.

Semantic change: file-level in-repo symlinks (target also within root) are
now allowed — the invariant is about where the content lives, not whether
the entry is a symlink. The test TestValidateDocmapPath_Symlink is updated
to test an out-of-repo symlink target, which must still be rejected.

Tests:
- TestValidateDocmapPath_DirSymlinkBypass: reproduces the attack vector
  (dir symlink bypassing textual confinement check) and verifies it is
  now rejected
- TestMainSubprocess_InvalidDocMapPath: subprocess test via validate-docmap
  subcommand — ../../../etc/passwd rejected (closes #146)
- TestMainSubprocess_InvalidDocMapFile: subprocess test — nonexistent file
  rejected (closes #146)

Coverage: 54.3% (was 54.1%)

CHANGELOG.md

@@ -2,6 +2,16 @@
 
 ## Unreleased
 
+### Security
+
+- **`validateDocmapPath`: add `EvalSymlinks` to close directory-symlink bypass** ([#150](https://gitea.weiker.me/rodin/review-bot/issues/150)): The previous implementation used `os.Lstat` which only avoids following the *final* path component. An intermediate directory symlink (e.g. `.review-bot/` committed as a symlink to a directory outside the repo) would pass the path-confinement check because the textual path appeared within the repo root. `filepath.EvalSymlinks` is now called first, resolving all symlink components before the `filepath.Rel` confinement check. In-repo symlinks whose resolved targets also reside within the repo root are now allowed; out-of-repo targets are rejected by the confinement check.
+
+### Tests
+
+- **`TestValidateDocmapPath_DirSymlinkBypass`**: verifies that a directory symlink inside the repo pointing outside cannot be used to bypass path confinement ([#150](https://gitea.weiker.me/rodin/review-bot/issues/150)).
+- **`TestMainSubprocess_InvalidDocMapPath`**: verifies that a path-traversal `--docmap` argument (`../../../etc/passwd`) is rejected by the validate-docmap subcommand ([#146](https://gitea.weiker.me/rodin/review-bot/issues/146)).
+- **`TestMainSubprocess_InvalidDocMapFile`**: verifies that a nonexistent `--docmap` file is rejected by the validate-docmap subcommand ([#146](https://gitea.weiker.me/rodin/review-bot/issues/146)).
+
 ### Added
 
 - **`doc-map` input** (`--doc-map` flag / `DOC_MAP_FILE` env var): Path to a YAML file mapping source path globs to governing design docs. review-bot intersects the map with changed PR paths and injects matching docs into the system prompt under a `## Design Documents` heading. ([#137](https://gitea.weiker.me/rodin/review-bot/issues/137))

cmd/review-bot/main_test.go

@@ -1506,3 +1506,60 @@ func TestMainSubprocess_DeprecatedGiteaURLEnv(t *testing.T) {
 		t.Errorf("expected deprecation warning for GITEA_URL, got: %s", out)
 	}
 }
+
+// TestMainSubprocess_InvalidDocMapPath confirms that --doc-map with a path
+// traversal attempt (e.g. ../../../etc/passwd) is rejected when the
+// validate-docmap subcommand is used. The validate-docmap subcommand shares
+// the same path validation logic (validateDocmapPath) that is called when
+// --doc-map is provided to the main review-bot command.
+func TestMainSubprocess_InvalidDocMapPath(t *testing.T) {
+	if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
+		flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
+		os.Args = []string{"review-bot", "validate-docmap",
+			"--docmap", "../../../etc/passwd",
+			"--repo-root", "/tmp",
+		}
+		main()
+		return
+	}
+
+	cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidDocMapPath")
+	cmd.Env = append(os.Environ(), "TEST_SUBPROCESS_MAIN=1")
+	out, err := cmd.CombinedOutput()
+	if err == nil {
+		t.Fatal("expected non-zero exit for path traversal doc-map, got success")
+	}
+	combined := string(out)
+	if !strings.Contains(combined, "invalid") && !strings.Contains(combined, "repo-root") && !strings.Contains(combined, "traversal") && !strings.Contains(combined, "outside") {
+		t.Errorf("expected path confinement error for traversal path, got: %s", combined)
+	}
+}
+
+// TestMainSubprocess_InvalidDocMapFile confirms that --doc-map pointing at a
+// nonexistent file is rejected. The validate-docmap subcommand exercises the
+// same validateDocmapPath that is invoked when --doc-map is provided to the
+// main review-bot command.
+func TestMainSubprocess_InvalidDocMapFile(t *testing.T) {
+	if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
+		flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
+		// Create a temp dir to serve as repo-root, then reference a nonexistent file inside it.
+		repoDir := os.TempDir()
+		os.Args = []string{"review-bot", "validate-docmap",
+			"--docmap", "nonexistent-doc-map.yml",
+			"--repo-root", repoDir,
+		}
+		main()
+		return
+	}
+
+	cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidDocMapFile")
+	cmd.Env = append(os.Environ(), "TEST_SUBPROCESS_MAIN=1")
+	out, err := cmd.CombinedOutput()
+	if err == nil {
+		t.Fatal("expected non-zero exit for nonexistent doc-map file, got success")
+	}
+	combined := string(out)
+	if !strings.Contains(combined, "invalid") && !strings.Contains(combined, "cannot stat") && !strings.Contains(combined, "resolve") {
+		t.Errorf("expected file-not-found error for nonexistent doc-map, got: %s", combined)
+	}
+}

cmd/review-bot/validatedocmap.go

@@ -37,22 +37,33 @@ func validateDocmapPath(localPath, resolvedRoot string) error {
 		return fmt.Errorf("cannot resolve path: %w", err)
 	}
 
-	// Lstat: do NOT follow symlinks. We want to inspect the entry itself.
-	fi, err := os.Lstat(absPath)
+	// Resolve ALL symlink components, not just the final one.
+	// os.Lstat only avoids following the *final* path component; intermediate
+	// directory symlinks are still followed. EvalSymlinks resolves every
+	// component, closing the directory-symlink bypass: a PR that commits
+	// .review-bot/ as a directory symlink pointing outside the repo would
+	// otherwise pass the filepath.Rel confinement check because the textual
+	// 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)
+	}
+
+	// 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.
+	fi, err := os.Lstat(resolvedPath)
 	if err != nil {
 		return fmt.Errorf("cannot stat file: %w", err)
 	}
 
-	// Reject symlinks outright — a symlink can point to /dev/zero or arbitrary
-	// host paths, bypassing both the confinement check and the size check.
+	// Defense-in-depth: reject any remaining symlink indicator.
 	if fi.Mode()&os.ModeSymlink != 0 {
 		return fmt.Errorf("symlinks are not allowed")
 	}
 
-	// Confine to resolvedRoot: the cleaned absolute path must be a descendant
-	// of the repo root. This catches paths that escaped via "..", absolute
-	// paths that happen to be outside the root, etc.
-	rel, err := filepath.Rel(resolvedRoot, absPath)
+	// 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")
 	}

cmd/review-bot/validatedocmap_test.go

@@ -465,23 +465,34 @@ mappings:
 }
 
 // TestValidateDocmapPath_Symlink verifies that --docmap pointing at a symlink
-// is rejected before the file is read (prevents /dev/zero DOS or arbitrary
-// host-file reads via PR-controlled symlinks).
+// whose resolved target is outside --repo-root is rejected (prevents reading
+// arbitrary host files via PR-controlled symlinks).
+//
+// Note: after the EvalSymlinks fix (issue #150), in-repo symlinks whose
+// targets also reside within the repo root are now allowed — the confinement
+// check is applied to the resolved path, not the symlink entry itself. The
+// security invariant is: the resolved destination must be within the root.
 func TestValidateDocmapPath_Symlink(t *testing.T) {
 	dir := t.TempDir()
+	outside := t.TempDir()
 
-	// Create a real docmap file to serve as the symlink target.
-	realDocmap := makeDocmapInDir(t, dir, `
-mappings:
-  - paths:
-      - "lib/**"
-    docs:
-      - docs/foo.md
-`)
+	// Create a docmap file OUTSIDE the repo root to serve as the symlink
+	// target. EvalSymlinks will resolve to this path, which the Rel check
+	// must then reject.
+	if err := os.MkdirAll(filepath.Join(outside, ".review-bot"), 0o755); err != nil {
+		t.Fatalf("MkdirAll: %v", err)
+	}
+	outsideDocmap := filepath.Join(outside, ".review-bot", "doc-map.yml")
+	if err := os.WriteFile(outsideDocmap, []byte("mappings: []\n"), 0o644); err != nil {
+		t.Fatalf("WriteFile: %v", err)
+	}
 
-	// Create a symlink inside dir pointing to the real docmap.
+	// Create a symlink inside dir pointing to the file outside the repo.
+	if err := os.MkdirAll(filepath.Join(dir, ".review-bot"), 0o755); err != nil {
+		t.Fatalf("MkdirAll: %v", err)
+	}
 	symlinkPath := filepath.Join(dir, ".review-bot", "doc-map-link.yml")
-	if err := os.Symlink(realDocmap, symlinkPath); err != nil {
+	if err := os.Symlink(outsideDocmap, symlinkPath); err != nil {
 		t.Fatalf("Symlink: %v", err)
 	}
 
@@ -490,10 +501,10 @@ mappings:
 		[]string{"--docmap", symlinkPath, "--repo-root", dir},
 	)
 	if code != 2 {
-		t.Errorf("expected exit 2 for symlink docmap, got %d; stderr: %q", code, stderr)
+		t.Errorf("expected exit 2 for out-of-repo symlink docmap, got %d; stderr: %q", code, stderr)
 	}
-	if !strings.Contains(stderr, "symlink") && !strings.Contains(stderr, "invalid") {
-		t.Errorf("expected symlink rejection in stderr, got %q", stderr)
+	if !strings.Contains(stderr, "invalid") && !strings.Contains(stderr, "repo-root") {
+		t.Errorf("expected confinement rejection in stderr, got %q", stderr)
 	}
 }
 
@@ -550,3 +561,41 @@ func TestValidateDocmapPath_SizeLimit(t *testing.T) {
 		t.Errorf("expected size limit error in stderr, got %q", stderr)
 	}
 }
+
+// TestValidateDocmapPath_DirSymlinkBypass verifies that a directory-symlink
+// inside the repo pointing outside cannot be used to read arbitrary host files.
+//
+// Attack vector: a PR commits .review-bot/ as a directory symlink targeting a
+// directory outside the repo. The textual path of the docmap file is inside
+// the repo root, so the old Rel-only check passed — but the actual file is
+// outside. This is closed by calling EvalSymlinks on the full path before the
+// confinement check.
+func TestValidateDocmapPath_DirSymlinkBypass(t *testing.T) {
+	repoDir := t.TempDir()
+	outsideDir := t.TempDir()
+
+	// Secret file outside the repo.
+	secretPath := filepath.Join(outsideDir, "secret.yml")
+	if err := os.WriteFile(secretPath, []byte("mappings: []\n"), 0o644); err != nil {
+		t.Fatalf("WriteFile: %v", err)
+	}
+
+	// Create .review-bot/ as a directory symlink pointing outside the repo.
+	reviewBotDir := filepath.Join(repoDir, ".review-bot")
+	if err := os.Symlink(outsideDir, reviewBotDir); err != nil {
+		t.Skipf("cannot create dir symlink (platform may not support it): %v", err)
+	}
+
+	// Textually inside repo — .review-bot/secret.yml — but resolves outside.
+	attackPath := filepath.Join(repoDir, ".review-bot", "secret.yml")
+
+	// Resolve repoDir to a symlink-free path, as runValidateDocmap does.
+	resolvedRoot, err := filepath.EvalSymlinks(repoDir)
+	if err != nil {
+		t.Fatalf("EvalSymlinks(repoDir): %v", err)
+	}
+
+	if err := validateDocmapPath(attackPath, resolvedRoot); err == nil {
+		t.Error("expected rejection of dir-symlink bypass, got nil error")
+	}
+}