Better handle basic authentication without a password #44610
Conversation
|
Makes sense to me! |
casperisfine
force-pushed
the
http-basic-no-password
branch
from
6562e5e
to
6d2d900
Compare
| 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.
Is there a reason name.to_s isn't called here instead?
Just wondering :)
| ActiveSupport::SecurityUtils.secure_compare(given_name.to_s, name) & | |
| ActiveSupport::SecurityUtils.secure_compare(given_name.to_s, name.to_s) & |
There was a problem hiding this comment.
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 to_s would take longer, the longer the password is).
But again it wasn't to prevent a specific scenario, just to be extra careful. Anyway I refactored the PR, so this to_s is no more.
| @@ -79,11 +79,13 @@ def http_basic_authenticate_with(name:, password:, realm: nil, **options) | |||
| end | |||
|
|
|||
| def http_basic_authenticate_or_request_with(name:, password:, realm: nil, message: nil) | |||
| name = name.to_s | |||
| password = password.to_s | |||
There was a problem hiding this comment.
How about nil -> raise because that might mean you've failed to pull a value out of ENV etc, "" -> acceptable slightly-more-explicit statement that you intend to allow a blank password?
There was a problem hiding this comment.
Yeah I agrees this isn't ideal, I actually don't want to allow nil when http_basic_authenticate_or_request_with is used, only authenticate_with_http_basic do.
|
@casperisfine I think it is not legitimate to use authentication without a password.
|
|
@mpestov that doesn't mean a missing password can't be valid. I've used |
rails#43209 immediately rejects the request if no password is passed, but there are legitimate uses for accepting authentication without a password.
casperisfine
force-pushed
the
http-basic-no-password
branch
from
6d2d900
to
9ebfb14
Compare
| @@ -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) | |||
There was a problem hiding this comment.
I assume this is supposed to be unless password.is_a?(String)?
There was a problem hiding this comment.
🤦 yes you are right. I'll fix directly in main. Thanks!
|
@casperisfine In my opinion your case is the Http Token authentication not Basic. authenticate_with_http_token do |*parts|
token = parts.select(&:present?).join('--')
ApiClient.authenticate(token)
endFor example Rack uses the same validation: https://github.com/rack/rack/blob/main/lib/rack/auth/basic.rb#L47 |
|
@mpestov I can assure you, there is A LOT of legitimate use. |
|
@lonesomewalker Yeah! You are right. |
#43209 immediately rejects the request if no password is passed, but there are legitimate uses for accepting authentication without a password.
cc @mpestov, @rafaelfranca, @p8