feat(remotefile): reject an unsafe server-chosen listing name (#1238) - #376
Merged
wshallwshall merged 5 commits intoAug 13, 2026
Merged
Conversation
…G #1238)
RemoteFileSource joined a filename from a remote SFTP/FTP server's own directory
listing straight onto the configured remote_dir with no containment check. The
only filter was fnmatch against the pattern, and fnmatch's '*' spans '/' unlike
glob -- so the default '*.hl7' matches '../../etc/passwd.hl7'.
Screened at the SOURCE: immediately after list_dir returns and before the
pattern filter, so it dominates every consumer. That placement is the point.
The raw name reaches at least four consumers -- the retrieve path, the
error/oversize move, the after_read disposition (move, delete or the leave-mode
dedup key) -- and three of _move's four callers are error paths not gated on
after_read, so no configuration avoids them. A per-consumer check would keep
missing one: the consumer set grew at every measurement pass and never shrank.
REJECT, NEVER REWRITE -- the owner's ruling, and the reason is recorded in the
helper because the obvious fix is actively harmful. posixpath.basename MUTATES
rather than refuses, so '../../etc/adt_20260812.hl7' becomes a name that
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, and because its output can never
contain '/', any containment check placed after it is unreachable and always
passes.
Refused: non-str, empty, '.', '..', any '/' or '\', control characters, and the
drive-relative form ('C:x.hl7') which carries no separator at all and so slips
a separator-only check.
A refused entry is left in place and logged, NOT quarantined: moving it would
join the hostile name onto a directory, which is the operation being refused.
Logged without the name, since this source does not log filenames at INFO+.
MUTATION-VERIFIED, twice, because one mutation only proves the test sees the
null case: removing the check fails both new tests, and replacing it with
basename -- the plausible WRONG fix rather than the absent one -- fails the
traversal test. The check is proven to discriminate against the near miss.
Verified: ruff, ruff format, mypy strict (265 files) clean; the remotefile
suite 86 passed. The FULL suite was still running when this was committed and
is NOT part of this claim -- CI is the gate for it.
wshallwshall
enabled auto-merge (squash)
August 13, 2026 16:34
…-listing-name-containment
wshallwshall
deleted the
claude/builder-2-1238-listing-name-containment
branch
August 13, 2026 19:46
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds #1238 -- refuses an unsafe server-chosen listing name rather than sanitizing it.
2 files, +127 -0.
The fix is the opposite of the obvious one, deliberately
posixpath.basenameis the natural reach here and is actively harmful:poll directory -- handing a hostile server a retrieve/move/delete primitive against the partner's
own drop directory. The "sanitized" name is more dangerous than the rejected one.
check placed after it always passes -- a guard that looks like hardening while being structurally
incapable of firing.
All three reasons are in the helper's docstring and the commit message, so a reviewer meets them
before reaching for the simpler fix. That placement is the point: the next person to "simplify" this
reads the code, not the PR.
Mutation-verified twice
basenameOne mutation only proves the test sees the null case. The second proves it discriminates against the
plausible near-miss -- which is the mutation someone would actually make.
Verification
ruff,ruff format,mypy --strictclean. remotefile suite: 86 passed.Checked independently: base is a real ancestor of
main, branched fresh, one commit,merge-treeCLEAN, carries zero #1237 commits, and does not touch the #369 branch.
Scope caveat, volunteered by the authoring session rather than discovered -- NOW SATISFIED. At
commit time the full suite was still running, so they explicitly did not claim it and neither did I.
It has since finished green on the exact tree at
b254dddd: 12412 passed, 827 skipped, 0 failed(37m51s). Recorded here rather than left standing, because a caveat that quietly evaporates is worse
than one never raised -- a later reader finds a PR body warning about a suite nobody confirmed and
cannot tell whether it was checked or forgotten. CI remains the gate; the local run does not substitute
for it.