Set X-Frame-Options to SAMEORIGIN and add description to application.rb ... #6515

homakov · 2012-05-28T13:32:18Z

...generator. Closes #6311

josevalim · 2012-05-28T13:33:56Z

railties/lib/rails/application/finisher.rb

@@ -102,6 +102,12 @@ module Finisher
          at_exit { app.queue_consumer.shutdown }
        end
      end
+
+      initializer :set_default_headers do


This can go inside ActionDispatch::Railtie:

https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/railtie.rb#L22

Hmm, Agreed! (how to fix that, PR again?)

but I think we will add https://developer.mozilla.org/en/Introducing_Content_Security_Policy pretty soon to the same initializer - and dispatch initializer is huge enough.

Agree with Valim here the if is not needed just assign directly

You can fix this pull request and push with -f

@homakove no need for new pull request, please do git commit --amend to amend to the last commit and git push --force to overwrite your branch

homakov · 2012-05-28T13:34:54Z

I found some time :)
This implementation seems good to me, ask if anything looks strange or should be in another place

/cc @spastorino @steveklabnik @NZKoz

spastorino · 2012-05-28T13:41:26Z

actionpack/lib/action_dispatch/http/response.rb

@@ -59,6 +59,7 @@ class Response
    LOCATION     = "Location".freeze

    cattr_accessor(:default_charset) { "utf-8" }
+    cattr_accessor(:default_x_frame_options)


I'd initialize this to nil to avoid warnings

it is nil by default

def cattr_reader(*syms) options = syms.extract_options! syms.each do |sym| raise NameError.new('invalid attribute name') unless sym =~ /^[_A-Za-z]\w*$/ class_eval(<<-EOS, __FILE__, __LINE__ + 1) unless defined? @@#{sym} @@#{sym} = nil end def self.#{sym} @@#{sym} end EOS

true, cool :)

spastorino · 2012-05-28T13:44:20Z

And test would be awesome :)

josevalim · 2012-05-28T13:53:13Z

actionpack/lib/action_dispatch/http/response.rb

@@ -160,6 +161,10 @@ def to_a

      @header[SET_COOKIE] = @header[SET_COOKIE].join("\n") if @header[SET_COOKIE].respond_to?(:join)

+      if !self.class.default_x_frame_options.nil?


This could simply be :

if self.class.default_x_frame_options

homakov · 2012-05-28T14:11:53Z

homakov added some commits

that commit contains fixes. I thought it will copy commit message :/

spastorino · 2012-05-28T14:38:34Z

@homakov git commit --amend change the commit message and git push -f :)

homakov · 2012-05-28T14:51:39Z

@spastorino voila! :)
P.s. Thank you all for advices, it was really helpful!

josevalim · 2012-05-28T15:14:30Z

Just a test is missing now. /cc @NZKoz wdyt?

exviva · 2012-05-28T15:17:24Z

actionpack/lib/action_dispatch/railtie.rb

@@ -24,6 +24,10 @@ class Railtie < Rails::Railtie
      ActionDispatch::Request.ignore_accept_header = app.config.action_dispatch.ignore_accept_header
      ActionDispatch::Response.default_charset = app.config.action_dispatch.default_charset || app.config.encoding

+      if config.action_dispatch.x_frame_options


Is this if really necessary? I think this PR also needs tests and documentation.

Good idea BTW!

@exviva it's just additional check. Someday we can change class attribute to another value and then it will rewrite even if it's nil and it can cause mess. So, it's not really necessary but we need it :)

I will provide 'em!

homakov · 2012-05-28T19:47:45Z

@josevalim added a test

X-Frame-Options is responsible for displaing the website content in frames. Disallowing it we mitigate Active XSSes and Clickjacking attacks. Closes #6311

homakov · 2012-05-29T11:49:30Z

hallelujah, fixed slight mistake with 'app.config' and made it as @exviva (for now we can assign directly)
also exteneded the commit message to give people a hence
@josevalim do I miss something?

josevalim · 2012-05-29T12:13:45Z

It looks good. I am waiting for @NZKoz feedback before we merge, thanks!

exviva · 2012-05-29T12:24:57Z

actionpack/test/dispatch/response_test.rb

@@ -147,6 +147,16 @@ def test_response_body_encoding
      ActionDispatch::Response.default_charset = original
    end
  end
+
+  test "read x_frame_options" do


How about also adding these tests:

when ActionDispatch::Response.default_x_frame_options is nil, the 'X-Frame-Options' header is not set

when the response header is already set, it's not overwritten by ActionDispatch::Response.default_x_frame_options

It's also good practice to reset global state after each test (in your case ActionDispatch::Response.default_x_frame_options = 'DENY' will persist between tests).

homakov · 2012-06-01T20:35:48Z

bump :) @NZKoz

homakov · 2012-06-02T16:55:36Z

do not merge, I got a new idea!
http://recxltd.blogspot.co.uk/2012/03/seven-web-server-http-headers-that.html
Currently we already have 7 headers and this is only security - related headers
I propose to add config.action_dispatch.default_headers hash, which will keep all default headers that rails will add to every response. configuration may look like this:

  config.action_dispatch.default_headers = {
"X-Frame-Options" => "SAMEORIGIN",
"Access-Control-Allow-Origin" => "*",
"X-Content-Type-Options" => "newsniff"
"X-XSS-Protection" => "1; mode=block",
"X-Content-Security-Policy" => "blahblahblahblah......."
}

This config is easy to understand and maintain, and agile if you just want to add your own default header. It may also contain content security policy string.
I can do that, thoughts? /cc @josevalim @steveklabnik @spastorino @wycats @NZKoz

josevalim · 2012-06-02T17:55:22Z

Good plan. I believe we already have a default headers thing?

Sent from my iPhone

homakov · 2012-06-02T19:33:40Z

do we? r u sure

josevalim · 2012-06-02T19:35:18Z

Not sure at all. :) Trust the code, not me. :)

homakov · 2012-06-03T09:20:01Z

@josevalim trust me, we don't :)
I can put it here https://github.com/ysoslow/rails/blob/24085a8a4717f447259845c17e154b4f722506e3/actionpack/lib/action_dispatch/http/response.rb#L68

      self.body, self.header, self.status = body, self.class.default_headers.merge(header), status

Introduce default_headers. closes #6311 #6515

josevalim reviewed May 28, 2012
View reviewed changes

spastorino reviewed May 28, 2012
View reviewed changes

josevalim reviewed May 28, 2012
View reviewed changes

exviva reviewed May 28, 2012
View reviewed changes

Set X-Frame-Options to SAMEORIGIN and add description to application.rb

24085a8

X-Frame-Options is responsible for displaing the website content in frames. Disallowing it we mitigate Active XSSes and Clickjacking attacks. Closes #6311

exviva reviewed May 29, 2012
View reviewed changes

rafaelfranca mentioned this pull request Aug 9, 2012

Introduce default_headers. closes #6311 #6515 #7302

Merged

tenderlove added a commit that referenced this pull request Aug 9, 2012

Merge pull request #7302 from homakov/default_headers

6794e92

Introduce default_headers. closes #6311 #6515

homakov closed this Aug 10, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set X-Frame-Options to SAMEORIGIN and add description to application.rb ... #6515

Set X-Frame-Options to SAMEORIGIN and add description to application.rb ... #6515

homakov commented May 28, 2012

josevalim May 28, 2012

homakov May 28, 2012

spastorino May 28, 2012

josevalim May 28, 2012

drogus May 28, 2012

homakov commented May 28, 2012

spastorino May 28, 2012

homakov May 28, 2012

spastorino May 28, 2012

spastorino commented May 28, 2012

josevalim May 28, 2012

homakov commented May 28, 2012

spastorino commented May 28, 2012

homakov commented May 28, 2012

josevalim commented May 28, 2012

exviva May 28, 2012

homakov May 28, 2012

homakov commented May 28, 2012

homakov commented May 29, 2012

josevalim commented May 29, 2012

exviva May 29, 2012

homakov commented Jun 1, 2012

homakov commented Jun 2, 2012

josevalim commented Jun 2, 2012

homakov commented Jun 2, 2012

josevalim commented Jun 2, 2012

homakov commented Jun 3, 2012

		@@ -160,6 +161,10 @@ def to_a

		@header[SET_COOKIE] = @header[SET_COOKIE].join("\n") if @header[SET_COOKIE].respond_to?(:join)

		if !self.class.default_x_frame_options.nil?

Set X-Frame-Options to SAMEORIGIN and add description to application.rb ... #6515

Set X-Frame-Options to SAMEORIGIN and add description to application.rb ... #6515

Conversation

homakov commented May 28, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homakov commented May 28, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spastorino commented May 28, 2012

Choose a reason for hiding this comment

homakov commented May 28, 2012

spastorino commented May 28, 2012

homakov commented May 28, 2012

josevalim commented May 28, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homakov commented May 28, 2012

homakov commented May 29, 2012

josevalim commented May 29, 2012

Choose a reason for hiding this comment

homakov commented Jun 1, 2012

homakov commented Jun 2, 2012

josevalim commented Jun 2, 2012

homakov commented Jun 2, 2012

josevalim commented Jun 2, 2012

homakov commented Jun 3, 2012