Collection of bug-fixes (#993)

* fix #948: missing return in IOU amount underflow path (#948)

  When an IOU value's normalized mantissa falls below MIN_IOU_MANTISSA
  or its exponent falls below MIN_IOU_EXPONENT, the serializer computed
  the canonical zero bytes but dropped the result on the floor. Execution
  fell through to the general serialization path, producing a non-zero
  amount for a value that should round to zero (e.g. "1e-82" serialized
  to c0405af3107a4000 instead of 8000000000000000).

  Add the missing `return` so underflowing IOU amounts serialize to the
  canonical zero representation defined in the XRPL binary format spec
  (type bit set, sign/exponent/mantissa all zero).

  Includes a regression test covering 1e-82, 1e-96, and -1e-96.

* fix: do not expose seed in Wallet init error message (#987)

  The XRPLAddressCodecException raised when a Wallet is constructed with
  an invalid seed embedded the raw seed in the message. Exception text is
  commonly logged or shipped to error-tracking systems, so this leaked
  secret material into places it should never reach.

  Remove the seed from the error message; keep the algorithm and the
  underlying decoder error for debuggability. Add a regression test that
  constructs a Wallet with a bogus seed and asserts the seed string is
  not present in str(exc).

* fix: redact secret fields in BaseModel __repr__ (#992)

  repr()/str() of Sign, SignFor, SignAndSubmit, and ChannelAuthorize
  included the raw `secret`, `seed`, `seed_hex`, and `passphrase` values
  because @DataClass auto-generated a __repr__ on each subclass that
  shadowed BaseModel's. That placed secret material anywhere object
  reprs land: logs, error-tracking systems (Sentry, Datadog), debugger
  output, and traceback frame locals.

  Fix centrally on BaseModel:
  - Define _SENSITIVE_FIELDS listing the redacted field names.
  - Install BaseModel.__repr__ via __init_subclass__ before @DataClass
    runs, so the decorator sees __repr__ already defined and skips
    auto-generating. Every BaseModel subclass now flows through the
    redacting repr with no per-class opt-in.
  - Rewrite BaseModel.__repr__ to iterate dataclasses.fields() and emit
    '***REDACTED***' for sensitive fields. to_dict() is unchanged, so the
    wire payload still carries real values.

* fix: use cryptographic RNG for WebSocket request IDs (#986)

  Request ID generation used random.randrange over a 1M-element keyspace.
  Mersenne Twister state is recoverable from observed outputs, so an
  attacker who can inject WebSocket frames (wss misconfiguration,
  TLS-intercepting proxy, relay between client and server) could predict
  upcoming IDs and race a forged response to resolve a pending await
  with attacker-controlled data — forged balances, fake tesSUCCESS on
  submit, bogus tx confirmations.

  The small keyspace also caused birthday collisions on long-lived
  connections around 1,177 requests, triggering spurious "already in
  progress" errors without an attacker.

  Switch to secrets.randbelow and widen _REQ_ID_MAX to 2**62 so both
  issues go away. IDs are embedded in a string so arbitrary Python
  ints are fine; no wire-format change.

* minor: linter fixes

* minor: fix mypy error

* minor: Use HIDDEN instead of REDACTED to denote sensitive field-values

* tests: address PR comments suggesting wider test coverage (points 4, 5)

* integ test: update docker command to  be consistent with recent updates to xrpld executable

* fix: skip malformed JSON frames in WebSocket handler (#977)

  A single malformed frame used to propagate json.JSONDecodeError out of
  _handler, killing the background task and silencing the client for the
  remainder of the connection. Wrap json.loads in try/except so malformed
  frames are dropped and the async-for loop continues.

  Also backfills CHANGELOG entries for this fix and for the earlier
  cryptographic RNG change (#986), both landing in websocket_base.py.

* minor: print warnings to stdout before dropping a malformed frame

* minor: address PR comments

* minor: remove traces of seed material in error msg

Author: ckeshava

CHANGELOG.md

@@ -16,6 +16,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Fixed correct mapping of `sfMutableFlags`, `sfStartDate`, and `sfPreviousPaymentDueDate` fields in the binary codec `definitions.json`.
 - Fixed `Amount` codec to correctly handle large integers with trailing zeros (precision is counted by significant digits, not total digits).
+- Fixed async WebSocket handler so a single malformed JSON frame is skipped instead of terminating the handler task and silencing the client for the remainder of the connection (issue #977).
+- Fixed WebSocket request-ID generation to use a cryptographic RNG (`secrets.randbelow`) and widened the ID range from `1_000_000` to `2**62`, making birthday-paradox collisions astronomically unlikely (expected collision after ~2**31 requests instead of ~1,177) (issue #986).
 
 ## [[4.5.0]]
 

tests/unit/asyn/clients/__init__.py

@@ -0,0 +1,87 @@
+"""Regression tests for issue #977: a malformed JSON frame must not kill
+the WebSocket handler task."""
+
+import asyncio
+import io
+import json
+from contextlib import redirect_stdout
+from typing import List
+from unittest import IsolatedAsyncioTestCase
+
+from xrpl.asyncio.clients.async_websocket_client import AsyncWebsocketClient
+
+
+class _FakeWebSocket:
+    """Minimal async-iterable stand-in for websockets.ClientConnection.
+
+    Yields each frame in ``frames`` and then terminates, which lets
+    ``_handler`` return normally (as it would when the server closes the
+    connection cleanly)."""
+
+    def __init__(self, frames: List[bytes]) -> None:
+        self._frames = frames
+
+    def __aiter__(self) -> "_FakeWebSocket":
+        self._iter = iter(self._frames)
+        return self
+
+    async def __anext__(self) -> bytes:
+        try:
+            return next(self._iter)
+        except StopIteration:
+            raise StopAsyncIteration
+
+
+class TestHandlerMalformedJson(IsolatedAsyncioTestCase):
+    async def test_handler_survives_malformed_frame(self) -> None:
+        """Send three frames: valid, malformed, valid. The handler must
+        enqueue both valid frames and simply skip the malformed one."""
+        frames = [
+            json.dumps({"id": "req_1", "result": "ok"}).encode(),
+            b"{ this is not valid json",
+            json.dumps({"id": "req_2", "result": "ok"}).encode(),
+        ]
+
+        ws = AsyncWebsocketClient("ws://test")
+        ws._websocket = _FakeWebSocket(frames)  # type: ignore[assignment]
+        ws._messages = asyncio.Queue()
+        ws._open_requests = {}
+
+        # Must not raise. Before the fix, json.JSONDecodeError on frame 2
+        # would propagate out and terminate the handler task.
+        await ws._handler()
+
+        # Both valid frames were enqueued; the malformed one was dropped.
+        enqueued = []
+        while not ws._messages.empty():
+            enqueued.append(ws._messages.get_nowait())
+        self.assertEqual(len(enqueued), 2)
+        self.assertEqual(enqueued[0], {"id": "req_1", "result": "ok"})
+        self.assertEqual(enqueued[1], {"id": "req_2", "result": "ok"})
+
+    async def test_handler_prints_malformed_frame(self) -> None:
+        """The skipped frame must be surfaced on stdout so the failure is
+        not silent."""
+        bad_frame = b"{ this is not valid json"
+        frames = [
+            json.dumps({"id": "req_1", "result": "ok"}).encode(),
+            bad_frame,
+            json.dumps({"id": "req_2", "result": "ok"}).encode(),
+        ]
+
+        ws = AsyncWebsocketClient("ws://test")
+        ws._websocket = _FakeWebSocket(frames)  # type: ignore[assignment]
+        ws._messages = asyncio.Queue()
+        ws._open_requests = {}
+
+        buf = io.StringIO()
+        with redirect_stdout(buf):
+            await ws._handler()
+        output = buf.getvalue()
+
+        # The malformed frame's repr must appear in stdout, and the two
+        # valid frames must not (only the bad one is logged).
+        self.assertIn(repr(bad_frame), output)
+        self.assertIn("malformed", output.lower())
+        self.assertNotIn("req_1", output)
+        self.assertNotIn("req_2", output)

tests/unit/asyn/clients/test_websocket_base.py

@@ -205,6 +205,36 @@ def test_from_value_issued_currency(self):
             amount_object = amount.Amount.from_value(json)
             self.assertEqual(amount_object.to_hex(), serialized)
 
+    def test_iou_underflow_rounds_to_zero(self):
+        """Regression test for issue #948.
+
+        A non-zero IOU value whose normalized mantissa falls below
+        MIN_IOU_MANTISSA (or whose exponent falls below MIN_IOU_EXPONENT)
+        must serialize to the canonical zero amount (only the "Not XRP"
+        bit set), and must round-trip back to "0"."""
+        issuer = "rDgZZ3wyprx4ZqrGQUkquE9Fs2Xs8XBcdw"
+        # 8-byte amount ("Not XRP" bit only) + 20-byte currency + 20-byte issuer.
+        # Canonical zero layout: only the type bit (bit 63) is set; sign,
+        # exponent, and mantissa are all 0. See:
+        # https://xrpl.org/docs/references/protocol/binary-format#token-amount-format
+        zero_amount_hex = "8000000000000000"
+        usd_currency_hex = "0000000000000000000000005553440000000000"
+        issuer_hex = "8B1CE810C13D6F337DAC85863B3D70265A24DF44"
+        zero_hex = zero_amount_hex + usd_currency_hex + issuer_hex
+        underflow_cases = ["1e-82", "1e-96", "-1e-96"]
+        for value in underflow_cases:
+            iou = {"value": value, "currency": "USD", "issuer": issuer}
+            amount_object = amount.Amount.from_value(iou)
+            self.assertEqual(
+                amount_object.to_hex(),
+                zero_hex,
+                f"IOU value {value!r} should serialize to canonical zero",
+            )
+            round_tripped = amount_object.to_json()
+            self.assertEqual(round_tripped["value"], "0")
+            self.assertEqual(round_tripped["currency"], "USD")
+            self.assertEqual(round_tripped["issuer"], issuer)
+
     def test_from_value_xrp(self):
         for json, serialized in XRP_CASES:
             amount_object = amount.Amount.from_value(json)

tests/unit/core/binarycodec/types/test_amount.py

@@ -80,3 +80,47 @@ def test_valid_seed(self):
             transaction=_TRANSACTION, seed=_SEED, key_type=CryptoAlgorithm.SECP256K1
         )
         self.assertTrue(request.is_valid())
+
+    def test_sensitive_fields_HIDDEN_in_repr(self):
+        """Regression test for issue #992: secret, seed, seed_hex, and
+        passphrase must never appear in repr() / str() output, since those
+        surfaces commonly feed logs and error-reporting pipelines. The raw
+        values must still round-trip through to_dict() so the RPC payload
+        is unchanged."""
+        for field, value in [
+            ("secret", _SECRET),
+            ("seed", _SEED),
+            ("seed_hex", _SEED_HEX),
+            ("passphrase", _PASSPHRASE),
+        ]:
+            request = Sign(transaction=_TRANSACTION, **{field: value})
+            self.assertNotIn(value, repr(request), f"{field} leaked via repr")
+            self.assertNotIn(value, str(request), f"{field} leaked via str")
+            self.assertIn("-HIDDEN-", repr(request))
+            self.assertIn("-HIDDEN-", str(request))
+            self.assertEqual(request.to_dict()[field], value)
+
+    def test_non_sensitive_fields_appear_in_repr(self):
+        """Redaction must not over-mask: ordinary fields must still appear in
+        repr() with their real values, and the overall shape must match the
+        standard dataclass format `ClassName(field=value, ...)`."""
+        request = Sign(
+            transaction=_TRANSACTION,
+            seed=_SEED,
+            key_type=CryptoAlgorithm.SECP256K1,
+            offline=True,
+            fee_mult_max=42,
+        )
+        rendered = repr(request)
+        self.assertTrue(rendered.startswith("Sign("))
+        self.assertTrue(rendered.endswith(")"))
+        # Non-sensitive scalar fields render with their real values
+        self.assertIn("offline=True", rendered)
+        self.assertIn("fee_mult_max=42", rendered)
+        self.assertIn("CryptoAlgorithm.SECP256K1", rendered)
+        # Nested transaction is rendered (not replaced by a placeholder)
+        self.assertIn("account='r9LqNeG6qHxjeUocjvVki2XR35weJ9mZgQ'", rendered)
+        self.assertIn("domain='asjcsodafsaid0f9asdfasdf'", rendered)
+        # None-valued sensitive fields are rendered as None, not -HIDDEN-
+        self.assertIn("secret=None", rendered)
+        self.assertIn("passphrase=None", rendered)

tests/unit/models/requests/test_sign.py

@@ -1,23 +1,32 @@
 import json
 import os
+from dataclasses import dataclass
 from unittest import TestCase
 
+from typing_extensions import Self
+
 from xrpl.models import XRPLModelException
 from xrpl.models.amounts import IssuedCurrencyAmount
+from xrpl.models.base_model import BaseModel
 from xrpl.models.currencies import XRP, IssuedCurrency
 from xrpl.models.requests import (
     AccountChannels,
     BookOffers,
+    ChannelAuthorize,
     PathFind,
     PathFindSubcommand,
     PathStep,
     Request,
     Sign,
+    SignAndSubmit,
+    SignFor,
     SubmitMultisigned,
     SubmitOnly,
 )
 from xrpl.models.requests.request import _DEFAULT_API_VERSION
 from xrpl.models.transactions import (
+    AccountSet,
+    AccountSetAsfFlag,
     AMMBid,
     AuthAccount,
     CheckCreate,
@@ -70,6 +79,51 @@ def test_repr(self):
         )
         self.assertEqual(repr(amount), expected_repr)
 
+    def test_init_subclass_preserves_subclass_defined_repr(self):
+        # A subclass that defines its own __repr__ must keep it. The
+        # __init_subclass__ hook installs the redacting __repr__ only when
+        # the subclass hasn't provided one; dropping that guard would
+        # silently overwrite any subclass-defined repr.
+        @dataclass(frozen=True)
+        class _CustomModel(BaseModel):
+            value: str = "x"
+
+            def __repr__(self: Self) -> str:
+                return "CUSTOM_REPR"
+
+        self.assertEqual(repr(_CustomModel(value="anything")), "CUSTOM_REPR")
+
+    def test_repr_redaction_applies_to_every_signing_request(self):
+        # The fix for issue #992 lives on BaseModel.__init_subclass__, so
+        # every subclass should inherit the redacting __repr__. test_sign.py
+        # only exercises Sign; this test proves the wiring reaches every
+        # secret-bearing request class. Protects against a future change
+        # (e.g. adding an explicit @dataclass(repr=True) on one subclass)
+        # that would force dataclass to regenerate __repr__ and silently
+        # reintroduce the leak on only that class.
+        transaction = AccountSet(
+            account="r9LqNeG6qHxjeUocjvVki2XR35weJ9mZgQ",
+            fee="0.00001",
+            set_flag=AccountSetAsfFlag.ASF_DISALLOW_XRP,
+            sequence=19048,
+        )
+        seed = "sEdTM1uX8pu2do5XvTnutH6HsouMaM2"
+        cases = [
+            Sign(transaction=transaction, seed=seed),
+            SignFor(
+                account="r9LqNeG6qHxjeUocjvVki2XR35weJ9mZgQ",
+                transaction=transaction,
+                seed=seed,
+            ),
+            SignAndSubmit(transaction=transaction, seed=seed),
+            ChannelAuthorize(channel_id="0" * 64, amount="1000", seed=seed),
+        ]
+        for request in cases:
+            with self.subTest(request_cls=type(request).__name__):
+                rendered = repr(request)
+                self.assertNotIn(seed, rendered)
+                self.assertIn("seed='-HIDDEN-'", rendered)
+
     def test_is_dict_of_model_when_true(self):
         self.assertTrue(
             IssuedCurrencyAmount.is_dict_of_model(

tests/unit/models/test_base_model.py

@@ -51,3 +51,37 @@ def test_init_ed25519_with_s_seed(self):
     def test_init_secp256k1_with_sEd_seed_fail(self):
         with self.assertRaises(XRPLAddressCodecException):
             Wallet.from_seed(SED_SEED, algorithm=CryptoAlgorithm.SECP256K1)
+
+    def test_invalid_seed_not_leaked_in_exception(self):
+        """Regression test for issue #987: exception messages for invalid
+        seeds must not include the raw seed, since they often get logged or
+        captured by error-tracking systems."""
+        secret_seed = "sInvalidSeedXXXXXXXXXXXXXX"
+        with self.assertRaises(XRPLAddressCodecException) as ctx:
+            Wallet(public_key="abc", private_key="def", seed=secret_seed)
+        self.assertNotIn(secret_seed, str(ctx.exception))
+
+    def test_invalid_seed_chars_not_leaked_via_base58_value_error(self):
+        """The XRPL base58 alphabet excludes '0', 'O', 'I', and 'l'. When a
+        seed contains any of these, ``base58.b58decode`` raises
+        ``ValueError("Invalid character '0' ...")`` with the offending byte
+        embedded in the message. ``Wallet.__init__`` must neither concatenate
+        ``str(e)`` into its own message nor leave the original exception
+        attached as ``__cause__`` / ``__context__``, since either path leaks
+        a character of the seed into logs and tracebacks.
+        """
+        # Every character below is in the base58 alphabet *except* '0', so
+        # any '0' in the message or chained cause could only have come from
+        # the seed.
+        seed = "sZZZZZZZZZZZZZZZZZZZZZZZZ0"
+        with self.assertRaises(XRPLAddressCodecException) as ctx:
+            Wallet(public_key="abc", private_key="def", seed=seed)
+
+        exc = ctx.exception
+        self.assertNotIn(seed, str(exc))
+        self.assertNotIn("0", str(exc))
+        # ``raise ... from None`` must suppress the chained cause so that
+        # the original ``ValueError("Invalid character '0'")`` can't reach
+        # full-traceback log sinks.
+        self.assertIsNone(exc.__cause__)
+        self.assertTrue(exc.__suppress_context__)

tests/unit/wallet/test_wallet.py

@@ -4,7 +4,7 @@
 
 import asyncio
 import json
-from random import randrange
+from secrets import randbelow
 from typing import TYPE_CHECKING, Any, Dict, Optional, cast
 
 from typing_extensions import Final, Self
@@ -18,7 +18,10 @@
 from xrpl.models.response import Response
 
 _PAYLOAD_MAX_SIZE: Final[int] = 2**24
-_REQ_ID_MAX: Final[int] = 1_000_000
+# Widened from 1_000_000 to 2**62 so birthday-paradox collisions are
+# astronomically unlikely (expected collision after ~2**31 requests instead
+# of ~1,177). IDs are embedded in a string, so arbitrary Python ints are fine.
+_REQ_ID_MAX: Final[int] = 2**62
 
 # the types from asyncio are not implemented as generics in python 3.8 and
 # lower, so we need to only subscript them when running typechecking.
@@ -47,7 +50,7 @@ def _inject_request_id(request: Request) -> Request:
     if request.id is not None:
         return request
     request_dict = request.to_dict()
-    request_dict["id"] = f"{request.method}_{randrange(_REQ_ID_MAX)}"
+    request_dict["id"] = f"{request.method}_{randbelow(_REQ_ID_MAX)}"
     return Request.from_dict(request_dict)
 
 
@@ -133,7 +136,16 @@ async def _handler(self: Self) -> None:
         As long as a given client remains open, this handler will be running as a Task.
         """
         async for response in cast(websocket_client.ClientConnection, self._websocket):
-            response_dict = json.loads(response)
+            try:
+                response_dict = json.loads(response)
+            except json.JSONDecodeError:
+                # Issue #977: a single malformed frame must not kill the
+                # handler task and leave the client deaf to every frame
+                # that follows. Surface the dropped frame so the failure is
+                # not silent.
+                # TODO: Introduce a module-level logging system across xrpl-py library
+                print(f"xrpl-py: skipping malformed WebSocket frame: {response!r}")
+                continue
 
             # if this response corresponds to request, fulfill the Future
             if "id" in response_dict and response_dict["id"] in self._open_requests:

xrpl/asyncio/clients/websocket_base.py

@@ -209,7 +209,7 @@ def _serialize_issued_currency_value(value: str) -> bytes:
 
     if exp < MIN_IOU_EXPONENT or mantissa < MIN_IOU_MANTISSA:
         # Round to zero
-        _ZERO_CURRENCY_AMOUNT_HEX.to_bytes(8, byteorder="big", signed=False)
+        return _ZERO_CURRENCY_AMOUNT_HEX.to_bytes(8, byteorder="big", signed=False)
 
     if exp > MAX_IOU_EXPONENT or mantissa > MAX_IOU_MANTISSA:
         raise XRPLBinaryCodecException(