The autoescape tag is inconsistent when applied to nested conditionals

[ericmorand]

Consider the following template:

{% set br = "<br />" %}
{% autoescape false %}
    {% autoescape 'html' %}
        {{ true ? "<br />" : br }}
        {{ true ? (true ? "<br />" : br) : br }}
    {% endautoescape %}
{% endautoescape %}

It ouputs:

    <br />
&lt;br /&gt;

The result is inconsistent here.

What is the escaping rule for conditional nodes? And more specifically, why is the first conditional not escaped?

Note that I totally understand the technical reasons behind this behaviour. My question is about the logical reason behind this behaviour - what is the rational that led to the current technical implementation.

[stof]

String literals are considered safe regarding auto-escaping. And for a ternary, the 2 branches are escaped separately (but it looks like this is not done recursively, probably because this case never came up until now).

[ericmorand]

Do you think that the second conditional should also not be escaped?

It makes a certain sense to me that it is not but I'm not sure my reasoning matches the philosophy of the language. Let me explain.

In Twig, type of data seem to change as soon as they are assigned to a variable.

In my example, "<br/>" in the first line is a string literal until it is assigned to the br name. Then it becomes... something else.

Hence, escaping "<br>" or br does not give the same result. As counter intuitive as it is, i think that it may explain why the second conditional is escaped: just like variable assignment change the type of the data, conditional change the type of the data.

So the expr2 value of the second conditional is not a literal anymore: it is something else that is considered as unsafe.

We find instances of this behaviour in other constructs like the concatenation:

{% autoescape %}
{{"<" ~ "br/>"}}
{{"<br/>"}}
{% endautoescape %}

Here the first variable is considered as unsafe while the second one is safe even though they seem to both be literals holding the exact same value. Does it also mean that types are lost when concatenating? Actually this example is even more disturbing because it demonstrates that type is not only lost on assignment, it can also be lost when applying a binary operation on two operands of the exact same type.

And it is not clear what binary operation are type safe and what are not.

What do you think?

[stof]

Auto-escaping is based on the AST of the template as it is a feature happening at compile time of the template.

What is relevant here is having an AST being PrintExpression(StringLiteralExpression()) vs PrintExpression(VariableExpression()) or PrintExpression(BinaryOperatorExpression(operator: 'concat'))

[ericmorand]

Make sense. Still counter intuitive from a template developer point of view, in my opinion, but the specification that you exposed makes sense: the safety comes from the node type, not the result of its execution.

Thanks a lot for your time. Much appreciated. 🙂

[stof]

We cannot mark values as safe at runtime as this would require a way to attach metadata to values when passing them around. Boxing all values into a TwigValue object would be a pain for the implementation of all functions and filters that would have to be aware of it and would have a performance impact (while the current auto-escaping has no impact on performance at all).

[ericmorand]

Yes, it makes sense.

Maybe I'm wrong, but doesn't it create an issue when rendering the same template with different environment autoescaping strategy? Since the template is compiled with the environment autoescaping strategy included (i.e. the AST contains all the legit escape nodes, with their strategy attribute set), rendering the same template with a different environment's autoescaping strategy would lead to an unexpected result: the execution of a template with a different auto-escaping strategy than the one passed to the environment.

$env = new \Twig\Environment($loader, [
    "autoescape": "html",
    "cache": "tmp"
]);

$env->render('index.twig', []); // would compile a template with escape nodes set to "html" strategy and execute it

$env = new  \Twig\Environment($loader, [
    "autoescape": "js",
    "cache": "tmp"
]);

$env->render('index.twig', []); // would execute the previously compiled template, hence would autoescape using "html" instead of "js"

I don't have a PHP setup to test it right now but I suspect that, since the options hash of the environment doesn't include the autoescaping strategy, the environment would reuse the template from the cache instead of compiling a new one with the properly configured escape nodes.

private function updateOptionsHash(): void
    {
        $this->optionsHash = implode(':', [
            $this->extensionSet->getSignature(),
            \PHP_MAJOR_VERSION,
            \PHP_MINOR_VERSION,
            self::VERSION,
            (int) $this->debug,
            (int) $this->strictVariables,
        ]);
    }

https://github.com/twigphp/Twig/blob/aeeec9a5e907a79e50a6bb78979154599401726e/src/Environment.php#L829C5-L839C6

Maybe I'm missing something, and even if I'm right this is a trivial problem that probably nobody ever encountered.

EDIT: Also, now that I think about it, it would create issues when you render a template from an environment where the filters or function don't have the same safety rules than the environment the template was first compiled with. Since the escaping rules are set in stone at compile time, changing the safety of a filter and re-render would keep using the previous filter safey rules. Again, trivial and probably not encountered by anyone ever, but interesting nonetheless.

[stof]

Sharing the same cache folder between several environments that don't have the same configuration is not guaranteed to work indeed. But supporting this (weird) use case would require a lot of complexity (and make things harder for authors of Twig extensions as Twig would have to make them do part of the work).