fix(forms): render public form embeds via SSR plugin routes#985
fix(forms): render public form embeds via SSR plugin routes#985ppppangu wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: e02e87d The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@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-forms
@emdash-cms/plugin-webhook-notifier
commit: |
|
/review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Targeted fix for the Workers SSR self-fetch issue, with the wrapped helper correctly gated on getPluginRouteMeta(...).public === true and decent unit coverage on both sides. Two things worth addressing before merge — the silent fallback when the dispatcher throws hides exactly the failures this PR is meant to solve, and Astro.locals.emdash.handlePluginApiRoute now has materially different semantics on the anonymous vs. full middleware paths (public-only vs. raw runtime). A couple of smaller robustness notes inline as well. Nothing here is a blocker.
| ); | ||
| } catch { | ||
| // Fall back to HTTP fetch for older runtimes or unexpected dispatcher failures. | ||
| } |
There was a problem hiding this comment.
The silent catch {} here swallows exactly the failures this PR is meant to fix. If the internal dispatcher throws on Workers SSR, we then fall through to fetchImpl(...) — which is a same-origin self-fetch, the very thing that's broken on Workers and what motivated the PR. The user sees a form that renders nothing with no log to grep for.
At minimum, log the error so it's grep-able in production. Consider also distinguishing "genuine fallback for older runtimes" (no dispatcher present at all, handled by the outer if (handlePluginApiRoute)) from "dispatcher present but threw" — the latter is a bug and probably shouldn't silently fall back to a path we know fails on Workers.
| } | |
| } catch (error) { | |
| // Fall back to HTTP fetch for older runtimes or unexpected dispatcher | |
| // failures. On Workers SSR the fetch path is the broken one this helper | |
| // exists to avoid, so make the failure visible. | |
| console.warn("[emdash-forms] public definition dispatcher failed, falling back to fetch:", error); | |
| } |
| setupVerified = true; | ||
| // eslint-disable-next-line typescript-eslint(no-unsafe-type-assertion) -- partial object; getPageRuntime() only checks for the page-contribution methods | ||
| locals.emdash = { | ||
| handlePluginApiRoute: createPublicPluginApiRouteHandler(runtime), |
There was a problem hiding this comment.
After this PR, Astro.locals.emdash.handlePluginApiRoute means two different things depending on which branch of the middleware runs:
- Anonymous fast path (this line): the wrapped helper that only dispatches when
getPluginRouteMeta(...).public === true. - Full path (line 490):
runtime.handlePluginApiRoute.bind(runtime)— the raw method, which will happily invoke private plugin routes with no auth / CSRF / scope check.
That's a meaningful difference. A plugin author calling this from a public Astro page is going to discover the difference the hard way when an editor visits the same page and suddenly more routes are reachable. The PR description claims "the core helper gates every internal dispatch on getPluginRouteMeta(...).public === true" — that's only true on the anonymous path.
A couple of options, roughly in order of cost:
- Use the wrapped helper on both paths (since the only documented caller is
FormEmbed, which only needs public routes anyway). That makes the property uniformly safe. - Rename the anonymous-path field to something like
dispatchPublicPluginRouteso the asymmetry is explicit in the type. - Leave as-is and add a comment here + on line 490 documenting the asymmetry.
Option 1 seems cleanest unless there's a known consumer of the raw method on the full path.
| } | ||
|
|
||
| return payload as unknown as PublicFormDefinition; | ||
| } |
There was a problem hiding this comment.
parsePublicFormDefinitionPayload accepts any object with status === "active" as a valid PublicFormDefinition and returns it cast straight through. If the dispatcher (or a future API shape change) ever returns something like { status: "active" } with missing pages/settings, FormEmbed.astro will crash at render time on form.pages.map(...) rather than returning null and rendering nothing.
Not introduced by this PR (the old code did the same), but you're now reaching this path from a new entry point (loadPublicFormDefinition via the internal dispatcher) where the shape is less constrained than a typed HTTP response. Worth at least an Array.isArray(payload.pages) && isObject(payload.settings) guard before the final cast.
|
|
||
| if ("data" in payload) { | ||
| return parsePublicFormDefinitionPayload(payload.data); | ||
| } |
There was a problem hiding this comment.
Minor: the "data" in payload unwrap is structurally ambiguous — if a future PublicFormDefinition ever grows a top-level data field (e.g. arbitrary form metadata), it'll be silently over-unwrapped and the function will return null. Given the form definition currently has name/slug/pages/settings/status/_turnstileSiteKey this is theoretical, but since you're now sharing this parser between two different call shapes (HTTP envelope vs. HandlerResponse), it'd be more robust to dispatch on shape explicitly — e.g. require "success" in payload for the dispatcher case and only strip a single data layer for the response case, instead of recursing on any data field.
| ); | ||
| const handlePluginApiRoute = (Astro.locals.emdash as | ||
| | { handlePluginApiRoute?: PublicPluginApiRouteHandler } | ||
| | undefined)?.handlePluginApiRoute; |
There was a problem hiding this comment.
Minor type-safety note: this cast (Astro.locals.emdash as { handlePluginApiRoute?: PublicPluginApiRouteHandler } | undefined) silently accepts any shape and degrades to the fetch fallback if handlePluginApiRoute is missing or has the wrong signature. If the core ever renames the field or changes its signature, this component won't fail to compile — it'll just quietly use the broken fetch path on Workers (the bug this PR is fixing).
Consider exporting a typed accessor from emdash/plugin-utils (or wherever the plugin-facing API lives) that returns PublicPluginApiRouteHandler | undefined and is checked at the core boundary, so a future rename produces a build failure instead of a silent regression.
| expect(typeof emdash.collectPageFragments).toBe("function"); | ||
| expect("getPluginRouteMeta" in emdash).toBe(false); | ||
| expect("handleContentList" in emdash).toBe(false); | ||
| expect("db" in emdash).toBe(false); |
There was a problem hiding this comment.
Two small gaps in the assertions here:
getPublicMediaUrlis assigned on the anonymous path (middleware.ts:367) — worth assertingtypeof emdash.getPublicMediaUrl === "function"so a future refactor that drops it gets caught.- The test doesn't actually exercise the wrapped helper end-to-end — it only checks shape. Given that the wrapping is the core security claim of this PR, a follow-up assertion like
await emdash.handlePluginApiRoute("demo", "POST", "/private", req)returningNOT_FOUND(with the proxy returning{ public: false }for that path) would tighten the contract considerably.
| if (prop === "collectPageMetadata") return async () => []; | ||
| if (prop === "collectPageFragments") return async () => []; | ||
| return async () => ({ success: true }); | ||
| }, |
There was a problem hiding this comment.
Minor: this Proxy returns an async function (async () => ({ success: true })) for any unknown property access, including symbols like Symbol.toPrimitive or Symbol.iterator. That's brittle — if the middleware ever does e.g. String(runtime) or for (const x of runtime) during init, the proxy returns an async function for the well-known symbol and you'll get a confusing error rather than the normal behavior. Returning undefined for any non-string prop (or any prop not in an explicit allowlist) would be safer:
| }, | |
| if (typeof prop !== "string") return undefined; | |
| if (prop === "then") return undefined; | |
| if (prop in target) return target[prop as keyof typeof target]; | |
| if (prop === "getPluginRouteMeta") return () => ({ public: true }); | |
| if (prop === "handlePluginApiRoute") return async () => ({ success: true, data: {} }); | |
| if (prop === "collectPageMetadata") return async () => []; | |
| if (prop === "collectPageFragments") return async () => []; | |
| return async () => ({ success: true }); |
|
Review posted successfully (ID 4263518046, 7 inline comments). The review covers:
|
2 participants
What does this PR do?
Fixes public form embeds during SSR by giving anonymous public pages a restricted way to call public plugin routes. Form embeds now resolve their public definition through the core dispatcher when available, which avoids server-side self-fetching in Worker SSR while preserving the old fetch path for older runtimes or dispatcher failures.
The core helper gates every internal dispatch on
getPluginRouteMeta(...).public === true, so private plugin routes still require the normal API route auth, scope, and CSRF path. The forms plugin only calls the fixedemdash-forms/definitionroute and continues to parse the existing{ data: ... }API envelope.Closes: none
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
No screenshots; this is an SSR/plugin route bug fix.
Verification run:
pnpm --filter emdash test -- public-plugin-api-routes middleware-prerenderpassedpnpm --filter @emdash-cms/plugin-forms test -- public-definitionpassedpnpm --filter @emdash-cms/plugin-forms typecheckpassedpnpm --filter emdash buildpassedpnpm typecheckpassedpnpm lintexited with 0 errors; it reports existing warnings outside this changepnpm exec prettier --checkon the touched files passedNotes:
pnpm formatbecausepnpm format:checkreports broad existing formatting drift in this Windows checkout; I kept formatting validation scoped to touched files to avoid unrelated churn.pnpm --filter emdash testcurrently has unrelated Windows-environment failures intests/unit/astro/vite-config.test.ts(File URL path must be absolute) andtests/unit/cli/bundle-utils.test.ts(systemtarlisting CRLF). The targeted core tests for this change pass.