Posted by yched on October 26, 2010 at 11:28pm
9 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | forms system |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Needs tests, security |
Issue Summary
When buttons use a '#limit_validation_errors' = array($section) property, errors are reported only on elements where :array_slice(explode('][', $name), 0, count($section)) === $section
That clause is never true for $sections containing numeric parts (i.e $form['foo'][0]['bar']), because the corresponding element will be '0' (string) in the left-side array, and 0 (numeric) in the right side array, which fails ===.
- would a simple == be OK there ?
- other than that, maybe reimplode both arrays into a string, and compare the strings ?
Comments
#1
Good catch. This should work:
array_slice(explode('][', $name), 0, count($section)) == array_values($section)I guess we need tests here.
Definitely "major" since field items have numeric indexes.
#2
Actually, no! Because 0 == 'foo' is true. I guess rather than exploding $name, you can implode $section, and then compare as strings.
#3
You mean something like
strpos($name, implode('][', $section)) === 0?#4
Like so ?
1st patch only adds a test showing the bug (should fail)
2nd patch is test + fix (should pass)
#5
bump ?
#6
@yched: Sorry I've delayed so long on this. I was hoping to get some time to look at this in more detail, and maybe even help with a new patch, but since that hasn't happened, and since we're nearing RC, here's a cursory review. I think this issue is critical, since it may lead to a security vulnerability, though I haven't looked at it closely enough to know for sure if the existing bug can be exploited as a security hole.
+++ includes/form.inc@@ -1423,12 +1423,15 @@ function form_set_error($name = NULL, $message = '', $limit_validation_errors =
+ $section_string = implode('][', $section);
+ if (strpos($name, $section_string) === 0) {
I think this should probably be changed to
strpos($name . ']', $section_string . ']')or something like that, so that 'foobar' isn't considered "inside" 'foo'. We need a test for this as well, both at the top level, and a nested level, like 'parent][foo'.+++ modules/simpletest/tests/form.test@@ -461,16 +461,24 @@ class FormValidationTestCase extends DrupalWebTestCase {
+ // Edge case of #limit_validation_errors containing numeric indexes: same
+ // thing with the 'Partial validate (numeric index)' button and the
+ // 'test_numeric_index' field.
Is it really an edge case, if it happens within Field API?
Powered by Dreditor.
#7
Added the suggestion from @effulgentsia in #6 to prevent substrings from matching incorrectly. Also added a test for it.
#8
Marking as needs review to trigger test.
#9
This is the same patch as above but the change is the smallest possible to fix the problem. I like small changes, you know?
#10
In case of a reroll, patch contains trailing whitespace.
#11
Discussing with webchick, actually we can throw out array_slice and just limit explode.
#12
@tsoeckler , mine? The previous contained fixing whitespace which I skipped.
#13
@effulgentsia : 'foo / foobar' : right, good catch. The $section_string needs a trailing ']', but I'm not sure the $name does.
The bug doesn't really happen within Field API for now - not until combofield is a reality.
Currently Field API's uses of #limit_validation_errors don't include numeric indexes (array paths do no go inside deltas)
The bug does however potentially affect any form structured with numeric indexes. How much of a sec hole this means is not too clear for me :-)
#14
#13 was written in an open tab while #9-#12 were posted :-p
#11 looks just fine.
#15
Ok, committed to HEAD!
#16
Unfortunately this patch has totally broken top level validation with #limit_validation_errors, e.g.:
<?php
$form['test'] = array('#tree' => true);
$form['test']['inner'] = array(
'#title' => t('Should be validated'),
'#type' => 'textfield',
'#required' => TRUE,
);
$form['actions']['submit'] = array(
'#type' => 'submit',
'#value' => t('Should validate test and inner'),
'#limit_validation_errors' => array(array('test')),
'#submit' => array('some_submit_function'),
);
?>
With the patch applied in #11, $form_state['values']['test']['inner'] is never validated, without the patch it is validated.
#17
explode($foo, $bar, $limit) doesn't work :
"If limit is set and positive, the returned array will contain a maximum of limit elements *with the last element containing the rest of string*."
The array_slice(explode()) is needed - i.e. revert #11, commit #9.
#18
#19
patch in #9 works and successfully validates children.
#20
Committed to CVS HEAD. Thanks.
#21
Automatically closed -- issue fixed for 2 weeks with no activity.