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("", *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("", *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("", *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, "", true}, + {"invalid space", "my bot", true}, + {"invalid dot", "my.bot", true}, + {"invalid slash", "my/bot", true}, + {"invalid angle", "bot