From da0183c12edfc359b09e5a07cf66d83c1de5c018 Mon Sep 17 00:00:00 2001
From: Rodin <rodin@forgedthought.ai>
Date: Fri, 1 May 2026 21:16:16 -0700
Subject: [PATCH] fix: symlink traversal + worst-wins pre-check + user scoping
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Security (MAJOR):
- Add filepath.EvalSymlinks after Clean for system-prompt-file
- Re-validate resolved path is still within workspace
- Prevents symlink → /etc/shadow exfiltration via malicious repo

Worst-wins:
- Check BEFORE posting (not after) — no delete+repost dance
- Identify sibling bots by <!-- review-bot: prefix in body
- Only escalates for bot reviews, not human REQUEST_CHANGES
- If sibling bot has REQUEST_CHANGES and we would APPROVE → post
  REQUEST_CHANGES instead

Addresses security review finding #1 (MAJOR) and sonnet finding #1.
---
 cmd/review-bot/main.go | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go
index 109436f..d4409e2 100644
--- a/cmd/review-bot/main.go
+++ b/cmd/review-bot/main.go
@@ -168,7 +168,15 @@ func main() {
 		if !strings.HasPrefix(promptPath, absWorkspace+string(filepath.Separator)) && promptPath != absWorkspace {
 			log.Fatalf("system-prompt-file resolves outside workspace (got %q, workspace %q)", promptPath, absWorkspace)
 		}
-		data, err := os.ReadFile(promptPath)
+		// Resolve symlinks and re-validate to prevent symlink traversal
+		resolvedPath, err := filepath.EvalSymlinks(promptPath)
+		if err != nil {
+			log.Fatalf("Failed to resolve system prompt file %q: %v", promptPath, err)
+		}
+		if !strings.HasPrefix(resolvedPath, absWorkspace+string(filepath.Separator)) && resolvedPath != absWorkspace {
+			log.Fatalf("system-prompt-file symlink resolves outside workspace (got %q, workspace %q)", resolvedPath, absWorkspace)
+		}
+		data, err := os.ReadFile(resolvedPath)
 		if err != nil {
 			log.Fatalf("Failed to read system prompt file %q: %v", promptPath, err)
 		}
@@ -226,21 +234,19 @@ func main() {
 		return
 	}
 
-	// Worst-wins: if we're about to APPROVE but a sibling review from the same
-	// user already has REQUEST_CHANGES, post as REQUEST_CHANGES too so we don't
-	// override the blocking state.
+	sentinel := fmt.Sprintf("<!-- review-bot:%s -->", *reviewerName)
+
+	// Worst-wins: if we would APPROVE but a sibling bot review (same token,
+	// different role) already has REQUEST_CHANGES, escalate to REQUEST_CHANGES.
+	// We identify sibling bot reviews by the <!-- review-bot: prefix (any role).
 	if event == "APPROVED" && *reviewerName != "" {
 		existing, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
 		if err == nil {
 			for _, r := range existing {
-				if !r.Stale && r.State == "REQUEST_CHANGES" {
-					// Check it's from the same user (same token) but a different role
-					sentinelCheck := fmt.Sprintf("<!-- review-bot:%s -->", *reviewerName)
-					if !strings.Contains(r.Body, sentinelCheck) {
-						log.Printf("Sibling review %d has REQUEST_CHANGES; escalating to REQUEST_CHANGES", r.ID)
-						event = "REQUEST_CHANGES"
-						break
-					}
+				if !r.Stale && r.State == "REQUEST_CHANGES" && strings.Contains(r.Body, "<!-- review-bot:") && !strings.Contains(r.Body, sentinel) {
+					log.Printf("Sibling bot review %d has REQUEST_CHANGES; escalating to REQUEST_CHANGES", r.ID)
+					event = "REQUEST_CHANGES"
+					break
 				}
 			}
 		}
@@ -254,7 +260,6 @@ func main() {
 	log.Printf("Review posted (id=%d, user=%s)", posted.ID, posted.User.Login)
 
 	// Delete stale reviews from this bot using sentinel matching
-	sentinel := fmt.Sprintf("<!-- review-bot:%s -->", *reviewerName)
 	if *updateExisting && *reviewerName != "" {
 		reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
 		if err != nil {