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
\Drupal\views_ui\ViewUI::renderPreview() 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
Comments
Comment #1
star-szrThis one is up for grabs.
Comment #2
leslieg CreditAttribution: leslieg as a volunteer commentedworking on this as part of NHDevDays2
Comment #3
leslieg CreditAttribution: leslieg as a volunteer commentedchanged SafeMarkup::set to SafeMarkup::format in 4 places in renderpreview()
Comment #4
dawehnerI'm sorry but this is kinda broken. Feel free to compare the two screenshots
Comment #5
dawehnerIn order to reproduce use
drush vd
in your drupal root and then preview a view.Comment #6
leslieg CreditAttribution: leslieg as a volunteer commentedfixed formatting issue identified in testing
Comment #7
dawehnerThank you!
For easier additional reviews it is nice to provide an interdiff, see https://www.drupal.org/documentation/git/interdiff
Comment #8
xjmThanks @leslieg and thanks @dawehner for the manual testing and screenshots. (Adding dawehner to the proposed issue credit.)
Looking at the code here, I think this markup should be added to render arrays instead of using
SafeMarkup::format()
to get it in there. There's already so much render array goodness being built in ViewUI::renderPreview(). Let's build it once and avoid early and nested sanitization, duplicatedSafeMarkup
entries, and unalterable markup.Edit: I mean if you don't use the render API in a method called
renderPreview()
, where can you? :)Comment #9
kgoel CreditAttribution: kgoel at Forum One commentedworking on it
Comment #10
kgoel CreditAttribution: kgoel at Forum One commentedComment #11
kgoel CreditAttribution: kgoel at Forum One commentedOoops...patch had wrong file, fixed it
I have added markup to render arrays as suggested in #8 and removed SafeMarkup::format from the patch
Comment #13
seantwalshReviewed the patch and tested on a clean branch of HEAD and then with the patch that refactored the code per #8, screenshots below. All the concerns in that comment have been addressed and now use render arrays instead of SafeMarkup::format().
In order to test it I first enabled the following Views Live Preview Settings (Show the SQL query; Show performance statistics; Show other queries run during render during live preview). Then I previewed several views, including Content and Taxonomy as depicted in the screenshots. In each case the results visually and in the markup were identical.
HEAD:
PATCH 11:
Comment #14
xjmNice work @kgoel! This is much cleaner now. Thanks also @crowdcg for the manual testing.
This issue is a required part of resolving a critical issue, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. Committed and pushed to 8.0.x.
Removed the now unused use statement on commit: