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.
Follow-up to #2280965: [meta] Remove every SafeMarkup::set() call
Field has a funky trait that uses SafeMarkup::set() this was documented in #2501441: Document SafeMarkup::set in AllowedTagsXssTrait::fieldFilterXss but we have new tools because of #2544684: Expand @internal documentation on SafeString and SafeStringInterface and introduce ViewsRenderPipelineSafeString and even with the SafeMarkup::set() it is not going to work because text transformations are applied to the result.
Comment | File | Size | Author |
---|---|---|---|
#37 | 2551511-2.37.patch | 22.86 KB | alexpott |
#37 | 31-37-interdiff.txt | 3.55 KB | alexpott |
#31 | 27-31-interdiff.txt | 1.79 KB | alexpott |
#31 | 2551511-2.31.patch | 21.89 KB | alexpott |
#27 | 2551511.27.patch | 20.35 KB | alexpott |
Comments
Comment #2
alexpottThe solution could do with the new render array capabilities added by #2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter
Also gonna to add test coverage for the transformations and the current broken-ness in HEAD.
Comment #3
Wim Leerss/Filter/Field/
C/P remnant.
Nit:
[]
.These are the two only places that will use #2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter. The IS made it sound like many places would use what's introduced there.
Parse error.
Comment #5
alexpott@Wim Leers is awesome.
Comment #7
JeroenTFixed fatal error in Drupal\options\Tests\OptionsFieldUITest.
Comment #9
stefan.r CreditAttribution: stefan.r commentedJust getting this green again for the moment.
edit:
and this is unused now as well
Comment #10
stefan.r CreditAttribution: stefan.r commentedAnd some nits:
Double space after
ins
Missing documentation, $this is not a type
Missing newlines
Comment #11
Wim LeersComment #12
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Material commentedAddressed the points by @stefan.r in #9 #10.
Missing documentation, $this is not a type but its getting returned as a safe String.
Comment #13
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Material commentedComment #14
Wim LeersDocs need to be improved, as #12 also said.
Unnecessary whitespace addition.
I guess these make this issue blocked on #2549791: Remove SafeMarkup::xssFilter() and provide ability to define #markup escaping strategy and what tags to filter? This can still continue in the mean time though.
caseTransform()
stuff, per #2.Comment #15
xjmComment #17
Mariano CreditAttribution: Mariano commentedLooking at this as part of the Acquia Build Week Hackathon
Comment #18
Mariano CreditAttribution: Mariano commentedAddressed comments on #14.
Comment #19
xjmThanks @Mariano!
Note that it's helpful to provide interdiffs when updating existing patches in the d.o patch-based workflow to make it easier to review the changes. I've attached one here. :)
Comment #20
Wim LeersThe gain of making the third parameter optional is very small. Let's just be explicit and require
FALSE
to be specified?This looks a bit strange. Let's do either:
or
But not this in-between variant. Either of the two would be consistent with the rest of core.
This is just asserting the same exact hardcoded list. That's not worth testing.
Perfect :)
Comment #21
Wim LeersComment #22
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedThe patch from #18 still applied.
Fixed #20, except for 3. The first tests an array, the other one a concatenated string (which uses that array). That is not the exact same thing. However, if you feel strongly about that, I'll remove it.
Comment #23
Wim LeersFair point. I don't feel very strongly about it. Let's let a committer decide then :)
Dear committer, see #20.3 — I think those are useless tests, especially
testAllowedTags()
. If you think this is useful, keep it, if you think it's not, remove them.Comment #24
alexpottI realised that FieldFilteredString::caseTransform is completely unnecessary... less code! We can just do the case transforming before the filtering.
Comment #25
alexpottImproved the
FieldFilteredStringTest
to actually test the filtering and normalisingFieldFilteredString::create()
does.And I agree with @Wim Leers -
testAllowedTags()
is pretty pointless.Comment #26
Wim LeersExcellent! Just one thing:
This
@todo
is still left and can now actually be done.(I forgot about that when RTBC'ing in #23 — sorry.)
Comment #27
alexpottAh yes... in fact here we don't need to even create the object - we can just use #allowed_tags for late filtering.
Comment #28
Wim LeersYep, exactly :)
Comment #30
Wim LeersFailing on:
This should actually be escaped AFAICT?
As should the next assertion (which is not failing because the test already failed on the above:
Comment #31
alexpottI worked off an old branch... alexpott--
Here's the correct patch.
Comment #32
Wim LeersSuch noob :P
Comment #33
stefan.r CreditAttribution: stefan.r commentedback to RTBC
Comment #34
catchThese docs are copy and pasted from the filter tips patch I think?
Needs updating, and also needs to apply to the usages across field modules and views, which makes this a bit less @internal.
Comment #35
Wim LeersHrm good point. It's still "relatively"
@internal
to the Field module, but given that it's now used by theoptions
,image
andviews
module, that's indeed less the case. It's still all places where "Field-specific stuff" is happening.Wondering what Alex Pott thinks about this.
Comment #36
alexpottSo perhaps we need to update the SafeStringInterface documentation since if you filter on create (like this implementation) it is way less dangerous and less deserve of the internal-ness.
Comment #37
alexpottSo I've tried to completely remove the need for FieldFilteredString but we got three problems:
This SafeStringInterface is way safer than the other SafeStringInterface objects because it filters on create.
Given the above I think we should proceed here with better documentation. Especially since the mixing of filtered and escaped strings in SafeMarkup is definitely asking for security issues at the worst and unexpected behaviour at the very least.
Comment #38
Wim LeersWow, good catch. Why was this not causing test failures before?
Comment #39
Wim Leersstefan_r answered #38 in IRC:
Of course. We're basically just keeping
AllowedXssTrait::fieldFilterXss()
for BC. Should we deprecate it?EDIT: deprecate for 9, of course.
Comment #40
stefan.r CreditAttribution: stefan.r commentedThe whole trait is deprecated in the patch, we could copy that @deprecated notice to all its methods if needed?
Comment #41
Wim Leers#40: oops, missed that. My bad.
Back to RTBC.
Comment #42
catchnormalize. Fixed on commit, for some definitions of fixed.