diff --git a/crates/buzz-acp/src/engram_fetch.rs b/crates/buzz-acp/src/engram_fetch.rs index 534d05837c0..4404e05b996 100644 --- a/crates/buzz-acp/src/engram_fetch.rs +++ b/crates/buzz-acp/src/engram_fetch.rs @@ -67,7 +67,8 @@ async fn fetch_core_body( agent_keys: &Keys, owner: &PublicKey, ) -> Result, String> { - let k_c = conversation_key(agent_keys.secret_key(), owner); + let k_c = conversation_key(agent_keys.secret_key(), owner) + .map_err(|e| format!("conversation key derivation failed: {e}"))?; let d = d_tag(&k_c, buzz_core::engram::CORE_SLUG); let filter = nostr::Filter::new() diff --git a/crates/buzz-cli/README.md b/crates/buzz-cli/README.md index a2dcdce6d21..c46b127ac7b 100644 --- a/crates/buzz-cli/README.md +++ b/crates/buzz-cli/README.md @@ -98,6 +98,15 @@ buzz channels list | jq '.[].name' constraint omitted from the command is removed. `protect list` reports malformed stored rules in `validation_error` so an owner can remove and repair them. +`mem patch` checks the value it is *replacing*, not the replacement. +`--base-hash ` (from `buzz mem hash `) refuses the write with exit 5 +if another writer moved the head first, and hunk context and `-` lines must +match the current value verbatim at their declared line numbers. Neither check +looks at the `+` lines: a patch that quotes the current value correctly can +insert content from anywhere and will be accepted. To check what actually +landed, use `--dry-run` (prints the sha256 that would be written) and re-read +the slug after writing. + ## Commands | Group | Subcommand | Description | @@ -164,7 +173,7 @@ stored rules in `validation_error` so an owner can remove and repair them. | | `get` | Print memory value to stdout | | | `hash` | Print SHA-256 hex of memory value | | | `set` | Write a memory value (use `-` for stdin) | -| | `patch` | Apply unified diff to memory value | +| | `patch` | Apply unified diff to memory value (checks the preimage only) | | | `rm` | Publish a tombstone to delete memory | ## Architecture diff --git a/crates/buzz-cli/src/commands/mem.rs b/crates/buzz-cli/src/commands/mem.rs index eb15921bd4c..3651cfaab9a 100644 --- a/crates/buzz-cli/src/commands/mem.rs +++ b/crates/buzz-cli/src/commands/mem.rs @@ -28,12 +28,32 @@ use nostr::PublicKey; use crate::client::BuzzClient; use crate::error::CliError; +/// Parse a pubkey supplied on the command line, rejecting keys that are not +/// points on the secp256k1 curve. +/// +/// `PublicKey::from_hex` only hex-decodes 32 bytes, so an x-coordinate with no +/// corresponding curve point (e.g. `00…05`) parses successfully and only fails +/// later, inside NIP-44 ECDH. Validating here keeps that failure a `Usage` +/// error naming the bad flag instead of an error surfaced from deep in the +/// engram layer. Same shape as `buzz_core::private_managed_agent`'s +/// `parse_canonical_pubkey`, which curve-checks with `xonly()` for the same +/// reason. +fn parse_pubkey_flag(flag: &str, value: &str) -> Result { + let key = PublicKey::from_hex(value) + .map_err(|e| CliError::Usage(format!("{flag} must be a 64-hex pubkey: {e}")))?; + key.xonly().map_err(|e| { + CliError::Usage(format!( + "{flag} is not a point on the secp256k1 curve: {value} ({e})" + )) + })?; + Ok(key) +} + /// Resolve the agent's owner pubkey: explicit `--owner` flag wins, otherwise /// fall back to the NIP-OA `auth_tag` (which carries owner pubkey in slot 1). fn resolve_owner(client: &BuzzClient, owner_flag: Option<&str>) -> Result { if let Some(s) = owner_flag { - return PublicKey::from_hex(s) - .map_err(|e| CliError::Usage(format!("--owner must be a 64-hex pubkey: {e}"))); + return parse_pubkey_flag("--owner", s); } let tag = client.auth_tag_owner_hex().ok_or_else(|| { CliError::Usage( @@ -41,8 +61,16 @@ fn resolve_owner(client: &BuzzClient, owner_flag: Option<&str>) -> Result String { /// the hunk — at which point regenerating the patch is the correct response, /// not silently landing the change at a different position. /// +/// **Scope: the preimage only.** `Line::Insert` is filtered out below, so the +/// replacement text is never examined. A patch whose context and deletions +/// quote the current value exactly, but whose insertions are unrelated +/// content, satisfies this function. It is a positional-integrity check on +/// what is being replaced, not an authenticity check on what replaces it — +/// see `tests::strict_position_authenticates_preimage_only_not_postimage`. +/// /// Returns `Ok(())` on a clean match, `Err(message)` otherwise. /// /// Line-number convention: unified-diff `@@ -N,M @@` uses 1-based line @@ -526,14 +561,30 @@ pub async fn cmd_hash( /// content fuzz; diffy will refuse a hunk whose context lines don't match /// the file verbatim), and writes the result. /// -/// Safety properties: +/// Safety properties — all of them about the value being *replaced*: /// - `--base-hash ` is **required** unless `--no-base-hash` is passed. -/// This makes concurrent edits safe: if the slug has changed since the -/// patch was generated, the write is refused. +/// It proves the slug still holds the value the patch was generated +/// against: if another writer moved the head first, the write is refused +/// with `Conflict` (exit 5). +/// - Hunk context and deletions must match the current value verbatim at the +/// declared line numbers (see [`verify_hunks_at_declared_position`]). /// - The result is rejected if it would be empty, unless `--allow-empty`. /// - `--dry-run` prints the post-application diff and exits without writing. /// - On a successful write, the new sha256 is printed to stderr so callers /// can chain edits. +/// +/// **What these checks do NOT do.** They authenticate the *preimage* only. +/// `verify_hunks_at_declared_position` filters `Line::Insert` out before +/// comparing, and `--base-hash` hashes the pre-edit value — so a patch whose +/// context and `-` lines quote the real current value while its `+` lines +/// carry content from somewhere else entirely passes every check here and is +/// written. Pinned by +/// `tests::strict_position_authenticates_preimage_only_not_postimage`. +/// +/// To gain confidence in the *postimage*, the caller must inspect the bytes +/// being written: `--dry-run` reports the sha256 that would be published, and +/// re-reading the slug after the write and diffing it against the intended +/// content is the only check that inspects what actually landed. #[allow(clippy::too_many_arguments)] pub async fn cmd_patch( client: &BuzzClient, @@ -1042,4 +1093,300 @@ mod tests { --- a/y\n+++ b/y\n@@ -1 +1 @@\n-c\n+d\n"; assert_eq!(multi.lines().filter(|l| l.starts_with("--- ")).count(), 2); } + + /// Pins the *limit* of `mem patch`'s safety, which the docs must not + /// overstate: the strict-position check reads only `Context` and `Delete` + /// lines (it filters `Insert` out), so it authenticates the **preimage** + /// side of the edit and says nothing about the postimage. + /// + /// Consequence: a patch that quotes the real current value in its context + /// and deletions, but whose `+` lines are content from somewhere else + /// entirely, is **accepted**. `--base-hash` does not change this — it + /// hashes the pre-edit value, which such a patch matches exactly. The + /// only defence is checking the postimage, e.g. re-reading the slug after + /// the write and diffing it against the bytes you meant to publish. + /// + /// Positive control included: corrupting the *preimage* side of the same + /// patch is refused, proving the check runs at all rather than being a + /// no-op that accepts everything. + #[test] + fn strict_position_authenticates_preimage_only_not_postimage() { + let current = "mine: alpha\nmine: beta\n"; + + // Real preimage (context + deletion quote the current value exactly), + // foreign postimage (the `+` line is another agent's content). + let foreign_postimage = "\ +--- a/x ++++ b/x +@@ -1,2 +1,2 @@ + mine: alpha +-mine: beta ++SOMEONE ELSE'S MEMORY +"; + let patch = diffy::Patch::from_str(foreign_postimage).unwrap(); + verify_hunks_at_declared_position(current, &patch) + .expect("preimage-side check accepts a foreign postimage — this is the known limit"); + + // And it really does apply, producing content the operator never wrote. + assert_eq!( + diffy::apply(current, &patch).unwrap(), + "mine: alpha\nSOMEONE ELSE'S MEMORY\n" + ); + + // `--base-hash` cannot catch it either: the gate compares against the + // *pre-edit* value, which this patch's preimage matches verbatim. + // (`cmd_patch` compares `sha256_hex(¤t)` to the flag.) + assert_eq!( + sha256_hex(current), + sha256_hex("mine: alpha\nmine: beta\n"), + "base-hash is computed over the preimage, which the patch matches" + ); + + // Positive control: corrupt the preimage side and the same check + // refuses. Only the `-` line differs from the accepted patch above. + let foreign_preimage = "\ +--- a/x ++++ b/x +@@ -1,2 +1,2 @@ + mine: alpha +-SOMEONE ELSE'S MEMORY ++mine: gamma +"; + let patch = diffy::Patch::from_str(foreign_preimage).unwrap(); + assert!( + verify_hunks_at_declared_position(current, &patch).is_err(), + "preimage-side corruption must be refused" + ); + } + + // ── Off-curve pubkey handling ───────────────────────────────────────── + // + // `PublicKey::from_hex` only hex-decodes 32 bytes, so a hex-valid + // x-coordinate with no curve point (here `00…05`) used to travel all the + // way into NIP-44 ECDH and abort the process. Every `buzz mem` subcommand + // that took such a key from `--agent`/`--owner` exited 101 with + // `valid keys produce conversation key: Key(Secp256k1(InvalidPublicKey))`. + // These tests pin the replacement behaviour: a `Usage` error (exit 1) that + // names the offending flag and key. + + /// An x-coordinate with no corresponding curve point. `x = 5` has no + /// square root of `x³ + 7` mod p; `x = 1` does, which is why the two make + /// a discriminating pair (see `conversation_key_rejects_off_curve_pubkey` + /// in buzz-core). + fn off_curve_hex() -> String { + format!("{:0>64}", 5) + } + + /// Same length, same alphabet, but a real curve point — the positive + /// control for every off-curve assertion below. + fn on_curve_hex() -> String { + format!("{:0>64}", 1) + } + + #[test] + fn parse_pubkey_flag_rejects_off_curve_but_accepts_on_curve() { + // Positive control first: an always-Err guard would pass the negative + // assertion alone. + parse_pubkey_flag("--owner", &on_curve_hex()).expect("x=1 is a curve point"); + + let err = parse_pubkey_flag("--owner", &off_curve_hex()).unwrap_err(); + assert!(matches!(err, CliError::Usage(_)), "got: {err:?}"); + let msg = err.to_string(); + assert!(msg.contains("--owner"), "must name the flag: {msg}"); + assert!(msg.contains(&off_curve_hex()), "must name the key: {msg}"); + assert!( + msg.contains("secp256k1 curve"), + "must say why it was refused: {msg}" + ); + assert_eq!(crate::error::exit_code(&err), 1, "usage errors exit 1"); + } + + #[test] + fn resolve_reader_rejects_off_curve_agent_flag() { + let client = test_client(nostr::Keys::generate()); + + // Positive control: a real agent pubkey resolves. + let other = nostr::Keys::generate(); + resolve_reader(&client, None, Some(&other.public_key().to_hex())).unwrap(); + + let err = resolve_reader(&client, None, Some(&off_curve_hex())).unwrap_err(); + assert!(matches!(err, CliError::Usage(_)), "got: {err:?}"); + assert!(err.to_string().contains("--agent"), "got: {err}"); + } + + #[test] + fn resolve_owner_rejects_off_curve_owner_flag() { + let client = test_client(nostr::Keys::generate()); + + let owner = nostr::Keys::generate(); + resolve_owner(&client, Some(&owner.public_key().to_hex())).unwrap(); + + let err = resolve_owner(&client, Some(&off_curve_hex())).unwrap_err(); + assert!(matches!(err, CliError::Usage(_)), "got: {err:?}"); + assert!(err.to_string().contains("--owner"), "got: {err}"); + } + + /// The owner pubkey can also arrive from `BUZZ_AUTH_TAG` rather than a + /// flag. That slot is attacker-influenced too, so it gets the same + /// curve check — surfaced as `Other` (exit 4) because a bad auth tag is + /// an environment defect, not a mistyped argument. + #[test] + fn resolve_owner_rejects_off_curve_auth_tag_owner() { + fn client_with_auth_tag(owner_hex: &str) -> BuzzClient { + let tag = + nostr::Tag::parse(["auth", owner_hex, "conditions", &"b".repeat(128)]).unwrap(); + BuzzClient::new( + "http://127.0.0.1:9".into(), + nostr::Keys::generate(), + Some(tag), + None, + ) + .unwrap() + } + + // Positive control: a real pubkey in the same slot resolves. + let real = nostr::Keys::generate().public_key().to_hex(); + let owner = resolve_owner(&client_with_auth_tag(&real), None).unwrap(); + assert_eq!(owner.to_hex(), real); + + let err = resolve_owner(&client_with_auth_tag(&off_curve_hex()), None).unwrap_err(); + assert!(matches!(err, CliError::Other(_)), "got: {err:?}"); + assert!(err.to_string().contains("secp256k1 curve"), "got: {err}"); + } + + /// Spin up a relay stub that answers `POST /query` with an empty event + /// array, so read commands can reach their "no head" branch without + /// network flakiness. Only `/query` is served: any command that tried to + /// *write* would 404 rather than silently pass. + async fn empty_query_relay() -> String { + use axum::body::Body; + use axum::http::{Response, StatusCode}; + use axum::Router; + + let app = Router::new().route( + "/query", + axum::routing::post(|| async { + Response::builder() + .status(StatusCode::OK) + .header("content-type", "application/json") + .body(Body::from("[]")) + .unwrap() + }), + ); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + tokio::spawn(async move { axum::serve(listener, app).await.unwrap() }); + format!("http://{addr}") + } + + /// Which subcommand to drive, and through which flag the pubkey arrives. + #[derive(Copy, Clone, Debug)] + enum Sub { + Get, + Hash, + Ls, + Set, + Rm, + Patch, + } + + /// Drive one subcommand with `pubkey` supplied via `--owner` (or + /// `--agent` when `via_agent_flag`). Everything else is fixed. + async fn run_sub( + sub: Sub, + via_agent_flag: bool, + pubkey: &str, + relay_url: &str, + patch_path: &str, + ) -> Result<(), CliError> { + let client = + BuzzClient::new(relay_url.to_string(), nostr::Keys::generate(), None, None).unwrap(); + let (owner, agent) = if via_agent_flag { + (None, Some(pubkey)) + } else { + (Some(pubkey), None) + }; + match sub { + Sub::Get => cmd_get(&client, "mem/x", owner, agent).await, + Sub::Hash => cmd_hash(&client, "mem/x", owner, agent).await, + Sub::Ls => cmd_ls(&client, owner, agent, true).await, + Sub::Set => cmd_set(&client, "mem/x", "value", owner, false).await, + Sub::Rm => cmd_rm(&client, "mem/x", owner).await, + // no_base_hash + dry_run: the base-hash gate and the write are + // orthogonal to key validation, and both need a head that the + // stub relay does not have. + Sub::Patch => { + cmd_patch( + &client, + "mem/x", + Some(patch_path), + None, + true, + true, + false, + owner, + ) + .await + } + } + } + + /// End-to-end over every subcommand that used to abort the process. + /// + /// The panic surface was wider than `--agent`: `get`/`hash`/`set`/`rm` + /// with `--owner` aborted too (measured at `a2d8be5e`). `ls` and `patch` + /// already failed cleanly, and are included so a future refactor cannot + /// regress them. + /// + /// Each command runs twice: once with a real curve point (must NOT be a + /// `Usage` error — the positive control proving the guard is not simply + /// refusing every key) and once with the off-curve key (must be `Usage`, + /// exit 1, never a panic). + #[tokio::test] + async fn off_curve_flags_are_usage_errors_across_mem_subcommands() { + let url = empty_query_relay().await; + + // A patch file keeps `mem patch` off stdin. + let dir = tempfile::tempdir().unwrap(); + let patch_path = dir.path().join("p.diff"); + std::fs::write(&patch_path, "--- a/x\n+++ b/x\n@@ -1 +1 @@\n-a\n+b\n").unwrap(); + let patch_path = patch_path.to_str().unwrap().to_string(); + + for (sub, via_agent_flag) in [ + (Sub::Get, false), + (Sub::Get, true), + (Sub::Hash, false), + (Sub::Hash, true), + (Sub::Ls, false), + (Sub::Ls, true), + (Sub::Set, false), + (Sub::Rm, false), + (Sub::Patch, false), + ] { + let flag = if via_agent_flag { "--agent" } else { "--owner" }; + + // Positive control: a real key must get past key validation. It + // may still fail later (no head on the stub relay, which serves + // no write route) — it must not fail as a *usage* error. + let real = nostr::Keys::generate().public_key().to_hex(); + let ok = run_sub(sub, via_agent_flag, &real, &url, &patch_path).await; + assert!( + !matches!(ok, Err(CliError::Usage(_))), + "{sub:?} {flag} with a real pubkey must not be a usage error: {ok:?}" + ); + + let err = run_sub(sub, via_agent_flag, &off_curve_hex(), &url, &patch_path) + .await + .unwrap_err(); + assert!( + matches!(err, CliError::Usage(_)), + "{sub:?} {flag}: expected Usage, got {err:?}" + ); + assert_eq!( + crate::error::exit_code(&err), + 1, + "{sub:?} {flag} must exit 1" + ); + } + } } diff --git a/crates/buzz-cli/src/lib.rs b/crates/buzz-cli/src/lib.rs index 2b041da57b5..f09dd9fc14a 100644 --- a/crates/buzz-cli/src/lib.rs +++ b/crates/buzz-cli/src/lib.rs @@ -1810,6 +1810,11 @@ pub enum MemCmd { /// Reads the diff from stdin or `--patch-file`. Refuses to apply if the /// slug has changed since `--base-hash` was captured, and refuses /// hunks whose context doesn't match the current value verbatim. + /// + /// Both checks cover the value being *replaced*, not the replacement: + /// a patch that quotes the current value correctly can still insert + /// content from anywhere. Verify the result (`--dry-run` prints the + /// sha256 that would be written; re-read the slug after writing). Patch { slug: String, /// Read the patch from a file instead of stdin. @@ -1818,12 +1823,13 @@ pub enum MemCmd { /// sha256 hex digest (lowercase) of the value the patch was generated /// against. Hashes the exact UTF-8 bytes returned by `buzz mem get`, /// not normalized lines. Run `buzz mem hash ` to capture this - /// before editing. + /// before editing. Proves the head has not moved; says nothing about + /// the content the patch inserts. #[arg(long)] base_hash: Option, - /// Skip the base-hash check. Unsafe if concurrent edits are possible — - /// the patch will be applied against whatever the current value is, - /// even if another agent rewrote it after the patch was generated. + /// Skip the base-hash check. The patch will be applied against + /// whatever the current value is, even if another agent rewrote it + /// after the patch was generated — a lost update becomes possible. #[arg(long, default_value_t = false)] no_base_hash: bool, /// Echo the input patch + resulting sha256 and exit without writing. diff --git a/crates/buzz-core/src/engram.rs b/crates/buzz-core/src/engram.rs index 7f38347f808..fcd78fbc913 100644 --- a/crates/buzz-core/src/engram.rs +++ b/crates/buzz-core/src/engram.rs @@ -45,6 +45,11 @@ pub enum EngramError { /// Event failed *Head selection* rule (1) — tag shape or addressing. #[error("invalid envelope: {0}")] InvalidEnvelope(String), + /// A supplied pubkey is not a point on the secp256k1 curve, so no + /// conversation key exists for it. `PublicKey::from_hex` accepts any + /// 32 bytes; only the ECDH step rejects a non-curve x-coordinate. + #[error("invalid key: {0}")] + InvalidKey(String), /// NIP-44 decryption failed. #[error("decrypt failed")] Decrypt, @@ -133,8 +138,24 @@ pub fn normalize_slug(raw: &str) -> Result { /// Derive the conversation key `K_c` for the agent ↔ owner pair (NIP-44 v2). /// /// `K_c` is symmetric: `derive(seckey_a, pubkey_o) == derive(seckey_o, pubkey_a)`. -pub fn conversation_key(my_seckey: &SecretKey, their_pubkey: &PublicKey) -> ConversationKey { - ConversationKey::derive(my_seckey, their_pubkey).expect("valid keys produce conversation key") +/// +/// Fallible because a `PublicKey` is *not* proof of a curve point: +/// `PublicKey::from_hex` only hex-decodes 32 bytes ([`nostr`] 0.44 +/// `key/public_key.rs`), while ECDH needs the x-only key to lift to a real +/// point. An x-coordinate with no corresponding y (e.g. `00…05`) reaches +/// this function intact and fails here, so every caller that accepts a +/// pubkey from user input MUST surface [`EngramError::InvalidKey`] rather +/// than unwrap it. +pub fn conversation_key( + my_seckey: &SecretKey, + their_pubkey: &PublicKey, +) -> Result { + ConversationKey::derive(my_seckey, their_pubkey).map_err(|e| { + EngramError::InvalidKey(format!( + "{} is not a valid secp256k1 public key: {e}", + their_pubkey.to_hex() + )) + }) } /// Compute the `d` tag for a slug under a conversation key. @@ -449,7 +470,7 @@ pub fn build_event( let plaintext_str = std::str::from_utf8(&plaintext) .map_err(|e| EngramError::Encrypt(format!("body JSON not UTF-8: {e}")))?; - let k_c = conversation_key(agent_keys.secret_key(), owner_pubkey); + let k_c = conversation_key(agent_keys.secret_key(), owner_pubkey)?; let ciphertext = nip44::encrypt( agent_keys.secret_key(), owner_pubkey, @@ -546,7 +567,7 @@ pub fn validate_and_decrypt( let body = Body::from_json_bytes(plaintext.as_bytes())?; // Rule (4): body slug re-derives to the event's d tag. - let k_c = conversation_key(my_seckey, their_pubkey); + let k_c = conversation_key(my_seckey, their_pubkey)?; let derived = d_tag(&k_c, body.slug()); if derived != d_value { return Err(EngramError::InvalidEnvelope( @@ -636,17 +657,111 @@ mod tests { fn conversation_key_matches_spec() { let a = keys_from_hex(SECKEY_A); let o = keys_from_hex(SECKEY_O); - let k_c_ao = conversation_key(a.secret_key(), &o.public_key()); - let k_c_oa = conversation_key(o.secret_key(), &a.public_key()); + let k_c_ao = conversation_key(a.secret_key(), &o.public_key()).unwrap(); + let k_c_oa = conversation_key(o.secret_key(), &a.public_key()).unwrap(); assert_eq!(hex::encode(k_c_ao.as_bytes()), K_C_HEX, "agent-side K_c"); assert_eq!(hex::encode(k_c_oa.as_bytes()), K_C_HEX, "owner-side K_c"); } + /// `PublicKey::from_hex` accepts any 32 bytes, so an x-coordinate with no + /// curve point reaches `conversation_key` intact. It must return + /// `InvalidKey` rather than panic. Discriminating pair: on secp256k1 + /// `x = 1` lifts to a point and `x = 5` does not, so the only difference + /// between these two inputs is curve membership. + #[test] + fn conversation_key_rejects_off_curve_pubkey() { + let a = keys_from_hex(SECKEY_A); + let on_curve = PublicKey::from_hex(&format!("{:0>64}", 1)).unwrap(); + let off_curve = PublicKey::from_hex(&format!("{:0>64}", 5)).unwrap(); + + // Positive control: without it, an always-Err implementation would + // also pass the negative assertion below. + assert!( + conversation_key(a.secret_key(), &on_curve).is_ok(), + "x=1 is on-curve and must derive" + ); + + let err = conversation_key(a.secret_key(), &off_curve).unwrap_err(); + assert!( + matches!(err, EngramError::InvalidKey(_)), + "expected InvalidKey, got: {err:?}" + ); + assert!( + err.to_string().contains(&off_curve.to_hex()), + "message must name the offending key: {err}" + ); + } + + /// The same off-curve key must not panic through `build_event`, which + /// derives `K_c` for the `d` tag before encrypting. + /// + /// `validate_and_decrypt` also derives `K_c` (head-selection rule 4), but + /// `nip44::decrypt` runs first and derives the same key internally, so an + /// off-curve peer surfaces there as `Decrypt`; its `?` is defensive depth, + /// not a separately reachable path. Pinned by + /// `off_curve_peer_in_validate_and_decrypt_is_decrypt_error` below. + #[test] + fn off_curve_owner_is_an_error_not_a_panic_in_build_event() { + let a = keys_from_hex(SECKEY_A); + let off_curve = PublicKey::from_hex(&format!("{:0>64}", 5)).unwrap(); + let body = Body::Core { + profile: "x".into(), + }; + + // Positive control on the same body: a real owner key succeeds. + let o = keys_from_hex(SECKEY_O); + assert!(build_event(&a, &o.public_key(), &body, 1_700_000_000).is_ok()); + + let err = build_event(&a, &off_curve, &body, 1_700_000_000).unwrap_err(); + assert!( + matches!(err, EngramError::InvalidKey(_)), + "expected InvalidKey, got: {err:?}" + ); + } + + /// Documents which error an off-curve *peer* key actually produces on the + /// read path, so a future reader doesn't assume `InvalidKey`: NIP-44 + /// decrypt fails before rule (4) re-derives `K_c`. Either way it does not + /// panic, which is the property under test. + #[test] + fn off_curve_peer_in_validate_and_decrypt_is_decrypt_error() { + let a = keys_from_hex(SECKEY_A); + let o = keys_from_hex(SECKEY_O); + let off_curve = PublicKey::from_hex(&format!("{:0>64}", 5)).unwrap(); + let body = Body::Core { + profile: "x".into(), + }; + let ev = build_event(&a, &o.public_key(), &body, 1_700_000_000).unwrap(); + + // Positive control: the real pair validates. + assert!(validate_and_decrypt( + &ev, + &a.public_key(), + &o.public_key(), + a.secret_key(), + &o.public_key() + ) + .is_ok()); + + let err = validate_and_decrypt( + &ev, + &a.public_key(), + &o.public_key(), + a.secret_key(), + &off_curve, + ) + .unwrap_err(); + assert!( + matches!(err, EngramError::Decrypt), + "expected Decrypt, got: {err:?}" + ); + } + #[test] fn d_tags_match_spec() { let a = keys_from_hex(SECKEY_A); let o = keys_from_hex(SECKEY_O); - let k_c = conversation_key(a.secret_key(), &o.public_key()); + let k_c = conversation_key(a.secret_key(), &o.public_key()).unwrap(); assert_eq!(d_tag(&k_c, "core"), D_CORE); assert_eq!(d_tag(&k_c, "mem/example"), D_EXAMPLE); assert_eq!(d_tag(&k_c, "mem/notes/2026-05-12"), D_NOTES); diff --git a/desktop/src-tauri/src/managed_agents/nest.rs b/desktop/src-tauri/src/managed_agents/nest.rs index 72cf4664272..093eb8fd289 100644 --- a/desktop/src-tauri/src/managed_agents/nest.rs +++ b/desktop/src-tauri/src/managed_agents/nest.rs @@ -52,7 +52,7 @@ const NEST_AGENTS_VERSION: u32 = 4; /// Template content version for SKILL.md. /// Bump this when changing `nest_skill.md` to trigger refresh on existing installs. -const NEST_SKILL_VERSION: u32 = 5; +const NEST_SKILL_VERSION: u32 = 6; const BEGIN_MARKER: &str = ""; diff --git a/desktop/src-tauri/src/managed_agents/nest_skill.md b/desktop/src-tauri/src/managed_agents/nest_skill.md index 79a5ea301d4..d4f21886ca7 100644 --- a/desktop/src-tauri/src/managed_agents/nest_skill.md +++ b/desktop/src-tauri/src/managed_agents/nest_skill.md @@ -151,7 +151,7 @@ Message content is rendered as GitHub-flavored Markdown on both desktop and mobi ## Mem Patch Workflow -For safe concurrent writes, use hash-based conflict detection: +Use hash-based conflict detection so a concurrent writer cannot be silently overwritten: ```bash HASH=$(buzz mem hash ) # 1. get current SHA-256 @@ -161,7 +161,14 @@ buzz mem patch --base-hash "$HASH" --patch-file diff.patch # 2. apply wi Exit code 5 if the value changed since the hash was read (another agent wrote first). Retry by re-reading, re-diffing, and re-patching. -Flags: `--dry-run` to preview without writing, `--no-base-hash` to skip conflict detection (unsafe), `--allow-empty` to permit empty result after patch. +Flags: `--dry-run` to preview without writing, `--no-base-hash` to skip the head-moved check (a concurrent write can then be lost), `--allow-empty` to permit empty result after patch. + +**What `--base-hash` proves, and what it doesn't.** It proves the head has not moved since you read it, and hunk context and `-` lines must match the current value verbatim at their declared line numbers. Both checks read only the value being *replaced*. The `+` lines are never checked against anything, so a patch that quotes your current value correctly can insert content from a completely different file and it will be accepted and published. Verify the bytes you are about to write, not just the bytes you are replacing: + +- `--dry-run` prints the sha256 that *would* be written — compare it to the sha256 of your intended content before the real run. +- After writing, `buzz mem get ` and diff it against what you meant to publish. That readback is the only check that inspects what actually landed. +- Assert something only you would write (your own agent name or identity string) is present in the postimage. A cross-file mix-up usually fails that assertion immediately. +- Write pre-edit snapshots to a path unique to you, e.g. `MEMORY_BACKUPS//core-$(date +%s).md`. Never use a generic shared filename such as `.scratch/core_now.md`: two agents running at once will clobber each other's snapshot and then restore the wrong one. ## Polling Pattern