fix(plugin-forms): defer the submission webhook and log a failed response - #3181
eisenbruch wants to merge 2 commits into
Conversation
…onse The webhook was a floating promise. Nothing awaited it and nothing handed it to the host's lifetime extender, so on Cloudflare Workers the isolate could be torn down once the visitor had their confirmation and the call never left. It now runs inside after(), which registers it with waitUntil where the host provides one. Only a rejected promise was logged, and fetch resolves on a 4xx or a 5xx. It also resolves on the sign-in page an authenticated endpoint redirects to, because http access follows redirects. Each of those looked like a delivered webhook and left nothing in the log, which is how a webhook can fail on every submission unnoticed. The response is now checked, and a 2xx that arrived by redirect is logged too, since the body came from somewhere other than the configured URL. Closes emdash-cms#3136
🦋 Changeset detectedLatest commit: e69a5df The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
eisenbruch
changed the title
@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-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-loader
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
@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 judgment: this is the right fix for the right problem. Moving the form submission webhook into after() matches EmDash’s deferred-task pattern and solves the Cloudflare Workers lifetime issue; inspecting the response for non-2xx statuses is also the right idea. I checked packages/core/src/after.ts, the virtual:emdash/wait-until wiring, and packages/core/src/plugins/context.ts (createHttpAccess / createUnrestrictedHttpAccess).
The implementation is mostly sound, but the redirect detection relies on a Response.redirected signal that the actual plugin HTTP wrapper never emits, because the wrapper follows redirects manually with redirect: "manual". As a result, the auth-wall redirect case described in the PR remains silently successful in production. The unit test passes only because it manually injects redirected: true on a constructed Response. Three related issues need fixing before this should land.
| }); | ||
| if (!response.ok) { | ||
| log.error("Webhook failed", { url, status: response.status }); | ||
| } else if (response.redirected && response.url !== url) { |
There was a problem hiding this comment.
[needs fixing] ctx.http.fetch is produced by createHttpAccess (or createUnrestrictedHttpAccess), which manually follows redirects using globalThis.fetch(..., { redirect: "manual" }) so it can strip credentials on cross-origin hops. Because each internal fetch uses manual redirect mode, the final Response it returns has redirected: false even when redirects were followed. The response.redirected guard therefore means this branch never fires in production: a webhook that redirects to a sign-in page and returns 200 is treated as success, exactly the silent failure the PR says it is fixing. Drop the response.redirected requirement (and normalize URL comparison if you want to avoid trailing-slash false positives) or surface redirect metadata from the HTTP wrapper itself.
| } else if (response.redirected && response.url !== url) { | |
| } else if (response.url !== url) { |
| function response(init: { status?: number; redirected?: boolean; url?: string }): Response { | ||
| const res = new Response("", { status: init.status ?? 200 }); | ||
| // `redirected` and `url` are read-only on a constructed Response; a real redirected fetch sets both. | ||
| Object.defineProperty(res, "redirected", { value: init.redirected ?? false }); | ||
| Object.defineProperty(res, "url", { value: init.url ?? WEBHOOK }); | ||
| return res; | ||
| } | ||
|
|
||
| describe("submission webhook", () => { | ||
| beforeEach(() => vi.clearAllMocks()); | ||
|
|
||
| it("logs a 5xx, which fetch resolves rather than rejects", async () => { | ||
| const { ctx, log } = context(async () => response({ status: 500 })); | ||
| // eslint-disable-next-line typescript/no-unsafe-argument -- minimal RouteContext for this step | ||
| const result = await submitHandler(ctx as never); | ||
| await settle(); | ||
|
|
||
| expect(result.success).toBe(true); // the visitor is never shown the webhook's problem | ||
| expect(log.error).toHaveBeenCalledWith("Webhook failed", { url: WEBHOOK, status: 500 }); | ||
| }); | ||
|
|
||
| it("logs a redirect to somewhere else, which an auth wall answers with a 200", async () => { | ||
| const { ctx, log } = context(async () => | ||
| response({ status: 200, redirected: true, url: "https://example.test/login" }), | ||
| ); | ||
| await submitHandler(ctx as never); | ||
| await settle(); | ||
|
|
||
| expect(log.warn).toHaveBeenCalledWith("Webhook was redirected", { | ||
| url: WEBHOOK, |
There was a problem hiding this comment.
[needs fixing] The response() helper fakes redirected: true and a different url, with a comment that "a real redirected fetch sets both." EmDash’s plugin HTTP fetch does not set redirected: true because it follows redirects manually. This test therefore exercises a code path that only exists in the fixture. A regression that broke real-world redirect detection would not fail this test, and a change that removed the response.redirected guard would still pass because the fixture sets it. Rewrite the redirect case against the actual createHttpAccess wrapper (or a mock that mirrors its manual-redirect behavior) so it reflects production semantics.
| "@emdash-cms/plugin-forms": patch | ||
| --- | ||
|
|
||
| Fixes a form's webhook silently doing nothing. The call was never awaited or handed to the runtime, so on Cloudflare Workers it could be dropped once the visitor's confirmation had been sent; it now runs through `after()`, which registers it with the host so it is guaranteed to finish. A response that is not a success is also logged now: `fetch` only rejects on a transport error, so a 4xx, a 5xx, and the sign-in page an authenticated endpoint redirects to were all treated as if the webhook had worked, leaving no trace anywhere. |
There was a problem hiding this comment.
[needs fixing] The entry tells readers that sign-in-page redirects are now logged, but the implementation in submit.ts cannot detect those redirects under the real ctx.http.fetch because the HTTP wrapper follows redirects manually and never sets response.redirected. Either fix the redirect detection (and the test), or revise the entry so it does not claim the redirect case is handled.
| Fixes a form's webhook silently doing nothing. The call was never awaited or handed to the runtime, so on Cloudflare Workers it could be dropped once the visitor's confirmation had been sent; it now runs through `after()`, which registers it with the host so it is guaranteed to finish. A response that is not a success is also logged now: `fetch` only rejects on a transport error, so a 4xx, a 5xx, and the sign-in page an authenticated endpoint redirects to were all treated as if the webhook had worked, leaving no trace anywhere. | |
| Fixes a form's webhook silently doing nothing. The call was never awaited or handed to the runtime, so on Cloudflare Workers it could be dropped once the visitor's confirmation had been sent; it now runs through `after()`, which registers it with the host so it is guaranteed to finish. A non-success response is also logged now: `fetch` only rejects on a transport error, so 4xx and 5xx responses were previously treated as delivered webhooks with no trace anywhere. |
emdashbot
Bot
added
review/awaiting-author
…s it The review is right: plugin HTTP access follows redirects itself with redirect: "manual" so it can strip credentials on a cross-origin hop, so the response it returns always reports redirected: false however many hops it took. Guarding on Response.redirected meant the auth-wall case never fired in production, which is the failure the change exists to surface. It now compares the final URL, which the wrapper does carry, ignoring a trailing-slash difference so a webhook answered at its own address with a slash is not reported as a redirect. The test deserved the criticism more than the code did. It built a Response and set redirected: true on it, so it passed against a fixture that does not exist in production and would have passed with the guard deleted. It now drives the wrapper's own manual-redirect loop over a stubbed global fetch, so the handler sees the response shape production gives it. Confirmed by putting the old guard back: the auth-wall case fails. The real createHttpAccess is not used directly because it resolves the hostname over DoH before every request, which a unit test cannot do offline; the loop is reproduced instead, which is the alternative the review offered. The changeset no longer claims more than the code does.
github-actions
Bot
added
review/needs-rereview
|
You are right on all three, and the test deserved the criticism more than the code did. Fixed in e69a5df. The redirect guard. I confirmed it in The test. This is the part I got wrong rather than merely incomplete. Setting I did try driving the real Verified the way it should have been the first time: I put the old The changeset no longer claims more than the code does; it names the URL comparison and why |
1 participant
What does this PR do?
Closes #3136.
The submission webhook was a floating promise, and only a rejected one was logged. Two consequences, both silent:
submitHandlerreturned immediately after firing it, so on Cloudflare Workers the isolate could be torn down before the request went out. It now runs insideafter(), which registers the promise withwaitUntilwhere the host provides one.fetchonly rejects on a transport error. A 4xx and a 5xx both resolve, and so does the sign-in page an authenticated endpoint redirects to, becausecreateHttpAccessfollows redirects and the plugin declaresallowedHosts: ["*"]. Every one of those looked like a delivered webhook and wrote nothing to the log. The response is now inspected, and a 2xx that arrived by redirect is logged as well, since the body came from somewhere other than the configured URL.We hit this on a live site with a webhook pointed at a route on the same host: submissions stored, notification emails sent, no webhook effect, nothing in the logs to say so.
The emails in steps 7 and 8 are already awaited, so they are left alone.
Type of change
Checklist
pnpm typecheckpasses (@emdash-cms/plugin-forms)pnpm lintpasses (oxlint --type-aware --deny-warnings, 0 diagnostics)pnpm testpasses (or targeted tests for my change):packages/plugins/forms, 5 files, 26 testspnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.@emdash-cms/plugin-forms: patch)AI-generated code disclosure
Screenshots / test output
Not applicable, no UI change.
packages/plugins/forms/tests/webhook.test.tscovers four cases: a 5xx, a redirect to a different URL, a transport error, and a clean success that logs nothing. The first two fail againstmainand pass with this change; the other two pass on both.There is one thing I did not do, and it is worth a maintainer's opinion. A same-origin webhook on Workers still goes out over the public hostname, so it passes through whatever sits in front of the site — Cloudflare Access, in our case — and gets the sign-in page. A service binding back to the Worker avoids that, but it looks like a docs note rather than something the plugin should reach for, so this PR only makes the failure visible rather than trying to solve it.