feat(remotefile): reject an unsafe server-chosen listing name (#1238)

messagefoundry/transports/remotefile.py

@@ -94,6 +94,37 @@
 _T = TypeVar("_T")
 
 
+def _is_contained_name(name: object) -> bool:
+    """True if ``name`` is a single, safe path component — a listing entry we may join onto the
+    configured remote directory (BACKLOG #1238, ASVS 5.3.2).
+
+    The name comes from a **remote server's own directory listing**, so it is chosen by a party we do
+    not control: a malicious or compromised partner could return ``../../etc/passwd.hl7``, and the
+    default ``pattern`` of ``*.hl7`` matches it (``fnmatch``'s ``*`` spans ``/``, unlike ``glob``).
+
+    **Reject, never rewrite.** ``posixpath.basename`` is the obvious fix and is actively harmful:
+    it MUTATES rather than refuses, so ``../../etc/adt_20260812.hl7`` becomes ``adt_20260812.hl7``,
+    which rejoins to a *real* file in the poll directory — handing a hostile server a retrieve /
+    move / delete primitive against the partner's own drop directory. It is also a no-op on the
+    Windows-separator form (``posixpath`` tokenizes only ``/``), and because its output can never
+    contain ``/`` any containment check placed after it is unreachable and always passes. For a
+    healthcare feed, refusing one file is strictly better than silently reading the wrong one, and
+    mutation cannot give that property.
+
+    Refused: a non-``str``; empty; ``.`` and ``..``; any ``/`` or ``\\``; a control character; and the
+    drive-relative form (``C:x.hl7``), which carries no separator at all and so slips a
+    separator-only check."""
+    if not isinstance(name, str) or not name or name in (".", ".."):
+        return False
+    if "/" in name or "\\" in name:
+        return False
+    if any(ord(ch) < 0x20 or ord(ch) == 0x7F for ch in name):
+        return False
+    # A drive-relative path ("C:x.hl7") resolves against the drive's CWD on Windows and contains no
+    # separator, so the checks above cannot see it. Two chars, ASCII letter, then a colon.
+    return not (len(name) >= 2 and name[0].isascii() and name[0].isalpha() and name[1] == ":")
+
+
 def _redact(host: str, path: str) -> str:
     """``host:path`` only — never credentials, for a log line."""
     return f"{host}:{path}"
@@ -909,7 +940,7 @@
             )
         return False
 
     async def _poll_once(self) -> None:
         import fnmatch
 
         assert self._handler is not None
@@ -920,6 +951,22 @@
         for name, size in sorted(entries):
             if self._stop.is_set():
                 break  # shutting down — leave the rest for the next start (at-least-once)
+            if not _is_contained_name(name):
+                # #1238 (ASVS 5.3.2): the server chose this name. Refuse it HERE — at the source,
+                # before the pattern filter — because the raw name reaches at least four consumers
+                # (the retrieve path, the error/oversize move, the after_read disposition, and the
+                # leave-mode dedup key), and a per-consumer check would keep missing one: the
+                # consumer set grew at every measurement pass and never shrank.
+                # NOT quarantined: moving it would join the hostile name onto a directory, which is
+                # the very operation being refused. Left in place and logged, so an operator sees it
+                # every poll rather than once. PHI-safe: names are not logged at INFO+ elsewhere in
+                # this source, so this stays WARNING-with-no-name — the count is the signal.
+                logger.warning(
+                    "REMOTEFILE %s: a listing entry was refused as an unsafe path component "
+                    "(not a single safe name); left in place, not retrieved",
+                    _redact(self._host, self._remote_dir),
+                )
+                continue
             if not fnmatch.fnmatch(name, self._pattern):
                 continue
             path = posixpath.join(self._remote_dir, name)

tests/test_remotefile_transport.py

@@ -39,6 +39,7 @@
     RemoteFileSource,
     _FtpClient,
     _ftps_ssl_context,
+    _is_contained_name,
     _RemoteClient,
     _RemoteError,
     _SftpClient,
@@ -1144,3 +1145,82 @@ async def test_remote_destination_validate_directory_test_probe_never_creates(
     with pytest.raises(DeliveryError):
         await dest.test_connection()
     assert client.dirs == []
+
+
+# --- #1238 (ASVS 5.3.2): a server-chosen listing name is REJECTED, never rewritten ---------------
+
+
+class _HostileListingClient(_FakeClient):
+    """A client whose listing returns names the SERVER chose, verbatim.
+
+    ``_FakeClient.list_dir`` derives names with ``posixpath.basename`` off its ``files`` keys, so it
+    structurally cannot produce a traversal name -- it would sanitize the very input under test. This
+    subclass returns the raw listing instead, which is what a hostile partner server does.
+    """
+
+    def __init__(self, names: list[str], **kw: Any) -> None:
+        super().__init__(**kw)
+        self._names = names
+
+    def list_dir(self, remote_dir: str) -> list[tuple[str, int]]:
+        return [(n, 10) for n in self._names]
+
+
+@pytest.mark.parametrize(
+    "name",
+    [
+        "../../etc/passwd.hl7",  # traversal, and the default *.hl7 pattern MATCHES it
+        "/etc/passwd.hl7",  # absolute
+        "..\..\etc\passwd.hl7",  # Windows separators -- posixpath.basename is a NO-OP on this
+        "sub/dir.hl7",  # a subdirectory component
+        ".",
+        "..",
+        "",
+        "a\x00b.hl7",  # NUL
+        "a\nb.hl7",  # newline
+        "C:evil.hl7",  # drive-relative: NO separator at all, so a separator-only check misses it
+    ],
+)
+def test_unsafe_listing_names_are_refused(name: str) -> None:  # #1238
+    assert _is_contained_name(name) is False
+
+
+@pytest.mark.parametrize(
+    "name",
+    ["a.hl7", "adt_20260812.hl7", "A-1.2_3.hl7", "file with spaces.hl7", "unicode-\u00e9.hl7"],
+)
+def test_legitimate_listing_names_are_accepted(name: str) -> None:  # #1238
+    # The refusal must not be so broad that it rejects ordinary partner filenames -- a check that
+    # refuses everything is not a control either.
+    assert _is_contained_name(name) is True
+
+
+async def test_traversal_entry_is_never_retrieved(monkeypatch: pytest.MonkeyPatch) -> None:  # #1238
+    """The whole point: a hostile listing entry reaches NO consumer.
+
+    Asserted on the client's recorded ops, not on the handler alone, because the raw name reaches at
+    least four consumers (retrieve, the error/oversize move, the after_read disposition, and the
+    leave-mode dedup key). Checking only "the handler was not called" would pass even if the engine
+    had already moved or deleted at the hostile path.
+    """
+    client = _HostileListingClient(["../../etc/passwd.hl7"])
+    src = _src(monkeypatch, client)
+    h = _RecordingHandler()
+    src._handler = h
+    await src._poll_once()
+    assert h.bodies == []  # nothing ingested
+    assert client.ops == []  # and NOTHING was retrieved, moved, renamed or removed
+
+
+async def test_a_safe_entry_beside_a_hostile_one_still_flows(
+    monkeypatch: pytest.MonkeyPatch,
+) -> None:  # #1238
+    # Refusing one entry must not abort the poll -- the legitimate file beside it is still delivered.
+    # Without this, a hostile server could suppress a real feed by planting one bad name.
+    client = _HostileListingClient(["../../etc/passwd.hl7", "good.hl7"])
+    client.files["/in/good.hl7"] = b"MSH|^~\&|A"
+    src = _src(monkeypatch, client)
+    h = _RecordingHandler()
+    src._handler = h
+    await src._poll_once()
+    assert h.bodies == [b"MSH|^~\&|A"]