Skip to content

Encode "+" as "%2B" since "+" is a valid SemVer character.#17

Merged
wagoodman merged 2 commits intoanchore:masterfrom
tiegz:encode-plus-sign
Mar 12, 2024
Merged

Encode "+" as "%2B" since "+" is a valid SemVer character.#17
wagoodman merged 2 commits intoanchore:masterfrom
tiegz:encode-plus-sign

Conversation

@tiegz
Copy link

@tiegz tiegz commented Jan 23, 2024

Currently this library is generating PURLs like pkg:golang/github.com/azure/azure-sdk-for-go@v56.3.0+incompatible.

The PURL spec says that versions should be a "percent-encoded string", and while the + character is fine to use while escaping for application/x-www-form-urlencoded content, the original RFC3986 spec for general URIs does not mention using + to escape .

I think it's preferable to escape + because downstream parsers of the URL will likely unescape the + as , resulting in invalid PURL versions as in pkg:golang/github.com/azure/azure-sdk-for-go@v56.3.0 incompatible.

There's a good analysis of this problem in package-url/purl-spec#261 as well.

packageurl.go Outdated
Copy link
Author

@tiegz tiegz Jan 23, 2024

Choose a reason for hiding this comment

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

note that url.QueryEscape will encode +, but that method was replaced with PathEscape in #1

Copy link
Author

Choose a reason for hiding this comment

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

before the fix in this PR, this test would fail as:

packageurl_test.go:389: expected pkg:type/name/space/name@versio%20n%2Bbeta?key=value#sub/path to parse as pkg:type/name/space/name@versio%20n%2Bbeta?key=value#sub/path but got pkg:type/name/space/name@versio%20n+beta?key=value#sub/path

@tiegz
Copy link
Author

tiegz commented Jan 29, 2024

another example where not-escaping + is undesirable: "pkg:rpm/rhel/libstdc++@8.5.0-18.el8". Downstream Purl libraries will likely interpret that package name as "libstdc "

@RicardoAReyes
Copy link

Has the anchor community looked into merging this code?

Signed-off-by: Tieg Zaharia <tieg@tidelift.com>
@wagoodman wagoodman self-requested a review February 29, 2024 14:15
@wagoodman
Copy link

I'll work on getting CI passing -- thanks for the fix! 🙌

@wagoodman wagoodman self-assigned this Mar 12, 2024
Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Copy link

@wagoodman wagoodman left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@wagoodman wagoodman merged commit 8b9478f into anchore:master Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants