https://gitlab.synchro.net/main/sbbs/-/merge_requests/735#note_10621
Looks correct to me. One design suggestion, no blocking defects.
**Checked and fine:**
- `ld` is allocated per request (and `hostname` seeded from `host_name` before
`parse_headers()` runs), so replacing it here is the right spot. The old
string is freed before the `strdup()`, and `ld == NULL` (HTTP logging off) is
guarded.
- `truncsp(value)` modifies the request's own `strdup()`ed `head_line`;
nothing downstream depends on the trailing whitespace.
- `inet_pton` is portable here: `sockwrap.h` maps it to `xp_inet_pton` on
Windows, and `websrvr.cpp` includes it (`findstr.c` already relies on the
same mapping). The 16-byte buffer covers IPv6.
- Resetting `host_ip` to the socket address on an invalid value (rather than
keeping whatever an earlier duplicate header set) is the safer choice.
**Suggestion: comma-separated `X-Forwarded-For` lists.**
Rejecting the list outright is safer than the old verbatim copy, but it discards the address in a common setup: nginx with
`proxy_add_x_forwarded_for` sends `client-supplied, real-client` whenever the client sent its own XFF. The **rightmost** entry is the one the trusted proxy appended; everything to its left is client-controlled. Taking the last comma-separated entry, then validating it, would keep XFF users working
instead of silently falling back to the proxy's address.
**Minor, not blocking:**
- With a valid header, `%h` becomes a bare IP; otherwise it stays the
reverse-DNS name. Not doing DNS on a client-supplied address is right, just
a visible difference for hostname-based log tooling.
- On the keep-alive case you listed as pre-existing: with this patch, a later
request without the header now logs the proxy while `host_ip` (and so
`REMOTE_ADDR`, ip.can checks) still holds the earlier forwarded IP. Harmless,
but a new inconsistency between the log and the rest of the server.
- Anyone who can reach the origin directly, bypassing Cloudflare, can put any
valid address in the header, which now also lands in the access log. That's
the existing trust model; no change needed.
-- *Authored by Claude (Claude Code), on behalf of @rswindell*
---
þ Synchronet þ Vertrauen þ Home of Synchronet þ [vert/cvs/bbs].synchro.net