feat(release-service): add OAuth custody stores - #2010
Conversation
|
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-cache | e4f8c79 | Jul 13 2026, 01:26 PM |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-playground | e4f8c79 | Jul 13 2026, 01:26 PM |
Scope checkThis PR changes 2,507 lines across 19 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
emdash-demo-do | e4f8c79 | Jul 13 2026, 01:24 PM |
There was a problem hiding this comment.
This PR adds OAuth custody stores for the delegated release service: D1 persistence for publisher accounts, OAuth transaction state, console sessions, and durable release delegations, plus public atproto client metadata/JWKS endpoints. The change is scoped to the private apps/release-service package, tracks the RFC in #1908, and uses the patterns already established in the app (parameterized D1 statements, envelope encryption with AEAD context binding, redirect canonicalization, compare-and-set leases).
What I checked: the migration schema, loadConfiguration/keyset validation, OAuthCustodyRepository round-trips, CAS/lease refresh flow, encryption contexts/purposes, redirect-target hardening, and the new tests.
Headline conclusion: the implementation is careful and well-tested. I have one design concern about the reauthorization recovery path before I’d call it fully safe to merge: the store transitions delegations to reauthorization_required when the assertion key is rotated away, but putDelegation treats any non-revoked row as a hard conflict. That means a later OAuth flow that successfully reauthorizes the publisher will still fail when atcute calls sessions.set, because the stale reauthorization_required row blocks the insert. Either putDelegation should replace a reauthorization_required row, or the transition should fully revoke the row so a new active grant can be created; as-is the state machine may leave the publisher stuck until something else manually revokes.
| async putDelegation(publisherDid: `did:${string}:${string}`, session: StoredSession) { | ||
| this.validateDelegationSession(publisherDid, session); | ||
| const existing = await this.getDelegationByPublisher(publisherDid); | ||
| if (existing) throw new OAuthCustodyError("OAUTH_DELEGATION_CAS_REQUIRED"); |
There was a problem hiding this comment.
[suggestion] putDelegation rejects any existing non-revoked delegation with OAUTH_DELEGATION_CAS_REQUIRED (line 515). However, transitionMissingClientKeyCas can move a delegation to reauthorization_required, and the natural recovery is to run the OAuth flow again and store a new active session. Because getDelegationByPublisher filters revoked_at IS NULL, a reauthorization_required row is still considered existing, so the new authorization will fail with the same CAS error.
Please confirm whether the caller is expected to explicitly revokeDelegation before reauthorizing. If the store is meant to handle reauthorization itself, putDelegation should either update the stale row in place or ignore/revoke reauthorization_required rows before inserting the new active grant. A conditional such as allowing replacement only for status === 'reauthorization_required' would avoid leaving publishers stuck after a key rotation.
|
Fixed in 91f4e84. Added a regression that rotates away the original assertion key, transitions the delegation to Verification: full build, package typecheck, release-service typecheck, lint 0, 209 Worker tests, and 52 Node tests. Independent scoped review found no remaining issue. ~ Sol 🤖 |
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
There was a problem hiding this comment.
The approach is sound and scoped correctly: this adds D1-backed OAuth custody stores for the private apps/release-service, tracks the RFC in #1908, and follows the app's existing patterns (parameterized D1 statements, envelope encryption with AEAD context binding, redirect canonicalization, compare-and-set leases). My prior concern about the reauthorization-recovery path is addressed: putDelegation now replaces a reauthorization_required row, and a new test verifies the full reauthorize-after-key-rotation flow.
What I checked: the D1 migration, loadConfiguration/keyset validation, OAuthCustodyRepository transaction/session/delegation round-trips, the CAS/lease refresh flow, encryption contexts/purposes, redirect-target hardening, the new public OAuth metadata/JWKS routes, and the test coverage.
Headline conclusion: the implementation is careful and well-tested, but the delegation state machine has two gaps that can wedge a publisher: a stale refreshing lease is a dead-end state, and the first-insert path is vulnerable to a read-then-insert race that exposes a raw D1 error instead of the intended CAS failure. I found no security issue in the SQL handling, encryption, or redirect hardening.
| async putDelegation(publisherDid: `did:${string}:${string}`, session: StoredSession) { | ||
| this.validateDelegationSession(publisherDid, session); | ||
| const existing = await this.getDelegationByPublisher(publisherDid); | ||
| if (existing && existing.status !== "reauthorization_required") { |
There was a problem hiding this comment.
[needs fixing] After a refresh worker claims a lease, the row is status = 'refreshing'. If the worker crashes or loses the lease without restoring the session, the row remains refreshing with lease_expires_at in the past. putDelegation rejects any row whose status is not reauthorization_required, and claimDelegationLeaseCas/getDelegationSessionForRefresh only target active or an unexpired refreshing lease. There is no cleanup or recovery path in this PR, so a publisher with a stale refreshing row can never create a new delegation until something manually repairs the row.
Consider treating an expired refreshing row as replaceable in this guard and in the CAS UPDATE ... WHERE clause that follows.
| if (existing && existing.status !== "reauthorization_required") { | |
| if ( | |
| existing && | |
| existing.status !== "reauthorization_required" && | |
| !( | |
| existing.status === "refreshing" && | |
| existing.lease_expires_at && | |
| existing.lease_expires_at <= new Date().toISOString() | |
| ) | |
| ) { | |
| throw new OAuthCustodyError("OAUTH_DELEGATION_CAS_REQUIRED"); | |
| } |
| } | ||
| await this.#db | ||
| .prepare( | ||
| `INSERT INTO delegations ( |
There was a problem hiding this comment.
[needs fixing] putDelegation first reads the existing row via getDelegationByPublisher and then inserts if no non-revoked row exists. Two concurrent calls can both observe no row and attempt the INSERT; the partial unique index idx_delegations_active_grant will reject one of them with a low-level SQLite/D1 constraint error instead of the repository's intended OAUTH_DELEGATION_CAS_REQUIRED. Map the conflict to the proper error so callers do not receive raw database errors.
| `INSERT INTO delegations ( | |
| const insertResult = await this.#db | |
| .prepare( | |
| `INSERT OR IGNORE INTO delegations ( | |
| id, publisher_did, release_nsid, encrypted_session, encryption_key_version, | |
| client_key_id, scope, status, refresh_before, created_at, updated_at | |
| ) VALUES (?, ?, ?, ?, ?, ?, ?, 'active', ?, ?, ?)`, | |
| ) | |
| .bind( | |
| id, | |
| publisherDid, | |
| this.#oauth.releaseNsid, | |
| encrypted.envelope, | |
| encrypted.keyVersion, | |
| getClientKeyId(session), | |
| this.#oauth.releaseScope, | |
| refreshBefore, | |
| now, | |
| now, | |
| ) | |
| .run(); | |
| if (insertResult.meta.changes !== 1) { | |
| throw new OAuthCustodyError("OAUTH_DELEGATION_CAS_REQUIRED"); | |
| } | |
| return id; |
|
Resolved both findings in e4f8c79.
Verified locally: full monorepo build, package and release-service typechecks, type-aware lint with 0 diagnostics, 211 Worker tests, and 52 Node tests. |
There was a problem hiding this comment.
Approach
This is the right change, scoped correctly: it adds the D1-backed OAuth custody stores (publisher accounts, single-use OAuth transactions, console sessions, durable delegations with CAS + leases) and the public atproto client-metadata/JWKS discovery routes to the private apps/release-service, tracking the delegated-release RFC (#1908). It follows the app's existing patterns — parameterized D1 statements, envelope encryption with purpose/table/primaryKey/owner AEAD binding, redirect canonicalization, and compare-and-set leases — and correctly keeps the atproto discovery endpoints outside the EmDash {data, requestId} API envelope via includeInApiSchema: false (those endpoints must emit raw atproto OAuth metadata, not the EmDash envelope).
Re-review: prior findings are resolved
-
Stale
refreshinglease dead-end.claimDelegationLeaseCasnow reclaims an expired lease (status = 'refreshing' AND lease_expires_at <= ?), so a crashed refresh worker no longer leaves the row in an unrecoverable state. Theidx_delegations_lease_expirypartial index supports the scan, andreclaims an expired delegation refresh leaseverifies the full reclaim → store cycle. -
Read-then-insert race exposing a raw D1 error.
putDelegation's insert path now usesON CONFLICT(publisher_did, release_nsid) WHERE revoked_at IS NULL DO NOTHING(correctly targeting the partial unique index) and checksresult.meta.changes !== 1to throw the intendedOAUTH_DELEGATION_CAS_REQUIRED.normalizes concurrent first-delegation conflicts(8 concurrentsessions.set) confirms exactly one wins and the rest get the typed CAS error.
What I re-checked
- Migration
CHECKconstraints ((status='revoked')=(revoked_at IS NOT NULL),(lease_owner IS NULL)=(lease_expires_at IS NULL),encrypted_session IS NOT NULL OR status='revoked',state_version >= 1) against every write path (putDelegationinsert/update,claimDelegationLeaseCas,storeDelegationSessionCas,transitionMissingClientKeyCas,revokeDelegation) — all consistent. loadConfigurationis now async and cached onglobalThisviaSymbol.for(AGENTS.md module-singleton convention) keyed by the bindings object + a value snapshot; all call sites (index.ts, tests)awaitit, and the cache returns a stable promise for identical bindings (verified by the test).parseAssertionKeysetvalidates key shape, decodes/length-checksx/y/d, and proves the pair is consistent by sign-then-verify;getPublicJwksstripsd; no private material reaches the JWKS route.- Redirect hardening (
canonicalizeRedirectTargetrejects//,\, control chars, foreign origin;parseOAuthUserStateenforces the userState redirect equals the options redirect) and the single-useDELETE ... RETURNINGtransaction read. - Encryption contexts/purposes bind ciphertext to purpose+table+primaryKey+ownerDid; the purpose-tamper test confirms cross-purpose decryption fails.
- The new public routes return host-independent metadata (bound to
PUBLIC_ORIGIN, not the request host) and are excluded from the OpenAPI schema and theROUTESregistry assertions.
Conclusion
The implementation is careful and well-tested, and both gaps from the prior review are closed. No security, data-integrity, SQL-safety, or convention issues remain. I found one low-confidence, forward-looking design observation (below) for the upcoming refresh orchestrator; it is not a defect in this custody store.
Headline: approve.
| async putDelegation(publisherDid: `did:${string}:${string}`, session: StoredSession) { | ||
| this.validateDelegationSession(publisherDid, session); | ||
| const existing = await this.getDelegationByPublisher(publisherDid); | ||
| if (existing && existing.status !== "reauthorization_required") { |
There was a problem hiding this comment.
[suggestion] The prior dead-end is resolved at the refresh-job layer (claimDelegationLeaseCas reclaims an expired refreshing lease), but the publisher-facing re-authorization path is still blocked from a refreshing row: this guard rejects any existing delegation whose status is not reauthorization_required, so atcute's post-callback sessions.set → putDelegation throws OAUTH_DELEGATION_CAS_REQUIRED while a refresh lease is in-flight (or stale until a refresh worker reclaims it). The only store-level recovery for the publisher is then sessions.delete (full revoke + fresh re-auth), because transitionMissingClientKeyCas is the sole refreshing → reauthorization_required transition and it is gated on a missing assertion key — there is no path for a refresh that fails for any other reason (e.g. an invalidated refresh token).
This is a design observation for the upcoming refresh orchestrator, not a defect in this custody store: refreshing is intentionally reserved for an in-flight lease, and letting re-auth clobber it would break lease semantics. Worth deciding explicitly how a persistently-failing (non-key) refresh should clear to reauthorization_required rather than relying on revocation + full re-authorization.
f244490
into
feat/delegated-release-service
What does this PR do?
Adds confidential OAuth client metadata and overlapping JWKS, encrypted D1 custody stores, and publisher, transaction, console-session, and durable delegation persistence for the delegated release service. The implementation binds OAuth purpose, DID, redirect state, assertion key, release namespace, leases, and encrypted row identity.
Related: #1908
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
Full monorepo build and package typecheck pass. Release-service tests: 208 workerd and 52 Node tests passed. The slice completed three adversarial review passes with no remaining substantive findings.
Try this PR
Open a fresh playground →
A full working EmDash site, deployed from this branch. Each visit gets its own session-scoped sandbox: no login needed and no shared state. Try the admin, edit content, hit the public site.
Tracks
feat/delegated-release-service-20-oauth-custody. Updated automatically when the playground redeploys.