feat: handle dead/drifted accounts on /account with sign-out and error page#32
Open
feat: handle dead/drifted accounts on /account with sign-out and error page#32
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughgetDidByEmail now returns Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthService as Auth Service
participant PDS
Client->>AuthService: Request to protected route
AuthService->>AuthService: requireBetterAuth middleware
AuthService->>PDS: getDidByEmail(email, pdsUrl, internalSecret)
alt PDS error / fetch failure
PDS-->>AuthService: null
AuthService-->>Client: 503 + renderError(message)
else Account exists but no DID
PDS-->>AuthService: { did: null }
AuthService->>AuthService: auth.api.signOut(...)
AuthService-->>Client: 403 + renderNoAccountPage()
else Account has DID
PDS-->>AuthService: { did: "did:..." }
AuthService->>AuthService: set res.locals.did & res.locals.betterAuthSession
AuthService-->>Client: continue (next)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
Pull Request Test Coverage Report for Build 23424602846Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/auth-service/src/routes/account-settings.ts (1)
101-125:⚠️ Potential issue | 🟡 MinorRemove the unreachable nullish fallback.
The
requireBetterAuth()middleware returns early whenresult.did === null(line 64–74) without callingnext(). It only callsnext()whenresult.didis non-null (line 79), guaranteeing thatres.locals.didis non-null in the route handler. Thedid ?? '(unknown)'fallback at line 118 is unreachable and matches the@typescript-eslint/no-unnecessary-conditionCI failure.🔧 Suggested fix
renderSettingsPage({ - did: did ?? '(unknown)', + did, email, handleDomain,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/account-settings.ts` around lines 101 - 125, The nullish fallback in the render call is unreachable because requireBetterAuth guarantees res.locals.did is non-null; remove the unnecessary "did ?? '(unknown)'" and pass did directly (the local const "did" already cast as string) to renderSettingsPage, ensuring the route no longer triggers `@typescript-eslint/no-unnecessary-condition`; update any related usages in this handler to rely on the non-null did from res.locals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/better-auth.ts`:
- Around line 194-195: The code treats a null PDS lookup (returned by
getDidByEmail) the same as a missing DID which incorrectly marks returning users
as "new"; change the logic around result and isNewUser in better-auth.ts so you
explicitly detect a failed lookup (result === null) and handle it as an
error/outage (e.g., return an error response or abort sending OTP) instead of
setting isNewUser to true; update the same computation in
packages/auth-service/src/__tests__/better-auth-otp.test.ts to assert the
failure-path behavior (simulate getDidByEmail returning null and expect the
request to fail or not send the welcome OTP) and reference the getDidByEmail
call and the isNewUser variable when making the change.
In `@packages/auth-service/src/routes/account-settings.ts`:
- Around line 54-70: The branch handling result.did === null currently sends
renderNoAccountPage() with a 200; change the response to include a 403 status by
calling res.status(403) before type('html').send(...). Locate the block that
logs the missing PDS account (checks result.did === null) in account-settings.ts
(the branch that calls auth.api.signOut and renderNoAccountPage) and replace the
send call to set status 403 while keeping the signOut try/catch and the final
return.
---
Outside diff comments:
In `@packages/auth-service/src/routes/account-settings.ts`:
- Around line 101-125: The nullish fallback in the render call is unreachable
because requireBetterAuth guarantees res.locals.did is non-null; remove the
unnecessary "did ?? '(unknown)'" and pass did directly (the local const "did"
already cast as string) to renderSettingsPage, ensuring the route no longer
triggers `@typescript-eslint/no-unnecessary-condition`; update any related usages
in this handler to rely on the non-null did from res.locals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3445c854-5bb6-414f-9243-cc24364619f5
📒 Files selected for processing (9)
.beads/issues.jsonldocs/design/testing-gaps.mdpackages/auth-service/src/__tests__/better-auth-otp.test.tspackages/auth-service/src/__tests__/get-did-by-email.test.tspackages/auth-service/src/better-auth.tspackages/auth-service/src/lib/get-did-by-email.tspackages/auth-service/src/routes/account-settings.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/complete.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/auth-service/src/routes/account-settings.ts (1)
57-73:⚠️ Potential issue | 🟠 MajorReturn the no-account HTML page with HTTP 403.
Line 72 sends an error page without setting status, so this branch responds
200 OK. It should be403for the no-account condition.🔧 Suggested fix
- res.type('html').send(renderNoAccountPage()) + res.status(403).type('html').send(renderNoAccountPage())As per coding guidelines "
packages/{auth-service,pds-core}/**/*.ts: Return HTTP error responses withres.status(N).json({ error: '...' })orres.status(N).type('html').send(renderError(...))for HTML pages."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/account-settings.ts` around lines 57 - 73, The no-account branch currently sends the HTML page with a 200 OK; change the response to return HTTP 403 by calling res.status(403) before sending the HTML (i.e., replace the existing res.type('html').send(renderNoAccountPage()) usage with a res.status(403).type('html').send(renderNoAccountPage()) pattern), keeping the existing auth.api.signOut try/catch and logger calls intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/auth-service/src/routes/account-settings.ts`:
- Around line 57-73: The no-account branch currently sends the HTML page with a
200 OK; change the response to return HTTP 403 by calling res.status(403) before
sending the HTML (i.e., replace the existing
res.type('html').send(renderNoAccountPage()) usage with a
res.status(403).type('html').send(renderNoAccountPage()) pattern), keeping the
existing auth.api.signOut try/catch and logger calls intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71043268-afc9-4c3b-9d16-e919cffbb01d
📒 Files selected for processing (6)
packages/auth-service/src/__tests__/render-error.test.tspackages/auth-service/src/lib/render-error.tspackages/auth-service/src/routes/account-settings.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/complete.tspackages/auth-service/src/routes/login-page.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/auth-service/src/routes/complete.ts
- packages/auth-service/src/routes/choose-handle.ts
Kzoeps
force-pushed
the
karma/hyper-137-changing-email-address-on-epds
branch
from
88b5b70 to
7e230c2
Compare
Kzoeps
force-pushed
the
karma/hyper-137-changing-email-address-on-epds
branch
from
a94a197 to
2beb064
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/middleware/require-better-auth.ts`:
- Line 38: The check uses redundant optional chaining on session.user; replace
the condition `if (!session?.user?.email)` by removing the extra `?` on `user`
so it reads `if (!session?.user.email)`, relying on getSession() guaranteeing
`user` when `session` exists (update the condition in the same block where
`session` is obtained, e.g., around the `getSession()`/`session` usage in
require-better-auth).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53dbad9c-0d88-404d-8d90-edaaf778a871
📒 Files selected for processing (13)
.beads/issues.jsonldocs/design/testing-gaps.mdpackages/auth-service/src/__tests__/better-auth-otp.test.tspackages/auth-service/src/__tests__/handle-validation.test.tspackages/auth-service/src/__tests__/render-error.test.tspackages/auth-service/src/__tests__/require-better-auth.test.tspackages/auth-service/src/better-auth.tspackages/auth-service/src/lib/render-error.tspackages/auth-service/src/middleware/require-better-auth.tspackages/auth-service/src/routes/account-settings.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/complete.tspackages/auth-service/src/routes/login-page.ts
✅ Files skipped from review due to trivial changes (4)
- packages/auth-service/src/routes/login-page.ts
- packages/auth-service/src/tests/render-error.test.ts
- packages/auth-service/src/routes/complete.ts
- packages/auth-service/src/lib/render-error.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/auth-service/src/tests/better-auth-otp.test.ts
- packages/auth-service/src/better-auth.ts
- docs/design/testing-gaps.md
Kzoeps
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/middleware/require-better-auth.ts`:
- Around line 68-71: Replace the info-level log that emits PII by changing the
logger call in require-better-auth middleware: stop logging session.user.email
and lower the level to debug (i.e., use logger.debug(...)); instead include
non-PII context such as session.id, user.id, or an anonymized flag and a clear
message like "No PDS account found for session" so the call that currently
references session.user.email is updated to a debug log with only non-PII
fields.
- Around line 72-76: The catch blocks currently suppress or downgrade errors;
update both catches to use structured error logging: replace logger.warn({ err
}, 'Failed to sign out on no-account error') with logger.error({ err }, 'Failed
to sign out on no-account error') and ensure the outer catch (around the
requireBetterAuth middleware flow) also logs the caught error using
logger.error({ err }, '...') instead of hiding details; look for the sign-out
call auth.api.signOut({ headers: fromNodeHeaders(req.headers) }) and the
surrounding requireBetterAuth middleware catch blocks to make these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 017de408-3b1e-472f-9682-8919d3a3fc12
📒 Files selected for processing (4)
docs/design/testing-gaps.mdpackages/auth-service/src/__tests__/require-better-auth.test.tspackages/auth-service/src/middleware/require-better-auth.tspackages/auth-service/src/routes/account-settings.ts
✅ Files skipped from review due to trivial changes (2)
- packages/auth-service/src/tests/require-better-auth.test.ts
- docs/design/testing-gaps.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/auth-service/src/routes/account-settings.ts
Kzoeps
force-pushed
the
karma/hyper-137-changing-email-address-on-epds
branch
from
2beb064 to
56c7e08
Compare
Kzoeps
force-pushed
the
karma/hyper-137-changing-email-address-on-epds
branch
from
56c7e08 to
3b602f2
Compare
Kzoeps
force-pushed
the
karma/hyper-137-changing-email-address-on-epds
branch
from
4494bb9 to
3b602f2
Compare
Kzoeps
force-pushed
the
karma/hyper-137-changing-email-address-on-epds
branch
from
3b602f2 to
a04470f
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/auth-service/src/__tests__/get-did-by-email.test.ts (1)
46-57: Add a malformed-didpayload test for the new tri-state contract.Please add a case for
200 OKwith an invalid/missingdidfield, assertingnull(outer failure). This prevents regressions where malformed payloads leak as ambiguous states.🧪 Suggested test case
+ it('returns null when 200 payload is missing/invalid did', async () => { + fetchSpy.mockResolvedValueOnce( + new Response(JSON.stringify({}), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }), + ) + + const result = await getDidByEmail('alice@example.com', PDS_URL, SECRET) + expect(result).toBeNull() + })As per coding guidelines: "
**/*.test.ts: Prefer unit tests for pure logic (crypto, validation, DB operations, middleware); use mock req/res objects".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/__tests__/get-did-by-email.test.ts` around lines 46 - 57, Add a unit test in the existing get-did-by-email.test.ts that covers a 200 OK response with a malformed or missing did payload: mock fetchSpy to return a Response with status 200 and body either {} or { did: "not-a-valid-did" } (Content-Type application/json), call getDidByEmail('newuser@example.com', PDS_URL, SECRET) and assert the result is { did: null }; name the test something like "returns { did: null } when PDS returns a malformed or missing did" and place it alongside the existing tests for getDidByEmail so the tri-state contract (valid DID, null DID, malformed DID) is fully covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/lib/get-did-by-email.ts`:
- Around line 32-33: The response JSON is being blindly cast and may contain an
unexpected shape; in getDidByEmail replace the cast with a runtime check: parse
res.json() into a local (data), verify
Object.prototype.hasOwnProperty.call(data, 'did') and then if typeof data.did
=== 'string' return { did: data.did }, else if data.did === null return { did:
null }, otherwise return { did: undefined } (and optionally log a warning or
throw if you prefer a hard failure). This uses the existing data variable and
getDidByEmail return so downstream consumers get a correct tri-state value
instead of possibly undefined from a bad cast.
---
Nitpick comments:
In `@packages/auth-service/src/__tests__/get-did-by-email.test.ts`:
- Around line 46-57: Add a unit test in the existing get-did-by-email.test.ts
that covers a 200 OK response with a malformed or missing did payload: mock
fetchSpy to return a Response with status 200 and body either {} or { did:
"not-a-valid-did" } (Content-Type application/json), call
getDidByEmail('newuser@example.com', PDS_URL, SECRET) and assert the result is {
did: null }; name the test something like "returns { did: null } when PDS
returns a malformed or missing did" and place it alongside the existing tests
for getDidByEmail so the tri-state contract (valid DID, null DID, malformed DID)
is fully covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3468076-856c-46c4-abd1-f266e99067ff
📒 Files selected for processing (15)
.beads/issues.jsonldocs/design/testing-gaps.mdpackages/auth-service/src/__tests__/better-auth-otp.test.tspackages/auth-service/src/__tests__/get-did-by-email.test.tspackages/auth-service/src/__tests__/handle-validation.test.tspackages/auth-service/src/__tests__/render-error.test.tspackages/auth-service/src/__tests__/require-better-auth.test.tspackages/auth-service/src/better-auth.tspackages/auth-service/src/lib/get-did-by-email.tspackages/auth-service/src/lib/render-error.tspackages/auth-service/src/middleware/require-better-auth.tspackages/auth-service/src/routes/account-settings.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/complete.tspackages/auth-service/src/routes/login-page.ts
✅ Files skipped from review due to trivial changes (2)
- packages/auth-service/src/lib/render-error.ts
- packages/auth-service/src/tests/require-better-auth.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/auth-service/src/routes/complete.ts
- packages/auth-service/src/tests/better-auth-otp.test.ts
- packages/auth-service/src/tests/render-error.test.ts
- docs/design/testing-gaps.md
- packages/auth-service/src/middleware/require-better-auth.ts
- packages/auth-service/src/routes/account-settings.ts
Comment on lines
32
to
+33
| const data = (await res.json()) as { did: string | null } | ||
| return data.did | ||
| return { did: data.did } |
There was a problem hiding this comment.
Validate the 200-response payload before returning { did }.
The current cast can return { did: undefined } if the upstream payload shape drifts, which breaks the tri-state contract and downstream branching.
🔧 Proposed fix
- const data = (await res.json()) as { did: string | null }
- return { did: data.did }
+ const data = (await res.json()) as { did?: unknown }
+ if (data.did !== null && typeof data.did !== 'string') {
+ logger.warn('Invalid account-by-email payload from PDS')
+ return null
+ }
+ return { did: data.did }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/auth-service/src/lib/get-did-by-email.ts` around lines 32 - 33, The
response JSON is being blindly cast and may contain an unexpected shape; in
getDidByEmail replace the cast with a runtime check: parse res.json() into a
local (data), verify Object.prototype.hasOwnProperty.call(data, 'did') and then
if typeof data.did === 'string' return { did: data.did }, else if data.did ===
null return { did: null }, otherwise return { did: undefined } (and optionally
log a warning or throw if you prefer a hard failure). This uses the existing
data variable and getDidByEmail return so downstream consumers get a correct
tri-state value instead of possibly undefined from a bad cast.
Contributor
Author
There was a problem hiding this comment.
this isnt a major issue. we cast because we are the ones controlling the fetch for getDidByEmail and it always returns in the format of {did: string | null}
There was a problem hiding this comment.
@Kzoeps, that's a fair point — since /_internal/account-by-email is an internal endpoint you own and control, the shape of the response is a reliable contract rather than an untrusted external payload. The as { did: string | null } cast is perfectly reasonable here. I over-stated the severity. Acknowledged!
✏️ Learnings added
Learnt from: Kzoeps
Repo: hypercerts-org/ePDS PR: 32
File: packages/auth-service/src/lib/get-did-by-email.ts:32-33
Timestamp: 2026-03-24T16:02:49.372Z
Learning: In `packages/auth-service/src/lib/get-did-by-email.ts` (hypercerts-org/ePDS): The `as { did: string | null }` cast in `getDidByEmail` is intentional. The `/_internal/account-by-email` PDS endpoint is an internal endpoint owned and controlled by the same team, so the response shape is a reliable contract. No runtime payload validation is needed; do not flag this cast as a defect.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Kzoeps
Repo: hypercerts-org/ePDS PR: 14
File: packages/auth-service/src/better-auth.ts:152-153
Timestamp: 2026-03-18T06:14:29.620Z
Learning: In `packages/auth-service/src/better-auth.ts` (hypercerts-org/ePDS): The `database: betterAuthDb as any` cast in `createBetterAuth()` is intentional and required. Removing it triggers TypeScript error TS4058 because `BetterSqlite3.Database` from `types/better-sqlite3` leaks into the inferred return type of the exported `createBetterAuth` function, and TypeScript cannot name that type in `.d.ts` declaration emit. The `as any` cast breaks the inference chain at the point of the `betterAuth()` call, preventing the TS4058 error. The `eslint-disable` comment should document this reason. This does NOT affect `runBetterAuthMigrations` because that function returns `Promise<void>` explicitly and the TS4058 issue does not apply there.
Learnt from: CR
Repo: hypercerts-org/ePDS PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-18T17:28:47.443Z
Learning: Applies to packages/{pds-core,auth-service}/src/**/*.ts : New PDS accounts require a real password passed to `createAccount()` (use `randomBytes(32).toString('hex')`); passing `undefined` skips `registerAccount()` and breaks `upsertDeviceAccount()` FK constraints
Learnt from: CR
Repo: hypercerts-org/ePDS PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-18T17:28:47.443Z
Learning: Applies to **/*.{ts,tsx} : `as any` casts are permitted only when working with untyped internals of `atproto/oauth-provider` branded types, with a comment explaining why
Learnt from: CR
Repo: hypercerts-org/ePDS PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-18T17:28:47.443Z
Learning: Applies to packages/auth-service/src/**/*.ts : Auth service must use `PDS_INTERNAL_URL` to reach pds-core over the internal network (Docker: `http://core:3000`, Railway: `http://<service>.railway.internal:3000`); without it, internal API calls fall back to public URL which is unreachable
Learnt from: CR
Repo: hypercerts-org/ePDS PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-18T17:28:47.443Z
Learning: Applies to packages/{pds-core,auth-service}/**/*.ts : Do not directly read or modify `atproto/pds` database tables; use `pds.ctx.accountManager.*` methods instead
Learnt from: Kzoeps
Repo: hypercerts-org/ePDS PR: 29
File: packages/auth-service/src/routes/login-page.ts:92-96
Timestamp: 2026-03-17T10:17:43.948Z
Learning: In `packages/auth-service/src/routes/login-page.ts` (hypercerts-org/ePDS): `epds_handle_mode` is delivered only via query parameter (`req.query.epds_handle_mode`), never via the Pushed Authorization Request (PAR) body. Do not flag missing PAR extraction for this parameter — clients do not send it via PAR. The issue `#22` spec mention of PAR as a delivery channel is not implemented.
Learnt from: Kzoeps
Repo: hypercerts-org/ePDS PR: 12
File: packages/auth-service/src/routes/choose-handle.ts:279-306
Timestamp: 2026-03-12T07:21:30.286Z
Learning: In `packages/auth-service/src/routes/choose-handle.ts` and `packages/auth-service/src/routes/otp-verification.ts` (hypercerts-org/ePDS): Do NOT call `ctx.db.recordClientLogin(email, clientId)` here. `recordClientLogin` must be called only on the consent page (after the user grants consent), not on the choose-handle or OTP-verification step. There is also a known bug where new users are not being shown the consent page — this is tracked in PR `#11`.
Contributor
Author
i think that amount of repetition is fine? i checked it out and i dont think it requires extracting to a function. |
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
1 participant
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.
Summary
When a user signs into /account with a valid better-auth session but the underlying PDS account no longer exists for that email (e.g. the account was deleted out-of-band, or the email was changed via XRPC without going through /account), the previous code had no handling for this case. The user would land on the account settings page with no DID — seeing handle update and backup email features that silently fail — or get caught in a redirect loop with no explanation.
Changes
This PR makes that failure mode explicit and recoverable:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation