fix(s7commplus): accept unprefixed response values

CHANGES.md

@@ -6,6 +6,10 @@ CHANGES
 
 Major release: new `s7commplus` package with S7CommPlus protocol support.
 
+* Accept GetVarSubStreamed responses whose PValue immediately follows the
+  return value, as captured from an S7-1511C during SessionKey legitimation.
+* Encode legacy SessionKey absolute DB reads with one address ID so scalar
+  reads are not misinterpreted as nested symbolic paths by S7-1500 PLCs.
 * Correlate S7CommPlus responses by opcode, function, and sequence; discard
   bounded stale replies from earlier requests, preserve interleaved
   notifications, and serialize synchronous wire requests.

s7commplus/client.py

@@ -219,6 +219,16 @@ def db_read(self, db_number: int, start: int, size: int) -> bytes:
         if self._connection.requires_substreamed:
             return self._db_read_substreamed(db_number, start, size)
 
+        # Legacy SessionKey PLCs interpret every value after AccessSubArea as
+        # another nested address ID.  A raw range encoded as [offset, size]
+        # therefore addresses ``size`` below the scalar at ``offset`` and old
+        # S7-1500 firmware terminates the connection.  Compatible clients send
+        # one absolute ID and let the returned PValue carry the value's size.
+        if self._connection.session_key_active:
+            access_area = Ids.DB_ACCESS_AREA_BASE + (db_number & 0xFFFF)
+            data = self.read_symbolic(access_area, [start + 1])
+            return _fit_absolute_read(data, size, db_number, start)
+
         payload = _build_read_payload([(db_number, start, size)], self._connection.protocol_version)
         response = self._connection.send_request(FunctionCode.GET_MULTI_VARIABLES, payload)
         results = _parse_read_response(response)
@@ -298,6 +308,11 @@ def db_read_multi(self, items: list[tuple[int, int, int]]) -> list[bytes]:
         if self._connection.requires_substreamed:
             return [self._db_read_substreamed(db, start, size) for db, start, size in items]
 
+        if self._connection.session_key_active:
+            addresses = [(Ids.DB_ACCESS_AREA_BASE + (db_number & 0xFFFF), [start + 1]) for db_number, start, _size in items]
+            values = self.read_symbolic_multi(addresses)
+            return [_fit_absolute_read(value, size, db_number, start) for value, (db_number, start, size) in zip(values, items)]
+
         payload = _build_read_payload(items, self._connection.protocol_version)
         response = self._connection.send_request(FunctionCode.GET_MULTI_VARIABLES, payload)
         parsed = _parse_read_response(response)
@@ -868,6 +883,18 @@ def __exit__(self, *args: Any) -> None:
 _SCALAR_RESPONSE_SUFFIX = bytes.fromhex("000400000000")
 
 
+def _fit_absolute_read(data: bytes | None, size: int, db_number: int, start: int) -> bytes:
+    """Fit one typed absolute-address value to the ``db_read`` byte contract."""
+    if data is None:
+        raise RuntimeError(f"DB{db_number} offset {start} could not be read")
+    if len(data) < size:
+        raise RuntimeError(
+            f"DB{db_number} offset {start} returned {len(data)} bytes, fewer than the requested {size}; "
+            "use browse() and read_symbolic() when the range crosses variable boundaries"
+        )
+    return data[:size]
+
+
 def _build_read_payload(items: list[tuple[int, int, int]], protocol_version: int = ProtocolVersion.V2) -> bytes:
     """Build a GetMultiVariables request payload.
 

s7commplus/connection.py

@@ -47,6 +47,7 @@
 import tempfile
 import threading
 from collections import deque
+from collections.abc import Callable
 from types import TracebackType
 from typing import Any, Optional, Type
 
@@ -322,6 +323,23 @@ def _build_get_var_substreamed_payload(
     return payload
 
 
+def _response_pvalue_offset(payload: bytes, offset: int, supported: Callable[[int, int], bool]) -> int:
+    """Locate a response PValue with or without the legacy leading zero marker.
+
+    Captured PLC responses use both layouts. Prefer a PValue beginning directly
+    after ReturnValue, then accept the older zero-prefixed form emitted by the
+    emulator and present in earlier captures.
+    """
+    if offset + 2 <= len(payload) and supported(payload[offset], payload[offset + 1]):
+        return offset
+
+    if payload[offset : offset + 1] == b"\x00":
+        offset += 1
+    if offset + 2 > len(payload):
+        raise ValueError("missing PValue header")
+    return offset
+
+
 def _parse_get_var_substreamed_response(payload: bytes) -> bytes:
     """Extract the typed value from a GetVarSubStreamed response payload."""
     from snap7.error import S7ConnectionError
@@ -331,13 +349,11 @@ def _parse_get_var_substreamed_response(payload: bytes) -> bytes:
         if return_value != 0:
             raise S7ConnectionError(f"GetVarSubStreamed failed: return_value=0x{return_value:X}")
 
-        offset = consumed
-        if offset >= len(payload):
-            raise ValueError("missing response marker")
-        offset += 1  # protocol-defined unknown byte
-
-        if offset + 2 > len(payload):
-            raise ValueError("missing PValue header")
+        offset = _response_pvalue_offset(
+            payload,
+            consumed,
+            lambda flags, datatype: datatype == DataType.BLOB or (datatype == DataType.USINT and bool(flags & 0x10)),
+        )
         flags = payload[offset]
         datatype = payload[offset + 1]
         offset += 2
@@ -369,12 +385,11 @@ def _parse_protection_level_response(payload: bytes) -> int:
         if return_value != 0:
             raise S7ConnectionError(f"GetVarSubStreamed for the protection level failed: return_value={return_value}")
 
-        if offset >= len(payload):
-            raise ValueError("missing response marker")
-        offset += 1  # protocol-defined unknown byte
-
-        if offset + 2 > len(payload):
-            raise ValueError("missing PValue header")
+        offset = _response_pvalue_offset(
+            payload,
+            offset,
+            lambda flags, datatype: datatype == DataType.UDINT and not flags & 0x10,
+        )
         flags = payload[offset]
         datatype = payload[offset + 1]
         offset += 2
@@ -589,6 +604,11 @@ def session_setup_ok(self) -> bool:
         """Whether the session setup (ServerSessionVersion echo) succeeded."""
         return self._session_setup_ok
 
+    @property
+    def session_key_active(self) -> bool:
+        """Whether legacy SessionKey authentication protects application traffic."""
+        return self._session_key is not None
+
     @property
     def requires_substreamed(self) -> bool:
         """Whether data operations must use substreamed function codes.

s7commplus/server.py

@@ -945,6 +945,9 @@ def _handle_get_multi_variables(self, seq_num: int, session_id: int, request_dat
         for i, (db_num, byte_offset, byte_size) in enumerate(items, 1):
             db = self._data_blocks.get(db_num)
             if db is not None:
+                if byte_size == 0:
+                    variable = next((var for var in db.variables.values() if var.byte_offset == byte_offset), None)
+                    byte_size = variable.byte_size if variable is not None else 1
                 data = db.read(byte_offset, byte_size)
                 response += encode_uint32_vlq(i)  # ItemNumber
                 response += encode_pvalue_blob(data)  # Value as BLOB
@@ -1288,9 +1291,11 @@ def _server_parse_read_request(request_data: bytes) -> list[tuple[int, int, int]
         # Extract db_number from AccessArea
         db_num = access_area & 0xFFFF
 
-        # Extract byte offset and size from LIDs (LID offsets are 1-based)
+        # A second ID is the historical emulator's raw-range extension. Real
+        # PLC absolute addresses contain one 1-based ID; the returned PValue
+        # determines the scalar's size.
         byte_offset = (lids[0] - 1) if len(lids) > 0 else 0
-        byte_size = lids[1] if len(lids) > 1 else 1
+        byte_size = lids[1] if len(lids) > 1 else 0
 
         items.append((db_num, byte_offset, byte_size))
 

tests/test_s7_unit.py

@@ -615,6 +615,36 @@ def test_db_read_multi_not_connected(self) -> None:
         with pytest.raises(RuntimeError, match="Not connected"):
             client.db_read_multi([(1, 0, 4)])
 
+    def test_session_key_db_read_uses_one_absolute_address_id(self) -> None:
+        client = S7CommPlusClient()
+        connection = MagicMock(requires_substreamed=False, session_key_active=True)
+        client._connection = connection
+        client.read_symbolic = MagicMock(return_value=b"\x12\x34")
+
+        assert client.db_read(116, 708, 2) == b"\x12\x34"
+        client.read_symbolic.assert_called_once_with(Ids.DB_ACCESS_AREA_BASE + 116, [709])
+
+    def test_session_key_db_read_rejects_cross_variable_range(self) -> None:
+        client = S7CommPlusClient()
+        connection = MagicMock(requires_substreamed=False, session_key_active=True)
+        client._connection = connection
+        client.read_symbolic = MagicMock(return_value=b"\x12\x34")
+
+        with pytest.raises(RuntimeError, match="crosses variable boundaries"):
+            client.db_read(116, 708, 4)
+
+    def test_session_key_db_read_multi_preserves_item_count(self) -> None:
+        client = S7CommPlusClient()
+        connection = MagicMock(requires_substreamed=False, session_key_active=True)
+        client._connection = connection
+        client.read_symbolic_multi = MagicMock(return_value=[b"\x01\x02", None])
+
+        with pytest.raises(RuntimeError, match="DB2 offset 10 could not be read"):
+            client.db_read_multi([(1, 0, 2), (2, 10, 1)])
+        client.read_symbolic_multi.assert_called_once_with(
+            [(Ids.DB_ACCESS_AREA_BASE + 1, [1]), (Ids.DB_ACCESS_AREA_BASE + 2, [11])]
+        )
+
     def test_db_write_multi_not_connected(self) -> None:
         client = S7CommPlusClient()
         with pytest.raises(RuntimeError, match="Not connected"):

tests/test_s7_v2.py

@@ -905,6 +905,12 @@ def test_parse_get_var_substreamed_usint_array(self) -> None:
 
         assert _parse_get_var_substreamed_response(response) == challenge
 
+    def test_parse_get_var_substreamed_usint_array_without_legacy_marker(self) -> None:
+        """Accept the response captured from the S7-1511C in GH-872."""
+        response = bytes.fromhex("00100214ac214373925d0fc9a8ef730c908fcaa2863f8abe0400000000")
+
+        assert _parse_get_var_substreamed_response(response) == bytes.fromhex("ac214373925d0fc9a8ef730c908fcaa2863f8abe")
+
     def test_parse_get_var_substreamed_blob(self) -> None:
         challenge = bytes(range(16))
         response = bytes([0x00, 0x00, 0x00, DataType.BLOB, 0x00])
@@ -1185,12 +1191,17 @@ class TestProtectionLevel:
     def test_parse_scalar_udint(self) -> None:
         assert _parse_protection_level_response(self.RESPONSE) == AccessLevel.NO_ACCESS
 
+    def test_parse_scalar_udint_without_legacy_marker(self) -> None:
+        response = bytes.fromhex("000004040700000000")
+
+        assert _parse_protection_level_response(response) == AccessLevel.NO_ACCESS
+
     def test_parse_rejects_nonzero_return(self) -> None:
         with pytest.raises(S7ConnectionError, match="return_value=4660"):
             _parse_protection_level_response(encode_uint32_vlq(0x1234))
 
-    def test_parse_rejects_missing_response_marker(self) -> None:
-        with pytest.raises(S7ConnectionError, match="missing response marker"):
+    def test_parse_rejects_missing_pvalue(self) -> None:
+        with pytest.raises(S7ConnectionError, match="missing PValue header"):
             _parse_protection_level_response(bytes([0x00]))
 
     def test_parse_rejects_truncated_pvalue_header(self) -> None: