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

Parse purls cautiously in getDeniedChanges #753

Merged
merged 7 commits into from Apr 29, 2024
Merged

Parse purls cautiously in getDeniedChanges #753

merged 7 commits into from Apr 29, 2024

Conversation

juxtin
Copy link
Contributor

@juxtin juxtin commented Apr 26, 2024

Closes #752

Bottom line: the problems in the linked issue appear to be caused by the fact that we've been playing fast and loose with package-urls in a few different ways:

  1. For deny-groups, we try to accept package-urls with a namespace but no name, but those kinds of package-urls are invalid according to the spec. In order to get around that, we sort of abuse the packageurl-js library.
  2. While comparing actual dependencies against denied dependencies, we try to use the package_url member from the comparison API. However, it appears that this isn't always populated. Specifically, when a dependency has a version range like ^1.2.3, there's no way to format it into a package-url, so we just don't. These missing package_urls have been causing runtime exceptions.
  3. While it's true that the purl-spec requires the namespace, name, and version fields to be percent-encoded, most implementations will tolerate a purl like pkg:npm/@ns/pkg@1.0.0 even though it's technically invalid. This is helpful for our users, many of whom probably don't know the minutiae of the package-url spec. The packageurl-js library does not make this affordance.

What I've done in this PR is write a very simple, permissive parser for package-urls that fits our purposes more closely than packageurl-js does. It allows us to parse partial purls, reports errors without throwing them, and tolerates special characters as long as the whole purl can still be unambiguously parsed.

In addition, I've made the following changes/improvements:

  1. Standardized on a format for deny-groups that requires a namespace to be present. We used to tell users to use a purl fragment like pkg:npm/express to block the express namespace, but pkg:npm/express is a complete and valid purl that specifies the express package. Instead, the format should be pkg:npm/express/, where the trailing/ indicates that express is a namespace and not a package name.
  2. Better config validation up front, so that users will know if they've included a package-url that we can't actually use. Validation for deny-groups requires a namespace.
  3. If there's no package_url for a dependency, get the namespace from the package name.

In my test repositories reproducing the error reports we've gotten from users, these changes have successfully fixed the problems.

@juxtin juxtin requested a review from a team as a code owner April 26, 2024 21:29
@juxtin juxtin force-pushed the juxtin/debug-purl branch 7 times, most recently from 1e3e4d6 to abc088e Compare April 26, 2024 23:10
@juxtin juxtin marked this pull request as draft April 26, 2024 23:30
@juxtin juxtin marked this pull request as ready for review April 29, 2024 03:28
Copy link
Contributor

@bteng22 bteng22 left a comment

Choose a reason for hiding this comment

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

Just a few clarifying comments, but overall LGTM 👍 Thanks for the help on this one and sorry for the release headaches. It should be have been a smell when we had to hack packageurl-js to fit our use case.

The build is breaking, but it looks like it's because it's pulling from the main branch and still using the packageurl-js so we should be good to merge here?

@@ -59,7 +59,7 @@ inputs:
description: A comma-separated list of package URLs to deny (e.g. "pkg:npm/express, pkg:pypi/pycrypto"). If version specified, only deny matching packages and version; else, deny all regardless of version.
required: false
deny-groups:
description: A comma-separated list of package URLs for group(s)/namespace(s) to deny (e.g. "pkg:npm/express, pkg:pypi/pycrypto")
description: A comma-separated list of package URLs for group(s)/namespace(s) to deny (e.g. "pkg:npm/express/, pkg:pypi/pycrypto/"). Please note that the group name must be followed by a `/`.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

__tests__/deny.test.ts Show resolved Hide resolved
__tests__/purl.test.ts Show resolved Hide resolved
src/purl.ts Show resolved Hide resolved
@juxtin
Copy link
Contributor Author

juxtin commented Apr 29, 2024

The build is breaking, but it looks like it's because it's pulling from the main branch and still using the packageurl-js so we should be good to merge here?

That's right, yeah.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v4.3.0 Causing PURL Processing Errors
2 participants