Skip to content
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

Check basic auth credentials before authenticate #43209

Merged
merged 1 commit into from Sep 20, 2021

Conversation

mpestov
Copy link

@mpestov mpestov commented Sep 12, 2021

Summary

If you send a request to a controller protected by basic authentication with wrong credentials (without the colon) you will get the error: NoMethodError: undefined method 'bytesize' for nil:NilClass.

For example:

class UsersController < ApplicationController
   http_basic_authenticate_with name: "king", password: "secret"

   def index
     render plain: "Something"
   end
end
credentials=$(echo -n king secret | base64)
curl 'http://localhost:3000/users' -H "Authorization: Basic $credentials"

Stacktrace:

NoMethodError: undefined method `bytesize' for nil:NilClass
  from activesupport (6.1.4.1) lib/active_support/security_utils.rb:34:in `secure_compare'
  from actionpack (6.1.4.1) lib/action_controller/metal/http_authentication.rb:82:in `block in http_basic_authenticate_or_request_with'
  from actionpack (6.1.4.1) lib/action_controller/metal/http_authentication.rb:101:in `authenticate'
  from actionpack (6.1.4.1) lib/action_controller/metal/http_authentication.rb:91:in `authenticate_with_http_basic'
  from actionpack (6.1.4.1) lib/action_controller/metal/http_authentication.rb:87:in `authenticate_or_request_with_http_basic'
  from actionpack (6.1.4.1) lib/action_controller/metal/http_authentication.rb:78:in `http_basic_authenticate_or_request_with'
  from actionpack (6.1.4.1) lib/action_controller/metal/http_authentication.rb:73:in `block in http_basic_authenticate_with'

@rails-bot rails-bot bot added the actionpack label Sep 12, 2021
Comment on lines 173 to 177
test "authentication request with credentials without colon" do
@request.env["HTTP_AUTHORIZATION"] = "Basic #{::Base64.encode64("David Goliath")}"
get :search
assert_response :unauthorized
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the credentials "David Goliath" are invalid as well, maybe it's better to move the test to a specific has_basic_credentials? test like we do for encode_credentials:

def test_encode_credentials_has_no_newline
username = "laskjdfhalksdjfhalkjdsfhalksdjfhklsdjhalksdjfhalksdjfhlakdsjfh"
password = "kjfhueyt9485osdfasdkljfh4lkjhakldjfhalkdsjf"
result = ActionController::HttpAuthentication::Basic.encode_credentials(
username, password)
assert_no_match(/\n/, result)
end

Something like:

Suggested change
test "authentication request with credentials without colon" do
@request.env["HTTP_AUTHORIZATION"] = "Basic #{::Base64.encode64("David Goliath")}"
get :search
assert_response :unauthorized
end
test "authentication request with credentials without colon" do
@request.env["HTTP_AUTHORIZATION"] = "Basic #{::Base64.encode64("David Goliath")}"
refute ActionController::HttpAuthentication::Basic.has_basic_credentials(@request)
end

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I changed the test to specific for has_basic_credentials?.

Copy link
Member

@p8 p8 Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 😄 I have one last change request.
Could you move it under the test_encode_credentials_has_no_newline test?
This will group the tests that don't do get requests together.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course! 🙂 I moved it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 😄

@p8 p8 added the ready PRs ready to merge label Sep 15, 2021
@rafaelfranca rafaelfranca merged commit 20db084 into rails:main Sep 20, 2021
rafaelfranca added a commit that referenced this pull request Sep 20, 2021
Check basic auth credentials before authenticate
@casperisfine
Copy link
Contributor

I don't think this patch is quite correct. it's perfectly legitimate to connect with a username but no password.

casperisfine pushed a commit to Shopify/rails that referenced this pull request Mar 4, 2022
rails#43209 immediately rejects
the request if no password is passed, but there are legitimate
uses for accepting authentication without a password.
byroot added a commit that referenced this pull request Mar 4, 2022
#43209 immediately rejects
the request if no password is passed, but there are legitimate
uses for accepting authentication without a password.
rafaelfranca added a commit that referenced this pull request Mar 8, 2022
@mpestov
Copy link
Author

mpestov commented Apr 12, 2022

@casperisfine Yeah! You are right, I did not support using the access token as username and the password it is not mandatory in this case.
Unfortunately, It is not obvious a use case.
Thank you that discovered it and fixed.

@adammiribyan
Copy link

This would through an error in development or test environments with something like:

http_basic_authenticate_with name: Rails.application.credentials.dig(:testing, :username), \
    password: Rails.application.credentials.dig(:testing, :password), if: -> { Rails.env.production? }

Since the is_a?(String) checks for name and password are made before the before_action conditional.

Can be resolved by defining empty username and password credentials for non-production environments, but still rather unexpected to have a callback throw an error before or without the conditional being satisfied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actionpack ready PRs ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants