Document lack of auto-escape in theme functions and add a theme autoescape helper function

What would be the drawback of automatically admin XSS filtering unsafe output? (other than the performance hit and disallowing people from intentionally putting javascript in a theme_* function)

The way to solve this issue is to list all the remaining theme_ functions and figure out what are they doing and whether they would suffer from an xss filtering. Then possibly whitelist those and throw an exception for any other to stop worrying. Possibly.

Well of course we could deprecate theme functions now, but the question is, is the effort we need to make it secure by default not worth the effort of potential break already ported contrib modules? Maybe its the right thing to break them, as they maybe are unsafe.

From a release management perspective I'd suggest: Apply Xss::filterAdmin, open (probably reopen) the conversions as major and then reevaluate afterwards again if its okay to remove the functionality . On top of Xss::filterAdmin() we should ideally mark the functionality as deprecated already, if possible.

@dawehner sorry to continue the off-topic discussion, but shouldn't we deprecate them anyway? The question is just whether for Drupal 8.0.0 or 9.0.0.

If for 8, we could just whitelist core theme functions while we try to get rid of them post-8.0.0. Which may break some existing Drupal 8 sites and some ported modules that use theme functions, and make the effort of porting D7 to D8 potentially slightly larger (though it's large anyway compared to D4->D5->D6->D7). But if we do this for 9, I do wonder what people might end up doing in contrib theme functions, and it's yet another (insecure) feature that would have to be supported all throughout the 8.x cycle, as we can't just to deprecate them for a minor release.

Do we have an existing issue about theme function deprecation/removal where we can grep through all of contrib and see how prevalent its use is anyway?

I am working on #2573231: Add a helper method to help theme functions to esacpe if the output is not safe yet..

Let's do that! It is secure, it encourages people to do it right. Let's make the deprecation later.

Per discussion with @alexpott, changing the title. We need to add something the mimics Twig autoescape for use in theme functions (or even tpl.php files):

see: \Drupal\Core\Template\TwigExtension::escapeFilter()

Here's a start on that as a patch.

Needs tests and the actual doc changes and possibly applying this to any theme fucntion that needs it

closed #2573231: Add a helper method to help theme functions to esacpe if the output is not safe yet. as duplicate

I agree, we want to make people it somehow possible write theme functions in a similar way than twig templates.

Starting on a test, but it's failing and need to debug.

Looking at that for a while.

Just want to mention this is also relevant for PHPTemplate since it's still hanging out in a dark corner of core.

Usually things work if they are done right.

1. +++ b/core/includes/theme.inc
   @@ -358,6 +360,64 @@ function theme_get_setting($setting_name, $theme = NULL) {
   + * Helper function to automatically escape variables in theme functions.
   ...
   +function theme_autoescape($arg) {
   

   I think we could be more explicit what the theme_autoescape actually does because it autoescapes the render array and returns the rendered output.

2. +++ b/core/includes/theme.inc
   @@ -358,6 +360,64 @@ function theme_get_setting($setting_name, $theme = NULL) {
   +  // @see
   ...
   +    elseif (method_exists($arg, '__toString')) {
   

   @see Drupal\Core\Template\TwigExtension\escapeFilter . This not really DRY so it might be worth moving this into some place where it affects both of the places where this logic occurs.

3. +++ b/core/includes/theme.inc
   @@ -358,6 +360,64 @@ function theme_get_setting($setting_name, $theme = NULL) {
   +  // This is a normal render array, which is safe by definition, with
   +  // special simple cases already handled.
   

   Nit: special fits the previous line

4. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -7,7 +7,9 @@
   +use Drupal\Component\Utility\Html;
   ...
   +use Drupal\Core\Render\RenderableInterface;
   

   Unnecessary

5. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeAutoescapeTest.php
   @@ -0,0 +1,58 @@
   +  /**
   +   * Modules to enable.
   +   *
   +   * @var array
   +   */
   +  public static $modules = [];
   

   No empty modules property is needed

6. +++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeAutoescapeTest.php
   @@ -0,0 +1,58 @@
   +
   +
   

   Nit: Double line change

So we need to add more test cases and docs to theme()

What about an assert(in_array($theme_function, $core_whitelist)) won't break production but will force developers to nuke 'em.

Any reason we can't do this to address #21.2?

(Sorry, left to eat dinner and found @stefan.r had posted a patch, thus the double interdiff...)

Talked briefly with @dawehner about the solution taken on #25 and I think we both agreed that its not optimal but since we are anyway adding Twig autoescape functionality for legacy system it might make sense to have it the way it is done there.

What are we gonna do to support escaping in PHP templates?

> @chx we should open a followup issue for that, but for me core needs to be converted first before we start enforcing anything, so no whitelist.

Why. The community had almost three years to convert the massive amounts of templates and theme functions. We got this far. That's amazing. Tough luck we couldn't finish it all. We want to release ASAP not wait on these. Contrib or custom won't face anything even remotely as bad as core. Some of those theme functions have seen no change since 2004-2005. Enforcing them to convert is fine.

> What are we gonna do to support escaping in PHP templates?

May I recommend a big fat serving of nothing? Seriously: just use Twig. It's not in core, not my circus, not my monkeys.

See the top of #1825952: Turn on twig autoescape by default

> a PHPTemplate engine in contrib is going to have to add back a way to securely format variables

It was never meant to be otherwise.

@dawehner walked me through this, explained the problem, and asked for my opinion.

1. It is not recommended to use theme functions, you should use templates. But we have to be realistic and still support theme functions. But theme functions don't have auto-escaping. Which is why we need this "do the escaping manually" function that theme functions can use.
2. I think it should be called theme_escape_and_render(), not theme_autoescape(). IMO the latter is nonsensical, because the fact that you have to call that function in the first place means it's manual, not automatic.
3. Finally, I think theme_escape_and_render() should not duplicate logic, but instead call the Twig extension's escapeFilter() method. Then we have full symmetry and no duplication: variables for templates and in theme functions then get the exact same treatment. Less to understand, less to maintain.

If we choose to not call the Twig extension's escapeFilter() method from theme_escape_and_render(), then I think it's absolutely required that it very explicitly, very visibly state that the code must be kept in sync with escapeFilter().

> What are we gonna do to support escaping in PHP templates?

May I recommend a big fat serving of nothing? Seriously: just use Twig. It's not in core, not my circus, not my monkeys.

Php template engine is still in Drupal core. See: https://api.drupal.org/api/drupal/core!themes!engines!phptemplate!phptem...

Now I read the entire issue.

#4

Well of course we could deprecate theme functions now, but the question is, is the effort we need to make it secure by default not worth the effort of potential break already ported contrib modules? Maybe its the right thing to break them, as they maybe are unsafe.

+1

In #11, @pwolanin changed the course of the issue significantly after a discussion with @alexpott that isn't recorded on the issue. I think because we need to provide this in the mean time, until we deprecate theme functions altogether? But then will we be able to also deprecate the function we add here? I guess?

#17:

Just want to mention this is also relevant for PHPTemplate since it's still hanging out in a dark corner of core.

:O Where?

#28 + #30:

@chx we should open a followup issue for that, but for me core needs to be converted first before we start enforcing anything, so no whitelist.

Why. The community had almost three years to convert the massive amounts of templates and theme functions. We got this far. That's amazing. Tough luck we couldn't finish it all. We want to release ASAP not wait on these. Contrib or custom won't face anything even remotely as bad as core. Some of those theme functions have seen no change since 2004-2005. Enforcing them to convert is fine.

I like @chx's thinking here. Why can't we do that?

But we have to be realistic and still support theme functions

@Wim Leers why is this being realistic? They're insecure, and elsewhere in core we "use" plenty of things without "supporting" them by marking them @internal. I don't understand why can't do so for theme functions - just because we might use some legacy theme functions doesnt mean they need to be part of the API.

Can't we somehow move this handful of legacy functions into its own thing so we can add an assert() that doesn't involve whitelisting? Or cripple the

URGH if phptemplate is still there then nuke it. Kill it with fire. Or move it to contrib. Whatever you do, don't ship with it!!!! URGH. Modules can't ship with phptemplate / .tpl.php files anyways! Thanks god. ThemeManager::render hardwires twig_render_template and .html.twig for modules otherwise ppl would pester contrib maintainers for templates in the engine they use. Urgh nope. I didn't add that but it's really wise. We nuked config.storage.install saying everyone will ship YAML and that be the end of it. Same sane reasoning. Anyways, phptemplate is definitely out of scope and we don't have the time to write a new layer for making phptemplate secure. If that's a doable thing. Just swipe it under the rug and forget about it.

Wouldn't it be enough to write docs that phptemplates don't have autoescaping and shouldn't be used for that reason? Modules can't change templating engine because otherwise themers would be very confused when they would have to create their own templates for their module.

People: The title of this issue is

Document lack of auto-escape in theme functions

,
PHPTemplate is a different thing: Added a new issue: #2574717: Remove PHPTemplate, and add test coverage for multiple theme engine support

Ideally, we get rid of theme functions. But that's a nice-to-have at this point in the cycle. We can't have auto-escape in theme functions. We shouldn't use theme functions. But theme functions in D7 had to take care of escaping themselves, and if they choose to upgrade to D8 and keep those instead of upgrading to Twig, then they still have to do the escaping manually. The helper function we add here ensures that it is done in a way that is consistent with how Twig does it.

@dawehner and @stefan.r firmly believe it is better to have this be entirely decoupled from Twig rather than calling Twig's escaping logic.

I think the only thing missing, is the actual documentation part. Let's add some documentation to theme.api.php? Do we need a change record?

Working on the documentation bit

+++ b/core/includes/theme.inc
@@ -358,6 +360,67 @@ function theme_get_setting($setting_name, $theme = NULL) {
+ * This mimics the behavior of the auto-escape fucntionality in Twig.

Nit: spelling of functionality.

1. +++ b/core/includes/theme.inc
   @@ -358,6 +360,67 @@ function theme_get_setting($setting_name, $theme = NULL) {
   + * Helper function to automatically escape variables in theme functions.
   

   I think this can just be "Automatically escapes variables for theme functions."

2. +++ b/core/includes/theme.inc
   @@ -358,6 +360,67 @@ function theme_get_setting($setting_name, $theme = NULL) {
   + * You use this method in theme functions to ensure that the result is safe for
   + * output inside HTML fragments.
   + * This mimics the behavior of the auto-escape fucntionality in Twig.
   

   Minor: spelling, line wrapping, remove the word "You" from the beginning.

3. +++ b/core/includes/theme.inc
   @@ -358,6 +360,67 @@ function theme_get_setting($setting_name, $theme = NULL) {
   + * @see \Drupal\Core\Template\TwigExtension::escapeFilter()
   + */
   +function theme_escape_and_render($arg) {
   +  if ($arg instanceOf SafeStringInterface) {
   +    return (string) $arg;
   +  }
   +  $return = NULL;
   +
   +  if (is_scalar($arg)) {
   +    $return = (string) $arg;
   +  }
   +  elseif (is_object($arg)) {
   +    if ($arg instanceof RenderableInterface) {
   +      $arg = $arg->toRenderable();
   +    }
   +    elseif (method_exists($arg, '__toString')) {
   +      $return = (string) $arg;
   +    }
   +    // You can't throw exceptions in the magic PHP __toString methods, see
   +    // http://php.net/manual/en/language.oop5.magic.php#object.tostring so
   +    // we also support a toString method.
   +    elseif (method_exists($arg, 'toString')) {
   +      $return = $arg->toString();
   +    }
   +    else {
   +      throw new \Exception(t('Object of type "@class" cannot be printed.', array('@class' => get_class($arg))));
   +    }
   +  }
   +
   +  // We have a string or an object converted to a string: Autoescape it!
   +  if (isset($return)) {
   +    return SafeMarkup::isSafe($return, 'html') ? $return:  Html::escape($return);
   +  }
   +
   +  // This is a normal render array, which is safe by definition, with special
   +  // simple cases already handled.
   +
   +  // Early return if this element was pre-rendered (no need to re-render).
   +  if (isset($arg['#printed']) && $arg['#printed'] == TRUE && isset($arg['#markup']) && strlen($arg['#markup']) > 0) {
   +    return (string) $arg['#markup'];
   +  }
   +  $arg['#printed'] = FALSE;
   +  return (string) \Drupal::service('renderer')->render($arg);
   +}
   +
   

   So I understand the desire not to couple the theme API to Twig, but this looks an awful lot like duplicated code. How do we ensure this and the Twig escape filter don't diverge? Could they both call a method from some other independent component?

   Also, I wonder if we should deprecate this function from the outset, with the goal of removing the need for this BC layer in 9.0.x? Or maybe that's part of a separate issue to discuss if we want to deprecate theme functions generally?

4. +++ b/core/lib/Drupal/Core/Render/theme.api.php
   @@ -86,7 +86,11 @@
   - * theming). In this case, a theme can override the default implementation by
   + * theming). It is important to know that theme functions have to escape
   + * variables. In order to do that properly we provide the helper function
   + * theme_escape_and_render(), which deals with render arrays, safe strings and
   + * similar to the way how twig autoescaping works.
   + * In this case, a theme can override the default implementation by
   

   I think we can probably make this a little more concise e.g. removing the words "It is important to know that" or starting a new paragraph. I'll apply the patch and see if I can suggest an improvement in context; just making a note for now.

5. +++ b/core/themes/engines/phptemplate/phptemplate.engine
   @@ -3,6 +3,15 @@
   + * Note: The PHP template engine is highly problematic, because each individual
   + * template needs to take care of escaping for each variable manually, so using
   + * it, makes it really hard to provide safe output. Instead its highly
   + * encouraged to use the twig template engine instead.
   + *
   + * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
   + *   The phptemplate engine is deprecated and replaced with the twig template
   + *   engine.
   

   This hunk should probably go in #2574717: Remove PHPTemplate, and add test coverage for multiple theme engine support.

6. +++ b/core/themes/engines/phptemplate/phptemplate.engine
   @@ -11,6 +20,7 @@
   +  trigger_error('The php template engine is deprecated and should no longer be used. Use the twig template engine instead.', E_USER_DEPRECATED);
   

   I think this hunk needs discussion but is in scope for #2574717: Remove PHPTemplate, and add test coverage for multiple theme engine support in any case rather than this issue.

Working on it

5 and 6 was accidental.

This is an incremental interdiff due to people throwing us out here.

looks great

We are still finishing off a few things in the patch; see above reviews. :)

I created a followup that we can reference for the code duplication: #2575065: Refactor theme_escape_and_render() and TwigExtension::escapeFilter() to share reused code

I'll update the patch some more.

Also filed #2575081: [policy, no patch] Use E_USER_DEPRECATED in Drupal 8 minor releases.

+++ b/core/modules/language/language.admin.inc
@@ -201,5 +201,5 @@ function template_preprocess_language_content_settings_table(&$variables) {
-  return '<h4>' . $variables['build']['#title'] . '</h4>' . drupal_render($variables['build']);
+  return '<h4>' . theme_escape_and_render($variables['build']['#title']) . '</h4>' . drupal_render($variables['build']);

Is this hunk just one that we already discovered was unsafe? Do we need to check the rest in a subsequent issue? Ideally of course we would complete the children of #2348381: [META-0 theme functions left] Convert/refactor core theme functions instead.

I added back the accidentally removed docs in theme.api.php and tweaked them a bit. I thought it was important to mention the need for escaping both times theme functions were mentioned, but these doc updates could probably use a double-check.

+++ b/core/lib/Drupal/Core/Render/theme.api.php
@@ -70,7 +70,10 @@
+ * templates will autescape variables, theme functions must explicitly escape

Typo: "autescape"

One nit which can be fixed on commit:

+++ b/core/lib/Drupal/Core/Render/theme.api.php
@@ -70,7 +70,10 @@
+ * templates will autescape variables, theme functions must explicitly escape

s/autescape/auto-escape

+++ b/core/includes/theme.inc
@@ -358,6 +360,74 @@ function theme_get_setting($setting_name, $theme = NULL) {
+ * Automatically escapes variables for theme functions.

I believe we should avoid the use of "auto" anything for this function as it's not automatically doing it. I'll post a patch updating the docs shortly.

Can someone explain why the approach in #25 was dismissed? From #29

Talked briefly with @dawehner about the solution taken on #25 and I think we both agreed that its not optimal but since we are anyway adding Twig autoescape functionality for legacy system it might make sense to have it the way it is done there.

And #32.3

Finally, I think theme_escape_and_render() should not duplicate logic, but instead call the Twig extension's escapeFilter() method.

Then in #41

@dawehner and @stefan.r firmly believe it is better to have this be entirely decoupled from Twig rather than calling Twig's escaping logic.

I believe a lot of these conversations happened on IRC or in-person at the Barcelona sprint. Can someone give those of us following along at home the gist of it? Thanks!

+++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeAutoescapeTest.php
@@ -0,0 +1,85 @@
+ * Tests the TranslatableString class.
+ *
+ * @group StringTranslation

Copy/paste leftovers?

Addresses points raised in #56/#57 and #59 along with a few doc cleanups that jumped out at me.

Hopefully these are the final nits to be picked!

Dang it...

Fatal error: Uncaught exception 'ReflectionException' with message 'Class Drupal\KernelTests\Core\Theme\ThemeAutoescapeTest does not exist' in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/src/TestDiscovery.php:306

Aside from fixing the test, this looks good to me too.

+++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeAutoescapeTest.php
@@ -14,11 +14,11 @@
+ * Tests the theme_escape_and_render function.

Could be fixed on commit: function name needs parens.

Also, just so it's documented here, @mikeker and I chatted a bit about #58. I suggested it was primarily for code decoupling, and that we can improve it in #2575065: Refactor theme_escape_and_render() and TwigExtension::escapeFilter() to share reused code.

+++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeAutoescapeTest.php
@@ -0,0 +1,87 @@
+ * Contains \Drupal\KernelTests\Core\Theme\ThemeAutoescapeTest.

Oh, in addition to renaming the file, we should also fix this too. :)

Fixing those things.

Oh, my question in #55 is still outstanding. Do we have any potential XSS vulnerabilities in core theme functions other than that function? Doesn't need to block this issue, but it's probably important to do.

I'd even suggest documenting on every theme function in core TBH. But especially an inline comment in the ones that actually need the escaping function, since people will copy them to do stuff.

Oh, my question in #55 is still outstanding. Do we have any potential XSS vulnerabilities in core theme functions other than that function? Doesn't need to block this issue, but it's probably important to do.

I haven't checked other theme functions, but this one seemed just too obvious.

+++ b/core/tests/Drupal/KernelTests/Core/Theme/ThemeEscapeAndRenderTest.php
@@ -2,7 +2,7 @@
  * @file
- * Contains \Drupal\KernelTests\Core\Theme\ThemeAutoescapeTest.
+ * Contains \Drupal\KernelTests\Core\Theme\ThemeEscapeAndRenderTest.
  */

Good catch!

I made this change first in #2574717: Remove PHPTemplate, and add test coverage for multiple theme engine support and later found out that this is that actual patch where the function is being introduced. I am repeating the same change here.

Can we have the new function in one place not both? or mark this issue duplicate and closed?

@peter don't make it harder than it is, seriously.

@pwolanin: I guess the idea is to keep the issues of documenting the escape and adding a helper separate from removing phptemplate from core (so that there's a git history). I imagine this issue must go in first, after which the patch in the other issue would have to be rerolled.

- if (isset($arg['#printed']) && $arg['#printed'] == TRUE && isset($arg['#markup']) && strlen($arg['#markup']) > 0) {
+ if (!empty($arg['#printed']) && isset($arg['#markup']) && strlen($arg['#markup']) > 0) {

... Note: This is coming directly from twig, so we should NOT change here.

Reverted that particular thing back, given that it should be more in parallel.

Looks great

RTBC++

I think theme_escape_and_render() is a confusing name, because it doesn't always escape. Also, we know we want to recommend its use outside of theme functions too (see #2528284: Document that alternate Drupal 8 theme engines must implement auto-escape or they are not secure and #2574717: Remove PHPTemplate, and add test coverage for multiple theme engine support)... although I suppose "theme" in the function name can mean "theme system" as much as "theme functions"....

I suggest calling it either render_safe() or theme_render_safe().

Or render_safe_html()/theme_render_safe_html() (because someone will point out that it's not safe in all contexts)...

It does not always render() either. Setting to needs work to address #83/#84.

Wrt to render context i think it was worth documenting that this is for the HTML context - not sure if it worth encoding that in the name since the 99% use-case of theme functions is to generate HTML.

We discussed this for a while here. We want to ensure that this is sort of part of the theme system (so its still usable in theme engines)
but discourage people to use it anywhere else, like in random controllers.

+++ b/core/modules/language/language.admin.inc
@@ -201,5 +201,5 @@ function template_preprocess_language_content_settings_table(&$variables) {
 function theme_language_content_settings_table($variables) {
...
+  return '<h4>' . theme_escape_and_render($variables['build']['#title']) . '</h4>' . drupal_render($variables['build']);

I think this is asking for trouble - can't the theme system/engine do this for us - i.e. it already knows which theme hooks are functions and which use a template - can't it call theme_escape_and_render on non templates automatically? We don't want to leave it to chance - it will be easy to forget.

I looked into auto-escaping in the ThemeManager...

    // Generate the output using either a function or a template.
    $output = '';
    if (isset($info['function'])) {
      if (function_exists($info['function'])) {
        // Theme functions do not render via the theme engine, so the output is
        // not autoescaped. However, we can only presume that the theme function
        // has been written correctly and that the markup is safe.
        $output = SafeString::create($info['function']($variables));
      }
    }

Unfortunately this is not possible - since we often just reach into $variables and get whatever we like. In @larowlan's example above, we render $variables['build'] and then reach in and get $variables['build']['#title'].

+++ b/core/includes/theme.inc
@@ -373,7 +373,8 @@ function theme_get_setting($setting_name, $theme = NULL) {
+ *   The rendered string. This result is not safe in case you use it inside
+ *   an HTML attribute.

We can make this more explicit. The rendered string, safe for use in HTML. The string is not safe when used as an HTML attribute.

I like the new name.

ok, I'm going to update this now

doc fix plus test fixes to avoid unhelpful exception on test fail plus adding descriptive keys and more test cases in the data provider.

This is lovely!

s/The string is not safe when used as an HTML attribute./The string is not safe when used inside an HTML attribute./

Do we need a CR for this particular issue?

text fix

@dawehner - do we need a CR for api additions usually?

@dawehner - do we need a CR for api additions usually?

It's important that theme_*() functions use this. So I'd say "yes" in this case.

Improving the comment about why it is impossible to do this prior to calling the theme function.

Yep, agreed on the CR.

/me works on a CR

dawehner--

Added https://www.drupal.org/node/2575445, adapted https://www.drupal.org/node/2296163 a bit and realized that we can now leverage the new function

While talking with @xjm on IRC we realized that we can actually do better and not call drupal_render() directly.

great. CR looks good.

The patch is still referencing the old "theme_escape_and_render" function name in several places.

Wrt to render context i think it was worth documenting that this is for the HTML context - not sure if it worth encoding that in the name since the 99% use-case of theme functions is to generate HTML.

I was thinking more that it is not safe for use in HTML attributes (which is now documented in the patch). I guess it's hard to really communicate that in a function name, but that's what I was aiming for. I'm not sure "autoescape" is the right name here, but it's definitely better than "escape" and it's better documented now also.

Fixed the outdated references.

Ship it.

This is a necessary API addition to make it possible to have theme_* functions that are compatible with Twig autoescaping. Basically we have to do what Twig does manually and as show in #89 we can't do anything automatic.

Committed 03ac04f and pushed to 8.0.x. Thanks!

Oh testbot

We can address some of the related documentation work in #2575551: Document the Drupal 8 sanitization API in the API docs and on Drupal.org. I published the CR.

Did anyone file a followup for checking the other 7 core theme functions?