fix: use transport peer ID as P2PEvent source for correct message routing

[mickvandijke]

Summary

• P2PEvent::Message was using the logical from field from the JSON message payload as the source, but send_message() looks up peers by their transport-level peer ID
• This mismatch meant consumers receiving a P2PEvent::Message could not reply using the source field
• Now converts the transport PeerId to the hex string used in the peers map and uses that as the event source

Test plan

• Verify cargo test passes
• Verify cargo clippy -- -D warnings -D clippy::unwrap_used -D clippy::expect_used passes
• Confirm round-trip message routing works with the corrected peer ID

🤖 Generated with Claude Code

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

[mickvandijke]

@claude please review this PR

[claude]

Claude finished @mickvandijke's task in 1m 36s —— View job

---

Code Review ✅

Summary

This PR correctly fixes a critical message routing bug. The change ensures that P2PEvent::Message.source uses the transport-level peer ID (from the authenticated QUIC connection) instead of the logical "from" field from the JSON payload. This enables consumers to reply using send_message() with the event source and eliminates a potential identity spoofing vector.

What Changed

1. New helper function parse_protocol_message() (lines 1750-1784):

   • Extracts protocol message parsing into a reusable function
   • Takes transport peer ID as a parameter and uses it as the authoritative source
   • Still validates that "from" field exists (required by wire protocol) but doesn't use it for routing
   • Returns Option<P2PEvent> for clean error handling
   • Excellent documentation explaining the security rationale

2. Refactored message receive loop (lines 1229-1256):

   • Converts ant-quic PeerId to hex string at reception point using ant_peer_id_to_string()
   • Passes transport peer ID to parse_protocol_message()
   • Simplified error handling with cleaner logging

3. Code cleanup:

   • Removed unused handle_received_message() method (54 lines)
   • Moved KEEPALIVE_PAYLOAD constant to module level for reusability
   • Consolidated imports for interval and Duration
   • Simplified repeated ant_peer_id_to_string() calls throughout

4. Comprehensive test coverage (98 lines of new tests):

   • Regression test confirming transport peer ID is used as source
   • Tests for all required field validations (from, protocol, data)
   • Edge case: invalid JSON, empty payloads
   • Clear documentation of the spoofing prevention requirement

CLAUDE.md Compliance ✅

• No panics in production code: All .unwrap(), .expect(), and panic!() calls are confined to test functions (#[test])
• Proper error handling: Uses Option<P2PEvent> return type with the ? operator for clean error propagation
• Clean code: No println!() - uses tracing macros (debug!, info!, warn!, trace!)

Code Quality ✅

• Security improvement: Prevents peer ID spoofing by using authenticated transport layer identity
• Better separation of concerns: Protocol parsing now isolated in a dedicated function
• Excellent documentation: The function doc comments clearly explain the security rationale
• Strong test coverage: Tests validate both the fix and guard against regressions
• No duplication: Eliminated repeated inline parsing logic

Issues Found

None. This is a well-implemented fix with proper testing and documentation.

Recommendations

This PR is ready to merge. The fix correctly addresses the routing mismatch while improving code quality and security.

--- |

[dirvine]

Code Review: PR #9 - fix: use transport peer ID as P2PEvent source

Hey @mickvandijke, excellent security fix! Here's my comprehensive review including ADR compliance check.

---

📝 Summary

This PR fixes a security issue where P2PEvent::Message.source was using the from field from the JSON payload (application layer), allowing peer ID spoofing. Now it correctly uses the transport-level peer ID from the authenticated QUIC connection.

---

✅ ADR Compliance Check

ADR-001: Multi-Layer Architecture ✅ COMPLIANT

The fix aligns perfectly with the layer separation principle:

"Dependencies flow downward only: Application → Identity → Trust/Placement → DHT → Adaptive → Transport"

Before: P2PEvent source used application-layer from field (could be spoofed)
After: Uses transport-layer peer ID from ant-quic (authenticated by QUIC handshake)

This correctly respects the boundary between layers - the transport layer's authenticated peer ID is the source of truth, not application-layer claims.

ADR-002: Delegated Transport via ant-quic ✅ COMPLIANT

The fix properly uses the adapter layer:

use crate::transport::ant_quic_adapter::{DualStackNetworkNode, ant_peer_id_to_string};
// ...
let transport_peer_id = ant_peer_id_to_string(&peer_id);

This follows the ADR-002 pattern: "The adapter layer translates between saorsa-core abstractions and ant-quic primitives."

---

🔒 Security Improvements

This is a significant security fix. The old code trusted the from field in the JSON payload:

// OLD (vulnerable)
source: from.to_string(),  // Attacker could spoof any peer ID

The new code uses the authenticated transport peer ID:

// NEW (secure)
source: source.to_string(),  // From QUIC connection, can't be spoofed

The docstring correctly documents this:

"This eliminates a spoofing vector where a peer could claim an arbitrary identity via the JSON payload."

---

✅ What Looks Good

1. Clean refactoring: New parse_protocol_message() function is a pure function with clear responsibilities
2. Comprehensive tests: 6 new unit tests covering:
   • Transport peer ID used as source (regression test)
   • Missing from field rejected
   • Missing protocol field rejected
   • Missing data field rejected
   • Invalid JSON rejected
   • Empty payload handled
3. Dead code removal: Removed unused handle_received_message function
4. Constant extraction: KEEPALIVE_PAYLOAD moved to module-level constant
5. Lifecycle separation: new() vs start() pattern (builds foundation for PR fix: remove duplicate receive loops in P2PNode #10)

---

⚠️ Minor Observations

1. The from field is still required in wire protocol

// "from" is a required protocol field (messages without it are dropped),
// but we use the transport peer ID as the authoritative source.
let logical_from = value.get("from")?.as_str()?;

This is correct - requiring from maintains backward compatibility with the wire protocol while ignoring its value for security purposes. The debug log shows both for debugging:

debug!("... source: {} (logical: {})", source, logical_from, ...);

2. Import organization

The interval import was moved from local scope to module level - this is a minor cleanup that's fine.

---

📋 Merge Checklist

• Builds cleanly
• Clippy passes (strict mode)
• Tests pass
• ADR-001 compliant (layer separation)
• ADR-002 compliant (transport adapter usage)
• Security improvement (prevents spoofing)
• CI integration tests

---

✅ Verdict

Approved - This is a well-executed security fix that:

1. Prevents peer ID spoofing attacks
2. Correctly respects the multi-layer architecture (ADR-001)
3. Properly uses the transport adapter (ADR-002)
4. Includes comprehensive regression tests
5. Cleans up dead code

This should be merged before PR #10, which is correctly stacked on top of this branch.

🤖 Reviewed by Claude