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 ourterms of serviceand privacy statement.We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActionController::InvalidAuthenticityToken when submitting a form via button withformmethod::delete #52290

Closed
pokonskiopened this issue Jul 8, 2024 · 6 comments

Comments

@pokonski
Copy link
Contributor

pokonski commented Jul 8, 2024

Steps to reproduce

  1. Create a new app withrails _7.2.0.beta2_ new csrf_repro -d sqlite3
  2. Add scaffold withrails g scaffold Products
  3. Replaceapp/views/products/show.html.erbwith following:
<%=form_with model: @product do |f|%>
<%=f.button "Remove product", formmethod::delete%>
<%end%>
  1. Disable JavaScript
  2. Navigate tohttp://localhost:3000/products/new
  3. Create new product
  4. Click "Remove product"

Repo with the project repro:https://github /pokonski/csrf_repro

Expected behavior

products#destroy action should be properly executed and product removed.

The docs suggest this is possible (https://guides.rubyonrails.org/form_helpers.html#how-do-forms-with-patch-put-or-delete-methods-work-questionmark) b

Actual behavior

ActionController::InvalidAuthenticityTokenis raised instead when Hotwire is disabled (via Javascript disable or just removing it from application.js)

The difference seems to be in how_methodparam is submitted:

  • with JS and Hotwire enabled there's only a single param:
image
  • without JS/Hotwire the_methodparam appears twice with both the original method (patch) anddelete
image

Issue is present in both Firefox 127 and Chrome 126

System configuration

Rails version:7.2.0-beta2

Ruby version:ruby 3.2.0 (2022-12-25 revision a528908271) [arm64-darwin23]

@pokonski
Copy link
Contributor Author

pokonski commented Jul 8, 2024

After debuggingvalid_authenticity_token?method inlib/action_controller/metal/request_forgery_protection.rbit seems the token check fails onvalid_per_form_csrf_token?(csrf_token)for the DELETE request, but succeeds when the method isPATCH

Most likely because

defper_form_csrf_token(session,action_path,method)#:doc:
csrf_token_hmac(session,[action_path,method.downcase].join("#"))
end

creates the token for the original method (PATCH) inform_withbut not for theformMethodon the button

@pokonski
Copy link
Contributor Author

pokonski commented Jul 8, 2024

Okay, so disabling per-form tokens fixes the issue:config.action_controller.per_form_csrf_tokens = false

Should that be documented in the guides about button'sformmethodattribute? Or maybe also a runtime warning? If so I can prepare a fix to the docs.

This works with Hotwire, because then it uses the global CSRF token from meta tags, not the method-specific one generated for this form.

@pokonski pokonski changed the title ActionController::InvalidAuthenticityToken when submitting a form via button withformaction::delete ActionController::InvalidAuthenticityToken when submitting a form via button withformmethod::delete Jul 8, 2024
@Jay0921
Copy link
Contributor

Jay0921 commented Jul 10, 2024

This seems like a bug to me. Not only is the token generated using the wrong identifier (it uses the method in the form rather thanformmethod), but the request is also verified against a global CSRF token instead of a form-specific CSRF token.

I looked into the code, and fi xing this might not be straightforward. Theform_withmethod doesn’t have access to theformmethodattribute within the block. Moving theformmethodto theform_withmethod could be a simpler solution, but I’m not sure about the side effects. Let’s wait for the core team’s response.

If we can accessformmethodhere, we can override thehtml_options.

builder=instantiate_builder(scope,model,options)
output=capture(builder,&block)
options[:multipart]||=builder.multipart?
html_options=html_options_for_form_with(url,model,**options)
form_tag_with_body(html_options,output)

@pokonski
Copy link
Contributor Author

pokonski commented Jul 10, 2024

I think this could be approached in the opposite direction. When callingbuttonwith a:formmethodattribute, it could switch the form'sauthenticity_tokento a global one instead of form-specific. Because we can't have multiple tokens targetting different methods in one form and we don't know which submit button user might press

Moving the formmethod to the form_with method could be a simpler solution

That won't work IMO because one can have multiple submit buttons with differentformmethodvalues

@Jay0921
Copy link
Contributor

Jay0921 commented Jul 10, 2024

@pokonskiYou are right. I wasn't aware of the use cases for multiple submit buttons. In that case, movingformmethodto theform_withmethod won't work.

@rails-bot
Copy link

rails-bot bot commented Oct 8, 2024

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the7-2-stablebranch or onmain,please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Oct 8, 2024
@rails-bot rails-bot bot closed this ascompleted Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants