Skip to content
Permalink
Browse files Browse the repository at this point in the history
Security fixes for v1.0.469
Temporarily disables SVG uploads
Improves path validation in FileDatasource & reorgs / cleans up Halcyon Builder
Adds XSS filtering to SystemException messages (related c16fefd#diff-4915b631d3234e0c5232490be34effe4)
  • Loading branch information
Luke Towers committed Sep 4, 2020
1 parent 0989697 commit 80aab47
Show file tree
Hide file tree
Showing 3 changed files with 113 additions and 98 deletions.
19 changes: 18 additions & 1 deletion src/Exception/SystemException.php
@@ -1,12 +1,29 @@
<?php namespace October\Rain\Exception;

use Exception;
use October\Rain\Html\HtmlBuilder;

/**
* This class represents a critical system exception.
* System exceptions are logged in the error log.
*
* @package october\exception
* @author Alexey Bobkov, Samuel Georges
* @author Alexey Bobkov, Samuel Georges, Luke Towers
*/
class SystemException extends ExceptionBase
{
/**
* Override the constructor to escape all messages to protect against potential XSS
* from user provided inputs being included in the exception message
*
* @param string $message Error message.
* @param int $code Error code.
* @param Exception $previous Previous exception.
*/
public function __construct($message = "", $code = 0, Exception $previous = null)
{
$message = HtmlBuilder::clean($message);

parent::__construct($message, $code, $previous);
}
}
2 changes: 0 additions & 2 deletions src/Filesystem/Definitions.php
Expand Up @@ -103,7 +103,6 @@ protected function defaultExtensions()
'png',
'webp',
'gif',
'svg',
'js',
'map',
'ico',
Expand Down Expand Up @@ -163,7 +162,6 @@ protected function assetExtensions()
'js',
'woff',
'woff2',
'svg',
'ttf',
'eot',
'json',
Expand Down
190 changes: 95 additions & 95 deletions src/Halcyon/Builder.php
Expand Up @@ -142,79 +142,99 @@ public function __construct(DatasourceInterface $datasource, Processor $processo
}

/**
* Switches mode to select a single template by its name.
* Get the compiled file content representation of the query.
*
* @param string $fileName
* @return $this
* @return string
*/
public function whereFileName($fileName)
public function toCompiled()
{
$this->selectSingle = $this->model->getFileNameParts($fileName);

return $this;
return $this->processor->processUpdate($this, []);
}

/**
* Set the directory name which the query is targeting.
* Get an array with the values of a given column.
*
* @param string $dirName
* @return $this
* @param string $column
* @param string $key
* @return array
*/
public function from($dirName)
public function lists($column, $key = null)
{
$this->from = $dirName;
$select = is_null($key) ? [$column] : [$column, $key];

return $this;
if (!is_null($this->cacheMinutes)) {
$results = $this->getCached($select);
}
else {
$results = $this->getFresh($select);
}

$collection = new Collection($results);

return $collection->lists($column, $key);
}

/**
* Set the "offset" value of the query.
* Set the "limit" value of the query.
*
* @param int $value
* @return $this
*/
public function offset($value)
public function limit($value)
{
$this->offset = max(0, $value);
if ($value >= 0) {
$this->limit = $value;
}

return $this;
}

/**
* Alias to set the "offset" value of the query.
* Alias to set the "limit" value of the query.
*
* @param int $value
* @return \October\Rain\Halcyon\Builder|static
*/
public function skip($value)
public function take($value)
{
return $this->offset($value);
return $this->limit($value);
}

/**
* Set the "limit" value of the query.
* Set the "offset" value of the query.
*
* @param int $value
* @return $this
*/
public function limit($value)
public function offset($value)
{
if ($value >= 0) {
$this->limit = $value;
}
$this->offset = max(0, $value);

return $this;
}

/**
* Alias to set the "limit" value of the query.
* Alias to set the "offset" value of the query.
*
* @param int $value
* @return \October\Rain\Halcyon\Builder|static
*/
public function take($value)
public function skip($value)
{
return $this->limit($value);
return $this->offset($value);
}

/**
* Set the directory name which the query is targeting.
*
* @param string $dirName
* @return $this
*/
public function from($dirName)
{
$this->from = $dirName;

return $this;
}

/**
Expand All @@ -239,46 +259,18 @@ public function first()
}

/**
* Execute the query as a "select" statement.
*
* @param array $columns
* @return \October\Rain\Halcyon\Collection|static[]
*/
public function get($columns = ['*'])
{
if (!is_null($this->cacheMinutes)) {
$results = $this->getCached($columns);
}
else {
$results = $this->getFresh($columns);
}

$models = $this->getModels($results ?: []);

return $this->model->newCollection($models);
}

/**
* Get an array with the values of a given column.
* Switches mode to select a single template by its name.
*
* @param string $column
* @param string $key
* @return array
* @param string $fileName
* @return $this
*/
public function lists($column, $key = null)
public function whereFileName($fileName)
{
$select = is_null($key) ? [$column] : [$column, $key];

if (!is_null($this->cacheMinutes)) {
$results = $this->getCached($select);
}
else {
$results = $this->getFresh($select);
}
$this->validateFileName($fileName);

$collection = new Collection($results);
$this->selectSingle = $this->model->getFileNameParts($fileName);

return $collection->lists($column, $key);
return $this;
}

/**
Expand Down Expand Up @@ -321,30 +313,23 @@ protected function runSelect()
}

/**
* Set a model instance for the model being queried.
* Execute the query as a "select" statement.
*
* @param \October\Rain\Halcyon\Model $model
* @return $this
* @param array $columns
* @return \October\Rain\Halcyon\Collection|static[]
*/
public function setModel(Model $model)
public function get($columns = ['*'])
{
$this->model = $model;

$this->extensions = $this->model->getAllowedExtensions();

$this->from($this->model->getObjectTypeDirName());
if (!is_null($this->cacheMinutes)) {
$results = $this->getCached($columns);
}
else {
$results = $this->getFresh($columns);
}

return $this;
}
$models = $this->getModels($results ?: []);

/**
* Get the compiled file content representation of the query.
*
* @return string
*/
public function toCompiled()
{
return $this->processor->processUpdate($this, []);
return $this->model->newCollection($models);
}

/**
Expand Down Expand Up @@ -408,10 +393,9 @@ public function update(array $values)
/**
* Delete a record from the database.
*
* @param string $fileName
* @return int
*/
public function delete($fileName = null)
public function delete()
{
$this->validateFileName();

Expand Down Expand Up @@ -442,6 +426,33 @@ public function lastModified()
);
}

/**
* Set a model instance for the model being queried.
*
* @param \October\Rain\Halcyon\Model $model
* @return $this
*/
public function setModel(Model $model)
{
$this->model = $model;

$this->extensions = $this->model->getAllowedExtensions();

$this->from($this->model->getObjectTypeDirName());

return $this;
}

/**
* Get the model instance being queried.
*
* @return \October\Rain\Halcyon\Model
*/
public function getModel()
{
return $this->model;
}

/**
* Get the hydrated models.
*
Expand All @@ -468,16 +479,6 @@ public function getModels(array $results)
return $models->all();
}

/**
* Get the model instance being queried.
*
* @return \October\Rain\Halcyon\Model
*/
public function getModel()
{
return $this->model;
}

//
// Validation (Hard)
//
Expand Down Expand Up @@ -778,9 +779,8 @@ public static function clearInternalCache()
*
* @param string $method
* @param array $parameters
* @return mixed
*
* @throws \BadMethodCallException
* @return void
*/
public function __call($method, $parameters)
{
Expand Down

13 comments on commit 80aab47

@acasar
Copy link
Contributor

@acasar acasar commented on 80aab47 Sep 11, 2020

Choose a reason for hiding this comment

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

@LukeTowers Not sure why, but this change seems to be the reason, a lot of my code broke.

Previously it was possible to load a template using base path:

$twig = Controller::getController()->getTwig();
$template = $twig->load(base_path('template/somewhere/in/the/project.htm'));
$template->render($data);

Now trying to so this, I get an error:

The specified file name [/var/www/project/template/somewhere/in/the/project.htm] is invalid.

Seems like it's connected to some maxNesting level changes. Is there any workaround?

@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.

@acasar use the System Twig environment instead of the CMS one. CMS partials should not be loading from anywhere but the theme itself.

@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.

And possible !== actually supported.

@acasar
Copy link
Contributor

@acasar acasar commented on 80aab47 Sep 11, 2020

Choose a reason for hiding this comment

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

use the System Twig environment instead of the CMS one

Is there any documentation on how to do that? Do I need to write the entire Twig initialization logic myself?

@LukeTowers
Copy link
Contributor Author

@LukeTowers LukeTowers commented on 80aab47 Sep 11, 2020

Choose a reason for hiding this comment

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

@acasar nope, just do the following:

use Twig;

Twig::parse($templateContents, $templateVars);

@acasar
Copy link
Contributor

@acasar acasar commented on 80aab47 Sep 11, 2020

Choose a reason for hiding this comment

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

@LukeTowers Sadly that's not suitable for me, since I still need support for tags like {% patrial '...' %} etc.

I know the "correct" solution would be to structure the project a bit differently, but I'd have to do that on 170+ websites, which is not really an option. I guess I'll need to find some other workaround....

@acasar
Copy link
Contributor

@acasar acasar commented on 80aab47 Sep 11, 2020

Choose a reason for hiding this comment

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

I ended up extending Halcyon Builder to re-add support for absolute paths:

protected function validateFileNamePath($filePath, $maxNesting = 2)
{
     if(file_exists($filePath)) {
        return true;
    }

    return parent::validateFileNamePath($filePath, $maxNesting);
}

@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.

@acasar message me on Slack or Discord and I'll help you come up with a better solution, we can have a video call too if you'd like.

@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.

@acasar I've come up with a possible fix for your use case so you don't have to change any code at all, as long as you aren't expecting base_path('template/somewhere/in/the/project.htm') to be treated as a Partial object (i.e. with support for a PHP code section and an INI configuration section)

@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.

@acasar try applying octobercms/october@dca6128 locally. Please let me know if that resolves your issue.

@acasar
Copy link
Contributor

@acasar acasar commented on 80aab47 Sep 14, 2020

Choose a reason for hiding this comment

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

@LukeTowers Yes, the octobercms/october@dca6128 works great and solves my issue. Thanks a lot! 👍

And sorry for late reply, was unavailable during the weekend.

@daftspunk
Copy link
Member

@daftspunk daftspunk commented on 80aab47 Sep 30, 2020

Choose a reason for hiding this comment

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

@acasar Upon further review from myself, this change will likely continue to break your implementation (octobercms/october@f9e14b0)

The approach you've taken is not (and will not ever be) supported natively by the system, that is the direct inclusion of external files in the theme engine outside the theme path, or in the Twig engine.

Try placing your reusable files in a views directory, using Laravel's engine. View::make('vendor.plugin::someview.htm') => {% include 'vendor.plugin::someview.htm' %}

However, Luke may have further input on this, so watch this space, I suppose

@acasar
Copy link
Contributor

@acasar acasar commented on 80aab47 Sep 30, 2020

Choose a reason for hiding this comment

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

@daftspunk ok, thanks for notifying me. Will keep my workaround in place for all existing projects all I'll structure new projects differently.

Please sign in to comment.