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

[pjcdawkins]

Refresh tokens are single-use: re-sending a rotated refresh token makes the auth server revoke every token from that login, and the CLI then logs the user out.

The refresh lock's check read the in-memory session, which is loaded from storage only once, so it never saw another process's refresh. It also did not run when the lock was free. So a process that loaded its tokens before another process refreshed would refresh again with the rotated token.

on_refresh_start now reloads the session from storage after acquiring the lock. If the stored refresh token differs from the one about to be sent, it returns the stored token. Otherwise the middleware refreshes. If waiting for the lock times out, it fails instead of refreshing or saving without the lock.

This updates two libraries with fixes this relies on:

• platformsh/oauth2 1.0.0-beta4 saves a refreshed token before calling onRefreshEnd, so the lock is held until the new token is saved (fix: save refreshed token before onRefreshEnd runs platformsh/platformsh-oauth2-php#9).
• platformsh/client reads session files under a shared lock, and adds Session::reload() (Fix session reads and saves across processes platformsh/platformsh-client-php#108).

FileLock also used check-then-write on a timestamp in the lock file, so two processes could both acquire a free lock, and a crashed holder blocked others for 30s. It now holds a non-blocking flock(), which is atomic and released by the OS when the holder exits.

Tests:

• The mock auth server now rotates refresh tokens and detects reuse, like the real one.
• An integration test runs three processes on the same expired session. Before the fix, two of them reused the rotated token and were logged out.
• A PHPUnit test checks that a killed process does not keep the lock.
• PHPUnit tests cover the stored token being used when the lock is free and after waiting, holding the lock during a refresh, and failing after a lock timeout.

🤖 Generated with Claude Code

[upsun-dispatch]

Warning

Changes suggested — 🟡 1 warning · 🔵 1 minor point · ⚪ 1 nitpick

🔍 Full review · 4 files reviewed

⚪ Nitpick

• legacy/src/Service/Api.php:476 — loadStoredToken() was inserted between tokenFromSession() and its docblock. The old block ("Loads and returns an AccessToken, if possible, from a session. @PARAM SessionInterface $session ... @return AccessToken|null") now sits directly before the new docblock. It documents nothing, and tokenFromSession() is left without a docblock.

Verification

• The middleware's finally block calls on_refresh_end, so the lock is also released when on_refresh_start returns the stored token.
• FileLock::acquireOrWait releases the lock (only if held) before returning a non-null $check result, so the waiting path never leaks the lock.
• Session, AccessToken and SessionStorageInterface are already imported in Api.php, and Session::__construct(id, data) matches the new loadStoredToken call.
• No existing Go test relied on the removed fixed test-refresh-token: the other session fixtures use tokens that should fail or never refresh.

The PR adds a PHPUnit test (ApiRefreshLockTest, run by ./scripts/test/unit.sh in ci.yml) and a Go integration test (TestAuthRefresh_Concurrent, run by make integration-test in ci.yml) with a rotating mock auth server. Neither test targets the gap between releasing the lock and saving the token, so the integration test can only catch that race by chance.

Review details

• Commit: 7bb1941
• Model: claude-opus-5-5

---

Review 1 of 10 for this pull request · View the full run

[upsun-dispatch]

Note

Reviewed — No new issues found · 3 still open (1 nitpick)

🔁 Incremental · 2 files reviewed

Outstanding from earlier reviews:

• 🟡 #4149876246 — legacy/src/Service/Api.php:331: Concurrent CLI processes can still reuse a rotated token and log the user out. — This pass didn't touch this. on_refresh_end still releases the lock before the middleware saves the new token, and the switch to flock only makes the handoff instant, so the window is still open. Unlocked File::load reads can still come back as null.
• 🔵 #4149876253 — legacy/src/Service/Api.php:324: Repeated keyring calls while waiting; a keyring failure fails the refresh with a raw error. — $check still calls sessionStorage->load() on every 500 ms poll and once more after the lock is acquired. A credential-helper failure still surfaces as a raw RuntimeException.
• ⚪ legacy/src/Service/Api.php:476: The old docblock is orphaned and now misleads readers. — The orphaned 'Loads and returns an AccessToken...' docblock still sits above loadStoredToken's docblock, and tokenFromSession has none. (first raised)

Verification

• acquireOrWait closes its handle on both the timeout return and the $check return, so a waiter that gives up never keeps a lock or leaks a descriptor.
• release() unlocks and closes the handle and removes the key, and __destruct iterates array_keys($this->locks), which matches the new handle-valued map.
• open() uses fopen mode 'c', so it never truncates a lock file that another process holds, and its mkdir check tolerates a concurrent mkdir.
• When acquireOrWait times out and returns null, the on_refresh_start caller in Api.php still runs $check() before refreshing, so the stored-token check is not skipped.
• The killed-holder test spawns PHP directly through array-form proc_open, so proc_terminate(…, 9) kills the process that holds the flock and not a shell wrapper.

This pass adds legacy/tests/Service/FileLockTest.php. It covers waiting on a held lock and the lock being freed when the holder is SIGKILLed. It should run under the legacy-php job in .github/workflows/ci.yml. No test covers the timeout path, where the caller refreshes without the lock after 30 s.

Review details

• Commits: 7bb1941..61ea0c1
• Model: claude-opus-5-5

---

Review 2 of 10 for this pull request · View the full run

[upsun-dispatch]

📋 PR Summary

Stops concurrent CLI processes from re-sending a refresh token that has already been rotated, which causes the auth server to revoke the whole login. After taking the lock, on_refresh_start reloads the session from storage and uses the stored token if another process already refreshed it. If waiting for the lock times out, it fails instead of refreshing. FileLock now uses an atomic non-blocking flock() in place of the timestamp check-then-write. The latest push only tightens the oauth2 dependency constraint to ^1.0.0-beta4.

Changes

Layer / File(s)	Summary
Dependencies
legacy/composer.json	Requires platformsh/oauth2 ^1.0.0-beta4 instead of ^1@beta. That version saves the refreshed token before calling onRefreshEnd.
legacy/composer.lock	Lock file updated for the new oauth2 and client library versions.
Refresh locking
legacy/src/Service/Api.php	The refresh-start hook acquires the lock, reloads the session from storage, and returns the stored token if it differs from the one about to be sent. It fails if the lock times out.
legacy/src/Service/FileLock.php	Replaces the timestamp-based lock with a non-blocking flock(), which the OS releases when the holder exits.
Tests
legacy/tests/LockHolderTrait.php	Test helper that holds the lock from a separate process.
legacy/tests/Service/ApiRefreshLockTest.php	Covers using the stored token, holding the lock during a refresh, and failing after a lock timeout.
legacy/tests/Service/FileLockTest.php	Checks that a killed process does not keep the lock.
integration-tests/auth_refresh_test.go	Runs three processes on the same expired session and checks that none of them reuses a rotated token.
pkg/mockapi/auth_server.go	The mock auth server now rotates refresh tokens and detects reuse, like the real server.

[upsun-dispatch]

Warning

Changes suggested — 🟡 1 warning · 🔵 1 minor point

🔁 Incremental · 6 files reviewed

Verification

• connector->saveToken() now runs inside on_refresh_start, before the middleware's finally calls on_refresh_end to release the lock, so the save-after-release window is closed.
• FileStorage::load reads under LOCK_SH, and File::save uses file_put_contents(..., LOCK_EX), which truncates only after taking the lock. A concurrent read therefore cannot see an empty or partial file.
• An IdentityProviderException thrown by the refresh inside on_refresh_start is still caught by the middleware's catch, which calls on_refresh_error, and its finally still releases the lock.
• The per-poll storage reload is gone. loadStoredToken now runs once after the lock, and its storage errors go through convertStorageException into KeyringUnavailableException.
• LockHolderTrait resolves dirname(__DIR__)/vendor/autoload.php from legacy/tests, which points at the same legacy/vendor as the old helper's dirname(__DIR__, 2) path from tests/Service.

ApiRefreshLockTest now covers three cases: the stored token used when the lock is free, the stored token used after waiting on a real holder process, and the refreshed token saved before returning (checked against a PHP built-in mock token server). FileLockTest covers a killed holder releasing the lock. Nothing tests the path where acquireOrWait times out, or storage via the credential helper.

Review details

• Commits: 61ea0c1..75f7d25
• Model: claude-opus-5-5

---

Review 3 of 10 for this pull request · View the full run

[upsun-dispatch]

Note

Reviewed — No new issues found · 1 still open

🔁 Incremental · 4 files reviewed

Outstanding from earlier reviews:

• 🔵 #4149931017 — legacy/src/Service/Api.php:348: Adds a redundant keyring or file write on every token refresh. — Still stands: $connector->saveToken($token) runs, and then the middleware saves the returned token again. The author accepts this and has said it will stay.

Verification

• on_refresh_start now throws instead of refreshing with $originalRefreshToken when acquireOrWait() timed out without taking the lock.
• The stored-token check still runs before the isHeld guard, so a timed-out waiter still returns another process's rotated token rather than failing.
• isHeld() returns true when api.disable_locks is set, so turning locks off does not make every refresh throw.
• A lock this process already holds makes acquireOrWait() return early, and isHeld() still reports it as held, so the guard passes when the lock is re-entered.

This push adds two tests. ApiRefreshLockTest::testFailsWithoutRefreshingAfterTimingOut injects a 1-second FileLock while a child process holds the lock, and FileLockTest::testIsHeldOnlyWhenAcquired covers isHeld() for the holder and for a waiter that timed out. The vendored middleware is not installed in this sandbox, so I could not check how it handles an exception thrown from on_refresh_start.

Review details

• Commits: 75f7d25..f64d20e
• Model: claude-opus-5-5

---

Review 4 of 10 for this pull request · View the full run

[upsun-dispatch]

Note

Reviewed — No blocking findings · 🔵 1 minor point

🔁 Incremental · 6 files reviewed

🔵 Minor point

• legacy/composer.lock:886 — Api::getConnectorOptions() now relies on two behaviours that only platformsh/oauth2 1.0.0-beta4 provides. First, on_refresh_start can return null, which means "refresh it yourself". Second, GuzzleMiddleware::acquireAccessToken() saves the token through setAccessToken() before on_refresh_end releases the lock. This push only bumps the lock file, though. legacy/composer.json still requires "platformsh/oauth2": "^1@beta", and the platformsh/client 3.x commit it pins requires ^1.0.0@beta, so composer still treats beta3 as a valid install. If a resolution picks beta3 (for example composer update --prefer-lowest, or an update that ends up below beta4), on_refresh_end releases the lock before the refreshed token is saved. That brings back the race this PR fixes, where another process reads the old token and reuses the rotated refresh token.

Verification

• In oauth2 beta4, acquireAccessToken() calls setAccessToken() (which runs the token save callback that writes the session) before its finally calls on_refresh_end, so the lock is held until the save.
• If on_refresh_start throws the timeout error, the middleware's finally still calls on_refresh_end, and FileLock::release() does nothing for a lock this process does not hold.
• In the pinned client, File::load() reads under LOCK_SH, so the deleted FileStorage subclass is not needed.
• SessionInterface declares reload() and Session::reload() forces a reload from storage, so on_refresh_start sees another process's saved token.
• When on_refresh_start returns the stored token, Connector::saveToken() writes nothing, because Session::save() skips data identical to what was loaded.

ApiRefreshLockTest was updated for the null return, but it no longer covers the save happening before the release, because that logic now lives in the upstream oauth2 middleware. The cross-process case is covered by integration-tests/auth_refresh_test.go, which runs in the ci.yml integration-test job. No workflow in the repo runs the legacy PHPUnit suite.

Review details

• Commits: c667ee4..ba12621
• Model: claude-opus-5-5

---

Review 7 of 10 for this pull request · View the full run