According to Forms API reference, #default_value should work with password fields (#type => 'password') but there is no value="xyz" in the rendered HTML.

CommentFileSizeAuthor
#1 password_default_value.patch.txt1.31 KBDeFr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DeFr’s picture

Version: 4.7.1 » 4.7.2
Assigned: Unassigned » DeFr
Status: Active » Needs review
FileSize
1.31 KB

This patch actually uses the (default-)value of the password fields and put it in the html.

webchick’s picture

Hm. I would call this "by design" and update the documentation accordingly. Storing passwords in plaintext is just a horrible idea.

DeFr’s picture

In fact, being able to actually show the value in the field doesn't mean the password is stored as plaintext, but that the module is responsible of applying an encryption of the value - using mcrypt for example.

The main reason for that is that in some case, a hash simply can't be used. Concrete example : I want my users to be able to set in their profile their login / password information for a remote imap server, and then allow them to check their mail... I can't do that if I don't encrypt the password in some reversible way.

For the main reason of the patch now : in the current situation, if in a module one does

function hook_user($op, &$edit, &$account, $category = NULL) { 
  if ($op == 'form' && $category == 'account') {
    $form['foo'] = array(
      '#type' => 'textfield',
      '#default_value' => $edit['foo']);
    return $form;
  }
}

then everything works as expected and you get the value stored automatically, but should you switch to

function hook_user($op, &$edit, &$account, $category = NULL) { 
  if ($op == 'form' && $category == 'account') {
    $form['foo'] = array(
      '#type' => 'password',
      '#default_value' => $edit['foo']);
    return $form;
  }
}

then the next time you validate the form without re-filling the password field, $edit['foo'] will be blanked and the value lost. Of course, I'm not advocating using directly such a code, something around the lines of :

function hook_user($op, &$edit, &$account, $category = NULL) { 
  if ($op == 'form' && $category == 'account') {
    $form['foo'] = array(
      '#type' => 'password',
      '#default_value' => _decrypt($edit['foo']));
    return $form;
  }
  else if ($op == 'update' && $category == 'account') {
    $edit['foo'] = _encrypt($edit['foo']);
  }
}

would be more suitable.

Borek-1’s picture

2 webchick: Horrible or not, developer should be in control. There are scenarios where I just need #default_value even in password fields.

webchick’s picture

@DeFr:
"In fact, being able to actually show the value in the field doesn't mean the password is stored as plaintext, but that the module is responsible of applying an encryption of the value - using mcrypt for example."

#default_value means that value="mypassword" gets stuck in the HTML src. That's plaintext. And that happens regardless of what encryption method is ultimately used on the server. I imagine it probably gets cached in the browser as well which means it's available long after the form has been submitted.

If you still REALLY want to do this for some reason, then I would create a new element type using hook_elements called '#type' => 'insecure_password' and hook_form_alter the user login form and others to use your password field instead of the default one.

I'll leave this open for chx or one of the security people to give a final ruling, but that's my (very strong) feeling on it. No web application worth its salt should EVER put this value in plaintext, imo, and Drupal should not make it easier for people to violate this rule.

killes@www.drop.org’s picture

Status: Needs review » Closed (won't fix)

no, thanks.

yang_yi_cn’s picture

Version: 4.7.2 » 6.8
Assigned: DeFr » Unassigned
Status: Closed (won't fix) » Active

It's a really old issue, but I still feel it's not user friendly to always empty the password field when loading a form.

Think about a very common scenario, user account editing:

1. The user created an account with a name, password, and some other personal information, no problem.

2. The user logged in and try to change some personal information, with no intention to change password.

- Here comes the problem, the password field is emptied and the user cannot save the form!

We don't necessarily show the old password in plain text. In many sites, the common practice will be showing the password field with some asterisks, indicating there's a default value, and if you just save it, it will not change the value. However, in the current Drupal implementation of the password field, it's just impossible.

yang_yi_cn’s picture

Category: bug » feature
Damien Tournoud’s picture

@yang_yi_cn: the user is never forced to enter something in the password fields. I'm not seeing how this is an issue.

yang_yi_cn’s picture

Priority: Normal » Minor

@Damien Tournoud

It's interesting. I didn't notice that because I was writing my own form and trying to use a '#type' => 'password' field. Drupal core user functionality does do some special treatment, at user.module,

function user_save($account, $array = array(), $category = 'account') {
...
    foreach ($array as $key => $value) {
      if ($key == 'pass' && !empty($value)) {
        $query .= "$key = '%s', ";
        $v[] = md5($value);
      }
...

It seems that I have to do my own handling to the password filed if I want to use it.

dopry’s picture

Alernative use case would be unsetting the titles and using the default values as labels.... And that isn't all that dirty in my opinion.

ngroupp’s picture

Setting the "value" attribute of a password input field is perfectly valid XHTML. There are plenty of situations where this is acceptable. Regarding the security implications, simple password authentication is, by its very nature, insecure. In addition, without SSL there should be no expectation of security whatsoever. Obfuscating user input in the password field only protects against casual shoulder-surfers. In no way, does this "feature" improve security. It should be patched in core, as DeFr suggested above, with appropriate warnings about the perceived security risks. BTW, here is a simple workaround:

$form['my_password_field'] = array(
		'#type' => 'password',
		'#title' => t('Passsword'),
		'#description' => t('Enter a password'),				
		'#post_render' => array('mysetpass'),
		'#size' => 25
	);

(...)

function mysetpass(&$form) {	
	$form = str_replace('input type="password"', 'input type="password" value="' . variable_get(mymodule_my_password_field', NULL) . '" ', $form);
	return($form);
}

While the password should be encrypted before being stored in the db, unless the form is secured with SSL, it will have already been broadcast in the clear so it doesn't matter all that much.

p.s. You might consider an 'annoying' option in the Priority menu. :)

Status: Active » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.