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

Inconsistency with variable evaluation when autoescaping is enabled #3917

Open
ericmorand opened this issue Nov 26, 2023 · 5 comments
Open

Comments

@ericmorand
Copy link
Contributor

ericmorand commented Nov 26, 2023

Consider the following template:

{{ (br == "<br/>") ? "true" : "false" }}
{{ (include("partial.twig") == "<br/>") ? "true" : "false" }}

With partial.twig being:

{{ br }}

Executing this template with "<br/>" as value for the br variable outputs:

true
false

In both conditions, we test that the value of the variable br is equal to "<br/>". But in the second case only, it is considered as false.

https://twigfiddle.com/zdltz2

Why is a variable considered as safe for evaluation in one case and not the other? I understand the technical reason - "in the second case what is evaluated is the result of the inclusion", I'm more interested in the philosophical reasons: why is a template inclusion considered as less safe than a variable inclusion even though a variable value is more likely to be provided at runtime (and thus to be unsafe) than a template inclusion?

@GromNaN
Copy link
Contributor

GromNaN commented Nov 26, 2023

The result of the include function is considered "safe" because everything inside the included template is escaped as needed (automatically or explicitly). This is necessary because we don't want to re-escape the result of a rendered template.

On the other hand, the variable will only be escaped at display time if the last function or filter that processed the value is not safe.

@ericmorand
Copy link
Contributor Author

ericmorand commented Nov 26, 2023

I understand the technical reason. My question is about the philosophical reason behind that technical decision.

Namely, quoting yourself:

The result of the include function is considered "safe" because everything inside the included template is escaped as needed (automatically or explicitly).

"because everything inside the included template is escaped as needed (automatically or explicitly)" < Why are you assuming this? Nothing guarantees that the included template is escaped, by any mean: when executing a template, the engine doesn't know where it is coming from so there is no reason to assume that it is escaped - it could totally not be, or it could be named with a way that doesn't trigger the autoescaping rule, and the consumer of this template has no way to know.

Look here: https://twigfiddle.com/j0by2n

The partial.twig template is not escaped, either by the main template or by itself. So, why do you assume that it is?

And since it is clear that included templates may totally not be safe, where is the following stance of yours coming from?

The result of the include function is considered "safe" because everything inside the included template is escaped as needed (automatically or explicitly).

On the other hand, the variable will only be escaped at display time if the last function or filter that processed the value is not safe

That's not true and it's a bold asumption.

Look here:

https://twigfiddle.com/j0by2n/2

image

The variable is escaped at compile time - by that I mean that the compiler knows that it will be safe once displayed. So, why are you assuming that it is unsafe?

More generally, let me ask again why you do assume that...

  • included templates are safe
  • variables are unsafe

...even though both of these two things are provided at runtime. And of course, since included templates are potentially coming from variables, why are they treated differently?

Why are they not both considered as unsafe?

@GromNaN
Copy link
Contributor

GromNaN commented Nov 27, 2023

You should not disable escaping if your are not sure the contents is safe.

Twig is a template engine to render HTML pages. The include function renders a template so it can be used inside an other template. The main use-case is to render fragments of the HTML document. If the result of the include function was considered unsafe, developers would have to systematically add the |raw filter to ensure their HTML is not escaped. This is not practical. We prefer an approach where everything is escaped by default in a template, so the template output is considered as safe by default. If you know the template output contains unsafe data because you disabled escaping, you can re-escape the output of the include function.

When I say "display time", I means the last operation before echo. Whether a function is safe or not is determined statically at compile time. Each filter and function has an is_safe or is_safe_callback option that indicates whether their output needs to be escaped or not, before being passed to echo.

@ericmorand
Copy link
Contributor Author

Thanks for the explanation but it is not totally clear to me.

At no point is there a specification that says that the include tag outputs a string that is escaped - or not escaped. And this is a good thing because anything can be included - HTML but also CSS or SVG or XML or JPEG or OpenGL shaders or video stream fragments or...well you get the point. Would the include tag enforce any kind of escaping it would become useless.

So, I'm not sure why you say that developers would have to use the |raw filter every time they include a fragment if the fragment were considered as unsafe: included fragments are not escaped and it is written nowhere that the include tag returns an escaped content - and again it is a relief because it would make Twig unusable in a lot of situations.

But anyway my question was more about the difference of behaviour of the PHP compiler between variables and included templates: why are the formers escaped by the auto escaping mechanism and not the latters?

I totally understand your point about convenience, but it is actually more convenient to disable autoescaping entirely. Plus it restores consistency: both variables and included templates are left untouched which solve the situation that you exposed:

If the result of the include function was considered unsafe, developers would have to systematically add the |raw filter to ensure their HTML is not escaped. This is not practical..

Disabling autoescaping is practical and prevents the developers to systematically add the raw filter.

Hence: why is this mechanism even proposed by the PHP compiler if what it brings is less practical and if it involves adding some specific behaviour to the include tag to restore the practicability that was lost?

@ericmorand
Copy link
Contributor Author

ericmorand commented Nov 27, 2023

Typically, consider these templates;

index.twig

{% set separator = "<br/>" %}
<html><head>{{ include("headContent.twig") }}</head><body>Hello{{separator }}world!</body></html>

headContent.twig

<title>{{ title }}</title><style>{{ include("style.twig") }}</style>

style.twig

body {
    background-color: orangered;
}

Rendering index.twig with a compiler that does not come with implicit auto-escaping, or with the PHP compiler with implicit auto-escaping disabled, outputs a perfectlty valid HTML document:

<html><head><title>My Page</title><style>body {
    background-color: orangered;
}</style></head><body>Hello<br />world!</body></html>

So, if practicability was the reason why the include tag doesn't auto-escape when the implicit auto-escaping mechanism is enabled, wouldn't it prove that the auto-escaping mechanism is not only useless but also penalizing? I mean, why enable a mechanism that doesn't change anything for template inclusion but brings escaping to variables, escaping that needs to be cancelled with an explicit use of {% autoescape false %}?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants