From 080030c764090c13348d692af4e514f1b93d8487 Mon Sep 17 00:00:00 2001 From: kst Date: Fri, 21 Aug 2026 17:19:13 +0900 Subject: [PATCH] fix: harden server reliability and OAuth security --- .env.example | 3 +- README.md | 18 +- deploy/nginx.remote-dev-mcp.conf | 6 +- deploy/remote-dev-mcp.env.example | 4 +- src/config.ts | 42 ++++- src/file-service.ts | 95 ++++++++-- src/http-server.ts | 59 +++++-- src/oauth.ts | 234 +++++++++++++++++++++---- src/process-manager.ts | 157 +++++++++++++++-- src/server.ts | 6 +- test/auth-security.integration.test.ts | 167 ++++++++++++++++++ test/config.test.ts | 57 ++++++ test/file-service.test.ts | 34 ++++ test/mcp.integration.test.ts | 19 ++ test/oauth.integration.test.ts | 89 +++++++++- test/process-manager.test.ts | 56 ++++++ 16 files changed, 946 insertions(+), 100 deletions(-) create mode 100644 test/auth-security.integration.test.ts diff --git a/.env.example b/.env.example index c894e89..4122bde 100644 --- a/.env.example +++ b/.env.example @@ -3,8 +3,9 @@ MCP_HOST=0.0.0.0 MCP_PORT=3000 MCP_ENDPOINT=/mcp MCP_PUBLIC_URL=https://mcp.example.com +MCP_TRUST_PROXY_HOPS=0 -# Authentication. A token is required unless MCP_ALLOW_NO_AUTH=true. +# Static bearer authentication. OAuth can instead use MCP_OAUTH_APPROVAL_KEY. MCP_AUTH_TOKEN=replace-with-a-long-random-token MCP_ALLOW_NO_AUTH=false diff --git a/README.md b/README.md index 07d23d7..62492c4 100644 --- a/README.md +++ b/README.md @@ -97,6 +97,7 @@ ChatGPT 연결용으로 내장 OAuth 2.1 Authorization Server를 활성화할 ```dotenv MCP_OAUTH_ENABLED=true +MCP_OAUTH_APPROVAL_KEY= MCP_PUBLIC_URL=https://mcp.example.com MCP_OAUTH_ISSUER=https://mcp.example.com MCP_OAUTH_RESOURCE=https://mcp.example.com/mcp @@ -110,9 +111,9 @@ MCP_OAUTH_STATE_FILE=/var/lib/remote-dev-mcp/oauth-state.json - Dynamic Client Registration(DCR) - Authorization Code + PKCE(S256) - `resource` audience 검증 -- 액세스 토큰, 회전형 refresh token, token revocation +- 액세스 토큰, 재사용 탐지형 refresh token 회전, grant 단위 token revocation -OAuth가 활성화되면 `mcp:tools` 단일 범위를 사용합니다. ChatGPT에서 연결을 승인할 때 표시되는 화면에는 `MCP_AUTH_TOKEN` 값을 입력합니다. 이 값은 승인용 비밀번호인 동시에 MCP를 직접 호출할 수 있는 정적 Bearer 토큰이므로 root 자격증명처럼 취급해야 합니다. 등록 클라이언트와 토큰 해시는 `MCP_OAUTH_STATE_FILE`에 권한 `600`으로 저장됩니다. +OAuth가 활성화되면 `mcp:tools` 단일 범위를 사용합니다. ChatGPT에서 연결을 승인할 때 표시되는 화면에는 `MCP_OAUTH_APPROVAL_KEY` 값을 입력합니다. OAuth만 사용할 때는 `MCP_AUTH_TOKEN`을 비워 두어 영구 정적 Bearer 우회 경로를 만들지 않는 구성을 권장합니다. 하위 호환성을 위해 승인키가 없으면 `MCP_AUTH_TOKEN`을 승인키로 사용하지만, 두 값을 분리하는 편이 안전합니다. 두 값 모두 root 자격증명처럼 취급해야 합니다. 등록 클라이언트, 클라이언트 비밀정보와 토큰 해시는 `MCP_OAUTH_STATE_FILE`에 권한 `600`으로 저장됩니다. OAuth 관련 HTTP 경로는 다음과 같습니다. @@ -166,14 +167,18 @@ sudo systemctl status remote-dev-mcp 공개 인터넷에서 사용할 때는 HTTPS가 필요합니다. [Nginx 예제](deploy/nginx.remote-dev-mcp.conf)의 도메인과 인증서 경로를 바꾸고 유효한 인증서를 준비한 뒤 활성화합니다. Node 서버는 `127.0.0.1`에 바인딩하고 80/443만 외부에 공개하는 구성을 권장합니다. 긴 도구 호출이 프록시에서 먼저 종료되지 않도록 충분한 read timeout을 사용합니다. +제공된 Nginx 예제처럼 프록시가 정확히 한 홉 앞에 있을 때만 `MCP_TRUST_PROXY_HOPS=1`을 설정합니다. Node 포트를 직접 공개하거나 프록시 홉 수가 다르면 이 값을 그대로 사용하지 마십시오. 잘못 신뢰한 `X-Forwarded-For` 값은 OAuth 속도 제한을 우회하는 데 악용될 수 있습니다. + 운영 환경 파일에서는 최소한 다음 값을 실제 도메인에 맞춰야 합니다. ```dotenv MCP_HOST=127.0.0.1 MCP_PUBLIC_URL=https://mcp.example.com MCP_ALLOWED_HOSTS=mcp.example.com,127.0.0.1,localhost -MCP_AUTH_TOKEN= +MCP_TRUST_PROXY_HOPS=1 +MCP_AUTH_TOKEN= MCP_OAUTH_ENABLED=true +MCP_OAUTH_APPROVAL_KEY= MCP_OAUTH_ISSUER=https://mcp.example.com MCP_OAUTH_RESOURCE=https://mcp.example.com/mcp ``` @@ -186,7 +191,7 @@ MCP_OAUTH_RESOURCE=https://mcp.example.com/mcp 2. [ChatGPT Plugins](https://chatgpt.com/plugins)에서 추가 버튼을 누르고 MCP URL을 입력합니다. 3. OAuth 고급 설정이 표시되면 등록 방식을 **Dynamic Client Registration(DCR)**으로 선택합니다. 4. 기본 범위는 `mcp:tools`, token endpoint 인증 방식은 `none`을 사용합니다. DCR에서는 Client ID와 Client Secret을 직접 입력하지 않습니다. -5. 연결 승인 화면에서 `MCP_AUTH_TOKEN`을 입력하고 ChatGPT로 돌아갑니다. +5. 연결 승인 화면에서 `MCP_OAUTH_APPROVAL_KEY`를 입력하고 ChatGPT로 돌아갑니다. 이 서버는 DCR을 제공하며 CIMD와 OIDC는 제공하지 않습니다. ChatGPT 설정 화면에 CIMD 또는 OIDC를 사용할 수 없다는 안내가 나타나는 것은 오류가 아닙니다. 개발자 모드 제공 여부는 계정이나 워크스페이스 정책에 따라 달라질 수 있습니다. @@ -281,9 +286,11 @@ npx vitest run test/all-tools.integration.test.ts | `MCP_ENDPOINT` | `/mcp` | Streamable HTTP MCP 경로 | | `MCP_PUBLIC_URL` | 없음 | `/mcp`를 제외한 외부 HTTPS 기준 URL | | `MCP_ALLOWED_HOSTS` | 없음 | 허용할 Host 헤더의 호스트명 목록(쉼표 구분) | -| `MCP_AUTH_TOKEN` | 없음 | bearer 토큰 | +| `MCP_TRUST_PROXY_HOPS` | `0` | 신뢰할 역방향 프록시 홉 수. 직접 노출 시 `0` 유지 | +| `MCP_AUTH_TOKEN` | 없음 | 선택적 정적 bearer 토큰 | | `MCP_ALLOW_NO_AUTH` | `false` | 인증 없이 시작 허용 | | `MCP_OAUTH_ENABLED` | `false` | ChatGPT용 내장 OAuth 2.1/DCR 활성화 | +| `MCP_OAUTH_APPROVAL_KEY` | `MCP_AUTH_TOKEN` | OAuth 연결 승인 화면 전용 키 | | `MCP_OAUTH_ISSUER` | `MCP_PUBLIC_URL` | OAuth issuer URL | | `MCP_OAUTH_RESOURCE` | `` | MCP resource audience | | `MCP_OAUTH_STATE_FILE` | 작업 디렉터리 내부 | 등록 클라이언트와 토큰 해시 저장 파일 | @@ -351,4 +358,3 @@ The user assumes full responsibility for all consequences arising from the use o - diff --git a/deploy/nginx.remote-dev-mcp.conf b/deploy/nginx.remote-dev-mcp.conf index 5f83930..ef61590 100644 --- a/deploy/nginx.remote-dev-mcp.conf +++ b/deploy/nginx.remote-dev-mcp.conf @@ -27,7 +27,7 @@ server { proxy_pass http://127.0.0.1:3000; proxy_http_version 1.1; proxy_set_header Host $host; - proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + proxy_set_header X-Forwarded-For $remote_addr; proxy_set_header X-Forwarded-Proto $scheme; proxy_buffering off; proxy_cache off; @@ -39,7 +39,7 @@ server { proxy_pass http://127.0.0.1:3000; proxy_http_version 1.1; proxy_set_header Host $host; - proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + proxy_set_header X-Forwarded-For $remote_addr; proxy_set_header X-Forwarded-Proto $scheme; proxy_cache off; } @@ -48,7 +48,7 @@ server { proxy_pass http://127.0.0.1:3000; proxy_http_version 1.1; proxy_set_header Host $host; - proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + proxy_set_header X-Forwarded-For $remote_addr; proxy_set_header X-Forwarded-Proto $scheme; proxy_cache off; } diff --git a/deploy/remote-dev-mcp.env.example b/deploy/remote-dev-mcp.env.example index 25242a3..6665d0e 100644 --- a/deploy/remote-dev-mcp.env.example +++ b/deploy/remote-dev-mcp.env.example @@ -3,9 +3,11 @@ MCP_PORT=3000 MCP_ENDPOINT=/mcp MCP_PUBLIC_URL=https://mcp.example.com MCP_ALLOWED_HOSTS=mcp.example.com,127.0.0.1,localhost -MCP_AUTH_TOKEN=replace-with-a-long-random-token +MCP_TRUST_PROXY_HOPS=1 +MCP_AUTH_TOKEN= MCP_ALLOW_NO_AUTH=false MCP_OAUTH_ENABLED=true +MCP_OAUTH_APPROVAL_KEY=replace-with-a-separate-long-random-key MCP_OAUTH_ISSUER=https://mcp.example.com MCP_OAUTH_RESOURCE=https://mcp.example.com/mcp MCP_OAUTH_STATE_FILE=/var/lib/remote-dev-mcp/oauth-state.json diff --git a/src/config.ts b/src/config.ts index a984a10..f3676be 100644 --- a/src/config.ts +++ b/src/config.ts @@ -6,9 +6,11 @@ export interface AppConfig { endpoint: string; publicUrl: string | undefined; allowedHosts: string[] | undefined; + trustProxyHops: number; authToken: string | undefined; allowNoAuth: boolean; oauthEnabled: boolean; + oauthApprovalKey: string | undefined; oauthIssuerUrl: string | undefined; oauthResourceUrl: string | undefined; oauthStateFile: string; @@ -44,13 +46,18 @@ function parseInteger( fallback: number, name: string, minimum: number, + maximum = Number.MAX_SAFE_INTEGER, ): number { if (value === undefined || value === "") { return fallback; } - const parsed = Number.parseInt(value, 10); - if (!Number.isSafeInteger(parsed) || parsed < minimum) { - throw new Error(`${name} must be an integer greater than or equal to ${minimum}`); + const normalized = value.trim(); + const parsed = /^[+-]?\d+$/.test(normalized) ? Number(normalized) : Number.NaN; + if (!Number.isSafeInteger(parsed) || parsed < minimum || parsed > maximum) { + const range = maximum === Number.MAX_SAFE_INTEGER + ? `greater than or equal to ${minimum}` + : `between ${minimum} and ${maximum}`; + throw new Error(`${name} must be an integer ${range}`); } return parsed; } @@ -73,10 +80,16 @@ function normalizeOAuthUrl(value: string | undefined, name: string): string { } catch { throw new Error(`${name} must be an absolute URL`); } - const isLoopback = url.hostname === "localhost" || url.hostname === "127.0.0.1"; + const isLoopback = + url.hostname === "localhost" || + url.hostname === "127.0.0.1" || + url.hostname === "[::1]"; if (url.protocol !== "https:" && !(url.protocol === "http:" && isLoopback)) { throw new Error(`${name} must use HTTPS (HTTP is allowed only for loopback tests)`); } + if (url.username || url.password) { + throw new Error(`${name} must not contain user credentials`); + } if (url.search || url.hash) { throw new Error(`${name} must not contain a query string or fragment`); } @@ -90,13 +103,18 @@ export function loadConfig( const allowNoAuth = parseBoolean(env.MCP_ALLOW_NO_AUTH, false); const authToken = env.MCP_AUTH_TOKEN?.trim() || undefined; const oauthEnabled = parseBoolean(env.MCP_OAUTH_ENABLED, false); - if (!allowNoAuth && !authToken) { + const oauthApprovalKey = oauthEnabled + ? env.MCP_OAUTH_APPROVAL_KEY?.trim() || authToken + : undefined; + if (!allowNoAuth && !authToken && !oauthEnabled) { throw new Error( "MCP_AUTH_TOKEN is required. Set MCP_ALLOW_NO_AUTH=true only when an upstream OAuth gateway or private network authenticates callers.", ); } - if (oauthEnabled && !authToken) { - throw new Error("MCP_AUTH_TOKEN is required as the OAuth authorization access key"); + if (oauthEnabled && !oauthApprovalKey) { + throw new Error( + "MCP_OAUTH_APPROVAL_KEY (or MCP_AUTH_TOKEN for backward compatibility) is required when OAuth is enabled", + ); } const defaultCwd = path.resolve(env.MCP_DEFAULT_CWD?.trim() || processCwd); @@ -118,13 +136,21 @@ export function loadConfig( return { host: env.MCP_HOST?.trim() || "0.0.0.0", - port: parseInteger(env.MCP_PORT, 3000, "MCP_PORT", 1), + port: parseInteger(env.MCP_PORT, 3000, "MCP_PORT", 1, 65_535), endpoint, publicUrl, allowedHosts: allowedHosts && allowedHosts.length > 0 ? allowedHosts : undefined, + trustProxyHops: parseInteger( + env.MCP_TRUST_PROXY_HOPS, + 0, + "MCP_TRUST_PROXY_HOPS", + 0, + 16, + ), authToken, allowNoAuth, oauthEnabled, + oauthApprovalKey, oauthIssuerUrl, oauthResourceUrl, oauthStateFile: path.resolve( diff --git a/src/file-service.ts b/src/file-service.ts index 613641e..dbe345e 100644 --- a/src/file-service.ts +++ b/src/file-service.ts @@ -100,6 +100,16 @@ function decodeContent(data: string, encoding: FileContentEncoding): Buffer { return encoding === "base64" ? decodeBase64(data) : Buffer.from(data, "utf8"); } +function isPathWithin(parentPath: string, candidatePath: string): boolean { + const relative = path.relative(parentPath, candidatePath); + return ( + relative !== "" && + relative !== ".." && + !relative.startsWith(`..${path.sep}`) && + !path.isAbsolute(relative) + ); +} + function utf8SequenceLength(firstByte: number): number { if (firstByte <= 0x7f) { return 1; @@ -418,7 +428,11 @@ export class FileService { `${resolvedPath} is ${info.size} bytes; replace_in_file limit is ${this.#options.maxEditFileBytes}`, ); } - const original = await readFile(resolvedPath, "utf8"); + const originalBuffer = await readFile(resolvedPath); + if (!isUtf8(originalBuffer)) { + throw new Error(`${resolvedPath} is not valid UTF-8`); + } + const original = originalBuffer.toString("utf8"); const occurrences = original.split(oldText).length - 1; const expected = expectedOccurrences ?? (replaceAll ? occurrences : 1); if (occurrences !== expected) { @@ -559,6 +573,13 @@ export class FileService { if (source === destination) { return { source, destination, moved: false, samePath: true }; } + await lstat(source); + if (isPathWithin(source, destination) || isPathWithin(destination, source)) { + throw new Error("Source and destination paths must not contain one another"); + } + + await mkdir(path.dirname(destination), { recursive: true }); + let destinationBackup: string | undefined; if (!overwrite) { try { await lstat(destination); @@ -569,19 +590,69 @@ export class FileService { } } } else { - await rm(destination, { recursive: true, force: true }); - } - await mkdir(path.dirname(destination), { recursive: true }); - try { - await rename(source, destination); - } catch (error) { - if ((error as NodeJS.ErrnoException).code !== "EXDEV") { - throw error; + try { + await lstat(destination); + destinationBackup = path.join( + path.dirname(destination), + `.cokacremote-move-backup-${randomUUID()}`, + ); + await rename(destination, destinationBackup); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ENOENT") { + throw error; + } } - await cp(source, destination, { recursive: true, force: overwrite }); - await rm(source, { recursive: true, force: true }); } - return { source, destination, moved: true }; + + let destinationMayBePartial = false; + try { + try { + await rename(source, destination); + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "EXDEV") { + throw error; + } + destinationMayBePartial = true; + await cp(source, destination, { + recursive: true, + force: false, + errorOnExist: true, + }); + try { + await rm(source, { recursive: true, force: true }); + } catch (removeError) { + await rm(destination, { recursive: true, force: true }).catch(() => undefined); + throw removeError; + } + } + } catch (error) { + const rollbackErrors: string[] = []; + if (destinationBackup || destinationMayBePartial) { + await rm(destination, { recursive: true, force: true }).catch((rollbackError) => { + rollbackErrors.push(`remove partial destination: ${errorMessage(rollbackError)}`); + }); + } + if (destinationBackup) { + await rename(destinationBackup, destination).catch((rollbackError) => { + rollbackErrors.push(`restore original destination: ${errorMessage(rollbackError)}`); + }); + } + if (rollbackErrors.length > 0) { + throw new Error(`${errorMessage(error)}; rollback failed: ${rollbackErrors.join("; ")}`); + } + throw error; + } + + const result: Record = { source, destination, moved: true }; + if (destinationBackup) { + try { + await rm(destinationBackup, { recursive: true, force: true }); + } catch (error) { + result.warning = `Move succeeded but the old destination backup could not be removed: ${errorMessage(error)}`; + result.backupPath = destinationBackup; + } + } + return result; } async removePath( diff --git a/src/http-server.ts b/src/http-server.ts index 024fc65..f5e9418 100644 --- a/src/http-server.ts +++ b/src/http-server.ts @@ -2,7 +2,11 @@ import { randomUUID } from "node:crypto"; import type { Server as HttpServer } from "node:http"; import { StreamableHTTPServerTransport } from "@modelcontextprotocol/sdk/server/streamableHttp.js"; -import { mcpAuthRouter } from "@modelcontextprotocol/sdk/server/auth/router.js"; +import { + createOAuthMetadata, + mcpAuthRouter, + type AuthRouterOptions, +} from "@modelcontextprotocol/sdk/server/auth/router.js"; import express, { type Request, type Response } from "express"; import { createBearerAuth, createHostValidation } from "./auth.js"; @@ -54,7 +58,9 @@ export async function startHttpServer( ): Promise { const app = express(); app.disable("x-powered-by"); - app.set("trust proxy", 1); + if (config.trustProxyHops > 0) { + app.set("trust proxy", config.trustProxyHops); + } app.use((request, response, next) => { if (request.path !== config.endpoint) { next(); @@ -90,7 +96,6 @@ export async function startHttpServer( }); next(); }); - app.use(express.json({ limit: config.maxRequestBody })); app.use(createHostValidation(config)); const activeRequests = new Set(); @@ -98,7 +103,7 @@ export async function startHttpServer( const oauthProvider = config.oauthEnabled ? new RemoteDevOAuthProvider(config) : undefined; if (oauthProvider) { app.get("/.well-known/oauth-protected-resource", (_request, response) => { - response.json({ + response.set("Access-Control-Allow-Origin", "*").json({ resource: oauthProvider.resourceUrl.href, authorization_servers: [oauthProvider.issuerUrl.href], scopes_supported: [...OAUTH_SCOPES], @@ -106,18 +111,33 @@ export async function startHttpServer( resource_name: "cokacremote", }); }); - app.use( - mcpAuthRouter({ - provider: oauthProvider, - issuerUrl: oauthProvider.issuerUrl, - resourceServerUrl: oauthProvider.resourceUrl, - scopesSupported: [...OAUTH_SCOPES], - resourceName: "cokacremote", - clientRegistrationOptions: { clientSecretExpirySeconds: 0 }, - }), - ); + const oauthRouterOptions = { + provider: oauthProvider, + issuerUrl: oauthProvider.issuerUrl, + resourceServerUrl: oauthProvider.resourceUrl, + scopesSupported: [...OAUTH_SCOPES], + resourceName: "cokacremote", + } satisfies AuthRouterOptions; + const oauthMetadata = { + ...createOAuthMetadata(oauthRouterOptions), + revocation_endpoint_auth_methods_supported: ["client_secret_post", "none"], + }; + const issuerPath = oauthProvider.issuerUrl.pathname.replace(/\/$/, ""); + const oauthMetadataPath = `/.well-known/oauth-authorization-server${issuerPath}`; + app.use((request, response, next) => { + if ( + (request.method === "GET" || request.method === "HEAD") && + request.path === oauthMetadataPath + ) { + response.set("Access-Control-Allow-Origin", "*").json(oauthMetadata); + return; + } + next(); + }); + app.use(mcpAuthRouter(oauthRouterOptions)); } const authenticate = createBearerAuth(config, oauthProvider); + const parseMcpJson = express.json({ limit: config.maxRequestBody }); app.get("/health", (_request, response) => { response.json({ @@ -176,9 +196,14 @@ export async function startHttpServer( rpcError(response, 405, "Stateless MCP accepts POST requests only"); }; - app.post(config.endpoint, authenticate, (request, response) => { - void postHandler(request, response); - }); + app.post( + config.endpoint, + authenticate, + parseMcpJson, + (request, response) => { + void postHandler(request, response); + }, + ); app.get(config.endpoint, authenticate, methodNotAllowed); app.delete(config.endpoint, authenticate, methodNotAllowed); diff --git a/src/oauth.ts b/src/oauth.ts index 7797dc2..8c390c3 100644 --- a/src/oauth.ts +++ b/src/oauth.ts @@ -1,12 +1,14 @@ import { createHash, randomBytes, randomUUID } from "node:crypto"; -import { chmod, mkdir, readFile, rename, unlink, writeFile } from "node:fs/promises"; +import { mkdir, readFile, rename, unlink, writeFile } from "node:fs/promises"; import path from "node:path"; import type { OAuthRegisteredClientsStore } from "@modelcontextprotocol/sdk/server/auth/clients.js"; import { + InvalidClientMetadataError, InvalidGrantError, InvalidScopeError, InvalidTargetError, + UnauthorizedClientError, } from "@modelcontextprotocol/sdk/server/auth/errors.js"; import type { AuthorizationParams, @@ -26,11 +28,12 @@ import type { AppConfig } from "./config.js"; export const OAUTH_SCOPES = ["mcp:tools"] as const; interface StoredToken { - type: "access" | "refresh"; + type: "access" | "refresh" | "used_refresh"; clientId: string; scopes: string[]; expiresAt: number; resource: string; + grantId?: string; } interface PersistedOAuthState { @@ -65,18 +68,100 @@ function randomToken(): string { return randomBytes(32).toString("base64url"); } +const LOOPBACK_REDIRECT_HOSTS = new Set(["localhost", "127.0.0.1", "[::1]"]); +const SUPPORTED_CLIENT_AUTH_METHODS = new Set(["none", "client_secret_post"]); +const SUPPORTED_GRANT_TYPES = new Set(["authorization_code", "refresh_token"]); + +function clientMetadataProblem(value: unknown): string | undefined { + if (!value || typeof value !== "object" || Array.isArray(value)) { + return "Client metadata must be an object"; + } + const client = value as Partial; + if (!Array.isArray(client.redirect_uris) || client.redirect_uris.length === 0) { + return "At least one redirect_uri is required"; + } + for (const redirectUri of client.redirect_uris) { + if (typeof redirectUri !== "string") { + return "Every redirect_uri must be an absolute URL"; + } + let parsed: URL; + try { + parsed = new URL(redirectUri); + } catch { + return "Every redirect_uri must be an absolute URL"; + } + const isLoopback = LOOPBACK_REDIRECT_HOSTS.has(parsed.hostname); + if (parsed.protocol !== "https:" && !(parsed.protocol === "http:" && isLoopback)) { + return "Every redirect_uri must use HTTPS or HTTP on a loopback host"; + } + if (parsed.hash || parsed.username || parsed.password) { + return "redirect_uris must not contain fragments or user credentials"; + } + } + + if ( + client.token_endpoint_auth_method !== undefined && + typeof client.token_endpoint_auth_method !== "string" + ) { + return "token_endpoint_auth_method must be a string"; + } + const authMethod = client.token_endpoint_auth_method ?? "client_secret_post"; + if (!SUPPORTED_CLIENT_AUTH_METHODS.has(authMethod)) { + return `Unsupported token_endpoint_auth_method: ${authMethod}`; + } + + if ( + client.grant_types !== undefined && + (!Array.isArray(client.grant_types) || + !client.grant_types.every((grantType) => typeof grantType === "string")) + ) { + return "grant_types must be an array of strings"; + } + const grantTypes = client.grant_types ?? ["authorization_code"]; + if ( + grantTypes.length === 0 || + !grantTypes.includes("authorization_code") || + !grantTypes.every((grantType) => SUPPORTED_GRANT_TYPES.has(grantType)) + ) { + return "grant_types must contain authorization_code and may contain refresh_token"; + } + + if ( + client.response_types !== undefined && + (!Array.isArray(client.response_types) || + !client.response_types.every((responseType) => typeof responseType === "string")) + ) { + return "response_types must be an array of strings"; + } + const responseTypes = client.response_types ?? ["code"]; + if (responseTypes.length !== 1 || responseTypes[0] !== "code") { + return "Only the code response_type is supported"; + } + + if (client.scope !== undefined && typeof client.scope !== "string") { + return "scope must be a string"; + } + const scopes = client.scope?.split(/\s+/).filter(Boolean) ?? []; + if (!scopes.every((scope) => OAUTH_SCOPES.includes(scope as (typeof OAUTH_SCOPES)[number]))) { + return "Only the mcp:tools scope is supported"; + } + return undefined; +} + function isStoredToken(value: unknown): value is StoredToken { if (!value || typeof value !== "object") { return false; } const token = value as Partial; return ( - (token.type === "access" || token.type === "refresh") && + (token.type === "access" || token.type === "refresh" || token.type === "used_refresh") && typeof token.clientId === "string" && Array.isArray(token.scopes) && token.scopes.every((scope) => typeof scope === "string") && typeof token.expiresAt === "number" && - typeof token.resource === "string" + typeof token.resource === "string" && + (token.grantId === undefined || typeof token.grantId === "string") && + (token.type !== "used_refresh" || typeof token.grantId === "string") ); } @@ -86,8 +171,10 @@ function parseState(value: string): PersistedOAuthState { parsed.version !== 1 || !parsed.clients || typeof parsed.clients !== "object" || + Array.isArray(parsed.clients) || !parsed.tokens || typeof parsed.tokens !== "object" || + Array.isArray(parsed.tokens) || !Object.values(parsed.tokens).every(isStoredToken) ) { throw new Error("Invalid OAuth state file format"); @@ -131,7 +218,6 @@ class PersistentOAuthStore implements OAuthRegisteredClientsStore { private async persist(): Promise { const directory = path.dirname(this.stateFile); await mkdir(directory, { recursive: true, mode: 0o700 }); - await chmod(directory, 0o700); const temporaryFile = `${this.stateFile}.${process.pid}.${randomBytes(6).toString("hex")}.tmp`; try { await writeFile(temporaryFile, `${JSON.stringify(this.state, null, 2)}\n`, { @@ -149,10 +235,16 @@ class PersistentOAuthStore implements OAuthRegisteredClientsStore { private async mutate(operation: () => T | Promise): Promise { await this.ensureLoaded(); const pending = this.mutationQueue.then(async () => { - this.pruneExpired(); - const result = await operation(); - await this.persist(); - return result; + const snapshot = structuredClone(this.state); + try { + this.pruneExpired(); + const result = await operation(); + await this.persist(); + return result; + } catch (error) { + this.state = snapshot; + throw error; + } }); this.mutationQueue = pending.then( () => undefined, @@ -164,7 +256,11 @@ class PersistentOAuthStore implements OAuthRegisteredClientsStore { async getClient(clientId: string): Promise { await this.ensureLoaded(); await this.mutationQueue; - return this.state.clients[clientId]; + const client = this.state.clients[clientId]; + if (!client || client.client_id !== clientId || clientMetadataProblem(client)) { + return undefined; + } + return client; } async registerClient( @@ -173,26 +269,48 @@ class PersistentOAuthStore implements OAuthRegisteredClientsStore { const supplied = client as Partial; const registered: OAuthClientInformationFull = { ...client, + token_endpoint_auth_method: + client.token_endpoint_auth_method ?? "client_secret_post", + grant_types: client.grant_types ?? ["authorization_code"], + response_types: client.response_types ?? ["code"], client_id: supplied.client_id || randomUUID(), client_id_issued_at: supplied.client_id_issued_at || Math.floor(Date.now() / 1000), }; + const problem = clientMetadataProblem(registered); + if (problem) { + throw new InvalidClientMetadataError(problem); + } return this.mutate(() => { this.state.clients[registered.client_id] = registered; return registered; }); } - async issueTokenPair(clientId: string, scopes: string[], resource: string): Promise { - return this.mutate(() => this.issueTokenPairWithoutPersist(clientId, scopes, resource)); + async issueTokenPair( + clientId: string, + scopes: string[], + resource: string, + issueRefreshToken = true, + ): Promise { + return this.mutate(() => + this.issueTokenPairWithoutPersist( + clientId, + scopes, + resource, + randomUUID(), + issueRefreshToken, + ), + ); } private issueTokenPairWithoutPersist( clientId: string, scopes: string[], resource: string, + grantId: string, + issueRefreshToken = true, ): OAuthTokens { const accessToken = randomToken(); - const refreshToken = randomToken(); const now = Date.now(); this.state.tokens[tokenHash(accessToken)] = { type: "access", @@ -200,21 +318,38 @@ class PersistentOAuthStore implements OAuthRegisteredClientsStore { scopes, expiresAt: now + this.accessTokenTtlSeconds * 1000, resource, + grantId, }; - this.state.tokens[tokenHash(refreshToken)] = { - type: "refresh", - clientId, - scopes, - expiresAt: now + this.refreshTokenTtlSeconds * 1000, - resource, - }; - return { + const tokens: OAuthTokens = { access_token: accessToken, token_type: "Bearer", expires_in: this.accessTokenTtlSeconds, - refresh_token: refreshToken, scope: scopes.join(" "), }; + if (issueRefreshToken) { + const refreshToken = randomToken(); + this.state.tokens[tokenHash(refreshToken)] = { + type: "refresh", + clientId, + scopes, + expiresAt: now + this.refreshTokenTtlSeconds * 1000, + resource, + grantId, + }; + tokens.refresh_token = refreshToken; + } + return tokens; + } + + private revokeGrantWithoutPersist(grantId: string, preserveReplayEvidence = false): void { + for (const [hash, token] of Object.entries(this.state.tokens)) { + if ( + token.grantId === grantId && + !(preserveReplayEvidence && token.type === "used_refresh") + ) { + delete this.state.tokens[hash]; + } + } } async rotateRefreshToken( @@ -228,21 +363,28 @@ class PersistentOAuthStore implements OAuthRegisteredClientsStore { const current = this.state.tokens[hash]; if ( !current || - current.type !== "refresh" || current.clientId !== clientId || current.resource !== resource || current.expiresAt <= Date.now() ) { return { status: "invalid" }; } + if (current.type === "used_refresh") { + this.revokeGrantWithoutPersist(current.grantId!, true); + return { status: "invalid" }; + } + if (current.type !== "refresh") { + return { status: "invalid" }; + } const scopes = requestedScopes ?? current.scopes; if (!scopes.every((scope) => current.scopes.includes(scope))) { return { status: "invalid_scope" }; } - delete this.state.tokens[hash]; + const grantId = current.grantId ?? randomUUID(); + this.state.tokens[hash] = { ...current, type: "used_refresh", grantId }; return { status: "ok", - tokens: this.issueTokenPairWithoutPersist(clientId, scopes, resource), + tokens: this.issueTokenPairWithoutPersist(clientId, scopes, resource, grantId), }; }); } @@ -254,14 +396,23 @@ class PersistentOAuthStore implements OAuthRegisteredClientsStore { if (!stored || stored.type !== "access" || stored.expiresAt <= Date.now()) { return undefined; } + const client = this.state.clients[stored.clientId]; + if (!client || clientMetadataProblem(client)) { + return undefined; + } return stored; } async revoke(token: string, clientId: string): Promise { await this.mutate(() => { const hash = tokenHash(token); - if (this.state.tokens[hash]?.clientId === clientId) { - delete this.state.tokens[hash]; + const stored = this.state.tokens[hash]; + if (stored?.clientId === clientId) { + if (stored.grantId) { + this.revokeGrantWithoutPersist(stored.grantId); + } else { + delete this.state.tokens[hash]; + } } }); } @@ -350,7 +501,7 @@ export class RemoteDevOAuthProvider implements OAuthServerProvider { private readonly authorizationCodes = new Map(); constructor(private readonly config: AppConfig) { - if (!config.oauthIssuerUrl || !config.oauthResourceUrl || !config.authToken) { + if (!config.oauthIssuerUrl || !config.oauthResourceUrl || !config.oauthApprovalKey) { throw new Error("OAuth configuration is incomplete"); } this.issuerUrl = new URL(config.oauthIssuerUrl); @@ -391,9 +542,11 @@ export class RemoteDevOAuthProvider implements OAuthServerProvider { params: AuthorizationParams, response: Response, ): Promise { + if (!(client.grant_types ?? ["authorization_code"]).includes("authorization_code")) { + throw new UnauthorizedClientError("Client is not authorized for authorization_code"); + } const resource = this.validateResource(params.resource); const scopes = this.validateScopes(params.scopes); - const redirectOrigin = new URL(params.redirectUri).origin; const request = response.req as Request; const accessKey = request.method === "POST" && typeof request.body?.access_key === "string" @@ -402,12 +555,12 @@ export class RemoteDevOAuthProvider implements OAuthServerProvider { response.set({ "Content-Security-Policy": - `default-src 'none'; style-src 'unsafe-inline'; form-action 'self' ${redirectOrigin}; base-uri 'none'; frame-ancestors 'none'`, + "default-src 'none'; style-src 'unsafe-inline'; form-action 'self'; base-uri 'none'; frame-ancestors 'none'", "Referrer-Policy": "no-referrer", "X-Content-Type-Options": "nosniff", }); - if (!accessKey || !tokensEqual(accessKey, this.config.authToken!)) { + if (!accessKey || !tokensEqual(accessKey, this.config.oauthApprovalKey!)) { response .status(accessKey ? 401 : 200) .type("html") @@ -431,7 +584,7 @@ export class RemoteDevOAuthProvider implements OAuthServerProvider { if (params.state !== undefined) { target.searchParams.set("state", params.state); } - response.redirect(302, target.href); + response.redirect(303, target.href); } async challengeForAuthorizationCode( @@ -464,7 +617,19 @@ export class RemoteDevOAuthProvider implements OAuthServerProvider { throw new InvalidGrantError("Invalid authorization code binding"); } this.authorizationCodes.delete(authorizationCode); - return this.clientsStore.issueTokenPair(client.client_id, record.scopes, record.resource); + try { + return await this.clientsStore.issueTokenPair( + client.client_id, + record.scopes, + record.resource, + client.grant_types?.includes("refresh_token") ?? false, + ); + } catch (error) { + if (record.expiresAt > Date.now() && !this.authorizationCodes.has(authorizationCode)) { + this.authorizationCodes.set(authorizationCode, record); + } + throw error; + } } async exchangeRefreshToken( @@ -473,6 +638,9 @@ export class RemoteDevOAuthProvider implements OAuthServerProvider { scopes?: string[], resource?: URL, ): Promise { + if (!client.grant_types?.includes("refresh_token")) { + throw new UnauthorizedClientError("Client is not authorized for refresh_token"); + } const resourceValue = this.validateResource(resource); const requestedScopes = scopes ? this.validateScopes(scopes) : undefined; const result = await this.clientsStore.rotateRefreshToken( diff --git a/src/process-manager.ts b/src/process-manager.ts index 0d0f71a..4be3e08 100644 --- a/src/process-manager.ts +++ b/src/process-manager.ts @@ -1,5 +1,6 @@ import { randomUUID } from "node:crypto"; import { spawn, type ChildProcessWithoutNullStreams } from "node:child_process"; +import { isAscii } from "node:buffer"; import { errorMessage } from "./errors.js"; @@ -25,6 +26,7 @@ interface ManagedProcess { error: string | undefined; timedOut: boolean; chunks: OutputChunk[]; + pendingOutput: Record; retainedBytes: number; totalOutputBytes: number; droppedOutputBytes: number; @@ -35,6 +37,99 @@ interface ManagedProcess { cleanup: (() => Promise) | undefined; } +function isContinuationByte(value: number): boolean { + return value >= 0x80 && value <= 0xbf; +} + +function utf8SequenceLengthAt( + data: Buffer, + offset: number, + final: boolean, +): number | undefined { + const first = data[offset]!; + if (first <= 0x7f) { + return 1; + } + + let length = 0; + if (first >= 0xc2 && first <= 0xdf) { + length = 2; + } else if (first >= 0xe0 && first <= 0xef) { + length = 3; + } else if (first >= 0xf0 && first <= 0xf4) { + length = 4; + } else { + return 1; + } + + if (offset + 1 >= data.length) { + return final ? 1 : undefined; + } + + const second = data[offset + 1]!; + if (!isContinuationByte(second)) { + return 1; + } + if ((first === 0xe0 && second < 0xa0) || (first === 0xed && second > 0x9f)) { + return 1; + } + if ((first === 0xf0 && second < 0x90) || (first === 0xf4 && second > 0x8f)) { + return 1; + } + for (let index = 2; index < length; index += 1) { + if (offset + index >= data.length) { + return final ? 1 : undefined; + } + if (!isContinuationByte(data[offset + index]!)) { + return 1; + } + } + return length; +} + +function splitOutputChunks( + data: Buffer, + final = false, +): { chunks: Buffer[]; remainder: Buffer } { + if (isAscii(data)) { + const chunks: Buffer[] = []; + for (let offset = 0; offset < data.length; offset += OUTPUT_CHUNK_BYTES) { + chunks.push(Buffer.from(data.subarray(offset, offset + OUTPUT_CHUNK_BYTES))); + } + return { chunks, remainder: Buffer.alloc(0) }; + } + + const chunks: Buffer[] = []; + let chunkStart = 0; + let cursor = 0; + while (cursor < data.length) { + const sequenceLength = utf8SequenceLengthAt(data, cursor, final); + if (sequenceLength === undefined) { + break; + } + if ( + cursor > chunkStart && + cursor + sequenceLength - chunkStart > OUTPUT_CHUNK_BYTES + ) { + chunks.push(Buffer.from(data.subarray(chunkStart, cursor))); + chunkStart = cursor; + continue; + } + cursor += sequenceLength; + if (cursor - chunkStart === OUTPUT_CHUNK_BYTES) { + chunks.push(Buffer.from(data.subarray(chunkStart, cursor))); + chunkStart = cursor; + } + } + if (cursor > chunkStart) { + chunks.push(Buffer.from(data.subarray(chunkStart, cursor))); + } + return { + chunks, + remainder: Buffer.from(data.subarray(cursor)), + }; +} + export interface StartProcessRequest { executable: string; args: string[]; @@ -113,6 +208,7 @@ export class ProcessManager { error: undefined, timedOut: false, chunks: [], + pendingOutput: { stdout: Buffer.alloc(0), stderr: Buffer.alloc(0) }, retainedBytes: 0, totalOutputBytes: 0, droppedOutputBytes: 0, @@ -130,6 +226,9 @@ export class ProcessManager { child.stderr.on("data", (data: Buffer | string) => { this.#appendOutput(managed, "stderr", Buffer.from(data)); }); + child.stdin.on("error", (error) => { + this.#recordStdinError(managed, error); + }); child.on("error", (error) => { managed.error = errorMessage(error); this.#finish(managed, null, null); @@ -155,7 +254,15 @@ export class ProcessManager { } if (request.stdin !== undefined && request.stdin.length > 0) { - child.stdin.write(request.stdin); + try { + child.stdin.write(request.stdin, (error) => { + if (error) { + this.#recordStdinError(managed, error); + } + }); + } catch (error) { + this.#recordStdinError(managed, error); + } } return sessionId; } @@ -387,17 +494,29 @@ export class ProcessManager { stream: ProcessOutputStream, data: Buffer, ): void { - for (let offset = 0; offset < data.length; offset += OUTPUT_CHUNK_BYTES) { - const chunkData = Buffer.from(data.subarray(offset, offset + OUTPUT_CHUNK_BYTES)); - managed.chunks.push({ - seq: managed.nextSeq, - stream, - data: chunkData, - }); + managed.totalOutputBytes += data.length; + const pending = managed.pendingOutput[stream]; + const combined = pending.length > 0 ? Buffer.concat([pending, data]) : data; + const split = splitOutputChunks(combined); + managed.pendingOutput[stream] = split.remainder; + this.#storeOutputChunks(managed, stream, split.chunks); + this.#trimRetainedOutput(managed); + this.#notify(managed); + } + + #storeOutputChunks( + managed: ManagedProcess, + stream: ProcessOutputStream, + chunks: Buffer[], + ): void { + for (const data of chunks) { + managed.chunks.push({ seq: managed.nextSeq, stream, data }); managed.nextSeq += 1; - managed.retainedBytes += chunkData.length; - managed.totalOutputBytes += chunkData.length; + managed.retainedBytes += data.length; } + } + + #trimRetainedOutput(managed: ManagedProcess): void { while ( managed.retainedBytes > this.#options.maxRetainedOutputBytes && managed.chunks.length > 0 @@ -408,6 +527,23 @@ export class ProcessManager { managed.droppedOutputBytes += removed.data.length; } } + } + + #flushPendingOutput(managed: ManagedProcess): void { + for (const stream of ["stdout", "stderr"] as const) { + const pending = managed.pendingOutput[stream]; + if (pending.length === 0) { + continue; + } + const split = splitOutputChunks(pending, true); + managed.pendingOutput[stream] = Buffer.alloc(0); + this.#storeOutputChunks(managed, stream, split.chunks); + } + this.#trimRetainedOutput(managed); + } + + #recordStdinError(managed: ManagedProcess, error: unknown): void { + managed.error ??= `stdin write failed: ${errorMessage(error)}`; this.#notify(managed); } @@ -419,6 +555,7 @@ export class ProcessManager { if (managed.endedAt !== undefined) { return; } + this.#flushPendingOutput(managed); managed.endedAt = Date.now(); managed.exitCode = code; managed.signal = signal; diff --git a/src/server.ts b/src/server.ts index d7e8a4c..47b16bf 100644 --- a/src/server.ts +++ b/src/server.ts @@ -15,10 +15,12 @@ async function main(): Promise { console.log(`default cwd: ${config.defaultCwd}`); console.log("execution mode: unrestricted host access"); console.log( - config.allowNoAuth && !config.authToken + config.allowNoAuth && !config.authToken && !config.oauthEnabled ? "authentication: disabled" : config.oauthEnabled - ? "authentication: static bearer + OAuth 2.1 (DCR/PKCE)" + ? config.authToken + ? "authentication: static bearer + OAuth 2.1 (DCR/PKCE)" + : "authentication: OAuth 2.1 (DCR/PKCE)" : "authentication: bearer token", ); diff --git a/test/auth-security.integration.test.ts b/test/auth-security.integration.test.ts new file mode 100644 index 0000000..8c28c55 --- /dev/null +++ b/test/auth-security.integration.test.ts @@ -0,0 +1,167 @@ +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import type { AddressInfo } from "node:net"; +import { createServer } from "node:net"; +import os from "node:os"; +import path from "node:path"; + +import { describe, expect, it } from "vitest"; + +import { loadConfig } from "../src/config.js"; +import { startHttpServer } from "../src/http-server.js"; +import { createServices } from "../src/mcp-server.js"; +import { RemoteDevOAuthProvider } from "../src/oauth.js"; + +async function reservePort(): Promise { + const server = createServer(); + await new Promise((resolve, reject) => { + server.once("error", reject); + server.listen(0, "127.0.0.1", resolve); + }); + const port = (server.address() as AddressInfo).port; + await new Promise((resolve, reject) => { + server.close((error) => (error ? reject(error) : resolve())); + }); + return port; +} + +describe("OAuth endpoint security boundaries", () => { + it("does not trust spoofed forwarded IPs unless a proxy is explicitly configured", async () => { + const temporaryDirectory = await mkdtemp( + path.join(os.tmpdir(), "cokacremote-auth-boundary-test-"), + ); + const port = await reservePort(); + const baseUrl = `http://127.0.0.1:${port}`; + const config = loadConfig( + { + MCP_OAUTH_ENABLED: "true", + MCP_OAUTH_APPROVAL_KEY: "oauth-approval-key", + MCP_PUBLIC_URL: baseUrl, + MCP_OAUTH_STATE_FILE: path.join(temporaryDirectory, "oauth-state.json"), + MCP_HOST: "127.0.0.1", + MCP_PORT: String(port), + MCP_DEFAULT_CWD: temporaryDirectory, + }, + temporaryDirectory, + ); + const running = await startHttpServer(config, createServices(config)); + + try { + const approvalKeyAsBearer = await fetch(`${baseUrl}/mcp`, { + method: "POST", + headers: { + authorization: "Bearer oauth-approval-key", + "content-type": "application/json", + }, + body: JSON.stringify({ jsonrpc: "2.0", id: 1, method: "initialize", params: {} }), + }); + expect(approvalKeyAsBearer.status).toBe(401); + + const statuses: number[] = []; + for (let index = 1; index <= 21; index += 1) { + const response = await fetch(`${baseUrl}/register`, { + method: "POST", + headers: { + "content-type": "application/json", + "x-forwarded-for": `203.0.113.${index}`, + }, + body: JSON.stringify({ + redirect_uris: ["https://chatgpt.com/connector/oauth/security-test"], + token_endpoint_auth_method: "none", + grant_types: ["authorization_code", "refresh_token"], + response_types: ["code"], + client_name: `security-test-${index}`, + scope: "mcp:tools", + }), + }); + statuses.push(response.status); + } + + expect(statuses.slice(0, 20)).toEqual(Array(20).fill(201)); + expect(statuses[20]).toBe(429); + } finally { + await running.close(); + await rm(temporaryDirectory, { recursive: true, force: true }); + } + }); + + it("rolls back failed state writes and revokes an entire token grant", async () => { + const temporaryDirectory = await mkdtemp( + path.join(os.tmpdir(), "cokacremote-oauth-store-test-"), + ); + const baseEnvironment = { + MCP_OAUTH_ENABLED: "true", + MCP_OAUTH_APPROVAL_KEY: "oauth-approval-key", + MCP_PUBLIC_URL: "http://127.0.0.1:34567", + }; + const metadata = { + client_id: "security-store-client", + redirect_uris: ["https://chatgpt.com/connector/oauth/security-store-test"], + token_endpoint_auth_method: "none", + grant_types: ["authorization_code", "refresh_token"], + response_types: ["code"], + client_name: "security store test", + scope: "mcp:tools", + }; + + try { + const blockedDirectory = path.join(temporaryDirectory, "blocked-state-directory"); + await mkdir(blockedDirectory); + const failedProvider = new RemoteDevOAuthProvider( + loadConfig( + { + ...baseEnvironment, + MCP_OAUTH_STATE_FILE: path.join(blockedDirectory, "state.json"), + }, + temporaryDirectory, + ), + ); + await expect( + failedProvider.clientsStore.getClient(metadata.client_id), + ).resolves.toBeUndefined(); + await rm(blockedDirectory, { recursive: true, force: true }); + await writeFile(blockedDirectory, "block state persistence"); + await expect( + failedProvider.clientsStore.registerClient( + metadata as Parameters[0], + ), + ).rejects.toThrow(); + await expect( + failedProvider.clientsStore.getClient(metadata.client_id), + ).resolves.toBeUndefined(); + + const stateFile = path.join(temporaryDirectory, "valid-state.json"); + const provider = new RemoteDevOAuthProvider( + loadConfig( + { ...baseEnvironment, MCP_OAUTH_STATE_FILE: stateFile }, + temporaryDirectory, + ), + ); + const client = await provider.clientsStore.registerClient( + metadata as Parameters[0], + ); + const resource = "http://127.0.0.1:34567/mcp"; + const pair = await provider.clientsStore.issueTokenPair( + client.client_id, + ["mcp:tools"], + resource, + ); + expect(pair.refresh_token).toBeTypeOf("string"); + + await provider.clientsStore.revoke(pair.refresh_token!, client.client_id); + + await expect( + provider.clientsStore.getAccessToken(pair.access_token), + ).resolves.toBeUndefined(); + await expect( + provider.clientsStore.rotateRefreshToken( + pair.refresh_token!, + client.client_id, + resource, + undefined, + ), + ).resolves.toMatchObject({ status: "invalid" }); + } finally { + await rm(temporaryDirectory, { recursive: true, force: true }); + } + }); +}); diff --git a/test/config.test.ts b/test/config.test.ts index 6837b83..c5bb6f0 100644 --- a/test/config.test.ts +++ b/test/config.test.ts @@ -22,11 +22,23 @@ describe("loadConfig", () => { expect(config).toMatchObject({ port: 4321, defaultCwd: "/", + trustProxyHops: 0, authToken: "secret", allowedHosts: ["mcp.example.com", "localhost"], }); }); + it("rejects partial integers and ports outside the valid range", () => { + for (const value of ["3000oops", "3000.9", "70000"]) { + expect(() => + loadConfig({ MCP_AUTH_TOKEN: "secret", MCP_PORT: value }, "/tmp"), + ).toThrow("MCP_PORT must be an integer between 1 and 65535"); + } + expect( + loadConfig({ MCP_AUTH_TOKEN: "secret", MCP_PORT: " 4321 " }, "/tmp").port, + ).toBe(4321); + }); + it("requires public HTTPS metadata when OAuth is enabled", () => { expect(() => loadConfig({ MCP_AUTH_TOKEN: "secret", MCP_OAUTH_ENABLED: "true" }, "/tmp"), @@ -43,9 +55,54 @@ describe("loadConfig", () => { ); expect(config).toMatchObject({ oauthEnabled: true, + oauthApprovalKey: "secret", oauthIssuerUrl: "https://mcp.example.com/", oauthResourceUrl: "https://mcp.example.com/mcp", oauthStateFile: "/tmp/oauth-state.json", }); }); + + it("supports OAuth-only authentication with a separate approval key", () => { + const config = loadConfig( + { + MCP_OAUTH_ENABLED: "true", + MCP_OAUTH_APPROVAL_KEY: "separate-oauth-approval-key", + MCP_PUBLIC_URL: "https://mcp.example.com", + MCP_TRUST_PROXY_HOPS: "1", + }, + "/tmp", + ); + + expect(config).toMatchObject({ + authToken: undefined, + oauthApprovalKey: "separate-oauth-approval-key", + trustProxyHops: 1, + }); + expect(() => + loadConfig( + { + MCP_OAUTH_ENABLED: "true", + MCP_PUBLIC_URL: "https://mcp.example.com", + }, + "/tmp", + ), + ).toThrow("MCP_OAUTH_APPROVAL_KEY"); + }); + + it("rejects unsafe proxy trust and OAuth URL settings", () => { + expect(() => + loadConfig({ MCP_AUTH_TOKEN: "secret", MCP_TRUST_PROXY_HOPS: "17" }, "/tmp"), + ).toThrow("MCP_TRUST_PROXY_HOPS must be an integer between 0 and 16"); + expect(() => + loadConfig( + { + MCP_AUTH_TOKEN: "secret", + MCP_OAUTH_ENABLED: "true", + MCP_OAUTH_ISSUER: "https://user:password@mcp.example.com", + MCP_OAUTH_RESOURCE: "https://mcp.example.com/mcp", + }, + "/tmp", + ), + ).toThrow("must not contain user credentials"); + }); }); diff --git a/test/file-service.test.ts b/test/file-service.test.ts index 8612714..e2b8ab0 100644 --- a/test/file-service.test.ts +++ b/test/file-service.test.ts @@ -204,6 +204,22 @@ describe("FileService", () => { "base64", ); expect(binary.content).toBe("//4="); + + const invalidEdit = Buffer.from([0xff, 0x78]); + await files.writeFileContent( + "invalid-edit.bin", + undefined, + invalidEdit.toString("base64"), + "base64", + "overwrite", + true, + ); + await expect( + files.replaceInFile("invalid-edit.bin", undefined, "x", "y", false, 1), + ).rejects.toThrow("not valid UTF-8"); + expect(await readFile(path.join(temporaryDirectory, "invalid-edit.bin"))).toEqual( + invalidEdit, + ); }); it("applies an explicit file mode when overwriting an existing file", async () => { @@ -256,4 +272,22 @@ describe("FileService", () => { expect(await readFile(path.join(temporaryDirectory, "destination/value.txt"), "utf8")) .toBe("destination"); }); + + it("preserves an existing move destination when the source is missing", async () => { + await files.writeFileContent( + "destination.txt", + undefined, + "valuable", + "utf8", + "overwrite", + true, + ); + + await expect( + files.movePath("missing.txt", "destination.txt", undefined, true), + ).rejects.toThrow(/ENOENT|no such file/i); + expect(await readFile(path.join(temporaryDirectory, "destination.txt"), "utf8")).toBe( + "valuable", + ); + }); }); diff --git a/test/mcp.integration.test.ts b/test/mcp.integration.test.ts index 78fe70e..70c5c44 100644 --- a/test/mcp.integration.test.ts +++ b/test/mcp.integration.test.ts @@ -67,6 +67,25 @@ describe("remote development MCP server", () => { expect(response.status).toBe(401); }); + it("authenticates MCP requests before parsing their JSON body", async () => { + const unauthenticated = await fetch(endpoint, { + method: "POST", + headers: { "content-type": "application/json" }, + body: "{", + }); + expect(unauthenticated.status).toBe(401); + + const authenticated = await fetch(endpoint, { + method: "POST", + headers: { + authorization: "Bearer integration-secret", + "content-type": "application/json", + }, + body: "{", + }); + expect(authenticated.status).toBe(400); + }); + it("lists tools and executes script and file workflows", async () => { const client = new Client({ name: "integration-test", version: "1.0.0" }); const transport = new StreamableHTTPClientTransport(endpoint, { diff --git a/test/oauth.integration.test.ts b/test/oauth.integration.test.ts index 6d4d467..3257712 100644 --- a/test/oauth.integration.test.ts +++ b/test/oauth.integration.test.ts @@ -1,5 +1,5 @@ import { createHash, randomBytes } from "node:crypto"; -import { mkdtemp, readFile, rm, stat } from "node:fs/promises"; +import { mkdir, mkdtemp, readFile, rm, stat } from "node:fs/promises"; import type { AddressInfo } from "node:net"; import { createServer } from "node:net"; import os from "node:os"; @@ -40,14 +40,16 @@ describe("OAuth 2.1 MCP authorization", () => { beforeAll(async () => { temporaryDirectory = await mkdtemp(path.join(os.tmpdir(), "remote-dev-mcp-oauth-test-")); - stateFile = path.join(temporaryDirectory, "oauth", "state.json"); + const stateDirectory = path.join(temporaryDirectory, "oauth"); + await mkdir(stateDirectory, { mode: 0o755 }); + stateFile = path.join(stateDirectory, "state.json"); const port = await reservePort(); baseUrl = `http://127.0.0.1:${port}`; resourceUrl = `${baseUrl}/mcp`; config = loadConfig( { - MCP_AUTH_TOKEN: "oauth-login-secret", MCP_OAUTH_ENABLED: "true", + MCP_OAUTH_APPROVAL_KEY: "oauth-login-secret", MCP_PUBLIC_URL: baseUrl, MCP_OAUTH_STATE_FILE: stateFile, MCP_HOST: "127.0.0.1", @@ -107,9 +109,54 @@ describe("OAuth 2.1 MCP authorization", () => { registration_endpoint: `${baseUrl}/register`, code_challenge_methods_supported: ["S256"], token_endpoint_auth_methods_supported: expect.arrayContaining(["none"]), + revocation_endpoint_auth_methods_supported: expect.arrayContaining(["none"]), }); const redirectUri = "https://chatgpt.com/connector/oauth/test-callback"; + for (const invalidMetadata of [ + { + redirect_uris: ["http://attacker.example/callback"], + token_endpoint_auth_method: "none", + grant_types: ["authorization_code"], + response_types: ["code"], + }, + { + redirect_uris: [redirectUri], + token_endpoint_auth_method: "private_key_jwt", + grant_types: ["authorization_code"], + response_types: ["code"], + }, + { + redirect_uris: [], + token_endpoint_auth_method: "none", + grant_types: ["authorization_code"], + response_types: ["code"], + }, + ]) { + const invalidRegistration = await fetch(`${baseUrl}/register`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(invalidMetadata), + }); + expect(invalidRegistration.status).toBe(400); + expect(await invalidRegistration.json()).toMatchObject({ + error: "invalid_client_metadata", + }); + } + + const defaultedRegistration = await fetch(`${baseUrl}/register`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ redirect_uris: [redirectUri] }), + }); + expect(defaultedRegistration.status).toBe(201); + expect(await defaultedRegistration.json()).toMatchObject({ + token_endpoint_auth_method: "client_secret_post", + grant_types: ["authorization_code"], + response_types: ["code"], + client_secret: expect.any(String), + }); + const registrationResponse = await fetch(`${baseUrl}/register`, { method: "POST", headers: { "content-type": "application/json" }, @@ -143,9 +190,7 @@ describe("OAuth 2.1 MCP authorization", () => { redirect: "manual", }); expect(loginPage.status).toBe(200); - expect(loginPage.headers.get("content-security-policy")).toContain( - "form-action 'self' https://chatgpt.com", - ); + expect(loginPage.headers.get("content-security-policy")).toContain("form-action 'self'"); expect(await loginPage.text()).toContain("MCP 인증키"); const rejectedLogin = await fetch(`${baseUrl}/authorize`, { @@ -162,7 +207,7 @@ describe("OAuth 2.1 MCP authorization", () => { body: form({ ...authorizationValues, access_key: "oauth-login-secret" }), redirect: "manual", }); - expect(approvedLogin.status).toBe(302); + expect(approvedLogin.status).toBe(303); const callback = new URL(approvedLogin.headers.get("location")!); expect(callback.origin + callback.pathname).toBe(redirectUri); expect(callback.searchParams.get("state")).toBe("oauth-test-state"); @@ -224,6 +269,35 @@ describe("OAuth 2.1 MCP authorization", () => { expect(refreshed.access_token).not.toBe(tokens.access_token); expect(refreshed.refresh_token).not.toBe(tokens.refresh_token); + await running.close(); + running = await startHttpServer(config, createServices(config)); + + const replayedRefresh = await fetch(`${baseUrl}/token`, { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded" }, + body: form({ + grant_type: "refresh_token", + client_id: registered.client_id, + refresh_token: tokens.refresh_token, + resource: resourceUrl, + }), + }); + expect(replayedRefresh.status).toBe(400); + expect(await replayedRefresh.json()).toMatchObject({ error: "invalid_grant" }); + + const revokedSuccessor = await fetch(`${baseUrl}/token`, { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded" }, + body: form({ + grant_type: "refresh_token", + client_id: registered.client_id, + refresh_token: refreshed.refresh_token, + resource: resourceUrl, + }), + }); + expect(revokedSuccessor.status).toBe(400); + expect(await revokedSuccessor.json()).toMatchObject({ error: "invalid_grant" }); + const revokeResponse = await fetch(`${baseUrl}/revoke`, { method: "POST", headers: { "content-type": "application/x-www-form-urlencoded" }, @@ -242,6 +316,7 @@ describe("OAuth 2.1 MCP authorization", () => { expect(revokedRequest.status).toBe(401); expect((await stat(stateFile)).mode & 0o777).toBe(0o600); + expect((await stat(path.dirname(stateFile))).mode & 0o777).toBe(0o755); const persisted = await readFile(stateFile, "utf8"); expect(persisted).not.toContain(tokens.access_token); expect(persisted).not.toContain(tokens.refresh_token); diff --git a/test/process-manager.test.ts b/test/process-manager.test.ts index cfa698b..2500193 100644 --- a/test/process-manager.test.ts +++ b/test/process-manager.test.ts @@ -76,4 +76,60 @@ describe("ProcessManager", () => { expect(result.timedOut).toBe(true); expect(result.error).toContain("timeout"); }); + + it("handles a rejected initial stdin write without crashing the server", async () => { + manager = createManager(); + const sessionId = manager.start({ + executable: "/bin/bash", + args: ["-c", "true"], + commandForDisplay: "true", + cwd: process.cwd(), + stdin: "x".repeat(1024 * 1024), + }); + + await manager.waitForExit(sessionId, 2000); + await new Promise((resolve) => setImmediate(resolve)); + const result = await manager.read(sessionId); + + expect(result.running).toBe(false); + expect(result.error).toMatch(/stdin write failed|EPIPE/i); + }); + + it("rejects a follow-up stdin write without emitting an unhandled error", async () => { + manager = createManager(); + const sessionId = manager.start({ + executable: "/bin/bash", + args: ["-c", "exec 0<&-; printf ready; sleep 2"], + commandForDisplay: "closed stdin", + cwd: process.cwd(), + }); + expect((await manager.read(sessionId, { waitMs: 1000 })).stdout).toContain("ready"); + + await expect(manager.write(sessionId, "x".repeat(1024 * 1024))).rejects.toThrow(); + await new Promise((resolve) => setImmediate(resolve)); + expect((await manager.read(sessionId)).error).toMatch(/stdin write failed|EPIPE/i); + }); + + it("preserves UTF-8 characters across paged process output", async () => { + manager = createManager(); + const expected = `${"a".repeat(16 * 1024 - 1)}😀B`; + const encoded = Buffer.from(expected).toString("base64"); + const sessionId = manager.start({ + executable: process.execPath, + args: ["-e", `process.stdout.write(Buffer.from(${JSON.stringify(encoded)}, "base64"))`], + commandForDisplay: "unicode output", + cwd: process.cwd(), + }); + + await manager.waitForExit(sessionId, 2000); + const first = await manager.read(sessionId, { maxOutputBytes: 16 * 1024 }); + const second = await manager.read(sessionId, { + afterSeq: first.nextSeq, + maxOutputBytes: 16 * 1024, + }); + + expect(first.hasMore).toBe(true); + expect(first.output + second.output).toBe(expected); + expect(first.output + second.output).not.toContain("�"); + }); });