WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 2 hours ago

#42957 closed enhancement (invalid)

Usernames ending in a period generate invalid reset password links in certain email clients

Reported by: paulcline Owned by: hellofromTonya
Milestone: 6.0 Priority: normal
Severity: normal Version: 5.3.2
Component: Users Keywords: has-patch has-unit-tests needs-dev-note dev-feedback
Focuses: Cc:

Description

Password reset links contain the username appended to the end of the URL. If the user name ends in a period the email client has to decide if the period is part of the URL or part of the punctuation of the sentence. For example:

<https://some-wordpress-site.com/wp-login.php?action=rp&key=V4LSmgBcwtqvFPEiFt0e&login=p.o.>

Gmail generates a clickable link that stops short of the final period. Outlook successfully links the entire URL.

Attachments (3)

42957-password-reset-username-ending-in-period.diff (870 bytes) - added by paulcline 4 years ago.
42957-password-reset-username-ending-in-period-v2.diff (1.6 KB) - added by paulcline 4 years ago.
42957.diff (3.4 KB) - added by daveagp 9 months ago.
Diff generated from github pull request

Download all attachments as: .zip

Change History (22)

#1 @paulcline
4 years ago

  • Keywords has-patch added

#2 @paulcline
4 years ago

Periods are valid in URLs, but we can avoid the issue by forcing "." to encode to "%2E" when generating the link in the email. PHP automatically converts the "%2E" back to "." when it's passed into the receiving side.

#3 @obrienlabs
4 years ago

It looks like this type of email can be sent out in 2 scenarios that I can find.

  1. New user email with a link to access their account
  2. Password reset.

I can confirm that both are broken for me in GMail, too. When clicking the link in GMail I get invalid token. I tested with the thick Outlook client and outlook.com webmail and both of those worked fine. Seems isolated to GMail. Given how many people use gmail this seems like a good one to fix.

Your initial patch fixed the Reset Password process with a test user.

Could you update the patch for the new user email process as well?

#4 @pento
3 years ago

  • Version trunk deleted

#5 @daveagp
16 months ago

It would be great to fix this bug, which I discovered as the source of pain for my users for years (and me, since I usually have to do a manual reset for them).

I see it's been dormant for a while, but I also see the fix. How can I help this fix go in?

This ticket was mentioned in PR #1095 on WordPress/wordpress-develop by daveagp.


9 months ago

On my website we get about one email a month from a person who can't create an account. This turns out to be caused by https://core.trac.wordpress.org/ticket/42957

  • you're using gmail
  • you try to create an account name ending in a period, e.g. Dave P.
  • the email is sent containing https://blah...&login=Dave%20P.
  • gmail generally assumes links ending in periods have the periods added by humans and thus ignores them
  • so clicking on it, you arrive at https://blah...&login=Dave%20P and the user's told the link is invalid

Fixes this by encoding the trailing period as an entity.

This is my first submitted pull request to WP, thanks in advance for telling me if any more steps are needed.

Trac ticket: https://core.trac.wordpress.org/ticket/42957

@daveagp
9 months ago

Diff generated from github pull request

#7 @daveagp
9 months ago

@spacedmonkey1 Attempting to ping you since I see you listed as a component maintainer on Users and I'm not sure what steps might be needed to get this reviewed

#8 @daveagp
9 months ago

  • Keywords has-unit-tests added

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


9 months ago

#10 @markparnell
9 months ago

  • Milestone changed from Awaiting Review to 5.8

Moving to 5.8 to see if we can close this out.

#11 @hellofromTonya
6 months ago

  • Milestone changed from 5.8 to 5.9

Today is 5.8 Beta 1. Ran out of time for this ticket to land. Punting to 5.9.

#12 @audrasjb
3 weeks ago

I committed some changes to the Doc Block in the proposed PR, and I fixed the conflicts against trunk: https://github.com/WordPress/wordpress-develop/pull/1095

#13 @audrasjb
3 weeks ago

  • Keywords needs-dev-note added

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


3 weeks ago

This ticket was mentioned in PR #1896 on WordPress/wordpress-develop by hellofromtonya.


3 weeks ago

Supersedes PR #1095 which was based on master instead of trunk. trunk has the polyfill and latest updates.

Trac ticket: https://core.trac.wordpress.org/ticket/42957

#16 @prbot
3 weeks ago

hellofromtonya commented on PR #1095:

Closing as this PR is based on master instead of trunk. PR #1896 supersedes it.

#17 @hellofromTonya
3 weeks ago

  • Keywords dev-feedback added
  • Milestone changed from 5.9 to 6.0
  • Owner set to hellofromTonya
  • Status changed from new to assigned

I'm not sure about the current approach to add a separate function. Rather, I'm wondering why this happens and if there are other ways to resolve it.

With 5.9 Beta 1 tomorrow, punting to 6.0 and assigning to myself to do the further investigation.

#18 @daveagp
3 weeks ago

IIRC the problem is in Gmail handling of urls. It assumes that if you end a url in a period, you actually mean the period as the end of a sentence. My hope is to add this compatibility for Gmail which at the same time is not harming any other service. Please let me know if there's anything I can do, because this is a pain for my users and causes toil for me that I would like to eliminate.

#19 @mobiseo
2 hours ago

  • Resolution set to invalid
  • Status changed from assigned to closed
  • Type changed from defect (bug) to enhancement
  • Version set to 5.3.2

My issue: Facebook for WooCommerce requires WooCommerce version 3.5.0 or higher. Please update WooCommerce to the latest version, or download the minimum required version »

My site here: dailymagazinenews.com

Note: See TracTickets for help on using tickets.