Refuse secret key/token writes through a planted dest symlink

CHANGELOG.md

@@ -8,6 +8,12 @@ loosely while pre-1.0 (breaking changes can land on minor bumps).
 ## [Unreleased]
 
 ### Fixed
+- **Secret key/token writes do not follow a planted dest symlink.**
+  `write_key_file` and admin-token generate opened
+  `~/.arbiter/{openrouter_api_key,search_api_key,admin_token}` with
+  `O_CREAT|O_TRUNC`, so a dest symlink redirected the secret into the
+  link target. Open with `O_NOFOLLOW` and require a regular file so a
+  planted symlink or FIFO cannot exfiltrate the bytes.
 - **Remote `--connect` base URL query/userinfo.** `normalize_api_base_url`
   now rejects query strings, fragments, URL userinfo, and control bytes
   instead of concatenating them into every request (`https://host?x/v1/…`

CMakeLists.txt

@@ -169,6 +169,7 @@ set(ARBITER_SOURCES
     src/remote/api_client.cpp
     src/remote/sse_turn.cpp
     src/atomic_file.cpp
+    src/secret_file.cpp
     src/loop_manager.cpp
     src/title_generator.cpp
     src/cli.cpp
@@ -1394,6 +1395,17 @@ if(INDEX_BUILD_TESTS)
     )
     add_test(NAME unit_artifacts COMMAND unit_artifacts)
 
+    # Secret dest writes (API keys / admin token): O_NOFOLLOW + regular file.
+    add_executable(unit_secret_file
+        tests/test_secret_file.cpp
+        src/secret_file.cpp
+    )
+    target_include_directories(unit_secret_file PRIVATE
+        ${CMAKE_SOURCE_DIR}/include
+        ${CMAKE_SOURCE_DIR}/third_party/doctest
+    )
+    add_test(NAME unit_secret_file COMMAND unit_secret_file)
+
     # MCP layer (JSON-RPC framing, subprocess wrapper, /mcp dispatch).
     # Live-server tests are intentionally not wired here — manual smoke
     # via `arbiter --api` covers the playwright path.

include/secret_file.h

@@ -0,0 +1,17 @@
+#pragma once
+// arbiter/include/secret_file.h — owner-only dest writes that do not
+// follow a planted symlink or other non-regular dest.
+//
+// Used for ~/.arbiter API-key files and the generated admin token.
+
+#include <string>
+
+namespace arbiter {
+
+// Write `data` to `path` at mode 0600.  Opens with O_NOFOLLOW so a
+// dest symlink cannot redirect the bytes into its target, and refuses
+// after open unless the fd is a regular file (FIFO / device / dir).
+// Returns false on I/O failure or a non-regular dest.
+bool write_secret_file(const std::string& path, const std::string& data);
+
+} // namespace arbiter

src/cli.cpp

@@ -7,6 +7,7 @@
 #include "logger.h"
 #include "orchestrator.h"
 #include "sandbox.h"
+#include "secret_file.h"
 #include "starters.h"
 #include "tenant_store.h"
 #include "tui/tui_design.h"
@@ -29,7 +30,6 @@
 #include <system_error>
 #include <thread>
 
-#include <fcntl.h>
 #include <sys/stat.h>
 #include <unistd.h>
 
@@ -50,7 +50,8 @@ void signal_handler(int) { g_running = 0; }
 // tenant tokens.  Resolution order on every `arbiter --api` startup:
 //   1. $ARBITER_ADMIN_TOKEN              (overrides the file)
 //   2. ~/.arbiter/admin_token  (file)    (owner-only; re-tightens perms)
-//   3. generate a fresh one, write it to #2 at mode 0600, print at startup
+//   3. generate a fresh one, write it to #2 at mode 0600 (O_NOFOLLOW),
+//      print at startup
 //
 // A `true` in `freshly_generated` tells the caller to announce the new
 // token prominently — this is the only time the operator sees the
@@ -99,20 +100,11 @@ std::string resolve_admin_token(const std::string& config_dir,
     static const char* hex = "0123456789abcdef";
     for (unsigned char c : buf) { tok += hex[c >> 4]; tok += hex[c & 0xF]; }
 
-    const int fd = ::open(path.c_str(), O_WRONLY | O_CREAT | O_TRUNC, 0600);
-    if (fd < 0) {
+    if (!write_secret_file(path, tok + "\n")) {
         std::cerr << "ERR: could not write " << path << ": "
                   << std::strerror(errno) << "\n";
         std::exit(1);
     }
-    const std::string out = tok + "\n";
-    ssize_t w = ::write(fd, out.data(), out.size());
-    ::close(fd);
-    ::chmod(path.c_str(), 0600);
-    if (w != static_cast<ssize_t>(out.size())) {
-        std::cerr << "ERR: short write to " << path << "\n";
-        std::exit(1);
-    }
     freshly_generated = true;
     return tok;
 }

src/cli_helpers.cpp

@@ -4,6 +4,7 @@
 #include "commands.h"
 #include "mcp/manager.h"
 #include "model_catalog.h"
+#include "secret_file.h"
 #include "starters.h"
 #include "theme.h"
 #include "tui/opentui/engine.h"
@@ -659,26 +660,11 @@ std::string read_secret_line() {
     return line;
 }
 
-// Write the key to ~/.arbiter/<filename> with mode 0600.  Opens with the
-// restrictive mode upfront so the file is never world-readable in between.
+// Write the key to ~/.arbiter/<filename> with mode 0600.  Opens with
+// O_NOFOLLOW so a planted dest symlink cannot exfiltrate the key.
 bool write_key_file(const std::string& filename, const std::string& key) {
     const std::string path = get_config_dir() + "/" + filename;
-    const int fd = ::open(path.c_str(), O_WRONLY | O_CREAT | O_TRUNC, 0600);
-    if (fd < 0) return false;
-    // open(2) mode is masked by umask — fchmod so the key is never
-    // briefly group/world-readable.
-    if (::fchmod(fd, 0600) != 0) { ::close(fd); return false; }
-    size_t off = 0;
-    while (off < key.size()) {
-        const ssize_t n = ::write(fd, key.data() + off, key.size() - off);
-        if (n <= 0) { ::close(fd); return false; }
-        off += static_cast<size_t>(n);
-    }
-    ::write(fd, "\n", 1);
-    ::close(fd);
-    // Re-apply mode in case the file pre-existed with wider perms.
-    ::chmod(path.c_str(), 0600);
-    return true;
+    return write_secret_file(path, key + "\n");
 }
 
 // Read a trimmed line.  Exits(1) on EOF/Ctrl-D so the wizard can't spin.

src/secret_file.cpp

@@ -0,0 +1,54 @@
+#include "secret_file.h"
+
+#include <cerrno>
+#include <fcntl.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+namespace arbiter {
+
+bool write_secret_file(const std::string& path, const std::string& data) {
+    // O_NOFOLLOW: a planted dest symlink used to send API keys / adm_
+    // tokens into the link target (open(O_CREAT|O_TRUNC) follows).
+    // O_NONBLOCK: a planted FIFO must not hang the wizard / --api
+    // startup waiting for a reader — and must not deliver the secret
+    // if a reader is already attached.
+    const int fd = ::open(path.c_str(),
+                          O_WRONLY | O_CREAT | O_TRUNC | O_NOFOLLOW |
+                              O_CLOEXEC | O_NONBLOCK,
+                          0600);
+    if (fd < 0) return false;
+
+    auto fail = [fd](int err = 0) {
+        if (err != 0) errno = err;
+        const int saved = errno;
+        ::close(fd);
+        errno = saved;
+        return false;
+    };
+
+    struct stat st{};
+    if (::fstat(fd, &st) != 0) return fail();
+    if (!S_ISREG(st.st_mode)) return fail(EINVAL);
+
+    // open(2) mode is masked by umask — fchmod so the secret is never
+    // briefly group/world-readable.  Path chmod is not used: it follows
+    // a dest that is swapped for a symlink after open.
+    if (::fchmod(fd, 0600) != 0) return fail();
+
+    size_t off = 0;
+    while (off < data.size()) {
+        const ssize_t n = ::write(fd, data.data() + off, data.size() - off);
+        if (n < 0) {
+            if (errno == EINTR) continue;
+            return fail();
+        }
+        if (n == 0) return fail(EIO);
+        off += static_cast<size_t>(n);
+    }
+    if (::fsync(fd) != 0) return fail();
+    ::close(fd);
+    return true;
+}
+
+} // namespace arbiter

tests/test_secret_file.cpp

@@ -0,0 +1,106 @@
+// tests/test_secret_file.cpp — API-key / admin-token dest writes must
+// not follow a planted symlink or hang on a FIFO.
+
+#define DOCTEST_CONFIG_IMPLEMENT_WITH_MAIN
+#include "doctest.h"
+
+#include "secret_file.h"
+
+#include <fcntl.h>
+#include <sys/stat.h>
+#include <unistd.h>
+
+#include <filesystem>
+#include <fstream>
+#include <sstream>
+#include <string>
+
+using namespace arbiter;
+namespace fs = std::filesystem;
+
+namespace {
+
+std::string read_all(const fs::path& path) {
+    std::ifstream f(path);
+    std::ostringstream buf;
+    buf << f.rdbuf();
+    return buf.str();
+}
+
+fs::path make_temp_dir(const char* tag) {
+    const auto dir = fs::temp_directory_path()
+        / (std::string(tag) + "-" + std::to_string(::getpid()));
+    fs::create_directories(dir);
+    return dir;
+}
+
+} // namespace
+
+TEST_CASE("write_secret_file creates a 0600 regular file") {
+    const auto dir = make_temp_dir("arbiter-secret-create");
+    const auto path = dir / "admin_token";
+
+    REQUIRE(write_secret_file(path.string(), "adm_fresh-token\n"));
+    CHECK(read_all(path) == "adm_fresh-token\n");
+    CHECK(fs::is_regular_file(path));
+    CHECK_FALSE(fs::is_symlink(path));
+    struct stat st{};
+    REQUIRE(::stat(path.c_str(), &st) == 0);
+    CHECK((st.st_mode & 0777) == 0600);
+
+    fs::remove_all(dir);
+}
+
+TEST_CASE("write_secret_file overwrites an existing regular file") {
+    const auto dir = make_temp_dir("arbiter-secret-overwrite");
+    const auto path = dir / "openrouter_api_key";
+    {
+        std::ofstream f(path);
+        f << "old-key\n";
+    }
+    ::chmod(path.c_str(), 0644);
+
+    REQUIRE(write_secret_file(path.string(), "sk-new-key\n"));
+    CHECK(read_all(path) == "sk-new-key\n");
+    struct stat st{};
+    REQUIRE(::stat(path.c_str(), &st) == 0);
+    CHECK((st.st_mode & 0777) == 0600);
+
+    fs::remove_all(dir);
+}
+
+TEST_CASE("write_secret_file does not follow a planted dest symlink") {
+    const auto dir = make_temp_dir("arbiter-secret-dest-link");
+    const auto dest = dir / "admin_token";
+    const auto victim = dir / "secret.txt";
+    {
+        std::ofstream f(victim);
+        f << "do-not-clobber";
+    }
+    fs::create_symlink(victim, dest);
+
+    CHECK_FALSE(write_secret_file(dest.string(), "adm_LEAKED_TOKEN\n"));
+    CHECK(read_all(victim) == "do-not-clobber");
+    CHECK(fs::is_symlink(dest));
+
+    fs::remove_all(dir);
+}
+
+TEST_CASE("write_secret_file refuses a planted dest FIFO without hanging") {
+    const auto dir = make_temp_dir("arbiter-secret-fifo");
+    const auto dest = dir / "search_api_key";
+    REQUIRE(::mkfifo(dest.c_str(), 0600) == 0);
+
+    CHECK_FALSE(write_secret_file(dest.string(), "search-secret\n"));
+    CHECK(fs::is_fifo(dest));
+
+    fs::remove_all(dir);
+}
+
+TEST_CASE("write_secret_file refuses a directory dest") {
+    const auto dir = make_temp_dir("arbiter-secret-dir");
+    CHECK_FALSE(write_secret_file(dir.string(), "nope\n"));
+    CHECK(fs::is_directory(dir));
+
+    fs::remove_all(dir);
+}