I just update from 6.x-3.15 to 6.x-3.17 version of webform module, and I have a problem with some fields of the webform with select or others... module.

When "Multiple" and "allow other option" options are checked for a webform field configuration, le field label is correctly displayed, but the choices are not shown to the user. So the user can't choose a value, and a "fied is required" error is displayed when he wants to submitted the form.

When "multiple" option is checked without "allow other option", everything is OK. When "allow other option" option is checked without "multiple" option, everything is OK.
When "multiple", "allow other option" and "listbox" options are selected, the field choices are correctly displayed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lucuhb’s picture

The problem seems to be due to the line 398 of components/select.inc

   if ($element['#select_type'] == 'checkboxes') {
    $element['select']['#process'][] = 'webform_expand_select_ids'; /* <- pb on this line */
  }
elseif ($element['#select_type'] == 'radios') {
    $element['select']['#select_type'] = array('expand_radios', 'webform_expand_select_ids');
  }

When the line $element['select']['#process'][] = 'webform_expand_select_ids'; is commented, the different values of the field are correctly displayed.

quicksketch’s picture

Thanks @lucuhb, I think this problem only affects the D6 version. I'll give it a look when I get a chance.

quicksketch’s picture

As @Jorrit pointed out in #1472920: Problem with select or others module when "Multiple" and "other" options are selected (which I marked duplicate) the conditional for radio buttons is also messed up, as it's setting "#select_type" when it should be setting "#process", so we've got all kinds of issues here.

tcarrillo’s picture

Is there a quick fix for this while we are waiting for an update? I am on webform 6.x-3.15 which has a security risk and needs to be updated to 6.x-3.17 but I can't update because most of my webforms use select or other and they all break when I update.

Thanx

eporama’s picture

Just to get the right link @Jorrit's issue is: #1478354: Checkboxes with select or other display empty element

LittleRedHen’s picture

If line 398 of components/select.inc is changed to

  $element['#process'][] = 'webform_expand_select_ids';

then the checkboxes-or-other element also renders properly. This matches the way that the same callback is added to multiple/checkbox lists that don't have the "or other" option: at line 364 (in the _webform_render_select method).

But having just read the comments above, I tried commenting the line out entirely, and can confirm that that seems to work as well. So I guess the unexpected addition to the $element['select']['#process'] sub-array must be confusing something? Checking the array contents, there is no ['#process'] entry before this, and it seems to be creating that particular entry at all that causes the problem, not which particular callback is added to it.

travist’s picture

Status: Active » Reviewed & tested by the community
FileSize
651 bytes

I can confirm that #6 above fixes the issue. Here is the patch.

quicksketch’s picture

FileSize
1.43 KB

The change in #6 doesn't have the security benefit the code was actually changed to support. (Though of course it being broken is probably even worse than the security problem). Unfortunately since everyone is pretty much aware of the problem at this point I don't see any reason to keep this "secret", since right now anyone who has select or other installed with Webform on Drupal 6 has a just plain broken installation (but at least it's "safe" :P).

My apologies for this breakage, I targeted the fix at Drupal 7 (which is fixed) and the D6 version didn't get the adequate testing to ensure the changes actually worked. But now that we've got an excellent group of people looking at solving the problem, if you guys can confirm this fix I'd appreciate it and we can make a new D6 release that fixes these problems.

The problem with the patch in #6 is that it doesn't prevent against XSS. If you enter an option label such as title<script>alert('hi');</script>, it should be printed out as titlealert('hi');. If it makes an alert box, it means it's vulnerable to XSS attacks. This patch fixes both the problem with select or other fields being broken and the XSS problem that was in previous versions of Webform. If you guys can test it out in these combinations I'd appreciate the confirmation:

- As checkboxes and radio buttons
- With the "Allow other" option enabled and disabled
- With typical labels and with those that contain <script> tags like my example above.

quicksketch’s picture

Status: Reviewed & tested by the community » Needs review
lucuhb’s picture

thanks for this patch. I tried this patch with :

  • checkboxes without "allow other" -> OK
  • checkboxes with "allow other" -> OK
  • radio buttons with "allow other" -> OK
  • radio buttons with "allow other" -> OK
  • with a label like "title<script>alert('hi');</script>" -> printed as titlealert('hi');
lucuhb’s picture

A restriction on my previous comment: when list selection is choosen, the label title<script>alert('hi');</script> is printed

quicksketch’s picture

the label title<script>alert('hi');</script> is printed

That's fine as long as the <script> tag is merely printed to the page, rather than being executed and a JS alert box appears. This is the intended functionality of Webform, since if you have a "strong" or "em" tag you want them to be rendered for checkboxes or radio buttons, but in the HTML standard, select lists don't support any tags inside of them, so they should just end up displaying as plain text.

So overall sounds like it's working correctly. Thanks for the testing @lucuhb!

lucuhb’s picture

Yes, of course, you're right.

Hope that a version incorporating this patch will be available soon ! Thanks for your work

quicksketch’s picture

Status: Needs review » Fixed

Committed to 6.x-3.x branch. I'll be getting 3.18 out shortly.

Status: Fixed » Closed (fixed)

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