From 80b836029ab5e6297cc2aee0c81c00bd13411ac5 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Mon, 19 Oct 2026 14:38:12 -0500 Subject: [PATCH 1/6] A sign-out's auth.logout row records the client address (BACKLOG #3339) AuthService.logout takes an optional client keyword and records it on the auth.logout audit row. POST /auth/logout and /ui/logout both pass client_ip(request), the extractor the other audited auth routes use. Closing step 2 was already met by PR 2384 (BACKLOG #3029): a sign-out that revokes nothing writes its own recorded detail.outcome, nothing_to_revoke. Unchanged here. Not fixed: a console sign-out that loses a rotation race keeps a NULL actor. The rotated row no longer answers to the presented hash, so a lookup by it finds nothing; naming the owner needs the store to keep a re-keyed session's prior hash, on three backends. Tests read the row back for each route (actor, client, outcome), with a pinned peer. Control: /ui/logout with a cookie naming no session records nothing_to_revoke, no actor, and the address. The web console seam digest moved (5d9c162790077dfc to 499c326be7a5a50b) because logout's signature changed. --- changelog.d/3339.security.md | 11 +++++ messagefoundry/api/_ui_seam.py | 8 +++- messagefoundry/api/auth_routes.py | 4 +- messagefoundry/auth/service.py | 15 +++++-- messagefoundry_webconsole/__init__.py | 2 +- messagefoundry_webconsole/routes/core.py | 2 +- .../tests/test_ui_logout_affordance.py | 42 ++++++++++++++++--- tests/golden/webconsole_seam.snapshot | 4 +- tests/test_api_auth.py | 26 ++++++++++-- 9 files changed, 96 insertions(+), 18 deletions(-) create mode 100644 changelog.d/3339.security.md diff --git a/changelog.d/3339.security.md b/changelog.d/3339.security.md new file mode 100644 index 0000000000..d04fade8ff --- /dev/null +++ b/changelog.d/3339.security.md @@ -0,0 +1,11 @@ +- **A sign-out's `auth.logout` audit row records the client address it came from (ASVS 16.2.1).** + Neither sign-out route passed an address, so on a first deployment an investigator could not + have told from the audit log where a sign-out came from. `POST /auth/logout` and the console's + `/ui/logout` now pass the same `client_ip` address the other audited auth routes record, and + `AuthService.logout` takes it as a new optional `client` keyword. The row's actor is unchanged: the + signed-in caller on `POST /auth/logout`, and the owner of the session it ended on `/ui/logout`. A + console sign-out that ended nothing, including one that lost a rotation race, still names no + actor, because the presented token no longer names a session. A sign-out that revokes nothing + keeps its own recorded `detail.outcome`, `nothing_to_revoke`. The web console seam digest moved, + so a console and engine built on either side of this change refuse each other at mount. + (`BACKLOG #3339`) diff --git a/messagefoundry/api/_ui_seam.py b/messagefoundry/api/_ui_seam.py index 33e82a8eec..a8d9e1c31a 100644 --- a/messagefoundry/api/_ui_seam.py +++ b/messagefoundry/api/_ui_seam.py @@ -594,12 +594,18 @@ #: covered both, so the move comes from the merge. #: Re-derived with ``--write`` on the merged tree, so the value below covers every entry above. #: +#: BACKLOG #3339: ``AuthService.logout`` gained an optional keyword ``client``, the caller's +#: address, which ``/ui/logout`` now passes so the ``auth.logout`` row records it. The digest moved +#: from ``5d9c162790077dfc`` because the discovered signature changed. A newer console on an older +#: engine would fail its sign-out with a ``TypeError``; the pinned digest refuses that pair at mount. +#: Re-derived with ``--write``. +#: #: The digest below covers the surface DISCOVERED from the console's own imports and uses, which is #: strictly larger than the five hand-maintained tuples it replaced -- those had drifted, and the #: proof is that commit 40a4d5d9 added a REQUIRED ``UploadedFileList.scope`` field the console renders #: unconditionally while touching no seam file at all. Regenerate with #: ``python scripts/webconsole_seam_snapshot.py --write``; never hand-edit it to silence a gate. -ENGINE_UI_SEAM: str = "5d9c162790077dfc" +ENGINE_UI_SEAM: str = "499c326be7a5a50b" @dataclass(frozen=True, slots=True) diff --git a/messagefoundry/api/auth_routes.py b/messagefoundry/api/auth_routes.py index 17a195be8b..fcbbab719d 100644 --- a/messagefoundry/api/auth_routes.py +++ b/messagefoundry/api/auth_routes.py @@ -652,7 +652,9 @@ async def logout( service: AuthService = Depends(_service), identity: Identity = Depends(require()), ) -> SimpleMessage: - await service.logout(bearer_token(request), actor=identity.username) + await service.logout( + bearer_token(request), actor=identity.username, client=client_ip(request) + ) return SimpleMessage(detail="logged out") @app.get("/auth/me", response_model=CurrentUser) diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 807121b5e8..4a22b4734c 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -9920,7 +9920,9 @@ async def identity_for_user_id(self, user_id: str) -> Identity | None: return None return await self._build_identity(user) - async def logout(self, token: str | None, *, actor: str | None = None) -> LogoutOutcome: + async def logout( + self, token: str | None, *, actor: str | None = None, client: str | None = None + ) -> LogoutOutcome: """End the session ``token`` names, audit ``auth.logout`` saying whether one ended, and return that same outcome. @@ -9944,7 +9946,12 @@ async def logout(self, token: str | None, *, actor: str | None = None) -> Logout The row's actor is ``actor`` when given (``POST /auth/logout`` passes the signed-in caller). Otherwise it is the OWNER of the row the revoke ended, so a console sign-out, which has no auth dependency to name a caller, still lands in that user's ``/me/security-events`` - feed; with nothing ended there is no owner, and the actor stays NULL.""" + feed; with nothing ended there is no owner, and the actor stays NULL. That includes a + console sign-out that loses a rotation race: the rotated row no longer answers to the + presented hash, so nothing here can name its owner (BACKLOG #3339). + + ``client`` is the caller's address from ``client_ip``, which both sign-out routes pass, so + the row records where the sign-out came from (BACKLOG #3339, ADR 0150).""" if not token: return "nothing_to_revoke" ended = await self._store.supersede_session(hash_token(token), now=time.time()) @@ -9954,7 +9961,9 @@ async def logout(self, token: str | None, *, actor: str | None = None) -> Logout actor = owner.username if owner is not None else None # Emit the documented auth.logout event (SECURITY.md, ASVS 16.3.3) even when nothing # ended, so the trail still records the sign-out request. - await self._audit("auth.logout", actor=actor, detail=_json({"outcome": outcome})) + await self._audit( + "auth.logout", actor=actor, detail=_json({"outcome": outcome}), client=client + ) return outcome async def _supersede_session_hash(self, prior_hash: str, *, client: str | None) -> bool: diff --git a/messagefoundry_webconsole/__init__.py b/messagefoundry_webconsole/__init__.py index f44c268877..c730957c0b 100644 --- a/messagefoundry_webconsole/__init__.py +++ b/messagefoundry_webconsole/__init__.py @@ -44,7 +44,7 @@ # If cross-seam support is ever genuinely wanted, re-widen this set AND add the CI matrix that # installs the MIN and MAX supported engine builds — the claim and its test land together, or not # at all. -SUPPORTED_ENGINE_SEAMS: frozenset[str] = frozenset({"5d9c162790077dfc"}) +SUPPORTED_ENGINE_SEAMS: frozenset[str] = frozenset({"499c326be7a5a50b"}) #: The vendored static assets shipped in THIS wheel (mounted at /ui/static by :func:`mount_ui`). STATIC_DIR = Path(__file__).parent / "static" diff --git a/messagefoundry_webconsole/routes/core.py b/messagefoundry_webconsole/routes/core.py index 2535d79477..701e44d09a 100644 --- a/messagefoundry_webconsole/routes/core.py +++ b/messagefoundry_webconsole/routes/core.py @@ -735,7 +735,7 @@ async def ui_logout(request: Request) -> Response: auth = get_auth(request) token = session_token(request) if auth is not None and token: - await auth.logout(token) + await auth.logout(token, client=client_ip(request)) resp = RedirectResponse("/ui/login?e=loggedout", status_code=303) # ASVS 14.3.1: revoking server-side and deleting the cookie leaves the browser's CACHED # representations of the operator's PHI pages behind, which Back / bfcache can resurrect. diff --git a/packaging/messagefoundry-webconsole/tests/test_ui_logout_affordance.py b/packaging/messagefoundry-webconsole/tests/test_ui_logout_affordance.py index 7ed8e09189..d8caa4a94b 100644 --- a/packaging/messagefoundry-webconsole/tests/test_ui_logout_affordance.py +++ b/packaging/messagefoundry-webconsole/tests/test_ui_logout_affordance.py @@ -171,12 +171,25 @@ def test_every_nav_suppressing_page_builder_is_accounted_for() -> None: # --- end-to-end: the confined must-change session can actually sign out --------------------------- +#: A non-loopback peer, pinned so an assertion on the audited ``client`` compares a value this file +#: chose rather than a transport default. +PEER_ADDRESS = "198.51.100.23" + + async def _service(engine: Engine) -> AuthService: service = AuthService(engine.store, AuthSettings()) await service.initialize() return service +async def _logout_rows(engine: Engine) -> list[tuple[object, object, object]]: + """Each ``auth.logout`` row as its actor, client address and ``outcome``.""" + return [ + (r["actor"], r["client"], json.loads(str(r["detail"]))["outcome"]) + for r in await engine.store.list_audit(action="auth.logout") + ] + + async def test_must_change_session_can_see_and_use_sign_out(engine: Engine) -> None: """A must_change_password account is confined to /ui/account/password. The page now renders the Sign-out form, and POST /ui/logout — which has no Depends gate precisely so this works — really @@ -194,7 +207,9 @@ async def test_must_change_session_can_see_and_use_sign_out(engine: Engine) -> N roles=[Role.OPERATOR.value], actor="test", ) - transport = httpx.ASGITransport(app=create_app(engine, auth=service, serve_ui=True)) + transport = httpx.ASGITransport( + app=create_app(engine, auth=service, serve_ui=True), client=(PEER_ADDRESS, 51234) + ) async with httpx.AsyncClient(transport=transport, base_url="http://t") as c: login = await c.post("/ui/login", data={"username": "rotate", "password": PW}) assert login.status_code == 303 @@ -209,8 +224,23 @@ async def test_must_change_session_can_see_and_use_sign_out(engine: Engine) -> N # the session is really gone: the change-password page now bounces to login assert (await c.get("/ui/account/password")).status_code in (302, 303, 401) # BACKLOG #3029: the route passes no actor, so the auth.logout row names the owner of the - # session it ended rather than carrying a NULL actor. - rows = await engine.store.list_audit(action="auth.logout") - assert [(r["actor"], json.loads(str(r["detail"]))["outcome"]) for r in rows] == [ - ("rotate", "revoked") - ] + # session it ended rather than carrying a NULL actor. BACKLOG #3339: it also records the + # address the sign-out came from. + assert await _logout_rows(engine) == [("rotate", PEER_ADDRESS, "revoked")] + + +async def test_ui_logout_with_a_cookie_naming_no_session_still_records_the_address( + engine: Engine, +) -> None: + """BACKLOG #3339, the control. A cookie that names no session ends nothing, so the row has no + owner to name and says ``nothing_to_revoke``; the client address is still recorded. Read beside + the test above, it shows the actor there comes from the ended session, not from the request.""" + service = await _service(engine) + transport = httpx.ASGITransport( + app=create_app(engine, auth=service, serve_ui=True), client=(PEER_ADDRESS, 51234) + ) + async with httpx.AsyncClient(transport=transport, base_url="http://t") as c: + c.cookies.set("mf_session", "a-token-that-names-no-session") + out = await c.post("/ui/logout") + assert out.status_code == 303 + assert await _logout_rows(engine) == [(None, PEER_ADDRESS, "nothing_to_revoke")] diff --git a/tests/golden/webconsole_seam.snapshot b/tests/golden/webconsole_seam.snapshot index b48e407d19..ceff2cd2d3 100644 --- a/tests/golden/webconsole_seam.snapshot +++ b/tests/golden/webconsole_seam.snapshot @@ -7,7 +7,7 @@ # by hand (BACKLOG #1220) - so a newly rendered DTO is covered with nobody editing a list. ## ENGINE_UI_SEAM -5d9c162790077dfc +499c326be7a5a50b ## dataclass messagefoundry.api._ui_seam.UiDeps engine_seam @@ -186,7 +186,7 @@ identity_for_token: (self, token: 'str | None', *, activity: 'bool' = True) -> ' kerberos_available: property -> 'bool' list_sessions: (self, user_id: 'str') -> 'list[SessionRecord]' login: (self, username: 'str', password: 'str', *, provider: 'AuthProvider' = , client: 'str | None' = None, supersedes: 'str | None' = None, totp_code: 'str | None' = None) -> 'LoginOutcome' -logout: (self, token: 'str | None', *, actor: 'str | None' = None) -> 'LogoutOutcome' +logout: (self, token: 'str | None', *, actor: 'str | None' = None, client: 'str | None' = None) -> 'LogoutOutcome' mfa_satisfied: (self, token: 'str | None') -> 'bool' mfa_status: (self, identity: 'Identity') -> 'MfaStatus' must_enrol_before_rotating: (self, identity: 'Identity') -> 'bool' diff --git a/tests/test_api_auth.py b/tests/test_api_auth.py index 4310c48a49..42a976a6ac 100644 --- a/tests/test_api_auth.py +++ b/tests/test_api_auth.py @@ -658,11 +658,31 @@ async def test_a_logout_raced_by_a_rotation_does_not_audit_a_revoke( rotated = rotations[0] assert rotated is not None, "the rotation failed closed, so the race was not driven" assert await service.identity_for_token(rotated, activity=False) is not None - outcomes = [ - json.loads(str(row["detail"] or "{}")).get("outcome") + # BACKLOG #3339: a sign-out that ended nothing still names the caller and the address. + assert await _logout_rows(store) == [("u", _DEFAULT_PEER[0], "nothing_to_revoke")], ( + "the auth.logout row claimed a revoke that missed, or lost its actor or address" + ) + + +async def _logout_rows(store: Any) -> list[tuple[object, object, object]]: + """Each ``auth.logout`` row as its actor, client address and ``detail.outcome``.""" + return [ + (row["actor"], row["client"], json.loads(str(row["detail"] or "{}")).get("outcome")) for row in await store.list_audit(action="auth.logout") ] - assert outcomes == ["nothing_to_revoke"], "the auth.logout row claimed a revoke that missed" + + +async def test_a_logout_audit_row_names_the_caller_and_the_client_address(engine: Engine) -> None: + """BACKLOG #3339. ``POST /auth/logout`` passes the signed-in caller and the address from + ``client_ip``, and the ``auth.logout`` row records both. The peer is pinned, so the ``client`` + assertion compares a value this test named (BACKLOG #1644). The route's control, a token that + names no session, is the rotation race test above: its dependency refuses any other dead token.""" + service = await _service(engine) + await _add(service, "u", Role.VIEWER) + async with _client(engine, service, peer=("192.0.2.44", 40001)) as c: + token = (await _login(c, "u")).json()["token"] + assert (await c.post("/auth/logout", headers=_auth(token))).status_code == 200 + assert await _logout_rows(engine.store) == [("u", "192.0.2.44", "revoked")] async def test_cluster_endpoints_gated_by_monitoring_read(engine: Engine) -> None: From 69d2c608857e63c2d76e7cbc0b36c39b3c92aab5 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Mon, 19 Oct 2026 14:52:16 -0500 Subject: [PATCH 2/6] logout reads the owner before it revokes, and client is required, review round 1 (BACKLOG #3339) - With no actor, logout reads the session through _load_session before supersede_session and names that row's owner. A console sign-out that loses a rotation race now names the owner, as POST /auth/logout's own dependency read does (engine PR 2384 open finding 3). The owner comes from a row whose seal verified, not from the blanked row the revoke returns. A cookie re-keyed before the request still names no actor. - client is a required keyword, as on revoke_own_session, so a new sign-out caller cannot omit it silently. Test call sites pass it. - The route tests sign in and sign out from different addresses, so the row cannot pass by recording the session's login address. - New service tests: client reaches the row on both outcomes, and the race test, red on the prior logout (its injection never fires) and red when the owner is taken only from an ended row. - Changelog drops its completeness claim; the seam digest is re-derived (now 1f2d208818510f18). --- changelog.d/3339.security.md | 19 +++++----- messagefoundry/api/_ui_seam.py | 10 ++--- messagefoundry/auth/service.py | 31 ++++++++++------ messagefoundry_webconsole/__init__.py | 2 +- .../tests/test_ui_logout_affordance.py | 21 +++++++---- tests/golden/webconsole_seam.snapshot | 4 +- tests/test_api_auth.py | 9 +++-- tests/test_asvs_phase0.py | 2 +- tests/test_auth_before_body.py | 4 +- tests/test_oidc_step_up.py | 2 +- tests/test_session_supersession.py | 37 +++++++++++++++---- 11 files changed, 90 insertions(+), 51 deletions(-) diff --git a/changelog.d/3339.security.md b/changelog.d/3339.security.md index d04fade8ff..1ccb063f8f 100644 --- a/changelog.d/3339.security.md +++ b/changelog.d/3339.security.md @@ -1,11 +1,10 @@ - **A sign-out's `auth.logout` audit row records the client address it came from (ASVS 16.2.1).** - Neither sign-out route passed an address, so on a first deployment an investigator could not - have told from the audit log where a sign-out came from. `POST /auth/logout` and the console's - `/ui/logout` now pass the same `client_ip` address the other audited auth routes record, and - `AuthService.logout` takes it as a new optional `client` keyword. The row's actor is unchanged: the - signed-in caller on `POST /auth/logout`, and the owner of the session it ended on `/ui/logout`. A - console sign-out that ended nothing, including one that lost a rotation race, still names no - actor, because the presented token no longer names a session. A sign-out that revokes nothing - keeps its own recorded `detail.outcome`, `nothing_to_revoke`. The web console seam digest moved, - so a console and engine built on either side of this change refuse each other at mount. - (`BACKLOG #3339`) + Neither sign-out route passed an address. On a first deployment, an investigator could not have + told from the audit log where a sign-out came from. `POST /auth/logout` and the console's + `/ui/logout` now pass the `client_ip` address, the extractor other audited auth routes use. + `AuthService.logout` takes it as a required `client` keyword, so a new caller cannot omit it. + A console sign-out now reads the session's owner before it revokes. A rotation that re-keys the + session in between no longer leaves the row with no actor. A cookie that named no live session + at that read still gives a row with no actor. A sign-out that revokes nothing keeps its own + recorded `detail.outcome`, `nothing_to_revoke`. The web console seam digest moved, so a console + and an engine built on either side of this change refuse each other at mount. (`BACKLOG #3339`) diff --git a/messagefoundry/api/_ui_seam.py b/messagefoundry/api/_ui_seam.py index a8d9e1c31a..eef95e991c 100644 --- a/messagefoundry/api/_ui_seam.py +++ b/messagefoundry/api/_ui_seam.py @@ -594,18 +594,18 @@ #: covered both, so the move comes from the merge. #: Re-derived with ``--write`` on the merged tree, so the value below covers every entry above. #: -#: BACKLOG #3339: ``AuthService.logout`` gained an optional keyword ``client``, the caller's +#: BACKLOG #3339: ``AuthService.logout`` gained a required keyword ``client``, the caller's #: address, which ``/ui/logout`` now passes so the ``auth.logout`` row records it. The digest moved -#: from ``5d9c162790077dfc`` because the discovered signature changed. A newer console on an older -#: engine would fail its sign-out with a ``TypeError``; the pinned digest refuses that pair at mount. -#: Re-derived with ``--write``. +#: from ``5d9c162790077dfc`` because the discovered signature changed. A console on an engine from +#: the other side of this change would fail its sign-out with a ``TypeError``; the pinned digest +#: refuses that pair at mount. Re-derived with ``--write``. #: #: The digest below covers the surface DISCOVERED from the console's own imports and uses, which is #: strictly larger than the five hand-maintained tuples it replaced -- those had drifted, and the #: proof is that commit 40a4d5d9 added a REQUIRED ``UploadedFileList.scope`` field the console renders #: unconditionally while touching no seam file at all. Regenerate with #: ``python scripts/webconsole_seam_snapshot.py --write``; never hand-edit it to silence a gate. -ENGINE_UI_SEAM: str = "499c326be7a5a50b" +ENGINE_UI_SEAM: str = "1f2d208818510f18" @dataclass(frozen=True, slots=True) diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 4a22b4734c..40829e071e 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -9921,7 +9921,7 @@ async def identity_for_user_id(self, user_id: str) -> Identity | None: return await self._build_identity(user) async def logout( - self, token: str | None, *, actor: str | None = None, client: str | None = None + self, token: str | None, *, client: str | None, actor: str | None = None ) -> LogoutOutcome: """End the session ``token`` names, audit ``auth.logout`` saying whether one ended, and return that same outcome. @@ -9944,20 +9944,27 @@ async def logout( so this returns ``nothing_to_revoke`` and writes no row. The row's actor is ``actor`` when given (``POST /auth/logout`` passes the signed-in - caller). Otherwise it is the OWNER of the row the revoke ended, so a console sign-out, which - has no auth dependency to name a caller, still lands in that user's ``/me/security-events`` - feed; with nothing ended there is no owner, and the actor stays NULL. That includes a - console sign-out that loses a rotation race: the rotated row no longer answers to the - presented hash, so nothing here can name its owner (BACKLOG #3339). - - ``client`` is the caller's address from ``client_ip``, which both sign-out routes pass, so - the row records where the sign-out came from (BACKLOG #3339, ADR 0150).""" + caller). Otherwise it is the OWNER of the session the token named, so a console sign-out, + which has no auth dependency to name a caller, still lands in that user's + ``/me/security-events`` feed. The owner is read BEFORE the revoke, through + :meth:`_load_session`, for two reasons (BACKLOG #3339). A rotation that re-keys the session + between that read and the revoke leaves the revoke ending nothing, and the row still names + the owner, as ``POST /auth/logout``'s own dependency read does. And the owner comes from a + row whose seal verified, never from the row the revoke returns, whose seal is blank. A + token that named no live, sealed session at that read has no owner, and the actor stays + NULL. That includes a cookie whose session a rotation re-keyed before the request arrived. + + ``client`` is the caller's address, from ``client_ip`` on a request; ``None`` means no + request was in scope (ADR 0150). It has no default, so a new sign-out caller cannot omit it + silently (BACKLOG #3339).""" if not token: return "nothing_to_revoke" - ended = await self._store.supersede_session(hash_token(token), now=time.time()) + token_hash = hash_token(token) + held = await self._load_session(token_hash) if actor is None else None + ended = await self._store.supersede_session(token_hash, now=time.time()) outcome: LogoutOutcome = "nothing_to_revoke" if ended is None else "revoked" - if actor is None and ended is not None: - owner = await self._load_user(ended.user_id) + if held is not None: + owner = await self._load_user(held.user_id) actor = owner.username if owner is not None else None # Emit the documented auth.logout event (SECURITY.md, ASVS 16.3.3) even when nothing # ended, so the trail still records the sign-out request. diff --git a/messagefoundry_webconsole/__init__.py b/messagefoundry_webconsole/__init__.py index c730957c0b..21d34240ee 100644 --- a/messagefoundry_webconsole/__init__.py +++ b/messagefoundry_webconsole/__init__.py @@ -44,7 +44,7 @@ # If cross-seam support is ever genuinely wanted, re-widen this set AND add the CI matrix that # installs the MIN and MAX supported engine builds — the claim and its test land together, or not # at all. -SUPPORTED_ENGINE_SEAMS: frozenset[str] = frozenset({"499c326be7a5a50b"}) +SUPPORTED_ENGINE_SEAMS: frozenset[str] = frozenset({"1f2d208818510f18"}) #: The vendored static assets shipped in THIS wheel (mounted at /ui/static by :func:`mount_ui`). STATIC_DIR = Path(__file__).parent / "static" diff --git a/packaging/messagefoundry-webconsole/tests/test_ui_logout_affordance.py b/packaging/messagefoundry-webconsole/tests/test_ui_logout_affordance.py index d8caa4a94b..0a739ad48c 100644 --- a/packaging/messagefoundry-webconsole/tests/test_ui_logout_affordance.py +++ b/packaging/messagefoundry-webconsole/tests/test_ui_logout_affordance.py @@ -171,8 +171,10 @@ def test_every_nav_suppressing_page_builder_is_accounted_for() -> None: # --- end-to-end: the confined must-change session can actually sign out --------------------------- -#: A non-loopback peer, pinned so an assertion on the audited ``client`` compares a value this file -#: chose rather than a transport default. +#: Non-loopback peers, pinned so an assertion on the audited ``client`` compares a value this file +#: chose rather than a transport default. The sign-in comes from a different address than the +#: sign-out, so the row cannot pass by recording the address the session was minted from. +LOGIN_PEER = ("198.51.100.10", 40000) PEER_ADDRESS = "198.51.100.23" @@ -185,7 +187,7 @@ async def _service(engine: Engine) -> AuthService: async def _logout_rows(engine: Engine) -> list[tuple[object, object, object]]: """Each ``auth.logout`` row as its actor, client address and ``outcome``.""" return [ - (r["actor"], r["client"], json.loads(str(r["detail"]))["outcome"]) + (r["actor"], r["client"], json.loads(str(r["detail"] or "{}")).get("outcome")) for r in await engine.store.list_audit(action="auth.logout") ] @@ -207,9 +209,8 @@ async def test_must_change_session_can_see_and_use_sign_out(engine: Engine) -> N roles=[Role.OPERATOR.value], actor="test", ) - transport = httpx.ASGITransport( - app=create_app(engine, auth=service, serve_ui=True), client=(PEER_ADDRESS, 51234) - ) + app = create_app(engine, auth=service, serve_ui=True) + transport = httpx.ASGITransport(app=app, client=LOGIN_PEER) async with httpx.AsyncClient(transport=transport, base_url="http://t") as c: login = await c.post("/ui/login", data={"username": "rotate", "password": PW}) assert login.status_code == 303 @@ -219,7 +220,11 @@ async def test_must_change_session_can_see_and_use_sign_out(engine: Engine) -> N confined = await c.get("/ui/account/password") assert confined.status_code == 200 assert LOGOUT_FORM in confined.text - out = await c.post("/ui/logout") + # BACKLOG #3339: the same session signs out from another address. + elsewhere = httpx.ASGITransport(app=app, client=(PEER_ADDRESS, 51234)) + async with httpx.AsyncClient(transport=elsewhere, base_url="http://t") as away: + away.cookies.set("mf_session", str(c.cookies.get("mf_session"))) + out = await away.post("/ui/logout") assert out.status_code == 303 # the session is really gone: the change-password page now bounces to login assert (await c.get("/ui/account/password")).status_code in (302, 303, 401) @@ -234,7 +239,7 @@ async def test_ui_logout_with_a_cookie_naming_no_session_still_records_the_addre ) -> None: """BACKLOG #3339, the control. A cookie that names no session ends nothing, so the row has no owner to name and says ``nothing_to_revoke``; the client address is still recorded. Read beside - the test above, it shows the actor there comes from the ended session, not from the request.""" + the test above, it shows the actor there comes from the session, not from the request.""" service = await _service(engine) transport = httpx.ASGITransport( app=create_app(engine, auth=service, serve_ui=True), client=(PEER_ADDRESS, 51234) diff --git a/tests/golden/webconsole_seam.snapshot b/tests/golden/webconsole_seam.snapshot index ceff2cd2d3..d6188acbe4 100644 --- a/tests/golden/webconsole_seam.snapshot +++ b/tests/golden/webconsole_seam.snapshot @@ -7,7 +7,7 @@ # by hand (BACKLOG #1220) - so a newly rendered DTO is covered with nobody editing a list. ## ENGINE_UI_SEAM -499c326be7a5a50b +1f2d208818510f18 ## dataclass messagefoundry.api._ui_seam.UiDeps engine_seam @@ -186,7 +186,7 @@ identity_for_token: (self, token: 'str | None', *, activity: 'bool' = True) -> ' kerberos_available: property -> 'bool' list_sessions: (self, user_id: 'str') -> 'list[SessionRecord]' login: (self, username: 'str', password: 'str', *, provider: 'AuthProvider' = , client: 'str | None' = None, supersedes: 'str | None' = None, totp_code: 'str | None' = None) -> 'LoginOutcome' -logout: (self, token: 'str | None', *, actor: 'str | None' = None, client: 'str | None' = None) -> 'LogoutOutcome' +logout: (self, token: 'str | None', *, client: 'str | None', actor: 'str | None' = None) -> 'LogoutOutcome' mfa_satisfied: (self, token: 'str | None') -> 'bool' mfa_status: (self, identity: 'Identity') -> 'MfaStatus' must_enrol_before_rotating: (self, identity: 'Identity') -> 'bool' diff --git a/tests/test_api_auth.py b/tests/test_api_auth.py index 42a976a6ac..308dfb3a92 100644 --- a/tests/test_api_auth.py +++ b/tests/test_api_auth.py @@ -675,12 +675,15 @@ async def _logout_rows(store: Any) -> list[tuple[object, object, object]]: async def test_a_logout_audit_row_names_the_caller_and_the_client_address(engine: Engine) -> None: """BACKLOG #3339. ``POST /auth/logout`` passes the signed-in caller and the address from ``client_ip``, and the ``auth.logout`` row records both. The peer is pinned, so the ``client`` - assertion compares a value this test named (BACKLOG #1644). The route's control, a token that - names no session, is the rotation race test above: its dependency refuses any other dead token.""" + assertion compares a value this test named (BACKLOG #1644). The sign-in comes from another + address, so the row cannot pass by recording the address the session was minted from. The + route's control, a token that names no session, is the rotation race test above: its dependency + refuses any other dead token.""" service = await _service(engine) await _add(service, "u", Role.VIEWER) - async with _client(engine, service, peer=("192.0.2.44", 40001)) as c: + async with _client(engine, service, peer=("192.0.2.10", 40000)) as c: token = (await _login(c, "u")).json()["token"] + async with _client(engine, service, peer=("192.0.2.44", 40001)) as c: assert (await c.post("/auth/logout", headers=_auth(token))).status_code == 200 assert await _logout_rows(engine.store) == [("u", "192.0.2.44", "revoked")] diff --git a/tests/test_asvs_phase0.py b/tests/test_asvs_phase0.py index 495392ac76..a6a5a831bf 100644 --- a/tests/test_asvs_phase0.py +++ b/tests/test_asvs_phase0.py @@ -158,7 +158,7 @@ async def test_logout_emits_audit_event() -> None: try: service = AuthService(store, AuthSettings()) identity, token, _ = await login_admin(service) - await service.logout(token, actor=identity.username) + await service.logout(token, actor=identity.username, client=None) actions = [row["action"] for row in await store.list_audit()] assert "auth.logout" in actions finally: diff --git a/tests/test_auth_before_body.py b/tests/test_auth_before_body.py index 9ba9b4b90b..ae523f7d20 100644 --- a/tests/test_auth_before_body.py +++ b/tests/test_auth_before_body.py @@ -879,7 +879,9 @@ async def test_a_session_revoked_while_the_body_arrives_is_refused_by_the_gate( # The early step passes, the session ends, and then the gate runs. request = _request(app, bearer) await found.step(request) - await service.logout(bearer["Authorization"].removeprefix("Bearer "), actor="admin") + await service.logout( + bearer["Authorization"].removeprefix("Bearer "), actor="admin", client=None + ) for gate in gates: with pytest.raises(HTTPException) as refused: await gate(request) diff --git a/tests/test_oidc_step_up.py b/tests/test_oidc_step_up.py index 03b8975d86..bace84d970 100644 --- a/tests/test_oidc_step_up.py +++ b/tests/test_oidc_step_up.py @@ -992,7 +992,7 @@ async def test_a_session_revoked_mid_flight_is_not_elevated( service = await _service(store, rsa_key) token = await _oidc_session(service, monkeypatch, rsa_key) flow_id, _url = await _begin(service, token) - await service.logout(token) + await service.logout(token, client=None) out = await _return_from_idp(service, monkeypatch, rsa_key, flow_id) diff --git a/tests/test_session_supersession.py b/tests/test_session_supersession.py index 6b8b01b43f..18427eb3fc 100644 --- a/tests/test_session_supersession.py +++ b/tests/test_session_supersession.py @@ -302,8 +302,8 @@ async def test_a_logout_records_whether_it_ended_a_session(store: MessageStore) # The return value is the same outcome the row records, in both cases. service = await _service(store) token = await _token(service) - first = await service.logout(token, actor="op") - replay = await service.logout(token, actor="op") + first = await service.logout(token, actor="op", client=None) + replay = await service.logout(token, actor="op", client=None) assert not await _live(service, token) assert [first, replay] == ["revoked", "nothing_to_revoke"] assert await _logouts(store) == [ @@ -329,21 +329,44 @@ async def test_a_logout_with_no_actor_names_the_owner_of_the_session_it_ended( # matching a default of "op". service = await _service(store) token = await _token(service, "other") - first = await service.logout(token) - replay = await service.logout(token) + first = await service.logout(token, client="203.0.113.5") + replay = await service.logout(token, client="203.0.113.6") assert [first, replay] == ["revoked", "nothing_to_revoke"] assert await _logouts(store) == [ {"outcome": "nothing_to_revoke", "actor": None}, {"outcome": "revoked", "actor": "other"}, ] + # BACKLOG #3339: each row records the address that call passed, on both branches. + rows = await store.list_audit(action="auth.logout") + assert [row["client"] for row in rows] == ["203.0.113.6", "203.0.113.5"] + + +async def test_a_logout_with_no_actor_that_loses_a_rotation_race_still_names_the_owner( + store: MessageStore, monkeypatch: pytest.MonkeyPatch +) -> None: + # BACKLOG #3339 (engine PR 2384, open finding 3). /ui/logout passes no actor, so the owner used + # to come only from the row the revoke ended. A rotation that re-keys the session first leaves + # the revoke ending nothing, and the row named nobody. logout now reads the owner before the + # revoke, so the rotation is injected right after that read. On the prior code nothing calls + # get_session, so the injection never fires and the first assertion goes red. + service = await _service(store) + token = await _token(service, "other") + rotations = rotate_after_first_call(monkeypatch, store, service, token, names=("get_session",)) + outcome = await service.logout(token, client=None) + + assert rotations, "the injection never fired, so this measured nothing" + rotated = rotations[0] + assert rotated is not None and await _live(service, rotated), "the race was not driven" + assert outcome == "nothing_to_revoke" + assert await _logouts(store) == [{"outcome": "nothing_to_revoke", "actor": "other"}] async def test_a_logout_with_no_token_returns_nothing_to_revoke_and_writes_no_row( store: MessageStore, ) -> None: service = await _service(store) - assert await service.logout(None) == "nothing_to_revoke" - assert await service.logout("") == "nothing_to_revoke" + assert await service.logout(None, client=None) == "nothing_to_revoke" + assert await service.logout("", client=None) == "nothing_to_revoke" assert await _logouts(store) == [] @@ -410,7 +433,7 @@ async def test_the_no_op_inputs_revoke_nothing_and_write_no_row( ) -> None: service = await _service(store) dead = await _token(service) - await service.logout(dead) + await service.logout(dead, client=None) prior: str | None = { "none": None, "empty": "", From 8662c557344f031cfe4427ca765b38fd8d9294d5 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Mon, 19 Oct 2026 15:06:36 -0500 Subject: [PATCH 3/6] Revert logout's owner pre-read, review round 2 (BACKLOG #3339) The round 1 pre-read went through _load_session, the one sanctioned session reader. It ends and reports a row whose seal fails, so the revoke then found nothing and the row said nothing_to_revoke with no actor for a session this call ended. It also put a failure point ahead of the revoke. A side-effect-free read would need a second get_session reader, which tests/test_session_seal.py forbids. logout attributes the owner of the row the revoke ended again, as PR 2384 left it. A console sign-out that loses a rotation race keeps a NULL actor; the docstring and changelog say so, and why. The POST /auth/logout race test now signs in from another address, so its client assertion is the sign-out's own address. --- changelog.d/3339.security.md | 9 ++++----- messagefoundry/auth/service.py | 24 ++++++++++-------------- tests/test_api_auth.py | 4 +++- tests/test_session_supersession.py | 20 -------------------- 4 files changed, 17 insertions(+), 40 deletions(-) diff --git a/changelog.d/3339.security.md b/changelog.d/3339.security.md index 1ccb063f8f..6ae68bca77 100644 --- a/changelog.d/3339.security.md +++ b/changelog.d/3339.security.md @@ -3,8 +3,7 @@ told from the audit log where a sign-out came from. `POST /auth/logout` and the console's `/ui/logout` now pass the `client_ip` address, the extractor other audited auth routes use. `AuthService.logout` takes it as a required `client` keyword, so a new caller cannot omit it. - A console sign-out now reads the session's owner before it revokes. A rotation that re-keys the - session in between no longer leaves the row with no actor. A cookie that named no live session - at that read still gives a row with no actor. A sign-out that revokes nothing keeps its own - recorded `detail.outcome`, `nothing_to_revoke`. The web console seam digest moved, so a console - and an engine built on either side of this change refuse each other at mount. (`BACKLOG #3339`) + The row's actor is unchanged. A console sign-out that ends nothing, including one that loses a + rotation race, still records no actor. A sign-out that revokes nothing keeps its own recorded + `detail.outcome`, `nothing_to_revoke`. The web console seam digest moved, so a console and an + engine built on either side of this change refuse each other at mount. (`BACKLOG #3339`) diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 40829e071e..51f95682ad 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -9944,27 +9944,23 @@ async def logout( so this returns ``nothing_to_revoke`` and writes no row. The row's actor is ``actor`` when given (``POST /auth/logout`` passes the signed-in - caller). Otherwise it is the OWNER of the session the token named, so a console sign-out, - which has no auth dependency to name a caller, still lands in that user's - ``/me/security-events`` feed. The owner is read BEFORE the revoke, through - :meth:`_load_session`, for two reasons (BACKLOG #3339). A rotation that re-keys the session - between that read and the revoke leaves the revoke ending nothing, and the row still names - the owner, as ``POST /auth/logout``'s own dependency read does. And the owner comes from a - row whose seal verified, never from the row the revoke returns, whose seal is blank. A - token that named no live, sealed session at that read has no owner, and the actor stays - NULL. That includes a cookie whose session a rotation re-keyed before the request arrived. + caller). Otherwise it is the OWNER of the row the revoke ended, so a console sign-out, which + has no auth dependency to name a caller, still lands in that user's ``/me/security-events`` + feed; with nothing ended there is no owner, and the actor stays NULL. That includes a + console sign-out that loses a rotation race (BACKLOG #3339, still open for that case). A + read before the revoke would name the owner only through :meth:`_load_session`, the one + sanctioned session reader, and that read ends and reports a row whose seal fails, so the + revoke would then record ``nothing_to_revoke`` for a session this call ended. ``client`` is the caller's address, from ``client_ip`` on a request; ``None`` means no request was in scope (ADR 0150). It has no default, so a new sign-out caller cannot omit it silently (BACKLOG #3339).""" if not token: return "nothing_to_revoke" - token_hash = hash_token(token) - held = await self._load_session(token_hash) if actor is None else None - ended = await self._store.supersede_session(token_hash, now=time.time()) + ended = await self._store.supersede_session(hash_token(token), now=time.time()) outcome: LogoutOutcome = "nothing_to_revoke" if ended is None else "revoked" - if held is not None: - owner = await self._load_user(held.user_id) + if actor is None and ended is not None: + owner = await self._load_user(ended.user_id) actor = owner.username if owner is not None else None # Emit the documented auth.logout event (SECURITY.md, ASVS 16.3.3) even when nothing # ended, so the trail still records the sign-out request. diff --git a/tests/test_api_auth.py b/tests/test_api_auth.py index 308dfb3a92..19a6ba8d52 100644 --- a/tests/test_api_auth.py +++ b/tests/test_api_auth.py @@ -647,8 +647,10 @@ async def test_a_logout_raced_by_a_rotation_does_not_audit_a_revoke( service = await _service(engine) await _add(service, "u", Role.VIEWER) store = engine.store - async with _client(engine, service) as c: + # Signed in from another address, so the row's client is the sign-out's own (BACKLOG #3339). + async with _client(engine, service, peer=("192.0.2.10", 40000)) as c: token = (await _login(c, "u")).json()["token"] + async with _client(engine, service) as c: rotations = rotate_after_first_call( monkeypatch, store, service, token, names=("get_session",) ) diff --git a/tests/test_session_supersession.py b/tests/test_session_supersession.py index 18427eb3fc..d1aedc1a23 100644 --- a/tests/test_session_supersession.py +++ b/tests/test_session_supersession.py @@ -341,26 +341,6 @@ async def test_a_logout_with_no_actor_names_the_owner_of_the_session_it_ended( assert [row["client"] for row in rows] == ["203.0.113.6", "203.0.113.5"] -async def test_a_logout_with_no_actor_that_loses_a_rotation_race_still_names_the_owner( - store: MessageStore, monkeypatch: pytest.MonkeyPatch -) -> None: - # BACKLOG #3339 (engine PR 2384, open finding 3). /ui/logout passes no actor, so the owner used - # to come only from the row the revoke ended. A rotation that re-keys the session first leaves - # the revoke ending nothing, and the row named nobody. logout now reads the owner before the - # revoke, so the rotation is injected right after that read. On the prior code nothing calls - # get_session, so the injection never fires and the first assertion goes red. - service = await _service(store) - token = await _token(service, "other") - rotations = rotate_after_first_call(monkeypatch, store, service, token, names=("get_session",)) - outcome = await service.logout(token, client=None) - - assert rotations, "the injection never fired, so this measured nothing" - rotated = rotations[0] - assert rotated is not None and await _live(service, rotated), "the race was not driven" - assert outcome == "nothing_to_revoke" - assert await _logouts(store) == [{"outcome": "nothing_to_revoke", "actor": "other"}] - - async def test_a_logout_with_no_token_returns_nothing_to_revoke_and_writes_no_row( store: MessageStore, ) -> None: From 6d3f81646fad5c6377ba1953bae91479cad5109e Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Mon, 19 Oct 2026 15:10:51 -0500 Subject: [PATCH 4/6] The no-local-Administrator WARNING names every account (BACKLOG #3282) The start WARNING from report_administrators_needing_an_outside_service put the usernames right after "no usable password:". The shipped CredentialScrubFilter reads "password: " as a credential and masked the first name. The list now follows "Accounts that need one:", ahead of the word "password". logging_setup.py and secretscrub.py are unchanged. Test: every name survives the filter chain every engine handler carries (filtered_sink, the real _install_phi_filters), not caplog. A control sends the old wording through the same chain and loses the first name. The test is red on the prior wording. --- changelog.d/3282.fixed.md | 5 +++ messagefoundry/auth/service.py | 10 ++++-- tests/test_local_administrator_notice.py | 40 ++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 3 deletions(-) create mode 100644 changelog.d/3282.fixed.md diff --git a/changelog.d/3282.fixed.md b/changelog.d/3282.fixed.md new file mode 100644 index 0000000000..ac4cc94b95 --- /dev/null +++ b/changelog.d/3282.fixed.md @@ -0,0 +1,5 @@ +- **The start WARNING about no usable local Administrator names every account again.** It listed the + accounts right after "no usable password:", and the log's credential scrub read that as a password + and masked the first name. The notice exists to name those accounts. The WARNING now gives the list + after "Accounts that need one:", ahead of the word "password", so every name reaches the log. + (`BACKLOG #3282`) diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index 51f95682ad..b94c40c711 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -11579,11 +11579,15 @@ async def report_administrators_needing_an_outside_service(self) -> tuple[str, . names = await self.administrators_needing_an_outside_service() if not names: return names + # The names come right after a neutral label and before the word "password". The shipped + # CredentialScrubFilter reads "password: " as a credential, so the old wording, with + # the list after "no usable password:", masked the first name (BACKLOG #3282). _log.warning( "no enabled Administrator can sign in without an outside identity service (the " - "directory or a federated identity provider); each is a directory account or a local " - "one with no usable password: %s. If that service is down, no Administrator can sign " - "in to manage this engine. Create a local Administrator now, before an outage, from " + "directory or a federated identity provider). Accounts that need one: %s. Each is a " + "directory account, or a local account with no usable password. If that service is " + "down, no Administrator can sign in to manage this engine. Create a local " + "Administrator now, before an outage, from " "the web console. During an outage, `messagefoundry provision-admin`, run at the host " "with the engine stopped, is not blocked by these accounts " "(docs/SECURITY.md, 'Keep a local Administrator').", diff --git a/tests/test_local_administrator_notice.py b/tests/test_local_administrator_notice.py index d8d7774465..e86fc51a53 100644 --- a/tests/test_local_administrator_notice.py +++ b/tests/test_local_administrator_notice.py @@ -12,6 +12,8 @@ from __future__ import annotations +import logging + import pytest from messagefoundry.auth.identity import AuthProvider @@ -20,6 +22,7 @@ from messagefoundry.config.settings import AuthSettings from messagefoundry.store.store import MessageStore from tests._admin_account import ADMIN_USERNAME, create_admin +from tests._phi_log_capture import filtered_sink _ACTION = "auth.no_local_administrator" @@ -141,3 +144,40 @@ async def broken() -> tuple[str, ...]: ) finally: await store.close() + + +#: The wording before BACKLOG #3282, kept only as the control below: the list followed a credential +#: label, so the shipped credential scrub masked the first name. +_OLD_WORDING = ( + "no enabled Administrator can sign in without an outside identity service (the directory or a " + "federated identity provider); each is a directory account or a local one with no usable " + "password: %s. If that service is down, no Administrator can sign in to manage this engine." +) + + +_LOGGER = "messagefoundry.auth.service" + + +async def test_every_named_administrator_survives_the_shipped_log_filters() -> None: + """BACKLOG #3282. The notice exists to name the accounts, so each name must reach the log + through the filter chain the engine installs. The control sends the old wording through the + same chain and loses the first name, so this test can tell the two wordings apart.""" + names = ("dir-a", "dir-b", "ops.lead") + store = await MessageStore.open(":memory:") + try: + service = AuthService(store, AuthSettings()) + for name in names: + await _directory_admin(service, name) + # filtered_sink carries the filters every engine handler carries. caplog sees records + # before any handler filter, so it cannot measure this. + with filtered_sink(_LOGGER) as written: + assert await service.report_administrators_needing_an_outside_service() == names + finally: + await store.close() + assert "outside identity service" in written.text, "the notice did not reach the handler" + assert "dir-a, dir-b, ops.lead" in written.text, written.text + + with filtered_sink(_LOGGER) as control: + logging.getLogger(_LOGGER).warning(_OLD_WORDING, ", ".join(names)) + assert "dir-b, ops.lead" in control.text, "the control wording did not reach the handler" + assert "dir-a" not in control.text, "the old wording keeps every name: this measures nothing" From 3411694c05230a92cb56963aeef41455b352518c Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Mon, 19 Oct 2026 15:25:58 -0500 Subject: [PATCH 5/6] Console changelog and logout docstring premises, branch review (BACKLOG #3339) - packaging/messagefoundry-webconsole/CHANGELOG.md records the console change and the seam move, as changelog.d/README.md requires. - The logout docstring no longer says a NULL client means no request: client_ip can find no address on a real request. The race residual cites engine PR 2384 open finding 3 rather than this item. - The /ui/logout control test says it pins still-open behaviour (PR 2384 open finding 6, held by vault BACKLOG #3749). --- messagefoundry/auth/service.py | 9 +++++---- packaging/messagefoundry-webconsole/CHANGELOG.md | 4 ++++ .../tests/test_ui_logout_affordance.py | 6 +++++- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/messagefoundry/auth/service.py b/messagefoundry/auth/service.py index b94c40c711..d820acecdd 100644 --- a/messagefoundry/auth/service.py +++ b/messagefoundry/auth/service.py @@ -9947,14 +9947,15 @@ async def logout( caller). Otherwise it is the OWNER of the row the revoke ended, so a console sign-out, which has no auth dependency to name a caller, still lands in that user's ``/me/security-events`` feed; with nothing ended there is no owner, and the actor stays NULL. That includes a - console sign-out that loses a rotation race (BACKLOG #3339, still open for that case). A + console sign-out that loses a rotation race (engine PR 2384, open finding 3). A read before the revoke would name the owner only through :meth:`_load_session`, the one sanctioned session reader, and that read ends and reports a row whose seal fails, so the revoke would then record ``nothing_to_revoke`` for a session this call ended. - ``client`` is the caller's address, from ``client_ip`` on a request; ``None`` means no - request was in scope (ADR 0150). It has no default, so a new sign-out caller cannot omit it - silently (BACKLOG #3339).""" + ``client`` is the caller's address, from ``client_ip`` on a request (ADR 0150). It is + ``None`` where ``client_ip`` finds no address, so on this row a NULL client does not by + itself mean no request was in scope. It has no default, so a new sign-out caller cannot + omit it silently (BACKLOG #3339).""" if not token: return "nothing_to_revoke" ended = await self._store.supersede_session(hash_token(token), now=time.time()) diff --git a/packaging/messagefoundry-webconsole/CHANGELOG.md b/packaging/messagefoundry-webconsole/CHANGELOG.md index a60af3dca0..be9ed009bd 100644 --- a/packaging/messagefoundry-webconsole/CHANGELOG.md +++ b/packaging/messagefoundry-webconsole/CHANGELOG.md @@ -85,6 +85,10 @@ this line.** - **Seam only: `/status` names the FHIR lookups that failed to start.** `EngineInfo` gains `fhir_lookups_failed` (vault `BACKLOG #3008`). The console does not show it yet, but the field is on the seam it pins, so the supported digest moved. +- **Sign-out records where it came from.** `/ui/logout` passes the caller's `client_ip` address to + `AuthService.logout`, so the `auth.logout` audit row records it. Seam change: `logout` takes a + required `client` keyword, so the supported digest moved. A console from either side of this + change refuses an engine from the other at mount. (`BACKLOG #3339`) - **Seam only: the posture reports the deny-by-default refusal count.** `SecurityPosture` gains `undeclared_route_refusals` (vault `BACKLOG #2954`). The console does not show it yet, but the field is on the seam it pins, so the supported digest moved. diff --git a/packaging/messagefoundry-webconsole/tests/test_ui_logout_affordance.py b/packaging/messagefoundry-webconsole/tests/test_ui_logout_affordance.py index 0a739ad48c..016e6803a4 100644 --- a/packaging/messagefoundry-webconsole/tests/test_ui_logout_affordance.py +++ b/packaging/messagefoundry-webconsole/tests/test_ui_logout_affordance.py @@ -239,7 +239,11 @@ async def test_ui_logout_with_a_cookie_naming_no_session_still_records_the_addre ) -> None: """BACKLOG #3339, the control. A cookie that names no session ends nothing, so the row has no owner to name and says ``nothing_to_revoke``; the client address is still recorded. Read beside - the test above, it shows the actor there comes from the session, not from the request.""" + the test above, it shows the actor there comes from the session, not from the request. + + It pins the CURRENT behaviour, which is still open: an unauthenticated sign-out can append a + row for a cookie naming no session (engine PR 2384, open finding 6, held by vault BACKLOG + #3749). A fix that writes no row here, or bounds the rows, is expected to change this test.""" service = await _service(engine) transport = httpx.ASGITransport( app=create_app(engine, auth=service, serve_ui=True), client=(PEER_ADDRESS, 51234) From b7b1d8417fbb0ad6c178cbd79359a827dcc391a8 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Mon, 19 Oct 2026 15:26:17 -0500 Subject: [PATCH 6/6] Changelog for the WARNING rewording states its limits, branch review (BACKLOG #3282) The WARNING lost its first name from the day it was added, so this is not a regression undone. A name that looks like a credential label or a date can still be rewritten by the log filters; the audit row holds the complete list. Proposed PR title: Sign-out audit rows record the client address, and the no-local-Administrator WARNING keeps every name (BACKLOG #3339, #3282) Proposed banner #3339: BUILT, pending merge. POST /auth/logout and /ui/logout pass client_ip, and AuthService.logout records it on the auth.logout row through a required client keyword. Tests read the row back for each route (actor, client, outcome), signing in and out from different addresses; a control signs out with a cookie naming no session. Closing step 2 was already met by engine PR 2384: a sign-out that revokes nothing records detail.outcome nothing_to_revoke on the same action. Open: a console sign-out that loses a rotation race keeps a NULL actor (PR 2384 finding 3); closing step 4, the ASVS 16.2.1 re-read, is vault record work. Web console seam digest moved to 1f2d208818510f18. Proposed banner #3282: BUILT, pending merge. The start WARNING lists the accounts after "Accounts that need one:", ahead of the word "password", so the shipped CredentialScrubFilter no longer masks the first name. A test sends the real WARNING through the shipped handler filter chain and finds every name; a control shows the old wording loses one. logging_setup.py and secretscrub.py are unchanged. --- changelog.d/3282.fixed.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/changelog.d/3282.fixed.md b/changelog.d/3282.fixed.md index ac4cc94b95..eda62f69c3 100644 --- a/changelog.d/3282.fixed.md +++ b/changelog.d/3282.fixed.md @@ -1,5 +1,6 @@ -- **The start WARNING about no usable local Administrator names every account again.** It listed the - accounts right after "no usable password:", and the log's credential scrub read that as a password - and masked the first name. The notice exists to name those accounts. The WARNING now gives the list - after "Accounts that need one:", ahead of the word "password", so every name reaches the log. - (`BACKLOG #3282`) +- **The start WARNING about no usable local Administrator no longer loses its first name to the + credential scrub.** It listed the accounts right after "no usable password:". The log's credential + scrub read that as a password and masked the first name, from the day the WARNING was added. The + list now follows "Accounts that need one:", ahead of the word "password". A name that itself looks + like a credential label or a date can still be rewritten by the log filters. The + `auth.no_local_administrator` audit row holds the complete list. (`BACKLOG #3282`)