two_factor_authentication icon indicating copy to clipboard operation
two_factor_authentication copied to clipboard

500 error on invalid 2fa code, if user has never authenticated successfully before.

Open BenjaminBenetti opened this issue 4 years ago • 0 comments

Issue

If a user has never preformed a successful two factor authentication, and they get the authentication code wrong, they will be met with a 500 error.

NoMethodError: undefined method `+' for nil:NilClass
/opt/XXXXX/XXXXX/vendor/bundle/ruby/2.6.0/gems/two_factor_authentication-2.2.0/app/controllers/devise/two_factor_authentication_controller.rb:60:in `after_two_factor_fail_for'
/opt/XXXXX/XXXXX/vendor/bundle/ruby/2.6.0/gems/two_factor_authentication-2.2.0/app/controllers/devise/two_factor_authentication_controller.rb:16:in `update'
/opt/XXXXX/XXXXX/vendor/bundle/ruby/2.6.0/gems/actionpack-5.2.3/lib/action_controller/metal/basic_implicit_render.rb:6:in `send_action'
/opt/XXXXX/XXXXX/vendor/bundle/ruby/2.6.0/gems/actionpack-5.2.3/lib/abstract_controller/base.rb:194:in `process_action'
/opt/XXXXX/XXXXX/vendor/bundle/ruby/2.6.0/gems/actionpack-5.2.3/lib/action_controller/metal/rendering.rb:30:in `process_action'
/opt/XXXXX/XXXXX/vendor/bundle/ruby/2.6.0/gems/actionpack-5.2.3/lib/abstract_controller/callbacks.rb:42:in `block in process_action'
/opt/XXXXX/XXXXX/vendor/bundle/ruby/2.6.0/gems/activesupport-5.2.3/lib/active_support/callbacks.rb:109:in `block in run_callbacks'
....

Cause

The migration generated in the initial setup instructions has a bug:

rails g migration AddTwoFactorFieldsToUsers second_factor_attempts_count:integer encrypted_otp_secret_key:string:index encrypted_otp_secret_key_iv:string encrypted_otp_secret_key_salt:string direct_otp:string direct_otp_sent_at:datetime totp_timestamp:timestamp

Generates a column add_column :users, :second_factor_attempts_count, :integer. On new or existing accounts this column will be null however in two_factor_authentication_controller.rb:60 this field is incremented by one, causing the NoMethodError: undefined method + for nil:NilClass Error.

Resolution

Handle the null case or update the migration to include default: 0

BenjaminBenetti avatar Jun 16 '20 19:06 BenjaminBenetti