Posted by Borek on June 6, 2006 at 9:19am
| Project: | Drupal core |
| Version: | 6.8 |
| Component: | forms system |
| Category: | feature request |
| Priority: | minor |
| Assigned: | Unassigned |
| Status: | active |
Issue Summary
According to Forms API reference, #default_value should work with password fields (#type => 'password') but there is no value="xyz" in the rendered HTML.
Comments
#1
This patch actually uses the (default-)value of the password fields and put it in the html.
#2
Hm. I would call this "by design" and update the documentation accordingly. Storing passwords in plaintext is just a horrible idea.
#3
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
<?phpfunction 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
<?phpfunction 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 :
<?phpfunction 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.
#4
2 webchick: Horrible or not, developer should be in control. There are scenarios where I just need #default_value even in password fields.
#5
@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.
#6
no, thanks.
#7
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.
#8
#9
@yang_yi_cn: the user is never forced to enter something in the password fields. I'm not seeing how this is an issue.
#10
@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,
<?phpfunction 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.
#11
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.
#12
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. :)