original title: "template_preprocess_menu_local_task() requires explicit plaining of link titles"

Problem/Motivation

\Drupal\Core\Menu\LocalTaskDefault::getTitle has

return $this->t($this->pluginDefinition['title'], $args, $options);

and t() calls SafeMarkup::set so at this point the title is marked safe despite the fact it might not be. In addition, it might be user input and thus polluting the translation database

If we resolve this and #2273923: Remove html => TRUE option from l() and link generator. gets in then template_preprocess_menu_local_task() can be simplified significantly to
#title => $link_text.

The relevant test is Drupal\block\Tests\BlockTest::testThemeName().

Proposed resolution

Change the YAML discovery of local tasks, actions, and contextual links so that we wrap text that's safe and translatable in a TranslationWrapper object. This way we can call t() on strings from YAML but not on strings in derivatives that might have definitions that come from user input

Follow-up at #2488304: Add more docs that translate() also marks strings as safe to improve the t() documentation to make it crystal clear that what gets passed into it must be a safe string that is not user input.

This solves all problems:

  1. This makes derivatives behave the same way across plugin types
  2. This ensures that just static titles are marked as safe
  3. This ensures that just static titles are translated. Reminder: There are thousands of Drupal sites out there with gigantic {locale} tables.

#67-#79 tried to escape but then call out to t(). This solves the 2) point, but not 1) and 3)

Remaining tasks

review

User interface changes

none

API changes

API addition / enhancement in YamlDiscovery

CommentFileSizeAuthor
#148 2338081_148.patch50.01 KBchx
#148 interdiff.txt1.19 KBchx
#147 interdiff.txt1.37 KBdawehner
#147 2338081-147.patch50.42 KBdawehner
#143 interdiff.txt1.02 KBdawehner
#143 2338081-143.patch50.45 KBdawehner
#140 interdiff.txt1.37 KBdawehner
#140 2338081-140.patch49.43 KBdawehner
#138 interdiff.txt1.52 KBdawehner
#138 2338081-138.patch49.39 KBdawehner
#137 interdiff.txt1.35 KBdawehner
#137 2338081-137.patch49.2 KBdawehner
#133 interdiff.txt1.11 KBdawehner
#133 2338081-133.patch49.22 KBdawehner
#130 increment.txt837 bytespwolanin
#130 2338081-130.patch49.22 KBpwolanin
#125 increment.txt16.83 KBpwolanin
#125 2338081-125.patch49.7 KBpwolanin
#123 increment.txt4.82 KBpwolanin
#123 2338081-123.patch55.35 KBpwolanin
#114 interdiff-113-114.txt1.74 KBmpdonadio
#114 local_tasks_actions-2338081-114.patch54.63 KBmpdonadio
#113 interdiff.txt1.2 KBdawehner
#113 2338081-113.patch54.76 KBdawehner
#111 interdiff.txt8.57 KBdawehner
#111 2338081-111.patch55.2 KBdawehner
#109 interdiff-108.txt3.04 KBbrandon.holtsclaw
#109 2338081-109.patch49.89 KBbrandon.holtsclaw
#108 increment.txt511 bytespwolanin
#108 2338081-108.patch48.98 KBpwolanin
#106 increment.txt3.38 KBpwolanin
#106 2338081-106.patch48.97 KBpwolanin
#103 interdiff.txt3.75 KBdawehner
#99 increment.txt809 bytespwolanin
#99 2338081-99.patch48.7 KBpwolanin
#97 increment.txt7.93 KBpwolanin
#97 2338081-97.patch48.67 KBpwolanin
#96 increment.txt4.88 KBpwolanin
#96 2338081-96.patch41.05 KBpwolanin
#94 increment.txt5 KBpwolanin
#94 2338081-94.patch41.49 KBpwolanin
#93 increment.txt878 bytespwolanin
#93 2338081-93.patch37.72 KBpwolanin
#88 interdiff.txt2.55 KBdawehner
#88 2338081-87.patch37.13 KBdawehner
#79 2338081-diff-69-77.txt4.18 KBvijaycs85
#79 2338081-77.patch10.08 KBvijaycs85
#76 menu.inc_.txt674 bytesmpdonadio
#75 interdiff.txt2.55 KBdawehner
#69 2338081-68.patch6.98 KBvijaycs85
#67 2338081-57.patch7.76 KBvijaycs85
#57 interdiff.txt3.03 KBdawehner
#57 2338081-57.patch34.58 KBdawehner
#56 interdiff.txt11.8 KBdawehner
#56 2338081-56.patch31.55 KBdawehner
#50 increment.txt9.41 KBpwolanin
#50 2338081-50.patch29.43 KBpwolanin
#46 increment.txt3.73 KBpwolanin
#46 2338081-yaml-translate-46.patch28.88 KBpwolanin
#44 increment.txt695 bytespwolanin
#44 2338081-yaml-translate-44.patch25.15 KBpwolanin
#42 increment.txt2.62 KBpwolanin
#42 2338081-yaml-translate-42.patch24.76 KBpwolanin
#41 increment.txt6.35 KBpwolanin
#41 2338081-yaml-translate-41.patch23.42 KBpwolanin
#39 increment.txt3.76 KBpwolanin
#39 2338081-yaml-translate-39.patch17.07 KBpwolanin
#38 2338081-yaml-translate-38.patch13.31 KBpwolanin
#33 increment.txt6.14 KBpwolanin
#33 2338081-33.patch13.71 KBpwolanin
#29 2338081-alternate-29.patch8.41 KBeffulgentsia
#24 interdiff.txt1.08 KBkgoel
#24 2338081-24.patch17.25 KBkgoel
#23 interdiff.txt6.15 KBkgoel
#23 2338081-23.patch17.3 KBkgoel
#20 interdiff.txt610 byteseffulgentsia
#20 2338081-20.patch17.26 KBeffulgentsia
#19 increment.txt7.12 KBpwolanin
#19 2338081-19.patch16.6 KBpwolanin
#16 2338081-16.patch10.11 KBkgoel
#10 2338081.9-FAIL.patch674 byteseffulgentsia
#9 interdiff.txt610 byteseffulgentsia
#9 2338081.9.patch1.56 KBeffulgentsia
#4 2338081.4.patch920 byteseffulgentsia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Assigned: Unassigned » pwolanin
Issue summary: View changes
chx’s picture

Issue summary: View changes
xjm’s picture

Issue tags: +SafeMarkup
effulgentsia’s picture

Status: Active » Needs review
Issue tags: +D8MI
FileSize
920 bytes

Amend t() documentation because it's not crystal clear that what gets passed into it must be a safe string.

Doesn't the documentation of bootstrap.inc's t() cover this pretty clearly:

 * You should never use t() to translate variables, such as calling
 * @code t($text); @endcode, unless the text that the variable holds has been
 * passed through t() elsewhere (e.g., $text is one of several translated
 * literal strings in an array). It is especially important never to call
 * @code t($user_text); @endcode, where $user_text is some text that a user
 * entered

Is the suggestion in this issue to copy/move that to the other t() wrappers that we have or to change that text?

As for fixing getTitle, I am assigning this to Peter for ideas.

Does LocalTaskDefault ::getTitle() need fixing? That implementation is intended to be for when title is coming from a YAML file, which I think is compatible with our D8MI rules, since it's both non-user-entered and can be statically parsed by localize.drupal.org. Maybe this needs to be documented more clearly?

The relevant test is Drupal\block\Tests\BlockTest::testThemeName().

Ah, so the problem here is that we have a deriver class that sets 'title' to a YAML-defined value (the human-readable theme name) that we want checkPlain'd? How about this patch as a way to do that? It will fail due to the additional String::checkPlain in template_preprocess_menu_local_task(), so if this is the correct fix, we may need to postpone it on #2273923: Remove html => TRUE option from l() and link generator., but leaving this issue unpostponed for discussion on whether this is the right approach.

effulgentsia’s picture

Component: menu.module » menu system

Status: Needs review » Needs work

The last submitted patch, 4: 2338081.4.patch, failed testing.

effulgentsia’s picture

+++ b/core/modules/block/src/Plugin/Derivative/ThemeLocalTask.php
@@ -64,7 +64,8 @@ public function getDerivativeDefinitions($base_plugin_definition) {
+        $this->derivatives[$theme_name]['title_arguments'] = array('@theme' => $theme->info['name']);

Also, do we still want to translate $theme->info['name']? i.e., should that be array('@theme' => t($theme->info['name'])) (or a dependency injected version of that)? Or, should that already be translated by the time it's in $theme->info? We have intelligent translation built into plugin annotations and config, but not sure what our current pattern/plan is for .info.yml values.

mgifford’s picture

Assigned: pwolanin » Unassigned

Unassigning so someone else can take it on.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
1.56 KB
610 bytes

Let's start with an updated bot check. This also includes the fix to template_preprocess_menu_local_task(), per the issue title.

effulgentsia’s picture

FileSize
674 bytes

Oops. Should have uploaded this one first, to see if only changing template_preprocess_menu_local_task() results in any failures.

Status: Needs review » Needs work

The last submitted patch, 10: 2338081.9-FAIL.patch, failed testing.

mpdonadio’s picture

Status: Needs work » Needs review

Setting back to Needs Review per the out of order patch upload; #9 came back green.

joelpittet’s picture

Needs the translation question answered still in #7.

Adding parent meta for safemakrup::set removal.

pwolanin’s picture

So - the intent was certainly as described in #4 - to localize the string coming from a static YAML file which should be safe.

So I guess part of the problem here is that subclass could run this could because the dev is not forced to think about where that string comes from?

Maybe we should have a private helper method or something so subclass are forced to implement something?

kgoel’s picture

working on this

kgoel’s picture

FileSize
10.11 KB

pwolanin and I worked on this together. While we were weighing different approaches for this issue, we looked into how views and search are handling local task. We were trying to add private method defaultGetTitle into LocalTaskDefault.p and call the private function into getTitle() to force other derivatives to re-implement the getTitle() method but there are several core classes using LocalTaskDefault for derivatives directly. We looked into ViewsLocalTask and SearchLocalTask which then end up using t() on the title entered by user which opens a XSS security hole.

We decided to create a new sub-class ModuleLocalTask to add private method which is using t() and removed t() from getTitle() in LocalTaskDefault class. In addition to this change we added a new YAMLDistinctDiscovery class for YAML files to be able to add a distinct class to plugin definitions found directly in YAML files as opposed to those created as derivatives.

pwolanin’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 16: 2338081-16.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
16.6 KB
7.12 KB

Test fix, new test, plus fix to function getLocalizedTitle()

effulgentsia’s picture

FileSize
17.26 KB
610 bytes

Merging #10 to see if this fixes the original bug here.

YesCT’s picture

Status: Needs review » Needs work

These notes I'm just saving for now, cause dinner is about to happen. Hope to return to this tonight.
Some of this is notes, some is questions, and some is coding standard nits.

Needs work, to at least make the class required in YamlDistinctDiscovery

  1. +++ b/core/lib/Drupal/Core/Menu/LocalTaskDefault.php
    @@ -76,15 +76,12 @@ public function getRouteParameters(RouteMatchInterface $route_match) {
    -    return $this->t($this->pluginDefinition['title'], $args, $options);
    +    // Substitute tokens without translation.
    +    return strtr($this->pluginDefinition['title'], $args);
    

    this comment makes sense, for the diff. But if we want to document that, we should really do it in the method @return docs.

  2. +++ b/core/lib/Drupal/Core/Menu/ModuleLocalTask.php
    @@ -0,0 +1,49 @@
    +class ModuleLocalTask extends LocalTaskDefault {
    +  use StringTranslationTrait;
    

    I guess we dont have a standard for if to have a newline or not between class and use trait. But I think we should since
    https://www.drupal.org/node/608152#indenting
    "Leave an empty line between start of class/interface definition and ..."

  3. +++ b/core/lib/Drupal/Core/Menu/ModuleLocalTask.php
    @@ -0,0 +1,49 @@
    +   * This method is a private because t() is called on plugin definition. In
    +   * this case title is coming from a YAML file so we know it's safe to call t().
    +   * However the title may not come from a YAML in a subclass so this method
    +   * needs to be re-implemented to ensure title is handled correctly.
    

    nit, one line over 80 chars.

  4. +++ b/core/lib/Drupal/Core/Menu/ModuleLocalTask.php
    @@ -0,0 +1,49 @@
    +  }
    +}
    \ No newline at end of file
    

    newline before the class closing }

    and a newline after. :)

  5. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDistinctDiscovery.php
    @@ -0,0 +1,78 @@
    +  protected $class;
    ...
    +  function __construct($name, array $directories, $class = '') {
    

    assign class to class

  6. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDistinctDiscovery.php
    @@ -0,0 +1,78 @@
    +   * @param string $name
    +   *   The file name suffix to use for discovery. E.g. 'test' will become
    +   *   'MODULE.test.yml'.
    

    MODULE?

  7. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDistinctDiscovery.php
    @@ -0,0 +1,78 @@
    +    if ($this->class) {
    +      $default['class'] = $this->class;
    +    }
    

    change so required.

  8. +++ b/core/modules/comment/src/Plugin/Menu/LocalTask/UnapprovedComments.php
    @@ -8,14 +8,16 @@
    -class UnapprovedComments extends LocalTaskDefault implements ContainerFactoryPluginInterface {
    +class UnapprovedComments extends ModuleLocalTask implements ContainerFactoryPluginInterface {
    

    unapproved comment is using t() on "unapproved comments @count" which is not user entered title.

  9. +++ b/core/modules/config_translation/src/Plugin/Menu/LocalTask/ConfigTranslationLocalTask.php
    @@ -9,11 +9,13 @@
     class ConfigTranslationLocalTask extends LocalTaskDefault {
    

    Jut a note: we know this is a derivative because in the local.task.yml, it says "deriver:"

    For derivers, we do not know if using t is ok, and we should check.

  10. +++ b/core/modules/system/tests/modules/menu_test/src/Plugin/Menu/LocalTask/TestTasksSettingsSub1.php
    @@ -8,8 +8,10 @@
    + use StringTranslationTrait;
    

    when ever in the patch, we are making a change like this, where adding the trait for translation, it means we have (or should have) checked where it is using t() and that it is ok.

    an example reason for it to be ok to use t, is that the thing it is translating is coming from yml.

    is there another example reason for being ok.

  11. +++ b/core/modules/tracker/src/Plugin/Menu/UserTrackerTab.php
    @@ -7,13 +7,13 @@
    -class UserTrackerTab extends LocalTaskDefault {
    +class UserTrackerTab extends ModuleLocalTask {
    

    this is an example where we allowing it to use t, by changing it to use ModuleLocalTask which includes t.

    wait, why are we making this change? it is not implementing getTitle, and it does not use t().

  12. +++ b/core/tests/Drupal/Tests/Core/Menu/ModuleLocalTaskTest.php
    @@ -0,0 +1,119 @@
    +  protected function setUp() {
    

    I'm pretty sure we inheritdoc setUp()

    https://www.drupal.org/node/325974

  13. +++ b/core/tests/Drupal/Tests/Core/Menu/ModuleLocalTaskTest.php
    @@ -0,0 +1,119 @@
    +}
    +
    

    extra ending newline

effulgentsia’s picture

I think #19/#20 is on the right track with respect to the key goals:

  1. To not t() the theme name that comes from the .info.yml file. This was an open question I had from #7, but I took a closer look at HEAD and noticed that nothing else from our .info.yml files is being t()'d anywhere. Per #7, I don't know what D8MI has to say about that, but I think any solution for it needs to tackle it generically for all of .info.yml, not special-case the theme name only when it appears in a local task. So +1 to #20 fixing that bug/side-effect from HEAD.
  2. To preserve the automatic t()'ing of titles that are defined in *.links.*.yml. To not do so would be a regression from D7. This surfaces that we have an inconsistency between t()'ing some non-config YML files and not others, but that's a pre-existing condition of HEAD.
  3. To not automatically t() any other link derivative plugin whose title doesn't come from the *.links.*.YML file.

However, I'm not crazy about the implementation. It makes the plugin classes deal with knowing if the $definition['title'] came from which YAML file. And at least theoretically, there's no need for those two things to be coupled. It also doesn't deal with hook_menu_links_discovered_alter(), which is another place that code could set the title to a variable that should not go through t().

Here's an idea for a completely alternate approach: subclass YamlDiscovery (or bake into it) with the ability to receive which YAML keys should be translated, and for those keys, have it create a TranslationWrapper object around the value. This then results in the same definition processing as AnnotatedClassDiscovery uses when it deals with a @Translation annotation. This then allows getTitle() to simply be return (string) $definition['title'];. And it means that whenever a deriver or an alter hook wants to set the title to something it wants t()'d, it needs to call t() itself, just like for annotation-based plugins.

Thoughts?

kgoel’s picture

Status: Needs work » Needs review
FileSize
17.3 KB
6.15 KB

Mostly fixed nitpicks in the patch. I haven't addressed #21 - 5, and 7 and #22 in this patch.

kgoel’s picture

FileSize
17.25 KB
1.08 KB

Addressed #21 - 5, 7. Separate patch than 23 because class is not optional in YamlDistinctDiscovery.php constructor. I want to see if it break things.
Still need to address #22

Gábor Hojtsy’s picture

Re @effulgentsia:

#22/1: Theme names and module names should not indeed be t()-ed. However I believe we do t() module descriptions when shown on the admin UI, so to say no info.yml string is translated IMHO is not correct. If we don't translate descriptions that would be sad. The reason we don't translate module names is for searchability, docs, etc. Basically you should be able to tell which modules you use so you can look up updates, docs, etc. for them, If we'd translate names, that would be much harder.

#22/2 and 3: Drupal 7 has hook_menu() which specifies a translation callback (by default t). We don't have that in Drupal 8, so we assume all tabs defined in links.yml are to be translated. Your suggested solution may work depending on how things are cached :) I don't have a strong opinion either way where the translation happens.

Gábor Hojtsy’s picture

Issue tags: +language-ui
davidhernandez’s picture

Issue tags: +D8 Accelerate
pwolanin’s picture

@effulgentsia - I don't really understand your suggestion about the translation wrapper, and I feel like you are ot being very clear about what happens at discovery time vs. at run time.

Can you point to an example where @Translation is solving this for another plugin?

effulgentsia’s picture

FileSize
8.41 KB

Sorry for the lack of clarity in #22. Here's a POC of what I mean.

Status: Needs review » Needs work

The last submitted patch, 29: 2338081-alternate-29.patch, failed testing.

pwolanin’s picture

Discussed this with Alex - this seems a viable (and better) approach than what we'd started before and would bring YAML-discovered plugins in line with Annotation-discovered ones.

Let's broaden this patch to include local tasks, local actions, and contextual links, and then tackle menu links, and other YAML-discovered items in 2 follow-up patches.

effulgentsia’s picture

pwolanin’s picture

Status: Needs work » Needs review
FileSize
13.71 KB
6.14 KB

The failing test is really slow. This patch has some changes in line w/ what effulgentsia and I discussed earlier today. In particular, adds a method to duplicate a translation wrapper object with new arguments so we can set arguments at runtime while assuring the original is immutable.

Might have more fails, but maybe solved the exceptions.

Fabianx’s picture

+++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
@@ -48,6 +66,24 @@ public function getDefinitions() {
+            $definition[$property] = new TranslationWrapper($definition[$property], $arguments, $options);

Would this not implicitly mean that we run t($definition['title']) again when this is printed?

Did we not just hide the problem with that?

Or do we already trust YamlDiscovered strings and such only exclude user included strings that are then auto-escaped?

Can we add a specific test for that? (if there is none yet.)

Thanks!

effulgentsia’s picture

Issue tags: +Needs tests

#34 only runs when new YamlDiscovery specifies translatable properties, which means in this patch, only for the *.links.*.yml files. Which means it specifically does not run for a theme's name that comes from its info.yml file, which is the one test we already have (see the failure in #10), but I agree the patch needs unit tests added for the new YamlDiscovery capability to be passed translatable properties, and maybe an integration test for a deriver and/or alter hook setting the title and making sure t() is not called on that.

pwolanin queued 33: 2338081-33.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 33: 2338081-33.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
13.31 KB
pwolanin’s picture

Title: template_preprocess_menu_local_task() requires explicit plaining of link titles » Local Tasks, Actions, and Contextual links should use a TranslationWrapper to encapsulate safe translatable strings from YAML files
Issue summary: View changes
FileSize
17.07 KB
3.76 KB

apply the same changes to actions and contextual links, let's see which tests fail.

Status: Needs review » Needs work

The last submitted patch, 39: 2338081-yaml-translate-39.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
23.42 KB
6.35 KB

Unit test fixups.

pwolanin’s picture

Removing the translation trait from the default plugins to discourage calling $this->t() on user input.

Status: Needs review » Needs work

The last submitted patch, 42: 2338081-yaml-translate-42.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
25.15 KB
695 bytes

test fix

Status: Needs review » Needs work

The last submitted patch, 44: 2338081-yaml-translate-44.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
28.88 KB
3.73 KB

fix some more test classes.

Fabianx’s picture

  1. +++ b/core/lib/Drupal/Core/Menu/LocalActionDefault.php
    @@ -7,8 +7,9 @@
    +use Drupal\Core\DependencyInjection\DependencySerializationTrait;
    
    @@ -19,6 +20,8 @@
    +  use DependencySerializationTrait;
    +
    

    Is that 'use' needed here?

    And if yes, why not for ContextualLinkDefault.

    Methods look similar.

  2. +++ b/core/lib/Drupal/Core/Menu/LocalTaskDefault.php
    @@ -75,16 +75,7 @@ public function getRouteParameters(RouteMatchInterface $route_match) {
    +    return (string) $this->pluginDefinition['title'];
    

    Lets add the comment here as well.

  3. +++ b/core/modules/comment/comment.links.task.yml
    @@ -24,7 +24,7 @@ comment.admin_new:
    -  title: 'Unapproved comments'
    +  title: 'Unapproved comments (@count)'
    

    Hurray!

  4. +++ b/core/modules/comment/src/Plugin/Menu/LocalTask/UnapprovedComments.php
    @@ -8,8 +8,10 @@
    +use Drupal\Core\DependencyInjection\DependencySerializationTrait;
    
    @@ -17,6 +19,8 @@
    +  use DependencySerializationTrait;
    +
    

    Is this still needed here?

  5. +++ b/core/modules/system/tests/modules/menu_test/src/Plugin/Menu/LocalAction/TestLocalAction4.php
    @@ -8,12 +8,15 @@
    +use Drupal\Core\StringTranslation\StringTranslationTrait;
    ...
    +  use StringTranslationTrait;
    +
    
    +++ b/core/modules/system/tests/modules/menu_test/src/Plugin/Menu/LocalTask/TestTasksSettingsSub1.php
    @@ -8,9 +8,12 @@
    +use Drupal\Core\StringTranslation\StringTranslationTrait;
    ...
    +  use StringTranslationTrait;
    +
    

    Those "seem" to be unused?

Gábor Hojtsy’s picture

Issue tags: +sprint
pwolanin’s picture

@Fabianx - I moved the default plugins from Core to Component PluginBase, so yes, I think all the places I added a "use" for a trait it's needed.

pwolanin’s picture

FileSize
29.43 KB
9.41 KB

per discussion with @effulgentsia this adds use DependencySerializationTrait; to all 3 of the default classes since that will help developers avoid bugs if they subclass.

In contrast, we do not want to supply StringTranslationTrait by default, but force devs to add it if they have a good reason, since translation of anything coming from YAML is now handled by the use of the Translation Wrapper object.

pwolanin queued 50: 2338081-50.patch for re-testing.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, thanks for addressing my feedback.

pwolanin’s picture

Priority: Major » Critical
Issue tags: -Needs issue summary update, -sprint +D8 Security Bounty, +Security

Bumping to critical since there are unresolved D8 security holes this fixes.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkDefault.php
    @@ -7,31 +7,22 @@
    +  use DependencySerializationTrait;
    

    This needs a comment ... as this class does not need this.

  2. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkDefault.php
    @@ -7,31 +7,22 @@
    +  use DependencySerializationTrait;
    

    Why is this necessary?

  3. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
    @@ -10,6 +10,7 @@
     /**
      * Allows YAML files to define plugin definitions.
    

    The class docblock needs updating for the new functionality.

  4. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
    @@ -33,9 +39,21 @@ class YamlDiscovery implements DiscoveryInterface {
    +   * @param array $translatable_properties
    +   *   An array of properties whose values should be treated. For example, if
    +   *   the values for the 'title' and 'description' keys in the YAML need
    +   *   translation, and if the corresponding translation arguments and context
    +   *   can be found in the 'title_arguments', 'title_context',
    +   *   'description_arguments', and 'description_context' keys, then
    +   *   $translation_properties should be set to:
    +   *   [
    +   *     'title' => ['title_arguments', 'title_context'],
    +   *     'description' => ['description_arguments', 'description_context'],
    +   *   ].
        */
    -  function __construct($name, array $directories) {
    +  function __construct($name, array $directories, $translatable_properties = array()) {
    

    I'm was not super-convinced by adding this functionality to YamlDiscovery but @pwolanin pointed out that this is kind of bring this into line with annotated discovery. That is true. However, there is not a single use of title_arguments or title_context in core. At the very least this patch needs to add test coverage to ensure it is not breaking anything. That said, I;m unconvinced that translation arguments part of this is actually required. I can not see the use case - the string string is in the YAML file - just put the arguments in the string. Anything dynamic will have to be added by code eg. the comment count.

  5. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -70,6 +70,16 @@ public function getUntranslatedString() {
       /**
    +   * Gets all arguments from this translation wrapper.
    +   *
    +   * @return array
    +   *   The array of arguments.
    +   */
    +  public function getArguments() {
    +    return $this->options;
    +  }
    
    @@ -83,6 +93,16 @@ public function getOption($name) {
       /**
    +   * Gets all options from this translation wrapper.
    +   *
    +   * @return array
    +   *   The array of options.
    +   */
    +  public function getOptions() {
    +    return $this->options;
    +  }
    

    Unused and untested.

  6. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -90,6 +110,25 @@ public function __toString() {
       /**
    +   * Renders a clone of the object with updated arguments and options.
    +   *
    +   * @param array $arguments
    +   *   (optional) An array with placeholder replacements, keyed by placeholder.
    +   *   If NULL, the current arguments will be used.
    +   * @param array $options
    +   *   (optional) An array of additional options. If NULL, the current options
    +   *   will be used.
    +   *
    +   * @return  \Drupal\Core\StringTranslation\TranslationWrapper
    +   *   The new object.
    +   */
    +  public function cloneWithArgs(array $arguments = NULL, array $options = NULL) {
    +    $arguments = isset($arguments) ? $arguments : $this->arguments;
    +    $options = isset($options) ? $options : $this->options;
    +    return new static($this->string, $arguments, $options);
    +  }
    

    The $options is not used and I'm not sure it is useful. I think we could rename this setNewArguments(array $arguments) that returns a clone - so the original object is not changed. The caller could call getArguments if it needs to merge. Basically this method looks way to helpful to me.

alexpott’s picture

Also if this is fixing security holes we should have tests showing that user generated content is no longer marked safe due to this.

dawehner’s picture

Status: Needs work » Needs review
FileSize
31.55 KB
11.8 KB

Still working on an integration test coverage ...

This needs a comment ... as this class does not need this.

Let's remove it. If a subclass needs it, it can be added again, just like for the other plugins which are part of the patch.

That said, I;m unconvinced that translation arguments part of this is actually required.

We added that because it was a regression relative to Drupal 7 and people requested it to be added everywhere. At least the context certainly makes sense, once you ever have been in the multilingual world of problems.

Unused and untested.

And broken ...

The $options is not used and I'm not sure it is useful. I think we could rename this setNewArguments(array $arguments) that returns a clone - so the original object is not changed. The caller could call getArguments if it needs to merge. Basically this method looks way to helpful to me.

Renamed the method in order to be more align with PSR-7 style immutable object clones. While doing that it was possible to rewrite the uses to be more readable IMHO.

dawehner’s picture

Added the first integration test coverage for local tasks ... you should really better start with an integration

larowlan’s picture

Couple of questions/minor nits

  1. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
    @@ -33,9 +49,21 @@ class YamlDiscovery implements DiscoveryInterface {
    +   *   An array of properties whose values should be treated. For example, if
    

    'An array of properties whose values should be treated' - should that be 'translated'? Either way I'm not sure what that means.

  2. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
    @@ -33,9 +49,21 @@ class YamlDiscovery implements DiscoveryInterface {
    +    $this->translatableProperties = $translatable_properties;
    

    Should we validate these here? We're expecting a particular format e.g. 'field_name' => ['arguments_field_name', 'context_field_name'] so I think we should make sure the argument matches that format and throw an exception if it does not. This will help developers with understanding but also save warnings later when we call list()

  3. +++ b/core/modules/comment/src/Plugin/Menu/LocalTask/UnapprovedComments.php
    @@ -57,7 +58,11 @@ public static function create(ContainerInterface $container, array $configuratio
    +    $title = $this->pluginDefinition['title'];
    +    if ($title instanceof TranslationWrapper) {
    

    UnapprovedComments should always use a TranslationWrapper right? otherwise the @count will remain - does that mean we can forego the instanceof?

  4. +++ b/core/tests/Drupal/Tests/Core/Plugin/Discovery/YamlDiscoveryTest.php
    @@ -71,6 +73,46 @@ public function testGetDefinitions() {
    +    $file_1 = <<<'EOS'
    +test_plugin:
    +  title: test title
    +EOS;
    +    $file_2 = <<<'EOS'
    +test_plugin2:
    +  title: test @count
    +  title_arguments: { @count: 12 }
    +  title_context: 'test-context'
    +EOS;
    +    vfsStream::create([
    +      'test_1' => [
    +        'test_1.test.yml' => $file_1,
    +      ],
    +      'test_2' => [
    +        'test_2.test.yml' => $file_2,
    +      ]]
    +    );
    

    nice!

Gábor Hojtsy’s picture

Issue tags: +sprint

I think the sprint tag removal was a mistake?

dawehner’s picture

Interesting I don't even see the comment which removes the tag.

mbrett5062’s picture

See #53, when this was bumped to critical :)

dawehner’s picture

@mbrett5062
OH I'm blind.

So @alexpott was asking in IRC why we not escape the string passed into t(). It is marked as safe afterwards so there is no danger of double escaping then.
This would not require us all the other changes in the current approach.

lauriii’s picture

I have tried in some other issue to change SafeMarkup::set to SafeMarkup::escape and it only caused less than 40 test failures so it definitely could be considered as a solution :)

josephdpurcell’s picture

+++ b/core/lib/Drupal/Core/Menu/ContextualLinkDefault.php
@@ -17,21 +17,10 @@ class ContextualLinkDefault extends PluginBase implements ContextualLinkInterfac
   public function getTitle(Request $request = NULL) {

Two problems: (1) $request is not used in this method, and (2) adding a parameter violates the ContextualLinkInterface.

Suggestion: since the only place a parameter is passed in is ContextualLinkDefaultTest->testGetTitleWithTitleArguments(), remove that test method and remove the $request parameter on getTitle.

Side note: the parameter was added by catch in commit 29110623 for issue #2120235. It would be worth confirming a regression was not introduced at some point.

tim.plunkett’s picture

It only violates the interface if it is required; since it is optional this is legal. However, you're right that it is unused.

josephdpurcell’s picture

It only violates the interface if it is required; since it is optional this is legal.

I apologize that I don't know how closely the Drupal community follows SOLID principles. I have created issue #2535750 to discuss this, but in short, the need for a subclass to modify what ContextualLinkInterface states as the contract suggests that either ContextualLinkDefault violates LSP or ContextualLinkInterface violates the Open/Closed principle. If the method does need arguments we have lots of options, and hopefully we can find those in #2535750.

vijaycs85’s picture

as per #62, just marking all title as SafeMarkup::escape.

Status: Needs review » Needs work

The last submitted patch, 67: 2338081-57.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
6.98 KB

Adding tests from #57 and using htmlspecialchars() instead of SafeMarkup::escape().

Status: Needs review » Needs work

The last submitted patch, 69: 2338081-68.patch, failed testing.

mpdonadio’s picture

@vijaycs85, make sure you post an interdiff with each patch so we can see what changes patch to patch.

Haven't been following this too closely, but

+++ b/core/lib/Drupal/Core/Menu/ContextualLinkDefault.php
@@ -31,7 +31,7 @@ public function getTitle(Request $request = NULL) {
 
-    return $this->t($this->pluginDefinition['title'], $args, $options);
+    return $this->t(htmlspecialchars($this->pluginDefinition['title'], ENT_QUOTES, 'UTF-8'), $args, $options);
   }
 

This will likely result in double escaping, first from htmlspecialchars() and then from t(). Using SafeMarkup() will mark the string as safe, so t() won't attempt to sanitize it again. The TestBot log doesn't show the details, but what exactly was going wrong with the fails in #67 when you run locally? Those tests were the ones that prompted this whole issue.

mpdonadio’s picture

BTW, I saw that the D8 Security Bounty tag was added a few days ago. I don't know who the intended recipient is supposed to be, but if it is me for being the original reporter back in October/14, (1) the DA can keep the money and (2) @chx found the bug and asked me to report it and essentially rewrote the IS based on a IRC conversation we had while he was helping me work through failing tests on #2273923: Remove html => TRUE option from l() and link generator..

vijaycs85’s picture

#71 @mpdonadio, we don't have diff as all the 3 patches are generated from HEAD (#57, #67 and #69), so no inter-diff. I'm trying to implement #62.

dawehner’s picture

This will likely result in double escaping, first from htmlspecialchars() and then from t().

No, everything in t() will be marked directly as safe.

  public function translate($string, array $args = array(), array $options = array()) {
    $string = $this->doTranslate($string, $options);
    if (empty($args)) {
      return SafeMarkup::set($string);
    }
    else {
      return SafeMarkup::format($string, $args);
    }
  }
dawehner’s picture

FileSize
2.55 KB

Here is also a test for local actions.

mpdonadio’s picture

FileSize
674 bytes

#74, I stand corrected. If this approach is taken, I think a comment may be warranted. It's an odd construct to stumble upon.

#73, I get it and thanks for working on this issue (it is a rather tricky one). Interdiffs help in cases like this so you can see changes patch to patch, unless they are radically different.

#67

+++ b/core/lib/Drupal/Core/Menu/ContextualLinkDefault.php
@@ -31,7 +32,7 @@ public function getTitle(Request $request = NULL) {
 
-    return $this->t($this->pluginDefinition['title'], $args, $options);
+    return $this->t(SafeMarkup::escape($this->pluginDefinition['title']), $args, $options);
   }
 

I ran this locally, and the fails are from double escapes. I am pretty sure this is coming from template_preprocess_menu_local_task() directly. Remove the `else` and the @todo (nearly sure I added that). See attached. BlockTest will then pass.

#69:

+++ b/core/modules/system/src/Tests/Menu/MenuRouterTest.php
@@ -72,6 +72,9 @@ protected function doTestHookMenuIntegration() {
     $this->assertLink('Local task B');
+    $this->assertNoLink('Local task C');
+    $this->assertNoRaw("<script>alert('Welcome to the jungle!')</script>");
+    $this->assertRaw(htmlspecialchars("<script>alert('Welcome to the jungle!')</script>", ENT_QUOTES, 'UTF-8'));
     // Confirm correct local task href.
     $this->assertLinkByHref(Url::fromRoute('menu_test.router_test1', ['bar' => $machine_name])->toString());

I think single ->assertEscaped() should be OK here instead of assertNoRaw() + explicitly calling htmlspecialchars.

So, I think the patch in #67 + removing the @todo is the proper place to continue from.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +Needs issue summary update

thanks @mpdonadio.
1. removed else case in menu.inc
2. updated test with assertEscaped
3. Added tests from #75

dawehner’s picture

@vijaycs85
Do you mind sharing the files :p

vijaycs85’s picture

FileSize
10.08 KB
4.18 KB

:)

Status: Needs review » Needs work

The last submitted patch, 79: 2338081-77.patch, failed testing.

mpdonadio’s picture

+++ b/core/lib/Drupal/Core/Menu/ContextualLinkDefault.php
@@ -31,7 +31,7 @@ public function getTitle(Request $request = NULL) {
 
-    return $this->t($this->pluginDefinition['title'], $args, $options);
+    return $this->t(htmlspecialchars($this->pluginDefinition['title'], ENT_QUOTES, 'UTF-8'), $args, $options);
   }
 

Should probably be SafeMarkup::escape() instead of htmlspecialchars()? Or are we using htmlspecialchars() so the string doesn't get marked as safe right off the bat? If the later, then a comment is probably warranted.

Re, the test fail, I don't think TestLocalAction5 needs StringTranslationTrait. If you yank that out, then you will see the fail. The text on the page is properly escaped if you look at the verbose output, but it looks like ->xpath in the test is resulting in the entities in what it pulls out getting decoded.

dawehner’s picture

Should probably be SafeMarkup::escape() instead of htmlspecialchars()? Or are we using htmlspecialchars() so the string doesn't get marked as safe right off the bat? If the later, then a comment is probably warranted.

Yeah I agree we should add a comment, why we do that.

Re, the test fail, I don't think TestLocalAction5 needs StringTranslationTrait. If you yank that out, then you will see the fail. The text on the page is properly escaped if you look at the verbose output, but it looks like ->xpath in the test is resulting in the entities in what it pulls out getting decoded.

Ah well, we could just go with assertRaw I guess?

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

I don't think I need to stay out of working on this, so I am going take #79, add in some comments, and get it to green. I will also take a stab at the IS update. Should have something posted tonight.

pwolanin’s picture

The current patch in #79 is just wrong, imho. It misses the part of the problem that needs to be solved of distinguishing static strings from user input so that the latter does not end up in the translation database.

Let's go back to the patch in #57, while rolling in any added test in the last patch.

dawehner’s picture

Title: Local Tasks, Actions, and Contextual links should use a TranslationWrapper to encapsulate safe translatable strings from YAML files » Local Tasks, Actions, and Contextual links mark strings from derivatives as safe and translated

First, let's fix the title.

dawehner’s picture

Issue summary: View changes

Sorry @vijaycs85, peter is right, and alex wasn't aware of all the consequences of his approach. Thank you for your work so far.

Updated the issue summary to explain a bit better what is going on.

mpdonadio’s picture

@dawehner @pwolanin, I have the IRC conversation saved and will double check the IS w/ what you guys hashed out.

dawehner’s picture

Status: Needs work » Needs review
FileSize
37.13 KB
2.55 KB

This adds the local action test for now. We still should add derivative based tests and one for contextual links.

Interdiff is relative to #57

Status: Needs review » Needs work

The last submitted patch, 88: 2338081-87.patch, failed testing.

alexpott’s picture

Yep I was was about the htmlspecialchars() approach - sorry @vijaycs85 and @pwolanin. I'm still concerned about put translation into yaml discovery - but this is far less concerning that using t() on a user entered string.

pwolanin’s picture

I'm going to work on this to change a class as discussed with dawhener in IRC and fix the 1 test fail.

effulgentsia’s picture

I'm still concerned about put translation into yaml discovery

What's the concern? We have it in Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery (via getAnnotationReader()), so what's concerning about adding it to Drupal\Core\Plugin\Discovery\YamlDiscovery?

+++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
@@ -33,9 +49,21 @@ class YamlDiscovery implements DiscoveryInterface {
+   * @param array $translatable_properties
+   *   An array of properties whose values should be treated. For example, if
+   *   the values for the 'title' and 'description' keys in the YAML need
+   *   translation, and if the corresponding translation arguments and context
+   *   can be found in the 'title_arguments', 'title_context',
+   *   'description_arguments', and 'description_context' keys, then
+   *   $translation_properties should be set to:
+   *   [
+   *     'title' => ['title_arguments', 'title_context'],
+   *     'description' => ['description_arguments', 'description_context'],
+   *   ].

Do we want to add/keep support for arguments and context being YAML-provided? I initially added it to this patch for consistency with the @Translation annotation, which supports those as options. But in core, we have no use case for annotation-provided arguments, and the annotation-provided contexts are questionable. Most are the "Validation" context, but that should perhaps come from the plugin manager, not each plugin, and the only truly class-specific one is in Drupal\views\Entity\View, but is "View entity type" a useful translation context, or should we make "Entity type" the context, and provide it via the manager?

It's out of scope for this issue to make changes to Annotation discovery, but just asking this to figure out what we want for YamlDiscovery before blindly copying something that might not make sense.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
37.72 KB
878 bytes

Here's with just the text fix - it's not as pretty, but I should verify the escaping in the raw HTML.

pwolanin’s picture

Title: Local Tasks, Actions, and Contextual links mark strings from derivatives as safe and translated » Local Tasks, Actions, and Contextual links mark strings from derivatives (or alter hooks) as safe and translated
Issue summary: View changes
FileSize
41.49 KB
5 KB

I think dawehner and effulgentsia discussed this via chat and came to agreement to proceed without adding another discovery class since translation is arguably an essential distinction already of Core vs. Component.

Added a new unit test and a tweak to TranslationWrapper

mpdonadio’s picture

Assigned: mpdonadio » Unassigned

Just taking myself off this as it looks like it is good hands. I'll look at the IS later and edit or remove the tag, as needed.

pwolanin’s picture

Add some test cases and restore some testing of hook_menu_local_tasks() which seems to have been totally lost.

pwolanin’s picture

oops - forgot to git add the new unit test locally.

Plus a start on testing for contextual links.

@mpdonadio re: #72. I added the D8 Security Bug Bounty tag because this problem was found by multiple people. they found problems with local tasks, etc, that were not covered by the original issue description.

So, it wasn't intended to imply reward, but rather that the program re-focused attention to this problem.

Status: Needs review » Needs work

The last submitted patch, 97: 2338081-97.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
48.7 KB
809 bytes

oops - duplicated a couple route paths. This should fix it.

Gábor Hojtsy’s picture

Issue summary: View changes

re

We should create a follow-up issue to amend the t() documentation because it's not crystal clear that what gets passed into it must be a safe string that is not user input. This is not a problem 99% of the time.

we have #2488304: Add more docs that translate() also marks strings as safe where reviewers @xjm, @jhodgdon, etc. pushed back that its already documented on the interfaces, even though we wanted to stress the point exactly for these reasons. Updated the issue summary accordingly.

Gábor Hojtsy’s picture

Reviewed the patch. Makes sense, looks good. Mostly just docs related issues found:

  1. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
    @@ -33,9 +49,21 @@ class YamlDiscovery implements DiscoveryInterface {
    +   *   An array of properties whose values should be treated. For example, if
    

    values should be treated [as translatable]?

  2. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
    @@ -33,9 +49,21 @@ class YamlDiscovery implements DiscoveryInterface {
    +   *   the values for the 'title' and 'description' keys in the YAML need
    +   *   translation, and if the corresponding translation arguments and context
    +   *   can be found in the 'title_arguments', 'title_context',
    +   *   'description_arguments', and 'description_context' keys, then
    +   *   $translation_properties should be set to:
    +   *   [
    +   *     'title' => ['title_arguments', 'title_context'],
    +   *     'description' => ['description_arguments', 'description_context'],
    +   *   ].
    

    While Ideally they would support arguments and context, what if some YAML discovered plugin does not? How should they specify this? Sounds like from the code below that these are required. Should document that then?

  3. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -92,6 +112,22 @@ public function __toString() {
    +  public function withArguments(array $arguments) {
    

    Sound like this is a cloneWithMergedArguments() or something like that? Maybe withArguments() is as clear to others, just noting if not :)

  4. +++ b/core/modules/comment/comment.links.task.yml
    @@ -24,7 +24,7 @@ comment.admin_new:
    +  title: 'Unapproved comments (@count)'
    
    +++ b/core/modules/comment/src/Plugin/Menu/LocalTask/UnapprovedComments.php
    @@ -57,7 +58,11 @@ public static function create(ContainerInterface $container, array $configuratio
    -    return t('Unapproved comments (@count)', array('@count' => $this->commentStorage->getUnapprovedCount()));
    +    $title = $this->pluginDefinition['title'];
    +    if ($title instanceof TranslationWrapper) {
    +      $title = $title->withArguments(['@count' => $this->commentStorage->getUnapprovedCount()]);
    +    }
    

    Woo, fancy!

  5. +++ b/core/modules/config_translation/src/Plugin/Menu/ContextualLink/ConfigTranslationContextualLink.php
    @@ -26,17 +27,16 @@ class ConfigTranslationContextualLink extends ContextualLinkDefault {
    +      // Take custom 'config_translation_plugin_id' plugin definition key to
    +      // retrieve title. We need to retrieve a runtime title (as opposed to
    
    +++ b/core/modules/config_translation/src/Plugin/Menu/LocalTask/ConfigTranslationLocalTask.php
    @@ -26,17 +27,16 @@ class ConfigTranslationLocalTask extends LocalTaskDefault {
         // Take custom 'config_translation_plugin_id' plugin definition key to
         // retrieve title. We need to retrieve a runtime title (as opposed to
    

    I see this is a preexisting problem, but this first sentence is pretty non-English. Either "Take [the value under the] custom ..." or "[Use the] custom ..." IMHO.

  6. +++ b/core/modules/system/tests/modules/menu_test/src/Plugin/Menu/LocalAction/TestLocalAction4.php
    @@ -8,12 +8,15 @@
     use Drupal\Core\Menu\LocalActionDefault;
    +use Drupal\Core\StringTranslation\StringTranslationTrait;
     
     /**
      * Defines a local action plugin with a dynamic title.
      */
     class TestLocalAction4 extends LocalActionDefault {
     
    +  use StringTranslationTrait;
    +
    
    +++ b/core/modules/system/tests/modules/menu_test/src/Plugin/Menu/LocalAction/TestLocalAction5.php
    @@ -0,0 +1,27 @@
    +use Drupal\Core\StringTranslation\StringTranslationTrait;
    ...
    +  use StringTranslationTrait;
    
    +++ b/core/modules/system/tests/modules/menu_test/src/Plugin/Menu/LocalTask/TestTasksSettingsSub1.php
    @@ -8,9 +8,12 @@
    +use Drupal\Core\StringTranslation\StringTranslationTrait;
     
     class TestTasksSettingsSub1 extends LocalTaskDefault {
     
    +  use StringTranslationTrait;
    +
    

    Not actually used?

alexpott’s picture

+++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
@@ -33,9 +49,21 @@ class YamlDiscovery implements DiscoveryInterface {
+   * @param array $translatable_properties
+   *   An array of properties whose values should be treated. For example, if
+   *   the values for the 'title' and 'description' keys in the YAML need
+   *   translation, and if the corresponding translation arguments and context
+   *   can be found in the 'title_arguments', 'title_context',
+   *   'description_arguments', and 'description_context' keys, then
+   *   $translation_properties should be set to:
+   *   [
+   *     'title' => ['title_arguments', 'title_context'],
+   *     'description' => ['description_arguments', 'description_context'],
+   *   ].

I'm not sure I see the point of this. Translation arguments seem pointless as what we're dealing with a hard coded strings. Contexts are more interesting but what I think here is that Yaml discovery should add an automated context based on the plugin manager name or class and the plugin id. Maybe a method on YamlDiscovery. So for example context link plugin manager would look like:


  /**
   * {@inheritdoc}
   */
  protected function getDiscovery() {
    if (!isset($this->discovery)) {
      $discovery = new YamlDiscovery('links.contextual', $this->moduleHandler->getModuleDirectories());
      $discovery->setTranslationContext('Contextual links');
      $this->discovery = new ContainerDerivativeDiscoveryDecorator($discovery);
    }
    return $this->discovery;
  }

@Gábor Hojtsy what do you think?

dawehner’s picture

FileSize
3.75 KB

I tried to write test coverage for derivatives and realized that we don't support them for those plugin type, but I think from a test point of view it is valuable to provide test coverage.

Not actually used?

Right :)

Gábor Hojtsy’s picture

@alexpott: when a contextual link says "View" it does not tell if its to view something or it refers to something being a view. The context is supposed to disambiguate the string, "Contextual link" has no disambiguation value because it does not explain what the string refers to but where its used. It is usually impossible to automatically specify context like this exactly because it highly depends on the actual strings (and that is why we have title_context, et. al. in links and routes, etc).

brandon.holtsclaw’s picture

I'm at the GovCon sprint taking a look into the testcases for this with @YesCT

pwolanin’s picture

@alexpott - we do need the arguments since the YAML might provide a default that's optionally overridden dynamically or by a hook or derivatives.

@Gábor Hojtsy

  • 101.1: fixed doc
  • 101.2: made them optional and fixed the doc
  • 101.3: I think it's ok with docs
  • 101.4: yes
  • 101.5: edited comment
  • 101.6: It's correct. TestLocalAction4, and TestTasksSettingsSub1 do need use StringTranslationTrait; because we removed the trait from the class. For estLocalAction5 it needs to be removed (but missed that in this patch)

@dawhener we sure do for sure support derivatives - see e.g. \Drupal\views\Plugin\Derivative\ViewsLocalTask

Status: Needs review » Needs work

The last submitted patch, 106: 2338081-106.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
48.98 KB
511 bytes

silly little fix, I think.

brandon.holtsclaw’s picture

Adding test coverage for title escaping for ContextualLink

dawehner’s picture

Nice @brandon, I'm about to upload for local task and local action.

dawehner’s picture

Perfect teamwork!

Status: Needs review » Needs work

The last submitted patch, 111: 2338081-111.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
54.76 KB
1.2 KB

Let's fix them.

mpdonadio’s picture

Slight test tweaks. Both changed tests pass locally for me.

I don't understand the patch well enough to properly review it, but I think the test coverage looks good and I like assertLocalTaskAppers() w/ the comment on why it is needed.

alexpott’s picture

we do need the arguments since the YAML might provide a default that's optionally overridden dynamically or by a hook or derivatives.

But that would require the string predicting which bit needs to be an argument - this use case looks really thin to me. Also if some overrides it by a hook or derivatives doesn't that mean the context changes too?

dawehner’s picture

@alexpott
I agree that the amount of usecases feel rare, but it seems that removing support would be a major follow up, as it does not solve the xss part. We could get rid of withArguments, and call to t() with a hard coded string.

pwolanin’s picture

@alexpott - I'm really not understanding which things here you don't like. We are basically taking 3 fields from YAML we would directly pass to t() and instead passing them to t() via the TrasnlationWrapper to fix the bug. I defer to Gabor on the right way to do the i18n - we had the original system in place based on his feedback, so I don't see a good reason to truncate its capabilities in this patch.

chx’s picture

Status: Needs review » Needs work
+      // Make sure we have a two element array.
+      $this->translatableProperties[$property] = array_values((array) $args_context)  + ['', ''];

That's a $a++ // increment $a by 1 level comment. Why? I can see quite well you want to have a two element array but why? What are these two (empty default) strings?

Gábor Hojtsy’s picture

@alexpott:

But that would require the string predicting which bit needs to be an argument - this use case looks really thin to me. Also if some overrides it by a hook or derivatives doesn't that mean the context changes too?

But as explained in #104, we need the context for statically provided strings in YAML as well. Menu items, local tasks, etc. are usually short so are especially prone to error if no context is provided. Or are you not arguing that but that altering it does not allow for the context to be changed? Why not? Looks like just need to replace the title in plugin definition?

alexpott’s picture

I get the point of context and I can't see anyway of doing this in another way. Unless we enable object storage in YAML and just return the TranslationWrapper object directly.

But I just don't get why we need to support arguments. There is not a single usage in core of the argument capability in either Yaml backed definitions or Annotation backed definitions. In fact if the argument was actually dynamic wouldn't we have take of bubbling up some cacheability metadata. Actually reading the docs in TranslationWrapper:

 * This class can be used to delay translating strings until the translation
 * system is ready. This is useful for using translation in very low level
 * subsystems like entity definition and stream wrappers.

I think we should completely remove support for arguments from that because support arguments at extremely low level is extremely risky. Obviously removing that support is followup material but not adding that support to YamlDiscovery is the concern of this issue.

alexpott’s picture

Created #2538514: Remove argument support from Translation Annotation

[Edit: heddn was right! thanks :)]

heddn’s picture

pwolanin’s picture

Status: Needs work » Needs review
FileSize
55.35 KB
4.82 KB

ok, this should fix the derivatives test, which is the last needed test I think.

Still need to address chx's comment about the code comment, and maybe wait for alexpott's issue to be committed.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
@@ -33,9 +49,26 @@ class YamlDiscovery implements DiscoveryInterface {
+  function __construct($name, array $directories, array $translatable_properties = []) {
     $this->discovery = new ComponentYamlDiscovery($name, $directories);
+    foreach($translatable_properties as $property => $args_context) {
+      // Make sure we have a two element array.
+      $this->translatableProperties[$property] = array_values((array) $args_context)  + ['', ''];
+    }

$translatable_properties is an ArrayPI. How about we just move it into a method you call after construction. Something like this:

$dis = new YamlDiscovery('foo', $directories);
$dis->addTranslateProperty($value_key, $context_key);

(obviously if we decide to keep arguments that we should add an $arguments_key but as I'm trying to argue I don't think YamlDiscovery should support translation arguments)

pwolanin’s picture

Ok, adjusting it as suggested by alexpott.

dawehner’s picture

+1 for that change!

pwolanin’s picture

Looks like we may need to add CacheableDependencyInterface to local tasks, local actions, and contextual links as it was for menu links. Is there an existing issue?

alexpott’s picture

Status: Needs review » Needs work
Issue tags: +Triaged D8 critical
  1. Discussed with @xjm, @catch, @effulgentsia and @webchick and we agreed that this issue is critical because of the security concerns of passing user test into t().
  2. +++ b/core/lib/Drupal/Core/Menu/ContextualLinkDefault.php
    @@ -17,21 +17,10 @@ class ContextualLinkDefault extends PluginBase implements ContextualLinkInterfac
    +    return (string) $this->pluginDefinition['title'];
    
    +++ b/core/lib/Drupal/Core/Menu/LocalActionDefault.php
    @@ -68,15 +71,8 @@ public function getRouteName() {
    +    return (string) $this->pluginDefinition['title'];
    
    +++ b/core/lib/Drupal/Core/Menu/LocalTaskDefault.php
    @@ -75,16 +78,8 @@ public function getRouteParameters(RouteMatchInterface $route_match) {
    +    return (string) $this->pluginDefinition['title'];
    

    Do we have to do the string cast here? We could just say we return a string or SafeStringInterface

  3. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
    @@ -39,6 +55,20 @@ function __construct($name, array $directories) {
    +  public function addTranslatableProperty($value_key, $context_key = '') {
    +    $this->translatableProperties[$value_key] = $context_key;
    +  }
    

    Let's return $this so the it's fluent.

  4. +++ b/core/lib/Drupal/Core/StringTranslation/TranslationWrapper.php
    @@ -72,6 +72,16 @@ public function getUntranslatedString() {
       /**
    +   * Gets all arguments from this translation wrapper.
    +   *
    +   * @return string[]
    +   *   The array of arguments.
    +   */
    +  public function getArguments() {
    +    return $this->arguments;
    +  }
    
    @@ -85,6 +95,16 @@ public function getOption($name) {
       /**
    +   * Gets all options from this translation wrapper.
    +   *
    +   * @return array
    +   *   The array of options.
    +   */
    +  public function getOptions() {
    +    return $this->options;
    +  }
    

    Not needed.

The last submitted patch, 125: 2338081-125.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
49.22 KB
837 bytes

@alexpott -
Yes - we should do the string cast, since anyone using the result is going to expect a string and we could have a fatal if it was e.g. used as an array key. I don't think we want to introduce those subtle bugs by having a variable return value.

I disagree about the getter method for options since it would be useful if you wanted to get and manipulate all options , but getArguments is removed.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Menu/LocalActionManager.php
@@ -142,8 +142,9 @@ public function __construct(ControllerResolverInterface $controller_resolver, Re
+      $yaml_discovery = new YamlDiscovery('links.action', $this->moduleHandler->getModuleDirectories());
+      $yaml_discovery->addTranslatableProperty('title', 'title_context');
+      $this->discovery = new ContainerDerivativeDiscoveryDecorator($yaml_discovery);

Now that we have a fluent interface again, we could online it again

pwolanin’s picture

@dawehner - I don't think it would really make it clearer to make it more compact

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests
FileSize
49.22 KB
1.11 KB

Fair. Here are just two minor nitpicks.

I think this is ready now. We have the test coverage now.

catch’s picture

On the 8.x critical triage call last night we realised that search pages in core have an XSS vulnerability due to this - is it worth adding integration test coverage explicitly for that case also?

alexpott’s picture

Here are the steps to reproduce the XSS attack.

  1. Give user A permission 'administer search'
  2. As user A go to /admin/config/search/pages/manage/node_search
  3. Enter script in label textfield.
  4. As any user visit /search to trigger the script

I think it is okay to add this as a followup since we do add test coverage for local tasks in this patch.

Before committing it would be good to get a +1 form @Gábor Hojtsy as he has not commented since we removed argument support from YamlDiscovery.

catch’s picture

Overall patch looks good, apart from the search XSS test (we could also move that to a follow-up, but I think it's worth doing somewhere), I just had a couple of nits and one question.

  1. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
    @@ -10,9 +10,16 @@
    + * was written in the YAML file and not coming from some other source of
    

    nit:

    'was written in the YAML file and did not come from'.

  2. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
    @@ -39,6 +55,23 @@ function __construct($name, array $directories) {
    +   * Set one or more of the YAML values as being translateable.
    

    nit: translateable.

  3. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
    @@ -39,6 +55,23 @@ function __construct($name, array $directories) {
    +   *   The key correspnding to the value in the YAML that contains a tanslatable
    

    nits: corresponding, translatable.

  4. +++ b/core/modules/system/src/Tests/Menu/LocalActionTest.php
    @@ -50,7 +52,9 @@ protected function assertLocalAction(array $actions) {
    +      // SimpleXML gives us the unescaped text, not the actual escaped markup,
    +      // so use a pattern instead to check the raw content.
    +      $this->assertPattern('@<a [^>]*class="[^"]*button-action[^"]*"[^>]*>' . preg_quote($title, '@') . '</@');
           $this->assertEqual($elements[$index]['href'], $url->toString());
    

    That's... unfortunate - so it's decoding?

dawehner’s picture

That's... unfortunate - so it's decoding?

Yeah, a bit annoying.

dawehner’s picture

Sadly this behaviour cannot be turned off, see https://bugs.php.net/bug.php?id=49437.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record
  1. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
    @@ -10,9 +10,15 @@
    + * For translatable values of plugin defintions, like title, you can specify
    + * $translatable_properties as part of the constructor. This is used in order
    

    First sentence not true anymore.

  2. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
    @@ -10,9 +10,15 @@
    + * to use a translation wrapper to 100% indicate that something is safe, as it
    + * was written in the YAML file and did not come from plugin definition.
    

    Well, the YAML file is a plugin definition, right? Or am I caught in terminology hell? :) Whats more important is it is not user input, no?

  3. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
    @@ -39,6 +54,23 @@ function __construct($name, array $directories) {
    +   * Set one or more of the YAML values as being translatable.
    

    It can only set one of them to be translatable, not more. (That it supports context does not make the context translatable, its just metadata). So remove "or more" AFAIS.

  4. +++ b/core/modules/config_translation/src/Plugin/Menu/ContextualLink/ConfigTranslationContextualLink.php
    @@ -26,17 +28,12 @@ class ConfigTranslationContextualLink extends ContextualLinkDefault {
    -    return $this->t($this->pluginDefinition['title'], array('@type_name' => $type_name), $options);
    +    return $this->t('Translate @type_name', array('@type_name' => $type_name));
    
    +++ b/core/modules/config_translation/src/Plugin/Menu/LocalTask/ConfigTranslationLocalTask.php
    @@ -26,17 +28,12 @@ class ConfigTranslationLocalTask extends LocalTaskDefault {
    -    return $this->t($this->pluginDefinition['title'], array('@type_name' => $type_name), $options);
    +    return $this->t('Translate @type_name', array('@type_name' => $type_name));
    

    I get why we are not using these from the plugin definition anymore, because they depend on dynamic plaecholders. Why did we keep the plugin definitions with their title with the placeholder then? If we don't support these then we should not attempt to use them in core:

    config_translation.contextual_links:
      title: 'Translate @type_name'
    
    config_translation.local_tasks:
      title: 'Translate @type_name'

Also we need a change notice for the API changes made.

dawehner’s picture

Status: Needs work » Needs review
FileSize
49.43 KB
1.37 KB

I get why we are not using these from the plugin definition anymore, because they depend on dynamic plaecholders. Why did we keep the plugin definitions with their title with the placeholder then? If we don't support these then we should not attempt to use them in core:

I think this is unnecessary abstraction and makes it actually harder to follow.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I think the feedback from @catch got addressed.

Working on the change record now.

dawehner’s picture

There it is.

dawehner’s picture

Fixed the last point of @Gábor Hojtsy's review

cilefen’s picture

pwolanin’s picture

Gábor Hojtsy’s picture

  1. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
    @@ -10,9 +10,16 @@
    + * For translatable values of plugin defintions, like title, you can specify
    + * add translatable properties using addTranslatableProperty(). This is used in
    + *  order to use a translation wrapper to 100% indicate that something is safe,
    + *  as it was written in the YAML file and did not come from a dynamic plugin
    + *  definition.
    

    Indentation wrong, "...specify add..." wrong too.

  2. +++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
    @@ -39,6 +55,23 @@ function __construct($name, array $directories) {
    +   * Set one of the YAML values of the YAML values as being translatable.
    

    of the YAML values of the YAML values

dawehner’s picture

of the YAML values of the YAML values

YAML YAML, everybody YAML!

chx’s picture

I've rewritten the class doxygen. No code changes.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4b12bbf and pushed to 8.0.x. Thanks!

Also credit @JvE on the commit since they reported the bug to the D8 bug bounty program.

diff --git a/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
index b4a6f0e..0d519f4 100644
--- a/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
+++ b/core/lib/Drupal/Core/Plugin/Discovery/YamlDiscovery.php
@@ -16,11 +16,12 @@
  * Allows YAML files to define plugin definitions.
  *
  * If the value of a key (like title) in the definition is translatable then
- * the addTranslatableProperty() can be used to mark it as such and also to add
- * translation context. Then \Drupal\Core\StringTranslation\TranslationWrapper
- * will be used to translate the string and also to mark it safe. Only strings
- * written in the YAML files should be marked as safe, strings coming from
- * dynamic plugin definitions potentially containing user input should not.
+ * the addTranslatableProperty() method can be used to mark it as such and also
+ * to add translation context. Then
+ * \Drupal\Core\StringTranslation\TranslationWrapper will be used to translate
+ * the string and also to mark it safe. Only strings written in the YAML files
+ * should be marked as safe, strings coming from dynamic plugin definitions
+ * potentially containing user input should not.
  */
 class YamlDiscovery implements DiscoveryInterface {
 
@@ -36,7 +37,7 @@ class YamlDiscovery implements DiscoveryInterface {
   /**
    * Contains an array of translatable properties passed along to t().
    *
-   * @see \Drupal\Core\Plugin\Discovery\YamlDiscovery::__construct
+   * @see \Drupal\Core\Plugin\Discovery\YamlDiscovery::addTranslatableProperty()
    *
    * @var array
    */

Minor fixes on commit.

  • alexpott committed 4b12bbf on 8.0.x
    Issue #2338081 by pwolanin, dawehner, effulgentsia, kgoel, vijaycs85,...
Berdir’s picture

The removed StringTranslationTrait actually broke a contrib module, I assume that was done to force code to think about using t(). That's fine, but the change record could mention this a bit more explicitly.

Gábor Hojtsy’s picture

Issue tags: -sprint

@Berdir: updated and published https://www.drupal.org/node/2539156 Yaml plugin discovery now returns TranslationWrappers objects just like annotations and added and published https://www.drupal.org/node/2540342 LocalActionDefault, LocalTaskDefault and ContextualLinkDefault class capability changes. Hope these help.

Gábor Hojtsy’s picture

Oh, also added and published LocalActionDefault, LocalTaskDefault and ContextualLinkDefault plugins do not support title_arguments anymore https://www.drupal.org/node/2540360

pwolanin’s picture

Issue summary: View changes
JvE’s picture

Bugcrowd asked me to comment here.

Status: Fixed » Closed (fixed)

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

The last submitted patch, 138: 2338081-138.patch, failed testing.