Verify that the pull request head commit exists (#55)

Some GitHub APIs have a delay propagating information about new commits,
particularly when the commits come from forks. While I believe the APIs
we currently use are safe, if they are not, the policy should fail
closed with an error instead of potentially failing open, as it does
currently.

Also reorganize some test data so that it is more consistent, making it
easier for tests to pass this new check.

pull/github.go

@@ -153,10 +153,21 @@ func (ghc *GitHubContext) Commits() ([]*Commit, error) {
 			return nil, err
 		}
 	}
+
 	if len(ghc.commits) >= MaxPullRequestCommits {
 		return nil, errors.Errorf("too many commits in pull request, maximum is %d", MaxPullRequestCommits)
 	}
-	return ghc.commits, nil
+
+	// verify that the head of the PR being evaluated exists in commit list
+	// some GitHub APIs have a delay propagating commit information
+	headSHA := ghc.pr.GetHead().GetSHA()
+	for i := len(ghc.commits) - 1; i >= 0; i-- {
+		if headSHA == ghc.commits[i].SHA {
+			return ghc.commits, nil
+		}
+	}
+
+	return nil, errors.Errorf("pull request head %s was missing from commit listing", headSHA)
 }
 
 func (ghc *GitHubContext) Comments() ([]*Comment, error) {

pull/github_test.go

@@ -31,7 +31,7 @@ func TestAuthor(t *testing.T) {
 	rp := &ResponsePlayer{}
 	pullsRule := rp.AddRule(
 		ExactPathMatcher("/repos/testorg/testrepo/pulls/123"),
-		"testdata/responses/pull_author.yml",
+		"testdata/responses/pull.yml",
 	)
 
 	ctx := makeContext(rp)
@@ -84,6 +84,10 @@ func TestChangedFiles(t *testing.T) {
 
 func TestCommits(t *testing.T) {
 	rp := &ResponsePlayer{}
+	rp.AddRule(
+		ExactPathMatcher("/repos/testorg/testrepo/pulls/123"),
+		"testdata/responses/pull.yml",
+	)
 	dataRule := rp.AddRule(
 		GraphQLNodePrefixMatcher("repository.pullRequest.commits"),
 		"testdata/responses/pull_data_commits.yml",
@@ -253,6 +257,10 @@ func TestIsTeamMember(t *testing.T) {
 
 func TestMixedPaging(t *testing.T) {
 	rp := &ResponsePlayer{}
+	rp.AddRule(
+		ExactPathMatcher("/repos/testorg/testrepo/pulls/123"),
+		"testdata/responses/pull.yml",
+	)
 	dataRule := rp.AddRule(
 		GraphQLNodePrefixMatcher("repository.pullRequest"),
 		"testdata/responses/pull_data_mixed.yml",
@@ -271,8 +279,8 @@ func TestMixedPaging(t *testing.T) {
 
 	assert.Equal(t, 3, dataRule.Count, "cached values were not used")
 	assert.Len(t, comments, 2, "incorrect number of comments")
-	assert.Len(t, reviews, 3, "incorrect number of reviews")
-	assert.Len(t, commits, 2, "incorrect number of commits")
+	assert.Len(t, reviews, 2, "incorrect number of reviews")
+	assert.Len(t, commits, 3, "incorrect number of commits")
 }
 
 func TestIsOrgMember(t *testing.T) {
@@ -323,15 +331,13 @@ func makeContext(rp *ResponsePlayer) Context {
 		pr = &github.PullRequest{}
 	}
 
-	// insert the values needed for the context
-	repoOwner, repoName, prNum := "testorg", "testrepo", 123
-	pr.Number = &prNum
+	pr.Number = github.Int(123)
 	pr.Base = &github.PullRequestBranch{
 		Repo: &github.Repository{
 			Owner: &github.User{
-				Login: &repoOwner,
+				Login: github.String("testorg"),
 			},
-			Name: &repoName,
+			Name: github.String("testrepo"),
 		},
 	}
 

pull/testdata/responses/pull.yml

@@ -0,0 +1,31 @@
+- status: 200
+  body: |
+    {
+      "number": 123,
+      "user": {
+        "login": "mhaypenny"
+      },
+      "head": {
+        "label": "testorg:test-branch",
+        "ref": "test-branch",
+        "sha": "e05fcae367230ee709313dd2720da527d178ce43",
+        "repo": {
+          "name": "testrepo",
+          "full_name": "testorg/testrepo",
+          "owner": {
+            "login": "testorg"
+          }
+        }
+      },
+      "base": {
+        "label": "testorg:develop",
+        "ref": "develop",
+        "repo": {
+          "name": "testrepo",
+          "full_name": "testorg/testrepo",
+          "owner": {
+            "login": "testorg"
+          }
+        }
+      }
+    }

pull/testdata/responses/pull_data_mixed.yml

@@ -63,7 +63,7 @@
                   "author": {
                     "login": "mhaypenny"
                   },
-                  "state": "APPROVED",
+                  "state": "CHANGES_REQUESTED",
                   "body": "",
                   "submittedAt": "2018-06-27T20:33:26Z"
                 }
@@ -90,7 +90,7 @@
             "commits": {
               "pageInfo": {
                 "endCursor": "3",
-                "hasNextPage": false
+                "hasNextPage": true
               },
               "nodes": [
                 {
@@ -114,16 +114,16 @@
             "reviews": {
               "pageInfo": {
                 "endCursor": "3",
-                "hasNextPage": true
+                "hasNextPage": false
               },
               "nodes": [
                 {
                   "author": {
-                    "login": "ttest"
+                    "login": "bkeyes"
                   },
-                  "state": "CHANGES_REQUESTED",
-                  "body": "",
-                  "submittedAt": "2018-06-27T20:44:26Z"
+                  "state": "APPROVED",
+                  "body": "the body",
+                  "submittedAt": "2018-06-27T20:33:27Z"
                 }
               ]
             }
@@ -146,27 +146,35 @@
               "nodes": []
             },
             "commits": {
-              "pageInfo": {
-                "endCursor": null,
-                "hasNextPage": false
-              },
-              "nodes": []
-            },
-            "reviews": {
               "pageInfo": {
                 "endCursor": "4",
                 "hasNextPage": false
               },
               "nodes": [
                 {
-                  "author": {
-                    "login": "bkeyes"
-                  },
-                  "state": "CHANGES_REQUESTED",
-                  "body": "",
-                  "submittedAt": "2018-06-27T20:55:26Z"
+                  "commit": {
+                    "oid": "e05fcae367230ee709313dd2720da527d178ce43",
+                    "pushedDate": "2018-12-06T12:34:56Z",
+                    "author": {
+                      "user": {
+                        "login": "ttest"
+                      }
+                    },
+                    "committer": {
+                      "user": {
+                        "login": "mhaypenny"
+                      }
+                    }
+                  }
                 }
               ]
+            },
+            "reviews": {
+              "pageInfo": {
+                "endCursor": null,
+                "hasNextPage": false
+              },
+              "nodes": []
             }
           }
         }