From 19e4cdfdf9044b691d89218a85378095dd87a4e9 Mon Sep 17 00:00:00 2001 From: Jack Nagy Date: Sun, 23 Aug 2026 09:58:23 +0100 Subject: [PATCH] fix(protocol): return only the error body when a Block2 transfer fails Block2Accumulator.add_response extended the accumulated representation with a non-success response's body before completing, so a 4.xx or 5.xx arriving mid-transfer produced one buffer holding assembled blocks followed by a diagnostic in the server's own format. get() returns (code, payload) with no boundary marker between the two, leaving the caller no way to split them. The error body now replaces the partial representation. Nothing observed the old behaviour: every caller gates on 2.05 before decoding, so the accumulated bytes had no reader once the code was not 2.xx. Where the error arrives before any block, this is unchanged from #36, which already returned the diagnostic where the pre-#36 path returned an empty buffer. This makes that the behaviour in every case. The payload bound moves with it, from the combined length to the error body's own, since the buffer now only ever holds one or the other. --- smartthings_local/protocol/coap.py | 16 +++++---- tests/test_coap_wire.py | 45 +++++++++++++++++++++++-- tests/test_dtls_session_reader_death.py | 4 +-- 3 files changed, 55 insertions(+), 10 deletions(-) diff --git a/smartthings_local/protocol/coap.py b/smartthings_local/protocol/coap.py index f716190..b384fc0 100644 --- a/smartthings_local/protocol/coap.py +++ b/smartthings_local/protocol/coap.py @@ -490,15 +490,19 @@ def add_response(self, message): if message.code == 0: raise BlockwiseError() - # A non-success response terminates the logical GET immediately while - # preserving the connected-session contract of returning bytes already - # accumulated before and in the error response. + # A non-success response terminates the logical GET. Its body is a + # diagnostic in the server's own format rather than a continuation of + # the representation, so it replaces whatever blocks arrived instead + # of extending them: get() returns (code, payload) with no boundary + # marker between the two, so concatenating leaves the caller one + # buffer holding two content types and no way to split it. Callers + # gate on 2.05 before decoding, so the accumulated bytes have no + # reader once the code is not 2.xx. if message.code >> 5 != 2: - if len(self._payload) + len(message.payload) > \ - self._max_payload_bytes: + if len(message.payload) > self._max_payload_bytes: raise BlockwiseError() self._code = message.code - self._payload.extend(message.payload) + self._payload = bytearray(message.payload) self._blocks_received += 1 self._complete = True return BLOCK2_COMPLETE diff --git a/tests/test_coap_wire.py b/tests/test_coap_wire.py index f6eba6a..a0e097e 100644 --- a/tests/test_coap_wire.py +++ b/tests/test_coap_wire.py @@ -434,7 +434,7 @@ def test_block2_continuation_requires_an_explicit_block2_option(): )) -def test_mid_transfer_error_preserves_connected_session_contract_by_default(): +def test_mid_transfer_error_replaces_the_partial_representation(): error = _message( code=0x80, payload=b'error', @@ -450,7 +450,20 @@ def test_mid_transfer_error_preserves_connected_session_contract_by_default(): )) assert accumulator.add_response(error) == BLOCK2_COMPLETE assert accumulator.code == 0x80 - assert accumulator.payload == b'a' * 16 + b'error' + assert accumulator.payload == b'error' + + +def test_error_before_any_block_returns_the_diagnostic_body(): + accumulator = Block2Accumulator(b'token') + assert accumulator.add_response(_message( + code=0x80, + payload=b'error', + include_block=False, + etag=None, + content_format=None, + )) == BLOCK2_COMPLETE + assert accumulator.code == 0x80 + assert accumulator.payload == b'error' def test_block2_accumulator_enforces_exact_block_and_payload_bounds(): @@ -499,6 +512,34 @@ def test_block2_accumulator_enforces_exact_block_and_payload_bounds(): content_format=None, )) + # An error body is bounded on its own length, since it replaces the + # accumulated blocks rather than extending them: a diagnostic that fits + # is accepted however many bytes arrived before it. + after_blocks = Block2Accumulator(b'token', max_payload_bytes=16) + after_blocks.add_response(_message( + number=0, + more=True, + payload=b'a' * 16, + )) + assert after_blocks.add_response(_message( + code=0x80, + payload=b'x' * 16, + include_block=False, + etag=None, + content_format=None, + )) == BLOCK2_COMPLETE + assert after_blocks.payload == b'x' * 16 + + oversized_error = Block2Accumulator(b'token', max_payload_bytes=16) + with pytest.raises(BlockwiseError): + oversized_error.add_response(_message( + code=0x80, + payload=b'x' * 17, + include_block=False, + etag=None, + content_format=None, + )) + def test_block2_accumulator_default_payload_bound_is_exactly_64_kib(): exact = Block2Accumulator(b'token') diff --git a/tests/test_dtls_session_reader_death.py b/tests/test_dtls_session_reader_death.py index 4f07570..1fe3bed 100644 --- a/tests/test_dtls_session_reader_death.py +++ b/tests/test_dtls_session_reader_death.py @@ -500,7 +500,7 @@ def wait_for_block(_event, per_wait): assert sess._pending_mids == {} -def test_get_preserves_mid_transfer_error_payload_contract(): +def test_get_returns_only_the_error_body_when_a_transfer_fails_mid_way(): sess = _make_session() def respond(datagram): @@ -527,7 +527,7 @@ def respond(datagram): sess._send_dgram = respond sess.pace = lambda: None - assert sess.get(['oic', 'res']) == (0x80, b'a' * 16 + b'error') + assert sess.get(['oic', 'res']) == (0x80, b'error') @pytest.mark.parametrize('method', ('get', 'post'))