Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make notes, comments and announcements markdown aware #3814

Merged
merged 6 commits into from
Jul 2, 2019

Conversation

nilmerg
Copy link
Member

@nilmerg nilmerg commented Jun 4, 2019

Adds a new view helper Markdown which allows to transform markdown.

$this->markdown($text)

At the moment markdown is transformed for notes, comments and announcements. (yep, I thought this suits here as well)

resolves #3684
resolves #3441
resolves #2831

@nilmerg nilmerg added enhancement New feature or improvement area/ui Affects the user interface labels Jun 4, 2019
@nilmerg nilmerg added this to the 2.7.0 milestone Jun 4, 2019
@nilmerg nilmerg requested a review from lippserd June 4, 2019 13:44
@nilmerg nilmerg force-pushed the feature/make-notes-and-comments-markdown-aware-3684 branch from 936ec10 to 5ef2c55 Compare June 4, 2019 13:53
@lippserd
Copy link
Member

I don't see why we should limit the allowed tags or CSS attributes for either text or line. We had kind of the same problem with the the plugin output. We added more and more allowed tags and CSS attributes over time. I think users would like the full flexibility. The default configuration should be sufficient in terms of security. Maybe we should consider setting Parsedown's safe mode to true with Parsedown:: setSafeMode(). The Markdown class should cache the Parsedown and HtmlPurifier instances.

@nilmerg
Copy link
Member Author

nilmerg commented Jun 24, 2019

Tags are not limited anymore. I've just omitted the esoteric ones such as e.g. ruby, details and tt. Basically everything parsedown may possibly produce is allowed.

Enabling safe mode is a good idea, yes. Though, prior allowing all attributes I'd verify that things like onclick or onhover don't work.

The Markdown class doesn't need to cache anything as the Parsedown class already caches its own instance and caching the HtmlPurifier does not help much. I'd rather introduce a cache directory.

@nilmerg nilmerg force-pushed the feature/make-notes-and-comments-markdown-aware-3684 branch from 5ef2c55 to 97fe893 Compare June 25, 2019 11:23
@nilmerg
Copy link
Member Author

nilmerg commented Jun 25, 2019

Removed the attribute limitations. HtmlPurifier is already smart enough to remove all risky ones. (e.g. onclick)

Parsedown's safe mode however also escapes any raw markup and only allows markdown syntax. And now switching over to only markdown doesn't seem right. So safe mode is not an option.

@lippserd
Copy link
Member

Sounds good. But the HTML tags are still limited. Why not just allow them all?

@nilmerg nilmerg force-pushed the feature/make-notes-and-comments-markdown-aware-3684 branch from 97fe893 to c6a61f9 Compare June 25, 2019 13:01
@nilmerg nilmerg force-pushed the feature/make-notes-and-comments-markdown-aware-3684 branch from c6a61f9 to bd20607 Compare June 25, 2019 13:04
nilmerg added a commit that referenced this pull request Jun 25, 2019
nilmerg added a commit that referenced this pull request Jun 25, 2019
@nilmerg nilmerg force-pushed the feature/make-notes-and-comments-markdown-aware-3684 branch from 0ee9715 to 6659e28 Compare June 25, 2019 14:18
nilmerg added a commit that referenced this pull request Jun 25, 2019
@nilmerg
Copy link
Member Author

nilmerg commented Jun 25, 2019

Removed tag limitations and dropped the distinction between phrasing and block level content. It's all everything now.

Also, HtmlPurifier now got its own cache directory in our file cache.

@nilmerg nilmerg force-pushed the feature/make-notes-and-comments-markdown-aware-3684 branch from 6659e28 to 6aa2cf6 Compare June 27, 2019 12:41
@nilmerg nilmerg merged commit 36524bc into master Jul 2, 2019
@nilmerg nilmerg deleted the feature/make-notes-and-comments-markdown-aware-3684 branch July 2, 2019 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui Affects the user interface enhancement New feature or improvement
Projects
None yet
2 participants