From 0b212a527fbdd2feaa8eac6fc265444e385782c0 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Mon, 24 Aug 2026 15:10:22 -0500 Subject: [PATCH] appview: guard the two UI delete paths against cross-repo destruction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same defect as the OCI path, two more places. The io.atcr.manifest record is keyed by digest alone, so one record backs every repository of a DID holding identical content, and both of these deleted it without asking whether another repository still wants it — then purged the layers on the hold. DeleteManifestHandler already removes this repository's tags before deleting the record, so a tag remaining at that point can only belong to another repository. It now checks IsManifestTaggedAnyRepo there and keeps the shared record when one does, reporting sharedRecordKept so the caller can tell the difference between "deleted" and "deliberately left alone". DeleteUntaggedManifestsHandler is the subtler one. Its digest list comes from GetAllUntaggedManifestDigests, whose tag join is scoped to one repository (m.repository = t.repository), so a digest tagged only in a DIFFERENT repository is reported as untagged and swept. The query is a reasonable per-repository view and a dangerous delete list; the guard goes at the delete, not in the query, matching how DeleteTagHandler already works. Skips are counted separately from failures, because a skip is the guard working and folding it into "failed" would make a correct run look broken. Both fail closed. Leaving a manifest behind is recoverable; deleting one another repository is still serving is not. The new db test pins both halves of the interaction: that the query really does report a cross-repo-tagged digest as untagged, so a change there is noticed, and that IsManifestTaggedAnyRepo answers DID-wide, which is the thing actually standing between the sweep and another repo's live image. DeleteTagHandler needed no change — 2580dcd already routed it through ShouldCascadeDeleteManifest, which is where the correct policy was written down. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh --- pkg/appview/db/queries_test.go | 74 ++++++++++++++++++++++++++++++++++ pkg/appview/handlers/images.go | 43 +++++++++++++++++++- 2 files changed, 115 insertions(+), 2 deletions(-) diff --git a/pkg/appview/db/queries_test.go b/pkg/appview/db/queries_test.go index bd11630..97fc199 100644 --- a/pkg/appview/db/queries_test.go +++ b/pkg/appview/db/queries_test.go @@ -1,6 +1,7 @@ package db import ( + "slices" "testing" "time" ) @@ -1991,3 +1992,76 @@ func TestGetRepoCards_NullLastPushStillSortsByCreatedAt(t *testing.T) { t.Error("expected 'fresh' LastUpdated to fall back to created_at, got zero time") } } + +// TestGetAllUntaggedManifestDigests_TagInOtherRepoIsNotUntagged pins the +// interaction that makes the delete-untagged path safe. +// +// GetAllUntaggedManifestDigests joins tags scoped to one repository +// (m.repository = t.repository), so a digest tagged only in a DIFFERENT +// repository of the same DID is reported as untagged here. That is fine as a +// per-repository view, and dangerous as a delete list: the io.atcr.manifest +// record is keyed by digest alone, so one record backs every repository holding +// identical content, and deleting on this basis strips the record out from +// under the other repo and purges the shared layers on the hold. +// +// The handler therefore gates each digest on IsManifestTaggedAnyRepo, which is +// DID-wide. Both halves are asserted: the query's repo-scoped answer, so a +// future change to it is noticed, and the guard's DID-wide answer, which is the +// thing actually standing between a sweep and another repo's live image. +func TestGetAllUntaggedManifestDigests_TagInOtherRepoIsNotUntagged(t *testing.T) { + db, err := InitDB(":memory:", LibsqlConfig{}) + if err != nil { + t.Fatalf("Failed to init database: %v", err) + } + defer db.Close() + + did := "did:plc:shared" + shared := "sha256:sharedcontent" + now := time.Now() + manifestType := "application/vnd.oci.image.manifest.v1+json" + + if err := UpsertUser(db, &User{ + DID: did, Handle: "shared.test", + PDSEndpoint: "https://pds.example.com", LastSeen: now, + }); err != nil { + t.Fatalf("Failed to insert user: %v", err) + } + + // Identical content in two repositories — what `crane copy` produces, and + // what any two images built on the same base produce by accident. + for _, repo := range []string{"app-a", "app-b"} { + if _, err := InsertManifest(db, &Manifest{ + DID: did, Repository: repo, Digest: shared, + HoldEndpoint: "did:web:hold.example.com", SchemaVersion: 2, + MediaType: manifestType, CreatedAt: now, + }); err != nil { + t.Fatalf("Failed to insert manifest for %s: %v", repo, err) + } + } + + // Tagged in app-b only. app-a holds the same content, untagged. + if err := UpsertTag(db, &Tag{ + DID: did, Repository: "app-b", Tag: "v1", + Digest: shared, CreatedAt: now, + }); err != nil { + t.Fatalf("Failed to insert tag: %v", err) + } + + untagged, err := GetAllUntaggedManifestDigests(db, did, "app-a") + if err != nil { + t.Fatalf("GetAllUntaggedManifestDigests: %v", err) + } + if !slices.Contains(untagged, shared) { + t.Errorf("expected the repo-scoped view to report %s as untagged in app-a, got %v; "+ + "if this query became DID-wide the handler guard is now redundant, not wrong", shared, untagged) + } + + tagged, err := IsManifestTaggedAnyRepo(db, did, shared) + if err != nil { + t.Fatalf("IsManifestTaggedAnyRepo: %v", err) + } + if !tagged { + t.Error("IsManifestTaggedAnyRepo said the shared digest is untagged while app-b still tags it; " + + "the delete-untagged sweep would remove the record and purge app-b's layers") + } +} diff --git a/pkg/appview/handlers/images.go b/pkg/appview/handlers/images.go index 3e02257..43fccee 100644 --- a/pkg/appview/handlers/images.go +++ b/pkg/appview/handlers/images.go @@ -212,6 +212,24 @@ func (h *DeleteManifestHandler) ServeHTTP(w http.ResponseWriter, r *http.Request holdDID = cached.HoldEndpoint } + // This repository's tags are gone by now, so a remaining tag can only + // belong to another repository. The io.atcr.manifest record is keyed by + // digest alone, so one record backs every repository of this DID holding + // identical content — deleting it here would strip it out from under a repo + // nobody touched and purge the shared layers on the hold. Same guard + // DeleteTagHandler applies via ShouldCascadeDeleteManifest. + if stillTagged, err := db.IsManifestTaggedAnyRepo(h.ReadOnlyDB, user.DID, digest); err != nil { + // Fail closed: leaving a manifest behind is recoverable, deleting one + // another repository is still serving is not. + http.Error(w, fmt.Sprintf("Failed to check cross-repository tags: %v", err), http.StatusInternalServerError) + return + } else if stillTagged { + slog.Info("delete-manifest: digest still tagged in another repository, keeping the shared record", + "did", user.DID, "repo", repo, "digest", digest) + render.JSON(w, r, map[string]any{"success": true, "sharedRecordKept": true}) + return + } + // Compute rkey for manifest record (digest without "sha256:" prefix) rkey := strings.TrimPrefix(digest, "sha256:") @@ -292,7 +310,25 @@ func (h *DeleteUntaggedManifestsHandler) ServeHTTP(w http.ResponseWriter, r *htt } var failures []failure + skipped := 0 for _, digest := range digests { + // GetAllUntaggedManifestDigests scopes its tag join to this repository, + // so a digest tagged in a DIFFERENT repository of the same DID is + // reported as untagged here. The record is keyed by digest alone, so + // deleting on that basis breaks the other repo and purges its layers. + if stillTagged, err := db.IsManifestTaggedAnyRepo(h.ReadOnlyDB, user.DID, digest); err != nil { + // Fail closed for this digest rather than guessing. + slog.Warn("delete-untagged: cross-repository tag check failed, skipping", + "did", user.DID, "repo", req.Repo, "digest", digest, "error", err) + skipped++ + continue + } else if stillTagged { + slog.Info("delete-untagged: digest still tagged in another repository, skipping", + "did", user.DID, "repo", req.Repo, "digest", digest) + skipped++ + continue + } + rkey := strings.TrimPrefix(digest, "sha256:") // Snapshot hold ownership before the delete so we can purge after. @@ -328,8 +364,11 @@ func (h *DeleteUntaggedManifestsHandler) ServeHTTP(w http.ResponseWriter, r *htt } render.JSON(w, r, map[string]any{ - "deleted": deleted, - "failed": len(failures), + "deleted": deleted, + "failed": len(failures), + // Reported separately from failed: a skip is the guard working, not an + // error, and conflating the two would make a correct run look broken. + "skipped": skipped, "total": len(digests), "failures": failures, })