diff --git a/internal/handler/pypi.go b/internal/handler/pypi.go index f5a0232..713f6c8 100644 --- a/internal/handler/pypi.go +++ b/internal/handler/pypi.go @@ -19,7 +19,14 @@ const ( minWheelParts = 5 // name + version + python + abi + platform minSubmatchParts = 2 // full match + first capture group minPyPIPathParts = 3 // hash_prefix + hash + filename - minPythonTagLen = 2 // minimum length for a python tag (e.g., "py") + minEggParts = 3 // name + version + python tag + + // PyPIMetadataSuffix is the PEP 658 core-metadata sidecar suffix that pip + // appends to a distribution URL when the index advertises core metadata. + // A sidecar resolves to the same name and version as the distribution it + // describes, so it is cached alongside it; consumers that expect an openable + // archive must skip these. + PyPIMetadataSuffix = ".metadata" ) // PyPIHandler handles PyPI registry protocol requests. @@ -433,56 +440,163 @@ func (h *PyPIHandler) handleDownload(w http.ResponseWriter, r *http.Request) { ServeArtifact(w, result) } +// archiveExtensions are sdist formats of the form {name}-{version}{ext}. They +// carry no trailing tags, but legacy sdist names may contain hyphens. +var archiveExtensions = []string{".tar.gz", ".tar.bz2", ".tar.xz", ".tar.Z", ".tgz", ".tar", ".zip"} + +// windowsInstallerExtensions are the legacy distutils bdist_wininst and +// bdist_msi formats, which share a filename layout. +var windowsInstallerExtensions = []string{".exe", ".msi"} + // parseFilename extracts package name and version from a PyPI filename. -// Handles both wheels and sdists: +// Handles wheels, sdists and legacy bdist formats: // - requests-2.31.0-py3-none-any.whl // - requests-2.31.0.tar.gz +// - numpy-1.8.0-py2.7-macosx-10.9-x86_64.egg +// - numpy-1.8.0.win32-py2.7.exe func (h *PyPIHandler) parseFilename(filename string) (name, version string) { - // Try wheel format first: {name}-{version}(-{build})?-{python}-{abi}-{platform}.whl - if strings.HasSuffix(filename, ".whl") { - base := strings.TrimSuffix(filename, ".whl") - parts := strings.Split(base, "-") - if len(parts) >= minWheelParts { - // Find where version ends (version followed by python tag) - for i := 1; i < len(parts)-2; i++ { - // Check if this looks like a python tag (py2, py3, cp39, etc) - if isPythonTag(parts[i]) { - name = strings.Join(parts[:i-1], "-") - version = parts[i-1] - return - } - } + // PEP 658/714 core-metadata sidecars are the distribution filename plus + // ".metadata"; they describe the same name and version. Without this, pip's + // metadata-only fetches fall back to a hash-derived package identifier. + filename = strings.TrimSuffix(filename, PyPIMetadataSuffix) + + switch { + case strings.HasSuffix(filename, ".whl"): + return parseWheelFilename(strings.TrimSuffix(filename, ".whl")) + case strings.HasSuffix(filename, ".egg"): + return parseEggFilename(strings.TrimSuffix(filename, ".egg")) + } + + for _, ext := range windowsInstallerExtensions { + if strings.HasSuffix(filename, ext) { + return parseWindowsInstallerFilename(strings.TrimSuffix(filename, ext)) } } - // Try sdist formats: {name}-{version}.tar.gz, {name}-{version}.zip - for _, ext := range []string{".tar.gz", ".tar.bz2", ".zip", ".tar"} { + for _, ext := range archiveExtensions { if strings.HasSuffix(filename, ext) { - base := strings.TrimSuffix(filename, ext) - // Find last hyphen followed by version - for i := len(base) - 1; i >= 0; i-- { - if base[i] == '-' && i+1 < len(base) && isVersionStart(base[i+1]) { - return base[:i], base[i+1:] - } - } + return splitNameVersion(strings.TrimSuffix(filename, ext)) } } return "", "" } -func isPythonTag(s string) bool { - if len(s) < minPythonTagLen { +// parseWheelFilename parses the PEP 427 layout +// {name}-{version}(-{build})?-{python}-{abi}-{platform}, base being the +// filename without its ".whl" suffix. The spec escapes every hyphen in the name +// and version to '_', so the first two fields are authoritative even when the +// optional build tag is present. +func parseWheelFilename(base string) (name, version string) { + parts := strings.Split(base, "-") + if len(parts) < minWheelParts { + return "", "" + } + + return parts[0], parts[1] +} + +// parseEggFilename parses the setuptools bdist_egg layout +// {name}-{version}-py{X.Y}(-{platform})?, base being the filename without its +// ".egg" suffix. setuptools escapes hyphens in the name and version to '_', but +// eggs built by other tooling do not always, so the version is located relative +// to the interpreter field rather than assumed to be the second field. +func parseEggFilename(base string) (name, version string) { + parts := strings.Split(base, "-") + // Scan from the end: the trailing platform fields never look like an + // interpreter tag, so the last match is the real one even when the package + // name itself carries a "py{N}" component. Stop before index 1, since a tag + // any earlier would leave no room for both a name and a version. + for i := len(parts) - 1; i >= minEggParts-1; i-- { + if !isEggPythonTag(parts[i]) || !isVersionField(parts[i-1]) { + continue + } + + return strings.Join(parts[:i-1], "-"), parts[i-1] + } + + // No interpreter field: {name}-{version}. + return splitNameVersion(base) +} + +// parseWindowsInstallerFilename parses the distutils bdist_wininst and +// bdist_msi layout {name}-{version}.{platform}(-py{X.Y})?, base being the +// filename without its ".exe" or ".msi" suffix. The platform is joined to the +// version with a '.' rather than a '-' and may itself contain a hyphen +// ("win-amd64"), so both trailing fields are stripped before the name and +// version are split apart. +func parseWindowsInstallerFilename(base string) (name, version string) { + if i := strings.LastIndex(base, "-py"); i >= 0 && isDottedNumber(base[i+len("-py"):]) { + base = base[:i] + } + + // The platform is the final '.'-separated field. Requiring it to start with + // a non-digit keeps a dotted version from being truncated when a filename + // carries no platform tag. + i := strings.LastIndex(base, ".") + if i < 0 || i+1 >= len(base) || isVersionStart(base[i+1]) { + return "", "" + } + + return splitFullname(base[:i]) +} + +// splitFullname splits the distutils fullname {name}-{version} that precedes a +// Windows installer's platform field. Unlike an sdist, a wininst fullname may +// carry a trailing build variant ("cx_Oracle-5.1.2-11g"), which belongs to +// neither the name nor the version, so the first purely numeric field wins and +// anything after it is discarded. +func splitFullname(fullname string) (name, version string) { + parts := strings.Split(fullname, "-") + for i := 1; i < len(parts); i++ { + if isDottedNumber(parts[i]) { + return strings.Join(parts[:i], "-"), parts[i] + } + } + + // No purely numeric field, e.g. a prerelease version like "1.0b1". + return splitNameVersion(fullname) +} + +// splitNameVersion splits a {name}-{version} pair at the last hyphen that +// starts a version, leaving hyphens inside the name intact. +func splitNameVersion(base string) (name, version string) { + for i := len(base) - 1; i >= 0; i-- { + if base[i] == '-' && i+1 < len(base) && isVersionStart(base[i+1]) { + return base[:i], base[i+1:] + } + } + + return "", "" +} + +// isEggPythonTag reports whether field is the py{X.Y} interpreter field that +// setuptools places directly after the version in an egg filename. +func isEggPythonTag(field string) bool { + const prefix = "py" + + return len(field) > len(prefix) && strings.HasPrefix(field, prefix) && isVersionStart(field[len(prefix)]) +} + +// isVersionField reports whether field can be a version, i.e. it is non-empty +// and starts with a digit as every PEP 440 release segment does. +func isVersionField(field string) bool { + return field != "" && isVersionStart(field[0]) +} + +// isDottedNumber reports whether s is a dotted numeric version such as "2.7". +func isDottedNumber(s string) bool { + if s == "" || !isVersionStart(s[0]) { return false } - // Python tags start with py, cp, pp, ip, jy - prefixes := []string{"py", "cp", "pp", "ip", "jy"} - for _, p := range prefixes { - if strings.HasPrefix(s, p) { - return true + + for i := range len(s) { + if !isVersionStart(s[i]) && s[i] != '.' { + return false } } - return false + + return true } func isVersionStart(c byte) bool { diff --git a/internal/handler/pypi_test.go b/internal/handler/pypi_test.go index 2b58960..5ae76ca 100644 --- a/internal/handler/pypi_test.go +++ b/internal/handler/pypi_test.go @@ -28,13 +28,58 @@ func TestPyPIParseFilename(t *testing.T) { {"aws-sdk-1.0.0.tar.gz", "aws-sdk", "1.0.0"}, {"zipp-3.17.0.zip", "zipp", "3.17.0"}, + // Additional sdist archive formats + {"lxml-4.9.3.tar.xz", "lxml", "4.9.3"}, + {"docutils-0.20.1.tgz", "docutils", "0.20.1"}, + {"psycopg2-2.9.9.tar.bz2", "psycopg2", "2.9.9"}, + // Wheel formats {"requests-2.31.0-py3-none-any.whl", "requests", "2.31.0"}, {"numpy-1.26.2-cp311-cp311-manylinux_2_17_x86_64.whl", "numpy", "1.26.2"}, {"cryptography-41.0.5-cp37-abi3-manylinux_2_28_x86_64.whl", "cryptography", "41.0.5"}, + // Wheels with a build tag must not fold the tag into the version + {"foo-1.0-1-py3-none-any.whl", "foo", "1.0"}, + {"tensorflow-2.15.0-2-cp311-cp311-manylinux_2_17_x86_64.whl", "tensorflow", "2.15.0"}, + + // PEP 658 core-metadata sidecars resolve to the distribution they describe + {"backports_asyncio_runner-1.2.0-py3-none-any.whl.metadata", "backports_asyncio_runner", "1.2.0"}, + {"requests-2.31.0-py3-none-any.whl.metadata", "requests", "2.31.0"}, + {"requests-2.31.0.tar.gz.metadata", "requests", "2.31.0"}, + + // Eggs: {name}-{version}-py{X.Y}(-{platform})?.egg. Unescaped hyphens in + // the name must not be mistaken for the field separator before the version. + {"numpy-1.8.0-py2.7-macosx-10.9-x86_64.egg", "numpy", "1.8.0"}, + {"aws-sdk-1.0.0-py3.11.egg", "aws-sdk", "1.0.0"}, + {"aws-sdk-1.0.0-py2.7-macosx-10.9-x86_64.egg", "aws-sdk", "1.0.0"}, + {"aws-sdk-1.0.0.egg", "aws-sdk", "1.0.0"}, + // A "py{N}" component inside the name is not the interpreter field, so + // the interpreter must be located from the end of the filename. + {"django-rest-py3-1.0-py3.6.egg", "django-rest-py3", "1.0"}, + + // Windows installers: {name}-{version}.{platform}(-py{X.Y})?.{exe,msi}. + // The platform is not part of the version, and may contain a hyphen. + {"foo-1.0.win32-py2.0.exe", "foo", "1.0"}, + {"pywin32-223.win32-py2.7.exe", "pywin32", "223"}, + {"numpy-1.8.0.win-amd64-py2.7.exe", "numpy", "1.8.0"}, + {"aws-sdk-1.0.0.win32-py2.7.exe", "aws-sdk", "1.0.0"}, + {"pywin32-223.win32.exe", "pywin32", "223"}, + {"cx_Oracle-5.1.2.win32-py2.7.msi", "cx_Oracle", "5.1.2"}, + {"numpy-1.8.0.win-amd64.msi", "numpy", "1.8.0"}, + // A trailing build variant belongs to neither the name nor the version. + {"cx_Oracle-5.1.2-11g.win32-py2.7.exe", "cx_Oracle", "5.1.2"}, + // A prerelease version has no purely numeric field to anchor on. + {"foo-1.0b1.win32-py2.7.exe", "foo", "1.0b1"}, + // Invalid {"invalid", "", ""}, + {"invalid.metadata", "", ""}, + {"backports.ssl_match_hostname-3.4.0.2-py2.7.whl", "", ""}, + {"invalid.exe", "", ""}, + {"foo-1.0.exe", "", ""}, + // An egg with an interpreter field but no version must not promote the + // trailing component of a hyphenated name to the version. + {"aws-sdk-py2.7.egg", "", ""}, } for _, tt := range tests { @@ -94,25 +139,24 @@ func TestPyPIRewriteJSONMetadataCooldown(t *testing.T) { } } -func TestIsPythonTag(t *testing.T) { - tests := []struct { - tag string - want bool - }{ - {"py3", true}, - {"py2", true}, - {"cp311", true}, - {"cp37", true}, - {"pp39", true}, - {"none", false}, - {"any", false}, - {"manylinux", false}, +// TestPyPIParseFilenameNoHashFallback guards the identifier used for caching: +// a filename that parses to an empty name makes handleDownload fall back to a +// "_hash_" package name, which surfaces as a bogus PURL in the package +// overview. +func TestPyPIParseFilenameNoHashFallback(t *testing.T) { + h := &PyPIHandler{proxy: &Proxy{Logger: slog.Default()}} + + filenames := []string{ + "backports_asyncio_runner-1.2.0-py3-none-any.whl", + "backports_asyncio_runner-1.2.0-py3-none-any.whl.metadata", + "backports_asyncio_runner-1.2.0.tar.gz", } - for _, tt := range tests { - got := isPythonTag(tt.tag) - if got != tt.want { - t.Errorf("isPythonTag(%q) = %v, want %v", tt.tag, got, tt.want) + for _, filename := range filenames { + name, version := h.parseFilename(filename) + if name != "backports_asyncio_runner" || version != "1.2.0" { + t.Errorf("parseFilename(%q) = (%q, %q), want (%q, %q)", + filename, name, version, "backports_asyncio_runner", "1.2.0") } } } diff --git a/internal/server/browse.go b/internal/server/browse.go index 56aa1c5..c60cba3 100644 --- a/internal/server/browse.go +++ b/internal/server/browse.go @@ -13,6 +13,7 @@ import ( "github.com/git-pkgs/archives/diff" "github.com/git-pkgs/magic" "github.com/git-pkgs/proxy/internal/database" + "github.com/git-pkgs/proxy/internal/handler" "github.com/git-pkgs/purl" "github.com/go-chi/chi/v5" ) @@ -27,6 +28,31 @@ const ( // memory exhaustion from a single request. const maxBrowseArchiveSize = 512 << 20 // 512 MB +// firstBrowsableArtifact returns the first cached artifact that can be opened as +// an archive, or nil if the version has none. +// +// A version's artifact list is not all archives: a PEP 658 core-metadata sidecar +// resolves to the same name and version as the distribution it describes, so it +// is cached under that version too. Sidecars are plain text, and because '-' +// sorts before '.' one can even precede the real distribution in the +// filename-ordered list, so selecting blindly would hand openArchive a file it +// cannot parse. +func firstBrowsableArtifact(artifacts []database.Artifact) *database.Artifact { + for i := range artifacts { + if artifacts[i].StoragePath.Valid && !isMetadataSidecar(artifacts[i].Filename) { + return &artifacts[i] + } + } + + return nil +} + +// isMetadataSidecar reports whether filename is a core-metadata sidecar rather +// than a distribution archive. +func isMetadataSidecar(filename string) bool { + return strings.HasSuffix(filename, handler.PyPIMetadataSuffix) +} + // detectSingleRootDir returns the single top-level directory name if all files // in the archive live under one common directory (e.g. GitHub zipballs use // "repo-hash/"). Returns "" if there's no single root or the archive is flat. @@ -214,14 +240,7 @@ func (s *Server) browseList(w http.ResponseWriter, r *http.Request, ecosystem, n return } - // Find the first cached artifact - var cachedArtifact *database.Artifact - for i := range artifacts { - if artifacts[i].StoragePath.Valid { - cachedArtifact = &artifacts[i] - break - } - } + cachedArtifact := firstBrowsableArtifact(artifacts) if cachedArtifact == nil { notFound(w, "artifact not cached") @@ -308,14 +327,7 @@ func (s *Server) browseFile(w http.ResponseWriter, r *http.Request, ecosystem, n return } - // Find the first cached artifact - var cachedArtifact *database.Artifact - for i := range artifacts { - if artifacts[i].StoragePath.Valid { - cachedArtifact = &artifacts[i] - break - } - } + cachedArtifact := firstBrowsableArtifact(artifacts) if cachedArtifact == nil { notFound(w, "artifact not cached") @@ -535,19 +547,8 @@ func (s *Server) compareDiff(w http.ResponseWriter, r *http.Request, ecosystem, } // Find cached artifacts - var fromArtifact, toArtifact *database.Artifact - for i := range fromArtifacts { - if fromArtifacts[i].StoragePath.Valid { - fromArtifact = &fromArtifacts[i] - break - } - } - for i := range toArtifacts { - if toArtifacts[i].StoragePath.Valid { - toArtifact = &toArtifacts[i] - break - } - } + fromArtifact := firstBrowsableArtifact(fromArtifacts) + toArtifact := firstBrowsableArtifact(toArtifacts) if fromArtifact == nil || toArtifact == nil { notFound(w, "one or both versions not cached") diff --git a/internal/server/browse_test.go b/internal/server/browse_test.go index 5240a92..6ea0f7e 100644 --- a/internal/server/browse_test.go +++ b/internal/server/browse_test.go @@ -829,3 +829,70 @@ func createTarGzArchive(t *testing.T, files map[string]string) []byte { } return buf.Bytes() } + +// TestFirstBrowsableArtifact guards artifact selection against PEP 658 +// core-metadata sidecars. A sidecar resolves to the same version as the +// distribution it describes, so it is cached under that version, but it is plain +// text and openArchive cannot parse it. +func TestFirstBrowsableArtifact(t *testing.T) { + cached := func(filename string) database.Artifact { + return database.Artifact{ + Filename: filename, + StoragePath: sql.NullString{String: "pypi/" + filename, Valid: true}, + } + } + uncached := func(filename string) database.Artifact { + return database.Artifact{Filename: filename} + } + + tests := []struct { + name string + artifacts []database.Artifact + want string + }{ + {"no artifacts", nil, ""}, + { + "sidecar only is not browsable", + []database.Artifact{cached("foo-1.0-py3-none-any.whl.metadata")}, + "", + }, + { + // '-' (0x2D) sorts before '.' (0x2E), so the sidecar precedes the + // sdist in the filename-ordered list the query returns. + "sidecar sorting ahead of the sdist is skipped", + []database.Artifact{cached("foo-1.0-py3-none-any.whl.metadata"), cached("foo-1.0.tar.gz")}, + "foo-1.0.tar.gz", + }, + { + "sidecar skipped in favour of its own wheel", + []database.Artifact{cached("foo-1.0-py3-none-any.whl.metadata"), cached("foo-1.0-py3-none-any.whl")}, + "foo-1.0-py3-none-any.whl", + }, + { + "uncached archive is still not selected", + []database.Artifact{cached("foo-1.0.tar.gz.metadata"), uncached("foo-1.0.tar.gz")}, + "", + }, + {"plain sdist", []database.Artifact{cached("foo-1.0.tar.gz")}, "foo-1.0.tar.gz"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := firstBrowsableArtifact(tt.artifacts) + + if tt.want == "" { + if got != nil { + t.Fatalf("firstBrowsableArtifact() = %q, want nil", got.Filename) + } + return + } + + if got == nil { + t.Fatalf("firstBrowsableArtifact() = nil, want %q", tt.want) + } + if got.Filename != tt.want { + t.Errorf("firstBrowsableArtifact() = %q, want %q", got.Filename, tt.want) + } + }) + } +} diff --git a/internal/server/server.go b/internal/server/server.go index 13c5997..153a94c 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -791,13 +791,9 @@ func (s *Server) showVersion(w http.ResponseWriter, r *http.Request, ecosystem, isOutdated := pkg.LatestVersion.Valid && pkg.LatestVersion.String != version - hasCached := false - for _, art := range artifacts { - if art.StoragePath.Valid { - hasCached = true - break - } - } + // A version whose only cached artifact is a metadata sidecar cannot be + // browsed, so it must not be advertised as cached. + hasCached := firstBrowsableArtifact(artifacts) != nil data := VersionShowData{ Layout: s.layoutFor(r),