Skip to content

Commit

Permalink
Implement support for backend.allow_unsafe_markdown and improve suppo…
Browse files Browse the repository at this point in the history
…rt for Swoole
  • Loading branch information
Luke Towers committed May 26, 2020
1 parent f85039b commit 6ae19a6
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 7 deletions.
34 changes: 27 additions & 7 deletions formwidgets/BlogMarkdown.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
*/
class BlogMarkdown extends MarkdownEditor
{
/**
* {@inheritDoc}
*/
public function init()
{
$this->viewPath = base_path().'/modules/backend/formwidgets/markdowneditor/partials';
Expand All @@ -29,12 +32,28 @@ public function init()
parent::init();
}

/**
* {@inheritDoc}
*/
protected function loadAssets()
{
$this->assetPath = '/modules/backend/formwidgets/markdowneditor/assets';
parent::loadAssets();
}

/**
* Disable HTML cleaning on the widget level since the PostModel will handle it
*
* @return boolean
*/
protected function shouldCleanHtml()
{
return false;
}

/**
* {@inheritDoc}
*/
public function onRefresh()
{
$content = post($this->formField->getName());
Expand All @@ -46,6 +65,11 @@ public function onRefresh()
];
}

/**
* Handle images being uploaded to the blog post
*
* @return void
*/
protected function checkUploadPostback()
{
if (!post('X_BLOG_IMAGE_UPLOAD')) {
Expand Down Expand Up @@ -90,11 +114,9 @@ protected function checkUploadPostback()
];

$response = Response::make()->setContent($result);
$response->send();
$this->controller->setResponse($response);

die();
}
catch (Exception $ex) {
} catch (Exception $ex) {
$message = $uploadedFileName
? Lang::get('cms::lang.asset.error_uploading_file', ['name' => $uploadedFileName, 'error' => $ex->getMessage()])
: $ex->getMessage();
Expand All @@ -105,9 +127,7 @@ protected function checkUploadPostback()
];

$response = Response::make()->setContent($result);
$response->send();

die();
$this->controller->setResponse($response);
}
}
}
6 changes: 6 additions & 0 deletions models/Post.php
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,12 @@ public static function formatHtml($input, $preview = false)
{
$result = Markdown::parse(trim($input));

// Check to see if the HTML should be cleaned from potential XSS
$user = BackendAuth::getUser();
if (!$user || !$user->hasAccess('backend.allow_unsafe_markdown')) {
$result = Html::clean($result);
}

if ($preview) {
$result = str_replace('<pre>', '<pre class="prettyprint">', $result);
}
Expand Down

11 comments on commit 6ae19a6

@aurelien-roy
Copy link

@aurelien-roy aurelien-roy commented on 6ae19a6 May 31, 2020

Choose a reason for hiding this comment

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

This commit has been pushed to rainlab/blog/master whereas the new "allow_unsafe_markdown" permission has not been released on master on OctoberCMS causing our blog writers no longer be able to use HTML in blog posts while the next October release is not out. :/ :/

@bennothommo
Copy link
Contributor

Choose a reason for hiding this comment

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

@aurelien-roy You'll need to rollback the Blog version to 1.4.0 or use the develop build of October. Although, I'm curious as to which HTML your blog writers are using, as Html::clean doesn't strip all HTML, just some potentially unsafe HTML.

@LukeTowers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aurelien-roy yes, please let us know ASAP what markup is affected, HTML::clean should only be removing potential XSS.

@aurelien-roy
Copy link

@aurelien-roy aurelien-roy commented on 6ae19a6 Jun 1, 2020

Choose a reason for hiding this comment

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

@LukeTowers @bennothommo Nothing anormal. The HTML contains iframe, which is a blocked tag (Usage: embed videos from various streaming platforms).

@bennothommo
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @aurelien-roy - given this is a security fix, you will need to use the develop build of October for now then, to be able to take advantage of the new permission.

@LukeTowers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bennothommo do you think we should modify Html::clean() to support passing tags that are allowed through so that we can allow iframes in blog content for less privileged authors? iframes are pretty commonly used for embedding content.

@msimkunas
Copy link
Contributor

Choose a reason for hiding this comment

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

@LukeTowers If I may ask, why is October rolling its own version of Html::clean instead of using a robust library like HTML Purifier that is built specifically for this task?

@LukeTowers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msimkunas because it's overkill for our needs and would require adding a pretty heavy dependency & interaction layer. We will be modifying our method to support specifying what tags are stripped out which will fix the iframe issue.

@msimkunas
Copy link
Contributor

Choose a reason for hiding this comment

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

@LukeTowers I'm afraid I don't follow. How does the use case in question not warrant the usage of an external library? I understand the reluctance to include a new dependency into the project, however, you're dealing with potentially unsafe user input here. Shouldn't it be properly sanitized?

This obviously depends on the threat model you have in mind. Rolling a homemade HTML parser seems like an acceptable solution when you trust the end user enough to not abuse rich text inputs like this. While this is somewhat okay in case of superusers, there may very well be other backend users with restricted permissions that you wouldn't want messing around with your inputs. I believe it's perfectly reasonable to not extend the same trust level to such users and the approach to validating their inputs should reflect that.

@LukeTowers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@msimkunas we've wanted to avoid adding such a heavy dependency to the project for a while, the existing solution has been developed over several years and gets updated whenever a new issue is found while remaining relatively lightweight. For the most part it works, providing 99.some arbitrary amount of 9s% security (whereas HTMLPurifier would add some amount more of arbitrary 9s), and part of why it works is because it's used for situations where the user is already partly trusted. Untrusted user input coming from a backend user with a valid account and access to that input is already more trusted than just any random request coming from the internet.

However, I do agree that October could provide a better developer experience by integrating with HTMLPurifier natively, so we'll be implementing a Html::purify() method to do so in a way that isn't as complex to use as the library happens to be natively.

@msimkunas
Copy link
Contributor

Choose a reason for hiding this comment

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

@LukeTowers Thanks for clarifying this. At the moment I'm using a plugin from the marketplace which adds the HTML Purifier as a Twig filter and it works very well. Here's hoping that this gets native support in the future!

Please sign in to comment.