WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 7 days ago

#43856 closed enhancement (fixed)

Include submitter IP details in password reset emails?

Reported by: cefiar Owned by: garrett-eclipse
Milestone: 5.6 Priority: normal
Severity: minor Version: 4.9.6
Component: Privacy Keywords: has-patch has-screenshots has-copy-review needs-testing
Focuses: ui-copy Cc:

Description

Could WP password reset emails include the IP of requester when someone asks for a password to be reset?

I've been seeing a lot of bots that seem to spam the password reset link (they find a username from a post, then hit the password reset link using that username), and this would make it easier to pick up and block that IP/range if it was in the reset email already, rather than having to dig through the webserver logs looking for which IP submitted the password reset request.

Note: From looking over wp-login.php this seems like it'd be fairly trivial to implement, but I wasn't sure what the best method for determining the clients IP address to use in the email template (no use creating a security hole or providing useless info), otherwise I might have included a patch.

FWIW: Google and various other sites usually report which IP either asked for the reset, or after a reset happened report that someone from that IP changed/reset the password, so basically I'm asking for similar sorts of detail from WP.

Attachments (6)

43856.diff (3.2 KB) - added by isharis 2 years ago.
43856.2.diff (4.3 KB) - added by garrett-eclipse 7 weeks ago.
Refreshed patch and included text for default privacy policy content
Screen Shot 2020-09-10 at 1.12.20 AM.png (96.3 KB) - added by garrett-eclipse 7 weeks ago.
New suggested text for the default privacy policy content
Screen Shot 2020-09-10 at 1.15.54 AM.png (100.8 KB) - added by garrett-eclipse 7 weeks ago.
Email w/ updated verbiage to include IP address
43856.3.diff (4.5 KB) - added by garrett-eclipse 2 weeks ago.
Refresh to address copy review by @bridgetwillard
43856.4.diff (2.1 KB) - added by garrett-eclipse 7 days ago.
Refresh patch to us $_SERVER['REMOTE_ADDR']

Download all attachments as: .zip

Change History (41)

#1 in reply to: ↑ description @iandunn
3 years ago

  • Keywords gdpr added

Replying to cefiar:

wasn't sure what the best method for determining the clients IP address to use in the email template (no use creating a security hole or providing useless info)

WP_Community_events::get_unsafe_client_ip() might be useful there.

Adding the gdpr keyword since this could be considered sharing "personal data" with an external system.

#2 @desrosj
2 years ago

  • Component changed from Login and Registration to Privacy

Moving to the new Privacy component.

This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.


2 years ago

#4 @desrosj
2 years ago

  • Milestone changed from Awaiting Review to 5.0

#5 @allendav
2 years ago

I like it. If we do this, the patch should also add something like this to wordpress' wp_add_privacy_policy_content call:

“If you request a reset of your password, your IP address will be included in an email to the site administrator."

#6 @desrosj
2 years ago

  • Keywords gdpr removed

Removing the GDPR keyword. This has been replaced by the new Privacy component and privacy focuses in Trac.

@isharis
2 years ago

#7 @isharis
2 years ago

  • Keywords has-patch 2nd-opinion ux-feedback added; needs-patch removed

Hello,

This is my first patch and I'd like to be involved in coming up with a solution to make it through the core. I think that the simple solution of adding IP of the form submitted in the email is one solution but what I think should happen is this:

Like twitter, password reset emails should include the device and location. This is enough information for a user.

For the admin, do admins need to get a password reset email for each user? In the case where a site admin is not getting attacked by bots, this can be annoying.

#8 @pento
2 years ago

  • Milestone changed from 5.0 to 5.1

#9 @desrosj
22 months ago

  • Milestone changed from 5.1 to 5.2

At first glance, 43856.diff needs the since annotation updated for wp_get_unsafe_client_ip() and the IP address: %s string is missing a `/* translators: */ comment.

@isharis, are you able to address that and refresh the patch?

#10 @garrett-eclipse
22 months ago

  • Keywords needs-refresh added

Applying needs-refresh due to @desrosj feedback.

This ticket was mentioned in Slack in #core-privacy by desrosj. View the logs.


21 months ago

#12 @desrosj
21 months ago

  • Owner set to garrett-eclipse
  • Status changed from new to assigned

#13 @garrett-eclipse
21 months ago

  • Milestone changed from 5.2 to Future Release
  • Version set to 4.9.6

Moving this off the milestone until I've had a chance to test and refresh

@garrett-eclipse
7 weeks ago

Refreshed patch and included text for default privacy policy content

@garrett-eclipse
7 weeks ago

New suggested text for the default privacy policy content

#14 @garrett-eclipse
7 weeks ago

  • Focuses ui-copy added
  • Keywords dev-feedback needs-privacy-review has-screenshots added; ux-feedback needs-refresh removed

Thanks for the initial patch @isharis I've refreshed it in 43856.2.diff to apply to trunk and make the following amendments;

  1. Added If you request a reset of your password, your IP address will be included in the reset email. to the default privacy policy content as suggested by @allendav.
  2. Addressed the comments by @desrosj updating to 5.6.0 and adding translator comment.
  3. Updated the verbiage in the email as just 'IP Address' felt like it could be confused with the website IP. Verbiage used This password reset request originated from the IP address %s.
  4. I made it conditional so if wp_get_unsafe_client_ip returns false the string isn't added.

Adding dev-feedback/needs-privacy-review as I feel we don't need to anonymize the IP in this context as this is a security measure so would fall into section f of the GDPR. The full IP is more useful in ensuring identity in this case. I didn't remove the anon_ip portion yet as I'd like some input on that.

@garrett-eclipse
7 weeks ago

Email w/ updated verbiage to include IP address

#15 @garrett-eclipse
7 weeks ago

  • Milestone changed from Future Release to 5.6

#16 follow-up: @garrett-eclipse
5 weeks ago

  • Keywords needs-copy-review added

#17 in reply to: ↑ 16 @bridgetwillard
2 weeks ago

Hi, @garrett-eclipse

I'm very happy to help edit the copy in the screenshot. Are you working in a Google Doc?

Replying to garrett-eclipse:

#18 follow-up: @garrett-eclipse
2 weeks ago

Hi @bridgetwillard we're just looking at two strings here so any suggestions can just be made on the ticket as a comment.

Specifically we're looking at introducing this string to the Privacy Policy Guide;

'If you request a reset of your password, your IP address will be included in the reset email.'

https://core.trac.wordpress.org/raw-attachment/ticket/43856/Screen%20Shot%202020-09-10%20at%201.12.20%20AM.png

And the password reset email;

'This password reset request originated from the IP address %s.'

Note: The %s is the IP address of password reset requestor.
https://core.trac.wordpress.org/raw-attachment/ticket/43856/Screen%20Shot%202020-09-10%20at%201.15.54%20AM.png

I greatly appreciate the review/input.

#19 in reply to: ↑ 18 @bridgetwillard
2 weeks ago

Thank you so much @garrett-eclipse. I saw the .diff files but it's not GitHub so I wasn't sure how to make the suggestion.

Suggested Edit

"Note: if you request a password reset, your IP Address will be included in the reset email."

I think the second phrase is succinct and well-said.

PS About The Word "Just"

I would eliminate the word "just" in the screenshot (not this string) that says "If this was a mistake, just ignore it..."

Replying to garrett-eclipse:

Hi @bridgetwillard we're just looking at two strings here so any suggestions can just be made on the ticket as a comment.

Specifically we're looking at introducing this string to the Privacy Policy Guide;

'If you request a reset of your password, your IP address will be included in the reset email.'

https://core.trac.wordpress.org/raw-attachment/ticket/43856/Screen%20Shot%202020-09-10%20at%201.12.20%20AM.png

And the password reset email;

'This password reset request originated from the IP address %s.'

Note: The %s is the IP address of password reset requestor.
https://core.trac.wordpress.org/raw-attachment/ticket/43856/Screen%20Shot%202020-09-10%20at%201.15.54%20AM.png

I greatly appreciate the review/input.

#20 @garrett-eclipse
2 weeks ago

  • Keywords has-copy-review needs-refresh added; needs-copy-review removed

Excellent, thanks @bridgetwillard for the copy review, great points, will refresh the patch when I have a chance.

@garrett-eclipse
2 weeks ago

Refresh to address copy review by @bridgetwillard

#21 @garrett-eclipse
2 weeks ago

  • Keywords dev-feedback 2nd-opinion needs-refresh removed

This ticket was mentioned in Slack in #core-privacy by hellofromtonya. View the logs.


13 days ago

#23 @garrett-eclipse
9 days ago

  • Keywords needs-privacy-review removed
  • Status changed from assigned to accepted

Removing needs privacy review as I feel I gave that initially. We're sending the users own IP back to them, and if it's not their then it's a flag that it's a potentially malicious action. As this is a improvement on the security of the action I see that falling under section (f) 'Legitimate Interests' of the GDPR.

Last edited 9 days ago by garrett-eclipse (previous) (diff)

This ticket was mentioned in Slack in #core-privacy by hellofromtonya. View the logs.


8 days ago

#25 @garrett-eclipse
8 days ago

  • Keywords needs-testing added

Aside from a little testing this is ready to go.

#26 follow-up: @SergeyBiryukov
8 days ago

If this was a mistake ignore this email and nothing will happen.

Just noting that it seems better to me with a comma:

If this was a mistake, ignore this email and nothing will happen.

#27 in reply to: ↑ 26 @garrett-eclipse
8 days ago

Replying to SergeyBiryukov:

If this was a mistake ignore this email and nothing will happen.

Just noting that it seems better to me with a comma:

If this was a mistake, ignore this email and nothing will happen.

Thanks @SergeyBiryukov I was on the fence about that, do you want me to refresh or will you handle as part of commit?

#28 @bridgetwillard
8 days ago

Generally a comma is used when the phrase can be taken out and the meaning remains the same.

In this case, I think the comma isn’t needed.

Just my two cents.

#29 @carike
7 days ago

  • Keywords needs-testing removed

Tested on a single site.
The IP is included in the password reset e-mail.

This ticket was mentioned in Slack in #core-privacy by carike. View the logs.


7 days ago

#31 @hellofromTonya
7 days ago

  • Keywords commit added

This ticket was mentioned in Slack in #core by helen. View the logs.


7 days ago

#33 @ocean90
7 days ago

  • Keywords needs-refresh added; commit removed

As discussed on Slack, 43856.3.diff should be simplified to only use $_SERVER['REMOTE_ADDR'] instead of introducing wp_get_unsafe_client_ip().

@garrett-eclipse
7 days ago

Refresh patch to us $_SERVER['REMOTE_ADDR']

#34 @garrett-eclipse
7 days ago

  • Keywords needs-testing added; needs-refresh removed

I've refreshed in 43856.4.diff to use $_SERVER['REMOTE_ADDR']. Please retest.

#35 @helen
7 days ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 49255:

Privacy: Add requester IP to password reset emails.

Props garrett-eclipse, bridgetwillard, isharis, ocean90.
Fixes #43856.

Note: See TracTickets for help on using tickets.