From 9cb4d80b7fa0214b863e9dec9a2cd331ca432cf1 Mon Sep 17 00:00:00 2001 From: Jarry Shaw Date: Wed, 28 Oct 2026 14:02:52 -0400 Subject: [PATCH] fix(ftp): reject a status code that is not three digits (#2416) FTP.make's code branch ran int(code) with no guard, so 'abc' raised a bare ValueError and [220] a bare TypeError, while True wrote code 001, 3.7 wrote 003, 1000 wrote b'1000' and -5 wrote b'-05' -- the %03d formatting accepted out-of-range values silently. The sibling cmmd (#2290) and args (#2410) parameters on the same method already guard their types. Normalise and validate the code in a new FTP._make_code helper: a bool is refused, an int (a ReturnCode member included) is taken as it is, a numeric str, bytes or bytearray is converted, and anything else -- or a value outside 0-999 -- raises "FTP: invalid status code: must be an int in 0-999 or a numeric str, bytes or bytearray, not ...". The range is the three digits the wire format holds and the [0-9]{3} of FTP_RESPONSE reads back, not the 1xx-5xx of RFC 959 section 4.2 alone, since 6xx is RFC 2228 section 5 and the read path accepts it. Widened the code annotation to bytearray, which int() already converted, and extended the Args:/Raises: docstring entries. Test: tests/protocols/application/test_ftp_unit.py gains test_ftp_make_rejects_a_status_code_that_is_not_three_digits, pinning the exact message for bool, float, list, memoryview, a .value-carrying object, a non-numeric str and bytes, and 1000, -5, '1000' and b'-5', plus the unchanged build for an int, a ReturnCode member and a numeric str, bytes and bytearray, and the leading zeroes at 0 and 999. Confirmed fail-without-fix: with the guard reverted the test fails 12/12 -- four with the bare ValueError/TypeError and eight with "ProtocolError not raised". tests/protocols/ in full: 2035 passed / 51410 subtests / 0 failed, after regenerating examples/captures via examples/generators/make_samples.py. mypy reports nothing on ftp.py and pylint's findings on both touched files are unchanged in kind from main -- the extracted helper keeps make off too-many-branches. --- pcapkit/protocols/application/ftp.py | 59 ++++++++++++++++++-- tests/protocols/application/test_ftp_unit.py | 28 ++++++++++ 2 files changed, 81 insertions(+), 6 deletions(-) diff --git a/pcapkit/protocols/application/ftp.py b/pcapkit/protocols/application/ftp.py index 37e42b1c4..ef955bd08 100644 --- a/pcapkit/protocols/application/ftp.py +++ b/pcapkit/protocols/application/ftp.py @@ -173,7 +173,7 @@ def read(self, length: 'Optional[int]' = None, **kwargs: 'Any') -> 'Data_FTP': def make(self, cmmd: 'Optional[Enum_Command | str | bytes | bytearray]' = None, - code: 'Optional[Enum_ReturnCode | int | str | bytes]' = None, + code: 'Optional[Enum_ReturnCode | int | str | bytes | bytearray]' = None, args: 'Optional[str | bytes | bytearray]' = None, more: 'bool' = False, charset: 'Optional[str]' = None, @@ -195,7 +195,10 @@ def make(self, cmmd: FTP command, a :class:`str` (an :class:`~pcapkit.const.ftp.command.Command` member is one), :class:`bytes` or :class:`bytearray`. - code: FTP status code. + code: FTP status code, an :class:`int` in ``0``--``999`` (an + :class:`~pcapkit.const.ftp.return_code.ReturnCode` member is + one) or a numeric :class:`str`, :class:`bytes` or + :class:`bytearray` holding one. args: Optional FTP command arguments and/or status messages, a :class:`str`, :class:`bytes` or :class:`bytearray`. more: More status messages to follow for response packets. @@ -213,8 +216,11 @@ def make(self, ProtocolError: If neither or both of ``cmmd`` and ``code`` are given, if ``cmmd`` or ``args`` is not a :class:`str`, :class:`bytes` or :class:`bytearray` (a :obj:`bool` included), - or if ``lines`` are given other than as the rest of a - multi-line reply. + if ``code`` is not an :class:`int` in ``0``--``999`` (a + :obj:`bool` included) nor a numeric :class:`str`, + :class:`bytes` or :class:`bytearray` holding one, or if + ``lines`` are given other than as the rest of a multi-line + reply. """ if cmmd is not None and code is None: @@ -231,8 +237,7 @@ def make(self, mf = b'' elif cmmd is None and code is not None: - code_val = int(code) - prefix = b'%03d' % code_val # reply codes are three digits, so keep a leading zero + prefix = self._make_code(code) mf = b'-' if more else b'' else: @@ -286,6 +291,48 @@ def _make_data(cls, data: 'Data_FTP') -> 'dict[str, Any]': # type: ignore[overr 'lines': getattr(data, 'lines', ()), } + @staticmethod + def _make_code(code: 'Enum_ReturnCode | int | str | bytes | bytearray') -> 'bytes': + """Render a reply code as the three digits a reply line opens with. + + The three digits hold ``0``--``999``, which is what the ``[0-9]{3}`` + of ``FTP_RESPONSE`` reads back, so that is the range accepted rather + than the 1xx--5xx of :rfc:`959#section-4.2` alone -- 6xx is + :rfc:`2228#section-5`, and the read path accepts it. + + Args: + code: Reply code, an :class:`int` (a + :class:`~pcapkit.const.ftp.return_code.ReturnCode` member is + one) or a numeric :class:`str`, :class:`bytes` or + :class:`bytearray` holding one. + + Returns: + The code as three digits, a leading zero kept. + + Raises: + ProtocolError: If ``code`` is of any other type, a :obj:`bool` + included, or does not fit in three digits. + + """ + # NOTE: ``ReturnCode`` is an ``IntEnum``, so a member takes the ``int`` + # branch. ``bool`` is an ``int`` subclass, so without the explicit check + # ``True`` would write code 001, and ``int()`` alone would raise a bare + # ValueError or TypeError rather than a ProtocolError (#2416). + value = None # type: Optional[int] + if isinstance(code, bool): + pass + elif isinstance(code, int): + value = code + elif isinstance(code, (str, bytes, bytearray)): + try: + value = int(code) + except ValueError: + pass + if value is None or not 0 <= value <= 999: + raise ProtocolError('FTP: invalid status code: must be an int in 0-999 or a ' + f'numeric str, bytes or bytearray, not {code!r}') + return b'%03d' % value + @staticmethod def _test_lines(code: 'bytes', lines: 'tuple[bytes, ...]') -> 'bool': """Whether ``lines`` are the rest of a multi-line reply with ``code``. diff --git a/tests/protocols/application/test_ftp_unit.py b/tests/protocols/application/test_ftp_unit.py index f784923f3..c0437a54c 100644 --- a/tests/protocols/application/test_ftp_unit.py +++ b/tests/protocols/application/test_ftp_unit.py @@ -236,6 +236,34 @@ def test_ftp_make_rejects_arguments_that_are_not_str_bytes_or_bytearray(self) -> self.assertEqual(ftp.make(code=230, args=bytearray(b'Logged in')).data, b'230 Logged in\r\n') + def test_ftp_make_rejects_a_status_code_that_is_not_three_digits(self) -> None: + """A code of any other type, or out of ``0``--``999``, raises (#2416).""" + from pcapkit.const.ftp.return_code import ReturnCode + from pcapkit.protocols.application.ftp import FTP + from pcapkit.utilities.exceptions import ProtocolError + + ftp = object.__new__(FTP) + + for code in (True, False, 3.7, [220], memoryview(b'230'), + SimpleNamespace(value=220), 'abc', b'abc', 1000, -5, '1000', b'-5'): + with self.subTest(code=code): + with self.assertRaises(ProtocolError) as context: + ftp.make(code=code, args='Ready') + self.assertEqual( + str(context.exception), + 'FTP: invalid status code: must be an int in 0-999 or a numeric ' + f'str, bytes or bytearray, not {code!r}') + + # an int, a member and a numeric str, bytes or bytearray all still build + for code in (230, ReturnCode.CODE_230, '230', b'230', bytearray(b'230')): + with self.subTest(code=code): + self.assertEqual(ftp.make(code=code, args='Logged in').data, + b'230 Logged in\r\n') + + # the three digits keep their leading zeroes at either end of the range + self.assertEqual(ftp.make(code=0).data, b'000\r\n') + self.assertEqual(ftp.make(code=999).data, b'999\r\n') + def test_ftp_make_data_preserves_request_and_response_fields(self) -> None: from pcapkit.const.ftp.command import Command from pcapkit.const.ftp.return_code import ReturnCode