peer/portforward: address Copilot review on #503 (round 2)

portforward/manual.go:
- ManualForwarder.MapPort now sets Protocol="TCP" to match the
  UPnP-based Forwarder, which hard-codes the same value. Samizdat-in
  inbound traffic is TCP-only on both code paths; widening to UDP
  later means widening both forwarders together.
- Defensive guard: MapPort returns an error when constructed with
  port==0. pickManualForwarder already range-checks 1..65535 before
  calling NewManualForwarder, but the check belongs on the type
  itself — a caller that bypasses the validator (programmatic use,
  tests, future code paths) gets a clear error instead of silently
  registering port 0 with lantern-cloud.

portforward/manual_test.go:
- Asserts Protocol="TCP" in TestManualForwarder.
- TestManualForwarder_RejectsZeroPort verifies the new guard.

peer/peer.go:
- slog warning in the env-var path used 'error' as the attribute
  key while every other log line in this file uses 'err'. Renamed
  for log-aggregation consistency.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Author: myleshorton

peer/peer.go

@@ -647,7 +647,7 @@ func pickManualForwarder() portForwarder {
 		port, err := portforward.ParseManualPort(raw)
 		if err != nil {
 			slog.Warn("ignoring invalid "+env.PeerExternalPort.String(),
-				"value", raw, "error", err)
+				"value", raw, "err", err)
 		} else {
 			slog.Info("peer client using manual port forward",
 				"port", port, "source", env.PeerExternalPort.String())

portforward/manual.go

@@ -45,11 +45,22 @@ func ParseManualPort(s string) (uint16, error) {
 
 // MapPort reports the configured port as both external and internal. The
 // router-side rule is already in place; nothing to do at the protocol
-// layer.
+// layer. Returns an error if the forwarder was constructed with port==0
+// (not a valid listen/registration port) so callers can fall through to
+// UPnP rather than register a port no peer will listen on.
+//
+// Protocol is set to "TCP" to match the UPnP-based Forwarder, which
+// hard-codes the same value. Samizdat-in inbound traffic is TCP-only;
+// when UDP support is added the two forwarders should be widened
+// together.
 func (m *ManualForwarder) MapPort(_ context.Context, _ uint16, _ string) (*Mapping, error) {
+	if m.port == 0 {
+		return nil, fmt.Errorf("manual forwarder constructed with port=0")
+	}
 	return &Mapping{
 		ExternalPort: m.port,
 		InternalPort: m.port,
+		Protocol:     "TCP",
 		Method:       "manual",
 	}, nil
 }

portforward/manual_test.go

@@ -43,16 +43,18 @@ func TestParseManualPort(t *testing.T) {
 }
 
 // ManualForwarder satisfies the portForwarder contract: MapPort returns
-// a Mapping with external==internal port and the "manual" method tag,
-// UnmapPort and StartRenewal are no-ops, ExternalIP returns "" so the
-// server substitutes the IP it observed on the register call.
+// a Mapping with external==internal port, Protocol="TCP" matching the
+// UPnP forwarder, and the "manual" method tag. UnmapPort and
+// StartRenewal are no-ops, ExternalIP returns "" so the server
+// substitutes the IP it observed on the register call.
 func TestManualForwarder(t *testing.T) {
 	f := NewManualForwarder(5698)
 
 	m, err := f.MapPort(context.Background(), 30001, "ignored")
 	require.NoError(t, err)
 	assert.Equal(t, uint16(5698), m.ExternalPort)
 	assert.Equal(t, uint16(5698), m.InternalPort, "external==internal — user mapped them themselves")
+	assert.Equal(t, "TCP", m.Protocol, "Protocol matches UPnP forwarder's hard-coded value")
 	assert.Equal(t, "manual", m.Method)
 
 	require.NoError(t, f.UnmapPort(context.Background()), "UnmapPort is a no-op for manual forwarders")
@@ -64,3 +66,13 @@ func TestManualForwarder(t *testing.T) {
 	require.NoError(t, err)
 	assert.Empty(t, ip, "empty IP signals server to use observed source address")
 }
+
+// MapPort defensively rejects a zero-port forwarder so a caller that
+// somehow gets one (bypassing pickManualForwarder's range check) can
+// fall through to UPnP rather than register a non-listening port.
+func TestManualForwarder_RejectsZeroPort(t *testing.T) {
+	f := NewManualForwarder(0)
+	m, err := f.MapPort(context.Background(), 30001, "ignored")
+	assert.Nil(t, m)
+	assert.Error(t, err)
+}