From ff8e47acb5168c49c1d1c48b06e657dc1da92cde Mon Sep 17 00:00:00 2001 From: Moncef Belyamani Date: Wed, 16 May 2018 22:39:16 -0400 Subject: [PATCH] Update secure_headers from 3.7.3 to 6.0.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previous versions of `secure_headers` obscured a bug that should have caused the spec on line 79 of `spec/features/users/sign_up_spec.rb` to fail. Before version 6.0.0, the gem kept track of which CSP directives weren't supported in which browsers, and parsed the user agent and overrode the app's CSP config to remove any directives that it thought were not supported. With the user agent parsing code, the `HeadlessChrome` User Agent was being interpreted by the `secure_headers` gem as a browser that does not support the `form-action` directive, which allowed the aforementioned spec to pass (because the `form-action` directive was removed from the CSP headers). I verified this using the middleware linked to in this [blog post](https://about.gitlab.com/2017/12/19/moving-to-headless-chrome/), which allows us to inspect the response headers (Selenium doesn't have support for `page.response_headers`). In the latest version of `secure_headers`, the `form-action` directive is not removed (which is a good thing), and the reason the test fails is because this is a JS test, and we have configured all JS tests to use `127.0.0.1` as the domain name, but in the last part of the test, when the user clicks on the button in the modal to cancel account creation and delete their account, it tries to go to `http://www.example.com/sign_up/start`. This is because the `UsersController` determines where to redirect based on the `cancel_link_url` of the `decorated_session`, which in this case is the `ServiceProviderDecoratedSession`, whose `cancel_link_url` was constructed using `Rails.application.routes.url_helpers`, which, for some reason, ignores the `default_url_options` defined in `ApplicationController` and defaults the host to `www.example.com`, and since the `form-action` directive doesn't allow `example.com`, the test fails to redirect. Note that the defaulting to `www.example.com` only happens in the test environment. I verified this works fine in production. I verified this by replacing `redirect_to url_after_cancellation` with `redirect_to sign_up_start_url` in `UsersController`, which allowed the test to pass. So then I tried passing in the `host` parameter to `sign_up_start_url` in the decorator, but that didn't work because Capybara uses a port with `127.0.0.1`, but the `sign_up_start_url` in the decorator doesn't include the port, and so the `form-action` directive will not allow this redirect. The port is random each time the test is run, so there's no obvious way to set the port, and `Capybara.always_include_port = false` didn't work. So then I looked at the decorator class some more, and noticed the `view_context` that gets passed to it and it struck me: we should be using `view_context.sign_up_start_url` instead of including `Rails.application.routes.url_helpers`, and 🎉! --- Gemfile | 2 +- Gemfile.lock | 6 ++---- .../service_provider_session_decorator.rb | 5 +---- app/decorators/session_decorator.rb | 11 ++++++++--- app/services/decorated_session.rb | 2 +- .../service_provider_session_decorator_spec.rb | 16 ++++++---------- spec/decorators/session_decorator_spec.rb | 8 ++++++-- spec/features/users/sign_up_spec.rb | 2 +- spec/support/features/session_helper.rb | 2 +- .../views/devise/passwords/new.html.slim_spec.rb | 3 +++ .../sign_up/registrations/new.html.slim_spec.rb | 1 + 11 files changed, 31 insertions(+), 27 deletions(-) diff --git a/Gemfile b/Gemfile index 0443d2b3269..8d05d9dedb1 100644 --- a/Gemfile +++ b/Gemfile @@ -49,7 +49,7 @@ gem 'saml_idp', git: 'https://github.com/18F/saml_idp.git', tag: 'v0.7.0-18f' gem 'sass-rails', '~> 5.0' gem 'savon' gem 'scrypt' -gem 'secure_headers', '~> 3.0' +gem 'secure_headers', '~> 6.0' gem 'sidekiq' gem 'simple_form' gem 'sinatra', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 7d228f03869..05391674335 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -543,8 +543,7 @@ GEM wasabi (~> 3.4) scrypt (3.0.5) ffi-compiler (>= 1.0, < 2.0) - secure_headers (3.7.3) - useragent + secure_headers (6.0.0) selenium-webdriver (3.11.0) childprocess (~> 0.5) rubyzip (~> 1.2) @@ -626,7 +625,6 @@ GEM unicode-display_width (1.3.0) uniform_notifier (1.11.0) user_agent_parser (2.4.1) - useragent (0.16.8) uuid (2.3.9) macaddr (~> 1.0) valid_email (0.1.0) @@ -754,7 +752,7 @@ DEPENDENCIES sass-rails (~> 5.0) savon scrypt - secure_headers (~> 3.0) + secure_headers (~> 6.0) shoulda-matchers (~> 3.0) sidekiq simple_form diff --git a/app/decorators/service_provider_session_decorator.rb b/app/decorators/service_provider_session_decorator.rb index 8ca638d4acf..366a5353906 100644 --- a/app/decorators/service_provider_session_decorator.rb +++ b/app/decorators/service_provider_session_decorator.rb @@ -1,7 +1,4 @@ class ServiceProviderSessionDecorator - include Rails.application.routes.url_helpers - include LocaleHelper - DEFAULT_LOGO = 'generic.svg'.freeze SP_ALERTS = { @@ -97,7 +94,7 @@ def sp_return_url end def cancel_link_url - sign_up_start_url(request_id: sp_session[:request_id], locale: locale_url_param) + view_context.sign_up_start_url(request_id: sp_session[:request_id]) end def sp_alert? diff --git a/app/decorators/session_decorator.rb b/app/decorators/session_decorator.rb index c161d8ddf41..95e1dece91b 100644 --- a/app/decorators/session_decorator.rb +++ b/app/decorators/session_decorator.rb @@ -1,6 +1,7 @@ class SessionDecorator - include Rails.application.routes.url_helpers - include LocaleHelper + def initialize(view_context: nil) + @view_context = view_context + end def return_to_service_provider_partial 'shared/null' @@ -31,7 +32,7 @@ def idv_hardfail4_partial end def cancel_link_url - root_url(locale: locale_url_param) + view_context.root_url end def sp_name; end @@ -51,4 +52,8 @@ def sp_alert?; end def sp_alert_name; end def sp_alert_learn_more; end + + private + + attr_reader :view_context end diff --git a/app/services/decorated_session.rb b/app/services/decorated_session.rb index 5a51d68fee7..4d69b7295e5 100644 --- a/app/services/decorated_session.rb +++ b/app/services/decorated_session.rb @@ -15,7 +15,7 @@ def call service_provider_request: service_provider_request ) else - SessionDecorator.new + SessionDecorator.new(view_context: view_context) end end diff --git a/spec/decorators/service_provider_session_decorator_spec.rb b/spec/decorators/service_provider_session_decorator_spec.rb index 083c52d1f81..8fcb26f208e 100644 --- a/spec/decorators/service_provider_session_decorator_spec.rb +++ b/spec/decorators/service_provider_session_decorator_spec.rb @@ -136,18 +136,14 @@ ) end - it 'returns sign_up_start_url with the request_id as a param' do - expect(decorator.cancel_link_url). - to eq 'http://www.example.com/sign_up/start?request_id=foo' + before do + allow(view_context).to receive(:sign_up_start_url). + and_return('https://www.example.com/sign_up/start') end - context 'in another language' do - before { I18n.locale = :fr } - - it 'keeps the language' do - expect(decorator.cancel_link_url). - to eq 'http://www.example.com/fr/sign_up/start?request_id=foo' - end + it 'returns view_context.sign_up_start_url' do + expect(decorator.cancel_link_url). + to eq 'https://www.example.com/sign_up/start' end end end diff --git a/spec/decorators/session_decorator_spec.rb b/spec/decorators/session_decorator_spec.rb index c032abe3c71..34d145ca63f 100644 --- a/spec/decorators/session_decorator_spec.rb +++ b/spec/decorators/session_decorator_spec.rb @@ -62,8 +62,12 @@ end describe '#cancel_link_url' do - it 'returns root url' do - expect(subject.cancel_link_url).to eq 'http://www.example.com/' + it 'returns view_context.root url' do + view_context = ActionController::Base.new.view_context + allow(view_context).to receive(:root_url).and_return('http://www.example.com') + decorator = SessionDecorator.new(view_context: view_context) + + expect(decorator.cancel_link_url).to eq 'http://www.example.com' end end end diff --git a/spec/features/users/sign_up_spec.rb b/spec/features/users/sign_up_spec.rb index a2495ab7652..68067da0816 100644 --- a/spec/features/users/sign_up_spec.rb +++ b/spec/features/users/sign_up_spec.rb @@ -82,7 +82,7 @@ click_on t('links.cancel') click_on t('sign_up.buttons.cancel') - expect(page).to have_current_path(sign_up_start_path) + expect(page).to have_current_path(sign_up_start_path(request_id: '123')) expect { User.find(user.id) }.to raise_error ActiveRecord::RecordNotFound end end diff --git a/spec/support/features/session_helper.rb b/spec/support/features/session_helper.rb index f492c2678d0..7a5b1ce9c18 100644 --- a/spec/support/features/session_helper.rb +++ b/spec/support/features/session_helper.rb @@ -50,7 +50,7 @@ def begin_sign_up_with_sp_and_loa(loa3:) Warden.on_next_request do |proxy| session = proxy.env['rack.session'] sp = ServiceProvider.from_issuer('http://localhost:3000') - session[:sp] = { loa3: loa3, issuer: sp.issuer } + session[:sp] = { loa3: loa3, issuer: sp.issuer, request_id: '123' } end visit account_path diff --git a/spec/views/devise/passwords/new.html.slim_spec.rb b/spec/views/devise/passwords/new.html.slim_spec.rb index a2ed8630266..cc3521e89b0 100644 --- a/spec/views/devise/passwords/new.html.slim_spec.rb +++ b/spec/views/devise/passwords/new.html.slim_spec.rb @@ -9,6 +9,9 @@ return_to_sp_url: 'www.awesomeness.com' ) view_context = ActionController::Base.new.view_context + allow(view_context).to receive(:sign_up_start_url). + and_return('https://www.example.com/sign_up/start') + @decorated_session = DecoratedSession.new( sp: @sp, view_context: view_context, diff --git a/spec/views/sign_up/registrations/new.html.slim_spec.rb b/spec/views/sign_up/registrations/new.html.slim_spec.rb index e4fbf730107..54ed039d27a 100644 --- a/spec/views/sign_up/registrations/new.html.slim_spec.rb +++ b/spec/views/sign_up/registrations/new.html.slim_spec.rb @@ -12,6 +12,7 @@ sp: nil, view_context: view_context, sp_session: {}, service_provider_request: nil ).call allow(view).to receive(:decorated_session).and_return(@decorated_session) + allow(view_context).to receive(:root_url).and_return('http://www.example.com') end it 'has a localized title' do