fix: decode PURL percent-encoding in versions and package paths - #244
Open
wickedOne wants to merge 1 commit into
Open
fix: decode PURL percent-encoding in versions and package paths#244wickedOne wants to merge 1 commit into
wickedOne wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
another claude generated patch to fix the following behaviour:

Problem
Package versions containing
+were unusable in the UI. Reported againstnmap_7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1_amd64.debfrom the Ubuntu archive (PROXY_UPSTREAM_DEBIAN=http://archive.ubuntu.com/ubuntu), which displayed as@7.91%2Bdfsg1%2Breally7.80%2Bdfsg1-2ubuntu0.1.The artifact download itself was always fine — the bug was in reading the version back out of the PURL. PURL percent-encodes
+in path components, so the version is stored aspkg:deb/nmap@7.91%2Bdfsg1%2B…. Two places treated the raw substring after@as the version:Version.Version()(strings.LastIndex(purl, "@")) and the SQLSUBSTR(v.purl, INSTR(v.purl,'@') + 1)inGetRecentlyCachedPackagesnever decoded, so%2Bleaked into the dashboard and package pages.r.URL.RawPathwhenever the URL contains an escape, so the handler received7.91%2B…back and re-encoded it intopkg:deb/nmap@7.91%252B…. The version page 404'd.This affects any ecosystem with non-alphanumeric version characters, not just Debian: npm scoped packages had the same problem via
%40.Changes
internal/database/types.go— addVersionFromPURL(strips qualifiers/subpath, then percent-decodes);Version.Version()uses it. AddVersion.DisplayPURL()for a readable PURL in the UI, decoding all path components.internal/database/queries.go—GetRecentlyCachedPackagesselectsv.purland derives the version in Go. This also removes the duplicated Postgres-specificSUBSTRING/POSITIONvariant of the query.internal/server/resolve.go—splitWildcardPathpercent-decodes each segment after splitting, so an encoded%2Finside a name is not mistaken for a separator.DisplayPURLfor version headings and labels.Storage is unchanged. Stored PURLs keep their canonical encoding (
%2B,%40), which is what the API, cache keys and all lookups use. This is display and inbound routing only, so no cache migration is needed. Old encoded URLs still resolve, so existing bookmarks keep working.Security
validatePackagePathnow validates the decoded segments and rejects..path elements. This closes a traversal vector: package names are rejoined and interpolated straight into upstream URLs by several registries (e.g.fmt.Sprintf("%s/web/packages/%s/DESCRIPTION", baseURL, name)for CRAN), and Go sends dot-segments verbatim for the upstream to resolve.GET /api/package/cran/pkg%2F..%2F..%2Fadminnow returns 400 instead of reaching upstream as/web/packages/pkg/../../admin/DESCRIPTION.Note this is stricter than before the change: the original code had no
..check at all, so a literal../..also passed validation.Testing
internal/database/version_purl_test.go(VersionFromPURL,DisplayPURL, andGetRecentlyCachedPackagesdecoding under both sqlite and Postgres).resolve_test.go(segment decoding, traversal rejection),server_test.go(version page reachable via both encoded and decoded URLs),debian_test.go(pool paths with+and~).+,+after-,~dfsg, leading0~, and npm@scope.~is correctly left unencoded (it is PURL-safe).golangci-lintpass; the 6 remaining staticcheck warnings are pre-existing in untouched test files.