From 424f69083bce9f7bb52d401a9f675675e563be51 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Thu, 30 Apr 2026 02:36:14 -0700 Subject: [PATCH] fix(security): require MCP auth before method dispatch --- docs/MCP_MODE.md | 10 +++--- src/OpenClaw.Shared/Mcp/McpHttpServer.cs | 31 ++++++++++--------- .../McpHttpServerTests.cs | 30 ++++++++++++++++++ 3 files changed, 51 insertions(+), 20 deletions(-) diff --git a/docs/MCP_MODE.md b/docs/MCP_MODE.md index 6392b1e..52bfedf 100644 --- a/docs/MCP_MODE.md +++ b/docs/MCP_MODE.md @@ -73,11 +73,11 @@ The bridge takes a `Func>` rather than a snapshot `OpenClaw.Shared/Mcp/McpHttpServer.cs` is `System.Net.HttpListener` bound to `http://127.0.0.1:8765/`. Loopback-only by construction; not reachable from any other machine even with firewall holes. A defensive `IPAddress.IsLoopback` check on each request acts as belt-and-suspenders. -`GET /` returns a friendly text probe. `POST /` is JSON-RPC. Anything else → `405`. +`GET /` returns a friendly text probe. `POST /` is JSON-RPC. Anything else → `405`. When a bearer token is configured, every verb must pass the token gate before method dispatch. ## Authentication -The HTTP transport requires a bearer token on every `POST`. Defense-in-depth on top of loopback bind + Origin/Host checks: if an attacker can run code in *any* local user context they can reach `127.0.0.1:8765`, so we don't want the listener to be open-by-construction. +The HTTP transport requires a bearer token on every request. Defense-in-depth on top of loopback bind + Origin/Host checks: if an attacker can run code in *any* local user context they can reach `127.0.0.1:8765`, so we don't want the listener to be open-by-construction. **Where the token lives.** `%APPDATA%\OpenClawTray\mcp-token.txt`. The exact path is composed by `NodeService.McpTokenPath` from `SettingsManager.SettingsDirectoryPath`, so the test-suite override `OPENCLAW_TRAY_DATA_DIR` isolates the token file too. The file inherits the parent directory's ACL — by default only the current user (and SYSTEM/Administrators) can read it. @@ -87,7 +87,7 @@ The HTTP transport requires a bearer token on every `POST`. Defense-in-depth on **Lifetime.** The token is **persistent across tray restarts**. It's only regenerated if the file is deleted or its contents are emptied. There is no automatic rotation. -**On the wire.** Every `POST /` must carry `Authorization: Bearer `. Missing or wrong token → `401 Unauthorized` with no body. `GET /` is unauthenticated (it's just a "yes I'm here" probe). +**On the wire.** Every request must carry `Authorization: Bearer ` when the server has a configured token. Missing or wrong token → `401 Unauthorized` with no body. `GET /` remains a "yes I'm here" probe after auth passes. **How users find it.** Settings → Developer Mode → MCP section shows the live token (masked, with Reveal/Copy buttons) and the storage path. For agents that read from disk (Claude Code, custom scripts), pointing them at `McpTokenPath` is preferable to embedding the token in their prompt or config — the path is stable, the token is a secret. For agents that only accept literal bearer values in config (Claude Desktop, Cursor), use Copy. @@ -218,8 +218,8 @@ curl -X POST http://127.0.0.1:8765/ -H "Content-Type: text/plain" --data '{"json These should **succeed**: ```powershell -curl http://127.0.0.1:8765/ # GET probe -curl -X POST http://127.0.0.1:8765/ -H "Content-Type: application/json" -d '{"jsonrpc":"2.0","id":1,"method":"ping"}' +curl http://127.0.0.1:8765/ -H "Authorization: Bearer " # GET probe +curl -X POST http://127.0.0.1:8765/ -H "Authorization: Bearer " -H "Content-Type: application/json" -d '{"jsonrpc":"2.0","id":1,"method":"ping"}' ``` ## What's deliberately deferred diff --git a/src/OpenClaw.Shared/Mcp/McpHttpServer.cs b/src/OpenClaw.Shared/Mcp/McpHttpServer.cs index d4a9690..b71225d 100644 --- a/src/OpenClaw.Shared/Mcp/McpHttpServer.cs +++ b/src/OpenClaw.Shared/Mcp/McpHttpServer.cs @@ -26,8 +26,8 @@ namespace OpenClaw.Shared.Mcp; /// satisfy (no Access-Control-Allow-Origin), so the cross-origin call /// fails before reaching capability code. /// -/// Bearer-token auth in front of the JSON-RPC body. Required on every POST when -/// constructed with a non-null token (the tray always passes one — see +/// Bearer-token auth in front of request dispatch. Required on every request +/// when constructed with a non-null token (the tray always passes one — see /// NodeService.McpTokenPath / McpAuthToken.LoadOrCreate; legacy /// callers that pass null disable the check, kept for in-process tests). The /// token defends against untrusted local processes that could otherwise reach @@ -66,9 +66,9 @@ public sealed class McpHttpServer : IDisposable private readonly IOpenClawLogger _logger; private readonly HttpListener _listener; /// - /// Required bearer token for POST requests. Empty/null disables auth (the + /// Required bearer token for HTTP requests. Empty/null disables auth (the /// pre-auth contract — kept so existing dev configs keep working). When set, - /// every POST must carry Authorization: Bearer <token>. + /// every request must carry Authorization: Bearer <token>. /// private string? _authToken; private readonly CancellationTokenSource _cts = new(); @@ -227,6 +227,18 @@ public sealed class McpHttpServer : IDisposable return; } + // Bearer-token check. Defends against untrusted local processes + // (browser helpers, editor extensions) that share the loopback + // surface with the legitimate MCP client. Token lives in a + // user-only-readable file under %LOCALAPPDATA%; CLI/agent + // registration reads from there. Keep this before method dispatch + // so alternate verbs cannot bypass the configured token gate. + if (authToken != null && !IsAuthorized(authToken, ctx.Request.Headers["Authorization"])) + { + Reject(ctx, HttpStatusCode.Unauthorized, "missing or invalid bearer token"); + return; + } + if (ctx.Request.HttpMethod == "GET") { // Friendly probe response — useful for confirming the server is up @@ -242,17 +254,6 @@ public sealed class McpHttpServer : IDisposable return; } - // Bearer-token check. Defends against untrusted local processes - // (browser helpers, editor extensions) that share the loopback - // surface with the legitimate MCP client. Token lives in a - // user-only-readable file under %LOCALAPPDATA%; CLI/agent - // registration reads from there. - if (authToken != null && !IsAuthorized(authToken, ctx.Request.Headers["Authorization"])) - { - Reject(ctx, HttpStatusCode.Unauthorized, "missing or invalid bearer token"); - return; - } - // Force application/json on POST. Combined with the Origin check, // this means a browser cross-origin fetch must use a non-simple // Content-Type and trigger a CORS preflight, which we don't honor. diff --git a/tests/OpenClaw.Shared.Tests/McpHttpServerTests.cs b/tests/OpenClaw.Shared.Tests/McpHttpServerTests.cs index e7c4f68..04c89a6 100644 --- a/tests/OpenClaw.Shared.Tests/McpHttpServerTests.cs +++ b/tests/OpenClaw.Shared.Tests/McpHttpServerTests.cs @@ -415,6 +415,36 @@ public class McpHttpServerTests Assert.Equal(System.Net.HttpStatusCode.OK, resp3.StatusCode); } + [Fact] + public async Task Auth_RequiresBearerToken_BeforeMethodDispatch_When_TokenConfigured() + { + const string token = "supersecret"; + var port = FreePort(); + var bridge = new McpToolBridge(() => new INodeCapability[] { new FakeCapability() }); + using var server = new McpHttpServer(bridge, port, NullLogger.Instance, token); + server.Start(); + + using var http = new HttpClient { BaseAddress = new Uri($"http://127.0.0.1:{port}/") }; + + var unauthenticatedGet = await http.GetAsync(""); + Assert.Equal(System.Net.HttpStatusCode.Unauthorized, unauthenticatedGet.StatusCode); + + var unauthenticatedPut = await http.PutAsync( + "", + new StringContent("{}", System.Text.Encoding.UTF8, "application/json")); + Assert.Equal(System.Net.HttpStatusCode.Unauthorized, unauthenticatedPut.StatusCode); + + http.DefaultRequestHeaders.Authorization = new System.Net.Http.Headers.AuthenticationHeaderValue("Bearer", token); + + var authorizedGet = await http.GetAsync(""); + Assert.Equal(System.Net.HttpStatusCode.OK, authorizedGet.StatusCode); + + var authorizedPut = await http.PutAsync( + "", + new StringContent("{}", System.Text.Encoding.UTF8, "application/json")); + Assert.Equal(System.Net.HttpStatusCode.MethodNotAllowed, authorizedPut.StatusCode); + } + [Fact] public async Task Auth_NoToken_AllowsAllRequests_LegacyDevMode() {