Skip to content

Commit

Permalink
FEATURE: add site setting to remove X-Frame-Options header.
Browse files Browse the repository at this point in the history
  • Loading branch information
vinothkannans committed Dec 5, 2019
1 parent 0098555 commit f7084a4
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 1 deletion.
2 changes: 1 addition & 1 deletion config/initializers/011-rack-protection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,4 @@

require 'rack/protection'

Rails.configuration.middleware.use Rack::Protection::FrameOptions
Rails.configuration.middleware.use Middleware::FrameOptions
1 change: 1 addition & 0 deletions config/locales/server.en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1517,6 +1517,7 @@ en:
content_security_policy_collect_reports: "Enable CSP violation report collection at /csp_reports"
content_security_policy_script_src: "Additional whitelisted script sources. The current host and CDN are included by default. See <a href='https://meta.discourse.org/t/mitigate-xss-attacks-with-content-security-policy/104243' target='_blank'>Mitigate XSS Attacks with Content Security Policy.</a>"
invalidate_inactive_admin_email_after_days: "Admin accounts that have not visited the site in this number of days will need to re-validate their email address before logging in. Set to 0 to disable."
allow_embedding_site_in_an_iframe: "Enable embedding of the site in iframes."
top_menu: "Determine which items appear in the homepage navigation, and in what order. Example latest|new|unread|categories|top|read|posted|bookmarks"
post_menu: "Determine which items appear on the post menu, and in what order. Example like|edit|flag|delete|share|bookmark|reply"
post_menu_hidden_items: "The menu items to hide by default in the post menu unless an expansion ellipsis is clicked on."
Expand Down
2 changes: 2 additions & 0 deletions config/site_settings.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1402,6 +1402,8 @@ security:
default: 365
min: 0
max: 2000
allow_embedding_site_in_an_iframe:
default: false

onebox:
enable_flash_video_onebox: false
Expand Down
15 changes: 15 additions & 0 deletions lib/middleware/frame_options.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# frozen_string_literal: true

module Middleware
class FrameOptions
def initialize(app, settings = {})
@app = app
end

def call(env)
status, headers, body = @app.call(env)
headers.except!('X-Frame-Options') if SiteSetting.allow_embedding_site_in_an_iframe
[status, headers, body]
end
end
end
14 changes: 14 additions & 0 deletions spec/requests/application_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,20 @@
end
end

describe 'allow_embedding_site_in_an_iframe' do

it "should have the 'X-Frame-Options' header with value 'sameorigin'" do
get("/latest")
expect(response.headers['X-Frame-Options']).to eq("SAMEORIGIN")
end

it "should not include the 'X-Frame-Options' header" do
SiteSetting.allow_embedding_site_in_an_iframe = true
get("/latest")
expect(response.headers).not_to include('X-Frame-Options')
end
end

describe 'Delegated auth' do
let :public_key do
<<~TXT
Expand Down

5 comments on commit f7084a4

@discoursebot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/discourse-integration-for-microsoft-teams/101577/24

@discoursereviewbot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SamSaffron posted:

Weird, what middleware do we have that is adding X-Frame-Options in the first place? Looks like Rack::Protection::FrameOptions was totally superfluous.

@discoursereviewbot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vinoth Kannan posted:

Yes, rails itself adding the X-Frame-Options header by default.

Every HTTP response from your Rails application receives the following default security headers.
https://guides.rubyonrails.org/security.html#default-headers

@discoursereviewbot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SamSaffron posted:

hmmm maybe it makes sense then just to do this in application_controller.rb in an after_action ?

I think people will understand it a bit better.

after_action :conditionally_allow_site_embedding


def conditionally_allow_site_embedding
   if SiteSetting.allow_embedding_site_in_an_iframe
      response.headers.delete('X-Frame-Options')
   end
end

Code is much shorter

@discoursereviewbot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.