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 23s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m23s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m33s
CI / test (pull_request) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 23s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m23s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m33s
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:
+18
-13
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user