diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b5aed90..5ae573b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -15,6 +15,10 @@ jobs: with: node-version: "22" - run: npm ci + - run: npm ci + working-directory: mcp-server + - run: npm test + working-directory: mcp-server - run: node scripts/check-schema-drift.mjs - run: npm test - run: npx tsc --noEmit diff --git a/docs/0-requirements.ja.md b/docs/0-requirements.ja.md index 768f649..d793806 100644 --- a/docs/0-requirements.ja.md +++ b/docs/0-requirements.ja.md @@ -391,6 +391,9 @@ Requirements: - MCP client user を認証する - installation 済み repository に access する - GitHub API read に token を使う +- cached Dynamic Client Registration は、登録済み redirect URI 集合が今回の + OAuth flow で要求する localhost callback URI を全て含む場合だけ再利用し、 + 含まない場合は authorization 前に client registration を置き換える ## Storage Rules diff --git a/docs/0-requirements.md b/docs/0-requirements.md index eec4b8a..20690ac 100644 --- a/docs/0-requirements.md +++ b/docs/0-requirements.md @@ -393,6 +393,9 @@ Requirements: - authenticate the MCP client user - access repositories through the installed app - use GitHub tokens for API reads +- reuse a cached Dynamic Client Registration only when its registered redirect + URI set covers every localhost callback URI requested for the current OAuth + flow; otherwise register a replacement client before authorization ## Storage Rules diff --git a/mcp-server/README.md b/mcp-server/README.md index 750f2a2..600a71f 100644 --- a/mcp-server/README.md +++ b/mcp-server/README.md @@ -84,6 +84,11 @@ OAuth client registration and tokens are stored in: - `~/.github-rag-mcp/oauth-client.json` (dynamic client registration) - `~/.github-rag-mcp/oauth-tokens.json` (access + refresh tokens) +The proxy reuses a cached client registration only when it includes every +redirect URI required by the current localhost callback listener. Because the +listener uses a random port, a registration from a previous authorization may +be replaced automatically before the browser opens. + Delete these files to force a fresh authorization flow. ## Tools exposed @@ -101,7 +106,8 @@ The `type` filter accepts: `issue`, `pull_request`, `release`, `doc`, `wiki_doc` ## Authentication flow 1. On first tool call, the proxy discovers OAuth metadata at `${RAG_WORKER_URL}/.well-known/oauth-authorization-server`. -2. It performs Dynamic Client Registration (RFC 7591) if no client is cached. +2. It performs Dynamic Client Registration (RFC 7591) if no compatible client + is cached for the current localhost callback URIs. 3. It starts a one-shot localhost HTTP listener on a random port and opens the browser to the Worker's authorization endpoint. 4. After you approve, the Worker redirects to `http://127.0.0.1:/callback` with an authorization code. 5. The proxy exchanges the code for tokens (PKCE S256) and saves them. @@ -112,6 +118,9 @@ The browser callback never leaves your machine; the authorization code is delive ## Troubleshooting - **Browser does not open.** The proxy logs the authorization URL to stderr; copy it into a browser manually. +- **`redirect_uri is not associated with this application`.** Upgrade the + proxy. Current versions replace cached client registrations whose redirect + URI set does not cover the callback port selected for this authorization. - **`OAuth callback timed out after 5 minutes`.** Re-invoke any tool to restart the flow. - **`Failed to reach worker`.** Check that `RAG_WORKER_URL` is correct and reachable from your machine. - **Stale credentials.** Remove `~/.github-rag-mcp/oauth-tokens.json` (and optionally `oauth-client.json`) and retry. diff --git a/mcp-server/package.json b/mcp-server/package.json index 936e2c3..a34d634 100644 --- a/mcp-server/package.json +++ b/mcp-server/package.json @@ -14,7 +14,7 @@ ], "scripts": { "start": "node server/index.js", - "test": "node --check server/index.js", + "test": "node --check server/index.js && node --test test/oauth-client-registration.test.js", "pack:mcpb": "mcpb pack" }, "dependencies": { diff --git a/mcp-server/server/index.js b/mcp-server/server/index.js index 6afe41e..0cf692c 100644 --- a/mcp-server/server/index.js +++ b/mcp-server/server/index.js @@ -23,6 +23,7 @@ import { homedir } from "node:os"; import { join } from "node:path"; import { exec } from "node:child_process"; import { createRequire } from "node:module"; +import { supportsRedirectUris } from "./oauth-client-registration.js"; const require = createRequire(import.meta.url); const { version: PACKAGE_VERSION } = require("../package.json"); @@ -89,7 +90,7 @@ async function saveClientRegistration(reg) { async function ensureClientRegistration(metadata, redirectUris) { const existing = await loadClientRegistration(); - if (existing) return existing; + if (supportsRedirectUris(existing, redirectUris)) return existing; if (!metadata.registration_endpoint) { throw new Error("OAuth server does not support dynamic client registration"); diff --git a/mcp-server/server/oauth-client-registration.js b/mcp-server/server/oauth-client-registration.js new file mode 100644 index 0000000..ad743fc --- /dev/null +++ b/mcp-server/server/oauth-client-registration.js @@ -0,0 +1,21 @@ +/** + * Return true only when a cached Dynamic Client Registration can be reused for + * every redirect URI requested by the current OAuth callback listener. + */ +export function supportsRedirectUris(registration, redirectUris) { + if ( + registration === null || + typeof registration !== "object" || + typeof registration.client_id !== "string" || + registration.client_id.length === 0 || + !Array.isArray(registration.redirect_uris) + ) { + return false; + } + + const registeredUris = new Set( + registration.redirect_uris.filter((uri) => typeof uri === "string"), + ); + + return redirectUris.every((uri) => registeredUris.has(uri)); +} diff --git a/mcp-server/test/oauth-client-registration.test.js b/mcp-server/test/oauth-client-registration.test.js new file mode 100644 index 0000000..18cb487 --- /dev/null +++ b/mcp-server/test/oauth-client-registration.test.js @@ -0,0 +1,80 @@ +import assert from "node:assert/strict"; +import test from "node:test"; + +import { supportsRedirectUris } from "../server/oauth-client-registration.js"; + +const REQUIRED_URIS = [ + "http://127.0.0.1:43123/callback", + "http://localhost:43123/callback", +]; + +test("reuses a registration that covers every requested redirect URI", () => { + assert.equal( + supportsRedirectUris( + { + client_id: "client-id", + redirect_uris: [...REQUIRED_URIS].reverse(), + }, + REQUIRED_URIS, + ), + true, + ); +}); + +test("reuses a registration whose redirect URI set is a superset", () => { + assert.equal( + supportsRedirectUris( + { + client_id: "client-id", + redirect_uris: [ + ...REQUIRED_URIS, + "http://127.0.0.1:49999/callback", + ], + }, + REQUIRED_URIS, + ), + true, + ); +}); + +test("rejects a stale registration from a different callback port", () => { + assert.equal( + supportsRedirectUris( + { + client_id: "client-id", + redirect_uris: [ + "http://127.0.0.1:40000/callback", + "http://localhost:40000/callback", + ], + }, + REQUIRED_URIS, + ), + false, + ); +}); + +test("rejects a registration missing one requested redirect URI", () => { + assert.equal( + supportsRedirectUris( + { + client_id: "client-id", + redirect_uris: [REQUIRED_URIS[0]], + }, + REQUIRED_URIS, + ), + false, + ); +}); + +test("rejects malformed cached registrations", () => { + assert.equal(supportsRedirectUris(null, REQUIRED_URIS), false); + assert.equal(supportsRedirectUris({}, REQUIRED_URIS), false); + assert.equal( + supportsRedirectUris({ client_id: "", redirect_uris: REQUIRED_URIS }, REQUIRED_URIS), + false, + ); + assert.equal( + supportsRedirectUris({ client_id: "client-id", redirect_uris: "not-an-array" }, REQUIRED_URIS), + false, + ); +});