From 09933454ffa2a8b625d8684af17618a48de0a222 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Tue, 6 Jul 2021 10:34:23 +0000 Subject: [PATCH 1/3] app: improve InvalidAuthenticityToken logging - Log on all controllers - Improve description of the controller action involved - Ignore Safari bogus requests --- app/controllers/application_controller.rb | 1 + .../application_controller/error_handling.rb | 29 +++++++++++ app/controllers/users/sessions_controller.rb | 12 ----- .../error_handling_spec.rb | 48 +++++++++++++++++++ 4 files changed, 78 insertions(+), 12 deletions(-) create mode 100644 app/controllers/application_controller/error_handling.rb create mode 100644 spec/controllers/application_controller/error_handling_spec.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index f8ba6a9be..b89fc6fdf 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -2,6 +2,7 @@ class ApplicationController < ActionController::Base include TrustedDeviceConcern include Pundit include Devise::StoreLocationExtension + include ApplicationController::ErrorHandling MAINTENANCE_MESSAGE = 'Le site est actuellement en maintenance. Il sera à nouveau disponible dans un court instant.' diff --git a/app/controllers/application_controller/error_handling.rb b/app/controllers/application_controller/error_handling.rb new file mode 100644 index 000000000..fadba01f2 --- /dev/null +++ b/app/controllers/application_controller/error_handling.rb @@ -0,0 +1,29 @@ +module ApplicationController::ErrorHandling + extend ActiveSupport::Concern + + included do + rescue_from ActionController::InvalidAuthenticityToken do + if cookies.count == 0 + # When some browsers (like Safari) re-open a previously closed tab, they attempts + # to reload the page – even if it is a POST request. But in that case, they don’t + # sends any of the cookies. + # + # Ignore this error. + render plain: "Les cookies doivent être activés pour utiliser #{APPLICATION_NAME}.", status: 403 + else + log_invalid_authenticity_token_error + raise # propagate the exception up, to render the default exception page + end + end + end + + def log_invalid_authenticity_token_error + Sentry.with_scope do |temp_scope| + tags = { + action: "#{self.class.name}#{action_name}" + } + temp_scope.set_tags(tags) + Sentry.capture_message("ActionController::InvalidAuthenticityToken") + end + end +end diff --git a/app/controllers/users/sessions_controller.rb b/app/controllers/users/sessions_controller.rb index e6190313a..75abdf80a 100644 --- a/app/controllers/users/sessions_controller.rb +++ b/app/controllers/users/sessions_controller.rb @@ -81,18 +81,6 @@ class Users::SessionsController < Devise::SessionsController private def handle_unverified_request - log_invalid_authenticity_token_error super end - - def log_invalid_authenticity_token_error - Sentry.with_scope do |temp_scope| - tags = { - request_tokens: request_authenticity_tokens.compact.map { |t| t.gsub(/.....$/, '*****') }.join(', '), - session_token: session[:_csrf_token]&.gsub(/.....$/, '*****') - } - temp_scope.set_tags(tags) - Sentry.capture_message("ActionController::InvalidAuthenticityToken in Users::SessionsController") - end - end end diff --git a/spec/controllers/application_controller/error_handling_spec.rb b/spec/controllers/application_controller/error_handling_spec.rb new file mode 100644 index 000000000..d510e8e07 --- /dev/null +++ b/spec/controllers/application_controller/error_handling_spec.rb @@ -0,0 +1,48 @@ +RSpec.describe ApplicationController::ErrorHandling, type: :controller do + controller(ActionController::Base) do + include ApplicationController::ErrorHandling + + def invalid_authenticity_token + raise ActionController::InvalidAuthenticityToken + end + end + + before do + routes.draw { post 'invalid_authenticity_token' => 'anonymous#invalid_authenticity_token' } + end + + describe 'handling ActionController::InvalidAuthenticityToken' do + let(:request_cookies) do + { 'some_cookie': true } + end + + before { cookies.update(request_cookies) } + + it 'logs the error' do + allow(Sentry).to receive(:capture_message) + post :invalid_authenticity_token rescue nil + expect(Sentry).to have_received(:capture_message) + end + + it 'forwards the error upwards' do + expect { post :invalid_authenticity_token }.to raise_error(ActionController::InvalidAuthenticityToken) + end + + context 'when Safari retries a POST request without cookies' do + let(:request_cookies) do + {} + end + + it 'returns a message' do + post :invalid_authenticity_token + + expect(response).to have_http_status(403) + expect(response.body).to include('cookies') + end + + it 'renders the standard exception page' do + expect { post :invalid_authenticity_token }.not_to raise_error + end + end + end +end From 0ce708028d58a581f3d2b0b03af7074fc284cf51 Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Tue, 6 Jul 2021 15:06:38 +0200 Subject: [PATCH 2/3] Prevent crashes in combo boxes --- app/javascript/components/shared/hooks.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/javascript/components/shared/hooks.js b/app/javascript/components/shared/hooks.js index 0111550cd..a64aca7c5 100644 --- a/app/javascript/components/shared/hooks.js +++ b/app/javascript/components/shared/hooks.js @@ -4,7 +4,7 @@ export function useDeferredSubmit(input) { const calledRef = useRef(false); const awaitFormSubmit = useCallback( (callback) => { - const form = input.form; + const form = input?.form; if (!form) { return; } From bc07a875eb3108b623ad61481735fcc951eb96f3 Mon Sep 17 00:00:00 2001 From: Christophe Robillard Date: Tue, 6 Jul 2021 15:00:33 +0200 Subject: [PATCH 3/3] integrate a mininum weight for the average dossier weight before this commit, the average dossier weight took account only pieces justificatives. With this commit, we add a minimum weight for other files included in an archive like pdf_export, log operations, attachments added to traitements. This minimum weight is set arbitrary, from the observation of some random procedures in production --- app/models/procedure.rb | 3 ++- spec/models/procedure_spec.rb | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/models/procedure.rb b/app/models/procedure.rb index e02100f4b..823badc02 100644 --- a/app/models/procedure.rb +++ b/app/models/procedure.rb @@ -55,6 +55,7 @@ class Procedure < ApplicationRecord MAX_DUREE_CONSERVATION = 36 MAX_DUREE_CONSERVATION_EXPORT = 3.hours + MIN_WEIGHT = 350000 has_many :revisions, -> { order(:id) }, class_name: 'ProcedureRevision', inverse_of: :procedure belongs_to :draft_revision, class_name: 'ProcedureRevision', optional: false belongs_to :published_revision, class_name: 'ProcedureRevision', optional: true @@ -684,7 +685,7 @@ class Procedure < ApplicationRecord .where(type: Champs::PieceJustificativeChamp.to_s, dossier: dossiers_sample) .sum('active_storage_blobs.byte_size') - total_size / dossiers_sample.length + MIN_WEIGHT + total_size / dossiers_sample.length else nil end diff --git a/spec/models/procedure_spec.rb b/spec/models/procedure_spec.rb index b6b8f7cf6..a24ad648a 100644 --- a/spec/models/procedure_spec.rb +++ b/spec/models/procedure_spec.rb @@ -1026,7 +1026,7 @@ describe Procedure do end it 'estimates average dossier weight' do - expect(procedure.reload.average_dossier_weight).to eq 5 + expect(procedure.reload.average_dossier_weight).to eq(5 + Procedure::MIN_WEIGHT) end end