From 20e98998352a50e17b274defdc95b353c1425a75 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 15 May 2026 14:54:13 +0000 Subject: [PATCH 1/8] =?UTF-8?q?docs(#150):=20fix=20stale=20comments=20in?= =?UTF-8?q?=20validateDocmapPath=20=E2=80=94=20reflect=20new=20in-repo-sym?= =?UTF-8?q?link=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 { From 0fedefad3fa2ab6433c63d639a855bcda5988614 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 15 May 2026 14:55:06 +0000 Subject: [PATCH 2/8] fix(#150): return resolved path from validateDocmapPath to close TOCTOU gap Finding 4 [MINOR] from self-review: Previously, validateDocmapPath validated *docmapFlag then returned error only, leaving the caller to re-open the original (unresolved) path via ParseDocMapConfig. In theory, the path could change between validation and use (check-then-use race). Change validateDocmapPath to return (string, error): on success it returns the filepath.EvalSymlinks-resolved absolute path. The caller now passes resolvedDocmap to ParseDocMapConfig instead of the original *docmapFlag string, eliminating any check-then-use window. Also update the test for TestValidateDocmapPath_DirSymlinkBypass to use the new two-value return: _ for the resolved path, err for the error. Low-risk in ephemeral CI but correct by construction. --- cmd/review-bot/validatedocmap.go | 26 +++++++++++++++----------- cmd/review-bot/validatedocmap_test.go | 2 +- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/cmd/review-bot/validatedocmap.go b/cmd/review-bot/validatedocmap.go index d8f2547..7ea7bd6 100644 --- a/cmd/review-bot/validatedocmap.go +++ b/cmd/review-bot/validatedocmap.go @@ -31,11 +31,11 @@ const maxDocmapBytes int64 = 10 * 1024 * 1024 // 10 MB // // resolvedRoot must already be an absolute, symlink-free path (obtained from // filepath.Abs + filepath.EvalSymlinks). -func validateDocmapPath(localPath, resolvedRoot string) error { +func validateDocmapPath(localPath, resolvedRoot string) (string, error) { // Resolve the docmap path to an absolute path. absPath, err := filepath.Abs(localPath) if err != nil { - return fmt.Errorf("cannot resolve path: %w", err) + return "", fmt.Errorf("cannot resolve path: %w", err) } // Resolve ALL symlink components, not just the final one. @@ -47,14 +47,14 @@ func validateDocmapPath(localPath, resolvedRoot string) error { // 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) + return "", fmt.Errorf("cannot resolve path (symlink): %w", err) } // 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) + return "", fmt.Errorf("cannot stat file: %w", err) } // Reject anything that is not a regular file (directories, FIFOs, device @@ -68,15 +68,15 @@ func validateDocmapPath(localPath, resolvedRoot string) error { // 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") + return "", fmt.Errorf("path must be within --repo-root") } // Enforce size cap before reading to prevent memory exhaustion. if fi.Size() > maxDocmapBytes { - return fmt.Errorf("file size %d bytes exceeds %d-byte limit", fi.Size(), maxDocmapBytes) + return "", fmt.Errorf("file size %d bytes exceeds %d-byte limit", fi.Size(), maxDocmapBytes) } - return nil + return resolvedPath, nil } // runValidateDocmap implements the `review-bot validate-docmap` subcommand. @@ -144,15 +144,19 @@ func runValidateDocmap(args []string) int { // 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 { + // validateDocmapPath returns the resolved path; use it directly to + // eliminate any TOCTOU race between validation and use. + resolvedDocmap, err := validateDocmapPath(*docmapFlag, resolvedRoot) + if err != nil { fmt.Fprintf(errWriter, "Error: --docmap %q is invalid: %v\n", *docmapFlag, err) return 2 } - // Parse docmap YAML. - cfg, err := review.ParseDocMapConfig(*docmapFlag) + // Parse docmap YAML using the resolved path — eliminates any TOCTOU race + // between validation and use. + cfg, err := review.ParseDocMapConfig(resolvedDocmap) if err != nil { - fmt.Fprintf(errWriter, "Error: failed to parse docmap %q: %v\n", *docmapFlag, err) + fmt.Fprintf(errWriter, "Error: failed to parse docmap %q: %v\n", resolvedDocmap, err) return 2 } diff --git a/cmd/review-bot/validatedocmap_test.go b/cmd/review-bot/validatedocmap_test.go index f30a08b..3d4e6d4 100644 --- a/cmd/review-bot/validatedocmap_test.go +++ b/cmd/review-bot/validatedocmap_test.go @@ -595,7 +595,7 @@ func TestValidateDocmapPath_DirSymlinkBypass(t *testing.T) { t.Fatalf("EvalSymlinks(repoDir): %v", err) } - if err := validateDocmapPath(attackPath, resolvedRoot); err == nil { + if _, err := validateDocmapPath(attackPath, resolvedRoot); err == nil { t.Error("expected rejection of dir-symlink bypass, got nil error") } } From 345f9a5aac059e05378d3f0a48c6e2a9258ce249 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 15 May 2026 14:55:30 +0000 Subject: [PATCH 3/8] test(#150): add positive test for in-repo symlink allowed by EvalSymlinks fix Finding 5 [NIT] from self-review: TestValidateDocmapPath_InRepoSymlinkAllowed verifies that a file-level symlink inside the repo root whose resolved target is also within the root is accepted by validateDocmapPath. This is the positive case for the issue #150 behavioral change (commit 4dce8e4): only symlinks whose resolved destination escapes the root are rejected. Intra-repo symlinks are permitted and their resolved path is returned to the caller. The test also asserts that the returned path is the resolved real file, not the symlink entry itself (i.e., EvalSymlinks did its job). --- cmd/review-bot/validatedocmap.go | 2 +- cmd/review-bot/validatedocmap_test.go | 42 +++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/cmd/review-bot/validatedocmap.go b/cmd/review-bot/validatedocmap.go index 7ea7bd6..af0abc2 100644 --- a/cmd/review-bot/validatedocmap.go +++ b/cmd/review-bot/validatedocmap.go @@ -61,7 +61,7 @@ func validateDocmapPath(localPath, resolvedRoot string) (string, error) { // nodes, etc.) — ParseDocMapConfig expects a plain YAML file and would // produce a confusing error on non-regular entries. if !fi.Mode().IsRegular() { - return fmt.Errorf("docmap must be a regular file") + return "", fmt.Errorf("docmap must be a regular file") } // Confine to resolvedRoot: use the fully-resolved path so that a directory diff --git a/cmd/review-bot/validatedocmap_test.go b/cmd/review-bot/validatedocmap_test.go index 3d4e6d4..1377b47 100644 --- a/cmd/review-bot/validatedocmap_test.go +++ b/cmd/review-bot/validatedocmap_test.go @@ -649,3 +649,45 @@ mappings: t.Errorf("expected exit 0 for './' prefixed covered file, got %d; stderr: %q", code, stderr) } } + +// TestValidateDocmapPath_InRepoSymlinkAllowed verifies that an in-repo +// file-level symlink whose resolved target is still within the repo root is +// accepted. This is the positive case for the issue #150 behavioral change: +// only symlinks that escape the root are rejected; intra-repo symlinks are +// allowed because EvalSymlinks resolves the target and the confinement check +// is applied to the resolved path, not the symlink entry itself. +func TestValidateDocmapPath_InRepoSymlinkAllowed(t *testing.T) { + dir := t.TempDir() + + // Create the real docmap file inside the repo root. + if err := os.MkdirAll(filepath.Join(dir, ".review-bot"), 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + realDocmap := filepath.Join(dir, ".review-bot", "doc-map-real.yml") + if err := os.WriteFile(realDocmap, []byte("mappings: []\n"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + // Create a symlink inside the repo root that points to the real file + // (also inside the root). + symlinkPath := filepath.Join(dir, ".review-bot", "doc-map-link.yml") + if err := os.Symlink(realDocmap, symlinkPath); err != nil { + t.Skipf("cannot create symlink (platform may not support it): %v", err) + } + + // Resolve dir to a symlink-free root, as runValidateDocmap does. + resolvedRoot, err := filepath.EvalSymlinks(dir) + if err != nil { + t.Fatalf("EvalSymlinks(dir): %v", err) + } + + // In-repo symlink whose target is within root: must be accepted. + resolved, err := validateDocmapPath(symlinkPath, resolvedRoot) + if err != nil { + t.Fatalf("expected in-repo symlink to be accepted, got error: %v", err) + } + // The returned resolved path must be the real file (not the symlink entry). + if resolved == symlinkPath { + t.Errorf("expected resolved path to differ from symlink path") + } +} From 6e11107c77a2cba7f7f331e87aa2ca8ed112249b Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 15 May 2026 16:10:27 -0700 Subject: [PATCH 4/8] nit(#150): fix misleading 'this is unreachable' in Lstat comment --- cmd/review-bot/validatedocmap.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/review-bot/validatedocmap.go b/cmd/review-bot/validatedocmap.go index af0abc2..31086ac 100644 --- a/cmd/review-bot/validatedocmap.go +++ b/cmd/review-bot/validatedocmap.go @@ -50,8 +50,8 @@ func validateDocmapPath(localPath, resolvedRoot string) (string, error) { return "", fmt.Errorf("cannot resolve path (symlink): %w", err) } - // Lstat the resolved path — EvalSymlinks guarantees resolvedPath is - // symlink-free, so ModeSymlink can never be set here; this is unreachable. + // Lstat the resolved path for size and existence checks — EvalSymlinks + // guarantees no symlink components remain, so ModeSymlink can never be set. fi, err := os.Lstat(resolvedPath) if err != nil { return "", fmt.Errorf("cannot stat file: %w", err) From 4359518e509c3f54e93f188a0c684adda2430325 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 15 May 2026 16:10:42 -0700 Subject: [PATCH 5/8] nit(#150): report original --docmap flag value in parse error, not resolved path --- cmd/review-bot/validatedocmap.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/review-bot/validatedocmap.go b/cmd/review-bot/validatedocmap.go index 31086ac..5db91ff 100644 --- a/cmd/review-bot/validatedocmap.go +++ b/cmd/review-bot/validatedocmap.go @@ -156,7 +156,7 @@ func runValidateDocmap(args []string) int { // between validation and use. cfg, err := review.ParseDocMapConfig(resolvedDocmap) if err != nil { - fmt.Fprintf(errWriter, "Error: failed to parse docmap %q: %v\n", resolvedDocmap, err) + fmt.Fprintf(errWriter, "Error: failed to parse docmap %q: %v\n", *docmapFlag, err) return 2 } From d6bab7a9cf6ce3d084d8f40a91a51c1a8fc084e7 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 15 May 2026 16:11:15 -0700 Subject: [PATCH 6/8] fix(#150): close residual TOCTOU with LimitedReader at docmap open --- cmd/review-bot/validatedocmap.go | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/cmd/review-bot/validatedocmap.go b/cmd/review-bot/validatedocmap.go index 5db91ff..2c1f364 100644 --- a/cmd/review-bot/validatedocmap.go +++ b/cmd/review-bot/validatedocmap.go @@ -152,9 +152,26 @@ func runValidateDocmap(args []string) int { return 2 } - // Parse docmap YAML using the resolved path — eliminates any TOCTOU race - // between validation and use. - cfg, err := review.ParseDocMapConfig(resolvedDocmap) + // Open and read the docmap with a LimitedReader — closes the residual TOCTOU + // window between the Lstat size check in validateDocmapPath and the file open + // here. The limit is maxDocmapBytes+1 so we can detect a file that grew past + // the cap after the stat without reading unbounded bytes. + f, err := os.Open(resolvedDocmap) + if err != nil { + fmt.Fprintf(errWriter, "Error: failed to open docmap %q: %v\n", *docmapFlag, err) + return 2 + } + defer f.Close() // nolint:errcheck + docmapData, err := io.ReadAll(io.LimitReader(f, maxDocmapBytes+1)) + if err != nil { + fmt.Fprintf(errWriter, "Error: failed to read docmap %q: %v\n", *docmapFlag, err) + return 2 + } + if int64(len(docmapData)) > maxDocmapBytes { + fmt.Fprintf(errWriter, "Error: --docmap %q exceeded %d-byte limit after open\n", *docmapFlag, maxDocmapBytes) + return 2 + } + cfg, err := review.ParseDocMapConfigContent(string(docmapData), *docmapFlag) if err != nil { fmt.Fprintf(errWriter, "Error: failed to parse docmap %q: %v\n", *docmapFlag, err) return 2 From c76e7dcd2ee7393fb169752b3304baebbbc292cd Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 15 May 2026 16:23:07 -0700 Subject: [PATCH 7/8] =?UTF-8?q?fix(#150):=20add=20os.SameFile=20check=20af?= =?UTF-8?q?ter=20open=20to=20close=20Lstat=E2=86=92open=20TOCTOU=20window?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After getting the resolved path from validateDocmapPath, Lstat the path immediately before os.Open, then compare with f.Stat() after open using os.SameFile. If the file was swapped between validation and open (e.g., replaced with a symlink pointing outside the repo), the inode comparison catches it and returns an error. Also changes defer f.Close() // nolint:errcheck to defer func() { _ = f.Close() }() to follow the project convention of explicit ignores over suppressor comments. Addresses security bot finding (review 4812) against d6bab7a9. --- cmd/review-bot/validatedocmap.go | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/cmd/review-bot/validatedocmap.go b/cmd/review-bot/validatedocmap.go index 2c1f364..8fc19bb 100644 --- a/cmd/review-bot/validatedocmap.go +++ b/cmd/review-bot/validatedocmap.go @@ -156,12 +156,33 @@ func runValidateDocmap(args []string) int { // window between the Lstat size check in validateDocmapPath and the file open // here. The limit is maxDocmapBytes+1 so we can detect a file that grew past // the cap after the stat without reading unbounded bytes. + // + // Defense-in-depth: stat the path immediately before and after open so we can + // detect a file swap between validateDocmapPath's validation and this open via + // os.SameFile. An attacker with workspace write access could otherwise replace + // the validated file with a symlink in the gap between validation and use. + preStat, err := os.Lstat(resolvedDocmap) + if err != nil { + fmt.Fprintf(errWriter, "Error: failed to stat docmap before open %q: %v\n", *docmapFlag, err) + return 2 + } f, err := os.Open(resolvedDocmap) if err != nil { fmt.Fprintf(errWriter, "Error: failed to open docmap %q: %v\n", *docmapFlag, err) return 2 } - defer f.Close() // nolint:errcheck + defer func() { _ = f.Close() }() + // Verify we opened the same file that was validated — rejects a swap between + // the pre-open Lstat and the open call. + postStat, err := f.Stat() + if err != nil { + fmt.Fprintf(errWriter, "Error: failed to stat open docmap %q: %v\n", *docmapFlag, err) + return 2 + } + if !os.SameFile(preStat, postStat) { + fmt.Fprintf(errWriter, "Error: --docmap %q changed between validation and open\n", *docmapFlag) + return 2 + } docmapData, err := io.ReadAll(io.LimitReader(f, maxDocmapBytes+1)) if err != nil { fmt.Fprintf(errWriter, "Error: failed to read docmap %q: %v\n", *docmapFlag, err) From eb0ff3aa69f152dd995de91c88227d3e32ac2917 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 15 May 2026 16:23:11 -0700 Subject: [PATCH 8/8] nit(#150): clarify why resolved != symlinkPath in InRepoSymlinkAllowed test Add a comment explaining that validateDocmapPath calls EvalSymlinks internally, so the returned path is always the fully-resolved real path and can never equal the symlink entry itself. Addresses sonnet bot NIT (review 4810) against d6bab7a9. --- cmd/review-bot/validatedocmap_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cmd/review-bot/validatedocmap_test.go b/cmd/review-bot/validatedocmap_test.go index 1377b47..62a204e 100644 --- a/cmd/review-bot/validatedocmap_test.go +++ b/cmd/review-bot/validatedocmap_test.go @@ -687,6 +687,9 @@ func TestValidateDocmapPath_InRepoSymlinkAllowed(t *testing.T) { t.Fatalf("expected in-repo symlink to be accepted, got error: %v", err) } // The returned resolved path must be the real file (not the symlink entry). + // validateDocmapPath calls filepath.EvalSymlinks internally, so the returned + // path is always the fully-resolved real path — it can never equal the + // symlink entry itself. if resolved == symlinkPath { t.Errorf("expected resolved path to differ from symlink path") }