fix(dashboard): owner-gate the skills CRUD write endpoints (#7344)

[patrigao]

Problem / Motivation

The dashboard skills CRUD write handlers in src/kiro_crew/dashboard/handlers/prompts.py — api_skills_create (POST /api/skills) and the PUT/DELETE branches of api_skill_detail — performed writes with no owner gate, no app-token refusal, and no SEL audit. Any non-owner dashboard session or app-token-bearing caller could create, edit, or delete skills.

Why it matters

Skill content is injected into agent context, so a skill write is an instruction-injection surface. The sibling surfaces already treat it that way: the skill-trust handlers in the same file gate on owner + app-token via _deny_non_owner_skill_trust, and ask_question.py carries _deny_app_token / _deny_non_owner. Leaving the CRUD writes open made the guard set asymmetric: the trust ledger was protected while the content it vouches for was not.

What changed (motivation → approach → change)

Symptom: unguarded skill writes. Root cause: the guard helper was scoped to the skill-trust operations only, so the CRUD handlers had nothing to call. Change: generalize the same-file helper _deny_non_owner_skill_trust → _deny_non_owner_skill_operation (parameterized on the operation label), and apply it at the top of api_skills_create, the PUT and DELETE branches of api_skill_detail, and — per the first-principles review round — the rest of the mutating skill-handler family: api_skill_pending_approve (promotes candidate content into the live catalog, the same instruction-injection surface), api_skill_pending_dismiss, api_skills_pending_dismiss_all, api_skill_pin, and api_skill_inject_on_trigger. Add per-outcome SEL audit records (denied and ok) via the _sel().log_tool_invocation pattern already used by api_prompts. GET behavior is unchanged (it already carries the foreign-app-slot guard for kiro-workspace names). The CAS/descriptor-pin half of the asymmetry is tracked separately in #5028 and is deliberately out of scope here.

Same-commit spec updates: docs/system-specs/modules/learn-cron-dashboard.md and docs/system-specs/modules/memory-skills-hooks.md now document the write-path guard.

Tests

New test/test_skill_write_guard.py (fixtures and request-identity setup follow the existing skill-trust guard tests):

• non-owner write → 403, for create, update, and delete
• app-token caller → denied, for all three write paths
• owner happy path still works (create/update/delete succeed)
• SEL events emitted per outcome (denied and ok records asserted)
• TestSkillLifecycleEndpointsAreGated: pending approve/dismiss/dismiss-all, pin, and inject-on-trigger each refuse app tokens and non-owner subjects with the audited 403, and the loader records zero calls (no mutation happens before the gate)

test/test_skill_trust_store.py updated for the renamed helper; test/test_skill_pending_api.py runs its business-logic cases as the owner (the gate has its own dedicated coverage). Focused suites: 160 passed; broader prompts/skills selection: 571 passed.

Manual verification

N/A — unit coverage sufficient: the change is a handler-level guard with no UI surface, and every denied/allowed path plus its SEL record is asserted directly.

Related Issues

Closes #7344

Pattern harvest

Rule candidate: review-prompt
Pattern: guard set applied to the read/trust surface of a resource but not to its sibling write handlers — check every mutating branch of a resource's handler family when one member carries an owner/app-token gate.

Checklist

• At most two commits (one is the norm), 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]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

The PR is a purely additive security hardening. I've verified:

• is_owner_dashboard_request refuses non-empty app tokens and non-owner subjects (source_providers.py:6589-6593).
• Each _deny_non_owner_skill_operation call short-circuits before any body parse, match_info read, or loader call.
• The readonly-prefix 405 before owner 403 is not an oracle (static prefixes).
• Outcome audits wrap log_tool_invocation in except Exception so a committed mutation can't 500.
• GET stays ungated (a read).

The discovery pass found no candidates; my independent falsification confirms the change is sound, and no new grounded finding at the 80+ bar surfaced.

No findings.

[OPUS-REVIEWED] d3f8264

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

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

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

The "gate every mutating skill handler" sweep stops at the prompts.py file boundary — api_skills_discover_install stays open to the same callers.

Watch

The description claims the sweep covered "the rest of the mutating skill-handler family," but api_skills_discover_install (src/kiro_crew/dashboard/handlers/discover.py:273, registered in routes/skills.py:42) writes third-party skill bundles — SKILL.md plus scripts — into the catalog with only an internal_auth refusal, no is_owner_dashboard_request gate. A non-owner dashboard subject or app-token bearer — exactly the callers this PR names as the threat — can still inject skill content, via the member that imports arbitrary registry content rather than merely editing local files. Only #5028 (CAS/descriptor-pin) is declared out of scope; this member isn't.
Clears when: api_skills_discover_install calls _deny_non_owner_skill_operation (here or in a named follow-up issue linked from the PR), with a denial test.

Suggestions

• The root recurrence mode is hand-enumerated per-handler gating — the PR's own pattern-harvest sentence describes it. A test that walks the registered route table and asserts every POST/PUT/DELETE under /api/skills invokes the owner gate would have caught the discover-install miss and pins future handlers structurally instead of one-by-one.

[DESIGN-REVIEWED] d3f8264

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of d3f8264c3af1444a4546e0b38a05807efa34dc91 — 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

One counted sibling stays open: api_skills_discover_install still lets a non-owner persist skill content — the exact harm this PR names.

Not justified as shipped

• Item 4 (lifecycle-endpoint gates) — rides along: beyond Adopt the prompt-write guard set on the skills CRUD handlers #7344's stated CRUD scope, though declared and carrying the same named harm.

What this change ships

Intent: stop non-owner and app-token callers from writing skill content that gets injected into agent context — a FIX (#7344, new tests fail on base).

1. Skill create by a non-owner or app token now gets an audited 403 — justified
2. Skill update likewise refused before any write — justified
3. Skill delete likewise refused — justified
4. Pending approve/dismiss/dismiss-all, pin, and inject-on-trigger now owner-only — rides along (declared, same harm)
5. Every skill-write outcome now leaves a SEL audit record — justified
6. Guard helper renamed trust→operation, one gate for the family — justified
7. Two specs updated in the same commit — justified
8. Existing business-logic tests now run as the owner — justified

Watch

Point patch with 1 counted unfixed sibling: grepped is_owner_dashboard_request|internal_auth in src/kiro_crew/dashboard/handlers/discover.py — one hit, the internal_auth check at discover.py:295. That refuses the agent, not the actors this PR's defect names: a non-owner dashboard subject (and an app token whose path-only grant prefix-matches /api/skills/-/discover, per server.py:818) can still install third-party skill content into the catalog. The added spec sentence — "it is not owner-gated" — records the gap without a derived reason a non-owner may install what they may not create.
Clears when: api_skills_discover_install calls _deny_non_owner_skill_operation, or a linked issue records the derived reason non-owner install stays open.

[FIRST-PRINCIPLES-REVIEWED] d3f8264

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] d3f8264

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

[patrigao]

Disposition: fixed in 559628f89 — the First Principles CONCERNS on the 5 unfixed mutating siblings.

5 counted unfixed siblings, same file, same root cause — api_skill_pending_approve, api_skill_inject_on_trigger, api_skill_pin, api_skill_pending_dismiss, api_skills_pending_dismiss_all were reachable without the owner gate.

The finding is legitimate and in scope: api_skill_pending_approve promotes candidate content into the live catalog — exactly the instruction-injection surface this PR gates — and the PR's own harvested pattern ("check every mutating branch of a resource's handler family") demanded the sweep. All five handlers now call _deny_non_owner_skill_operation before touching state or the loader, with the same SEL-audited allowed/denied outcomes. New TestSkillLifecycleEndpointsAreGated pins the widened coverage (app-token + non-owner refusal per endpoint, zero loader calls before the gate); test_skill_pending_api.py business-logic cases now run as owner. Spec docs and the PR body updated to match the true coverage. Focused suites 160 passed, mypy clean.

[patrigao]

• First-use SEL initialization blocks the event loop — span=6f77c3fb4ba7 (prompts.py:801)

Disposition: rebutted — the synchronous SEL call inside an async handler is the file's pre-existing, file-wide idiom, not something this diff introduces or makes newly reachable.

Fresh gateway -> mutation handler -> synchronous SEL key/directory I/O -> all gateway tasks stall.

Evidence: _deny_non_owner_skill_operation and its sync _sel().log_api_access calls exist on main today, invoked by the skill-trust endpoints; this PR only adds call sites. The same file carries ~30 pre-existing synchronous _sel().log_tool_invocation calls in async handlers, including api_prompts_list (prompts.py:207), which runs on every dashboard prompt listing — first-use SEL initialization therefore already fires on ordinary GET traffic on main long before any skill mutation, so this diff does not change when or where that one-time init cost is paid. This lane passed the identical calls in the three CRUD handlers on the round-1 head. Converting only these 8 call sites to asyncio.to_thread while the rest of the file stays synchronous would fork the idiom mid-file; the consistent fix is a file-wide (arguably codebase-wide) async-SEL refactor, which is disproportional to an owner-gate PR. Happy to file that refactor as a follow-up if maintainers want it.

[patrigao]

• Audit initialization failure reports 500 after deletion — span=6f77c3fb4ba7 (prompts.py:1198)

Disposition: fixed in d623352d2.

Invalid/unreadable SEL key -> gate suppresses initialization failure -> deletion commits -> audit retries and raises -> client receives 500 for a completed mutation.

Legitimate: the three post-mutation outcome audits this PR added (create/update/delete) were naked, while the file's own convention — the guard helper — explicitly swallows SEL failures to preserve the response. A raised audit after delete_skill committed would have returned a 500 for a completed mutation. All three audits are now wrapped in try/except with a debug log, mirroring the guard's comments ("the mutation is committed; never 500 on an audit write"). New TestAuditFailureNeverBreaksACommittedMutation pins the contract: with log_tool_invocation raising, create/update/delete all still return 200.

[patrigao]

• Spec claim overstates the gated surface — span=c3b0cae4175d (learn-cron-dashboard.md:1088)

Disposition: fixed in d623352d2.

the added claim so no skill mutation is reachable by an app token or non-owner subject (#7393 first-principles round) is false: api_skills_discover_install … refuses only request.get("internal_auth"), never is_owner_…

Confirmed against discover.py:238: the discover-install endpoint refuses internal-secret callers only, by design (it keeps registry installs a deliberate dashboard action while letting agents read via skill_fetch), and is not owner-gated. Both spec docs now scope the claim precisely: the owner gate covers every mutating skill handler in prompts.py, and the discover-module install endpoint is named explicitly with its own guard model. Gating discover-install under the owner check would change a different module's deliberate auth design and belongs to its own review if maintainers want it.

[patrigao]

• Docstring says "every mutating skill handler" while pending-list GET prunes expired candidates ungated — span=dd99d2a43380 (prompts.py:61)

Disposition: rebutted — the enumerated claim is exact, and TTL pruning is outside the threat model the gate (and the spec sentences) address.

"every mutating skill handler" is false because pending-list GET calls prune_pending, deleting expired candidates without this gate

The docstring phrase is immediately qualified by its colon-enumerated list — CRUD writes, pending approve/dismiss/dismiss-all, pin, inject-on-trigger — which is the precise covered set; prune_pending is not a handler but opportunistic TTL housekeeping inside the read endpoint (prompts.py:700, pre-existing design). The gate exists because skill mutations are an instruction-injection surface: content flowing into agent context. Pruning cannot inject anything — it removes candidates already expired per pending_ttl_days, age measured from filesystem mtime specifically so a caller cannot influence what is deleted (skills.py:3958 documents that hardening). A non-owner GET can neither author, promote, nor select content through it. Narrowing the prose to exclude a non-injectable, non-caller-controlled expiry sweep would trade accuracy in the threat-model sense for pedantry in the state-change sense, at the cost of re-arming a fully green review round.

[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 #7105 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7105: MERGE_DISCUSSION. Independent and complementary concerns in the same handlers; only the refusal ordering is worth a note when both land, and no code needs to change in this PR for it. Files: src/kiro_crew/dashboard/handlers/prompts.py.
• This PR is OVERLAPPING with PR #5161. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7393: REBASE. Same campaign, no code conflict, but they entrench two different owner-gate spellings and error codes. Worth one maintainer decision on whether the skills family should land on the shared _shared.py boundary; that decision should not block 7393, since 5161 gives the skills routes no coverage at all. Files: src/kiro_crew/dashboard/handlers/_shared.py.
• This PR is OVERLAPPING with PR #8259. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7393: REBASE. Complementary guards on the same handlers; both are needed and neither is redundant. The second to land needs a mechanical rebase of the shared insertion points and the shared spec paragraphs. Files: src/kiro_crew/dashboard/handlers/prompts.py, docs/system-specs/modules/learn-cron-dashboard.md.

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

[NicholasRBowers]

🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: This PR has been inactive for 7+ days with a merge conflict. I've assessed the blockers and they appear resolvable — I'll push fixes directly to this branch as a co-author.

Assessment: Readiness already passed and all five AI lanes are green; the only blocker is the mechanical rebase the relationship audit forecast when #8259 landed on the same handlers. Plan: keep both guard layers in prompts.py (owner gate above the write-key refusals, memoised GET untouched) and merge the spec-doc paragraphs so both descriptions survive, then re-run the focused guard suites and push.

If you'd prefer I don't touch this PR, add the pr-no-autofix label.

[bolichen97]

Rebased onto main 1192049a3 by a maintainer as part of the 2026-09-08 open-PR audit.

Conflicts resolved (all mechanical, from merged #8259):

• handlers/prompts.py: kept main's await asyncio.to_thread(...) create/update/delete calls, reseated your owner gate above each and the SEL log_tool_invocation block after each. Both guard layers are kept; the readonly-territory 405/400 checks are unchanged, so on a kiro-user//kiro-workspace/ key the territory refusal still answers before the owner 403 for PUT/DELETE. Say the word if you want the owner gate hoisted above it.
• learn-cron-dashboard.md, memory-skills-hooks.md: merged, not overwritten. Your owner-gate prose plus main's readonly-territory prose.
• Added your autouse owner fixture to main's newer test_skill_write_territory_guard.py and to the skill case in test_dashboard_pinned_write_migration.py; without it the new gate 403s them.

Gates run locally on changed files: black, isort, flake8 (clean; test_skill_pending_api.py is black-baselined and already fails on main), pytest over the 6 skill/prompts test modules, 281 passed.

Please review the resolution. A maintainer push makes the maintainer the last pusher, so a second approver is needed under the repo's last-push rule. Reply if anything looks wrong.