In the settings form where Simpletest asks for optional HTTP authentication credentials, the password field is of Form API type 'textfield'. It should use 'password' instead.

(Looking through the CVS logs, it appears it did this at one point, but then the code was removed and put back into core once or twice, and somewhere along the way it got converted to a textfield...)

This isn't really a security issue, since I assume that if the site is behind HTTP authentication, the user would have already had to know these credentials in order to be able to see this administrative page in the first place. However, I'm marking this as a critical bug, because it still seems like really bad practice to display the password in plain text on the screen.

Files: 
CommentFileSizeAuthor
#13 simpletest-http-auth-credentials-799932-5.patch2.49 KBStefan Freudenberg
PASSED: [[SimpleTest]]: [MySQL] 33,661 pass(es).
[ View ]
#4 simpletest-http-auth-credentials-799932-4.patch2.53 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 31,747 pass(es).
[ View ]
#1 simpletest-http-auth-credentials-799932-1.patch2.07 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 20,361 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new2.07 KB
PASSED: [[SimpleTest]]: [MySQL] 20,361 pass(es).
[ View ]

Here is a patch to fix it.

We need to play around with things a little since 'password' form elements don't display a default value (with good reason), but it isn't that complicated for the most part.

Priority:Critical» Normal

why is this critical?

Status:Needs review» Needs work

Critical
When a bug renders a module, a core system, or a popular function unusable. Possible examples from core are the inability to create a new node or blocks won't display. These are to be fixed immediately because the software is not usable at all.

Unrelated to the issue being non-critical: At time 0 you have a u/p combination, next up , the password is removed but the username still exists so you go to Drupal to delete the pass. Whopsie. You can say I am an idiot and this is not a valid use case. Sure :) But I thought I will at least raise the concern.

Status:Needs work» Needs review
StatusFileSize
new2.53 KB
PASSED: [[SimpleTest]]: [MySQL] 31,747 pass(es).
[ View ]

I think this should be "major" but we don't have that status yet. We really shouldn't release Drupal 7 with it displaying passwords on the screen.

The case @chx brought up in #3 can be worked around by submitting the form twice (once to clear all credentials, then a second time to add back just the username). It's a little awkward, but I think acceptable for what is kind of an extreme edge case :) So, I rerolled the patch with some extra help text which explains that removing the username deletes all credentials. Maybe for Drupal 8 we need a better, more general way to deal with form elements where the password is optional... it is kind of an awkward situation.

Priority:Normal» Major

Changing to major as per tag.

Tag update

Priority:Major» Normal

This is just a bug. Fix is nice to have, but definitely not important.

Priority:Normal» Major
Issue tags:+string freeze

I don't see how displaying passwords in plain text on the screen can be anything less than a major bug.

While in many scenarios HTTP authentication passwords aren't "really" secure to begin with, they're still more secure than warrants displaying them on the screen where anyone walking by the computer can see them.

This patch changes strings, so I'm adding the "string freeze" tag.

Version:7.x-dev» 8.x-dev
Issue tags:+needs backport to D7

Status:Needs review» Needs work

+++ modules/simpletest/simpletest.pages.inc 6 Jun 2010 22:52:15 -0000
@@ -468,16 +471,37 @@ function simpletest_settings_form($form,
+    '#description' => $username && $password ? t('Leave this blank to delete both the existing username and password.') : NULL,
...
+    '#description' => $password ? t('To change the password, enter the new password here.') : NULL,

Can we use separated if conditions/control structures for these?

Aside from that, I think it's safe to commit + backport this patch. Strings in SimpleTest don't really matter.

StatusFileSize
new2.49 KB
PASSED: [[SimpleTest]]: [MySQL] 33,661 pass(es).
[ View ]

Updated the patch replacing the ternary operator as suggested.

Status:Needs work» Needs review

The last patch needs review.

Status:Needs review» Reviewed & tested by the community

Thanks!

Status:Reviewed & tested by the community» Needs review

Patch applies also to 7.x

Status:Needs review» Reviewed & tested by the community

I don't know why the status changed.

Status:Reviewed & tested by the community» Fixed

This is adding quite a few strings, all of which are highlighting the absolutely terrible UX these fields have, but I agree that this is better than showing passwords in plaintext.

Committed and pushed to 8.x and 7.x. Thanks.

Status:Fixed» Closed (fixed)
Issue tags:-string freeze, -needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.