diff --git a/.claude/skills/resolving-issues/SKILL.md b/.claude/skills/resolving-issues/SKILL.md index 88aa0df2..0554cfc7 100644 --- a/.claude/skills/resolving-issues/SKILL.md +++ b/.claude/skills/resolving-issues/SKILL.md @@ -99,6 +99,9 @@ Fresh agent per issue, scoped to one issue + master branch ref. Steps: 6. **Scope-creep guard:** if root-cause fix touches > 5 files OR > 200 LOC AND brainstorm in step 3 didn't already approve that scope, return to coordinator with a brainstorm note before pushing. Coordinator decides: split, defer, or proceed. Don't unilaterally balloon a small-scope ticket. **Mechanical call-site migration is part of the fix, not scope creep.** If the fix changes a small API (e.g. swapping `map.insert(k, v)` for `lru.insert(k, v)` to make a new cap take effect), every call-site rewrite is load-bearing — without them the cap is dead code. Count them in the LOC delta but don't abort just because they push past 200. Justify the count in the brainstorm + commit body so the human can see why the fan-out was unavoidable. Real scope creep = unrelated cleanup, drive-by refactors, "while I'm in here" tweaks — those still abort. + + **Cargo.lock conflicts with in-flight PRs are usually additive — don't refuse the dep.** When a fix needs a new workspace dep and `Cargo.lock` is in another open PR's file list, don't abort: dep additions are strictly additive in `Cargo.lock` (your row gets appended; the other PR's rows stay untouched). Merge resolution post-PR-#X is mechanical. Note the `Cargo.lock` churn in the commit body so the human reviewer expects a small textual conflict on the master PR's merge. Refusing on this basis creates infinite "wait for PR X to merge" deadlocks that block dependency upgrades and dual-target fixes indefinitely. Same logic applies to additive-only edits in any "shared by every change" file (e.g. workspace `Cargo.toml`'s `[workspace.dependencies]` table) — coordinator-narrowed briefs should explicitly authorise additive churn. + 7. **Local merge gate.** Run, in order: - `cargo fmt --all -- --check` (or `just fmt-check` if available) - `cargo clippy --all-targets -- -D warnings` — scope to touched crate(s) for speed; workspace-wide if changes ripple @@ -113,6 +116,8 @@ Fresh agent per issue, scoped to one issue + master branch ref. Steps: **Browser tests (`wasm-pack` + Firefox + geckodriver) may be unavailable.** `cargo check --target wasm32-unknown-unknown -p willow-web --tests` is the fallback gate — confirms the test compiles. Real headless run executes on master-PR CI. Flag the gap in the commit body. + **Non-Rust file changes still need `cargo test` on related crates.** Coordinator briefs sometimes argue "CSS / HTML / JSON / YAML / `.well-known` change — no Rust touched, skip cargo test." This is a trap: integration tests (`crates//tests/*.rs`) commonly assert on the contents of static assets — e.g. `crates/web/tests/static_assets.rs` greps `index.html` for required CSP directives, manifest.json for icon refs, etc. A non-Rust change that flips an asserted substring is a CI-red landmine. **Default rule:** if you touched any file in `crates//` or anywhere those crates' tests read at runtime (HTML / CSS / JSON / TOML / YAML / sw.js), still run `cargo test -p ` (or at minimum `cargo test -p --test ` if you can identify the relevant integration suite). Cost is a few seconds; saves a master-PR CI-rescue dispatch (~5-10 min) and a noisy `wip`-shape commit on the master branch. + 8. **Commit + push.** Use `caveman:caveman-commit` for the message. Conventional Commits format. `Refs #N` (NOT `Fixes` — that lives only on the master PR). Push directly to origin master branch. **Never push `wip:` / `chore: checkpoint` / "work-in-progress" commits to the master branch.** Make all your edits, run the local gate, then commit ONCE with the proper Conventional Commits message. Coordinator's master PR body assembles `Fixes #N` rows from per-commit messages — `wip:` rows look like junk to a human reviewer and force a finalize-implementer detour to squash + force-push (real cost: ~10–15 min of cargo lock contention while the rescue agent re-runs gates, plus a `--force-with-lease` against a branch that may have already accumulated more commits from later dispatches if the coordinator misjudges sequencing). If you genuinely need intermediate checkpoints (long sessions, sandbox interference per next bullet), make them in a Pattern B local feature branch and squash via `git merge --no-ff` when done — never push intermediate commits to master. diff --git a/Cargo.lock b/Cargo.lock index d447c70a..591b7280 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6146,6 +6146,7 @@ dependencies = [ "wasm-bindgen", "wasm-bindgen-futures", "web-sys", + "web-time", "willow-actor", "willow-common", "willow-crypto", diff --git a/Cargo.toml b/Cargo.toml index 6d055b08..e45a87c9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -36,6 +36,10 @@ blake3 = "1" # Time & IDs chrono = { version = "0.4", features = ["serde"] } uuid = { version = "1", features = ["v4", "serde", "js"] } +# Portable monotonic clock. Native uses `std::time::Instant`; WASM uses +# `Performance.now()`. Drop-in replacement for `std::time::Instant` so +# lib crates can build for both targets without `#[cfg]` gates. +web-time = "1" # Logging tracing = "0.1" diff --git a/crates/client/Cargo.toml b/crates/client/Cargo.toml index a6b6c4c3..da60ec8c 100644 --- a/crates/client/Cargo.toml +++ b/crates/client/Cargo.toml @@ -33,6 +33,7 @@ uuid = { workspace = true } tracing = { workspace = true } parking_lot = { workspace = true } futures = { workspace = true } +web-time = { workspace = true } regex = "1" [target.'cfg(not(target_arch = "wasm32"))'.dependencies] diff --git a/crates/client/src/worker_cache.rs b/crates/client/src/worker_cache.rs index 613ded6b..dadbb898 100644 --- a/crates/client/src/worker_cache.rs +++ b/crates/client/src/worker_cache.rs @@ -5,8 +5,9 @@ //! (default 30s) without a heartbeat. use std::collections::HashMap; -use std::time::{Duration, Instant}; +use std::time::Duration; +use web_time::Instant; use willow_common::{WorkerAnnouncement, WorkerRoleInfo}; use willow_identity::EndpointId; diff --git a/crates/identity/src/lib.rs b/crates/identity/src/lib.rs index 92b5cecc..227a6121 100644 --- a/crates/identity/src/lib.rs +++ b/crates/identity/src/lib.rs @@ -333,6 +333,14 @@ impl std::fmt::Debug for Identity { // ───── Standalone verification ────────────────────────────────────────────── /// Verify a signature against a public key without needing an [`Identity`]. +/// +/// This delegates to [`PublicKey::verify`] from `iroh-base`, which internally +/// uses `ed25519_dalek::VerifyingKey::verify_strict` (RFC 8032 strict mode). +/// Strict verification rejects non-canonical signature encodings — in particular, +/// signatures whose `S` component is not reduced modulo the curve order — which +/// closes off Ed25519 signature malleability vectors. Defense in depth: any +/// future change that bypasses `iroh-base`'s wrapper must continue to use the +/// strict primitive. pub fn verify(key: &PublicKey, data: &[u8], sig: &Signature) -> bool { key.verify(data, sig).is_ok() } @@ -745,6 +753,37 @@ mod tests { assert!(!verify(&id.public_key(), b"wrong data", &sig)); } + /// Regression: `verify` must use strict Ed25519 verification (RFC 8032), + /// rejecting signatures whose `S` component is non-canonical (S ≥ ℓ). + /// + /// Setting the top bit of byte 63 of `S` makes `S` larger than the curve + /// order ℓ ≈ 2^252 + …, so non-strict verification might accept it while + /// strict verification must reject it. This pins the call-site invariant: + /// the underlying primitive is `verify_strict`, providing defense in depth + /// against signature malleability. + #[test] + fn verify_rejects_non_canonical_s_component() { + let id = Identity::generate(); + let data = b"test data"; + let sig = id.sign(data); + + // Original signature is valid. + assert!(verify(&id.public_key(), data, &sig)); + + // Mutate the S scalar's high bit. S occupies bytes 32..64 of the + // signature; its most-significant byte is at index 63. Flipping the top + // bit pushes S above the curve order, producing a non-canonical encoding + // that strict verification rejects. + let mut bytes = sig.to_bytes(); + bytes[63] ^= 0x80; + let mutated = Signature::from_bytes(&bytes); + + assert!( + !verify(&id.public_key(), data, &mutated), + "verify must reject non-canonical S (signature malleability vector)" + ); + } + #[test] fn endpoint_id_hash_map_key() { use std::collections::HashMap; diff --git a/crates/messaging/src/lib.rs b/crates/messaging/src/lib.rs index 021e1cd0..d56aefc4 100644 --- a/crates/messaging/src/lib.rs +++ b/crates/messaging/src/lib.rs @@ -92,6 +92,51 @@ impl std::fmt::Display for ChannelId { // ───── Content types ───────────────────────────────────────────────────────── +/// Maximum allowed length, in bytes, for a [`Content::File`] filename. +/// +/// Matches POSIX `NAME_MAX` (255). Peer-supplied filenames longer than +/// this are rejected by [`Content::validate`]. +pub const MAX_FILENAME_BYTES: usize = 255; + +/// Maximum allowed length, in bytes, for a [`Content::File`] MIME type. +/// +/// RFC 6838 §4.2 caps the registered `type/subtype` form at 127+127 +/// characters; we use 255 as a conservative POSIX-aligned bound that +/// covers all realistic media types plus parameters. Peer-supplied +/// MIME types longer than this are rejected by [`Content::validate`]. +pub const MAX_MIME_BYTES: usize = 255; + +/// Errors returned by [`Content::validate`] / [`Message::validate`] when a +/// peer-supplied payload exceeds the structural bounds enforced by this +/// crate. +/// +/// These checks guard against unbounded `String` fields in `Content::File` +/// being abused to inflate gossip payloads or memory footprint after a +/// message has already cleared signature verification. The `Content` enum +/// is decoded by `bincode` before any structural check runs, so callers +/// receiving peer-supplied `Content` MUST invoke `validate()` on the +/// decoded value before trusting any of its fields. +#[derive(Debug, thiserror::Error, PartialEq, Eq)] +pub enum MessageValidationError { + /// `Content::File::filename` exceeded [`MAX_FILENAME_BYTES`]. + #[error("filename too long: {actual} bytes (max {max})")] + FilenameTooLong { + /// Observed length in bytes. + actual: usize, + /// Configured maximum. + max: usize, + }, + + /// `Content::File::mime_type` exceeded [`MAX_MIME_BYTES`]. + #[error("mime_type too long: {actual} bytes (max {max})")] + MimeTypeTooLong { + /// Observed length in bytes. + actual: usize, + /// Configured maximum. + max: usize, + }, +} + /// The payload inside a [`Message`]. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum Content { @@ -105,11 +150,21 @@ pub enum Content { File { /// Content-addressed hash of the file data. hash: String, - /// Original filename. + /// Original filename. Bounded to [`MAX_FILENAME_BYTES`] by + /// [`Content::validate`]; values exceeding the cap are rejected + /// at the inbound boundary. filename: String, - /// MIME type (e.g. `image/png`). + /// MIME type (e.g. `image/png`). Bounded to [`MAX_MIME_BYTES`] + /// by [`Content::validate`]; values exceeding the cap are + /// rejected at the inbound boundary. mime_type: String, - /// File size in bytes. + /// File size in bytes, as declared by the sender. + /// + /// **Attacker-declared.** UI MAY display this for the user but + /// MUST NOT use it for preallocation (`Vec::with_capacity`, + /// `reserve`, etc.), allocation hints, or trust decisions. The + /// authoritative size is whatever the content-addressed `hash` + /// resolves to once the file bytes are actually fetched. size_bytes: u64, }, @@ -155,6 +210,45 @@ pub enum Content { Encrypted(SealedContent), } +impl Content { + /// Validate structural bounds on peer-supplied content. + /// + /// `Content` is a wire enum decoded with `bincode` before any + /// structural check runs, so callers handling peer-supplied values + /// MUST invoke this method after decoding and before trusting the + /// fields. Currently the only bounded fields are + /// [`Content::File::filename`] (≤ [`MAX_FILENAME_BYTES`]) and + /// [`Content::File::mime_type`] (≤ [`MAX_MIME_BYTES`]); other + /// variants always validate successfully. + /// + /// `Content::Encrypted` is intentionally not recursed into here — + /// the inner `Content` is opaque ciphertext until the channel key + /// is applied; callers should re-invoke `validate()` on the + /// decrypted `Content` before using its fields. + pub fn validate(&self) -> Result<(), MessageValidationError> { + if let Content::File { + filename, + mime_type, + .. + } = self + { + if filename.len() > MAX_FILENAME_BYTES { + return Err(MessageValidationError::FilenameTooLong { + actual: filename.len(), + max: MAX_FILENAME_BYTES, + }); + } + if mime_type.len() > MAX_MIME_BYTES { + return Err(MessageValidationError::MimeTypeTooLong { + actual: mime_type.len(), + max: MAX_MIME_BYTES, + }); + } + } + Ok(()) + } +} + /// The encrypted form of a [`Content`] value. /// /// Produced by encrypting a serialized `Content` with ChaCha20-Poly1305. @@ -258,6 +352,16 @@ impl Message { } } + /// Validate structural bounds on this message's payload. + /// + /// Convenience wrapper around [`Content::validate`]; see that + /// method's docs for the contract. Callers receiving a peer-supplied + /// [`Message`] over the wire MUST invoke this before trusting any + /// fields of the inner [`Content`]. + pub fn validate(&self) -> Result<(), MessageValidationError> { + self.content.validate() + } + /// Create a file-sharing message. pub fn file( channel_id: ChannelId, @@ -535,4 +639,105 @@ mod tests { }; assert_eq!(sealed.ratchet_counter, 0); } + + // ── validate() bounds ─────────────────────────────────────────────────── + + fn file_content(filename: &str, mime_type: &str) -> Content { + Content::File { + hash: "deadbeef".into(), + filename: filename.into(), + mime_type: mime_type.into(), + size_bytes: 0, + } + } + + #[test] + fn validate_rejects_oversized_filename() { + let too_long = "a".repeat(MAX_FILENAME_BYTES + 1); + let content = file_content(&too_long, "image/png"); + assert_eq!( + content.validate(), + Err(MessageValidationError::FilenameTooLong { + actual: MAX_FILENAME_BYTES + 1, + max: MAX_FILENAME_BYTES, + }) + ); + } + + #[test] + fn validate_rejects_oversized_mime_type() { + let too_long = "a".repeat(MAX_MIME_BYTES + 1); + let content = file_content("ok.txt", &too_long); + assert_eq!( + content.validate(), + Err(MessageValidationError::MimeTypeTooLong { + actual: MAX_MIME_BYTES + 1, + max: MAX_MIME_BYTES, + }) + ); + } + + #[test] + fn validate_accepts_filename_at_boundary() { + let at_cap = "a".repeat(MAX_FILENAME_BYTES); + let mime_at_cap = "a".repeat(MAX_MIME_BYTES); + let content = file_content(&at_cap, &mime_at_cap); + assert!(content.validate().is_ok()); + } + + #[test] + fn validate_accepts_empty_filename_and_mime() { + let content = file_content("", ""); + assert!(content.validate().is_ok()); + } + + #[test] + fn validate_is_noop_for_non_file_variants() { + let cases = [ + Content::Text { + body: "x".repeat(1024), + }, + Content::Reaction { + target: MessageId::new(), + emoji: "👍".into(), + }, + Content::Reply { + parent: MessageId::new(), + body: "ok".into(), + }, + Content::Edit { + target: MessageId::new(), + new_body: "edited".into(), + }, + Content::Delete { + target: MessageId::new(), + }, + Content::System { + description: "joined".into(), + }, + Content::Encrypted(SealedContent { + ciphertext: vec![1, 2, 3], + nonce: [0u8; 12], + key_epoch: 0, + ratchet_counter: 0, + }), + ]; + for content in &cases { + assert!(content.validate().is_ok(), "expected ok for {content:?}"); + } + } + + #[test] + fn message_validate_delegates_to_content() { + let mut hlc = hlc::HLC::new(); + let peer = Identity::generate().endpoint_id(); + let channel = ChannelId::new(); + let too_long = "a".repeat(MAX_FILENAME_BYTES + 1); + + let msg = Message::file(channel, peer, "hash", too_long, "image/png", 42, &mut hlc); + assert!(matches!( + msg.validate(), + Err(MessageValidationError::FilenameTooLong { .. }) + )); + } } diff --git a/crates/messaging/src/store.rs b/crates/messaging/src/store.rs index 5ae7d8e0..edbf34ee 100644 --- a/crates/messaging/src/store.rs +++ b/crates/messaging/src/store.rs @@ -10,7 +10,7 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use willow_identity::EndpointId; -use crate::{hlc::HlcTimestamp, ChannelId, Message, MessageId}; +use crate::{hlc::HlcTimestamp, ChannelId, Message, MessageId, MessageValidationError}; /// Delivery state for a single message. /// @@ -48,6 +48,12 @@ pub enum StoreError { /// The requested message was not found. #[error("message not found: {0}")] NotFound(MessageId), + + /// Attempted to insert a message that failed structural validation + /// (e.g. peer-supplied `Content::File` with an oversized filename + /// or MIME type). See [`MessageValidationError`] for the variants. + #[error("invalid message: {0}")] + Invalid(#[from] MessageValidationError), } /// Trait for message storage backends. @@ -184,6 +190,18 @@ impl InMemoryStore { impl MessageStore for InMemoryStore { fn insert(&mut self, message: Message) -> Result<(), StoreError> { + // Structural validation guards against unbounded peer-supplied + // `Content::File` strings (filename, mime_type) being persisted + // without ever passing through a length check. Tracked in #583. + // + // NOTE: Ideally validation also gates earlier, at the network + // ingress in `crates/client/src/listeners.rs`, before a `Message` + // ever reaches a store. Those files are in flight under PR #566 + // (coordinator decision); revisit after merge so peers can't + // pin oversized payloads in client memory even when the store + // is bypassed. + message.validate()?; + if self.messages.contains_key(&message.id) { return Err(StoreError::DuplicateId(message.id)); } diff --git a/crates/state/src/dag.rs b/crates/state/src/dag.rs index 8fceefae..8d275897 100644 --- a/crates/state/src/dag.rs +++ b/crates/state/src/dag.rs @@ -127,18 +127,21 @@ impl EventDag { /// The first event must be `EventKind::CreateServer` with seq=1 and /// prev=ZERO. Unknown deps are silently accepted (soft-accept). pub fn insert(&mut self, event: Event) -> Result<(), InsertError> { - // 1. Verify signature. - if !event.verify() { - return Err(InsertError::InvalidSignature); - } - - // 2. Anti-DoS vector caps (SEC-V-07). + // 1. Anti-DoS vector caps (SEC-V-07). // // A peer holding any permission could otherwise broadcast events // with pathologically large `deps` or `encrypted_keys` blobs that // every other peer would clone into a `BTreeMap` during apply. // Reject at the inbound DAG boundary so over-cap events never // even reach `applied_events` / `materialize`. + // + // ORDERING: cheap structural caps first; signature verify last to + // short-circuit DoS. Ed25519 + bincode + blake3 verify costs ~50µs + // per event — an attacker with `SendMessages` could otherwise force + // every receiver to pay that cost on each over-cap event before + // rejection. The syntactic checks here are O(1) / O(n) over already- + // deserialized fields, so running them first lets us drop pathological + // payloads without ever invoking the expensive crypto path. if event.deps.len() > MAX_EVENT_DEPS { return Err(InsertError::DepsTooLong { got: event.deps.len(), @@ -156,6 +159,11 @@ impl EventDag { } } + // 2. Verify signature. + if !event.verify() { + return Err(InsertError::InvalidSignature); + } + // 3. Check duplicate. if self.events.contains_key(&event.hash) { return Err(InsertError::Duplicate); @@ -681,6 +689,48 @@ mod tests { assert!(matches!(err, InsertError::InvalidSignature)); } + /// SEC-V-07 regression: structural caps must short-circuit BEFORE the + /// expensive Ed25519 verify path, so an attacker with `SendMessages` + /// permission cannot force receivers to pay sig-verify cost on + /// pathologically over-cap events. Construct an event whose signature + /// is obviously bogus AND whose deps exceed `MAX_EVENT_DEPS`; we expect + /// `DepsTooLong` (not `InvalidSignature`), which proves the cheap cap + /// was checked first. + #[test] + fn insert_deps_cap_rejects_before_signature_verify() { + use willow_identity::Signature; + + let id = Identity::generate(); + let mut dag = test_dag(&id); + + // Build an event with deps.len() > MAX_EVENT_DEPS, then clobber the + // signature with all-zero bytes so that any attempt to verify would + // fail. If verify ran first we'd see `InvalidSignature`; if the + // structural cap runs first we get `DepsTooLong`. + let oversize_deps = vec![EventHash::ZERO; MAX_EVENT_DEPS + 1]; + let mut event = dag.create_event( + &id, + EventKind::SetProfile { + display_name: "dos".into(), + }, + oversize_deps, + 0, + ); + event.sig = Signature::from_bytes(&[0u8; 64]); + + let err = dag.insert(event).unwrap_err(); + assert!( + matches!( + err, + InsertError::DepsTooLong { + got, + max: MAX_EVENT_DEPS, + } if got == MAX_EVENT_DEPS + 1 + ), + "expected DepsTooLong (cheap cap first), got {err:?}", + ); + } + #[test] fn insert_rejects_seq_gap() { let id = Identity::generate(); diff --git a/crates/state/src/tests/materialize.rs b/crates/state/src/tests/materialize.rs index 134f884e..610f61a0 100644 --- a/crates/state/src/tests/materialize.rs +++ b/crates/state/src/tests/materialize.rs @@ -46,7 +46,7 @@ fn non_admin_set_profile_is_accepted() { let alice = Identity::generate(); // Grant SendMessages so alice is a member. - do_emit( + let grant = do_emit( &mut dag, &admin, EventKind::GrantPermission { @@ -55,13 +55,18 @@ fn non_admin_set_profile_is_accepted() { }, ); // Alice sets her own profile (no admin needed). - do_emit( - &mut dag, + // Causal dep on grant ensures topo-sort applies GrantPermission first + // (PR #505's membership gate); without this dep the test was flaky + // depending on HashMap iter order. See issue #565. + let set_profile = dag.create_event( &alice, EventKind::SetProfile { display_name: "Alice".to_string(), }, + vec![grant.hash], + 0, ); + dag.insert(set_profile).unwrap(); let state = materialize(&dag); assert_eq!( diff --git a/crates/web/index.html b/crates/web/index.html index 11506818..db43d79c 100644 --- a/crates/web/index.html +++ b/crates/web/index.html @@ -21,11 +21,15 @@ relay's /bootstrap-id HTTP endpoint. * img-src and media-src include data:/blob: so that avatar data URIs and runtime URL.createObjectURL blobs (file attachments, - inline media) render. frame-ancestors 'none' blocks clickjacking; + inline media) render. img-src additionally allows https: so the + chat auto-embed code path (see issue #584 / PR #243, which added + referrerpolicy="no-referrer" to external tags) can fetch + remote images over HTTPS without a mixed-content downgrade. + frame-ancestors 'none' blocks clickjacking; base-uri / form-action / object-src lock down the remaining injection surfaces. --> - + diff --git a/crates/web/tests/static_assets.rs b/crates/web/tests/static_assets.rs index 593d9af3..cd44c083 100644 --- a/crates/web/tests/static_assets.rs +++ b/crates/web/tests/static_assets.rs @@ -130,7 +130,7 @@ const REQUIRED_CSP_DIRECTIVES: &[&str] = &[ // ws/wss for relay transport, https for the relay HTTP bootstrap probe. "connect-src 'self' ws: wss: https:", // data: for avatar URIs, blob: for runtime createObjectURL attachments. - "img-src 'self' data: blob:", + "img-src 'self' https: data: blob:", "media-src 'self' blob:", "worker-src 'self'", "object-src 'none'",