fix(legacy): prevent concurrent token refreshes from reusing a rotated token

integration-tests/auth_refresh_test.go

@@ -0,0 +1,62 @@
+package tests
+
+import (
+	"net/http/httptest"
+	"strings"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/upsun/cli/pkg/mockapi"
+)
+
+// TestAuthRefresh_Concurrent runs several processes that load the same expired session.
+// Only one should use the refresh token: the others must use the token it saved.
+func TestAuthRefresh_Concurrent(t *testing.T) {
+	authServer := mockapi.NewAuthServer(t)
+	defer authServer.Close()
+	// Keep the first refresh in progress while the other processes start.
+	authServer.SetRefreshDelay(2 * time.Second)
+	authServer.AddRefreshToken("initial-refresh-token")
+
+	apiHandler := mockapi.NewHandler(t)
+	apiHandler.SetMyUser(&mockapi.User{ID: "u1"})
+	apiServer := httptest.NewServer(apiHandler)
+	defer apiServer.Close()
+
+	f := newCommandFactory(t, apiServer.URL, authServer.URL)
+	f.extraEnv = append(f.extraEnv, EnvPrefix+"TOKEN=")
+	// Initialize before running commands concurrently.
+	f.dir = t.TempDir()
+	getCommandName(t)
+	writeOAuthSession(t, f.home, "default", map[string]any{
+		"accessToken":  "expired-token",
+		"tokenType":    "bearer",
+		"expires":      time.Now().Add(-time.Hour).Unix(),
+		"refreshToken": "initial-refresh-token",
+	})
+
+	const processes = 3
+	type result struct {
+		stdout, stderr string
+		err            error
+	}
+	results := make([]result, processes)
+	var wg sync.WaitGroup
+	for i := range processes {
+		wg.Go(func() {
+			stdout, stderr, err := f.RunCombinedOutput("auth:token", "--no-warn")
+			results[i] = result{stdout, stderr, err}
+		})
+	}
+	wg.Wait()
+
+	for i, r := range results {
+		if assert.NoError(t, r.err, "process %d stderr: %s", i, r.stderr) {
+			assert.Equal(t, "access-token-1", strings.TrimSpace(r.stdout), "process %d", i)
+		}
+	}
+	assert.False(t, authServer.ReuseDetected(), "a rotated refresh token was reused")
+}

legacy/composer.json

@@ -21,7 +21,7 @@
         "khill/php-duration": "^1.1",
         "symfony/polyfill-mbstring": "^1.19",
         "symfony/polyfill-iconv": "^1.19",
-        "platformsh/oauth2": "^1@beta",
+        "platformsh/oauth2": "^1.0.0-beta4",
         "giggsey/libphonenumber-for-php-lite": "^8.13",
         "symfony/var-dumper": "^7.3"
     },

legacy/src/Service/Api.php

@@ -312,17 +312,30 @@ private function getConnectorOptions(): array
         // Acquire a lock to prevent tokens being refreshed at the same time in
         // different CLI processes.
         $refreshLockName = 'refresh--' . $this->config->getSessionIdSlug();
-        $connectorOptions['on_refresh_start'] = function ($originalRefreshToken) use ($refreshLockName) {
+        $connectorOptions['on_refresh_start'] = function (string $originalRefreshToken) use ($refreshLockName): ?AccessToken {
             $this->io->debug('Refreshing access token');
-            $connector = $this->getClient(false)->getConnector();
-            return $this->fileLock->acquireOrWait($refreshLockName, function (): void {
+            $this->fileLock->acquireOrWait($refreshLockName, function (): void {
                 $this->stdErr->writeln('Waiting for token refresh lock', OutputInterface::VERBOSITY_VERBOSE);
-            }, function () use ($connector, $originalRefreshToken) {
-                $session = $connector->getSession();
-                $accessToken = $this->tokenFromSession($session);
-                return $accessToken && $accessToken->getRefreshToken() !== $originalRefreshToken
-                    ? $accessToken : null;
             });
+            // Without the lock, the token could be refreshed or saved over another process's newer token.
+            if (!$this->fileLock->isHeld($refreshLockName)) {
+                throw new \RuntimeException('Timed out waiting for another process to refresh the access token. Please try again.');
+            }
+
+            // Refresh tokens are single-use, so use the stored token if another process has refreshed it.
+            $session = $this->getClient(false)->getConnector()->getSession();
+            try {
+                $session->reload();
+            } catch (\RuntimeException $e) {
+                throw $this->convertStorageException($e);
+            }
+            $storedToken = $this->tokenFromSession($session);
+            if ($storedToken && $storedToken->getRefreshToken() !== $originalRefreshToken) {
+                return $storedToken;
+            }
+
+            // The middleware refreshes the token, and saves it before calling on_refresh_end.
+            return null;
         };
         $connectorOptions['on_refresh_end'] = function () use ($refreshLockName): void {
             $this->fileLock->release($refreshLockName);
@@ -460,6 +473,22 @@ private function isApiTokenInvalid(mixed $body): bool
         return false;
     }
 
+    /**
+     * Converts a session storage error into a keyring error, if applicable.
+     */
+    private function convertStorageException(\RuntimeException $e): \RuntimeException
+    {
+        if ($this->sessionStorage instanceof CredentialHelperStorage) {
+            $previous = $e->getPrevious();
+            if ($previous instanceof ProcessTimedOutException) {
+                return KeyringUnavailableException::fromTimeout($previous);
+            } elseif ($previous instanceof ProcessFailedException) {
+                return KeyringUnavailableException::fromFailure($previous);
+            }
+        }
+        return $e;
+    }
+
     /**
      * Loads and returns an AccessToken, if possible, from a session.
      *
@@ -549,15 +578,7 @@ public function getClient(bool $autoLogin = true, bool $reset = false): Platform
                 try {
                     $session->setStorage($this->sessionStorage);
                 } catch (\RuntimeException $e) {
-                    if ($this->sessionStorage instanceof CredentialHelperStorage) {
-                        $previous = $e->getPrevious();
-                        if ($previous instanceof ProcessTimedOutException) {
-                            throw KeyringUnavailableException::fromTimeout($previous);
-                        } elseif ($previous instanceof ProcessFailedException) {
-                            throw KeyringUnavailableException::fromFailure($previous);
-                        }
-                    }
-                    throw $e;
+                    throw $this->convertStorageException($e);
                 }
             }