fix(dashboard): redact crew record strings at both roster serializers - #8465
fix(dashboard): redact crew record strings at both roster serializers#8465dwu96 wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
readiness: checking
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound redaction chokepoint, but the read-side fix spawns a three-layer write-back protocol, and the new key-redaction rule is deliberately withheld from sibling surfaces it equally indicts. Watch
[DESIGN-REVIEWED] a56bb4b |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of First-Principles-Verdict: CONCERNS The redaction fix is derived and mechanism-level; the What this change shipsIntent: stop agent-writable config strings (credentials/exfil URLs) reaching the browser via the two roster endpoints — a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] a56bb4b |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsNo findings. Both candidates require an agent [OPUS-REVIEWED] a56bb4b Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — 🔴 changes requested (blocking)GPT 5.6 found at least one blocking issue that must be resolved before merging This comment is updated in place on each push. BLOCKING -- website/src/utils/redactedPrefillGuard.ts:62 -- Marker equality is mistaken for an untouched field (origin: validation) |
github-actions
Bot
added
readiness: action required
a4b8938 to
cf50490
Compare
|
span=a4416c88f899 — fixed in cf50490. Finding ( Fix: Evidence: red-first —
[operator: dwu96] |
cf50490 to
b390de7
Compare
|
span=d77e9c4049d0 — fixed in b390de7. Finding ( Fix: Evidence: red-first —
[operator: dwu96] |
github-actions
Bot
added
readiness: checking
b390de7 to
570247e
Compare
|
span=d77e9c4049d0 — fixed in 570247e. Finding ( Fix — the suggested design adopted in full, plus a server-side floor:
Residual, stated rather than implied (also recorded in Evidence: stale-race test red on b390de7, green on 570247e with the newer value preserved; manifest pinned at helper + endpoint + project-row level; mutants killed individually (drop pure-marker rule → exactly the stale-race test red; empty manifest → exactly the three manifest tests red); frontend omission pinned by 5 unit tests incl. edited/cleared/marker-paste/no-manifest branches; tsc + eslint clean; marker-over-benign paste and clear-to-empty behaviors preserved green.
[operator: dwu96] |
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of The frontend holds all the context I need: the guard is save-side only, and no visible affordance was added for a redacted prefill. Emitting the review. UX-Verdict: CONCERNS A Watch
Suggestions
[UX-REVIEWED] a56bb4b |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
570247e to
816c7b6
Compare
|
span=3933f2677293 — fixed in 816c7b6. Finding ( Fix — the suggested tracking adopted: Evidence: new unit test
[operator: dwu96] |
|
span=d77e9c4049d0 — fixed in 816c7b6. Finding ( Fix: the probe load now runs off-loop via Evidence: red-first —
[operator: dwu96] |
0768809 to
5e5c3d0
Compare
|
span=d77e9c4049d0 — fixed in 5e5c3d0. Finding ( Fix: Evidence: red-first —
[operator: dwu96] |
github-actions
Bot
added
readiness: action required
Free-text string fields on the crew/agent record (description, triggers, workspace, memory_store, ...) are agent-writable through config.json and were echoed to dashboard JSON verbatim by GET /api/agents (a full dataclasses.asdict spread, both row sources) and GET /api/members (an explicit allowlist with no redaction pass over the values). A short credential (e.g. a 20-char AWS access key id) written into any such field reached the browser unredacted. Fix: one field-generic serialization chokepoint per endpoint. _shared.redact_record_strings applies redact_exfiltration_urls then redact_credentials (the members.py _sanitize order) to every string value of a serialized record, recursing into nested dicts/lists by delegating to _redact_memory_field, the shared recursive scrubber. /api/agents runs each row through it after the usage sort (covering the cfg.agents spread AND the project-scope default rows); /api/members runs the allowlisted record dict through it at construction (the allowlist contract stays; only values are scrubbed). The transcript last_message preview keeps its existing pre-truncation _sanitize pass untouched. Fixes #8447
5e5c3d0 to
a56bb4b
Compare
|
span=34179bd30024 — fixed in a56bb4b. Finding ( Fix — the suggested first option adopted: the clause is dropped; Evidence: red-first —
[operator: dwu96] |
github-actions
Bot
added
readiness: checking
|
span=3933f2677293 — needs-a-decision (maintainer ruling requested; lane-vs-lane conflict on an already-adjudicated span). The round-6 finding asks to (a) replace the snapshot-equality omission with explicit per-field touched tracking and (b) remove the backend pure-marker suppression. Both halves recycle ground this PR adopted at GPT's own direction and that GPT stamped CLEAN one head earlier:
The residual the finding names is real but is a DESIGN TRADEOFF, not a one-sided defect: a value that is byte-identical to a bare redaction marker is indistinguishable from a stale echo by construction. The two lanes now demand opposites on it — Opus: refuse more such values (fail toward preserving the stored value at a security boundary); GPT: preserve them as explicit edits (fail toward accepting a marker-shaped write into config.json). The current code takes the Opus/fail-safe side: the only capability lost is deliberately storing a value that IS a bare Question for the maintainer: should [operator: dwu96] |
… it cannot show
api_kirocrew_agents built each roster row with
{"name": name, "scope": ..., **dataclasses.asdict(agent_cfg)}, so the
endpoint's response contract was "every field KiroCrewAgentConfig has now,
plus every field anyone adds later", automatically. handlers/members.py
made the opposite call for GET /api/members and documented why: the
response is a network-boundary contract, and a spread ships a future
field -- internal bookkeeping, a filesystem path, a credential-shaped one --
to the browser by omission.
KEY half: both row sources (the cfg.agents rows and the project-scope
rows) now go through one _agent_roster_row allowlist naming 11 keys, so
the two cannot drift into different key sets either. Three record fields
are withheld, each with no consumer anywhere in website/src: the two
per-agent watchdog windows and the deprecated, inert telegram_account.
VALUE half, two co-operating halves. Every record value is agent- or
package-writable -- an agent can edit config.json, and _do_agents_sync
copies description straight off a discovered agent spec -- so a value the
redactors would alter, or a non-string the loader let through, is replaced
WHOLESALE by _SENSITIVE_MASK, the sentinel _masked_config_dict already
uses for the same job on GET /api/config/kirocrew. Benign content is
byte-identical.
api_kirocrew_agent_update then drops any body field carrying that mask,
treating it as unchanged -- the remedy _masked_config_dict's docstring
prescribes verbatim. Without it the read half would destroy stored config:
the agents page seeds its edit sheet from a roster row and sends every
field on every save so that "" can clear a pin.
A FIXED sentinel rather than redacting in place is the load-bearing
choice. Recomputing the redaction to recognise the view breaks two ways a
sentinel does not: a second redaction chain over the same response (#8465)
produces a view the predicate no longer matches, and a stored value that
changes between the GET and the PUT makes the stale view read as a genuine
edit, writing redaction markers into the config. The sentinel depends on
neither. Cost, named: a value containing one credential-shaped token is
masked entirely rather than partially, the same trade the config endpoint
already makes.
Masking a non-string rather than coercing it to "" is what lets the write
rule PRESERVE it; an echoed "" would read as a genuine edit.
name is the single exemption, and only for the owner: it travels in the URL
rather than the body so the write rule cannot protect it, and masking it
would make /api/agents/{name} unaddressable. An app token cannot reach
those owner-gated routes, so name is masked there too. dict[str, str] is
now true rather than aspirational.
test_agents_roster_contract.py pins the exact key set at the endpoint for
both row sources, ratchets the allowlist against dataclasses.fields, and
covers both halves over HTTP including the stale-view case and that a
genuine edit still writes through.
Fixes #8454
… it cannot show
api_kirocrew_agents built each roster row with
{"name": name, "scope": ..., **dataclasses.asdict(agent_cfg)}, so the
endpoint's response contract was "every field KiroCrewAgentConfig has now,
plus every field anyone adds later", automatically. handlers/members.py
made the opposite call for GET /api/members and documented why: the
response is a network-boundary contract, and a spread ships a future
field -- internal bookkeeping, a filesystem path, a credential-shaped one --
to the browser by omission.
KEY half: both row sources (the cfg.agents rows and the project-scope
rows) now go through one _agent_roster_row allowlist naming 11 keys, so
the two cannot drift into different key sets either. Three record fields
are withheld, each with no consumer anywhere in website/src: the two
per-agent watchdog windows and the deprecated, inert telegram_account.
VALUE half, two co-operating halves. Every record value is agent- or
package-writable -- an agent can edit config.json, and _do_agents_sync
copies description straight off a discovered agent spec -- so a value the
redactors would alter, or a non-string the loader let through, is replaced
WHOLESALE by _SENSITIVE_MASK, the sentinel _masked_config_dict already
uses for the same job on GET /api/config/kirocrew. Benign content is
byte-identical.
api_kirocrew_agent_update then drops any body field carrying that mask,
treating it as unchanged -- the remedy _masked_config_dict's docstring
prescribes verbatim. Without it the read half would destroy stored config:
the agents page seeds its edit sheet from a roster row and sends every
field on every save so that "" can clear a pin.
A FIXED sentinel rather than redacting in place is the load-bearing
choice. Recomputing the redaction to recognise the view breaks two ways a
sentinel does not: a second redaction chain over the same response (#8465)
produces a view the predicate no longer matches, and a stored value that
changes between the GET and the PUT makes the stale view read as a genuine
edit, writing redaction markers into the config. The sentinel depends on
neither. Cost, named: a value containing one credential-shaped token is
masked entirely rather than partially, the same trade the config endpoint
already makes.
Masking a non-string rather than coercing it to "" is what lets the write
rule PRESERVE it; an echoed "" would read as a genuine edit.
name is the single exemption, and only for the owner: it travels in the URL
rather than the body so the write rule cannot protect it, and masking it
would make /api/agents/{name} unaddressable. An app token cannot reach
those owner-gated routes, so name is masked there too. dict[str, str] is
now true rather than aspirational.
test_agents_roster_contract.py pins the exact key set at the endpoint for
both row sources, ratchets the allowlist against dataclasses.fields, and
covers both halves over HTTP including the stale-view case and that a
genuine edit still writes through.
Fixes #8454
… it cannot show
api_kirocrew_agents built each roster row with
{"name": name, "scope": ..., **dataclasses.asdict(agent_cfg)}, so the
endpoint's response contract was "every field KiroCrewAgentConfig has now,
plus every field anyone adds later", automatically. handlers/members.py
made the opposite call for GET /api/members and documented why: the
response is a network-boundary contract, and a spread ships a future
field -- internal bookkeeping, a filesystem path, a credential-shaped one --
to the browser by omission.
KEY half: both row sources (the cfg.agents rows and the project-scope
rows) now go through one _agent_roster_row allowlist naming 11 keys, so
the two cannot drift into different key sets either. Three record fields
are withheld, each with no consumer anywhere in website/src: the two
per-agent watchdog windows and the deprecated, inert telegram_account.
VALUE half, two co-operating halves. Every record value is agent- or
package-writable -- an agent can edit config.json, and _do_agents_sync
copies description straight off a discovered agent spec -- so a value the
redactors would alter, or a non-string the loader let through, is replaced
WHOLESALE by _SENSITIVE_MASK, the sentinel _masked_config_dict already
uses for the same job on GET /api/config/kirocrew. Benign content is
byte-identical.
api_kirocrew_agent_update then drops any body field carrying that mask,
treating it as unchanged -- the remedy _masked_config_dict's docstring
prescribes verbatim. Without it the read half would destroy stored config:
the agents page seeds its edit sheet from a roster row and sends every
field on every save so that "" can clear a pin.
A FIXED sentinel rather than redacting in place is the load-bearing
choice. Recomputing the redaction to recognise the view breaks two ways a
sentinel does not: a second redaction chain over the same response (#8465)
produces a view the predicate no longer matches, and a stored value that
changes between the GET and the PUT makes the stale view read as a genuine
edit, writing redaction markers into the config. The sentinel depends on
neither. Cost, named: a value containing one credential-shaped token is
masked entirely rather than partially, the same trade the config endpoint
already makes.
Masking a non-string rather than coercing it to "" is what lets the write
rule PRESERVE it; an echoed "" would read as a genuine edit.
name is the single exemption, and only for the owner: it travels in the URL
rather than the body so the write rule cannot protect it, and masking it
would make /api/agents/{name} unaddressable. An app token cannot reach
those owner-gated routes, so name is masked there too. dict[str, str] is
now true rather than aspirational.
test_agents_roster_contract.py pins the exact key set at the endpoint for
both row sources, ratchets the allowlist against dataclasses.fields, and
covers both halves over HTTP including the stale-view case and that a
genuine edit still writes through.
Fixes #8454
… it cannot show
api_kirocrew_agents built each roster row with
{"name": name, "scope": ..., **dataclasses.asdict(agent_cfg)}, so the
endpoint's response contract was "every field KiroCrewAgentConfig has now,
plus every field anyone adds later", automatically. handlers/members.py
made the opposite call for GET /api/members and documented why: the
response is a network-boundary contract, and a spread ships a future
field -- internal bookkeeping, a filesystem path, a credential-shaped one --
to the browser by omission.
KEY half: both row sources (the cfg.agents rows and the project-scope
rows) now go through one _agent_roster_row allowlist naming 11 keys, so
the two cannot drift into different key sets either. Three record fields
are withheld, each with no consumer anywhere in website/src: the two
per-agent watchdog windows and the deprecated, inert telegram_account.
VALUE half, two co-operating halves. Every record value is agent- or
package-writable -- an agent can edit config.json, and _do_agents_sync
copies description straight off a discovered agent spec -- so a value the
redactors would alter, or a non-string the loader let through, is replaced
WHOLESALE by _SENSITIVE_MASK, the sentinel _masked_config_dict already
uses for the same job on GET /api/config/kirocrew. Benign content is
byte-identical.
api_kirocrew_agent_update then drops any body field carrying that mask,
treating it as unchanged -- the remedy _masked_config_dict's docstring
prescribes verbatim. Without it the read half would destroy stored config:
the agents page seeds its edit sheet from a roster row and sends every
field on every save so that "" can clear a pin.
A FIXED sentinel rather than redacting in place is the load-bearing
choice. Recomputing the redaction to recognise the view breaks two ways a
sentinel does not: a second redaction chain over the same response (#8465)
produces a view the predicate no longer matches, and a stored value that
changes between the GET and the PUT makes the stale view read as a genuine
edit, writing redaction markers into the config. The sentinel depends on
neither. Cost, named: a value containing one credential-shaped token is
masked entirely rather than partially, the same trade the config endpoint
already makes.
Masking a non-string rather than coercing it to "" is what lets the write
rule PRESERVE it; an echoed "" would read as a genuine edit.
name is the single exemption, and only for the owner: it travels in the URL
rather than the body so the write rule cannot protect it, and masking it
would make /api/agents/{name} unaddressable. An app token cannot reach
those owner-gated routes, so name is masked there too. dict[str, str] is
now true rather than aspirational.
test_agents_roster_contract.py pins the exact key set at the endpoint for
both row sources, ratchets the allowlist against dataclasses.fields, and
covers both halves over HTTP including the stale-view case and that a
genuine edit still writes through.
Fixes #8454
… it cannot show
api_kirocrew_agents built each roster row with
{"name": name, "scope": ..., **dataclasses.asdict(agent_cfg)}, so the
endpoint's response contract was "every field KiroCrewAgentConfig has now,
plus every field anyone adds later", automatically. handlers/members.py
made the opposite call for GET /api/members and documented why: the
response is a network-boundary contract, and a spread ships a future
field -- internal bookkeeping, a filesystem path, a credential-shaped one --
to the browser by omission.
KEY half: both row sources (the cfg.agents rows and the project-scope
rows) now go through one _agent_roster_row allowlist naming 11 keys, so
the two cannot drift into different key sets either. Three record fields
are withheld, each with no consumer anywhere in website/src: the two
per-agent watchdog windows and the deprecated, inert telegram_account.
VALUE half, two co-operating halves. Every record value is agent- or
package-writable -- an agent can edit config.json, and _do_agents_sync
copies description straight off a discovered agent spec -- so a value the
redactors would alter, or a non-string the loader let through, is replaced
WHOLESALE by _SENSITIVE_MASK, the sentinel _masked_config_dict already
uses for the same job on GET /api/config/kirocrew. Benign content is
byte-identical.
api_kirocrew_agent_update then drops any body field carrying that mask,
treating it as unchanged -- the remedy _masked_config_dict's docstring
prescribes verbatim. Without it the read half would destroy stored config:
the agents page seeds its edit sheet from a roster row and sends every
field on every save so that "" can clear a pin.
A FIXED sentinel rather than redacting in place is the load-bearing
choice. Recomputing the redaction to recognise the view breaks two ways a
sentinel does not: a second redaction chain over the same response (#8465)
produces a view the predicate no longer matches, and a stored value that
changes between the GET and the PUT makes the stale view read as a genuine
edit, writing redaction markers into the config. The sentinel depends on
neither. Cost, named: a value containing one credential-shaped token is
masked entirely rather than partially, the same trade the config endpoint
already makes.
Masking a non-string rather than coercing it to "" is what lets the write
rule PRESERVE it; an echoed "" would read as a genuine edit.
name is the single exemption, and only for the owner: it travels in the URL
rather than the body so the write rule cannot protect it, and masking it
would make /api/agents/{name} unaddressable. An app token cannot reach
those owner-gated routes, so name is masked there too. dict[str, str] is
now true rather than aspirational.
test_agents_roster_contract.py pins the exact key set at the endpoint for
both row sources, ratchets the allowlist against dataclasses.fields, and
covers both halves over HTTP including the stale-view case and that a
genuine edit still writes through.
Fixes #8454
github-actions
Bot
added
the
merge conflict
… it cannot show
api_kirocrew_agents built each roster row with
{"name": name, "scope": ..., **dataclasses.asdict(agent_cfg)}, so the
endpoint's response contract was "every field KiroCrewAgentConfig has now,
plus every field anyone adds later", automatically. handlers/members.py
made the opposite call for GET /api/members and documented why: the
response is a network-boundary contract, and a spread ships a future
field -- internal bookkeeping, a filesystem path, a credential-shaped one --
to the browser by omission.
KEY half: both row sources (the cfg.agents rows and the project-scope
rows) now go through one _agent_roster_row allowlist naming 11 keys, so
the two cannot drift into different key sets either. Three record fields
are withheld, each with no consumer anywhere in website/src: the two
per-agent watchdog windows and the deprecated, inert telegram_account.
VALUE half, two co-operating halves. Every record value is agent- or
package-writable -- an agent can edit config.json, and _do_agents_sync
copies description straight off a discovered agent spec -- so a value the
redactors would alter, or a non-string the loader let through, is replaced
WHOLESALE by _SENSITIVE_MASK, the sentinel _masked_config_dict already
uses for the same job on GET /api/config/kirocrew. Benign content is
byte-identical.
api_kirocrew_agent_update then drops any body field carrying that mask,
treating it as unchanged -- the remedy _masked_config_dict's docstring
prescribes verbatim. Without it the read half would destroy stored config:
the agents page seeds its edit sheet from a roster row and sends every
field on every save so that "" can clear a pin.
A FIXED sentinel rather than redacting in place is the load-bearing
choice. Recomputing the redaction to recognise the view breaks two ways a
sentinel does not: a second redaction chain over the same response (#8465)
produces a view the predicate no longer matches, and a stored value that
changes between the GET and the PUT makes the stale view read as a genuine
edit, writing redaction markers into the config. The sentinel depends on
neither. Cost, named: a value containing one credential-shaped token is
masked entirely rather than partially, the same trade the config endpoint
already makes.
Masking a non-string rather than coercing it to "" is what lets the write
rule PRESERVE it; an echoed "" would read as a genuine edit.
name is the single exemption, and only for the owner: it travels in the URL
rather than the body so the write rule cannot protect it, and masking it
would make /api/agents/{name} unaddressable. An app token cannot reach
those owner-gated routes, so name is masked there too. dict[str, str] is
now true rather than aspirational.
test_agents_roster_contract.py pins the exact key set at the endpoint for
both row sources, ratchets the allowlist against dataclasses.fields, and
covers both halves over HTTP including the stale-view case and that a
genuine edit still writes through.
Fixes #8454
… it cannot show
api_kirocrew_agents built each roster row with
{"name": name, "scope": ..., **dataclasses.asdict(agent_cfg)}, so the
endpoint's response contract was "every field KiroCrewAgentConfig has now,
plus every field anyone adds later", automatically. handlers/members.py
made the opposite call for GET /api/members and documented why: the
response is a network-boundary contract, and a spread ships a future
field -- internal bookkeeping, a filesystem path, a credential-shaped one --
to the browser by omission.
KEY half: both row sources (the cfg.agents rows and the project-scope
rows) now go through one _agent_roster_row allowlist naming 11 keys, so
the two cannot drift into different key sets either. Three record fields
are withheld, each with no consumer anywhere in website/src: the two
per-agent watchdog windows and the deprecated, inert telegram_account.
VALUE half, two co-operating halves. Every record value is agent- or
package-writable -- an agent can edit config.json, and _do_agents_sync
copies description straight off a discovered agent spec -- so a value the
redactors would alter, or a non-string the loader let through, is replaced
WHOLESALE by _SENSITIVE_MASK, the sentinel _masked_config_dict already
uses for the same job on GET /api/config/kirocrew. Benign content is
byte-identical.
api_kirocrew_agent_update then drops any body field carrying that mask,
treating it as unchanged -- the remedy _masked_config_dict's docstring
prescribes verbatim. Without it the read half would destroy stored config:
the agents page seeds its edit sheet from a roster row and sends every
field on every save so that "" can clear a pin.
A FIXED sentinel rather than redacting in place is the load-bearing
choice. Recomputing the redaction to recognise the view breaks two ways a
sentinel does not: a second redaction chain over the same response (#8465)
produces a view the predicate no longer matches, and a stored value that
changes between the GET and the PUT makes the stale view read as a genuine
edit, writing redaction markers into the config. The sentinel depends on
neither. Cost, named: a value containing one credential-shaped token is
masked entirely rather than partially, the same trade the config endpoint
already makes.
Masking a non-string rather than coercing it to "" is what lets the write
rule PRESERVE it; an echoed "" would read as a genuine edit.
name is the single exemption, and only for the owner: it travels in the URL
rather than the body so the write rule cannot protect it, and masking it
would make /api/agents/{name} unaddressable. An app token cannot reach
those owner-gated routes, so name is masked there too. dict[str, str] is
now true rather than aspirational.
test_agents_roster_contract.py pins the exact key set at the endpoint for
both row sources, ratchets the allowlist against dataclasses.fields, and
covers both halves over HTTP including the stale-view case and that a
genuine edit still writes through.
Fixes #8454
… it cannot show
api_kirocrew_agents built each roster row with
{"name": name, "scope": ..., **dataclasses.asdict(agent_cfg)}, so the
endpoint's response contract was "every field KiroCrewAgentConfig has now,
plus every field anyone adds later", automatically. handlers/members.py
made the opposite call for GET /api/members and documented why: the
response is a network-boundary contract, and a spread ships a future
field -- internal bookkeeping, a filesystem path, a credential-shaped one --
to the browser by omission.
KEY half: both row sources (the cfg.agents rows and the project-scope
rows) now go through one _agent_roster_row allowlist naming 11 keys, so
the two cannot drift into different key sets either. Three record fields
are withheld, each with no consumer anywhere in website/src: the two
per-agent watchdog windows and the deprecated, inert telegram_account.
VALUE half, two co-operating halves. Every record value is agent- or
package-writable -- an agent can edit config.json, and _do_agents_sync
copies description straight off a discovered agent spec -- so a value the
redactors would alter, or a non-string the loader let through, is replaced
WHOLESALE by _SENSITIVE_MASK, the sentinel _masked_config_dict already
uses for the same job on GET /api/config/kirocrew. Benign content is
byte-identical.
api_kirocrew_agent_update then drops any body field carrying that mask,
treating it as unchanged -- the remedy _masked_config_dict's docstring
prescribes verbatim. Without it the read half would destroy stored config:
the agents page seeds its edit sheet from a roster row and sends every
field on every save so that "" can clear a pin.
A FIXED sentinel rather than redacting in place is the load-bearing
choice. Recomputing the redaction to recognise the view breaks two ways a
sentinel does not: a second redaction chain over the same response (#8465)
produces a view the predicate no longer matches, and a stored value that
changes between the GET and the PUT makes the stale view read as a genuine
edit, writing redaction markers into the config. The sentinel depends on
neither. Cost, named: a value containing one credential-shaped token is
masked entirely rather than partially, the same trade the config endpoint
already makes.
Masking a non-string rather than coercing it to "" is what lets the write
rule PRESERVE it; an echoed "" would read as a genuine edit.
name is the single exemption, and only for the owner: it travels in the URL
rather than the body so the write rule cannot protect it, and masking it
would make /api/agents/{name} unaddressable. An app token cannot reach
those owner-gated routes, so name is masked there too. dict[str, str] is
now true rather than aspirational.
test_agents_roster_contract.py pins the exact key set at the endpoint for
both row sources, ratchets the allowlist against dataclasses.fields, and
covers both halves over HTTP including the stale-view case and that a
genuine edit still writes through.
Fixes #8454
… it cannot show
api_kirocrew_agents built each roster row with
{"name": name, "scope": ..., **dataclasses.asdict(agent_cfg)}, so the
endpoint's response contract was "every field KiroCrewAgentConfig has now,
plus every field anyone adds later", automatically. handlers/members.py
made the opposite call for GET /api/members and documented why: the
response is a network-boundary contract, and a spread ships a future
field -- internal bookkeeping, a filesystem path, a credential-shaped one --
to the browser by omission.
KEY half: both row sources (the cfg.agents rows and the project-scope
rows) now go through one _agent_roster_row allowlist naming 11 keys, so
the two cannot drift into different key sets either. Three record fields
are withheld, each with no consumer anywhere in website/src: the two
per-agent watchdog windows and the deprecated, inert telegram_account.
VALUE half, two co-operating halves. Every record value is agent- or
package-writable -- an agent can edit config.json, and _do_agents_sync
copies description straight off a discovered agent spec -- so a value the
redactors would alter, or a non-string the loader let through, is replaced
WHOLESALE by _SENSITIVE_MASK, the sentinel _masked_config_dict already
uses for the same job on GET /api/config/kirocrew. Benign content is
byte-identical.
api_kirocrew_agent_update then drops any body field carrying that mask,
treating it as unchanged -- the remedy _masked_config_dict's docstring
prescribes verbatim. Without it the read half would destroy stored config:
the agents page seeds its edit sheet from a roster row and sends every
field on every save so that "" can clear a pin.
A FIXED sentinel rather than redacting in place is the load-bearing
choice. Recomputing the redaction to recognise the view breaks two ways a
sentinel does not: a second redaction chain over the same response (#8465)
produces a view the predicate no longer matches, and a stored value that
changes between the GET and the PUT makes the stale view read as a genuine
edit, writing redaction markers into the config. The sentinel depends on
neither. Cost, named: a value containing one credential-shaped token is
masked entirely rather than partially, the same trade the config endpoint
already makes.
Masking a non-string rather than coercing it to "" is what lets the write
rule PRESERVE it; an echoed "" would read as a genuine edit.
name is the single exemption, and only for the owner: it travels in the URL
rather than the body so the write rule cannot protect it, and masking it
would make /api/agents/{name} unaddressable. An app token cannot reach
those owner-gated routes, so name is masked there too. dict[str, str] is
now true rather than aspirational.
test_agents_roster_contract.py pins the exact key set at the endpoint for
both row sources, ratchets the allowlist against dataclasses.fields, and
covers both halves over HTTP including the stale-view case and that a
genuine edit still writes through.
Fixes #8454
… it cannot show (#8472) api_kirocrew_agents built each roster row with {"name": name, "scope": ..., **dataclasses.asdict(agent_cfg)}, so the endpoint's response contract was "every field KiroCrewAgentConfig has now, plus every field anyone adds later", automatically. handlers/members.py made the opposite call for GET /api/members and documented why: the response is a network-boundary contract, and a spread ships a future field -- internal bookkeeping, a filesystem path, a credential-shaped one -- to the browser by omission. KEY half: both row sources (the cfg.agents rows and the project-scope rows) now go through one _agent_roster_row allowlist naming 11 keys, so the two cannot drift into different key sets either. Three record fields are withheld, each with no consumer anywhere in website/src: the two per-agent watchdog windows and the deprecated, inert telegram_account. VALUE half, two co-operating halves. Every record value is agent- or package-writable -- an agent can edit config.json, and _do_agents_sync copies description straight off a discovered agent spec -- so a value the redactors would alter, or a non-string the loader let through, is replaced WHOLESALE by _SENSITIVE_MASK, the sentinel _masked_config_dict already uses for the same job on GET /api/config/kirocrew. Benign content is byte-identical. api_kirocrew_agent_update then drops any body field carrying that mask, treating it as unchanged -- the remedy _masked_config_dict's docstring prescribes verbatim. Without it the read half would destroy stored config: the agents page seeds its edit sheet from a roster row and sends every field on every save so that "" can clear a pin. A FIXED sentinel rather than redacting in place is the load-bearing choice. Recomputing the redaction to recognise the view breaks two ways a sentinel does not: a second redaction chain over the same response (#8465) produces a view the predicate no longer matches, and a stored value that changes between the GET and the PUT makes the stale view read as a genuine edit, writing redaction markers into the config. The sentinel depends on neither. Cost, named: a value containing one credential-shaped token is masked entirely rather than partially, the same trade the config endpoint already makes. Masking a non-string rather than coercing it to "" is what lets the write rule PRESERVE it; an echoed "" would read as a genuine edit. name is the single exemption, and only for the owner: it travels in the URL rather than the body so the write rule cannot protect it, and masking it would make /api/agents/{name} unaddressable. An app token cannot reach those owner-gated routes, so name is masked there too. dict[str, str] is now true rather than aspirational. test_agents_roster_contract.py pins the exact key set at the endpoint for both row sources, ratchets the allowlist against dataclasses.fields, and covers both halves over HTTP including the stale-view case and that a genuine edit still writes through. Fixes #8454 Co-authored-by: gh-autofix#2887 <chenmingwei23@users.noreply.github.com>
chenmingwei23
added
needs-author-decision
|
🤖 Kiro Crew [operator: chenmingwei23#de330d0c]: This PR has been inactive for 7+ days. I reviewed the blockers but they require your input:
When you have addressed these, the pipeline will re-assess on its next cycle. |
|
@dwu96 thank you for this, and please keep it open, but it needs a hard rescope. Audited at a56bb4b. Already on main. Merged #8472 covers the GET /api/agents half. Still missing on main. Ask. Please narrow this PR to those two items and drop Overlap with #8497 (cc @xuejinT): #8497 deletes the Session Color editor in Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong. |
5 participants
Problem / Motivation
Free-text string fields on the crew/agent record —
description,triggers,workspace,memory_store,model,session_color, … (KiroCrewAgentConfig,config/sections.py) — are agent-writable throughconfig.json, and two dashboard endpoints echo that record back to the browser without any redaction pass:GET /api/agents(agents.py::api_kirocrew_agents) builds each row as{"name": name, "scope": "global", **dataclasses.asdict(agent_cfg)}and spreadsdataclasses.asdict(KiroCrewAgentConfig())for project-scope rows — the spread ships every field the record has now and every field added later, unredacted.GET /api/members(members.py::api_members) uses a deliberate explicit allowlist (name,slug,kiro_agent,workspace,memory_store,model) but applies no redaction to the values; its own docstring recorded the omission ("limited to what costs no IO and no redaction pass").A short secret written into any such field — e.g. a 20-character AWS access key id, well under any preview truncation — reaches dashboard JSON verbatim.
Scope note: the issue as filed frames this around
avatar.traits/_safe_avatar. Those symbols do not exist on main — they arrive with the still-open PR #7443. On main,avataris a singlestrpath field. This PR therefore implements the issue's recommended option (1), a field-generic chokepoint over the serialized record's string values, which covers the trait axes automatically the moment #7443 lands, with no second patch there. The issue's option (2) (harden_safe_avatar) is out of scope not merely unpreferred:_safe_avataris not a function that exists on main.Why it matters
This is the same defect class as #6189/#8132 (dashboard egress without the credential/exfiltration-URL chain), on a surface every dashboard page loads: the agent roster. An agent that can write
config.json— which is the normal, supported way agents are defined — can park a credential or an exfiltration URL in a free-text field and have the dashboard serve it to the browser, bypassing the redaction posture the rest of the output surfaces enforce.What changed (motivation → approach → change)
Symptom → root cause: both endpoints serialize the agent record straight into
web.json_responsewith no redaction pass over the record's string values.Approach: one field-generic serialization chokepoint per endpoint, not per-field patches — a per-field fix would silently stop covering the record the next time a field is added (exactly the situation #7443 is about to create). The in-tree precedent followed is
_shared.py::_redact_memory_field, the shared recursive scrubber already used bymemory.pyandcron.py, which applies the exact chainmembers.py's own_sanitizeuses:redact_exfiltration_urlsthenredact_credentials.Change:
dashboard/handlers/_shared.py— newredact_record_strings(record) -> dict: applies the chain to everystrvalue of a serialized record, recursing into nested dicts/lists by delegating to_redact_memory_field. Non-string values pass through untouched.agents.py::api_kirocrew_agents— every row funnels through the helper at the response, after the usage sort (ordering logic untouched, and it reads the raw names). This single point covers BOTH row sources: thecfg.agentsspread and the project-scope default rows.members.py::api_members— the allowlisted record dict funnels through the helper at construction. The explicit allowlist stays — it is a deliberate network-boundary contract with its own comment; only the values are scrubbed. The transcriptlast_messagepreview keeps its existing pre-truncation_sanitizepass unchanged (already correct; not double-wrapped).docs/system-specs/modules/security.md— new "Crew roster serializers" bullet stating exactly which two endpoints are covered (and explicitly that the wider config surface is not).security_posture.py— comment-accuracy updates only on the existingNON_EGRESS_REDACTION_MODULESentries for_shared.py/agents.py/members.py(all three were already classified; the drift guard passes unchanged).Deliberately NOT done in this PR: converting
/api/agents' spread into an allowlist.members.py's comment argues for one, but that is a separate contract change with frontend ripple — filed as a sibling issue instead (see Related Issues).Overlap note (sequencing): PR #7443 touches both handlers (it adds the avatar trait axes this issue was carved out of); this PR does not depend on it and does not reference its symbols — the field-generic chokepoint is what lets the two compose in either merge order. PR #7235 and #8307 append new endpoints to these files and do not rewrite either function changed here.
Tests
test/test_roster_record_redaction.py— all red-first against unmodified main (8 of 9 failed; the 9th is the allowlist-contract preservation test, which passes on main by design):/api/agents: credential indescriptionredacted; exfiltration URL inworkspaceredacted. The two planted shapes are chosen so each isolates one half of the chain (a bare AWS key id is invisible to the exfil half; a long-query URL with no credential marker is invisible to the credential half) — a fix wiring only one half fails the other test./api/agentsproject-scope rows: a poisoned-defaults stand-in proves the second row source goes through the chokepoint (today's defaults are benign constants, so this is the only way to observe that row source)./api/members: credential inworkspaceredacted; exfiltration URL inmemory_storeredacted; allowlist contract preserved (non-allowlisted fields still do not ship at all).Mutation checks — five mutants, each applied individually, each killed by exactly the expected tests, tree restored (cp-aside/cp-back, never
git checkout), suite re-verified green after each restore:redact_exfiltration_urlshalfredact_credentialshalf/api/memberschokepoint (cover/api/agentsonly)scope=="global"rows on/api/agentsRed-first + mutants together prove both directions: the tests catch the bug, and each guard is individually load-bearing.
Neighbors:
test_members_dm_thread.py,test_api_agents_order.py,test_security_posture.py(incl. the redactor call-site drift guard),test_config_api.py,test_agents_endpoints_owner_auth.py— 169 passed.Gates:
scripts/local-gate.py --base origin/main— backend-only diff plan: full backend suite + 206 frontend guard specs. Backend (branch): 236 failed + 2 errors out of 84,773 passed — the known environmental baseline (see below). Frontend guards: 208 files / 5,702 tests, all passed (run under node 22 per the gate plan). black (26.3.1, CI pin) / isort / flake8 / mypy clean on every touched file; thetest_security_posture.pyredactor call-site drift guard passes unchanged.Zero-regression proof: full backend suite on this branch vs a pristine
git worktreeatorigin/main(dabd83e91, the same base commit): branch 236 failed + 2 errors, main 236 failed + 2 errors, and the sorted failing-test id sets are byte-identical in both directions (238 = 238 ids, empty symmetric difference). This repo has a known environmental failure baseline, so identical sets — not "0 failures" — is the proof.Screenshots / video
Why no screenshot: the frontend change alters only the SAVE PAYLOAD contents (untouched redacted prefills are omitted from the PUT body) - no component, layout, or rendered pixel changes; the evidence is the redactedPrefillGuard unit tests and the backend round-trip tests.
Manual verification
N/A — unit coverage sufficient: both endpoints are exercised end-to-end through aiohttp
TestClient(real routing, middleware, and JSON serialization), asserting on the raw response body.Related Issues
Fixes #8447
Pattern harvest
Rule candidate: review-prompt
Pattern: "a dashboard endpoint that serializes an agent-writable record (dataclass spread OR allowlist) must funnel the serialized values through the credential + exfiltration-URL chain at one chokepoint; a spread without one ships every future field unredacted". Knowingly out-of-scope sibling surface:
/api/agents' spread-vs-allowlist contract itself (filed separately, see Related Issues).Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)