New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better handle basic authentication without a password #44610
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -74,6 +74,8 @@ module ClassMethods | |||||
# | ||||||
# See ActionController::HttpAuthentication::Basic for example usage. | ||||||
def http_basic_authenticate_with(name:, password:, realm: nil, **options) | ||||||
raise ArgumentError, "Expected name: to be a String, got #{name.class}" unless name.is_a?(String) | ||||||
raise ArgumentError, "Expected password: to be a String, got #{password.class}" unless name.is_a?(String) | ||||||
before_action(options) { http_basic_authenticate_or_request_with name: name, password: password, realm: realm } | ||||||
end | ||||||
end | ||||||
|
@@ -82,8 +84,8 @@ def http_basic_authenticate_or_request_with(name:, password:, realm: nil, messag | |||||
authenticate_or_request_with_http_basic(realm, message) do |given_name, given_password| | ||||||
# This comparison uses & so that it doesn't short circuit and | ||||||
# uses `secure_compare` so that length information isn't leaked. | ||||||
ActiveSupport::SecurityUtils.secure_compare(given_name, name) & | ||||||
ActiveSupport::SecurityUtils.secure_compare(given_password, password) | ||||||
ActiveSupport::SecurityUtils.secure_compare(given_name.to_s, name) & | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit handwavy, but the idea was to cast to string once and for all at boot, so that if somehow you passed something that acts as a string but isn't, we don't open a timing attack (because presumably But again it wasn't to prevent a specific scenario, just to be extra careful. Anyway I refactored the PR, so this |
||||||
ActiveSupport::SecurityUtils.secure_compare(given_password.to_s, password) | ||||||
end | ||||||
end | ||||||
|
||||||
|
@@ -107,7 +109,7 @@ def authenticate(request, &login_procedure) | |||||
end | ||||||
|
||||||
def has_basic_credentials?(request) | ||||||
request.authorization.present? && (auth_scheme(request).downcase == "basic") && user_name_and_password(request).length == 2 | ||||||
request.authorization.present? && (auth_scheme(request).downcase == "basic") | ||||||
end | ||||||
|
||||||
def user_name_and_password(request) | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is supposed to be
unless password.is_a?(String)
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 yes you are right. I'll fix directly in
main
. Thanks!