Merge pull request #6351 from betagouv/main

app/controllers/application_controller.rb

@@ -2,6 +2,7 @@ class ApplicationController < ActionController::Base
   include TrustedDeviceConcern
   include Pundit
   include Devise::StoreLocationExtension
+  include ApplicationController::LongLivedAuthenticityToken
   include ApplicationController::ErrorHandling
 
   MAINTENANCE_MESSAGE = 'Le site est actuellement en maintenance. Il sera à nouveau disponible dans un court instant.'

app/controllers/application_controller/long_lived_authenticity_token.rb

@@ -0,0 +1,45 @@
+module ApplicationController::LongLivedAuthenticityToken
+  extend ActiveSupport::Concern
+
+  COOKIE_NAME = :_csrf_token
+
+  # Override ActionController::RequestForgeryProtection#real_csrf_token with a
+  # version that reads from an external long-lived cookie (instead of reading from the session).
+  #
+  # See also:
+  # - The Architecture Documentation Record for this choice: docs/adr-csrf-forgery.md
+  # - The Rails issue: https://github.com/rails/rails/issues/21948
+  def real_csrf_token(session) # :doc:
+    # Read the CSRF token from the external long-lived cookie (or generate a new one)
+    #
+    # NB: For retro-compatibility with tokens created before this code was deployed,
+    # also try to read the token from the session.
+
+    csrf_token = cookies.signed[COOKIE_NAME] || session[:_csrf_token] || generate_csrf_token
+
+    # Write the (potentially new) token to an external long-lived cookie.
+    #
+    # NB: for forward-compatibility if we ever remove this code and revert back to session cookies,
+    # also write the token to the session.
+    cookies.signed[COOKIE_NAME] = {
+      value: csrf_token,
+      expires: 1.year.from_now,
+      httponly: true
+    }
+    session[:_csrf_token] = csrf_token
+
+    decode_csrf_token(csrf_token)
+  end
+end
+
+# Clean-up the long-lived cookie if the winning strategy requests so.
+# See:
+# - devise-4.2.0/lib/devise/hooks/csrf_cleaner.rb
+# - http://blog.plataformatec.com.br/2013/08/csrf-token-fixation-attacks-in-devise/
+Warden::Manager.after_authentication do |_record, warden, _options|
+  clean_up_for_winning_strategy = !warden.winning_strategy.respond_to?(:clean_up_csrf?) ||
+    warden.winning_strategy.clean_up_csrf?
+  if Devise.clean_up_csrf_token_on_authentication && clean_up_for_winning_strategy
+    warden.cookies.delete(ApplicationController::LongLivedAuthenticityToken::COOKIE_NAME)
+  end
+end

app/controllers/users/sessions_controller.rb

@@ -77,10 +77,4 @@ class Users::SessionsController < Devise::SessionsController
       redirect_to link_sent_path(email: instructeur.email)
     end
   end
-
-  private
-
-  def handle_unverified_request
-    super
-  end
 end

config/application.rb

@@ -41,18 +41,6 @@ module TPS
     default_allowed_tags = ActionView::Base.sanitized_allowed_tags
     config.action_view.sanitized_allowed_tags = default_allowed_tags + ['u']
 
-    # Some mobile browsers have a behaviour where, although they will delete the session
-    # cookie when the browser shutdowns, they will still serve a cached version
-    # of the page on relaunch.
-    # The CSRF token in the HTML is then mismatched with the CSRF token in the session cookie
-    # (because the session cookie has been cleared). This causes form submissions to fail with
-    # a "ActionController::InvalidAuthenticityToken" exception.
-    # To prevent this, tell browsers to never cache the HTML of a page.
-    # (This doesn’t affect assets files, which are still sent with the proper cache headers).
-    #
-    # See https://github.com/rails/rails/issues/21948
-    config.action_dispatch.default_headers['Cache-Control'] = 'no-store, no-cache'
-
     # ActionDispatch's IP spoofing detection is quite limited, and often rejects
     # legitimate requests from misconfigured proxies (such as mobile telcos).
     #

doc/adr-csrf-forgery.md

@@ -0,0 +1,98 @@
+# Protecting against request forgery using CRSF tokens
+
+## Context
+
+Rails has CSRF protection enabled by default, to protect against POST-based CSRF attacks.
+
+To protect from this, Rails stores two copies of a random token (the so-named CSRF token) on each request:
+- one copy embedded in each HTML page,
+- another copy in the user session.
+
+When performing a POST request, Rails checks that the two copies match – and otherwise denies the request. This protects against an attacker that would generate a form secretly pointing to our website: the attacker can't read the token in the session, and so can't post a form with a valid token.
+
+The problem is that, much more often, this has false positives. There are several cases for that, including:
+
+1. The web browser (often mobile) loads a page containing a form, then is closed by the user. Later, when the browser is re-opened, it restores the page from the cache. But the session cookie has expired, and so is not restored – so the copy of the CSRF token stored in the session is missing. When the user submits the form, they get an "InvalidAuthenticityToken" exception.
+
+2. The user attempts to fill a form, and gets an error message (usually in response to a POST request). They close the browser. When the browser is re-opened, it attempts to restore the page. On Chrome this is blocked by the browser, because the browser denies retrying a (probably non-idempotent) POST request. Safari however happily retries the POST request – but without sending any cookies (in an attempt to avoid having unexpected side-effects). So the copy of the CSRF token in the session is missing (because no cookie was sent), and the user get an "InvalidAuthenticityToken" exception.
+
+## Options considered
+
+### Extend the session cookie duration
+
+We can configure the session cookie to be valid for a longer time (like 2 weeks).
+
+Pros:
+- It solves 1., because when the browser restores the page, the session cookie is still valid.
+
+Cons:
+- Users would be signed-in for a much longer time by default, which has unacceptable security implications.
+- It doesn't solve 2. (because Safari doesn't send any cookie when restoring a page from a POST request)
+
+### Change the cache parameters
+
+We can send a HTTP cache header stating 'Cache-Control: no-store, no-cache'. This instructs the browser to never keep any copy of the page, and to always make a request to the server to restore it.
+
+This solution was attempted during a year in production, and solved 1. – but also introduced another type of InvalidAuthenticityToken errors. In that scenario, the user attempts to fill a form, and gets an error message (usually in response to a POST request). They then navigate on another domain (like France Connect), then hit the "Back" button. Crossing back the domain boundary may cause the browser to either block the request or retry an invalid POST request.
+
+Pros:
+- It solves 1., because on relaunch the browser requests a fresh page again (instead of serving it from its cache), thus retrieving a fresh session and a fresh matching CSRF token.
+
+Cons:
+- It doesn't solve 2.
+- It causes another type of InvalidAuthenticityToken errors.
+
+### Using a null-session strategy
+
+We can change the default protect_from_forgery strategy to :null_session. This makes the current request use an empty session for the request duration.
+
+Pros:
+- It kind of solves 1., by redirecting to a "Please sign-in" page when a stale form is submitted.
+
+Cons:
+- The user is asked to sign-in only after filling and submitting the form, losing their time and data
+- The user will not be redirected to their original page after signing-in
+- It has potential security implications: as the (potentically malicious) request runs anyway, variables cached by a controller before the Null session is created may allow the form submission to succeed anyway (https://www.veracode.com/blog/managing-appsec/when-rails-protectfromforgery-fails)
+
+### Using a reset-session strategy
+
+We can change the default protect_from_forgery strategy to :reset_session. This clears the user session permanently, logging them out until they log in again.
+
+Pros: 
+- It kind of solves 1., by redirecting to a "Please sign-in" page when a stale form is submitted.
+
+Cons:
+- A forgery error in a browser tab will disconnect the user in all its open tabs
+- It has potential security implications: as the (potentically malicious) request runs anyway, variables cached by a controller before the Null session is created may allow the form submission to succeed anyway (https://www.veracode.com/blog/managing-appsec/when-rails-protectfromforgery-fails)
+- It allows an attacker to disconnect an user on demand, which is not only inconvenient, but also has security implication (the attacker could then log the user on it's own attacker account, pretending to be the user account)
+
+### Redirect to login form
+
+When a forgery error occurs, we can instead redirect to the login form.
+
+Pros:
+- It kind of solves 1., by redirecting to a "Please sign-in" page when a stale form is submitted (but the user data is lost).
+- It kind of solves 2., by redirecting to a "Please sign-in" page when a previously POSTed form is reloaded.
+
+Cons:
+- Not all forms require authentication – so for public forms there is no point redirecting to the login form. 
+- The user will not be redirected to their original page after signing-in (because setting the redirect path is a state-changing action, and it is dangerous to let an unauthorized request changing the state – an attacker could control the path where an user is automatically redirected to.)
+- The implementation is finicky, and may introduce security errors. For instance, a naive implementation that catches the exception and redirect_to the sign-in page will prevent Devise from running a cleanup code – which means the user will still be logged, and the CSRF protection is bypassed. However a well-tested implementation that lets Devise code run should avoid these pittfalls.
+
+### Using a long-lived cookie for CSRF tokens
+
+Instead of storing the CSRF token in the session cookie (which is deleted when the browser is closed), we can instead store it in a longer-lived cookie. For this we need to patch Rails.
+
+Pros:
+- It solves 1., because when the user submits a stale form, even if the session cookie because stale, the long-lived CSRF cookie is still valid.
+
+Cons:
+- It doesn't solve 2., because when Safari retries a POST request, it sends none of the cookies (not even long-lived ones).
+- Patching Rails may introduce security issues (now or in the future)
+
+
+## Decision
+
+The only option that fully solves 1. without introducing other issues is the **long-lived CSRF cookie**. This is what we will be using now.
+
+No option solves 2. – but it can be mitigated by a better-looking custom exception page, which we'll also implement.

spec/controllers/application_controller/long_lived_authenticity_token_spec.rb

@@ -0,0 +1,100 @@
+RSpec.describe ApplicationController::LongLivedAuthenticityToken, type: :controller do
+  controller(ActionController::Base) do
+    include ApplicationController::LongLivedAuthenticityToken
+  end
+
+  describe '#real_csrf_token' do
+    subject { controller.send(:real_csrf_token, session) }
+
+    context 'when the long-lived cookie has a token' do
+      before do
+        token = controller.send(:generate_csrf_token)
+
+        @controller.send(:cookies).signed[ApplicationController::LongLivedAuthenticityToken::COOKIE_NAME] = {
+          value: token,
+          expires: 1.year.from_now,
+          httponly: true
+        }
+
+        @decrypted_token = controller.send(:decode_csrf_token, token)
+      end
+
+      it 'returns the decoded token' do
+        expect(subject).to eq @decrypted_token
+      end
+    end
+
+    context 'when the long-lived cookie is empty, but the session has a token' do
+      before do
+        token = controller.send(:generate_csrf_token)
+
+        session[:_csrf_token] = token
+
+        @decrypted_token = controller.send(:decode_csrf_token, token)
+      end
+
+      it 'returns the decoded token' do
+        expect(subject).to eq @decrypted_token
+      end
+    end
+
+    context 'when no token is present' do
+      it 'generates a new token' do
+        expect(subject).to be_present
+      end
+
+      it 'stores the new token in the long-lived cookie' do
+        subject
+        expect(controller.send(:cookies).signed[ApplicationController::LongLivedAuthenticityToken::COOKIE_NAME]).to be_present
+      end
+
+      it 'stores the new token in the session' do
+        subject
+        expect(controller.session[:_csrf_token]).to be_present
+      end
+    end
+  end
+end
+
+RSpec.describe "CSRF cleanup", type: :request do
+  describe 'csrf_cleaner hook', :allow_forgery_protection do
+    let(:user) { create(:user, password: password) }
+    let(:password) { 'my-very-secure-password' }
+
+    it 'refreshes the long-lived cookie after authentication' do
+      get new_user_session_path
+      cookie_token = long_lived_cookie
+
+      # The token in the long-lived cookie doesn't change between requests
+      # (This is not strictly needed, but ensures we read the signed cookie properly.)
+      get new_user_session_path
+
+      expect(long_lived_cookie).to be_present
+      expect(long_lived_cookie).to eq cookie_token
+
+      # The token in the long-lived cookie is refreshed after authentication
+      post user_session_path,
+           params: { user: { email: user.email, password: password } },
+           headers: { 'HTTP_X_CSRF_TOKEN' => header_authenticity_token(response) }
+      follow_redirect!
+      follow_redirect! # After sign-in, we are redirected twice
+
+      expect(response).to have_http_status(200)
+      expect(long_lived_cookie).to be_present
+      expect(long_lived_cookie).not_to eq cookie_token
+    end
+  end
+
+  private
+
+  def header_authenticity_token(response)
+    regex = /meta name="csrf-token" content="(?<token>.+)"/
+    parts = response.body.match(regex)
+    parts['token'] if parts
+  end
+
+  def long_lived_cookie
+    jar = ActionDispatch::Cookies::CookieJar.build(request, cookies.to_hash)
+    jar.signed[ApplicationController::LongLivedAuthenticityToken::COOKIE_NAME.to_s]
+  end
+end

spec/features/forgery_spec.rb

@@ -0,0 +1,56 @@
+feature 'Protecting against request forgeries:', :allow_forgery_protection, :show_exception_pages do
+  let(:user) { create(:user, password: password) }
+  let(:password) { 'ThisIsTheUserPassword' }
+
+  before do
+    visit new_user_session_path
+  end
+
+  context 'when the browser send a request after the session cookie expired' do
+    before do
+      delete_session_cookie
+    end
+
+    context 'when the long-lived CSRF cookie is still present' do
+      scenario 'the change is allowed' do
+        fill_sign_in_form
+        click_on 'Se connecter'
+        expect(page).to have_content('Connecté')
+      end
+    end
+
+    context 'when the long-lived CSRF cookie is invalid or missing' do
+      before do
+        delete_long_lived_csrf_cookie
+      end
+
+      scenario 'the user sees an error page' do
+        fill_sign_in_form
+        click_on 'Se connecter'
+        expect(page).to have_text('L’action demandée a été rejetée')
+      end
+    end
+  end
+
+  private
+
+  def fill_sign_in_form
+    fill_in :user_email, with: user.email
+    fill_in :user_password, with: password
+  end
+
+  def delete_session_cookie
+    session_cookie_name = Rails.application.config.session_options[:key]
+    delete_cookie(session_cookie_name)
+  end
+
+  def delete_long_lived_csrf_cookie
+    csrf_cookie_name = ApplicationController::LongLivedAuthenticityToken::COOKIE_NAME
+    delete_cookie(csrf_cookie_name)
+  end
+
+  def delete_cookie(cookie_name)
+    raise 'The cookie to be deleted can’t be nil' if cookie_name.nil?
+    page.driver.browser.set_cookie("#{cookie_name}=''")
+  end
+end

spec/rails_helper.rb

@@ -93,6 +93,36 @@ RSpec.configure do |config|
     Flipper.enable(:instructeur_bypass_email_login_token)
   end
 
+  # By default, forgery protection is disabled in the test environment.
+  # (See `config.action_controller.allow_forgery_protection` in `config/test.rb`)
+  #
+  # Examples tagged with the :allow_forgery_protection have the forgery protection enabled anyway.
+  config.around(:each, :allow_forgery_protection) do |example|
+    previous_allow_forgery_protection = ActionController::Base.allow_forgery_protection
+    ActionController::Base.allow_forgery_protection = true
+    begin
+      example.call
+    ensure
+      ActionController::Base.allow_forgery_protection = previous_allow_forgery_protection
+    end
+  end
+
+  # By default, the default HTML templates for exceptions are not rendered in the test environment.
+  # (See `config.action_dispatch.show_exceptions` in `config/test.rb`)
+  #
+  # Examples tagged with the :show_exception_pages render the exception HTML page anyway.
+  config.around(:each, :show_exception_pages) do |example|
+    app = Rails.application
+    previous_show_exceptions = app.env_config['action_dispatch.show_exceptions'] || app.config.action_dispatch.show_exceptions
+
+    begin
+      app.env_config['action_dispatch.show_exceptions'] = true
+      example.call
+    ensure
+      app.env_config['action_dispatch.show_exceptions'] = previous_show_exceptions
+    end
+  end
+
   config.include Shoulda::Matchers::ActiveRecord, type: :model
   config.include Shoulda::Matchers::ActiveModel, type: :model
   config.include Devise::Test::ControllerHelpers, type: :controller