Problem/Motivation

drupal_check_module() 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.
  • Add the @requirements_message, parentheses around the "(Currently using...)" text, and the word "version" to the translated string.

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.
  4. Needs feedback from multilingual contributor regarding point three in comment #6

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. Adjust a module install to make its requirements be FALSE, for instance in aggregator.install change $has_curl = function_exists('curl_init'); to $has_curl = !function_exists('curl_init');
  3. Now, attempt to install the Aggregator module and it will show the error as seen below.
  4. Compare the output in HEAD and with the patch applied. Confirm that there is no double-escaping.
  5. If there is any user or calling code input in the string, submit
    alert('XSS');

    and ensure that it is sanitized.

User interface changes

The word "version" has been added for clarity prior to the @version output.

Before:

After:

API changes

N/A

Files: 
CommentFileSizeAuthor
#16 interdiff-2501639-15.txt919 bytescrowdcg
#16 2501639-15.patch1.31 KBcrowdcg
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,902 pass(es). View
#16 PATCH15-admin-modules-1434386095780.png200.01 KBcrowdcg
#14 Error-admin-modules-1434385771277.png201.16 KBcrowdcg
#10 interdiff-2501639-10.txt980 bytescrowdcg
#10 2501639-10.patch1.28 KBcrowdcg
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,864 pass(es). View
#7 interdiff-2501639-2-7.txt1.05 KBcrowdcg
#7 2501639-7.patch1.37 KBcrowdcg
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,776 pass(es). View
#4 Screen Shot 2015-06-06 at 11.48.50 AM.png196.85 KBcwells
#4 Screen Shot 2015-06-06 at 11.47.15 AM.png425.76 KBcwells
#2 remove_or_document-2501639-2.patch1.24 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,532 pass(es). View

Comments

Cottser’s picture

Working on this with a dozen other people at the NH Dev Days sprint!

Cottser’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,532 pass(es). View

This is what we came up with. Have at it, testbot!

Cottser’s picture

Title: Remove or document SafeMarkup::set in drupal_check_module() » Remove SafeMarkup::set in drupal_check_module()
cwells’s picture

Without this patch, an evil module could cause XSS errors.
code and alert showing xss vulnerability

With this patch, we're much better off. So this is not just a refactor, but also safer.
code and drupal message showing escaped tags

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Very nice. Thank you for the manual testing.

I reviewed the code and everything looks above board;)

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Nice cleanup with the placeholders!

Looking at this, I'm actually thinking it's wrong for the parentheses to be outside the translatable string. Punctuation is very much a language-specific thing.

I grepped to see if this exact string was translated elsewhere such that we'd be adding work for translators:

[mandelbrot:maintainer | Thu 20:32:10] $ grep -r "Currently using" *
core/includes/install.inc:          $message .= ' (' . t('Currently using !item !version', array('!item' => $requirement['title'], '!version' => $requirement['value'])) . ')';
core/modules/system/system.install:            'description' => t('@name requires this module and version. Currently using @required_name version @version', array('@name' => $name, '@required_name' => $required_name, '@version' => $version)),

Interesting and close (and no parentheses in that one...), but not the same thing, so it has to be translated separately anyway. So I think we should move the parentheses inside the t().

I looked in drupal_check_module() to see what $message was:

 $message = SafeMarkup::escape($requirement ['description']);

Can we nuke that now that we're using an @ placeholder? See in drupal_set_message(); it does its own escaping/safe check, so the code path where only $message is used is also still safe.

Finally, I asked myself if it would be appropriate to do one t('@module_requirement_message (Currently using @required_name version @version.)'), and I actually think that would be helpful context for translators, because I can imagine cases where it might subtly affect the translation. It's probably worth confirming that that is acceptable with a multilingual contributor, but as far as I can see it's not making anything worse, and it simplifies the whole thing and reduces double sanitization. (I would like feedback on this point though.)

crowdcg’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,776 pass(es). View
1.05 KB

Per #6 I've addressed the first 2 points, however waiting for feedback on the last point from a multilingual contributor.

  1. Moved parentheses inside t()
  2. Removed the SafeMarkup::escape from $message since drupal_set_message(); escapes this
xjm’s picture

Issue tags: +D8MI
crowdcg’s picture

Working on a patch for this with the last change, then the multilingual contributor can decide if it should be included or not, but you'll have the patch either way.

crowdcg’s picture

Issue summary: View changes
FileSize
1.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,864 pass(es). View
980 bytes

This patch should implement the suggestion in #6 point three.

Finally, I asked myself if it would be appropriate to do one t('@module_requirement_message (Currently using @required_name version @version.)'), and I actually think that would be helpful context for translators, because I can imagine cases where it might subtly affect the translation...

xjm’s picture

Thanks @crowdcg. I pinged @Gábor Hojtsy to ask him whether this is an appropriate use of t() or not.

Edit: Note that #10 has the parentheses twice now, so if #10 turns out to be the better choice than #12, we'll want to fix that. And also we'll want to be sure to manually test the before-and-after.

Gábor Hojtsy’s picture

Issue tags: +language-ui
+++ b/core/includes/install.inc
@@ -980,14 +980,11 @@ function drupal_check_module($module) {
+          $message = t('@message (Currently using @item @version)', array('@message' => $requirement['description'], '@item' => $requirement['title'], '@version' => $requirement['value']));

I agree this gives better context to the generic (Currently using @item @version). I also agree maybe made more specific, eg. @requirements_message sounds fine.

xjm’s picture

Status: Needs review » Needs work

@Gábor Hojtsy, great, thanks!

So let's use a more specific placeholder label and then this is probably ready.

crowdcg’s picture

Issue summary: View changes
FileSize
201.16 KB

Example steps to test:

  1. Clean install of Drupal 8.
  2. Adjust a module install to make its requirements be FALSE, for instance in aggregator.install change $has_curl = function_exists('curl_init'); to $has_curl = !function_exists('curl_init');
  3. Now, attempt to install the Aggregator module and it will show the error as seen below.

YesCT’s picture

I tried it also, and I dont see the double parens mentioned in #11.
And I agree, adding the word version to the message helps.

crowdcg’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
200.01 KB
1.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,902 pass(es). View
919 bytes

Thanks @Gábor Hojtsy and @xjm! I've added the more specific placeholder label as well as the word "version". I realized I hadn't included that in the previous patch and it definitely makes the message make more sense. Here is a screenshot with this patch applied. Also, updated the issue summary.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

I checked the interdiff and the patch and it looks like this addresses all the concerns so far. rtbc.

crowdcg’s picture

Issue summary: View changes

  • xjm committed 5a3e3b1 on 8.0.x
    Issue #2501639 by crowdcg, Cottser, cwells, YesCT, Gábor Hojtsy: Remove...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Okay awesome! Yay for simpler code and better translator experience to boot.

This patch is a required part of a critical issue and so is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x. Thanks!

Removed unused use on commit:

diff --git a/core/includes/install.inc b/core/includes/install.inc
index a464762..31d472a 100644
--- a/core/includes/install.inc
+++ b/core/includes/install.inc
@@ -9,7 +9,6 @@
 use Symfony\Component\HttpFoundation\RedirectResponse;
 use Drupal\Component\Utility\Crypt;
 use Drupal\Component\Utility\OpCodeCache;
-use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Component\Utility\UrlHelper;
 use Drupal\Core\Extension\ExtensionDiscovery;
 use Drupal\Core\Site\Settings;

Status: Fixed » Closed (fixed)

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