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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

Status: Active » Needs review
FileSize
2.07 KB

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.

pwolanin’s picture

Priority: Critical » Normal

why is this critical?

chx’s picture

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.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
2.53 KB

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.

marcingy’s picture

Priority: Normal » Major

Changing to major as per tag.

MustangGB’s picture

Tag update

sun.core’s picture

Priority: Major » Normal

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

David_Rothstein’s picture

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.

David_Rothstein’s picture

lotyrin’s picture

sun’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
sun’s picture

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.

Stefan Freudenberg’s picture

Updated the patch replacing the ternary operator as suggested.

Stefan Freudenberg’s picture

Status: Needs work » Needs review

The last patch needs review.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Stefan Freudenberg’s picture

Status: Reviewed & tested by the community » Needs review

Patch applies also to 7.x

Stefan Freudenberg’s picture

Status: Needs review » Reviewed & tested by the community

I don't know why the status changed.

webchick’s picture

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.