Fix inline images not showing in FAQ/canned response editor

[wronex]

Format::htmlchars() was called on contents handed over to HTML editor. This would remove the image URLs to file.php. This fix keeps the file.php URLs intact making the image preview work while editing FAQs and canned responses.

[wronex]

Tested on v1.17.3 (ca95150)

[JediKev]

@wronex

I would not update htmlchars at all. The traditional way to use this is to wrap the content with viewableImages then that with htmlchars (eg. Format::htmlchars(Format::viewableImages($value))). If you were to update htmlchars you’d have to go through the whole codebase, find every instance we use both, and change those as well.

So my suggestion is completely revert the changes you’ve made and just wrap the content in both places with viewableImages like above.

Cheers.

[wronex]

@JediKev I agree, that is a much nicer solution. Better yet, the code is already there. If we pass false to Format::htmlchars() instead of true to skip the call to Format::sanetize().

I've updated the patch to include this change instead.

[wronex]

Now there is only one question. Why did someone call Format::htmlchars(..., true) in the first place?

[JediKev]

@wronex

What do you mean exactly? The true in your example is for $sanitize which determines wether or not to call Format::sanitize() within the htmlchars() method.

Cheers.

[JediKev]

@wronex

Your new changes are incorrect. You are just removing the true; this option is actually needed to further sanitize the content to prevent security vulnerabilities. You can view the Blame on that line and read the commit where it was introduced for more context. All you need to do is wrap the content being passed to htmlchars() with viewableImages().

Cheers.

[wronex]

Thank you for taking time to answer this pull-request.

You are unfortunately mistaken. What you are saying would result in a empty pull-request.

The issue this pull request is trying to solve is caused by calling Format::htmlchars with $sanetize = true on the result from Format::viewableImages().

This is the code today:

cannedresponse.inc.php:

// ...
$info['response'] = $canned->getResponseWithImages(); // = Format::viewableImages($canned->getResponse());
// ...
$info=Format::htmlchars(($errors && $_POST)?$_POST:$info, true);

As you can see we are already doing what you propose. The issue here is that Format::htmlchars(..., true) reverts the changes made by Format::viewableImages(). The call to Format::viewableImages() is useless inside Format::htmlchars(..., true).

$html = '<img src="cid:xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" data-image="zzz">';

viewableImages($html):
<img src="/file.php?key=XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX&expires=1234567890&signature=yyy" data-cid="xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" data-image="zzz">

htmlchars(viewableImages($html), false):
&lt;img src=&quot;/file.php?key=XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX&amp;expires=1234567890&amp;signature=yyy&quot; data-cid=&quot;xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx&quot; data-image=&quot;zzz&quot;&gt;

htmlchars(viewableImages($html), true):
&lt;img src=&quot;cid:XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX&quot; data-cid=&quot;xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx&quot; data-image=&quot;zzz&quot;&gt;

As far as I can tell the last result invalidates the image src URL. This causes issues when editing an FAQ or canned response. The editor cannot render the image since the source is a CID instead of an URL. The result is a "image" placeholder in the editor.

When calling Format::htmlchars(..., true) the content is passed to Format::localizeInlineImages() which "changes file.php urls back to content-id's".

But this can be fixed...

[JediKev]

@wronex

Well we need to look deeper for a fix (as this will affect other things). We will review the issue and make a suggested patch.

Cheers.

[wronex]

I've pushed a solution that introduces a version of Format::viewableImages that work on the results from htmlspecialchars(). I don't love this solution since there are now two copies of Format::viewableImages. But it has limited impact on the rest of the project at least. I will leave the rest to you! Good luck!

[wronex]

Here is some test-code that illustrates the problem (the output is above):

<?php

function htmlchars($var, $sanitize = false) {
  // ...
  if ($sanitize)
      $var = sanitize($var);
  // ...
  $flags = ENT_COMPAT;
  return htmlspecialchars((string)$var, $flags, 'UTF-8', false);
}

function sanitize($text, $striptags=false, $spec=false) {
  // ...
  $text = localizeInlineImages($text);
  // ...
  return $text;
}

function localizeInlineImages($text) {
    // Change file.php urls back to content-id's
    return preg_replace(
        '`src="(?:https?:/)?(?:/[^/"]+)*?/file\\.php\\?(?:\w+=[^&]+&(?:amp;)?)*?key=([^&]+)[^"]*`',
        'src="cid:$1', $text);
}

function viewableImages($html, $options=array()) {
  // ...
  return preg_replace_callback('/"cid:([\w._-]{32})"/',
    function($match) use ($options) {
      // ...
      return sprintf('"%s" data-cid="%s"', 
                     getDownloadUrl($options), $match[1]);
    }, $html);
}

function getDownloadUrl($options=array()) {
  // ...
  $handler = "/file.php";
  $args = array(
      'key' => "XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX",
      'expires' => "1234567890",
      'signature' => "yyy",
  );
  // ...
  return sprintf('%s?%s', $handler, http_build_query($args));
}

$html = '<img src="cid:xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" data-image="zzz">';

echo '$html = ' . $html . ";\n\n";

echo 'viewableImages($html):'."\n";
echo viewableImages($html) . "\n\n";

echo 'htmlchars(viewableImages($html), false):'."\n";
echo htmlchars(viewableImages($html), false) . "\n\n";

echo 'htmlchars(viewableImages($html), true):'."\n";
echo htmlchars(viewableImages($html), true) . "\n\n";

[JediKev]

@wronex

I reviewed our code and I see what you mean. After review, all we really need is a simple Format::display($content, true, false); in some places. However, since this same concept affects a lot of other places with inline images I'm looking to see if/where we can refactor things. I'll make an appropriate patch once I have it all figured out.

Cheers.