Classify bare paths, validate forwarded query values, evict query variants

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 {

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)
 		}
 	}

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

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
 	}

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",

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)
 	}

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)
+	}
+}