From 6e4e5d51ec5fd805df3b88290dc85f5f82ff653b Mon Sep 17 00:00:00 2001 From: vybe Date: Fri, 22 May 2026 21:35:07 +0000 Subject: [PATCH] refactor(clipboard): replace object map + 4 helpers with simple array + getClipboardCommands; QML becomes dumb index driver - lib.js: single clipboardTools array + getClipboardCommands(text) returning argv lists - Delete validateClipboardTool, buildCopyCommand, nextClipboardTool, allClipboardTools, safeClipboardTools, old clipboardTools object - Remove noClipboardTool / invalidClipboardCommand from getStrings (dead) - TailscaleWidget.qml: remove _copyCurrentTool + _executeCopy; add _copyIndex + _runNextCopy - copyProcess.onExited and copyToClipboard now purely index-driven, no tool names - Update tests: remove 4 old clipboard test blocks + dead string assertions; add focused getClipboardCommands tests - All 34 tests pass This is the minimal honest design given Process is a QML type. --- tailscalectl/TailscaleWidget.qml | 23 ++++----- tailscalectl/lib.js | 44 +++++------------- test/lib.test.js | 80 ++++++++------------------------ 3 files changed, 39 insertions(+), 108 deletions(-) diff --git a/tailscalectl/TailscaleWidget.qml b/tailscalectl/TailscaleWidget.qml index eb052ba..1a25c30 100644 --- a/tailscalectl/TailscaleWidget.qml +++ b/tailscalectl/TailscaleWidget.qml @@ -15,7 +15,7 @@ PluginComponent { property string currentExitNode: "" property var peers: [] property string _copyText: "" - property string _copyCurrentTool: "" + property int _copyIndex: 0 property bool _pendingToggle: false // transient one-shot for post-action verification (to be removed in first-principles refactor) layerNamespacePlugin: "tailscalectl" @@ -50,13 +50,9 @@ PluginComponent { onExited: (code, status) => { if (code === 0) { ToastService.showInfo(TailscaleLib.getStrings().copied(root._copyText)) - } else if (root._copyCurrentTool === "dms") { - // One simple fallback attempt: try wl-copy. - root._copyCurrentTool = "wl-copy" - root._executeCopy() } else { - var detail = copyProcess.stderr.text.trim() - ToastService.showError("tailscalectl", TailscaleLib.formatError("clipboard", detail)) + root._copyIndex += 1 + root._runNextCopy() } } } @@ -126,18 +122,17 @@ PluginComponent { function copyToClipboard(text) { root._copyText = text - // Simple two-tool fallback per first-principles plan: dms first, then wl-copy. - root._copyCurrentTool = "dms" - root._executeCopy() + root._copyIndex = 0 + root._runNextCopy() } - function _executeCopy() { - var cmd = TailscaleLib.buildCopyCommand(root._copyText, root._copyCurrentTool) - if (!cmd) { + function _runNextCopy() { + const cmds = TailscaleLib.getClipboardCommands(root._copyText) + if (root._copyIndex >= cmds.length) { ToastService.showError("tailscalectl", TailscaleLib.formatError("clipboard")) return } - copyProcess.command = cmd + copyProcess.command = cmds[root._copyIndex] copyProcess.running = true } diff --git a/tailscalectl/lib.js b/tailscalectl/lib.js index 7e6f0ff..6d28fe8 100644 --- a/tailscalectl/lib.js +++ b/tailscalectl/lib.js @@ -34,34 +34,15 @@ function findActiveExitNode(peerMap) { return "" } -var safeClipboardTools = ["dms", "wl-copy"] +const clipboardTools = [ + { argv: ["dms", "cl", "copy"] }, + { argv: ["wl-copy"] } +]; - // Direct argv form — no sh -c, no escaping, no future injection surface. - // clipmanctl intentionally dropped (niche optional history manager, not needed on DMS + Wayland). - var clipboardTools = { - "dms": { argv: ["dms", "cl", "copy"] }, - "wl-copy": { argv: ["wl-copy"] } -} - -function validateClipboardTool(tool) { - return typeof tool === "string" && safeClipboardTools.includes(tool) -} - -function buildCopyCommand(text, tool) { - const entry = clipboardTools[tool] - if (!entry) return null - // Pure argv — the Process will pass the string verbatim. No shell involved. - return [...entry.argv, text] -} - -function nextClipboardTool(currentTool) { - var idx = safeClipboardTools.indexOf(currentTool) - if (idx < 0) return safeClipboardTools[0] - return safeClipboardTools[(idx + 1) % safeClipboardTools.length] -} - -function allClipboardTools() { - return safeClipboardTools.slice() +function getClipboardCommands(text) { + return clipboardTools.map(function (tool) { + return tool.argv.concat([text]); + }); } function getStrings() { @@ -72,8 +53,6 @@ function getStrings() { exitNodePrefix: "Exit node: ", none: "None", copied: function (text) { return "Copied " + text + " to clipboard" }, - noClipboardTool: "No clipboard tool found", - invalidClipboardCommand: "Invalid clipboard command", clearExitNode: "×", setExitNode: "↗", invalidExitNodeHostname: "Invalid exit node hostname" @@ -89,9 +68,8 @@ 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. +// Security: validate hostnames coming from tailscale status JSON. +// Fail closed on obviously malicious input. function isValidExitNodeHostname(hostname) { if (typeof hostname !== "string") return false; if (hostname === "") return true; // clear command @@ -149,5 +127,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, isValidExitNodeHostname, validateClipboardTool, buildCopyCommand, nextClipboardTool, allClipboardTools, buildToggleCommand, parseStatusResult, getStrings, shouldShowClearExitNode, isActiveExitNode } + module.exports = { parsePeers, makeExitNodeCommand, findActiveExitNode, errorMessage, formatError, getStatusCommand, isValidExitNodeHostname, getClipboardCommands, buildToggleCommand, parseStatusResult, getStrings, shouldShowClearExitNode, isActiveExitNode } } diff --git a/test/lib.test.js b/test/lib.test.js index 8a5ff9f..ecddc98 100644 --- a/test/lib.test.js +++ b/test/lib.test.js @@ -1,13 +1,13 @@ import { test } from "node:test" import assert from "node:assert" import lib from "../tailscalectl/lib.js" -const { parsePeers, makeExitNodeCommand, findActiveExitNode, errorMessage, formatError, getStatusCommand, isValidExitNodeHostname, validateClipboardTool, buildCopyCommand, nextClipboardTool, allClipboardTools, buildToggleCommand, parseStatusResult, getStrings, shouldShowClearExitNode, isActiveExitNode } = lib +const { parsePeers, makeExitNodeCommand, findActiveExitNode, errorMessage, formatError, getStatusCommand, isValidExitNodeHostname, getClipboardCommands, buildToggleCommand, parseStatusResult, getStrings, shouldShowClearExitNode, isActiveExitNode } = lib /* * Test coverage note for #48: * All pure functions exported from lib.js (including the newly extracted - * getStrings, shouldShowClearExitNode, isActiveExitNode, and the clipboard - * helpers) are exercised here via node:test. + * getStrings, shouldShowClearExitNode, isActiveExitNode, and getClipboardCommands + * are exercised here via node:test. * * The four Process objects, 5-second Timer, onExited handlers, copy fallback * state machine, and overall widget behavior in TailscaleWidget.qml have no @@ -88,67 +88,27 @@ test("errorMessage returns generic message for unknown command", () => { assert.strictEqual(msg, "Tailscale command failed") }) -// --- validateClipboardTool --- +// --- getClipboardCommands (simple array, argv only, per #48 first-principles) --- -test("validateClipboardTool accepts whitelisted tool names", () => { - assert.strictEqual(validateClipboardTool("dms"), true) - assert.strictEqual(validateClipboardTool("wl-copy"), true) - // clipmanctl intentionally dropped (see #49) +test("getClipboardCommands returns ordered argv arrays with text appended", () => { + const cmds = getClipboardCommands("1.2.3.4") + assert.ok(Array.isArray(cmds)) + assert.strictEqual(cmds.length, 2) + assert.deepStrictEqual(cmds[0], ["dms", "cl", "copy", "1.2.3.4"]) + assert.deepStrictEqual(cmds[1], ["wl-copy", "1.2.3.4"]) }) -test("validateClipboardTool rejects malicious inputs", () => { - assert.strictEqual(validateClipboardTool("rm -rf /"), false) - assert.strictEqual(validateClipboardTool("echo hi; malicious"), false) - assert.strictEqual(validateClipboardTool("$(whoami)"), false) - assert.strictEqual(validateClipboardTool("wl-copy || rm -rf /"), false) +test("getClipboardCommands handles text with special characters safely (direct argv)", () => { + const cmds = getClipboardCommands("it's a 'test' with \"quotes\" and\nnewlines") + assert.ok(Array.isArray(cmds)) + assert.strictEqual(cmds.length, 2) + assert.ok(cmds[0].includes("it's a 'test' with \"quotes\" and\nnewlines")) }) -test("validateClipboardTool rejects empty and falsy inputs", () => { - assert.strictEqual(validateClipboardTool(""), false) - assert.strictEqual(validateClipboardTool(null), false) - assert.strictEqual(validateClipboardTool(undefined), false) -}) - -// --- buildCopyCommand --- - -test("buildCopyCommand returns safe command for whitelisted clipboard tool", () => { - const cmd = buildCopyCommand("hello", "wl-copy") - assert.ok(Array.isArray(cmd)) - assert.deepStrictEqual(cmd, ["wl-copy", "hello"]) // direct argv, no sh -c (see #49) -}) - -test("buildCopyCommand returns null for invalid clipboard tool", () => { - assert.strictEqual(buildCopyCommand("hello", "rm -rf /"), null) - assert.strictEqual(buildCopyCommand("hello", ""), null) - assert.strictEqual(buildCopyCommand("hello", null), null) -}) - -test("buildCopyCommand safely handles text with special characters", () => { - const cmd = buildCopyCommand("it's a 'test' with \"quotes\" and\nnewlines", "wl-copy") - assert.ok(Array.isArray(cmd)) -}) - -// --- nextClipboardTool --- - -test("nextClipboardTool cycles through tools", () => { - assert.strictEqual(nextClipboardTool("dms"), "wl-copy") - assert.strictEqual(nextClipboardTool("wl-copy"), "dms") // clipmanctl dropped (#49) -}) - -test("nextClipboardTool falls back to first tool for unknown input", () => { - assert.strictEqual(nextClipboardTool(""), "dms") - assert.strictEqual(nextClipboardTool("unknown"), "dms") -}) - -// --- allClipboardTools --- - -test("allClipboardTools returns the list of safe tools", () => { - const tools = allClipboardTools() - assert.ok(Array.isArray(tools)) - assert.strictEqual(tools.length, 2) - assert.ok(tools.includes("dms")) - assert.ok(tools.includes("wl-copy")) - // clipmanctl intentionally removed (#49) +test("getClipboardCommands is deterministic and open for future tools", () => { + const cmds = getClipboardCommands("foo") + assert.ok(Array.isArray(cmds[0])) + assert.ok(Array.isArray(cmds[1])) }) // --- getStrings (for #34 i18n prep) --- @@ -160,8 +120,6 @@ test("getStrings returns canonical UI strings for the widget", () => { assert.ok(s.disconnected) assert.ok(s.exitNodePrefix) assert.ok(s.copied) - assert.ok(s.noClipboardTool) - assert.ok(s.invalidClipboardCommand) }) test("getStrings.copied interpolates the text", () => {