appview: stop the upgrade banner inventing an improvement across arch mismatch

On a public repo, to anonymous visitors, the digest banner read "2 fixes, 12 Critical / 27 High / 26 Medium vulns, -22 layers, -53.6 MB" while the same page showed 244 vulnerabilities and Layers (22), and its own "View diff" link landed on "Layers 22 -> 22, every layer Unchanged". Platform matching only ran when both sides were manifest lists. The comment after that block said the mismatched case would "fall through and show a basic banner without layer/vuln details", but no such branch was ever written and nothing guarded the fallthrough, so execution continued into the layer and vuln computation with the unresolved originals still in place. The newer side was the multi-arch index, which carries no layers of its own and is not scanned, so all 22 layers of the other side read as removed and the vuln delta was computed against an absent scan. Returns 204 for either mismatch direction, as the no-common-platform path already does. The promised "basic banner" is not implementable as the function stands, which is presumably why it never appeared: the template renders only NewerTag, DiffURL and Summary, and DiffSummary is nothing but deltas, so a delta-less banner collapses to the tag name and would be suppressed by the existing "nothing meaningful changed" guard anyway. The comment is replaced with one that says what is actually true. Showing a real banner here would mean resolving the index to the child matching the single-arch side's platform, and that side's os/arch is not in the appview DB at all: Platforms is populated only for manifest lists and the manifests table has no os/arch columns. It would need either a config fetch from the hold at render time or denormalising os/arch during ingest. Not attempted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PDqoCE1j3njokkZ9b1C5n9
2026-09-10 04:06:06 +00:00 · 2026-09-02 21:37:38 -05:00
parent 39919cc832
commit 44a17cbcdc
2 changed files with 296 additions and 2 deletions
@@ -129,9 +129,21 @@ func (h *UpgradeBannerHandler) ServeHTTP(w http.ResponseWriter, r *http.Request)
 			w.WriteHeader(http.StatusNoContent)
 			return
 		}
+	} else if currentManifest.IsManifestList != newerManifest.IsManifestList {
+		// One side is a multi-arch index, the other a single-arch image. The two
+		// cannot be compared without resolving the index down to the child
+		// manifest matching the single-arch side's platform, and that is not
+		// implemented. Diffing the index itself is not a fallback: an index
+		// carries no layers and is not scanned, so every layer of the other side
+		// reads as removed and the vuln delta is computed against an absent scan,
+		// which invents a security improvement that did not happen. Show no
+		// banner rather than a wrong one.
+		slog.Debug("Upgrade banner: multi-arch/single-arch mismatch, cannot compare",
+			"currentIsManifestList", currentManifest.IsManifestList,
+			"newerIsManifestList", newerManifest.IsManifestList)
+		w.WriteHeader(http.StatusNoContent)
+		return
 	}
-	// If one is multi-arch and the other isn't, we can't match platforms —
-	// fall through and show a basic banner without layer/vuln details.

 	// Fetch layers for both
 	currentDBLayers, _ := db.GetLayersForManifest(h.ReadOnlyDB, currentManifestForLayers.Key)
@@ -0,0 +1,282 @@
+package handlers
+
+import (
+	"context"
+	"database/sql"
+	"fmt"
+	"html/template"
+	"net/http"
+	"net/http/httptest"
+	"path/filepath"
+	"testing"
+	"time"
+
+	"atcr.io/pkg/appview/db"
+	"atcr.io/pkg/atproto"
+	"github.com/bluesky-social/indigo/atproto/identity"
+	"github.com/bluesky-social/indigo/atproto/syntax"
+	"github.com/go-chi/chi/v5"
+	_ "github.com/tursodatabase/go-libsql"
+)
+
+const (
+	upgradeTestDID    = "did:plc:upgradebanner123"
+	upgradeTestHandle = "upgrade.test"
+	upgradeTestRepo   = "app"
+
+	ociIndexMediaType    = "application/vnd.oci.image.index.v1+json"
+	ociManifestMediaType = "application/vnd.oci.image.manifest.v1+json"
+)
+
+// upgradeStubDirectory resolves only the single DID used by these tests, so the
+// handler's ResolveIdentity call never touches the network.
+type upgradeStubDirectory struct{}
+
+func (d *upgradeStubDirectory) LookupDID(_ context.Context, did syntax.DID) (*identity.Identity, error) {
+	if did.String() != upgradeTestDID {
+		return nil, fmt.Errorf("%w: %s", identity.ErrDIDNotFound, did)
+	}
+	return &identity.Identity{
+		DID:    syntax.DID(upgradeTestDID),
+		Handle: syntax.Handle(upgradeTestHandle),
+		Services: map[string]identity.ServiceEndpoint{
+			"atproto_pds": {Type: "AtprotoPersonalDataServer", URL: "https://pds.example.com"},
+		},
+	}, nil
+}
+
+func (d *upgradeStubDirectory) LookupHandle(_ context.Context, handle syntax.Handle) (*identity.Identity, error) {
+	return nil, fmt.Errorf("%w: %s", identity.ErrHandleNotFound, handle)
+}
+
+func (d *upgradeStubDirectory) Lookup(ctx context.Context, atid syntax.AtIdentifier) (*identity.Identity, error) {
+	if did, err := atid.AsDID(); err == nil {
+		return d.LookupDID(ctx, did)
+	}
+	return nil, fmt.Errorf("%w: %s", identity.ErrHandleResolutionFailed, atid)
+}
+
+func (d *upgradeStubDirectory) Purge(_ context.Context, _ syntax.AtIdentifier) error { return nil }
+
+// newUpgradeBannerDB returns a file-backed database with the real schema plus
+// the owning user. A file rather than ":memory:" because an in-memory libsql
+// database is per-connection.
+func newUpgradeBannerDB(t *testing.T) *sql.DB {
+	t.Helper()
+	database, err := db.InitDB(filepath.Join(t.TempDir(), "appview.db"), db.LibsqlConfig{})
+	if err != nil {
+		t.Fatalf("InitDB: %v", err)
+	}
+	t.Cleanup(func() { _ = database.Close() })
+
+	if err := db.UpsertUser(database, &db.User{
+		DID:         upgradeTestDID,
+		Handle:      upgradeTestHandle,
+		PDSEndpoint: "https://pds.example.com",
+		LastSeen:    time.Now(),
+	}); err != nil {
+		t.Fatalf("UpsertUser: %v", err)
+	}
+	return database
+}
+
+// insertUpgradeManifest writes a manifest row and returns its manifest key.
+func insertUpgradeManifest(t *testing.T, database *sql.DB, digest, mediaType string, createdAt time.Time) string {
+	t.Helper()
+	key := "key-" + digest
+	_, err := database.Exec(`
+		INSERT INTO manifests (manifest_key, did, repository, digest, hold_endpoint,
+			schema_version, media_type, artifact_type, created_at)
+		VALUES (?, ?, ?, ?, '', 2, ?, 'container-image', ?)
+	`, key, upgradeTestDID, upgradeTestRepo, digest, mediaType, createdAt)
+	if err != nil {
+		t.Fatalf("insert manifest %s: %v", digest, err)
+	}
+	return key
+}
+
+func insertUpgradeLayers(t *testing.T, database *sql.DB, manifestKey string, sizes ...int64) {
+	t.Helper()
+	for i, size := range sizes {
+		_, err := database.Exec(`
+			INSERT INTO layers (manifest_key, digest, size, media_type, layer_index)
+			VALUES (?, ?, ?, 'application/vnd.oci.image.layer.v1.tar+gzip', ?)
+		`, manifestKey, fmt.Sprintf("sha256:%s-layer%d", manifestKey, i), size, i)
+		if err != nil {
+			t.Fatalf("insert layer: %v", err)
+		}
+	}
+}
+
+// insertUpgradeRef points a manifest list at a linux/amd64 child.
+func insertUpgradeRef(t *testing.T, database *sql.DB, indexKey, childDigest string) {
+	t.Helper()
+	_, err := database.Exec(`
+		INSERT INTO manifest_references (manifest_key, digest, media_type, size,
+			platform_architecture, platform_os, platform_variant, is_attestation, reference_index)
+		VALUES (?, ?, ?, 100, 'amd64', 'linux', '', 0, 0)
+	`, indexKey, childDigest, ociManifestMediaType)
+	if err != nil {
+		t.Fatalf("insert manifest reference: %v", err)
+	}
+}
+
+func insertUpgradeTag(t *testing.T, database *sql.DB, tag, digest string, createdAt time.Time) {
+	t.Helper()
+	_, err := database.Exec(`
+		INSERT INTO tags (did, repository, tag, digest, created_at) VALUES (?, ?, ?, ?, ?)
+	`, upgradeTestDID, upgradeTestRepo, tag, digest, createdAt)
+	if err != nil {
+		t.Fatalf("insert tag: %v", err)
+	}
+}
+
+// serveUpgradeBanner runs the handler against the given database for a page
+// showing currentDigest, and returns the recorder.
+func serveUpgradeBanner(t *testing.T, database *sql.DB, currentDigest string) *httptest.ResponseRecorder {
+	t.Helper()
+
+	atproto.SetDirectory(&upgradeStubDirectory{})
+	t.Cleanup(func() { atproto.SetDirectory(nil) })
+
+	tmpl := template.Must(template.New("test").Parse(
+		`{{ define "upgrade-banner" }}BANNER {{ .NewerTag }} layers {{ .Summary.LayerCountFrom }}->{{ .Summary.LayerCountTo }}{{ end }}`))
+
+	h := &UpgradeBannerHandler{BaseUIHandler: BaseUIHandler{
+		Templates:  tmpl,
+		DB:         database,
+		ReadOnlyDB: database,
+	}}
+
+	req := httptest.NewRequest(http.MethodGet, "/upgrade-banner?digest="+currentDigest, nil)
+	rctx := chi.NewRouteContext()
+	rctx.URLParams.Add("handle", upgradeTestDID)
+	rctx.URLParams.Add("*", "/"+upgradeTestRepo)
+	req = req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx))
+
+	rr := httptest.NewRecorder()
+	h.ServeHTTP(rr, req)
+	return rr
+}
+
+// bothMultiArch builds an old and a new manifest list that share linux/amd64,
+// with differing layer counts on the children. Returns the current digest.
+func bothMultiArch(t *testing.T, database *sql.DB) string {
+	t.Helper()
+	old := time.Now().Add(-2 * time.Hour)
+	recent := time.Now().Add(-1 * time.Hour)
+
+	oldChild := insertUpgradeManifest(t, database, "sha256:oldchild", ociManifestMediaType, old)
+	insertUpgradeLayers(t, database, oldChild, 100, 200)
+	oldIndex := insertUpgradeManifest(t, database, "sha256:oldindex", ociIndexMediaType, old)
+	insertUpgradeRef(t, database, oldIndex, "sha256:oldchild")
+	insertUpgradeTag(t, database, "v1", "sha256:oldindex", old)
+
+	newChild := insertUpgradeManifest(t, database, "sha256:newchild", ociManifestMediaType, recent)
+	insertUpgradeLayers(t, database, newChild, 100)
+	newIndex := insertUpgradeManifest(t, database, "sha256:newindex", ociIndexMediaType, recent)
+	insertUpgradeRef(t, database, newIndex, "sha256:newchild")
+	insertUpgradeTag(t, database, "v2", "sha256:newindex", recent)
+
+	return "sha256:oldindex"
+}
+
+// bothSingleArch builds an old and a new single-arch image with differing layer
+// counts. Returns the current digest.
+func bothSingleArch(t *testing.T, database *sql.DB) string {
+	t.Helper()
+	old := time.Now().Add(-2 * time.Hour)
+	recent := time.Now().Add(-1 * time.Hour)
+
+	oldKey := insertUpgradeManifest(t, database, "sha256:oldimage", ociManifestMediaType, old)
+	insertUpgradeLayers(t, database, oldKey, 100, 200)
+	insertUpgradeTag(t, database, "v1", "sha256:oldimage", old)
+
+	newKey := insertUpgradeManifest(t, database, "sha256:newimage", ociManifestMediaType, recent)
+	insertUpgradeLayers(t, database, newKey, 100)
+	insertUpgradeTag(t, database, "v2", "sha256:newimage", recent)
+
+	return "sha256:oldimage"
+}
+
+func TestUpgradeBanner_BothMultiArchCommonPlatform_Renders(t *testing.T) {
+	database := newUpgradeBannerDB(t)
+	current := bothMultiArch(t, database)
+
+	rr := serveUpgradeBanner(t, database, current)
+
+	if rr.Code != http.StatusOK {
+		t.Fatalf("expected 200, got %d (body %q)", rr.Code, rr.Body.String())
+	}
+	// The children were resolved, so the deltas describe the children's layers
+	// (2 -> 1), not the layer-less indexes.
+	if got, want := rr.Body.String(), "BANNER v2 layers 2->1"; got != want {
+		t.Errorf("expected body %q, got %q", want, got)
+	}
+}
+
+func TestUpgradeBanner_BothSingleArch_Renders(t *testing.T) {
+	database := newUpgradeBannerDB(t)
+	current := bothSingleArch(t, database)
+
+	rr := serveUpgradeBanner(t, database, current)
+
+	if rr.Code != http.StatusOK {
+		t.Fatalf("expected 200, got %d (body %q)", rr.Code, rr.Body.String())
+	}
+	if got, want := rr.Body.String(), "BANNER v2 layers 2->1"; got != want {
+		t.Errorf("expected body %q, got %q", want, got)
+	}
+}
+
+// A single-arch image viewed while the newest tag is a multi-arch index. The
+// index has no layers and no scan of its own, so comparing against it directly
+// reports all of the current image's layers as removed. No banner is correct.
+func TestUpgradeBanner_SingleArchCurrentMultiArchNewer_NoContent(t *testing.T) {
+	database := newUpgradeBannerDB(t)
+	old := time.Now().Add(-2 * time.Hour)
+	recent := time.Now().Add(-1 * time.Hour)
+
+	oldKey := insertUpgradeManifest(t, database, "sha256:oldimage", ociManifestMediaType, old)
+	insertUpgradeLayers(t, database, oldKey, 100, 200, 300)
+	insertUpgradeTag(t, database, "v1", "sha256:oldimage", old)
+
+	newChild := insertUpgradeManifest(t, database, "sha256:newchild", ociManifestMediaType, recent)
+	insertUpgradeLayers(t, database, newChild, 100, 200, 300)
+	newIndex := insertUpgradeManifest(t, database, "sha256:newindex", ociIndexMediaType, recent)
+	insertUpgradeRef(t, database, newIndex, "sha256:newchild")
+	insertUpgradeTag(t, database, "v2", "sha256:newindex", recent)
+
+	rr := serveUpgradeBanner(t, database, "sha256:oldimage")
+
+	if rr.Code != http.StatusNoContent {
+		t.Fatalf("expected 204 for single-arch current vs multi-arch newer, got %d (body %q)",
+			rr.Code, rr.Body.String())
+	}
+}
+
+// The mirror image: a multi-arch index viewed while the newest tag is a
+// single-arch image. The index contributes no layers, so every layer of the
+// newer image reads as added.
+func TestUpgradeBanner_MultiArchCurrentSingleArchNewer_NoContent(t *testing.T) {
+	database := newUpgradeBannerDB(t)
+	old := time.Now().Add(-2 * time.Hour)
+	recent := time.Now().Add(-1 * time.Hour)
+
+	oldChild := insertUpgradeManifest(t, database, "sha256:oldchild", ociManifestMediaType, old)
+	insertUpgradeLayers(t, database, oldChild, 100, 200, 300)
+	oldIndex := insertUpgradeManifest(t, database, "sha256:oldindex", ociIndexMediaType, old)
+	insertUpgradeRef(t, database, oldIndex, "sha256:oldchild")
+	insertUpgradeTag(t, database, "v1", "sha256:oldindex", old)
+
+	newKey := insertUpgradeManifest(t, database, "sha256:newimage", ociManifestMediaType, recent)
+	insertUpgradeLayers(t, database, newKey, 100, 200, 300)
+	insertUpgradeTag(t, database, "v2", "sha256:newimage", recent)
+
+	rr := serveUpgradeBanner(t, database, "sha256:oldindex")
+
+	if rr.Code != http.StatusNoContent {
+		t.Fatalf("expected 204 for multi-arch current vs single-arch newer, got %d (body %q)",
+			rr.Code, rr.Body.String())
+	}
+}