Problem/Motivation
Views calls SafeMarkup::set()
when rendering multiple fields, like on entity fields.
Proposed resolution
Use an inline template to split things up.
Now that we have an interface for handlers, let's improve things at the same time and add an interface for field plugins and field plugins which render mulitple items
at the same time. This moves a bunch of really ugly code in the context of multiple field rendering.
Remaining tasks
User interface changes
API changes
Original report by @chx
The parent made sure FieldPluginBase::advancedRender
"returns" safe markup by adding $this->last_render = SafeMarkup::create($this->last_render);
. That is not a fix. That's a kludge. Resolve it.
Beta phase evaluation
Issue category | Task, because its part of the meta to remove Safemarkup::set() |
---|
Comment | File | Size | Author |
---|---|---|---|
#25 | interdiff.txt | 1.34 KB | dawehner |
#25 | 2280961-25.patch | 23.42 KB | dawehner |
#20 | views-2280961-20.patch | 23.12 KB | LinL |
#19 | views-2280961-19.patch | 23.11 KB | martin107 |
#16 | interdiff.txt | 1.12 KB | dawehner |
Comments
Comment #1
chx CreditAttribution: chx commentedComment #2
seantwalshComment #3
xjmComment #4
xjmComment #5
Anonymous (not verified) CreditAttribution: Anonymous commentedlooking at this today at tcdrupal.
Comment #6
xjmComment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedi see two approaches we could take here:
- remove
$this->last_render = SafeMarkup::create($this->last_render);
, see what breaks, and fix it. this is the piecemeal approach- fix views to use not render early, and instead have it use a 'normal' Twig pipeline, so
$this->last_render = SafeMarkup::create($this->last_render);
becomes unnecessary. this is the fix views approachi don't have a feel for which of those is better at this point. i think the piecemeal approach may be better from a 'release D8 soon' point of view, but is ugly. the fix views approach seems better from code point of view, but may take longer.
so, views people - which way should we go?
Comment #8
jibranTagging.
Comment #9
effulgentsia CreditAttribution: effulgentsia commentedWhile I agree that this will be a very nice thing to fix, I don't see what makes this a regression from D7, so does it need to be critical? Downgrading in case it's not, but please re-raise to critical if there's something I'm missing about what makes this one so important.
Comment #10
chx CreditAttribution: chx commentedWe mark everything rendered via calls from FieldPluginBase::advanced_render as safe even those things that are not. Are you sure that's not a problem? It's not a regression from D7 but it behaves differently from the rest of D8 and as such it's quite dangerous.
Comment #11
dawehnerWell, we sanitize the stuff while its rendered:
which by default does as String::checkPlain(), so why do you consider it to not be safe?
Comment #12
chx CreditAttribution: chx commentedWell, render is nice and all, but we are talking of advancedRender and that calls renderItems and I see tons of SafeMarkup calls in Field::renderItems ; and I am unable to wrap my head around the safeness of FieldPluginBase::renderText either.
Comment #13
dawehnerI really try to understand what would be needed here, to do things properly.
If we manage to use twig templates to render anything (like construction the link created in advancedRender()) we would be fine?
Would that imply that we would drop the checkPlain sanitzation in there but just keep the xss one?
@chx
It would be great if you could provide more general overview/context. Would like to solve the problem with you together.
Comment #14
dawehnershould be.
Comment #16
dawehnerOkay I was stupid.
Comment #17
Fabianx CreditAttribution: Fabianx commentedI think we should keep the:
$separator = Xss::filterAdmin($this->options['separator']);
Unless we sometime added filterAdmin to safe_join twig filter, which is a pretty brilliant idea actually.
I think the message in the comment needs updating.
While this implements a really nice interface and probably works great, it does not remove the line stated in the issue summary.
The conversion to use inline template looks good to me however so far :). Thanks, Daniel!
Would it still work with that line removed?
Comment #18
jibranComment #19
martin107 CreditAttribution: martin107 commentedReroll
One conflict resolved, Field now implements an interface inherited from HEAD, and one inherited from this patch.
Given the age of last patch ... I am not confident that this will pass all tests.
That site-install works is all I can guarantee. testbot will give the definitive answer.
Comment #20
LinL CreditAttribution: LinL commentedReroll. Patch no longer applied due to changes in
function theme(ResultRow $values)
doc block incore/modules/views/src/Plugin/views/field/FieldPluginBase.php
Comment #21
dawehnerCool, that we have a gren patch. Let's update the issue summary.
Comment #22
Fabianx CreditAttribution: Fabianx commentedPlease use SafeMarkup::filterXSS() here to ensure the separator is either safe already OR filtered via Xss::filterAdmin().
Comment #23
dawehnerTo be honest I would have expected that safe_join takes care about that for you.
Can we change the internal implementation of it?
Comment #24
Fabianx CreditAttribution: Fabianx commentedYes, we should for sure, I opened #2383009: Create SafeMarkup::filterSeparator and make safe_join safe by default for that.
Comment #25
dawehnerAlright, let's do that @Fabianx suggested.
Comment #26
mikeker CreditAttribution: mikeker commentedThere is a note in FieldPluginBase.php (line 1163):
But that isn't addressed by this patch.
Comment #27
dawehnerMh, this is true.
For now this patch "just" solves the multi-render
SafeMarkup::set()
. That call you outlined is though much more difficultas it results in tokens being used everywhere.
Comment #28
dawehnerI'm not really sure how to fix the SafeMarkup call mentioned in #26
Maybe someone could spin of the patch in #25 into a separate issue, get just that parts in, so this issue would be just about a single one?
Comment #29
dawehnerOpened up #2397727: Remove the SafeMarkup::set() call in field/Field.php to get at least the current progress in. The SafeMarkup::set() call mentioned in
#26 requires a total different approach and is less easy.
Comment #30
effulgentsia CreditAttribution: effulgentsia commentedI discussed this issue on a critical triage call with @xjm, @catch, @alexpott, and @webchick. The branch maintainers decided that this issue isn't critical on the same grounds that #2273923-193: Remove html => TRUE option from l() and link generator. was reprioritized: that this issue hardens security, but doesn't fix a known vulnerability.
That doesn't qualify as a known vulnerability. Furthermore, if a vulnerability exists but requires "Administer views" permission, that wouldn't even qualify as a critical bug, given that that permission is already marked with the 'restrict access' flag.
However, it's possible that a real XSS vulnerability does exist here. So, if someone discovers that, then please do the following:
Note that Major does not mean "don't fix it". We won't hold up an 8.0.0 release on it, but this would be a very nice thing to fix.
Comment #31
effulgentsia CreditAttribution: effulgentsia commentedAlso, recategorizing this as a bug, because calling SafeMarkup::set() on something that you do not know to be safe is using the API incorrectly. Since what Views calls that on is the result of some other function, those functions should be the ones to mark things safe based on whatever knowledge they have. There's a similar Major bug like that in #2399037: String::format() marks a resulting string as safe even when passed an unsafe passthrough argument which I'd love reviews on :)
Comment #32
dawehner@effulgentsia
Just a general question, should views call out to
Xss::filter()
or proxy it throughSafeMarkup
.Is there also a reason why we don't have
SafeMarkup::xssFilter
?Comment #33
YesCT CreditAttribution: YesCT commented#2343669-12: Remove _l() and _url() says it is blocked on this, but I dont see how.
Will this remove the _l() usages from FieldPluginBase?
Comment #34
dawehnerIts one of the steps to clean this mess up in order to be able to remove the
_l()
call at some point.Comment #35
webchickWould you be able to clarify that? I don't quite understand how this issue blocks #2410499: Remove remaining _l() calls in Views.
Comment #36
dawehnerSo it does not block it anymore, given that the other one is fixed implicit already.
We have the inline rendering already, I'm not sure whether this patch still needs additional action.
Comment #37
mpdonadioThis no longer applies. I'll re-reroll it tonight.
Comment #38
mpdonadioI think I misunderstood #36. Since #2397727: Remove the SafeMarkup::set() call in field/Field.php landed, the reroll of #25 is essentially an empty patch...
Comment #39
dawehner@mpdonadio
So we can mark this issue as fixed?
Comment #40
mpdonadio@dawehner, I have no idea. #26 isn't addressed and the @todo is still in the code. Do we still need to do that in this issue?
Comment #41
xjmComment #42
kgoel CreditAttribution: kgoel at Forum One commentedI was looking into outstanding issues for #2280965. This issue has already been fixed in https://www.drupal.org/node/2397727 but there is two outstanding SafeMarkup::set call in FieldPluginBase.php
and
Read https://www.drupal.org/node/2280961#comment-9412899 and https://www.drupal.org/node/2280961#comment-9455329, I am going to close this issue as a duplicate of https://www.drupal.org/node/2397727. I have created a followup for removing SafeMarkup::set in FieldPluginBase.php https://www.drupal.org/node/2505679
Comment #43
davidhernandezComment #44
mpdonadio