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

Improve SimpleEngine #35

Merged
merged 4 commits into from
Aug 23, 2023
Merged

Conversation

zachee54
Copy link
Contributor

SimpleEngine

Improve SimpleEngine to make it harder to guess with random values.

Customizable deadlock duration

Allow custom deadlock duration in case maxPerUser is exceeded.
However, this is an "at most" threshold, since the client IP or the session ID may change in the meanwhile.

Resigned : display smart error message as the captcha image.

No idea if this feature would really be useful, but I scratched my head to display simple text as an image. I wrote (smelling) code which doesn't even pass coding standards.
I've given up because I don't know how to do and have already wasted too much time for this detail. You can have a look to zachee54@f3d1cdd if you're interested in.

@codecov-commenter
Copy link

codecov-commenter commented Aug 23, 2023

Codecov Report

Merging #35 (104a4f7) into master (a7c6f9c) will decrease coverage by 0.65%.
The diff coverage is 87.50%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@             Coverage Diff              @@
##             master      #35      +/-   ##
============================================
- Coverage     84.69%   84.05%   -0.65%     
- Complexity      109      110       +1     
============================================
  Files            12       12              
  Lines           392      395       +3     
============================================
  Hits            332      332              
- Misses           60       63       +3     
Files Changed Coverage Δ
src/Engine/Math/SimpleMath.php 88.23% <83.33%> (-8.64%) ⬇️
src/Model/Table/CaptchasTable.php 94.20% <100.00%> (+0.08%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dereuromark
Copy link
Owner

dereuromark commented Aug 23, 2023

Maybe we could display validation message "try again later" for the case of having too many "open" captchas for this user?
If we do validation rule with count we should be able to know that this one doesnt pass, especially if the captcha doesnt exist we are trying to solve.
This way we get more clear feedback to the user in what scenario he is and why a correct answer results in the form not validating.

@zachee54
Copy link
Contributor Author

Yes, but this would require to

  1. recall CaptchasTable::getCount(), which implies a duplicate call to the DB
  2. recall MaxRule outside the ORM.

The design would smell, wouldn't it ?

@dereuromark
Copy link
Owner

I wonder if we need maxRule and could just move all of it to validation rule

@dereuromark
Copy link
Owner

What's the first count? On Display? Maybe we can make validation in a way that we only need get count once

@dereuromark dereuromark merged commit 70141c7 into dereuromark:master Aug 23, 2023
8 of 10 checks passed
@zachee54 zachee54 deleted the improve-simpleengine branch August 23, 2023 21:05
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.

None yet

3 participants