Make WordPress Core

Opened 7 years ago

Closed 2 weeks ago

Last modified 2 weeks ago

#42957 closed defect (bug) (fixed)

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

Reported by: paulcline's profile paulcline Owned by: dmsnell's profile dmsnell
Milestone: 6.7 Priority: normal
Severity: normal Version:
Component: Users Keywords: has-patch has-unit-tests needs-dev-note dev-reviewed
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 7 years ago.
42957-password-reset-username-ending-in-period-v2.diff (1.6 KB) - added by paulcline 7 years ago.
42957.diff (3.4 KB) - added by daveagp 3 years ago.
Diff generated from github pull request

Download all attachments as: .zip

Change History (46)

#1 @paulcline
7 years ago

  • Keywords has-patch added

#2 @paulcline
7 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
7 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
6 years ago

  • Version trunk deleted

#5 @daveagp
4 years 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.


3 years ago
#6

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
3 years ago

Diff generated from github pull request

#7 @daveagp
3 years 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
3 years ago

  • Keywords has-unit-tests added

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


3 years ago

#10 @markparnell
3 years ago

  • Milestone changed from Awaiting Review to 5.8

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

#11 @hellofromTonya
3 years 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 years 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 years ago

  • Keywords needs-dev-note added

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


3 years ago

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


3 years ago
#15

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

hellofromtonya commented on PR #1095:


3 years ago
#16

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

#17 @hellofromTonya
3 years 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 years 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.

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


2 years ago

#20 @costdev
2 years ago

Per the discussion on the bug scrub, pinging @hellofromTonya to see if you've had time to do further investigation on a possible alternative approach?

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


2 years ago

#22 @costdev
2 years ago

  • Milestone changed from 6.0 to Future Release

Per the discussion in the bug scrub, I'm moving this to Future Release as it still needs investigation on a possible alternative approach.

As the investigation isn't already scheduled, let's keep this off the 6.1 milestone until it progresses towards a resolution.

Last edited 2 years ago by costdev (previous) (diff)

#23 @daveagp
2 years ago

Hi, what investigation is needed exactly? I would love to get this fixed ASAP.

Why is this code needed? Gmail auto-hyperlinking ignores periods at the end of plaintext links.

Why does it do this? My understanding is that it's a combination of two accessibility features:

(1) Gmail automatically turns plaintext links into hyperlinks. So you can click on even plaintext links, instead of copying and pasting.

(2) It assumes that if someone writes a plaintext link with a period at the end, it's meant as punctuation. E.g. "To blog, go to wordpress.com." The implied link is not wordpress.com. but it is wordpress.com instead.

The proposed fix changes the wordpress-generated plaintext link to one which doesn't fall into this accessibility interpretation. Of course, nothing is currently broken for people who copy-and-paste the URL, but this is not what most people do (can confirm as, in my volunteer role for cscircles.cemc.uwaterloo.ca I get lots of support requests from teachers and students who are running into this deficit).

I happen to work at Google (not on gmail) but if that is what is needed to move this forward I can even try to get any specific questions you have answered from folks working on gmail.

Thanks!

#24 @hellofromTonya
4 months ago

  • Owner hellofromTonya deleted

Clearing me as the ticket owner, as I'm no longer am a sponsored contributor. Clearing ownership opens the ticket for someone to step in to shepherd this ticket forward to resolution.

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


3 months ago

#26 @SergeyBiryukov
3 months ago

  • Milestone changed from Future Release to 6.6

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


3 months ago

#28 @hmbashar
6 weeks ago

  • Keywords needs-testing added

#29 @oglekler
6 weeks ago

  • Keywords needs-testing removed

@hmbashar I don't think that current patch is ready for testing, I assume it needs more work to be done first.

#30 @daveagp
6 weeks ago

Hi, I'm the author of the patch and tests but would love to help however I can. I've tested on my own machine the link generation and link functionality. If more details would help or there's other tasks I can contribute to please let me know.

#31 @oglekler
6 weeks ago

@daveagp last PR has a comment that isn't addressed. Please check it out.

I also wonder if

$url = str_replace('.', '%2E', $url);

will be more logical.

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


5 weeks ago
#32

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 (technically second but it's a fix to the first), thanks in advance for telling me if any more steps are needed.

#33 @daveagp
5 weeks ago

Thank you @oglekler !

Ok, I didn't exactly know the details of comments on the second PR which copied mine but from the right trunk. I've tried again in https://github.com/WordPress/wordpress-develop/pull/6834/files and renamed it according to a comment I saw. I didn't get a notice of the comments on the second PR since it wasn't mine but hopefully I'll see any on this 3rd one.

The comments on 2nd PR also say "If this function remains, it should be a core-only private function". I don't know if that is already true or not, but if important, I'd need a hint on what concretely to change, thank you in advance.

I agree that str_replace would work, but given that periods mid-user name are extremely common and unproblematic for all email clients (I've got many user reports from the trailing period but none from any other position), my thinking is not to unnecessarily create mumbo jumbo in their invitation emails.

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


4 weeks ago

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


3 weeks ago

#36 @nhrrob
3 weeks ago

  • Milestone changed from 6.6 to 6.7

We are very close to RC1.
Hopefully it can make into 6.7

#37 @dmsnell
3 weeks ago

Here are a couple of thoughts on this:

It seems like a very specific failure and not entirely a bug in WordPress, as WordPress is properly generating links. It's the email clients that misinterpret them, so this is more like a way for WordPress to interoperate better in the ecosystem. To that end I don't know if it makes a lot of sense to introduce a new unclear abstraction in functions.php. Maybe what would do well here is a very specific targeted fix that doesn't raise other questions.

I also wonder if

$url = str_replace('.', '%2E', $url);

will be more logical.

For example, this will break almost every URL that passes through it, by replacing the actual dots in the host part of the URL with percent-escapes. There are also problems with running URLs through rawurlencode(): for one, it's not safe to run the entire URL through that function; for two, it will double-escape and corrupt anything that was already escaped.


So now I wonder if there is a simpler way to address this that resolves the issue without raising framework level questions. In the third patch there are two changes to generated URLs but only one of the URLs has the problem in various email clients. Each URL contains a query arg action=rp

What if instead of all of this, we simply moved the login=" . rawurlencode( $user_login ) . " part to the front and ended each URL with &action=rp? There would be no conflict, the exact issue would be avoided, and the change would require essentially no review because there are no new overarching questions or API design work involved.

-	$message .= network_site_url( "wp-login.php?action=rp&key=$key&login=" . rawurlencode( $user->user_login ), 'login' ) . "\r\n\r\n";
+	$message .= network_site_url( "wp-login.php?login=" . rawurlencode( $user->user_login ) . "&key=$key&action=rp", 'login' ) . "\r\n\r\n";

@daveagp commented on PR #6834:


3 weeks ago
#38

I've reimplemented using the nice solution from https://core.trac.wordpress.org/ticket/42957#comment:37. Thanks @dmsnell for the review and thoughtful suggestion!

#39 @dmsnell
2 weeks ago

@daveagp can you reconfirm the problem and that the latest patch resolves it? I'd love to see the generated URLs used in your test. I think that it's safe to merge after that confirmation because the latest patch involves no framework-level changes and no broader questions: it's simply avoiding the problem altogether.

#40 @daveagp
2 weeks ago

Sorry for the delay - I have now confirmed it works end to end. Here is an sanitized example url it generated during account creation:

https://cscircles.cemc.uwaterloo.ca/dev/wp-login.php?login=testuser.&key=fake&action=rp

Gmail was able to understand it and present it to the user correctly

#41 @dmsnell
2 weeks ago

  • Owner set to dmsnell
  • Resolution set to fixed
  • Status changed from assigned to closed

In 58674:

Users: Avoid ambiguous password reset URLs for usernames ending in a period.

When WordPress sends out a password-reset or new-user email, it generates
a link for someone to follow in order to take them to the reset page. If
the user login name ends in a period, however, that generated URL will
end in a period and many email clients will confuse it with a
sentence-ending period instead of being part of the query arguments.

In this patch, the generated URL's query argument are rearranged so that
the link will never end in a period. Alternative ideas were explored to
create a new function to escape URL-ending periods, but this patch resolves
the reported problem without raising any further architectural questions.

Developed in https://github.com/WordPress/wordpress-develop/pull/6834
Discussed in https://core.trac.wordpress.org/ticket/42957

Props audrasjb, costdev, daveagp, dmsnell, hellofromTonya, markparnell, mukesh27, nhrrob, obrienlabs, paulcline.
Fixes #42957.

#43 @dmsnell
2 weeks ago

  • Keywords dev-reviewed added; dev-feedback removed
Note: See TracTickets for help on using tickets.