Three scripts covering what only a browser can see, run end to end against the
sandbox with a real checkout and a real portal cancellation.
batch13-billing-drive.mjs runs the same account either side of one config
change -- whether its default hold appears in server.managed_holds. Managed:
the billing tab offers real tiers, checkout 302s to Stripe, the portal is
reachable. Self-hosted: checkout 403s, the portal still 302s, and the advisor
answers managed_hold_required rather than upgrade_required, which matters
because telling a paying subscriber to "upgrade" would sell them a tier they
already hold.
batch13-portal-cancel.mjs walks into Stripe's portal instead of asserting the
redirect, because the batch card calls a subscriber who cannot cancel the worst
outcome here and a 302 does not prove a cancel control exists at the far end.
batch13-webhook-downgrade.mjs creates three webhooks under an allowance of ten,
then reads the page back after the downgrade.
Every one of these is invisible on a hold owner's account: GetSubscriptionInfo
returns a synthetic "Captain" tier before any Stripe lookup, and
GetWebhookLimits / HasAIAdvisor / GetSupporterBadge bypass on the same first
line. The first run used the shared e2e profile, which still had the owner
signed in, and reported a clean pass built entirely on that bypass. The scripts
now assert the page does not render "Captain", and take a separate profile.
Two traps worth keeping: a bare button[type=submit] matches the nav's hidden
logout button before the form's own submit, and hx-confirm here renders a
custom modal whose backdrop swallows clicks -- strip the attribute rather than
trying to dismiss a dialog that never fires.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwxF2N3HuZ8xSkx6nkirgB
The suite built its Manager with a nil database, which switches off every
`m.db != nil` branch in HandleWebhook: the idempotency check, the per-customer
ordering guard, and the processed-event record. It ran against real Stripe and
exercised none of the code those guards live in, so it read as far broader
coverage than it was.
It now opens a libsql file under t.TempDir (":memory:" is per-connection, so
the pool's second connection would see no tables), and two new tests cover the
branches that were dead: a redelivery is skipped, and a stale out-of-order
event is ignored. Both assert that the second delivery did not reach a handler
rather than counting rows -- RecordStripeEvent is an idempotent upsert, so
deleting either guard leaves the table identical. Both were mutation-verified
against the guard they cover.
Two fixes fall out of turning the database on:
buildEventPayload never set `created`, which unmarshals as 0. Harmless with no
database; with one, the ordering guard reads every later event for a customer
as older than what it already applied, so the second event silently becomes a
no-op. It is stamped now, with buildEventPayloadAt for the ordering test.
TestHandleWebhookAllSubscribedEvents was failing on this branch and nothing
caught it, because stripe-integration-test is not part of `make test`. It
posted events for the literal "cus_fake" and expected "No user DID found" --
which was true only while a failed customer.Get collapsed to an empty DID.
Since that became a retryable error, the fixture reached the error branch
instead. It now uses a real sandbox customer carrying no user_did, so the
assertion tests the branch it names.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VwxF2N3HuZ8xSkx6nkirgB
It can, and normally in well under a second. Two launches hung for the full
180s handshake timeout under heavy concurrent docker and test load, and I wrote
that up as "headed is impossible from an agent shell" and moved everything to
headless. That was wrong, and wrong in a way that would have quietly degraded
every future browser check.
The display is reachable: DISPLAY=:0, XAUTHORITY set to the mutter XWayland
cookie, both the Wayland socket and /tmp/.X11-unix/X0 present, xdpyinfo happy.
The README now says to check xdpyinfo and retry rather than conclude anything,
and batch10-anonpull.mjs defaults to headed again.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
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
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
Five things that each produced a confident wrong answer during val/10-anonpull,
so the next batch does not rediscover them: the repo page route, tags living in
a <select>, headed Chromium not launching from an agent shell, logged-out
checks needing their own browser profile, and the dev hold defaulting to
public:false with a propagation delay after the flip.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
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
The API half of val/10-anonpull is covered by the auth matrix. This covers what
Go tests structurally cannot see: whether a logged-out repo page renders, 500s,
or comes back as a blank panel.
Public hold, logged out: 200, all four tags render, and the digest on the page
matches what the registry serves for the selected tag. That last check compares
against whichever tag is selected rather than a hardcoded one, because the page
renders the selected tag's digest and a hardcoded comparison silently fails
whenever the default changes.
Three instrument bugs are baked into the script as comments, because each of
them produced a confident wrong answer first:
* The repo page is /r/{handle}/*, not /{handle}/{repo}. The latter is a 404
"Lost at Sea" page, which reads exactly like a denial if you don't check.
* Tags are <option>s in a <select>. Scraping a,td,span finds nothing and
reports "no tags" on a page that is rendering them correctly.
* Logged-out checks use a throwaway persistent profile. A plain
chromium.launch() does not complete its handshake here, and clearing the
shared profile's cookies would cost an interactive re-login.
Private hold, logged out: the plan expects a denial. It is not what happens —
the page returns 200 with every tag listed while /v2/ refuses the same repo
with 401 for the same caller. Recorded as a documented divergence rather than
asserted as a failure: the manifest records are world-readable in the user's
PDS by design, so nothing secret is exposed, and /r/ has used OptionalAuth
since before validate-base with the page never consulting captain.Public. The
two read paths simply disagree, and that predates this range.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
batch09-token.sh drives the request shapes against a running stack, which is
where the interesting part of b25aee3 lives: handler_test.go proves each shape
in-process, but it cannot show which form a real client picks, and that is the
whole reason the commit exists.
What driving real clients turned up, now encoded in the script's comments so a
re-run re-checks it:
* Docker 29.7.2 and skopeo 1.22.2 use the GET form even holding a credential
helper secret, and take two token requests for a pull with no 401 retry —
so neither exercises the POST path at all.
* containerd 2.3.3 does POST, and gets 200. That is the client b25aee3 was
written for, and the only one here that would have eaten the old 405.
* There is no anonymous branch in the handler at this branch; anonymous GET
is a 401. The anonymous path arrives with val/10-anonpull, so the plan's
"anonymous pull is GET-only" note describes a later batch.
seed-legacy-devices.go reproduces the day-one production devices table for the
08121f3 check: every row legacy, the real device inserted last so it sits at
the end of the rowid-order scan. Measured here at 200 rows: 7.83s first auth,
backfilled, 0.006s second. It is build-tagged `ignore` so it stays out of
go build ./... while remaining go run-able.
The 9d4ad84 comment claimed the reference PDS and tranquil both answer 403
InsufficientScope for a read-only app password. Only the tranquil half is
supported: the observation is issue #26 on pds.sqrl.systems, and the reference
PDS has no read-only app passwords at all, so the branch cannot be reproduced
against one. The comment now records that provenance and the reason a wrong
guess is harmless — the classification only adds a branch, and an unrecognised
error name falls through to the 503 that shipped before it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SeaUS5AFPX9gqCahoLRMRh
Three scripts, split by what they cost to run.
batch07-stale-preview.mjs stages a preview and waits out the 30-minute
maxPreviewAgeForDelete constant. batch07-stale-click.mjs is the resumable
half: it re-renders whatever preview the hold already holds and clicks delete
on it. GET /admin/api/gc/status re-renders lastPreview WITHOUT touching
lastPreviewAt, so showing an old preview does not reset its age — which is
what makes a failed run cost seconds instead of another 31 minutes.
Result against the dev hold: "preview is 34m0s old (limit 30m0s) — run Scan
again before deleting" rendered through the progress-to-error fragment chain,
with all 387 records still there afterwards. That chain is the point; the
refusal logic itself already has a Go test, but a refusal that renders as
nothing is indistinguishable from "there was nothing to delete".
batch07-sweep.mjs then runs the destructive path for real: 387 records
deleted of 387 staged, orphaned count to zero, referenced blobs unchanged at
15. Safe only against the dev hold on Storj; production is Bunny + UpCloud
and is not reachable from here.
page.on('dialog') did not reliably intercept hx-confirm on this page, and an
unaccepted native dialog blocks every later evaluate() and innerText(), so
the script hangs rather than fails — the worst failure mode for an unattended
check. Both scripts now strip the hx-confirm attribute before clicking. The
confirm is not what is under test.
Two findings worth carrying, neither introduced by this range:
* deleteOrphanedBlobs is still unexercised. The bucket holds 19 objects,
of which 8 are past the 7-day blob grace, and none are unreferenced — so
there is nothing for it to collect. More pushes cannot help: fresh blobs
are inside the grace window by definition.
* Storage accounting is derived from layer records, so this sweep moved the
dashboard from 1.3 GB to 1.1 KB while the bucket held 147 MB throughout.
It was overstating by ~9x before (records for blobs held by another hold)
and understates now (referenced blobs with no layer records). Quotas and
billing read the same number. Belongs to batch 12.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Drives "Scan for Orphans" through the admin panel and asserts the wiring the
Go tests structurally cannot: that the progress fragment swaps into
#gc-results and hands off to the preview fragment, that every advertised stat
renders a value, and that each table's row count agrees with the stat above
it. A GC that decides correctly and renders a blank panel still gets someone
to click delete on the wrong thing.
Then it hand-checks the claim itself, which is the part that matters: for
each distinct manifest behind an orphaned record, resolve the owner's PDS and
ask whether that manifest is really gone.
Two instrument bugs were found writing this, both in the script rather than
the product, and both worth keeping as comments:
* The three tables overlap on a Digest column, so classifying by "has
Digest but no RKey" swallowed Missing Records as orphaned blobs and
reported 3 blobs against a stat of 0. Classification is now by exact
header set.
* Asserting the manifest is ABSENT from the PDS is too strong. A manifest
can be alive and name a different hold, which is exactly what happens
when defaultHold is repointed and the image re-pushed. Those records are
legitimately orphaned here. The only state that means GC is staged to
destroy live data is a manifest that exists AND still names this hold.
Against the dev hold: 387 orphaned records over 68 distinct manifests, 25
sampled — 17 gone, 8 alive but now pointing at the production hold, 0 still
naming this hold. Orphaned blobs 0, referenced 15, and the counts agree with
the tables.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
oauth-refresh-e2e.sh covers the refresh mechanics; this covers the symptom the
range exists to stop — a signed-in browser being thrown out when the access
token rotates underneath it.
The test is only meaningful because ui_sessions is a table carrying an
oauth_session_id rather than an in-memory map, so restarting the appview to
clear the refresher's cache does not by itself log the browser out. Verified
against the row the browser actually uses: one oauth_sessions row, and the
ui_sessions row created by the login points at it. rev advances 1 to 2 while
/settings/user keeps rendering.
Drives the login itself. Given a handle it fills the field and clicks through
consent, which is the whole flow whenever the PDS already has a session. Two
things it must not do, both learned by doing them:
* Never navigate while waiting for a human. The first version re-issued
goto() every two seconds and wiped the login form out from under whoever
was typing into it.
* Never bail permanently at a password field. Returning there left the flow
stranded on the Authorize screen once the password had been submitted,
because nothing was left to click it. It now pauses and resumes.
ATCR_E2E_FRESH clears only the appview's cookies. Clearing all of them takes
the PDS session with it, which turns a handle-and-consent flow into a password
prompt and makes the login impossible to drive unattended.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
client_test.go reproduces the cancellation precisely, but in-process against an
httptest PDS. The failure this range fixes — mass sign-outs — happened against a
real one, through the real refresher and the real oauth_sessions row, so the
unit tests alone are a thinner sign-off than the batch deserves.
Staling the access token in the live row and restarting the appview (the
refresher caches sessions in memory, so editing the DB alone changes nothing)
forces the real refresh path. Four concurrent pulls then advance rev 1 to 3
rather than 1 to 4: the compare-and-swap collapses four racing refreshes into
two rotations, with the losers adopting the winner's token instead of each
burning one. Killing a pull 150ms into that window and retrying still succeeds.
Backs ui.db up first, since a burned refresh token would otherwise leave the
account signed out.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Migrations are forward-only and the appview applies whatever is missing on
boot, so moving to the next batch does not need a reset at all — Air rebuilds
into the new code and the live DB migrates in place. Verified moving onto
val/04-oauth: 0031 appeared in schema_migrations on its own, on top of a
level-27 database, with the appview healthy afterwards.
That matters more than it sounds. ui.db holds the OAuth sessions and the
appview's signing keys, so the old wipe-on-every-switch cost an interactive
`docker-credential-atcr login` per batch, which is most of what made the stack
awkward to hand to an agent. Validating in stack order is all forward motion,
so in the normal case there is now no login at all.
A reset is still done when the live DB carries migrations the branch's code has
never heard of, which is what going backward means, and the per-set snapshot is
still banked so that case can restore rather than start empty.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Batching reorders migrations. val/04-oauth carries e75b2e2 (commit 48 of the
range), whose migration is 0031, while 0028-0030 only arrive in batches 09, 12
and 14. So val/04 holds {..0027, 0031} and val/09 holds {..0027, 0028, 0031}.
Both have a max version of 0031, so keying the snapshot on the max would
restore val/04's database onto val/09 — a database missing 0028's schema while
schema_migrations claims otherwise.
Snapshots are now keyed on a fingerprint of every migration file present, which
distinguishes those two states. The max version is kept in the filename purely
so the directory stays readable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Wiping ui.db on every switch also wipes the OAuth sessions and the appview's
oauth_p256/jwt_rsa keys, so each batch cost an interactive
`docker-credential-atcr login`. There are only five distinct migration levels
across the stack (27 for batches 00-08, 28 for 09-11, 29 for 12-13, 32 for 14,
34 for 15), so the DB is snapshotted per level and restored instead of
re-migrated. One login now serves every batch sharing a level.
Also skip the compose pin from val/01 onward: a7c7db6 lands there, so the
branch's own compose already has the shared netns and pinning would drag in
later batches' changes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jobs_test.go covers the job framework thoroughly, but nothing covers the
wiring: whether the kickoff handler renders the progress fragment into the
right hx-target, and whether the loop actually outlives the request it was
started from. Both are what ab4a4eb changed, and both are invisible to Go
tests — a typo in an hx-target or a fragment that renders blank passes every
assertion we have.
The load-bearing check drives crew import rather than the tier remap. A
one-member remap completes in under a second, so closing the tab "mid-run"
proves nothing; import does a PDS write plus a network PLC lookup per entry,
which leaves a real window to close the browser and watch the job keep going.
It is caught mid-flight at a progress tick with no admin page open.
Seeded members are created on the local-only dev hold and removed in a
finally block. README records the environment traps found while building
this: 127.0.0.1 vs localhost, in-memory sessions dying on every hold rebuild,
UA/IP pinning that makes curl log you out, and the forward-only appview
migrations that require a per-batch DB reset.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
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>
Webhook delivery was neither idempotent nor order-safe, and every failure
returned 400, which Stripe does not retry. A transient DB or hold error
therefore dropped a subscription change silently and permanently.
- New stripe_processed_events table: event_id as primary key dedups
redelivery, and event_created per customer drops stale out-of-order
deliveries.
- HandleWebhook distinguishes ErrWebhookSignature (400, no retry) from
ErrWebhookProcessing (500, Stripe redelivers). The event handlers
return errors instead of swallowing them. ErrBillingDisabled maps to
400: the route is mounted but billing is off, so redelivery can never
succeed and Stripe should stop rather than retry to exhaustion.
- Refuse to boot when billing is enabled with an empty
STRIPE_WEBHOOK_SECRET. Stripe HMACs with the empty key, so an
attacker can reproduce the signature and the endpoint is forgeable.
- UpdateCrewTierOnAllHolds retries each hold (3 attempts, linear
backoff, 5s per request) and returns a joined error so the webhook
can fail and let Stripe redeliver.
The fan-out contacts holds concurrently rather than in sequence. Serially,
one unreachable hold burns the caller's entire 10s budget on its own
retries (3 x 5s plus backoff) and the holds after it are never contacted;
because Stripe redelivers in the same order, a persistently-down first
hold means the rest are never updated at all.
On the hold, the signature-validated sub claim is now the source of truth
for updateCrewTier: a mismatched body userDid is rejected with 403 rather
than retargeting the grant to another DID. "Not crew on this hold" is a
200 no-op, since the appview fans updates out to every managed hold and a
subscriber is not crew everywhere.
That no-op has to be told apart from a storage failure. GetCrewMember
collapsed both into one generic error, so a CAR-store failure read as
"not a member", answered 200, and let the appview record the event as
processed — losing the tier grant permanently, which is exactly the
failure mode this commit exists to prevent. Missing records now carry an
ErrCrewMemberNotFound sentinel, and anything else returns 500 so Stripe
redelivers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
