[Blueprints] Reload after autologin to set login cookies during boot #1914
Conversation
bgrgicak
added
[Type] Bug
Co-authored-by: Alex Kirk <akirk@users.noreply.github.com>
| * The redirect should only run if the current PHP request is | ||
| * a HTTP request. If it's a PHP run, there is nothing to reload. | ||
| */ | ||
| if (!empty($_SERVER['REQUEST_URI'])) { |
There was a problem hiding this comment.
@adamziel I would love to get your thoughts on this.
If the current script execution is a PHP run and not an HTTP request, there is no way for us to redirect.
But, there is also no way for us to set the cookies correctly. Should we just skip autologin during PHP runs?
Is there a better way of detecting this then checking if the $_SERVER global is populated?
bgrgicak
force-pushed
the
fix/set-cookies-after-autologin
branch
from
200eb6e to
11d4345
Compare
Safari bug recapIn Safari the WordPress iframe stopped loading the WordPress page in this commit (and older). The network tab didn't show any scoped requests like After some research, it turns out the issue was in the URL used in |
| /** | ||
| * The redirect should only run if the current PHP request is | ||
| * a HTTP request. If it's a PHP CLI run, we can't login the user | ||
| * because logins require cookies which aren't available in the CLI. | ||
| * | ||
| * Currently all Playground requests use the "cli" SAPI name | ||
| * to ensure support for WP-CLI, so the best way to distinguish | ||
| * between a CLI run and an HTTP request is by checking if the | ||
| * $_SERVER['REQUEST_URI'] global is set. | ||
| * | ||
| * If $_SERVER['REQUEST_URI'] is not set, we assume it's a CLI run. | ||
| */ | ||
| if (empty($_SERVER['REQUEST_URI'])) { |
| @@ -131,27 +147,42 @@ export async function setupPlatformLevelMuPlugins(php: UniversalPHP) { | |||
| if (!$user) { | |||
| return; | |||
| } | |||
| /** | |||
| * This approach is described in a comment on | |||
| * Both WordPress home url and REQUEST_URI may include the site scope | ||
| * subdirectory. | ||
| * To prevent the redirect from including two scopes, we need to | ||
| * remove the scope from the REQUEST_URI if it's present. |
There was a problem hiding this comment.
How can we remove this special case? WordPress sites installed in a subdirectory seem to support redirects just fine, e.g. https://w.org/playground. What are they doing differently than this function?
There was a problem hiding this comment.
The home_url will always include the scope subdirectory.
REQUEST_URI might sometimes include it and other times not. I need to check why it's not consistent.
There was a problem hiding this comment.
It's possible to make unscoped requests to PHP using the request handler.
For example this unit test will make an unscoped request to PHP.
We could update the test to include a scope, but it's also something a user might do, so I think it's better to cover that case.
There was a problem hiding this comment.
| $redirect_url = '/' . implode('/', array_slice($parts, 2)); | ||
| } | ||
| wp_redirect( | ||
| home_url($redirect_url), |
There was a problem hiding this comment.
Is the home_url() call necessary here?
There was a problem hiding this comment.
I think this is about providing an absolute URL to Safari as @bgrgicak mentioned above. Can you add this info as a comment in a follow-up PR?
There was a problem hiding this comment.
Good point @akirk, I can explain this in a comment.
There was a problem hiding this comment.
This PR follows up on #1856 and forces a reload after autologin to ensure the user is logged in correctly during boot.
Autologin will now only run during HTTP requests. If the code was executed using the
php.run()method it's a CLI call, so we can't login because there are no cookies which are required for logins to work.Before this PR, WordPress auth cookies weren't set in the
$_COOKIESglobal after boot. Adding them required a refresh.Not having a correct
$_COOKIESglobal meant that some features like nonce generated on boot was incorrect.If that nonce was used to make a request, it would fail because of the nonce.
Testing Instructions (or ideally a Blueprint)