docs: recover, re-examine, and spec migration off the auth subdomain split - #201
docs: recover, re-examine, and spec migration off the auth subdomain split#201aspiers wants to merge 10 commits into
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR documents the tradeoffs of subdomain-based auth hosting and proposes a phased transition to single-origin routing, followed by an optional single-process merge with Express integration, callback-boundary, dependency, and rollout considerations. ChangesAuth architecture consolidation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@docs/architecture.md`:
- Around line 143-150: Update the “Fewer privileged cross-service endpoints”
paragraph to clarify that co-locating services on one origin does not remove the
HMAC-signed and internal HTTP boundaries while they remain separate processes.
Make in-process calls and the reduced endpoint attack surface explicitly
conditional on merging the processes, while preserving the separate path-routing
decision.
- Around line 78-80: Update the “Auth Service on a subdomain” guidance in the
architecture documentation so AUTH_HOSTNAME being a subdomain of PDS_HOSTNAME is
described as a recommended production relationship or legacy deployment
constraint, not a mandatory runtime requirement. Preserve support for unrelated
hostnames and the resulting null shared-cookie domain documented by the
implementation in packages/pds-core/src/index.ts.
- Around line 121-127: Define one precise cookie-isolation contract across both
documentation sites: in docs/architecture.md lines 121-127, distinguish unique
cookie-name isolation from __Host- cookies and path-scoped alternatives,
including that __Host- requires Path=/ and cannot use /auth; in
docs/design/single-domain-migration.md lines 21-22, document the exact
Set-Cookie attributes for Auth session and device-session cookies, requiring
unique names and omitting any parent Domain attribute.
In `@docs/design/single-domain-migration.md`:
- Around line 137-140: Update the migration design’s auth configuration
terminology and URL construction: keep AuthServiceConfig.hostname as a hostname,
define separate origin and AUTH_PATH_PREFIX values (or an explicit
AUTH_BASE_URL), and join them without conflating paths with origins. Preserve
AUTH_HOSTNAME as the optional legacy override for existing subdomain
deployments.
- Around line 116-129: Update the transition mitigation for auth.<host> in the
OAuth and email-link sections to explicitly require preserving the complete
request target, including the path and query string. Ensure redirects/proxying
retain OAuth parameters and verification-link tokens while transitioning to the
path-based URLs.
In `@docs/design/single-process-merge.md`:
- Around line 135-143: Update the single-process merge design to retain
EPDS_CALLBACK_SECRET, the signature-verification middleware, and HMAC validation
for the HTTP /oauth/epds-callback route while legacy Auth instances can still
call it. Remove these compatibility protections only after all old callers are
drained and the direct-call path is fully deployed, using that migration state
as the compatibility gate.
- Around line 21-25: Update the single-process issuance design around the auth
flow and `/auth/complete` path to retain one-time, session-bound, idempotent
guards when the code-issuance function is called directly. Remove only the HMAC
boundary and related cross-service verification; ensure repeated completion
requests cannot mint more than once.
🪄 Autofix (Beta)
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
Run ID: 6dcaf186-566d-4783-a17b-cf8bbc32dd14
📒 Files selected for processing (3)
docs/architecture.mddocs/design/single-domain-migration.mddocs/design/single-process-merge.md
There was a problem hiding this comment.
Pull request overview
This docs-only PR records and re-evaluates the rationale for running auth-service on a sibling subdomain (auth.<host>) and specifies two migration paths to collapse to a single origin (and optionally a single Node process), as tracked in #200.
Changes:
- Adds an architecture decision entry documenting the original motivations for the subdomain split, its concrete costs, and the re-examination conclusion.
- Introduces a single-origin (single-domain) migration design with phased rollout considerations for published URLs.
- Introduces an optional “single process” merge design detailing Express integration hazards and dependency-resolution risks.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| docs/architecture.md | Adds/updates the “Auth Service on a subdomain” decision record and links to the new migration design. |
| docs/design/single-domain-migration.md | New single-origin migration plan (path routing, blockers, phases). |
| docs/design/single-process-merge.md | New single-process merge plan (mounting auth onto pds.app, integration hazards, peer-dep risk). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Coverage Report for CI Build 30915758529Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.4%) to 57.672%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions56 previously-covered lines in 6 files lost coverage.
Coverage Stats
💛 - Coveralls |
|
🚅 Deployed to the ePDS-pr-201 environment in ePDS
|
The subdomain split (AUTH_HOSTNAME as a subdomain of PDS_HOSTNAME) was inherited whole from upstream magic-pds at the initial commit, and its rationale lived only in better-auth-migration-plan.md. Surface it in the main architecture doc as a Key Design Decision: cookie isolation, security-header isolation, and independent deployability — distinct from the authorization_endpoint metadata override, which is the mechanism, not the motivation. Note the accepted costs (same-site rewrite, cross-subdomain cookie plumbing) and link the open re-evaluation (#200). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
None of the three benefits (cookie isolation, security-header isolation, independent deployability) is a non-negotiable — all are reproducible on a single origin, and every documented cost (sec-fetch-site rewrite, cross-subdomain cookie plumbing) is an artifact of being cross-origin-but-same-site. Strengthen the architecture.md decision with this analysis and add docs/design/single-domain-migration.md laying out the collapse to a path-prefixed single origin, the real blockers (published authorization_endpoint, email links, AUTH_HOSTNAME config), and a phased rollout. Tracked in #200. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cover two further considerations: (1) merging removes privileged cross-service endpoints (HMAC callback, /_internal lookups) and yields operational simplicity, and (2) npm peer-dependency clashes are a risk only if the two *processes* merge, not if they stay separate behind path routing — bounded anyway by the pnpm non-flat workspace. Distinguish origin-merge (recommended, no version risk) from process-merge throughout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add single-process-merge.md covering the maximal collapse: mount the auth-service Express app onto pds.app under /auth, delete the HMAC-signed /oauth/epds-callback and /_internal/* HTTP hops in favour of in-process calls, and retire the second process. Documents the real integration hazards (better-auth body-parser ordering, /static and favicon collisions, trust-proxy/CSRF/error-handler scoping), the now-live npm peer-dependency risk, and a reversible phased rollout gated on a peer audit and a callback-core extraction refactor. Cross-linked from single-domain-migration.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- 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>
be040b1 to
9fe538e
Compare
| - **Merge the _origin_ (recommended baseline).** One hostname, Caddy path-routes | ||
| to two still-separate processes. Dissolves every cross-origin cost; keeps the | ||
| HMAC trust boundary; carries **no** npm-version risk. | ||
| - **Merge the _processes_ (optional, later).** Fold auth-service into the |
There was a problem hiding this comment.
I'm already assuming we won't do this since it's effectively forking the pds.
There was a problem hiding this comment.
It would not be a fork, in the same way that the existing pds-core package isn't a fork: it's a wrapper which does not contain the PDS codebase and instead just imports the relevant modules.
So I think it's definitely worth at least considering merging the processes.
There was a problem hiding this comment.
I'm skeptical we want to actually merge the processes since the router will likely involve moving better-auth into it (similar to how pds entryway holds login data for everything that delegates to it) and the isolation ePDS has works nicely with it since the logic is already there and if it's collapsed into a single process we risk introducing complexity for the sake of perceived simplicity (in particular, hitching onto internal pds processes feels like a can of worms in terms of scaling)
There was a problem hiding this comment.
There is a whole separate .md file which covers the benefits and risks of merging processes in detail - did you have a chance to review that yet? If you think there are specific things missing from that then it would be great if you could comment on that file.
Either way, the recommendation is to merge domains first, and then consider the process merge as a second step.
There was a problem hiding this comment.
@Ashex Would be great if you could get to this before Friday as this is a key part of the architectural discussion.
There was a problem hiding this comment.
(reply generated by Claude Opus 5 via Claude Code)
Not resolving this thread — it is an open architectural disagreement discussion for @Ashex and @aspiers to settle, not something an agent should close. Adding two factual updates that bear on the specific concerns raised above.
On "hitching onto internal pds processes feels like a can of worms": the doc now records three concrete instances of exactly that, added in 446c524 after review — merging the processes does not merge the SQLite transaction boundaries (a crash between the auth-flow write and the account write can consume flow state with no account); the two CSP middlewares would write the same response header, where setHeader overwrites rather than merges; and /_internal/* removal needs a per-endpoint drain because those routes have callers beyond the callback. These are arguments for the caution expressed here, now written down rather than left implicit.
On the cost of keeping the split: a live example landed in 18c6493, from HYPER-219 / #234. Recording that an email was verified had to cross the service boundary, and the natural implementation instead let pds-core infer it from the arrival of a valid signed callback — correct while an emailed code is the only sign-in path, silently wrong the moment a passkey flow sends a signed callback carrying an email merely to locate the account. Making it safe took a new required field in the HMAC payload, wiring through both producers, a consumer check, and fail-closed tests. That cost recurs per authentication mechanism.
Neither point settles the question, and the recommendation in the doc is unchanged: merge the origin first, treat the process merge as a separate later decision. Flagging them so the discussion is working from the current text.
There was a problem hiding this comment.
(reply generated by Claude Opus 5 via Claude Code)
Correction to my previous reply: I described this as an open disagreement. That was wrong — @aspiers has clarified that the concerns raised here are addressed in docs/design/single-process-merge.md, which just has not been read yet. It is a pending review, not a dispute.
Still not resolving the thread, since it is waiting on @Ashex rather than on any change to the PR.
The pointers in my earlier reply are all to sections of that file, so they may be easier to read there in context than here: the transaction-boundary, CSP-precedence and /_internal/* drain hazards are in "Integration hazards" and the rollout steps, and the recurring per-mechanism cost of the split is in "Auth facts across the boundary".
…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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
docs/design/single-domain-migration.md:115
parentCookieDomainis referenced here as a derived value inpds-core/src/index.ts, but the current code usescookieDomain(see the “Cookie domain broadening” section inpackages/pds-core/src/index.tsaround line 930). Referring to the real symbol avoids confusion when readers try to follow the reference.
3. **`authHostname.endsWith('.' + pdsHostname)` special-casing** — e.g.
`session-reuse.ts:195-200`, the `parentCookieDomain` derivation in
`pds-core/src/index.ts:930`, and `authOrigin` derivation in
`chooser-enrichment.ts`. The two hostnames become one.
docs/design/single-domain-migration.md:201
- The phased rollout step 2 says to route
/oauth/authorizeand/account/*to auth-service on the merged origin, but earlier in this doc the “Path shadowing” section explains those root paths conflict with (and would shadow) pds-core’s upstream/oauth/*and/account*endpoints. On a single origin, the auth surface should be exposed under/auth/*; if legacy root paths need support, they should redirect to/auth/...rather than being proxied to auth-service.
2. **Caddy path routing.** Add a single-origin Caddyfile variant that routes
`/auth/*`, `/oauth/authorize`, `/account/*` to auth-service and the rest to
the PDS. Stand it up in a test/preview env.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@docs/design/single-domain-migration.md`:
- Around line 35-38: Update the migration design’s path contract and Step 2
routing instructions so the shared origin routes only the non-conflicting
/auth/* prefix to auth-service; remove any routing of /oauth/authorize or
/account/* at the public root. Make stripping /auth before forwarding mandatory,
or explicitly require auth-service to register equivalent /auth-prefixed
upstream routes, and keep the same contract consistent in the additionally
affected section.
- Around line 151-160: Update the migration guidance around EPDS_LINK_BASE_URL
and its transition redirects to use path-specific mappings rather than a blanket
subdomain rule: route legacy /oauth/authorize to /auth/oauth/authorize, /auth/*
to /auth/*, and /account/* to /auth/account/*. Require each redirect to preserve
the complete path and query string, and ensure the new environment URL uses the
path-based form for newly generated links.
- Around line 21-22: Update the cookie handoff section near the device-session
and auth session cookie entries to explicitly define behavior when changing from
auth.<host> to <host>: in-flight flows and existing sessions must transition
through server-side handoff or proxying, or restart after a controlled drain.
Clarify that redirecting path and query alone does not transfer epds_csrf,
epds_auth_flow, or the existing auth session.
In `@docs/design/single-process-merge.md`:
- Around line 128-135: Expand the “Shared context / DB handles” section to
define transaction boundaries and recovery semantics for auth-service and
pds-core, covering commit order, idempotency keys, and failure/retry behavior
when SQLite handles remain separate. Specify the recovery tests required before
replacing the HTTP callback with in-process calls, while preserving the existing
account.sqlite and pds.ctx.accountManager guidance.
- Around line 121-126: Update the “CSP middleware convergence” section to define
which middleware owns Content-Security-Policy and which policy takes precedence
when both run; document and test the final header values separately for /auth/*
and PDS routes, including that auth’s unsafe-inline policy does not appear on
PDS responses.
- Around line 270-274: Update the compatibility gate in the design document to
separately gate removal of the PDS /_internal/* endpoints on their consumers:
inventory getDidByEmail, pingParRequest, and handle availability callers,
migrate each from HTTP to the corresponding in-process function, and require an
observed drain window before deleting the routes.
🪄 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: 2ac05be5-079c-483e-838c-92fa2a732183
📒 Files selected for processing (3)
docs/architecture.mddocs/design/single-domain-migration.mddocs/design/single-process-merge.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/architecture.md
The authorization_endpoint takeover forces PDS Core to police upstream's two unreachable auth UIs, producing brittle guards (auth-ui-guard.ts, white-boxing item 18; HYPER-367). Clarify this cost is orthogonal to the subdomain split: the opaque boundary is PDS Core <-> upstream atproto, so neither an origin merge nor a process merge removes it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Six review findings, all valid on inspection. single-domain-migration.md: - Step 2 routed /oauth/authorize and /account/* to auth-service, which the "Path shadowing" section two pages earlier says cannot be done -- those roots are served by upstream on pds-core. Route only /auth/*, and require an explicit choice between stripping the prefix and registering /auth-prefixed routes, since that choice determines the redirect targets in step 4. - Step 4's blanket auth.<host>/* -> <host>/auth/* redirect double- prefixes existing links: EPDS_LINK_BASE_URL is already https://auth.pds.example/auth/verify, so it would resolve to /auth/auth/verify and break every verification email in flight. Replace with a per-path mapping table. - The Set-Cookie contract covered steady state but not cutover. These cookies are host-only, so nothing set at auth.<host> is sent to <host>: a redirect carries the URL but not epds_csrf, epds_auth_flow or the session. Document the controlled drain (short OTP TTL makes it practical) versus a server-side handoff, and require the no-cookies arrival to degrade to "start sign-in again". single-process-merge.md: - CSP said the two policies "already coexist". They coexist because the processes are separate; on one Express app both write the same header and setHeader overwrites, so ordering silently decides the policy. Require path-scoped mounting, single-writer-per-request, and tests asserting the header on a real request rather than the middleware in isolation. - Merging processes does not merge transaction boundaries: the auth DB and account.sqlite stay separate, so a crash between the two writes can consume flow state with no account, or leave a replayable flow. Specify commit order, idempotency keys, and failure-injection recovery tests before replacing the callback. - Step 5 deleted /_internal/* alongside the callback. Callback drain says nothing about getDidByEmail, pingParRequest or handle lookups, which have their own callers; gate each endpoint on its own drain. Raised by CodeRabbit on PR #201.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (10)
docs/architecture.md:102
auth-ui-guard.tsis referenced without its package path; in-repo it lives underpackages/pds-core/src/. Using the full path avoids ambiguity for readers trying to jump to the implementation.
(`auth-ui-guard.ts`; [pds-white-boxing.md](design/pds-white-boxing.md)
docs/architecture.md:155
- These file references omit
packages/(andclient-css-injection.tsis undersrc/lib/). Updating them to the actual paths will keep the architecture doc accurate and make grepping/navigation straightforward.
- _Security-header isolation_ is already per-route in both packages
(`auth-service/src/lib/security-headers.ts` sets an auth-specific CSP per
request; `pds-core` already rewrites the upstream CSP per response in
`chooser-enrichment.ts` / `client-css-injection.ts`). Per-route CSP does
not need per-origin hosting.
docs/design/single-domain-migration.md:115
- These file references are missing their
packages/...prefixes. Using the full paths makes it clear which package owns the logic and makes the cited line numbers actionable.
- Today the repo is a **pnpm workspace** (`pnpm --recursive`) with pnpm's
default non-flat `node_modules`, so `auth-service` and `pds-core` already
resolve their dependencies independently.
docs/design/single-domain-migration.md:123
- The code reference
pds-core/src/index.ts:643doesn’t match the repo path (it’s underpackages/pds-core/...). Using the correct path makes this pointer easier to follow.
docs/design/single-domain-migration.md:139 - This path reference should include the
packages/prefix so readers can find the referenced code location in-repo.
`chooser-enrichment.ts`. The two hostnames become one.
docs/design/single-domain-migration.md:161
- The referenced file is
packages/auth-service/src/better-auth.ts(notauth-service/src/better-auth.ts). Updating the path keeps the TTL citation easy to verify.
### 1. `authorization_endpoint` is cached by OAuth clients
docs/design/single-domain-migration.md:167
- These references omit the
packages/prefix (anddemo/.env.exampleis actually underpackages/demo/). Using the exact repo paths will make these pointers actionable.
would strand in-flight and cached clients.
**Mitigation:** keep `auth.<host>` resolving during a transition window. Serve a
docs/architecture.md:84
- The referenced source path
pds-core/src/cookie-domain.tsdoesn’t match the repo layout (the file lives underpackages/pds-core/...). Using the correct path makes it easier for readers to locatederiveCookieDomain.
This issue also appears in the following locations of the same file:
- line 102
- line 151
host-only (`deriveCookieDomain` in `pds-core/src/cookie-domain.ts`). The
docs/design/single-domain-migration.md:110
- This reference to
auth-service/src/lib/session-reuse.tsis missing thepackages/prefix; the actual path ispackages/auth-service/src/lib/session-reuse.ts.
This issue also appears in the following locations of the same file:
- line 112
- line 123
- line 139
- line 165
docs/design/single-process-merge.md:89
- This comment points at
auth-service/src/index.ts, but elsewhere the doc uses the fullpackages/auth-service/...path. Updating it keeps file references consistent and accurate.
// auth-service/src/index.ts
The migration doc cited a `parentCookieDomain` derivation at pds-core/src/index.ts:930. No such symbol exists: the value is `cookieDomain`, derived by `deriveCookieDomain(authHostname, handleDomain)` from pds-core/src/cookie-domain.ts. Drop the line number too, since it had already drifted. Raised by Copilot on PR #201 (suppressed comment).
|
(comment generated by Claude Opus 5 via Claude Code) Addressed the two suppressed comments from the latest Copilot review, since they have no inline threads to reply on:
Note the phrasing differed slightly between the two reviewers: Copilot suggested legacy root paths should redirect to |
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (12)
docs/design/single-domain-migration.md:139
- This item cites
session-reuse.ts/chooser-enrichment.tswithout their real repo paths;chooser-enrichment.tsis also ambiguous without the package prefix. Using the fullpackages/...paths keeps these implementation pointers accurate.
3. **`authHostname.endsWith('.' + pdsHostname)` special-casing** — e.g.
`session-reuse.ts:195-200`, the `cookieDomain` derivation in
`pds-core/src/index.ts` (via `deriveCookieDomain(authHostname, handleDomain)`
in `pds-core/src/cookie-domain.ts`), and `authOrigin` derivation in
docs/design/single-domain-migration.md:181
- These citations point at paths that don’t exist (
auth-service/.env.example,auth-service/src/better-auth.ts). The actual files live underpackages/…, so the current references are hard to follow.
`auth-service/.env.example:94` — `EPDS_LINK_BASE_URL=https://auth.pds.example/auth/verify`.
Verification/recovery links already delivered to inboxes point at the subdomain.
**Mitigation:** same transition redirect covers these — and, as above, it must
docs/design/single-domain-migration.md:191
- This list of
AUTH_HOSTNAMEreferences contains at least one incorrect line citation (packages/auth-service/src/index.tsusesAUTH_HOSTNAMEat ~164, not 138) and several shorthand paths (demo/.env.example,sec-fetch-site-rewrite.ts) that don’t exist as written. Updating these to the realpackages/...locations will keep the doc actionable.
Referenced in `pds-core/src/index.ts:138`, `auth-service/src/index.ts:138`,
`session-reuse.ts`, `chooser-enrichment.ts` (`authOrigin`),
docs/architecture.md:103
auth-ui-guard.tsis referenced without its repo path; there is no top-levelauth-ui-guard.ts, so this is hard to locate. Consider pointing at the actual file (packages/pds-core/src/auth-ui-guard.ts) to keep the decision record traceable.
in upstream-private logic PDS Core cannot cleanly wrap, so the guard
(`auth-ui-guard.ts`; [pds-white-boxing.md](design/pds-white-boxing.md)
item 18) either mirrors that decision pre-route or detects it in the
docs/architecture.md:155
- The CSP implementation pointers here use shorthand paths (
auth-service/src/...,chooser-enrichment.ts,client-css-injection.ts) that don’t exist as written (e.g.client-css-injection.tslives underpackages/pds-core/src/lib/). Using the real file paths will keep this section verifiable.
- _Security-header isolation_ is already per-route in both packages
(`auth-service/src/lib/security-headers.ts` sets an auth-specific CSP per
request; `pds-core` already rewrites the upstream CSP per response in
`chooser-enrichment.ts` / `client-css-injection.ts`). Per-route CSP does
not need per-origin hosting.
docs/design/single-process-merge.md:106
- Auth currently serves the favicon at
/favicon.ico(not/favicon). If the intent is to avoid collisions when mounted under/auth, the namespaced path should also be/auth/favicon.icofor consistency with the existing route.
Both apps do `app.use('/static', express.static(publicDir))` and serve a
favicon. Merged, these collide. Namespace the auth assets under `/auth/static`
(and `/auth/favicon`) so the two static roots don't shadow each other.
docs/design/single-domain-migration.md:163
- This citation uses
pds-core/src/index.ts:643, but that path doesn’t exist in the repo (it’spackages/pds-core/src/index.ts).
docs/design/single-domain-migration.md:263 - The fixture references here (
preview.ts,preview-emails.ts,demo/.env.example) are ambiguous and at least one is not a valid repo path (demo/.env.examplelives underpackages/demo/). Using the concrete file locations makes it easier to audit what must change during migration.
the _origin_.
- Preview/e2e environments assume the subdomain in several fixtures
(`preview.ts`, `preview-emails.ts`, `demo/.env.example`). These need updating
docs/design/single-domain-migration.md:147
pds-core/src/index.tsisn’t a real repo path (the file is underpackages/pds-core/...). Using the correct path keeps this pointer actionable.
docs/design/single-domain-migration.md:135- The file references in this deletion item use paths that don’t exist in the repo (e.g.
pds-core/auth-service/src/...). Using the actualpackages/...paths makes the doc navigable and avoids confusion during implementation.
This issue also appears in the following locations of the same file:
- line 136
- line 147
- line 163
- line 177
- line 189
- ...and 1 more
2. **Cross-subdomain device-cookie plumbing** (white-boxing items 14–15) — the
`Domain=<parent>` broadening in `pds-core` and the sibling-reading logic in
`auth-service/src/lib/session-reuse.ts`. On one origin the device session is
directly readable; cookies stay host-only.
docs/architecture.md:84
- This parenthetical cites
pds-core/src/cookie-domain.ts, but that path doesn’t exist in the repo (it’s underpackages/pds-core/...). Updating the reference avoids sending readers to a dead path.
This issue also appears in the following locations of the same file:
- line 101
- line 151
Railway preview envs where both services sit under `up.railway.app`), in
which case the shared cookie domain is `null` and device cookies stay
host-only (`deriveCookieDomain` in `pds-core/src/cookie-domain.ts`). The
docs/design/single-process-merge.md:92
- The code sample comment points at
auth-service/src/index.ts, but the actual file ispackages/auth-service/src/index.ts. Using the correct path makes the snippet easier to verify against the codebase.
This issue also appears on line 104 of the same file.
```ts
// auth-service/src/index.ts
app.all('/api/auth/*', toNodeHandler(betterAuthInstance)) // BEFORE json()
app.use(express.urlencoded({ extended: true }))
app.use(express.json())
What
Documents-only PR. Recovers, records, and re-examines why the ePDS auth service runs on its own subdomain (
auth.<host>) separate from the PDS, and specs the path to collapsing the split.Closes the investigation tracked in #200.
Why
The subdomain split was inherited whole from the upstream
magic-pdsproject at ePDS's first commit; its rationale lived only inbetter-auth-migration-plan.md. This surfaces it in the main architecture docs and, on re-examination, finds none of the three stated benefits is a non-negotiable — all are reproducible on a single origin, and every documented cost (sec-fetch-siterewrite, cross-subdomain cookie plumbing;pds-white-boxing.mditems 5, 14, 15) is an artifact of being cross-origin-but-same-site.Changes
docs/architecture.md— new "Auth Service on a subdomain" decision entry: the three benefits, theauthorization_endpointoverride as mechanism not motivation, the accepted costs, and the re-examination (benefits reproducible on one origin; merge upside: fewer privileged endpoints + operational simplicity; npm-version risk scoped to process-merge only).docs/design/single-domain-migration.md— full single-origin migration design: origin-merge vs. process-merge distinction, what gets deleted, external-addressability blockers (authorization_endpoint,EPDS_LINK_BASE_URL,AUTH_HOSTNAME), confirmed non-blockers (no AT Protocol identity coupling), phased rollout.docs/design/single-process-merge.md— maximal collapse to one origin and one Node process: mount the auth Express app ontopds.appunder/auth, replace the HMAC/oauth/epds-callbackand/_internal/*HTTP hops with in-process calls, and the real integration hazards (better-auth body-parser ordering,/static/favicon collisions,trust proxy/CSRF/error-handler scoping, peer-dependency audit).Scope
No code changes — design/decision docs only. No behaviour change. The migrations described are proposals gated on the phased rollouts in each doc; nothing here commits to executing them.
Companion
A shareable web summary (exec summary + verdict table) of this review was produced alongside these docs for team discussion.
🤖 Generated with Claude Code
Summary by CodeRabbit
Summary by CodeRabbit
/auth/*path with phased rollout and transition safeguards.