Compare commits

..

1 Commits

Author SHA1 Message Date
Rodin 4dce8e4454 fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m3s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m16s
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
- TestValidateDocmapPath_Symlink: updated to test out-of-repo symlink

Coverage: 54.0%
2026-05-15 08:37:31 +00:00
2 changed files with 0 additions and 58 deletions
-2
View File
@@ -9,8 +9,6 @@
### 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
-56
View File
@@ -1507,59 +1507,3 @@ func TestMainSubprocess_DeprecatedGiteaURLEnv(t *testing.T) {
}
}
// 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)
}
}