Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Problem/Motivation
TranslationManager::translate 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.
Remaining tasks
- Refactor 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.
Manual testing steps (for XSS and double escaping)
Do these steps both with HEAD and with the patch applied:
- Clean install of Drupal 8.
- Compare the output above 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
N/A
API changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#35 | 2555825-35.patch | 1.94 KB | stefan.r |
#30 | language-withpatch.png | 16.26 KB | stefan.r |
#28 | 2555825-27.patch | 1.33 KB | stefan.r |
#23 | 2555825-fail2.patch | 2.64 KB | stefan.r |
#22 | 2555825-fail.patch | 2.64 KB | stefan.r |
Comments
Comment #2
cilefen CreditAttribution: cilefen commentedComment #3
stefan.r CreditAttribution: stefan.r commentedComment #4
cilefen CreditAttribution: cilefen commented#3 LOL, But that was my plan! Ha ha
Comment #5
alexpottWell this will still work because we're not preventing an empty array. But it does mean extra costs and unnecessary function calls. This is the only call have no idea how to replace :(
Comment #6
dawehnerOne potential feature of the RendererContext was to be able to also cary over safe strings from one level, as long you need it. Could we expand the render context to store safe strings,
until they are used in a template?
Comment #7
catchThat was also Fabian's idea here: #2488538-28: Add SafeMarkup::remove() to free memory from marked strings when they're printed
It would get rid of the static safe list, which would be good.
I don't think there's another way around it - possibly in the future we can try to handle translation at the Twig level and not actually do any string manipulation before then - which is where https://groups.drupal.org/node/160704 was heading, but we haven't got there.
Comment #8
alexpottWell if this is the last
SafeMarkup::set()
call maybe we can just useSafeMarkup::setMultiple()
since we don't have any plans to remove that.Comment #9
stefan.r CreditAttribution: stefan.r commentedSo let's do that if it allows us to get rid of
SafeMarkup::set()
, at least that makes it explicit what we're doing.Comment #10
alexpottSafeMarkup::setMultiple is way more awkward than that :)
Comment #11
stefan.r CreditAttribution: stefan.r commentedLike such?
Comment #13
stefan.r CreditAttribution: stefan.r commentedNo..
Comment #14
heykarthikwithuComment #15
stefan.r CreditAttribution: stefan.r commentedComment #17
stefan.r CreditAttribution: stefan.r commented#10 wasn't kidding :)
Comment #19
stefan.r CreditAttribution: stefan.r commentedComment #20
alexpottThis change is incorrect - it never returns an object.
I'm not sure about these...
Comment #21
stefan.r CreditAttribution: stefan.r commentedSo the string cast solved *something*, let's see what kind of object they might have been. @alexpott mentioned maybe TranslationWrapper.
Comment #22
stefan.r CreditAttribution: stefan.r commentedwrong patch
Comment #23
stefan.r CreditAttribution: stefan.r commentedComment #27
dawehnerI'm confused, don't we cast things now?
This really sounds like an assertion for me. Its a pure developer mistake if an object is passed into it.
LOLOLOL, what a workaround :)
Comment #28
stefan.r CreditAttribution: stefan.r commentedComment #29
stefan.r CreditAttribution: stefan.r commented@dawehner these are just test patches that are supposed to fail -- #1/#2 will not be in the final patch.
So no, they won't be SafeString but they were TranslationWrappers in some cases and we're just trying to figure out where we have those being passed into t() now.
Comment #30
stefan.r CreditAttribution: stefan.r commentedActually this patch may be final, removing that t() call fixed the failures and everything is still translated:
Just for the record we used to be passing TranslationWrapper('Not specified') into t(), which wasn't necessary.
Comment #31
Wim LeersHrm, so this is just a work-around exploiting the fact that
set()
is removed, butsetMultiple()
isn't. But as #8 indicates, that's a reasonable work-around, if this really is the last/only use.But… isn't the real solution here to add
TranslatedString implements SafeStringInterface
, and use that instead?Comment #32
alexpott@Wim Leers that's going to prove very tricky at this point... because of TranslationWrapper::__toString() if that is called before render it needs to convert to string and therefore loses safe-ness. I think the removal of the safe string list for
SafeMarkup::format()
andTranslationManager::translate()
will have to be D9. Oh and we also have to deal withTranslationManager::formatPlural()
...However what we could do is document that the result of t() might be an object the implements SafeStringInterface and therefore the result of it can not be used in array keys - even though atm it can never be said object.
Comment #33
alexpottHere is an absolutely crazy idea - #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list that manages to properly remove t()'d strings from SafeMarkup...
Comment #34
Wim LeersAha! I think something like #32's first paragraph should be added to the code comments, to make future readers (and truly getting rid of
SafeMarkup
during D9) easier, because we won't have to do a lot of archaeological digging then.I don't find the second paragraph very convincing: if it's possible, and common sense says it's possible, it will be done, regardless of what the docs say.
Once that's documented, I think this is RTBC.
Comment #35
stefan.r CreditAttribution: stefan.r commentedComment #36
Wim LeersThe committer may choose to add
@todo Improve in Drupal 9.
to the comment, but besides that, this looks ready.Comment #38
catch#2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list reminds me of https://groups.drupal.org/node/160704 / #1213510: A modern t().
This is an obvious hack but I think removing SafeMarkup::set() is more important than removing SafeMarkup::setMultiple() - the very difficulty getting it to work is at least some discouragement to use it - although I think we could add a follow-up to mark SafeMarkup::setMultiple() @internal - really no contrib or custom code should ever be using that.
Committed/pushed to 8.0.x, thanks!
Comment #40
stefan.r CreditAttribution: stefan.r commentedcreated #2557893: Mark SafeMarkup::setMultiple as internal