security(#261): ne pas permettre d'introduire du JS dans les messages d'erreur
fix #261 (closed)
Rapports de requête de fusion
Activité
@maieul a ajouté 1 révision : 0b9e4edaa2d8ba1b31e5f638c3203631165daf14
@rastapopoulos @tcharlss @marcimat un avis ?
Je pense que la succession de
#GET
pour aller chercher la bonne clé dans le tableau ne doit pas aider, et du coup le|interdire_script
qui est inclu sur#ENV
ne fait rien car il traite un tableau et non pas une chaine...(au passage je ne comprends pas ce que fait la succession de
#SET{erreurs,...
, mais je suppose qu'il y a une raison)En ce qui concerne les messages d'erreur la bonne pratique est donc
- de ne pas echapper le html qu'ils peuvent contenir (input, mise en forme...)
- mais de bloquer un éventuel JS
D'où le
#ENV*{erreur/truc}
sur lequel on a aboutitÇa vaudrait le coup de corriger proprement et de peut etre remettre à plat cet empilement de
#GET
pour permettre d'avoir du code un peu plus maintenable...@maieul a fait référence à cette PR depuis spip-contrib-extensions/formidable!155 change: fin du comportement dérogatoire sur les retours des erreurs dans
@maieul a force-pushed 2 révisions : 0b9e4edaa2d8ba1b31e5f638c3203631165daf14 1531a5d6a2b9d652a9607bba36a25725fb8c1fef
La nouvelle version de la PR simplifie considérablement le code. Le problème venait du fait qu'on avait 3 manières de décrire un tableau d'erreur lorsqu'on avait des saisies dont les names étaient tabulaires.
- Soit clé format SPIP
'saisie/soussaisies' => 'erreur'
- Soit clé format HTML
'saisie[soussaisies]' => 'erreur'
- Soit structure "formidable" encore plus chelou :
saisie => ['sous_saisie' => 'erreur']
.
J'ai pris le partie de passer dans
saisie_base
uniquement au format HTML.Ainsi on est cohérent avec le nom de la saisie qui est deja normalisé au format tabulaire.
A noter:
a. On cesse de supporter le format dérogatoire formidable, cf. https://git.spip.net/spip-contrib-extensions/formidable/pulls/155 -> il n'y avait je pense que formidable pour l'utiliser (cf. commentaire!) b. Du coup les erreurs sur saisies tabulaire déclarés au format SPIP ne s'appliquent que pour un tableau de saisie PHP, pas en
#SAISIE
-> en même temps c'est moi qui avait ajouté cela, et je ne pense pas que ce soit valable en saisie.Ca casse donc un comportement a priori utilisé par personnes. Mais du coup je proposerai d'intégrer cela dans la v5, sachant que le risque de sécurité est assez minime (il faut deja pouvoir créer ses propres saisies, donc avoir un accès formidable/champs extra interface).
Mais si vraiment on tient
a. Je pense qu'on peut faire une fonction de conversion, type
saisies_tableau_imbrique2name
b. Pour le support en#SAISIE
j'ai cherché, et je ne vois pas trop comment faire : il faudrait pouvoir appliquer la|saisies_nom2name
directement au moment d'appler le INCLURE. Ou alors faudrait que#SAISIE
soit remapper sur#GENERER_SAISIE
(et encore pas sur).Cela me parait 2 cas assez marginal.
- Soit clé format SPIP
@maieul a force-pushed 2 révisions : 1531a5d6a2b9d652a9607bba36a25725fb8c1fef 9cfe0ec8098c1ac439e30ddb3c7129a04e6341e7
Bon, j'ai réussi à lever a. et b.
Pour a.c'est 294d936f517a673f0da5f0bdcc20100d325d86e2 Pour b. c'est 586b8062efd2455de3970bd0d61cc26c2ff83e43
Le code de
saisie/_base
est plus lisible désormais : que l'on passe par #SAISIE ou par #GENERER_SAISIES au moment où l'on arrive dansbase
.- Les nom des saisies sont au format HTML
- Le tableau d'erreur est un tableau plat dont les clés sont au format HTML
=> Il y a donc coconmitance entre le tableau des noms et le tableau des erreurs. YOupi.
Pour moi c'est fusionnable désormais.
@maieul a force-pushed 2 révisions : 9cfe0ec8098c1ac439e30ddb3c7129a04e6341e7 76a27f39fd2c3c259bd33e2b1564eacbc48a95ce
@maieul a force-pushed 2 révisions : 76a27f39fd2c3c259bd33e2b1564eacbc48a95ce a0d0201ff3504d84a3d2e7daed3b08bb5b8bacda
J'avoue que j'ai du mal à suivre tout ce micmac.
Ya vraiment que ça qui utilise un tableau normal ? Car non pas bizarre mais parfaitement "normal" par rapport à PHP : c'est le même type de tableau que ce qu'on passe… dans charger() donc ça pourrait paraitre logique d'accepter le même partout.
Dans les valeurs, quand on envoie les défauts ou le contexte d'env de charger() bah on envoie bien un tableau complet quand ya des sous valeurs : un vrai array PHP quoi
[ 'truc' => [ 'bidule' => [ 'chouette' => 'valeur' ] ] ]
Donc c'est pas si déconnant que ça marche pareil partout, que ça accepte le même format dans verifier() ensuite pour "mapper" les messages avec leur champ quand ya de l'imbrication de tableau.
Hum, oui, le problème c'est que l'attribut
nom
on le passe pas sous la forme[ 'truc' => [ 'bidule' => [ 'chouette' => 'lenomultime' ] ] ]
Mais soit sous la forme
truc/bidule/chouette/lenomultime
soit sous la forme
truc[bidule][chouette][lenomultime]
Et va retrouver ta valeur/ton erreur à la fin avec la deuxième forme (avec la première forme,
|table_valeur
fait l'affaire).=> Ca expliquait le micmac au niveau de #SET/#GET.
Ma proposition est la suivante : il faut que toutes les données associées à une saisie (name, valeur, erreurs) soit formatés de la même manière lorsque cela arrive dans saisies/_base.
Au choix donc :
- Choix 1
truc/bidule/chouette/lenomultime
- Choix 2
truc[bidule][chouette][lenomultime]
Il me semblait que le choix 2 était le plus pertinent.
Cela étant tu viens de me rappeler que je devrais regarder comment arrivent
valeurs
à ce stade.Le départ de ton message est pas bon, le tableur imbriqué correspond aux VALEURS pas au nom. Le "nom ultime" dans ce tableau c'est "chouette", et non pas la valeur de la clé.
Ce que je disais donc c'est que pour charger(), toutes les valeurs sont passées comme ça, normalement, en vrai array() PHP, puisque c'est bien ça que génère le HTML quand il est posté sur tous les langages serveurs (PHP ou autres peu importe). Les "valeurs" du charger() sont censées arriver en vrai tableau, c'est la base côté serveur.
Et donc je disais que ça pouvait paraitre logique et cohérent que les erreurs puissent aussi être renvoyées sous la même forme, et que c'est même "le premier choix" pour ça, puisque c'est bien ce que génère le HTML au final comme type de tableau.
Le fait de lister "à plat" avec "truc/bidule/chouette" = "ma valeur" ou "truc/bidule/chouette" = "mon erreur", ça c'est une norme propre à SPIP. Et pour l'écriture avec crohets, c'est la norme HTML du côté du HTML en premier lieu, mais ensuite côté serveur ça se transforme en vrai tableau.
D'ailleurs même pour l'écriture SPIP, c'est censé être utilisé comme "clé d'accès", mais par contre c'est pour accéder… à un truc qui est stocké dans un tableau, c'est pas une clé en soi.
Alors je sais pas si on peut uniformiser, ou si faut gérer toutes les écritures ET stockages…
Le départ de ton message est pas bon, le tableur imbriqué correspond aux VALEURS pas au nom. Le "nom ultime" dans ce tableau c'est "chouette", et non pas la valeur de la clé.
Certe, mais pour savoir (au sein du squelette) quelle valeur (ou erreur) il faut afficher / mettre dans le html il faut bien qu'on ait le nom correspondant :)
Et donc si tu as le tableau de valeur/erreur qui est sous la forme suivante
[ 'truc' => [ 'bidule' => [ 'chouette' => 'valeur' ] ] ]
mais que le
#ENV{nom}
est de la forme suivantetruc[bidule][chouette]
bah
#ENV{erreurs/#ENV{noms}}
ne va rien renvoyer.
D'où le fait que, pour ce qui est du charger(), ça va chercher la "vraie valeur" automatiquement, suivant le chemin du name HTML, mais bien dans un array PHP : https://git.spip.net/spip-contrib-extensions/saisies/src/branch/master/inc/saisies_afficher.php#L194
Je ne dis pas que le comportement doit obligatoirement être le même pour verifier(), c'est à débattre hein, mais faut pas l'écarter quoi, ça peut être légitime et cohérent d'avoir le même comportement aussi et que l'uniformisation soit bien sur un vrai tableau PHP. Ou pas. Ou aucune uniformisation et et ça gère plusieurs manières à la fois. J'ai pas encore d'avis à cet instant T haha.
Ah bah oui j'avais oublié qu'on passait
valeur
et pasvaleurs
(! valeur unique vs tableau). Alors que par contre on passeerreurs
et paserreur
:)Et heu, le code que tu cite il est utilisé par
#SAISIE
? ou bien ca marche que pour#GENERER_SAISIE
.Mon avis c'est qu'on devrait avoir un truc cohérent tout le temps. Peu importe la méthode, mais que en entrée de
saisies/_base.html
on sache où trouver les choses.Donc là maintenant avec toutes ces données en tête, ma proposition serait la suivante:
- Au lieu de passer à
saisies/_base.html
erreurs
on passeerreur
en donnant directement l'erreur correspondante. - Le
#ENV{nom}
danssaisies/_base
est systématiquement en norme HTML (normalisation en amont). - Lorsqu'on utilise
#SAISIE
, on passe par|saisies_generer_html
qui du coup s'occupe de de faire les recherches > on a le même comportement entre#GENERER_SAISIES
et#SAISIE
. - Les lignes 194-213 de
inc/saisies_afficher.php
sont sorties dans une fonction à part histoire de pouvoir faire des tests unitaires et mutualiser.
- Au lieu de passer à
Le code que je cite n'est pas utilisé par #SAISIE non, c'est propre à l'API PHP.
Dans #SAISIE ça ajoute un "valeur" magiquement en amont depuis le tout début qui est #ENV{#ENV{nom}}
Et dans l'API PHP, on est plus malin et on va chercher plus de "valeur" possible.
Mais donc c'était possible parce que dès le début #VALEUR rajoutait un "valeur" au singulier, et donc dans tous les squelettes de saisies/ ça utilisait déjà cette valeur là.
Bon pour les erreurs, c'est que dans "base", donc si on change ça devrait pas toucher grand monde… (seulement les rares ayant surchargé). Donc théoriquement ça me parait pas une énorme casse si on va dans cette direction.
Je ne sais pas si le #ENV{nom} doit être normalisé par contre, ça peut poser des problèmes en cascade si yen a qui l'utilise pour des #ENV ou des table_valeur etc, quand c'est un chemin/comme/ça. C'est d'ailleurs l'avantage de choisir cette norme SPIP là : ensuite ça peut vraiment s'utiliser comme requête tel qel dans les #ENV, table_valeur, etc.
Bon pour les erreurs, c'est que dans "base", donc si on change ça devrait pas toucher grand monde… (seulement les rares ayant surchargé). Donc théoriquement ça me parait pas une énorme casse si on va dans cette direction.
en tout cas sur la zone, personne ne surcharge
saisies/_base.html
.Si vraiment on se préoccupe de cela, à la rigueur on ne met cette PR que dans Saisies v5.
Bon,
je me suis dit qu'on pouvait faire plus simple, en utilisant table_valeur sur
#ENV{erreurs}
. Sauf que si#ENV{erreurs}
a cette gueule`` [ 'tableau/cle/input' => 'erreur' ]
bah `|table_valeur{tableau/cle/input}` ne trouvera rien Et comme ce type de tableau c'est ce qui est renvoyé par `saisies_verifier`. Donc je pense qu'en fait on peut en rester à ma PR tel qu'elle est actuellement.
sinon l'autre solution qui est plus propre, c'est de faire une fonction
saisies_trouver_erreurs($tableau_erreurs, $nom)
qui s'occupe de trouver l'erreur quelque soit la forme du$tableau_erreurs
et de$nom
, et qui passeinterdire_script
dessus.Avantage :
- on ne casse rien à ce qui existe actuellement
- pas besoin de prenormaliser en amont
- on peut faire des tests unitaires
@maieul a fermé cette PR
@maieul a supprimé la branche issue261_etoile_message_erreur