Commit Graph
9 Commits
Author SHA1 Message Date
Evan JarrettandClaude Opus 5 b17ebb69a5 test: cover the nested-repo tag rkey on the delete paths
c035f50 fixed a hand-built tag rkey in DeleteManifestHandler and shipped with
no test. The hazard is not specific to that handler: io.atcr.tag rkeys come
from RepositoryTagToRKey, which encodes "/" as "~", so any code building one by
hand targets a record that does not exist — and deleteRecord being idempotent
makes that a silent no-op. The local view looks right and the tag returns on
the next backfill.

The by-digest path now builds tag rkeys too (594d73b), so it could reintroduce
exactly this bug. TestManifestDelete_NestedRepoTagRKey pins it there: push to
stream/cache, delete by digest, and assert the tag is no longer listed.
Listing is what catches a survivor — TagStore.All reads the records back from
the PDS and filters by repository, so a stale one is still reported.

Mutation-verified by hand-building the rkey as "repo:tag": the nested test
fails with the tag still listed, and TestManifestDelete passes unchanged. That
second half is the point — every existing delete test uses a flat repository
name, and a flat name cannot reproduce this bug at all.

batch11-nested-rkey.mjs drives the same property through the UI handler that
c035f50 actually fixed, asserting against the PDS record rather than the page,
since the page looks correct either way until a backfill runs. It needs an
interactive appview login in the Playwright profile and is not yet run; the
session that exists belongs to a different browser profile. Two instrument
notes are baked in: probe /settings rather than the repo page to detect a
session, because /r/ renders for anonymous visitors and can never report a
missing one, and use maxRedirects:0, because RequireAuth 302s and a followed
redirect surfaces as a confusing 405 on DELETE /login.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 701c866723 appview: stop an OCI manifest delete destroying another repo's image
The io.atcr.manifest record is keyed by digest alone (digestToRKey), so one
record backs every repository of a DID holding identical content.
ManifestStore.Delete removed it unconditionally and then called
purgeDeletedManifest, which asks the hold to drop the layer records and free
the blobs. Deleting through one repository therefore stripped the manifest out
from under every other repository sharing that digest and took their bytes with
it — reachable from the public OCI API with `crane delete`, and not recoverable.

Reproduced end to end before fixing: push identical content to shared-a and
shared-b, delete shared-a by digest, and shared-b:v1 answers 404.

The codebase already had the right guard and the right policy written down.
2580dcd added cleanupUntaggedManifest for exactly this hazard, and its comment
says it plainly — the check is "deliberately not filtered to rctx.Repository"
because "a tag in any of them keeps it alive". TagStore.Untag routes through it.
The by-digest path never did.

Delete now makes one pass over the DID's tag records, which answers both
questions at once: which tags in THIS repository point at the digest, and
whether any other repository still does. This repository's tags are removed
either way, because a DELETE scoped to a repository has to stop that repository
serving the image; the shared record and the hold purge only happen when
nothing else tags it. Enumeration failure is returned as an error rather than
swallowed, so the caller fails closed — leaving a manifest behind is
recoverable, deleting a live one is not, and the page budget exists for the
same reason cleanupUntaggedManifest has one.

Covered twice on purpose. The integration test proves the user-visible property
(repo B still pulls). The unit test asserts the thing an end-to-end pull can
only infer: that no deleteRecord for the manifest collection is issued at all.
Both fail against the pre-fix code.

TestManifestStore_Delete needed updating rather than fixing: its fake server
asserted every request was a deleteRecord, which the new tag-listing call
breaks. It now serves an empty tag list and additionally asserts the manifest
delete still happens, so the unshared path stays pinned.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 43bf79c71f test/integration: make every pull client actually read a blob
Two of the three matrix clients never fetched one. orasClient.Pull stopped at
repo.Resolve (a HEAD on the manifest) and regclient's stopped at ManifestHead,
so their pull rows — including anonymous_pull_denied, stranger_pull and
crew_read_only_pull on the private-hold matrix — were passing without ever
exercising blob authorization. They passed on the manifest denial alone.

That is invisible from the outside because it produces the right verdicts for
the wrong reason. The full suite is still green after the fix, so no
authorization bug was hiding behind it; what was hiding was the coverage.

craneClient.Pull was already correct: 5aa13ab added its layer-materialization
loop precisely because crane is lazy and ATCR serves manifests from the user's
PDS, where they are world-readable by design. The oras comment still carried
the pre-5aa13ab rationale — "We don't need to fetch blobs; that mirrors
crane.Pull followed by .Digest(), which is also manifest-only" — which that
commit had already invalidated. Both clients now match crane, and the stale
comment is gone.

TestPullClientsReadBlobs guards all three against regressing to manifest-only.
It runs each client against a registry that serves the manifest happily and
403s the layer, and asserts both that a blob was requested and that the refusal
surfaces as an error. Written first and run before the fix, where it passed for
crane and failed for oras and regclient — which is how the gap was found.

This also closes the hole the plan flagged for craneClient.Pull alone: nothing
protected that loop, and deleting it left the entire suite green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 4542897f08 hold/gc: escape the DID in listRecords/getRecord repo parameters
fetchUserManifestsFromEndpoint, fetchUserTags and fetchUserProfile
interpolated the user DID straight into a query string. A did:web carrying a
port spells that port as a literal %3A, so the PDS received the parameter
decoded back to ":" — a different DID, matching no repo. listRecords then
answers 200 with an empty list and GC reads it as "this user has no
manifests": every blob they own drops out of the referenced set and is
deleted once past the seven-day blob grace.

There is no error and no status code to notice, which is the same soft
failure 95d4f7c fixed one function away in this file. getRecord for
manifests and checkPredecessorAt already escaped; three of the five call
sites did not.

Scope is local testing only. did:plc, which every production user has,
contains nothing that needs escaping, and did:web with a port is not really
valid in atproto — but it is what the dev stack runs on, so GC there sees a
referenced set of zero and considers every blob in the bucket collectable.
That made it impossible to validate the sweep end to end, which is how it
surfaced.

Found by test/integration/gc_test.go, added here: it pushes an image and
asserts GC accounts for every blob the push wrote. The blob grace period is
a package constant, so nothing pushed during a test can age past it and "no
orphans" is vacuous; the load-bearing assertion is referenced == total,
which grace does not touch. Against the unescaped code it reported
referenced=0 of 4.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-25 16:34:25 -05:00
Evan JarrettandClaude Opus 5 5aa13abdc2 auth: make anonymous pull work, and let the hold decide it
a7569a7 added credential-less pulls of public images. Three things about it
were wrong, all of them in how the appview handled the decision that belongs
to the hold.

**Scope handling was all-or-nothing.** IsPullOnlyScope required every
requested action to already be "pull", but clients routinely ask for more
than the operation needs — pull,push is common for a plain read, and some
ask for pull,push,delete up front. Those were rejected and challenged,
leaving a credential-less client no way to pull even a public image, which
is the entire feature. NarrowToPullOnly drops the write actions and issues a
token carrying "pull" and nothing else. Granting a subset is what the
distribution token spec expects. The allowlist property is preserved: "pull"
is the only action that survives, and "*" is deliberately not expanded into
it, since a wildcard request is not evidence the caller wants a read.

**The appview-side read gate was inert.** checkReadAccess passed p.ctx.DID,
the DID of the repository *owner*, not the requester. Any non-empty DID
satisfies a private hold's check, and the owner's is never empty, so it asked
"may the owner read their own hold", answered yes, and admitted everyone.
Worse, CheckReadAccessWithCaptain admitted any authenticated DID to a private
hold at all, on an explicitly-MVP assumption that holding a DID was close
enough to being a sailor. Every doc says otherwise (docs/hold.md:109 "Crew
with blob:read", CLAUDE.md:140, docs/BYOS.md:280) and so does the hold
(ValidateBlobReadAccess: owner, or crew carrying blob:read/blob:write). It
now takes isCrew and requires owner-or-crew, and callers only pay for the
crew lookup when it can change the answer — a public hold or an anonymous
caller is decided by the captain record alone. Nothing here loosens access;
it brings the local gate into agreement with the authority.

**Denials could not reach the client.** distribution's blobHandler.GetBlob
maps everything except ErrBlobUnknown to ErrorCodeUnknown, so a 401 raised
in the blob store left as a 500 — misreporting an auth failure as a server
fault, and giving BearerChallenge no 401 to attach WWW-Authenticate to, so
Docker was told "server error" instead of being prompted for credentials.
Clients that retry 5xx looped: 4.1s per case in the matrix, now 0.01s. The
check moves to Repository(), where an errcode.Error is passed through
verbatim by the registry app — the same mechanism a7569a7 used for
NAME_UNKNOWN. It fails open on a lookup error, since the hold is the
authority and a transient failure should not break public pulls.

Removes auth.allow_anonymous_pull. It could only ever withhold — captain.Public
is what grants — so it was a second flag for a decision the hold already owns,
and gating it appview-side was never the intent. Layer bytes 307 straight to
S3, so the appview is not even in the path whose cost might have justified an
operator-side lever.

Tests: TestAuthMatrix only ever ran against a public hold, and its pull cases
never fetched a layer — crane.Pull is lazy and img.Digest() needs only the
manifest, which ATCR serves from the user's PDS where it is world-readable, so
no pull row in the matrix touched blob authorization at all. Pulls now
materialize layer bytes, and testharness.WithPrivateHold plus
TestAuthMatrixPrivateHold cover public:false + allow_all_crew:true — the
production shape, where anyone with an account pulls and pushes and anonymous
gets nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-10 21:43:32 -05:00
Evan JarrettandClaude Opus 5 a7569a7717 registry: allow anonymous pull of public images
Credential-less pulls of public images. /auth/token issues a pull-only
token with an empty subject when no Basic auth is present; the
destination hold still enforces captain.Public, and push or delete always
challenges.

  - token.IsPullOnlyScope and AuthMethodAnonymous;
    Handler.issueAnonymousToken skips the authorizer gate and the
    service-auth pre-mint, since there is no identity to reconcile and no
    AppView-to-hold service token to bind. The token is still stamped
    with the resolved registry domain, so anonymous pull works on
    secondary front doors whose access controller demands their own
    audience.
  - auth.allow_anonymous_pull (default true) turns it fully off, restoring
    the previous always-challenge behavior. Mirrored into the deploy
    template, since the default means existing deploys pick this up.
  - RegistryContext.Anonymous is plumbed from the middleware.
  - ProxyBlobStore sends no Authorization header when the service token is
    empty, and returns 401 rather than 403 for anonymous denials so Docker
    prompts for credentials, including when a stale captain cache lets the
    request through and the hold says private.
  - BearerChallenge wraps the /v2/ subtree so a 401 raised deep in the
    stack via errcode.ServeJSON still carries WWW-Authenticate.
    Distribution's own scoped challenges are left alone.

IsPullOnlyScope allowlists the pull action instead of denylisting push and
delete. Distribution's actionSet.contains treats "*" as *every* action, so
a scope of `repository:victim/img:*` names neither denied string and would
have handed an unauthenticated caller a token valid for push and delete on
someone else's repository — clearing the authgate entirely, since anonymous
tokens deliberately skip it. Writes would still have failed further down
(no PDS credential), but the gate itself was bypassable. Now every
requested action must be exactly "pull". Covered by new claims tests.

Unresolvable identities return NAME_UNKNOWN instead of a bare error that
distribution renders as 500. This path was previously unreachable without
credentials; anonymous pull opens it to the internet, and a 5xx on
arbitrary input both misreports a bad request as a server fault and sends
clients that retry 5xx into a retry loop. That loop was real: in the auth
matrix, regclient spent 83s on a single case before this fix, and the
suite now runs in 5s.

Stat preserves an authorization verdict from getPresignedURL rather than
flattening it to ErrBlobUnknown. Distribution calls Stat before ServeBlob
on GET and HEAD, so without this an anonymous pull from a private hold
answered 404 and BearerChallenge had no 401 to annotate — the 401 path
above could never actually reach a client.

The auth matrix is updated to match: anonymous pull of the seeded public
repo now succeeds, anonymous push is denied against a real identity's
namespace (rather than an unresolvable one, which was testing name
resolution rather than authorization), and a new case pins the
NAME_UNKNOWN behavior for an unknown identity.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-09 21:14:58 -05:00
Evan JarrettandClaude Opus 5 1b917686b2 appview: support OCI manifest DELETE
DELETE /v2/<name>/manifests/<ref> answered UNSUPPORTED before ever
reaching the ATProto-backed stores: distribution v3.1.1's DeleteManifest
handler short-circuits unless app.deleteEnabled is set, which comes from
storage.delete.enabled. Set it (mirrored in the test harness). The
companion storage.EnableDelete option it appends only affects
distribution's built-in store, which RoutingRepository replaces, so it is
a no-op for us.

With the route reachable, make the stores do the right thing:

  - ManifestStore.Delete purges the hold's per-layer, scan and image
    config records on a detached context, since the DELETE handler
    returns immediately and cancels the request context.
  - TagStore.Untag resolves the digest before deleting the tag record,
    then deletes the manifest if that was its last tag and it is not a
    manifest list child, matching the web UI's delete-tag behavior so
    deleting an only-tagged image doesn't orphan the manifest.
  - cleanupUntaggedManifest becomes package-level over *RegistryContext
    so both stores share one implementation.
  - purgeOnHold moves out of handlers into pkg/appview/holdpurge so the
    storage layer can call it: handlers already depends on storage via
    middleware, so storage to handlers would be an import cycle.
  - ProxyBlobStore.Delete returns distribution.ErrUnsupported, so the
    always-registered blob DELETE route gives a clean OCI UNSUPPORTED
    error instead of a generic 500. Layer bytes are reclaimed by the
    hold's refcounted GC.

The cascade's still-tagged re-check pages through the tag records rather
than reading a single capped page. Tags for all of a user's repositories
share one collection, so one page is a per-account budget: past ~100 tags
a live tag fell off the end and the manifest was deleted while still
referenced. Incomplete enumeration now skips the delete, since an
orphaned manifest is recoverable and a deleted live one is not.

This also makes the over-quota delete grant added in 6e426dc load-bearing:
it hands out pull,delete tokens, which could not do anything while
distribution rejected every DELETE.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-09 20:54:50 -05:00
Evan Jarrett be64dbb364 add more integration tests 2026-05-11 19:53:13 -05:00
Evan Jarrett a0cc862798 lots of new work for authenticating between appview -> hold. fixed quota handling, improve integration tests for round-trip push/pull/auth/quota checking 2026-05-11 09:20:55 -05:00