From 2f4b9741304e292c4902cf64f3ee34382e84f80b Mon Sep 17 00:00:00 2001 From: Paulo Castellano Date: Thu, 2 Jul 2026 20:32:55 -0300 Subject: [PATCH] Fix X token chain breaking from over-rotation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X OAuth2 refresh tokens are single-use: each refresh rotates the pair and invalidates the previous refresh_token, and reusing a rotated one kills the whole family. Three things made this fragile and disconnected accounts far more often than necessary: - The proactive refresh job called refreshToken() directly, bypassing the access-token-first guard in verify() and rotating on every run. - RefreshExpiringTokens used a 2h window on an hourly schedule — equal to the 2h access-token lifetime — so every X account was rotated every hour even while its token was still valid. - A single 4xx refresh failure disconnected the account without checking whether a concurrent refresh had already persisted a working token. Changes: - RefreshSocialToken now routes through verify() (access-token-first), so it only rotates when the access_token is actually invalid. - Shrink the proactive window to 30m and run the command every 15m, so the window still covers the run interval but rotation happens near real expiry. - verify() tolerates the lost-rotation race: on a 4xx refresh, reload and verify with a concurrently-refreshed token before marking TokenExpired. Refs #126 --- .../Commands/RefreshExpiringTokens.php | 4 +- app/Jobs/RefreshSocialToken.php | 2 +- app/Services/Social/ConnectionVerifier.php | 39 +++++++++++++++---- routes/console.php | 2 +- .../Commands/RefreshExpiringTokensTest.php | 10 ++--- tests/Feature/Jobs/RefreshSocialTokenTest.php | 37 +++++++++++++++--- .../Social/ConnectionVerifierTest.php | 31 +++++++++++++++ 7 files changed, 102 insertions(+), 23 deletions(-) diff --git a/app/Console/Commands/RefreshExpiringTokens.php b/app/Console/Commands/RefreshExpiringTokens.php index 200c3063..981fe1b1 100644 --- a/app/Console/Commands/RefreshExpiringTokens.php +++ b/app/Console/Commands/RefreshExpiringTokens.php @@ -13,7 +13,7 @@ class RefreshExpiringTokens extends Command { protected $signature = 'social:refresh-expiring-tokens'; - protected $description = 'Proactively refresh tokens expiring in the next 2 hours (or already expired)'; + protected $description = 'Proactively refresh tokens expiring in the next 30 minutes (or already expired)'; public function handle(): void { @@ -22,7 +22,7 @@ public function handle(): void SocialAccount::query() ->where('status', Status::Connected) ->whereNotNull('token_expires_at') - ->where('token_expires_at', '<=', now()->addHours(2)) + ->where('token_expires_at', '<=', now()->addMinutes(30)) ->chunk(50, function ($accounts) use (&$count) { foreach ($accounts as $account) { RefreshSocialToken::dispatch($account); diff --git a/app/Jobs/RefreshSocialToken.php b/app/Jobs/RefreshSocialToken.php index 25b6b1bf..53df7da8 100644 --- a/app/Jobs/RefreshSocialToken.php +++ b/app/Jobs/RefreshSocialToken.php @@ -24,7 +24,7 @@ public function __construct(public SocialAccount $account) {} public function handle(ConnectionVerifier $verifier): void { try { - $verifier->refreshToken($this->account); + $verifier->verify($this->account); } catch (PlatformUnavailableException $e) { Log::warning('Token refresh skipped: platform unavailable', [ 'account_id' => $this->account->id, diff --git a/app/Services/Social/ConnectionVerifier.php b/app/Services/Social/ConnectionVerifier.php index 085699b1..4cb29072 100644 --- a/app/Services/Social/ConnectionVerifier.php +++ b/app/Services/Social/ConnectionVerifier.php @@ -31,9 +31,7 @@ public function verify(SocialAccount $account): bool // refresh, so proactive refreshes during races cause false-positive // disconnects even though the access_token still works fine. if ($account->is_token_expired) { - $this->refreshToken($account); - - return $this->callVerifyEndpoint($account); + return $this->refreshThenVerify($account); } try { @@ -41,14 +39,39 @@ public function verify(SocialAccount $account): bool } catch (TokenExpiredException $e) { // Verify returned 401: the access_token is actually invalid. // Refresh and retry once with the new token. - try { - $this->refreshToken($account); - } catch (TokenExpiredException) { - throw $e; + return $this->refreshThenVerify($account, $e); + } + } + + /** + * Refresh the token, then verify with the new one. + * + * If the refresh is rejected (4xx) but a concurrent refresh has already + * rotated the account and persisted a fresh access_token, reload and + * verify with that token instead of giving up — X (and other providers + * that single-use their refresh_token) otherwise disconnect a still-usable + * account whenever two refreshes race and one loses the rotation. + * + * @throws TokenExpiredException + * @throws PlatformUnavailableException + */ + private function refreshThenVerify(SocialAccount $account, ?TokenExpiredException $original = null): bool + { + $accessTokenBeforeRefresh = $account->access_token; + + try { + $this->refreshToken($account); + } catch (TokenExpiredException $e) { + $account->refresh(); + + if ($account->access_token !== $accessTokenBeforeRefresh) { + return $this->callVerifyEndpoint($account); } - return $this->callVerifyEndpoint($account); + throw $original ?? $e; } + + return $this->callVerifyEndpoint($account); } /** diff --git a/routes/console.php b/routes/console.php index 0fa9db84..dd2b3ef6 100644 --- a/routes/console.php +++ b/routes/console.php @@ -14,7 +14,7 @@ Schedule::command(ProcessScheduledPosts::class)->everyMinute()->withoutOverlapping()->onOneServer(); Schedule::command(CheckSocialConnections::class)->daily()->withoutOverlapping()->onOneServer(); -Schedule::command(RefreshExpiringTokens::class)->hourly()->withoutOverlapping()->onOneServer(); +Schedule::command(RefreshExpiringTokens::class)->everyFifteenMinutes()->withoutOverlapping()->onOneServer(); Schedule::command(RecoverStuckPosts::class)->everyThirtyMinutes()->withoutOverlapping()->onOneServer(); Schedule::command(FireScheduleTriggers::class)->everyMinute()->withoutOverlapping()->onOneServer(); Schedule::command(ProcessAutomationDelays::class)->everyMinute()->withoutOverlapping()->onOneServer(); diff --git a/tests/Feature/Commands/RefreshExpiringTokensTest.php b/tests/Feature/Commands/RefreshExpiringTokensTest.php index 5656d28b..ea8b8019 100644 --- a/tests/Feature/Commands/RefreshExpiringTokensTest.php +++ b/tests/Feature/Commands/RefreshExpiringTokensTest.php @@ -9,25 +9,25 @@ use App\Models\Workspace; use Illuminate\Support\Facades\Queue; -test('it dispatches refresh jobs for tokens expiring within 2 hours or already expired', function () { +test('it dispatches refresh jobs for tokens expiring within 30 minutes or already expired', function () { Queue::fake(); $workspace = Workspace::factory()->create(); - // Should be refreshed (expires in 1 hour) + // Should be refreshed (expires in 15 minutes — inside the proactive window) $expiringSoon = SocialAccount::factory()->create([ 'workspace_id' => $workspace->id, 'platform' => Platform::LinkedIn, 'status' => Status::Connected, - 'token_expires_at' => now()->addHour(), + 'token_expires_at' => now()->addMinutes(15), ]); - // Should NOT be refreshed (expires in 5 hours — outside the proactive window) + // Should NOT be refreshed (expires in 1 hour — outside the proactive window) SocialAccount::factory()->create([ 'workspace_id' => $workspace->id, 'platform' => Platform::Instagram, 'status' => Status::Connected, - 'token_expires_at' => now()->addHours(5), + 'token_expires_at' => now()->addHour(), ]); // SHOULD be refreshed (already expired — last-chance attempt before the diff --git a/tests/Feature/Jobs/RefreshSocialTokenTest.php b/tests/Feature/Jobs/RefreshSocialTokenTest.php index c9a9226d..6299504e 100644 --- a/tests/Feature/Jobs/RefreshSocialTokenTest.php +++ b/tests/Feature/Jobs/RefreshSocialTokenTest.php @@ -11,6 +11,7 @@ use App\Models\User; use App\Models\Workspace; use App\Services\Social\ConnectionVerifier; +use Illuminate\Support\Facades\Http; use Illuminate\Support\Facades\Log; use Illuminate\Support\Facades\Queue; @@ -24,22 +25,46 @@ ]); }); -test('refresh job calls refreshToken (not verify) on the verifier', function () { +test('refresh job routes through verify (access-token-first) not refreshToken', function () { $verifier = mock(ConnectionVerifier::class); - $verifier->shouldReceive('refreshToken')->once()->with( + $verifier->shouldReceive('verify')->once()->with( Mockery::on(fn ($account) => $account->id === $this->account->id) ); - $verifier->shouldNotReceive('verify'); + $verifier->shouldNotReceive('refreshToken'); app()->instance(ConnectionVerifier::class, $verifier); (new RefreshSocialToken($this->account))->handle($verifier); }); +test('proactive refresh does NOT rotate the X refresh token while the access token still works', function () { + Http::fake([ + config('trypost.platforms.x.api').'/users/me' => Http::response(['data' => ['id' => '123']], 200), + config('trypost.platforms.x.api').'/oauth2/token' => Http::response([ + 'access_token' => 'should-not-be-used', + 'refresh_token' => 'should-not-be-used', + 'expires_in' => 7200, + ], 200), + ]); + + // Token is "expiring soon" (inside the proactive window) but still valid. + $this->account->update([ + 'token_expires_at' => now()->addMinutes(20), + 'refresh_token' => 'original-refresh-token', + ]); + + (new RefreshSocialToken($this->account))->handle(app(ConnectionVerifier::class)); + + Http::assertSent(fn ($request) => str_contains($request->url(), '/users/me')); + Http::assertNotSent(fn ($request) => str_contains($request->url(), '/oauth2/token')); + expect($this->account->fresh()->refresh_token)->toBe('original-refresh-token'); + expect($this->account->fresh()->status)->toBe(Status::Connected); +}); + test('refresh job marks account as TokenExpired when refresh_token is rejected', function () { Queue::fake(); $verifier = mock(ConnectionVerifier::class); - $verifier->shouldReceive('refreshToken')->once()->andThrow( + $verifier->shouldReceive('verify')->once()->andThrow( new TokenExpiredException('refresh_token revoked') ); app()->instance(ConnectionVerifier::class, $verifier); @@ -61,7 +86,7 @@ }); $verifier = mock(ConnectionVerifier::class); - $verifier->shouldReceive('refreshToken')->once()->andThrow(new RuntimeException('network blip')); + $verifier->shouldReceive('verify')->once()->andThrow(new RuntimeException('network blip')); app()->instance(ConnectionVerifier::class, $verifier); (new RefreshSocialToken($this->account))->handle($verifier); @@ -79,7 +104,7 @@ }); $verifier = mock(ConnectionVerifier::class); - $verifier->shouldReceive('refreshToken')->once()->andThrow( + $verifier->shouldReceive('verify')->once()->andThrow( new PlatformUnavailableException('X API returned 503 during token refresh', 503) ); app()->instance(ConnectionVerifier::class, $verifier); diff --git a/tests/Feature/Services/Social/ConnectionVerifierTest.php b/tests/Feature/Services/Social/ConnectionVerifierTest.php index 332d6860..74d1bbe9 100644 --- a/tests/Feature/Services/Social/ConnectionVerifierTest.php +++ b/tests/Feature/Services/Social/ConnectionVerifierTest.php @@ -2,6 +2,7 @@ declare(strict_types=1); +use App\Enums\SocialAccount\Status; use App\Exceptions\PlatformUnavailableException; use App\Exceptions\TokenExpiredException; use App\Models\SocialAccount; @@ -414,6 +415,36 @@ expect(fn () => $verifier->refreshToken($account))->toThrow(PlatformUnavailableException::class); }); +test('does not disconnect when a concurrent refresh already rotated the token (lost-rotation race)', function () { + Http::fake([ + // Our stale refresh_token is rejected — a concurrent process already used it. + config('trypost.platforms.x.api').'/oauth2/token' => Http::response(['error' => 'invalid_grant'], 400), + // But the access_token the winning refresh persisted still works. + config('trypost.platforms.x.api').'/users/me' => Http::response(['data' => ['id' => '123']], 200), + ]); + + $account = SocialAccount::factory()->x()->create([ + 'status' => Status::Connected, + 'access_token' => 'stale-token', + 'refresh_token' => 'already-rotated', + 'token_expires_at' => now()->subHour(), + ]); + + // Simulate the concurrent refresh: a separate instance persists a fresh, + // valid token (through the encrypted cast) while our in-memory copy stays + // the stale, expired one. + SocialAccount::find($account->id)->update([ + 'access_token' => 'fresh-token', + 'token_expires_at' => now()->addHours(2), + ]); + + expect((new ConnectionVerifier)->verify($account))->toBeTrue(); + expect($account->fresh()->status)->toBe(Status::Connected); + + Http::assertSent(fn ($request) => str_contains($request->url(), '/users/me') + && $request->header('Authorization')[0] === 'Bearer fresh-token'); +}); + test('4xx during refresh keeps raising TokenExpiredException', function () { Http::fake([ config('trypost.platforms.x.api').'/oauth2/token' => Http::response(['error' => 'invalid_grant'], 400),