Refuse MCP registry writes through a planted path.tmp symlink

CHANGELOG.md

@@ -7,10 +7,14 @@ loosely while pre-1.0 (breaking changes can land on minor bumps).
 
 ## [Unreleased]
 
-### Fixed
+- **MCP registry writes do not follow a planted `.tmp` symlink.**
+  `save_server_registry` opened `<path>.tmp` with `O_CREAT|O_TRUNC`, so
+  a symlink at that staging name redirected the write (including
+  registry `env` secrets) into the link target before `rename` replaced
+  only the symlink. Open the staging file with `O_NOFOLLOW` / `O_EXCL`
+  after `unlink` (which does not follow). Dest-symlink `rename` already
+  replaced the link, not its target.
 - **`/schedule` calendar dates.** `on YYYY-MM-DD` now rejects impossible
-  dates (Feb 31, Jun 31, Feb 29 in a non-leap year) instead of letting
-  `mktime` overflow into the next month and fire on a different day.
 - **MCP string JSON-RPC ids.** `parse_response` now accepts a decimal-string
 - **Reconcile rollback no longer wipes the workspace on a failed restore.**
 - **LaTeX math recursion depth.** `latex_math_to_plain` now stops converting

include/mcp/manager.h

@@ -52,7 +52,10 @@ std::vector<ServerSpec> load_server_registry(const std::string& path);
 std::string serialize_server_registry(const std::vector<ServerSpec>& specs);
 
 // Atomically write the registry to `path` (mode 0600).  Returns false on
-// I/O failure.  Empty specs writes `{ "servers": {} }`.
+// I/O failure.  Empty specs writes `{ "servers": {} }`.  The staging
+// name is opened with O_NOFOLLOW / O_EXCL after unlink (which does not
+// follow) so a planted `<path>.tmp` symlink cannot redirect the write
+// — registry env blocks may hold secrets.
 bool save_server_registry(const std::string& path,
                           const std::vector<ServerSpec>& specs);
 

src/mcp/manager.cpp

@@ -3,6 +3,7 @@
 #include "mcp/manager.h"
 
 #include <algorithm>
+#include <cerrno>
 #include <cstdio>
 #include <fcntl.h>
 #include <fstream>
@@ -189,8 +190,17 @@ bool save_server_registry(const std::string& path,
 
     // Write tmp with mode 0600 up front — registry env blocks may hold
     // secrets, and umask-default create + later chmod leaves a window.
+    //
+    // Drop a leftover crash file or a planted name first. unlink(2)
+    // removes a symlink itself, so this cannot clobber the target of
+    // `<path>.tmp` → elsewhere. open(O_CREAT|O_TRUNC) used to follow
+    // that link and write secrets into the target before rename
+    // replaced only the symlink.
     const std::string tmp = path + ".tmp";
-    const int fd = ::open(tmp.c_str(), O_WRONLY | O_CREAT | O_TRUNC, 0600);
+    ::unlink(tmp.c_str());
+    const int fd = ::open(tmp.c_str(),
+                          O_WRONLY | O_CREAT | O_EXCL | O_NOFOLLOW | O_CLOEXEC,
+                          0600);
     if (fd < 0) return false;
     // open(2) mode is masked by umask — force 0600 before any bytes land.
     if (::fchmod(fd, 0600) != 0) {
@@ -202,8 +212,14 @@ bool save_server_registry(const std::string& path,
     bool ok = true;
     while (ok && off < body.size()) {
         const ssize_t n = ::write(fd, body.data() + off, body.size() - off);
-        if (n <= 0) ok = false;
-        else off += static_cast<size_t>(n);
+        if (n < 0) {
+            if (errno == EINTR) continue;
+            ok = false;
+        } else if (n == 0) {
+            ok = false;
+        } else {
+            off += static_cast<size_t>(n);
+        }
     }
     if (ok) ok = (::fsync(fd) == 0);
     ::close(fd);

tests/test_mcp.cpp

@@ -28,6 +28,7 @@
 #include <filesystem>
 #include <fstream>
 #include <iostream>
+#include <sstream>
 #include <atomic>
 #include <string>
 #include <sys/stat.h>
@@ -836,6 +837,101 @@ TEST_CASE("save_server_registry rejects empty argv without throwing") {
     CHECK_FALSE(save_server_registry("/tmp/arbiter-mcp-should-not-exist.json", {s}));
 }
 
+TEST_CASE("save_server_registry does not follow a planted path.tmp symlink") {
+    namespace fs = std::filesystem;
+    const auto dir = fs::temp_directory_path()
+        / ("arbiter-mcp-tmp-link-" + std::to_string(::getpid()));
+    fs::create_directories(dir);
+    const auto path = (dir / "mcp_servers.json").string();
+    const auto victim = (dir / "secret.txt").string();
+    {
+        std::ofstream f(victim);
+        f << "do-not-clobber";
+    }
+    fs::create_symlink(victim, path + ".tmp");
+
+    ServerSpec s;
+    s.name = "playwright";
+    s.argv = {"npx"};
+    s.env_extra = {"TOKEN=super-secret"};
+    REQUIRE(save_server_registry(path, {s}));
+
+    {
+        std::ifstream f(victim);
+        std::ostringstream buf;
+        buf << f.rdbuf();
+        CHECK(buf.str() == "do-not-clobber");
+    }
+    CHECK(fs::is_regular_file(path));
+    CHECK_FALSE(fs::is_symlink(path));
+    CHECK_FALSE(fs::exists(path + ".tmp"));
+    struct stat st{};
+    REQUIRE(::stat(path.c_str(), &st) == 0);
+    CHECK((st.st_mode & 0777) == 0600);
+    auto loaded = load_server_registry(path);
+    REQUIRE(loaded.size() == 1);
+    REQUIRE(loaded[0].env_extra.size() == 1);
+    CHECK(loaded[0].env_extra[0] == "TOKEN=super-secret");
+
+    fs::remove_all(dir);
+}
+
+TEST_CASE("save_server_registry replaces a leftover regular tmp") {
+    namespace fs = std::filesystem;
+    const auto dir = fs::temp_directory_path()
+        / ("arbiter-mcp-tmp-stale-" + std::to_string(::getpid()));
+    fs::create_directories(dir);
+    const auto path = (dir / "mcp_servers.json").string();
+    {
+        std::ofstream f(path + ".tmp");
+        f << "stale crash leftover";
+    }
+
+    ServerSpec s;
+    s.name = "ok";
+    s.argv = {"npx"};
+    REQUIRE(save_server_registry(path, {s}));
+    CHECK_FALSE(fs::exists(path + ".tmp"));
+    auto loaded = load_server_registry(path);
+    REQUIRE(loaded.size() == 1);
+    CHECK(loaded[0].name == "ok");
+
+    fs::remove_all(dir);
+}
+
+TEST_CASE("save_server_registry rename replaces a dest symlink instead of writing through it") {
+    namespace fs = std::filesystem;
+    const auto dir = fs::temp_directory_path()
+        / ("arbiter-mcp-dest-link-" + std::to_string(::getpid()));
+    fs::create_directories(dir);
+    const auto path = (dir / "mcp_servers.json").string();
+    const auto victim = (dir / "outside.txt").string();
+    {
+        std::ofstream f(victim);
+        f << "keep-me";
+    }
+    fs::create_symlink(victim, path);
+
+    ServerSpec s;
+    s.name = "ok";
+    s.argv = {"npx"};
+    REQUIRE(save_server_registry(path, {s}));
+
+    {
+        std::ifstream f(victim);
+        std::ostringstream buf;
+        buf << f.rdbuf();
+        CHECK(buf.str() == "keep-me");
+    }
+    CHECK(fs::is_regular_file(path));
+    CHECK_FALSE(fs::is_symlink(path));
+    auto loaded = load_server_registry(path);
+    REQUIRE(loaded.size() == 1);
+    CHECK(loaded[0].name == "ok");
+
+    fs::remove_all(dir);
+}
+
 TEST_CASE("serialize_server_registry escapes quotes in names and args") {
     ServerSpec s;
     s.name = "weird\"name";