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
General question: Given now that safe string is used by a module and not only the internal systems, I'm curious whether the @internal flag on SafeString can still be justified as it is at the moment
Proposed resolution
Improve the @internal
documentation to explain when a module should provide its own implementation of SafeStringInterface objects and that they should be marked @internal
as well.
Remaining tasks
- Review the patch.
User interface changes
None.
API changes
None.
Data model changes
None.
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Create a patch | Instructions | Yes | |
Update the issue summary noting if allowed during the beta | Instructions | ||
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions |
Beta phase evaluation
Issue category | Task because using @internal object works it just looks bad |
---|---|
Issue priority | Major because a blocker for #2501931: Remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender |
Prioritized changes | The main goal of this issue is security. By using SafeString outside the intended context we give contrib and custom a bad example to follow. |
Disruption | Not disruptive |
Comment | File | Size | Author |
---|---|---|---|
#26 | 2544684.26.patch | 9.26 KB | alexpott |
#26 | 22-26-interdiff.txt | 1.52 KB | alexpott |
#22 | 2544684.22.patch | 9.14 KB | alexpott |
#22 | 17-22-interdiff.txt | 7.26 KB | alexpott |
#17 | 2544684.17.patch | 5.49 KB | alexpott |
Comments
Comment #1
cilefen CreditAttribution: cilefen commentedThis is suitably novice.
Comment #2
Pravin Ajaaz CreditAttribution: Pravin Ajaaz commentedRemoved @internal tag.
Comment #3
Pravin Ajaaz CreditAttribution: Pravin Ajaaz commentedComment #4
cilefen CreditAttribution: cilefen commentedComment #5
xjmComment #6
xjmI spoke with @alexpott about this issue. I don't think this is a good idea.
SafeStringInterface
should be treated as internal for the same reasonSafeMarkup::set()
is: it declares an arbitrary string as safe, rather than ensuring that the string is safe. While it's true that it has fewer side effects thanSafeMarkup::set()
, it still allows a string to skip sanitization, circumventing Twig autoescape and/or the markup filtering in the renderer, and is therefore not a good pattern for modules to use.That doesn't mean I know what to do about #2501931: Remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender though. :P
Comment #7
alexpottI'm inclined to agree with @xjm. I think the usage in #2501931: Remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender actually falls under the current documentation which is:
Views is intercepting the render system and doing it's own funky stuff. Therefore it needs to preserve safeness whilst it does this.
Comment #8
alexpottDrats... we have other issues wanting to use it. See #2547741: Introduce FilteredString and get rid of all SafeMarkup::set() calls in the Filter module
Comment #9
alexpottThe basic problem is this...
While that is true modules still need to be able to mark something as safe when they know something is safe and don't want Twig or the render system to touch it. How are they supposed to do that?
Comment #10
dawehnerWhat about making that more clear by renaming
SafeString::create()
toSafeString::createFromSafeString()
(I agree that is super confusing but what about something similar?)
Comment #11
Wim Leersalso applies to #2547741: Introduce FilteredString and get rid of all SafeMarkup::set() calls in the Filter module.
It's 1) a filter (which means the output of the filter is going to be assumed to be safe anyway), 2) using the render system as part of its filtering (because it wants to replace a wrap a certain piece of markup in a themeable piece of markup, i.e. it needs the render/theme system).
See #2547741-22: Introduce FilteredString and get rid of all SafeMarkup::set() calls in the Filter module for more details about why this is ok.
In the particular case of #2547741, it could be argued that we need something like
FilteredString
for those filters that want/need to use the render/theme system (otherwise Twig would auto-escape my already-filtered string and… that'd break things). But then the question is: is it really worth having another thing that is very much likeSafeString
? I think semantically speaking, the answer is . But then the discoverability of that becomes a problem. Although there we could argue that it should just be explicitly documented in the filter system docs, for example — or something like that.Similarly, I think (this is based on a very quick skim of #2501931, I could be wrong here) Views could have a
ViewsRenderPipelineSafeString implements SafeStringInterface
value object. Because that's the real reason that Views needs to deal with this at all: because it has its own render pipeline. (For good reasons, render arrays are only for HTML, Views also wants to generate JSON etc.)Comment #12
alexpottI quite like the proposal @Wim Leers's makes in #11 because then it is clearly the responsibility of each module. Each concrete class should be @internal to the module. So I guess we could document this on the SafeStringInterface?
Comment #13
Wim Leers#12: sounds good (
@internal
to the module, plus documenting onSafeStringInterface
).Comment #14
dawehnerI agree with #11., let's make doing the right thing easy and make it harder if you are doing something more complex.
This would then basically mean that we mark the issue as won't fix, right and create subissues for the various subsystems with the requirement of such an object.
Comment #15
Wim LeersWouldn't it be better to do it here, where we already have the entire discussion/reasoning that led to this?
Comment #16
alexpottI think we can add the documentation on SafeStringInterface here. And the new objects in issues that require them.
Comment #17
alexpottWell Views was already using SafeString so did the conversion here.
Comment #18
alexpottComment #19
alexpottComment #20
dawehnerI would have made SafeString a trait and use it here
Comment #21
Wim LeersIMO this looks great. Just some nits below.
I just have one bigger remark: I'm not sure I entirely like the "interrupt the render pipeline" explanation. I don't think Views "interrupts" the render pipeline, nor does Filter. I think both have their own render pipeline, but for integration purposes, they happen to choose to also use the render/theme system. Which means data is flowing from their own render pipeline into the regular render pipeline, and because they do that, they need to deal with this, otherwise the input they send into the regular render pipeline would be auto-escaped.
I guess "interrupt" is a very terse way of saying
regular render pipeline -> CUSTOM RENDER PIPELINE ( -> reuses regular render pipeline for bits and pieces) -> regular render pipeline
— because in the end, the output of Filter and (some of) the output of Views make it back into the regular render pipeline. But that's kind of a detail here; what matters most is that they reuse the regular render pipeline.(Which is why the only problematic case in Filter is
FilterCaption
, which is the only filter calling the render/theme system.)"for fast rendering of fields" -> I don't think this is the main reason, I think the main reason is that Views also wants to render JSON etc.
I'll leave this to @dawehner.
s/during/in/
?
Comment #22
alexpottThanks @Wim Leers and @dawehner.
Comment #23
dawehnerShould we point to the trait from the Interface, maybe an @see ?
This is a odd line ...
Comment #24
Wim LeersThis totally makes sense, because it's replacing Views' "post-render tokens", which are most definitely a Views render pipeline-specific concept.
RTBC.
Nit, can be fixed on commit: "into it. For" still fits on the previous line.
Comment #25
Wim LeersCross-posted.
Comment #26
alexpottThanks @dawehner and @Wim Leers. Feedback addressed.
Comment #27
Wim LeersComment #28
catchComment #29
catchSo I feel like the views render pipeline usage is something that we could eventually at some point factor out -we've already done that for a lot of Views's custom rendering. Seems odd to add the trait and docs for one very unique use case.
However in irc alexpott pointed me to FilterCaption, which while not changed in this patch yet shows there's two use-cases in core. Issue for that is #2547741: Introduce FilteredString and get rid of all SafeMarkup::set() calls in the Filter module.
Given two use cases, and one of them being a filter which is not an uncommon case, this makes sense then after all.
Committed/pushed to 8.0.x, thanks!
Comment #31
xjmComment #32
webflo CreditAttribution: webflo at UEBERBIT GmbH commentedI think this patch introduces a bug related to result groups. Created #2553035: Views grouping in results is broken because of SafeString as follow-up.