Skip to content

Commit aa3e166

Browse files
bindlegirlbindlegirl
committed
Protect from race conditions
1 parent 6fca61e commit aa3e166

File tree

2 files changed

+224
-30
lines changed

2 files changed

+224
-30
lines changed

‎projects/plugins/wpcomsh/connection/class-atomic-storage-provider.php‎

Lines changed: 72 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ public function get( $option_name ) {
6767
return null;
6868
}
6969
$tokens = $this->get_user_tokens( $email, $secret );
70-
return $tokens ? $tokens : null;
70+
return ( is_array( $tokens ) && ! empty( $tokens ) ) ? $tokens : null;
7171
}
7272

7373
return null;
@@ -107,65 +107,107 @@ public function get_master_user_id( $email ) {
107107
}
108108

109109
/**
110-
* Validates user tokens and removes conflicting tokens.
110+
* Remove conflicting tokens for a given normalized token and user.
111111
*
112-
* Removes any tokens that:
113-
* 1. Belong to the current user but don't match the external storage token
114-
* 2. Have the same secret as external storage but belong to a different user (orphaned tokens)
112+
* Conflicts are:
113+
* - Current user has a different token string than normalized token
114+
* - Any other user has a token sharing the same secret prefix
115115
*
116116
* @since $$next-version$$
117117
*
118-
* @param string $normalized_token The normalized token from external storage (token_key.secret.user_id).
119-
* @param array $existing_tokens The existing tokens from the database.
120-
* @param int $user_id The user ID to validate tokens for.
121-
* @return array The tokens array with conflicting tokens removed.
118+
* @param array $tokens Tokens array keyed by user ID.
119+
* @param string $normalized_token Normalized token (token_key.secret.user_id).
120+
* @param int $user_id Local user ID for whom the token applies.
121+
* @return array { Updated tokens and whether any conflicts were removed }
122+
* @phpstan-return array{ tokens: array, had_conflicts: bool }
122123
*/
123-
private function validate_user_tokens( $normalized_token, $existing_tokens, $user_id ) {
124-
$has_conflicts = false;
124+
private function remove_conflicting_tokens( $tokens, $normalized_token, $user_id ) {
125+
$had_conflicts = false;
125126
$last_dot_pos = strrpos( $normalized_token, '.' );
126127

127-
// Validate token format - it must contain a dot to separate secret from user_id
128+
// Validate token format - must contain a dot to separate secret from user_id.
128129
if ( false === $last_dot_pos ) {
129130
// phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log
130-
error_log( "Invalid token format in validate_user_tokens: '{$normalized_token}'" );
131-
return $existing_tokens;
131+
error_log( "Invalid token format in remove_conflicting_tokens: '{$normalized_token}'" );
132+
return array(
133+
'tokens' => $tokens,
134+
'had_conflicts' => false,
135+
);
132136
}
133137

134138
$secret_prefix = substr( $normalized_token, 0, $last_dot_pos );
135139

136-
// Check if current user has a mismatched token
137-
if ( isset( $existing_tokens[ $user_id ] )
138-
&& is_string( $existing_tokens[ $user_id ] )
139-
&& ! hash_equals( $normalized_token, $existing_tokens[ $user_id ] ) ) {
140+
// Remove mismatched token for the current user.
141+
if ( isset( $tokens[ $user_id ] )
142+
&& is_string( $tokens[ $user_id ] )
143+
&& ! hash_equals( $normalized_token, $tokens[ $user_id ] ) ) {
140144
// phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log
141145
error_log( "Removing conflicting token for user {$user_id}" );
142-
unset( $existing_tokens[ $user_id ] );
143-
$has_conflicts = true;
146+
unset( $tokens[ $user_id ] );
147+
$had_conflicts = true;
144148
}
145149

146-
// Check if any other user has a token with the same secret (orphaned token from previous owner)
147-
foreach ( $existing_tokens as $token_user_id => $token ) {
148-
if ( $token_user_id !== $user_id && strpos( $token, $secret_prefix . '.' ) === 0 ) {
150+
// Remove orphaned tokens (same secret, different user).
151+
foreach ( $tokens as $token_user_id => $token ) {
152+
if ( is_string( $token ) && (int) $token_user_id !== $user_id && strpos( $token, $secret_prefix . '.' ) === 0 ) {
149153
// phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log
150154
error_log( "Removing orphaned token with same secret for user {$token_user_id}" );
151-
unset( $existing_tokens[ $token_user_id ] );
152-
$has_conflicts = true;
155+
unset( $tokens[ $token_user_id ] );
156+
$had_conflicts = true;
153157
}
154158
}
155159

160+
return array(
161+
'tokens' => $tokens,
162+
'had_conflicts' => $had_conflicts,
163+
);
164+
}
165+
166+
/**
167+
* Validates user tokens and removes conflicting tokens.
168+
*
169+
* Removes any tokens that:
170+
* 1. Belong to the current user but don't match the external storage token
171+
* 2. Have the same secret as external storage but belong to a different user (orphaned tokens)
172+
*
173+
* Re-reads the latest state before persisting to minimize race condition window.
174+
*
175+
* @since $$next-version$$
176+
*
177+
* @param string $normalized_token The normalized token from external storage (token_key.secret.user_id).
178+
* @param array $existing_tokens The existing tokens from the database.
179+
* @param int $user_id The user ID to validate tokens for.
180+
* @return array The tokens array with conflicting tokens removed.
181+
*/
182+
private function validate_user_tokens( $normalized_token, $existing_tokens, $user_id ) {
183+
$result = $this->remove_conflicting_tokens( $existing_tokens, $normalized_token, $user_id );
184+
$has_conflicts = $result['had_conflicts'];
185+
156186
// Only persist changes if conflicts were found
157187
if ( $has_conflicts ) {
158-
// Persist the change to the database to prevent repeated error logging
159-
$private_options = \Jetpack_Options::get_raw_option( 'jetpack_private_options', array() );
160-
$private_options['user_tokens'] = $existing_tokens;
161-
update_option( 'jetpack_private_options', $private_options );
188+
// Re-read latest state right before writing to minimize race window
189+
$latest_options = \Jetpack_Options::get_raw_option( 'jetpack_private_options', array() );
190+
$latest_tokens = isset( $latest_options['user_tokens'] ) && is_array( $latest_options['user_tokens'] )
191+
? $latest_options['user_tokens']
192+
: array();
193+
194+
// Re-apply cleanup to latest tokens (might find no conflicts now if state changed)
195+
$latest_result = $this->remove_conflicting_tokens( $latest_tokens, $normalized_token, $user_id );
196+
197+
// Write the cleaned latest state
198+
$latest_options['user_tokens'] = $latest_result['tokens'];
199+
\Jetpack_Options::update_raw_option( 'jetpack_private_options', $latest_options, false );
162200

163201
// Also clear master_user from database since connection owner data has changed
164202
// External storage will provide the correct value on next read
165203
\Jetpack_Options::delete_option( 'master_user' );
204+
205+
// Return what we actually wrote to the database
206+
return $latest_result['tokens'];
166207
}
167208

168-
return $existing_tokens;
209+
// No conflicts, return cleaned tokens
210+
return $result['tokens'];
169211
}
170212

171213
/**

‎projects/plugins/wpcomsh/tests/AtomicStorageProviderTest.php‎

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,4 +164,156 @@ public function test_get_user_tokens_existing_matching() {
164164
);
165165
$this->assertSame( $expected, $result );
166166
}
167+
168+
/**
169+
* Test get_user_tokens removes conflicting token for current user.
170+
*/
171+
public function test_get_user_tokens_removes_conflicting_token() {
172+
$user_id = static::factory()->user->create( array( 'user_email' => '[email protected]' ) );
173+
174+
// Set existing token with different secret for same user
175+
$existing_tokens = array(
176+
$user_id => 'old.secret.' . $user_id,
177+
);
178+
update_option( 'jetpack_private_options', array( 'user_tokens' => $existing_tokens ) );
179+
180+
// Call with new secret
181+
$result = $this->provider->get_user_tokens( '[email protected]', 'new.secret' );
182+
183+
// Should have removed old token and added new one
184+
$expected = array(
185+
$user_id => 'new.secret.' . $user_id,
186+
);
187+
$this->assertSame( $expected, $result );
188+
189+
// Verify it was persisted to database
190+
$private_options = get_option( 'jetpack_private_options' );
191+
$this->assertSame( $expected, $private_options['user_tokens'] );
192+
}
193+
194+
/**
195+
* Test get_user_tokens removes orphaned tokens with same secret.
196+
*/
197+
public function test_get_user_tokens_removes_orphaned_tokens() {
198+
$old_owner_id = static::factory()->user->create( array( 'user_email' => '[email protected]' ) );
199+
$new_owner_id = static::factory()->user->create( array( 'user_email' => '[email protected]' ) );
200+
$other_user_id = static::factory()->user->create( array( 'user_email' => '[email protected]' ) );
201+
202+
// Old owner had this secret, now new owner has same secret
203+
$existing_tokens = array(
204+
$old_owner_id => 'shared.secret.' . $old_owner_id,
205+
$other_user_id => 'different.secret.' . $other_user_id,
206+
);
207+
update_option( 'jetpack_private_options', array( 'user_tokens' => $existing_tokens ) );
208+
209+
// New owner connecting with same secret prefix
210+
$result = $this->provider->get_user_tokens( '[email protected]', 'shared.secret' );
211+
212+
// Should remove old owner's token (same secret) but keep other user's token
213+
$expected = array(
214+
$new_owner_id => 'shared.secret.' . $new_owner_id,
215+
$other_user_id => 'different.secret.' . $other_user_id,
216+
);
217+
$this->assertSame( $expected, $result );
218+
}
219+
220+
/**
221+
* Test provider get('user_tokens') returns null when email missing.
222+
*/
223+
public function test_get_user_tokens_via_provider_returns_null_when_email_missing() {
224+
\Atomic_Persistent_Data::delete( 'JETPACK_CONNECTION_OWNER_EMAIL' );
225+
\Atomic_Persistent_Data::set( 'JETPACK_CONNECTION_OWNER_TOKEN_SECRET', 'token.secret' );
226+
227+
$this->assertNull( $this->provider->get( 'user_tokens' ) );
228+
}
229+
230+
/**
231+
* Test provider get('user_tokens') returns null when secret missing.
232+
*/
233+
public function test_get_user_tokens_via_provider_returns_null_when_secret_missing() {
234+
\Atomic_Persistent_Data::set( 'JETPACK_CONNECTION_OWNER_EMAIL', '[email protected]' );
235+
\Atomic_Persistent_Data::delete( 'JETPACK_CONNECTION_OWNER_TOKEN_SECRET' );
236+
237+
$this->assertNull( $this->provider->get( 'user_tokens' ) );
238+
}
239+
240+
/**
241+
* Test provider get('user_tokens') returns null when get_user_tokens returns false.
242+
*/
243+
public function test_get_user_tokens_via_provider_returns_null_for_invalid_user() {
244+
\Atomic_Persistent_Data::set( 'JETPACK_CONNECTION_OWNER_EMAIL', '[email protected]' );
245+
\Atomic_Persistent_Data::set( 'JETPACK_CONNECTION_OWNER_TOKEN_SECRET', 'token.secret' );
246+
247+
$this->assertNull( $this->provider->get( 'user_tokens' ) );
248+
}
249+
250+
/**
251+
* Test provider get('user_tokens') returns array when valid.
252+
*/
253+
public function test_get_user_tokens_via_provider_returns_array_when_valid() {
254+
$user_id = static::factory()->user->create( array( 'user_email' => '[email protected]' ) );
255+
\Atomic_Persistent_Data::set( 'JETPACK_CONNECTION_OWNER_EMAIL', '[email protected]' );
256+
\Atomic_Persistent_Data::set( 'JETPACK_CONNECTION_OWNER_TOKEN_SECRET', 'token.secret' );
257+
258+
$result = $this->provider->get( 'user_tokens' );
259+
260+
$this->assertIsArray( $result );
261+
$this->assertArrayHasKey( $user_id, $result );
262+
$this->assertSame( 'token.secret.' . $user_id, $result[ $user_id ] );
263+
}
264+
265+
/**
266+
* Test validate_user_tokens re-reads latest state before persisting.
267+
*/
268+
public function test_validate_user_tokens_rereads_before_write() {
269+
$user_id = static::factory()->user->create( array( 'user_email' => '[email protected]' ) );
270+
271+
// Initial state: old owner token
272+
$initial_tokens = array(
273+
$user_id => 'old.secret.' . $user_id,
274+
);
275+
update_option( 'jetpack_private_options', array( 'user_tokens' => $initial_tokens ) );
276+
277+
// Simulate: After reading but before validate_user_tokens persists, another user connects
278+
// We can't truly test the race condition without threading, but we can verify
279+
// that get_user_tokens returns the expected result
280+
281+
$result = $this->provider->get_user_tokens( '[email protected]', 'new.secret' );
282+
283+
// Should have new owner token
284+
$this->assertArrayHasKey( $user_id, $result );
285+
$this->assertSame( 'new.secret.' . $user_id, $result[ $user_id ] );
286+
}
287+
288+
/**
289+
* Test get('blog_token') returns null when empty.
290+
*/
291+
public function test_get_blog_token_returns_null_when_empty() {
292+
\Atomic_Persistent_Data::delete( 'JETPACK_BLOG_TOKEN' );
293+
$this->assertNull( $this->provider->get( 'blog_token' ) );
294+
}
295+
296+
/**
297+
* Test get('blog_token') returns token when set.
298+
*/
299+
public function test_get_blog_token_returns_token_when_set() {
300+
\Atomic_Persistent_Data::set( 'JETPACK_BLOG_TOKEN', 'blog.token.value' );
301+
$this->assertSame( 'blog.token.value', $this->provider->get( 'blog_token' ) );
302+
}
303+
304+
/**
305+
* Test get('id') returns null when empty.
306+
*/
307+
public function test_get_id_returns_null_when_empty() {
308+
\Atomic_Persistent_Data::delete( 'JETPACK_BLOG_ID' );
309+
$this->assertNull( $this->provider->get( 'id' ) );
310+
}
311+
312+
/**
313+
* Test get('id') returns int when set.
314+
*/
315+
public function test_get_id_returns_int_when_set() {
316+
\Atomic_Persistent_Data::set( 'JETPACK_BLOG_ID', '12345' );
317+
$this->assertSame( 12345, $this->provider->get( 'id' ) );
318+
}
167319
}

0 commit comments

Comments
 (0)