Opened 5 years ago

Closed 18 months ago

#1485137 closed Feature Patches (fixed)

WasHtml Strips FORM Elements

Reported by: brian Owned by:
Priority: 5 Milestone: 0.7-beta
Component: PHP backend Version: 0.2-alpha
Severity: normal Keywords:
Cc:

Description

The update to the washtml library disabled the display of forms in emails. Most of the attributes are not allowed, so I would not expect it to have any security risk by enabling it. You cannot submit the forms or execute any JS actions.

I have received emails where the contents were wrapped in a <form> set of tags.

In the folder: /program/lib/ In the file: washtml.php In line: 77 Change the $html_elements to include 'form' as an allowed element.

Updated line:

  static $html_elements = array('a', 'abbr', 'acronym', 'address', 'area', 'b', 'basefont', 'bdo', 'big', 'blockquote', 'body', 'br', 'caption', 'center', 'cite', 'code', 'col', 'colgroup', 'dd', 'del', 'dfn', 'dir', 'div', 'dl', 'dt', 'em', 'fieldset', 'font', 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'hr', 'i', 'ins', 'label', 'legend', 'li', 'map', 'menu', 'ol', 'p', 'pre', 'q', 's', 'samp', 'small', 'span', 'strike', 'strong', 'sub', 'sup', 'table', 'tbody', 'td', 'tfoot', 'th', 'thead', 'title', 'tr', 'tt', 'u', 'ul', 'var', 'img', 'form');

Change History (7)

comment:1 Changed 5 years ago by alec

  • Milestone changed from later to 0.2-beta

comment:2 Changed 5 years ago by alec

  • Resolution set to fixed
  • Status changed from new to closed

Fixed in [4897adcb].

comment:3 Changed 5 years ago by thomasb

No, no! Forms should not be allowed.

In r16073/svn I've added a callback function (rcmail_washtml_callback) which turns form elements into <div class="form">. This way we allow the content of form tags to be displayed but without the danger of having forms within our html page.

Reverted in [d368a68e]

comment:4 Changed 19 months ago by estadtherr

  • Component changed from Client Scripts to PHP backend
  • Resolution fixed deleted
  • Status changed from closed to reopened

The callback doesn't seem to be working as intended. The washing process is turning this:

<table>
   ... other content ...
   <table>
     <tr>
       <a href="..."></a>
       <form name="main" method="GET" action="...">
         <td><input ... /></td>
         <td><img src="..."/></td>
         <td>
           <select ...>
             <option value="1">val1</option>
             <option value="2">val2</option>
             <option value="3">val3</option>
             ... more options ...
           </select>
         </td>
       </form>
     </tr>
   </table>
   ... other content ...
</table>

into this:

<table>
   ... other content ...
   <a href="..."></a>
   <div class="form"></div>
   <table>
     <tbody>
       <tr>
         <td><!-- input ignored --></td>
         <td><img src="..."/></td>
         <td><!-- select ignored --><!-- option ignored -->val1<!-- option ignored -->val2<!-- option ignored -->val3... more options ...</td>
       </tr>
     </tbody>
   </table>
   ... other content ...
</table>

which is rendered as a single line with all of the select options strung together:

val1val2val3...

As you can see, the div element which was seemingly intended to replace the original form element is in a strange place, and some of the original content has been rearranged.

I will try to track down the issue, but wanted to re-open the ticket to document the erroneous behavior.

Last edited 19 months ago by estadtherr (previous) (diff)

comment:5 Changed 19 months ago by alec

  • Milestone changed from 0.2-beta to 0.7-beta

I cannot confirm the form tag issue. It's replaced properly for me. However, I think select/input/radio/textarea fields should not be ignored while we're "disabling" the form.

comment:6 Changed 18 months ago by estadtherr

It turns out that the altered HTML formatting is a result of Safari's Web Inspector and not the output from Roundcube, so that part of it is a false alarm. However, I do agree with Alec that it would be better to show the form elements than unformatted selection values, even if we disable submitting of the form.

comment:7 Changed 18 months ago by alec

  • Resolution set to fixed
  • Status changed from reopened to closed

Fixed in [c1fcd1b8].

Note: See TracTickets for help on using tickets.