Problem/Motivation
Follow-up to #1825952: Turn on twig autoescape by default
Rather than benefiting from Drupal 8's autoescaping, LinkGenerator::generate()
(and by extension, Drupal::l()
and LinkGeneratorTrait::l()
) still use Drupal 7's legacy approach of requiring the caller to pass an 'html' => TRUE
option in order to not escape the link text. This is not ideal, because:
- To prevent double-escaping of plain text, the caller must know if the variable has already been escaped.
- The caller can set that option, but pass in a variable containing user-submitted text that hasn't been filtered, resulting in a XSS vulnerability.
Both of the above are especially fragile in cases where the code that sets the option is separate from the code that sets the text, such as in tablesort_header()
or (Views)DisplayPluginBase::optionLink()
.
Proposed resolution
Remove the html
option and change LinkGenerator::generate()
from conditionally escaping based on that option to autoescaping (in other words, conditionally escaping based on whether SafeMarkup knows the string to already be safe).
In most places, this results in the caller simply being able to remove setting that option. Examples:
- "Read more" link in
NodeViewBuilder::buildLinks()
: The text includes a SPAN tag for accessibility, but is produced via t(), so is already marked safe. Actions::preRenderActionsDropbutton()
: The text is the result of a drupal_render() so is already marked safe.
However, in some cases, there is code that passes a variable that is safe, but SafeMarkup doesn't know that. There are several categories of such situations, and here is what is proposed for each of them:
Combining two variables
Use String::format()
to combine them in a way that ensures the result is safe and registered as such. Example:
- Within tablesort_header(), change
\Drupal::l($cell_content . $image, ...
to
\Drupal::l(String::format('@cell_content@image', array('@cell_content' => $cell_content, '@image' => $image)), ...
Combining literal HTML with a variable
Use String::format()
to combine them in a way that registers the result as safe. Example:
- Within template_preprocess_views_ui_rearrange_filter_form(), change
'#title' => '<span>' . t('Remove') . '</span>',
to
'#title' => String::format('<span>@text</span>', array('@text' => t('Remove'))),
A literal containing HTML without any text
There are no examples of this in core. An example might be an image, but core always renders images with a render array rather than a literal <img>
tag. If there were an example of this, it could be implemented with SafeMarkup::set(SOME_LITERAL_STRING)
, but #2297703: [meta] Refactor and remove as many SafeMarkup::set() calls as possible claims SafeMarkup::set() is for internal use only, so if we accept that claim, then it could alternatively be implemented with String::format(SOME_LITERAL_STRING)
, though it seems odd to call String::format() without the 2nd argument. Let's discuss this further in that issue.
A literal containing HTML and text
There should be no example of this in non-test code, because all literal text should pass through t(), at which point it's no longer a literal and is covered by the "literal HTML with a variable" example above. Within test code, however, this can occur, such as within Drupal\dblog\Tests\Views\ViewsIntegrationTest::testIntegration()
, but within test code, it's okay to use SafeMarkup::set()
despite its warning of being for internal use only.
Remaining tasks
Review. Commit.
User interface changes
n/a
API changes
See "Proposed resolution".
Beta phase evaluation
Issue priority | Major because this security hardens one of Drupal's most commonly called functions. Not critical because this is merely security hardening, not fixing an actual vulnerability, which only occurs if the caller makes a mistake. Also, the lack of this hardening didn't stop any prior version of Drupal from being released. |
---|---|
Prioritized changes | The main goal of this issue is security improvement. |
Disruption | Disruptive for contributed and custom modules, because there are some cases in which it leads to escaping strings that the module author does not want escaped, and therefore requires the module author to change the code that constructs that string (see "Proposed resolution" section for details). However, this is the same condition that already exists in other places, such as setting the value of a 'data' element of a table cell, or any variable passed to a Twig template, so this is just applying the same requirement to link text. |
Comment | File | Size | Author |
---|---|---|---|
#227 | interdiff.txt | 2.57 KB | effulgentsia |
#227 | remove_html_true-2273923-227.patch | 49.56 KB | effulgentsia |
#226 | remove_html_true-2273923-226.patch | 46.97 KB | effulgentsia |
#219 | interdiff.txt | 3.06 KB | effulgentsia |
#219 | remove_html_true-2273923-219.patch | 46.96 KB | effulgentsia |
Comments
Comment #1
chx CreditAttribution: chx commentedComment #2
joelpittetComment #3
joelpittetComment #4
chx CreditAttribution: chx commentedComment #5
chx CreditAttribution: chx commentedComment #6
xjmJust a reminder that only core maintainers (or someone commenting on their behalf) should add the beta blocker tag. :) In this case, I'd agree that this change, if we need to make it, is absolutely impactful enough to be a beta blocker. Let's get catch's feedback on this issue.
Comment #7
chx CreditAttribution: chx commentedI think Joel added the beta blocker because this is a split off from a proper beta blocker only separate to make the parent easier to review.
Comment #8
joelpittetI am a core maintainer:P(or at least I thought I was...), but that's besides the point.
@chx is right I cloned this off of an existing beta blocker so I preserved that issue tags as it is a child of that issue and prevents that issue from balooning with security fixes that would make the patch harder to review the overall major changes. (The bigger the patch the harder it is to find reviewers, ime)
There could be a plethora of reasons this might bikeshed the other issue, and maybe people decide for it or against it. So we'll let the community decide on it's own merits and also makes this change with it's ~50 odd #html=>true properties to change to wrapping in SafeMarkup easier to review on it's own.
I'm also fine with removing the beta blocker tag, but I think my logic for keeping it is fairly sound?
Comment #9
xjmThere are four Drupal 8 core maintainers: Dries, webchick, alexpott, and catch. You and me and chx are core component maintainers, but it's a different responsibility. ;) The point though is that the scope of the beta release is defined by the four core maintainers. (Apologies as this is completely off-topic; just documenting it here in case anyone else reads and is confused. More details at http://buytaert.net/the-next-step-for-drupal-8-is-a-beta.)
I agree that it's better to do this as a separate issue, absolutely.
Comment #10
joelpittetOh I've been calling that group d8 'core committers', I thought everybody in the MAINTAINERS.txt was a 'core maintainer'... my bad.
Comment #11
joelpittetLet's see if catch can provide some feedback:)
Comment #12
nevets CreditAttribution: nevets commentedIf you do this, what will be the proper way to created an image link?
Comment #13
joelpittet@nevets great question.
If you pass it in as a renderable array it would just still work because drupal_render would be returning a SafeMarkup object. If you pass it in as a string you'd need to just wrap your markup in a SafeMarkup object.
becomes
Adding to issue summary.
Comment #14
xjm#13 looks like really nice DX. So what would happen if I did
l('<img src="foo">', $path)
without the safe markup? Would it be autoescaped?Comment #15
tim.plunkettClarified that it's a bit more than just it appears, you have to write the use statement as well, instead of just 'html' => TRUE...
Comment #16
joelpittet@xjm Yes it would be, which would be the same as now without the html flag.
@tim.plunkett thanks I forgot that. Would be nice if you didn't have to add use statement on some things, but that's probably asking too much...
Comment #17
chx CreditAttribution: chx commentedComment #18
xjmComment #19
xjmLooking at the non-test uses first:
Here's what's using it:
(This one will go away when the Views conversion lands.)
<em>
tags from thet()
%
placeholder type.<span>
<span>
.<span>
s and a couple other cases that aren't obvious to me.Comment #20
xjmTagging because pretty much all the weird stuff is in Views.
This actually needs to be postponed on #1825952: Turn on twig autoescape by default, right?
And we'd have a patch (or patches) with a lot like the attached?
Comment #21
joelpittet@xjm thanks for the summary, re #20, yes BUT not for the example you have in there because it has a renderable array being passed in, only strings containing markup would need this, renderable arrays are default safe. So I guess drupal_render() or array('#theme' => 'image', ...) or whatever render array you can think of. Great to check all
l('...
andl($...
while grepping though because just because it's a variable doesn't mean it's not an unsafe string.I think yes postponed and we'll see what @catch says as well and welcome any concerns/comments from anybody(especially security team peeps).
Comment #22
xjmAh! I get it now. Thanks @joelpittet. So e.g. looking at form_pre_render_actions_dropbutton(). It is returning a render array, but also renders the button to a string early (with an @todo):
So would that one would need the SafeMarkup to avoid that button string being re-escaped later, if the early rendering isn't fixed first? (Also: the @todo seems out of date as it references theme_item_list().)
Comment #23
joelpittetThat's a red herring it looks like, I wonder why that @todo doesn't have an issue tied to it because I haven't seen that bit yet...
Anyways, drupal_render() is currently (in the autoescape on by default sandbox/patches) returning a SafeMarkup object, which has a __toString() method for when it's printed and extends Twig_Markup from twig which Twig will avoid escaping. So for that one:
All great questions, you've pretty much helped write the change record:) And that reminds me we'll need to check
template_preprocess_links()
becomes
Comment #24
xjmTagging for security feedback for the proposal (though it might depend on the final implementation in #1825952: Turn on twig autoescape by default and/or the actual patch).
Comment #25
xjmFor real.
Comment #26
chx CreditAttribution: chx commentedI have reviewed it :) very seriously: who do you want to review it in light of all that transpired these last couple months?
Comment #27
effulgentsia CreditAttribution: effulgentsia commentedPrefixing title to indicate how many issues this is postponed on, for beta blocker management.
Comment #28
catchI think this is OK as a release blocker.
Not convinced it's a beta blocker - the worst that happens if this gets in post-beta and your module uses html => TRUE is you have to update to avoid double-escaping no? And html => TRUE will be redundant. That seems OK to change past beta to me.
Comment #29
xjmThanks @catch!
Comment #30
xjmComment #31
catchComment #32
xjmI strongly suggest we do not change
checkPlain()
like that. With the current implementation in #1825952: Turn on twig autoescape by default, this would mean that once someone marks a string as safe withSafeMarkup::set()
,checkPlain()
can never escape it. That's a really bad idea and would make the concerns I've raised from #219 on in that issue even worse.Comment #33
xjmComment #34
tim.plunkettComment #35
dawehnerNote #2314599: Use title/url instead of l() for building breadcrumb partly solved the html => TRUE bit.
Comment #36
pfrenssenThis seems like a nice cleanup, going to make a start on implementing this.
The approach suggested in the summary is outdated. The approach that finally was taken in #1825952: Turn on twig autoescape by default allows to manually mark strings as safe using
SafeMarkup::set()
so it does not require a SafeMarkup object being passed around. That makes this issue a bit simpler, and there are no changes needed incheckPlain()
.Comment #37
RainbowArrayMy understanding is that SafeMarkup::set() is something we want to avoid if at all possible.
https://www.drupal.org/node/2311123 documents better alternatives.
Entirely possible I am not fully understanding this issue, but SafeMarkup::set() triggers my "I have heard that is problematic" sense.
Comment #38
chx CreditAttribution: chx commentedSafeMarkup::set internally is not too bad and yeah the IS is outdated; you can check whether the incoming string is marked as safe easily with SafeMarkup::isSafe.
Comment #39
pfrenssenHere's an initial pass on this.
Regarding SafeMarkup::set(), using this feels very natural so I'm afraid this will proliferate in contrib as well. What would be better?
Comment #41
pfrenssenPicking this back up, I'll start by updating the summary, then will see to the failures and finally work on the ideas that @chx came up with on IRC last night.
Comment #42
pfrenssenComment #43
pfrenssenComment #44
pfrenssenWhile working on this I just found that inline templates are not actually escaping the variables. Reported it in #2330503: [sechole] Inline templates pass through unsafe strings. This issue is blocked on that, I was writing a test using an inline template and this causes it to fail.
Comment #45
pfrenssenWoops I seem to have accidentally deleted the patch. Here it is again. This is the same patch that was in #3.edit: wrong browser tab, this was intended for #2330503: [sechole] Inline templates pass through unsafe strings.Comment #46
pfrenssenDid some more work on this.
Didn't address the test failures yet since this is now failing as well due to #2330503: [sechole] Inline templates pass through unsafe strings.
Comment #47
star-szrUn-postponing because #2330503: [sechole] Inline templates pass through unsafe strings got in. Thanks a ton @pfrenssen!
Comment #48
pfrenssenYou're welcome :)
Changing status to let the bot have a go at this so we have a list of failures to focus on.
Comment #50
mpdonadioRe-rolling this as part of core mentoring.
Comment #51
mpdonadioReroll; resolved conflicts manually. Please double check the change to theme_views_ui_rearrange_filter_form(), #46 had a change to a line that looks like is now gone.
Comment #53
dawehnerProbably caused by the LinkGeneratorTest
Comment #54
pfrenssenYeah this call to
drupal_render()
should be wrapped in adrupalRender()
method and mocked in the test.Unassigning myself, won't have time to work on this in the coming 2 weeks.
Comment #55
mpdonadioI can tackle this.
Comment #56
mpdonadioOK, changes since the reroll.
Advice needed on the proper way to handle the mocked method in the setup. I couldn't quite explain myself over IRC to get help to do this properly.
Comment #58
mpdonadioDid fetch instead of a pull when I made #56...
Comment #60
tim.plunkettTHe comment should be wrapped to two lines. Also do we have code coverage for this change?
This line is being added. Do we have code coverage for it? Or a comment for why it is needed now and wasn't before?
Comment #61
mpdonadio1. Was added by me because \Drupal\Tests\Core\Utility\LinkGeneratorTest::testGenerateWithHtml() was failing after the change (HTML in the title was being escaped instead of passing through). Wrong thing to do here?
2. That is my bad. Was looking at the AJAX controller as an example. I'll remove it, and also add a @todo to reference #2182149: Move drupalRender() and drupalRenderCollectCacheTags() out of HtmlControllerBase
Will look into the other test fails.
Comment #62
mpdonadioOK, I am at a loss why these are failing.
Drupal\block\Tests\BlockTest::testThemeName() has a fail. I think the problem has to do with template_preprocess_menu_local_task(), but I don't see what is going wrong here. Mirroring what was done in seven_preprocess_menu_local_tasks () caused weird fails; I will investigate this again. The second fail was fixed by changing a ! to a @ placeholder in the string (I'll post the patch with this, and @tim.plunkett 's requests later).
Drupal\system\Tests\Entity\EntityOperationsTest::testEntityOperationAlter() has a fail that shows up twice because of the look. I don't see what is going wrong here.
Drupal\views\Tests\Plugin\DisplayTest::testDisplayPlugin() has a fail on `$this->assertRaw($this->randomString);`. No clue about this.
Advice needed on how to proceed.
Comment #63
tim.plunkettIn the trim() call, $text is assumed to be a string.
And it is in HEAD. But you're turning it to an array here.
You can probably move the trim call to the top of the function.
Same thing here. You're passing $active to t(), not to l(), so it cannot be an array. Not sure why this change is needed at all, actually.
Comment #64
mpdonadioOK, make progress on this. Block tests pass. Have not started on the other fails.
I removed the changes to seven_preprocess_menu_local_tasks(), and folded them into template_preprocess_menu_local_tasks. One thing that may be objectionable is that the translation of the active tab is now split into two t() calls (which is arguably better).
Comments from #60 should be addressed, too.
Will work on remaining fails.
Comment #66
mpdonadioI started to look at `Drupal\system\Tests\Entity\EntityOperationsTest::testEntityOperationAlter()`, and I think there is a problem with the test. Since the role name is random, you need to run it a few times to get it to one with an escapable character. In HEAD and alpha-14, the HTML test page will contain double escaped text, and the test passes. See alpha14-output.png and alpha14-src.png, which show the double output. When this patch is applied, everything is escaped properly. See remove_html_true-2273923-output.png and remove_html_true-2273923-src.png.
Can someone confirm this before I change the test to
Comment #67
mpdonadioFixed inline template call / order in Drupal\views\Tests\Plugin\DisplayTest::testDisplayPlugin(). Did not include proposed change to test mentioned in #66.
Comment #69
mpdonadioRegarding `Drupal\system\Tests\Entity\EntityOperationsTest::testEntityOperationAlter()`, after a long conversation with @chx on IRC, it looks assertLink in HEAD is bogus.
In HEAD, l() is escaping the entire title, and the role name is ending up being escaped twice (see screenshots from the test output). ->assertLink does an xpath, which is returning escaped text. Therefore, the two cancel each other out, and the end result is a false pass. This example demonstrates what is going on:
The interdiff shows the solution. The $label gets decoded, and then the xpath will reencode it, so all is well.
Comment #70
chx CreditAttribution: chx commentedBut let me add that
entity_test_entity_operation_alter
is also bogus because it misses an'options' => ['html' => true]
in the link definition. That's why the test actually passes. I will be surprised a little if #69 doesn't reveal more ... interesting ... tests but we will see.Comment #72
mpdonadioFixed Drupal\views\Tests\Plugin::testDisplayPlugin()
Comment #73
dawehnerON top of that someone should take into account http://paste.ubuntu.com/8338214/ as otherwise we will have double escaping, wont' we?
should this bit talk about inline_template?
Note: link['title'] is coming from MenuLocalTaskDefault::getTitle() which is already translated. The point of the t() used to be able to override the way how the title and the active class is connected. This t() call will introduce a lot of additional locale entries
Did you considered to use t in the template directly?
This change seems to be a little bit out of scope, I don't care but just avoid those, if possible.
Comment #74
dawehner.
Comment #75
mpdonadioEverything from @dawehner's comments in #72 should be addressed.
Comment #76
mpdonadioReroll of #75 so it applies cleanly to HEAD.
Comment #79
mpdonadio@pwolanin, do you need a re-roll of this? Was going to wait for all of the commits from DrupalConAms to get in before I did.
Comment #80
mpdonadioTalked about this in core-mentoting, will reroll. Hoping to have done in today or tomorrow.
Comment #81
mpdonadioReroll against dbea832. Throwing at the testbot to see what barfs before running specific tests locally.
Comment #83
mpdonadioFixed Drupal\block\Tests\BlockTest and Drupal\Tests\Core\Utility\LinkGeneratorTest. Want to see what TestBot says about the other two problems.
Will followup pending on what happens, because the change to block.module may need discussion and I need time to summarize it. The change to Drupal\Tests\Core\Utility\LinkGeneratorTest was related to the reroll for the Url changes.
Comment #84
mpdonadioOK, all good. Other two problems must have been from the earlier oopsie to core.
The patch includes this:
I have no idea what is going on here, or what the proper thing to do is. Before the Url change, that function used l() with ! placeholders w/o problem. Using \Drupal::l() with ! placeholders makes Fixed Drupal\block\Tests\BlockTest fail, and also makes the theme name an XSS liability. Part of the patch is to check escaped text in a few places with
to ensure that it is actually escaped (shameless plug for #2340785: Create proper test method for determining if text has been escaped properly). So, changing the output in this case to use escaped placeholders rather than the fudge the test seemed the proper thing to do here, but I am open to suggestions.
Comment #85
pfrenssenCan you post an interdiff? That's very helpful to see what has changed ;)
Comment #86
pfrenssen"use an a render array" has too much "an". There are also two spaces following the period.
l() has been renamed to _l().
This has a syntax error: the #type declaration misses its opening quote.
l() has been renamed to _l().
Maybe now that these lines are touched, replace the deprecated call to _l()?
This is probably not correct. The text in $variables['text'] could have been altered and be different from the original version that is stored in $text.
Also, is it really the best solution to use SafeMarkup::set() here? Using that method is discouraged. If this is a good solution, then add some documentation explaining why it is acceptable to use SafeMarkup::set() here.
You can just
return drupal_render(...);
.Now that this code is touched, maybe replace the deprecated call _l() with LinkGenerator::generate().
Now that this is touched, maybe replace _l().
It is not common to refer to methods in the current scope as
->xpath
. More common would be::xpath()
or simplyxpath()
or$this->xpath()
.Use
$this->t()
instead oft()
.Let's stick to
$this->t()
instead of using the venerablet()
.Maybe replace _l() now that this is touched.
Maybe replace _l().
Maybe replace _l().
Comment #87
mpdonadioThanks. Most of these are easy, assuming the conflicts from further rebasing are minimized when I make the patch. I should have something ready tonight (EDT).
@pfrenssen, regarding item 6 in #86, that was added to address the test for LinkGenerator. IIRC, the issue was the test with the inline template was failing with double escaped output (or barfing outright). Since we are explicitly rendering the text near the top of LinkGenerator::generate() now to account for inline Twig templates, and therefore should be a safe string already per the final lines of drupal_render(), perhaps the safest thing to do is:
which take into account the text being altered to an unsafe string.
Comment #88
pfrenssenYeah most are just minor cleanups, this is starting to look pretty good.
Your proposal to check if the string is safe before escaping looks OK too!
Comment #89
mpdonadioOK, throwing this as the TestBot. Tests work locally for me.
Done from #86 above:
1,2,3,4,7,10,11,12 were all easy changes.
5 looks like it was part of HEAD already when I did a pull+rebase.
8,9 are done and use the injected link generator.
13 uses \Drupal::l()
Not done from #86 above:
My idea for 6 didn't work. I need to spend some time with the debugger to figure out what is going wrong here. My guess is that the string is being set as unsafe again as a result of the alter. That will be a tricky situation if it is the case.
14 and 15 use
javascript:void
as the URL, which I don't think can be handled yet with LinkGenerator?Comment #90
mpdonadioAddressed item 6 from #86 above. Looks like the root problem if my idea was insufficient mocking of what drupal_render() would do. Tests work locally.
Comment #91
pfrenssenThis looks like the image will not change direction when the table sort order is flipped.
$ts['sort']
is changed after the image has been defined.Removing the query arguments doesn't seem right either.
Seeing that the tests pass with these changes in place it seems we are missing test coverage on the tablesort functionality.
Comment #92
pfrenssenRestored the tablesort functionality.
Comment #93
mpdonadioYeah, that does look better now. I suspect this was the result of a bad conflict merge on my part.
Comment #94
dawehnerGreat work!
Calling _l() is is certainly not the recommended way ...
Maybe I am dump but is there a reason we cannot at least move the text itself into the main twig file? This would allow much better speed as well as better adaptability.
On the other hand this is maybe out of scope.
If you compare the two approaches we now pass along the title arguments as parameters, this can't work.
Isn't that exactly the same as
$text = SafeMarkup::escape($variables['text'])
?mh why is all the documentation removed here?
Dump question, do we need to call drupal_render() here or could we just do it implicit inside the template?
We also have a replacement for l() inside a template so should we use it there?
nitpick: let's use camelCase here for the linkGenerator .
Let's drop that line.
It would be great if you could check whether this not accidentally introduces some secholes. Just try out to insert some basic
alert(123);in there.
Comment #95
mpdonadioThanks Daniel. I'll post a patch later with the easy stuff, and then we can discuss the a few of these points (I think just 2 and 3, but maybe 6) and how we should proceed.
Edit, It take the timeframe back. There are a lot of conflicts from the rebase, which is where some of these errors keep creeping back in. Will post patch when I can.
Comment #96
mpdonadioAddressed from #94
4, 5, 6, 8, 9, 10 (via visual verification with an XSS vector)
Not addressed from #94
1. This is part of the documentation for _l(), so the usage shows how to use _l(), even though it is deprecated. The proper replacements are documented further down.
2,7. Awaiting feedback on whether we want to do this in scope for this patch. 2 would also ripple into the core themes and potentially affect any Drupal 8 themes.
3. Defer to @pfrenssen on this
The attached interdiff is after I rebased, but was hand edited to remove a few self-comments I accidentally committed while doing the merges.
I think TestBot will like this.
Comment #98
mpdonadioOk, the fail in Drupal\block\Tests\BlockTest was from a bad conflict merge on my part. Yay tests catching it.
Pretty sure the fail in Drupal\views_ui\Tests\DisplayPathTest is from #2353347: Random failure in DisplayPathTest.
Two interdiffs attached, one against #92 and one showing the change to fix the merge oopsie.
Comment #99
cilefen CreditAttribution: cilefen commentedThis is a partial review of some things I noticed. Thank you for keeping this big issue rolling.
I am not sure why this is here. The issue mentioned is "closed, works as designed" #2182149: Move drupalRender() and drupalRenderCollectCacheTags() out of HtmlControllerBase
There is a comment typo here. While fixing it, remember the blank link after the last @param.
LinkGenerator is not injected, the container is. I would recommend changing this comment to "The link generator."
This comment is unnecessary.
Comment #100
mpdonadioI'll fixe these, and I suspect I will have another batch of conflicts to merge first.
@cilefen, your first point in #99 was added in the patch in #56 for test mocking purposes.
The comment was copied from \Drupal\Core\Ajax\AjaxResponseRenderer, which had to do something similar for testing purposes.
Comment #101
cilefen CreditAttribution: cilefen commentedI see. I guess the @todo should come out in that case.
Comment #102
mpdonadioNo merge conflicts when I rebased.
Addressed all comments in #99. In addition, I changed the member properties mentioned in 3 and 4 to use camel case, and to be protected instead of private. Also added the explicit declaration w/ type hint to ResponsiveImageFieldDisplayTest to mirror ImageFieldDisplayTest. Tests pass locally.
Comment #103
dawehner_l is still no recommended API, se earlier review :( If there is one place than it should be the LinkGeneratorInterface where this specific piece of documentation should exist.
Did you tried out whether this change maybe results in a double escaping problem?
Is there a reason we use once String::checkPlain and in the other case SafeMarkup::escape?
So yeah we wrap that for better unit test support.
This is still wrong ... just try it out, it will have no proper title attribute.
Comment #104
mpdonadioNew patch. To address points some of the points in #102.
1. Per some IRC advice, I fixed the documentation for _l(), restoring it back to better align with HEAD, minus the 'html' option. I took a pass at expanding the example code in the @deprecated section to show the LinkGenerator. Rewrites here appreciated.
2. The explicit plaining is a result of #2338081: Local Tasks, Actions, and Contextual links mark strings from derivatives (or alter hooks) as safe and translated. Added a @todo that references this. chx and I discovered this one night. Though my name is on that bug report, he wrote the issue summary with all of the details. I don't fully grasp the scope of that.
5. Got to the root of this. This is HEAD right now in core/includes/tablesort.inc:65Neither 'attributes' nor 'html' are valid in the $options parameter for the Url constructor, but there they are. This carried over into the patch. It is fixed in this patch.Comment #105
dawehner@mpdonadio
Thank you for the hard work on this quite important security improvement issue!
I don't want to sound pedantic but still, you are introducing a regression compared to something which worked before. Go to "admin/content/comment" and you will see that your
patch removes the title attributes, as expected.
Before
After
Comment #106
mpdonadio@dawehner, it's not about you being pedantic (which is the point of peer review), It is me being dense :) and perhaps an opportunity to add some test coverage for tablesort in the future.
Title is back: visually verified the browser tooltip and saw the attribute in a View Source on the admin/content page.
Comment #107
dawehner@mpdonadio
Thank you! Do you want to open a follow up to add explicit test coverage for the title?
Comment #108
mpdonadioI made #2357997: Add test coverage for tablesort header link titles and will make the patch.
Comment #109
cilefen CreditAttribution: cilefen commentedComment #110
cilefen CreditAttribution: cilefen commentedReroll
Comment #111
dawehnerMh, i don't get that ... you already have covered the exact same usecases above with
\Drupal::l()
?I am not convinced by this change ... doens't t() gets marked as safe, so that this introduces a security issue here?
This change is not needed here ...
Comment #113
mpdonadioWill address comments and look into the feed label thing. Also did confirm that #2324371: Fix common HTML escaped render #key values due to Twig autoescape didn't impact this.
Comment #114
mpdonadioPatch to address comments from #111
1. Per IRC conversation, moved the link_generator examples to LinkGeneratorInterface
2. This function was removed in #2138115: Split aggregator theme functions to a separate file. It is gone from patch now.
3. Reverted to match HEAD.
Comment #115
xjmComment #116
Fabianx CreditAttribution: Fabianx commentedSo that issue could still go in with a) leaving html => TRUE/FALSE as a deprecated option, removing all usage of that API from core (setting a good example) and using escape:: instead of checkPlain. No BC break, but still good example and no double escape for already escaped text.
The BUG here is that putting in already escaped text, gets double escaped.
This is something you might still expect from l(), but from the new link generator?
That bug and empowering core to be able to remove all html => TRUE cases from the code base (to set a good example => secure by default!) is what should really be fixed here.
catch seemed to have been okay with the BC break for the html => TRUE, but I am generally okay for this issue to go in with or without BC break as long as the usage of html => TRUE is very very much discouraged and deprecated.
On the criticalness itself, consider:
At least in D7 that code would have been a security hole:
We need to decide if we really want to allow / encourage that mistake in the future given we have so much more powerful tools and do everything to prevent people from shooting themselves in the foot now. Especially with the new link generator being a new and non-finalized API still anyway (I could understand resentments more if l() would still exist like in D7, but it does not.)
Comment #117
pfrenssenRerolled after #2364161: Replace nearly all existing _l calls got in. We will need to replace all remaining calls to
_l()
with\Drupal::l()
.Comment #119
pfrenssenI would still document the properties here, even though it is just a simple wrapper.
Document with @inheritdoc.
Now that we're touching this, let's fix this docblock to @inheritdoc.
Coding standards: split this array declaration to multiple lines.
Reviewed the top 75% of the patch.
Comment #120
mpdonadioWhat is the status of this patch actually getting in, now that Twig autoescape is committed?
I'm happy to fix all of this, though, and the failed tests (which based on the message should just be simple fixes).
Comment #121
Fabianx CreditAttribution: Fabianx commentedChances are high it will get in - but possibly with a not-recommended-you-are-on-your-own BC layer.
Comment #122
mpdonadioThis just address the fails / exceptions from remove_html_true-2273923-117.patch so there is hopefully something cleaner to review. The changes mainly brought some bits closer to HEAD.
Comment #123
catchI'm neutral on the bc layer. We can always defer removal of that to a follow-up if the usages are done here.
Comment #124
martin107 CreditAttribution: martin107 commentedMinor nitpick ...This needs to be removed in the next iteration.
Comment #125
mpdonadioA bunch of commits got made against HEAD and #122 no longer applies. I will reroll and address comments in #119 and #124, and any other reviews that come in before I am ready to post something new.
Comment #126
mpdonadioThis addresses the reroll, and #119 and #124. I should have done the reroll first, but noticed after I had done some things, and can't get a clean interdiff.
I found a bunch more places with `'html' => TRUE` that I will purge...
Comment #128
mpdonadioFixed failing tests from above; found a few more cases of `'html' => TRUE` that I removed, and tests pass locally for me.
I am going to put some debug exceptions in UrlGenerator to see if any other calls have been missed, and post that to the IGNORE issue.
Comment #131
mpdonadioSetting back to needs review based on the re-test passing. I think this was a spurious fail. I ran \Drupal\system\Tests\Entity\EntityOperationsTest about 30 times locally and they all passed.
Comment #132
pfrenssen@mpdonadio, thanks for your awesome work on this issue!
Comment #133
mpdonadioNo problem. I will post the next patch on Monday or Tuesday. Via check+test exception in LinkGenerator, I found one more case of setting HTML (It was a FASLSE, though, in a link formatter), but I want to wait for everything from BADCamp to get in to see if there are more merge conflicts.
Comment #134
Fabianx CreditAttribution: Fabianx commentedYes, indeed thanks so much. I have this patch open for later review, but waiting for BAD camp patches to settle down is definitely a good idea!
Comment #135
mpdonadioOK, I think this covers all current cases of HTML from being sent to the link generator.
Comment #136
dawehnerI consider this fix to be wrong, given that it won't run path aliases anymore.
Comment #137
mpdonadioI'll look into this particular use, but code blips like this
are used in a bunch of places in Views and Views UI in HEAD, and #2364157: Replace most existing _url calls with Url objects introduce even more.
Comment #138
mpdonadioComment #139
dawehnerWell ... these are cases where you really don't have a route name/parameter available, so the actual proper
way here would be to use URL objects in views fields everywhere possible (route names or unrouted urls).
In general I think linking to
/node/1
should resolve the path alias, shouldn't it?Comment #140
pfrenssenRerolled patch against latest HEAD.
Comment #141
martin107 CreditAttribution: martin107 commentedtriggering testbot
Comment #142
dawehnerI'm sorry, but I don't think this is solved yet.
We rather need support for URL objects and use them in views somehow, as this will give us support for path aliases again.
Comment #143
mpdonadioWould something like this work?
Comment #144
pfrenssenI was confused about #142 so I asked @dawehner for input.
This
Drupal\views\Plugin\views\field\Url
handler is used to optionally render a URL field as a link. A URL field is a pure URL string, it does not have any metadata like a title, attributes or anything. In core it is only used in the Aggregator module, in the Feed display of the Aggregator Sources view. In this particular use case it is only possible to enter fully qualified URLs which works fine.@dawehner's concern is that if a field allows relative URLs (like path aliases) then these should be rendered correctly as well. It looks like @mpdonadio's suggestion is on the right path. I will have a look and see if we can get some test coverage for this.
Comment #145
pfrenssenI just thought of another use case: an administrator might enter a dynamic link to a local path that is not handled by Drupal, and this should be handled as well. So we should support:
Comment #146
pfrenssenThe suggested approach from #143 was indeed correct. Rolled it in the patch and extended test coverage.
Comment #147
jibranWe don't need it. it is a service now.
Comment #148
pfrenssenThanks! Going to address it.
Comment #149
dawehnerI'm fine with that change in the Url handler.
In general I really like getting rid of the 'html' in that many places, though I'm not 100% sure
about removing the feature, but well, in case we really need it, we can still add it back, if needed.
Ideally we should not introduce drupal_render(), but this is alright, its not critical for itself.
Comment #150
pfrenssenI replaced the wrapped
drupal_render()
with the injected renderer service. Was stuck for a long time when I was adapting the PHPUnit tests due to a puzzling problem in which PHP was not making a copy of the $text variable in the render() method when it was overwritten. This is probably a bug in PHP. I even checked in 5.4.17, 5.5.16 and 5.6.2 and 5.6.3, but could replicate it in all PHP versions. I had to resort to renaming the variable in the end:I also fixed a bug in my previous patch, if a dynamic path was starting with a slash it was throwing an error. I also added a test case for this.
Comment #151
Fabianx CreditAttribution: Fabianx commentedWow, fascinating.
I tried to reproduce it on 3v4l.org, but no dice:
http://3v4l.org/UAkFP
Comment #152
pfrenssen@Fabianx, can you replicate it with the LinkGeneratorTest? Using your minimal script on 3v4l.org I also cannot replicate it. It might be due to the renderer being mocked in the test.
Comment #153
dawehnerThis final thing is just such a nitpick that I don't want to not RTBC just for that.
Renaming the variables is sure fine, given that it makes the code a bit more explicit.
Final nitpick: Let's typehint the interface here ...
It is great that we describe why we do things here.
Comment #154
pfrenssenAhh the drop is on the move again, the patch doesn't apply any more. Will reroll and fix the type hint.
Comment #155
pfrenssenRerolled and unpicked nits. Only documentation changes so if this is green this is RTBC again.
Comment #156
swentel CreditAttribution: swentel commentedRTBC as per #153
Comment #157
alexpottLooks like we need a change record for this. And we need to link to the original conversion
l()
too.Comment #158
pfrenssenWill write change record.
Comment #159
pfrenssenNoticed something while looking at the patch while writing the change record:
Indentation is wrong here.
Comment #160
pfrenssenDraft change record is at 'html' => TRUE option is removed from l() and link generator.
Fixed the indentation.
Comment #161
pfrenssenComment #162
Wim LeersCR looks awesome. But this is still missing:
Comment #163
dawehnerUpdated
Comment #164
alexpottThis issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 0bcabf5 and pushed to 8.0.x. Thanks!
Comment #166
alexpottThis is causing random fails see #2392865: Random fail in \Drupal\system\Tests\Entity\EntityOperationsTest. The suspect code is:
Reverting...
Comment #168
tim.plunkettThis is the patch from #160 but with that hunk removed. That looks like exactly the culprit.
Let's see what fails without it!
Comment #171
pfrenssenGoing to try to get this green again today on the last day of the DA sprint in Ghent.
Comment #172
pfrenssenThe failure occurs in
Drupal\system\Tests\Entity\EntityOperationsTest::testEntityOperationAlter()
. In this test a link to a path is generated which includes the entity label. In this particular case the entity is a user role, and inWebTestBase::drupalCreateRole()
a random label is generated which can includes characters that needs HTML encoding.It's clearly the test here that is at fault and not the functionality. We should not generate paths including HTML encoded characters and then call assertLink() which cannot deal with HTML encoded characters. We have two possible ways to fix this:
Comment #173
pfrenssenWent for option #1 since this is least disruptive, even though it is a workaround.
The change to
AssertContentTrait::assertLink()
that was introduced in this issue is now undone, so this should not cause any other potential random failures.Interdiff is against patch #160.
Comment #175
pfrenssenWoops just saw that I goofed up the documentation. Cancelled the previous test and started a new one.
Comment #176
xjmComment #177
Fabianx CreditAttribution: Fabianx commentedBack to RTBC, changes are test-only
Comment #179
Wim LeersComment #180
martin107 CreditAttribution: martin107 commentedNo conflicts, just automerging
That the site installs is about all I can see. Lets see what testbot has to say.
Comment #183
martin107 CreditAttribution: martin107 commentedComment #184
joelpittetBack to RTBC as per #177
Comment #186
alexpottI have looked at this issue several time over the past few months and talked about it with @xjm and @effulgentsia. Even though I committed this once I'm not convinced that this actually critical or worth pursuing in its current form.
re #116 - just because a developer could do:
in Drupal 7 doesn't make this critical.
The changes here make link generation more difficult for little DX gain. What is to stop someone doing:
I just don't think that the DX of inline templates (which is terrible) is worth the suggested security benefits. Yes people can shoot themselves in the foot with API code - this is not the only place that is possible.
That said I think it is reasonable for link generation to be able to accept a render array and not require the
html => TRUE
setting if a render array is provided as in certain instances this results in nicer DX.I'm leaving at critical for the week so that other people can have their say but I won't be committing this unless I can be convinced of the benefits.
Comment #187
alexpottLooking at the patch...
Is an unrelated change
Comment #188
pfrenssenIf this is demoted from critical it is not eligible for inclusion in 8.0.x any more?
I think it's a nice improvement to be able to use render arrays here, and I do think it is a security improvement - at least if you use the API properly. In the patch you find some examples where this gives additional protection. For example look at the change in shortcut_preprocess_page():
In the original code $link_text is not escaped since it is known to be safe - it contains a choice of some hardcoded strings. However in the future $link_text might change to include some potentially unsafe user sourced data. In the original case this would require a check_plain() to be added, but thanks to this patch we are protected by default.
Comment #189
dawehner@pfrenssen
You can still allow arbitrary strings but on top some render array.
Comment #190
effulgentsia CreditAttribution: effulgentsia commentedI agree with reprioritizing this from Critical to Major. I also think this is more of a Task rather than a Bug, because even though this might fix some double-escaping bugs, I think we can achieve that same fix with a much less disruptive patch, such as only keeping the change from String::checkPlain() to SafeMarkup::escape() within LinkGenerator::generate(). However, per https://www.drupal.org/contribute/core/beta-changes, non-critical security improvements are still open for consideration during the beta phase, so long as a branch maintainer believes the net benefit to justify the net disruption.
FWIW, I think that it is worth pursuing in roughly its current form, but I think we can improve some of the hunks where inline templates are used, and I think other inline template hunks are acceptable. Below, I'll comment on each of the patch's 15 inline_template additions (note that some of these points have been raised in prior comments as well, but I'm too lazy right now to read through all of them to give proper attribution):
This is the preprocess function for menu-local-task.html.twig. How about moving all this accessibility markup into that Twig file?
This is a pretty advanced combination of translating a string with both embedded markup and a variable. I don't think the DX of the inline template is significantly more cumbersome than the existing code.
These are tests. I think within a test, it would be perfectly fine to use
\Drupal::l(SafeMarkup::set('<b>‹</b> ' . $previous->label()), $url)
instead, especially if we're able to control our setUp() method to generate labels without HTML entities. If we're unable (or unwilling) to control that, then HEAD's code is actually a bug that contains random failures, and I think the use of an inline template is an ok way to get the correct escaping and fix those random failures.This is a test, and we have a literal, so I think
\Drupal::l(SafeMarkup::set('<object>Link</object>'), ...)
would be fine. In general, I don't see any problem with using SafeMarkup::set() on a literal, even within non-test code; it's only the embedding of variables where there's a risk.Especially when you consider that #prefix and #suffix, I think this entire render array can benefit from a Twig file template.
Generic test coverage addition. This is good.
This looks to me like a pointless span, and one added to the front-end, not just to Views UI. Can we get rid of it?
To me, it looks like the real problem here is that Views UI is adding a whole lot of unnecessary spans. I suspect this is a historical artifact of supporting old browsers, and that there's no longer a need for them. Or, if there is, then I think we need a new Twig file template, like
views-ui-label
.So in summary, outside of tests, the only inline template that I see as having a good justification for remaining is the one in
FeedViewBuilder
. And to me, this indicates that it's a sufficiently uncommon thing that the extra verbosity of it isn't a big deal.Whether we want to make "fixing" the other usages a prerequisite for this issue or a follow-up, I don't know. I'd be fine with either.
Comment #191
effulgentsia CreditAttribution: effulgentsia commented#2273925: Ensure #markup is XSS escaped in Renderer::doRender() is for addressing that problem space.
This issue isn't about preventing someone from using an API incorrectly; it's about establishing the API for correct usage. Concatenating HTML and variables is a problem, and doing so as the value of #markup is using the #markup API incorrectly. I think Twig, whether a file or an inline template, is our best API for doing that combination.
Comment #192
effulgentsia CreditAttribution: effulgentsia commentedI've rethought this. I think
String::format()
is a perfectly adequate API for combining literals and variables into a safe HTML string. Here's a patch with some of those inline templates converted. Curious to get feedback on this before doing the others. Related: #2399037: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument.Comment #193
alexpottAs per #186 and #190 I'm downgrading this to major and making a task.
This issue is a major task. To evaluate whether we should make this change during the beta, we need to update the issue summary according to https://www.drupal.org/core/beta-changes to explain what improvements it makes and what the disruption of the change would be. Can someone add Drupal 8 beta phase evaluation template to the issue summary.
Comment #194
effulgentsia CreditAttribution: effulgentsia commentedThere's a bit more I want to do on this patch before updating the issue summary, but here's the remaining inline_template => String::format() conversions in the meantime.
Comment #195
effulgentsia CreditAttribution: effulgentsia commentedI went through and cleaned up some remaining code and comments, and reverted a bunch of hunks that I think are outside the scope of this issue. I'll open followups for them so that we don't lose the work that went into them, but let's keep this issue focused.
Comment #197
effulgentsia CreditAttribution: effulgentsia commentedI haven't yet filled in the beta evaluation, but have updated the summary for the new approach of String::format() rather than inline_template.
Comment #198
effulgentsia CreditAttribution: effulgentsia commentedComment #199
effulgentsia CreditAttribution: effulgentsia commentedComment #201
dawehnerNo question, the
String::format()
approach provides much better DX!Just a quick comment. One advantage of the inline template was, that you could easily spot places which probably should be converted to be a template.
Comment #202
effulgentsia CreditAttribution: effulgentsia commentedA search for
String::format('<
would be the new way to find those. Frankly, I'd love to get to a point where you could do a search for</
within all PHP files and find nothing (i.e., no HTML tags at all within PHP code), but we're not even close to there yet.Comment #203
effulgentsia CreditAttribution: effulgentsia commentedFilled in beta evaluation. I think this is ready for human review (and maybe an RTBC?).
Comment #204
effulgentsia CreditAttribution: effulgentsia commentedComment #205
effulgentsia CreditAttribution: effulgentsia commentedRemoved this part from the issue summary:
because there is now a patch on that issue that isn't disruptive to this one.
So, I think this patch is now good to go if others agree.
Comment #208
pfrenssenThat was to be expected after 3 weeks.
Comment #209
pcambraPlain reroll of this
Comment #211
effulgentsia CreditAttribution: effulgentsia commentedNeeds another reroll :(
Comment #212
pcambraNot a plain reroll this time
Comment #214
effulgentsia CreditAttribution: effulgentsia commentedSome fixes.
Comment #215
effulgentsia CreditAttribution: effulgentsia commentedCleaned up issue summary now that both #2399037: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument and #2435493: Change String::format()'s '@' and '%' placeholders to be auto-escaped rather than always-escaped have been committed. The patch in #214 is already fixed to benefit from the latter.
Comment #216
effulgentsia CreditAttribution: effulgentsia commentedFixed pluralization error in the issue summary.
Comment #218
mpdonadio@effulgentsia do you want help with the remaining fails?
Comment #219
effulgentsia CreditAttribution: effulgentsia commented@mpdonadio: thanks for the offer, but I think this should now be green. If it is, I'd love a review though.
Comment #220
mpdonadio@effulgentsia I can give this a looking at over the next few days, but how much of the originally committed patch do you think remains? I had a fair amount of work in that version, so I don't know if is appropriate for me to RBTC if there are still signifiant chunks left (though I think a lot of my work was converting things that this version redid...)
Comment #221
effulgentsia CreditAttribution: effulgentsia commented@mpdonadio: it's up to you, but in my opinion, you're eligible to RTBC it, because it's substantially different. Your work was absolutely instrumental in getting this issue figured out, but I think the actual lines of code are different enough that you'd be reviewing it with sufficiently fresh eyes.
Comment #222
mpdonadioAs part of a review, I took the patch in #219 and added
and threw it as TestBot, and it came back green. Did a search w/ PhpStorm, and am not seeing any stray uses relating to link generation, either, except in two comments
I think we can be pretty confident that this patch does catch everything. I will take a closer look at everything tomorrow.
Comment #223
Fabianx CreditAttribution: Fabianx commentedApologize if this was explained already, but this looks different.
When I look at the function I can see that $link_element was constructed.
I think it was be saner to not rely on that structured just here without changing e.g. $link['attributes'] unless there is a reason.
This is really nice.
I wonder if we not safely could replace lots of String::checkPlain calls directly with SafeMarkup::check().
(That is a follow-up.)
The result is the same basically.
Overall this looks really great to me!
Leaving for mpdonadio for final RTBC!
Comment #224
mpdonadioJust noticed the CR for this was still published, and it out of date for the proposed solution in the IS.
Comment #225
mpdonadioIs there a problem that the string '(active tab)' is running through t() twice? Double translation?
Interesting followup.
I just checked the docblock on String::format(). It doesn't explicitly mention that the return value is marked as safe as long as a !arg isn't used. There should probably be a followup to add this in (maybe a followup one of the String::format issues?) . The new version of this patch relies on this behavior, so it should be documented on this function.
The checkPlains in the function are removed because they get passed into #default_value in ::buildOptionsForm() or get somewhere Twig auto-escape is used?
This is just to add a different boolean key to make sure they merge properly?
\Drupal\link\Plugin\Field\FieldFormatter\LinkSeparateFormatter::viewElements() that need to be fixed.
When I read this in context, the change made sense. @Fabianx, I don't totally follow your comment on this?
I hadn't really paid close attention to this after the first version was reverted, as I got busy with other issues. The new version of this is really nice.
None of this is major.
Comment #226
effulgentsia CreditAttribution: effulgentsia commentedJust a straight reroll. Working on above feedback next.
Comment #227
effulgentsia CreditAttribution: effulgentsia commentedFollowing same numbering as #225:
Comment #228
effulgentsia CreditAttribution: effulgentsia commentedUpdated https://www.drupal.org/node/2392803 and unpublished it pending patch commit.
Comment #229
mpdonadioChanges look good. Patch is green. CR update looks good. This looks ready for prime-time.
Comment #230
alexpottThis is way better than the first patch I committed way back when. I think that the disruption cause by removing the html option is worth the costs - see beta evaluation in the issue summary. Thanks for adding it.
Committed db60a8c and pushed to 8.0.x. Thanks!
Comment #232
Fabianx CreditAttribution: Fabianx commentedI published the change record! Thanks so much for all the work on this @all!