Whilst discussing #2506195: Remove SafeMarkup::set() from Xss::filter() it became obvious that if we are relying on Twig's auto-escaping for security then any other theme engine will need to implement logic to escape unsafe html before printing.

Comments

alexpott’s picture

cilefen’s picture

Issue summary: View changes
star-szr’s picture

Definitely, thanks for this.

xjm’s picture

Issue tags: +SafeMarkup
David_Rothstein’s picture

@corbacho in #1537050-53: [meta] Should we keep / improve multiple theme engine functionality? points out that:

  1. https://www.drupal.org/node/1918824 is one place this should be documented
  2. Drupal 8 is in fact still shipping with PHPTemplate at the moment (!). Surprised me, but it's true.

I think it would make sense for core to add a simple handy procedural wrapper, something like:

function safe($text) {
  return SafeMarkup::escape($text);
}

That way all it takes for an alternative theme engine such as PHPTemplate to be safe is to do this everywhere in the template:

print safe($variable);

rather than:

print $variable;

Of course the wrapper isn't strictly necessary, but it's a good place to document things, and asking people to deal with namespaces in a .tpl.php file would be painful. It's similar to how Drupal 7 introduced functions like hide(), show(), and render() to make life a little easier on themers.

dawehner’s picture

Status: Active » Needs review
StatusFileSize
new837 bytes

Just a bit of a start

joelpittet’s picture

Issue tags: +rc eligible
cilefen’s picture

StatusFileSize
new833 bytes
star-szr’s picture

Status: Needs review » Needs work

Great start thanks @cilefen!

+++ b/core/lib/Drupal/Core/Render/theme.api.php
@@ -704,6 +704,12 @@ function hook_extension() {
+ * \Drupal\Component\Utility\SafeStringInterface. Drupal is inherently unsafe if

This is \Drupal\Component\Render\MarkupInterface now as of #2576533: Rename SafeStringInterface to MarkupInterface and move related classes.

cilefen’s picture

Status: Needs work » Needs review
StatusFileSize
new695 bytes
new831 bytes
dawehner’s picture

I'm curious whether we could point to the nyan cat test one as an example?

lauriii’s picture

Nyan cat engine is simple example how to implement auto escape. I was wondering, maybe we could add some extra docs there and then use it as reference.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

fabianx’s picture

Bump

nikhilesh gupta’s picture

Version: 8.1.x-dev » 8.2.x-dev

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

markhalliwell’s picture

Status: Needs review » Reviewed & tested by the community

LGTM

alexpott’s picture

#12 and #13 haven't been answered? I looked at nyan_cat_render_template() - I'm not sure though there is that much to be gained though since the docs added point to theme_render_and_autoescape() and that's really all that nyan_cat_render_template() calls. I'll ping @dawehner and @lauriii.

dawehner’s picture

#12 and #13 haven't been answered? I looked at nyan_cat_render_template() - I'm not sure though there is that much to be gained though since the docs added point to theme_render_and_autoescape() and that's really all that nyan_cat_render_template() calls. I'll ping @dawehner and @lauriii.

While I get your point I think seeing that implementing auto-escaping would actually not be hard on a real instance might be motivating for people to do so. On the other hand, yeah, I'm totally fine with not adding the pointer.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

@lauriii pinged me in slack and said +1 to commit... so...

Committed and pushed 4105b5560b to 8.6.x and 5b8696b263 to 8.5.x. Thanks!

  • alexpott committed 4105b55 on 8.6.x
    Issue #2528284 by cilefen, dawehner, Cottser, David_Rothstein: Document...

  • alexpott committed 5b8696b on 8.5.x
    Issue #2528284 by cilefen, dawehner, Cottser, David_Rothstein: Document...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.