From 50071692ceaf1cb2d78f3c66006c96e46ac0606c Mon Sep 17 00:00:00 2001 From: Spencer Schrock Date: Thu, 2 May 2024 10:43:13 -0700 Subject: [PATCH] :bug: fix signed-releases lookback limit precedence (#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. https://github.com/ossf/scorecard/issues/4059 Signed-off-by: Spencer Schrock * make exit condition stronger any release after the lookback should be skipped Signed-off-by: Spencer Schrock --------- Signed-off-by: Spencer Schrock Signed-off-by: seelder --- probes/releasesAreSigned/impl.go | 8 +-- probes/releasesAreSigned/impl_test.go | 33 +++++++++++ probes/releasesHaveProvenance/impl.go | 6 +- probes/releasesHaveProvenance/impl_test.go | 69 ++++++++++++++++++++++ 4 files changed, 109 insertions(+), 7 deletions(-) diff --git a/probes/releasesAreSigned/impl.go b/probes/releasesAreSigned/impl.go index 4f86c96c3a4..217f50af633 100644 --- 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 index 525ba71e6b8..ac2057876e0 100644 --- 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 index 74fdc27199f..8a87696ff0c 100644 --- 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 index 5b94964016e..d5dbd9d90af 100644 --- 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