Stop public form submissions reaching Sentry as unhandled errors

[arpitdalal]

The issues

Four ways a POST to /contact or /resources/newsletter could reach Sentry as an unhandled error. All four are reachable by anyone who can send a request to a public form URL, and all four were found in production logs or by review of the code that caused them.

1. An unreadable body threw before the action's own code ran

request.formData() is not a parser that can fail softly. undici throws a TypeError the moment the Content-Type is not multipart/form-data or application/x-www-form-urlencoded — and it throws it while reading the body, before line 33 of the action executes. This is the error in the report:

TypeError: Content-Type was not one of "multipart/form-data" or "application/x-www-form-urlencoded".
    at action (../app/routes/_marketing+/contact.tsx:32:19)

Any POST carrying a JSON body, no body at all, or an invented Content-Type became an unhandled 500. A body is not required to carry a Content-Type — a bare curl -X POST /contact sends neither — so the simplest scanner request was enough. The timestamp in the report (60s after boot) is a scanner, not a user.

Two things made this worse than a 500 should be. It answered 500, which claims the server broke when the request was merely not understood. And it threw ahead of checkHoneypot, so the 400 path built for exactly this traffic (issue #14) never saw these requests.

2. A media-type allowlist alone was not enough

The obvious fix — check Content-Type, reject otherwise — is necessary but not sufficient. multipart/form-data carries its boundary as a header parameter, and formData() rejects when it is missing. Verified against the runtime rather than assumed:

multipart/form-data, no boundary  -> THREW TypeError | Failed to parse body as FormData.

So a request declaring a media type we accept can still be unreadable. Enumerating the headers that produce a parse failure is not a fix; it is a list of today's, and the Fetch spec only promises a TypeError for "fails for some reason".

3. The honeypot could be skipped entirely

remix-utils' shouldCheckHoneypot returns false for a form carrying neither honeypot field, and check then returns without throwing. That is the right default for a library — a route with no honeypot should not be forced to carry one. But every form on these two routes renders HoneypotInputs unconditionally, so a form without the fields cannot have come from our page, and silently accepting it meant a bot skipped the check entirely by leaving two fields out. No crypto, no forged token, no knowledge of what the fields are called.

4. The fix for all of the above was itself broken

Worth calling out separately, because it is the reason this PR is more than two small guards. A Response thrown from an action becomes an ErrorResponse to React Router exactly as a crash does, and GeneralErrorBoundary called captureException unconditionally. So every rejection added by issues 1–3 was reported from the browser — with replaysOnErrorSampleRate: 1.0, attaching a session replay to whoever submitted. The newsletter form is in the footer of every page, so one stray POST against it filed an issue and a replay for an innocent visitor. The rejections also replaced the whole page with a bare error, taking the user's typed message with it.

The claim "junk traffic is cheap and silent" was false in both halves. The first two commits in this branch asserted it; review disproved it.

The solutions

app/utils/form-data.server.ts (new) — read the body, or answer 400

Two rejections, because there are two ways a body can be unreadable:

• Media type checked first, so the common case — a request that never claimed to be a form — is refused without touching the body.
• The parse itself is wrapped, so a TypeError from formData() is answered 400 whatever the reason. This closes issue 2 without enumerating headers, and it holds for reasons nobody has thought of yet.

Narrowed to TypeError on purpose. A body already read or locked rejects with a TypeError too — the spec's consume body step 1 says so, and it is what a double formData() produces — but that is our bug, not the client's, and converting it would file a server fault as a rejected submission and hide it permanently. request.bodyUsed is the spec'd way to tell them apart. The test reproduces the double read for real rather than mocking it.

The cross-realm Symbol.toStringTag read matches isMalformedHoneypotFieldError in the same change — two ways of asking the same question in one branch is how the wrong one gets copied.

app/utils/honeypot.server.ts — stop letting a form be exempt

shouldCheckHoneypot is protected, which is the library marking it an extension point, so overriding it is the supported way to be strict. Verified that this gets the same SpamError through the library's own "Missing honeypot input" branch:

FORCE override -> SpamError | Missing honeypot input

Field names are read off the base class rather than hardcoded. getInputProps() is public and returns both, and the getters are reachable from a subclass. This also keeps the requirement true under randomizeNameFieldName, which a hardcoded name__confirm would have broken by rejecting every genuine submission.

app/utils/rejected-submission.ts (new) — one answer, in one place

Four things can refuse a submission: an unreadable body, a filled honeypot, an undecryptable token, and omitted fields. All answer with the same 400, deliberately — reporting which check failed hands a scanner a map of what the endpoint validates. It is a separate module with no server-only imports because the client boundary has to recognise it, and a .server import there would pull node:crypto into the browser bundle.

app/components/error-boundary.tsx — stop reporting an answer as a fault

isRejectedSubmission recognises the guards' own response so captureException is not called for it. Narrowed on the status and the shared body, because either alone is wrong:

• body alone would silence any future 500 carrying the same text — a real fault is exactly what this must not swallow
• status alone would re-report the 400 React Router raises for a scanner probing a path, which sentry-event-filters.ts already drops server-side

The test that pins the 500 case caught this: my first version matched on body only and failed it.

The honeypot is a speed bump, not authentication

Stating this plainly so nobody inherits the false impression that "the fields are now required" closes the spam story. A bot that GETs /contact, copies both fields, and posts them back passes everything here — the token is in the HTML of every page, and it has no TTL. The compensating control is the 10/min/IP rate limit, which is real but per-IP and trivially distributed. The highest-value follow-up is a max-age on from__confirm; noted, not done here.

Verification

npm run validate (lint, typecheck, format) and 232 tests pass. Beyond the suites, six behaviours checked against a running production build:

request	result
no body / no Content-Type	400
multipart/form-data without boundary	400
omits the honeypot fields	400
filled honeypot	400
real scraped submission	200
same, with ; charset=UTF-8	200

Every rejected-submission path is silent server-side (no unhandled error reports), and isRejectedSubmission is unit-tested from both directions so a real 500 cannot be silenced.

Notes for review

• The rate-limit budget is a real coupling in the smoke test. /contact and /resources/newsletter share one express-rate-limit instance keyed by IP, so the budget is 10/min total, not 10 each. Adding an end-to-end case there will 429 an unrelated test. That is why the boundary matrix lives in tests/form-data.test.ts (no budget cost), and why the suite now counts its own POSTs and fails in afterAll with a message about the budget — verified to actually fire.
• One existing test changed meaning. rethrows a non-DOMException unchanged passed new FormData() and reached its mock only because of the bypass. It now posts a well-formed form, which is what it was always meant to test.
• Review found two of my own comments confidently wrong and both are fixed: that TESTING disables the timestamp half the suite depends on (it sets the empty string, falsy, so the crypto path is live), and that the two paths have 10/min each.
• No issue is linked — this was raised as a Sentry error rather than a filed report, and the two problems are independent (one is about body parsing, one about spam), so a single issue would have covered neither well.

Summary by CodeRabbit

• Bug Fixes
  • Contact and newsletter forms now handle supported form submissions more consistently, including URL-encoded and multipart forms.
  • Submissions with missing or invalid form data are rejected with a consistent error response.
  • Contact form submissions missing required verification fields are rejected, while valid submissions continue to work.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: cbc48b7a-a365-4199-9d48-ba93e408f47a

📥 Commits

Reviewing files that changed from the base of the PR and between bcd65e4 and d0dd256.

📒 Files selected for processing (10)

• app/components/error-boundary.tsx
• app/routes/_marketing+/contact.tsx
• app/routes/resources+/newsletter.tsx
• app/utils/form-data.server.ts
• app/utils/honeypot.server.ts
• app/utils/rejected-submission.ts
• tests/error-boundary.test.ts
• tests/form-data.test.ts
• tests/honeypot.test.ts
• tests/server.smoke.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

📝 Walkthrough

Walkthrough

Contact and newsletter actions now use shared form-data parsing. Contact honeypot validation rejects submissions that omit required fields. Rejected submissions use a shared response value, which the error boundary excludes from Sentry capture and console logging.

Changes

Form submission validation

Layer / File(s)	Summary
Shared form parsing and route integration app/utils/rejected-submission.ts, app/utils/form-data.server.ts, app/routes/_marketing+/contact.tsx, app/routes/resources+/newsletter.tsx, tests/form-data.test.ts, tests/server.smoke.test.ts	The form-data utility accepts URL-encoded and multipart content types and returns a 400 response for unsupported or unparseable submissions. Both actions use the utility. Tests cover supported formats, parsing errors, and body-less POST requests.
Required honeypot fields app/utils/honeypot.server.ts, tests/honeypot.test.ts, tests/form-data.test.ts, tests/server.smoke.test.ts	The exported honeypot exposes configured required field names. Honeypot checks reject submissions that omit required fields. Tests cover missing fields and valid readable submissions.
Rejected-submission error handling app/components/error-boundary.tsx, tests/error-boundary.test.ts	The error boundary recognizes 400 route error responses with the shared rejection message and skips Sentry capture and console logging for those responses.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Browser
  participant ContactAction
  participant readFormData
  participant checkHoneypot
  participant GeneralErrorBoundary
  Browser->>ContactAction: POST contact form
  ContactAction->>readFormData: readFormData(request)
  readFormData-->>ContactAction: FormData
  ContactAction->>checkHoneypot: checkHoneypot(formData)
  checkHoneypot-->>ContactAction: 400 REJECTED_SUBMISSION if fields are missing
  ContactAction-->>GeneralErrorBoundary: route error response

Loading

Merge Risk: ⚪ Minimal · up to d0dd2

The change makes unreadable or incomplete public form submissions return a consistent 400 response and keeps them out of error reporting. No concrete merge-blocking risk was identified.

Security Architecture Review

Security architecture risk: 🔵 Low · up to d0dd2

The change tightens public form validation without expanding access to email or newsletter operations. Error-report suppression is narrowly matched to expected rejections. Remaining uncertainty concerns runtime error classification and existing external-service failure behavior, rather than a demonstrated new attack path.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• observed — The exposed paths are unauthenticated contact and newsletter submissions. Accepted contact data reaches SMTP delivery with a configured sender and recipient; accepted newsletter data reaches subscription for the configured publication. The reviewed action changes do not add callers or broaden those destination selections.

Security Findings and Attack Paths

• observed — Requests with missing or unsupported media types terminate before body parsing. Forms omitting required honeypot fields terminate before honeypot decryption and downstream side effects. These rejection paths reduce attacker-triggered processing rather than expanding access to the sensitive operations.

Trust Boundaries and Controls

• observed — Client-side reporting suppression requires a router error response, status 400, and the exact application-defined rejection body. It does not suppress arbitrary 400 responses, ordinary Errors, or 500 responses carrying the same text; unmatched errors retain capture and console-reporting paths.

Resilience and Maintainability Implications

• inferred — Newsletter subscription remains a single external mutation with success or form-error mapping, without an application idempotency key, concurrency lock, or durable recovery marker. Response loss can leave an ambiguous provider outcome followed by a retry. These limitations predate the parsing change; provider-side duplicate suppression remains unestablished.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 56.25% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 10 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: preventing rejected public form submissions from reaching Sentry as unhandled errors.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}