v1.22.0: SFTP correctness, safe mount location, and mount-state visibility

AGENTS.md

@@ -57,6 +57,7 @@ These structural constraints must hold at all times:
 | IPC Unix socket | `/run/user/<uid>/kcd/kcd.sock` (`$XDG_RUNTIME_DIR/kcd/kcd.sock`) |
 | Album art cache | `~/.cache/kcd/art/` (`$XDG_CACHE_HOME/kcd/art`) — resolved `kdeconnect://` art URIs, keyed by `kdeArtHash` |
 | Downloaded files | `~/Downloads/kcd/` (overridable via `download_dir` in config) |
+| SFTP mount points | `$XDG_RUNTIME_DIR/kcd/mnt/` (`/run/user/1000/kcd/mnt`; `$XDG_STATE_HOME/kcd/mnt` with no user session) — never under a bulk-deletable user folder, since `rm -rf` descends into a live mount |
 | systemd user unit | `~/.config/systemd/user/kcd.service` |
 
 > **Note:** The socket lives in a `kcd/` subdirectory of the runtime dir, not directly in `/run/user/<uid>/`.
@@ -79,6 +80,7 @@ These structural constraints must hold at all times:
 10. Start transport layer in a goroutine (`runTransport` → TCP listener + discovery broadcaster + mDNS)
 11. Send `READY=1` to `$NOTIFY_SOCKET` if present (systemd sd_notify)
 12. Block on `<-ctx.Done()`
+13. On shutdown, call `SftpPlugin.UnmountAll` (bounded by `shutdownUnmountBudget`). `OnDisconnect` only fires on a dropped connection, so without this every graceful stop leaves mounts live.
 
 ---
 

README.md

@@ -233,7 +233,8 @@ download_dir = "~/Downloads/kcd"
 
 [sftp]
 # auto_open    = true
-# mount_dir    = "/home/user/mnt"
+# read_only    = false          # mount read-only, so deletions cannot reach the phone
+# mount_dir    = "/run/user/1000/kcd/mnt"   # keep mounts out of folders you wipe in bulk
 
 [commands]
 uptime   = "uptime"

cmd/kcd/cli_sftp.go

@@ -7,6 +7,37 @@ import (
 	"github.com/urfave/cli/v2"
 )
 
+// readOnlyOverride maps the --ro/--no-ro flags onto the pointer the client
+// expects. Both flags absent leaves it nil, which means "use the daemon's
+// configured default" -- distinguishable from an explicit false, so an old
+// client cannot quietly turn a read-only default back into a writable mount.
+func readOnlyOverride(c *cli.Context) *bool {
+	switch {
+	case c.Bool("ro"):
+		v := true
+		return &v
+	case c.Bool("no-ro"):
+		v := false
+		return &v
+	default:
+		return nil
+	}
+}
+
+// readOnlyFlags is the flag pair shared by every command that can mount.
+func readOnlyFlags() []cli.Flag {
+	return []cli.Flag{
+		&cli.BoolFlag{
+			Name:  "ro",
+			Usage: "Mount read-only, so the phone's files cannot be deleted through it",
+		},
+		&cli.BoolFlag{
+			Name:  "no-ro",
+			Usage: "Mount writable even if `read_only = true` is set in [sftp]",
+		},
+	}
+}
+
 var sftpCmd = &cli.Command{
 	Name:  "sftp",
 	Usage: "Manage SFTP connections to a device",
@@ -25,7 +56,7 @@ The device responds with connection credentials on 'kcd watch'.`,
 				if err != nil {
 					return err
 				}
-				if err := cl.SftpMount(c.Args().First()); err != nil {
+				if err := cl.SftpMount(c.Args().First(), nil); err != nil {
 					return err
 				}
 				fmt.Println("SFTP mount requested. Run 'kcd sftp info' or 'kcd watch' to see details.")
@@ -43,6 +74,10 @@ Use 'kcd sftp request' first to populate the cache.`,
 					Name:  "json",
 					Usage: "Output raw JSON",
 				},
+				&cli.BoolFlag{
+					Name:  "show-password",
+					Usage: "Include the SFTP password (masked by default)",
+				},
 			},
 			Action: func(c *cli.Context) error {
 				if c.NArg() < 1 {
@@ -52,7 +87,8 @@ Use 'kcd sftp request' first to populate the cache.`,
 				if err != nil {
 					return err
 				}
-				info, err := cl.SftpInfo(c.Args().First())
+				showPassword := c.Bool("show-password")
+				info, err := cl.SftpInfo(c.Args().First(), showPassword)
 				if err != nil {
 					return err
 				}
@@ -61,11 +97,20 @@ Use 'kcd sftp request' first to populate the cache.`,
 					fmt.Println(string(out))
 					return nil
 				}
+				password := "*******"
+				if showPassword {
+					password = info.Password
+				}
 				fmt.Printf("IP:       %s\n", info.IP)
 				fmt.Printf("Port:     %s\n", info.Port)
 				fmt.Printf("User:     %s\n", info.User)
-				fmt.Printf("Password: %s\n", info.Password)
+				fmt.Printf("Password: %s\n", password)
 				fmt.Printf("Path:     %s\n", info.Path)
+				if info.Mounted {
+					fmt.Printf("Mounted:  yes (%s)\n", info.MountPoint)
+				} else {
+					fmt.Println("Mounted:  no")
+				}
 				if len(info.Volumes) > 0 {
 					fmt.Println("\nStorage volumes:")
 					for _, v := range info.Volumes {
@@ -122,6 +167,7 @@ Uses the multiPaths/pathNames fields from the cached SFTP credentials.`,
 			Description: `Send a request, wait for the phone to respond with credentials,
 mount the filesystem via sshfs, and open it in the default file manager.
 Requires sshfs to be installed.`,
+			Flags: readOnlyFlags(),
 			Action: func(c *cli.Context) error {
 				if c.NArg() < 1 {
 					return fmt.Errorf("missing device ID")
@@ -131,7 +177,7 @@ Requires sshfs to be installed.`,
 					return err
 				}
 				fmt.Println("Requesting SFTP credentials from phone (waiting up to 20s)…")
-				path, err := cl.SftpMountLocal(c.Args().First())
+				path, err := cl.SftpMountLocal(c.Args().First(), readOnlyOverride(c))
 				if err != nil {
 					return err
 				}
@@ -192,7 +238,7 @@ Examples:
 				}
 
 				fmt.Println("Requesting SFTP credentials from phone (waiting up to 20s)…")
-				path, volumes, err := cl.SftpBrowse(c.Args().First(), volume)
+				path, volumes, err := cl.SftpBrowse(c.Args().First(), volume, readOnlyOverride(c))
 				if err != nil {
 					return err
 				}

cmd/kcd/cli_sftp_deadline_test.go

@@ -0,0 +1,38 @@
+package main
+
+import (
+	"testing"
+	"time"
+)
+
+// The daemon waits credentials_timeout_secs for the phone; the client must
+// outlast it or the socket deadline wins and blames the connection.
+func TestSftpCredentialDeadline_ExceedsDaemonWait(t *testing.T) {
+	tests := []struct {
+		name     string
+		seconds  int
+		wantWait time.Duration
+	}{
+		{"unset falls back to the daemon default", 0, 20 * time.Second},
+		{"configured value", 45, 45 * time.Second},
+		{"one second", 1, time.Second},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := sftpCredentialDeadline(tc.seconds)
+			if got <= tc.wantWait {
+				t.Errorf("sftpCredentialDeadline(%d) = %v, must exceed the daemon's %v", tc.seconds, got, tc.wantWait)
+			}
+			if got > time.Duration(1<<62) {
+				t.Errorf("sftpCredentialDeadline(%d) = %v, looks like an overflow", tc.seconds, got)
+			}
+		})
+	}
+}
+
+func TestSftpCredentialDeadline_NoOverflow(t *testing.T) {
+	const huge = 1 << 40 // seconds
+	if got := sftpCredentialDeadline(huge); got <= 0 {
+		t.Errorf("sftpCredentialDeadline returned %v for an absurd value", got)
+	}
+}

cmd/kcd/format_event.go

@@ -19,12 +19,75 @@ func truncate(s string, max int) string {
 	return s
 }
 
+// runCommandLabel names the execution, falling back to the id when the plugin
+// published no key.
+func runCommandLabel(payload map[string]any) string {
+	if key := str(payload, "key"); key != "" {
+		return oneLine(truncate(key, 40))
+	}
+	return fmt.Sprintf("#%v", payload["id"])
+}
+
+// runCommandDetail renders one runcommand event body. A batch carries separate
+// stdout/stderr line lists; the lifecycle events carry a single `output`
+// transcript. Branching on status rather than probing for `success` matters:
+// an absent boolean would otherwise make a start look like a failure.
+func runCommandDetail(payload map[string]any) string {
+	if lines, ok := payload["stdout"].([]any); ok {
+		groups := make([]string, 0, 2)
+		if out := runCommandLines(lines, "out"); out != "" {
+			groups = append(groups, out)
+		}
+		if errOut := runCommandLines(payload["stderr"], "err"); errOut != "" {
+			groups = append(groups, errOut)
+		}
+		return strings.Join(groups, " | ")
+	}
+	if str(payload, "status") == "started" {
+		return "running"
+	}
+	if text := oneLine(truncate(str(payload, "output"), 200)); text != "" {
+		return text
+	}
+	if ok, _ := payload["success"].(bool); ok {
+		return "ok"
+	}
+	return "failed"
+}
+
+func runCommandLines(raw any, tag string) string {
+	lines, ok := raw.([]any)
+	if !ok || len(lines) == 0 {
+		return ""
+	}
+	parts := make([]string, 0, len(lines))
+	for _, l := range lines {
+		if s, ok := l.(string); ok && s != "" {
+			parts = append(parts, tag+": "+oneLine(s))
+		}
+	}
+	if len(parts) == 0 {
+		return ""
+	}
+	return strings.Join(parts, " | ")
+}
+
 // oneLine collapses newlines so a multi-line message cannot break the stream's
 // line-per-event shape.
 func oneLine(s string) string {
 	return strings.Join(strings.Fields(s), " ")
 }
 
+// volumeSuffix renders the optional volume an event was mounted for. The
+// daemon omits the key when the phone picked the volume itself, so an absent
+// key has to render as nothing rather than as a stray separator.
+func volumeSuffix(payload map[string]any) string {
+	if v, ok := payload["volume"].(string); ok && v != "" {
+		return " (" + v + ")"
+	}
+	return ""
+}
+
 // decodePayload re-decodes an event payload into a concrete type. The daemon
 // hands the CLI generic JSON, so a payload that was published as a struct
 // arrives as an untyped map and has to be round-tripped to reuse the same
@@ -102,6 +165,12 @@ func formatEvent(ev events.Event) string {
 	case events.TypeSftpMount:
 		return fmt.Sprintf("[%s] SFTP credentials received: %s\n", ev.DeviceID, payload["uri"])
 
+	case events.TypeSftpMounted:
+		return fmt.Sprintf("[%s] SFTP mounted at %s%s\n", ev.DeviceID, payload["mountPoint"], volumeSuffix(payload))
+
+	case events.TypeSftpUnmounted:
+		return fmt.Sprintf("[%s] SFTP unmounted (was %s)\n", ev.DeviceID, payload["mountPoint"])
+
 	case events.TypePairRequested:
 		return fmt.Sprintf("[%s] pair request from %s (%s). code: %v\n", ev.DeviceID, payload["name"], payload["type"], payload["verificationKey"])
 
@@ -181,6 +250,9 @@ func formatEvent(ev events.Event) string {
 		}
 		return fmt.Sprintf("[%s] volume: %s %v%%\n", ev.DeviceID, name, payload["volume"])
 
+	case events.TypeRunCommandOutput:
+		return fmt.Sprintf("[%s] runcommand %s: %s\n", ev.DeviceID, runCommandLabel(payload), runCommandDetail(payload))
+
 	case events.TypeContactsUpdated:
 		if str(payload, "phase") == "vcards" {
 			stored, _ := num(payload, "stored")
@@ -195,7 +267,14 @@ func formatEvent(ev events.Event) string {
 	// device.connected / device.added keep the bare type token as the first
 	// field so anything grepping for it still matches; the detail follows.
 	case events.TypeDeviceAdded:
-		name, _ := ev.Payload.(string)
+		// The payload is a full device view. Still tolerating a bare string
+		// keeps the renderer working if a payload ever predates that change.
+		var name string
+		if s, ok := ev.Payload.(string); ok {
+			name = s
+		} else if info, ok := ev.Payload.(map[string]any); ok {
+			name = str(info, "name")
+		}
 		if name == "" {
 			break
 		}

cmd/kcd/format_event_test.go

@@ -57,6 +57,21 @@ func TestFormatEventExistingFormats(t *testing.T) {
 			ev:   events.Event{Type: events.TypeSftpMount, DeviceID: "d1", Payload: map[string]any{"uri": "sftp://x"}},
 			want: "[d1] SFTP credentials received: sftp://x\n",
 		},
+		{
+			name: "sftp mounted",
+			ev:   events.Event{Type: events.TypeSftpMounted, DeviceID: "d1", Payload: map[string]any{"mountPoint": "/mnt/kcd-sftp-d1"}},
+			want: "[d1] SFTP mounted at /mnt/kcd-sftp-d1\n",
+		},
+		{
+			name: "sftp mounted with volume",
+			ev:   events.Event{Type: events.TypeSftpMounted, DeviceID: "d1", Payload: map[string]any{"mountPoint": "/mnt/kcd-sftp-d1", "volume": "/storage/ABCD-1234"}},
+			want: "[d1] SFTP mounted at /mnt/kcd-sftp-d1 (/storage/ABCD-1234)\n",
+		},
+		{
+			name: "sftp unmounted",
+			ev:   events.Event{Type: events.TypeSftpUnmounted, DeviceID: "d1", Payload: map[string]any{"mountPoint": "/mnt/kcd-sftp-d1"}},
+			want: "[d1] SFTP unmounted (was /mnt/kcd-sftp-d1)\n",
+		},
 		{
 			name: "pair requested",
 			ev:   events.Event{Type: events.TypePairRequested, DeviceID: "d1", Payload: map[string]any{"name": "Pixel", "type": "phone", "verificationKey": "12345"}},
@@ -186,6 +201,33 @@ func TestFormatEventNewTypes(t *testing.T) {
 			ev:   events.Event{Type: events.TypeContactsUpdated, DeviceID: "d1", Payload: map[string]any{"phase": "uids", "added": 2, "updated": 0, "deleted": 0, "pending": 5}},
 			want: "[d1] contacts: 2 added, 5 pending\n",
 		},
+		{
+			name: "runcommand started",
+			ev:   events.Event{Type: events.TypeRunCommandOutput, DeviceID: "d1", Payload: map[string]any{"id": 7, "key": "uptime", "status": "started"}},
+			want: "[d1] runcommand uptime: running\n",
+		},
+		{
+			name: "runcommand output batch",
+			ev: events.Event{Type: events.TypeRunCommandOutput, DeviceID: "d1", Payload: map[string]any{
+				"id": 7, "key": "uptime", "status": "output",
+				"stdout": []any{"up 3 days"}, "stderr": []any{"warn: x"},
+			}},
+			want: "[d1] runcommand uptime: out: up 3 days | err: warn: x\n",
+		},
+		{
+			name: "runcommand finished",
+			ev: events.Event{Type: events.TypeRunCommandOutput, DeviceID: "d1", Payload: map[string]any{
+				"id": 7, "key": "uptime", "status": "finished", "success": true, "output": "up 3 days",
+			}},
+			want: "[d1] runcommand uptime: up 3 days\n",
+		},
+		{
+			name: "runcommand finished without output",
+			ev: events.Event{Type: events.TypeRunCommandOutput, DeviceID: "d1", Payload: map[string]any{
+				"id": 7, "key": "lock", "status": "finished", "success": false, "output": "",
+			}},
+			want: "[d1] runcommand lock: failed\n",
+		},
 		{
 			name: "contacts vcards phase drops a zero skip count",
 			ev:   events.Event{Type: events.TypeContactsUpdated, DeviceID: "d1", Payload: map[string]any{"phase": "vcards", "stored": 4, "skipped": 0}},
@@ -212,7 +254,7 @@ func TestFormatEventNewTypes(t *testing.T) {
 		},
 		{
 			name: "device added keeps type token first",
-			ev:   events.Event{Type: events.TypeDeviceAdded, DeviceID: "d1", Payload: "Pixel 8"},
+			ev:   events.Event{Type: events.TypeDeviceAdded, DeviceID: "d1", Payload: map[string]any{"id": "d1", "name": "Pixel 8", "type": "phone", "state": "UNPAIRED", "connected": false}},
 			want: "[d1] device.added: Pixel 8\n",
 		},
 		{
@@ -251,7 +293,9 @@ func TestFormatEventStateSnapshotStaysBare(t *testing.T) {
 func TestFormatEventSurvivesBadPayloads(t *testing.T) {
 	types := []events.EventType{
 		events.TypeBatteryUpdate, events.TypeBatteryThreshold, events.TypeNotification,
-		events.TypeSftpMount, events.TypePairAccepted, events.TypeMprisUpdate,
+		events.TypeSftpMount, events.TypeSftpMounted, events.TypeSftpUnmounted,
+		events.TypeRunCommandOutput,
+		events.TypePairAccepted, events.TypeMprisUpdate,
 		events.TypeSMSIncoming, events.TypeSMSAttachment, events.TypePingReceived,
 		events.TypeConnectivityUpdate, events.TypeTelephonyRinging, events.TypeTelephonyMissed,
 		events.TypeTelephonyTalking, events.TypeTelephonyCanceled, events.TypeVolumeUpdate,