LG 15241 Bug fix for recaptcha failure not incrementing rate limit for sign in #11703
Conversation
| @@ -371,7 +371,7 @@ | |||
| rate_limited: false, | |||
| valid_captcha_result: false, | |||
| captcha_validation_performed: true, | |||
| bad_password_count: 0, | |||
| bad_password_count: 1, | |||
There was a problem hiding this comment.
Apparently this piggy-backs on the bad password rate limiter. I wonder if it should have its own.
Probably out of scope for this bug fix.
There was a problem hiding this comment.
Yeah that's a good thought. Another option would be to rename this and the associated session value to be less specific to incorrect password and more generalized to unsuccessful authentication.
There was a problem hiding this comment.
Do you recommend including that change with this bug fix? Relabeling seems pretty minor and an reasonable fix.
There was a problem hiding this comment.
I don't feel too strongly one way or the other. My initial instinct was that it'd be tricky because it involves renaming a session value and we'd have to worry about migrating existing session values when it goes live, but on second thought it could be okay to not migrate session values if the only consequence is that we'd reset the counters of everyone's session-based rate-limiting.
| @@ -3,33 +3,33 @@ | |||
| RSpec.feature 'Visitor signs in with bad passwords and gets locked out' do | |||
| include ActionView::Helpers::DateHelper | |||
| let(:user) { create(:user, :fully_registered) } | |||
| let(:bad_password) { 'badpassword' } | |||
| let(:window) { IdentityConfig.store.max_bad_passwords_window_in_seconds.seconds } | |||
| let(:sign_in_failure) { 'badpassword' } | |||
There was a problem hiding this comment.
I think the previous name was a bit more accurate to its purpose.
| let(:sign_in_failure) { 'badpassword' } | |
| let(:bad_password) { 'badpassword' } |
config/application.yml.default
Outdated
| max_bad_passwords: 5 | ||
| max_bad_passwords_window_in_seconds: 60 | ||
| max_sign_in_failures: 5 | ||
| max_sign_in_failures_window_in_seconds: 60 |
There was a problem hiding this comment.
We'll want to check that we're not configuring this in any deployed environments that'll need to be updated to use the new name, but I think we're using the defaults.
There was a problem hiding this comment.
I checked and it's currently set in int and dev but not in staging or prod. I'll make sure to update those when this PR is merged.
There was a problem hiding this comment.
It looks like they're set to the defaults in those environments, so we should probably just remove that configuration override altogether.
There was a problem hiding this comment.
I removed those configs
app-s3-secret: Here's a preview of your changes:
90d89
< max_bad_passwords: 5
app-s3-secret: Here's a preview of your changes:
110d109
< max_bad_passwords: '5'
There was a problem hiding this comment.
Can we add a test case that confirms that someone who racks up enough reCAPTCHA failures will hit the session limiter?
There was a problem hiding this comment.
Sounds good! added with cf6c6bd
kevinsmaster5
force-pushed
the
kmas-lg-15241-recaptcha-failure-does-not-increment-rate-limit
branch
from
17c94c1 to
cf6c6bd
Compare
| user = create(:user, :fully_registered) | ||
| freeze_time do | ||
| current_time = Time.zone.now | ||
| time_in_hours = distance_of_time_in_words( |
There was a problem hiding this comment.
Minor: The default lockout period is 1 minute, so "in hours" doesn't seem to apply here. Technically it is configurable and could be hours, but maybe we're better off leaving the unit out altogether.
| time_in_hours = distance_of_time_in_words( | |
| rate_limit_time_left = distance_of_time_in_words( |
There was a problem hiding this comment.
changed that to suggested
fee0505
| @@ -385,6 +385,37 @@ | |||
| expect(response).to redirect_to sign_in_security_check_failed_url | |||
| end | |||
| end | |||
|
|
|||
| context 'recpatcha lock out' do | |||
There was a problem hiding this comment.
Minor: Typo
| context 'recpatcha lock out' do | |
| context 'recaptcha lock out' do |
| context 'recpatcha lock out' do | ||
| let(:locked_at) { Time.zone.now } | ||
| let(:sign_in_failure_window) { IdentityConfig.store.max_sign_in_failures_window_in_seconds } | ||
| it 'prevents attempt and logs after exceeding maximum rate limit' do |
There was a problem hiding this comment.
Where are we checking for the "and logs" referenced in this test case?
There was a problem hiding this comment.
copy-pasta mistake. I have removed "and logs"
|
DNM - pending team standup discussion 1/13 |
🎫 Ticket
Link to the relevant ticket:
LG-15241
🛠 Summary of changes
Swapped the order of rate_limit_password and return process_captcha_failed as was recommended in the bug ticket.
📜 Testing Plan
For testing in local development dial down the
sign_in_user_id_per_ip_max_attemptsconfig in application.yml to 5You should receive the lock out message.
👀 Screenshots