diff --git a/docs/research/instruments-that-returned-confident-wrong-answers.md b/docs/research/instruments-that-returned-confident-wrong-answers.md new file mode 100644 index 00000000..b08e9f19 --- /dev/null +++ b/docs/research/instruments-that-returned-confident-wrong-answers.md @@ -0,0 +1,78 @@ +# Instruments that returned confident wrong answers + +Measured across one session, 2026-08-22, by several sessions working the same tree. Tracked here +rather than left in a coordination handoff because `/mefor-coord/handoffs/` sits +inside `.git`, which git cannot track by construction, so no rescue ref covers it (COMMON 5.6b). + +**Every entry is a real measurement that was correct about what it measured and wrong about what it +was asked.** None is a typo or a slip. That is the point: the failures survive care, so the guard has +to be structural. + +## The single shape + +**An instrument answered a question adjacent to the one asked.** Name the question and the answer in +the same sentence and check they are the same sentence. That is SDS-3.8, and every entry below is an +instance of it. + +## The catalogue + +| The check | What it answered | What was asked | +|---|---|---| +| `grep ... \| head -60` | The first 60 lines of the result | How many sites exist. It FOUND four and PRINTED three | +| `grep -c ` | How many times a name occurs | Whether the code handles it. All 8 hits WERE the handling | +| A count on a settings model | That one real field took | Whether unknown keys are dropped. Only a BOGUS key discriminates | +| `git status` | Does this differ from HEAD | Is someone mid-edit. A restore-to-current renders identically | +| `git status` in a stale checkout | Differs from a HEAD 28 commits behind | Did anyone edit this | +| Two mail timestamps | The gap between two messages | What caused an event three minutes earlier | +| Reading a commit by SHA | What was true at that commit | What is true now. A SHA is a snapshot, a TIP is a state | +| `grep -i "superseded"` on a row | The word is present | The row's status. Both bodies used it as NARRATIVE | +| `parse_items` `is_open` | The BANNER state | Which row the authors say survives. Different questions | +| A `tail`-piped background run | Nothing until exit | Progress. The buffer hid a 0-byte file for a run that never started | +| A harness "exit code 0" | The wrapper's status | Whether the suite passed. It reported 0 over `3 failed` | + +## Four rules that survived the session + +**1. A zero needs a positive control -- AND the control proves the instrument, not the AIM.** +Confirming a pattern fires on a corpus establishes the grep works THERE. It cannot establish that +there is the right place to look. Both halves are needed and the second is the one that gets skipped. + +**2. Not on origin means UNPUBLISHED, not UNREADABLE.** Its mirror: WHAT YOU READ IS NOT NECESSARILY +WHAT IS COMMITTED. Several checks stop at `origin/main` and report unknown when the answer sits in a +sibling worktree. Read through `git show :` so the command NAMES the corpus -- but note +the limit: naming the corpus prevents drift, not misaim. + +**3. Never filter a command whose output IS evidence** -- a count, an id, a receipt, a verdict. A +truncated COUNT is wrong immediately and might be caught. A truncated ARTIFACT is not wrong at all, +it is ABSENT, and absence surfaces only when something downstream needs it. + +**4. A completeness claim is a liability.** Prefer "at least" (SDS-3.6). Two published population +claims in one session were unmeasured extrapolations from a real mechanism, and both had to be +withdrawn. The honest form is "the next one will do this silently", not "these are everywhere". + +## Two shapes that are not instrument failures + +**A STATUS MARKER MUST NOT BE A WORD THAT ALSO APPEARS IN NARRATION.** A row containing +"THIS ROW SURVIVES A CROSS-SUPERSEDE" was read as superseded by three separate readers grepping for +that word; a fourth read "SUPERSEDED BY #1326" in a body paragraph and took it for the row's status. +Both misreadings are presence-equals-meaning -- the positional-meaning defect CLAUDE.md section 11 +gives as the reason not to use status glyphs, reproduced exactly, in prose. + +The rule is not about pictographs. It is about tokens whose meaning depends on the sentence around +them, and PROSE IS NOT IMMUNE: a word carries its scope in the sentence, which is precisely why the +same word used as BOTH a status and a narration cannot. In this repo the banner alphabet is the status +channel and `parse_items` is its only correct reader; anything written in the body is narration, no +matter how emphatic. **Four seats grepped the body for a status word. None of them was careless.** + +**Mutual deference has no fixed point, and neither does mutual assertion.** Two seats deferring to a +third produced two records of one defect; both then yielding to each other produced zero; both then +asserting produced two again. Same courtesy, three directions, never convergence. Only an ASYMMETRIC +decider settles it -- and by position rather than by judgement, since a merit tie-break between two +good options is still a tie. + +## The one that generalises furthest + +**A test that cannot fail in the direction of the bug will certify it.** Observed four times: a test +asserting a string the loader rejects; a gate reading a refusal and reporting `skipped`; a settings +object silently dropping the flag a test is named after; an assertion reading `.path` where httpx +decodes what the fix encodes. **Mutation testing is the only thing that answers "can this test fail +at all", and it caught two tests written by the person applying the rule.** diff --git a/messagefoundry_webconsole/_security.py b/messagefoundry_webconsole/_security.py index 49f903aa..11db9427 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,92 @@ 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`` in :func:`.mount.mount_ui`, 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. + """ + return path == "/ui" or path.startswith("/ui/") + + +#: A cross-site request that is a SAFE TOP-LEVEL NAVIGATION is allowed -- intranet links and the OIDC +#: callback are both cross-site by construction. Anything outside this set arriving cross-site as a +#: navigation is a CSRF form submission, which :func:`._auth.assert_same_origin` also refuses per-route. +_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 #1122, ASVS 3.5.3). + + It shares the membership set with :func:`._auth.assert_not_cross_site`, 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, and building it as one is a defect the suite catches. That helper guards + hand-picked routes (a CSP report sink, state-changing POSTs) where nothing legitimate EVER arrives + cross-site; its name says FETCH because its callers have already established that. Applying the + bare set to every /ui request adds 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, where the IdP redirect back is cross-site BY + CONSTRUCTION. ``test_oidc_callback_survives_a_cross_site_navigation`` exists to say exactly that, + and warns that otherwise *"every real login would 403 while every hermetic test still passed"*. + So the refusal must read ``Sec-Fetch-Mode`` too, and fires only when the request is NOT a safe + top-level navigation. METHOD is part of safe: a cross-site navigation carrying a POST is a CSRF + form submission, and no supported flow makes one (the OIDC leg is a GET; ``response_mode=form_post`` + is not implemented here). ``object``/``embed`` destinations are refused because they are framing + rather than navigation. + + **ABSENT IS ALLOWED, AND THAT IS THE LOAD-BEARING HALF.** ``Sec-Fetch-Site`` is browser-populated: + an old browser, a user-agent's 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 + (``tray/probe.py`` builds its client with no headers at all) and 332 headerless call sites in the + /ui test corpus -- MEASURED, both. ``_auth`` already records the same reasoning for the CSP report + sink, where a strict check "would 403 every modern report and silently blind the 3.7.5 canary". + So this rejects only a header that is PRESENT and says cross-site or same-site. + + **403, NEVER 404.** ``tray/probe.py`` classifies 404 as ``DISABLED`` and every other status as + ``ENABLED``, so a 404 here would make the tray 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; the tests pin both halves. + """ + + 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 a71b6258..15c8cfbd 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,10 @@ 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) + + # BACKLOG #1122 (ASVS 3.5.3): the cross-site refusal lifted from a route dependency to middleware, + # because /ui/static is a 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. Same re-mount guard as + # above, and the same append-by-pattern contract. + 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 6312c7c8..82c1107b 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,24 @@ 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 ``/`` (ASVS 1.2.2, BACKLOG #1107). + + ``quote`` defaults to ``safe="/"``, which leaves 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. + + CONNECTION NAMES are why this is not theoretical. They are unconstrained free text -- + ``Registry._add`` 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. The remaining id-carrying sites are deliberately NOT routed through here yet: that argument + is probably true of them, but it rests on a data-grammar invariant no line of URL-building code + asserts, and deciding it is a separate piece of work. + + NOT for a path legitimately carried in a QUERY parameter. ``_auth``'s reauth ``next`` uses + ``safe="/"`` on purpose, and routing it through here would break it -- the reason the research on + #1107 says to partition these sites by READING each one rather than by a blanket builder. + """ + return quote(str(value), safe="") diff --git a/messagefoundry_webconsole/pages/admin.py b/messagefoundry_webconsole/pages/admin.py index 23319523..4d8981d9 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,11 @@ 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" + # _seg, NOT bare interpolation: on a rejected submit `ui_role_update` rebuilds this page from + # `CustomRoleInfo(id=role_id, ...)` where role_id is the RAW path param -- a ValidationError on + # CustomRoleRequest short-circuits before `update_custom_role` runs, so the 404 lookup that + # constrains every OTHER id on this surface never happens (BACKLOG #1107, ASVS 1.2.2). + 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 +383,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 75ab3188..d25940c6 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 3c501b1d..c105b059 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 00000000..9d33226e --- /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 00000000..ce512e5e --- /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"