diff --git a/docs/configuration.md b/docs/configuration.md index 1ac89a6b..9b900977 100644 --- a/docs/configuration.md +++ b/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 diff --git a/internal/handler/generic.go b/internal/handler/generic.go index 0e08140e..2ed5dd66 100644 --- a/internal/handler/generic.go +++ b/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 diff --git a/internal/handler/generic_test.go b/internal/handler/generic_test.go index a74be8c8..4cbe4f6e 100644 --- a/internal/handler/generic_test.go +++ b/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 {