From 95d6c69ca3b6804af267a07d73bebf00b9500775 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 2 Oct 2026 04:12:27 +0300 Subject: [PATCH 01/17] fix(sftp): mask the SFTP password in info output `kcd sftp info` printed the live SFTP password in both human and --json output, so an informational command put a working credential into scrollback, logs and any $(...) capture. The JSON tag had no omitempty, so the secret was emitted on every machine-readable call too. Masking happens on the daemon side: the credential no longer crosses the IPC socket unless the client explicitly asks for it, rather than being transmitted and then hidden at print time. omitempty keeps the field out of the response entirely, so a masked response cannot be mistaken for a device whose SFTP server genuinely has no password. The sftp.mount event still carries the password, which is the path clients use to mount. It is a separate payload and is deliberately untouched. Also renumbers CLIENT_GUIDE section 5, where 5.7 and 5.10 were each used twice, and documents that the event stream is where credentials come from. --- cmd/kcd/cli_sftp.go | 13 ++++++++++-- docs/CLI.md | 8 +++++++- docs/CLIENT_GUIDE.md | 15 ++++++++------ docs/IPC_PROTOCOL.md | 7 ++++++- internal/daemon/ipc_routes_sftp.go | 4 ++-- internal/ipc/proto.go | 13 +++++++++++- internal/plugins/sftp/info_test.go | 33 ++++++++++++++++++++++++++++++ internal/plugins/sftp/request.go | 20 ++++++++++++------ internal/plugins/sftp/types.go | 7 ++++++- pkg/client/client_sftp.go | 7 +++++-- 10 files changed, 105 insertions(+), 22 deletions(-) create mode 100644 internal/plugins/sftp/info_test.go diff --git a/cmd/kcd/cli_sftp.go b/cmd/kcd/cli_sftp.go index dc8eb73..38b9b6d 100644 --- a/cmd/kcd/cli_sftp.go +++ b/cmd/kcd/cli_sftp.go @@ -43,6 +43,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 +56,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,10 +66,14 @@ 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 len(info.Volumes) > 0 { fmt.Println("\nStorage volumes:") diff --git a/docs/CLI.md b/docs/CLI.md index 77d392e..637f7fb 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -699,9 +699,15 @@ 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** ``` 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..ccfc2f7 100644 --- a/docs/IPC_PROTOCOL.md +++ b/docs/IPC_PROTOCOL.md @@ -533,9 +533,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 diff --git a/internal/daemon/ipc_routes_sftp.go b/internal/daemon/ipc_routes_sftp.go index b6b7292..b915355 100644 --- a/internal/daemon/ipc_routes_sftp.go +++ b/internal/daemon/ipc_routes_sftp.go @@ -49,9 +49,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"} } diff --git a/internal/ipc/proto.go b/internal/ipc/proto.go index 5d52f94..979bb1f 100644 --- a/internal/ipc/proto.go +++ b/internal/ipc/proto.go @@ -170,12 +170,23 @@ 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"` } 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/request.go b/internal/plugins/sftp/request.go index b3fd556..0667687 100644 --- a/internal/plugins/sftp/request.go +++ b/internal/plugins/sftp/request.go @@ -144,7 +144,13 @@ func (p *SftpPlugin) MountLocally(ctx context.Context, deviceID string) (string, // 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. +func (p *SftpPlugin) Info(deviceID string, includePassword bool) *SftpInfo { p.mu.RLock() defer p.mu.RUnlock() body, ok := p.lastBody[deviceID] @@ -152,11 +158,13 @@ func (p *SftpPlugin) Info(deviceID string) *SftpInfo { return nil } 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, + } + if includePassword { + info.Password = body.Password } for i, mp := range body.MultiPaths { name := mp diff --git a/internal/plugins/sftp/types.go b/internal/plugins/sftp/types.go index 2625d4a..775b666 100644 --- a/internal/plugins/sftp/types.go +++ b/internal/plugins/sftp/types.go @@ -61,11 +61,16 @@ 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"` } diff --git a/pkg/client/client_sftp.go b/pkg/client/client_sftp.go index 60f7bd2..0aa097b 100644 --- a/pkg/client/client_sftp.go +++ b/pkg/client/client_sftp.go @@ -13,8 +13,11 @@ func (c *Client) SftpMount(deviceID string) error { } // 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 } From 2b0aa2aaca4f262e914cc8f2df004a2c12e150e1 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 2 Oct 2026 04:20:29 +0300 Subject: [PATCH 02/17] fix(sftp): make mount idempotent and unmount errors distinguishable Mounting a device that was already mounted ran sshfs onto a live mount point. That fails with "fusermount3: failed to access mountpoint ...: Permission denied", which reads like a FUSE permissions problem and sent users to edit /etc/fuse.conf when it was already correct. mountWithBody now returns the existing mount point instead, so a repeat request is also a cheap way to re-open the file manager. The check goes in mountWithBody rather than in the two callers, so both paths get it from one place. Unmount dropped its tracked state before attempting fusermount, so a failed unmount left the daemon reporting the device as unmounted while the FUSE mount was still live and holding an sshfs process. State is now dropped only once the mount is actually released, which makes the failure retryable. Errors are split into the three cases a client must handle differently: not mounted, stale mount that could not be released, and success. The /etc/fuse.conf hint fired on any message containing "fusermount", including errors that had nothing to do with fuse.conf. It is now scoped to permission-shaped failures. The duplicate-mount message is textually identical to a real permission failure, so the ambiguity is resolved by the idempotency fix rather than by the hint. --- docs/CLI.md | 18 +++- internal/plugins/sftp/mount.go | 109 ++++++++++++++++------ internal/plugins/sftp/mount_test.go | 139 ++++++++++++++++++++++++++++ 3 files changed, 235 insertions(+), 31 deletions(-) create mode 100644 internal/plugins/sftp/mount_test.go diff --git a/docs/CLI.md b/docs/CLI.md index 637f7fb..3352a99 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -750,17 +750,27 @@ 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 `. +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. ### 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` | The daemon has no mount for this device | +| `stale SFTP mount at could not be released` | The mount exists but `fusermount` failed; retry, or unmount by path | +| _(none)_ | Unmounted cleanly | ### sftp browse diff --git a/internal/plugins/sftp/mount.go b/internal/plugins/sftp/mount.go index d55beab..2bb03a9 100644 --- a/internal/plugins/sftp/mount.go +++ b/internal/plugins/sftp/mount.go @@ -80,10 +80,47 @@ 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) { + // 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), + ) + p.autoOpen(existing) + return existing, nil + } + baseDir := p.cfg.MountDir if baseDir == "" { baseDir = os.TempDir() @@ -121,10 +158,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) } @@ -153,22 +188,29 @@ func (p *SftpPlugin) mountWithBody(ctx context.Context, deviceID string, body Sf log.String("browse_path", browsePath), ) - // 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 } +// 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) { @@ -198,21 +240,20 @@ func (p *SftpPlugin) OnDisconnect(dev device.Sender) { // 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() + p.mu.RLock() mountPoint, ok := p.mountPoints[deviceID] - if ok { - delete(p.mountPoints, deviceID) - } pid, hasPID := p.mountPIDs[deviceID] - if hasPID { - delete(p.mountPIDs, deviceID) - } - p.mu.Unlock() + p.mu.RUnlock() if !ok { - return fmt.Errorf("no active SFTP mount for device %s", deviceID) + return fmt.Errorf("not mounted: no SFTP mount for device %s", deviceID) } p.logger.Info("unmounting SFTP share", log.String("mount_point", mountPoint)) @@ -248,13 +289,27 @@ 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", + // Keep the tracked state: the sshfs process is already gone, but the + // FUSE mount point may still be held, and the caller needs a retry to + // clear it. + detail := strings.TrimSpace(string(out)) + if detail != "" { + detail = ": " + detail + } + p.logger.Warn("fusermount failed", log.String("mount_point", mountPoint), log.Error(err), log.String("output", strings.TrimSpace(string(out))), ) + return fmt.Errorf("stale SFTP mount at %s could not be released, %s failed: %v%s", + mountPoint, tool, err, detail) } + p.mu.Lock() + delete(p.mountPoints, deviceID) + delete(p.mountPIDs, deviceID) + p.mu.Unlock() + _ = os.Remove(mountPoint) p.logger.Info("SFTP unmounted", log.String("mount_point", mountPoint)) return nil diff --git a/internal/plugins/sftp/mount_test.go b/internal/plugins/sftp/mount_test.go new file mode 100644 index 0000000..c34ebd3 --- /dev/null +++ b/internal/plugins/sftp/mount_test.go @@ -0,0 +1,139 @@ +package sftp + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/bethropolis/kcd/internal/config" + "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, "") + 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 TestUnmount_KeepsStateWhenFusermountFails(t *testing.T) { + dir := t.TempDir() + 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 + + // No sshfs process is running for this path, so fusermount has nothing to + // release and fails. Tracked state must survive so the caller can retry. + err := p.Unmount("dev1") + if err == nil { + t.Skip("fusermount succeeded on a path that was never mounted; cannot exercise the failure path") + } + if !strings.Contains(err.Error(), "could not be released") { + t.Errorf("error should name the stale mount and the failing tool, got: %v", err) + } + if _, stillTracked := p.mountPoints["dev1"]; !stillTracked { + t.Error("mount state was dropped even though the unmount failed; the daemon would claim unmounted while the FUSE mount is live") + } +} + +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) + } + }) + } +} From 3843fb9e69378b17943d5f98ca817745525b03ef Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 2 Oct 2026 04:36:11 +0300 Subject: [PATCH 03/17] feat(sftp): expose mount state via events and device summaries A client had no way to learn whether a device's storage was mounted. The only signal was sftp.mount, which fires when credentials arrive rather than when a mount completes, and mountPoints was in-memory and unexported. Clients that needed it were scraping /proc/mounts, duplicating state the daemon already had. Adds sftp.mounted and sftp.unmounted for the transitions themselves, plus mounted/mountPoint on SftpInfo and an sftp object on DeviceSummary, so both the info command and state.snapshot can answer the question without local inspection. The snapshot field is omitted rather than reported as a standing "not mounted", since a permanent false would force clients to special-case a device that has simply never been mounted. The idempotent path deliberately publishes nothing: reusing a mount point is not a state transition. Also corrects the documented `sftp info` example, which showed a Device line, numbered volume arrows and spacing the code never emitted, and claimed an errored request surfaces errorMessage -- it does not, because a failed response is never cached, so the error arrives on the event instead. --- cmd/kcd/cli_sftp.go | 5 +++ cmd/kcd/format_event.go | 16 ++++++++ cmd/kcd/format_event_test.go | 18 ++++++++- docs/ARCHITECTURE.md | 6 ++- docs/CLI.md | 26 ++++++++---- docs/IPC_PROTOCOL.md | 33 ++++++++++++++++ internal/events/bus.go | 46 ++++++++++++---------- internal/ipc/proto.go | 3 ++ internal/ipc/snapshot.go | 17 ++++++++ internal/plugins/sftp/mount.go | 23 +++++++++++ internal/plugins/sftp/mount_test.go | 61 +++++++++++++++++++++++++++++ internal/plugins/sftp/request.go | 13 ++++++ internal/plugins/sftp/types.go | 6 +++ 13 files changed, 241 insertions(+), 32 deletions(-) diff --git a/cmd/kcd/cli_sftp.go b/cmd/kcd/cli_sftp.go index 38b9b6d..3d46a10 100644 --- a/cmd/kcd/cli_sftp.go +++ b/cmd/kcd/cli_sftp.go @@ -75,6 +75,11 @@ Use 'kcd sftp request' first to populate the cache.`, fmt.Printf("User: %s\n", info.User) 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 { diff --git a/cmd/kcd/format_event.go b/cmd/kcd/format_event.go index 6d023f9..d119d2c 100644 --- a/cmd/kcd/format_event.go +++ b/cmd/kcd/format_event.go @@ -25,6 +25,16 @@ 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 +112,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"]) diff --git a/cmd/kcd/format_event_test.go b/cmd/kcd/format_event_test.go index c83daf6..3a42843 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"}}, @@ -251,7 +266,8 @@ 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.TypePairAccepted, events.TypeMprisUpdate, events.TypeSMSIncoming, events.TypeSMSAttachment, events.TypePingReceived, events.TypeConnectivityUpdate, events.TypeTelephonyRinging, events.TypeTelephonyMissed, events.TypeTelephonyTalking, events.TypeTelephonyCanceled, events.TypeVolumeUpdate, diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 58415ad..8dbfe16 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -292,7 +292,7 @@ goroutine leak when the child wedges. | `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. | | `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, and `Info(deviceID, includePassword)` gates the credential — `kcd sftp info` masks it by default because the output lands in scrollback and logs | | `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 3352a99..0500228 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -711,18 +711,26 @@ 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 (/home/user/Downloads/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 @@ -1042,6 +1050,8 @@ 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}` | | `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/IPC_PROTOCOL.md b/docs/IPC_PROTOCOL.md index ccfc2f7..ca9b2bd 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` @@ -1162,6 +1163,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": "/home/user/Downloads/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": "/home/user/Downloads/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` diff --git a/internal/events/bus.go b/internal/events/bus.go index 6bd0de1..d249418 100644 --- a/internal/events/bus.go +++ b/internal/events/bus.go @@ -12,27 +12,31 @@ 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" TypeNotificationCanceled EventType = "notification.canceled" TypeVolumeUpdate EventType = "volume.update" TypeSMSIncoming EventType = "sms.incoming" diff --git a/internal/ipc/proto.go b/internal/ipc/proto.go index 979bb1f..e8a3d64 100644 --- a/internal/ipc/proto.go +++ b/internal/ipc/proto.go @@ -189,6 +189,9 @@ type SftpInfoResponse struct { 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. 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/sftp/mount.go b/internal/plugins/sftp/mount.go index 2bb03a9..add336f 100644 --- a/internal/plugins/sftp/mount.go +++ b/internal/plugins/sftp/mount.go @@ -14,6 +14,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/plugin" ) @@ -187,12 +188,33 @@ 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) 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. @@ -312,6 +334,7 @@ func (p *SftpPlugin) Unmount(deviceID string) error { _ = os.Remove(mountPoint) p.logger.Info("SFTP unmounted", log.String("mount_point", mountPoint)) + p.publishUnmounted(deviceID, mountPoint) return nil } diff --git a/internal/plugins/sftp/mount_test.go b/internal/plugins/sftp/mount_test.go index c34ebd3..9b9d7f8 100644 --- a/internal/plugins/sftp/mount_test.go +++ b/internal/plugins/sftp/mount_test.go @@ -6,8 +6,10 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/bethropolis/kcd/internal/config" + "github.com/bethropolis/kcd/internal/events" "github.com/bethropolis/kcd/internal/log" ) @@ -137,3 +139,62 @@ func TestSshfsHint(t *testing.T) { }) } } + +// 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{}, ""); 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) + } +} diff --git a/internal/plugins/sftp/request.go b/internal/plugins/sftp/request.go index 0667687..5235582 100644 --- a/internal/plugins/sftp/request.go +++ b/internal/plugins/sftp/request.go @@ -150,6 +150,10 @@ func (p *SftpPlugin) MountLocally(ctx context.Context, deviceID string) (string, // 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() @@ -162,6 +166,10 @@ func (p *SftpPlugin) Info(deviceID string, includePassword bool) *SftpInfo { Port: body.Port, User: body.User, Path: body.Path, + // Read under the same lock as the credential cache so the two halves + // of the response cannot disagree. + Mounted: p.mountPoints[deviceID] != "", + MountPoint: p.mountPoints[deviceID], } if includePassword { info.Password = body.Password @@ -211,3 +219,8 @@ func (p *SftpPlugin) MountedPath(deviceID string) string { defer p.mu.RUnlock() return p.mountPoints[deviceID] } + +// IsMounted reports whether a device's filesystem is currently mounted. +func (p *SftpPlugin) IsMounted(deviceID string) bool { + return p.MountedPath(deviceID) != "" +} diff --git a/internal/plugins/sftp/types.go b/internal/plugins/sftp/types.go index 775b666..eaccddb 100644 --- a/internal/plugins/sftp/types.go +++ b/internal/plugins/sftp/types.go @@ -73,6 +73,12 @@ type SftpInfo struct { 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" } From 97111999d03980cbf0172578622bc03e6df63f5a Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 2 Oct 2026 04:49:39 +0300 Subject: [PATCH 04/17] feat(runcommand): publish execution output to the event bus Command output reached only the phone's in-app card and a capped desktop notification, so it was invisible to kcd watch and to any other client. Clients that wanted to follow a command had no way to. Publishes runcommand.output in three shapes keyed by status: started, output (a batch of stdout/stderr lines), and finished (carrying the whole capped transcript plus the outcome). Batches are gated on HasSubscribers while the lifecycle events are not. 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 those drop warnings fire for output nobody asked for. Subscribers still get the result either way, through the finished event. Only an execution that actually started publishes, so a client never sees a finish with no matching start. The outputStream.command field, previously assigned and never read, now carries the command key in the payload. Renumbers IPC_PROTOCOL section 5 to make room: MPRIS was 5.14 and is now 5.15. --- cmd/kcd/format_event.go | 56 ++++++++++++ cmd/kcd/format_event_test.go | 28 ++++++ docs/ARCHITECTURE.md | 2 +- docs/CLI.md | 1 + docs/IPC_PROTOCOL.md | 42 ++++++++- internal/daemon/plugins.go | 2 +- internal/events/bus.go | 6 +- internal/plugins/runcommand/list_test.go | 10 +-- internal/plugins/runcommand/output.go | 60 ++++++++++++- internal/plugins/runcommand/output_test.go | 89 ++++++++++++++++++- internal/plugins/runcommand/runcommand.go | 5 +- .../plugins/runcommand/runcommand_test.go | 3 + 12 files changed, 289 insertions(+), 15 deletions(-) diff --git a/cmd/kcd/format_event.go b/cmd/kcd/format_event.go index d119d2c..57c263c 100644 --- a/cmd/kcd/format_event.go +++ b/cmd/kcd/format_event.go @@ -19,6 +19,59 @@ 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 { @@ -197,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") diff --git a/cmd/kcd/format_event_test.go b/cmd/kcd/format_event_test.go index 3a42843..31c3fb8 100644 --- a/cmd/kcd/format_event_test.go +++ b/cmd/kcd/format_event_test.go @@ -201,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}}, @@ -267,6 +294,7 @@ func TestFormatEventSurvivesBadPayloads(t *testing.T) { types := []events.EventType{ events.TypeBatteryUpdate, events.TypeBatteryThreshold, events.TypeNotification, events.TypeSftpMount, events.TypeSftpMounted, events.TypeSftpUnmounted, + events.TypeRunCommandOutput, events.TypePairAccepted, events.TypeMprisUpdate, events.TypeSMSIncoming, events.TypeSMSAttachment, events.TypePingReceived, events.TypeConnectivityUpdate, events.TypeTelephonyRinging, events.TypeTelephonyMissed, diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 8dbfe16..6c9760e 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -290,7 +290,7 @@ 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`. 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, and `Info(deviceID, includePassword)` gates the credential — `kcd sftp info` masks it by default because the output lands in scrollback and logs | | `share` | `kdeconnect.share.request` | Streaming file receive + URL/text handling; fires progress events | diff --git a/docs/CLI.md b/docs/CLI.md index 0500228..e670da0 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -1052,6 +1052,7 @@ kcd watch [--events ] [--json] | `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/IPC_PROTOCOL.md b/docs/IPC_PROTOCOL.md index ca9b2bd..a80d6ca 100644 --- a/docs/IPC_PROTOCOL.md +++ b/docs/IPC_PROTOCOL.md @@ -1296,7 +1296,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/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/events/bus.go b/internal/events/bus.go index d249418..e86db98 100644 --- a/internal/events/bus.go +++ b/internal/events/bus.go @@ -35,8 +35,10 @@ const ( 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" + 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/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, ) From 4810c2f4c3d43a53a66c51d183691b8494e7932c Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 2 Oct 2026 04:57:09 +0300 Subject: [PATCH 05/17] fix(sftp): reconcile mounts orphaned by a daemon restart mountPoints was in-memory with no persistence, so after a daemon restart the FUSE mount was still live in the kernel while the daemon had forgotten it. Unmount then reported "not mounted" while the mount sat there holding an sshfs process, and a later mount ran sshfs over it and failed. Nothing is persisted. The kernel is the source of truth and the map is a cache: MountedPath falls back to parsing /proc/mounts for a fuse.* entry named kcd-sftp- and adopts what it finds into the cache. A persisted entry would go stale after a crash and need reconciling anyway, so it would buy nothing. /proc is read directly rather than through findmnt, matching findSSHFSPID, so there is no new dependency or PATH lookup. Kernel octal escaping is reversed for mount points containing spaces, and non-FUSE entries are left alone -- adopting one and later running fusermount against it would be worse than not adopting. Unmount resolves through the same path, so an adopted mount is unmountable by device id, and looks up the sshfs PID when none is tracked so the graceful shutdown still happens. OnDisconnect uses it too, so an orphan is cleaned up instead of surviving every disconnect. --- docs/ARCHITECTURE.md | 2 +- docs/CLI.md | 5 + internal/plugins/sftp/mount.go | 34 +++--- internal/plugins/sftp/mounttable.go | 80 ++++++++++++++ internal/plugins/sftp/mounttable_test.go | 127 +++++++++++++++++++++++ internal/plugins/sftp/request.go | 54 ++++++---- 6 files changed, 271 insertions(+), 31 deletions(-) create mode 100644 internal/plugins/sftp/mounttable.go create mode 100644 internal/plugins/sftp/mounttable_test.go diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 6c9760e..8d36f1d 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -292,7 +292,7 @@ goroutine leak when the child wedges. | `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. 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`. 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, and `Info(deviceID, includePassword)` gates the credential — `kcd sftp info` masks it by default because the output lands in scrollback and logs | +| `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, 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` falls back to parsing `/proc/mounts` for a `fuse.*` entry named `kcd-sftp-` and adopts what it finds, so a mount that outlived a daemon restart is still reported mounted, cleaned up on disconnect, and unmountable by device id | | `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` | diff --git a/docs/CLI.md b/docs/CLI.md index e670da0..5852145 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -780,6 +780,11 @@ to each: | `stale SFTP mount at could not be released` | The mount exists but `fusermount` failed; retry, or unmount by path | | _(none)_ | Unmounted cleanly | +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 Request fresh SFTP credentials and list available storage volumes or mount diff --git a/internal/plugins/sftp/mount.go b/internal/plugins/sftp/mount.go index add336f..d1d14fe 100644 --- a/internal/plugins/sftp/mount.go +++ b/internal/plugins/sftp/mount.go @@ -236,12 +236,10 @@ func (p *SftpPlugin) autoOpen(browsePath string) { 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), ) @@ -269,18 +267,30 @@ func (p *SftpPlugin) OnDisconnect(dev device.Sender) { // 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.RLock() - mountPoint, ok := p.mountPoints[deviceID] - pid, hasPID := p.mountPIDs[deviceID] - p.mu.RUnlock() - - if !ok { + // 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 == "" { return fmt.Errorf("not mounted: no SFTP mount for device %s", deviceID) } + 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) diff --git a/internal/plugins/sftp/mounttable.go b/internal/plugins/sftp/mounttable.go new file mode 100644 index 0000000..406f213 --- /dev/null +++ b/internal/plugins/sftp/mounttable.go @@ -0,0 +1,80 @@ +package sftp + +import ( + "bufio" + "os" + "path/filepath" + "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. +func liveMountPoint(deviceID string) (string, bool) { + f, err := os.Open(mountTablePath) + if err != nil { + return "", false + } + defer f.Close() + + // The daemon only ever creates mounts named after the device, so the + // basename is already specific to us; the fstype check keeps an unrelated + // mount that happens to share the name from being adopted. + want := "kcd-sftp-" + deviceID + scanner := bufio.NewScanner(f) + for scanner.Scan() { + fields := strings.Fields(scanner.Text()) + if len(fields) < 3 { + continue + } + if !strings.HasPrefix(fields[2], "fuse") { + continue + } + if filepath.Base(unescapeMountPath(fields[1])) == want { + return unescapeMountPath(fields[1]), 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..c19c10f --- /dev/null +++ b/internal/plugins/sftp/mounttable_test.go @@ -0,0 +1,127 @@ +package sftp + +import ( + "os" + "path/filepath" + "testing" +) + +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) + } + }) + } +} + +func TestLiveMountPoint_IgnoresUnrelatedMounts(t *testing.T) { + // A device with no live mount must never be reported as mounted. The test + // machine may or may not have any FUSE mounts, so the only safe assertion + // is that an id we certainly never mounted comes back absent. + if mp, ok := liveMountPoint("kcd-nonexistent-device-9a5c23ea7195"); ok { + t.Errorf("invented device reported as mounted at %q", mp) + } +} + +// 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 device whose mount is absent from the cache but present in the kernel is +// exactly 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 is still holding an sshfs process. +func TestMountedPath_AdoptsOrphanFromKernel(t *testing.T) { + useFakeMountTable(t, `sysfs /sys sysfs rw 0 0 +proc /proc proc rw 0 0 +phone:/storage/emulated/0 /home/user/Downloads/kcd/mnt/kcd-sftp-dev1 fuse.sshfs rw,nosuid,nodev 0 0 +`) + + p := newTestPlugin(t, t.TempDir()) + if _, cached := p.mountPoints["dev1"]; cached { + t.Fatal("test precondition: cache should start empty") + } + + got := p.MountedPath("dev1") + if got != "/home/user/Downloads/kcd/mnt/kcd-sftp-dev1" { + t.Fatalf("MountedPath = %q, want the kernel's mount point", got) + } + if _, adopted := p.mountPoints["dev1"]; !adopted { + t.Error("orphan was resolved but not adopted into the cache, so every lookup re-reads /proc") + } + if !p.IsMounted("dev1") { + t.Error("IsMounted = false for a live mount the daemon had forgotten") + } +} + +// A different device's mount must not be adopted for this one. +func TestMountedPath_DoesNotAdoptAnotherDevice(t *testing.T) { + useFakeMountTable(t, `phone:/storage/emulated/0 /mnt/kcd-sftp-other fuse.sshfs rw 0 0 +`) + + p := newTestPlugin(t, t.TempDir()) + 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) { + useFakeMountTable(t, `/dev/sdb1 /mnt/kcd-sftp-dev1 ext4 rw 0 0 +`) + + p := newTestPlugin(t, t.TempDir()) + if got := p.MountedPath("dev1"); got != "" { + t.Errorf("adopted a non-FUSE mount: %q", got) + } +} + +// Mount points with spaces are octal-escaped by the kernel. +func TestMountedPath_UnescapesKernelPath(t *testing.T) { + useFakeMountTable(t, `phone:/storage/ABCD /mnt/my\040disk/kcd-sftp-dev1 fuse.sshfs rw 0 0 +`) + + p := newTestPlugin(t, t.TempDir()) + if got := p.MountedPath("dev1"); got != "/mnt/my disk/kcd-sftp-dev1" { + t.Errorf("MountedPath = %q, want the unescaped path", got) + } +} + +// 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") + } +} diff --git a/internal/plugins/sftp/request.go b/internal/plugins/sftp/request.go index 5235582..fa4b735 100644 --- a/internal/plugins/sftp/request.go +++ b/internal/plugins/sftp/request.go @@ -156,31 +156,29 @@ func (p *SftpPlugin) MountLocally(ctx context.Context, deviceID string) (string, // 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, - Path: body.Path, - // Read under the same lock as the credential cache so the two halves - // of the response cannot disagree. - Mounted: p.mountPoints[deviceID] != "", - MountPoint: p.mountPoints[deviceID], + IP: body.IP, + Port: body.Port, + User: body.User, + Path: body.Path, + Volumes: volumes, + Mounted: mountPoint != "", + MountPoint: mountPoint, } if includePassword { info.Password = body.Password } - 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}) - } return info } @@ -214,10 +212,30 @@ 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 + } + + mountPoint, live := liveMountPoint(deviceID) + if !live { + return "" + } + // Adopt it, so later calls hit the cache and Unmount knows the path. + p.mu.Lock() + p.mountPoints[deviceID] = mountPoint + p.mu.Unlock() + p.logger.Info("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. From e9b0d80b133aa7d2d0c89a5c1815ec7f2428f532 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 2 Oct 2026 05:02:22 +0300 Subject: [PATCH 06/17] fix(device): emit a full device summary on device.added device.added published the device name as a bare string, so a client receiving it had an event envelope carrying only the device id and had to synthesise the rest -- inventing a state and a connected flag, then waiting for the next snapshot to correct the guess. The payload is now DeviceInfo, the same shape one devices entry uses. Cached sub-states are absent, which is correct rather than a gap: at discovery no battery, media, signal or mount has been reported yet, and each arrives in its own event. Adds Device.Info() so the daemon's persistence path and this publish build the view the same way instead of repeating the field list. LastIP and LastPort are omitempty and already exposed by state.snapshot, so this publishes nothing new. The watch renderer still accepts a bare string so an older payload shape does not blank the line. --- cmd/kcd/format_event.go | 9 ++++++++- cmd/kcd/format_event_test.go | 2 +- docs/CLI.md | 2 +- docs/IPC_PROTOCOL.md | 19 ++++++++++++++++++- internal/device/device_test.go | 32 ++++++++++++++++++++++++++++++++ internal/device/registry.go | 5 ++++- internal/device/state.go | 20 ++++++++++++++++++++ 7 files changed, 84 insertions(+), 5 deletions(-) diff --git a/cmd/kcd/format_event.go b/cmd/kcd/format_event.go index 57c263c..0da0ef5 100644 --- a/cmd/kcd/format_event.go +++ b/cmd/kcd/format_event.go @@ -267,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 31c3fb8..c4ef846 100644 --- a/cmd/kcd/format_event_test.go +++ b/cmd/kcd/format_event_test.go @@ -254,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", }, { diff --git a/docs/CLI.md b/docs/CLI.md index 5852145..a7fbad3 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -1033,7 +1033,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 | diff --git a/docs/IPC_PROTOCOL.md b/docs/IPC_PROTOCOL.md index a80d6ca..1f7a7bc 100644 --- a/docs/IPC_PROTOCOL.md +++ b/docs/IPC_PROTOCOL.md @@ -859,7 +859,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` 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 From 79176020004298eb459ca35031832a422ad16af2 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 2 Oct 2026 05:05:24 +0300 Subject: [PATCH 07/17] refactor: tighten comments in the dial and transport paths Comment volume, not comment quality, was the problem: transport.go carried 80 comment lines in 311 and device_core.go 68 in 210, with 5-7 line blocks per field. The content is mostly load-bearing -- the security reason lastPort is separate from discoveryIP, why a same-IP sighting must not trigger a redial -- so this compresses rather than deletes. Drops the "TCP Listener" and "UDP/mDNS Listener" banners, which restate the function name, and folds the dial-policy preamble into one paragraph. Every invariant the longer comments encoded is preserved, including the security ones and the reason same-IP roam repair has to arrive inbound. --- internal/daemon/transport.go | 55 ++++++++++++++------------------ internal/device/device_core.go | 58 ++++++++++++++++------------------ 2 files changed, 51 insertions(+), 62 deletions(-) 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). From e50033cfbb38b0cecea6a3e6850510ef3f56c0d1 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 2 Oct 2026 05:08:51 +0300 Subject: [PATCH 08/17] docs: correct the reload instructions, document SIGHUP, tighten the example The example config claimed "Reload requires a daemon restart". That was simply wrong: daemon.go hot-reloads the [commands] table, the notification filters and the log level on SIGHUP. The feature was documented nowhere, and the unit file had no ExecReload, so the obvious instruction (systemctl --user reload kcd) would have failed -- systemd has no way to signal a unit without one. Adds it, and a table in CLI.md saying which settings a reload covers and which still need a restart. The old sentence was also garbled: it said filters were not applied on reload, when filters are the main thing that is. The example config was carrying content that belongs elsewhere and will drift: ~20 lines of CLI reference duplicating CLI.md, four lines of the blocked-numbers caveat that ARCHITECTURE.md already states, and five lines of broadcast architecture. A config file should say what a knob does, its default and its external dependency; nothing else. Every key, default, dependency note and cross-field constraint is kept -- those are what make the file worth reading. 323 lines down to 310, mostly prose removed rather than settings. Section headers also had a blank line after them inconsistently. --- docs/CLI.md | 17 +++++++-- packaging/kcd-user.service | 6 ++++ packaging/kcd.example.toml | 71 ++++++++++++++++---------------------- 3 files changed, 49 insertions(+), 45 deletions(-) diff --git a/docs/CLI.md b/docs/CLI.md index a7fbad3..d7e431f 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 | |---|---| 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..ea8fa0e 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. @@ -307,15 +297,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 ───────────────────────────────────────── From 3e216871b3ff96647beba0cd98c33b8c6c81b441 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 2 Oct 2026 06:42:45 +0300 Subject: [PATCH 09/17] fix(sftp): clear state for a mount the kernel has already dropped Unmount could wedge a device permanently. The daemon still had it cached as mounted, but the FUSE mount was already gone -- the connection had died, or it had been released by hand -- so fusermount failed with "entry for ... not found in /etc/mtab" on every attempt. Keeping the cached state on failure is right for a mount that really is still there, and wrong for one that is not: there is nothing to release, so the retry could never succeed and the device could neither mount nor unmount again. Unmount now asks the kernel, which is already the source of truth for the lookup, before and after the release attempt. A mount that has gone is forgotten and reported unmounted, and the leftover directory is removed -- which also unblocks the next mount, since MkdirAll over an existing stale directory would otherwise leave the same trap. A mount that is present but unresponsive (a dead FUSE connection refuses a normal unmount with "Transport endpoint is not connected") is retried lazily with -uz, and only a mount the kernel still reports keeps its state and its retryable error. Observed on 9a5c23ea_... against v1.21.0 and reproduced in TestUnmount_ClearsStateWhenKernelHasNoMount, whose failure message matches the reported log line exactly. --- docs/ARCHITECTURE.md | 2 +- docs/CLI.md | 10 ++- internal/plugins/sftp/mount.go | 108 +++++++++++++++++++++++----- internal/plugins/sftp/mount_test.go | 102 +++++++++++++++++++------- internal/plugins/sftp/mounttable.go | 36 +++++++--- internal/plugins/sftp/types.go | 13 ++++ 6 files changed, 216 insertions(+), 55 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 8d36f1d..b7e2939 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -292,7 +292,7 @@ goroutine leak when the child wedges. | `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. 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`. 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, 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` falls back to parsing `/proc/mounts` for a `fuse.*` entry named `kcd-sftp-` and adopts what it finds, so a mount that outlived a daemon restart is still reported mounted, cleaned up on disconnect, and unmountable by device id | +| `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` falls back to parsing `/proc/mounts` for a `fuse.*` entry named `kcd-sftp-` and adopts what it finds, so a mount that outlived a daemon restart is still reported mounted, cleaned up on disconnect, and unmountable by device id | | `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` | diff --git a/docs/CLI.md b/docs/CLI.md index d7e431f..6ef17a1 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -787,10 +787,16 @@ to each: | Error contains | Meaning | |---|---| -| `not mounted` | The daemon has no mount for this device | -| `stale SFTP mount at could not be released` | The mount exists but `fusermount` failed; retry, or unmount by path | +| `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 diff --git a/internal/plugins/sftp/mount.go b/internal/plugins/sftp/mount.go index d1d14fe..521b099 100644 --- a/internal/plugins/sftp/mount.go +++ b/internal/plugins/sftp/mount.go @@ -122,11 +122,7 @@ func (p *SftpPlugin) mountWithBody(ctx context.Context, deviceID string, body Sf return existing, nil } - baseDir := p.cfg.MountDir - if baseDir == "" { - baseDir = os.TempDir() - } - mountPoint := filepath.Join(baseDir, "kcd-sftp-"+deviceID) + mountPoint := p.mountPointFor(deviceID) if err := os.MkdirAll(mountPoint, 0700); err != nil { return "", fmt.Errorf("create mount point %s: %w", mountPoint, err) } @@ -273,9 +269,32 @@ func (p *SftpPlugin) Unmount(deviceID string) error { // 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) } + // 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() @@ -320,23 +339,62 @@ 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 { - // Keep the tracked state: the sshfs process is already gone, but the - // FUSE mount point may still be held, and the caller needs a retry to - // clear it. - detail := strings.TrimSpace(string(out)) - if detail != "" { - detail = ": " + detail + 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("fusermount failed", + 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))), ) - return fmt.Errorf("stale SFTP mount at %s could not be released, %s failed: %v%s", - mountPoint, tool, err, detail) + 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) @@ -345,7 +403,21 @@ func (p *SftpPlugin) Unmount(deviceID string) error { _ = os.Remove(mountPoint) p.logger.Info("SFTP unmounted", log.String("mount_point", mountPoint)) p.publishUnmounted(deviceID, mountPoint) - return nil +} + +// 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) + 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. diff --git a/internal/plugins/sftp/mount_test.go b/internal/plugins/sftp/mount_test.go index 9b9d7f8..5328361 100644 --- a/internal/plugins/sftp/mount_test.go +++ b/internal/plugins/sftp/mount_test.go @@ -56,30 +56,6 @@ func TestUnmount_NotMountedIsDistinguishable(t *testing.T) { } } -func TestUnmount_KeepsStateWhenFusermountFails(t *testing.T) { - dir := t.TempDir() - 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 - - // No sshfs process is running for this path, so fusermount has nothing to - // release and fails. Tracked state must survive so the caller can retry. - err := p.Unmount("dev1") - if err == nil { - t.Skip("fusermount succeeded on a path that was never mounted; cannot exercise the failure path") - } - if !strings.Contains(err.Error(), "could not be released") { - t.Errorf("error should name the stale mount and the failing tool, got: %v", err) - } - if _, stillTracked := p.mountPoints["dev1"]; !stillTracked { - t.Error("mount state was dropped even though the unmount failed; the daemon would claim unmounted while the FUSE mount is live") - } -} - func TestSshfsHint(t *testing.T) { tests := []struct { name string @@ -198,3 +174,81 @@ func TestInfoReportsMountState(t *testing.T) { 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") + } +} diff --git a/internal/plugins/sftp/mounttable.go b/internal/plugins/sftp/mounttable.go index 406f213..755d4a8 100644 --- a/internal/plugins/sftp/mounttable.go +++ b/internal/plugins/sftp/mounttable.go @@ -25,27 +25,43 @@ var mountTablePath = "/proc/mounts" // recoverable without persisting anything -- which is what we want, since a // persisted entry would go stale after a crash and need reconciling anyway. func liveMountPoint(deviceID string) (string, bool) { + want := "kcd-sftp-" + deviceID + return eachMount(func(mountPoint string) bool { + return filepath.Base(mountPoint) == want + }) +} + +// 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() - // The daemon only ever creates mounts named after the device, so the - // basename is already specific to us; the fstype check keeps an unrelated - // mount that happens to share the name from being adopted. - want := "kcd-sftp-" + deviceID scanner := bufio.NewScanner(f) for scanner.Scan() { fields := strings.Fields(scanner.Text()) - if len(fields) < 3 { - continue - } - if !strings.HasPrefix(fields[2], "fuse") { + if len(fields) < 3 || !strings.HasPrefix(fields[2], "fuse") { continue } - if filepath.Base(unescapeMountPath(fields[1])) == want { - return unescapeMountPath(fields[1]), true + mountPoint := unescapeMountPath(fields[1]) + if match(mountPoint) { + return mountPoint, true } } return "", false diff --git a/internal/plugins/sftp/types.go b/internal/plugins/sftp/types.go index eaccddb..4f5a132 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" @@ -85,3 +87,14 @@ 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 ever uses 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. +func (p *SftpPlugin) mountPointFor(deviceID string) string { + baseDir := p.cfg.MountDir + if baseDir == "" { + baseDir = os.TempDir() + } + return filepath.Join(baseDir, "kcd-sftp-"+deviceID) +} From 941bf84e166a75b7bc18634dc6b54aacc53ccb60 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 2 Oct 2026 07:33:36 +0300 Subject: [PATCH 10/17] fix(sftp): move the default mount point out of user documents mount_dir defaulted to ~/Downloads/kcd/mnt, which is *inside* download_dir (~/Downloads/kcd) -- the cache for files this tool receives. So the phone's storage was mounted inside the very directory a user is most likely to wipe, and three ordinary things reach it: - `rm -rf ~/Downloads/kcd`, the natural way to reclaim space from received files. rm crosses filesystem boundaries by default; --one-file-system exists for this but is opt-in, and has no --no- variant, so it is off. - `rm -rf ~/Downloads`, same. - The file manager. AutoOpen is on by default and the mount sits under Downloads, so it appears as an ordinary folder and an empty-folder gesture deletes through it. Compounding it, backup tools archive $HOME, so Borg/Restic would traverse into the mount and pull the phone's storage into a backup -- or delete it during prune. The default is now $XDG_RUNTIME_DIR/kcd/mnt. That is what GNOME's own remote file access uses, it is a tmpfs, no backup tool walks it, and nothing routinely deletes it. Falls back to the state directory only when there is no user session, where the hazard warning applies instead. An empty mount_dir used to mean /tmp, which has the same problem (rm -rf /tmp/*), so it now means the default. Not a breaking change. kcd never writes kcd.toml, so Defaults() cannot overwrite a pinned value, and a configured mount_dir is still honoured -- including the old path. Upgrading users who pinned it see no change and can delete the line to opt into the new default. The state-directory fallback is a new helper rather than filepath.Join(StatePath(), ...), which would have produced .../devices.json/mnt. Mounts at the previous location stay findable: MountedPath falls back to the legacy path, preferring the configured one when both exist. Without that, upgrading would orphan exactly the mounts users already have. liveMountPoint's basename matching is replaced by an exact-path check, which is more precise and drops a helper. --- internal/config/config_store.go | 30 ++++++- internal/config/plugins.go | 6 +- internal/plugins/sftp/mounttable.go | 8 -- internal/plugins/sftp/mounttable_test.go | 110 +++++++++++++++-------- internal/plugins/sftp/request.go | 27 +++++- internal/plugins/sftp/types.go | 20 ++++- 6 files changed, 143 insertions(+), 58 deletions(-) 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..b97b665 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" ) @@ -163,8 +160,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/plugins/sftp/mounttable.go b/internal/plugins/sftp/mounttable.go index 755d4a8..7dfa18b 100644 --- a/internal/plugins/sftp/mounttable.go +++ b/internal/plugins/sftp/mounttable.go @@ -3,7 +3,6 @@ package sftp import ( "bufio" "os" - "path/filepath" "strings" ) @@ -24,13 +23,6 @@ var mountTablePath = "/proc/mounts" // 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. -func liveMountPoint(deviceID string) (string, bool) { - want := "kcd-sftp-" + deviceID - return eachMount(func(mountPoint string) bool { - return filepath.Base(mountPoint) == want - }) -} - // 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. diff --git a/internal/plugins/sftp/mounttable_test.go b/internal/plugins/sftp/mounttable_test.go index c19c10f..d0722d7 100644 --- a/internal/plugins/sftp/mounttable_test.go +++ b/internal/plugins/sftp/mounttable_test.go @@ -3,7 +3,10 @@ package sftp import ( "os" "path/filepath" + "strings" "testing" + + "github.com/bethropolis/kcd/internal/config" ) func TestUnescapeMountPath(t *testing.T) { @@ -31,15 +34,6 @@ func TestUnescapeMountPath(t *testing.T) { } } -func TestLiveMountPoint_IgnoresUnrelatedMounts(t *testing.T) { - // A device with no live mount must never be reported as mounted. The test - // machine may or may not have any FUSE mounts, so the only safe assertion - // is that an id we certainly never mounted comes back absent. - if mp, ok := liveMountPoint("kcd-nonexistent-device-9a5c23ea7195"); ok { - t.Errorf("invented device reported as mounted at %q", mp) - } -} - // useFakeMountTable points the reconciliation at a fixture for one test. func useFakeMountTable(t *testing.T, contents string) { t.Helper() @@ -52,39 +46,67 @@ func useFakeMountTable(t *testing.T, contents string) { t.Cleanup(func() { mountTablePath = orig }) } -// A device whose mount is absent from the cache but present in the kernel is -// exactly 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 is still holding an sshfs process. +// 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) { - useFakeMountTable(t, `sysfs /sys sysfs rw 0 0 -proc /proc proc rw 0 0 -phone:/storage/emulated/0 /home/user/Downloads/kcd/mnt/kcd-sftp-dev1 fuse.sshfs rw,nosuid,nodev 0 0 -`) - 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") } - - got := p.MountedPath("dev1") - if got != "/home/user/Downloads/kcd/mnt/kcd-sftp-dev1" { - t.Fatalf("MountedPath = %q, want the kernel's mount point", got) + 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 into the cache, so every lookup re-reads /proc") + 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 different device's mount must not be adopted for this one. -func TestMountedPath_DoesNotAdoptAnotherDevice(t *testing.T) { - useFakeMountTable(t, `phone:/storage/emulated/0 /mnt/kcd-sftp-other fuse.sshfs rw 0 0 -`) +// 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) } @@ -93,23 +115,27 @@ func TestMountedPath_DoesNotAdoptAnotherDevice(t *testing.T) { // 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) { - useFakeMountTable(t, `/dev/sdb1 /mnt/kcd-sftp-dev1 ext4 rw 0 0 -`) - 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 with spaces are octal-escaped by the kernel. +// Mount points containing spaces arrive octal-escaped from the kernel. func TestMountedPath_UnescapesKernelPath(t *testing.T) { - useFakeMountTable(t, `phone:/storage/ABCD /mnt/my\040disk/kcd-sftp-dev1 fuse.sshfs rw 0 0 -`) + // 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") - p := newTestPlugin(t, t.TempDir()) - if got := p.MountedPath("dev1"); got != "/mnt/my disk/kcd-sftp-dev1" { - t.Errorf("MountedPath = %q, want the unescaped path", got) + if got := p.MountedPath("dev1"); got != want { + t.Errorf("MountedPath = %q, want the unescaped %q", got, want) } } @@ -125,3 +151,15 @@ func TestMountedPath_AbsentLeavesCacheClean(t *testing.T) { 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 fa4b735..2c3da0f 100644 --- a/internal/plugins/sftp/request.go +++ b/internal/plugins/sftp/request.go @@ -223,15 +223,34 @@ func (p *SftpPlugin) MountedPath(deviceID string) string { return mountPoint } - mountPoint, live := liveMountPoint(deviceID) - if !live { + // 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 "" } - // Adopt it, so later calls hit the cache and Unmount knows the path. p.mu.Lock() p.mountPoints[deviceID] = mountPoint p.mu.Unlock() - p.logger.Info("adopted SFTP mount left by a previous daemon session", + p.logger.Debug("adopted SFTP mount left by a previous daemon session", log.String("device_id", deviceID), log.String("mount_point", mountPoint), ) diff --git a/internal/plugins/sftp/types.go b/internal/plugins/sftp/types.go index 4f5a132..e96fe39 100644 --- a/internal/plugins/sftp/types.go +++ b/internal/plugins/sftp/types.go @@ -88,13 +88,29 @@ 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 ever uses for a device. +// 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 = os.TempDir() + 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) +} From e846a99de358ea225197a2ba8bc5b71376e77592 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 2 Oct 2026 07:41:45 +0300 Subject: [PATCH 11/17] feat(sftp): warn when the mount directory sits somewhere users wipe A mount makes the phone's storage reachable through the filesystem, so where it lives decides what can destroy it. The default is now safe, but a user who pinned mount_dir -- or upgraded with the old default still written in their config -- can still be in a folder that gets wiped. The check covers the freedesktop set of user-content folders rather than three of them. The hazard is never "~/Documents" specifically; it is anywhere a user runs `rm -rf` to reclaim space, and clearing old photos is at least as plausible as clearing downloads. A narrower list would be arbitrary. Folder names come from ~/.config/user-dirs.dirs, so a localized or relocated setup is still recognised rather than silently missed. Warns at most once per mount directory per daemon run, and is hoisted above the idempotency check so it also reaches a user reusing an existing mount -- otherwise the one person most likely to have a stale hazardous location is never told about it. Warning only; refusing would break working setups. Named bulkDeletableDir rather than documentDir: "document dir" read as the literal ~/Documents folder. --- internal/plugins/sftp/mount.go | 7 +- internal/plugins/sftp/mountdirs.go | 149 +++++++++++++++++++ internal/plugins/sftp/mountdirs_test.go | 181 ++++++++++++++++++++++++ internal/plugins/sftp/types.go | 5 +- 4 files changed, 340 insertions(+), 2 deletions(-) create mode 100644 internal/plugins/sftp/mountdirs.go create mode 100644 internal/plugins/sftp/mountdirs_test.go diff --git a/internal/plugins/sftp/mount.go b/internal/plugins/sftp/mount.go index 521b099..a00f2b0 100644 --- a/internal/plugins/sftp/mount.go +++ b/internal/plugins/sftp/mount.go @@ -108,6 +108,12 @@ func sshfsHint(msg string) string { // 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) { + 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 @@ -122,7 +128,6 @@ func (p *SftpPlugin) mountWithBody(ctx context.Context, deviceID string, body Sf return existing, nil } - mountPoint := p.mountPointFor(deviceID) if err := os.MkdirAll(mountPoint, 0700); err != nil { return "", fmt.Errorf("create mount point %s: %w", mountPoint, err) } 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..b6e34d6 --- /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{}, ""); 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/types.go b/internal/plugins/sftp/types.go index e96fe39..039534a 100644 --- a/internal/plugins/sftp/types.go +++ b/internal/plugins/sftp/types.go @@ -20,7 +20,10 @@ 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 + // 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 { From 979bc90e0d3f45477abb639349f067ad1547ae9a Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 2 Oct 2026 07:45:43 +0300 Subject: [PATCH 12/17] fix(sftp): release mounts on daemon shutdown OnDisconnect is only called when a connection drops, never on shutdown -- daemon.Run just logged and returned. So every graceful stop left every SFTP mount live, holding an sshfs process. That was already untidy, and with the mount directory moved it becomes a data loss path: in the no-session fallback the mount lives under $XDG_STATE_HOME/kcd, and uninstall.sh --purge does `rm -rf` on exactly that directory. rm descends into a live FUSE mount, so a routine purge would delete the phone's files. daemon.Run now calls UnmountAll after the shutdown log. It walks the plugin's own mountPoints rather than the device registry -- that is the plugin's record of what it believes is mounted, including mounts adopted from the kernel earlier in the session. Unmounts run concurrently. One can take up to 13s on its own (3s waiting for sshfs to exit, then a 10s fusermount bound), so a serial loop over several devices would overrun TimeoutStopSec=10 and get SIGKILLed part-way through, stranding one. The whole set is bounded by shutdownUnmountBudget (5s) instead, and whatever has not finished is logged -- the caller cannot do better, because systemd kills the process next. UnmountAll_VisitsEveryTrackedDevice covers iteration, concurrency and the per-device transition. Actually detaching a live mount needs a real FUSE mount, which a unit test cannot create, so the live-release path is asserted through Unmount instead; the test says so rather than pretending otherwise. --- internal/daemon/daemon.go | 17 ++++++++ internal/plugins/sftp/mount.go | 62 ++++++++++++++++++++++++++++ internal/plugins/sftp/mount_test.go | 63 +++++++++++++++++++++++++++++ 3 files changed, 142 insertions(+) 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/plugins/sftp/mount.go b/internal/plugins/sftp/mount.go index a00f2b0..832eeb5 100644 --- a/internal/plugins/sftp/mount.go +++ b/internal/plugins/sftp/mount.go @@ -10,6 +10,7 @@ import ( "regexp" "strconv" "strings" + "sync" "syscall" "time" @@ -451,3 +452,64 @@ 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 the whole thing +// instead, and anything still running when it expires is logged -- the caller +// cannot do better, because systemd will SIGKILL next. +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 index 5328361..9aeb279 100644 --- a/internal/plugins/sftp/mount_test.go +++ b/internal/plugins/sftp/mount_test.go @@ -252,3 +252,66 @@ func TestUnmount_LiveMountTakesTheReleasePath(t *testing.T) { 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") + } +} From 421f867918d9cf7e9f27160566af3826d9083af2 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 2 Oct 2026 07:57:23 +0300 Subject: [PATCH 13/17] fix(scripts): do not delete through a live SFTP mount uninstall.sh --purge removes the state directory recursively, and the "to remove later" tip printed a bare `rm -rf` of it. rm descends into nested filesystems by default, so if an SFTP mount were still live the phone's files would be deleted through it. With the mount directory now falling back to the state directory when there is no user session, that directory can hold a live mount. Uses --one-file-system, which makes rm stop at the filesystem boundary, with a probe rather than an assumption: the flag arrived in coreutils 8.30 and BusyBox spells it -x, and an unrecognized option under `set -euo pipefail` would abort mid-uninstall and leave a half-installed system. If neither spelling exists, removal is refused while a mount is live rather than attempting it blind. The mountpoint test compares st_dev against the parent, so it needs neither findmnt nor mountpoint -- nothing else in these scripts depends on either. It checks the current location and the pre-v1.22 one, since a mount can outlive the daemon by way of an unclean shutdown. The tip now prints the form this system actually supports. Also pins hk to the version hk.pkl amends. At "latest", a mise upgrade could pull hk past its own package:// import and the steps would stop resolving. Drops the shell profile from hk.pkl. Nothing runs it -- CI has no shell tooling and no shell tool is pinned in mise.toml -- so `hk check --profile shell` failed on missing tools and reported nothing about the code while looking like it was configured. go_vuln_check stays on the slow profile. --- hk.pkl | 16 ++++---- mise.toml | 5 ++- scripts/uninstall.sh | 90 ++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 97 insertions(+), 14 deletions(-) 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/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/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 From 11cd5262d73bd05085d6da2dfbbc10b591e048e8 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 2 Oct 2026 08:04:46 +0300 Subject: [PATCH 14/17] feat(sftp): support read-only mounts A mount exposes the phone's storage through the filesystem, and the kernel happily forwards unlink/rmdir/write to it. Mounting read-only removes that class of accident entirely: the VFS rejects those calls locally, so `rm` reports "Read-only file system" instead of deleting from the phone. Default stays read-write, so nothing existing changes. `read_only = true` in [sftp] sets the default and `kcd sftp mount|browse --ro` overrides it for one request, with --no-ro to force writable when the default is on. The IPC field is a *bool, not a bool. The daemon has to tell "the client asked for read-write" from "the client said nothing", because the second means "use the [sftp] default" -- and a plain bool with omitempty cannot express it, so an older client would quietly flip a read-only default back to writable. Emitted before ExtraSshfsOpts, since that is operator config and sshfs honours the last -o ro/rw; otherwise a configured override would be silently ignored. Mounting stays idempotent, and a mode cannot be changed on an existing mount, so the mode each mount was created with is recorded. A --ro request against an already-writable mount now logs what the mount actually is rather than appearing to honour the flag. --- cmd/kcd/cli_sftp.go | 38 +++++++++++- internal/config/plugins.go | 3 + internal/daemon/ipc_routes_sftp.go | 19 ++++-- internal/ipc/payload.go | 1 + internal/ipc/proto.go | 12 ++++ internal/plugins/sftp/mount.go | 36 ++++++++++-- internal/plugins/sftp/mount_test.go | 4 +- internal/plugins/sftp/mountdirs_test.go | 2 +- internal/plugins/sftp/request.go | 18 ++++-- internal/plugins/sftp/sftp_test.go | 78 ++++++++++++++++++++++++- internal/plugins/sftp/types.go | 16 +++-- pkg/client/client_sftp.go | 15 +++-- 12 files changed, 208 insertions(+), 34 deletions(-) diff --git a/cmd/kcd/cli_sftp.go b/cmd/kcd/cli_sftp.go index 3d46a10..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.") @@ -136,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") @@ -145,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 } @@ -206,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/internal/config/plugins.go b/internal/config/plugins.go index b97b665..7c97868 100644 --- a/internal/config/plugins.go +++ b/internal/config/plugins.go @@ -75,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 { diff --git a/internal/daemon/ipc_routes_sftp.go b/internal/daemon/ipc_routes_sftp.go index b915355..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 { @@ -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/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 e8a3d64..8faf382 100644 --- a/internal/ipc/proto.go +++ b/internal/ipc/proto.go @@ -200,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/plugins/sftp/mount.go b/internal/plugins/sftp/mount.go index 832eeb5..a70e22b 100644 --- a/internal/plugins/sftp/mount.go +++ b/internal/plugins/sftp/mount.go @@ -36,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) } @@ -74,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 { @@ -108,7 +114,7 @@ func sshfsHint(msg string) string { // 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) { +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 @@ -125,6 +131,16 @@ func (p *SftpPlugin) mountWithBody(ctx context.Context, deviceID string, body Sf 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 } @@ -148,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 @@ -171,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. @@ -259,6 +277,14 @@ 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. @@ -404,6 +430,7 @@ 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) @@ -420,6 +447,7 @@ func (p *SftpPlugin) removeLeftoverDir(deviceID, mountPoint string) bool { 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. diff --git a/internal/plugins/sftp/mount_test.go b/internal/plugins/sftp/mount_test.go index 9aeb279..2101c17 100644 --- a/internal/plugins/sftp/mount_test.go +++ b/internal/plugins/sftp/mount_test.go @@ -35,7 +35,7 @@ func TestMountWithBody_IsIdempotent(t *testing.T) { 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, "") + 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) } @@ -129,7 +129,7 @@ func TestMountStateEventsPublished(t *testing.T) { // 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{}, ""); err != nil { + if _, err := p.mountWithBody(context.Background(), "dev1", SftpBody{}, "", false); err != nil { t.Fatalf("idempotent mount: %v", err) } select { diff --git a/internal/plugins/sftp/mountdirs_test.go b/internal/plugins/sftp/mountdirs_test.go index b6e34d6..3c5bf9a 100644 --- a/internal/plugins/sftp/mountdirs_test.go +++ b/internal/plugins/sftp/mountdirs_test.go @@ -172,7 +172,7 @@ func TestWarnFiresOnReusedMount(t *testing.T) { p.mountPoints["dev1"] = hazardous // Already mounted, so mountWithBody takes the reuse path. - if _, err := p.mountWithBody(context.Background(), "dev1", SftpBody{}, ""); err != nil { + if _, err := p.mountWithBody(context.Background(), "dev1", SftpBody{}, "", false); err != nil { t.Fatalf("reuse path: %v", err) } if !p.warnedDirs[hazardous] { diff --git a/internal/plugins/sftp/request.go b/internal/plugins/sftp/request.go index 2c3da0f..2588225 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") } @@ -63,7 +63,7 @@ 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) @@ -75,7 +75,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") } @@ -118,7 +118,7 @@ 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 } @@ -132,14 +132,14 @@ func (p *SftpPlugin) RequestAndMountVolume(ctx context.Context, dev device.Sende // 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. @@ -261,3 +261,9 @@ func (p *SftpPlugin) adoptIfMounted(deviceID, mountPoint string) string { func (p *SftpPlugin) IsMounted(deviceID string) bool { return p.MountedPath(deviceID) != "" } + +// 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 039534a..c95902b 100644 --- a/internal/plugins/sftp/types.go +++ b/internal/plugins/sftp/types.go @@ -20,6 +20,9 @@ type SftpPlugin struct { mu sync.RWMutex lastBody map[string]SftpBody mountPoints map[string]string // deviceID -> local mountPoint path + // 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 @@ -28,12 +31,13 @@ type SftpPlugin struct { 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), } } diff --git a/pkg/client/client_sftp.go b/pkg/client/client_sftp.go index 0aa097b..0a4025a 100644 --- a/pkg/client/client_sftp.go +++ b/pkg/client/client_sftp.go @@ -7,8 +7,12 @@ import ( ) // 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 } @@ -44,8 +48,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.Call(ipc.CmdSftpMountLocal, ipc.SftpMountPayload{DeviceID: deviceID, ReadOnly: readOnly}) if err != nil { return "", err } @@ -68,10 +72,11 @@ 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) { +func (c *Client) SftpBrowse(deviceID string, volume string, readOnly *bool) (string, []ipc.StorageVolumeResponse, error) { resp, err := c.Call(ipc.CmdSftpBrowse, ipc.SftpBrowsePayload{ DeviceID: deviceID, Volume: volume, + ReadOnly: readOnly, }) if err != nil { return "", nil, err From 91c65cf9acfe4761d84b5cbd1dfcc33536f32dba Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 2 Oct 2026 08:07:34 +0300 Subject: [PATCH 15/17] docs: document the new mount location, read-only mounts, and shutdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records where mounts now live and why, so the default is not "magic": /run/user/1000/kcd/mnt is a tmpfs, matches 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. Replaces the illustrative /home/user/Downloads paths in CLI.md and IPC_PROTOCOL.md, which taught the location this change moves away from, and fixes an sftp_mount_local example still showing /tmp. Documents --ro/--no-ro, read_only, the fact that readOnly is a pointer on the wire so "unset" and "false" stay distinguishable, and the shutdown unmount in both AGENTS.md's startup order and ARCHITECTURE.md. Fixes the example config's claim that mount_dir "defaults to temp": it defaults to $XDG_RUNTIME_DIR, and /tmp was never it. --- AGENTS.md | 2 ++ README.md | 3 ++- docs/ARCHITECTURE.md | 2 +- docs/CLI.md | 13 +++++++++++-- docs/IPC_PROTOCOL.md | 17 ++++++++++++----- packaging/kcd.example.toml | 11 ++++++++++- 6 files changed, 38 insertions(+), 10 deletions(-) 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/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index b7e2939..1f7988b 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -292,7 +292,7 @@ goroutine leak when the child wedges. | `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. 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`. 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` falls back to parsing `/proc/mounts` for a `fuse.*` entry named `kcd-sftp-` and adopts what it finds, so a mount that outlived a daemon restart is still reported mounted, cleaned up on disconnect, and unmountable by device id | +| `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` | diff --git a/docs/CLI.md b/docs/CLI.md index 6ef17a1..9127cdd 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -727,7 +727,7 @@ Port: 8022 User: sftp-user Password: ******** Path: /storage/emulated/0 -Mounted: yes (/home/user/Downloads/kcd/mnt/kcd-sftp-a1b2c3d4) +Mounted: yes (/run/user/1000/kcd/mnt/kcd-sftp-a1b2c3d4) Storage volumes: Internal shared storage /storage/emulated/0 @@ -773,6 +773,15 @@ 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. @@ -829,7 +838,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: diff --git a/docs/IPC_PROTOCOL.md b/docs/IPC_PROTOCOL.md index 1f7a7bc..9527d28 100644 --- a/docs/IPC_PROTOCOL.md +++ b/docs/IPC_PROTOCOL.md @@ -591,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` @@ -613,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_..."}` @@ -628,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"} @@ -1192,7 +1199,7 @@ state. **Payload:** ```json -{"mountPoint": "/home/user/Downloads/kcd/mnt/kcd-sftp-a1b2c3d4", "volume": "/storage/ABCD-1234"} +{"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 @@ -1205,7 +1212,7 @@ The device's filesystem was released. **Payload:** ```json -{"mountPoint": "/home/user/Downloads/kcd/mnt/kcd-sftp-a1b2c3d4"} +{"mountPoint": "/run/user/1000/kcd/mnt/kcd-sftp-a1b2c3d4"} ``` Mount state is also available without waiting for an event: `state.snapshot` diff --git a/packaging/kcd.example.toml b/packaging/kcd.example.toml index ea8fa0e..9788da2 100644 --- a/packaging/kcd.example.toml +++ b/packaging/kcd.example.toml @@ -270,7 +270,16 @@ suspend = "systemctl suspend" # ─── SFTP: remote filesystem mounting ──────────────────────────────────────── [sftp] -# mount_dir = "" # parent directory for mounts, defaults to temp +# 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 phone credentials # keepalive_interval_secs = 15 # sshfs ServerAliveInterval # keepalive_count = 3 # sshfs ServerAliveCountMax From b010258e7fdeea6f4512a50231d904b9db42307f Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 2 Oct 2026 08:20:53 +0300 Subject: [PATCH 16/17] fix(cli): report a silent phone, not a dead socket, on SFTP mount `kcd sftp mount` against a phone that has not granted kcd file access failed with "read response: read unix @->.../kcd.sock: i/o timeout". The client deadline is 5s while the daemon waits 20s for the phone, so the socket deadline always won and the error blamed the connection -- which is not the problem. The client deadline for the calls that block on the phone is now derived from [sftp] credentials_timeout_secs, mirroring what PairListenTimeout already does for pair_listen. The daemon's message is the actionable one, so it is now the one that arrives. That message also names the actual cause: the phone not answering usually means the app is closed or file access is off, and "is the app open" alone sent people looking in the wrong place. Overflow fix found by the test: the deadline is built from a raw int of config-supplied seconds, and time.Duration(seconds)*time.Second wraps for anything past ~292 years, which would have produced a negative deadline and timed out instantly. The bound is now checked on the seconds before converting. pairListenDeadline does not have this because it takes an already-bounded Duration. Deduplicates the credentials-timeout setup the two request paths each had. --- cmd/kcd/cli_sftp_deadline_test.go | 38 +++++++++++++++++++++++++++++++ cmd/kcd/main.go | 23 +++++++++++++++++++ docs/CLI.md | 6 +++++ internal/plugins/sftp/request.go | 24 +++++++++++-------- packaging/kcd.example.toml | 6 ++++- pkg/client/client.go | 12 ++++++++++ pkg/client/client_sftp.go | 16 ++++++++++--- 7 files changed, 111 insertions(+), 14 deletions(-) create mode 100644 cmd/kcd/cli_sftp_deadline_test.go 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/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/CLI.md b/docs/CLI.md index 9127cdd..6f7f4d3 100644 --- a/docs/CLI.md +++ b/docs/CLI.md @@ -769,6 +769,12 @@ Request credentials and immediately mount the phone's filesystem using `sshfs`. kcd sftp mount ``` +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. diff --git a/internal/plugins/sftp/request.go b/internal/plugins/sftp/request.go index 2588225..f7904bf 100644 --- a/internal/plugins/sftp/request.go +++ b/internal/plugins/sftp/request.go @@ -41,10 +41,7 @@ func (p *SftpPlugin) RequestAndMount(ctx context.Context, dev device.Sender, rea 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() @@ -66,7 +63,7 @@ func (p *SftpPlugin) RequestAndMount(ctx context.Context, dev device.Sender, rea 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) } } } @@ -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() @@ -125,7 +119,7 @@ func (p *SftpPlugin) RequestAndMountVolume(ctx context.Context, dev device.Sende 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) } } } @@ -262,6 +256,16 @@ 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 { diff --git a/packaging/kcd.example.toml b/packaging/kcd.example.toml index 9788da2..d58fa98 100644 --- a/packaging/kcd.example.toml +++ b/packaging/kcd.example.toml @@ -280,7 +280,11 @@ suspend = "systemctl suspend" # 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 phone credentials +# 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 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 0a4025a..4161ebd 100644 --- a/pkg/client/client_sftp.go +++ b/pkg/client/client_sftp.go @@ -1,6 +1,8 @@ package client import ( + "time" + "encoding/json" "github.com/bethropolis/kcd/internal/ipc" @@ -49,7 +51,7 @@ func (c *Client) SftpVolumes(deviceID string) ([]ipc.StorageVolumeResponse, erro // 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, readOnly *bool) (string, error) { - resp, err := c.Call(ipc.CmdSftpMountLocal, ipc.SftpMountPayload{DeviceID: deviceID, ReadOnly: readOnly}) + resp, err := c.callWithTimeout(ipc.CmdSftpMountLocal, ipc.SftpMountPayload{DeviceID: deviceID, ReadOnly: readOnly}, c.sftpTimeout()) if err != nil { return "", err } @@ -73,11 +75,11 @@ func (c *Client) SftpUnmount(deviceID string) error { // 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, readOnly *bool) (string, []ipc.StorageVolumeResponse, error) { - resp, err := c.Call(ipc.CmdSftpBrowse, ipc.SftpBrowsePayload{ + resp, err := c.callWithTimeout(ipc.CmdSftpBrowse, ipc.SftpBrowsePayload{ DeviceID: deviceID, Volume: volume, ReadOnly: readOnly, - }) + }, c.sftpTimeout()) if err != nil { return "", nil, err } @@ -87,3 +89,11 @@ func (c *Client) SftpBrowse(deviceID string, volume string, readOnly *bool) (str } 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 +} From a0330dd6882a3c4df27dc444af0c9e7279f92d53 Mon Sep 17 00:00:00 2001 From: bethropolis <66518866+bethropolis@users.noreply.github.com> Date: Fri, 2 Oct 2026 08:42:04 +0300 Subject: [PATCH 17/17] fix(sftp): don't let an in-flight unmount outlive its caller TestUnmountAll_RespectsContextBudget cancelled the context immediately, so UnmountAll returned while the goroutine it had just spawned was still running and logging. That goroutine then called t.Log after the test had completed, which is a data race on testing's own state and a panic under CI's -race runner. The test now waits for the in-flight unmount to finish before returning. Also corrects the UnmountAll contract, which the race exposed. It said anything still running when ctx expired is logged; it cannot be, because the caller has already returned by then. In-flight unmounts do keep going, each bounded by its own timeouts, so an early return is not "unmounted" -- which is only acceptable because the sole caller is shutdown, where the process exits immediately and systemd would kill the stragglers anyway. Stated rather than implied, so the next caller knows. --- internal/plugins/sftp/mount.go | 11 ++++++++--- internal/plugins/sftp/mount_test.go | 17 +++++++++++++++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/internal/plugins/sftp/mount.go b/internal/plugins/sftp/mount.go index a70e22b..d689fa4 100644 --- a/internal/plugins/sftp/mount.go +++ b/internal/plugins/sftp/mount.go @@ -497,9 +497,14 @@ func findSSHFSPID(mountPoint string) (int, error) { // // 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 the whole thing -// instead, and anything still running when it expires is logged -- the caller -// cannot do better, because systemd will SIGKILL next. +// 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)) diff --git a/internal/plugins/sftp/mount_test.go b/internal/plugins/sftp/mount_test.go index 2101c17..83c6462 100644 --- a/internal/plugins/sftp/mount_test.go +++ b/internal/plugins/sftp/mount_test.go @@ -314,4 +314,21 @@ func TestUnmountAll_RespectsContextBudget(t *testing.T) { 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) + } }