diff --git a/CHANGELOG.md b/CHANGELOG.md index ef78201..23396e1 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go index 22b7a6c..86acd82 100644 --- a/cmd/review-bot/main_test.go +++ b/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) + } +} diff --git a/cmd/review-bot/validatedocmap.go b/cmd/review-bot/validatedocmap.go index 6b86b07..3db4c76 100644 --- a/cmd/review-bot/validatedocmap.go +++ b/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") } diff --git a/cmd/review-bot/validatedocmap_test.go b/cmd/review-bot/validatedocmap_test.go index edd131f..e572ecc 100644 --- a/cmd/review-bot/validatedocmap_test.go +++ b/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") + } +}