diff --git a/AGENTS.md b/AGENTS.md index 9dfdf4e..63256c6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -57,6 +57,7 @@ These structural constraints must hold at all times: | IPC Unix socket | `/run/user//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//`. @@ -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. --- diff --git a/README.md b/README.md index 475891a..b5be0e5 100644 --- a/README.md +++ b/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" diff --git a/cmd/kcd/cli_sftp.go b/cmd/kcd/cli_sftp.go index dc8eb73..c36138c 100644 --- a/cmd/kcd/cli_sftp.go +++ b/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 } diff --git a/cmd/kcd/cli_sftp_deadline_test.go b/cmd/kcd/cli_sftp_deadline_test.go new file mode 100644 index 0000000..201805b --- /dev/null +++ b/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) + } +} diff --git a/cmd/kcd/format_event.go b/cmd/kcd/format_event.go index 6d023f9..0da0ef5 100644 --- a/cmd/kcd/format_event.go +++ b/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 } diff --git a/cmd/kcd/format_event_test.go b/cmd/kcd/format_event_test.go index c83daf6..c4ef846 100644 --- a/cmd/kcd/format_event_test.go +++ b/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, diff --git a/cmd/kcd/main.go b/cmd/kcd/main.go index f99803e..94c6898 100644 --- a/cmd/kcd/main.go +++ b/cmd/kcd/main.go @@ -33,9 +33,32 @@ func getClient(c *cli.Context) (*client.Client, error) { SocketPath: cfg.SocketPath, Timeout: 5 * time.Second, PairListenTimeout: pairListenDeadline(config.Duration(cfg.Pairing.ListenTimeout)), + SftpTimeout: sftpCredentialDeadline(cfg.SFTP.CredentialsTimeoutSecs), }, nil } +// sftpCredentialDeadline is the client's deadline for the calls that wait on +// the phone's SFTP response. It has to outlast the daemon's own wait, or the +// socket deadline fires first and the user is told the connection timed out +// instead of being told the phone never answered -- which is the actual problem +// when the phone has not granted kcd file access. +func sftpCredentialDeadline(seconds int) time.Duration { + const overhead = 10 * time.Second + const maxDuration = time.Duration(1<<63 - 1) + if seconds <= 0 { + seconds = 20 + } + // Bound the seconds before converting, not after: seconds comes straight + // from the config file, and time.Duration(seconds)*time.Second overflows + // for any value past ~292 years, which would wrap to a negative deadline + // and time out instantly. + const maxSeconds = int64(maxDuration/time.Second) - int64(overhead/time.Second) + if int64(seconds) > maxSeconds { + return maxDuration + } + return time.Duration(seconds)*time.Second + overhead +} + // pairListenDeadline adds response overhead without overflowing a duration. func pairListenDeadline(timeout time.Duration) time.Duration { const maxDuration = time.Duration(1<<63 - 1) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 58415ad..1f7988b 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -290,9 +290,9 @@ goroutine leak when the child wedges. | `notification` | `kdeconnect.notification` | Downloads icon payload over TLS side-channel; per-app filter via `SetFilters()`; `notify-send --help` probe for `--print-id` support; `tlsConfig` + `logger` required in constructor | | `pair` | `kdeconnect.pair` | Manages the pairing handshake and certificate fingerprint verification | | `ping` | `kdeconnect.ping` | Fires `ping.received`; can be sent outbound | -| `runcommand` | `kdeconnect.runcommand`, `kdeconnect.runcommand.output` | Executes commands from the `[commands]` config table; results stream to the phone's output card via `runcommand.output` (`commandStarted` → batched `commandOutput` → `commandFinished`, all sharing one 32-bit id). A capped notification is still sent as a fallback. A 15s bound per execution; `{"stop":true}` from the phone cancels it, as does disconnect. | +| `runcommand` | `kdeconnect.runcommand`, `kdeconnect.runcommand.output` | Executes commands from the `[commands]` config table; results stream to the phone's output card via `runcommand.output` (`commandStarted` → batched `commandOutput` → `commandFinished`, all sharing one 32-bit id). A capped notification is still sent as a fallback. A 15s bound per execution; `{"stop":true}` from the phone cancels it, as does disconnect. Execution output is also published as `runcommand.output` bus events so `kcd watch` can follow it: lifecycle events are unconditional, but per-batch output is gated on `HasSubscribers`, so a chatty command does not flood the bus when nobody is listening. | | `sms` | `kdeconnect.sms.messages`, `kdeconnect.sms.attachment_file` | Sends `kdeconnect.sms.request`, `kdeconnect.sms.request_conversations`, `kdeconnect.sms.request_conversation`, `kdeconnect.sms.request_attachment` | -| `sftp` | `kdeconnect.sftp` | Parses `multiPaths`, `pathNames`, and `errorMessage` from the phone's response. `Info()` returns cached credentials + `StorageVolume` slices; `Volumes()` lists storage roots with human-readable names. `Handle()` logs errors when the phone returns `errorMessage` (e.g. missing storage permission). Mounts at server root to avoid chroot double-path bug; tracks mounts in `mountPoints` map; `Unmount()` calls `fusermount3`/`fusermount` | +| `sftp` | `kdeconnect.sftp` | Parses `multiPaths`, `pathNames`, and `errorMessage` from the phone's response. `Info()` returns cached credentials + `StorageVolume` slices; `Volumes()` lists storage roots with human-readable names. `Handle()` logs errors when the phone returns `errorMessage` (e.g. missing storage permission). Mounts at server root to avoid chroot double-path bug; tracks mounts in `mountPoints` map; `Unmount()` calls `fusermount3`/`fusermount`. Mounting is idempotent (`mountWithBody` returns an existing mount point rather than re-running `sshfs` over a live one), tracked state is dropped only after `fusermount` actually releases the mount so failures stay retryable — except when the kernel no longer has the mount at all, where there is nothing to release, so it is forgotten and reported unmounted, and a still-present unresponsive mount is retried with a lazy unmount, and `Info(deviceID, includePassword)` gates the credential — `kcd sftp info` masks it by default because the output lands in scrollback and logs. `mountPoints` is a **cache**, not the source of truth: `MountedPath` checks the configured path and then the pre-v1.22 `~/Downloads` one, parsing `/proc/mounts` for a `fuse.*` entry at either, so a mount that outlived a daemon restart is still reported mounted, cleaned up on disconnect, and unmountable by device id. The default mount directory is `$XDG_RUNTIME_DIR/kcd/mnt` — `/run` is a tmpfs, is what GNOME uses for its own remote file access, and is the one place a routine `rm -rf ~/…`, a dotfile cleaner or a backup tool will not descend into a live mount. A mount that resolves under a bulk-deletable user folder (`user-dirs.dirs`, else the conventional names) logs a warning, since the default is only as good as what the user overrides it with. `read_only` mounts pass `-o ro`, which makes the kernel reject `unlink`/`rmdir` before they reach the phone; it is emitted before `extra_sshfs_opts` so an operator override still wins. On graceful shutdown `daemon.Run` calls `UnmountAll`, because `OnDisconnect` only fires on a dropped connection and would otherwise leave every mount live | | `share` | `kdeconnect.share.request` | Streaming file receive + URL/text handling; fires progress events | | `systemvolume` | `kdeconnect.systemvolume` | Accepts `bus`; publishes `volume.update` on volume/mute changes | | `telephony` | `kdeconnect.telephony` | Fires `telephony.ringing`, `.missed`, `.canceled` | @@ -338,7 +338,9 @@ Each `Subscriber` holds a buffered channel (capacity 64). If a subscriber falls | `telephony.canceled` | Call ended | | `connectivity.update` | Signal strength report | | `volume.update` | Desktop volume changed from phone | -| `sftp.mount` | SFTP credentials received | +| `sftp.mount` | SFTP credentials received (carries the live password — subscribers must not surface it) | +| `sftp.mounted` | A device's storage finished mounting (`{mountPoint, volume?}`) | +| `sftp.unmounted` | A device's storage was released (`{mountPoint}`) | | `sms.incoming` | SMS message received from phone (batch, one event per message) | | `sms.attachment` | MMS attachment file downloaded to cache directory | diff --git a/docs/CLI.md b/docs/CLI.md index 77d392e..6f7f4d3 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -23,9 +23,20 @@ These flags apply to every command: Edit `$XDG_CONFIG_HOME/kcd/kcd.toml` (default `~/.config/kcd/kcd.toml`). All settings are optional; see the annotated example in the packaging directory. -Apply changes with `systemctl --user restart kcd` (or restart `kcd daemon` -when running it directly). Timing, storage and notification branding settings -require a restart; reloading notification filters alone does not apply them. + +Changes take effect either by reload or by restart: + +| What changed | How to apply | +|---|---| +| `[commands]` / `[commands_per_device]` | `systemctl --user reload kcd` | +| `[notifications]` filters | `systemctl --user reload kcd` | +| `log_level` | `systemctl --user reload kcd` | +| Everything else — network timing, storage paths, plugin toggles, branding | `systemctl --user restart kcd` | + +A reload re-reads the file and applies only those three; the rest of the +configuration is captured at startup. Running the daemon directly, send +`SIGHUP` (`kill -HUP `) instead. Notification *branding* +(`[notifications].app_name`) is not reloaded — only the per-app filters are. | Section | Settings and defaults | |---|---| @@ -699,24 +710,38 @@ kcd watch --json --events=sftp.mount | jq -r 'select(.type=="sftp.mount") | .pay Show cached SFTP connection details for a paired device, including available storage volumes: ``` -kcd sftp info [--json] +kcd sftp info [--json] [--show-password] ``` +The password is a working credential for the phone's SFTP server, so it is +masked by default in both the human and `--json` output. Pass `--show-password` +to include it, and note that the `sftp.mount` event always carries it — a +client that needs to mount can take the credentials from the event stream +instead of asking for them. + **Example output** ``` -Device: a1b2c3d4_e5f6_7890_abcd_ef1234567890 (Pixel 8 Pro) -IP: 192.168.1.50 -Port: 8022 -User: sftp-user +IP: 192.168.1.50 +Port: 8022 +User: sftp-user Password: ******** +Path: /storage/emulated/0 +Mounted: yes (/run/user/1000/kcd/mnt/kcd-sftp-a1b2c3d4) -Volumes: - 1. Internal shared storage → /storage/emulated/0 - 2. SD card → /storage/ABCD-1234 +Storage volumes: + Internal shared storage /storage/emulated/0 + SD card /storage/ABCD-1234 ``` -If the phone returned an error (e.g. storage permission not granted), the `errorMessage` field is shown instead. +`Mounted:` reports the current mount state, so a client or script can check it +without touching `/proc`. Subscribe to the `sftp.mounted` and `sftp.unmounted` +events to be told when it changes. + +A failed request is not shown here: when the phone returns an error (e.g. +storage permission not granted) the credentials are never cached, so `info` +reports no cached credentials. The error itself arrives on the `sftp.mount` +event as `{"error": "..."}`. ### sftp volumes @@ -744,17 +769,53 @@ Request credentials and immediately mount the phone's filesystem using `sshfs`. kcd sftp mount ``` -The mount point is printed to stdout. Unmount with `fusermount -u `. +Waits up to `[sftp] credentials_timeout_secs` for the phone to start its SFTP +server. If it does not answer, the phone is either not running KDE Connect or +has not granted kcd file access (on Android, enable file access for KDE +Connect) — the CLI's own deadline is derived from that same setting, so you get +that message rather than a socket timeout. + +The mount point is printed to stdout. Mounting is idempotent: if the device +is already mounted, the existing mount point is returned and `sshfs` is not run +again, so repeating the command is a cheap way to re-open the file manager. + +``` +kcd sftp mount [--ro] [--no-ro] +``` + +`--ro` mounts read-only, so writes and deletions fail locally instead of +reaching the phone. `--no-ro` forces writable when `[sftp] read_only = true`. +Neither means the configured default applies. A mode cannot be changed on an +existing mount, so unmount first to switch. ### sftp unmount Cleanly unmount a previously mounted phone filesystem. - kcd sftp unmount +``` +kcd sftp unmount +``` Calls `fusermount3` (or `fusermount` on older systems) and removes the -temporary mount point directory. Returns an error if the device was never -mounted in this daemon session. +mount point directory. Failures are distinguishable so a client can react +to each: + +| Error contains | Meaning | +|---|---| +| `not mounted` | Nothing is mounted for this device. Any leftover mount directory was removed as part of this | +| `stale SFTP mount at could not be released` | The mount is still in the kernel but `fusermount` could not detach it; retry, or unmount by path | +| _(none)_ | Unmounted cleanly | + +Unmount is idempotent. A mount that has already gone — its FUSE connection +died, or it was released by hand — is reported as unmounted and the daemon +forgets it, rather than retrying forever against a mount that no longer +exists. A mount that is still present but unresponsive is retried with a lazy +unmount, which detaches it. + +Mounts survive a daemon restart. The daemon treats the kernel's mount table +as the source of truth and its own record as a cache, so a mount made before a +restart is still reported as mounted, is adopted for cleanup, and can be +unmounted by device id as usual. ### sftp browse @@ -783,7 +844,7 @@ specified volume via sshfs, opening it in the default file manager: ``` $ kcd sftp browse a1b2c3d4 "SD card" Requesting SFTP credentials from phone (waiting up to 20s)… -Mounted at: /home/user/Downloads/kcd/mnt/kcd-sftp-a1b2c3d4 +Mounted at: /run/user/1000/kcd/mnt/kcd-sftp-a1b2c3d4 ``` The volume argument is resolved in this order: @@ -1004,7 +1065,7 @@ kcd watch [--events ] [--json] | Event type | Description | |---|---| -| `device.added` | A new device was seen for the first time | +| `device.added` | A new device was seen for the first time. Payload is the full device record (`id`, `name`, `type`, `state`, `connected`, `last_seen`), not just the name | | `device.removed` | A device was unpaired and removed | | `device.connected` | A device established a TCP connection | | `device.disconnected` | A device's connection dropped | @@ -1026,6 +1087,9 @@ kcd watch [--events ] [--json] | `volume.update` | Device volume changed: `{name, volume, muted}` | | `mpris.update` | Now playing: `{player, title, artist, album, isPlaying, pos, length, volume}` | | `sftp.mount` | SFTP credentials: `{uri, ip, port, user, password, path, multiPaths, pathNames, errorMessage}` | +| `sftp.mounted` | Mount finished: `{mountPoint, volume?}` | +| `sftp.unmounted` | Mount released: `{mountPoint}` | +| `runcommand.output` | Local command execution: `{id, key, status, stdout?, stderr?, output?, success?, truncated?}`, where `status` is `started`, `output` or `finished`. Output batches arrive only while a client is subscribed | | `battery.threshold` | Battery low/full alert: `{charge, charging, event}` | | `telephony.talking` | Call in progress: `{contactName, phoneNumber}` | | `sms.incoming` | SMS/MMS received: `{body, sender, date, thread_id, read}` | diff --git a/docs/CLIENT_GUIDE.md b/docs/CLIENT_GUIDE.md index d652420..4b93405 100644 --- a/docs/CLIENT_GUIDE.md +++ b/docs/CLIENT_GUIDE.md @@ -415,14 +415,14 @@ if resp["ok"]: ipc_request(sock, "contacts_clear", {"deviceId": dev_id}) ``` -### 5.7 Lock/Unlock +### 5.8 Lock/Unlock ```python ipc_request(sock, "lock", {"deviceId": dev_id}) ipc_request(sock, "unlock", {"deviceId": dev_id}) ``` -### 5.8 Push Clipboard +### 5.9 Push Clipboard ```python ipc_request(sock, "clipboard_push", {"deviceId": dev_id}) @@ -430,7 +430,7 @@ ipc_request(sock, "clipboard_push", {"deviceId": dev_id}) The daemon reads the local clipboard (`wl-paste`/`xclip`) and sends it. -### 5.9 Remote Volume Control +### 5.10 Remote Volume Control ```python # List audio sinks @@ -454,16 +454,19 @@ Volume changes from the phone arrive as `volume.update` events. The event payloa is either `{"name", "volume", "muted"}` for a single sink change or `{"sinks": [...]}` for the full sink list. -### 5.10 Get SFTP Connection Info +### 5.11 Get SFTP Connection Info ```python -resp = ipc_request(sock, "sftp_info", {"deviceId": dev_id}) +# The password is omitted unless you ask for it: it is a live credential for +# the phone's SFTP server. The sftp.mount event always carries it if you need +# the credentials to mount. +resp = ipc_request(sock, "sftp_info", {"deviceId": dev_id, "showPassword": True}) if resp["ok"]: info = resp["data"] print(f'SSH: {info["user"]}@{info["ip"]} -p {info["port"]}') ``` -### 5.10 Get Daemon Status +### 5.12 Get Daemon Status ```python resp = ipc_request(sock, "status") diff --git a/docs/IPC_PROTOCOL.md b/docs/IPC_PROTOCOL.md index c3f1c90..9527d28 100644 --- a/docs/IPC_PROTOCOL.md +++ b/docs/IPC_PROTOCOL.md @@ -101,6 +101,7 @@ Fields: | `battery` | object (optional) | `{"charge": 85, "charging": true, "batteryAgeMs": 1234}` — cached battery state; absent when the device never reported | | `media` | object (optional) | Cached `NowPlaying` plus `mediaAgeMs` (ms since the phone reported); absent when the device never reported media | | `signal` | object (optional) | Cached connectivity report (`{"signalStrengths": {...}}`); absent when never reported | +| `sftp` | object (optional) | `{"mounted": true, "mountPoint": "/path/to/mnt"}`; present only while the device's storage is mounted, so absence means not mounted | #### `pair` @@ -533,9 +534,14 @@ Get SFTP connection details for a device. **Request payload:** ```json -{"deviceId": "a1b2c3d4e5f6_..."} +{"deviceId": "a1b2c3d4e5f6_...", "showPassword": false} ``` +`showPassword` is optional and defaults to `false`. The password is a working +credential for the phone's SFTP server, so the daemon omits the field entirely +unless the client asks for it. A client that needs the credentials to mount +can take them from the `sftp.mount` event instead, which always carries them. + **Response data:** `SftpInfo` ```json @@ -585,10 +591,15 @@ Mount a device's storage at a local temporary path. **Request payload:** ```json -{"deviceId": "a1b2c3d4e5f6_..."} +{"deviceId": "a1b2c3d4e5f6_...", "readOnly": true} ``` -**Response data:** `{"path": "/tmp/kcd-sftp-abcdef123456"}` +`readOnly` is optional on every command that mounts. It is a **pointer** on the +wire: omitting it means "use the daemon's `[sftp] read_only` default", which is +distinct from an explicit `false`. A client that always sent `false` would +quietly turn a read-only default back into a writable mount. + +**Response data:** `{"path": "/run/user/1000/kcd/mnt/kcd-sftp-a1b2c3d4"}` #### `sftp_unmount` @@ -607,6 +618,8 @@ Unmount a previously mounted SFTP filesystem. Request fresh SFTP credentials and either list available storage volumes or mount a specific one. +`readOnly` behaves as on `sftp_mount_local`. + **Request payload:** - Without volume (list mode): `{"deviceId": "a1b2c3d4e5f6_..."}` @@ -622,7 +635,7 @@ fresh credentials. { "ok": true, "data": { - "path": "/home/user/Downloads/kcd/mnt/kcd-sftp-a1b2c3d4", + "path": "/run/user/1000/kcd/mnt/kcd-sftp-a1b2c3d4", "volumes": [ {"name": "Internal shared storage", "path": "/storage/emulated/0"}, {"name": "SD card", "path": "/storage/ABCD-1234"} @@ -853,7 +866,24 @@ Client authors are encouraged to adopt a similar strategy. A new device was discovered on the network. -**Payload:** `string` (the device name) +**Payload:** `DeviceInfo` — the same shape as one entry of `devices`: + +```json +{ + "id": "a1b2c3d4e5f6_...", + "name": "Pixel 9", + "type": "phone", + "state": "UNPAIRED", + "cert_fp": "", + "last_seen": "2026-05-27T10:00:00Z", + "connected": false +} +``` + +Clients should add the device directly from this payload rather than +synthesising an entry from the event envelope, which carries only the device +id. Cached sub-states (`battery`, `media`, `signal`, `sftp`) are absent here: +none have been reported at discovery time, and they arrive in their own events. #### `device.removed` @@ -1157,6 +1187,38 @@ SFTP credentials received (success) or error. {"error": "SFTP server rejected credentials"} ``` +Note that the success payload carries the live SFTP password. Subscribers to +`sftp.mount` receive a working credential and must not surface it. + +#### `sftp.mounted` + +The device's filesystem finished mounting. Fired on the mount transition, not +when credentials arrive — a client can subscribe to this alone to track mount +state. + +**Payload:** + +```json +{"mountPoint": "/run/user/1000/kcd/mnt/kcd-sftp-a1b2c3d4", "volume": "/storage/ABCD-1234"} +``` + +`volume` is present only when a specific storage volume was mounted; when the +daemon auto-selected the phone's default volume the key is absent. + +#### `sftp.unmounted` + +The device's filesystem was released. + +**Payload:** + +```json +{"mountPoint": "/run/user/1000/kcd/mnt/kcd-sftp-a1b2c3d4"} +``` + +Mount state is also available without waiting for an event: `state.snapshot` +carries a `sftp` object per mounted device, and `sftp_info` returns `mounted` +and `mountPoint`. + ### 5.10 Volume Events #### `volume.update` @@ -1258,7 +1320,47 @@ this daemon to ring). **Payload:** none (`null`) -### 5.14 MPRIS Events +### 5.14 RunCommand Events + +#### `runcommand.output` + +Output of a command the phone triggered locally, in three shapes distinguished +by `status`. + +**Payload (`status: "started"`):** + +```json +{"id": 7, "key": "uptime", "status": "started"} +``` + +**Payload (`status: "output"`)** — a batch of lines: + +```json +{"id": 7, "key": "uptime", "status": "output", "stdout": ["up 3 days"], "stderr": [], "truncated": false} +``` + +**Payload (`status: "finished"`):** + +```json +{"id": 7, "key": "uptime", "status": "finished", "success": true, "output": "up 3 days"} +``` + +`id` matches the execution id the phone sent, and `key` is the command label +from the `[commands]` config table. + +> **Batches are subscriber-gated.** `started` and `finished` are always +> published. `output` batches are published **only while at least one client is +> subscribed to `runcommand.output`**, so a chatty command does not push four +> events a second to nobody. A client that wants the transcript without +> subscribing to every batch can rely on `finished`, which carries the whole +> (capped) transcript. + +> **Caps:** output is bounded to 2000 lines and 1024 characters per line. A +> batch reports `"truncated": true` once the line cap was hit, and the +> `finished` transcript is capped separately at 4000 bytes since it is a +> summary rather than a log. + +### 5.15 MPRIS Events #### `mpris.update` diff --git a/hk.pkl b/hk.pkl index 5b1da5f..bcfe11e 100644 --- a/hk.pkl +++ b/hk.pkl @@ -17,17 +17,15 @@ steps { ["trailing_whitespace"] = Builtins.trailing_whitespace } -// The repo ships shell scripts and GitHub workflows that nothing currently -// checks. These are worth having, but the tools are not installed and CI does -// not run them, so they stay opt-in: `hk check --profile shell`. +// The repo ships shell scripts and GitHub workflows that nothing checks: CI +// runs no shell tooling, and the tools are not pinned in mise.toml, so there is +// nowhere for them to come from. Listing them here would make `hk check +// --profile shell` look configured while failing on missing tools and reporting +// nothing about the code, so they are declared only when actually available. // -// Promote one to the default set once its tool is added to mise.toml and CI. +// go_vuln_check is kept behind the "slow" profile: it works, it just takes long +// enough that it should not run on every commit. local extra = new Mapping { - ["shellcheck"] = (Builtins.shellcheck) { profiles = List("shell") } - ["shellharden"] = (Builtins.shellharden) { profiles = List("shell") } - ["shfmt"] = (Builtins.shfmt) { profiles = List("shell") } - ["actionlint"] = (Builtins.actionlint) { profiles = List("shell") } - ["zizmor"] = (Builtins.zizmor) { profiles = List("shell") } ["go_vuln_check"] = (Builtins.go_vuln_check) { profiles = List("slow") } } diff --git a/internal/config/config_store.go b/internal/config/config_store.go index b03ec45..ab61435 100644 --- a/internal/config/config_store.go +++ b/internal/config/config_store.go @@ -52,14 +52,38 @@ func (c *Config) Save(path string) error { return nil } -// StatePath returns the path to the device state file. -func StatePath() string { +// StateDir returns kcd's state directory. +func StateDir() string { stateHome := os.Getenv("XDG_STATE_HOME") if stateHome == "" { home, _ := os.UserHomeDir() stateHome = filepath.Join(home, ".local", "state") } - return filepath.Join(stateHome, "kcd", "devices.json") + return filepath.Join(stateHome, "kcd") +} + +// StatePath returns the path to the device state file. +func StatePath() string { + return filepath.Join(StateDir(), "devices.json") +} + +// DefaultMountDir returns the directory SFTP mounts are created under. +// +// XDG_RUNTIME_DIR, because location is a safety property here, not a +// convenience one. A mount makes the phone's storage reachable through the +// filesystem, so where it lives decides what can destroy it: `rm` crosses +// filesystem boundaries by default, backup tools archive $HOME, and file +// managers offer an empty-folder gesture. $XDG_RUNTIME_DIR is what GNOME's own +// remote file access uses, it is a tmpfs, and nothing routinely deletes it. +// +// The state directory is only a fallback for when there is no user session +// (a system unit, or a container without XDG_RUNTIME_DIR set). MountWarning +// covers the difference for anyone who ends up there. +func DefaultMountDir() string { + if runtimeDir := os.Getenv("XDG_RUNTIME_DIR"); runtimeDir != "" { + return filepath.Join(runtimeDir, "kcd", "mnt") + } + return filepath.Join(StateDir(), "mnt") } // DefaultConfigPath returns the default config file path. diff --git a/internal/config/plugins.go b/internal/config/plugins.go index 69d302d..7c97868 100644 --- a/internal/config/plugins.go +++ b/internal/config/plugins.go @@ -1,9 +1,6 @@ package config import ( - "os" - "path/filepath" - "github.com/bethropolis/kcd/internal/protocol" ) @@ -78,6 +75,9 @@ type SFTPConfig struct { AutoOpen bool `toml:"auto_open"` OpenCommand string `toml:"open_command"` ExtraSshfsOpts []string `toml:"extra_sshfs_opts"` + // ReadOnly mounts the device's filesystem read-only by default. --ro on a + // single mount request overrides this either way. + ReadOnly bool `toml:"read_only"` } type PingConfig struct { @@ -163,8 +163,7 @@ func (c *ShareConfig) Defaults() { } func (c *SFTPConfig) Defaults() { - home, _ := os.UserHomeDir() - c.MountDir = filepath.Join(home, "Downloads", "kcd", "mnt") + c.MountDir = DefaultMountDir() c.CredentialsTimeoutSecs = 20 c.KeepaliveIntervalSecs = 15 c.KeepaliveCount = 3 diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go index 2c860a4..5c610b6 100644 --- a/internal/daemon/daemon.go +++ b/internal/daemon/daemon.go @@ -21,6 +21,7 @@ import ( "github.com/bethropolis/kcd/internal/plugin" "github.com/bethropolis/kcd/internal/plugins/notification" "github.com/bethropolis/kcd/internal/plugins/runcommand" + "github.com/bethropolis/kcd/internal/plugins/sftp" "github.com/bethropolis/kcd/internal/protocol" ) @@ -28,6 +29,12 @@ import ( // Defaults to "dev" for local builds without ldflags. var Version = "dev" +// shutdownUnmountBudget bounds how long shutdown waits for SFTP mounts to be +// released. Each Unmount can take up to 13s on its own (3s waiting for sshfs to +// exit, then a 10s fusermount bound), so the budget -- not the per-mount cost -- +// is what keeps shutdown inside the unit's TimeoutStopSec=10. +const shutdownUnmountBudget = 5 * time.Second + // syncReconnectBroadcast starts UDP broadcast (reconnect owner) while any // paired device is offline, and withdraws it otherwise. Pure state, no // timers — callers invoke it from event handlers and startup. @@ -290,6 +297,16 @@ func Run(ctx context.Context, cfg *config.Config) error { logger.Info("kcd daemon shutting down") + // Release SFTP mounts before returning. OnDisconnect only fires on a + // dropped connection, so a graceful stop used to leave every mount live. + // Bounded well under the unit's TimeoutStopSec so systemd does not SIGKILL + // us part-way through and strand one. + if pl, ok := plugins.GetByName("SFTP"); ok { + unmountCtx, unmountCancel := context.WithTimeout(context.Background(), shutdownUnmountBudget) + pl.(*sftp.SftpPlugin).UnmountAll(unmountCtx) + unmountCancel() + } + acquires, misses := protocol.PoolStats() hits := acquires - misses logger.Debug("packet pool stats", diff --git a/internal/daemon/ipc_routes_sftp.go b/internal/daemon/ipc_routes_sftp.go index b6b7292..1ad2095 100644 --- a/internal/daemon/ipc_routes_sftp.go +++ b/internal/daemon/ipc_routes_sftp.go @@ -11,6 +11,17 @@ import ( "github.com/bethropolis/kcd/internal/plugins/sftp" ) +// readOnlyFor resolves a per-request override against the [sftp] default. +// Absent means "use the configured default", which is why the field is a +// pointer: a literal false has to be expressible, or --ro-less requests from +// an older client would quietly flip a read-only default back to writable. +func readOnlyFor(pl plugin.Plugin, override *bool) bool { + if override != nil { + return *override + } + return pl.(*sftp.SftpPlugin).ReadOnlyByDefault() +} + // resolveVolume resolves a user-supplied volume argument (index, name, or path) // against a list of StorageVolume. Returns the matching path or empty string. func resolveVolume(arg string, volumes []ipc.StorageVolumeResponse) string { @@ -49,9 +60,9 @@ func resolveVolume(arg string, volumes []ipc.StorageVolumeResponse) string { func registerSftpRoutes(handler *ipc.Handler, devices *device.Registry, plugins *plugin.Registry) { handler.Register(ipc.CmdSftpInfo, func(req ipc.Request) ipc.Response { - var p ipc.DevicePayload + var p ipc.SftpInfoPayload return pluginRoute(req, &p, plugins, "SFTP", func(pl plugin.Plugin) ipc.Response { - info := pl.(*sftp.SftpPlugin).Info(p.DeviceID) + info := pl.(*sftp.SftpPlugin).Info(p.DeviceID, p.ShowPassword) if info == nil { return ipc.Response{OK: false, Error: "no SFTP credentials cached for this device — use 'kcd sftp request' first"} } @@ -69,7 +80,7 @@ func registerSftpRoutes(handler *ipc.Handler, devices *device.Registry, plugins }) }) handler.Register(ipc.CmdSftpMount, func(req ipc.Request) ipc.Response { - var p ipc.DevicePayload + var p ipc.SftpMountPayload return deviceRoute(req, &p, devices, plugins, "SFTP", func(dev *device.Device, pl plugin.Plugin) ipc.Response { if err := pl.(*sftp.SftpPlugin).RequestMount(dev); err != nil { return ipc.Response{OK: false, Error: err.Error()} @@ -78,9 +89,9 @@ func registerSftpRoutes(handler *ipc.Handler, devices *device.Registry, plugins }) }) handler.Register(ipc.CmdSftpMountLocal, func(req ipc.Request) ipc.Response { - var p ipc.DevicePayload + var p ipc.SftpMountPayload return deviceRoute(req, &p, devices, plugins, "SFTP", func(dev *device.Device, pl plugin.Plugin) ipc.Response { - browsePath, err := pl.(*sftp.SftpPlugin).RequestAndMount(context.Background(), dev) + browsePath, err := pl.(*sftp.SftpPlugin).RequestAndMount(context.Background(), dev, readOnlyFor(pl, p.ReadOnly)) if err != nil { return ipc.Response{OK: false, Error: err.Error()} } @@ -117,7 +128,7 @@ func registerSftpRoutes(handler *ipc.Handler, devices *device.Registry, plugins } } - mountPath, volumes, err := sftpPl.RequestAndMountVolume(context.Background(), dev, volumePath) + mountPath, volumes, err := sftpPl.RequestAndMountVolume(context.Background(), dev, volumePath, readOnlyFor(pl, p.ReadOnly)) if err != nil { return ipc.Response{OK: false, Error: err.Error()} } diff --git a/internal/daemon/plugins.go b/internal/daemon/plugins.go index b7c161a..b082b78 100644 --- a/internal/daemon/plugins.go +++ b/internal/daemon/plugins.go @@ -53,7 +53,7 @@ func setupPlugins(cfg *config.Config, bus *events.Bus, tlsCfg *tls.Config, logge plugins.Register(share.NewSharePlugin(cfg.DownloadDir, cfg.Share, tlsCfg, bus, logger, sidechannel)) } if cfg.Plugins.RunCommand { - plugins.Register(runcommand.NewRunCommandPlugin(cfg.Commands, cfg.CommandsPerDevice, logger)) + plugins.Register(runcommand.NewRunCommandPlugin(cfg.Commands, cfg.CommandsPerDevice, bus, logger)) } if cfg.Plugins.Ping { plugins.Register(ping.NewPingPlugin(cfg.Ping, bus, logger, cfg.Notifications)) diff --git a/internal/daemon/transport.go b/internal/daemon/transport.go index b15e4d9..d782e2a 100644 --- a/internal/daemon/transport.go +++ b/internal/daemon/transport.go @@ -48,7 +48,6 @@ const discoveryDialMinInterval = 2 * time.Second func runTransport(ctx context.Context, cfg *tls.Config, bc *discovery.BroadcasterController, identity *protocol.Packet, devices *device.Registry, plugins *plugin.Registry, localDeviceID string, logger log.Logger, opts *config.Config) { - // TCP Listener tcpListener, err := transport.Listen(ctx, fmt.Sprintf(":%d", opts.TCPPort)) if err != nil { logger.Error("failed to start TCP listener", log.Error(err)) @@ -57,24 +56,19 @@ func runTransport(ctx context.Context, cfg *tls.Config, bc *discovery.Broadcaste defer tcpListener.Close() tcpListener.SetKeepAliveIdle(config.Duration(opts.Network.KeepAliveIdle)) - // Broadcast is off by default — controlled via `kcd pair` or IPC. - // The controller is started in stopped state. + // Broadcast is off by default — controlled via `kcd pair` or IPC, and the + // controller starts in the stopped state. - // UDP/mDNS Listener (onDeviceFound) + // Discovery is event-driven with no timers or polling. Paired devices, + // pairing mode (`kcd pair` listen) and explicit `kcd pair ` intent dial + // and keep the connection; an unpaired stranger gets exactly one ephemeral + // dial per unpaired era so both sides can list each other, and the next + // sighting closes that socket again. // - // Discovery is event-driven with no timers or polling: - // - Paired devices, pairing mode (`kcd pair` listen), and explicit - // `kcd pair ` intent dial and keep the connection. - // - An unpaired stranger gets exactly one ephemeral dial per unpaired - // era so both sides can list each other (the TCP identity exchange - // is what makes the PC appear on the phone). The next sighting - // closes the socket again while it is still unpaired. - // Ephemeral-dial rate limiting: a hostile or buggy peer minting fresh - // device IDs per broadcast could otherwise spawn an unbounded dial per - // announcement. Stranger dials are throttled globally (1/s) and per - // announcer IP (1/5s). Paired and explicit-intent dials bypass this — - // paired dials have their own per-device throttle below and intent - // dials are user-initiated. + // Stranger dials are throttled globally (1/s) and per announcer IP (1/5s): + // a hostile or buggy peer minting fresh device IDs per broadcast could + // otherwise spawn an unbounded dial per announcement. Paired and + // explicit-intent dials bypass this. var dialMu sync.Mutex lastEphemeralGlobal := time.Now().Add(-time.Minute) lastEphemeralByIP := map[string]time.Time{} @@ -121,8 +115,8 @@ func runTransport(ctx context.Context, cfg *tls.Config, bc *discovery.Broadcaste dev, known := devices.Get(body.DeviceID) if known && dev.IsConnected() { - // Fresh sighting of a connected device: close it again if it - // exists only for the discovery handshake. + // A fresh sighting of a connected device closes it again if the + // connection only ever existed for the discovery handshake. if shouldEphemeralClose(dev, pairingMode) { logger.Debug("closing ephemeral discovery connection", log.String("device_id", body.DeviceID)) @@ -135,18 +129,17 @@ func runTransport(ctx context.Context, cfg *tls.Config, bc *discovery.Broadcaste } if known && dev.State() == device.StatePaired { - // Paired sighting proves the peer is alive at the sighted - // address. Redial (rate-limited) when disconnected or when - // the sighted IP differs (true roam — the live socket is a - // half-open zombie). A same-IP sighting on a live socket is - // ignored like upstream: phone traffic is event-driven with - // long quiet gaps, so read-idle can't tell a zombie from a - // healthy session, and redialling churns duplicates the peer - // RSTs (split-second flap). Same-IP roam repair arrives via - // phone-initiated inbound, which replaces via Connect(). - // Whoever answers must present the paired certificate (CN + - // pinned fingerprint are verified in handleNewConnection) or - // setup fails. + // A paired sighting proves the peer is alive at the sighted address, + // so redial (rate-limited) when disconnected or when the sighted IP + // differs — a live socket at a new address is a half-open zombie. + // + // A same-IP sighting on a live socket is ignored, as upstream does. + // Phone traffic is event-driven with long quiet gaps, so read-idle + // cannot tell a zombie from a healthy session, and redialling churns + // duplicates the peer RSTs. Same-IP roam repair arrives instead via + // phone-initiated inbound, which replaces the connection outright. + // Whoever answers must present the paired certificate (CN + pinned + // fingerprint, verified in handleNewConnection) or setup fails. dev.SetLastSeen(time.Now()) if dev.NoteSighting(ip) { dev.ResetReconnectAttempt() diff --git a/internal/device/device_core.go b/internal/device/device_core.go index a4a9bc9..3d24fd3 100644 --- a/internal/device/device_core.go +++ b/internal/device/device_core.go @@ -27,30 +27,27 @@ type Device struct { lastSeen time.Time lastIP net.IP // cached from last successful connection; survives Disconnect - // lastPort is the tcpPort the peer last advertised over the authenticated - // (post-TLS) identity exchange. Used with lastIP as the dial target for - // paired devices so unauthenticated discovery packets can never redirect - // a paired auto-dial. Zero means unknown (fall back to the configured - // tcp_port, protocol.DefaultTCPPort by default). + // lastPort pairs with lastIP as the dial target. It is the port the peer + // advertised over the authenticated post-TLS identity exchange, so an + // unauthenticated discovery packet can never redirect a paired auto-dial. + // Zero means unknown, and callers fall back to the configured port. lastPort int - // discoveryIP/discoveryPort remember where a device was last seen - // announcing itself (UDP/mDNS), even if we never opened a TCP - // connection to it. Used to dial on explicit user request - // (e.g. `kcd pair `) without auto-dialling strangers. + // discoveryIP/discoveryPort is where the device last announced itself + // (UDP/mDNS), even with no TCP connection ever opened to it. Used to dial + // on explicit user request (`kcd pair `) without auto-dialing strangers. discoveryIP net.IP discoveryPort int - // pairDialRequested is the one-shot outbound dial trigger for an explicit - // `kcd pair ` request. It is consumed by the first discovery - // announcement after the request so the pair request can be delivered. + // pairDialRequested is the one-shot dial trigger for `kcd pair `, + // consumed by the first discovery announcement so the request can be + // delivered. Contrast pairIntentUntil, which is not consumed. pairDialRequested atomic.Bool // pairIntentUntil is the Unix-nano deadline until which an explicit pair - // intent keeps a connection alive. Unlike pairDialRequested (consumed on - // first sighting), the intent survives dial/connect cycles until pairing - // starts, is rejected, succeeds, or the deadline (pairDialIntentTTL) - // expires — so a slow phone-side accept can't downgrade into an + // intent keeps a connection alive. Unlike pairDialRequested it survives + // dial/connect cycles, until pairing starts, resolves, or pairDialIntentTTL + // expires — so a slow phone-side accept cannot downgrade into an // ephemeral-close flap. pairIntentUntil atomic.Int64 @@ -59,13 +56,12 @@ type Device struct { // (or a spoofed broadcast storm) can't cause a dial per packet. lastDiscoveryDial time.Time - // ephemeralDialed marks that this device already received its one - // ephemeral discovery dial for the current unpaired era. Ephemeral - // dials let a stranger complete the TCP identity exchange (so both - // sides list each other) without staying connected: the next sighting - // closes the socket again while the device is still unpaired. The - // marker is cleared when the device is explicitly unpaired/rejected, - // making it eligible again. Paired devices and pairing mode bypass it. + // ephemeralDialed records that this device already got its one ephemeral + // discovery dial for the current unpaired era. Such a dial lets a stranger + // finish the TCP identity exchange -- so both sides list each other -- + // without staying connected; the next sighting closes the socket again. + // Cleared when the device is explicitly unpaired or rejected. Paired + // devices and pairing mode bypass it. ephemeralDialed bool conn *transport.Conn @@ -73,19 +69,19 @@ type Device struct { done chan struct{} closeOnce sync.Once - // lastConnect marks the last completed handshake; new handshakes - // inside reconnectCooldown are refused to starve duplicate bursts. + // lastConnect marks the last completed handshake; handshakes inside + // reconnectCooldown are refused so duplicate bursts cannot starve a retry. lastConnect time.Time - // lastSightedIP remembers the previous discovery sighting so only - // confirmed roams reset the reconnect backoff (see NoteSighting). + // lastSightedIP is the previous sighting, so only confirmed roams reset + // the reconnect backoff (see NoteSighting). lastSightedIP net.IP BatteryCharge int IsCharging bool - // batterySeen marks that at least one kdeconnect.battery packet was - // received. Until then the zero values above are not measurements — - // they must not be published (a fresh pair would otherwise report a - // stable, bogus 0% that no later packet corrects at steady charge). + // batterySeen marks that a kdeconnect.battery packet has arrived. Until + // then the zero values above are not measurements and must not be + // published, or a fresh pair reports a stable bogus 0% that no later + // packet corrects at steady charge. batterySeen bool // lastBatteryAt is when the last battery packet arrived, so clients // can apply their own staleness rules (mirrors mediaAgeMs). diff --git a/internal/device/device_test.go b/internal/device/device_test.go index 18f1cad..e9e8f85 100644 --- a/internal/device/device_test.go +++ b/internal/device/device_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "github.com/bethropolis/kcd/internal/events" "github.com/bethropolis/kcd/internal/log" "github.com/bethropolis/kcd/internal/protocol" "github.com/bethropolis/kcd/internal/transport" @@ -33,6 +34,37 @@ func TestRegistry_Deduplicate(t *testing.T) { } } +// device.added used to publish only the device name, which left a client with +// nothing but the event envelope and forced it to invent a state and connected +// flag. The payload has to be the full view. +func TestRegistry_AddPublishesDeviceInfo(t *testing.T) { + bus := events.NewBus(log.Nop()) + sub := bus.Subscribe(0, events.TypeDeviceAdded) + defer sub.Close() + + reg := NewRegistry(bus) + reg.Add(NewDevice("dev1", "Pixel 8", "phone", log.Nop())) + + select { + case ev := <-sub.C: + info, ok := ev.Payload.(DeviceInfo) + if !ok { + t.Fatalf("payload is %T, want DeviceInfo", ev.Payload) + } + if info.ID != "dev1" || info.Name != "Pixel 8" || info.Type != "phone" { + t.Errorf("payload lost identity fields: %+v", info) + } + if info.State != StateUnpaired { + t.Errorf("State = %v, want %v so a client need not guess", info.State, StateUnpaired) + } + if info.Connected { + t.Error("Connected = true for a device that has never connected") + } + case <-time.After(time.Second): + t.Fatal("no device.added event published") + } +} + func TestReconnectBackoff(t *testing.T) { max := 60 * time.Second tests := []struct { diff --git a/internal/device/registry.go b/internal/device/registry.go index 096fe7f..a983c3a 100644 --- a/internal/device/registry.go +++ b/internal/device/registry.go @@ -25,7 +25,10 @@ func (r *Registry) Add(d *Device) { d.SetBus(r.bus) r.devices.Store(d.ID(), d) if r.bus != nil { - r.bus.Publish(events.TypeDeviceAdded, d.ID(), d.Name()) + // The full device view, not just the name: a client receiving this + // has nothing else to go on and would otherwise invent a state and a + // connected flag, then wait for the next snapshot to be corrected. + r.bus.Publish(events.TypeDeviceAdded, d.ID(), d.Info()) } } diff --git a/internal/device/state.go b/internal/device/state.go index a0886f8..7b2d393 100644 --- a/internal/device/state.go +++ b/internal/device/state.go @@ -89,6 +89,26 @@ type DeviceInfo struct { LastPort int `json:"last_port,omitempty"` } +// Info builds the serialisable view of a device, matching what the devices +// command and state.snapshot expose. +func (d *Device) Info() DeviceInfo { + info := DeviceInfo{ + ID: d.ID(), + Name: d.Name(), + Type: d.Type, + State: d.State(), + CertFP: d.CertFP, + LastSeen: d.LastSeen(), + Connected: d.IsConnected(), + LastPort: d.LastPort(), + } + // net.IP.String() on nil renders "" -- store empty instead. + if ip := d.LastIP(); ip != nil { + info.LastIP = ip.String() + } + return info +} + // DialTarget returns the persisted dial target for auto-dial after a // restart. A garbage IP yields nil (no target); an out-of-range port // yields 0 and callers fall back to the default port. The state file is diff --git a/internal/events/bus.go b/internal/events/bus.go index 6bd0de1..e86db98 100644 --- a/internal/events/bus.go +++ b/internal/events/bus.go @@ -12,27 +12,33 @@ type EventType string // Known event types. const ( - TypeDeviceAdded EventType = "device.added" - TypeDeviceRemoved EventType = "device.removed" - TypeDeviceConnected EventType = "device.connected" - TypeDeviceDisconnected EventType = "device.disconnected" - TypePairRequested EventType = "pair.requested" - TypePairAccepted EventType = "pair.accepted" - TypePairRejected EventType = "pair.rejected" - TypeBatteryUpdate EventType = "battery.update" - TypeBatteryThreshold EventType = "battery.threshold" - TypeNotification EventType = "notification" - TypeShareProgress EventType = "share.progress" - TypeShareComplete EventType = "share.complete" - TypeShareText EventType = "share.text" - TypeShareURL EventType = "share.url" - TypePingReceived EventType = "ping.received" - TypeTelephonyRinging EventType = "telephony.ringing" - TypeTelephonyMissed EventType = "telephony.missed" - TypeTelephonyTalking EventType = "telephony.talking" - TypeTelephonyCanceled EventType = "telephony.canceled" - TypeConnectivityUpdate EventType = "connectivity.update" - TypeSftpMount EventType = "sftp.mount" + TypeDeviceAdded EventType = "device.added" + TypeDeviceRemoved EventType = "device.removed" + TypeDeviceConnected EventType = "device.connected" + TypeDeviceDisconnected EventType = "device.disconnected" + TypePairRequested EventType = "pair.requested" + TypePairAccepted EventType = "pair.accepted" + TypePairRejected EventType = "pair.rejected" + TypeBatteryUpdate EventType = "battery.update" + TypeBatteryThreshold EventType = "battery.threshold" + TypeNotification EventType = "notification" + TypeShareProgress EventType = "share.progress" + TypeShareComplete EventType = "share.complete" + TypeShareText EventType = "share.text" + TypeShareURL EventType = "share.url" + TypePingReceived EventType = "ping.received" + TypeTelephonyRinging EventType = "telephony.ringing" + TypeTelephonyMissed EventType = "telephony.missed" + TypeTelephonyTalking EventType = "telephony.talking" + TypeTelephonyCanceled EventType = "telephony.canceled" + TypeConnectivityUpdate EventType = "connectivity.update" + TypeSftpMount EventType = "sftp.mount" + // TypeSftpMounted and TypeSftpUnmounted report mount state transitions, + // unlike TypeSftpMount which fires when credentials arrive. + TypeSftpMounted EventType = "sftp.mounted" + TypeSftpUnmounted EventType = "sftp.unmounted" + // TypeRunCommandOutput reports locally executed command output. + TypeRunCommandOutput EventType = "runcommand.output" TypeNotificationCanceled EventType = "notification.canceled" TypeVolumeUpdate EventType = "volume.update" TypeSMSIncoming EventType = "sms.incoming" diff --git a/internal/ipc/payload.go b/internal/ipc/payload.go index 63990e0..18316e1 100644 --- a/internal/ipc/payload.go +++ b/internal/ipc/payload.go @@ -4,6 +4,7 @@ package ipc // payload uniformly. Each method is a one-line accessor over the // payload's deviceId field; the JSON shapes are unchanged. func (p DevicePayload) GetDeviceID() string { return p.DeviceID } +func (p SftpMountPayload) GetDeviceID() string { return p.DeviceID } func (p SharePayload) GetDeviceID() string { return p.DeviceID } func (p NotifyReplyPayload) GetDeviceID() string { return p.DeviceID } func (p NotifyDismissPayload) GetDeviceID() string { return p.DeviceID } diff --git a/internal/ipc/proto.go b/internal/ipc/proto.go index 5d52f94..8faf382 100644 --- a/internal/ipc/proto.go +++ b/internal/ipc/proto.go @@ -170,14 +170,28 @@ type StatusResponse struct { Devices []StatusDevice `json:"devices,omitempty"` } +// SftpInfoPayload is used for CmdSftpInfo. +// +// ShowPassword is opt-in because the response carries a live credential for +// the phone's SFTP server; without it the daemon omits the password entirely. +type SftpInfoPayload struct { + DeviceID string `json:"deviceId"` + ShowPassword bool `json:"showPassword,omitempty"` +} + // SftpInfoResponse carries cached SFTP connection details returned by CmdSftpInfo. +// +// Password is omitted unless the request set ShowPassword. type SftpInfoResponse struct { IP string `json:"ip"` Port json.Number `json:"port"` User string `json:"user"` - Password string `json:"password"` + Password string `json:"password,omitempty"` Path string `json:"path"` Volumes []StorageVolumeResponse `json:"volumes,omitempty"` + Mounted bool `json:"mounted"` + // MountPoint is the local directory the device's storage is mounted at. + MountPoint string `json:"mountPoint,omitempty"` } // StorageVolumeResponse describes a single browsable storage root on a device. @@ -186,10 +200,22 @@ type StorageVolumeResponse struct { Path string `json:"path"` } +// SftpMountPayload is used for CmdSftpMount and CmdSftpMountLocal. +// +// ReadOnly overrides the [sftp] read_only default for this one mount. It has +// to be a pointer so an absent field is distinguishable from an explicit +// false, letting the daemon fall back to config rather than silently forcing +// a writable mount. +type SftpMountPayload struct { + DeviceID string `json:"deviceId"` + ReadOnly *bool `json:"readOnly,omitempty"` +} + // SftpBrowsePayload is used for CmdSftpBrowse. type SftpBrowsePayload struct { DeviceID string `json:"deviceId"` Volume string `json:"volume,omitempty"` + ReadOnly *bool `json:"readOnly,omitempty"` } // SftpBrowseResponse is returned by CmdSftpBrowse. diff --git a/internal/ipc/snapshot.go b/internal/ipc/snapshot.go index 89038ad..6aad8d2 100644 --- a/internal/ipc/snapshot.go +++ b/internal/ipc/snapshot.go @@ -7,6 +7,7 @@ import ( "github.com/bethropolis/kcd/internal/plugin" "github.com/bethropolis/kcd/internal/plugins/connectivity" "github.com/bethropolis/kcd/internal/plugins/mpris" + "github.com/bethropolis/kcd/internal/plugins/sftp" ) // BatteryStatus mirrors the battery state for embedding in summaries. @@ -31,6 +32,14 @@ type DeviceSummary struct { Battery *BatteryStatus `json:"battery,omitempty"` Media *MediaState `json:"media,omitempty"` Signal *connectivity.ConnectivityBody `json:"signal,omitempty"` + Sftp *SftpMountState `json:"sftp,omitempty"` +} + +// SftpMountState carries a device's current SFTP mount state so clients can +// render a mount toggle from a snapshot alone. +type SftpMountState struct { + Mounted bool `json:"mounted"` + MountPoint string `json:"mountPoint,omitempty"` } // SummarizeDevice builds the enriched view of one device. Absent plugins @@ -84,6 +93,14 @@ func SummarizeDevice(dev *device.Device, plugins *plugin.Registry) DeviceSummary sum.Signal = &r } } + // Only report a mount that exists. A device with no SFTP credentials + // cached and nothing mounted stays omitted rather than advertising a + // permanent "not mounted" the client has to special-case. + if pl, ok := plugins.GetByName("SFTP"); ok { + if mp := pl.(*sftp.SftpPlugin).MountedPath(dev.ID()); mp != "" { + sum.Sftp = &SftpMountState{Mounted: true, MountPoint: mp} + } + } return sum } diff --git a/internal/plugins/runcommand/list_test.go b/internal/plugins/runcommand/list_test.go index ce22d78..42e6bd6 100644 --- a/internal/plugins/runcommand/list_test.go +++ b/internal/plugins/runcommand/list_test.go @@ -53,7 +53,7 @@ func replyWith(t *testing.T, commandList string) *protocol.Packet { func TestRequestListReturnsDeviceCommands(t *testing.T) { logger := log.NewTest(t) - p := NewRunCommandPlugin(nil, nil, logger) + p := NewRunCommandPlugin(nil, nil, nil, logger) sender := &listSender{id: "dev1"} sender.onSend = func(pkt *protocol.Packet) { @@ -87,7 +87,7 @@ func TestRequestListReturnsDeviceCommands(t *testing.T) { // second `kcd run list` would be rejected as "already in flight" forever. func TestRequestListReleasesWaiterAfterReply(t *testing.T) { logger := log.NewTest(t) - p := NewRunCommandPlugin(nil, nil, logger) + p := NewRunCommandPlugin(nil, nil, nil, logger) sender := &listSender{id: "dev1"} sender.onSend = func(pkt *protocol.Packet) { @@ -112,7 +112,7 @@ func TestRequestListReleasesWaiterAfterReply(t *testing.T) { // read loop or crash on a nil channel. func TestHandleListReplyWithoutWaiterDoesNotBlock(t *testing.T) { logger := log.NewTest(t) - p := NewRunCommandPlugin(nil, nil, logger) + p := NewRunCommandPlugin(nil, nil, nil, logger) sender := &listSender{id: "dev1"} done := make(chan struct{}) @@ -133,7 +133,7 @@ func TestHandleListReplyWithoutWaiterDoesNotBlock(t *testing.T) { // A malformed list must not wedge the waiter or panic. func TestHandleListReplyMalformedIsDropped(t *testing.T) { logger := log.NewTest(t) - p := NewRunCommandPlugin(nil, nil, logger) + p := NewRunCommandPlugin(nil, nil, nil, logger) sender := &listSender{id: "dev1"} if err := p.Handle(context.Background(), sender, replyWith(t, `not json`)); err != nil { @@ -175,7 +175,7 @@ func TestParseCommandListFallsBackToKey(t *testing.T) { // than race for the single reply. func TestRequestListRejectsConcurrentRequest(t *testing.T) { logger := log.NewTest(t) - p := NewRunCommandPlugin(nil, nil, logger) + p := NewRunCommandPlugin(nil, nil, nil, logger) // No onSend: the first request parks until its context is cancelled. sender := &listSender{id: "dev1"} diff --git a/internal/plugins/runcommand/output.go b/internal/plugins/runcommand/output.go index c54a394..f555c9b 100644 --- a/internal/plugins/runcommand/output.go +++ b/internal/plugins/runcommand/output.go @@ -10,6 +10,7 @@ import ( "time" "github.com/bethropolis/kcd/internal/device" + "github.com/bethropolis/kcd/internal/events" "github.com/bethropolis/kcd/internal/log" "github.com/bethropolis/kcd/internal/protocol" ) @@ -54,6 +55,7 @@ type outputStream struct { mu sync.Mutex dev device.Sender logger log.Logger + bus *events.Bus id int32 command string @@ -67,6 +69,9 @@ type outputStream struct { // dropped records that the line cap was hit, so the final batch can say so // rather than silently ending early. dropped bool + // announced records that the started event went out, so the paired + // finished event is not published for an execution that never began. + announced bool // stopped records that the execution was cancelled, so the finish packet // reports failure even if the process happened to exit zero. stopped bool @@ -159,12 +164,44 @@ func (s *outputStream) send(stdout, stderr []string, dropped bool) { // flush sends a batch only when lines are buffered, so an idle command does not // generate traffic. +// +// The bus copy is gated on subscribers. A chatty command would otherwise +// publish four events a second to nobody, and the bus drops events (with a +// warning) once a slow subscriber fills its channel -- so an unfiltered feed +// would both waste work and make the drop warnings fire for output nobody +// asked for. The lifecycle events are unconditional, so a client can still +// learn the result of a command it was not watching line by line. func (s *outputStream) flush() { if !s.pending() { return } stdout, stderr, dropped := s.take() s.send(stdout, stderr, dropped) + + if s.bus == nil || !s.bus.HasSubscribers(events.TypeRunCommandOutput) { + return + } + s.bus.Publish(events.TypeRunCommandOutput, s.dev.ID(), map[string]any{ + "id": s.id, + "key": s.command, + "status": "output", + "stdout": stdout, + "stderr": stderr, + "truncated": dropped, + }) +} + +// publishLifecycle announces a start or finish. Unconditional: these are two +// events per command and carry the result a client needs most. +func (s *outputStream) publishLifecycle(status string, extra map[string]any) { + if s.bus == nil { + return + } + payload := map[string]any{"id": s.id, "key": s.command, "status": status} + for k, v := range extra { + payload[k] = v + } + s.bus.Publish(events.TypeRunCommandOutput, s.dev.ID(), payload) } // line is one scanned line tagged with the stream it came from. @@ -191,13 +228,29 @@ func (p *RunCommandPlugin) streamOutput( // The finish packet reports the outcome, so it is always sent exactly // once, including on the early-return paths below. var success bool + // summary is filled in on the normal exit path and read by the deferred + // publish, so the transcript is rendered once rather than twice. + var summary string defer func() { if stream != nil && stream.stopped { success = false } + // Only for an execution that actually started, so a client never sees + // a finish with no matching start. + if stream != nil && stream.announced { + stream.publishLifecycle("finished", map[string]any{ + "success": success, + "output": summary, + }) + } p.finishOutput(dev, id, success) }() + // Built before the pipes exist so the deferred finish can still find it on + // the early-return paths; announced stays false until the process is + // actually running, so a failed start publishes neither half. + stream = &outputStream{dev: dev, logger: p.logger, bus: p.bus, id: id, command: key} + stdoutPipe, err := cmd.StdoutPipe() if err != nil { p.logger.Warn("runcommand: stdout pipe", log.Error(err)) @@ -212,8 +265,8 @@ func (p *RunCommandPlugin) streamOutput( p.logger.Warn("runcommand: start", log.Error(err)) return "" } - - stream = &outputStream{dev: dev, logger: p.logger, id: id, command: key} + stream.announced = true + stream.publishLifecycle("started", nil) // exec.CommandContext kills only the direct child. If the shell forks // rather than execs, the orphan keeps the write end of the pipes open, so @@ -275,7 +328,8 @@ func (p *RunCommandPlugin) streamOutput( waitErr := cmd.Wait() success = waitErr == nil - return stream.summary() + summary = stream.summary() + return summary } // drainLines consumes everything currently buffered and reports whether the diff --git a/internal/plugins/runcommand/output_test.go b/internal/plugins/runcommand/output_test.go index e8a370f..2c4a475 100644 --- a/internal/plugins/runcommand/output_test.go +++ b/internal/plugins/runcommand/output_test.go @@ -5,11 +5,13 @@ import ( "crypto/x509" "encoding/json" "net" + "slices" "sync" "testing" "time" "github.com/bethropolis/kcd/internal/device" + "github.com/bethropolis/kcd/internal/events" "github.com/bethropolis/kcd/internal/log" "github.com/bethropolis/kcd/internal/protocol" ) @@ -83,7 +85,11 @@ func runKey(t *testing.T, p *RunCommandPlugin, dev *outputSender, key string) { } func newTestPlugin(commands map[string]string) *RunCommandPlugin { - return NewRunCommandPlugin(commands, nil, log.Nop()) + return NewRunCommandPlugin(commands, nil, nil, log.Nop()) +} + +func newTestPluginWithBus(commands map[string]string, bus *events.Bus) *RunCommandPlugin { + return NewRunCommandPlugin(commands, nil, bus, log.Nop()) } // The phone keys its output rows off the id registered by commandStarted, and @@ -475,3 +481,84 @@ func TestStopCancelsCommandThatForksChildren(t *testing.T) { t.Fatal("stop did not cancel a forking command") } } + +// drainEvents collects published payloads for the given event type. +func drainEvents(t *testing.T, sub *events.Subscriber) []map[string]any { + t.Helper() + var out []map[string]any + for { + select { + case ev := <-sub.C: + payload, ok := ev.Payload.(map[string]any) + if !ok { + t.Fatalf("payload is %T, want map[string]any", ev.Payload) + } + out = append(out, payload) + case <-time.After(150 * time.Millisecond): + return out + } + } +} + +func statuses(evs []map[string]any) []string { + got := make([]string, 0, len(evs)) + for _, e := range evs { + s, _ := e["status"].(string) + got = append(got, s) + } + return got +} + +// With a subscriber attached, output must stream as it arrives -- that is the +// whole point of the gated publish. +func TestOutputStream_PublishesBatchesWhenSubscribed(t *testing.T) { + bus := events.NewBus(log.Nop()) + sub := bus.Subscribe(0, events.TypeRunCommandOutput) + defer sub.Close() + + p := newTestPluginWithBus(map[string]string{"hello": "echo one; echo two"}, bus) + dev := &outputSender{id: "dev1"} + runKey(t, p, dev, "hello") + + evs := drainEvents(t, sub) + got := statuses(evs) + if got[0] != "started" { + t.Errorf("first event = %q, want started", got[0]) + } + if got[len(got)-1] != "finished" { + t.Errorf("last event = %q, want finished", got[len(got)-1]) + } + if len(got) < 3 { + t.Fatalf("expected at least started/output/finished, got %v", got) + } + if !slices.Contains(got, "output") { + t.Errorf("no output batch was published while subscribed, got %v", got) + } + if evs[len(evs)-1]["success"] != true { + t.Errorf("finished event success = %v, want true", evs[len(evs)-1]["success"]) + } +} + +// The batch gate asks HasSubscribers, so what matters is that an unrelated +// filter does not open it. There is no way to observe a *suppressed* publish +// from outside -- the bus drops anything the subscriber did not ask for, which +// is exactly the same observable outcome -- so this pins the gate's input. +func TestRunCommandOutputGate_FollowsSubscribers(t *testing.T) { + bus := events.NewBus(log.Nop()) + + if bus.HasSubscribers(events.TypeRunCommandOutput) { + t.Error("gate open with no subscribers at all") + } + + other := bus.Subscribe(0, events.TypeBatteryUpdate) + defer other.Close() + if bus.HasSubscribers(events.TypeRunCommandOutput) { + t.Error("gate opened by a subscriber filtered on another event type") + } + + watcher := bus.Subscribe(0, events.TypeRunCommandOutput) + defer watcher.Close() + if !bus.HasSubscribers(events.TypeRunCommandOutput) { + t.Error("gate closed while a runcommand.output subscriber is attached") + } +} diff --git a/internal/plugins/runcommand/runcommand.go b/internal/plugins/runcommand/runcommand.go index eb47cbc..0890198 100644 --- a/internal/plugins/runcommand/runcommand.go +++ b/internal/plugins/runcommand/runcommand.go @@ -10,6 +10,7 @@ import ( "time" "github.com/bethropolis/kcd/internal/device" + "github.com/bethropolis/kcd/internal/events" "github.com/bethropolis/kcd/internal/log" "github.com/bethropolis/kcd/internal/protocol" ) @@ -30,11 +31,12 @@ type RunCommandPlugin struct { // timestamp because the phone reads the id with getInt, which would // overflow on a nanosecond value. execSeq int32 + bus *events.Bus logger log.Logger wg sync.WaitGroup // exported for tests to synchronize with background goroutines } -func NewRunCommandPlugin(commands map[string]string, commandsPerDevice map[string]map[string]string, logger log.Logger) *RunCommandPlugin { +func NewRunCommandPlugin(commands map[string]string, commandsPerDevice map[string]map[string]string, bus *events.Bus, logger log.Logger) *RunCommandPlugin { if commandsPerDevice == nil { commandsPerDevice = make(map[string]map[string]string) } @@ -43,6 +45,7 @@ func NewRunCommandPlugin(commands map[string]string, commandsPerDevice map[strin CommandsPerDevice: commandsPerDevice, pendingLists: make(map[string]chan []Command), running: make(map[string]map[int32]context.CancelFunc), + bus: bus, logger: logger.With(log.String("plugin", "runcommand")), } } diff --git a/internal/plugins/runcommand/runcommand_test.go b/internal/plugins/runcommand/runcommand_test.go index 5378602..aee60d6 100644 --- a/internal/plugins/runcommand/runcommand_test.go +++ b/internal/plugins/runcommand/runcommand_test.go @@ -15,6 +15,7 @@ func TestRunCommandPlugin_Handle_GlobalCommand(t *testing.T) { p := NewRunCommandPlugin( map[string]string{"key1": "echo test"}, nil, + nil, logger, ) @@ -33,6 +34,7 @@ func TestRunCommandPlugin_Handle_PerDeviceOverrides(t *testing.T) { map[string]map[string]string{ "dev1": {"cmd": "echo per-device"}, }, + nil, logger, ) @@ -61,6 +63,7 @@ func TestRunCommandPlugin_Handle_RequestCommandList(t *testing.T) { map[string]map[string]string{ "dev1": {"device-only": "echo dev-only"}, }, + nil, logger, ) diff --git a/internal/plugins/sftp/info_test.go b/internal/plugins/sftp/info_test.go new file mode 100644 index 0000000..99c3da1 --- /dev/null +++ b/internal/plugins/sftp/info_test.go @@ -0,0 +1,33 @@ +package sftp + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/bethropolis/kcd/internal/config" + "github.com/bethropolis/kcd/internal/log" +) + +func TestInfo_PasswordIsOptIn(t *testing.T) { + p := NewSftpPlugin(config.SFTPConfig{}, nil, log.NewTest(t)) + p.lastBody["dev1"] = SftpBody{IP: "192.168.1.42", User: "u0_a123", Password: "hunter2", Path: "/storage/emulated/0"} + + if got := p.Info("dev1", false).Password; got != "" { + t.Errorf("password leaked with includePassword=false: %q", got) + } + + // Absent rather than empty, so a masked response stays distinguishable + // from a device whose SFTP server genuinely has no password. + raw, err := json.Marshal(p.Info("dev1", false)) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(raw), "password") { + t.Errorf("password key present in masked JSON: %s", raw) + } + + if got := p.Info("dev1", true).Password; got != "hunter2" { + t.Errorf("includePassword=true returned %q, want %q", got, "hunter2") + } +} diff --git a/internal/plugins/sftp/mount.go b/internal/plugins/sftp/mount.go index d55beab..d689fa4 100644 --- a/internal/plugins/sftp/mount.go +++ b/internal/plugins/sftp/mount.go @@ -10,10 +10,12 @@ import ( "regexp" "strconv" "strings" + "sync" "syscall" "time" "github.com/bethropolis/kcd/internal/device" + "github.com/bethropolis/kcd/internal/events" "github.com/bethropolis/kcd/internal/log" "github.com/bethropolis/kcd/internal/plugin" ) @@ -34,7 +36,7 @@ var sshHostPattern = regexp.MustCompile(`^[A-Za-z0-9]([A-Za-z0-9.-]*[A-Za-z0-9]) // separator — unsupported by older sshfs 2.x) is what prevents option // injection: no validated value can begin with '-', so sshfs/fuse option // parsing can never reinterpret remoteRoot as a flag like -oProxyCommand. -func buildSSHFSArgs(body SftpBody, remotePath, mountPoint string, uid, gid int, keepaliveInterval, keepaliveCount int, extraOpts []string) ([]string, error) { +func buildSSHFSArgs(body SftpBody, remotePath, mountPoint string, uid, gid int, keepaliveInterval, keepaliveCount int, extraOpts []string, readOnly bool) ([]string, error) { if !sshUserPattern.MatchString(body.User) || len(body.User) > 64 { return nil, fmt.Errorf("sftp: refusing suspicious ssh user %q", body.User) } @@ -72,6 +74,12 @@ func buildSSHFSArgs(body SftpBody, remotePath, mountPoint string, uid, gid int, "-o", "gid=" + strconv.Itoa(gid), } + // Read-only is set before ExtraSshfsOpts so an explicit operator override + // in the config can still force a writable mount. + if readOnly { + args = append(args, "-o", "ro") + } + // ExtraSshfsOpts comes from the local operator config, not the phone — // passed through as-is. for _, opt := range extraOpts { @@ -80,15 +88,63 @@ func buildSSHFSArgs(body SftpBody, remotePath, mountPoint string, uid, gid int, return args, nil } +// sshfsHint maps an sshfs failure message to an actionable hint, or "" when +// the cause is not one we can advise about. +// +// The FUSE hint is deliberately narrow. It used to fire on any message +// containing "fusermount", which also matched errors that have nothing to do +// with /etc/fuse.conf — most visibly re-running sshfs onto an already live +// mountpoint, whose message names fusermount3 but is caused by the duplicate +// mount. Telling a user to edit fuse.conf there sends them down a dead end. +// mountWithBody is now idempotent, but precision here is still worth having. +func sshfsHint(msg string) string { + switch { + case strings.Contains(msg, "Operation not permitted"), + strings.Contains(msg, "user_allow_other"), + strings.Contains(msg, "Permission denied"): + return "Hint: FUSE requires user_allow_other in /etc/fuse.conf.\nRun: sudo sed -i 's/^#user_allow_other/user_allow_other/' /etc/fuse.conf" + case strings.Contains(msg, "sshfs: not found"), + strings.Contains(msg, "executable file not found"): + return "Hint: sshfs is not installed.\nInstall: sudo apt install sshfs (or the equivalent for your distro)" + default: + return "" + } +} + // mountWithBody performs the sshfs mount and returns the local browse path. // volumePath specifies which storage volume to mount. If empty, the first // available volume is selected automatically. -func (p *SftpPlugin) mountWithBody(ctx context.Context, deviceID string, body SftpBody, volumePath string) (string, error) { - baseDir := p.cfg.MountDir - if baseDir == "" { - baseDir = os.TempDir() +func (p *SftpPlugin) mountWithBody(ctx context.Context, deviceID string, body SftpBody, volumePath string, readOnly bool) (string, error) { + mountPoint := p.mountPointFor(deviceID) + // Warned before the reuse check as well as after it: a user who already + // has a mount at a hazardous location still needs telling, and it is + // logged once per location, so hoisting it costs nothing. + p.warnIfInBulkDeletableDir(mountPoint) + + // Idempotent. Re-running sshfs onto a live mountpoint fails with + // "fusermount3: failed to access mountpoint ... Permission denied", which + // reads like a FUSE permissions problem and is not one. Returning the + // existing mount point also makes a repeated request a cheap way to + // re-open the file manager. + if existing := p.MountedPath(deviceID); existing != "" { + p.logger.Info("SFTP already mounted, reusing mount point", + log.String("device_id", deviceID), + log.String("mount_point", existing), + ) + // Mounting is idempotent, so a mode request cannot be applied to an + // existing mount. Say what the mount actually is rather than appearing + // to honour the flag. + if existingRO := p.mountIsReadOnly(deviceID); existingRO != readOnly { + p.logger.Info("read-only mode differs from the existing mount and cannot be changed in place; unmount first", + log.String("device_id", deviceID), + log.Bool("existing_read_only", existingRO), + log.Bool("requested_read_only", readOnly), + ) + } + p.autoOpen(existing) + return existing, nil } - mountPoint := filepath.Join(baseDir, "kcd-sftp-"+deviceID) + if err := os.MkdirAll(mountPoint, 0700); err != nil { return "", fmt.Errorf("create mount point %s: %w", mountPoint, err) } @@ -108,7 +164,7 @@ func (p *SftpPlugin) mountWithBody(ctx context.Context, deviceID string, body Sf remotePath = body.Path } } - args, err := buildSSHFSArgs(body, remotePath, mountPoint, os.Getuid(), os.Getgid(), p.cfg.KeepaliveIntervalSecs, p.cfg.KeepaliveCount, p.cfg.ExtraSshfsOpts) + args, err := buildSSHFSArgs(body, remotePath, mountPoint, os.Getuid(), os.Getgid(), p.cfg.KeepaliveIntervalSecs, p.cfg.KeepaliveCount, p.cfg.ExtraSshfsOpts, readOnly) if err != nil { _ = os.Remove(mountPoint) return "", err @@ -121,10 +177,8 @@ func (p *SftpPlugin) mountWithBody(ctx context.Context, deviceID string, body Sf _ = os.Remove(mountPoint) msg := strings.TrimSpace(string(out)) errMsg := fmt.Sprintf("sshfs failed: %v\n%s", err, msg) - if strings.Contains(msg, "Operation not permitted") || strings.Contains(msg, "fusermount") { - errMsg += "\n\nHint: FUSE requires user_allow_other in /etc/fuse.conf.\nRun: sudo sed -i 's/^#user_allow_other/user_allow_other/' /etc/fuse.conf" - } else if strings.Contains(msg, "sshfs: not found") || strings.Contains(msg, "executable file not found") { - errMsg += "\n\nHint: sshfs is not installed.\nInstall: sudo apt install sshfs (or the equivalent for your distro)" + if hint := sshfsHint(msg); hint != "" { + errMsg += "\n\n" + hint } return "", fmt.Errorf("%s", errMsg) } @@ -133,9 +187,11 @@ func (p *SftpPlugin) mountWithBody(ctx context.Context, deviceID string, body Sf // directly to the mount point — no extra navigation needed. browsePath := mountPoint - // Track the mount point so Unmount() can call fusermount. + // Track the mount point so Unmount() can call fusermount, and its mode so a + // later --ro request against this mount can report the truth. p.mu.Lock() p.mountPoints[deviceID] = mountPoint + p.mountReadOnly[deviceID] = readOnly p.mu.Unlock() // Find and track the sshfs daemon PID for graceful shutdown. @@ -152,32 +208,58 @@ func (p *SftpPlugin) mountWithBody(ctx context.Context, deviceID string, body Sf log.String("mount_point", mountPoint), log.String("browse_path", browsePath), ) + p.publishMounted(deviceID, mountPoint, volumePath) - // Open in the default file manager (best effort, non-blocking). - if p.cfg.AutoOpen { - go func() { - cmd := p.cfg.OpenCommand - if cmd == "" { - cmd = "xdg-open" - } - if err := exec.CommandContext(context.Background(), cmd, browsePath).Start(); err != nil { - p.logger.Debug("auto-open failed", log.String("command", cmd), log.Error(err)) - } - }() - } + p.autoOpen(browsePath) return browsePath, nil } +// publishMounted announces a completed mount so clients can render a truthful +// mount toggle without inspecting /proc themselves. +func (p *SftpPlugin) publishMounted(deviceID, mountPoint, volumePath string) { + if p.bus == nil { + return + } + payload := map[string]any{"mountPoint": mountPoint} + if volumePath != "" { + payload["volume"] = volumePath + } + p.bus.Publish(events.TypeSftpMounted, deviceID, payload) +} + +func (p *SftpPlugin) publishUnmounted(deviceID, mountPoint string) { + if p.bus == nil { + return + } + p.bus.Publish(events.TypeSftpUnmounted, deviceID, map[string]any{"mountPoint": mountPoint}) +} + +// autoOpen opens a browse path in the configured file manager, best effort. +// It runs the spawn in a goroutine so a slow or hung file manager never +// blocks the caller's mount or unmount path. +func (p *SftpPlugin) autoOpen(browsePath string) { + if !p.cfg.AutoOpen { + return + } + cmd := p.cfg.OpenCommand + if cmd == "" { + cmd = "xdg-open" + } + go func() { + if err := exec.CommandContext(context.Background(), cmd, browsePath).Start(); err != nil { + p.logger.Debug("auto-open failed", log.String("command", cmd), log.Error(err)) + } + }() +} + func (p *SftpPlugin) OnConnect(_ device.Sender) {} func (p *SftpPlugin) OnDisconnect(dev device.Sender) { - p.mu.Lock() deviceID := dev.ID() - _, mounted := p.mountPoints[deviceID] - p.mu.Unlock() - - if mounted { + // Via MountedPath, so a mount adopted from the kernel is cleaned up too + // rather than surviving every disconnect untouched. + if p.MountedPath(deviceID) != "" { p.logger.Info("device disconnected, cleaning up SFTP mount", log.String("device_id", deviceID), ) @@ -195,29 +277,71 @@ func (p *SftpPlugin) OnDisconnect(dev device.Sender) { p.mu.Unlock() } +// mountIsReadOnly reports whether the device's existing mount is read-only. +// False for a mount kcd did not create or no longer tracks. +func (p *SftpPlugin) mountIsReadOnly(deviceID string) bool { + p.mu.RLock() + defer p.mu.RUnlock() + return p.mountReadOnly[deviceID] +} + // Unmount cleanly unmounts a previously mounted SFTP filesystem. // It first attempts a graceful shutdown of the sshfs process (SIGTERM → wait → SIGKILL), // then uses fusermount to ensure the mount point is released. -// Returns an error if the device was never mounted. +// +// Errors distinguish the three cases a client has to react to differently: +// not mounted at all, a mount that exists but could not be released, and a +// clean unmount. Tracked state is dropped only once the mount is actually +// released, so a failed unmount can be retried instead of leaving the daemon +// believing the device is unmounted while the FUSE mount is still live. func (p *SftpPlugin) Unmount(deviceID string) error { - p.mu.Lock() - mountPoint, ok := p.mountPoints[deviceID] - if ok { - delete(p.mountPoints, deviceID) - } - pid, hasPID := p.mountPIDs[deviceID] - if hasPID { - delete(p.mountPIDs, deviceID) + // Resolved rather than read from the map directly: a mount made before a + // daemon restart is absent from the cache but still live, and reporting + // "not mounted" there would leave it holding an sshfs process with no way + // to clear it. + mountPoint := p.MountedPath(deviceID) + if mountPoint == "" { + // Nothing mounted. A leftover directory can still be sitting there + // from a mount whose release already succeeded, so clear it and report + // the state the caller asked for rather than an error. + if leftover := p.mountPointFor(deviceID); p.removeLeftoverDir(deviceID, leftover) { + p.logger.Info("removed leftover SFTP mount directory", + log.String("device_id", deviceID), + log.String("mount_point", leftover), + ) + } + return fmt.Errorf("not mounted: no SFTP mount for device %s", deviceID) } - p.mu.Unlock() - if !ok { - return fmt.Errorf("no active SFTP mount for device %s", deviceID) + // The kernel is the source of truth. A mount that has already gone -- the + // FUSE connection died, or it was released by hand -- has nothing to + // release, and fusermount will fail on it forever. Treating that as an + // error while keeping the cached state would wedge the device permanently + // in "mounted", unable to mount or unmount again. + if !mountExists(mountPoint) { + p.logger.Info("SFTP mount already released, clearing stale state", + log.String("device_id", deviceID), + log.String("mount_point", mountPoint), + ) + p.finishUnmount(deviceID, mountPoint) + return nil } + p.mu.RLock() + pid := p.mountPIDs[deviceID] + p.mu.RUnlock() + p.logger.Info("unmounting SFTP share", log.String("mount_point", mountPoint)) - // Graceful shutdown: SIGTERM → wait → SIGKILL. + // Graceful shutdown: SIGTERM → wait → SIGKILL. A mount adopted from the + // kernel has no tracked PID, so look one up rather than skipping the + // graceful step. + hasPID := pid != 0 + if !hasPID { + if found, err := findSSHFSPID(mountPoint); err == nil { + pid, hasPID = found, true + } + } if hasPID { p.logger.Debug("sending SIGTERM to sshfs", log.Int("pid", pid)) proc, err := os.FindProcess(pid) @@ -247,17 +371,87 @@ func (p *SftpPlugin) Unmount(deviceID string) error { } unmountCtx, unmountCancel := context.WithTimeout(context.Background(), 10*time.Second) defer unmountCancel() - if out, err := plugin.RunCommandSync(unmountCtx, tool, "-u", mountPoint); err != nil { - p.logger.Warn("fusermount cleanup failed", + out, err := plugin.RunCommandSync(unmountCtx, tool, "-u", mountPoint) + if err == nil { + p.finishUnmount(deviceID, mountPoint) + return nil + } + + detail := strings.TrimSpace(string(out)) + + // It may have gone while we were killing the sshfs process. + if !mountExists(mountPoint) { + p.logger.Info("SFTP mount released during unmount", + log.String("mount_point", mountPoint), + log.String("output", detail), + ) + p.finishUnmount(deviceID, mountPoint) + return nil + } + + // Still present but not responding: a dead FUSE connection refuses a + // normal unmount ("Transport endpoint is not connected"). A lazy unmount + // detaches it anyway, and any process still inside will see ENOTCONN + // rather than hang, which is the state a crashed mount is in regardless. + if strings.Contains(detail, "Transport endpoint is not connected") { + lazyOut, lazyErr := plugin.RunCommandSync(unmountCtx, tool, "-uz", mountPoint) + if lazyErr == nil { + p.logger.Info("SFTP mount force-detached with a lazy unmount", + log.String("mount_point", mountPoint), + ) + p.finishUnmount(deviceID, mountPoint) + return nil + } + p.logger.Warn("lazy unmount failed", log.String("mount_point", mountPoint), - log.Error(err), - log.String("output", strings.TrimSpace(string(out))), + log.Error(lazyErr), + log.String("output", strings.TrimSpace(string(lazyOut))), ) + detail = detail + " (lazy unmount also failed: " + strings.TrimSpace(string(lazyOut)) + ")" + } + + // Keep the tracked state: the mount is genuinely still there, and the + // caller needs a retry to clear it. + p.logger.Warn("fusermount failed", + log.String("mount_point", mountPoint), + log.Error(err), + log.String("output", detail), + ) + if detail != "" { + detail = ": " + detail } + return fmt.Errorf("stale SFTP mount at %s could not be released, %s failed: %v%s", + mountPoint, tool, err, detail) +} + +// finishUnmount clears tracked state and removes the mount directory. Called +// once the mount is confirmed released. +func (p *SftpPlugin) finishUnmount(deviceID, mountPoint string) { + p.mu.Lock() + delete(p.mountPoints, deviceID) + delete(p.mountPIDs, deviceID) + delete(p.mountReadOnly, deviceID) + p.mu.Unlock() _ = os.Remove(mountPoint) p.logger.Info("SFTP unmounted", log.String("mount_point", mountPoint)) - return nil + p.publishUnmounted(deviceID, mountPoint) +} + +// removeLeftoverDir removes a mount directory left behind by a mount that is +// no longer in the kernel, reporting whether there was one. +func (p *SftpPlugin) removeLeftoverDir(deviceID, mountPoint string) bool { + if _, err := os.Stat(mountPoint); err != nil { + return false + } + p.mu.Lock() + delete(p.mountPoints, deviceID) + delete(p.mountPIDs, deviceID) + delete(p.mountReadOnly, deviceID) + p.mu.Unlock() + // Best effort: a non-empty or busy directory just stays, and the error + // above still tells the caller nothing is mounted. + return os.Remove(mountPoint) == nil } // findSSHFSPID scans /proc to find the sshfs daemon PID for a given mount point. @@ -286,3 +480,69 @@ func findSSHFSPID(mountPoint string) (int, error) { } return 0, fmt.Errorf("no sshfs process found for mount point %s", mountPoint) } + +// UnmountAll releases every mount the plugin is tracking, and returns once +// they are all released or ctx is done. +// +// Shutdown has to do this. OnDisconnect only fires when a connection drops, so +// a graceful stop used to leave every mount live -- and with the mount +// directory under $XDG_STATE_HOME in the no-session fallback, `uninstall.sh +// --purge`'s `rm -rf` would then descend into a live mount and delete the +// phone's files. Even with the runtime-dir default, leaving a mount behind on +// every restart is not a reasonable way to end. +// +// Tracked devices are the source for the list rather than the device registry: +// this is the plugin's own record of what it believes is mounted, including +// mounts adopted from the kernel earlier in the session. +// +// Unmounts run concurrently because each can take up to 13s (3s waiting for +// sshfs to exit, then a 10s fusermount bound) and a serial loop over several +// devices would overrun the unit's TimeoutStopSec. +// +// ctx bounds how long *this call* waits, not the work itself: on expiry it +// returns and the in-flight unmounts carry on in the background, each still +// bounded by its own timeouts. That is the right trade only because the sole +// caller is shutdown, where the process exits immediately afterwards and +// systemd would SIGKILL the stragglers regardless. A caller that intends to keep +// running must not treat an early return as "unmounted". +func (p *SftpPlugin) UnmountAll(ctx context.Context) { + p.mu.RLock() + ids := make([]string, 0, len(p.mountPoints)) + for id := range p.mountPoints { + ids = append(ids, id) + } + p.mu.RUnlock() + + if len(ids) == 0 { + return + } + p.logger.Info("releasing SFTP mounts on shutdown", log.Int("count", len(ids))) + + var wg sync.WaitGroup + for _, id := range ids { + wg.Add(1) + go func(deviceID string) { + defer wg.Done() + if err := p.Unmount(deviceID); err != nil { + p.logger.Warn("could not release SFTP mount on shutdown", + log.String("device_id", deviceID), + log.Error(err), + ) + } + }(id) + } + + done := make(chan struct{}) + go func() { + wg.Wait() + close(done) + }() + + select { + case <-done: + case <-ctx.Done(): + p.logger.Warn("timed out releasing SFTP mounts; some may survive this shutdown", + log.Error(ctx.Err()), + ) + } +} diff --git a/internal/plugins/sftp/mount_test.go b/internal/plugins/sftp/mount_test.go new file mode 100644 index 0000000..83c6462 --- /dev/null +++ b/internal/plugins/sftp/mount_test.go @@ -0,0 +1,334 @@ +package sftp + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/bethropolis/kcd/internal/config" + "github.com/bethropolis/kcd/internal/events" + "github.com/bethropolis/kcd/internal/log" +) + +func newTestPlugin(t *testing.T, mountDir string) *SftpPlugin { + t.Helper() + cfg := config.SFTPConfig{} + cfg.MountDir = mountDir + return NewSftpPlugin(cfg, nil, log.NewTest(t)) +} + +// Mounting an already-mounted device must not reach sshfs. Verified without a +// real sshfs by pointing an existing mount entry at the path: a second mount +// returns that path instead of trying to exec over it. +func TestMountWithBody_IsIdempotent(t *testing.T) { + dir := t.TempDir() + p := newTestPlugin(t, dir) + + existing := filepath.Join(dir, "kcd-sftp-dev1") + if err := os.MkdirAll(existing, 0700); err != nil { + t.Fatal(err) + } + p.mountPoints["dev1"] = existing + + body := SftpBody{IP: "192.168.1.42", Port: "1776", User: "u0_a123", Password: "x", Path: "/storage/emulated/0"} + + got, err := p.mountWithBody(context.Background(), "dev1", body, "", false) + if err != nil { + t.Fatalf("second mount should succeed by reusing the mount point, got: %v", err) + } + if got != existing { + t.Errorf("second mount returned %q, want the existing mount point %q", got, existing) + } +} + +func TestUnmount_NotMountedIsDistinguishable(t *testing.T) { + p := newTestPlugin(t, t.TempDir()) + + err := p.Unmount("never-mounted") + if err == nil { + t.Fatal("expected an error for an unmounted device") + } + if !strings.Contains(err.Error(), "not mounted") { + t.Errorf("error should say 'not mounted' so clients can react to it, got: %v", err) + } +} + +func TestSshfsHint(t *testing.T) { + tests := []struct { + name string + msg string + wantSub string // "" means no hint expected + }{ + { + name: "fuse permission denied", + msg: "fusermount3: failed to access mountpoint /mnt/x: Permission denied", + wantSub: "/etc/fuse.conf", + }, + { + name: "operation not permitted", + msg: "fusermount: mount failed: Operation not permitted", + wantSub: "/etc/fuse.conf", + }, + { + name: "sshfs binary missing", + msg: `exec: "sshfs": executable file not found in $PATH`, + wantSub: "sshfs is not installed", + }, + { + // Textually indistinguishable from the fuse.conf case above, and + // genuinely ambiguous on its own. mountWithBody is now idempotent, + // so this message is no longer produced by a duplicate mount -- + // the ambiguity is resolved upstream rather than by the hint. + name: "permission denied on mountpoint", + msg: "fusermount3: failed to access mountpoint /mnt/kcd-sftp-dev1: Permission denied", + wantSub: "/etc/fuse.conf", + }, + { + // Names fusermount3 but is not a fuse.conf problem: this is the + // class of error the old substring match mis-advised on. + name: "unrelated fusermount error", + msg: "fusermount3: entry for /mnt/kcd-sftp-dev1 is not a directory", + wantSub: "", + }, + { + name: "unknown failure", + msg: "read: Connection reset by peer", + wantSub: "", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := sshfsHint(tc.msg) + if tc.wantSub == "" { + if got != "" { + t.Errorf("expected no hint, got:\n%s", got) + } + return + } + if !strings.Contains(got, tc.wantSub) { + t.Errorf("hint missing %q, got:\n%s", tc.wantSub, got) + } + }) + } +} + +// Mount state transitions must reach the bus, since that is the only way a +// client can render a mount toggle without inspecting the host mount table. +func TestMountStateEventsPublished(t *testing.T) { + bus := events.NewBus(log.NewTest(t)) + sub := bus.Subscribe(0, events.TypeSftpMounted, events.TypeSftpUnmounted) + defer sub.Close() + + p := newTestPlugin(t, t.TempDir()) + p.bus = bus + + // The idempotent path reuses the mount point without a state change, so it + // must not announce a transition that did not happen. + p.mountPoints["dev1"] = "/mnt/kcd-sftp-dev1" + if _, err := p.mountWithBody(context.Background(), "dev1", SftpBody{}, "", false); err != nil { + t.Fatalf("idempotent mount: %v", err) + } + select { + case ev := <-sub.C: + t.Fatalf("unexpected event on a no-op mount: %+v", ev) + case <-time.After(50 * time.Millisecond): + } + + p.publishMounted("dev1", "/mnt/kcd-sftp-dev1", "/storage/ABCD-1234") + ev := <-sub.C + if ev.Type != events.TypeSftpMounted { + t.Fatalf("got %s, want %s", ev.Type, events.TypeSftpMounted) + } + if got := ev.Payload.(map[string]any)["mountPoint"]; got != "/mnt/kcd-sftp-dev1" { + t.Errorf("mountPoint = %v, want /mnt/kcd-sftp-dev1", got) + } + if got := ev.Payload.(map[string]any)["volume"]; got != "/storage/ABCD-1234" { + t.Errorf("volume = %v, want /storage/ABCD-1234", got) + } + + p.publishUnmounted("dev1", "/mnt/kcd-sftp-dev1") + ev = <-sub.C + if ev.Type != events.TypeSftpUnmounted { + t.Fatalf("got %s, want %s", ev.Type, events.TypeSftpUnmounted) + } +} + +func TestInfoReportsMountState(t *testing.T) { + p := newTestPlugin(t, t.TempDir()) + p.lastBody["dev1"] = SftpBody{IP: "192.168.1.42", User: "u0_a123", Path: "/storage/emulated/0"} + + if info := p.Info("dev1", false); info.Mounted { + t.Error("reported mounted with no mount tracked") + } + + p.mountPoints["dev1"] = "/mnt/kcd-sftp-dev1" + info := p.Info("dev1", false) + if !info.Mounted { + t.Error("reported not mounted while a mount is tracked") + } + if info.MountPoint != "/mnt/kcd-sftp-dev1" { + t.Errorf("MountPoint = %q, want /mnt/kcd-sftp-dev1", info.MountPoint) + } +} + +// The reported bug: the daemon still had a device cached as mounted, but the +// kernel had no such mount, so fusermount failed with "not found in +// /etc/mtab" forever. Keeping the cached state on failure -- correct for a +// mount that really is still there -- meant this state could never clear, +// leaving the device unable to mount or unmount again. +func TestUnmount_ClearsStateWhenKernelHasNoMount(t *testing.T) { + dir := t.TempDir() + useFakeMountTable(t, "proc /proc proc rw 0 0\n") + + p := newTestPlugin(t, dir) + mountPoint := filepath.Join(dir, "kcd-sftp-dev1") + if err := os.MkdirAll(mountPoint, 0700); err != nil { + t.Fatal(err) + } + p.mountPoints["dev1"] = mountPoint + + if err := p.Unmount("dev1"); err != nil { + t.Fatalf("unmounting a mount the kernel already dropped must succeed, got: %v", err) + } + if _, cached := p.mountPoints["dev1"]; cached { + t.Error("cached state survived; the device stays wedged as mounted") + } + if _, err := os.Stat(mountPoint); !os.IsNotExist(err) { + t.Error("leftover mount directory was not removed") + } + if p.MountedPath("dev1") != "" { + t.Error("MountedPath still reports the device as mounted") + } +} + +// With an empty cache and nothing in the kernel, a leftover directory from an +// earlier release should still be cleaned up. +func TestUnmount_RemovesLeftoverDirectoryWithoutCache(t *testing.T) { + dir := t.TempDir() + useFakeMountTable(t, "proc /proc proc rw 0 0\n") + + p := newTestPlugin(t, dir) + leftover := filepath.Join(dir, "kcd-sftp-dev1") + if err := os.MkdirAll(leftover, 0700); err != nil { + t.Fatal(err) + } + + err := p.Unmount("dev1") + if err == nil || !strings.Contains(err.Error(), "not mounted") { + t.Errorf("want a 'not mounted' error, got: %v", err) + } + if _, statErr := os.Stat(leftover); !os.IsNotExist(statErr) { + t.Error("leftover directory survived; it will block the next mount's MkdirAll forever") + } +} + +// A genuinely live mount must still be released through fusermount, and the +// normal path must not be short-circuited by the new kernel check. +func TestUnmount_LiveMountTakesTheReleasePath(t *testing.T) { + dir := t.TempDir() + mountPoint := filepath.Join(dir, "kcd-sftp-dev1") + if err := os.MkdirAll(mountPoint, 0700); err != nil { + t.Fatal(err) + } + + // Present in the kernel's view, so the release path runs. Nothing is + // actually mounted here, so fusermount will fail -- the point is that we + // got as far as trying it and then kept the state, because the mount is + // (as far as we can tell) still there. + useFakeMountTable(t, "phone:/storage/emulated/0 "+mountPoint+" fuse.sshfs rw 0 0\n") + + p := newTestPlugin(t, dir) + p.mountPoints["dev1"] = mountPoint + + err := p.Unmount("dev1") + if err != nil && !strings.Contains(err.Error(), "could not be released") { + t.Errorf("unexpected error shape: %v", err) + } + if _, cached := p.mountPoints["dev1"]; !cached { + t.Error("state was dropped for a mount the kernel still reports; it should stay retryable") + } +} + +// Shutdown has to release mounts: OnDisconnect never fires on a graceful stop, +// so without this every restart leaves the phone's storage mounted. +// +// What UnmountAll owns is visiting every tracked device, concurrently, and +// publishing the transition. Releasing a genuinely live mount needs a real +// FUSE mount, which a unit test cannot create -- so the table here reports the +// mounts as already gone, and each Unmount clears its own state on that path. +// The live-release path is covered by TestUnmount_LiveMountTakesTheReleasePath. +func TestUnmountAll_VisitsEveryTrackedDevice(t *testing.T) { + useFakeMountTable(t, "proc /proc proc rw 0 0\n") + + bus := events.NewBus(log.NewTest(t)) + sub := bus.Subscribe(0, events.TypeSftpUnmounted) + defer sub.Close() + + p := newTestPlugin(t, t.TempDir()) + p.bus = bus + for _, id := range []string{"dev1", "dev2", "dev3"} { + p.mountPoints[id] = p.mountPointFor(id) + } + + p.UnmountAll(context.Background()) + + if left := p.mountPoints; len(left) != 0 { + t.Errorf("still tracking %d mount(s) after UnmountAll: %v", len(left), left) + } + for i := range 3 { + select { + case <-sub.C: + case <-time.After(time.Second): + t.Fatalf("only %d of 3 sftp.unmounted events arrived", i) + } + } +} + +func TestUnmountAll_NoMountsIsANoOp(t *testing.T) { + p := newTestPlugin(t, t.TempDir()) + p.UnmountAll(context.Background()) // must not block or panic +} + +// The budget is what protects shutdown from a wedged FUSE mount, so an expired +// context has to end the wait rather than hang it. +func TestUnmountAll_RespectsContextBudget(t *testing.T) { + p := newTestPlugin(t, t.TempDir()) + p.mountPoints["dev1"] = p.mountPointFor("dev1") + // The kernel does not know this mount, so Unmount takes the + // already-released path and returns; the point is that a cancelled context + // still terminates the call. + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + done := make(chan struct{}) + go func() { + p.UnmountAll(ctx) + close(done) + }() + select { + case <-done: + case <-time.After(2 * time.Second): + t.Fatal("UnmountAll ignored a cancelled context") + } + + // UnmountAll returns as soon as ctx is done, so the unmount it spawned is + // still in flight. Wait for it before the test ends: it logs as it goes, + // and t.Log after the test completes is what the race detector catches. + deadline := time.Now().Add(5 * time.Second) + for { + p.mu.RLock() + _, tracked := p.mountPoints["dev1"] + p.mu.RUnlock() + if !tracked { + return + } + if time.Now().After(deadline) { + t.Fatal("the in-flight unmount never finished after UnmountAll returned") + } + time.Sleep(10 * time.Millisecond) + } +} diff --git a/internal/plugins/sftp/mountdirs.go b/internal/plugins/sftp/mountdirs.go new file mode 100644 index 0000000..ad5f54c --- /dev/null +++ b/internal/plugins/sftp/mountdirs.go @@ -0,0 +1,149 @@ +package sftp + +import ( + "bufio" + "os" + "path/filepath" + "strings" + + "github.com/bethropolis/kcd/internal/log" +) + +// userDirsFile is the freedesktop file naming the user's document folders. +// Reading it matters because those folders are relocatable: on a localized +// desktop Downloads may be "Descargas", and a check hardcoded to $HOME would +// miss exactly the setups where a user has customised them. +var userDirsFile = func() string { + configHome := os.Getenv("XDG_CONFIG_HOME") + if configHome == "" { + home, err := os.UserHomeDir() + if err != nil { + return "" + } + configHome = filepath.Join(home, ".config") + } + return filepath.Join(configHome, "user-dirs.dirs") +}() + +// bulkDeletableDirs are the XDG user folders whose contents a user is likely +// to wipe in one go, with the conventional name to fall back on when +// user-dirs.dirs has no entry. +// +// The category, not a single folder: the hazard is never "~/Documents", it is +// "anywhere a user runs rm -rf to reclaim space". Music and Pictures get wiped +// for exactly the same reason Downloads does -- someone clearing old photos is +// as plausible as someone clearing downloads -- so a narrower list would be +// arbitrary. This is the freedesktop set of user-content folders. +var bulkDeletableDirs = []struct{ key, name string }{ + {"XDG_DOWNLOAD_DIR", "Downloads"}, + {"XDG_DOCUMENTS_DIR", "Documents"}, + {"XDG_DESKTOP_DIR", "Desktop"}, + {"XDG_MUSIC_DIR", "Music"}, + {"XDG_PICTURES_DIR", "Pictures"}, + {"XDG_VIDEOS_DIR", "Videos"}, +} + +// userBulkDeletableDirs returns the resolved paths of the folders above. +func userBulkDeletableDirs() []string { + home, err := os.UserHomeDir() + if err != nil { + return nil + } + + configured := map[string]string{} + if userDirsFile != "" { + if f, openErr := os.Open(userDirsFile); openErr == nil { + defer f.Close() + scanner := bufio.NewScanner(f) + for scanner.Scan() { + if key, value, ok := parseUserDirsLine(scanner.Text()); ok { + configured[key] = value + } + } + } + } + + out := make([]string, 0, len(bulkDeletableDirs)) + for _, d := range bulkDeletableDirs { + dir, ok := configured[d.key] + if !ok || dir == "" { + dir = filepath.Join(home, d.name) + } + out = append(out, filepath.Clean(dir)) + } + return out +} + +// parseUserDirsLine reads one `KEY=value` line, unquoting the value and +// expanding $HOME. Values are quoted in the file because they may contain +// spaces, and the placeholder is literal there. +func parseUserDirsLine(line string) (key, value string, ok bool) { + line = strings.TrimSpace(line) + if line == "" || strings.HasPrefix(line, "#") { + return "", "", false + } + key, value, found := strings.Cut(line, "=") + if !found { + return "", "", false + } + key = strings.TrimSpace(key) + value = strings.TrimSpace(value) + if len(value) >= 2 && (value[0] == '"' && value[len(value)-1] == '"' || value[0] == '\'' && value[len(value)-1] == '\'') { + value = value[1 : len(value)-1] + } + if value == "" { + return "", "", false + } + if home, err := os.UserHomeDir(); err == nil { + value = strings.ReplaceAll(value, "$HOME", home) + } + return key, value, true +} + +// containingBulkDeletableDir returns the bulk-deletable folder path sits +// inside, or "" if it is not inside one. +func containingBulkDeletableDir(path string) string { + abs, err := filepath.Abs(path) + if err != nil { + abs = filepath.Clean(path) + } + for _, dir := range userBulkDeletableDirs() { + if isWithin(dir, abs) { + return dir + } + } + return "" +} + +// isWithin reports whether path is dir itself or below it. +func isWithin(dir, path string) bool { + rel, err := filepath.Rel(dir, path) + if err != nil { + return false + } + // A leading ".." means path is outside dir; "." means it is dir itself. + return rel == "." || !strings.HasPrefix(rel, "..") +} + +// warnIfInBulkDeletableDir emits the mount-location warning at most once per +// mount directory per daemon run, so a user who deliberately pinned a location +// is told once rather than on every mount. +func (p *SftpPlugin) warnIfInBulkDeletableDir(mountPoint string) { + owner := containingBulkDeletableDir(mountPoint) + + p.mu.Lock() + if p.warnedDirs == nil { + p.warnedDirs = make(map[string]bool) + } + already := p.warnedDirs[mountPoint] + p.warnedDirs[mountPoint] = true + p.mu.Unlock() + + if already || owner == "" { + return + } + p.logger.Warn("mount directory is inside a user folder that is often wiped in bulk. A mount makes the phone's storage reachable through the filesystem, and `rm -rf` descends into it -- rm only stops at a filesystem boundary when given -x/--one-file-system. Set mount_dir in kcd.toml to somewhere else, e.g. $XDG_RUNTIME_DIR/kcd/mnt.", + log.String("mount_point", mountPoint), + log.String("user_folder", owner), + ) +} diff --git a/internal/plugins/sftp/mountdirs_test.go b/internal/plugins/sftp/mountdirs_test.go new file mode 100644 index 0000000..3c5bf9a --- /dev/null +++ b/internal/plugins/sftp/mountdirs_test.go @@ -0,0 +1,181 @@ +package sftp + +import ( + "context" + "os" + "path/filepath" + "testing" +) + +func TestParseUserDirsLine(t *testing.T) { + home, err := os.UserHomeDir() + if err != nil { + t.Skip("no home directory") + } + + tests := []struct { + name string + line string + wantKey string + wantValue string + wantOK bool + }{ + {name: "comment ignored", line: "# generated", wantOK: false}, + {name: "blank ignored", line: " ", wantOK: false}, + {name: "no equals sign ignored", line: "garbage", wantOK: false}, + {name: "empty value ignored", line: `XDG_DESKTOP_DIR=""`, wantOK: false}, + { + name: "double quoted with $HOME", + line: `XDG_DOWNLOAD_DIR="$HOME/Descargas"`, + wantKey: "XDG_DOWNLOAD_DIR", wantValue: filepath.Join(home, "Descargas"), wantOK: true, + }, + { + name: "single quoted with $HOME", + line: `XDG_DOCUMENTS_DIR='$HOME/Dokumente'`, + wantKey: "XDG_DOCUMENTS_DIR", wantValue: filepath.Join(home, "Dokumente"), wantOK: true, + }, + { + name: "unquoted value", + line: `XDG_DESKTOP_DIR=/srv/desktop`, + wantKey: "XDG_DESKTOP_DIR", wantValue: "/srv/desktop", wantOK: true, + }, + { + name: "spaces around the equals sign", + line: `XDG_DOWNLOAD_DIR = "$HOME/Downloads"`, + wantKey: "XDG_DOWNLOAD_DIR", wantValue: filepath.Join(home, "Downloads"), wantOK: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + key, value, ok := parseUserDirsLine(tc.line) + if ok != tc.wantOK { + t.Fatalf("ok = %v, want %v", ok, tc.wantOK) + } + if !tc.wantOK { + return + } + if key != tc.wantKey || value != tc.wantValue { + t.Errorf("got (%q, %q), want (%q, %q)", key, value, tc.wantKey, tc.wantValue) + } + }) + } +} + +// A relocated Downloads must still be recognised, or the check misses exactly +// the setups where a user has customised their folders. +func TestUserBulkDeletableDirs_HonorsUserDirsFile(t *testing.T) { + dir := t.TempDir() + file := filepath.Join(dir, "user-dirs.dirs") + body := `XDG_DOWNLOAD_DIR="$HOME/Descargas"` + "\n" + if err := os.WriteFile(file, []byte(body), 0o600); err != nil { + t.Fatal(err) + } + orig := userDirsFile + userDirsFile = file + t.Cleanup(func() { userDirsFile = orig }) + + home, err := os.UserHomeDir() + if err != nil { + t.Skip("no home directory") + } + want := filepath.Join(home, "Descargas") + + if got := containingBulkDeletableDir(filepath.Join(want, "kcd", "mnt", "kcd-sftp-dev1")); got != want { + t.Errorf("containingDocumentDir = %q, want %q", got, want) + } + // The conventional name must no longer match once it has been relocated. + if got := containingBulkDeletableDir(filepath.Join(home, "Downloads", "kcd")); got != "" { + t.Errorf("still matched the relocated-away Downloads: %q", got) + } +} + +func TestContainingBulkDeletableDir(t *testing.T) { + orig := userDirsFile + userDirsFile = filepath.Join(t.TempDir(), "does-not-exist") + t.Cleanup(func() { userDirsFile = orig }) + + home, err := os.UserHomeDir() + if err != nil { + t.Skip("no home directory") + } + + tests := []struct { + name string + path string + want string + }{ + {"inside Downloads", filepath.Join(home, "Downloads", "kcd", "mnt"), filepath.Join(home, "Downloads")}, + {"Downloads itself", filepath.Join(home, "Downloads"), filepath.Join(home, "Downloads")}, + {"inside Documents", filepath.Join(home, "Documents", "x"), filepath.Join(home, "Documents")}, + {"inside Desktop", filepath.Join(home, "Desktop", "x"), filepath.Join(home, "Desktop")}, + // Clearing old photos is at least as likely as clearing downloads, so + // narrowing this to three folders would be arbitrary. + {"inside Pictures", filepath.Join(home, "Pictures", "x"), filepath.Join(home, "Pictures")}, + {"inside Music", filepath.Join(home, "Music", "x"), filepath.Join(home, "Music")}, + {"inside Videos", filepath.Join(home, "Videos", "x"), filepath.Join(home, "Videos")}, + {"home itself is not a bulk-deletable dir", home, ""}, + {"runtime dir is not a bulk-deletable dir", "/run/user/1000/kcd/mnt", ""}, + {"sibling with a shared prefix", filepath.Join(home, "Downloads-old", "kcd"), ""}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := containingBulkDeletableDir(tc.path); got != tc.want { + t.Errorf("containingBulkDeletableDir(%q) = %q, want %q", tc.path, got, tc.want) + } + }) + } +} + +// The default location must never trip the warning; that is the point of it. +func TestDefaultMountDir_DoesNotWarn(t *testing.T) { + p := newTestPlugin(t, t.TempDir()) + if owner := containingBulkDeletableDir(p.mountPointFor("dev1")); owner != "" { + t.Errorf("default mount point flagged as inside %q", owner) + } + p.warnIfInBulkDeletableDir(p.mountPointFor("dev1")) // must not panic +} + +// The warning fires for a user who pinned a hazardous location, and only once. +func TestWarnIfInBulkDeletableDir_FiresOncePerDir(t *testing.T) { + orig := userDirsFile + userDirsFile = filepath.Join(t.TempDir(), "does-not-exist") + t.Cleanup(func() { userDirsFile = orig }) + + home, err := os.UserHomeDir() + if err != nil { + t.Skip("no home directory") + } + p := newTestPlugin(t, t.TempDir()) + hazardous := filepath.Join(home, "Downloads", "kcd", "mnt", "kcd-sftp-dev1") + + p.warnIfInBulkDeletableDir(hazardous) + if !p.warnedDirs[hazardous] { + t.Fatal("expected the hazardous directory to be recorded as warned") + } + p.warnIfInBulkDeletableDir(hazardous) // second time: no new warning +} + +// The warning must reach a user who reuses an existing mount, not only one +// creating a new one -- otherwise the location is never mentioned to them. +func TestWarnFiresOnReusedMount(t *testing.T) { + orig := userDirsFile + userDirsFile = filepath.Join(t.TempDir(), "does-not-exist") + t.Cleanup(func() { userDirsFile = orig }) + + home, err := os.UserHomeDir() + if err != nil { + t.Skip("no home directory") + } + p := newTestPlugin(t, filepath.Join(home, "Pictures", "kcd", "mnt")) + hazardous := p.mountPointFor("dev1") + p.mountPoints["dev1"] = hazardous + + // Already mounted, so mountWithBody takes the reuse path. + if _, err := p.mountWithBody(context.Background(), "dev1", SftpBody{}, "", false); err != nil { + t.Fatalf("reuse path: %v", err) + } + if !p.warnedDirs[hazardous] { + t.Error("hazardous location not warned about on the reuse path") + } +} diff --git a/internal/plugins/sftp/mounttable.go b/internal/plugins/sftp/mounttable.go new file mode 100644 index 0000000..7dfa18b --- /dev/null +++ b/internal/plugins/sftp/mounttable.go @@ -0,0 +1,88 @@ +package sftp + +import ( + "bufio" + "os" + "strings" +) + +// mountTablePath is the kernel's view of what is actually mounted. /proc is +// read directly rather than shelling out to findmnt, matching findSSHFSPID, so +// there is no external dependency and no PATH lookup to fail. +// +// It is a var so tests can point it at a fixture. +var mountTablePath = "/proc/mounts" + +// liveMountPoint reports the mount point the kernel still has for a device, or +// ("", false) if there is none. +// +// This exists because mountPoints is in-memory. After a daemon restart the +// FUSE mount is still live, but the daemon has forgotten it: unmount would +// claim the device was never mounted while the mount sat there holding an +// sshfs process, and a later mount would try to run sshfs over it. Treating +// the kernel as the source of truth and the map as a cache makes that +// recoverable without persisting anything -- which is what we want, since a +// persisted entry would go stale after a crash and need reconciling anyway. +// mountExists reports whether the kernel currently has a FUSE mount at this +// exact path. Unmount uses it to tell "nothing is mounted" apart from "the +// release failed", which are very different outcomes for a caller. +func mountExists(path string) bool { + _, found := eachMount(func(mountPoint string) bool { return mountPoint == path }) + return found +} + +// eachMount returns the first FUSE mount for which match reports true, and +// whether any matched. +// +// FUSE-only on purpose: the daemon only ever creates `kcd-sftp-` +// mounts via sshfs, so adopting an unrelated mount that happens to share the +// name — and later running fusermount against it — would be worse than not +// adopting it. +func eachMount(match func(mountPoint string) bool) (string, bool) { + f, err := os.Open(mountTablePath) + if err != nil { + return "", false + } + defer f.Close() + + scanner := bufio.NewScanner(f) + for scanner.Scan() { + fields := strings.Fields(scanner.Text()) + if len(fields) < 3 || !strings.HasPrefix(fields[2], "fuse") { + continue + } + mountPoint := unescapeMountPath(fields[1]) + if match(mountPoint) { + return mountPoint, true + } + } + return "", false +} + +// unescapeMountPath reverses the octal escaping the kernel applies to a +// mountpoint containing whitespace ("/mnt/my\040disk" -> "/mnt/my disk"). +// Getwd-style escapes are limited to space, tab, newline and backslash. +func unescapeMountPath(s string) string { + if !strings.Contains(s, `\`) { + return s + } + var b strings.Builder + b.Grow(len(s)) + for i := 0; i < len(s); i++ { + if s[i] != '\\' || i+3 >= len(s) { + b.WriteByte(s[i]) + continue + } + var v byte + switch { + case s[i+1] >= '0' && s[i+1] <= '7' && s[i+2] >= '0' && s[i+2] <= '7' && s[i+3] >= '0' && s[i+3] <= '7': + v = (s[i+1]-'0')<<6 | (s[i+2]-'0')<<3 | (s[i+3] - '0') + default: + b.WriteByte(s[i]) + continue + } + b.WriteByte(v) + i += 3 + } + return b.String() +} diff --git a/internal/plugins/sftp/mounttable_test.go b/internal/plugins/sftp/mounttable_test.go new file mode 100644 index 0000000..d0722d7 --- /dev/null +++ b/internal/plugins/sftp/mounttable_test.go @@ -0,0 +1,165 @@ +package sftp + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/bethropolis/kcd/internal/config" +) + +func TestUnescapeMountPath(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {name: "plain path is untouched", in: "/mnt/kcd-sftp-dev1", want: "/mnt/kcd-sftp-dev1"}, + {name: "space", in: `/mnt/my\040disk`, want: "/mnt/my disk"}, + {name: "tab", in: `/mnt/a\011b`, want: "/mnt/a\tb"}, + {name: "newline", in: `/mnt/a\012b`, want: "/mnt/a\nb"}, + {name: "backslash", in: `/mnt/a\134b`, want: `/mnt/a\b`}, + {name: "several escapes", in: `/mnt/a\040b\040c`, want: "/mnt/a b c"}, + {name: "trailing lone backslash", in: `/mnt/odd\`, want: `/mnt/odd\`}, + {name: "non-octal escape is left alone", in: `/mnt/a\999b`, want: `/mnt/a\999b`}, + {name: "truncated escape is left alone", in: `/mnt/a\04`, want: `/mnt/a\04`}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := unescapeMountPath(tc.in); got != tc.want { + t.Errorf("unescapeMountPath(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +// useFakeMountTable points the reconciliation at a fixture for one test. +func useFakeMountTable(t *testing.T, contents string) { + t.Helper() + path := filepath.Join(t.TempDir(), "mounts") + if err := os.WriteFile(path, []byte(contents), 0o600); err != nil { + t.Fatal(err) + } + orig := mountTablePath + mountTablePath = path + t.Cleanup(func() { mountTablePath = orig }) +} + +// A mount the daemon has forgotten but the kernel still has is the +// post-restart state: the FUSE mount survived, the daemon's memory did not. +// It has to be adopted, or unmount reports "not mounted" while the mount +// keeps holding an sshfs process. +func TestMountedPath_AdoptsOrphanFromKernel(t *testing.T) { + p := newTestPlugin(t, t.TempDir()) + mountPoint := p.mountPointFor("dev1") + useFakeMountTable(t, "proc /proc proc rw 0 0\nphone:/storage/emulated/0 "+mountPoint+" fuse.sshfs rw 0 0\n") + + if _, cached := p.mountPoints["dev1"]; cached { + t.Fatal("test precondition: cache should start empty") + } + if got := p.MountedPath("dev1"); got != mountPoint { + t.Fatalf("MountedPath = %q, want %q", got, mountPoint) + } + if _, adopted := p.mountPoints["dev1"]; !adopted { + t.Error("orphan was resolved but not adopted, so every lookup re-reads /proc") + } + if !p.IsMounted("dev1") { + t.Error("IsMounted = false for a live mount the daemon had forgotten") + } +} + +// A mount at the location kcd used to default to has to stay findable, or +// upgrading orphans exactly the mounts the user already has. +func TestMountedPath_AdoptsLegacyLocation(t *testing.T) { + p := newTestPlugin(t, t.TempDir()) + legacy := legacyMountPointFor("dev1") + if legacy == "" { + t.Skip("no home directory in this environment") + } + useFakeMountTable(t, "phone:/storage/emulated/0 "+legacy+" fuse.sshfs rw 0 0\n") + + if got := p.MountedPath("dev1"); got != legacy { + t.Fatalf("MountedPath = %q, want the legacy mount at %q", got, legacy) + } + if cached := p.mountPoints["dev1"]; cached != legacy { + t.Errorf("cache = %q, want %q", cached, legacy) + } +} + +// The configured location wins when both exist, so the stale one is left +// visible to the user rather than silently adopted. +func TestMountedPath_PrefersConfiguredOverLegacy(t *testing.T) { + p := newTestPlugin(t, t.TempDir()) + primary := p.mountPointFor("dev1") + legacy := legacyMountPointFor("dev1") + if legacy == "" || legacy == primary { + t.Skip("no distinct legacy path in this environment") + } + useFakeMountTable(t, "a "+legacy+" fuse.sshfs rw 0 0\nb "+primary+" fuse.sshfs rw 0 0\n") + + if got := p.MountedPath("dev1"); got != primary { + t.Errorf("MountedPath = %q, want the configured location %q", got, primary) + } +} + +func TestMountedPath_DoesNotAdoptAnotherDevice(t *testing.T) { + p := newTestPlugin(t, t.TempDir()) + useFakeMountTable(t, "phone:/storage/emulated/0 "+p.mountPointFor("other")+" fuse.sshfs rw 0 0\n") + + if got := p.MountedPath("dev1"); got != "" { + t.Errorf("adopted another device's mount: %q", got) + } +} + +// Non-FUSE mounts sharing the naming scheme must be left alone: adopting one +// and later running fusermount against it would be worse than not adopting. +func TestMountedPath_IgnoresNonFuseEntries(t *testing.T) { + p := newTestPlugin(t, t.TempDir()) + useFakeMountTable(t, "/dev/sdb1 "+p.mountPointFor("dev1")+" ext4 rw 0 0\n") + + if got := p.MountedPath("dev1"); got != "" { + t.Errorf("adopted a non-FUSE mount: %q", got) + } +} + +// Mount points containing spaces arrive octal-escaped from the kernel. +func TestMountedPath_UnescapesKernelPath(t *testing.T) { + // t.TempDir() has no space in it, so build the mount dir explicitly. + p := newTestPlugin(t, filepath.Join(t.TempDir(), "my mount dir")) + want := p.mountPointFor("dev1") + if !strings.Contains(want, " ") { + t.Fatal("test setup: mount point should contain a space") + } + escaped := strings.ReplaceAll(want, " ", `\040`) + useFakeMountTable(t, "phone:/storage/ABCD "+escaped+" fuse.sshfs rw 0 0\n") + + if got := p.MountedPath("dev1"); got != want { + t.Errorf("MountedPath = %q, want the unescaped %q", got, want) + } +} + +// An absent mount reports not mounted and stays out of the cache. +func TestMountedPath_AbsentLeavesCacheClean(t *testing.T) { + useFakeMountTable(t, "proc /proc proc rw 0 0\n") + + p := newTestPlugin(t, t.TempDir()) + if got := p.MountedPath("dev1"); got != "" { + t.Errorf("MountedPath = %q, want empty", got) + } + if _, cached := p.mountPoints["dev1"]; cached { + t.Error("cached an empty resolution") + } +} + +// The default must not be reachable by a routine `rm -rf` of a user +// directory. This is the safety property the whole move is about. +func TestDefaultMountDir_IsNotUnderUserDocuments(t *testing.T) { + dir := config.DefaultMountDir() + for _, doc := range []string{"Downloads", "Documents", "Desktop"} { + if strings.Contains(dir, string(filepath.Separator)+doc+string(filepath.Separator)) || + strings.HasSuffix(dir, string(filepath.Separator)+doc) { + t.Errorf("default mount dir %q sits inside a user document directory", dir) + } + } +} diff --git a/internal/plugins/sftp/request.go b/internal/plugins/sftp/request.go index b3fd556..f7904bf 100644 --- a/internal/plugins/sftp/request.go +++ b/internal/plugins/sftp/request.go @@ -26,7 +26,7 @@ func (p *SftpPlugin) RequestMount(dev device.Sender) error { // RequestAndMount sends the SFTP request, waits for the Android device to // respond with credentials (up to 20 s), mounts the filesystem via sshfs, // and returns the local path the user should open. -func (p *SftpPlugin) RequestAndMount(ctx context.Context, dev device.Sender) (string, error) { +func (p *SftpPlugin) RequestAndMount(ctx context.Context, dev device.Sender, readOnly bool) (string, error) { if p.bus == nil { return "", fmt.Errorf("event bus not available") } @@ -41,10 +41,7 @@ func (p *SftpPlugin) RequestAndMount(ctx context.Context, dev device.Sender) (st p.logger.Info("SFTP request sent, waiting for phone response", log.String("device", dev.ID())) - timeout := time.Duration(p.cfg.CredentialsTimeoutSecs) * time.Second - if timeout == 0 { - timeout = 20 * time.Second - } + timeout := p.credentialsTimeout() deadline, cancel := context.WithTimeout(ctx, timeout) defer cancel() @@ -63,10 +60,10 @@ func (p *SftpPlugin) RequestAndMount(ctx context.Context, dev device.Sender) (st if !exists { return "", fmt.Errorf("credentials missing after event (internal error)") } - return p.mountWithBody(ctx, dev.ID(), body, "") + return p.mountWithBody(ctx, dev.ID(), body, "", readOnly) case <-deadline.Done(): - return "", fmt.Errorf("timed out after %s waiting for SFTP response — is the KDE Connect app open on the phone?", timeout) + return "", fmt.Errorf("timed out after %s waiting for SFTP response — the phone did not start its SFTP server. Either the KDE Connect app is not open, or it has not granted kcd file access (on Android, enable file access for KDE Connect)", timeout) } } } @@ -75,7 +72,7 @@ func (p *SftpPlugin) RequestAndMount(ctx context.Context, dev device.Sender) (st // mounts the specified volume. If volumePath is empty, the available volumes // are returned without mounting (list mode). The caller is responsible for // closing the returned closer when done with the mounted path. -func (p *SftpPlugin) RequestAndMountVolume(ctx context.Context, dev device.Sender, volumePath string) (mountPath string, volumes []StorageVolume, err error) { +func (p *SftpPlugin) RequestAndMountVolume(ctx context.Context, dev device.Sender, volumePath string, readOnly bool) (mountPath string, volumes []StorageVolume, err error) { if p.bus == nil { return "", nil, fmt.Errorf("event bus not available") } @@ -89,10 +86,7 @@ func (p *SftpPlugin) RequestAndMountVolume(ctx context.Context, dev device.Sende p.logger.Info("SFTP request sent, waiting for phone response", log.String("device", dev.ID())) - timeout := time.Duration(p.cfg.CredentialsTimeoutSecs) * time.Second - if timeout == 0 { - timeout = 20 * time.Second - } + timeout := p.credentialsTimeout() deadline, cancel := context.WithTimeout(ctx, timeout) defer cancel() @@ -118,52 +112,66 @@ func (p *SftpPlugin) RequestAndMountVolume(ctx context.Context, dev device.Sende return "", vols, nil } - path, err := p.mountWithBody(ctx, dev.ID(), body, volumePath) + path, err := p.mountWithBody(ctx, dev.ID(), body, volumePath, readOnly) if err != nil { return "", nil, err } return path, vols, nil case <-deadline.Done(): - return "", nil, fmt.Errorf("timed out after %s waiting for SFTP response — is the KDE Connect app open on the phone?", timeout) + return "", nil, fmt.Errorf("timed out after %s waiting for SFTP response — the phone did not start its SFTP server. Either the KDE Connect app is not open, or it has not granted kcd file access (on Android, enable file access for KDE Connect)", timeout) } } } // MountLocally mounts using previously cached credentials. // Prefer RequestAndMount for a one-step experience. -func (p *SftpPlugin) MountLocally(ctx context.Context, deviceID string) (string, error) { +func (p *SftpPlugin) MountLocally(ctx context.Context, deviceID string, readOnly bool) (string, error) { p.mu.RLock() body, ok := p.lastBody[deviceID] p.mu.RUnlock() if !ok { return "", fmt.Errorf("no SFTP credentials cached for device %s — use 'kcd sftp mount' which requests them automatically", deviceID) } - return p.mountWithBody(ctx, deviceID, body, "") + return p.mountWithBody(ctx, deviceID, body, "", readOnly) } // Info returns the cached SFTP connection details for a device. // Returns nil if no credentials have been received yet. -func (p *SftpPlugin) Info(deviceID string) *SftpInfo { +// +// includePassword gates the credential: it is a working password for the +// phone's SFTP server, and `kcd sftp info` is an informational command whose +// output routinely lands in scrollback, logs and $(...) captures. Callers that +// genuinely need it (mounting) get credentials from the sftp.mount event +// instead, which is unaffected by this gate. +// +// Returns nil if no credentials have been received yet, even for a device that +// is still mounted -- mount state outlives the credential cache, which is +// cleared on disconnect. +func (p *SftpPlugin) Info(deviceID string, includePassword bool) *SftpInfo { p.mu.RLock() - defer p.mu.RUnlock() body, ok := p.lastBody[deviceID] + var volumes []StorageVolume + if ok { + volumes = p.buildVolumes(body) + } + p.mu.RUnlock() + if !ok { return nil } + mountPoint := p.MountedPath(deviceID) info := &SftpInfo{ - IP: body.IP, - Port: body.Port, - User: body.User, - Password: body.Password, - Path: body.Path, + IP: body.IP, + Port: body.Port, + User: body.User, + Path: body.Path, + Volumes: volumes, + Mounted: mountPoint != "", + MountPoint: mountPoint, } - for i, mp := range body.MultiPaths { - name := mp - if i < len(body.PathNames) { - name = body.PathNames[i] - } - info.Volumes = append(info.Volumes, StorageVolume{Name: name, Path: mp}) + if includePassword { + info.Password = body.Password } return info } @@ -198,8 +206,68 @@ func (p *SftpPlugin) Volumes(deviceID string) []StorageVolume { } // MountedPath returns the local mount point for a device, or "" if not mounted. +// +// Reconciles against the kernel mount table on a cache miss, so a mount made +// before a daemon restart is still reported as mounted. func (p *SftpPlugin) MountedPath(deviceID string) string { p.mu.RLock() - defer p.mu.RUnlock() - return p.mountPoints[deviceID] + mountPoint, cached := p.mountPoints[deviceID] + p.mu.RUnlock() + if cached { + return mountPoint + } + + // The kernel is the source of truth. The configured location first, then + // the location mounts used to default to, so a mount made before the + // default moved is still found rather than orphaned. + if mountPoint = p.adoptIfMounted(deviceID, p.mountPointFor(deviceID)); mountPoint != "" { + return mountPoint + } + if legacy := legacyMountPointFor(deviceID); legacy != "" && legacy != p.mountPointFor(deviceID) { + if mountPoint = p.adoptIfMounted(deviceID, legacy); mountPoint != "" { + p.logger.Info("adopted SFTP mount left at the previous default location", + log.String("device_id", deviceID), + log.String("mount_point", mountPoint), + ) + return mountPoint + } + } + return "" +} + +// adoptIfMounted caches mountPoint as this device's mount when the kernel +// reports it, and returns it; otherwise returns "". +func (p *SftpPlugin) adoptIfMounted(deviceID, mountPoint string) string { + if !mountExists(mountPoint) { + return "" + } + p.mu.Lock() + p.mountPoints[deviceID] = mountPoint + p.mu.Unlock() + p.logger.Debug("adopted SFTP mount left by a previous daemon session", + log.String("device_id", deviceID), + log.String("mount_point", mountPoint), + ) + return mountPoint +} + +// IsMounted reports whether a device's filesystem is currently mounted. +func (p *SftpPlugin) IsMounted(deviceID string) bool { + return p.MountedPath(deviceID) != "" +} + +// credentialsTimeout is how long to wait for the phone's SFTP response. The +// CLI derives its own socket deadline from the same configured value, so the +// daemon reports the timeout rather than the client reporting a dead socket. +func (p *SftpPlugin) credentialsTimeout() time.Duration { + if p.cfg.CredentialsTimeoutSecs <= 0 { + return 20 * time.Second + } + return time.Duration(p.cfg.CredentialsTimeoutSecs) * time.Second +} + +// ReadOnlyByDefault reports the configured read-only default, used when a +// mount request does not override it. +func (p *SftpPlugin) ReadOnlyByDefault() bool { + return p.cfg.ReadOnly } diff --git a/internal/plugins/sftp/sftp_test.go b/internal/plugins/sftp/sftp_test.go index c581ef8..29b690f 100644 --- a/internal/plugins/sftp/sftp_test.go +++ b/internal/plugins/sftp/sftp_test.go @@ -103,7 +103,7 @@ func validBody() SftpBody { } func TestBuildSSHFSArgs_Valid(t *testing.T) { - args, err := buildSSHFSArgs(validBody(), "/storage/emulated/0", "/mnt/kcd", 1000, 1000, 15, 3, nil) + args, err := buildSSHFSArgs(validBody(), "/storage/emulated/0", "/mnt/kcd", 1000, 1000, 15, 3, nil, false) if err != nil { t.Fatalf("buildSSHFSArgs returned error: %v", err) } @@ -145,7 +145,7 @@ func TestBuildSSHFSArgs_RejectsInjection(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - if _, err := buildSSHFSArgs(tc.body, tc.path, "/mnt/kcd", 1000, 1000, 15, 3, nil); err == nil { + if _, err := buildSSHFSArgs(tc.body, tc.path, "/mnt/kcd", 1000, 1000, 15, 3, nil, false); err == nil { t.Errorf("buildSSHFSArgs accepted %q / %q, want error", tc.body, tc.path) } }) @@ -156,8 +156,80 @@ func TestBuildSSHFSArgs_Hostnames(t *testing.T) { for _, host := range []string{"phone.local", "android-1", "192.168.1.42", "::1"} { body := validBody() body.IP = host - if _, err := buildSSHFSArgs(body, "/x", "/mnt/kcd", 1000, 1000, 15, 3, nil); err != nil { + if _, err := buildSSHFSArgs(body, "/x", "/mnt/kcd", 1000, 1000, 15, 3, nil, false); err != nil { t.Errorf("buildSSHFSArgs(%q) = %v, want nil", host, err) } } } + +// -o ro is what makes deletion impossible: the kernel rejects unlink/rmdir +// before it ever reaches the phone, so an accidental `rm` cannot propagate. +func TestBuildSSHFSArgs_ReadOnly(t *testing.T) { + args, err := buildSSHFSArgs(validBody(), "/storage/emulated/0", "/mnt/kcd", 1000, 1000, 15, 3, nil, true) + if err != nil { + t.Fatalf("buildSSHFSArgs: %v", err) + } + if !containsPair(args, "ro") { + t.Errorf("expected -o ro in %v", args) + } + + args, err = buildSSHFSArgs(validBody(), "/storage/emulated/0", "/mnt/kcd", 1000, 1000, 15, 3, nil, false) + if err != nil { + t.Fatalf("buildSSHFSArgs: %v", err) + } + if containsPair(args, "ro") { + t.Errorf("did not ask for read-only but got -o ro in %v", args) + } +} + +// ExtraSshfsOpts is operator config, so it has to be able to override the +// per-request mode -- hence -o ro being emitted before it. +func TestBuildSSHFSArgs_ExtraOptsCanOverrideReadOnly(t *testing.T) { + args, err := buildSSHFSArgs(validBody(), "/storage/emulated/0", "/mnt/kcd", 1000, 1000, 15, 3, []string{"rw"}, true) + if err != nil { + t.Fatalf("buildSSHFSArgs: %v", err) + } + // sshfs takes the last -o ro/rw, so the operator's "rw" must come after ours. + roIdx, rwIdx := indexOfPair(args, "ro"), indexOfPair(args, "rw") + if roIdx < 0 || rwIdx < 0 { + t.Fatalf("expected both ro and rw in %v", args) + } + if roIdx > rwIdx { + t.Errorf("operator override must follow -o ro, got ro at %d and rw at %d in %v", roIdx, rwIdx, args) + } +} + +func TestReadOnlyByDefault(t *testing.T) { + p := newTestPlugin(t, t.TempDir()) + if p.ReadOnlyByDefault() { + t.Error("default should be writable; read-only is opt-in") + } + p.cfg.ReadOnly = true + if !p.ReadOnlyByDefault() { + t.Error("config default not reported") + } +} + +// The mode an existing mount was created with has to be remembered, since a +// later --ro cannot change it and the log must say what the mount actually is. +func TestMountIsReadOnly_TracksCreatedMode(t *testing.T) { + p := newTestPlugin(t, t.TempDir()) + if p.mountIsReadOnly("dev1") { + t.Error("an untracked device must not report read-only") + } + p.mountReadOnly["dev1"] = true + if !p.mountIsReadOnly("dev1") { + t.Error("recorded mode not reported") + } +} + +func containsPair(args []string, opt string) bool { return indexOfPair(args, opt) >= 0 } + +func indexOfPair(args []string, opt string) int { + for i := 0; i+1 < len(args); i++ { + if args[i] == "-o" && args[i+1] == opt { + return i + } + } + return -1 +} diff --git a/internal/plugins/sftp/types.go b/internal/plugins/sftp/types.go index 2625d4a..c95902b 100644 --- a/internal/plugins/sftp/types.go +++ b/internal/plugins/sftp/types.go @@ -2,6 +2,8 @@ package sftp import ( "encoding/json" + "os" + "path/filepath" "sync" "time" @@ -18,17 +20,24 @@ type SftpPlugin struct { mu sync.RWMutex lastBody map[string]SftpBody mountPoints map[string]string // deviceID -> local mountPoint path - mountPIDs map[string]int // deviceID -> sshfs PID for graceful shutdown + // mountReadOnly records the mode each mount was created with, so a later + // --ro request against an existing mount can report what it actually is. + mountReadOnly map[string]bool + // warnedDirs records mount directories the document-folder warning has + // already fired for, so it is logged once per location per run. + warnedDirs map[string]bool + mountPIDs map[string]int // deviceID -> sshfs PID for graceful shutdown } func NewSftpPlugin(cfg config.SFTPConfig, bus *events.Bus, logger log.Logger) *SftpPlugin { return &SftpPlugin{ - cfg: cfg, - bus: bus, - logger: logger.With(log.String("plugin", "sftp")), - lastBody: make(map[string]SftpBody), - mountPoints: make(map[string]string), - mountPIDs: make(map[string]int), + cfg: cfg, + bus: bus, + logger: logger.With(log.String("plugin", "sftp")), + lastBody: make(map[string]SftpBody), + mountPoints: make(map[string]string), + mountReadOnly: make(map[string]bool), + mountPIDs: make(map[string]int), } } @@ -61,16 +70,54 @@ type StorageVolume struct { } // SftpInfo holds the complete cached SFTP connection details for a device. +// +// Password is a live credential for the phone's SFTP server, so it is left +// empty unless the caller explicitly asked for it (see Info). omitempty keeps +// the field out of the JSON entirely rather than emitting an empty string, +// so a masked response cannot be mistaken for a device with no password. type SftpInfo struct { IP string `json:"ip"` Port json.Number `json:"port"` User string `json:"user"` - Password string `json:"password"` + Password string `json:"password,omitempty"` Path string `json:"path"` Volumes []StorageVolume `json:"volumes,omitempty"` + + // Mounted reports whether this device's filesystem is currently mounted, + // and MountPoint is where. Clients use it to render a mount toggle + // without inspecting the host's mount table. + Mounted bool `json:"mounted"` + MountPoint string `json:"mountPoint,omitempty"` } func (p *SftpPlugin) Name() string { return "SFTP" } func (p *SftpPlugin) Timeout() time.Duration { return 5 * time.Second } func (p *SftpPlugin) IncomingTypes() []string { return []string{"kdeconnect.sftp"} } func (p *SftpPlugin) OutgoingTypes() []string { return []string{"kdeconnect.sftp.request"} } + +// mountPointFor is the one mount path the daemon creates for a device. +// Derived rather than looked up, so it still identifies leftovers when the +// cache is empty -- which is exactly the state after a restart. +// +// An empty MountDir falls back to the same default config.Defaults() uses, +// rather than to a temp directory: the daemon never writes the config file, +// so `mount_dir = ""` is an explicit request for the default, not a way to +// ask for somewhere that gets swept up by periodic tmp cleaning. +func (p *SftpPlugin) mountPointFor(deviceID string) string { + baseDir := p.cfg.MountDir + if baseDir == "" { + baseDir = config.DefaultMountDir() + } + return filepath.Join(baseDir, "kcd-sftp-"+deviceID) +} + +// legacyMountPointFor is where mounts lived before the default moved out of +// ~/Downloads. Mounts there outlive the daemon, so Unmount has to keep being +// able to find and release them. +func legacyMountPointFor(deviceID string) string { + home, err := os.UserHomeDir() + if err != nil { + return "" + } + return filepath.Join(home, "Downloads", "kcd", "mnt", "kcd-sftp-"+deviceID) +} diff --git a/mise.toml b/mise.toml index c1acd20..d1a7551 100644 --- a/mise.toml +++ b/mise.toml @@ -1,5 +1,8 @@ [tools] -hk = "latest" +# Pinned to the version hk.pkl amends. With "latest" a mise upgrade could pull +# hk forward of the config's own `package://.../hk@2.4.0` import, and the check +# steps would stop resolving rather than just reporting differently. +hk = "2.4.0" # Pinned to match .github/workflows/ci.yml (golangci-lint-action v9, v2.11). # Bump both together; a local version ahead of CI means local checks can pass # while CI fails. diff --git a/packaging/kcd-user.service b/packaging/kcd-user.service index e32cd97..1122f33 100644 --- a/packaging/kcd-user.service +++ b/packaging/kcd-user.service @@ -21,6 +21,12 @@ NotifyAccess=main # Matches the binary installed by scripts/install.sh. ExecStart=%h/.local/bin/kcd daemon +# `systemctl --user reload kcd` sends SIGHUP. The daemon reloads the [commands] +# table, the notification filters and the log level in place; anything else +# still needs a restart. Without this line the reload verb fails, because +# systemd has no way to signal the unit. +ExecReload=/bin/kill -HUP $MAINPID + # Graceful shutdown: send SIGTERM and give the daemon up to 10 s to finish # in-progress file transfers before SIGKILL. KillMode=mixed diff --git a/packaging/kcd.example.toml b/packaging/kcd.example.toml index 8cdd73b..d58fa98 100644 --- a/packaging/kcd.example.toml +++ b/packaging/kcd.example.toml @@ -1,7 +1,9 @@ # kcd configuration file # Copy this file to ~/.config/kcd/kcd.toml and edit to taste. # All settings are optional — safe defaults are applied automatically. -# Reload requires a daemon restart: systemctl --user restart kcd +# `systemctl --user reload kcd` re-reads this file and applies [commands], +# the [notifications] filters and log_level. Everything else needs +# systemctl --user restart kcd. # ─── Identity ───────────────────────────────────────────────────────────────── @@ -21,11 +23,9 @@ # Default: 1716 # tcp_port = 1716 -# Broadcast is on demand. mDNS advertisement runs always (responder-only, -# negligible cost); UDP broadcast starts automatically when you run -# `kcd pair` (listen mode) and while any paired device is offline, and -# stops otherwise. Paired phones reconnect via the last known address, -# refreshed from sightings and persisted across restarts. +# Nothing to configure here. Broadcast is on demand: UDP runs while a paired +# device is offline, and mDNS advertises always at negligible cost. Paired +# phones reconnect via their last known address. # ─── Pairing ────────────────────────────────────────────────────────────────── @@ -35,6 +35,7 @@ # To initiate pairing to a specific device, run `kcd pair `. # ─── Device Management ───────────────────────────────────────────────────────── + # Automatically remove stale (unpaired, disconnected) devices from the registry. # This prevents old discovery entries from accumulating in `kcd devices`. # Set to "0" to disable auto-pruning entirely. @@ -65,6 +66,7 @@ # log_level = "info" # ─── Plugins ────────────────────────────────────────────────────────────────── + # Each plugin can be individually enabled or disabled. # All plugins are enabled by default. @@ -112,13 +114,7 @@ connectivity = true mousepad = true # Browse the phone's filesystem over SFTP. -# Optional: requires sshfs for the `kcd sftp mount` command. -# Commands: -# kcd sftp request — Ask the phone to start its SFTP server -# kcd sftp info — Show cached connection details and volumes -# kcd sftp volumes — List available storage volumes (multiPaths) -# kcd sftp mount — Request credentials, mount via sshfs, open -# kcd sftp unmount — Unmount a previously mounted filesystem +# Optional: requires sshfs for `kcd sftp mount`. See `kcd sftp --help`. sftp = true # Ring the phone to locate it. @@ -134,19 +130,11 @@ lockdevice = true systemvolume = true # Send and receive SMS messages via a connected phone. -# Commands: -# kcd sms send — Send an SMS -# kcd sms conversations — List all conversations (results via `kcd watch --events sms.incoming`) -# kcd sms conversation — Show a specific conversation -# kcd sms attachment — Request an MMS attachment file -# See the [sms] section below for notification settings. +# See `kcd sms --help`, and the [sms] section below for notifications. sms = true # Sync the phone address book (vCards cached under -# ~/.local/share/kcd/contacts//). -# Commands: -# kcd contacts sync — Request a sync (results via `kcd watch --events contacts.updated`) -# kcd contacts list [--json] — List cached contacts +# ~/.local/share/kcd/contacts//). See `kcd contacts --help`. # The phone gates this on READ_CONTACTS plus per-device opt-in; unpairing # wipes the cached address book. contacts = true @@ -157,13 +145,11 @@ contacts = true presenter = true # Control the remote device's audio volume from this machine. -# Commands: -# kcd volume list — List audio sinks on the phone -# kcd volume set <0-100> — Set volume for a sink -# kcd volume mute — Mute/unmute a sink +# See `kcd volume --help`. remotesystemvolume = true # ─── Connection timing and storage ──────────────────────────────────────────── + # Durations use Go syntax (e.g. "750ms", "10s", "5m") and must be positive. # Restart the daemon after changing these settings. @@ -179,9 +165,11 @@ remotesystemvolume = true # initial_backoff = "2s" # max_backoff = "5m" # must be >= initial_backoff # flap_threshold = "15s" # minimum lifetime for a stable connection -# sighting_driven = true # park redial on discovery sightings; false = legacy pure-timer loop +# sighting_driven = true # dial as soon as a paired phone is seen on + # the network instead of on a timer # fallback_max = "1h" # must be >= max_backoff; silent-case spacing while parked -# stale_after = "24h" # give up (zero timers) past this silence; next sighting respawns +# stale_after = "24h" # stop redialing entirely after this much + # silence; a sighting starts it again [discovery] # broadcast_interval = "30s" @@ -190,15 +178,14 @@ remotesystemvolume = true # they do not enable permanent broadcast or change mDNS advertisement. [mpris] -# The local D-Bus watcher is event-driven; this section tunes the position -# poller that keeps the phone's now-playing display exact while music plays. -# poll_while_playing = true # false = pure event-driven (position extrapolates - # from posAnchorMs; no poller at all, so a dropped - # D-Bus signal can leave the display stale) -# position_interval = "2s" # D-Bus re-read cadence while playing only. - # D-Bus signals arm the poller on playback and it - # stops itself on a confirmed pause, so a paused - # or absent player costs zero wakeups. +# The D-Bus watcher is event-driven; these two tune the position poller that +# keeps the phone's display moving in real time. +# poll_while_playing = true # false = trust the extrapolated position from + # D-Bus signals alone. Cheaper, but a dropped + # signal leaves the phone's position stale. +# position_interval = "2s" # Re-read cadence while playing. Signals start + # and stop the poller, so a paused or absent + # player costs zero wakeups. [cache] # Empty values preserve existing storage paths; use absolute paths to override. @@ -212,6 +199,7 @@ remotesystemvolume = true # push_on_connect = false # ─── RunCommand: exposed commands ───────────────────────────────────────────── + # Shell commands that the RunCommand plugin makes available to your phone. # Trigger them from the KDE Connect Android app → your device → Run command. # Keys are the display labels; values are the shell commands executed locally. @@ -229,6 +217,7 @@ suspend = "systemctl suspend" # screenshot = "grimblast save screen ~/Pictures/screenshot-$(date +%F-%T).png" # ─── Notification filters ───────────────────────────────────────────────────── + # Control which phone app notifications appear on the desktop. # Keys are Android app package names. Values: "show" or "silent". # "silent" still emits a notification event (visible in kcd watch) but skips @@ -242,6 +231,7 @@ suspend = "systemctl suspend" # "*" = "show" # ─── Battery: notifications and thresholds ──────────────────────────────────── + # In addition to monitoring your phone's battery, the daemon also sends this # machine's battery state to connected devices. This happens once on connect # and whenever the phone explicitly requests it — no periodic polling. @@ -280,8 +270,21 @@ suspend = "systemctl suspend" # ─── SFTP: remote filesystem mounting ──────────────────────────────────────── [sftp] -# mount_dir = "" # parent directory for mounts, defaults to temp -# credentials_timeout_secs = 20 # how long to wait for phone credentials +# Parent directory for mounts. Default: $XDG_RUNTIME_DIR/kcd/mnt +# (/run/user/1000/kcd/mnt), or $XDG_STATE_HOME/kcd/mnt with no user session. +# A mount exposes the phone's storage through the filesystem, so keep it out +# of folders you wipe in bulk -- `rm -rf` descends into a mount unless given +# -x/--one-file-system. kcd warns if this lands under Downloads, Documents, +# Desktop, Music, Pictures or Videos. +# mount_dir = "" +# read_only = false # mount read-only, so deletions cannot reach + # the phone; `kcd sftp mount --ro` / --no-ro + # override this per request +# credentials_timeout_secs = 20 # how long to wait for the phone's SFTP + # response. Also sets the kcd CLI's deadline for + # the commands that wait on it, so a phone that + # never answers is reported as such rather than + # as a socket timeout. # keepalive_interval_secs = 15 # sshfs ServerAliveInterval # keepalive_count = 3 # sshfs ServerAliveCountMax # auto_open = true # automatically open mount in file manager @@ -307,15 +310,12 @@ suspend = "systemctl suspend" [sms] # notify_incoming = true # show a desktop notification on incoming SMS -# always_arm = false # ask the phone to push new SMS on every +# always_arm = false # Ask the phone to push new SMS on every # connect, so notifications work with no # client attached. Off by default because an # armed phone cannot be un-armed; without it, # `kcd watch --events sms.incoming` receives # messages instead. - # The phone applies its blocked-numbers list - # only to the deprecated telephony push, so - # blocked senders can still arrive here. # ─── Mousepad: remote input settings ───────────────────────────────────────── diff --git a/pkg/client/client.go b/pkg/client/client.go index d1381d0..e283ef7 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -19,6 +19,12 @@ type Client struct { // PairListenTimeout is the client's deadline for a pair_listen call. // Set it above the daemon's pairing.listen_timeout; zero defaults to 70s. PairListenTimeout time.Duration + // SftpTimeout is the client's deadline for the calls that block waiting for + // the phone's SFTP response (mount_local, browse). Must exceed the daemon's + // sftp.credentials_timeout_secs, or the socket deadline fires first and the + // user is told the connection timed out instead of being told the phone did + // not answer. Zero defaults to 60s. + SftpTimeout time.Duration } // Call dialed the daemon, sends a request, and returns the response. @@ -27,6 +33,12 @@ func (c *Client) Call(cmd string, payload interface{}) (*ipc.Response, error) { if timeout <= 0 { timeout = 5 * time.Second } + return c.callWithTimeout(cmd, payload, timeout) +} + +// callWithTimeout is Call with an explicit deadline, for the few commands whose +// daemon-side wait is longer than the default client deadline. +func (c *Client) callWithTimeout(cmd string, payload interface{}, timeout time.Duration) (*ipc.Response, error) { ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() diff --git a/pkg/client/client_sftp.go b/pkg/client/client_sftp.go index 60f7bd2..4161ebd 100644 --- a/pkg/client/client_sftp.go +++ b/pkg/client/client_sftp.go @@ -1,20 +1,29 @@ package client import ( + "time" + "encoding/json" "github.com/bethropolis/kcd/internal/ipc" ) // SftpMount requests the daemon to initiate an SFTP connection to the remote device. -func (c *Client) SftpMount(deviceID string) error { - _, err := c.Call(ipc.CmdSftpMount, ipc.DevicePayload{DeviceID: deviceID}) +// +// readOnly overrides the daemon's [sftp] read_only default for this request. +// Pass a pointer to true for --ro, a pointer to false to force writable, or +// nil to use the configured default. +func (c *Client) SftpMount(deviceID string, readOnly *bool) error { + _, err := c.Call(ipc.CmdSftpMount, ipc.SftpMountPayload{DeviceID: deviceID, ReadOnly: readOnly}) return err } // SftpInfo returns the cached SFTP connection details for a device. -func (c *Client) SftpInfo(deviceID string) (*ipc.SftpInfoResponse, error) { - resp, err := c.Call(ipc.CmdSftpInfo, ipc.DevicePayload{DeviceID: deviceID}) +// +// The password is a live credential for the phone's SFTP server and is only +// returned when showPassword is true; otherwise the daemon omits it. +func (c *Client) SftpInfo(deviceID string, showPassword bool) (*ipc.SftpInfoResponse, error) { + resp, err := c.Call(ipc.CmdSftpInfo, ipc.SftpInfoPayload{DeviceID: deviceID, ShowPassword: showPassword}) if err != nil { return nil, err } @@ -41,8 +50,8 @@ func (c *Client) SftpVolumes(deviceID string) ([]ipc.StorageVolumeResponse, erro // SftpMountLocal requests the daemon to request SFTP credentials from the // phone, wait for the response, mount via sshfs, and open the result in // the default file manager. Returns the local browse path on success. -func (c *Client) SftpMountLocal(deviceID string) (string, error) { - resp, err := c.Call(ipc.CmdSftpMountLocal, ipc.DevicePayload{DeviceID: deviceID}) +func (c *Client) SftpMountLocal(deviceID string, readOnly *bool) (string, error) { + resp, err := c.callWithTimeout(ipc.CmdSftpMountLocal, ipc.SftpMountPayload{DeviceID: deviceID, ReadOnly: readOnly}, c.sftpTimeout()) if err != nil { return "", err } @@ -65,11 +74,12 @@ func (c *Client) SftpUnmount(deviceID string) error { // available volumes (volume arg empty) or mounts the specified volume. // volume can be an index (0-based), volume name, or path. // Returns the mount path (empty if listing) and available volumes. -func (c *Client) SftpBrowse(deviceID string, volume string) (string, []ipc.StorageVolumeResponse, error) { - resp, err := c.Call(ipc.CmdSftpBrowse, ipc.SftpBrowsePayload{ +func (c *Client) SftpBrowse(deviceID string, volume string, readOnly *bool) (string, []ipc.StorageVolumeResponse, error) { + resp, err := c.callWithTimeout(ipc.CmdSftpBrowse, ipc.SftpBrowsePayload{ DeviceID: deviceID, Volume: volume, - }) + ReadOnly: readOnly, + }, c.sftpTimeout()) if err != nil { return "", nil, err } @@ -79,3 +89,11 @@ func (c *Client) SftpBrowse(deviceID string, volume string) (string, []ipc.Stora } return result.Path, result.Volumes, nil } + +// sftpTimeout is the deadline for calls that wait on the phone. +func (c *Client) sftpTimeout() time.Duration { + if c.SftpTimeout > 0 { + return c.SftpTimeout + } + return 60 * time.Second +} diff --git a/scripts/uninstall.sh b/scripts/uninstall.sh index 5805291..a539681 100755 --- a/scripts/uninstall.sh +++ b/scripts/uninstall.sh @@ -146,18 +146,93 @@ else skip "Nautilus extension not installed" fi +# ── Safe recursive delete ────────────────────────────────────────────────────── +# rm descends into nested filesystems by default, so a live SFTP mount inside +# one of these directories would be traversed and the phone's files deleted. +# --one-file-system makes rm stop at the filesystem boundary instead +# (coreutils >= 8.30 spells it long, BusyBox uses -x). +# +# Probed rather than assumed: on an older coreutils the unrecognized option +# would abort the script under `set -euo pipefail`, leaving a half-uninstalled +# system rather than a merely un-uninstalled one. +if rm --help 2>&1 | grep -q -- '--one-file-system'; then + KCD_RM_ONE_FS=(--one-file-system) +elif rm --help 2>&1 | grep -qE -- '(^|[[:space:]])-x([[:space:],]|$)'; then + KCD_RM_ONE_FS=(-x) +else + KCD_RM_ONE_FS=() +fi + +# _rm_rf removes directories without crossing a filesystem boundary when rm +# supports it. +_rm_rf() { + rm "${KCD_RM_ONE_FS[@]+"${KCD_RM_ONE_FS[@]}"}" -rf "$@" +} + +# _is_mountpoint reports whether a directory is itself a mount point. +# Compared by device id, since a mount sits on a different st_dev than its +# parent. This avoids depending on findmnt or mountpoint, which nothing else in +# these scripts uses. +_is_mountpoint() { + local dir="$1" parent dev_dir dev_parent + [ -d "${dir}" ] || return 1 + parent="$(dirname -- "${dir}")" + [ -d "${parent}" ] || return 1 + dev_dir="$(stat -c %d -- "${dir}" 2>/dev/null)" || return 1 + dev_parent="$(stat -c %d -- "${parent}" 2>/dev/null)" || return 1 + [ "${dev_dir}" != "${dev_parent}" ] +} + +# _live_sftp_mounts prints any SFTP mount point that is still mounted, under +# the current location or the pre-v1.22 one. Unmounting is the daemon's job, but +# a mount can outlive it -- an unclean shutdown, a crash, or a device id the +# daemon no longer knows -- and this is the last chance to notice. +_live_sftp_mounts() { + local base dir found="" + for base in "${STATE_DIR}/mnt" "${HOME}/Downloads/kcd/mnt"; do + [ -d "${base}" ] || continue + for dir in "${base}"/kcd-sftp-*; do + [ -d "${dir}" ] || continue + # The glob also matches the literal parent when nothing is there. + case "$(basename -- "${dir}")" in + kcd-sftp-?*) ;; + *) continue ;; + esac + if _is_mountpoint "${dir}"; then + found="${found}${dir}"$'\n' + fi + done + done + printf '%s' "${found}" +} + # ── Config and state ────────────────────────────────────────────────────────── step "Configuration and state" _remove_data() { - local removed=false + local removed=false live + + # Without a boundary-aware rm there is no safe way to delete a directory that + # still contains a live mount, so refuse rather than delete through it. + if [ "${#KCD_RM_ONE_FS[@]}" -eq 0 ]; then + live="$(_live_sftp_mounts)" + if [ -n "${live}" ]; then + warn "This system's rm has no --one-file-system, so refusing to delete." + warn "Unmount the SFTP mount(s) first, then re-run:" + while IFS= read -r mount_point; do + [ -n "${mount_point}" ] && warn " kcd sftp unmount # ${mount_point}" + done <<<"${live}" + return 1 + fi + fi + if [[ -d "${CONFIG_DIR}" ]]; then - rm -rf "${CONFIG_DIR}" + _rm_rf "${CONFIG_DIR}" success "Removed ${CONFIG_DIR}" removed=true fi if [[ -d "${STATE_DIR}" ]]; then - rm -rf "${STATE_DIR}" + _rm_rf "${STATE_DIR}" success "Removed ${STATE_DIR} (paired device fingerprints deleted)" removed=true fi @@ -186,7 +261,14 @@ else skip "Config and state preserved" printf "\n" printf " ${BLUE}Tip:${RESET} To remove later, run:\n" - printf " rm -rf ${CONFIG_DIR} ${STATE_DIR}\n" + # Print the boundary-aware form this system actually supports, rather than a + # bare `rm -rf` that would descend into a live SFTP mount. + printf " rm${KCD_RM_ONE_FS[*]:+ ${KCD_RM_ONE_FS[*]}} -rf %s %s\n" \ + "${CONFIG_DIR}" "${STATE_DIR}" + if [[ "${#KCD_RM_ONE_FS[@]}" -eq 0 ]]; then + printf " ${YELLOW}Note:${RESET} this system's rm has no --one-file-system, so\n" + printf " unmount any kcd SFTP mount before deleting by hand.\n" + fi fi fi