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

Parsedown should not have an advisory? #44

Closed
taylorotwell opened this issue Feb 26, 2018 · 2 comments
Closed

Parsedown should not have an advisory? #44

taylorotwell opened this issue Feb 26, 2018 · 2 comments

Comments

@taylorotwell
Copy link

taylorotwell commented Feb 26, 2018

Parsedown is listed here as having a security advisory, but I'm not 100% sure it is warranted. It is a Markdown parsing library that takes the Markdown input it is given and turns it into HTML. Period. That is all it does. It is not output sanitizer and the author has stated that that is not a goal of the library. It already is possible to combine Parsedown with a sanitizer in order to sanitize output, for example:

use Parsedown;
use Stauros\Stauros;
use Stauros\HTML\Config;

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

$stauros = new Stauros(
    new Config([
        'a' => ['href' => true, 'title' => true],
        'b' => [],
        'br' => [],
        'blockquote' => ['cite' => true],
        'cite' => [],
        'code' => [],
        'em' => [],
        'i' => [],
        'p' => [],
        'q' => ['cite' => true],
        's' => [],
        'strike' => [],
        'strong' => [],
    ])
);

$html = $stauros->scanHTML(
    $parsedown->text($markdown)
);

IMO, these two things are separate concerns and forcing Parsedown to implement a full HTML sanitization feature or integrate with other libraries to do so when it can easily be done flexibly and easily in user-land doesn't make a ton of sense.

I would prefer someone other than @Ocramius review this issue as I'm not sure he can keep personal bias out of the discussion, since he fairly routinely tweets inflammatory things regarding libraries I have created. Thanks.

Thoughts?

@taylorotwell taylorotwell changed the title Parsedown status is questionable? Parsedown should not have an advisory? Feb 26, 2018
@Ocramius
Copy link
Member

Ocramius commented Feb 26, 2018

Please refer FriendsOfPHP/security-advisories#257

This repo just broadcasts the security issues

@aidantwoods
Copy link

@taylorotwell I explained a little in FriendsOfPHP/security-advisories#257 why this should be a bug in my opinion, but allow me to give an example because I feel it might be a helpful reference at least:

Parsedown has an option ->setMarkupEscaped(true). Among other things, the Wiki states that the goal of this is escaping HTML.
Except, take a look at this v.s. what it should do (with the patch applied)
screen shot 2018-02-26 at 17 51 02

As you can see, it is possible to get Parsedown to output arbitrary HTML when using this option (this is a bug with security consequences).
There are multiple other issues fixed in erusev/parsedown#495 too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants