Ensure user name is a string when validating

I got it too
PDOException: SQLSTATE[42000]: Syntax error or access violation: 1582 Incorrect parameter count in the call to native function 'LOWER': SELECT name FROM {users} WHERE LOWER(mail) = LOWER(:name_0, :name_1); Array ( [:name_0] => test3 [:name_1] => test ) in logintoboggan_user_login_validate() (line 551 [...]

I've seen these recently. The code doesn't have the same structure. Here are the queries with LOWER in them in the code:

SELECT name FROM {users} WHERE LOWER(mail) = LOWER(:name)

SELECT uid FROM {users} WHERE LOWER(mail) = LOWER(:mail) AND uid <> :uid

SELECT uid FROM {users} WHERE LOWER(name) = LOWER(:name) AND uid <> :uid

I don't know how they are injecting a name_0 and name_1 in there.

Can the data be checked to see if it's an array before using it?

e.g. instead of:

function logintoboggan_user_login_validate($form, &$form_state) {
  if (isset($form_state['values']['name']) && $form_state['values']['name']) {
    if ($name = db_query("SELECT name FROM {users} WHERE LOWER(mail) = LOWER(:name)", array(
      ':name' => $form_state['values']['name'],
    ))->fetchField()) {
      form_set_value($form['name'], $name, $form_state);
    }
  }
}

use:

function logintoboggan_user_login_validate($form, &$form_state) {
  if (isset($form_state['values']['name']) && $form_state['values']['name'] && !is_array($form_state['values']['name'])) {
    if ($name = db_query("SELECT name FROM {users} WHERE LOWER(mail) = LOWER(:name)", array(
      ':name' => $form_state['values']['name'],
    ))->fetchField()) {
      form_set_value($form['name'], $name, $form_state);
    }
  }
}

This would best be done at the field level with a validate function on 'name', but for now
the validate function should at least throw an error.

<?php
/**
 * Custom validation for user login form
 *
 * @ingroup logintoboggan_form
 */
function logintoboggan_user_login_validate($form, &$form_state) {
  if (isset($form_state['values']['name']) && $form_state['values']['name']) {
    if (!is_string($form_state['values']['name'])) {
      form_set_error('name', t('Invalid username.'));
    }
    if ($name = db_query("SELECT name FROM {users} WHERE LOWER(mail) = LOWER(:name)", array(
      ':name' => $form_state['values']['name'],
    ))->fetchField()) {
      form_set_value($form['name'], $name, $form_state);
    }
  }
}

?>

In the log I have:
Warning: mb_strlen() expects parameter 1 to be string, array given in drupal_strlen() (Zeile 482 von ...../includes/unicode.inc).
before the PDOException: message.

I always have mb_strlen errors in conjunction with these as well:

Warning: mb_strlen() expects parameter 1 to be string, array given in drupal_strlen() (line 478 of /.../code/includes/unicode.inc).

Regarding #4, that seems like a sane approach. Can you make a patch?

Pest the tatch.

+++ b/logintoboggan.module
@@ -545,6 +545,9 @@ function logintoboggan_user_register_submit($form, &$form_state) {
+      form_set_error('name', t('Invalid username.'));
+    }
     if ($name = db_query("SELECT name FROM {users} WHERE LOWER(mail) = LOWER(:name)", array(

Shouldn't this be an elseif? i.e. We don't want the query to run if it's invalid.

Good catch. Or maybe just return FALSE to prevent any other validation? I see no point in even continuing.

Note that it could also be done at the field level, something like this which is not tested and I'm not sure a return FALSE stops the form validation from running. If this is the preferred approach, then we'd could use it on other forms like login block, etc.

<?php
// Note: untested code. For illustration purposes only.

function logintoboggan_element_validate_string($element, &$form_state) {
  $value = $element['#value'];
  if (empty($value) || !is_string($value)) {
    $title = !empty($element['#title']) ? $element['#title'] : t('Username');
    form_error($element, t('%name is invalid.', array('%name' => $title)));
    return FALSE;
  }
}

// alter forms like so
function logintoboggan_form_user_login_form_alter(&$form, &$form_state) {
  if (isset($form['account']['name'])) {
    if (empty($form['account']['name']['#element_validate'])) {
      $form['account']['name']['#element_validate'] = array();
    }
    $form['account']['name']['#element_validate'][] = 'logintoboggan_element_validate_string';
  }
}
?>

Looks like this module is getting some attention now, so how about we push for this:

<?php

function logintoboggan_user_login_validate($form, &$form_state) {
  if (!empty($form_state['values']['name']) && is_string($form_state['values']['name'])) {
    $submitted_name = strtolower($form_state['values']['name']);
    if ($name = db_query("SELECT name FROM {users} WHERE LOWER(mail) = :name", array(
      ':name' => $submitted_name,
    ))->fetchField()) {
      form_set_value($form['name'], $name, $form_state);
    }
  }
}

?>

Note that also placeholder value ':name' is now preprocessed via PHP strtolower() vs. the current LOWER(:name) SQL syntax.

The patch from #8 I think looks good enough. From #11, I'm a bit confused of the change from LOWER(:name) to strtolower(). I'm not sure why is it necessary to convert. Anyway, attached the patch without the change to strtolower().

Just to point out that patch #12 only lets the code continue to run until the same error will happen down the line if an array is passed as 'name'. So ultimately it would need to also be addressed in core for the error to completely vanish.

from user_login_authenticate_validate()

<?php
  if (!empty($form_state['values']['name']) && !empty($password)) {
    // ...
    $account = db_query("SELECT * FROM {users} WHERE name = :name AND status = 1", array(':name' => $form_state['values']['name']))->fetchObject();
    if ($account) {
    // ...
    }
    //...
  }
?>