From 1e096c86c184bea4b883ea894cbddc62f7f437f7 Mon Sep 17 00:00:00 2001 From: Fernando Date: Wed, 16 Sep 2026 11:54:30 -0400 Subject: [PATCH 1/2] =?UTF-8?q?fix(#984):=20harden=20the=20opt-in=20HTTP?= =?UTF-8?q?=20API=20=E2=80=94=20POST-only=20tool=20dispatch,=20optional=20?= =?UTF-8?q?token,=20loopback=20bind?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The --http-port REST surface can run EVERY tool, mutating ones included, and it (1) executed any tool from a bare `GET /api/tools/` with no arguments, (2) had no authentication, and (3) bound QHostAddress::Any — so despite being described as a localhost API, anything on the LAN could `GET …/decimate_mesh`. - GET /api/tools/ now answers 405 + `Allow: POST` and dispatches nothing; `GET /api/tools` still lists; tools execute only via POST. - Optional shared secret: `--http-token `, else `QTMESH_HTTP_TOKEN`, else QSettings `mcp/httpToken`. When set, every request except the CORS preflight must carry `Authorization: Bearer ` or `X-Api-Key: `, else 401 + WWW-Authenticate. Constant-time compare (`httpRequestAuthorized`, pure + unit-tested). No token configured → unchanged open local access, so the scripts/anim-*.sh harnesses (all POST + a GET-list probe) run as before. - Bind loopback by default; `--http-bind ` / `QTMESH_HTTP_BIND` opt into 0.0.0.0 (containers). Non-loopback without a token logs a warning. - One JSON writer for every reply so status/CORS/framing cannot drift. Also fixes a latent bug this surfaced: `AppLaunchHandler::collectGuiLaunchPaths` checked `startsWith('-')` BEFORE the GUI-flag branch, so the `++i` that skips a flag's VALUE was dead code — `--http-port 8080` only appeared to work because "8080" is not an importable file. A `--http-token` naming an existing mesh would have been opened in the editor. The flag branch now runs first; the new test uses a real .obj as the value so the skip is observable. Verified: 76 MCPServerHttp*/AppLaunchHandler* tests pass; mutation check — a length-only token compare is killed by WrongTokenIs401 and the pure parse test (same-length wrong token) while the correct-token test still passes; e2e on the real `--mcp` binary with curl: 401 / 401 wrong / 200 Bearer / 200 X-Api-Key / 405 GET / 204 preflight, lsof shows 127.0.0.1 by default and *:port only with --http-bind 0.0.0.0. Docs: README, CLAUDE.md (Build Commands + MCP Server), website REST card. Fixes #984 Co-Authored-By: Claude Fable 5.1 --- CLAUDE.md | 4 +- README.md | 2 +- src/AppLaunchHandler.cpp | 19 ++- src/AppLaunchHandler_coverage_test.cpp | 34 ++++- src/MCPServer.cpp | 136 ++++++++++++++++-- src/MCPServer.h | 34 +++++ src/MCPServer_test.cpp | 191 ++++++++++++++++++++++++- src/main.cpp | 21 +++ website/src/data/content.js | 2 +- 9 files changed, 420 insertions(+), 23 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index a709fcde3..56bd25ccf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -31,7 +31,8 @@ cmake --build build_local --target UnitTests -j4 ```bash ./build_local/bin/QtMeshEditor.app/Contents/MacOS/QtMeshEditor --with-mcp # GUI + MCP ./build_local/bin/QtMeshEditor.app/Contents/MacOS/QtMeshEditor --mcp # headless MCP only -./build_local/bin/QtMeshEditor.app/Contents/MacOS/QtMeshEditor --with-mcp --http-port 8080 # with HTTP API +./build_local/bin/QtMeshEditor.app/Contents/MacOS/QtMeshEditor --with-mcp --http-port 8080 # with HTTP API (loopback-only; POST /api/tools/ executes, GET lists) +./build_local/bin/QtMeshEditor.app/Contents/MacOS/QtMeshEditor --with-mcp --http-port 8080 --http-token s3cret --http-bind 0.0.0.0 # #984: require `Authorization: Bearer s3cret` (or X-Api-Key), expose beyond localhost ``` **CLI pipeline (`qtmesh`):** @@ -368,6 +369,7 @@ The animation pipeline started skeleton-only; the #517 epic broadens it. Slices - Launch modes: `--mcp` (headless), `--with-mcp` (GUI + MCP). - stdout is redirected to stderr to isolate MCP JSON-RPC from Ogre/Qt debug output; original stdout fd saved for MCP responses. - HTTP API uses QTcpServer with deferred tool execution (QTimer::singleShot) to avoid re-entrant crashes from Ogre event processing. +- **HTTP API hardening (#984)**: the REST surface can run EVERY tool, mutating ones included, so (1) it binds **loopback** by default — `--http-bind ` / `QTMESH_HTTP_BIND` opt into `0.0.0.0` (containers with a mapped port), and a non-loopback bind without a token logs a warning; (2) tools execute **only via `POST /api/tools/`** — `GET /api/tools/` answers `405` + `Allow: POST` and dispatches nothing (it used to run the tool with no arguments, so any link/prefetch could decimate a mesh); `GET /api/tools` still lists; (3) an **optional shared secret** — `--http-token `, else `QTMESH_HTTP_TOKEN`, else QSettings `mcp/httpToken` (`MCPServer::resolveHttpToken`) — when set, every request except the CORS preflight (which cannot carry credentials by spec) must send `Authorization: Bearer ` or `X-Api-Key: `, else `401` + `WWW-Authenticate`; the compare is constant-time (`httpRequestAuthorized`, pure + unit-tested). Default (no token) stays open to local processes, so the `scripts/anim-*.sh` harnesses (all POST, plus a `GET /api/tools` liveness probe) run unchanged. `AppLaunchHandler::isGuiModeValueFlag` skips the `--http-token`/`--http-bind` VALUES so a token never becomes a launch path — and that skip is now checked BEFORE the generic leading-`-` `continue` in `collectGuiLaunchPaths`; it used to sit after it, i.e. was dead code, and `--http-port 8080` only appeared to work because "8080" is not an importable file (the test uses a real `.obj` as the value so the skip is observable). - **`take_screenshot` captures via an Ogre RTT, NOT `QWidget::grab()`** — Ogre renders straight to the native window surface (`WA_PaintOnScreen`) so `grab()` returns a black buffer. The tool renders the active viewport's `SpaceCamera` camera into an offscreen `PF_BYTE_RGBA` render target (RTSS `MSN_SHADERGEN` scheme + a temporary ambient boost & directional key light so imported materials aren't black, restored after) and reads it back to PNG. `load_mesh` calls `frameSceneInActiveViewport()` (select every user node → `frameSelection()`) so a headless `load_mesh`→`take_screenshot` actually frames + shows the mesh. This makes autonomous visual QA (load → optionally explode via `transform_submesh` → screenshot) work without a GUI operator. ### CLI Pipeline diff --git a/README.md b/README.md index 966c7c570..95fcb841a 100755 --- a/README.md +++ b/README.md @@ -320,7 +320,7 @@ Split View|Skeleton Animation Controls - **Performance capture** — video/webcam → facial morph animation (ARKit blendshapes), head pose, and full-body skeletal capture onto humanoid rigs; live preview + record in the editor, `qtmesh mocap` on the CLI (`-DENABLE_MOCAP` builds) - **AI chat** — natural language scene editing via local LLMs - **MCP server** — 57+ tools for AI agents (Claude, Cursor, etc.), including HDR/IBL (`set_hdr_environment`, `set_tonemap`, …) and QtMesh Cloud (`cloud_*`) -- **REST API** — HTTP interface for external automation +- **REST API** — opt-in HTTP interface for external automation (`--with-mcp --http-port 8080`). Bound to **localhost** by default; tools execute only via `POST /api/tools/` (`GET /api/tools` lists them); set `QTMESH_HTTP_TOKEN` (or `--http-token`) to require `Authorization: Bearer ` on every request, and `--http-bind 0.0.0.0` only when you mean to expose it --- diff --git a/src/AppLaunchHandler.cpp b/src/AppLaunchHandler.cpp index 8edb1898f..0df80caad 100644 --- a/src/AppLaunchHandler.cpp +++ b/src/AppLaunchHandler.cpp @@ -38,10 +38,17 @@ bool isCliSubcommand(const QString& arg) return kSubcommands.contains(arg); } +// Flags that take a value — the value must not be mistaken for a launch path. +bool isGuiModeValueFlag(const QString& arg) +{ + return arg == QStringLiteral("--http-port") || arg == QStringLiteral("--http-token") + || arg == QStringLiteral("--http-bind"); +} + bool isGuiModeFlag(const QString& arg) { return arg == QStringLiteral("--mcp") || arg == QStringLiteral("-mcp") - || arg == QStringLiteral("--with-mcp") || arg == QStringLiteral("--http-port"); + || arg == QStringLiteral("--with-mcp") || isGuiModeValueFlag(arg); } } // namespace @@ -109,13 +116,17 @@ QStringList AppLaunchHandler::collectGuiLaunchPaths(const QStringList& arguments QStringList paths; for (int i = 1; i < arguments.size(); ++i) { const QString& arg = arguments.at(i); - if (arg.startsWith(QLatin1Char('-'))) - continue; + // Value-taking flags FIRST: the generic '-' skip below used to run + // before this branch, so the ++i never executed and the VALUE was + // examined as a launch path (#984 — a --http-token naming an existing + // mesh would have been opened in the editor). if (isGuiModeFlag(arg)) { - if (arg == QStringLiteral("--http-port") && i + 1 < arguments.size()) + if (isGuiModeValueFlag(arg) && i + 1 < arguments.size()) ++i; continue; } + if (arg.startsWith(QLatin1Char('-'))) + continue; if (isCliSubcommand(arg)) break; diff --git a/src/AppLaunchHandler_coverage_test.cpp b/src/AppLaunchHandler_coverage_test.cpp index c38626160..6ae254198 100644 --- a/src/AppLaunchHandler_coverage_test.cpp +++ b/src/AppLaunchHandler_coverage_test.cpp @@ -223,8 +223,10 @@ TEST(AppLaunchHandlerCoverageTest, DefaultImportExtensions_NotEmpty) TEST(AppLaunchHandlerCoverageTest, Collect_HttpPortSkipsPortNumber) { // "--http-port" is a GUI-mode flag; the numeric arg right after must be - // consumed by the ++i advance and never treated as a path. The port number - // also is not importable, but the ++i guarantees it's skipped entirely. + // consumed by the ++i advance and never treated as a path. NB a port number + // is not importable anyway, so this case alone cannot tell the skip from a + // no-op — Collect_HttpTokenAndBindValuesAreSkipped uses a REAL file as the + // value for that (it caught the skip being dead code, #984). const QStringList args = { QStringLiteral("QtMeshEditor"), QStringLiteral("--http-port"), @@ -243,6 +245,34 @@ TEST(AppLaunchHandlerCoverageTest, Collect_HttpPortAtEndNoFollowingArg) EXPECT_TRUE(AppLaunchHandler::collectGuiLaunchPaths(args).isEmpty()); } +TEST(AppLaunchHandlerCoverageTest, Collect_HttpTokenAndBindValuesAreSkipped) +{ + // #984: --http-token / --http-bind take a value like --http-port does. The + // value is skipped even when it names a REAL importable file — otherwise a + // token that happens to look like a path would be opened in the editor. + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString decoy = dir.filePath(QStringLiteral("token.obj")); + QFile obj(decoy); + ASSERT_TRUE(obj.open(QIODevice::WriteOnly)); + obj.write("v 0 0 0\n"); + obj.close(); + + const QStringList args = { + QStringLiteral("QtMeshEditor"), + QStringLiteral("--with-mcp"), + QStringLiteral("--http-token"), decoy, + QStringLiteral("--http-bind"), QStringLiteral("0.0.0.0"), + }; + EXPECT_TRUE(AppLaunchHandler::collectGuiLaunchPaths(args).isEmpty()); + + // …while a real file AFTER the value pairs is still collected. + const QStringList withFile = args + QStringList{decoy}; + const QStringList paths = AppLaunchHandler::collectGuiLaunchPaths(withFile); + ASSERT_EQ(paths.size(), 1); + EXPECT_EQ(paths.front(), QFileInfo(decoy).absoluteFilePath()); +} + TEST(AppLaunchHandlerCoverageTest, Collect_HttpPortThenRealFileStillCollected) { QTemporaryDir dir; diff --git a/src/MCPServer.cpp b/src/MCPServer.cpp index 498d361bc..244db9802 100644 --- a/src/MCPServer.cpp +++ b/src/MCPServer.cpp @@ -1,4 +1,5 @@ #include "MCPServer.h" +#include #include "ImageTo3D/BackgroundRemover.h" #include "PhotoDepth.h" #include "TextureInpaint.h" @@ -12889,15 +12890,120 @@ QJsonObject MCPServer::buildToolDefinition(const QString &name, const QString &d // HTTP REST API +namespace { + +// Constant-time byte comparison so a token check does not leak how many +// leading bytes matched through response timing. +bool constantTimeEquals(const QByteArray &a, const QByteArray &b) +{ + if (a.size() != b.size()) return false; + unsigned char diff = 0; + for (int i = 0; i < a.size(); ++i) + diff |= static_cast(a[i]) ^ static_cast(b[i]); + return diff == 0; +} + +// One writer for every JSON reply so status line, CORS and framing cannot +// drift between the branches. +void writeHttpJson(QTcpSocket *socket, int status, const char *reason, + const QJsonObject &json, const QByteArray &extraHeaders = {}) +{ + const QByteArray body = QJsonDocument(json).toJson(QJsonDocument::Compact); + QByteArray resp; + resp.append(QStringLiteral("HTTP/1.1 %1 %2\r\n").arg(status).arg(QLatin1String(reason)).toUtf8()); + resp.append("Content-Type: application/json\r\n"); + resp.append("Access-Control-Allow-Origin: *\r\n"); + resp.append(extraHeaders); + resp.append("Connection: close\r\n"); + resp.append(QStringLiteral("Content-Length: %1\r\n").arg(body.size()).toUtf8()); + resp.append("\r\n"); + resp.append(body); + socket->write(resp); + socket->flush(); + socket->deleteLater(); +} + +} // namespace + +void MCPServer::setHttpToken(const QString &token) +{ + m_httpToken = token; + m_httpTokenExplicit = true; +} + +QString MCPServer::httpToken() const { return m_httpToken; } + +void MCPServer::setHttpBindAddress(const QHostAddress &address) +{ + m_httpBindAddress = address; + m_httpBindExplicit = true; +} + +QHostAddress MCPServer::httpBindAddress() const +{ + return m_httpServer ? m_httpServer->serverAddress() : m_httpBindAddress; +} + +QString MCPServer::resolveHttpToken() +{ + const QByteArray env = qgetenv("QTMESH_HTTP_TOKEN"); + if (!env.trimmed().isEmpty()) return QString::fromUtf8(env.trimmed()); + QSettings settings; + return settings.value(QStringLiteral("mcp/httpToken")).toString().trimmed(); +} + +QHostAddress MCPServer::resolveHttpBindAddress() +{ + const QString env = QString::fromUtf8(qgetenv("QTMESH_HTTP_BIND")).trimmed(); + if (env.isEmpty()) return QHostAddress(QHostAddress::LocalHost); + QHostAddress addr(env); + if (addr.isNull()) { + qWarning() << "QTMESH_HTTP_BIND" << env << "is not a valid address — binding loopback"; + return QHostAddress(QHostAddress::LocalHost); + } + return addr; +} + +bool MCPServer::httpRequestAuthorized(const QString &headerBlock, const QString &token) +{ + if (token.isEmpty()) return false; + const QByteArray expected = token.toUtf8(); + for (const QString &line : headerBlock.split(QStringLiteral("\r\n"))) { + const int colon = line.indexOf(QLatin1Char(':')); + if (colon <= 0) continue; + const QString name = line.left(colon).trimmed(); + const QString value = line.mid(colon + 1).trimmed(); + if (name.compare(QLatin1String("Authorization"), Qt::CaseInsensitive) == 0) { + const int space = value.indexOf(QLatin1Char(' ')); + if (space <= 0) continue; + if (value.left(space).compare(QLatin1String("Bearer"), Qt::CaseInsensitive) != 0) continue; + if (constantTimeEquals(value.mid(space + 1).trimmed().toUtf8(), expected)) return true; + } else if (name.compare(QLatin1String("X-Api-Key"), Qt::CaseInsensitive) == 0) { + if (constantTimeEquals(value.toUtf8(), expected)) return true; + } + } + return false; +} + bool MCPServer::startHttp(int port) { m_httpPort = port; + if (!m_httpTokenExplicit) m_httpToken = resolveHttpToken(); + if (!m_httpBindExplicit) m_httpBindAddress = resolveHttpBindAddress(); m_httpServer = new QTcpServer(this); connect(m_httpServer, &QTcpServer::newConnection, this, &MCPServer::onHttpConnection); - if (m_httpServer->listen(QHostAddress::Any, m_httpPort)) { + if (m_httpServer->listen(m_httpBindAddress, m_httpPort)) { m_httpPort = m_httpServer->serverPort(); // update to actual port (important when port=0) - qDebug() << "HTTP REST API listening on port" << m_httpPort; + qDebug() << "HTTP REST API listening on" << m_httpServer->serverAddress().toString() + << "port" << m_httpPort + << (m_httpToken.isEmpty() ? "(no token — open to any local process)" + : "(token required)"); + if (!m_httpServer->serverAddress().isLoopback() && m_httpToken.isEmpty()) { + qWarning() << "HTTP REST API is bound to a non-loopback address WITHOUT a token: " + "anyone who can reach this port can run every tool. Set " + "QTMESH_HTTP_TOKEN (or --http-token)."; + } return true; } else { qWarning() << "Failed to start HTTP server on port" << m_httpPort @@ -12973,7 +13079,7 @@ void MCPServer::handleHttpRequest(QTcpSocket *socket) QByteArray resp = "HTTP/1.1 204 No Content\r\n" "Access-Control-Allow-Origin: *\r\n" "Access-Control-Allow-Methods: GET, POST, OPTIONS\r\n" - "Access-Control-Allow-Headers: Content-Type\r\n" + "Access-Control-Allow-Headers: Content-Type, Authorization, X-Api-Key\r\n" "Connection: close\r\n\r\n"; socket->write(resp); socket->flush(); @@ -12981,6 +13087,16 @@ void MCPServer::handleHttpRequest(QTcpSocket *socket) return; } + // #984: when a token is configured, every request past the preflight must + // present it — listing included, since the surface itself is information. + if (!m_httpToken.isEmpty() && !httpRequestAuthorized(headers, m_httpToken)) { + writeHttpJson(socket, 401, "Unauthorized", + QJsonObject{{"error", "Unauthorized: send the configured token as " + "'Authorization: Bearer ' or 'X-Api-Key: '"}}, + "WWW-Authenticate: Bearer realm=\"qtmesh\"\r\n"); + return; + } + // Parse route and arguments, then defer execution via QTimer::singleShot // so the socket event is fully processed before any tool runs. QString toolName; @@ -13010,15 +13126,19 @@ void MCPServer::handleHttpRequest(QTcpSocket *socket) } } } - // Route: GET /api/tools/:name - call tool with no args + // Route: GET /api/tools/:name — REFUSED (#984). A GET used to execute the + // tool with no arguments, so any link, prefetcher or could + // decimate a mesh or delete a light. Tools run only via POST. else if (method == "GET" && path.startsWith("/api/tools/")) { - toolName = path.mid(11); - int qmark = toolName.indexOf('?'); - if (qmark >= 0) toolName = toolName.left(qmark); + writeHttpJson(socket, 405, "Method Not Allowed", + QJsonObject{{"error", "Tools are executed with POST /api/tools/ " + "(a JSON body carries the arguments); GET does not run tools."}}, + "Allow: POST\r\n"); + return; } else { httpStatus = 404; - responseJson["error"] = "Not found. Use GET /api/tools or POST /api/tools/"; + responseJson["error"] = "Not found. Use GET /api/tools (list) or POST /api/tools/ (execute)"; } // If we need to call a tool, defer it so socket events are fully drained first diff --git a/src/MCPServer.h b/src/MCPServer.h index 6176cd39b..529483737 100644 --- a/src/MCPServer.h +++ b/src/MCPServer.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -91,6 +92,35 @@ class MCPServer : public QObject */ int httpPort() const; + // ---- HTTP API hardening (#984) ---- + // The HTTP API can drive EVERY tool, mutating ones included, so it is + // loopback-only and (optionally) token-protected. Tools execute only via + // POST /api/tools/; a GET there is answered 405 and runs nothing. + + /** Shared secret for the HTTP API. Empty = no authentication (the + * default). When set, every /api request except the CORS preflight must + * carry `Authorization: Bearer ` or `X-Api-Key: `, else 401. + * An explicit value wins over the env/QSettings resolution done by + * startHttp(). */ + void setHttpToken(const QString &token); + QString httpToken() const; + + /** Interface the HTTP server binds. Default is loopback (127.0.0.1); + * pass QHostAddress::Any to expose it (e.g. inside a container with a + * mapped port) — then a token is strongly advised. */ + void setHttpBindAddress(const QHostAddress &address); + QHostAddress httpBindAddress() const; + + /** Token from `QTMESH_HTTP_TOKEN`, else QSettings `mcp/httpToken`, else empty. */ + static QString resolveHttpToken(); + /** Bind address from `QTMESH_HTTP_BIND` (e.g. "0.0.0.0"), else loopback. */ + static QHostAddress resolveHttpBindAddress(); + /** True when the raw request header block carries `token` as a Bearer + * Authorization or X-Api-Key value (header names and the Bearer scheme are + * case-insensitive; the comparison is constant-time). An empty `token` + * never authorizes. Pure — unit-tested without a socket. */ + static bool httpRequestAuthorized(const QString &headerBlock, const QString &token); + /** * @brief Check if server is running */ @@ -517,6 +547,10 @@ private slots: void handleHttpRequest(QTcpSocket *socket); QTcpServer *m_httpServer = nullptr; int m_httpPort = 8080; + QString m_httpToken; + bool m_httpTokenExplicit = false; + QHostAddress m_httpBindAddress = QHostAddress::LocalHost; + bool m_httpBindExplicit = false; QMap m_httpBuffers; // Member variables diff --git a/src/MCPServer_test.cpp b/src/MCPServer_test.cpp index dd7cd4bd6..ae478ac89 100644 --- a/src/MCPServer_test.cpp +++ b/src/MCPServer_test.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -1962,6 +1963,11 @@ class MCPServerHttpTest : public ::testing::Test // HTTP tests only need MCPServer — not full Ogre. // Tool calls that need Ogre will return errors, which is fine for // testing the HTTP routing and response handling. + // #984: the token/bind resolution reads the environment + QSettings — + // scrub both so a developer's shell cannot flip the open-access cases. + qunsetenv("QTMESH_HTTP_TOKEN"); + qunsetenv("QTMESH_HTTP_BIND"); + QSettings().remove(QStringLiteral("mcp/httpToken")); server = std::make_unique(); } @@ -2118,22 +2124,31 @@ TEST_F(MCPServerHttpTest, PostToolCall) EXPECT_TRUE(response.contains("application/json")); } -// --- HTTP GET /api/tools/ (no body) --- +// --- HTTP GET /api/tools/ — refused (#984) --- -TEST_F(MCPServerHttpTest, GetToolCallNoBody) +TEST_F(MCPServerHttpTest, GetToolCallIsRefusedWith405AndRunsNothing) { server->setOgreInitFailed(true); ASSERT_TRUE(server->startHttp(0)); int port = server->httpPort(); + // Before #984 this executed list_materials with no arguments — and would + // equally have executed decimate_mesh or delete_light from a bare link. QByteArray request = "GET /api/tools/list_materials HTTP/1.1\r\n" "Host: 127.0.0.1\r\nConnection: close\r\n\r\n"; QByteArray response = sendHttpRequest(port, request); ASSERT_FALSE(response.isEmpty()); - EXPECT_EQ(getHttpStatus(response), 200); + EXPECT_EQ(getHttpStatus(response), 405); + EXPECT_TRUE(response.contains("Allow: POST")); + const QJsonObject json = parseHttpResponse(response); + EXPECT_TRUE(json.contains("error")); + EXPECT_TRUE(json["error"].toString().contains("POST")); + // Nothing was dispatched: no tool result shape (content/isError) leaks out. + EXPECT_FALSE(json.contains("content")); + EXPECT_FALSE(json.contains("isError")); } // --- HTTP OPTIONS (CORS preflight) --- @@ -2238,13 +2253,14 @@ TEST_F(MCPServerHttpTest, GetToolCallWithQueryStringStripsSuffix) ASSERT_TRUE(server->startHttp(0)); int port = server->httpPort(); + // A query string does not turn a GET into an execution either (#984). QByteArray request = "GET /api/tools/list_materials?format=json HTTP/1.1\r\n" "Host: 127.0.0.1\r\nConnection: close\r\n\r\n"; QByteArray response = sendHttpRequest(port, request); ASSERT_FALSE(response.isEmpty()); - EXPECT_EQ(getHttpStatus(response), 200); + EXPECT_EQ(getHttpStatus(response), 405); EXPECT_TRUE(response.contains("application/json")); } @@ -2256,8 +2272,8 @@ TEST_F(MCPServerHttpTest, BusyToolRequestReturns503) server->m_httpBusy = true; int port = server->httpPort(); - QByteArray request = "GET /api/tools/list_materials HTTP/1.1\r\n" - "Host: 127.0.0.1\r\nConnection: close\r\n\r\n"; + QByteArray request = "POST /api/tools/list_materials HTTP/1.1\r\n" + "Host: 127.0.0.1\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"; QByteArray response = sendHttpRequest(port, request); @@ -4558,6 +4574,169 @@ TEST_F(MCPServerHttpTest, StartHttp_PortAlreadyInUseReturnsFalse) EXPECT_EQ(server->m_httpServer, nullptr); } +// ========================================================================== +// #984 — HTTP API hardening: token + bind address +// ========================================================================== + +namespace { +QByteArray postListMaterials(const QByteArray &extraHeaders = {}) +{ + return "POST /api/tools/list_materials HTTP/1.1\r\nHost: 127.0.0.1\r\n" + + extraHeaders + "Content-Length: 0\r\nConnection: close\r\n\r\n"; +} +} // namespace + +TEST_F(MCPServerHttpTest, TokenConfigured_PostWithoutTokenIs401) +{ + server->setOgreInitFailed(true); + server->setHttpToken("s3cret-token"); + ASSERT_TRUE(server->startHttp(0)); + + const QByteArray response = sendHttpRequest(server->httpPort(), postListMaterials()); + ASSERT_FALSE(response.isEmpty()); + EXPECT_EQ(getHttpStatus(response), 401); + EXPECT_TRUE(response.contains("WWW-Authenticate: Bearer")); + EXPECT_FALSE(parseHttpResponse(response).contains("content")); // tool did not run +} + +TEST_F(MCPServerHttpTest, TokenConfigured_WrongTokenIs401) +{ + server->setOgreInitFailed(true); + server->setHttpToken("s3cret-token"); + ASSERT_TRUE(server->startHttp(0)); + + // Same length as the real token — the compare must not pass on length alone. + const QByteArray response = sendHttpRequest( + server->httpPort(), postListMaterials("Authorization: Bearer s3cret-tokeX\r\n")); + EXPECT_EQ(getHttpStatus(response), 401); +} + +TEST_F(MCPServerHttpTest, TokenConfigured_BearerTokenIsAccepted) +{ + server->setOgreInitFailed(true); + server->setHttpToken("s3cret-token"); + ASSERT_TRUE(server->startHttp(0)); + + const QByteArray response = sendHttpRequest( + server->httpPort(), postListMaterials("Authorization: Bearer s3cret-token\r\n")); + EXPECT_EQ(getHttpStatus(response), 200); + EXPECT_TRUE(parseHttpResponse(response).contains("content")); // reached the tool +} + +TEST_F(MCPServerHttpTest, TokenConfigured_XApiKeyIsAccepted) +{ + server->setOgreInitFailed(true); + server->setHttpToken("s3cret-token"); + ASSERT_TRUE(server->startHttp(0)); + + const QByteArray response = sendHttpRequest( + server->httpPort(), postListMaterials("x-api-key: s3cret-token\r\n")); + EXPECT_EQ(getHttpStatus(response), 200); +} + +TEST_F(MCPServerHttpTest, TokenConfigured_ListingRequiresTokenToo) +{ + server->setHttpToken("s3cret-token"); + ASSERT_TRUE(server->startHttp(0)); + + QByteArray response = sendHttpRequest(server->httpPort(), + "GET /api/tools HTTP/1.1\r\nHost: 127.0.0.1\r\nConnection: close\r\n\r\n"); + EXPECT_EQ(getHttpStatus(response), 401); + + response = sendHttpRequest(server->httpPort(), + "GET /api/tools HTTP/1.1\r\nHost: 127.0.0.1\r\n" + "Authorization: Bearer s3cret-token\r\nConnection: close\r\n\r\n"); + EXPECT_EQ(getHttpStatus(response), 200); + EXPECT_TRUE(parseHttpResponse(response).contains("tools")); +} + +TEST_F(MCPServerHttpTest, TokenConfigured_CorsPreflightNeedsNoTokenAndAdvertisesAuthHeaders) +{ + // A browser preflight cannot carry credentials by spec; refusing it would + // make the token unusable from any web client. + server->setHttpToken("s3cret-token"); + ASSERT_TRUE(server->startHttp(0)); + + const QByteArray response = sendHttpRequest(server->httpPort(), + "OPTIONS /api/tools/list_materials HTTP/1.1\r\nHost: 127.0.0.1\r\n" + "Origin: http://localhost\r\nConnection: close\r\n\r\n"); + EXPECT_EQ(getHttpStatus(response), 204); + EXPECT_TRUE(response.contains("Authorization")); + EXPECT_TRUE(response.contains("X-Api-Key")); +} + +TEST_F(MCPServerHttpTest, NoTokenConfigured_StaysOpenForLocalCallers) +{ + // Default behaviour is unchanged: no token → no 401 (existing harnesses). + server->setOgreInitFailed(true); + ASSERT_TRUE(server->startHttp(0)); + EXPECT_TRUE(server->httpToken().isEmpty()); + const QByteArray response = sendHttpRequest(server->httpPort(), postListMaterials()); + EXPECT_EQ(getHttpStatus(response), 200); +} + +TEST_F(MCPServerHttpTest, TokenResolution_EnvWinsOverSettings_ExplicitWinsOverBoth) +{ + QSettings().setValue(QStringLiteral("mcp/httpToken"), QStringLiteral("from-settings")); + EXPECT_EQ(MCPServer::resolveHttpToken(), QStringLiteral("from-settings")); + + qputenv("QTMESH_HTTP_TOKEN", "from-env "); + EXPECT_EQ(MCPServer::resolveHttpToken(), QStringLiteral("from-env")); // trimmed + + // startHttp() picks the resolved token up when none was set explicitly… + ASSERT_TRUE(server->startHttp(0)); + EXPECT_EQ(server->httpToken(), QStringLiteral("from-env")); + server->stopHttp(); + + // …and an explicit setter beats env + settings. + auto other = std::make_unique(); + other->setHttpToken("explicit"); + ASSERT_TRUE(other->startHttp(0)); + EXPECT_EQ(other->httpToken(), QStringLiteral("explicit")); + other->stopHttp(); + + qunsetenv("QTMESH_HTTP_TOKEN"); + QSettings().remove(QStringLiteral("mcp/httpToken")); + EXPECT_TRUE(MCPServer::resolveHttpToken().isEmpty()); +} + +TEST_F(MCPServerHttpTest, BindAddress_DefaultIsLoopback_AnyIsOptIn) +{ + ASSERT_TRUE(server->startHttp(0)); + EXPECT_TRUE(server->httpBindAddress().isLoopback()) + << server->httpBindAddress().toString().toStdString(); + server->stopHttp(); + + auto exposed = std::make_unique(); + exposed->setHttpBindAddress(QHostAddress::Any); + ASSERT_TRUE(exposed->startHttp(0)); + EXPECT_FALSE(exposed->httpBindAddress().isLoopback()); + exposed->stopHttp(); + + qputenv("QTMESH_HTTP_BIND", "not an address"); + EXPECT_TRUE(MCPServer::resolveHttpBindAddress().isLoopback()); // invalid → safe default + qputenv("QTMESH_HTTP_BIND", "0.0.0.0"); + EXPECT_FALSE(MCPServer::resolveHttpBindAddress().isLoopback()); + qunsetenv("QTMESH_HTTP_BIND"); +} + +TEST(MCPServerHttpAuthParse, BearerAndApiKeyParsing) +{ + using M = MCPServer; + EXPECT_TRUE (M::httpRequestAuthorized("Host: x\r\nAuthorization: Bearer abc\r\n", "abc")); + EXPECT_TRUE (M::httpRequestAuthorized("authorization: bearer abc \r\n", "abc")); // case + spaces + EXPECT_TRUE (M::httpRequestAuthorized("X-API-KEY: abc\r\n", "abc")); + EXPECT_FALSE(M::httpRequestAuthorized("Authorization: Basic abc\r\n", "abc")); // wrong scheme + EXPECT_FALSE(M::httpRequestAuthorized("Authorization: Bearer ab\r\n", "abc")); + EXPECT_FALSE(M::httpRequestAuthorized("Authorization: Bearer abcd\r\n", "abc")); + EXPECT_FALSE(M::httpRequestAuthorized("Authorization: Bearer abd\r\n", "abc")); // same length, wrong bytes + EXPECT_FALSE(M::httpRequestAuthorized("X-Api-Key: abd\r\n", "abc")); + EXPECT_FALSE(M::httpRequestAuthorized("Authorization: Bearer\r\n", "abc")); // no value + EXPECT_FALSE(M::httpRequestAuthorized("X-Api-Key: abc\r\n", "")); // empty token never authorizes + EXPECT_FALSE(M::httpRequestAuthorized("", "abc")); + EXPECT_FALSE(M::httpRequestAuthorized("Cookie: token=abc\r\n", "abc")); // other header +} + TEST_F(MCPServerProtocolTest, HandleResourcesReadCurrentMaterialUsesMaterialEditorTextWhenAvailable) { QMainWindow fakeWindow; diff --git a/src/main.cpp b/src/main.cpp index 977d8d07c..98e63b62c 100755 --- a/src/main.cpp +++ b/src/main.cpp @@ -33,6 +33,7 @@ #include "PropertiesPanelController.h" #include "ThemeManager.h" #include "MCPServer.h" +#include #include "SentryReporter.h" #include "CLIPipeline.h" #include "AppConsoleLog.h" @@ -171,6 +172,8 @@ int main(int argc, char *argv[]) bool mcpOnlyMode = false; bool mcpWithGuiMode = false; int httpPort = 8080; + QString httpToken; // #984: --http-token (else QTMESH_HTTP_TOKEN / QSettings) + QString httpBind; // #984: --http-bind (else QTMESH_HTTP_BIND / loopback) for (int i = 1; i < argc; ++i) { QString arg = QString(argv[i]); if (arg == "--mcp" || arg == "-mcp") { @@ -179,8 +182,24 @@ int main(int argc, char *argv[]) mcpWithGuiMode = true; } else if (arg == "--http-port" && i + 1 < argc) { httpPort = QString(argv[++i]).toInt(); + } else if (arg == "--http-token" && i + 1 < argc) { + httpToken = QString(argv[++i]); + } else if (arg == "--http-bind" && i + 1 < argc) { + httpBind = QString(argv[++i]); } } + // Applies the CLI overrides before startHttp(); anything not given falls + // back to the env/QSettings resolution inside MCPServer. + auto configureHttp = [&](MCPServer &server) { + if (!httpToken.isEmpty()) server.setHttpToken(httpToken); + if (!httpBind.isEmpty()) { + const QHostAddress addr(httpBind); + if (addr.isNull()) + qWarning() << "--http-bind" << httpBind << "is not a valid address — binding loopback"; + else + server.setHttpBindAddress(addr); + } + }; // When running in MCP mode, redirect stdout to stderr so that // Ogre/Qt debug output doesn't interfere with MCP JSON-RPC protocol. @@ -216,6 +235,7 @@ int main(int argc, char *argv[]) // Note: In standalone MCP mode, we don't have a MainWindow server.setOutputFd(savedStdoutFd); server.start(); + configureHttp(server); server.startHttp(httpPort); return a.exec(); @@ -448,6 +468,7 @@ int main(int argc, char *argv[]) mcpServer->setMainWindow(&w); mcpServer->setOutputFd(savedStdoutFd); mcpServer->start(); + configureHttp(*mcpServer); mcpServer->startHttp(httpPort); w.setMCPServer(mcpServer); // MainWindow takes ownership qDebug() << "MCP Server started alongside GUI"; diff --git a/website/src/data/content.js b/website/src/data/content.js index 14ac9d38f..018d449a2 100644 --- a/website/src/data/content.js +++ b/website/src/data/content.js @@ -185,7 +185,7 @@ export const highlightFeatures = [ }, { title: 'REST API', - body: 'Drive mesh and scene operations from external tools and automation scripts.' + body: 'Drive mesh and scene operations from external tools and automation scripts. Opt-in and loopback-only by default; tools run via POST, with an optional bearer token for shared machines.' }, { title: 'MCP / AI agent integration', From 7fc0f787a1c0bea69c4bd0d49ef2ef1125f26d88 Mon Sep 17 00:00:00 2001 From: Fernando Date: Wed, 16 Sep 2026 12:02:53 -0400 Subject: [PATCH 2/2] fix(#984): token from a file, never argv; CLI detection consumes HTTP option values (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings: P1 — `--http-token ` put the secret on the command line, where every local user can read it via ps / /proc//cmdline for the whole session, defeating the token on exactly the shared machines it targets. Replaced by `--http-token-file ` (MCPServer::readHttpTokenFile: trimmed; warns when group/other-readable; an unreadable or blank file means the HTTP server is NOT started — down beats up-and-unprotected). `--http-token` on argv is now refused at startup (exit 2, message names the three accepted sources) rather than being ignored, which would have started the API silently unprotected. QTMESH_HTTP_TOKEN and QSettings mcp/httpToken are unchanged. P2 — AppLaunchHandler::isCliInvocation did not consume the values of the HTTP options, so `--with-mcp --http-token-file scan` (or a value of `--cli`) routed the whole launch into CLIPipeline::run. Both of its scans now skip the value of --http-port / --http-token / --http-token-file / --http-bind; a real subcommand after the value pairs is still detected. Verified: 78 MCPServerHttp*/AppLaunchHandler* tests; e2e on the binary — `--http-token abc` exits 2 with the message; `--with-mcp --http-token scan` is refused, not run as `qtmesh scan`; `--http-token-file` → 401 without / 200 with the file's secret; a missing token file leaves nothing listening. Co-Authored-By: Claude Fable 5.1 --- CLAUDE.md | 4 +-- README.md | 2 +- src/AppLaunchHandler.cpp | 10 +++++- src/AppLaunchHandler_coverage_test.cpp | 38 +++++++++++++++++++-- src/MCPServer.cpp | 21 ++++++++++++ src/MCPServer.h | 5 +++ src/MCPServer_test.cpp | 28 +++++++++++++++ src/main.cpp | 47 ++++++++++++++++++++------ 8 files changed, 137 insertions(+), 18 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 56bd25ccf..bcc3781b2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -32,7 +32,7 @@ cmake --build build_local --target UnitTests -j4 ./build_local/bin/QtMeshEditor.app/Contents/MacOS/QtMeshEditor --with-mcp # GUI + MCP ./build_local/bin/QtMeshEditor.app/Contents/MacOS/QtMeshEditor --mcp # headless MCP only ./build_local/bin/QtMeshEditor.app/Contents/MacOS/QtMeshEditor --with-mcp --http-port 8080 # with HTTP API (loopback-only; POST /api/tools/ executes, GET lists) -./build_local/bin/QtMeshEditor.app/Contents/MacOS/QtMeshEditor --with-mcp --http-port 8080 --http-token s3cret --http-bind 0.0.0.0 # #984: require `Authorization: Bearer s3cret` (or X-Api-Key), expose beyond localhost +./build_local/bin/QtMeshEditor.app/Contents/MacOS/QtMeshEditor --with-mcp --http-port 8080 --http-token-file ~/.qtmesh_http_token --http-bind 0.0.0.0 # #984: require `Authorization: Bearer ` (or X-Api-Key), expose beyond localhost. NB `--http-token ` is REFUSED (argv is readable by every local user via ps); use the file, QTMESH_HTTP_TOKEN, or QSettings mcp/httpToken ``` **CLI pipeline (`qtmesh`):** @@ -369,7 +369,7 @@ The animation pipeline started skeleton-only; the #517 epic broadens it. Slices - Launch modes: `--mcp` (headless), `--with-mcp` (GUI + MCP). - stdout is redirected to stderr to isolate MCP JSON-RPC from Ogre/Qt debug output; original stdout fd saved for MCP responses. - HTTP API uses QTcpServer with deferred tool execution (QTimer::singleShot) to avoid re-entrant crashes from Ogre event processing. -- **HTTP API hardening (#984)**: the REST surface can run EVERY tool, mutating ones included, so (1) it binds **loopback** by default — `--http-bind ` / `QTMESH_HTTP_BIND` opt into `0.0.0.0` (containers with a mapped port), and a non-loopback bind without a token logs a warning; (2) tools execute **only via `POST /api/tools/`** — `GET /api/tools/` answers `405` + `Allow: POST` and dispatches nothing (it used to run the tool with no arguments, so any link/prefetch could decimate a mesh); `GET /api/tools` still lists; (3) an **optional shared secret** — `--http-token `, else `QTMESH_HTTP_TOKEN`, else QSettings `mcp/httpToken` (`MCPServer::resolveHttpToken`) — when set, every request except the CORS preflight (which cannot carry credentials by spec) must send `Authorization: Bearer ` or `X-Api-Key: `, else `401` + `WWW-Authenticate`; the compare is constant-time (`httpRequestAuthorized`, pure + unit-tested). Default (no token) stays open to local processes, so the `scripts/anim-*.sh` harnesses (all POST, plus a `GET /api/tools` liveness probe) run unchanged. `AppLaunchHandler::isGuiModeValueFlag` skips the `--http-token`/`--http-bind` VALUES so a token never becomes a launch path — and that skip is now checked BEFORE the generic leading-`-` `continue` in `collectGuiLaunchPaths`; it used to sit after it, i.e. was dead code, and `--http-port 8080` only appeared to work because "8080" is not an importable file (the test uses a real `.obj` as the value so the skip is observable). +- **HTTP API hardening (#984)**: the REST surface can run EVERY tool, mutating ones included, so (1) it binds **loopback** by default — `--http-bind ` / `QTMESH_HTTP_BIND` opt into `0.0.0.0` (containers with a mapped port), and a non-loopback bind without a token logs a warning; (2) tools execute **only via `POST /api/tools/`** — `GET /api/tools/` answers `405` + `Allow: POST` and dispatches nothing (it used to run the tool with no arguments, so any link/prefetch could decimate a mesh); `GET /api/tools` still lists; (3) an **optional shared secret** — `--http-token-file ` (`MCPServer::readHttpTokenFile`, trimmed; warns when group/other-readable; an unreadable/blank file means the HTTP server is NOT started — down beats up-and-unprotected), else `QTMESH_HTTP_TOKEN`, else QSettings `mcp/httpToken` (`MCPServer::resolveHttpToken`). **`--http-token ` on argv is refused at startup (exit 2)** — review caught that a command-line secret sits in `ps`/`/proc//cmdline` for the whole session, readable by every local user, i.e. useless on exactly the shared machines it targets — when set, every request except the CORS preflight (which cannot carry credentials by spec) must send `Authorization: Bearer ` or `X-Api-Key: `, else `401` + `WWW-Authenticate`; the compare is constant-time (`httpRequestAuthorized`, pure + unit-tested). Default (no token) stays open to local processes, so the `scripts/anim-*.sh` harnesses (all POST, plus a `GET /api/tools` liveness probe) run unchanged. `AppLaunchHandler::isGuiModeValueFlag` skips the `--http-port`/`--http-token(-file)`/`--http-bind` VALUES in BOTH `collectGuiLaunchPaths` (a token file never becomes a launch path) AND `isCliInvocation` (a value equal to a subcommand name or `--cli` — `--http-token-file scan` — must not reroute the whole launch into `CLIPipeline::run`; review finding) — and that skip is now checked BEFORE the generic leading-`-` `continue` in `collectGuiLaunchPaths`; it used to sit after it, i.e. was dead code, and `--http-port 8080` only appeared to work because "8080" is not an importable file (the test uses a real `.obj` as the value so the skip is observable). - **`take_screenshot` captures via an Ogre RTT, NOT `QWidget::grab()`** — Ogre renders straight to the native window surface (`WA_PaintOnScreen`) so `grab()` returns a black buffer. The tool renders the active viewport's `SpaceCamera` camera into an offscreen `PF_BYTE_RGBA` render target (RTSS `MSN_SHADERGEN` scheme + a temporary ambient boost & directional key light so imported materials aren't black, restored after) and reads it back to PNG. `load_mesh` calls `frameSceneInActiveViewport()` (select every user node → `frameSelection()`) so a headless `load_mesh`→`take_screenshot` actually frames + shows the mesh. This makes autonomous visual QA (load → optionally explode via `transform_submesh` → screenshot) work without a GUI operator. ### CLI Pipeline diff --git a/README.md b/README.md index 95fcb841a..cb6a841cc 100755 --- a/README.md +++ b/README.md @@ -320,7 +320,7 @@ Split View|Skeleton Animation Controls - **Performance capture** — video/webcam → facial morph animation (ARKit blendshapes), head pose, and full-body skeletal capture onto humanoid rigs; live preview + record in the editor, `qtmesh mocap` on the CLI (`-DENABLE_MOCAP` builds) - **AI chat** — natural language scene editing via local LLMs - **MCP server** — 57+ tools for AI agents (Claude, Cursor, etc.), including HDR/IBL (`set_hdr_environment`, `set_tonemap`, …) and QtMesh Cloud (`cloud_*`) -- **REST API** — opt-in HTTP interface for external automation (`--with-mcp --http-port 8080`). Bound to **localhost** by default; tools execute only via `POST /api/tools/` (`GET /api/tools` lists them); set `QTMESH_HTTP_TOKEN` (or `--http-token`) to require `Authorization: Bearer ` on every request, and `--http-bind 0.0.0.0` only when you mean to expose it +- **REST API** — opt-in HTTP interface for external automation (`--with-mcp --http-port 8080`). Bound to **localhost** by default; tools execute only via `POST /api/tools/` (`GET /api/tools` lists them); set `QTMESH_HTTP_TOKEN` (or `--http-token-file `; a secret on the command line is refused, since `ps` shows it to every local user) to require `Authorization: Bearer ` on every request, and `--http-bind 0.0.0.0` only when you mean to expose it --- diff --git a/src/AppLaunchHandler.cpp b/src/AppLaunchHandler.cpp index 0df80caad..993264023 100644 --- a/src/AppLaunchHandler.cpp +++ b/src/AppLaunchHandler.cpp @@ -41,8 +41,11 @@ bool isCliSubcommand(const QString& arg) // Flags that take a value — the value must not be mistaken for a launch path. bool isGuiModeValueFlag(const QString& arg) { + // --http-token is listed so its VALUE is still consumed (never read as a + // path or a subcommand) even though main() refuses it (#984 review: a + // secret on argv is visible to every local user via ps). return arg == QStringLiteral("--http-port") || arg == QStringLiteral("--http-token") - || arg == QStringLiteral("--http-bind"); + || arg == QStringLiteral("--http-token-file") || arg == QStringLiteral("--http-bind"); } bool isGuiModeFlag(const QString& arg) @@ -74,8 +77,12 @@ bool AppLaunchHandler::isCliInvocation(int argc, char* argv[]) if (execName.startsWith(QStringLiteral("qtmesh")) && !execName.contains(QStringLiteral("editor"))) return true; + // #984 review: the values of --http-port/--http-token-file/--http-bind are + // opaque — a token file named "scan" or a value "--cli" must not route the + // launch into the CLI. Consume each value in BOTH scans. for (int i = 1; i < argc; ++i) { const QString arg = QString::fromLocal8Bit(argv[i]); + if (isGuiModeValueFlag(arg)) { ++i; continue; } if (arg == QStringLiteral("--cli") || arg == QStringLiteral("--help") || arg == QStringLiteral("-h") || arg == QStringLiteral("--version") || arg == QStringLiteral("-v")) { @@ -85,6 +92,7 @@ bool AppLaunchHandler::isCliInvocation(int argc, char* argv[]) for (int i = 1; i < argc; ++i) { const QString arg = QString::fromLocal8Bit(argv[i]); + if (isGuiModeValueFlag(arg)) { ++i; continue; } if (arg.startsWith(QLatin1Char('-'))) continue; if (isCliSubcommand(arg)) diff --git a/src/AppLaunchHandler_coverage_test.cpp b/src/AppLaunchHandler_coverage_test.cpp index 6ae254198..ee39ed5bb 100644 --- a/src/AppLaunchHandler_coverage_test.cpp +++ b/src/AppLaunchHandler_coverage_test.cpp @@ -245,9 +245,41 @@ TEST(AppLaunchHandlerCoverageTest, Collect_HttpPortAtEndNoFollowingArg) EXPECT_TRUE(AppLaunchHandler::collectGuiLaunchPaths(args).isEmpty()); } +TEST(AppLaunchHandlerCoverageTest, Cli_HttpOptionValuesNeverRouteToCli) +{ + // #984 review: a token (file) that happens to equal a subcommand, or a value + // of "--cli", must not turn a GUI/MCP launch into a CLI run. + { + char a0[] = "QtMeshEditor", a1[] = "--with-mcp", a2[] = "--http-token-file", a3[] = "scan"; + char* argv[] = {a0, a1, a2, a3}; + EXPECT_FALSE(AppLaunchHandler::isCliInvocation(4, argv)); + } + { + char a0[] = "QtMeshEditor", a1[] = "--http-token", a2[] = "scan"; // refused by main(), still consumed here + char* argv[] = {a0, a1, a2}; + EXPECT_FALSE(AppLaunchHandler::isCliInvocation(3, argv)); + } + { + char a0[] = "QtMeshEditor", a1[] = "--http-token-file", a2[] = "--cli"; + char* argv[] = {a0, a1, a2}; + EXPECT_FALSE(AppLaunchHandler::isCliInvocation(3, argv)); + } + { + char a0[] = "QtMeshEditor", a1[] = "--http-bind", a2[] = "0.0.0.0", a3[] = "--http-port", a4[] = "9000"; + char* argv[] = {a0, a1, a2, a3, a4}; + EXPECT_FALSE(AppLaunchHandler::isCliInvocation(5, argv)); + } + { + // …while a REAL subcommand after the value pairs is still detected. + char a0[] = "QtMeshEditor", a1[] = "--http-port", a2[] = "9000", a3[] = "info"; + char* argv[] = {a0, a1, a2, a3}; + EXPECT_TRUE(AppLaunchHandler::isCliInvocation(4, argv)); + } +} + TEST(AppLaunchHandlerCoverageTest, Collect_HttpTokenAndBindValuesAreSkipped) { - // #984: --http-token / --http-bind take a value like --http-port does. The + // #984: --http-token-file / --http-bind take a value like --http-port does. The // value is skipped even when it names a REAL importable file — otherwise a // token that happens to look like a path would be opened in the editor. QTemporaryDir dir; @@ -261,8 +293,8 @@ TEST(AppLaunchHandlerCoverageTest, Collect_HttpTokenAndBindValuesAreSkipped) const QStringList args = { QStringLiteral("QtMeshEditor"), QStringLiteral("--with-mcp"), - QStringLiteral("--http-token"), decoy, - QStringLiteral("--http-bind"), QStringLiteral("0.0.0.0"), + QStringLiteral("--http-token-file"), decoy, + QStringLiteral("--http-bind"), QStringLiteral("0.0.0.0"), }; EXPECT_TRUE(AppLaunchHandler::collectGuiLaunchPaths(args).isEmpty()); diff --git a/src/MCPServer.cpp b/src/MCPServer.cpp index 244db9802..bdbe1f722 100644 --- a/src/MCPServer.cpp +++ b/src/MCPServer.cpp @@ -12952,6 +12952,27 @@ QString MCPServer::resolveHttpToken() return settings.value(QStringLiteral("mcp/httpToken")).toString().trimmed(); } +QString MCPServer::readHttpTokenFile(const QString &path, QString *error) +{ + QFile f(path); + if (!f.open(QIODevice::ReadOnly)) { + if (error) *error = QStringLiteral("cannot read HTTP token file %1: %2").arg(path, f.errorString()); + return {}; + } + // A secret file should not be readable by other local users — that would + // recreate the argv leak this option exists to avoid. Warn, don't refuse: + // the platform's permission model may not map onto these bits. + const QFileDevice::Permissions perms = f.permissions(); + if (perms & (QFileDevice::ReadGroup | QFileDevice::ReadOther)) + qWarning() << "HTTP token file" << path << "is readable by other users — consider chmod 600"; + const QString token = QString::fromUtf8(f.read(4096)).trimmed(); + if (token.isEmpty()) { + if (error) *error = QStringLiteral("HTTP token file %1 is empty").arg(path); + return {}; + } + return token; +} + QHostAddress MCPServer::resolveHttpBindAddress() { const QString env = QString::fromUtf8(qgetenv("QTMESH_HTTP_BIND")).trimmed(); diff --git a/src/MCPServer.h b/src/MCPServer.h index 529483737..f8625783f 100644 --- a/src/MCPServer.h +++ b/src/MCPServer.h @@ -113,6 +113,11 @@ class MCPServer : public QObject /** Token from `QTMESH_HTTP_TOKEN`, else QSettings `mcp/httpToken`, else empty. */ static QString resolveHttpToken(); + /** Read a token from a file (`--http-token-file`): whole content trimmed of + * surrounding whitespace/newlines. Empty result + `error` set when the + * file is missing, unreadable or blank. The secret is never placed on the + * command line, where every local user could read it via `ps`. */ + static QString readHttpTokenFile(const QString &path, QString *error = nullptr); /** Bind address from `QTMESH_HTTP_BIND` (e.g. "0.0.0.0"), else loopback. */ static QHostAddress resolveHttpBindAddress(); /** True when the raw request header block carries `token` as a Bearer diff --git a/src/MCPServer_test.cpp b/src/MCPServer_test.cpp index ae478ac89..c7ed98f74 100644 --- a/src/MCPServer_test.cpp +++ b/src/MCPServer_test.cpp @@ -4700,6 +4700,34 @@ TEST_F(MCPServerHttpTest, TokenResolution_EnvWinsOverSettings_ExplicitWinsOverBo EXPECT_TRUE(MCPServer::resolveHttpToken().isEmpty()); } +TEST_F(MCPServerHttpTest, TokenFile_TrimmedContentOrError) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString good = dir.filePath(QStringLiteral("token")); + { QFile f(good); ASSERT_TRUE(f.open(QIODevice::WriteOnly)); f.write(" s3cret-token\n\n"); } + QString err; + EXPECT_EQ(MCPServer::readHttpTokenFile(good, &err), QStringLiteral("s3cret-token")); + EXPECT_TRUE(err.isEmpty()); + + const QString blank = dir.filePath(QStringLiteral("blank")); + { QFile f(blank); ASSERT_TRUE(f.open(QIODevice::WriteOnly)); f.write("\n \n"); } + EXPECT_TRUE(MCPServer::readHttpTokenFile(blank, &err).isEmpty()); + EXPECT_TRUE(err.contains("empty")); + + err.clear(); + EXPECT_TRUE(MCPServer::readHttpTokenFile(dir.filePath(QStringLiteral("missing")), &err).isEmpty()); + EXPECT_FALSE(err.isEmpty()); + + // The file-sourced token protects the API exactly like an explicit one. + server->setOgreInitFailed(true); + server->setHttpToken(MCPServer::readHttpTokenFile(good)); + ASSERT_TRUE(server->startHttp(0)); + EXPECT_EQ(getHttpStatus(sendHttpRequest(server->httpPort(), postListMaterials())), 401); + EXPECT_EQ(getHttpStatus(sendHttpRequest(server->httpPort(), + postListMaterials("Authorization: Bearer s3cret-token\r\n"))), 200); +} + TEST_F(MCPServerHttpTest, BindAddress_DefaultIsLoopback_AnyIsOptIn) { ASSERT_TRUE(server->startHttp(0)); diff --git a/src/main.cpp b/src/main.cpp index 98e63b62c..58e5a8ff9 100755 --- a/src/main.cpp +++ b/src/main.cpp @@ -172,8 +172,9 @@ int main(int argc, char *argv[]) bool mcpOnlyMode = false; bool mcpWithGuiMode = false; int httpPort = 8080; - QString httpToken; // #984: --http-token (else QTMESH_HTTP_TOKEN / QSettings) - QString httpBind; // #984: --http-bind (else QTMESH_HTTP_BIND / loopback) + QString httpTokenFile; // #984: --http-token-file (else QTMESH_HTTP_TOKEN / QSettings) + QString httpBind; // #984: --http-bind (else QTMESH_HTTP_BIND / loopback) + bool httpTokenOnArgv = false; for (int i = 1; i < argc; ++i) { QString arg = QString(argv[i]); if (arg == "--mcp" || arg == "-mcp") { @@ -182,16 +183,39 @@ int main(int argc, char *argv[]) mcpWithGuiMode = true; } else if (arg == "--http-port" && i + 1 < argc) { httpPort = QString(argv[++i]).toInt(); - } else if (arg == "--http-token" && i + 1 < argc) { - httpToken = QString(argv[++i]); + } else if (arg == "--http-token-file" && i + 1 < argc) { + httpTokenFile = QString(argv[++i]); + } else if (arg == "--http-token") { + // Refused (review on #984): a secret on the command line is visible + // to every local user via ps / /proc//cmdline for the whole + // session, which defeats the token on exactly the shared machines + // it is meant for. Refuse loudly rather than start unprotected. + httpTokenOnArgv = true; + if (i + 1 < argc) ++i; } else if (arg == "--http-bind" && i + 1 < argc) { httpBind = QString(argv[++i]); } } + if (httpTokenOnArgv) { + fprintf(stderr, "Error: --http-token is not accepted — a secret on the command line is " + "visible to other local users (ps). Use --http-token-file , the " + "QTMESH_HTTP_TOKEN environment variable, or QSettings mcp/httpToken.\n"); + return 2; + } // Applies the CLI overrides before startHttp(); anything not given falls - // back to the env/QSettings resolution inside MCPServer. - auto configureHttp = [&](MCPServer &server) { - if (!httpToken.isEmpty()) server.setHttpToken(httpToken); + // back to the env/QSettings resolution inside MCPServer. Returns false when + // a token file was requested but cannot be read — the caller must then NOT + // start the HTTP server (down beats up-and-unprotected). + auto configureHttp = [&](MCPServer &server) -> bool { + if (!httpTokenFile.isEmpty()) { + QString err; + const QString token = MCPServer::readHttpTokenFile(httpTokenFile, &err); + if (token.isEmpty()) { + qCritical().noquote() << err << "— HTTP API NOT started"; + return false; + } + server.setHttpToken(token); + } if (!httpBind.isEmpty()) { const QHostAddress addr(httpBind); if (addr.isNull()) @@ -199,6 +223,7 @@ int main(int argc, char *argv[]) else server.setHttpBindAddress(addr); } + return true; }; // When running in MCP mode, redirect stdout to stderr so that @@ -235,8 +260,8 @@ int main(int argc, char *argv[]) // Note: In standalone MCP mode, we don't have a MainWindow server.setOutputFd(savedStdoutFd); server.start(); - configureHttp(server); - server.startHttp(httpPort); + if (configureHttp(server)) + server.startHttp(httpPort); return a.exec(); } @@ -468,8 +493,8 @@ int main(int argc, char *argv[]) mcpServer->setMainWindow(&w); mcpServer->setOutputFd(savedStdoutFd); mcpServer->start(); - configureHttp(*mcpServer); - mcpServer->startHttp(httpPort); + if (configureHttp(*mcpServer)) + mcpServer->startHttp(httpPort); w.setMCPServer(mcpServer); // MainWindow takes ownership qDebug() << "MCP Server started alongside GUI"; }