`safe_mode='escape'` problems

[justanotheranonymoususer]

I tried to use safe_mode and saw some unexpected behavior. Below are the problems I encountered. I didn't want to spam the issues, but let me know if you prefer an issue per item.

Version: 9a88ce1

import markdown2

# <code> tags are escaped inside links.
# Expected: <p><a href="https://example.com"><code>example.com</code></a></p>
# Actual:   <p><a href="https://example.com">&lt;code>example.com&lt;/code></a></p>
markdown_text = R"[`example.com`](https://example.com)"
print(markdown2.markdown(markdown_text, safe_mode='escape'))

# No handling of escaped HTML characters.
# Expected: <p>&lt;abc&gt;&amp;amp;</p>
# Actual:   <p>\&lt;abc\&gt;\&amp;</p>
markdown_text = R"\<abc\>\&amp;"
print(markdown2.markdown(markdown_text, safe_mode='escape'))

# Excessive escaping in inline code.
# Expected: <p>Path: <code>C:\&lt;folder 1&gt;</code></p>
# Actual:   <p>Path: <code>C:\&amp;lt;folder 1&gt;</code></p>
markdown_text = R"Path: `C:\<folder 1>`"
print(markdown2.markdown(markdown_text, safe_mode='escape'))

# Fenced code blocks joined to list.
#
# Expected:
# <ul>
# <li>Item 1</li>
# <li>Item 2</li>
# </ul>
#
# <pre><code>// Some code
# </code></pre>
#
# Actual:
# <ul>
# <li>Item 1</li>
# <li>Item 2
# <pre><code>// Some code
# </code></pre></li>
# </ul>
markdown_text = R"""
* Item 1
* Item 2

```
// Some code
```
"""
print(markdown2.markdown(markdown_text, safe_mode='escape',
                         extras={'fenced-code-blocks': None}))

# Asterisks not turned to html.
# Expected: <p>&lt;<em>test</em>&gt;</p>
# Actual:   <p>&lt;*test*&gt;</p>
markdown_text = R"<*test*>"
print(markdown2.markdown(markdown_text, safe_mode='escape'))

# Closing HTML tags not escaped.
# Expected: <h1>&lt;some text&gt;</h1>
# Actual:   <h1>&lt;some text></h1>
markdown_text = R"# <some text>"
print(markdown2.markdown(markdown_text, safe_mode='escape'))

# Escaped backslashes not handled.
#
# Expected:
# <p>a\b
# &lt;x&gt;\&lt;y&gt;</p>
#
# Actual:
# <p>a\b
# &lt;x&gt;\\&lt;y&gt;</p>
markdown_text = R"""
a\\b
<x>\\<y>
"""
print(markdown2.markdown(markdown_text, safe_mode='escape'))

# HTML comments are escaped, should be removed or left alone.
# Expected: <p>test  test2</p>
# Actual:   <p>test &lt;!-- comment --&gt; test2</p>
markdown_text = R"test <!-- comment --> test2"
print(markdown2.markdown(markdown_text, safe_mode='escape'))

# Closing quotes aren't escaped.
# Expected: <p>test&gt;</p>
# Actual:   <p>test></p>
markdown_text = R"test>"
print(markdown2.markdown(markdown_text, safe_mode='escape'))

[Crozzers]

I've gone through each of these cases and fixed a few, which I intend to submit a PR for. Had some questions about some of the examples though.

# No handling of escaped HTML characters.
markdown_text = R"\<abc\>\&amp;"

I'm not sure what exactly this snippet is trying to do?
For the tag you're expecting \< to turn into <, but in safe mode this is redundant as the angles get escaped anyway. Remove the redundant escape and you get what you want.

# Asterisks not turned to html.
markdown_text = R"<*test*>"

This is essentially a HTML/autolink tag, even if it's invalid, so safe mode treats it like markup. The angles get escaped and then the tag contents are hashed to prevent them from being processed.
I'm not sure this needs fixing outright as you can write <*test*>

# Closing HTML tags not escaped.
markdown_text = R"# <some text>"

I ran this one through babelmark and most other processors either mishandle this or do it the same way as us. Don't see a reason to fix this

# HTML comments are escaped, should be removed or left alone.
markdown_text = R"test <!-- comment --> test2"

This behaviour is intentional. See #569 and #563

# Closing quotes aren't escaped.
markdown_text = R"test>"

Closing angles don't really need to be escaped, and this is how the library works in most places. Opening angles are escaped but closing ones are not

[justanotheranonymoususer]

Thanks for the reply. General notes:

• "most other processors either mishandle this" - I don't think this should be used as a general excuse. If other projects misbehave, this project shouldn't misbehave as well.
• Let's separate between "works as intended" and "not a priority"/"too much work". It's OK to say that something is not important enough for now, but still agree that something can ideally be improved.
• "I'm not sure this needs fixing outright as you can write" - I'm dealing with a large amount of existing documents, so this is not an option. Generally, this is about the library doing the right thing.

# No handling of escaped HTML characters.
markdown_text = R"\<abc\>\&amp;"

For the tag you're expecting \< to turn into <, but in safe mode this is redundant as the angles get escaped anyway

\<abc\>\&amp; should be displayed as <abc>&amp;, with or without safe mode. Aside from explicit tags such as <b> etc., there should be no difference in the result of non-safe mode vs. safe mode. It should make the operation safe, not change the semantics.

# Asterisks not turned to html.
markdown_text = R"<*test*>"

The hashing part is an implementation detail. Ideally I think that safe mode should treat < and > as text, and keep treating the rest as markdown. In this case, I have some content in which the author meant for it to be emphasized, and I think it makes sense to expect it to work.

Closing angles don't really need to be escaped, and this is how the library works in most places. Opening angles are escaped but closing ones are not

This is harmless in most cases, but could have effect if there's an opening angel concatenated to the result for some reason. It'd be safer to escape them as well.

[Crozzers]

It should make the operation safe, not change the semantics

Yeah that's fair. Pushed a commit to fix that test case

Ideally I think that safe mode should treat < and > as text, and keep treating the rest as markdown

That works in this test case, but take the following examples:

<div class="some_long_classname">Some text</div>
<div class="_my class name">my_filename<div>
<div style="/*CSS comment*/"></div>

If we escape the angles and just process the rest as markdown we'll get a bunch of text emphasised:

&lt;div class="some<em>long</em>classname"&gt;Some text&lt;/div&gt;
&lt;div class="<em>my class name"&gt;my</em>filename&lt;div&gt;
&lt;div style="/<em>A CSS comment</em>/"&gt;&lt;/div&gt;

Examples 1 and 2 could potentially be fixed by disabling middle-word-em, but regardless I think this would be very unexpected behaviour for the author.

I think for safe mode, HTML is HTML and it should be escaped and sanitised, not processed as markdown. If a user wants to process it as markdown then it should be escaped.

With the latest version of my branch I tested the following snippet, which I think is the best compromise:

<*test>  # &lt;*test*&gt;
\<*test*>  # &lt;<em>test</em>&gt;

[justanotheranonymoususer]

With the latest version of my branch I tested the following snippet, which I think is the best compromise:

I think this is an acceptable compromise. Maybe I'll be able to pre-process to handle this on my side, but I think it's a reasonable choice for the library. Thanks.

[Crozzers]

Raised a PR to close this issue.

Regarding the following unresolved examples:

# Closing HTML tags not escaped.
# <some text>
# Closing angles aren't escaped.
test>

I don't think these are bugs that need to be fixed as they generate valid HTML and aren't causing issues, even if they're not perfect. Although you can always spin out a separate issue if you feel strongly about it

[justanotheranonymoususer]

Not feeling strongly enough, it can be closed, thanks. It may bite me one day but hopefully it won't. Currently, the other issues I created have more significant impact.