From 10d2b11cc91c17de93e7d64d0664bf39eaf64a29 Mon Sep 17 00:00:00 2001 From: Kevin Veen-Birkenbach Date: Sat, 31 Oct 2026 10:40:40 +0100 Subject: [PATCH] fix(sys-svc-webserver-core): keep URL tokens out of the openresty access log The front proxy logged the full request line: production had no access_log, so nginx fell back to `combined` ($request, Referer, $remote_user), and the debug format logged $request and the Referer. Query-string tokens (?access_token=, ?key=, ?token=) and token routes such as /password/reset/{token} and /i/confirm-email/{user}/{random} ended up in plain text in the container log. nginx.conf.j2 now declares a map from $request_uri to $infinito_access_log_path: - the path is written without the query string; - the segments after a known token route (password/reset, password-reset, reset-password, confirm-email, lostpassword/reset/form, email-login, activate-account; matched case-insensitively at any depth) are written as [redacted], so the request, its status and timings stay visible without the token. Both modes write an explicit access_log to /dev/stdout: - production uses the new `redacted` format, laid out like `combined` (remote address, time, "method path protocol", status, bytes, user agent) with Referer and basic-auth user written as "-", so combined-style parsers keep working; - debug keeps its fields (host, timings, upstream time, lengths, X-Forwarded-For, scheme, server name) but logs "method path protocol" instead of $request and drops the Referer. The path comes from $request_uri instead of $uri so internal rewrites (error_page, try_files) do not replace the path the client requested. Nothing in the repository parses the proxy access log (Wazuh, the health checks and the Prometheus log_by_lua metrics do not read it), so no consumer needs to change. The error log still carries `request:` and `referrer:` on error-level entries; that is outside this change. A unit test renders nginx.conf.j2 in both modes and asserts that every access_log uses a format declared there, that no format references $request, $request_uri, $args, $query_string, $is_args, $arg_*, $http_referer or $remote_user, and, by resolving the map as nginx does, that query strings are dropped, token routes are redacted and lookalike paths stay unchanged. Verified live with web-svc-html in production mode (deploy failed=0, so openresty accepted the config). A probe request to /password/reset/?token= was logged as "GET /password/reset/[redacted] HTTP/1.1" 404 62 "-" "curl/8.14.1", and /plain/page as "GET /plain/page HTTP/1.1". Negative control: the same request against the previous config logged the probe path token and the query token in plain text. The debug format with the fix is covered by the unit test only. Closes #1192 Co-Authored-By: Claude Opus 5.5 --- roles/sys-svc-webserver-core/README.md | 1 + .../templates/nginx.conf.j2 | 12 +- .../roles/sys-svc-webserver-core/__init__.py | 0 .../templates/__init__.py | 5 + .../templates/test_nginx_access_log.py | 194 ++++++++++++++++++ 5 files changed, 210 insertions(+), 2 deletions(-) create mode 100644 tests/unit/python/roles/sys-svc-webserver-core/__init__.py create mode 100644 tests/unit/python/roles/sys-svc-webserver-core/templates/__init__.py create mode 100644 tests/unit/python/roles/sys-svc-webserver-core/templates/test_nginx_access_log.py diff --git a/roles/sys-svc-webserver-core/README.md b/roles/sys-svc-webserver-core/README.md index 9e8c9c54d8..d78181cb93 100644 --- a/roles/sys-svc-webserver-core/README.md +++ b/roles/sys-svc-webserver-core/README.md @@ -40,6 +40,7 @@ Solid `1:1` edges are fixed relationships; dashed `0..1` edges are conditional ( - **General logs**: `journalctl -f -u NGINX` - **Filter by host**: `journalctl -u NGINX -f | grep "{{ inventory_hostname }}"` - **Enable detailed format**: set `MODE_DEBUG: true` and reload NGINX. +- **Redacted request path**: both access log formats write the path without query string, Referer or basic-auth user. Path segments after a password-reset, e-mail confirmation, e-mail login or account activation route (e.g. `/password/reset/`, `/confirm-email/`) are written as `[redacted]`; extend the `$infinito_access_log_path` map in [nginx.conf.j2](./templates/nginx.conf.j2) for further token routes. ## Credits diff --git a/roles/sys-svc-webserver-core/templates/nginx.conf.j2 b/roles/sys-svc-webserver-core/templates/nginx.conf.j2 index 97b3e84dbf..d14b87b3f8 100644 --- a/roles/sys-svc-webserver-core/templates/nginx.conf.j2 +++ b/roles/sys-svc-webserver-core/templates/nginx.conf.j2 @@ -26,10 +26,13 @@ http server_names_hash_bucket_size 128; # size of each bucket for server_name lookups (in bytes) server_names_hash_max_size 512; # maximum total buckets for the server_name hash table {% include 'roles/sys-svc-proxy/templates/headers/buffers.conf.j2' %} + map $request_uri $infinito_access_log_path { + "~*^(?P[^?]*?/(?:password/reset|password-reset|reset-password|confirm-email|lostpassword/reset/form|email-login|activate-account)/)" "${infinito_access_log_token_route}[redacted]"; + "~^(?P[^?]*)" "${infinito_access_log_plain_path}"; + } {% if MODE_DEBUG | bool %} log_format debug '$host - $remote_addr [$time_local] ' - '"$request" $status $body_bytes_sent ' - '"Referer: $http_referer" ' + '"$request_method $infinito_access_log_path $server_protocol" $status $body_bytes_sent ' '"User-Agent: $http_user_agent" ' '"ReqTime: $request_time" "UpstreamTime: $upstream_response_time" ' '"ReqLength: $request_length" "BytesSent: $bytes_sent" ' @@ -37,6 +40,11 @@ http '"X-Forwarded-For: $http_x_forwarded_for" ' '"Scheme: $scheme" "Protocol: $server_protocol" "ServerName: $server_name"'; access_log /dev/stdout debug; +{% else %} + log_format redacted '$remote_addr - - [$time_local] ' + '"$request_method $infinito_access_log_path $server_protocol" $status $body_bytes_sent ' + '"-" "$http_user_agent"'; + access_log /dev/stdout redacted; {% endif %} error_log /dev/stderr info; sendfile on; diff --git a/tests/unit/python/roles/sys-svc-webserver-core/__init__.py b/tests/unit/python/roles/sys-svc-webserver-core/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/unit/python/roles/sys-svc-webserver-core/templates/__init__.py b/tests/unit/python/roles/sys-svc-webserver-core/templates/__init__.py new file mode 100644 index 0000000000..087c384b57 --- /dev/null +++ b/tests/unit/python/roles/sys-svc-webserver-core/templates/__init__.py @@ -0,0 +1,5 @@ +from pathlib import Path + +PROJECT_ROOT: Path = Path(__file__).resolve().parents[6] + +__all__ = ["PROJECT_ROOT"] diff --git a/tests/unit/python/roles/sys-svc-webserver-core/templates/test_nginx_access_log.py b/tests/unit/python/roles/sys-svc-webserver-core/templates/test_nginx_access_log.py new file mode 100644 index 0000000000..db0e3fbd5e --- /dev/null +++ b/tests/unit/python/roles/sys-svc-webserver-core/templates/test_nginx_access_log.py @@ -0,0 +1,194 @@ +"""The front proxy's access log keeps no tokens from URLs.""" + +from __future__ import annotations + +import re +import unittest + +from jinja2 import Environment, FileSystemLoader, StrictUndefined, select_autoescape + +from . import PROJECT_ROOT + +TEMPLATE = "roles/sys-svc-webserver-core/templates/nginx.conf.j2" + +LEAKING_VARIABLE = re.compile( + r"\$\{?(request|request_uri|args|query_string|is_args|arg_\w+|http_referer|remote_user)\b" +) + +REDACTED_PATH = "$infinito_access_log_path" + +MODES = {"debug": True, "production": False} + + +def render(debug: bool) -> str: + """Return the rendered nginx.conf. + + Args: + debug: the value of ``MODE_DEBUG``. + """ + env = Environment( + loader=FileSystemLoader(str(PROJECT_ROOT)), + undefined=StrictUndefined, + autoescape=select_autoescape(), + ) + env.filters["bool"] = bool + return env.get_template(TEMPLATE).render( + MODE_DEBUG=debug, + DEPLOYMENT_MODE="compose", + WEBSERVER_PROXY_WORKER_PROCESSES=1, + WEBSERVER_PROXY_WORKER_CONNECTIONS=256, + WEBSERVER_PROXY_WORKER_RLIMIT_NOFILE=512, + lookup=lambda _kind, key: ( + ["/etc/nginx/conf.d/http/"] + if key.endswith("http_includes") + else f"/var/lib/nginx/{key}" + ), + ) + + +def directives(rendered: str, name: str) -> list[str]: + """Return the arguments of every ``name`` directive, joined across lines. + + Args: + rendered: the rendered nginx.conf. + name: the directive, e.g. ``log_format``. + """ + return re.findall(rf"^\s*{name}\s+([^;]*);", rendered, re.MULTILINE) + + +def log_formats(rendered: str) -> dict[str, str]: + """Return every declared log format body by its name. + + Args: + rendered: the rendered nginx.conf. + """ + return { + body.split(maxsplit=1)[0]: body.split(maxsplit=1)[1] + for body in directives(rendered, "log_format") + } + + +def logged_path(rendered: str, request_uri: str) -> str: + """Return the path the access log writes, resolved as nginx's map does. + + Args: + rendered: the rendered nginx.conf. + request_uri: the request URI as the client sent it. + """ + block = re.search( + r"map \$request_uri \$infinito_access_log_path \{(.*?)\n\s*\}", + rendered, + re.DOTALL, + ) + if block is None: + raise AssertionError("nginx.conf declares no map for $infinito_access_log_path") + for source, value in re.findall( + r'^\s*"([^"]+)"\s+"([^"]*)";', block.group(1), re.MULTILINE + ): + caseless = source.startswith("~*") + match = re.search( + source[2:] if caseless else source[1:], + request_uri, + re.IGNORECASE if caseless else 0, + ) + if match: + return re.sub( + r"\$\{(\w+)\}|\$(\w+)", + lambda var, m=match: m.group(var.group(1) or var.group(2)), + value, + ) + return "" + + +class TestAccessLogFormat(unittest.TestCase): + def test_every_mode_writes_its_access_log_in_a_format_declared_here(self): + for mode, debug in MODES.items(): + with self.subTest(mode=mode): + rendered = render(debug) + logs = [args.split() for args in directives(rendered, "access_log")] + self.assertTrue(logs) + for log in logs: + self.assertEqual(len(log), 2, log) + self.assertIn(log[1], log_formats(rendered)) + + def test_no_format_logs_the_query_string_the_referer_or_the_raw_request(self): + for mode, debug in MODES.items(): + for name, body in log_formats(render(debug)).items(): + with self.subTest(mode=mode, format=name): + self.assertIsNone(LEAKING_VARIABLE.search(body), body) + + def test_every_format_logs_method_redacted_path_and_status(self): + for mode, debug in MODES.items(): + for name, body in log_formats(render(debug)).items(): + with self.subTest(mode=mode, format=name): + self.assertIn( + f'"$request_method {REDACTED_PATH} $server_protocol"', body + ) + self.assertIn("$status", body) + + def test_the_debug_format_keeps_host_timings_and_upstream(self): + body = log_formats(render(True))["debug"] + for variable in ("$host", "$request_time", "$upstream_response_time"): + with self.subTest(variable=variable): + self.assertIn(variable, body) + + +class TestLoggedPath(unittest.TestCase): + rendered: str + + @classmethod + def setUpClass(cls) -> None: + cls.rendered = render(False) + + def test_the_query_string_is_dropped(self): + for request_uri, expected in ( + ("/", "/"), + ("/index.php?access_token=s3cr3t", "/index.php"), + ("/api/v1/items?page=2&token=s3cr3t", "/api/v1/items"), + ( + "/realms/x/login-actions/action-token?key=s3cr3t", + "/realms/x/login-actions/action-token", + ), + ): + with self.subTest(request_uri=request_uri): + self.assertEqual(logged_path(self.rendered, request_uri), expected) + + def test_token_routes_are_redacted(self): + for request_uri, expected in ( + ("/password/reset/s3cr3t?email=a%40b.c", "/password/reset/[redacted]"), + ("/i/confirm-email/s3cr3t/s3cr3t2", "/i/confirm-email/[redacted]"), + ("/accounts/confirm-email/s3cr3t/", "/accounts/confirm-email/[redacted]"), + ( + "/index.php/lostpassword/reset/form/s3cr3t/alice", + "/index.php/lostpassword/reset/form/[redacted]", + ), + ("/u/password-reset/s3cr3t", "/u/password-reset/[redacted]"), + ("/reset-password/s3cr3t", "/reset-password/[redacted]"), + ("/session/email-login/s3cr3t", "/session/email-login/[redacted]"), + ("/u/activate-account/s3cr3t", "/u/activate-account/[redacted]"), + ("/Password/Reset/s3cr3t", "/Password/Reset/[redacted]"), + ): + with self.subTest(request_uri=request_uri): + logged = logged_path(self.rendered, request_uri) + self.assertEqual(logged, expected) + self.assertNotIn("s3cr3t", logged) + + def test_lookalike_paths_are_logged_unchanged(self): + for request_uri in ( + "/password/reset", + "/xpassword/reset/abc", + "/settings/password", + "/confirm-email", + ): + with self.subTest(request_uri=request_uri): + self.assertEqual(logged_path(self.rendered, request_uri), request_uri) + + def test_both_modes_share_the_same_map(self): + self.assertEqual( + logged_path(render(True), "/password/reset/s3cr3t?x=1"), + logged_path(self.rendered, "/password/reset/s3cr3t?x=1"), + ) + + +if __name__ == "__main__": + unittest.main()