[FIXED] PROXY v1 parsing may discard client bytes

[sciascid]

If the parser read past the \r\n of a PROXY protocol v1 header, then the remaining bytes, if any, would be dropped. That left the connection desynchronized before normal client parsing saw CONNECT.
readProxyProtoHeader now returns the remaining bytes, to be read by the next protocol parsing step.

Signed-off-by: Daniele Sciascia daniele@nats.io

[sciascid]

@claude review

[synadia-claude-reviewer]

Claude finished @sciascid's task in 1m 45s —— View job

---

Claude's Review

• Gathering context
• Reading changed files
• Analyzing the fix
• Posting review

---

Summary

This is a clean, focused fix for a real protocol-level data loss bug. The root cause is correctly identified: conn.Read() in the v1 header parser could return more bytes than belong to the PROXY header (OS/TCP coalescing is common), and those extra bytes were silently discarded before they reached the NATS client parser. Everything looks good.

---

Fix Correctness

readProxyProtoV1Header (client_proxyproto.go:136-223)

The buffer loop is correct. buf is allocated at maxRemaining bytes; each conn.Read(buf[len(line):]) appends into the unused tail; when CRLF is found at position i, line[i+2:] is exactly the over-read data:

remaining = append(remaining, line[i+2:]...)
line = line[:i]
goto foundCRLF

Using append here makes a copy of the over-read bytes into a fresh allocation rather than returning a sub-slice of buf. This is the safer choice – the caller receives an independent slice.

server.go:3483-3484

The one-line change from pre = nil to pre = proxyPre is exactly right. pre flows into c.readLoop(pre) at line 3544, so the CONNECT (and any bytes coalesced in the same TCP segment) are no longer lost.

V2 path (readProxyProtoHeader:269)

V2 always returns nil for the extra bytes (return addr, nil, err). This is correct: v2 uses an explicit address-length field and reads exactly that many bytes, so it cannot over-read into client data.

UNKNOWN command (client_proxyproto.go:179)

if parts[0] == proxyProtoV1Unknown {
    return nil, remaining, nil
}

remaining is correctly returned here too. UNKNOWN is a health-check but bytes coalesced after it should still be preserved for the client parser. Good.

---

Test Coverage

Two targeted improvements:

1. TestClientProxyProtoV1PreservesCoalescedClientBytes (new unit test, line 456) – directly asserts that remaining equals the CONNECT bytes, and that the underlying conn has nothing left to read. This is the precise invariant the fix is introducing.

2. TestClientProxyProtoV1EndToEnd (improved, line 522) – the test now sends header + CONNECT + PING in a single conn.Write() call. This forces the parser to deal with coalesced bytes and then asserts INFO, +OK, and PONG responses arrive correctly. This directly reproduces the original bug scenario.

The updated lock discipline in the end-to-end test is also a minor improvement – s.mu now wraps both the map read and the iteration loop.

---

Minor Observations (non-blocking)

1. remaining is only ever appended once (the goto ensures that), so the append on a nil slice is a no-op allocation when there are no over-read bytes (line[i+2:] is empty). That's fine idiomatic Go.

2. V2 post-address-data bytes: If a v2 header were somehow followed by extra bytes (e.g. TLV extensions beyond addrLen), those are consumed by io.CopyN(io.Discard, ...) or io.ReadFull, so v2 already handles that correctly. The nil return for v2 remaining is appropriate.

3. s.mu unlock coverage in TestClientProxyProtoV2EndToEnd (line 347): The v2 end-to-end test still uses the older pattern of unlocking s.mu before iterating clients. Not introduced by this PR, just worth noting for a follow-up.

---

LGTM. The fix is minimal, correct, and well-tested.

[neilalexander]

LGTM