feat(security): restrict email sender #2719

Michaelvilleneuve · 2025-03-18T11:02:40Z

Cette PR permet d'améliorer la sécurité liée à l'envoi du mail d'information lors de l'ajout d'un usager dans une autre organisation du département.

Voici les changements fonctionnels apportés :

Il n'est plus possible de changer le destinataire du mail

Désormais le destinaire est figé, il reste affiché en read only dans la modale pour information mais n'est plus modifiable par l'usager.
J'ai aussi enlevé la sélection car dans tous les cas nous n'utilisions que le mail de l'organisation.
Aussi nous n'utilisons plus les params envoyé pour déduire l'email, on va le chercher depuis l'organisation en elle même.

Il n'est plus possible d'éditer le contenu principal

Le contenu principal du mail devient lui aussi figé afin de garantir que l'information principale (le transfert de l'usager) reste le texte principal affiché dans le mail.

Il devient possible d'ajouter un message personnalisé

J'ajoute la possibilité d'ajouter un message custom, celui-ci est désormais restreint à l'intérieur d'un bloc blockquote afin de le séparer visuellement et il clairement affiché comme étant un message personnalisé de l'organisation.

On ajoute de la vérification sur les pièces jointes

Toutes les pièces jointes ajoutées sont désormais scannées :

On autorise uniquement les extensions suivantes : jpg .jpeg .png .pdf .xlsx .csv
On vérifie la cohérence du mime_type
On s'assure que ça fasse moins de 5MO
On checke les premières séquences de bits du fichiers pour vérifier si elle est cohérente avec son type

Envoi de mail depuis la fiche usager

Envoi de mail depuis la création d'orientation

Visuel du mail avec message custom

Visuel du mail depuis la création d'orientations

Amélioration potentielle

En l'état actuel les fichiers invalides sont simplement retirés de la liste.
Je sais pas si c'est hors scope mais vu que ça demanderait plus de travail que l'estimation actuelle je préfère déjà proposer cette PR.

Mais je pense qu'on devrait aussi revoir le workflow pour permettre plus de gestion d'erreurs et ainsi notifier l'utilisateur que certains fichiers sont invalides et les raisons associées pour qu'il puisse revoir sa liste.
En l'état dès que l'on appuie sur envoyer la modal est dismissed donc ça rend l'affichage simple des erreurs inutiles puisque l'usager n'a pas moyen d'éditer son contenu, faudra donc revoir ça aussi.

Fix #2696

…ntroller

Michaelvilleneuve · 2025-03-20T11:02:21Z

Gemfile

@@ -39,6 +39,9 @@ gem "redis", "~> 4.0"
 # Use Active Model has_secure_password
 # gem 'bcrypt', '~> 3.1.7'

+# Ensure uploaded file are secure
+gem "mimemagic", "~> 0.4"


Pour info cette Gem dépend du package système shared-mime-info

J'ai vérifié il est pré-installé sur nos serveurs

aminedhobb

Merci @Michaelvilleneuve 👍 , globalement ça m'a l'air cool et le mécanisme a l'air ok, je t'ai juste laissé quelques remarques.

En l'état actuel les fichiers invalides sont simplement retirés de la liste.
Je sais pas si c'est hors scope mais vu que ça demanderait plus de travail que l'estimation actuelle je préfère déjà proposer cette PR.

Pour moi c'est ok, faut juste confirmer avec le mécanisme entier avec l'équipe produit.

aminedhobb · 2025-03-24T20:42:13Z

app/views/organisations/user_added_notifications/_form.html.erb

-  <%= form_for(:email, url: organisations_user_added_notifications_path, method: :post) do |f| %>
+  <%= form_for(:email, url: department_organisation_user_added_notifications_path(department_id: current_department.id, organisation_id: organisation.id), method: :post) do |f| %>
+      <%= f.hidden_field :user_id, value: user.id %>
+      <%= f.hidden_field :source, value: local_assigns[:source] %>


Pourquoi est-ce qu'on ne passe pas toujours une source à ce formulaire ?
Ce serait plus clair, là ça ne se voit pas que quand on en met pas que ça veut dire qu'on sur la page show

Oui c'est pas faux, mon idée était d'en mettre une quand elle diffère de la source originale qui est l'affectation directe. Mais je suis d'accord ça sera plus clair si on en met systématiquement une

aminedhobb · 2025-03-24T20:42:44Z

app/views/organisations/user_added_notifications/_form.html.erb

+            <%= f.text_area :content, readonly: true, class: "form-control", cols: 15, rows: 4, value: user_added_notification_content(source: local_assigns[:source], user:, organisation:) %>
+        </div>
+        <div class="mb-3">
+          <h5 class="text-dark-blue h4-as-labels">Ajouter un message personnalisé</h5>
+          <p>Ce message apparaitra après le message ci-dessus</p>
+            <%= f.text_area :custom_content, class: "form-control", cols: 15, rows: 6, placeholder: "Votre message personnalisé." %>


nitpick: il y a qq problèmes d'indentations ici

aminedhobb · 2025-03-24T20:43:37Z

app/javascript/stylesheets/components/_form.scss

+}
+
+.form-control[readonly] {
+  background-color: white;


ça risque de causer des soucis si on le met sur toute l'appli, on pourrait pas définir la couleur au niveau de l'input ?

Alors j'avais vérifié il n'y aucun autre endroit impacté dans l'application et ce venait venait overrider un code déjà existant au global :

Je me suis dit que le readonly ne méritait pas le même traitement qu'un disable donc j'ai overridé car ce gris était violent.

À la limite je vais confirmer l'ensemble visuellement + l'input custom avec l'équipe produit et je mettrai le code spécifiquement au niveau de l'input s'ils souhaitent poursuivre avec visuel 👍

aminedhobb · 2025-03-24T20:46:07Z

app/models/uploaded_file_sanitizer.rb

+  end
+
+  def sanitize
+    return nil unless @uploaded_file.respond_to?(:original_filename)


dans quel cas est-ce qu'on peut tomber dans ce cas ?

C'était au cas où quelqu'un manipule la requête et essai d'envoyer d'autres choses que des ActionDispatch::Http::UploadedFile en paramètre attachments du formulaire.
Théoriquement l'appli aurait crash sur les lignes suivantes mais là elle se contente du comportement classique, enlever le fichier des attachments.
C'est peut-être superflu, let me know

aminedhobb · 2025-03-24T20:46:57Z

app/models/uploaded_file_sanitizer.rb

+    filename = @uploaded_file.original_filename
+    extension = File.extname(filename).downcase
+    mime_type = @uploaded_file.content_type
+    file_size = @uploaded_file.size


gros nitpick: on pourrait extraire dans des méthodes d'instance privées si on le voulait vu que les méthodes qui les utilisent sont privées mais ça marche comme ça aussi

C'est vrai que ce n'est pas forcément la responsabilité de cette méthode que d'avoir cette connaissance. Je fais le refacto 👍

aminedhobb · 2025-03-24T20:47:27Z

config/routes.rb

+    resources :organisations, only: [] do
+      resources :user_added_notifications, only: [:create], controller: 'organisations/user_added_notifications'
+    end


il y a une raison particulière de changer les routes ?

Oui, j'aurais du expliquer dans la description de la PR, en fait désormais vu qu'on ne passe plus le contenu en paramètre direct, on a besoin de savoir de reconstituer le message en backend, donc on doit savoir à quelle orga est destinée le message, on a donc besoin des paramètres suivants :

department_id -> Car on veut s'assurer que l'agent vient bien du même département que l'orga à qui il a ajouté l'usager

organisation_id -> Car on veut pouvoir récupérer le destinaire non pas via param mais via l'orga directement

user_id -> Il nous faut préciser le nom et prénom de l'usager dans le contenu du mail et sachant qu'il n'est plus passé en paramètre

Ces 3 éléments font que cette route fait désormais plus de sens scopé à l'intérieur du département et de l'orga à laquelle est destiné l'email, que génériquement à la racine.

aminedhobb · 2025-03-24T20:51:53Z

app/models/uploaded_file_sanitizer.rb

+    ".csv" => "text/csv"
+  }.freeze
+
+  def self.sanitize(uploaded_files)


nitpick: pour être iso avec la méthode d'instance sanitize ce serait bien de renommer cette méthode (par ex sanitize_all) pour montrer qu'elle prend plusieurs arguments

aminedhobb · 2025-03-24T20:54:17Z

app/models/uploaded_file_sanitizer.rb

+    ".jpg" => "image/jpeg",
+    ".jpeg" => "image/jpeg",
+    ".png" => "image/png",
+    ".pdf" => "application/pdf",
+    ".xlsx" => ["application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", "application/zip"],
+    ".csv" => "text/csv"


nitpick: j'ai l'impression que l'implémentation marche tel quel mais pour plus de cohérence est-ce qu'on ne devrait pas avoir que des array en valeur ?

aminedhobb · 2025-03-24T20:55:53Z

app/controllers/organisations/user_added_notifications_controller.rb

-    def user_attachements
-      (email_params[:attachments] || []).compact_blank
+    def custom_content
+      ActionView::Base.full_sanitizer.sanitize(email_params[:custom_content])


Moi ça m'a l'air ok le fait de pouvoir rajouter un élément custom de la façon dont tu le proposes mais est-ce que t'as validé le principe et le wording avec l'équipe produit ?

Non en effet, je leur en touche un mot pour confirmer ça et voir s'ils veulent modifier le visuel (à cause de la notion de read-only pour les autres champs qui peut être confusante)

Michaelvilleneuve · 2025-03-31T09:30:11Z

J'ai mis à jour le visuel suite aux discussions avec Samantha :
https://mattermost.incubateur.net/betagouv/pl/i989hrnkmfdf9kh9od9ez3179e et au nouveau design proposé : https://www.figma.com/design/ngUPYLSKMq1ttDl0sJHQCF/04_Dossier-Usager?node-id=1449-379&t=gqGC7Gyi3fRwNUGR-4

Michaelvilleneuve added 7 commits March 18, 2025 12:02

wip: restrict email sender

6ceb69f

Merge branch 'staging' into fix/restrict-email-sender

98df12a

Merge branch 'staging' into fix/restrict-email-sender

75c3ee8

fix: linter

3577ed3

wip: checking file mime type

b23422a

feat: magic type file detection

e2d2077

user added improvement

ee07244

Michaelvilleneuve changed the title ~~wip: restrict email sender~~ feat(security): restrict email sender Mar 19, 2025

Michaelvilleneuve added 2 commits March 20, 2025 10:10

feat: testing uploaded_file

6c3a188

fix(orientation): share similar partial and forward source data to co…

c9d607a

…ntroller

Michaelvilleneuve marked this pull request as ready for review March 20, 2025 10:58

Michaelvilleneuve commented Mar 20, 2025

View reviewed changes

Michaelvilleneuve requested a review from aminedhobb March 20, 2025 11:02

aminedhobb reviewed Mar 24, 2025

View reviewed changes

Michaelvilleneuve added 3 commits March 25, 2025 11:00

fix: cr feedback

ce232bf

fix: add ability to manage input files list

3a8ebef

fix: minor visual improvement

fee2262

Michaelvilleneuve requested a review from aminedhobb March 31, 2025 09:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(security): restrict email sender #2719

feat(security): restrict email sender #2719

Michaelvilleneuve commented Mar 18, 2025 •

edited

Loading

Michaelvilleneuve Mar 20, 2025

aminedhobb left a comment

aminedhobb Mar 24, 2025

Michaelvilleneuve Mar 25, 2025

aminedhobb Mar 24, 2025

aminedhobb Mar 24, 2025

Michaelvilleneuve Mar 25, 2025

aminedhobb Mar 24, 2025

Michaelvilleneuve Mar 25, 2025

aminedhobb Mar 24, 2025

Michaelvilleneuve Mar 25, 2025

aminedhobb Mar 24, 2025

Michaelvilleneuve Mar 25, 2025

aminedhobb Mar 24, 2025

aminedhobb Mar 24, 2025

aminedhobb Mar 24, 2025

Michaelvilleneuve Mar 25, 2025

Michaelvilleneuve commented Mar 31, 2025

feat(security): restrict email sender #2719

Are you sure you want to change the base?

feat(security): restrict email sender #2719

Conversation

Michaelvilleneuve commented Mar 18, 2025 • edited Loading

Il n'est plus possible de changer le destinataire du mail

Il n'est plus possible d'éditer le contenu principal

Il devient possible d'ajouter un message personnalisé

On ajoute de la vérification sur les pièces jointes

Envoi de mail depuis la fiche usager

Envoi de mail depuis la création d'orientation

Visuel du mail avec message custom

Visuel du mail depuis la création d'orientations

Amélioration potentielle

Choose a reason for hiding this comment

aminedhobb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelvilleneuve commented Mar 31, 2025

Michaelvilleneuve commented Mar 18, 2025 •

edited

Loading