Color module security fixes from SA-CORE-2011-001 not yet applied

Still needs test.
I'm not going to do this unless someone gives me a complete walkthrough in irc.

Just out of curiosity why are there brackets in form_set_error behind palette?
form_set_error('palette][' ...)

#2 @aspilicious

@@ -269,6 +270,18 @@
+      form_set_error('palette][' . $key, t('%name must be a valid hexadecimal CSS color value.', array('%name' => $form['color']['palette'][$key]['#title'])));

This is how form_set_error() works. If you have eg. $form['palette']['color'], you are setting the form error on color with form_set_error('palette][color', 'Error message'); in case that *palette* is the parent of the *color* - palette has #tree set to TRUE.

Powered by Dreditor.

@aspilicious: writing tests should be easy enough, you just have to make sure that invalid CSS color codes trigger a form error with the message '%name must be a valid hexadecimal CSS color value'. invalid color codes are for example #abcdefg, #z012345, etc. these tests could also be backported to D7.

I'm on it (at least I'm trying :) )

I can't test on my windows system. Database timeout or something like that.

I tried to make a patch with the help of Swentel.
I'm curious what the testbot will say :)

+++ b/modules/color/color.test
@@ -93,4 +93,22 @@
+    $this->assertText('%name must be a valid hexadecimal CSS color value');
+
+    $edit['palette[bg]'] = '#z012345';
+    $this->drupalPost($settings_path, $edit, t('Save configuration'));
+    $this->assertText('%name must be a valid hexadecimal CSS color value');

I doubt this will work, the value of %name is missing from assertText().

and with '#z012345', I meant to write '#z01234', that is a 6 digit code but with an invalid character.

Maybe this will work... not sure...

Hmm lets try to login this time...

I think this is ok now. Test needs a better comment or a '.' in the end.
I'm attaching a drupal 7 test only patch.

Drupal 8: patch in #10
Drupal 7: patch in #12

Needs review for #10

The test should be backported to D7.

Code review:

+++ b/modules/color/color.module
@@ -269,6 +270,18 @@
+      form_set_error('palette][' . $key, t('%name must be a valid hexadecimal CSS color value.', array('%name' => $form['color']['palette'][$key]['#title'])));

I don't like that error message. %name will be something like "Main background" and inserting that, the sentence doesn't sound very good. Also I think we can drop the "CSS" part. Suggestion:

You must enter a valid hexadecimal color value for %name.

+++ b/modules/color/color.test
@@ -93,4 +93,23 @@
+    $edit['palette[bg]'] = '#abcdefg';
...
+    $edit['palette[bg]'] = '#z01234';

I'm generally not a fan of testing these kind of low-level things, but if we are already testing this, I think we should add a proper set of tests (and a foreach :)).
Maybe:

$tests = array(
  '#000' => TRUE,
  '#123456' => TRUE,
  '#abcdef' => TRUE,
  '#0' => FALSE,
  '#00' => FALSE,
  '#0000' => FALSE,
  '#00000' => FALSE,
  '123456' => FALSE,
  '#00000g' => FALSE,
);

or whatever the best syntax is.

That said, I tried the patch out and it works.

Ok the first two patches contain the tests for D7. One breaks strings the other doesn't
The last patch contains the D8 patch.

#16: 1179424-color-security-fix-16.patch queued for re-testing.

Lets try to upload this again. (only the D8 version). Stupid test bot...

Looks RTBC to me, but leaving for more reviews.

That looks good to me too.

Beautiful! Thanks so much, folks!

Committed to 8.x, and committed the tests only to 7.x.

D'oh. Well that'll teach me to commit blindly. :P This fails testbot in 7.x.

Rolled back the 7.x changes. Moving this back to a normal task now.

Ah sorry, that was because of the changed wording in D8. I should have noted that (which I would, if I had thought of it). Will roll a better test in a minute.

Hmm... actually the patch #16-2 from above should be fine. Re-uploading.

This is RTBC. Sorry again for the trouble, which was in part my fault.
Note that this patch is not mine, but aspilicous'.

Great; thanks for the fast turnaround on this.

Committed to 7.x.

Also, restoring metadata.

Removing tags (needs backport to D7 queue cleanup)

Actually the last tag is actually useful for historical reference. Restoring that tag.

Restoring the other two, too, for the same reason.