From 4a818c07bb76a9a02928b0a1e788705f295683f7 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley Date: Sat, 23 Dec 2017 17:02:23 +0000 Subject: [PATCH 1/4] Only include one host in embedded shopfornt headers --- app/controllers/application_controller.rb | 10 ++++++++-- .../consumer/shopping/embedded_shopfronts_spec.rb | 1 + spec/requests/embedded_shopfronts_headers_spec.rb | 7 +++++-- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index b241dede09..7a64aa1776 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -56,16 +56,22 @@ class ApplicationController < ActionController::Base def enable_embedded_shopfront whitelist = Spree::Config[:embedded_shopfronts_whitelist] - return unless Spree::Config[:enable_embedded_shopfronts] && whitelist.present? + domain = embedded_shopfront_referer + return unless Spree::Config[:enable_embedded_shopfronts] && whitelist.present? && domain.present? && whitelist.include?(domain) return if request.referer && URI(request.referer).scheme != 'https' && !Rails.env.test? && !Rails.env.development? response.headers.delete 'X-Frame-Options' - response.headers['Content-Security-Policy'] = "frame-ancestors #{whitelist}" + response.headers['Content-Security-Policy'] = "frame-ancestors #{domain}" check_embedded_request set_embedded_layout end + def embedded_shopfront_referer + return if request.referer.blank? + URI(request.referer).host.sub!(/^www./, '') + end + def check_embedded_request return unless params[:embedded_shopfront] diff --git a/spec/features/consumer/shopping/embedded_shopfronts_spec.rb b/spec/features/consumer/shopping/embedded_shopfronts_spec.rb index 12770fc070..acac3028df 100644 --- a/spec/features/consumer/shopping/embedded_shopfronts_spec.rb +++ b/spec/features/consumer/shopping/embedded_shopfronts_spec.rb @@ -25,6 +25,7 @@ feature "Using embedded shopfront functionality", js: true do Spree::Config[:embedded_shopfronts_whitelist] = 'localhost' page.driver.browser.js_errors = false + allow_any_instance_of(ApplicationController).to receive(:embedded_shopfront_referer).and_return('localhost') Capybara.current_session.driver.visit('spec/support/views/iframe_test.html') end diff --git a/spec/requests/embedded_shopfronts_headers_spec.rb b/spec/requests/embedded_shopfronts_headers_spec.rb index a991d7362c..8056946f23 100644 --- a/spec/requests/embedded_shopfronts_headers_spec.rb +++ b/spec/requests/embedded_shopfronts_headers_spec.rb @@ -43,16 +43,19 @@ describe "setting response headers for embedded shopfronts", type: :request do context "with a valid whitelist" do before do - Spree::Config[:embedded_shopfronts_whitelist] = "test.com" + Spree::Config[:embedded_shopfronts_whitelist] = "example.com external-site.com" + allow_any_instance_of(ActionDispatch::Request).to receive(:referer).and_return('http://www.external-site.com/shop?embedded_shopfront=true') end it "allows iframes on certain pages when enabled in configuration" do get shops_path + expect(response.status).to be 200 expect(response.headers['X-Frame-Options']).to be_nil - expect(response.headers['Content-Security-Policy']).to eq "frame-ancestors test.com" + expect(response.headers['Content-Security-Policy']).to eq "frame-ancestors external-site.com" get spree.admin_path + expect(response.status).to be 200 expect(response.headers['X-Frame-Options']).to eq 'DENY' expect(response.headers['Content-Security-Policy']).to eq "frame-ancestors 'none'" From e173f823c8a6b086e1aec083b6cf6191d4142d5d Mon Sep 17 00:00:00 2001 From: Matt-Yorkley Date: Fri, 5 Jan 2018 21:34:26 +0000 Subject: [PATCH 2/4] Refactor embedded logic --- app/controllers/application_controller.rb | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 7a64aa1776..8501e6743b 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -55,13 +55,11 @@ class ApplicationController < ActionController::Base end def enable_embedded_shopfront - whitelist = Spree::Config[:embedded_shopfronts_whitelist] - domain = embedded_shopfront_referer - return unless Spree::Config[:enable_embedded_shopfronts] && whitelist.present? && domain.present? && whitelist.include?(domain) - return if request.referer && URI(request.referer).scheme != 'https' && !Rails.env.test? && !Rails.env.development? + return unless embeddable? + return if embedding_without_https? response.headers.delete 'X-Frame-Options' - response.headers['Content-Security-Policy'] = "frame-ancestors #{domain}" + response.headers['Content-Security-Policy'] = "frame-ancestors #{embedded_shopfront_referer}" check_embedded_request set_embedded_layout @@ -72,6 +70,16 @@ class ApplicationController < ActionController::Base URI(request.referer).host.sub!(/^www./, '') end + def embeddable? + whitelist = Spree::Config[:embedded_shopfronts_whitelist] + domain = embedded_shopfront_referer + Spree::Config[:enable_embedded_shopfronts] && whitelist.present? && domain.present? && whitelist.include?(domain) + end + + def embedding_without_https? + request.referer && URI(request.referer).scheme != 'https' && !Rails.env.test? && !Rails.env.development? + end + def check_embedded_request return unless params[:embedded_shopfront] From 9506ea456e707ed5c864d43395116c6fc7ed5b58 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley Date: Sat, 6 Jan 2018 15:37:31 +0000 Subject: [PATCH 3/4] Stub the request object instead of controller method --- spec/features/consumer/shopping/embedded_shopfronts_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/features/consumer/shopping/embedded_shopfronts_spec.rb b/spec/features/consumer/shopping/embedded_shopfronts_spec.rb index acac3028df..deb79a5ad2 100644 --- a/spec/features/consumer/shopping/embedded_shopfronts_spec.rb +++ b/spec/features/consumer/shopping/embedded_shopfronts_spec.rb @@ -22,10 +22,10 @@ feature "Using embedded shopfront functionality", js: true do add_variant_to_order_cycle(exchange, variant) Spree::Config[:enable_embedded_shopfronts] = true - Spree::Config[:embedded_shopfronts_whitelist] = 'localhost' + Spree::Config[:embedded_shopfronts_whitelist] = 'test.com' page.driver.browser.js_errors = false - allow_any_instance_of(ApplicationController).to receive(:embedded_shopfront_referer).and_return('localhost') + allow_any_instance_of(ActionDispatch::Request).to receive(:referer).and_return('https://www.test.com') Capybara.current_session.driver.visit('spec/support/views/iframe_test.html') end From 7bd0de99ac49fcbfd0d046322457604905b8bf3f Mon Sep 17 00:00:00 2001 From: Matt-Yorkley Date: Fri, 23 Feb 2018 16:35:46 +0000 Subject: [PATCH 4/4] Adjust brittle referer header check --- app/controllers/application_controller.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 8501e6743b..e19c9bb26b 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -67,7 +67,8 @@ class ApplicationController < ActionController::Base def embedded_shopfront_referer return if request.referer.blank? - URI(request.referer).host.sub!(/^www./, '') + domain = URI(request.referer).host.downcase + domain.start_with?('www.') ? domain[4..-1] : domain end def embeddable?