Values from "in_operator" exposed filters are double-escaped (SA-CONTRIB-2009-037 regression)

They appear just fine using Firefox. That's the standard HTML entity for the ampersand and should display as '&'. Which browser are you using?

I don't see that HTML at all. All I see is in the source is reviewed & tested by the community. Also tested with Safari 4 with Vista.

Aha! Now I see what you mean: the status select field, not the actual status values in the issue listings. I can confirm it looks double encoded. PS screenshots are helpful.

This is either a bug in project_issue's views integration (likely) or in views itself (unlikely). Either way, it's not specific to d.o. Probably we're just double-escaping the status options in some cases. Should be relatively easy to find and fix if anyone's interested in looking under project_issue/views/handlers/*filter*.

I tracked this down to a bug in views, introduced here:

http://drupal.org/cvs?commit=223804
http://drupal.org/cvs?commit=223802

SA-CONTRIB-2009-037: Filter checkboxes could be displayed unfiltered allowing XSS attacks.

This code is a bit overzealous. ;) We've got an exposed filter using views_handler_filter_in_operator. Here's the problem:

A) views_handler_filter_in_operator::value_form() has the following:

    if ($which == 'all' || $which == 'value') {
      if ($this->value_form_type == 'checkboxes') {
        foreach ($options as $key => $option) {
          $options[$key] = check_plain($option);
        }
      }
      ...
    }

at this point, value_form_type is still 'checkboxes', so we escape everything. We need this since FAPI is inconsistent and does not automatically escape the '#options' values for checkboxes, even though it does so for every other form element.

B) views_handler_filter::exposed_form() calls this->value_form() and gets the form array, still as type checkboxes, with all of the elements still check_plain()'ed.

C) views_handler_filter::exposed_form() then calls $this->exposed_translate($form[$value], 'value');. views_handler_filter::exposed_translate() contains the following:

    // Checkboxes don't work so well in exposed forms due to GET conversions.
    if ($form['#type'] == 'checkboxes') {
      if (empty($form['#no_convert']) || !empty($this->options['expose']['single'])) {
        $form['#type'] = 'select';
      }
      if (empty($this->options['expose']['single'])) {
        $form['#multiple'] = TRUE;
      }
    }

Once our form element's '#options' array has been check_plain()'ed to make it secure as checkboxes in step (A), it now gets double-escaped by FAPI when the form element is converted into a 'select' box. :(

So, it seems like we're doing the check_plain() on the '#options' array way too early in the game. We need to do that later down the chain of events, because once we check_plain() assuming we're going to be 'checkboxes' there's no good way to invert that and get back to the unescaped values for use in 'select'...

I'll try to ping Earl about this in IRC and see if we can work out a viable solution that neither opens us up to XSS again, nor leads to double-escaping.

/me curses at FAPI's inconsistent checkboxes behavior again!!!

I would simply add a #pre_render to the checkboxes widget, instead of destructively modifying the options:

$form['widget'] = array(
  '#type' => 'checkboxes',
  '#options' => $this->options,
  '#pre_render' => array('views_check_plain_options'),
);

function views_check_plain_options($element) {
  $element['#options'] = array_map('check_plain', $element['#options']);
  return $element;
}

@Damien: Lovely idea! However, #pre_render won't work here because it's #type checkboxes and we're setting #process to use 'expand_checkboxes', and then #pre_render won't help. :( So, I'm using a #process callback to process the #options array (before we call expand_checkboxes), and now it all works great.

I added a project issue status code with the name <script>alert('issue-status-xss')</script>. There's still no XSS when you edit/configure an exposed filter, because the #process is still invoked to do the array_map(). However, there's no double-escaped values in the exposed filters on the advanced search page since when we decide to turn the element back into a 'select' box, we unset the #process to check_plain() the options. See attached patch...

@merlinofchaos: any objections, or should I commit this? ;)

Fixed typo in the PHPDoc for views_process_check_plain_options() and removed a stray newline.

Not sure if this is more legible (or faster), but here's a copy using array_search() instead of iterating over #process ourselves.

Got Earl to look at this in IRC. He pointed out that if the #process callback was a bit smarter, we wouldn't even have to worry about unsetting it when we switch something back to 'select'. The callback can just test $element['#type'] and only do the check_plain if it's checkbox*. See attached patch. Tested to confirm this is still working properly and doesn't introduce XSS.

Is this patch the only way to fix this issue with exposed filters in views? I'm not a developer and am intimidated by the idea of installing a patch....

thanks,
cj

Hello cj, I just checked and the current DEV version seems to have the patch applied. So just download that version (called 6.x-2.x-dev) instead of the current official 2.6 release.

Indeed, this was committed to both HEAD and DRUPAL-6--3:

http://drupal.org/cvs?commit=267356
http://drupal.org/cvs?commit=267354

So yeah, the fix will be out in the next official release...