Skip to content
Extraits de code Groupes Projets

security(#261): ne pas permettre d'introduire du JS dans les messages d'erreur

Fermé Maïeul requested to merge gh-0536ee7e/267/unknown/refs/pull/267/head into master

Rapports de requête de fusion

Fermée par avatar (janv. 22, 2025 1:12am UTC)

Loading

Activité

Filtrer l'activité
  • Approbations
  • Assignés et relecteurs
  • Commentaires (des bots)
  • Commentaires (des utilisateurs)
  • Branches et validations
  • Modifications
  • Labels
  • État de verrouillage
  • Mentions
  • État de la demande de fusion
  • Suivi
  • @maieul a ajouté 1 révision : 0b9e4edaa2d8ba1b31e5f638c3203631165daf14

  • Dans le core il reste quand même une étoile, et pas d'emploi de |interdire_script :

    <span class='erreur_message'>(#ENV*{erreurs/titre})</span>

    Quelle est la raison de faire différemment ?

  • Auteur Maintainer

    J'avais testé, et cela ne marchait pas. Ca m'avait aussi beaucoup étonné. J'imagine que le passage par set/get joue un role, mais pas le courage de tester plus.

  • Maintainer

    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 force-pushed 2 révisions : 0b9e4edaa2d8ba1b31e5f638c3203631165daf14 1531a5d6a2b9d652a9607bba36a25725fb8c1fef

  • Auteur Maintainer

    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.

  • @maieul a force-pushed 2 révisions : 1531a5d6a2b9d652a9607bba36a25725fb8c1fef 9cfe0ec8098c1ac439e30ddb3c7129a04e6341e7

  • Auteur Maintainer

    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 dans base.

    • 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.

  • Auteur Maintainer

    Je pense que ferais un rebasage avant la fusion pour scinder certains commits en 2.

  • @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.

  • Auteur Maintainer

    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 :

    1. Choix 1
    truc/bidule/chouette/lenomultime
    1. 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…

  • Auteur Maintainer

    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 suivante

    truc[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.

  • Auteur Maintainer

    Ah bah oui j'avais oublié qu'on passait valeur et pas valeurs (! valeur unique vs tableau). Alors que par contre on passe erreurs et pas erreur :)

    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:

    1. Au lieu de passer à saisies/_base.html erreurs on passe erreur en donnant directement l'erreur correspondante.
    2. Le #ENV{nom} dans saisies/_base est systématiquement en norme HTML (normalisation en amont).
    3. 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.
    4. Les lignes 194-213 de inc/saisies_afficher.php sont sorties dans une fonction à part histoire de pouvoir faire des tests unitaires et mutualiser.
  • 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.

  • Auteur Maintainer

    bah pour le coup le nom est deja normalisé.

  • Auteur Maintainer

    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.

  • Auteur Maintainer

    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.
  • Auteur Maintainer

    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 passe interdire_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 force-pushed 2 révisions : a0d0201ff3504d84a3d2e7daed3b08bb5b8bacda 7305e20c

  • Auteur Maintainer

    Je ferme suite à l'option alternative #299

  • @maieul a fermé cette PR

  • @maieul a supprimé la branche issue261_etoile_message_erreur

Veuillez vous inscrire ou vous connecter pour répondre
Chargement en cours