diff --git a/docs/installation.ja.md b/docs/installation.ja.md index c3aa9f4..334d94e 100644 --- a/docs/installation.ja.md +++ b/docs/installation.ja.md @@ -46,6 +46,8 @@ A browser window should have opened automatically. Sign in on GitHub, then retry > **ブラウザ自動オープンが失敗した場合:** 応答と stderr ログに URL がそのまま残るので、手動でコピーしてブラウザに貼り付けてください。Windows では `start`、macOS では `open`、Linux では `xdg-open` を使用します。 +> **v0.11.2 Windows hotfix:** v0.11.1 の Windows 版はブラウザ自動オープン時に authorize URL を cmd.exe にクォート無しで渡していたため、URL に含まれる `&` が command separator として解釈され、`state` パラメータが欠落して `/oauth/authorize` が 400 を返していました。v0.11.2 では URL を `"..."` で囲んで shell 経由で起動するようにしたので、`&` がリテラル扱いされ state が正しくブラウザに届きます。v0.11.1 で自動オープンが機能しなかった Windows ユーザーは v0.11.2 に更新してください(手動 URL コピーは引き続きフォールバックとして動作します)。 + > **旧バージョンからの移行:** v0.11.0 以前(localhost callback flow / device flow どちらも)の `~/.github-webhook-mcp/oauth-tokens.json` は flow marker が一致しないため自動で無視され、初回ツール呼び出し時に新しい web OAuth flow で再認証が走ります。特別な手作業は不要です。 ### Claude Desktop — デスクトップ拡張 (.mcpb) diff --git a/docs/installation.md b/docs/installation.md index 6329636..ec33332 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -46,6 +46,8 @@ A browser window should have opened automatically. Sign in on GitHub, then retry > **ブラウザ自動オープンが失敗した場合:** 応答と stderr ログに URL がそのまま残るので、手動でコピーしてブラウザに貼り付けてください。Windows では `start`、macOS では `open`、Linux では `xdg-open` を使用します。 +> **v0.11.2 Windows hotfix:** v0.11.1 の Windows 版はブラウザ自動オープン時に authorize URL を cmd.exe にクォート無しで渡していたため、URL に含まれる `&` が command separator として解釈され、`state` パラメータが欠落して `/oauth/authorize` が 400 を返していました。v0.11.2 では URL を `"..."` で囲んで shell 経由で起動するようにしたので、`&` がリテラル扱いされ state が正しくブラウザに届きます。v0.11.1 で自動オープンが機能しなかった Windows ユーザーは v0.11.2 に更新してください(手動 URL コピーは引き続きフォールバックとして動作します)。 + > **旧バージョンからの移行:** v0.11.0 以前(localhost callback flow / device flow どちらも)の `~/.github-webhook-mcp/oauth-tokens.json` は flow marker が一致しないため自動で無視され、初回ツール呼び出し時に新しい web OAuth flow で再認証が走ります。特別な手作業は不要です。 ### Claude Desktop — デスクトップ拡張 (.mcpb) diff --git a/local-mcp/src/index.ts b/local-mcp/src/index.ts index 5ca51f7..7d36683 100644 --- a/local-mcp/src/index.ts +++ b/local-mcp/src/index.ts @@ -188,13 +188,26 @@ function openBrowser(url: string): void { const plat = osPlatform(); let command: string; let args: string[]; - const options = { detached: true, stdio: "ignore" as const }; + const options: { detached: boolean; stdio: "ignore"; shell?: boolean } = { + detached: true, + stdio: "ignore" as const, + }; if (plat === "win32") { - // `start` is a cmd.exe builtin. The empty "" argument is the window - // title placeholder expected when the URL is quoted. - command = "cmd.exe"; - args = ["/c", "start", "", url]; + // `start` is a cmd.exe builtin, not a standalone executable, so the + // command must run through a shell. The empty "" is the window-title + // placeholder expected by `start` when the following token is quoted. + // + // URL-quoting is mandatory on Windows: cmd.exe treats `&` as a command + // separator unless the argument is wrapped in double quotes, so an + // unquoted OAuth authorize URL like + // https://worker/oauth/authorize?client_id=X&state=Y + // would truncate at `&state=` and drop the `state` parameter (v0.11.2 + // fix for #211). `shell: true` hands the full command string to + // cmd.exe so the outer quotes survive argv reassembly intact. + command = `start "" "${url}"`; + args = []; + options.shell = true; } else if (plat === "darwin") { command = "open"; args = [url]; diff --git a/mcp-server/manifest.json b/mcp-server/manifest.json index 6cd8ee6..705523e 100644 --- a/mcp-server/manifest.json +++ b/mcp-server/manifest.json @@ -2,7 +2,7 @@ "manifest_version": "0.3", "name": "github-webhook-mcp", "display_name": "GitHub Webhook MCP", - "version": "0.11.1", + "version": "0.11.2", "description": "Real-time GitHub webhook notifications via Cloudflare Worker + Durable Object.", "long_description": "GitHub Webhook MCP bridges GitHub webhook events to Claude via a Cloudflare Worker backend. Events are stored in a Durable Object with SQLite, queried through MCP tools, and optionally pushed in real-time via SSE channel notifications. No local webhook receiver needed — just point your GitHub webhook at the Worker URL.", "author": { diff --git a/mcp-server/package.json b/mcp-server/package.json index d7f0872..89f7c5c 100644 --- a/mcp-server/package.json +++ b/mcp-server/package.json @@ -1,6 +1,6 @@ { "name": "github-webhook-mcp", - "version": "0.11.1", + "version": "0.11.2", "description": "MCP server bridging GitHub webhooks via Cloudflare Worker", "type": "module", "bin": { diff --git a/mcp-server/server.json b/mcp-server/server.json index 90efea6..224f183 100644 --- a/mcp-server/server.json +++ b/mcp-server/server.json @@ -2,7 +2,7 @@ "$schema": "https://static.modelcontextprotocol.io/schemas/2025-12-11/server.schema.json", "name": "io.github.Liplus-Project/github-webhook-mcp", "description": "MCP server bridging GitHub webhooks via Cloudflare Worker for real-time event streaming", - "version": "0.11.1", + "version": "0.11.2", "repository": { "url": "https://github.com/Liplus-Project/github-webhook-mcp", "source": "github", @@ -13,7 +13,7 @@ { "registryType": "npm", "identifier": "github-webhook-mcp", - "version": "0.11.1", + "version": "0.11.2", "runtimeHint": "npx", "transport": { "type": "stdio" diff --git a/mcp-server/server/index.js b/mcp-server/server/index.js index bdc573c..c413851 100644 --- a/mcp-server/server/index.js +++ b/mcp-server/server/index.js @@ -171,11 +171,20 @@ function openBrowser(url) { let options = { detached: true, stdio: "ignore" }; if (plat === "win32") { - // `start` is a cmd.exe builtin, not a standalone executable. - // The empty "" argument is the window title placeholder expected by - // `start` when the URL is quoted. - command = "cmd.exe"; - args = ["/c", "start", "", url]; + // `start` is a cmd.exe builtin, not a standalone executable, so the + // command must run through a shell. The empty "" is the window-title + // placeholder expected by `start` when the following token is quoted. + // + // URL-quoting is mandatory on Windows: cmd.exe treats `&` as a command + // separator unless the argument is wrapped in double quotes, so an + // unquoted OAuth authorize URL like + // https://worker/oauth/authorize?client_id=X&state=Y + // would truncate at `&state=` and drop the `state` parameter (v0.11.2 + // fix for #211). `shell: true` hands the full command string to + // cmd.exe so the outer quotes survive argv reassembly intact. + command = `start "" "${url}"`; + args = []; + options.shell = true; } else if (plat === "darwin") { command = "open"; args = [url]; diff --git a/mcp-server/test/open-browser.test.mjs b/mcp-server/test/open-browser.test.mjs new file mode 100644 index 0000000..a7ee5fd --- /dev/null +++ b/mcp-server/test/open-browser.test.mjs @@ -0,0 +1,191 @@ +/** + * Argument-construction tests for openBrowser() platform branches. + * + * Root cause of #211: on Windows the previous implementation passed the + * OAuth authorize URL to `cmd.exe /c start "" ` unquoted. cmd.exe + * interprets `&` as a command separator, truncating URLs like + * https://worker/oauth/authorize?client_id=X&state=Y + * at `&state=...`, dropping the state parameter and breaking the Worker's + * `/oauth/authorize` handler (`invalid_request: state parameter is required`). + * + * The v0.11.2 fix wraps the URL in double quotes and runs through a shell + * so that cmd.exe treats the quoted `&` as literal. macOS (`open`) and + * Linux (`xdg-open`) branches are unaffected because both launchers accept + * `&` as a normal argv character. + * + * We re-implement openBrowser() inline for the same reason migration.test.mjs + * and web-auth-required.test.mjs do: server/index.js cannot be imported + * without starting an MCP server (top-level await on server.connect). + * The assertions below lock down the argv shape for each platform and + * verify the `&`-containing URL survives intact to the spawn boundary. + */ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +/** + * Platform-parameterised mirror of openBrowser() from server/index.js. + * Returns the exact { command, args, options } tuple that would be passed + * to child_process.spawn, without actually spawning anything. + */ +function buildSpawnArgsForPlatform(plat, url) { + let command; + let args; + const options = { detached: true, stdio: "ignore" }; + + if (plat === "win32") { + command = `start "" "${url}"`; + args = []; + options.shell = true; + } else if (plat === "darwin") { + command = "open"; + args = [url]; + } else { + command = "xdg-open"; + args = [url]; + } + + return { command, args, options }; +} + +const AUTHORIZE_URL_WITH_AMPERSAND = + "https://github-webhook.smgjp.com/oauth/authorize?client_id=abc123&state=xyz789"; + +test("windows branch wraps URL in double quotes and runs through shell", () => { + const { command, args, options } = buildSpawnArgsForPlatform( + "win32", + AUTHORIZE_URL_WITH_AMPERSAND, + ); + + // The command string must quote the URL so cmd.exe treats `&` as literal. + assert.equal( + command, + `start "" "${AUTHORIZE_URL_WITH_AMPERSAND}"`, + "windows command must be `start \"\" \"\"` with the URL in quotes", + ); + assert.ok( + command.includes(`"${AUTHORIZE_URL_WITH_AMPERSAND}"`), + "authorize URL must appear inside the outer double-quote pair", + ); + assert.deepEqual(args, [], "no separate argv when using shell: true"); + assert.equal(options.shell, true, "shell: true is required for the cmd.exe builtin `start`"); + assert.equal(options.detached, true); + assert.equal(options.stdio, "ignore"); +}); + +test("windows branch preserves the state parameter across & (regression for #211)", () => { + const { command } = buildSpawnArgsForPlatform( + "win32", + AUTHORIZE_URL_WITH_AMPERSAND, + ); + + // The whole URL, including everything after `&`, must appear verbatim in + // the command string — this is the condition that lets cmd.exe's quote + // handling keep `&state=xyz789` attached to the authorize URL instead of + // treating it as a second command. + assert.ok( + command.includes("&state=xyz789"), + "state parameter must survive on the Windows command line", + ); + assert.ok( + command.includes("client_id=abc123"), + "client_id must also survive on the Windows command line", + ); +}); + +test("darwin branch uses `open` with the URL as a plain argv", () => { + const { command, args, options } = buildSpawnArgsForPlatform( + "darwin", + AUTHORIZE_URL_WITH_AMPERSAND, + ); + + assert.equal(command, "open"); + assert.deepEqual(args, [AUTHORIZE_URL_WITH_AMPERSAND]); + assert.notEqual(options.shell, true, "no shell needed on macOS"); +}); + +test("linux branch uses `xdg-open` with the URL as a plain argv", () => { + const { command, args, options } = buildSpawnArgsForPlatform( + "linux", + AUTHORIZE_URL_WITH_AMPERSAND, + ); + + assert.equal(command, "xdg-open"); + assert.deepEqual(args, [AUTHORIZE_URL_WITH_AMPERSAND]); + assert.notEqual(options.shell, true, "no shell needed on Linux"); +}); + +test("non-win32 platforms pass URL as single argv element (no cmd interpretation)", () => { + for (const plat of ["darwin", "linux", "freebsd", "openbsd"]) { + const { args } = buildSpawnArgsForPlatform(plat, AUTHORIZE_URL_WITH_AMPERSAND); + assert.equal(args.length, 1, `${plat}: URL must be a single argv element`); + assert.equal( + args[0], + AUTHORIZE_URL_WITH_AMPERSAND, + `${plat}: URL must survive verbatim (argv bypasses shell interpretation)`, + ); + } +}); + +/** + * Integration-style test: swap in a fake spawn and verify openBrowser's + * caller-visible side effects. The inline openBrowser mirrors server/index.js + * exactly; the fake spawn captures the call shape and exposes a `.on` hook so + * the error-handler wiring is exercised too. + */ +test("openBrowser dispatches to spawn with platform-correct arguments", () => { + const calls = []; + + function fakeSpawn(command, args, options) { + calls.push({ command, args, options }); + return { + on: () => {}, + unref: () => {}, + }; + } + + function openBrowser(url, plat) { + if (!url || typeof url !== "string") return; + const { command, args, options } = buildSpawnArgsForPlatform(plat, url); + const child = fakeSpawn(command, args, options); + child.on("error", () => {}); + if (typeof child.unref === "function") child.unref(); + } + + openBrowser(AUTHORIZE_URL_WITH_AMPERSAND, "win32"); + openBrowser(AUTHORIZE_URL_WITH_AMPERSAND, "darwin"); + openBrowser(AUTHORIZE_URL_WITH_AMPERSAND, "linux"); + + assert.equal(calls.length, 3); + + // win32 + assert.equal(calls[0].command, `start "" "${AUTHORIZE_URL_WITH_AMPERSAND}"`); + assert.equal(calls[0].options.shell, true); + + // darwin + assert.equal(calls[1].command, "open"); + assert.deepEqual(calls[1].args, [AUTHORIZE_URL_WITH_AMPERSAND]); + + // linux + assert.equal(calls[2].command, "xdg-open"); + assert.deepEqual(calls[2].args, [AUTHORIZE_URL_WITH_AMPERSAND]); +}); + +test("openBrowser is a no-op on empty/invalid input", () => { + const calls = []; + function fakeSpawn() { + calls.push("called"); + return { on: () => {}, unref: () => {} }; + } + function openBrowser(url, plat) { + if (!url || typeof url !== "string") return; + const { command, args, options } = buildSpawnArgsForPlatform(plat, url); + fakeSpawn(command, args, options); + } + + openBrowser("", "win32"); + openBrowser(null, "win32"); + openBrowser(undefined, "win32"); + openBrowser(42, "win32"); + + assert.equal(calls.length, 0, "invalid URLs must short-circuit before spawn"); +});