[HYPER-219] record emailConfirmedAt after OTP-verified sign-up - #234
Open
aspiers wants to merge 11 commits into
Open
[HYPER-219] record emailConfirmedAt after OTP-verified sign-up#234aspiers wants to merge 11 commits into
aspiers wants to merge 11 commits into
Conversation
🦋 Changeset detectedLatest commit: 9d0d0fb The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
🚅 Deployed to the ePDS-pr-234 environment in ePDS
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds signed email-verification propagation from auth-service to PDS. PDS confirms eligible accounts during sign-in and provides an idempotent operator backfill with filtering, dry-run support, reporting, and failure handling. ChangesEmail confirmation tracking
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AuthService
participant SharedCrypto
participant PDS
participant AccountManager
AuthService->>SharedCrypto: sign email_verified claim
SharedCrypto-->>AuthService: signed callback parameters
AuthService->>PDS: redirect with signed callback
PDS->>SharedCrypto: verify callback claim
PDS->>AccountManager: confirm eligible account email
AccountManager-->>PDS: confirmation result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for CI Build 31580298359Coverage increased (+0.04%) to 60.234%Details
Uncovered Changes
Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR ensures ePDS accounts created via the OTP-verified sign-up flow are recorded as having a confirmed email by stamping account.emailConfirmedAt, and provides an opt-in operator backfill script for pre-existing accounts.
Changes:
- Add a small
lib/module to (a) stampemailConfirmedAtfor newly created accounts and (b) backfill legacy rows, plus unit tests. - Call the best-effort stamping helper during
/oauth/epds-callbackfor new accounts only. - Add an operator script (
backfill:email-confirmed) and a Changeset describing the behavior change and backfill instructions.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/pds-core/src/lib/email-confirmed.ts | Adds helpers to set emailConfirmedAt, backfill legacy rows, and a best-effort “hot path” wrapper. |
| packages/pds-core/src/index.ts | Hooks email-confirm stamping into the callback flow for new accounts only. |
| packages/pds-core/src/backfill-email-confirmed.ts | Adds an operator-run script that boots PDS context and executes the backfill. |
| packages/pds-core/src/tests/email-confirmed.test.ts | Adds unit tests covering single-row stamping, best-effort behavior, and backfill selection/update criteria. |
| packages/pds-core/package.json | Registers the new backfill:email-confirmed script. |
| .changeset/record-email-as-confirmed.md | Documents the behavioral change and provides operator backfill guidance. |
Suppressed comments (1)
packages/pds-core/src/tests/email-confirmed.test.ts:153
- The backfill UPDATE mock currently resolves to undefined, but the production code should read result.numUpdatedRows from executeTakeFirst(). Return an UpdateResult-like object here so updated counts are exercised (including bigint-to-number coercion).
return chain(updateWheres, () => {
updateExecuted++
return Promise.resolve(undefined)
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
aspiers
added a commit
that referenced
this pull request
Aug 4, 2026
The reported `updated` count reused the earlier SELECT's row count, which assumes nothing changes between the two statements. A sign-in that stamps one of those rows in between leaves the UPDATE touching fewer rows than were counted, and this number is the operator's only evidence of what the script did. Read Kysely's UpdateResult.numUpdatedRows via executeTakeFirst() instead. The test double now mirrors the real surface -- SELECT via execute(), UPDATE via executeTakeFirst() returning a bigint count -- and rejects an UPDATE sent to execute(), so the mock cannot drift back into accepting an API the production code no longer uses. Verified against a real account.sqlite built by upstream's migrator: numUpdatedRows matches the rows actually written, and dry-run, idempotent-rerun and skip-empty-email behaviour are unchanged. Raised by Copilot in review on PR #234.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pds-core/src/__tests__/email-confirmed.test.ts`:
- Around line 45-46: Replace the `as any` casts in the test fake factories with
explicit `EmailConfirmedDb` and `BackfillDb` typing, using `satisfies`, return
annotations, or narrowly typed fluent builders. Keep the fakes limited to those
local interfaces and do not use unsupported casts for the database shapes.
In `@packages/pds-core/src/index.ts`:
- Around line 447-461: Update the Step 4b email-confirmation flow around
markEmailConfirmed so it runs after every successful OTP callback, regardless of
existingAccount. Remove the !existingAccount guard while preserving the existing
best-effort behavior and arguments, allowing failed writes to be retried on
later sign-ins.
In `@packages/pds-core/src/lib/email-confirmed.ts`:
- Around line 71-75: Replace the direct account-table updates in both
single-account and bulk confirmation helpers with the supported
pds.ctx.accountManager operation(s), preserving the existing emailConfirmedAt
values and retry behavior. If the manager lacks bulk confirmation support, add
or use a narrow account-manager operation rather than accessing the table
directly.
- Around line 145-153: Update the email confirmation flow around the UPDATE
query to capture its execution result and return the database-reported
affected-row count as updated instead of candidates. Preserve candidates for the
selection count, and add a test covering a mismatch between candidate count and
updated rows.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 641d7825-22a5-4d9a-876b-1b82fa93d76b
📒 Files selected for processing (6)
.changeset/record-email-as-confirmed.mdpackages/pds-core/package.jsonpackages/pds-core/src/__tests__/email-confirmed.test.tspackages/pds-core/src/backfill-email-confirmed.tspackages/pds-core/src/index.tspackages/pds-core/src/lib/email-confirmed.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/pds-core/src/lib/email-confirmed.ts:138
- The backfill SELECT runs without
AccountDb.executeWithRetry, so the operator script may fail on transient SQLite busy/locking errors when run against a live DB. Since the query builder already has an.execute()method, wrap it inexecuteWithRetrylike other account-db writes.
}): Promise<BackfillResult> {
const dryRun = opts.dryRun ?? false
const emailConfirmedAt = opts.emailConfirmedAt ?? new Date().toISOString()
const rows = await opts.db.db
.selectFrom('account')
.select('did')
packages/pds-core/src/lib/email-confirmed.ts:153
- The backfill UPDATE isn’t wrapped in
executeWithRetry, and the function reportsupdated: candidatesregardless of how many rows were actually modified. If rows change between the SELECT and UPDATE, or if the UPDATE touches fewer rows, the report will be incorrect. PreferexecuteTakeFirst()and deriveupdatedfromnumUpdatedRows(as done elsewhere in pds-core) while also usingexecuteWithRetryfor the write.
if (dryRun || candidates === 0) {
return { candidates, updated: 0, dryRun }
}
// Report what the UPDATE actually touched rather than reusing the
// SELECT's count. The two are separate statements, so a sign-in
// that stamps one of these rows in between would make `candidates`
// an overstatement — and this number is the operator's only
// evidence of what the script did.
packages/pds-core/src/lib/email-confirmed.ts:90
BackfillDbomitsexecuteWithRetry, butAccountManager.dbprovides it and it’s important for operator scripts that may run against a live SQLite DB (avoids transient SQLITE_BUSY failures). Including it in the structural slice also makes the expectation explicit and keeps fakes honest.
This issue also appears in the following locations of the same file:
- line 132
- line 145
export interface BackfillDb {
db: {
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- structural slice of Kysely's fluent builder; the real types come from AccountDb at the call site
selectFrom: (table: 'account') => any
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- ditto
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/pds-core/src/lib/email-confirmed.ts:142
- backfillEmailConfirmedAt() executes its SELECT/UPDATE directly. When run against a deployment where the server is still using the SQLite DB, this can fail with transient SQLITE_BUSY. Since the call site already passes AccountDb, use executeWithRetry when it’s available to make the backfill more reliable.
.select('did')
.where('emailConfirmedAt', 'is', null)
.where('email', 'is not', null)
.where('email', '!=', '')
.execute()
packages/pds-core/src/lib/email-confirmed.ts:90
- BackfillDb doesn’t expose the AccountDb SQLite busy-retry wrapper, so backfillEmailConfirmedAt() can’t reliably reuse executeWithRetry when it’s available. When this script is run against a live deployment, a SQLITE_BUSY could fail the one-off backfill unnecessarily.
This issue also appears on line 138 of the same file.
export interface BackfillDb {
db: {
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- structural slice of Kysely's fluent builder; the real types come from AccountDb at the call site
selectFrom: (table: 'account') => any
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- ditto
updateTable: (table: 'account') => any
}
}
packages/pds-core/src/backfill-email-confirmed.ts:53
- The script error output uses String(err), which can degrade to "[object Object]" and drops stack traces for Error instances. Including the stack when available will make operator debugging much easier.
main().catch((err: unknown) => {
process.stderr.write(`Backfill failed: ${String(err)}\n`)
process.exit(1)
aspiers
added a commit
that referenced
this pull request
Aug 4, 2026
AGENTS.md is explicit: "Do not directly read or modify @atproto/pds database tables -- use pds.ctx.accountManager.* methods." The Kysely UPDATE this PR introduced violated that, on my mistaken claim that no public API could do the job. There is one. createEmailToken() and confirmEmail() are both public AccountManager methods, and together they are the supported route to a confirmed address: confirmEmail validates the token, deletes it and sets emailConfirmedAt in a single transaction. createEmailToken only inserts the row and returns the token -- upstream's XRPC handlers do the mailing separately -- so nothing is sent to the user, which matters when they just proved ownership via the OTP. Also fix the retry gap this exposed. Confirmation was keyed on the account being new, so a failed write was never retried and the account stayed unconfirmed until an operator ran the backfill. It is now keyed on the account being *unconfirmed*, so a returning user whose earlier attempt failed is repaired on their next sign-in, while already-confirmed accounts still cost no writes. The backfill now confirms one account at a time through the same API rather than a single set-based UPDATE: more round-trips, but it keeps to the account-manager boundary and lets one bad account be reported without abandoning the run. Failures are listed by DID and the script exits non-zero. Enumerating accounts remains a direct read -- the manager exposes getAccounts(dids) but no "every account" query -- and is confined to this operator-invoked script. Dropping the Kysely fakes removes the `as any` casts from the tests; they now type against the local interface, so a signature change breaks them instead of passing silently. Verified against a real account.sqlite built by upstream's migrator: unconfirmed accounts are confirmed, already-confirmed ones untouched, empty-email skipped, re-runs find nothing, and zero email_token rows are left behind. Raised by CodeRabbit in review on PR #234.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/pds-core/src/lib/email-confirmed.ts:28
- PR description says the write path "goes through the public, typed
AccountManager.db" (direct UPDATE) to avoid deep imports, but the implementation here uses a mint-then-redeem flow (createEmailToken+confirmEmail) instead. Please align the PR description with the actual approach (or change the implementation to match the described direct-DB write), so operators/reviewers aren’t misled about the chosen mechanism and its side effects (token creation).
* ## Why mint-then-redeem rather than writing the column
*
* AGENTS.md: "Do not directly read or modify `@atproto/pds` database
* tables — use `pds.ctx.accountManager.*` methods." `createEmailToken`
* and `confirmEmail` are both public `AccountManager` methods, and
* together they are exactly the supported route to a confirmed email:
* `confirmEmail` validates the token, deletes it, and sets
* `emailConfirmedAt` in a single transaction, so no token row is left
* behind.
packages/pds-core/package.json:10
- The operator backfill script is configured to run via
tsx src/backfill-email-confirmed.ts, but production builds/Docker images for pds-core only shipdist/(nosrc/). That makes the documentedpnpm --filter @certified-app/pds-core backfill:email-confirmedcommand fail in deployed environments. Prefer invoking the compiled entrypoint fromdist/so it works anywhere the service runs.
"backfill:email-confirmed": "tsx src/backfill-email-confirmed.ts"
aspiers
added a commit
that referenced
this pull request
Aug 4, 2026
…dary The merge analysis framed the HMAC callback purely as an authenticity check. HYPER-219 (PR #234) showed the boundary also forces every authentication *fact* to be re-serialised: the code that knows how the user authenticated lives in auth-service, the API that records it lives in pds-core, and nothing but an explicit signed field can carry the fact between them. The natural implementation instead inferred it -- a valid callback had only ever followed an OTP, so arrival was treated as proof of control of the address. That is correct for today's single sign-in flow and silently wrong for the next one: a passkey flow would send a signed callback carrying the email merely to locate the account, and the address would be marked confirmed with nothing proved. Add the bullet to the merge benefits, a worked example section, and a note to the recommendation. The phasing is unchanged -- the integration hazards still argue for single-origin first -- but the boundary's cost is now known to recur per authentication mechanism rather than being a fixed one-off, which raises the standing cost of not merging. Also captures two details worth reusing at any such boundary: make carried auth facts required rather than sentinel-defaulted, so an omitting caller fails loudly instead of claiming a false negative; and reset the fact when the subject is rebound, as the recovery path does when it swaps a verified backup address for the primary.
aspiers
added a commit
that referenced
this pull request
Aug 4, 2026
pds-core inferred that a valid signed callback meant the user had proved control of the email it carried. That held only because an emailed one-time code was the sole route to such a callback, and the assumption lived in a comment rather than in the payload. A passkey or similar flow breaks it: that flow would legitimately sign a callback carrying `email` merely to locate the account, having proved nothing about the address. pds-core would then mark it confirmed -- asserting email_verified: true to relying parties on no evidence, and arming upstream's email-change verification gate on an address nobody proved. The failure would be silent and in the dangerous direction. Carry the fact instead of inferring it. auth-service reads better-auth's emailVerified where it already reads the session and signs it into the callback as `email_verified`; pds-core records confirmation only on an explicit '1'. The field is required rather than sentinel-defaulted like handle and client_id. The payload is positional, so a producer that omits it signs a different payload and is rejected at the trust boundary. A future flow whose author forgets it fails loudly instead of quietly claiming verification. It is inside the HMAC, so it cannot be flipped from '0' to '1' by anyone holding the URL. The recovery path sets it false: it rebinds `email` from the verified backup address to the account's primary, so the session's flag no longer describes the address being signed. Also repairs two expiry tests that hand-build the payload. They were passing on a stale signature rather than on the timestamp check, so they would have kept passing with expiry validation removed entirely. Operators must deploy both services together; a mixed pair rejects sign-in with "Invalid callback signature" until the rollout completes. Raised by Adam in review of PR #234.
aspiers
added a commit
that referenced
this pull request
Aug 4, 2026
…as errors Three review points from PR #234. Add an optional trailing argument to the backfill that scopes the run to addresses containing it, case-insensitively -- "@gmail.com" for a domain, a full address for one account. Operators can then work through a deployment in batches or repair a single user instead of being forced to process the whole table in one go. No argument still means every account: "no filter" must never be read as "match nothing", or a mistyped invocation would silently do nothing and look like a clean run. The report names the filter, because otherwise "0 account(s)" cannot be distinguished between nothing-left-to-do and your-filter-matched-nothing. Raise the failed-confirmation log from warn to error. Swallowing the exception keeps the user signed in, but the account is left claiming an unverified address to every relying party until some later sign-in happens to succeed, and the operator has no other signal it occurred. Self-healing is not the same as harmless, so it belongs at error-level alerting. Add End users to the changeset's audience list. The claim is user-visible: once #233 lands, the PDS /account route stops telling people to verify an address they have already confirmed. Verified against a real account.sqlite built by upstream's migrator: the domain filter matched mixed-case addresses, left non-matching accounts untouched, a later unfiltered run picked up the remainder, and no email_token rows were left behind.
aspiers
force-pushed
the
adam/hyper-219-set-emailconfirmedat-on-epds-account-after-otp-verified
branch
from
August 4, 2026 21:20
ec559a9 to
a4ec9c0
Compare
aspiers
added a commit
that referenced
this pull request
Aug 4, 2026
The changeset had grown to a full operator runbook: six bullets of command syntax, flag semantics, exit codes and a caveat about provisioning accounts outside the sign-in flow. None of that is release-note material, and the writing-changesets skill says so plainly -- "keep it as short as the change deserves", two to four sentences per audience, and cut anything the reader does not act on. Move the procedure to a "Backfilling Email Confirmation" section in docs/deployment.md, alongside the other operator runbooks, where it can be found by someone doing the task rather than only by someone reading the changelog for the release that introduced it. The changeset keeps one sentence per audience and points operators at the doc: 23 lines down to 13. Raised by Adam in review on PR #234.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/pds-core/src/backfill-email-confirmed.ts`:
- Around line 75-77: Update the main().catch fatal-error handler to use the
project’s pino logger and log the original error object as structured metadata
via logger.error({ err }, ...), replacing the process.stderr.write(String(err))
call; preserve process.exit(1) after logging.
- Around line 53-56: Update the backfill flow around the account enumeration to
avoid accessing accountManager.db.db directly. Add and use a narrow enumeration
method on the account-manager API that returns the required did, email, and
emailConfirmedAt fields for BackfillCandidate, or establish the repository
exception if that boundary cannot be extended.
In `@packages/pds-core/src/lib/email-confirmed.ts`:
- Around line 48-67: The confirmation flow must bind verification to the email
proved by the callback, not only the account DID. In
packages/pds-core/src/lib/email-confirmed.ts lines 48-67, extend
EmailConfirmingAccountManager and confirmAccountEmail to accept the expected
email and call an upstream atomic operation that compares it before recording
confirmation; at lines 88-100, propagate that email through the best-effort
wrapper. In packages/pds-core/src/index.ts lines 497-506, pass the HMAC-bound
callback email to markEmailConfirmed(). In
packages/pds-core/src/lib/email-confirmed.ts lines 214-217, pass each backfill
candidate’s email and skip or report rows whose email changed since scanning.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 92711487-21d7-46da-a19f-35b660da6dba
📒 Files selected for processing (12)
.changeset/record-email-as-confirmed.mddocs/deployment.mdpackages/auth-service/src/__tests__/build-epds-callback-url.test.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/complete.tspackages/pds-core/package.jsonpackages/pds-core/src/__tests__/email-confirmed.test.tspackages/pds-core/src/backfill-email-confirmed.tspackages/pds-core/src/index.tspackages/pds-core/src/lib/email-confirmed.tspackages/shared/src/__tests__/crypto.test.tspackages/shared/src/crypto.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/pds-core/package.json
Without this change, every ePDS account reports its email address as unverified. Accounts are created by /oauth/epds-callback, which only runs once auth-service has verified a one-time code sent to that address, but upstream populates emailConfirmedAt solely from confirmEmail()'s token flow -- a token the OTP flow never issues. The column therefore stays null, so the email_verified claim is always false, and upstream's requestEmailUpdate skips its confirmation-token gate entirely because that gate only engages when emailConfirmedAt is set. Stamp emailConfirmedAt on newly created accounts, at the point where both creation branches converge and the account is known to be new. The write is best-effort: the user has already proven ownership of the address, so a failed write is logged at warn rather than failing their sign-in. Write through the public, typed AccountManager.db rather than upstream's setEmailConfirmedAt helper, which is not re-exported from the package root and has no exports map -- reaching it means a deep import into dist/ that nothing guarantees across upgrades. The emitted statement is identical to upstream's. Accounts predating this are handled by an opt-in backfill script rather than a startup fixup. A null emailConfirmedAt cannot be proven to mean "OTP-verified but unrecorded": ePDS does not block upstream's com.atproto.server.createAccount XRPC route, so a deployment that provisioned accounts by other means must not have those addresses silently promoted to verified. Only the operator knows which case applies, so they choose when to run it, with --dry-run to preview. Verified against a real account.sqlite built by upstream's migrator: dry run writes nothing, the real run stamps only unconfirmed accounts with a genuine address, already-confirmed timestamps are preserved, and re-running is a no-op. Refs HYPER-219.
Coveralls flagged a -0.06% drop: backfill-email-confirmed.ts is an entry point, so none of its lines are exercised by the suite. The repo counts entry points rather than excluding them (index.ts sits at 0/372), so adding an exclusion would bend the project's coverage convention to flatter this branch. Move the two testable pieces -- --dry-run parsing and the completion line -- into lib/email-confirmed.ts, leaving the script as env loading, PDS.create and teardown. Both are worth asserting on their own terms: a near-miss flag such as --dry must not be read as --dry-run, since writing when the operator meant to preview is the one unrecoverable mistake here, and a zero-row run must say so rather than print nothing. Line coverage 57.37% -> 57.45%; 1093 -> 1098 tests.
The reported `updated` count reused the earlier SELECT's row count, which assumes nothing changes between the two statements. A sign-in that stamps one of those rows in between leaves the UPDATE touching fewer rows than were counted, and this number is the operator's only evidence of what the script did. Read Kysely's UpdateResult.numUpdatedRows via executeTakeFirst() instead. The test double now mirrors the real surface -- SELECT via execute(), UPDATE via executeTakeFirst() returning a bigint count -- and rejects an UPDATE sent to execute(), so the mock cannot drift back into accepting an API the production code no longer uses. Verified against a real account.sqlite built by upstream's migrator: numUpdatedRows matches the rows actually written, and dry-run, idempotent-rerun and skip-empty-email behaviour are unchanged. Raised by Copilot in review on PR #234.
AGENTS.md is explicit: "Do not directly read or modify @atproto/pds database tables -- use pds.ctx.accountManager.* methods." The Kysely UPDATE this PR introduced violated that, on my mistaken claim that no public API could do the job. There is one. createEmailToken() and confirmEmail() are both public AccountManager methods, and together they are the supported route to a confirmed address: confirmEmail validates the token, deletes it and sets emailConfirmedAt in a single transaction. createEmailToken only inserts the row and returns the token -- upstream's XRPC handlers do the mailing separately -- so nothing is sent to the user, which matters when they just proved ownership via the OTP. Also fix the retry gap this exposed. Confirmation was keyed on the account being new, so a failed write was never retried and the account stayed unconfirmed until an operator ran the backfill. It is now keyed on the account being *unconfirmed*, so a returning user whose earlier attempt failed is repaired on their next sign-in, while already-confirmed accounts still cost no writes. The backfill now confirms one account at a time through the same API rather than a single set-based UPDATE: more round-trips, but it keeps to the account-manager boundary and lets one bad account be reported without abandoning the run. Failures are listed by DID and the script exits non-zero. Enumerating accounts remains a direct read -- the manager exposes getAccounts(dids) but no "every account" query -- and is confined to this operator-invoked script. Dropping the Kysely fakes removes the `as any` casts from the tests; they now type against the local interface, so a signature change breaks them instead of passing silently. Verified against a real account.sqlite built by upstream's migrator: unconfirmed accounts are confirmed, already-confirmed ones untouched, empty-email skipped, re-runs find nothing, and zero email_token rows are left behind. Raised by CodeRabbit in review on PR #234.
pds-core inferred that a valid signed callback meant the user had proved control of the email it carried. That held only because an emailed one-time code was the sole route to such a callback, and the assumption lived in a comment rather than in the payload. A passkey or similar flow breaks it: that flow would legitimately sign a callback carrying `email` merely to locate the account, having proved nothing about the address. pds-core would then mark it confirmed -- asserting email_verified: true to relying parties on no evidence, and arming upstream's email-change verification gate on an address nobody proved. The failure would be silent and in the dangerous direction. Carry the fact instead of inferring it. auth-service reads better-auth's emailVerified where it already reads the session and signs it into the callback as `email_verified`; pds-core records confirmation only on an explicit '1'. The field is required rather than sentinel-defaulted like handle and client_id. The payload is positional, so a producer that omits it signs a different payload and is rejected at the trust boundary. A future flow whose author forgets it fails loudly instead of quietly claiming verification. It is inside the HMAC, so it cannot be flipped from '0' to '1' by anyone holding the URL. The recovery path sets it false: it rebinds `email` from the verified backup address to the account's primary, so the session's flag no longer describes the address being signed. Also repairs two expiry tests that hand-build the payload. They were passing on a stale signature rather than on the timestamp check, so they would have kept passing with expiry validation removed entirely. Operators must deploy both services together; a mixed pair rejects sign-in with "Invalid callback signature" until the rollout completes. Raised by Adam in review of PR #234.
…as errors Three review points from PR #234. Add an optional trailing argument to the backfill that scopes the run to addresses containing it, case-insensitively -- "@gmail.com" for a domain, a full address for one account. Operators can then work through a deployment in batches or repair a single user instead of being forced to process the whole table in one go. No argument still means every account: "no filter" must never be read as "match nothing", or a mistyped invocation would silently do nothing and look like a clean run. The report names the filter, because otherwise "0 account(s)" cannot be distinguished between nothing-left-to-do and your-filter-matched-nothing. Raise the failed-confirmation log from warn to error. Swallowing the exception keeps the user signed in, but the account is left claiming an unverified address to every relying party until some later sign-in happens to succeed, and the operator has no other signal it occurred. Self-healing is not the same as harmless, so it belongs at error-level alerting. Add End users to the changeset's audience list. The claim is user-visible: once #233 lands, the PDS /account route stops telling people to verify an address they have already confirmed. Verified against a real account.sqlite built by upstream's migrator: the domain filter matched mixed-case addresses, left non-matching accounts untouched, a later unfiltered run picked up the remainder, and no email_token rows were left behind.
Two suppressed Copilot findings, both real. The backfill script ran via `tsx src/backfill-email-confirmed.ts`, but the runtime image copies only `dist/` and tsx is a devDependency (Dockerfile.pds:36). The documented command would therefore fail in exactly the deployed environment an operator needs it in. Point the script at the compiled entrypoint, which tsconfig already emits since it includes all of src/, and keep the tsx form as `backfill:email-confirmed:dev` for local use. Verified `node dist/backfill-email-confirmed.js` loads and reaches config validation. A callback missing `email_verified` failed as "Invalid callback signature", since the field is inside the HMAC. Accurate but misleading: during a mixed-version rollout the cause is an old auth-service, not a secret mismatch, and that error sends an operator hunting in the wrong place. Reject a missing or non-'0'/'1' value up front with an explicit 400 naming the parameter, mirroring the existing expired-vs-invalid split that exists for the same diagnosability reason.
The changeset had grown to a full operator runbook: six bullets of command syntax, flag semantics, exit codes and a caveat about provisioning accounts outside the sign-in flow. None of that is release-note material, and the writing-changesets skill says so plainly -- "keep it as short as the change deserves", two to four sentences per audience, and cut anything the reader does not act on. Move the procedure to a "Backfilling Email Confirmation" section in docs/deployment.md, alongside the other operator runbooks, where it can be found by someone doing the task rather than only by someone reading the changelog for the release that introduced it. The changeset keeps one sentence per audience and points operators at the doc: 23 lines down to 13. Raised by Adam in review on PR #234.
Applies the three review suggestions verbatim. Note this now differs from the other pending changesets, which start lowercase after the bold label. Worth settling one way in the skill so the generated changelog reads consistently.
The atproto upgrade in #233 changed both halves of the confirmation call. `confirmEmail` takes positional arguments and now also takes the address — `confirmEmail(did, email, token)` — and `Account.sub` became `Account.did`. `createEmailToken` takes a `DidString`. Upstream now compares the supplied address against the account's current one and throws `InvalidEmail` on a mismatch. That closes the TOCTOU gap CodeRabbit raised on this PR: pass the address whose control was actually proved and the check becomes atomic and upstream-enforced, rather than something this code has to re-derive. So thread the proved address through instead of only the DID: - the callback passes the HMAC-signed `email`, not the account's stored address, so a change between signing and confirmation is rejected; - the backfill passes each candidate's scanned address, so a row whose email moved between scan and write is reported as a failure rather than confirmed on stale evidence. The test fake records the address it receives, so a future signature change breaks these tests rather than passing silently. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aspiers
force-pushed
the
adam/hyper-219-set-emailconfirmedat-on-epds-account-after-otp-verified
branch
from
August 11, 2026 23:34
a4ec9c0 to
86068b5
Compare
…ception `String(err)` collapsed the error to its message, discarding the type and stack — exactly the context an operator needs when a one-off backfill dies. Use the repo's `createLogger` so pino serialises `err` in full. `process.exit(1)` is unchanged, so scripted runs still see the failure. The account enumeration this script does was defended in a code comment, but AGENTS.md states the account-manager boundary as an unqualified rule. Record the exception where the rule lives instead: `AccountManager` offers only `getAccount`, `getAccounts(dids)` and `getAccountByEmail`, all requiring the identifier up front, so there is no supported way to list every account. Noted as something to re-check on each `@atproto/pds` upgrade. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Contributor
Author
|
(comment generated by Claude Opus 5 via Claude Code) The red
Everything else on this PR is green: 16 checks passing, 0 unresolved review threads, Coveralls +0.04% to 60.234%. Once #246 is sorted, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Every ePDS account reported its email address as unverified.
Accounts are created after the user verifies an emailed one-time code, so the address genuinely is verified. But upstream only records confirmation via
confirmEmail()'s token flow, which the OTP path never triggered, soemailConfirmedAtstayed null. Two consequences:email_verifiedwas always false. Upstream's oauth-store derives it asemailConfirmedAt != null, so relying parties saw a verified address reported as unverified.requestEmailUpdateonly demands a confirmation token whenemailConfirmedAtis set.What this does
Confirmation goes through the public
AccountManagerAPI —createEmailToken+confirmEmail, per AGENTS.md's "usepds.ctx.accountManager.*methods, do not directly read or modify@atproto/pdsdatabase tables".confirmEmailvalidates the token, deletes it and setsemailConfirmedAtin a single transaction, so no token row is left behind. No email is sent:createEmailTokenonly inserts the row and returns the token; upstream's XRPC handlers do the mailing separately.Keyed on the account being unconfirmed, not new. A returning user whose earlier confirmation failed is repaired on their next sign-in; already-confirmed accounts do no writes. Failures are logged at
errorand never block sign-in — the user has already proven ownership of the address.Proof of email control now travels in the signed callback. pds-core previously inferred that a valid callback meant the address had been proved, which held only because an emailed code was the sole sign-in path. A passkey flow would legitimately send a signed callback carrying
emailmerely to locate the account, having proved nothing about that address — and pds-core would have marked it confirmed, assertingemail_verified: trueto relying parties on no evidence. auth-service now reads better-auth'semailVerifiedwhere it verifies the code, and signs it into the callback asemail_verified; pds-core acts only on an explicit'1'.The field is required, not sentinel-defaulted: the HMAC payload is positional, so a producer that omits it signs a different payload and is rejected. A future flow that forgets it fails loudly rather than silently claiming verification. The recovery path sets it
false, because it rebindsemailfrom the verified backup address to the account's primary.Existing accounts are handled by an opt-in script rather than a startup fixup — a null
emailConfirmedAtcannot be proven to mean "verified but unrecorded", since ePDS does not block upstream'scom.atproto.server.createAccountXRPC route:pnpm --filter @certified-app/pds-core backfill:email-confirmed --dry-run pnpm --filter @certified-app/pds-core backfill:email-confirmed pnpm --filter @certified-app/pds-core backfill:email-confirmed @gmail.com # scope to a domainIdempotent; a trailing argument scopes the run by case-insensitive address substring; failed accounts are listed by DID and the command exits non-zero.
Deploying
Ship auth-service and pds-core together. The signed handover carries a new required field, so a mixed pair rejects sign-in with an explicit 400 naming the parameter until both are updated. Sign-in recovers on its own once the rollout completes; no data is affected.
Verification
typecheck,lint,formatclean; 1114 tests. Beyond unit fakes, the backfill was exercised against a realaccount.sqlitebuilt by upstream's own migrator:@gmail.comfilteremail_tokenrowsThat exercise also surfaced that
account.emailisNOT NULLin the PDS schema, so "no address" arrives as the empty string.Notes for review
AccountManagerexposesgetAccounts(dids)but no "every account" query, and a backfill cannot know the DIDs in advance. Confined to the operator-invoked script; the request path never does it.Refs HYPER-219.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation