From 4900746ec2c297edda10fcd50408cef2fe5d2425 Mon Sep 17 00:00:00 2001 From: Arpit Dalal Date: Tue, 29 Sep 2026 18:09:50 -0400 Subject: [PATCH 1/4] fix(forms): stop unreadable bodies and omitted honeypot fields 500ing Two ways a POST to /contact or /resources/newsletter could reach Sentry as an unhandled exception, both reachable by anyone who can send a request to a public form URL. request.formData() 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, before any of the action's own code runs. That answered 500 for every request carrying a JSON body, no body at all, or an invented Content-Type -- and it threw ahead of checkHoneypot, so the 400 path that already exists for junk traffic never saw them. remix-utils' shouldCheckHoneypot returns false for a form carrying neither honeypot field, and check then returns without throwing. That is the right call for a route with no honeypot, but every form on these two routes renders one unconditionally, so a bot could skip the check entirely by not sending the fields. readFormData reads the media type off the header and throws a 400 otherwise, the same contract checkHoneypot already uses. checkHoneypot now requires name__confirm, and from__confirm when it is configured, before delegating. Omission returns the same 400 as a filled field, so neither reaches Sentry. --- app/routes/_marketing+/contact.tsx | 3 +- app/routes/resources+/newsletter.tsx | 3 +- app/utils/form-data.server.ts | 71 +++++++++++++ app/utils/honeypot.server.ts | 63 +++++++++++- tests/honeypot.test.ts | 90 +++++++++++++++- tests/server.smoke.test.ts | 147 +++++++++++++++++++++++++++ 6 files changed, 369 insertions(+), 8 deletions(-) create mode 100644 app/utils/form-data.server.ts diff --git a/app/routes/_marketing+/contact.tsx b/app/routes/_marketing+/contact.tsx index e7cecb4..b61ed67 100644 --- a/app/routes/_marketing+/contact.tsx +++ b/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 }) diff --git a/app/routes/resources+/newsletter.tsx b/app/routes/resources+/newsletter.tsx index 85d5dd0..12f029c 100644 --- a/app/routes/resources+/newsletter.tsx +++ b/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 }) diff --git a/app/utils/form-data.server.ts b/app/utils/form-data.server.ts new file mode 100644 index 0000000..3968c29 --- /dev/null +++ b/app/utils/form-data.server.ts @@ -0,0 +1,71 @@ +/** + * 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. + */ + +/** + * 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', +]) + +/** + * 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. + * + * The media type is read off the header rather than inferred from the body. A + * body is not required to carry a `Content-Type` at all — 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. The parameters after `;` + * are dropped, because the boundary on a `multipart/form-data` type is required + * and the charset on a urlencoded one is optional; comparing the whole header + * string would reject valid submissions that merely declared a charset. + * + * 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 { + const mediaType = request.headers + .get('content-type') + ?.split(';', 1)[0] + ?.trim() + .toLowerCase() + + if (!mediaType || !FORM_MEDIA_TYPES.has(mediaType)) { + throw new Response( + 'Content-Type was not one of "multipart/form-data" or "application/x-www-form-urlencoded".', + { + status: 400, + }, + ) + } + + return request.formData() +} diff --git a/app/utils/honeypot.server.ts b/app/utils/honeypot.server.ts index 74b0b00..f978931 100644 --- a/app/utils/honeypot.server.ts +++ b/app/utils/honeypot.server.ts @@ -1,8 +1,26 @@ import { createHash } from 'node:crypto' import { Honeypot, SpamError } from 'remix-utils/honeypot/server' +/** + * The field names, named here rather than left to `remix-utils`' defaults so + * that `checkHoneypot` can require them and stay in step with the config below. + * `Honeypot` exposes both as `protected`, so there is no supported way to read + * back what it was configured with. + */ +const NAME_FIELD_NAME = 'name__confirm' + +/** + * `null` disables the `from__confirm` half of the check, which is what the test + * suite wants: it makes `check` return before it ever decrypts, so a case that + * accidentally depends on the crypto fails loudly instead of passing for the + * wrong reason. The consequence is that the input is not rendered either, so + * the field cannot be required — see `missingHoneypotFields`. + */ +const VALID_FROM_FIELD_NAME = process.env.TESTING ? null : 'from__confirm' + export const honeypot = new Honeypot({ - validFromFieldName: process.env.TESTING ? null : undefined, + nameFieldName: NAME_FIELD_NAME, + validFromFieldName: VALID_FROM_FIELD_NAME, encryptionSeed: process.env.HONEYPOT_SECRET, }) @@ -59,13 +77,52 @@ export function isMalformedHoneypotFieldError(error: unknown) { ) } +/** + * The honeypot fields this submission is missing, if any. + * + * `remix-utils` treats a form carrying *neither* honeypot field as one it has + * no opinion about: `shouldCheckHoneypot` returns false and `check` returns + * without throwing. That is a deliberate accommodation for forms that are not + * protected by a honeypot at all — but on these routes every form is, and the + * fields are rendered unconditionally by `HoneypotInputs`, so their absence + * means the submission was not produced by our page. A bot that simply omits + * them walks straight past the check that exists to catch it. + * + * Requiring `name__confirm` is what closes that. It is the field the check is + * built around: a human cannot see it and leaves it blank, and a bot filling + * every input it finds trips it. A submission without it has skipped the + * question rather than answered it. + * + * `from__confirm` is required when it is configured, and skipped when it is + * `null` — with the timestamp half disabled the input is not rendered, so + * demanding it would reject every real submission in the environments that + * disable it. Read through the module's own constant rather than + * `honeypot.validFromFieldName`, which is `protected`. + */ +function missingHoneypotFields(formData: FormData): string[] { + const missing: string[] = [] + if (!formData.has(NAME_FIELD_NAME)) missing.push(NAME_FIELD_NAME) + if (VALID_FROM_FIELD_NAME && !formData.has(VALID_FROM_FIELD_NAME)) { + missing.push(VALID_FROM_FIELD_NAME) + } + return missing +} + export async function checkHoneypot(formData: FormData) { + // Checked before `honeypot.check` so a submission that skipped the fields + // is answered without paying for the decrypt, and so the rejection is ours + // rather than dependent on `check`'s no-op behaviour above. + const missing = missingHoneypotFields(formData) + if (missing.length > 0) { + throw new Response('Form not submitted properly', { 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 }) diff --git a/tests/honeypot.test.ts b/tests/honeypot.test.ts index 162c0e0..0ed6f0c 100644 --- a/tests/honeypot.test.ts +++ b/tests/honeypot.test.ts @@ -43,9 +43,9 @@ afterAll(() => { * runs — so without this every case below would be a green 400 for the wrong * reason. */ -function formDataWith(validFrom: string) { +function formDataWith(validFrom: string, name = '') { const formData = new FormData() - formData.set('name__confirm', '') + formData.set('name__confirm', name) formData.set('from__confirm', validFrom) return formData } @@ -153,6 +153,83 @@ describe('checkHoneypot with a real submission', () => { }) }) +describe('checkHoneypot with the honeypot fields omitted', () => { + // `remix-utils` has no opinion about a form carrying neither honeypot + // field: `shouldCheckHoneypot` returns false and `check` returns without + // throwing. That is the right call for routes with no honeypot, and the + // wrong one here, where every form renders the fields — so a bot could skip + // the check entirely by not sending them. These pin the requirement that + // closes it. + // + // This is the bypass in its most obvious form, and it needs no crypto, no + // forged token, and no knowledge of what the fields are called. + + it('rejects a submission carrying none of the honeypot fields', async () => { + // A well-formed contact submission with the honeypot simply left out — + // exactly what a bot that posts straight from a JSON payload sends. + const formData = new FormData() + formData.set('name', 'a') + formData.set('email', 'a@example.com') + formData.set('message', 'hi') + + const response = await captureResponse(checkHoneypot(formData)) + expect(response.status).toBe(400) + }) + + it('rejects a submission carrying only from__confirm', async () => { + // One field present is enough for `shouldCheckHoneypot` to say yes, and + // `check` then rejects it as a missing honeypot input — a 400, but by + // the slow path through the field check rather than the requirement + // this file is about. Pinned so the two cannot be confused. + const { encryptedValidFrom } = await honeypot.getInputProps() + const formData = new FormData() + formData.set('from__confirm', encryptedValidFrom) + + const response = await captureResponse(checkHoneypot(formData)) + expect(response.status).toBe(400) + }) + + it('rejects a submission carrying only name__confirm', async () => { + // The mirror image, and the one the requirement has to cover: with + // `name__confirm` present and blank, `check` gets all the way to the + // `from__confirm` lookup and throws "Missing honeypot valid from input". + // That is a rejection too, but it is incidental — it exists only + // because the timestamp is configured. Without this case, a + // `from__confirm`-only bypass would look covered by the happy path. + const formData = new FormData() + formData.set('name__confirm', '') + + const response = await captureResponse(checkHoneypot(formData)) + expect(response.status).toBe(400) + }) + + it('answers with the same 400 as a filled honeypot', async () => { + // Omission and filling are the same answer, so a bot cannot tell from + // the response which mistake it made, and neither reaches Sentry. + const { encryptedValidFrom } = await honeypot.getInputProps() + const omitted = await captureResponse(checkHoneypot(new FormData())) + const filled = await captureResponse( + checkHoneypot(formDataWith(encryptedValidFrom, 'I am a bot')), + ) + + expect(omitted.status).toBe(filled.status) + expect(await omitted.text()).toBe(await filled.text()) + }) + + it('does not reject a form that has the fields, however they arrived', async () => { + // The regression this could cause: the requirement has to key on + // presence, not on anything about the values, or a real browser + // submission stops working. A filled `name__confirm` is still spam and + // a valid `from__confirm` is still fine — both are covered above and + // in the happy-path describe; what matters here is that requiring the + // fields did not turn into rejecting them. + const { encryptedValidFrom } = await honeypot.getInputProps() + await expect( + checkHoneypot(formDataWith(encryptedValidFrom)), + ).resolves.toBeUndefined() + }) +}) + describe('checkHoneypot with an error that is not spam', () => { it('rethrows an unrecognised DOMException instead of dressing it up as spam', async () => { // The narrowed check is only worth having if the default is still to @@ -171,8 +248,15 @@ describe('checkHoneypot with an error that is not spam', () => { it('rethrows a non-DOMException unchanged', async () => { const boom = new Error('something else broke') vi.spyOn(honeypot, 'check').mockRejectedValueOnce(boom) + // Both honeypot fields have to be present, or the check for missing + // fields rejects first and the mock is never reached. This case is + // about what happens *after* a well-formed submission is handed to + // `check`, so the form has to be well-formed. + const { encryptedValidFrom } = await honeypot.getInputProps() - await expect(checkHoneypot(new FormData())).rejects.toBe(boom) + await expect(checkHoneypot(formDataWith(encryptedValidFrom))).rejects.toBe( + boom, + ) }) }) diff --git a/tests/server.smoke.test.ts b/tests/server.smoke.test.ts index 01027f0..b36875f 100644 --- a/tests/server.smoke.test.ts +++ b/tests/server.smoke.test.ts @@ -416,6 +416,153 @@ describe('production server', () => { ).toBeNull() }) + // A `Content-Type` `formData()` cannot read used to throw a `TypeError` + // out of the action, which is a 500 and an unhandled exception in Sentry. + // Scanners reach a public form URL on their own, and a body-less `POST` — + // no `Content-Type` at all — is the cheapest request they send, so this is + // traffic that arrives rather than a shape to imagine. Both routes are + // covered because both actions read a form body. + // + // The status alone cannot separate the two 400s these routes have: the + // honeypot's own rejection is also a 400 (see above). The guard's body text + // is what tells them apart, so both are asserted on it. + // + // The label is carried in the fixture so the assertion can name the case + // that failed — `vitest/valid-expect` rejects a second argument that is not + // a literal, and a hoisted variable would not qualify. The results are + // compared as one array so a single `toEqual` diff names the bad case. + const UNREADABLE_POSTS: Array<{ + label: string + path: string + init: { body?: string; headers?: Record } + }> = [ + { label: '/contact, no content-type', path: '/contact', init: {} }, + { + label: '/contact, application/json', + path: '/contact', + init: { + body: JSON.stringify({ name: 'bot' }), + headers: { 'content-type': 'application/json' }, + }, + }, + { + label: '/resources/newsletter, no content-type', + path: '/resources/newsletter', + init: {}, + }, + { + label: '/resources/newsletter, application/json', + path: '/resources/newsletter', + init: { body: '{}', headers: { 'content-type': 'application/json' } }, + }, + ] + + /** + * A real `from__confirm`, read out of the rendered `/contact` page. + * + * Since the honeypot fields became required, no hand-built `FormData` can + * stand in for a browser submission: one without them is now a 400, which + * is correct but makes a test that expects the form to be *accepted* prove + * nothing. The token is encrypted with `HONEYPOT_SECRET` and minted per + * render, so the only way to get a valid one is to read it off the page + * exactly as a browser would. + */ + async function scrapeHoneypotFields() { + const html = await (await fetch(`${base}/contact`)).text() + const encryptedValidFrom = /name="from__confirm"[^>]*value="([^"]*)"/.exec( + html, + )?.[1] + if (!encryptedValidFrom) { + throw new Error( + `no from__confirm in the rendered /contact page${serverOutput ? `\n${serverOutput}` : ''}`, + ) + } + return { encryptedValidFrom } + } + + it('answers an unreadable form body with 400, not 500', async () => { + const results: Array<{ label: string; status: number; guarded: boolean }> = + [] + for (const { label, path, init } of UNREADABLE_POSTS) { + const response = await fetch(`${base}${path}`, { + method: 'POST', + redirect: 'manual', + ...init, + }) + results.push({ + label, + status: response.status, + guarded: (await response.text()).includes( + 'Content-Type was not one of', + ), + }) + } + + expect(results).toEqual( + UNREADABLE_POSTS.map(({ label }) => ({ + label, + status: 400, + guarded: true, + })), + ) + + await new Promise((resolve) => setTimeout(resolve, 500)) + expect( + server?.exitCode, + `server should still be running${serverOutput ? `\n${serverOutput}` : ''}`, + ).toBeNull() + }) + + // The honeypot fields are now required, so a submission that omits them is + // answered 400 even though its body is perfectly readable. That is the + // intended behaviour — a form that skipped the fields was not produced by + // our page — and it is the other half of what the fix was for. Without this + // case the requirement is only pinned at the unit boundary, where a test can + // hand `checkHoneypot` a `FormData` no browser would ever build. + it('answers a submission that omits the honeypot fields with 400', async () => { + const response = await fetch(`${base}/contact`, { + method: 'POST', + redirect: 'manual', + body: new URLSearchParams({ + name: 'a', + email: 'a@example.com', + message: 'hi', + }), + headers: { 'content-type': 'application/x-www-form-urlencoded' }, + }) + expect(response.status).toBe(400) + expect(await response.text()).toContain('Form not submitted properly') + }) + + // A `charset` on the media type must not be mistaken for an unreadable + // body: a browser form is entitled to send one, and rejecting it would + // break the contact form for every real user whose browser does. + // + // The proof that the body was read is that the request gets all the way to + // Zod, which answers 200 with a validation reply. The email is deliberately + // invalid, and the honeypot fields are real ones scraped from the rendered + // page — with the fields required, a body without them is rejected before + // Zod is ever reached and this test would pass for the wrong reason. + it('reads a form body that declares a charset', async () => { + const { encryptedValidFrom } = await scrapeHoneypotFields() + const response = await fetch(`${base}/contact`, { + method: 'POST', + redirect: 'manual', + body: new URLSearchParams({ + name: 'a', + email: 'bad', + message: 'hi', + name__confirm: '', + from__confirm: encryptedValidFrom, + }), + headers: { + 'content-type': 'application/x-www-form-urlencoded; charset=UTF-8', + }, + }) + expect(response.status).toBe(200) + expect(await response.text()).not.toContain('Content-Type was not one of') + }) + // The morgan `url` token decodes the whole request URL, query string // included. morgan evaluates its tokens from the response's `finished` // event with no try/catch, so a URIError there escapes as an uncaught From 2d538538c3207fb7959cc3df8cde251f9dd59c6a Mon Sep 17 00:00:00 2001 From: Arpit Dalal Date: Tue, 29 Sep 2026 18:22:58 -0400 Subject: [PATCH 2/4] fix(forms): catch every body formData() can refuse, not just bad media types CodeRabbit caught that the allowlist in readFormData was necessary but not sufficient: multipart/form-data carries its boundary as a header *parameter*, so a request declaring a media type we accept can still be unreadable. Verified against the runtime -- a boundary-less multipart body makes formData() throw a TypeError, which escaped as the same 500 this was meant to remove. Enumerating the headers that produce a parse failure would not be a fix, it would be a list of today's, and undici is free to refuse a body for a reason nobody enumerated. So the parse itself is wrapped and every TypeError it throws is answered 400. Only a TypeError is converted: that is how formData() reports an unreadable body, and anything else is a fault on this side of the wire that still belongs in Sentry. The boundary cases now live in tests/form-data.test.ts rather than the smoke test. /contact and /resources/newsletter are guarded by a single express-rate-limit instance keyed by IP, so both draw on one budget of 10 per minute rather than 10 each -- an end-to-end matrix spent it and started failing the honeypot and charset cases with 429s that had nothing to do with the code. The smoke test keeps one case per route, which is all it was ever proving: that each action is wired to the helper. All rejections now share one body, so a client cannot tell a refused body from a honeypot rejection and cannot map what the endpoint checks. --- app/utils/form-data.server.ts | 67 ++++++++----- tests/form-data.test.ts | 177 ++++++++++++++++++++++++++++++++++ tests/server.smoke.test.ts | 73 ++++++-------- 3 files changed, 251 insertions(+), 66 deletions(-) create mode 100644 tests/form-data.test.ts diff --git a/app/utils/form-data.server.ts b/app/utils/form-data.server.ts index 3968c29..c0f51c9 100644 --- a/app/utils/form-data.server.ts +++ b/app/utils/form-data.server.ts @@ -32,6 +32,13 @@ const FORM_MEDIA_TYPES = new Set([ 'multipart/form-data', ]) +/** + * The body text for both rejections, so a client sees one answer regardless of + * why its body was refused. The alternative — reporting which check failed — + * hands a scanner a map of what this endpoint validates. + */ +const UNREADABLE_BODY = 'Form not submitted properly' + /** * Reads the request body as `FormData`, or answers 400. * @@ -39,33 +46,49 @@ const FORM_MEDIA_TYPES = new Set([ * both failure modes on these routes land as a 400 that React Router turns into * a status code rather than an unhandled exception. * - * The media type is read off the header rather than inferred from the body. A - * body is not required to carry a `Content-Type` at all — 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. The parameters after `;` - * are dropped, because the boundary on a `multipart/form-data` type is required - * and the charset on a urlencoded one is optional; comparing the whole header - * string would reject valid submissions that merely declared a charset. - * * 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 { - const mediaType = request.headers - .get('content-type') - ?.split(';', 1)[0] - ?.trim() - .toLowerCase() + // 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()` throws when it is missing — so a request + // that declares 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 undici is free to reject a body for a reason nobody + // enumerated. So every throw from the parse itself is caught and answered the + // same way, which holds regardless of which reason it was. + // + // Only a `TypeError` is converted. `formData()` signals an unparseable body + // with one, and it is the only thing it throws for this input; anything else + // (a `File` handle that cannot be read, say) is a fault on this side of the + // wire and still belongs in Sentry. + try { + const mediaType = request.headers + .get('content-type') + ?.split(';', 1)[0] + ?.trim() + .toLowerCase() - if (!mediaType || !FORM_MEDIA_TYPES.has(mediaType)) { - throw new Response( - 'Content-Type was not one of "multipart/form-data" or "application/x-www-form-urlencoded".', - { - status: 400, - }, - ) - } + if (!mediaType || !FORM_MEDIA_TYPES.has(mediaType)) { + throw new Response(UNREADABLE_BODY, { status: 400 }) + } - return request.formData() + return await request.formData() + } catch (error) { + if (error instanceof TypeError) { + throw new Response(UNREADABLE_BODY, { status: 400 }) + } + throw error + } } diff --git a/tests/form-data.test.ts b/tests/form-data.test.ts new file mode 100644 index 0000000..1281099 --- /dev/null +++ b/tests/form-data.test.ts @@ -0,0 +1,177 @@ +import { describe, expect, it, vi } from 'vitest' + +/** + * `readFormData` is a pure function of the `Content-Type` header and the body, + * so every shape it has to survive can be exercised here without a server. + * + * This is deliberately a unit test rather than more cases in + * `tests/server.smoke.test.ts`. The two "strong" paths in `server/index.ts` — + * `/contact` and `/resources/newsletter` — are guarded by a *single* + * `express-rate-limit` instance keyed by IP, so every POST to either one draws + * from the same budget of 10 per minute. An end-to-end matrix of every + * unreadable body therefore spends rate-limit budget that the honeypot and + * end-to-end cases need, and starts failing with a 429 that has nothing to do + * with the code under test. The boundary cases belong here; the smoke test + * keeps one representative per route to prove the helper is the one wired in. + */ + +const { readFormData } = await import('#app/utils/form-data.server') + +/** The 400 every rejection answers with, deliberately uniform. */ +const UNREADABLE_BODY = 'Form not submitted properly' + +function request(contentType: string | null, body?: string): Request { + const headers: Record = {} + if (contentType !== null) headers['content-type'] = contentType + return new Request('http://localhost/contact', { + method: 'POST', + body, + headers, + }) +} + +async function captureStatus(promise: Promise) { + try { + await promise + } catch (thrown) { + expect(thrown).toBeInstanceOf(Response) + const response = thrown as Response + expect(await response.text()).toBe(UNREADABLE_BODY) + return response.status + } + throw new Error('expected readFormData to reject, but it resolved') +} + +describe('readFormData with a media type it does not accept', () => { + // The common case, and the one the original Sentry report was: scanner + // traffic that never claimed to be a form. A body is not required to carry + // a `Content-Type` at all, so its absence has to be handled as a refusal + // rather than read past. + it.each([ + ['no content-type at all', null], + ['a JSON body', 'application/json'], + ['text/plain', 'text/plain'], + ['an empty content-type', ''], + ['a near miss on the spelling', 'application/x-www-form-urlencode'], + ['a type with trailing whitespace and mixed case', ' APPLICATION/JSON '], + ])('answers 400 for %s', async (_label, contentType) => { + expect(await captureStatus(readFormData(request(contentType, 'x')))).toBe( + 400, + ) + }) +}) + +describe('readFormData with a form media type it cannot parse', () => { + // The case a media-type allowlist alone does not catch, and the reason the + // parse is wrapped rather than the headers enumerated. `multipart/form-data` + // carries its boundary as a header *parameter*, so a client that declares + // the type and omits the boundary produces a body undici refuses — verified + // against the runtime rather than assumed, since it is the whole justification + // for catching the `TypeError` instead of listing more header shapes. + it('answers 400 for multipart/form-data with no boundary', async () => { + const req = request('multipart/form-data', 'not a multipart body') + // Pin the premise: this really is unreadable, so a 400 below is the fix + // working rather than the media-type check doing all of it. + await expect( + new Request('http://localhost/', { + method: 'POST', + body: 'not a multipart body', + headers: { 'content-type': 'multipart/form-data' }, + }).formData(), + ).rejects.toThrow(TypeError) + + expect(await captureStatus(readFormData(req))).toBe(400) + }) + + it('answers 400 for a multipart body whose boundary is not in the body', async () => { + // A boundary that is declared but never used to delimit anything is + // still an unparseable body. + expect( + await captureStatus( + readFormData( + request('multipart/form-data; boundary=abc123', 'no parts here'), + ), + ), + ).toBe(400) + }) +}) + +describe('readFormData with a body it can read', () => { + // The regression these guard: a guard that is too eager breaks the contact + // form for every real user, and the only thing that catches that here is + // these cases. + it('reads a urlencoded body', async () => { + const formData = await readFormData( + request('application/x-www-form-urlencoded', 'name=Arpit&message=hi'), + ) + expect(formData.get('name')).toBe('Arpit') + expect(formData.get('message')).toBe('hi') + }) + + it('reads a urlencoded body that declares a charset', async () => { + // The parameters after `;` are dropped before comparison. A browser form + // is entitled to send a charset, and rejecting it would break the form + // for real users — so this is a behavioural requirement, not a nicety. + const formData = await readFormData( + request( + 'application/x-www-form-urlencoded; charset=UTF-8', + 'name=Arpit&message=hi', + ), + ) + expect(formData.get('name')).toBe('Arpit') + }) + + it('reads a urlencoded body regardless of the media type casing', async () => { + const formData = await readFormData( + request('Application/X-WWW-Form-Urlencoded', 'name=Arpit'), + ) + expect(formData.get('name')).toBe('Arpit') + }) + + it('reads a multipart body with a boundary', async () => { + const boundary = '----testBoundary' + const body = [ + `--${boundary}`, + 'Content-Disposition: form-data; name="name"', + '', + 'Arpit', + `--${boundary}--`, + '', + ].join('\r\n') + + const formData = await readFormData( + request(`multipart/form-data; boundary=${boundary}`, body), + ) + expect(formData.get('name')).toBe('Arpit') + }) +}) + +describe('readFormData and errors that are not about the body', () => { + // The narrowing that keeps this a fix rather than a blanket catch. + // `formData()` signals an unreadable body with a `TypeError`, so that is the + // only thing converted; anything else is a fault on this side of the wire + // and still belongs in Sentry. Without this case, widening the catch to + // `catch (e) { throw new Response(...) }` silences real faults too and every + // other test in this file stays green. + it('rethrows a non-TypeError from the parse unchanged', async () => { + const original = request('application/x-www-form-urlencoded', 'name=Arpit') + // Stands in for a `File` handle that cannot be read, or any future + // failure that is not "this body is junk". + const boom = new Error('something on this side broke') + vi.spyOn(original, 'formData').mockRejectedValueOnce(boom) + + await expect(readFormData(original)).rejects.toBe(boom) + }) + + it('converts a TypeError from the parse, which is the body being unreadable', async () => { + // The other half of the narrowing, and the reason it is drawn on the + // class rather than on a message: a `TypeError` is how `formData()` + // reports a body it cannot parse, so it is converted whatever its text. + const original = request('application/x-www-form-urlencoded', 'name=Arpit') + vi.spyOn(original, 'formData').mockRejectedValueOnce( + new TypeError('Failed to parse body as FormData.'), + ) + + expect(await captureStatus(readFormData(original))).toBe(400) + }) +}) diff --git a/tests/server.smoke.test.ts b/tests/server.smoke.test.ts index b36875f..d217cbc 100644 --- a/tests/server.smoke.test.ts +++ b/tests/server.smoke.test.ts @@ -372,8 +372,11 @@ describe('production server', () => { // without touching the crypto path. No other field is needed: both actions // run the honeypot check before validating. // - // Four requests here plus the page load above stay well inside the 10/min - // limit on /contact and the 100/min one on /resources/newsletter. + // Four requests here plus the page load above stay inside the rate limit: + // `/contact` and `/resources/newsletter` share one `express-rate-limit` + // instance keyed by IP, so the budget is 10 per minute across both rather + // than 10 each. Adding a case to this file can therefore break a test + // elsewhere in it with a 429 — see `UNREADABLE_POSTS` below. const HONEYPOT_POSTS: Array<[path: string, validFrom: string]> = [ ['/contact', '!!!not base64!!!'], ['/contact', 'a'], @@ -416,45 +419,30 @@ describe('production server', () => { ).toBeNull() }) - // A `Content-Type` `formData()` cannot read used to throw a `TypeError` - // out of the action, which is a 500 and an unhandled exception in Sentry. - // Scanners reach a public form URL on their own, and a body-less `POST` — - // no `Content-Type` at all — is the cheapest request they send, so this is - // traffic that arrives rather than a shape to imagine. Both routes are - // covered because both actions read a form body. + // A body `formData()` cannot read used to throw a `TypeError` out of the + // action, which is a 500 and an unhandled exception in Sentry. Scanners reach + // a public form URL on their own, and a body-less `POST` — no `Content-Type` + // at all — is the cheapest request they send, so this is traffic that + // arrives rather than a shape to imagine. // - // The status alone cannot separate the two 400s these routes have: the - // honeypot's own rejection is also a 400 (see above). The guard's body text - // is what tells them apart, so both are asserted on it. - // - // The label is carried in the fixture so the assertion can name the case - // that failed — `vitest/valid-expect` rejects a second argument that is not - // a literal, and a hoisted variable would not qualify. The results are - // compared as one array so a single `toEqual` diff names the bad case. + // One case per route, deliberately. `/contact` and `/resources/newsletter` + // are guarded by a *single* `express-rate-limit` instance keyed by IP (see + // `strongPaths` in `server/index.ts`), so every POST to either draws on one + // shared budget of 10 per minute — not 10 each. The full matrix of + // unreadable bodies lives in `tests/form-data.test.ts`, where it costs no + // budget at all; what is worth an end-to-end request here is only that each + // action is wired to the helper at all. const UNREADABLE_POSTS: Array<{ label: string path: string init: { body?: string; headers?: Record } }> = [ { label: '/contact, no content-type', path: '/contact', init: {} }, - { - label: '/contact, application/json', - path: '/contact', - init: { - body: JSON.stringify({ name: 'bot' }), - headers: { 'content-type': 'application/json' }, - }, - }, { label: '/resources/newsletter, no content-type', path: '/resources/newsletter', init: {}, }, - { - label: '/resources/newsletter, application/json', - path: '/resources/newsletter', - init: { body: '{}', headers: { 'content-type': 'application/json' } }, - }, ] /** @@ -481,29 +469,23 @@ describe('production server', () => { } it('answers an unreadable form body with 400, not 500', async () => { - const results: Array<{ label: string; status: number; guarded: boolean }> = - [] + // Status only, deliberately. The body text is the same for every + // rejection on these routes, so it cannot tell a refused body from a + // honeypot rejection — and that is the intent: one answer for "we did + // not accept this submission", whatever the reason. + const results: Array<{ label: string; status: number }> = [] for (const { label, path, init } of UNREADABLE_POSTS) { const response = await fetch(`${base}${path}`, { method: 'POST', redirect: 'manual', ...init, }) - results.push({ - label, - status: response.status, - guarded: (await response.text()).includes( - 'Content-Type was not one of', - ), - }) + results.push({ label, status: response.status }) + await response.text() } expect(results).toEqual( - UNREADABLE_POSTS.map(({ label }) => ({ - label, - status: 400, - guarded: true, - })), + UNREADABLE_POSTS.map(({ label }) => ({ label, status: 400 })), ) await new Promise((resolve) => setTimeout(resolve, 500)) @@ -560,7 +542,10 @@ describe('production server', () => { }, }) expect(response.status).toBe(200) - expect(await response.text()).not.toContain('Content-Type was not one of') + // A 200 here already proves the body was read — a refused body is a 400 + // on the way in — so this only rules out the charset itself being + // mistaken for something unreadable, which would have produced that 400. + expect(await response.text()).not.toContain('Form not submitted properly') }) // The morgan `url` token decodes the whole request URL, query string From ba2b9eeb84f70cfc17d8e22852e37c62b3779c66 Mon Sep 17 00:00:00 2001 From: Arpit Dalal Date: Tue, 29 Sep 2026 18:59:13 -0400 Subject: [PATCH 3/4] fix(forms): keep a rejected submission out of Sentry and off the page A three-axis review of the previous two commits found the fix did not actually achieve its stated goal. It said junk traffic is cheap and silent, and neither was true: a 400 thrown from an action is an ErrorResponse to React Router exactly as a crash is, and GeneralErrorBoundary calls captureException unconditionally. So every rejection this change added 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 was enough. The 400 also replaced the whole page with a bare error, taking the user's typed message with it. isRejectedSubmission now recognises the guards' own answer, on the status and the shared body together: the body alone would silence any future 500 carrying the same text, and the status alone would re-report the 400 the router raises for a scanner probing a path, which sentry-event-filters.ts already drops server-side. Narrowed on both, and pinned from both sides in tests/error-boundary.test.ts. Also fixed, from the same review: The honeypot no longer hardcodes its field names. shouldCheckHoneypot is protected, which is the library marking it an extension point, so overriding it is the supported way to be strict -- verified that it gets the same SpamError through the library's own path. Reading the names off the base class instead of restating them also keeps the requirement true under randomizeNameFieldName, which a literal would have broken by rejecting every genuine submission. A body already read rejects with a TypeError too -- the spec's `consume body` step 1 says so, and it is what a double formData() produces. That is our bug, not the client's, and the catch was filing it as a rejected submission. request.bodyUsed is the spec'd way to tell them apart, and the test reproduces the double read for real rather than mocking it. The shared rejection text lived in three places and is now one constant in a module with no server-only imports, because the client boundary has to recognise it and could not import a .server file. The smoke test now counts its POSTs and fails in afterAll if it exceeds the shared 10/min budget, rather than leaving the next case to discover it as a 429 somewhere unrelated. It also deletes TESTING from the child env: a developer with it exported would render no from__confirm input and fail a test about a charset. Two comments the previous commits made confidently and wrongly: that TESTING disables the timestamp half the test suite depends on (it sets the empty string, which is falsy, so the crypto path is live), and that /contact and /resources/newsletter have 10/min each (one limiter, one IP key, 10/min shared). --- app/components/error-boundary.tsx | 37 +++++++++- app/utils/form-data.server.ts | 74 ++++++++++++++------ app/utils/honeypot.server.ts | 111 +++++++++++++++++------------- app/utils/rejected-submission.ts | 23 +++++++ tests/error-boundary.test.ts | 68 ++++++++++++++++++ tests/form-data.test.ts | 83 ++++++++++++++++++++++ tests/server.smoke.test.ts | 57 ++++++++++----- 7 files changed, 369 insertions(+), 84 deletions(-) create mode 100644 app/utils/rejected-submission.ts create mode 100644 tests/error-boundary.test.ts diff --git a/app/components/error-boundary.tsx b/app/components/error-boundary.tsx index 59feb30..d84a560 100644 --- a/app/components/error-boundary.tsx +++ b/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 }) => 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 }) => (

@@ -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 ( diff --git a/app/utils/form-data.server.ts b/app/utils/form-data.server.ts index c0f51c9..bf5b05e 100644 --- a/app/utils/form-data.server.ts +++ b/app/utils/form-data.server.ts @@ -23,6 +23,8 @@ * 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. @@ -33,11 +35,21 @@ const FORM_MEDIA_TYPES = new Set([ ]) /** - * The body text for both rejections, so a client sees one answer regardless of - * why its body was refused. The alternative — reporting which check failed — - * hands a scanner a map of what this endpoint validates. + * 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. */ -const UNREADABLE_BODY = 'Form not submitted properly' +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. @@ -55,24 +67,25 @@ export async function readFormData(request: Request): Promise { // 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. + // 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()` throws when it is missing — so a request - // that declares 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 undici is free to reject a body for a reason nobody - // enumerated. So every throw from the parse itself is caught and answered the - // same way, which holds regardless of which reason it was. + // *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. // - // Only a `TypeError` is converted. `formData()` signals an unparseable body - // with one, and it is the only thing it throws for this input; anything else - // (a `File` handle that cannot be read, say) is a fault on this side of the - // wire and still belongs in Sentry. + // 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') @@ -81,13 +94,32 @@ export async function readFormData(request: Request): Promise { .toLowerCase() if (!mediaType || !FORM_MEDIA_TYPES.has(mediaType)) { - throw new Response(UNREADABLE_BODY, { status: 400 }) + 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) { - if (error instanceof TypeError) { - throw new Response(UNREADABLE_BODY, { status: 400 }) + // `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 } diff --git a/app/utils/honeypot.server.ts b/app/utils/honeypot.server.ts index f978931..8818032 100644 --- a/app/utils/honeypot.server.ts +++ b/app/utils/honeypot.server.ts @@ -1,27 +1,66 @@ import { createHash } from 'node:crypto' import { Honeypot, SpamError } from 'remix-utils/honeypot/server' +import { REJECTED_SUBMISSION } from '#app/utils/rejected-submission' /** - * The field names, named here rather than left to `remix-utils`' defaults so - * that `checkHoneypot` can require them and stay in step with the config below. - * `Honeypot` exposes both as `protected`, so there is no supported way to read - * back what it was configured with. + * 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. */ -const NAME_FIELD_NAME = 'name__confirm' +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 + } +} /** - * `null` disables the `from__confirm` half of the check, which is what the test - * suite wants: it makes `check` return before it ever decrypts, so a case that - * accidentally depends on the crypto fails loudly instead of passing for the - * wrong reason. The consequence is that the input is not rendered either, so - * the field cannot be required — see `missingHoneypotFields`. + * `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. */ -const VALID_FROM_FIELD_NAME = process.env.TESTING ? null : 'from__confirm' - -export const honeypot = new Honeypot({ - nameFieldName: NAME_FIELD_NAME, - validFromFieldName: VALID_FROM_FIELD_NAME, +export const honeypot = new StrictHoneypot({ encryptionSeed: process.env.HONEYPOT_SECRET, + validFromFieldName: process.env.TESTING ? null : undefined, }) /** @@ -80,41 +119,21 @@ export function isMalformedHoneypotFieldError(error: unknown) { /** * The honeypot fields this submission is missing, if any. * - * `remix-utils` treats a form carrying *neither* honeypot field as one it has - * no opinion about: `shouldCheckHoneypot` returns false and `check` returns - * without throwing. That is a deliberate accommodation for forms that are not - * protected by a honeypot at all — but on these routes every form is, and the - * fields are rendered unconditionally by `HoneypotInputs`, so their absence - * means the submission was not produced by our page. A bot that simply omits - * them walks straight past the check that exists to catch it. - * - * Requiring `name__confirm` is what closes that. It is the field the check is - * built around: a human cannot see it and leaves it blank, and a bot filling - * every input it finds trips it. A submission without it has skipped the - * question rather than answered it. - * - * `from__confirm` is required when it is configured, and skipped when it is - * `null` — with the timestamp half disabled the input is not rendered, so - * demanding it would reject every real submission in the environments that - * disable it. Read through the module's own constant rather than - * `honeypot.validFromFieldName`, which is `protected`. + * 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[] { - const missing: string[] = [] - if (!formData.has(NAME_FIELD_NAME)) missing.push(NAME_FIELD_NAME) - if (VALID_FROM_FIELD_NAME && !formData.has(VALID_FROM_FIELD_NAME)) { - missing.push(VALID_FROM_FIELD_NAME) - } - return missing + 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, and so the rejection is ours - // rather than dependent on `check`'s no-op behaviour above. - const missing = missingHoneypotFields(formData) - if (missing.length > 0) { - throw new Response('Form not submitted properly', { status: 400 }) + // 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 { @@ -125,7 +144,7 @@ export async function checkHoneypot(formData: FormData) { // 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 } diff --git a/app/utils/rejected-submission.ts b/app/utils/rejected-submission.ts new file mode 100644 index 0000000..abca1a4 --- /dev/null +++ b/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' diff --git a/tests/error-boundary.test.ts b/tests/error-boundary.test.ts new file mode 100644 index 0000000..2ca503a --- /dev/null +++ b/tests/error-boundary.test.ts @@ -0,0 +1,68 @@ +import { UNSAFE_ErrorResponseImpl as ErrorResponseImpl } from 'react-router' +import { describe, expect, it } from 'vitest' +import { isRejectedSubmission } from '#app/components/error-boundary' +import { REJECTED_SUBMISSION } from '#app/utils/rejected-submission' + +/** + * `isRejectedSubmission` decides whether a route error reaches Sentry, so it is + * worth pinning on both sides: a submission the site refuses on purpose must + * stay out of Sentry, and everything else must not be silenced by accident. + * + * This matters more than it looks. A thrown `Response` is how both form guards + * answer a submission they will not accept, and React Router turns one into an + * `ErrorResponse` at the nearest boundary exactly as it does for a crash — so + * without this distinction every rejected submission was reported from the + * browser, with `replaysOnErrorSampleRate: 1.0` attaching a session replay to + * whoever happened to submit it. The newsletter form is in the footer of every + * page, so a single stray POST was enough. + */ + +function routeErrorResponse(data: unknown, status = 400) { + return new ErrorResponseImpl(status, statusText(status), data) +} + +function statusText(status: number) { + return status === 400 ? 'Bad Request' : 'OK' +} + +describe('isRejectedSubmission', () => { + it('recognises the 400 the form guards throw', () => { + expect(isRejectedSubmission(routeErrorResponse(REJECTED_SUBMISSION))).toBe( + true, + ) + }) + + // The narrowing that stops this from becoming "ignore every 400". React + // Router raises 400 itself for a request with no matching loader, which on a + // public site is a scanner probing for routes, and `sentry-event-filters.ts` + // already drops that server-side. Reporting it here would undo that work, so + // the match is on the shared body rather than the status. + it('does not claim a 400 the router raised itself', () => { + expect( + isRejectedSubmission( + routeErrorResponse('No route matches URL "/wp-admin"', 400), + ), + ).toBe(false) + }) + + it('does not claim a 500 that happens to carry the same text', () => { + expect( + isRejectedSubmission(routeErrorResponse(REJECTED_SUBMISSION, 500)), + ).toBe(false) + }) + + it('does not claim a real error', () => { + expect(isRejectedSubmission(new Error(REJECTED_SUBMISSION))).toBe(false) + expect(isRejectedSubmission(new TypeError(REJECTED_SUBMISSION))).toBe(false) + }) + + it('is not fooled by a plain object that looks like an error response', () => { + // `isRouteErrorResponse` checks the prototype, not the shape. Matching on + // shape instead would let anything on the page suppress its own reporting. + expect( + isRejectedSubmission({ status: 400, data: REJECTED_SUBMISSION }), + ).toBe(false) + expect(isRejectedSubmission(undefined)).toBe(false) + expect(isRejectedSubmission(null)).toBe(false) + }) +}) diff --git a/tests/form-data.test.ts b/tests/form-data.test.ts index 1281099..4c785fb 100644 --- a/tests/form-data.test.ts +++ b/tests/form-data.test.ts @@ -16,6 +16,13 @@ import { describe, expect, it, vi } from 'vitest' */ const { readFormData } = await import('#app/utils/form-data.server') +// Imported here rather than at the top of the file because +// `honeypot.server.ts` builds its `Honeypot` from `process.env` at import time, +// so the env has to be pinned first — the same reason `tests/honeypot.test.ts` +// does this. Only the seed matters here, and only for determinism: the token is +// minted by this same instance and verified by this same instance. +vi.stubEnv('HONEYPOT_SECRET', 'form-data-test-secret') +const { checkHoneypot, honeypot } = await import('#app/utils/honeypot.server') /** The 400 every rejection answers with, deliberately uniform. */ const UNREADABLE_BODY = 'Form not submitted properly' @@ -146,6 +153,63 @@ describe('readFormData with a body it can read', () => { }) }) +describe('readFormData and checkHoneypot composed', () => { + // The two guards are the whole risk surface of these routes and both are + // one edit away from breaking the form for every human, so they are + // exercised here *together* on a body shaped the way a browser sends one. + // Individually they are covered elsewhere; what this pins is that a + // genuine submission passes both, which no single-file test can show — + // the unit suites hand each guard a hand-built `FormData` and stop. + // + // No network and no rate-limit budget, unlike an end-to-end accept case: + // a valid submission would reach `sendEmail` and open an SMTP connection + // to a host that does not exist. + it('accepts a browser-shaped submission through both guards', async () => { + // Exactly what `HoneypotInputs` renders: the name field blank because + // nobody can see it, and the timestamp token from the root loader. + const { encryptedValidFrom, nameFieldName, validFromFieldName } = + await honeypot.getInputProps() + const body = new URLSearchParams({ + name: 'Arpit', + email: 'someone@example.com', + message: 'hello', + [nameFieldName]: '', + [validFromFieldName as string]: encryptedValidFrom, + }) + + const request = new Request('http://localhost/contact', { + method: 'POST', + body, + headers: { 'content-type': 'application/x-www-form-urlencoded' }, + }) + + const formData = await readFormData(request) + await expect(checkHoneypot(formData)).resolves.toBeUndefined() + }) + + it('still rejects that same submission when the honeypot fields are dropped', async () => { + // The negative half, and the reason the positive one above is worth + // having: it proves the accept path is the guards agreeing, not the + // guards being absent. + const { nameFieldName } = await honeypot.getInputProps() + const body = new URLSearchParams({ + name: 'Arpit', + email: 'someone@example.com', + message: 'hello', + [nameFieldName]: '', + }) + + const request = new Request('http://localhost/contact', { + method: 'POST', + body, + headers: { 'content-type': 'application/x-www-form-urlencoded' }, + }) + + const formData = await readFormData(request) + expect(await captureStatus(checkHoneypot(formData))).toBe(400) + }) +}) + describe('readFormData and errors that are not about the body', () => { // The narrowing that keeps this a fix rather than a blanket catch. // `formData()` signals an unreadable body with a `TypeError`, so that is the @@ -174,4 +238,23 @@ describe('readFormData and errors that are not about the body', () => { expect(await captureStatus(readFormData(original))).toBe(400) }) + + it('rethrows the body-already-used TypeError rather than answering 400', async () => { + // The case that made the narrowing wrong. A disturbed or locked body + // rejects with a `TypeError` too — the spec's `consume body` step 1 says + // so, and it is what a double `formData()` produces — but it is *our* + // bug rather than the client's, and converting it would file a server + // fault as a rejected submission and hide it permanently. + // + // Reproduced for real rather than mocked, so the premise is pinned: this + // is genuinely the error a second read produces. + const original = request('application/x-www-form-urlencoded', 'name=Arpit') + await original.formData() + expect(original.bodyUsed).toBe(true) + await expect(original.formData()).rejects.toThrow(TypeError) + + await expect(readFormData(original)).rejects.toThrow( + /already consumed before the action read it/, + ) + }) }) diff --git a/tests/server.smoke.test.ts b/tests/server.smoke.test.ts index d217cbc..6352d8b 100644 --- a/tests/server.smoke.test.ts +++ b/tests/server.smoke.test.ts @@ -57,6 +57,23 @@ const TEST_ENV = { let server: ChildProcess | undefined let serverOutput = '' +/** + * Every POST this file makes to a rate-limited "strong" path. Kept as a running + * count so `afterAll` can prove the suite is inside the budget rather than + * leaving the next person to discover it as a 429 in an unrelated test. + */ +let STRONG_PATH_POSTS = 0 + +/** A `POST` to a strong path, counted against the budget above. */ +function postStrongPath(path: string, init: RequestInit = {}) { + STRONG_PATH_POSTS++ + return fetch(`${base}${path}`, { + method: 'POST', + redirect: 'manual', + ...init, + }) +} + async function waitForServer(url: string, child: ChildProcess) { // The healthcheck route is the cheapest thing that proves both that the // server is listening and that the React Router handler is mounted. @@ -111,6 +128,12 @@ beforeAll(async () => { // ALLOW_INDEXING. A developer who exports it would otherwise make the // "sends no X-Robots-Tag by default" test below fail on their machine only. delete env.ALLOW_INDEXING + // `TESTING` for the same reason and worse: `honeypot.server.ts` reads it to + // decide whether the `from__confirm` input exists at all, so a truthy value + // in a developer's shell would leave `scrapeHoneypotFields` below with + // nothing to scrape and fail a test about a charset. Not a key of `env`'s + // inferred type, so it goes through the index signature. + delete (env as Record).TESTING const child = spawn(process.execPath, ['index.ts'], { env, @@ -129,6 +152,15 @@ beforeAll(async () => { }, 60_000) afterAll(() => { + // The shared rate-limit budget is a real coupling in this file and nothing + // else enforces it, so a case added later would otherwise surface as an + // unrelated 429 inside whichever test ran next. Asserting it here turns that + // into a message about the budget. + if (STRONG_PATH_POSTS > 10) { + throw new Error( + `${STRONG_PATH_POSTS} POSTs to /contact or /resources/newsletter; they share one 10/min rate limit (server/index.ts strongPaths), so this suite is over budget`, + ) + } server?.kill('SIGKILL') }) @@ -372,11 +404,12 @@ describe('production server', () => { // without touching the crypto path. No other field is needed: both actions // run the honeypot check before validating. // - // Four requests here plus the page load above stay inside the rate limit: - // `/contact` and `/resources/newsletter` share one `express-rate-limit` - // instance keyed by IP, so the budget is 10 per minute across both rather - // than 10 each. Adding a case to this file can therefore break a test - // elsewhere in it with a 429 — see `UNREADABLE_POSTS` below. + // The requests below, plus the page load above, spend 8 of the shared + // 10/min budget. `/contact` and `/resources/newsletter` are guarded by one + // `express-rate-limit` instance keyed by IP (`strongPaths` in + // `server/index.ts`), so the budget is not per-path. Two to spare is enough + // for now and not enforced by anything — see `STRONG_PATH_POSTS` in + // `afterAll`, which fails loudly if a future case pushes past it. const HONEYPOT_POSTS: Array<[path: string, validFrom: string]> = [ ['/contact', '!!!not base64!!!'], ['/contact', 'a'], @@ -398,11 +431,9 @@ describe('production server', () => { name__confirm: '', from__confirm: validFrom, }) - const response = await fetch(`${base}${path}`, { - method: 'POST', + const response = await postStrongPath(path, { body, headers: { 'content-type': 'application/x-www-form-urlencoded' }, - redirect: 'manual', }) expect(response.status, `${path} with from__confirm=${validFrom}`).toBe( 400, @@ -475,11 +506,7 @@ describe('production server', () => { // not accept this submission", whatever the reason. const results: Array<{ label: string; status: number }> = [] for (const { label, path, init } of UNREADABLE_POSTS) { - const response = await fetch(`${base}${path}`, { - method: 'POST', - redirect: 'manual', - ...init, - }) + const response = await postStrongPath(path, init) results.push({ label, status: response.status }) await response.text() } @@ -502,9 +529,7 @@ describe('production server', () => { // case the requirement is only pinned at the unit boundary, where a test can // hand `checkHoneypot` a `FormData` no browser would ever build. it('answers a submission that omits the honeypot fields with 400', async () => { - const response = await fetch(`${base}/contact`, { - method: 'POST', - redirect: 'manual', + const response = await postStrongPath('/contact', { body: new URLSearchParams({ name: 'a', email: 'a@example.com', From d0dd25646ae9e6f933a50b47da9cdc8b94788c87 Mon Sep 17 00:00:00 2001 From: Arpit Dalal Date: Tue, 29 Sep 2026 19:01:44 -0400 Subject: [PATCH 4/4] test(smoke): count the charset POST against the shared rate-limit budget The last strong-path POST bypassed postStrongPath, so the afterAll guard undercounted by one and would have let a future case push the suite over budget without saying so. The finding also suggested counting the GET requests scrapeHoneypotFields makes; those go to the separate generalRateLimit, so they are not on this budget, and the comment now records why rather than leaving the next reader to work it out. --- tests/server.smoke.test.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/server.smoke.test.ts b/tests/server.smoke.test.ts index 6352d8b..7b33188 100644 --- a/tests/server.smoke.test.ts +++ b/tests/server.smoke.test.ts @@ -61,6 +61,11 @@ let serverOutput = '' * Every POST this file makes to a rate-limited "strong" path. Kept as a running * count so `afterAll` can prove the suite is inside the budget rather than * leaving the next person to discover it as a 429 in an unrelated test. + * + * POSTs only, and that is the limiter's own distinction: `server/index.ts` + * routes GET and HEAD to a separate, far larger `generalRateLimit`, so the page + * loads this file makes — including the one `scrapeHoneypotFields` does — are + * not drawing on this budget. */ let STRONG_PATH_POSTS = 0 @@ -552,9 +557,7 @@ describe('production server', () => { // Zod is ever reached and this test would pass for the wrong reason. it('reads a form body that declares a charset', async () => { const { encryptedValidFrom } = await scrapeHoneypotFields() - const response = await fetch(`${base}/contact`, { - method: 'POST', - redirect: 'manual', + const response = await postStrongPath('/contact', { body: new URLSearchParams({ name: 'a', email: 'bad',