Refuse a query string on a GitHub release asset

docs/configuration.md

@@ -470,7 +470,8 @@ 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. 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
@@ -481,7 +482,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
+answers 400.
 
 Upstreams pointed at these hosts get version-pinned paths built in, whatever
 the upstream is named:
@@ -510,7 +512,9 @@ upstream:
 ```
 
 Only list paths whose content never changes for a given version: a match is
-never revalidated.
+never revalidated. 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

@@ -147,6 +147,11 @@ func (h *GenericHandler) Routes() http.Handler {
 		}
 
 		if artifact, ok := h.parseArtifact(repository, rest); ok {
+			if artifact.releaseAsset && r.URL.RawQuery != "" {
+				// Each distinct query would be fetched and stored again.
+				http.Error(w, "a release asset takes no query string", http.StatusBadRequest)
+				return
+			}
 			h.handleArtifact(w, r, repository, upstreamURL, rest, artifact)
 			return
 		}
@@ -192,6 +197,8 @@ type genericArtifact struct {
 	name     string
 	version  string
 	filename string
+	// releaseAsset is set for a GitHub release asset, which takes no query.
+	releaseAsset bool
 }
 
 // parseArtifact returns the identity of a version-pinned download path: one
@@ -204,9 +211,10 @@ func (h *GenericHandler) parseArtifact(repository, path string) (genericArtifact
 	}
 	if asset, ok := parseGitHubReleaseAsset(path); ok {
 		return genericArtifact{
-			name:     asset.owner + "/" + asset.repo,
-			version:  asset.tag,
-			filename: asset.filename,
+			name:         asset.owner + "/" + asset.repo,
+			version:      asset.tag,
+			filename:     asset.filename,
+			releaseAsset: true,
 		}, true
 	}
 	return genericArtifact{}, false

internal/handler/generic_test.go

@@ -415,6 +415,42 @@ 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", "?"} {
+		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)
+	}
+}
+
 func TestGenericHandler_ServesReleaseAssetCachedUnderUnhashedKey(t *testing.T) {
 	asset := []byte("jq binary bytes")
 	var available atomic.Bool
@@ -451,11 +487,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 {