Allow more than one valid recovery mode link

[flixos90]

In the new recovery mode (see #46130), it is currently only possible for a single recovery mode link at a time to be valid. That is because the key is stored in a recovery_key option, with the latest key always overriding the previous one.

This is not too critical at this point since those links are only sent via a rate-limited email, and due to that rate limit the link would expire when the next one is sent anyway. However, a future iteration of the recovery mode will allow additional ways of obtaining such a link, most likely via request by a user with sufficient permissions. This means multiple links need to be valid at the same time.

I talked about this with @timothyblynjacobs earlier, and we think the following would be a good fix:

• Store key under recovery_key_{$random_chars}
• Add {$random_chars} to recovery link URL, so that it looks like wp-login.php?action=enter_recovery_mode&rm_key={$key}&rm_lookup={$random_chars} (not sure about the term "rm_lookup", but that's the general idea.
• When checking the key, get the option with the suffix {$random_chars} that is included in the URL.

[TimothyBlynJacobs]

One of the difficulties of using the variable option name we discussed, is it makes cleanup hard. It'd be easy for the database to be filled with loads of these options as new recovery links come in. Right now, we don't invalidate when processing a recovery key. We could add that, and it'd help somewhat, but not enough IMO.

An alternative would be to make recovery_keys an associative array with the random chars as the keys. Then, whenever a new recovery key is generated, we could loop through all existing recovery keys and remove them if they expired.

[flixos90]

@TimothyBlynJacobs I'm not sure I agree. We also use variable names containing the session ID for paused extensions, and I think the cleanup you're suggesting would be sufficient for a core implementation. I don't like the associative array option much because it would cause less efficient queries and processing - but if there are more arguments for it to be the better alternative, I'm not greatly opposed either.

We should furthermore make the key a real nonce in the way that it should only be valid once. After validating a key, it should be deleted from the database. This also takes care of the cleanup mentioned above.

[TimothyBlynJacobs]

The main cleanup issue is for links that don't get clicked, they'll never have an opportunity to be cleaned.

[pbearne]

patch to add keyed data to allow for more than one link with update unit test

[TimothyBlynJacobs]

This shouldn't be using the nonce API as it isn't a real nonce implementation, is tied to the current user ( or logged-out user ), and has its own time limit that will interfere with the filterable TTL.

Instead, the token should be a randomized string, and the token deleted when it is used. This shouldn't happen in verify() but in a separate method like delete_key() which should also check for all other stored keys that have expired.

[pbearne]

Added a patch to support more than one link
Update Unit tests to match we may need some more
made it so a LINK can only be used ONCE
If a user request more than one link only the last will as the same nonce is used as the key

this code doesn't clean the old tokens as not sure when to run the clean up code

this code will clean it happy to add to patch

<?php
        /**
         * Removes old recovery keys.
         *
         * @since 5.2.0
         *
         *
         * @return null.
         */
        public function clean_recovery_key_options(){

                $records = get_option( 'recovery_key' );
                
                foreach ( $records as $key => $record ){
                        if ( ! wp_verify_nonce( $key, 'recover_mode_token' ) ) {
                                unset( $records[ $key ] );
                        }
                }
                                
                update_option( 'recovery_key', $records );
        }

[pbearne]

@TimothyBlynJacobs

I am not sure it is a bad thing that the link is tied to a user via the nonce and having the TTL capped by the Nonce TTL may not be a bad side effect. Do really what link to OK after more than a day?

Will move the delete into a side function

[pbearne]

added clean/delete function

[TimothyBlynJacobs]

It is an issue. The email may be sent at any point in the request by any user. If the email isn't triggered by the admin user who goes to receive the email, they won't be able to use the recovery mode link.

[pbearne]

fixed typo

[pbearne]

OK will recode

[TimothyBlynJacobs]

Thanks, and thank you for the patch! Feel free to ping me on slack.

[pbearne]

lets try this version, remove nonce and now using random string

[pbearne]

ping if you need more changes

[pbearne]

Code style fixes

[pbearne]

more code style tweeks

[pbearne]

more CS fixes

[flixos90]

Thanks @pbearne for tackling this, great work! Only some observations regarding 46595.7.patch​ to improve the code:

• Let's rename the option to recovery_keys as it's multiple now.
• In WP_Recovery_Mode_Key_Service, the generate_and_store_recovery_mode_key() is too complex and odd with the two return values. Let's instead add a new generate_recovery_mode_token() method that just does return wp_generate_password( 22, false ) and introduce a parameter to the former method, so that it is generate_and_store_recovery_mode_key( $token ) - it should then continue to only return the key. Code using these parts should then call generate_recovery_mode_token() first and then pass that token.
• The clean_expired_keys( $ttl, $token = '' ) method does too much as well. Let's split it into clean_key( $token ) and clean_expired_keys( $ttl ) so that both methods have distinct functionality.
• validate_and_consume_recovery_mode_key() is too long of a name and too detailed. Let's stick with the previous name validate_recovery_mode_key() and clarify the one-time usage of a key in the doc block. That method should then call clean_key( $token ), as it should only need to worry about the currently checked key and token.
• clean_expired_keys( $ttl ) should be handled separately and be hooked into WP Cron so that it is executed regularly.
• I'd prefer if we didn't add the generate_recovery_mode_key action. That's something we should do if it proves to be needed.

[pbearne]

What schedule would you like the clean_expired_keys to be on

[TimothyBlynJacobs]

I'd prefer if we didn't add the generate_recovery_mode_key action. That's something we should do if it proves to be needed.

Just want to point out that action has already been committed. It is quite useful for logging.

[pbearne]

Would it be ok to use the 'wp_scheduled_delete' to hook the clean function into or do I need to create a new schedule?

[pbearne]

changes from @flixos90 comments

[pbearne]

lets see how this looks :-)

[pbearne]

ping:
Do we need another changes to get this into 5.2?

Paul

[TimothyBlynJacobs]

Uploaded a patch with some minor code formatting/cleanup and docs. Additionally, I moved the cron handling to the base WP_Recovery_Mode class so that the key service remains pure. I also moved the $token parameter to always come first, I think this helps make it more clear that the $token identifies the $key. I think the daily schedule is also sufficient since keys would only be generated every 4 hours.

---

Blurgh, that should be a .9 diff. I guess grunt upload_patch doesn't look at .patch extensions for naming?

[TimothyBlynJacobs]

Adjusted to cast to the option to an array based off of @schlessera's feedback.

[flixos90]

46595.3.diff​ performs minor tweaks to code style and order of property declarations, to match related code parts. In addition it introduces private methods get_keys() and update_keys() to abstract the logic for reading and writing the option, which gets rid of the requirement to always remind ourselves to write the array typecasting bit.

[flixos90]

In 45211:

Bootstrap/Load: Allow more than one recovery link to be valid at a time.

While currently a recovery link is only made available via the admin email address, this will be expanded in the future. In order to accomplish that, the mechanisms to store and validate recovery keys must support multiple keys to be valid at the same time.

This changeset adds that support, adding an additional token parameter which is part of a recovery link in addition to the key. A key itself is always associated with a token, so the two are only valid in combination. These associations are stored in a new recovery_keys option, which is regularly cleared in a new Cron hook, to prevent potential cluttering from unused recovery keys.

This changeset does not have any user-facing implications otherwise.

Props pbearne, timothyblynjacobs.
Fixes #46595. See #46130.