Allow overriding escape method

[mozarcik]

Change self to static to allow override escape method.

[aidantwoods]

I'm not sure there is another way of reasonably implementing escape, is there a particular use case attached to wanting to override this method?

[mozarcik]

Oh yeah, we have use case when we do not allow any html input from user and only allow markdown syntax. We have this implemented by escaping everything before passing text to parsedown and that way we have double escaped code blocks. This will allow to disable escaping by parsedown.

Also i thought it would be no problem as escape method is protected so you can override it but it won't be used because of self. So maybe if you don't want this method to be overriden then it should be private?

[aidantwoods]

Oh yeah, we have use case when we do not allow any html input from user and only allow markdown syntax. We have this implemented by escaping everything before passing text to parsedown and that way we have double escaped code blocks. This will allow to disable escaping by parsedown.

In this case you probably still don't want to override the method, e.g. if you don't escape quotes (which you'd need to permit for some markdown syntaxes) then consider what would happen given the following markdown input:

[foo](bar 'baz" onmouseover="alert(1)')

Right now this will result in

<a href="bar" title="baz&quot; onmouseover=&quot;alert(1)">foo</a>

But if you stop the escape method from escaping, it'll instead be:

<a href="bar" title="baz" onmouseover="alert(1)">foo</a>

which is XSS.

The way in which markdown is required to be processed means that escaping input prior to parsing isn't sufficient (unless you actively counter markdown syntax specific vectors). You could perhaps reason about this by considering that escaping prior only guarantees safety as HTML, but not as interpreted markdown (and thus escaping prior isn't sufficient unless the markdown processor also has safety guarantees).

Incidentally you can still achieve XSS even if you escape quotes, via something like:

[xss](javascript:alert%281%29)

which outputs as

<p><a href="javascript:alert%281%29">xss</a></p>

Fortunately Parsedown has a safe-mode to deal with XSS from HTML input, and also with markdown specific vectors (like that last one), which I would advise using :)

Also i thought it would be no problem as escape method is protected so you can override it but it won't be used because of self. So maybe if you don't want this method to be overriden then it should be private?

It is protected, though that is more to do with visibility (i.e. it permits escape to be called by an extension). We could perhaps mark it as final, although calling it via the class name (i.e. self) is also sufficient to use the known implementation without the need to prevent overriding. final might better communicate the intent I suppose.

[mozarcik]

In this case you probably still don't want to override the method, e.g. if you don't escape quotes (which you'd need to permit for some markdown syntaxes) then consider what would happen given the following markdown input:

[foo](bar 'baz" onmouseover="alert(1)')

Right now this will result in

<a href="bar" title="baz&quot; onmouseover=&quot;alert(1)">foo</a>

But if you stop the escape method from escaping, it'll instead be:

<a href="bar" title="baz" onmouseover="alert(1)">foo</a>

which is XSS.

Yes, you are right but we are escaping text before passing it to parsedown so in this case we have

[foo](bar 'baz" onmouseover="alert(1)')

and parsedown produces:

<a href="bar" title="baz&quot; onmouseover=&quot;alert(1)">foo</a>

which is okay. But if user enter proper html such as:

<a href="bar" title="baz" onmouseover="alert(1)">foo</a>

then parsedown won't escape that and we have XSS. If we escape all text without modifying parsedown then if we have escaped that html properly but when user enter markdown:

```
<a href="bar" title="baz" onmouseover="alert(1)">foo</a>
```

then and we escape this text we have:

```
<a href="bar" title="baz" onmouseover="alert(1)">foo</a>
```

and then when we pass this text to parsedown then we have:

<pre><code>&amp;lt;a href=&amp;quot;bar&amp;quot; title=&amp;quot;baz&amp;quot; onmouseover=&amp;quot;alert(1)&amp;quot;&amp;gt;foo&amp;lt;/a&amp;gt;</code></pre>

which is double escaped.

Again we want to allow creating links (and some other elements) with parsedown such as: [foo](link) but we don't want to allow inserting html. I hope I'm clear.
Maybe there's better way to prevent html but allowing markdown but i didn't find it :)

[aidantwoods]

Yes, you are right but we are escaping text before passing it to parsedown so in this case we have

[foo](bar 'baz" onmouseover="alert(1)')

Indeed this will solve the concern about breaking out of quotes, but as mentioned, quotes are required for some markdown syntaxes (e.g. titles in link destinations), and so escaping them will break this type of markdown. Additionally this method still doesn't work, since escaping does nothing to address XSS caused by link destinations (inserting HTML directly isn't the only way of achieving XSS since markdown induces non-trivial transformations and converts perfectly safe characters from the escaper's perspective, to HTML—which isn't so safe).

Maybe there's better way to prevent html but allowing markdown but i didn't find it :)

Yes, there is! :) (as mentioned above) safe mode will deal with escaping HTML, as well as preventing things introduced by the markdown syntax (like javascript link destinations). Safe mode isn't enabled by default, because HTML is part of the markdown spec, so you've have to explicitly enable it by:

$parsedown = new Parsedown;
$parsedown->setSafeMode(true);

The benefit of using safe-mode over trying to combat the examples I've given here is that safe-mode is enforced by the parser (which understands the type of content it is producing from some given markdown syntax), and it'll also remain updated incase any new feature is added that permits similar danger to links.

[mozarcik]

Oh ok, thanks. I didn't notice that somehow :)