Skip to content

Commit

Permalink
Attempt to avoid database errors when invalid tokens are submitted
Browse files Browse the repository at this point in the history
The vast majority of Rodauth users are using integer values for
database keys, and this change checks that the key values
embedded in tokens are valid integers, avoiding exceptions raised
by the database if they are not.

For Rodauth users not using integer values for database keys,
the convert_token_id configuration method can be used to perform
the appropriate checks or conversions.
  • Loading branch information
jeremyevans committed Dec 23, 2022
1 parent e52d6ac commit ab82cc7
Show file tree
Hide file tree
Showing 7 changed files with 60 additions and 3 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
=== master

* Attempt to avoid database errors when invalid tokens are submitted (jeremyevans)

* Allow button template to be overridden just as other templates can be (jeremyevans) (#280)

=== 2.26.1 (2022-11-08)
Expand Down
2 changes: 2 additions & 0 deletions doc/base.rdoc
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ cache_templates :: Whether to cache templates. True by default. It may be worth
check_csrf? :: Whether Rodauth should use Roda's +check_csrf!+ method for checking CSRF tokens before dispatching to Rodauth routes, true by default.
check_csrf_opts :: Options to pass to Roda's +check_csrf!+ if Rodauth calls it before dispatching.
check_csrf_block :: Proc for block to pass to Roda's +check_csrf!+ if Rodauth calls it before dispatching.
convert_token_id_to_integer? :: Whether token ids should be converted to a valid 64-bit integer value. If not set, defaults to true if +account_id_column+ uses an integer type, and false otherwise.
default_field_attributes :: The default attributes to use for input field tags, if field_attributes returns nil for the field.
default_redirect :: Where to redirect after most successful actions.
field_attributes(field) :: The attributes to use for the input field tags for the given field (parameter name).
Expand Down Expand Up @@ -96,6 +97,7 @@ before_login_attempt :: Run arbitrary code after an account has been located, bu
before_rodauth :: Run arbitrary code before handling any rodauth route, but after CSRF checks if Rodauth is doing CSRF checks.
check_csrf :: Checks CSRF token using Roda's +check_csrf!+ method.
clear_session :: Clears the current session.
convert_token_id(id) :: Convert the token id string to an appropriate object to use for the token id (or return +nil+ to signal an invalid token id). By default, converts to a 64-bit signed integer if +convert_token_id_to_integer?+ is true.
csrf_tag(path=request.path) :: The HTML fragment containing the CSRF tag to use, if any.
function_name(name) :: The name of the database function to call. It's passed either :rodauth_get_salt or :rodauth_valid_password_hash.
logged_in? :: Whether the current session is logged in.
Expand Down
26 changes: 26 additions & 0 deletions lib/rodauth/features/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ module Rodauth
auth_value_method :check_csrf_block, nil
auth_value_method :check_csrf_opts, {}.freeze
auth_value_method :default_redirect, '/'
auth_value_method :convert_token_id_to_integer?, nil
flash_key :flash_error_key, :error
flash_key :flash_notice_key, :notice
auth_value_method :hmac_secret, nil
Expand Down Expand Up @@ -115,6 +116,7 @@ module Rodauth
auth_private_methods(
:account_from_login,
:account_from_session,
:convert_token_id,
:field_attributes,
:field_error_attributes,
:formatted_field_error,
Expand Down Expand Up @@ -401,6 +403,11 @@ def only_json?
def post_configure
require 'bcrypt' if require_bcrypt?
db.extension :date_arithmetic if use_date_arithmetic?

if convert_token_id_to_integer?.nil? && db.schema(accounts_table).find{|col, v| break v[:type] == :integer if col == account_id_column}
self.class.send(:define_method, :convert_token_id_to_integer?){true}
end

route_hash= {}
self.class.routes.each do |meth|
route_meth = "#{meth.to_s.sub(/\Ahandle_/, '')}_route"
Expand Down Expand Up @@ -520,6 +527,25 @@ def split_token(token)
token.split(token_separator, 2)
end

def convert_token_id(id)
if convert_token_id_to_integer?
convert_token_id_to_integer(id)
else
id
end
end

def convert_token_id_to_integer(id)
if id = (Integer(id, 10) rescue nil)
if id > 9223372036854775807 || id < -9223372036854775808
# Only allow 64-bit signed integer range to avoid problems on PostgreSQL
id = nil
end
end

id
end

def redirect(path)
request.redirect(path)
end
Expand Down
1 change: 1 addition & 0 deletions lib/rodauth/features/email_base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ def convert_email_token_key(key)

def account_from_key(token, status_id=nil)
id, key = split_token(token)
id = convert_token_id(id)
return unless id && key

return unless actual = yield(id)
Expand Down
4 changes: 3 additions & 1 deletion lib/rodauth/features/jwt_refresh.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ def _account_from_refresh_token(token)
id, token_id, key = _account_refresh_token_split(token)

unless key &&
(id == session_value.to_s) &&
(id.to_s == session_value.to_s) &&
(actual = get_active_refresh_token(id, token_id)) &&
timing_safe_eql?(key, convert_token_key(actual)) &&
jwt_refresh_token_match?(key)
Expand All @@ -126,9 +126,11 @@ def _account_from_refresh_token(token)

def _account_refresh_token_split(token)
id, token = split_token(token)
id = convert_token_id(id)
return unless id && token

token_id, key = split_token(token)
token_id = convert_token_id(token_id)
return unless token_id && key

[id, token_id, key]
Expand Down
16 changes: 14 additions & 2 deletions spec/jwt_refresh_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,20 @@
res = json_request("/jwt-refresh", :refresh_token=>refresh_token.gsub('_', '-'))
res.must_equal [400, {"error"=>"invalid JWT refresh token"}]
token_parts = refresh_token.split('_', 2)
res = json_request("/jwt-refresh", :refresh_token=>"#{token_parts[0]}_#{token_parts[1].gsub('_', '-')}")
res.must_equal [400, {"error"=>"invalid JWT refresh token"}]
if DB[:accounts].get(:id).is_a?(Integer)
res = json_request("/jwt-refresh", :refresh_token=>"#{token_parts[0]}_9223372036854775807#{token_parts[1]}")
res.must_equal [400, {"error"=>"invalid JWT refresh token"}]
res = json_request("/jwt-refresh", :refresh_token=>"9223372036854775807#{token_parts[0]}_#{token_parts[1]}")
res.must_equal [400, {"error"=>"invalid JWT refresh token"}]
res = json_request("/jwt-refresh", :refresh_token=>"#{token_parts[0]}_-9223372036854775807#{token_parts[1]}")
res.must_equal [400, {"error"=>"invalid JWT refresh token"}]
res = json_request("/jwt-refresh", :refresh_token=>"-9223372036854775807#{token_parts[0]}_#{token_parts[1]}")
res.must_equal [400, {"error"=>"invalid JWT refresh token"}]
res = json_request("/jwt-refresh", :refresh_token=>"v#{token_parts[0]}_#{token_parts[1]}")
res.must_equal [400, {"error"=>"invalid JWT refresh token"}]
res = json_request("/jwt-refresh", :refresh_token=>"#{token_parts[0]}_v#{token_parts[1]}")
res.must_equal [400, {"error"=>"invalid JWT refresh token"}]
end

# Third refresh token is valid
res = json_request("/jwt-refresh", :refresh_token=>third_refresh_token)
Expand Down
12 changes: 12 additions & 0 deletions spec/lockout_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,17 @@
visit link[0...-1]
page.find('#error_flash').text.must_equal "There was an error unlocking your account: invalid or expired unlock account key"

if DB[:accounts].get(:id).is_a?(Integer)
visit link.sub('key=', 'key=18446744073709551616')
page.find('#error_flash').text.must_equal "There was an error unlocking your account: invalid or expired unlock account key"

visit link.sub('key=', 'key=-18446744073709551616')
page.find('#error_flash').text.must_equal "There was an error unlocking your account: invalid or expired unlock account key"

visit link.sub('key=', 'key=v')
page.find('#error_flash').text.must_equal "There was an error unlocking your account: invalid or expired unlock account key"
end

visit link
click_button 'Unlock Account'
page.find('#notice_flash').text.must_equal 'Your account has been unlocked'
Expand Down Expand Up @@ -114,6 +125,7 @@
rodauth do
enable :lockout
max_invalid_logins 2
convert_token_id_to_integer? false
end
roda do |r|
r.rodauth
Expand Down

0 comments on commit ab82cc7

Please sign in to comment.