Part of #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping
Problem/Motivation
SafeMarkup::checkPlain()
is unnecessary. #options
for radio and checkboxes often needs to be escaped however this has to be done by calling code because the default in using XSS admin filtering. Using SafeMarkup::checkPlain()
is unnecessary because XSS admin filter leaves it unchanged. There is a slight performance cost but we gain the ability to remove SafeMarkup::checkPlain()
and, by extension, the entire safe list which is worth it.
Proposed resolution
Convert SafeMarkup::checkPlain()
to Html::escape()
when it's being used in conjunction with #options.
Don't try to use autoescaping features, as its super tricky.
Remaining tasks
Commit
User interface changes
None
API changes
#options is autoescaped
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#61 | 2560863-2-61.patch | 20.95 KB | alexpott |
#61 | 59-61-interdiff.txt | 4.01 KB | alexpott |
#59 | 2560863-2-59.patch | 19.99 KB | alexpott |
#54 | 2560863-54.patch | 28.92 KB | alexpott |
#54 | 49-54-interdiff.txt | 1.76 KB | alexpott |
Comments
Comment #2
alexpottComment #3
alexpottExtracted all the changes out of #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping and reviewed code paths...
Comment #5
stefan.r CreditAttribution: stefan.r commentedLooks like for the OptionsWidgetsTests, we now autoescape instead of filter, and for the entity reference ones, the labels we get from getReferenceableEntities() are now not sanitized as mentioned in the interface docs.
Comment #6
legolasboJust a quick review of the code.
Isn't it possible to prevent this code duplication?
Shouldn't these be $this->assertEscaped like in
ElementTest::testOptions
Comment #8
star-szrJust want to make sure that we don't regress #1919338: Select widget (from the options module) prone to double encoding here.
Comment #9
alexpottThe changes to this test show I've gone too far. Configurable fields values should use FieldFilterString...
Comment #10
stefan.r CreditAttribution: stefan.r commented@legolasbo we could make the labels FieldFilteredString again so the test changes are not needed (i.e. revert some of the changes in #3). You can take a stab at this and post a patch if you like?
Comment #11
legolasbo@stefan.r,
I would love to take a stab at it, but I will not have time to do so until Friday. Just reviewing some patches between work issues to clear my head.
Comment #12
alexpottFix field options widgets to be able to use their limited set of markup. And made the same fixes as #5 for the entity reference changes.
Comment #13
stefan.r CreditAttribution: stefan.r commented^
This seems a bit odd and I wonder if escaping wouldn't make more sense, but I guess we don't want to change existing behavior here.
scrip
Comment #14
alexpottThanks @stefan.r
1. The #options here will be FieldFilteredString so all good.
2. Yeah totally out-of-scope to change this (tested) behaviour
3. Fixed.
I've manually tested entity reference with markup in a node title and referencing that node and autocomplete and it works exactly the same in HEAD and with the patch. The title is escaped in the drop down. Once you select the value the unescaped version is used in the textfield.
Comment #15
stefan.r CreditAttribution: stefan.r commentedPatch looks good, this all looks fairly straightforward. For the radios/checkboxes/selects I assume we already have plenty of automated test coverage...
Comment #16
alexpottI would be nice to find (or create) some test coverage of Entity reference because we've changed tests there.
Comment #17
alexpottElaborating on #16.
Steps to test:
<em>Markup</em>
<em>Markup</em>
- in the dropdown list it should be double escaped and once selected it should not be. This is the behaviour before and after the patch.I'm not sure the behaviour is correct mind you - but the behaviour is unchanged.
Comment #18
lauriiiJust posting my progress if someone wants to give a shot on this one. I have created a base for the test but there is still one fail because for some reason the node doesn't still display the entity reference field value. I will work on this tomorrow anyway if nobody has fixed this before it.
Comment #20
stefan.r CreditAttribution: stefan.r commentedreroll
the "field_" bit here was probably breaking things?
Comment #21
stefan.r CreditAttribution: stefan.r commentedComment #24
stefan.r CreditAttribution: stefan.r commentedThe test only fail suggests either something changed in HEAD, the reroll is bad or behavior before and after the patch wasn't the same after all?
Comment #25
stefan.r CreditAttribution: stefan.r commentedComment #26
alexpott@stefan.r the patch is changing behaviour. It is making #options autoescape the values if they are not marked safe.
Comment #27
alexpottAdding tests to
EntityAutocompleteTest
to ensure that that still escapes markup and merging tests inEntityReferenceXSSTest
because installing Drupal twice is unnecessary.Comment #28
stefan.r CreditAttribution: stefan.r commentedAlso tests the entity view display
We may want to either put these back in or change the title, the title with stripped tags is merely "I am kitten" - which may or may not include the escaped or unescaped
<em>
tags.Comment #29
alexpott@stefan.r nice spot re the changed behaviour. In HEAD
OptionsSelectWidget::sanitizeLabel()
is receiving an escaped $label - it strips it (doing nothing) and then decodes it (producing well formed HTML)... which then gets to Twig... which escapes it again :) This behaviour is obviously nuts. A select list can not contain HTML - unlike radios but it is passed HTML it should just rely on twig to escape it. If the caller wants to strip the tags then that is okay too.The test only patch attached should pass showing we have no behaviour change.
Comment #31
alexpottUploading testonly patch again...
Comment #32
alexpottFixing the test fails in
OptionsWidgetsTest
which were testing for the tag stripping. What is interesting is that now the both optgroups and options are both escaped making for a more consistent and easier to predict API. I've changed the optgroup escape testing to use$this->assertEscaped()
for consistency.Comment #34
alexpottRandom fail in
Drupal\views\Tests\Update\EntityViewsDataUpdateTest
... DrupalCI passed.Comment #37
alexpottThe testonly patch has an unrelated test fail too...
Drupal\rdf\Tests\FileFieldAttributesTest
. But as you can see from the DrupalCI result is green as expected.Comment #38
stefan.r CreditAttribution: stefan.r commentedclarifying 2 bits
Comment #39
lauriiiThanks for adding the documentation for the few confusing parts. I think I'm allowed to RTBC this because I was only working on a small part of the issue.
Comment #40
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedYikes, in Drupal 7 the form API takes care of filtering these automatically, at least I'm pretty sure it does (see theme_form_element_label(),
$title = filter_xss_admin($element ['#title'])
). Does this issue mean that security feature went missing in Drupal 8? Could indicate some missing test coverage in Drupal 7 if so...For radio and checkbox labels, Drupal 7 allows HTML by default since filter_xss_admin() is used. I can see changing this in Drupal 8 since the auto-escaping is just a fallback behavior (only used if the string wasn't marked safe already) but does that mean this issue should get a change notice?
Comment #41
xjmBoth the points in #40 seem worth addressing here.
Also, while this is fine, I do worry about authors of JSON consumers being confused about when their JSON is vulnerable to XSS and when it isn't. (Compare with #2503963: XSS in Quick Edit: entity title is not safely encoded. @alexpott and I have discussed this previously, but I'm not sure if there's a reference on d.o.)
Comment #42
alexpottRe #40 this is about
#options
so I'm not sure howtheme_form_element_label()
relates? I must be missing something. Afaics Drupal 7 check_plain()'d everything for select lists in form_select_options() and left checkboxes and radios up to the caller. Looking at FormElementTestCase.Wrt to #41 I'm not sure what we can do here. Anything that consumes JSON needs to be aware of how safe or not the content of the JSON is. We have no defined policy on this. There is the beginning of one on #2503963: XSS in Quick Edit: entity title is not safely encoded but obviously we're completely inconsistent in core :(
Comment #43
alexpottCreated https://www.drupal.org/node/2565515
Comment #44
lauriiiCreated follow-up for the Json issue: #2565527: Ensure that all different output methods can output safe markup
Comment #45
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThat's what I believed for a long time too, but it's not the case. Each element in #options gets converted to an individual checkbox/radio label in form_process_radios() and form_process_checkboxes(), then escaped through filter_xss_admin() via the code I mentioned above. This is actually a documented security feature in Drupal 7 (see https://www.drupal.org/node/28984).
Looking right now, I can't seem to find any tests for this in Drupal 7 (although it's hard to search for the absence of something) so it's possible it did go missing entirely in Drupal 8. If checkboxes and radios aren't being sanitized at all now in Drupal 8 (??) I think this issue should be critical - although it is only a security hardening, a security hardening that was present in Drupal 7 and gone in Drupal 8 seems like a bad idea to release with.
Comment #46
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #47
alexpott@David_Rothstein I've confirmed that in HEAD #options is XSS filtered for radios and checkboxes.
So this gets admin filtered by default.
The question now is should we change this to auto-escape like everything else. I think so since we now expect things to be escaped by default and filtering is the special case. I would love to get the point where we have a well defined list of things that are auto-filtered as part of a render array - #markup, #description, #prefix, #suffix, #field_prefix, #field_suffix because they are expected to contain markup. Everything else should be escaped by Twig unless it's a safe string.
If we go down this route we'll also have to fix
template_preprocess_fieldset()
Comment #49
alexpottFixing the test fail.
Comment #53
dawehnerRegarding the test failures, what happens is that you have a render array looking like:
so I guess #markup needs render array support ideally? Alternative everytime we do something like
we would first have to check whether
#title
is already an array?IMHO its the wrong level to esacpe in HTML and not on the presentation layer, which is javascript, but well, I think its too late to fight that battle ...
Its not clear for me the label is encoded in the first place ...
Seriously in tests I kinda think its better to Use
EntityFormDisplay::load() | EntityViewDisplay::load()
does someone have some opinion about it?This is soooo much better DX
Comment #54
alexpott@dawehner / #53
©
in.Comment #55
alexpottComment #56
alexpottSo this is incredibly tricky... this will make a title admin filtered when it should be escaped. Yet again we are thwarted because we don't have a render key that actually behaves the same way as twig - there are some similar discussions on #2568045: Make it impossible to double escape with #plain_text
Comment #59
alexpottSo thinking about this more I think we should step back and just make this about removing
SafeMarkup::checkPlain()
from #options since escaped text is unchanged by admin filtering and we can discuss the default behaviour in another issue - since as the changes above show it's way more complex. No interdiff because I'm kind of starting again.I leave all the test coverage added so if we do make changes to the underlying behaviour we've got coverage.
Comment #60
alexpottUpdated IS to detail new limited direction.
Comment #61
alexpott@lauriii asked me in IRC if this was correct. I originally thought it was not but testing has shown that it is. This is only used for optgroups and optgroups are only supported by select elements and they are escaped. I've added additional tests around this.
Comment #62
lauriiiThanks for making that change. This looks good for me now!
Comment #63
dawehnerDoes someone mind explaining in the issue summary why auto escaping wasn't applied as in earlier patches?
I think it is totally fine to limit the issue as much as possible, but it would be nice to have some information why this was done.
Comment #64
alexpott@dawehner this is because changing the behaviour of radios and checkboxes means you end up getting totally entangled with how the low level form element rendering system works. If you look at the changes in #54:
These are likely to have widespread impact because it changes the default behaviour from admin filtering to escaping. I do believe this is the correct change to make and I've put lots of tests in this issue that will fail when we make that change so it is easier to work out the impacts BUT I'm not sure it is critical. Making that change is going to take time I think we don't have. Opened #2568647: Make #title in form elements autoescape instead of auto XSS admin filter for more discussion.
Comment #65
catchNit fixable on commit: supprted.
I think it makes sense to do #2568647: Make #title in form elements autoescape instead of auto XSS admin filter separately.
Comment #66
dawehnerYeah it makes sense.
Comment #67
catchCommitted/pushed to 8.0.x, thanks!