Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Captcha in forms #204

Merged
merged 6 commits into from
Mar 18, 2022
Merged

Captcha in forms #204

merged 6 commits into from
Mar 18, 2022

Conversation

pettinarip
Copy link
Member

@pettinarip pettinarip commented Mar 11, 2022

Description

Adds hCaptcha to forms.

TODO

  • decide which captcha service to use (using hCaptcha for testing)
  • add it to all the forms we have
  • test and fix possible cors issues
  • support mobile
  • validate token in serverside

#198

@pettinarip pettinarip marked this pull request as ready for review March 14, 2022 16:11
@pettinarip pettinarip marked this pull request as draft March 14, 2022 17:04
@pettinarip pettinarip marked this pull request as ready for review March 15, 2022 14:08
additional.d.ts Show resolved Hide resolved
src/components/forms/AcademicGrantsForm.tsx Show resolved Hide resolved
@nhsz
Copy link
Member

nhsz commented Mar 17, 2022

@pettinarip LGTM, great work! 🎉 Haven't approved yet because maybe the middleware logic can be abstracted using Next's middleware (and maybe it can even help to avoid extending NextApiRequest types declaration). In case you already check that I'll approve.

@pettinarip
Copy link
Member Author

@nhsz yes, I think I didn't use it because I thought it was meant to be used only on server side requests but not on API routes since you have a NextRequest here and not a NextApiRequest.

Also there are a couple of official docs talking about how to use middlewares on the api routes:

I guess those docs are a bit old and we could refactor it using the new _middleware files, I like that more. Will try to see to do the refactor.

Only concern right now is that the _middleware file is applied to all the files in the folder....it doesn't let you choose which middleware you want to apply to each route within the folder. E.g. I want to only apply the multipartyParse middleware on one specific route, not to all of them. I see that use case a bit dirty. I should add conditions like if (path === '/specific-form').

But I guess that overall, I like that approach more than the current solution. Let me work on that and I'll let you know.

@nhsz
Copy link
Member

nhsz commented Mar 17, 2022

@nhsz yes, I think I didn't use it because I thought it was meant to be used only on server side requests but not on API routes since you have a NextRequest here and not a NextApiRequest.

Also there are a couple of official docs talking about how to use middlewares on the api routes:

I guess those docs are a bit old and we could refactor it using the new _middleware files, I like that more. Will try to see to do the refactor.

Only concern right now is that the _middleware file is applied to all the files in the folder....it doesn't let you choose which middleware you want to apply to each route within the folder. E.g. I want to only apply the multipartyParse middleware on one specific route, not to all of them. I see that use case a bit dirty. I should add conditions like if (path === '/specific-form').

But I guess that overall, I like that approach more than the current solution. Let me work on that and I'll let you know.

Agree. You can apply the middleware to certain routes only https://nextjs.org/docs/middleware#execution-order, so maybe nesting the multipartyParse middleware inside that dir should execute it for that route only 🤔

Regarding extending NextApiRequest, it is recommended to use functions instead if required, for type-safety.

@pettinarip
Copy link
Member Author

maybe nesting the multipartyParse middleware inside that dir should execute it for that route only thinking

You mean having something like this:

- /pages
    _middleware.ts # check captcha tokens
    devcon-grants.tsx
    - /project-grants
      _middleware.ts # apply multiparty parsing
      index.tsx

The problem with that is that I need to apply the parsing before the captcha check in this case.

I think I will go with just one _middleware.ts at the root level and add a condition to apply the multiparty parsing to a specific route.

@pettinarip
Copy link
Member Author

I'm having some troubles querying req.body on a NextRequest. It seems that is it not being sent to this type of request.
When I do await req.json() I receive an error TypeError: invalid json body reason: Unexpected end of JSON input due to req.body being empty 🤷🏼

I've upgraded Next to 12.1.0 which has this PR vercel/next.js#34294 but it is not working for me.

Then formidable is using the fs module which is not allowed to be used in Edge functions https://nextjs.org/docs/api-reference/edge-runtime#unsupported-apis

Given these conditions I would say to skip this refactor for now.

@nhsz nhsz self-requested a review March 18, 2022 15:18
@pettinarip pettinarip merged commit 6f130d3 into master Mar 18, 2022
@pettinarip pettinarip deleted the captcha-button branch March 18, 2022 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants