switch signed-releases lookback limit precedence

if the 6th release had no assets, the lookback limit exit condition was being skipped. This led to scenarios where too many releases were being considered by the Signed-Releases check. ossf#4059 Signed-off-by: Spencer Schrock <[email protected]>
spencerschrock · Apr 29, 2024 · 9c722e1 · 9c722e1
1 parent 71aed95
commit 9c722e1
Show file tree

Hide file tree

Showing 4 changed files with 109 additions and 7 deletions.
diff --git a/probes/releasesAreSigned/impl.go b/probes/releasesAreSigned/impl.go
@@ -53,14 +53,14 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
 
 	totalReleases := 0
 	for releaseIndex, release := range releases {
-		if len(release.Assets) == 0 {
-			continue
-		}
-
 		if releaseIndex == releaseLookBack {
 			break
 		}
 
+		if len(release.Assets) == 0 {
+			continue
+		}
+
 		totalReleases++
 		signed := false
 		for j := range release.Assets {

diff --git a/probes/releasesAreSigned/impl_test.go b/probes/releasesAreSigned/impl_test.go
@@ -189,6 +189,30 @@ func Test_Run(t *testing.T) {
 				finding.OutcomeTrue,
 			},
 		},
+		{
+			// https://github.com/ossf/scorecard/issues/4059
+			name: "lookback cutoff not skipped if 6th release has no assets",
+			raw: &checker.RawResults{
+				SignedReleasesResults: checker.SignedReleasesData{
+					Releases: []clients.Release{
+						release("v0.8.5"),
+						release("v0.8.4"),
+						release("v0.8.3"),
+						release("v0.8.2"),
+						release("v0.8.1"),
+						releaseNoAssets("v0.8.0"),
+						release("v0.7.0"),
+					},
+				},
+			},
+			outcomes: []finding.Outcome{
+				finding.OutcomeTrue,
+				finding.OutcomeTrue,
+				finding.OutcomeTrue,
+				finding.OutcomeTrue,
+				finding.OutcomeTrue,
+			},
+		},
 	}
 	for _, tt := range tests {
 		tt := tt // Re-initializing variable so it is not changed while executing the closure below
@@ -251,3 +275,12 @@ func release(version string) clients.Release {
 		},
 	}
 }
+
+func releaseNoAssets(version string) clients.Release {
+	return clients.Release{
+		TagName:         version,
+		URL:             fmt.Sprintf("https://github.com/test/test_artifact/releases/tag/%s", version),
+		TargetCommitish: "master",
+		Assets:          []clients.ReleaseAsset{},
+	}
+}
diff --git a/probes/releasesHaveProvenance/impl.go b/probes/releasesHaveProvenance/impl.go
@@ -56,12 +56,12 @@ func Run(raw *checker.RawResults) ([]finding.Finding, string, error) {
 
 	for i := range releases {
 		release := releases[i]
-		if len(release.Assets) == 0 {
-			continue
-		}
 		if i == releaseLookBack {
 			break
 		}
+		if len(release.Assets) == 0 {
+			continue
+		}
 		totalReleases++
 		hasProvenance := false
 		for j := range release.Assets {

diff --git a/probes/releasesHaveProvenance/impl_test.go b/probes/releasesHaveProvenance/impl_test.go
@@ -217,6 +217,75 @@ func Test_Run(t *testing.T) {
 				finding.OutcomeTrue,
 			},
 		},
+		{
+			// https://github.com/ossf/scorecard/issues/4059
+			name: "lookback cutoff not skipped if 6th release has no assets",
+			raw: &checker.RawResults{
+				SignedReleasesResults: checker.SignedReleasesData{
+					Releases: []clients.Release{
+						{
+							TagName: "v7.0",
+							Assets: []clients.ReleaseAsset{
+								{Name: "binary.tar.gz"},
+								{Name: "binary.tar.gz.sig"},
+								{Name: "binary.tar.gz.intoto.jsonl"},
+							},
+						},
+						{
+							TagName: "v6.0",
+							Assets: []clients.ReleaseAsset{
+								{Name: "binary.tar.gz"},
+								{Name: "binary.tar.gz.sig"},
+								{Name: "binary.tar.gz.intoto.jsonl"},
+							},
+						},
+						{
+							TagName: "v5.0",
+							Assets: []clients.ReleaseAsset{
+								{Name: "binary.tar.gz"},
+								{Name: "binary.tar.gz.sig"},
+								{Name: "binary.tar.gz.intoto.jsonl"},
+							},
+						},
+						{
+							TagName: "v4.0",
+							Assets: []clients.ReleaseAsset{
+								{Name: "binary.tar.gz"},
+								{Name: "binary.tar.gz.sig"},
+								{Name: "binary.tar.gz.intoto.jsonl"},
+							},
+						},
+						{
+							TagName: "v3.0",
+							Assets: []clients.ReleaseAsset{
+								{Name: "binary.tar.gz"},
+								{Name: "binary.tar.gz.sig"},
+								{Name: "binary.tar.gz.intoto.jsonl"},
+							},
+						},
+						{
+							TagName: "v2.0",
+							Assets:  []clients.ReleaseAsset{},
+						},
+						{
+							TagName: "v1.0",
+							Assets: []clients.ReleaseAsset{
+								{Name: "binary.tar.gz"},
+								{Name: "binary.tar.gz.sig"},
+								{Name: "binary.tar.gz.intoto.jsonl"},
+							},
+						},
+					},
+				},
+			},
+			outcomes: []finding.Outcome{
+				finding.OutcomeTrue,
+				finding.OutcomeTrue,
+				finding.OutcomeTrue,
+				finding.OutcomeTrue,
+				finding.OutcomeTrue,
+			},
+		},
 	}
 	for _, tt := range tests {
 		tt := tt // Re-initializing variable so it is not changed while executing the closure below