From 14f80ced3479999c86c6690e5005dfc00a398841 Mon Sep 17 00:00:00 2001 From: Philipp Garbe Date: Mon, 10 Aug 2026 10:21:49 +0200 Subject: [PATCH 01/24] fix(npm): use combined Accept header to support Artifactory upstreams (#241) When cooldown is disabled, send: Accept: application/vnd.npm.install-v1+json;q=1.0, application/json;q=0.8 This allows upstreams like JFrog Artifactory that return 406 for the abbreviated packument type to fall back to full JSON metadata, while letting the public npm registry continue to serve the smaller abbreviated format it prefers. When cooldown is enabled, keep sending only application/json because the abbreviated format omits the "time" map required for version age filtering. Fixes #228 Co-authored-by: Claude Sonnet 4.6 --- internal/handler/npm.go | 11 +++++++---- internal/handler/npm_test.go | 12 ++++++------ 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/internal/handler/npm.go b/internal/handler/npm.go index ee9b59c..78aa5e4 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -13,7 +13,7 @@ import ( const ( npmUpstream = "https://registry.npmjs.org" - npmAbbreviatedCT = "application/vnd.npm.install-v1+json" + npmAcceptDefault = "application/vnd.npm.install-v1+json;q=1.0, application/json;q=0.8" scopedParts = 2 // scope + name in scoped packages ) @@ -71,9 +71,12 @@ func (h *NPMHandler) handlePackageMetadata(w http.ResponseWriter, r *http.Reques upstreamURL := fmt.Sprintf("%s/%s", h.upstreamURL, url.PathEscape(packageName)) - // Use abbreviated metadata when cooldown is disabled — it's much smaller - // (e.g. drizzle-orm: 4MB vs 92MB) but lacks the time map needed for cooldown. - accept := npmAbbreviatedCT + // Prefer the smaller abbreviated packument format but include application/json + // as a fallback so upstreams that reject the abbreviated type (e.g. JFrog + // Artifactory, which returns 406) can still respond with full metadata. + // When cooldown is enabled we must use full metadata exclusively because the + // abbreviated format omits the "time" map required for version age filtering. + accept := npmAcceptDefault if h.proxy.Cooldown != nil && h.proxy.Cooldown.Enabled() { accept = contentTypeJSON } diff --git a/internal/handler/npm_test.go b/internal/handler/npm_test.go index e0257dd..fd6f080 100644 --- a/internal/handler/npm_test.go +++ b/internal/handler/npm_test.go @@ -396,7 +396,7 @@ func TestNPMHandlerUsesAbbreviatedMetadata(t *testing.T) { })) defer upstream.Close() - t.Run("no cooldown uses abbreviated metadata", func(t *testing.T) { + t.Run("no cooldown uses combined accept header", func(t *testing.T) { h := &NPMHandler{ proxy: testProxy(), upstreamURL: upstream.URL, @@ -407,12 +407,12 @@ func TestNPMHandlerUsesAbbreviatedMetadata(t *testing.T) { w := httptest.NewRecorder() h.handlePackageMetadata(w, req) - if gotAccept != npmAbbreviatedCT { - t.Errorf("Accept = %q, want abbreviated metadata header", gotAccept) + if gotAccept != npmAcceptDefault { + t.Errorf("Accept = %q, want %q", gotAccept, npmAcceptDefault) } }) - t.Run("cooldown enabled uses full metadata", func(t *testing.T) { + t.Run("cooldown enabled uses full metadata only", func(t *testing.T) { proxy := testProxy() proxy.Cooldown = &cooldown.Config{Default: "3d"} @@ -426,8 +426,8 @@ func TestNPMHandlerUsesAbbreviatedMetadata(t *testing.T) { w := httptest.NewRecorder() h.handlePackageMetadata(w, req) - if gotAccept == npmAbbreviatedCT { - t.Error("cooldown enabled should use full metadata, not abbreviated") + if gotAccept != contentTypeJSON { + t.Errorf("Accept = %q, want %q (cooldown requires full metadata)", gotAccept, contentTypeJSON) } }) } From 4fa903e01e26aecdfc0001a965c0a4598b015afd Mon Sep 17 00:00:00 2001 From: oscar-broman Date: Mon, 10 Aug 2026 12:27:03 +0400 Subject: [PATCH 02/24] Enforce cooldown on artifact downloads (#240) Cooldown filtering only ran when rewriting metadata, so a version could be missing from the npm packument and the PyPI simple index while its tarball stayed reachable. Lockfiles record artifact URLs verbatim, so npm ci and pinned pip requirements reach handleDownload without ever requesting metadata. The shared artifact path has no publish time to check against, since updateCacheDB upserts versions without PublishedAt and the column is only set by enrichment. Each handler now resolves the publish time from metadata it already fetches and returns 404 while a version is inside the window. Versions with no usable publish time are still served, as they are when filtering metadata. --- internal/handler/npm.go | 51 +++++++++++++++++++++++ internal/handler/npm_test.go | 78 +++++++++++++++++++++++++++++++++++ internal/handler/pypi.go | 22 ++++++++++ internal/handler/pypi_test.go | 56 +++++++++++++++++++++++++ 4 files changed, 207 insertions(+) diff --git a/internal/handler/npm.go b/internal/handler/npm.go index 78aa5e4..b7d96a3 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -268,6 +268,13 @@ func (h *NPMHandler) handleDownload(w http.ResponseWriter, r *http.Request) { h.proxy.Logger.Info("npm download request", "package", packageName, "version", version, "filename", filename) + if h.versionInCooldown(r, packageName, version) { + h.proxy.Logger.Info("cooldown: withholding npm tarball", + "package", packageName, "version", version) + JSONError(w, http.StatusNotFound, "version not found") + return + } + downloadURL := fmt.Sprintf( "%s/%s/-/%s", h.upstreamURL, @@ -290,6 +297,50 @@ func (h *NPMHandler) handleDownload(w http.ResponseWriter, r *http.Request) { ServeArtifact(w, result) } +// versionInCooldown reports whether a version is still inside the cooldown +// window. Filtering the packument is not enough on its own: tarball URLs are +// predictable and lockfiles record them directly, so `npm ci` reaches the +// download path without ever requesting metadata. +// +// The packument is served from the metadata cache, so this normally costs no +// extra upstream request. A version with no usable publish time is allowed +// through, matching how applyCooldownFiltering treats it. +func (h *NPMHandler) versionInCooldown(r *http.Request, packageName, version string) bool { + if h.proxy.Cooldown == nil || !h.proxy.Cooldown.Enabled() { + return false + } + + upstreamURL := fmt.Sprintf("%s/%s", h.upstreamURL, url.PathEscape(packageName)) + + body, _, err := h.proxy.FetchOrCacheMetadata(r.Context(), "npm", packageName, upstreamURL, contentTypeJSON) + if err != nil { + h.proxy.Logger.Warn("cooldown: could not fetch npm metadata for download check", + "package", packageName, "version", version, "error", err) + return false + } + + var metadata struct { + Time map[string]string `json:"time"` + } + if err := json.Unmarshal(body, &metadata); err != nil { + h.proxy.Logger.Warn("cooldown: could not parse npm metadata for download check", + "package", packageName, "version", version, "error", err) + return false + } + + published, ok := metadata.Time[version] + if !ok { + return false + } + + publishedAt, err := time.Parse(time.RFC3339, published) + if err != nil { + return false + } + + return !h.proxy.Cooldown.IsAllowed("npm", canonicalPackagePURL("npm", packageName), publishedAt) +} + func escapeNPMDownloadPackage(packageName string) string { scope, name, scoped := strings.Cut(packageName, "/") if scoped && strings.HasPrefix(scope, "@") && len(scope) > 1 && name != "" && !strings.Contains(name, "/") { diff --git a/internal/handler/npm_test.go b/internal/handler/npm_test.go index fd6f080..07da9c3 100644 --- a/internal/handler/npm_test.go +++ b/internal/handler/npm_test.go @@ -454,3 +454,81 @@ func TestNPMHandlerMetadataNotFound(t *testing.T) { t.Errorf("status = %d, want %d", w.Code, http.StatusNotFound) } } + +func TestNPMDownloadCooldown(t *testing.T) { + now := time.Now() + packument := `{ + "name": "leftpad", + "dist-tags": {"latest": "2.0.0"}, + "time": { + "1.0.0": "` + now.Add(-30*24*time.Hour).Format(time.RFC3339) + `", + "2.0.0": "` + now.Add(-1*time.Hour).Format(time.RFC3339) + `" + }, + "versions": {"1.0.0": {}, "2.0.0": {}} + }` + + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", contentTypeJSON) + _, _ = io.WriteString(w, packument) + })) + defer upstream.Close() + + tests := []struct { + name string + version string + wantStatus int + }{ + {"published before the window serves the tarball", testVersion100, http.StatusOK}, + {"published inside the window is withheld", "2.0.0", http.StatusNotFound}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + proxy, _, _, fetcher := setupTestProxy(t) + proxy.HTTPClient = upstream.Client() + proxy.Cooldown = &cooldown.Config{Default: "7d"} + fetcher.artifact = &fetch.Artifact{ + Body: io.NopCloser(strings.NewReader("tarball data")), + ContentType: "application/octet-stream", + } + + h := NewNPMHandler(proxy, "http://proxy.test", upstream.URL) + srv := httptest.NewServer(h.Routes()) + defer srv.Close() + + resp, err := http.Get(srv.URL + "/leftpad/-/leftpad-" + tt.version + ".tgz") + if err != nil { + t.Fatalf("request failed: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != tt.wantStatus { + t.Errorf("status = %d, want %d", resp.StatusCode, tt.wantStatus) + } + if tt.wantStatus == http.StatusNotFound && fetcher.fetchCalled { + t.Error("fetched a version that is still inside the cooldown window") + } + }) + } +} + +func TestNPMDownloadCooldownDisabled(t *testing.T) { + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + t.Error("metadata must not be fetched when cooldown is disabled") + w.WriteHeader(http.StatusInternalServerError) + })) + defer upstream.Close() + + proxy, _, _, fetcher := setupTestProxy(t) + proxy.HTTPClient = upstream.Client() + fetcher.artifact = &fetch.Artifact{ + Body: io.NopCloser(strings.NewReader("tarball data")), + ContentType: "application/octet-stream", + } + + h := NewNPMHandler(proxy, "http://proxy.test", upstream.URL) + + if h.versionInCooldown(httptest.NewRequest(http.MethodGet, "/", nil), "leftpad", testVersion100) { + t.Error("versionInCooldown = true, want false when cooldown is not configured") + } +} diff --git a/internal/handler/pypi.go b/internal/handler/pypi.go index 713f6c8..7875cdd 100644 --- a/internal/handler/pypi.go +++ b/internal/handler/pypi.go @@ -311,6 +311,21 @@ func (h *PyPIHandler) shouldFilterRelease(packagePURL string, files any) bool { return !publishedAt.IsZero() && !h.proxy.Cooldown.IsAllowed("pypi", packagePURL, publishedAt) } +// versionInCooldown reports whether a version is still inside the cooldown +// window. Filtering the simple index is not enough on its own: file URLs are +// recorded in lockfiles and requirements pins, so pip can reach the download +// path without ever reading the index. +// +// A release whose upload time cannot be determined is allowed through, matching +// how fetchFilteredVersions treats it. +func (h *PyPIHandler) versionInCooldown(r *http.Request, name, version string) bool { + if h.proxy.Cooldown == nil || !h.proxy.Cooldown.Enabled() { + return false + } + + return h.fetchFilteredVersions(r, name)[version] +} + // rewriteFileEntries rewrites URLs in a list of file entries. func (h *PyPIHandler) rewriteFileEntries(files any) { filesArr, ok := files.([]any) @@ -417,6 +432,13 @@ func (h *PyPIHandler) handleDownload(w http.ResponseWriter, r *http.Request) { filename := parts[len(parts)-1] name, version := h.parseFilename(filename) + if name != "" && h.versionInCooldown(r, name, version) { + h.proxy.Logger.Info("cooldown: withholding pypi file", + "name", name, "version", version, "filename", filename) + http.Error(w, "not found", http.StatusNotFound) + return + } + if name == "" { // Can't determine name/version, use hash as identifier name = fmt.Sprintf("_hash_%s", hashPath(path)) diff --git a/internal/handler/pypi_test.go b/internal/handler/pypi_test.go index 5ae76ca..6bbcf3e 100644 --- a/internal/handler/pypi_test.go +++ b/internal/handler/pypi_test.go @@ -236,3 +236,59 @@ func TestPyPIHandler_DownloadCacheMiss(t *testing.T) { t.Error("expected fetcher to be called on cache miss") } } + +func TestPyPIDownloadCooldown(t *testing.T) { + now := time.Now() + releases := `{"releases": { + "1.0.0": [{"upload_time_iso_8601": "` + now.Add(-30*24*time.Hour).Format(time.RFC3339) + `"}], + "2.0.0": [{"upload_time_iso_8601": "` + now.Add(-1*time.Hour).Format(time.RFC3339) + `"}] + }}` + + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", contentTypeJSON) + _, _ = io.WriteString(w, releases) + })) + defer upstream.Close() + + tests := []struct { + name string + filename string + wantStatus int + }{ + {"published before the window serves the file", "newpkg-1.0.0.tar.gz", http.StatusOK}, + {"published inside the window is withheld", "newpkg-2.0.0.tar.gz", http.StatusNotFound}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + proxy, _, _, fetcher := setupTestProxy(t) + proxy.HTTPClient = upstream.Client() + proxy.Cooldown = &cooldown.Config{Default: "7d"} + fetcher.artifact = &fetch.Artifact{ + Body: io.NopCloser(strings.NewReader("sdist data")), + ContentType: "application/octet-stream", + } + + h := &PyPIHandler{ + proxy: proxy, + upstreamURL: upstream.URL, + proxyURL: "http://localhost", + } + srv := httptest.NewServer(h.Routes()) + defer srv.Close() + + resp, err := http.Get(srv.URL + "/packages/packages/ab/cd/ef0123456789/" + tt.filename) + if err != nil { + t.Fatalf("request failed: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != tt.wantStatus { + t.Errorf("status = %d, want %d", resp.StatusCode, tt.wantStatus) + } + if tt.wantStatus == http.StatusNotFound && fetcher.fetchCalled { + t.Error("fetched a version that is still inside the cooldown window") + } + }) + } +} From 30e40526154eb8fb23f8d3b4e810967b5155c89d Mon Sep 17 00:00:00 2001 From: Andrew Nesbitt Date: Mon, 10 Aug 2026 09:34:56 +0100 Subject: [PATCH 03/24] Document upstream.debian in README and config.example.yaml (#237) Follow-up to #229 which added the config key and env var but didn't touch docs. --- README.md | 7 +++++++ config.example.yaml | 3 +++ 2 files changed, 10 insertions(+) diff --git a/README.md b/README.md index 4609b66..01012e2 100644 --- a/README.md +++ b/README.md @@ -376,6 +376,13 @@ Replace your existing sources.list entries, then: sudo apt update ``` +The upstream defaults to `http://deb.debian.org/debian`. To proxy a different APT repository (e.g. Ubuntu), set `upstream.debian` in the config file or `PROXY_UPSTREAM_DEBIAN` in the environment: + +```yaml +upstream: + debian: "http://archive.ubuntu.com/ubuntu" +``` + ### RPM / Yum / DNF Configure yum/dnf to use the proxy in `/etc/yum.repos.d/proxy.repo`: diff --git a/config.example.yaml b/config.example.yaml index 1176f5b..7cf7bb2 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -95,6 +95,9 @@ upstream: # Cargo crate download URL cargo_download: "https://static.crates.io/crates" + # Debian/APT repository URL (used by /debian endpoint) + debian: "http://deb.debian.org/debian" + # Authentication for upstream registries # Keys are URL prefixes matched against request URLs. # Values can reference environment variables using ${VAR_NAME} syntax. From a17bdc7c898c85c2f361b3b9aee15c705a30ef2b Mon Sep 17 00:00:00 2001 From: Andrew Nesbitt Date: Mon, 10 Aug 2026 16:42:52 +0100 Subject: [PATCH 04/24] Sign published container images (#227) * Sign published container images * Attest per-platform SPDX SBOMs with cosign Extract each platform's SPDX document from the BuildKit SBOM attestation and sign it as a cosign spdxjson attestation against the manifest-list digest, so downstream consumers (e.g. Kyverno image-verification policies) can verify the predicate signature rather than relying on the unsigned BuildKit attachment. --- .github/workflows/publish.yml | 43 +++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index e2b1084..1c4eb1e 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -18,6 +18,7 @@ jobs: permissions: packages: write contents: read + id-token: write steps: - name: Check out the repo uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 @@ -45,7 +46,10 @@ jobs: with: images: ghcr.io/${{ github.repository }} + - uses: sigstore/cosign-installer@6f9f17788090df1f26f669e9d70d6ae9567deba6 # v4.1.2 + - name: Build and push Docker image + id: build uses: docker/build-push-action@53b7df96c91f9c12dcc8a07bcb9ccacbed38856a with: context: . @@ -53,3 +57,42 @@ jobs: push: true tags: ${{ steps.meta.outputs.tags }} labels: ${{ steps.meta.outputs.labels }} + provenance: mode=max + sbom: true + + - name: Sign image by digest + env: + DIGEST: ${{ steps.build.outputs.digest }} + IMAGE: ghcr.io/${{ github.repository }} + run: | + set -euo pipefail + [[ "$DIGEST" =~ ^sha256:[0-9a-f]{64}$ ]] + cosign sign --yes "${IMAGE}@${DIGEST}" + + - name: Verify BuildKit attestations and extract SPDX predicates + env: + DIGEST: ${{ steps.build.outputs.digest }} + IMAGE: ghcr.io/${{ github.repository }} + run: | + set -euo pipefail + reference="${IMAGE}@${DIGEST}" + docker buildx imagetools inspect "$reference" --format '{{ json .Provenance }}' > provenance.json + docker buildx imagetools inspect "$reference" --format '{{ json .SBOM }}' > sbom.json + + for platform in linux/amd64 linux/arm64; do + jq -e --arg p "$platform" '.[$p].SLSA | type == "object" and length > 0' \ + provenance.json >/dev/null + jq -e --arg p "$platform" '.[$p].SPDX' sbom.json > "sbom-${platform//\//-}.spdx.json" + done + + - name: Attest platform SBOMs by digest + env: + DIGEST: ${{ steps.build.outputs.digest }} + IMAGE: ghcr.io/${{ github.repository }} + run: | + set -euo pipefail + [[ "$DIGEST" =~ ^sha256:[0-9a-f]{64}$ ]] + reference="${IMAGE}@${DIGEST}" + for predicate in sbom-linux-amd64.spdx.json sbom-linux-arm64.spdx.json; do + cosign attest --yes --type spdxjson --predicate "$predicate" "$reference" + done From bbea63f04640572a68bd531b932c367dfb3b89d1 Mon Sep 17 00:00:00 2001 From: Andrew Nesbitt Date: Thu, 13 Aug 2026 07:08:23 +0100 Subject: [PATCH 05/24] fix(upstream): apply authentication through shared transport (#198) * upstream: apply authentication through shared transport * Address upstream authentication review findings --- config.example.yaml | 3 +- docs/architecture.md | 2 + docs/configuration.md | 6 +- internal/config/config.go | 75 ++++- internal/config/config_test.go | 71 +++++ internal/httpclient/transport.go | 433 ++++++++++++++++++++++++++ internal/httpclient/transport_test.go | 251 +++++++++++++++ internal/server/server.go | 32 +- 8 files changed, 852 insertions(+), 21 deletions(-) create mode 100644 internal/httpclient/transport.go create mode 100644 internal/httpclient/transport_test.go diff --git a/config.example.yaml b/config.example.yaml index 7cf7bb2..7ada017 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -99,7 +99,8 @@ upstream: debian: "http://deb.debian.org/debian" # Authentication for upstream registries - # Keys are URL prefixes matched against request URLs. + # Keys are absolute URL scopes. Scheme, host, effective port, and path + # segment boundaries must match; the longest matching scope wins. # Values can reference environment variables using ${VAR_NAME} syntax. # # Supported auth types: diff --git a/docs/architecture.md b/docs/architecture.md index f04d548..cf8b0e2 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -240,6 +240,8 @@ Fetches artifacts from upstream registries. - Exponential backoff retry on 429 (rate limit) and 5xx errors - Returns streaming reader (doesn't load into memory) - Configurable user-agent +- Shares an authentication-aware transport with metadata requests so URL-scoped credentials apply consistently +- Discovers and caches scoped OCI Bearer tokens from registry challenges **Resolver:** - Determines download URL for a package/version diff --git a/docs/configuration.md b/docs/configuration.md index 8998ac2..fa0f576 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -123,7 +123,9 @@ upstream: ## Authentication -Configure authentication for private upstream registries. Auth is matched by URL prefix, and credentials can reference environment variables using `${VAR_NAME}` syntax. +Configure authentication for private upstream registries. The same authentication-aware client is used for metadata and artifact downloads, and credentials can reference environment variables using `${VAR_NAME}` syntax. + +OCI registries that return a Bearer challenge from a `/v2/{repository}/…` endpoint are handled automatically. The proxy discovers the token realm from `WWW-Authenticate`, applies any configured credentials for the token URL, and reuses the scoped token until shortly before it expires. ### Bearer Token @@ -172,7 +174,7 @@ upstream: ### URL Matching -Auth configs are matched by URL prefix. The longest matching prefix wins, so you can configure different credentials for different paths: +Auth keys must be absolute URLs. Matching compares the scheme, host, effective port, and path-segment prefix, preventing credentials for `registry.example.com` from being sent to a lookalike host such as `registry.example.com.evil.test`. The longest matching scope wins, so you can configure different credentials for different paths: ```yaml upstream: diff --git a/internal/config/config.go b/internal/config/config.go index 3dabc62..31f8c26 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -310,23 +310,29 @@ type UpstreamConfig struct { Debian string `json:"debian" yaml:"debian"` // Auth configures authentication for upstream registries. - // Keys are URL prefixes that are matched against request URLs. + // Keys are absolute URL scopes matched by scheme, host, effective port, + // and path-segment prefix. // Example: "https://npm.pkg.github.com" matches all requests to that host. Auth map[string]AuthConfig `json:"auth" yaml:"auth"` } // AuthForURL returns the auth config that matches the given URL. -// Matches are based on URL prefix - the longest matching prefix wins. +// The longest matching URL scope wins. func (u *UpstreamConfig) AuthForURL(url string) *AuthConfig { if u.Auth == nil { return nil } + target, err := parseAuthURL(url) + if err != nil { + return nil + } var bestMatch *AuthConfig var bestLen int for pattern, auth := range u.Auth { - if strings.HasPrefix(url, pattern) && len(pattern) > bestLen { + configured, err := parseAuthURL(pattern) + if err == nil && authURLMatches(configured, target) && len(pattern) > bestLen { a := auth // copy to avoid loop variable capture bestMatch = &a bestLen = len(pattern) @@ -336,6 +342,55 @@ func (u *UpstreamConfig) AuthForURL(url string) *AuthConfig { return bestMatch } +// Validate checks upstream authentication URL scopes. +func (u *UpstreamConfig) Validate() error { + for pattern := range u.Auth { + if _, err := parseAuthURL(pattern); err != nil { + return fmt.Errorf("invalid upstream.auth URL %q: %w", pattern, err) + } + } + return nil +} + +func parseAuthURL(value string) (*url.URL, error) { + parsed, err := url.Parse(value) + if err != nil || !parsed.IsAbs() || parsed.Hostname() == "" || parsed.Opaque != "" { + return nil, fmt.Errorf("invalid authentication URL") + } + return parsed, nil +} + +func authURLMatches(configured, target *url.URL) bool { + if !strings.EqualFold(configured.Scheme, target.Scheme) || + !strings.EqualFold(configured.Hostname(), target.Hostname()) || + authURLPort(configured) != authURLPort(target) { + return false + } + if configured.RawQuery != "" && configured.RawQuery != target.RawQuery { + return false + } + + configuredPath := strings.TrimSuffix(configured.EscapedPath(), "/") + if configuredPath == "" { + return true + } + targetPath := strings.TrimSuffix(target.EscapedPath(), "/") + return targetPath == configuredPath || strings.HasPrefix(targetPath, configuredPath+"/") +} + +func authURLPort(value *url.URL) string { + if port := value.Port(); port != "" { + return port + } + if strings.EqualFold(value.Scheme, "https") { + return "443" + } + if strings.EqualFold(value.Scheme, "http") { + return "80" + } + return "" +} + // AuthConfig configures authentication for an upstream registry. type AuthConfig struct { // Type is the authentication type: "bearer", "basic", or "header". @@ -577,15 +632,19 @@ func (c *Config) Validate() error { return err } + return c.validateComponents() +} + +func (c *Config) validateComponents() error { + if err := c.Upstream.Validate(); err != nil { + return err + } + if err := c.Health.Validate(); err != nil { return err } - if err := c.Gradle.BuildCache.Validate(); err != nil { - return err - } - - return nil + return c.Gradle.BuildCache.Validate() } // Validate checks the /health configuration. An unset interval is allowed diff --git a/internal/config/config_test.go b/internal/config/config_test.go index ef6ac90..e4677c3 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -3,6 +3,7 @@ package config import ( "os" "path/filepath" + "strings" "testing" "time" ) @@ -795,3 +796,73 @@ func TestDatabaseConfigString(t *testing.T) { } } } + +func TestUpstreamAuthForURLMatchesURLComponents(t *testing.T) { + registryAuth := AuthConfig{Type: "bearer", Token: "registry-token"} + privateAuth := AuthConfig{Type: "bearer", Token: "private-token"} + config := UpstreamConfig{Auth: map[string]AuthConfig{ + "https://registry.example.com": registryAuth, + "https://registry.example.com/private": privateAuth, + }} + + tests := []struct { + name string + url string + wantToken string + }{ + {name: "registry root", url: "https://registry.example.com/package", wantToken: "registry-token"}, + {name: "host is case insensitive", url: "https://REGISTRY.EXAMPLE.COM/package", wantToken: "registry-token"}, + {name: "longest path match", url: "https://registry.example.com/private/package", wantToken: "private-token"}, + {name: "exact path match", url: "https://registry.example.com/private", wantToken: "private-token"}, + {name: "path segment boundary", url: "https://registry.example.com/private-other/package", wantToken: "registry-token"}, + {name: "lookalike host rejected", url: "https://registry.example.com.evil.test/package"}, + {name: "different scheme rejected", url: "http://registry.example.com/package"}, + {name: "different port rejected", url: "https://registry.example.com:8443/package"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + auth := config.AuthForURL(tt.url) + if tt.wantToken == "" { + if auth != nil { + t.Fatalf("AuthForURL() = %+v, want nil", auth) + } + return + } + if auth == nil { + t.Fatal("AuthForURL() = nil, want authentication") + } + if auth.Token != tt.wantToken { + t.Errorf("token = %q, want %q", auth.Token, tt.wantToken) + } + }) + } +} + +func TestValidateUpstreamAuthURLs(t *testing.T) { + t.Run("valid absolute URL", func(t *testing.T) { + cfg := Default() + cfg.Upstream.Auth = map[string]AuthConfig{ + "https://registry.example.com/private": {Type: "bearer", Token: "token"}, + } + + if err := cfg.Validate(); err != nil { + t.Fatalf("Validate() error = %v", err) + } + }) + + t.Run("invalid URL", func(t *testing.T) { + cfg := Default() + cfg.Upstream.Auth = map[string]AuthConfig{ + "registry.example.com": {Type: "bearer", Token: "token"}, + } + + err := cfg.Validate() + if err == nil { + t.Fatal("Validate() error = nil, want invalid upstream.auth URL error") + } + if !strings.Contains(err.Error(), "upstream.auth") || !strings.Contains(err.Error(), "registry.example.com") { + t.Errorf("Validate() error = %q, want field and URL", err) + } + }) +} diff --git a/internal/httpclient/transport.go b/internal/httpclient/transport.go new file mode 100644 index 0000000..d96c827 --- /dev/null +++ b/internal/httpclient/transport.go @@ -0,0 +1,433 @@ +// Package httpclient provides authentication-aware HTTP transports for upstream requests. +package httpclient + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "strings" + "sync" + "time" +) + +const ( + defaultTokenLifetime = 60 * time.Second + tokenExpirySkew = 5 * time.Second + maxTokenResponseSize = 1 << 20 + shortTokenSkewDivisor = 10 +) + +// AuthFunc returns a configured authentication header for a URL. +type AuthFunc func(url string) (headerName, headerValue string) + +// Transport adds configured authentication and follows OCI Bearer challenges. +type Transport struct { + base http.RoundTripper + authForURL AuthFunc + + mu sync.Mutex + tokens map[string]cachedToken + challenges map[string]bearerChallenge +} + +type cachedToken struct { + value string + expiresAt time.Time +} + +type bearerChallenge struct { + realm string + service string + scopes []string +} + +type tokenResponse struct { + Token string `json:"token"` + AccessToken string `json:"access_token"` + ExpiresIn int64 `json:"expires_in"` + IssuedAt string `json:"issued_at"` +} + +// NewTransport creates an authentication-aware transport around base. +func NewTransport(base http.RoundTripper, authForURL AuthFunc) *Transport { + if base == nil { + base = http.DefaultTransport + } + return &Transport{ + base: base, + authForURL: authForURL, + tokens: make(map[string]cachedToken), + challenges: make(map[string]bearerChallenge), + } +} + +// RoundTrip implements http.RoundTripper. +func (t *Transport) RoundTrip(req *http.Request) (*http.Response, error) { + hasExplicitAuthorization := req.Header.Get("Authorization") != "" + outbound := cloneRequest(req) + t.applyAuthentication(outbound, hasExplicitAuthorization) + + resp, err := t.base.RoundTrip(outbound) + if err != nil || resp.StatusCode != http.StatusUnauthorized { + return resp, err + } + if hasExplicitAuthorization { + return resp, nil + } + if registryProtectionSpace(req.URL) == "" { + return resp, nil + } + + challenge, ok := parseBearerChallenge(resp.Header.Values("WWW-Authenticate")) + if !ok || !canReplay(req) { + return resp, nil + } + + drainAndClose(resp.Body) + token, err := t.token(req.Context(), challenge) + if err != nil { + return nil, fmt.Errorf("registry authentication: %w", err) + } + t.rememberChallenge(req.URL, challenge) + + retry, err := cloneRequestForRetry(req) + if err != nil { + return nil, err + } + t.applyConfiguredAuthentication(retry) + retry.Header.Set("Authorization", "Bearer "+token) + return t.base.RoundTrip(retry) +} + +func (t *Transport) applyAuthentication(req *http.Request, hasExplicitAuthorization bool) { + t.applyConfiguredAuthentication(req) + if hasExplicitAuthorization { + return + } + if token := t.cachedTokenForRequest(req.URL); token != "" { + req.Header.Set("Authorization", "Bearer "+token) + } +} + +func (t *Transport) applyConfiguredAuthentication(req *http.Request) { + if t.authForURL == nil { + return + } + name, value := t.authForURL(req.URL.String()) + if name != "" && value != "" && req.Header.Get(name) == "" { + req.Header.Set(name, value) + } +} + +func (t *Transport) token(ctx context.Context, challenge bearerChallenge) (string, error) { + key := challenge.key() + if token := t.cachedToken(key); token != "" { + return token, nil + } + + token, expiresAt, err := t.fetchToken(ctx, challenge) + if err != nil { + return "", err + } + + t.cacheToken(key, cachedToken{value: token, expiresAt: expiresAt}) + return token, nil +} + +func (t *Transport) cacheToken(key string, token cachedToken) { + now := time.Now() + t.mu.Lock() + defer t.mu.Unlock() + + for cachedKey, cached := range t.tokens { + if !now.Before(cached.expiresAt) { + delete(t.tokens, cachedKey) + } + } + t.tokens[key] = token +} + +func (t *Transport) fetchToken(ctx context.Context, challenge bearerChallenge) (string, time.Time, error) { + tokenURL, err := url.Parse(challenge.realm) + if err != nil || !tokenURL.IsAbs() || (tokenURL.Scheme != "https" && tokenURL.Scheme != "http") { + return "", time.Time{}, fmt.Errorf("invalid token realm %q", challenge.realm) + } + + query := tokenURL.Query() + if challenge.service != "" { + query.Set("service", challenge.service) + } + for _, scope := range challenge.scopes { + query.Add("scope", scope) + } + query.Set("client_id", "git-pkgs-proxy") + tokenURL.RawQuery = query.Encode() + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, tokenURL.String(), nil) + if err != nil { + return "", time.Time{}, err + } + + client := &http.Client{Transport: configuredTransport{parent: t}} + resp, err := client.Do(req) + if err != nil { + return "", time.Time{}, fmt.Errorf("requesting token: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { + body, _ := io.ReadAll(io.LimitReader(resp.Body, maxTokenResponseSize)) + return "", time.Time{}, fmt.Errorf("token service returned %d: %s", resp.StatusCode, strings.TrimSpace(string(body))) + } + + var payload tokenResponse + if err := json.NewDecoder(io.LimitReader(resp.Body, maxTokenResponseSize)).Decode(&payload); err != nil { + return "", time.Time{}, fmt.Errorf("decoding token response: %w", err) + } + token := payload.Token + if token == "" { + token = payload.AccessToken + } + if token == "" { + return "", time.Time{}, fmt.Errorf("token response did not contain a token") + } + + issuedAt := time.Now() + if payload.IssuedAt != "" { + if parsed, parseErr := time.Parse(time.RFC3339, payload.IssuedAt); parseErr == nil { + issuedAt = parsed + } + } + lifetime := time.Duration(payload.ExpiresIn) * time.Second + if lifetime <= 0 { + lifetime = defaultTokenLifetime + } + expiresAt := issuedAt.Add(lifetime).Add(-expirySkew(lifetime)) + return token, expiresAt, nil +} + +type configuredTransport struct { + parent *Transport +} + +func (t configuredTransport) RoundTrip(req *http.Request) (*http.Response, error) { + outbound := cloneRequest(req) + t.parent.applyConfiguredAuthentication(outbound) + return t.parent.base.RoundTrip(outbound) +} + +func (t *Transport) cachedTokenForRequest(requestURL *url.URL) string { + space := registryProtectionSpace(requestURL) + if space == "" { + return "" + } + + t.mu.Lock() + challenge, ok := t.challenges[space] + t.mu.Unlock() + if !ok { + return "" + } + return t.cachedToken(challenge.key()) +} + +func (t *Transport) cachedToken(key string) string { + now := time.Now() + t.mu.Lock() + defer t.mu.Unlock() + + token, ok := t.tokens[key] + if !ok { + return "" + } + if !now.Before(token.expiresAt) { + delete(t.tokens, key) + return "" + } + return token.value +} + +func (t *Transport) rememberChallenge(requestURL *url.URL, challenge bearerChallenge) { + space := registryProtectionSpace(requestURL) + if space == "" { + return + } + t.mu.Lock() + t.challenges[space] = challenge + t.mu.Unlock() +} + +func (c bearerChallenge) key() string { + return c.realm + "\x00" + c.service + "\x00" + strings.Join(c.scopes, "\x00") +} + +func registryProtectionSpace(u *url.URL) string { + const registryPrefix = "/v2/" + if u == nil || !strings.HasPrefix(u.Path, registryPrefix) { + return "" + } + rest := strings.TrimPrefix(u.Path, registryPrefix) + end := len(rest) + for _, marker := range []string{"/blobs/", "/manifests/", "/tags/", "/referrers/"} { + if index := strings.Index(rest, marker); index >= 0 && index < end { + end = index + } + } + if end == len(rest) || end == 0 { + return "" + } + return u.Scheme + "://" + u.Host + registryPrefix + rest[:end] +} + +func parseBearerChallenge(values []string) (bearerChallenge, bool) { + for _, value := range values { + params, ok := bearerParameters(value) + if !ok || params["realm"] == "" { + continue + } + challenge := bearerChallenge{ + realm: params["realm"], + service: params["service"], + } + if scope := params["scope"]; scope != "" { + challenge.scopes = append(challenge.scopes, scope) + } + return challenge, true + } + return bearerChallenge{}, false +} + +func bearerParameters(value string) (map[string]string, bool) { + start := findAuthScheme(value, "Bearer") + if start < 0 { + return nil, false + } + rest := value[start+len("Bearer"):] + params := make(map[string]string) + for { + rest = strings.TrimLeft(rest, " \t,") + if rest == "" { + break + } + + keyEnd := strings.IndexAny(rest, "= \t,") + if keyEnd <= 0 { + break + } + key := strings.ToLower(rest[:keyEnd]) + rest = strings.TrimLeft(rest[keyEnd:], " \t") + if rest == "" || rest[0] != '=' { + break + } + rest = strings.TrimLeft(rest[1:], " \t") + + parsed, remaining, ok := parseAuthValue(rest) + if !ok { + return nil, false + } + params[key] = parsed + rest = remaining + } + return params, true +} + +func findAuthScheme(value, scheme string) int { + inQuote := false + escaped := false + for index := 0; index+len(scheme) <= len(value); index++ { + char := value[index] + if escaped { + escaped = false + continue + } + if char == '\\' && inQuote { + escaped = true + continue + } + if char == '"' { + inQuote = !inQuote + continue + } + if inQuote || !strings.EqualFold(value[index:index+len(scheme)], scheme) { + continue + } + beforeOK := index == 0 || value[index-1] == ',' || value[index-1] == ' ' || value[index-1] == '\t' + after := index + len(scheme) + afterOK := after < len(value) && (value[after] == ' ' || value[after] == '\t') + if beforeOK && afterOK { + return index + } + } + return -1 +} + +func parseAuthValue(value string) (parsed, remaining string, ok bool) { + if value == "" { + return "", "", false + } + if value[0] != '"' { + end := strings.IndexAny(value, " \t,") + if end < 0 { + return value, "", true + } + return value[:end], value[end:], end > 0 + } + + var builder strings.Builder + escaped := false + for index := 1; index < len(value); index++ { + char := value[index] + if escaped { + builder.WriteByte(char) + escaped = false + continue + } + if char == '\\' { + escaped = true + continue + } + if char == '"' { + return builder.String(), value[index+1:], true + } + builder.WriteByte(char) + } + return "", "", false +} + +func cloneRequest(req *http.Request) *http.Request { + clone := req.Clone(req.Context()) + clone.Header = req.Header.Clone() + return clone +} + +func canReplay(req *http.Request) bool { + return req.Body == nil || req.GetBody != nil +} + +func cloneRequestForRetry(req *http.Request) (*http.Request, error) { + clone := cloneRequest(req) + if req.Body == nil { + return clone, nil + } + body, err := req.GetBody() + if err != nil { + return nil, fmt.Errorf("replaying authenticated request: %w", err) + } + clone.Body = body + return clone, nil +} + +func expirySkew(lifetime time.Duration) time.Duration { + if lifetime < tokenExpirySkew*2 { + return lifetime / shortTokenSkewDivisor + } + return tokenExpirySkew +} + +func drainAndClose(body io.ReadCloser) { + _, _ = io.Copy(io.Discard, io.LimitReader(body, maxTokenResponseSize)) + _ = body.Close() +} diff --git a/internal/httpclient/transport_test.go b/internal/httpclient/transport_test.go new file mode 100644 index 0000000..bd1f266 --- /dev/null +++ b/internal/httpclient/transport_test.go @@ -0,0 +1,251 @@ +package httpclient + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +func TestTransportFollowsBearerChallengeAndCachesToken(t *testing.T) { + var registryRequests int + var tokenRequests int + var server *httptest.Server + + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/token": + tokenRequests++ + if got := r.URL.Query().Get("service"); got != "registry.test" { + t.Errorf("service = %q, want %q", got, "registry.test") + } + if got := r.URL.Query().Get("scope"); got != "repository:library/test:pull" { + t.Errorf("scope = %q, want %q", got, "repository:library/test:pull") + } + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"token":"registry-token","expires_in":3600}`) + case "/v2/library/test/blobs/sha256:first", "/v2/library/test/blobs/sha256:second": + registryRequests++ + if r.Header.Get("Authorization") != "Bearer registry-token" { + w.Header().Set("WWW-Authenticate", `Bearer realm="`+server.URL+`/token",service="registry.test",scope="repository:library/test:pull"`) + http.Error(w, "authentication required", http.StatusUnauthorized) + return + } + _, _ = io.WriteString(w, "blob") + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + client := &http.Client{Transport: NewTransport(http.DefaultTransport, nil)} + for _, digest := range []string{"sha256:first", "sha256:second"} { + resp, err := client.Get(server.URL + "/v2/library/test/blobs/" + digest) + if err != nil { + t.Fatalf("GET %s: %v", digest, err) + } + body, readErr := io.ReadAll(resp.Body) + _ = resp.Body.Close() + if readErr != nil { + t.Fatalf("read %s response: %v", digest, readErr) + } + if resp.StatusCode != http.StatusOK { + t.Fatalf("GET %s status = %d, want %d", digest, resp.StatusCode, http.StatusOK) + } + if string(body) != "blob" { + t.Errorf("GET %s body = %q, want %q", digest, body, "blob") + } + } + + if tokenRequests != 1 { + t.Errorf("token requests = %d, want 1", tokenRequests) + } + if registryRequests != 3 { + t.Errorf("registry requests = %d, want 3", registryRequests) + } +} + +func TestTransportAddsConfiguredAuthentication(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("X-Registry-Token"); got != "configured-token" { + t.Errorf("X-Registry-Token = %q, want %q", got, "configured-token") + } + w.WriteHeader(http.StatusNoContent) + })) + defer server.Close() + + authForURL := func(url string) (string, string) { + if strings.HasPrefix(url, server.URL) { + return "X-Registry-Token", "configured-token" + } + return "", "" + } + client := &http.Client{Transport: NewTransport(http.DefaultTransport, authForURL)} + + resp, err := client.Get(server.URL + "/metadata") + if err != nil { + t.Fatalf("GET metadata: %v", err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusNoContent { + t.Errorf("status = %d, want %d", resp.StatusCode, http.StatusNoContent) + } +} + +func TestTransportPreservesExplicitAuthentication(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("Authorization"); got != "Bearer explicit-token" { + t.Errorf("Authorization = %q, want %q", got, "Bearer explicit-token") + } + w.WriteHeader(http.StatusNoContent) + })) + defer server.Close() + + authForURL := func(string) (string, string) { + return "Authorization", "Bearer configured-token" + } + client := &http.Client{Transport: NewTransport(http.DefaultTransport, authForURL)} + req, err := http.NewRequest(http.MethodGet, server.URL+"/artifact", nil) + if err != nil { + t.Fatal(err) + } + req.Header.Set("Authorization", "Bearer explicit-token") + + resp, err := client.Do(req) + if err != nil { + t.Fatalf("GET artifact: %v", err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusNoContent { + t.Errorf("status = %d, want %d", resp.StatusCode, http.StatusNoContent) + } +} + +func TestTransportDoesNotReplaceExplicitAuthenticationAfterBearerChallenge(t *testing.T) { + var registryRequests int + var tokenRequests int + var server *httptest.Server + + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/token": + tokenRequests++ + _, _ = io.WriteString(w, `{"token":"registry-token"}`) + case "/v2/library/test/blobs/sha256:test": + registryRequests++ + if got := r.Header.Get("Authorization"); got != "Bearer explicit-token" { + t.Errorf("Authorization = %q, want %q", got, "Bearer explicit-token") + } + w.Header().Set("WWW-Authenticate", `Bearer realm="`+server.URL+`/token"`) + http.Error(w, "authentication required", http.StatusUnauthorized) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + client := &http.Client{Transport: NewTransport(http.DefaultTransport, nil)} + req, err := http.NewRequest(http.MethodGet, server.URL+"/v2/library/test/blobs/sha256:test", nil) + if err != nil { + t.Fatal(err) + } + req.Header.Set("Authorization", "Bearer explicit-token") + + resp, err := client.Do(req) + if err != nil { + t.Fatalf("GET blob: %v", err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusUnauthorized { + t.Errorf("status = %d, want %d", resp.StatusCode, http.StatusUnauthorized) + } + if registryRequests != 1 { + t.Errorf("registry requests = %d, want 1", registryRequests) + } + if tokenRequests != 0 { + t.Errorf("token requests = %d, want 0", tokenRequests) + } +} + +func TestTransportDoesNotForwardConfiguredAuthenticationOnTokenRedirect(t *testing.T) { + destination := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("X-Registry-Token"); got != "" { + t.Errorf("redirected X-Registry-Token = %q, want empty", got) + } + _, _ = io.WriteString(w, `{"token":"registry-token"}`) + })) + defer destination.Close() + + source := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("X-Registry-Token"); got != "configured-token" { + t.Errorf("source X-Registry-Token = %q, want %q", got, "configured-token") + } + http.Redirect(w, r, destination.URL+"/token", http.StatusFound) + })) + defer source.Close() + + authForURL := func(rawURL string) (string, string) { + if strings.HasPrefix(rawURL, source.URL) { + return "X-Registry-Token", "configured-token" + } + return "", "" + } + transport := NewTransport(http.DefaultTransport, authForURL) + token, _, err := transport.fetchToken(context.Background(), bearerChallenge{realm: source.URL + "/token"}) + if err != nil { + t.Fatalf("fetchToken: %v", err) + } + if token != "registry-token" { + t.Errorf("token = %q, want %q", token, "registry-token") + } +} + +func TestTransportPrunesExpiredTokens(t *testing.T) { + transport := NewTransport(http.DefaultTransport, nil) + transport.tokens["expired-unused"] = cachedToken{ + value: "expired-token", + expiresAt: time.Now().Add(-time.Minute), + } + transport.cacheToken("current", cachedToken{ + value: "current-token", + expiresAt: time.Now().Add(time.Minute), + }) + + if got := transport.cachedToken("current"); got != "current-token" { + t.Errorf("cachedToken(current) = %q, want %q", got, "current-token") + } + if _, ok := transport.tokens["expired-unused"]; ok { + t.Error("expired unused token was not pruned") + } +} + +func TestTransportDoesNotFollowBearerChallengeOutsideOCIRegistry(t *testing.T) { + tokenRequests := 0 + var server *httptest.Server + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/token" { + tokenRequests++ + _, _ = io.WriteString(w, `{"token":"unexpected"}`) + return + } + w.Header().Set("WWW-Authenticate", `Bearer realm="`+server.URL+`/token"`) + http.Error(w, "authentication required", http.StatusUnauthorized) + })) + defer server.Close() + + client := &http.Client{Transport: NewTransport(http.DefaultTransport, nil)} + resp, err := client.Get(server.URL + "/api/packages") + if err != nil { + t.Fatalf("GET API: %v", err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusUnauthorized { + t.Errorf("status = %d, want %d", resp.StatusCode, http.StatusUnauthorized) + } + if tokenRequests != 0 { + t.Errorf("token requests = %d, want 0", tokenRequests) + } +} diff --git a/internal/server/server.go b/internal/server/server.go index 856fe2d..c98d1cb 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -58,17 +58,19 @@ import ( "strings" "time" + "github.com/git-pkgs/cooldown" swaggerdoc "github.com/git-pkgs/proxy/docs/swagger" "github.com/git-pkgs/proxy/internal/config" - "github.com/git-pkgs/cooldown" "github.com/git-pkgs/proxy/internal/database" "github.com/git-pkgs/proxy/internal/enrichment" "github.com/git-pkgs/proxy/internal/handler" + upstreamhttp "github.com/git-pkgs/proxy/internal/httpclient" "github.com/git-pkgs/proxy/internal/metrics" "github.com/git-pkgs/proxy/internal/mirror" "github.com/git-pkgs/proxy/internal/storage" "github.com/git-pkgs/purl" "github.com/git-pkgs/registries/fetch" + "github.com/git-pkgs/registries/safehttp" "github.com/git-pkgs/spdx" "github.com/go-chi/chi/v5" "github.com/go-chi/chi/v5/middleware" @@ -84,12 +86,12 @@ const ( // Server is the main proxy server. type Server struct { - cfg *config.Config - db *database.DB - storage storage.Storage - logger *slog.Logger - http *http.Server - templates *Templates + cfg *config.Config + db *database.DB + storage storage.Storage + logger *slog.Logger + http *http.Server + templates *Templates cancel context.CancelFunc healthCache *healthCache } @@ -156,8 +158,18 @@ func New(cfg *config.Config, logger *slog.Logger) (*Server, error) { // Start starts the HTTP server. func (s *Server) Start() error { - // Create shared components with circuit breaker - baseFetcher := fetch.NewFetcher(fetch.WithAuthFunc(s.authForURL)) + // Use one authentication-aware transport for metadata and artifacts so + // configured credentials and cached OCI challenges apply consistently. + safeClient := safehttp.New(nil, safehttp.Options{}) + authTransport := upstreamhttp.NewTransport(safeClient.Transport, upstreamhttp.AuthFunc(s.authForURL)) + metadataClient := *safeClient + metadataClient.Timeout = s.cfg.ParseHTTPTimeout() + metadataClient.Transport = authTransport + artifactClient := metadataClient + artifactClient.Timeout = serverWriteTimeout + + // Create shared components with circuit breaker. + baseFetcher := fetch.NewFetcher(fetch.WithHTTPClient(&artifactClient)) fetcher := fetch.NewCircuitBreakerFetcher(baseFetcher) resolver := fetch.NewResolver() cd := &cooldown.Config{ @@ -166,7 +178,7 @@ func (s *Server) Start() error { Packages: s.cfg.Cooldown.NormalizedPackages(), } proxy := handler.NewProxy(s.db, s.storage, fetcher, resolver, s.logger) - proxy.HTTPClient.Timeout = s.cfg.ParseHTTPTimeout() + proxy.HTTPClient = &metadataClient proxy.AuthForURL = s.authForURL proxy.Cooldown = cd proxy.CacheMetadata = s.cfg.CacheMetadata From 538a15d9f8950e401e7de09479b31a36ef141c52 Mon Sep 17 00:00:00 2001 From: Andrew Nesbitt Date: Thu, 13 Aug 2026 07:35:07 +0100 Subject: [PATCH 06/24] fix(container): serve cached images when upstream is unavailable (#199) * container: cache manifests for offline pulls * Preserve direct-serve redirects for blob HEAD requests --- docs/architecture.md | 1 + docs/configuration.md | 2 + internal/database/metadata_cache_test.go | 55 +- internal/database/queries.go | 14 +- internal/database/schema.go | 19 + internal/database/types.go | 23 +- internal/handler/container.go | 131 +---- internal/handler/container_manifest.go | 251 +++++++++ internal/handler/container_test.go | 560 ++++++++++++++++--- internal/handler/handler.go | 39 +- internal/handler/handler_test.go | 6 +- internal/handler/notfound_ecosystems_test.go | 16 +- 12 files changed, 888 insertions(+), 229 deletions(-) create mode 100644 internal/handler/container_manifest.go diff --git a/docs/architecture.md b/docs/architecture.md index cf8b0e2..6d9bfda 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -353,6 +353,7 @@ Eviction can be implemented as: - Fresh data - new versions visible immediately - Metadata is small, upstream fetch is fast - Set `cache_metadata: true` or use the mirror command to enable metadata caching for offline use via the `metadata_cache` table +- OCI manifests are the exception: they are cached automatically so previously fetched images remain pullable when the registry or token service is unavailable **Why stream artifacts?** - Memory efficient - don't load large files into RAM diff --git a/docs/configuration.md b/docs/configuration.md index fa0f576..1e5a1c7 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -248,6 +248,8 @@ Note: Hex cooldown requires disabling registry signature verification since the By default the proxy fetches metadata fresh from upstream on every request. Enable `cache_metadata` to store metadata responses in the database and storage backend for offline fallback. When upstream is unreachable, the proxy serves the last cached copy. ETag-based revalidation avoids re-downloading unchanged metadata. +OCI manifests are always cached because cached image blobs cannot be pulled without their manifests. Digest-addressed manifests are immutable and served directly from cache. Tag-addressed manifests follow `metadata_ttl`, revalidate when stale, and fall back to the last cached response when the registry is unavailable. + ```yaml cache_metadata: true ``` diff --git a/internal/database/metadata_cache_test.go b/internal/database/metadata_cache_test.go index 5701816..09dcba3 100644 --- a/internal/database/metadata_cache_test.go +++ b/internal/database/metadata_cache_test.go @@ -30,8 +30,12 @@ func TestUpsertAndGetMetadataCache(t *testing.T) { StoragePath: "_metadata/npm/lodash/metadata", ETag: sql.NullString{String: `"abc123"`, Valid: true}, ContentType: sql.NullString{String: "application/json", Valid: true}, - Size: sql.NullInt64{Int64: 1024, Valid: true}, - FetchedAt: sql.NullTime{Time: time.Now(), Valid: true}, + ContentDigest: sql.NullString{ + String: "sha256:0123456789abcdef", + Valid: true, + }, + Size: sql.NullInt64{Int64: 1024, Valid: true}, + FetchedAt: sql.NullTime{Time: time.Now(), Valid: true}, } err := db.UpsertMetadataCache(entry) @@ -62,6 +66,9 @@ func TestUpsertAndGetMetadataCache(t *testing.T) { if !got.ContentType.Valid || got.ContentType.String != "application/json" { t.Errorf("content_type = %v, want %q", got.ContentType, "application/json") } + if !got.ContentDigest.Valid || got.ContentDigest.String != "sha256:0123456789abcdef" { + t.Errorf("content_digest = %v, want %q", got.ContentDigest, "sha256:0123456789abcdef") + } if !got.Size.Valid || got.Size.Int64 != 1024 { t.Errorf("size = %v, want 1024", got.Size) } @@ -178,3 +185,47 @@ func TestMetadataCacheTableCreatedByMigration(t *testing.T) { t.Error("metadata_cache table should exist after migration") } } + +func TestMetadataCacheContentDigestMigrationPreservesExistingRows(t *testing.T) { + dbPath := filepath.Join(t.TempDir(), "test.db") + db, err := Create(dbPath) + if err != nil { + t.Fatalf("Create failed: %v", err) + } + defer func() { _ = db.Close() }() + + if _, err := db.Exec("ALTER TABLE metadata_cache DROP COLUMN content_digest"); err != nil { + t.Fatalf("dropping content_digest: %v", err) + } + if _, err := db.Exec("DELETE FROM migrations WHERE name = ?", "006_add_metadata_content_digest"); err != nil { + t.Fatalf("resetting digest migration: %v", err) + } + if _, err := db.Exec(` + INSERT INTO metadata_cache (ecosystem, name, storage_path, content_type, size, fetched_at, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + `, "oci-manifest", "cache-key", "_metadata/oci-manifest/cache-key/metadata", "application/json", 2, time.Now(), time.Now(), time.Now()); err != nil { + t.Fatalf("inserting legacy cache row: %v", err) + } + + if err := db.MigrateSchema(); err != nil { + t.Fatalf("MigrateSchema() error = %v", err) + } + hasDigest, err := db.HasColumn("metadata_cache", "content_digest") + if err != nil { + t.Fatalf("HasColumn() error = %v", err) + } + if !hasDigest { + t.Fatal("metadata_cache.content_digest was not added") + } + + entry, err := db.GetMetadataCache("oci-manifest", "cache-key") + if err != nil { + t.Fatalf("GetMetadataCache() error = %v", err) + } + if entry == nil || entry.StoragePath != "_metadata/oci-manifest/cache-key/metadata" { + t.Fatalf("existing metadata cache row was not preserved: %#v", entry) + } + if entry.ContentDigest.Valid { + t.Errorf("legacy content digest = %q, want NULL", entry.ContentDigest.String) + } +} diff --git a/internal/database/queries.go b/internal/database/queries.go index 5d95596..f8b76fe 100644 --- a/internal/database/queries.go +++ b/internal/database/queries.go @@ -894,7 +894,7 @@ func (db *DB) GetMetadataCache(ecosystem, name string) (*MetadataCacheEntry, err var entry MetadataCacheEntry query := db.Rebind(` SELECT id, ecosystem, name, storage_path, etag, content_type, - size, last_modified, fetched_at, created_at, updated_at + content_digest, size, last_modified, fetched_at, created_at, updated_at FROM metadata_cache WHERE ecosystem = ? AND name = ? `) err := db.Get(&entry, query, ecosystem, name) @@ -914,12 +914,13 @@ func (db *DB) UpsertMetadataCache(entry *MetadataCacheEntry) error { if db.dialect == DialectPostgres { query = ` INSERT INTO metadata_cache (ecosystem, name, storage_path, etag, content_type, - size, last_modified, fetched_at, created_at, updated_at) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) + content_digest, size, last_modified, fetched_at, created_at, updated_at) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) ON CONFLICT(ecosystem, name) DO UPDATE SET storage_path = EXCLUDED.storage_path, etag = EXCLUDED.etag, content_type = EXCLUDED.content_type, + content_digest = EXCLUDED.content_digest, size = EXCLUDED.size, last_modified = EXCLUDED.last_modified, fetched_at = EXCLUDED.fetched_at, @@ -928,12 +929,13 @@ func (db *DB) UpsertMetadataCache(entry *MetadataCacheEntry) error { } else { query = ` INSERT INTO metadata_cache (ecosystem, name, storage_path, etag, content_type, - size, last_modified, fetched_at, created_at, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + content_digest, size, last_modified, fetched_at, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON CONFLICT(ecosystem, name) DO UPDATE SET storage_path = excluded.storage_path, etag = excluded.etag, content_type = excluded.content_type, + content_digest = excluded.content_digest, size = excluded.size, last_modified = excluded.last_modified, fetched_at = excluded.fetched_at, @@ -943,7 +945,7 @@ func (db *DB) UpsertMetadataCache(entry *MetadataCacheEntry) error { _, err := db.Exec(query, entry.Ecosystem, entry.Name, entry.StoragePath, entry.ETag, - entry.ContentType, entry.Size, entry.LastModified, entry.FetchedAt, now, now, + entry.ContentType, entry.ContentDigest, entry.Size, entry.LastModified, entry.FetchedAt, now, now, ) if err != nil { return fmt.Errorf("upserting metadata cache: %w", err) diff --git a/internal/database/schema.go b/internal/database/schema.go index c8d8d1e..c73877d 100644 --- a/internal/database/schema.go +++ b/internal/database/schema.go @@ -102,6 +102,7 @@ CREATE TABLE IF NOT EXISTS metadata_cache ( storage_path TEXT NOT NULL, etag TEXT, content_type TEXT, + content_digest TEXT, size INTEGER, last_modified DATETIME, fetched_at DATETIME, @@ -202,6 +203,7 @@ CREATE TABLE IF NOT EXISTS metadata_cache ( storage_path TEXT NOT NULL, etag TEXT, content_type TEXT, + content_digest TEXT, size BIGINT, last_modified TIMESTAMP, fetched_at TIMESTAMP, @@ -359,6 +361,7 @@ var migrations = []migration{ {"003_ensure_artifacts_table", migrateEnsureArtifactsTable}, {"004_ensure_vulnerabilities_table", migrateEnsureVulnerabilitiesTable}, {"005_ensure_metadata_cache_table", migrateEnsureMetadataCacheTable}, + {"006_add_metadata_content_digest", migrateAddMetadataContentDigest}, } // isTableNotFound returns true if the error indicates a missing table. @@ -581,6 +584,20 @@ func migrateEnsureMetadataCacheTable(db *DB) error { return db.EnsureMetadataCacheTable() } +func migrateAddMetadataContentDigest(db *DB) error { + hasColumn, err := db.HasColumn("metadata_cache", "content_digest") + if err != nil { + return fmt.Errorf("checking metadata_cache content_digest column: %w", err) + } + if hasColumn { + return nil + } + if _, err := db.Exec("ALTER TABLE metadata_cache ADD COLUMN content_digest TEXT"); err != nil { + return fmt.Errorf("adding metadata_cache content_digest column: %w", err) + } + return nil +} + // EnsureMetadataCacheTable creates the metadata_cache table if it doesn't exist. func (db *DB) EnsureMetadataCacheTable() error { has, err := db.HasTable("metadata_cache") @@ -601,6 +618,7 @@ func (db *DB) EnsureMetadataCacheTable() error { storage_path TEXT NOT NULL, etag TEXT, content_type TEXT, + content_digest TEXT, size BIGINT, last_modified TIMESTAMP, fetched_at TIMESTAMP, @@ -618,6 +636,7 @@ func (db *DB) EnsureMetadataCacheTable() error { storage_path TEXT NOT NULL, etag TEXT, content_type TEXT, + content_digest TEXT, size INTEGER, last_modified DATETIME, fetched_at DATETIME, diff --git a/internal/database/types.go b/internal/database/types.go index 47dc47e..7128f12 100644 --- a/internal/database/types.go +++ b/internal/database/types.go @@ -78,17 +78,18 @@ func (a *Artifact) IsCached() bool { // MetadataCacheEntry represents a cached metadata blob for offline serving. type MetadataCacheEntry struct { - ID int64 `db:"id" json:"id"` - Ecosystem string `db:"ecosystem" json:"ecosystem"` - Name string `db:"name" json:"name"` - StoragePath string `db:"storage_path" json:"storage_path"` - ETag sql.NullString `db:"etag" json:"etag,omitempty"` - ContentType sql.NullString `db:"content_type" json:"content_type,omitempty"` - Size sql.NullInt64 `db:"size" json:"size,omitempty"` - LastModified sql.NullTime `db:"last_modified" json:"last_modified,omitempty"` - FetchedAt sql.NullTime `db:"fetched_at" json:"fetched_at,omitempty"` - CreatedAt time.Time `db:"created_at" json:"created_at"` - UpdatedAt time.Time `db:"updated_at" json:"updated_at"` + ID int64 `db:"id" json:"id"` + Ecosystem string `db:"ecosystem" json:"ecosystem"` + Name string `db:"name" json:"name"` + StoragePath string `db:"storage_path" json:"storage_path"` + ETag sql.NullString `db:"etag" json:"etag,omitempty"` + ContentType sql.NullString `db:"content_type" json:"content_type,omitempty"` + ContentDigest sql.NullString `db:"content_digest" json:"content_digest,omitempty"` + Size sql.NullInt64 `db:"size" json:"size,omitempty"` + LastModified sql.NullTime `db:"last_modified" json:"last_modified,omitempty"` + FetchedAt sql.NullTime `db:"fetched_at" json:"fetched_at,omitempty"` + CreatedAt time.Time `db:"created_at" json:"created_at"` + UpdatedAt time.Time `db:"updated_at" json:"updated_at"` } // Vulnerability represents a cached vulnerability record. diff --git a/internal/handler/container.go b/internal/handler/container.go index 0710ed1..3a3b267 100644 --- a/internal/handler/container.go +++ b/internal/handler/container.go @@ -12,7 +12,6 @@ import ( const ( dockerHubRegistry = "https://registry-1.docker.io" - dockerHubAuth = "https://auth.docker.io" blobMatchCount = 3 // full match + name + digest manifestMatchCount = 3 // full match + name + reference tagsListMatchCount = 2 // full match + name @@ -24,7 +23,6 @@ const ( type ContainerHandler struct { proxy *Proxy registryURL string - authURL string proxyURL string } @@ -33,7 +31,6 @@ func NewContainerHandler(proxy *Proxy, proxyURL string) *ContainerHandler { return &ContainerHandler{ proxy: proxy, registryURL: dockerHubRegistry, - authURL: dockerHubAuth, proxyURL: strings.TrimSuffix(proxyURL, "/"), } } @@ -90,31 +87,34 @@ func (h *ContainerHandler) handleBlobDownload(w http.ResponseWriter, r *http.Req h.proxy.Logger.Info("container blob request", "name", name, "digest", digest) - // Get auth token for upstream - token, err := h.getAuthToken(r.Context(), name, "pull") + filename := digest + cached, err := h.proxy.GetCachedArtifact(r.Context(), "oci", name, digest, filename) if err != nil { - h.proxy.Logger.Error("failed to get auth token", "error", err) - h.containerError(w, http.StatusUnauthorized, "UNAUTHORIZED", "failed to authenticate") + h.proxy.Logger.Error("failed to check blob cache", "error", err) + h.containerError(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to check blob cache") + return + } + if cached != nil { + w.Header().Set("Docker-Content-Digest", digest) + w.Header().Set("Content-Type", "application/octet-stream") + serveArtifact(w, r.Method, cached) return } // For HEAD requests, just proxy to upstream if r.Method == http.MethodHead { - h.proxyBlobHead(w, r, name, digest, token) + h.proxyBlobHead(w, r, name, digest) return } - // Try to get from cache, or fetch from upstream with auth - filename := digest - headers := http.Header{"Authorization": {"Bearer " + token}} - result, err := h.proxy.GetOrFetchArtifactFromURLWithHeaders( + // Try to get from cache, or fetch from the authentication-aware upstream client. + result, err := h.proxy.GetOrFetchArtifactFromURL( r.Context(), "oci", name, digest, // use digest as version filename, fmt.Sprintf("%s/v2/%s/blobs/%s", h.registryURL, name, digest), - headers, ) if err != nil { @@ -132,8 +132,7 @@ func (h *ContainerHandler) handleBlobDownload(w http.ResponseWriter, r *http.Req ServeArtifact(w, result) } -// handleManifest proxies manifest requests to upstream. -// Manifests change when tags are updated, so we proxy these directly. +// handleManifest serves immutable manifests from cache and revalidates mutable tags. // Path format: {name}/manifests/{reference} func (h *ContainerHandler) handleManifest(w http.ResponseWriter, r *http.Request, path string) { if r.Method != http.MethodGet && r.Method != http.MethodHead { @@ -148,57 +147,7 @@ func (h *ContainerHandler) handleManifest(w http.ResponseWriter, r *http.Request } h.proxy.Logger.Info("container manifest request", "name", name, "reference", reference) - - // Get auth token - token, err := h.getAuthToken(r.Context(), name, "pull") - if err != nil { - h.proxy.Logger.Error("failed to get auth token", "error", err) - h.containerError(w, http.StatusUnauthorized, "UNAUTHORIZED", "failed to authenticate") - return - } - - // Proxy to upstream - upstreamURL := fmt.Sprintf("%s/v2/%s/manifests/%s", h.registryURL, name, reference) - - req, err := http.NewRequestWithContext(r.Context(), r.Method, upstreamURL, nil) - if err != nil { - h.containerError(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to create request") - return - } - - req.Header.Set("Authorization", "Bearer "+token) - - // Forward Accept header for content negotiation - if accept := r.Header.Get("Accept"); accept != "" { - req.Header.Set("Accept", accept) - } else { - // Default accept headers for manifests - req.Header.Set("Accept", strings.Join([]string{ - "application/vnd.oci.image.manifest.v1+json", - "application/vnd.oci.image.index.v1+json", - "application/vnd.docker.distribution.manifest.v2+json", - "application/vnd.docker.distribution.manifest.list.v2+json", - "application/vnd.docker.distribution.manifest.v1+prettyjws", - }, ", ")) - } - - resp, err := h.proxy.HTTPClient.Do(req) - if err != nil { - h.proxy.Logger.Error("failed to fetch manifest", "error", err) - h.containerError(w, http.StatusBadGateway, "INTERNAL_ERROR", "failed to fetch from upstream") - return - } - defer func() { _ = resp.Body.Close() }() - - // Copy relevant headers - for _, header := range []string{"Content-Type", "Content-Length", "Docker-Content-Digest", "ETag"} { - if v := resp.Header.Get(header); v != "" { - w.Header().Set(header, v) - } - } - - w.WriteHeader(resp.StatusCode) - _, _ = io.Copy(w, resp.Body) + h.serveManifest(w, r, name, reference) } // handleTagsList proxies tag list requests to upstream. @@ -214,13 +163,6 @@ func (h *ContainerHandler) handleTagsList(w http.ResponseWriter, r *http.Request return } - // Get auth token - token, err := h.getAuthToken(r.Context(), name, "pull") - if err != nil { - h.containerError(w, http.StatusUnauthorized, "UNAUTHORIZED", "failed to authenticate") - return - } - upstreamURL := fmt.Sprintf("%s/v2/%s/tags/list", h.registryURL, name) if r.URL.RawQuery != "" { upstreamURL += "?" + r.URL.RawQuery @@ -232,8 +174,6 @@ func (h *ContainerHandler) handleTagsList(w http.ResponseWriter, r *http.Request return } - req.Header.Set("Authorization", "Bearer "+token) - resp, err := h.proxy.HTTPClient.Do(req) if err != nil { h.containerError(w, http.StatusBadGateway, "INTERNAL_ERROR", "failed to fetch from upstream") @@ -246,45 +186,8 @@ func (h *ContainerHandler) handleTagsList(w http.ResponseWriter, r *http.Request _, _ = io.Copy(w, resp.Body) } -// getAuthToken gets a bearer token for the specified repository. -// Docker Hub requires auth even for public images. -func (h *ContainerHandler) getAuthToken(_ interface{ Done() <-chan struct{} }, repository, action string) (string, error) { - // For Docker Hub: https://auth.docker.io/token?service=registry.docker.io&scope=repository:{repo}:pull - authURL := fmt.Sprintf("%s/token?service=registry.docker.io&scope=repository:%s:%s", - h.authURL, repository, action) - - req, err := http.NewRequest(http.MethodGet, authURL, nil) - if err != nil { - return "", err - } - - resp, err := h.proxy.HTTPClient.Do(req) - if err != nil { - return "", err - } - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode != http.StatusOK { - return "", fmt.Errorf("auth failed with status %d", resp.StatusCode) - } - - var tokenResp struct { - Token string `json:"token"` - AccessToken string `json:"access_token"` - } - - if err := json.NewDecoder(resp.Body).Decode(&tokenResp); err != nil { - return "", err - } - - if tokenResp.Token != "" { - return tokenResp.Token, nil - } - return tokenResp.AccessToken, nil -} - // proxyBlobHead handles HEAD requests for blobs. -func (h *ContainerHandler) proxyBlobHead(w http.ResponseWriter, r *http.Request, name, digest, token string) { +func (h *ContainerHandler) proxyBlobHead(w http.ResponseWriter, r *http.Request, name, digest string) { upstreamURL := fmt.Sprintf("%s/v2/%s/blobs/%s", h.registryURL, name, digest) req, err := http.NewRequestWithContext(r.Context(), http.MethodHead, upstreamURL, nil) @@ -293,8 +196,6 @@ func (h *ContainerHandler) proxyBlobHead(w http.ResponseWriter, r *http.Request, return } - req.Header.Set("Authorization", "Bearer "+token) - resp, err := h.proxy.HTTPClient.Do(req) if err != nil { h.containerError(w, http.StatusBadGateway, "INTERNAL_ERROR", "failed to fetch from upstream") diff --git a/internal/handler/container_manifest.go b/internal/handler/container_manifest.go new file mode 100644 index 0000000..245ced4 --- /dev/null +++ b/internal/handler/container_manifest.go @@ -0,0 +1,251 @@ +package handler + +import ( + "bytes" + "context" + "crypto/sha256" + "database/sql" + "encoding/hex" + "fmt" + "io" + "net/http" + "regexp" + "strconv" + "strings" + "time" + + "github.com/git-pkgs/proxy/internal/database" +) + +const ( + containerManifestCacheEcosystem = "oci-manifest" + containerStaleWarning = `110 - "Response is Stale"` +) + +var manifestDigestReferencePattern = regexp.MustCompile(`^[a-z0-9]+:[a-f0-9]+$`) + +type cachedContainerManifest struct { + body []byte + contentType string + contentDigest string + etag string + size int64 + fetchedAt time.Time +} + +func (h *ContainerHandler) serveManifest(w http.ResponseWriter, r *http.Request, name, reference string) { + accept := containerManifestAccept(r) + cacheKey := h.containerManifestCacheKey(name, reference, accept) + cached, err := h.loadContainerManifest(r.Context(), cacheKey) + if err != nil { + h.proxy.Logger.Warn("failed to read cached container manifest", "error", err) + cached = nil + } + + immutable := manifestDigestReferencePattern.MatchString(reference) + if cached != nil && (immutable || h.containerManifestFresh(cached)) { + writeContainerManifest(w, r.Method, cached, false) + return + } + + upstreamURL := fmt.Sprintf("%s/v2/%s/manifests/%s", h.registryURL, name, reference) + req, err := http.NewRequestWithContext(r.Context(), r.Method, upstreamURL, nil) + if err != nil { + h.containerError(w, http.StatusInternalServerError, "INTERNAL_ERROR", "failed to create request") + return + } + req.Header.Set("Accept", accept) + if cached != nil && cached.etag != "" { + req.Header.Set("If-None-Match", cached.etag) + } + + resp, err := h.proxy.HTTPClient.Do(req) + if err != nil { + h.serveStaleManifestOrError(w, r, cached, err) + return + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode == http.StatusNotModified && cached != nil { + cached.fetchedAt = time.Now() + if err := h.storeContainerManifest(r.Context(), cacheKey, cached); err != nil { + h.proxy.Logger.Warn("failed to refresh cached container manifest", "error", err) + } + writeContainerManifest(w, r.Method, cached, false) + return + } + if resp.StatusCode != http.StatusOK { + if cached != nil && shouldServeStaleManifest(resp.StatusCode) { + writeContainerManifest(w, r.Method, cached, true) + return + } + copyContainerManifestHeaders(w.Header(), resp.Header) + w.WriteHeader(resp.StatusCode) + _, _ = io.Copy(w, resp.Body) + return + } + + if r.Method == http.MethodHead { + copyContainerManifestHeaders(w.Header(), resp.Header) + w.WriteHeader(http.StatusOK) + return + } + + body, err := h.proxy.ReadMetadata(resp.Body) + if err != nil { + h.serveStaleManifestOrError(w, r, cached, fmt.Errorf("reading manifest: %w", err)) + return + } + manifest := &cachedContainerManifest{ + body: body, + contentType: resp.Header.Get("Content-Type"), + contentDigest: resp.Header.Get("Docker-Content-Digest"), + etag: resp.Header.Get("ETag"), + size: int64(len(body)), + fetchedAt: time.Now(), + } + if manifest.contentDigest == "" { + manifest.contentDigest = sha256Digest(body) + } + if err := h.storeContainerManifest(r.Context(), cacheKey, manifest); err != nil { + h.proxy.Logger.Warn("failed to cache container manifest", "error", err) + } + if manifest.contentDigest != reference && manifestDigestReferencePattern.MatchString(manifest.contentDigest) { + digestKey := h.containerManifestCacheKey(name, manifest.contentDigest, accept) + if err := h.storeContainerManifest(r.Context(), digestKey, manifest); err != nil { + h.proxy.Logger.Warn("failed to cache container manifest by digest", "error", err) + } + } + writeContainerManifest(w, r.Method, manifest, false) +} + +func (h *ContainerHandler) serveStaleManifestOrError(w http.ResponseWriter, r *http.Request, cached *cachedContainerManifest, err error) { + if cached != nil { + h.proxy.Logger.Warn("upstream manifest fetch failed, serving stale cache", "error", err) + writeContainerManifest(w, r.Method, cached, true) + return + } + h.proxy.Logger.Error("failed to fetch manifest", "error", err) + h.containerError(w, http.StatusBadGateway, "INTERNAL_ERROR", "failed to fetch from upstream") +} + +func (h *ContainerHandler) containerManifestFresh(manifest *cachedContainerManifest) bool { + return h.proxy.MetadataTTL > 0 && !manifest.fetchedAt.IsZero() && time.Since(manifest.fetchedAt) < h.proxy.MetadataTTL +} + +func (h *ContainerHandler) containerManifestCacheKey(name, reference, accept string) string { + identity := strings.Join([]string{h.registryURL, name, reference, accept}, "\x00") + sum := sha256.Sum256([]byte(identity)) + return hex.EncodeToString(sum[:]) +} + +func (h *ContainerHandler) loadContainerManifest(ctx context.Context, cacheKey string) (*cachedContainerManifest, error) { + if h.proxy.DB == nil || h.proxy.Storage == nil { + return nil, nil + } + entry, err := h.proxy.DB.GetMetadataCache(containerManifestCacheEcosystem, cacheKey) + if err != nil || entry == nil { + return nil, err + } + reader, err := h.proxy.Storage.Open(ctx, entry.StoragePath) + if err != nil { + return nil, nil + } + defer func() { _ = reader.Close() }() + body, err := h.proxy.ReadMetadata(reader) + if err != nil { + return nil, err + } + + manifest := &cachedContainerManifest{body: body, size: int64(len(body))} + if entry.ContentType.Valid { + manifest.contentType = entry.ContentType.String + } + if entry.ContentDigest.Valid { + manifest.contentDigest = entry.ContentDigest.String + } else { + manifest.contentDigest = sha256Digest(body) + } + if entry.ETag.Valid { + manifest.etag = entry.ETag.String + } + if entry.Size.Valid { + manifest.size = entry.Size.Int64 + } + if entry.FetchedAt.Valid { + manifest.fetchedAt = entry.FetchedAt.Time + } + return manifest, nil +} + +func (h *ContainerHandler) storeContainerManifest(ctx context.Context, cacheKey string, manifest *cachedContainerManifest) error { + if h.proxy.DB == nil || h.proxy.Storage == nil { + return nil + } + storagePath := metadataStoragePath(containerManifestCacheEcosystem, cacheKey) + size, _, err := h.proxy.Storage.Store(ctx, storagePath, bytes.NewReader(manifest.body)) + if err != nil { + return fmt.Errorf("storing manifest: %w", err) + } + manifest.size = size + return h.proxy.DB.UpsertMetadataCache(&database.MetadataCacheEntry{ + Ecosystem: containerManifestCacheEcosystem, + Name: cacheKey, + StoragePath: storagePath, + ETag: sql.NullString{String: manifest.etag, Valid: manifest.etag != ""}, + ContentType: sql.NullString{String: manifest.contentType, Valid: manifest.contentType != ""}, + ContentDigest: sql.NullString{String: manifest.contentDigest, Valid: manifest.contentDigest != ""}, + Size: sql.NullInt64{Int64: size, Valid: true}, + FetchedAt: sql.NullTime{Time: manifest.fetchedAt, Valid: !manifest.fetchedAt.IsZero()}, + }) +} + +func writeContainerManifest(w http.ResponseWriter, method string, manifest *cachedContainerManifest, stale bool) { + if manifest.contentType != "" { + w.Header().Set("Content-Type", manifest.contentType) + } + w.Header().Set("Content-Length", strconv.FormatInt(manifest.size, 10)) + if manifest.contentDigest != "" { + w.Header().Set("Docker-Content-Digest", manifest.contentDigest) + } + if manifest.etag != "" { + w.Header().Set("ETag", manifest.etag) + } + if stale { + w.Header().Set("Warning", containerStaleWarning) + } + w.WriteHeader(http.StatusOK) + if method != http.MethodHead { + _, _ = w.Write(manifest.body) + } +} + +func containerManifestAccept(r *http.Request) string { + if accept := r.Header.Get("Accept"); accept != "" { + return accept + } + return strings.Join([]string{ + "application/vnd.oci.image.manifest.v1+json", + "application/vnd.oci.image.index.v1+json", + "application/vnd.docker.distribution.manifest.v2+json", + "application/vnd.docker.distribution.manifest.list.v2+json", + "application/vnd.docker.distribution.manifest.v1+prettyjws", + }, ", ") +} + +func copyContainerManifestHeaders(destination, source http.Header) { + for _, header := range []string{"Content-Type", "Content-Length", "Docker-Content-Digest", "ETag", "WWW-Authenticate"} { + if value := source.Get(header); value != "" { + destination.Set(header, value) + } + } +} + +func shouldServeStaleManifest(status int) bool { + return status == http.StatusTooManyRequests || status >= http.StatusInternalServerError +} + +func sha256Digest(body []byte) string { + digest := sha256.Sum256(body) + return "sha256:" + hex.EncodeToString(digest[:]) +} diff --git a/internal/handler/container_test.go b/internal/handler/container_test.go index 853059e..6e7322c 100644 --- a/internal/handler/container_test.go +++ b/internal/handler/container_test.go @@ -1,16 +1,15 @@ package handler import ( - "bytes" - "context" "encoding/json" "io" - "log/slog" "net/http" "net/http/httptest" + "strconv" "testing" + "time" - "github.com/git-pkgs/proxy/internal/database" + upstreamhttp "github.com/git-pkgs/proxy/internal/httpclient" "github.com/git-pkgs/registries/fetch" ) @@ -135,90 +134,521 @@ func TestContainerHandler_parseTagsListPath(t *testing.T) { } } -func TestContainerHandler_BlobDownload_CachesWithAuth(t *testing.T) { - // Set up a mock auth server that returns a token - authServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(map[string]string{"token": "test-token-123"}) +func TestContainerHandler_BlobDownload_DiscoversBearerChallenge(t *testing.T) { + digest := "sha256:abc123def456abc123def456abc123def456abc123def456abc123def456abcd" + registryRequests := 0 + tokenRequests := 0 + var upstream *httptest.Server + upstream = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/token": + tokenRequests++ + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "token": "discovered-token", + "expires_in": 3600, + }) + case "/v2/library/nginx/blobs/" + digest: + registryRequests++ + if r.Header.Get("Authorization") != "Bearer discovered-token" { + w.Header().Set("WWW-Authenticate", `Bearer realm="`+upstream.URL+`/token",service="registry.test",scope="repository:library/nginx:pull"`) + http.Error(w, "authentication required", http.StatusUnauthorized) + return + } + w.Header().Set("Content-Type", "application/octet-stream") + _, _ = io.WriteString(w, "upstream blob") + default: + http.NotFound(w, r) + } })) - defer authServer.Close() + defer upstream.Close() - // Set up mock fetcher that captures headers - var capturedHeaders http.Header - mf := &mockFetcherWithHeaders{ - fetchFn: func(_ context.Context, _ string, headers http.Header) (*fetch.Artifact, error) { - capturedHeaders = headers - return &fetch.Artifact{ - Body: io.NopCloser(bytes.NewReader([]byte("blob-content"))), - Size: 12, - ContentType: "application/octet-stream", - }, nil - }, - } - - dir := t.TempDir() - db, err := database.Create(dir + "/test.db") - if err != nil { - t.Fatalf("failed to create test database: %v", err) - } - t.Cleanup(func() { _ = db.Close() }) - - store := newMockStorage() - logger := slog.New(slog.NewTextHandler(io.Discard, nil)) - proxy := &Proxy{ - DB: db, - Storage: store, - Fetcher: mf, - Logger: logger, - HTTPClient: &http.Client{}, - } + proxy, _, _, _ := setupTestProxy(t) + authTransport := upstreamhttp.NewTransport(http.DefaultTransport, nil) + client := &http.Client{Transport: authTransport} + artifactFetcher := fetch.NewFetcher( + fetch.WithHTTPClient(client), + fetch.WithMaxRetries(0), + ) + t.Cleanup(func() { _ = artifactFetcher.Close() }) + proxy.Fetcher = artifactFetcher + proxy.HTTPClient = client h := &ContainerHandler{ proxy: proxy, - registryURL: "https://registry-1.docker.io", - authURL: authServer.URL, + registryURL: upstream.URL, proxyURL: "http://localhost:8080", } - handler := h.Routes() - req := httptest.NewRequest(http.MethodGet, "/library/nginx/blobs/sha256:abc123def456abc123def456abc123def456abc123def456abc123def456abcd", nil) + for range 2 { + req := httptest.NewRequest(http.MethodGet, "/library/nginx/blobs/"+digest, nil) + w := httptest.NewRecorder() + h.Routes().ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusOK, w.Body.String()) + } + if got := w.Body.String(); got != "upstream blob" { + t.Errorf("body = %q, want %q", got, "upstream blob") + } + } + + if tokenRequests != 1 { + t.Errorf("token requests = %d, want 1", tokenRequests) + } + if registryRequests != 2 { + t.Errorf("registry requests = %d, want 2", registryRequests) + } +} + +func TestContainerHandler_CachedImagePullSurvivesRegistryAndTokenOutages(t *testing.T) { + digest := "sha256:abc123def456abc123def456abc123def456abc123def456abc123def456abcd" + manifest := `{"schemaVersion":2,"mediaType":"application/vnd.oci.image.manifest.v1+json"}` + blob := "cached image blob" + registryAvailable := true + tokenAvailable := true + registryRequests := 0 + tokenRequests := 0 + + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + tokenRequests++ + if !tokenAvailable { + http.Error(w, "token service unavailable", http.StatusServiceUnavailable) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "token": "discovered-token", + "expires_in": 3600, + }) + })) + defer tokenServer.Close() + + registryServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + registryRequests++ + if !registryAvailable { + http.Error(w, "registry unavailable", http.StatusServiceUnavailable) + return + } + if r.Header.Get("Authorization") != "Bearer discovered-token" { + w.Header().Set("WWW-Authenticate", `Bearer realm="`+tokenServer.URL+`",service="registry.test",scope="repository:library/nginx:pull"`) + http.Error(w, "authentication required", http.StatusUnauthorized) + return + } + + switch r.URL.Path { + case "/v2/library/nginx/manifests/latest": + w.Header().Set("Content-Type", "application/vnd.oci.image.manifest.v1+json") + w.Header().Set("Docker-Content-Digest", digest) + _, _ = io.WriteString(w, manifest) + case "/v2/library/nginx/blobs/" + digest: + w.Header().Set("Content-Type", "application/octet-stream") + _, _ = io.WriteString(w, blob) + default: + http.NotFound(w, r) + } + })) + defer registryServer.Close() + + warmProxy, db, store, _ := setupTestProxy(t) + warmClient := &http.Client{Transport: upstreamhttp.NewTransport(http.DefaultTransport, nil)} + warmFetcher := fetch.NewFetcher( + fetch.WithHTTPClient(warmClient), + fetch.WithMaxRetries(0), + ) + t.Cleanup(func() { _ = warmFetcher.Close() }) + warmProxy.Fetcher = warmFetcher + warmProxy.HTTPClient = warmClient + warmProxy.MetadataTTL = time.Hour + warmHandler := (&ContainerHandler{ + proxy: warmProxy, + registryURL: registryServer.URL, + proxyURL: "http://localhost:8080", + }).Routes() + + for _, request := range []struct { + path string + body string + }{ + {path: "/library/nginx/manifests/latest", body: manifest}, + {path: "/library/nginx/blobs/" + digest, body: blob}, + } { + response := httptest.NewRecorder() + warmHandler.ServeHTTP(response, httptest.NewRequest(http.MethodGet, request.path, nil)) + if response.Code != http.StatusOK { + t.Fatalf("warming %s: status = %d, want %d; body: %s", request.path, response.Code, http.StatusOK, response.Body.String()) + } + if got := response.Body.String(); got != request.body { + t.Fatalf("warming %s: body = %q, want %q", request.path, got, request.body) + } + } + + warmRegistryRequests := registryRequests + warmTokenRequests := tokenRequests + registryAvailable = false + tokenAvailable = false + + offlineClient := &http.Client{Transport: upstreamhttp.NewTransport(http.DefaultTransport, nil)} + offlineFetcher := fetch.NewFetcher( + fetch.WithHTTPClient(offlineClient), + fetch.WithMaxRetries(0), + ) + t.Cleanup(func() { _ = offlineFetcher.Close() }) + offlineProxy := NewProxy(db, store, offlineFetcher, fetch.NewResolver(), warmProxy.Logger) + offlineProxy.HTTPClient = offlineClient + offlineProxy.MetadataTTL = time.Hour + offlineHandler := (&ContainerHandler{ + proxy: offlineProxy, + registryURL: registryServer.URL, + proxyURL: "http://localhost:8080", + }).Routes() + + for _, request := range []struct { + name string + path string + body string + }{ + {name: "tag manifest", path: "/library/nginx/manifests/latest", body: manifest}, + {name: "digest manifest", path: "/library/nginx/manifests/" + digest, body: manifest}, + {name: "blob", path: "/library/nginx/blobs/" + digest, body: blob}, + } { + t.Run(request.name, func(t *testing.T) { + response := httptest.NewRecorder() + offlineHandler.ServeHTTP(response, httptest.NewRequest(http.MethodGet, request.path, nil)) + if response.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body: %s", response.Code, http.StatusOK, response.Body.String()) + } + if got := response.Body.String(); got != request.body { + t.Errorf("body = %q, want %q", got, request.body) + } + if got := response.Header().Get("Docker-Content-Digest"); got != digest { + t.Errorf("Docker-Content-Digest = %q, want %q", got, digest) + } + }) + } + + if registryRequests != warmRegistryRequests { + t.Errorf("offline registry requests = %d, want 0", registryRequests-warmRegistryRequests) + } + if tokenRequests != warmTokenRequests { + t.Errorf("offline token requests = %d, want 0", tokenRequests-warmTokenRequests) + } +} + +func TestContainerHandler_BlobDownload_CacheHitSkipsAuth(t *testing.T) { + proxy, db, store, fetcher := setupTestProxy(t) + digest := "sha256:abc123def456abc123def456abc123def456abc123def456abc123def456abcd" + seedPackage(t, db, store, "oci", "library/nginx", digest, digest, "cached blob") + + upstreamRequests := 0 + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + upstreamRequests++ + http.Error(w, "upstream unavailable", http.StatusServiceUnavailable) + })) + defer upstream.Close() + + h := &ContainerHandler{ + proxy: proxy, + registryURL: upstream.URL, + proxyURL: "http://localhost:8080", + } + + req := httptest.NewRequest(http.MethodGet, "/library/nginx/blobs/"+digest, nil) w := httptest.NewRecorder() - handler.ServeHTTP(w, req) + h.Routes().ServeHTTP(w, req) if w.Code != http.StatusOK { - t.Errorf("got status %d, want %d; body: %s", w.Code, http.StatusOK, w.Body.String()) + t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusOK, w.Body.String()) } - - // Verify auth header was passed to the fetcher - if capturedHeaders == nil { - t.Fatal("expected headers to be passed to fetcher, got nil") + if got := w.Body.String(); got != "cached blob" { + t.Errorf("body = %q, want %q", got, "cached blob") } - auth := capturedHeaders.Get("Authorization") - if auth != "Bearer test-token-123" { - t.Errorf("Authorization = %q, want %q", auth, "Bearer test-token-123") + if upstreamRequests != 0 { + t.Errorf("upstream requests = %d, want 0", upstreamRequests) } - - // Verify response headers - if got := w.Header().Get("Docker-Content-Digest"); got != "sha256:abc123def456abc123def456abc123def456abc123def456abc123def456abcd" { - t.Errorf("Docker-Content-Digest = %q, want digest", got) + if fetcher.fetchCalled { + t.Error("fetcher should not be called on cache hit") } } -// mockFetcherWithHeaders captures headers passed to FetchWithHeaders. -type mockFetcherWithHeaders struct { - fetchFn func(ctx context.Context, url string, headers http.Header) (*fetch.Artifact, error) +func TestContainerHandler_BlobHead_CacheHitSkipsUpstreamAndAuth(t *testing.T) { + proxy, db, store, fetcher := setupTestProxy(t) + digest := "sha256:abc123def456abc123def456abc123def456abc123def456abc123def456abcd" + seedPackage(t, db, store, "oci", "library/nginx", digest, digest, "cached blob") + + upstreamRequests := 0 + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + upstreamRequests++ + http.Error(w, "upstream unavailable", http.StatusServiceUnavailable) + })) + defer upstream.Close() + proxy.HTTPClient = upstream.Client() + + h := &ContainerHandler{ + proxy: proxy, + registryURL: upstream.URL, + proxyURL: "http://localhost:8080", + } + + req := httptest.NewRequest(http.MethodHead, "/library/nginx/blobs/"+digest, nil) + w := httptest.NewRecorder() + h.Routes().ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusOK, w.Body.String()) + } + if got := w.Header().Get("Docker-Content-Digest"); got != digest { + t.Errorf("Docker-Content-Digest = %q, want %q", got, digest) + } + if got := w.Header().Get("Content-Length"); got != "11" { + t.Errorf("Content-Length = %q, want %q", got, "11") + } + if w.Body.Len() != 0 { + t.Errorf("HEAD response body length = %d, want 0", w.Body.Len()) + } + if upstreamRequests != 0 { + t.Errorf("upstream requests = %d, want 0", upstreamRequests) + } + if fetcher.fetchCalled { + t.Error("fetcher should not be called on cache hit") + } } -func (f *mockFetcherWithHeaders) Fetch(ctx context.Context, url string) (*fetch.Artifact, error) { - return f.FetchWithHeaders(ctx, url, nil) +func TestContainerHandler_BlobHead_DirectServeRedirects(t *testing.T) { + proxy, db, store, fetcher := setupTestProxy(t) + digest := "sha256:abc123def456abc123def456abc123def456abc123def456abc123def456abcd" + seedPackage(t, db, store, "oci", "library/nginx", digest, digest, "cached blob") + store.signedURL = "https://storage.example.test/cached-blob?signature=test" + proxy.DirectServe = true + + h := &ContainerHandler{ + proxy: proxy, + registryURL: "https://registry.example.test", + proxyURL: "http://localhost:8080", + } + + req := httptest.NewRequest(http.MethodHead, "/library/nginx/blobs/"+digest, nil) + w := httptest.NewRecorder() + h.Routes().ServeHTTP(w, req) + + if w.Code != http.StatusFound { + t.Fatalf("status = %d, want %d", w.Code, http.StatusFound) + } + if got := w.Header().Get("Location"); got != store.signedURL { + t.Errorf("Location = %q, want %q", got, store.signedURL) + } + if got := w.Header().Get("ETag"); got != `"abc123"` { + t.Errorf("ETag = %q, want %q", got, `"abc123"`) + } + if w.Body.Len() != 0 { + t.Errorf("HEAD response body length = %d, want 0", w.Body.Len()) + } + if fetcher.fetchCalled { + t.Error("fetcher should not be called on cache hit") + } } -func (f *mockFetcherWithHeaders) FetchWithHeaders(ctx context.Context, url string, headers http.Header) (*fetch.Artifact, error) { - return f.fetchFn(ctx, url, headers) +func TestContainerHandler_ManifestByDigest_CacheHitSkipsUpstream(t *testing.T) { + digest := "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + manifest := `{"schemaVersion":2,"mediaType":"application/vnd.oci.image.manifest.v1+json"}` + upstreamAvailable := true + upstreamRequests := 0 + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + upstreamRequests++ + if !upstreamAvailable { + http.Error(w, "upstream unavailable", http.StatusServiceUnavailable) + return + } + if r.URL.Path != "/v2/library/nginx/manifests/"+digest { + http.NotFound(w, r) + return + } + w.Header().Set("Content-Type", "application/vnd.oci.image.manifest.v1+json") + w.Header().Set("Docker-Content-Digest", digest) + w.Header().Set("ETag", `"manifest-etag"`) + if r.Method != http.MethodHead { + _, _ = io.WriteString(w, manifest) + } + })) + defer upstream.Close() + + proxy, _, _, _ := setupTestProxy(t) + proxy.HTTPClient = upstream.Client() + h := &ContainerHandler{proxy: proxy, registryURL: upstream.URL, proxyURL: "http://localhost:8080"} + + first := httptest.NewRecorder() + h.Routes().ServeHTTP(first, httptest.NewRequest(http.MethodGet, "/library/nginx/manifests/"+digest, nil)) + if first.Code != http.StatusOK { + t.Fatalf("initial status = %d, want %d; body: %s", first.Code, http.StatusOK, first.Body.String()) + } + if first.Body.String() != manifest { + t.Fatalf("initial body = %q, want %q", first.Body.String(), manifest) + } + + upstreamAvailable = false + second := httptest.NewRecorder() + h.Routes().ServeHTTP(second, httptest.NewRequest(http.MethodGet, "/library/nginx/manifests/"+digest, nil)) + if second.Code != http.StatusOK { + t.Fatalf("cached status = %d, want %d; body: %s", second.Code, http.StatusOK, second.Body.String()) + } + if second.Body.String() != manifest { + t.Errorf("cached body = %q, want %q", second.Body.String(), manifest) + } + if got := second.Header().Get("Docker-Content-Digest"); got != digest { + t.Errorf("cached Docker-Content-Digest = %q, want %q", got, digest) + } + + head := httptest.NewRecorder() + h.Routes().ServeHTTP(head, httptest.NewRequest(http.MethodHead, "/library/nginx/manifests/"+digest, nil)) + if head.Code != http.StatusOK { + t.Fatalf("cached HEAD status = %d, want %d", head.Code, http.StatusOK) + } + wantLength := strconv.Itoa(len(manifest)) + if got := head.Header().Get("Content-Length"); got != wantLength { + t.Errorf("cached HEAD Content-Length = %q, want %q", got, wantLength) + } + if head.Body.Len() != 0 { + t.Errorf("cached HEAD body length = %d, want 0", head.Body.Len()) + } + if upstreamRequests != 1 { + t.Errorf("upstream requests = %d, want 1", upstreamRequests) + } } -func (f *mockFetcherWithHeaders) Head(_ context.Context, _ string) (int64, string, error) { - return 0, "", nil +func TestContainerHandler_ManifestByTag_UsesStaleCacheOnUpstreamFailure(t *testing.T) { + digest := "sha256:bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + manifest := `{"schemaVersion":2,"mediaType":"application/vnd.oci.image.index.v1+json"}` + upstreamAvailable := true + upstreamRequests := 0 + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + upstreamRequests++ + if !upstreamAvailable { + http.Error(w, "upstream unavailable", http.StatusServiceUnavailable) + return + } + w.Header().Set("Content-Type", "application/vnd.oci.image.index.v1+json") + w.Header().Set("Docker-Content-Digest", digest) + _, _ = io.WriteString(w, manifest) + })) + defer upstream.Close() + + proxy, _, _, _ := setupTestProxy(t) + proxy.HTTPClient = upstream.Client() + proxy.MetadataTTL = 0 + h := &ContainerHandler{proxy: proxy, registryURL: upstream.URL, proxyURL: "http://localhost:8080"} + + first := httptest.NewRecorder() + h.Routes().ServeHTTP(first, httptest.NewRequest(http.MethodGet, "/library/nginx/manifests/latest", nil)) + if first.Code != http.StatusOK { + t.Fatalf("initial status = %d, want %d; body: %s", first.Code, http.StatusOK, first.Body.String()) + } + + upstreamAvailable = false + second := httptest.NewRecorder() + h.Routes().ServeHTTP(second, httptest.NewRequest(http.MethodGet, "/library/nginx/manifests/latest", nil)) + if second.Code != http.StatusOK { + t.Fatalf("stale status = %d, want %d; body: %s", second.Code, http.StatusOK, second.Body.String()) + } + if second.Body.String() != manifest { + t.Errorf("stale body = %q, want %q", second.Body.String(), manifest) + } + if got := second.Header().Get("Warning"); got != `110 - "Response is Stale"` { + t.Errorf("Warning = %q, want stale warning", got) + } + if got := second.Header().Get("Docker-Content-Digest"); got != digest { + t.Errorf("stale Docker-Content-Digest = %q, want %q", got, digest) + } + if upstreamRequests != 2 { + t.Errorf("upstream requests = %d, want 2", upstreamRequests) + } +} + +func TestContainerHandler_ManifestByTag_CachesDigestAlias(t *testing.T) { + digest := "sha256:cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc" + manifest := `{"schemaVersion":2,"mediaType":"application/vnd.oci.image.manifest.v1+json"}` + upstreamAvailable := true + upstreamRequests := 0 + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + upstreamRequests++ + if !upstreamAvailable { + http.Error(w, "upstream unavailable", http.StatusServiceUnavailable) + return + } + if r.URL.Path != "/v2/library/nginx/manifests/latest" { + http.NotFound(w, r) + return + } + w.Header().Set("Content-Type", "application/vnd.oci.image.manifest.v1+json") + w.Header().Set("Docker-Content-Digest", digest) + _, _ = io.WriteString(w, manifest) + })) + defer upstream.Close() + + proxy, _, _, _ := setupTestProxy(t) + proxy.HTTPClient = upstream.Client() + h := &ContainerHandler{proxy: proxy, registryURL: upstream.URL, proxyURL: "http://localhost:8080"} + + first := httptest.NewRecorder() + h.Routes().ServeHTTP(first, httptest.NewRequest(http.MethodGet, "/library/nginx/manifests/latest", nil)) + if first.Code != http.StatusOK { + t.Fatalf("tag status = %d, want %d; body: %s", first.Code, http.StatusOK, first.Body.String()) + } + + upstreamAvailable = false + byDigest := httptest.NewRecorder() + h.Routes().ServeHTTP(byDigest, httptest.NewRequest(http.MethodGet, "/library/nginx/manifests/"+digest, nil)) + if byDigest.Code != http.StatusOK { + t.Fatalf("digest status = %d, want %d; body: %s", byDigest.Code, http.StatusOK, byDigest.Body.String()) + } + if byDigest.Body.String() != manifest { + t.Errorf("digest body = %q, want %q", byDigest.Body.String(), manifest) + } + if got := byDigest.Header().Get("Docker-Content-Digest"); got != digest { + t.Errorf("Docker-Content-Digest = %q, want %q", got, digest) + } + if upstreamRequests != 1 { + t.Errorf("upstream requests = %d, want 1", upstreamRequests) + } +} + +func TestContainerHandler_ManifestByTag_StaleHeadChecksUpstream(t *testing.T) { + oldDigest := "sha256:dddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddddd" + newDigest := "sha256:eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee" + currentDigest := oldDigest + upstreamRequests := 0 + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + upstreamRequests++ + w.Header().Set("Content-Type", "application/vnd.oci.image.manifest.v1+json") + w.Header().Set("Docker-Content-Digest", currentDigest) + w.Header().Set("ETag", `"`+currentDigest+`"`) + if r.Method != http.MethodHead { + _, _ = io.WriteString(w, `{"schemaVersion":2}`) + } + })) + defer upstream.Close() + + proxy, _, _, _ := setupTestProxy(t) + proxy.HTTPClient = upstream.Client() + proxy.MetadataTTL = 0 + h := &ContainerHandler{proxy: proxy, registryURL: upstream.URL, proxyURL: "http://localhost:8080"} + + first := httptest.NewRecorder() + h.Routes().ServeHTTP(first, httptest.NewRequest(http.MethodGet, "/library/nginx/manifests/latest", nil)) + if first.Code != http.StatusOK { + t.Fatalf("initial status = %d, want %d", first.Code, http.StatusOK) + } + + currentDigest = newDigest + head := httptest.NewRecorder() + h.Routes().ServeHTTP(head, httptest.NewRequest(http.MethodHead, "/library/nginx/manifests/latest", nil)) + if head.Code != http.StatusOK { + t.Fatalf("HEAD status = %d, want %d", head.Code, http.StatusOK) + } + if got := head.Header().Get("Docker-Content-Digest"); got != newDigest { + t.Errorf("Docker-Content-Digest = %q, want %q", got, newDigest) + } + if upstreamRequests != 2 { + t.Errorf("upstream requests = %d, want 2", upstreamRequests) + } } func TestContainerHandler_Routes_VersionCheck(t *testing.T) { diff --git a/internal/handler/handler.go b/internal/handler/handler.go index fc78dbf..72b9c28 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -136,18 +136,25 @@ type CacheResult struct { // GetOrFetchArtifact retrieves an artifact from cache or fetches from upstream. func (p *Proxy) GetOrFetchArtifact(ctx context.Context, ecosystem, name, version, filename string) (*CacheResult, error) { - pkgPURL := purl.MakePURLString(ecosystem, name, "") - versionPURL := purl.MakePURLString(ecosystem, name, version) - - if cached, err := p.checkCache(ctx, pkgPURL, versionPURL, filename); err != nil { + if cached, err := p.GetCachedArtifact(ctx, ecosystem, name, version, filename); err != nil { return nil, err } else if cached != nil { return cached, nil } + pkgPURL := purl.MakePURLString(ecosystem, name, "") + versionPURL := purl.MakePURLString(ecosystem, name, version) return p.fetchAndCache(ctx, ecosystem, name, version, filename, pkgPURL, versionPURL) } +// GetCachedArtifact retrieves an artifact from cache without contacting an upstream. +// It returns nil when no usable cache entry exists. +func (p *Proxy) GetCachedArtifact(ctx context.Context, ecosystem, name, version, filename string) (*CacheResult, error) { + pkgPURL := purl.MakePURLString(ecosystem, name, "") + versionPURL := purl.MakePURLString(ecosystem, name, version) + return p.checkCache(ctx, pkgPURL, versionPURL, filename) +} + // checkCache looks up an artifact in the cache. Returns nil if not cached. func (p *Proxy) checkCache(ctx context.Context, pkgPURL, versionPURL, filename string) (*CacheResult, error) { pkg, err := p.DB.GetPackageByPURL(pkgPURL) @@ -363,6 +370,10 @@ func (p *Proxy) updateCacheDB(ecosystem, name, filename, pkgPURL, versionPURL, u // ServeArtifact writes a CacheResult to an HTTP response. func ServeArtifact(w http.ResponseWriter, result *CacheResult) { + serveArtifact(w, http.MethodGet, result) +} + +func serveArtifact(w http.ResponseWriter, method string, result *CacheResult) { if result.RedirectURL != "" { if result.Hash != "" { w.Header().Set("ETag", fmt.Sprintf(`"%s"`, result.Hash)) @@ -372,12 +383,14 @@ func ServeArtifact(w http.ResponseWriter, result *CacheResult) { return } - defer func() { _ = result.Reader.Close() }() + if result.Reader != nil { + defer func() { _ = result.Reader.Close() }() + } if result.ContentType != "" { w.Header().Set("Content-Type", result.ContentType) } - if result.Size > 0 { + if result.Size > 0 || (method == http.MethodHead && result.Size == 0) { w.Header().Set("Content-Length", fmt.Sprintf("%d", result.Size)) } if result.Hash != "" { @@ -385,7 +398,9 @@ func ServeArtifact(w http.ResponseWriter, result *CacheResult) { } w.WriteHeader(http.StatusOK) - _, _ = io.Copy(w, result.Reader) + if method != http.MethodHead && result.Reader != nil { + _, _ = io.Copy(w, result.Reader) + } } // ProxyUpstream forwards a request to an upstream URL without caching. @@ -807,18 +822,16 @@ func (p *Proxy) GetOrFetchArtifactFromURL(ctx context.Context, ecosystem, name, } // GetOrFetchArtifactFromURLWithHeaders retrieves an artifact from cache or fetches from a URL -// with additional HTTP headers. This is needed for registries that require authentication -// (e.g. Docker Hub requires a Bearer token even for public images). +// with additional request-specific HTTP headers. func (p *Proxy) GetOrFetchArtifactFromURLWithHeaders(ctx context.Context, ecosystem, name, version, filename, downloadURL string, headers http.Header) (*CacheResult, error) { - pkgPURL := purl.MakePURLString(ecosystem, name, "") - versionPURL := purl.MakePURLString(ecosystem, name, version) - - if cached, err := p.checkCache(ctx, pkgPURL, versionPURL, filename); err != nil { + if cached, err := p.GetCachedArtifact(ctx, ecosystem, name, version, filename); err != nil { return nil, err } else if cached != nil { return cached, nil } + pkgPURL := purl.MakePURLString(ecosystem, name, "") + versionPURL := purl.MakePURLString(ecosystem, name, version) return p.fetchAndCacheFromURL(ctx, ecosystem, name, version, filename, pkgPURL, versionPURL, downloadURL, headers) } diff --git a/internal/handler/handler_test.go b/internal/handler/handler_test.go index 9a2b329..a3bd9b3 100644 --- a/internal/handler/handler_test.go +++ b/internal/handler/handler_test.go @@ -5,7 +5,6 @@ import ( "context" "database/sql" "errors" - "fmt" "io" "log/slog" "net/http" @@ -17,6 +16,7 @@ import ( "github.com/git-pkgs/proxy/internal/config" "github.com/git-pkgs/proxy/internal/database" "github.com/git-pkgs/proxy/internal/storage" + "github.com/git-pkgs/purl" "github.com/git-pkgs/registries/fetch" ) @@ -152,7 +152,7 @@ func seedPackage(t *testing.T, db *database.DB, store *mockStorage, ecosystem, n t.Helper() pkg := &database.Package{ - PURL: fmt.Sprintf("pkg:%s/%s", ecosystem, name), + PURL: purl.MakePURLString(ecosystem, name, ""), Ecosystem: ecosystem, Name: name, } @@ -160,7 +160,7 @@ func seedPackage(t *testing.T, db *database.DB, store *mockStorage, ecosystem, n t.Fatalf("failed to upsert package: %v", err) } - versionPURL := fmt.Sprintf("pkg:%s/%s@%s", ecosystem, name, version) + versionPURL := purl.MakePURLString(ecosystem, name, version) ver := &database.Version{ PURL: versionPURL, PackagePURL: pkg.PURL, diff --git a/internal/handler/notfound_ecosystems_test.go b/internal/handler/notfound_ecosystems_test.go index 6f04113..44c4486 100644 --- a/internal/handler/notfound_ecosystems_test.go +++ b/internal/handler/notfound_ecosystems_test.go @@ -1,7 +1,6 @@ package handler import ( - "context" "net/http" "net/http/httptest" "strings" @@ -117,23 +116,12 @@ func TestComposerDownloadUpstreamNotFoundReturns404(t *testing.T) { } func TestContainerBlobUpstreamNotFoundReturns404(t *testing.T) { - authServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte(`{"token": "test-token-123"}`)) - })) - defer authServer.Close() - - proxy, _, _, _ := setupTestProxy(t) - proxy.Fetcher = &mockFetcherWithHeaders{ - fetchFn: func(_ context.Context, _ string, _ http.Header) (*fetch.Artifact, error) { - return nil, fetch.ErrNotFound - }, - } + proxy, _, _, fetcher := setupTestProxy(t) + fetcher.fetchErr = fetch.ErrNotFound h := &ContainerHandler{ proxy: proxy, registryURL: "https://registry-1.docker.io", - authURL: authServer.URL, proxyURL: "http://localhost:8080", } From 6fcc57c994f5d3b3ca51ac51a3a56bd045c07e2a Mon Sep 17 00:00:00 2001 From: Andrew Nesbitt Date: Thu, 13 Aug 2026 08:06:41 +0100 Subject: [PATCH 07/24] Optimize cached artifact serving (#245) --- internal/database/database_test.go | 80 +++++++ internal/database/queries.go | 22 ++ internal/database/types.go | 10 + internal/handler/handler.go | 65 +++--- internal/handler/handler_bench_test.go | 300 +++++++++++++++++++++++++ internal/handler/handler_test.go | 4 +- 6 files changed, 444 insertions(+), 37 deletions(-) create mode 100644 internal/handler/handler_bench_test.go diff --git a/internal/database/database_test.go b/internal/database/database_test.go index 6fca4ea..3e92b91 100644 --- a/internal/database/database_test.go +++ b/internal/database/database_test.go @@ -239,6 +239,86 @@ func TestArtifactCRUD(t *testing.T) { }) } +func TestGetCachedArtifact(t *testing.T) { + runWithBothDatabases(t, func(t *testing.T, db *DB) { + const ( + packagePURL = "pkg:npm/lodash" + versionPURL = "pkg:npm/lodash@4.17.21" + filename = "lodash-4.17.21.tgz" + ) + seedCachedArtifactTestData(t, db, packagePURL, versionPURL, filename) + + cached, err := db.GetCachedArtifact(packagePURL, versionPURL, filename) + if err != nil { + t.Fatalf("GetCachedArtifact before cache failed: %v", err) + } + if cached != nil { + t.Fatalf("expected no cached artifact, got %+v", cached) + } + + if err := db.MarkArtifactCached(versionPURL, filename, "/cache/npm/"+filename, + "sha256-abc", 12345, "application/gzip"); err != nil { + t.Fatalf("MarkArtifactCached failed: %v", err) + } + + cached, err = db.GetCachedArtifact(packagePURL, versionPURL, filename) + if err != nil { + t.Fatalf("GetCachedArtifact failed: %v", err) + } + if cached == nil { + t.Fatal("expected cached artifact, got nil") + } + if cached.Ecosystem != "npm" { + t.Errorf("expected npm ecosystem, got %q", cached.Ecosystem) + } + if cached.StoragePath != "/cache/npm/"+filename { + t.Errorf("expected cached storage path, got %q", cached.StoragePath) + } + if cached.ContentHash.String != "sha256-abc" { + t.Errorf("expected cached content hash, got %q", cached.ContentHash.String) + } + if cached.Size.Int64 != 12345 { + t.Errorf("expected cached size 12345, got %d", cached.Size.Int64) + } + if cached.ContentType.String != "application/gzip" { + t.Errorf("expected cached content type, got %q", cached.ContentType.String) + } + if cached.Integrity.String != "sha512-abc123" { + t.Errorf("expected cached integrity, got %q", cached.Integrity.String) + } + + cached, err = db.GetCachedArtifact("pkg:npm/other", versionPURL, filename) + if err != nil { + t.Fatalf("GetCachedArtifact with wrong package failed: %v", err) + } + if cached != nil { + t.Fatalf("expected package mismatch to miss cache, got %+v", cached) + } + }) +} + +func seedCachedArtifactTestData(t *testing.T, db *DB, packagePURL, versionPURL, filename string) { + t.Helper() + + if err := db.UpsertPackage(&Package{PURL: packagePURL, Ecosystem: "npm", Name: "lodash"}); err != nil { + t.Fatalf("UpsertPackage failed: %v", err) + } + if err := db.UpsertVersion(&Version{ + PURL: versionPURL, + PackagePURL: packagePURL, + Integrity: sql.NullString{String: "sha512-abc123", Valid: true}, + }); err != nil { + t.Fatalf("UpsertVersion failed: %v", err) + } + if err := db.UpsertArtifact(&Artifact{ + VersionPURL: versionPURL, + Filename: filename, + UpstreamURL: "https://registry.npmjs.org/lodash/-/" + filename, + }); err != nil { + t.Fatalf("UpsertArtifact failed: %v", err) + } +} + func TestCacheManagement(t *testing.T) { runWithBothDatabases(t, func(t *testing.T, db *DB) { pkg := &Package{ diff --git a/internal/database/queries.go b/internal/database/queries.go index f8b76fe..b01abe4 100644 --- a/internal/database/queries.go +++ b/internal/database/queries.go @@ -191,6 +191,28 @@ func (db *DB) GetArtifact(versionPURL, filename string) (*Artifact, error) { return &a, nil } +// GetCachedArtifact returns the fields needed to serve a cached artifact. +func (db *DB) GetCachedArtifact(packagePURL, versionPURL, filename string) (*CachedArtifact, error) { + var artifact CachedArtifact + query := db.Rebind(` + SELECT packages.ecosystem, artifacts.storage_path, artifacts.content_hash, artifacts.size, + artifacts.content_type, versions.integrity + FROM artifacts + JOIN versions ON versions.purl = artifacts.version_purl + JOIN packages ON packages.purl = versions.package_purl + WHERE packages.purl = ? AND artifacts.version_purl = ? AND artifacts.filename = ? + AND artifacts.storage_path IS NOT NULL AND artifacts.fetched_at IS NOT NULL + `) + err := db.Get(&artifact, query, packagePURL, versionPURL, filename) + if err == sql.ErrNoRows { + return nil, nil + } + if err != nil { + return nil, err + } + return &artifact, nil +} + func (db *DB) GetArtifactByPath(storagePath string) (*Artifact, error) { var a Artifact query := db.Rebind(` diff --git a/internal/database/types.go b/internal/database/types.go index 7128f12..3826c7a 100644 --- a/internal/database/types.go +++ b/internal/database/types.go @@ -76,6 +76,16 @@ func (a *Artifact) IsCached() bool { return a.StoragePath.Valid && a.FetchedAt.Valid } +// CachedArtifact contains the fields needed to serve a cached artifact. +type CachedArtifact struct { + Ecosystem string `db:"ecosystem"` + StoragePath string `db:"storage_path"` + ContentHash sql.NullString `db:"content_hash"` + Size sql.NullInt64 `db:"size"` + ContentType sql.NullString `db:"content_type"` + Integrity sql.NullString `db:"integrity"` +} + // MetadataCacheEntry represents a cached metadata blob for offline serving. type MetadataCacheEntry struct { ID int64 `db:"id" json:"id"` diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 72b9c28..6a7aab4 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -13,6 +13,7 @@ import ( "net/url" "strconv" "strings" + "sync" "time" "github.com/git-pkgs/cooldown" @@ -48,6 +49,15 @@ func hasDotDotSegment(path string) bool { const defaultHTTPTimeout = 30 * time.Second +const artifactCopyBufferSize = 32 << 10 + +var artifactCopyBufferPool = sync.Pool{ //nolint:gochecknoglobals // shared across artifact responses + New: func() any { + buffer := make([]byte, artifactCopyBufferSize) + return &buffer + }, +} + // canonicalPackagePURL returns a versionless PURL in canonical form so cooldown // lookups match keys produced by config.CooldownConfig.NormalizedPackages. func canonicalPackagePURL(ecosystem, name string) string { @@ -157,27 +167,11 @@ func (p *Proxy) GetCachedArtifact(ctx context.Context, ecosystem, name, version, // checkCache looks up an artifact in the cache. Returns nil if not cached. func (p *Proxy) checkCache(ctx context.Context, pkgPURL, versionPURL, filename string) (*CacheResult, error) { - pkg, err := p.DB.GetPackageByPURL(pkgPURL) - if err != nil { - return nil, fmt.Errorf("checking package cache: %w", err) - } - if pkg == nil { - return nil, nil - } - - ver, err := p.DB.GetVersionByPURL(versionPURL) - if err != nil { - return nil, fmt.Errorf("checking version cache: %w", err) - } - if ver == nil { - return nil, nil - } - - artifact, err := p.DB.GetArtifact(versionPURL, filename) + artifact, err := p.DB.GetCachedArtifact(pkgPURL, versionPURL, filename) if err != nil { return nil, fmt.Errorf("checking artifact cache: %w", err) } - if artifact == nil || !artifact.IsCached() { + if artifact == nil { return nil, nil } @@ -189,39 +183,39 @@ func (p *Proxy) checkCache(ctx context.Context, pkgPURL, versionPURL, filename s } if p.DirectServe { - signed, err := p.Storage.SignedURL(ctx, artifact.StoragePath.String, p.DirectServeTTL) + signed, err := p.Storage.SignedURL(ctx, artifact.StoragePath, p.DirectServeTTL) if err == nil { result.RedirectURL = rewriteSignedURLHost(signed, p.DirectServeBaseURL) - p.recordCacheHit(pkgPURL, versionPURL, filename) + p.recordCacheHit(artifact.Ecosystem, versionPURL, filename) return result, nil } if !errors.Is(err, storage.ErrSignedURLUnsupported) { p.Logger.Warn("failed to sign storage URL, falling back to streaming", - "path", artifact.StoragePath.String, "error", err) + "path", artifact.StoragePath, "error", err) } } start := time.Now() - reader, err := p.Storage.Open(ctx, artifact.StoragePath.String) + reader, err := p.Storage.Open(ctx, artifact.StoragePath) metrics.RecordStorageOperation("read", time.Since(start)) if err != nil { metrics.RecordStorageError("read") p.Logger.Warn("cached artifact missing from storage, will refetch", - "path", artifact.StoragePath.String, "error", err) + "path", artifact.StoragePath, "error", err) return nil, nil } - result.Reader = newVerifyingReader(reader, artifact.ContentHash.String, ver.Integrity.String, + result.Reader = newVerifyingReader(reader, artifact.ContentHash.String, artifact.Integrity.String, func(reason string) { p.Logger.Error("cached artifact failed integrity check", "purl", versionPURL, "filename", filename, - "path", artifact.StoragePath.String, "reason", reason) - metrics.RecordIntegrityFailure(pkg.Ecosystem) + "path", artifact.StoragePath, "reason", reason) + metrics.RecordIntegrityFailure(artifact.Ecosystem) if err := p.DB.ClearArtifactCache(versionPURL, filename); err != nil { p.Logger.Warn("failed to clear corrupt artifact from cache", "error", err) } }) - p.recordCacheHit(pkgPURL, versionPURL, filename) + p.recordCacheHit(artifact.Ecosystem, versionPURL, filename) return result, nil } @@ -245,11 +239,9 @@ func rewriteSignedURLHost(signed, baseURL string) string { return s.String() } -func (p *Proxy) recordCacheHit(pkgPURL, versionPURL, filename string) { +func (p *Proxy) recordCacheHit(ecosystem, versionPURL, filename string) { _ = p.DB.RecordArtifactHit(versionPURL, filename) - if parsed, err := purl.Parse(pkgPURL); err == nil { - metrics.RecordCacheHit(purl.PURLTypeToEcosystem(parsed.Type)) - } + metrics.RecordCacheHit(purl.NormalizeEcosystem(ecosystem)) } func (p *Proxy) fetchAndCache(ctx context.Context, ecosystem, name, version, filename, pkgPURL, versionPURL string) (*CacheResult, error) { @@ -376,7 +368,7 @@ func ServeArtifact(w http.ResponseWriter, result *CacheResult) { func serveArtifact(w http.ResponseWriter, method string, result *CacheResult) { if result.RedirectURL != "" { if result.Hash != "" { - w.Header().Set("ETag", fmt.Sprintf(`"%s"`, result.Hash)) + w.Header().Set("ETag", `"`+result.Hash+`"`) } w.Header().Set("Location", result.RedirectURL) w.WriteHeader(http.StatusFound) @@ -391,15 +383,18 @@ func serveArtifact(w http.ResponseWriter, method string, result *CacheResult) { w.Header().Set("Content-Type", result.ContentType) } if result.Size > 0 || (method == http.MethodHead && result.Size == 0) { - w.Header().Set("Content-Length", fmt.Sprintf("%d", result.Size)) + w.Header().Set("Content-Length", strconv.FormatInt(result.Size, 10)) } if result.Hash != "" { - w.Header().Set("ETag", fmt.Sprintf(`"%s"`, result.Hash)) + w.Header().Set("ETag", `"`+result.Hash+`"`) } w.WriteHeader(http.StatusOK) if method != http.MethodHead && result.Reader != nil { - _, _ = io.Copy(w, result.Reader) + buffer := artifactCopyBufferPool.Get().(*[]byte) + defer artifactCopyBufferPool.Put(buffer) + // Hide optional ReaderFrom methods so io.CopyBuffer uses the pooled buffer. + _, _ = io.CopyBuffer(struct{ io.Writer }{w}, result.Reader, *buffer) } } diff --git a/internal/handler/handler_bench_test.go b/internal/handler/handler_bench_test.go new file mode 100644 index 0000000..cdbb524 --- /dev/null +++ b/internal/handler/handler_bench_test.go @@ -0,0 +1,300 @@ +package handler + +import ( + "bytes" + "context" + "crypto/sha256" + "database/sql" + "encoding/hex" + "fmt" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/git-pkgs/proxy/internal/database" + "github.com/git-pkgs/proxy/internal/storage" + "github.com/git-pkgs/purl" + "github.com/git-pkgs/registries/fetch" +) + +const benchmarkArtifactSize = 64 << 10 + +const benchmarkMetadataSize = 1 << 20 + +type benchmarkResponseWriter struct { + header http.Header +} + +func (w *benchmarkResponseWriter) Header() http.Header { + return w.header +} + +func (w *benchmarkResponseWriter) Write(p []byte) (int, error) { + return len(p), nil +} + +func (w *benchmarkResponseWriter) WriteHeader(_ int) {} + +func benchmarkCachedProxy(b *testing.B) (*Proxy, *mockStorage) { + b.Helper() + + proxy, db, store, _ := setupTestProxy(b) + content := strings.Repeat("x", benchmarkArtifactSize) + seedPackage(b, db, store, "npm", "lodash", "4.17.21", "lodash-4.17.21.tgz", content) + + artifact, err := db.GetArtifact("pkg:npm/lodash@4.17.21", "lodash-4.17.21.tgz") + if err != nil { + b.Fatalf("get seeded artifact: %v", err) + } + sum := sha256.Sum256([]byte(content)) + artifact.ContentHash.String = hex.EncodeToString(sum[:]) + if err := db.UpsertArtifact(artifact); err != nil { + b.Fatalf("update seeded artifact hash: %v", err) + } + + return proxy, store +} + +func BenchmarkArtifactCacheHit(b *testing.B) { + ctx := context.Background() + + b.Run("stream-64KiB", func(b *testing.B) { + proxy, _ := benchmarkCachedProxy(b) + w := &benchmarkResponseWriter{header: make(http.Header)} + b.SetBytes(benchmarkArtifactSize) + b.ReportAllocs() + b.ResetTimer() + + for b.Loop() { + result, err := proxy.GetOrFetchArtifact(ctx, "npm", "lodash", "4.17.21", "lodash-4.17.21.tgz") + if err != nil { + b.Fatal(err) + } + ServeArtifact(w, result) + } + }) + + b.Run("direct-serve", func(b *testing.B) { + proxy, store := benchmarkCachedProxy(b) + proxy.DirectServe = true + store.signedURL = "https://storage.example/npm/lodash-4.17.21.tgz?signature=abc" + w := &benchmarkResponseWriter{header: make(http.Header)} + b.ReportAllocs() + b.ResetTimer() + + for b.Loop() { + result, err := proxy.GetOrFetchArtifact(ctx, "npm", "lodash", "4.17.21", "lodash-4.17.21.tgz") + if err != nil { + b.Fatal(err) + } + ServeArtifact(w, result) + } + }) +} + +func BenchmarkArtifactCacheHitParallel(b *testing.B) { + proxy, _ := benchmarkCachedProxy(b) + ctx := context.Background() + b.SetBytes(benchmarkArtifactSize) + b.ReportAllocs() + b.ResetTimer() + + b.RunParallel(func(pb *testing.PB) { + w := &benchmarkResponseWriter{header: make(http.Header)} + for pb.Next() { + result, err := proxy.GetOrFetchArtifact(ctx, "npm", "lodash", "4.17.21", "lodash-4.17.21.tgz") + if err != nil { + b.Error(err) + return + } + ServeArtifact(w, result) + } + }) +} + +func BenchmarkReadMetadata(b *testing.B) { + payload := bytes.Repeat([]byte("x"), benchmarkMetadataSize) + proxy := &Proxy{MetadataMaxSize: benchmarkMetadataSize} + b.SetBytes(benchmarkMetadataSize) + b.ReportAllocs() + + var data []byte + for b.Loop() { + var err error + data, err = proxy.ReadMetadata(bytes.NewReader(payload)) + if err != nil { + b.Fatal(err) + } + } + if len(data) != len(payload) { + b.Fatalf("metadata size = %d, want %d", len(data), len(payload)) + } +} + +func BenchmarkArtifactPURLConstruction(b *testing.B) { + for _, tc := range []struct { + name string + ecosystem string + packageID string + }{ + {"npm", "npm", "lodash"}, + {"scoped-npm", "npm", "@scope/package"}, + {"go", "golang", "github.com/git-pkgs/proxy"}, + } { + b.Run(tc.name, func(b *testing.B) { + b.ReportAllocs() + var packagePURL, versionPURL string + for b.Loop() { + packagePURL = purl.MakePURLString(tc.ecosystem, tc.packageID, "") + versionPURL = purl.MakePURLString(tc.ecosystem, tc.packageID, "1.2.3") + } + if packagePURL == "" || versionPURL == "" { + b.Fatal("empty PURL") + } + }) + } +} + +type benchmarkNPMServer struct { + client *http.Client + requestURL string + db *database.DB + versionPURL string + filename string +} + +func newBenchmarkNPMServer(b *testing.B) *benchmarkNPMServer { + b.Helper() + + ctx := context.Background() + dir := b.TempDir() + db, err := database.Create(filepath.Join(dir, "benchmark.db")) + if err != nil { + b.Fatalf("create database: %v", err) + } + b.Cleanup(func() { _ = db.Close() }) + + store, err := storage.OpenBucket(ctx, "file://"+filepath.Join(dir, "cache")) + if err != nil { + b.Fatalf("open storage: %v", err) + } + b.Cleanup(func() { _ = store.Close() }) + + content := bytes.Repeat([]byte("x"), benchmarkArtifactSize) + storagePath := storage.ArtifactPath("npm", "", "lodash", "4.17.21", "lodash-4.17.21.tgz") + size, hash, err := store.Store(ctx, storagePath, bytes.NewReader(content)) + if err != nil { + b.Fatalf("store artifact: %v", err) + } + + pkg := &database.Package{PURL: "pkg:npm/lodash", Ecosystem: "npm", Name: "lodash"} + if err := db.UpsertPackage(pkg); err != nil { + b.Fatalf("seed package: %v", err) + } + version := &database.Version{PURL: "pkg:npm/lodash@4.17.21", PackagePURL: pkg.PURL} + if err := db.UpsertVersion(version); err != nil { + b.Fatalf("seed version: %v", err) + } + artifact := &database.Artifact{ + VersionPURL: version.PURL, + Filename: "lodash-4.17.21.tgz", + UpstreamURL: "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz", + StoragePath: sql.NullString{String: storagePath, Valid: true}, + ContentHash: sql.NullString{String: hash, Valid: true}, + Size: sql.NullInt64{Int64: size, Valid: true}, + ContentType: sql.NullString{String: "application/gzip", Valid: true}, + FetchedAt: sql.NullTime{Time: time.Now(), Valid: true}, + } + if err := db.UpsertArtifact(artifact); err != nil { + b.Fatalf("seed artifact: %v", err) + } + + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + proxy := NewProxy(db, store, &mockFetcher{}, fetch.NewResolver(), logger) + handler := NewNPMHandler(proxy, "http://proxy.example", "https://registry.npmjs.org") + server := httptest.NewServer(handler.Routes()) + b.Cleanup(server.Close) + client := server.Client() + return &benchmarkNPMServer{ + client: client, + requestURL: server.URL + "/lodash/-/lodash-4.17.21.tgz", + db: db, + versionPURL: version.PURL, + filename: artifact.Filename, + } +} + +func (s *benchmarkNPMServer) request() error { + resp, err := s.client.Get(s.requestURL) + if err != nil { + return fmt.Errorf("GET cached artifact: %w", err) + } + defer func() { _ = resp.Body.Close() }() + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("GET cached artifact status = %d, want %d", resp.StatusCode, http.StatusOK) + } + n, err := io.Copy(io.Discard, resp.Body) + if err != nil { + return fmt.Errorf("read cached artifact: %w", err) + } + if n != benchmarkArtifactSize { + return fmt.Errorf("cached artifact size = %d, want %d", n, benchmarkArtifactSize) + } + return nil +} + +func (s *benchmarkNPMServer) hitCount(b *testing.B) int64 { + b.Helper() + artifact, err := s.db.GetArtifact(s.versionPURL, s.filename) + if err != nil { + b.Fatalf("get artifact hit count: %v", err) + } + return artifact.HitCount +} + +func benchmarkNPMArtifactCacheHitHTTP(b *testing.B, parallel bool) { + server := newBenchmarkNPMServer(b) + if err := server.request(); err != nil { + b.Fatal(err) + } + startHits := server.hitCount(b) + + b.SetBytes(benchmarkArtifactSize) + b.ReportAllocs() + b.ResetTimer() + if parallel { + b.RunParallel(func(pb *testing.PB) { + for pb.Next() { + if err := server.request(); err != nil { + b.Error(err) + return + } + } + }) + } else { + for b.Loop() { + if err := server.request(); err != nil { + b.Fatal(err) + } + } + } + b.StopTimer() + + if hitCount := server.hitCount(b) - startHits; hitCount != int64(b.N) { + b.Fatalf("new artifact hits = %d, want %d", hitCount, b.N) + } + b.ReportMetric(float64(b.N)/b.Elapsed().Seconds(), "requests/s") +} + +func BenchmarkNPMArtifactCacheHitHTTP(b *testing.B) { + benchmarkNPMArtifactCacheHitHTTP(b, false) +} + +func BenchmarkNPMArtifactCacheHitHTTPParallel(b *testing.B) { + benchmarkNPMArtifactCacheHitHTTP(b, true) +} diff --git a/internal/handler/handler_test.go b/internal/handler/handler_test.go index a3bd9b3..d52e7b6 100644 --- a/internal/handler/handler_test.go +++ b/internal/handler/handler_test.go @@ -128,7 +128,7 @@ func (f *mockFetcher) Head(_ context.Context, _ string) (int64, string, error) { } // setupTestProxy creates a Proxy with a real DB (SQLite in temp dir) and mock storage/fetcher. -func setupTestProxy(t *testing.T) (*Proxy, *database.DB, *mockStorage, *mockFetcher) { +func setupTestProxy(t testing.TB) (*Proxy, *database.DB, *mockStorage, *mockFetcher) { t.Helper() dir := t.TempDir() @@ -148,7 +148,7 @@ func setupTestProxy(t *testing.T) (*Proxy, *database.DB, *mockStorage, *mockFetc } // seedPackage creates a package, version, and cached artifact in the test DB and storage. -func seedPackage(t *testing.T, db *database.DB, store *mockStorage, ecosystem, name, version, filename, content string) { +func seedPackage(t testing.TB, db *database.DB, store *mockStorage, ecosystem, name, version, filename, content string) { t.Helper() pkg := &database.Package{ From ed540053fadc13f1f6f520b5bca333b64d40ac9a Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 14 Aug 2026 09:15:33 +0100 Subject: [PATCH 08/24] Bump zizmorcore/zizmor-action from 0.6.1 to 0.6.2 (#251) Bumps [zizmorcore/zizmor-action](https://github.com/zizmorcore/zizmor-action) from 0.6.1 to 0.6.2. - [Release notes](https://github.com/zizmorcore/zizmor-action/releases) - [Commits](https://github.com/zizmorcore/zizmor-action/compare/6fc4b006235f201fdab3722e17240ab420d580e5...3dc1ecc9bcb9e94e9b2c709687979e1298497054) --- updated-dependencies: - dependency-name: zizmorcore/zizmor-action dependency-version: 0.6.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/zizmor.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/zizmor.yml b/.github/workflows/zizmor.yml index 4df0e18..ac587dd 100644 --- a/.github/workflows/zizmor.yml +++ b/.github/workflows/zizmor.yml @@ -26,4 +26,4 @@ jobs: persist-credentials: false - name: Run zizmor - uses: zizmorcore/zizmor-action@6fc4b006235f201fdab3722e17240ab420d580e5 # v0.6.1 + uses: zizmorcore/zizmor-action@3dc1ecc9bcb9e94e9b2c709687979e1298497054 # v0.6.2 From 849500de1ec35442ef675361e4fe3769c86c9d9b Mon Sep 17 00:00:00 2001 From: wickedOne Date: Fri, 14 Aug 2026 11:38:08 +0200 Subject: [PATCH 09/24] fix: decode PURL percent-encoding in versions and package paths (#244) * fix: decode PURL percent-encoding in versions and package paths * review fix --- internal/database/queries.go | 37 ++- internal/database/types.go | 76 +++++- internal/database/version_purl_test.go | 159 ++++++++++++ internal/handler/debian_test.go | 5 + internal/server/api.go | 10 +- internal/server/browse.go | 34 ++- internal/server/browse_test.go | 17 +- internal/server/resolve.go | 95 ++++++- internal/server/resolve_test.go | 115 +++++++-- internal/server/server.go | 27 +- internal/server/server_test.go | 235 ++++++++++++++++++ .../server/templates/pages/browse_source.html | 11 +- .../templates/pages/compare_versions.html | 8 +- .../server/templates/pages/package_show.html | 12 +- .../server/templates/pages/version_show.html | 6 +- 15 files changed, 745 insertions(+), 102 deletions(-) create mode 100644 internal/database/version_purl_test.go diff --git a/internal/database/queries.go b/internal/database/queries.go index b01abe4..9fa5381 100644 --- a/internal/database/queries.go +++ b/internal/database/queries.go @@ -465,11 +465,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) { @@ -483,10 +486,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 @@ -496,25 +499,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 3826c7a..5ddb9f3 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,79 @@ 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) +} + +// EscapedVersion returns the version escaped for use as a single URL path +// segment. +// +// Version returns decoded text, which is what should be shown to a user but is +// not safe to drop into a link: html/template preserves reserved characters and +// existing escapes in a URL, so "release/1" would split into two path segments, +// "v1?build" would start a query string, and a literal "%2B" would be read back +// as "+". Escaping here and decoding in splitWildcardPath round-trips the value, +// so the link resolves to the version that was stored. +func (v *Version) EscapedVersion() string { + return url.PathEscape(v.Version()) +} + +// 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:] } - return "" + + 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 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..517022b --- /dev/null +++ b/internal/database/version_purl_test.go @@ -0,0 +1,159 @@ +package database + +import ( + "database/sql" + "net/url" + "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) + } + }) + } +} + +// TestVersionEscapedVersion checks the value the templates put in a URL. It +// must survive the round trip back through the router: escaping here and +// decoding per path segment on the way in has to yield the original version. +func TestVersionEscapedVersion(t *testing.T) { + tests := []struct { + name string + purl string + want string + }{ + {"simple", "pkg:npm/lodash@4.17.21", "4.17.21"}, + // "+" is legal in a path segment, so it stays literal and the UI keeps + // showing the version the way Debian writes it. + {"plus stays literal", "pkg:deb/nmap@7.91%2Bdfsg1-2ubuntu0.1", "7.91+dfsg1-2ubuntu0.1"}, + // A slash would otherwise split the version into two path segments. + {"slash", "pkg:golang/example@release%2F1", "release%2F1"}, + // A question mark would otherwise start the query string. + {"question mark", "pkg:npm/example@v1%3Fbuild", "v1%3Fbuild"}, + // A version containing a literal "%2B" is stored double-encoded; the + // link must re-encode it or it decodes back to "+" instead. + {"literal percent escape", "pkg:npm/example@1.0%252B", "1.0%252B"}, + {"space", "pkg:npm/example@1.0%20beta", "1.0%20beta"}, + {"no version", "pkg:npm/lodash", ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v := &Version{PURL: tt.purl} + got := v.EscapedVersion() + if got != tt.want { + t.Errorf("EscapedVersion() for %q = %q, want %q", tt.purl, got, tt.want) + } + // The router decodes each path segment, which must give back the + // version the page displays. + decoded, err := url.PathUnescape(got) + if err != nil { + t.Fatalf("PathUnescape(%q) failed: %v", got, err) + } + if decoded != v.Version() { + t.Errorf("round trip for %q = %q, want %q", tt.purl, decoded, v.Version()) + } + }) + } +} + +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 60fa23a..b086fdf 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/api.go b/internal/server/api.go index ddb9ca7..992d736 100644 --- a/internal/server/api.go +++ b/internal/server/api.go @@ -139,12 +139,11 @@ type BulkResponse struct { // Resolves namespaced package names (Composer vendor/name, npm @scope/name) from the path. func (h *APIHandler) HandlePackagePath(w http.ResponseWriter, r *http.Request) { ecosystem := chi.URLParam(r, "ecosystem") - wildcard := chi.URLParam(r, "*") - if err := validatePackagePath(wildcard); err != nil { + segments, err := packagePathSegments(r) + if err != nil { badRequest(w, err.Error()) return } - segments := splitWildcardPath(wildcard) if ecosystem == "" || len(segments) == 0 { badRequest(w, "ecosystem and name are required") @@ -277,12 +276,11 @@ func (h *APIHandler) getVersion(w http.ResponseWriter, r *http.Request, ecosyste // Supports both {name} and {name}/{version} paths with namespaced package names. func (h *APIHandler) HandleVulnsPath(w http.ResponseWriter, r *http.Request) { ecosystem := chi.URLParam(r, "ecosystem") - wildcard := chi.URLParam(r, "*") - if err := validatePackagePath(wildcard); err != nil { + segments, err := packagePathSegments(r) + if err != nil { badRequest(w, err.Error()) return } - segments := splitWildcardPath(wildcard) if ecosystem == "" || len(segments) == 0 { badRequest(w, "ecosystem and name are required") diff --git a/internal/server/browse.go b/internal/server/browse.go index c60cba3..43ad9ae 100644 --- a/internal/server/browse.go +++ b/internal/server/browse.go @@ -147,12 +147,11 @@ type BrowseFileInfo struct { // {name}/{version}/file/{path} -> browse file func (s *Server) handleBrowsePath(w http.ResponseWriter, r *http.Request) { ecosystem := chi.URLParam(r, "ecosystem") - wildcard := chi.URLParam(r, "*") - if err := validatePackagePath(wildcard); err != nil { + segments, err := packagePathSegments(r) + if err != nil { badRequest(w, err.Error()) return } - segments := splitWildcardPath(wildcard) if ecosystem == "" || len(segments) < 2 { badRequest(w, "ecosystem, name, and version required") @@ -203,12 +202,11 @@ func (s *Server) handleBrowsePath(w http.ResponseWriter, r *http.Request) { // Supported paths: {name}/{fromVersion}/{toVersion} func (s *Server) handleComparePath(w http.ResponseWriter, r *http.Request) { ecosystem := chi.URLParam(r, "ecosystem") - wildcard := chi.URLParam(r, "*") - if err := validatePackagePath(wildcard); err != nil { + segments, err := packagePathSegments(r) + if err != nil { badRequest(w, err.Error()) return } - segments := splitWildcardPath(wildcard) if ecosystem == "" || len(segments) < 3 { badRequest(w, "ecosystem, name, fromVersion, and toVersion required") @@ -506,11 +504,16 @@ func isLikelyText(filename string) bool { } // BrowseSourceData contains data for the browse source page. +// +// Version is the decoded version, for display. EscapedVersion is the same value +// escaped as a single URL path segment and is what the links and the browse API +// calls must use; see database.Version.EscapedVersion. type BrowseSourceData struct { Layout - Ecosystem string - PackageName string - Version string + Ecosystem string + PackageName string + Version string + EscapedVersion string } // handleBrowseSource is now showBrowseSource in server.go, dispatched via handlePackagePath. @@ -601,12 +604,17 @@ func (s *Server) compareDiff(w http.ResponseWriter, r *http.Request, ecosystem, } // ComparePageData contains data for the version comparison page. +// +// FromVersion and ToVersion are decoded, for display; the Escaped variants are +// the path-segment form used to build the compare API URL. type ComparePageData struct { Layout - Ecosystem string - PackageName string - FromVersion string - ToVersion string + Ecosystem string + PackageName string + FromVersion string + ToVersion string + EscapedFromVersion string + EscapedToVersion string } // handleComparePage is now showComparePage in server.go, dispatched via handlePackagePath. diff --git a/internal/server/browse_test.go b/internal/server/browse_test.go index 6ea0f7e..3cc37c8 100644 --- a/internal/server/browse_test.go +++ b/internal/server/browse_test.go @@ -450,8 +450,10 @@ func TestHandleBrowseSourcePage(t *testing.T) { if !strings.Contains(body, "const packageName = 'test-browse'") { t.Error("browse source page missing packageName variable") } - if !strings.Contains(body, "const version = '1.0.0'") { - t.Error("browse source page missing version variable") + // The version reaches the browse API as one path segment, so the page holds + // its escaped form. + if !strings.Contains(body, "const versionPath = '1.0.0'") { + t.Error("browse source page missing versionPath variable") } // Verify content type @@ -617,12 +619,13 @@ func TestHandleComparePage(t *testing.T) { body := w.Body.String() - // Check that versions are set correctly in JavaScript - if !strings.Contains(body, "const fromVersion = '1.0.0'") { - t.Error("page should set fromVersion") + // Check that versions are set correctly in JavaScript. The compare API takes + // each version as a path segment, so the page holds their escaped forms. + if !strings.Contains(body, "const fromVersionPath = '1.0.0'") { + t.Error("page should set fromVersionPath") } - if !strings.Contains(body, "const toVersion = '2.0.0'") { - t.Error("page should set toVersion") + if !strings.Contains(body, "const toVersionPath = '2.0.0'") { + t.Error("page should set toVersionPath") } // Test invalid format (missing separator) diff --git a/internal/server/resolve.go b/internal/server/resolve.go index 51f203d..f7a1b23 100644 --- a/internal/server/resolve.go +++ b/internal/server/resolve.go @@ -2,10 +2,13 @@ package server import ( "fmt" + "net/http" + "net/url" "strings" "unicode" "github.com/git-pkgs/proxy/internal/database" + "github.com/go-chi/chi/v5" ) // maxPackagePathLen bounds the wildcard portion of package routes (name plus @@ -13,22 +16,69 @@ import ( // longer, so 512 leaves room without admitting pathological inputs. const maxPackagePathLen = 512 +// packagePathSegments validates the wildcard portion of a package route and +// splits it into decoded path segments. +func packagePathSegments(r *http.Request) ([]string, error) { + wildcard := chi.URLParam(r, "*") + encoded := wildcardIsEncoded(r) + if err := validatePackagePath(wildcard, encoded); err != nil { + return nil, err + } + + return splitWildcardPath(wildcard, encoded), nil +} + +// wildcardIsEncoded reports whether the chi wildcard for this request is still +// percent-encoded. +// +// chi routes on r.URL.RawPath when it is set and on r.URL.Path otherwise, and +// net/url only sets RawPath when the request's escaping differs from the +// canonical encoding of the decoded path. A version such as "release%2F1" is +// therefore routed raw, while "1.0%252B" (a version whose text contains a +// literal "%2B") encodes canonically and arrives already decoded once. The +// distinction decides whether the segments still need decoding: decoding the +// second case again would turn it into "1.0+" and resolve a different version. +func wildcardIsEncoded(r *http.Request) bool { + return r.URL.RawPath != "" +} + // validatePackagePath rejects wildcard package paths that cannot be valid in // any supported ecosystem. It is a coarse filter applied before database or // enrichment lookups; ecosystem-specific name rules are layered on top. -func validatePackagePath(path string) error { +// +// encoded has the meaning described on wildcardIsEncoded. +func validatePackagePath(path string, encoded bool) error { if path == "" { return fmt.Errorf("package name required") } 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") - } - if unicode.IsControl(r) { - return fmt.Errorf("package path contains control character %#U", r) + // 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, encoded) { + // Each segment is checked both as the handlers see it and decoded once + // more: a segment can reach a handler with escapes intact, and the + // upstream registry is then the one that decodes them. + for _, value := range []string{seg, decodePathSegment(seg)} { + // 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(value, "/") { + if elem == ".." { + return fmt.Errorf("package path contains parent directory segment") + } + } + for _, r := range value { + 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 +110,37 @@ func resolvePackageName(db *database.DB, ecosystem string, segments []string) (n // splitWildcardPath splits a chi wildcard path value into segments, // trimming any leading/trailing slashes. -func splitWildcardPath(path string) []string { +// +// When encoded is set the value is still percent-encoded (see +// wildcardIsEncoded), 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, encoded bool) []string { path = strings.Trim(path, "/") if path == "" { return nil } - return strings.Split(path, "/") + segments := strings.Split(path, "/") + if !encoded { + return segments + } + 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..1867f46 100644 --- a/internal/server/resolve_test.go +++ b/internal/server/resolve_test.go @@ -1,12 +1,15 @@ package server import ( + "net/http" + "net/http/httptest" "os" "path/filepath" "strings" "testing" "github.com/git-pkgs/proxy/internal/database" + "github.com/go-chi/chi/v5" ) func newTestDB(t *testing.T) (*database.DB, func()) { @@ -95,26 +98,44 @@ func TestResolvePackageName(t *testing.T) { func TestSplitWildcardPath(t *testing.T) { tests := []struct { - input string - want []string + input string + encoded bool + want []string }{ - {"lodash", []string{"lodash"}}, - {"lodash/4.17.21", []string{"lodash", "4.17.21"}}, - {"monolog/monolog", []string{"monolog", "monolog"}}, - {"symfony/console/6.0.0/browse", []string{"symfony", "console", "6.0.0", "browse"}}, - {"", nil}, - {"/", nil}, + {"lodash", false, []string{"lodash"}}, + {"lodash/4.17.21", false, []string{"lodash", "4.17.21"}}, + {"monolog/monolog", false, []string{"monolog", "monolog"}}, + {"symfony/console/6.0.0/browse", false, []string{"symfony", "console", "6.0.0", "browse"}}, + {"", false, nil}, + {"/", false, nil}, + // chi routes on the raw path when it differs from the canonical + // encoding of the decoded path, so segments arrive percent-encoded and + // must be decoded. + { + "nmap/7.91%2Bdfsg1%2Breally7.80%2Bdfsg1-2ubuntu0.1", true, + []string{"nmap", "7.91+dfsg1+really7.80+dfsg1-2ubuntu0.1"}, + }, + {"%40babel/core/7.0.0", true, []string{"@babel", "core", "7.0.0"}}, + // An encoded separator stays inside its segment rather than splitting. + {"vendor%2Fname/1.0.0", true, []string{"vendor/name", "1.0.0"}}, + // Invalid escapes are passed through untouched. + {"lodash/1.0%zz", true, []string{"lodash", "1.0%zz"}}, + // When chi routed on the already-decoded path, an escape that survived + // is part of the value: a version whose text is "1.0%2B" reaches here + // as "1.0%2B" and decoding it again would yield "1.0+". + {"nmap/1.0%2B", false, []string{"nmap", "1.0%2B"}}, } for _, tt := range tests { - got := splitWildcardPath(tt.input) + got := splitWildcardPath(tt.input, tt.encoded) if len(got) != len(tt.want) { - t.Errorf("splitWildcardPath(%q) = %v, want %v", tt.input, got, tt.want) + t.Errorf("splitWildcardPath(%q, %v) = %v, want %v", tt.input, tt.encoded, got, tt.want) continue } for i := range got { if got[i] != tt.want[i] { - t.Errorf("splitWildcardPath(%q)[%d] = %q, want %q", tt.input, i, got[i], tt.want[i]) + t.Errorf("splitWildcardPath(%q, %v)[%d] = %q, want %q", + tt.input, tt.encoded, i, got[i], tt.want[i]) } } } @@ -132,8 +153,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}, @@ -145,9 +177,64 @@ func TestValidatePackagePath(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := validatePackagePath(tt.path) - if (err != nil) != tt.wantErr { - t.Errorf("validatePackagePath(%q) error = %v, wantErr %v", tt.path, err, tt.wantErr) + // The verdict must not depend on whether chi routed on the raw or + // on the already-decoded path: an escape that reaches a handler + // undecoded is decoded by the upstream registry instead, so it is + // rejected either way. + for _, encoded := range []bool{false, true} { + err := validatePackagePath(tt.path, encoded) + if (err != nil) != tt.wantErr { + t.Errorf("validatePackagePath(%q, %v) error = %v, wantErr %v", + tt.path, encoded, err, tt.wantErr) + } + } + }) + } +} + +// TestPackagePathSegments drives the real router, which is what decides whether +// the wildcard still carries percent-encoding. Go decodes the request path +// itself unless the escaping is non-canonical, so the same version can arrive +// either way and only one of the two forms may be decoded again. +func TestPackagePathSegments(t *testing.T) { + tests := []struct { + name string + target string + want []string + }{ + {"plain", "/pkg/npm/lodash/4.17.21", []string{"lodash", "4.17.21"}}, + {"encoded plus", "/pkg/deb/nmap/7.91%2Bdfsg1-2ubuntu0.1", []string{"nmap", "7.91+dfsg1-2ubuntu0.1"}}, + {"decoded plus", "/pkg/deb/nmap/7.91+dfsg1-2ubuntu0.1", []string{"nmap", "7.91+dfsg1-2ubuntu0.1"}}, + // An encoded slash is one segment, not a separator. + {"encoded slash", "/pkg/composer/vendor%2Fname/1.0.0", []string{"vendor/name", "1.0.0"}}, + {"question mark", "/pkg/npm/example/v1%3Fbuild", []string{"example", "v1?build"}}, + // "1.0%252B" is the escaped form of the version "1.0%2B"; net/url + // already decoded it once, so it must not be decoded again. + {"literal percent escape", "/pkg/npm/example/1.0%252B", []string{"example", "1.0%2B"}}, + {"browse suffix", "/pkg/deb/nmap/7.91%2Bdfsg1/browse", []string{"nmap", "7.91+dfsg1", "browse"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var got []string + var gotErr error + + router := chi.NewRouter() + router.Get("/pkg/{ecosystem}/*", func(_ http.ResponseWriter, r *http.Request) { + got, gotErr = packagePathSegments(r) + }) + router.ServeHTTP(httptest.NewRecorder(), httptest.NewRequest("GET", tt.target, nil)) + + if gotErr != nil { + t.Fatalf("packagePathSegments(%q) failed: %v", tt.target, gotErr) + } + if len(got) != len(tt.want) { + t.Fatalf("segments for %q = %v, want %v", tt.target, got, tt.want) + } + for i := range got { + if got[i] != tt.want[i] { + t.Errorf("segments for %q [%d] = %q, want %q", tt.target, i, got[i], tt.want[i]) + } } }) } diff --git a/internal/server/server.go b/internal/server/server.go index c98d1cb..e677bc9 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -54,6 +54,7 @@ import ( "fmt" "log/slog" "net/http" + "net/url" "strconv" "strings" "time" @@ -680,12 +681,11 @@ func (s *Server) handlePackagesList(w http.ResponseWriter, r *http.Request) { // {name}/compare/{v1}...{v2} -> compare versions func (s *Server) handlePackagePath(w http.ResponseWriter, r *http.Request) { ecosystem := chi.URLParam(r, "ecosystem") - wildcard := chi.URLParam(r, "*") - if err := validatePackagePath(wildcard); err != nil { + segments, err := packagePathSegments(r) + if err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } - segments := splitWildcardPath(wildcard) if ecosystem == "" || len(segments) == 0 { http.Error(w, "ecosystem and package name required", http.StatusBadRequest) @@ -825,10 +825,11 @@ func (s *Server) showVersion(w http.ResponseWriter, r *http.Request, ecosystem, func (s *Server) showBrowseSource(w http.ResponseWriter, r *http.Request, ecosystem, name, version string) { data := BrowseSourceData{ - Layout: s.layoutFor(r), - Ecosystem: ecosystem, - PackageName: name, - Version: version, + Layout: s.layoutFor(r), + Ecosystem: ecosystem, + PackageName: name, + Version: version, + EscapedVersion: url.PathEscape(version), } if err := s.templates.Render(w, "browse_source", data); err != nil { @@ -846,11 +847,13 @@ func (s *Server) showComparePage(w http.ResponseWriter, r *http.Request, ecosyst } data := ComparePageData{ - Layout: s.layoutFor(r), - Ecosystem: ecosystem, - PackageName: name, - FromVersion: parts[0], - ToVersion: parts[1], + Layout: s.layoutFor(r), + Ecosystem: ecosystem, + PackageName: name, + FromVersion: parts[0], + ToVersion: parts[1], + EscapedFromVersion: url.PathEscape(parts[0]), + EscapedToVersion: url.PathEscape(parts[1]), } if err := s.templates.Render(w, "compare_versions", data); err != nil { diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 2d27147..98b58cc 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -4,12 +4,16 @@ import ( "database/sql" "encoding/json" "fmt" + "html" "io" "log/slog" "net/http" "net/http/httptest" + "net/url" "os" "path/filepath" + "regexp" + "strconv" "strings" "testing" "time" @@ -18,6 +22,7 @@ import ( "github.com/git-pkgs/proxy/internal/database" "github.com/git-pkgs/proxy/internal/handler" "github.com/git-pkgs/proxy/internal/storage" + "github.com/git-pkgs/purl" "github.com/git-pkgs/registries/fetch" "github.com/go-chi/chi/v5" ) @@ -764,6 +769,236 @@ 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) + } + } +} + +// TestVersionURLEscaping covers versions whose characters are significant in a +// URL path: "/" splits off another path segment, "?" starts a query string, and +// a literal "%xx" is read back as the character it encodes. The pages show the +// decoded version but must build every link from a separately escaped value, +// and those links have to resolve back to the same version. +func TestVersionURLEscaping(t *testing.T) { + // A second version is needed for the compare controls to be rendered. + const otherVersion = "1.0.0" + + tests := []struct { + name string + version string + }{ + {"slash", "release/1"}, + {"question mark", "v1?build"}, + {"literal percent escape", "1.0%2B"}, + {"plus", "7.91+dfsg1-2ubuntu0.1"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ts := newTestServer(t) + defer ts.close() + seedEscapingVersions(t, ts.db, tt.version, otherVersion) + + // The package page links to the escaped version. + escaped := url.PathEscape(tt.version) + versionPath := "/ui/package/deb/nmap/" + escaped + packagePage := ts.getOK(t, "/ui/package/deb/nmap") + if !containsValue(attrValues(packagePage, "href"), versionPath) { + t.Fatalf("package page has no link to %q; hrefs: %v", + versionPath, attrValues(packagePage, "href")) + } + + ts.checkVersionAndBrowsePages(t, versionPath, tt.version, escaped) + ts.checkComparePage(t, packagePage, tt.version, escaped, otherVersion) + }) + } +} + +// seedEscapingVersions stores a Debian package with the given versions, each +// with a cached artifact so that the version page offers its browse link. +func seedEscapingVersions(t *testing.T, db *database.DB, versions ...string) { + t.Helper() + + pkg := &database.Package{PURL: "pkg:deb/nmap", Ecosystem: "deb", Name: "nmap"} + if err := db.UpsertPackage(pkg); err != nil { + t.Fatalf("failed to upsert package: %v", err) + } + for _, v := range versions { + versionPURL := purl.MakePURLString("deb", "nmap", v) + if err := db.UpsertVersion(&database.Version{ + PURL: versionPURL, PackagePURL: pkg.PURL, + }); err != nil { + t.Fatalf("failed to upsert version %q: %v", v, err) + } + if err := db.UpsertArtifact(&database.Artifact{ + VersionPURL: versionPURL, + Filename: "nmap.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("failed to upsert artifact for %q: %v", v, err) + } + } +} + +// checkVersionAndBrowsePages follows a version link from the package page and +// then the browse link from the version page, checking that both resolve to the +// stored version and display it decoded. +func (ts *testServer) checkVersionAndBrowsePages(t *testing.T, versionPath, version, escaped string) { + t.Helper() + + versionPage := ts.getOK(t, versionPath) + wantPURL := "pkg:deb/nmap@" + version + if !strings.Contains(html.UnescapeString(versionPage), wantPURL) { + t.Errorf("version page does not show %q", wantPURL) + } + + browsePath := versionPath + "/browse" + if !containsValue(attrValues(versionPage, "href"), browsePath) { + t.Fatalf("version page has no browse link to %q; hrefs: %v", + browsePath, attrValues(versionPage, "href")) + } + + browsePage := ts.getOK(t, browsePath) + if !strings.Contains(html.UnescapeString(browsePage), "nmap@"+version) { + t.Errorf("browse page does not show the decoded version %q", version) + } + // The browse API is called with the escaped version, not with the text shown + // in the heading. + if got := jsConstant(t, browsePage, "versionPath"); got != escaped { + t.Errorf("browse page passes %q to the browse API, want %q", got, escaped) + } +} + +// checkComparePage builds the compare URL the way the package page's script +// does, from the values its checkboxes carry, and checks the page it reaches. +func (ts *testServer) checkComparePage(t *testing.T, packagePage, version, escaped, otherVersion string) { + t.Helper() + + selectable := attrValues(packagePage, "data-version-path") + if !containsValue(selectable, escaped) { + t.Fatalf("package page compare data holds %v, want %q", selectable, escaped) + } + + comparePage := ts.getOK(t, "/ui/package/deb/nmap/compare/"+escaped+"..."+otherVersion) + decoded := html.UnescapeString(comparePage) + for _, want := range []string{version, otherVersion} { + if !strings.Contains(decoded, want) { + t.Errorf("compare page does not show version %q", want) + } + } + if got := jsConstant(t, comparePage, "fromVersionPath"); got != escaped { + t.Errorf("compare page passes %q to the compare API, want %q", got, escaped) + } +} + +// getOK performs a GET against the server and fails the test unless it returns +// 200, returning the response body. +func (ts *testServer) getOK(t *testing.T, path string) string { + t.Helper() + + req := httptest.NewRequest("GET", path, nil) + w := httptest.NewRecorder() + ts.handler.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Fatalf("GET %s: expected status 200, got %d", path, w.Code) + } + + return w.Body.String() +} + +// attrValues returns the value of every occurrence of an HTML attribute in a +// rendered page, with HTML entities resolved so that values can be compared +// against the raw strings they were built from. +func attrValues(body, attr string) []string { + re := regexp.MustCompile(regexp.QuoteMeta(attr) + `="([^"]*)"`) + + var values []string + for _, match := range re.FindAllStringSubmatch(body, -1) { + values = append(values, html.UnescapeString(match[1])) + } + + return values +} + +// jsConstant returns the value of a single-quoted JavaScript string constant in +// a rendered page. html/template escapes characters that are significant in +// JavaScript, rendering "+" as "\\u002b" for instance, so the escapes are +// resolved to recover the value the page actually uses. +func jsConstant(t *testing.T, body, name string) string { + t.Helper() + + re := regexp.MustCompile(`const ` + regexp.QuoteMeta(name) + ` = '([^']*)'`) + match := re.FindStringSubmatch(body) + if match == nil { + t.Fatalf("page does not declare the constant %q", name) + } + + unescaped, err := strconv.Unquote(`"` + match[1] + `"`) + if err != nil { + t.Fatalf("cannot unescape %q: %v", match[1], err) + } + + return unescaped +} + +func containsValue(values []string, want string) bool { + for _, v := range values { + if v == want { + return true + } + } + + return false +} + func TestPackageShowPage_WithLicense(t *testing.T) { ts := newTestServer(t) defer ts.close() diff --git a/internal/server/templates/pages/browse_source.html b/internal/server/templates/pages/browse_source.html index ca06652..a949111 100644 --- a/internal/server/templates/pages/browse_source.html +++ b/internal/server/templates/pages/browse_source.html @@ -7,7 +7,7 @@ / {{.PackageName}} / - {{.Version}} + {{.Version}} / Browse Source @@ -51,7 +51,10 @@