diff --git a/docs/BACKLOG.md b/docs/BACKLOG.md index f9b740e82..6d507991e 100644 --- a/docs/BACKLOG.md +++ b/docs/BACKLOG.md @@ -17317,3 +17317,59 @@ committed alongside the fix rather than filed separately, since neither makes se **Not done:** pushing, opening a PR, or anything that would exercise this in real CI. Committed and handed to the Lander. + +## 1370. operator-supplied names reach /ui URL paths unencoded or half-encoded, so a name carrying a slash addresses a different route + +> 🔢 **Filed 2026-08-27 (builder 2) - BUILT IN THIS COMMIT, not yet landed.** Gap 2 of the two the Lander scoped on **PR 530**, this seat's own abandoned lane, re-implemented against current `main` rather than cherry-picked. +> Verdict: build +> Closing-act: code + +**Cluster:** Web console security. **Priority:** P2. **Verdict:** build. +**Severity:** no engine effect, no PHI axis, and **no deployment axis (sec. 0)** -- with zero deployments this is a condition a FIRST deployment would meet, not an exposure anyone has today. + +**What:** connection names and channel ids are interpolated into `/ui` URL paths. They are **unconstrained free text** -- the registry checks only for a duplicate and no charset gate exists -- so a name carrying `/` splits into two path segments and addresses a different route. + +**MAIN WAS IN TWO DIFFERENT STATES, AND THE HALF-PROTECTED ONE IS THE MORE INTERESTING:** + +``` +connections.py:64 quote(r.name) PARTIAL -- quote's default is safe="/", so the separator passes +connections.py:200 quote(name) PARTIAL -- same +admin.py:363,381 {role.id} RAW +messages.py:618,628 {ch}/{dest} RAW +``` + +**A bare `quote()` reads as protection and provides none against the one character that matters.** Measured, not reasoned: `quote("IB/ACME")` returns it **unchanged**, while `quote("a?b")` and `quote("a#b")` are encoded -- so the call looks like it works everywhere it is tested by hand. `safe=""` is the whole fix. + +**The fix:** `_seg()` in `pages/_common.py`, and the six sites routed through it. + +**NOT A BLANKET SWEEP, AND THE EXCLUSION IS THE LOAD-BEARING PART.** `_auth`'s re-auth `next` carries a whole PATH inside a QUERY parameter, where `safe="/"` is **correct**; routing it through `_seg` would break every re-auth redirect. `connections.py:59` and `:384` keep a bare `quote()` for the same reason -- they build `?channel_id=` query values, not path segments. **A sweep of "every `quote()` call" would ship a broken login**, so a test pins the re-auth encoding against exactly that. + +**Verification:** the seven tests come from the abandoned branch and **pass unmodified against this re-implementation** -- written for a different implementation of the same contract, so they corroborate rather than restate. Five mutants, all killed, each by a distinct red set: `_seg` reverted to the default `safe`; `_seg` over-encoding (the negative control fires); one site reverted to raw; **one site reverted to a bare `quote()` -- caught by the structural scan alone**; the custom-role site reverted. + +**Related:** the sibling gap from the same PR 530 dispatch -- fetch-metadata does not cover the `/ui/static` mount (`_is_ui_fetch_scope` absent from main) -- is **NOT in this commit** and still wants building. + +**Source:** dispatched by the Lander off PR 530 with a per-file measurement. Three searches in that scouting returned false zeros on SPELLING alone: `def test` missing 8 `async def test` functions, `def seg` matching `segment_ids` as a prefix, and `\bseg\(` unable to match `_seg(` because underscore is a word character. +## 1371. fetch-metadata never reaches the /ui/static mount, because a route dependency cannot run for a Mount + +> 🔢 **Filed 2026-08-27 (builder 2) - BUILT IN THIS COMMIT, not yet landed.** Gap 1 of the two the Lander scoped on **PR 530**, re-implemented against current `main` rather than cherry-picked. +> Verdict: build +> Closing-act: code + +**Cluster:** Web console security. **Priority:** P2. **Verdict:** build. +**Severity:** no engine effect, no PHI axis, and **no deployment axis (sec. 0)** -- a condition a FIRST deployment would meet, not an exposure anyone has today. + +**What:** `main` DOES check fetch-metadata -- `_auth`'s per-route helper refuses any request whose `Sec-Fetch-Site` says cross-site. **But `/ui/static` is a Starlette `Mount`, not an `APIRoute`, so no route dependency ever runs for it.** The asset tier is the one `/ui` surface the per-route check cannot reach, and at a glance the console looks covered because the helper exists and is used across five modules. + +**The fix is MIDDLEWARE, which is the only tier that sees a Mount.** `UiFetchMetadataMiddleware` plus `_is_ui_fetch_scope`, deliberately WIDER than the existing `_is_ui_html_path`: that predicate excludes `/ui/static` correctly, because CSP headers only apply to HTML. **Collapsing the two would remove this check's only purpose.** + +**THREE CARVE-OUTS, EACH OF WHICH LOOKS LIKE A WEAKNESS AND IS NOT.** Every one is pinned by a test, and every corresponding mutant is a plausible hardening pass: + +- **A cross-site top-level NAVIGATION passes.** An intranet link into the console is one; so is the OIDC callback, cross-site by construction. Without reading `Sec-Fetch-Mode`, **every real SSO login would 403 while every hermetic test still passed**. Method is part of safe -- a cross-site navigation carrying a POST is a CSRF submission -- and `object`/`embed` are refused because that is framing, not navigation. +- **An ABSENT header passes.** `Sec-Fetch-Site` is browser-populated; old browsers, reporting agents and every non-browser client omit it. Failing closed would refuse **the shipped Windows tray's own `GET /ui` probe**, which builds its client with no headers at all. +- **403, NEVER 404.** The tray classifies 404 as DISABLED and every other status as ENABLED, so a "do not disclose the route" pass would make a healthy console report as switched off. + +**Verification:** the eight tests come from the abandoned branch and **pass unmodified against this re-implementation** -- written for a different implementation of the same contract. Five mutants, all killed, each by a distinct red set, and each is a change someone would plausibly propose as an improvement: narrow the scope to the HTML predicate; fail closed on absence; return 404; drop the navigation carve-out; admit framing. Full webconsole suite 393 passed, 3 skipped. + +**Related:** #1370, the sibling gap from the same PR 530 dispatch. Both are now built; PR 530's branch itself remains superseded and should not be cherry-picked. + +**Source:** dispatched by the Lander off PR 530. Its brief called this file a possible silent-green test file -- 180 lines with zero `def test`. It collects **8**; they are `async def test`, and a pattern anchored on `def test` cannot match one. diff --git a/messagefoundry_webconsole/_security.py b/messagefoundry_webconsole/_security.py index 49f903aae..c220e1a44 100644 --- a/messagefoundry_webconsole/_security.py +++ b/messagefoundry_webconsole/_security.py @@ -172,9 +172,10 @@ import secrets from starlette.datastructures import MutableHeaders +from starlette.responses import PlainTextResponse from starlette.types import ASGIApp, Message, Receive, Scope, Send -from ._auth import browser_hardening_enabled, security_headers_context +from ._auth import _CROSS_ORIGIN_FETCH, browser_hardening_enabled, security_headers_context from ._html import reset_csp_nonce, set_csp_nonce #: The route (registered in :mod:`.routes.core`) the browser POSTs CSP violation reports to, and the @@ -222,6 +223,84 @@ def _is_ui_html_path(path: str) -> bool: return (path == "/ui" or path.startswith("/ui/")) and not path.startswith("/ui/static") +def _is_ui_fetch_scope(path: str) -> bool: + """Every /ui path INCLUDING the static mount -- deliberately WIDER than :func:`_is_ui_html_path`. + + The asset tier is exactly what a per-route validator cannot reach: ``/ui/static`` is mounted as a + Starlette ``Mount``, not registered as an ``APIRoute``, so a route dependency never runs for it. + THAT GAP IS THE REASON THIS CHECK IS MIDDLEWARE RATHER THAN A DEPENDENCY, so excluding the mount + here would remove its only purpose. The narrower predicate above is correct for CSP headers, which + only apply to HTML; do not collapse the two. + """ + return path == "/ui" or path.startswith("/ui/") + + +#: A cross-site request that is a SAFE TOP-LEVEL NAVIGATION is allowed -- an intranet link into the +#: console is one, and so is the OIDC callback, where the IdP's redirect back is cross-site BY +#: CONSTRUCTION. METHOD is part of safe: a cross-site navigation carrying a POST is a CSRF form +#: submission, and no supported flow makes one. +_SAFE_NAVIGATION_METHODS = frozenset({"GET", "HEAD"}) +#: ``object``/``embed`` pull a subresource into someone else's page while still reporting +#: ``Sec-Fetch-Mode: navigate``. That is framing, not navigation, so it does not get the carve-out. +_FRAMING_DESTINATIONS = frozenset({"object", "embed"}) + + +class UiFetchMetadataMiddleware: + """Refuse a /ui request the BROWSER ITSELF labels cross-site (BACKLOG #1371, ASVS 3.5.3). + + It shares its membership set with ``_auth``'s per-route cross-site check, lifted to middleware so + it ALSO covers the ``/ui/static`` Mount that route dependencies cannot see -- but it is NOT that + check at a wider scope. The per-route helper guards hand-picked routes where nothing legitimate + EVER arrives cross-site; applying that bare set to every /ui request would add top-level + NAVIGATIONS, which those callers never see. + + **A CROSS-SITE TOP-LEVEL NAVIGATION IS LEGITIMATE AND MUST PASS.** An intranet link into the + console is one; so is the OIDC callback. Without reading ``Sec-Fetch-Mode`` as well, every real + SSO login would 403 while every hermetic test still passed. + + **ABSENT IS ALLOWED, AND THAT IS THE LOAD-BEARING HALF.** ``Sec-Fetch-Site`` is browser-populated: + an old browser, an out-of-band reporting agent, and every non-browser client omit it entirely. + Failing closed on absence would refuse the shipped Windows tray's own ``GET /ui`` probe, which + builds its client with no headers at all. So this rejects only a header that is PRESENT and says + cross-site or same-site. + + **403, NEVER 404.** The tray classifies 404 as DISABLED and every other status as ENABLED, so a + 404 here would make it report a healthy console as switched off. A later "return 404 rather than + disclose the route" hardening pass would look like an improvement and silently break the tray. + """ + + def __init__(self, app: ASGIApp) -> None: + self.app = app + + async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: + if scope["type"] != "http" or not _is_ui_fetch_scope(scope.get("path", "")): + await self.app(scope, receive, send) + return + # Read from the raw scope rather than building a Request: this runs for every /ui asset, and + # header names on the wire are lower-cased bytes by ASGI contract. + site = mode = dest = None + for key, value in scope.get("headers") or (): + if key == b"sec-fetch-site": + site = value.decode("latin-1") + elif key == b"sec-fetch-mode": + mode = value.decode("latin-1") + elif key == b"sec-fetch-dest": + dest = value.decode("latin-1") + if site is None or site not in _CROSS_ORIGIN_FETCH: + await self.app(scope, receive, send) + return + if ( + mode == "navigate" + and str(scope.get("method", "")).upper() in _SAFE_NAVIGATION_METHODS + and dest not in _FRAMING_DESTINATIONS + ): + await self.app(scope, receive, send) + return + await PlainTextResponse("cross-site request rejected", status_code=403)( + scope, receive, send + ) + + class UiSecurityHeadersMiddleware: """Pure-ASGI /ui browser-security hardening (see the module docstring).""" diff --git a/messagefoundry_webconsole/mount.py b/messagefoundry_webconsole/mount.py index a71b6258f..c0c44fc2c 100644 --- a/messagefoundry_webconsole/mount.py +++ b/messagefoundry_webconsole/mount.py @@ -24,7 +24,7 @@ from messagefoundry.api._ui_seam import UiDeps from . import STATIC_DIR, _auth, assert_engine_seam, pages -from ._security import UiSecurityHeadersMiddleware +from ._security import UiFetchMetadataMiddleware, UiSecurityHeadersMiddleware from ._static import AllowlistedStaticFiles from .routes import ( account, @@ -101,3 +101,8 @@ def mount_ui(app: FastAPI, deps: UiDeps) -> None: # See :mod:`._security`. if not any(getattr(m, "cls", None) is UiSecurityHeadersMiddleware for m in app.user_middleware): app.add_middleware(UiSecurityHeadersMiddleware) + # Fetch-metadata (BACKLOG #1371). Registered the same idempotent way, and separately from the + # headers middleware because its SCOPE is deliberately wider -- it must reach the /ui/static Mount, + # which no route dependency can see. + if not any(getattr(m, "cls", None) is UiFetchMetadataMiddleware for m in app.user_middleware): + app.add_middleware(UiFetchMetadataMiddleware) diff --git a/messagefoundry_webconsole/pages/_common.py b/messagefoundry_webconsole/pages/_common.py index 6312c7c8f..023dd830d 100644 --- a/messagefoundry_webconsole/pages/_common.py +++ b/messagefoundry_webconsole/pages/_common.py @@ -8,6 +8,8 @@ from __future__ import annotations +from urllib.parse import quote + def _num(value: object) -> str: """Render a count/None as text ('—' for None).""" @@ -19,3 +21,23 @@ def _secs(value: float | None) -> str: if value is None: return "—" return f"{value:.0f}s" + + +def _seg(value: object) -> str: + """Percent-encode ONE path segment, INCLUDING ``/`` (BACKLOG #1370). + + ``quote`` DEFAULTS TO ``safe="/"``, which leaves alone the single character a path segment turns + on. Measured rather than reasoned: ``quote("IB/ACME")`` returns it UNCHANGED, so a name carrying a + slash silently becomes two segments and addresses a different route. ``safe=""`` is the whole fix, + and it is why a bare ``quote`` call at one of these sites reads as protection while providing none + against the one character that matters. + + CONNECTION NAMES ARE WHY THIS IS NOT THEORETICAL. They are unconstrained free text -- the registry + checks only for a duplicate and no charset gate exists -- so the "every interpolated id is a + ``uuid4().hex``" argument that covers most /ui interpolations is FALSE for them. + + NOT FOR A PATH LEGITIMATELY CARRIED IN A QUERY PARAMETER. ``_auth``'s re-auth ``next`` uses + ``safe="/"`` deliberately, and routing it through here would break it. These sites are partitioned + by READING each one, never by a blanket builder. + """ + return quote(str(value), safe="") diff --git a/messagefoundry_webconsole/pages/admin.py b/messagefoundry_webconsole/pages/admin.py index 23319523a..c956b47b5 100644 --- a/messagefoundry_webconsole/pages/admin.py +++ b/messagefoundry_webconsole/pages/admin.py @@ -23,6 +23,7 @@ ) from .._html import Markup, el, page, register_nav, rows_table +from ._common import _seg __all__ = [ "ad_groups_page", @@ -360,7 +361,7 @@ def role_form_page( description if description is not None else (role.description or "" if role else "") ) perm_checked = checked if checked is not None else (role.permissions if role else ()) - action = f"/ui/roles/custom/{role.id}/update" if role else "/ui/roles/custom" + action = f"/ui/roles/custom/{_seg(role.id)}/update" if role else "/ui/roles/custom" form = el( "form", el("label", "Name", el("input", name="display_name", value=name_value, autofocus=True)), @@ -378,7 +379,7 @@ def role_form_page( "form", el("button", "Delete role", type="submit"), method="post", - action=f"/ui/roles/custom/{role.id}/delete", + action=f"/ui/roles/custom/{_seg(role.id)}/delete", class_="ctl", ) ) diff --git a/messagefoundry_webconsole/pages/connections.py b/messagefoundry_webconsole/pages/connections.py index 75ab31884..d25940c64 100644 --- a/messagefoundry_webconsole/pages/connections.py +++ b/messagefoundry_webconsole/pages/connections.py @@ -17,7 +17,7 @@ from messagefoundry.api.models import ConnectionEventInfo, ConnectionRow from .._html import Markup, el, page, rows_table, text -from ._common import _num, _secs +from ._common import _num, _secs, _seg __all__ = [ "bulk_control_result", @@ -61,7 +61,7 @@ def _name_cell(r: ConnectionRow) -> Markup: el( "a", "ⓘ", - href=f"/ui/connection/{quote(r.name)}", + href=f"/ui/connection/{_seg(r.name)}", class_="detail-link", title="Connection details", aria_label=f"Details for {_display_name(r.name)}", @@ -197,7 +197,7 @@ def _flag_cell(r: ConnectionRow) -> Markup: aria_label=("Unflag " if r.flagged else "Flag ") + name, ), method="post", - action=f"/ui/connections/{quote(name)}/flag", + action=f"/ui/connections/{_seg(name)}/flag", class_="ctl flagform", ) diff --git a/messagefoundry_webconsole/pages/messages.py b/messagefoundry_webconsole/pages/messages.py index 3c501b1dd..c105b059a 100644 --- a/messagefoundry_webconsole/pages/messages.py +++ b/messagefoundry_webconsole/pages/messages.py @@ -21,6 +21,7 @@ from messagefoundry.parsing.tree import TreeNode from .._html import Markup, el, page, rows_table, text +from ._common import _seg __all__ = [ "dead_letter_pending", @@ -615,7 +616,7 @@ def dead_letters(data: DeadLetterList) -> Markup: "form", el("button", f"Replay all dead — {ch}", type="submit"), method="post", - action=f"/ui/dead-letters/{ch}/replay", + action=f"/ui/dead-letters/{_seg(ch)}/replay", class_="ctl", ) for ch in channels @@ -625,7 +626,7 @@ def dead_letters(data: DeadLetterList) -> Markup: "form", el("button", f"Replay {ch} → {dest}", type="submit"), method="post", - action=f"/ui/dead-letters/{ch}/{dest}/replay", + action=f"/ui/dead-letters/{_seg(ch)}/{_seg(dest)}/replay", class_="ctl", ) for ch, dest in pairs diff --git a/packaging/messagefoundry-webconsole/tests/test_ui_fetch_metadata_mount.py b/packaging/messagefoundry-webconsole/tests/test_ui_fetch_metadata_mount.py new file mode 100644 index 000000000..9d33226e5 --- /dev/null +++ b/packaging/messagefoundry-webconsole/tests/test_ui_fetch_metadata_mount.py @@ -0,0 +1,180 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""The cross-site refusal reaches the /ui/static MOUNT, and does not overreach (BACKLOG #1122). + +``assert_not_cross_site`` runs as a route dependency, and ``/ui/static`` is a Starlette ``Mount`` +rather than an ``APIRoute`` — a dependency never runs for it, so the asset tier was the one /ui +surface the per-route check could not reach. That gap is why this is middleware, and the first test +here is the only one that would notice if it were moved back to a dependency. + +The rest pin constraints that each look like a hardening improvement and each break shipped +behaviour. The NAVIGATION one was not caught by this file — ``test_webui.py`` caught it, because a +first cut of this middleware read ``Sec-Fetch-Site`` alone and 403'd every real SSO login. These +tests exist so the rule is pinned where the rule lives: + +* **absent is allowed** — the tray's own ``GET /ui`` probe builds its httpx client with no headers at + all, and 332 headerless /ui call sites exist in this corpus. Failing closed on absence refuses every + non-browser client. +* **403, never 404** — ``tray/probe.py`` classifies 404 as ``DISABLED`` and everything else as + ``ENABLED``, so a 404 here makes the shipped Windows tray report a healthy console as switched off. +* **a cross-site top-level NAVIGATION is allowed** — an intranet link and the OIDC callback redirect + are both cross-site by construction. Refusing them breaks login while every hermetic test that + omits the headers still passes, which is precisely how it got shipped into this branch once. +""" + +from __future__ import annotations + +import httpx + +from messagefoundry.api import create_app +from messagefoundry.auth.service import AuthService +from messagefoundry.config.settings import AuthSettings +from messagefoundry.pipeline import Engine + + +async def _service(engine: Engine) -> AuthService: + service = AuthService(engine.store, AuthSettings(require_mfa=False)) + await service.initialize() + return service + + +def _client(engine: Engine, service: AuthService) -> httpx.AsyncClient: + transport = httpx.ASGITransport(app=create_app(engine, auth=service, serve_ui=True)) + return httpx.AsyncClient(transport=transport, base_url="http://t") + + +async def test_the_static_mount_is_covered_which_a_route_dependency_cannot_be( + engine: Engine, +) -> None: + """THE REASON THIS IS MIDDLEWARE. Move the check back to a dependency and only this goes red. + + A ``Mount`` runs no route dependencies, so before #1122 a cross-site fetch of an asset was served + normally while the same fetch of an HTML route was refused. + """ + service = await _service(engine) + async with _client(engine, service) as c: + r = await c.get("/ui/static/app.css", headers={"Sec-Fetch-Site": "cross-site"}) + assert r.status_code == 403, ( + f"a cross-site fetch of a /ui/static asset was not refused (got {r.status_code}) — the " + "check is not reaching the Mount" + ) + + +async def test_a_headerless_request_is_allowed_because_the_tray_sends_none( + engine: Engine, +) -> None: + """POSITIVE CONTROL, and the half that would break shipped behaviour if inverted. + + ``Sec-Fetch-Site`` is browser-populated. The tray probe, every non-browser client and 332 call + sites in this corpus omit it entirely. This must NOT 403 — if it does, the tray's console item + and most of this suite go with it. + """ + service = await _service(engine) + async with _client(engine, service) as c: + r = await c.get("/ui/static/app.css") + assert r.status_code != 403, "a headerless request was refused; absence must be allowed" + + +async def test_a_refusal_is_403_and_never_404_because_404_disables_the_tray( + engine: Engine, +) -> None: + """404 would look like route-disclosure hardening and would silently disable the tray's console. + + ``tray/probe.py`` maps 404 to ``DISABLED`` and EVERY other status to ``ENABLED``, so the status + choice here is load-bearing on a different component's UI. + """ + service = await _service(engine) + async with _client(engine, service) as c: + r = await c.get("/ui", headers={"Sec-Fetch-Site": "cross-site"}) + assert r.status_code == 403, f"expected 403, got {r.status_code}" + assert r.status_code != 404, "404 makes the Windows tray report a healthy console as DISABLED" + + +async def test_same_origin_and_none_still_pass(engine: Engine) -> None: + """SECOND POSITIVE CONTROL: a guard that refused everything would satisfy the first test alone.""" + service = await _service(engine) + async with _client(engine, service) as c: + for site in ("same-origin", "none"): + r = await c.get("/ui/static/app.css", headers={"Sec-Fetch-Site": site}) + assert r.status_code != 403, f"Sec-Fetch-Site: {site} must not be refused" + + +async def test_a_cross_site_top_level_navigation_is_allowed_because_a_real_login_is_one( + engine: Engine, +) -> None: + """THE REGRESSION THIS FILE MISSED FIRST TIME. Reading ``Sec-Fetch-Site`` alone 403s every SSO login. + + The IdP redirect back to ``/ui/oidc/callback`` and a plain intranet link into the console are both + ``Sec-Fetch-Site: cross-site`` with ``Sec-Fetch-Mode: navigate``. ``_auth``'s per-route helper never + sees one — its callers are a CSP sink and state-changing POSTs — so lifting its membership test to + every /ui request without also reading the MODE refuses traffic the product depends on. + """ + service = await _service(engine) + async with _client(engine, service) as c: + r = await c.get( + "/ui", + headers={"Sec-Fetch-Site": "cross-site", "Sec-Fetch-Mode": "navigate"}, + ) + assert r.status_code != 403, ( + "a cross-site TOP-LEVEL NAVIGATION was refused — this is what an intranet link and the OIDC " + "callback both look like, so this 403 is every real SSO login failing" + ) + + +async def test_a_cross_site_non_navigation_fetch_is_still_refused(engine: Engine) -> None: + """NEGATIVE CONTROL for the carve-out: it must not have opened the door generally. + + A cross-site ``cors`` fetch is the drive-by ambient-auth probe ASVS 3.5.3 is about. Only + ``navigate`` earns the exemption. + """ + service = await _service(engine) + async with _client(engine, service) as c: + r = await c.get( + "/ui/static/app.css", + headers={"Sec-Fetch-Site": "cross-site", "Sec-Fetch-Mode": "cors"}, + ) + assert r.status_code == 403, ( + f"a cross-site non-navigation fetch was allowed (got {r.status_code}) — the navigation " + "carve-out must not cover ordinary fetches" + ) + + +async def test_a_cross_site_navigation_carrying_a_post_is_refused(engine: Engine) -> None: + """METHOD is part of "safe": a cross-site navigation with a POST is a CSRF form submission. + + No supported flow makes one — the OIDC callback is a GET and ``response_mode=form_post`` is not + implemented — so the carve-out is limited to GET/HEAD rather than to ``navigate`` alone. + """ + service = await _service(engine) + async with _client(engine, service) as c: + r = await c.post( + "/ui", + headers={"Sec-Fetch-Site": "cross-site", "Sec-Fetch-Mode": "navigate"}, + ) + assert r.status_code == 403, ( + f"a cross-site POST navigation was not refused (got {r.status_code}) — that is a CSRF form " + "submission wearing the navigation carve-out" + ) + + +async def test_object_and_embed_do_not_get_the_navigation_carve_out(engine: Engine) -> None: + """``object``/``embed`` report ``Sec-Fetch-Mode: navigate`` while loading INTO someone else's page. + + That is framing rather than navigation, so the destination has to be checked too or the carve-out + hands back the embedding it was meant to refuse. + """ + service = await _service(engine) + async with _client(engine, service) as c: + for dest in ("object", "embed"): + r = await c.get( + "/ui", + headers={ + "Sec-Fetch-Site": "cross-site", + "Sec-Fetch-Mode": "navigate", + "Sec-Fetch-Dest": dest, + }, + ) + assert r.status_code == 403, ( + f"Sec-Fetch-Dest: {dest} was allowed through the navigation carve-out (got " + f"{r.status_code}) — that is cross-site framing, not navigation" + ) diff --git a/packaging/messagefoundry-webconsole/tests/test_ui_path_segment_encoding.py b/packaging/messagefoundry-webconsole/tests/test_ui_path_segment_encoding.py new file mode 100644 index 000000000..ce512e5eb --- /dev/null +++ b/packaging/messagefoundry-webconsole/tests/test_ui_path_segment_encoding.py @@ -0,0 +1,167 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Organization and contributors +"""Connection names cannot escape a /ui path segment (ASVS 1.2.2, BACKLOG #1107 clause 2). + +The apiclient half of this item shipped with `_seg` and is pinned by `tests/test_apiclient.py`. The +console half was left: `safe=""` appeared ZERO times in `messagefoundry_webconsole/`, the two sites +that encoded a path segment used `quote`'s DEFAULT `safe="/"`, and the dead-letter replay forms +interpolated a channel id and a destination name with no encoding at all. + +`quote`'s default is the whole defect: measured, `quote("IB/ACME")` returns it UNCHANGED, because the +one character it leaves alone is the one a path segment turns on. + +**All 54 interpolation sites were then partitioned by reading each value's PRODUCER**, not its +interpolation line. That is the only way to answer this: "every interpolated id is a `uuid4().hex`" +is true of most sites and FALSE for connection names, because `Registry._add` checks only for a +duplicate, so a name is unconstrained free text. + +The partition found the id sites genuinely safe, but NOT for the reason usually given. They are safe +because every one is read back from the store after a lookup that 404s on a miss -- so a crafted path +param never reaches a render. **`ui_role_update` is the single exception on the whole surface**, and +the last two tests cover it: a `ValidationError` short-circuits before that lookup runs. + +**A blanket sweep of the remaining sites would be WRONG**, which is why one test exists only to stop +it: `_auth`'s reauth `next` is CORRECTLY `safe="/"` because it carries a whole path inside a query +parameter. It is also safe for a DIFFERENT reason than its own comment implies -- adversarial review +showed attacker-influenceable bytes do reach it, and the `quote()` at the site is what holds. Remove +that call on a "server-generated anyway" argument and it opens. +""" + +from __future__ import annotations + +import pathlib +import re + +from messagefoundry.api.models import DeadLetterList, DeadLetterRow +from messagefoundry_webconsole.pages._common import _seg + + +def test_seg_encodes_the_separator_that_the_default_leaves_alone() -> None: + """The unit fact the rest rests on, with a benign name as the negative control.""" + from urllib.parse import quote + + assert quote("IB/ACME") == "IB/ACME", ( + "the premise of this whole file just changed: quote's default no longer leaves '/' alone" + ) + assert _seg("IB/ACME") == "IB%2FACME" + assert _seg("a?b") == "a%3Fb" + assert _seg("a#b") == "a%23b" + # NEGATIVE CONTROL: a guard that mangled everything would satisfy the assertions above. + assert _seg("IB_ACME_ADT") == "IB_ACME_ADT" + + +def _dead_letters(channel: str, destination: str) -> DeadLetterList: + row = DeadLetterRow( + outbox_id="o1", + message_id="m1", + channel_id=channel, + destination_name=destination, + attempts=1, + last_error=None, + failed_at=0.0, + control_id=None, + message_type=None, + received_at=0.0, + ) + return DeadLetterList(total=1, limit=50, offset=0, dead_letters=[row]) + + +def test_the_dead_letter_replay_forms_encode_a_name_carrying_a_slash() -> None: + """RENDERS the real page rather than reading the f-string, so the assertion is about output. + + These two forms were the unencoded pair: before this fix a connection named ``IB/ACME`` produced + ``/ui/dead-letters/IB/ACME/replay``, which is a different route with an extra segment. + """ + from messagefoundry_webconsole.pages.messages import dead_letters + + html = str(dead_letters(_dead_letters("IB/ACME", "OB/PARTNER"))) + + assert "/ui/dead-letters/IB%2FACME/replay" in html + assert "/ui/dead-letters/IB%2FACME/OB%2FPARTNER/replay" in html + assert "/ui/dead-letters/IB/ACME/" not in html, ( + "the name escaped its path segment; the action addresses a different route" + ) + + +def test_a_benign_connection_name_still_renders_readably() -> None: + """NEGATIVE CONTROL for the render path: encoding must not disfigure ordinary names.""" + from messagefoundry_webconsole.pages.messages import dead_letters + + html = str(dead_letters(_dead_letters("IB_ACME_ADT", "OB_PARTNER_ADT"))) + assert "/ui/dead-letters/IB_ACME_ADT/replay" in html + assert "%5F" not in html, "an unreserved character was percent-encoded" + + +def test_every_connection_name_route_interpolates_through_seg() -> None: + """GUARD THE GUARD: a new site on these routes reds this rather than slipping in unencoded. + + Scans the page builders for f-string path literals on the three routes that carry a connection + name, and requires each interpolation to go through ``_seg``. Mutation: revert any one site to a + bare ``quote(...)`` or a raw ``{name}``. Red: that literal is listed in the failure. + """ + pages = pathlib.Path(__file__).resolve().parents[3] / "messagefoundry_webconsole" / "pages" + literals: list[str] = [] + for path in sorted(pages.glob("*.py")): + for lit in re.findall( + r'f"(/ui/(?:connection|connections|dead-letters)/[^"]*)"', + path.read_text(encoding="utf-8"), + ): + if "{" in lit: + literals.append(f"{path.name}: {lit}") + assert literals, "found NO connection-name path literals -- the scan is broken, not the code" + unencoded = [lit for lit in literals if "_seg(" not in lit] + assert not unencoded, f"connection-name path segments not routed through _seg: {unencoded}" + + +def test_the_reauth_next_parameter_is_left_alone() -> None: + """The site a blanket path-segment sweep would BREAK, pinned so the sweep cannot happen quietly. + + ``_auth``'s reauth ``next`` carries a whole PATH inside a query parameter, so ``safe="/"`` is + correct there. Encoding it as one segment would turn every re-auth redirect into a broken link. + """ + auth = pathlib.Path(__file__).resolve().parents[3] / "messagefoundry_webconsole" / "_auth.py" + source = auth.read_text(encoding="utf-8") + assert 'quote(next_path if next_path is not None else request.url.path, safe="/")' in source, ( + "the reauth 'next' encoding changed; if a path-segment builder was applied here it is wrong " + "-- that value is a path carried in a query parameter" + ) + + +def test_a_rejected_custom_role_submit_cannot_escape_its_path_segment() -> None: + """THE ONE SITE ON THIS SURFACE WHERE THE 404 LOOKUP IS BYPASSED. + + Every other id rendered by the console is read back from the store, so a request path param that + matched nothing 404s before anything renders. ``ui_role_update`` is the exception: a + ``ValidationError`` from ``CustomRoleRequest`` short-circuits BEFORE ``update_custom_role`` runs, + and the 400 branch then rebuilds the page from ``CustomRoleInfo(id=role_id, ...)`` using the RAW + path param. ``CustomRoleInfo.id`` is a bare ``id: str`` with no ``Field`` constraint. + + So an operator who submits an invalid form to a crafted role path gets that path reflected into + the update and delete form actions. Encoded, it stays one segment. + """ + from messagefoundry.api.auth_models import CustomRoleInfo + from messagefoundry_webconsole.pages.admin import role_form_page + + role = CustomRoleInfo(id="custom:abc/evil", display_name="x", description=None, permissions=[]) + html = str(role_form_page(["messages:read"], role=role, error="invalid input")) + + assert "/ui/roles/custom/custom%3Aabc%2Fevil/update" in html + assert "/ui/roles/custom/custom%3Aabc%2Fevil/delete" in html + assert "/ui/roles/custom/custom:abc/evil/" not in html, ( + "the reflected role id escaped its path segment; the form now posts to a different route" + ) + + +def test_a_real_custom_role_id_still_addresses_its_own_route() -> None: + """NEGATIVE CONTROL. A genuine id is ``custom:`` + uuid4().hex, so the colon IS encoded -- that is + harmless (FastAPI decodes the path param back) but it must still be ONE segment, and the benign + case must not be mangled beyond that.""" + from messagefoundry.api.auth_models import CustomRoleInfo + from messagefoundry_webconsole.pages.admin import role_form_page + + role = CustomRoleInfo( + id="custom:0123456789abcdef", display_name="ops", description=None, permissions=[] + ) + html = str(role_form_page(["messages:read"], role=role)) + assert "/ui/roles/custom/custom%3A0123456789abcdef/update" in html + assert "%2F" not in html, "a legitimate id contains no slash, so none should be encoded"