fix: symlink traversal + worst-wins pre-check + user scoping
CI / test (pull_request) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 24s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m13s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m18s

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.
This commit is contained in:
Rodin
2026-05-01 21:16:16 -07:00
parent 687005d982
commit 777158b681
+10 -21
View File
@@ -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,7 @@ 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
}
}
}
}
}
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 +244,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 {