Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

homakov commented May 28, 2012

...generator. Closes #6311

@josevalim josevalim and 3 others commented on an outdated diff May 28, 2012

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
@homakov

homakov May 28, 2012

Contributor

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.

@spastorino

spastorino May 28, 2012

Owner

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

@josevalim

josevalim May 28, 2012

Contributor

You can fix this pull request and push with -f

@drogus

drogus May 28, 2012

Member

@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

Contributor

homakov commented May 28, 2012

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 spastorino commented on the diff May 28, 2012

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)
@spastorino

spastorino May 28, 2012

Owner

I'd initialize this to nil to avoid warnings

@homakov

homakov May 28, 2012

Contributor

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


@spastorino

spastorino May 28, 2012

Owner

true, cool :)

@spastorino spastorino and 2 others commented on an outdated diff May 28, 2012

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?
+ @header['X-Frame-Options'] ||= self.class.default_x_frame_options
+ end
+
@spastorino

spastorino May 28, 2012

Owner

Why not just @header['X-Frame-Options'] = self.class.default_x_frame_options ?

@josevalim

josevalim May 28, 2012

Contributor

The user may also set this value to something different in his controller.

@homakov

homakov May 28, 2012

Contributor

@spastorino @header['X-Frame-Options'] ||= nil

rescues


Internal Server Error

undefined method `split' for nil:NilClass

anyway, We don't need to send x-frame-options(nil or anything) if we don't have this in config

@homakov

homakov May 28, 2012

Contributor

yes, also we need ||= because of custom headers, @josevalim is right

Owner

spastorino commented May 28, 2012

And test would be awesome :)

@josevalim josevalim commented on an outdated diff May 28, 2012

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?
@josevalim

josevalim May 28, 2012

Contributor

This could simply be :

if self.class.default_x_frame_options
Contributor

homakov commented May 28, 2012

homakov added some commits

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

Owner

spastorino commented May 28, 2012

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

Contributor

homakov commented May 28, 2012

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

Contributor

josevalim commented May 28, 2012

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

@exviva exviva and 1 other commented on an outdated diff May 28, 2012

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
@exviva

exviva May 28, 2012

Contributor

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

Good idea BTW!

@homakov

homakov May 28, 2012

Contributor

@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!

Contributor

homakov commented May 28, 2012

@josevalim added a test

Set X-Frame-Options to SAMEORIGIN and add description to application.rb
X-Frame-Options is responsible for displaing the website content in frames.
Disallowing it we mitigate Active XSSes and Clickjacking attacks.
Closes #6311
Contributor

homakov commented May 29, 2012

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?

Contributor

josevalim commented May 29, 2012

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

@exviva exviva commented on the diff May 29, 2012

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
@exviva

exviva May 29, 2012

Contributor

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).

Contributor

homakov commented Jun 1, 2012

bump :) @NZKoz

Contributor

homakov commented Jun 2, 2012

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

Contributor

josevalim commented Jun 2, 2012

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

Sent from my iPhone

Contributor

homakov commented Jun 2, 2012

do we? r u sure

Contributor

josevalim commented Jun 2, 2012

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

Contributor

homakov commented Jun 3, 2012

@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

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

@homakov homakov closed this Aug 10, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment