Enable extension pausing when in "Recovery Mode" #6
Conversation
There was a problem hiding this comment.
Great work so far! I pointed out a few concerns. Here are some more general observations and thoughts (partly also touched in inline comments):
- I think there should be a class for recovery mode functionality.
WP_Recovery_Modeor something like that (should probably be afinalclass) - simply for the ability to better group functionality together and to make methods private that shouldn't be public. - No files should be included anywhere but in code that a 100% surely terminates the request itself.
- We need to think about whether to use options or network options (in case we're in a multisite that's relevant). I think network options certainly make sense for the fallback key and salt that just replace a constant - for the rest, that's worth discussing.
- I don't think we need the concept of protected endpoints any longer. If in recovery mode, pause extensions - if not, don't. IMO there's no need for this additional complexity, i.e. the
is_protected_endpoint()should be dropped completely in favor ofwp_is_recovery_mode().
+1 on using custom code for the hashing etc. - since we can't use the pluggable functions in these circumstances, that's a fine approach I'd say.
This introduces a WP_Recovery_Mode_Controller interface. Core would ship with the email controller which handles recovery mode by sending an email and setting a cookie. But this will allow for hosts or mu-plugins to substitute any handler that implements that interface. The cookie handler has also been abstracted out to a separate service and can be reused by a custom handler. It also allows for substituting all the cookie config values in the constructor. This should make it easier to ship this eventually to network activated plugins as well, since a primary concern there is that the correct cookie constants have not been populated until after those plugins have been loaded. This does not yet drop `is_protected_endpoint()` since that is still to be discussed. Additionally, I have not had time yet to refactor the extension storage to use one option as @flixos90 suggested.
- Support RECOVERY_MODE_EMAIL constant to set the email address. - Change default rate limit to 4 hours. - Ensure link is valid for at least as long as the rate limit duration. - Only send recovery mode email when the errors occurs in a protected endpoint.
This allows for better reusability for alternate recovery mode controllers and allows for better testing.
TimothyBJacobs
force-pushed
the
wsod-protection-refined
branch
from
a5c52ae
to
9e6a091
Compare
of firing an action. This mostly reflects the Google Docs proposed version. I don't think the recovery mode controller should at all be responsible for pausing extensions. This makes extending the functionality through drop-ins unsafe since we are moving unrelated functionality into that method. If we change how extensions are recorded or want to perform different behavior, the drop-ins would be incompatible.
|
User Flow & Technical Doc: https://docs.google.com/document/d/1XWQghXj0p0X8GF-Byuv4vfPQG0Rs0XoVfOepVjiMkfM/edit I do think we are in a good place extensibility wise to be able to support a user email specific flow in 5.3 with our current framework and IMO could be implemented as a drop-in with some minor changes. The main thing being an I think that putting an exit mode in the admin bar makes sense. I think it’d be quickest to add the underlying technical support for exiting recovery mode, like a method, in the patch before commit and then worry about that specific UI shortly thereafter. Looking at a specific of the technical document, I don’t think we should overload the |
Given the current approach, this completely makes sense. Let's have a
Yes. The reason I made them private methods in the doc was mostly to have a better understanding when reading about them - I see it as a starting point to abstract from further. The multiple classes approach is better organized, but a bit harder to grasp at first glance. |
|
For first iteration (release in 5.2) I think this should be disabled for WordPress multisite. There's ux/ui workflows that need to be considered in a multisite environment and typically those working with multisite have a better handle on dealing with php/plugin upgrades etc. so the urgency for these protections in that environment isn't as great. |
There was a problem hiding this comment.
I did a more thorough review and pointed out some details. Most of it is looking good.
A few more general things:
- Let's keep the
WP_Fatal_Error_Handlerclass as generic as it can be. Storing errors, redirecting, etc. already ties in with recovery mode and how we envision it, so these things should be covered by the recovery mode controller. The only connection should be the fatal error handler calling thehandle_fatal_error()method. - Let's be conservative and not expose too much code through functions that we only call in one location. We use class structures here, and the feature is in a first iteration, so we don't want to expose too much for outside usage here.
- The same applies to overriding. People can override the fatal error handler, and thus technically already have the ability to override in whichever way they want. For the first iteration, let's not get more granular, i.e. not allow to override the recovery mode handler specifically. In
is_recovery_mode_active(), we can check a constant that MU plugins can define - that allows for customizing that bit, while not restricting us in what we can do to our class infrastructure later. We don't want to corner ourselves. - Function/method descriptions in doc-blocks should start with a third-person verb rather than infinitive. :)
but save the error to the same option. Drop multisite option handling. This can be reintroduced safely in a later version when we want to support Multisite.
|
Just taking a note here that we should wrap the calls in |
Now `WP_Recovery_Mode` is the object responsible for managing recovery mode. WP_Fatal_Error_Handler passes off control to `WP_Recovery_Mode::handle_error()` when a fatal error occurs. `WP_Recovery_Mode_Email_Controller` is renamed to `WP_Recovery_Mode_Email`. This keeps the line between general Recovery Mode processing, like handling storing errors and doing redirects, separate from the method of notifying the user to enter recovery mode. Adjusted redirect_protected functionality to account for when headers have already been sent ( display errors being on ). This right now just displays the default error template. We might want to adjust that. Adds a admin menu bar link to exit recovery mode. The cookie check can be overridden by using the `WP_RECOVERY_MODE_SESSION_ID` constant, but this probably shouldn't be encouraged.
…very mode and have an active session ID before use. This blocks mistakes from 3rd party code loading the storage instance too early.
Consistently name properties.
| ), | ||
| array( | ||
| $url, | ||
| 'TBD', |
There was a problem hiding this comment.
Is pending this issue: WordPress/servehappy#40 :)
Co-Authored-By: TimothyBJacobs <timothy@ironbounddesigns.com>
and more specific.
|
Merged against latest trunk, where the basic fatal error handler is already committed now. |
…e fatal error handler.
…press-develop into wsod-protection-refined
…lightly incorrect.
… errors) and provide according feedback.
|
Done! https://core.trac.wordpress.org/changeset/44973 Thanks for this truly outstanding work @TimothyBJacobs! 🎉 |
|
Thank you! The recovery mode code wouldn't be anywhere near where it is without your extensive help. |
See #46130 for an in-depth explanation.