ConnectResponse sent to joiner's private address instead of observed external address

[sanity]

Summary

When a peer behind NAT joins the network, the gateway correctly observes their external address from the UDP socket, but sends the ConnectResponse to the joiner's self-reported private address (e.g., 127.0.0.1) instead of the observed external address. This causes the join handshake to never complete, leaving the peer without a ring location and unable to participate in DHT operations.

Impact

• Peers behind NAT cannot complete the join process
• own_location stays unassigned (-1)
• All PUT operations are cached locally with warning: "Not forwarding PUT – own ring location not assigned yet"
• GET/Subscribe operations fail because peer can't route through DHT
• River and other Freenet applications are completely non-functional for users behind NAT

Root Cause

File: crates/core/src/operations/connect.rs, line 824

When handling a ConnectRequest, the relay correctly updates the joiner's address with the observed external address at line 282:

// Line 282 - CORRECT: Updates joiner address with observed external address
self.request.joiner.peer.addr = joiner_addr;

And uses the updated address when sending ObservedAddress at lines 784-792:

// Lines 784-792 - CORRECT: Uses updated target from actions
if let Some((target, address)) = actions.observed_address {
    let msg = ConnectMsg::ObservedAddress { ... target: target.clone(), ... };
    network_bridge.send(&target.peer, ...).await?;  // Sends to correct address
}

But when sending ConnectResponse, it uses the original from parameter which still has the private address:

// Lines 820-830 - BUG: Uses original 'from' instead of updated joiner
if let Some(response) = actions.accept_response {
    let response_msg = ConnectMsg::Response {
        id: self.id,
        sender: env.self_location().clone(),
        target: from.clone(),  // <-- BUG: from.peer.addr is still 127.0.0.1
        payload: response,
    };

Evidence from Logs

Gateway logs show it trying to send ConnectResponse to 127.0.0.1:43227:

Sending outbound message to peer, tx: 01KAWDVAY8..., 
  msg_type: Message {ConnectResponse { sender: v6MWKgqHiBMNcGtG, target: v6MWKgqKEpQJPprk, ... }}, 
  target_peer: v6MWKgqKEpQJPprk

WARN: No existing outbound connection, establishing connection first, 
  id: 01KAWDVAY8..., target: v6MWKgqKEpQJPprk

NodeEvent::ConnectPeer received, tx: 01KAWDVAY8..., 
  remote: v6MWKgqKEpQJPprk, 
  remote_addr: 127.0.0.1:43227   <-- WRONG ADDRESS

OutboundFailed { transaction: 01KAWDVAY8..., peer: v6MWKgqKEpQJPprk, 
  error: TransportError("failed while establishing connection, reason: max connection attempts reached") }

Meanwhile, ObservedAddress was sent to the correct address and arrived successfully:

# Gateway sends to correct address:
Sending outbound message to peer, tx: 01KAWDVAY8..., 
  msg_type: Message {ObservedAddress { target: v6MWKgqKEpQJPprk (@ 0.409...), address: 136.62.52.28:43227 }}
Message successfully sent to peer connection

# Local peer receives it:
Received inbound message from peer - processing, tx: 01KAWDVAY8..., 
  msg_type: Message {ObservedAddress { target: v6MWKgqKEpQJPprk (@ 0.409...), address: 136.62.52.28:43227 }}

Suggested Fix

Line 824 should use the updated joiner address from actions.observed_address instead of the original from:

if let Some(response) = actions.accept_response {
    // Use updated joiner address if available, otherwise fall back to from
    let response_target = actions.observed_address
        .as_ref()
        .map(|(target, _)| target.clone())
        .unwrap_or_else(|| from.clone());

    let response_msg = ConnectMsg::Response {
        id: self.id,
        sender: env.self_location().clone(),
        target: response_target,  // Uses correct external address
        payload: response,
    };
    return Ok(store_operation_state_with_msg(&mut self, Some(response_msg)));
}

Alternatively, add a response_target field to RelayActions that's always set to the correct joiner address.

Why Tests Don't Catch This

1. Tests run on localhost where all addresses are 127.0.0.1 - packets to loopback still arrive
2. The existing test relay_emits_observed_address_for_private_joiner validates that ObservedAddress uses the correct address, but doesn't test ConnectResponse
3. Test network configurations often pre-set locations via location: Some(...), bypassing the need for the join handshake to complete

Test Case Needed

Add a test that verifies ConnectResponse is sent to the observed external address, not the joiner's self-reported address:

#[test]
fn connect_response_uses_observed_address_not_private() {
    // Setup: joiner with private address 127.0.0.1:5050
    // Gateway observes external address 203.0.113.10:5050
    // Verify: ConnectResponse target has 203.0.113.10:5050, not 127.0.0.1:5050
}

Reproduction Steps

1. Run a Freenet peer behind NAT (or with a private IP that differs from external)
2. Configure it to connect to a public gateway
3. Observe in logs:
   • Peer sends ConnectRequest with private address in joiner field
   • Gateway sends ObservedAddress with correct external address (peer receives this)
   • Gateway sends ConnectResponse to private address (peer never receives this)
   • Peer logs show: "Not forwarding PUT – own ring location not assigned yet"

Related Files

• crates/core/src/operations/connect.rs - Main bug location (line 824)
• crates/core/src/operations/connect.rs:277-289 - Where joiner address is correctly updated
• crates/core/src/node/network_bridge/p2p_protoc.rs:333-335 - Where observed_addr is set from UDP socket

[github-actions]

🤖 Auto-labeled

Applied labels:

• T-bug (99% confidence)
• P-high (90% confidence)
• E-medium (80% confidence)
• A-networking (95% confidence)

Reasoning: The report describes a clear incorrect behavior in connect.rs where ConnectResponse is sent to the joiner's self-reported private address instead of the observed external address, breaking NATed peers' ability to join and causing major functionality loss for users behind NAT. This is a bug (T-bug) that impacts many users (P-high). The fix appears localized and straightforward but requires adding a test and careful verification, so effort is estimated as medium (E-medium). The root cause and symptom are squarely in networking/peer-connection code, so A-networking applies.

Previous labels: none

If these labels are incorrect, please update them. This helps improve the auto-labeling system.

[sanity]

@claude Can you very skeptically review this github issue and the code it references, do you think the issue's analysis is correct?

[claude]

Claude finished @sanity's task —— View job

---

Code Review of Issue #2141

I've conducted a thorough and skeptical review of the issue analysis and the referenced code. Here's my verdict:

Verdict: ✅ The issue analysis is CORRECT

The bug is real, the root cause analysis is accurate, and this is indeed a critical issue preventing NAT traversal.

---

Detailed Analysis

✅ Claim 1: Line 282 correctly updates joiner address

VERIFIED - In RelayState::handle_request (lines 277-289):

if let Some(joiner_addr) = self.request.observed_addr {
    if !self.observed_sent {
        self.request.joiner.peer.addr = joiner_addr;  // Line 282 ✓
        ...
        actions.observed_address = Some((self.request.joiner.clone(), joiner_addr));
    }
}

The code correctly updates the joiner's address to the observed external address.

✅ Claim 2: Lines 784-792 send ObservedAddress to correct address

VERIFIED - The ObservedAddress message uses target from actions.observed_address, which was set at line 287 after the joiner address was updated at line 282. The message is sent to &target.peer, which has the correct external address.

✅ Claim 3: Line 824 sends ConnectResponse to wrong address

VERIFIED - This is the bug:

if let Some(response) = actions.accept_response {
    let response_msg = ConnectMsg::Response {
        id: self.id,
        sender: env.self_location().clone(),
        target: from.clone(),  // ❌ BUG: 'from' is the original parameter
        payload: response,
    };

The from variable at line 824 is the original from parameter from the ConnectMsg::Request (extracted at line 775). This was never updated - only self.request.joiner inside the RelayState was updated.

Evidence from code flow:

1. Line 775: ConnectMsg::Request { from, payload, .. } - original joiner with private address
2. Line 782: self.handle_request(&env, from.clone(), ...) - passes original from
3. Line 282: Updates self.request.joiner.peer.addr - but not the from parameter
4. Line 824: Uses from.clone() - still has the original private address

---

Issue with Suggested Fix

The suggested fix in the issue has a subtle problem:

let response_target = actions.observed_address
    .as_ref()
    .map(|(target, _)| target.clone())
    .unwrap_or_else(|| from.clone());  // ⚠️ Problematic fallback

Problem: actions.observed_address is only set once (guarded by !self.observed_sent at line 281). On subsequent calls to handle_request, if the relay accepts the joiner, actions.observed_address will be None (because observed_sent is already true), causing the code to fall back to the wrong from address.

---

Better Fix Options

Option 1: Extract from RelayState (recommended)

if let Some(response) = actions.accept_response {
    let response_target = if let Some(ConnectState::Relaying(state)) = &self.state {
        state.request.joiner.clone()  // Always has correct address after line 282
    } else {
        from.clone()  // Shouldn't happen, but safe fallback
    };
    
    let response_msg = ConnectMsg::Response {
        id: self.id,
        sender: env.self_location().clone(),
        target: response_target,
        payload: response,
    };

Option 2: Add field to RelayActions (cleaner)

pub(crate) struct RelayActions {
    pub accept_response: Option<ConnectResponse>,
    pub response_target: Option<PeerKeyLocation>,  // Add this
    pub expect_connection_from: Option<PeerKeyLocation>,
    pub forward: Option<(PeerKeyLocation, ConnectRequest)>,
    pub observed_address: Option<(PeerKeyLocation, SocketAddr)>,
}

Then set it in RelayState::handle_request:

if !self.accepted_locally && ctx.should_accept(&self.request.joiner) {
    self.accepted_locally = true;
    let acceptor = ctx.self_location().clone();
    actions.accept_response = Some(ConnectResponse { ... });
    actions.response_target = Some(self.request.joiner.clone());  // Add this
    actions.expect_connection_from = Some(self.request.joiner.clone());

---

Additional Observations

1. Log evidence aligns perfectly - The gateway logs show ConnectResponse being sent to 127.0.0.1:43227 while ObservedAddress correctly went to the external address.

2. Test gap is real - The existing test relay_emits_observed_address_for_private_joiner only validates ObservedAddress, not ConnectResponse.

3. Why tests don't catch this - The issue correctly identifies that localhost tests don't catch this because packets to 127.0.0.1 arrive even when they shouldn't.

4. Critical impact - This bug indeed breaks all NAT traversal, making Freenet unusable for users behind NAT.

---

Recommendation

The issue analysis is sound and the bug is real. I recommend:

1. Fix using Option 2 (add response_target to RelayActions) - it's cleaner and more explicit
2. Add the suggested test case to prevent regression
3. Priority: High - This breaks core functionality for most real-world users

The issue author clearly understands the codebase and has done excellent debugging work. The analysis is correct.

---