fix(dashboard): mask agent free-text fields in the config endpoint response (#8717)

[bolichen97]

Summary

GET /api/config/kirocrew's _masked_config_dict is schema-driven: it masks only values the JSON schema marks sensitive=True. Agent-record free-text fields (description, triggers, and siblings) are agent- and package-writable — an agent can edit config.json directly, and agent sync copies description straight off a discovered agent spec — but they are not schema-sensitive, so they left BOTH response sites of the endpoint (the GET body and the PATCH echo) unmasked. This is the config-endpoint half of the class whose roster half PR #8472 proposes to close; issue #8717 records the asymmetry.

The fix adds a second pass to _masked_config_dict:

• _AGENT_UNTRUSTED_TEXT_FIELDS names every str field of KiroCrewAgentConfig whose load path does not pin its shape (description, triggers, kiro_agent, workspace, memory_store, model, source, telegram_account). reasoning_effort (coerce_effort) and session_color (_safe_color) are excluded because their load guards already refuse redactable content. An invariant test enumerates the dataclass's str fields against the tuple plus that exception set, so a newly added free-text field fails loudly instead of shipping unmasked.
• _mask_agent_free_text replaces a value _redact_external would alter (credential- or exfiltration-URL-shaped) WHOLESALE with the existing _SENSITIVE_MASK sentinel; a non-string is masked too (description has no load-time type guard, so one genuinely reaches here). Benign content passes through byte-identical. Keyed on _redact_external itself so this rule and the roster's cannot drift apart.
• Both response sites funnel through this one function, so the pass gives the redaction rule surface coverage on this endpoint.

Write-back safety: neither branch of the endpoint can echo the sentinel into storage — the PATCH allowlist (_EDITABLE_CONFIG) names no agents.* path, and the PUT branch reads only the singular agent section against a hardcoded key list. The save() path is never masked.

Named costs: a value containing one credential-shaped token is masked entirely (the same trade _masked_config_dict already makes); the overview config tab renders kiro_agent/workspace/memory_store and cross-references the latter two, so a masked value breaks that "used by" row — but only for a record whose value is already credential-shaped and therefore already meaningless as a reference. The record KEY (the agent name) is handled by REMOVAL: agent sync stores a discovered agent's name as the map key, so a credential-shaped package name would ship verbatim as a key, and masking a key would collide two suspicious records into one entry. A suspicious-keyed record is dropped from the browser view (save() still carries it) and the name-reference fields that could still spell it (default_agent, session.pool_agent) are masked when they match. Named cost: such a record is invisible in the config tab — its name was never renderable content.

Out of scope: GET /api/agents still ships these fields verbatim on main — that half of the class is PR #8472's, which carries the same mask plus the mask-means-unchanged write rule its route needs.

Tests

• New test/test_config_agent_fields_redaction.py (18 tests): credential- and URL-shaped values masked wholesale; benign / empty / CJK strings byte-identical; non-strings masked; partial-credential wholesale-mask pinned; extended field set covered; structural fields untouched; save() path untouched; redactor-precondition assertions localizing detector regressions; a dataclass-enumeration invariant ratcheting the field tuple; and both response sites driven over HTTP (real GET route, real PATCH handler).
• Mutation-verified 8 ways: removing the second pass, in-place scrub instead of the sentinel, dropping triggers from the tuple, dropping workspace from the tuple, non-string passthrough, unconditional masking, keeping a suspicious-keyed record, and skipping the name-reference masking each turn distinct tests red.
• Local gates: black-baseline, isort, flake8, mypy (1294 files), subprocess-encoding, brand-name, harness-parity all green; 282 targeted tests across the config-handler test files pass.
• Pre-push review fleet: GPT lane NO-FINDINGS with evidence; Opus lane raised 2 BLOCKING (docstring accuracy against the unmerged fix(dashboard): name every field GET /api/agents ships, instead of spreading the record #8472; field-set coverage) + 5 CONCERNS — each verified on source and fixed in this head.

Pattern harvest

Rule candidate: flag any aiohttp json_response handler that serializes a config/record dataclass where some str fields lack a load-time shape guard and the response path applies only schema-driven masking — the unguarded fields ship agent-writable free text verbatim (semgrep: web.json_response($X) where $X flows from to_dict()/asdict() without a per-field mask pass).
Class: schema-driven redaction gives point coverage; writable free-text fields outside the schema's sensitive set need their own named-field mask pass, ratcheted by a dataclass-enumeration invariant test.

Closes #8717

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Real boundary (agent-writable text reaching the browser), closed with the existing detector and sentinel semantics, costs named, write-back safety verified.

Suggestions

• The ratchet test matches f.type in ("str", str) only, so a future str | None / Optional[str] record field escapes the invariant while staying unmasked; widen the match to optional-str annotations so the recurrence guard actually covers the annotation forms people write.
• When fix(dashboard): name every field GET /api/agents ships, instead of spreading the record #8472 lands, hoist _redact_external + the wholesale-mask rule out of handlers.discover into one shared browser-view helper for agent records; today core imports a sibling handler's private and the same rule will exist in two handlers, which is exactly the drift the PR says it wants to prevent.

[DESIGN-REVIEWED] 3fb317c

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 3fb317c

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

Based on my analysis:

CANDIDATE 1 claims the removal-reference masking is incomplete because slack.channels.<id>.agent also spells a removed credential-shaped agent name and isn't masked.

Verifying the pieces:

• ChannelConfig.agent (sections.py:3581) is real free text serialized under slack.channels.<id>.agent (loader.py:3746-3749), and this endpoint does not strip the slack section — so it does ship to the browser.
• The if removed: block (core.py:275-280) masks only default_agent and session.pool_agent, not channel bindings. So the docstring's claim that "the name-reference fields that could still spell it are masked" is indeed incomplete for this field.

But testing (a)/(b)/(c) at the required bar:

• default_agent/pool_agent carry a credential-shaped name automatically whenever the active/pooled agent has that name. slack.channels.<id>.agent is an optional per-channel override defaulting to ""; it holds the name only if a user deliberately binds a channel to a credential-named agent. That is exactly the "if a caller were to" condition the instructions require me to drop, not an input that occurs in practice.
• The channel agent field was never masked by prior code either, so this is a pre-existing surface the diff neighbours rather than one the diff introduces — at most advisory, and its harm crosses no boundary (owner viewing their own token-authenticated dashboard) that the covered fields don't.

The candidate's own confidence is "medium," with the channel-binding link explicitly unverified. It does not clear 80.

Step 2 review of the changed lines (_mask_agent_free_text non-str/empty handling, the pop over a copied key list, the removed-set reference masking) surfaces no grounded crash, data-loss, or leak on a path the diff adds.

No findings.

[OPUS-REVIEWED] 3fb317c

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

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 3fb317c479fba8b56c78434939b13e74a56f7b11 — 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.

All claims verified: agent sync copies description verbatim from discovered specs (agents.py:2719), _EDITABLE_CONFIG is a dotted-path allowlist with no agents.* entries, the tuple plus two exceptions exactly cover the dataclass's 10 str fields, and both response sites funnel through _masked_config_dict (core.py:1700, core.py:2483). One sibling surface the PR never mentions: GET /api/members ships kiro_agent, workspace, memory_store, model verbatim (members.py:155-158).

First-Principles-Verdict: CONCERNS

Every item is derived from the external-content boundary and lands at the choke point, but a third surface shipping four of the same fields verbatim goes uncounted.

What this change ships

Intent: stop agent- and package-writable free text in agent records from reaching the browser unmasked through the config endpoint (#8717). This is a FIX.

1. Credential/exfil-shaped agent description/triggers now render as the mask in the config view — justified (the fix)
2. Six more record fields get the same conditional mask — justified; invariant test ratchets the set
3. Masked values break the config tab's "used by" cross-reference — declared cost, only for already-suspicious values
4. Non-string field values render as the mask, not raw JSON — justified; load path has no type guard
5. A credential-shaped agent NAME makes its whole record vanish from the config view — declared; masking a key would collide records
6. default_agent/session.pool_agent show the mask when they name a vanished record — justified; closes the residual leak from item 5
7. GET /api/agents still ships these fields verbatim — declared out of scope (PR fix(dashboard): name every field GET /api/agents ships, instead of spreading the record #8472)

Watch

• Unfixed sibling the description never names: GET /api/members ships kiro_agent, workspace, memory_store, model verbatim (members.py:155-158) — 4 of the 8 fields this PR classifies as untrusted, on a third surface that is neither "half" of the class (Agent description/triggers leave unmasked from GET /api/config/kirocrew, so #8472's redaction is point coverage #8717's config endpoint, fix(dashboard): name every field GET /api/agents ships, instead of spreading the record #8472's roster). Grepped cfg.agents.items() across dashboard/: config endpoint, roster, and members are the three serialization sites; the first two are accounted for, this one is not.
• The wholesale-sentinel-over-in-place-scrub docstring leans on "any future write-side rule" — a so-we-can-later justification for an endpoint whose own analysis proves no write-back exists. The choice is harmless and mutation-pinned; only the stated rationale is inherited.

[FIRST-PRINCIPLES-REVIEWED] 3fb317c

[bolichen97]

Disposition of the First Principles advisory CONCERNS on 3fb317c47. One finding per record (all lanes green; nothing blocking).

• First Principles — the uncounted third surface. Verified real: GET /api/members ships name/kiro_agent/workspace/memory_store/model verbatim (members.py:153-158). Filed as GET /api/members ships agent-record free text verbatim — third surface of #8717's class #8781 with the closed enumeration (the three cfg.agents.items() serialization sites in dashboard/ are now all accounted for: this PR, fix(dashboard): name every field GET /api/agents ships, instead of spreading the record #8472, GET /api/members ships agent-record free text verbatim — third surface of #8717's class #8781). Out of this PR's scope by the same reasoning the PR body applies to the roster half.

[bolichen97]

Disposition of Design Review suggestion 1 on 3fb317c47 (lane verdict PASS; advisory). One finding per record.

• Design — ratchet-test annotation forms (str | None escaping f.type in ("str", str)): accepted, folded into GET /api/members ships agent-record free text verbatim — third surface of #8717's class #8781 so the widening lands with that fix rather than re-running this PR's full CI for a test-only tweak on a green head.

[bolichen97]

Disposition of Design Review suggestion 2 on 3fb317c47 (lane verdict PASS; advisory). One finding per record.

• Design — hoist _redact_external + the mask rule into a shared browser-view helper once fix(dashboard): name every field GET /api/agents ships, instead of spreading the record #8472 lands: accepted and recorded in GET /api/members ships agent-record free text verbatim — third surface of #8717's class #8781, whose fix would be the third consumer of the rule — the natural point for the hoist.