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
ValidationTest::assertErrorMessages 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
- 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.
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 |
---|---|---|---|
#26 | remove_or_document-2550965-25.patch | 1.45 KB | joelpittet |
#26 | interdiff.txt | 1.08 KB | joelpittet |
Comments
Comment #2
josephdpurcell CreditAttribution: josephdpurcell commentedComment #3
josephdpurcell CreditAttribution: josephdpurcell commentedComment #4
josephdpurcell CreditAttribution: josephdpurcell commentedComment #5
akalata CreditAttribution: akalata commentedMight want to use #2501975: Determine how to update code that currently joins strings in SafeMarkup::set() to find examples of marking joined strings as safe.
Comment #6
akalata CreditAttribution: akalata commentedPostponed for real on #2505931: Remove SafeMarkup::set in ViewListBuilder. Whatever pattern we find works there can most likely be used here as well.
Comment #7
xjmActually, this is a test, so we should assert the expected output instead, similarly to #2550961: Remove or document SafeMarkup::set in UserBlocksTest::testUserLoginBlock. Thanks!
Comment #8
akalata CreditAttribution: akalata commented@xjm that might make the test difficult to maintain, since the assertErrorMessages() is a helper function that is generating the text we expect to see based on the $messages that we are testing for. We're not specifically testing the creation of the error message itself.
Comment #9
kgoel CreditAttribution: kgoel at Forum One commentedworking on this
Comment #10
kgoel CreditAttribution: kgoel at Forum One commentedComment #11
Anonymous (not verified) CreditAttribution: Anonymous commentedI don't see any issues with the code and tests pass locally.
Comment #12
akalata CreditAttribution: akalata commentedI don't think the two asserts should be within the
foreach
.If my understanding of xjm's concern is correct, we should not be passing
$links
as a variable to the assert; instead we should specify the HTML of what $links should contain.Comment #13
kgoel CreditAttribution: kgoel at Forum One commentedI think I clearly misunderstood and so going to read test class.
Comment #14
kgoel CreditAttribution: kgoel at Forum One commentedComment #16
joelpittetI think this is the exception:
The second argument is not needed, error_links is an array.
Comment #17
joelpittetLet's see if this passes, I leave the formatPlural in but removed the errors from it.
Comment #18
stefan.r CreditAttribution: stefan.r commentedunrelated change
Comment #19
dawehner.
Comment #20
stefan.r CreditAttribution: stefan.r commentedMaybe we can also find a solution that maintains the original translation string and asserts the errors appear in the right format/place (as before?)
Comment #21
stefan.r CreditAttribution: stefan.r commentedWe also still have #2550981: Remove SafeMarkup::set in FormErrorHandler::displayErrorMessages() open, should this be postponed on that issue?
Comment #22
joelpittetWas hoping to avoid translating string outside of translation tests and also adding to the SafeMarkup list by way of t(). That's why I kept that in.
Comment #23
stefan.r CreditAttribution: stefan.r commentedYes, I wasn't aware we want to avoid t() in tests anyway. There is also no need for this to be blocked on the other issue as the outpit won't change.
Per #20 we probably want to keep the implode() though and assert for the full string (message + counts appended).
Comment #24
joelpittetI'm trying to see what we'd be losing with the current patch. To me it's a space and some comma separation and order. Hmm
Comment #25
stefan.r CreditAttribution: stefan.r commentedCorrect, so let's keep testing for that :)
Comment #26
joelpittetLet's put that back and see, raw like
Comment #27
stefan.r CreditAttribution: stefan.r commentedYes that will also work ;)
For me that's RTBC if green.
Comment #28
stefan.r CreditAttribution: stefan.r commentedComment #29
xjmComment #30
ssingla6c CreditAttribution: ssingla6c commentedlast patch seems good towards double escape bugs and also removes SafeMarkup:set() call.so, it is good to be in RTBC
Comment #31
alexpottCommitted ee6c2d5 and pushed to 8.0.x. Thanks!