fix(apps): confirm-gate out-of-install dev-mode grants (#6907) - #7169
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of The design holds up across all the gate questions: real named harm (same-origin app self-grant across the unauthenticated UI route), root-cause fix (attestation enforced at the consumption point with a kernel-sealed artifact, not evadable command-text scans), fail-closed failure modes, validate-before-write atomicity, docs updated in the same commit, and the refactor removes a tracked circular-import workaround. The residuals (sandbox-off hosts, sandboxed CLI toggles redirected to the dashboard toggle) are documented and have working alternate paths. Design-Verdict: PASS Attestation moved from evadable command-text scans to a kernel-sealed consumption point — the right root-cause shape, with residuals disclosed. Suggestions
[DESIGN-REVIEWED] aba292b |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsThe single candidate describes a brace-expansion gap in the deny rule's argv floor (tier 1 of three). I verified: No findings. [OPUS-REVIEWED] aba292b Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of First-Principles-Verdict: CONCERNS Every gate tier traces to a named evasion, but one tier re-runs a probe that already passed, and a fixture respelling rides along undeclared. What this change shipsIntent: stop an app's own page code from granting itself dev-mode serving of out-of-install directories — a FIX (deferred from #6854), plus a declared refactor and docs.
WatchThe structural half of Subtractions
[FIRST-PRINCIPLES-REVIEWED] aba292b |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- src/kiro_crew/apps/dev_mode.py:75 -- False positive or not applicable? A repository writer can comment: |
89cdba9 to
6b8603d
Compare
Verified legitimate: no builtin deny rule covered |
Verified legitimate: |
6b8603d to
d40645e
Compare
Verified legitimate: the |
Verified legitimate: same root cause as the Opus lane's finding — the |
d40645e to
d28aa15
Compare
|
Audit note — #7224 is being closed in favour of this PRYou are the surviving implementation; #7224 is being closed. What the two sharedBoth PRs are open (verified live: 7169 head d28aa15, 7224 head a9a0632, neither merged, both mergeable_state=dirty) and both implement the same two deferred items of open issue #6907 off the identical base blobs. Verified in the diffs: both add the same What #7224 had that this PR does notPlease pick these up (or say they are not wanted) so they do not disappear with that branch: Two items from #7224, both small and both style/robustness tier: (1) src/kiro_crew/pinned_fs.py — the module-scope platform guards for the promoted helper instead of #7169's verbatim function-local imports: top-level This PR still needs work:
|
d28aa15 to
f200a22
Compare
aa83ea1 to
feeec9e
Compare
Also in this push: rebased onto current main (base d28aa15 → 760d8f5; 6 conflict files resolved — main's spec_builder handlers refactor moved the |
|
Review-ready at head |
Three follow-ups deferred out of the app UI-route TOCTOU fix's review: - Hardening: the dev-mode toggle carries no app-vs-operator identity, and app UI bundles run as same-origin modules with the dashboard's own credentials — so a same-origin caller (an app's own page code included) could grant itself dev mode on a benign out-of-install ui root and have the unauthenticated UI route serve it. Enabling dev mode on a root that resolves outside the app's install directory now fails closed: the HTTP endpoint always answers 400 (code dev_mode_out_of_install_confirmation_required) and deliberately has no confirmation field, because a request-body flag from the dashboard origin is app-controllable data, not operator attestation. Confirmation is host-boundary only: kirocrew app dev <name> --confirm-out-of-install-root. The sensitive-root refusal stays unconditional and cannot be overridden; in-install grants are unaffected; a refusal leaves prior state untouched. The flag is operator-only against agent shells through three tiers (the third added per maintainer ruling on the review's same-span stall): (1) the builtin deny rule self-protection-dev-mode-out-of-root-confirm, matched as literal text AND via an argv-structural floor on the shell-de-escaped command, with allow_abbrev=False on the dev subparser; (2) a runtime human-vs-agent check at the flag's consumption point — sandbox.agent_confinement_evidence() (launcher marker + macOS kernel Seatbelt verdict) refuses the confirmation from any agent-confined process (code dev_mode_operator_attestation_required), closing runtime flag synthesis ($(printf ...)) that no command-text scan can see; (3) the grant record apps/.dev-grants.json is sealed read-only inside the agent OS sandbox (_CREW_READONLY_LEAVES, alongside the other keystone ceilings) so a sandboxed process cannot mint a grant no matter how the toggle is spelled — the gateway materializes the record at startup (the Linux bind-seal needs an existing target), a structural write-probe backs the runtime check, and any grant-touching toggle from a process that cannot write the record is refused up front (code dev_mode_grant_record_readonly) rather than half-applied. All refusals and the confirmed grant are SEL-audited (dev_mode_out_of_install_grant denied/granted, dev_mode_grant_write denied). - Refactor: promote hooks._fd_real_path to pinned_fs.fd_real_path (its canonical home with the other descriptor-pinned primitives), behavior byte-identical including the Windows fail-closed branch; repoint apps/routes.py, spec_builder, and sandbox.py (dropping sandbox's circular-import workaround), keep a load-bearing module-local alias in hooks for its own call sites and test seams, and pin the location with tests. - Docs: rewrite the App Dev Mode sections of docs/app-kit/api-reference.md and docs/system-specs/modules/cli.md against merged behavior: the operator grant record (.dev-grants.json bound to the resolved ui root), the point-in-time sensitivity screen, the 400-until-re-toggle contract for repointed or pre-upgrade escaped roots, the directory-not-file symlink rule, the CLI-only confirmation contract, and the three-tier operator-attestation model. Closes #6907
feeec9e to
aba292b
Compare
|
|
Review-ready again at head |
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
bolichen97
left a comment
There was a problem hiding this comment.
Closes the app self-grant path correctly: the HTTP toggle never forwards confirm_out_of_install_root, so an out-of-install ui root always 400s with dev_mode_out_of_install_confirmation_required, and the CLI-only confirmation is backed by a defense-in-depth stack that fails closed in the refusal direction — the literal-text deny rule plus argv floor, allow_abbrev=False so --confirm can't slip past the literal match, agent_confinement_evidence(), and decisively the kernel-sealed apps/.dev-grants.json write probe. The up-front _grant_record_unwritable() check keeps the refusal atomic (no installed.json/sentinel write left claiming a grant the record never made), the startup materialization is genuinely required because the Linux bind-seal only applies to an existing target, and the empty file the probe's O_CREAT can leave behind reads back as {} through _read_dev_grants's JSONDecodeError path — fail-closed, not fail-open. The _fd_real_path -> pinned_fs.fd_real_path promotion is byte-identical including the Windows fail-closed branch, and drops sandbox.py's circular-import workaround legitimately since pinned_fs is stdlib-only.
Problem / Motivation
Three follow-ups deferred out of PR #6854 (the #6809 app UI-route TOCTOU fix) on its Design Review advisory:
POST /api/apps/{name}/devagainst its own name, grant itself dev mode on auilink repointed at any non-sensitive external directory, and have the unauthenticated/apps/{name}/ui/*route serve files out of it._fd_real_pathoutgrew its home. The descriptor-to-real-path primitive lived private inhooks.pywith three cross-module importers (apps/routes.py, spec_builder, and a lazy borrow insandbox.pybehind a circular-import workaround comment).docs/app-kit/api-reference.mdstill presentedinstalled.jsondevas the only source of truth and recommended the symlinked-ui/setup unconditionally — predating the operator grant record, the sensitivity screen, and theO_NOFOLLOWper-file-symlink behavior that fix: serve app UI files from a pinned descriptor, not a re-opened path #6854 shipped.Why it matters
The self-grant path lets a malicious or compromised app widen the unauthenticated UI route onto directories the operator never approved (the sensitivity screen and root binding narrow the damage, but "any non-sensitive readable directory" is still a real exfiltration surface). The stale docs actively recommend a setup whose failure modes (
400on escaped roots,404on per-file links) they do not explain, and the duplicated-primitive drift risk grows with each new_fd_real_pathimporter.What changed (motivation → approach → change)
Hardening — out-of-install grants fail closed over HTTP; confirmation is CLI-only. The root cause is that no HTTP-reachable signal can distinguish the operator from same-origin app code: the dashboard session is shared, so a request-body confirmation field would be app-controllable data, not attestation.
set_dev_modetherefore gains a keyword-onlyconfirm_out_of_install_rootthat only the CLI passes (kirocrew app dev <name> --confirm-out-of-install-root— the host process boundary is what proves the operator). The HTTP toggle deliberately has no confirmation field and never forwards the kwarg: enabling dev mode on a ui root resolving outside the install directory always answers400withcode: "dev_mode_out_of_install_confirmation_required"and an error naming the CLI command. The sensitive-root refusal stays unconditional (confirmation cannot override it), in-install grants are unaffected, and a refusal is validate-before-write (prior dev state untouched). The CLI also warns when the flag is combined with--off(it grants nothing there). The load-bearing serving guarantees remain the resolved-root equality binding and the toggle-time sensitivity screen — this gate closes the self-grant path to them.Hardening (round 2, from review) — the flag is agent-denied and the decision is SEL-audited. Review pointed out two gaps in the CLI boundary: an agent with an auto-approved shell could pass
--confirm-out-of-install-rootitself (the host-process boundary alone does not exclude agent shells), and the permission decision left no audit trail. Both are closed: a new builtin deny rule (self-protection-dev-mode-out-of-root-confirm) refuses any agent command carrying the flag, enforced in two tiers: the catalog regex matches the flag's literal text (direct form, nested shell payloads, quoted interpreter argv), and a paired argv floor (_is_dev_mode_out_of_root_confirm, round 4 from review) re-checks the shell-de-escaped text and every tokenized argv frame, so quote-splitting inside the token (--confirm-out-of-install-'root') is denied the same as the plain spelling — making the confirmation supplied only from the operator's own terminal, which the agent deny rules do not govern. The floor is pinned bytest_dev_mode_confirm_flag_denied_under_quote_splitting. And both outcomes of the decision now emit a SEL event (dev_mode_out_of_install_grant, outcomedenied/granted, naming the resolved root); the granted event is written only after the grant record lands, so the log asserts an authority change that actually happened. Thedevsubparser is additionally built withallow_abbrev=False: argparse would otherwise accept--confirmas an unambiguous abbreviation, a spelling the literal-text deny rule never sees — abbreviations are now rejected at parse time, pinned byTestDevConfirmFlagNoAbbreviation.Hardening (round 5, per maintainer ruling) — runtime human-vs-agent enforcement at the flag's consumption point. Review round 5 showed the text tiers' structural limit: a command can synthesize the flag at runtime (
$(printf ...)), so no command-text scan — regex or argv floor — can ever see it. After a same-span stall escalation, the maintainer ruled: check human-vs-agent at run time. Two runtime tiers now decide atset_dev_mode, each unforgeable in the refusal direction: (1)sandbox.agent_confinement_evidence()(the launcher-set sandbox marker plus, on macOS, the kernel's ownsandbox_checkSeatbelt verdict) refuses the confirmation from any agent-confined process (dev_mode_operator_attestation_required, SEL-audited); and (2) the authorization record itself (apps/.dev-grants.json) is sealed read-only inside the agent OS sandbox via_CREW_READONLY_LEAVES— the same keystone-ceiling mechanism protectingdenied_commands.json— so a sandboxed process cannot mint a grant even by writing the file directly; the gateway materializes the record at startup (the Linux bind-seal needs an existing target), a structural write-probe backs the runtime check underenv -uscrubbing, and any grant-touching toggle from a process that cannot write the record is refused atomically up front (dev_mode_grant_record_readonly) rather than half-applied. The r3/r4 text tiers stay as defense in depth. Documented residual:agent.sandbox='off'hosts have no structural boundary for any control (existing logged opt-out, out of scope).Refactor.
hooks._fd_real_pathis promoted topinned_fs.fd_real_path— public, exported in__all__, behavior byte-identical including the Windows fail-closed branch. Importers repointed:apps/routes.pyand spec_builder import the public name;sandbox.pyimports it top-level (its circular-import workaround is gone:
pinned_fs is stdlib-only, so the import is cycle-free). hooks.py keeps a load-bearing module-local alias for its own call sites and their test monkeypatch seams. A location-pinning test asserts the definition site and that every consumer binds the pinned_fs object.
Docs. The App Dev Mode sections of docs/app-kit/api-reference.md and docs/system-specs/modules/cli.md are rewritten against merged behavior, each claim verified against code on current main: the operator grant record (.dev-grants.json at the apps root, written only by the toggle, bound to the resolved ui root), the point-in-time sensitivity screen, the 400-until-re-toggle contract for repointed or pre-upgrade escaped roots (re-toggle after re-pointing to re-bind), the directory-not-file symlink rule under the O_NOFOLLOW open, and the CLI-only confirmation contract.
Tests
test_denied_commands_security.py: the flag is denied in direct, nested-shell, and interpreter-argv forms; ordinary dev-mode toggles stay allowed; golden fixture + catalog counts updated (141 rules); the new rule registers an interposed-flag template.test_app_dev_mode.py::test_out_of_install_confirmation_decisions_are_SEL_audited: the unconfirmed refusal emits a SEL denial, a confirmed enable emitsgrantedonly after the grant record is written, and an in-install enable emits nothing.Manual verification
N/A beyond unit coverage: the toggle, route, and CLI paths are all exercised end-to-end by the aiohttp TestClient and argparse-level tests above; no UI surface changed.
Related Issues
Closes #6907
Pattern harvest
Rule candidate: An "operator-only" input can never be enforced by scanning what the command says — regex deny rules and argv-structural floors are all evadable by runtime construction (
$(printf ...)) because the guarded text only exists at execution time. Enforce operator-only-ness at the input's consumption point with signals about what the process is: evidence of agent-shell confinement (sandbox marker + kernel verdict), and — decisive — a kernel-sealed authorization artifact (_CREW_READONLY_LEAVES) the confined process cannot write regardless of spelling or environment. Text tiers remain useful as early, legible refusals (defense in depth), never as the load-bearing boundary. Corollary for the seal mechanism: the Linux bind-seal only applies to an existing target, so the owning process must materialize the file at startup or the seal silently doesn't exist on fresh installs.