From 36c463c9bc946aa593ea5c9dc89b32ca9431d2e0 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 5 Apr 2023 11:19:25 -0700 Subject: [PATCH 1/6] Look for codeQL action use with local files instead of search. Signed-off-by: Spencer Schrock --- checks/sast.go | 75 ++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 61 insertions(+), 14 deletions(-) diff --git a/checks/sast.go b/checks/sast.go index 1cedc4e099c..e55b14958fc 100644 --- a/checks/sast.go +++ b/checks/sast.go @@ -23,9 +23,10 @@ import ( "regexp" "strings" + "github.com/rhysd/actionlint" + "github.com/ossf/scorecard/v4/checker" "github.com/ossf/scorecard/v4/checks/fileparser" - "github.com/ossf/scorecard/v4/clients" sce "github.com/ossf/scorecard/v4/errors" "github.com/ossf/scorecard/v4/finding" ) @@ -33,11 +34,15 @@ import ( // CheckSAST is the registered name for SAST. const CheckSAST = "SAST" -var errInvalid = errors.New("invalid") +var ( + errInvalid = errors.New("invalid") + errInvalidArgLength = errors.New("invalid arg length") + errInvalidArgType = errors.New("invalid arg type") -var sastTools = map[string]bool{"github-code-scanning": true, "lgtm-com": true, "sonarcloud": true} + sastTools = map[string]bool{"github-code-scanning": true, "lgtm-com": true, "sonarcloud": true} -var allowedConclusions = map[string]bool{"success": true, "neutral": true} + allowedConclusions = map[string]bool{"success": true, "neutral": true} +) //nolint:gochecknoinits func init() { @@ -187,19 +192,18 @@ func sastToolInCheckRuns(c *checker.CheckRequest) (int, error) { } func codeQLInCheckDefinitions(c *checker.CheckRequest) (int, error) { - searchRequest := clients.SearchRequest{ - Query: "github/codeql-action/analyze", - Path: "/.github/workflows", - } - resp, err := c.RepoClient.Search(searchRequest) + var workflowPaths []string + err := fileparser.OnMatchingFileContentDo(c.RepoClient, fileparser.PathMatcher{ + Pattern: ".github/workflows/*", + CaseSensitive: false, + }, searchGitHubActionWorkflowCodeQL, &workflowPaths) if err != nil { - return checker.InconclusiveResultScore, - sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("Client.Search.Code: %v", err)) + return checker.InconclusiveResultScore, err } - for _, result := range resp.Results { + for _, path := range workflowPaths { c.Dlogger.Debug(&checker.LogMessage{ - Path: result.Path, + Path: path, Type: finding.FileTypeSource, Offset: checker.OffsetDefault, Text: "CodeQL detected", @@ -208,7 +212,7 @@ func codeQLInCheckDefinitions(c *checker.CheckRequest) (int, error) { // TODO: check if it's enabled as cron or presubmit. // TODO: check which branches it is enabled on. We should find main. - if resp.Hits > 0 { + if len(workflowPaths) > 0 { c.Dlogger.Info(&checker.LogMessage{ Text: "SAST tool detected: CodeQL", }) @@ -221,6 +225,49 @@ func codeQLInCheckDefinitions(c *checker.CheckRequest) (int, error) { return checker.MinResultScore, nil } +// Check file content. +var searchGitHubActionWorkflowCodeQL fileparser.DoWhileTrueOnFileContent = func(path string, + content []byte, + args ...interface{}, +) (bool, error) { + if !fileparser.IsWorkflowFile(path) { + return true, nil + } + + if len(args) != 1 { + return false, fmt.Errorf( + "searchGitHubActionWorkflowCodeQL requires exactly 1 arguments: %w", errInvalidArgLength) + } + + // Verify the type of the data. + paths, ok := args[0].(*[]string) + if !ok { + return false, fmt.Errorf( + "searchGitHubActionWorkflowCodeQL expects arg[0] of type []string: %w", errInvalidArgType) + } + + workflow, errs := actionlint.Parse(content) + if len(errs) > 0 && workflow == nil { + return false, fileparser.FormatActionlintError(errs) + } + + for _, job := range workflow.Jobs { + for _, step := range job.Steps { + e, ok := step.Exec.(*actionlint.ExecAction) + if !ok { + continue + } + // Parse out repo / SHA. + uses := strings.TrimPrefix(e.Uses.Value, "actions://") + action, _, _ := strings.Cut(uses, "@") + if action == "github/codeql-action/analyze" { + *paths = append(*paths, path) + } + } + } + return true, nil +} + type sonarConfig struct { url string file checker.File From 4825589a1a10aff5a323166f38f29b5eeefab5bc Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 5 Apr 2023 12:00:16 -0700 Subject: [PATCH 2/6] Switch SAST mocks to using local file contents. Signed-off-by: Spencer Schrock --- checks/sast_test.go | 70 ++++++++++++++++--- .../github-workflow-sast-codeql.yaml | 24 +++++++ .../github-workflow-sast-no-codeql.yaml | 38 ++++++++++ 3 files changed, 124 insertions(+), 8 deletions(-) create mode 100644 checks/testdata/.github/workflows/github-workflow-sast-codeql.yaml create mode 100644 checks/testdata/.github/workflows/github-workflow-sast-no-codeql.yaml diff --git a/checks/sast_test.go b/checks/sast_test.go index 1ed5080fe62..cf8cf446281 100644 --- a/checks/sast_test.go +++ b/checks/sast_test.go @@ -19,6 +19,7 @@ import ( "errors" "fmt" "os" + "strings" "testing" "time" @@ -105,9 +106,7 @@ func Test_SAST(t *testing.T) { }, }, }, - searchresult: clients.SearchResponse{Hits: 1, Results: []clients.SearchResult{{ - Path: "test.go", - }}}, + path: ".github/workflows/github-workflow-sast-codeql.yaml", checkRuns: []clients.CheckRun{ { Status: "completed", @@ -144,7 +143,7 @@ func Test_SAST(t *testing.T) { }, }, }, - searchresult: clients.SearchResponse{}, + path: ".github/workflows/github-workflow-sast-no-codeql.yaml", checkRuns: []clients.CheckRun{ { App: clients.CheckRunApp{ @@ -201,14 +200,14 @@ func Test_SAST(t *testing.T) { }, { name: "sonartype config 1 line", - path: "./testdata/pom-1line.xml", + path: "pom-1line.xml", expected: checker.CheckResult{ Score: 10, }, }, { name: "sonartype config 2 lines", - path: "./testdata/pom-2lines.xml", + path: "pom-2lines.xml", expected: checker.CheckResult{ Score: 10, }, @@ -234,13 +233,16 @@ func Test_SAST(t *testing.T) { mockRepoClient.EXPECT().Search(searchRequest).Return(tt.searchresult, nil).AnyTimes() mockRepoClient.EXPECT().ListFiles(gomock.Any()).DoAndReturn( func(predicate func(string) (bool, error)) ([]string, error) { - return []string{"pom.xml"}, nil + if strings.Contains(tt.path, "pom") { + return []string{"pom.xml"}, nil + } + return []string{tt.path}, nil }).AnyTimes() mockRepoClient.EXPECT().GetFileContent(gomock.Any()).DoAndReturn(func(fn string) ([]byte, error) { if tt.path == "" { return nil, nil } - content, err := os.ReadFile(tt.path) + content, err := os.ReadFile("./testdata/" + tt.path) if err != nil { return content, fmt.Errorf("%w", err) } @@ -342,3 +344,55 @@ func Test_validateSonarConfig(t *testing.T) { }) } } + +func Test_SAST_workflow_contents(t *testing.T) { + t.Parallel() + tests := []struct { + name string + filename string + expected int + }{ + { + name: "detects codeql", + filename: ".github/workflows/github-workflow-sast-codeql.yaml", + expected: checker.MaxResultScore, + }, + { + name: "no codeql present", + filename: ".github/workflows/github-workflow-sast-no-codeql.yaml", + expected: checker.MinResultScore, + }, + } + for _, tt := range tests { + tt := tt // Re-initializing variable so it is not changed while executing the closure below + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mockRepoClient := mockrepo.NewMockRepoClient(ctrl) + mockRepoClient.EXPECT().ListCommits().Return(nil, nil) + mockRepoClient.EXPECT().ListFiles(gomock.Any()).Return([]string{tt.filename}, nil).AnyTimes() + mockRepoClient.EXPECT().GetFileContent(gomock.Any()).DoAndReturn(func(file string) ([]byte, error) { + // This will read the file and return the content + content, err := os.ReadFile("testdata/" + file) + if err != nil { + return content, fmt.Errorf("%w", err) + } + return content, nil + }).AnyTimes() + + dl := scut.TestDetailLogger{} + req := checker.CheckRequest{ + RepoClient: mockRepoClient, + Ctx: context.TODO(), + Dlogger: &dl, + } + res := SAST(&req) + + if res.Score != tt.expected { + t.Errorf("Expected score %d, got %d for %v", tt.expected, res.Score, tt.name) + } + ctrl.Finish() + }) + } +} diff --git a/checks/testdata/.github/workflows/github-workflow-sast-codeql.yaml b/checks/testdata/.github/workflows/github-workflow-sast-codeql.yaml new file mode 100644 index 00000000000..9758f530da5 --- /dev/null +++ b/checks/testdata/.github/workflows/github-workflow-sast-codeql.yaml @@ -0,0 +1,24 @@ +# Copyright 2021 OpenSSF Scorecard Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +name: absent workflow +on: [push] + +jobs: + Explore-GitHub-Actions: + runs-on: ubuntu-latest + steps: + - name: Perform CodeQL Analysis + uses: github/codeql-action/analyze@v1 + - name: some name + uses: docker/build-push-action@1.2.3 diff --git a/checks/testdata/.github/workflows/github-workflow-sast-no-codeql.yaml b/checks/testdata/.github/workflows/github-workflow-sast-no-codeql.yaml new file mode 100644 index 00000000000..9d3b2c4ffe3 --- /dev/null +++ b/checks/testdata/.github/workflows/github-workflow-sast-no-codeql.yaml @@ -0,0 +1,38 @@ +# Copyright 2021 OpenSSF Scorecard Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +on: + push: + paths: + - 'source/common/**' + pull_request: + +jobs: + Some-Build: + + strategy: + fail-fast: false + + # CodeQL runs on ubuntu-latest and windows-latest + runs-on: ubuntu-latest + + steps: + - name: Checkout repository + uses: actions/checkout@v2 + with: + fetch-depth: 2 + + - name: Acme CodeQL + uses: acme/codeql-action/init@v2 # some comment + with: + languages: cpp From 968a4fffb5aba360dc7c02afecffb2a034b16d4b Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 5 Apr 2023 12:39:13 -0700 Subject: [PATCH 3/6] Update e2e test Signed-off-by: Spencer Schrock --- e2e/sast_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/e2e/sast_test.go b/e2e/sast_test.go index 82c48546fa8..2cd7743b7ee 100644 --- a/e2e/sast_test.go +++ b/e2e/sast_test.go @@ -44,10 +44,10 @@ var _ = Describe("E2E TEST:"+checks.CheckSAST, func() { } expected := scut.TestReturn{ Error: nil, - Score: 0, - NumberOfWarn: 2, - NumberOfInfo: 0, - NumberOfDebug: 0, + Score: 10, + NumberOfWarn: 1, + NumberOfInfo: 1, + NumberOfDebug: 1, } result := checks.SAST(&req) // New version. From 27280dbc3901b82d29969eb093a6fdce729cb2bd Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Fri, 7 Apr 2023 14:19:20 -0700 Subject: [PATCH 4/6] Remove unneeded code. The tests deleted here were merged with another test in an earlier commit. Signed-off-by: Spencer Schrock --- checks/sast.go | 8 +++---- checks/sast_test.go | 52 --------------------------------------------- 2 files changed, 3 insertions(+), 57 deletions(-) diff --git a/checks/sast.go b/checks/sast.go index e55b14958fc..3d23374f9e9 100644 --- a/checks/sast.go +++ b/checks/sast.go @@ -35,9 +35,7 @@ import ( const CheckSAST = "SAST" var ( - errInvalid = errors.New("invalid") - errInvalidArgLength = errors.New("invalid arg length") - errInvalidArgType = errors.New("invalid arg type") + errInvalid = errors.New("invalid") sastTools = map[string]bool{"github-code-scanning": true, "lgtm-com": true, "sonarcloud": true} @@ -236,14 +234,14 @@ var searchGitHubActionWorkflowCodeQL fileparser.DoWhileTrueOnFileContent = func( if len(args) != 1 { return false, fmt.Errorf( - "searchGitHubActionWorkflowCodeQL requires exactly 1 arguments: %w", errInvalidArgLength) + "searchGitHubActionWorkflowCodeQL requires exactly 1 arguments: %w", errInvalid) } // Verify the type of the data. paths, ok := args[0].(*[]string) if !ok { return false, fmt.Errorf( - "searchGitHubActionWorkflowCodeQL expects arg[0] of type []string: %w", errInvalidArgType) + "searchGitHubActionWorkflowCodeQL expects arg[0] of type []string: %w", errInvalid) } workflow, errs := actionlint.Parse(content) diff --git a/checks/sast_test.go b/checks/sast_test.go index cf8cf446281..7f2d23dde93 100644 --- a/checks/sast_test.go +++ b/checks/sast_test.go @@ -344,55 +344,3 @@ func Test_validateSonarConfig(t *testing.T) { }) } } - -func Test_SAST_workflow_contents(t *testing.T) { - t.Parallel() - tests := []struct { - name string - filename string - expected int - }{ - { - name: "detects codeql", - filename: ".github/workflows/github-workflow-sast-codeql.yaml", - expected: checker.MaxResultScore, - }, - { - name: "no codeql present", - filename: ".github/workflows/github-workflow-sast-no-codeql.yaml", - expected: checker.MinResultScore, - }, - } - for _, tt := range tests { - tt := tt // Re-initializing variable so it is not changed while executing the closure below - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - ctrl := gomock.NewController(t) - mockRepoClient := mockrepo.NewMockRepoClient(ctrl) - mockRepoClient.EXPECT().ListCommits().Return(nil, nil) - mockRepoClient.EXPECT().ListFiles(gomock.Any()).Return([]string{tt.filename}, nil).AnyTimes() - mockRepoClient.EXPECT().GetFileContent(gomock.Any()).DoAndReturn(func(file string) ([]byte, error) { - // This will read the file and return the content - content, err := os.ReadFile("testdata/" + file) - if err != nil { - return content, fmt.Errorf("%w", err) - } - return content, nil - }).AnyTimes() - - dl := scut.TestDetailLogger{} - req := checker.CheckRequest{ - RepoClient: mockRepoClient, - Ctx: context.TODO(), - Dlogger: &dl, - } - res := SAST(&req) - - if res.Score != tt.expected { - t.Errorf("Expected score %d, got %d for %v", tt.expected, res.Score, tt.name) - } - ctrl.Finish() - }) - } -} From e3f8278052c8f26f4b1953e0fdacad5884c21555 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Fri, 7 Apr 2023 14:20:34 -0700 Subject: [PATCH 5/6] update Signed-off-by: Spencer Schrock --- checks/sast.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/checks/sast.go b/checks/sast.go index 3d23374f9e9..2a433334df4 100644 --- a/checks/sast.go +++ b/checks/sast.go @@ -34,13 +34,11 @@ import ( // CheckSAST is the registered name for SAST. const CheckSAST = "SAST" -var ( - errInvalid = errors.New("invalid") +var errInvalid = errors.New("invalid") - sastTools = map[string]bool{"github-code-scanning": true, "lgtm-com": true, "sonarcloud": true} +var sastTools = map[string]bool{"github-code-scanning": true, "lgtm-com": true, "sonarcloud": true} - allowedConclusions = map[string]bool{"success": true, "neutral": true} -) +var allowedConclusions = map[string]bool{"success": true, "neutral": true} //nolint:gochecknoinits func init() { @@ -241,7 +239,7 @@ var searchGitHubActionWorkflowCodeQL fileparser.DoWhileTrueOnFileContent = func( paths, ok := args[0].(*[]string) if !ok { return false, fmt.Errorf( - "searchGitHubActionWorkflowCodeQL expects arg[0] of type []string: %w", errInvalid) + "searchGitHubActionWorkflowCodeQL expects arg[0] of type *[]string: %w", errInvalid) } workflow, errs := actionlint.Parse(content) From f3deb66ea1b4eab1a99f914b9117609234d8dc23 Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Wed, 12 Apr 2023 14:45:59 -0700 Subject: [PATCH 6/6] Add tests to get code coverage up. Signed-off-by: Spencer Schrock --- checks/sast_test.go | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/checks/sast_test.go b/checks/sast_test.go index 7f2d23dde93..5e653a8e4a4 100644 --- a/checks/sast_test.go +++ b/checks/sast_test.go @@ -344,3 +344,46 @@ func Test_validateSonarConfig(t *testing.T) { }) } } + +func Test_searchGitHubActionWorkflowCodeQL_invalid(t *testing.T) { + t.Parallel() + + //nolint: govet + tests := []struct { + name string + path string + args []any + }{ + { + name: "too few arguments", + path: ".github/workflows/github-workflow-sast-codeql.yaml", + args: []any{}, + }, + { + name: "wrong arguments", + path: ".github/workflows/github-workflow-sast-codeql.yaml", + args: []any{ + &[]int{}, + }, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + var content []byte + var err error + if tt.path != "" { + content, err = os.ReadFile("./testdata/" + tt.path) + if err != nil { + t.Errorf("ReadFile: %v", err) + } + } + _, err = searchGitHubActionWorkflowCodeQL(tt.path, content, tt.args...) + if err == nil { + t.Errorf("Expected error but err was nil") + } + }) + } +}