diff --git a/docs/spec/core/registry.md b/docs/spec/core/registry.md index 9693067b..fb750078 100644 --- a/docs/spec/core/registry.md +++ b/docs/spec/core/registry.md @@ -304,6 +304,32 @@ struct ActionLogPolicy { When `coalesce` is `true`, a checkpoint keeps only the most recent entry per `(modelType, entityKey, actionType)` triple. +### `fromJson` is the codec boundary for an action payload + +`morph::wire` carries an execute envelope's `body` as an opaque `std::string` +and never parses it — `wire.hpp` states this directly ("payload smuggled +*inside* `body` is invisible to any structural/depth check"). So +`ActionTraits::fromJson` is the first and only place the body's contents are +decoded into typed fields, which makes it the layer responsible for what a +malformed payload means. + +That matters for values whose decode **cannot fail**. +[`morph::math::Rational`](../util/rational.md) is the case in point: `setWire` +clamps what it cannot represent rather than rejecting, so +`{"num":5,"den":0,"dp":2}` would otherwise arrive as a perfectly plausible +`5/1`. A model's own `validate()` runs *after* the decode and has nothing left +to notice — the value looks fine by then. + +`fromJson` therefore wraps its `glz::read` in a `morph::math::WireClampScope` +and throws `ParseError` if anything was clamped. `Rational` reports the fact; +this layer decides it is a protocol violation, because this is the layer that +knows the bytes came off a wire. A local caller constructing the same value in +code is unaffected. + +Note that a *non-canonical but representable* value is accepted: `4/8` reduces +to `1/2`, and reduction is canonicalisation, not clamping — the value survives +intact. + ## Type-erased holders and factory ### `IModelHolder` diff --git a/docs/spec/util/rational.md b/docs/spec/util/rational.md index b2e2222e..cb2ce980 100644 --- a/docs/spec/util/rational.md +++ b/docs/spec/util/rational.md @@ -77,9 +77,10 @@ floating-point input, overflow during decimal scaling) return | `one(p)` | `static constexpr Rational one(DecimalPlaces) noexcept` | `1/1` at the given precision. | **Wire path.** The Glaze deserialisation path (`setWire`) rebuilds through the -canonicalising constructor, silently clamping hostile input (`den == 0`, -out-of-range `dp`, `INT64_MIN` components whose negation would overflow) instead -of asserting. +canonicalising constructor, silently clamping what it cannot represent +(`den == 0`, out-of-range `dp`, a component whose magnitude does not fit) +instead of asserting, and counting the clamp for the decoding layer to act on. +See [Decoding cannot fail](#decoding-cannot-fail-so-the-clamp-is-reported-instead). ## Arithmetic @@ -331,6 +332,31 @@ route serialisation through the `Wire` struct. A `to_json_schema` specialisation preserves schema shape by delegating to `Wire`'s schema. The `glz::meta` also fixes the schema type name to `"Rational"`. +### Decoding cannot fail, so the clamp is reported instead + +`setWire` rebuilds through the canonicalising constructor, which clamps rather +than rejects. `{"num":5,"den":0,"dp":2}` therefore decodes to a plausible +`5/1`, and nothing downstream can tell the value was altered. + +`Rational` reports the fact and stops there. `Wire::validate()` is the +predicate — non-canonical but representable input (`4/8`, a negative `den`) is +**valid**, since reducing and sign-normalising round-trip the same value — and +`WireClampScope` counts clamps across a decode: + +```cpp +morph::math::WireClampScope clamps; +if (auto err = glz::read(action, json)) { ... } +if (clamps.clamped() != 0) { /* reject the payload */ } +``` + +Deciding what a clamp *means* is not this type's call. The same clamp is a +protocol violation when the bytes came off a socket and a harmless +normalisation when a local caller wrote them, and only the decoding layer +knows which. `ActionTraits::fromJson` is that layer for action payloads — +`morph::wire` carries an execute envelope's `body` as an opaque string and +never parses it, so `fromJson` is the first and only place a `Rational` inside +it is decoded — and it rejects a clamped payload with `ParseError`. + **Absent fields fall back to `Wire`'s member defaults.** A payload missing a key decodes to that field's default — `num = 0`, `den = 1`, `dp = 1` — so `"{}"` reads as canonical zero at precision 1 (`Rational::zero(dp1)`), overwriting @@ -364,7 +390,9 @@ through `setWire`. | `checkedAdd(a, b)` | `constexpr expected noexcept` — exact sum, or `Overflow`. | | `checkedSub(a, b)` | `constexpr expected noexcept` — exact difference, or `Overflow`. | | `checkedMul(a, b)` | `constexpr expected noexcept` — exact product, or `Overflow`. | -| `setWire(Wire)` | `void noexcept` — rebuilds through canonicalising constructor, clamping hostile input. | +| `setWire(Wire)` | `void noexcept` — rebuilds through the canonicalising constructor, clamping what it cannot represent and counting the clamp. | +| `Wire::validate()` | `constexpr bool noexcept` — whether these raw values decode without being clamped. | +| `WireClampScope` | Scoped observer: how many `Rational` values were clamped while decoding. | | `getWire()` | `Wire noexcept` — canonical members ready for JSON encoding. | | `struct Wire` | `{ int64_t num; int64_t den; uint32_t dp; }` — flat JSON representation. | diff --git a/include/morph/core/registry.hpp b/include/morph/core/registry.hpp index 8169bba9..f9d41b3e 100644 --- a/include/morph/core/registry.hpp +++ b/include/morph/core/registry.hpp @@ -5,6 +5,8 @@ #include #include #include +#include + #include #include #include @@ -686,9 +688,21 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio /* guaranteed trailing '\0' (e.g. an execute envelope's `body`) — see */ \ /* the identical fix + rationale on morph::wire::decode (wire.hpp). */ \ static constexpr glz::opts kLenientRead{.null_terminated = false, .error_on_unknown_keys = false}; \ + /* The codec boundary for an action payload: morph::wire carries `body` as \ + an opaque string and never parses it, so this is the first and only \ + place a Rational inside it is decoded. A Rational decode cannot fail -- \ + it clamps what it cannot represent -- so {"num":5,"den":0,"dp":2} would \ + otherwise arrive as a plausible 5/1 that no model-level validate() could \ + recognise as altered. Deciding that a silently-altered payload is a \ + protocol violation belongs here, where we know the bytes came off a wire. */ \ + ::morph::math::WireClampScope clampedRationals; \ if (auto errCode = glz::read(action, jsonStr)) { \ throw morph::model::detail::ParseError{glz::format_error(errCode, jsonStr)}; \ } \ + if (clampedRationals.clamped() != 0) { \ + throw morph::model::detail::ParseError{ \ + "action body contains a Rational that cannot be represented exactly"}; \ + } \ return action; \ } \ static std::string resultToJson(const Result& result) { \ @@ -746,9 +760,21 @@ bool registerActionExecutorOnce(std::string_view modelId, std::string_view actio /* guaranteed trailing '\0' (e.g. an execute envelope's `body`) — see */ \ /* the identical fix + rationale on morph::wire::decode (wire.hpp). */ \ static constexpr glz::opts kLenientRead{.null_terminated = false, .error_on_unknown_keys = false}; \ + /* The codec boundary for an action payload: morph::wire carries `body` as \ + an opaque string and never parses it, so this is the first and only \ + place a Rational inside it is decoded. A Rational decode cannot fail -- \ + it clamps what it cannot represent -- so {"num":5,"den":0,"dp":2} would \ + otherwise arrive as a plausible 5/1 that no model-level validate() could \ + recognise as altered. Deciding that a silently-altered payload is a \ + protocol violation belongs here, where we know the bytes came off a wire. */ \ + ::morph::math::WireClampScope clampedRationals; \ if (auto errCode = glz::read(action, jsonStr)) { \ throw morph::model::detail::ParseError{glz::format_error(errCode, jsonStr)}; \ } \ + if (clampedRationals.clamped() != 0) { \ + throw morph::model::detail::ParseError{ \ + "action body contains a Rational that cannot be represented exactly"}; \ + } \ return action; \ } \ static std::string resultToJson(const Result& result) { \ diff --git a/include/morph/util/rational.hpp b/include/morph/util/rational.hpp index 5ab61c57..925a2435 100644 --- a/include/morph/util/rational.hpp +++ b/include/morph/util/rational.hpp @@ -92,6 +92,7 @@ #include +#include #include #include #include @@ -101,8 +102,10 @@ #include #include #include +#include #include #include +#include namespace morph::math { @@ -168,6 +171,63 @@ enum class RationalError : std::uint8_t { namespace detail { +/// @brief Per-thread clamp counter, scoped by `WireClampScope`. +/// @return Reference to this thread's counter. +[[nodiscard]] inline std::size_t& wireClampCounter() noexcept { + static thread_local std::size_t clamped = 0; + return clamped; +} + +} // namespace detail + +/// @brief Observes whether any `Rational` had to be clamped while decoding +/// inside this scope. +/// +/// Decoding a `Rational` cannot fail. `setWire` rebuilds through the +/// canonicalising constructor, which clamps rather than rejects, so +/// `{"num":5,"den":0,"dp":2}` becomes a perfectly plausible `5/1` and nothing +/// downstream can tell the value was altered. +/// +/// This makes that observable at the point where it matters -- around a decode +/// -- without giving `Rational` an opinion about what should happen next. +/// Whether a clamped value is a protocol violation to reject or a harmless +/// normalisation depends on whether the caller is reading a trusted local +/// value or an untrusted payload off a socket, and only the decoding layer +/// knows which: +/// +/// @code +/// morph::math::WireClampScope clamps; +/// if (auto err = glz::read(action, json)) { ... } +/// if (clamps.clamped() != 0) { +/// // reject the payload +/// } +/// @endcode +/// +/// Thread-local and scoped: a decode is synchronous on one thread, and a +/// nested decode must not steal its parent's count. +class WireClampScope { + public: + /// @brief Starts a fresh count, saving any enclosing scope's. + WireClampScope() noexcept : _saved{detail::wireClampCounter()} { detail::wireClampCounter() = 0; } + + WireClampScope(const WireClampScope&) = delete; + WireClampScope& operator=(const WireClampScope&) = delete; + WireClampScope(WireClampScope&&) = delete; + WireClampScope& operator=(WireClampScope&&) = delete; + + /// @brief Folds this scope's count back into the enclosing one. + ~WireClampScope() { detail::wireClampCounter() += _saved; } + + /// @brief How many `Rational` values were clamped so far in this scope. + /// @return The clamp count. + [[nodiscard]] std::size_t clamped() const noexcept { return detail::wireClampCounter(); } + + private: + std::size_t _saved; +}; + +namespace detail { + /// @brief Clamps a raw precision into `[0, kMaxDecimalPlaces]` silently — for /// untrusted wire input. /// @@ -534,14 +594,48 @@ struct Rational { std::int64_t num{0}; ///< Signed numerator as sent/received. std::int64_t den{1}; ///< Denominator as sent/received; may be non-canonical. std::uint32_t dp{1}; ///< Decimal-precision tag as sent/received. + + /// @brief Whether these raw values decode without being clamped. + /// + /// Names the three clamps `setWire` would otherwise apply silently, so + /// "was this value altered on the way in?" is answerable *before* the + /// canonicalising constructor has already hidden the answer. A caller + /// that has its own decoded `Wire` can ask directly; the wire codec + /// asks on its behalf, and a `WireClampScope` around the decode acts on it. + /// + /// Note that a non-canonical but representable denominator (`4/8`, or + /// a negative `den`) is *valid*: reducing and sign-normalising it is + /// canonicalisation, not clamping, and round-trips the same value. + /// + /// @return `true` if the values survive decoding unaltered in magnitude. + [[nodiscard]] constexpr bool validate() const noexcept { + // `std::in_range` on the magnitude, rather than `!= INT64_MIN`: + // the actual requirement is that the component can be negated, + // which is to say its magnitude is representable as int64. That is + // what canonicalising needs, and it says so directly. + return den != 0 && dp <= kMaxDecimalPlaces && std::in_range(detail::absU64(num)) + && std::in_range(detail::absU64(den)); + } }; /// @brief Wire-codec entry (Glaze read side): rebuilds through the - /// canonicalising constructor, silently clamping hostile input - /// (`den == 0`, out-of-range `dp`, `INT64_MIN` components whose - /// negation would overflow) instead of asserting. + /// canonicalising constructor, clamping what it cannot represent. + /// + /// `den == 0`, an out-of-range `dp`, or a component whose magnitude does + /// not fit are clamped rather than rejected, so this never fails — which + /// means `{"num":5,"den":0,"dp":2}` decodes to a perfectly plausible + /// `5/1`, and nothing downstream can tell the value was altered. + /// + /// Rejecting here would be the wrong layer's call. Whether a clamped value + /// is a protocol violation or a harmless normalisation depends on where + /// the bytes came from, and this function cannot know. It records the fact + /// instead, for a `WireClampScope` around the decode to act on. + /// /// @param wire Raw values decoded from JSON. -void setWire(Wire wire) noexcept { + void setWire(Wire wire) noexcept { + if (!wire.validate()) { + ++detail::wireClampCounter(); + } constexpr auto int64Min = std::numeric_limits::min(); constexpr auto negatableMin = -std::numeric_limits::max(); *this = Rational{Numerator{wire.num == int64Min ? negatableMin : wire.num}, diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 8d6668e4..07469868 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -35,6 +35,7 @@ add_executable(morph_tests test_remote_step_interleaving.cpp test_remote_execute_ordering.cpp test_action_validation.cpp + test_action_wire_rejection.cpp test_security_fixes.cpp test_bridge_lifetime.cpp test_client_execute_deadline.cpp @@ -69,6 +70,7 @@ add_executable(morph_tests test_journal_format_versioning.cpp test_outbox.cpp test_rational_checked.cpp + test_rational_wire_policy.cpp test_rational.cpp test_ledger_rational_fuzz.cpp test_quantity.cpp diff --git a/tests/test_action_wire_rejection.cpp b/tests/test_action_wire_rejection.cpp new file mode 100644 index 00000000..b970e48c --- /dev/null +++ b/tests/test_action_wire_rejection.cpp @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// The decode boundary rejects an action payload carrying a Rational that +// cannot be represented. +// +// morph::wire carries an execute envelope's `body` as an opaque string and +// never parses it (wire.hpp says so: "payload smuggled *inside* `body` is +// invisible to any structural/depth check"). ActionTraits::fromJson is +// therefore the first and only place a Rational inside that body is decoded -- +// which makes it the layer that has to decide what a clamped value means, +// because it is the layer that knows the bytes came off a wire. + +#include +#include +#include + +#include + +#include + +// External linkage, deliberately: glaze's reflection needs it -- a type in an +// anonymous namespace fails with "used but not defined in this translation +// unit, and cannot be defined in any other because its type does not have +// linkage". +struct WireAmountAction { + morph::math::Rational amount; +}; + +struct WireAmountResult { + std::int64_t numerator = 0; +}; + +struct WireAmountModel { + WireAmountResult execute(const WireAmountAction& action) { + return WireAmountResult{.numerator = action.amount.numerator}; + } +}; + +BRIDGE_REGISTER_MODEL(WireAmountModel, "Test_WireRejection_Model") +BRIDGE_REGISTER_ACTION(WireAmountModel, WireAmountAction, "Test_WireRejection_Action") + +TEST_CASE("A well-formed Rational in an action body decodes normally", "[registry][wire]") { + const auto action = + morph::model::ActionTraits::fromJson(R"({"amount":{"num":5,"den":2,"dp":2}})"); + CHECK(action.amount.numerator == 5); + CHECK(action.amount.denominator == 2); +} + +TEST_CASE("A non-canonical but representable Rational is accepted, not rejected", "[registry][wire]") { + // 4/8 reduces to 1/2. Reduction is canonicalisation, not clamping: the + // value survives intact, so the payload is legitimate. + const auto action = + morph::model::ActionTraits::fromJson(R"({"amount":{"num":4,"den":8,"dp":2}})"); + CHECK(action.amount.numerator == 1); + CHECK(action.amount.denominator == 2); +} + +TEST_CASE("A zero denominator is rejected at the decode boundary", "[registry][wire]") { + // Previously this decoded to a perfectly plausible 5/1 and travelled on. + // The model's own validate() could not have caught it -- by then there is + // nothing to see. + CHECK_THROWS_AS( + morph::model::ActionTraits::fromJson(R"({"amount":{"num":5,"den":0,"dp":2}})"), + morph::model::detail::ParseError); +} + +TEST_CASE("An out-of-range precision is rejected at the decode boundary", "[registry][wire]") { + CHECK_THROWS_AS( + morph::model::ActionTraits::fromJson(R"({"amount":{"num":5,"den":2,"dp":99}})"), + morph::model::detail::ParseError); +} + +TEST_CASE("A component whose magnitude is not representable is rejected", "[registry][wire]") { + CHECK_THROWS_AS(morph::model::ActionTraits::fromJson( + R"({"amount":{"num":-9223372036854775808,"den":2,"dp":2}})"), + morph::model::detail::ParseError); +} + +TEST_CASE("Rejection does not leak into the next decode", "[registry][wire]") { + CHECK_THROWS_AS( + morph::model::ActionTraits::fromJson(R"({"amount":{"num":5,"den":0,"dp":2}})"), + morph::model::detail::ParseError); + + // The clamp count is scoped to one decode; a rejected payload must not + // poison the payload after it. + const auto action = + morph::model::ActionTraits::fromJson(R"({"amount":{"num":7,"den":2,"dp":2}})"); + CHECK(action.amount.numerator == 7); +} diff --git a/tests/test_rational_wire_policy.cpp b/tests/test_rational_wire_policy.cpp new file mode 100644 index 00000000..0a398436 --- /dev/null +++ b/tests/test_rational_wire_policy.cpp @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// Rational's wire round-trip, and the clamp fact it reports to whoever is +// decoding. Deciding what a clamp *means* is the decoding layer's job, not +// this type's -- see test_action_wire_rejection.cpp for that half. + +#include + +#include + +#include + +#include +#include + +using morph::math::DecimalPlaces; +using morph::math::Denominator; +using morph::math::Numerator; +using morph::math::Rational; +using morph::math::WireClampScope; + +namespace { + +constexpr auto kMax = std::numeric_limits::max(); + +[[nodiscard]] std::string encode(const Rational& value) { + std::string out; + REQUIRE_FALSE(glz::write_json(value, out)); + return out; +} + +[[nodiscard]] Rational decode(const std::string& json) { + Rational value; + REQUIRE_FALSE(glz::read_json(value, json)); + return value; +} + +} // namespace + +TEST_CASE("A valid Rational round-trips through its own formatter unchanged", + "[rational][wire]") { + // The property the type owes on its own: whatever it can represent, it can + // write and read back identically. + const Rational values[] = { + Rational{Numerator{0}, Denominator{1}, DecimalPlaces{0}}, + Rational{Numerator{5}, Denominator{2}, DecimalPlaces{2}}, + Rational{Numerator{-5}, Denominator{2}, DecimalPlaces{2}}, + Rational{Numerator{1}, Denominator{3}, DecimalPlaces{18}}, + Rational{Numerator{kMax}, Denominator{1}, DecimalPlaces{2}}, + Rational{Numerator{-kMax}, Denominator{kMax}, DecimalPlaces{9}}, + }; + + for (const auto& original : values) { + const auto reread = decode(encode(original)); + INFO("round-tripping " << original.numerator << "/" << original.denominator); + CHECK(reread == original); + CHECK(reread.numerator == original.numerator); + CHECK(reread.denominator == original.denominator); + CHECK(reread.decimalPlaces.value == original.decimalPlaces.value); + // And re-encoding is byte-identical, so the round trip is a fixed point. + CHECK(encode(reread) == encode(original)); + } +} + +TEST_CASE("Round-tripping a valid value reports no clamp", "[rational][wire]") { + const WireClampScope clamps; + const auto reread = decode(encode(Rational{Numerator{4}, Denominator{8}, DecimalPlaces{2}})); + + // 4/8 canonicalises to 1/2 -- that is reduction, not clamping, and it + // round-trips the same value. + CHECK(reread.numerator == 1); + CHECK(reread.denominator == 2); + CHECK(clamps.clamped() == 0); +} + +TEST_CASE("Wire::validate names exactly the values that cannot be represented", + "[rational][wire]") { + constexpr auto int64Min = std::numeric_limits::min(); + + CHECK(Rational::Wire{.num = 5, .den = 2, .dp = 2}.validate()); + CHECK(Rational::Wire{.num = 4, .den = 8, .dp = 2}.validate()); // non-canonical, representable + CHECK(Rational::Wire{.num = 1, .den = -2, .dp = 2}.validate()); // negative den, representable + + CHECK_FALSE(Rational::Wire{.num = 5, .den = 0, .dp = 2}.validate()); + CHECK_FALSE(Rational::Wire{.num = 5, .den = 2, .dp = morph::math::kMaxDecimalPlaces + 1}.validate()); + CHECK_FALSE(Rational::Wire{.num = int64Min, .den = 2, .dp = 2}.validate()); + CHECK_FALSE(Rational::Wire{.num = 5, .den = int64Min, .dp = 2}.validate()); +} + +TEST_CASE("Decoding an unrepresentable value still succeeds, and says so", "[rational][wire]") { + // Decoding cannot fail -- that is the point. The value looks entirely + // plausible afterwards, which is why the clamp has to be reported rather + // than left for a downstream validate() that has nothing to notice. + const WireClampScope clamps; + const auto value = decode(R"({"num":5,"den":0,"dp":2})"); + + CHECK(value.numerator == 5); + CHECK(value.denominator == 1); + CHECK(clamps.clamped() == 1); +} + +TEST_CASE("Clamp counts nest without stealing the enclosing scope's", "[rational][wire]") { + const WireClampScope outer; + CHECK(outer.clamped() == 0); + { + const WireClampScope inner; + (void)decode(R"({"num":5,"den":0,"dp":2})"); + CHECK(inner.clamped() == 1); + } + // The inner scope folds its count back into the outer one rather than + // discarding it: a nested decode's clamp is still this decode's clamp. + CHECK(outer.clamped() == 1); +}