Repository: package-url/packageurl-python

Issue:
Creation PackageURL objects for invalid PURLs / Possible improper encoding of colons (":") in PURL fields
It is possible to create PackageURL objects that contain invalid fields, specifically by using the `PackageURL` kwarg constructor and passing in values that contain colons.

Simple example:
```
>>> from packageurl import PackageURL
>>> p = PackageURL(type="generic", name="Foo: <Bar>", version="1.2.3")
>>> p.to_string()
'pkg:generic/Foo:%20%3CBar%3E@1.2.3'
>>> PackageURL.from_string(p.to_string())
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/vossn/finitestate/finite-state-sip/venv/lib/python3.10/site-packages/packageurl/__init__.py", line 514, in from_string
    raise ValueError(msg)
ValueError: Invalid purl 'pkg:generic/Foo:%20%3CBar%3E@1.2.3' cannot contain a "user:pass@host:port" URL Authority component: ''.
```

On closer inspection, it looks like the problem might be that colons (`:`) are not being percent-encoded correctly? I would expect the colon in the name to be encoded to `%3A`, but it looks like it is being left as a literal `:` in the `to_string()` function:
```
>>> p = PackageURL(type="generic", name="Foo: <Bar>", version="1.2.3")
>>> p
PackageURL(type='generic', namespace=None, name='Foo: <Bar>', version='1.2.3', qualifiers={}, subpath=None)
>>> p.to_string()
'pkg:generic/Foo:%20%3CBar%3E@1.2.3'
```

I'm not sure I'm interpreting the [PURL spec](https://github.com/package-url/purl-spec/blob/master/PURL-SPECIFICATION.rst) correctly with regards to the treatment of colon characters, but on the surface it sounds like any colon appearing within an individual field value should simply be percent-encoded if the field itself calls for it.

Discussion:
@itookyourboo: Totally agree. Quote from PURL specification:

> A name must be a percent-encoded string
@mprpic: Colons are explicitly not encoded for an unknown reason:

https://github.com/package-url/packageurl-python/blob/f98abf0f3c295873e18f968ebd00138a02d63b25/src/packageurl/__init__.py#L71C40-L71C40

This line was added as part of commit d7be0209d00fefd819d27804b1ee536765e6509e, but there is no explanation as to why. This applies to all fields, not just name, fwiw.

@pombredanne as the author of this particular piece of the code, can you share why colons are treated special in this context? :-)@jkugler: I just had this long discussion with the folks at Syft to fix a bug in their tool which caused improper PURLs to be generated. Colons in names, namespaces, in versions (probably) need to be escaped.

See this bug:
```py
>>> p = PackageURL.from_string('pkg:nuget/libiconv%3A%20character%20set%20conversion%20library@1.9?package-id=e11a609df352e292')
>>> p.to_string()
'pkg:nuget/libiconv:%20character%20set%20conversion%20library@1.9?package-id=e11a609df352e292'
>>> PackageURL.from_string("pkg:nuget/libiconv:%20character%20set%20conversion%20library@1.9?package-id=e11a609df352e292")
Traceback (most recent call last):
  File "<input>", line 1, in <module>
    PackageURL.from_string("pkg:nuget/libiconv:%20character%20set%20conversion%20library@1.9?package-id=e11a609df352e292")
  File "/Users/tek30584/repos/asset/kodiak-orchestrator/.venv.python312/lib/python3.12/site-packages/packageurl/__init__.py", line 508, in from_string
    raise ValueError(msg)
ValueError: Invalid purl 'pkg:nuget/libiconv:%20character%20set%20conversion%20library@1.9?package-id=e11a609df352e292' cannot contain a "user:pass@host:port" URL Authority component: ''.
```
Also:
```py
>>> p = PackageURL(type="nuget", name="libiconv: character set conversion library", version="1.9", qualifiers={'package-id': 'e11a609df352e292'})
>>> p
PackageURL(type='nuget', namespace=None, name='libiconv: character set conversion library', version='1.9', qualifiers={'package-id': 'e11a609df352e292'}, subpath=None)
>>> p.to_string()
'pkg:nuget/libiconv:%20character%20set%20conversion%20library@1.9?package-id=e11a609df352e292'
```

This is a huge blocker for us, so I'm going to work on a PR.@jkugler: OK, so let's talk about the standard, from here: https://github.com/package-url/purl-spec/blob/master/PURL-SPECIFICATION.rst

Namespace:
> Each namespace segment must be a percent-encoded string

Name:
> A name must be a percent-encoded string

Version:
> A version must be a percent-encoded string

Qualifiers:
> A key must NOT be percent-encoded

> A value must be a percent-encoded string

Subpath:
> Each subpath segment must be a percent-encoded string

So, it would appear there are incorrect tests. For example:
```
E       AssertionError: assert 'pkg:docker/c...ry_url=gcr.io' == 'pkg:docker/c...ry_url=gcr.io'
E
E         - pkg:docker/customer/dockerimage@sha256%3A244fd47e07d1004f0aed9c?repository_url=gcr.io
E         ?                                       ^^^
E         + pkg:docker/customer/dockerimage@sha256:244fd47e07d1004f0aed9c?repository_url=gcr.io
E         ?
```
Hmm...does this mean the test data is wrong?
```
  {
    "description": "docker uses qualifiers and hash image id as versions",
    "purl": "pkg:docker/customer/dockerimage@sha256:244fd47e07d1004f0aed9c?repository_url=gcr.io",
    "canonical_purl": "pkg:docker/customer/dockerimage@sha256:244fd47e07d1004f0aed9c?repository_url=gcr.io",
    "type": "docker",
    "namespace": "customer",
    "name": "dockerimage",
    "version": "sha256:244fd47e07d1004f0aed9c",
    "qualifiers": {"repository_url": "gcr.io"},
    "subpath": null,
    "is_invalid": false
  },
```
If the version is supposed to be percent-encoded, then a version of `sha256:244fd47e07d1004f0aed9c` should end up as `sha256%3A244fd47e07d1004f0aed9c`

How do we go forward with this?

This is also incorrect:
```
E       AssertionError: 'pkg:[42 chars]https://github.com/z3APA3A/3proxy/releases/dow[26 chars].zip' != 'pkg:[42 chars]https%3A//github.com/z3APA3A/3proxy/releases/d[28 chars].zip'
E       - pkg:github/z3apa3a/3proxy@0.8.11?download_url=https://github.com/z3APA3A/3proxy/releases/download/0.8.11/3proxy-0.8.11.zip
E       ?                                                    ^
E       + pkg:github/z3apa3a/3proxy@0.8.11?download_url=https%3A//github.com/z3APA3A/3proxy/releases/download/0.8.11/3proxy-0.8.11.zip
```
The spec says the value of a qualifier *must be* percent-encoded. Thus, the `:` and `/` characters in the URL should be percent encoded.

Where do I go from here?@matt-phylum: > The spec says the value of a qualifier _must be_ percent-encoded. Thus, the `:` and `/` characters in the URL should be percent encoded.

This is a misunderstanding about what "percent-encoded" means. `:` and `/` are not supposed to be percent encoded in qualifiers: https://github.com/package-url/purl-spec/blob/master/PURL-SPECIFICATION.rst#character-encoding@mprpic: @matt-phylum I don't think that's right, the spec says `:` and `/` are not supposed to be encoded _only_ when used as separators of the individual fields, for example the colon between `pkg:deb/...` or the slash in `pkg:pypi/django...`. When used in the values of individual fields, they must be encoded.@matt-phylum: That is what the spec says, but it's not very clear about it. "It is unambiguous unencoded everywhere" means it is never percent encoded. However, it is actually wrong in some cases. For example, `/` in the name is ambiguous and must be encoded, but I don't know of any current types that do this.@jkugler: Please see the comment above: https://github.com/package-url/packageurl-python/issues/152#issuecomment-2635487896

package-url python can't de-serialize a string it generates, instead throwing an error. I'm pretty sure `:` in a name or namespace should be encoded. A `:` or a `/` in a qualifier value could be left up for debate.@matt-phylum: That's a bug in packageurl-python's parsing code. No other PURL implementation that I am aware of fails in this way.

althonos/packageurl.rs, anchore/packageurl-go, giterlizzi/perl-URI-PackageURL, package-url/packageurl-dotnet, package-url/packageurl-go, package-url/packageurl-java, package-url/packageurl-js, package-url/packageurl-php, package-url/packageurl-ruby, package-url/packageurl-swift, phylum-dev/purl, sonatype/package-url-java all decode the PURL with the unencoded colon correctly.

maennchen/purl successfully parses but doesn't decode the name correctly: https://github.com/maennchen/purl/issues/10@jkugler: That's really interesting. I mean, the error message isn't *wrong* as a PURL `cannot contain a "user:pass@host:port" URL Authority component` but does that mean there should be no un-encoded `:` in the name to avoid ambiguity? The Spec does say a PURL is a URL, so a `:` in the name makes it look like there is "login" information in the URL.

> \<long list of libraries\> all decode the PURL with the unencoded colon correctly.

Just curious: are there tests that show it decoding a PURL with a `:` in the name or namespace correctly? Or did you run examples for all those languages as a test?@matt-phylum: I ran everything. I found that I often wanted to know how all the different PURL implementations would handle things so I wrote https://github.com/phylum-dev/purl-survey which provides a purl->json and json->purl for every implementation that I know of. It's been really useful for things like the ` `/`+`/`%2F`/`%2B` problem.@jkugler: @matt-phylum Awesome!

So...sounds like the PR needs to fix the decoding...not the encoding.@matt-phylum: Is there really a package named "libiconv: character set conversion library"? I thought NuGet IDs could not contain spaces.@jkugler: I believe the "name" comes from the binary found by Syft. We recently closed a bug with syft where they now encode `:` found in names and namespaces. But we still have old SBOMs around with `:` in names, so we need to be able to handle them, especially if packageurl-python is also generating names with `:` in them. :(@jkugler: I've opened a PR. I hope it can be merged quickly and unblock my team. :)@matt-phylum: Syft is making `pkg:nuget` PURLs in this file: https://github.com/anchore/syft/blob/e584c9f416028051fbdc80fffda4e624aaed528d/syft/pkg/cataloger/dotnet/parse_dotnet_portable_executable.go

Get the name of a DLL from the resources (this is wrong):
https://github.com/anchore/syft/blob/e584c9f416028051fbdc80fffda4e624aaed528d/syft/pkg/cataloger/dotnet/parse_dotnet_portable_executable.go#L57

Look up the name from the ProductName resource:
https://github.com/anchore/syft/blob/e584c9f416028051fbdc80fffda4e624aaed528d/syft/pkg/cataloger/dotnet/parse_dotnet_portable_executable.go#L202-L222

Generate a PURL using that name:
https://github.com/anchore/syft/blob/e584c9f416028051fbdc80fffda4e624aaed528d/syft/pkg/cataloger/dotnet/parse_dotnet_portable_executable.go#L83

The specific iconv string likely comes from some variant of libiconv.rc:

https://github.com/winlibs/libiconv/blob/880a1fa8b5581e37e136a7b051947d3ea39097b6/MSVC14/libiconv.rc#L35

It might work for cases where the file comes from a NuGet package and the project producing the NuGet package is using the .Net SDK to build the package and neither the name of the package nor the product name have been customized. Syft is making a big assumption and will often be wrong. In this case it's producing a PURL that refers to a package that AFAIK cannot exist because its name contains characters forbidden by the [nuspec spec](https://learn.microsoft.com/en-us/nuget/reference/nuspec#id), but it could produce the name of a different NuGet package.@pombredanne: @matt-phylum Thanks for the detailed research in https://github.com/package-url/packageurl-python/issues/152#issuecomment-2640719424

The issue there is to consider windows PE metadata as mapping to a nuget which is not correct at any scale. It may randomly work in some cases, but this is by chance.

Solution patch:
diff --git a/src/packageurl/__init__.py b/src/packageurl/__init__.py
index 8199e39..9677b73 100644
--- a/src/packageurl/__init__.py
+++ b/src/packageurl/__init__.py
@@ -459,12 +459,17 @@ class PackageURL(
             url=remainder, scheme="", allow_fragments=True
         )
 
-        if scheme or authority:
-            msg = (
-                f'Invalid purl {purl!r} cannot contain a "user:pass@host:port" '
-                f"URL Authority component: {authority!r}."
-            )
-            raise ValueError(msg)
+        # The spec (seems) to allow colons in the name and namespace.
+        # urllib.urlsplit splits on : considers them parts of scheme
+        # and authority.
+        # Other libraries do not care about this.
+        # See https://github.com/package-url/packageurl-python/issues/152#issuecomment-2637692538
+        # We do + ":" + to put the colon back that urlsplit removed.
+        if authority:
+            path = authority + ":" + path
+
+        if scheme:
+            path = scheme + ":" + path
 
         path = path.lstrip("/")
 


Test patch:
diff --git a/tests/test_packageurl.py b/tests/test_packageurl.py
index cb419e2..5d14220 100644
--- a/tests/test_packageurl.py
+++ b/tests/test_packageurl.py
@@ -330,3 +330,47 @@ class NormalizePurlTest(unittest.TestCase):
 def test_purl_is_hashable():
     s = {PackageURL(name="hashable", type="pypi")}
     assert len(s) == 1
+
+
+def test_colons_in_name_are_handled_correctly() -> None:
+    p = PackageURL.from_string(
+        "pkg:nuget/libiconv:%20character%20set%20conversion%20library@1.9?package-id=e11a609df352e292"
+    )
+
+    assert p.type == "nuget"
+    assert p.namespace is None
+    assert p.name == "libiconv: character set conversion library"
+    assert p.version == "1.9"
+    assert p.qualifiers == {"package-id": "e11a609df352e292"}
+    assert p.subpath is None
+
+    assert PackageURL.from_string(p.to_string()).to_string() == p.to_string()
+
+
+def test_colons_in_namespace_are_handled_correctly() -> None:
+    p = PackageURL.from_string(
+        "pkg:nuget/an:odd:space/libiconv:%20character%20set%20conversion%20library@1.9?package-id=e11a609df352e292"
+    )
+
+    assert p.type == "nuget"
+    assert p.namespace == "an:odd:space"
+    assert p.name == "libiconv: character set conversion library"
+    assert p.version == "1.9"
+    assert p.qualifiers == {"package-id": "e11a609df352e292"}
+    assert p.subpath is None
+
+    assert PackageURL.from_string(p.to_string()).to_string() == p.to_string()
+
+
+def test_encoding_stuff_with_colons_correctly() -> None:
+    p = PackageURL(
+        type="nuget",
+        namespace="an:odd:space",
+        name="libiconv: character set conversion library",
+        version="1.9",
+        qualifiers={"package-id": "e11a609df352e292"},
+    )
+    assert (
+        p.to_string()
+        == "pkg:nuget/an:odd:space/libiconv:%20character%20set%20conversion%20library@1.9?package-id=e11a609df352e292"
+    )