fix: gate agent-spec mutating endpoints on owner auth (#4944)

[iamwhatever]

Problem / Motivation

Ten mutating HTTP handlers in src/kiro_crew/dashboard/handlers/agents.py change the machine-global agent store (~/.kiro/agents/ and cfg.agents) without first calling is_owner_dashboard_request(request). Any authenticated non-owner dashboard subject — including an app-scoped token — could rewrite an agent spec (PUT /api/agent/config), switch the default agent, install/uninstall MCP servers, skills, and agent packages, sync plugins, or create/update/delete Kiro Crew agents.

Why it matters

An agent-spec write installs tool grants and MCP server commands that every later session on the host executes — it is a privilege-escalation surface, not a preference toggle. The same predicate already gates the sibling owner-only surfaces (mcp_apps.py, handlers_instances.py, handlers_cloud.py, chat_handlers.py), so these routes were a completeness gap in an existing security boundary. PR #2383 added the check to its two NEW installed-agent routes but deliberately left the pre-existing handlers alone, which is why they were still open.

What changed (motivation → approach → change)

Symptom: mutating agents-module routes answer any authenticated subject. Root cause: the owner boundary was enforced per-surface, and this module predates it. Change: a shared _require_owner(request, operation) helper — mirroring the reference denial in mcp_apps.api_mcp_apps_call (SEL log_api_access(..., outcome="denied"), then a 403 {"error": "owner authorization required", "code": "owner_only"}) — now runs at the top of every mutating handler in the module, before any body parse or write:

• api_agent_config (PUT) and api_default_agent (PUT)
• api_capability_mcp_install / _uninstall
• api_capability_skills_install / _uninstall
• _mutate_agent_package (single chokepoint covering the capability agents install + uninstall wrappers)
• api_capability_plugins_sync
• api_agent_detail (DELETE and PATCH; GET stays open)
• api_kirocrew_agents_create / _update / _delete
• api_kirocrew_agents_sync

The four routes beyond the issue's list (plugins/sync, agents/sync, agent delete, agent-detail PATCH) mutate the same machine-global state and share the threat model, so they are gated in the same change so a reviewer can check the MODULE's set is complete. The boundary here is deliberately module-scoped: ~13 mutating routes in other handler modules (mcp.py, mcp_custom.py, mcp_discover.py, core.py) write the same machine-global state and remain open — several are reachable from internal loopback (X-Internal-Secret) callers and need a per-route internal_auth carve-out decision, so that sweep is tracked separately in #5014 rather than implied closed here. Identity comes from the token-auth middleware claims (request["user"] / request["app"]), never from client-set headers, and no internal loopback (X-Internal-Secret) caller reaches these endpoints, so no legitimate caller regresses. The learn-cron-dashboard.md module spec documents the boundary in the same commit.

Tests

• test/test_agents_endpoints_owner_auth.py (new) — enumerate-the-invariant coverage: walks the real route registrars and asserts every mutating verb resolving to this module refuses (a) a non-owner subject and (b) an app-scoped token with a 403 carrying code: owner_only, before any body parse (requests deliberately carry no JSON body, so a handler that parsed first would answer 400, not 403). Proven red on base. A sanity floor pins the 15 known mutating routes so the walk cannot go vacuous, and a read-path test pins that GETs stay open.
• Eight existing agents-handler test modules gain an autouse owner-identity fixture so they keep exercising behavior past the boundary, which now has its own dedicated coverage.

Manual verification

N/A — unit coverage sufficient: the enumeration test exercises every gated route through a real aiohttp TestServer, and the denial shape byte-matches the already-shipped mcp_apps.py reference site.

Related Issues

Closes #4944

Checklist

• Single commit with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 79ceb08

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Real privilege-escalation gap closed at the correct boundary, reusing the shipped owner predicate, with drift-proof route-enumeration coverage and an honest, tracked scope cut.

[DESIGN-REVIEWED] 79ceb08

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 79ceb08a7687a69bfca0be04f5e76478195b1cdb — 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.

I've read the contract, the intent file, and the patch, and verified the surrounding code: the is_owner_dashboard_request predicate and its existing call sites, the sibling owner-deny wrappers (mcp_apps.py, source_providers.py, kiro_prerequisite.py, handlers_instances.py, handlers_cloud.py), the route registrars, and the ungated sibling modules the description defers to #5014. Here is the review.

First-Principles-Verdict: CONCERNS

The fix is real and derived, but _require_owner is the codebase's sixth private spelling of the owner-deny wrapper, and #5014 will mint more.

What this change ships

Intent: stop any authenticated non-owner from rewriting the machine-global agent store — a FIX (closes #4944).

1. Non-owner and app-token callers now get 403 on the ten issue-listed agent-mutation routes — justified (reported defect, agent/owner boundary)
2. Four extra routes (plugins sync, agents sync, agent delete, detail PATCH) gated too — rides along, declared, same threat
3. Denied attempts are SEL-audited off the event loop — justified (existing audit boundary)
4. 403 body carries code: "owner_only" — mandated (AGENTS.md non-2xx code rule)
5. New route-walk test pinning every mutating verb in the module — justified, proven red on base
6. Eight handler test modules now run as owner via autouse fixture — required by item 1
7. learn-cron-dashboard.md updated in same commit — mandated (spec-sync rule)
8. _require_owner helper, module-local — duplicate of existing wrappers (see Watch)
9. Black reformat hunks in test_api_agents_create_template.py — rides along, derived (format-what-you-touched gate)
10. ~13 mutating routes in mcp.py/mcp_custom.py/mcp_discover.py/core.py stay open — declared deferral (Owner-gate the remaining mutating MCP/config routes and converge the three denial-helper spellings #5014)

Watch

• Sixth spelling of one capability. Grepped is_owner_dashboard_request (9 source files) and owner-deny 403 blocks: mcp_apps.py:134 (inline, no code), source_providers.py:4172 _owner_mutation_response, kiro_prerequisite.py:78 _dashboard_owner_only, handlers_instances.py:619, handlers_cloud.py:67 — now agents.py:120 _require_owner. The description itself names the cause ("the owner boundary was enforced per-surface, and this module predates it") and then re-instantiates per-surface enforcement; the ~13 Owner-gate the remaining mutating MCP/config routes and converge the three denial-helper spellings #5014 routes will need a seventh copy unless one wrapper is shared then.
• kiro_prerequisite._is_dashboard_owner (kiro_prerequisite.py:66) is a byte-equivalent reimplementation of the predicate this PR imports — an already-existing divergence this pattern breeds.
• Point patch with counted unfixed siblings (~13 routes, 4 modules) — accepted-and-deferred via tracked Owner-gate the remaining mutating MCP/config routes and converge the three denial-helper spellings #5014, per the description.

Subtractions

• When the Owner-gate the remaining mutating MCP/config routes and converge the three denial-helper spellings #5014 sweep lands, delete _require_owner's module-local copy in favor of the one wrapper that sweep must share — do not copy agents.py:120 into mcp.py et al.
• Drop the dangling comment reference connections._audit_started (agents.py:136) — grep finds no such symbol; the real precedent is the unnamed to_thread blocks in connections.py:316,412.

[FIRST-PRINCIPLES-REVIEWED] 79ceb08

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

The change is a clean, well-scoped security hardening. The discovery pass found no candidates, and my own falsification confirms:

• All 15 mutating handlers in the module are gated; read (GET) paths stay open.
• _require_owner uses the same is_owner_dashboard_request predicate already governing mcp_apps and source-provider mutations; internal loopback callers (MCP/cron) reach capability installs via the manager directly, not these HTTP routes, so denying non-owner HTTP callers breaks no legitimate flow.
• The SEL audit is off-loop and swallows failures without changing the 403 outcome.
• The enumerate-the-invariant test pins the gated set against the real router walk.

No grounded defect at 80+ in the changed lines, and nothing new to add.

No findings.

[OPUS-REVIEWED] 79ceb08

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

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

[iamwhatever]

Dispositions for the First Principles CONCERNS on 3b8e438aad34:

• Unfixed siblings, counted (~13 open mutating routes in handlers/mcp.py, mcp_custom.py, mcp_discover.py, core.py) — accepted-and-deferred.

  the module boundary is not the threat boundary, and ~13 routes writing the same machine-global MCP/agent state stay open

  The count is correct and the concern is real. It is deferred, not folded in, for a concrete reason: unlike the agents module (where I verified no internal loopback caller reaches any gated route), several /api/mcp/* mutators ARE reachable from internal X-Internal-Secret paths (MCP tool plumbing), so each sibling route needs a per-route internal_auth carve-out decision that would more than double this PR's review surface. Filed as Owner-gate the remaining mutating MCP/config routes and converge the three denial-helper spellings #5014, which names every open route, includes the reviewer's convergence point below, and requires the loopback-caller check per route before gating. The PR body's scope sentence has been updated to name the module boundary explicitly rather than implying the sweep is complete.

• Third denial-wrapper spelling (_require_owner vs mcp_apps.py:134 inline vs kiro_prerequisite.py:78) — accepted-and-deferred, same follow-up.

  3 module-private spellings of "SEL-audit then 403" that will drift; the sibling sweep above is the moment to converge on one

  Agreed that the sweep is the right moment to converge: hoisting a shared helper now would touch mcp_apps.py and kiro_prerequisite.py — files this fix otherwise does not change — while Owner-gate the remaining mutating MCP/config routes and converge the three denial-helper spellings #5014 must touch them anyway. Step 2 of Owner-gate the remaining mutating MCP/config routes and converge the three denial-helper spellings #5014 is that convergence.

[github-actions]

UX Review (Fable 5) — ⏭️ skipped

Revision 79ceb08a7687a69bfca0be04f5e76478195b1cdb touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Advisory — does not block merge.

[iamwhatever]

Review-ready again at 79ceb08a7.

The GPT 5.6 blocking finding (synchronous SEL audit in _require_owner could stall the event loop on first-construction — trust-dir creation, key validation, Windows icacls) is fixed: the audit now runs via asyncio.to_thread, awaited at all 13 call sites, matching the established pattern in connections.py. Squashed to a single commit per PR Hygiene.

All checks green, GPT ✅, Opus ✅, Design ✅; First Principles CONCERNS remains advisory (dispositioned in #5014).

[bolichen97]

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

• PR #2383 is PARTIALLY_COVERED relative to this PR. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #2383: REBASE. Only the incidental owner-gate implementation is now redundant; the create/edit/clone surface and the security predicates remain entirely absent from main. Rebase onto the shared owner helper and drop the inline copy rather than closing anything. Files: src/kiro_crew/dashboard/handlers/agents.py.

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