Add server-side OTP and TOTP format validations #1251
Conversation
monfresh
force-pushed
the
mb-field-validations
branch
from
6b7c5c9
to
7980ea1
Compare
app/forms/otp_verification_form.rb
Outdated
| @@ -13,6 +13,8 @@ def submit | |||
| attr_reader :code, :user | |||
|
|
|||
| def valid_direct_otp_code? | |||
| code_length = Devise.direct_otp_length | |||
| return false unless code =~ /^\d{#{code_length}}$/ | |||
There was a problem hiding this comment.
For defensive code like this, how about anchoring with \A and \Z as beginning and end of input (rather than line)?
This protects against nefarious inputs with newlines:
"aaaaa\n123456\naaaaaaaaa" =~ /^\d{6}$/
=> 6
"aaaaa\n123456\naaaaaaaaa" =~ /\A\d{6}\Z/
=> nilThere was a problem hiding this comment.
Yes. That's what I meant to do but forgot to push. I thought there was a Rubocop setting that detects that, but it looks like it only looks for it when you use the Rails format validation.
app/forms/totp_verification_form.rb
Outdated
| @@ -13,6 +13,8 @@ def submit | |||
| attr_reader :user, :code | |||
|
|
|||
| def valid_totp_code? | |||
| code_length = Devise.otp_length | |||
| return false unless code =~ /^\d{#{code_length}}$/ | |||
|
Updated and added a test with newlines. PTAL. |
app/forms/otp_verification_form.rb
Outdated
| @@ -13,6 +13,8 @@ def submit | |||
| attr_reader :code, :user | |||
|
|
|||
| def valid_direct_otp_code? | |||
| code_length = Devise.direct_otp_length | |||
| return false unless code =~ /\A\d{#{code_length}}\Z/ | |||
There was a problem hiding this comment.
would it be helpful to name this regex for clarity?
| context 'when the code is not exactly Devise.otp_length digits' do | ||
| it 'returns FormResponse with success: false' do | ||
| user = build_stubbed(:user) | ||
| codes = %W(123abc 1234567 abcdef aaaaa\n123456\naaaaaaaaa) |
There was a problem hiding this comment.
I am not sure what Devise.otp_length is...would it make sense to reference it here explicitly somehow? (my guess is that all of these are the wrong length but I am not sure)
There was a problem hiding this comment.
I see what you mean. I was trying to make this dynamic based on the Devise setting, in case it changes, but the wrong codes themselves are not derived from the setting, so I think in this case it would be clearer to just say 6 digits, and if the setting changes, we'll update the spec. What do you think?
There was a problem hiding this comment.
Yes that makes sense. I was looking for more explicitness here around why these are not valid. If we label them as such, that would be great.
There was a problem hiding this comment.
Ah, got it. Commit coming up.
There was a problem hiding this comment.
How is this? 6cf0d70
**Why**: To minimize our attack surface and only accept allowed inputs, which in this case is a string containing only numeric characters with a length corresponding to the Devise.direct_otp_length and Devise.otp_length settings, respectively.
monfresh
force-pushed
the
mb-field-validations
branch
from
5bbc49e
to
41cdbac
Compare
| user.authenticate_direct_otp(code) | ||
| end | ||
|
|
||
| def pattern_matching_otp_code_format |
Why: To minimize our attack surface and only accept allowed inputs,
which in this case is a string containing only numeric characters with
a length corresponding to the Devise.direct_otp_length and
Devise.otp_length settings, respectively.