From 8792d9f4b8d5c95da0f763736435ae425e0aa816 Mon Sep 17 00:00:00 2001 From: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> Date: Wed, 27 Dec 2023 18:05:55 -0600 Subject: [PATCH 1/3] :bug: Fix signed release error for empty gitlab repo - Fixed the issue where an empty gitlab repo is causing this error. `Error: check runtime error: Signed-Releases: internal error: could not get release name 2023/12/27 18:07:19 error during command execution: check runtime error: Signed-Releases: internal error: could not get release name exit status 1` Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> --- checks/evaluation/signed_releases.go | 24 +++++---- checks/evaluation/signed_releases_test.go | 62 ++++++++++++++++++++--- 2 files changed, 69 insertions(+), 17 deletions(-) diff --git a/checks/evaluation/signed_releases.go b/checks/evaluation/signed_releases.go index 9f3673ffe21..b53ef039818 100644 --- a/checks/evaluation/signed_releases.go +++ b/checks/evaluation/signed_releases.go @@ -47,10 +47,13 @@ func SignedReleases(name string, f := &findings[i] // Debug release name - releaseName, err := getReleaseName(f) - if err != nil { - e := sce.WithMessage(sce.ErrScorecardInternal, "could not get release name") - return checker.CreateRuntimeErrorResult(name, e) + releaseName := getReleaseName(f) + if releaseName == "" { + dl.Warn(&checker.LogMessage{ + Text: "no GitHub releases found", + }) + // Generic summary. + return checker.CreateInconclusiveResult(name, "no releases found") } if !contains(loggedReleases, releaseName) { dl.Debug(&checker.LogMessage{ @@ -77,11 +80,10 @@ func SignedReleases(name string, for i := range findings { f := &findings[i] - releaseName, err := getReleaseName(f) - if err != nil { - return checker.CreateRuntimeErrorResult(name, err) + releaseName := getReleaseName(f) + if releaseName == "" { + return checker.CreateInconclusiveResult(name, "no releases found") } - if !contains(uniqueReleaseTags, releaseName) { uniqueReleaseTags = append(uniqueReleaseTags, releaseName) } @@ -126,7 +128,7 @@ func SignedReleases(name string, return checker.CreateResultWithScore(name, reason, score) } -func getReleaseName(f *finding.Finding) (string, error) { +func getReleaseName(f *finding.Finding) string { m := f.Values for k, v := range m { var value int @@ -137,10 +139,10 @@ func getReleaseName(f *finding.Finding) (string, error) { value = int(releasesHaveProvenance.ValueTypeRelease) } if v == value { - return k, nil + return k } } - return "", sce.WithMessage(sce.ErrScorecardInternal, "could not get release tag") + return "" } func contains(releases []string, release string) bool { diff --git a/checks/evaluation/signed_releases_test.go b/checks/evaluation/signed_releases_test.go index 8aa8c8fe5f4..cfbff61771f 100644 --- a/checks/evaluation/signed_releases_test.go +++ b/checks/evaluation/signed_releases_test.go @@ -38,12 +38,6 @@ const ( asset1 = 1 asset2 = 2 asset3 = 3 - asset4 = 4 - asset5 = 5 - asset6 = 6 - asset7 = 7 - asset8 = 8 - asset9 = 9 ) func signedProbe(release, asset int, outcome finding.Outcome) finding.Finding { @@ -282,3 +276,59 @@ func TestSignedReleases(t *testing.T) { }) } } + +func Test_getReleaseName(t *testing.T) { + t.Parallel() + type args struct { + f *finding.Finding + } + tests := []struct { + name string + args args + want string + }{ + { + name: "no release", + args: args{ + f: &finding.Finding{ + Values: map[string]int{}, + }, + }, + want: "", + }, + { + name: "release", + args: args{ + f: &finding.Finding{ + Values: map[string]int{ + "v1": int(releasesAreSigned.ValueTypeRelease), + }, + Probe: releasesAreSigned.Probe, + }, + }, + want: "v1", + }, + { + name: "release and asset", + args: args{ + f: &finding.Finding{ + Values: map[string]int{ + "v1": int(releasesAreSigned.ValueTypeRelease), + "artifact-1": int(releasesAreSigned.ValueTypeReleaseAsset), + }, + Probe: releasesAreSigned.Probe, + }, + }, + want: "v1", + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + if got := getReleaseName(tt.args.f); got != tt.want { + t.Errorf("getReleaseName() = %v, want %v", got, tt.want) + } + }) + } +} From e7c63bf5cf1308fa0b23c6410c8de3b1e4430a09 Mon Sep 17 00:00:00 2001 From: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> Date: Thu, 28 Dec 2023 17:45:23 -0600 Subject: [PATCH 2/3] Fixes based on review. Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> --- checks/evaluation/signed_releases.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/checks/evaluation/signed_releases.go b/checks/evaluation/signed_releases.go index b53ef039818..46ff2740d3c 100644 --- a/checks/evaluation/signed_releases.go +++ b/checks/evaluation/signed_releases.go @@ -47,14 +47,22 @@ func SignedReleases(name string, f := &findings[i] // Debug release name + if f.Outcome == finding.OutcomeNotApplicable { + dl.Warn(&checker.LogMessage{ + Text: "no releases found", + }) + // Generic summary. + return checker.CreateInconclusiveResult(name, "no releases found") + } releaseName := getReleaseName(f) if releaseName == "" { dl.Warn(&checker.LogMessage{ - Text: "no GitHub releases found", + Text: "no releases found", }) // Generic summary. return checker.CreateInconclusiveResult(name, "no releases found") } + if !contains(loggedReleases, releaseName) { dl.Debug(&checker.LogMessage{ Text: fmt.Sprintf("GitHub release found: %s", releaseName), @@ -63,13 +71,6 @@ func SignedReleases(name string, } // Check if outcome is NotApplicable - if f.Outcome == finding.OutcomeNotApplicable { - dl.Warn(&checker.LogMessage{ - Text: "no GitHub releases found", - }) - // Generic summary. - return checker.CreateInconclusiveResult(name, "no releases found") - } } totalPositive := 0 From d4be15a58d47a84d15dd12a7a0baf78cd165d981 Mon Sep 17 00:00:00 2001 From: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> Date: Sat, 30 Dec 2023 10:36:10 -0600 Subject: [PATCH 3/3] Fixed codereview changes. Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com> --- checks/evaluation/signed_releases.go | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/checks/evaluation/signed_releases.go b/checks/evaluation/signed_releases.go index 46ff2740d3c..35c705622ae 100644 --- a/checks/evaluation/signed_releases.go +++ b/checks/evaluation/signed_releases.go @@ -15,6 +15,7 @@ package evaluation import ( + "errors" "fmt" "math" @@ -25,6 +26,8 @@ import ( "github.com/ossf/scorecard/v4/probes/releasesHaveProvenance" ) +var errNoReleaseFound = errors.New("no release found") + // SignedReleases applies the score policy for the Signed-Releases check. func SignedReleases(name string, findings []finding.Finding, dl checker.DetailLogger, @@ -48,19 +51,13 @@ func SignedReleases(name string, // Debug release name if f.Outcome == finding.OutcomeNotApplicable { - dl.Warn(&checker.LogMessage{ - Text: "no releases found", - }) // Generic summary. return checker.CreateInconclusiveResult(name, "no releases found") } releaseName := getReleaseName(f) if releaseName == "" { - dl.Warn(&checker.LogMessage{ - Text: "no releases found", - }) // Generic summary. - return checker.CreateInconclusiveResult(name, "no releases found") + return checker.CreateRuntimeErrorResult(name, errNoReleaseFound) } if !contains(loggedReleases, releaseName) { @@ -83,7 +80,7 @@ func SignedReleases(name string, releaseName := getReleaseName(f) if releaseName == "" { - return checker.CreateInconclusiveResult(name, "no releases found") + return checker.CreateRuntimeErrorResult(name, errNoReleaseFound) } if !contains(uniqueReleaseTags, releaseName) { uniqueReleaseTags = append(uniqueReleaseTags, releaseName)