Item11480: Contents of HTML input blocks should not be rendered as wikitext.

When editing a formfield value that contains an exclamation marker (!) to prevent autolinking (poor man's nop) then these formfields are displayed properly.

Not so when generating the HTML markup manually or by any plugin in the middle of the rendering loop. Basically any exclamation marker will be replaced with nops and these are then kicked out at the end of the rendering loop finally.

The bug now is that this must not happen to stuff part of a HTML attribute, i.e. a value attribute of an input field.

Example:

Formfield value is:

!WikiWord !WikiWord !WikiWord

HTML markup:

<input type="text" value="!WikiWord !WikiWord !WikiWord" />

Renders as:

<input type="text" value="!WikiWord WikiWord WikiWord" />

Should be:

<input type="text" value="!WikiWord !WikiWord !WikiWord" />

Test:

Strange thing is: it looks fine part of an "edit" screen rendering the DataForm using ROWVALUE and whatever hack-mack is going on there in lib/Foswiki/UI/Edit.pm locally.

-- MichaelDaum - 30 Jan 2012

It really helps fix these things if a test could be written. A 5 line test in FormattingTests exposes the issue. The reason the first WikiWord is not converted to a nop, is because it's not a wikiword. (value="! ) I suspect the quote prevents it from being detected as a WikiWord, so it is not escaped. Still trying to trace my way through Render.

Please write tests!

+# WikiWord in Input Field
+sub test_escapedWikwordFormfield {
+    my $this     = shift;
+    my $expected = <<EXPECTED;
+<input type=text value=!WikiWord !WikiWord !WikiWord>
+EXPECTED
+
+    my $actual = <<ACTUAL;
+<input type="text" value="!WikiWord !WikiWord !WikiWord" />
+ACTUAL
+    $this->do_test( $expected, $actual );
+}

Sven points out that same issue exists for other formatting:

- <input type=text value=!WikiWord !WikiWord !WikiWord *bold* __boldItalic__ >
+ <input type=text value=!WikiWord <nop>WikiWord <nop>WikiWord <strong>bold</strong> <strong><em>boldItalic</em></strong> >

I'm going to defer this to 1.2, it's a long-standing issue, and there is enough good stuff in 1.1.5. I suspect changing render is too high risk.

• Formatting should not apply to input fields
• Macro expansion however must apply.

Checking in the test with and expected fail.

-- GeorgeClark - 06 Mar 2012

Michael,

The following diff seems to work well. Fixes the issue, and all unit tests pass. But touching Render.pm is much to high risk for 1.1.5. Please test this if you can and we can plan it for 1.2.

diff --git a/core/lib/Foswiki/Render.pm b/core/lib/Foswiki/Render.pm
index 957ff44..3f3f51a 100644
--- a/core/lib/Foswiki/Render.pm
+++ b/core/lib/Foswiki/Render.pm
@@ -1188,6 +1188,10 @@ sub getRenderedVersion {
         $text = $rt;
     }
 
+    $text =
+      $this->_takeOutProtected( $text, qr/<input\b.*?\/>/si, 'input',
+        $removed );
+
     # Escape rendering: Change ' !AnyWord' to ' <nop>AnyWord',
     # for final ' AnyWord' output
     $text =~ s/$STARTWW\!(?=[\w\*\=])/<nop>/gm;
@@ -1422,6 +1426,7 @@ sub getRenderedVersion {
         \&verbatimCallBack );
     $text =~ s|\n?<nop>\n$||o;    # clean up clutch
 
+    $this->_putBackProtected( \$text, 'input', $removed );
     $this->_putBackProtected( \$text, 'script', $removed, \&_filterScript );
     Foswiki::putBackBlocks( \$text, $removed, 'literal', '', \&_filterLiteral );
     $this->_putBackProtected( \$text, 'literal', $removed );

-- GeorgeClark - 06 Mar 2012

I think that the putBack for input fields needs to be just a bit earlier than the above shows. Probably just before the "pre" blocks are restored. This way the deprecated endRenderingHandler will still see the input fields.

-- GeorgeClark - 06 Mar 2012

I'm concerned that special-casing the input tag will just push this problem into different edge-cases, also another corner of Foswiki::Render that must be maintained.

Is this bug really that Foswiki::Render touches <input.../> tags at all?

We should promote the use of <literal> and friends, when you don't want the render to mess with things.

Maybe George's fix is simple enough, but I am uneasy about it. It might be the correct path forward but let's consider it carefully.

-- PaulHarvey - 06 Mar 2012

Also consider mangled alt or title attributes on <img .../> tags and others; this isn't a problem specific to <input.../>. Example:

<a href="#" title="This _is_ interesting, isn't it">interest</a>

interest

-- PaulHarvey - 06 Mar 2012

After a little discussion on IRC, I think I want us to make a proposal for Foswiki 1.2 that we simply banish renderTML from the insides of all <html tags> generally.

The challenge will be to find a case where you ever want rendered output inside a tag (attribute value or otherwise), I don't think there are any - AFAIK HTML isn't supposed to have "nested" < or > brackets at all ('inner' < or > brackets should be encoded as HTML entities, i.e. &lt; & &gt;).

-- PaulHarvey - 06 Mar 2012

Producing html within an html tag should be prevented. So stopping the renderer from doing so seems obvious.

Producing escaped html should be possible, though I am not able to do it right now either using something like:

<input type="text" value="..." title="%ENCODE{"here comes some __important__ tooltip" type="html"}%" />

Maybe I am using the wrong type for %ENCODE. The idea is to have a tooltip displaying formatted content using some tooltip library.

That's basically the only use case I can think of where you'd require escaped html in attribute position of an html tag. This however is so marginal that I think we can ignore it as tooltip libraries have different approaches to load rich tooltips anyway.

-- MichaelDaum - 06 Mar 2012

The Render.pm diff above breaks SolrSearch result topic links.
Here are my modifications to make [[web.topic]] clickable again.

$ svn diff Render.pm
Index: Render.pm
===================================================================
--- Render.pm   (revision 14645)
+++ Render.pm   (working copy)
@@ -1226,9 +1226,9 @@
     }
 
     # Remove input fields: Item11480
-    $text =
-      $this->_takeOutProtected( $text, qr/<input\b.*?\/>/si, 'input',
-        $removed );
+    #$text =
+    #  $this->_takeOutProtected( $text, qr/<input\b.*?\/>/si, 'input',
+    #    $removed );
 
     # Escape rendering: Change ' !AnyWord' to ' <nop>AnyWord',
     # for final ' AnyWord' output
@@ -1461,7 +1461,7 @@
     }
 
     # Restore input fields before calling the end/post handlers
-    $this->_putBackProtected( \$text, 'input', $removed );
+    # $this->_putBackProtected( \$text, 'input', $removed );
 
     Foswiki::putBackBlocks( \$text, $removed, 'pre' );

-- UlrichLeodolter - 21 Apr 2012

From what I can see your diff simply removes the protection we added for input fields. That's not an acceptable fix. Could you try to explain how removal of <input.../> fields breaks links? Maybe provide a verbatim example of the html rendered with the patch installed.

It's incorrect for foswiki to do any rendering of markup inside input fields. They should be unmodified when sent to the browser. And the fix should not in any way be impacting <a> clickable links.

-- GeorgeClark - 21 Apr 2012

I haven't seen any breackage in SolrSearch, at least not related to protecting input fields.

-- MichaelDaum - 27 Aug 2012

The fix for this is flawed. The results can be seen on http://trunk.foswiki.org/Sandbox/ApacheConfigGenerator The problem is that input fields can be written as self closing, or with a separate closing element. For ex:

<input type="radio" name="blah" > Label </input>
<input type="submit" value="Submit"/>

The original fix will run past a non-self-closed input tag and remove everything up to the end of the next self closing tag.

-- GeorgeClark - 03 Nov 2012

The solution (or at least the next try) is to not worry about how the tag is closed. All we really want to protect is markup inside the tag. So removing up to the first > appears to be sufficient.

-- GeorgeClark - 03 Nov 2012

There is one side effect. Because we used to render input fields, converting wikiwords to links, etc. they were being protected by using <nop> in them. In particular the login template protected the pre-filled user name field, and the %SIGNATUREFORMAT% included a <nop> to prevent the wikiname from expanding inside the signature cut/paste box.

Now that render has been changed to protect input fields from being rendered, the nop's are unnecessary, and actually break the html. It's not legal to have un-escaped html inside a html tag. The closing > of the nop was being detected as the close of the input tag.

This was showing up in the HTMLValidationTests.

-- GeorgeClark - 04 Nov 2012

I have an ugly hack to Render.pm that lets the <input> field survive with embedded nop tags. I don't have any examples of needing to handle literal tags, but Arthur mentioned them on IRC.

+# Temporary marker for <nop> tags. They are used as follows:
+#  - Remove all <nop> tags
+#  - Take out <input ..> tags
+#  - Restore all <nop>
+#  - ... do other rendering
+#  - Put back all <input ...> tags
+#  - Remove any extraneous nop markers that come back in with the input blocks.
+our $NOPMARK = "\2";
+
     # Remove input fields: Item11480
+    $text =~ s/<nop>/N$NOPMARK/g;
     $text =
       $this->_takeOutProtected( $text, qr/<input\b.*?>/si, 'input', $removed );
+    $text =~ s/N$NOPMARK/<nop>/g;
 
<rendering occurrs>
 
     # Restore input fields before calling the end/post handlers
     $this->_putBackProtected( \$text, 'input', $removed );
+    $text =~ s/N$NOPMARK//g;

I've tested it and the HTML Validation tests pass with the old <nop>'s still in place.

Arthur, Babar, Any thoughts on whether this is needed? I don't like asking users to fix up their forms because of the render change. I can't think of any better solution within the constraints of our current rendering.

I've updated the above. The attempt to remove <literal> tags won't work. (Sorry, forgot to force a new revision so you don't get to see my broken code handing literals). Literals are removed much earlier in the rendering process, replaced with a HTML comment, which also breaks the "takeoutblocks for input tags. Other than the overhead of scans to remove, replace, and remove again the encoded nops, it seems to work fine.

Are <literal> tags an issue? The fix for them will be more complicated.

-- GeorgeClark - 04 Nov 2012

Now that's broken even more:

(see http://trunk.foswiki.org/Sandbox/InputTest)

You type:

<div style="padding:1em;border:1px solid red">
  <input type="hidden" name="topic" id="topic" value="%IF{"defined NAME" then="%NAME%" else="<nop>%BASETOPIC%"}%" />
</div>

You get:

Simulated:

The closing > is rendered as a > instead.

This ...

--- lib/Foswiki/Render.pm       (revision 15911)
+++ lib/Foswiki/Render.pm       (working copy)
@@ -292,7 +292,7 @@
 
     # Remove input fields: Item11480
     $text =
-      $this->_takeOutProtected( $text, qr/<input\b.*?>/si, 'input', $removed );
+      $this->_takeOutProtected( $text, qr/<input\b.*?\/>/si, 'input', $removed );

... fixes it but that's probably bringing back something else. Not sure why my prev fix was flawed, Though I did see TML not being rendered when there's a preceding non-closing input element.

-- MichaelDaum - 05 Nov 2012

nop is now protected within an input field. literal will be much more difficult to solve, as it was temporarily removed much earlier in the rendering. I think that this is good to go as is.

-- GeorgeClark - 09 Nov 2012

Seems fine now. Thanks.

-- MichaelDaum - 09 Nov 2012

No idea why this is waiting for me. I just expressed the opinion that parsing HTML with regex is bound to fail, but that's all we have for now. I think the current fix is as good as it can get without rewriting the renderrer.

Removing myself from the list, and waiting for Arthur.

-- OlivierRaginel - 09 Nov 2012

Changing to Waiting For Release.

-- GeorgeClark - 27 Nov 2012

Commenting is disabled while not logged in