fix(honeypot): answer 400 for a malformed honeypot field instead of 500

app/entry.server.tsx

@@ -11,6 +11,7 @@ import {
 	type HandleDocumentRequestFunction,
 } from 'react-router'
 import { getEnv, init } from './utils/env.server'
+import { logHoneypotSeedFingerprint } from './utils/honeypot.server'
 import { NonceProvider } from './utils/nonce-provider'
 import {
 	getRouteErrorCause,
@@ -21,6 +22,7 @@ import { makeTimings } from './utils/timing.server'
 export const streamTimeout = 5000
 
 init()
+logHoneypotSeedFingerprint()
 global.ENV = getEnv()
 
 type DocRequestArgs = Parameters<HandleDocumentRequestFunction>

app/utils/honeypot.server.ts

@@ -1,17 +1,107 @@
+import { createHash } from 'node:crypto'
 import { Honeypot, SpamError } from 'remix-utils/honeypot/server'
 
 export const honeypot = new Honeypot({
 	validFromFieldName: process.env.TESTING ? null : undefined,
 	encryptionSeed: process.env.HONEYPOT_SECRET,
 })
 
+/**
+ * The two `DOMException` names a junk `from__confirm` provokes:
+ * `InvalidCharacterError` when `atob` rejects the string, and `OperationError`
+ * when AES-GCM rejects what the string decoded to.
+ */
+const MALFORMED_FIELD_ERROR_NAMES = new Set([
+	'InvalidCharacterError',
+	'OperationError',
+])
+
+/**
+ * Whether `error` is the crypto layer choking on a `from__confirm` value, which
+ * makes it spam rather than a bug.
+ *
+ * `remix-utils` decrypts the field before it validates it, with no shape check
+ * in between, so a bot that fills the honeypot with junk takes the request down
+ * before any validation runs: `atob` rejects a value outside the base64
+ * alphabet, and a value that decodes to too few bytes for AES-GCM's IV and GCM
+ * tag leaves it with nothing it can authenticate.
+ *
+ * Matched on the class *and* `.name`, never on `.message`, which is not even
+ * stable within one Node version — `atob('!!!')` and `atob('a')` raise
+ * different text on the Node 26 in `engines`, and the "The provided data is too
+ * small" that production reported before this fix is not the text the same call
+ * produces today. The name is spec'd and held across the range.
+ *
+ * `DOMException` is a cross-realm class, so the class is read through the
+ * `Symbol.toStringTag` WebIDL puts on the interface rather than `instanceof`.
+ * The server is single-realm and `instanceof` would hold there; the test
+ * environment is not — vitest's jsdom global hands this module jsdom's
+ * `DOMException` while `atob` and `crypto.subtle` remain Node's — so
+ * `instanceof` misses exactly the two errors handled here. The tag is correct
+ * in both.
+ *
+ * Narrowed on purpose, because a `DOMException` outside this set still has to
+ * surface: `DataError` and `NotSupportedError` are the failures a real key or
+ * algorithm problem would raise, and dressing those up as spam would hide a
+ * genuine fault. Two limits are worth knowing about. This cannot tell *where*
+ * inside `check` the error came from, so a `DOMException` with either name
+ * raised by anything else in that call tree is downgraded too — today `atob`
+ * and `crypto.subtle` are the only things in there that throw. And an
+ * `OperationError` from a failed authentication tag is indistinguishable from
+ * a short one, so a `from__confirm` we *did* issue that no longer decrypts
+ * lands here as well; keeping `HONEYPOT_SECRET` stable across deploys is what
+ * prevents that, and it is a required env var validated at boot.
+ */
+export function isMalformedHoneypotFieldError(error: unknown) {
+	return (
+		Object.prototype.toString.call(error) === '[object DOMException]' &&
+		MALFORMED_FIELD_ERROR_NAMES.has((error as DOMException).name)
+	)
+}
+
 export async function checkHoneypot(formData: FormData) {
 	try {
 		await honeypot.check(formData)
 	} catch (error) {
-		if (error instanceof SpamError) {
+		// 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
+		// and stays a 500.
+		if (error instanceof SpamError || isMalformedHoneypotFieldError(error)) {
 			throw new Response('Form not submitted properly', { status: 400 })
 		}
 		throw error
 	}
 }
+
+/**
+ * Log a short fingerprint of the encryption seed, once, at boot.
+ *
+ * `checkHoneypot` answers 400 for a `from__confirm` it cannot decrypt. That is
+ * right for junk, and it is also true — silently, with nothing in Sentry —
+ * for a token this server issued before `HONEYPOT_SECRET` changed, because a
+ * failed authentication tag and a ciphertext too short to authenticate are the
+ * same `OperationError`. The two are indistinguishable where the failure
+ * happens, so the boot log is the only place a rotation is visible: two
+ * consecutive deploys printing different fingerprints is the signal.
+ *
+ * Eight hex characters is enough to tell two secrets apart and useless for
+ * recovering either. Called after `init()`, so the seed is known good by then;
+ * the unset branch is only here because `Honeypot` falls back to a random
+ * per-process seed, which would break every token the moment a second process
+ * existed.
+ */
+export function logHoneypotSeedFingerprint() {
+	const seed = process.env.HONEYPOT_SECRET
+	if (!seed) {
+		console.warn(
+			'⚠️ HONEYPOT_SECRET is unset. The honeypot is using a per-process random seed, so a from__confirm issued by one process will not verify in another.',
+		)
+		return
+	}
+	const fingerprint = createHash('sha256')
+		.update(seed)
+		.digest('hex')
+		.slice(0, 8)
+	console.info(`honeypot seed fingerprint: ${fingerprint}`)
+}

tests/honeypot.test.ts

@@ -0,0 +1,265 @@
+import { SpamError } from 'remix-utils/honeypot/server'
+import { afterAll, describe, expect, it, vi } from 'vitest'
+
+/**
+ * `remix-utils` decrypts the honeypot's `from__confirm` field before it
+ * validates it, with no shape check in between, so anything a bot puts there
+ * throws out of `check` as a `DOMException` rather than a `SpamError`:
+ *
+ * - not base64 at all → `atob` throws `InvalidCharacterError`
+ * - base64, but decoding to less than an IV plus a GCM tag → AES-GCM has
+ *   nothing to authenticate and `crypto.subtle.decrypt` throws `OperationError`
+ *
+ * Both were reaching Sentry as unhandled 500s from `POST /contact` and
+ * `POST /resources/newsletter`, on input anyone can send. `checkHoneypot` has
+ * to answer 400 for them without swallowing a genuine crypto failure, and
+ * without touching the happy path.
+ */
+
+// `honeypot.server.ts` builds its `Honeypot` from `process.env` at import time,
+// so the env has to be pinned first and the module registry cleared before the
+// import — otherwise a value exported in a developer's shell changes what these
+// tests assert. `TESTING` is the key that matters: setting it turns
+// `validFromFieldName` off, which makes `check` return before it ever decrypts.
+// Every case that depends on the decrypt would then fail loudly rather than
+// pass for the wrong reason — except the happy path, which would go green
+// without decrypting anything, and that is the one the "future timestamp" case
+// exists to catch.
+vi.stubEnv('TESTING', '')
+vi.stubEnv('HONEYPOT_SECRET', 'test-honeypot-secret')
+
+const { checkHoneypot, honeypot, isMalformedHoneypotFieldError } =
+	await import('#app/utils/honeypot.server')
+
+afterAll(() => {
+	vi.restoreAllMocks()
+	vi.unstubAllEnvs()
+})
+
+/**
+ * A form a human submits. `name__confirm` has to be here and blank: `check`
+ * only decrypts once it has passed that field, and a form carrying just
+ * `from__confirm` is rejected as missing the honeypot input before the crypto
+ * runs — so without this every case below would be a green 400 for the wrong
+ * reason.
+ */
+function formDataWith(validFrom: string) {
+	const formData = new FormData()
+	formData.set('name__confirm', '')
+	formData.set('from__confirm', validFrom)
+	return formData
+}
+
+/** The 400 both a `SpamError` and a malformed field are meant to produce. */
+async function captureResponse(promise: Promise<unknown>) {
+	try {
+		await promise
+	} catch (thrown) {
+		expect(thrown).toBeInstanceOf(Response)
+		return thrown as Response
+	}
+	throw new Error('expected checkHoneypot to reject, but it resolved')
+}
+
+describe('checkHoneypot with a malformed from__confirm', () => {
+	it('rejects a value that is not base64', async () => {
+		const response = await captureResponse(
+			checkHoneypot(formDataWith('!!!not base64!!!')),
+		)
+		expect(response.status).toBe(400)
+	})
+
+	it('rejects a single character, which is a base64 length that cannot exist', async () => {
+		// A length of 1 (mod 4) is not decodable at all, so this stops at
+		// `atob`. The shortest value that gets as far as AES-GCM is two
+		// characters, covered below.
+		const response = await captureResponse(checkHoneypot(formDataWith('a')))
+		expect(response.status).toBe(400)
+	})
+
+	it('rejects a value that decodes to fewer bytes than the IV', async () => {
+		// Two characters decode to one byte, which is less than the IV AES-GCM
+		// slices off, so it never gets far enough to look at a ciphertext. This
+		// is the shape a bot gets by echoing back any short base64 string.
+		const response = await captureResponse(checkHoneypot(formDataWith('aa')))
+		expect(response.status).toBe(400)
+	})
+
+	it('rejects a value that decodes to a whole IV but no GCM tag', async () => {
+		// 12 bytes clears the IV slice and still leaves nothing to authenticate,
+		// which is a different rejection inside AES-GCM from the short-IV one
+		// above even though both surface as `OperationError`.
+		const twelveBytes = btoa('x'.repeat(12))
+		expect(atob(twelveBytes)).toHaveLength(12)
+		const response = await captureResponse(
+			checkHoneypot(formDataWith(twelveBytes)),
+		)
+		expect(response.status).toBe(400)
+	})
+
+	it('rejects a well-formed value that fails authentication', async () => {
+		// Long enough to be a real token shape — a 12-byte IV plus ciphertext
+		// and tag — but the tag does not verify. This is what a bot produces by
+		// echoing a `from__confirm` scraped from somewhere else, and it is the
+		// case that makes the narrowed check worth having.
+		const forged = btoa('x'.repeat(40))
+		expect(atob(forged)).toHaveLength(40)
+		const response = await captureResponse(checkHoneypot(formDataWith(forged)))
+		expect(response.status).toBe(400)
+	})
+
+	it('rejects a non-string value sent as a file upload', async () => {
+		// A bot that names a file `from__confirm` makes `atob` stringify a
+		// `File`, which is outside the base64 alphabet.
+		const formData = new FormData()
+		formData.set('name__confirm', '')
+		formData.set(
+			'from__confirm',
+			new File(['junk'], 'junk.txt', { type: 'text/plain' }),
+		)
+		const response = await captureResponse(checkHoneypot(formData))
+		expect(response.status).toBe(400)
+	})
+})
+
+describe('checkHoneypot with a real submission', () => {
+	it('accepts a genuine from__confirm issued by getInputProps()', async () => {
+		const { encryptedValidFrom } = await honeypot.getInputProps()
+		await expect(
+			checkHoneypot(formDataWith(encryptedValidFrom)),
+		).resolves.toBeUndefined()
+	})
+
+	it('rejects a future from__confirm with the same 400', async () => {
+		// Doubles as the guard against this file going vacuous: the only thing
+		// that can turn a correctly encrypted value into a 400 is the timestamp
+		// check, which only runs if `check` decrypted the field.
+		const { encryptedValidFrom } = await honeypot.getInputProps({
+			validFromTimestamp: Date.now() + 60_000,
+		})
+		const response = await captureResponse(
+			checkHoneypot(formDataWith(encryptedValidFrom)),
+		)
+		expect(response.status).toBe(400)
+	})
+
+	it('rejects a filled honeypot field as before', async () => {
+		const { encryptedValidFrom } = await honeypot.getInputProps()
+		const formData = new FormData()
+		formData.set('name__confirm', 'I am a bot')
+		formData.set('from__confirm', encryptedValidFrom)
+		const response = await captureResponse(checkHoneypot(formData))
+		expect(response.status).toBe(400)
+	})
+})
+
+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
+		// report, and that default lives in `checkHoneypot` rather than in the
+		// predicate — so it has to be asserted here. Without this case, replacing
+		// the catch with a blanket one leaves every other case in this file green.
+		const cryptoFailure = new DOMException('the key is bad', 'DataError')
+		vi.spyOn(honeypot, 'check').mockRejectedValueOnce(cryptoFailure)
+		const { encryptedValidFrom } = await honeypot.getInputProps()
+
+		await expect(checkHoneypot(formDataWith(encryptedValidFrom))).rejects.toBe(
+			cryptoFailure,
+		)
+	})
+
+	it('rethrows a non-DOMException unchanged', async () => {
+		const boom = new Error('something else broke')
+		vi.spyOn(honeypot, 'check').mockRejectedValueOnce(boom)
+
+		await expect(checkHoneypot(new FormData())).rejects.toBe(boom)
+	})
+})
+
+describe('isMalformedHoneypotFieldError', () => {
+	/**
+	 * The property this predicate is built on, read off the errors the runtime
+	 * actually throws rather than off one this test file constructed — the
+	 * `new DOMException(...)` cases below cannot show that a runtime's error
+	 * carries the tag, only that the constructor being used here sets it.
+	 */
+	it('recognises the DOMExceptions atob and crypto.subtle really throw', async () => {
+		const thrown: unknown[] = []
+		try {
+			atob('!!!not base64!!!')
+		} catch (error) {
+			thrown.push(error)
+		}
+		const key = await crypto.subtle.importKey(
+			'raw',
+			new Uint8Array(32),
+			{ name: 'AES-GCM' },
+			false,
+			['decrypt'],
+		)
+		try {
+			await crypto.subtle.decrypt(
+				{ name: 'AES-GCM', iv: new Uint8Array(12) },
+				key,
+				new Uint8Array(0),
+			)
+		} catch (error) {
+			thrown.push(error)
+		}
+
+		expect(thrown).toHaveLength(2)
+		for (const error of thrown) {
+			expect(Object.prototype.toString.call(error)).toBe(
+				'[object DOMException]',
+			)
+			expect(isMalformedHoneypotFieldError(error)).toBe(true)
+		}
+		// Both are different classes from this module's `DOMException` global,
+		// which is the whole reason the predicate reads the tag rather than
+		// using `instanceof`.
+		for (const error of thrown) {
+			expect(error).not.toBeInstanceOf(DOMException)
+		}
+	})
+
+	it('recognises the two errors the honeypot field provokes', () => {
+		expect(
+			isMalformedHoneypotFieldError(
+				new DOMException('not correctly encoded', 'InvalidCharacterError'),
+			),
+		).toBe(true)
+		expect(
+			isMalformedHoneypotFieldError(
+				new DOMException('data is too small', 'OperationError'),
+			),
+		).toBe(true)
+	})
+
+	it('does not swallow a crypto error from another source', () => {
+		// The point of narrowing on `.name`: a `DataError` or
+		// `NotSupportedError` — what a bad key or algorithm raises — still has to
+		// reach Sentry rather than be dressed up as spam.
+		expect(
+			isMalformedHoneypotFieldError(new DOMException('bad key', 'DataError')),
+		).toBe(false)
+		expect(
+			isMalformedHoneypotFieldError(
+				new DOMException('too many arguments', 'NotSupportedError'),
+			),
+		).toBe(false)
+	})
+
+	it('is not fooled by a non-DOMException wearing one of those names', () => {
+		// `.name` alone is assignable; the class tag is what makes the check
+		// mean "the runtime's DOMException" rather than "anything named that".
+		const impostor = new Error('too small')
+		impostor.name = 'OperationError'
+		expect(isMalformedHoneypotFieldError(impostor)).toBe(false)
+		expect(isMalformedHoneypotFieldError(new SpamError('filled'))).toBe(false)
+		expect(isMalformedHoneypotFieldError(new Error('boom'))).toBe(false)
+		expect(isMalformedHoneypotFieldError(undefined)).toBe(false)
+		expect(isMalformedHoneypotFieldError({ name: 'OperationError' })).toBe(
+			false,
+		)
+	})
+})