betagouv / rdv-service-public

Prise de RDV pour les services publics
https://rdv.anct.gouv.fr
GNU Affero General Public License v3.0
17 stars 2 forks source link

prevent passing sensitive job arguments to Sentry #4361

Closed adipasquale closed 3 months ago

adipasquale commented 3 months ago

J’étais surpris de voir que les jobs marqués explicitement self.log_arguments = false passent quand même leurs params à Sentry

log_arguments est en fait un attribut de ActiveJob, il n’est pas pris en compte par Sentry.

Dans cette PR je modifie le code de notre instrumentation Sentry pour prendre en compte ce param et ne pas transmettre d’infos sensibles dans Sentry

testable en console en local après avoir rajouté le SENTRY_DSN :

class OnPurposeError < StandardError; end; class FailJob4 < ApplicationJob; self.log_arguments = false; def perform
(a); raise(OnPurposeError, "test adrien"); end; end;  FailJob4.perform_now("premier argument")
adipasquale commented 3 months ago

@victormours tu avais l’air d’avoir un avis potentiellement différent sur cette PR ?

adipasquale commented 3 months ago

je n’ai pas d’avis fort sur ce sujet non plus, c’était plus une intuition que la politique de protection des données sensibles devraient être la même dans les logs que dans le sentry mais effectivement rien n’y oblige.

Dans notre doc on dit qu’on garde les logs et sentry pour des raisons qui se recoupent mais pas identiques et la durée est différente. donc ça peut justifier cette différence de traitement et le fait d’en stocker plus sur sentry que dans les logs.

comme vous le sentez ! je pense que si personne n’a d’avis fort je vais fermer cette PR et donc ne rien changer par rapport à l’existant

francois-ferrandis commented 3 months ago

J'ai effectivement l'impression que cette Pr va dans le sens contraire de l'issue #4359 (tl;dr : c'est ok de mettre des données nécessaire au debug dans Sentry), donc je suis pas sûr qu'elle soit nécessaire. Après si vous pensez que ça ne complique pas le debug, c'est toujours un nice to have de ne pas balader trop de données personnelles.

Pour le cas des jobs, nous avons ces deux différences :

  1. nous sommes sûrs qu'ils contiennent des données personnelles
  2. nous avons de quoi débugger car on a le job_id dans Sentry, et donc on peut aller sur https://rdv.anct.gouv.fr/super_admins/good_job/jobs/:job_id pour observer les paramètres du job

Et donc je suis d'avis de passer cette PR, ça fait beaucoup moins de données personnelles dans Sentry.

On pourrait même, d'après le point 2 ci-dessus, cesser de logger les params pour tous les jobs, dans une éventuelle autre PR bien sûr.

sentry-sentry-incubateur-net[bot] commented 3 months ago

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

Did you find this useful? React with a 👍 or 👎