From e73b37820166901e1bd9ebc2641696a5b6ef61a9 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley Date: Fri, 15 Jun 2018 21:50:44 +0100 Subject: [PATCH 1/4] Adjust embedded response headers --- app/controllers/application_controller.rb | 6 ++++-- spec/requests/embedded_shopfronts_headers_spec.rb | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index d22e8a6ba4..e3ea8b375f 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -60,7 +60,7 @@ class ApplicationController < ActionController::Base return if embedding_without_https? response.headers.delete 'X-Frame-Options' - response.headers['Content-Security-Policy'] = "frame-ancestors #{URI(request.referer).host.downcase}" + response.headers['Content-Security-Policy'] = "frame-ancestors 'self' #{URI(request.referer).host.downcase}" check_embedded_request set_embedded_layout @@ -73,8 +73,10 @@ class ApplicationController < ActionController::Base end def embeddable? - whitelist = Spree::Config[:embedded_shopfronts_whitelist] domain = embedded_shopfront_referer + return true if domain == request.host + + whitelist = Spree::Config[:embedded_shopfronts_whitelist] Spree::Config[:enable_embedded_shopfronts] && whitelist.present? && domain.present? && whitelist.include?(domain) end diff --git a/spec/requests/embedded_shopfronts_headers_spec.rb b/spec/requests/embedded_shopfronts_headers_spec.rb index 9d2c1c523e..6d6ea4b9f6 100644 --- a/spec/requests/embedded_shopfronts_headers_spec.rb +++ b/spec/requests/embedded_shopfronts_headers_spec.rb @@ -52,7 +52,7 @@ describe "setting response headers for embedded shopfronts", type: :request do 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 external-site.com" + expect(response.headers['Content-Security-Policy']).to eq "frame-ancestors 'self' external-site.com" get spree.admin_path @@ -73,7 +73,7 @@ describe "setting response headers for embedded shopfronts", type: :request do 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 www.external-site.com" + expect(response.headers['Content-Security-Policy']).to eq "frame-ancestors 'self' www.external-site.com" end end end From ff0e0d9f3d6461caf8f749942f1e6de607779619 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley Date: Sat, 16 Jun 2018 02:49:22 +0100 Subject: [PATCH 2/4] Move logic from ApplicationController to service and improve clarity --- app/controllers/application_controller.rb | 47 +--------- app/services/embedded_page_service.rb | 90 +++++++++++++++++++ .../embedded_shopfronts_headers_spec.rb | 4 +- 3 files changed, 95 insertions(+), 46 deletions(-) create mode 100644 app/services/embedded_page_service.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index e3ea8b375f..08a552cb6b 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -56,50 +56,9 @@ class ApplicationController < ActionController::Base end def enable_embedded_shopfront - return unless embeddable? - return if embedding_without_https? - - response.headers.delete 'X-Frame-Options' - response.headers['Content-Security-Policy'] = "frame-ancestors 'self' #{URI(request.referer).host.downcase}" - - check_embedded_request - set_embedded_layout - end - - def embedded_shopfront_referer - return if request.referer.blank? - domain = URI(request.referer).host.downcase - domain.start_with?('www.') ? domain[4..-1] : domain - end - - def embeddable? - domain = embedded_shopfront_referer - return true if domain == request.host - - whitelist = Spree::Config[:embedded_shopfronts_whitelist] - 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] - - # Show embedded shopfront CSS - session[:embedded_shopfront] = true - - # Get shopfront slug and set redirect path - if params[:controller] == 'enterprises' && params[:action] == 'shop' && params[:id] - slug = params[:id] - session[:shopfront_redirect] = '/' + slug + '/shop?embedded_shopfront=true' - end - end - - def set_embedded_layout - return unless session[:embedded_shopfront] - @shopfront_layout = 'embedded' + embed_service = EmbeddedPageService.new(params, session, request, response) + embed_service.embed! + @shopfront_layout = 'embedded' if embed_service.use_embedded_layout end def action diff --git a/app/services/embedded_page_service.rb b/app/services/embedded_page_service.rb new file mode 100644 index 0000000000..b4ab2372cc --- /dev/null +++ b/app/services/embedded_page_service.rb @@ -0,0 +1,90 @@ +# Processes requests for pages embedded in iframes + +class EmbeddedPageService + attr_reader :use_embedded_layout + + def initialize(params, session, request, response) + @params = params + @session = session + @request = request + @response = response + + @embedding_domain = @session[:embedding_domain] + @use_embedded_layout = false + end + + def embed! + return unless embeddable? + return if embedding_without_https? + + process_embedded_request + set_response_headers + set_embedded_layout + end + + private + + def embeddable? + return true if current_referer == @request.host + + domain = current_referer_without_www + whitelist = Spree::Config[:embedded_shopfronts_whitelist] + + embedding_enabled? && 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 process_embedded_request + return unless @params[:embedded_shopfront] + + set_embedding_domain + + @session[:embedded_shopfront] = true + set_logout_redirect + end + + def set_response_headers + @response.headers.delete 'X-Frame-Options' + @response.headers['Content-Security-Policy'] = "frame-ancestors 'self' #{@embedding_domain}" + end + + def set_embedding_domain + return unless @params[:embedded_shopfront] + return if current_referer == @request.host + + @embedding_domain = current_referer + @session[:embedding_domain] = current_referer + end + + def set_logout_redirect + return unless enterprise_slug + @session[:shopfront_redirect] = '/' + enterprise_slug + '/shop?embedded_shopfront=true' + end + + def enterprise_slug + return false unless @params[:controller] == 'enterprises' && @params[:action] == 'shop' && @params[:id] + @params[:id] + end + + def current_referer + return if @request.referer.blank? + URI(@request.referer).host.downcase + end + + def current_referer_without_www + return unless current_referer + current_referer.start_with?('www.') ? current_referer[4..-1] : current_referer + end + + def set_embedded_layout + return unless @session[:embedded_shopfront] + @use_embedded_layout = true + end + + def embedding_enabled? + Spree::Config[:enable_embedded_shopfronts] + end +end diff --git a/spec/requests/embedded_shopfronts_headers_spec.rb b/spec/requests/embedded_shopfronts_headers_spec.rb index 6d6ea4b9f6..6428b009be 100644 --- a/spec/requests/embedded_shopfronts_headers_spec.rb +++ b/spec/requests/embedded_shopfronts_headers_spec.rb @@ -48,7 +48,7 @@ describe "setting response headers for embedded shopfronts", type: :request do end it "allows iframes on certain pages when enabled in configuration" do - get shops_path + get enterprise_shop_path(enterprise) + '?embedded_shopfront=true' expect(response.status).to be 200 expect(response.headers['X-Frame-Options']).to be_nil @@ -69,7 +69,7 @@ describe "setting response headers for embedded shopfronts", type: :request do end it "matches the URL structure in the header" do - get shops_path + get enterprise_shop_path(enterprise) + '?embedded_shopfront=true' expect(response.status).to be 200 expect(response.headers['X-Frame-Options']).to be_nil From 6e8187145995cdd88725ad74c57dea9585c91018 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley Date: Wed, 20 Jun 2018 15:14:56 +0100 Subject: [PATCH 3/4] Specs for new EmbeddedPageService --- spec/services/embedded_page_service_spec.rb | 59 +++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 spec/services/embedded_page_service_spec.rb diff --git a/spec/services/embedded_page_service_spec.rb b/spec/services/embedded_page_service_spec.rb new file mode 100644 index 0000000000..f1ea8a971e --- /dev/null +++ b/spec/services/embedded_page_service_spec.rb @@ -0,0 +1,59 @@ +require 'spec_helper' + +describe EmbeddedPageService do + let(:enterprise_slug) { 'test-enterprise' } + let(:params) { { controller: 'enterprises', action: 'shop', id: enterprise_slug, embedded_shopfront: true } } + let(:session) { {} } + let(:request) { ActionController::TestRequest.new('HTTP_HOST' => 'ofn-instance.com', 'HTTP_REFERER' => 'https://embedding-enterprise.com') } + let(:response) { ActionController::TestResponse.new(200, 'X-Frame-Options' => 'DENY', 'Content-Security-Policy' => "frame-ancestors 'none'") } + let(:service) { EmbeddedPageService.new(params, session, request, response) } + + before do + Spree::Config.set( + enable_embedded_shopfronts: true, + embedded_shopfronts_whitelist: 'embedding-enterprise.com example.com' + ) + end + + describe "#embed!" do + context "when the request's referer is in the whitelist" do + before { service.embed! } + + it "sets the response headers to enables embedding requests from the embedding site" do + expect(response.headers).to_not include 'X-Frame-Options' => 'DENY' + expect(response.headers).to include 'Content-Security-Policy' => "frame-ancestors 'self' embedding-enterprise.com" + end + + it "sets session variables" do + expect(session[:embedded_shopfront]).to eq true + expect(session[:embedding_domain]).to eq 'embedding-enterprise.com' + expect(session[:shopfront_redirect]).to eq '/' + enterprise_slug + '/shop?embedded_shopfront=true' + end + end + + context "when embedding is enabled for a different site in the current session" do + before do + session[:embedding_domain] = 'another-enterprise.com' + session[:shopfront_redirect] = '/another-enterprise/shop?embedded_shopfront=true' + service.embed! + end + + it "resets the session variables for the new request" do + expect(session[:embedded_shopfront]).to eq true + expect(session[:embedding_domain]).to eq 'embedding-enterprise.com' + expect(session[:shopfront_redirect]).to eq '/' + enterprise_slug + '/shop?embedded_shopfront=true' + end + end + + context "when the request's referer is not in the whitelist" do + before do + Spree::Config.set(embedded_shopfronts_whitelist: 'example.com') + service.embed! + end + + it "does not enable embedding" do + expect(response.headers['X-Frame-Options']).to eq 'DENY' + end + end + end +end From 172fa168ead0a01be6e623045b46f401c3cf4377 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley Date: Thu, 21 Jun 2018 14:18:32 +0100 Subject: [PATCH 4/4] Change layout attribute to method with question mark --- app/controllers/application_controller.rb | 2 +- app/services/embedded_page_service.rb | 6 ++++-- spec/services/embedded_page_service_spec.rb | 6 +++++- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 08a552cb6b..65d1fb1f33 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -58,7 +58,7 @@ class ApplicationController < ActionController::Base def enable_embedded_shopfront embed_service = EmbeddedPageService.new(params, session, request, response) embed_service.embed! - @shopfront_layout = 'embedded' if embed_service.use_embedded_layout + @shopfront_layout = 'embedded' if embed_service.use_embedded_layout? end def action diff --git a/app/services/embedded_page_service.rb b/app/services/embedded_page_service.rb index b4ab2372cc..8d6e27df26 100644 --- a/app/services/embedded_page_service.rb +++ b/app/services/embedded_page_service.rb @@ -1,8 +1,6 @@ # Processes requests for pages embedded in iframes class EmbeddedPageService - attr_reader :use_embedded_layout - def initialize(params, session, request, response) @params = params @session = session @@ -22,6 +20,10 @@ class EmbeddedPageService set_embedded_layout end + def use_embedded_layout? + @use_embedded_layout + end + private def embeddable? diff --git a/spec/services/embedded_page_service_spec.rb b/spec/services/embedded_page_service_spec.rb index f1ea8a971e..eb44b014ab 100644 --- a/spec/services/embedded_page_service_spec.rb +++ b/spec/services/embedded_page_service_spec.rb @@ -15,7 +15,7 @@ describe EmbeddedPageService do ) end - describe "#embed!" do + describe "processing embedded page requests" do context "when the request's referer is in the whitelist" do before { service.embed! } @@ -29,6 +29,10 @@ describe EmbeddedPageService do expect(session[:embedding_domain]).to eq 'embedding-enterprise.com' expect(session[:shopfront_redirect]).to eq '/' + enterprise_slug + '/shop?embedded_shopfront=true' end + + it "publicly reports that embedded layout should be used" do + expect(service.use_embedded_layout?).to be true + end end context "when embedding is enabled for a different site in the current session" do