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
Follow-up to #2273925: Ensure #markup is XSS escaped in Renderer::doRender()
+++ b/core/lib/Drupal/Core/Render/Element/HtmlTag.php
@@ -94,7 +94,7 @@ public static function preRenderHtmlTag($element) {
if (!empty($element['#noscript'])) {
- $element['#markup'] = '<noscript>' . $markup . '</noscript>';
+ $element['#markup'] = SafeMarkup::format('<noscript>!markup</noscript>', array('!markup' => $markup));
}
Needs followup: the #noscript element is not documented at alllll
Proposed resolution
Document it
Remaining tasks
Do it
User interface changes
None
API changes
None
Beta phase evaluation
Issue category | Bug (adding missing documentation). |
---|---|
Issue priority | Minor, but wanted for a critical |
Unfrozen changes | Unfrozen because it only changes documentation. |
Disruption | Not disruptive. |
Comment | File | Size | Author |
---|---|---|---|
#16 | document_noscript_element-2494293-16.patch | 679 bytes | umarzaffer |
#12 | document_noscript_element-2494293-11.patch | 563 bytes | ashutoshsngh |
#7 | document_noscript_element-2494293-7.patch | 1.05 KB | umarzaffer |
#6 | document_noscript_element-2494293-6.patch | 1.05 KB | umarzaffer |
#3 | document_noscript_element-2494293-3.patch | 1.05 KB | pguillard |
Comments
Comment #1
xjmComment #2
xjmComment #3
pguillard CreditAttribution: pguillard commentedI wonder about that one:
Maybe too long, maybe not a flag..
Hope it helps anyway...
Comment #4
pguillard CreditAttribution: pguillard commentedComment #5
joelpittetWe can actually use
@
here.!
is now more meant for plain text use-cases. As long as the variable is safe it won't be escaped similar to what happens in the twig template.Regarding the comment. I think we could probably stop at this without explaining what a noscript tag is for:
What do you think?
Thank you for the patch @pguillard!
Comment #6
umarzaffer CreditAttribution: umarzaffer at Srijan | A Material+ Company commentedSorry, made a silly miss! Submitting the corrected one next
Comment #7
umarzaffer CreditAttribution: umarzaffer at Srijan | A Material+ Company commentedComment #9
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedThank you @umarzaffer for the patch.
@joelpittet, the patch #7 looks good to me.
This patch is using @ instead of !
Moving this to RTBC.
Comment #10
xjmThanks @umarzaffer and @RavindraSingh!
I think we can remove everything after the comma here since the tag is a part of the HTML spec.
The scope of the issue is just to document it; this change is out of scope. Can we remove it?
Comment #11
lokapujyaComment #12
ashutoshsngh CreditAttribution: ashutoshsngh at Srijan | A Material+ Company commentedComment #13
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedThank you @xjm, for giving quick feedback.
Thanks @ashutoshsngh for new patch and @lokapujya for changes summary.
The patch added in #12 looks what @xjm has suggested. So moving this to RTBC
Comment #14
xjmApologies for the slightly hurried review before; I think there are a couple more things we need to do here:
(optional)
at the beginning since it's not a required key.So something like:
Sorry for not catching this the first time around. :)
Comment #15
xjmUpdating the beta evaluation. It doesn't matter here, but be sure to review what's unfrozen for other issues. (Converting code to best practices is not unfrozen.) Thanks!
Comment #16
umarzaffer CreditAttribution: umarzaffer at Srijan | A Material+ Company commentedThanks @xjm for reviewing and providing suggestions. Have added the documentation for #noscript as per your suggestion.
Comment #17
pguillard CreditAttribution: pguillard commentedI have not taken the time to continue the work. I think it is fair to move to RTBC.
Comment #18
xjmJust to confirm that the docs were complete and not missing anything, I went to look for when the element was originally added. It was in #2068471: Normalize Controller/View-listener behavior with a Page object to work around something:
Those problems were addressed in two interdiffs in #190 and #207 of that issue. Those comments didn't make it clear to me at first why setting the noscript tags as prefix and suffix didn't work, and I was trying to decide if we should document that here...
\Drupal\Tests\Core\Render\Element\HtmlTagTest
has test coverage and those tests only assert that the element does exactly what we say here. :) So this is fine I think, even if it is a weird internal-use edgecase thing.Since I kind of wrote the comment in its current form via my review, I think I probably shouldn't commit this myself, so assigning to @jhodgdon in case she has a chance to take a look at it.
Comment #19
jhodgdonLooks good to me! Committed to 8.0.x. Thanks all!