fix(oauth): degrade to re-auth on corrupt credential cache instead of crashing (#208)

* fix(oauth): degrade to re-auth on corrupt credential cache instead of crashing

DirectoryPersistence.readTokens/readClientInfo/readState routed a corrupt
(truncated or malformed) cache file's JSON.parse SyntaxError straight up
through CompositePersistence, crashing the MCP connection instead of degrading
to re-auth. Every sibling reader already tolerates this: VaultPersistence
(oauth-vault.ts) catches SyntaxError to rebuild, and the daemon/server-proxy
readers wrap in catch-all. These three were the lone outliers.

Wrap the three JSON readers in a narrow helper that maps only SyntaxError to
undefined; genuine I/O faults still propagate. readJsonFile is left untouched so
the vault keeps distinguishing corrupt-from-missing for its repair path.

Fixes #207.

* fix(oauth): keep corrupt OAuth state failing closed (addresses Codex P2)

Codex review on #208 noted that making readState() corrupt-tolerant skips the
CSRF state check on the authorization callback: oauth.ts only rejects when the
stored expectedState is truthy, so a corrupt state.txt degrading to undefined
would let a mismatched/absent callback state through.

Narrow the tolerance to the credential caches only (readTokens/readClientInfo).
readState() keeps throwing on corrupt input so the OAuth flow fails closed.
Test now asserts state reads reject with SyntaxError while tokens/client degrade.

* test(oauth): preserve callback cleanup for read errors

---------

Co-authored-by: KrasimirKralev <krasi@idrobots.com>
Co-authored-by: Vincent Koc <25068+vincentkoc@users.noreply.github.com>

src/oauth-persistence.ts

@@ -152,7 +152,7 @@ class DirectoryPersistence implements OAuthPersistence {
   }
 
   async readTokens(): Promise<OAuthTokens | undefined> {
-    return readJsonFile<OAuthTokens>(this.tokenPath);
+    return this.readJsonOrUndefined<OAuthTokens>(this.tokenPath);
   }
 
   async saveTokens(tokens: OAuthTokens): Promise<void> {
@@ -162,7 +162,7 @@ class DirectoryPersistence implements OAuthPersistence {
   }
 
   async readClientInfo(): Promise<OAuthClientInformationMixed | undefined> {
-    return readJsonFile<OAuthClientInformationMixed>(this.clientInfoPath);
+    return this.readJsonOrUndefined<OAuthClientInformationMixed>(this.clientInfoPath);
   }
 
   async saveClientInfo(info: OAuthClientInformationMixed): Promise<void> {
@@ -187,9 +187,31 @@ class DirectoryPersistence implements OAuthPersistence {
   }
 
   async readState(): Promise<string | undefined> {
+    // Deliberately NOT corrupt-tolerant: a corrupt OAuth state must fail the
+    // flow closed. Returning undefined here would skip the CSRF state check on
+    // the authorization callback (see oauth.ts), so only the credential caches
+    // (tokens/client) degrade to re-auth.
     return readJsonFile<string>(this.statePath);
   }
 
+  // A present-but-corrupt credential cache (tokens/client) means "no usable
+  // credentials": degrade to re-auth instead of crashing the connection,
+  // mirroring VaultPersistence and the daemon/server-proxy readers. Genuine I/O
+  // faults still propagate (readJsonFile re-throws everything except ENOENT).
+  // OAuth state is intentionally excluded (see readState) so its CSRF check
+  // still fails closed on a corrupt state file.
+  private async readJsonOrUndefined<T>(filePath: string): Promise<T | undefined> {
+    try {
+      return await readJsonFile<T>(filePath);
+    } catch (error) {
+      if (!(error instanceof SyntaxError)) {
+        throw error;
+      }
+      this.logger?.debug?.(`Ignoring corrupt OAuth cache file ${filePath}: ${error.message}`);
+      return undefined;
+    }
+  }
+
   async saveState(value: string): Promise<void> {
     await this.ensureDir();
     await writeJsonFile(this.statePath, value);

src/oauth.ts

@@ -150,8 +150,8 @@ class PersistentOAuthClientProvider implements OAuthClientProvider {
     // previous client registration is cached with a different redirect URI the
     // auth server will reject the request with `invalid_redirect_uri`.  Clear
     // the stale registration so the next flow re-registers with the new URI.
-    // Wrapped in try/catch so persistence errors (malformed JSON, permission
-    // issues) close the already-bound callback server instead of leaking it.
+    // Wrapped in try/catch so non-recoverable persistence errors (for example,
+    // permission issues) close the already-bound callback server instead of leaking it.
     if (usesDynamicPort) {
       try {
         const cachedClient = await persistence.readClientInfo();

tests/oauth-persistence.test.ts

@@ -44,6 +44,30 @@ describe('oauth persistence', () => {
     await Promise.all(tempRoots.splice(0).map((dir) => fs.rm(dir, { recursive: true, force: true })));
   });
 
+  it('degrades corrupt credential caches to undefined but keeps corrupt OAuth state failing closed', async () => {
+    const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'mcporter-oauth-corrupt-'));
+    tempRoots.push(tmp);
+    homedirSpy = vi.spyOn(os, 'homedir').mockReturnValue(tmp);
+    hasSpy = true;
+
+    const cacheDir = path.join(tmp, 'cache');
+    await fs.mkdir(cacheDir, { recursive: true });
+    // Truncated / malformed credential files, e.g. an interrupted write.
+    await fs.writeFile(path.join(cacheDir, 'tokens.json'), '{ "access_token": "part');
+    await fs.writeFile(path.join(cacheDir, 'client.json'), 'not json at all');
+    await fs.writeFile(path.join(cacheDir, 'state.txt'), '"unterminated');
+
+    const persistence = await buildOAuthPersistence(mkDef('service', cacheDir));
+
+    // Corrupt credential caches must read as "no usable credentials" (degrade to
+    // re-auth), not surface a SyntaxError that crashes the connection.
+    expect(await persistence.readTokens()).toBeUndefined();
+    expect(await persistence.readClientInfo()).toBeUndefined();
+    // OAuth state must NOT silently degrade: returning undefined would skip the
+    // CSRF state check on callback (oauth.ts). It must fail closed.
+    await expect(persistence.readState()).rejects.toThrow(SyntaxError);
+  });
+
   it('prefers explicit tokenCacheDir before vault when reading tokens', async () => {
     const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'mcporter-oauth-'));
     tempRoots.push(tmp);

tests/oauth-session.test.ts

@@ -148,10 +148,9 @@ describe('FileOAuthClientProvider session lifecycle', () => {
     expect(logger.info).toHaveBeenCalledWith(expect.stringContaining('clearing stale client registration'));
   });
 
-  it('closes the callback server when stale-client reads throw', async () => {
+  it('closes the callback server when stale-client reads have I/O errors', async () => {
     const tokenCacheDir = await fs.mkdtemp(path.join(os.tmpdir(), 'mcporter-oauth-test-'));
     tempDirs.push(tokenCacheDir);
-    await fs.writeFile(path.join(tokenCacheDir, 'client.json'), '{not-valid-json', 'utf8');
     const definition: ServerDefinition = {
       name: 'test-oauth-read-failure',
       description: 'Test OAuth server',
@@ -165,6 +164,8 @@ describe('FileOAuthClientProvider session lifecycle', () => {
       error: vi.fn(),
     };
 
+    const readError = Object.assign(new Error('permission denied'), { code: 'EACCES' });
+    const readFileSpy = vi.spyOn(fs, 'readFile').mockRejectedValueOnce(readError);
     const originalCreateServer = http.createServer.bind(http);
     const createdServers: http.Server[] = [];
     const createServerSpy = vi.spyOn(http, 'createServer').mockImplementation((...args) => {
@@ -174,11 +175,12 @@ describe('FileOAuthClientProvider session lifecycle', () => {
     });
 
     try {
-      await expect(createOAuthSession(definition, logger)).rejects.toThrow(SyntaxError);
+      await expect(createOAuthSession(definition, logger)).rejects.toMatchObject({ code: 'EACCES' });
       await new Promise((resolve) => setTimeout(resolve, 0));
       expect(createdServers).toHaveLength(1);
       expect(createdServers[0]?.listening).toBe(false);
     } finally {
+      readFileSpy.mockRestore();
       createServerSpy.mockRestore();
     }
   });