appview: guard the two UI delete paths against cross-repo destruction

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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
This commit is contained in:
Evan Jarrett
2026-08-25 16:34:25 -05:00
co-authored by Claude Opus 5
parent 701c866723
commit 0b212a527f
2 changed files with 115 additions and 2 deletions
+74
View File
@@ -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")
}
}
+41 -2
View File
@@ -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,
})