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
theme() calls SafeMarkup::set() which is meant to be for internal use only.
Proposed resolution
- 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/2311123Identify 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:
- Not necessary, we are only adding documentation..
User interface changes
N/A
API changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#26 | interdiff-2501745-17-25.txt | 845 bytes | cmanalansan |
#25 | remove_or_document-2501745-25.patch | 950 bytes | cmanalansan |
#17 | remove_or_document-2501745-17.patch | 728 bytes | akalata |
#15 | remove_or_document-2501745-15.patch | 729 bytes | akalata |
#10 | remove_or_document-2501745-10.patch | 652 bytes | dani3lr0se |
Comments
Comment #1
dani3lr0se CreditAttribution: dani3lr0se as a volunteer commentedMy very first patch! Hopefully this helps.
Comment #2
dani3lr0se CreditAttribution: dani3lr0se as a volunteer commentedComment #3
dani3lr0se CreditAttribution: dani3lr0se as a volunteer commentedComment #4
star-szrThanks @Daniel_Rose!
Maybe this could be something along the lines of:
"Theme functions do not go through Twig so the output is not autoescaped. Theme function output is always expected to be 100% safe."
Undo these changes :)
Comment #5
dani3lr0se CreditAttribution: dani3lr0se as a volunteer commentedThanks for the feedback @Cottser! How does this look?
Comment #6
dani3lr0se CreditAttribution: dani3lr0se as a volunteer commentedComment #7
dani3lr0se CreditAttribution: dani3lr0se as a volunteer commentedOops. My apologies. I realized that I didn't actually undo all of those changes in #2 in your comment @Cottser. Hopefully this looks a little better.
Comment #8
ultimikeLooks ready to go.
-mike
Comment #9
star-szrI think the comment should live right above the SafeMarkup::set(), like the other issues have done.
Comment #10
dani3lr0se CreditAttribution: dani3lr0se as a volunteer commentedHow's this look?
Comment #11
dani3lr0se CreditAttribution: dani3lr0se as a volunteer commentedComment #12
akalata CreditAttribution: akalata as a volunteer commentedThe patch itself looks fine, though I have a slight issue with the wording of the comment -- I'd prefer the "is presumed to be safe" over the "is always expected to be safe", unless there is a test somewhere that confirms the expectation? Are there internal requirements that any theme function is guaranteed to be safe, or are we only presuming that this will be the case?
Just thinking in terms of the precision of uncertainty -- will keep as Needs Review and ping a theme system expert to review.
Comment #13
star-szr@akalata very good point I would tend to agree the wording could be more precise.
Comment #14
akalata CreditAttribution: akalata as a volunteer commentedComment #15
akalata CreditAttribution: akalata as a volunteer commentedI've updated our language a bit, which hopefully more clearly indicates the responsibility of the writer of the theme function (since the ones left in core are few and far between). I'm not sure if this is a documented standard or best practice, but it probably should be (even if my proposed standard is completely wrong). Quoting dawehner in IRC: "we output the string here, so we should sanitize it here".
Comment #16
davidhernandezI believe we use autoescape as one work, not with a hyphen. Otherwise, looks fine.
Comment #17
akalata CreditAttribution: akalata as a volunteer commentedComment #18
davidhernandezComment #19
dani3lr0se CreditAttribution: dani3lr0se as a volunteer commentedI like it, but would it make more sense to say:
//Theme functions do not render via the "Twig template engine"....Instead of just saying "the theme engine.."
Especially since the "PHPTemplate" engine is still in the engines folder?
Comment #20
davidhernandez"theme engine" is more generic. Theme functions would not render using phptemplate either.
Comment #21
dani3lr0se CreditAttribution: dani3lr0se as a volunteer commentedThat makes sense. I was thinking that might also be the case, but wasn't 100% sure. I've been reading through every document and resource I could find about theme functions via Twig, etc., and a lot of it made sense, but some seemed contradictory. This stuff is awesome. Much to learn though.
Patch looks good to go then. Thanks all for the feedback and help! What would be the next step in this process? Do we need more reviewing and testing?
Comment #22
xjmThanks folks for the patch! I think the current wording is good. However, I actually feel like we should be trying to remove this call.
@alexpott suggested postponing this on #2506581: Remove SafeMarkup::set() from Renderer::doRender, which is the only place that uses
theme()
directly. So doing that now.Comment #23
lauriiiUnpostoponing
Comment #24
dawehnerSo yeah I think the proper way is to return a safe string. This also removes a bit of the memory overhead we do have.
Comment #25
cmanalansan CreditAttribution: cmanalansan commentedRemoved SafeMarkup::set(), and added additional explanation:
Comment #26
cmanalansan CreditAttribution: cmanalansan commented@davidhernandez (yelling at someone else):
Comment #28
cmanalansan CreditAttribution: cmanalansan commentedWe are going to fix this in #2501931: Remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender, so I am marking as duplicate.