From c12ecad3e35b5a6fe825b919301f3e7e72c1a0a4 Mon Sep 17 00:00:00 2001 From: Codex Date: Wed, 2 Sep 2026 01:34:51 +0300 Subject: [PATCH] docs: require the trusted-proxy switch for the rate-limit source (#43) User-Visible: no Issue: #43 --- docs/specs/043-private-support-report.md | 21 ++++++---- scripts/support-relay/README.md | 16 +++++-- scripts/support-relay/tests/test_relay.py | 51 +++++++++++++++++++++++ 3 files changed, 77 insertions(+), 11 deletions(-) diff --git a/docs/specs/043-private-support-report.md b/docs/specs/043-private-support-report.md index e8d85a7f..4fdb6fb6 100644 --- a/docs/specs/043-private-support-report.md +++ b/docs/specs/043-private-support-report.md @@ -444,12 +444,17 @@ if they are unavailable, #43 receives `blocked` without weakening the contract. - message/contact rendered as escaped plain text only; - 5 attempts/hour and 20/day per source address plus a global circuit breaker; - **the source address is the one supplied by the trusted proxy, never one the - client can choose.** The relay reads the *last* element of `X-Forwarded-For`, - because the first element is whatever the caller sent, and the reverse proxy in - front of it is configured to overwrite the header outright rather than append - to it. Both halves are required: without them a caller picks its own rate-limit - bucket by changing one line of the request, and every other limit in this - section becomes decorative; + client can choose.** Behind a reverse proxy the relay runs with its + trusted-proxy switch on (`HP_RELAY_TRUSTED_PROXY`, on by default) and reads the + *last* element of `X-Forwarded-For`, because the first element is whatever the + caller sent; the proxy is configured to overwrite the header outright rather + than append to it. Both halves are required: without them a caller picks its own + rate-limit bucket by changing one line of the request, and every other limit in + this section becomes decorative. With the switch off the relay ignores the + header entirely and uses the connection address — correct only for a node + exposed directly, and **forbidden on any deployment behind a proxy**, where every + connection arrives from the proxy and all callers would share one bucket. Both + production hosts run behind Caddy, so both keep the switch on; - source IP is used only through a daily-keyed rate-limit hash with ≤24 h TTL; raw address is not written to app logs/storage/mail; - idempotency key retained 24 h and returns the original report id; @@ -536,7 +541,7 @@ parse them; user message/contact remain verbatim plain text. | AC10 | Expired/replaced/discarded tokens fail; config changes after preview do not change cached bytes. | Fake-clock backend tests. | | AC11 | Submit is HTTPS/fixed-host/no-redirect, bounded and idempotent; logs contain no message/contact/body. | Stub aiohttp server + caplog + SSRF/redirect tests. | | AC12 | Relay enforces schema, size, hash, idempotency and rate limits; HTML remains inert plain text. | Receiver unit/integration suite with a recording fake provider. | -| AC12a | A caller cannot select its own rate-limit bucket: requests carrying different forged `X-Forwarded-For` values land in one source key, and the proxy configuration overwrites the header. | Receiver test plus the deployed proxy fragment under `scripts/support-relay/deploy/`. | +| AC12a | A caller cannot select its own rate-limit bucket. With the trusted-proxy switch on, requests carrying different forged `X-Forwarded-For` values land in one source key, and the proxy configuration overwrites the header; with it off, the header is ignored altogether and the connection address is used. | Two receiver tests (one per switch position) plus the deployed proxy fragment under `scripts/support-relay/deploy/`. | | AC13 | Success shows stable report id; timeout/error preserves form and exposes retry/manual recovery without claiming success. | Browser smoke across success/429/timeout/unknown command. | | AC14 | Phone/tablet dialog, keyboard/focus and 44 px target satisfy View touch/accessibility contract. | Reviewed desktop + phone + tablet goldens and touch smoke. | | AC15 | No existing backup/diagnostics/preflight behavior or payload changes. | Existing targeted frontend/backend suites unchanged. | @@ -591,6 +596,8 @@ Mutation gate must prove at least: - relay skips hash/rate/idempotency check → receiver tests red; - relay trusts the client-supplied end of `X-Forwarded-For` (first element instead of last) → `test_client_cannot_pick_its_own_rate_bucket` red; +- relay reads `X-Forwarded-For` regardless of the trusted-proxy switch (the + switch check is dropped) → `test_direct_node_ignores_the_forwarded_header` red; - success shown on timeout → browser smoke red. ## 15. Performance and reliability budgets diff --git a/scripts/support-relay/README.md b/scripts/support-relay/README.md index 0f9e412f..1b191442 100644 --- a/scripts/support-relay/README.md +++ b/scripts/support-relay/README.md @@ -43,6 +43,13 @@ потому, что цена ошибки здесь — обход частотного лимита сменой одной строки в запросе, и полагаться на умолчания чужого конфига для этого нельзя. +Заголовок читается только при включённом `HP_RELAY_TRUSTED_PROXY` (умолчание — +включён). Со снятым переключателем relay игнорирует заголовок и берёт адрес +соединения: это верно для узла, выставленного в интернет напрямую, и **запрещено +для узла за прокси** — там все соединения приходят от прокси, и весь публичный +эндпоинт делил бы одну корзину лимита на всех. Оба продовых инстанса стоят за +Caddy, поэтому у обоих переключатель включён. + Сообщение и контакт нормализуются и очищаются от управляющих символов, включая маркеры двунаправленного письма: доставка выводит их буквальным текстом без разметки, поэтому подделать вид сообщения нельзя. @@ -196,12 +203,13 @@ sudo -u hprelay cat /var/lib/hp-support-relay/prod/reports/2026-09/hpr-…/repor cd scripts/support-relay && python3 -m unittest discover -s tests -q ``` -Тридцать пять проверок: схема, размеры, хеш, идемпотентность, частота, +Тридцать шесть проверок: схема, размеры, хеш, идемпотентность, частота, ретеншн, буквальность текста, отсутствие адреса в журналах, невозможность выбрать себе корзину лимита подделкой заголовка, поведение рубильника. -Каждая проверялась отрицательным прогоном — тринадцать мутаций рабочего кода +Каждая проверялась отрицательным прогоном — четырнадцать мутаций рабочего кода (снять сверку хеша, разрешить лишнюю часть, не чистить управляющие символы, снять лимит, писать адрес в журнал, игнорировать идемпотентность, отключить рубильник, отключить ретеншн, не проверять секции пакета, брать первый элемент -`X-Forwarded-For`, подменить `source` в вебхуке, приложить пакет к вебхуку) -роняют ровно те проверки, ради которых написаны. +`X-Forwarded-For`, подменить `source` в вебхуке, приложить пакет к вебхуку, +читать заголовок независимо от переключателя доверия прокси) роняют ровно те +проверки, ради которых написаны. diff --git a/scripts/support-relay/tests/test_relay.py b/scripts/support-relay/tests/test_relay.py index e4312955..11f4bacd 100644 --- a/scripts/support-relay/tests/test_relay.py +++ b/scripts/support-relay/tests/test_relay.py @@ -309,6 +309,57 @@ class RelayTestCase(unittest.TestCase): self.assertEqual(config.RATE_TTL_SECONDS, 24 * 3600) +class DirectNodeTestCase(unittest.TestCase): + """Узел без прокси: заголовку верить нельзя, потому что подставить его некому.""" + + def setUp(self) -> None: + self._tmp = TemporaryDirectory() + cfg = config.load({ + "HP_RELAY_SPOOL": str(Path(self._tmp.name) / "s"), + "HP_RELAY_TRUSTED_PROXY": "0", + }) + self.service = app.Service(cfg) + self.service.delivery = RecordingDelivery() + self.server = ThreadingHTTPServer(("127.0.0.1", 0), app.make_handler(self.service)) + self.port = self.server.server_address[1] + self.thread = threading.Thread(target=self.server.serve_forever, daemon=True) + self.thread.start() + + def tearDown(self) -> None: + self.server.shutdown() + self.server.server_close() + self._tmp.cleanup() + + def _post(self, key: str, forwarded: str | None) -> int: + blob = package_bytes() + content_type, body = build_body(request_json(blob, key=key), blob) + request = urllib.request.Request( + f"http://127.0.0.1:{self.port}/v1/reports", data=body, method="POST", + ) + request.add_header("Content-Type", content_type) + if forwarded: + request.add_header("X-Forwarded-For", forwarded) + try: + with urllib.request.urlopen(request, timeout=10) as response: + return response.status + except urllib.error.HTTPError as error: + return error.code + + def test_direct_node_ignores_the_forwarded_header(self): + """Со снятым переключателем заголовок не участвует вовсе. + + Иначе узел, стоящий в интернете напрямую, доверял бы строке, которую + полностью пишет вызывающий, — то есть каждый выбирал бы себе корзину сам. + """ + self.assertEqual(self._post("direct-key-0001", "203.0.113.10"), 200) + self.assertEqual(self._post("direct-key-0002", "198.51.100.20"), 200) + self.assertEqual(self._post("direct-key-0003", None), 200) + spool = self.service.cfg.spool + buckets = [path.name for path in (spool / "rate").glob("*.json") + if path.name != "_global.json"] + self.assertEqual(len(buckets), 1, buckets) + + class WebhookChannelTestCase(unittest.TestCase): """Канал «через Home Assistant»: что уходит и что остаётся."""