Sachant que c’est converti ailleurs dans SPIP, je ne saisis pas l’importance du blocage de ce paramètre.
Parce que ces id_ sont utilisés partout dans SPIP et les plugins, et que c’est un risque facile à éviter.
A mon avis ça conduit un faux sentiment de sécurité. Il vaut mieux prendre l’habitude d’utiliser des intval() que de compter sur un écran de sécurité pas toujours présent (surtout pour les anciennes versions de SPIP).
En plus, ça peut pénaliser d’autres appli si on veut les faire fonctionner avec l’écran de sécurité pour profiter, par exemple, du mécanisme de rejet pour les robots en cas de surcharge du serveur (applis sur lesquelles on a pas la main)
En effet quand on programme il ne faut jamais compter sur l’écran de sécurité et chaque code devrait nettoyer lui-même les valeurs d’entrée. Mais globalement ça évite de se prendre par une étourderie.
Pour ce qui est des autres applis, tu parles d’un cas concret ou en général ? Car si ça bloque une appli existante il faut voir si on peut améliorer le filtrage, de manière à assainir la variable sans bloquer un autre usage (réel).
Le cas concret que ça protège c'est l'injection de PHP dans les squelettes qui font
<?php
$id_rubrique='#ID_RUBRIQUE';
...
?>
C'est une mauvaise pratique (trop) courante et le but de l'écran était de protéger ce cas sur lequel on a pas de prise.
A mon avis le mieux c’est de faire cette protection (si ce n’est pas déjà le cas) dans _request()
On peut aussi se dire que s’il fait du PHP, ce n’est pas à SPIP de gérer les risques de sécu qu’il met dans son code (il peut aussi écrire un $url="#ENV{redirect}"; tout aussi risqué – quoi que peut-être pas vu les traitements sur #ENV)
En tout cas voici ce que ça donnerait simplement dans SPIP
Index: ecrire/inc/utils.php
Pourquoi pas. Mais je suis un peu étonné par ton code car je croyais qu’on ne pouvait pas mettre des array() dans un define() ?
Sinon ma question reste : quelles sont les exceptions à traiter, concrètement. L’écran a déjà deux exceptions : ‘id_table’,‘id_base’
2015-05-01 10:46 GMT+02:00 Fil <fil@rezo.net>:
2015-05-01 10:46 GMT+02:00 Fil <fil@rezo.net>:
On peut toujours dire que c'est chacun pour soi etc. Le but de l'écran c'est de patcher les failles certaines mais aussi les failles frequentes liées à une mauvaise pratique dans les squelettes ou dans un plugin.
Bref, ce id_xx forcé en numérique est parfois embêtant mais il n'est pas inutile. Ton patch dans _request() ne suffit pas car #ENV n'est pas rempli à partir de _request().
Sinon en effet si on affecte une variable PHP avec un autre #ENV qui n'est pas numérique on a la même faille, et d'ailleurs on avait eu une alerte de sécu sur #LANG aussi. Cela dit la plupart des autres variables utilisées dans les skels viennent d'une boucle et n'ont pas ce risque.
Donc oui ce patch ne couvre pas tous les cas, oui il est parfois gênant, mais c'est un bon compromis qui couvre une large palette de cas risqués et qui casse pas grand chose.
Au cas par cas tu peux décider de le commenter sur ta version de l'écran de sécu que tu utilises si il te gêne vraiment, mais j'ai l'impression que ce serait plutôt mieux de le garder... (même si parfois il m'a gêné, y compris sur quelques plugins de la zone qui utilisent par exemple le selecteur article/rubrique sur un id_xxx alors que le résultat fournit est de la forme "article|23")
Ok j’insiste plus.
Bon, on a un peu l’impression de sortir avec 3 pulls en plein été pour ne pas attraper un rhume mais pourquoi pas.
Comme cela a été très bien dit avant, côté optimisation il y a certainement mieux à trouver ailleurs.