diff --git a/test/e2e/batch11-nested-rkey.mjs b/test/e2e/batch11-nested-rkey.mjs new file mode 100644 index 0000000..00dfd22 --- /dev/null +++ b/test/e2e/batch11-nested-rkey.mjs @@ -0,0 +1,86 @@ +// batch11-nested-rkey.mjs — the drive check for c035f50. +// +// DeleteManifestHandler used to hand-build the tag rkey as "repo:tag" while the +// write path uses RepositoryTagToRKey ("repo_tag", with "/" encoded as "~"). +// For a nested repository like stream/cache the two never match, so the delete +// removed the local cache row while leaving the io.atcr.tag record on the PDS — +// and the tag reappeared on the next backfill. +// +// A flat repo name cannot reproduce this: the bug lives entirely in the "/"→"~" +// encoding. The assertion is therefore against the PDS record, not the page: +// the UI looks correct either way until a backfill runs. +// +// Needs the signed-in profile (lib.mjs PROFILE) because the delete endpoint is +// session-authenticated. Drive it via ctx.request so the cookie jar and UA come +// from the browser — see README, a mismatched UA deletes the session. +import { chromium } from '@playwright/test'; +import { APPVIEW, PROFILE } from './lib.mjs'; + +const HANDLE = process.env.ATCR_E2E_HANDLE ?? 'evan.jarrett.net'; +const DID = process.env.ATCR_E2E_DID ?? 'did:plc:pddp4xt5lgnv2qsegbzzs4xg'; +const PDS = process.env.ATCR_E2E_PDS ?? 'https://jarrett.app'; +const REPO = process.env.ATCR_E2E_NESTED_REPO ?? 'stream/cache'; +const TAG = 'v1'; + +let pass = 0, fail = 0; +const check = (label, ok, detail = '') => { + console.log(` ${ok ? 'ok ' : 'FAIL'} ${label}${detail ? ' — ' + detail : ''}`); + ok ? pass++ : fail++; +}; + +const tagRecords = async () => { + const r = await fetch(`${PDS}/xrpc/com.atproto.repo.listRecords?repo=${DID}&collection=io.atcr.tag&limit=100`); + const d = await r.json(); + return (d.records ?? []).filter((rec) => rec.value?.repository === REPO); +}; + +const before = await tagRecords(); +check(`the nested repo has a tag record to delete`, before.length > 0, + before.map((r) => r.uri.split('/').pop()).join(', ') || 'none — push it first'); +if (before.length === 0) process.exit(1); +// Confirms the write path encodes the slash, which is what the delete path has +// to match. If this ever reads "stream/cache_v1" the encoding changed and the +// rest of this check is meaningless. +check('the rkey is slash-encoded', before[0].uri.split('/').pop().includes('~'), + before[0].uri.split('/').pop()); + +const ctx = await chromium.launchPersistentContext(PROFILE, { + headless: process.env.ATCR_E2E_HEADLESS === '1', + viewport: null, +}); +const page = ctx.pages()[0] ?? (await ctx.newPage()); +// Probe /settings, NOT the repo page: /r/ renders for anonymous visitors, so +// "did not redirect to login" there is true whether or not there is a session, +// and the delete then fails with a confusing 405 (RequireAuth 302s, the client +// follows it, and DELETE /login is 405). +await page.goto(`${APPVIEW}/settings`, { waitUntil: 'networkidle' }); +const signedIn = !/\/login/.test(page.url()); +check('signed in (probed against /settings, which requires auth)', signedIn, page.url()); +if (!signedIn) { + console.error('\n Session is gone. Sign in at ' + APPVIEW + ' in the profile at ' + PROFILE); + await ctx.close(); + process.exit(2); +} +await page.goto(`${APPVIEW}/r/${HANDLE}/${REPO}`, { waitUntil: 'networkidle' }); + +const digest = (await page.content()).match(/sha256:[a-f0-9]{64}/)?.[0]; +check('found the manifest digest on the page', !!digest, digest?.slice(0, 19) + '…'); + +// confirm=true: the manifest is tagged, and without it the handler returns 409 +// asking for confirmation rather than deleting. +const resp = await ctx.request.delete(`${APPVIEW}/api/manifests`, { + data: { repo: REPO, digest, confirm: true }, + maxRedirects: 0, +}); +check('DELETE /api/manifests accepted', resp.ok(), `HTTP ${resp.status()} ${(await resp.text()).slice(0, 120)}`); + +// The PDS is the authority here. The local cache row goes either way; only the +// record surviving tells you the rkey was wrong. +await new Promise((r) => setTimeout(r, 3000)); +const after = await tagRecords(); +check('the io.atcr.tag record is gone from the PDS', after.length === 0, + after.length ? `still present: ${after.map((r) => r.uri.split('/').pop()).join(', ')}` : 'removed'); + +console.log(`\npassed ${pass}, failed ${fail}`); +await ctx.close(); +process.exit(fail === 0 ? 0 : 1); diff --git a/test/integration/manifest_delete_test.go b/test/integration/manifest_delete_test.go index f068362..059b753 100644 --- a/test/integration/manifest_delete_test.go +++ b/test/integration/manifest_delete_test.go @@ -5,6 +5,7 @@ package integration import ( "context" "fmt" + "slices" "testing" "github.com/google/go-containerregistry/pkg/crane" @@ -173,3 +174,61 @@ func TestManifestDelete_SharedDigestAcrossRepos(t *testing.T) { t.Fatalf("repo B is no longer pullable after repo A was deleted: %v", err) } } + +// TestManifestDelete_NestedRepoTagRKey covers the encoding hazard c035f50 fixed +// in the UI handler, on the by-digest path. +// +// io.atcr.tag rkeys come from RepositoryTagToRKey, which encodes "/" as "~", so +// stream/cache:v1 is stored as "stream~cache_v1". Any code that hand-builds the +// rkey ("repo:tag", "repo_tag") targets a record that does not exist, and +// because deleteRecord is idempotent it fails silently: the local view looks +// right and the tag comes back on the next backfill. +// +// A flat repository name cannot reproduce it — the bug is entirely in the "/" +// encoding — which is why every earlier delete test missed it. Listing tags +// after the delete is what catches a surviving record: TagStore.All reads the +// records back from the PDS and filters by repository, so a stale one is still +// reported. +func TestManifestDelete_NestedRepoTagRKey(t *testing.T) { + h := testharness.New(t) + alice := h.AddSailor("alice.test") + creds := h.RegistryCreds(alice) + authOpts := []crane.Option{crane.WithAuth(toAuthn(creds)), crane.Insecure} + + repo := fmt.Sprintf("%s/%s/stream/cache", h.AppViewHostPort(), alice.Handle()) + tagRef := repo + ":v1" + + img, err := random.Image(1<<20, 2) + if err != nil { + t.Fatalf("build image: %v", err) + } + if err := crane.Push(img, tagRef, authOpts...); err != nil { + t.Fatalf("push nested: %v", err) + } + + tags, err := crane.ListTags(repo, authOpts...) + if err != nil { + t.Fatalf("list tags before delete: %v", err) + } + if !slices.Contains(tags, "v1") { + t.Fatalf("fixture is wrong: v1 not listed before delete, got %v", tags) + } + + dgst, err := img.Digest() + if err != nil { + t.Fatalf("digest: %v", err) + } + if err := crane.Delete(fmt.Sprintf("%s@%s", repo, dgst.String()), authOpts...); err != nil { + t.Fatalf("delete nested by digest: %v", err) + } + + tags, err = crane.ListTags(repo, authOpts...) + if err != nil { + t.Fatalf("list tags after delete: %v", err) + } + if slices.Contains(tags, "v1") { + t.Errorf("tag v1 survived the delete on a nested repository: %v — the tag rkey did not match "+ + "the slash-encoded key the write path uses, so the record is still on the PDS and will "+ + "reappear on the next backfill", tags) + } +}