Follow-up to #2042143: [META] Fix unsafe core translatable strings
Problem/Motivation
The locale_string_is_safe
function is used by the Locale module to ensure that translations are safe before saving them in the database. It does so by comparing an Xss::filter-ed version to the original version so nothing serious can happen.
The problem is that some translatable strings could include tokens being wrongly filtered by the Xss::filter method.
For example, the locale module contains the following string that is not translatable because of the token inside :
This page allows you to translate the user interface or modify existing translations. If you have installed your site initially in English, you must first add another language on the <a href="[site:url]/admin/config/regional/language">Languages page</a>, in order to use this page.
When filtered, <a href="[site:url]/admin/config/regional/language">
becomes <a href="url]/admin/config/regional/language">
so the locale_string_is_safe
function fails.
Beta phase evaluation
Issue category | Bug because it doesn't allow to translate a string if it has tokens in it. |
---|---|
Issue priority | Not critical because it only affect the translation of strings. |
Prioritized changes | This is a prioritized change, because it's a bug. The issue is tagged as security, as it's related to translation's security, but this is not a security vulnerability itself. |
Disruption | Not disruptive for core/contributed and custom modules/themes because the affected function is not widely used. Only on the translation section of admin. Contrib and custom modules shouldn't use it directly. |
Proposed resolution
As seen with dawehner and chx, we should remove really safe tokens from the string before passing it to Xss::filter.
We chose this option as it is far easier and safer than trying to change Xss::filter. A specific issue has been opened to keep a track of this problem. See #2372127: Prevent Xss::filter from stripping a token at the beginning of an html attribute.
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Create a patch | Instructions | Done in #2 | |
Add automated tests | Instructions | Done in #2 | |
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions | ||
Review patch to ensure that it does not introduce a security issue |
User interface changes
None
API changes
None
Comments
Comment #1
DuaelFrComment #2
DuaelFrComment #4
YesCT CreditAttribution: YesCT commentedhow do we know this is the pattern that would only match safe things?
Comment #5
YesCT CreditAttribution: YesCT commentednit, needs to wrap at 80 chars.
Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over
https://www.drupal.org/coding-standards/docs#drupal
probably a copy and paste error. should match the name of the file.
this seems more limited in scope given the file name.
I'm not sure what is more usual, to {@inheritdoc} on this property, or just regular doc.
did a quick guess/check:
ag " extends KernelTestBase" -C7 | grep inheritdoc | wc -l
is 9
ag " extends KernelTestBase" -C7 | grep "Modules to enable" | wc -l
is 30
so.. not clear and not blocking this issue either way.
fixed those little things.
this still needs a review.
Comment #6
YesCT CreditAttribution: YesCT commentedoops forgot the first thing. here it is.
Comment #7
YesCT CreditAttribution: YesCT commentedmentioned in the hallway, but documenting feedback here.
1.
we should be more specific in the comments about what trouble is.
let's talk about why they are not translatable, etc.
2.
Also, I think we should open an issue to fix this in Xss::filter, and add @todo comment linking to that issue.
3.
also, in the summary of this issue, we should explain why we are doing this fix before the call to Xss:filter and why we are not just fixing it there directly.
...
4.
and, I wonder if this pattern is too liberal. maybe there are some strings we should not remove before calling Xss::filter. dawehner mentioned something about javascript: ... ??
Comment #8
dawehner... we should describe here what the troubles are. It should also kinda describe that replacing tokens is not a security problem ... and why.
Comment #9
DuaelFr1.
Done. I hope my english is not as confuse as I think it is.
2.
Done #2372127: Prevent Xss::filter from stripping a token at the beginning of an html attribute
3.
Done.
4.
I tried to give an explanation in the comment. Tell me if it seems good for you.
Comment #10
DuaelFrComment #11
YesCT CreditAttribution: YesCT commentedSat with @DuaelFr to reword the comment.
This is now ready for someone to check the whole thing and see if the approach and the docs are good.
Comment #12
dawehnerIt would be great to give some examples ...
I would love to see a dedicated test which ensures that t and then token replace passed through a template does NOT call a security issue. Try to use a typical
<script>alert('moeeeep');</script>
in there.Comment #13
DuaelFr@dawehner Happy ? ;)
More seriously, I'm not sure about the way I made that new test. Can you have a look?
Comment #14
DuaelFrSome improvements:
- test made more readable
- comments fixed
Comment #15
YesCT CreditAttribution: YesCT commentedlittle rewording and newline.
this test is easier for me to understand than #13. Very nice.
Comment #17
DuaelFrFixes the issue in the test introduced by the newline at the end of the template.
Comment #20
DuaelFrJust changed the regexp separators to be more consistent with other regexp in core.
Comment #21
dawehnerFor me the test coverage looks fairly reasonable and the regex limits quite good.
On the other hand this should have a dedicated review from someone from the security review and maybe gabor itself.
Comment #22
Gábor HojtsyI don't understand the problem itself. Why do we use tokens in those strings and not @url with a t() replacement later?! Think of our poor translators who need to understand each different format we make up for source strings and which parts they are supposed to touch / not touch.
Comment #23
DuaelFr@Gábor Hojtsy:
First, these strings are in a yml file used to configure the Tour module so I don't know how we could process the placeholders as the t() function call is automated. Should we introduce a new configuration concept the add placeholders in translatable strings? It could be really complicated.
Then, we might not forbid contrib developpers to include tokens in their translatable strings. So, to keep these strings translatable, we need to fix this issue (or the related one dealing with Xss::filter).
Comment #24
Gábor HojtsyOk I can see that tour module wants to use tokens instead of exposing its own translation-standard replacement patterns like @url to be used in the string. That makes sense if a whole set of tokens are supported in an extensible way. Is that the case? Where is that evaluated?
Comment #25
DuaelFrThe code replacing tokens in the body string is
where
$this->token
is a\Drupal\Core\Utility\Token
instance.According to the
Token::replace()
method's documentation, as the second parameter has been left empty, only the tokens that are not related to a particular context might be available. In the Core, that's the case of the following tokens :The thing is that, even if we could try to find another solution, we have to fix the issue at least to avoid contributed modules that would like to use a token in a translatable string to become untranslatables.
In D7 a common case was to transform account creation emails to use an HTML format then try to translate them. As it has been converted to Configuration Translation in D8 that is not a problem anymore but we may encounter that issue otherwise.
Comment #26
Gábor HojtsyAll right, let's fix this then. Thanks for the background info. I am sad different replacement styles are used but we seem to be putting developers ahead of translators again here which we did so many times in D8.
Comment #27
tuchoComment #28
tuchoComment #29
tuchoI have made a little improvement to the RegEx that removes the tokens from the string.
The original RegEx matches strings that are not valid tokens, like the following:
Comment #30
DuaelFrIt was made on purpose. If we wanted to match all possible tokens, that RegEx should have been far more complicated as about every character are allowed in a token except colon and closing square brackets. You patch is still valid and still answer the need, though.
Comment #31
YesCT CreditAttribution: YesCT commentedhere is a tests only to see if this fails in the way we want it to.
Comment #33
DuaelFrPatches rerolled and new coding standards applied to the new code.
Comment #35
DuaelFrI'm becoming tired of this issue. I makes half of Commerce untranslatable and a lot of other modules that relies on tokens.
What's missing to make it move forward, please?
(The patch still applies)
Comment #37
Gábor HojtsyUploading again for testbot.
Comment #38
Wim LeersI know you're talking about the general token problem. But, for URLs specifically, you should use
internal:/admin/config/regional/languages
.Comment #39
DuaelFr@Wim Leers I can see 3 occurences of this string, all in the tour module config files. Drupal Commerce 7.x contains loads of these strings too. All unstranslatables.
Comment #42
DuaelFrThat failure seems to be caused by Twig autoescaping.
That patch fixes the tests.
Comment #45
DuaelFrOnce again. I'm not mastering Twig escaping yet :/
Comment #47
Gábor HojtsyThere are 5 affected strings in Drupal 8 core that are not translatable due to this bug. https://localize.drupal.org/translate/languages/hu/translate?project=dru...
Comment #48
andypostClosed as duplicate #2640886: Unable to import translation, HTML restriction
Testing manually a patch it works fine, also covered with tests
Comment #50
catchThe patch is straightforward enough but the title kept confusing me.
Comment #51
catchCommitted/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!
Comment #54
Gábor HojtsyAmazing, thanks all! Should now be backported to Drupal 7 though.
Comment #55
pietmarcus CreditAttribution: pietmarcus as a volunteer commentedI'll try to fix it for D7. The patch itself isn't a problem, but I've got some work to with the testscripts.
Comment #56
pietmarcus CreditAttribution: pietmarcus as a volunteer commentedI have backported the fix to Drupal 7. Since Drupal 7 doesn't use Twig and strings aren't considered safe by translating them, I didn't include the testLocalizedTokenizedString-testcase. The testLocaleStringIsSafe is included and should fail without the patch.
Please review and let me know if I missed anything or there should be more/better tests.
Comment #57
pietmarcus CreditAttribution: pietmarcus as a volunteer commentedOops, meant to post a test-only patch, but forgot to remove the fix... This one should fail.
Comment #60
pietmarcus CreditAttribution: pietmarcus as a volunteer commentedAs predicted the test only patch failed. The patch with the fix was succesfull. Please review.
Comment #61
DuaelFrThe patch in #56 can still be applied on 7.x and is working properly.
Thank you @pietmarcus for that port.
Comment #62
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedThis is an easy one.
Marking for commit.
Comment #63
stefan.r CreditAttribution: stefan.r commented+1 to this, it fixes a long-standing very pesky issue, nice work!
Comment #64
stefan.r CreditAttribution: stefan.r commentedComment #66
stefan.r CreditAttribution: stefan.r commentedCommitted and pushed to 7.x, thanks!
Comment #67
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedHere's a quick followup for some documentation fixes (hopefully a quick enough fix that it's not worth a separate issue).
The main thing I did here was remove a paragraph that I think was only relevant for Drupal 8... since we don't mark strings safe/unsafe in Drupal 7.
Comment #68
stefan.r CreditAttribution: stefan.r commentedI actually don't think that bit is relevant to Drupal 8 anymore either :)
Comment #69
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedMarking for commit (not at a computer where I can commit right now).
Comment #71
stefan.r CreditAttribution: stefan.r commentedCommitted and pushed the follow-up as well, thanks!