From baedc1f32a919e36f307770e15119e91fb8643e4 Mon Sep 17 00:00:00 2001 From: Minoru Mizutani Date: Tue, 29 Apr 2025 11:30:51 +0900 Subject: [PATCH] Fix tests, permissions --- deno.json | 14 ++++---- deno.lock | 1 + src/lib/coordination.ts | 2 +- src/lib/deno-http-server.ts | 18 +++++++++-- src/lib/mcp-auth-config.ts | 2 +- src/lib/utils.ts | 6 ++-- tests/deno-http-server_test.ts | 58 ++++++++++++++++++++++++---------- tests/mcp-auth-config_test.ts | 9 ++---- 8 files changed, 74 insertions(+), 36 deletions(-) diff --git a/deno.json b/deno.json index 98b2a68..4ec27cd 100644 --- a/deno.json +++ b/deno.json @@ -14,13 +14,13 @@ ] }, "tasks": { - "proxy:start": "deno run --allow-net --allow-env --allow-read --allow-run --allow-sys --allow-ffi src/proxy.ts", - "proxy:watch": "deno run --watch --allow-net --allow-env --allow-read --allow-run --allow-sys --allow-ffi src/proxy.ts", - "client:start": "deno run --allow-net --allow-env --allow-read --allow-run --allow-sys --allow-ffi src/client.ts", - "client:watch": "deno run --watch --allow-net --allow-env --allow-read --allow-run --allow-sys --allow-ffi src/client.ts", - "test": "deno test --allow-net --allow-env --allow-read --allow-run --allow-sys tests/", - "test:watch": "deno test --watch --allow-net --allow-env --allow-read --allow-run --allow-sys tests/", - "test:coverage": "deno test --coverage=coverage --allow-net --allow-env --allow-read --allow-run --allow-sys tests/ && deno coverage coverage" + "proxy:start": "deno run --allow-env --allow-read --allow-sys --allow-run=open --allow-write=\"$HOME/.mcp-auth/mcp-remote-deno-1.0.0\" --allow-net=0.0.0.0,localhost src/proxy.ts", + "proxy:watch": "deno run --watch --allow-env --allow-read --allow-sys --allow-run=open --allow-write=\"$HOME/.mcp-auth/mcp-remote-deno-1.0.0\" --allow-net=0.0.0.0,localhost src/proxy.ts", + "client:start": "deno run --allow-env --allow-read --allow-sys --allow-run=open --allow-write=\"$HOME/.mcp-auth/mcp-remote-deno-1.0.0\" --allow-net=0.0.0.0,localhost src/client.ts", + "client:watch": "deno run --watch --allow-env --allow-read --allow-sys --allow-run=open --allow-write=\"$HOME/.mcp-auth/mcp-remote-deno-1.0.0\" --allow-net=0.0.0.0,localhost src/client.ts", + "test": "deno test --allow-net --allow-env --allow-read tests/", + "test:watch": "deno test --watch --allow-net --allow-env --allow-read tests/", + "test:coverage": "deno test --coverage=coverage --allow-net --allow-env --allow-read tests/ && deno coverage coverage" }, "imports": { "std/": "https://deno.land/std@0.224.0/", diff --git a/deno.lock b/deno.lock index 68ba07c..c85d114 100644 --- a/deno.lock +++ b/deno.lock @@ -1394,6 +1394,7 @@ "https://deno.land/std@0.224.0/data_structures/comparators.ts": "17dfa68bf1550edadbfdd453a06f9819290bcb534c9945b5cec4b30242cff475", "https://deno.land/std@0.224.0/data_structures/red_black_tree.ts": "2222be0c46842fc932e2c8589a66dced9e6eae180914807c5c55d1aa4c8c1b9b", "https://deno.land/std@0.224.0/fmt/colors.ts": "508563c0659dd7198ba4bbf87e97f654af3c34eb56ba790260f252ad8012e1c5", + "https://deno.land/std@0.224.0/http/server.ts": "f9313804bf6467a1704f45f76cb6cd0a3396a3b31c316035e6a4c2035d1ea514", "https://deno.land/std@0.224.0/internal/diff.ts": "6234a4b493ebe65dc67a18a0eb97ef683626a1166a1906232ce186ae9f65f4e6", "https://deno.land/std@0.224.0/internal/format.ts": "0a98ee226fd3d43450245b1844b47003419d34d210fa989900861c79820d21c2", "https://deno.land/std@0.224.0/internal/mod.ts": "534125398c8e7426183e12dc255bb635d94e06d0f93c60a297723abe69d3b22e", diff --git a/src/lib/coordination.ts b/src/lib/coordination.ts index 0d4dcb9..d81d3ba 100644 --- a/src/lib/coordination.ts +++ b/src/lib/coordination.ts @@ -159,7 +159,7 @@ export async function coordinateAuth( log("Authentication completed by another instance"); // Setup a dummy server - the client will use tokens directly from disk - const dummyServer = express().listen(0); // Listen on any available port + const dummyServer = express().listen(0, "localhost"); // Listen on any available port on localhost only // This shouldn't actually be called in normal operation, but provide it for API compatibility const dummyWaitForAuthCode = () => { diff --git a/src/lib/deno-http-server.ts b/src/lib/deno-http-server.ts index 2804e14..c919b57 100644 --- a/src/lib/deno-http-server.ts +++ b/src/lib/deno-http-server.ts @@ -48,12 +48,25 @@ export class DenoHttpServer { /** * Start the server listening on the specified port * @param port The port to listen on + * @param hostname Optional hostname to bind to * @param callback Optional callback when server is ready */ - listen(port: number, callback?: () => void): Server { + listen(port: number, hostname?: string | (() => void), callback?: () => void): Server { + // Handle optional hostname parameter + let hostnameStr: string | undefined; + let callbackFn = callback; + + if (typeof hostname === 'function') { + callbackFn = hostname; + hostnameStr = undefined; + } else { + hostnameStr = hostname; + } + this.server = Deno.serve({ port, - onListen: callback ? () => callback() : undefined, + hostname: hostnameStr, + onListen: callbackFn ? () => callbackFn() : undefined, handler: async (request: Request) => { const url = new URL(request.url); const path = url.pathname; @@ -73,6 +86,7 @@ export class DenoHttpServer { // This is needed to maintain API compatibility return { close: () => this.close(), + address: () => ({ port }), } as unknown as Server; } diff --git a/src/lib/mcp-auth-config.ts b/src/lib/mcp-auth-config.ts index c1713fe..aa0c85b 100644 --- a/src/lib/mcp-auth-config.ts +++ b/src/lib/mcp-auth-config.ts @@ -98,7 +98,7 @@ export function getConfigDir(): string { const baseConfigDir = Deno.env.get("MCP_REMOTE_CONFIG_DIR") || path.join(os.homedir(), ".mcp-auth"); // Add a version subdirectory so we don't need to worry about backwards/forwards compatibility yet - return path.join(baseConfigDir, `mcp-remote-${MCP_REMOTE_VERSION}`); + return path.join(baseConfigDir, `mcp-remote-deno-${MCP_REMOTE_VERSION}`); } /** diff --git a/src/lib/utils.ts b/src/lib/utils.ts index d960503..346448f 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -247,7 +247,7 @@ export function setupOAuthCallbackServerWithLongPoll( options.events.emit("auth-code-received", code); }); - const server = app.listen(options.port, () => { + const server = app.listen(options.port, "localhost", () => { log(`OAuth callback server running at http://127.0.0.1:${options.port}`); }); @@ -281,7 +281,7 @@ export function findAvailablePort( server.on("error", (err: NodeJS.ErrnoException) => { if (err.code === "EADDRINUSE") { // If preferred port is in use, get a random port - server.listen(0); + server.listen({ port: 0, hostname: "localhost" }); } else { reject(err); } @@ -295,7 +295,7 @@ export function findAvailablePort( }); // Try preferred port first, or get a random port - server.listen(preferredPort || 0); + server.listen({ port: preferredPort || 0, hostname: "localhost" }); }); } diff --git a/tests/deno-http-server_test.ts b/tests/deno-http-server_test.ts index 5d08a77..becd256 100644 --- a/tests/deno-http-server_test.ts +++ b/tests/deno-http-server_test.ts @@ -1,13 +1,19 @@ -import { assertEquals } from "std/assert/mod.ts"; import { describe, it, beforeEach, afterEach } from "std/testing/bdd.ts"; import { DenoHttpServer, ResponseBuilder } from "../src/lib/deno-http-server.ts"; +import { assertEquals } from "std/assert/mod.ts"; describe("DenoHttpServer", () => { let server: DenoHttpServer; let serverInstance: ReturnType; + // Define testPort at the describe level so it's available to all tests + const testPort = 9876; beforeEach(() => { server = new DenoHttpServer(); + // Start the server on a random available port + serverInstance = server.listen(testPort, "localhost", () => { + // Server started + }); }); afterEach(async () => { @@ -29,12 +35,6 @@ describe("DenoHttpServer", () => { res.status(200).send("OK"); }); - // Start the server on a random available port - const testPort = 9876; - serverInstance = server.listen(testPort, () => { - // Server started - }); - // Send a request to the server const response = await fetch(`http://localhost:${testPort}/test?param=value`); const text = await response.text(); @@ -47,17 +47,43 @@ describe("DenoHttpServer", () => { assertEquals(reqQuery.param, "value"); }); - it("returns 404 for non-existent routes", async () => { - // Start the server on a random available port - const testPort = 9877; - serverInstance = server.listen(testPort); + it("should handle 404 for non-existent routes", async () => { + const server = new DenoHttpServer(); + const localTestPort = 9877; + let serverInstance!: ReturnType; - // Send a request to a non-existent route - const response = await fetch(`http://localhost:${testPort}/non-existent`); + try { + // Start the server on a random available port + serverInstance = server.listen(localTestPort, "localhost"); - // Verify the response - assertEquals(response.status, 404); - await response.body?.cancel(); // Consume the body to prevent leaks + // Send a request to a non-existent route + const response = await fetch(`http://localhost:${localTestPort}/non-existent`); + + // Verify the response + assertEquals(response.status, 404); + await response.body?.cancel(); // Consume the body to prevent leaks + } finally { + if (serverInstance) { + server.close(); + } + } + }); + + it("should listen without callback", () => { + const server = new DenoHttpServer(); + const localTestPort = 9878; + let serverInstance!: ReturnType; + + try { + serverInstance = server.listen(localTestPort, "localhost"); + // Use the port property directly - we know our implementation returns an object with port + const port = (serverInstance as unknown as { port: number }).port; + assertEquals(port, localTestPort); + } finally { + if (serverInstance) { + server.close(); + } + } }); }); diff --git a/tests/mcp-auth-config_test.ts b/tests/mcp-auth-config_test.ts index 128d3a0..1ca81e6 100644 --- a/tests/mcp-auth-config_test.ts +++ b/tests/mcp-auth-config_test.ts @@ -1,11 +1,8 @@ import { assertEquals, - assertMatch, assertStringIncludes, } from "std/assert/mod.ts"; -import { describe, it, beforeEach, afterEach } from "std/testing/bdd.ts"; -import { assertSpyCalls, spy, stub } from "std/testing/mock.ts"; -import { FakeTime } from "std/testing/time.ts"; +import { describe, it, afterEach } from "std/testing/bdd.ts"; import { getConfigDir, getConfigFilePath, @@ -35,7 +32,7 @@ describe("mcp-auth-config", () => { const configDir = getConfigDir(); assertStringIncludes(configDir, customDir); - assertStringIncludes(configDir, `mcp-remote-${MCP_REMOTE_VERSION}`); + assertStringIncludes(configDir, `mcp-remote-deno-${MCP_REMOTE_VERSION}`); }); it("falls back to ~/.mcp-auth if environment variable is not set", () => { @@ -48,7 +45,7 @@ describe("mcp-auth-config", () => { const configDir = getConfigDir(); assertStringIncludes(configDir, expectedBase); - assertStringIncludes(configDir, `mcp-remote-${MCP_REMOTE_VERSION}`); + assertStringIncludes(configDir, `mcp-remote-deno-${MCP_REMOTE_VERSION}`); }); });