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.
Steps to reproduce
- Create a new view for "Log entries"
- Edit the "Watchdog: WID" field (there by default), check "Format plural" and set "Plural form" to something like
@count &
(i.e., containing HTML special characters). - The preview will now show the entered plural form double-escaped (e.g.,
523 &
).
To me, it seems the actual problem is in \Drupal\views\Plugin\views\HandlerBase::sanitizeValue()
, which I think should just return MarkupInterface
unaltered (since they usually have been sanitized already), at least for the default options. (Others should probably retrieve the markup content in unencoded form, if possible, and then do their thing.)
However, changing such an often-used method seems pretty "dangerous", so maybe this should be fixed locally in NumericField
after all.
Comment | File | Size | Author |
---|---|---|---|
#28 | drupal-views-2649488-sanitize-value-23.patch | 2.82 KB | smustgrave |
#26 | 2649488-26-test-only.patch | 2.07 KB | ravi.shankar |
#23 | drupal-views-2649488-sanitize-value-23.patch | 2.82 KB | ershov.andrey |
| |||
#13 | interdiff-10-13.txt | 739 bytes | gaurav.kapoor |
#13 | drupal-views-2649488-sanitize-value-13.patch | 3.68 KB | gaurav.kapoor |
Comments
Comment #2
drunken monkeyBoth the attached patches fix the problem for me. They implement my two suggested solutions above.
Comment #6
dawehnerGood observation! I fear we need some tests here as well :(
Comment #7
cburschkaDX-wise, I would lean toward the first option. MarkupInterface is supposed to skip further filtering and escaping. If handlers call sanitizeValue categorically and let that function handle safe markup, there is less room for vulnerabilities than if each handler uses its own logic to decide whether or not sanitization is needed.
Comment #8
cburschkaOne point: ::sanitizeValue() declares its return type to be ViewsRenderPipelineMarkup, not MarkupInterface. Strictly speaking, this means a received MarkupInterface needs to be wrapped in ViewsRenderPipelineMarkup::create() (which will automatically cast it to string first, so that's okay).
Edit: Wait no; MarkupTrait::create() leaves existing MarkupInterface instances alone. That means we can't (and maybe shouldn't) guarantee the specific return type, and it should probably be changed to @return MarkupInterface.
Comment #9
cburschkaHere's a unit test that should cover pretty much all the cases.
Comment #10
cburschkaFixed a linter error.
Comment #11
dawehnerNice test coverage!
Comment #12
alexpottThis looks wrong. It looks like we should be sanitizing the value before doing the translation. Something like this...
Comment #13
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedComment #23
ershov.andrey CreditAttribution: ershov.andrey at Evolving Web commentedUpdate patch #10 to be aligned with the Drupal 9.5.x-dev.
It looks like patch #13 is doing extra work which is not required for the proper work of field.
Comment #24
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedPatch #23 is green so I think it's ready for review.
Comment #25
smustgrave CreditAttribution: smustgrave at Mobomo commentedAppears tests have been added. But could use a test-only patch to verify.
Comment #26
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded test only patch as per comment #25, lets hope this will fail.
Comment #28
smustgrave CreditAttribution: smustgrave at Mobomo commented@ravi.shankar thank you for the tests-only patch. FYI should also upload a copy of the full patch 2nd so we can see the red-green. Also so the bot won't keep moving back to NW.
Reuploaded patch #23
Comment #29
quietone CreditAttribution: quietone at PreviousNext commentedBetter title.
Comment #30
alexpottThis doesn't feel right. It feels dangerous. I think we should change the fix to be something more like #12 and fix the problem at source.
Comment #31
alexpottAlso this code:
Looks really really odd - joining the strings together like this loses the MarkupInterface wrapper anyway... so maybe that's why the double escape is happening.
Potentially the fix here is to do:
or it might be better to do:
Comment #33
FeyP CreditAttribution: FeyP as a volunteer and at werk21 commentedAdding a related issue where a similar fix to #28 was suggested by @lendude. Depending on the fix here, that might then be a potential duplicate issue.
W/r/t #30, if used correctly, MarkupInterface should be used for safe strings only, so in theory it shouldn't be dangerous to just return the value as is. In fact, ViewsRenderPipelineMarkup already has similar code, it just isn't useful here since the (double) escaping happens before the call to ViewsRenderPipelineMarkup.