Problem/Motivation

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

Proposed resolution

  • Remove the call by refactoring the code.
  • 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. Refactor 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.

Manual testing steps (for XSS and double escaping)

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

  1. Clean install of Drupal 8.
  2. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.
  3. If there is any user or calling code input in the string, submit
    alert('XSS');

    and ensure that it is sanitized.

User interface changes

N/A

API changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cilefen created an issue. See original summary.

cilefen’s picture

Status: Active » Needs review
FileSize
945 bytes
stefan.r’s picture

cilefen’s picture

#3 LOL, But that was my plan! Ha ha

alexpott’s picture

Well this will still work because we're not preventing an empty array. But it does mean extra costs and unnecessary function calls. This is the only call have no idea how to replace :(

dawehner’s picture

One potential feature of the RendererContext was to be able to also cary over safe strings from one level, as long you need it. Could we expand the render context to store safe strings,
until they are used in a template?

catch’s picture

That was also Fabian's idea here: #2488538-28: Add SafeMarkup::remove() to free memory from marked strings when they're printed

It would get rid of the static safe list, which would be good.

I don't think there's another way around it - possibly in the future we can try to handle translation at the Twig level and not actually do any string manipulation before then - which is where https://groups.drupal.org/node/160704 was heading, but we haven't got there.

alexpott’s picture

Well if this is the last SafeMarkup::set() call maybe we can just use SafeMarkup::setMultiple() since we don't have any plans to remove that.

stefan.r’s picture

So let's do that if it allows us to get rid of SafeMarkup::set(), at least that makes it explicit what we're doing.

alexpott’s picture

Status: Needs review » Needs work

SafeMarkup::setMultiple is way more awkward than that :)

stefan.r’s picture

Status: Needs work » Needs review
FileSize
774 bytes

Like such?

Status: Needs review » Needs work

The last submitted patch, 11: 2555825-11.patch, failed testing.

stefan.r’s picture

No..

heykarthikwithu’s picture

stefan.r’s picture

Status: Needs work » Needs review
FileSize
782 bytes

Status: Needs review » Needs work

The last submitted patch, 15: 2555825-14.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
798 bytes

#10 wasn't kidding :)

Status: Needs review » Needs work

The last submitted patch, 17: 2555825-17.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
2.5 KB

alexpott’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
    @@ -21,7 +21,7 @@
    +   * @param string|\Drupal\Component\Utility\SafeStringInterface $string
    

    This change is incorrect - it never returns an object.

  2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -155,7 +157,7 @@ public function translate($string, array $args = array(), array $options = array
    -   * @param string $string
    +   * @param string|\Drupal\Component\Utility\SafeStringInterface $string
    
    @@ -163,7 +165,7 @@ public function translate($string, array $args = array(), array $options = array
    -   * @return string
    +   * @return string||\Drupal\Component\Utility\SafeStringInterface
    

    I'm not sure about these...

stefan.r’s picture

Status: Needs work » Needs review
FileSize
3.32 KB

So the string cast solved *something*, let's see what kind of object they might have been. @alexpott mentioned maybe TranslationWrapper.

stefan.r’s picture

stefan.r’s picture

The last submitted patch, 21: 2555825-assert-FAIL.patch, failed testing.

The last submitted patch, 22: 2555825-fail.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 23: 2555825-fail2.patch, failed testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationInterface.php
    @@ -21,7 +21,7 @@
    -   * @param string $string
    +   * @param string|\Drupal\Component\Utility\SafeStringInterface $string
    

    I'm confused, don't we cast things now?

  2. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -140,12 +140,17 @@ public function getStringTranslation($langcode, $string, $context) {
    +    if (is_object($string)) {
    +      throw new \Exception('$string is an instance of ' . get_class($string));
    +    }
    

    This really sounds like an assertion for me. Its a pure developer mistake if an object is passed into it.

  3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
    @@ -140,12 +140,17 @@ public function getStringTranslation($langcode, $string, $context) {
    +      SafeMarkup::setMultiple([$string => ['html' => TRUE]]);
    

    LOLOLOL, what a workaround :)

stefan.r’s picture

Status: Needs work » Needs review
FileSize
1.33 KB
stefan.r’s picture

@dawehner these are just test patches that are supposed to fail -- #1/#2 will not be in the final patch.

So no, they won't be SafeString but they were TranslationWrappers in some cases and we're just trying to figure out where we have those being passed into t() now.

stefan.r’s picture

Issue summary: View changes
FileSize
16.26 KB

Actually this patch may be final, removing that t() call fixed the failures and everything is still translated:

Just for the record we used to be passing TranslationWrapper('Not specified') into t(), which wasn't necessary.

Wim Leers’s picture

+++ b/core/lib/Drupal/Core/StringTranslation/TranslationManager.php
@@ -145,7 +145,8 @@ public function translate($string, array $args = array(), array $options = array
-      return SafeMarkup::set($string);
+      SafeMarkup::setMultiple([$string => ['html' => TRUE]]);

Hrm, so this is just a work-around exploiting the fact that set() is removed, but setMultiple() isn't. But as #8 indicates, that's a reasonable work-around, if this really is the last/only use.

But… isn't the real solution here to add TranslatedString implements SafeStringInterface, and use that instead?

alexpott’s picture

@Wim Leers that's going to prove very tricky at this point... because of TranslationWrapper::__toString() if that is called before render it needs to convert to string and therefore loses safe-ness. I think the removal of the safe string list for SafeMarkup::format() and TranslationManager::translate() will have to be D9. Oh and we also have to deal with TranslationManager::formatPlural()...

However what we could do is document that the result of t() might be an object the implements SafeStringInterface and therefore the result of it can not be used in array keys - even though atm it can never be said object.

alexpott’s picture

Here is an absolutely crazy idea - #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list that manages to properly remove t()'d strings from SafeMarkup...

Wim Leers’s picture

Aha! I think something like #32's first paragraph should be added to the code comments, to make future readers (and truly getting rid of SafeMarkup during D9) easier, because we won't have to do a lot of archaeological digging then.
I don't find the second paragraph very convincing: if it's possible, and common sense says it's possible, it will be done, regardless of what the docs say.

Once that's documented, I think this is RTBC.

stefan.r’s picture

Wim Leers’s picture

Assigned: Unassigned » catch
Status: Needs review » Reviewed & tested by the community

The committer may choose to add @todo Improve in Drupal 9. to the comment, but besides that, this looks ready.

  • catch committed 7bf9b9a on 8.0.x
    Issue #2555825 by stefan.r, cilefen: Remove SafeMarkup::set call from...
catch’s picture

Status: Reviewed & tested by the community » Fixed

#2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list reminds me of https://groups.drupal.org/node/160704 / #1213510: A modern t().

This is an obvious hack but I think removing SafeMarkup::set() is more important than removing SafeMarkup::setMultiple() - the very difficulty getting it to work is at least some discouragement to use it - although I think we could add a follow-up to mark SafeMarkup::setMultiple() @internal - really no contrib or custom code should ever be using that.

Committed/pushed to 8.0.x, thanks!

The last submitted patch, 9: 2555825-9.patch, failed testing.

stefan.r’s picture

Status: Fixed » Closed (fixed)

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