fix(core): honor siteUrl in /.well-known OAuth discovery on the anonymous fast path#2030
fix(core): honor siteUrl in /.well-known OAuth discovery on the anonymous fast path#2030swissky wants to merge 1 commit into
Conversation
…mous fast path The middleware's anonymous fast path attaches locals.emdash without `config`, and the root-level /.well-known routes are public by design — so MCP clients always hit them with locals.emdash?.config undefined, getPublicOrigin() fell through to url.origin, and behind Cloudflare's proxy the discovery document advertised http:// (clients refuse to attach). Fall back to the build-time virtual config, which carries the origin-normalized siteUrl. Also corrects the public-url.ts comment (process.env IS readable on Workers with nodejs_compat_populate_process_env) and documents the Workers env-var setup in the siteUrl reference. Fixes emdash-cms#2016
🦋 Changeset detectedLatest commit: b81333b The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
area/core
area/docs
size/M
review/needs-review
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approach: This is the right fix for the right problem, and it fits EmDash's architecture.
The two OAuth discovery routes (/.well-known/oauth-protected-resource and /.well-known/oauth-authorization-server/_emdash) are registered at the site root (confirmed in routes.ts), so they land on the middleware's anonymous fast path. I traced middleware.ts: on that path locals.emdash is set to a handlers-only object (handlePublicPluginApiRoute, collectPageMetadata, collectPageFragments, getPublicMediaUrl, storage) with no config property — and if runtime init fails, locals.emdash is never set at all. Previously getPublicOrigin(url, locals.emdash?.config) therefore saw undefined for config and fell through getEnvSiteUrl() → url.origin, which is http:// behind Cloudflare's proxy. The fix falls back to the build-time virtual:emdash/config, whose default export the integration serializes with the origin-normalized siteUrl (confirmed in index.ts → serializableConfig.siteUrl → generateConfigModule). This is idiomatic — the middleware already imports the same virtual module — and ?? correctly preserves runtime-config precedence (on the full path locals.emdash.config === virtualConfig, so the left operand wins anyway).
What I checked:
- Verified the anonymous fast path omits
configand that runtime-init failure leaveslocals.emdashunset — both cases are now covered by the?? virtualConfigfallback. - Traced every
getPublicOrigincall-site in the tree. All other callers are either under/_emdash/(full middleware path → config present) or inPUBLIC_RUNTIME_ROUTES(sitemap/robots → full path). The two well-known routes are the only root-level public callers, so the fix is precisely scoped with no sibling inconsistency left behind. - Confirmed the env-var fallback (
EMDASH_SITE_URL) still works viagetPublicOrigin's internal chain, so the change is additive, not a regression for env-var-only deployments (with the documentednodejs_compat_populate_process_envcaveat on Workers). - Conventions:
// @ts-ignore - virtual modulematches AGENTS.md;.jsinternal imports preserved; thesiteUrl?: stringaddition toVirtualConfiglines up with the already-serialized value; a changeset is present with proper release-notes tone; reproducing tests cover the three relevant scenarios (fast path, init-failed, precedence). - Docs/comment edits (
public-url.ts,configuration.mdx) accurately describe the Workers env-var mechanism and thesiteUrl≠Astro.urldistinction.
Conclusion: Clean, well-targeted bug fix with reproducing tests, a changeset, and accurate documentation. No issues found.
There was a problem hiding this comment.
Approach judgment
This is the right fix for the right problem, and it fits EmDash's architecture cleanly. The root cause is real and well-scoped: /.well-known/oauth-protected-resource and /.well-known/oauth-authorization-server/_emdash are root-level, unauthenticated routes, so unauthenticated MCP clients land on the middleware's anonymous fast path (the !isEmDashRoute && !isPublicRuntimeRoute && !hasEditCookie branch in packages/core/src/astro/middleware.ts). On that path locals.emdash is populated with handlers but no config — so locals.emdash?.config is undefined and getPublicOrigin() fell through getEnvSiteUrl() (empty on Workers) to url.origin, which is http:// behind Cloudflare's TLS-terminating proxy. MCP clients then refuse the discovery document.
The fix — getPublicOrigin(url, locals.emdash?.config ?? virtualConfig) — is idiomatic: the middleware itself already uses the build-time virtual:emdash/config as the canonical config source (getConfig() returns virtualConfig directly), and the full-init path sets locals.emdash.config to that same virtualConfig object. So the fallback restores the intended resolution order (config.siteUrl → env var → url.origin) on the one path that was accidentally dropping config.siteUrl. The decision to deliberately exclude the issue's protocol-into-allowedDomains suggestion is defensible and is reasoned on the linked issue.
What I verified
siteUrlactually reaches the virtual module.packages/core/src/astro/integration/index.tsserializessiteUrl: resolvedConfig.siteUrlintoserializableConfig(pre-existing), andresolvedConfig.siteUrlis origin-normalized at startup (parsed.origin), so the build-time fallback value is the correct public origin — no path contamination into${origin}/_emdash/....- No precedence regression. When
siteUrlis unset at build time,virtualConfig.siteUrlisundefinedandgetPublicOriginfalls through togetEnvSiteUrl()→url.origin, identical to prior behavior. When set,config.siteUrlcorrectly takes precedence over the env var, matching the documented order used everywhere else (passkeys, etc.) — the well-known routes were the only place the env var could silently override build-timesiteUrl; this fix makes them consistent. virtualConfigis a sound fallback in all branches. On the full-init pathlocals.emdash.config === virtualConfig, so?? virtualConfigis a no-op there; it only matters on the fast path (and thelocals.emdashentirely-absent / runtime-init-failed cases), which is exactly the target.- Scope is complete for these routes.
robots.txt/sitemap*.xmlusegetPublicOrigin(url, emdash?.config)too but are inPUBLIC_RUNTIME_ROUTES, so they take the full-init path whereconfigis always set (and additionally fall back to a DB-storedsettings.url). The/_emdash/.well-known/authroute is under/_emdash(full path). The two OAuth routes are the only affected ones. - Type accuracy. Adding
siteUrl?: stringtoVirtualConfiginvirtual-modules.d.tscorrectly types the new read; the@ts-ignoreimport pattern matches the rest of the codebase. - Tests are meaningful, not false confidence.
well-known-public-origin.test.tsmocksvirtual:emdash/configwith a realsiteUrland covers the fast path (locals.emdashpresent withoutconfig), the absent-locals.emdashcase, and runtime-config precedence. They exercise the actual route handlers and assert the advertisedresource/issuer/authorization_serversURLs, not a mocked return value. Existingdiscovery-endpoints.test.ts(nositeUrlin the vitest stub{}→ falls tourl.origin) remains compatible since the stub leavesvirtualConfig.siteUrlundefined. - Changeset present (
"emdash": patch), satisfying the changeset convention for a published-package change.
Conclusion
No bugs, regressions, security issues, or convention violations found. The comment fix in public-url.ts (Workers process.env is empty by default but readable with nodejs_compat_populate_process_env) is accurate, and the docs additions are correct. Clean merge.
github-actions
Bot
added
review/approved
khoinguyenpham04
left a comment
khoinguyenpham04
left a comment
There was a problem hiding this comment.
Everything looks good to me! I only left a couple of non-blocking suggestions.
Thanks for tackling this issue :)
| // Same fallback as oauth-protected-resource.ts: this route is public and | ||
| // root-level, so it lands on the anonymous fast path where locals.emdash | ||
| // carries no config (#2016). |
There was a problem hiding this comment.
Could we shorten this while keeping the reason for the fallback?
| // Same fallback as oauth-protected-resource.ts: this route is public and | |
| // root-level, so it lands on the anonymous fast path where locals.emdash | |
| // carries no config (#2016). | |
| // Anonymous discovery requests omit runtime config; use build-time config | |
| // so `siteUrl` can override the proxy's internal origin. |
| // Anonymous requests outside /_emdash take the middleware fast path, | ||
| // which attaches locals.emdash WITHOUT config — and MCP clients hit this | ||
| // route unauthenticated by design. Fall back to the build-time config so | ||
| // `siteUrl` still reaches discovery (#2016); behind Cloudflare's proxy | ||
| // url.origin is http:// and clients refuse to attach. |
There was a problem hiding this comment.
Could we make this comment more concise? Maybe:
| // Anonymous requests outside /_emdash take the middleware fast path, | |
| // which attaches locals.emdash WITHOUT config — and MCP clients hit this | |
| // route unauthenticated by design. Fall back to the build-time config so | |
| // `siteUrl` still reaches discovery (#2016); behind Cloudflare's proxy | |
| // url.origin is http:// and clients refuse to attach. | |
| // The anonymous fast path omits runtime config, so fall back to build-time config. |
2 participants
What does this PR do?
siteUrlnever reached/.well-known/oauth-protected-resource(and/.well-known/oauth-authorization-server): these routes are root-level and unauthenticated — exactly how MCP clients fetch them — so they land on the middleware's anonymous fast path, wherelocals.emdashcarries handlers but noconfig.getPublicOrigin()then fell through tourl.origin, which ishttp://behind Cloudflare's proxy, and MCP clients refused to attach because the discovery document advertised non-https URLs.The fix: both routes now fall back to the build-time
virtual:emdash/config(which carries the origin-normalizedsiteUrl) whenlocals.emdash?.configis absent. This also covers the case where anonymous-path runtime init fails andlocals.emdashis never set at all. Verified the symptom on a real Workers deployment before the fix (curl .../.well-known/oauth-protected-resource→"resource":"http://…").Also included, from the same report (#2016):
public-url.tscomment:process.envis readable on Workers whennodejs_compat_populate_process_envis enabled — the old comment pointed people away from the one mechanism that worked.nodejs_compat_populate_process_env+vars) in thesiteUrlreference, and clarifies thatsiteUrldoesn't changeAstro.urlin user templates (use Astro'ssiteoption for self-built canonical/OG URLs).Deliberately not included: the issue's suggestion to pass
protocolintosecurity.allowedDomains. The Cloudflare adapter passesrequest.urlstraight through and never runs Astro's forwarded-header validation, so that change wouldn't affectAstro.urlon Workers — and on the Node path it could regress proxy setups that send onlyHost(nox-forwarded-host), since a protocol pattern makes host validation fail for the internal http hop. Analysis posted on the issue.Closes #2016
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
New regression tests in
packages/core/tests/unit/astro/well-known-public-origin.test.ts(namedreproduces #2016): siteUrl honored with config-lesslocals.emdash, withlocals.emdashentirely absent, and locals-config precedence preserved. Fullemdashpackage suite: