fix: recognize host-level UNC roots in path containment checks - #1327
fix: recognize host-level UNC roots in path containment checks#1327crowecawcaw wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
waiting-on-maintainers
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| pytest.skip("no free drive letter to map the share onto") | ||
|
|
||
| try: | ||
| asset = Path(drive + "\\") / "mapped_probe.txt" |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| leading = len(rest) - len(rest.lstrip(separators)) | ||
| # POSIX gives '//' its own root spelling, but collapses three or more. | ||
| root_length = 2 if (leading == 2 and path_module.sep == "/") else 1 | ||
| return drive, rest[:root_length], rest[root_length:] |
There was a problem hiding this comment.
On Python 3.9–3.11 (ntpath.splitroot was added in 3.12, and requires-python is >=3.9), this backport diverges from the real splitroot for a share-less UNC host like \\server — the exact root form in issue #1321 (\\192.168.20.20).
ntpath.splitdrive("\\\\server") returns ("", "\\\\server") (no share ⇒ empty drive), so here drive="" and the backport produces root="\\" (a single backslash). _split_anchored then falls through to drive + root and yields anchor \ + ["server"].
The real 3.12 splitroot("\\\\server") instead returns ("\\\\server", "", "") — the host in the drive field — so _split_anchored takes the drive.startswith(_UNC_ANCHOR) branch and yields the UNC anchor \\ + ["server"].
A candidate such as \\server\share\file always has a share, so splitdrive puts \\server\share in the drive on every version, giving anchor \\. Comparing components then fails on 3.9–3.11:
candidate: ["\\\\", "server", "share", "file"] # UNC anchor "\\"
root: ["\\", "server"] # single "\" ← mismatch
So is_path_contained(r"\\server\share\file", r"\\server") returns False on 3.9–3.11, meaning the #1321 fix does not work there and the corresponding unit tests (e.g. (r"\\host\share\file", r"\\host", True)) would fail on those interpreters. Consider having the backport detect a share-less \\host and route the host into the drive field (or normalize the anchor) so it matches splitroot.
666fe0c to
a1c2c95
Compare
os.path.commonpath cannot compare a Windows host-level UNC path (\\server) with
a path under one of its shares: ntpath.splitdrive reports no drive for the former
and \\server\share for the latter, so it raises "Paths don't have the same
drive". It raises the same way for two shares of one host, and for a share-root
directory compared with its own files ("Can't mix absolute and relative paths").
_is_known_path caught that ValueError as "not contained", so every asset path
under a \\server known root was reported as unknown and submissions from a
Windows network share could not proceed without confirmation. Three other
containment checks did not catch it at all and surfaced a raw ValueError instead
of a verdict: job bundle symlink containment (for a bundle at a share root, or a
symlink escaping a drive-letter bundle onto a share), PATH parameter default
containment, and the download summary.
Add deadline.client._path_utils, which compares paths component by component so a
UNC host is an ordinary ancestor of its shares. Path spaces are discriminated with
splitroot (backported for Python < 3.12) rather than inferred from the string, so
a rooted driveless path, a drive root, a drive-relative path, the UNC namespace,
and the device namespace can never be confused for one another. Route all four
containment checks and the summary through it, and ban commonpath/commonprefix via
ruff TID251 so the bug class cannot return.
Containment fails closed on everything it cannot resolve, since every caller uses
it to decide whether a path is trusted. Extended-length and device paths
(\\?\..., \\.\...) keep their prefix and occupy a path space of their own rather
than being folded into the plain form they denote, so they never alias it; those
prefixes disable path normalization, so that space has no share-relative form and
a root there still contains its own files. The bare \\ anchor is not a root
directory the way POSIX / is -- splitroot reads it as a drive with an empty root,
an incomplete UNC spelling naming no server -- so it is an ancestor of nothing.
Treating it as one would trust every reachable share from a single root, and
ntpath.isabs reports it absolute, so it survives a caller's isabs filter.
Also harden the known-asset-path handling this exposed. Roots are expanded for
'~' and dropped unless absolute, rather than kept and compared or resolved against
the working directory. A non-absolute root cannot match the absolute candidates it
is checked against, so it is a hazard only once a caller normalizes it: resolving
one would mark an unrelated tree as trusted (os.path.abspath("") is the whole
working directory), suppressing the unknown-path warning and letting a
non-interactive submit upload files the user never designated. Dropping it at the
boundary keeps that from depending on which normalization a future caller reaches
for, and costs only a warning. An empty root arrives from --known-asset-path "",
the MCP tool's unvalidated JSON array, and a PATH parameter whose allowedValues
suppressed absolutization. Redundancy filtering now compares components, so a UNC
host subsumes its shares (Path.parts collapses \\server\share into one atom), case
variants of one location dedupe on Windows, and the caller's first,
highest-precedence spelling is the one retained.
Windows semantics are tested through an explicit path_module so they run on every
platform; UNC paths cannot be built with os.path.join(os.sep, ...), so tests
written against the native module silently skipped them on POSIX. Containment is
additionally checked against pathlib.PurePath.is_relative_to as an independent
oracle on Python 3.12+, where the only permitted disagreement is the UNC-ancestor
case this change adds. Reflexivity, ancestor soundness, and transitivity are
asserted over a corpus spanning every path space.
test/integ/windows_smb validates the fix against a real SMB share on a Windows
runner, since every other test models Windows lexically and cannot confirm the
redirector agrees. It is excluded from the default test paths and dispatched
manually, because creating a share requires administrator rights.
Fixes aws-deadline#1321
Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com>
a1c2c95 to
e56265f
Compare
| expanded = (os.path.expanduser(path) for path in known_asset_paths if path) | ||
| # normpath, not abspath: dedupes equivalent spellings without consulting the cwd. | ||
| ordered = list( | ||
| dict.fromkeys(os.path.normpath(path) for path in expanded if os.path.isabs(path)) |
There was a problem hiding this comment.
A bare \\ in the known-asset-paths list survives this isabs filter and then poisons the TRIE, silently dropping every UNC known path.
ntpath.isabs("\\\\") is True (it only inspects the first three characters for a leading separator) — the _path_utils code even notes this at _path_utils.py:140. So "\\\\" passes os.path.isabs, os.path.normpath leaves it unchanged, and path_components yields the single component ["\\\\"] (the _UNC_ANCHOR).
Because it has one component it sorts first, and the loop marks dir_tree["\\\\"] = True. Every subsequent UNC root then walks parts[:-1] == ["\\\\", "server", ...], hits current.get("\\\\") is True on the first part, and is filtered out as redundant. So _filter_redundant_known_paths given [r"\\", r"\\server\share", r"\\other\share"] returns only the bare anchor — both real roots are dropped.
That contradicts is_path_contained, which deliberately special-cases the bare anchor so it contains nothing (if root_components == [_UNC_ANCHOR]: return candidate_components == [_UNC_ANCHOR]).
Net effect: with a stray \\ entry the returned list holds a root that matches no candidate, and the genuine UNC roots that would have matched are gone — so files under them get reported as unknown locations, producing spurious warnings and blocking non-interactive submits. A bare \\ reaches here from the same inputs the docstring already calls out for the empty-string case: settings.known_asset_paths split on os.pathsep, --known-asset-path, and MCP input.
Since the two functions disagree about what \\ means, it seems cleanest to drop it where empty/relative roots are already dropped — e.g. also exclude roots whose path_components(...) is exactly the bare UNC anchor.
| # | ||
| # Not part of Code Quality: creating a share needs administrator rights, and the loopback | ||
| # share is slower and more environment-dependent than a unit test. | ||
| on: |
There was a problem hiding this comment.
This workflow has no automatic trigger, so the "regression coverage for issue #1321" it provides never actually runs.
on: declares only workflow_dispatch and workflow_call, and nothing in the repo calls it — the only uses: ./.github/workflows/... references are code_quality.yml and the three dcm_integration_test_* files (from manual_pypi_release.yml, release_publish.yml, dcm_integration_tests.yml). The tag input and the workflow_call block suggest a caller was intended.
Since test/integ is outside testpaths and outside hatch run test, test/integ/windows_smb is not reached by any other job either, so today the only way these tests execute is a manual dispatch. If the intent is for the SMB check to guard the fix, it needs to be wired into a scheduled or release-time workflow (e.g. alongside the dcm_integration_test_windows call in release_publish.yml) — otherwise a future regression in _path_utils UNC handling ships uncaught by anything but the lexical ntpath unit tests, which by their own docstring "say nothing about SMB".
2 participants
Fixes #1321
Problem
os.path.commonpathcannot compare a Windows host-level UNC path (\\server) with a path under one of its shares.ntpath.splitdrivereports no drive for the former and\\server\sharefor the latter, so it raises. Three distinct pairs fail:commonpathresult\\hostvs\\host\share\fValueError: Paths don't have the same drive(the reported case)\\host\s1\avs\\host\s2\bValueError: Paths don't have the same drive\\host\sharevs\\host\share\fValueError: Can't mix absolute and relative paths_is_known_pathcaught thatValueErroras "not contained", so every asset path under a\\serverknown root was reported as unknown and submissions from a network share could not proceed without confirmation.The audit found the same root cause at three more sites, where the exception was not caught at all and surfaced as a raw
ValueErrorrather than a verdict — two of them latent crashes rather than merely wrong answers:_submit_job_bundle.py:_is_known_pathValueError→ valid paths silently reported unknown_submit_job_bundle.py:_filter_redundant_known_pathsPath.partscollapses\\server\shareinto one atom, so a host root never subsumed its sharesjob_bundle/loader.pysymlink containmentjob_bundle/parameters.pyPATH defaultcli/_groups/job_group.pydownload summaryApproach
New private
deadline/client/_path_utils.pycompares paths component by component, so a UNC host is an ordinary ancestor of its shares.Path spaces are discriminated with
splitroot(backported for Python 3.9–3.11) rather than inferred from the string. That keeps a rooted driveless path, a drive root, a drive-relative path, the UNC namespace, and the device namespace from ever being confused for one another — the first draft inferred this from leading empty strings and had three fail-open defects as a result. Extended-length prefixes fold to the plain form they denote (\\?\C:\→C:\); those with no plain spelling (Volume{GUID},\\.\) keep their prefix and their own space.commonpathandcommonprefixare now banned via ruffTID251, so the bug class cannot return silently.Hardening this exposed
Known-asset-path handling had a fail-open worth calling out on its own: roots were compared raw against absolute candidates. Making them absolute naively meant
os.path.abspath("")— the entire working directory — became a trusted root, so every file beside the submitting shell was treated as known. Because a suppressed warning is what lets a non-interactive submit proceed, those files would upload silently.Roots are now
~-expanded and dropped unless absolute. An empty root is reachable three ways:--known-asset-path "", the MCP tool's unvalidated JSON array, and a PATH/FILE parameter whoseallowedValuessuppressed absolutization (leavingdirname(value)empty). Failing closed costs only a warning.Testing
Windows semantics run through an explicit
path_module, so they execute on every platform. This matters: UNC paths cannot be constructed withos.path.join(os.sep, ...), so tests written against the native module silently skipped every UNC case on Linux and macOS — which is how the original bug shipped.Beyond the case tables:
PurePath.is_relative_toon 3.12+, where the only permitted disagreement is the UNC-ancestor case this PR adds. Mutation-tested: reintroducing either of two earlier fail-opens turns it red.common_ancestor's result contains all its inputs", and transitivity, over a corpus spanning every path space.splitrootbackport — differential-tested against the stdlib over 181k generated cases (0 mismatches), with a negative control confirming the harness detects divergence.test/integ/windows_smb) — every other test models Windows lexically, which cannot confirm the SMB redirector agrees. This creates a loopback share on a Windows runner and checks that a host-level root contains files on its shares,realpathoutput stays contained, a bundle at a share root validates, an escaping symlink is still rejected, and a mapped drive stays a distinct path space. Excluded from default test paths and dispatched manually, since creating a share needs administrator rights.hatch run fmt/lint/testclean: 3322 passed, 100% coverage on the new module. The twodeadline_mcpcollection errors are pre-existing (the optionalmcpextra is not installed) and reproduce onmainline.Reviewer notes
\\server-contains-its-shares rule is the one piece of hand-written path logic left. No stdlib helper models it, because Windows does not treat\\serveras a mountable directory. It is the highest-value thing to attack.pathlib-based rewrite was prototyped and rejected:PureWindowsPathparses\\srvas drive-less before Python 3.12, so it would have un-fixed this issue on 3.9–3.11 (verified on real 3.9/3.11 interpreters) and introduced cross-namespace aliasing.deadline-cloud-job-attachments(_utils._is_relative_to,_normalize_windows_path, and threecommonpathsites, one guarding a TOCTOU symlink check). Deliberately out of scope here. Note those must be fixed together: correcting_is_relative_toalone regroups inputs under\\hostand triggers thecommonpathcrash that the containment bug currently masks.\\192.168.20.20).