Problem/Motivation

theme() calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

  • If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required.

Remaining tasks

  1. Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
  2. Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.
  3. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.

Manual testing steps (for XSS and double escaping)

Do these steps both with HEAD and with the patch applied:

  1. Not necessary, we are only adding documentation..

User interface changes

N/A

API changes

N/A

Files: 
CommentFileSizeAuthor
#26 interdiff-2501745-17-25.txt845 bytescmanalansan
#25 remove_or_document-2501745-25.patch950 bytescmanalansan
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#17 remove_or_document-2501745-17.patch728 bytesakalata
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,944 pass(es). View
#15 remove_or_document-2501745-15.patch729 bytesakalata
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,877 pass(es). View
#10 remove_or_document-2501745-10.patch652 bytesDaniel_Rose
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,114 pass(es). View
#7 remove_or_document-2501745-7.patch637 bytesDaniel_Rose
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,723 pass(es). View

Comments

Daniel_Rose’s picture

FileSize
870 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,713 pass(es). View

My very first patch! Hopefully this helps.

Daniel_Rose’s picture

Issue summary: View changes
Daniel_Rose’s picture

Status: Active » Needs review
Cottser’s picture

Status: Needs review » Needs work

Thanks @Daniel_Rose!

  1. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
    @@ -312,6 +312,8 @@ protected function theme($hook, $variables = array()) {
    +    // It's ok to use SafeMarkup because it's being used for internal use
    +    // and content is already presumed to be sanitized.
    

    Maybe this could be something along the lines of:

    "Theme functions do not go through Twig so the output is not autoescaped. Theme function output is always expected to be 100% safe."

  2. +++ b/example.gitignore
    @@ -34,3 +34,6 @@ sites/simpletest
    +
    +# PHPStorm
    +# .idea
    

    Undo these changes :)

Daniel_Rose’s picture

FileSize
866 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,753 pass(es). View

Thanks for the feedback @Cottser! How does this look?

Daniel_Rose’s picture

Status: Needs work » Needs review
Daniel_Rose’s picture

FileSize
637 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,723 pass(es). View

Oops. My apologies. I realized that I didn't actually undo all of those changes in #2 in your comment @Cottser. Hopefully this looks a little better.

ultimike’s picture

Status: Needs review » Reviewed & tested by the community

Looks ready to go.

-mike

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

I think the comment should live right above the SafeMarkup::set(), like the other issues have done.

Daniel_Rose’s picture

FileSize
652 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,114 pass(es). View

How's this look?

Daniel_Rose’s picture

Status: Needs work » Needs review
akalata’s picture

Assigned: Daniel_Rose » Unassigned

The patch itself looks fine, though I have a slight issue with the wording of the comment -- I'd prefer the "is presumed to be safe" over the "is always expected to be safe", unless there is a test somewhere that confirms the expectation? Are there internal requirements that any theme function is guaranteed to be safe, or are we only presuming that this will be the case?

Just thinking in terms of the precision of uncertainty -- will keep as Needs Review and ping a theme system expert to review.

Cottser’s picture

@akalata very good point I would tend to agree the wording could be more precise.

akalata’s picture

Status: Needs review » Needs work
akalata’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
729 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,877 pass(es). View

I've updated our language a bit, which hopefully more clearly indicates the responsibility of the writer of the theme function (since the ones left in core are few and far between). I'm not sure if this is a documented standard or best practice, but it probably should be (even if my proposed standard is completely wrong). Quoting dawehner in IRC: "we output the string here, so we should sanitize it here".

davidhernandez’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
@@ -315,6 +315,9 @@ protected function theme($hook, $variables = array()) {
+        // is not auto-escaped. However, we can only presume that the theme

I believe we use autoescape as one work, not with a hyphen. Otherwise, looks fine.

akalata’s picture

Status: Needs work » Needs review
FileSize
728 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,944 pass(es). View
davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community
Daniel_Rose’s picture

I like it, but would it make more sense to say:

//Theme functions do not render via the "Twig template engine"....Instead of just saying "the theme engine.."

Especially since the "PHPTemplate" engine is still in the engines folder?

davidhernandez’s picture

"theme engine" is more generic. Theme functions would not render using phptemplate either.

Daniel_Rose’s picture

That makes sense. I was thinking that might also be the case, but wasn't 100% sure. I've been reading through every document and resource I could find about theme functions via Twig, etc., and a lot of it made sense, but some seemed contradictory. This stuff is awesome. Much to learn though.

Patch looks good to go then. Thanks all for the feedback and help! What would be the next step in this process? Do we need more reviewing and testing?

xjm’s picture

Status: Reviewed & tested by the community » Postponed
Related issues: +#2506581: Remove SafeMarkup::set() from Renderer::doRender

Thanks folks for the patch! I think the current wording is good. However, I actually feel like we should be trying to remove this call.

@alexpott suggested postponing this on #2506581: Remove SafeMarkup::set() from Renderer::doRender, which is the only place that uses theme() directly. So doing that now.

lauriii’s picture

Status: Postponed » Needs work

Unpostoponing

dawehner’s picture

So yeah I think the proper way is to return a safe string. This also removes a bit of the memory overhead we do have.

cmanalansan’s picture

Status: Needs work » Needs review
FileSize
950 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Removed SafeMarkup::set(), and added additional explanation:

Therefore, the theme function itself is responsible for using
SafeMarkup::set() as necessary, not this function, i.e.: render().

cmanalansan’s picture

FileSize
845 bytes

@davidhernandez (yelling at someone else):

You better have an interdiff!

Status: Needs review » Needs work

The last submitted patch, 25: remove_or_document-2501745-25.patch, failed testing.

cmanalansan’s picture

Status: Needs work » Closed (duplicate)