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:
- This makes derivatives behave the same way across plugin types
- This ensures that just static titles are marked as safe
- 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
Comment | File | Size | Author |
---|---|---|---|
#148 | 2338081_148.patch | 50.01 KB | chx |
#148 | interdiff.txt | 1.19 KB | chx |
#147 | interdiff.txt | 1.37 KB | dawehner |
#147 | 2338081-147.patch | 50.42 KB | dawehner |
#143 | interdiff.txt | 1.02 KB | dawehner |
Comments
Comment #1
chx CreditAttribution: chx commentedComment #2
chx CreditAttribution: chx commentedComment #3
xjmComment #4
effulgentsia CreditAttribution: effulgentsia commentedDoesn't the documentation of bootstrap.inc's t() cover this pretty clearly:
Is the suggestion in this issue to copy/move that to the other t() wrappers that we have or to change that text?
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?
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.
Comment #5
effulgentsia CreditAttribution: effulgentsia commentedComment #7
effulgentsia CreditAttribution: effulgentsia commentedAlso, do we still want to translate
$theme->info['name']
? i.e., should that bearray('@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.Comment #8
mgiffordUnassigning so someone else can take it on.
Comment #9
effulgentsia CreditAttribution: effulgentsia at Acquia commentedLet's start with an updated bot check. This also includes the fix to template_preprocess_menu_local_task(), per the issue title.
Comment #10
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOops. Should have uploaded this one first, to see if only changing template_preprocess_menu_local_task() results in any failures.
Comment #12
mpdonadioSetting back to Needs Review per the out of order patch upload; #9 came back green.
Comment #13
joelpittetNeeds the translation question answered still in #7.
Adding parent meta for safemakrup::set removal.
Comment #14
pwolanin CreditAttribution: pwolanin commentedSo - 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?
Comment #15
kgoel CreditAttribution: kgoel at Forum One commentedworking on this
Comment #16
kgoel CreditAttribution: kgoel at Forum One commentedpwolanin 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.
Comment #17
pwolanin CreditAttribution: pwolanin commentedComment #19
pwolanin CreditAttribution: pwolanin at Acquia commentedTest fix, new test, plus fix to function getLocalizedTitle()
Comment #20
effulgentsia CreditAttribution: effulgentsia at Acquia commentedMerging #10 to see if this fixes the original bug here.
Comment #21
YesCT CreditAttribution: YesCT commentedThese 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
this comment makes sense, for the diff. But if we want to document that, we should really do it in the method @return docs.
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 ..."
nit, one line over 80 chars.
newline before the class closing }
and a newline after. :)
assign class to class
MODULE?
change so required.
unapproved comment is using t() on "unapproved comments @count" which is not user entered title.
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.
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.
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().
I'm pretty sure we inheritdoc setUp()
https://www.drupal.org/node/325974
extra ending newline
Comment #22
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think #19/#20 is on the right track with respect to the key goals:
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?
Comment #23
kgoel CreditAttribution: kgoel at Forum One commentedMostly fixed nitpicks in the patch. I haven't addressed #21 - 5, and 7 and #22 in this patch.
Comment #24
kgoel CreditAttribution: kgoel at Forum One commentedAddressed #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
Comment #25
Gábor HojtsyRe @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.
Comment #26
Gábor HojtsyComment #27
davidhernandezComment #28
pwolanin CreditAttribution: pwolanin at Acquia commented@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?
Comment #29
effulgentsia CreditAttribution: effulgentsia at Acquia commentedSorry for the lack of clarity in #22. Here's a POC of what I mean.
Comment #31
pwolanin CreditAttribution: pwolanin at Acquia commentedDiscussed 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.
Comment #32
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWhile working on #29, I noticed: #2509180: [meta] Various plugin definition alter() hooks incorrectly document to use t() .
Comment #33
pwolanin CreditAttribution: pwolanin at Acquia commentedThe 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.
Comment #34
Fabianx CreditAttribution: Fabianx for Drupal Association commentedWould 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!
Comment #35
effulgentsia CreditAttribution: effulgentsia at Acquia commented#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.Comment #38
pwolanin CreditAttribution: pwolanin at Acquia commentedRe-roll for conflict due to #2422815: Don't initialize the discovery object in plugin managers, unless needed
Comment #39
pwolanin CreditAttribution: pwolanin as a volunteer commentedapply the same changes to actions and contextual links, let's see which tests fail.
Comment #41
pwolanin CreditAttribution: pwolanin as a volunteer commentedUnit test fixups.
Comment #42
pwolanin CreditAttribution: pwolanin as a volunteer commentedRemoving the translation trait from the default plugins to discourage calling $this->t() on user input.
Comment #44
pwolanin CreditAttribution: pwolanin as a volunteer commentedtest fix
Comment #46
pwolanin CreditAttribution: pwolanin as a volunteer commentedfix some more test classes.
Comment #47
Fabianx CreditAttribution: Fabianx for Drupal Association commentedIs that 'use' needed here?
And if yes, why not for ContextualLinkDefault.
Methods look similar.
Lets add the comment here as well.
Hurray!
Is this still needed here?
Those "seem" to be unused?
Comment #48
Gábor HojtsyComment #49
pwolanin CreditAttribution: pwolanin as a volunteer commented@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.
Comment #50
pwolanin CreditAttribution: pwolanin as a volunteer commentedper 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.
Comment #52
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC, thanks for addressing my feedback.
Comment #53
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedBumping to critical since there are unresolved D8 security holes this fixes.
Comment #54
alexpottThis needs a comment ... as this class does not need this.
Why is this necessary?
The class docblock needs updating for the new functionality.
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
ortitle_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.Unused and untested.
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 callgetArguments
if it needs to merge. Basically this method looks way to helpful to me.Comment #55
alexpottAlso if this is fixing security holes we should have tests showing that user generated content is no longer marked safe due to this.
Comment #56
dawehnerStill working on an integration test coverage ...
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.
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.
And broken ...
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.
Comment #57
dawehnerAdded the first integration test coverage for local tasks ... you should really better start with an integration
Comment #58
larowlanCouple of questions/minor nits
'An array of properties whose values should be treated' - should that be 'translated'? Either way I'm not sure what that means.
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 calllist()
UnapprovedComments should always use a TranslationWrapper right? otherwise the @count will remain - does that mean we can forego the instanceof?
nice!
Comment #59
Gábor HojtsyI think the sprint tag removal was a mistake?
Comment #60
dawehnerInteresting I don't even see the comment which removes the tag.
Comment #61
mbrett5062 CreditAttribution: mbrett5062 commentedSee #53, when this was bumped to critical :)
Comment #62
dawehner@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.
Comment #63
lauriiiI 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 :)
Comment #64
josephdpurcell CreditAttribution: josephdpurcell commentedTwo 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.
Comment #65
tim.plunkettIt only violates the interface if it is required; since it is optional this is legal. However, you're right that it is unused.
Comment #66
josephdpurcell CreditAttribution: josephdpurcell commentedI 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.
Comment #67
vijaycs85as per #62, just marking all title as SafeMarkup::escape.
Comment #69
vijaycs85Adding tests from #57 and using htmlspecialchars() instead of SafeMarkup::escape().
Comment #71
mpdonadio@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
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.
Comment #72
mpdonadioBTW, 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..
Comment #73
vijaycs85#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.
Comment #74
dawehnerNo, everything in t() will be marked directly as safe.
Comment #75
dawehnerHere is also a test for local actions.
Comment #76
mpdonadio#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
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:
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.
Comment #77
vijaycs85thanks @mpdonadio.
1. removed else case in menu.inc
2. updated test with assertEscaped
3. Added tests from #75
Comment #78
dawehner@vijaycs85
Do you mind sharing the files :p
Comment #79
vijaycs85:)
Comment #81
mpdonadioShould 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.
Comment #82
dawehnerYeah I agree we should add a comment, why we do that.
Ah well, we could just go with assertRaw I guess?
Comment #83
mpdonadioI 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.
Comment #84
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedThe 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.
Comment #85
dawehnerFirst, let's fix the title.
Comment #86
dawehnerSorry @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.
Comment #87
mpdonadio@dawehner @pwolanin, I have the IRC conversation saved and will double check the IS w/ what you guys hashed out.
Comment #88
dawehnerThis adds the local action test for now. We still should add derivative based tests and one for contextual links.
Interdiff is relative to #57
Comment #90
alexpottYep 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.
Comment #91
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedI'm going to work on this to change a class as discussed with dawhener in IRC and fix the 1 test fail.
Comment #92
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWhat's the concern? We have it in
Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery
(via getAnnotationReader()), so what's concerning about adding it toDrupal\Core\Plugin\Discovery\YamlDiscovery
?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.
Comment #93
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedHere's with just the text fix - it's not as pretty, but I should verify the escaping in the raw HTML.
Comment #94
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedI 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
Comment #95
mpdonadioJust 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.
Comment #96
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedAdd some test cases and restore some testing of hook_menu_local_tasks() which seems to have been totally lost.
Comment #97
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedoops - 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.
Comment #99
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedoops - duplicated a couple route paths. This should fix it.
Comment #100
Gábor Hojtsyre
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.
Comment #101
Gábor HojtsyReviewed the patch. Makes sense, looks good. Mostly just docs related issues found:
values should be treated [as translatable]?
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?
Sound like this is a cloneWithMergedArguments() or something like that? Maybe withArguments() is as clear to others, just noting if not :)
Woo, fancy!
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.
Not actually used?
Comment #102
alexpottI'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:
@Gábor Hojtsy what do you think?
Comment #103
dawehnerI 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.
Right :)
Comment #104
Gábor Hojtsy@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).
Comment #105
brandon.holtsclaw CreditAttribution: brandon.holtsclaw as a volunteer commentedI'm at the GovCon sprint taking a look into the testcases for this with @YesCT
Comment #106
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented@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
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
Comment #108
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedsilly little fix, I think.
Comment #109
brandon.holtsclaw CreditAttribution: brandon.holtsclaw as a volunteer commentedAdding test coverage for title escaping for ContextualLink
Comment #110
dawehnerNice @brandon, I'm about to upload for local task and local action.
Comment #111
dawehnerPerfect teamwork!
Comment #113
dawehnerLet's fix them.
Comment #114
mpdonadioSlight 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.
Comment #115
alexpottBut 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?
Comment #116
dawehner@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.
Comment #117
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented@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.
Comment #118
chx CreditAttribution: chx commentedThat'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?Comment #119
Gábor Hojtsy@alexpott:
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?
Comment #120
alexpottI 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:
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.
Comment #121
alexpottCreated #2538514: Remove argument support from Translation Annotation
[Edit: heddn was right! thanks :)]
Comment #122
heddnI think #121 means to say: #2538514: Remove argument support from Translation Annotation
Comment #123
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedok, 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.
Comment #124
alexpott$translatable_properties is an ArrayPI. How about we just move it into a method you call after construction. Something like this:
(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)Comment #125
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedOk, adjusting it as suggested by alexpott.
Comment #126
dawehner+1 for that change!
Comment #127
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedLooks 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?
Comment #128
alexpottDo we have to do the string cast here? We could just say we return a string or SafeStringInterface
Let's return $this so the it's fluent.
Not needed.
Comment #130
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented@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.
Comment #131
dawehnerNow that we have a fluent interface again, we could online it again
Comment #132
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented@dawehner - I don't think it would really make it clearer to make it more compact
Comment #133
dawehnerFair. Here are just two minor nitpicks.
I think this is ready now. We have the test coverage now.
Comment #134
catchOn 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?
Comment #135
alexpottHere are the steps to reproduce the XSS attack.
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.
Comment #136
catchOverall 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.
nit:
'was written in the YAML file and did not come from'.
nit: translateable.
nits: corresponding, translatable.
That's... unfortunate - so it's decoding?
Comment #137
dawehnerYeah, a bit annoying.
Comment #138
dawehnerSadly this behaviour cannot be turned off, see https://bugs.php.net/bug.php?id=49437.
Comment #139
Gábor HojtsyFirst sentence not true anymore.
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?
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.
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:
Also we need a change notice for the API changes made.
Comment #140
dawehnerI think this is unnecessary abstraction and makes it actually harder to follow.
Comment #141
dawehnerI think the feedback from @catch got addressed.
Working on the change record now.
Comment #142
dawehnerThere it is.
Comment #143
dawehnerFixed the last point of @Gábor Hojtsy's review
Comment #144
cilefen CreditAttribution: cilefen commented#2539246: Search page local task label was an XSS vector—add tests
Comment #145
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedThanks @cilefen
Comment #146
Gábor HojtsyIndentation wrong, "...specify add..." wrong too.
of the YAML values of the YAML values
Comment #147
dawehnerYAML YAML, everybody YAML!
Comment #148
chx CreditAttribution: chx commentedI've rewritten the class doxygen. No code changes.
Comment #149
alexpottCommitted 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.
Minor fixes on commit.
Comment #151
BerdirThe 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.
Comment #152
Gábor Hojtsy@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.
Comment #153
Gábor HojtsyOh, also added and published LocalActionDefault, LocalTaskDefault and ContextualLinkDefault plugins do not support title_arguments anymore https://www.drupal.org/node/2540360
Comment #154
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedComment #155
JvE CreditAttribution: JvE at One Shoe commentedBugcrowd asked me to comment here.