From 53747cac636285c65c0c7f64b9a16057d55c1484 Mon Sep 17 00:00:00 2001 From: Krasimir Kralev <263465593+KrasimirKralev@users.noreply.github.com> Date: Wed, 17 Jun 2026 12:32:24 +0300 Subject: [PATCH] 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 Co-authored-by: Vincent Koc <25068+vincentkoc@users.noreply.github.com> --- src/oauth-persistence.ts | 26 ++++++++++++++++++++++++-- src/oauth.ts | 4 ++-- tests/oauth-persistence.test.ts | 24 ++++++++++++++++++++++++ tests/oauth-session.test.ts | 8 +++++--- 4 files changed, 55 insertions(+), 7 deletions(-) diff --git a/src/oauth-persistence.ts b/src/oauth-persistence.ts index 5b8233d..46fd1e0 100644 --- a/src/oauth-persistence.ts +++ b/src/oauth-persistence.ts @@ -152,7 +152,7 @@ class DirectoryPersistence implements OAuthPersistence { } async readTokens(): Promise { - return readJsonFile(this.tokenPath); + return this.readJsonOrUndefined(this.tokenPath); } async saveTokens(tokens: OAuthTokens): Promise { @@ -162,7 +162,7 @@ class DirectoryPersistence implements OAuthPersistence { } async readClientInfo(): Promise { - return readJsonFile(this.clientInfoPath); + return this.readJsonOrUndefined(this.clientInfoPath); } async saveClientInfo(info: OAuthClientInformationMixed): Promise { @@ -187,9 +187,31 @@ class DirectoryPersistence implements OAuthPersistence { } async readState(): Promise { + // 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(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(filePath: string): Promise { + try { + return await readJsonFile(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 { await this.ensureDir(); await writeJsonFile(this.statePath, value); diff --git a/src/oauth.ts b/src/oauth.ts index 054209a..68a8bc7 100644 --- a/src/oauth.ts +++ b/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(); diff --git a/tests/oauth-persistence.test.ts b/tests/oauth-persistence.test.ts index 4ebc548..1af9f47 100644 --- a/tests/oauth-persistence.test.ts +++ b/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); diff --git a/tests/oauth-session.test.ts b/tests/oauth-session.test.ts index a138704..890a058 100644 --- a/tests/oauth-session.test.ts +++ b/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(); } });