diff --git a/app/entry.server.tsx b/app/entry.server.tsx index 854cdb2..f606ca6 100644 --- a/app/entry.server.tsx +++ b/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 diff --git a/app/utils/honeypot.server.ts b/app/utils/honeypot.server.ts index e0fd321..74b0b00 100644 --- a/app/utils/honeypot.server.ts +++ b/app/utils/honeypot.server.ts @@ -1,3 +1,4 @@ +import { createHash } from 'node:crypto' import { Honeypot, SpamError } from 'remix-utils/honeypot/server' export const honeypot = new Honeypot({ @@ -5,13 +6,102 @@ export const honeypot = new Honeypot({ 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}`) +} diff --git a/tests/honeypot.test.ts b/tests/honeypot.test.ts new file mode 100644 index 0000000..162c0e0 --- /dev/null +++ b/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) { + 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, + ) + }) +}) diff --git a/tests/server.smoke.test.ts b/tests/server.smoke.test.ts index 750d0c6..01027f0 100644 --- a/tests/server.smoke.test.ts +++ b/tests/server.smoke.test.ts @@ -359,6 +359,63 @@ describe('production server', () => { expect(await response.text()).not.toContain('name="robots"') }) + // The acceptance criteria for the honeypot fix are phrased at this + // boundary, not at `checkHoneypot`, and tests/honeypot.test.ts can only + // reach them by reasoning about how React Router turns a thrown Response + // into a status code. Three shapes of the `from__confirm` field, because a + // bot that fills it with junk reaches the same code by three routes: not + // base64, a base64 length that cannot exist, and base64 that decodes to too + // few bytes for AES-GCM's IV and tag. + // + // `name__confirm` has to be present and blank, or `check` rejects the form + // as a missing honeypot before it decrypts anything and the test would pass + // 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. + const HONEYPOT_POSTS: Array<[path: string, validFrom: string]> = [ + ['/contact', '!!!not base64!!!'], + ['/contact', 'a'], + ['/contact', 'aa'], + ['/resources/newsletter', '!!!not base64!!!'], + ] + + it('logs a honeypot seed fingerprint at boot', async () => { + // The 400 above is deliberately silent: a `from__confirm` this server + // issued that no longer decrypts is answered the same way as junk, with + // nothing in Sentry. The fingerprint is the only place a rotated + // HONEYPOT_SECRET shows up, so pin that it is still being printed. + expect(serverOutput).toMatch(/honeypot seed fingerprint: [0-9a-f]{8}/) + }) + + it('answers a malformed honeypot field with 400, not 500', async () => { + for (const [path, validFrom] of HONEYPOT_POSTS) { + const body = new URLSearchParams({ + name__confirm: '', + from__confirm: validFrom, + }) + const response = await fetch(`${base}${path}`, { + method: 'POST', + body, + headers: { 'content-type': 'application/x-www-form-urlencoded' }, + redirect: 'manual', + }) + expect(response.status, `${path} with from__confirm=${validFrom}`).toBe( + 400, + ) + await response.text() + } + + // Nothing above should have taken the process with it, and a 500 here + // would have been reported to Sentry as an unhandled action error. + await new Promise((resolve) => setTimeout(resolve, 500)) + expect( + server?.exitCode, + `server should still be running${serverOutput ? `\n${serverOutput}` : ''}`, + ).toBeNull() + }) + // 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