Skip to content

Commit be040b1

Browse files
aspiersclaude
andcommitted
docs: address CodeRabbit and Copilot review feedback
- Reflect current code: magic_account_session no longer exists (historical note only), /_magic/check-email replaced by /_internal/account-by-email, 600s OTP TTL sourced to better-auth.ts not auth-flow.ts. - AUTH_HOSTNAME subdomain is a recommended production relationship, not a hard runtime requirement (unrelated hostnames supported with null cookie domain). - Scope the "in-process calls / fewer privileged endpoints" benefit to process-merge; origin-merge alone keeps the HTTP boundary. - Add path-shadowing note (auth surface must move under /auth so it doesn't shadow pds-core's /oauth/* and /account*) and an explicit Set-Cookie contract (unique names, no Domain, __Host- vs /auth path-scope exclusivity). - Require transition redirects to preserve full path + query string. - Distinguish origin/base-URL/hostname config values (AUTH_PATH_PREFIX / AUTH_BASE_URL) instead of concatenating a path onto a hostname. - Single-process merge: preserve one-time issuance gating in the direct path, and retain HMAC + signed callback route until legacy callers are drained (compatibility gate). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent ad09939 commit be040b1

3 files changed

Lines changed: 125 additions & 49 deletions

File tree

docs/architecture.md

Lines changed: 31 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -75,9 +75,16 @@ endpoint to issue an AT Protocol authorization code. For new users, a handle-pic
7575
`/oauth/epds-callback` is signed with `EPDS_CALLBACK_SECRET` so PDS Core can verify
7676
it was produced by a legitimate auth flow.
7777

78-
- **Auth Service on a subdomain, not a single shared domain**: `AUTH_HOSTNAME`
79-
must be a subdomain of `PDS_HOSTNAME` (e.g. `auth.pds.example` /
80-
`pds.example`). A single-domain design — one origin, routing the auth paths
78+
- **Auth Service on a subdomain, not a single shared domain**: the
79+
recommended production relationship is `AUTH_HOSTNAME` as a subdomain of
80+
`PDS_HOSTNAME` (e.g. `auth.pds.example` / `pds.example`). This is not a hard
81+
runtime requirement — the code also supports unrelated hostnames (e.g.
82+
Railway preview envs where both services sit under `up.railway.app`), in
83+
which case the shared cookie domain is `null` and device cookies stay
84+
host-only (`deriveCookieDomain` in `pds-core/src/cookie-domain.ts`). The
85+
cross-subdomain **device-session reuse** path only works when the subdomain
86+
relationship holds, which is why it is the recommended arrangement. A
87+
single-domain design — one origin, routing the auth paths
8188
by path prefix the way [pds-gatekeeper](https://tangled.org/baileytownsend.dev/pds-gatekeeper)
8289
does — was **considered and rejected**. The reasons:
8390
- **Mechanism (how the takeover happens)**: PDS Core overrides the
@@ -89,7 +96,8 @@ endpoint to issue an AT Protocol authorization code. For new users, a handle-pic
8996
motivation.
9097

9198
- **Cookie isolation (the motivation)**: the Auth Service's session cookies
92-
(Better Auth's `session`, historically `magic_account_session`) are a
99+
(the Better Auth session cookie; historically `magic_account_session`,
100+
no longer present in code) are a
93101
_separate authentication domain_ from AT Protocol access/refresh tokens.
94102
Giving the Auth Service its own origin keeps the two cookie namespaces from
95103
colliding by construction. (Note the tension: cross-subdomain cookie
@@ -117,12 +125,17 @@ endpoint to issue an AT Protocol authorization code. For new users, a handle-pic
117125
**On re-examination, none of the three benefits is a non-negotiable, and all
118126
the costs are artifacts of being cross-origin-but-same-site:**
119127
- _Cookie isolation_ is already done by explicit cookie **naming**
120-
(`epds_csrf`, `magic_account_session`, the `DEVICE_COOKIE_NAMES` set), not
121-
by origin — so it survives a collapse to one origin, optionally hardened
122-
with a `__Host-`/path scope. The split does not just fail to be _required_
123-
for isolation; it actively _creates_ a cookie problem — the
124-
`Domain=<parent>` sharing in items 14–15 — that a single origin deletes
125-
outright.
128+
(`epds_csrf`, `epds_auth_flow`, the Better Auth session cookie, and the
129+
`DEVICE_COOKIE_NAMES` device-session set), not by origin — so it survives a
130+
collapse to one origin. Concretely, on a shared origin the contract is:
131+
guaranteed-unique names, and **no parent `Domain` attribute** (cookies stay
132+
host-only). This unique-name isolation is distinct from — and does not
133+
require — `__Host-` prefixing; note `__Host-` mandates `Path=/` and so
134+
cannot be combined with a `/auth` path scope, whereas plain path scoping
135+
(`Path=/auth`) is an available but separate hardening option. The split
136+
does not just fail to be _required_ for isolation; it actively _creates_ a
137+
cookie problem — the `Domain=<parent>` sharing in items 14–15 — that a
138+
single origin deletes outright.
126139
- _Security-header isolation_ is already per-route in both packages
127140
(`auth-service/src/lib/security-headers.ts` sets an auth-specific CSP per
128141
request; `pds-core` already rewrites the upstream CSP per response in
@@ -139,12 +152,14 @@ endpoint to issue an AT Protocol authorization code. For new users, a handle-pic
139152
benefits the split forecloses:
140153
- **Fewer privileged cross-service endpoints.** The split forces the two
141154
services to talk over authenticated HTTP: the HMAC-signed
142-
`/oauth/epds-callback` and the `/_internal/*` / `/_magic/*` lookup
143-
endpoints (e.g. `account-by-email`, `check-email`), each an attack surface
144-
that has to be signed, gated, and kept in sync. Co-located on one origin,
145-
much of this can become in-process calls with no wire boundary to secure.
146-
(Whether to also merge the _processes_ — versus keep two behind path
147-
routing — is a separate decision; see the migration doc.)
155+
`/oauth/epds-callback` and the `/_internal/*` lookup endpoints (e.g.
156+
`/_internal/account-by-email`, which replaced the old unauthenticated
157+
`/_magic/check-email`), each an attack surface that has to be signed,
158+
gated, and kept in sync. **Note this benefit requires merging the
159+
_processes_, not just the origin**: a single origin with two processes
160+
still has an HTTP boundary (merely same-origin now). Only a process merge
161+
turns these into in-process calls with no wire boundary to secure — a
162+
separate decision from origin-merging; see the migration doc.
148163

149164
- **Operational simplicity.** One origin means one TLS cert, one DNS record,
150165
one CSP surface, and one set of cross-origin edge cases to reason about,

docs/design/single-domain-migration.md

Lines changed: 65 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -13,20 +13,46 @@ that case is accepted and focuses on _how_.
1313

1414
## Summary of the change
1515

16-
| Concern | Today (two origins) | After (one origin) |
17-
| -------------------------------------- | ------------------------------------------------------------------- | --------------------------------------------------------------------- |
18-
| Auth UI location | `https://auth.pds.example/oauth/authorize`, `/auth/*`, `/account/*` | `https://pds.example/auth/*` (path-prefixed) |
19-
| `authorization_endpoint` (AS metadata) | `https://auth.<host>/oauth/authorize` | `https://<host>/auth/oauth/authorize` |
20-
| `sec-fetch-site` on `/oauth/authorize` | `same-site`, rewritten to `same-origin` (item 5) | naturally `same-origin` — rewrite **deleted** |
21-
| Device-session cookies | `Domain=<parent>` so sibling subdomain can read them (items 14–15) | host-only on the single origin — **plumbing deleted** |
22-
| Auth session cookie | `magic_account_session` / better-auth `session` on `auth.<host>` | same names, path-scoped `/auth` on `<host>` |
23-
| CSP | per-route in each service (unchanged approach) | per-route, unchanged |
24-
| Deploy units | two services | still two processes, one origin (Caddy path-routes) — or merged later |
16+
| Concern | Today (two origins) | After (one origin) |
17+
| -------------------------------------- | ---------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------- |
18+
| Auth UI location | `https://auth.pds.example/oauth/authorize`, `/auth/*`, `/account/*` | `https://pds.example/auth/*` — including `/auth/oauth/authorize` and `/auth/account/*` (see path-shadowing note below) |
19+
| `authorization_endpoint` (AS metadata) | `https://auth.<host>/oauth/authorize` | `https://<host>/auth/oauth/authorize` |
20+
| `sec-fetch-site` on `/oauth/authorize` | `same-site`, rewritten to `same-origin` (item 5) | naturally `same-origin` — rewrite **deleted** |
21+
| Device-session cookies | `Domain=<parent>` so sibling subdomain can read them (items 14–15) | host-only on the single origin — **plumbing deleted** (see Set-Cookie contract below) |
22+
| Auth session cookie | Better Auth session cookie (historically `magic_account_session`) on `auth.<host>` | same cookie, host-only on `<host>` (see Set-Cookie contract below) |
23+
| CSP | per-route in each service (unchanged approach) | per-route, unchanged |
24+
| Deploy units | two services | still two processes, one origin (Caddy path-routes) — or merged later |
2525

2626
The routing model is exactly pds-gatekeeper's: **Caddy routes by path**, sending
27-
`/auth/*` (and the auth-owned `/oauth/authorize`, `/account/*`) to the
28-
auth-service process and everything else to the PDS. The auth-service stays a
29-
separate process; only its _origin_ changes.
27+
the auth surface to the auth-service process and everything else to the PDS. The
28+
auth-service stays a separate process; only its _origin_ changes.
29+
30+
### Path shadowing — the auth surface must move under `/auth`
31+
32+
On a single origin the auth service **cannot** keep serving `/oauth/authorize`
33+
and `/account/*` at the root: upstream `@atproto/oauth-provider` already renders
34+
`/oauth/*` and `/account*` on pds-core, so routing those roots to auth-service
35+
would shadow the PDS's own endpoints. The migration therefore exposes the entire
36+
auth surface under a non-conflicting prefix — `/auth/oauth/authorize`,
37+
`/auth/account/*`, `/auth/*` — optionally with a Caddy path-strip rewrite so the
38+
auth-service process can keep its current internal route paths unchanged.
39+
40+
### Set-Cookie contract on the merged origin
41+
42+
With one origin, the cookie isolation that the subdomain previously provided by
43+
separation must instead be provided by the cookie attributes:
44+
45+
- **Auth session cookie** (Better Auth) and **CSRF / auth-flow cookies**
46+
(`epds_csrf`, `epds_auth_flow`): guaranteed-**unique names**, **no `Domain`
47+
attribute** (host-only), `Secure`, `HttpOnly` where applicable,
48+
`SameSite=Lax`. Optionally scope non-`__Host-` cookies with `Path=/auth`.
49+
- **Device-session cookies** (`DEVICE_COOKIE_NAMES`): host-only — emit with **no
50+
`Domain` attribute** at all, deleting the `Domain=<parent>` broadening (items
51+
14–15).
52+
- `__Host-` prefixing is a possible hardening but is **mutually exclusive with a
53+
`/auth` path scope**: `__Host-` requires `Path=/`. Pick unique-name +
54+
host-only as the baseline; add either `__Host-` (at `Path=/`) or `Path=/auth`
55+
scoping, not both.
3056

3157
## Two levels of "merge" — keep them distinct
3258

@@ -46,7 +72,8 @@ This matters for both benefits and risks, so fix the terms up front:
4672
- **Fewer privileged cross-service endpoints.** The split _requires_ the two
4773
services to communicate over authenticated HTTP: the HMAC-signed
4874
`/oauth/epds-callback` plus the internal lookup endpoints (`/_internal/*`,
49-
`/_magic/check-email`, `account-by-email`). Each is an attack surface that
75+
e.g. `/_internal/account-by-email`, which replaced the old unauthenticated
76+
`/_magic/check-email`). Each is an attack surface that
5077
must be signed, gated, and version-matched across the boundary. Merging the
5178
origin lets some of these relax; merging the processes lets most become
5279
in-process calls with no wire boundary at all.
@@ -87,7 +114,7 @@ The whole point. These exist solely because of cross-origin-same-site:
87114
`pds-core/src/index.ts:930`, and `authOrigin` derivation in
88115
`chooser-enrichment.ts`. The two hostnames become one.
89116
4. **(Process-merge only) some privileged cross-service endpoints** — the
90-
`/_internal/*` / `/_magic/*` HTTP lookups become in-process calls. The
117+
`/_internal/*` HTTP lookups become in-process calls. The
91118
HMAC-signed `/oauth/epds-callback` can also collapse to an in-process call
92119
if the processes merge. See "Two levels of merge" above.
93120

@@ -116,28 +143,43 @@ would strand in-flight and cached clients.
116143
**Mitigation:** keep `auth.<host>` resolving during a transition window. Serve a
117144
301/302 (or continue proxying) from `auth.<host>/oauth/authorize`
118145
`<host>/auth/oauth/authorize` until metadata TTLs expire and clients re-fetch.
119-
Only retire the subdomain DNS/cert after the window.
146+
The redirect/proxy **must preserve the complete request target** — full path and
147+
query string — because the authorization request carries `client_id`,
148+
`redirect_uri`, `code_challenge`, `state`, etc.; dropping the query breaks
149+
in-flight logins. Only retire the subdomain DNS/cert after the window.
120150

121151
### 2. `EPDS_LINK_BASE_URL` appears in live email links
122152

123153
`auth-service/.env.example:94``EPDS_LINK_BASE_URL=https://auth.pds.example/auth/verify`.
124154
Verification/recovery links already delivered to inboxes point at the subdomain.
125155

126-
**Mitigation:** same transition redirect covers these. Update the env var to the
127-
new path-based URL for _new_ emails; keep the subdomain redirecting for the
128-
lifetime of the longest-lived link (OTP/verification TTLs are short — 600s per
129-
`auth-flow.ts` — so this window is small).
156+
**Mitigation:** same transition redirect covers these — and, as above, it must
157+
**preserve the full path and query string** so the verification token in the
158+
link survives the hop. Update the env var to the new path-based URL for _new_
159+
emails; keep the subdomain redirecting for the lifetime of the longest-lived
160+
link (OTP/verification TTLs are short — `expiresIn: 600` in
161+
`auth-service/src/better-auth.ts` — so this window is small).
130162

131163
### 3. `AUTH_HOSTNAME` is load-bearing config
132164

133165
Referenced in `pds-core/src/index.ts:138`, `auth-service/src/index.ts:138`,
134166
`session-reuse.ts`, `chooser-enrichment.ts` (`authOrigin`),
135167
`sec-fetch-site-rewrite.ts` allowlist, `demo/.env.example` (`AUTH_ENDPOINT`).
136168

137-
**Approach:** introduce an `AUTH_PATH_PREFIX` (default `/auth`) and derive the
138-
auth origin as `PDS_HOSTNAME + AUTH_PATH_PREFIX`. Keep `AUTH_HOSTNAME` as an
139-
optional legacy override so existing subdomain deployments keep working during
140-
transition (config-gated, not a hard cutover).
169+
**Approach:** keep these as distinct values rather than conflating them —
170+
`PDS_HOSTNAME + AUTH_PATH_PREFIX` yields `pds.example/auth`, which is a **base
171+
URL/path, not an origin**, and today's `AuthServiceConfig.hostname` expects a
172+
bare hostname while public-URL config carries the scheme. So:
173+
174+
- keep `AuthServiceConfig.hostname` a hostname;
175+
- introduce `AUTH_PATH_PREFIX` (default `/auth`) as the path the auth surface
176+
mounts under;
177+
- derive the full mount point from an explicit **origin** (`https://<host>`)
178+
joined with the prefix — or introduce an explicit `AUTH_BASE_URL` — and update
179+
URL-joining so paths and origins are never concatenated as bare strings.
180+
181+
Keep `AUTH_HOSTNAME` as an optional legacy override so existing subdomain
182+
deployments keep working during transition (config-gated, not a hard cutover).
141183

142184
### Non-blockers (confirmed)
143185

docs/design/single-process-merge.md

Lines changed: 29 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,13 @@ adds a distinct set of wins that single-origin alone cannot deliver:
2222
auth-service can prove to pds-core that a redirect came from a legitimate auth
2323
flow, over the wire, signed with `EPDS_CALLBACK_SECRET`. In one process the
2424
auth flow can call the code-issuance path **directly** as a function — no
25-
HMAC, no secret to rotate, no signature-verification code, no replay window.
26-
- **The internal HTTP lookups disappear.** `/_internal/account-by-email`,
27-
`/_magic/check-email`, and the `/_internal/ping-request` keepalive become
25+
HMAC, no secret to rotate, no signature-verification code. Note this removes
26+
only the cross-service _authenticity_ check; the one-time, session-bound
27+
issuance gating must be preserved in the direct path (see "Preserving
28+
one-time issuance" below).
29+
- **The internal HTTP lookups disappear.** `/_internal/account-by-email` (which
30+
replaced the old unauthenticated `/_magic/check-email`) and the
31+
`/_internal/ping-request` keepalive become
2832
in-process function calls against the same objects (`pds.ctx.accountManager`,
2933
`provider.requestManager`) that pds-core already holds. No auth, no JSON
3034
round-trip, no drift between caller and callee.
@@ -132,9 +136,21 @@ Merged: the auth-complete handler calls the same `setAuthorized` path
132136
`requestManager.setAuthorized` / `createAccount`) into a plain function on a
133137
shared module, taking typed args instead of a signed request.
134138
- The auth `/auth/complete` handler calls that function directly.
135-
- `EPDS_CALLBACK_SECRET` and the signature-verification middleware are deleted.
136-
- **Keep the HTTP `/oauth/epds-callback` route only if** a transition period
137-
needs old auth-service instances to still call it; otherwise remove it.
139+
- **Preserve the one-time issuance gating in the extracted function.** Dropping
140+
HMAC removes only the cross-service _authenticity_ check — it does **not**
141+
remove the need for one-time, session-bound, idempotent guards. The direct
142+
path must still ensure a repeated or replayed `/auth/complete` cannot mint a
143+
second authorization code (the auth-flow row is consumed exactly once, keyed
144+
to the session). Carry these guards into the shared function; they are
145+
independent of the HMAC and must not be deleted alongside it.
146+
- **Retain `EPDS_CALLBACK_SECRET`, the signature-verification middleware, and
147+
the HTTP `/oauth/epds-callback` route while any legacy Auth instance can
148+
still call it.** During a rolling deploy an old auth-service may still POST
149+
the signed callback; deleting the verifier early would either break it or
150+
(worse) expose an unauthenticated issuance route. Remove the secret, the
151+
verifier, and the route **only** in a later compatibility gate, after all old
152+
callers are drained and the direct-call path is fully deployed (see rollout
153+
steps 4–5).
138154

139155
This is the highest-value deletion in the whole merge and also the most
140156
security-sensitive change — the HMAC existed to stop an attacker forging a
@@ -172,10 +188,13 @@ come first. This process-merge adds:
172188
mounted at `/auth` on the PDS app; resolve hazards 1–6 above. Keep
173189
auth-service's standalone `listen()` behind a flag for rollback.
174190
4. **Switch `/auth/complete` to the in-process call.** Route code issuance
175-
through the extracted function; stop signing/sending the HMAC callback.
176-
5. **Delete** `EPDS_CALLBACK_SECRET`, the callback signature middleware, the
177-
`/_internal/*` + `/_magic/*` HTTP endpoints, and auth-service's standalone
178-
server. Retire the second process.
191+
through the extracted function; stop signing/sending the HMAC callback. The
192+
signed HTTP route stays live and verified here — it is not yet removed.
193+
5. **Compatibility gate — delete only once legacy callers are drained.** After
194+
confirming no old auth-service instance still POSTs the callback, delete
195+
`EPDS_CALLBACK_SECRET`, the callback signature middleware, the HTTP
196+
`/oauth/epds-callback` route, the `/_internal/*` HTTP endpoints, and
197+
auth-service's standalone server. Retire the second process.
179198

180199
Each step is independently shippable and reversible until step 5.
181200

0 commit comments

Comments
 (0)