Problem/Motivation
Many calls to SafeMarkup::set() are a result of concatenating safe markup together, either with no glue or known safe glue (not coming from user input). Existing patterns / usecases:
- A determinate (but possibly conditional) series of translated strings, joined together with whitespace (spaces, break tags, paragraph tags) and sometimes other basic "text" markup like lists, sometimes with some conditional logic based on user permissions, etc. Commonly used in:
- drupal_set_message(). See: #2505497: Support render arrays for drupal_set_message(). Example issues:
- hook_requirements(). See: #2505499: Explicitly support render arrays in hook_requirements(). Example issues:
- #2296929: Remove system_requirements() SafeMarkup::set() use
- #2501683: Remove SafeMarkup::set in _update_message_text() (note the text is also used in update_mail()
- hook_help(). Currently, hook_help() always expects a string of HTML that is set to a
#markup
element of a render array, so individual hook_help() pages always go through SafeMarkup::checkAdminXss() following #2273925: Ensure #markup is XSS escaped in Renderer::doRender(), and therefore there are no SafeMarkup::set() calls that need to be removed.
- An indeterminate, imploded list of translated or sanitized strings that is a list of items. Example issues:
- #2501937: Remove SafeMarkup::set in ViewsUIController::reportFields()
- #2501971: Remove SafeMarkup::set in FieldStorageConfigListBuilder
- #2505931: Remove SafeMarkup::set in ViewListBuilder
- An indeterminate, imploded list of strings that is not a list of items. Examples:
- #2478845: Locked view warning message rendering as code
- #2501931: Remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender
- Composed HTML at the element/tag level. Example issues:
- One of the hunks in #2502089: Remove SafeMarkup::set() in template_preprocess_views_view_table()
- Composed HTML at the sub-tag level. Example issues:
- #2501705: Remove SafeMarkup::set() in LinkGenerator and document SafeMarkup::set() in LinkGeneratorTest
- #2505701: Document SafeMarkup::set and Use htmlspecialchars() directly in Attribute() so we don't bloat the list of safe strings
- Other issues:
- #2501757: Remove SafeMarkup::set in NodeSearch::prepareResults() does not really fit into category 1 as these are not translated strings defined in the current function, but processed strings and markup from other sources.
Currently there is a way to safely join an array of strings only in Twig templates via |safe_join
which maps to twig_drupal_join_filter() in twig.engine. Also currently, there is no known way to replicate this directly in PHP for when you need strings other than using #inline_template and immediately rendering the inline template.
In earlier versions of the autoescape issue there was a SafeMarkup::implode() but after discussion there it was not committed. See the discussion starting around #1825952-235: Turn on twig autoescape by default.
Examples
The following examples could likely benefit from whatever we decide on.
- \Drupal\views\Plugin\views\style\Rss::render()
- \Drupal\views_ui\ViewListBuilder::buildRow()
- _batch_test_finished_helper()
Proposed resolution
One suggestion is
- Bring back
SafeMarkup::implode()
in the form used for|safe_join
. - Call it
SafeMarkup::join()
because it's simpler to type. - Let
twig_drupal_join_filter()
use the newSafeMarkup::join()
to avoid code duplication.
Remaining tasks
TBD
User interface changes
N/A
API changes
TBD
Comment | File | Size | Author |
---|---|---|---|
#29 | Screen Shot 2015-07-11 at 7.05.07 PM.png | 72.99 KB | cilefen |
Comments
Comment #1
star-szrComment #2
joelpittetUpdating IS with proposal
Comment #3
xjmJust quoting my comment from that issue so people see the context here:
I still think the correct solution is to refactor to remove early escaping and rendering wherever possible. This was also what @effulgentsia, @alexpott, and I discussed for
SafeMarkup
in general at DrupalCon Amsterdam (notes on SafeMarkup from that AMS meeting).However, I recognize this would be a lot more work, and probably disruptive in some cases, and maybe it's too late depending on what exactly it would entail.
To me, the goal of removing the
SafeMarkup::set()
calls is not only just that it's for internal use and we don't want to set a bad/dangerous precedent. It's also to actually take the places in core we hacked around originally and fix them so we're leveraging Twig more rather than doing all this work twice. (Part of the original goal for enabling autoescaping was to remove overhead, but we acknowledged in the committed patch that we didn't get there and that it would need to be followup work.)Comment #4
xjmRetitling to focus less on a specific solution. I'm -1 on the proposed resolution still.
Comment #5
xjmOne key difference between SafeMarkup and Twig is that Twig is only escaping strings right there where they are printed. SafeMarkup is adding them to a giant memory hog of a static array with strings within strings all saved separately, and the string that's marked safe once is marked safe for everywhere. See also #2295823: Ensure that we don't store excessive lists of safe strings and #2488538: Add SafeMarkup::remove() to free memory from marked strings when they're printed (the summary documents an actual memory problem on the core permissions page already).
Comment #6
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFrom a cursory look at the IS examples, I think such a refactor (to not require SafeMarkup::join() or inline_template) is quite doable for all but the first one. Not sure yet about that first one though. So perhaps we should get through the ones that we can first and then see what's left and whether to solve them with this issue's proposal or something else?
Comment #7
tetranz CreditAttribution: tetranz as a volunteer commentedI'm kind of new here so you're all way ahead of me but I came here after looking at https://www.drupal.org/node/2501971
If we were to bring back SafeFormat::implode, would something like this be the way to go?
I don't think that's quite the same as the earlier uncommitted code.
I can see how it would be better if it at all possible to get the list all the way to Twig as an array and let Twig handle it.
Comment #8
joelpittetAdding another one that would benefit from this helper method in PHP. #2501971: Remove SafeMarkup::set in FieldStorageConfigListBuilder
The biggest reason is that it's using
#type table.
and the particular cells aren't easily modified in the template and it's trying to build a comma separated list of the pieces.Comment #9
joelpittet@tetranz sorry you said the same thing... so +1 to that:)
Comment #10
star-szrAdding another example. @effulgentsia I would love to know more!
Comment #11
joelpittetAdding #2501667: Remove SafeMarkup::set in __toString() in Attribute class to the list in the Issue summary.
Comment #12
joelpittetRemoving template_preprocess_views_ui_view_info because I'm moving that to the template with
|safe_join
.Comment #13
joelpittetAdding this to the list: #2501931: Remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender
Comment #14
chrisfromredfinOK, just spitballing here. Not sure if this helps at all or not, but what about a helper method that does the implode via an inline Twig template?
...with that said, quite frankly, this seems pretty dumb. I have no idea why this is would be an advantage at all, but since I'm not 100% sure what I'm doing, hopefully this might at least spark something. :)
Another thought I had is that maybe we have a limited number of use-cases here, and maybe we could have some sort of glue whitelist? That might mean something like SafeMarkup::joinComma(), SafeMarkup::simpleConcat(), and SafeMarkup::joinBreak()... or maybe it's something like:
We're talking about DX here, so the question is... do we default to trusting our developers, or no? If we should just trust them, then we need to just have even more faith in our code review process, to make sure things like
...don't make it in.
Final thought - if all can be refactored except the first one why can't #2501937: Remove SafeMarkup::set in ViewsUIController::reportFields() (the first one) be refactored as an item list? Perhaps as an inline twig template? It's a list of views that implement a plugin (and the other SafeMarkup::set() that appears in this file is a similar list). So I tend to lean on the xjm side, if these can all be refactored, why not? (Potential answers being it's either too hard, or we're refactoring into something that's even lamer than the proposed safe join.)
Comment #15
joelpittetAdding another one to the list #2478845: Locked view warning message rendering as code
Comment #16
joelpittetRemoving the
template_preprocess_file_widget_multiple()
because I think I have a solution here:#2502781: Remove SafeMarkup::set in template_preprocess_file_widget_multiple()
Comment #17
xjmSo, it occurs to me: a comma-joined string is a special case of an item list. I've overridden item list theming in the past to make things comma-separated. See #2502095: Remove SafeMarkup::set in template_preprocess_views_ui_view_info() which joins the items in the template, and also my latest comment on #2501827: Remove SafeMarkup::set() in file_save_upload() and allow render/template code to control a single-item list about the item listing there.
This brings us back to the issue that certain dedicated HTML-string-printing messaging things (like
drupal_set_message()
,hook_requirements()
, probablyhook_help()
, etc.) should potentially support render arrays, and the tradeoff there for needing to add a dependency on the renderer and needing to have a fallback for non-HTML output at least in tools like drush.Comment #18
cilefen CreditAttribution: cilefen commentedSomething we tried: #2505931: Remove SafeMarkup::set in ViewListBuilder.
Comment #19
xjmI had a long discussion with @alexpott yesterday. There are actually a number of different patterns where markup is being joined. This list has the same intent as #2280965: [meta] Remove every SafeMarkup::set() call, but the categories I've identified are actually different:
#markup
element of a render array, so individual hook_help() pages always go through SafeMarkup::checkAdminXss() following #2273925: Ensure #markup is XSS escaped in Renderer::doRender(), and therefore there are no SafeMarkup::set() calls that need to be removed.There are also a whole mess of different possible implementations, which I'll post more on later.
Comment #21
xjmCategorizing the issues in the summary previously into the categories I've defined above.
Comment #22
xjmComment #23
xjmSo, note that #2501667: Remove SafeMarkup::set in __toString() in Attribute class was in the original summary as an example of an issue that could use this proposed
join()
method, and that's actually an example of exactly the sort of misuse of the API I thinkjoin()
encourages, and why @pwolanin and I removed it in the original #1825952: Turn on twig autoescape by default. It encourages polluting the string list with partial strings and partial tags that there is no need to print independently. The pollution of the string list is not a trivial problem; by coming up with a different solution for Attribute in #2505701: Document SafeMarkup::set and Use htmlspecialchars() directly in Attribute() so we don't bloat the list of safe strings, @pwolanin helped us remove 15 MB (!) from the SimpleTest UI, according to @alexpott's testing.join()
also can be misused for some really bad practices, producing malformed or unsafe HTML when assembled (when combined with use ofset()
orformat()
that it seems to encourage). Again, here's a couple examples that were in the original SafeMarkup patch:This gives a false sense of security and can lead to adding XSS to the safe list.
By the way, I also have similar concerns about using
SafeMarkup::format()
for assembly of HTML elements from incomplete parts. Consider this example from patches in #2502089: Remove SafeMarkup::set() in template_preprocess_views_view_table():The code by itself gives a false sense of security. In the context in Views this is safe (or at least covered by a privileged permission) but suppose someone managed to pass this input:
Boom. Plus the same thing about polluting the string list.
So, in general, I think we need to be very careful about what API we provide and what examples we provide of using it.
Comment #24
xjmI also filed #2506021: When testing for XSS with assembled HTML, use a test exploit that does not contain quotes after thinking about this.
Comment #25
xjmI also filed #2506035: Do not add placeholders from SafeMarkup::format() to the safe list.
Comment #26
xjmAlright, so, separately from the different categories of joining strings. Here are strategies that have been used in patches to replace
SafeMarkup::set($some_joined_strings)
(mostly just the fixed issues for now; I'm going through the open ones still):SafeMarkup::format('@string1 @string2')
(sometimes with additional markup/formatting) where the strings are locally defined with t()SafeMarkup::format('@string1 @string2')
(sometimes with additional markup/formatting) where the strings are variables passed in from elsewhereSafeMarkup::format()
with sub-tag elementst('Additional string that is relevant string for context. @string2');
t('Previously separate strings. Actually part of the same paragraph.')
SafeMarkup::checkAdminXss()
or Xss::filter()|safe_join
filterEdits: adding additional issues.
Comment #27
hass CreditAttribution: hass commentedCould someone give me a hint why http://cgit.drupalcode.org/google_analytics/tree/src/Form/GoogleAnalytic... shows now
<div class="description">
and how I should fix it now, please?Comment #28
cilefen CreditAttribution: cilefen commented@hass I answered you on the cross-post #2280965-95: [meta] Remove every SafeMarkup::set() call. Wrapping in Safemarkup::format() should do fine here.
(edited a typo)
Comment #29
cilefen CreditAttribution: cilefen commented@hass
Comment #30
hass CreditAttribution: hass commentedWhat is better with
SafeMarkup::format()
thanSafeMarkup::set()
?set
is shorter and seems working the same way...Comment #31
cilefen CreditAttribution: cilefen commented@hass #2280965: [meta] Remove every SafeMarkup::set() call. It is considered an internal function.
Comment #32
cilefen CreditAttribution: cilefen commentedAll:
I went with SafeMarkup::format() on #2531652: Fix double escaping of domain_mode radios. But, was it done properly? I think I should have used tokens. Despite having attended a contribution sprint on this very topic, I am perpetually confused by it and I think a lot of contrib authors are or will be.
Comment #33
hass CreditAttribution: hass commentedComment #34
cilefen CreditAttribution: cilefen commentedRe #32, that one should be good because the user-entered strings passed through t().
Comment #35
lauriiiThis one is blocking #2501975: Determine how to update code that currently joins strings in SafeMarkup::set()
Comment #36
cilefen CreditAttribution: cilefen commented@lauriii Then we are in trouble for sure.
Comment #37
lauriiiLOL! This is the issue this is blocking #2501971: Remove SafeMarkup::set in FieldStorageConfigListBuilder
Comment #38
lauriiiIs there anything I could do to try to move this forward? This is blocking few issues that are blocking Critical meta from getting finished
Comment #39
akalata CreditAttribution: akalata commentedComment #40
akalata CreditAttribution: akalata commentedStarting to review and see if there's a consensus buried in here somewhere...
Comment #41
akalata CreditAttribution: akalata commentedfrom xjm @ MWDS: this issue should no longer be listed as a blocker for anything
Comment #42
xjmAll the
SafeMarkup::set()
calls have been removed from core, so this was resolved through those issues. I'm working on updating https://www.drupal.org/node/2296163 with the kinds of solutions used, but we can call this issue a duplicate now. Thanks everyone!