Skip to content

feat(release-service): add OAuth custody stores - #2010

Merged
ascorbic merged 3 commits into
feat/delegated-release-servicefrom
feat/delegated-release-service-20-oauth-custody
Jul 13, 2026
Merged

feat(release-service): add OAuth custody stores#2010
ascorbic merged 3 commits into
feat/delegated-release-servicefrom
feat/delegated-release-service-20-oauth-custody

Conversation

@ascorbic

@ascorbic ascorbic commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

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

  • Bug fix
  • Feature (requires maintainer-approved Discussion)
  • Refactor (no behavior change)
  • Translation
  • Documentation
  • Performance improvement
  • Tests
  • Chore (dependencies, CI, tooling)

Checklist

  • I have read CONTRIBUTING.md
  • pnpm typecheck passes
  • pnpm lint passes
  • pnpm test passes (or targeted tests for my change)
  • pnpm format has been run
  • I have added/updated tests for my changes (if applicable)
  • User-visible strings in the admin UI are wrapped for translation (if applicable). N/A, no admin UI strings.
  • I have added a changeset (if this PR changes a published package). N/A, release-service is private.
  • New features link to an approved Discussion: N/A, maintainer-owned integration implementing the delegated-release RFC tracked by Delegated release service #1908.

AI-generated code disclosure

  • This PR includes AI-generated code — model/tool: OpenCode with GPT-5.6 Sol and independent adversarial review agents

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.

@changeset-bot

changeset-bot Bot commented Jul 13, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: e4f8c79

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 13, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
emdash-demo-cache e4f8c79 Jul 13 2026, 01:26 PM

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 13, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
emdash-playground e4f8c79 Jul 13 2026, 01:26 PM

@github-actions github-actions Bot added review/needs-review No maintainer or bot review yet size/XL labels Jul 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Scope check

This 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.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jul 13, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
emdash-demo-do e4f8c79 Jul 13 2026, 01:24 PM

@ascorbic ascorbic added the bot:review Trigger an emdashbot code review on this PR label Jul 13, 2026

@emdashbot emdashbot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread apps/release-service/src/oauth/store.ts Outdated
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");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@github-actions github-actions Bot added review/awaiting-author Reviewed; waiting on the author to respond and removed review/needs-review No maintainer or bot review yet labels Jul 13, 2026
@github-actions github-actions Bot added review/needs-rereview Author pushed changes since the last review and removed review/awaiting-author Reviewed; waiting on the author to respond labels Jul 13, 2026
@ascorbic

Copy link
Copy Markdown
Collaborator Author

Fixed in 91f4e84. putDelegation now permits only a reauthorization_required row to be replaced, updates it in place so the ID-bound encryption context remains valid, and guards the update by ID, publisher DID, release NSID, status, and non-revocation. Active and refreshing grants still return OAUTH_DELEGATION_CAS_REQUIRED, and a concurrent state change causes the replacement CAS to fail.

Added a regression that rotates away the original assertion key, transitions the delegation to reauthorization_required, stores a newly authorized session under the active key, and verifies the same row returns to active with an incremented state version.

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 🤖

@ascorbic ascorbic added bot:review Trigger an emdashbot code review on this PR and removed bot:review Trigger an emdashbot code review on this PR labels Jul 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Overlapping PRs

This PR modifies files that are also changed by other open PRs:

This may cause merge conflicts or duplicated work. A maintainer will coordinate.

@emdashbot emdashbot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
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 (

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
`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;

@github-actions github-actions Bot added review/awaiting-author Reviewed; waiting on the author to respond and removed review/needs-rereview Author pushed changes since the last review labels Jul 13, 2026
@ascorbic

Copy link
Copy Markdown
Collaborator Author

Resolved both findings in e4f8c79.

  • The first-delegation insert now uses the targeted partial-index conflict clause and checks meta.changes from the same statement, so concurrent winners produce OAUTH_DELEGATION_CAS_REQUIRED without masking unrelated constraint failures.
  • claimDelegationLeaseCas now permits a caller with the expected state_version to reclaim an expired refreshing lease. The version increment continues to fence the stale owner.
  • Added regression coverage for concurrent first inserts, expired-lease recovery, and stale-owner rejection.

Verified locally: full monorepo build, package and release-service typechecks, type-aware lint with 0 diagnostics, 211 Worker tests, and 52 Node tests.

@ascorbic ascorbic added bot:review Trigger an emdashbot code review on this PR and removed bot:review Trigger an emdashbot code review on this PR labels Jul 13, 2026
@github-actions github-actions Bot added review/needs-rereview Author pushed changes since the last review and removed review/awaiting-author Reviewed; waiting on the author to respond labels Jul 13, 2026
@ascorbic ascorbic added bot:review Trigger an emdashbot code review on this PR and removed bot:review Trigger an emdashbot code review on this PR labels Jul 13, 2026

@emdashbot emdashbot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Stale refreshing lease dead-end. claimDelegationLeaseCas now reclaims an expired lease (status = 'refreshing' AND lease_expires_at <= ?), so a crashed refresh worker no longer leaves the row in an unrecoverable state. The idx_delegations_lease_expiry partial index supports the scan, and reclaims an expired delegation refresh lease verifies the full reclaim → store cycle.

  2. Read-then-insert race exposing a raw D1 error. putDelegation's insert path now uses ON CONFLICT(publisher_did, release_nsid) WHERE revoked_at IS NULL DO NOTHING (correctly targeting the partial unique index) and checks result.meta.changes !== 1 to throw the intended OAUTH_DELEGATION_CAS_REQUIRED. normalizes concurrent first-delegation conflicts (8 concurrent sessions.set) confirms exactly one wins and the rest get the typed CAS error.

What I re-checked

  • Migration CHECK constraints ((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 (putDelegation insert/update, claimDelegationLeaseCas, storeDelegationSessionCas, transitionMissingClientKeyCas, revokeDelegation) — all consistent.
  • loadConfiguration is now async and cached on globalThis via Symbol.for (AGENTS.md module-singleton convention) keyed by the bindings object + a value snapshot; all call sites (index.ts, tests) await it, and the cache returns a stable promise for identical bindings (verified by the test).
  • parseAssertionKeyset validates key shape, decodes/length-checks x/y/d, and proves the pair is consistent by sign-then-verify; getPublicJwks strips d; no private material reaches the JWKS route.
  • Redirect hardening (canonicalizeRedirectTarget rejects //, \, control chars, foreign origin; parseOAuthUserState enforces the userState redirect equals the options redirect) and the single-use DELETE ... RETURNING transaction 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 the ROUTES registry 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") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.setputDelegation 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.

@github-actions github-actions Bot added review/approved Approved; no new commits since and removed review/needs-rereview Author pushed changes since the last review labels Jul 13, 2026
@ascorbic
ascorbic merged commit f244490 into feat/delegated-release-service Jul 13, 2026
12 of 13 checks passed
@ascorbic
ascorbic deleted the feat/delegated-release-service-20-oauth-custody branch July 13, 2026 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:review Trigger an emdashbot code review on this PR cla: signed overlap review/approved Approved; no new commits since size/XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant