Merge pull request 'fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass' (#152) from issue-150 into main

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

cmd/review-bot/main_test.go

@@ -903,12 +903,17 @@ func TestMainSubprocess_InvalidRepo(t *testing.T) {
 		flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
 		args := baseSubprocessArgs()
 		// Replace the canonical --repo value with an invalid one.
+		found := false
 		for i, a := range args {
 			if a == "--repo" && i+1 < len(args) {
 				args[i+1] = "invalidrepo"
+				found = true
 				break
 			}
 		}
+		if !found {
+			t.Fatal("baseSubprocessArgs() does not contain --repo; test is broken")
+		}
 		os.Args = args
 		main()
 		return
@@ -930,12 +935,17 @@ func TestMainSubprocess_InvalidPRNumber(t *testing.T) {
 		flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
 		args := baseSubprocessArgs()
 		// Replace the canonical --pr value with a non-numeric string.
+		found := false
 		for i, a := range args {
 			if a == "--pr" && i+1 < len(args) {
 				args[i+1] = "notanumber"
+				found = true
 				break
 			}
 		}
+		if !found {
+			t.Fatal("baseSubprocessArgs() does not contain --pr; test is broken")
+		}
 		os.Args = args
 		main()
 		return

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
+//  2. The resolved path is within resolvedRoot: in-repo file-level symlinks
-//     information disclosure via symlinks that point outside the workspace.
+//     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
+	// Lstat the resolved path for size and existence checks — EvalSymlinks
-	// ModeSymlink will never be set. We keep the check as defense-in-depth.
+	// 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)
+		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.
 	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.
+	// Open and read the docmap with a LimitedReader — closes the residual TOCTOU
-	cfg, err := review.ParseDocMapConfig(*docmapFlag)
+	// 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

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")
+	}
+}

docs/dev-loop-spec.md

@@ -231,6 +231,8 @@ These are statically checked by `~/.openclaw/workspace/scripts/test/check-invari
 | S6 | Active WIP does not cause early exit (only sets ACTIVE_WIP flag) |
 | S7 | SPAWN:impl guarded by `ACTIVE_WIP == 0` check |
 | S8 | No merge calls in any worker template |
+| S9 | Zero close-PR API calls in dispatch script (`state=closed` does not appear) |
+| S10 | No close-PR API calls in any worker template; every worker template contains `NEVER close a PR` |
 
 ---
 
@@ -263,9 +265,20 @@ Each worker receives a precise task description with substituted values:
 
 Workers **always** remove the WIP label on completion and reply `NO_REPLY`.
 
+### Worker Absolute Constraints
+
+Every worker template begins with an `⛔ ABSOLUTE CONSTRAINTS` section containing these rules:
+
+- **NEVER close a PR.** Never call `PATCH /pulls/{id}` with `state=closed`. Closing a PR requires human action. "Duplicate", "superseded", or "already done" are never a worker's call.
+- **NEVER merge a PR.** Never call the merge API. Merging requires human approval.
+- **NEVER use the gitea-aweiker token.** All API calls use the gitea-rodin token only.
+- **NEVER act on a PR with active REQUEST_CHANGES.** Fix the findings first.
+
+The first two constraints are statically enforced by `check-invariants.sh`: S1 and S9 cover the dispatch script (no merge, no close); S8 covers worker templates (no merge calls); S10 covers worker templates (no close calls, with NEVER-close text verified present in each). The remaining two constraints (token usage and REQUEST_CHANGES gate) are enforced by runtime logic.
+
 ---
 
-## 9. Fixes for Issues #144 and #145
+## 9. Fixes for Issues #144, #145, and #157
 
 **Issue #144** (autonomous merge):
 The dispatch script contains no merge API calls anywhere. The `~/.openclaw/workspace/scripts/test/check-invariants.sh`
@@ -276,3 +289,13 @@ Rule 2 is the **first** rule evaluated per PR. It cannot be skipped, reasoned pa
 or bypassed. It is checked before CI, before self-review, before handoff. The check
 uses latest-per-reviewer state, so a reviewer who re-approved after REQUEST_CHANGES
 is correctly handled.
+
+**Issue #157** (autonomous PR close):
+Worker templates were missing an explicit constraint against closing PRs. The dispatch
+script never had a close call, but workers could reason their way into calling
+`PATCH /pulls/{id}` with `state=closed`. All worker templates now include
+`NEVER close a PR` in their ABSOLUTE CONSTRAINTS section. Invariant S9 verifies
+the dispatch script contains no close calls. Invariant S10 verifies
+worker templates contain no close calls and each contains the NEVER-close text.
+
+Regression tests in `dispatch.bats` statically verify all of these constraints.