Repository: reviewdog/reviewdog
Pull request #1521 (2023-08-09 16:15:53)

strict check of pr-review write permission
There is currently no way to check the available scope of the tokens provided by GitHub; the tokens available for GitHub Actions vary in permissions in complex ways, depending on the repository, workflow settings, and how the workflow is launched.

The most reliable way is to actually post review comments.

- [x] Updated Unreleased section in [CHANGELOG](https://github.com/reviewdog/reviewdog/blob/master/CHANGELOG.md) or it's not notable changes.

close https://github.com/reviewdog/reviewdog/issues/1210



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index edc59b26..ee4a4717 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - [#1573](https://github.com/reviewdog/reviewdog/pull/1573) Add filter tests for file/nofilter mode
 - [#1576](https://github.com/reviewdog/reviewdog/pull/1576) Support `--filter-mode=file` in `github-pr-review`
 - [#1596](https://github.com/reviewdog/reviewdog/pull/1596) Use `CI_MERGE_REQUEST_DIFF_BASE_SHA` envvar if available in `gitlab-mr-discussion`
+- [#1521](https://github.com/reviewdog/reviewdog/pull/1521) strict check of pr-review write permission
 
 ### :bug: Fixes
 - ...
diff --git a/cienv/github_actions.go b/cienv/github_actions.go
index 0c31d3ca..abc94667 100644
--- a/cienv/github_actions.go
+++ b/cienv/github_actions.go
@@ -103,7 +103,9 @@ func getBuildInfoFromGitHubActionEventPath(eventPath string) (*BuildInfo, bool,
 
 // IsInGitHubAction returns true if reviewdog is running in GitHub Actions.
 func IsInGitHubAction() bool {
-	// https://docs.github.com/en/actions/configuring-and-managing-workflows/using-environment-variables#default-environment-variables
+	// https://docs.github.com/en/actions/learn-github-actions/variables#default-environment-variables
+	// > Always set to true when GitHub Actions is running the workflow.
+	// > You can use this variable to differentiate when tests are being run locally or by GitHub Actions.
 	return os.Getenv("GITHUB_ACTIONS") != ""
 }
 
diff --git a/cmd/reviewdog/main.go b/cmd/reviewdog/main.go
index 12bf4392..bee52b07 100644
--- a/cmd/reviewdog/main.go
+++ b/cmd/reviewdog/main.go
@@ -33,7 +33,6 @@ import (
 	bbservice "github.com/reviewdog/reviewdog/service/bitbucket"
 	gerritservice "github.com/reviewdog/reviewdog/service/gerrit"
 	githubservice "github.com/reviewdog/reviewdog/service/github"
-	"github.com/reviewdog/reviewdog/service/github/githubutils"
 	gitlabservice "github.com/reviewdog/reviewdog/service/gitlab"
 )
 
@@ -274,21 +273,7 @@ func run(r io.Reader, w io.Writer, opt *option) error {
 			fmt.Fprintln(os.Stderr, "reviewdog: this is not PullRequest build.")
 			return nil
 		}
-		// If it's running in GitHub Actions and it's PR from forked repository,
-		// replace comment writer to GitHubActionLogWriter to create annotations
-		// instead of review comment because if it's PR from forked repository,
-		// GitHub token doesn't have write permission due to security concern and
-		// cannot post results via Review API.
-		if cienv.IsInGitHubAction() && cienv.HasReadOnlyPermissionGitHubToken() {
-			fmt.Fprintln(os.Stderr, `reviewdog: This GitHub token doesn't have write permission of Review API [1], 
-so reviewdog will report results via logging command [2] and create annotations similar to
-github-pr-check reporter as a fallback.
-[1]: https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target, 
-[2]: https://help.github.com/en/actions/automating-your-workflow-with-github-actions/development-tools-for-github-actions#logging-commands`)
-			cs = githubutils.NewGitHubActionLogWriter(opt.level)
-		} else {
-			cs = reviewdog.MultiCommentService(gs, cs)
-		}
+		cs = reviewdog.MultiCommentService(gs, cs)
 		ds = gs
 	case "gitlab-mr-discussion":
 		build, cli, err := gitlabBuildWithClient()
@@ -478,7 +463,7 @@ func githubService(ctx context.Context, opt *option) (gs *githubservice.PullRequ
 		g.PullRequest = prID
 	}
 
-	gs, err = githubservice.NewGitHubPullRequest(client, g.Owner, g.Repo, g.PullRequest, g.SHA)
+	gs, err = githubservice.NewGitHubPullRequest(client, g.Owner, g.Repo, g.PullRequest, g.SHA, opt.level)
 	if err != nil {
 		return nil, false, err
 	}
diff --git a/comment.go b/comment.go
index efedb828..5d05c7e4 100644
--- a/comment.go
+++ b/comment.go
@@ -2,7 +2,7 @@ package reviewdog
 
 import "context"
 
-var _ BulkCommentService = &multiCommentService{}
+var _ BulkCommentService = (*multiCommentService)(nil)
 
 type multiCommentService struct {
 	services []CommentService
diff --git a/service/github/github.go b/service/github/github.go
index b256c8b7..5b1ef13e 100644
--- a/service/github/github.go
+++ b/service/github/github.go
@@ -4,6 +4,9 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"log"
+	"net/http"
+	"os"
 	"path/filepath"
 	"strings"
 	"sync"
@@ -18,8 +21,8 @@ import (
 	"github.com/reviewdog/reviewdog/service/serviceutil"
 )
 
-var _ reviewdog.CommentService = &PullRequest{}
-var _ reviewdog.DiffService = &PullRequest{}
+var _ reviewdog.CommentService = (*PullRequest)(nil)
+var _ reviewdog.DiffService = (*PullRequest)(nil)
 
 const maxCommentsPerRequest = 30
 
@@ -28,11 +31,20 @@ const (
 	invalidSuggestionPost = "</details>"
 )
 
+func isPermissionError(err error) bool {
+	var githubErr *github.ErrorResponse
+	if !errors.As(err, &githubErr) {
+		return false
+	}
+	status := githubErr.Response.StatusCode
+	return status == http.StatusForbidden || status == http.StatusNotFound
+}
+
 // PullRequest is a comment and diff service for GitHub PullRequest.
 //
 // API:
 //
-//	https://developer.github.com/v3/pulls/comments/#create-a-comment
+//	https://docs.github.com/en/rest/pulls/comments?apiVersion=2022-11-28#create-a-review-comment-for-a-pull-request
 //	POST /repos/:owner/:repo/pulls/:number/comments
 type PullRequest struct {
 	cli   *github.Client
@@ -41,8 +53,10 @@ type PullRequest struct {
 	pr    int
 	sha   string
 
-	muComments   sync.Mutex
-	postComments []*reviewdog.Comment
+	muComments    sync.Mutex
+	postComments  []*reviewdog.Comment
+	logWriter     *githubutils.GitHubActionLogWriter
+	fallbackToLog bool
 
 	postedcs commentutil.PostedComments
 
@@ -52,18 +66,27 @@ type PullRequest struct {
 
 // NewGitHubPullRequest returns a new PullRequest service.
 // PullRequest service needs git command in $PATH.
-func NewGitHubPullRequest(cli *github.Client, owner, repo string, pr int, sha string) (*PullRequest, error) {
+//
+// The GitHub Token generated by GitHub Actions may not have the necessary permissions.
+// For example, in the case of a PR from a forked repository, or when write permission is prohibited in the repository settings [1].
+//
+// In such a case, the service will fallback to GitHub Actions workflow commands [2].
+//
+// [1]: https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token
+// [2]: https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions
+func NewGitHubPullRequest(cli *github.Client, owner, repo string, pr int, sha, level string) (*PullRequest, error) {
 	workDir, err := serviceutil.GitRelWorkdir()
 	if err != nil {
 		return nil, fmt.Errorf("PullRequest needs 'git' command: %w", err)
 	}
 	return &PullRequest{
-		cli:   cli,
-		owner: owner,
-		repo:  repo,
-		pr:    pr,
-		sha:   sha,
-		wd:    workDir,
+		cli:       cli,
+		owner:     owner,
+		repo:      repo,
+		pr:        pr,
+		sha:       sha,
+		logWriter: githubutils.NewGitHubActionLogWriter(level),
+		wd:        workDir,
 	}, nil
 }
 
@@ -90,29 +113,46 @@ func (g *PullRequest) Flush(ctx context.Context) error {
 }
 
 func (g *PullRequest) postAsReviewComment(ctx context.Context) error {
-	comments := make([]*github.DraftReviewComment, 0, len(g.postComments))
+	if g.fallbackToLog {
+		// we don't have permission to post a review comment.
+		// Fallback to GitHub Actions log as report.
+		for _, c := range g.postComments {
+			if err := g.logWriter.Post(ctx, c); err != nil {
+				return err
+			}
+		}
+		return g.logWriter.Flush(ctx)
+	}
+
+	postComments := g.postComments
+	g.postComments = nil
+	rawComments := make([]*reviewdog.Comment, 0, len(postComments))
+	plainComments := make([]*github.PullRequestComment, 0, len(postComments))
+	reviewComments := make([]*github.DraftReviewComment, 0, len(postComments))
 	remaining := make([]*reviewdog.Comment, 0)
-	for _, c := range g.postComments {
+	for _, c := range postComments {
 		if !c.Result.InDiffFile {
 			// GitHub Review API cannot report results outside diff. If it's running
-			// in GitHub Actions, fallback to GitHub Actions log as report .
+			// in GitHub Actions, fallback to GitHub Actions log as report.
 			if cienv.IsInGitHubAction() {
-				githubutils.ReportAsGitHubActionsLog(c.ToolName, "warning", c.Result.Diagnostic)
+				if err := g.logWriter.Post(ctx, c); err != nil {
+					return err
+				}
 			}
 			continue
 		}
 		body := buildBody(c)
 		if g.postedcs.IsPosted(c, githubCommentLine(c), body) {
+			// it's already posted. skip it.
 			continue
 		}
+
+		rawComments = append(rawComments, c)
 		if !c.Result.InDiffContext {
 			// If the result is outside of diff context, fallback to GitHub Review
 			// Comment API.
 			comment := buildPullRequestComment(c, body, g.sha)
-			_, _, err := g.cli.PullRequests.CreateComment(ctx, g.owner, g.repo, g.pr, comment)
-			if err != nil {
-				return err
-			}
+			plainComments = append(plainComments, comment)
 			continue
 		}
 		// Only posts maxCommentsPerRequest comments per 1 request to avoid spammy
@@ -122,26 +162,70 @@ func (g *PullRequest) postAsReviewComment(ctx context.Context) error {
 		// > 403 You have triggered an abuse detection mechanism and have been
 		// > temporarily blocked from content creation. Please retry your request
 		// > again later.
-		// https://developer.github.com/v3/#abuse-rate-limits
-		if len(comments) >= maxCommentsPerRequest {
+		// https://docs.github.com/en/rest/overview/resources-in-the-rest-api?apiVersion=2022-11-28#rate-limiting
+		if len(reviewComments) >= maxCommentsPerRequest {
 			remaining = append(remaining, c)
 			continue
 		}
-		comments = append(comments, buildDraftReviewComment(c, body))
+		reviewComments = append(reviewComments, buildDraftReviewComment(c, body))
+	}
+	if err := g.logWriter.Flush(ctx); err != nil {
+		return err
 	}
 
-	if len(comments) == 0 {
-		return nil
+	if len(plainComments) > 0 {
+		// send pull request comments to GitHub.
+		for _, c := range plainComments {
+			_, _, err := g.cli.PullRequests.CreateComment(ctx, g.owner, g.repo, g.pr, c)
+			if err != nil {
+				log.Printf("reviewdog: failed to post a pull request comment: %v", err)
+				// GitHub returns 403 or 404 if we don't have permission to post a review comment.
+				// fallback to log message in this case.
+				if isPermissionError(err) && cienv.IsInGitHubAction() {
+					goto FALLBACK
+				}
+				return err
+			}
+		}
 	}
 
-	review := &github.PullRequestReviewRequest{
-		CommitID: &g.sha,
-		Event:    github.String("COMMENT"),
-		Comments: comments,
-		Body:     github.String(g.remainingCommentsSummary(remaining)),
+	if len(reviewComments) > 0 {
+		// send review comments to GitHub.
+		review := &github.PullRequestReviewRequest{
+			CommitID: &g.sha,
+			Event:    github.String("COMMENT"),
+			Comments: reviewComments,
+			Body:     github.String(g.remainingCommentsSummary(remaining)),
+		}
+		_, _, err := g.cli.PullRequests.CreateReview(ctx, g.owner, g.repo, g.pr, review)
+		if err != nil {
+			log.Printf("reviewdog: failed to post a review comment: %v", err)
+			// GitHub returns 403 or 404 if we don't have permission to post a review comment.
+			// fallback to log message in this case.
+			if isPermissionError(err) && cienv.IsInGitHubAction() {
+				goto FALLBACK
+			}
+			return err
+		}
+	}
+
+	return nil
+
+FALLBACK:
+	// fallback to GitHub Actions log as report.
+	fmt.Fprintln(os.Stderr, `reviewdog: This GitHub Token doesn't have write permission of Review API [1],
+so reviewdog will report results via logging command [2] and create annotations similar to
+github-pr-check reporter as a fallback.
+[1]: https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target
+[2]: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions`)
+	g.fallbackToLog = true
+
+	for _, c := range rawComments {
+		if err := g.logWriter.Post(ctx, c); err != nil {
+			return err
+		}
 	}
-	_, _, err := g.cli.PullRequests.CreateReview(ctx, g.owner, g.repo, g.pr, review)
-	return err
+	return g.logWriter.Flush(ctx)
 }
 
 // Document: https://docs.github.com/en/rest/reference/pulls#create-a-review-comment-for-a-pull-request
@@ -230,6 +314,7 @@ func (g *PullRequest) remainingCommentsSummary(remaining []*reviewdog.Comment) s
 	return sb.String()
 }
 
+// setPostedComment get posted comments from GitHub.
 func (g *PullRequest) setPostedComment(ctx context.Context) error {
 	g.postedcs = make(commentutil.PostedComments)
 	cs, err := g.comment(ctx)


Test patch:
diff --git a/service/github/github_test.go b/service/github/github_test.go
index 055d6884..02b73b5c 100644
--- a/service/github/github_test.go
+++ b/service/github/github_test.go
@@ -67,7 +67,7 @@ func TestGitHubPullRequest_Post(t *testing.T) {
 	pr := 2
 	sha := "cce89afa9ac5519a7f5b1734db2e3aa776b138a7"
 
-	g, err := NewGitHubPullRequest(client, owner, repo, pr, sha)
+	g, err := NewGitHubPullRequest(client, owner, repo, pr, sha, "warning")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -150,7 +150,7 @@ index 61450f3..f63f149 100644
 	owner := "haya14busa"
 	repo := "reviewdog"
 	pr := 2
-	g, err := NewGitHubPullRequest(client, owner, repo, pr, "")
+	g, err := NewGitHubPullRequest(client, owner, repo, pr, "", "warning")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -175,7 +175,7 @@ func TestGitHubPullRequest_comment(t *testing.T) {
 	owner := "haya14busa"
 	repo := "reviewdog"
 	pr := 2
-	g, err := NewGitHubPullRequest(client, owner, repo, pr, "")
+	g, err := NewGitHubPullRequest(client, owner, repo, pr, "", "warning")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -529,7 +529,7 @@ func TestGitHubPullRequest_Post_Flush_review_api(t *testing.T) {
 
 	cli := github.NewClient(nil)
 	cli.BaseURL, _ = url.Parse(ts.URL + "/")
-	g, err := NewGitHubPullRequest(cli, "o", "r", 14, "sha")
+	g, err := NewGitHubPullRequest(cli, "o", "r", 14, "sha", "warning")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -1254,7 +1254,7 @@ func TestGitHubPullRequest_Post_toomany(t *testing.T) {
 
 	cli := github.NewClient(nil)
 	cli.BaseURL, _ = url.Parse(ts.URL + "/")
-	g, err := NewGitHubPullRequest(cli, "o", "r", 14, "sha")
+	g, err := NewGitHubPullRequest(cli, "o", "r", 14, "sha", "warning")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -1299,7 +1299,7 @@ func TestGitHubPullRequest_workdir(t *testing.T) {
 	moveToRootDir()
 	defer setupEnvs()()
 
-	g, err := NewGitHubPullRequest(nil, "", "", 0, "")
+	g, err := NewGitHubPullRequest(nil, "", "", 0, "", "warning")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -1318,7 +1318,7 @@ func TestGitHubPullRequest_workdir(t *testing.T) {
 	if err := os.Chdir(subDir); err != nil {
 		t.Fatal(err)
 	}
-	g, _ = NewGitHubPullRequest(nil, "", "", 0, "")
+	g, _ = NewGitHubPullRequest(nil, "", "", 0, "", "warning")
 	if g.wd != subDir {
 		t.Fatalf("gitRelWorkdir() = %q, want %q", g.wd, subDir)
 	}
@@ -1349,7 +1349,7 @@ func TestGitHubPullRequest_Diff_fake(t *testing.T) {
 
 	cli := github.NewClient(nil)
 	cli.BaseURL, _ = url.Parse(ts.URL + "/")
-	g, err := NewGitHubPullRequest(cli, "o", "r", 14, "sha")
+	g, err := NewGitHubPullRequest(cli, "o", "r", 14, "sha", "warning")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -1360,3 +1360,66 @@ func TestGitHubPullRequest_Diff_fake(t *testing.T) {
 		t.Errorf("GitHub API should be called once; called %v times", apiCalled)
 	}
 }
+
+func TestGitHubPullRequest_Post_NoPermission(t *testing.T) {
+	cwd, _ := os.Getwd()
+	defer os.Chdir(cwd)
+	moveToRootDir()
+	t.Setenv("GITHUB_ACTIONS", "true")
+
+	listCommentsAPICalled := 0
+	postCommentsAPICalled := 0
+
+	mux := http.NewServeMux()
+	mux.HandleFunc("/repos/o/r/pulls/14/comments", func(w http.ResponseWriter, r *http.Request) {
+		listCommentsAPICalled++
+		if err := json.NewEncoder(w).Encode([]*github.PullRequestComment{}); err != nil {
+			t.Fatal(err)
+		}
+	})
+	mux.HandleFunc("/repos/o/r/pulls/14/reviews", func(w http.ResponseWriter, r *http.Request) {
+		postCommentsAPICalled++
+		w.WriteHeader(http.StatusNotFound)
+	})
+	ts := httptest.NewServer(mux)
+	defer ts.Close()
+
+	cli := github.NewClient(nil)
+	cli.BaseURL, _ = url.Parse(ts.URL + "/")
+	g, err := NewGitHubPullRequest(cli, "o", "r", 14, "sha", "warning")
+	if err != nil {
+		t.Fatal(err)
+	}
+	comments := []*reviewdog.Comment{
+		{
+			Result: &filter.FilteredDiagnostic{
+				Diagnostic: &rdf.Diagnostic{
+					Location: &rdf.Location{
+						Path: "service/github/github_test.go",
+						Range: &rdf.Range{Start: &rdf.Position{
+							Line: 1,
+						}},
+					},
+					Message: "test message for TestGitHubPullRequest_Post_NoPermission",
+				},
+				InDiffFile:    true,
+				InDiffContext: true,
+			},
+			ToolName: "service/github/github_test.go",
+		},
+	}
+	for _, c := range comments {
+		if err := g.Post(context.Background(), c); err != nil {
+			t.Error(err)
+		}
+	}
+	if err := g.Flush(context.Background()); err != nil {
+		t.Error(err)
+	}
+	if want := 1; listCommentsAPICalled != want {
+		t.Errorf("GitHub List PullRequest comments API called %v times, want %d times", listCommentsAPICalled, want)
+	}
+	if want := 1; postCommentsAPICalled != want {
+		t.Errorf("GitHub post PullRequest comments API called %v times, want %d times", postCommentsAPICalled, want)
+	}
+}