Conversation
The daemon fast path (`nanocoder daemon start`) never touched the directory trust machinery: cli.tsx branched off before App.tsx, and nothing in source/daemon/ checked preferences.trustedDirectories. Once booted, headless mode skips tool confirmations for execute_bash, write_file, string_replace, diff_edit and MCP tools, so a daemon started in an untrusted repo armed every .nanocoder/ skill subscription for unattended execution (issue Nano-Collective#1245). Add a non-interactive trust gate with the same rules the plain shell already enforces, applied at both layers that can boot a daemon: - start(): refuse with a user-facing message unless the project root is in preferences.trustedDirectories, the caller passes --trust-directory, or NANOCODER_TRUST_DIRECTORY=1 is set. Both explicit mechanisms record standing trust, because the daemon boots detached and can boot again via autostart without the CLI. - entry.ts: the boot itself re-checks, so launchd/systemd autostart boots (set up by `daemon install`) that never run the CLI are covered too. Regression tests in cli.spec.ts cover refuse-when-untrusted (no spawn, no trust writes), trusted-via-preferences (no rewrite), and both explicit trust mechanisms (persisted, daemon spawned).
nc-review: needs work — 1 blocking, 4 important, 2 nits@yetuge — there is a blocking item below. The PR correctly ports the directory-trust gate from the plain shell into both the
🔴 blocking · PR #1259 ( 🟠 important · The four new tests exercise 🟠 important · The After this PR, 🟠 important · The boot-time gate added in 🟠 important ·
⚪ nit · This is a user-facing security change ( ⚪ nit · The new wiring is 🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with |
Second review pass on the daemon trust gate, addressing the nc-review findings: - Read the flag position-agnostically through a shared `hasTrustDirectoryFlag()` helper, so `nanocoder --trust-directory daemon start` no longer quietly degrades to "untrusted" while the general parser accepts the same spelling. - Move the boot rule into `checkDaemonBootTrust()` and have `entry.ts` call it. `entry.ts` runs `main()` on load, so a spec cannot import it; the rule is testable where it now lives. - Correct the `--trust-directory` help text, its example, and the matching row in docs/getting-started, which still claimed the flag was valid only with `run` and never touched the preferences file. - Document the deliberate persistence asymmetry against the plain shell on both sides of the two helpers. - Add the changeset the user-facing security change was missing. New spec file `source/daemon/trust.spec.ts` (9 tests) covers flag position, the persistence asymmetry, the boot gate refusing/permitting a directory, and that the boot never derives trust from the invoking argv - autostart boots carry no argv at all.
|
Addressed in b27380a — thanks, this round was useful.
|
Keeps the branch mergeable after Nano-Collective#1261 (feature/new-ui) landed on main: the PR was reported as conflicting with the base branch. The only conflict was the `--trust-directory` row of the CLI flag table in docs/getting-started/index.md - main rewrote the neighbouring `--alt-screen` rows while this branch rewrites the trust row. Resolved by keeping this branch's trust wording and main's alt-screen wording. source/cli.tsx auto-merged: the daemon fast path still reads the flag through hasTrustDirectoryFlag() and the help text keeps the corrected wording. Post-merge checks: tsc --noEmit clean; daemon trust + cli specs and plain/shell specs 43 passed.
|
Follow-up: merged |
will-lamerton
left a comment
There was a problem hiding this comment.
The fix itself is sound, and the response to the previous round was thorough. I fetched the branch, merged current main in (clean), and ran tsc --noEmit, biome check on the touched files, and the trust / daemon-cli / plain-shell / cli specs: all clean, 100 tests pass. The two-layer design checks out, runDaemonCli is the only production caller of launchDaemon, the plist and systemd unit set only NANOCODER_PROJECT_ROOT so autostart really does depend on persisted trust, and the boot gate sits before createLLMClient and startDaemon.
Requesting changes on the following.
Blocking, not yours to fix: #1259 is still open and #1245 is still assigned to @addyCooks, so a maintainer needs to pick a lane before either lands. Separately, the required pr-checks workflow has never run on this PR (only changeset-check and label did), so the green ticks above mean less than they look. The branch is also 128 commits behind main.
Changeset is factually wrong. .changeset/daemon-boot-trust-gate.md says NANOCODER_TRUST_DIRECTORY=1 "works for a single boot". It doesn't: the env-var path calls markTrusted too, so it persists standing trust exactly like the flag. The behaviour is right (it mirrors plain/shell.ts), only the text is wrong, and that text ships in the changelog.
The --trust-directory split deserves one more look. Same flag, opposite persistence semantics: ephemeral for run, a permanent global trust grant for daemon start. You documented it well, but documenting a footgun doesn't disarm it, and nanocoder daemon start --trust-directory in a throwaway or shared build directory permanently trusts that path. Worth considering: have daemon start --trust-directory propagate NANOCODER_TRUST_DIRECTORY=1 into the spawned child's env rather than persisting. The boot gate passes without a permanent write, and autostart, the one case that genuinely needs persistence, is already behind the explicit daemon install.
Docs still walk users into the refusal. docs/features/skills.md:173 and docs/features/scheduler.md:74 both tell people to run nanocoder daemon start with no mention of the new gate. The flag table in getting-started was updated, but those are the two pages where people actually meet the daemon. Related: daemon install stays ungated by design, so installing in an untrusted directory succeeds and then every autostart boot fails into daemon.log where nobody looks. A warning at install time closes that cheaply.
Silent savePreferences failure. savePreferences swallows write errors and returns void, and markTrusted returns true regardless. On a failed write the CLI prints "Marked X as trusted. Daemon started" while the child's own gate refuses and waitForLockfile times out. Rare, and it fails safe, but the message is untrue.
Test gap: please take your own offer here rather than deferring it. Nothing asserts that cli.tsx and entry.ts actually call the helpers, so a regression deleting either call site passes the suite. For a security gate that's the regression most worth catching.
Minor: the gate runs before readLiveLockfile, so daemon start in an untrusted directory that already has a daemon running reports "not trusted" instead of "already running".
|
Hi @yetuge, thanks for this PR! It looks like a maintainer has left feedback Whenever you get a chance, could you take a look at the open comments? |
Fixes #1245
Root cause
nanocoder daemon startis a fast path insource/cli.tsxthat branches off before the interactive app and goes straight torunDaemonCli("start")→launchDaemon→entry.ts. Nothing along that path — or anywhere undersource/daemon/— consultspreferences.trustedDirectories: the trust check only lives in the interactive TUI (useDirectoryTrust) and the plain shell (source/plain/shell.ts).Once booted, the daemon dispatches subagents in headless mode, which skips tool confirmations for
execute_bash,write_file,string_replace,diff_editand MCP tools, and OS-level sandboxing is opt-in (unsupported on Windows). So a daemon started in a directory the user never trusted arms every.nanocoder/skill subscription for unattended execution.Fix
Add a non-interactive trust gate with the same rules the plain shell already enforces (
ensureDirectoryTrustinsource/plain/shell.ts), applied at both layers that can boot a daemon:start()(shared helper in the newsource/daemon/trust.ts): refuses with a user-facing message unless the project root is already inpreferences.trustedDirectories, the caller passes--trust-directory, orNANOCODER_TRUST_DIRECTORY=1is set. Both explicit mechanisms record standing trust (like the plain shell's env-var path), because the daemon boots detached and can boot again via autostart without the CLI — ephemeral trust would not survive the spawn.entry.ts: the boot itself re-checks the same rule before standing up the pipeline, so autostart boots (launchd/systemd, set up bydaemon install) that never run the CLI are covered too. The CLI gate is still needed: it fails before spawning, with a message the user actually sees, whereas a boot-gate-only refusal would land indaemon.log.--trust-directoryis now honored fordaemon start(previously documented as applying only tonanocoder run); for a daemon it records standing trust rather than "this run only", since the process it arms outlives the invoking shell.daemon installitself is deliberately left ungated — it only writes the autostart registration, and the boot gate now covers the path it arms.Tests
New regression tests in
source/daemon/cli.spec.ts(run against unpatchedmainwith only the tests applied, then with the fix):start refuses to spawn when the project directory is not trusted— asserts exit 1, a/not trusted/message, no launcher invocation and no trust writes. Failed onmain(the daemon spawned regardless).start marks the directory trusted and proceeds when NANOCODER_TRUST_DIRECTORY=1— asserts standing trust is persisted. Failed onmain.start records standing trust when --trust-directory is passed— failed onmain(the flag was ignored).start proceeds when the directory is already trusted— guards the happy path; passes before and after.Red→green: on
main(537ee6c) the new tests report3 tests failed; with the fix,pnpm exec ava source/daemon/cli.spec.ts→12 tests passed. Also green:source/daemon/lockfile.spec.ts+source/daemon/install.spec.ts(30),source/plain/shell.spec.ts(22),source/cli.spec.ts(54).pnpm test:typesandbiome checkare clean on the touched files.Fixes #1245