Skip to content

fix(apps): confirm-gate out-of-install dev-mode grants (#6907) - #7169

Merged
bolichen97 merged 1 commit into
mainfrom
fix/dev-mode-grant-followups-6907
Sep 4, 2026
Merged

fix(apps): confirm-gate out-of-install dev-mode grants (#6907)#7169
bolichen97 merged 1 commit into
mainfrom
fix/dev-mode-grant-followups-6907

Conversation

@NicholasRBowers

@NicholasRBowers NicholasRBowers commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

Three follow-ups deferred out of PR #6854 (the #6809 app UI-route TOCTOU fix) on its Design Review advisory:

  1. A residual self-grant surface. The dev-mode toggle carries no app-vs-operator identity, and app UI bundles execute as same-origin ES modules with the dashboard's own credentials — so an app's page code could POST /api/apps/{name}/dev against its own name, grant itself dev mode on a ui link repointed at any non-sensitive external directory, and have the unauthenticated /apps/{name}/ui/* route serve files out of it.
  2. _fd_real_path outgrew its home. The descriptor-to-real-path primitive lived private in hooks.py with three cross-module importers (apps/routes.py, spec_builder, and a lazy borrow in sandbox.py behind a circular-import workaround comment).
  3. Stale App Kit docs. docs/app-kit/api-reference.md still presented installed.json dev as the only source of truth and recommended the symlinked-ui/ setup unconditionally — predating the operator grant record, the sensitivity screen, and the O_NOFOLLOW per-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 (400 on escaped roots, 404 on per-file links) they do not explain, and the duplicated-primitive drift risk grows with each new _fd_real_path importer.

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_mode therefore gains a keyword-only confirm_out_of_install_root that 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 answers 400 with code: "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-root itself (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 by test_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, outcome denied/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. The dev subparser is additionally built with allow_abbrev=False: argparse would otherwise accept --confirm as an unambiguous abbreviation, a spelling the literal-text deny rule never sees — abbreviations are now rejected at parse time, pinned by TestDevConfirmFlagNoAbbreviation.

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 at set_dev_mode, each unforgeable in the refusal direction: (1) sandbox.agent_confinement_evidence() (the launcher-set sandbox marker plus, on macOS, the kernel's own sandbox_check Seatbelt 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 protecting denied_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 under env -u scrubbing, 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_path is promoted to pinned_fs.fd_real_path — public, exported in __all__, behavior byte-identical including the Windows fail-closed branch. Importers repointed: apps/routes.py and spec_builder import the public name; sandbox.py imports it top-level (its circ
ular-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_enable_out_of_install_root_requires_confirmation: red-before proof (an unconfirmed out-of-install enable used to succeed); pins the refusal code, the no-state-left semantics, and the confirmed-grant binding to the escaped root.
  • test_dev_endpoint_never_confirms_out_of_install_roots: pins the security property itself: a POST supplying confirmOutOfInstallRoot=true still answers 400 and mints no grant.
  • test_confirmation_cannot_override_the_sensitivity_screen: gate ordering (sensitive refusal wins even with confirmation).
  • test_an_unconfirmed_refusal_preserves_prior_dev_state: validate-before-write holds for the new refusal.
  • CLI: flag forwarding, refusal-to-stderr, and the --off + flag warning.
  • TestFdRealPathHome (test_pinned_staging.py): definition-site pin, consumer identity pin, descriptor-resolution smoke, dead-descriptor fail-closed.
  • Existing descriptor-validation suites (test_hooks_coverage, test_app_ui_file_route, test_sandbox_argv) pass with monkeypatch targets updated to the new bindings.
  • 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 emits granted only 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.

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of aba292ba38f2562edf4d351f203a06520dce4b25 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • The documented residual names only agent.sandbox='off', but native Windows has no fs-sealing sandbox either, so the decisive tier (sealed .dev-grants.json) and the Seatbelt verdict are both absent there — name Windows explicitly alongside the opt-out in the residual disclosure.

[DESIGN-REVIEWED] aba292b

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed aba292ba38f2562edf4d351f203a06520dce4b25 — this comment is updated in place on each push.

Review details

The single candidate describes a brace-expansion gap in the deny rule's argv floor (tier 1 of three). I verified: _SELF_FLOOR_QUOTE_JUNK_RE strips only "'\ and _self_token_frames does no brace expansion, so --confirm-out-of-install-roo{t,t} does slip past both the regex tier and the floor. But the observable wrong outcome — an actual self-granted serving grant — does not occur: _operator_attestation_refusal() runs at flag-consumption time inside set_dev_mode and refuses any agent process (sandbox marker, Seatbelt verdict, or sealed-record write-probe) before a grant is minted, and that compensating tier lives in this same diff. (c) collapses to "no grant minted," so the candidate is a defense-in-depth gap in one redundant tier, not a reachable defect at 80+. The fd_real_path move is clean — pinned_fs imports nothing from kiro_crew, so no circular import — and empty/absent grant records fail closed to {}.

No findings.

[OPUS-REVIEWED] aba292b

Verdict parsed from the review's SHA-scoped output markers for commit aba292ba38f2562edf4d351f203a06520dce4b25.

False positive or not applicable? A repository writer can comment:
/ai-review override fable aba292ba38f2562edf4d351f203a06520dce4b25: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of aba292ba38f2562edf4d351f203a06520dce4b25 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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 ships

Intent: 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.

  1. Out-of-install dev-mode enable over HTTP now always answers 400 naming the CLI fix — justified (no HTTP signal distinguishes operator from same-origin app code).
  2. New CLI flag --confirm-out-of-install-root — justified (host process boundary is the attestation).
  3. CLI warns when the flag rides with --off — justified, declared.
  4. Agent shells carrying the flag are denied (new rule + argv floor) — justified (agent-untrusted boundary).
  5. dev subparser rejects abbreviations — justified (argparse would accept --confirm, invisible to the literal rule).
  6. Agent-confined processes refused at runtime (dev_mode_operator_attestation_required) — justified, maintainer-ruled.
  7. Grant record sealed via existing _CREW_READONLY_LEAVES keystone, materialized at startup, unwritable-record toggles refused atomically — justified (OS rule: Linux bind-seal needs an existing target).
  8. Both grant outcomes SEL-audited — justified (kept audit control).
  9. _fd_real_path promoted to pinned_fs.fd_real_path — justified move (4 consumers counted: hooks, sandbox, apps/routes, spec_builder; deletes the tracked circular-import borrow).
  10. Three golden-fixture descriptions respelled \u2014 — undeclared, rides along, JSON-identical.

Watch

The structural half of _operator_attestation_refusal (dev_mode.py:243) is unreachable: its one caller (dev_mode.py:526) runs only under enabled, and the upfront probe (dev_mode.py:407-408) already ran the identical _grant_record_unwritable() and passed on every path there — test_confirm_flag_refused_when_grant_record_sealed accepting either code confirms the shadowing.

Subtractions

  • Shrink _operator_attestation_refusal to agent_confinement_evidence() alone (or call it directly at dev_mode.py:526) — the sealed-record tier is already delivered by the upfront dev_mode_grant_record_readonly probe.
  • Revert the three \u2014 re-escapes in test/fixtures/denied_commands_golden.json — byte churn with no parsed difference, in a fixture whose other entries keep literal em-dashes.

[FIRST-PRINCIPLES-REVIEWED] aba292b

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of aba292ba38f2562edf4d351f203a06520dce4b25 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/apps/dev_mode.py:75 -- "never created by the startup reconcile" contradicts the startup _write_dev_grants(live) call and repeated docs -> Fix: clarify that reconcile creates only an empty record and never adds grants. (origin: validation)
[GPT-REVIEWED] aba292b

False positive or not applicable? A repository writer can comment:
/ai-review override gpt aba292ba38f2562edf4d351f203a06520dce4b25: <one-sentence reason>

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 31, 2026
@NicholasRBowers
NicholasRBowers force-pushed the fix/dev-mode-grant-followups-6907 branch from 89cdba9 to 6b8603d Compare August 31, 2026 02:56
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 31, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • CLI confirmation is reachable from the untrusted agent — span=e0fecac8beaa — fixed in 6b8603d.

Auto-approved Bash -> unrestricted CLI flag -> grant minted -> unauthenticated UI route exposes an unapproved directory.

Verified legitimate: no builtin deny rule covered kirocrew app dev, so an agent shell could pass --confirm-out-of-install-root itself. Fixed by adding the builtin deny rule self-protection-dev-mode-out-of-root-confirm (plain-substring match on the flag's literal text — argparse only accepts the flag when that text appears, so the match also covers nested shell payloads and quoted interpreter argv). The confirmation can now only come from the operator's own terminal, which the agent deny rules do not govern. Pinned by TestDevModeConfirmFlagIsAgentInaccessible (direct, nested-shell, and interpreter-argv forms denied; ordinary dev toggles stay allowed), with golden fixture + catalog counts and the interposed-flag template walk updated.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • Confirmation decisions bypass SEL — span=e0fecac8beaa — fixed in 6b8603d.

CLI confirmation -> grant or refusal -> no SEL event records the permission decision.

Verified legitimate: set_dev_mode emitted no SEL events. Both outcomes of the out-of-install permission decision now emit sel().log_api_access(operation="dev_mode_out_of_install_grant", ...) naming the resolved root: the unconfirmed refusal emits outcome="denied", and a confirmed enable emits outcome="granted" only AFTER the grant record is written, so the event asserts an authority change that actually happened. Pinned by test_out_of_install_confirmation_decisions_are_SEL_audited (denial on refusal, granted-after-write on confirm, and no event for in-install enables).

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 31, 2026
@NicholasRBowers
NicholasRBowers force-pushed the fix/dev-mode-grant-followups-6907 branch from 6b8603d to d40645e Compare August 31, 2026 03:09
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • argparse abbreviation bypasses the confirmation deny rule — span=c9886dc1579f — fixed in d40645e.

--confirm, --c, or --confirm-out-of-install-roo all uniquely abbreviate to the flag ... the text lacks the literal substring so is_denied() returns None ... The agent self-supplies the attestation.

Verified legitimate: the dev subparser used argparse's default allow_abbrev=True, so an abbreviated spelling reached confirm_out_of_install_root=True without the literal flag text the deny rule matches. Fixed exactly as prescribed: app_sub.add_parser("dev", ..., allow_abbrev=False) in cli.py, so argparse rejects every abbreviation (exit 2, unrecognized arguments) and only the literal flag — which the deny rule always sees — parses. Pinned by TestDevConfirmFlagNoAbbreviation: --confirm, --confirm-out-of-install-roo, and --c all exit 2 through cli.main() without reaching the handler, and the literal flag still parses to confirm_out_of_install_root=True.

@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Aug 31, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • argparse abbreviations bypass the confirmation deny — span=3c5b15a2d41e — fixed in d40645e.

Auto-approved Bash with --confirm -> deny misses abbreviated flag -> CLI grants external UI root -> unauthenticated route exposes it.

Verified legitimate: same root cause as the Opus lane's finding — the dev subparser accepted unambiguous abbreviations, so the attestation could be spelled without the literal text the deny rule requires. Fixed with allow_abbrev=False on the dev subparser (the prescribed fix): abbreviations are rejected at parse time, so the only spelling that grants is the literal flag, which the substring rule always matches. Pinned by TestDevConfirmFlagNoAbbreviation (three abbreviated spellings exit 2 without reaching _handle_app; the literal flag still parses).

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 31, 2026
@NicholasRBowers
NicholasRBowers force-pushed the fix/dev-mode-grant-followups-6907 branch from d40645e to d28aa15 Compare August 31, 2026 04:30
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 31, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • Shell quoting bypasses operator-only confirmation — span=3c5b15a2d41e — disposition: fixed in d28aa15.

Legitimate: the catalog rule matched the flag as raw text, so quote-splitting inside the token (--confirm-out-of-install-'root') reached argparse as the accepted flag while the raw command never carried the literal.
Fix: enforcement now evaluates the executed argv, as the finding demands — a new self-protection argv floor (_is_dev_mode_out_of_root_confirm), wired into is_denied exactly like the existing credential-mint/self-kill/restart floors, re-checks (a) the whole command with quote/backslash glue removed and (b) every tokenized argv frame (the same nested-payload descent the other floors use, which also decodes printf/$'…' escapes). The regex tier stays as the raw-text belt; the floor runs only while the rule is enabled, and a floor hit reports a structural-match note in the refusal and the SEL event.
Pinned by test_dev_mode_confirm_flag_denied_under_quote_splitting (8 quoting spellings incl. the reported one and a nested bash -c form) and the floor-wiring invariant in test_self_protection_floor_covers_every_subcommand_rule.
Span ledger: 2nd round landing a blocking finding in this span (r3: flag reachable at all → deny rule + allow_abbrev=False; r4: quoting evades the raw-text tier → argv floor). One more round in this span triggers the same-span stall escalation; the floor now matches the de-escaped argv itself, which is the invariant the rounds were converging toward.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 31, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Audit note — #7224 is being closed in favour of this PR

You are the surviving implementation; #7224 is being closed.

What the two shared

Both 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 fd_real_path(fd) to src/kiro_crew/pinned_fs.py with the same three platform routes plus the same "fd_real_path", all entry, both delete the same 53-line _fd_real_path block from hooks.py (base blob 55b4155443 on both sides) and replace it with the same aliasing top-level import, both repoint spec_builder's importer, and both rewrite the same 'App Dev Mode' section of docs/app-kit/api-reference.md from base blob 342a75a8cc at base line 760 with the same claims (directory-vs-per-file symlink under O_NOFOLLOW, the .dev-grants.json operator grant bound to the realpath at toggle time, toggle-only writes / prune-only reconcile, self-invalidation on repoint, re-toggle to re-bind, the sensitive-root refusal, the added endpoint 400). That section is still the pre-#6854 text on origin/main, so this is the same prose authored twice, not a hub-file co-edit. The relationship is asymmetric: 7224's own body defers 'explicit operator confirmation for escaping grants' to a follow-up, and that follow-up is exactly what 7169 additionally ships (set_dev_mode's confirm_out_of_install_root kwarg with a validate-before-write refusal code, the CLI flag + allow_abbrev=False, cli_commands forwarding, the self-protection-dev-mode-out-of-root-confirm deny rule + _is_dev_mode_out_of_root_confirm argv floor, the SEL audit, ~400 test lines) plus the sandbox.py and apps/routes.py repoints and the location-pinning TestFdRealPathHome that 7224's own Design and First-Principles reviews demanded of it. Neither is superseded by main: main's set_dev_mode is still the two-argument signature, pinned_fs has no fd_real_path, and hooks.py still owns the private helper. Not a stacked branch: single commits off different merge-bases by different authors.

What #7224 had that this PR does not

Please 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 import ctypes, try: import fcntl / except ImportError: fcntl = None # type: ignore[assignment], try: import msvcrt / except ImportError: msvcrt = None, and the macOS branch rewritten to if fcntl is not None and hasattr(fcntl, "F_GETPATH"); this satisfies AUTOSDE.yaml's advisory top-level-imports rule and is the fork GPT review's finding on #7224. (2) The per-module import guard split — give fd_real_path its own try/except independent of from kiro_crew.hooks import safe_read_file_bytes_nolink, so an ImportError from hooks no longer nulls the leaf-module primitive; apply it at src/kiro_crew/apps/builtins/spec_builder/backend/repository.py:41-50 (where the block now lives on main), not at backend/routes.py. Do NOT carry #7224's two unique docs phrasings: code: dev_grant_mismatch (no emitter exists — apps/routes.py returns bare 'invalid' -> a 400 with no code field) and the naming of the internal .dev-apps.json sentinel; both of #7224's reviewers asked for their removal.

This PR still needs work: REBASE

Neither side has merged, so no supersession applies. Parent PR #6854 is merged and landed the grant machinery (.dev-grants.json, _read/_write_dev_grants, the sensitive-root refusal, dev_mode_granted_root) but nothing either PR adds; #6907 is an ISSUE and covers nothing. Main has, however, moved under both of them: the spec_builder fd_real_path guard migrated from backend/routes.py to backend/repository.py, and sandbox.py gained a SECOND lazy from kiro_crew.hooks import _fd_real_path (main sandbox.py:7172) carrying the same now-false '#6907 tracks promoting it' comment, which neither PR repoints. Whichever survives owes both of those on rebase.


From a repository-wide duplicate/overlap audit of every pull request open against main (2026-09-02, 330 PRs, one reviewer per PR). Each PR was read as its full merge-base diff plus its description and every comment and review, then compared against each candidate PR's own diff and against origin/main at 1a765b88ceb7. This PR is not being closed — the note is informational. If the reading is wrong, please correct the reasoning rather than just the conclusion.

@NicholasRBowers
NicholasRBowers force-pushed the fix/dev-mode-grant-followups-6907 branch from d28aa15 to f200a22 Compare September 2, 2026 21:01
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 2, 2026
@NicholasRBowers
NicholasRBowers force-pushed the fix/dev-mode-grant-followups-6907 branch 2 times, most recently from aa83ea1 to feeec9e Compare September 2, 2026 21:32
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • Runtime construction of the confirmation flag evades the static deny tiers — span=3c5b15a2d41e — disposition: fixed in f200a22, per maintainer ruling.

Legitimate, and correctly identified as unfixable by any command-text scan: $(printf %s -- confirm-out-of-install-root)-style synthesis means the flag exists only at execution time, so the r3 deny rule and the r4 argv floor (both retained as defense in depth) can never see it.
Escalated after 3 consecutive blocking rounds in this span (same-span stall protocol; see the escalation comment with the span ledger and options). Maintainer ruling (nrb, 2026-09-02): move enforcement to the flag's consumption point — check human-vs-agent at run time.
Fix, two runtime tiers at the consumption point (set_dev_mode), each unforgeable in the refusal direction:

  1. Runtime attestation checksandbox.agent_confinement_evidence() (the launcher-set sandbox marker + on macOS the kernel's own sandbox_check Seatbelt verdict) refuses confirm_out_of_install_root=True from any agent-confined process with dev_mode_operator_attestation_required, SEL-audited naming the evidence. What the command looked like is now irrelevant; what the process is decides.
  2. Kernel-enforced seal — the authorization record itself (apps/.dev-grants.json) joins _CREW_READONLY_LEAVES, the same keystone-ceiling mechanism that already stops an agent rewriting denied_commands.json to defeat its own deny gate. Seatbelt denies by path pattern; the Linux launcher bind-seals the file read-only (the gateway materializes it at startup so the seal always has a target). A sandboxed process therefore cannot mint, extend, or rewrite a grant even if it bypassed the CLI entirely and wrote the file directly — closing the follow-on evasion beyond this finding. A structural write-probe backs tier 1 (holds under env -u scrubbing), and any grant-touching toggle from a process that cannot write the record is refused atomically up front (dev_mode_grant_record_readonly, SEL dev_mode_grant_write denied) rather than half-applied.
    Residual, documented: a host where the operator explicitly disabled the OS sandbox (agent.sandbox='off' / sandbox_allow_no_isolation) has no structural boundary for ANY control — that opt-out already carries a logged security warning and is out of scope here.
    Pinned by test_confirm_flag_refused_from_agent_sandboxed_process, test_confirm_flag_refused_when_kernel_reports_confinement, test_confirm_flag_refused_when_grant_record_sealed, test_sealed_grant_record_refuses_every_toggle_atomically, test_reconcile_materializes_the_grant_record, and the keystone-seal parametrizations in test_sandbox_governance_mask.py (both launchers, both crew-home spellings).
    Span ledger closes: r3 text rule → r4 argv floor → r5 runtime synthesis → runtime + kernel enforcement at the consumption point, the invariant the rounds were converging toward.

Also in this push: rebased onto current main (base d28aa15760d8f5; 6 conflict files resolved — main's spec_builder handlers refactor moved the fd_real_path call sites into repository.py, repointed there; deny-rule count assertions now 149 = main's 148 + this PR's 1).

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Sep 3, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

Review-ready at head feeec9e1b03a: all 65 checks green, GPT + Opus lanes clean, 0 unresolved threads, all disposition records valid. The ruled fix (runtime human-vs-agent check at the confirm flag's consumption point: sandbox confinement evidence + kernel-sealed grant record) passed both reviewer lanes with no findings. Auto-merge not armed per drive-to-green protocol — awaiting human review.

@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 3, 2026
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
@NicholasRBowers
NicholasRBowers force-pushed the fix/dev-mode-grant-followups-6907 branch from feeec9e to aba292b Compare September 3, 2026 16:40
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running labels Sep 3, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor Author
  • Module comment says the grant record is "never created by the startup reconcile" — span=b9d76b5d23eb — disposition: accepted (stale wording; no code change this round).

Correct catch: since f200a22 the reconcile DOES materialize the empty record at startup (the Linux bind-seal needs an existing target). The wording at dev_mode.py:75 predates that and wasn't tightened.
The security-relevant claim survives intact: reconcile never creates or adds grant ENTRIES — it only prunes dead ones and materializes the empty {} record — and that behavior is documented precisely at the reconcile write-site (dev_mode.py ~328-337, "Never ADD a grant from installed.json"). No reader of the enforcement path is misled about the authorization model.
Not pushing a one-line comment reword now: both reviewer lanes are clean on this head and a push restarts the full CI + review round. The wording will be tightened in the next commit that touches this file for any substantive reason.

@NicholasRBowers

Copy link
Copy Markdown
Contributor Author

Review-ready again at head aba292ba3 after resolving the merge conflict with main (one-line import collision in sandbox.py; kept both imports). All 65 checks green, GPT + Opus lanes clean on this head (GPT's one advisory — stale comment wording — dispositioned above), 0 unresolved threads. Auto-merge not armed; awaiting human review.

@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This 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

  • This PR is OVERLAPPING with PR #7224. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7169: KEEP. PR #7224 was closed unmerged on 2026-09-02 explicitly in favour of this PR (audit note on this thread, and the cross-referenced event in timeline.json). PR #7169 subsumes everything PR #7224 carried, so nothing is lost and there is no live duplicate to reconcile. Files: src/kiro_crew/pinned_fs.py, docs/app-kit/api-reference.md, src/kiro_crew/apps/dev_mode.py.
  • This PR is OVERLAPPING with PR #8262. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7169: KEEP. Additive catalog growth with a numeric collision; both can land, in either order, with a count adjustment on the second. Files: test/test_denied_commands_security.py.
  • This PR is OVERLAPPING with PR #8283. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7169: KEEP. Mechanical counter and fixture collision only, in different rule categories with no behavioral interaction. Not a reason to close or hold either PR; the second one to land needs a count bump. Files: test/test_denied_commands_security.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bolichen97
bolichen97 enabled auto-merge (squash) September 4, 2026 17:56
@bolichen97
bolichen97 merged commit 5185a43 into main Sep 4, 2026
65 checks passed
@bolichen97
bolichen97 deleted the fix/dev-mode-grant-followups-6907 branch September 4, 2026 17:57
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docs+hardening follow-ups from #6854: app-kit dev-mode grant doc, toggle operator confirmation, promote _fd_real_path

2 participants