harden rpm virtual redirects, fail closed, keep data files resolvable

- reject absolute and dot-segment member paths, escape the target, keep the query
- return 502 when any member's repodata can't be fetched
- serve recently merged data files by hash for 5m after repomd changes
- fold xml:base under a member upstream into the virtual href

internal/api/v1/proxy.go

@@ -162,7 +162,12 @@ func (h *ProxyHandler) handleVirtual(w http.ResponseWriter, r *http.Request) {
 
 	proxyBaseURL := fmt.Sprintf("%s://%s", scheme(r), r.Host)
 
-	if loc, ok := h.virtualEngine.MemberRedirect(r.Context(), *virt, path, proxyBaseURL); ok {
+	loc, ok, err := h.virtualEngine.MemberRedirect(r.Context(), *virt, path, r.URL.RawQuery, proxyBaseURL)
+	if err != nil {
+		http.Error(w, err.Error(), http.StatusBadRequest)
+		return
+	}
+	if ok {
 		http.Redirect(w, r, loc, http.StatusFound)
 		return
 	}

internal/virtual/engine.go

@@ -8,9 +8,11 @@ import (
 	"log/slog"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"slices"
 	"strings"
 	"sync"
+	"time"
 
 	"git.unkin.net/unkin/artifactapi/internal/database"
 	"git.unkin.net/unkin/artifactapi/internal/provider"
@@ -21,10 +23,26 @@ import (
 type Engine struct {
 	db          *database.DB
 	proxyEngine *proxy.Engine
+	getRemote   func(context.Context, string) (*models.Remote, error)
+	rpmMember   func(context.Context, string) (*RPMMember, error)
+
+	mu       sync.Mutex
+	rpmFiles map[string]rpmFile
 }
 
+type rpmFile struct {
+	body    []byte
+	expires time.Time
+}
+
+// rpmFileTTL keeps content-hashed data files resolvable after repomd moves on,
+// so a client that fetched the previous repomd can still fetch its data.
+const rpmFileTTL = 5 * time.Minute
+
 func NewEngine(db *database.DB, proxyEngine *proxy.Engine) *Engine {
-	return &Engine{db: db, proxyEngine: proxyEngine}
+	e := &Engine{db: db, proxyEngine: proxyEngine, getRemote: db.GetRemote}
+	e.rpmMember = e.fetchRPMMember
+	return e
 }
 
 func (e *Engine) Fetch(ctx context.Context, virt models.Virtual, path string, proxyBaseURL string) ([]byte, string, error) {
@@ -143,61 +161,89 @@ func (e *Engine) fetchLocalIndex(ctx context.Context, remote models.Remote, path
 	return indexer.GenerateLocalIndex(ctx, e.db, remote.Name, path)
 }
 
-var ErrNotFound = errors.New("not found")
+var (
+	ErrNotFound = errors.New("not found")
+	ErrBadPath  = errors.New("invalid path")
+)
 
 // MemberRedirect maps an rpm virtual package path (<member>/<href>, as written
-// by MergeRPM) to the owning member's route. ok is false for any other path.
-func (e *Engine) MemberRedirect(ctx context.Context, virt models.Virtual, path, proxyBaseURL string) (string, bool) {
+// by MergeRPM) to the owning member's route. ok is false for any other path;
+// ErrBadPath means the path is absolute or escapes the member.
+func (e *Engine) MemberRedirect(ctx context.Context, virt models.Virtual, path, rawQuery, proxyBaseURL string) (string, bool, error) {
 	if virt.PackageType != models.PackageRPM || strings.HasPrefix(path, "repodata/") {
-		return "", false
+		return "", false, nil
 	}
 	name, rest, found := strings.Cut(path, "/")
 	if !found || !slices.Contains(virt.Members, name) {
-		return "", false
+		return "", false, nil
 	}
-	remote, err := e.db.GetRemote(ctx, name)
+	segs, err := memberSegments(rest)
 	if err != nil {
-		return "", false
+		return "", false, err
 	}
-	return fmt.Sprintf("%s/api/v1/%s/%s/%s", strings.TrimRight(proxyBaseURL, "/"), remote.RepoType, name, rest), true
+	remote, err := e.getRemote(ctx, name)
+	if err != nil {
+		return "", false, nil
+	}
+	loc := fmt.Sprintf("%s/api/v1/%s/%s/%s", strings.TrimRight(proxyBaseURL, "/"), remote.RepoType, url.PathEscape(name), strings.Join(segs, "/"))
+	if rawQuery != "" {
+		loc += "?" + rawQuery
+	}
+	return loc, true, nil
+}
+
+// memberSegments decodes rest and returns its path-escaped segments, rejecting
+// absolute paths and any "." or ".." segment.
+func memberSegments(rest string) ([]string, error) {
+	dec, err := url.PathUnescape(rest)
+	if err != nil || dec == "" || strings.HasPrefix(dec, "/") {
+		return nil, ErrBadPath
+	}
+	segs := strings.Split(dec, "/")
+	for i, s := range segs {
+		if s == "." || s == ".." || strings.Contains(s, "\\") {
+			return nil, ErrBadPath
+		}
+		segs[i] = url.PathEscape(s)
+	}
+	return segs, nil
 }
 
 func (e *Engine) fetchRPM(ctx context.Context, virt models.Virtual, path string) ([]byte, string, error) {
 	if !strings.HasPrefix(path, "repodata/") {
 		return nil, "", ErrNotFound
 	}
+	if path != "repodata/repomd.xml" {
+		if body, ok := e.cachedRPMFile(path); ok {
+			return body, "application/gzip", nil
+		}
+	}
 
-	results := make([]*RPMMember, len(virt.Members))
+	members := make([]RPMMember, len(virt.Members))
+	errs := make([]error, len(virt.Members))
 	var wg sync.WaitGroup
 	for i, name := range virt.Members {
 		wg.Add(1)
 		go func() {
 			defer wg.Done()
-			m, err := e.fetchRPMMember(ctx, name)
+			m, err := e.rpmMember(ctx, name)
 			if err != nil {
-				slog.Warn("virtual member fetch failed", "virtual", virt.Name, "member", name, "error", err)
+				errs[i] = fmt.Errorf("member %q: %w", name, err)
 				return
 			}
-			results[i] = m
+			members[i] = *m
 		}()
 	}
 	wg.Wait()
-
-	var members []RPMMember
-	for _, m := range results {
-		if m != nil {
-			members = append(members, *m)
-		}
-	}
-	if len(members) == 0 {
-		return nil, "", fmt.Errorf("no members reachable for virtual %q", virt.Name)
+	if err := errors.Join(errs...); err != nil {
+		return nil, "", fmt.Errorf("virtual %q: %w", virt.Name, err)
 	}
 
-	// ponytail: merges on every repodata request; cache keyed on member repomd checksums if member count/size makes this slow.
 	repo, err := MergeRPM(members)
 	if err != nil {
 		return nil, "", fmt.Errorf("merge rpm repodata: %w", err)
 	}
+	e.storeRPMFiles(repo.Files)
 	if path == "repodata/repomd.xml" {
 		return repo.Repomd, "application/xml", nil
 	}
@@ -207,8 +253,38 @@ func (e *Engine) fetchRPM(ctx context.Context, virt models.Virtual, path string)
 	return nil, "", ErrNotFound
 }
 
+// ponytail: per-replica cache; behind a non-sticky LB a data request landing on
+// another replica re-merges and 404s if a member changed in between. Move to the
+// shared redis cache if that shows up.
+func (e *Engine) storeRPMFiles(files map[string][]byte) {
+	now := time.Now()
+	e.mu.Lock()
+	defer e.mu.Unlock()
+	if e.rpmFiles == nil {
+		e.rpmFiles = map[string]rpmFile{}
+	}
+	for k, f := range e.rpmFiles {
+		if now.After(f.expires) {
+			delete(e.rpmFiles, k)
+		}
+	}
+	for k, body := range files {
+		e.rpmFiles[k] = rpmFile{body: body, expires: now.Add(rpmFileTTL)}
+	}
+}
+
+func (e *Engine) cachedRPMFile(path string) ([]byte, bool) {
+	e.mu.Lock()
+	defer e.mu.Unlock()
+	f, ok := e.rpmFiles[path]
+	if !ok || time.Now().After(f.expires) {
+		return nil, false
+	}
+	return f.body, true
+}
+
 func (e *Engine) fetchRPMMember(ctx context.Context, name string) (*RPMMember, error) {
-	remote, err := e.db.GetRemote(ctx, name)
+	remote, err := e.getRemote(ctx, name)
 	if err != nil {
 		return nil, fmt.Errorf("remote %q: %w", name, err)
 	}
@@ -220,7 +296,7 @@ func (e *Engine) fetchRPMMember(ctx context.Context, name string) (*RPMMember, e
 	if err != nil {
 		return nil, err
 	}
-	m := &RPMMember{RemoteName: name, Timestamp: ts, Data: map[string][]byte{}}
+	m := &RPMMember{RemoteName: name, Bases: remote.UpstreamPool(), Timestamp: ts, Data: map[string][]byte{}}
 	for t, href := range locs {
 		raw, err := e.fetchMemberPath(ctx, *remote, href)
 		if err != nil {

internal/virtual/engine_test.go

@@ -0,0 +1,89 @@
+package virtual
+
+import (
+	"context"
+	"errors"
+	"regexp"
+	"testing"
+
+	"git.unkin.net/unkin/artifactapi/pkg/models"
+)
+
+func fakeEngine(members map[string]*RPMMember) *Engine {
+	return &Engine{
+		getRemote: func(_ context.Context, name string) (*models.Remote, error) {
+			return &models.Remote{Name: name, RepoType: models.RepoTypeRemote}, nil
+		},
+		rpmMember: func(_ context.Context, name string) (*RPMMember, error) {
+			if m := members[name]; m != nil {
+				return m, nil
+			}
+			return nil, errors.New("upstream down")
+		},
+	}
+}
+
+func rpmVirt(members ...string) models.Virtual {
+	return models.Virtual{Name: "v", PackageType: models.PackageRPM, Members: members}
+}
+
+func TestFetchRPMFailsClosed(t *testing.T) {
+	e := fakeEngine(map[string]*RPMMember{
+		"a": {RemoteName: "a", Data: map[string][]byte{"primary": primaryXML(primaryPkgXML("foo", "1", "aaa", "foo.rpm"))}},
+	})
+	_, _, err := e.Fetch(context.Background(), rpmVirt("a", "b"), "repodata/repomd.xml", "")
+	if err == nil || errors.Is(err, ErrNotFound) {
+		t.Fatalf("want upstream error with a member down, got %v", err)
+	}
+}
+
+func TestFetchRPMDataSurvivesMemberChange(t *testing.T) {
+	m := &RPMMember{RemoteName: "a", Data: map[string][]byte{"primary": primaryXML(primaryPkgXML("foo", "1", "aaa", "foo.rpm"))}}
+	e := fakeEngine(map[string]*RPMMember{"a": m})
+	virt := rpmVirt("a")
+
+	repomd, _, err := e.Fetch(context.Background(), virt, "repodata/repomd.xml", "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	m.Data = map[string][]byte{"primary": primaryXML(primaryPkgXML("foo", "2", "bbb", "foo2.rpm"))}
+
+	href := regexp.MustCompile(`repodata/[0-9a-f]+-primary\.xml\.gz`).Find(repomd)
+	body, _, err := e.Fetch(context.Background(), virt, string(href), "")
+	if err != nil {
+		t.Fatalf("data from previous repomd must stay resolvable: %v", err)
+	}
+	if got := string(gunzip(t, body)); !regexp.MustCompile(`ver="1"`).MatchString(got) {
+		t.Fatalf("served wrong primary:\n%s", got)
+	}
+
+	newMD, _, err := e.Fetch(context.Background(), virt, "repodata/repomd.xml", "")
+	if err != nil || string(newMD) == string(repomd) {
+		t.Fatalf("repomd should reflect the member change (err %v)", err)
+	}
+}
+
+func TestMemberRedirect(t *testing.T) {
+	e := fakeEngine(nil)
+	virt := rpmVirt("gh", "other")
+	for _, tc := range []struct {
+		path, query, want string
+		ok                bool
+		err               error
+	}{
+		{path: "gh/o/r/releases/download/v1/c++-1.rpm", query: "x=1", ok: true,
+			want: "http://h/api/v1/remote/gh/o/r/releases/download/v1/c++-1.rpm?x=1"},
+		{path: "gh/a%20b/c%3Fd.rpm", ok: true, want: "http://h/api/v1/remote/gh/a%20b/c%3Fd.rpm"},
+		{path: "gh/../../remote/other/x", err: ErrBadPath},
+		{path: "gh/%2e%2e/%2e%2e/remote/other/x", err: ErrBadPath},
+		{path: "gh/a/./b", err: ErrBadPath},
+		{path: "gh//etc/passwd", err: ErrBadPath},
+		{path: "repodata/repomd.xml"},
+		{path: "nope/x.rpm"},
+	} {
+		got, ok, err := e.MemberRedirect(context.Background(), virt, tc.path, tc.query, "http://h/")
+		if got != tc.want || ok != tc.ok || !errors.Is(err, tc.err) {
+			t.Errorf("%s: got (%q, %v, %v), want (%q, %v, %v)", tc.path, got, ok, err, tc.want, tc.ok, tc.err)
+		}
+	}
+}

internal/virtual/rpm_merger.go

@@ -33,6 +33,7 @@ var locationRe = regexp.MustCompile(`<location\b[^>]*/>`)
 // keyed by repomd data type. Missing types contribute no packages.
 type RPMMember struct {
 	RemoteName string
+	Bases      []string
 	Timestamp  int64
 	Data       map[string][]byte
 }
@@ -161,6 +162,24 @@ func splitPackages(doc []byte) ([][]byte, error) {
 	}
 }
 
+// memberHref returns href relative to the member root. An xml:base under one of
+// the member's upstream bases is folded into the href; any other xml:base is a
+// host the member can't serve, so ok is false and the location is left as is.
+func memberHref(m RPMMember, base, href string) (string, bool) {
+	href = strings.TrimLeft(href, "/")
+	if base == "" {
+		return href, true
+	}
+	base = strings.TrimRight(base, "/") + "/"
+	for _, b := range m.Bases {
+		b = strings.TrimRight(b, "/") + "/"
+		if strings.HasPrefix(base, b) {
+			return strings.TrimPrefix(base, b) + href, true
+		}
+	}
+	return "", false
+}
+
 func xmlEscape(s string) string {
 	var b bytes.Buffer
 	_ = xml.EscapeText(&b, []byte(s))
@@ -196,8 +215,8 @@ func MergeRPM(members []RPMMember) (*RPMRepo, error) {
 			seen[nevra] = true
 			kept[fmt.Sprintf("%d/%s", i, p.Checksum)] = true
 
-			if p.Location.Base == "" {
-				loc := `<location href="` + xmlEscape(m.RemoteName+"/"+strings.TrimLeft(p.Location.Href, "/")) + `"/>`
+			if href, ok := memberHref(m, p.Location.Base, p.Location.Href); ok {
+				loc := `<location href="` + xmlEscape(m.RemoteName+"/"+href) + `"/>`
 				raw = locationRe.ReplaceAll(raw, []byte(loc))
 			}
 			out["primary"] = append(out["primary"], raw)

internal/virtual/rpm_merger_test.go

@@ -145,13 +145,17 @@ func TestMergeRPMOutputIsWellFormed(t *testing.T) {
 }
 
 func TestMergeRPMHrefRewriting(t *testing.T) {
-	based := `<package type="rpm"><name>ext</name><arch>noarch</arch><version epoch="0" ver="1" rel="1"/>` +
-		`<checksum type="sha256" pkgid="YES">eee</checksum><location xml:base="https://mirror.example/" href="Packages/ext.rpm"/></package>`
-	repo, err := MergeRPM([]RPMMember{{RemoteName: "gh", Data: map[string][]byte{
+	based := func(name, base string) string {
+		return `<package type="rpm"><name>` + name + `</name><arch>noarch</arch><version epoch="0" ver="1" rel="1"/>` +
+			`<checksum type="sha256" pkgid="YES">` + name + `</checksum><location xml:base="` + base + `" href="Packages/` + name + `.rpm"/></package>`
+	}
+	repo, err := MergeRPM([]RPMMember{{RemoteName: "gh", Bases: []string{"https://up.example/el9", "https://mirror.example/el9/"}, Data: map[string][]byte{
 		"primary": primaryXML(
 			primaryPkgXML("foo", "1.0", "aaa", "storytold/photocraft/releases/download/v1.0/foo&amp;bar.rpm"),
 			primaryPkgXML("lead", "1.0", "bbb", "/Packages/lead.rpm"),
-			based,
+			based("root", "https://up.example/el9/"),
+			based("sub", "https://mirror.example/el9/extra"),
+			based("ext", "https://other.example/"),
 		),
 	}}})
 	if err != nil {
@@ -161,7 +165,9 @@ func TestMergeRPMHrefRewriting(t *testing.T) {
 	for _, want := range []string{
 		`<location href="gh/storytold/photocraft/releases/download/v1.0/foo&amp;bar.rpm"/>`,
 		`<location href="gh/Packages/lead.rpm"/>`,
-		`<location xml:base="https://mirror.example/" href="Packages/ext.rpm"/>`,
+		`<location href="gh/Packages/root.rpm"/>`,
+		`<location href="gh/extra/Packages/sub.rpm"/>`,
+		`<location xml:base="https://other.example/" href="Packages/ext.rpm"/>`,
 	} {
 		if !strings.Contains(primary, want) {
 			t.Errorf("missing %s in:\n%s", want, primary)