Honeypot not working via GraphQL submissions

tremby commented 1 year ago

Describe the bug

Either honeypot isn't working when submitting via GraphQL or I'm doing something wrong.

My frontend implementation follows yours -- if honeypot is enabled I add a field in a hidden container with name beesknees and it defaults to the blank string.

The GQL inspector tells me that the submission mutation is expecting a field called honeypotCaptcha of type FormieCaptchaInput, and this type has a string name and string value.

So I'm sending a mutation like this:

{
  "operationName": "SubmitForm",
  "variables": {},
  "query": "mutation SubmitForm { save_newsletterSignup_Submission(email: \"test+honeypot7@example.com\", honeypotCaptcha: {name: \"beesknees\", value: \"\"}) {__typename}}"
}

Which is accepted with no errors.

When I look at the submissions, I find that it appears only under spam, and it gives the message Failed Captcha “Honeypot”: “Honeypot param missing: beesknees.".

I also tried with a value present in the honeypot, i.e. ...{name: "beesknees", value: "something"}..., and this ends up in spam with the same message.

Is this code:

https://github.com/verbb/formie/blob/b478d572893c1445e5e0cfa9d6bd2d19c0d36659/src/integrations/captchas/Honeypot.php#L59

possibly not taking into account that the submission may have come from GQL? In a normal form submission it would appear to PHP as just another field alongside the other submitted fields, with name beesknees, but with GQL it's sent separately.

Or is the mutation signature not fully implemented, perhaps; maybe beesknees should appear as one of the mutation's arguments rather than being nested under honeypotCaptcha?

Steps to reproduce

Enable the honeypot plugin
Make a form, and enable honeypot
Try to submit to it using GraphQL
Look at submissions; it will be under spam

Form settings

Multi-page form: n/a
Submission Method: GQL
Client-side Validation: n/a
Custom Form Templates: no

Craft CMS version

4.4.14

Plugin version

2.0.33

Multi-site?

yes

Additional context

No response

engram-design commented 1 year ago

Happy to double check this, but the getRequestParam() function checks for variables.honeypotCaptcha which is empty in your case?

tremby commented 1 year ago

I showed you what my query looks like; that's right, I'm not using variables.

Maybe I could, but it seemed easier at the time to build it with the values escaped right there in the query string. Memory is a bit foggy but I think it was something to do with not knowing the valid types of the fields according to the GQL schema, since this data doesn't come along with the Formie data, and in GQL you need to declare the type of each variable you're going to send.

If I remember correctly, when I realized groups and repeaters all had their own special types, I noped right out. I did not (and still do not) want to write code which uses the GraphQL introspection system to figure out the valid types for each piece of the form.

So instead, I build the query without variables, like this:

      const form = event.target as HTMLFormElement;
      const formData = new FormData(form);
      const args: [string, any][] = [];
      for (const page of pages) { // Note that pages, pages.rows, row.fields etc are my own transformations of the data Formie gives me about a form, to make it easier for me to work with it
        for (const row of page.rows) {
          for (const field of row.fields) {
            const data = getFormInput(field, formData);
            if (data != null) args.push([field.handle, data]);
          }
        }
      }

      // If honeypot is enabled, add the value
      if (config.honeypotEnabled)
        args.push([
          "honeypotCaptcha",
          { name: "beesknees", value: formData.get("beesknees") },
        ]);

      try {
        await client.mutate({
          mutation: gql`
            mutation SubmitForm {
              ${config.submissionMutationName}(
                ${args
                  .map(([key, value]) => `${key}:${serializeForGql(value)}`)
                  .join(",")}
              ) {
                __typename
              }
            }
          `,
        });
        // ...

I won't show serializeForGql but it's a bit like a JSON serializer except it outputs something valid for the GQL query.

Now, the above is sort of overkill for this particular little form, but we have much more complicated forms too. And the whole point of using Formie is so the client can go and build whatever forms they like. My code handles repeaters, groups, and various other field types. I didn't implement every single one, but a lot of them.

It comes out like this -- this is the same as in my original post but unwrapped from the JSON transport and formatted:

mutation SubmitForm {
  save_newsletterSignup_Submission(
    email: "test+honeypot7@example.com"
    honeypotCaptcha: {
      name: "beesknees"
      value: ""
    }
  ) {
    __typename
  }
}

This runs just fine, but fails the honeypot test. The email address makes it into the database.

Now, is it failing because that's badly formed in some way? No, I don't think so. If I misspell it:

# ...
    honeypotCaptchaaaa: {
      name: "beesknees"
      value: ""
    }
# ...

I get an error: Unknown argument "honeypotCaptchaaaa" on field "save_newsletterSignup_Submission" of type "Mutation". Did you mean "honeypotCaptcha"?

Similarly if I misspell one of the nested properties I get an error:

Field "nameNope" is not defined by type FormieCaptchaInput; Did you mean name?

I'd say it's pretty clear from the above that I'm sending the data properly, at least as far as GraphQL is concerned. Nothing in the GQL spec says I need to use variables in my queries.

Even if I do use variables, why would my variable necessarily be called honeypotCaptcha?

Let's use some variables:

mutation SubmitForm($name: String, $value: String) {
  save_newsletterSignup_Submission(
    email: "test+honeypot7@example.com"
    honeypotCaptcha: {
      name: $name
      value: $value
    }
# ...

{"name": "beesknees", "value": ""}

This is also accepted, but again fails the honeypot test.

Now let's try...

mutation SubmitForm($honeypot: FormieCaptchaInput) {
  save_newsletterSignup_Submission(
    email: "test+honeypot7@example.com"
    honeypotCaptcha: $honeypot
  ) {
# ...

{"honeypot": {"name": "beesknees", "value": ""}}

Accepted, but again fails the honeypot test.

Now let's try this, which honestly I thought was going to work, since as far I understand things, it's exactly what you say it's looking for:

mutation SubmitForm($honeypotCaptcha: FormieCaptchaInput) {
  save_newsletterSignup_Submission(
    email: "test+honeypot7@example.com"
    honeypotCaptcha: $honeypotCaptcha
  ) {
    __typename
  }
}

{"honeypotCaptcha": {"name": "beesknees", "value": ""}}

Accepted, but fails the honeypot test.

I really thought that last one was it, given the code you showed me.

So I dug into the code. In my tests I'm sending data using your expected variable name; the last example above.

Your code, minus your comments, plus my comments:

    public function validateSubmission(Submission $submission): bool
    {
        if (
            $this->getRequestParam(self::HONEYPOT_INPUT_NAME)
            // This test runs. `getRequestParam` finds the right param,
            // and correctly returns the empty string, since that was the
            // associated value. So the condition fails, and the body doesn't run.
            // All good so far.
        ) {
            // (This doesn't run)
            $this->spamReason = Craft::t('formie', 'Honeypot input has value: {v}.', ['v' => $this->getRequestParam(self::HONEYPOT_INPUT_NAME)]);

            return false;
        }

        // So we've skipped ahead to here.
        if (
            Craft::$app->getRequest()->getParam(self::HONEYPOT_INPUT_NAME) === null
            // Here's a bug.
            // `$app->getRequest()->getParam("beesknees")` returns null,
            // because it only checked for "traditional" posted params,
            // it didn't check for the GQL version.
            // Here's the source code for that method:
            // https://github.com/craftcms/cms/blob/v3/src/web/Request.php#L1053-L1064
        ) {
            // And so this body runs and the method returns false:
            $this->spamReason = Craft::t('formie', 'Honeypot param missing: {v}.', ['v' => self::HONEYPOT_INPUT_NAME]);

            return false;
        }

        return true;
    }

So you've got a bug there; you check for the GQL-style variable in the first check but not in the second.

But it's also a bug (the one I'm much more keen on getting fixed, as explained at the top) that you're demanding the use of variables, and what's more, variables with specific names.

I don't know if it's thanks to your code or Craft's, or to some dependency, but the other values I'm sending are being understood just fine, variables or no variables.

engram-design commented 1 year ago

Now you raise an excellent point where I'm using Craft::$app->getRequest()->getParam - and I shouldn't. It should always use getRequestParam() which is supposed to handle fetching POST values, or variables in a GQL mutation.

I believe this was the solution I came up with at the time, because I couldn't figure out how to dive into the mutation data supplied within a class such as this example, without it being really gross (parsing the raw GQL string data to find what we need). Happy to give it another look, but Craft's GQL implementation is a little odd from my understanding...

And yes, captchas are the special case here in that I need to grab the values differently than fields as you've mentioned work just as you'd expect in the mutation query. This is because fields are stored on the submission GQL object, and captchas aren't really as such.

Will give it another go, but will likely be a bit more involved!

tremby commented 1 year ago

It occurs to me that I could use a variable just for the honeypot, and continue to do everything else as I already am.

That'd mean much less work for you by the sound of it: just fix the getRequest()->getParam vs getRequestParam issue. Leave the whole "don't make me use variables" thing alone, call it a "known limitation". (I mean, I get the feeling I'm the only person trying to use Formie in this way :laughing:)

And then it'd only be a small amount of work for me. It's the other, unknown, fields where I want to avoid using variables. This honeypot one is known and fixed.

But if you did want to take another dig at the "complete fix", by all means, it'd make for a more bulletproof solution.

engram-design commented 1 year ago

It's certainly something on my list to not have to use variables, as it feels like a bit of a limitation. Purely - it was just to get it working at the time until I could figure it out properly!

I've also fixed that getRequest()->getParam vs getRequestParam in https://github.com/verbb/formie/commit/1745ee51422db6af9af903104187a0194d0dd760 for the next release.

Nothing else should have a requirement of using variables other than captchas.

tremby commented 1 year ago

Hard to word delicately... :sweat:

That doesn't fix it. That function is returning null whether the honeypot param was totally absent or if it was there with the empty string value you want to see. So it's returning null either way, and so the body is running marking it as spam. (There's :grimacing: already a comment on the line right above saying why you weren't already using getRequestParam.)

PR incoming...

engram-design commented 1 year ago

Yeah, you're right 🤦‍♂️

Much prefer your PR solution in any case!

verbb / formie