diff --git a/internal/api/v1/forwardquery_test.go b/internal/api/v1/forwardquery_test.go new file mode 100644 index 0000000..6121838 --- /dev/null +++ b/internal/api/v1/forwardquery_test.go @@ -0,0 +1,58 @@ +package v1 + +import ( + "net/url" + "strings" + "testing" + + "git.unkin.net/unkin/artifactapi/internal/provider" +) + +type plainProvider struct{ provider.Provider } + +type forwardingProvider struct{ plainProvider } + +func digits(v string) (string, bool) { + if v == "" || strings.Trim(v, "0123456789") != "" { + return "", false + } + return strings.TrimLeft(v, "0"), true +} + +func (forwardingProvider) ForwardedQuery(path string) map[string]provider.QueryValidator { + if strings.HasSuffix(path, ".tar.gz") { + return nil + } + return map[string]provider.QueryValidator{"limit": digits, "offset": digits} +} + +func TestForwardQuery(t *testing.T) { + tests := []struct { + name string + prov provider.Provider + path string + query string + want string + }{ + {"non-forwarder keeps path", plainProvider{}, "a/versions/", "limit=10&offset=5", "a/versions/"}, + {"no query", forwardingProvider{}, "a/versions/", "", "a/versions/"}, + {"allowlisted params sorted", forwardingProvider{}, "a/versions/", "offset=5&limit=10", "a/versions/?limit=10&offset=5"}, + {"unknown params dropped", forwardingProvider{}, "a/versions/", "x=1&limit=10&cb=zz", "a/versions/?limit=10"}, + {"only unknown params", forwardingProvider{}, "a/versions/", "x=1", "a/versions/"}, + {"first value wins", forwardingProvider{}, "a/versions/", "limit=10&limit=20", "a/versions/?limit=10"}, + {"invalid value dropped", forwardingProvider{}, "a/versions/", "limit=1%202&offset=5", "a/versions/?offset=5"}, + {"value canonicalised", forwardingProvider{}, "a/versions/", "limit=0010", "a/versions/?limit=10"}, + {"path without allowlist", forwardingProvider{}, "a/b-1.0.0.tar.gz", "limit=10", "a/b-1.0.0.tar.gz"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + q, err := url.ParseQuery(tt.query) + if err != nil { + t.Fatal(err) + } + if got := forwardQuery(tt.prov, tt.path, q); got != tt.want { + t.Errorf("forwardQuery = %q, want %q", got, tt.want) + } + }) + } +} diff --git a/internal/api/v1/proxy.go b/internal/api/v1/proxy.go index dbe083e..486394b 100644 --- a/internal/api/v1/proxy.go +++ b/internal/api/v1/proxy.go @@ -8,6 +8,7 @@ import ( "log/slog" "net" "net/http" + "net/url" "strconv" "strings" @@ -115,7 +116,7 @@ func (h *ProxyHandler) handleProxy(w http.ResponseWriter, r *http.Request) { } func (h *ProxyHandler) serveFetch(w http.ResponseWriter, r *http.Request, remote *models.Remote, prov provider.Provider, path string) { - result, err := h.engine.Fetch(r.Context(), *remote, path, prov, r.Header) + result, err := h.engine.Fetch(r.Context(), *remote, forwardQuery(prov, path, r.URL.Query()), prov, r.Header) if err != nil { var proxyErr *proxy.ProxyError if errors.As(err, &proxyErr) { @@ -325,6 +326,25 @@ func Scheme(r *http.Request) string { return "http" } +// forwardQuery appends the provider's validated query params to path in a +// canonical (sorted, single-valued) form, since the result is the cache key. +func forwardQuery(prov provider.Provider, path string, query url.Values) string { + f, ok := prov.(provider.QueryForwarder) + if !ok { + return path + } + kept := url.Values{} + for name, valid := range f.ForwardedQuery(path) { + if v, ok := valid(query.Get(name)); ok { + kept.Set(name, v) + } + } + if len(kept) == 0 { + return path + } + return path + "?" + kept.Encode() +} + // rewriteServed rewrites a cached body for the requesting host and marks the // response uncacheable by shared caches, since it varies per Host. func rewriteServed(w http.ResponseWriter, r *http.Request, sr provider.ServeRewriter, remote models.Remote, src io.Reader) ([]byte, bool) { diff --git a/internal/provider/cap_query_forwarder.go b/internal/provider/cap_query_forwarder.go new file mode 100644 index 0000000..096336c --- /dev/null +++ b/internal/provider/cap_query_forwarder.go @@ -0,0 +1,12 @@ +package provider + +// QueryForwarder is a remote provider whose upstream answers depend on the +// query string (e.g. paginated JSON APIs). Only the named params for path are +// forwarded upstream and kept in the cache key, each as the canonical value +// its validator returns; a value the validator rejects is dropped. +type QueryForwarder interface { + ForwardedQuery(path string) map[string]QueryValidator +} + +// QueryValidator canonicalises a raw query value, reporting false to drop it. +type QueryValidator func(v string) (string, bool) diff --git a/internal/proxy/classifier.go b/internal/proxy/classifier.go index 82b889c..28aee30 100644 --- a/internal/proxy/classifier.go +++ b/internal/proxy/classifier.go @@ -2,6 +2,7 @@ package proxy import ( "regexp" + "strings" "sync" "git.unkin.net/unkin/artifactapi/internal/provider" @@ -37,7 +38,9 @@ func NewClassifier(p provider.Provider) *Classifier { return &Classifier{provider: p} } +// Classify judges the bare path; a forwarded query must not change the verdict. func (c *Classifier) Classify(remote models.Remote, path string) Classification { + path, _, _ = strings.Cut(path, "?") if matchesAny(path, compilePatterns(remote.Blocklist)) { return ClassDenied } diff --git a/internal/proxy/classifier_extra_test.go b/internal/proxy/classifier_extra_test.go index dd5403a..7eaf152 100644 --- a/internal/proxy/classifier_extra_test.go +++ b/internal/proxy/classifier_extra_test.go @@ -38,6 +38,24 @@ func TestClassifierBranches(t *testing.T) { } } +func TestClassifierIgnoresForwardedQuery(t *testing.T) { + gp, err := provider.Get(models.PackageGeneric) + if err != nil { + t.Fatal(err) + } + c := NewClassifier(gp) + if got := c.Classify(models.Remote{Blocklist: []string{`\.exe$`}}, "x.exe?limit=1"); got != ClassDenied { + t.Errorf("blocklist bypassed by query: %v", got) + } + allow := models.Remote{Patterns: []string{`/versions/$`}} + if got := c.Classify(allow, "a/versions/?limit=1&offset=100"); got != ClassImmutable { + t.Errorf("allowlisted paginated path = %v, want immutable", got) + } + if got := c.Classify(models.Remote{MutablePatterns: []string{`/versions/$`}}, "a/versions/?offset=100"); got != ClassMutable { + t.Errorf("mutable pattern on paginated path = %v, want mutable", got) + } +} + func TestClassificationString(t *testing.T) { for c, want := range map[Classification]string{ ClassImmutable: "immutable", diff --git a/internal/proxy/engine.go b/internal/proxy/engine.go index 99d24b7..779bec9 100644 --- a/internal/proxy/engine.go +++ b/internal/proxy/engine.go @@ -217,7 +217,8 @@ func (e *Engine) Fetch(ctx context.Context, remote models.Remote, path string, p // Evict drops path from every cache layer (artifact row, index object, Redis // freshness and ETag keys) so the next request refetches from upstream. A -// trailing "/*" evicts every path under that directory. +// trailing "/*" evicts every path under that directory; a plain path also +// drops its forwarded-query variants ("path?..."). func (e *Engine) Evict(ctx context.Context, remoteName, path string) error { prefix, wildcard := strings.CutSuffix(path, "*") if wildcard && !strings.HasSuffix(prefix, "/") { @@ -240,11 +241,23 @@ func (e *Engine) Evict(ctx context.Context, remoteName, path string) error { if err := e.store.Delete(ctx, storage.IndexKey(remoteName, path)); err != nil { return fmt.Errorf("delete index: %w", err) } - return e.cache.ForgetPath(ctx, remoteName, path) + if err := e.cache.ForgetPath(ctx, remoteName, path); err != nil { + return err + } + if strings.Contains(path, "?") { + return nil + } + return e.evictPrefix(ctx, remoteName, path+"?") } // ponytail: no lock for wildcards; a Fetch already in flight under the // prefix can re-cache its path after the evict. Per-path locks over a // directory would close it if that ever matters. + return e.evictPrefix(ctx, remoteName, prefix) +} + +// evictPrefix drops every cached path starting with prefix, including the +// forwarded-query variants ("path?...") of a single path. +func (e *Engine) evictPrefix(ctx context.Context, remoteName, prefix string) error { if err := e.db.DeleteArtifactsByPrefix(ctx, remoteName, prefix); err != nil { return fmt.Errorf("delete artifacts: %w", err) } diff --git a/internal/proxy/evict_test.go b/internal/proxy/evict_test.go index 55635f4..aa05abc 100644 --- a/internal/proxy/evict_test.go +++ b/internal/proxy/evict_test.go @@ -271,3 +271,28 @@ func TestEvictWildcardClearsPrefixOnly(t *testing.T) { t.Errorf("path outside prefix = %q, want cached rev1", got) } } + +func TestEvictPathDropsQueryVariants(t *testing.T) { + requireStack(t) + srv, _, _ := changingUpstream(t) + r := rpmRemote(t, "evict-query", srv.URL) + const ( + path = "8/Everything/x86_64/repodata/repomd.xml" + variant = path + "?limit=10" + sibling = "8/Everything/x86_64/repodata/repomd.xml.asc" + ) + for _, p := range []string{path, variant, sibling} { + fetchBody(t, r, p) + } + if err := testEngine.Evict(context.Background(), r.Name, path); err != nil { + t.Fatalf("evict: %v", err) + } + for _, p := range []string{path, variant} { + if c := layers(t, r.Name, p); c != (cached{}) { + t.Errorf("%s after evict = %+v, want every layer gone", p, c) + } + } + if c := layers(t, r.Name, sibling); c == (cached{}) { + t.Errorf("%s sharing the name prefix was evicted", sibling) + } +}