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(); } });