Harden shared agent instruction review - #4220
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9536a5673
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Requesting changes on Wes's behalf after independent security, regression, and integration reviews of d56523fcbab119e78cc2a208c0b78cae55dfe4b3.
Two blockers remain:
- The catalog accepts unverified relay events as executable definitions and attributes them to attacker-selected pubkeys. A compromised relay can forge a member's shared agent and prompt a user to import arbitrary instructions.
- The validator rejects legitimate VS16/ZWJ emoji sequences, breaking edits to existing local agents and hiding valid shared definitions from the catalog.
The literal <pre> review surface, Rust ingress validation, snapshot/team-snapshot coverage, pre-retention validation, and TypeScript/Rust range parity otherwise look sound. Existing CI at this exact head is green; reviewers did not duplicate CI-equivalent suites locally.
wesbillman
left a comment
There was a problem hiding this comment.
Requesting changes on Wes's behalf after coordinated review of fc53800ef0d849a127d850598f35b9d2d867c099 with Princess Donut and Mongo.
The two prior blockers are fixed: catalog events are now verified on fresh wire-shaped objects before they can claim coordinates or influence paging, and the TS/Rust validators permit contextual VS16/ZWJ emoji while rejecting detached formatting. The literal review surface and catalog add path preserve exact prompt bytes. Mongo independently found no remaining issue in those paths.
One blocker remains: the Rust safety invariant is incomplete for other executable, relay-synced definition text.
- Definition-less
ManagedAgentRecord.system_promptis executable, butcreate_managed_agentandupdate_managed_agentaccept it withoutvalidate_agent_definition_text; those records are persisted and queued for publication. Inbound definition-less records do get validated, leaving an asymmetric local bypass. - Team instructions are executable, but team create/update and inbound kind
30176apply them without the validator. Team snapshot validation validates member definitions but not the executableteam.instructions(or the team name), then import persists those fields.
As a result, invisible/bidi text such as Review\u{200B}code can still be persisted, executed, and published through local managed-agent/team paths even though equivalent persona and inbound managed-agent definitions are rejected. Please centralize validation for executable name/instruction fields across create, update, import, sync, and publication boundaries, with direct regressions for definition-less agents and teams—or explicitly narrow the promised security contract and demonstrate why these executable fields are outside it.
Existing CI is green at this exact head. Focused local test attempts in my clean review worktree were environment-blocked (missing JS dependencies; missing Tauri sidecar), so I did not count them as validation. Mongo separately ran the focused TS suites: 44/44 passed.
|
Scope clarification on my changes-requested review, on Wes's behalf: I agree the team-instructions work can be split into a follow-up. It is executable shared text and the gap is real, but this PR's concrete review surface is agent/persona definitions; extending the invariant to teams broadens the product/UI contract enough to justify separate work. Please track that follow-up before merge. I do not think the definition-less managed-agent gap is separable. This PR explicitly added inbound validation for definition-less kind So the narrowed blocker is: validate definition-less managed-agent name/prompt on local create/update (and at publication as defense in depth), with direct regressions. Team validation may follow separately with an issue/PR. |
Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
3858539 to
e6aa358
Compare
Summary
Security rationale
Shared system prompts are executable configuration. Previously, catalog prompts were projected through the chat Markdown renderer, which could hide text, replace link destinations with benign labels, and turn image syntax into remote loads. Zero-width and bidirectional controls could also make reviewed text differ from what the agent executes.
This change establishes a review invariant: the prompt a user sees is the prompt the agent executes. Definitions that cannot be reviewed faithfully are rejected rather than rewritten. Catalog events must also pass Nostr ID/signature verification before they can claim a publisher, coordinate, or cursor.
What changed
Validation reports the offending code point and never silently removes it.
E2E recording
buzz-shared-agent-security-e2e.webm
The recording demonstrates:
U+200Bis rejectedU+202Eis rejectedVerification
Passed locally:
just test: all 10 unit and Docker-backed integration stagesinbox-editimmediate-attachment smoke test after rebase: 1 testThe branch was rebased onto current
main, which includes the upstream attachment-button label fix. Fresh post-rebase GitHub CI is green for every required and selected check: Desktop Core, all four Desktop Smoke E2E shards, both Desktop E2E Integration shards and their aggregate, Desktop E2E Relay, Desktop Build (macOS), Windows Rust, Rust Lint, DCO, security scanners, and Desktop Release Candidate. The previously failingDesktop Smoke E2E (3)shard now passes.The repository-wide desktop check also reports existing CSS formatting/
!importantfindings incomponents.cssandterminal.css; neither file is changed by this PR. GitHub's Desktop Core lint and format stage passes on the rebased branch.