Problem/Motivation

form_select_options() is a recursive markup generating function. It uses SafeMarkup::set() to get around the fact that it's basically a theme function in disguise, hiding inside template_preprocess_select().

Commit credit: also add cilefen, porchlight, lbainbridge.

Proposed resolution

refactor to actually printing the options markup in select.html.twig.

Remaining tasks

  • (done) Update docs of form_select_options().
  • (done) change return to mixed[]
  • (done - not needed. See #72) We also need to identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.

Manual testing steps (for XSS and double escaping)

In head

  1. edit form.inc to take out the checkplains
    $options .= '<option value="' . $key . '"' . $selected . '>' . SafeMarkup::checkPlain($choice) . '</option>';
  2. add a text field to basic page content type
  3. trick it into putting script tag by closing the quotes
  4. use these allowed values
    "><script>alert('XSS');</script><"|one
    two|two
    three|three
  5. load a create content page and see the alert. (or load the field settings page)

API changes

Removes form_select_options(), however this was an internal function only called (in core at least) by template_preprocess_select().

Files: 
CommentFileSizeAuthor
#71 interdiff-54-71.txt598 bytesalimac
#71 form_select_options-2501481-71.patch5.32 KBalimac
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,972 pass(es). View
#67 do-not-commit.checking-for-test-coverage.patch582 bytesYesCT
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,907 pass(es). View
#54 interdiff-2501481-47to54.txt1.07 KBdavidhernandez
#54 form_select_options-2501481-54.patch5.32 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,573 pass(es). View
#47 interdiff-2501481-44to47.txt944 bytesdavidhernandez
#47 form_select_options-2501481-47.patch5.31 KBdavidhernandez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 98,049 pass(es). View
#44 form_select_options-2501481-44.patch4.9 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,310 pass(es). View
#37 interdiff.txt1.09 KBCottser
#37 form_select_options-2501481-37.patch5.62 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,241 pass(es). View
#35 formselectafter.png48.07 KBdavidhernandez
#35 formselectbefore.png48.36 KBdavidhernandez
#33 interdiff.txt512 bytesCottser
#33 form_select_options-2501481-33.patch4.45 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,239 pass(es). View
#27 interdiff.txt1.11 KBCottser
#27 form_select_options-2501481-26.patch4.41 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,002 pass(es), 3 fail(s), and 1 exception(s). View
#25 form_select_options-2501481-25.patch4.39 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#15 interdiff-2501481-7to15.txt7.07 KBdavidhernandez
#15 form_select_options-2501481-15.patch8.75 KBdavidhernandez
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,859 pass(es), 1 fail(s), and 1 exception(s). View
#7 interdiff.txt1.53 KBkfriend
#7 form_select_options-2501481-5.patch3.18 KBkfriend
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,778 pass(es), 1 fail(s), and 5,103 exception(s). View
#5 interdiff.txt1.22 KBkfriend
#5 form_select_options-2501481-4.patch3.36 KBkfriend
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,189 pass(es), 54 fail(s), and 5,114 exception(s). View
#3 form_select_options-2501481-3.patch3.28 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,176 pass(es), 64 fail(s), and 5,111 exception(s). View

Comments

Cottser’s picture

Assigned: Unassigned » Cottser
Cottser’s picture

Working on this, so far so good.

Cottser’s picture

Assigned: Cottser » Unassigned
Issue summary: View changes
Status: Active » Needs review
FileSize
3.28 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,176 pass(es), 64 fail(s), and 5,111 exception(s). View

Initial patch, worked on this with @kfriend. It will have fails.

The Classy template removal is only temporary to avoid having to update multiple template files while iterating.

Status: Needs review » Needs work

The last submitted patch, 3: form_select_options-2501481-3.patch, failed testing.

kfriend’s picture

Status: Needs work » Needs review
FileSize
3.36 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,189 pass(es), 54 fail(s), and 5,114 exception(s). View
1.22 KB

Status: Needs review » Needs work

The last submitted patch, 5: form_select_options-2501481-4.patch, failed testing.

kfriend’s picture

FileSize
3.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,778 pass(es), 1 fail(s), and 5,103 exception(s). View
1.53 KB

Progress on work. Passed option widget tests.

kfriend’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: form_select_options-2501481-5.patch, failed testing.

davidhernandez’s picture

Assigned: Unassigned » davidhernandez

Taking a look at this now, and will work on the tests. I think we can remove form_select_options since I don't find anything else calling it in core. I'd possibly want framework manager confirmation on that since maybe it is needed for some other, broader api reason?

Cottser’s picture

Issue summary: View changes

Yeah I think it can be removed. Could potentially grep D7 contrib if we want to see if anyone is using it for stuff.

davidhernandez’s picture

+++ b/core/modules/system/templates/select.html.twig
@@ -12,4 +12,55 @@
+        {% if element['#value'] is not defined %}
+          {% set selected = false %}
+        {% elseif element['#value'] is not iterable %}
+          {% set selected = ((element['#value'] ~ "") == key) %}
+        {% elseif key in element['#value'] %}
+          {% set selected = true %}
+        {% elseif element['#value'] is empty and element['#multiple'] is not empty and key == '_none' %}
+          {% set selected = true %}
+        {% endif %}

Question about the conditions me by this 'if'. What exactly is the functionality this is satisfying? All I'm seeing is selected being set, but it isn't printed anywhere. It is only checked later on and if it is true ' selected="selected"' gets printed.

Cottser’s picture

It's just a Boolean, never printed. Just used to determine whether to print as you said.

davidhernandez’s picture

Right, what I mean is why the various elseifs; checking if the element value equals the key, versus it just existing, and this set {% set selected = ((element['#value'] ~ "") == key) %}?

davidhernandez’s picture

Assigned: davidhernandez » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
FileSize
8.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,859 pass(es), 1 fail(s), and 1 exception(s). View
7.07 KB

Ignore my previous comment. I see now what that is doing, and the replication from the original PHP function.

I'm uploading a patch with a few things.

1) Added comment to the template about the macro with twig reference link. We've done this in other macros and I think it is helpful.
2) Removed form_select_options(). I did not see it used anywhere else in core.
3) Removed the "defined test accommodates..." comment from the template. I don't see what it is in reference to. Leftover from testing something?
4) Moved the spaceless tag higher, because the markup was still creating a large amount of empty space that I feel made the markup more annoying to read.
5) Fixed a mistake in the code around choice.option. This should be options. This is what caused the php errors and failed tests for me. It is also singular in the original PHP code, which means that code is likely not working at all in HEAD. The fail was on dblog, because the select list created there seems to be one of the few that would even satisfy that condition.
6 ) Included the Classy template.

cilefen should be included in the commit credit. We had to yell at each other for a good while to figure out 5).

Status: Needs review » Needs work

The last submitted patch, 15: form_select_options-2501481-15.patch, failed testing.

tim.plunkett’s picture

Tempted to switch this to form system...
Sure it abuses SafeMarkup::set(), but it also is a self-contained recursive function.
This change generates the world's most complex twig file, that cannot be unit tested and clearly wishes it were just PHP.

tim.plunkett’s picture

Component: theme system » forms system

Yeah this is not a change to the theme system. It changes the forms system.

Cottser’s picture

Tim that sounds great too. Thanks!

Cottser’s picture

And what I mean is I would love a solution to this that isn't tied up with the theme system if at all possible.

tim.plunkett’s picture

Actually, this seems to be duplicating a lot of #342316: Introduce proper Form API #types for 'option' and 'optgroup', and make #options consistent., which seems like a reasonable approach.

Cottser’s picture

What if we sort out all the crazy edge cases in PHP (pre render probably) and build out the structured data as an array or similar to pass to the template? Middle ground to be found there? I do think we're stretching Twig a bit here, but not because it's recursive. Menus are recursive too.

tim.plunkett’s picture

+100 to 22

Cottser’s picture

Thanks Tim! I'm thinking since it wouldn't be preparing a render array then just in the preprocess may actually make more sense than pre render here. Definitely seems to be one of the more complicated form API elements.

Cottser’s picture

Title: form_select_options() is a theme function in disguise and abuses SafeMarkup::set » form_select_options() is a theme function in disguise and should not use SafeMarkup::set
Status: Needs work » Needs review
FileSize
4.39 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Rough prototype. Not all that recursive, seeing what fails for now. Early feedback definitely welcome!

No interdiff because it's a new approach.

Status: Needs review » Needs work

The last submitted patch, 25: form_select_options-2501481-25.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
4.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,002 pass(es), 3 fail(s), and 1 exception(s). View
1.11 KB

Oops, a couple obvious fixes :)

Cottser’s picture

One thing that's not super clear to me is whether or not HEAD supports nested optgroups. It seems like the HTML5 spec does not: http://www.w3.org/TR/2012/WD-html5-20121025/the-optgroup-element.html#th...

Status: Needs review » Needs work

The last submitted patch, 27: form_select_options-2501481-26.patch, failed testing.

davidhernandez’s picture

Cool that's what I was just about to do last night. The template is simpler, and thank you for moving the whitespace modifier further out. The resultant markup was making me twitch. As long as we are getting markup out of php and removing the SafeMarkup calls, it should be an improvement.

The taxonomy uid test is the same that was failing for me. You can see the ui fail when you try to add a taxonomy filter in a view. For me it looked like an issue with the 'if' checking the option, but that is completely gone in this patch.

Cottser’s picture

So I tried to run Drupal\taxonomy\Tests\Views\TaxonomyIndexTidUiTest locally and 2 hours later I took my headphones off and wondered why my fans were spinning. Test was still running, yikes. Manual testing steps are super helpful though thanks @davidhernandez! I'll comment before I start working on this again.

Edit: And that means anyone else is also welcome to jump in of course!

Cottser’s picture

Working on this at Drupal North.

Cottser’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
4.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,239 pass(es). View
512 bytes

So far I can't see any problems with this approach.

@porchlight and @lbainbridge also helped.

Cottser’s picture

Issue summary: View changes
davidhernandez’s picture

FileSize
48.36 KB
48.07 KB

Did that reset really just fix everything? Good catch.

Looking this over now. Here is a before and after screenshot of some markup. Looks the same. Is there a place in core to check an optgroup? I don't find one anywhere and it doesn't look like any of the UIs let you make one.


davidhernandez’s picture

Status: Needs review » Needs work

Found an optgroup. "Add a new field" when adding a field to a content type uses it.

This looks good. I didn't see any issues and did some manual testing. I did not have any issues with the safety of the strings.

Setting back to needs work, because we need the Classy template.

Cottser’s picture

Status: Needs work » Needs review
FileSize
5.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,241 pass(es). View
1.09 KB

Here's that. As far as the template itself, do we want to make it DRYer by putting the <option> output into a macro or something? Or is that just unneeded complexity?

davidhernandez’s picture

I thought about that, but the way the template is right now it is very easy to understand, and I don't think we gain anything with the added complexity.

davidhernandez’s picture

Status: Needs review » Reviewed & tested by the community

Looks pretty good.

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

Thanks, still has todos though.

tim.plunkett’s picture

+++ b/core/modules/ckeditor/js/ckeditor.js
@@ -30,7 +30,7 @@
-      var label = $('label[for=' + element.getAttribute('id') + ']').html();
+      var label = $('label[for=' + element.getAttribute('id') + ']').text();

This reverts a recent security fix :D

Cottser’s picture

Looks like I forgot to rebase. I'll fix that tonight, thanks @tim.plunkett!

davidhernandez’s picture

Oh, I didn't even notice that. interdiff-- davidhernandez-- :(

Cottser’s picture

Status: Needs work » Needs review
FileSize
4.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,310 pass(es). View

Didn't get to it last night so here's the rebased patch. No other changes from #37.

dawehner’s picture

I really like the fact that we move HTML into the template, where it belongs to.
On the other hand I don't feel to be confident to RTBC this patch, given my limited knowledge.

lauriii’s picture

Status: Needs review » Needs work
+++ b/core/includes/form.inc
@@ -78,13 +78,16 @@ function template_preprocess_select(&$variables) {
+ *   @todo Document the format of this array.

There is still @todo comment which needs to be solved

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
5.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 98,049 pass(es). View
944 bytes

See if this makes sense.

pwolanin’s picture

Minor, but I might use current() here:

+      $option = form_select_options($element, $choice->option);
+      $options[] = reset($option);
$options[] = current(form_select_options($element, $choice->option));

Looks like some of the other cases specifying an array return value seem to use a : like "containing the following keys:", but it's not especially consistent.

pwolanin’s picture

Actually, better still would probably be to use array_merge()

davidhernandez’s picture

+++ b/core/includes/form.inc
@@ -94,29 +103,36 @@ function form_select_options($element, $choices = NULL) {
     elseif (is_object($choice) && isset($choice->option)) {

Does anyone know the use-case that satisfies the elseif where $choice is an object?

Cottser’s picture

Status: Needs review » Needs work

That one test that was failing via Views UI I'm pretty sure uses that method.

The docs are an awesome start thanks @davidhernandez. They should probably say that it can be a nested array, and options applies to non-optgroup selects as well.

davidhernandez’s picture

..and options applies to non-optgroup selects as well.

+++ b/core/includes/form.inc
@@ -94,29 +103,36 @@ function form_select_options($element, $choices = NULL) {
+        'options' => form_select_options($element, $choice),

I don't think that is correct. This is the only place I see 'options' set, so it only applies to an optgroup?

Cottser’s picture

Sorry, yes of course you're right.

davidhernandez’s picture

Status: Needs work » Needs review
FileSize
5.32 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,573 pass(es). View
1.07 KB
lauriii’s picture

Patch itself looks quite good. We have @todo inside the IS which needs to be addressed.

+++ b/core/includes/form.inc
@@ -78,13 +78,22 @@ function template_preprocess_select(&$variables) {
+ * @return array

s/array/mixed[]

davidhernandez’s picture

Is it mixed if it's always an array, regardless of the depth of the array?

Which todo are you referring to? The testing? I assume someone will do that when reviewing. I did some manual testing myself, and did not find any issues.

lauriii’s picture

Its array but mixed[] means its an array containing mixed items.

It would be helpful to have it there so its easier to review & move forward.

We also need to identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.

dawehner’s picture

+++ b/core/includes/form.inc
@@ -78,13 +78,22 @@ function template_preprocess_select(&$variables) {
+ * @return array

Well technically its array[], its an array of arrays, of which array is not further defined.

lauriii’s picture

I disagree because according to the documentation it is possible there to be string values too which makes it mixed.

davidhernandez’s picture

+++ b/core/includes/form.inc
@@ -94,29 +103,35 @@ function form_select_options($element, $choices = NULL) {
+  $options = [];

Which documentation? This should always return an array. The structure of the array might be different though.

YesCT’s picture

I will review this and work on the todo in the issue summary.

YesCT’s picture

Issue summary: View changes

@davidhernandez is clever. :)
adding steps to reproduce an alert in head...

YesCT’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

manually tested with the patch, and there is no alert. and when I turn off twig autoescape (thanks @tim.plunkett) by:

diff --git a/core/lib/Drupal/Core/Template/TwigEnvironment.php b/core/lib/Drupal/Core/Template/TwigEnvironment.php
index 346dcc2..6014ab1 100644
--- a/core/lib/Drupal/Core/Template/TwigEnvironment.php
+++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
@@ -58,7 +58,7 @@ public function __construct($root, \Twig_LoaderInterface $loader = NULL, $option
       'auto_reload' => NULL,
     );
     // Ensure autoescaping is always on.
-    $options['autoescape'] = TRUE;
+    $options['autoescape'] = FALSE;

     $this->loader = $loader;
     parent::__construct($this->loader, $options);

and running drush cr

and reloading the page, the alert does happen.

----

next I'm going to read the entire patch.

YesCT’s picture

I think @return array is fine the way it is. it is accurate. (if we really want to be more specific, then I think it would be mixed[])

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

so... I read the whole thing, and could go either way on the return. so I think it is fine the way it is.

YesCT’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review

darn. forgot about checking for test coverage. noting in the issue summary so it is not missed again, but I'll also look for it right now.

YesCT’s picture

FileSize
582 bytes
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,907 pass(es). View

let's try this. if there is test coverage, then this should fail.

lauriii’s picture

@davidhernandez: thats why its mixed[], array containing mixed types of items. You can also say stdClass[] which would mean array containing stdClass type of items and string[] if all the items are strings

lauriii’s picture

Issue tags: +Needs tests
YesCT’s picture

Issue summary: View changes
Status: Needs review » Needs work

ok. needs work for mixed[] and tests.

alimac’s picture

Status: Needs work » Needs review
FileSize
5.32 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,972 pass(es). View
598 bytes

Changed array to mixed[].

I would like to write the automated test for this, but I am not sure where to start. Any hints?

davidhernandez’s picture

There is already test coverage for the functionality of select form items in Drupal\system\Tests\Form and we are now changing the functionality to output through Twig instead of directly in form_select_options. Don't we now have test coverage through Twig? I understand that we do not have proper test coverage in head, but I think the functional change here gives us new test coverage by changing the way this outputs.

The mixed[] change is fine.

YesCT’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

ok. that addresses the remaining todo's.

YesCT’s picture

Issue tags: -Needs tests
cilefen’s picture

Re #15 - oh that was at the NJ sprint.

lbainbridge’s picture

As @cottser mentioned in #32/33 we worked on this together with @porchlight at Drupal North, I was asked to comment so I could get credit.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This is a nice fix of a SafeMarkup::set(). Nice work everyone! Committed 8aa9b3a and pushed to 8.0.x. Thanks!

  • alexpott committed 8aa9b3a on 8.0.x
    Issue #2501481 by Cottser, davidhernandez, kfriend, alimac, YesCT,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.