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
Turbo stream partial with button_to tag throws ActionView::Template::Error #243
Comments
The same is happening with forms that are either post, patch or delete. method: :get works fine. |
Are you sure you have a working session store, as the error mentions? Can you create a boiled down app that replicates this? Not seeing it on my end. |
Yeah, the session store is fine. There's something strange going on here, because a it actually does replace with a normal _form.html.erb. I'll close it for now, won't bother you with it, I'm definitely doing something wrong 😅 |
I believe this needs to be reopened. Rails 7 alpha raises an error when sessions are disabled, which appears to break model broadcasts. A minimal application reproducing the error can be found here: https://github.com/DavidColby/turbo_rails_7_bug This app includes an class Post < ApplicationRecord
after_create_commit -> { broadcast_prepend_to 'posts' }
end And it includes a <%= button_to "Destroy this post", post_path(post), method: :delete %> Note that a To reproduce on this app, boot up the app, head to /posts/new and see that Uncomment this line and reboot the app and see that post submission works as expected. |
Looking at this further, it may be more appropriate to raise this issue against Rails since the change to default to raising errors when the session is disabled causes rendering outside of the controller context to fail and turbo-rails isn't the only library that expects to be able to safely render partials from models, background jobs, etc. I'll leave this here for now and see what others think. IMO, a default in Rails 7 that explicitly breaks rendering from models seems like an incorrect default given the reliance on this pattern in a number of libraries. |
cc @byroot |
It's not really what it's doing. It's a default that ensure that if you view depends on a session store (e..g because it need to store a CSRF token), then that feature ensure it raise loudly rather than letting you generate tokens that will later be rejected because they weren't saved in any session. So I don't think we should change that default, but we should figure out how to get your rendering context access to the session, or your view to no longer need a session. I'm away for the next ~48 hours, but I can dig into this when I get back.
Does the button work too? If the CSRF token wasn't saved in the session, the button shouldn't work. Unless of course you have CSRF protection disabled, in which case the rendering shouldn't generate a token. |
I was able to address this be changing from |
@jwilsjustin Yeah, swapping the You can also do weirder stuff like wrapping the button/form in a lazy loaded @byroot I know very little about the internal workings of CSRF or anything else really, so I'll happily defer to your judgement on all of this stuff. If Rails ships with this as the default behavior, it might at least be worth adding
Yes, the button works as expected, which is surprising to me. To be really sure, I even manually added I'm on another project today but tonight I will spin up a production app that demonstrates the functionality working with As you said, it seems like it should impossible for a form generated without access to the session to pass CSRF checks, but that doesn't seem to be the case in my testing. |
Conceptually it's not that complicated. When you generate a form, you generate a token, store it in session and add it in an hidden field. When the form is submitted you compare the token with the one inside the session to ensure they match. So to generate CSRF protected forms, you need to be able to write in the user session.
Yes, that's rather odd, I don't get how it can possibly work. I'll dig into this Wednesday. Maybe the root problem was worked around somehow? (e.g. by generating the CSRF in the main page, not in the snippets, or something along these lines). If that's the case we'd need to let Action View know that it shouldn't generate a CSRF token in that form. |
It looks like this already happens. Using the example above, when a form is rendered via a broadcast the HTML looks like this: <form class="button_to" method="post" action="/posts/33">
<input type="hidden" name="_method" value="delete">
<button data-confirm="Are you sure?" type="submit">Destroy this post</button>
</form> The same form rendered during a normal page turn looks like this: <form class="button_to" method="post" action="/posts/33">
<input type="hidden" name="_method" value="delete">
<button data-confirm="Are you sure?" type="submit">Destroy this post</button>
<input type="hidden" name="authenticity_token" value="somesecuretoken">
</form> I suppose this means there must be some path that bypasses token generation when the session is unwriteable. Then when the form is submitted without a token, the token from the |
That's what I'd expect yes. |
Some additional info on this, and some thoughts on possible paths forward that don't put folks using Turbo Stream broadcasts, ActionCable broadcasts, or CableReady into a tough spot when they're upgrading. Users can avoid triggering the error (or deprecation warning) for forms rendered without a session like this: <%= form_with(model: resource, authenticity_token: false) do |form| %>
If Another potential route that avoids the need to ask users to add def token_tag(token = nil, form_options: {})
if token != false && defined?(session) && session.enabled? && defined?(protect_against_forgery?) && protect_against_forgery?
# generate token
else
""
end
end
The latter option of adjusting This feels like it is pretty far outside the scope of the original report at this point, and I'm happy to open a new issue against Rails to track this if that's preferable. |
I know little of hotwire & al, but yes we should find a solution that doesn't require it.
Ideally we wouldn't rely on What would be best would be to detect wether we're rendering a full page or just a fragment. |
That would run into problems since it is common to render a partial from a normal controller action, and in those cases users would likely expect their form to generate an authenticity token. Perhaps something like this, borrowing from request.controller_class to check if the request is originating from a controller? def controller_request?
request.present? && request.params["controller"].present?
end
def token_tag(token = nil, form_options: {})
if token != false && controller_request? && defined?(protect_against_forgery?) && protect_against_forgery?
token ||= form_authenticity_token(form_options: form_options)
tag(:input, type: "hidden", name: request_forgery_protection_token.to_s, value: token, autocomplete: "off")
else
""
end
end This bails out prior to the session check and seems to resolve the issue locally. I'm unsure if there are other paths that one can take that would result in a request without a controller present that might make this approach problematic. |
In preparation for troubleshooting [hotwired#243][], this commit expands the GitHub Actions CI matrix to account for both `rails@~>6.1` and `rails@main`. [hotwired#243]: hotwired#243
In preparation for troubleshooting [hotwired#243][], this commit expands the GitHub Actions CI matrix to account for both `rails@~>6.1` and `rails@main`. [hotwired#243]: hotwired#243
In preparation for troubleshooting [hotwired#243][], this commit expands the GitHub Actions CI matrix to account for both `rails@~>6.1` and `rails@main`. [hotwired#243]: hotwired#243
In preparation for troubleshooting [hotwired#243][], this commit expands the GitHub Actions CI matrix to account for both `rails@~>6.1` and `rails@main`. [hotwired#243]: hotwired#243
In preparation for troubleshooting [hotwired#243][], this commit expands the GitHub Actions CI matrix to account for both `rails@~>6.1` and `rails@main`. [hotwired#243]: hotwired#243
In preparation for troubleshooting [hotwired#243][], this commit expands the GitHub Actions CI matrix to account for both `rails@~>6.1` and `rails@main`. [hotwired#243]: hotwired#243
In preparation for troubleshooting [hotwired#243][], this commit expands the GitHub Actions CI matrix to account for both `rails@~>6.1` and `rails@main`. [hotwired#243]: hotwired#243
I agree, I think this issue needs to be re-opened. I believe the issue is related to the disparity between how the implementations for The The default behavior is to treat a missing Alternatively, the The As a short-term, minimal change, I've opened rails/rails#43417 to add support for passing I believe a more broad solution would involve making the Ruby-side changes to continue to deprecate UJS. However, it's unclear to me what the desired behavior should be once support for It might be worthwhile to introduce a global configuration value that doesn't target a specific form building interface (be it In the short term, could rails/rails#43417 combined with |
@seanpdoyle Thanks for taking the time to look at this in detail!
That change would make it possible to use The default behavior in Rails 6 is for rendering forms like this to work without needing to worry about the authenticity token so I expect even with the I like the idea of a global config value (perhaps repurposing |
@DavidColby I think all of these pieces could fit together in a way that resolves the underlying issue. Based on the way you've outlined things, I wonder if the In the code snippet you've shared about, what is the significance of: def controller_request?
request.present? && request.params["controller"].present?
end Under what circumstances would the return value of |
I haven't worked through the Rails internals enough so I may be misunderstanding what's happening but I believe that when turbo-rails calls Running things locally, if I add a
|
What about #256 ? |
I opened rails/rails#43427 instead. I don't see a good way to identify these cases, so let's get rid of the error/warning for It will still raise if you try to write to a disabled session directly, so we keep the useful error for all other cases. |
In theory this should have warned early that the CSRF check will fail, which would have been less puzzling for the developer. However there are several cases where we render forms but the session is inacessible. That's the case of turbo (hotwired/turbo-rails#243) as well as some others. So unless we figure a proper way to detect these cases, we're better to not cause this error. Writing to a disabled session directly will still raise, this only silence it for the specific case of CSRF.
I've tested rails/rails#43427 locally, and it seems to resolve this issue. Thank you both @casperisfine @byroot! It seems like rails/rails#43417 and rails/rails#43418 aren't necessary for resolving this issue. I'm planning on keeping them open, since they might still be viable in their own right. |
I have a related error: |
When I was using <%= form_with(model: post, method: :delete, authenticity_token: false) do |form| %>
<%= form.button :submit, class: "..." do %>
<svg ...>
</svg>
<% end %>
<% end %> But now, using <%= button_to post_path(post), method: :delete, class: "..." do %>
<svg ...>
</svg>
<% end %> |
My solution was a little different: import { Controller } from "@hotwired/stimulus"
export default class extends Controller {
connect() {
let form = this.element
if (this.element.type === "submit" || this.element.type === "button") {
form = this.element.closest("form")
}
if (form.querySelector("input[name='authenticity_token']") == null) {
form.appendChild(this.authenticityToken)
}
}
get authenticityToken() {
const input = document.createElement("input")
input.type = "hidden"
input.name = "authenticity_token"
input.autocomplete = "off"
input.value = window.mrujs.csrfToken()
return input
}
} I use mrujs to set an authenticity token dynamically based on the one on the top level. |
The same is happening with
|
I have a similar situation like @tbcooney has with |
@tbcooney @AhmedNadar Broadcast from your background job a lazy-loaded turbo frame. That lazy-loaded frame will then in turn load your form with a second request, using the current user's session. E.g. # notes/app/jobs/bookmark_to_note_job.rb
class BookmarkToNoteJob < ApplicationJob
queue_as :default
def perform(bookmark)
....
broadcast_prepend_to("#{bookmark.notebook.id}_notes",
target: 'notes',
partial: 'notes/note_frame',
object: bookmark.note,
as: :note, locals: { note_active: true })
end
end <!-- notes/app/views/notes/_note_frame.html.erb -->
<%= turbo_frame_tag note, src: edit_note_path(note), loading: :lazy do %>
<!-- The notes/edit form will render here -->
<% end %> <!-- notes/app/views/notes/edit.html.erb -->
<turbo-frame id="<%= dom_id(@note) %>">
<%= form_with(model: @note) do |form| %>
....
<%= form.rich_text_area :details %>
<% end %>
</turbo-frame> |
Well, I might be late to the party. When I have Is implementing redirects to external domains using turbo a solution for me? |
If you are redirecting outside of your own domain then there is no real benefit to using Turbo, but to your point I don’t believe the API will allow it. Make sure when you redirect to Stripe Checkout/Portal in your controller use use https://api.rubyonrails.org/classes/ActionController/Redirecting.html#method-i-redirect_to |
I have |
Correct, which is why the only good solution here is to disable Turbo for this action |
Yes, but then the |
My solution with using mrujs to inject the authenticity token as turbo adds it to the DOM is pretty slick @alhafoudh |
@mhenrixon It might be, but that covers only |
Hello, just another update on the problem @aantix seems the only solution through turbo atm when triggering the broadcast from a worker. But in the case of displaying a collection that relies on this broadcast you are then forced to setup an n+x situation. So it can be a solution, but you are quickly forced to use another system whenever the n+x is too much of trouble when you can't do without authentication. |
I was having this same issue and was able to get around it with a pretty simple stimulus controller which I believe could be used with any type of rails form, however I've only used it with refresh_authenticity_token_controller.js
html.erb usage<%= button_to "Do something", some_path(model), form: { data: { controller: "refresh-authenticity-token",
action: "submit->refresh-authenticity-token#refresh"}}, method: :post %> |
Stumbled upon this today. a button_to is used in a partial that is broadcasted. Clicking on it results in "ActionController::InvalidAuthenticityToken (Can't verify CSRF token authenticity.)" It just seems surprising that it does not work out of the box |
We fixed this issue a very long time ago, but we forgot to close the issue. You must be experiencing something different, would be best to open another issue. @dhh could you close this issue? |
Oh, thanks for letting me know. You are probably right. I saw some merger,
but as the issue was still open though there is more work to be done.
Will see what is causing it on my end.
…On Wed, 1 Mar 2023, 17:58 Jean Boussier, ***@***.***> wrote:
We fixed this issue a very long time ago, but we forgot to close the issue.
You must be experiencing something different, would be best to open
another issue.
@dhh <https://github.com/dhh> could you close this issue?
—
Reply to this email directly, view it on GitHub
<#243 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAKAGBIOIWOJYW7GBOAY47TWZ5W2BANCNFSM5EQXTQVA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
I had something similar, but I was replacing a view component using
|
after_create_commit
withbroadcast_append_to
throws this error:ActionView::Template::Error (Request forgery protection requires a working session store but your application has sessions disabled. You need to either disable request forgery protection, or configure a working session store.)
(seems to happen to all broadcasts with partials)
Log trace:
Rails 7 alpha2, turbo-rails 0.7.14
The text was updated successfully, but these errors were encountered: