Email: Iterate based on feedback.

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;