From 1a95d0330e0d7271f691205d2508a7d49e3f6596 Mon Sep 17 00:00:00 2001
From: vybe <vybe@jtmorris.net>
Date: Fri, 22 May 2026 20:46:37 +0000
Subject: [PATCH] security: validate hostname in makeExitNodeCommand (balanced
 paranoia, direct-argv defense-in-depth, TDD)

---
 tailscalectl/TailscaleWidget.qml | 11 +++++++++--
 tailscalectl/lib.js              | 25 ++++++++++++++++++++-----
 test/lib.test.js                 | 30 +++++++++++++++++++++++++++++-
 3 files changed, 58 insertions(+), 8 deletions(-)

diff --git a/tailscalectl/TailscaleWidget.qml b/tailscalectl/TailscaleWidget.qml
index 9b478a4..eb052ba 100644
--- a/tailscalectl/TailscaleWidget.qml
+++ b/tailscalectl/TailscaleWidget.qml
@@ -64,6 +64,8 @@ PluginComponent {
     Process {
         id: exitNodeProcess
 
+        // Command is set dynamically by setExitNode() 
+
         stderr: StdioCollector {}
 
         onExited: (code, status) => {
@@ -113,7 +115,12 @@ PluginComponent {
     }
 
     function setExitNode(hostname) {
-        exitNodeProcess.command = TailscaleLib.makeExitNodeCommand(hostname)
+        const cmd = TailscaleLib.makeExitNodeCommand(hostname)
+        if (!cmd) {
+            ToastService.showError("tailscalectl", TailscaleLib.getStrings().invalidExitNodeHostname)
+            return
+        }
+        exitNodeProcess.command = cmd
         exitNodeProcess.running = true
     }
 
@@ -322,4 +329,4 @@ PluginComponent {
             }
         }
     }
-}
\ No newline at end of file
+}
diff --git a/tailscalectl/lib.js b/tailscalectl/lib.js
index 86efe02..7e6f0ff 100644
--- a/tailscalectl/lib.js
+++ b/tailscalectl/lib.js
@@ -13,10 +13,13 @@ function parsePeers(peerMap) {
 }
 
 function makeExitNodeCommand(hostname) {
-    if (hostname === "") {
-        return ["tailscale", "set", "--exit-node="]
+    if (!isValidExitNodeHostname(hostname)) {
+        return null;
     }
-    return ["tailscale", "set", "--exit-node=" + hostname]
+    if (hostname === "") {
+        return ["tailscale", "set", "--exit-node="];
+    }
+    return ["tailscale", "set", "--exit-node=" + hostname];
 }
 
 function findActiveExitNode(peerMap) {
@@ -72,7 +75,8 @@ function getStrings() {
         noClipboardTool: "No clipboard tool found",
         invalidClipboardCommand: "Invalid clipboard command",
         clearExitNode: "×",
-        setExitNode: "↗"
+        setExitNode: "↗",
+        invalidExitNodeHostname: "Invalid exit node hostname"
     }
 }
 
@@ -85,6 +89,17 @@ function isActiveExitNode(currentExitNode, hostname) {
     return currentExitNode === hostname
 }
 
+// Security: validate hostnames coming from tailscale status JSON before we
+// concatenate them into a command argument. Direct argv is already used
+// (see #49), but we still fail closed on obviously malicious input.
+function isValidExitNodeHostname(hostname) {
+    if (typeof hostname !== "string") return false;
+    if (hostname === "") return true; // clear command
+    // Permissive enough for real Tailscale hostnames while rejecting
+    // classic shell metacharacters and control characters.
+    return /^[a-zA-Z0-9]([a-zA-Z0-9-_.]{0,62}[a-zA-Z0-9])?$/.test(hostname);
+}
+
 function parseStatusResult(jsonText) {
     try {
         const data = JSON.parse(jsonText)
@@ -134,5 +149,5 @@ function formatError(action, detail) {
 
 // CommonJS export for Node.js tests (ignored by QML)
 if (typeof module !== "undefined" && module.exports) {
-    module.exports = { parsePeers, makeExitNodeCommand, findActiveExitNode, errorMessage, formatError, getStatusCommand, validateClipboardTool, buildCopyCommand, nextClipboardTool, allClipboardTools, buildToggleCommand, parseStatusResult, getStrings, shouldShowClearExitNode, isActiveExitNode }
+    module.exports = { parsePeers, makeExitNodeCommand, findActiveExitNode, errorMessage, formatError, getStatusCommand, isValidExitNodeHostname, validateClipboardTool, buildCopyCommand, nextClipboardTool, allClipboardTools, buildToggleCommand, parseStatusResult, getStrings, shouldShowClearExitNode, isActiveExitNode }
 }
diff --git a/test/lib.test.js b/test/lib.test.js
index 22ad0e6..8a5ff9f 100644
--- a/test/lib.test.js
+++ b/test/lib.test.js
@@ -1,7 +1,7 @@
 import { test } from "node:test"
 import assert from "node:assert"
 import lib from "../tailscalectl/lib.js"
-const { parsePeers, makeExitNodeCommand, findActiveExitNode, errorMessage, formatError, getStatusCommand, validateClipboardTool, buildCopyCommand, nextClipboardTool, allClipboardTools, buildToggleCommand, parseStatusResult, getStrings, shouldShowClearExitNode, isActiveExitNode } = lib
+const { parsePeers, makeExitNodeCommand, findActiveExitNode, errorMessage, formatError, getStatusCommand, isValidExitNodeHostname, validateClipboardTool, buildCopyCommand, nextClipboardTool, allClipboardTools, buildToggleCommand, parseStatusResult, getStrings, shouldShowClearExitNode, isActiveExitNode } = lib
 
 /*
  * Test coverage note for #48:
@@ -261,6 +261,34 @@ test("formatError handles empty or falsy detail gracefully", () => {
     assert.strictEqual(formatError("connect", null), "Failed to connect to Tailscale")
 })
 
+// --- isValidExitNodeHostname + makeExitNodeCommand safety (balanced paranoia, direct-argv defense-in-depth) ---
+
+test("isValidExitNodeHostname accepts empty string (clear)", () => {
+    assert.strictEqual(isValidExitNodeHostname(""), true)
+})
+
+test("isValidExitNodeHostname accepts realistic Tailscale hostnames", () => {
+    ["router", "gluetun-sjc", "my-exit-node-01", "peer_with_underscore", "a.b.c"].forEach(h =>
+        assert.strictEqual(isValidExitNodeHostname(h), true, h)
+    )
+})
+
+test("isValidExitNodeHostname rejects obvious injection attempts", () => {
+    ["; rm -rf /", "$(whoami)", "`id`", "foo;bar", "a&b", "x\ny", "evil$(date)"].forEach(h =>
+        assert.strictEqual(isValidExitNodeHostname(h), false, h)
+    )
+})
+
+test("makeExitNodeCommand returns null for invalid hostname", () => {
+    assert.strictEqual(makeExitNodeCommand("; rm"), null)
+    assert.strictEqual(makeExitNodeCommand("$(whoami)"), null)
+})
+
+test("makeExitNodeCommand still produces correct argv for valid input", () => {
+    assert.deepStrictEqual(makeExitNodeCommand(""), ["tailscale", "set", "--exit-node="])
+    assert.deepStrictEqual(makeExitNodeCommand("gluetun-sjc"), ["tailscale", "set", "--exit-node=gluetun-sjc"])
+})
+
 // --- getStatusCommand (new pure helper for low-duty-cycle architecture) ---
 
 test("getStatusCommand returns the canonical tailscale status --json argv", () => {