From 355ff5807bc8fbadff3161dec7179fdfa4448fb4 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Tue, 11 Aug 2026 23:24:42 +0300 Subject: [PATCH 1/6] Closes #63: stamp Bridge's session onto register/attach/assign/deregister envelopes Add IBackend::setSession(session::Context), mirroring the setReconnectHandler/setConnectHandler/setDisconnectHandler no-op-default pattern. Bridge pushes the current default session to the active backend from its constructor, from setDefaultSession(), and to the incoming backend in switchBackend() (before phase 1's re-registration loop runs, so re-registered handlers' control envelopes carry it too). SimulatedRemoteBackend, SocketBackend, and QtWebSocketBackend override setSession to store the session and stamp it onto every register, registerShared, attach, assign, and deregister envelope they build. LocalBackend needs no change -- the local path never serialises a Context onto a wire envelope. Previously these control envelopes always carried a default-constructed, unauthenticated session::Context regardless of Bridge::setDefaultSession(), so RemoteServer::authorizeRegister could never see a caller's identity and the owner principal recorded at register time was always empty -- degrading authorizeInstance's ownership check to allow-all for every Bridge-registered instance. Updated docs/spec/core/backend.md, docs/spec/core/bridge.md, and docs/spec/session/session.md to document the new hook and its call sites. Note: SocketBackend's fix (include/morph/net/socket_backend.hpp) could not be compile-tested on this Windows machine -- morph::net is POSIX-only and MORPH_BUILD_NET is ignored on Windows -- but mirrors the verified SimulatedRemoteBackend fix exactly. Same for QtWebSocketBackend, fixed for consistency though not explicitly required by the issue's hint. Co-Authored-By: Claude Sonnet 5 --- docs/spec/core/backend.md | 51 +++++++- docs/spec/core/bridge.md | 24 +++- docs/spec/session/session.md | 23 +++- include/morph/core/backend.hpp | 26 ++++ include/morph/core/bridge.hpp | 31 ++++- include/morph/core/remote.hpp | 47 +++++-- include/morph/net/socket_backend.hpp | 45 +++++-- include/morph/qt/qt_websocket_backend.hpp | 7 + src/qt/qt_websocket_backend.cpp | 28 ++-- tests/CMakeLists.txt | 1 + tests/test_register_session.cpp | 150 ++++++++++++++++++++++ 11 files changed, 390 insertions(+), 43 deletions(-) create mode 100644 tests/test_register_session.cpp diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 93b94a2d..94174556 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -79,6 +79,7 @@ holds a `unique_ptr` and delegates all model operations to it. | `setReconnectHandler(handler)` | Installs a callback invoked when the backend reconnects to its peer. Fires only on the *second and later* connects, never the first — used by `Bridge` to re-register handlers after a drop. Used by backends with transport (e.g. `QtWebSocketBackend`). Default implementation is a no-op. | | `setConnectHandler(handler)` | Installs a callback invoked on every successful connect, including the first — the complementary hook `setReconnectHandler` deliberately skips (see [Connect/disconnect notifications](#connectdisconnect-notifications)). Default implementation is a no-op. | | `setDisconnectHandler(handler)` | Installs a callback invoked whenever the transport drops, before any reconnect is scheduled. Default implementation is a no-op. | +| `setSession(session)` | Installs the `session::Context` stamped onto every control envelope (`register`, `registerShared`, `attach`, `assign`, `deregister`) this backend subsequently builds. Pushed by `Bridge::setDefaultSession()` and `Bridge::switchBackend()`. Default implementation is a no-op. See [Session propagation to control envelopes](#session-propagation-to-control-envelopes). | ## Connect/disconnect notifications @@ -116,6 +117,35 @@ its `connected`/`disconnected` `QWebSocket` signal slots invoke they already invoke `_reconnectHandler`/schedule a reconnect — see that section below. +## Session propagation to control envelopes + +`Bridge::executeVia` stamps `Bridge::defaultSession()` onto the `ActionCall` +passed to `execute()` (see [bridge.md](bridge.md)), so `execute` envelopes +always carry the current session. Control messages — `register`, +`registerShared` (`registerModelShared`), `attach` (`attachModel`), `assign` +(`assignPrimary`), and `deregister` (`deregisterModel`) — are different: each +is built directly inside the concrete backend, which has no other route to +the `Bridge`'s session. Before `IBackend::setSession` existed, every one of +these envelopes carried a default-constructed (empty, unauthenticated) +`session::Context` regardless of what `Bridge::setDefaultSession()` held, so +`RemoteServer::authorizeRegister` could never see a caller's identity and the +owner principal it records at `register` time was always empty — degrading +`IAuthorizer::authorizeInstance`'s ownership check to allow-all for every +instance a `Bridge` registered (see [session.md](../session/session.md)). + +`Bridge` calls `IBackend::setSession` in two places: once from its +constructor (with the just-constructed, typically empty, default session) and +again every time `setDefaultSession()` installs a new one; `switchBackend()` +also calls it on the incoming backend, **before** phase 1's +per-binding re-registration loop runs, so every `register`/`registerShared` +envelope built while re-registering handlers on the new backend already +carries the current session. A wire-backed backend that overrides +`setSession` — `SimulatedRemoteBackend`, `SocketBackend`, `QtWebSocketBackend` +— stores the session and reads it back into every control envelope's +`session` field it subsequently builds. `LocalBackend` does not override +`setSession`: the local path never serialises a `Context` onto a wire +envelope, so there is nothing to stamp. + ## Asynchronous registration — `registerModelAsync` `registerModel`/`registerModelWithContext` are synchronous: a backend whose @@ -224,6 +254,7 @@ Four exception types are thrown into in-flight `Completion`s: it never runs under `_regMtx` or `Bridge::_mtx`, so a sink that re-enters the bridge cannot deadlock. - `setReconnectHandler`/`setConnectHandler`/`setDisconnectHandler` — no-op (no transport to (dis)connect). +- `setSession` — not overridden (the default no-op stands): the local path never serialises a `Context` onto a wire envelope, so there is nothing to stamp. Each model instance gets its own strand so actions are serialised per-model without a global lock on the pool. @@ -568,6 +599,10 @@ in-process simulation of remote execution. locally. - `cancelPending` snapshots and resolves pending completions, same pattern as `LocalBackend`. +- `setSession` stores the session (guarded by its own mutex); every + subsequently built `register`/`registerShared`/`attach`/`assign`/`deregister` + envelope's `session` field is set from it before the `handleInline` call — + see [Session propagation to control envelopes](#session-propagation-to-control-envelopes). **Connection scope.** The default constructor, `SimulatedRemoteBackend(RemoteServer&)`, carries `ConnectionId{0}` — the server's "unscoped" sentinel — on every call it @@ -715,6 +750,12 @@ server assigns fresh ones on the new connection (cross-ref bridge.md). application installs them directly on the backend (not through `Bridge`) to drive its own connection-state UI. +**`setSession(session)`** stores the session in `_session` (this backend is +single-threaded — Qt event loop thread only — so no lock is needed); every +subsequently built `register`/`registerShared`/`attach`/`assign`/`deregister` +envelope's `session` field is set from it before sending. See +[Session propagation to control envelopes](#session-propagation-to-control-envelopes). + **`waitForConnected(timeoutMs = 5000)`** pumps the Qt event loop until the socket connects or the timeout elapses; returns the current `_connected` flag. Intended to be called once after construction on the Qt thread. @@ -939,7 +980,14 @@ peer no longer leaks the model, because `SocketServer` now participates in `RemoteServer`'s connection-scope contract exactly as `QtWebSocketServer` does — see below); `execute` assigns a monotonic `callId`, is fully asynchronous, and supports concurrent in-flight calls matched by -`callId` exactly like the Qt transport. Reconnect is configured by +`callId` exactly like the Qt transport. `setSession` stores the session +under its own mutex (`_sessionMtx`, guarding the one field it protects — this +backend is driven from multiple threads, unlike the single-threaded Qt +transport) and every subsequently built `register`/`registerShared`/ +`attach`/`assign`/`deregister` envelope's `session` field is set from it +before sending — see +[Session propagation to control envelopes](#session-propagation-to-control-envelopes). +Reconnect is configured by `SocketBackendConfig` (aliased `SocketBackend::Config`), with the same four fields and defaults as `QtWebSocketBackendConfig` (`reconnectEnabled`, `initialReconnectDelay`, `maxReconnectDelay`, `backoffMultiplier`) plus one new @@ -1144,6 +1192,7 @@ thread to marshal onto. | `setReconnectHandler` | `virtual void setReconnectHandler(const function&)` | Default: no-op. Fires only on the second and later connects. | | `setConnectHandler` | `virtual void setConnectHandler(const function&)` | Default: no-op. Fires on every successful connect, first included. | | `setDisconnectHandler` | `virtual void setDisconnectHandler(const function&)` | Default: no-op. Fires whenever the transport drops, before any reconnect is scheduled. | +| `setSession` | `virtual void setSession(session::Context)` | Default: no-op. Stamped onto every control envelope (`register`/`registerShared`/`attach`/`assign`/`deregister`) subsequently built. See [Session propagation to control envelopes](#session-propagation-to-control-envelopes). | ### Error types diff --git a/docs/spec/core/bridge.md b/docs/spec/core/bridge.md index 938a7bfd..84782af4 100644 --- a/docs/spec/core/bridge.md +++ b/docs/spec/core/bridge.md @@ -185,7 +185,14 @@ two overloads: every existing call site ambiguous. It converts to a `shared_ptr` and delegates to the overload above. -Both run the same two phases under `_mtx`: +Before either phase, the current default session is pushed onto the new +backend via `newBackend->setSession(...)` (read under `_sessionMtx` alone, +never held while calling into the backend) — so every control envelope phase +1 builds while re-registering handlers on the new backend already carries the +session, exactly as it would on the backend that is being replaced. See +`IBackend::setSession` and [backend.md](backend.md#session-propagation-to-control-envelopes). + +Both phases below run under `_mtx`: - **Phase 1 — stage, do not mutate.** Every live binding is registered on the new backend and the resulting `(binding, newId)` pairs are collected into a @@ -242,7 +249,14 @@ sending a now-destroyed `ModelId` to the backend. **`setDefaultSession(session)`** / **`defaultSession()`** installs a default `morph::session::Context` that is attached to every `executeVia()` call. -Thread-safe, separate mutex from `_mtx`. +Thread-safe, separate mutex from `_mtx`. `setDefaultSession` also pushes the +new session to the active backend via `IBackend::setSession` (copied out from +under `_sessionMtx` before the call, never while holding it), so every +control envelope (`register`/`registerShared`/`attach`/`assign`/`deregister`) +the backend subsequently builds carries the session too — not only `execute` +envelopes. The constructor does the same with the (typically empty) initial +session. See [session.md](../session/session.md#how-a-context-originates-and-flows) +and [backend.md](backend.md#session-propagation-to-control-envelopes). **`setPrincipal(principal)`** / **`currentPrincipal()`** installs and reads back a `morph::session::Principal` — the verified identity + roles, readable @@ -547,14 +561,14 @@ make teardown order-independent.) | Member | Signature | Notes | |---|---|---| -| ctor | `explicit Bridge(unique_ptr)` | Installs reconnect handler on the backend. | +| ctor | `explicit Bridge(unique_ptr)` | Installs reconnect handler on the backend, then pushes the (initially empty) default session via `setSession`. | | dtor | `~Bridge()` | Clears the active backend's reconnect handler, then cancels all pending completions with `BridgeDestroyedError`. | | `registerHandler` | `shared_ptr registerHandler()` | Default factory. Prefers `IBackend::registerModelAsync`; see `backend.md`. | | `registerHandler(binding)` | `void registerHandler(const shared_ptr&)` | Pre-built binding. Same async-preferring behavior. | -| `switchBackend` | `void switchBackend(unique_ptr)` / `void switchBackend(shared_ptr)` | Atomic: stages all re-registrations on the new backend, commits (publishes new ids + swaps) only if all succeed, else rolls back and rethrows leaving old backend + `currentId`s intact. Cancels old backend's pending ops with `BackendChangedError`. Holds both `_mtx` and `_attachMtx` for its duration. The `unique_ptr` overload is a template on the concrete backend type and delegates to the `shared_ptr` one — see below. | +| `switchBackend` | `void switchBackend(unique_ptr)` / `void switchBackend(shared_ptr)` | Pushes the current default session onto the new backend via `setSession` before staging. Atomic: stages all re-registrations on the new backend, commits (publishes new ids + swaps) only if all succeed, else rolls back and rethrows leaving old backend + `currentId`s intact. Cancels old backend's pending ops with `BackendChangedError`. Holds both `_mtx` and `_attachMtx` for its duration. The `unique_ptr` overload is a template on the concrete backend type and delegates to the `shared_ptr` one — see below. | | `deregisterHandler` | `void deregisterHandler(const shared_ptr&)` | Deregisters from active backend (if bound), resets `currentId` to 0, removes from tracking. | | `executeVia` | `Completion executeVia(const shared_ptr&, Action, IExecutor*)` | Lock-free dispatch. Attaches default session. On `LocalBackend`, rejects an action whose `ActionValidator::ready` returns `false` with `morph::model::ValidationError` via `onError`, before `Model::execute` runs. Records a journal `LogEntry` for loggable actions on both success (`Outcome::Succeeded`) and a throwing `Model::execute` (`Outcome::Failed`, rethrown unchanged). Value-forwarding into the typed `Completion` is `try`/`catch`-guarded — a throwing result move/copy resolves the completion via `onError` instead of hanging or terminating. The bridge-touching side effects (`onResult`, `hasSubscribers()`/`publishResult`, and the `pendingCalls()` decrement) are gated on the `_liveness` token, checked before any runs, so a completion resolving after `~Bridge()` skips them instead of touching the dangling `Bridge`. Increments `pendingCalls()` once per call before dispatch (never for the synchronous "handler not bound" early return); decrements it exactly once, from whichever of the two mutually-exclusive resolution continuations actually fires. | -| `setDefaultSession` | `void setDefaultSession(session::Context)` | Installs default session context. | +| `setDefaultSession` | `void setDefaultSession(session::Context)` | Installs default session context; also pushes it to the active backend via `IBackend::setSession` so control envelopes (register/attach/assign/deregister) carry it too, not only `execute`. | | `defaultSession` | `session::Context defaultSession() const` | Returns snapshot of default session. | | `setPrincipal` | `void setPrincipal(session::Principal)` | Installs the verified `Principal`, readable outside a dispatch. Pass `Principal{}` to clear (sign-out). | | `currentPrincipal` | `session::Principal currentPrincipal() const` | Returns a snapshot of the installed `Principal`; default-constructed if none was ever set. | diff --git a/docs/spec/session/session.md b/docs/spec/session/session.md index 4868ccb7..a646301d 100644 --- a/docs/spec/session/session.md +++ b/docs/spec/session/session.md @@ -76,9 +76,28 @@ From the `ActionCall` the session travels backend-specifically: authorizer, then (post-`authenticate`) installed via `ScopedContext` around dispatch. +**Control envelopes carry the session too.** The above covers `execute` +envelopes, built once per call by `Bridge::executeVia`. Control envelopes — +`register`, `registerShared`, `attach`, `assign`, `deregister` — are built +directly inside the concrete backend (`registerModelWithContext`, +`registerModelShared`, `attachModel`, `assignPrimary`, `deregisterModel`), +which has no other route to the `Bridge`'s session. `IBackend::setSession` +closes this: `Bridge` calls it on construction and on every +`setDefaultSession()`, and on the incoming backend during `switchBackend()` +(before any control envelope is built to re-register handlers), so a +wire-backed backend (`SimulatedRemoteBackend`, `SocketBackend`, +`QtWebSocketBackend`) always has the current session on hand to stamp onto +these envelopes too. Without this, `RemoteServer::authorizeRegister` could +never see a caller's identity and the owner principal recorded at `register` +time was always empty, degrading `authorizeInstance`'s ownership check to +allow-all (see [backend.md](../core/backend.md)'s "Session propagation to +control envelopes"). `LocalBackend` does not override `setSession`: the local +path never serialises a `Context` onto a wire envelope in the first place. + The bridge plumbing (`setDefaultSession`/`defaultSession`, the `ActionCall` -stamp) is specified in [bridge.md](../core/bridge.md); this spec covers only the -`Context` payload and its server-side handling. +stamp, `IBackend::setSession`) is specified in [bridge.md](../core/bridge.md) +and [backend.md](../core/backend.md); this spec covers only the `Context` +payload and its server-side handling. ## Principal — readable authorization state outside a dispatch diff --git a/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index d10e484e..c4b9236f 100644 --- a/include/morph/core/backend.hpp +++ b/include/morph/core/backend.hpp @@ -389,6 +389,32 @@ struct IBackend { /// @param handler Callable invoked on the backend's transport thread whenever /// the connection drops. Pass `nullptr` to clear. virtual void setDisconnectHandler(const std::function& handler) { (void)handler; } + + /// @brief Installs the session `Bridge` stamps onto every control envelope + /// this backend builds (`register`, `registerShared`, `attach`, + /// `assign`, `deregister`). + /// + /// `Bridge::executeVia` already stamps `Bridge::defaultSession()` onto the + /// `ActionCall` passed to `execute()`, so the session reaches `execute` + /// envelopes regardless of this hook. Control messages are different: they + /// are built directly by the concrete backend (`registerModelWithContext`, + /// `registerModelShared`, `attachModel`, `assignPrimary`, `deregisterModel`), + /// which has no other way to learn the `Bridge`'s current session. Without + /// this hook those envelopes always carried a default-constructed (empty, + /// unauthenticated) `session::Context`, so `RemoteServer::authorizeRegister` + /// could never see a caller's identity and the owner principal recorded at + /// `register` time was always empty — degrading `authorizeInstance`'s + /// ownership check to allow-all for every instance a `Bridge` registered. + /// + /// `Bridge::setDefaultSession()` calls this immediately, and + /// `Bridge::switchBackend()` calls it on the new backend before any + /// re-registration runs, so every control envelope built afterward carries + /// the current session. Default implementation: store-and-ignore, matching + /// `setReconnectHandler`'s pattern. `LocalBackend` needs no override — the + /// local path never serialises a session onto a wire envelope in the first + /// place (see docs/spec/session/session.md). + /// @param session Session to stamp onto every subsequently built control envelope. + virtual void setSession(::morph::session::Context session) { (void)session; } }; // NOLINTEND(cppcoreguidelines-special-member-functions) diff --git a/include/morph/core/bridge.hpp b/include/morph/core/bridge.hpp index bc8c27a0..337cda02 100644 --- a/include/morph/core/bridge.hpp +++ b/include/morph/core/bridge.hpp @@ -243,6 +243,11 @@ class Bridge { explicit Bridge(std::unique_ptr<::morph::backend::detail::IBackend> backend) : _backend{std::shared_ptr<::morph::backend::detail::IBackend>(std::move(backend))} { installReconnectHandler(_backend); + // A null initial backend is tolerated (see installReconnectHandler's own + // null check just above) — there is nothing yet to stamp a session onto. + if (_backend) { + _backend->setSession(_defaultSession); + } } /// @brief Cancels every still-pending completion on the active backend. @@ -668,8 +673,19 @@ class Bridge { /// /// @param session The new default. Pass `{}` to clear. void setDefaultSession(::morph::session::Context session) { - std::scoped_lock const lock{_sessionMtx}; - _defaultSession = std::move(session); + std::shared_ptr<::morph::backend::detail::IBackend> backend; + { + std::scoped_lock const lock{_sessionMtx}; + _defaultSession = session; + backend = loadBackend(); + } + // Pushed to the backend outside _sessionMtx: IBackend::setSession's + // default implementation just stores a copy, but a concrete override + // must not run under this bridge's own lock. Mirrors defaultSession()'s + // copy-out-then-release pattern. + if (backend) { + backend->setSession(std::move(session)); + } } /// @brief Returns a copy of the currently installed default session. Thread-safe. @@ -769,6 +785,17 @@ class Bridge { /// @param newBackend Replacement backend, shared with the caller. void switchBackend(std::shared_ptr<::morph::backend::detail::IBackend> newBackend) { auto newShared = std::move(newBackend); + // Stamp the current default session onto the new backend before phase 1 + // below builds a single `register`/`registerShared` envelope per live + // binding — otherwise every control envelope re-registering handlers on + // the new backend would carry a default-constructed (unauthenticated) + // session, exactly the #63 gap this hook closes. Read under + // `_sessionMtx` alone (a leaf mutex never held while calling into this + // backend), mirroring `executeVia`'s copy-then-release pattern. + { + std::scoped_lock const lock{_sessionMtx}; + newShared->setSession(_defaultSession); + } std::shared_ptr<::morph::backend::detail::IBackend> previous; { // Both mutexes: this phase reads/writes every live binding's diff --git a/include/morph/core/remote.hpp b/include/morph/core/remote.hpp index f135d5b8..b283c080 100644 --- a/include/morph/core/remote.hpp +++ b/include/morph/core/remote.hpp @@ -1543,8 +1543,9 @@ class SimulatedRemoteBackend : public detail::IBackend { ::morph::exec::detail::ModelId registerModelWithContext( const std::string& typeId, std::function()> /*factory*/, std::string_view contextKey) override { - auto reply = ::morph::wire::decode(_server.handleInline( - ::morph::wire::encode(::morph::wire::makeRegister(typeId, std::string{contextKey})), _cid)); + auto env = ::morph::wire::makeRegister(typeId, std::string{contextKey}); + env.session = currentSession(); + auto reply = ::morph::wire::decode(_server.handleInline(::morph::wire::encode(env), _cid)); if (reply.kind == "ok") { return ::morph::exec::detail::ModelId{reply.modelId}; } @@ -1567,10 +1568,10 @@ class SimulatedRemoteBackend : public detail::IBackend { if (identity.primary.empty()) { return registerModelWithContext(typeId, std::move(factory), identity.contextKey); } - auto reply = ::morph::wire::decode(_server.handleInline( - ::morph::wire::encode(::morph::wire::makeRegisterShared(typeId, std::string{identity.primary}, - std::string{identity.contextKey})), - _cid)); + auto env = ::morph::wire::makeRegisterShared(typeId, std::string{identity.primary}, + std::string{identity.contextKey}); + env.session = currentSession(); + auto reply = ::morph::wire::decode(_server.handleInline(::morph::wire::encode(env), _cid)); if (reply.kind == "ok") { return ::morph::exec::detail::ModelId{reply.modelId}; } @@ -1597,10 +1598,10 @@ class SimulatedRemoteBackend : public detail::IBackend { } return registerModelWithContext(typeId, std::move(factory), identity.contextKey); } - auto reply = ::morph::wire::decode(_server.handleInline( - ::morph::wire::encode(::morph::wire::makeAttach(typeId, std::string{identity.primary}, current.v, - std::string{identity.contextKey})), - _cid)); + auto env = ::morph::wire::makeAttach(typeId, std::string{identity.primary}, current.v, + std::string{identity.contextKey}); + env.session = currentSession(); + auto reply = ::morph::wire::decode(_server.handleInline(::morph::wire::encode(env), _cid)); if (reply.kind == "ok") { return ::morph::exec::detail::ModelId{reply.modelId}; } @@ -1616,8 +1617,9 @@ class SimulatedRemoteBackend : public detail::IBackend { if (primary.empty() || mid.v == 0U) { return; } - (void)_server.handleInline(::morph::wire::encode(::morph::wire::makeAssign(typeId, std::string{primary}, mid.v)), - _cid); + auto env = ::morph::wire::makeAssign(typeId, std::string{primary}, mid.v); + env.session = currentSession(); + (void)_server.handleInline(::morph::wire::encode(env), _cid); } /// @brief Asks the server for the live shared primary keys of @p typeId. @@ -1645,7 +1647,9 @@ class SimulatedRemoteBackend : public detail::IBackend { /// `deregister` does (`backend.md`, "Connection scopes"). /// @param mid Id of the model to deregister. void deregisterModel(::morph::exec::detail::ModelId mid) override { - (void)_server.handleInline(::morph::wire::encode(::morph::wire::makeDeregister(mid.v)), _cid); + auto env = ::morph::wire::makeDeregister(mid.v); + env.session = currentSession(); + (void)_server.handleInline(::morph::wire::encode(env), _cid); } /// @brief Sends a `"hello"` envelope to the server and classifies its reply. @@ -1730,6 +1734,15 @@ class SimulatedRemoteBackend : public detail::IBackend { } } + /// @brief Installs the session stamped onto every control envelope this + /// backend subsequently builds (`register`, `registerShared`, + /// `attach`, `assign`, `deregister`). See `IBackend::setSession`. + /// @param session Session to stamp; typically pushed by `Bridge::setDefaultSession()`. + void setSession(::morph::session::Context session) override { + std::scoped_lock const lock{_sessionMtx}; + _session = std::move(session); + } + private: void trackPending(const std::shared_ptr<::morph::async::detail::CompletionState>>& state) { std::scoped_lock const lock{_pendingMtx}; @@ -1737,6 +1750,12 @@ class SimulatedRemoteBackend : public detail::IBackend { _pending.emplace_back(state); } + /// @brief Returns a copy of the session last installed via `setSession`. + [[nodiscard]] ::morph::session::Context currentSession() const { + std::scoped_lock const lock{_sessionMtx}; + return _session; + } + RemoteServer& _server; // 0 = unscoped (the default constructor's behavior, unchanged); non-zero // when constructed with a ConnectionId from server.openConnection() (see @@ -1745,6 +1764,8 @@ class SimulatedRemoteBackend : public detail::IBackend { ConnectionId _cid{0}; std::mutex _pendingMtx; std::vector>>> _pending; + mutable std::mutex _sessionMtx; + ::morph::session::Context _session; }; } // namespace morph::backend diff --git a/include/morph/net/socket_backend.hpp b/include/morph/net/socket_backend.hpp index fc9e42a8..7141f07d 100644 --- a/include/morph/net/socket_backend.hpp +++ b/include/morph/net/socket_backend.hpp @@ -132,9 +132,11 @@ class SocketBackend : public ::morph::backend::detail::IBackend { ::morph::exec::detail::ModelId registerModel( const std::string& typeId, std::function()> /*factory*/) override { + auto env = ::morph::wire::makeRegister(typeId); + env.session = currentSession(); std::string replyJson; try { - replyJson = sendSync(::morph::wire::encode(::morph::wire::makeRegister(typeId))); + replyJson = sendSync(::morph::wire::encode(env)); } catch (const std::exception& exc) { throw std::runtime_error(std::string{"register failed: "} + exc.what()); } @@ -160,9 +162,10 @@ class SocketBackend : public ::morph::backend::detail::IBackend { if (identity.primary.empty()) { return registerModelWithContext(typeId, std::move(factory), identity.contextKey); } - return sendControlForId(::morph::wire::makeRegisterShared(typeId, std::string{identity.primary}, - std::string{identity.contextKey}), - "register"); + auto env = ::morph::wire::makeRegisterShared(typeId, std::string{identity.primary}, + std::string{identity.contextKey}); + env.session = currentSession(); + return sendControlForId(env, "register"); } /// @brief Sends an `attach` and blocks for the reply, re-pointing from @p current. @@ -181,9 +184,10 @@ class SocketBackend : public ::morph::backend::detail::IBackend { } return registerModelWithContext(typeId, std::move(factory), identity.contextKey); } - return sendControlForId( - ::morph::wire::makeAttach(typeId, std::string{identity.primary}, current.v, std::string{identity.contextKey}), - "attach"); + auto env = ::morph::wire::makeAttach(typeId, std::string{identity.primary}, current.v, + std::string{identity.contextKey}); + env.session = currentSession(); + return sendControlForId(env, "attach"); } /// @brief Files a live server-side instance under @p primary. @@ -195,7 +199,9 @@ class SocketBackend : public ::morph::backend::detail::IBackend { if (primary.empty() || mid.v == 0U) { return; } - (void)sendControlForId(::morph::wire::makeAssign(typeId, std::string{primary}, mid.v), "assign"); + auto env = ::morph::wire::makeAssign(typeId, std::string{primary}, mid.v); + env.session = currentSession(); + (void)sendControlForId(env, "assign"); } /// @brief Asks the server for the live shared primary keys of @p typeId. @@ -225,8 +231,9 @@ class SocketBackend : public ::morph::backend::detail::IBackend { void deregisterModel(::morph::exec::detail::ModelId mid) override { if (_connected.load()) { try { - sendFrame(::morph::net::detail::WsOpcode::kText, - ::morph::wire::encode(::morph::wire::makeDeregister(mid.v))); + auto env = ::morph::wire::makeDeregister(mid.v); + env.session = currentSession(); + sendFrame(::morph::net::detail::WsOpcode::kText, ::morph::wire::encode(env)); } catch (const std::exception&) { // Fire-and-forget: same documented trade-off as // QtWebSocketBackend — a failed send just leaks the model on @@ -321,7 +328,22 @@ class SocketBackend : public ::morph::backend::detail::IBackend { _reconnectHandler = handler; } + /// @brief Installs the session stamped onto every control envelope this + /// backend subsequently builds (`register`, `registerShared`, + /// `attach`, `assign`, `deregister`). See `IBackend::setSession`. + /// @param session Session to stamp; typically pushed by `Bridge::setDefaultSession()`. + void setSession(::morph::session::Context session) override { + std::scoped_lock lock{_sessionMtx}; + _session = std::move(session); + } + private: + /// @brief Returns a copy of the session last installed via `setSession`. + [[nodiscard]] ::morph::session::Context currentSession() const { + std::scoped_lock lock{_sessionMtx}; + return _session; + } + struct PendingExecute { std::shared_ptr<::morph::async::detail::CompletionState>> state; std::function(std::string_view)> deserialize; @@ -624,6 +646,9 @@ class SocketBackend : public ::morph::backend::detail::IBackend { std::mutex _reconnectHandlerMtx; std::function _reconnectHandler; + mutable std::mutex _sessionMtx; + ::morph::session::Context _session; + std::mutex _handlerMtx; std::condition_variable _handlerCv; bool _handlerPending{false}; diff --git a/include/morph/qt/qt_websocket_backend.hpp b/include/morph/qt/qt_websocket_backend.hpp index 1b44e1e9..e5bdfab5 100644 --- a/include/morph/qt/qt_websocket_backend.hpp +++ b/include/morph/qt/qt_websocket_backend.hpp @@ -318,6 +318,12 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { /// drops. Pass `nullptr` to clear. void setDisconnectHandler(const std::function& handler) override; + /// @brief Installs the session stamped onto every control envelope this + /// backend subsequently builds (`register`, `registerShared`, + /// `attach`, `assign`, `deregister`). See `IBackend::setSession`. + /// @param session Session to stamp; typically pushed by `Bridge::setDefaultSession()`. + void setSession(::morph::session::Context session) override; + private: /// @brief Sends @p msg synchronously by blocking the Qt thread via a nested event loop. std::string sendSync(const std::string& msg); @@ -357,6 +363,7 @@ class QtWebSocketBackend : public ::morph::backend::detail::IBackend { std::function _reconnectHandler; std::function _connectHandler; std::function _disconnectHandler; + ::morph::session::Context _session; std::string _pendingReply; QEventLoop* _syncLoop{nullptr}; diff --git a/src/qt/qt_websocket_backend.cpp b/src/qt/qt_websocket_backend.cpp index 0d597153..0fa25abc 100644 --- a/src/qt/qt_websocket_backend.cpp +++ b/src/qt/qt_websocket_backend.cpp @@ -138,9 +138,11 @@ std::string QtWebSocketBackend::sendSync(const std::string& msg) { ::morph::exec::detail::ModelId QtWebSocketBackend::registerModel( const std::string& typeId, std::function()> /*factory*/) { + auto env = ::morph::wire::makeRegister(typeId); + env.session = _session; std::string replyJson; try { - replyJson = sendSync(::morph::wire::encode(::morph::wire::makeRegister(typeId))); + replyJson = sendSync(::morph::wire::encode(env)); } catch (const std::exception& exc) { // sendSync throws "disconnected" if the socket drops (or was never // connected) while the register reply was outstanding — the nested event @@ -194,6 +196,7 @@ void QtWebSocketBackend::sendRegisterAsync(const std::string& typeId, std::strin } auto env = ::morph::wire::makeRegister(typeId, std::string{contextKey}); env.callId = callId; + env.session = _session; _socket.sendTextMessage(QString::fromStdString(::morph::wire::encode(env))); } @@ -241,9 +244,10 @@ ::morph::exec::detail::ModelId QtWebSocketBackend::registerModelShared( if (identity.primary.empty()) { return registerModelWithContext(typeId, std::move(factory), identity.contextKey); } - return modelIdFromReply(sendSync(::morph::wire::encode(::morph::wire::makeRegisterShared( - typeId, std::string{identity.primary}, std::string{identity.contextKey}))), - "register"); + auto env = ::morph::wire::makeRegisterShared(typeId, std::string{identity.primary}, + std::string{identity.contextKey}); + env.session = _session; + return modelIdFromReply(sendSync(::morph::wire::encode(env)), "register"); } ::morph::exec::detail::ModelId QtWebSocketBackend::attachModel( @@ -255,10 +259,10 @@ ::morph::exec::detail::ModelId QtWebSocketBackend::attachModel( } return registerModelWithContext(typeId, std::move(factory), identity.contextKey); } - return modelIdFromReply( - sendSync(::morph::wire::encode(::morph::wire::makeAttach(typeId, std::string{identity.primary}, current.v, - std::string{identity.contextKey}))), - "attach"); + auto env = ::morph::wire::makeAttach(typeId, std::string{identity.primary}, current.v, + std::string{identity.contextKey}); + env.session = _session; + return modelIdFromReply(sendSync(::morph::wire::encode(env)), "attach"); } void QtWebSocketBackend::assignPrimary(::morph::exec::detail::ModelId mid, const std::string& typeId, @@ -266,8 +270,9 @@ void QtWebSocketBackend::assignPrimary(::morph::exec::detail::ModelId mid, const if (primary.empty() || mid.v == 0U) { return; } - (void)modelIdFromReply( - sendSync(::morph::wire::encode(::morph::wire::makeAssign(typeId, std::string{primary}, mid.v))), "assign"); + auto env = ::morph::wire::makeAssign(typeId, std::string{primary}, mid.v); + env.session = _session; + (void)modelIdFromReply(sendSync(::morph::wire::encode(env)), "assign"); } bool QtWebSocketBackend::assignPrimaryAsync(::morph::exec::detail::ModelId mid, const std::string& typeId, @@ -329,6 +334,7 @@ void QtWebSocketBackend::deregisterModel(::morph::exec::detail::ModelId mid) { } auto env = ::morph::wire::makeDeregister(mid.v); env.callId = callId; + env.session = _session; _socket.sendTextMessage(QString::fromStdString(::morph::wire::encode(env))); } @@ -418,6 +424,8 @@ void QtWebSocketBackend::setConnectHandler(const std::function& handler) void QtWebSocketBackend::setDisconnectHandler(const std::function& handler) { _disconnectHandler = handler; } +void QtWebSocketBackend::setSession(::morph::session::Context session) { _session = std::move(session); } + void QtWebSocketBackend::scheduleReconnect() { _reconnectTimer.start(static_cast(_currentReconnectDelay.count())); // Pre-compute the next backoff so the timer above used the *current* one. diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index a415fdf6..41ae7a35 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -78,6 +78,7 @@ add_executable(morph_tests test_session_auth.cpp test_policy_hardening.cpp test_register_authorization.cpp + test_register_session.cpp test_opaque_model_ids.cpp test_graceful_shutdown.cpp test_pinned_facts.cpp diff --git a/tests/test_register_session.cpp b/tests/test_register_session.cpp new file mode 100644 index 00000000..67c4f3df --- /dev/null +++ b/tests/test_register_session.cpp @@ -0,0 +1,150 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Reproduces and verifies the fix for #63: register/attach/assign/deregister +// envelopes built by the wire-backed IBackend implementations (SimulatedRemoteBackend, +// SocketBackend) never carried Bridge::defaultSession(), so a RemoteServer whose +// authorizer requires authentication rejected every register — and, even under an +// allow-all authorizer, the recorded owner principal was always empty, degrading +// authorizeInstance's ownership check to allow-all. See docs/spec/session/session.md +// ("How a Context originates and flows") and docs/spec/core/backend.md ("IBackend"). + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "test_support.hpp" + +using SyncExecutor = morph::testing::InlineExecutor; + +struct RegSessAction { + int value = 0; +}; +struct RegSessModel { + int execute(const RegSessAction& act) { return act.value + 1; } +}; + +BRIDGE_REGISTER_MODEL(RegSessModel, "RS_Model") +BRIDGE_REGISTER_ACTION(RegSessModel, RegSessAction, "RS_Action") + +namespace { + +// Requires authentication (a validly-signed token) before allowing register — +// exactly the RegisterGate example in docs/spec/session/session.md. +struct RegisterRequiresAuth : morph::session::SigningAuthorizer { + using SigningAuthorizer::SigningAuthorizer; + [[nodiscard]] bool authorizeRegister(const morph::session::Context& ctx, std::string_view) const override { + return !ctx.principal.empty(); + } +}; + +} // namespace + +TEST_CASE("Bridge::setDefaultSession's token reaches the register envelope so authorizeRegister can allow it", + "[bridge][remote][session]") { + morph::exec::ThreadPoolExecutor serverPool{2}; + const std::string secret = "reg-session-secret"; + auto authz = std::make_shared(secret); + auto server = std::make_shared(serverPool, authz); + + SyncExecutor cbExec; + morph::bridge::Bridge bridge{std::make_unique(*server)}; + + morph::session::Context session; + session.principal = "alice"; + session.token = morph::session::TokenIssuer{secret}.issue({.principal = "alice", .expiresAtMs = 9999999999999}); + bridge.setDefaultSession(session); + + // Before the fix, registerModelWithContext (called from BridgeHandler's + // constructor) sent a register envelope with a default-constructed + // (empty) session, so RegisterRequiresAuth::authorizeRegister denied it + // and this constructor threw "register failed: unauthorized". + morph::bridge::BridgeHandler handler{bridge, &cbExec}; + + std::atomic result{-1}; + handler.execute(RegSessAction{41}).then([&](int val) { result.store(val); }).onError([](const std::exception_ptr&) { + }); + + for (int idx = 0; idx < 50 && result.load() == -1; ++idx) { + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } + REQUIRE(result.load() == 42); +} + +namespace { + +// Ownership-enforcing authorizer, as in tests/test_policy_hardening.cpp: the +// principal recorded at register time must match the principal executing. +struct OwnershipAuthz : morph::session::SigningAuthorizer { + using SigningAuthorizer::SigningAuthorizer; + [[nodiscard]] bool authorizeInstance(const morph::session::Context& ctx, std::string_view, std::string_view, + std::uint64_t, std::string_view ownerPrincipal) const override { + return ownerPrincipal.empty() || ownerPrincipal == ctx.principal; + } +}; + +} // namespace + +TEST_CASE("register's recorded owner principal is the Bridge's authenticated default session, not always empty", + "[bridge][remote][session]") { + morph::exec::ThreadPoolExecutor serverPool{2}; + const std::string secret = "owner-session-secret"; + auto authz = std::make_shared(secret); + auto server = std::make_shared(serverPool, authz); + + SyncExecutor cbExec; + morph::bridge::Bridge aliceBridge{std::make_unique(*server)}; + morph::session::Context aliceSession; + aliceSession.principal = "alice"; + aliceSession.token = + morph::session::TokenIssuer{secret}.issue({.principal = "alice", .expiresAtMs = 9999999999999}); + aliceBridge.setDefaultSession(aliceSession); + + // Registers on behalf of "alice" — before the fix, the owner recorded on + // the server was always empty (the envelope's session was never set), + // which degraded authorizeInstance's ownership check to allow-all. + morph::bridge::BridgeHandler aliceHandler{aliceBridge, &cbExec}; + + std::atomic aliceResult{-1}; + aliceHandler.execute(RegSessAction{1}) + .then([&](int val) { aliceResult.store(val); }) + .onError([](const std::exception_ptr&) {}); + for (int idx = 0; idx < 50 && aliceResult.load() == -1; ++idx) { + std::this_thread::sleep_for(std::chrono::milliseconds(10)); + } + REQUIRE(aliceResult.load() == 2); + + // Bob (a different, also-valid principal) attempts to execute against the + // *same* model id alice's handler was bound to. Before the fix this would + // succeed (owner was empty => allow-all); after the fix it is denied. + morph::bridge::Bridge bobBridge{std::make_unique(*server)}; + morph::session::Context bobSession; + bobSession.principal = "bob"; + bobSession.token = morph::session::TokenIssuer{secret}.issue({.principal = "bob", .expiresAtMs = 9999999999999}); + bobBridge.setDefaultSession(bobSession); + + // Build a raw envelope targeting alice's modelId directly (bypassing a + // second registration) so we exercise authorizeInstance on the recorded owner. + auto probe = morph::wire::Envelope{}; + probe.kind = "execute"; + probe.modelId = aliceHandler.binding()->currentId.load(); + probe.modelType = "RS_Model"; + probe.actionType = "RS_Action"; + probe.body = R"({"value":1})"; + probe.session = bobSession; + morph::testing::WaitReply bobReply; + server->handle(morph::wire::encode(probe), std::ref(bobReply)); + REQUIRE(bobReply.await()); + REQUIRE(bobReply.env.kind == "err"); + REQUIRE(bobReply.env.message == "unauthorized"); +} From 5dccc1908a54d6b7acad0fbde158d2b00ef6187f Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Tue, 11 Aug 2026 23:37:34 +0300 Subject: [PATCH 2/6] Closes #62: apply control-byte JSON escaping to the three sibling writers Apply the same control-byte escaping option morph::wire::encode already uses (morph::wire::detail::EscapingWriteOpts) to the three writers on caller-supplied strings that never got the fix: - morph::journal::toJson(LogEntry) (journal/action_log.hpp) - morph::offline::detail::toJson(FileQueueRecord) (offline/file_offline_queue.hpp) - morph::session::TokenIssuer::issue(SessionToken) (session/session_auth.hpp) glaze 7.4 leaves ASCII control bytes (0x00-0x1F) unescaped by default: RFC 8259 requires them escaped, so a raw control byte in any of these writers' string fields produced JSON the sibling reader either failed to decode outright, or -- when the same string also held an escaped `\`/`"` -- silently corrupted via glaze's chunked writer path before the payload ever reached disk or was signed. Each file gets its own local EscapingWriteOpts (duplicated, not shared, to avoid a new core/ dependency from journal/offline/session -- these stay independently includable, e.g. under MORPH_CLIENT_ONLY). Also routes TokenIssuer::issue's previously-discarded (void)glz::write_json(...) through a new throwOnGlazeError-style helper (mirroring the other two writers already had), adding a TokenIssuanceError type. Not realistically reachable for SessionToken's flat shape, but keeps the writer consistent with its siblings instead of silently serialising a claims blob that could later fail to verify. Updated docs/spec/journal/journal.md, docs/spec/offline/offline.md, and docs/spec/security.md to document the escaping and the new error type. Co-Authored-By: Claude Sonnet 5 --- docs/spec/journal/journal.md | 13 +- docs/spec/offline/offline.md | 7 +- docs/spec/security.md | 12 +- include/morph/journal/action_log.hpp | 25 +++- include/morph/offline/file_offline_queue.hpp | 22 ++- include/morph/session/session_auth.hpp | 57 +++++++- tests/CMakeLists.txt | 1 + tests/test_control_byte_escaping.cpp | 144 +++++++++++++++++++ 8 files changed, 271 insertions(+), 10 deletions(-) create mode 100644 tests/test_control_byte_escaping.cpp diff --git a/docs/spec/journal/journal.md b/docs/spec/journal/journal.md index ec42a8e0..d80a2915 100644 --- a/docs/spec/journal/journal.md +++ b/docs/spec/journal/journal.md @@ -81,8 +81,15 @@ the journal gains an entry it previously lacked. ### `toJson(LogEntry const&) -> std::string` -Encodes a `LogEntry` as JSON via Glaze. Throws `SerializationError` on failure -(not realistically reachable for a flat struct of strings/integers — see +Encodes a `LogEntry` as JSON via Glaze, writing with `detail::EscapingWriteOpts` +so a raw ASCII control byte (0x00-0x1F) in `entityKey`/`payload`/`error`/ +`principal`/`idempotencyKey` round-trips through `fromJson` instead of +producing invalid JSON — or, when the same string also holds an escaped `\`/`"`, +silently corrupted JSON (glaze's chunked writer path rewrites the control byte +as two `0x00` bytes in that case). Mirrors `morph::wire::detail::EscapingWriteOpts` +(`core/wire.hpp`) exactly; duplicated locally rather than shared so this header +stays free of a `core/` dependency. Throws `SerializationError` on failure (not +realistically reachable for a flat struct of strings/integers — see `detail::throwOnGlazeError`). ### `fromJson(std::string_view) -> LogEntry` @@ -684,7 +691,7 @@ All symbols live in `namespace morph::journal`. | `LogEntry` | struct | Flat aggregate: `seq`, `modelType`, `entityKey`, `actionType`, `payload`, `result`, `outcome`, `error`, `principal`, `timestampMs`, `idempotencyKey`, `v` (line-format version, default `kLogFormatVersion`). Glaze-reflected (no `glz::meta` of its own; `outcome`'s type `Outcome` has one). | | `Outcome` | `enum class : std::uint8_t` | `Succeeded` (default) or `Failed`. Has a `glz::meta` specialisation so it (de)serialises as the string, not the underlying int. | | `kLogFormatVersion` | `inline constexpr std::uint32_t` | Current line-format version (`1`). Bumped only on a breaking change to `LogEntry`'s shape. See [Line-format version (`v`)](#line-format-version-v). | -| `toJson` | free function | `std::string toJson(const LogEntry&)` — encodes as JSON. Throws `SerializationError`. | +| `toJson` | free function | `std::string toJson(const LogEntry&)` — encodes as JSON with `detail::EscapingWriteOpts` (control-byte escaping). Throws `SerializationError`. | | `fromJson` | free function | `LogEntry fromJson(std::string_view)` — decodes from JSON leniently (`error_on_unknown_keys = false`). Throws `SerializationError` on malformed JSON or if the decoded `v` exceeds `kLogFormatVersion`. | | `SerializationError` | struct | `: std::runtime_error`. Thrown by `toJson`/`fromJson`. | | `detail::throwOnGlazeError` | inline function | `void throwOnGlazeError(const glz::error_ctx&, std::string_view)` — shared error path for `toJson`/`fromJson`. | diff --git a/docs/spec/offline/offline.md b/docs/spec/offline/offline.md index a33a1e17..86d3dd88 100644 --- a/docs/spec/offline/offline.md +++ b/docs/spec/offline/offline.md @@ -194,7 +194,12 @@ restarts with **no extra dependency** — it ships in the default `morph` target alongside `InMemoryOfflineQueue`. Each mutation (`enqueue`, `markDone`, `setAttempts`, `setIdempotencyKey`) appends one JSON line (`{"op": "put"|"done", "id", "payload", "idempotencyKey", "attempts"}`) and -immediately `fflush`+`fsync`s it. On open, the file is replayed +immediately `fflush`+`fsync`s it. The line is written with +`detail::EscapingWriteOpts` (mirroring `morph::wire::detail::EscapingWriteOpts`, +`core/wire.hpp`) so a raw ASCII control byte in `payload`/`idempotencyKey` +round-trips instead of producing invalid JSON that breaks replay on the next +open — or, alongside an escaped `\`/`"` in the same string, JSON glaze's +writer silently corrupts before it ever reaches disk. On open, the file is replayed last-write-wins-per-id and rewritten in compacted form — this both bounds file growth and heals a torn trailing line left by a crash mid-write, tolerating it the same way `FileActionLog` does (a malformed *trailing* line is logged and diff --git a/docs/spec/security.md b/docs/spec/security.md index bec67bc4..243527bf 100644 --- a/docs/spec/security.md +++ b/docs/spec/security.md @@ -124,7 +124,17 @@ payload)` and `payload` is the base64url claims segment. The claims are a | `roles` | Coarse-grained roles an authorization policy can key on. | The claims are JSON (Glaze); adding application claims is compatible because -unknown fields are ignored on read. +unknown fields are ignored on read. `TokenIssuer::issue` writes the claims +with the same control-byte-escaping option `morph::wire::encode` uses +(`detail::EscapingWriteOpts`, duplicated locally in `session_auth.hpp` rather +than shared, to keep this header free of a `core/` dependency): a raw ASCII +control byte (0x00-0x1F) in `principal`/`roles` — most plausibly one embedded +in an application-supplied identity string — round-trips through +`TokenVerifier::verify` instead of producing JSON that either fails to decode +outright or, when the same string also holds an escaped `\`/`"`, is silently +corrupted before signing. `issue` throws `TokenIssuanceError` on encode +failure, though `SessionToken`'s flat shape (strings/integers only) makes that +not realistically reachable in practice. #### Expiry is mandatory — `expiresAtMs == 0` is expired, not eternal diff --git a/include/morph/journal/action_log.hpp b/include/morph/journal/action_log.hpp index aa2ee3ab..c73b8ebb 100644 --- a/include/morph/journal/action_log.hpp +++ b/include/morph/journal/action_log.hpp @@ -134,6 +134,24 @@ inline void throwOnGlazeError(const glz::error_ctx& errCode, std::string_view co } } +/// @brief Write options that escape ASCII control bytes as `\\uXXXX` sequences. +/// +/// glaze 7.4 leaves control bytes (0x00-0x1F) unescaped by default, which +/// breaks a `LogEntry` carrying one in `entityKey`/`payload`/`error`/`principal`/ +/// `idempotencyKey` two ways: RFC 8259 requires those bytes escaped, so the raw +/// byte alone yields JSON `fromJson`'s `glz::read` throws on; worse, once the +/// same string also contains an escaped `\` or `"`, glaze's chunked writer path +/// silently rewrites the control byte as two 0x00 bytes, corrupting the payload +/// before it ever reaches disk. This mirrors `morph::wire::detail::EscapingWriteOpts` +/// (`core/wire.hpp`) exactly; duplicated here (rather than shared) so this header +/// stays free of a `core/` dependency for `MORPH_CLIENT_ONLY`-style consumers +/// that only want the journal. Escaping is lossless, so any such byte still +/// round-trips through `fromJson` unchanged. +struct EscapingWriteOpts : glz::opts { + // NOLINTNEXTLINE(readability-identifier-naming) — glaze's option name, matched by name. + bool escape_control_characters = true; +}; + } // namespace detail /// @brief Encodes @p entry as JSON. @@ -144,11 +162,16 @@ inline void throwOnGlazeError(const glz::error_ctx& errCode, std::string_view co /// field is `Outcome`, which does have a `glz::meta` — see above — so it reads /// back as `"Succeeded"`/`"Failed"`, not `0`/`1`.) Used by sinks that need an /// opaque string representation (`FileActionLog`). +/// +/// Writes with `detail::EscapingWriteOpts` so a raw ASCII control byte in any +/// string field (`entityKey`, `payload`, `error`, `principal`, `idempotencyKey`) +/// round-trips through `fromJson` instead of producing invalid or silently +/// corrupted JSON — see that struct's doc comment. /// @throws SerializationError on encode failure (see `detail::throwOnGlazeError` /// for why this is not realistically reachable for `LogEntry`). inline std::string toJson(const LogEntry& entry) { std::string out; - detail::throwOnGlazeError(glz::write_json(entry, out), out); + detail::throwOnGlazeError(glz::write(entry, out), out); return out; } diff --git a/include/morph/offline/file_offline_queue.hpp b/include/morph/offline/file_offline_queue.hpp index 9d70fe39..d2a80d96 100644 --- a/include/morph/offline/file_offline_queue.hpp +++ b/include/morph/offline/file_offline_queue.hpp @@ -56,9 +56,29 @@ inline void throwOnGlazeError(const glz::error_ctx& errCode, std::string_view co } } +/// @brief Write options that escape ASCII control bytes as `\\uXXXX` sequences. +/// +/// glaze 7.4 leaves control bytes (0x00-0x1F) unescaped by default, which +/// breaks a `FileQueueRecord` carrying one in `payload`/`idempotencyKey` two +/// ways: RFC 8259 requires those bytes escaped, so the raw byte alone yields +/// JSON `fromJson`'s `glz::read` throws on for any non-trailing line (a +/// permanent `FileOfflineQueueError` on every later `open()`, per this file's +/// torn-trailing-line tolerance); worse, once the same string also contains an +/// escaped `\` or `"`, glaze's chunked writer path silently rewrites the +/// control byte as two 0x00 bytes, corrupting the payload before it ever +/// reaches disk. Mirrors `morph::wire::detail::EscapingWriteOpts` (`core/wire.hpp`) +/// exactly; duplicated here (rather than shared) so this header stays free of +/// a `core/` dependency. Escaping is lossless, so any such byte still +/// round-trips through `fromJson` unchanged. +struct EscapingWriteOpts : glz::opts { + // NOLINTNEXTLINE(readability-identifier-naming) — glaze's option name, matched by name. + bool escape_control_characters = true; +}; + +/// @brief Encodes @p record as JSON, escaping control bytes in `payload`/`idempotencyKey`. inline std::string toJson(const FileQueueRecord& record) { std::string out; - throwOnGlazeError(glz::write_json(record, out), out); + throwOnGlazeError(glz::write(record, out), out); return out; } diff --git a/include/morph/session/session_auth.hpp b/include/morph/session/session_auth.hpp index 6dd6f930..cbcdf53c 100644 --- a/include/morph/session/session_auth.hpp +++ b/include/morph/session/session_auth.hpp @@ -312,6 +312,52 @@ enum class AuthError : std::uint8_t { NotYetValid, ///< `issuedAtMs` is set and more than `kClockSkewMs` in the future. }; +/// @brief Thrown by `TokenIssuer::issue` if serialising the claims fails. +/// +/// Not realistically reachable for `SessionToken` (a flat aggregate of +/// strings/integers, same as `journal::LogEntry`/`FileQueueRecord`), but +/// routing through a throwing helper rather than discarding the error keeps +/// this writer consistent with its two sibling writers +/// (`journal::toJson`/`offline::detail::toJson`), instead of silently +/// serialising a claims blob that later fails to decode. +struct TokenIssuanceError : std::runtime_error { + using std::runtime_error::runtime_error; +}; + +namespace detail { + +/// @brief Converts a Glaze error into a `TokenIssuanceError`, or does nothing +/// if @p errCode reports success. See `journal::detail::throwOnGlazeError` +/// for the identical pattern in the sibling writer. +/// @param errCode Result of a `glz::write` call. +/// @param context Buffer passed to `glz::format_error` for the message. +inline void throwOnGlazeError(const glz::error_ctx& errCode, std::string_view context) { + if (errCode) { + throw TokenIssuanceError{glz::format_error(errCode, context)}; + } +} + +/// @brief Write options that escape ASCII control bytes as `\\uXXXX` sequences. +/// +/// glaze 7.4 leaves control bytes (0x00-0x1F) unescaped by default, which +/// breaks a `SessionToken` carrying one in `principal`/`roles` two ways: RFC +/// 8259 requires those bytes escaped, so the raw byte alone yields JSON this +/// morph's own `TokenVerifier` cannot decode — a signed token that fails to +/// verify, a silent issue-succeeds/verify-fails asymmetry; worse, once the +/// same string also contains an escaped `\` or `"`, glaze's chunked writer +/// path silently rewrites the control byte as two 0x00 bytes, corrupting the +/// claims before they are ever signed. Mirrors +/// `morph::wire::detail::EscapingWriteOpts` (`core/wire.hpp`) exactly; +/// duplicated here (rather than shared) so this header stays free of a +/// `core/` dependency. Escaping is lossless, so any such byte still +/// round-trips through `TokenVerifier::verify` unchanged. +struct EscapingWriteOpts : glz::opts { + // NOLINTNEXTLINE(readability-identifier-naming) — glaze's option name, matched by name. + bool escape_control_characters = true; +}; + +} // namespace detail + /// @brief Mints signed bearer tokens from claims using a shared secret. /// /// Wire format: `base64url(claimsJson) "." base64url(mac(secret, payload))`. @@ -337,13 +383,18 @@ class TokenIssuer { #endif /// @brief Serialises @p claims and returns a signed token string. + /// + /// Writes with `detail::EscapingWriteOpts` so a raw ASCII control byte in + /// `principal`/`roles` round-trips through `TokenVerifier::verify` instead + /// of producing invalid JSON (or, alongside an escaped `\`/`"`, silently + /// corrupted JSON) — see that struct's doc comment. /// @param claims Claims to embed and sign. /// @return The signed `payload.sig` token. + /// @throws TokenIssuanceError on encode failure (see `detail::throwOnGlazeError` + /// for why this is not realistically reachable for `SessionToken`). [[nodiscard]] std::string issue(const SessionToken& claims) const { std::string json; - // `SessionToken` is a flat aggregate, so writing it into a `std::string` - // cannot fail — the result is unconditional. - (void)glz::write_json(claims, json); + detail::throwOnGlazeError(glz::write(claims, json), json); const std::string payload = detail::base64UrlEncode(json); const std::string sig = detail::base64UrlEncode(_mac(_secret, payload)); return payload + "." + sig; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 41ae7a35..30a8d20b 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -79,6 +79,7 @@ add_executable(morph_tests test_policy_hardening.cpp test_register_authorization.cpp test_register_session.cpp + test_control_byte_escaping.cpp test_opaque_model_ids.cpp test_graceful_shutdown.cpp test_pinned_facts.cpp diff --git a/tests/test_control_byte_escaping.cpp b/tests/test_control_byte_escaping.cpp new file mode 100644 index 00000000..45836b03 --- /dev/null +++ b/tests/test_control_byte_escaping.cpp @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Reproduces and verifies the fix for #62: three sibling writers on +// caller-supplied strings never got the control-byte escaping fix that +// morph::wire::encode already applies via morph::wire::detail::EscapingWriteOpts +// (see docs/spec/session/session.md and docs/spec/security.md): +// - morph::journal::toJson(LogEntry) (journal/action_log.hpp) +// - morph::offline::detail::toJson(FileQueueRecord) (offline/file_offline_queue.hpp) +// - morph::session::TokenIssuer::issue(SessionToken) (session/session_auth.hpp) +// +// glaze 7.4 leaves ASCII control bytes (0x00-0x1F) unescaped by default, which +// produces invalid JSON (RFC 8259 requires them escaped) that a later +// glz::read_json either fails outright or, worse, silently corrupts when the +// same string also contains a `\` or `"` earlier (glaze's chunked writer path +// rewrites a control byte as two 0x00 bytes in that case). + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace { + +// A function, not a namespace-scope object, matching test_wire_hardening.cpp's +// own rationale: a static-storage std::string could throw before main(). +std::string ctl() { return std::string("a") + static_cast(0x0B) + "b"; } + +std::filesystem::path tempQueuePath() { + static std::atomic counter{0}; + auto const now = std::chrono::steady_clock::now().time_since_epoch().count(); + return std::filesystem::temp_directory_path() / + ("morph_control_byte_escaping_test_" + std::to_string(now) + "_" + std::to_string(++counter) + ".ndjson"); +} + +} // namespace + +// ── journal::LogEntry ──────────────────────────────────────────────────────── + +TEST_CASE("journal::toJson escapes control bytes in LogEntry string fields so fromJson round-trips", + "[control_byte_escaping][journal]") { + morph::journal::LogEntry entry{ + .seq = 1, + .modelType = "M", + .entityKey = ctl(), + .actionType = "A", + .payload = ctl(), + .result = {}, + .outcome = morph::journal::Outcome::Failed, + .error = ctl(), + .principal = ctl(), + .timestampMs = 0, + }; + std::string json; + REQUIRE_NOTHROW(json = morph::journal::toJson(entry)); + + morph::journal::LogEntry back; + REQUIRE_NOTHROW(back = morph::journal::fromJson(json)); + CHECK(back.entityKey == ctl()); + CHECK(back.payload == ctl()); + CHECK(back.error == ctl()); + CHECK(back.principal == ctl()); +} + +TEST_CASE("journal::toJson escapes a control byte alongside an escaped character without corrupting it", + "[control_byte_escaping][journal]") { + // Regression guard for glaze's corrupting fast path (see test_wire_hardening.cpp's + // identical case for wire::encode): a `\` earlier in the same string sends + // the unescaped writer down a path that mangles a later control byte into + // two 0x00 bytes instead of merely producing invalid JSON. + std::string payload = "\\x"; + payload.push_back(static_cast(0x0B)); + payload += "\"tail"; + + morph::journal::LogEntry entry{ + .seq = 1, .modelType = "M", .entityKey = {}, .actionType = "A", .payload = payload, .result = {}, + }; + std::string json; + REQUIRE_NOTHROW(json = morph::journal::toJson(entry)); + + morph::journal::LogEntry back; + REQUIRE_NOTHROW(back = morph::journal::fromJson(json)); + CHECK(back.payload == payload); +} + +// ── offline::FileOfflineQueue (FileQueueRecord) ───────────────────────────── + +TEST_CASE("FileOfflineQueue: a control byte in payload survives being written and reopened", + "[control_byte_escaping][file_queue]") { + auto path = tempQueuePath(); + std::filesystem::remove(path); + std::string payload = ctl(); + { + morph::offline::FileOfflineQueue queue{path}; + // A second, ordinary item after the control-byte one so the + // control-byte line is NOT the trailing line — FileOfflineQueue + // tolerates (skips) a malformed *trailing* line as a torn-write + // heuristic, but rethrows on any earlier malformed line, so this is + // the shape that actually exercises the bug rather than the + // torn-line tolerance. + queue.enqueue(payload); + queue.enqueue("second"); + } // close the file handle before reopening/removing -- required on Windows + + std::vector items; + { + morph::offline::FileOfflineQueue reopened{path}; + items = reopened.drain(); + } // close the file handle before removing -- required on Windows + REQUIRE(items.size() == 2); + CHECK(items[0].payload == payload); + CHECK(items[1].payload == "second"); + + std::filesystem::remove(path); +} + +// ── session::TokenIssuer ───────────────────────────────────────────────────── + +TEST_CASE("TokenIssuer::issue escapes control bytes in principal/roles so the token verifies", + "[control_byte_escaping][session_auth]") { + const std::string secret = "ctl-byte-secret"; + const morph::session::TokenIssuer issuer{secret}; + const morph::session::TokenVerifier verifier{secret}; + + const morph::session::SessionToken claims{ + .principal = ctl(), + .issuedAtMs = 0, + .expiresAtMs = 9'999'999'999'999, + .roles = {ctl()}, + }; + std::string token; + REQUIRE_NOTHROW(token = issuer.issue(claims)); + + const auto verified = verifier.verify(token, 1000); + REQUIRE(verified.has_value()); + CHECK(verified->principal == ctl()); + REQUIRE(verified->roles.size() == 1); + CHECK(verified->roles[0] == ctl()); +} From 018fa4aac0d0d7b46abe22bad4266d9eff376c3c Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 12 Aug 2026 08:40:58 +0300 Subject: [PATCH 3/6] docs: add missing @brief on the three EscapingWriteOpts::escape_control_characters members Doxygen's WARN_AS_ERROR=FAIL_ON_WARNINGS gate flagged the two of these three duplicated structs it happened to reach as undocumented (journal and session; the offline-queue copy has the identical gap, fixed here too for consistency). Matches the @brief already present on the original in core/wire.hpp that these three deliberately mirror. Co-Authored-By: Claude Sonnet 5 --- include/morph/journal/action_log.hpp | 1 + include/morph/offline/file_offline_queue.hpp | 1 + include/morph/session/session_auth.hpp | 1 + 3 files changed, 3 insertions(+) diff --git a/include/morph/journal/action_log.hpp b/include/morph/journal/action_log.hpp index c73b8ebb..bae6699f 100644 --- a/include/morph/journal/action_log.hpp +++ b/include/morph/journal/action_log.hpp @@ -148,6 +148,7 @@ inline void throwOnGlazeError(const glz::error_ctx& errCode, std::string_view co /// that only want the journal. Escaping is lossless, so any such byte still /// round-trips through `fromJson` unchanged. struct EscapingWriteOpts : glz::opts { + /// @brief Emit control bytes as `\\uXXXX` rather than raw. // NOLINTNEXTLINE(readability-identifier-naming) — glaze's option name, matched by name. bool escape_control_characters = true; }; diff --git a/include/morph/offline/file_offline_queue.hpp b/include/morph/offline/file_offline_queue.hpp index d2a80d96..7b39fc66 100644 --- a/include/morph/offline/file_offline_queue.hpp +++ b/include/morph/offline/file_offline_queue.hpp @@ -71,6 +71,7 @@ inline void throwOnGlazeError(const glz::error_ctx& errCode, std::string_view co /// a `core/` dependency. Escaping is lossless, so any such byte still /// round-trips through `fromJson` unchanged. struct EscapingWriteOpts : glz::opts { + /// @brief Emit control bytes as `\\uXXXX` rather than raw. // NOLINTNEXTLINE(readability-identifier-naming) — glaze's option name, matched by name. bool escape_control_characters = true; }; diff --git a/include/morph/session/session_auth.hpp b/include/morph/session/session_auth.hpp index cbcdf53c..f81d1888 100644 --- a/include/morph/session/session_auth.hpp +++ b/include/morph/session/session_auth.hpp @@ -352,6 +352,7 @@ inline void throwOnGlazeError(const glz::error_ctx& errCode, std::string_view co /// `core/` dependency. Escaping is lossless, so any such byte still /// round-trips through `TokenVerifier::verify` unchanged. struct EscapingWriteOpts : glz::opts { + /// @brief Emit control bytes as `\\uXXXX` rather than raw. // NOLINTNEXTLINE(readability-identifier-naming) — glaze's option name, matched by name. bool escape_control_characters = true; }; From 1e5dd5aef59d404ee1f9a91038bd28d23044166d Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 12 Aug 2026 08:51:04 +0300 Subject: [PATCH 4/6] test: fix -Wmissing-designated-field-initializers on SessionToken/LogEntry literals Both structs gained fields (roles on SessionToken; outcome, error, principal, timestampMs, idempotencyKey on LogEntry) at various points in their history, and this branch's new test files built designated initializers against the earlier, shorter field lists. MSVC doesn't warn on this; Linux clang-debug's strict -Werror, -Wmissing-designated-field-initializers does. List every field explicitly at each call site. Co-Authored-By: Claude Sonnet 5 --- tests/test_control_byte_escaping.cpp | 12 +++++++++++- tests/test_register_session.cpp | 10 ++++++---- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/tests/test_control_byte_escaping.cpp b/tests/test_control_byte_escaping.cpp index 45836b03..5ccc7548 100644 --- a/tests/test_control_byte_escaping.cpp +++ b/tests/test_control_byte_escaping.cpp @@ -78,7 +78,17 @@ TEST_CASE("journal::toJson escapes a control byte alongside an escaped character payload += "\"tail"; morph::journal::LogEntry entry{ - .seq = 1, .modelType = "M", .entityKey = {}, .actionType = "A", .payload = payload, .result = {}, + .seq = 1, + .modelType = "M", + .entityKey = {}, + .actionType = "A", + .payload = payload, + .result = {}, + .outcome = morph::journal::Outcome::Succeeded, + .error = {}, + .principal = {}, + .timestampMs = 0, + .idempotencyKey = {}, }; std::string json; REQUIRE_NOTHROW(json = morph::journal::toJson(entry)); diff --git a/tests/test_register_session.cpp b/tests/test_register_session.cpp index 67c4f3df..8a79735b 100644 --- a/tests/test_register_session.cpp +++ b/tests/test_register_session.cpp @@ -62,7 +62,8 @@ TEST_CASE("Bridge::setDefaultSession's token reaches the register envelope so au morph::session::Context session; session.principal = "alice"; - session.token = morph::session::TokenIssuer{secret}.issue({.principal = "alice", .expiresAtMs = 9999999999999}); + session.token = morph::session::TokenIssuer{secret}.issue( + {.principal = "alice", .issuedAtMs = 0, .expiresAtMs = 9999999999999, .roles = {}}); bridge.setDefaultSession(session); // Before the fix, registerModelWithContext (called from BridgeHandler's @@ -106,8 +107,8 @@ TEST_CASE("register's recorded owner principal is the Bridge's authenticated def morph::bridge::Bridge aliceBridge{std::make_unique(*server)}; morph::session::Context aliceSession; aliceSession.principal = "alice"; - aliceSession.token = - morph::session::TokenIssuer{secret}.issue({.principal = "alice", .expiresAtMs = 9999999999999}); + aliceSession.token = morph::session::TokenIssuer{secret}.issue( + {.principal = "alice", .issuedAtMs = 0, .expiresAtMs = 9999999999999, .roles = {}}); aliceBridge.setDefaultSession(aliceSession); // Registers on behalf of "alice" — before the fix, the owner recorded on @@ -130,7 +131,8 @@ TEST_CASE("register's recorded owner principal is the Bridge's authenticated def morph::bridge::Bridge bobBridge{std::make_unique(*server)}; morph::session::Context bobSession; bobSession.principal = "bob"; - bobSession.token = morph::session::TokenIssuer{secret}.issue({.principal = "bob", .expiresAtMs = 9999999999999}); + bobSession.token = morph::session::TokenIssuer{secret}.issue( + {.principal = "bob", .issuedAtMs = 0, .expiresAtMs = 9999999999999, .roles = {}}); bobBridge.setDefaultSession(bobSession); // Build a raw envelope targeting alice's modelId directly (bypassing a From b4c595940a1cb170a21055cf6abe45f2752af076 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 12 Aug 2026 21:25:31 +0300 Subject: [PATCH 5/6] docs: reword a test comment to state the current behavior, not history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comments should describe the present implementation and its rationale, not narrate what a fix changed — once merged, that framing has no context to anchor to. Co-Authored-By: Claude Sonnet 5 --- tests/test_control_byte_escaping.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_control_byte_escaping.cpp b/tests/test_control_byte_escaping.cpp index 5ccc7548..c934b0b1 100644 --- a/tests/test_control_byte_escaping.cpp +++ b/tests/test_control_byte_escaping.cpp @@ -110,8 +110,8 @@ TEST_CASE("FileOfflineQueue: a control byte in payload survives being written an // A second, ordinary item after the control-byte one so the // control-byte line is NOT the trailing line — FileOfflineQueue // tolerates (skips) a malformed *trailing* line as a torn-write - // heuristic, but rethrows on any earlier malformed line, so this is - // the shape that actually exercises the bug rather than the + // heuristic, but rethrows on any earlier malformed line, so this + // shape exercises the escaping path itself rather than the // torn-line tolerance. queue.enqueue(payload); queue.enqueue("second"); From 8c2c4da02839975b7d2e0d05e6cf669b23076772 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Wed, 12 Aug 2026 21:37:15 +0300 Subject: [PATCH 6/6] docs: reword session-propagation comments to state current behavior, not history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docs/spec/core/backend.md, docs/spec/session/session.md, include/morph/core/backend.hpp, and two test file headers/comments narrated the pre-fix state ('before IBackend::setSession existed', 'was always empty', 'reproduces and verifies the fix for #63') instead of describing the current design. Rewritten to state only present-tense facts and their rationale — once merged, 'before the fix' framing has no context to anchor to. Co-Authored-By: Claude Sonnet 5 --- docs/spec/core/backend.md | 14 ++++++------ docs/spec/session/session.md | 12 +++++----- include/morph/core/backend.hpp | 12 +++++----- tests/test_control_byte_escaping.cpp | 8 +++---- tests/test_register_session.cpp | 33 ++++++++++++++-------------- 5 files changed, 40 insertions(+), 39 deletions(-) diff --git a/docs/spec/core/backend.md b/docs/spec/core/backend.md index 94174556..614f977c 100644 --- a/docs/spec/core/backend.md +++ b/docs/spec/core/backend.md @@ -125,13 +125,13 @@ always carry the current session. Control messages — `register`, `registerShared` (`registerModelShared`), `attach` (`attachModel`), `assign` (`assignPrimary`), and `deregister` (`deregisterModel`) — are different: each is built directly inside the concrete backend, which has no other route to -the `Bridge`'s session. Before `IBackend::setSession` existed, every one of -these envelopes carried a default-constructed (empty, unauthenticated) -`session::Context` regardless of what `Bridge::setDefaultSession()` held, so -`RemoteServer::authorizeRegister` could never see a caller's identity and the -owner principal it records at `register` time was always empty — degrading -`IAuthorizer::authorizeInstance`'s ownership check to allow-all for every -instance a `Bridge` registered (see [session.md](../session/session.md)). +the `Bridge`'s session except `IBackend::setSession`. Every wire-backed +implementation stamps the session `setSession` last installed onto these +envelopes too, so `RemoteServer::authorizeRegister` sees the caller's +identity and the owner principal it records at `register` time reflects the +registering session — which is what `IAuthorizer::authorizeInstance`'s +ownership check relies on for every instance a `Bridge` registers (see +[session.md](../session/session.md)). `Bridge` calls `IBackend::setSession` in two places: once from its constructor (with the just-constructed, typically empty, default session) and diff --git a/docs/spec/session/session.md b/docs/spec/session/session.md index a646301d..850e173b 100644 --- a/docs/spec/session/session.md +++ b/docs/spec/session/session.md @@ -81,16 +81,16 @@ envelopes, built once per call by `Bridge::executeVia`. Control envelopes — `register`, `registerShared`, `attach`, `assign`, `deregister` — are built directly inside the concrete backend (`registerModelWithContext`, `registerModelShared`, `attachModel`, `assignPrimary`, `deregisterModel`), -which has no other route to the `Bridge`'s session. `IBackend::setSession` -closes this: `Bridge` calls it on construction and on every +which has no other route to the `Bridge`'s session except +`IBackend::setSession`: `Bridge` calls it on construction and on every `setDefaultSession()`, and on the incoming backend during `switchBackend()` (before any control envelope is built to re-register handlers), so a wire-backed backend (`SimulatedRemoteBackend`, `SocketBackend`, `QtWebSocketBackend`) always has the current session on hand to stamp onto -these envelopes too. Without this, `RemoteServer::authorizeRegister` could -never see a caller's identity and the owner principal recorded at `register` -time was always empty, degrading `authorizeInstance`'s ownership check to -allow-all (see [backend.md](../core/backend.md)'s "Session propagation to +these envelopes too. This is what lets `RemoteServer::authorizeRegister` see +the caller's identity and the owner principal recorded at `register` time +reflect the registering session, which `authorizeInstance`'s ownership check +relies on (see [backend.md](../core/backend.md)'s "Session propagation to control envelopes"). `LocalBackend` does not override `setSession`: the local path never serialises a `Context` onto a wire envelope in the first place. diff --git a/include/morph/core/backend.hpp b/include/morph/core/backend.hpp index c4b9236f..65a731a0 100644 --- a/include/morph/core/backend.hpp +++ b/include/morph/core/backend.hpp @@ -399,12 +399,12 @@ struct IBackend { /// envelopes regardless of this hook. Control messages are different: they /// are built directly by the concrete backend (`registerModelWithContext`, /// `registerModelShared`, `attachModel`, `assignPrimary`, `deregisterModel`), - /// which has no other way to learn the `Bridge`'s current session. Without - /// this hook those envelopes always carried a default-constructed (empty, - /// unauthenticated) `session::Context`, so `RemoteServer::authorizeRegister` - /// could never see a caller's identity and the owner principal recorded at - /// `register` time was always empty — degrading `authorizeInstance`'s - /// ownership check to allow-all for every instance a `Bridge` registered. + /// which has no other way to learn the `Bridge`'s current session except + /// this hook. Stamping the stored session onto those envelopes is what lets + /// `RemoteServer::authorizeRegister` see a caller's identity and the owner + /// principal recorded at `register` time reflect the registering session — + /// which `authorizeInstance`'s ownership check relies on for every instance + /// a `Bridge` registers. /// /// `Bridge::setDefaultSession()` calls this immediately, and /// `Bridge::switchBackend()` calls it on the new backend before any diff --git a/tests/test_control_byte_escaping.cpp b/tests/test_control_byte_escaping.cpp index c934b0b1..a21c139e 100644 --- a/tests/test_control_byte_escaping.cpp +++ b/tests/test_control_byte_escaping.cpp @@ -1,9 +1,9 @@ // SPDX-License-Identifier: Apache-2.0 // -// Reproduces and verifies the fix for #62: three sibling writers on -// caller-supplied strings never got the control-byte escaping fix that -// morph::wire::encode already applies via morph::wire::detail::EscapingWriteOpts -// (see docs/spec/session/session.md and docs/spec/security.md): +// Verifies that three sibling writers on caller-supplied strings apply the +// same control-byte escaping morph::wire::encode applies via +// morph::wire::detail::EscapingWriteOpts (see docs/spec/session/session.md +// and docs/spec/security.md): // - morph::journal::toJson(LogEntry) (journal/action_log.hpp) // - morph::offline::detail::toJson(FileQueueRecord) (offline/file_offline_queue.hpp) // - morph::session::TokenIssuer::issue(SessionToken) (session/session_auth.hpp) diff --git a/tests/test_register_session.cpp b/tests/test_register_session.cpp index 8a79735b..1b2c68b0 100644 --- a/tests/test_register_session.cpp +++ b/tests/test_register_session.cpp @@ -1,12 +1,13 @@ // SPDX-License-Identifier: Apache-2.0 // -// Reproduces and verifies the fix for #63: register/attach/assign/deregister -// envelopes built by the wire-backed IBackend implementations (SimulatedRemoteBackend, -// SocketBackend) never carried Bridge::defaultSession(), so a RemoteServer whose -// authorizer requires authentication rejected every register — and, even under an -// allow-all authorizer, the recorded owner principal was always empty, degrading -// authorizeInstance's ownership check to allow-all. See docs/spec/session/session.md -// ("How a Context originates and flows") and docs/spec/core/backend.md ("IBackend"). +// The wire-backed IBackend implementations (SimulatedRemoteBackend, +// SocketBackend) stamp Bridge::defaultSession() onto every +// register/attach/assign/deregister envelope, so a RemoteServer whose +// authorizer requires authentication can allow register, and the recorded +// owner principal reflects the registering session rather than always being +// empty — which is what authorizeInstance's ownership check relies on. See +// docs/spec/session/session.md ("How a Context originates and flows") and +// docs/spec/core/backend.md ("IBackend"). #include #include @@ -66,10 +67,10 @@ TEST_CASE("Bridge::setDefaultSession's token reaches the register envelope so au {.principal = "alice", .issuedAtMs = 0, .expiresAtMs = 9999999999999, .roles = {}}); bridge.setDefaultSession(session); - // Before the fix, registerModelWithContext (called from BridgeHandler's - // constructor) sent a register envelope with a default-constructed - // (empty) session, so RegisterRequiresAuth::authorizeRegister denied it - // and this constructor threw "register failed: unauthorized". + // registerModelWithContext (called from BridgeHandler's constructor) + // stamps the Bridge's default session onto the register envelope, so + // RegisterRequiresAuth::authorizeRegister sees "alice" as the principal + // and allows it — this constructor does not throw. morph::bridge::BridgeHandler handler{bridge, &cbExec}; std::atomic result{-1}; @@ -111,9 +112,8 @@ TEST_CASE("register's recorded owner principal is the Bridge's authenticated def {.principal = "alice", .issuedAtMs = 0, .expiresAtMs = 9999999999999, .roles = {}}); aliceBridge.setDefaultSession(aliceSession); - // Registers on behalf of "alice" — before the fix, the owner recorded on - // the server was always empty (the envelope's session was never set), - // which degraded authorizeInstance's ownership check to allow-all. + // Registers on behalf of "alice" — the register envelope carries the + // Bridge's default session, so the server records "alice" as the owner. morph::bridge::BridgeHandler aliceHandler{aliceBridge, &cbExec}; std::atomic aliceResult{-1}; @@ -126,8 +126,9 @@ TEST_CASE("register's recorded owner principal is the Bridge's authenticated def REQUIRE(aliceResult.load() == 2); // Bob (a different, also-valid principal) attempts to execute against the - // *same* model id alice's handler was bound to. Before the fix this would - // succeed (owner was empty => allow-all); after the fix it is denied. + // *same* model id alice's handler was bound to. The recorded owner is + // "alice", so authorizeInstance denies Bob rather than falling back to + // allow-all. morph::bridge::Bridge bobBridge{std::make_unique(*server)}; morph::session::Context bobSession; bobSession.principal = "bob";