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 strict_properties, array_methods options #3913

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions doc/api.rst
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,23 @@ The following options are available:
replace them with a ``null`` value. When set to ``true``, Twig throws an
exception instead (default to ``false``).

* ``strict_properties`` *boolean*

If set to ``true``, treats property accesses in templates only as
property accesses, without ever trying to invoke methods
with the same name (or `get{name}` methods) if the property
does not exist (defaults to ``false``).

Note that many Symfony libraries rely on the default,
non-strict behavior, so only set this value to ``true``
if using Twig without using Symfony.


* ``array_methods`` *boolean*

If ``true``, treats method calls on callable array elements
as if they were object method calls (defaults to ``false``).

* ``autoescape`` *string*

Sets the default auto-escaping strategy (``name``, ``html``, ``js``, ``css``,
Expand Down
74 changes: 74 additions & 0 deletions src/Environment.php
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ class Environment
private $resolvedGlobals;
private $loadedTemplates;
private $strictVariables;
private $arrayMethods;
private $strictProperties;
private $templateClassPrefix = '__TwigTemplate_';
private $originalCache;
private $extensionSet;
Expand Down Expand Up @@ -88,6 +90,17 @@ class Environment
* * strict_variables: Whether to ignore invalid variables in templates
* (default to false).
*
* * strict_properties: Whether to treat property accesses in templates only as property accesses,
* without ever invoking methods with the same name if the property does not exist
* (default to false).
*
* Note that many Symfony libraries rely on the default,
* non-strict behavior, so only set this value to true
* if using Twig without using Symfony.
*
* * array_methods: Whether to treat method calls on callable array elements as if they were object method calls
* (defaults to false).
*
* * autoescape: Whether to enable auto-escaping (default to html):
* * false: disable auto-escaping
* * html, js: set the autoescaping to one of the supported strategies
Expand All @@ -106,6 +119,8 @@ public function __construct(LoaderInterface $loader, $options = [])
'debug' => false,
'charset' => 'UTF-8',
'strict_variables' => false,
'array_methods' => false,
'strict_properties' => false,
'autoescape' => 'html',
'cache' => false,
'auto_reload' => null,
Expand All @@ -116,6 +131,8 @@ public function __construct(LoaderInterface $loader, $options = [])
$this->setCharset($options['charset'] ?? 'UTF-8');
$this->autoReload = null === $options['auto_reload'] ? $this->debug : (bool) $options['auto_reload'];
$this->strictVariables = (bool) $options['strict_variables'];
$this->arrayMethods = (bool) $options['array_methods'];
$this->strictProperties = (bool) $options['strict_properties'];
$this->setCache($options['cache']);
$this->extensionSet = new ExtensionSet();

Expand Down Expand Up @@ -206,6 +223,63 @@ public function isStrictVariables()
return $this->strictVariables;
}


/**
* Enables the strict_properties option.
*/
public function enableStrictProperties()
{
$this->strictProperties = true;
$this->updateOptionsHash();
}

/**
* Disables the strict_properties option.
*/
public function disableStrictProperties()
{
$this->strictProperties = false;
$this->updateOptionsHash();
}

/**
* Checks if the strict_properties option is enabled.
*
* @return bool true if strict_properties is enabled, false otherwise
*/
public function isStrictProperties()
{
return $this->strictProperties;
}

/**
* Enables the array_methods option.
*/
public function enableArrayMethods()
{
$this->arrayMethods = true;
$this->updateOptionsHash();
}

/**
* Disables the array_methods option.
*/
public function disableArrayMethods()
{
$this->arrayMethods = false;
$this->updateOptionsHash();
}

/**
* Checks if the array_methods option is enabled.
*
* @return bool true if array_methods is enabled, false otherwise
*/
public function isArrayMethods()
{
return $this->arrayMethods;
}

/**
* Gets the current cache implementation.
*
Expand Down
47 changes: 45 additions & 2 deletions src/Extension/CoreExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -1469,7 +1469,7 @@ function twig_array_batch($items, $size, $fill = null, $preserveKeys = true)
function twig_get_attribute(Environment $env, Source $source, $object, $item, array $arguments = [], $type = /* Template::ANY_CALL */ 'any', $isDefinedTest = false, $ignoreStrictCheck = false, $sandboxed = false, int $lineno = -1)
{
// array
if (/* Template::METHOD_CALL */ 'method' !== $type) {
if (/* Template::METHOD_CALL */ 'method' !== $type || $env->isArrayMethods()) {
$arrayItem = \is_bool($item) || \is_float($item) ? (int) $item : $item;

if (((\is_array($object) || $object instanceof \ArrayObject) && (isset($object[$arrayItem]) || \array_key_exists($arrayItem, (array) $object)))
Expand All @@ -1479,7 +1479,38 @@ function twig_get_attribute(Environment $env, Source $source, $object, $item, ar
return true;
}

return $object[$arrayItem];
if ($type === 'method') {
if (is_callable($object[$arrayItem])) {
if ($sandboxed) {
if (is_array($object[$arrayItem])) {
$env->getExtension(SandboxExtension::class)->checkMethodAllowed(
$object[$arrayItem][0],
$object[$arrayItem][0],
$lineno,
$source
);
} elseif (is_string($object[$arrayItem])) {
$env->getExtension(SandboxExtension::class)->checkSecurity(
[],
[],
[$object[$arrayItem]],
);
} elseif ($object[$arrayItem] instanceof Closure) {
$env->getExtension(SandboxExtension::class)->checkMethodAllowed(
$object[$arrayItem],
'call',
$lineno,
$source
);
} else {
throw new AssertionError("Unreachable");
}
}
return $object[$arrayItem](...$arguments);
Copy link
Member

Choose a reason for hiding this comment

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

This is not properly integrated with the sandbox system.

Copy link
Author

Choose a reason for hiding this comment

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

Done!

}
} else {
return $object[$arrayItem];
}
}

if (/* Template::ARRAY_CALL */ 'array' === $type || !\is_object($object)) {
Expand Down Expand Up @@ -1554,6 +1585,18 @@ function twig_get_attribute(Environment $env, Source $source, $object, $item, ar

return $object->$item;
}

if ($env->isStrictProperties()) {
if ($isDefinedTest) {
return false;
}

if ($ignoreStrictCheck || !$env->isStrictVariables()) {
Copy link
Member

Choose a reason for hiding this comment

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

Why a check on strict variables here ?

Copy link
Author

Choose a reason for hiding this comment

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

This avoids falling through to the method call logic below, without throwing an exception (for consistency with the chosen strict_variables setting).

Copy link
Member

Choose a reason for hiding this comment

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

The strict_variables setting has a well-defined semantic: deciding whether undefined values are returned as null or throwing an exception.

Copy link
Author

Choose a reason for hiding this comment

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

This logic aims precisely at maintaining this behavior, regardless of the value of strict_properties.

return;
}

throw new RuntimeError(sprintf('The property "%1$s" does not exist in class "%2$s".', $item, get_class($object)), $lineno, $source);
}
}

static $cache = [];
Expand Down
61 changes: 61 additions & 0 deletions tests/ErrorTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
*/

use PHPUnit\Framework\TestCase;
use stdClass;
use Twig\Environment;
use Twig\Error\Error;
use Twig\Error\RuntimeError;
Expand Down Expand Up @@ -58,6 +59,66 @@ public function testTwigExceptionGuessWithMissingVarAndArrayLoader()
}
}

public function testTwigExceptionGuessWithMissingPropAndArrayLoader()
{
$loader = new ArrayLoader([
'base.html' => '{% block content %}{% endblock %}',
'index.html' => <<<EOHTML
{% extends 'base.html' %}
{% block content %}
{{ foo.bar }}
{% endblock %}
{% block foo %}
{{ foo.bar }}
{% endblock %}
EOHTML
]);

$twig = new Environment($loader, ['strict_properties' => true, 'strict_variables' => true, 'debug' => true, 'cache' => false]);

$template = $twig->load('index.html');
try {
$template->render(['foo' => new stdClass]);

$this->fail("No exception was thrown!");
} catch (RuntimeError $e) {
$this->assertEquals('The property "bar" does not exist in class "stdClass" in "index.html" at line 3.', $e->getMessage());
$this->assertEquals(3, $e->getTemplateLine());
$this->assertEquals('index.html', $e->getSourceContext()->getName());
}
}

public function testTwigExceptionArrayFetchOnCallableWithArrayMethodsEnabled()
{
$loader = new ArrayLoader([
'base.html' => '{% block content %}{% endblock %}',
'index.html' => <<<EOHTML
{% extends 'base.html' %}
{% block content %}
{{ foo.bar }}
{% endblock %}
{% block foo %}
{{ foo.bar }}
{% endblock %}
EOHTML
]);

$twig = new Environment($loader, ['strict_properties' => true, 'strict_variables' => true, 'debug' => true, 'cache' => false]);

$template = $twig->load('index.html');
try {
$template->render(['foo' => [
'bar' => function () { return 'ok'; }
]]);

$this->fail("No exception was thrown!");
} catch (RuntimeError $e) {
$this->assertEquals('An exception has been thrown during the rendering of a template ("Object of class Closure could not be converted to string") in "index.html" at line 3.', $e->getMessage());
$this->assertEquals(3, $e->getTemplateLine());
$this->assertEquals('index.html', $e->getSourceContext()->getName());
}
}

public function testTwigExceptionGuessWithExceptionAndArrayLoader()
{
$loader = new ArrayLoader([
Expand Down
Loading