Skip to content

fix: include client addresses in WebUI request logs#2982

Merged
1 commit merged into
nesquena:masterfrom
xolom:fix/log-request-client-address
May 26, 2026
Merged

fix: include client addresses in WebUI request logs#2982
1 commit merged into
nesquena:masterfrom
xolom:fix/log-request-client-address

Conversation

@xolom

@xolom xolom commented May 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add remote client IP to structured WebUI request logs
  • Preserve the first X-Forwarded-For address as forwarded_for when present
  • Cover malformed requests, direct clients, and proxied clients in log_request tests

Why

This makes failed WebUI login/access logs usable for downstream security tooling such as fail2ban, especially when the WebUI is behind a reverse proxy.

Test Plan

  • /tmp/hermes-webui-test-venv/bin/python -m pytest tests/test_issue2775_log_request.py -q

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Summary

Reading server.py:274-298 on origin/master and the diff at this PR, the change is small and well-scoped: log_request gains a remote field (raw socket address, falling back to "-") and an optional forwarded_for field (first comma-token of X-Forwarded-For when present). The malformed-request defensive pattern from PR #2832 is preserved — getattr(self, 'client_address', None) mirrors the existing getattr(self, 'command', None) style — so the fail2ban-friendly log shape doesn't reintroduce the AttributeError that #2775 closed.

Code reference

The new shape, from the diff:

record_data = {
    'ts': time.strftime('%Y-%m-%dT%H:%M:%SZ', time.gmtime()),
    'remote': remote,
    'method': getattr(self, 'command', None) or '-',
    'path': getattr(self, 'path', None) or '-',
    'status': int(code) if str(code).isdigit() else code,
    'ms': duration_ms,
}
if forwarded_for:
    record_data['forwarded_for'] = forwarded_for

The malformed test gains assert record["remote"] == "-", and two new cases pin the direct-client and proxy-client shapes (tests/test_issue2775_log_request.py:18-54).

Observations

Two small things to consider before this merges:

  1. X-Real-IP parity. The onboarding gate already established a precedent: api/routes.py:5996-5998 (and the /onboarding/setup + /onboarding/probe mirrors at lines 6032 and 6064) reads forwarded headers in the order X-Forwarded-ForX-Real-IP → raw socket:

    _xff = handler.headers.get("X-Forwarded-For", "").split(",")[0].strip()
    _xri = handler.headers.get("X-Real-IP", "").strip()
    _raw = handler.client_address[0]

    Some reverse proxies (notably nginx in its default proxy_set_header X-Real-IP $remote_addr; configuration) set only X-Real-IP. Adding a real_ip field (or folding X-Real-IP into forwarded_for when XFF is absent) keeps the log-line useful for fail2ban setups behind nginx-without-XFF.

  2. Spoofability note for downstream rules. A direct (non-proxied) client can set X-Forwarded-For itself. The forwarded_for field is therefore safe to display but not safe to trust as the auth-identity unless the operator knows the listener is behind a sanitizing proxy. Worth a one-line CHANGELOG hint ("trust only when the operator controls the reverse proxy") so a fail2ban author doesn't ban on forwarded_for when the WebUI is reachable directly.

  3. CHANGELOG.md. AGENTS.md asks for a CHANGELOG entry for "user-visible behavior" changes; the new structured-log fields qualify (anyone parsing [webui] {…} lines downstream sees two new keys). Past log-shape PRs landed CHANGELOG lines (e.g. PR fix(server): tolerate malformed request logging #2832 at line 241 of CHANGELOG.md); a one-liner under the unreleased Added section would keep that pattern.

Test plan

Existing tests pass under the diff; the three cases — malformed-no-client-address, direct-client, and XFF-present — line up with the three code paths through the try/except blocks. A fourth case for X-Real-IP alone (no XFF) would only be needed if you pick up suggestion 1 above.

Nothing in this PR needs a hold from me; the suggestions are non-blocking. Thanks for the targeted fix.

@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Merged in Release DJ / v0.51.138 (stage-batch20 — 7-PR ultra-safe batch with PRs #2948 #2949 #2950 #2960 #2972 #2975 #2982).

Thanks @xolom! 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants