diff --git a/plugins/wporg-5ftf/includes/email.php b/plugins/wporg-5ftf/includes/email.php index 0032186..522ea88 100644 --- a/plugins/wporg-5ftf/includes/email.php +++ b/plugins/wporg-5ftf/includes/email.php @@ -13,12 +13,18 @@ * we email them time-restricted, once-time-use auth tokens when they want to "log in". * * WP "nonces" aren't ideal for this purpose from a security perspective, because they're less secure. They're - * reusable, last up to 24 hours, and have a much smaller search space in brute force attacks. They also create an - * inconsistent UX, because a token could be valid for 24 hours, or for 1 second, due to how `wp_nonce_tick()` - * works. That would lead to some situations where a nonce had already expired by the time the contributor opened - * the email and clicked on the link. + * reusable, last up to 24 hours, and have a much smaller search space in brute force attacks. They're only + * intended to prevent CSRF, and should not be used for authentication or authorization. * - * So instead, true NONCEs are implemented; see `is_valid_authentication_token()` for details. + * They also create an inconsistent UX, because a nonce could be valid for 24 hours, or for 1 second, due to their + * stateless nature -- see `wp_nonce_tick()`. That would lead to some situations where a nonce had already expired + * by the time the contributor opened the email and clicked on the link. + * + * So instead, true stateful CSPRN authentication tokens are generated; see `get_authentication_url()` and + * `is_valid_authentication_token()` for details. + * + * For additional background: + * - https://stackoverflow.com/a/35715087/450127 (which is better security advice than ircmarxell's 2010 answer). */ namespace WordPressDotOrg\FiveForTheFuture\Email; @@ -27,6 +33,9 @@ defined( 'WPINC' ) || die(); const TOKEN_PREFIX = '5ftf_auth_token_'; +// Longer than `get_password_reset_key()` just to be safe. See https://core.trac.wordpress.org/ticket/43546#comment:34 +const TOKEN_LENGTH = 32; + /** * Wrap `wp_mail()` with shared functionality. * @@ -47,7 +56,7 @@ function send_email( $to, $subject, $message ) { } /** - * Generate an action URL with a unique authentication token. + * Generate an action URL with a secure, unique authentication token. * * @param int $pledge_id * @param string $action @@ -58,7 +67,8 @@ function send_email( $to, $subject, $message ) { */ function get_authentication_url( $pledge_id, $action, $action_page_id ) { $auth_token = array( - 'value' => wp_generate_password( 20, false ), // Similar to `get_password_reset_key()`. + // This will create a CSPRN and is similar to how `get_password_reset_key()` works. + 'value' => wp_generate_password( TOKEN_LENGTH, false ), // todo should encrypt at rest? core doesn't but others do 'expiration' => time() + ( 2 * HOUR_IN_SECONDS ), ); @@ -106,6 +116,18 @@ function is_valid_authentication_token( $pledge_id, $action, $unverified_token ) $verified = false; $valid_token = get_post_meta( $pledge_id, TOKEN_PREFIX . $action, true ); + /* + * Later on we'll compare the value to user input, and the user could input null/false/etc, so let's guarantee + * that the thing we're comparing against is really what we expect it to be. + */ + if ( ! is_array( $valid_token ) || ! array_key_exists( 'value', $valid_token ) || ! array_key_exists( 'expiration', $valid_token ) ) { + return false; + } + + if ( ! is_string( $valid_token['value'] ) || TOKEN_LENGTH !== strlen( $valid_token['value'] ) ) { + return false; + } + if ( $valid_token && $valid_token['expiration'] > time() && $unverified_token === $valid_token['value'] ) { $verified = true; diff --git a/plugins/wporg-5ftf/includes/pledge-meta.php b/plugins/wporg-5ftf/includes/pledge-meta.php index af9e6bb..40e19e5 100755 --- a/plugins/wporg-5ftf/includes/pledge-meta.php +++ b/plugins/wporg-5ftf/includes/pledge-meta.php @@ -185,7 +185,7 @@ function render_meta_boxes( $pledge, $box ) { */ function save_pledge( $pledge_id, $pledge ) { $get_action = filter_input( INPUT_GET, 'action' ); - $post_action = $_POST['action'] ?? null; + $post_action = filter_input( INPUT_POST, 'action' ); $ignored_actions = array( 'trash', 'untrash', 'restore' ); /* diff --git a/plugins/wporg-5ftf/includes/pledge.php b/plugins/wporg-5ftf/includes/pledge.php index 8605f54..9a05b8b 100755 --- a/plugins/wporg-5ftf/includes/pledge.php +++ b/plugins/wporg-5ftf/includes/pledge.php @@ -134,7 +134,6 @@ function create_new_pledge( $name ) { if ( ! is_wp_error( $pledge_id ) ) { send_pledge_verification_email( $pledge_id, get_post()->ID ); - send_contributor_verification_email(); } return $pledge_id; @@ -168,8 +167,3 @@ function send_pledge_verification_email( $pledge_id, $action_page_id ) { $message ); } - -// todo -function send_contributor_verification_email() { - // todo -} diff --git a/plugins/wporg-5ftf/views/form-pledge-confirm-email.php b/plugins/wporg-5ftf/views/form-pledge-confirm-email.php index d2ea0a6..345f8c8 100755 --- a/plugins/wporg-5ftf/views/form-pledge-confirm-email.php +++ b/plugins/wporg-5ftf/views/form-pledge-confirm-email.php @@ -13,7 +13,7 @@ namespace WordPressDotOrg\FiveForTheFuture\View;
- Thank you for confirming your address! Your pledge will show up in the directory once one of your contributors confirms their participation. + Thank you for confirming your address! We've emailed confirmation links to your contributors, and your pledge will show up in the directory once one of them confirms their participation.
Post a job listing on jobs.wordpress.net.', 'wporg' ) + // todo ask mel about moving this outside the `notice-success`, since it's not really part of the success notification, and distracts from it. + // many users have notification fatigue and no longer trust them or pay attention to them, because they're so often misused for non-critical information, + // and the jobs thing is more of an "ad" in this context than something directly related to the process the user wants to complete ); ?>