-
Notifications
You must be signed in to change notification settings - Fork 47
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: fix parsing of names and namespaces with colons #178
base: main
Are you sure you want to change the base?
feat: fix parsing of names and namespaces with colons #178
Conversation
This attempts to comply with the specification. Closes package-url#152 (I think) See discussion at package-url#152 Signed-off-by: Joshua Kugler <[email protected]>
Howdy @tdruez, could you have a look? Thanks! |
@jkugler Thanks. You may want to run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Do you have actual real package names and namespaces with colons, rather than made up ones?
|
||
assert p.type == "nuget" | ||
assert p.namespace is None | ||
assert p.name == "libiconv: character set conversion library" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a real name, seen in the wild? I do not think this would be a valid NuGet name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the name that started me down this nightmare of journey. :) It's a name picked up by syft in a binary component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the name that started me down this nightmare of journey. :) It's a name picked up by syft in a binary component.
Do you have a URL to this? Because afaik this is not a legal NuGet name. It could be a bug in syft, which is known to create incorrect PURLs.
|
||
def test_colons_in_namespace_are_handled_correctly() -> None: | ||
p = PackageURL.from_string( | ||
"pkg:nuget/an:odd:space/libiconv:%20character%20set%20conversion%[email protected]?package-id=e11a609df352e292" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use actual real life data rather than made up ones? In all cases the colon should be encoded there IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't seen a :
in a namespace, but just in a name. I used a real-life name in this test that I had found in an SBOM that was given to us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so you saw a colon in a namespace in a PURL? I would really like to see that exact PURL and which tool did create that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not have a namespace with a colon, but I did see a name with a colon.
Change a bunch of things I never touched, but ok... :) Signed-off-by: Joshua Kugler <[email protected]>
It would be great if we could move forward with this, as this is a huge blocker for us. Thanks! |
Signed-off-by: Joshua Kugler <[email protected]>
This attempts to comply with the specification; at minimum brings it in line with what all the other libraries do.
Closes #152
(I think)
See comment at #152 (comment)