Vérification systématique du type de session dans SessionNamespace by francoisfreitag · Pull Request #5933 · gip-inclusion/les-emplois

francoisfreitag · 2025-04-10T11:57:07Z

🤔 Pourquoi ?

Sécurité : détecter les pollutions de sessions par les utilisateurs et s’en prémunir. La session est un endroit supposé sûr, hors du contrôle des utilisateurs.

tonial · 2025-04-10T12:38:14Z

Je t'ai ajouté à ma PR #5931
Vu que je touche aussi aux sessions, tu as l'air au point sur le sujet :)

xavfernandez

👍

EwenKorr

C'est élégant et plus générique en effet.

dejafait

LGTM et j'apprends plein de trucs sur notre mécanisme de session, merci 😊

rsebille · 2025-04-10T12:11:27Z

+        view.
+        """
+        try:
+            session_kind = self._session[self.session_kind_key]


Un peu naïvement j'aurais plutôt mis ça en dessous de self._session[self.name] afin que tout soit contenu au même endroit mais ça veux dire devoir ajouter un niveau entre self._session[self.name] et data si on ne veux pas polluer 🤔.

La transition m’a fait un peu peur, parce qu’il faudrait réécrire les sessions en cours. Et l’accès direct via request.session["name"] serait cassé (bien qu’il ne soit pas encouragé). J’ai trouvé que stocker les données à côté permettait de répondre au problème de manière moins invasive.

rsebille · 2025-04-10T12:34:29Z

+                session_kind = None
+        if session_kind is not None and session_kind != self.expected_session_kind:
+            logger.warning(f"Loading a {session_kind} while expecting a {self.expected_session_kind}.")
+            raise Http404


Pas sûr que Http404 soit la bonne exception, le type ne devrais pas être différent donc si on est dans ce cas alors c'est qu'il y a un loup et donc on veux un truc qui casse bien fort et pas être silencieux.

Http404 ne devrait se produire que si la session est corrompue (elle a un session_kind qui n’est pas l’attendu).
On peut mettre un erreur plus costaud pour Sentry.

Http404 n’est pas une super exception, mais permet de mutualiser le comportement actuel plutôt que de demander à chaque utilisateur de SessionNamespace de garder chaque lecture avec try: ... except SessionCorrupted: raise Http404.

Pour moi on ne veux pas spécialement attraper l'erreur mais la laisser passer, pour moi on est dans la même logique que le token CSRF ou autre joyeuseté de ce type donc retourner une 404 me semble bizarre pour ce genre de truc.

Dans un monde parfait, oui. C’est aux vues de décider quoi faire en cas d’erreur, et pas à un utilitaire de gestion de session.

Le problème est que l’erreur peut sortir à n’importe quel appel, parce que le SessionNamespace est initialisé paresseusement. Si SessionNamespace accédait immédiatement à la session, on pourrait ne garder que dans __init__() et demander aux appelant de gérer ce cas. Vu la multitude de méthode de l’API de SessionNamespace, une demander à l’appelant de garder chaque appel me semble se faire des nœuds au cerveau.

J’ai déjà pensé plein de fois à passer SessionNamespace en accès immédiat, mais c’est long et pénible, et les gains me semblent minimes.

Si on veut, on peut aussi définir une:

class SessionNamespaceCorrupted(Http404): pass

Ça permet aux vues « bonnes élèves » de gérer proprement le cas, et d’éviter la 500 sur les autres.

francoisfreitag · 2025-04-14T07:47:44Z

+            if session_kind is not None and session_kind != self.expected_session_kind:
+                logger.warning(f"Loading a {session_kind} while expecting a {self.expected_session_kind}.")
+                raise Http404
+            self.kind_verified = True


On marquera les sessions actives qui ne vérifiaient pas session_kind comme verified. C’est juste pour une semaine et pas bien pire que le comportement actuel.

On est d'accord que cela ne concerne que la session de candidature ?

francoisfreitag · 2025-04-14T09:44:42Z

J’ai poussé un fixup pour les deux commentaires résolus :

Écrire kind_verified dans verify_kind().
Bouger le check if kind_verified: dans verify_kind.

francoisfreitag · 2025-04-14T09:50:03Z

Sans autre retour, je pense merger cette PR en l’état (après squash) demain. J’accorde très facilement des délais supplémentaires sur simple MP.

xavfernandez · 2025-04-14T11:45:00Z

+            if session_kind is not None and session_kind != self.expected_session_kind:
+                logger.warning(f"Loading a {session_kind} while expecting a {self.expected_session_kind}.")
+                raise Http404
+            self.kind_verified = True


On est d'accord que cela ne concerne que la session de candidature ?

rsebille

Toujours circonspect d'utiliser http404 mais c'est ce que font les wizard actuellement donc soit 🤷.

Will help land future changes, where more keys should be ignored according with `.endswith()`.

Multiple forms on the site are quite long, and were split over multiple pages, storing the intermediary data in the session. Data is only written to the database at the end of the editing process. In order to work well with tabs, the sessions are namespaced (usually stored under a UUID at the root of the session), and the user passes the namespace through the URL. Which means the user can read the UUID from the URL, and try reusing a session from an editing process in other views, allowing users to somewhat manipulate their session. Although most processes are expected to fail when given data from another editing process, it remains a security risk as developers cannot reasonably keep track of the intersection of session data for all editing processes on the site. To protect against such issues, some views (subclasses of WizardView) store a session_kind, and verify it when they load the user session. Generalize the verification to all `SessionNamespace`, to force all users of `SessionNamespace` to follow that pattern.

francoisfreitag added the no-changelog Ne doit pas figurer dans le journal des changements. label Apr 10, 2025

francoisfreitag self-assigned this Apr 10, 2025

francoisfreitag requested review from EwenKorr, rsebille, tonial and xavfernandez April 10, 2025 11:57

francoisfreitag added tech and removed no-changelog Ne doit pas figurer dans le journal des changements. labels Apr 10, 2025

francoisfreitag changed the title ~~Vérifier le type de session dans SessionNamespace~~ Vérification systématique du type de session dans SessionNamespace Apr 10, 2025

francoisfreitag force-pushed the ff/session-namespace-kind-2 branch 4 times, most recently from 475a053 to f1de717 Compare April 10, 2025 12:24

xavfernandez approved these changes Apr 10, 2025

View reviewed changes

Comment thread itou/utils/session.py Outdated

Comment thread tests/utils/test.py

tonial approved these changes Apr 10, 2025

View reviewed changes

Comment thread itou/utils/session.py Outdated

EwenKorr approved these changes Apr 10, 2025

View reviewed changes

dejafait self-requested a review April 10, 2025 13:21

dejafait approved these changes Apr 10, 2025

View reviewed changes

rsebille reviewed Apr 10, 2025

View reviewed changes

francoisfreitag force-pushed the ff/session-namespace-kind-2 branch 2 times, most recently from 8ed8f7b to da2657c Compare April 10, 2025 15:41

xavfernandez reviewed Apr 10, 2025

View reviewed changes

Comment thread itou/utils/session.py Outdated

francoisfreitag commented Apr 14, 2025

View reviewed changes

xavfernandez approved these changes Apr 14, 2025

View reviewed changes

rsebille approved these changes Apr 14, 2025

View reviewed changes

Comment thread itou/utils/session.py Outdated

francoisfreitag added 2 commits April 14, 2025 17:21

Extract test helper session_data_without_known_keys

23cb7ac

Will help land future changes, where more keys should be ignored according with `.endswith()`.

francoisfreitag force-pushed the ff/session-namespace-kind-2 branch from 294afce to 3d0e0f8 Compare April 14, 2025 15:21

xavfernandez approved these changes Apr 14, 2025

View reviewed changes

francoisfreitag added this pull request to the merge queue Apr 15, 2025

Merged via the queue into master with commit 0a5b541 Apr 15, 2025

francoisfreitag deleted the ff/session-namespace-kind-2 branch April 15, 2025 07:13

francoisfreitag mentioned this pull request Apr 15, 2025

Candidature: Travail préparatoire en vue de refondre la session #5931

Merged

2 tasks

Conversation

francoisfreitag commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤔 Pourquoi ?

Uh oh!

tonial commented Apr 10, 2025

Uh oh!

xavfernandez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EwenKorr left a comment

Choose a reason for hiding this comment

Uh oh!

dejafait left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

francoisfreitag Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francoisfreitag commented Apr 14, 2025

Uh oh!

francoisfreitag commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsebille left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

francoisfreitag commented Apr 10, 2025 •

edited

Loading

francoisfreitag Apr 14, 2025 •

edited

Loading

francoisfreitag commented Apr 14, 2025 •

edited

Loading