fix(cli): refuse a profile republish that would drop the owner attestation - #5751
fix(cli): refuse a profile republish that would drop the owner attestation#5751Cynthia427 wants to merge 1 commit into
Conversation
wolfyy970
left a comment
There was a problem hiding this comment.
The fail-closed behavior is right: a profile update must not silently remove an existing owner attestation.
I think this needs two small changes before it is mergeable:
- Please restack the
941ec0bchange alone on current main. This branch also carries three older Desktop/runtime commits and is currently dirty, which turns a narrow CLI safety fix into an unrelated merge. - Please test the real
cmd_set_profileseam, not onlyevent_has_auth_tag: an authenticated current profile with no configured attestation must submit nothing; supplying a valid attestation must publish one replacement with the newauthtag; and an unattested profile must remain editable.
That production test is the important proof here. It verifies the live query, guard, signing path and absence of a relay write together.
…ation kind:0 is replaceable, so a profile write replaces the stored event whole. cmd_set_profile already merges CONTENT, but the NIP-OA auth tag is not a content field: it is re-attached only by sign_event from BUZZ_AUTH_TAG, and sign_event deliberately forbids callers supplying one (it is a signed attestation, not a value to copy forward). So running set-profile without BUZZ_AUTH_TAG against a profile that HAS the tag destroys it, silently and successfully. That is not hypothetical. On 2026-08-12 an avatar re-host republished eight agent profiles this way. The tag vanished, check_sibling_via_profile then returned false for every agent, and author_allowed dropped every agent-to-agent message for ~23 hours. Nothing failed loudly: the writes succeeded, units stayed active, heartbeats fired, channels stayed subscribed. Only the owner could still reach the agents, because the owner is matched by pubkey and never consults the profile — which made a fleet-wide fault look like one agent ignoring one sender. Carrying the old tag forward would be the wrong fix and is already forbidden: an attestation must be re-issued, not copied. So fail closed instead — tell the operator exactly what the write would destroy and how to supply it. Tested at the cmd_set_profile seam against a fake relay, so the live query → guard → sign → submit chain is exercised rather than the predicate alone, with the relay's received events as the evidence: - an attested profile with no configured attestation submits NOTHING (a refusal raised after the write would already have destroyed the tag) - supplying the attestation publishes exactly one replacement that still carries the auth tag, and preserves unspecified content fields — the incident began as an avatar rewrite that blanked `picture` - an unattested profile stays editable, so the guard cannot become a blanket block Mutation-verified: disabling the guard fails the first test and only that one. event_has_auth_tag is also kept pure and unit-tested, including the exact shape the incident left behind (content intact, tags emptied). Signed-off-by: Cynthia Rohr <cynthia.r@kreativreason.co>
941ec0b to
a54bf43
Compare
|
Both addressed — thank you, the second point was the right call. 1. Restacked. 2. Tested at the
Mutation-verified: stubbing the guard to One extra fix while in here: the change had orphaned
|
wolfyy970
left a comment
There was a problem hiding this comment.
Re-reviewed at a54bf43. The branch is now the single CLI fix, and the new tests cross the real query, guard, signing and submission path. They prove the refusal writes nothing, a verified configured attestation survives replacement, and unattested profiles remain editable. I ran the focused tests and strict clippy against this exact head; both pass.
Approved.
Problem
kind:0is replaceable, so a profile write replaces the stored event whole.cmd_set_profilealready merges content. But the NIP-OAauthtag is not a content field — it is re-attached only bysign_eventfromBUZZ_AUTH_TAG, andsign_eventdeliberately refuses a caller-supplied one:So running
set-profilewithoutBUZZ_AUTH_TAGagainst a profile that has the tag destroys it — silently, with a successful write.Not hypothetical
On 2026-08-12 an avatar re-host republished eight agent profiles this way (
display_name+picture, noBUZZ_AUTH_TAG). Visible in the relay DB astags_len214 → 2 across all eight in the same minute.check_sibling_via_profileproves same-owner siblinghood by finding that tag. With it gone it returned false for every agent, soauthor_alloweddropped every agent-to-agent message for ~23 hours.Nothing failed loudly: writes succeeded, units stayed
active, heartbeats fired, channels stayed subscribed. Only the owner could still reach the agents, because the owner is matched by pubkey and never consults the profile. A fleet-wide fault therefore presented as "one agent ignoring one sender", which sent the operators looking in entirely the wrong place.Why not carry the tag forward
That would be the wrong fix, and
sign_eventalready forbids it. An attestation is a signature over a claim — it must be re-issued, not copied by whoever happens to be writing a profile. Preserving it blindly would weaken the invariant that auth tags originate only from the configured attestation.The fix: fail closed
If the live profile carries an
authtag andBUZZ_AUTH_TAGis unset, refuse and say what would be destroyed:No behaviour change when
BUZZ_AUTH_TAGis set, or when the profile has no tag.Tests
event_has_auth_tagextracted as a pure function so the decision is testable, matching the existingpresence_subjectstyle in this module. 4 tests including the exact shape the incident left behind (content intact,tags: []), plus malformed-tag and missing-tagscases.Mutation-verified: changing the match to a non-
authliteral fails two of them.Disclosed limitation: the guard's call site is not covered —
cmd_set_profileneeds a live client and this module has no client harness, so the wiring is verified by inspection rather than by test. Happy to add a harness if you'd prefer that before merge.cargo test -p buzz-cli --lib: 256 passed, 0 failed.