WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 5 months ago

Last modified 3 months ago

#46595 closed defect (bug) (fixed)

Allow more than one valid recovery mode link

Reported by: flixos90 Owned by: timothyblynjacobs
Milestone: 5.2 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: servehappy has-patch has-unit-tests commit
Focuses: Cc:

Description

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.

Attachments (11)

46595.patch (10.3 KB) - added by pbearne 6 months ago.
patch to add keyed data to allow for more than one link with update unit test
46595.2.patch (10.8 KB) - added by pbearne 6 months ago.
added clean/delete function
46595.3.patch (10.8 KB) - added by pbearne 6 months ago.
fixed typo
46595.4.patch (10.6 KB) - added by pbearne 6 months ago.
lets try this version, remove nonce and now using random string
46595.5.patch (11.1 KB) - added by pbearne 6 months ago.
Code style fixes
46595.6.patch (9.2 KB) - added by pbearne 6 months ago.
more code style tweeks
46595.7.patch (12.1 KB) - added by pbearne 6 months ago.
more CS fixes
46595.8.patch (15.3 KB) - added by pbearne 6 months ago.
changes from @flixos90 comments
46595.diff (17.6 KB) - added by TimothyBlynJacobs 5 months ago.
46595.2.diff (17.7 KB) - added by TimothyBlynJacobs 5 months ago.
46595.3.diff (18.0 KB) - added by flixos90 5 months ago.

Download all attachments as: .zip

Change History (36)

#1 @flixos90
6 months ago

  • Keywords servehappy added

#2 @TimothyBlynJacobs
6 months ago

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.

This ticket was mentioned in Slack in #core-php by flixos90. View the logs.


6 months ago

#4 @flixos90
6 months ago

@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.

#5 @TimothyBlynJacobs
6 months ago

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

@pbearne
6 months ago

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

#6 @TimothyBlynJacobs
6 months ago

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.

#7 @pbearne
6 months ago

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 );
        }

#8 @pbearne
6 months ago

@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
6 months ago

added clean/delete function

#9 @TimothyBlynJacobs
6 months ago

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
6 months ago

fixed typo

#10 @pbearne
6 months ago

OK will recode

#11 @TimothyBlynJacobs
6 months ago

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

@pbearne
6 months ago

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

#12 @pbearne
6 months ago

  • Keywords has-patch has-unit-tests dev-feedback added; needs-patch removed

ping if you need more changes

@pbearne
6 months ago

Code style fixes

@pbearne
6 months ago

more code style tweeks

@pbearne
6 months ago

more CS fixes

#13 @flixos90
6 months ago

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.

#14 @pbearne
6 months ago

What schedule would you like the clean_expired_keys to be on

#15 @TimothyBlynJacobs
6 months ago

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.

#16 @pbearne
6 months ago

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
6 months ago

changes from @flixos90 comments

#17 @pbearne
6 months ago

lets see how this looks :-)

#18 @pbearne
5 months ago

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

Paul

Last edited 5 months ago by pbearne (previous) (diff)

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


5 months ago

#20 @TimothyBlynJacobs
5 months ago

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?

This ticket was mentioned in Slack in #core-php by schlessera. View the logs.


5 months ago

#22 @TimothyBlynJacobs
5 months ago

  • Keywords dev-feedback removed

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

@flixos90
5 months ago

#23 @flixos90
5 months ago

  • Keywords commit added

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.

#24 @flixos90
5 months ago

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

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.

#25 @spacedmonkey
3 months ago

  • Component changed from Bootstrap/Load to Site Health
Note: See TracTickets for help on using tickets.