From 436e6a88246795e600ff76029a3ed1f98f1992c8 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.
---
 .gitea/actions/review/action.yml |   2 +-
 cmd/review-bot/main.go           |  75 +++++++++++-----
 cmd/review-bot/main_test.go      | 150 +++++++++++++++++++++++++++++++
 3 files changed, 206 insertions(+), 21 deletions(-)
 create mode 100644 cmd/review-bot/main_test.go

diff --git a/.gitea/actions/review/action.yml b/.gitea/actions/review/action.yml
index 912c7da..78f1005 100644
--- a/.gitea/actions/review/action.yml
+++ b/.gitea/actions/review/action.yml
@@ -67,7 +67,7 @@ inputs:
     required: false
     default: 'false'
   update-existing:
-    description: 'Delete previous review from same bot before posting. Accepts: true/1/yes or false/0/no (default true)'
+    description: 'Delete previous review from same bot after posting new one. Accepts: true/1/yes or false/0/no (default true)'
     required: false
     default: 'true'
   system-prompt-file:
diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go
index 109436f..d071934 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,25 +234,11 @@ 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.
-	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
-					}
-				}
-			}
-		}
+	// Validate reviewer-name: only safe characters allowed in sentinel
+	if err := validateReviewerName(*reviewerName); err != nil {
+		log.Fatalf("%v", err)
 	}
+	sentinel := fmt.Sprintf("<!-- review-bot:%s -->", *reviewerName)
 
 	log.Printf("Posting review (event=%s)...", event)
 	posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody)
@@ -254,7 +248,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 {
@@ -270,6 +263,22 @@ func main() {
 				}
 			}
 
+			// Worst-wins: if we posted APPROVE but a sibling review from the
+			// same user (same token, different role) has REQUEST_CHANGES,
+			// delete ours and re-post as REQUEST_CHANGES to maintain the block.
+			if event == "APPROVED" && shouldEscalate(reviews, posted.ID, posted.User.Login, sentinel) {
+				log.Printf("Sibling review has REQUEST_CHANGES; escalating")
+				if err := giteaClient.DeleteReview(ctx, owner, repoName, prNumber, posted.ID); err != nil {
+					log.Printf("Warning: could not delete review for escalation: %v", err)
+				} else {
+					_, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, "REQUEST_CHANGES", reviewBody)
+					if err != nil {
+						log.Printf("Warning: could not re-post as REQUEST_CHANGES: %v", err)
+					} else {
+						log.Printf("Review escalated to REQUEST_CHANGES")
+					}
+				}
+			}
 		}
 	}
 }
@@ -413,3 +422,29 @@ func envOrDefaultBool(key string, defaultVal bool) bool {
 	}
 	return v == "true" || v == "1" || v == "yes"
 }
+
+// validateReviewerName checks that the name contains only safe characters
+// for embedding in an HTML comment sentinel ([a-zA-Z0-9_-]).
+func validateReviewerName(name string) error {
+	if name == "" {
+		return nil
+	}
+	for _, ch := range name {
+		if !((ch >= 'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z') || (ch >= '0' && ch <= '9') || ch == '-' || ch == '_') {
+			return fmt.Errorf("reviewer-name must contain only [a-zA-Z0-9_-] (got %q)", name)
+		}
+	}
+	return nil
+}
+
+// shouldEscalate checks if the current APPROVED review should be escalated
+// to REQUEST_CHANGES because a sibling bot review (same user, different role)
+// already has REQUEST_CHANGES.
+func shouldEscalate(reviews []gitea.Review, postedID int64, postedLogin, ownSentinel string) bool {
+	for _, r := range reviews {
+		if r.ID != postedID && !r.Stale && r.User.Login == postedLogin && r.State == "REQUEST_CHANGES" && strings.Contains(r.Body, "<!-- review-bot:") && !strings.Contains(r.Body, ownSentinel) {
+			return true
+		}
+	}
+	return false
+}
diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go
new file mode 100644
index 0000000..38304c5
--- /dev/null
+++ b/cmd/review-bot/main_test.go
@@ -0,0 +1,150 @@
+package main
+
+import (
+	"testing"
+
+	"gitea.weiker.me/rodin/review-bot/gitea"
+)
+
+func TestValidateReviewerName(t *testing.T) {
+	tests := []struct {
+		name    string
+		input   string
+		wantErr bool
+	}{
+		{"valid simple", "sonnet", false},
+		{"valid with dash", "code-review", false},
+		{"valid with underscore", "my_bot", false},
+		{"valid alphanumeric", "bot123", false},
+		{"valid uppercase", "MyBot", false},
+		{"empty is valid", "", false},
+		{"invalid html close", "foo-->", true},
+		{"invalid space", "my bot", true},
+		{"invalid dot", "my.bot", true},
+		{"invalid slash", "my/bot", true},
+		{"invalid angle", "bot<script>", true},
+		{"invalid colon", "bot:name", true},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			err := validateReviewerName(tc.input)
+			if tc.wantErr && err == nil {
+				t.Errorf("expected error for %q, got nil", tc.input)
+			}
+			if !tc.wantErr && err != nil {
+				t.Errorf("expected no error for %q, got %v", tc.input, err)
+			}
+		})
+	}
+}
+
+func makeReview(id int64, login, state string, stale bool, body string) gitea.Review {
+	r := gitea.Review{
+		ID:    id,
+		Body:  body,
+		State: state,
+		Stale: stale,
+	}
+	r.User.Login = login
+	return r
+}
+
+func TestShouldEscalate(t *testing.T) {
+	tests := []struct {
+		name        string
+		reviews     []gitea.Review
+		postedID    int64
+		postedLogin string
+		ownSentinel string
+		want        bool
+	}{
+		{
+			name:        "no reviews",
+			reviews:     nil,
+			postedID:    100,
+			postedLogin: "bot",
+			ownSentinel: "<!-- review-bot:sonnet -->",
+			want:        false,
+		},
+		{
+			name: "sibling same user has REQUEST_CHANGES",
+			reviews: []gitea.Review{
+				makeReview(101, "bot", "REQUEST_CHANGES", false, "bad\n<!-- review-bot:security -->"),
+			},
+			postedID:    100,
+			postedLogin: "bot",
+			ownSentinel: "<!-- review-bot:sonnet -->",
+			want:        true,
+		},
+		{
+			name: "sibling different user has REQUEST_CHANGES (should NOT escalate)",
+			reviews: []gitea.Review{
+				makeReview(101, "other-bot", "REQUEST_CHANGES", false, "bad\n<!-- review-bot:gpt -->"),
+			},
+			postedID:    100,
+			postedLogin: "bot",
+			ownSentinel: "<!-- review-bot:sonnet -->",
+			want:        false,
+		},
+		{
+			name: "same user REQUEST_CHANGES but stale (should NOT escalate)",
+			reviews: []gitea.Review{
+				makeReview(101, "bot", "REQUEST_CHANGES", true, "old\n<!-- review-bot:security -->"),
+			},
+			postedID:    100,
+			postedLogin: "bot",
+			ownSentinel: "<!-- review-bot:sonnet -->",
+			want:        false,
+		},
+		{
+			name: "same user same sentinel (own stale review, should NOT escalate)",
+			reviews: []gitea.Review{
+				makeReview(101, "bot", "REQUEST_CHANGES", false, "old\n<!-- review-bot:sonnet -->"),
+			},
+			postedID:    100,
+			postedLogin: "bot",
+			ownSentinel: "<!-- review-bot:sonnet -->",
+			want:        false,
+		},
+		{
+			name: "same user APPROVED sibling (should NOT escalate)",
+			reviews: []gitea.Review{
+				makeReview(101, "bot", "APPROVED", false, "good\n<!-- review-bot:security -->"),
+			},
+			postedID:    100,
+			postedLogin: "bot",
+			ownSentinel: "<!-- review-bot:sonnet -->",
+			want:        false,
+		},
+		{
+			name: "human REQUEST_CHANGES no sentinel (should NOT escalate)",
+			reviews: []gitea.Review{
+				makeReview(101, "bot", "REQUEST_CHANGES", false, "please fix this"),
+			},
+			postedID:    100,
+			postedLogin: "bot",
+			ownSentinel: "<!-- review-bot:sonnet -->",
+			want:        false,
+		},
+		{
+			name: "skip own posted ID",
+			reviews: []gitea.Review{
+				makeReview(100, "bot", "REQUEST_CHANGES", false, "x\n<!-- review-bot:security -->"),
+			},
+			postedID:    100,
+			postedLogin: "bot",
+			ownSentinel: "<!-- review-bot:sonnet -->",
+			want:        false,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := shouldEscalate(tc.reviews, tc.postedID, tc.postedLogin, tc.ownSentinel)
+			if got != tc.want {
+				t.Errorf("shouldEscalate() = %v, want %v", got, tc.want)
+			}
+		})
+	}
+}