From ddd5a65c7e00955104e6f566ab9b5f7b6ffe4743 Mon Sep 17 00:00:00 2001 From: Mario Zechner Date: Fri, 6 Feb 2026 19:01:42 +0100 Subject: [PATCH] fix(coding-agent): handle compromised auth lock without crashing closes #1322 --- .../coding-agent/src/core/auth-storage.ts | 17 ++++++ .../coding-agent/test/auth-storage.test.ts | 54 ++++++++++++++++++- 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/packages/coding-agent/src/core/auth-storage.ts b/packages/coding-agent/src/core/auth-storage.ts index 21b62153..c4dbaa73 100644 --- a/packages/coding-agent/src/core/auth-storage.ts +++ b/packages/coding-agent/src/core/auth-storage.ts @@ -196,6 +196,13 @@ export class AuthStorage { } let release: (() => Promise) | undefined; + let lockCompromised = false; + let lockCompromisedError: Error | undefined; + const throwIfLockCompromised = () => { + if (lockCompromised) { + throw lockCompromisedError ?? new Error("OAuth refresh lock was compromised"); + } + }; try { // Acquire exclusive lock with retry and timeout @@ -209,8 +216,14 @@ export class AuthStorage { randomize: true, }, stale: 30000, // Consider lock stale after 30 seconds + onCompromised: (err) => { + lockCompromised = true; + lockCompromisedError = err; + }, }); + throwIfLockCompromised(); + // Re-read file after acquiring lock - another instance may have refreshed this.reload(); @@ -223,6 +236,7 @@ export class AuthStorage { // (another instance may have already refreshed it) if (Date.now() < cred.expires) { // Token is now valid - another instance refreshed it + throwIfLockCompromised(); const apiKey = provider.getApiKey(cred); return { apiKey, newCredentials: cred }; } @@ -237,11 +251,14 @@ export class AuthStorage { const result = await getOAuthApiKey(providerId, oauthCreds); if (result) { + throwIfLockCompromised(); this.data[providerId] = { type: "oauth", ...result.newCredentials }; this.save(); + throwIfLockCompromised(); return result; } + throwIfLockCompromised(); return null; } finally { // Always release the lock diff --git a/packages/coding-agent/test/auth-storage.test.ts b/packages/coding-agent/test/auth-storage.test.ts index 0fdab3c8..0b395538 100644 --- a/packages/coding-agent/test/auth-storage.test.ts +++ b/packages/coding-agent/test/auth-storage.test.ts @@ -1,7 +1,9 @@ import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { afterEach, beforeEach, describe, expect, test } from "vitest"; +import { registerOAuthProvider } from "@mariozechner/pi-ai"; +import lockfile from "proper-lockfile"; +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { AuthStorage } from "../src/core/auth-storage.js"; import { clearConfigValueCache } from "../src/core/resolve-config-value.js"; @@ -21,6 +23,7 @@ describe("AuthStorage", () => { rmSync(tempDir, { recursive: true }); } clearConfigValueCache(); + vi.restoreAllMocks(); }); function writeAuthJson(data: Record) { @@ -287,6 +290,55 @@ describe("AuthStorage", () => { }); }); + describe("oauth lock compromise handling", () => { + test("returns undefined on compromised lock and allows a later retry", async () => { + const providerId = `test-oauth-provider-${Date.now()}-${Math.random().toString(36).slice(2)}`; + registerOAuthProvider({ + id: providerId, + name: "Test OAuth Provider", + async login() { + throw new Error("Not used in this test"); + }, + async refreshToken(credentials) { + return { + ...credentials, + access: "refreshed-access-token", + expires: Date.now() + 60_000, + }; + }, + getApiKey(credentials) { + return `Bearer ${credentials.access}`; + }, + }); + + writeAuthJson({ + [providerId]: { + type: "oauth", + refresh: "refresh-token", + access: "expired-access-token", + expires: Date.now() - 10_000, + }, + }); + + authStorage = new AuthStorage(authJsonPath); + + const realLock = lockfile.lock.bind(lockfile); + const lockSpy = vi.spyOn(lockfile, "lock"); + lockSpy.mockImplementationOnce(async (file, options) => { + options?.onCompromised?.(new Error("Unable to update lock within the stale threshold")); + return realLock(file, options); + }); + + const firstTry = await authStorage.getApiKey(providerId); + expect(firstTry).toBeUndefined(); + + lockSpy.mockRestore(); + + const secondTry = await authStorage.getApiKey(providerId); + expect(secondTry).toBe("Bearer refreshed-access-token"); + }); + }); + describe("runtime overrides", () => { test("runtime override takes priority over auth.json", async () => { writeAuthJson({