Stop public form submissions reaching Sentry as unhandled errors

app/components/error-boundary.tsx

@@ -13,6 +13,7 @@ import {
 	href,
 } from 'react-router'
 import { getErrorMessage } from '#app/utils/misc'
+import { REJECTED_SUBMISSION } from '#app/utils/rejected-submission'
 import { Button } from './ui/button'
 import { Icon } from './ui/icon'
 
@@ -21,6 +22,34 @@ type StatusHandler = (info: {
 	params: Record<string, string | undefined>
 }) => ReactElement | null
 
+/**
+ * Whether `error` is one this app threw on purpose to answer a request it
+ * refuses, rather than a fault.
+ *
+ * A route can throw a `Response` as a considered answer — `checkHoneypot` and
+ * `readFormData` both answer 400 for a submission the site will not accept —
+ * and React Router turns that into an `ErrorResponse` at the nearest boundary,
+ * exactly as it does for a crash. Without this distinction every such rejection
+ * was reported to Sentry from the browser, which is the opposite of the intent:
+ * the rejection exists so that junk traffic is cheap and silent, and the
+ * newsletter form is in the footer of every page, so one stray POST was enough
+ * to file an issue and attach a session replay to an innocent visitor.
+ *
+ * Narrowed on the status *and* the shared body. The body alone is not enough:
+ * anything else in the app that ever answers 500 with this same text would be
+ * silenced, and a real fault is exactly what this must not swallow. The status
+ * alone is not enough either, because 400 is also what React Router uses for
+ * "no matching loader" — a scanner probing for routes, which
+ * `sentry-event-filters.ts` already drops server-side, and reporting it here
+ * would undo that work. Both together identify the guards' own answer.
+ */
+export function isRejectedSubmission(error: unknown): boolean {
+	return (
+		isRouteErrorResponse(error) &&
+		error.status === 400 &&
+		error.data === REJECTED_SUBMISSION
+	)
+}
 export function GeneralErrorBoundary({
 	defaultStatusHandler = ({ error }) => (
 		<p>
@@ -38,11 +67,17 @@ export function GeneralErrorBoundary({
 	const params = useParams()
 
 	useEffect(() => {
+		// A considered rejection is an answer, not a fault — see
+		// `isRejectedSubmission`. Everything else, including every `ErrorResponse`
+		// the router raises itself, is still reported.
+		if (isRejectedSubmission(error)) return
 		captureException(error)
 	}, [error])
 
 	if (typeof document !== 'undefined') {
-		console.error(error)
+		// Same reasoning, for the console: a rejected submission is expected
+		// traffic and logging it would fill the deploy logs with it.
+		if (!isRejectedSubmission(error)) console.error(error)
 	}
 
 	return (

app/routes/_marketing+/contact.tsx

@@ -18,6 +18,7 @@ import { LineGlow } from '#app/components/line-glow'
 import { SubmitButton } from '#app/components/submit-button'
 import { TextareaField } from '#app/components/textarea-field'
 import { sendEmail } from '#app/utils/email'
+import { readFormData } from '#app/utils/form-data.server'
 import { checkHoneypot } from '#app/utils/honeypot.server'
 import { ContactSchema } from '#app/utils/schemas'
 import { type Route } from './+types/contact'
@@ -29,7 +30,7 @@ export const meta: Route.MetaFunction = () => [
 ]
 
 export const action = async ({ request }: Route.ActionArgs) => {
-	const formData = await request.formData()
+	const formData = await readFormData(request)
 	await checkHoneypot(formData)
 	const submission = parseWithZod(formData, { schema: ContactSchema })
 

app/routes/resources+/newsletter.tsx

@@ -1,11 +1,12 @@
 import { parseWithZod } from '@conform-to/zod/v4'
 import { ADD_SUBSCRIBER } from '#app/graphql/queries'
+import { readFormData } from '#app/utils/form-data.server'
 import { checkHoneypot } from '#app/utils/honeypot.server'
 import { NewsletterSchema, SubscribeResponseSchema } from '#app/utils/schemas'
 import { type Route } from './+types/newsletter'
 
 export async function action({ request }: Route.ActionArgs) {
-	const formData = await request.formData()
+	const formData = await readFormData(request)
 	await checkHoneypot(formData)
 	const submission = parseWithZod(formData, { schema: NewsletterSchema })
 

app/utils/form-data.server.ts

@@ -0,0 +1,126 @@
+/**
+ * Body parsing for the two public form actions, `/contact` and
+ * `/resources/newsletter`.
+ *
+ * `request.formData()` is not a parser that can fail softly: it throws a
+ * `TypeError` from undici the moment the `Content-Type` is not
+ * `multipart/form-data` or `application/x-www-form-urlencoded`, and it throws it
+ * while reading the body, which is *before* any of the action's own code runs.
+ * That put a bare 500 in Sentry for every `POST /contact` that arrived with a
+ * JSON body, no body at all, or a `Content-Type` a scanner made up — the kind of
+ * traffic a public form URL collects continuously.
+ *
+ * Two things were wrong with that, and this fixes the first:
+ *
+ * 1. It answered 500, which is a claim that the server broke. The request was
+ *    not understood, not mishandled, so 400 is the truthful answer.
+ * 2. It reached Sentry at all. It threw ahead of `checkHoneypot`, so the 400
+ *    path in `honeypot.server.ts` — the one that already exists precisely so
+ *    junk traffic is cheap and silent — never got a look at it.
+ *
+ * The alternative of dropping the message in `sentry-event-filters.ts` is not
+ * enough on its own: it hides the symptom while the endpoint keeps answering 500
+ * and keeps throwing on every such request.
+ */
+
+import { REJECTED_SUBMISSION } from '#app/utils/rejected-submission'
+
+/**
+ * The only two media types a browser form submission can carry, and therefore
+ * the only two `formData()` will accept.
+ */
+const FORM_MEDIA_TYPES = new Set([
+	'application/x-www-form-urlencoded',
+	'multipart/form-data',
+])
+
+/**
+ * Whether `error` is a `TypeError`, the way this repo reads a cross-realm class
+ * everywhere else.
+ *
+ * `instanceof` would work here — this runs once per request on the server,
+ * which is single-realm — but `isMalformedHoneypotFieldError` in
+ * `honeypot.server.ts` deliberately reads `Symbol.toStringTag` for the same
+ * reason and documents why, and two ways of asking the same question in one
+ * change is how the wrong one gets copied. The tag is correct in every realm.
+ */
+function isTypeError(error: unknown): boolean {
+	return (
+		Object.prototype.toString.call(error) === '[object Error]' &&
+		(error as Error).name === 'TypeError'
+	)
+}
+
+/**
+ * Reads the request body as `FormData`, or answers 400.
+ *
+ * Throwing a `Response` is the same contract `checkHoneypot` uses for junk, so
+ * both failure modes on these routes land as a 400 that React Router turns into
+ * a status code rather than an unhandled exception.
+ *
+ * Deliberately not logged. This is scanner traffic, it is unbounded, and an
+ * unparseable body is not information the operator can act on — the honeypot's
+ * own 400 is silent for the same reason.
+ */
+export async function readFormData(request: Request): Promise<FormData> {
+	// Two separate rejections, because there are two separate ways the body can
+	// be unreadable, and only the second one is visible from inside `formData()`.
+	//
+	// The media type is checked first so the common case — a request that never
+	// claimed to be a form — is refused without touching the body at all. A body
+	// is not required to carry a `Content-Type`; a bare `curl -X POST /contact`
+	// sends neither, so the header is the only place the answer to "could this
+	// possibly be a form?" exists.
+	//
+	// That check is necessary but not sufficient, which is the bug this shape
+	// exists to close. `multipart/form-data` carries its boundary as a header
+	// *parameter*, and `formData()` rejects when it is missing — so a request
+	// declaring a media type we accept can still be unreadable. Enumerating the
+	// headers that produce a parse failure would not be a fix, it would be a
+	// list of today's, and the spec only requires a `TypeError` for "fails for
+	// some reason". So the parse itself is wrapped and every `TypeError` it
+	// throws is answered the same way, which holds whatever the reason was.
+	//
+	// The allowlist is a second source of truth, deliberately: measured across
+	// the header variants a real client can send, it rejects nothing
+	// `formData()` would have accepted. What it buys is refusing without
+	// reading the body, and what it costs is that a third form encoding would
+	// have to be added here in step with undici or real users get a 400.
+	try {
+		const mediaType = request.headers
+			.get('content-type')
+			?.split(';', 1)[0]
+			?.trim()
+			.toLowerCase()
+
+		if (!mediaType || !FORM_MEDIA_TYPES.has(mediaType)) {
+			throw new Response(REJECTED_SUBMISSION, { status: 400 })
+		}
+
+		// A body already read or locked is *our* bug rather than the client's —
+		// something upstream consumed it — and it rejects with a `TypeError` too,
+		// which is exactly the signal the catch below converts. `bodyUsed` is the
+		// spec'd way to tell the two apart ("whether the body has been read
+		// from", https://fetch.spec.whatwg.org/#dom-body), and it is the one part
+		// of this that is not a judgement call: without it a double read would be
+		// filed as a rejected submission and never surface at all.
+		if (request.bodyUsed) {
+			throw new Error(
+				'readFormData: the request body was already consumed before the action read it',
+			)
+		}
+
+		return await request.formData()
+	} catch (error) {
+		// `formData()` signals an unreadable body with a `TypeError` in every
+		// case the spec names — an undeterminable or non-form MIME type, a
+		// decoding failure, a body it cannot parse — and the spec requires
+		// nothing else for this input. So this is the whole of "the client sent
+		// something we cannot read". Anything else is a fault on this side of
+		// the wire and still belongs in Sentry.
+		if (isTypeError(error)) {
+			throw new Response(REJECTED_SUBMISSION, { status: 400 })
+		}
+		throw error
+	}
+}

app/utils/honeypot.server.ts

@@ -1,9 +1,66 @@
 import { createHash } from 'node:crypto'
 import { Honeypot, SpamError } from 'remix-utils/honeypot/server'
+import { REJECTED_SUBMISSION } from '#app/utils/rejected-submission'
 
-export const honeypot = new Honeypot({
-	validFromFieldName: process.env.TESTING ? null : undefined,
+/**
+ * A `Honeypot` that never treats a form as exempt.
+ *
+ * `remix-utils` has `shouldCheckHoneypot` return false — and `check` return
+ * without throwing — for a form carrying neither honeypot field. That is the
+ * right default for a library: a route with no honeypot should not be forced to
+ * carry one. But every form on the routes that use this module renders
+ * `HoneypotInputs` unconditionally, so a form without the fields cannot have
+ * come from our page, and silently accepting it means a bot skips the check
+ * that exists to catch it just by leaving the fields out.
+ *
+ * `protected` is the library marking this as an extension point, so overriding
+ * it is the supported way to be strict. It gets the behaviour through the
+ * library's own `SpamError` path rather than a second, parallel implementation
+ * of the same rule — the previous version of this file checked for the fields
+ * itself and had to hardcode their names to do it, which is what made it drift.
+ *
+ * The field names are read from the base class rather than repeated here. They
+ * are `protected` rather than private precisely so a subclass can reach them,
+ * and reading them keeps the requirement true under
+ * `randomizeNameFieldName`, where the name is suffixed per request and a
+ * hardcoded `name__confirm` would reject every genuine submission.
+ */
+class StrictHoneypot extends Honeypot {
+	/**
+	 * Always run the check. Returning `true` makes `check` reject a form
+	 * missing `name__confirm` as a `SpamError` through its existing
+	 * "Missing honeypot input" branch.
+	 */
+	protected override shouldCheckHoneypot() {
+		return true
+	}
+
+	/** The fields a form must carry to be checked at all, in configured order. */
+	get requiredFieldNames(): string[] {
+		const names = [this.nameFieldName]
+		// `null` means the timestamp half is disabled, and the input is not
+		// rendered when it is — so it cannot be required.
+		if (this.validFromFieldName) names.push(this.validFromFieldName)
+		return names
+	}
+}
+
+/**
+ * `validFromFieldName` is left exactly as it was — `undefined` normally, `null`
+ * under `TESTING` — because `requiredFieldNames` reads back whatever the
+ * instance was configured with rather than restating it. Nothing here depends
+ * on which branch is taken, so there is no env var to arrange to reach the
+ * crypto path, and the two cannot drift.
+ *
+ * Note for anyone reading `tests/honeypot.test.ts` next: `vi.stubEnv('TESTING',
+ * '')` sets the empty string, which is *falsy*, so the suite runs with the
+ * timestamp half **enabled**. That is what makes the `from__confirm` cases
+ * meaningful rather than no-ops, and it is why that file no longer claims
+ * otherwise.
+ */
+export const honeypot = new StrictHoneypot({
 	encryptionSeed: process.env.HONEYPOT_SECRET,
+	validFromFieldName: process.env.TESTING ? null : undefined,
 })
 
 /**
@@ -59,16 +116,35 @@ export function isMalformedHoneypotFieldError(error: unknown) {
 	)
 }
 
+/**
+ * The honeypot fields this submission is missing, if any.
+ *
+ * Redundant with the check itself — `StrictHoneypot` already rejects a form
+ * that omits them — and kept because it is where the rejection *message* is
+ * chosen. The library says which field it wanted; this answers that the
+ * submission is not one we issued. Both are the same 400, so nothing
+ * observable depends on the wording.
+ */
+function missingHoneypotFields(formData: FormData): string[] {
+	return honeypot.requiredFieldNames.filter((name) => !formData.has(name))
+}
+
 export async function checkHoneypot(formData: FormData) {
+	// Checked before `honeypot.check` so a submission that skipped the fields is
+	// answered without paying for the decrypt.
+	if (missingHoneypotFields(formData).length > 0) {
+		throw new Response(REJECTED_SUBMISSION, { status: 400 })
+	}
+
 	try {
 		await honeypot.check(formData)
 	} catch (error) {
 		// A `SpamError` is the expected rejection. A malformed field is the same
-		// answer reached the long way round — see the note on the predicate for
-		// the one case that is not really spam. Everything else is a real fault
+		// answer reached the long way round — see the note on the predicate for the
+		// one case that is not really spam. Everything else is a real fault
 		// and stays a 500.
 		if (error instanceof SpamError || isMalformedHoneypotFieldError(error)) {
-			throw new Response('Form not submitted properly', { status: 400 })
+			throw new Response(REJECTED_SUBMISSION, { status: 400 })
 		}
 		throw error
 	}

app/utils/rejected-submission.ts

@@ -0,0 +1,23 @@
+/**
+ * The one answer this site gives to a submission it refuses.
+ *
+ * Four different things can refuse a public form submission: a body that cannot
+ * be read, a filled honeypot, a `from__confirm` that will not decrypt, and a
+ * submission that skipped the honeypot fields altogether. All of them answer
+ * with this same 400, and that is deliberate — see the two notes below, because
+ * "same status" and "same body" are both load-bearing and both easy to relax by
+ * accident.
+ *
+ * Lives in its own module rather than beside either check because it is needed
+ * on both sides of the wire: the two `.server` modules throw it, and the client
+ * error boundary has to recognise it to stay quiet. A `.server` import in a
+ * client component would pull `node:crypto` and the honeypot into the browser
+ * bundle, which is the whole reason those files are server-only.
+ */
+
+/**
+ * The body text. Uniform on purpose: reporting which check failed hands a
+ * scanner a map of what this endpoint validates, and there is nothing a client
+ * can usefully do with the distinction.
+ */
+export const REJECTED_SUBMISSION = 'Form not submitted properly'