From 9012db7306a52d4a3086b47b31952e47e340fa6d Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Mon, 6 Nov 2023 15:47:28 -0800 Subject: [PATCH] :bug: fallback to releases/v1 branch for codeql-action. (#484) Signed-off-by: Spencer Schrock --- .../testdata/api/github/containsCommit.json | 7 +++ app/server/testdata/api/github/divergent.json | 7 +++ .../testdata/api/github/repository.json | 8 +++ app/server/verify_workflow.go | 40 ++++++++++----- app/server/verify_workflow_test.go | 51 +++++++++++++++++++ 5 files changed, 101 insertions(+), 12 deletions(-) create mode 100644 app/server/testdata/api/github/containsCommit.json create mode 100644 app/server/testdata/api/github/divergent.json create mode 100644 app/server/testdata/api/github/repository.json diff --git a/app/server/testdata/api/github/containsCommit.json b/app/server/testdata/api/github/containsCommit.json new file mode 100644 index 00000000..1056af90 --- /dev/null +++ b/app/server/testdata/api/github/containsCommit.json @@ -0,0 +1,7 @@ +{ + "url": "https://api.github.com/repos/github/codeql-action/compare/releases/v1...54d8b0da6bba52eaed1c8042082f6560bc02ecb6", + "status": "behind", + "ahead_by": 0, + "behind_by": 197, + "total_commits": 0 +} diff --git a/app/server/testdata/api/github/divergent.json b/app/server/testdata/api/github/divergent.json new file mode 100644 index 00000000..3907abc1 --- /dev/null +++ b/app/server/testdata/api/github/divergent.json @@ -0,0 +1,7 @@ +{ + "url": "https://api.github.com/repos/github/codeql-action/compare/main...54d8b0da6bba52eaed1c8042082f6560bc02ecb6", + "status": "diverged", + "ahead_by": 193, + "behind_by": 1280, + "total_commits": 193 +} diff --git a/app/server/testdata/api/github/repository.json b/app/server/testdata/api/github/repository.json new file mode 100644 index 00000000..c3b8f6e5 --- /dev/null +++ b/app/server/testdata/api/github/repository.json @@ -0,0 +1,8 @@ +{ + "url": "https://api.github.com/repos/github/codeql-action", + "id": 259445878, + "node_id": "MDEwOlJlcG9zaXRvcnkyNTk0NDU4Nzg=", + "name": "codeql-action", + "full_name": "github/codeql-action", + "default_branch": "main" +} diff --git a/app/server/verify_workflow.go b/app/server/verify_workflow.go index 7c476d5e..d0e2ceb7 100644 --- a/app/server/verify_workflow.go +++ b/app/server/verify_workflow.go @@ -249,25 +249,26 @@ type githubVerifier struct { client *github.Client } -// contains currently makes two "core" API calls: one for the default branch, and one to compare the target hash -// This could also be done with the search commits API and a query of q=hash:+repo:/. +// contains makes two "core" API calls: one for the default branch, and one to compare the target hash to a branch +// if the repo is "github/codeql-action", also check releases/v1 before failing. func (g *githubVerifier) contains(owner, repo, hash string) (bool, error) { defaultBranch, err := g.defaultBranch(owner, repo) if err != nil { return false, err } - opts := &github.ListOptions{PerPage: 1} - diff, resp, err := g.client.Repositories.CompareCommits(g.ctx, owner, repo, defaultBranch, hash, opts) + contains, err := g.branchContains(defaultBranch, owner, repo, hash) if err != nil { - if resp.StatusCode == http.StatusNotFound { - // NotFound can be returned for some divergent cases: "404 No common ancestor between ..." - return false, nil - } - return false, fmt.Errorf("error comparing revisions: %w", err) + return false, err } - - // Target should be behind or at the base ref if it is considered contained. - return diff.GetStatus() == "behind" || diff.GetStatus() == "identical", nil + if contains { + return true, nil + } + // github/codeql-action has commits from their v1 release branch that don't show up in the default branch + // this isn't the best approach for now, but theres no universal "does this commit belong to this repo" call + if owner == "github" && repo == "codeql-action" { + contains, err = g.branchContains("releases/v1", owner, repo, hash) + } + return contains, err } func (g *githubVerifier) defaultBranch(owner, repo string) (string, error) { @@ -280,3 +281,18 @@ func (g *githubVerifier) defaultBranch(owner, repo string) (string, error) { } return *githubRepository.DefaultBranch, nil } + +func (g *githubVerifier) branchContains(branch, owner, repo, hash string) (bool, error) { + opts := &github.ListOptions{PerPage: 1} + diff, resp, err := g.client.Repositories.CompareCommits(g.ctx, owner, repo, branch, hash, opts) + if err != nil { + if resp.StatusCode == http.StatusNotFound { + // NotFound can be returned for some divergent cases: "404 No common ancestor between ..." + return false, nil + } + return false, fmt.Errorf("error comparing revisions: %w", err) + } + + // Target should be behind or at the base ref if it is considered contained. + return diff.GetStatus() == "behind" || diff.GetStatus() == "identical", nil +} diff --git a/app/server/verify_workflow_test.go b/app/server/verify_workflow_test.go index ceeca9b4..f2751044 100644 --- a/app/server/verify_workflow_test.go +++ b/app/server/verify_workflow_test.go @@ -15,10 +15,14 @@ package server import ( + "context" + "net/http" "os" + "strings" "testing" "unicode/utf8" + "github.com/google/go-github/v42/github" "github.com/stretchr/testify/assert" ) @@ -89,6 +93,53 @@ func TestVerifyInvalidWorkflows(t *testing.T) { } } +// suffix may not be the best term, but maps the final part of a path to a response file. +// this is helpful when multiple API calls need to be made. +// e.g. a call to /foo/bar/some/endpoint would have "endpoint" as a suffix. +type suffixStubTripper struct { + // key is suffix, value is response file. + responsePaths map[string]string +} + +func (s suffixStubTripper) RoundTrip(r *http.Request) (*http.Response, error) { + pathParts := strings.Split(r.URL.Path, "/") + suffix := pathParts[len(pathParts)-1] + f, err := os.Open(s.responsePaths[suffix]) + if err != nil { + return nil, err + } + return &http.Response{ + Status: "200 OK", + StatusCode: http.StatusOK, + Body: f, + }, nil +} + +func Test_githubVerifier_contains(t *testing.T) { + t.Parallel() + httpClient := http.Client{ + Transport: suffixStubTripper{ + responsePaths: map[string]string{ + "codeql-action": "./testdata/api/github/repository.json", // api call which finds the default branch + "main...somehash": "./testdata/api/github/divergent.json", // doesnt belong to default branch + "v1...somehash": "./testdata/api/github/containsCommit.json", // belongs to releases/v1 branch + }, + }, + } + client := github.NewClient(&httpClient) + gv := githubVerifier{ + ctx: context.Background(), + client: client, + } + got, err := gv.contains("github", "codeql-action", "somehash") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != true { + t.Errorf("expected to contain hash, but it didnt") + } +} + func FuzzVerifyWorkflow(f *testing.F) { testfiles := []string{ "testdata/workflow-valid.yml",