From 9136552e480198da0c6ac87f6142eeaec1030eaf Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Sat, 12 Sep 2026 08:50:42 -0400 Subject: [PATCH] fix(pds)!: keep the data directory out of a refusal a client reads A store refusal now carries the sizes and the sentence about the caller's own write, and the chain with the file path in it goes to a log line beside it. Co-Authored-By: Claude Opus 5 (1M context) Change-Id: I3cbc2c6a5fc23573d662dd5b822547c8773f08e3 --- crates/didbot-pds/src/durable.rs | 33 ++++++--- crates/didbot-pds/src/heap.rs | 17 +++++ crates/didbot-pds/src/records.rs | 20 ++++-- crates/didbot-pds/src/wal/mod.rs | 16 +++++ crates/didbot-pds/tests/refusal.rs | 112 +++++++++++++++++++++++------ crates/didbot-serve/src/error.rs | 8 ++- 6 files changed, 162 insertions(+), 44 deletions(-) diff --git a/crates/didbot-pds/src/durable.rs b/crates/didbot-pds/src/durable.rs index 8cc3a171..cec36632 100644 --- a/crates/didbot-pds/src/durable.rs +++ b/crates/didbot-pds/src/durable.rs @@ -874,14 +874,20 @@ impl crate::sequence::SequenceLog for FileSequenceLog { /// and it should come back — two things a client does something about, and /// one it does not. See [`crate::wal`]'s "Running out of room". fn record_backend(error: WalError) -> RecordError { + let reason = error.refusal(); match error { - WalError::TooLarge { .. } => RecordError::TooLarge { - reason: backend(error), - }, - WalError::Full { .. } => RecordError::StorageFull { - reason: backend(error), - }, - other => RecordError::Backend(backend(other)), + WalError::TooLarge { .. } => { + tracing::warn!(error = %backend(error), "refused a record larger than a frame"); + RecordError::TooLarge { reason } + } + WalError::Full { .. } => { + tracing::warn!(error = %backend(error), "refused a record: the log is full"); + RecordError::StorageFull { reason } + } + other => { + tracing::error!(error = %backend(other), "the record store's log failed"); + RecordError::Backend(reason) + } } } @@ -892,11 +898,16 @@ fn record_backend(error: WalError) -> RecordError { /// still refuse to provision, and that refusal is clean — see /// [`StoreError::Full`]. fn store_backend(error: WalError) -> StoreError { + let reason = error.refusal(); match error { - WalError::Full { .. } => StoreError::Full { - reason: backend(error), - }, - other => StoreError::Backend(backend(other)), + WalError::Full { .. } => { + tracing::warn!(error = %backend(error), "refused an account: the log is full"); + StoreError::Full { reason } + } + other => { + tracing::error!(error = %backend(other), "the account store's log failed"); + StoreError::Backend(reason) + } } } diff --git a/crates/didbot-pds/src/heap.rs b/crates/didbot-pds/src/heap.rs index 92188061..6096b640 100644 --- a/crates/didbot-pds/src/heap.rs +++ b/crates/didbot-pds/src/heap.rs @@ -179,6 +179,23 @@ pub enum HeapError { }, } +impl HeapError { + /// The part of this failure that is about the caller's request. + /// + /// The same split [`crate::wal::WalError::refusal`] makes, for the file + /// the record bodies live in: the sizes and the sentence go to the client, + /// and the chain with the path in it goes to the log line beside it. + pub(crate) fn refusal(&self) -> String { + match self { + Self::TooLarge { size, limit, .. } => format!( + "a value of {size} bytes is larger than the {limit} bytes a frame can carry" + ), + Self::Full { reason, .. } => reason.clone(), + _ => "the record heap could not take the value".to_owned(), + } + } +} + /// An append-only file of framed bodies, with a buffer in front of it. #[derive(Debug)] pub struct Heap { diff --git a/crates/didbot-pds/src/records.rs b/crates/didbot-pds/src/records.rs index 16b59d08..d379e95d 100644 --- a/crates/didbot-pds/src/records.rs +++ b/crates/didbot-pds/src/records.rs @@ -2591,14 +2591,20 @@ pub fn encode_record(record: &Value) -> Result<(Vec, Cid), RecordError> { /// the same reason: a full disk and a record too large are things a client /// does something about, and a backend error is not. pub(crate) fn heap_backend(error: HeapError) -> RecordError { + let reason = error.refusal(); match error { - HeapError::TooLarge { .. } => RecordError::TooLarge { - reason: error.to_string(), - }, - HeapError::Full { .. } => RecordError::StorageFull { - reason: error.to_string(), - }, - other => RecordError::Backend(other.to_string()), + HeapError::TooLarge { .. } => { + tracing::warn!(%error, "refused a record body larger than a frame"); + RecordError::TooLarge { reason } + } + HeapError::Full { .. } => { + tracing::warn!(%error, "refused a record body: the heap is full"); + RecordError::StorageFull { reason } + } + other => { + tracing::error!(error = %other, "the record heap failed"); + RecordError::Backend(reason) + } } } diff --git a/crates/didbot-pds/src/wal/mod.rs b/crates/didbot-pds/src/wal/mod.rs index 47b84bdc..79a165be 100644 --- a/crates/didbot-pds/src/wal/mod.rs +++ b/crates/didbot-pds/src/wal/mod.rs @@ -955,6 +955,22 @@ impl WalError { source, } } + + /// The part of this failure that is about the caller's request. + /// + /// A store refusal is handed to the client that made the write, so it + /// carries the sizes and the sentence and nothing about where this + /// deployment keeps its files. The whole chain, path included, belongs in + /// the log line beside it. + pub(crate) fn refusal(&self) -> String { + match self { + Self::TooLarge { size, limit, .. } => format!( + "an entry of {size} bytes is larger than the {limit} bytes a frame can carry" + ), + Self::Full { reason, .. } => reason.clone(), + _ => "the write-ahead log could not take the entry".to_owned(), + } + } } /// What replaying a log produced. diff --git a/crates/didbot-pds/tests/refusal.rs b/crates/didbot-pds/tests/refusal.rs index 3bf0c218..4cf840a3 100644 --- a/crates/didbot-pds/tests/refusal.rs +++ b/crates/didbot-pds/tests/refusal.rs @@ -63,6 +63,34 @@ fn write(store: &impl RecordStore, text: &str) -> Result { .map(|written| written.rkey) } +/// An account this deployment could hold, and the DID naming it. +fn account(agent_id: &str) -> (HostedDid, AgentAccount) { + let did = HostedDid::host( + AgentDid::parse(&format!("did:web:{agent_id}.agents.localhost")).expect("a did"), + &ZoneRegistry::single(Zone::new("agents.localhost").expect("a zone")), + ) + .expect("hosted"); + let account = AgentAccount { + did: did.clone(), + agent_id: agent_id.to_owned(), + handle: None, + harness: None, + agent_type: None, + created_at: OffsetDateTime::now_utc(), + kind: AccountKind::default(), + name_provenance: NameProvenance::default(), + retention: Retention::default(), + provenance: None, + parent: None, + state: AccountState::Active, + locks: didbot_pds::Locks::none(), + holds: didbot_pds::Holds::none(), + node_key: None, + expected_operator: None, + }; + (did, account) +} + fn stored(durable: &Durable) -> usize { durable .records() @@ -199,29 +227,7 @@ fn a_full_log_refuses_an_account_without_making_half_of_one() { let wal = durable.wal(); wal.set_capacity(Some(wal.len())); - let did = HostedDid::host( - AgentDid::parse("did:web:gannet.agents.localhost").expect("a did"), - &ZoneRegistry::single(Zone::new("agents.localhost").expect("a zone")), - ) - .expect("hosted"); - let account = AgentAccount { - did: did.clone(), - agent_id: "gannet".to_owned(), - handle: None, - harness: None, - agent_type: None, - created_at: OffsetDateTime::now_utc(), - kind: AccountKind::default(), - name_provenance: NameProvenance::default(), - retention: Retention::default(), - provenance: None, - parent: None, - state: AccountState::Active, - locks: didbot_pds::Locks::none(), - holds: didbot_pds::Holds::none(), - node_key: None, - expected_operator: None, - }; + let (did, account) = account("gannet"); let refused = accounts .insert(account, SigningKey::generate()) .expect_err("a full log accepted an account"); @@ -241,3 +247,63 @@ fn a_full_log_refuses_an_account_without_making_half_of_one() { "an account that was refused came back from the log" ); } + +/// A refusal a client reads says nothing about where the files are. +/// +/// These messages travel: `RecordError` and `StoreError` become the `message` +/// field of the JSON an XRPC route answers with, so a stranger who writes an +/// oversized record reads whatever is in one. The sizes and the sentence are +/// about the caller's own write. Which directory this deployment keeps its log +/// and its heap in is not, and it goes to the log line instead. +#[test] +fn a_refusal_a_client_reads_says_nothing_about_the_disk() { + let dir = Dir::new("paths"); + let durable = open(&dir); + let records = durable.records(); + + let huge = json!({ "text": "x".repeat(MAX_ENTRY + 1) }); + let refused = records + .put(DID, COLLECTION, None, huge, &Precondition::Unconditional) + .expect_err("an entry larger than a frame was accepted"); + assert!( + matches!(refused, RecordError::TooLarge { .. }), + "an oversized record was refused as {refused:?} rather than as too large" + ); + says_nothing_about_the_disk(&dir, &refused.to_string()); + + // A deployment with no room refuses both stores, and both are read by + // whoever made the write. + let wal = durable.wal(); + wal.set_capacity(Some(wal.len())); + + let refused = write(&*records, "quernstone").expect_err("a full log accepted a record"); + assert!( + matches!(refused, RecordError::StorageFull { .. }), + "a full log refused a record as {refused:?} rather than as full" + ); + says_nothing_about_the_disk(&dir, &refused.to_string()); + + let (_, account) = account("sillabub"); + let refused = durable + .accounts() + .insert(account, SigningKey::generate()) + .expect_err("a full log accepted an account"); + assert!( + matches!(refused, StoreError::Full { .. }), + "a full log refused an account as {refused:?} rather than as full" + ); + says_nothing_about_the_disk(&dir, &refused.to_string()); +} + +/// Asserts a message is free of this deployment's data directory. +fn says_nothing_about_the_disk(dir: &Dir, message: &str) { + let data = dir.0.display().to_string(); + assert!( + !message.contains(&data), + "a refusal handed to a client names the data directory `{data}`: {message}" + ); + assert!( + !message.contains(std::path::MAIN_SEPARATOR), + "a refusal handed to a client carries a path separator: {message}" + ); +} diff --git a/crates/didbot-serve/src/error.rs b/crates/didbot-serve/src/error.rs index 32320328..38062387 100644 --- a/crates/didbot-serve/src/error.rs +++ b/crates/didbot-serve/src/error.rs @@ -251,9 +251,11 @@ impl ApiError { impl From<&ProvisionError> for ApiError { /// Maps a provisioning failure onto a status and an error name. /// - /// The `Display` text of the underlying error becomes the message. That is - /// safe to hand back because no error in the chain formats a secret or a - /// signing key into its text. + /// The `Display` text of the underlying error becomes the message. Every + /// error in the chain is written for the caller to read: a secret, a + /// signing key and the path to a file this deployment keeps are each kept + /// out of it, and a store refusal carries only the sizes and the sentence + /// that are about the request itself. fn from(err: &ProvisionError) -> Self { let (status, name) = match err { ProvisionError::Identity(..) => (StatusCode::BAD_REQUEST, "InvalidIdentity"), -- 2.51.2