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 6 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 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 |
![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 29753966499Coverage remained the same at 57.272%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. 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.
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