Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: remediate to greater than or equal versions for github alerts #31393

Merged
merged 11 commits into from
Sep 18, 2024

Conversation

rarkins
Copy link
Collaborator

@rarkins rarkins commented Sep 15, 2024

Changes

Adds new field vulnerabilityFixVersion to use in place of allowedVersions for vulnerability rules.

Context

When we (currently) use allowedVersions for vulnerability fixes then it means we override/break any allowedVersions rules which users have configured.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

@rarkins rarkins requested a review from Churro September 15, 2024 04:40
@rarkins rarkins mentioned this pull request Sep 15, 2024
6 tasks
@rarkins
Copy link
Collaborator Author

rarkins commented Sep 15, 2024

Another reason for this refactor was to be able to enable #31395

@rarkins
Copy link
Collaborator Author

rarkins commented Sep 15, 2024

@Churro I didn't make the changes to OSV alerts yet. This functionality is intended to work side-by-side and allow a later refactoring

@rarkins rarkins changed the title refactor: use vulnerabilityFixVersion for github alerts feat: remediate to greater than or equal versions for github alerts Sep 16, 2024
Copy link
Collaborator

@Churro Churro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since matchCurrentVersion also matches version or range, for consistency I'd advocate calling the field vulnerabilityFixVersion. With vulnerabilityFix I'm somewhat lacking the "what it is"-suffix.

I tested this PR on https://github.com/renovate-demo/31393-github-alerts/pulls and the result of PRs created is basically identical to #29600 and the reproducer in https://github.com/renovate-demo/29600-github-vuln-alerts/pulls.

lib/workers/repository/process/lookup/index.ts Outdated Show resolved Hide resolved
lib/workers/repository/process/lookup/types.ts Outdated Show resolved Hide resolved
@Churro
Copy link
Collaborator

Churro commented Sep 16, 2024

I also tested it with OSV, changing only all occurrences of allowedVersions to vulnerabilityFix. The PR output is identical to the ones raised via GitHub alerts (https://github.com/renovate-demo/31393-osv-alerts/pulls) but the log included a warning:

DEBUG: Vulnerability GHSA-g3vv-g2j5-45f2 affects github.com/ipld/go-codec-dagpb v1.3.0 (repository=renovate-demo/31393-osv-alerts)
DEBUG: Setting allowed version >= 1.3.1 to fix vulnerability GHSA-967g-cjx4-h7j6 in github.com/ipld/go-codec-dagpb v1.3.0 (repository=renovate-demo/31393-osv-alerts)

 WARN: vulnerabilityFix is not valid (repository=renovate-demo/31393-osv-alerts)
       "vulnerabilityFix": ">= 1.3.1",
       "packageName": "github.com/ipld/go-codec-dagpb"
DEBUG: Vulnerability alert found: limiting results to a single release (repository=renovate-demo/31393-osv-alerts)
       "filteredReleases": [{"version": "v1.3.1", "releaseTimestamp": "2022-03-16T09:19:38.000Z"}]

The issue is that go uses semver and semver.isValid(">= 1.3.1") is false. It worked with allowedVersions due to this fallback:

} else if (
config.versioning !== npmVersioning.id &&
semver.validRange(allowedVersions)
) {
logger.debug(
{ depName: config.depName },
'Falling back to npm semver syntax for allowedVersions',
);
filteredReleases = filteredReleases.filter((r) =>
semver.satisfies(
semver.valid(r.version)
? r.version
: /* istanbul ignore next: not reachable, but it's safer to preserve it */ semver.coerce(
r.version,
)!,
allowedVersions,
),
);

However, this shouldn't be an issue with GitHub alerts, as they always return a single version. Just mentioning it with regards to porting this change also to OSV. If OSV advisories use "last affected", it will inevitably result in a constraint like "> 1.2.3".

@rarkins
Copy link
Collaborator Author

rarkins commented Sep 17, 2024

@Churro regarding using OSV, can we return vulnerabilityFixVersion as a single version? The warning you describe above is due to Renovate go versioning not having the concept of range syntax plus the lack of semver fallback you already identified. The thing is that in go versioning, ">= 1.3.1" is unnecessary anyway - "1.3.1" is essentially the same thing (although technically it's like ^1.3.1 and does not go past 2.0.0).

@rarkins rarkins requested a review from Churro September 17, 2024 05:15
@Churro
Copy link
Collaborator

Churro commented Sep 17, 2024

@Churro regarding using OSV, can we return vulnerabilityFixVersion as a single version? The warning you describe above is due to Renovate go versioning not having the concept of range syntax plus the lack of semver fallback you already identified. The thing is that in go versioning, ">= 1.3.1" is unnecessary anyway - "1.3.1" is essentially the same thing (although technically it's like ^1.3.1 and does not go past 2.0.0).

With OSV, it depends which fix info is provided in the advisory:

  • with explicit fix version -> same versioning.isGreaterThan() check will work as for first_patched_version in GH alerts. Of course, after reworking the impl to no longer build constraints like >=.
  • with only a "last affected" version, there will be a constraint like > 1.3.1. Not >= but really >, which is why specifying "1.3.1" only wouldn't be enough. In that case, the same warning would show up since semver.valid("> 1.3.1") is also false.

A workaround to omit the fallback would be to use npmVersioning for go (and probably others with semver), since it also has support for ranges. Probably there are downsides to that approach that I'm not aware of.

@rarkins
Copy link
Collaborator Author

rarkins commented Sep 17, 2024

@Churro let's discuss that later on then. This PR already has more logic than it needs to work here, i.e. the support for ranges is unnecessary for the github alerts. If OSV requires special treatment then I'd prefer to do it there

Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise LGTM

lib/workers/repository/process/lookup/index.ts Outdated Show resolved Hide resolved
lib/workers/repository/process/lookup/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should split to smaller functions in a follow up PR

@rarkins rarkins added this pull request to the merge queue Sep 18, 2024
Merged via the queue into main with commit dce6912 Sep 18, 2024
36 checks passed
@rarkins rarkins deleted the refactor/vulnerability-fix-version branch September 18, 2024 10:18
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 38.88.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants