Add a new html_input option

[mnapoli]

See #253, this is still WIP. That was easier than I thought, however I think it will require some more thinking. I'm pushing it "as-is" so that we can progress on what to do:

• what name for the option controlling "allowing unsafe links"?
• do the constants on the Environment class make sense to you?

Feel free to send any other comment about the code, including small coding style stuff, I don't mind at all.

The new html_input option allows 3 different behaviors:

• strip HTML input
• allow HTML input
• escape HTML input

[mnapoli]

What I first wanted to do is to remove the safe option completely and, if set by the user, cascade its value to the new html_input option (if it's not set). However I'm not sure the Environment class or the Configuration class is the best place for adding such custom logic about a specific option, so as of now I have kept safe as it is.

Maybe it would make sense to have "getters" for each option value on the Configuration class? That would allow implementing some logic for each config value. But that's maybe a bit too much changes for a single pull request.

[mnapoli]

FYI I have opened a discussion on Reddit about "how secure" escaping HTML could be: https://fr.reddit.com/r/PHP/comments/4qbf4g/improving_handling_of_untrusted_html_input_in/

[colinodell]

I'm wondering if maybe we need 4 options:

• strip - Don't render any HTML nodes
• allow_raw - Render HTML nodes as-is
• allow_filtered - Run HTML content through HTMLPurifier and render that
• escape - Escape HTML entities

[colinodell]

What I first wanted to do is to remove the safe option completely and, if set by the user, cascade its value to the new html_input option (if it's not set). However I'm not sure the Environment class or the Configuration class is the best place for adding such custom logic about a specific option, so as of now I have kept safe as it is.

That's fine. I'd be okay tackling that afterwards (as a separate PR).

Maybe it would make sense to have "getters" for each option value on the Configuration class? That would allow implementing some logic for each config value. But that's maybe a bit too much changes for a single pull request.

Its possible for other developers to create extensions which have their own configuration options. I don't know if your proposed change might lead them to think they must extend Configuration to add their own getters.

What name for the option controlling "allowing unsafe links"?

How about allow_unsafe_links (boolean)?

Do the constants on the Environment class make sense to you?

I like the idea of constants. Perhaps we can put them on CommonMarkCoreExtension instead? Thoughts?

[colinodell]

Thinking out loud again:

Putting the constants on CommonMarkCoreExtension keeps them out of the general-purpose Environment. However, Environment already contains default configuration options. So maybe its better to keep everything in one place?

[mnapoli]

As a user they would be much easier to find on the Environment class.

allow_filtered - Run HTML content through HTMLPurifier and render that

Could make sense but it would mean requiring that package as a dependency. Also users can run the whole generated HTML (from CommonMark) into HTMLPurifier themselves, the only advantage of integrating it directly into this library is more guidance for users. However that could also be solved with the documentation: explain that allow is unsafe and that using HTMLPurifier is recommended with untrusted HTML? (I'm just afraid this library might overstep its role of Markdown parser with the inclusion of HTMLPurifier)

How about allow_unsafe_links (boolean)?

Sounds good, I'll have a look at it tonight.

[mnapoli]

I have pushed a new commit to add a new allow_unsafe_links option.

[mnapoli]

FYI I've replaced htmlentities() with htmlspecialchars() (see https://fr.reddit.com/r/PHP/comments/4qbf4g/improving_handling_of_untrusted_html_input_in/d4te6ms?context=3 for the reason).

This PR should be good for review now.

[colinodell]

This looks perfect to me! Thank you so much for implementing this. I'll release a new version with this change momentarily.

[colinodell]

0.14.0 has been released with this change!

The new options have been added to the documentation's Configuration page. I've also created a Security page encouraging users to use this new option.

Thanks again for implementing this!

[mnapoli]

Awesome thanks for the quick merge. I had no idea where the documentation was, it's a separate repository?

[GrahamCampbell]

https://github.com/thephpleague/commonmark/tree/gh-pages

[mnapoli]

Ohh I didn't even think about checking that, thanks!