diff --git a/internal/database/queries.go b/internal/database/queries.go index 5d95596..6d329e2 100644 --- a/internal/database/queries.go +++ b/internal/database/queries.go @@ -443,11 +443,14 @@ func (db *DB) GetMostPopularPackages(limit int) ([]PopularPackage, error) { } type RecentPackage struct { - Ecosystem string `db:"ecosystem"` - Name string `db:"name"` - Version string `db:"version"` - CachedAt time.Time `db:"fetched_at"` - Size int64 `db:"size"` + Ecosystem string `db:"ecosystem"` + Name string `db:"name"` + VersionPURL string `db:"version_purl"` + CachedAt time.Time `db:"fetched_at"` + Size int64 `db:"size"` + // Version is derived from VersionPURL rather than selected, so that the + // PURL percent-encoding is decoded (e.g. "%2B" back to "+"). + Version string `db:"-"` } func (db *DB) GetRecentlyCachedPackages(limit int) ([]RecentPackage, error) { @@ -461,10 +464,10 @@ func (db *DB) GetRecentlyCachedPackages(limit int) ([]RecentPackage, error) { } var packages []RecentPackage - // We need to extract version from the purl since there's no separate version column + // There is no separate version column, so the full version PURL is selected + // and the version is decoded from it in Go. query := db.Rebind(` - SELECT p.ecosystem, p.name, - SUBSTR(v.purl, INSTR(v.purl, '@') + 1) as version, + SELECT p.ecosystem, p.name, v.purl as version_purl, a.fetched_at, COALESCE(a.size, 0) as size FROM artifacts a JOIN versions v ON v.purl = a.version_purl @@ -474,25 +477,13 @@ func (db *DB) GetRecentlyCachedPackages(limit int) ([]RecentPackage, error) { LIMIT ? `) - // For postgres, use different string function - if db.dialect == DialectPostgres { - query = db.Rebind(` - SELECT p.ecosystem, p.name, - SUBSTRING(v.purl FROM POSITION('@' IN v.purl) + 1) as version, - a.fetched_at, COALESCE(a.size, 0) as size - FROM artifacts a - JOIN versions v ON v.purl = a.version_purl - JOIN packages p ON p.purl = v.package_purl - WHERE a.storage_path IS NOT NULL AND a.fetched_at IS NOT NULL - ORDER BY a.fetched_at DESC - LIMIT ? - `) - } - err = db.Select(&packages, query, limit) if err != nil { return nil, err } + for i := range packages { + packages[i].Version = VersionFromPURL(packages[i].VersionPURL) + } return packages, nil } diff --git a/internal/database/types.go b/internal/database/types.go index 47dc47e..dae483e 100644 --- a/internal/database/types.go +++ b/internal/database/types.go @@ -2,6 +2,7 @@ package database import ( "database/sql" + "net/url" "strings" "time" ) @@ -47,10 +48,66 @@ type Version struct { // Version extracts the version string from the PURL. // e.g., "pkg:npm/lodash@4.17.21" -> "4.17.21" func (v *Version) Version() string { - if idx := strings.LastIndex(v.PURL, "@"); idx >= 0 { - return v.PURL[idx+1:] + return VersionFromPURL(v.PURL) +} + +// DisplayPURL returns the PURL with its path components percent-decoded, for +// showing in the UI. The stored PURL keeps the canonical encoding (which is +// what the API and all lookups use); this is only a readable rendering, so that +// a version like "7.91+dfsg1-2ubuntu0.1" is not shown as "7.91%2Bdfsg1-2ubuntu0.1" +// and an npm scope is shown as "@babel" rather than "%40babel". Qualifiers and +// subpath keep their encoding, since decoding those would be ambiguous. +func (v *Version) DisplayPURL() string { + base, suffix := v.PURL, "" + if i := strings.IndexAny(base, "?#"); i >= 0 { + base, suffix = base[:i], base[i:] + } + + name, version := base, "" + if idx := strings.LastIndex(base, "@"); idx >= 0 { + name, version = base[:idx], "@"+decodePURLComponent(base[idx+1:]) + } + + parts := strings.Split(name, "/") + for i, part := range parts { + parts[i] = decodePURLComponent(part) + } + return strings.Join(parts, "/") + version + suffix +} + +// VersionFromPURL extracts the decoded version string from a PURL. +// +// PURL percent-encodes characters that are not safe in a path component, so a +// Debian version like "7.91+dfsg1-2ubuntu0.1" is stored as +// "pkg:deb/nmap@7.91%2Bdfsg1-2ubuntu0.1". The raw substring after "@" is +// therefore not the version: it must be percent-decoded before being displayed +// or used to build a URL, otherwise "%2B" leaks into the UI and round-tripping +// the value back into a PURL double-encodes it. +// +// e.g., "pkg:npm/lodash@4.17.21" -> "4.17.21" +func VersionFromPURL(p string) string { + // Qualifiers ("?key=value") and subpath ("#path") follow the version. + if i := strings.IndexAny(p, "?#"); i >= 0 { + p = p[:i] + } + idx := strings.LastIndex(p, "@") + if idx < 0 { + return "" + } + return decodePURLComponent(p[idx+1:]) +} + +// decodePURLComponent percent-decodes a single PURL path component, returning +// the input unchanged if it is not valid percent-encoding. +func decodePURLComponent(s string) string { + if !strings.Contains(s, "%") { + return s + } + decoded, err := url.PathUnescape(s) + if err != nil { + return s } - return "" + return decoded } // Artifact represents a cached artifact in the database. diff --git a/internal/database/version_purl_test.go b/internal/database/version_purl_test.go new file mode 100644 index 0000000..1dc44a2 --- /dev/null +++ b/internal/database/version_purl_test.go @@ -0,0 +1,114 @@ +package database + +import ( + "database/sql" + "testing" + "time" +) + +func TestVersionFromPURL(t *testing.T) { + tests := []struct { + name string + purl string + want string + }{ + {"simple", "pkg:npm/lodash@4.17.21", "4.17.21"}, + {"namespaced", "pkg:composer/symfony/console@6.0.0", "6.0.0"}, + // Debian/Ubuntu versions routinely contain "+", which PURL encodes. + {"encoded plus", "pkg:deb/nmap@7.91%2Bdfsg1%2Breally7.80%2Bdfsg1-2ubuntu0.1", "7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1"}, + {"encoded epoch", "pkg:deb/curl@1%3A7.81.0-1", "1:7.81.0-1"}, + {"encoded plus with qualifier", "pkg:deb/nmap@7.91%2Bdfsg1?repository_url=http%3A%2F%2Fexample.com", "7.91+dfsg1"}, + {"tilde is not encoded", "pkg:deb/foo@1.0~rc1", "1.0~rc1"}, + {"no version", "pkg:npm/lodash", ""}, + {"invalid escape passed through", "pkg:npm/lodash@1.0%zz", "1.0%zz"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := VersionFromPURL(tt.purl); got != tt.want { + t.Errorf("VersionFromPURL(%q) = %q, want %q", tt.purl, got, tt.want) + } + v := &Version{PURL: tt.purl} + if got := v.Version(); got != tt.want { + t.Errorf("Version.Version() for %q = %q, want %q", tt.purl, got, tt.want) + } + }) + } +} + +func TestVersionDisplayPURL(t *testing.T) { + tests := []struct { + name string + purl string + want string + }{ + {"simple", "pkg:npm/lodash@4.17.21", "pkg:npm/lodash@4.17.21"}, + { + "encoded plus", + "pkg:deb/nmap@7.91%2Bdfsg1%2Breally7.80%2Bdfsg1-2ubuntu0.1", + "pkg:deb/nmap@7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1", + }, + { + "qualifier preserved", + "pkg:deb/nmap@7.91%2Bdfsg1?repository_url=http%3A%2F%2Fexample.com", + "pkg:deb/nmap@7.91+dfsg1?repository_url=http%3A%2F%2Fexample.com", + }, + // The namespace is encoded too: MakePURLString("npm", "@babel/core", …) + // produces "pkg:npm/%40babel/core@…". + {"encoded npm scope", "pkg:npm/%40babel/core@7.0.0", "pkg:npm/@babel/core@7.0.0"}, + {"encoded scope without version", "pkg:npm/%40babel/core", "pkg:npm/@babel/core"}, + {"no version", "pkg:npm/lodash", "pkg:npm/lodash"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v := &Version{PURL: tt.purl} + if got := v.DisplayPURL(); got != tt.want { + t.Errorf("DisplayPURL() for %q = %q, want %q", tt.purl, got, tt.want) + } + }) + } +} + +// TestGetRecentlyCachedPackagesDecodesVersion guards the dashboard's "recently +// cached" list, which derives the version from the version PURL. +func TestGetRecentlyCachedPackagesDecodesVersion(t *testing.T) { + runWithBothDatabases(t, func(t *testing.T, db *DB) { + const versionPURL = "pkg:deb/nmap@7.91%2Bdfsg1%2Breally7.80%2Bdfsg1-2ubuntu0.1" + + if err := db.UpsertPackage(&Package{ + PURL: "pkg:deb/nmap", Ecosystem: "deb", Name: "nmap", + }); err != nil { + t.Fatalf("UpsertPackage failed: %v", err) + } + if err := db.UpsertVersion(&Version{ + PURL: versionPURL, PackagePURL: "pkg:deb/nmap", + }); err != nil { + t.Fatalf("UpsertVersion failed: %v", err) + } + if err := db.UpsertArtifact(&Artifact{ + VersionPURL: versionPURL, + Filename: "nmap_7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1_amd64.deb", + UpstreamURL: "http://archive.ubuntu.com/ubuntu/pool/universe/n/nmap/nmap.deb", + StoragePath: sql.NullString{String: "/cache/nmap.deb", Valid: true}, + FetchedAt: sql.NullTime{Time: time.Now(), Valid: true}, + }); err != nil { + t.Fatalf("UpsertArtifact failed: %v", err) + } + + recent, err := db.GetRecentlyCachedPackages(10) + if err != nil { + t.Fatalf("GetRecentlyCachedPackages failed: %v", err) + } + if len(recent) != 1 { + t.Fatalf("expected 1 recent package, got %d", len(recent)) + } + const want = "7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1" + if recent[0].Version != want { + t.Errorf("Version = %q, want %q", recent[0].Version, want) + } + if recent[0].VersionPURL != versionPURL { + t.Errorf("VersionPURL = %q, want %q", recent[0].VersionPURL, versionPURL) + } + }) +} diff --git a/internal/handler/debian_test.go b/internal/handler/debian_test.go index dfdd326..d02337e 100644 --- a/internal/handler/debian_test.go +++ b/internal/handler/debian_test.go @@ -12,6 +12,11 @@ func TestDebianHandler_parsePoolPath(t *testing.T) { {"pool/main/libn/libncurses/libncurses6_6.2-1_amd64.deb", "libncurses6", "6.2-1", "amd64"}, {"pool/contrib/v/virtualbox/virtualbox_6.1.38-1_amd64.deb", "virtualbox", "6.1.38-1", "amd64"}, {"pool/main/g/git/git_2.39.2-1_arm64.deb", "git", "2.39.2-1", "arm64"}, + { + "pool/universe/n/nmap/nmap_7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1_amd64.deb", + "nmap", "7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1", "amd64", + }, + {"pool/main/o/openssl/openssl_3.0.2-0ubuntu1.15~build1_amd64.deb", "openssl", "3.0.2-0ubuntu1.15~build1", "amd64"}, {"invalid/path", "", "", ""}, {"pool/main/n/nginx/nginx.deb", "", "", ""}, }) diff --git a/internal/server/resolve.go b/internal/server/resolve.go index 51f203d..5876c33 100644 --- a/internal/server/resolve.go +++ b/internal/server/resolve.go @@ -2,6 +2,7 @@ package server import ( "fmt" + "net/url" "strings" "unicode" @@ -23,12 +24,25 @@ func validatePackagePath(path string) error { if len(path) > maxPackagePathLen { return fmt.Errorf("package path exceeds %d bytes", maxPackagePathLen) } - for _, r := range path { - if r == 0 { - return fmt.Errorf("package path contains null byte") + // Validate the decoded segments: the handlers work with decoded values, so + // an escape such as "%00" or "%2E%2E" must not slip past these checks. + for _, seg := range splitWildcardPath(path) { + // A decoded segment can itself contain slashes (from "%2F"), and the + // segments are later rejoined into a package name that registries + // interpolate straight into an upstream URL. Check every path element, + // not just the segment as a whole, or "a%2F..%2F..%2Fb" traverses. + for _, elem := range strings.Split(seg, "/") { + if elem == ".." { + return fmt.Errorf("package path contains parent directory segment") + } } - if unicode.IsControl(r) { - return fmt.Errorf("package path contains control character %#U", r) + for _, r := range seg { + if r == 0 { + return fmt.Errorf("package path contains null byte") + } + if unicode.IsControl(r) { + return fmt.Errorf("package path contains control character %#U", r) + } } } return nil @@ -60,10 +74,34 @@ func resolvePackageName(db *database.DB, ecosystem string, segments []string) (n // splitWildcardPath splits a chi wildcard path value into segments, // trimming any leading/trailing slashes. +// +// chi routes on the raw (still percent-encoded) path whenever the request URL +// contains an escape, so each segment is decoded after splitting. Splitting +// first keeps an encoded "%2F" inside a name from being mistaken for a +// separator. Decoding matters for versions such as "1.0%2Bbuild1", which must +// reach the handlers as "1.0+build1" so that rebuilding the PURL yields the +// value that was stored rather than a double-encoded one. func splitWildcardPath(path string) []string { path = strings.Trim(path, "/") if path == "" { return nil } - return strings.Split(path, "/") + segments := strings.Split(path, "/") + for i, seg := range segments { + segments[i] = decodePathSegment(seg) + } + return segments +} + +// decodePathSegment percent-decodes a single URL path segment, returning it +// unchanged if it is not valid percent-encoding. +func decodePathSegment(seg string) string { + if !strings.Contains(seg, "%") { + return seg + } + decoded, err := url.PathUnescape(seg) + if err != nil { + return seg + } + return decoded } diff --git a/internal/server/resolve_test.go b/internal/server/resolve_test.go index dd7d2dc..dd2b452 100644 --- a/internal/server/resolve_test.go +++ b/internal/server/resolve_test.go @@ -104,6 +104,14 @@ func TestSplitWildcardPath(t *testing.T) { {"symfony/console/6.0.0/browse", []string{"symfony", "console", "6.0.0", "browse"}}, {"", nil}, {"/", nil}, + // chi routes on the raw path when the URL contains an escape, so + // segments arrive percent-encoded and must be decoded. + {"nmap/7.91%2Bdfsg1%2Breally7.80%2Bdfsg1-2ubuntu0.1", []string{"nmap", "7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1"}}, + {"%40babel/core/7.0.0", []string{"@babel", "core", "7.0.0"}}, + // An encoded separator stays inside its segment rather than splitting. + {"vendor%2Fname/1.0.0", []string{"vendor/name", "1.0.0"}}, + // Invalid escapes are passed through untouched. + {"lodash/1.0%zz", []string{"lodash", "1.0%zz"}}, } for _, tt := range tests { @@ -132,8 +140,19 @@ func TestValidatePackagePath(t *testing.T) { {"composer namespaced", "symfony/console/6.0.0", false}, {"maven coordinates", "org.apache.commons/commons-lang3/3.12.0", false}, {"unicode", "café/1.0.0", false}, + {"encoded plus in version", "nmap/7.91%2Bdfsg1-2ubuntu0.1", false}, {"empty", "", true}, {"null byte", "lodash\x00/4.17.21", true}, + {"encoded null byte", "lodash/%00", true}, + {"encoded newline", "lodash/1.0%0A", true}, + {"parent segment", "lodash/../4.17.21", true}, + {"encoded parent segment", "lodash/%2E%2E/4.17.21", true}, + // A decoded segment can contain slashes, so traversal can hide inside + // one segment. Registries interpolate the resolved name straight into + // an upstream URL, and Go sends dot-segments verbatim. + {"traversal inside one segment", "pkg%2F..%2F..%2Fadmin", true}, + {"traversal via encoded dots and slash", "pkg%2f%2e%2e%2fadmin", true}, + {"encoded slash alone is allowed", "vendor%2Fname/1.0.0", false}, {"null byte suffix", "lodash\x00", true}, {"newline", "lodash\n4.17.21", true}, {"carriage return", "lodash\r", true}, diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 2d27147..54b6e99 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -764,6 +764,57 @@ func TestVersionShowPage_NotFoundServer(t *testing.T) { } } +// TestVersionShowPage_PlusInVersion covers Debian/Ubuntu style versions such as +// nmap's "7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1". PURL percent-encodes "+" as +// "%2B", so the UI must show the decoded version and resolve both the decoded +// and the still-encoded form of the URL back to the same version. +func TestVersionShowPage_PlusInVersion(t *testing.T) { + ts := newTestServer(t) + defer ts.close() + + const version = "7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1" + const versionPURL = "pkg:deb/nmap@7.91%2Bdfsg1%2Breally7.80%2Bdfsg1-2ubuntu0.1" + + pkg := &database.Package{PURL: "pkg:deb/nmap", Ecosystem: "deb", Name: "nmap"} + if err := ts.db.UpsertPackage(pkg); err != nil { + t.Fatalf("failed to upsert package: %v", err) + } + if err := ts.db.UpsertVersion(&database.Version{ + PURL: versionPURL, PackagePURL: pkg.PURL, + }); err != nil { + t.Fatalf("failed to upsert version: %v", err) + } + + // The package page must link to and display the decoded version. + req := httptest.NewRequest("GET", "/ui/package/deb/nmap", nil) + w := httptest.NewRecorder() + ts.handler.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Fatalf("package page: expected status 200, got %d", w.Code) + } + body := w.Body.String() + if strings.Contains(body, "%2B") { + t.Error("package page leaks PURL percent-encoding into the UI") + } + // html/template renders "+" as the "+" entity inside attributes and text. + if !strings.Contains(body, "7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1") { + t.Error("expected package page to show the decoded version") + } + + // Both the decoded and the encoded URL must reach the version page. + for _, path := range []string{ + "/ui/package/deb/nmap/" + version, + "/ui/package/deb/nmap/7.91%2Bdfsg1%2Breally7.80%2Bdfsg1-2ubuntu0.1", + } { + req := httptest.NewRequest("GET", path, nil) + w := httptest.NewRecorder() + ts.handler.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Errorf("GET %s: expected status 200, got %d", path, w.Code) + } + } +} + func TestPackageShowPage_WithLicense(t *testing.T) { ts := newTestServer(t) defer ts.close() diff --git a/internal/server/templates/pages/package_show.html b/internal/server/templates/pages/package_show.html index a66fd57..e9a7a7d 100644 --- a/internal/server/templates/pages/package_show.html +++ b/internal/server/templates/pages/package_show.html @@ -64,7 +64,7 @@

Versions ({{len .Versions}})

- {{.PURL}} + {{.DisplayPURL}} {{if .Yanked}}yanked{{end}}
{{if .PublishedAt.Valid}}{{.PublishedAt.Time.Format "2006-01-02"}}{{end}} diff --git a/internal/server/templates/pages/version_show.html b/internal/server/templates/pages/version_show.html index a30d8b5..15dafc4 100644 --- a/internal/server/templates/pages/version_show.html +++ b/internal/server/templates/pages/version_show.html @@ -1,4 +1,4 @@ -{{define "title"}}{{.Package.Name}}@{{.Version.PURL}} - git-pkgs proxy{{end}} +{{define "title"}}{{.Version.DisplayPURL}} - git-pkgs proxy{{end}} {{define "content"}}
@@ -9,7 +9,7 @@
{{template "ecosystem_badge" .Package.Ecosystem}} -

{{.Version.PURL}}

+

{{.Version.DisplayPURL}}

{{if .IsOutdated}} outdated {{end}}