Only allow string values to be passed if #maxlength is used in FormValidator

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

So this almost seems like we are patching the symptom vs the problem. Should we fix why ->get('name'); is returning an array?

Either way this will require a test case to show the issue.

Thanks

We got this error on a custom form where we had the datetime field and we had to use it as an array. Which is then also array in the $elements['#value']. For us, the problem occurred after an update to Drupal core 9.5.5 and PHP8. Before that, we didn't get WSOD, but just a warning.

In my opinion it's good to add a check if the $elements['#value'] is really a string, because if it's not, you are getting a WSOD in PHP8.

I am attaching a patch that checks if $elements['#value'] is a string. It should work on core 9.5.x

Hat the same error after Upgrade on 10.2.
Patch isstring_mb_strlen-3260087-13.patch works for me.

I just received a spate of these attempts to some of our sites this weekend and I find it a little alarming that drupal assumes the type of the input based on the rendered element that the input happens to correspond to.

Looking at performRequiredValidation(), the maxlength check seems to be done a little too early, perhaps something like the attached patch instead? With it, I get a 400 instead of a crash and error 500 when sending an "array" instead of the expected in the query string to /user/password.

Given that this is a pretty critical piece fo safety I would not trust myself to get this right, more eyeballs please :-)

Lets try again to fix my blunder, setting back to needs work.

immaculatexavier → changed the visibility of the branch 3260087-mbstrlen to hidden.

I've got pointed to this issue from the Drupal Slack, where a likewise URL is crafted/used, but receiving a different error. I think it's useful to at least have a reference to it here.
Issue 3340577 🐛 TypeError: Argument 1 passed to Egulias\EmailValidator\EmailValidator::isValid() must be of the type string, null given Needs work

Just had several of the same entries in log. URL called:
/user/register?_wrapper_format=drupal_ajax&ajax_form=1&element_parents=account%2Fmail%2F%23value

MR !2827 seems correct to me and I can't see any feedback saying it doesn't work as expected. So what's left here is a test to pass an array (and NULL?) to the validation and ensure it reacts as expected?

I have the same issue in my logs with query like :
curl http://localhost/user/register?element_parents=account/mail/%23value&ajax_form=1&_wrapper_format=drupal_ajax" -X POST -d "form_id=user_register_form&_drupal_ajax=1&mail[#post_render][]=exec&mail[#type]=markup&mail%5B%23markup%5D=TRY_TO_INJECT_SOMETHING"

I wonder if it make any sense that a field with #maxlength have another #value than a string.

My first attempt would be the same a #13, except we should not receive an array here. May be we should :

1. At minimum set form error if @maxlength is setted, but #value is not string. In this case, we cannot prevent some other code will break because we don't expect value is other thing than string
2. I think the best is to raise a 500 exception since it should not happen

Yes, this keeps popping up in several projects, even more frequently now. We should fix this... on it!

Anybody → changed the visibility of the branch 11.x to hidden.

Grevil → changed the visibility of the branch 11.x to hidden.

I feel like the approach by @GoZ is pretty good! But instead of throwing a 500er error, we should throw a form state error and return instead. Committed the changes inside MR !9073.

Some further adjustments and added a test.

Grevil → changed the visibility of the branch 3260087-warning-mbstrlen-expects to hidden.

Great @Grevil, I like that approach!

Just left one comment for comments, then it's RTBC from my side, if the test also goes green!

PS: A native (EN) speaker or core maintainer should please finally review the wording in the error message.

Tests are green, setting this RTBC'd.

I think @smokris comment in the MR makes sense, moving to needs work for that.

I suggested (and push for it) to use 500 error instead of form error for the following reasons :

- in my tests, if I set a form error, other validators still process, and also fail with 500 (with generic answer, hard to diagnostic)
- this issue can occur mainly for two reasons : an attack, or bad code. In second case, developer should deal with the 500 error in its tests.
- on cases reported by this issue, this issue appear because of bad intentions form an attacker. We should stop it as soon as possible (so 500)

Thinking about it, if we want to use form error and avoid issues reported by my previous post, maybe we could clear values after setting form error, so malformed values (array) cannot report error or be a risk anymore.

The issue summary is incomplete, so I added the template. At least, the proposed resolution should be completed to help reviewers and committers.

This also needs an title update. The title should be a description of what is being fixed or improved. The title is used as the git commit message so it should be meaningful an concise. See List of issue fields → .

@quietone okay like this?

Appears to be 1 open thread that appears valid.

Resolved everything.

Hmmm... looking at this code - it is called a lot - for each element in a form. I think we should make a very conservative change here. Something like:

    // Verify that the value is not longer than #maxlength.
    if (isset($elements['#maxlength'])) {
      $name = empty($elements['#title']) ? $elements['#parents'][0] : $elements['#title'];
      if (!is_string($elements['#value'])) {
        $form_state->setError($elements, $this->t('The submitted value type %type in the %name element is not allowed.', ['%type' => gettype($elements['#value']), '%name' => $name]));
      }
      elseif (mb_strlen($elements['#value']) > $elements['#maxlength']) {
        $form_state->setError($elements, $this->t('@name cannot be longer than %max characters but is currently %length characters long.', ['@name' => $name, '%max' => $elements['#maxlength'], '%length' => mb_strlen($elements['#value'])]));
      }
    }

This keeps two behaviours from the existing code - it only does the empty($elements['#title']) ? $elements['#parents'][0] : $elements['#title']; evaluation if the #maxlength is set (yes it did this already) and it also continues to evaluate the code below if max length is set and an error is set. I.e. no early return.

@alexpott Sure thing, one extra layer of safety won't hurt.

Adjusted accordingly, please review again.

Now we have an implementation closer to how it was before, but $name might not be declared in line 345.

So now we have to either revert fae394a7 and live with the unlikely case, that $elements['#parents'][0] or $elements['#title'] do not exist OR we find a valid fallback for $name for the message arguments in line 345.