From 9ce8d6c3c4af26d654cf5573d2c2b6c41765f5a3 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Tue, 29 Sep 2026 19:30:00 -0400 Subject: [PATCH] fix(web): origin guard accepts an https page behind a TLS proxy; log only the site - A portless Host now matches the default port of either the browser's scheme or Flask's, so nginx terminating TLS in front of a plain-http upstream (Origin https://pi.example -> 443, Flask sees http -> 80) no longer refuses every legitimate write. A non-default port still has to match exactly. - The refusal log records only scheme://host[:port] of Origin/Referer, never a Referer's path or query (which can carry tokens), and repr()s the path. - Docs: forward $http_host, not $host (nginx's $host drops the port). Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 ++- docs/REST_API_REFERENCE.md | 4 +- docs/WEB_INTERFACE_GUIDE.md | 3 +- test/test_web_origin_guard.py | 51 +++++++++++++++++++ web_interface/origin_guard.py | 96 ++++++++++++++++++++++++++--------- 5 files changed, 130 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08d7020a..5d9200c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,8 +38,10 @@ accepts both, but the store flags the old spelling as deprecated dashboard or userscript on another host) can no longer call the mutating API; call it server-side instead. Anyone posting a form body to `system/action` must switch to JSON. Behind a reverse proxy, forward the - original `Host` (`proxy_set_header Host $host;`); `X-Forwarded-Host` is - not trusted. + original `Host`, port included (`proxy_set_header Host $http_host;`; + nginx's `$host` drops the port); `X-Forwarded-Host` is not trusted. A + TLS-terminating proxy needs nothing more: a portless `Host` matches an + `https://` page. ### Fixes diff --git a/docs/REST_API_REFERENCE.md b/docs/REST_API_REFERENCE.md index 1110b779..6f08fb65 100644 --- a/docs/REST_API_REFERENCE.md +++ b/docs/REST_API_REFERENCE.md @@ -26,8 +26,8 @@ driving the Pi through a LAN user's browser. Scripts, curl, Home Assistant and the MQTT bridge send neither header and are unaffected. A browser page on another origin (a dashboard you host elsewhere, say) can no longer call the API; call it server-side instead. Behind a reverse proxy, pass the original -`Host` through (nginx: `proxy_set_header Host $host;`) -- `X-Forwarded-Host` -is not read. +`Host` through, port included (nginx: `proxy_set_header Host $http_host;`; +`$host` drops the port) -- `X-Forwarded-Host` is not read. ## Table of Contents diff --git a/docs/WEB_INTERFACE_GUIDE.md b/docs/WEB_INTERFACE_GUIDE.md index 63ceb1ee..f42cc432 100644 --- a/docs/WEB_INTERFACE_GUIDE.md +++ b/docs/WEB_INTERFACE_GUIDE.md @@ -419,7 +419,8 @@ The API blueprint (`web_interface/blueprints/api_v3/`) is registered at (403 `CROSS_SITE_REQUEST`), so use the interface from its own address. - Scripts, curl, Home Assistant and the MQTT bridge send no such header and keep working. Behind a reverse proxy, forward the original `Host` header - (nginx: `proxy_set_header Host $host;`). + with its port (nginx: `proxy_set_header Host $http_host;` -- `$host` + drops the port). **Best Practices:** 1. Run on a private network (not exposed to internet) diff --git a/test/test_web_origin_guard.py b/test/test_web_origin_guard.py index b7b04601..22244c27 100644 --- a/test/test_web_origin_guard.py +++ b/test/test_web_origin_guard.py @@ -90,6 +90,57 @@ def test_the_same_host_on_another_port_is_another_site(probe): assert resp.status_code == 403 +def test_an_https_page_behind_a_tls_terminating_proxy_passes(probe): + # nginx terminates TLS and forwards a portless Host to the plain-http + # upstream: the browser's Origin is https (443), Flask sees http (80). + resp = probe.post('/change', headers={ + 'Host': 'pi.example', 'Origin': 'https://pi.example'}) + assert resp.status_code == 200 + resp = probe.post('/change', headers={ + 'Host': 'pi.example', 'Referer': 'https://pi.example/v3'}) + assert resp.status_code == 200 + + +def test_a_portless_host_still_refuses_a_nondefault_port(probe): + # Only the standard port of either scheme counts as "no port". + for origin in ('https://pi.example:8443', 'http://pi.example:5000', + 'http://pi.example:443', 'https://evil.example'): + resp = probe.post('/change', headers={ + 'Host': 'pi.example', 'Origin': origin}) + assert resp.status_code == 403, origin + + +def test_an_explicit_host_port_must_match_exactly(probe): + # A Host with a port (the proxy forwards $http_host) is compared as is. + assert probe.post('/change', headers={ + 'Host': 'pi.example:8443', + 'Origin': 'https://pi.example:8443'}).status_code == 200 + assert probe.post('/change', headers={ + 'Host': 'pi.example:8443', + 'Origin': 'https://pi.example'}).status_code == 403 + + +def test_a_refusal_logs_only_the_site_never_the_referer_path(probe, caplog): + # A Referer's path and query can carry tokens. + with caplog.at_level('WARNING', logger='web_interface.origin_guard'): + resp = probe.post('/change', headers={ + 'Referer': EVIL + '/page?token=s3cret#frag'}) + assert resp.status_code == 403 + logged = caplog.text + assert 'evil.example' in logged + assert 's3cret' not in logged + assert '/page' not in logged + + +def test_a_refusal_log_cannot_be_forged_with_newlines(probe, caplog): + with caplog.at_level('WARNING', logger='web_interface.origin_guard'): + probe.post('/change%0D%0AFAKE', headers={'Origin': EVIL}) + assert len(caplog.records) == 1 + message = caplog.records[0].getMessage() + assert '\n' not in message and '\r' not in message + assert 'FAKE' in message # the path was logged, escaped + + def test_no_origin_and_no_referer_passes(probe): # curl, Home Assistant, the MQTT bridge: not a browser. assert probe.post('/change').status_code == 200 diff --git a/web_interface/origin_guard.py b/web_interface/origin_guard.py index 6d0c8145..2eeeb6e9 100644 --- a/web_interface/origin_guard.py +++ b/web_interface/origin_guard.py @@ -29,14 +29,18 @@ it follows whatever name or address the user typed: ``ledpi.local:5000``, portal's port 80 -> 5000 redirect keeps the Host the browser sent, and the setup page's fetches go back to that same host). -The scheme is deliberately not compared, only host and port (with each side's -default port filled in from its own scheme). A TLS-terminating reverse proxy -that passes ``Host`` through makes the browser say ``https://pi.example`` while -Flask sees ``http``; an attacker cannot use that gap, because to match they -would need to serve a page from this same host and port. The app does not use -``ProxyFix`` and so does not trust ``X-Forwarded-Host``: a proxy that rewrites -``Host`` to the upstream address (nginx's default ``proxy_pass`` does) must -be configured to pass the original one (``proxy_set_header Host $host;``). +The scheme is deliberately not compared, only host and port. The claimed +value's default port comes from its own scheme. A ``Host`` without a port +means "the default port of whatever scheme the browser used", and that scheme +is not always the one Flask sees: a TLS-terminating reverse proxy makes the +browser say ``https://pi.example`` (443) while Flask sees ``http`` (80). So a +portless ``Host`` accepts either default. An attacker cannot use that gap, +because to match they would need to serve a page from this same host on its +standard port. The app does not use ``ProxyFix`` and so does not trust +``X-Forwarded-Host`` or ``X-Forwarded-Proto``: a proxy that rewrites ``Host`` +to the upstream address (nginx's default ``proxy_pass`` does) must be +configured to pass the original one, port included +(``proxy_set_header Host $http_host;`` -- nginx's ``$host`` drops the port). Not covered: DNS rebinding (an attacker's hostname re-pointed at the Pi is "same origin" to the browser), and anyone who can reach the port directly. @@ -55,14 +59,14 @@ STATE_CHANGING_METHODS = frozenset({'POST', 'PUT', 'PATCH', 'DELETE'}) _DEFAULT_PORTS = {'http': 80, 'https': 443} -def _host_port(scheme: str, netloc: str): - """``(hostname, port)`` for a URL's authority, or None if it has none. +def _authority(netloc: str): + """``(hostname, port)`` for an authority; port is None when it has none. - Lower-cases the host and fills in the scheme's default port, so - ``http://Pi.local`` and a ``Host: pi.local:80`` header compare equal. + Lower-cases the host and drops a trailing dot, so ``Pi.local.`` and + ``pi.local`` compare equal. None if the authority is unreadable. """ try: - parts = urlsplit(f'{scheme}://{netloc}') + parts = urlsplit(f'//{netloc}') hostname = parts.hostname port = parts.port except ValueError: @@ -70,25 +74,63 @@ def _host_port(scheme: str, netloc: str): return None if not hostname: return None - if port is None: - port = _DEFAULT_PORTS.get(scheme.lower()) return hostname.lower().rstrip('.'), port def _url_host_port(url: str): - """``(hostname, port)`` for an Origin or Referer value, or None.""" + """``(hostname, port, default_port)`` for an Origin or Referer, or None. + + ``port`` is the explicit port or, failing that, the URL scheme's default, + which is also returned as ``default_port``. + """ try: parts = urlsplit(url.strip()) except ValueError: return None - if parts.scheme.lower() not in _DEFAULT_PORTS or not parts.netloc: + scheme = parts.scheme.lower() + if scheme not in _DEFAULT_PORTS or not parts.netloc: return None - return _host_port(parts.scheme.lower(), parts.netloc.rsplit('@', 1)[-1]) + authority = _authority(parts.netloc.rsplit('@', 1)[-1]) + if authority is None: + return None + hostname, port = authority + default_port = _DEFAULT_PORTS[scheme] + return hostname, default_port if port is None else port, default_port -def _request_host_port(): - """``(hostname, port)`` this request was addressed to, per its Host.""" - return _host_port(request.scheme, request.host) +def _names_this_server(claimed) -> bool: + """Whether a claimed ``(hostname, port, default_port)`` is this request's + own ``Host``.""" + own = _authority(request.host) + if own is None: + return False + hostname, port = own + claimed_host, claimed_port, claimed_default = claimed + if claimed_host != hostname: + return False + if port is not None: + return claimed_port == port + # A portless Host is the default port of the scheme the browser used. + # Behind a TLS-terminating proxy that is https/443 while Flask sees + # http/80, so accept the default of either scheme. + return claimed_port in (claimed_default, + _DEFAULT_PORTS.get(request.scheme)) + + +def _loggable(value: str) -> str: + """Just the ``scheme://host[:port]`` of an Origin/Referer, for the log. + + A Referer's path and query can carry tokens or other private data, and + only the site matters when reading a refusal. + """ + try: + parts = urlsplit(value.strip()) + netloc = parts.netloc.rsplit('@', 1)[-1] + except ValueError: + return '' + if not parts.scheme or not netloc: + return '' + return f'{parts.scheme}://{netloc}' def check_request_origin(): @@ -115,7 +157,7 @@ def check_request_origin(): claimed = _url_host_port(value) if claimed is None: return f'{header} header is not a valid http(s) URL' - if claimed != _request_host_port(): + if not _names_this_server(claimed): # The claimed value is attacker-chosen: the hook logs it, but the # reason (echoed in the 403 body) never repeats it. return header + ' names a different host than this interface' @@ -130,10 +172,14 @@ def init_app(app: Flask) -> None: reason = check_request_origin() if reason is None: return None - logger.warning("Refused cross-site %s %s: %s (Origin=%r, Referer=%r)", + # Only the site each header names, never a Referer's path or query + # (which can carry tokens); %r keeps CR/LF from forging log lines. + origin = request.headers.get('Origin') + referer = request.headers.get('Referer') + logger.warning("Refused cross-site %s %r: %s (Origin=%r, Referer=%r)", request.method, request.path, reason, - request.headers.get('Origin'), - request.headers.get('Referer')) + None if origin is None else _loggable(origin), + None if referer is None else _loggable(referer)) return jsonify({ 'status': 'error', 'error_code': 'CROSS_SITE_REQUEST',