diff --git a/internal/api/v1/forwardquery_test.go b/internal/api/v1/forwardquery_test.go index be758e2..6121838 100644 --- a/internal/api/v1/forwardquery_test.go +++ b/internal/api/v1/forwardquery_test.go @@ -12,15 +12,21 @@ type plainProvider struct{ provider.Provider } type forwardingProvider struct{ plainProvider } -func (forwardingProvider) ForwardedQuery(path string) []string { +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 []string{"limit", "offset"} + return map[string]provider.QueryValidator{"limit": digits, "offset": digits} } func TestForwardQuery(t *testing.T) { - long := strings.Repeat("9", maxForwardedValueLen+1) tests := []struct { name string prov provider.Provider @@ -34,8 +40,8 @@ func TestForwardQuery(t *testing.T) { {"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"}, - {"values re-encoded", forwardingProvider{}, "a/versions/", "limit=1%202", "a/versions/?limit=1+2"}, - {"overlong value dropped", forwardingProvider{}, "a/versions/", "limit=" + long + "&offset=5", "a/versions/?offset=5"}, + {"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 { diff --git a/internal/api/v1/proxy.go b/internal/api/v1/proxy.go index c64c139..8100658 100644 --- a/internal/api/v1/proxy.go +++ b/internal/api/v1/proxy.go @@ -94,9 +94,7 @@ func (h *ProxyHandler) handleProxy(w http.ResponseWriter, r *http.Request) { } } - path = forwardQuery(prov, path, r.URL.Query()) - - 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) { @@ -262,9 +260,7 @@ func scheme(r *http.Request) string { return "http" } -const maxForwardedValueLen = 32 - -// forwardQuery appends the provider's allowlisted query params to path in a +// 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) @@ -272,8 +268,8 @@ func forwardQuery(prov provider.Provider, path string, query url.Values) string return path } kept := url.Values{} - for _, name := range f.ForwardedQuery(path) { - if v := query.Get(name); v != "" && len(v) <= maxForwardedValueLen { + for name, valid := range f.ForwardedQuery(path) { + if v, ok := valid(query.Get(name)); ok { kept.Set(name, v) } } diff --git a/internal/provider/provider.go b/internal/provider/provider.go index 37afb2d..66cf2b5 100644 --- a/internal/provider/provider.go +++ b/internal/provider/provider.go @@ -67,11 +67,15 @@ type MetadataStore interface { // 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; nil forwards nothing. +// 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) []string + ForwardedQuery(path string) map[string]QueryValidator } +// QueryValidator canonicalises a raw query value, reporting false to drop it. +type QueryValidator func(v string) (string, bool) + // RemoteServer lets a remote provider fully answer a request itself instead of // going through the byte-proxy engine. It is the remote-side analog of // LocalIndexer: a metadata-only remote (e.g. github_rpm) uses it to synthesize 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 45a1b87..a2e82e3 100644 --- a/internal/proxy/engine.go +++ b/internal/proxy/engine.go @@ -216,7 +216,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, "/") { @@ -238,11 +239,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 15b8def..347de74 100644 --- a/internal/proxy/evict_test.go +++ b/internal/proxy/evict_test.go @@ -269,3 +269,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) + } +}