🐛 fix signed-releases lookback limit precedence (ossf#4060)

* 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]> * make exit condition stronger any release after the lookback should be skipped Signed-off-by: Spencer Schrock <[email protected]> --------- Signed-off-by: Spencer Schrock <[email protected]> Signed-off-by: seelder <[email protected]>
seelder · May 3, 2024 · 5007169 · 5007169
1 parent 645d5e4
commit 5007169
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,12 +53,12 @@ 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 releaseIndex == releaseLookBack {
-			break
+		if len(release.Assets) == 0 {
+			continue
 		}
 
 		totalReleases++

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 i >= releaseLookBack {
+			break
+		}
 		if len(release.Assets) == 0 {
 			continue
 		}
-		if i == releaseLookBack {
-			break
-		}
 		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