PF_FormPrinter disallows html comments in field tags; e.g. show on select
Closed, ResolvedPublicBUG REPORT
Actions

Assigned To

Authored By

	Schtom
	Apr 21 2022, 7:41 AM

Description

List of steps to reproduce (step by step, including full links if applicable):

{{{field|samplefield|input type=dropdown|show on select=<!--
-->Name=>name;<!--
-->My name=>my_name;<!--
-->My name2=>my_name2;<!--
-->My name3=>my_name3;<!--
-->My name4=>my_name4;<!--
-->My name5=>my_name5;<!--
-->My name6=>my_name6;<!--
-->}}}

What happens?:
The form does not load fully and instead prints Error in form definition! The following field tag contains forbidden characters:

What should have happened instead?:
The form loads fully w/o errors

Software version (if not a Wikimedia wiki), browser information, screenshots, other information, etc.:
PageForms since this commit 40f5ad6b109f

HTML comments make template code more readable. We use long lists for show on selects. It's difficult to maintain code w/o being able to use comments in this case.

Details

	Subject	Repo	Branch	Lines +/-
	Remove HTML comments from form tags	mediawiki/extensions/PageForms	master	+2 -0

Customize query in gerrit

Related Objects

Mentioned Here: rEPFM40f5ad6b109f: Expand angled brackets check from "for template" to most field tags

Event Timeline

Schtom created this task.Apr 21 2022, 7:41 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 21 2022, 7:41 AM

Schtom renamed this task from PF_FormPrinter disallows html comments to PF_FormPrinter disallows html comments in field tags; e.g. show on select.Apr 21 2022, 7:46 AM

Schtom updated the task description. (Show Details)

if you agree that html comments are useful in this case, i would propose such a fix.

diff --git a/includes/PF_FormPrinter.php b/includes/PF_FormPrinter.php                                                                                                                                          
index f76a988a..d1689f13 100644
--- a/includes/PF_FormPrinter.php
+++ b/includes/PF_FormPrinter.php
@@ -1022,7 +1022,8 @@ END;
                        if ( count( $tagParts ) == 2 && $tagParts[0] == 'default filename' ) { 
                            continue;
                        }
-                       if ( strpos( $tag_component, '<' ) !== false && strpos( $tag_component, '>' ) !== false ) {
+                       $tag_component_to_test = str_replace( ['<!--', '-->'], '@COMMENT-MARKER@', $tag_component );
+                       if ( strpos( $tag_component_to_test, '<' ) !== false && strpos( $tag_component_to_test, '>' ) !== false ) {
                            throw new MWException(
                                '<div class="error">Error in form definition! The following field tag contains forbidden characters:</div>' .
                                "\n<pre>" . htmlspecialchars( $tag_component ) . "</pre>"

Shouldn't the contents within the comments also get removed/replaced?

In T306597#7872315, @Yaron_Koren wrote:

Shouldn't the contents within the comments also get removed/replaced?

good point. as the replacement only takes place within tag components, i assumed nobody would ever use html comments that contain html tags. i thought it was not worth adding "extra complexity". but in that case i'd add a preg_replace.

i am not a regex expert, so i used the one from stack overflow. you might wanna check the regex101 sample. the only drawback i see is that you cannot use comments within comments ( -->). but that would still trigger the original <, > check after the replacement.

diff --git a/includes/PF_FormPrinter.php b/includes/PF_FormPrinter.php                                                                                                                                          
index f76a988a..227ceb45 100644
--- a/includes/PF_FormPrinter.php
+++ b/includes/PF_FormPrinter.php
@@ -1022,7 +1022,8 @@ END;
                        if ( count( $tagParts ) == 2 && $tagParts[0] == 'default filename' ) { 
                            continue;
                        }
-                       if ( strpos( $tag_component, '<' ) !== false && strpos( $tag_component, '>' ) !== false ) {
+                       $tag_component_to_test = preg_replace('/(?=<!--)([\s\S]*?)-->/', '', $tag_component);
+                       if ( strpos( $tag_component_to_test, '<' ) !== false && strpos( $tag_component_to_test, '>' ) !== false ) {
                            throw new MWException(
                                '<div class="error">Error in form definition! The following field tag contains forbidden characters:</div>' .
                                "\n<pre>" . htmlspecialchars( $tag_component ) . "</pre>"

Change 881377 had a related patch set uploaded (by Techwizzie; author: Techwizzie):

[mediawiki/extensions/PageForms@master] Handle HTML comments in show on select field

https://gerrit.wikimedia.org/r/881377

gerritbot added a project: Patch-For-Review.Jan 19 2023, 8:29 AM

Change 881377 merged by jenkins-bot:

[mediawiki/extensions/PageForms@master] Remove HTML comments from form tags

https://gerrit.wikimedia.org/r/881377

Maintenance_bot removed a project: Patch-For-Review.Jan 20 2023, 5:30 PM

I hope the patch addresses this problem. Feel free to reopen this task if the problem persists

Techwizzie closed this task as Resolved.Jan 21 2023, 4:54 AM

Techwizzie claimed this task.

PF_FormPrinter disallows html comments in field tags; e.g. show on selectClosed, ResolvedPublicBUG REPORTActions

Description

Details

Related Objects

Event Timeline

PF_FormPrinter disallows html comments in field tags; e.g. show on select
Closed, ResolvedPublicBUG REPORT
Actions