TextformatterSmartypants incompatible with TextformatterEntities

[Toutouwai]

Briefly describe the issue.

TextformatterSmartypants transforms "dumb" typography into "smart" typography, encoded as HTML entities. But it doesn't convert all necessary characters to entities (e.g. >) so it often needs to be used in conjunction with TextformatterEntities. But this results in either:

• the double-encoding of ampersands (Smartypants before Entities)
• or dumb typography (Entities before Smartypants)

What did you expect to happen?

TextformatterSmartypants and TextformatterEntities are able to be used together with normal functionality for each textformatter.

What did actually happen?

• the double-encoding of ampersands (Smartypants before Entities)
• or dumb typography (Entities before Smartypants)

Optional: Do you have screenshots/links that demonstrate the issue?

Bad:

Also bad:

Optional: Have you a suggestion for a possible fix?

The simplest fix would be to decode entities coming from Smartypants. In TextformatterSmartypants.module:

public function format(&$str) {
    require_once(dirname(__FILE__) . "/smartypants.php"); 
    // original
    // $str = \SmartyPants($str);

    // fixed
    $str = html_entity_decode(\SmartyPants($str));
}

Alternatively each entity in smartypants.php could be replaced with its UTF-8 equivalent.

What are the steps to reproduce the issue?

1. Apply both TextformatterSmartypants and TextformatterEntities to the same field, which contains dumb quotes.
2. Check the result of the textformatters on the front-end.

Tell us about the setup you experienced the bug in.

• ProcessWire version: 3.0.35

[ryancramerdesign]

As I understand it, SmartyPants and Markdown are usually meant to be used in combination together, like SmartyPants is something you add on when using Markdown. But of course it makes sense that one could use SmartyPants outside of that too. Do you think there's any possible issues with the suggested fix when used in combination with Markdown?

[tbba]

Just for info:
I use SmartyPants (without the Markdown module, but with CKE) all the time!

Editors tend to be not very accurate with these things and I am glad that the module exists to fix typing casualties there.

If languages are active, I even use the superior SuperSmartyPants. Both modules are not up-to-date unfortunately, but at least with the 3rd party SuperSmartyPants I can set all those entities freely.

[LostKobrakai]

Most people even just don't know there are various types of dashes or quotes. It's sometimes not even lazyness. So the formatter does make a lot of sense without markdown as well.

[ryancramerdesign]

Good to hear, I guess I didn't realize Smartypants had a broader usage context with existing HTML and stuff, that's great.

[ryancramerdesign]

One potential issue here is that Smartypants is required to be applied after Markdown according to the author (and those instructions we've duplicated in the module). So if we decode entities after the smartypants call (like in the example above), we're left with characters that should be encoded, that are no longer encoded (which would be a security problem in some instances).

[Toutouwai]

After some more investigation...

1. Smartypants doesn't work properly with the default settings if applied after Markdown. It needs the 'w' configuration option set, because Markdown converts double quote characters into ". Strange that the Smartypants author doesn't mention this seeing as he specifically mentions applying Smartypants after Markdown. Incidentally, if you set 'w' you also need to manually set the other configuration options to get the default functionality also - I think the config string would be 'qBdegw'.
2. As a consequence (and to make the module more flexible) TextformatterSmartypants should have a module config setting for defining Smartypants configuration options.
3. When using the Markdown textformatter, TextformatterEntities should also be applied subsequently for security's sake. This is because Markdown does not entity-encode single quote characters so there is still a potentially vulnerability to this type of XSS exploit. So if Smartypants were to be used with Markdown the double-encoding problem with TextformatterSmartypants/TextformatterEntities would still need to be solved.
4. So maybe the solution lies with TextformatterEntities rather than TextformatterSmartypants. The double_encode boolean could be set to false.

public function format(&$str) {
    static $charset = false;
    if($charset === false) $charset = $this->config->dbCharset;
    if($charset == 'utf8') $str = htmlspecialchars($str, ENT_QUOTES, "UTF-8", false);
        else $str = htmlspecialchars($str, ENT_QUOTES, null, false);
}

Or this could be a configurable option.

[ryancramerdesign]

Smartypants doesn't work properly with the default settings if applied after Markdown. It needs the 'w' configuration option set, because Markdown converts double quote characters into ". Strange that the Smartypants author doesn't mention this seeing as he specifically mentions applying Smartypants after Markdown. Incidentally, if you set 'w' you also need to manually set the other configuration options to get the default functionality also - I think the config string would be 'qBdegw'.

I have a feeling it's because we're no longer using the Markdown library from that author, but from a different library (Parsedown). So this must be a different behavior between the two libraries.

So maybe the solution lies with TextformatterEntities rather than TextformatterSmartypants. The double_encode boolean could be set to false.

I'd be reluctant to change this behavior, as I think there are potential security implications with turning off the double_encode option. Textformatter modules aren't configurable per-field, just as a module, so I don't think configuration is a good option here either. Probably the best bet is to see if we can get the TextformatterMarkdown to produce output consistent with the old module.

When using the Markdown textformatter, TextformatterEntities should also be applied subsequently for security's sake. This is because Markdown does not entity-encode single quote characters so there is still a potentially vulnerability to this type of XSS exploit.

Markdown allows for both Markdown and HTML. So it's not supposed to entity encode anything except text using markdown tags. Whereas the Markdown in this editor here is what's called "GitHub flavored markdown", which exhibits the additional encoding behavior you described. What we're using is regular Markdown, which is meant for trusted users. PW does provide a method that can be used for non-trusted users for Markdown via the $sanitizer->entitiesMarkdown() method.

One potential solution is that we modify the Smartypants module to use proper UTF8 characters rather than entities for the things that it does. I think the use of entities for em dash, smart quotes and such are a result of the library's age and the way things were done back then. It's possible I'm forgetting something, but I don't see any real benefit to using entities for these Smartypants enhancements anymore. What do you think?

[Toutouwai]

One potential solution is that we modify the Smartypants module to use proper UTF8 characters rather than entities for the things that it does. I think the use of entities for em dash, smart quotes and such are a result of the library's age and the way things were done back then. It's possible I'm forgetting something, but I don't see any real benefit to using entities for these Smartypants enhancements anymore. What do you think?

Sounds good - I actually made this modification myself some time ago so I could use TextformatterSmartypants together with TextformatterEntities.

I thought you might have been reluctant to make changes to smartypants.php because it would mean future releases of michelf/php-smartypants (which has received some recent updates, incidentally) cannot be simply dropped into TextformatterSmartypants. But maybe there are already PW-specific changes there anyway?

[ryancramerdesign]

Thanks for letting me know about the new SmartyPants version. I've plugged that in and noticed that it also has a method for doing what we are talking about: https://github.com/michelf/php-smartypants/blob/lib/Michelf/SmartyPantsTypographer.php#L151

The problem is the method doesn't seem like it works entirely, as it covers some cases and not others. Since there's no documentation for that method, it's very possible it's doing what it's intended to do. When you get a chance, can you try the latest version of the TextformatterSmartypants module on the dev branch and let me know if it resolves the issue? After doing a Modules > Refresh, You'll have to do the TextformatterSmartypants module configuration, and click on the checkbox for "UTF8 replacements".

Because the included method did some UTF8 replacements and not others, I added my own code to try and cover the ones it doesn't do. This code is commented out at present, because I'm not positive it's wise. That's because it could potentially decode HTML entities that were present before SmartyPants had it's way with it, which I think wouldn't be desirable. But if you want to try it, just uncomment the line that says "Uncomment this..." in the .module file.

[tbba]

Maybe it is also worth taking a look to the supersmartypants? That one is multilingual and has a cool way of storing setting directly in languages (if activated). Maybe, when reworking this all anyway, since PW is now multilingual, some ideas of that one could be included?

[Toutouwai]

Thanks @ryancramerdesign. I tested the new version and it works well with quotation marks but not so well with dashes (see below).

Looking at the replacements done with the aggressive UTF8 option nothing leaps out at me as being risky but you'll know better than me.

Could the aggressive UTF8 replacement be an option in the module configuration, with an appropriate warning? For several entities the UTF8 replacement built into Smartypants only replaces the entity if it's preceded by a space, but often you want to use an endash for a range where there would be no space: e.g. 5–7. And traditionally an emdash should be used without spaces either side when separating portions of a sentence.