Problem/Motivation

(name of function/class::method that calls it) calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

  • Remove the call by refactoring the code.
  • If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required.

Use of the functionality itself was removed in #1851414: Convert Views to use the abstracted dialog modal and is now dead code, so it can be removed.

Remaining tasks

  1. Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
  2. Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.
  3. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.

Manual testing steps (for XSS and double escaping)

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

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

    and ensure that it is sanitized.

User interface changes

N/A

API changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

leslieg’s picture

Working on removing SafeMarkup::set in ViewsUI::getDefaultAJAXMessage() as part of NHDevDays2

leslieg’s picture

Status: Active » Needs review
FileSize
614 bytes

changed SafeMarkup::set to SafeMarkup::format

star-szr’s picture

Status: Needs review » Needs work
+++ b/core/modules/views_ui/src/ViewUI.php
@@ -199,7 +199,7 @@ public function set($property_name, $value, $notify = TRUE) {
-    return SafeMarkup::set('<div class="message">' . t("Click on an item to edit that item's details.") . '</div>');
+    return SafeMarkup::format('<div class="message">@message</div>', ['@message' => "Click on an item to edit that item's details."]);

We lost the t()! Need to get t() back :)

leslieg’s picture

Status: Needs work » Needs review
FileSize
617 bytes

Fixed issue with missing t that @cottser found

Status: Needs review » Needs work

The last submitted patch, 4: remove_safemarkup_set-2501933-4.patch, failed testing.

ashutoshsngh’s picture

Status: Needs work » Needs review
FileSize
643 bytes
566 bytes
star-szr’s picture

+++ b/core/modules/views_ui/src/ViewUI.php
@@ -199,7 +199,7 @@ public function set($property_name, $value, $notify = TRUE) {
-    return SafeMarkup::set('<div class="message">' . t("Click on an item to edit that item's details.") . '</div>');
+    return SafeMarkup::format('<div class="message">@message</div>', ['@message' => t("Click on an item to edit that item's details.)"]);

@leslieg - Just need to move the " one character to the left here.

@ashutoshsngh thanks for the patch. Normally when someone is working on an issue we give them a bit of time to respond to reviewers, and it's nice to say in your comment what you are changing. Although the interdiff is helpful there!

I think I like the approach in #4 better but could go either way.

star-szr’s picture

Title: Remove SafeMarkup::set in ViewsUI::getDefaultAJAXMessage » Remove SafeMarkup::set in ViewUI::getDefaultAJAXMessage
Issue tags: +VDC
ashutoshsngh’s picture

@Cottser will keep in mind your suggestion from next time.I worked because #4 failed testing.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

This does the trick, thank you.

joelpittet’s picture

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks folks for taking this on.

+++ b/core/modules/views_ui/src/ViewUI.php
@@ -199,7 +199,8 @@ public function set($property_name, $value, $notify = TRUE) {
+    $message = t("Click on an item to edit that item's details.");
+    return SafeMarkup::format('<div class="message">@message</div>', ['@message' => $message]);

So in general this is a pattern I think we should discourage as much as possible. We're essentially calling SafeMarkup::format() twice on the same string (and therefore doing the entire sanitization twice and using the internal SafeMarkup::set() twice, adding variants of the same string to the SafeMarkup list).

It would be better to either (a) put the markup within one t() call if translators are the audience that might alter the markup, or (b) use a template, inline template, or render array if translators should not have to deal with the markup and/or themers are the ones who should control it. In this case, given that it's a div with a specific class and referencing the guidelines for HTML in translatable strings, I think the second option is correct. I think an inline template would be the minimum fix here if we can make it handle a render array, and refactoring for actual themeable output would be better still. Let's look at the usage of this method in the Views UI and see what we can do.

joelpittet’s picture

a) I'd rather this than put the markup in t(). Translators have a enough to worry about let alone breaking markup. I'd take this patch 100 times more than putting the markup in the t().

b) Inline template feels a bit like an overkill from DX considering we don't need any template logic.

xjm’s picture

But why should Views a div and class that themers can't control? We're essentially just hacking around Views doing a not-best-practice thing.

After all: https://api.drupal.org/api/drupal/8/search/status-messages.html.twig

xjm’s picture

Oh, I should have mentioned that I talked to @alexpott about this issue briefly this morning and he seemed to agree (though we didn't dig into what Views was doing at the time).

But my perspective is that part of the original "contract" of the SafeMarkup issue is that we would remove the technical debt by doing less concatenating of hardcoded HTML in PHP and convert more of it to the render and theme system. So this is a case to me where that at least seems possible. If we decide it's too much work and disruption to get the critical done, I'd be up for filing a postponed followup, but I'd like to have at least tried it and know what the change would entail in the Views UI.

xjm’s picture

(Adding @joelpittet and @Cottser to the suggested commit message for their review and feedback here.)

dawehner’s picture

The best solution for this issue is to remove that function and its single usage.

The only usage of that has been in javascript, though, got removed as part of #1851414: Convert Views to use the abstracted dialog modal long time ago,
see

$ git show 3efe9ba1fd790eeffc79a36a0eda20476ea7845f | grep "defaultForm"
-    Drupal.AjaxCommands.prototype.viewsSetForm({}, {'title': '', 'output': drupalSettings.views.ajax.defaultForm});
xjm’s picture

@dawehner, that sounds even better yet.

xjm’s picture

Technically we should consider whether we just deprecate it, but this might be a scenario where removing it is better if it's already essentially mostly dead code and you'd only use it if, what, you were overriding the Views UI? GLHF. :)

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.01 KB

ok. I see in #1851414: Convert Views to use the abstracted dialog modal
- Drupal.AjaxCommands.prototype.viewsSetForm({}, {'title': '', 'output': drupalSettings.views.ajax.defaultForm});

So I removed the function, and also the place where drupalSettings.views.ajax.defaultForm was set.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Technically we should consider whether we just deprecate it, but this might be a scenario where removing it is better if it's already essentially mostly dead code and you'd only use it if, what, you were overriding the Views UI? GLHF. :)

This is why we need to declare what we define as API and what not.

IMHO this is certainly not API

The fix for itself looks perfect

xjm’s picture

Title: Remove SafeMarkup::set in ViewUI::getDefaultAJAXMessage » Remove dead code in ViewUI::getDefaultAJAXMessage()
Issue summary: View changes

Excellent! I confirmed that these were the only uses of that AJAX element and method.

xjm’s picture

Status: Reviewed & tested by the community » Fixed

Since this code is not used, it made me wonder how this SafeMarkup set got added in the first place, since it doesn't appear to have test coverage that would have failed and there was no manually testable. It got added in the original issue in #1825952-99: Turn on twig autoescape by default by chx with no specific explanation of why, but it was following a large merge/change.

I don't think this needs a change record.

This issue helps solve a required part of a critical and reduces fragility by removing dead code, , so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. Committed and pushed to 8.0.x.

  • xjm committed f1d2776 on 8.0.x
    Issue #2501933 by leslieg, ashutoshsngh, YesCT, joelpittet, Cottser,...
xjm’s picture

Oh, I also checked for whether we could remove the use statement, but it is used other times in the file.

olli’s picture

+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -126,7 +126,6 @@ public function form(array $form, FormStateInterface $form_state) {
       'id' => '.views-ajax-body',
       'title' => '.views-ajax-title',
       'popup' => '.views-ajax-popup',
-      'defaultForm' => $view->getDefaultAJAXMessage(),

Filed #2505965: Remove dead code related to old views ui modal to remove also views-ajax-body, -title and -popup.

Status: Fixed » Closed (fixed)

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