feat: Implement OSPS-QA-04.02 - #453
Conversation
Satarupa22-SD
changed the title
Signed-off-by: Satarupa22-SD <satarupa2212@gmail.com>
jmeridth
left a comment
jmeridth
left a comment
There was a problem hiding this comment.
🤖 (summary) Nice, careful implementation — the doc comments explaining the single-repo-scanner limitation and the graceful degradation ladder are exactly the right shape, and the tests cover the branches well. Two independent review models flagged the same handful of correctness gaps, all clustered around how the step decides "no subprojects": the self-repo identity, the empty-list branch, and the release gate's scope. Details in the line comments.
|
|
||
| self := "" | ||
| if insights.Repository != nil { | ||
| self = normalizeRepoURL(string(insights.Repository.Url)) |
There was a problem hiding this comment.
🤖 (medium) The self-filter relies entirely on the SI-declared repository.url, but that section is optional in the SI v2 spec — ensureInsightsInitialized in data/rest-data.go substitutes an empty &si.Repository{} when it's absent (e.g. the header.project-si-source flow). Then self is "", nothing is filtered, and a project whose project.repositories lists only the scanned repo returns NeedsReview naming the repo itself as "1 additional project repository" instead of NotApplicable. The scanner already knows what it's evaluating (payload.Config.GetString("owner") / ("repo") are used later in this file), so a fallback self identity of github.com/{owner}/{repo} when repository.url is empty would close this.
|
|
||
| subprojects := subprojectRepositories(payload.Insights) | ||
| if len(subprojects) == 0 { | ||
| return gemara.NotApplicable, "Security Insights lists no repositories beyond the one under evaluation, so no release subprojects are in scope", gemara.Medium |
There was a problem hiding this comment.
🤖 (medium) When the SI file exists but project.repositories is empty or missing, this returns NotApplicable — but the doc comment above is right that without the repository list "the scanner cannot tell whether subprojects exist". An absent list is a QA-04.01 violation, not evidence the project is single-repo, so a multi-repo project that just failed to document its repos gets silently exempted here. NeedsReview at Low confidence matches the missing-file branch above:
| return gemara.NotApplicable, "Security Insights lists no repositories beyond the one under evaluation, so no release subprojects are in scope", gemara.Medium | |
| return gemara.NeedsReview, "Security Insights does not list the project's repositories, so the scanner cannot tell whether release subprojects exist; manually review whether all subprojects enforce security requirements as strict or stricter than the primary codebase", gemara.Low |
| seen := map[string]bool{} | ||
| for _, repo := range insights.Project.Repositories { | ||
| url := normalizeRepoURL(string(repo.Url)) | ||
| if url == "" || url == self || seen[url] { |
There was a problem hiding this comment.
🤖 (medium) Entries with an empty url are silently skipped here, and if every entry lacks one, the caller reports "Security Insights lists no repositories beyond the one under evaluation" and rules the control out. An SI file that plainly declares subprojects (by name, with urls omitted or blank) ends up NotApplicable instead of flagged. Worth counting url-less entries and degrading to NeedsReview when any exist.
| return gemara.NeedsReview, "Release data is unavailable; manually review whether a release comprises multiple repositories and whether all subprojects enforce security requirements as strict as the primary codebase", gemara.Low | ||
| } | ||
| if !released { | ||
| return gemara.NotApplicable, "No published releases found; the subproject security requirement does not apply", gemara.High |
There was a problem hiding this comment.
🤖 (medium) This gate checks only the scanned repository's own releases, but the control text is project-scoped: "When the project has made a release comprising multiple source code repositories…". Subproject repos usually cut no releases of their own (the release comes from the primary repo), so scanning a subproject returns NotApplicable at High confidence for exactly the repositories this control targets, without ever consulting the SI repository list. If gating on the scanned repo's releases is a deliberate single-repo-scanner trade-off, it should be at most Medium confidence and called out in the doc comment; otherwise the SI repository list should be consulted before ruling the control out.
2 participants
fixes #25