From 40837188dd855d81f6ba0f5fa609876e2b5dbc25 Mon Sep 17 00:00:00 2001 From: wickedOne Date: Thu, 30 Jul 2026 17:16:38 +0200 Subject: [PATCH 1/2] resolve name and version for PEP 658 metadata sidecars --- internal/handler/pypi.go | 90 ++++++++++++++++++++--------------- internal/handler/pypi_test.go | 53 ++++++++++++++------- 2 files changed, 87 insertions(+), 56 deletions(-) diff --git a/internal/handler/pypi.go b/internal/handler/pypi.go index f5a0232..133d3cd 100644 --- a/internal/handler/pypi.go +++ b/internal/handler/pypi.go @@ -19,7 +19,11 @@ 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") + minTaggedParts = 2 // name + version + + // pypiMetadataSuffix is the PEP 658 core-metadata sidecar suffix that pip + // appends to a distribution URL when the index advertises core metadata. + pypiMetadataSuffix = ".metadata" ) // PyPIHandler handles PyPI registry protocol requests. @@ -433,37 +437,59 @@ func (h *PyPIHandler) handleDownload(w http.ResponseWriter, r *http.Request) { ServeArtifact(w, result) } +// taggedExtensions are distribution formats whose filenames append build, +// interpreter and platform tags after the version. Their name and version +// fields are escaped so neither can contain a hyphen, which makes the first two +// hyphen-separated fields authoritative. minParts is the field count a +// well-formed filename of that format has at minimum. +var taggedExtensions = []struct { + ext string + minParts int +}{ + {".whl", minWheelParts}, + {".egg", minTaggedParts}, + {".exe", minTaggedParts}, + {".msi", minTaggedParts}, +} + +// archiveExtensions are sdist formats of the form {name}-{version}{ext}. Unlike +// the tagged formats these 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"} + // 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 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) + + // Wheels are {name}-{version}(-{build})?-{python}-{abi}-{platform}.whl; + // per PEP 427 the name and version fields have any hyphen escaped to '_'. + for _, format := range taggedExtensions { + if !strings.HasSuffix(filename, format.ext) { + continue } + parts := strings.Split(strings.TrimSuffix(filename, format.ext), "-") + if len(parts) < format.minParts { + return "", "" + } + return parts[0], parts[1] } - // Try sdist formats: {name}-{version}.tar.gz, {name}-{version}.zip - for _, ext := range []string{".tar.gz", ".tar.bz2", ".zip", ".tar"} { - 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:] - } + for _, ext := range archiveExtensions { + if !strings.HasSuffix(filename, ext) { + continue + } + 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:] } } } @@ -471,20 +497,6 @@ func (h *PyPIHandler) parseFilename(filename string) (name, version string) { return "", "" } -func isPythonTag(s string) bool { - if len(s) < minPythonTagLen { - 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 - } - } - return false -} - func isVersionStart(c byte) bool { return c >= '0' && c <= '9' } diff --git a/internal/handler/pypi_test.go b/internal/handler/pypi_test.go index 2b58960..61b9a17 100644 --- a/internal/handler/pypi_test.go +++ b/internal/handler/pypi_test.go @@ -28,13 +28,33 @@ 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"}, + + // Legacy bdist formats + {"numpy-1.8.0-py2.7-macosx-10.9-x86_64.egg", "numpy", "1.8.0"}, + {"pywin32-223.win32-py2.7.exe", "pywin32", "223.win32"}, + // Invalid {"invalid", "", ""}, + {"invalid.metadata", "", ""}, + {"backports.ssl_match_hostname-3.4.0.2-py2.7.whl", "", ""}, } for _, tt := range tests { @@ -94,25 +114,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") } } } From 780e53f168a73dde33820958c5450c20db3afd2e Mon Sep 17 00:00:00 2001 From: wickedOne Date: Sat, 1 Aug 2026 15:54:24 +0200 Subject: [PATCH 2/2] fix(pypi): parse Windows installer and egg filenames separately The bdist_wininst and bdist_msi layout joins the platform to the version with a '.' rather than a '-', so treating .exe/.msi like a wheel folded the platform into the version: foo-1.0.win32-py2.0.exe resolved to version "1.0.win32". Eggs shared the problem, as setuptools' hyphen escaping is not universal: aws-sdk-1.0.0-py3.11.egg resolved to name "aws", version "sdk". Give each format its own parser. Wheels keep the PEP 427 spec-guaranteed field positions, eggs locate the version relative to the py{X.Y} interpreter field, and Windows installers strip the platform and interpreter fields before splitting name from version. A PEP 658 sidecar resolves to the same name and version as the distribution it describes, so it is cached under that version. Browse and compare took the first cached artifact without checking its extension, handing openArchive plain text: a version pip had only fetched metadata for reported hasCached and then 500'd. Add firstBrowsableArtifact, replacing five duplicated selection loops, and export PyPIMetadataSuffix so the suffix has a single definition. Co-Authored-By: Claude Opus 5 (1M context) --- internal/handler/pypi.go | 180 ++++++++++++++++++++++++++------- internal/handler/pypi_test.go | 29 +++++- internal/server/browse.go | 59 +++++------ internal/server/browse_test.go | 67 ++++++++++++ internal/server/server.go | 10 +- 5 files changed, 268 insertions(+), 77 deletions(-) diff --git a/internal/handler/pypi.go b/internal/handler/pypi.go index 133d3cd..713f6c8 100644 --- a/internal/handler/pypi.go +++ b/internal/handler/pypi.go @@ -19,11 +19,14 @@ const ( minWheelParts = 5 // name + version + python + abi + platform minSubmatchParts = 2 // full match + first capture group minPyPIPathParts = 3 // hash_prefix + hash + filename - minTaggedParts = 2 // name + version + minEggParts = 3 // name + version + python tag - // pypiMetadataSuffix is the PEP 658 core-metadata sidecar suffix that pip + // PyPIMetadataSuffix is the PEP 658 core-metadata sidecar suffix that pip // appends to a distribution URL when the index advertises core metadata. - pypiMetadataSuffix = ".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. @@ -437,66 +440,165 @@ func (h *PyPIHandler) handleDownload(w http.ResponseWriter, r *http.Request) { ServeArtifact(w, result) } -// taggedExtensions are distribution formats whose filenames append build, -// interpreter and platform tags after the version. Their name and version -// fields are escaped so neither can contain a hyphen, which makes the first two -// hyphen-separated fields authoritative. minParts is the field count a -// well-formed filename of that format has at minimum. -var taggedExtensions = []struct { - ext string - minParts int -}{ - {".whl", minWheelParts}, - {".egg", minTaggedParts}, - {".exe", minTaggedParts}, - {".msi", minTaggedParts}, -} - -// archiveExtensions are sdist formats of the form {name}-{version}{ext}. Unlike -// the tagged formats these carry no trailing tags, but legacy sdist names may -// contain hyphens. +// 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 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) { // 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) + filename = strings.TrimSuffix(filename, PyPIMetadataSuffix) - // Wheels are {name}-{version}(-{build})?-{python}-{abi}-{platform}.whl; - // per PEP 427 the name and version fields have any hyphen escaped to '_'. - for _, format := range taggedExtensions { - if !strings.HasSuffix(filename, format.ext) { - continue - } - parts := strings.Split(strings.TrimSuffix(filename, format.ext), "-") - if len(parts) < format.minParts { - return "", "" + 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)) } - return parts[0], parts[1] } for _, ext := range archiveExtensions { - if !strings.HasSuffix(filename, ext) { + if strings.HasSuffix(filename, ext) { + return splitNameVersion(strings.TrimSuffix(filename, ext)) + } + } + + return "", "" +} + +// 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 } - 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 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 + } + + for i := range len(s) { + if !isVersionStart(s[i]) && s[i] != '.' { + return false + } + } + + return true +} + func isVersionStart(c byte) bool { return c >= '0' && c <= '9' } diff --git a/internal/handler/pypi_test.go b/internal/handler/pypi_test.go index 61b9a17..5ae76ca 100644 --- a/internal/handler/pypi_test.go +++ b/internal/handler/pypi_test.go @@ -47,14 +47,39 @@ func TestPyPIParseFilename(t *testing.T) { {"requests-2.31.0-py3-none-any.whl.metadata", "requests", "2.31.0"}, {"requests-2.31.0.tar.gz.metadata", "requests", "2.31.0"}, - // Legacy bdist formats + // 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"}, - {"pywin32-223.win32-py2.7.exe", "pywin32", "223.win32"}, + {"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 { 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),