Code

Opened 2 years ago

Closed 2 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

Description

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@… 2 years ago.
Result when safe mode is disabled (expected)
safe_mode.png (28.2 KB) - added by simonas@… 2 years ago.
Result when safe mode is enabled (unexpected)

Download all attachments as: .zip

Change History (8)

comment:1 Changed 2 years ago by ptone

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

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 : https://docs.djangoproject.com/en/dev/releases/1.4/#attributes-disabled-in-markdown-when-safe-mode-set

So if you want to enable markdown features that are not safe, just do not pass the "safe" parameter to the tag.

Version 0, edited 2 years ago by ptone (next)

comment:2 Changed 2 years ago by carljm

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 2 years ago by simonas@…

Result when safe mode is disabled (expected)

Changed 2 years ago by simonas@…

Result when safe mode is enabled (unexpected)

comment:3 Changed 2 years ago by simonas@…

  • Resolution invalid deleted
  • Status changed from closed to reopened

Sorry for lack of information.

First of all two comments:

[Google](http://google.com) is search engine.
Google is search engine

Without safe mode enabled result looks like expected: https://code.djangoproject.com/raw-attachment/ticket/17994/no_safe_mode.png

However when safe mode is enabled comment with link fails to render completely: https://code.djangoproject.com/raw-attachment/ticket/17994/safe_mode.png

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 2 years ago by ptone

Sorry for the premature closing of the ticket.

Unfortunately this looks like a bug in python-markdown

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

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

https://github.com/waylan/Python-Markdown/issues/87

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:

https://github.com/waylan/Python-Markdown/issues/82

comment:5 Changed 2 years ago by carljm

  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

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

Or we could switch to https://github.com/trentm/python-markdown2 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 2 years ago by ptone

  • Resolution set to wontfix
  • Status changed from reopened to closed

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:

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.