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.
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedHere 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.
Comment #2
pwolanin CreditAttribution: pwolanin commentedwhy is this critical?
Comment #3
chx CreditAttribution: chx commentedUnrelated 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.
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #5
marcingy CreditAttribution: marcingy commentedChanging to major as per tag.
Comment #6
MustangGB CreditAttribution: MustangGB commentedTag update
Comment #7
sun.core CreditAttribution: sun.core commentedThis is just a bug. Fix is nice to have, but definitely not important.
Comment #8
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commented#4: simpletest-http-auth-credentials-799932-4.patch queued for re-testing.
Comment #10
lotyrin CreditAttribution: lotyrin commented#4: simpletest-http-auth-credentials-799932-4.patch queued for re-testing.
Comment #11
sunComment #12
sunCan 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.
Comment #13
Stefan Freudenberg CreditAttribution: Stefan Freudenberg commentedUpdated the patch replacing the ternary operator as suggested.
Comment #14
Stefan Freudenberg CreditAttribution: Stefan Freudenberg commentedThe last patch needs review.
Comment #15
sunThanks!
Comment #16
Stefan Freudenberg CreditAttribution: Stefan Freudenberg commentedPatch applies also to 7.x
Comment #17
Stefan Freudenberg CreditAttribution: Stefan Freudenberg commentedI don't know why the status changed.
Comment #18
webchickThis 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.