From 255f42d6c7bd17ab64fdb37c1dd57c117118a4e1 Mon Sep 17 00:00:00 2001 From: Tom Petr Date: Fri, 16 Oct 2026 21:16:58 +0000 Subject: [PATCH] fix(mcp): authenticate lazily instead of polling idle accounts --- modules/linear-mcp/README.md | 17 +++++++---- modules/linear-mcp/scripts/linear-mcp.mjs | 19 +++--------- modules/linear-mcp/tests/adapter.test.mjs | 35 +++++++++++++++++++---- modules/notion-mcp/README.md | 15 +++++++--- modules/notion-mcp/scripts/notion-mcp.mjs | 19 +++--------- modules/notion-mcp/tests/adapter.test.mjs | 35 +++++++++++++++++++---- modules/slack-mcp/README.md | 15 +++++++--- modules/slack-mcp/scripts/slack-mcp.mjs | 19 +++--------- modules/slack-mcp/tests/adapter.test.mjs | 35 +++++++++++++++++++---- 9 files changed, 135 insertions(+), 74 deletions(-) diff --git a/modules/linear-mcp/README.md b/modules/linear-mcp/README.md index d9212ff..cb0546a 100644 --- a/modules/linear-mcp/README.md +++ b/modules/linear-mcp/README.md @@ -69,12 +69,17 @@ the Coder linking page is retained; query parameters and raw subprocess errors are never printed. If the credential context is unavailable and Coder cannot supply a URL, the reply gives the CLI command to retrieve it. -The adapter checks authentication every 30 seconds. After linking Linear in -Coder, it sends `notifications/tools/list_changed` when tools become available. -Clients that honor that notification update automatically; clients that cache -tool lists may require a reconnect. Unlinking hides tools again. Rejected -upstream credentials also hide tools while keeping the local MCP healthy. -Network outages return a sanitized MCP error and can recover on later requests. +Authentication is lazy: initialization and idle time do not retrieve credentials +or contact the provider. Listing or calling tools retrieves a current Coder token +for each upstream HTTP request, without a separate preflight check. There is no +background authentication timer or credential cache. + +After linking in Coder, refresh the client's tool list or reconnect if it caches +an empty list. Account changes are discovered on the next request, rather than +while idle; unlinking or HTTP 401/403 hides tools and keeps the local MCP healthy. +Tool-list-change notifications are sent when a request connects or disconnects +the upstream client. Upstream outages return sanitized errors and recover on +later requests. ## Installation and credentials diff --git a/modules/linear-mcp/scripts/linear-mcp.mjs b/modules/linear-mcp/scripts/linear-mcp.mjs index 7a301bb..3432f8e 100644 --- a/modules/linear-mcp/scripts/linear-mcp.mjs +++ b/modules/linear-mcp/scripts/linear-mcp.mjs @@ -56,7 +56,6 @@ export function createAdapter({ providerId = 'linear', accessMode = 'read-only', let connecting; let initialized = false; let closed = false; - let poll; let shutdownPromise; let reportedAuthUrl; const notify = () => { @@ -86,9 +85,7 @@ export function createAdapter({ providerId = 'linear', accessMode = 'read-only', }; const ensureClient = async () => { if (closed) throw new Error('Adapter closed'); - // Check even when a session exists: account unlinking must hide tools. - await getToken(); - if (closed) throw new Error('Adapter closed'); + // Authentication is checked by authenticatedFetch for each upstream request. if (client) return client; if (!connecting) connecting = (async () => { const candidate = new Client({ name: 'coder-linear-mcp', version: '1.0.0' }); @@ -142,26 +139,18 @@ export function createAdapter({ providerId = 'linear', accessMode = 'read-only', }; } }); - // Clients can discover newly linked accounts without restarting. Also - // detect account removal while idle; revoked tokens are caught by fetch. - const checkAuth = async () => { - try { await ensureClient(); } catch (error) { reportAuth(error); await disconnect(); } - }; + // Stay idle until the client lists or calls tools. Coder may validate tokens + // against the provider, so even an idle authentication poll can consume quota. server.oninitialized = () => { initialized = true; - poll = setInterval(() => { checkAuth(); }, 30000); - poll.unref(); - // Defer until the initial stdio handshake has completed. - setImmediate(() => { checkAuth(); }); }; const close = () => shutdownPromise ??= (async () => { closed = true; - clearInterval(poll); await disconnect(); await connecting?.catch(() => {}); await server.close(); })(); - return { server, checkAuth, close }; + return { server, close }; } async function main() { diff --git a/modules/linear-mcp/tests/adapter.test.mjs b/modules/linear-mcp/tests/adapter.test.mjs index 9efe3d6..ea1ca26 100644 --- a/modules/linear-mcp/tests/adapter.test.mjs +++ b/modules/linear-mcp/tests/adapter.test.mjs @@ -46,6 +46,31 @@ async function connect(t, options) { return { adapter, client }; } +test('initialization and idle time do not acquire credentials or contact the provider', async t => { + t.mock.timers.enable({ apis: ['setInterval'] }); + let credentials = 0; + const remote = fakeLinear(); + const { client } = await connect(t, { + getToken: async () => { credentials++; return 'fake-token'; }, + fetchImpl: remote.fetchImpl, + }); + await new Promise(resolve => setImmediate(resolve)); + assert.equal(credentials, 0); + assert.equal(remote.requests.length, 0); + t.mock.timers.tick(120000); + await new Promise(resolve => setImmediate(resolve)); + assert.equal(credentials, 0); + assert.equal(remote.requests.length, 0); + + await client.listTools(); + const used = credentials; + assert.ok(used > 0); + t.mock.timers.tick(120000); + await new Promise(resolve => setImmediate(resolve)); + assert.equal(credentials, used); + assert.equal(remote.requests.length, used); +}); + test('unlinked accounts initialize successfully and expose zero tools without network calls', async t => { const { client } = await connect(t, { getToken: async () => { throw new AuthUnavailable(); }, @@ -61,7 +86,7 @@ test('unlinked accounts initialize successfully and expose zero tools without ne test('linking and unlinking update tools on the same MCP connection', async t => { let linked = false; const remote = fakeLinear(); - const { adapter, client } = await connect(t, { + const { client } = await connect(t, { getToken: async () => { if (!linked) throw new AuthUnavailable(); return 'fake-token'; }, fetchImpl: remote.fetchImpl, }); @@ -69,11 +94,10 @@ test('linking and unlinking update tools on the same MCP connection', async t => let changes = 0; client.setNotificationHandler(ToolListChangedNotificationSchema, () => { changes++; }); linked = true; - await adapter.checkAuth(); assert.equal((await client.listTools()).tools[0].name, 'list_issues'); assert.equal((await client.callTool({ name: 'list_issues', arguments: {} })).content[0].text, 'Example issue'); linked = false; - await adapter.checkAuth(); + assert.equal((await client.callTool({ name: 'list_issues', arguments: {} })).isError, true); assert.deepEqual((await client.listTools()).tools, []); await client.ping(); assert.ok(changes >= 2); @@ -90,6 +114,7 @@ test('each upstream request retrieves a fresh Coder token, including long sessio const tokens = remote.requests.map(request => request.token); assert.ok(tokens.length >= 3); assert.equal(new Set(tokens).size, tokens.length); + assert.equal(credentials, remote.requests.length, 'no redundant token preflight checks'); }); test('rejected OAuth credentials leave a healthy empty MCP without browser authentication', async t => { @@ -158,7 +183,7 @@ test('invalid access modes cannot select an arbitrary endpoint', () => { test('unlinked tool replies include Coder linking URL and diagnostics are emitted once', async t => { const url = 'https://coder.example.test/external-auth/linear'; const messages = []; - const { adapter, client } = await connect(t, { + const { client } = await connect(t, { getToken: async () => { throw new AuthUnavailable(url); }, log: message => messages.push(message), fetchImpl: async () => assert.fail('must not contact Linear'), @@ -167,7 +192,7 @@ test('unlinked tool replies include Coder linking URL and diagnostics are emitte const result = await client.callTool({ name: 'list_issues', arguments: {} }); assert.equal(result.isError, true); assert.ok(result.content[0].text.includes(url)); - await adapter.checkAuth(); + await client.listTools(); await client.ping(); assert.deepEqual(messages, [`Linear MCP: link your account at ${url}`]); }); diff --git a/modules/notion-mcp/README.md b/modules/notion-mcp/README.md index daff304..dfa0a0b 100644 --- a/modules/notion-mcp/README.md +++ b/modules/notion-mcp/README.md @@ -95,10 +95,17 @@ Coder supplies a linking URL, the adapter includes it in the reply and prints it once on stderr, stripping query parameters and raw subprocess diagnostics. Otherwise the reply gives the CLI command to obtain the link. -The adapter checks authentication every 30 seconds and sends tool-list-change -notifications after linking/unlinking. Clients that cache tools may need to -reconnect. HTTP 401/403 hides tools without launching a separate browser OAuth -flow; upstream outages produce sanitized errors and recover on later calls. +Authentication is lazy: initialization and idle time do not retrieve credentials +or contact the provider. Listing or calling tools retrieves a current Coder token +for each upstream HTTP request, without a separate preflight check. There is no +background authentication timer or credential cache. + +After linking in Coder, refresh the client's tool list or reconnect if it caches +an empty list. Account changes are discovered on the next request, rather than +while idle; unlinking or HTTP 401/403 hides tools and keeps the local MCP healthy. +Tool-list-change notifications are sent when a request connects or disconnects +the upstream client. Upstream outages return sanitized errors and recover on +later requests. Every upstream request fetches a current token with `coder external-auth access-token `. Tokens remain in memory, diff --git a/modules/notion-mcp/scripts/notion-mcp.mjs b/modules/notion-mcp/scripts/notion-mcp.mjs index 3fad72b..2e1a456 100644 --- a/modules/notion-mcp/scripts/notion-mcp.mjs +++ b/modules/notion-mcp/scripts/notion-mcp.mjs @@ -87,7 +87,6 @@ export function createAdapter({ providerId = 'notion', accessMode = 'read-only', let connecting; let initialized = false; let closed = false; - let poll; let shutdownPromise; let reportedAuthUrl; const notify = () => { @@ -117,9 +116,7 @@ export function createAdapter({ providerId = 'notion', accessMode = 'read-only', }; const ensureClient = async () => { if (closed) throw new Error('Adapter closed'); - // Check even when a session exists: account unlinking must hide tools. - await getToken(); - if (closed) throw new Error('Adapter closed'); + // Authentication is checked by authenticatedFetch for each upstream request. if (client) return client; if (!connecting) connecting = (async () => { const candidate = new Client({ name: 'coder-notion-mcp', version: '1.0.0' }); @@ -195,26 +192,18 @@ export function createAdapter({ providerId = 'notion', accessMode = 'read-only', }; } }); - // Clients can discover newly linked accounts without restarting. Also - // detect account removal while idle; revoked tokens are caught by fetch. - const checkAuth = async () => { - try { await ensureClient(); } catch (error) { reportAuth(error); await disconnect(); } - }; + // Stay idle until the client lists or calls tools. Coder may validate tokens + // against the provider, so even an idle authentication poll can consume quota. server.oninitialized = () => { initialized = true; - poll = setInterval(() => { checkAuth(); }, 30000); - poll.unref(); - // Defer until the initial stdio handshake has completed. - setImmediate(() => { checkAuth(); }); }; const close = () => shutdownPromise ??= (async () => { closed = true; - clearInterval(poll); await disconnect(); await connecting?.catch(() => {}); await server.close(); })(); - return { server, checkAuth, close }; + return { server, close }; } async function main() { diff --git a/modules/notion-mcp/tests/adapter.test.mjs b/modules/notion-mcp/tests/adapter.test.mjs index ef79dc1..36a48c7 100644 --- a/modules/notion-mcp/tests/adapter.test.mjs +++ b/modules/notion-mcp/tests/adapter.test.mjs @@ -49,6 +49,31 @@ async function connect(t, options) { return { adapter, client }; } +test('initialization and idle time do not acquire credentials or contact the provider', async t => { + t.mock.timers.enable({ apis: ['setInterval'] }); + let credentials = 0; + const remote = fakeNotion(); + const { client } = await connect(t, { + getToken: async () => { credentials++; return 'fake-token'; }, + fetchImpl: remote.fetchImpl, + }); + await new Promise(resolve => setImmediate(resolve)); + assert.equal(credentials, 0); + assert.equal(remote.requests.length, 0); + t.mock.timers.tick(120000); + await new Promise(resolve => setImmediate(resolve)); + assert.equal(credentials, 0); + assert.equal(remote.requests.length, 0); + + await client.listTools(); + const used = credentials; + assert.ok(used > 0); + t.mock.timers.tick(120000); + await new Promise(resolve => setImmediate(resolve)); + assert.equal(credentials, used); + assert.equal(remote.requests.length, used); +}); + test('unlinked accounts initialize successfully and expose zero tools without network calls', async t => { const { client } = await connect(t, { getToken: async () => { throw new AuthUnavailable(); }, @@ -64,7 +89,7 @@ test('unlinked accounts initialize successfully and expose zero tools without ne test('linking and unlinking update tools on the same MCP connection', async t => { let linked = false; const remote = fakeNotion(); - const { adapter, client } = await connect(t, { + const { client } = await connect(t, { getToken: async () => { if (!linked) throw new AuthUnavailable(); return 'fake-token'; }, fetchImpl: remote.fetchImpl, }); @@ -72,11 +97,10 @@ test('linking and unlinking update tools on the same MCP connection', async t => let changes = 0; client.setNotificationHandler(ToolListChangedNotificationSchema, () => { changes++; }); linked = true; - await adapter.checkAuth(); assert.equal((await client.listTools()).tools[0].name, 'notion-fetch'); assert.equal((await client.callTool({ name: 'notion-fetch', arguments: {} })).content[0].text, 'Example page'); linked = false; - await adapter.checkAuth(); + assert.equal((await client.callTool({ name: 'notion-search', arguments: {} })).isError, true); assert.deepEqual((await client.listTools()).tools, []); await client.ping(); assert.ok(changes >= 2); @@ -93,6 +117,7 @@ test('each upstream request retrieves a fresh Coder token, including long sessio const tokens = remote.requests.map(request => request.token); assert.ok(tokens.length >= 3); assert.equal(new Set(tokens).size, tokens.length); + assert.equal(credentials, remote.requests.length, 'no redundant token preflight checks'); }); test('rejected OAuth credentials leave a healthy empty MCP without browser authentication', async t => { @@ -161,7 +186,7 @@ test('invalid access modes cannot select an arbitrary endpoint', () => { test('unlinked tool replies include Coder linking URL and diagnostics are emitted once', async t => { const url = 'https://coder.example.test/external-auth/notion'; const messages = []; - const { adapter, client } = await connect(t, { + const { client } = await connect(t, { getToken: async () => { throw new AuthUnavailable(url); }, log: message => messages.push(message), fetchImpl: async () => assert.fail('must not contact Notion'), @@ -170,7 +195,7 @@ test('unlinked tool replies include Coder linking URL and diagnostics are emitte const result = await client.callTool({ name: 'notion-fetch', arguments: {} }); assert.equal(result.isError, true); assert.ok(result.content[0].text.includes(url)); - await adapter.checkAuth(); + await client.listTools(); await client.ping(); assert.deepEqual(messages, [`Notion MCP: link your account at ${url}`]); }); diff --git a/modules/slack-mcp/README.md b/modules/slack-mcp/README.md index dcb41c4..f12a464 100644 --- a/modules/slack-mcp/README.md +++ b/modules/slack-mcp/README.md @@ -79,10 +79,17 @@ read-tool error replies and printed once to stderr. Only the linking-page URL is retained; query parameters and raw subprocess diagnostics are not relayed. If no URL is available, the reply gives the CLI command to obtain one. -The adapter polls authentication every 30 seconds and notifies clients when -linking/unlinking changes tool availability. Clients that cache tools may need -a reconnect. Rejected credentials hide tools while keeping MCP healthy; -upstream outages produce sanitized MCP errors and can recover on later calls. +Authentication is lazy: initialization and idle time do not retrieve credentials +or contact the provider. Listing or calling tools retrieves a current Coder token +for each upstream HTTP request, without a separate preflight check. There is no +background authentication timer or credential cache. + +After linking in Coder, refresh the client's tool list or reconnect if it caches +an empty list. Account changes are discovered on the next request, rather than +while idle; unlinking or HTTP 401/403 hides tools and keeps the local MCP healthy. +Tool-list-change notifications are sent when a request connects or disconnects +the upstream client. Upstream outages return sanitized errors and recover on +later requests. Each upstream request obtains a current token using `coder external-auth access-token `. Coder manages refresh. diff --git a/modules/slack-mcp/scripts/slack-mcp.mjs b/modules/slack-mcp/scripts/slack-mcp.mjs index 22c951e..3ef4ace 100644 --- a/modules/slack-mcp/scripts/slack-mcp.mjs +++ b/modules/slack-mcp/scripts/slack-mcp.mjs @@ -67,7 +67,6 @@ export function createAdapter({ providerId = 'slack', accessMode = 'read-only', let connecting; let initialized = false; let closed = false; - let poll; let shutdownPromise; let reportedAuthUrl; const notify = () => { @@ -97,9 +96,7 @@ export function createAdapter({ providerId = 'slack', accessMode = 'read-only', }; const ensureClient = async () => { if (closed) throw new Error('Adapter closed'); - // Check even when a session exists: account unlinking must hide tools. - await getToken(); - if (closed) throw new Error('Adapter closed'); + // Authentication is checked by authenticatedFetch for each upstream request. if (client) return client; if (!connecting) connecting = (async () => { const candidate = new Client({ name: 'coder-slack-mcp', version: '1.0.0' }); @@ -172,26 +169,18 @@ export function createAdapter({ providerId = 'slack', accessMode = 'read-only', }; } }); - // Clients can discover newly linked accounts without restarting. Also - // detect account removal while idle; revoked tokens are caught by fetch. - const checkAuth = async () => { - try { await ensureClient(); } catch (error) { reportAuth(error); await disconnect(); } - }; + // Stay idle until the client lists or calls tools. Coder may validate tokens + // against the provider, so even an idle authentication poll can consume quota. server.oninitialized = () => { initialized = true; - poll = setInterval(() => { checkAuth(); }, 30000); - poll.unref(); - // Defer until the initial stdio handshake has completed. - setImmediate(() => { checkAuth(); }); }; const close = () => shutdownPromise ??= (async () => { closed = true; - clearInterval(poll); await disconnect(); await connecting?.catch(() => {}); await server.close(); })(); - return { server, checkAuth, close }; + return { server, close }; } async function main() { diff --git a/modules/slack-mcp/tests/adapter.test.mjs b/modules/slack-mcp/tests/adapter.test.mjs index 2727dce..3a5f2e1 100644 --- a/modules/slack-mcp/tests/adapter.test.mjs +++ b/modules/slack-mcp/tests/adapter.test.mjs @@ -49,6 +49,31 @@ async function connect(t, options) { return { adapter, client }; } +test('initialization and idle time do not acquire credentials or contact the provider', async t => { + t.mock.timers.enable({ apis: ['setInterval'] }); + let credentials = 0; + const remote = fakeSlack(); + const { client } = await connect(t, { + getToken: async () => { credentials++; return 'fake-token'; }, + fetchImpl: remote.fetchImpl, + }); + await new Promise(resolve => setImmediate(resolve)); + assert.equal(credentials, 0); + assert.equal(remote.requests.length, 0); + t.mock.timers.tick(120000); + await new Promise(resolve => setImmediate(resolve)); + assert.equal(credentials, 0); + assert.equal(remote.requests.length, 0); + + await client.listTools(); + const used = credentials; + assert.ok(used > 0); + t.mock.timers.tick(120000); + await new Promise(resolve => setImmediate(resolve)); + assert.equal(credentials, used); + assert.equal(remote.requests.length, used); +}); + test('unlinked accounts initialize successfully and expose zero tools without network calls', async t => { const { client } = await connect(t, { getToken: async () => { throw new AuthUnavailable(); }, @@ -64,7 +89,7 @@ test('unlinked accounts initialize successfully and expose zero tools without ne test('linking and unlinking update tools on the same MCP connection', async t => { let linked = false; const remote = fakeSlack(); - const { adapter, client } = await connect(t, { + const { client } = await connect(t, { getToken: async () => { if (!linked) throw new AuthUnavailable(); return 'fake-token'; }, fetchImpl: remote.fetchImpl, }); @@ -72,11 +97,10 @@ test('linking and unlinking update tools on the same MCP connection', async t => let changes = 0; client.setNotificationHandler(ToolListChangedNotificationSchema, () => { changes++; }); linked = true; - await adapter.checkAuth(); assert.equal((await client.listTools()).tools[0].name, 'slack_read_channel'); assert.equal((await client.callTool({ name: 'slack_read_channel', arguments: {} })).content[0].text, 'Example message'); linked = false; - await adapter.checkAuth(); + assert.equal((await client.callTool({ name: 'slack_search_public', arguments: {} })).isError, true); assert.deepEqual((await client.listTools()).tools, []); await client.ping(); assert.ok(changes >= 2); @@ -93,6 +117,7 @@ test('each upstream request retrieves a fresh Coder token, including long sessio const tokens = remote.requests.map(request => request.token); assert.ok(tokens.length >= 3); assert.equal(new Set(tokens).size, tokens.length); + assert.equal(credentials, remote.requests.length, 'no redundant token preflight checks'); }); test('rejected OAuth credentials leave a healthy empty MCP without browser authentication', async t => { @@ -161,7 +186,7 @@ test('invalid access modes cannot select an arbitrary endpoint', () => { test('unlinked tool replies include Coder linking URL and diagnostics are emitted once', async t => { const url = 'https://coder.example.test/external-auth/slack'; const messages = []; - const { adapter, client } = await connect(t, { + const { client } = await connect(t, { getToken: async () => { throw new AuthUnavailable(url); }, log: message => messages.push(message), fetchImpl: async () => assert.fail('must not contact Slack'), @@ -170,7 +195,7 @@ test('unlinked tool replies include Coder linking URL and diagnostics are emitte const result = await client.callTool({ name: 'slack_read_channel', arguments: {} }); assert.equal(result.isError, true); assert.ok(result.content[0].text.includes(url)); - await adapter.checkAuth(); + await client.listTools(); await client.ping(); assert.deepEqual(messages, [`Slack MCP: link your account at ${url}`]); });