identity-idp
identity-idp copied to clipboard
LG-12712 Use phone fingerprint as rate limit key for phone confirmation
🎫 Ticket
Link to the relevant ticket: LG-12712
🛠 Summary of changes
Adds a method to PhoneSetupController that gets called before create and checks the submitted phone as an encoded fingerprint against a RateLimit. Specify limit to 20 submissions in a 10 minute window and not on a per-user basis but rather on a per phone basis.
📜 Testing Plan
(in your local application.yml set phone_submissions_per_fingerprint_limit: 5 for easier testing)
- [ ] Log in at http://localhost:3000
- [ ] Attempt to add a phone and when OTP page shows click 'Use another phone number' and repeat 5x with the same number
- [ ] Observe you are sent to your account page with a flash warning ' you tried too many times ... ' and a 10 minute cooldown
- [ ] Log out, log in as another user
- [ ] Attempt to add the same phone number as above and receive the same warning and lockout to account page
Unfortunately, I think this behavior may already be implemented, stemming back to @mitchellhenke's comment in https://github.com/18F/identity-idp/pull/10459#discussion_r1571044675 . It's a little non-obvious, especially since it happens through the TwoFactorAuthenticationController controller, which is typically only responsible for authentication, not MFA setup. But in the case of phone setup, the code confirmation also goes through that controller.
There's an existing feature spec here which appears to verify that this behavior is expected to be enforced:
https://github.com/18F/identity-idp/blob/dc7ea61e55f212cc82838b552be105bd58530ecc/spec/support/shared_examples/phone/rate_limitting.rb#L26-L44
The actual numbers of the rate limit may be different from what we had come up with for this work, but I think we'd be okay with slight differences from what we'd planned.
Can you take a look and confirm if the behavior we were seeking to implement here is already handled via OtpRateLimiter?
Can you take a look and confirm if the behavior we were seeking to implement here is already handled via OtpRateLimiter?
Affirmative. It does in fact apply rate limiting for phone fingerprint regardless of the account used. The difference being as you noted OtpRateLimiter checks during 2FA rather than at phone setup as in this PR. Also a distinction between that and what I've been working on is the user doesn't get signed out and their 2FA locked where in the OtpRateLimiter it does. Which ironically could lead to a scenario cautioned against in this comment and in our slack discussion.