Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
- Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
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.If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.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:
- Clean install of Drupal 8.
- 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');
- Now, attempt to install the Aggregator module and it will show the error as seen below.
- Compare the output in HEAD and with the patch applied. Confirm that there is no double-escaping.
- 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
Comment | File | Size | Author |
---|---|---|---|
#16 | interdiff-2501639-15.txt | 919 bytes | seantwalsh |
#16 | 2501639-15.patch | 1.31 KB | seantwalsh |
#16 | PATCH15-admin-modules-1434386095780.png | 200.01 KB | seantwalsh |
#14 | Error-admin-modules-1434385771277.png | 201.16 KB | seantwalsh |
#10 | interdiff-2501639-10.txt | 980 bytes | seantwalsh |
Comments
Comment #1
star-szrWorking on this with a dozen other people at the NH Dev Days sprint!
Comment #2
star-szrThis is what we came up with. Have at it, testbot!
Comment #3
star-szrComment #4
chrisfromredfinWithout this patch, an evil module could cause XSS errors.
With this patch, we're much better off. So this is not just a refactor, but also safer.
Comment #5
joelpittetVery nice. Thank you for the manual testing.
I reviewed the code and everything looks above board;)
Comment #6
xjmNice 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:
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: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.)Comment #7
seantwalshPer #6 I've addressed the first 2 points, however waiting for feedback on the last point from a multilingual contributor.
Comment #8
xjmComment #9
seantwalshWorking 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.
Comment #10
seantwalshThis patch should implement the suggestion in #6 point three.
Comment #11
xjmThanks @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.
Comment #12
Gábor HojtsyI agree this gives better context to the generic (Currently using @item @version). I also agree maybe made more specific, eg. @requirements_message sounds fine.
Comment #13
xjm@Gábor Hojtsy, great, thanks!
So let's use a more specific placeholder label and then this is probably ready.
Comment #14
seantwalshExample steps to test:
$has_curl = function_exists('curl_init');
to$has_curl = !function_exists('curl_init');
Comment #15
YesCT CreditAttribution: YesCT commentedI tried it also, and I dont see the double parens mentioned in #11.
And I agree, adding the word version to the message helps.
Comment #16
seantwalshThanks @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.
Comment #17
YesCT CreditAttribution: YesCT commentedI checked the interdiff and the patch and it looks like this addresses all the concerns so far. rtbc.
Comment #18
seantwalshComment #20
xjmOkay 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: