[core] Posting an array as value of a form element is allowed even when a string is expected (and bypasses #maxlength constraints) - first step: text fields

I should not get commit credit - I commented, but didn't work on the patch specifically.

This makes a lot of sense. I had thought about how to fix this problem in the light of the SA but did not come up with a solution. A #value_callback is a very elegant solution and is exactly the right spot. Nice!

Leaving for some others to review, but to me this is fine.

If contrib or custom modules have set their own #value_callback's this would not conflict (all though they of course would not get the added security) so the only impact I see with this would be contrib modules relying on arrays to be able to be input into textfields and textareas. Which I doubt anyone realized was possible before the SA and doubt even more anyone is actively expecting.

What is seemingly not supported in the patch is passing e.g. integers into a textarea (not a textfield) programmatically. I don't think this is a very common use-case either, so from my point of view this would be good to go.

ahh yes, I missed that this was already marked RTBC. awesome

I think the point about integers being possible in a programmatically-submitted textarea is a good one, actually. (Also, what about password confirm elements, where it might be more likely?) Let's not break any such code, if it exists. Here's a reroll that allows scalars in those cases, plus adds tests for that. I also fixed a few other minor things.

---

A little more background on the security team discussion:

1. For the fundamental issue here (arrays being allowed where string are expected) we could not think of any direct security implications.
2. The #required constraint is not actually being bypassed here (if you manage to POST an empty array it would be rejected). You can submit array('') and get it through, but that's not actually empty, so it's no worse than the point above.
3. For the #maxlength constraint, again we couldn't think of any security implications (basically you can submit an arbitrarily large array, but the fact that it's an array is the biggest problem anyway).

We decided we can go ahead and fix this for the core text-like fields via the patches here first, to make some progress. But the same problem seems to exist for any other form element that expects a string (regardless of whether it's a text-like field or not). So as a followup, unless we want every form element in Drupal core and contrib to deal with this via a custom #value_callback we talked about some other ideas:

1. Fix the #maxlength issue specifically via a more direct fix (make the form API validation do something sensible if #maxlength is applied to an array element).
2. Add an "#input_type" key (or similar) to the form API that allowed you to specify whether string vs. array is expected, to handle the simplest cases (rather than repeating the same validation in each #value_callback).
3. As a fully automatic solution, keep a running tally of what goes through drupal_attributes() as the form is being rendered (looking for 'name' attributes), then compare that to $_POST when the form is submitted. So that for example if the form had <input name="some_name[some_key]"> on it, we expect $_POST['some_name']['some_key'] to be a string (if it's present at all) but not an array.

   This might be an ugly and fragile solution due to Drupal's architecture, but technically it's the correct, comprehensive fix - it ensures that no one messes with the array vs. string structure of any part of the form as it was built server-side.

Tagging as a reminder.

Committed to 7.x - thanks!

So... this goes to Drupal 8 now to forward-port there first? (I'm not sure what the prospects of getting this committed to Drupal 6 are.)

Updating tags, since 7.35 was a security release instead.

Decided to add a change notice here, since there are some recommended changes as a result of this that probably apply to contrib modules who define their own form element types, or individual form elements with value callbacks, etc.

https://www.drupal.org/node/2462723

Testing :)

This caused a regression with input handling for textareas, see #2502263: Drupal 7.36 regression: hidden field textarea #default_value is ignored.

1. +++ b/includes/form.inc
   @@ -1311,6 +1322,26 @@ function form_type_select_value($form, $edit = FALSE) {
   +function form_type_textarea_value($form, $edit = FALSE) {
   +  if ($edit !== FALSE) {
   +    // This should be a string, but allow other scalars since they might be
   +    // valid input in programmatic form submissions.
   +    return is_scalar($edit) ? (string) $edit : '';
   +  }
   +}
   

   I kinda like the explicit return NULL; for example

2. +++ b/includes/form.inc
   @@ -1326,9 +1357,12 @@ function form_type_select_value($form, $edit = FALSE) {
   +    }
   +    return str_replace(array("\r", "\n"), '', (string) $edit);
   

   Mh out of scope, but I'm curious why we need to str_replace() here

Since this issue is now only about Drupal 6, moving to the D6LTS queue and re-opening.

Put [core] into the title for easier sorting in this queue

In #7 it was mentioned:

> For the fundamental issue here (arrays being allowed where string are expected) we could not think of any direct security implications.

And I don't see anything else in this thread that suggests otherwise, so I think it makes sense to demote this.

Using patch in #23 for sometime without problems on php 7.3

Continuing to use this for sometime without problems. Is it time to reconsider for commit?