Update the form_tag helper to comply with CSP

[rbhitchcock]

Content Security Policy by default does not allow for inline style attributes in HTML tags. The form_for and form_tag actionview helpers, when adding hidden input fields (such as the one containing the CSRF token) to the generated form, create a div tag that has some inline style rules applied.

<!-- Generated form -->
<form>
  <div style="margin:0;padding:0;display:inline">
     <input name="authenticity_token" type="hidden" value="xxx">
  </div>
</form>

In order to comply with CSP, this style attribute should be removed and replaced with a class (if it is desired that the style attributes continue to be applied).

<!-- Generated form -->
<form>
  <div class="extra_tags">
    <input name="authenticity_token" type="hidden" value="xxx">
  </div>
</form>

/* Stylesheet definition */
form div.extra_tags {
  padding: 0;
  margin: 0;
  display: inline;
}

If this change is not made, any developers who wish to return the Content Security Policy header for their application will have to:

1. Patch their install of rails to not use an inline style attribute with this helper
2. Not use the form helpers
3. Define their CSP directive to allow inline style attributes (not ideal or secure)

Hopefully the class name is descriptive enough. First pull request... tried to follow the instructions in the contribution guide. Apologies in advance for anything I overlooked. Thanks!

[Alamoz]

👍 Though I would call the class 'default_tags':

form div.default_tags

Very cool. CSP is an important component of security; I was planning to add CSP stuff myself.

[senny]

/cc @NZKoz

[rbhitchcock]

@Alamoz I would be fine with changing the name of the class. I named it based on the method where it gets created def extra_tags_for_form. Just let me know.

[gaurish]

This adds Stylesheet definition in scaffold's css. But what happens incase there is no scaffold stylesheet present i.e you used form_tag without scaffolding? will the styles still applied correctly?

[Alamoz]

@rbhitchcock the label 'extra_tags' is within the context of ActionView::Helpers::FormTagHelper, externally the tags you are moving into CSS are style tags, so I would change the name of that class to form_helper_default_style_tags in order to be properly descriptive.

[Alamoz]

@gaurish has a good point, a Rails app doesn't generate any css by default, so currently there isn't any place to put that css. But it is certainly good to follow CSP recommendations and those inline style elements are considered insecure. Someone will have to decide whether to add a default stylesheet to rails apps. Also, there is discussion to the effect that inline style elements are not a real security threat (see http://lists.w3.org/Archives/Public/public-webappsec/2013Feb/0015.html) and that JQuery relies on inline style elements for things like tooltips. So this warrants further investigation.

I'm surprised there doesn't seem to be a sub team checking for security weakness in rails.

@tenderlove @dhh

[Alamoz]

@rbhitchcock I think the inline style tags from ActionView::Helpers::FormTagHelper should just be removed altogether and let users extend <form/> in their own stylesheets. There really is no good reason to add them in the helper, especially since all that is happening is margin:0;padding:0;display:inline So I recommend modifying your PR to just remove the inline style tags from the helper and docs and drop the scaffold stylesheet addition.

@tenderlove

[rbhitchcock]

@Alamoz I was actually about to suggest the same thing. They seem to serve no purpose. Initially I didn't define the class in the CSS generator but at the last moment decided to duplicate the style definition.

[Alamoz]

I've sent a message to a web security expert asking his opinion on the vulnerability of inline style sheets. Will post his comments here if and when he responds. Still think you should just remove them.

[NZKoz]

The style is there because without them it caused some rendering glitches in some browser or other, before we remove them we'd have to use git blame to track down the original commit and verify that it's not an issue anymore. From memory the div was only added for some misguided HTML validation reason. No one validates HTML anymore so perhaps the real fix is to just have the inputs inside the form?

If it is an issue, then we'd probably have to leave them in by default because, as mentioned, we don't ship any CSS by default. We could add an option to form_tag to skip the div entirely perhaps?

As for the security issues of inline styles, there are various dangerous things you can do with expressions and moz-binding etc, which is why the CSP disallows them. However there's no issue with the values we're current,y generating,

[gaurish]

+1 for option to form_tag something like csp_compliant : true to skip the inline styles or anything else that makes form CSP compliant

[Alamoz]

@gaurish That sounds like the best solution. I still prefer just removing it. Not finding anything with git-blame, just one comment about inline styles at some point past where the change occurred, so it looks like the change was uncommented when committed.

[steveklabnik]

Yes, there were rendering issues in certain browsers, this behavior was absolutely intended.

[Alamoz]

So the correct way to do it then is to have those tags render by default and if a developer cares more about CSP compliance than compatibility with older browsers an initialization parameter to form_tag will disable them. @gaurish 's suggestion of csp_compliant : true is a good choice.

[Alamoz]

"However there's no issue with the values we're currently generating"

As far as anyone knows at this point 😉 Apparently it isn't the values themselves that are the problem.

[rbhitchcock]

That makes sense to me as well. I will get this request updated in a day or two. Thanks to everyone for the help!

[Alamoz]

I contacted Adam Barth (http://www.schemehostport.com/) and though he didn't offer details (something security experts are loath to do) here is what he had to say:

"Inline style elements certainly give the attacker leverage. The question isn't whether it's a vulnerability but rather how severe a vulnerability."

A good video of a talk he gave on CSP: http://www.youtube.com/watch?v=pocsv39pNXA

cc/ @tenderlove

[NZKoz]

@Alamoz you're missing the point a bit here, there's no question about whether the code we're generating is insecure, it's not. There's no vulnerability at all ever from setting padding and margin on a div.

The issue is that the content security policy doesn't allow inline styles by default, and people want to use CSP and the form_tag makes that annoying / impossible.

As for the name of the option, csp_compliant is a pretty crummy one, it should focus on what changes about the generated markup, not what that happens to enable.

[Alamoz]

"As for the name of the option, csp_compliant is a pretty crummy one, it should focus on what changes about the generated markup, not what that happens to enable."

Interesting. Very HTML centric view of the world.

[Alamoz]

"The issue is that the content security policy doesn't allow inline styles by default, and people want to use CSP and the form_tag makes that annoying / impossible."

As stated earlier, some people will prefer not using CSP for the sake of browser compatibility. That is the point of providing an option.

[NZKoz]

forgive me the sin of having an HTML centric view of the world when discussing the naming of an option that changes the HTML generated by a helper which generates HTML ;)

[Alamoz]

It isn't a sin as much as a bias 😉

In the case of someone who is only interested in enabling CSP, it makes sense to have an option name relating to that intention. But of course anyone thinking of CSP should know that inline style elements are being suppressed. That, and there should be a way to suppress inline styles anyway, as overriding inline styles in css is so ugly. So I agree the option name should relate to HTML. @gaurish I hope you are not disappointed that @NZKoz doesn't like your option name 😉

[gaurish]

@Alamoz
I don't have the bandwidth to get involved into bike shedding type discussions. if you think csp_compliant: true or some other option name is good choice. please go ahead submit a PR, then we will discuss 😍

[Alamoz]

@gaurish Will leave it to @rbhitchcock to name the option, this is his PR. 😄 Other than that, the logic behind his PR is pretty strait-forward.

[rbhitchcock]

Just updated with changes incorporating the discussion so far. Thanks again everyone for the feedback.

[Alamoz]

@rbhitchcock You still need to remove the form_helper_default_tags class reference:

     if !html_options.delete("without_inline_styles")
       content_tag(:div, tags, :style => 'margin:0;padding:0;display:inline')
     else
       content_tag(:div, tags, :class => 'form_helper_default_tags')
     end 

... change to:

    else
      content_tag(:div, tags)

And also change any tests accordingly.

[rbhitchcock]

Sorry... thought you still wanted a class attribute on the div. I've removed it. Thanks!

[Alamoz]

@rbhitchcock Better to leave that to the user if they decide to suppress the default html output.

[guilleiguaran]

Found this discussion about the topic: https://rails.lighthouseapp.com/projects/8994/tickets/5901-suppress-the-generation-of-the-authenticity_token-div

The original commit in Rails 6 years ago:

4e3ed5b#L6L404

P.S: csrf_killer svn repo doesn't exist anymore

[guilleiguaran]

I agree with @dorian, I don't think we need this hack anymore (probably it was for IE5.x or IE6) but we will need to do some researching to be sure about it.

[Alamoz]

@guilleiguaran @dorian - Is the form authenticity token within the scope of this PR?

[Alamoz]

@dorian - Ah yes. I think it is better without the

, it doesn't add anything useful.

[Alamoz]

@rbhitchcock To also suppress the div you will have to make some more changes, specifically in actionpack-4.0.0/lib/action_view/helpers/tag_helper.rb. That or create a safe default value for the div.

[stve]

I found this while working through CSP on a site of mine.

Looks like this has stalled, anything we can do to move this one forward?

[tilsammans]

@guilleiguaran today there is no reason the hidden inputs are wrapped in a div. All browsers hide hidden inputs, including IE6. I think these inline styles stem from a bygone era when we thought inline styles and inline onclicks were fine. Now, we know better.

I vote not to add a configuration option (based on a negative no less) but to get rid of this wrapping div completely. Like in the jsfiddle posted by @dorian, there are no browsers in current use where hidden inputs are visible. Everyone can check this. This div is a barnacle, we should chip it off.

[jeremy]

Here's what led to a <div> with these idiosyncratic inline styles:

1. <input> is not a valid child of <form> by HTML 4.01 Strict and XHTML Strict. Hence the <div> wrapper.
2. Older browsers (Firefox 12, for example) would not submit hidden inputs. Hence the zero-size inline block instead of a hidden div.
3. Applications' CSS would inadvertently override the div's style, causing rendering issues on an element that isn't visible in the template, confusing designers. Hence the explicit inline style.

Note that display:inline was already changed to display:none in #6608. If we shed html-strict validation, which was of dubious real value in the first place, we can drop the div entirely. However, that will affect apps that target the div using a :first-child selector.

[tilsammans]

If there is no outcry from the changes in #6608 it seems safe to get rid of the div altogether. Pull request submitted.

[rbhitchcock]

Removing the div seems like a good solution to me from the standpoint of preventing the CSP violation although the backward compatibility issue pointed out by @jeremy is something to consider.

[rafaelfranca]

Closed by #14738