Fwd: [SPIP][Notifications/Commit Zone] [incarner] suppression de intval (déjà fait) + $id_objet (inutilisé)

Dev
#1

Hello,

même si ça semble faire doublon, c’est une bonne pratique que de garder les échappements dans la requête car cela permert de vérifier qu’une requête SQL est safe sans avoir à relire tout le code en amont (ie en faisant un grep sur les fonctions sql_…) alors que sinon c’est la croix et la bannière si pour chaque requête SQL on doit relire 10 ou 20 lignes de codes (sans parler du risque plus grand d’erreur et d’oubli).

En s’en fiche de faire 2 fois un intval si ça permet d’avoir un meilleur niveau de sécurité de manière générale !
Donc on ne supprime pas un intval « déjà fait » dans une requête SQL :stuck_out_tongue:

–
Cédric
---------- Message transféré ----------
De : SPIP-contrib via Discuter de SPIP noreply@discuter.spip.net
Date : 17 oct. 2022 à 22:46 +0200
À : cedric@yterium.com
Objet : [SPIP][Notifications/Commit Zone] [incarner] suppression de intval (déjà fait) + $id_objet (inutilisé)

SPIP-contrib
Octobre 17
spip-contrib-extensions/incarner
Par jluc, le 17 octobre 2022 à 22h16min :
suppression de intval (déjà fait) + $id_objet (inutilisé)
Modifié
incarner_pipelines.php
Détails : suppression de intval (déjà fait) + $id_objet (inutilisé) · 13a0c62b61 - incarner - SPIP on GIT
Voir le sujet ou répondre à ce courriel pour répondre.
Vous recevez ce courriel car vous avez activé la liste de diffusion.
Pour vous désabonner de ces courriels, cliquez ici.

#2

Hello Cédric,
Que voudrais tu alors ?
Que je recommite l’ajout d’un intval ?
Car je saurai pas force pusher un squash (pour autant que ça existe).

Moi ce qui me gêne (ailleurs) c’est l’absence de parenthèses dans les expressions logiques.
Les priorités d’opérateurs sont traîtres et il y a régulièrement des erreurs comme tu sais.
Du coup quand il n’y a pas autant de parenthèses qu’il serait utile pour que ça soit clair, ça oblige à se creuser les méninges pour lever l’ambiguïté ou vérifier qu’il y a pas erreur… ou alors on laisse l’évidence et parfois il y a erreur.
Et je trouve ça bien plus pénible de chercher à lever une ambiguité ou de détecter une erreur de parenthèses que de chercher un intval.
Entre autre parce qu’il y a des outils dans les IDE pour en un clic suivre l’origine et les différents usages d’une variable – et donc voir de suite s’il y a un intval ou un type int déclaré.

Alors je trouve qu’il faudrait plus souvent mettre des parenthèses dans les expressions logiques, même si elles ne sont pas strictement nécessaire.

#3

On ne force-push jamais sur le master, donc il faut commit un revert en effet.
La différence avec les parenthèses parfois manquante dans les expressions logiques c’est qu’on est ici dans le domaine de la sécurité et du risque d’injection SQL.

Si les expressions sql sont correctement échappées il est rapide d’auditer un plugin avec un grep sql_. Mais à chaque expression qui ne fait pas apparaitre d’échappement on doit ouvrir l’IDE et analyse le code en amont pour voir si il y a risque ou pas, ce qui tends à augmenter les risques.

Je pense pas que quelqu’un se plaindra si tu ajoutes des parenthèses dans des expressions complexes pour en faciliter la lisibilité et la clareté, mais dans le cas que j’ai pointé on a enlevé de la lisibilité, ce qui est facheux.

#4

Ok je vais (essayer de) reverter (aussi proprement que possible).

Je pige pas bien quand même : les appels aux sql_ ont souvent des formes syntaxiques variées, avec une suite d’arguments sur plusieurs lignes, des chaines ou des tableaux, etc. Un grep sql_ devra être sacrément balèse pour ramener quelque chose d’utile à analyser ! Peut être une suite de grep ciblants avec du code autour peut parvenir à en ramener une part, ou sinon il faut un vrai parseur PHP… Gros boulot.

Pour l’ajout de parenthèses, la clarté peut être un effet second, et un moyen, mais l’utilité principale est la garantie de l’absence de bugs.