Refuse a query string on release assets and built-in generic artifacts

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

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,18 +87,19 @@ 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, "/")
 	}
 	// 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,15 +111,15 @@ 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 {
 			h.proxy.Logger.Warn("skipping generic artifact pattern",
 				"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

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/(?P<version>v22[^/]*)/(?P<file>[^/]+)`}})
+	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,18 +537,59 @@ 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 {
 		t.Errorf("repointed upstream: status = %d, want 502", w.Code)
 	}
 }
 
+// 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/(?P<name>tool)/(?P<version>[^/]+)/linux/(?P<file>[^/]+)`}})
+	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)