Posted by peterpoe on August 9, 2012 at 5:41pm
4 followers
| Project: | Drupal Commerce |
| Version: | 7.x-1.x-dev |
| Component: | Cart |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
When presenting an add to cart form with product attributes, Commerce Cart uses the API function _options_properties to prepare the options, but always passes "select" as the first argument, so tags are stripped from the options even if the widget is radios (where tags would be legal).
The fix is simple, change the hardcoded "select" argument to $commerce_cart_settings['attribute_widget'], which contains the actual widget used (patch follows).
Comments
#1
Patch!
#2
Can you tell me if this would open up an XSS vulnerability given the different nature of the two types of form elements? If there is a potential for someone to put malicious code into attribute options, we'd need to ensure users could only create these products with "admin" permissions (i.e. permissions that have a warning label on them).
#3
I just did a test and it properly filters out JavaScript. Any thoughts on remote file execution?
#4
The options module, in options_field_widget_form(), takes care of label sanitization in two steps:
- First, in _options_properties(), will add the 'strip_tags' property if $type is 'select', or the 'filter_xss' property if $type is 'button'.
- Second, in _options_prepare_options(), will call strip_tags() on the label if 'strip_tags' is TRUE, and field_filter_xss() if 'filter_xss' is TRUE.
By replicating the options module behavior, we don't need to worry about security issues.
Here is a better patch that extracts the widget type in the same way options_field_widget_form(), so it's always correct.
#5
I was about to commit this when I ran a simple test on a module that I know of that defines a new potential widget type for attribute fields on Add to Cart forms. With the current patch, we end up with a $properties array that is still set to default, with no filter_xss check being called since it defaults to FALSE for that. I'm afraid of what security holes we might open up if we change this now, so I have a modest proposal - if we get a widget type that is recognizable to _options_properties (select, buttons, or onoff), then we pass that, otherwise we default to select to maintain the current level of escaping.
Whaddya think?
#6
Reasonable. Patch as per #5, with an added comment explaining what's going on.
#7
New patch with better checking for the widget type (previously wrongly I assumed that the type name always began with "options_").
#8
Thanks for the re-roll. I realized while reviewing that we don't actually need this to be based on the field instance's widget but on the actual widget used for the attribute field on the Add to Cart form. This comes from the field instance's attribute settings array, so I changed it to that. However, upon review it wasn't functioning as expected, and it turns out it's because we don't use 'buttons' as the attribute_widget value. We actually use the name of the form element - 'radios'. Oy vey.
I made a simple mapping for $widget_type detection to just look for 'radios' and use 'buttons', 'checkbox' to use 'onoff', and anything else to use 'select'.
Commit: http://drupalcode.org/project/commerce.git/commitdiff/b627812
#9
Automatically closed -- issue fixed for 2 weeks with no activity.