Beaucoup de code pour un simple safehtml() partout à la fin #2

Open
opened 2 months ago by cerdic · 2 comments
cerdic commented 2 months ago
Owner

Bon, j'ai eu du mal à dérouler la pelotte pour comprendre ce que faisait vraiment le plugin.

En fait,

Donc en gros dès qu'on a une balise html on appelle echappe_js(), ce qui est le cas de tout ce qui passe dans propre()

Une fois dans echappe_js, ona cette rule un peu chelou

https://git.spip.net/spip-contrib-extensions/htmlpurifier/src/branch/master/wheels/spip/echappe-js.yaml#L24

match: "{<[a-z]+.*?($|>)}UisS"
avec un double "non greedy" qui s'annule, et donc en gros on prend toutes les balises et on les envoie dans la fonction echappe_anti_xss()

laquelle fait donc un safehtml() qui est un appel à la lib Purifier
https://git.spip.net/spip-contrib-extensions/htmlpurifier/src/branch/master/wheels/spip/echappe-js.php#L23

En résumé, beaucoup de surcharges de code et de complication pour juste faire un appel à purifier sur chaque balise html du contenu en post-propre et post-typo, ce qui pourrait se faire bien plus simplement par un appel depuis les 2 pipelines, une fois que SPIP a fait tout son travail.

(et éventuellement en annulant la fonction echapper_html_suspect() si elle nous embête plutôt qu'autre chose).

Bon, j'ai eu du mal à dérouler la pelotte pour comprendre ce que faisait vraiment le plugin. En fait, * la fonction `echapper_html_suspect()` appelée https://git.spip.net/spip-contrib-extensions/htmlpurifier/src/branch/master/inc/texte.php#L817 ne sert plus à rien puisque on echappe tout le html juste au dessus * tout se fait via la rule `securite-js` https://git.spip.net/spip-contrib-extensions/htmlpurifier/src/branch/master/wheels/spip/interdire-scripts.yaml#L21 qui dans le core est `if_match: "/<(?:script|iframe|embed|object|img|image|body|bgsound|meta)/iS"` et est devenue `if_match: "/<[a-z]+/iS"` Donc en gros dès qu'on a une balise html on appelle `echappe_js()`, ce qui est le cas de tout ce qui passe dans `propre()` Une fois dans echappe_js, ona cette rule un peu chelou https://git.spip.net/spip-contrib-extensions/htmlpurifier/src/branch/master/wheels/spip/echappe-js.yaml#L24 `match: "{<[a-z]+.*?($|>)}UisS"` avec un double "non greedy" qui s'annule, et donc en gros on prend toutes les balises et on les envoie dans la fonction `echappe_anti_xss()` laquelle fait donc un `safehtml()` qui est un appel à la lib Purifier https://git.spip.net/spip-contrib-extensions/htmlpurifier/src/branch/master/wheels/spip/echappe-js.php#L23 En résumé, beaucoup de surcharges de code et de complication pour juste faire un appel à purifier sur chaque balise html du contenu en post-propre et post-typo, ce qui pourrait se faire bien plus simplement par un appel depuis les 2 pipelines, une fois que SPIP a fait tout son travail. (et éventuellement en annulant la fonction `echapper_html_suspect()` si elle nous embête plutôt qu'autre chose).
Poster
Owner

Et surtout la conclusion, c'est que le plugin fait exactement ce qu'on veut éviter dans le core pour des raisons de performance, à savoir passer tous les contenus dans SafeHtml (implémenté via htmlpurifier ici).

C'est OK si un plugin propose ça - mais il devrait plutôt s'appeler PurifierPartout ou quelque chose du genre - mais on ne veut pas ce comportement par défaut dans le core, car cela alourdit énormément les traitements.

Je pense que ce que le plugin rate c'est que si on traite avant la transformation des raccourcis on peut être beaucoup plus efficace puique dans 90% des cas on aura aucune balise html dans le contenu, et donc on peut voir très vite qu'il n'y a rien a faire, et on sait que le html produit par la suite par les raccourcis SPIP est safe.

Alors qu'en intervenant après transformation des raccourcis, on est bon pour tout traiter tout le temps ce qui sort de propre(), car le contenu produit contiendra toujours du HTML, donc in faut sanitizer.

C'est moins grave pour typo puisque sauf html mis à la main dans un titre, le passage de typo sur titre ne produit pas de html en principe

Et surtout la conclusion, c'est que le plugin fait exactement ce qu'on veut éviter dans le core pour des raisons de performance, à savoir passer tous les contenus dans SafeHtml (implémenté via htmlpurifier ici). C'est OK si un plugin propose ça - mais il devrait plutôt s'appeler PurifierPartout ou quelque chose du genre - mais on ne veut pas ce comportement par défaut dans le core, car cela alourdit énormément les traitements. Je pense que ce que le plugin rate c'est que si on traite *avant* la transformation des raccourcis on peut être beaucoup plus efficace puique dans 90% des cas on aura aucune balise html dans le contenu, et donc on peut voir très vite qu'il n'y a rien a faire, et on sait que le html produit par la suite par les raccourcis SPIP est safe. Alors qu'en intervenant *après* transformation des raccourcis, on est bon pour tout traiter tout le temps ce qui sort de `propre()`, car le contenu produit contiendra toujours du HTML, donc in faut sanitizer. C'est moins grave pour typo puisque sauf html mis à la main dans un titre, le passage de typo sur titre ne produit pas de html en principe
Collaborator

C'est moins grave pour typo puisque sauf html mis à la main dans un titre, le passage de typo sur titre ne produit pas de html en principe

En natif SPIP, oui, mais avec le plugin Enluminures Typo, non, car le gras, les exposant, l'italique (et sans doute quelques autres) sont traités par le plugin.

C'est pareil avec orthotypo qui rajoute au exposants, des span pour les espaces fines...

> C'est moins grave pour typo puisque sauf html mis à la main dans un titre, le passage de typo sur titre ne produit pas de html en principe En natif SPIP, oui, mais avec le plugin Enluminures Typo, non, car le gras, les exposant, l'italique (et sans doute quelques autres) sont traités par le plugin. C'est pareil avec orthotypo qui rajoute au exposants, des span pour les espaces fines...
Sign in to join this conversation.
No Label
No Milestone
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.