Problem/Motivation

  • system_theme_settings_submit() unsets submitted form values instead of cleaning them up properly.

Proposed resolution

  • Use form_state_values_clean() to clean the submitted values.

Original report by sun

Extracted from #1376166: Custom logo and favicon functionality inanely tries to support absolute local file paths

CommentFileSizeAuthor
#27 variable-unserialize-error-1284364-26.patch1.44 KBkbasarab
#26 1397882-system_theme_settings_submit-26.patch1.44 KBpingers
#24 1397882-system_theme_settings_submit-24.patch1.44 KBkbasarab
#21 1397882-system_theme_settings_submit-21.patch1.22 KBkbasarab
#16 system_theme_settings_submit.patch947 bytescatch
#12 drupal8.system-theme-settings-submit.12.patch1.02 KBsun
#6 drupal8.system-theme-settings-submit.6.patch765 bytessun
#3 drupal.system-settings-form-submit.3.patch1.62 KBsun
drupal8.system-theme-settings-submit.0.patch1.24 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs review » Reviewed & tested by the community

This cleanup appears correct and straightforward.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x. Thanks.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Version: 8.x-dev » 7.x-dev
Status: Fixed » Needs review
FileSize
drupal.system-settings-form-submit.3.patch1.62 KB
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Needs review » Reviewed & tested by the community

Yep, can't think of a reason not to backport it, either.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue summary: View changes

Updated issue summary.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Yeah, this seems like it would be pretty harmless.

It's weird to commit a bug fix without a test, but I guess the existing tests would cover that the theme settings form is working, and all we're really doing here is cleaning up the existing behaviour.

Committed and pushed to 7.x. Thanks!

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Version: 7.x-dev » 8.x-dev
Status: Fixed » Needs review
FileSize
drupal8.system-theme-settings-submit.6.patch765 bytes

Apparently, we want to do a quick follow-up fix/adjustment here: The 'var' value should not be removed from $form_state['values'].

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

That last comment/patch was fairly obtuse, and none of us in #drupal-contribute could figure out what it meant. I think it means "+Needs tests" ;)

To be on the safe side, I've rolled this back from D7 since we're rolling a release in about an hour. Happy to re-commit it later.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/system/system.admin.inc
@@ -668,10 +668,10 @@ function _system_theme_settings_validate_path($path) {
-  unset($form_state['values']['var']);
...
+  unset($values['var']);

The follow-up patch does not remove the 'var' from the submitted form values, but only from the local copy of the form values being processed in this submission handler. Thus, 'var' remains as submitted form value for other modules.

Pointless to test.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work

The follow-up needs a comment, and someone else to mark it RTBC ;)

jrreid’s picture

jrreid CreditAttribution: jrreid commented
Status: Needs work » Reviewed & tested by the community

Makes sense to me and I see no issues either.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work

Since it doesn't make sense to either of us, I think we still need that comment. :)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal8.system-theme-settings-submit.12.patch1.02 KB

Hrm.

If you look at the new code, it's actually very clear what is happening, and why. Translating that into proper English requires a comment of 4 lines to be technically correct.

So here is that addition. I'd still prefer to commit #6.

cweagans’s picture

cweagans
He/Him
Primary language English
Location Boise, ID, USA
CreditAttribution: cweagans commented
Issue tags: +Needs backport to D7

Fixing tags per http://drupal.org/node/1517250

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

#12: drupal8.system-theme-settings-submit.12.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Please choose either #6 or #12. :)

I still don't think that the extra comment in #12 is needed, so I'd prefer to go with #6.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs review
FileSize
system_theme_settings_submit.patch947 bytes

No that still looks awkward, what's wrong with this?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Fine with me :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

#16: system_theme_settings_submit.patch queued for re-testing.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)

Thanks!

kbasarab’s picture

kbasarab CreditAttribution: kbasarab commented
FileSize
1397882-system_theme_settings_submit-21.patch1.22 KB

D7 backport attached.

kbasarab’s picture

kbasarab CreditAttribution: kbasarab commented
Status: Patch (to be ported) » Needs review
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Assigned: sun » Unassigned
Status: Needs review » Needs work

#21 seems to be direct backport of #0, but we additionally need to merge in the follow-up patch from #16. Can you do that? :)

kbasarab’s picture

kbasarab CreditAttribution: kbasarab commented
Status: Needs work » Needs review
FileSize
1397882-system_theme_settings_submit-24.patch1.44 KB

Oops. Thought I had done that. Here it is.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Needs work 
+++ b/modules/system/system.admin.inc
@@ -691,8 +691,16 @@ function _system_theme_settings_validate_path($path) {
+  $unset($values['var']);

Stray $ - unset() is function :)

pingers’s picture

pingers CreditAttribution: pingers commented
Status: Needs work » Needs review
FileSize
1397882-system_theme_settings_submit-26.patch1.44 KB

Re-roll without "$".

kbasarab’s picture

kbasarab CreditAttribution: kbasarab commented
FileSize
variable-unserialize-error-1284364-26.patch1.44 KB

This is what I get for just making a quick switch. Updated now.

EDIT: Posted at same time as pingers. Thanks for the fix pingers.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Excellent! Thanks all :)

pingers’s picture

pingers CreditAttribution: pingers commented

@kbasarab - hehe, no problem ;)

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Status: Reviewed & tested by the community » Fixed

I think this is OK for Drupal 7. In theory it could affect another submit handler which runs after this one, but the use case for checking those particular items in $form_state['values'] in a submit handler is probably pretty slim. (It would not be hard to change the patch to protect against that if we wanted, though.)

I committed the patch to Drupal 7, but because of the above issue I added it to CHANGELOG.txt also: http://drupalcode.org/project/drupal.git/commit/a6cc373

I also corrected the misspelling of "unnecessary" on commit (the Drupal 8 patch already had it right) :)

Status: Fixed » Closed (fixed)

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

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Issue tags: +7.17 release notes

Drupal 7.16 was a security release only, so this issue is now scheduled for Drupal 7.17 instead.

Fixing tags accordingly.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Issue summary: View changes

Updated issue summary.