From e70756c4d3ecfdcc0d8f0be9bf3f6aa1f72d4629 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 1 Apr 2021 14:20:59 +0100 Subject: [PATCH 1/2] Sign in helper will take every user past the root path Instead of starting on the dashboard_path, we follow the real user journey of hitting our landing page first. Before this, visiting the dashbaord would redirect the user to auth/dfe with a GET request. This needed to change due to a security vulnerability to become a POST request. This redirection then becomes impossible. Despite the vulnerability, changing to visit the root_path first in most feature tests makes sense for complete end-to-end coverage. --- spec/support/sign_in_helpers.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/support/sign_in_helpers.rb b/spec/support/sign_in_helpers.rb index 706c052ef..4f903547e 100644 --- a/spec/support/sign_in_helpers.rb +++ b/spec/support/sign_in_helpers.rb @@ -13,7 +13,8 @@ def user_sign_in_attempt_fails(dsi_uid: SecureRandom.uuid) end def user_starts_the_journey - visit dashboard_path + visit root_path + click_link I18n.t("generic.button.start") click_link I18n.t("dashboard.create.link") end From 3d435798f3a73e7caf6e89c09b94945ced881c08 Mon Sep 17 00:00:00 2001 From: Tom Hipkin Date: Thu, 1 Apr 2021 14:28:53 +0100 Subject: [PATCH 2/2] Resolve CSRF vulnerability with Omniauth We change the link_to to a button_to to change the sign in button from making a GET request to a POST request. The POST request will be protected by Rails CSRF protection to prevent malicious requests from signing in when they shouldn't. https://github.com/omniauth/omniauth/wiki/Resolving-CVE-2015-9284 Remove now unused sessions resource and redirect/link to `/auth/dfe`directly. If a user becomes unauthenticated we change where they go. Instead of automatically taking them to the sign in page - through a GET request - we link them back to the root_path so that they may click "Sign-in" with the safe POST. This makes it safer, though slightly more inconvienent for the user. We could add flash messages in future. --- Gemfile | 1 + Gemfile.lock | 4 ++++ app/controllers/application_controller.rb | 2 +- app/controllers/sessions_controller.rb | 5 ----- app/views/pages/specifying_start_page.html.erb | 4 ++-- config/routes.rb | 6 ------ .../anyone_can_sign_in_with_dfe_sign_in_spec.rb | 3 ++- spec/requests/authentication_spec.rb | 12 ++++++------ 8 files changed, 16 insertions(+), 21 deletions(-) diff --git a/Gemfile b/Gemfile index f3b6ba298..a38027de5 100644 --- a/Gemfile +++ b/Gemfile @@ -18,6 +18,7 @@ gem "pg" gem "mini_racer" gem "omniauth" gem "omniauth_openid_connect" +gem "omniauth-rails_csrf_protection" gem "puma", "~> 5.2" gem "redcarpet", "~> 3.5" gem "redis", "~> 4.2" diff --git a/Gemfile.lock b/Gemfile.lock index 57b635830..47c8f9ba6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -203,6 +203,9 @@ GEM omniauth (1.9.1) hashie (>= 3.4.6) rack (>= 1.6.2, < 3) + omniauth-rails_csrf_protection (0.1.2) + actionpack (>= 4.2) + omniauth (>= 1.3.1) omniauth_openid_connect (0.3.5) addressable (~> 2.5) omniauth (~> 1.9) @@ -426,6 +429,7 @@ DEPENDENCIES mini_racer mock_redis omniauth + omniauth-rails_csrf_protection omniauth_openid_connect pg pry diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index fa5a4fd58..89e441516 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -27,7 +27,7 @@ def authenticate_user! return if current_user session.delete(:dfe_sign_in_uid) - redirect_to new_dfe_path + redirect_to root_path end def current_journey diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 5e1699776..036fa8ae7 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -4,11 +4,6 @@ class SessionsController < ApplicationController skip_before_action :authenticate_user! protect_from_forgery except: :bypass_callback - def new - # This is defined by the class name of our Omniauth strategy - redirect_to "/auth/dfe" - end - def create UserSession.new(session: session) .persist_successful_dfe_sign_in_claim!(omniauth_hash: auth_hash) diff --git a/app/views/pages/specifying_start_page.html.erb b/app/views/pages/specifying_start_page.html.erb index 9de73becc..111897048 100644 --- a/app/views/pages/specifying_start_page.html.erb +++ b/app/views/pages/specifying_start_page.html.erb @@ -37,7 +37,7 @@

<%= I18n.t("specifying.start_page.pause_and_resume_body") %>

<% if DfESignIn.bypass? %> - <%= link_to "#{I18n.t("generic.button.start")} (bypass DSI)", "/auth/developer", class: "govuk-button" %> + <%= button_to "#{I18n.t("generic.button.start")} (bypass DSI)", "/auth/developer", class: "govuk-button" %> <% else %> - <%= link_to I18n.t("generic.button.start"), new_dfe_path, class: "govuk-button" %> + <%= button_to I18n.t("generic.button.start"), "/auth/dfe", class: "govuk-button" %> <% end %> diff --git a/config/routes.rb b/config/routes.rb index ebc47537a..5e0b32555 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -8,12 +8,6 @@ post "/api/contentful/entry_updated" => "api/contentful/entries#changed" # DfE Sign In - resource :sessions, - only: %i[create new], - as: :dfe, - path: "/dfe/sessions", - controller: "sessions" - get "/auth/dfe/callback", to: "sessions#create" get "/auth/dfe/signout", to: "sessions#destroy" get "/auth/failure", to: "sessions#failure" diff --git a/spec/features/visitors/anyone_can_sign_in_with_dfe_sign_in_spec.rb b/spec/features/visitors/anyone_can_sign_in_with_dfe_sign_in_spec.rb index 95a7f73db..92f7f8f6f 100644 --- a/spec/features/visitors/anyone_can_sign_in_with_dfe_sign_in_spec.rb +++ b/spec/features/visitors/anyone_can_sign_in_with_dfe_sign_in_spec.rb @@ -58,7 +58,8 @@ .with("Sign in failed unexpectedly") .and_call_original - visit dashboard_path + visit root_path + click_button I18n.t("generic.button.start") expect(page).to have_content(I18n.t("errors.sign_in.unexpected_failure.page_title")) expect(page).to have_content(I18n.t("errors.sign_in.unexpected_failure.page_body")) diff --git a/spec/requests/authentication_spec.rb b/spec/requests/authentication_spec.rb index 6c238e818..f58d705d5 100644 --- a/spec/requests/authentication_spec.rb +++ b/spec/requests/authentication_spec.rb @@ -18,7 +18,7 @@ end it "users can access the new session endpoint" do - get new_dfe_path + post "/auth/dfe" expect(response).to have_http_status(:found) end @@ -36,29 +36,29 @@ describe "Endpoints that do require authentication" do it "users cannot access the new journey path" do get new_journey_path - expect(response).to redirect_to(new_dfe_path) + expect(response).to redirect_to(root_path) end it "users cannot access an existing journey" do journey = create(:journey) get journey_path(journey) - expect(response).to redirect_to(new_dfe_path) + expect(response).to redirect_to(root_path) end it "users cannot edit an answer" do answer = create(:radio_answer) get edit_journey_step_path(answer.step.journey, answer.step) - expect(response).to redirect_to(new_dfe_path) + expect(response).to redirect_to(root_path) end it "users cannot see the journey map" do get new_journey_map_path - expect(response).to redirect_to(new_dfe_path) + expect(response).to redirect_to(root_path) end it "users cannot see the preview endpoints" do get preview_entry_path("an-entry-id") - expect(response).to redirect_to(new_dfe_path) + expect(response).to redirect_to(root_path) end end