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

Add markdown_escape setting #785

Merged
merged 1 commit into from Mar 4, 2017

Conversation

ArthurHoaro
Copy link
Member

This setting allows to escape HTML in markdown rendering or not.
The goal behind it is to avoid XSS issue in shared instances.

More info:

  • the setting is set to true by default
  • it is set to false for anyone who already have the plugin enabled
    (avoid breaking existing entries)
  • improve the HTML sanitization when the setting is set to false - but don't consider it XSS proof
  • mention the setting in the plugin README

This need to be backported and released in v0.7.x and v0.8.x.

This PR is high priority.

@ArthurHoaro ArthurHoaro added this to the 0.9.0 milestone Feb 27, 2017
@ArthurHoaro ArthurHoaro self-assigned this Feb 27, 2017
/**
* Test updateMethodEscapeMarkdown with nothing to do (setting already set)
*/
public function testEscapeMarkdownSettingNothingToDo()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be split in two tests:

  • setting previously enabled
  • setting previously disabled

This setting allows to escape HTML in markdown rendering or not.
The goal behind it is to avoid XSS issue in shared instances.

More info:

  * the setting is set to true by default
  * it is set to false for anyone who already have the plugin enabled
  (avoid breaking existing entries)
  * improve the HTML sanitization when the setting is set to false - but don't consider it XSS proof
  * mention the setting in the plugin README
@ArthurHoaro
Copy link
Member Author

Updated and rebased.

Copy link
Member

@virtualtam virtualtam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Travis CI is currently down due to an AWS outage, hence the failing/unavailable builds

https://www.traviscistatus.com/incidents/hx7cnxbch9xf

Copy link
Member

@virtualtam virtualtam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build passed ;-)

@virtualtam virtualtam merged commit 74198dc into shaarli:master Mar 4, 2017
@virtualtam
Copy link
Member

virtualtam commented Mar 4, 2017

v0.8.4 released, v0.7.1 requires a bit more work as some tests fail on v0.7

EDIT:

  • there is 1 test on v0.7.0 that fails with PHP 7.1 (not supported on the v0.7.x series)
  • ConfigManager was introduced in v0.8.x, so backporting this fix to v0.7 is not straightforward

EDIT 2:

virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Mar 8, 2017
Adapted from shaarli#785

Signed-off-by: VirtualTam <virtualtam@flibidi.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin bells and whistles security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants