honeypot.check() should throw on missing input

[clovis1122]

Currently when calling await honeypot.check(formData);, it will check if the fields exist, and bail if they don't. This is done by the shouldCheckHoneypot call here:

remix-utils/src/server/honeypot.ts

Lines 87 to 91 in 1325b68

	if (!this.shouldCheckHoneypot(formData, nameFieldName)) return;
	if (!formData.has(nameFieldName)) {
	throw new SpamError("Missing honeypot input");
	}

I think a better default is to throw an error when the fields are missing:

• The current behavior makes it easy to forget to add the fields, and the check would be marked as successful.
• I can't think of a use case where you'd want to call honeypot.check(formData); and have it work when the honeypot fields are missing.

We could add a second parameter {strict: false} to control this behavior, and flip the default in the next major version. The reason for strict is in case there are legitimate use cases where this and other lax behaviors are desired (I don't think there are any?). If there were, I can see other things where we're currently lax which should be more strict, like checking if one or the two fields are set.

[sergiodxa]

The shouldCheckHoneypot method is marked as protected (among most other methods) so you can extend the Honeypot class and overwrite it to customize the parts you want. The method as the name implies checks if it should check the honeypot, the idea is that not every form will need to do it, e.g. forms behind a login will most likely not need it.

[clovis1122]

The method as the name implies checks if it should check the honeypot, the idea is that not every form will need to do it, e.g. forms behind a login will most likely not need it.

Can you clarify? I believe that, currently you need to explicitly add the Honeyinput fields to the form, and then call check on the server.

• If you added the fields to the form, most likely you intend to validate them on the server as well. Is there any use case where this wouldn't be the case?
• The other side is also true (if you're explicitly adding honeyinput.check to a remix action, it is likely because you expect the fields to be sent as form data).

In terms of default behavior, explicitly throwing seems like a better idea because it makes these mistakes a little easier to find.

[sergiodxa]

You can import the Honeypot class used server-side, extend it to override the method and use that to instantiate it.

import { Honeypot } from "remix-utils/honeypot/server";

class CustomHoneypot extends Honeypot {
  override shouldCheckHoneypot(
    formData: FormData,
    nameFieldName: string,
  ): boolean {
    if (super.shouldCheckHoneypot(formData, nameFieldName)) return true;
    throw new Error("Missing honeypot fields");
  }
}

export const honeypot = new CustomHoneypot({
  randomizeNameFieldName: false,
  nameFieldName: "name__confirm",
  validFromFieldName: "from__confirm", // null to disable it
  encryptionSeed: undefined, // Ideally it should be unique even between processes
});

Then in your loaders you import that honeypot instance that will use your custom method.

[clovis1122]

I understand that 😅. This is about changing the current behavior.