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

CHANGELOG.md

@@ -2,6 +2,14 @@
 
 ## 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)).
+
 ### 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/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.
+	// Resolve ALL symlink components, not just the final one.
-	fi, err := os.Lstat(absPath)
+	// 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
+	// Defense-in-depth: reject any remaining symlink indicator.
-	// host paths, bypassing both the confinement check and the size check.
 	if fi.Mode()&os.ModeSymlink != 0 {
 		return fmt.Errorf("symlinks are not allowed")
 	}
 
-	// Confine to resolvedRoot: the cleaned absolute path must be a descendant
+	// Confine to resolvedRoot: use the fully-resolved path so that a directory
-	// of the repo root. This catches paths that escaped via "..", absolute
+	// symlink inside the repo cannot carry the path outside the root.
-	// paths that happen to be outside the root, etc.
+	rel, err := filepath.Rel(resolvedRoot, resolvedPath)
-	rel, err := filepath.Rel(resolvedRoot, absPath)
 	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
+// whose resolved target is outside --repo-root is rejected (prevents reading
-// host-file reads via PR-controlled symlinks).
+// 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.
+	// Create a docmap file OUTSIDE the repo root to serve as the symlink
-	realDocmap := makeDocmapInDir(t, dir, `
+	// target. EvalSymlinks will resolve to this path, which the Rel check
-mappings:
+	// must then reject.
-  - paths:
+	if err := os.MkdirAll(filepath.Join(outside, ".review-bot"), 0o755); err != nil {
-      - "lib/**"
+		t.Fatalf("MkdirAll: %v", err)
-    docs:
+	}
-      - docs/foo.md
+	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") {
+	if !strings.Contains(stderr, "invalid") && !strings.Contains(stderr, "repo-root") {
-		t.Errorf("expected symlink rejection in stderr, got %q", stderr)
+		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")
+	}
+}