Ajouter une profondeur maximale d'imbrication pour les détails repliables

[scavin]

Je souhaiterais suggérer l’ajout d’une profondeur maximale d’imbrication pour la fonctionnalité intégrée [details], ou l’introduction d’un paramètre de site permettant aux administrateurs de configurer cette limite.

Le problème

Récemment, un utilisateur de mon forum Discourse a créé un message contenant 24 niveaux de sections [details] imbriquées.

Cela a causé plusieurs problèmes :

1. Performance du navigateur : Les utilisateurs ont signalé que le développement de plusieurs niveaux rendait Firefox extrêmement lent ou non réactif.
2. Délais d’attente de l’endpoint Markdown : L’accès à l’endpoint .md du sujet est devenu extrêmement lent. Nous avons observé des requêtes prenant 19,8 secondes et 31,1 secondes.
3. Erreurs serveur : L’endpoint Markdown renvoyait parfois une page d’erreur « Oops ». Les journaux Discourse affichaient également des avertissements de délai d’attente du travailleur Pitchfork lors de la conversion Markdown.
4. Épuisement potentiel des ressources : Étant donné que les moteurs de recherche et les robots d’IA demandent fréquemment les endpoints .md, un message fortement imbriqué pourrait déclencher répétitivement un traitement coûteux et consommer les ressources du serveur.

Après la suppression du message problématique, l’endpoint Markdown a renvoyé HTTP 200 et a répondu en environ 1,7 seconde.

Le sujet affecté était :

https://meta.appinn.net/t/topic/87672

(La réponse problématique a déjà été supprimée.)

Amélioration suggérée

Je pense qu’il serait utile de :

• Limiter l’imbrication de [details] à une profondeur raisonnable, telle que 2 ou 3 niveaux.
• Ajouter un paramètre de site, tel que details_max_nesting_depth, permettant aux administrateurs de configurer la profondeur maximale d’imbrication.

Pour la compatibilité ascendante, le paramètre pourrait par défaut être illimité, tout en permettant aux administrateurs d’appliquer une limite si nécessaire.

Si les utilisateurs dépassent la limite configurée, Discourse pourrait rejeter le message avec un message de validation clair.

Solution temporaire : un plugin

J’ai créé un petit plugin qui limite l’imbrication de [details] à un maximum de 2 niveaux :

GitHub - scavin/discourse-details-depth-limit · GitHub

Le plugin valide les messages côté serveur et rejette les messages contenant plus de deux niveaux de sections repliables imbriquées.

Je l’ai testé avec succès dans mon environnement de développement local Discourse.

Pour les administrateurs rencontrant des problèmes similaires, ce plugin peut servir de solution temporaire jusqu’à ce qu’une solution officielle soit disponible.

Je crois qu’une limite configurable dans le plugin Details intégré serait une mesure de sécurité utile contre l’imbrication accidentelle ou excessive.

[Ethsim2 (Ethan)]

Bonjour @scavin, merci d’avoir signalé ce problème et d’avoir fourni les détails de reproduction.

J’ai enquêté sur le ralentissement du point d’entrée Markdown que vous avez décrit et j’ai identifié un problème dans MarkdownEndpoint::CookedProcessor#replace_details : les éléments <details> imbriqués étaient convertis de manière répétée, ce qui entraînait un traitement exponentiel.

J’ai ouvert une PR en brouillon en amont avec une correction :

https://github.com/discourse/discourse/pull/44461

À huit niveaux d’imbrication, la correction a réduit les conversions récursives de 255 à 8, avec une accélération d’environ 31 fois dans mon benchmark local. La PR a passé ses vérifications CI GitHub initiales.

Je travaille également sur le paramètre de site details_max_nesting_depth que vous avez proposé, au sein du plugin Details intégré, en utilisant 0 pour une imbrication illimitée afin de préserver le comportement existant. La première implémentation est écrite, mais ses tests d’intégration sont toujours en cours.

La correction de performances traite le problème de conversion Markdown côté serveur ; la limite configurable fournirait une mesure de sécurité supplémentaire, en particulier pour les performances côté navigateur.

Je serais ravi de recevoir vos retours sur le paramètre proposé et sur le fait de savoir s’il serait préférable de l’inclure dans la même PR ou de le soumettre séparément.

[scavin]

Merci pour l’investigation et le correctif rapides !
Je pense qu’il serait préférable de les séparer en deux PR, afin que le correctif de performance puisse être examiné et fusionné indépendamment, sans attendre la nouvelle configuration.
Je soutiens également la limite de profondeur configurable, avec 0 par défaut pour assurer la rétrocompatibilité.

[Ethsim2 (Ethan)]

Merci @scavin. Je vois l’intérêt de les séparer, bien que les deux modifications traitent du même rapport de base et soient désormais implémentées et testées en CI. J’ai activé les modifications de mainteneur sur tous mes PR, donc je suis tout à fait d’accord pour suivre la préférence de l’équipe Discourse, que ce soit pour examiner les deux commits ensemble ou les séparer.

[zogstrip]

merci @scavin pour le signalement et @Ethsim2 pour la PR j’ai également rendu la PR plus légère et fixé l’imbrication maximale à 10. De toute façon, je ne pense pas qu’on ait jamais besoin de supporter plus que ça.