From c741335683407fccc1edff26cbbbe54d8a1e3227 Mon Sep 17 00:00:00 2001 From: laurentsimon <64505099+laurentsimon@users.noreply.github.com> Date: Wed, 21 Jul 2021 09:21:43 -0700 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8=20[migration=20to=20score]=203:=20bra?= =?UTF-8?q?nch=20protection,=20frozen-deps,=20token=20permissions=20(#719)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * details-1 * nits * typo * commments * dependabot and binary artifacts checks * typo * linter * missing errors.go * linter * merge fix * branch protection, frozen-deps, token permissions * linter * linter --- checks/branch_protected.go | 111 ++-- checks/branch_protected_test.go | 797 +++++++++++------------ checks/frozen_deps.go | 212 ++++-- checks/frozen_deps_test.go | 616 ++++++++---------- checks/permissions.go | 68 +- checks/permissions_test.go | 148 ++--- checks/shell_download_validate.go | 65 +- checks/testdata/Dockerfile-not-pinned-as | 6 + e2e/branchprotection_test.go | 21 +- e2e/frozen_deps_test.go | 42 +- e2e/security_policy_test.go | 1 - 11 files changed, 1051 insertions(+), 1036 deletions(-) diff --git a/checks/branch_protected.go b/checks/branch_protected.go index 286eb1b5..73f7d59f 100644 --- a/checks/branch_protected.go +++ b/checks/branch_protected.go @@ -16,18 +16,18 @@ package checks import ( "context" - "errors" + "fmt" "regexp" "github.com/google/go-github/v32/github" "github.com/ossf/scorecard/checker" + sce "github.com/ossf/scorecard/errors" ) const ( - // CheckBranchProtection is the registered name for BranchProtection. CheckBranchProtection = "Branch-Protection" - minReviews = 1 + minReviews = 2 ) //nolint:gochecknoinits @@ -46,31 +46,23 @@ type repositories interface { *github.Protection, *github.Response, error) } -type logger func(s string, f ...interface{}) - -// ErrCommitishNil TargetCommitish nil for release. -var ErrCommitishNil = errors.New("target_commitish is nil for release") - -// ErrBranchNotFound branch from TargetCommitish not found. -var ErrBranchNotFound = errors.New("branch not found") - func BranchProtection(c *checker.CheckRequest) checker.CheckResult { - // Checks branch protection on both release and development branch - return checkReleaseAndDevBranchProtection(c.Ctx, c.Client.Repositories, c.Logf, c.Owner, c.Repo) + // Checks branch protection on both release and development branch. + return checkReleaseAndDevBranchProtection(c.Ctx, c.Client.Repositories, c.Dlogger, c.Owner, c.Repo) } -func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, l logger, ownerStr, +func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, dl checker.DetailLogger, ownerStr, repoStr string) checker.CheckResult { // Get all branches. This will include information on whether they are protected. branches, _, err := r.ListBranches(ctx, ownerStr, repoStr, &github.BranchListOptions{}) if err != nil { - return checker.MakeRetryResult(CheckBranchProtection, err) + return checker.CreateRuntimeErrorResult(CheckBranchProtection, err) } // Get release branches releases, _, err := r.ListReleases(ctx, ownerStr, repoStr, &github.ListOptions{}) if err != nil { - return checker.MakeRetryResult(CheckBranchProtection, err) + return checker.CreateRuntimeErrorResult(CheckBranchProtection, err) } var checks []checker.CheckResult @@ -79,7 +71,9 @@ func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, l l for _, release := range releases { if release.TargetCommitish == nil { // Log with a named error if target_commitish is nil. - checks = append(checks, checker.MakeFailResult(CheckBranchProtection, ErrCommitishNil)) + e := sce.Create(sce.ErrScorecardInternal, errInternalCommitishNil.Error()) + r := checker.CreateRuntimeErrorResult(CheckBranchProtection, e) + checks = append(checks, r) continue } @@ -92,7 +86,9 @@ func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, l l name, err := resolveBranchName(branches, *release.TargetCommitish) if err != nil { // If the commitish branch is still not found, fail. - checks = append(checks, checker.MakeFailResult(CheckBranchProtection, ErrBranchNotFound)) + e := sce.Create(sce.ErrScorecardInternal, errInternalBranchNotFound.Error()) + r := checker.CreateRuntimeErrorResult(CheckBranchProtection, e) + checks = append(checks, r) continue } @@ -100,10 +96,10 @@ func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, l l checkBranches[*name] = true } - // Add default branch + // Add default branch. repo, _, err := r.Get(ctx, ownerStr, repoStr) if err != nil { - return checker.MakeRetryResult(CheckBranchProtection, err) + return checker.CreateRuntimeErrorResult(CheckBranchProtection, err) } checkBranches[*repo.DefaultBranch] = true @@ -111,23 +107,22 @@ func checkReleaseAndDevBranchProtection(ctx context.Context, r repositories, l l for b := range checkBranches { protected, err := isBranchProtected(branches, b) if err != nil { - checks = append(checks, checker.MakeFailResult(CheckBranchProtection, ErrBranchNotFound)) + e := sce.Create(sce.ErrScorecardInternal, errInternalBranchNotFound.Error()) + r := checker.CreateRuntimeErrorResult(CheckBranchProtection, e) + checks = append(checks, r) } if !protected { - l("!! branch protection not enabled for branch %s", b) - checks = append(checks, checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Confidence: checker.MaxResultConfidence, - }) + r := checker.CreateMinScoreResult(CheckBranchProtection, + fmt.Sprintf("branch protection not enabled for branch '%s'", b)) + checks = append(checks, r) } else { // The branch is protected. Check the protection. - res := getProtectionAndCheck(ctx, r, l, ownerStr, repoStr, b) + res := getProtectionAndCheck(ctx, r, dl, ownerStr, repoStr, b) checks = append(checks, res) } } - return checker.MakeAndResult(checks...) + return checker.MakeAndResult2(checks...) } func resolveBranchName(branches []*github.Branch, name string) (*string, error) { @@ -144,7 +139,8 @@ func resolveBranchName(branches []*github.Branch, name string) (*string, error) return resolveBranchName(branches, "main") } - return nil, ErrBranchNotFound + //nolint + return nil, sce.Create(sce.ErrScorecardInternal, errInternalBranchNotFound.Error()) } func isBranchProtected(branches []*github.Branch, name string) (bool, error) { @@ -154,71 +150,79 @@ func isBranchProtected(branches []*github.Branch, name string) (bool, error) { return b.GetProtected(), nil } } - return false, ErrBranchNotFound + //nolint + return false, sce.Create(sce.ErrScorecardInternal, errInternalBranchNotFound.Error()) } -func getProtectionAndCheck(ctx context.Context, r repositories, l logger, ownerStr, repoStr, +func getProtectionAndCheck(ctx context.Context, r repositories, dl checker.DetailLogger, ownerStr, repoStr, branch string) checker.CheckResult { // We only call this if the branch is protected. An error indicates not found. protection, resp, err := r.GetBranchProtection(ctx, ownerStr, repoStr, branch) const fileNotFound = 404 if resp.StatusCode == fileNotFound { - return checker.MakeRetryResult(CheckBranchProtection, err) + e := sce.Create(sce.ErrScorecardInternal, err.Error()) + return checker.CreateRuntimeErrorResult(CheckBranchProtection, e) } - return IsBranchProtected(protection, branch, l) + return IsBranchProtected(protection, branch, dl) } -func IsBranchProtected(protection *github.Protection, branch string, l logger) checker.CheckResult { - totalChecks := 6 +func IsBranchProtected(protection *github.Protection, branch string, dl checker.DetailLogger) checker.CheckResult { + totalChecks := 10 totalSuccess := 0 // This is disabled by default (good). if protection.GetAllowForcePushes() != nil && protection.AllowForcePushes.Enabled { - l("!! branch protection - AllowForcePushes should be disabled on %s", branch) + dl.Warn("AllowForcePushes enabled on branch '%s'", branch) } else { + dl.Info("AllowForcePushes disabled on branch '%s'", branch) totalSuccess++ } // This is disabled by default (good). if protection.GetAllowDeletions() != nil && protection.AllowDeletions.Enabled { - l("!! branch protection - AllowDeletions should be disabled on %s", branch) + dl.Warn("AllowDeletions enabled on branch '%s'", branch) } else { + dl.Info("AllowDeletions disabled on branch '%s'", branch) totalSuccess++ } // This is disabled by default (bad). if protection.GetEnforceAdmins() != nil && protection.EnforceAdmins.Enabled { + dl.Info("EnforceAdmins disabled on branch '%s'", branch) totalSuccess++ } else { - l("!! branch protection - EnforceAdmins should be enabled on %s", branch) + dl.Warn("EnforceAdmins disabled on branch '%s'", branch) } // This is disabled by default (bad). if protection.GetRequireLinearHistory() != nil && protection.RequireLinearHistory.Enabled { + dl.Info("Linear history enabled on branch '%s'", branch) totalSuccess++ } else { - l("!! branch protection - Linear history should be enabled on %s", branch) + dl.Warn("Linear history disabled on branch '%s'", branch) } - if requiresStatusChecks(protection, branch, l) { + if requiresStatusChecks(protection, branch, dl) { + dl.Info("Strict status check enabled on branch '%s'", branch) totalSuccess++ } - if requiresThoroughReviews(protection, branch, l) { + if requiresThoroughReviews(protection, branch, dl) { totalSuccess++ } - return checker.MakeProportionalResult(CheckBranchProtection, totalSuccess, totalChecks, 1.0) + return checker.CreateProportionalScoreResult(CheckBranchProtection, + "%d out of %d branch protection settings are enabled", totalSuccess, totalChecks) } // Returns true if several PR status checks requirements are enabled. Otherwise returns false and logs why it failed. -func requiresStatusChecks(protection *github.Protection, branch string, l logger) bool { +func requiresStatusChecks(protection *github.Protection, branch string, dl checker.DetailLogger) bool { // This is disabled by default (bad). if protection.GetRequiredStatusChecks() != nil && protection.RequiredStatusChecks.Strict && @@ -228,17 +232,17 @@ func requiresStatusChecks(protection *github.Protection, branch string, l logger switch { case protection.RequiredStatusChecks == nil || !protection.RequiredStatusChecks.Strict: - l("!! branch protection - Status checks for merging should be enabled on %s", branch) + dl.Warn("Status checks for merging disabled on branch '%s'", branch) case len(protection.RequiredStatusChecks.Contexts) == 0: - l("!! branch protection - Status checks for merging should have specific status to check for on %s", branch) + dl.Warn("Status checks for merging have no specific status to check on branch '%s'", branch) default: - panic("!! branch protection - Unhandled status checks error") + panic("Unhandled status checks error") } return false } // Returns true if several PR review requirements are enabled. Otherwise returns false and logs why it failed. -func requiresThoroughReviews(protection *github.Protection, branch string, l logger) bool { +func requiresThoroughReviews(protection *github.Protection, branch string, dl checker.DetailLogger) bool { // This is disabled by default (bad). if protection.GetRequiredPullRequestReviews() != nil && protection.RequiredPullRequestReviews.RequiredApprovingReviewCount >= minReviews && @@ -248,18 +252,19 @@ func requiresThoroughReviews(protection *github.Protection, branch string, l log } switch { case protection.RequiredPullRequestReviews == nil: - l("!! branch protection - Pullrequest reviews should be enabled on %s", branch) + dl.Warn("Pullrequest reviews disabled on branch '%s'", branch) fallthrough case protection.RequiredPullRequestReviews.RequiredApprovingReviewCount < minReviews: - l("!! branch protection - %v pullrequest reviews should be enabled on %s", minReviews, branch) + dl.Warn("Number of required reviewers is only %d on branch '%s'", + protection.RequiredPullRequestReviews.RequiredApprovingReviewCount, branch) fallthrough case !protection.RequiredPullRequestReviews.DismissStaleReviews: - l("!! branch protection - Stale review dismissal should be enabled on %s", branch) + dl.Warn("Stale review dismissal disabled on branch '%s'", branch) fallthrough case !protection.RequiredPullRequestReviews.RequireCodeOwnerReviews: - l("!! branch protection - Owner review should be enabled on %s", branch) + dl.Warn("Owner review not required on branch '%s'", branch) default: - panic("!! branch protection - Unhandled pull request error") + panic("Unhandled pull request error") } return false } diff --git a/checks/branch_protected_test.go b/checks/branch_protected_test.go index 106fdc22..8778f062 100644 --- a/checks/branch_protected_test.go +++ b/checks/branch_protected_test.go @@ -16,24 +16,16 @@ package checks import ( "context" - "fmt" "net/http" "testing" "github.com/google/go-github/v32/github" "github.com/ossf/scorecard/checker" + sce "github.com/ossf/scorecard/errors" + scut "github.com/ossf/scorecard/utests" ) -// TODO: these logging functions are repeated from lib/check_fn.go. Reuse code. -type log struct { - messages []string -} - -func (l *log) Logf(s string, f ...interface{}) { - l.messages = append(l.messages, fmt.Sprintf(s, f...)) -} - type mockRepos struct { branches []*string protections map[string]*github.Protection @@ -68,7 +60,8 @@ func (m mockRepos) GetBranchProtection(ctx context.Context, o string, r string, return nil, &github.Response{ Response: &http.Response{StatusCode: http.StatusNotFound}, }, - ErrBranchNotFound + //nolint + sce.Create(sce.ErrScorecardInternal, errInternalBranchNotFound.Error()) } func (m mockRepos) ListBranches(ctx context.Context, owner string, repo string, @@ -81,9 +74,8 @@ func (m mockRepos) ListBranches(ctx context.Context, owner string, repo string, return res, nil, nil } -func TestReleaseAndDevBranchProtected(t *testing.T) { //nolint:tparallel // mocks return different results per test case +func TestReleaseAndDevBranchProtected(t *testing.T) { t.Parallel() - l := log{} rel1 := "release/v.1" sha := "8fb3cb86082b17144a80402f5367ae65f06083bd" @@ -91,14 +83,21 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { //nolint:tparallel // mock //nolint tests := []struct { name string + expected scut.TestReturn branches []*string defaultBranch *string releases []*string protections map[string]*github.Protection - want checker.CheckResult }{ { - name: "Only development branch", + name: "Only development branch", + expected: scut.TestReturn{ + Errors: nil, + Score: 2, + NumberOfWarn: 6, + NumberOfInfo: 2, + NumberOfDebug: 0, + }, defaultBranch: &main, branches: []*string{&rel1, &main}, releases: nil, @@ -137,17 +136,16 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { //nolint:tparallel // mock }, }, }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 7, - ShouldRetry: false, - Error: nil, - }, }, { - name: "Take worst of release and development", + name: "Take worst of release and development", + expected: scut.TestReturn{ + Errors: nil, + Score: 2, + NumberOfWarn: 9, + NumberOfInfo: 7, + NumberOfDebug: 0, + }, defaultBranch: &main, branches: []*string{&rel1, &main}, releases: []*string{&rel1}, @@ -219,17 +217,16 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { //nolint:tparallel // mock }, }, }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 7, - ShouldRetry: false, - Error: nil, - }, }, { - name: "Both release and development are OK", + name: "Both release and development are OK", + expected: scut.TestReturn{ + Errors: nil, + Score: 5, + NumberOfWarn: 6, + NumberOfInfo: 10, + NumberOfDebug: 0, + }, defaultBranch: &main, branches: []*string{&rel1, &main}, releases: []*string{&rel1}, @@ -301,17 +298,16 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { //nolint:tparallel // mock }, }, }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: true, - Details: nil, - Confidence: 10, - ShouldRetry: false, - Error: nil, - }, }, { - name: "Ignore a non-branch targetcommitish", + name: "Ignore a non-branch targetcommitish", + expected: scut.TestReturn{ + Errors: nil, + Score: 2, + NumberOfWarn: 6, + NumberOfInfo: 2, + NumberOfDebug: 0, + }, defaultBranch: &main, branches: []*string{&rel1, &main}, releases: []*string{&sha}, @@ -350,17 +346,16 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { //nolint:tparallel // mock }, }, }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 7, - ShouldRetry: false, - Error: nil, - }, }, { - name: "TargetCommittish nil", + name: "TargetCommittish nil", + expected: scut.TestReturn{ + Errors: []error{sce.ErrScorecardInternal}, + Score: checker.InconclusiveResultScore, + NumberOfWarn: 6, + NumberOfInfo: 2, + NumberOfDebug: 0, + }, defaultBranch: &main, branches: []*string{&main}, releases: []*string{nil}, @@ -399,476 +394,430 @@ func TestReleaseAndDevBranchProtected(t *testing.T) { //nolint:tparallel // mock }, }, }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 10, - ShouldRetry: false, - Error: ErrCommitishNil, - }, }, } - for _, tt := range tests { //nolint:paralleltest // mocks return different results per test case + for _, tt := range tests { tt := tt // Re-initializing variable so it is not changed while executing the closure below - l.messages = []string{} - t.Run(tt.name, func(t *testing.T) { + t.Parallel() m := mockRepos{ defaultBranch: tt.defaultBranch, branches: tt.branches, releases: tt.releases, protections: tt.protections, } - got := checkReleaseAndDevBranchProtection(context.Background(), m, - l.Logf, "testowner", "testrepo") - got.Details = l.messages - if got.Confidence != tt.want.Confidence || got.Pass != tt.want.Pass { - t.Errorf("IsBranchProtected() = %s, %v, want %v", tt.name, got, tt.want) - } + dl := scut.TestDetailLogger{} + r := checkReleaseAndDevBranchProtection(context.Background(), m, + &dl, "testowner", "testrepo") + scut.ValidateTestReturn(t, tt.name, &tt.expected, &r, &dl) }) } } func TestIsBranchProtected(t *testing.T) { t.Parallel() - type args struct { - protection *github.Protection - } - l := log{} tests := []struct { - name string - args args - want checker.CheckResult + name string + protection *github.Protection + expected scut.TestReturn }{ { name: "Nothing is enabled", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: false, - Contexts: nil, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: false, - RequireCodeOwnerReviews: false, - RequiredApprovingReviewCount: 0, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: false, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 2, + NumberOfWarn: 6, + NumberOfInfo: 2, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: false, + Contexts: nil, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: false, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: false, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: false, }, + DismissStaleReviews: false, + RequireCodeOwnerReviews: false, + RequiredApprovingReviewCount: 0, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: false, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: false, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: false, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: false, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 7, - ShouldRetry: false, - Error: nil, }, }, { name: "Required status check enabled", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: true, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: false, - RequireCodeOwnerReviews: false, - RequiredApprovingReviewCount: 0, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: false, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 3, + NumberOfWarn: 5, + NumberOfInfo: 3, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: true, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: false, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: false, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: false, }, + DismissStaleReviews: false, + RequireCodeOwnerReviews: false, + RequiredApprovingReviewCount: 0, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: false, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: false, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: false, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: false, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 5, - ShouldRetry: false, - Error: nil, }, }, { name: "Required status check enabled without checking for status string", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: true, - Contexts: nil, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: false, - RequireCodeOwnerReviews: false, - RequiredApprovingReviewCount: 0, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: false, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 2, + NumberOfWarn: 6, + NumberOfInfo: 2, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: true, + Contexts: nil, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: false, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: false, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: false, }, + DismissStaleReviews: false, + RequireCodeOwnerReviews: false, + RequiredApprovingReviewCount: 0, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: false, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: false, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: false, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: false, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 7, - ShouldRetry: false, - Error: nil, }, }, - { name: "Required pull request enabled", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: false, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: false, - RequireCodeOwnerReviews: false, - RequiredApprovingReviewCount: 1, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: false, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 3, + NumberOfWarn: 5, + NumberOfInfo: 3, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: false, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: true, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: false, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: false, }, + DismissStaleReviews: false, + RequireCodeOwnerReviews: false, + RequiredApprovingReviewCount: 1, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: false, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: true, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: false, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: false, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 5, - ShouldRetry: false, - Error: nil, }, }, { name: "Required admin enforcement enabled", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: false, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: false, - RequireCodeOwnerReviews: false, - RequiredApprovingReviewCount: 0, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: true, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 3, + NumberOfWarn: 5, + NumberOfInfo: 3, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: false, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: false, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: false, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: false, }, + DismissStaleReviews: false, + RequireCodeOwnerReviews: false, + RequiredApprovingReviewCount: 0, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: true, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: false, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: false, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: false, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 5, - ShouldRetry: false, - Error: nil, }, }, { name: "Required linear history enabled", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: false, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: false, - RequireCodeOwnerReviews: false, - RequiredApprovingReviewCount: 0, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: false, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 3, + NumberOfWarn: 5, + NumberOfInfo: 3, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: false, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: true, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: false, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: false, }, + DismissStaleReviews: false, + RequireCodeOwnerReviews: false, + RequiredApprovingReviewCount: 0, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: false, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: true, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: false, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: false, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 5, - ShouldRetry: false, - Error: nil, }, }, { name: "Allow force push enabled", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: false, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: false, - RequireCodeOwnerReviews: false, - RequiredApprovingReviewCount: 0, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: false, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 1, + NumberOfWarn: 7, + NumberOfInfo: 1, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: false, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: false, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: true, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: false, }, + DismissStaleReviews: false, + RequireCodeOwnerReviews: false, + RequiredApprovingReviewCount: 0, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: false, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: false, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: true, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: false, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 9, - ShouldRetry: false, - Error: nil, }, }, { name: "Allow deletions enabled", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: false, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: false, - RequireCodeOwnerReviews: false, - RequiredApprovingReviewCount: 0, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: false, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 1, + NumberOfWarn: 7, + NumberOfInfo: 1, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: false, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: false, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: false, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: true, }, + DismissStaleReviews: false, + RequireCodeOwnerReviews: false, + RequiredApprovingReviewCount: 0, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: false, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: false, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: false, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: true, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: false, - Details: nil, - Confidence: 9, - ShouldRetry: false, - Error: nil, }, }, { name: "Branches are protected", - args: args{ - protection: &github.Protection{ - RequiredStatusChecks: &github.RequiredStatusChecks{ - Strict: true, - Contexts: []string{"foo"}, - }, - RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ - DismissalRestrictions: &github.DismissalRestrictions{ - Users: nil, - Teams: nil, - }, - DismissStaleReviews: true, - RequireCodeOwnerReviews: true, - RequiredApprovingReviewCount: 1, - }, - EnforceAdmins: &github.AdminEnforcement{ - URL: nil, - Enabled: true, - }, - Restrictions: &github.BranchRestrictions{ + expected: scut.TestReturn{ + Errors: nil, + Score: 5, + NumberOfWarn: 3, + NumberOfInfo: 5, + NumberOfDebug: 0, + }, + protection: &github.Protection{ + RequiredStatusChecks: &github.RequiredStatusChecks{ + Strict: true, + Contexts: []string{"foo"}, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + DismissalRestrictions: &github.DismissalRestrictions{ Users: nil, Teams: nil, - Apps: nil, - }, - RequireLinearHistory: &github.RequireLinearHistory{ - Enabled: true, - }, - AllowForcePushes: &github.AllowForcePushes{ - Enabled: false, - }, - AllowDeletions: &github.AllowDeletions{ - Enabled: false, }, + DismissStaleReviews: true, + RequireCodeOwnerReviews: true, + RequiredApprovingReviewCount: 1, + }, + EnforceAdmins: &github.AdminEnforcement{ + URL: nil, + Enabled: true, + }, + Restrictions: &github.BranchRestrictions{ + Users: nil, + Teams: nil, + Apps: nil, + }, + RequireLinearHistory: &github.RequireLinearHistory{ + Enabled: true, + }, + AllowForcePushes: &github.AllowForcePushes{ + Enabled: false, + }, + AllowDeletions: &github.AllowDeletions{ + Enabled: false, }, - }, - want: checker.CheckResult{ - Name: CheckBranchProtection, - Pass: true, - Details: nil, - Confidence: 10, - ShouldRetry: false, - Error: nil, }, }, } for _, tt := range tests { tt := tt // Re-initializing variable so it is not changed while executing the closure below - l.messages = []string{} t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := IsBranchProtected(tt.args.protection, "test", l.Logf) - got.Details = l.messages - if got.Confidence != tt.want.Confidence || got.Pass != tt.want.Pass { - t.Errorf("IsBranchProtected() = %s, %v, want %v", tt.name, got, tt.want) - } + dl := scut.TestDetailLogger{} + r := IsBranchProtected(tt.protection, "test", &dl) + scut.ValidateTestReturn(t, tt.name, &tt.expected, &r, &dl) }) } } diff --git a/checks/frozen_deps.go b/checks/frozen_deps.go index 30c2ecfc..bc83d17e 100644 --- a/checks/frozen_deps.go +++ b/checks/frozen_deps.go @@ -15,7 +15,6 @@ package checks import ( - "errors" "fmt" "regexp" "strings" @@ -24,17 +23,12 @@ import ( "gopkg.in/yaml.v2" "github.com/ossf/scorecard/checker" + sce "github.com/ossf/scorecard/errors" ) // CheckFrozenDeps is the registered name for FrozenDeps. const CheckFrozenDeps = "Frozen-Deps" -// ErrInvalidDockerfile : Invalid docker file. -var ErrInvalidDockerfile = errors.New("invalid docker file") - -// ErrEmptyFile : Invalid docker file. -var ErrEmptyFile = errors.New("file has no content") - // Structure for workflow config. // We only declare the fields we need. // Github workflows format: https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions @@ -65,7 +59,7 @@ func init() { // FrozenDeps will check the repository if it contains frozen dependecies. func FrozenDeps(c *checker.CheckRequest) checker.CheckResult { - return checker.MultiCheckAnd( + return checker.MultiCheckAnd2( isPackageManagerLockFilePresent, isGitHubActionsWorkflowPinned, isDockerfilePinned, @@ -78,28 +72,70 @@ func FrozenDeps(c *checker.CheckRequest) checker.CheckResult { // TODO(laurent): need to support GCB pinning. func isShellScriptFreeOfInsecureDownloads(c *checker.CheckRequest) checker.CheckResult { - return CheckFilesContent(CheckFrozenDeps, "*", false, c, validateShellScriptDownloads) + r, err := CheckFilesContent2("*", false, c, validateShellScriptIsFreeOfInsecureDownloads) + return createResultForIsShellScriptFreeOfInsecureDownloads(r, err) } -func validateShellScriptDownloads(pathfn string, content []byte, - logf func(s string, f ...interface{})) (bool, error) { +func createResultForIsShellScriptFreeOfInsecureDownloads(r bool, err error) checker.CheckResult { + if err != nil { + return checker.CreateRuntimeErrorResult(CheckFrozenDeps, err) + } + if !r { + return checker.CreateMinScoreResult(CheckFrozenDeps, + "insecure (unpinned) dependency downloads found in shell scripts") + } + + return checker.CreateMaxScoreResult(CheckFrozenDeps, + "no insecure (unpinned) dependency downloads found in shell scripts") +} + +func testValidateShellScriptIsFreeOfInsecureDownloads(pathfn string, + content []byte, dl checker.DetailLogger) checker.CheckResult { + r, err := validateShellScriptIsFreeOfInsecureDownloads(pathfn, content, dl) + return createResultForIsShellScriptFreeOfInsecureDownloads(r, err) +} + +func validateShellScriptIsFreeOfInsecureDownloads(pathfn string, content []byte, + dl checker.DetailLogger) (bool, error) { // Validate the file type. if !isShellScriptFile(pathfn, content) { return true, nil } - return validateShellFile(pathfn, content, logf) + return validateShellFile(pathfn, content, dl) } func isDockerfileFreeOfInsecureDownloads(c *checker.CheckRequest) checker.CheckResult { - return CheckFilesContent(CheckFrozenDeps, "*Dockerfile*", false, c, validateDockerfileDownloads) + r, err := CheckFilesContent2("*Dockerfile*", false, c, validateDockerfileIsFreeOfInsecureDownloads) + return createResultForIsDockerfileFreeOfInsecureDownloads(r, err) } -func validateDockerfileDownloads(pathfn string, content []byte, - logf func(s string, f ...interface{})) (bool, error) { +// Create the result. +func createResultForIsDockerfileFreeOfInsecureDownloads(r bool, err error) checker.CheckResult { + if err != nil { + return checker.CreateRuntimeErrorResult(CheckFrozenDeps, err) + } + if !r { + return checker.CreateMinScoreResult(CheckFrozenDeps, + "insecure (unpinned) dependency downloads found in Dockerfiles") + } + + return checker.CreateMaxScoreResult(CheckFrozenDeps, + "no insecure (unpinned) dependency downloads found in Dockerfiles") +} + +func testValidateDockerfileIsFreeOfInsecureDownloads(pathfn string, + content []byte, dl checker.DetailLogger) checker.CheckResult { + r, err := validateDockerfileIsFreeOfInsecureDownloads(pathfn, content, dl) + return createResultForIsDockerfileFreeOfInsecureDownloads(r, err) +} + +func validateDockerfileIsFreeOfInsecureDownloads(pathfn string, content []byte, + dl checker.DetailLogger) (bool, error) { contentReader := strings.NewReader(string(content)) res, err := parser.Parse(contentReader) if err != nil { - return false, fmt.Errorf("cannot read dockerfile content: %w", err) + //nolint + return false, sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("%v: %v", errInternalInvalidDockerFile, err)) } // nolint: prealloc @@ -119,7 +155,8 @@ func validateDockerfileDownloads(pathfn string, content []byte, } if len(valueList) == 0 { - return false, ErrParsingDockerfile + //nolint + return false, sce.Create(sce.ErrScorecardInternal, errInternalInvalidDockerFile.Error()) } // Build a file content. @@ -127,15 +164,33 @@ func validateDockerfileDownloads(pathfn string, content []byte, bytes = append(bytes, cmd...) bytes = append(bytes, '\n') } - return validateShellFile(pathfn, bytes, logf) + return validateShellFile(pathfn, bytes, dl) } func isDockerfilePinned(c *checker.CheckRequest) checker.CheckResult { - return CheckFilesContent(CheckFrozenDeps, "*Dockerfile*", false, c, validateDockerfile) + r, err := CheckFilesContent2("*Dockerfile*", false, c, validateDockerfileIsPinned) + return createResultForIsDockerfilePinned(r, err) } -func validateDockerfile(pathfn string, content []byte, - logf func(s string, f ...interface{})) (bool, error) { +// Create the result. +func createResultForIsDockerfilePinned(r bool, err error) checker.CheckResult { + if err != nil { + return checker.CreateRuntimeErrorResult(CheckFrozenDeps, err) + } + if r { + return checker.CreateMaxScoreResult(CheckFrozenDeps, "Dockerfile dependencies are pinned") + } + + return checker.CreateMinScoreResult(CheckFrozenDeps, "unpinned dependencies found Dockerfiles") +} + +func testValidateDockerfileIsPinned(pathfn string, content []byte, dl checker.DetailLogger) checker.CheckResult { + r, err := validateDockerfileIsPinned(pathfn, content, dl) + return createResultForIsDockerfilePinned(r, err) +} + +func validateDockerfileIsPinned(pathfn string, content []byte, + dl checker.DetailLogger) (bool, error) { // Users may use various names, e.g., // Dockerfile.aarch64, Dockerfile.template, Dockerfile_template, dockerfile, Dockerfile-name.template // Templates may trigger false positives, e.g. FROM { NAME }. @@ -150,7 +205,8 @@ func validateDockerfile(pathfn string, content []byte, pinnedAsNames := make(map[string]bool) res, err := parser.Parse(contentReader) if err != nil { - return false, fmt.Errorf("cannot read dockerfile content: %w", err) + //nolint + return false, sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("%v: %v", errInternalInvalidDockerFile, err)) } for _, child := range res.AST.Children { @@ -188,44 +244,70 @@ func validateDockerfile(pathfn string, content []byte, // Not pinned. ret = false - logf("!! frozen-deps/docker - %v has non-pinned dependency '%v'", pathfn, name) + dl.Warn("unpinned dependency detected in %v: '%v'", pathfn, name) // FROM name. case len(valueList) == 1: name := valueList[0] if !regex.Match([]byte(name)) { ret = false - logf("!! frozen-deps/docker - %v has non-pinned dependency '%v'", pathfn, name) + dl.Warn("unpinned dependency detected in %v: '%v'", pathfn, name) } default: // That should not happen. - return false, ErrInvalidDockerfile + //nolint + return false, sce.Create(sce.ErrScorecardInternal, errInternalInvalidDockerFile.Error()) } } // The file should have at least one FROM statement. if !fromFound { - return false, ErrInvalidDockerfile + //nolint + return false, sce.Create(sce.ErrScorecardInternal, errInternalInvalidDockerFile.Error()) } return ret, nil } func isGitHubWorkflowScriptFreeOfInsecureDownloads(c *checker.CheckRequest) checker.CheckResult { - return CheckFilesContent(CheckFrozenDeps, ".github/workflows/*", false, c, validateGitHubWorkflowShellScriptDownloads) + r, err := CheckFilesContent2(".github/workflows/*", false, c, validateGitHubWorkflowIsFreeOfInsecureDownloads) + return createResultForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r, err) } -func validateGitHubWorkflowShellScriptDownloads(pathfn string, content []byte, - logf func(s string, f ...interface{})) (bool, error) { +// Create the result. +func createResultForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r bool, err error) checker.CheckResult { + if err != nil { + return checker.CreateRuntimeErrorResult(CheckFrozenDeps, err) + } + if !r { + return checker.CreateMinScoreResult(CheckFrozenDeps, + "insecure (unpinned) dependency downloads found in GitHub workflows") + } + + return checker.CreateMaxScoreResult(CheckFrozenDeps, + "no insecure (unpinned) dependency downloads found in GitHub workflows") +} + +func testValidateGitHubWorkflowScriptFreeOfInsecureDownloads(pathfn string, + content []byte, dl checker.DetailLogger) checker.CheckResult { + r, err := validateGitHubWorkflowIsFreeOfInsecureDownloads(pathfn, content, dl) + return createResultForIsGitHubWorkflowScriptFreeOfInsecureDownloads(r, err) +} + +func validateGitHubWorkflowIsFreeOfInsecureDownloads(pathfn string, content []byte, + dl checker.DetailLogger) (bool, error) { if len(content) == 0 { - return false, ErrEmptyFile + //nolint + return false, sce.Create(sce.ErrScorecardInternal, errInternalEmptyFile.Error()) } var workflow gitHubActionWorkflowConfig err := yaml.Unmarshal(content, &workflow) if err != nil { - return false, fmt.Errorf("!! frozen-deps - cannot unmarshal file %v\n%v: %w", pathfn, string(content), err) + //nolint + return false, sce.Create(sce.ErrScorecardInternal, + fmt.Sprintf("%v:%s:%s:%v", errInternalInvalidYamlFile, pathfn, string(content), err)) } githubVarRegex := regexp.MustCompile(`{{[^{}]*}}`) @@ -262,7 +344,7 @@ func validateGitHubWorkflowShellScriptDownloads(pathfn string, content []byte, } if scriptContent != "" { - validated, err = validateShellFile(pathfn, []byte(scriptContent), logf) + validated, err = validateShellFile(pathfn, []byte(scriptContent), dl) if err != nil { return false, err } @@ -273,19 +355,40 @@ func validateGitHubWorkflowShellScriptDownloads(pathfn string, content []byte, // Check pinning of github actions in workflows. func isGitHubActionsWorkflowPinned(c *checker.CheckRequest) checker.CheckResult { - return CheckFilesContent(CheckFrozenDeps, ".github/workflows/*", true, c, validateGitHubActionWorkflow) + r, err := CheckFilesContent2(".github/workflows/*", true, c, validateGitHubActionWorkflow) + return createResultForIsGitHubActionsWorkflowPinned(r, err) +} + +// Create the result. +func createResultForIsGitHubActionsWorkflowPinned(r bool, err error) checker.CheckResult { + if err != nil { + return checker.CreateRuntimeErrorResult(CheckFrozenDeps, err) + } + if r { + return checker.CreateMaxScoreResult(CheckFrozenDeps, "GitHub actions are pinned") + } + + return checker.CreateMinScoreResult(CheckFrozenDeps, "GitHub actions are not pinned") +} + +func testIsGitHubActionsWorkflowPinned(pathfn string, content []byte, dl checker.DetailLogger) checker.CheckResult { + r, err := validateGitHubActionWorkflow(pathfn, content, dl) + return createResultForIsGitHubActionsWorkflowPinned(r, err) } // Check file content. -func validateGitHubActionWorkflow(pathfn string, content []byte, logf func(s string, f ...interface{})) (bool, error) { +func validateGitHubActionWorkflow(pathfn string, content []byte, dl checker.DetailLogger) (bool, error) { if len(content) == 0 { - return false, ErrEmptyFile + //nolint + return false, sce.Create(sce.ErrScorecardInternal, errInternalEmptyFile.Error()) } var workflow gitHubActionWorkflowConfig err := yaml.Unmarshal(content, &workflow) if err != nil { - return false, fmt.Errorf("!! frozen-deps - cannot unmarshal file %v\n%v: %w", pathfn, string(content), err) + //nolint + return false, sce.Create(sce.ErrScorecardInternal, + fmt.Sprintf("%v:%s:%s:%v", errInternalInvalidYamlFile, pathfn, string(content), err)) } hashRegex := regexp.MustCompile(`^.*@[a-f\d]{40,}`) @@ -301,7 +404,7 @@ func validateGitHubActionWorkflow(pathfn string, content []byte, logf func(s str match := hashRegex.Match([]byte(step.Uses)) if !match { ret = false - logf("!! frozen-deps/github-actions - %v has non-pinned dependency '%v' (job '%v')", pathfn, step.Uses, jobName) + dl.Warn("unpinned dependency detected in %v: '%v' (job '%v')", pathfn, step.Uses, jobName) } } } @@ -312,38 +415,49 @@ func validateGitHubActionWorkflow(pathfn string, content []byte, logf func(s str // Check presence of lock files thru validatePackageManagerFile(). func isPackageManagerLockFilePresent(c *checker.CheckRequest) checker.CheckResult { - return CheckIfFileExists(CheckFrozenDeps, c, validatePackageManagerFile) + r, err := CheckIfFileExists2(CheckFrozenDeps, c, validatePackageManagerFile) + if err != nil { + return checker.CreateRuntimeErrorResult(CheckFrozenDeps, err) + } + if !r { + return checker.CreateInconclusiveResult(CheckFrozenDeps, "no lock files detected for a package manager") + } + + return checker.CreateMaxScoreResult(CheckFrozenDeps, "lock file detected for a package manager") } // validatePackageManagerFile will validate the if frozen dependecies file name exists. // TODO(laurent): need to differentiate between libraries and programs. // TODO(laurent): handle multi-language repos. -func validatePackageManagerFile(name string, logf func(s string, f ...interface{})) (bool, error) { +func validatePackageManagerFile(name string, dl checker.DetailLogger) (bool, error) { switch strings.ToLower(name) { - case "go.mod", "go.sum": - logf("go modules found: %s", name) + // TODO(laurent): "go.mod" is for libraries + case "go.sum": + dl.Info("go lock file detected: %s", name) return true, nil case "vendor/", "third_party/", "third-party/": - logf("vendor dir found: %s", name) + dl.Info("vendoring detected in: %s", name) return true, nil case "package-lock.json", "npm-shrinkwrap.json": - logf("nodejs packages found: %s", name) + dl.Info("javascript lock file detected: %s", name) return true, nil // TODO(laurent): add check for hashbased pinning in requirements.txt - https://davidwalsh.name/hashin - case "requirements.txt", "pipfile.lock": - logf("python requirements found: %s", name) + // Note: because requirements.txt does not handle transitive dependencies, we consider it + // not a lock file, until we have remediation steps for pip-build. + case "pipfile.lock": + dl.Info("python lock file detected: %s", name) return true, nil case "gemfile.lock": - logf("ruby gems found: %s", name) + dl.Info("ruby lock file detected: %s", name) return true, nil case "cargo.lock": - logf("rust crates found: %s", name) + dl.Info("rust lock file detected: %s", name) return true, nil case "yarn.lock": - logf("yarn packages found: %s", name) + dl.Info("yarn lock file detected: %s", name) return true, nil case "composer.lock": - logf("composer packages found: %s", name) + dl.Info("composer lock file detected: %s", name) return true, nil default: return false, nil diff --git a/checks/frozen_deps_test.go b/checks/frozen_deps_test.go index b4c22114..b0306581 100644 --- a/checks/frozen_deps_test.go +++ b/checks/frozen_deps_test.go @@ -15,525 +15,421 @@ package checks import ( - "errors" "fmt" "io/ioutil" - "strings" "testing" + + "github.com/ossf/scorecard/checker" + sce "github.com/ossf/scorecard/errors" + scut "github.com/ossf/scorecard/utests" ) func TestGithubWorkflowPinning(t *testing.T) { t.Parallel() - //nolint - type args struct { - Log log - Filename string - } - type returnValue struct { - Error error - Result bool - NumberOfErrors int - } - - //nolint tests := []struct { - args args - want returnValue - name string + name string + filename string + expected scut.TestReturn }{ { - name: "Zero size content", - args: args{ - Filename: "", - Log: log{}, - }, - want: returnValue{ - Error: ErrEmptyFile, - Result: false, - NumberOfErrors: 0, + name: "Zero size content", + filename: "", + expected: scut.TestReturn{ + Errors: []error{sce.ErrScorecardInternal}, + Score: checker.InconclusiveResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "Pinned workflow", - args: args{ - Filename: "./testdata/workflow-pinned.yaml", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: true, - NumberOfErrors: 0, + name: "Pinned workflow", + filename: "./testdata/workflow-pinned.yaml", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "Non-pinned workflow", - args: args{ - Filename: "./testdata/workflow-not-pinned.yaml", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 1, + name: "Non-pinned workflow", + filename: "./testdata/workflow-not-pinned.yaml", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 1, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, } - //nolint 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() var content []byte var err error - if tt.args.Filename == "" { + if tt.filename == "" { content = make([]byte, 0) } else { - content, err = ioutil.ReadFile(tt.args.Filename) + content, err = ioutil.ReadFile(tt.filename) if err != nil { panic(fmt.Errorf("cannot read file: %w", err)) } } - r, err := validateGitHubActionWorkflow(tt.args.Filename, content, tt.args.Log.Logf) - - if !errors.Is(err, tt.want.Error) || - r != tt.want.Result || - len(tt.args.Log.messages) != tt.want.NumberOfErrors { - t.Errorf("TestDockerfileScriptDownload:\"%v\": %v (%v,%v,%v) want (%v, %v, %v)\n%v", - tt.name, tt.args.Filename, r, err, len(tt.args.Log.messages), tt.want.Result, tt.want.Error, tt.want.NumberOfErrors, - strings.Join(tt.args.Log.messages, "\n")) - } + dl := scut.TestDetailLogger{} + r := testIsGitHubActionsWorkflowPinned(tt.filename, content, &dl) + scut.ValidateTestReturn(t, tt.name, &tt.expected, &r, &dl) }) } } func TestDockerfilePinning(t *testing.T) { t.Parallel() - type args struct { - Logf func(s string, f ...interface{}) - Filename string - } - - type returnValue struct { - Error error - Result bool - } - - l := log{} tests := []struct { - args args - want returnValue - name string + name string + filename string + expected scut.TestReturn }{ { - name: "Invalid dockerfile", - args: args{ - Filename: "./testdata/Dockerfile-invalid", - Logf: l.Logf, - }, - want: returnValue{ - Error: ErrInvalidDockerfile, - Result: false, + name: "Invalid dockerfile", + filename: "./testdata/Dockerfile-invalid", + expected: scut.TestReturn{ + Errors: []error{sce.ErrScorecardInternal}, + Score: checker.InconclusiveResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "Pinned dockerfile", - args: args{ - Filename: "./testdata/Dockerfile-pinned", - Logf: l.Logf, - }, - want: returnValue{ - Error: nil, - Result: true, + name: "Pinned dockerfile", + filename: "./testdata/Dockerfile-pinned", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "Pinned dockerfile as", - args: args{ - Filename: "./testdata/Dockerfile-pinned-as", - Logf: l.Logf, - }, - want: returnValue{ - Error: nil, - Result: true, + name: "Pinned dockerfile as", + filename: "./testdata/Dockerfile-pinned-as", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "Non-pinned dockerfile as", - args: args{ - Filename: "./testdata/Dockerfile-not-pinned-as", - Logf: l.Logf, - }, - want: returnValue{ - Error: nil, - Result: false, + name: "Non-pinned dockerfile as", + filename: "./testdata/Dockerfile-not-pinned-as", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 3, // TODO: should be 2, https://github.com/ossf/scorecard/issues/701. + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "Non-pinned dockerfile", - args: args{ - Filename: "./testdata/Dockerfile-not-pinned", - Logf: l.Logf, - }, - want: returnValue{ - Error: nil, - Result: false, + name: "Non-pinned dockerfile", + filename: "./testdata/Dockerfile-not-pinned", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 1, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, } for _, tt := range tests { tt := tt // Re-initializing variable so it is not changed while executing the closure below - l.messages = []string{} t.Run(tt.name, func(t *testing.T) { t.Parallel() var content []byte var err error - - content, err = ioutil.ReadFile(tt.args.Filename) - if err != nil { - panic(fmt.Errorf("cannot read file: %w", err)) - } - - r, err := validateDockerfile(tt.args.Filename, content, tt.args.Logf) - - if !errors.Is(err, tt.want.Error) || - r != tt.want.Result { - t.Errorf("TestGithubWorkflowPinning:\"%v\": %v (%v,%v) want (%v, %v)", - tt.name, tt.args.Filename, r, err, tt.want.Result, tt.want.Error) + if tt.filename == "" { + content = make([]byte, 0) + } else { + content, err = ioutil.ReadFile(tt.filename) + if err != nil { + panic(fmt.Errorf("cannot read file: %w", err)) + } } + dl := scut.TestDetailLogger{} + r := testValidateDockerfileIsPinned(tt.filename, content, &dl) + scut.ValidateTestReturn(t, tt.name, &tt.expected, &r, &dl) }) } } func TestDockerfileScriptDownload(t *testing.T) { t.Parallel() - //nolint - type args struct { - // Note: this seems to be defined in e2e/e2e_suite_test.go - Log log - Filename string - } - - type returnValue struct { - Error error - Result bool - NumberOfErrors int - } - - //nolint tests := []struct { - args args - want returnValue - name string + name string + filename string + expected scut.TestReturn }{ { - name: "curl | sh", - args: args{ - Filename: "testdata/Dockerfile-curl-sh", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 4, + name: "curl | sh", + filename: "testdata/Dockerfile-curl-sh", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 4, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "wget | /bin/sh", - args: args{ - Filename: "testdata/Dockerfile-wget-bin-sh", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 3, + name: "wget | /bin/sh", + filename: "testdata/Dockerfile-wget-bin-sh", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 3, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "wget no exec", - args: args{ - Filename: "testdata/Dockerfile-script-ok", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: true, - NumberOfErrors: 0, + name: "wget no exec", + filename: "testdata/Dockerfile-script-ok", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "curl file sh", - args: args{ - Filename: "testdata/Dockerfile-curl-file-sh", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 12, + name: "curl file sh", + filename: "testdata/Dockerfile-curl-file-sh", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 12, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "proc substitution", - args: args{ - Filename: "testdata/Dockerfile-proc-subs", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 6, + name: "proc substitution", + filename: "testdata/Dockerfile-proc-subs", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 6, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "wget file", - args: args{ - Filename: "testdata/Dockerfile-wget-file", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 10, + name: "wget file", + filename: "testdata/Dockerfile-wget-file", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 10, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "gsutil file", - args: args{ - Filename: "testdata/Dockerfile-gsutil-file", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 17, + name: "gsutil file", + filename: "testdata/Dockerfile-gsutil-file", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 17, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "aws file", - args: args{ - Filename: "testdata/Dockerfile-aws-file", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 15, + name: "aws file", + filename: "testdata/Dockerfile-aws-file", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 15, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "pkg managers", - args: args{ - Filename: "testdata/Dockerfile-pkg-managers", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 27, + name: "pkg managers", + filename: "testdata/Dockerfile-pkg-managers", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 27, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, } - //nolint 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() var content []byte var err error - content, err = ioutil.ReadFile(tt.args.Filename) - if err != nil { - panic(fmt.Errorf("cannot read file: %w", err)) - } - - r, err := validateDockerfileDownloads(tt.args.Filename, content, tt.args.Log.Logf) - - if !errors.Is(err, tt.want.Error) || - r != tt.want.Result || - len(tt.args.Log.messages) != tt.want.NumberOfErrors { - t.Errorf("TestDockerfileScriptDownload:\"%v\": %v (%v,%v,%v) want (%v, %v, %v)\n%v", - tt.name, tt.args.Filename, r, err, len(tt.args.Log.messages), tt.want.Result, tt.want.Error, tt.want.NumberOfErrors, - strings.Join(tt.args.Log.messages, "\n")) + if tt.filename == "" { + content = make([]byte, 0) + } else { + content, err = ioutil.ReadFile(tt.filename) + if err != nil { + panic(fmt.Errorf("cannot read file: %w", err)) + } } + dl := scut.TestDetailLogger{} + r := testValidateDockerfileIsFreeOfInsecureDownloads(tt.filename, content, &dl) + scut.ValidateTestReturn(t, tt.name, &tt.expected, &r, &dl) }) } } func TestShellScriptDownload(t *testing.T) { t.Parallel() - //nolint - type args struct { - // Note: this seems to be defined in e2e/e2e_suite_test.go - Log log - Filename string - } - - type returnValue struct { - Error error - Result bool - NumberOfErrors int - } - - //nolint tests := []struct { - args args - want returnValue - name string + name string + filename string + expected scut.TestReturn }{ { - name: "sh script", - args: args{ - Filename: "testdata/script-sh", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 7, + name: "sh script", + filename: "testdata/script-sh", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 7, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "bash script", - args: args{ - Filename: "testdata/script-bash", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 7, + name: "bash script", + filename: "testdata/script-bash", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 7, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "sh script 2", - args: args{ - Filename: "testdata/script.sh", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 7, + name: "sh script 2", + filename: "testdata/script.sh", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 7, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "pkg managers", - args: args{ - Filename: "testdata/script-pkg-managers", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 24, + name: "pkg managers", + filename: "testdata/script-pkg-managers", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 24, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, } - //nolint 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() var content []byte var err error - content, err = ioutil.ReadFile(tt.args.Filename) - if err != nil { - panic(fmt.Errorf("cannot read file: %w", err)) - } - - r, err := validateShellScriptDownloads(tt.args.Filename, content, tt.args.Log.Logf) - - if !errors.Is(err, tt.want.Error) || - r != tt.want.Result || - len(tt.args.Log.messages) != tt.want.NumberOfErrors { - t.Errorf("TestDockerfileScriptDownload:\"%v\": %v (%v,%v,%v) want (%v, %v, %v)\n%v", - tt.name, tt.args.Filename, r, err, len(tt.args.Log.messages), tt.want.Result, tt.want.Error, tt.want.NumberOfErrors, - strings.Join(tt.args.Log.messages, "\n")) + if tt.filename == "" { + content = make([]byte, 0) + } else { + content, err = ioutil.ReadFile(tt.filename) + if err != nil { + panic(fmt.Errorf("cannot read file: %w", err)) + } } + dl := scut.TestDetailLogger{} + r := testValidateShellScriptIsFreeOfInsecureDownloads(tt.filename, content, &dl) + scut.ValidateTestReturn(t, tt.name, &tt.expected, &r, &dl) }) } } func TestGitHubWorflowRunDownload(t *testing.T) { t.Parallel() - //nolint - type args struct { - // Note: this seems to be defined in e2e/e2e_suite_test.go - Log log - Filename string - } - - type returnValue struct { - Error error - Result bool - NumberOfErrors int - } - - //nolint tests := []struct { - args args - want returnValue - name string + name string + filename string + expected scut.TestReturn }{ { - name: "workflow curl default", - args: args{ - Filename: "testdata/github-workflow-curl-default", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 1, + name: "workflow curl default", + filename: "testdata/github-workflow-curl-default", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 1, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "workflow curl no default", - args: args{ - Filename: "testdata/github-workflow-curl-no-default", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 1, + name: "workflow curl no default", + filename: "testdata/github-workflow-curl-no-default", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 1, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "wget across steps", - args: args{ - Filename: "testdata/github-workflow-wget-across-steps", - Log: log{}, - }, - want: returnValue{ - Error: nil, - Result: false, - NumberOfErrors: 2, + name: "wget across steps", + filename: "testdata/github-workflow-wget-across-steps", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 2, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, } - //nolint 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() var content []byte var err error - content, err = ioutil.ReadFile(tt.args.Filename) - if err != nil { - panic(fmt.Errorf("cannot read file: %w", err)) - } - - r, err := validateGitHubWorkflowShellScriptDownloads(tt.args.Filename, content, tt.args.Log.Logf) - - if !errors.Is(err, tt.want.Error) || - r != tt.want.Result || - len(tt.args.Log.messages) != tt.want.NumberOfErrors { - t.Errorf("TestDockerfileScriptDownload:\"%v\": %v (%v,%v,%v) want (%v, %v, %v)\n%v", - tt.name, tt.args.Filename, r, err, len(tt.args.Log.messages), tt.want.Result, tt.want.Error, tt.want.NumberOfErrors, - strings.Join(tt.args.Log.messages, "\n")) + if tt.filename == "" { + content = make([]byte, 0) + } else { + content, err = ioutil.ReadFile(tt.filename) + if err != nil { + panic(fmt.Errorf("cannot read file: %w", err)) + } } + dl := scut.TestDetailLogger{} + r := testValidateGitHubWorkflowScriptFreeOfInsecureDownloads(tt.filename, content, &dl) + scut.ValidateTestReturn(t, tt.name, &tt.expected, &r, &dl) }) } } diff --git a/checks/permissions.go b/checks/permissions.go index 838dfd0c..8a3d71bc 100644 --- a/checks/permissions.go +++ b/checks/permissions.go @@ -15,46 +15,46 @@ package checks import ( - "errors" "fmt" "strings" "gopkg.in/yaml.v2" "github.com/ossf/scorecard/checker" + sce "github.com/ossf/scorecard/errors" ) const CheckPermissions = "Token-Permissions" -// ErrInvalidGitHubWorkflowFile : Invalid GitHub workflow file. -var ErrInvalidGitHubWorkflowFile = errors.New("invalid GitHub workflow file") - //nolint:gochecknoinits func init() { registerCheck(CheckPermissions, leastPrivilegedTokens) } func leastPrivilegedTokens(c *checker.CheckRequest) checker.CheckResult { - return CheckFilesContent(CheckPermissions, ".github/workflows/*", true, c, validateGitHubActionTokenPermissions) + r, err := CheckFilesContent2(".github/workflows/*", false, c, validateGitHubActionTokenPermissions) + return createResultForLeastPrivilegeTokens(r, err) } func validatePermission(key string, value interface{}, path string, - logf func(s string, f ...interface{})) (bool, error) { + dl checker.DetailLogger) (bool, error) { val, ok := value.(string) if !ok { - return false, ErrInvalidGitHubWorkflowFile + //nolint + return false, sce.Create(sce.ErrScorecardInternal, errInvalidGitHubWorkflowFile.Error()) } if strings.EqualFold(val, "write") { - logf("!! token-permissions/github-token - %v permission set to '%v' in %v", key, val, path) + dl.Warn("'%v' permission set to '%v' in %v", key, val, path) return false, nil } + dl.Info("'%v' permission set to '%v' in %v", key, val, path) return true, nil } func validateMapPermissions(values map[interface{}]interface{}, path string, - logf func(s string, f ...interface{})) (bool, error) { + dl checker.DetailLogger) (bool, error) { permissionRead := true var r bool var err error @@ -63,10 +63,11 @@ func validateMapPermissions(values map[interface{}]interface{}, path string, for k, v := range values { key, ok := k.(string) if !ok { - return false, ErrInvalidGitHubWorkflowFile + //nolint + return false, sce.Create(sce.ErrScorecardInternal, errInvalidGitHubWorkflowFile.Error()) } - if r, err = validatePermission(key, v, path, logf); err != nil { + if r, err = validatePermission(key, v, path, dl); err != nil { return false, err } @@ -78,13 +79,13 @@ func validateMapPermissions(values map[interface{}]interface{}, path string, } func validateReadPermissions(config map[interface{}]interface{}, path string, - logf func(s string, f ...interface{})) (bool, error) { + dl checker.DetailLogger) (bool, error) { var permissions interface{} // Check if permissions are set explicitly. permissions, ok := config["permissions"] if !ok { - logf("!! token-permissions/github-token - no permission defined in %v", path) + dl.Warn("no permission defined in %v", path) return false, nil } @@ -93,17 +94,18 @@ func validateReadPermissions(config map[interface{}]interface{}, path string, // Empty string is nil type. // It defaults to 'none' case nil: - + dl.Info("permission set to 'none' in %v", path) // String type. case string: if !strings.EqualFold(val, "read-all") && val != "" { - logf("!! token-permissions/github-token - permission set to '%v' in %v", val, path) + dl.Warn("permission set to '%v' in %v", val, path) return false, nil } + dl.Info("permission set to '%v' in %v", val, path) // Map type. case map[interface{}]interface{}: - if res, err := validateMapPermissions(val, path, logf); err != nil { + if res, err := validateMapPermissions(val, path, dl); err != nil { return false, err } else if !res { return false, nil @@ -111,17 +113,39 @@ func validateReadPermissions(config map[interface{}]interface{}, path string, // Invalid type. default: - return false, ErrInvalidGitHubWorkflowFile + //nolint + return false, sce.Create(sce.ErrScorecardInternal, errInvalidGitHubWorkflowFile.Error()) } return true, nil } +// Create the result. +func createResultForLeastPrivilegeTokens(r bool, err error) checker.CheckResult { + if err != nil { + return checker.CreateRuntimeErrorResult(CheckPermissions, err) + } + if !r { + return checker.CreateMinScoreResult(CheckPermissions, + "non read-only tokens detected in GitHub workflows") + } + + return checker.CreateMaxScoreResult(CheckPermissions, + "tokens are read-only in GitHub workflows") +} + +func testValidateGitHubActionTokenPermissions(pathfn string, + content []byte, dl checker.DetailLogger) checker.CheckResult { + r, err := validateGitHubActionTokenPermissions(pathfn, content, dl) + return createResultForLeastPrivilegeTokens(r, err) +} + // Check file content. func validateGitHubActionTokenPermissions(path string, content []byte, - logf func(s string, f ...interface{})) (bool, error) { + dl checker.DetailLogger) (bool, error) { if len(content) == 0 { - return false, ErrEmptyFile + //nolint + return false, sce.Create(sce.ErrScorecardInternal, errInternalEmptyFile.Error()) } var workflow map[interface{}]interface{} @@ -129,8 +153,8 @@ func validateGitHubActionTokenPermissions(path string, content []byte, var err error err = yaml.Unmarshal(content, &workflow) if err != nil { - return false, fmt.Errorf("!! token-permissions - cannot unmarshal file %v\n%v\n%v: %w", - path, content, string(content), err) + //nolint + return false, sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("yaml.Unmarshal: %v", err)) } // 1. Check that each file uses 'content: read' only or 'none'. @@ -138,7 +162,7 @@ func validateGitHubActionTokenPermissions(path string, content []byte, // https://docs.github.com/en/actions/reference/authentication-in-a-workflow#example-1-passing-the-github_token-as-an-input, // https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/, // https://docs.github.com/en/actions/reference/authentication-in-a-workflow#modifying-the-permissions-for-the-github_token. - if r, err = validateReadPermissions(workflow, path, logf); err != nil { + if r, err = validateReadPermissions(workflow, path, dl); err != nil { return false, nil } if !r { diff --git a/checks/permissions_test.go b/checks/permissions_test.go index ffbd496c..729afe61 100644 --- a/checks/permissions_test.go +++ b/checks/permissions_test.go @@ -15,130 +15,118 @@ package checks import ( - "errors" "fmt" "io/ioutil" "testing" + + "github.com/ossf/scorecard/checker" + scut "github.com/ossf/scorecard/utests" ) +//nolint func TestGithubTokenPermissions(t *testing.T) { t.Parallel() - type args struct { - Logf func(s string, f ...interface{}) - Filename string - } - type returnValue struct { - Error error - Result bool - } - - l := log{} tests := []struct { - args args - want returnValue - name string + name string + filename string + expected scut.TestReturn }{ { - name: "Write all test", - args: args{ - Filename: "./testdata/github-workflow-permissions-writeall.yaml", - Logf: l.Logf, - }, - want: returnValue{ - Error: nil, - Result: false, + name: "Write all test", + filename: "./testdata/github-workflow-permissions-writeall.yaml", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 1, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "Read all test", - args: args{ - Filename: "./testdata/github-workflow-permissions-readall.yaml", - Logf: l.Logf, - }, - want: returnValue{ - Error: nil, - Result: true, + name: "Read all test", + filename: "./testdata/github-workflow-permissions-readall.yaml", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 1, + NumberOfDebug: 0, }, }, { - name: "No permission test", - args: args{ - Filename: "./testdata/github-workflow-permissions-absent.yaml", - Logf: l.Logf, - }, - want: returnValue{ - Error: nil, - Result: false, + name: "No permission test", + filename: "./testdata/github-workflow-permissions-absent.yaml", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 1, + NumberOfInfo: 0, + NumberOfDebug: 0, }, }, { - name: "Writes test", - args: args{ - Filename: "./testdata/github-workflow-permissions-writes.yaml", - Logf: l.Logf, - }, - want: returnValue{ - Error: nil, - Result: false, + name: "Writes test", + filename: "./testdata/github-workflow-permissions-writes.yaml", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MinResultScore, + NumberOfWarn: 1, + NumberOfInfo: 1, + NumberOfDebug: 0, }, }, { - name: "Reads test", - args: args{ - Filename: "./testdata/github-workflow-permissions-reads.yaml", - Logf: l.Logf, - }, - want: returnValue{ - Error: nil, - Result: true, + name: "Reads test", + filename: "./testdata/github-workflow-permissions-reads.yaml", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 10, + NumberOfDebug: 0, }, }, { - name: "Nones test", - args: args{ - Filename: "./testdata/github-workflow-permissions-nones.yaml", - Logf: l.Logf, - }, - want: returnValue{ - Error: nil, - Result: true, + name: "Nones test", + filename: "./testdata/github-workflow-permissions-nones.yaml", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 10, + NumberOfDebug: 0, }, }, { - name: "None test", - args: args{ - Filename: "./testdata/github-workflow-permissions-none.yaml", - Logf: l.Logf, - }, - want: returnValue{ - Error: nil, - Result: true, + name: "None test", + filename: "./testdata/github-workflow-permissions-none.yaml", + expected: scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 1, + NumberOfDebug: 0, }, }, } for _, tt := range tests { tt := tt // Re-initializing variable so it is not changed while executing the closure below - l.messages = []string{} t.Run(tt.name, func(t *testing.T) { t.Parallel() var content []byte var err error - if tt.args.Filename == "" { + if tt.filename == "" { content = make([]byte, 0) } else { - content, err = ioutil.ReadFile(tt.args.Filename) + content, err = ioutil.ReadFile(tt.filename) if err != nil { panic(fmt.Errorf("cannot read file: %w", err)) } } - r, err := validateGitHubActionTokenPermissions(tt.args.Filename, content, tt.args.Logf) - - if !errors.Is(err, tt.want.Error) || - r != tt.want.Result { - t.Errorf("TestGithubTokenPermissions:\"%v\": %v (%v,%v) want (%v, %v)", - tt.name, tt.args.Filename, r, err, tt.want.Result, tt.want.Error) - } + dl := scut.TestDetailLogger{} + r := testValidateGitHubActionTokenPermissions(tt.filename, content, &dl) + scut.ValidateTestReturn(t, tt.name, &tt.expected, &r, &dl) }) } } diff --git a/checks/shell_download_validate.go b/checks/shell_download_validate.go index 064eb269..b31e32a7 100644 --- a/checks/shell_download_validate.go +++ b/checks/shell_download_validate.go @@ -17,7 +17,6 @@ package checks import ( "bufio" "bytes" - "errors" "fmt" "net/url" "path" @@ -26,14 +25,11 @@ import ( "strings" "mvdan.cc/sh/v3/syntax" + + "github.com/ossf/scorecard/checker" + sce "github.com/ossf/scorecard/errors" ) -// ErrParsingDockerfile indicates a problem parsing the dockerfile. -var ErrParsingDockerfile = errors.New("file cannot be parsed") - -// ErrParsingShellCommand indicates a problem parsing a shell command. -var ErrParsingShellCommand = errors.New("shell command cannot be parsed") - // List of interpreters. var pythonInterpreters = []string{"python", "python3", "python2.7"} @@ -106,7 +102,8 @@ func getWgetOutputFile(cmd []string) (pathfn string, ok bool, err error) { u, err := url.Parse(cmd[i]) if err != nil { - return "", false, fmt.Errorf("url.Parse: %w", err) + //nolint + return "", false, sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("url.Parse: %v", err)) } return path.Base(u.Path), true, nil } @@ -125,7 +122,8 @@ func getGsutilOutputFile(cmd []string) (pathfn string, ok bool, err error) { // Directory. u, err := url.Parse(cmd[i]) if err != nil { - return "", false, fmt.Errorf("url.Parse: %w", err) + //nolint + return "", false, sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("url.Parse: %v", err)) } return filepath.Join(filepath.Dir(pathfn), path.Base(u.Path)), true, nil } @@ -150,7 +148,8 @@ func getAWSOutputFile(cmd []string) (pathfn string, ok bool, err error) { if filepath.Clean(filepath.Dir(ofile)) == filepath.Clean(ofile) { u, err := url.Parse(ifile) if err != nil { - return "", false, fmt.Errorf("url.Parse: %w", err) + //nolint + return "", false, sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("url.Parse: %v", err)) } return filepath.Join(filepath.Dir(ofile), path.Base(u.Path)), true, nil } @@ -266,7 +265,7 @@ func extractCommand(cmd interface{}) ([]string, bool) { } func isFetchPipeExecute(node syntax.Node, cmd, pathfn string, - logf func(s string, f ...interface{})) bool { + dl checker.DetailLogger) bool { // BinaryCmd {Op=|, X=CallExpr{Args={curl, -s, url}}, Y=CallExpr{Args={bash,}}}. bc, ok := node.(*syntax.BinaryCmd) if !ok { @@ -295,8 +294,7 @@ func isFetchPipeExecute(node syntax.Node, cmd, pathfn string, return false } - logf("!! frozen-deps/fetch-execute - %v is fetching and executing non-pinned program '%v'", - pathfn, cmd) + dl.Warn("insecure (unpinned) download detected in %v: '%v'", pathfn, cmd) return true } @@ -322,7 +320,7 @@ func getRedirectFile(red []*syntax.Redirect) (string, bool) { } func isExecuteFiles(node syntax.Node, cmd, pathfn string, files map[string]bool, - logf func(s string, f ...interface{})) bool { + dl checker.DetailLogger) bool { ce, ok := node.(*syntax.CallExpr) if !ok { return false @@ -336,8 +334,7 @@ func isExecuteFiles(node syntax.Node, cmd, pathfn string, files map[string]bool, ok = false for fn := range files { if isInterpreterWithFile(c, fn) || isExecuteFile(c, fn) { - logf("!! frozen-deps/fetch-execute - %v is fetching and executing non-pinned program '%v'", - pathfn, cmd) + dl.Warn("insecure (unpinned) download detected in %v: '%v'", pathfn, cmd) ok = true } } @@ -479,7 +476,7 @@ func isPipUnpinnedDownload(cmd []string) bool { } func isUnpinnedPakageManagerDownload(node syntax.Node, cmd, pathfn string, - logf func(s string, f ...interface{})) bool { + dl checker.DetailLogger) bool { ce, ok := node.(*syntax.CallExpr) if !ok { return false @@ -492,15 +489,13 @@ func isUnpinnedPakageManagerDownload(node syntax.Node, cmd, pathfn string, // Go get/install. if isGoUnpinnedDownload(c) { - logf("!! frozen-deps/fetch-execute - %v is fetching an non-pinned dependency '%v'", - pathfn, cmd) + dl.Warn("insecure (unpinned) download detected in %v: '%v'", pathfn, cmd) return true } // Pip install. if isPipUnpinnedDownload(c) { - logf("!! frozen-deps/fetch-execute - %v is fetching an non-pinned dependency '%v'", - pathfn, cmd) + dl.Warn("insecure (unpinned) download detected in %v: '%v'", pathfn, cmd) return true } @@ -533,7 +528,7 @@ func recordFetchFileFromNode(node syntax.Node) (pathfn string, ok bool, err erro } func isFetchProcSubsExecute(node syntax.Node, cmd, pathfn string, - logf func(s string, f ...interface{})) bool { + dl checker.DetailLogger) bool { ce, ok := node.(*syntax.CallExpr) if !ok { return false @@ -583,8 +578,7 @@ func isFetchProcSubsExecute(node syntax.Node, cmd, pathfn string, return false } - logf("!! frozen-deps/fetch-execute - %v is fetching and executing non-pinned program '%v'", - pathfn, cmd) + dl.Warn("insecure (unpinned) download detected in %v: '%v'", pathfn, cmd) return true } @@ -655,17 +649,20 @@ func nodeToString(p *syntax.Printer, node syntax.Node) (string, error) { err := p.Print(&buf, node) // This is ugly, but the parser does not have a defined error type :/. if err != nil && !strings.Contains(err.Error(), "unsupported node type") { - return "", fmt.Errorf("syntax.Printer.Print: %w", err) + //nolint + return "", sce.Create(sce.ErrScorecardInternal, fmt.Sprintf("syntax.Printer.Print: %v", err)) } return buf.String(), nil } func validateShellFileAndRecord(pathfn string, content []byte, files map[string]bool, - logf func(s string, f ...interface{})) (bool, error) { + dl checker.DetailLogger) (bool, error) { in := strings.NewReader(string(content)) f, err := syntax.NewParser().Parse(in, "") if err != nil { - return false, ErrParsingShellCommand + //nolint + return false, sce.Create(sce.ErrScorecardInternal, + fmt.Sprintf("%v: %v", errInternalInvalidShellCode, err)) } printer := syntax.NewPrinter() @@ -682,7 +679,7 @@ func validateShellFileAndRecord(pathfn string, content []byte, files map[string] c, ok := extractInterpreterCommandFromNode(node) // nolinter if ok { - ok, e := validateShellFileAndRecord(pathfn, []byte(c), files, logf) + ok, e := validateShellFileAndRecord(pathfn, []byte(c), files, dl) validated = ok if e != nil { err = e @@ -691,23 +688,23 @@ func validateShellFileAndRecord(pathfn string, content []byte, files map[string] } // `curl | bash` (supports `sudo`). - if isFetchPipeExecute(node, cmdStr, pathfn, logf) { + if isFetchPipeExecute(node, cmdStr, pathfn, dl) { validated = false } // Check if we're calling a file we previously downloaded. // Includes `curl > /tmp/file [&&|;] [bash] /tmp/file` - if isExecuteFiles(node, cmdStr, pathfn, files, logf) { + if isExecuteFiles(node, cmdStr, pathfn, files, dl) { validated = false } // `bash <(wget -qO- http://website.com/my-script.sh)`. (supports `sudo`). - if isFetchProcSubsExecute(node, cmdStr, pathfn, logf) { + if isFetchProcSubsExecute(node, cmdStr, pathfn, dl) { validated = false } // Package manager's unpinned installs. - if isUnpinnedPakageManagerDownload(node, cmdStr, pathfn, logf) { + if isUnpinnedPakageManagerDownload(node, cmdStr, pathfn, dl) { validated = false } // TODO(laurent): add check for cat file | bash. @@ -783,7 +780,7 @@ func isShellScriptFile(pathfn string, content []byte) bool { return false } -func validateShellFile(pathfn string, content []byte, logf func(s string, f ...interface{})) (bool, error) { +func validateShellFile(pathfn string, content []byte, dl checker.DetailLogger) (bool, error) { files := make(map[string]bool) - return validateShellFileAndRecord(pathfn, content, files, logf) + return validateShellFileAndRecord(pathfn, content, files, dl) } diff --git a/checks/testdata/Dockerfile-not-pinned-as b/checks/testdata/Dockerfile-not-pinned-as index 8af7a8ba..ee0c1b90 100644 --- a/checks/testdata/Dockerfile-not-pinned-as +++ b/checks/testdata/Dockerfile-not-pinned-as @@ -23,4 +23,10 @@ RUN CGO_ENABLED=0 make build-scorecard FROM build RUN /hello-world +FROM base as base2 +RUN ls + +FROM base2 +RUN ls + FROM python@sha256:45b23dee08af5e43a7fea6c4cf9c25ccf269ee113168c19722f87876677c5cb2 \ No newline at end of file diff --git a/e2e/branchprotection_test.go b/e2e/branchprotection_test.go index b8464bbe..6692fcf4 100644 --- a/e2e/branchprotection_test.go +++ b/e2e/branchprotection_test.go @@ -22,13 +22,15 @@ import ( "github.com/ossf/scorecard/checker" "github.com/ossf/scorecard/checks" + sce "github.com/ossf/scorecard/errors" + scut "github.com/ossf/scorecard/utests" ) var _ = Describe("E2E TEST:Branch Protection", func() { Context("E2E TEST:Validating branch protection", func() { It("Should fail to return branch protection on other repositories", func() { - l := log{} - checkRequest := checker.CheckRequest{ + dl := scut.TestDetailLogger{} + req := checker.CheckRequest{ Ctx: context.Background(), Client: ghClient, HTTPClient: httpClient, @@ -36,11 +38,22 @@ var _ = Describe("E2E TEST:Branch Protection", func() { Owner: "apache", Repo: "airflow", GraphClient: graphClient, - Logf: l.Logf, + Dlogger: &dl, } - result := checks.BranchProtection(&checkRequest) + expected := scut.TestReturn{ + Errors: []error{sce.ErrScorecardInternal}, + Score: checker.InconclusiveResultScore, + NumberOfWarn: 0, + NumberOfInfo: 0, + NumberOfDebug: 0, + } + result := checks.BranchProtection(&req) + // UPGRADEv2: to remove. + // Old version. Expect(result.Error).ShouldNot(BeNil()) Expect(result.Pass).Should(BeFalse()) + // New version. + Expect(scut.ValidateTestReturn(nil, "branch protection not accessible", &expected, &result, &dl)).Should(BeTrue()) }) }) }) diff --git a/e2e/frozen_deps_test.go b/e2e/frozen_deps_test.go index 3844ddc8..cde9d71d 100644 --- a/e2e/frozen_deps_test.go +++ b/e2e/frozen_deps_test.go @@ -12,7 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -//nolint: dupl // repeating test cases that are slightly different is acceptable package e2e import ( @@ -24,17 +23,20 @@ import ( "github.com/ossf/scorecard/checker" "github.com/ossf/scorecard/checks" "github.com/ossf/scorecard/clients/githubrepo" + scut "github.com/ossf/scorecard/utests" ) +// TODO: use dedicated repo that don't change. +// TODO: need negative results. var _ = Describe("E2E TEST:FrozenDeps", func() { Context("E2E TEST:Validating deps are frozen", func() { It("Should return deps are not frozen", func() { - l := log{} + dl := scut.TestDetailLogger{} repoClient := githubrepo.CreateGithubRepoClient(context.Background(), ghClient) err := repoClient.InitRepo("tensorflow", "tensorflow") Expect(err).Should(BeNil()) - checkRequest := checker.CheckRequest{ + req := checker.CheckRequest{ Ctx: context.Background(), Client: ghClient, HTTPClient: httpClient, @@ -42,19 +44,30 @@ var _ = Describe("E2E TEST:FrozenDeps", func() { Owner: "tensorflow", Repo: "tensorflow", GraphClient: graphClient, - Logf: l.Logf, + Dlogger: &dl, } - result := checks.FrozenDeps(&checkRequest) + expected := scut.TestReturn{ + Errors: nil, + Score: checker.InconclusiveResultScore, + NumberOfWarn: 219, + NumberOfInfo: 0, + NumberOfDebug: 0, + } + result := checks.FrozenDeps(&req) + // UPGRADEv2: to remove. + // Old version. Expect(result.Error).Should(BeNil()) Expect(result.Pass).Should(BeFalse()) + // New version. + Expect(scut.ValidateTestReturn(nil, "deps not frozen", &expected, &result, &dl)).Should(BeTrue()) }) It("Should return deps are frozen", func() { - l := log{} + dl := scut.TestDetailLogger{} repoClient := githubrepo.CreateGithubRepoClient(context.Background(), ghClient) err := repoClient.InitRepo("ossf", "scorecard") Expect(err).Should(BeNil()) - checkRequest := checker.CheckRequest{ + req := checker.CheckRequest{ Ctx: context.Background(), Client: ghClient, HTTPClient: httpClient, @@ -62,11 +75,22 @@ var _ = Describe("E2E TEST:FrozenDeps", func() { Owner: "ossf", Repo: "scorecard", GraphClient: graphClient, - Logf: l.Logf, + Dlogger: &dl, } - result := checks.FrozenDeps(&checkRequest) + expected := scut.TestReturn{ + Errors: nil, + Score: checker.MaxResultScore, + NumberOfWarn: 0, + NumberOfInfo: 1, + NumberOfDebug: 0, + } + result := checks.FrozenDeps(&req) + // UPGRADEv2: to remove. + // Old version. Expect(result.Error).Should(BeNil()) Expect(result.Pass).Should(BeTrue()) + // New version. + Expect(scut.ValidateTestReturn(nil, "deps frozen", &expected, &result, &dl)).Should(BeTrue()) }) }) }) diff --git a/e2e/security_policy_test.go b/e2e/security_policy_test.go index 7ac87afd..d943c6a8 100644 --- a/e2e/security_policy_test.go +++ b/e2e/security_policy_test.go @@ -12,7 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -//nolint:dupl // repeating test cases that are slightly different is acceptable package e2e import (