v1.22.0: SFTP correctness, safe mount location, and mount-state visibility - #48
Merged
Merged
Conversation
`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.
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.
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.
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.
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-<deviceID> 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.
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.
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.
…xample 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.
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.
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.
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.
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.
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.
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.
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.
`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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Targeting v1.22.0. Two themes: a set of SFTP correctness and safety fixes
that a downstream plugin author reported, and the SFTP mount lifecycle made safe
by default.
Correctness
kcd sftp infoprinted the live SFTP password in bothhuman and
--jsonoutput, whiledocs/CLI.mdalready documentedPassword: ********. Masking is daemon-side, so the secret no longer crossesthe IPC socket unless asked for. The
sftp.mountevent still carries it --that is the path clients mount through.
sshfsonto alive mount point and failed with a message that reads like a FUSE permission
problem. The check lives in
mountWithBody, so both entry points get it.not mounted/stale SFTP mount/clean), so a client no longer has to string-match
Transport endpoint is not connected.Unmountused to drop its tracked statebefore attempting
fusermount, so a failed unmount left the device stuck as"mounted" forever. State is cleared only once the mount is released -- or, if
the kernel no longer has the mount at all, reported as already unmounted and
forgotten. Observed on a real device and reproduced in a test whose failure
message matches the report exactly.
truth and the map is a cache, so an orphaned mount is adopted, cleaned up on
disconnect, and unmountable by device id.
OnDisconnectonly fires on a droppedconnection, so every graceful stop used to leave mounts live.
kcd sftp mounton a silent phone. The CLI's 5s socket deadline beat thedaemon's 20s wait, so the user was told the connection timed out rather than
that the phone never answered. The deadline is now derived from
credentials_timeout_secs, mirroringPairListenTimeout.device.addedcarries a fullDeviceInfo, not just the name, so clientsstop inventing a state and a connected flag.
runcommand.output, gated onHasSubscribersso a chatty command does not publish to nobody.Mount safety
mount_dirdefaulted to~/Downloads/kcd/mnt-- insidedownload_dir, thecache for files this tool receives. The phone's storage was therefore mounted
inside the directory a user is most likely to wipe, reachable three ways:
rm -rf ~/Downloads/kcd(rmcrosses filesystems unless given--one-file-system, which is opt-in and has no--no-variant),rm -rf ~/Downloads, and the file manager's empty-folder gesture, sinceAutoOpenputsit under Downloads. Backup tools archive
$HOME, so Borg/Restic would alsotraverse into it.
The default is now
$XDG_RUNTIME_DIR/kcd/mnt: a tmpfs, what GNOME uses for itsown remote file access, and not walked by backup tools or home cleaners. A
hazard warning covers a user who overrode it, across the six freedesktop
user-content folders resolved via
user-dirs.dirs.--romounts read-only sothe kernel rejects deletion before it reaches the phone.
uninstall.shusesrm --one-file-systemwith a capability probe, refusing when a mount is live andthe flag is unavailable.
Not a breaking change: kcd never writes
kcd.toml, so a pinnedmount_diris honoured unchanged. The documented migration is to delete the line. Mounts at
the previous location stay findable via a legacy fallback.
Cleanup
Comment volume tightened in the dial and transport paths (every invariant and
security rationale preserved). Corrected a wrong "reload requires a restart"
claim, documented the SIGHUP hot-reload that already existed, added the missing
ExecReloadto the user unit, renumbered two colliding sections inCLIENT_GUIDE, and replaced asftp infoexample that showed output the codenever emitted.
Verification
hk check --all(local only -- CI runs no hk), the exact CI test invocation(
go test -p 1 -race -count=1 ./...), integration tests,goreleaser check,static build, and the example config parses.
uninstall.shwas checked withshellcheck; note the shell profile is no longer declared in
hk.pklbecause noshell tool is pinned anywhere, so it reported missing tools rather than anything
about the code.