Permalink
Browse files

Do not include the authenticity token in forms where remote: true as …

…ajax forms use the meta-tag value
  • Loading branch information...
1 parent f2aea24 commit 16ee611fad37b6b271088eda4cdbe3d6be088af1 @dhh dhh committed Mar 14, 2012
View
@@ -1,5 +1,7 @@
## Rails 3.2.3 (unreleased) ##
+* Do not include the authenticity token in forms where remote: true as ajax forms use the meta-tag value *DHH*
+
* Turn off verbose mode of rack-cache, we still have X-Rack-Cache to
check that info. Closes #5245. *Santiago Pastorino*
@@ -609,8 +609,15 @@ def html_options_for_form(url_for_options, options)
# responsibility of the caller to escape all the values.
html_options["action"] = url_for(url_for_options)
html_options["accept-charset"] = "UTF-8"
- html_options["data-remote"] = true if html_options.delete("remote")
- html_options["authenticity_token"] = html_options.delete("authenticity_token") if html_options.has_key?("authenticity_token")
+
+ if html_options.delete("remote")
+ html_options["data-remote"] = true
+
+ # The authenticity token is taken from the meta tag in this case
+ html_options["authenticity_token"] = false
+ else
+ html_options["authenticity_token"] = html_options.delete("authenticity_token") if html_options.has_key?("authenticity_token")
@exviva

exviva Mar 28, 2012

Contributor

What's the sense of this code? I know that's how it used to be, I'm just curious, maybe it didn't make sense before. It seems to me that it's deleting a value from a hash and immediately assigning it back under the same key.

On a side note, what's the rationale behind not including authenticity_token for AJAX forms? If it doesn't hurt, maybe it doesn't make sense to have this conditional, and an extra config option, and another option for form_for.

@drogus

drogus Mar 28, 2012

Member

On a side note, what's the rationale behind not including authenticity_token for AJAX forms?
If it doesn't hurt, maybe it doesn't make sense to have this conditional, and an extra config
option, and another option for form_for.

It can hurt if you cache the form. If you use ajax only you will get auth token from meta tags anyway, so it matters only if you want to also handle the form without javascript. also, see this: d646d9d

@exviva

exviva Mar 28, 2012

Contributor

Don't the UJS drivers override the authenticity_token hidden input's value with the meta tag's value anyway?

I was referring to your commit when mentioning the extra config option. All in all, I'm still trying to fully understand this change - thanks for the information.

@drogus

drogus Mar 28, 2012

Member

Don't the UJS drivers override the authenticity_token hidden input's value with the meta tag's value anyway?

Frankly, I'm not sure :)

@drogus

drogus Mar 28, 2012

Member

But even if yes, you don't necessarily need to use UJS drivers. We can't rely on it.

+ end
end
end
@@ -37,6 +37,10 @@ def form_for_without_protection
render :inline => "<%= form_for(:some_resource, :authenticity_token => false ) {} %>"
end
+ def form_for_remote
+ render :inline => "<%= form_for(:some_resource, :remote => true ) {} %>"
+ end
+
def rescue_action(e) raise e end
end
@@ -103,6 +107,13 @@ def test_should_render_button_to_with_token_tag
assert_select 'form>div>input[name=?][value=?]', 'custom_authenticity_token', @token
end
+ def test_should_render_form_without_token_tag_if_remote
+ assert_not_blocked do
+ get :form_for_remote
+ end
+ assert_no_match /authenticity_token/, response.body
+ end
+
def test_should_allow_get
assert_not_blocked { get :index }
end

6 comments on commit 16ee611

Contributor

khustochka replied Mar 19, 2012

I wonder what happens in terms of graceful degradation then. With JavaScript off will the form remain protected from forgery?

Member

drogus replied Mar 19, 2012

In one of the next commits there is an option added to pass authenticity_token: true in order to keep that behavior.

Contributor

khustochka replied Mar 19, 2012

Thanks, drogus. My fault, I must have looked at the next commit.

Contributor

christos replied Mar 27, 2012

Not trolling here, but wouldn't this change (and the one in the next commit), break a lot of code where people do The Right Thing, and make their forms unobtrusive with graceful degradation?

I am not protesting the commit, just the fact that it was included in a patch version update (3.2.3) and it has the potential to catch a lot of people unawares...

Owner

jeremy replied Mar 27, 2012

👍 to preserve the original behavior for 3-2-stable. We could use a config option to disable 'em instead.

Member

drogus replied Mar 28, 2012

@christos fixed here: d646d9d, thanks for reporting!

Please sign in to comment.