fix(node): treat missing delta envelope signature as verification failure

[chefsale]

Replaces if let Some(sig) guards with hard failures on None at all
four delta verification sites:

• apply_authorized_state_delta (gossip path)
• verify_fetched_parent in delta_request (DAG parent-pull)
• request_dag_heads_and_sync in manager (DAG head-pull)
• replay_buffered_delta (snapshot-sync replay)

Previously an attacker could strip the signature field and bypass the
anti-impersonation check entirely, then rely on membership_status_at
keying off the attacker-supplied author_id to pass authorization.

Genesis deltas are unaffected: all four sites already return early via
the all-zeros author sentinel before reaching the signature check.

[meroreviewer]

🤖 MeroReviewer

Reviewed by 1 agents | Quality score: 28% | Review time: 59.5s

---

🟡 Warning (1)

1. Silent drop on missing signature returns Ok(()) instead of a distinct error

File: crates/node/src/handlers/state_delta/mod.rs (line 177-215) | Consensus: 1/1 agents ✓

All four rejection sites (gossip, parent-fetch, DAG-catchup, buffered-replay) return Ok(()) / Ok(false) on a missing signature, which is indistinguishable from a normal 'nothing to do' path. A caller that checks the return value for success cannot tell whether the delta was applied, skipped legitimately, or silently dropped due to a stripped signature. This makes it harder to detect an active stripping attack in metrics/monitoring and could mask bugs where signatures are accidentally omitted by a legitimate sender.

Suggested fix:

Consider returning a typed rejection (e.g. a new `DeltaRejected` variant or a dedicated `Err`) so callers and metrics can distinguish 'applied', 'pending', and 'rejected' outcomes. At minimum, increment a rejection counter metric here so the rate of missing-signature drops is observable.

Found by: security-reviewer

💡 Suggestion (2)

1. Missing-signature rejection does not record the source peer

File: crates/node/src/handlers/state_delta/mod.rs (line 177-195) | Consensus: 1/1 agents ✓

The warn! log for a missing envelope signature omits the source (PeerId) field that is available in scope at this call site. An attacker stripping signatures from relayed deltas would be invisible in logs — only context_id, author_id, and delta_id are recorded. The gossip path has source in scope from the destructured StateDeltaMessage.

Suggested fix:

Add `%source` to the `warn!` fields: `warn!(%context_id, %author_id, %source, delta_id = ?delta_id, "Rejecting state delta — missing envelope signature");`

Found by: security-reviewer

2. verify_fetched_parent skips membership check on missing-signature path but not on decode-error path

File: crates/node/src/sync/delta_request.rs (line 131-170) | Consensus: 1/1 agents ✓

When governance_position_blob fails to decode (line ~120), VerifiedParent::Skip is returned before the signature check. When the signature is None (line ~145), VerifiedParent::Skip is also returned. Both are correct rejections, but the decode-error path logs %e while the missing-signature path does not log the delta_id consistently — delta_id is logged as delta_id = ?delta_id (Debug) in the missing-sig branch but as delta_id = ?delta_id in the decode-error branch too. This is consistent. However, the function signature takes delta_id: [u8; 32] by value while fetched.delta.id is the canonical id — the caller passes missing_id which was the requested id, not the received id. If a peer substitutes a different delta body (different fetched.delta.id) the signature check would verify against missing_id (the requested id) rather than fetched.delta.id (the actual id), potentially accepting a valid signature over the wrong payload.

Suggested fix:

Verify the signature against `fetched.delta.id` (the actual received delta id) rather than the caller-supplied `delta_id` (the requested id). The id-mismatch sanity check that follows in the caller (`request_missing_deltas`) catches the substitution after the fact, but the signature should bind the actual payload id. Change the `verify_delta_signature` call to use `fetched.delta.id` and move the id-mismatch check before the signature verification.

Found by: security-reviewer

---

_{🤖 Generated by MeroReviewer | Review ID: review-aa8766f6}

[meroreviewer]

Documentation Review

The following documentation may need updates based on the changes in this PR:

• 🟡 architecture/crates/node.html: Files matching crates/node/** were changed but architecture/crates/node.html was not updated (per source_to_docs_mapping).
• 🟡 architecture/crates/sync.html: Files matching crates/node/** were changed but architecture/crates/sync.html was not updated (per source_to_docs_mapping).

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 47% | Review time: 149.7s

---

🟡 Warning (2)

1. Silent drop on missing signature returns Ok(()) instead of a distinct error

File: crates/node/src/handlers/state_delta/mod.rs (line 177-215) | Consensus: 1/2 agents

All four rejection sites (gossip, parent-fetch, DAG-catchup, replay) return Ok(()) / Ok(false) on a missing signature, which is indistinguishable from a successful no-op. A caller that checks only the Result variant cannot tell whether the delta was applied, silently skipped due to a missing signature, or skipped for any other reason. This makes it impossible to surface the rejection in metrics or to distinguish it from normal processing in tests. The PR description says this is a security fix, but the fix is silent — an operator watching logs will see a warn! but no counter increment or distinct return value.

Suggested fix:

Consider returning a typed rejection (e.g. a dedicated `DeltaRejected` variant or at minimum recording a metric via `crate::node_metrics::record_delta_outcome("missing_signature")`) so the rejection is observable in dashboards, not just in log lines.

Found by: security-reviewer

2. Signature borrow tied to moved delta_signature field

File: crates/node/src/handlers/state_delta/mod.rs (line 196-203) | Consensus: 1/2 agents

The sig borrow is taken as Some(ref s) => s from delta_signature, which is a field of the destructured message. Later at line ~260, delta_signature is moved into the BufferedDelta constructor (the HLC fence path). Because sig is a reference into the local delta_signature binding, the borrow must end before the move. In the current layout the borrow does end before the move (the verify_delta_signature call completes before the fence block), so this compiles. However, the sig variable is a &[u8; 64] pointing into the Option<[u8; 64]> on the stack — if the code is ever refactored to extend sig's lifetime (e.g. storing it for a second use after the fence), the borrow-vs-move tension will cause a compile error that is non-obvious to diagnose. A cleaner pattern is to copy the 64-byte array out: let sig = match delta_signature { Some(s) => s, None => { … } }; (no ref), then pass &sig to verify_delta_signature. This makes the ownership explicit and removes the latent fragility.

Suggested fix:

Change `Some(ref s) => s` to `Some(s) => s` (copy the `[u8; 64]` value out) and pass `&sig` to `verify_delta_signature`. Repeat the same pattern for `sig_for_parent`, `sig_for_replay`, and `sig_for_head` in the other three sites.

Found by: patterns-reviewer

💡 Suggestion (3)

1. DAG-catchup head-pull missing-signature rejection does not increment any metric

File: crates/node/src/sync/manager/mod.rs (line 2250-2290) | Consensus: 1/2 agents

The new None => { warn!(...); continue; } arm in request_dag_heads_and_sync correctly rejects the delta, but the heads_attempted counter is incremented BEFORE the signature check (it is incremented at the top of the DeltaResponse arm). A delta rejected for a missing signature therefore increments heads_attempted but not heads_admitted, which will trigger the all-rejected bail-out (heads_attempted > 0 && heads_admitted == 0) if ALL heads have stripped signatures. That bail-out path is correct behaviour, but the error message blames "signature / group-id / membership" without distinguishing the missing-signature case. The log message at the bail-out site should be updated to also mention missing signatures.

Suggested fix:

Update the bail-out message at the `heads_attempted > 0 && heads_admitted == 0` check to read: "…rejected by the apply-time verify chain (missing or invalid signature / group-id / membership)…"

Found by: security-reviewer

2. No test coverage for the new None-signature rejection path in verify_fetched_parent

File: crates/node/src/sync/delta_request.rs (line 131-175) | Consensus: 1/2 agents

The PR adds a hard-failure branch for None signatures in verify_fetched_parent, but the existing test suite (visible in the diff and the full file) has no unit test that constructs a FetchedDelta with delta_signature: None and a non-genesis author and asserts VerifiedParent::Skip is returned. The gossip path has inline tests in mod.rs; this function has none. A regression (e.g. accidentally re-introducing the if let Some guard) would be invisible until an integration test caught it.

Suggested fix:

Add a unit test in `delta_request.rs` (or a sibling `tests` module) that calls `verify_fetched_parent` with a `FetchedDelta` whose `delta_signature` is `None` and `author_id` is not the genesis sentinel, and asserts the result is `VerifiedParent::Skip`.

Found by: patterns-reviewer

3. verify_fetched_parent skips membership check when signature is missing but logs at warn level only

File: crates/node/src/sync/delta_request.rs (line 131-175) | Consensus: 1/2 agents

The new None arm in verify_fetched_parent returns VerifiedParent::Skip with a warn! log, which is correct. However, the function is fn (not async) and has no way to increment a metric counter. The DAG-catchup path is the most likely vector for a stripped-signature attack because it is triggered by a peer-controlled response, yet it produces no observable signal beyond a log line. At minimum, a crate::node_metrics::record_delta_outcome call here would make the attack detectable in production.

Suggested fix:

Add `crate::node_metrics::record_delta_outcome("missing_signature_dag_catchup");` before `return VerifiedParent::Skip;` in the new `None` arm.

Found by: security-reviewer

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-fbfc8395}

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 37% | Review time: 161.0s

🟡 3 warnings. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-c9918563}

[meroreviewer]

🟡 Signature borrow tied to moved delta_signature field

The sig binding borrows delta_signature via Some(ref s) => s, but delta_signature is later moved into two separate BufferedDelta struct literals (the HLC-fence absorb closure at ~line 255 and the snapshot-buffer path at ~line 290). Because sig is only used immediately in the verify_delta_signature call that follows the match, the borrow ends before those moves, so this compiles today. However, the pattern is fragile: if any code is inserted between the match and the verify_delta_signature call that also borrows or moves delta_signature, the compiler will reject it with a confusing "cannot move out of delta_signature because it is borrowed" error. The existing if let Some(ref sig) pattern avoided this entirely. Consider cloning the signature bytes into a local [u8; 64] before the match so the binding is independent of the original field: let sig_bytes: [u8; 64] = delta_signature.ok_or_else(|| { warn!(...); }).map_err(|_| return Ok(()))?; — or restructure to extract and verify before any subsequent use of delta_signature.

Suggested fix:

Extract the signature early: `let sig_bytes = match delta_signature { Some(s) => s, None => { warn!(...); return Ok(()); } };` — this moves the bytes out of the Option into a local `[u8; 64]`, eliminating the borrow-while-later-moved fragility and making subsequent moves of the enclosing struct fields unconditional.

[meroreviewer]

🟡 Signature borrowed from moved delta_signature field

The delta_signature field is destructured out of message as Option<[u8; 64]>. The match arm borrows s from delta_signature with Some(ref s), so sig is &[u8; 64] with a lifetime tied to the local delta_signature variable. This is fine for the immediate verify_delta_signature call, but the same delta_signature value is later moved into two BufferedDelta struct literals (lines ~248 and ~280). Because sig is a borrow of delta_signature, the compiler will reject any attempt to move delta_signature while sig is still live. In practice the borrow ends before those moves, but the pattern is fragile: if the verify block is ever refactored to extend sig's lifetime (e.g. storing it in a struct), the moves will silently fail to compile. The safer pattern is to copy the bytes: let sig = match delta_signature { Some(s) => s, None => { … } }; and pass &sig to verify_delta_signature. This also makes the ownership flow explicit.

Suggested fix:

Change `Some(ref s) => s` to `Some(s) => s` (copy the `[u8; 64]` value) and pass `&sig` to `verify_delta_signature`. The `[u8; 64]` is `Copy`, so this is zero-cost and removes the borrow-before-move fragility.

[meroreviewer]

🟡 Signature borrow tied to moved response_delta_signature field

Same pattern as the other two sites: sig_for_head borrows response_delta_signature via Some(ref s) => s. The borrow ends before the add_delta call that consumes response_delta_signature, so it compiles. But the correctness depends on the borrow ending before the move, which is a non-local invariant. Extracting by value removes the dependency.

Suggested fix:

Use `let sig_for_head = match response_delta_signature { Some(s) => s, None => { warn!(...); continue; } };` and pass `Some(sig_for_head)` to `add_delta`.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 44% | Review time: 275.3s

---

🟡 Warning (4)

1. Buffering path stores delta_signature: None after signature is consumed by move

File: crates/node/src/handlers/state_delta/mod.rs (line 253-260) | Consensus: 1/2 agents

At line ~253, delta_signature is destructured from StateDeltaMessage and moved into sig via the match. Later, when constructing BufferedDelta for the HLC-fence absorb path (around line 270) and the snapshot-sync buffer path (around line 320), the code writes delta_signature — but that binding is now None because the Some(s) arm moved the value into sig. The original delta_signature field is gone. The buffered record therefore always carries delta_signature: None, which means when it is later replayed through replay_buffered_delta it will be immediately rejected by the new None-is-failure gate added in this very PR — creating a silent infinite-drop loop for any delta that hits the HLC-fence or snapshot-buffer path.

Suggested fix:

Before the `match`, clone or copy the signature for the buffer sites: `let delta_signature_for_buffer = delta_signature;` then `let sig = match delta_signature_for_buffer { Some(s) => s, None => { ... return Ok(()); } };`. Use `delta_signature_for_buffer` (which is `Option<[u8;64]>`) in every `BufferedDelta` constructor, and `&sig` for the `verify_delta_signature` call. Since `[u8; 64]` is `Copy`, this is zero-cost.

Found by: patterns-reviewer

2. Buffered delta's delta_signature field is None after move into sig_for_replay

File: crates/node/src/handlers/state_delta/mod.rs (line 1743-1780) | Consensus: 1/2 agents

In replay_buffered_delta, buffered.delta_signature is moved out of buffered into sig_for_replay via the match. Later, when add_delta_with_events is called (around line 1870), buffered.delta_signature is passed again — but it is now None because the Some(s) arm already moved the value. The persisted DAG row therefore always has delta_signature: None, breaking the relay invariant (subsequent DAG-catchup serves from this node will return DeltaNotFound for this row because effective_author resolves to None for non-genesis rows without a signature, or the row is served without a signature and downstream peers reject it under the new policy).

Suggested fix:

Copy the signature before the match: `let sig_for_replay = match buffered.delta_signature { Some(s) => s, None => { ... } };` and then pass `Some(sig_for_replay)` (or a copy of it) to `add_delta_with_events`. Since `[u8; 64]` is `Copy`, `Some(sig_for_replay)` is sufficient.

Found by: patterns-reviewer

3. Moved response_delta_signature into sig_for_parent but then passed as response_delta_signature to fetched_deltas.push

File: crates/node/src/handlers/state_delta/mod.rs (line 1357-1400) | Consensus: 1/2 agents

In request_missing_deltas, response_delta_signature is moved into sig_for_parent via the match. The fetched_deltas.push call at the end of the arm (around line 1430) passes response_delta_signature — which is now None because the Some(s) arm consumed it. The persisted row therefore always has delta_signature: None, and subsequent DAG-catchup serves from this node will fail the new None-is-failure check on downstream peers.

Suggested fix:

After extracting `sig_for_parent`, reconstruct the option for storage: pass `Some(sig_for_parent)` to `fetched_deltas.push` instead of `response_delta_signature`. Since `[u8; 64]` is `Copy`, this is `Some(sig_for_parent)`.

Found by: patterns-reviewer

4. Moved sig value used after delta_signature is consumed

File: crates/node/src/handlers/state_delta/mod.rs (line 237-237) | Consensus: 1/2 agents

After the match delta_signature { Some(s) => s, None => ... } block, delta_signature is moved into sig. Later in the function (around line 295 and line 330), delta_signature is used again in BufferedDelta struct literals (e.g., delta_signature, in the HLC fence closure and the buffering path). Because delta_signature was moved into sig, these later uses would be a compile error unless sig is Copy (which [u8; 64] is). Since [u8; 64] implements Copy, this is not a runtime bug, but the code now passes sig (the extracted value) to verify_delta_signature while the original delta_signature field (which is Option<[u8; 64]>) is used in the BufferedDelta constructors. The BufferedDelta constructors use the original delta_signature binding which is now None-checked — but since Option<[u8; 64]> is also Copy, the original delta_signature variable is still accessible after the match (it was copied, not moved). This is subtle: if the type were not Copy, this would be a use-after-move bug. The code is correct for [u8; 64] but fragile — a future type change could silently break it. Consider using delta_signature.as_ref() or restructuring to make the dependency explicit.

Suggested fix:

Store the result as `let sig = delta_signature?` style or keep `delta_signature` as `Option<[u8;64]>` and pass `sig` only to the verify call, making it clear the original `Option` is still available for the `BufferedDelta` constructors.

Found by: security-reviewer

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-b9de329f}

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 45% | Review time: 166.6s

---

🟡 Warning (3)

1. Buffered delta stores unverified delta_signature field after signature check

File: crates/node/src/handlers/state_delta/mod.rs (line 248-260) | Consensus: 1/2 agents

After the signature is verified at line ~215, the code later constructs BufferedDelta structs (e.g. lines ~248-260 in the HLC fence buffer path and ~280-295 in the snapshot-sync buffer path) using delta_signature from the destructured StateDeltaMessage. However, delta_signature was moved into sig via the match at line ~215 (let sig = match delta_signature { Some(s) => s, ... }). The original delta_signature binding is now None (it was consumed). When the code later writes delta_signature into the BufferedDelta, it writes None — meaning the buffered delta loses its signature. When that delta is later replayed via replay_buffered_delta, the new None-is-failure check will reject it. This is a logic regression introduced by this PR: the fix that prevents bypass also silently breaks the buffer→replay path for legitimate deltas.

Suggested fix:

After extracting `sig` from `delta_signature`, reconstruct the `Option<[u8;64]>` for downstream use: `let delta_signature_opt = Some(sig);` and use `delta_signature_opt` wherever `delta_signature` is referenced in `BufferedDelta` construction. Alternatively, keep `delta_signature` as `Option` and only borrow `sig` as a reference: `let sig = delta_signature.as_ref().ok_or(...)?` or use `if let Some(ref sig) = delta_signature` for the verify call while keeping `delta_signature` intact for later use.

Found by: security-reviewer

2. Buffered delta replay rejects all pre-existing buffered deltas that lack a signature

File: crates/node/src/handlers/state_delta/mod.rs (line 1743-1760) | Consensus: 1/2 agents

The replay_buffered_delta function now hard-fails on buffered.delta_signature == None. However, any delta that was buffered by a node running the OLD code (before this PR) will have been stored with delta_signature: None if the original gossip path used the old if let Some(ref sig) guard (which tolerated None). On upgrade, all such previously-buffered deltas will be permanently rejected with 'missing envelope signature' and Ok(false) returned, silently dropping them. This is a backward-compatibility break for in-flight buffered deltas across a rolling upgrade.

Suggested fix:

Consider adding a migration grace period or a separate log/metric for the `None` case that distinguishes 'legacy buffered delta (pre-signing)' from 'actively stripped signature'. Alternatively, document that a node upgrade requires draining the buffer first, and add a startup warning if any buffered deltas lack signatures.

Found by: security-reviewer

3. Buffered delta constructed with delta_signature: delta_signature after signature is moved

File: crates/node/src/handlers/state_delta/mod.rs (line 258-263) | Consensus: 1/2 agents

At line ~258, apply_authorized_state_delta destructures delta_signature from message and then moves it into sig via the match. Later, when constructing BufferedDelta inside the HLC fence closure (the || calimero_node_primitives::delta_buffer::BufferedDelta { ... } lambda), the field delta_signature is set to delta_signature — but delta_signature was already moved into sig. The same pattern repeats for the second BufferedDelta construction in the snapshot-buffer branch. This would be a compile error if [u8; 64] were not Copy, but since it is Copy the compiler silently copies sig back. The intent is clearly to store the original Option<[u8; 64]> in the buffer, but after the match the local binding delta_signature no longer exists — the code is storing sig (the unwrapped [u8; 64]) wrapped back into Some implicitly via the copy. Verify that the buffered delta always carries Some(sig) and never None at this point, which is correct post-fix, but the variable name delta_signature used in the struct literal is now a use-after-move of the original Option binding. If the compiler accepts it, it is because [u8; 64]: Copy makes sig copyable back; document this or rename to Some(sig) explicitly to make the intent unambiguous.

Suggested fix:

In the `BufferedDelta` struct literals that follow the signature check, replace `delta_signature` (the now-consumed `Option`) with `Some(sig)` to make the intent explicit and avoid relying on implicit Copy semantics of the inner array.

Found by: patterns-reviewer

💡 Suggestion (2)

1. Moved delta_signature into sig makes later delta_signature references compile-error or silently use moved value

File: crates/node/src/handlers/state_delta/mod.rs (line 215-230) | Consensus: 1/2 agents

The destructuring let StateDeltaMessage { ..., delta_signature, ... } = message; binds delta_signature: Option<[u8;64]>. The subsequent let sig = match delta_signature { Some(s) => s, None => { ... return Ok(()); } }; moves the inner value out of the Option, consuming delta_signature. Any later reference to delta_signature in the same scope (e.g. in BufferedDelta { delta_signature, ... }) would either fail to compile (if [u8;64] is not Copy) or silently use None (if the compiler allows it). Since [u8;64] IS Copy, the Some(s) arm copies s out, leaving delta_signature as a moved-from Option — subsequent uses see the original binding, which is now partially moved. This needs careful audit to confirm the buffered delta carries Some(sig) not None.

Suggested fix:

Explicitly reconstruct: `let delta_signature_for_buffer = Some(sig);` immediately after the match, and use that in all `BufferedDelta` constructions below.

Found by: security-reviewer

2. Signature variable sig_for_replay is consumed by verify_delta_signature but buffered.delta_signature is used later for DAG persistence

File: crates/node/src/handlers/state_delta/mod.rs (line 1743-1780) | Consensus: 1/2 agents

In replay_buffered_delta, the code extracts sig_for_replay from buffered.delta_signature via a match that moves the value. Later, when calling delta_store.add_delta_with_events(...), the field buffered.delta_signature is passed again. Since buffered is a struct (not a reference) and delta_signature is Option<[u8; 64]> where [u8; 64]: Copy, the compiler allows this via copy. However, after the match moves sig_for_replay out of buffered.delta_signature, buffered.delta_signature is None for the rest of the function — meaning the DAG persistence call receives None for delta_signature, defeating the purpose of persisting the verified signature for downstream serves. This is a latent correctness bug: the absorb-buffer path and the normal DAG-add path both pass buffered.delta_signature which is now None.

Suggested fix:

Store the signature before the match: `let verified_sig = buffered.delta_signature;` then use `verified_sig` for both the verification call and the later persistence calls, keeping `buffered.delta_signature` intact. Or reconstruct `Some(sig_for_replay)` when passing to `add_delta_with_events`.

Found by: patterns-reviewer

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-e49ef29b}