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

Adjust nonce logic #1680

Merged
merged 2 commits into from
Oct 23, 2024
Merged

Adjust nonce logic #1680

merged 2 commits into from
Oct 23, 2024

Conversation

claudiulodro
Copy link
Contributor

All Submissions:

Changes proposed in this Pull Request:

Note: This is a hotfix

This PR adjusts the nonce logic for newsletter signups. Now it will only check the nonce if reCaptcha is inactive. Technically we don't even need a nonce for logged-out forms, but I like having the little extra security to protect from bot signups.

How to test the changes in this Pull Request:

  1. Try the newsletter subscribe block. It should continue working nicely with no visible change.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 20.13%. Comparing base (ab29893) to head (f1aed62).
Report is 2 commits behind head on release.

Additional details and impacted files
@@            Coverage Diff             @@
##             release    #1680   +/-   ##
==========================================
  Coverage      20.13%   20.13%           
  Complexity      2410     2410           
==========================================
  Files             46       46           
  Lines           9890     9890           
==========================================
  Hits            1991     1991           
  Misses          7899     7899           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@leogermani
Copy link
Contributor

Technically we don't even need a nonce for logged-out forms, but I like having the little extra security to protect from bot signups.

But doesn't that mean that the cached nonce will still be an issue for sites not using recaptcha?

Also, I don't see any added protection there, since the nonce is part of the form as a hidden field anyways

@claudiulodro
Copy link
Contributor Author

It's at least as much protection as the honeypot field! :) Good points though, I'll just remove it

@claudiulodro
Copy link
Contributor Author

Adjusted in f1aed62

Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for revisiting it!

Tip for your future self, in cases like this you can use phpcs:disable Rule.... and then phpcs:enable at the end to avoid all these repetitions

@claudiulodro claudiulodro merged commit 5da024d into release Oct 23, 2024
9 checks passed
@claudiulodro claudiulodro deleted the fix/nonce-hotfix branch October 23, 2024 19:17
@matticbot
Copy link
Contributor

🎉 This PR is included in version 3.2.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants