Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: Template content being 'escaped' showing HTML entities #459

Merged
merged 3 commits into from Mar 9, 2020

Conversation

@Maaiins
Copy link
Collaborator

Maaiins commented Feb 28, 2020

Ref: #418

Copy link
Collaborator

padams left a comment

I'm not sure I'm following this one. What's the intent here?

$string = html_entity_decode($string, $quotes);

// replace all html tags, to prevent encoding
$string = preg_replace('#\<(.*?)\>(.*?)\</(.*?)\>#', '[html_open]\\1[html_close]\\2[html_open]/\\3[html_close]', $string);

This comment has been minimized.

Copy link
@Maaiins

Maaiins Mar 7, 2020

Author Collaborator

Displaying html content requires to escape all values but not the < and > of html code, which would happen when you simply run htmlentities() function on the content. When you don´t replace them, htmlentities() will replace them and you can not display any html content. So first replace them with a placeholder, this regex is searching for complete html tags and replaces < and >

This comment has been minimized.

Copy link
@Maaiins

Maaiins Mar 7, 2020

Author Collaborator

Described in #418


// recover html
$string = str_replace('[html_open]', '<', $string);
$string = str_replace('[html_close]', '>', $string);

This comment has been minimized.

Copy link
@Maaiins

Maaiins Mar 7, 2020

Author Collaborator

This is the second part to display html, replace the placeholders with < or >. Now well formed html is the result

return htmlentities($string, $quotes, $encoding);

// revert special chars, some values are saved encoded in the database eg. page title
$string = html_entity_decode($string, $quotes);

This comment has been minimized.

Copy link
@Maaiins

Maaiins Mar 7, 2020

Author Collaborator

This one is necessary because some values are stored already encoded with htmlentities() for example the product name in commerce_line_item
image

First of all you need to revert the encoding to prevent double encoding. Double encoding displays something like fl&auml;chenbrechnung in rendered templates and not flächenberechnung.

@Maaiins

This comment has been minimized.

Copy link
Collaborator Author

Maaiins commented Mar 7, 2020

There are two different scenarios:
Displaying html and displaying already escaped content.

I commented the explanation in code

@padams

This comment has been minimized.

Copy link
Collaborator

padams commented Mar 7, 2020

Got it. The only place i see this method used is by owa_template::makeLink which is being called in the construction of these error messages. Im really hesitant to allow about HTML in output as it's an XSS vector. Let me look at these error message strings and see what's happening here.

@Maaiins

This comment has been minimized.

Copy link
Collaborator Author

Maaiins commented Mar 7, 2020

That´s a point. Maybe writing a separated message function to display system controlled message would be the solution.

@padams

This comment has been minimized.

Copy link
Collaborator

padams commented Mar 8, 2020

OK. Here's what's going on. The error msg strings that are being passed to the template for display contain HTML tags - which is not a good idea for this very reason.

You can see these in owa/conf/messages.php which contains some of the error message strings (which in some cases are sprintf templates). This was just a lazy way to add style as far as I can tell. Other error msgs are hard coded variables in various views.

owa_base:: getMsg looks up the error msg and performs a sprintf substitution if required.

Then the view will assign the completely formatted msg to a variable that the msgs.tpl template will then display using the owa_template::out method -- which is paranoid about escaping html entities as I think it should be.

So i think the fix is to remove the HTML tags from error msg strings and NOT make the output sanitization allow for HTML entities.

So we have two options as I see it:

  1. strip all HTML tags out of the error msgs. (this s probably the easiest fix)

  2. Refactor error msgs to be more structured so that they can be styled properly at the template level. For example, we could restructure the error msg array to be an associate array that has discrete headline and msg parts. Example:

3601 => array('headline' => 'Error.', 'message' => array(Login failed using email %s, 1).'

Then we could refactor owa_base::getMsg to return an array with variable substitutions completed. for example:

array('headline' => 'Error.', 'message' => 'Login failed using email peter@foo.com.')

And then finally we could create owa_template::displayMsgs that applies HTML and does whatever escaping we think is warranted.

Thoughts?

Maaiins added 2 commits Mar 9, 2020
This reverts commit 08a5b45
);


<?php

This comment has been minimized.

Copy link
@Maaiins

Maaiins Mar 9, 2020

Author Collaborator

Complete refactoring of messages

if ( $version[0] < 5 && $version[1] < 2 ) {
$errors['php_version']['name'] = 'PHP Version';
$errors['php_version']['value'] = phpversion();
$errors['php_version']['msg'] = $this->getMsgAsString(3301);

This comment has been minimized.

Copy link
@Maaiins

Maaiins Mar 9, 2020

Author Collaborator

Unfortunately the whole document is marked as changed... there´s the real change

@@ -1,6 +0,0 @@
<span class="inline_h2">The form that you completed had some errors:</span>

This comment has been minimized.

Copy link
@Maaiins

Maaiins Mar 9, 2020

Author Collaborator

Moved to: modules/base/templates/msgs.tpl

* @param array $substitutions
* @return array
*/
function getMsg($code, $substitutions = []) {

This comment has been minimized.

Copy link
@Maaiins

Maaiins Mar 9, 2020

Author Collaborator

Refactored getMsg function. Now accepting an array of substitutions, cleaner solution with vsprintf

See:

  • modules/base/passwordResetRequest.php
endif;

// assign validation errors
if (!empty($this->data['validation_errors'])) {

This comment has been minimized.

Copy link
@Maaiins

Maaiins Mar 9, 2020

Author Collaborator

Removed the subview and separated rendering, moved into modules/base/templates/msgs.tpl

This comment has been minimized.

Copy link
@padams

padams Mar 9, 2020

Collaborator

great. pushing rendering down to the template level is so much cleaner.

* @param array $substitutions
* @return string
*/
public function getMsgAsString($code, $substitutions = [])

This comment has been minimized.

Copy link
@Maaiins

Maaiins Mar 9, 2020

Author Collaborator

Added function to return message as simple string instead of an array, see:

  • modules/base/classes/installController.php
  • modules/base/classes/installManager.php
  • modules/base/installCheckEnv.php
  • modules/base/updates/004.php
@Maaiins

This comment has been minimized.

Copy link
Collaborator Author

Maaiins commented Mar 9, 2020

I thought I deactivated all normalizations via git (Line endings or separator). But there is still trouble... Normally my git should commit 'as it is' and not normalizing to unix style. Hopefully it's ok that I just added comments on the specific parts and not reverted all.

@padams
padams approved these changes Mar 9, 2020
Copy link
Collaborator

padams left a comment

Nicely done man!

@padams padams merged commit 28591c0 into Open-Web-Analytics:master Mar 9, 2020
@Maaiins Maaiins deleted the Maaiins:fix_escaped_template_content branch Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.