Opened 5 years ago

Closed 4 years ago

#17994 closed Bug (wontfix)

Markdown safe mode fails to return result if there's link in content

Reported by: simonas@… Owned by: nobody
Component: contrib.markup Version: 1.4
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no


I have been using

{{ comment.comment|markdown:"safe,nl2br"|nofollow }}

to render my comments and after moving to 1.4 I noticed that some comments fail to render.

After some testing I noticed that:

  1. Only comments with links fail to render.
  1. Safe mode is to blame (specifically this line). Disabling it (or setting enable_attributes to True) fixes the problem.

Attachments (2)

no_safe_mode.png (13.9 KB) - added by simonas@… 5 years ago.
Result when safe mode is disabled (expected)
safe_mode.png (28.2 KB) - added by simonas@… 5 years ago.
Result when safe mode is enabled (unexpected)

Download all attachments as: .zip

Change History (8)

comment:1 Changed 5 years ago by Preston Holmes

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Resolution: invalid
Status: newclosed

The markdown library has an interesting use of the term "safe mode" as it also enables the markdown syntax to inject JS attributes. see #17837

as such, the safe mode feature and markdown's "enable_attributes" were coupled in Django 1.4 :

So if you want to enable markdown features that are not safe, including links and attributes, the safe parameter must now be dropped.

A customized tag could always be used if you want to override Django's behavior, but these were tightly coupled as a security measure.

Last edited 5 years ago by Preston Holmes (previous) (diff)

comment:2 Changed 5 years ago by Carl Meyer

I would have closed this needsinfo, not invalid. From the provided information, it is not clear whether the OP is doing something unsafe, or there is a bug. Markdown links are not unsafe, and need to work for markdown to be useful. The ticket asserts that links are not working in safe mode; if that is true, it's a serious problem.

Also, we need a clearer definition of "fail to render". Turning off enable_attributes should not cause markdown to fail entirely, it should just cause the attribute extension constructs to be passed through as-is to the output instead of turning into HTML attributes.

simonas, could you provide the full markdown text of a comment that "fails to render," and clarify exactly what "fails to render" means?

Changed 5 years ago by simonas@…

Attachment: no_safe_mode.png added

Result when safe mode is disabled (expected)

Changed 5 years ago by simonas@…

Attachment: safe_mode.png added

Result when safe mode is enabled (unexpected)

comment:3 Changed 5 years ago by simonas@…

Resolution: invalid
Status: closedreopened

Sorry for lack of information.

First of all two comments:

[Google]( is search engine.
Google is search engine

Without safe mode enabled result looks like expected:

However when safe mode is enabled comment with link fails to render completely:

Some more examples of failing links:

[Hash link](#comment-xxx)
[Relative link](/great-article/)

By the way markdown.version = '2.1.0'

comment:4 Changed 4 years ago by Preston Holmes

Sorry for the premature closing of the ticket.

Unfortunately this looks like a bug in python-markdown

>>> markdown.markdown('[Google]( is search engine.',safe=True)
u'<p><a href="">Google</a> is search engine.</p>'
>>> markdown.markdown('[Google]( is search engine.',safe=True, enable_attributes=False)

This seems to be a flaw in the parsing that effects more than just links:

Not clear how to best triage this from the Django point of view. Django appears to be doing the right thing.

Certainly the markdown tag could use an upgrade that would potentially expose the newer options to the safe param in markdown (other than just safe=True). From the point of view of Django - the safe_mode and enable_attributes had to be coupled, because the option to the tag is only "safe", so it needs to be the safest it can be.

It also looks like the markdown issue referred to in #17837 has recently been reopened by the maintainer:

comment:5 Changed 4 years ago by Carl Meyer

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

Ugh. So pending fixes in the upstream Markdown library, we have the choice of insecure or broken.

Or we could switch to python-markdown2 - that's what I've done for my own projects, and have had no issues so far. This would have to be done over a deprecation path, or we could add support for markdown2 but maintain support for python-markdown; one would have to be a fallback for the other.

Marking release blocker as this is a serious regression that makes the "markdown" filter (practically speaking) unusable.

comment:6 Changed 4 years ago by Preston Holmes

Resolution: wontfix
Status: reopenedclosed

Because Django only provides a simple on/off 'safe' option to the markup tag, the output needs to be the most secure - which includes setting enable_attributes false. Hopefully python-markdown can get this bug fixed, if you are impacted by this bug you might:

Note: See TracTickets for help on using tickets.
Back to Top