diff --git a/docs/configuration.md b/docs/configuration.md index b75650fc..8a94c34e 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -479,7 +479,9 @@ its patterns match; any other path answers 404. The patterns use the syntax of `upstream.git` `repos`: `path.Match`, where `*` does not cross a `/`, and a pattern ending in `/**` also matches every path nested below its prefix. A path containing `%`, `?`, `#` or a backslash is refused for an upstream with -`generic_paths`. An upstream without `generic_paths` serves every path. +`generic_paths`. The patterns only check the path; the query string is passed +on as it is, except for release assets and the built-in artifact paths below, +which take none. An upstream without `generic_paths` serves every path. When `upstream.auth` has credentials for a generic upstream, `generic_paths` is required and the proxy refuses to start without it: everyone who can reach @@ -490,7 +492,8 @@ other token but cannot read anything private, even through an allowed path. Paths shaped like `{owner}/{repo}/releases/download/{tag}/{asset}` are version-pinned GitHub release assets: they are stored in the artifact cache and served from it without revalidation, including while the upstream is -down. +down. A release asset takes no query string; a request for one with a query, +or with an escaped `?` or `#` in its path, answers 400. Upstreams pointed at these hosts get version-pinned paths built in, whatever the upstream is named: @@ -501,6 +504,11 @@ the upstream is named: | `https://nodejs.org` | `dist/v{version}/{file}` | the upstream name | | `https://dl.google.com/go` | `go{version}.{os}-{arch}.tar.gz`, `.zip` and their `.sha256` | the upstream name | +Like release assets, these built-in paths take no query string; a request for +one with a query answers 400, as does one with an escaped `?` or `#` in its +path. A path that a configured `generic_artifacts` pattern matches first keeps +its query. + Other upstreams can declare their own version-pinned paths in `generic_artifacts`. These are tried before any built-in ones, so a configured pattern that overlaps a built-in decides the package name. Each pattern is a regular expression that must match the @@ -519,7 +527,9 @@ upstream: ``` Only list paths whose content never changes for a given version: a match is -never revalidated. +never revalidated. For a configured pattern the query string is passed on and is part of the cache +key, so each distinct query is fetched and stored as a copy of its own. Only +`storage.max_size` or a `generic` retention rule removes them. Every other path is served through the metadata cache (`cache_metadata` must be enabled for offline fallback): fresh within `metadata_ttl`, then diff --git a/internal/handler/generic.go b/internal/handler/generic.go index 0e08140e..7660dae2 100644 --- a/internal/handler/generic.go +++ b/internal/handler/generic.go @@ -71,6 +71,9 @@ type GenericHandler struct { proxy *Proxy repositories map[string]string artifactPatterns map[string][]*regexp.Regexp + // builtinPatterns holds each upstream's built-in patterns, tried after + // the configured ones. A path they match takes no query. + builtinPatterns map[string][]*regexp.Regexp // paths holds each upstream's upstream.generic_paths patterns; an // upstream without any serves every path. paths map[string][]string @@ -84,6 +87,7 @@ func NewGenericHandler(proxy *Proxy, repositories map[string]string, artifactPat proxy: proxy, repositories: make(map[string]string, len(repositories)), artifactPatterns: make(map[string][]*regexp.Regexp), + builtinPatterns: make(map[string][]*regexp.Regexp), } for name, upstreamURL := range repositories { h.repositories[name] = strings.TrimSuffix(upstreamURL, "/") @@ -91,11 +95,11 @@ func NewGenericHandler(proxy *Proxy, repositories map[string]string, artifactPat // The first matching pattern wins, so configured patterns go first. if len(artifactPatterns) > 0 { for name, patterns := range artifactPatterns[0] { - h.addArtifactPatterns(name, patterns) + h.addArtifactPatterns(h.artifactPatterns, name, patterns) } } for name, upstreamURL := range h.repositories { - h.addArtifactPatterns(name, builtinGenericArtifacts[upstreamURL]) + h.addArtifactPatterns(h.builtinPatterns, name, builtinGenericArtifacts[upstreamURL]) } return h } @@ -107,7 +111,7 @@ func (h *GenericHandler) WithPaths(paths map[string][]string) *GenericHandler { return h } -func (h *GenericHandler) addArtifactPatterns(name string, patterns []string) { +func (h *GenericHandler) addArtifactPatterns(into map[string][]*regexp.Regexp, name string, patterns []string) { for _, pattern := range patterns { re, err := config.CompileGenericArtifactPattern(pattern) if err != nil { @@ -115,7 +119,7 @@ func (h *GenericHandler) addArtifactPatterns(name string, patterns []string) { "repository", name, "pattern", pattern, "error", err) continue } - h.artifactPatterns[name] = append(h.artifactPatterns[name], re) + into[name] = append(into[name], re) } } @@ -147,6 +151,12 @@ func (h *GenericHandler) Routes() http.Handler { } if artifact, ok := h.parseArtifact(repository, rest); ok { + if artifact.noQuery && (r.URL.RawQuery != "" || strings.ContainsAny(rest, "?#")) { + // Each distinct query would be fetched and stored again. An + // escaped ? or # in the path would reach the upstream as one. + http.Error(w, "this download takes no query string", http.StatusBadRequest) + return + } h.handleArtifact(w, r, repository, upstreamURL, rest, artifact) return } @@ -192,21 +202,32 @@ type genericArtifact struct { name string version string filename string + // noQuery is set for a GitHub release asset and for a path matched by a + // built-in pattern, which take no query. + noQuery bool } // parseArtifact returns the identity of a version-pinned download path: one -// matching the upstream's artifact patterns, else a GitHub release asset. +// matching the upstream's configured artifact patterns, then its built-in +// ones, else a GitHub release asset. func (h *GenericHandler) parseArtifact(repository, path string) (genericArtifact, bool) { for _, pattern := range h.artifactPatterns[repository] { if artifact, ok := matchArtifactPattern(pattern, repository, path); ok { return artifact, true } } + for _, pattern := range h.builtinPatterns[repository] { + if artifact, ok := matchArtifactPattern(pattern, repository, path); ok { + artifact.noQuery = true + return artifact, true + } + } if asset, ok := parseGitHubReleaseAsset(path); ok { return genericArtifact{ name: asset.owner + "/" + asset.repo, version: asset.tag, filename: asset.filename, + noQuery: true, }, true } return genericArtifact{}, false diff --git a/internal/handler/generic_test.go b/internal/handler/generic_test.go index a74be8c8..a9d536ab 100644 --- a/internal/handler/generic_test.go +++ b/internal/handler/generic_test.go @@ -146,23 +146,23 @@ func TestGenericHandler_BuiltinArtifactPatterns(t *testing.T) { {"apache", "dist/maven/maven-3/3.9.9/binaries/apache-maven-3.9.9-bin.tar.gz.sha512", genericArtifact{name: "maven", version: "3.9.9", filename: "apache-maven-3.9.9-bin.tar.gz.sha512"}, true}, {"apache", "dist/maven/maven-4/4.0.0/binaries/apache-maven-4.0.0-bin.tar.gz", - genericArtifact{name: "maven/maven-4", version: "4.0.0", filename: "apache-maven-4.0.0-bin.tar.gz"}, true}, + genericArtifact{name: "maven/maven-4", version: "4.0.0", filename: "apache-maven-4.0.0-bin.tar.gz", noQuery: true}, true}, {"apache", "dist/maven/mvnd/1.0.6/maven-mvnd-1.0.6-linux-amd64.tar.gz", - genericArtifact{name: "maven/mvnd", version: "1.0.6", filename: "maven-mvnd-1.0.6-linux-amd64.tar.gz"}, true}, + genericArtifact{name: "maven/mvnd", version: "1.0.6", filename: "maven-mvnd-1.0.6-linux-amd64.tar.gz", noQuery: true}, true}, {"apache", "dist/maven/maven-3/", genericArtifact{}, false}, {"apache", "dist/maven/maven-3/3.9.9/source/apache-maven-3.9.9-src.tar.gz", genericArtifact{}, false}, {"apache", "extra/1.0/file.txt", genericArtifact{name: "apache", version: "1.0", filename: "file.txt"}, true}, {"node", "dist/v22.11.0/node-v22.11.0-linux-x64.tar.gz", - genericArtifact{name: "node", version: "v22.11.0", filename: "node-v22.11.0-linux-x64.tar.gz"}, true}, + genericArtifact{name: "node", version: "v22.11.0", filename: "node-v22.11.0-linux-x64.tar.gz", noQuery: true}, true}, {"node", "dist/v22.11.0/SHASUMS256.txt", - genericArtifact{name: "node", version: "v22.11.0", filename: "SHASUMS256.txt"}, true}, + genericArtifact{name: "node", version: "v22.11.0", filename: "SHASUMS256.txt", noQuery: true}, true}, {"node", "dist/index.json", genericArtifact{}, false}, {"golang", "go1.23.2.linux-amd64.tar.gz", - genericArtifact{name: "golang", version: "1.23.2", filename: "go1.23.2.linux-amd64.tar.gz"}, true}, + genericArtifact{name: "golang", version: "1.23.2", filename: "go1.23.2.linux-amd64.tar.gz", noQuery: true}, true}, {"golang", "go1.23.2.darwin-arm64.tar.gz.sha256", - genericArtifact{name: "golang", version: "1.23.2", filename: "go1.23.2.darwin-arm64.tar.gz.sha256"}, true}, + genericArtifact{name: "golang", version: "1.23.2", filename: "go1.23.2.darwin-arm64.tar.gz.sha256", noQuery: true}, true}, {"golang", "go1.24rc1.windows-amd64.zip", - genericArtifact{name: "golang", version: "1.24rc1", filename: "go1.24rc1.windows-amd64.zip"}, true}, + genericArtifact{name: "golang", version: "1.24rc1", filename: "go1.24rc1.windows-amd64.zip", noQuery: true}, true}, {"mirror", "dist/maven/maven-3/3.9.9/binaries/apache-maven-3.9.9-bin.tar.gz", genericArtifact{}, false}, } @@ -415,6 +415,92 @@ func TestGenericHandler_ReleaseAssetIsCachedAndServedWhenUpstreamDown(t *testing } } +// A GitHub release asset never needs a query, and each distinct one would be +// fetched and stored again, so one with a query is refused before it reaches +// the upstream. +func TestGenericHandler_RefusesAQueryOnAReleaseAsset(t *testing.T) { + var requests atomic.Int32 + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + _, _ = w.Write([]byte("jq binary bytes")) + })) + defer upstream.Close() + + proxy, _, store, _ := setupTestProxy(t) + store.seekable = true + fetcher := fetch.NewFetcher(fetch.WithHTTPClient(upstream.Client()), fetch.WithMaxRetries(0)) + proxy.Fetcher = fetcher + t.Cleanup(func() { _ = fetcher.Close() }) + h := NewGenericHandler(proxy, map[string]string{"github": upstream.URL}) + + for _, query := range []string{"?a=1", "?a=2", "%3Fa=1", "%23a", "?"} { + w := serveGenericRequest(h, "/github"+testReleaseAssetPath+query) + if query == "?" { + // An empty query is no query; it is served from the same entry. + if w.Code != http.StatusOK { + t.Errorf("%q: status = %d, want 200", query, w.Code) + } + continue + } + if w.Code != http.StatusBadRequest { + t.Errorf("%q: status = %d, want 400", query, w.Code) + } + } + if got := requests.Load(); got != 1 { + t.Errorf("upstream requests = %d, want 1", got) + } +} + +// The built-in hosts are static file hosts and take no query either. A +// configured pattern is tried first and keeps its query, also where it +// overlaps a built-in one. +func TestGenericHandler_RefusesAQueryOnABuiltInArtifact(t *testing.T) { + const path = "/dist/v22.0.0/node-v22.0.0.tar.gz" + var requests atomic.Int32 + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + _, _ = w.Write([]byte("node tarball bytes")) + })) + defer upstream.Close() + builtinGenericArtifacts[upstream.URL] = builtinGenericArtifacts["https://nodejs.org"] + t.Cleanup(func() { delete(builtinGenericArtifacts, upstream.URL) }) + + newHandler := func(configured map[string][]string) *GenericHandler { + proxy, _, store, _ := setupTestProxy(t) + store.seekable = true + fetcher := fetch.NewFetcher(fetch.WithHTTPClient(upstream.Client()), fetch.WithMaxRetries(0)) + proxy.Fetcher = fetcher + t.Cleanup(func() { _ = fetcher.Close() }) + return NewGenericHandler(proxy, map[string]string{"node": upstream.URL}, configured) + } + + h := newHandler(nil) + for query, want := range map[string]int{"?a=1": http.StatusBadRequest, "?a=2": http.StatusBadRequest, "": http.StatusOK} { + if w := serveGenericRequest(h, "/node"+path+query); w.Code != want { + t.Errorf("built-in %q: status = %d, want %d", query, w.Code, want) + } + } + // An escaped ? or # in the path would reach the upstream as a query or + // a fragment, and be stored as a copy of its own. + for _, suffix := range []string{"%3Fa=1", "%23a"} { + if w := serveGenericRequest(h, "/node"+path+suffix); w.Code != http.StatusBadRequest { + t.Errorf("built-in %q: status = %d, want 400", suffix, w.Code) + } + } + if got := requests.Load(); got != 1 { + t.Errorf("upstream requests = %d, want 1", got) + } + + h = newHandler(map[string][]string{"node": {`dist/(?Pv22[^/]*)/(?P[^/]+)`}}) + if w := serveGenericRequest(h, "/node"+path+"?a=1"); w.Code != http.StatusOK { + t.Errorf("configured overlap: status = %d, want 200", w.Code) + } + // A path only the built-in pattern matches is still refused. + if w := serveGenericRequest(h, "/node/dist/v20.0.0/node-v20.0.0.tar.gz?a=1"); w.Code != http.StatusBadRequest { + t.Errorf("built-in beside configured: status = %d, want 400", w.Code) + } +} + func TestGenericHandler_ServesReleaseAssetCachedUnderUnhashedKey(t *testing.T) { asset := []byte("jq binary bytes") var available atomic.Bool @@ -451,11 +537,6 @@ func TestGenericHandler_ServesReleaseAssetCachedUnderUnhashedKey(t *testing.T) { t.Fatalf("legacy entry: status = %d, body = %q; want 200 %q", w.Code, w.Body.String(), asset) } - w = serveGenericRequest(h, "/github"+testReleaseAssetPath+"?x=1") - if w.Code != http.StatusBadGateway { - t.Errorf("with query: status = %d, want 502", w.Code) - } - repointed := NewGenericHandler(proxy, map[string]string{"github": upstream.URL + "/mirror"}) w = serveGenericRequest(repointed, "/github"+testReleaseAssetPath) if w.Code != http.StatusBadGateway { @@ -463,6 +544,52 @@ func TestGenericHandler_ServesReleaseAssetCachedUnderUnhashedKey(t *testing.T) { } } +// A configured artifact cached under the unhashed key before hashed keys is +// served only for the query it was fetched with, not for another one. +func TestGenericHandler_ServesConfiguredArtifactCachedUnderUnhashedKeyOnlyForItsQuery(t *testing.T) { + asset := []byte("tool bytes") + var available atomic.Bool + available.Store(true) + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !available.Load() { + http.Error(w, "unavailable", http.StatusServiceUnavailable) + return + } + _, _ = w.Write(asset) + })) + defer upstream.Close() + + proxy, _, store, _ := setupTestProxy(t) + store.seekable = true + fetcher := fetch.NewFetcher(fetch.WithHTTPClient(upstream.Client()), fetch.WithMaxRetries(0)) + proxy.Fetcher = fetcher + t.Cleanup(func() { _ = fetcher.Close() }) + + const path = "/releases/tool/1.0.0/linux/tool.tar.gz" + result, err := proxy.GetOrFetchArtifactFromURL(context.Background(), genericEcosystem, + "tool", "1.0.0", "downloads/tool.tar.gz", upstream.URL+path+"?variant=a") + if err != nil { + t.Fatalf("priming legacy entry: %v", err) + } + if result.Reader != nil { + _ = result.Reader.Close() + } + available.Store(false) + + h := NewGenericHandler(proxy, + map[string]string{"downloads": upstream.URL}, + map[string][]string{"downloads": {`releases/(?Ptool)/(?P[^/]+)/linux/(?P[^/]+)`}}) + w := serveGenericRequest(h, "/downloads"+path+"?variant=a") + if w.Code != http.StatusOK || w.Body.String() != string(asset) { + t.Fatalf("same query: status = %d, body = %q; want 200 %q", w.Code, w.Body.String(), asset) + } + for _, query := range []string{"?variant=b", ""} { + if w := serveGenericRequest(h, "/downloads"+path+query); w.Code != http.StatusBadGateway { + t.Errorf("query %q: status = %d, want 502", query, w.Code) + } + } +} + func TestGenericHandler_ReleaseAssetNotFoundIsNotCached(t *testing.T) { upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { http.NotFound(w, r)