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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed passing of delete method on button_to tag, creating wrong form csrf token #23752

Merged
merged 2 commits into from Feb 22, 2016

Conversation

Projects
None yet
4 participants
@vipulnsward
Member

vipulnsward commented Feb 18, 2016

@sikachu I took a stab at this, while looking around. 馃榿

r? @rafaelfranca

Fixes #23524

@kaspth

View changes

actionpack/test/controller/request_forgery_protection_test.rb Outdated
# This is required because PATH_INFO isn't reset between requests.
@request.env['PATH_INFO'] = '/per_form_tokens/post_one'
assert_nothing_raised do
delete :post_one, params: {custom_authenticity_token: form_token}

This comment has been minimized.

@kaspth

kaspth Feb 18, 2016

Member

Please add spaces between the braces everywhere: params: { custom_authenticity_token: form_token }.

This comment has been minimized.

@vipulnsward

vipulnsward Feb 18, 2016

Member

Ha! I need to fix my editor for this.

This comment has been minimized.

@vipulnsward

vipulnsward Feb 18, 2016

Member

Actually, I just copied this from above. This is used at many places this way.

This comment has been minimized.

@kaspth

kaspth Feb 18, 2016

Member

Really? Then let's go with the file's precedent. Ugh.

This comment has been minimized.

@vipulnsward

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 18, 2016

Member

Even if the above method is using wrong style new code should use only the right style.

@kaspth

View changes

actionview/lib/action_view/helpers/url_helper.rb Outdated
token_tag(nil, form_options: form_options)
token_tag_method = method == 'delete' ? 'delete' : form_method
request_token_tag = if (form_method == 'post' || method == 'delete')
token_tag(nil, form_options: form_options.merge(method: token_tag_method))

This comment has been minimized.

@kaspth

kaspth Feb 18, 2016

Member

We should just assign the method to form_options on line 310.

This comment has been minimized.

@kaspth

kaspth Feb 18, 2016

Member

Could even change 307 to:

form_method  = method == 'get' || method == 'delete' ? method : 'post'

This comment has been minimized.

@vipulnsward

vipulnsward Feb 18, 2016

Member

No, we still need that to be post

This comment has been minimized.

@kaspth

kaspth Feb 18, 2016

Member

Yeah, just realized. Let's move token_tag_method into the if though.

This comment has been minimized.

@kaspth

kaspth Feb 18, 2016

Member

Actually, those 3 lines are heavily condensed for no reason. Prefer some incidental duplication with:

request_token_tag = \
  if form_method == 'post'
    token_tag(nil, form_options: form_options)
  elsif method == 'delete'
    token_tag(nil, form_options: form_options.merge(method: 'delete'))
  else
    ''
  end
@kaspth

This comment has been minimized.

Member

kaspth commented Feb 18, 2016

Doesn't button_to also support PUT and PATCH?

@rafaelfranca

View changes

actionview/lib/action_view/helpers/url_helper.rb Outdated
@@ -311,8 +311,9 @@ def button_to(name = nil, options = nil, html_options = nil, &block)
form_options[:action] = url
form_options[:'data-remote'] = true if remote
request_token_tag = if form_method == 'post'
token_tag(nil, form_options: form_options)

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 18, 2016

Member

The only change we need is in this line to token_tag(nil, form_options: { action: url, method: method })

This comment has been minimized.

@vipulnsward
@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Feb 18, 2016

Doesn't button_to also support PUT and PATCH?

Yes. I commented with the code that would make all methods work. We need tests for put and patch too.

@rafaelfranca

View changes

actionpack/test/controller/request_forgery_protection_test.rb Outdated
@@ -710,6 +714,46 @@ def test_rejects_token_for_incorrect_method
end
end
def test_rejects_token_for_incorrect_method_button_to
get :button_to, params: {form_method: 'delete'}

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 18, 2016

Member

space after the { and before }

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Feb 18, 2016

@vipulnsward vipulnsward force-pushed the vipulnsward:23524-fix-button_to_delete branch Feb 19, 2016

@vipulnsward

This comment has been minimized.

Member

vipulnsward commented Feb 19, 2016

All done here.

@kaspth

This comment has been minimized.

Member

kaspth commented Feb 19, 2016

There's still a little ways to go. This code doesn't support PUT or PATCH verbs, we need those too plus tests for them.

Luckily, this is easy if we follow @rafaelfranca's suggestion. We don't have to compare method == 'delete' or any other verb, we only care if the form_method is POST. If so, just pass method straight on through to token_tag. That's what Rafael meant with:

The only change we need is in this line to token_tag(nil, form_options: { action: url, method: method })

@kaspth

View changes

actionpack/test/controller/request_forgery_protection_test.rb Outdated
actual = @controller.send(:unmask_token, Base64.strict_decode64(form_token))
expected = @controller.send(:per_form_csrf_token, session, '/per_form_tokens/post_one', 'delete')
assert_equal expected, actual

This comment has been minimized.

@kaspth

kaspth Feb 19, 2016

Member

Hard to track the purpose of these 8 lines comparing the generated token to the one on the server. Could do well with a custom assertion. E.g. assert_matches_session_token form_token. With that we'd need a helper to extract the form_token ala:

def form_csrf_token
  assert_select 'input[name="custom_authenticity_token"]' do |input|
    return input.first['value']
  end
end
@kaspth

View changes

actionpack/test/controller/request_forgery_protection_test.rb Outdated
form_token = nil
assert_select 'input[name=custom_authenticity_token]' do |elts|
form_token = elts.first['value']
assert_not_nil form_token

This comment has been minimized.

@kaspth

kaspth Feb 19, 2016

Member

Would move this into assert_matches_session_token

@kaspth

View changes

actionpack/test/controller/request_forgery_protection_test.rb Outdated
get :button_to, params: { form_method: 'delete' }
form_token = nil
assert_select 'input[name=custom_authenticity_token]' do |elts|

This comment has been minimized.

@kaspth

kaspth Feb 19, 2016

Member

Nokogiri doesn't like selecting on params without quotes, switch to: 'input[name="custom_authenticity_token"]'

@kaspth

View changes

actionpack/test/controller/request_forgery_protection_test.rb Outdated
# This is required because PATH_INFO isn't reset between requests.
@request.env['PATH_INFO'] = '/per_form_tokens/post_one'
assert_nothing_raised do

This comment has been minimized.

@kaspth

kaspth Feb 19, 2016

Member

We can't make other assertions than assert_nothing_raised?

@rafaelfranca

View changes

actionview/lib/action_view/helpers/url_helper.rb Outdated
@@ -311,8 +311,8 @@ def button_to(name = nil, options = nil, html_options = nil, &block)
form_options[:action] = url
form_options[:'data-remote'] = true if remote
request_token_tag = if form_method == 'post'
token_tag(nil, form_options: form_options)
request_token_tag = if (form_method == 'post' || method == 'delete')

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 19, 2016

Member

We don't need to add the || here.

@rafaelfranca

View changes

actionpack/test/controller/request_forgery_protection_test.rb Outdated
end
def test_accepts_proper_token_for_delete_method_button_to
get :button_to, params: { form_method: 'delete' }

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 19, 2016

Member

We need to test with put and patch too.

@vipulnsward vipulnsward force-pushed the vipulnsward:23524-fix-button_to_delete branch Feb 21, 2016

@vipulnsward

This comment has been minimized.

Member

vipulnsward commented Feb 21, 2016

Refactored the tests to dry them up, into meaningful chunks.

@eileencodes

View changes

actionpack/test/controller/request_forgery_protection_test.rb Outdated
expected = @controller.send(:per_form_csrf_token, session, '/per_form_tokens/post_one', method)
assert_equal expected, actual
end

This comment has been minimized.

@eileencodes

eileencodes Feb 22, 2016

Member

Nitpick: can you remove this extra space here, and you have 2 spaces above where def assert_presence_and_fetch_form_csrf_token starts. Can you remove those too?

@vipulnsward vipulnsward force-pushed the vipulnsward:23524-fix-button_to_delete branch to 2b4c0ae Feb 22, 2016

actual = @controller.send(:unmask_token, Base64.strict_decode64(form_token))
expected = @controller.send(:per_form_csrf_token, session, '/per_form_tokens/post_one', method)
assert_equal expected, actual
end

This comment has been minimized.

@vipulnsward

vipulnsward Feb 22, 2016

Member

@eileencodes done, marked as private as well.

@rafaelfranca rafaelfranca self-assigned this Feb 22, 2016

@rafaelfranca rafaelfranca merged commit 2b4c0ae into rails:master Feb 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

rafaelfranca added a commit that referenced this pull request Feb 22, 2016

Merge pull request #23752 from vipulnsward/23524-fix-button_to_delete
Fixed passing of delete method on button_to tag, creating wrong form csrf token

vipulnsward added a commit to rubys/awdwr that referenced this pull request Feb 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment