From cccb04d72588de8320c690f8bd0aa546f1fc3a48 Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Wed, 20 Nov 2019 16:03:40 +0100 Subject: [PATCH 1/3] ActiveStorage url should expire after an hour --- app/models/concerns/blob_signed_id_concern.rb | 12 ++++++++++++ ...irus_scanner.rb => blob_virus_scanner_concern.rb} | 2 +- config/initializers/active_storage.rb | 5 ++++- 3 files changed, 17 insertions(+), 2 deletions(-) create mode 100644 app/models/concerns/blob_signed_id_concern.rb rename app/models/concerns/{blob_virus_scanner.rb => blob_virus_scanner_concern.rb} (95%) diff --git a/app/models/concerns/blob_signed_id_concern.rb b/app/models/concerns/blob_signed_id_concern.rb new file mode 100644 index 000000000..64b981993 --- /dev/null +++ b/app/models/concerns/blob_signed_id_concern.rb @@ -0,0 +1,12 @@ +module BlobSignedIdConcern + extend ActiveSupport::Concern + + included do + # We override signed_id to add `expires_in` option to generated hash. + # This is a measure to ensure that we never under any circumstance + # expose permanent attachment url + def signed_id + ActiveStorage.verifier.generate(id, purpose: :blob_id, expires_in: ActiveStorage::Service.url_expires_in) + end + end +end diff --git a/app/models/concerns/blob_virus_scanner.rb b/app/models/concerns/blob_virus_scanner_concern.rb similarity index 95% rename from app/models/concerns/blob_virus_scanner.rb rename to app/models/concerns/blob_virus_scanner_concern.rb index c6b0966c9..80fb3151a 100644 --- a/app/models/concerns/blob_virus_scanner.rb +++ b/app/models/concerns/blob_virus_scanner_concern.rb @@ -2,7 +2,7 @@ # (rather than on blob creation). # This will help to avoid cloberring metadata accidentally (as metadata # are more stable on attachment creation than on blob creation). -module BlobVirusScanner +module BlobVirusScannerConcern extend ActiveSupport::Concern included do diff --git a/config/initializers/active_storage.rb b/config/initializers/active_storage.rb index dd4d57d48..e20e48620 100644 --- a/config/initializers/active_storage.rb +++ b/config/initializers/active_storage.rb @@ -6,7 +6,10 @@ ActiveStorage::Service.url_expires_in = 1.hour # Rails 6 adds support for `.on_load(:active_storage_attachment)`, which is # cleaner (as it allows to enqueue the virus scan on attachment creation, rather # than on blob creation). -ActiveSupport.on_load(:active_storage_blob) { include BlobVirusScanner } +ActiveSupport.on_load(:active_storage_blob) do + include BlobSignedIdConcern + include BlobVirusScannerConcern +end # When an OpenStack service is initialized it makes a request to fetch # `publicURL` to use for all operations. We intercept the method that reads From 785a09b326a5c5919d6d93d724efd949ecf532df Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Thu, 19 Dec 2019 18:15:37 +0100 Subject: [PATCH 2/3] Use service_url instead of blob_url on secure attachments --- app/controllers/instructeurs/dossiers_controller.rb | 2 +- app/controllers/users/dossiers_controller.rb | 2 +- app/graphql/types/file.rb | 2 +- app/models/champs/piece_justificative_champ.rb | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/instructeurs/dossiers_controller.rb b/app/controllers/instructeurs/dossiers_controller.rb index caa08f48a..06c6e5ac4 100644 --- a/app/controllers/instructeurs/dossiers_controller.rb +++ b/app/controllers/instructeurs/dossiers_controller.rb @@ -15,7 +15,7 @@ module Instructeurs def attestation if dossier.attestation.pdf.attached? - redirect_to url_for(dossier.attestation.pdf) + redirect_to dossier.attestation.pdf.service_url end end diff --git a/app/controllers/users/dossiers_controller.rb b/app/controllers/users/dossiers_controller.rb index 2baaab0fe..df9204e89 100644 --- a/app/controllers/users/dossiers_controller.rb +++ b/app/controllers/users/dossiers_controller.rb @@ -49,7 +49,7 @@ module Users def attestation if dossier.attestation&.pdf&.attached? - redirect_to url_for(dossier.attestation.pdf) + redirect_to dossier.attestation.pdf.service_url else flash.notice = "L'attestation n'est plus disponible sur ce dossier." redirect_to dossier_path(dossier) diff --git a/app/graphql/types/file.rb b/app/graphql/types/file.rb index 58b79d083..6f30cd9a9 100644 --- a/app/graphql/types/file.rb +++ b/app/graphql/types/file.rb @@ -7,7 +7,7 @@ module Types field :content_type, String, null: false def url - Rails.application.routes.url_helpers.url_for(object) + object.service_url end end end diff --git a/app/models/champs/piece_justificative_champ.rb b/app/models/champs/piece_justificative_champ.rb index 27bda0eb8..35403deb6 100644 --- a/app/models/champs/piece_justificative_champ.rb +++ b/app/models/champs/piece_justificative_champ.rb @@ -48,7 +48,7 @@ class Champs::PieceJustificativeChamp < Champ def for_api if piece_justificative_file.attached? && (piece_justificative_file.virus_scanner.safe? || piece_justificative_file.virus_scanner.pending?) - Rails.application.routes.url_helpers.url_for(piece_justificative_file) + piece_justificative_file.service_url end end end From c6326bfa772052c889c6cb395c73e51042b26109 Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Thu, 19 Dec 2019 18:29:01 +0100 Subject: [PATCH 3/3] Fix tests involving attachment urls --- spec/controllers/api/v2/graphql_controller_spec.rb | 4 +--- spec/controllers/instructeurs/dossiers_controller_spec.rb | 4 ++-- .../new_administrateur/mail_templates_controller_spec.rb | 2 +- spec/controllers/users/dossiers_controller_spec.rb | 2 +- spec/models/champs/piece_justificative_champ_spec.rb | 4 ++-- spec/serializers/champ_serializer_spec.rb | 2 +- spec/serializers/dossier_serializer_spec.rb | 3 ++- spec/spec_helper.rb | 2 ++ .../instructeur/dossiers/_state_button.html.haml_spec.rb | 2 +- spec/views/users/dossiers/brouillon.html.haml_spec.rb | 2 +- 10 files changed, 14 insertions(+), 13 deletions(-) diff --git a/spec/controllers/api/v2/graphql_controller_spec.rb b/spec/controllers/api/v2/graphql_controller_spec.rb index 9192325a6..d92e06160 100644 --- a/spec/controllers/api/v2/graphql_controller_spec.rb +++ b/spec/controllers/api/v2/graphql_controller_spec.rb @@ -209,7 +209,6 @@ describe API::V2::GraphqlController do checksum byteSize contentType - url } } avis { @@ -270,8 +269,7 @@ describe API::V2::GraphqlController do filename: commentaire.piece_jointe.filename.to_s, contentType: commentaire.piece_jointe.content_type, checksum: commentaire.piece_jointe.checksum, - byteSize: commentaire.piece_jointe.byte_size, - url: Rails.application.routes.url_helpers.url_for(commentaire.piece_jointe) + byteSize: commentaire.piece_jointe.byte_size }, email: commentaire.email } diff --git a/spec/controllers/instructeurs/dossiers_controller_spec.rb b/spec/controllers/instructeurs/dossiers_controller_spec.rb index db3e30ef0..264d68ae3 100644 --- a/spec/controllers/instructeurs/dossiers_controller_spec.rb +++ b/spec/controllers/instructeurs/dossiers_controller_spec.rb @@ -17,9 +17,9 @@ describe Instructeurs::DossiersController, type: :controller do context 'when a dossier has an attestation' do let(:dossier) { create(:dossier, :accepte, attestation: create(:attestation, :with_pdf), procedure: procedure) } - it 'redirects to attestation pdf' do + it 'redirects to a service tmp_url' do get :attestation, params: { procedure_id: procedure.id, dossier_id: dossier.id } - expect(response).to redirect_to(dossier.attestation.pdf_url.gsub('http://localhost:3000', '')) + expect(response.location).to match '/rails/active_storage/disk/' end end end diff --git a/spec/controllers/new_administrateur/mail_templates_controller_spec.rb b/spec/controllers/new_administrateur/mail_templates_controller_spec.rb index b93d4857c..527dc11d7 100644 --- a/spec/controllers/new_administrateur/mail_templates_controller_spec.rb +++ b/spec/controllers/new_administrateur/mail_templates_controller_spec.rb @@ -14,7 +14,7 @@ describe NewAdministrateur::MailTemplatesController, type: :controller do it { expect(response).to have_http_status(:ok) } it 'displays the procedure logo' do - expect(response.body).to have_css("img[src*='#{procedure.logo_url}']") + expect(response.body).to have_css("img[src*='/rails/active_storage/blobs/']") end it 'displays the action buttons' do diff --git a/spec/controllers/users/dossiers_controller_spec.rb b/spec/controllers/users/dossiers_controller_spec.rb index 72c7833b6..f9fd181b5 100644 --- a/spec/controllers/users/dossiers_controller_spec.rb +++ b/spec/controllers/users/dossiers_controller_spec.rb @@ -145,7 +145,7 @@ describe Users::DossiersController, type: :controller do it 'redirects to attestation pdf' do get :attestation, params: { id: dossier.id } - expect(response).to redirect_to(dossier.attestation.pdf_url.gsub('http://localhost:3000', '')) + expect(response.location).to match '/rails/active_storage/disk/' end end end diff --git a/spec/models/champs/piece_justificative_champ_spec.rb b/spec/models/champs/piece_justificative_champ_spec.rb index 4b38613d0..506eba015 100644 --- a/spec/models/champs/piece_justificative_champ_spec.rb +++ b/spec/models/champs/piece_justificative_champ_spec.rb @@ -9,12 +9,12 @@ describe Champs::PieceJustificativeChamp do context 'when file is safe' do let(:status) { ActiveStorage::VirusScanner::SAFE } - it { is_expected.to include("/rails/active_storage/blobs/") } + it { is_expected.to include("/rails/active_storage/disk/") } end context 'when file is not scanned' do let(:status) { ActiveStorage::VirusScanner::PENDING } - it { is_expected.to include("/rails/active_storage/blobs/") } + it { is_expected.to include("/rails/active_storage/disk/") } end context 'when file is infected' do diff --git a/spec/serializers/champ_serializer_spec.rb b/spec/serializers/champ_serializer_spec.rb index 07ff5e10e..602356312 100644 --- a/spec/serializers/champ_serializer_spec.rb +++ b/spec/serializers/champ_serializer_spec.rb @@ -14,7 +14,7 @@ describe ChampSerializer do end after { champ.piece_justificative_file.purge } - it { is_expected.to include(value: url_for(champ.piece_justificative_file)) } + it { expect(subject[:value]).to match('/rails/active_storage/disk/') } end context 'when type champ is not piece justificative' do diff --git a/spec/serializers/dossier_serializer_spec.rb b/spec/serializers/dossier_serializer_spec.rb index 54c29b033..f40d45186 100644 --- a/spec/serializers/dossier_serializer_spec.rb +++ b/spec/serializers/dossier_serializer_spec.rb @@ -79,13 +79,14 @@ describe DossierSerializer do ], pieces_justificatives: [ { - "content_url" => champ_pj.for_api, + "content_url" => subject[:pieces_justificatives][0]["content_url"], "created_at" => champ_pj.created_at.in_time_zone('UTC').iso8601(3), "type_de_piece_justificative_id" => original_pj_id, "user" => a_hash_including("id" => dossier.user.id) } ] ) + expect(subject[:pieces_justificatives][0]["content_url"]).to match('/rails/active_storage/disk/') end it "does not expose the PJ as a champ" do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 655686eb2..538b95806 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -150,6 +150,8 @@ RSpec.configure do |config| Typhoeus::Expectation.clear ActionMailer::Base.deliveries.clear + + ActiveStorage::Current.host = 'http://test.host' } RSpec::Matchers.define :have_same_attributes_as do |expected, options| diff --git a/spec/views/instructeur/dossiers/_state_button.html.haml_spec.rb b/spec/views/instructeur/dossiers/_state_button.html.haml_spec.rb index 048b8b7bb..1ee153818 100644 --- a/spec/views/instructeur/dossiers/_state_button.html.haml_spec.rb +++ b/spec/views/instructeur/dossiers/_state_button.html.haml_spec.rb @@ -103,7 +103,7 @@ describe 'instructeurs/dossiers/state_button.html.haml', type: :view do it 'allows to download the justificatif' do expect(rendered).to have_dropdown_item('Justificatif') - expect(rendered).to have_link(href: url_for(dossier.justificatif_motivation.attachment.blob)) + expect(response).to have_css("a[href*='/rails/active_storage/blobs/']", text: dossier.justificatif_motivation.attachment.filename.to_s) end end end diff --git a/spec/views/users/dossiers/brouillon.html.haml_spec.rb b/spec/views/users/dossiers/brouillon.html.haml_spec.rb index 5a1802c1e..dc13af78a 100644 --- a/spec/views/users/dossiers/brouillon.html.haml_spec.rb +++ b/spec/views/users/dossiers/brouillon.html.haml_spec.rb @@ -17,7 +17,7 @@ describe 'users/dossiers/brouillon.html.haml', type: :view do end it 'affiche un lien vers la notice' do - expect(rendered).to have_link("Guide de la démarche", href: url_for(procedure.notice)) + expect(response).to have_css("a[href*='/rails/active_storage/blobs/']", text: "Guide de la démarche") end it 'affiche les boutons de validation' do