diff --git a/cmd/review-bot/validatedocmap.go b/cmd/review-bot/validatedocmap.go index e29fa25..8fc19bb 100644 --- a/cmd/review-bot/validatedocmap.go +++ b/cmd/review-bot/validatedocmap.go @@ -23,18 +23,19 @@ 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. // // 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. @@ -46,41 +47,36 @@ 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 — 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 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) - } - - // Defense-in-depth: reject any remaining symlink indicator. - if fi.Mode()&os.ModeSymlink != 0 { - return fmt.Errorf("symlinks are not allowed") + return "", fmt.Errorf("cannot stat file: %w", err) } // 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. 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 // 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,16 +140,59 @@ 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 { + // 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) + // 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. + // + // 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 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) + 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 diff --git a/cmd/review-bot/validatedocmap_test.go b/cmd/review-bot/validatedocmap_test.go index f30a08b..62a204e 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") } } @@ -649,3 +649,48 @@ 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). + // 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") + } +}