From 889f67d1eac4677676ed01e99c783704fa081e41 Mon Sep 17 00:00:00 2001 From: Arpit Dalal Date: Sun, 27 Sep 2026 15:56:20 -0400 Subject: [PATCH 1/4] fix(honeypot): answer 400 for a malformed honeypot field instead of 500 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `remix-utils` decrypts the `from__confirm` 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 with `InvalidCharacterError`, and a value that decodes to fewer than 12 bytes leaves AES-GCM with an empty ciphertext and `OperationError`. Both are `DOMException`s, so `checkHoneypot` rethrew them and `POST /contact` answered 500 on attacker-controlled input — the only Ongoing issue left in Sentry. Anything we did not issue is spam, so those get the same clean 400 a `SpamError` already gets. The match is on the class *and* `.name`, never `.message`: the text is Node-version dependent, and `instanceof DOMException` is false for both errors because the runtime's realm owns them, so the class is read through the cross-realm-safe `toString` tag. Closes #14 --- app/utils/honeypot.server.ts | 41 +++++++++ tests/honeypot.test.ts | 168 +++++++++++++++++++++++++++++++++++ 2 files changed, 209 insertions(+) create mode 100644 tests/honeypot.test.ts diff --git a/app/utils/honeypot.server.ts b/app/utils/honeypot.server.ts index e0fd321..25eb3b4 100644 --- a/app/utils/honeypot.server.ts +++ b/app/utils/honeypot.server.ts @@ -5,6 +5,44 @@ export const honeypot = new Honeypot({ encryptionSeed: process.env.HONEYPOT_SECRET, }) +/** + * The `DOMException` names `atob` raises on a string outside the base64 + * alphabet and `crypto.subtle.decrypt` raises when the buffer is too small to + * hold an IV plus a GCM tag. + */ +const MALFORMED_FIELD_ERROR_NAMES = new Set([ + 'InvalidCharacterError', + 'OperationError', +]) + +/** + * Whether `error` is the crypto layer choking on a `from__confirm` value we + * never issued — which makes it spam, not 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 down the + * request: `atob` rejects the string outright, or decodes it to fewer than 12 + * bytes, leaving AES-GCM with an empty ciphertext to reject. Either way the + * submission is something we did not issue and deserves the same 400 a + * `SpamError` gets, rather than an unhandled exception that Sentry reports on + * attacker-controlled input. + * + * Matched on the class *and* `.name`, never on `.message`: the message text + * comes from the Node version's bundled undici and changes across releases. + */ +export function isMalformedHoneypotFieldError(error: unknown) { + // `DOMException` is a cross-realm class. The instances `atob` and + // `crypto.subtle` throw belong to the runtime's realm, not to this module's + // global, so `instanceof DOMException` is false for exactly the errors + // being handled here. The `toString` tag is the cross-realm-safe read of + // the class; narrowing further on `.name` keeps a real crypto failure + // (`DataError`, `NotSupportedError`, …) reporting as itself. + 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) @@ -12,6 +50,9 @@ export async function checkHoneypot(formData: FormData) { if (error instanceof SpamError) { throw new Response('Form not submitted properly', { status: 400 }) } + if (isMalformedHoneypotFieldError(error)) { + throw new Response('Form not submitted properly', { status: 400 }) + } throw error } } diff --git a/tests/honeypot.test.ts b/tests/honeypot.test.ts new file mode 100644 index 0000000..5d55f48 --- /dev/null +++ b/tests/honeypot.test.ts @@ -0,0 +1,168 @@ +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 fewer than 12 bytes → AES-GCM gets an empty + * ciphertext 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, +// and every `from__confirm` case below would pass vacuously. An empty string is +// falsy, so it pins production behaviour, and the "future timestamp" case +// further down is what holds the rest of this file honest about it. +vi.stubEnv('TESTING', '') +vi.stubEnv('HONEYPOT_SECRET', 'test-honeypot-secret') + +const { checkHoneypot, honeypot, isMalformedHoneypotFieldError } = + await import('#app/utils/honeypot.server') + +afterAll(() => { + vi.unstubAllEnvs() +}) + +/** A form a human submits: the honeypot is present and left blank. */ +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 value that decodes to fewer bytes than the IV', async () => { + // `atob('a')` is an empty buffer, so `slice(12)` is an empty + // ciphertext. This is the shape a bot gets by echoing back any short + // base64 string, which is why it is the most common of the two. + const response = await captureResponse(checkHoneypot(formDataWith('a'))) + expect(response.status).toBe(400) + }) + + it('rejects five bytes of well-formed base64', async () => { + // Longer than zero, so it clears the IV slice and fails inside AES-GCM + // instead of at `atob` — the second half of `OperationError`. + const response = await captureResponse( + checkHoneypot(formDataWith(btoa('12345'))), + ) + 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 whole 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. With + // `validFromFieldName` off this resolves instead, and fails. + 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('isMalformedHoneypotFieldError', () => { + 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 real `OperationError` raised + // outside the honeypot, or another DOMException kind entirely, 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, + ) + }) +}) From 0ef1506ecb96c23a840f6908ba0a30a5521de57f Mon Sep 17 00:00:00 2001 From: Arpit Dalal Date: Sun, 27 Sep 2026 16:09:15 -0400 Subject: [PATCH 2/4] fix(honeypot): correct the reasoning, close the gaps the first pass left MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follows the review of the first cut, all of it in what the code and tests claim rather than in what they do. The `from__confirm=a` case from the issue does not reach AES-GCM: a base64 length of 1 (mod 4) is undecodable, so `atob` rejects it with the same `InvalidCharacterError` as garbage. The shortest value that gets as far as AES-GCM is two characters. The tests now name the mechanism each input actually hits, and cover the two shapes the first pass missed — a value that decodes to a whole IV but no GCM tag, and a well-formed token whose authentication fails. The `.message` reason was wrong: it is Node's `node:buffer` and `node:internal/crypto/aes`, not undici, and the message is not even stable within one Node version. Node 22's "The provided data is too small" became "The operation failed for an operation-specific reason" in Node 24, and `atob` raises two different texts on Node 26 depending on the input. The cross-realm claim was true only of the test environment. Vitest's jsdom global gives this module jsdom's `DOMException` while `atob` and `crypto.subtle` stay Node's, so `instanceof` misses the two errors being handled — but the server is single-realm and `instanceof` would hold there. The tag check is correct in both, so it stays, with the reason stated accurately. Adds the case the acceptance criteria name but nothing asserted: that `checkHoneypot` rethrows an error the predicate does not name. A blanket `catch` now fails the suite rather than passing it. Closes #14 --- app/utils/honeypot.server.ts | 60 ++++++++++------ tests/honeypot.test.ts | 133 +++++++++++++++++++++++++++++------ 2 files changed, 150 insertions(+), 43 deletions(-) diff --git a/app/utils/honeypot.server.ts b/app/utils/honeypot.server.ts index 25eb3b4..3f81452 100644 --- a/app/utils/honeypot.server.ts +++ b/app/utils/honeypot.server.ts @@ -6,9 +6,9 @@ export const honeypot = new Honeypot({ }) /** - * The `DOMException` names `atob` raises on a string outside the base64 - * alphabet and `crypto.subtle.decrypt` raises when the buffer is too small to - * hold an IV plus a GCM tag. + * 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', @@ -16,27 +16,41 @@ const MALFORMED_FIELD_ERROR_NAMES = new Set([ ]) /** - * Whether `error` is the crypto layer choking on a `from__confirm` value we - * never issued — which makes it spam, not a bug. + * 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 down the - * request: `atob` rejects the string outright, or decodes it to fewer than 12 - * bytes, leaving AES-GCM with an empty ciphertext to reject. Either way the - * submission is something we did not issue and deserves the same 400 a - * `SpamError` gets, rather than an unhandled exception that Sentry reports on - * attacker-controlled input. + * 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 less than an IV plus a GCM tag leaves + * AES-GCM with a ciphertext too short to authenticate. * - * Matched on the class *and* `.name`, never on `.message`: the message text - * comes from the Node version's bundled undici and changes across releases. + * Matched on the class *and* `.name`, never on `.message`. The message is not + * even stable within one Node version — `atob('!!!')` and `atob('a')` raise + * different text on Node 26 — and Node 22's "The provided data is too small" + * became "The operation failed for an operation-specific reason" in Node 24, + * so a message match would break on a runtime bump. The name is spec'd, and + * held across the Node 22/24/26 range in `engines`. + * + * `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. The one thing this cannot tell apart is an `OperationError` + * from a failed authentication tag, which is also what a `from__confirm` we + * *did* issue throws if `HONEYPOT_SECRET` changes under an open form. That + * case is answered 400 rather than reported; keeping `HONEYPOT_SECRET` stable + * across deploys is what stops it from happening, and it is a required env + * var. */ export function isMalformedHoneypotFieldError(error: unknown) { - // `DOMException` is a cross-realm class. The instances `atob` and - // `crypto.subtle` throw belong to the runtime's realm, not to this module's - // global, so `instanceof DOMException` is false for exactly the errors - // being handled here. The `toString` tag is the cross-realm-safe read of - // the class; narrowing further on `.name` keeps a real crypto failure - // (`DataError`, `NotSupportedError`, …) reporting as itself. return ( Object.prototype.toString.call(error) === '[object DOMException]' && MALFORMED_FIELD_ERROR_NAMES.has((error as DOMException).name) @@ -47,10 +61,10 @@ export async function checkHoneypot(formData: FormData) { try { await honeypot.check(formData) } catch (error) { - if (error instanceof SpamError) { - throw new Response('Form not submitted properly', { status: 400 }) - } - if (isMalformedHoneypotFieldError(error)) { + // A `SpamError` is the expected rejection. A malformed field is the same + // answer reached the long way round, because the value is one we never + // issued either. 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 diff --git a/tests/honeypot.test.ts b/tests/honeypot.test.ts index 5d55f48..83789a9 100644 --- a/tests/honeypot.test.ts +++ b/tests/honeypot.test.ts @@ -7,8 +7,8 @@ import { afterAll, describe, expect, it, vi } from 'vitest' * throws out of `check` as a `DOMException` rather than a `SpamError`: * * - not base64 at all → `atob` throws `InvalidCharacterError` - * - base64, but decoding to fewer than 12 bytes → AES-GCM gets an empty - * ciphertext and `crypto.subtle.decrypt` throws `OperationError` + * - 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 @@ -20,10 +20,10 @@ import { afterAll, describe, expect, it, vi } from 'vitest' // 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, -// and every `from__confirm` case below would pass vacuously. An empty string is -// falsy, so it pins production behaviour, and the "future timestamp" case -// further down is what holds the rest of this file honest about it. +// `validFromFieldName` off, which makes `check` return before it ever decrypts. +// Every case below would then fail loudly rather than pass for the wrong +// reason, except the happy path, which would go green without decrypting +// anything — that is the one the "future timestamp" case exists to catch. vi.stubEnv('TESTING', '') vi.stubEnv('HONEYPOT_SECRET', 'test-honeypot-secret') @@ -31,10 +31,17 @@ const { checkHoneypot, honeypot, isMalformedHoneypotFieldError } = await import('#app/utils/honeypot.server') afterAll(() => { + vi.restoreAllMocks() vi.unstubAllEnvs() }) -/** A form a human submits: the honeypot is present and left blank. */ +/** + * 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', '') @@ -61,23 +68,45 @@ describe('checkHoneypot with a malformed from__confirm', () => { 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 () => { - // `atob('a')` is an empty buffer, so `slice(12)` is an empty + // Two characters decode to one byte, so `slice(12)` is an empty // ciphertext. This is the shape a bot gets by echoing back any short // base64 string, which is why it is the most common of the two. - const response = await captureResponse(checkHoneypot(formDataWith('a'))) + const response = await captureResponse(checkHoneypot(formDataWith('aa'))) expect(response.status).toBe(400) }) - it('rejects five bytes of well-formed base64', async () => { - // Longer than zero, so it clears the IV slice and fails inside AES-GCM - // instead of at `atob` — the second half of `OperationError`. + 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 check inside AES-GCM from the empty-ciphertext 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(btoa('12345'))), + 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. @@ -101,10 +130,9 @@ describe('checkHoneypot with a real submission', () => { }) it('rejects a future from__confirm with the same 400', async () => { - // Doubles as the guard against this whole 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. With - // `validFromFieldName` off this resolves instead, and fails. + // 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, }) @@ -124,7 +152,72 @@ describe('checkHoneypot with a real submission', () => { }) }) +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) + } + // The two are different classes in this environment, which is the whole + // reason the predicate reads the tag instead of using `instanceof`. + expect(thrown[0]).not.toBeInstanceOf(DOMException) + }) + it('recognises the two errors the honeypot field provokes', () => { expect( isMalformedHoneypotFieldError( @@ -139,9 +232,9 @@ describe('isMalformedHoneypotFieldError', () => { }) it('does not swallow a crypto error from another source', () => { - // The point of narrowing on `.name`: a real `OperationError` raised - // outside the honeypot, or another DOMException kind entirely, still - // has to reach Sentry rather than be dressed up as spam. + // 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) From 0d46be6a07ce8c1bc5dd536480cffe80d7047f72 Mon Sep 17 00:00:00 2001 From: Arpit Dalal Date: Sun, 27 Sep 2026 16:17:36 -0400 Subject: [PATCH 3/4] docs(honeypot): drop the last claim the comments cannot carry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The inline note at the `if` said a malformed field is spam "because the value is one we never issued either" — fifteen lines below the JSDoc that says that is not always true. It now points at the JSDoc instead. Also corrects two mechanisms the comments still had wrong: a value decoding to fewer than 12 bytes is rejected on the IV, not on a short ciphertext, and the Node 24 wording change could not be verified from here, so the evidence is what was observed — two different `atob` texts on one Node version, and the "too small" message production used to report being absent today. Adds the limit the narrowing cannot escape: the predicate cannot tell where inside `check` an error came from, so any `DOMException` with one of the two names raised elsewhere in that call tree is downgraded too. Closes #14 --- app/utils/honeypot.server.ts | 34 ++++++++++++++++++---------------- tests/honeypot.test.ts | 24 ++++++++++++++---------- 2 files changed, 32 insertions(+), 26 deletions(-) diff --git a/app/utils/honeypot.server.ts b/app/utils/honeypot.server.ts index 3f81452..a73435c 100644 --- a/app/utils/honeypot.server.ts +++ b/app/utils/honeypot.server.ts @@ -22,15 +22,14 @@ const MALFORMED_FIELD_ERROR_NAMES = new Set([ * `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 less than an IV plus a GCM tag leaves - * AES-GCM with a ciphertext too short to authenticate. + * 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`. The message is not - * even stable within one Node version — `atob('!!!')` and `atob('a')` raise - * different text on Node 26 — and Node 22's "The provided data is too small" - * became "The operation failed for an operation-specific reason" in Node 24, - * so a message match would break on a runtime bump. The name is spec'd, and - * held across the Node 22/24/26 range in `engines`. + * 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`. @@ -43,12 +42,14 @@ const MALFORMED_FIELD_ERROR_NAMES = new Set([ * 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. The one thing this cannot tell apart is an `OperationError` - * from a failed authentication tag, which is also what a `from__confirm` we - * *did* issue throws if `HONEYPOT_SECRET` changes under an open form. That - * case is answered 400 rather than reported; keeping `HONEYPOT_SECRET` stable - * across deploys is what stops it from happening, and it is a required env - * var. + * 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 ( @@ -62,8 +63,9 @@ export async function checkHoneypot(formData: FormData) { await honeypot.check(formData) } catch (error) { // A `SpamError` is the expected rejection. A malformed field is the same - // answer reached the long way round, because the value is one we never - // issued either. Everything else is a real fault and stays a 500. + // 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 83789a9..162c0e0 100644 --- a/tests/honeypot.test.ts +++ b/tests/honeypot.test.ts @@ -21,9 +21,10 @@ import { afterAll, describe, expect, it, vi } from 'vitest' // 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 below would then fail loudly rather than pass for the wrong -// reason, except the happy path, which would go green without decrypting -// anything — that is the one the "future timestamp" case exists to catch. +// 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') @@ -77,16 +78,16 @@ describe('checkHoneypot with a malformed from__confirm', () => { }) it('rejects a value that decodes to fewer bytes than the IV', async () => { - // Two characters decode to one byte, so `slice(12)` is an empty - // ciphertext. This is the shape a bot gets by echoing back any short - // base64 string, which is why it is the most common of the two. + // 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 check inside AES-GCM from the empty-ciphertext one + // 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) @@ -213,9 +214,12 @@ describe('isMalformedHoneypotFieldError', () => { ) expect(isMalformedHoneypotFieldError(error)).toBe(true) } - // The two are different classes in this environment, which is the whole - // reason the predicate reads the tag instead of using `instanceof`. - expect(thrown[0]).not.toBeInstanceOf(DOMException) + // 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', () => { From 86ac88cbf486a6cfd9e1acaa2d47462a3b1d0f29 Mon Sep 17 00:00:00 2001 From: Arpit Dalal Date: Sun, 27 Sep 2026 16:33:34 -0400 Subject: [PATCH 4/4] feat(honeypot): make the 400s visible, and prove them over HTTP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 400 is deliberately silent — a `from__confirm` this server issued before HONEYPOT_SECRET changed is answered the same way as junk, and nothing reaches Sentry. `OperationError` covers both "ciphertext too short" and "the GCM tag did not verify", so the two cannot be told apart where the failure happens, and pre-validating the field's length does not separate them: a real token passes a length check and still fails authentication. The boot log is the only place left, so log eight hex characters of SHA-256(HONEYPOT_SECRET) once at startup. Two consecutive deploys printing different fingerprints is the signal, and eight hex characters is useless for recovering a secret. The smoke suite gains the coverage the issue's acceptance criteria are actually phrased in. It boots the production server, so it can assert the 400 the criteria name instead of inferring it from how React Router turns a thrown Response into a status, across three shapes of the field and both public forms. A companion test pins that the fingerprint is still printed. Closes #14 --- app/entry.server.tsx | 2 ++ app/utils/honeypot.server.ts | 33 +++++++++++++++++++++ tests/server.smoke.test.ts | 57 ++++++++++++++++++++++++++++++++++++ 3 files changed, 92 insertions(+) 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 a73435c..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({ @@ -72,3 +73,35 @@ export async function checkHoneypot(formData: FormData) { 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/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