diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ad846c..53ce97c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,40 @@ skip versions are the composition of each intervening adjacent release's steps in order — no A-to-B path is pre-computed beyond that. +## 0.28.0 — 2026-05-28 + +**Minor — security-hardening follow-up (Session-0026 audit, informational +findings I3 + I4). No operator action required: no migration, no schema +change, no config/overlay change, no API/behavior change for any caller. +A plain code deploy picks it up. Shipped from driver session 0032.0.** + +Two informational findings from the Session-0026 audit, both +framework-internal defense-in-depth: + +- **I3 — dead HTML-email branch guarded.** `email_envelope.build_envelope` + accepted a `body_html=` argument that built a `multipart/alternative` + body, but no send path ever passed it — every rfc-app mail is plain + text. An unused branch that would emit HTML built from (potentially + unescaped) user content is the C1 stored-XSS class waiting in the mail + channel. The branch is now a loud guard: passing `body_html` raises + `NotImplementedError`. The argument is kept in the signature for + documented future symmetry; enabling HTML mail becomes a deliberate + change that MUST HTML-escape user content at the call site and remove + the guard in the same commit. +- **I4 — Turnstile siteverify no longer blocks the event loop.** + `turnstile.verify_token` was a synchronous function issuing a blocking + `httpx.post` from inside the async `/auth/otc/request` handler, so a + slow CloudFlare response stalled the single worker for up to the 10s + timeout. It is now `async` and awaits the call on an + `httpx.AsyncClient` (matching the codebase's existing async-httpx + pattern), isolated behind a narrow `_siteverify_post` seam. The sole + caller (`main.py`) now `await`s it. + +Upgrade steps: **none.** Both changes are internal. The `verify_token` +signature changed from sync to `async` (callers must `await`), but the +only caller is in-tree (`main.py`) and is updated in this release; no +deployment-facing surface, config key, or migration is affected. + ## 0.27.0 — 2026-05-28 **Minor — security-hardening release (Session-0026 audit remediation). diff --git a/VERSION b/VERSION index 1b58cc1..697f087 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.27.0 +0.28.0 diff --git a/backend/app/email_envelope.py b/backend/app/email_envelope.py index 78d011e..b443c4e 100644 --- a/backend/app/email_envelope.py +++ b/backend/app/email_envelope.py @@ -60,12 +60,17 @@ def build_envelope( `from_name` is the display label that goes through `formataddr` so spaces / commas in the display string are encoded correctly. - `body_plain` is mandatory. `body_html`, if supplied, lands as the - second part of a `multipart/alternative` body — mail clients - that prefer HTML render it; clients that don't fall back to the - plain part. The text/plain part comes first per RFC 2046, so a - plain-text client that picks the first body gets the readable - text. + `body_plain` is mandatory. `body_html` is **reserved and not yet + enabled** (security-audit-0026 I3): no send path supplies it today — + every rfc-app mail is plain text — and passing it raises + `NotImplementedError`. The parameter is kept in the signature for + documented future symmetry: when HTML mail is enabled it will land + as the second part of a `multipart/alternative` body (text/plain + first per RFC 2046, so a plain-text client picking the first part + still gets the readable text). Enabling it is a deliberate act — the + caller MUST HTML-escape any user content into `body_html` first (cf. + the C1 stored-XSS class: a mail client renders the HTML) and remove + the guard below in the same change. `reply_to`, when set, lets a send path point replies at a different mailbox than the From line (e.g., a watcher @@ -131,13 +136,20 @@ def build_envelope( # idempotent and not require auth. See # `api_notifications.py` for the receiver. msg["List-Unsubscribe-Post"] = "List-Unsubscribe=One-Click" - if body_html: - # multipart/alternative: text/plain first, text/html second. - # `set_content` sets the first part (and the message's main - # body); `add_alternative` adds the second part and - # restructures the message as multipart/alternative. - msg.set_content(body_plain) - msg.add_alternative(body_html, subtype="html") - else: - msg.set_content(body_plain) + if body_html is not None: + # I3 (security-audit-0026): the multipart/alternative HTML path + # is intentionally NOT enabled. No send path passes `body_html` + # today, and emitting an HTML body built from user-supplied + # content without escaping it first would reintroduce the C1 + # stored-XSS class in the mail channel (the recipient's client + # renders the HTML). Fail loudly here rather than silently + # shipping HTML: enabling HTML mail is a deliberate change that + # MUST HTML-escape user content at the call site and remove this + # guard together. The text/plain path below is the only live one. + raise NotImplementedError( + "HTML email is not enabled (security-audit-0026 I3): do not " + "pass body_html until user content is HTML-escaped at the " + "call site and this guard is intentionally removed." + ) + msg.set_content(body_plain) return msg diff --git a/backend/app/main.py b/backend/app/main.py index 18d99c2..f580a28 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -267,7 +267,7 @@ def _oauth_router(config) -> APIRouter: # secret AND TURNSTILE_REQUIRED=false (the default), the gate # opens — see `backend/app/turnstile.py` for the full matrix. client_ip = request.client.host if request.client else None - ts = turnstile.verify_token(body.turnstile_token, client_ip=client_ip) + ts = await turnstile.verify_token(body.turnstile_token, client_ip=client_ip) if not ts.ok: if ts.reason == "misconfigured": # TURNSTILE_REQUIRED=true but the secret is unset. This diff --git a/backend/app/turnstile.py b/backend/app/turnstile.py index d8efc24..1f7301d 100644 --- a/backend/app/turnstile.py +++ b/backend/app/turnstile.py @@ -73,6 +73,19 @@ def _siteverify_url() -> str: return os.environ.get("TURNSTILE_SITEVERIFY_URL", "").strip() or SITEVERIFY_URL +async def _siteverify_post(url: str, data: dict) -> httpx.Response: + """Perform the siteverify POST on an `httpx.AsyncClient`. + + Isolated as a narrow seam (I4, security-audit-0026): the call is + awaited so a slow CloudFlare response can't block the event loop, + and tests patch *this function* rather than the shared + `httpx.AsyncClient` (which other modules — gitea, docs — also + construct, so a global patch would break app boot). + """ + async with httpx.AsyncClient(timeout=10.0) as client: + return await client.post(url, data=data) + + @dataclass class VerifyOutcome: """Result of a Turnstile siteverify call. @@ -98,7 +111,7 @@ class VerifyOutcome: reason: str -def verify_token(token: str | None, *, client_ip: str | None = None) -> VerifyOutcome: +async def verify_token(token: str | None, *, client_ip: str | None = None) -> VerifyOutcome: """Validate a Turnstile token against CloudFlare's siteverify endpoint. Returns a VerifyOutcome describing whether the calling endpoint @@ -108,9 +121,14 @@ def verify_token(token: str | None, *, client_ip: str | None = None) -> VerifyOu * 'misconfigured' → 500 "auth misconfigured" * 'missing-token' / 'failed' / 'network' → 400 "verification failed" - Tests monkeypatch `httpx.post` (or set `TURNSTILE_SITEVERIFY_URL` - + a MockTransport client) to avoid touching the real CloudFlare - endpoint. No real keys are ever embedded in tests. + Async (I4, security-audit-0026): the siteverify call is awaited on an + `httpx.AsyncClient` so a slow CloudFlare response can't block the + event loop (the prior synchronous `httpx.post` stalled the single + worker for up to the 10s timeout). Callers must `await` it. + + Tests monkeypatch `_siteverify_post` (the narrow async seam) to avoid + touching the real CloudFlare endpoint and to keep the patch off the + shared `httpx.AsyncClient`. No real keys are ever embedded in tests. """ secret = _secret() required = _required() @@ -133,7 +151,7 @@ def verify_token(token: str | None, *, client_ip: str | None = None) -> VerifyOu data["remoteip"] = client_ip try: - response = httpx.post(_siteverify_url(), data=data, timeout=10.0) + response = await _siteverify_post(_siteverify_url(), data) payload = response.json() except Exception as exc: # network, JSON parse, etc. log.warning("Turnstile siteverify call failed: %s", exc) diff --git a/backend/tests/test_email_envelope.py b/backend/tests/test_email_envelope.py index 9ad20be..05cd684 100644 --- a/backend/tests/test_email_envelope.py +++ b/backend/tests/test_email_envelope.py @@ -11,6 +11,8 @@ from __future__ import annotations from email.utils import parsedate_to_datetime +import pytest + from app.email_envelope import build_envelope @@ -160,14 +162,18 @@ def test_envelope_plain_only_body_is_text_plain(): assert msg.get_content().strip() == "Hello, world." -def test_envelope_with_html_is_multipart_alternative(): - msg = build_envelope(**_base_kwargs(body_html="

Hello, world.

")) - assert msg.get_content_type() == "multipart/alternative" - # Two parts: text/plain first (so plain-text clients picking the - # first part get the readable text), text/html second. - parts = list(msg.iter_parts()) - assert len(parts) == 2 - assert parts[0].get_content_type() == "text/plain" - assert parts[1].get_content_type() == "text/html" - assert "Hello, world." in parts[0].get_content() - assert "world" in parts[1].get_content() +def test_envelope_html_body_is_guarded_not_enabled(): + # I3 (security-audit-0026): the HTML/multipart-alternative path is + # intentionally not enabled — passing body_html must fail loudly so + # a future caller can't silently ship unescaped user HTML (the C1 + # stored-XSS class in the mail channel). When HTML mail is enabled + # deliberately, this test flips to assert the multipart shape. + with pytest.raises(NotImplementedError): + build_envelope(**_base_kwargs(body_html="

Hello, world.

")) + + +def test_envelope_html_none_is_plain_only(): + # The guard keys on `is not None`, so the default (None) stays the + # live plain-text path — exercised here to lock the boundary. + msg = build_envelope(**_base_kwargs(body_html=None)) + assert msg.get_content_type() == "text/plain" diff --git a/backend/tests/test_turnstile_vertical.py b/backend/tests/test_turnstile_vertical.py index db7316b..f420bfe 100644 --- a/backend/tests/test_turnstile_vertical.py +++ b/backend/tests/test_turnstile_vertical.py @@ -55,13 +55,19 @@ def _outbound_otc_envelopes(to_address: str | None = None) -> list[dict]: def _patch_siteverify(monkeypatch, *, success: bool, error_codes: list[str] | None = None): - """Replace `httpx.post` inside `app.turnstile` with a stub that + """Replace `turnstile._siteverify_post` with an async stub that returns the requested success shape. The stub does not touch the real CloudFlare endpoint and never sees a real secret. + + I4 (security-audit-0026): the siteverify call is now awaited on an + `httpx.AsyncClient`, isolated behind the `_siteverify_post` seam. + Patching that narrow function (rather than the shared + `httpx.AsyncClient`, which gitea/docs also construct) keeps app boot + intact. """ captured = {} - def fake_post(url, *, data=None, timeout=None, **kwargs): + async def fake_post(url, data): captured["url"] = url captured["data"] = data body = {"success": bool(success)} @@ -70,7 +76,7 @@ def _patch_siteverify(monkeypatch, *, success: bool, error_codes: list[str] | No return SimpleNamespace(json=lambda: body) from app import turnstile as turnstile_mod - monkeypatch.setattr(turnstile_mod.httpx, "post", fake_post) + monkeypatch.setattr(turnstile_mod, "_siteverify_post", fake_post) return captured @@ -170,14 +176,15 @@ def test_otc_request_admits_when_secret_unset_and_not_required(app_with_fake_git monkeypatch.delenv("CLOUDFLARE_TURNSTILE_SECRET", raising=False) monkeypatch.delenv("TURNSTILE_REQUIRED", raising=False) - # The httpx.post inside turnstile must not be called in this path — - # patch it to a sentinel that explodes if it ever runs. + # The siteverify call inside turnstile must not be made in this path — + # patch the seam to a sentinel that explodes if it ever runs + # (I4: the seam is now `_siteverify_post`, not module-level httpx.post). from app import turnstile as turnstile_mod - def must_not_be_called(*a, **kw): + async def must_not_be_called(*a, **kw): raise AssertionError("siteverify should not run when no secret is configured") - monkeypatch.setattr(turnstile_mod.httpx, "post", must_not_be_called) + monkeypatch.setattr(turnstile_mod, "_siteverify_post", must_not_be_called) app, _fake = app_with_fake_gitea with TestClient(app) as client: @@ -218,3 +225,27 @@ def test_otc_request_refuses_when_required_but_secret_unset(app_with_fake_gitea, ) assert r.status_code == 500, r.text assert _outbound_otc_envelopes("alice@example.com") == [] + + +# --------------------------------------------------------------------------- +# I4 (security-audit-0026): verify_token is a coroutine — calling it returns +# an awaitable, not a VerifyOutcome. Locks the async contract so a revert to +# the synchronous event-loop-blocking shape fails here, not just in the +# integration paths. +# --------------------------------------------------------------------------- + + +def test_verify_token_is_async_and_soft_skips_without_secret(monkeypatch): + import asyncio + + from app import turnstile as turnstile_mod + + monkeypatch.delenv("CLOUDFLARE_TURNSTILE_SECRET", raising=False) + monkeypatch.delenv("TURNSTILE_REQUIRED", raising=False) + + coro = turnstile_mod.verify_token("any-token") + assert asyncio.iscoroutine(coro), "verify_token must be a coroutine (I4)" + outcome = asyncio.run(coro) + # No secret + not required → the gate stays open without any network call. + assert outcome.ok is True + assert outcome.reason == "skipped" diff --git a/frontend/package.json b/frontend/package.json index b299b6d..6beccd6 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,7 +1,7 @@ { "name": "rfc-app-frontend", "private": true, - "version": "0.27.0", + "version": "0.28.0", "type": "module", "scripts": { "dev": "vite",