Shell interpolation of clipboard detection output in TailscaleWidget.qml #12

New issue

Closed

opened 2026-05-19 02:37:48 +00:00 by vybe · 9 comments

vybe commented 2026-05-19 02:37:48 +00:00

Collaborator

Copy link

Problem

The clipboard tool detection flow in TailscaleWidget.qml runs shell commands, captures raw stdout as a string, and later interpolates that string into another shell command for execution. This creates an unsafe data path where unvalidated shell output becomes executable code.

Where

Detection — line 65:

The detectClipboard Process runs a compound sh -c command that checks for clipboard tools via which and echoes the matching command string to stdout:

["sh", "-c", "which dms >/dev/null 2>&1 && echo 'dms cl copy' || which wl-copy >/dev/null 2>&1 && echo 'wl-copy' || which clipmanctl >/dev/null 2>&1 && echo 'clipmanctl copy' || echo none"]

The raw stdout is stored in root.clipboardCmd on line 69:

root.clipboardCmd = this.text.trim()

Execution — line 144:

The stored string is interpolated directly into a new sh -c command:

copyProcess.command = ["sh", "-c", "printf '%s' '" + text.replace(/'/g, "'\\''") + "' | " + root.clipboardCmd]

No validation is performed on clipboardCmd before it's executed as shell code.

Why It's a Problem

1. Shell code injection via which output. The detection command trusts the output of which and echo without validation. If which is aliased, wrapped, or otherwise compromised, its output could include arbitrary shell code that gets executed on line 144. For example, an alias like alias which='which; echo; malicious_command #' would append extra lines to the captured output.

2. No whitelist validation. The code assumes the detection command will only produce one of four known strings (dms cl copy, wl-copy, clipmanctl copy, or none), but there's no enforcement of this. Any unexpected output — from a PATH manipulation, a which wrapper, or even the logic bug in the &&/|| chain — becomes executable shell code.

3. Logic bug in detection command. The &&/|| chain on line 65 is malformed. Each echo is followed by ||, meaning if echo itself fails (unlikely but possible), the next which runs. More importantly, the structure doesn't properly group the check-and-echo pairs, so the fallthrough behavior may not match intent.

4. String interpolation instead of command arrays. The rest of the file uses safe command arrays (e.g., ["tailscale", "status", "--json"] on line 81). The clipboard path is the only place that constructs a shell command string via concatenation, making it an outlier and a higher-risk surface.

## Problem The clipboard tool detection flow in `TailscaleWidget.qml` runs shell commands, captures raw stdout as a string, and later interpolates that string into another shell command for execution. This creates an unsafe data path where unvalidated shell output becomes executable code. ## Where **Detection** — line 65: The `detectClipboard` Process runs a compound `sh -c` command that checks for clipboard tools via `which` and echoes the matching command string to stdout: ``` ["sh", "-c", "which dms >/dev/null 2>&1 && echo 'dms cl copy' || which wl-copy >/dev/null 2>&1 && echo 'wl-copy' || which clipmanctl >/dev/null 2>&1 && echo 'clipmanctl copy' || echo none"] ``` The raw stdout is stored in `root.clipboardCmd` on line 69: ``` root.clipboardCmd = this.text.trim() ``` **Execution** — line 144: The stored string is interpolated directly into a new `sh -c` command: ``` copyProcess.command = ["sh", "-c", "printf '%s' '" + text.replace(/'/g, "'\\''") + "' | " + root.clipboardCmd] ``` No validation is performed on `clipboardCmd` before it's executed as shell code. ## Why It's a Problem 1. **Shell code injection via `which` output.** The detection command trusts the output of `which` and `echo` without validation. If `which` is aliased, wrapped, or otherwise compromised, its output could include arbitrary shell code that gets executed on line 144. For example, an alias like `alias which='which; echo; malicious_command #'` would append extra lines to the captured output. 2. **No whitelist validation.** The code assumes the detection command will only produce one of four known strings (`dms cl copy`, `wl-copy`, `clipmanctl copy`, or `none`), but there's no enforcement of this. Any unexpected output — from a PATH manipulation, a `which` wrapper, or even the logic bug in the `&&`/`||` chain — becomes executable shell code. 3. **Logic bug in detection command.** The `&&`/`||` chain on line 65 is malformed. Each `echo` is followed by `||`, meaning if `echo` itself fails (unlikely but possible), the next `which` runs. More importantly, the structure doesn't properly group the check-and-echo pairs, so the fallthrough behavior may not match intent. 4. **String interpolation instead of command arrays.** The rest of the file uses safe command arrays (e.g., `["tailscale", "status", "--json"]` on line 81). The clipboard path is the only place that constructs a shell command string via concatenation, making it an outlier and a higher-risk surface.

vybe commented 2026-05-19 03:57:24 +00:00

Author

Collaborator

Copy link

Cross-model audit consensus (5 models: qwen3.6-27b, claude-sonnet-4-6, openai/gpt-5.5, gemini-3.1-pro-preview, grok-4.3) flags this as the single most severe issue.

Key additional points from the group:

• Even though the single-quote escaping on text is currently sound, the raw interpolation of root.clipboardCmd (populated from which stdout) remains a latent injection surface for any future change to detection logic.
• Strong recommendation across models: eliminate sh -c entirely for the copy operation. Use Process with stdin (or dms cl copy <text> as argv) instead of piping through shell.
• Several audits also noted the missing onExited handler on copyProcess (success toast fires unconditionally at line 145-146), so failed clipboard commands are reported as successful to the user.

This aligns closely with the existing description but adds the multi-model severity signal and the explicit "remove shell" direction.

Cross-model audit consensus (5 models: qwen3.6-27b, claude-sonnet-4-6, openai/gpt-5.5, gemini-3.1-pro-preview, grok-4.3) flags this as the single most severe issue. Key additional points from the group: - Even though the single-quote escaping on `text` is currently sound, the raw interpolation of `root.clipboardCmd` (populated from `which` stdout) remains a latent injection surface for any future change to detection logic. - Strong recommendation across models: eliminate `sh -c` entirely for the copy operation. Use `Process` with stdin (or `dms cl copy <text>` as argv) instead of piping through shell. - Several audits also noted the missing `onExited` handler on `copyProcess` (success toast fires unconditionally at line 145-146), so failed clipboard commands are reported as successful to the user. This aligns closely with the existing description but adds the multi-model severity signal and the explicit "remove shell" direction.

jtmorris commented 2026-05-19 04:02:59 +00:00

Owner

Copy link

Gemini also flagged a good point about the which command being non-POSIX compliant and not as universal as the POSIX command command -v. While fixing this issue, serious consideration to switching to the POSIX compliant version should be given.

Gemini also flagged a good point about the `which` command being non-POSIX compliant and not as universal as the POSIX command `command -v`. While fixing this issue, serious consideration to switching to the POSIX compliant version should be given.

vybe commented 2026-05-19 05:07:49 +00:00

Author

Collaborator

Copy link

See also #17 (premature success toast on copyProcess) — same clipboard copy path.

See also #17 (premature success toast on copyProcess) — same clipboard copy path.

vybe referenced this issue 2026-05-19 05:22:07 +00:00

Missing edge-case tests for parsePeers and findActiveExitNode #21

vybe commented 2026-05-19 05:47:13 +00:00

Author

Collaborator

Copy link

Proposed Path Forward — Clipboard Security Overhaul

Resolves: #12 (shell injection), #17 (premature Copied toast), #25 (no stderr capture)

---

Overview

These three issues share a common root: the clipboard code path is the only place in the widget that constructs shell commands via string interpolation, lacks exit-status verification, and discards stderr. The fix is a single cohesive refactor of the clipboard detection and execution flow.

Plan

Step 1 — Whitelist-based clipboard detection (fixes #12)

Replace the compound sh -c detection command with three separate which invocations, each as a proper command array. Store only the binary path, not the full command string. In copyToClipboard(), validate clipboardCmd against a hardcoded whitelist (["dms", "wl-copy", "clipmanctl"]) before constructing the copy command. If the value is not on the whitelist, fall through to the "No clipboard tool found" error.

Construct the copy command as a proper array instead of string interpolation:

• For dms: ["dms", "cl", "copy", text] — but this still needs printf | pipe. Alternative: use process.stdin if Quickshell supports it, or keep the sh -c but only interpolate the user-provided text (which is already escaped) into a printf, piped to a whitelisted binary name.

The key change: root.clipboardCmd stores only the binary name ("dms", "wl-copy", "clipmanctl", or ""). The copy command is constructed from a switch/map that maps the binary name to its known-safe argument structure. No arbitrary string ever reaches sh -c.

Step 2 — Exit-status check on copyProcess (fixes #17)

Add an onExited handler to copyProcess. Move the "Copied" toast from copyToClipboard() into onExited(code === 0). Add a failure toast for code !== 0.

Step 3 — Stderr capture on all Process components (fixes #25)

Add stderr: StdioCollector {} to toggleProcess, exitNodeProcess, and copyProcess. In each onExited handler, if code !== 0, include the stderr text in the error toast (truncated to a reasonable length). This gives users and developers the actual reason a command failed.

Success Criteria

1. clipboardCmd can only contain one of four known-safe values after detection.
2. No user-controlled or externally-sourced string is interpolated into a shell command.
3. copyProcess.onExited fires before any toast is shown. Success toast only on exit code 0.
4. All four Process components capture stderr and include it in error toasts on failure.
5. Existing unit tests pass. No new unit tests needed (this is QML-only), but manual verification of clipboard copy and error paths required.

AFK Classification

Partial AFK. Steps 1-3 are pure code edits to TailscaleWidget.qml. The agent can write the code and verify syntax. However, clipboard operations and toast display require a running DMS instance with a compositor — HITL needed to verify the popout UI, toast messages, and clipboard functionality actually work.

---

Files Modified

• tailscalectl/TailscaleWidget.qml — clipboard detection logic, copyProcess handler, stderr collectors on all processes

Also resolves: #17, #25

## Proposed Path Forward — Clipboard Security Overhaul **Resolves:** #12 (shell injection), #17 (premature Copied toast), #25 (no stderr capture) --- ### Overview These three issues share a common root: the clipboard code path is the only place in the widget that constructs shell commands via string interpolation, lacks exit-status verification, and discards stderr. The fix is a single cohesive refactor of the clipboard detection and execution flow. ### Plan **Step 1 — Whitelist-based clipboard detection (fixes #12)** Replace the compound `sh -c` detection command with three separate `which` invocations, each as a proper command array. Store only the binary path, not the full command string. In `copyToClipboard()`, validate `clipboardCmd` against a hardcoded whitelist (`["dms", "wl-copy", "clipmanctl"]`) before constructing the copy command. If the value is not on the whitelist, fall through to the "No clipboard tool found" error. Construct the copy command as a proper array instead of string interpolation: - For `dms`: `["dms", "cl", "copy", text]` — but this still needs `printf | pipe`. Alternative: use `process.stdin` if Quickshell supports it, or keep the `sh -c` but only interpolate the user-provided `text` (which is already escaped) into a `printf`, piped to a **whitelisted binary name**. The key change: `root.clipboardCmd` stores only the binary name (`"dms"`, `"wl-copy"`, `"clipmanctl"`, or `""`). The copy command is constructed from a switch/map that maps the binary name to its known-safe argument structure. No arbitrary string ever reaches `sh -c`. **Step 2 — Exit-status check on copyProcess (fixes #17)** Add an `onExited` handler to `copyProcess`. Move the "Copied" toast from `copyToClipboard()` into `onExited(code === 0)`. Add a failure toast for `code !== 0`. **Step 3 — Stderr capture on all Process components (fixes #25)** Add `stderr: StdioCollector {}` to `toggleProcess`, `exitNodeProcess`, and `copyProcess`. In each `onExited` handler, if `code !== 0`, include the stderr text in the error toast (truncated to a reasonable length). This gives users and developers the actual reason a command failed. ### Success Criteria 1. `clipboardCmd` can only contain one of four known-safe values after detection. 2. No user-controlled or externally-sourced string is interpolated into a shell command. 3. `copyProcess.onExited` fires before any toast is shown. Success toast only on exit code 0. 4. All four `Process` components capture stderr and include it in error toasts on failure. 5. Existing unit tests pass. No new unit tests needed (this is QML-only), but manual verification of clipboard copy and error paths required. ### AFK Classification **Partial AFK.** Steps 1-3 are pure code edits to `TailscaleWidget.qml`. The agent can write the code and verify syntax. However, clipboard operations and toast display require a running DMS instance with a compositor — **HITL needed** to verify the popout UI, toast messages, and clipboard functionality actually work. --- ### Files Modified - `tailscalectl/TailscaleWidget.qml` — clipboard detection logic, copyProcess handler, stderr collectors on all processes **Also resolves:** #17, #25

jtmorris commented 2026-05-19 06:23:54 +00:00

Owner

Copy link

I'm not sold on this approach. It's better than what's there, but it feels overengineered and like we are papering over the problem.

The whitelist validation is only necessary because the design still trusts output from an external process (which). You're adding a guard to compensate for a design decision that doesn't need to exist in the first place. Strip out the detection phase entirely and the whitelist becomes unnecessary.

Here's what I'd rather see: just try dms cl copy first. This is a widget for Dank Material Shell. In the overwhelming majority of real-world deployments, dms is going to be present and control the clipboard. If dms exits non-zero (not found, failed, whatever), fall back to wl-copy. If that fails, try clipmanctl copy. Cache whichever one succeeds so we're not reprobing on every copy operation. This is functionally identical to the level of clipboard checking the whitelist method did. However, this approach:

• Eliminates the detection process entirely — no which, no command -v, no separate startup phase. It reduces an assumption and dependency. Even a minor dependency.
• Never stores external process output in a property and later rams that somewhat untrustworthy, unsanitized output into a commandl. This injection surface disappears by design, not by guarding against it.
• Keeps all command construction, except the node name, from hardcoded internal state, rather than a subprocess' output.

I know sh -c still needs to be there for the stdin pipe (printf ... | dms cl copy), and that's fine — the key is that the binary name in that shell string comes from a hardcoded array in the source, never from subprocess output, and we remove overcomplicated checking that relies on other tools. That's the meaningful security line. It still isn't perfect, but it functionally achieves the same thing with a smaller surface, simpler code, and likely lower performance hit.

I'm not sold on this approach. It's better than what's there, but it feels overengineered and like we are papering over the problem. The whitelist validation is only necessary because the design still trusts output from an external process (which). You're adding a guard to compensate for a design decision that doesn't need to exist in the first place. Strip out the detection phase entirely and the whitelist becomes unnecessary. Here's what I'd rather see: just try dms cl copy first. This is a widget for Dank Material Shell. In the overwhelming majority of real-world deployments, dms is going to be present and control the clipboard. If dms exits non-zero (not found, failed, whatever), fall back to wl-copy. If that fails, try clipmanctl copy. Cache whichever one succeeds so we're not reprobing on every copy operation. This is functionally identical to the level of clipboard checking the whitelist method did. However, this approach: - Eliminates the detection process entirely — no which, no command -v, no separate startup phase. It reduces an assumption and dependency. Even a minor dependency. - Never stores external process output in a property and later rams that somewhat untrustworthy, unsanitized output into a commandl. This injection surface disappears by design, not by guarding against it. - Keeps all command construction, except the node name, from hardcoded internal state, rather than a subprocess' output. I know sh -c still needs to be there for the stdin pipe (printf ... | dms cl copy), and that's fine — the key is that the binary name in that shell string comes from a hardcoded array in the source, never from subprocess output, and we remove overcomplicated checking that relies on other tools. That's the meaningful security line. It still isn't perfect, but it functionally achieves the same thing with a smaller surface, simpler code, and likely lower performance hit.

vybe commented 2026-05-19 06:31:49 +00:00

Author

Collaborator

Copy link

Proposed Path Forward — Clipboard Security Overhaul (Revised)

Resolves: #12 (shell injection), #17 (premature Copied toast), #25 (no stderr capture)

---

Overview

Per @jtmorris's feedback, the original plan was overengineered. The whitelist was compensating for a design flaw — trusting subprocess output. The correct fix is to eliminate the detection phase entirely and use a try-fallback-cached approach with hardcoded tool names. No subprocess output ever enters the command construction path.

Plan

Step 1 — Remove detectClipboard entirely (fixes #12 root cause)

Delete the detectClipboard Process component and all references to it:

• Remove the Process definition (lines ~62-71)
• Remove property string clipboardCmd: ""
• Remove the detectClipboard.onExited handler
• Move the initial statusCheck trigger to Component.onCompleted: statusCheck.running = true so startup status polling still happens

This eliminates the entire injection surface. No which, no command -v, no subprocess output stored in a property.

Step 2 — Try-fallback-cached copy logic (fixes #12, #17)

Replace copyToClipboard(text) with a state-machine approach:

New properties:

property string cachedClipboardTool: ""  // "dms", "wl-copy", "clipmanctl", or ""
property string _copyText: ""
property string _copyCurrentTool: ""

copyToClipboard(text):

1. Set _copyText = text
2. Set _copyCurrentTool to cachedClipboardTool if non-empty, else "dms" (default first try)
3. Call _executeCopy()

_executeCopy():

1. Look up the command string from a hardcoded map: {"dms": "dms cl copy", "wl-copy": "wl-copy", "clipmanctl": "clipmanctl copy"}
2. Construct the command as ["sh", "-c", "printf '%s' '<escaped>' | <hardcodedCmd>"] — the binary name comes from source code, never from subprocess output
3. Start copyProcess

copyProcess.onExited(code, status):

• If code === 0: set cachedClipboardTool = _copyCurrentTool, show "Copied" toast
• If code !== 0: move to next tool in the hardcoded list ["dms", "wl-copy", "clipmanctl"]. If more tools remain, update _copyCurrentTool and call _executeCopy() again. If all exhausted, show "No clipboard tool found" error toast.

This means:

• First copy attempt after install: tries dms, falls back to wl-copy, then clipmanctl
• Subsequent copies: tries the cached (last-successful) tool first, falls back if it fails
• If the cached tool becomes unavailable, the fallback discovers a working replacement and caches it

Step 3 — Stderr capture on all Process components (fixes #25)

Add stderr: StdioCollector {} to toggleProcess, exitNodeProcess, and copyProcess. In each onExited handler, if code !== 0, include the stderr text in the error toast (truncated to ~120 chars). This gives users and developers the actual reason a command failed.

For copyProcess, stderr is only shown if all fallback tools fail (the final error toast).

Success Criteria

1. detectClipboard Process is removed. No startup detection phase exists.
2. clipboardCmd property is removed. No subprocess output is stored or later interpolated.
3. All binary names in copy commands come from hardcoded QML source, never from external process output.
4. copyProcess.onExited fires before any toast. Success toast only on exit code 0.
5. Failed copy attempts silently try the next tool; only the final failure (all tools exhausted) shows an error toast.
6. All four Process components capture stderr and include it in error toasts on failure.
7. Initial statusCheck fires at component load via Component.onCompleted.
8. Existing unit tests pass.

AFK Classification

Partial AFK. The code changes are well-defined QML edits. The agent can write the code and verify syntax/logic. However, clipboard operations, fallback behavior, and toast display require a running DMS instance — HITL needed to verify:

• Copy succeeds with available tool
• Fallback works when primary tool is missing
• Cached tool is updated on fallback success
• Error toast appears when no tools are available
• Stderr is included in error toasts

---

Files Modified

• tailscalectl/TailscaleWidget.qml — remove detectClipboard, add try-fallback-cached copy logic, add stderr collectors, move initial statusCheck trigger

## Proposed Path Forward — Clipboard Security Overhaul (Revised) **Resolves:** #12 (shell injection), #17 (premature Copied toast), #25 (no stderr capture) --- ### Overview Per @jtmorris's feedback, the original plan was overengineered. The whitelist was compensating for a design flaw — trusting subprocess output. The correct fix is to **eliminate the detection phase entirely** and use a try-fallback-cached approach with hardcoded tool names. No subprocess output ever enters the command construction path. ### Plan **Step 1 — Remove detectClipboard entirely (fixes #12 root cause)** Delete the `detectClipboard` Process component and all references to it: - Remove the Process definition (lines ~62-71) - Remove `property string clipboardCmd: ""` - Remove the `detectClipboard.onExited` handler - Move the initial `statusCheck` trigger to `Component.onCompleted: statusCheck.running = true` so startup status polling still happens This eliminates the entire injection surface. No `which`, no `command -v`, no subprocess output stored in a property. **Step 2 — Try-fallback-cached copy logic (fixes #12, #17)** Replace `copyToClipboard(text)` with a state-machine approach: New properties: ```qml property string cachedClipboardTool: "" // "dms", "wl-copy", "clipmanctl", or "" property string _copyText: "" property string _copyCurrentTool: "" ``` `copyToClipboard(text)`: 1. Set `_copyText = text` 2. Set `_copyCurrentTool` to `cachedClipboardTool` if non-empty, else `"dms"` (default first try) 3. Call `_executeCopy()` `_executeCopy()`: 1. Look up the command string from a hardcoded map: `{"dms": "dms cl copy", "wl-copy": "wl-copy", "clipmanctl": "clipmanctl copy"}` 2. Construct the command as `["sh", "-c", "printf '%s' '<escaped>' | <hardcodedCmd>"]` — the binary name comes from source code, never from subprocess output 3. Start `copyProcess` `copyProcess.onExited(code, status)`: - If `code === 0`: set `cachedClipboardTool = _copyCurrentTool`, show "Copied" toast - If `code !== 0`: move to next tool in the hardcoded list `["dms", "wl-copy", "clipmanctl"]`. If more tools remain, update `_copyCurrentTool` and call `_executeCopy()` again. If all exhausted, show "No clipboard tool found" error toast. This means: - First copy attempt after install: tries `dms`, falls back to `wl-copy`, then `clipmanctl` - Subsequent copies: tries the cached (last-successful) tool first, falls back if it fails - If the cached tool becomes unavailable, the fallback discovers a working replacement and caches it **Step 3 — Stderr capture on all Process components (fixes #25)** Add `stderr: StdioCollector {}` to `toggleProcess`, `exitNodeProcess`, and `copyProcess`. In each `onExited` handler, if `code !== 0`, include the stderr text in the error toast (truncated to ~120 chars). This gives users and developers the actual reason a command failed. For `copyProcess`, stderr is only shown if *all* fallback tools fail (the final error toast). ### Success Criteria 1. `detectClipboard` Process is removed. No startup detection phase exists. 2. `clipboardCmd` property is removed. No subprocess output is stored or later interpolated. 3. All binary names in copy commands come from hardcoded QML source, never from external process output. 4. `copyProcess.onExited` fires before any toast. Success toast only on exit code 0. 5. Failed copy attempts silently try the next tool; only the final failure (all tools exhausted) shows an error toast. 6. All four `Process` components capture stderr and include it in error toasts on failure. 7. Initial `statusCheck` fires at component load via `Component.onCompleted`. 8. Existing unit tests pass. ### AFK Classification **Partial AFK.** The code changes are well-defined QML edits. The agent can write the code and verify syntax/logic. However, clipboard operations, fallback behavior, and toast display require a running DMS instance — **HITL needed** to verify: - Copy succeeds with available tool - Fallback works when primary tool is missing - Cached tool is updated on fallback success - Error toast appears when no tools are available - Stderr is included in error toasts --- ### Files Modified - `tailscalectl/TailscaleWidget.qml` — remove detectClipboard, add try-fallback-cached copy logic, add stderr collectors, move initial statusCheck trigger

vybe referenced this issue 2026-05-19 06:49:38 +00:00

Lax / fragmented error handling throughout the widget (catch-all masking, missing exit checks, no stderr) #29

jtmorris commented 2026-05-19 06:49:38 +00:00

Owner

Copy link

Looks okay enough. Give it a shot @vybe.

Looks okay enough. Give it a shot @vybe.

jtmorris referenced this issue 2026-05-19 06:58:42 +00:00

statusCheck triggered from too many sites without deduplication (wasteful concurrent polls) #30

vybe referenced this issue 2026-05-20 09:33:46 +00:00

Extract QML-embedded logic into testable JavaScript modules #35

vybe referenced this issue 2026-05-20 09:39:26 +00:00

Extract QML-embedded logic into testable JavaScript modules #35

vybe commented 2026-05-21 06:09:47 +00:00

Author

Collaborator

Copy link

@vibe's latest plan for clipboard security overhaul needs update in light of recent master commits (e.g. 93b257b, 7c12793, a86d5e2, dddd921, ce8707c, b521af3) which:

• Extracted all JS logic (parseStatusResult, parseClipboardDetection, validateClipboardCmd, buildCopyCommand, buildToggleCommand, errorMessage, etc.) into tailscalectl/lib.js
• Added comprehensive test suite under test/
• Improved detection to use if/elif/else with xclip/xsel support (still using sh -c but now parsed/validated via lib.js)

The plan's core "remove detectClipboard entirely + try-fallback-cached" approach was not taken. Instead, detection was retained + hardened with whitelist validation and safe command builders in lib.js.

This changes the implementation surface: future work must edit lib.js for clipboard logic (and add/update tests), not just QML. The onExited handling, stderr, etc. still apply but now integrate with lib functions.

Updated plan should target lib.js + TailscaleWidget.qml + tests, and verify via npm test or equivalent.

Written by AI agent working for @jtmorris. Model: Grok 4.3.

@vibe's latest plan for clipboard security overhaul needs update in light of recent master commits (e.g. 93b257b, 7c12793, a86d5e2, dddd921, ce8707c, b521af3) which: - Extracted all JS logic (parseStatusResult, parseClipboardDetection, validateClipboardCmd, buildCopyCommand, buildToggleCommand, errorMessage, etc.) into `tailscalectl/lib.js` - Added comprehensive test suite under `test/` - Improved detection to use if/elif/else with xclip/xsel support (still using sh -c but now parsed/validated via lib.js) The plan's core "remove detectClipboard entirely + try-fallback-cached" approach was not taken. Instead, detection was retained + hardened with whitelist validation and safe command builders in lib.js. This changes the implementation surface: future work must edit lib.js for clipboard logic (and add/update tests), not just QML. The onExited handling, stderr, etc. still apply but now integrate with lib functions. Updated plan should target lib.js + TailscaleWidget.qml + tests, and verify via `npm test` or equivalent. Written by AI agent working for @jtmorris. Model: Grok 4.3.

vybe commented 2026-05-21 06:43:13 +00:00

Author

Collaborator

Copy link

This has been fixed and merged to vibes for review. Here's what changed:

Removed:

• detectClipboard Process and the clipboardCmd property
• Shell-based clipboard detection at startup (no more which chain)
• parseClipboardDetection from lib.js

Replaced with:

• Hardcoded whitelist of safe tool names: ["dms", "wl-copy", "clipmanctl"]
• Try-fallback-cached strategy: attempt cached tool first, then cycle through the list on failure
• validateClipboardTool() only accepts exact matches against the whitelist — no shell interpolation
• buildCopyCommand() constructs commands from the whitelist map, not from user/detection input

Additional improvements:

• Added stderr StdioCollector to all Process components so error toasts include up to 120 chars of stderr detail
• No startup delay — clipboard tools are tried on-demand when the user clicks to copy

The unsafe data path (raw shell output → interpolated into another shell command) no longer exists. All 27 tests pass.

Ready for review on vibes.

This has been fixed and merged to `vibes` for review. Here's what changed: **Removed:** - `detectClipboard` Process and the `clipboardCmd` property - Shell-based clipboard detection at startup (no more `which` chain) - `parseClipboardDetection` from `lib.js` **Replaced with:** - Hardcoded whitelist of safe tool names: `["dms", "wl-copy", "clipmanctl"]` - Try-fallback-cached strategy: attempt cached tool first, then cycle through the list on failure - `validateClipboardTool()` only accepts exact matches against the whitelist — no shell interpolation - `buildCopyCommand()` constructs commands from the whitelist map, not from user/detection input **Additional improvements:** - Added `stderr` StdioCollector to all Process components so error toasts include up to 120 chars of stderr detail - No startup delay — clipboard tools are tried on-demand when the user clicks to copy The unsafe data path (raw shell output → interpolated into another shell command) no longer exists. All 27 tests pass. Ready for review on `vibes`.

vybe closed this issue 2026-05-21 06:43:27 +00:00

Sign in to join this conversation.

No Branch/Tag specified

Branches Tags

master

vibes

testing

No results found.

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: jtmorris/dms_tailscalectl#12

No description provided.