remove $CleartextPassword from ChangePasswordEmail.ss

[christopherdarling]

The $CleartextPassword value is never populated as reported #3257 and in agreeance with the two comments (cc @kinglozzer) I also don't think plaintext passwords should be included in emails by default. As Loz suggests if somebody wants this it can easily be added by overriding the template.

[dhensby]

Hmm, is this really ok to go into 3.x? I think it has to be master :(

[jonom]

I think it could be considered a security patch and go in a point release if we wanted because it's pretty bad form to email plain text passwords. I hate when websites email me a copy of a password I'm otherwise careful not to record outside of an encryption app.

Also I wouldn't interpret this as a SemVer violation personally if it went to 3 branch - the API (barely) promises that users will be notified if their password is changed but doesn't specify how we do that. Plus how long has this functionality been broken? If it never worked there's no harm in removing it.

Some other ideas for this template:

Don't these messages usually include something like "If this wasn't you FREAK OUT" and maybe an IP address as well? Might be nice to add.

Should this template be checking Member.unique_identifier_field to include something other than Member.Email if appropriate? That affects what field you use to log in doesn't it (e.g. Username instead of Email)?

If we're only going to show the email address I don't see much point in emailing 'james@brown.com' and dedicating a paragraph to telling them that their email address is 'james@brown.com'. They would have just entered this in the password reset form and are using that email address to read the email after all. If we do display something other than email address if appropriate as the username then I do see value in including it.

[dhensby]

The only issue i see is of someone relies on this template and had patched the code to work. Then we'll break their site.

I agree it should not be emailed.

Happy to merge given concensus from other core members

[tractorcow]

Even if it's a security improvement, it probably still needs to be a major release if it violates semver.

The correct plaintext template is $Password, which only works at a particular point in time just before the record finishes writing (at which point it's hashed).

[dhensby]

Well, it's broken atm. So we either fix it to make it less secure or we remove it as a patch.

I vote patch

[christopherdarling]

Perhaps for 3.x we do <% if $CleartextPassword %> in the template to prevent breakage for those that have patched the code (although I would imagine most would opt for patching the template) but fixes the missing content issue for everyone else and then for 4 remove it from the template completely as all seem to agree about the security concerns

[dhensby]

Great idea

[christopherdarling]

PR updated for 3.x + updated commit message

would you like me to raise this against 4 with this removed?

[dhensby]

@tractorcow thoughts?

[tractorcow]

I agree; Fix in 3.x, remove in master.

[christopherdarling]

4.x version here #5221

[dhensby]

Personally I'd prefer template syntax on it's own lines, there's no need to keep it on the unrelated line, I'll fix while merging...

[dhensby]

thanks @christopherdarling