Sécurise le tri du dashboard instruction contre l'injection SQL (DPP-77) - #1729
Merged
jbfeldis merged 2 commits intoAug 26, 2026
Conversation
Le tri Ransack (nom de colonne + direction) était interpolé en SQL brut dans .order(), protégé uniquement par le garde Rails disallow_raw_sql!. Un nom de tri non autorisé provoquait un 500 (injection bloquée par Rails, ou colonne inexistante). Construit désormais l'ORDER BY via Arel (identifiant quoté) avec repli sur le tri par défaut quand la colonne demandée n'existe pas. Couvre les deux sous-classes Dashboard*Search via la classe de base. Ref DPP-77
jbfeldis
marked this pull request as ready for review
August 13, 2026 10:09
jbfeldis
changed the title
Isalafont
self-requested a review
August 24, 2026 14:07
Isalafont
requested changes
Aug 24, 2026
Isalafont
left a comment
Isalafont
left a comment
Contributor
There was a problem hiding this comment.
Merci pour la PR !
J'ai noté une suggestion mais c'est pas pas obligatoire ^^
Par contre j'ai une question, les tests ici vérifient que la requête ne plante pas mais pas que le tri est le bon.
Du coup, si quelqu'un réintroduit une interpolation dans le order demain, les tests restent verts ?
Peux tu les faire assert sur le SQL produit plutôt que sur l'absence d'erreur ?
Retours de review sur la PR #1729. Les specs de tri se contentaient de vérifier que la requête ne plantait pas : une réintroduction de l'interpolation dans .order() les aurait laissées vertes. Elles asserent désormais sur le SQL produit (identifiant quoté, NULLS LAST, absence du fragment injecté), ce qui couvre à la fois le repli sur le tri par défaut et l'absence d'interpolation. Ferme aussi la duplication du ternaire de direction via #normalized_direction. Ref DPP-77
Contributor
Author
|
J'ai amélioré le test pour qu'il reflète mieux le clean mais on ne peut pas écrire un test qui vérifie que le code ne change pas 😅 |
Isalafont
approved these changes
Aug 26, 2026
Isalafont
left a comment
Isalafont
left a comment
Contributor
There was a problem hiding this comment.
Top merci beaucoup !!
Let's go 🚀
jbfeldis
deleted the
feature/dpp-77-ts-securiser-le-tri-du-dashboard-instruction-order-interpole
branch
August 26, 2026 08:33
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contexte
Sentry
DATAPASS-REBORN-EV: le tri du dashboard instruction interpolait le nom de colonne et la direction issus de Ransack directement dans.order():Ces valeurs viennent du param utilisateur
search_query[s]. L'injection SQL n'était stoppée que par le garde framework Railsdisallow_raw_sql!— on ne veut pas en dépendre. Un nom de tri non autorisé provoquait un500(injection bloquée par Rails, ou colonne inexistante).Changement
Construction de l'
ORDER BYvia Arel (identifiant quoté) avec repli sur le tri par défaut quand la colonne demandée n'est pas une vraie colonne du modèle. Le fix est dans la classe de baseInstruction::Search::DashboardSearch→ couvre les deux sous-classes (DashboardHabilitationsSearch,DashboardDemandesSearch).SQL généré vérifié :
"authorizations"."created_at" DESC NULLS LAST(repli, identifiant quoté, aucune interpolation)created_at asc→"authorizations"."created_at" ASC NULLS LAST(préservé,NULLS LASTconservé)Tests
search_demandes+search_habilitations) : 28 examples, 0 failuresPérimètre
Correctif ciblé. Le rescue global des inputs de scan (400/404/422) et la coupure du scan à la source (token staging, DPP-10) sont hors périmètre → projet Sécurité applicative.
Closes DPP-77