-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a redirect middleware to redirect /log-in/lostpassword for wordpress.com #98390
base: trunk
Are you sure you want to change the base?
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~73 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the feedback! I tested WordPress.com, Woo, and Blaze Pro. It works as expected. 👍
Fixes #96291
Proposed Changes
This PR adds a redirect function called
redirectLostPassword
to redirect WordPress.com users towp-login.php?action=lostpassword
.Why are these changes being made?
You can read more details in #96291, but in short WooCommerce and Blaze Pro use
/log-in/lostpassword
for password resets. WordPress.com does not. Much of the functionality was introduced #68885, although originally limited to WooCommerce clients only. A regression was introduced in #93163 which exposed the "Forgot Password" URL on WordPress.com. That regression was fixed in #96506.However, since the route is public to all Calypso clients, WordPress.com users can still navigate to that URL. If they navigate to that page, the styles and functionality are broken.
Considerations
This may be a naive approach.
It makes the assumption that
blazepro.tumblr.com
andwoocommerce.com
will exist in theredirect_to
portion of the query. Should either of those clients update their redirect URLs to not include their domains, those users will be redirected towp-login.php?action=lostpassword
.Ideally, the context would already be set, but I don't see that happening anywhere yet.
Alternatively, I could have updated WooCommerce and Blaze Pro's dynamic routes to identify themselves better but thought that may introduce more risk and I'm not that familiar with the codebase yet.
I'm open to other ideas.
Testing Instructions
Note: I was only able to set up testing by mapping WordPress.com to local calypso.
So assuming that configuration:
Test Default WordPress.com reset
/log-in/
on WordPress.com.wp-login.php?lostpassword
.Test Default WordPress.com reset redirect
wordpress.com/log-in/lostpassword
wp-login.php?lostpassword
.Test WooCommerce.com reset
wordpress.com/log-in/lostpassword
Test Blaze Pro reset
wordpress.com/log-in/lostpassword
Pre-merge Checklist