From e56265fc9d973856d5022d8e75d4cef88377c55d Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Fri, 7 Aug 2026 16:21:45 -0700 Subject: [PATCH 01/28] fix: recognize host-level UNC roots in path containment checks 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 #1321 Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- .github/workflows/windows_smb_test.yml | 66 +++ pyproject.toml | 21 +- src/deadline/client/_path_utils.py | 193 +++++++ src/deadline/client/api/_submit_job_bundle.py | 51 +- src/deadline/client/cli/_groups/job_group.py | 3 +- src/deadline/client/job_bundle/loader.py | 4 +- src/deadline/client/job_bundle/parameters.py | 4 +- .../windows_smb/test_unc_path_containment.py | 222 ++++++++ .../cli/test_cli_bundle_submit_known_paths.py | 142 ++++++ test/unit/deadline_client/cli/test_cli_job.py | 33 ++ .../job_bundle/test_job_bundle_loader.py | 85 ++++ .../job_bundle/test_job_parameters.py | 88 ++++ test/unit/deadline_client/test_path_utils.py | 476 ++++++++++++++++++ 13 files changed, 1362 insertions(+), 26 deletions(-) create mode 100644 .github/workflows/windows_smb_test.yml create mode 100644 src/deadline/client/_path_utils.py create mode 100644 test/integ/windows_smb/test_unc_path_containment.py create mode 100644 test/unit/deadline_client/test_path_utils.py diff --git a/.github/workflows/windows_smb_test.yml b/.github/workflows/windows_smb_test.yml new file mode 100644 index 000000000..ab2cdf99a --- /dev/null +++ b/.github/workflows/windows_smb_test.yml @@ -0,0 +1,66 @@ +name: Windows SMB Path Test + +# Validates UNC path containment against a real SMB share, which lexical ntpath modeling +# cannot do. Regression coverage for issue #1321. +# +# 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: + workflow_dispatch: + workflow_call: + inputs: + tag: + description: Git ref (tag/branch/SHA) to test. Defaults to the triggering ref. + required: false + type: string + default: '' + +jobs: + test: + name: UNC Containment (real SMB) + runs-on: windows-latest + timeout-minutes: 15 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ inputs.tag || github.ref }} + + - uses: actions/setup-python@5fda3b95a4ea91299a34e894583c3862153e4b97 # v7.0.0 + with: + python-version: '3.12' + + - name: Confirm SMB prerequisites + # Fail with a clear message here rather than having every test skip itself, + # which would look like a pass. + shell: pwsh + run: | + $admin = ([Security.Principal.WindowsPrincipal] ` + [Security.Principal.WindowsIdentity]::GetCurrent() + ).IsInRole([Security.Principal.WindowsBuiltInRole]::Administrator) + if (-not $admin) { throw "Administrator rights are required to create an SMB share." } + Get-Service LanmanServer, LanmanWorkstation | Format-Table -AutoSize + Start-Service LanmanServer + Start-Service LanmanWorkstation + # Developer Mode lets a non-elevated process create symlinks; the escape + # test needs one and skips itself otherwise. + $key = 'HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\AppModelUnlock' + New-Item -Path $key -Force | Out-Null + Set-ItemProperty -Path $key -Name AllowDevelopmentWithoutDevLicense -Value 1 -Type DWord + + - name: Install + run: | + python -m pip install --upgrade pip hatch + + - name: Run the SMB path tests + shell: pwsh + run: | + hatch run pytest test/integ/windows_smb -v --no-cov -p no:randomly + + - name: Report skips + # A skipped SMB test is indistinguishable from a passing one in the summary, + # so surface the count explicitly. + if: always() + shell: pwsh + run: | + hatch run pytest test/integ/windows_smb --no-cov -q -rs 2>&1 | + Select-String -Pattern 'SKIPPED|passed|failed' diff --git a/pyproject.toml b/pyproject.toml index e9a373419..98ae9d1da 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -123,9 +123,28 @@ line-length = 100 # E402 (imports must be at the top of the file) is pinned explicitly here so it # can't silently regress if a future ruff drops it from the defaults; it is # build-failing via `hatch run lint`. -extend-select = ["RUF022", "E402"] +# TID251 bans path helpers that raise on Windows UNC paths; see banned-api below. +extend-select = ["RUF022", "E402", "TID251"] ignore = ["E501"] +[tool.ruff.lint.flake8-tidy-imports.banned-api] +# commonpath raises ValueError on Windows UNC paths (issue #1321): callers that catch it +# silently reject valid paths, callers that don't crash. ntpath/posixpath are banned too +# because this codebase passes explicit path modules around. +"os.path.commonpath".msg = "Use deadline.client._path_utils.common_ancestor (or is_path_contained for containment checks); commonpath raises ValueError on Windows UNC paths. See issue #1321." +"ntpath.commonpath".msg = "Use deadline.client._path_utils.common_ancestor (or is_path_contained for containment checks); commonpath raises ValueError on Windows UNC paths. See issue #1321." +"posixpath.commonpath".msg = "Use deadline.client._path_utils.common_ancestor (or is_path_contained for containment checks); commonpath raises ValueError on Windows UNC paths. See issue #1321." +# commonprefix compares strings, not path components, so it reports '\\host\share2' as +# sharing a prefix with '\\host\share'. It is never the right containment primitive. +"os.path.commonprefix".msg = "Use deadline.client._path_utils.common_ancestor; commonprefix is a string-prefix match, not a path-component match." + +[tool.ruff.lint.per-file-ignores] +# The sanctioned wrappers, and the one place allowed to reach for what they replace. +"src/deadline/client/_path_utils.py" = ["TID251"] +# These compare the wrappers against the stdlib behavior they replace. +"test/unit/deadline_client/test_path_utils.py" = ["TID251"] +"test/unit/deadline_client/api/test_job_bundle_submission_asset_refs.py" = ["TID251"] + [tool.ruff.lint.isort] known-first-party = ["deadline"] diff --git a/src/deadline/client/_path_utils.py b/src/deadline/client/_path_utils.py new file mode 100644 index 000000000..64a5ddbad --- /dev/null +++ b/src/deadline/client/_path_utils.py @@ -0,0 +1,193 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + +""" +Path containment helpers that understand Windows UNC paths. + +``os.path.commonpath`` raises ``ValueError`` rather than comparing a host-level UNC path +(``\\\\server``) with a path under one of its shares: ``splitdrive`` reports no drive for +the former and ``\\\\server\\share`` for the latter. It raises the same way for two shares +on one host. Callers that read that exception as "not contained" reject valid paths. + +These helpers compare paths component by component instead, so a UNC host is an ordinary +ancestor of its shares. Every function takes an explicit ``path_module`` +(``ntpath``/``posixpath``), so Windows semantics stay testable on non-Windows hosts. + +Comparisons are lexical -- pass ``realpath`` output in if symlinks must be resolved -- and +never raise. Anything unresolvable fails closed, since callers use containment to decide +whether a path is trusted. +""" + +from __future__ import annotations + +import os +from typing import Any, Iterable, Sequence + +__all__ = [ + "common_ancestor", + "is_any_path_contained", + "is_path_contained", + "path_components", +] + +# Anchors the UNC path space. It names no server on its own, so unlike POSIX '/' it is not +# a directory and contains nothing. +_UNC_ANCHOR = "\\\\" + +_PARDIR = ".." + + +def _splitroot(text: str, path_module: Any) -> tuple[str, str, str]: + """``path_module.splitroot``, backported for Python < 3.12. + + ``(drive, root)`` is what distinguishes one path space from another: ``('', '\\\\')`` + (rooted but driveless) and ``('\\\\\\\\', '')`` (UNC) both consist only of separators. + """ + splitroot = getattr(path_module, "splitroot", None) + if splitroot is not None: + return splitroot(text) + + drive, rest = path_module.splitdrive(text) + separators = path_module.sep + (getattr(path_module, "altsep", None) or "") + if rest[:1] not in separators or not rest: + return drive, "", rest + 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:] + + +def _split_anchored(path: Any, path_module: Any, normalize_case: bool) -> tuple[str, list[str]]: + """Return ``(anchor, parts)``, where ``anchor + sep.join(parts)`` reconstructs ``path``. + + The anchor names the path space and carries its own trailing separator. A UNC anchor is + the bare ``\\\\`` marker, leaving the server and share as ordinary parts -- which is what + lets a host-level root contain the shares beneath it. + """ + text = str(path) + windows = path_module.sep == "\\" + if windows: + text = text.replace("/", "\\") + text = path_module.normpath(text) + if normalize_case: + text = path_module.normcase(text) + + drive, root, tail = _splitroot(text, path_module) + parts = [part for part in tail.split(path_module.sep) if part] + + if not windows: + # '//foo' and '/foo' are the same file on the platforms this client targets. + return (path_module.sep if root else ""), parts + + if drive[:4] in ("\\\\?\\", "\\\\.\\"): + # These prefixes disable normalization: the drive is a whole anchor, so it never + # aliases the plain path it resembles, and a share root and its files -- which + # differ only by a trailing separator -- still take the same anchor. + return drive + path_module.sep, parts + if drive.startswith(_UNC_ANCHOR): + return _UNC_ANCHOR, [p for p in drive[len(_UNC_ANCHOR) :].split("\\") if p] + parts + return drive + root, parts + + +def _leading_pardir_count(parts: list[str]) -> int: + """Count the leading '..' run that ``normpath`` could not resolve. + + Counted on parts rather than whole components because an anchor can precede the run + ('C:..\\x' is the parent of the working directory on drive C:). + """ + count = 0 + for part in parts: + if part != _PARDIR: + break + count += 1 + return count + + +def path_components( + path: Any, + *, + path_module: Any = os.path, + normalize_case: bool = True, +) -> list[str]: + """Split ``path`` into the components used for ancestor comparisons. + + ``..`` segments are resolved first. The first component is the path space (``'/'``, + ``'C:\\'``, ``'C:'`` for drive-relative, ``'\\\\'`` for UNC, absent when relative) and + the rest are the path's parts, so comparing these lists component-wise confuses neither + one path space for another nor a string prefix for a directory prefix. + + ``normalize_case`` lowercases components on Windows to match the filesystem. + """ + anchor, parts = _split_anchored(path, path_module, normalize_case) + return ([anchor] if anchor else []) + parts + + +def is_path_contained( + path: Any, + root: Any, + *, + path_module: Any = os.path, +) -> bool: + """Return True iff ``path`` equals or is a descendant of ``root``. + + Containment is anchored on whole components, so a sibling that merely shares a string + prefix (root ``/trusted/project`` vs path ``/trusted/project-secret``) is outside the + root. Paths in unrelated spaces -- different drives, different UNC hosts, one relative + and one absolute -- are not contained. + """ + root_components = path_components(root, path_module=path_module) + candidate_components = path_components(path, path_module=path_module) + # A bare '\\' root names no server, so it is an ancestor of nothing -- otherwise it + # would prefix, and so trust, every reachable share. ntpath.isabs lets it reach here. + if root_components == [_UNC_ANCHOR]: + return candidate_components == [_UNC_ANCHOR] + if candidate_components[: len(root_components)] != root_components: + return False + # Shared leading '..' belongs to the root; one below it could climb back out. + return _PARDIR not in candidate_components[len(root_components) :] + + +def is_any_path_contained( + path: Any, + roots: Iterable[Any], + *, + path_module: Any = os.path, +) -> bool: + """Return True iff ``path`` is contained by any root in ``roots``.""" + return any(is_path_contained(path, root, path_module=path_module) for root in roots) + + +def common_ancestor(paths: Sequence[Any], *, path_module: Any = os.path) -> str: + """Return the deepest directory containing every path in ``paths``. + + This is ``os.path.commonpath`` without the exceptions: paths in unrelated spaces return + ``""`` rather than raising, and a UNC host is a valid answer for paths on different + shares of one server. The result keeps the first path's spelling and, like + ``commonpath``, is purely lexical. + """ + if not paths: + return "" + + split = [_split_anchored(p, path_module, normalize_case=True) for p in paths] + normalized = [([a] if a else []) + parts for a, parts in split] + anchor, spelled_parts = _split_anchored(paths[0], path_module, normalize_case=False) + spelled = ([anchor] if anchor else []) + spelled_parts + + # '..' and '../..' are rooted at different unknown places, so runs of differing depth + # share nothing. Comparing them positionally would return the shallower path, which is + # not an ancestor of the deeper one -- os.path.commonpath has that bug. + if len({_leading_pardir_count(parts) for _, parts in split}) > 1: + return "" + + shared = min(len(components) for components in normalized) + while shared > 0 and any(other[:shared] != normalized[0][:shared] for other in normalized): + shared -= 1 + if shared == 0: + return "" + # Matching only the bare anchor means different servers, so no shared directory. + if shared == 1 and normalized[0][0] == _UNC_ANCHOR: + return "" + + # The anchor carries its own separator, so it abuts the first part directly. + if anchor: + return anchor + path_module.sep.join(spelled[1:shared]) + return path_module.sep.join(spelled[:shared]) diff --git a/src/deadline/client/api/_submit_job_bundle.py b/src/deadline/client/api/_submit_job_bundle.py index 691f32891..4eb6634b5 100644 --- a/src/deadline/client/api/_submit_job_bundle.py +++ b/src/deadline/client/api/_submit_job_bundle.py @@ -71,6 +71,7 @@ summarize_path_list, ) from ...job_attachments.api._hashing import _hash_attachments +from .._path_utils import is_any_path_contained, path_components logger = logging.getLogger(__name__) @@ -83,22 +84,12 @@ def hashing_telemetry_callback(hashing_summary: SummaryStatistics): def _is_known_path(path: Path | str, known_roots: Iterable[Path | str]) -> bool: """Return True iff ``path`` equals or is a descendant of any root in ``known_roots``. - Containment is anchored via ``os.path.commonpath`` equality (the same idiom as - loader.py): a path is contained only when it shares a whole-component prefix with a - root, so a sibling that merely shares a string prefix (root ``/trusted/project`` vs - candidate ``/trusted/project-secret``) is outside the root. + Containment is anchored on whole components, so a sibling that merely shares a + string prefix (root ``/trusted/project`` vs candidate ``/trusted/project-secret``) + is outside the root. """ - norm_candidate = os.path.normpath(str(path)) - for known_path in known_roots: - norm_root = os.path.normpath(str(known_path)) - try: - if os.path.commonpath([norm_root, norm_candidate]) == norm_root: - return True - except ValueError: - # commonpath raises for mixed absolute/relative paths or different Windows - # drives; such paths are not contained. - continue - return False + # Passed explicitly, and read at call time, so tests can patch it for another platform. + return is_any_path_contained(path, known_roots, path_module=os.path) def _summarize_asset_paths( @@ -294,22 +285,42 @@ def _filter_redundant_known_paths(known_asset_paths: Iterable[str]) -> list[str] This algorithm identifies any paths that have a different path as a prefix, and removes them from the list. Pseudo-code is: - 1. Sort the paths from shortest to longest, so any prefix of a path has + 1. Sort the paths from fewest to most components, so any prefix of a path has to happen before that path. 2. For each path, split it into parts (i.e. '/mnt/prod/project' becomes - ['/', 'mnt', 'prod', 'project']), and then insert it part by part into + ['', 'mnt', 'prod', 'project']), and then insert it part by part into a nested dict called dir_tree organized as a TRIE. The value True in the TRIE indicates that a path with that as its final part is in the list. 3. While inserting a path into the TRIE, detect whether another path already had a prefix of the parts, and filter out the path when that occurs. + + Components come from ``path_components`` rather than ``Path.parts`` so a Windows UNC + host is an ancestor of its shares (``Path.parts`` collapses '\\\\server\\share' into one + atom) and case variants of one location dedupe on Windows. + + Roots are expanded for '~' (the config file and the CLI submitter's default data + directory supply one unexpanded) and dropped unless absolute. A non-absolute root + matches no candidate anyway, but dropping it here means a future caller cannot turn it + into a trusted tree by resolving it -- ``os.path.abspath("")`` is the whole working + directory, which would suppress the unknown-path warning and let a non-interactive + submit upload undesignated files. An empty root arrives from a PATH parameter whose + allowedValues suppressed absolutization, and from ``--known-asset-path``/MCP input. """ + # Passed explicitly, and read at call time, so tests can patch it for another platform. + 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)) + ) + components = {path: path_components(path, path_module=os.path) for path in ordered} # This directory tree gets filled with the known asset paths, with # a True value as a marker for the last part of already seen paths. dir_tree: dict[str, Any] = {} filtered_paths: list[str] = [] - # Process the paths from shortest to longest, so that prefixes are always seen first - for path in sorted(known_asset_paths, key=len): - parts = Path(path).parts + # Fewest components first, so prefixes are seen first. Ties keep input order, so of two + # spellings of one location the caller's first -- highest precedence -- is retained. + for path in sorted(ordered, key=lambda p: (len(components[p]), ordered.index(p))): + parts = components[path] current: Optional[dict[str, Any]] = dir_tree for part in parts[:-1]: # If we see a True value, another path is a prefix so we can skip it. diff --git a/src/deadline/client/cli/_groups/job_group.py b/src/deadline/client/cli/_groups/job_group.py index 21519cef9..6aba259b3 100644 --- a/src/deadline/client/cli/_groups/job_group.py +++ b/src/deadline/client/cli/_groups/job_group.py @@ -41,6 +41,7 @@ from ... import api from ...config import config_file +from ..._path_utils import common_ancestor from ...exceptions import DeadlineOperationError, DeadlineOperationTimedOut from .._common import ( _OUTPUT_FORMAT_HELP, @@ -910,7 +911,7 @@ def _get_summary_of_files_to_download_message( return _get_json_line(JSON_MSG_TYPE_PRESUMMARY, output_paths_by_root) else: paths_message_joined = " " + "\n ".join( - f"{os.path.commonpath([os.path.join(directory, p) for p in output_paths])} ({len(output_paths)} file{'s' if len(output_paths) > 1 else ''})" + f"{common_ancestor([os.path.join(directory, p) for p in output_paths], path_module=os.path)} ({len(output_paths)} file{'s' if len(output_paths) > 1 else ''})" for directory, output_paths in output_paths_by_root.items() ) return f"\nSummary of files to download:\n{paths_message_joined}\n" diff --git a/src/deadline/client/job_bundle/loader.py b/src/deadline/client/job_bundle/loader.py index 5094a92e7..eaaacc935 100644 --- a/src/deadline/client/job_bundle/loader.py +++ b/src/deadline/client/job_bundle/loader.py @@ -16,6 +16,7 @@ import yaml +from .._path_utils import is_path_contained from ..exceptions import DeadlineOperationError @@ -34,8 +35,7 @@ def validate_directory_symlink_containment(job_bundle_dir: str) -> None: for path in chain(dir_names, file_names): norm_path = os.path.normpath(os.path.join(root_dir, path)) resolved_path = os.path.realpath(norm_path) - common_path = os.path.commonpath([resolved_root, resolved_path]) - if common_path != resolved_root: + if not is_path_contained(resolved_path, resolved_root, path_module=os.path): raise DeadlineOperationError( f"Job bundle cannot contain a path that resolves outside of the resolved bundle directory:\n{resolved_root}\n\nPath in bundle:\n{norm_path}\nResolves to:\n{resolved_path}" ) diff --git a/src/deadline/client/job_bundle/parameters.py b/src/deadline/client/job_bundle/parameters.py index 4721eb721..fd2fa2a46 100644 --- a/src/deadline/client/job_bundle/parameters.py +++ b/src/deadline/client/job_bundle/parameters.py @@ -23,6 +23,7 @@ NotRequired = object TypedDict = object +from .._path_utils import is_path_contained from ..exceptions import DeadlineOperationError from .loader import read_yaml_or_json_object @@ -799,8 +800,7 @@ def read_job_bundle_parameters(bundle_dir: str) -> list[JobParameter]: ) bundle_real_path = os.path.realpath(bundle_dir) default_real_path = os.path.realpath(os.path.join(bundle_real_path, default)) - common_path = os.path.commonpath([bundle_real_path, default_real_path]) - if common_path != bundle_real_path: + if not is_path_contained(default_real_path, bundle_real_path, path_module=os.path): raise DeadlineOperationError( f"Job Template for job bundle {bundle_dir}:\nDefault PATH '{default_real_path}' for parameter '{name}' specifies files outside of Job Bundle directory '{bundle_real_path}'.\nPATH values must be relative, and must resolve within the Job Bundle directory." ) diff --git a/test/integ/windows_smb/test_unc_path_containment.py b/test/integ/windows_smb/test_unc_path_containment.py new file mode 100644 index 000000000..f7bf7be78 --- /dev/null +++ b/test/integ/windows_smb/test_unc_path_containment.py @@ -0,0 +1,222 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + +""" +Real-SMB validation of UNC path containment. + +Every other test of the path helpers models Windows lexically through ``ntpath``, which +is faithful to Windows' string rules but says nothing about SMB: whether a host-level UNC +path can be listed, whether ``realpath`` rewrites a mapped drive back to UNC form, or +whether a share walks like a directory. These run against a loopback share, so the +verdicts are checked against a real redirector. + +Requires Windows and administrator rights; see .github/workflows/windows_smb_test.yml. +Regression coverage for https://github.com/aws-deadline/deadline-cloud/issues/1321. +""" + +from __future__ import annotations + +import os +import socket +import subprocess +import sys +import uuid +from pathlib import Path +from typing import Iterator + +import pytest + +from deadline.client._path_utils import common_ancestor, is_path_contained +from deadline.client.api._submit_job_bundle import ( + _filter_redundant_known_paths, + _is_known_path, +) +from deadline.client.job_bundle.loader import validate_directory_symlink_containment +from deadline.client.exceptions import DeadlineOperationError + +pytestmark = [ + pytest.mark.integ, + pytest.mark.skipif(sys.platform != "win32", reason="SMB shares require Windows."), +] + + +def _run(*args: str) -> subprocess.CompletedProcess: + return subprocess.run(args, capture_output=True, text=True, check=False) + + +@pytest.fixture(scope="module") +def smb_share(tmp_path_factory) -> Iterator[tuple[str, Path]]: + """Share a local directory over SMB and yield ``(unc_root, local_path)``. + + ``unc_root`` is the share path (``\\\\\\``); the host-level root is + derived from it by the tests that need one. + """ + local_path = tmp_path_factory.mktemp("smb_export") + share_name = f"dltest{uuid.uuid4().hex[:8]}" + + created = _run("net", "share", f"{share_name}={local_path}", "/GRANT:Everyone,FULL") + if created.returncode != 0: + pytest.skip( + f"could not create an SMB share: {created.stdout.strip()} {created.stderr.strip()}" + ) + + # The loopback host name matters: 'localhost' and '127.0.0.1' are both valid UNC + # hosts, but the machine name is what a real farm would use. + unc_root = rf"\\{socket.gethostname()}\{share_name}" + try: + # Fail fast and clearly if the redirector cannot reach the new share, rather + # than letting every assertion below fail with a confusing error. + if not os.path.isdir(unc_root): + pytest.skip(f"SMB share {unc_root} is not reachable from this host") + yield unc_root, local_path + finally: + _run("net", "share", share_name, "/DELETE", "/Y") + + +def test_host_level_root_contains_share_contents(smb_share): + """The reported bug: a '\\\\server' root must contain files on its shares.""" + unc_root, local_path = smb_share + host_root = unc_root.rsplit("\\", 1)[0] + + asset = Path(unc_root) / "assets" / "scene.c4d" + asset.parent.mkdir(parents=True, exist_ok=True) + asset.write_text("scene", encoding="utf8") + + assert os.path.isfile(asset), f"{asset} was not written through the share" + assert is_path_contained(asset, host_root) + assert is_path_contained(asset, unc_root) + assert _is_known_path(asset, [host_root]) + assert _is_known_path(asset, [unc_root]) + + +def test_host_level_root_survives_redundancy_filtering(smb_share): + """A host-level root must reach the containment check intact. + + It is absolute per ``os.path.isabs`` and must subsume its own shares, so the + filter keeps the host and drops the share. + """ + unc_root, _ = smb_share + host_root = unc_root.rsplit("\\", 1)[0] + + assert _filter_redundant_known_paths([host_root]) == [host_root] + assert _filter_redundant_known_paths([host_root, unc_root]) == [host_root] + + +def test_neighbouring_host_is_not_contained(smb_share): + """A different host must not be contained, even one sharing a string prefix.""" + unc_root, _ = smb_share + host_root = unc_root.rsplit("\\", 1)[0] + + assert not is_path_contained(rf"{host_root}2\share\file", host_root) + assert not is_path_contained(r"\\other-host\share\file", host_root) + + +def test_realpath_of_share_content_stays_contained(smb_share): + """``realpath`` output must still be recognized as inside the share. + + Both containment guards resolve their operands first, so any rewriting by the + redirector would make them silently reject valid paths. + """ + unc_root, _ = smb_share + host_root = unc_root.rsplit("\\", 1)[0] + + nested = Path(unc_root) / "resolve_probe" / "file.txt" + nested.parent.mkdir(parents=True, exist_ok=True) + nested.write_text("probe", encoding="utf8") + + resolved = os.path.realpath(nested) + assert is_path_contained(resolved, os.path.realpath(unc_root)) + assert is_path_contained(resolved, host_root), ( + f"realpath rewrote {nested} to {resolved}, which no longer resolves under {host_root}" + ) + + +def test_bundle_on_share_passes_symlink_containment(smb_share): + """A job bundle living on a share must validate, including at the share root. + + ``\\\\server\\share`` vs its own files is one of the pairs ``os.path.commonpath`` + rejected outright. + """ + unc_root, _ = smb_share + + bundle = Path(unc_root) / "bundle" + bundle.mkdir(parents=True, exist_ok=True) + (bundle / "template.yaml").write_text( + "specificationVersion: jobtemplate-2023-09\n", encoding="utf8" + ) + validate_directory_symlink_containment(str(bundle)) + + # And a bundle that IS the share root. + root_bundle = Path(unc_root) / "root_bundle" + root_bundle.mkdir(parents=True, exist_ok=True) + (root_bundle / "template.yaml").write_text( + "specificationVersion: jobtemplate-2023-09\n", encoding="utf8" + ) + validate_directory_symlink_containment(str(root_bundle)) + + +def test_symlink_escaping_the_share_is_rejected(smb_share): + """A symlink out of a bundle on a share must still be caught. + + This is the security direction: the lexical tests assert it, but only a real + filesystem exercises the ``realpath`` resolution the guard depends on. + """ + unc_root, local_path = smb_share + + bundle = Path(unc_root) / "escape_bundle" + bundle.mkdir(parents=True, exist_ok=True) + outside = local_path / "outside_secret.txt" + outside.write_text("secret", encoding="utf8") + + link = bundle / "escape.txt" + try: + os.symlink(outside, link) + except OSError as exc: # pragma: no cover - depends on runner privileges + pytest.skip(f"cannot create a symlink on this share: {exc}") + + with pytest.raises(DeadlineOperationError): + validate_directory_symlink_containment(str(bundle)) + + +def test_mapped_drive_resolves_and_compares(smb_share): + """A mapped drive letter is a distinct path space from the UNC path it points at. + + Studios commonly map a share to a drive letter. Whichever spelling ``realpath`` + reports, containment must agree with it rather than mixing the two. + """ + unc_root, _ = smb_share + host_root = unc_root.rsplit("\\", 1)[0] + + for letter in ("Y:", "Z:"): + if _run("net", "use", letter, unc_root).returncode == 0: + drive = letter + break + else: + pytest.skip("no free drive letter to map the share onto") + + try: + asset = Path(drive + "\\") / "mapped_probe.txt" + asset.write_text("mapped", encoding="utf8") + + resolved = os.path.realpath(asset) + # Whatever spelling realpath returns, it must be contained by the matching + # root and not by the other path space. + if resolved.startswith("\\\\"): + assert is_path_contained(resolved, host_root) + else: + assert is_path_contained(resolved, drive + "\\") + assert not is_path_contained(resolved, host_root) + finally: + _run("net", "use", drive, "/DELETE", "/Y") + + +def test_common_ancestor_across_shares_on_one_host(smb_share): + """Files on two shares of one host share only the host. + + ``os.path.commonpath`` raises ``ValueError: Paths don't have the same drive`` for + this pair, which is what made the download summary crash. + """ + unc_root, _ = smb_share + host_root = unc_root.rsplit("\\", 1)[0] + + ancestor = common_ancestor([rf"{host_root}\share1\a.exr", rf"{host_root}\share2\b.exr"]) + assert ancestor.rstrip("\\").lower() == host_root.lower(), ancestor diff --git a/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py b/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py index 4a827f437..63a202a19 100644 --- a/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py +++ b/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py @@ -4,6 +4,7 @@ Tests for the known asset paths functionality in the bundle_submit CLI command. """ +import ntpath import os import json import tempfile @@ -16,6 +17,7 @@ from deadline.client import config from deadline.client.cli import main +from deadline.client.api import _submit_job_bundle as sjb from deadline.client.api._submit_job_bundle import ( _filter_redundant_known_paths, _generate_message_for_asset_paths, @@ -146,6 +148,146 @@ def test_is_known_path(path, roots, expected): assert _is_known_path(path, roots) is expected +@pytest.mark.parametrize( + "path, roots, expected", + [ + # Regression for https://github.com/aws-deadline/deadline-cloud/issues/1321: + # a host-level UNC root must contain paths under any of its shares. + ( + r"\\192.168.20.20\projects\assets\FA_Anim\260304_FA_Anim.c4d", + [r"\\192.168.20.20"], + True, + ), + (r"\\host\share\file", [r"\\host"], True), + (r"\\host\share\file", ["\\\\host\\"], True), + (r"\\host\share\file", [r"\\host\share"], True), + # Neither a different nor a prefix-sharing host is contained. + (r"\\other\share\file", [r"\\host"], False), + (r"\\host2\share\file", [r"\\host"], False), + (r"\\host\share2\file", [r"\\host\share"], False), + # A UNC candidate is not contained by a drive-letter root, and vice versa. + (r"\\host\share\file", [r"C:\trusted"], False), + (r"C:\trusted\file", [r"\\host\share"], False), + # Contained by the second of several roots, including a mismatched-drive first root. + (r"\\host\share\file", [r"D:\other", r"\\host"], True), + # A bare UNC anchor names no server, so it must not trust every reachable share. It + # passes the isabs filter, so '--known-asset-path \\' reaches here as a root. + (r"\\corp\finance\salaries.xlsx", ["\\\\"], False), + (r"\\corp\finance\salaries.xlsx", ["//"], False), + (r"\\corp\finance\salaries.xlsx", ["\\\\?\\UNC\\"], False), + # A useless root must not shadow a real one that follows it. + (r"\\host\share\file", ["\\\\", r"\\host"], True), + ], +) +def test_is_known_path_windows_semantics(path, roots, expected): + """Windows path semantics, exercised via ntpath so the cases run on every platform.""" + with patch.object(sjb.os, "path", ntpath): + assert _is_known_path(path, roots) is expected + + +@pytest.mark.parametrize( + "input, expected", + [ + # A host-level root makes its shares redundant. + ([r"\\host", r"\\host\share"], [r"\\host"]), + ([r"\\host\share", r"\\host"], [r"\\host"]), + ([r"\\host\share\a", r"\\host"], [r"\\host"]), + # Distinct hosts and shares are all kept. + ([r"\\host\s1", r"\\host\s2"], [r"\\host\s1", r"\\host\s2"]), + ([r"\\host1", r"\\host2"], [r"\\host1", r"\\host2"]), + # A host sharing a string prefix is not made redundant. + ([r"\\host", r"\\host2\share"], [r"\\host", r"\\host2\share"]), + # Case variants of the same location are redundant on Windows. + ([r"\\host\Share", r"\\HOST\share\sub"], [r"\\host\Share"]), + ([r"C:\proj", r"c:\PROJ\sub"], [r"C:\proj"]), + # Drive-letter roots stay separate from UNC roots. + ([r"C:\proj", r"\\host\share"], [r"C:\proj", r"\\host\share"]), + ], +) +def test_filter_redundant_known_paths_windows_semantics(input, expected): + # abspath is left native so the already-absolute inputs pass through unchanged. + with patch.object(sjb.os.path, "abspath", lambda p: p), patch.object(sjb.os, "path", ntpath): + assert _filter_redundant_known_paths(input) == expected + + +def test_filter_redundant_known_paths_expands_user_paths(): + """ + A '~'-prefixed root has to be expanded to match an absolute candidate. Such a root + reaches here from the config file and the CLI job submitter's default data + directory, neither of which goes through shell expansion. + """ + home_root = os.path.join("~", "projects") + expected_home = os.path.join(os.path.expanduser("~"), "projects") + + assert _filter_redundant_known_paths([home_root]) == [expected_home] + assert _is_known_path(os.path.join(expected_home, "scene.ma"), [expected_home]) is True + + # Expanding must not defeat redundancy filtering: '~/projects' and its subdirectory + # name the same tree, so only the ancestor survives. + assert _filter_redundant_known_paths([home_root, os.path.join(home_root, "sub")]) == [ + expected_home + ] + + +@pytest.mark.parametrize( + "known_path", + [ + # An empty known path reaches this code from `--known-asset-path ""`, from the + # MCP tool's unvalidated JSON array, and from a PATH/FILE job parameter whose + # allowedValues suppressed absolutization (os.path.dirname("scene.ma") == ""). + "", + # Relative roots, including the Windows root-relative and drive-relative forms. + "assets", + os.path.join("..", "shared"), + "\\projects", + "C:rel", + ], +) +def test_filter_redundant_known_paths_drops_unanchored_paths(known_path): + """ + A root that names no absolute location must be dropped, not resolved against the cwd. + + It matches no candidate either way, but dropping it at the boundary means a future + caller cannot turn it into a trusted tree: os.path.abspath("") is the whole working + directory, which would suppress the unknown-asset-path warning and let a + non-interactive submit upload undesignated files. + """ + assert _filter_redundant_known_paths([known_path]) == [] + + # A real root alongside an unanchored one still survives. + real_root = os.path.abspath(os.path.join(os.sep, "trusted", "project")) + assert _filter_redundant_known_paths([known_path, real_root]) == [real_root] + + +def test_filter_redundant_known_paths_unanchored_path_does_not_trust_cwd(): + """The working directory must not become a known root via an empty path.""" + cwd_file = os.path.join(os.getcwd(), "unrelated_secret.txt") + assert _is_known_path(cwd_file, _filter_redundant_known_paths([""])) is False + + +def test_generate_message_for_asset_paths_unc_host_root_is_known(): + """ + Regression for issue #1321: files on a share under a host-level UNC known root + must not trigger the unknown-path warning. + """ + known_root = r"\\192.168.20.20" + inside_file = r"\\192.168.20.20\projects\assets\FA_Anim\260304_FA_Anim.c4d" + + upload_group = AssetUploadGroup( + asset_groups=[AssetRootGroup(root_path=r"\\192.168.20.20\projects", inputs={inside_file})], # type: ignore[arg-type] + total_input_files=1, + total_input_bytes=12, + ) + + with patch("deadline.client.api._submit_job_bundle.os.path", ntpath): + message, no_warnings = _generate_message_for_asset_paths( + upload_group, storage_profile=None, known_asset_paths=[known_root] + ) + + assert no_warnings is True, message + assert "WARNING: Files were specified outside of known asset paths." not in message, message + + def test_generate_message_for_asset_paths_sibling_prefix_is_unknown(): """ Security regression test: a known root must NOT "contain" a sibling path that diff --git a/test/unit/deadline_client/cli/test_cli_job.py b/test/unit/deadline_client/cli/test_cli_job.py index 61b51e8d9..d7ba97057 100644 --- a/test/unit/deadline_client/cli/test_cli_job.py +++ b/test/unit/deadline_client/cli/test_cli_job.py @@ -7,6 +7,7 @@ from datetime import timezone import datetime import json +import ntpath import os from typing import Dict, List import pytest @@ -913,6 +914,38 @@ def test_get_summary_of_files_to_download_message_windows( ) +@pytest.mark.parametrize( + "output_paths_by_root, expected_result", + [ + # A root under a UNC share summarizes to the shared subdirectory. + ( + {r"\\host\share": ["renders/image1.png", "renders/image2.png"]}, + "\nSummary of files to download:\n \\\\host\\share\\renders (2 files)\n", + ), + # Files directly at a UNC share root summarize to the share itself. os.path.commonpath + # returns '\\\\host\\share\\' here, leaving a stray trailing separator in the message. + ( + {r"\\host\share": ["image1.png", "image2.png"]}, + "\nSummary of files to download:\n \\\\host\\share (2 files)\n", + ), + ( + {r"\\host\share": ["only.png"]}, + "\nSummary of files to download:\n \\\\host\\share\\only.png (1 file)\n", + ), + ], +) +def test_get_summary_of_files_to_download_message_unc_paths( + output_paths_by_root: Dict[str, List[str]], + expected_result: str, +): + """UNC path summaries, exercised via ntpath so the cases run on every platform.""" + with patch.object(job_group.os, "path", ntpath): + assert ( + _get_summary_of_files_to_download_message(output_paths_by_root, is_json_format=False) + == expected_result + ) + + def test_cli_job_wait_succeeded(fresh_deadline_config): """ Test that job wait command returns exit code 0 when job succeeds. diff --git a/test/unit/deadline_client/job_bundle/test_job_bundle_loader.py b/test/unit/deadline_client/job_bundle/test_job_bundle_loader.py index 01858a774..08f5e3bce 100644 --- a/test/unit/deadline_client/job_bundle/test_job_bundle_loader.py +++ b/test/unit/deadline_client/job_bundle/test_job_bundle_loader.py @@ -7,13 +7,17 @@ """ import json +import ntpath import os import sys +from contextlib import contextmanager +from unittest.mock import patch import pytest import yaml from deadline.client.exceptions import DeadlineOperationError +from deadline.client.job_bundle import loader from deadline.client.job_bundle.loader import ( parse_yaml_or_json_content, read_yaml_or_json, @@ -241,6 +245,87 @@ def test_validate_directory_symlink_containment_fail(tmpdir): validate_directory_symlink_containment(str(test_root)) +class TestSymlinkContainmentWindowsPaths: + """ + Windows path semantics for validate_directory_symlink_containment, exercised through + a simulated ntpath filesystem so the cases run on every platform. + + os.path.commonpath raises ValueError for a bundle located at a UNC share root + ('\\\\host\\share' vs '\\\\host\\share\\template.yaml' -> "Can't mix absolute and + relative paths"), and for a symlink escaping a drive-letter bundle onto a UNC share + ('C:\\bundle' vs '\\\\host\\share\\x' -> "Paths don't have the same drive"). Neither + exception is caught, so both would surface as a raw ValueError rather than a + containment verdict. + """ + + @contextmanager + def _simulated_windows_bundle(self, bundle_dir, entries, resolves_to): + """Simulate an ntpath filesystem holding ``entries`` under ``bundle_dir``. + + ``resolves_to`` maps a normalized path to the location it resolves to, standing + in for a symlink target. + """ + + class _WindowsPath: + def __getattr__(self, name): + return getattr(ntpath, name) + + @staticmethod + def isdir(path): + return path == bundle_dir + + @staticmethod + def realpath(path): + return resolves_to.get(ntpath.normpath(path), ntpath.normpath(path)) + + def walk(top): + yield top, [], list(entries) + + with patch.object(loader.os, "walk", walk), patch.object(loader.os, "path", _WindowsPath()): + yield + + def test_bundle_at_unc_share_root_is_valid(self): + """A bundle directory that is itself a UNC share root contains its own files.""" + bundle_dir = r"\\host\share" + with self._simulated_windows_bundle(bundle_dir, ["template.yaml"], {}): + validate_directory_symlink_containment(bundle_dir) + + def test_bundle_under_unc_share_is_valid(self): + bundle_dir = r"\\host\share\bundle" + with self._simulated_windows_bundle(bundle_dir, ["template.yaml"], {}): + validate_directory_symlink_containment(bundle_dir) + + def test_symlink_escaping_unc_share_root_is_rejected(self): + bundle_dir = r"\\host\share" + with self._simulated_windows_bundle( + bundle_dir, + ["escape.yaml"], + {r"\\host\share\escape.yaml": r"\\host\other\secret.yaml"}, + ): + with pytest.raises(DeadlineOperationError): + validate_directory_symlink_containment(bundle_dir) + + def test_symlink_from_drive_bundle_onto_unc_share_is_rejected(self): + bundle_dir = r"C:\bundle" + with self._simulated_windows_bundle( + bundle_dir, + ["escape.yaml"], + {r"C:\bundle\escape.yaml": r"\\host\share\secret.yaml"}, + ): + with pytest.raises(DeadlineOperationError): + validate_directory_symlink_containment(bundle_dir) + + def test_symlink_to_sibling_prefix_directory_is_rejected(self): + bundle_dir = r"C:\bundle" + with self._simulated_windows_bundle( + bundle_dir, + ["escape.yaml"], + {r"C:\bundle\escape.yaml": r"C:\bundle-secret\secret.yaml"}, + ): + with pytest.raises(DeadlineOperationError): + validate_directory_symlink_containment(bundle_dir) + + class TestHiddenParameterValidation: """Tests for hidden parameter validation in read_job_bundle_parameters.""" diff --git a/test/unit/deadline_client/job_bundle/test_job_parameters.py b/test/unit/deadline_client/job_bundle/test_job_parameters.py index 0e960c802..f44b86280 100644 --- a/test/unit/deadline_client/job_bundle/test_job_parameters.py +++ b/test/unit/deadline_client/job_bundle/test_job_parameters.py @@ -7,6 +7,11 @@ from __future__ import annotations +import ntpath +from contextlib import contextmanager +from copy import deepcopy +from unittest.mock import patch + import pytest from deadline.client.job_bundle import parameters @@ -686,3 +691,86 @@ def test_ui_control_for_parameter_definition_errors(parameter_def): def test_parameter_definition_difference(parameter1, parameter2, expected_difference): """Test that parameter_definition_difference returns expected differences.""" assert parameters.parameter_definition_difference(parameter1, parameter2) == expected_difference + + +class TestPathDefaultContainmentWindowsPaths: + """ + Windows path semantics for the PATH-default containment check in + read_job_bundle_parameters, exercised through a simulated ntpath filesystem so the + cases run on every platform. + + os.path.commonpath raises ValueError when the bundle sits at a UNC share root + ('\\\\host\\share' vs '\\\\host\\share\\sub' -> "Can't mix absolute and relative + paths"). That exception is not caught, so a valid template would fail to load with a + raw ValueError instead of resolving its default. + """ + + TEMPLATE = { + "specificationVersion": "jobtemplate-2023-09", + "name": "PathDefault", + "parameterDefinitions": [ + { + "name": "OutDir", + "type": "PATH", + "objectType": "DIRECTORY", + "dataFlow": "OUT", + "default": "output", + } + ], + } + + @contextmanager + def _simulated_windows_bundle(self, bundle_dir, resolves_to=None): + resolves_to = resolves_to or {} + + class _WindowsPath: + def __getattr__(self, name): + return getattr(ntpath, name) + + @staticmethod + def realpath(path): + return resolves_to.get(ntpath.normpath(path), ntpath.normpath(path)) + + def read_yaml_or_json_object(bundle_dir, filename, required): + # Deep-copied because read_job_bundle_parameters sets 'value' on the + # parameter definitions in place. + return deepcopy(self.TEMPLATE) if filename == "template" else None + + with ( + patch.object(parameters.os, "path", _WindowsPath()), + patch.object(parameters, "read_yaml_or_json_object", read_yaml_or_json_object), + ): + yield + + def _out_dir_value(self, result): + return next(p for p in result if p["name"] == "OutDir")["value"] + + def test_bundle_at_unc_share_root_resolves_default(self): + bundle_dir = r"\\host\share" + with self._simulated_windows_bundle(bundle_dir): + result = parameters.read_job_bundle_parameters(bundle_dir) + assert self._out_dir_value(result) == r"\\host\share\output" + + def test_bundle_under_unc_share_resolves_default(self): + bundle_dir = r"\\host\share\bundle" + with self._simulated_windows_bundle(bundle_dir): + result = parameters.read_job_bundle_parameters(bundle_dir) + assert self._out_dir_value(result) == r"\\host\share\bundle\output" + + def test_default_resolving_outside_unc_share_is_rejected(self): + bundle_dir = r"\\host\share\bundle" + with self._simulated_windows_bundle( + bundle_dir, + {r"\\host\share\bundle\output": r"\\host\other\secret"}, + ): + with pytest.raises(exceptions.DeadlineOperationError): + parameters.read_job_bundle_parameters(bundle_dir) + + def test_default_resolving_from_drive_bundle_onto_unc_share_is_rejected(self): + bundle_dir = r"C:\bundle" + with self._simulated_windows_bundle( + bundle_dir, + {r"C:\bundle\output": r"\\host\share\secret"}, + ): + with pytest.raises(exceptions.DeadlineOperationError): + parameters.read_job_bundle_parameters(bundle_dir) diff --git a/test/unit/deadline_client/test_path_utils.py b/test/unit/deadline_client/test_path_utils.py new file mode 100644 index 000000000..9ea413ae3 --- /dev/null +++ b/test/unit/deadline_client/test_path_utils.py @@ -0,0 +1,476 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + +""" +Tests for the path containment helpers. + +Windows semantics go through an explicit ``ntpath`` so these run on every platform. UNC +paths cannot be built with ``os.path.join(os.sep, ...)``, so tests written against the +native path module silently skip them on POSIX. +""" + +import itertools +import ntpath +import posixpath +import sys +from pathlib import PurePosixPath, PureWindowsPath +from typing import Any + +import pytest + +from deadline.client._path_utils import ( + _splitroot, + common_ancestor, + is_any_path_contained, + is_path_contained, + path_components, +) + + +@pytest.mark.parametrize( + "candidate, root, expected", + [ + # Regression for https://github.com/aws-deadline/deadline-cloud/issues/1321: + # a host-level UNC root contains the shares beneath it. os.path.commonpath + # rejects this pair because it reads '\\192.168.20.20' as having no drive but + # '\\192.168.20.20\projects' as being a drive. + ( + r"\\192.168.20.20\projects\assets\FA_Anim\260304_FA_Anim.c4d", + r"\\192.168.20.20", + True, + ), + (r"\\host\share\file", r"\\host", True), + (r"\\host\share", r"\\host", True), + (r"\\host", r"\\host", True), + # A trailing separator on a host-level root does not change containment. + (r"\\host\share\file", "\\\\host\\", True), + # A different host is not contained. + (r"\\other\share\file", r"\\host", False), + # A host that merely shares a string prefix is not contained. + (r"\\host2\share\file", r"\\host", False), + # Share-level roots behave like directories. + (r"\\host\share\file", r"\\host\share", True), + (r"\\host\share", r"\\host\share", True), + (r"\\host\share2\file", r"\\host\share", False), + # The host is not contained by one of its shares. + (r"\\host", r"\\host\share", False), + # Forward slashes are accepted on Windows. + ("//host/share/file", r"\\host", True), + (r"\\host\share\file", "//host/share", True), + # Case-insensitive, matching the filesystem. + (r"\\HOST\Share\File", r"\\host\share", True), + # Drive letters. + (r"C:\trusted\project\sub\file", r"C:\trusted\project", True), + (r"C:\trusted\project", r"C:\trusted\project", True), + (r"C:\trusted\project-secret\file", r"C:\trusted\project", False), + (r"C:\trusted\projectextra", r"C:\trusted\project", False), + (r"C:\trusted", r"C:\trusted\project", False), + (r"c:\trusted\project\file", r"C:\TRUSTED\Project", True), + # '..' is resolved before comparing. + (r"C:\trusted\project\..\project-secret\f", r"C:\trusted\project", False), + (r"C:\trusted\project\sub\..\f", r"C:\trusted\project", True), + (r"\\host\share\a\..\..\b\f", r"\\host\share\a", False), + # Windows clamps '..' at a share root, so this stays inside the share. + (r"\\host\share\sub\..\..\other\f", r"\\host\share", True), + # A '..' that normpath cannot resolve (there is no share to clamp against) + # fails closed rather than being read as a component named '..'. + (r"\\host\..\other\share\f", r"\\host", False), + # Mismatched drives are simply not contained; no exception. + (r"D:\trusted\project\file", r"C:\trusted\project", False), + (r"\\host\share\file", r"C:\trusted\project", False), + (r"C:\trusted\project\file", r"\\host\share", False), + # A drive-relative path ('C:file' means 'file' relative to the cwd on C:) + # cannot be resolved here, so it fails closed. + ("C:file", "C:\\", False), + # Relative paths are not contained by absolute roots and vice versa. + (r"relative\file", r"C:\trusted", False), + (r"C:\trusted\file", r"relative", False), + # An extended-length path occupies its own path space rather than being folded into + # the plain form it denotes, so comparing across the two spellings fails closed. No + # caller needs the fold: every call site feeds realpath output or isabs-filtered + # roots, neither of which carries a '\\?\' prefix. + (r"\\?\C:\trusted\project\file", r"C:\trusted\project", False), + (r"C:\trusted\project\file", r"\\?\C:\trusted\project", False), + (r"\\?\UNC\host\share\file", r"\\host\share", False), + (r"\\host\share\file", r"\\?\UNC\host\share", False), + (r"\\?\UNC\host\share\file", r"\\host", False), + (r"\\?\C:\trusted\project\file", r"\\?\C:\trusted\project", True), + (r"\\?\UNC\host\share\file", r"\\?\UNC\host\share", True), + (r"\\?\C:\trusted\project-secret\f", r"\\?\C:\trusted\project", False), + # A rooted, driveless root ('\') is a different path space than the UNC + # namespace, so it must not contain remote paths -- nor they it. + (r"\\attacker\share\evil", "\\", False), + ("\\x", "\\\\", False), + ("\\x", "\\", True), + # The bare anchor names no server, so it is an ancestor of nothing -- treating it as + # POSIX '/' would trust every reachable share. ntpath.isabs('\\') is True, so a + # caller filtering roots on that lets it through; '//' and '\\?\UNC\' normalize to it. + (r"\\host\share\file", "\\\\", False), + (r"\\host\share\file", "\\\\\\\\", False), + (r"\\host\share\file", "//", False), + (r"\\host\share\file", "\\\\?\\UNC\\", False), + (r"\\host", "\\\\", False), + # The anchor is still reflexive, and a root naming an actual server still works. + ("\\\\", "\\\\", True), + (r"\\host\share\file", r"\\host", True), + # 'C:' means the cwd on drive C:, so it contains drive-relative paths but not + # the drive root's absolute contents. + (r"C:\Windows", "C:", False), + ("C:foo", "C:", True), + (r"C:\a", "C:\\", True), + # A prefixed drive with no plain spelling keeps its own space: a device path + # must not alias the drive it resembles, in either direction. + (r"\\.\C:\secret", "C:\\", False), + (r"C:\secret", r"\\.\C:", False), + (r"\\?\Volume{abc}\trusted\f", r"Volume{abc}\trusted", False), + (r"\\?\Volume{abc}\trusted\f", r"\\?\Volume{abc}\trusted", True), + # '\\?\C:' must contain paths in neither the drive-relative 'C:' space nor plain + # 'C:\'. isabs reports it absolute, so a caller filtering on that lets it through. + ("C:foo", r"\\?\C:", False), + (r"C:\a", r"\\?\C:", False), + (r"C:\a\f", "\\\\?\\C:\\", False), + # Within its own space it behaves like the drive root it spells. + (r"\\?\C:\a", r"\\?\C:", True), + # A relative path is contained in itself even when normpath leaves a leading + # '..' it cannot cancel; only a '..' below the root can climb back out. + (r"..\a", r"..\a", True), + (r"..\a\b", r"..\a", True), + (r"..\a\..\b", r"..\a", False), + ], +) +def test_is_path_contained_windows(candidate, root, expected): + assert is_path_contained(candidate, root, path_module=ntpath) is expected + + +@pytest.mark.parametrize( + "candidate, root, expected", + [ + ("/trusted/project", "/trusted/project", True), + ("/trusted/project/sub/file", "/trusted/project", True), + ("/trusted/project/file", "/trusted/project/", True), + ("/trusted/project-secret/f", "/trusted/project", False), + ("/trusted/projectextra", "/trusted/project", False), + ("/trusted", "/trusted/project", False), + ("/somewhere/else", "/trusted/project", False), + ("/trusted/project/../project-secret/f", "/trusted/project", False), + ("/trusted/project/sub/../f", "/trusted/project", True), + ("relative/file", "/trusted/project", False), + ("/trusted/file", "relative", False), + # Everything absolute is contained by the root directory. + ("/trusted/project", "/", True), + # POSIX paths are case-sensitive, so a case variant fails closed. + ("/Trusted/Project/f", "/trusted/project", False), + # A backslash is an ordinary filename character on POSIX, so a Windows-style + # UNC string is just a relative filename and matches nothing. + (r"\\host\share\file", r"\\host", False), + # A doubled root names the same file, so containment does not depend on how + # many leading slashes either side was spelled with. + ("//mnt/shared/f", "/mnt/shared", True), + ("///mnt/shared/f", "/mnt/shared", True), + ("/mnt/shared/f", "//mnt/shared", True), + # Reflexive, and tolerant of a leading '..' shared with the root. + ("../a", "../a", True), + ("../a/b", "../a", True), + ("../a/../b", "../a", False), + ("..", "..", True), + # A root of '.' is not the working directory: normpath renders it as a lone '.' + # component, which prefixes nothing. Unreachable (callers pass absolute roots) and + # fails closed, so it is pinned as a known limitation rather than fixed. + ("rel", ".", False), + ("rel/f", ".", False), + ("/abs/f", ".", False), + (".", ".", True), + (".", "rel", False), + ], +) +def test_is_path_contained_posix(candidate, root, expected): + assert is_path_contained(candidate, root, path_module=posixpath) is expected + + +@pytest.mark.parametrize("path_module", [ntpath, posixpath]) +def test_is_path_contained_is_reflexive(path_module): + """Every path contains itself, whatever space it is in.""" + paths = ( + [ + r"\\host", + r"\\host\share\a", + "C:", + "C:\\", + r"C:\a", + "\\", + r"..\a", + r"rel\f", + ".", + r"C:..\x", + r"C:..\..\x", + r"\\?\C:", + ] + if path_module is ntpath + else ["/", "//", "/a/b", "../a", "../../a", "rel/f", "."] + ) + for path in paths: + assert is_path_contained(path, path, path_module=path_module) is True, path + + +@pytest.mark.parametrize("path_module", [ntpath, posixpath]) +def test_common_ancestor_contains_its_inputs(path_module): + """A non-empty common_ancestor must contain every path it was derived from.""" + paths = ( + [ + r"\\host\share\a", + r"\\host\s2\b", + r"C:\a\b", + r"C:\a\c", + "C:foo", + r"..\a\b", + r"..\a\c", + # Drive-relative '..' puts the run behind an anchor, where a guard counting + # from index zero would miss it. + r"C:..\x", + r"C:..\..\x", + r"C:..\a\y", + r"\\?\C:", + r"\\?\C:\a", + ] + if path_module is ntpath + else ["/a/b", "/a/c", "//a/d", "../a/b", "../a/c", "../../a/b", "rel/f", "rel/g"] + ) + for first in paths: + for second in paths: + ancestor = common_ancestor([first, second], path_module=path_module) + if not ancestor: + continue + assert is_path_contained(first, ancestor, path_module=path_module), (first, ancestor) + assert is_path_contained(second, ancestor, path_module=path_module), (second, ancestor) + + +@pytest.mark.parametrize("path_module", [ntpath, posixpath]) +def test_splitroot_backport_matches_stdlib(path_module): + """The Python < 3.12 shim must agree with splitroot on every path space. + + Python 3.12 added ``splitroot``; this project supports 3.9, so on older + interpreters the shim is what distinguishes one path space from another. Hiding + ``splitroot`` exercises the shim on any interpreter. + """ + if not hasattr(path_module, "splitroot"): + pytest.skip("stdlib splitroot unavailable, nothing to compare against") + + cases = ( + [ + "\\\\", + "\\", + r"\\srv", + r"\\srv\share", + r"\\srv\share\a", + "C:", + "C:\\", + "C:foo", + r"C:\a", + "C:\\\\a", + r"\\?\C:\a", + r"\\?\UNC\srv\sh\a", + r"\\?\Volume{abc}\a", + r"\\.\C:\a", + "", + r"rel\f", + "\\\\\\srv", + ] + if path_module is ntpath + else ["/", "//", "///", "////", "/a", "//a/b", "///a", "rel", "rel/f", ""] + ) + + class _NoSplitroot: + """Proxy that hides splitroot so the backport path is taken.""" + + splitroot = None + + def __getattr__(self, name): + return getattr(path_module, name) + + for case in cases: + assert _splitroot(case, _NoSplitroot()) == path_module.splitroot(case), case + + +@pytest.mark.skipif( + sys.version_info < (3, 12), + reason="pathlib parses a host-only UNC path as drive-less before 3.12, so it is not a" + " usable oracle there.", +) +@pytest.mark.parametrize("path_module", [ntpath, posixpath]) +def test_agrees_with_pathlib_except_for_unc_hosts(path_module): + """Differential check against ``PurePath.is_relative_to`` as an independent oracle. + + pathlib folds a UNC server and share into one atom, so it cannot see a host-level root + as an ancestor of its shares -- that gap is issue #1321 and the only sanctioned + disagreement. Elsewhere pathlib is the reference. It does not resolve '..', so the + corpus avoids inputs needing normalization; this supplements the explicit cases above + rather than replacing them. + """ + if path_module is ntpath: + flavour: Any = PureWindowsPath + # Spans every path space, including the three where earlier versions of this + # module wrongly reported containment: bare roots, the device namespace, and + # drive-relative paths. + corpus = [ + r"\\srv", + r"\\srv\share", + r"\\srv\share\a", + r"\\srv\other\b", + r"\\srv2\share", + "C:\\", + r"C:\a", + r"C:\a\b", + r"C:\a-secret", + r"D:\a", + "rel", + r"rel\f", + "\\", + "\\\\", + r"\x", + "C:", + "C:foo", + r"\\.\C:\a", + r"\\?\Volume{abc}\a", + ] + else: + flavour = PurePosixPath + corpus = ["/", "/a", "/a/b", "/a-secret", "rel", "rel/f"] + + for candidate, root in itertools.permutations(corpus, 2): + ours = is_path_contained(candidate, root, path_module=path_module) + pathlibs = flavour(candidate).is_relative_to(flavour(root)) + if ours == pathlibs: + continue + # The only sanctioned disagreement: a UNC root that pathlib cannot see as an + # ancestor because it folds the server and share into one atom. We may only be + # more permissive than pathlib here, never elsewhere and never in reverse. + assert path_module is ntpath, (candidate, root, ours, pathlibs) + assert ours is True and pathlibs is False, (candidate, root, ours, pathlibs) + # The root must name an actual server. The bare '\\\\' anchor names none, so it + # is not a sanctioned disagreement -- pathlib is right to contain nothing there. + assert flavour(root).drive.startswith("\\\\"), (candidate, root) + assert str(root) != "\\\\", (candidate, root) + assert flavour(candidate).drive.startswith("\\\\"), (candidate, root) + assert not flavour(root).parts[1:], (candidate, root) + + +def test_is_any_path_contained(): + assert is_any_path_contained("/a/f", ["/b", "/a"]) is True + assert is_any_path_contained("/c/f", ["/b", "/a"]) is False + # No roots means nothing is contained. + assert is_any_path_contained("/a/f", []) is False + assert ( + is_any_path_contained(r"\\host\share\f", [r"D:\other", r"\\host"], path_module=ntpath) + is True + ) + + +@pytest.mark.parametrize( + "path, path_module, expected", + [ + # The first component is the path space; a UNC server and share are ordinary + # parts beneath the '\\\\' anchor, which is what lets a host-level root + # contain them. + (r"\\host", ntpath, ["\\\\", "host"]), + ("\\\\host\\", ntpath, ["\\\\", "host"]), + (r"\\host\share", ntpath, ["\\\\", "host", "share"]), + (r"\\host\share\a\b", ntpath, ["\\\\", "host", "share", "a", "b"]), + ("C:\\", ntpath, ["c:\\"]), + (r"C:\a", ntpath, ["c:\\", "a"]), + # 'C:' (drive-relative, meaning the cwd on C:) is a different space than 'C:\'. + ("C:", ntpath, ["c:"]), + ("C:foo", ntpath, ["c:", "foo"]), + # A rooted, driveless path is its own space, distinct from the UNC anchor. + ("\\", ntpath, ["\\"]), + ("\\\\", ntpath, ["\\\\"]), + # A prefixed drive keeps its prefix and stays whole, so it occupies a space of + # its own and cannot alias the plain drive or UNC path it resembles. + (r"\\?\C:\a", ntpath, ["\\\\?\\c:\\", "a"]), + # The anchor carries its own trailing separator, so a share root and a file + # under it share an anchor and containment holds between them. + (r"\\?\UNC\host\share", ntpath, ["\\\\?\\unc\\host\\share\\"]), + (r"\\?\UNC\host\share\f", ntpath, ["\\\\?\\unc\\host\\share\\", "f"]), + (r"\\?\Volume{abc}\a", ntpath, ["\\\\?\\volume{abc}\\", "a"]), + (r"\\.\C:\a", ntpath, ["\\\\.\\c:\\", "a"]), + ("/", posixpath, ["/"]), + ("/a/b", posixpath, ["/", "a", "b"]), + ("/a/b/", posixpath, ["/", "a", "b"]), + ("a/b", posixpath, ["a", "b"]), + # '//foo' and '/foo' are the same file on every supported platform, so a + # doubled root collapses rather than forming a separate namespace. + ("//a/b", posixpath, ["/", "a", "b"]), + ("///a/b", posixpath, ["/", "a", "b"]), + ], +) +def test_path_components(path, path_module, expected): + assert path_components(path, path_module=path_module) == expected + + +def test_path_components_preserves_case_when_asked(): + assert path_components(r"\\Host\Share\File", path_module=ntpath, normalize_case=False) == [ + "\\\\", + "Host", + "Share", + "File", + ] + + +@pytest.mark.parametrize( + "paths, path_module, expected", + [ + # The common ancestor of paths under one share, spelled with its real case. + ( + [r"\\host\Share\Proj\a.txt", r"\\host\Share\Proj\sub\b.txt"], + ntpath, + r"\\host\Share\Proj", + ), + # Different shares on one host share only the host. os.path.commonpath raises + # ValueError for this pair. + ([r"\\host\s1\a", r"\\host\s2\b"], ntpath, r"\\host"), + # Different hosts share nothing. There is no location above a UNC host, so the + # bare '\\\\' that their leading components have in common is not an answer. + ([r"\\host1\s\a", r"\\host2\s\b"], ntpath, ""), + ([r"\\host1", r"\\host2"], ntpath, ""), + # Different drives share nothing. + ([r"C:\a\b", r"D:\a\b"], ntpath, ""), + ([r"C:\a\b", r"\\host\share\b"], ntpath, ""), + ([r"C:\proj\a", r"C:\proj\b"], ntpath, r"C:\proj"), + ([r"C:\proj\a"], ntpath, r"C:\proj\a"), + (["/a/b/c", "/a/b/d"], posixpath, "/a/b"), + (["/a/b", "/c/d"], posixpath, "/"), + # A doubled POSIX root is the same space as '/', so these behave like ordinary + # absolute paths rather than a separate namespace. + (["//a/b", "//c/d"], posixpath, "/"), + (["//a/b", "//a/c"], posixpath, "/a"), + (["/a", "/b"], posixpath, "/"), + (["/a/b"], posixpath, "/a/b"), + (["a/b", "/c/d"], posixpath, ""), + ([], posixpath, ""), + # Paths whose unresolved leading '..' runs differ in depth are rooted at + # different unknown directories, so they share none. Positional comparison + # would wrongly read the shared '..' as one directory and return '..', which + # is not an ancestor of '../../up'. os.path.commonpath has that bug. + (["../up", "../../up"], posixpath, ""), + (["../../up", "../up"], posixpath, ""), + ([r"..\up", r"..\..\up"], ntpath, ""), + # The '..' run can sit behind an anchor, where a guard counting from index 0 + # would not see it. 'C:..' is the cwd's parent on C:, 'C:..\..' its grandparent. + ([r"C:..\x", r"C:..\..\x"], ntpath, ""), + ([r"C:..", r"C:..\.."], ntpath, ""), + # Equal depth behind an anchor is still comparable. + ([r"C:..\a\x", r"C:..\a\y"], ntpath, r"C:..\a"), + # Equal '..' depth is comparable again. + (["../a/x", "../a/y"], posixpath, "../a"), + (["../../a/x", "../../a/y"], posixpath, "../../a"), + # Relative inputs keep their own spelling and gain no leading separator. A + # Windows-style path read under posixpath semantics is one of these, since + # 'C:' is an ordinary component there rather than a drive. + (["a/b/c", "a/b/d"], posixpath, "a/b"), + ( + ["C:/Users/u/renders/i1.png", "C:/Users/u/renders/i2.png"], + posixpath, + "C:/Users/u/renders", + ), + ], +) +def test_common_ancestor(paths, path_module, expected): + assert common_ancestor(paths, path_module=path_module) == expected From fb66df65c32e4a34cbc09907e534d8c607f4a4f9 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Wed, 12 Aug 2026 21:05:23 -0700 Subject: [PATCH 02/28] fix: recognize UNC roots before Python 3.11, fold extended-length prefixes Three defects of one shape: a Windows path spelling that names an ordinary location was read as a different path space, so a root stopped containing its own contents. Before Python 3.11 both ntpath.normpath and ntpath.splitdrive strip a UNC path that names no share down to a rooted, driveless one ('\\host' -> '\host', splitdrive reporting no drive at all). A host-level known-asset root therefore landed in a different path space than the candidates under it, leaving issue #1321 unfixed on 3.9 and 3.10 -- two of the six interpreters the CI matrix covers. UNC-ness is now read off the path text rather than off splitdrive's drive, and a collapsed anchor is restored. _filter_redundant_known_paths deduped roots with a raw os.path.normpath, which truncated a host-level root the same way. That list is what _is_known_path compares against, so the damage reached the trust decision rather than staying cosmetic. It now uses normalized_path, which keeps the anchor. An extended-length prefix ('\\?\') only turns off Win32 normalization; it denotes the same location as the plain spelling. It now folds to that spelling instead of occupying a path space of its own, so a prefixed path is contained by exactly the roots its plain form is -- job-attachments carries that form through its internals, so it can reach these checks. Folding ahead of normpath also makes '..' resolution independent of the running Python. Forms denoting no plain path (Volume{GUID}, GLOBALROOT and the '\\.\' device namespace) keep their own space and still alias nothing. common_ancestor moves to _path_summary, leaving _path_utils to the trust decision alone. It is display-only with a single caller, and it carried the unresolved-'..' and preserved-spelling handling that only a printed string needs. Verified by differential across 3.9, 3.10, 3.11 and 3.14: 42 component cases and 1722 containment permutations, zero disagreements. The pre-3.11 branch is exercised on every interpreter through an injected path module that reproduces the old normpath and splitdrive, with assertions that the proxy is not passing vacuously. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- pyproject.toml | 9 +- src/deadline/client/_path_summary.py | 74 +++++ src/deadline/client/_path_utils.py | 136 ++++---- src/deadline/client/api/_submit_job_bundle.py | 11 +- src/deadline/client/cli/_groups/job_group.py | 2 +- .../windows_smb/test_unc_path_containment.py | 3 +- .../unit/deadline_client/test_path_summary.py | 109 +++++++ test/unit/deadline_client/test_path_utils.py | 301 +++++++++++------- 8 files changed, 462 insertions(+), 183 deletions(-) create mode 100644 src/deadline/client/_path_summary.py create mode 100644 test/unit/deadline_client/test_path_summary.py diff --git a/pyproject.toml b/pyproject.toml index 98ae9d1da..b137e7746 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -131,18 +131,19 @@ ignore = ["E501"] # commonpath raises ValueError on Windows UNC paths (issue #1321): callers that catch it # silently reject valid paths, callers that don't crash. ntpath/posixpath are banned too # because this codebase passes explicit path modules around. -"os.path.commonpath".msg = "Use deadline.client._path_utils.common_ancestor (or is_path_contained for containment checks); commonpath raises ValueError on Windows UNC paths. See issue #1321." -"ntpath.commonpath".msg = "Use deadline.client._path_utils.common_ancestor (or is_path_contained for containment checks); commonpath raises ValueError on Windows UNC paths. See issue #1321." -"posixpath.commonpath".msg = "Use deadline.client._path_utils.common_ancestor (or is_path_contained for containment checks); commonpath raises ValueError on Windows UNC paths. See issue #1321." +"os.path.commonpath".msg = "Use deadline.client._path_utils.is_path_contained for containment checks, or deadline.client._path_summary.common_ancestor for a displayed summary; commonpath raises ValueError on Windows UNC paths. See issue #1321." +"ntpath.commonpath".msg = "Use deadline.client._path_utils.is_path_contained for containment checks, or deadline.client._path_summary.common_ancestor for a displayed summary; commonpath raises ValueError on Windows UNC paths. See issue #1321." +"posixpath.commonpath".msg = "Use deadline.client._path_utils.is_path_contained for containment checks, or deadline.client._path_summary.common_ancestor for a displayed summary; commonpath raises ValueError on Windows UNC paths. See issue #1321." # commonprefix compares strings, not path components, so it reports '\\host\share2' as # sharing a prefix with '\\host\share'. It is never the right containment primitive. -"os.path.commonprefix".msg = "Use deadline.client._path_utils.common_ancestor; commonprefix is a string-prefix match, not a path-component match." +"os.path.commonprefix".msg = "Use deadline.client._path_utils.is_path_contained or deadline.client._path_summary.common_ancestor; commonprefix is a string-prefix match, not a path-component match." [tool.ruff.lint.per-file-ignores] # The sanctioned wrappers, and the one place allowed to reach for what they replace. "src/deadline/client/_path_utils.py" = ["TID251"] # These compare the wrappers against the stdlib behavior they replace. "test/unit/deadline_client/test_path_utils.py" = ["TID251"] +"test/unit/deadline_client/test_path_summary.py" = ["TID251"] "test/unit/deadline_client/api/test_job_bundle_submission_asset_refs.py" = ["TID251"] [tool.ruff.lint.isort] diff --git a/src/deadline/client/_path_summary.py b/src/deadline/client/_path_summary.py new file mode 100644 index 000000000..f15126f80 --- /dev/null +++ b/src/deadline/client/_path_summary.py @@ -0,0 +1,74 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + +""" +Summarizing a group of paths for display. + +Kept out of ``_path_utils`` because this is presentation rather than a trust decision, and +it carries cases that only a displayed string cares about -- unresolved ``..`` runs and +preserving the caller's spelling -- which a reader auditing containment should not have to +read past. + +Like the containment helpers, this is purely lexical and never raises. +""" + +from __future__ import annotations + +import os +from typing import Any, Sequence + +from ._path_utils import _PARDIR, _UNC_ANCHOR, _split_anchored + +__all__ = [ + "common_ancestor", +] + + +def _leading_pardir_count(parts: list[str]) -> int: + """Count the leading '..' run that ``normpath`` could not resolve. + + Counted on parts rather than whole components because an anchor can precede the run + ('C:..\\x' is the parent of the working directory on drive C:). + """ + count = 0 + for part in parts: + if part != _PARDIR: + break + count += 1 + return count + + +def common_ancestor(paths: Sequence[Any], *, path_module: Any = os.path) -> str: + """Return the deepest directory containing every path in ``paths``. + + This is ``os.path.commonpath`` without the exceptions: paths in unrelated spaces return + ``""`` rather than raising, and a UNC host is a valid answer for paths on different + shares of one server. The result keeps the first path's spelling and, like + ``commonpath``, is purely lexical. + """ + if not paths: + return "" + + split = [_split_anchored(p, path_module, normalize_case=True) for p in paths] + normalized = [([a] if a else []) + parts for a, parts in split] + anchor, spelled_parts = _split_anchored(paths[0], path_module, normalize_case=False) + spelled = ([anchor] if anchor else []) + spelled_parts + + # '..' and '../..' are rooted at different unknown places, so runs of differing depth + # share nothing. Comparing them positionally would return the shallower path, which is + # not an ancestor of the deeper one -- os.path.commonpath has that bug. + if len({_leading_pardir_count(parts) for _, parts in split}) > 1: + return "" + + shared = min(len(components) for components in normalized) + while shared > 0 and any(other[:shared] != normalized[0][:shared] for other in normalized): + shared -= 1 + if shared == 0: + return "" + # Matching only the bare anchor means different servers, so no shared directory. + if shared == 1 and normalized[0][0] == _UNC_ANCHOR: + return "" + + # The anchor carries its own separator, so it abuts the first part directly. + if anchor: + return anchor + path_module.sep.join(spelled[1:shared]) + return path_module.sep.join(spelled[:shared]) diff --git a/src/deadline/client/_path_utils.py b/src/deadline/client/_path_utils.py index 64a5ddbad..89e048af0 100644 --- a/src/deadline/client/_path_utils.py +++ b/src/deadline/client/_path_utils.py @@ -20,12 +20,13 @@ from __future__ import annotations import os -from typing import Any, Iterable, Sequence +import string +from typing import Any, Iterable __all__ = [ - "common_ancestor", "is_any_path_contained", "is_path_contained", + "normalized_path", "path_components", ] @@ -33,6 +34,12 @@ # a directory and contains nothing. _UNC_ANCHOR = "\\\\" +# Spelled out rather than taken from os.* because these helpers parse Windows paths on any +# host, where os.sep is '/'. +_EXTENDED_PREFIX = "\\\\?\\" +_DEVICE_PREFIX = "\\\\.\\" +_EXTENDED_UNC_MARKER = "UNC" + _PARDIR = ".." @@ -56,6 +63,33 @@ def _splitroot(text: str, path_module: Any) -> tuple[str, str, str]: return drive, rest[:root_length], rest[root_length:] +def _denotes_drive(text: str) -> bool: + """True for a bare drive spelling such as ``'C:'``.""" + return len(text) == 2 and text[1] == ":" and text[0] in string.ascii_letters + + +def _fold_extended_length_prefix(text: str) -> str: + """Rewrite an extended-length path as the plain path it denotes. + + ``\\\\?\\`` only turns off Win32 normalization; it names the same location as the plain + spelling. Folding it keeps one location from having two sets of components, which would + report a prefixed path outside a root that plainly contains it. Forms with no plain + spelling (``Volume{GUID}``, ``GLOBALROOT``, and the ``\\\\.\\`` device namespace) are + left alone, so they keep a path space of their own and alias nothing. + """ + if not text.startswith(_EXTENDED_PREFIX): + return text + denoted = text[len(_EXTENDED_PREFIX) :] + head = denoted.split("\\", 1)[0] + if head.upper() == _EXTENDED_UNC_MARKER: + # '\\?\UNC\server\share' is '\\server\share'. 'UNC' alone names no server, so it + # folds to the bare anchor, which contains nothing. + return _UNC_ANCHOR + denoted[len(_EXTENDED_UNC_MARKER) :].lstrip("\\") + if _denotes_drive(head): + return denoted + return text + + def _split_anchored(path: Any, path_module: Any, normalize_case: bool) -> tuple[str, list[str]]: """Return ``(anchor, parts)``, where ``anchor + sep.join(parts)`` reconstructs ``path``. @@ -67,7 +101,24 @@ def _split_anchored(path: Any, path_module: Any, normalize_case: bool) -> tuple[ windows = path_module.sep == "\\" if windows: text = text.replace("/", "\\") + # Folded before normpath, which leaves '..' alone inside a '\\?\' path before 3.11 + # and collapses it after. Folding first makes the result the same on every + # supported interpreter. + text = _fold_extended_length_prefix(text) + # Read off the text, not off splitdrive's drive: before Python 3.11 splitdrive reports + # no drive at all for a UNC path that names no share, which would put a host-level root + # in the rooted-driveless space and stop it containing its own shares -- the bug this + # module exists to fix. + in_unc_space = ( + windows + and text.startswith(_UNC_ANCHOR) + and not text.startswith(_EXTENDED_PREFIX) + and not text.startswith(_DEVICE_PREFIX) + ) text = path_module.normpath(text) + if in_unc_space and not text.startswith(_UNC_ANCHOR): + # Those same versions collapse the leading pair itself ('\\host' -> '\host'). + text = _UNC_ANCHOR + text.lstrip(path_module.sep) if normalize_case: text = path_module.normcase(text) @@ -78,30 +129,18 @@ def _split_anchored(path: Any, path_module: Any, normalize_case: bool) -> tuple[ # '//foo' and '/foo' are the same file on the platforms this client targets. return (path_module.sep if root else ""), parts - if drive[:4] in ("\\\\?\\", "\\\\.\\"): - # These prefixes disable normalization: the drive is a whole anchor, so it never - # aliases the plain path it resembles, and a share root and its files -- which - # differ only by a trailing separator -- still take the same anchor. + if drive.startswith(_EXTENDED_PREFIX) or drive.startswith(_DEVICE_PREFIX): + # Whatever reaches here has no plain spelling to fold to, so the drive is a whole + # anchor occupying its own space. The anchor carries its own trailing separator, so + # a share root and the files under it -- which differ only by it -- still match. return drive + path_module.sep, parts - if drive.startswith(_UNC_ANCHOR): - return _UNC_ANCHOR, [p for p in drive[len(_UNC_ANCHOR) :].split("\\") if p] + parts + if in_unc_space: + # The server and share are ordinary parts beneath the bare anchor, which is what + # lets a host-level root contain the shares under it. + return _UNC_ANCHOR, [p for p in text[len(_UNC_ANCHOR) :].split(path_module.sep) if p] return drive + root, parts -def _leading_pardir_count(parts: list[str]) -> int: - """Count the leading '..' run that ``normpath`` could not resolve. - - Counted on parts rather than whole components because an anchor can precede the run - ('C:..\\x' is the parent of the working directory on drive C:). - """ - count = 0 - for part in parts: - if part != _PARDIR: - break - count += 1 - return count - - def path_components( path: Any, *, @@ -115,12 +154,28 @@ def path_components( the rest are the path's parts, so comparing these lists component-wise confuses neither one path space for another nor a string prefix for a directory prefix. + An extended-length prefix folds to the plain path it denotes, so ``\\\\?\\C:\\a`` and + ``C:\\a`` yield the same components. Prefixed forms that denote no plain path keep a + space of their own. + ``normalize_case`` lowercases components on Windows to match the filesystem. """ anchor, parts = _split_anchored(path, path_module, normalize_case) return ([anchor] if anchor else []) + parts +def normalized_path(path: Any, *, path_module: Any = os.path) -> str: + """Return ``path`` with ``..``, ``.``, repeated separators and separator style resolved. + + ``path_module.normpath`` with the version differences handled: before Python 3.11 it + collapses the leading pair on a UNC path that names no share (``\\\\host`` -> ``\\host``), + moving a host-level root out of the UNC space so it matches none of its own shares. + Case is preserved, unlike the components used for comparison. + """ + anchor, parts = _split_anchored(path, path_module, normalize_case=False) + return anchor + path_module.sep.join(parts) + + def is_path_contained( path: Any, root: Any, @@ -154,40 +209,3 @@ def is_any_path_contained( ) -> bool: """Return True iff ``path`` is contained by any root in ``roots``.""" return any(is_path_contained(path, root, path_module=path_module) for root in roots) - - -def common_ancestor(paths: Sequence[Any], *, path_module: Any = os.path) -> str: - """Return the deepest directory containing every path in ``paths``. - - This is ``os.path.commonpath`` without the exceptions: paths in unrelated spaces return - ``""`` rather than raising, and a UNC host is a valid answer for paths on different - shares of one server. The result keeps the first path's spelling and, like - ``commonpath``, is purely lexical. - """ - if not paths: - return "" - - split = [_split_anchored(p, path_module, normalize_case=True) for p in paths] - normalized = [([a] if a else []) + parts for a, parts in split] - anchor, spelled_parts = _split_anchored(paths[0], path_module, normalize_case=False) - spelled = ([anchor] if anchor else []) + spelled_parts - - # '..' and '../..' are rooted at different unknown places, so runs of differing depth - # share nothing. Comparing them positionally would return the shallower path, which is - # not an ancestor of the deeper one -- os.path.commonpath has that bug. - if len({_leading_pardir_count(parts) for _, parts in split}) > 1: - return "" - - shared = min(len(components) for components in normalized) - while shared > 0 and any(other[:shared] != normalized[0][:shared] for other in normalized): - shared -= 1 - if shared == 0: - return "" - # Matching only the bare anchor means different servers, so no shared directory. - if shared == 1 and normalized[0][0] == _UNC_ANCHOR: - return "" - - # The anchor carries its own separator, so it abuts the first part directly. - if anchor: - return anchor + path_module.sep.join(spelled[1:shared]) - return path_module.sep.join(spelled[:shared]) diff --git a/src/deadline/client/api/_submit_job_bundle.py b/src/deadline/client/api/_submit_job_bundle.py index 4eb6634b5..43bf220c6 100644 --- a/src/deadline/client/api/_submit_job_bundle.py +++ b/src/deadline/client/api/_submit_job_bundle.py @@ -71,7 +71,7 @@ summarize_path_list, ) from ...job_attachments.api._hashing import _hash_attachments -from .._path_utils import is_any_path_contained, path_components +from .._path_utils import is_any_path_contained, normalized_path, path_components logger = logging.getLogger(__name__) @@ -308,9 +308,14 @@ def _filter_redundant_known_paths(known_asset_paths: Iterable[str]) -> list[str] """ # Passed explicitly, and read at call time, so tests can patch it for another platform. expanded = (os.path.expanduser(path) for path in known_asset_paths if path) - # normpath, not abspath: dedupes equivalent spellings without consulting the cwd. + # normalized_path, not abspath: dedupes equivalent spellings without consulting the cwd. + # Not os.path.normpath, which before Python 3.11 collapses the leading pair on a + # host-level UNC root ('\\host' -> '\host'), moving it out of the UNC space so it then + # matches none of its own shares -- and this list is what _is_known_path compares. ordered = list( - dict.fromkeys(os.path.normpath(path) for path in expanded if os.path.isabs(path)) + dict.fromkeys( + normalized_path(path, path_module=os.path) for path in expanded if os.path.isabs(path) + ) ) components = {path: path_components(path, path_module=os.path) for path in ordered} # This directory tree gets filled with the known asset paths, with diff --git a/src/deadline/client/cli/_groups/job_group.py b/src/deadline/client/cli/_groups/job_group.py index 6aba259b3..d3c3a8ef9 100644 --- a/src/deadline/client/cli/_groups/job_group.py +++ b/src/deadline/client/cli/_groups/job_group.py @@ -41,7 +41,7 @@ from ... import api from ...config import config_file -from ..._path_utils import common_ancestor +from ..._path_summary import common_ancestor from ...exceptions import DeadlineOperationError, DeadlineOperationTimedOut from .._common import ( _OUTPUT_FORMAT_HELP, diff --git a/test/integ/windows_smb/test_unc_path_containment.py b/test/integ/windows_smb/test_unc_path_containment.py index f7bf7be78..14acad6a8 100644 --- a/test/integ/windows_smb/test_unc_path_containment.py +++ b/test/integ/windows_smb/test_unc_path_containment.py @@ -25,7 +25,8 @@ import pytest -from deadline.client._path_utils import common_ancestor, is_path_contained +from deadline.client._path_summary import common_ancestor +from deadline.client._path_utils import is_path_contained from deadline.client.api._submit_job_bundle import ( _filter_redundant_known_paths, _is_known_path, diff --git a/test/unit/deadline_client/test_path_summary.py b/test/unit/deadline_client/test_path_summary.py new file mode 100644 index 000000000..514577e7f --- /dev/null +++ b/test/unit/deadline_client/test_path_summary.py @@ -0,0 +1,109 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + +""" +Tests for the path-group summary helper. + +Windows semantics go through an explicit ``ntpath`` so these run on every platform. +""" + +import ntpath +import posixpath + +import pytest + +from deadline.client._path_summary import common_ancestor +from deadline.client._path_utils import is_path_contained + + +@pytest.mark.parametrize("path_module", [ntpath, posixpath]) +def test_common_ancestor_contains_its_inputs(path_module): + """A non-empty common_ancestor must contain every path it was derived from.""" + paths = ( + [ + r"\\host\share\a", + r"\\host\s2\b", + r"C:\a\b", + r"C:\a\c", + "C:foo", + r"..\a\b", + r"..\a\c", + # Drive-relative '..' puts the run behind an anchor, where a guard counting + # from index zero would miss it. + r"C:..\x", + r"C:..\..\x", + r"C:..\a\y", + r"\\?\C:", + r"\\?\C:\a", + ] + if path_module is ntpath + else ["/a/b", "/a/c", "//a/d", "../a/b", "../a/c", "../../a/b", "rel/f", "rel/g"] + ) + for first in paths: + for second in paths: + ancestor = common_ancestor([first, second], path_module=path_module) + if not ancestor: + continue + assert is_path_contained(first, ancestor, path_module=path_module), (first, ancestor) + assert is_path_contained(second, ancestor, path_module=path_module), (second, ancestor) + + +@pytest.mark.parametrize( + "paths, path_module, expected", + [ + # The common ancestor of paths under one share, spelled with its real case. + ( + [r"\\host\Share\Proj\a.txt", r"\\host\Share\Proj\sub\b.txt"], + ntpath, + r"\\host\Share\Proj", + ), + # Different shares on one host share only the host. os.path.commonpath raises + # ValueError for this pair. + ([r"\\host\s1\a", r"\\host\s2\b"], ntpath, r"\\host"), + # Different hosts share nothing. There is no location above a UNC host, so the + # bare '\\\\' that their leading components have in common is not an answer. + ([r"\\host1\s\a", r"\\host2\s\b"], ntpath, ""), + ([r"\\host1", r"\\host2"], ntpath, ""), + # Different drives share nothing. + ([r"C:\a\b", r"D:\a\b"], ntpath, ""), + ([r"C:\a\b", r"\\host\share\b"], ntpath, ""), + ([r"C:\proj\a", r"C:\proj\b"], ntpath, r"C:\proj"), + ([r"C:\proj\a"], ntpath, r"C:\proj\a"), + (["/a/b/c", "/a/b/d"], posixpath, "/a/b"), + (["/a/b", "/c/d"], posixpath, "/"), + # A doubled POSIX root is the same space as '/', so these behave like ordinary + # absolute paths rather than a separate namespace. + (["//a/b", "//c/d"], posixpath, "/"), + (["//a/b", "//a/c"], posixpath, "/a"), + (["/a", "/b"], posixpath, "/"), + (["/a/b"], posixpath, "/a/b"), + (["a/b", "/c/d"], posixpath, ""), + ([], posixpath, ""), + # Paths whose unresolved leading '..' runs differ in depth are rooted at + # different unknown directories, so they share none. Positional comparison + # would wrongly read the shared '..' as one directory and return '..', which + # is not an ancestor of '../../up'. os.path.commonpath has that bug. + (["../up", "../../up"], posixpath, ""), + (["../../up", "../up"], posixpath, ""), + ([r"..\up", r"..\..\up"], ntpath, ""), + # The '..' run can sit behind an anchor, where a guard counting from index 0 + # would not see it. 'C:..' is the cwd's parent on C:, 'C:..\..' its grandparent. + ([r"C:..\x", r"C:..\..\x"], ntpath, ""), + ([r"C:..", r"C:..\.."], ntpath, ""), + # Equal depth behind an anchor is still comparable. + ([r"C:..\a\x", r"C:..\a\y"], ntpath, r"C:..\a"), + # Equal '..' depth is comparable again. + (["../a/x", "../a/y"], posixpath, "../a"), + (["../../a/x", "../../a/y"], posixpath, "../../a"), + # Relative inputs keep their own spelling and gain no leading separator. A + # Windows-style path read under posixpath semantics is one of these, since + # 'C:' is an ordinary component there rather than a drive. + (["a/b/c", "a/b/d"], posixpath, "a/b"), + ( + ["C:/Users/u/renders/i1.png", "C:/Users/u/renders/i2.png"], + posixpath, + "C:/Users/u/renders", + ), + ], +) +def test_common_ancestor(paths, path_module, expected): + assert common_ancestor(paths, path_module=path_module) == expected diff --git a/test/unit/deadline_client/test_path_utils.py b/test/unit/deadline_client/test_path_utils.py index 9ea413ae3..a2c83445f 100644 --- a/test/unit/deadline_client/test_path_utils.py +++ b/test/unit/deadline_client/test_path_utils.py @@ -19,9 +19,9 @@ from deadline.client._path_utils import ( _splitroot, - common_ancestor, is_any_path_contained, is_path_contained, + normalized_path, path_components, ) @@ -84,17 +84,19 @@ # Relative paths are not contained by absolute roots and vice versa. (r"relative\file", r"C:\trusted", False), (r"C:\trusted\file", r"relative", False), - # An extended-length path occupies its own path space rather than being folded into - # the plain form it denotes, so comparing across the two spellings fails closed. No - # caller needs the fold: every call site feeds realpath output or isabs-filtered - # roots, neither of which carries a '\\?\' prefix. - (r"\\?\C:\trusted\project\file", r"C:\trusted\project", False), - (r"C:\trusted\project\file", r"\\?\C:\trusted\project", False), - (r"\\?\UNC\host\share\file", r"\\host\share", False), - (r"\\host\share\file", r"\\?\UNC\host\share", False), - (r"\\?\UNC\host\share\file", r"\\host", False), + # An extended-length prefix only turns off Win32 normalization; it denotes an + # ordinary location, so it folds to the plain spelling and compares equal to it in + # either direction. job-attachments carries the '\\?\' form through its internals + # and strips it only at display boundaries, so a prefixed path does reach here. + (r"\\?\C:\trusted\project\file", r"C:\trusted\project", True), + (r"C:\trusted\project\file", r"\\?\C:\trusted\project", True), + (r"\\?\UNC\host\share\file", r"\\host\share", True), + (r"\\host\share\file", r"\\?\UNC\host\share", True), + (r"\\?\UNC\host\share\file", r"\\host", True), (r"\\?\C:\trusted\project\file", r"\\?\C:\trusted\project", True), (r"\\?\UNC\host\share\file", r"\\?\UNC\host\share", True), + # Folding does not weaken component anchoring: a sibling that merely shares a + # string prefix is still outside the root. (r"\\?\C:\trusted\project-secret\f", r"\\?\C:\trusted\project", False), # A rooted, driveless root ('\') is a different path space than the UNC # namespace, so it must not contain remote paths -- nor they it. @@ -123,13 +125,14 @@ (r"C:\secret", r"\\.\C:", False), (r"\\?\Volume{abc}\trusted\f", r"Volume{abc}\trusted", False), (r"\\?\Volume{abc}\trusted\f", r"\\?\Volume{abc}\trusted", True), - # '\\?\C:' must contain paths in neither the drive-relative 'C:' space nor plain - # 'C:\'. isabs reports it absolute, so a caller filtering on that lets it through. - ("C:foo", r"\\?\C:", False), + # '\\?\C:' folds to the drive-relative 'C:' space and '\\?\C:\' to the drive root, + # so each behaves as the plain spelling it denotes -- including keeping those two + # spaces apart, which is why the last two disagree. + ("C:foo", r"\\?\C:", True), (r"C:\a", r"\\?\C:", False), - (r"C:\a\f", "\\\\?\\C:\\", False), - # Within its own space it behaves like the drive root it spells. - (r"\\?\C:\a", r"\\?\C:", True), + (r"C:\a\f", "\\\\?\\C:\\", True), + (r"\\?\C:\a", "\\\\?\\C:\\", True), + (r"\\?\C:\a", r"\\?\C:", False), # A relative path is contained in itself even when normpath leaves a leading # '..' it cannot cancel; only a '..' below the root can climb back out. (r"..\a", r"..\a", True), @@ -211,36 +214,128 @@ def test_is_path_contained_is_reflexive(path_module): assert is_path_contained(path, path, path_module=path_module) is True, path -@pytest.mark.parametrize("path_module", [ntpath, posixpath]) -def test_common_ancestor_contains_its_inputs(path_module): - """A non-empty common_ancestor must contain every path it was derived from.""" - paths = ( - [ - r"\\host\share\a", - r"\\host\s2\b", - r"C:\a\b", - r"C:\a\c", - "C:foo", - r"..\a\b", - r"..\a\c", - # Drive-relative '..' puts the run behind an anchor, where a guard counting - # from index zero would miss it. - r"C:..\x", - r"C:..\..\x", - r"C:..\a\y", - r"\\?\C:", - r"\\?\C:\a", - ] - if path_module is ntpath - else ["/a/b", "/a/c", "//a/d", "../a/b", "../a/c", "../../a/b", "rel/f", "rel/g"] - ) - for first in paths: - for second in paths: - ancestor = common_ancestor([first, second], path_module=path_module) - if not ancestor: - continue - assert is_path_contained(first, ancestor, path_module=path_module), (first, ancestor) - assert is_path_contained(second, ancestor, path_module=path_module), (second, ancestor) +@pytest.mark.parametrize( + "root, contained", + [ + # The reported case: a host-level root and a file on one of its shares. Before + # Python 3.11 both normpath and splitdrive strip a share-less UNC path down to a + # rooted-driveless one ('\\host' -> '\host', splitdrive -> no drive), which put the + # root in a different path space than the candidate and left #1321 unfixed on 3.9 + # and 3.10. + (r"\\host", True), + ("\\\\host\\", True), + (r"\\host\share", True), + # A different server, and the bare anchor that names none, must not contain it. + (r"\\host2", False), + ("\\\\", False), + ("\\", False), + ], +) +def test_host_level_unc_root_containment_is_version_independent(root, contained): + """Issue #1321 on every supported interpreter, not just 3.10+.""" + assert is_path_contained(r"\\host\share\f", root, path_module=ntpath) is contained + + +class _PreThreeElevenNtpath: + """``ntpath`` as it behaved before Python 3.11 for a UNC path that names no share. + + Both ``normpath`` and ``splitdrive`` stripped such a path down to a rooted, driveless + one. Injecting this exercises that branch on any interpreter, rather than only on the + 3.9 and 3.10 jobs -- the same reason the rest of this file injects ``ntpath``. + """ + + # Forces the _splitroot backport, which is what those versions had. + splitroot = None + + @staticmethod + def _is_shareless_unc(text: str) -> bool: + return text.startswith("\\\\") and "\\" not in text[2:] + + @staticmethod + def normpath(text: str) -> str: + result = ntpath.normpath(text) + if _PreThreeElevenNtpath._is_shareless_unc(result): + return result[1:] + return result + + @staticmethod + def splitdrive(text: str): + if _PreThreeElevenNtpath._is_shareless_unc(text): + return "", text + return ntpath.splitdrive(text) + + def __getattr__(self, name): + return getattr(ntpath, name) + + +def test_host_level_unc_root_survives_pre_3_11_normpath(): + """A host-level root stays in the UNC space even when normpath collapses its anchor.""" + legacy: Any = _PreThreeElevenNtpath() + # Confirm the proxy actually reproduces the old behavior, so this cannot pass vacuously. + assert legacy.normpath("\\\\host") == "\\host" + assert legacy.splitdrive("\\\\host") == ("", "\\\\host") + + assert path_components(r"\\host", path_module=legacy) == ["\\\\", "host"] + assert is_path_contained(r"\\host\share\f", r"\\host", path_module=legacy) is True + assert normalized_path(r"\\host", path_module=legacy) == r"\\host" + # A rooted, driveless path must not be promoted into the UNC space by the restore. + assert path_components(r"\host", path_module=legacy) == ["\\", "host"] + assert is_path_contained(r"\\host\share\f", "\\", path_module=legacy) is False + + +@pytest.mark.parametrize( + "prefixed, plain", + [ + (r"\\?\C:\proj\a.txt", r"C:\proj\a.txt"), + (r"\\?\UNC\host\share\a.txt", r"\\host\share\a.txt"), + ], +) +def test_extended_length_prefix_agrees_with_plain_spelling(prefixed, plain): + """A prefixed path is contained by exactly the roots its plain spelling is. + + job-attachments carries the '\\\\?\\' form through its internals and strips it only at + display boundaries, so a prefixed path can reach a containment check. Treating it as its + own path space would report it outside a root that plainly contains it. + """ + roots = [ + plain, + ntpath.dirname(plain), + r"C:\proj", + "C:\\", + r"\\host\share", + r"\\host", + r"D:\other", + ] + for root in roots: + assert is_path_contained(prefixed, root, path_module=ntpath) is is_path_contained( + plain, root, path_module=ntpath + ), root + # A prefixed *root* folds the same way, so it behaves like its plain spelling. + assert is_path_contained(plain, root, path_module=ntpath) is is_path_contained( + plain, _prefixed_form(root), path_module=ntpath + ), root + + +def _prefixed_form(path: str) -> str: + """Spell ``path`` in extended-length form.""" + if path.startswith("\\\\"): + return "\\\\?\\UNC" + path[1:] + return "\\\\?\\" + path + + +def test_extended_length_prefix_resolves_dot_segments_uniformly(): + """normpath leaves '..' alone inside a '\\\\?\\' path before 3.10 and collapses it after. + + Folding to the plain spelling first makes the components the same on every supported + interpreter, so containment does not depend on the running Python. + """ + assert path_components(r"\\?\C:\a\..\b", path_module=ntpath) == ["c:\\", "b"] + assert path_components(r"\\?\UNC\host\share\a\..\b", path_module=ntpath) == [ + "\\\\", + "host", + "share", + "b", + ] @pytest.mark.parametrize("path_module", [ntpath, posixpath]) @@ -382,14 +477,25 @@ def test_is_any_path_contained(): # A rooted, driveless path is its own space, distinct from the UNC anchor. ("\\", ntpath, ["\\"]), ("\\\\", ntpath, ["\\\\"]), - # A prefixed drive keeps its prefix and stays whole, so it occupies a space of - # its own and cannot alias the plain drive or UNC path it resembles. - (r"\\?\C:\a", ntpath, ["\\\\?\\c:\\", "a"]), - # The anchor carries its own trailing separator, so a share root and a file - # under it share an anchor and containment holds between them. - (r"\\?\UNC\host\share", ntpath, ["\\\\?\\unc\\host\\share\\"]), - (r"\\?\UNC\host\share\f", ntpath, ["\\\\?\\unc\\host\\share\\", "f"]), + # An extended-length prefix only turns off Win32 normalization: it denotes the + # same location, so it folds to the plain spelling rather than occupying a space + # of its own. Otherwise a prefixed path reads as outside a root that plainly + # contains it, which is the same false negative as issue #1321. + (r"\\?\C:\a", ntpath, ["c:\\", "a"]), + (r"\\?\c:\a", ntpath, ["c:\\", "a"]), + (r"\\?\C:", ntpath, ["c:"]), + ("//?/C:/a", ntpath, ["c:\\", "a"]), + (r"\\?\UNC\host\share", ntpath, ["\\\\", "host", "share"]), + (r"\\?\UNC\host\share\f", ntpath, ["\\\\", "host", "share", "f"]), + (r"\\?\unc\host\share\f", ntpath, ["\\\\", "host", "share", "f"]), + (r"\\?\UNC\host", ntpath, ["\\\\", "host"]), + # 'UNC' alone names no server, so it folds to the bare anchor, which contains + # nothing rather than prefixing every reachable share. + (r"\\?\UNC", ntpath, ["\\\\"]), + # These denote no plain path, so they keep their prefix and a space of their own + # and cannot alias the drive or UNC path they resemble. (r"\\?\Volume{abc}\a", ntpath, ["\\\\?\\volume{abc}\\", "a"]), + (r"\\?\GLOBALROOT\Device\X\f", ntpath, ["\\\\?\\globalroot\\", "device", "x", "f"]), (r"\\.\C:\a", ntpath, ["\\\\.\\c:\\", "a"]), ("/", posixpath, ["/"]), ("/a/b", posixpath, ["/", "a", "b"]), @@ -405,6 +511,33 @@ def test_path_components(path, path_module, expected): assert path_components(path, path_module=path_module) == expected +@pytest.mark.parametrize( + "path, path_module, expected", + [ + # The reason this exists rather than calling normpath directly: before Python 3.11 + # normpath collapses the leading pair on a UNC path that names no share, which moves + # a host-level known-asset root out of the UNC space so it matches none of its own + # shares. _filter_redundant_known_paths feeds its output to _is_known_path. + (r"\\host", ntpath, r"\\host"), + ("\\\\host\\", ntpath, r"\\host"), + ("\\\\", ntpath, "\\\\"), + (r"\\host\share\a\..\b", ntpath, r"\\host\share\b"), + # Case is preserved, unlike the components used for comparison. + (r"\\Host\Share", ntpath, r"\\Host\Share"), + (r"C:\A\.\b\..\c", ntpath, r"C:\A\c"), + ("C:/a/b", ntpath, r"C:\a\b"), + # An extended-length prefix folds to the plain path it denotes. + (r"\\?\C:\a", ntpath, r"C:\a"), + (r"\\?\UNC\host\share\f", ntpath, r"\\host\share\f"), + ("/a/", posixpath, "/a"), + ("/a/b/../c", posixpath, "/a/c"), + ("/", posixpath, "/"), + ], +) +def test_normalized_path(path, path_module, expected): + assert normalized_path(path, path_module=path_module) == expected + + def test_path_components_preserves_case_when_asked(): assert path_components(r"\\Host\Share\File", path_module=ntpath, normalize_case=False) == [ "\\\\", @@ -412,65 +545,3 @@ def test_path_components_preserves_case_when_asked(): "Share", "File", ] - - -@pytest.mark.parametrize( - "paths, path_module, expected", - [ - # The common ancestor of paths under one share, spelled with its real case. - ( - [r"\\host\Share\Proj\a.txt", r"\\host\Share\Proj\sub\b.txt"], - ntpath, - r"\\host\Share\Proj", - ), - # Different shares on one host share only the host. os.path.commonpath raises - # ValueError for this pair. - ([r"\\host\s1\a", r"\\host\s2\b"], ntpath, r"\\host"), - # Different hosts share nothing. There is no location above a UNC host, so the - # bare '\\\\' that their leading components have in common is not an answer. - ([r"\\host1\s\a", r"\\host2\s\b"], ntpath, ""), - ([r"\\host1", r"\\host2"], ntpath, ""), - # Different drives share nothing. - ([r"C:\a\b", r"D:\a\b"], ntpath, ""), - ([r"C:\a\b", r"\\host\share\b"], ntpath, ""), - ([r"C:\proj\a", r"C:\proj\b"], ntpath, r"C:\proj"), - ([r"C:\proj\a"], ntpath, r"C:\proj\a"), - (["/a/b/c", "/a/b/d"], posixpath, "/a/b"), - (["/a/b", "/c/d"], posixpath, "/"), - # A doubled POSIX root is the same space as '/', so these behave like ordinary - # absolute paths rather than a separate namespace. - (["//a/b", "//c/d"], posixpath, "/"), - (["//a/b", "//a/c"], posixpath, "/a"), - (["/a", "/b"], posixpath, "/"), - (["/a/b"], posixpath, "/a/b"), - (["a/b", "/c/d"], posixpath, ""), - ([], posixpath, ""), - # Paths whose unresolved leading '..' runs differ in depth are rooted at - # different unknown directories, so they share none. Positional comparison - # would wrongly read the shared '..' as one directory and return '..', which - # is not an ancestor of '../../up'. os.path.commonpath has that bug. - (["../up", "../../up"], posixpath, ""), - (["../../up", "../up"], posixpath, ""), - ([r"..\up", r"..\..\up"], ntpath, ""), - # The '..' run can sit behind an anchor, where a guard counting from index 0 - # would not see it. 'C:..' is the cwd's parent on C:, 'C:..\..' its grandparent. - ([r"C:..\x", r"C:..\..\x"], ntpath, ""), - ([r"C:..", r"C:..\.."], ntpath, ""), - # Equal depth behind an anchor is still comparable. - ([r"C:..\a\x", r"C:..\a\y"], ntpath, r"C:..\a"), - # Equal '..' depth is comparable again. - (["../a/x", "../a/y"], posixpath, "../a"), - (["../../a/x", "../../a/y"], posixpath, "../../a"), - # Relative inputs keep their own spelling and gain no leading separator. A - # Windows-style path read under posixpath semantics is one of these, since - # 'C:' is an ordinary component there rather than a drive. - (["a/b/c", "a/b/d"], posixpath, "a/b"), - ( - ["C:/Users/u/renders/i1.png", "C:/Users/u/renders/i2.png"], - posixpath, - "C:/Users/u/renders", - ), - ], -) -def test_common_ancestor(paths, path_module, expected): - assert common_ancestor(paths, path_module=path_module) == expected From 890c0581fc1fff3ef280a4ea54189c08c2c7581c Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Wed, 12 Aug 2026 21:19:58 -0700 Subject: [PATCH 03/28] fix: stop ntpath.isabs reading a UNC share as a relative path Fourth site of the same version-dependent defect, and the remaining cause of the red CI. Before Python 3.11 ntpath.isabs tests what splitdrive leaves behind, and for a UNC path that names a share splitdrive consumes the whole string: 3.9 / 3.10 : ntpath.isabs(r"\\host\share") -> False 3.11+ : -> True Three call sites gate trust on that answer: - _filter_redundant_known_paths dropped every UNC root naming a share, since it drops roots that are not absolute. That is why 4 of its cases still failed after the previous commit. - The pre-submission hook check requires PATH values to be absolute, so a hook emitting a valid UNC path was rejected as relative -- on exactly the setup issue #1321 reports. - The PATH-default check requires the opposite, so an absolute UNC default slipped past it. That one still failed closed on the containment check immediately after, but reported the wrong reason. All three now use is_absolute_path, which derives the answer from the anchor the rest of the module already computes. It is deliberately as strict as the newest stdlib rather than as loose as the oldest: a drive-relative path ('C:x') needs the working directory on that drive and a rooted, driveless one ('\x') needs the current drive, so neither may be trusted as a root. ntpath.isabs accepted the latter until 3.13. Verified against the stdlib on 3.9, 3.10, 3.11 and 3.14: identical to 3.14 on every case, and never accepts something the running interpreter's isabs rejects except a UNC share, which is the defect being fixed. A test pins that property so the helper cannot quietly widen. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- src/deadline/client/_path_utils.py | 23 ++++++ src/deadline/client/api/_submit_job_bundle.py | 16 ++++- src/deadline/client/job_bundle/parameters.py | 7 +- test/unit/deadline_client/test_path_utils.py | 72 ++++++++++++++++++- 4 files changed, 111 insertions(+), 7 deletions(-) diff --git a/src/deadline/client/_path_utils.py b/src/deadline/client/_path_utils.py index 89e048af0..7816eb33f 100644 --- a/src/deadline/client/_path_utils.py +++ b/src/deadline/client/_path_utils.py @@ -24,6 +24,7 @@ from typing import Any, Iterable __all__ = [ + "is_absolute_path", "is_any_path_contained", "is_path_contained", "normalized_path", @@ -164,6 +165,28 @@ def path_components( return ([anchor] if anchor else []) + parts +def is_absolute_path(path: Any, *, path_module: Any = os.path) -> bool: + """Return True iff ``path`` names a location without consulting the working directory. + + ``path_module.isabs`` cannot be used before Python 3.11: it tests what ``splitdrive`` + leaves behind, and for a UNC path that names a share ``splitdrive`` consumes the whole + string, so ``isabs(r'\\\\host\\share')`` is False there. Callers use this to decide + whether a path may be trusted as a root or accepted as a parameter value, so a UNC + share silently reading as relative drops valid roots and rejects valid values. + + Only a fully qualified path counts. On Windows a drive-relative anchor (``C:x``) needs + the working directory on that drive and a rooted, driveless one (``\\x``) needs the + current drive, so neither qualifies -- ``ntpath.isabs`` accepted ``\\x`` until 3.13, and + this is deliberately as strict as the newest stdlib rather than as loose as the oldest. + """ + anchor, _ = _split_anchored(path, path_module, normalize_case=True) + if not anchor: + return False + if path_module.sep != "\\": + return True + return not _denotes_drive(anchor) and anchor != path_module.sep + + def normalized_path(path: Any, *, path_module: Any = os.path) -> str: """Return ``path`` with ``..``, ``.``, repeated separators and separator style resolved. diff --git a/src/deadline/client/api/_submit_job_bundle.py b/src/deadline/client/api/_submit_job_bundle.py index 43bf220c6..baf5b5df9 100644 --- a/src/deadline/client/api/_submit_job_bundle.py +++ b/src/deadline/client/api/_submit_job_bundle.py @@ -71,7 +71,12 @@ summarize_path_list, ) from ...job_attachments.api._hashing import _hash_attachments -from .._path_utils import is_any_path_contained, normalized_path, path_components +from .._path_utils import ( + is_absolute_path, + is_any_path_contained, + normalized_path, + path_components, +) logger = logging.getLogger(__name__) @@ -314,7 +319,9 @@ def _filter_redundant_known_paths(known_asset_paths: Iterable[str]) -> list[str] # matches none of its own shares -- and this list is what _is_known_path compares. ordered = list( dict.fromkeys( - normalized_path(path, path_module=os.path) for path in expanded if os.path.isabs(path) + normalized_path(path, path_module=os.path) + for path in expanded + if is_absolute_path(path, path_module=os.path) ) ) components = {path: path_components(path, path_module=os.path) for path in ordered} @@ -807,7 +814,10 @@ def _path_parameter_known_paths(resolved_parameters): bundle_parameter_types.get(name) == "PATH" and isinstance(value, str) and value != "" - and not os.path.isabs(value) + # Not os.path.isabs: before Python 3.11 it reads a UNC path naming a + # share as relative, rejecting a valid value on the very setup #1321 + # reports. + and not is_absolute_path(value, path_module=os.path) ): raise DeadlineOperationError( f"Pre-submission hook emitted a relative PATH value for parameter " diff --git a/src/deadline/client/job_bundle/parameters.py b/src/deadline/client/job_bundle/parameters.py index fd2fa2a46..40ec94729 100644 --- a/src/deadline/client/job_bundle/parameters.py +++ b/src/deadline/client/job_bundle/parameters.py @@ -23,7 +23,7 @@ NotRequired = object TypedDict = object -from .._path_utils import is_path_contained +from .._path_utils import is_absolute_path, is_path_contained from ..exceptions import DeadlineOperationError from .loader import read_yaml_or_json_object @@ -794,7 +794,10 @@ def read_job_bundle_parameters(bundle_dir: str) -> list[JobParameter]: ): default = parameter.get("default") if default: - if os.path.isabs(default): + # Not os.path.isabs, which before Python 3.11 reads a UNC path naming a + # share as relative -- such a default reached the containment check below + # and failed there, reporting the wrong reason. + if is_absolute_path(default, path_module=os.path): raise DeadlineOperationError( f"Job Template for job bundle {bundle_dir}:\nDefault PATH '{default}' for parameter '{name}' is absolute.\nPATH values must be relative, and must resolve within the Job Bundle directory." ) diff --git a/test/unit/deadline_client/test_path_utils.py b/test/unit/deadline_client/test_path_utils.py index a2c83445f..d7576e3cf 100644 --- a/test/unit/deadline_client/test_path_utils.py +++ b/test/unit/deadline_client/test_path_utils.py @@ -19,6 +19,7 @@ from deadline.client._path_utils import ( _splitroot, + is_absolute_path, is_any_path_contained, is_path_contained, normalized_path, @@ -104,8 +105,9 @@ ("\\x", "\\\\", False), ("\\x", "\\", True), # The bare anchor names no server, so it is an ancestor of nothing -- treating it as - # POSIX '/' would trust every reachable share. ntpath.isabs('\\') is True, so a - # caller filtering roots on that lets it through; '//' and '\\?\UNC\' normalize to it. + # POSIX '/' would trust every reachable share. It counts as fully qualified, so a + # caller filtering roots on is_absolute_path lets it reach here; '//' and + # '\\?\UNC\' normalize to it. (r"\\host\share\file", "\\\\", False), (r"\\host\share\file", "\\\\\\\\", False), (r"\\host\share\file", "//", False), @@ -511,6 +513,72 @@ def test_path_components(path, path_module, expected): assert path_components(path, path_module=path_module) == expected +@pytest.mark.parametrize( + "path, path_module, expected", + [ + # The reason this exists rather than calling isabs directly: before Python 3.11 + # ntpath.isabs tests what splitdrive leaves behind, and for a UNC path naming a + # share splitdrive consumes the whole string -- so isabs(r"\\host\s1") is False + # there. Callers gate trust on this, so a valid root was dropped and a valid + # PATH parameter value rejected. + (r"\\host\s1", ntpath, True), + (r"\\host\s1\f", ntpath, True), + ("\\\\host\\", ntpath, True), + (r"\\host", ntpath, True), + # The bare anchor names no server but is still fully qualified, so it reaches the + # containment check -- which rejects it, as test_is_path_contained_windows pins. + ("\\\\", ntpath, True), + (r"C:\a", ntpath, True), + ("C:\\", ntpath, True), + (r"\\?\C:\a", ntpath, True), + (r"\\?\UNC\host\share", ntpath, True), + (r"\\.\C:\a", ntpath, True), + # Drive-relative needs the working directory on that drive; rooted-driveless needs + # the current drive. Neither is fully qualified, so neither may be trusted as a + # root. ntpath.isabs accepted the latter until 3.13. + ("C:", ntpath, False), + ("C:foo", ntpath, False), + ("\\", ntpath, False), + (r"\x", ntpath, False), + ("rel", ntpath, False), + (r"rel\f", ntpath, False), + (r"..\a", ntpath, False), + ("", ntpath, False), + (".", ntpath, False), + ("/a", posixpath, True), + ("//a", posixpath, True), + ("rel/f", posixpath, False), + ("../a", posixpath, False), + ("", posixpath, False), + ], +) +def test_is_absolute_path(path, path_module, expected): + assert is_absolute_path(path, path_module=path_module) is expected + + +def test_is_absolute_path_is_at_least_as_strict_as_the_stdlib(): + """Never accept something ``isabs`` rejects on the running interpreter. + + This is what makes the helper safe to swap in at trust boundaries: it may be stricter + than the stdlib (it is, for rooted-driveless paths before 3.13), and it may accept a UNC + share that older versions wrongly rejected, but it must not otherwise widen what counts + as absolute. + """ + for path_module in (ntpath, posixpath): + corpus = ( + [r"\\host", r"\\host\s1", "\\", r"\x", "C:", "C:foo", r"C:\a", "rel", ""] + if path_module is ntpath + else ["/", "/a", "rel", "../a", ""] + ) + for path in corpus: + if not is_absolute_path(path, path_module=path_module): + continue + # The only sanctioned widening: a UNC path whose share splitdrive swallowed. + if not path_module.isabs(path): + assert path_module is ntpath, path + assert path.startswith("\\\\"), path + + @pytest.mark.parametrize( "path, path_module, expected", [ From aa4f4996e0c56dc77a3f0976caf1f7b7d5c79f55 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Wed, 12 Aug 2026 21:36:10 -0700 Subject: [PATCH 04/28] fix: keep a rooted, driveless root absolute on every Python version ntpath.isabs disagrees with itself twice across the supported range, not once. The previous commit handled the first disagreement and adopted the newest stdlib's answer for the second, which was wrong: ntpath.isabs(r"\x") 3.9-3.12: True 3.13+: False ntpath.isabs(r"\\h\s") 3.9/3.10: False 3.11+: True Being as strict as 3.13 dropped rooted, driveless roots, which broke test_filter_redundant_known_paths on Windows 3.11. A rooted, driveless path names the current drive's root rather than the working directory, so it does not carry the risk the known-root hardening exists to prevent -- only a drive-relative path ('C:x') does. It is now absolute on every version, and the verdict no longer changes with the interpreter. That test was already latently failing on Windows 3.13 and 3.14 before this commit, for the same reason: this PR introduced the isabs filter, and on 3.13+ the stdlib answer discards a '/a' root. Fail-fast cancelled those jobs before they reported. Also corrects a second Windows-only failure this PR introduced in the same test. mainline returned each root's original spelling; this PR normalizes them, so a '/a' root now comes back as '\a' on Windows. Normalizing is the intent -- it is what dedupes equivalent spellings without consulting the working directory -- so the test's expectation is now platform-aware rather than the normalization being reverted. Verified by replaying all three of its Windows assertions under ntpath on 3.9, 3.10, 3.11 and 3.14. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- src/deadline/client/_path_utils.py | 18 +++---- .../cli/test_cli_bundle_submit_known_paths.py | 13 +++-- test/unit/deadline_client/test_path_utils.py | 48 +++++++++---------- 3 files changed, 42 insertions(+), 37 deletions(-) diff --git a/src/deadline/client/_path_utils.py b/src/deadline/client/_path_utils.py index 7816eb33f..b6bfceb87 100644 --- a/src/deadline/client/_path_utils.py +++ b/src/deadline/client/_path_utils.py @@ -174,17 +174,17 @@ def is_absolute_path(path: Any, *, path_module: Any = os.path) -> bool: whether a path may be trusted as a root or accepted as a parameter value, so a UNC share silently reading as relative drops valid roots and rejects valid values. - Only a fully qualified path counts. On Windows a drive-relative anchor (``C:x``) needs - the working directory on that drive and a rooted, driveless one (``\\x``) needs the - current drive, so neither qualifies -- ``ntpath.isabs`` accepted ``\\x`` until 3.13, and - this is deliberately as strict as the newest stdlib rather than as loose as the oldest. + A drive-relative path (``C:x``, meaning ``x`` under the working directory on ``C:``) is + not absolute, because resolving it needs the working directory -- which is the thing the + known-root hardening must never let a caller supply implicitly. A rooted, driveless path + (``\\x``) is absolute: it names the current drive's root, not the working directory. + + That second answer is why this cannot just call ``path_module.isabs`` on the newest + interpreters either -- ``ntpath.isabs`` returns True for ``\\x`` through 3.12 and False + from 3.13. Answering from the anchor keeps the verdict the same on every version. """ anchor, _ = _split_anchored(path, path_module, normalize_case=True) - if not anchor: - return False - if path_module.sep != "\\": - return True - return not _denotes_drive(anchor) and anchor != path_module.sep + return bool(anchor) and not _denotes_drive(anchor) def normalized_path(path: Any, *, path_module: Any = os.path) -> str: diff --git a/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py b/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py index 63a202a19..dfc4d2929 100644 --- a/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py +++ b/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py @@ -46,14 +46,19 @@ ], ) def test_filter_redundant_known_paths(input, expected): + if os.name == "nt": + # The filter normalizes its roots, so a '/a' root comes back spelled '\a' here. + # Redundancy filtering is what these cases pin; the separator is os.path's business. + expected = [path.replace("/", "\\") for path in expected] assert sorted(_filter_redundant_known_paths(input)) == expected if os.name == "nt": - assert sorted(_filter_redundant_known_paths(path.replace("/", "\\") for path in input)) == [ - path.replace("/", "\\") for path in expected - ] + assert ( + sorted(_filter_redundant_known_paths(path.replace("/", "\\") for path in input)) + == expected + ) assert sorted( _filter_redundant_known_paths("C:" + path.replace("/", "\\") for path in input) - ) == ["C:" + path.replace("/", "\\") for path in expected] + ) == ["C:" + path for path in expected] @pytest.mark.parametrize( diff --git a/test/unit/deadline_client/test_path_utils.py b/test/unit/deadline_client/test_path_utils.py index d7576e3cf..1797d457b 100644 --- a/test/unit/deadline_client/test_path_utils.py +++ b/test/unit/deadline_client/test_path_utils.py @@ -533,13 +533,18 @@ def test_path_components(path, path_module, expected): (r"\\?\C:\a", ntpath, True), (r"\\?\UNC\host\share", ntpath, True), (r"\\.\C:\a", ntpath, True), - # Drive-relative needs the working directory on that drive; rooted-driveless needs - # the current drive. Neither is fully qualified, so neither may be trusted as a - # root. ntpath.isabs accepted the latter until 3.13. + # A rooted, driveless path names the current drive's root, not the working + # directory, so it is absolute. ntpath.isabs agrees through 3.12 and disagrees from + # 3.13; answering from the anchor keeps the verdict version-independent, and a + # cross-platform caller passing '/a' roots on Windows depends on this. + ("\\", ntpath, True), + (r"\x", ntpath, True), + ("/a", ntpath, True), + ("/", ntpath, True), + # Drive-relative needs the working directory on that drive, which is exactly what + # the known-root hardening must not let a caller supply implicitly. ("C:", ntpath, False), ("C:foo", ntpath, False), - ("\\", ntpath, False), - (r"\x", ntpath, False), ("rel", ntpath, False), (r"rel\f", ntpath, False), (r"..\a", ntpath, False), @@ -556,27 +561,22 @@ def test_is_absolute_path(path, path_module, expected): assert is_absolute_path(path, path_module=path_module) is expected -def test_is_absolute_path_is_at_least_as_strict_as_the_stdlib(): - """Never accept something ``isabs`` rejects on the running interpreter. +def test_is_absolute_path_never_accepts_a_working_directory_relative_path(): + """The property the known-root hardening depends on. - This is what makes the helper safe to swap in at trust boundaries: it may be stricter - than the stdlib (it is, for rooted-driveless paths before 3.13), and it may accept a UNC - share that older versions wrongly rejected, but it must not otherwise widen what counts - as absolute. + ``ntpath.isabs`` disagrees with itself across supported versions in two places -- a UNC + path naming a share (False before 3.11) and a rooted, driveless path (True through 3.12) + -- so it cannot be the reference. What must hold on every version is narrower: a path + that needs the working directory to resolve is never absolute, because such a root would + let the directory the shell happens to be in become trusted. """ - for path_module in (ntpath, posixpath): - corpus = ( - [r"\\host", r"\\host\s1", "\\", r"\x", "C:", "C:foo", r"C:\a", "rel", ""] - if path_module is ntpath - else ["/", "/a", "rel", "../a", ""] - ) - for path in corpus: - if not is_absolute_path(path, path_module=path_module): - continue - # The only sanctioned widening: a UNC path whose share splitdrive swallowed. - if not path_module.isabs(path): - assert path_module is ntpath, path - assert path.startswith("\\\\"), path + relative = { + ntpath: ["rel", r"rel\f", r"..\a", ".", "", "C:", "C:foo", r"C:..\x"], + posixpath: ["rel", "rel/f", "../a", ".", ""], + } + for path_module, paths in relative.items(): + for path in paths: + assert is_absolute_path(path, path_module=path_module) is False, (path_module, path) @pytest.mark.parametrize( From a1d3b237b52eb69d1c28f4a146a5d9da26eb03f6 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 13 Aug 2026 07:04:45 -0700 Subject: [PATCH 05/28] fix: drop rooted, driveless known-asset roots on Windows Reverts the leniency of the previous commit. Two tests in this PR disagreed about a rooted, driveless path, and on Windows they cannot both hold because '/a' and '\projects' are the same shape there: - test_filter_redundant_known_paths_drops_unanchored_paths lists '\projects' among the roots that must be dropped. - test_filter_redundant_known_paths, which predates this PR's isabs filter, passes '/a' roots and expects them kept. Dropping is the correct answer. '\projects' resolves at the root of whichever drive the process happens to be on, so like 'C:x' it names no fixed location, and letting it through is exactly what the known-root hardening exists to prevent. Dropping a root costs a warning; trusting an ambiguous one does not fail closed. CPython reached the same conclusion in 3.13, when ntpath.isabs stopped accepting the form. So the pre-existing test is the one that was wrong on Windows, and only because this PR added the filter: its POSIX-style roots are root-relative there, not absolute. It already carried a drive-qualified variant for the real case, which is now what pins the redundancy behaviour; the two unanchored spellings assert they are dropped. is_absolute_path therefore answers from the anchor and rejects both Windows working-directory-dependent forms on every version, diverging from ntpath.isabs where the stdlib disagrees with itself -- accepting a UNC share it rejects before 3.11, rejecting a rooted driveless path it accepts through 3.12. It matches 3.14 exactly. Replaying both tests' Windows assertions under ntpath on 3.9, 3.10, 3.11 and 3.14 gives 26 for 26 on each. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- src/deadline/client/_path_utils.py | 23 +++++++++------- .../cli/test_cli_bundle_submit_known_paths.py | 26 +++++++++---------- test/unit/deadline_client/test_path_utils.py | 21 +++++++-------- 3 files changed, 37 insertions(+), 33 deletions(-) diff --git a/src/deadline/client/_path_utils.py b/src/deadline/client/_path_utils.py index b6bfceb87..7f83a4043 100644 --- a/src/deadline/client/_path_utils.py +++ b/src/deadline/client/_path_utils.py @@ -174,17 +174,22 @@ def is_absolute_path(path: Any, *, path_module: Any = os.path) -> bool: whether a path may be trusted as a root or accepted as a parameter value, so a UNC share silently reading as relative drops valid roots and rejects valid values. - A drive-relative path (``C:x``, meaning ``x`` under the working directory on ``C:``) is - not absolute, because resolving it needs the working directory -- which is the thing the - known-root hardening must never let a caller supply implicitly. A rooted, driveless path - (``\\x``) is absolute: it names the current drive's root, not the working directory. - - That second answer is why this cannot just call ``path_module.isabs`` on the newest - interpreters either -- ``ntpath.isabs`` returns True for ``\\x`` through 3.12 and False - from 3.13. Answering from the anchor keeps the verdict the same on every version. + Only a fully anchored path counts. On Windows neither a drive-relative path (``C:x``, + meaning ``x`` under the working directory on ``C:``) nor a rooted, driveless one + (``\\x``, meaning ``x`` at the root of whichever drive the process is on) names a fixed + location: resolving either consults the working directory, which is exactly what the + known-root hardening must not let a caller supply implicitly. + + ``ntpath.isabs`` accepted ``\\x`` through 3.12 and rejects it from 3.13, so it cannot be + the reference in either direction. Answering from the anchor keeps the verdict the same + on every version. """ anchor, _ = _split_anchored(path, path_module, normalize_case=True) - return bool(anchor) and not _denotes_drive(anchor) + if not anchor: + return False + if path_module.sep != "\\": + return True + return not _denotes_drive(anchor) and anchor != path_module.sep def normalized_path(path: Any, *, path_module: Any = os.path) -> str: diff --git a/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py b/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py index dfc4d2929..307621b29 100644 --- a/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py +++ b/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py @@ -46,19 +46,19 @@ ], ) def test_filter_redundant_known_paths(input, expected): - if os.name == "nt": - # The filter normalizes its roots, so a '/a' root comes back spelled '\a' here. - # Redundancy filtering is what these cases pin; the separator is os.path's business. - expected = [path.replace("/", "\\") for path in expected] - assert sorted(_filter_redundant_known_paths(input)) == expected - if os.name == "nt": - assert ( - sorted(_filter_redundant_known_paths(path.replace("/", "\\") for path in input)) - == expected - ) - assert sorted( - _filter_redundant_known_paths("C:" + path.replace("/", "\\") for path in input) - ) == ["C:" + path for path in expected] + if os.name != "nt": + assert sorted(_filter_redundant_known_paths(input)) == expected + return + + # On Windows these POSIX-style paths are root-relative rather than absolute: '\a' + # resolves against whichever drive the process is on, so it is dropped as unanchored -- + # see test_filter_redundant_known_paths_drops_unanchored_paths. Only the drive-qualified + # spelling is a usable root here, so that is the one carrying the redundancy cases. + assert _filter_redundant_known_paths(input) == [] + assert _filter_redundant_known_paths(path.replace("/", "\\") for path in input) == [] + assert sorted( + _filter_redundant_known_paths("C:" + path.replace("/", "\\") for path in input) + ) == ["C:" + path.replace("/", "\\") for path in expected] @pytest.mark.parametrize( diff --git a/test/unit/deadline_client/test_path_utils.py b/test/unit/deadline_client/test_path_utils.py index 1797d457b..5571ebe77 100644 --- a/test/unit/deadline_client/test_path_utils.py +++ b/test/unit/deadline_client/test_path_utils.py @@ -533,16 +533,14 @@ def test_path_components(path, path_module, expected): (r"\\?\C:\a", ntpath, True), (r"\\?\UNC\host\share", ntpath, True), (r"\\.\C:\a", ntpath, True), - # A rooted, driveless path names the current drive's root, not the working - # directory, so it is absolute. ntpath.isabs agrees through 3.12 and disagrees from - # 3.13; answering from the anchor keeps the verdict version-independent, and a - # cross-platform caller passing '/a' roots on Windows depends on this. - ("\\", ntpath, True), - (r"\x", ntpath, True), - ("/a", ntpath, True), - ("/", ntpath, True), - # Drive-relative needs the working directory on that drive, which is exactly what - # the known-root hardening must not let a caller supply implicitly. + # Neither Windows form that consults the working directory is absolute: '\x' is at + # the root of whichever drive the process is on, and 'C:x' is under the working + # directory on C:. ntpath.isabs accepted the former through 3.12 and rejects it from + # 3.13, so answering from the anchor is what keeps this version-independent. + ("\\", ntpath, False), + (r"\x", ntpath, False), + ("/a", ntpath, False), + ("/", ntpath, False), ("C:", ntpath, False), ("C:foo", ntpath, False), ("rel", ntpath, False), @@ -571,7 +569,8 @@ def test_is_absolute_path_never_accepts_a_working_directory_relative_path(): let the directory the shell happens to be in become trusted. """ relative = { - ntpath: ["rel", r"rel\f", r"..\a", ".", "", "C:", "C:foo", r"C:..\x"], + # '\x' and '/a' are here because on Windows they resolve against the current drive. + ntpath: ["rel", r"rel\f", r"..\a", ".", "", "C:", "C:foo", r"C:..\x", "\\", r"\x", "/a"], posixpath: ["rel", "rel/f", "../a", ".", ""], } for path_module, paths in relative.items(): From d8f2f77cae44dcf0352858da8bc4ad9da50b37ed Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 13 Aug 2026 07:41:40 -0700 Subject: [PATCH 06/28] test: bind the mapped drive letter before the skip CodeQL flagged 'drive' as possibly used before assignment in the mapped-drive SMB test. pytest.skip() raises, so the for/else could not actually fall through with it unbound, but the plain form says so without relying on the reader knowing that. The alert has been open since this PR's first push and is the only remaining red check. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- test/integ/windows_smb/test_unc_path_containment.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/integ/windows_smb/test_unc_path_containment.py b/test/integ/windows_smb/test_unc_path_containment.py index 14acad6a8..069436de5 100644 --- a/test/integ/windows_smb/test_unc_path_containment.py +++ b/test/integ/windows_smb/test_unc_path_containment.py @@ -187,11 +187,12 @@ def test_mapped_drive_resolves_and_compares(smb_share): unc_root, _ = smb_share host_root = unc_root.rsplit("\\", 1)[0] + drive = None for letter in ("Y:", "Z:"): if _run("net", "use", letter, unc_root).returncode == 0: drive = letter break - else: + if drive is None: pytest.skip("no free drive letter to map the share onto") try: From 90bfdd57f8f6465160555eb6d7976935f676adeb Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 13 Aug 2026 08:31:31 -0700 Subject: [PATCH 07/28] fix: drop the bare UNC anchor from known-asset roots Raised by automated review, reproduced before changing anything. A bare '\\' is fully qualified, so it passed the is_absolute_path filter. Its components are the single ['\\'], so it sorted first and was inserted into the trie as a marker -- and being one component it is a prefix of *every* real UNC root, so each one after it was skipped as redundant: filter(['\\', '\\host']) -> ['\\'] filter(['\\', '\\server\share', '\\other\s2']) -> ['\\'] is_path_contained deliberately treats that anchor as containing nothing, so the surviving root matched nothing while the roots that would have matched were gone. Every path under them became unknown -- the spurious "outside of known asset paths" warning, and a blocked non-interactive submit. Same user-visible failure as #1321, reintroduced through the filter this PR added. It reaches the filter from the inputs the docstring already lists for the empty root, plus two spellings that are not obvious: '//' and '\\?\UNC\', the latter because _fold_extended_length_prefix collapses it to the anchor. That fold is new in this PR, so it widened the reachable surface. Dropped alongside the unanchored roots, via a named is_bare_unc_anchor: it is the one absolute path that names no location, which is why is_absolute_path is the wrong place to express it. Tests: the filter cases including both orderings and both alternate spellings, the anchor alone yielding no roots, is_bare_unc_anchor across path spaces, and an end-to-end _generate_message_for_asset_paths case that runs the filter before the containment check. The halves each looked correct in isolation -- _is_known_path handles the anchor properly, and the filter deleted the real root before it ever got there -- so only the combined test sees this. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- src/deadline/client/_path_utils.py | 15 +++++ src/deadline/client/api/_submit_job_bundle.py | 7 ++- .../cli/test_cli_bundle_submit_known_paths.py | 59 ++++++++++++++++++- test/unit/deadline_client/test_path_utils.py | 39 ++++++++++++ 4 files changed, 118 insertions(+), 2 deletions(-) diff --git a/src/deadline/client/_path_utils.py b/src/deadline/client/_path_utils.py index 7f83a4043..531672059 100644 --- a/src/deadline/client/_path_utils.py +++ b/src/deadline/client/_path_utils.py @@ -26,6 +26,7 @@ __all__ = [ "is_absolute_path", "is_any_path_contained", + "is_bare_unc_anchor", "is_path_contained", "normalized_path", "path_components", @@ -192,6 +193,20 @@ def is_absolute_path(path: Any, *, path_module: Any = os.path) -> bool: return not _denotes_drive(anchor) and anchor != path_module.sep +def is_bare_unc_anchor(path: Any, *, path_module: Any = os.path) -> bool: + """True iff ``path`` is the bare ``\\\\`` marker, which names no server. + + It is fully qualified yet identifies no location, so it contains nothing -- + :func:`is_path_contained` enforces that. As a trusted root it is worse than useless: in a + component trie it is a prefix of *every* real UNC root, so keeping it would filter them + all out and leave only a root that matches nothing. + + ``'//'`` and ``'\\\\?\\UNC\\'`` both normalize to it, so it arrives from more spellings + than it looks like. + """ + return path_components(path, path_module=path_module) == [_UNC_ANCHOR] + + def normalized_path(path: Any, *, path_module: Any = os.path) -> str: """Return ``path`` with ``..``, ``.``, repeated separators and separator style resolved. diff --git a/src/deadline/client/api/_submit_job_bundle.py b/src/deadline/client/api/_submit_job_bundle.py index baf5b5df9..69c48d486 100644 --- a/src/deadline/client/api/_submit_job_bundle.py +++ b/src/deadline/client/api/_submit_job_bundle.py @@ -73,6 +73,7 @@ from ...job_attachments.api._hashing import _hash_attachments from .._path_utils import ( is_absolute_path, + is_bare_unc_anchor, is_any_path_contained, normalized_path, path_components, @@ -304,7 +305,10 @@ def _filter_redundant_known_paths(known_asset_paths: Iterable[str]) -> list[str] atom) and case variants of one location dedupe on Windows. Roots are expanded for '~' (the config file and the CLI submitter's default data - directory supply one unexpanded) and dropped unless absolute. A non-absolute root + directory supply one unexpanded), and dropped unless absolute and naming a location. + The bare UNC anchor is dropped for the second reason: it contains nothing, and being a + single component it would prefix every real UNC root in the trie below and filter them + all out, leaving only a root that matches nothing. A non-absolute root matches no candidate anyway, but dropping it here means a future caller cannot turn it into a trusted tree by resolving it -- ``os.path.abspath("")`` is the whole working directory, which would suppress the unknown-path warning and let a non-interactive @@ -322,6 +326,7 @@ def _filter_redundant_known_paths(known_asset_paths: Iterable[str]) -> list[str] normalized_path(path, path_module=os.path) for path in expanded if is_absolute_path(path, path_module=os.path) + and not is_bare_unc_anchor(path, path_module=os.path) ) ) components = {path: path_components(path, path_module=os.path) for path in ordered} diff --git a/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py b/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py index 307621b29..a645c02e1 100644 --- a/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py +++ b/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py @@ -180,7 +180,11 @@ def test_is_known_path(path, roots, expected): (r"\\corp\finance\salaries.xlsx", ["\\\\"], False), (r"\\corp\finance\salaries.xlsx", ["//"], False), (r"\\corp\finance\salaries.xlsx", ["\\\\?\\UNC\\"], False), - # A useless root must not shadow a real one that follows it. + # A useless root must not shadow a real one that follows it. _is_known_path is only + # half the story here -- the submit flow runs _filter_redundant_known_paths first, + # where the bare anchor used to prefix and so delete every real UNC root. See + # test_filter_redundant_known_paths_drops_the_bare_unc_anchor and + # test_generate_message_for_asset_paths_bare_anchor_does_not_shadow_a_real_root. (r"\\host\share\file", ["\\\\", r"\\host"], True), ], ) @@ -270,6 +274,59 @@ def test_filter_redundant_known_paths_unanchored_path_does_not_trust_cwd(): assert _is_known_path(cwd_file, _filter_redundant_known_paths([""])) is False +@pytest.mark.parametrize( + "input, expected", + [ + # The bare anchor is a single component, so it sorts first and would prefix -- and + # therefore delete -- every real UNC root in the trie, leaving only a root that + # matches nothing. It is dropped instead. + (["\\\\", r"\\host"], [r"\\host"]), + ([r"\\host", "\\\\"], [r"\\host"]), + (["\\\\", r"\\server\share", r"\\other\share"], [r"\\server\share", r"\\other\share"]), + # It arrives from more spellings than it looks like: '//' and '\\?\UNC\' both + # normalize to it, the latter via _fold_extended_length_prefix. + (["//", r"\\host"], [r"\\host"]), + ([r"\\?\UNC\\", r"\\host"], [r"\\host"]), + # On its own it leaves no roots at all, which is correct: it contains nothing, so + # every path is unknown and the warning is the right outcome. + (["\\\\"], []), + ], +) +def test_filter_redundant_known_paths_drops_the_bare_unc_anchor(input, expected): + """The bare anchor is anchored but names no location, unlike every other absolute root.""" + with patch.object(sjb.os, "path", ntpath): + assert _filter_redundant_known_paths(input) == expected + + +def test_generate_message_for_asset_paths_bare_anchor_does_not_shadow_a_real_root(): + """End-to-end: the filter runs before containment, so the two must agree about '\\\\'. + + The halves are covered separately above; this pins them together, because the bug this + guards against was invisible to either one alone -- _is_known_path handles the bare + anchor correctly, and the filter deleted the real root before it ever got there. + """ + upload_group = AssetUploadGroup( + asset_groups=[ + AssetRootGroup( + root_path=r"\\host\projects", + inputs={r"\\host\projects\scene.ma"}, # type: ignore[arg-type] + ) + ], + total_input_files=1, + total_input_bytes=12, + ) + + with patch("deadline.client.api._submit_job_bundle.os.path", ntpath): + known_asset_paths = _filter_redundant_known_paths(["\\\\", r"\\host"]) + message, no_warnings = _generate_message_for_asset_paths( + upload_group, storage_profile=None, known_asset_paths=known_asset_paths + ) + + assert known_asset_paths == [r"\\host"], known_asset_paths + assert no_warnings is True, message + assert "WARNING: Files were specified outside of known asset paths." not in message, message + + def test_generate_message_for_asset_paths_unc_host_root_is_known(): """ Regression for issue #1321: files on a share under a host-level UNC known root diff --git a/test/unit/deadline_client/test_path_utils.py b/test/unit/deadline_client/test_path_utils.py index 5571ebe77..0cfd4bb3b 100644 --- a/test/unit/deadline_client/test_path_utils.py +++ b/test/unit/deadline_client/test_path_utils.py @@ -21,6 +21,7 @@ _splitroot, is_absolute_path, is_any_path_contained, + is_bare_unc_anchor, is_path_contained, normalized_path, path_components, @@ -612,3 +613,41 @@ def test_path_components_preserves_case_when_asked(): "Share", "File", ] + + +@pytest.mark.parametrize( + "path, path_module, expected", + [ + ("\\\\", ntpath, True), + ("\\\\\\\\", ntpath, True), + ("//", ntpath, True), + # _fold_extended_length_prefix collapses this to the bare anchor. + ("\\\\?\\UNC\\", ntpath, True), + (r"\\?\UNC", ntpath, True), + # A server name makes it a location, so it is no longer bare. + (r"\\host", ntpath, False), + (r"\\host\share", ntpath, False), + # Other path spaces are never the UNC anchor, however root-like. + ("\\", ntpath, False), + ("C:\\", ntpath, False), + ("", ntpath, False), + ("/", posixpath, False), + ("//", posixpath, False), + ], +) +def test_is_bare_unc_anchor(path, path_module, expected): + assert is_bare_unc_anchor(path, path_module=path_module) is expected + + +def test_bare_unc_anchor_contains_nothing_and_is_contained_by_nothing_real(): + """Why callers must drop it rather than treat it as a root. + + It is the one absolute path that names no location, so a caller that keeps it holds a + root matching nothing -- and in a component trie it prefixes every real UNC root. + """ + assert is_absolute_path("\\\\", path_module=ntpath) is True + assert is_path_contained(r"\\host\share\f", "\\\\", path_module=ntpath) is False + assert is_path_contained(r"\\host", "\\\\", path_module=ntpath) is False + # Reflexive, and still its own path space rather than the rooted-driveless one. + assert is_path_contained("\\\\", "\\\\", path_module=ntpath) is True + assert is_path_contained("\\\\", "\\", path_module=ntpath) is False From 7b1c27b0441ca70d0b14c0090db787b151973d38 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 13 Aug 2026 08:31:31 -0700 Subject: [PATCH 08/28] ci: run the SMB path test post-merge, not only on dispatch Raised by automated review. windows_smb_test.yml declared only workflow_dispatch and workflow_call, and nothing in the repo calls it, so the regression coverage it provides for #1321 never actually ran. test/integ is outside testpaths and outside 'hatch run test', so no other job reaches test/integ/windows_smb either -- a future regression in UNC handling would have shipped guarded by nothing but the unit tests, which are lexical by their own docstring and say nothing about what the SMB redirector does. Adds 'push: branches: [mainline]', matching dcm_integration_tests.yml. Kept off per-PR CI deliberately: it needs administrator rights to create the share and is slower and more environment-dependent than a unit test. workflow_call stays so a release-time caller can still invoke it with a tag. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- .github/workflows/windows_smb_test.yml | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/workflows/windows_smb_test.yml b/.github/workflows/windows_smb_test.yml index ab2cdf99a..0cc916c13 100644 --- a/.github/workflows/windows_smb_test.yml +++ b/.github/workflows/windows_smb_test.yml @@ -4,9 +4,14 @@ name: Windows SMB Path Test # cannot do. Regression coverage for issue #1321. # # 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. +# share is slower and more environment-dependent than a unit test. It runs post-merge +# instead, matching dcm_integration_tests.yml -- if it only ran on manual dispatch a +# regression in UNC handling would ship, since test/integ is outside testpaths and so no +# other job reaches it, and the unit tests are lexical by their own docstring. on: workflow_dispatch: + push: + branches: [mainline] workflow_call: inputs: tag: From cc26271957b9c6d311b76cc45269b7c011fafefa Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Wed, 19 Aug 2026 07:55:53 -0700 Subject: [PATCH 09/28] ci: fail the SMB suite rather than skip it when it cannot run pytest exits 0 when every test skips, so a module-scoped fixture skip made a runner that could not build the share report a green job while asserting nothing about SMB -- and this workflow is the only place these tests run. Environment skips now fail when DEADLINE_SMB_TESTS_REQUIRED is set, which the workflow sets and a developer without administrator rights does not. That covers the share, the redirector, the symlink privilege, and the drive letter; a non-Windows interpreter raises at collection for the same reason. Drops the Report skips step, which re-ran the whole suite -- a second share create and delete cycle -- to print counts it could not act on. -rs on the one run reports the same thing. Pins numprocesses=0 over the auto in addopts: each xdist worker built its own share and they raced for the one free drive letter, which is a spurious skip in its own right. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- .github/workflows/windows_smb_test.yml | 25 +++++---- .../windows_smb/test_unc_path_containment.py | 53 +++++++++++++++++-- 2 files changed, 60 insertions(+), 18 deletions(-) diff --git a/.github/workflows/windows_smb_test.yml b/.github/workflows/windows_smb_test.yml index 0cc916c13..d7933cd3b 100644 --- a/.github/workflows/windows_smb_test.yml +++ b/.github/workflows/windows_smb_test.yml @@ -35,8 +35,8 @@ jobs: python-version: '3.12' - name: Confirm SMB prerequisites - # Fail with a clear message here rather than having every test skip itself, - # which would look like a pass. + # Fail with a clear message here rather than leaving the tests to diagnose the + # runner for us. shell: pwsh run: | $admin = ([Security.Principal.WindowsPrincipal] ` @@ -47,7 +47,7 @@ jobs: Start-Service LanmanServer Start-Service LanmanWorkstation # Developer Mode lets a non-elevated process create symlinks; the escape - # test needs one and skips itself otherwise. + # test needs one and fails below if it cannot get one. $key = 'HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\AppModelUnlock' New-Item -Path $key -Force | Out-Null Set-ItemProperty -Path $key -Name AllowDevelopmentWithoutDevLicense -Value 1 -Type DWord @@ -58,14 +58,13 @@ jobs: - name: Run the SMB path tests shell: pwsh + env: + # Turn the fixtures' environment skips into failures. pytest exits 0 when every + # test skips, so without this a runner that could not build the share reports a + # green job that asserted nothing about SMB -- and this workflow is the only + # place these tests run. + DEADLINE_SMB_TESTS_REQUIRED: '1' run: | - hatch run pytest test/integ/windows_smb -v --no-cov -p no:randomly - - - name: Report skips - # A skipped SMB test is indistinguishable from a passing one in the summary, - # so surface the count explicitly. - if: always() - shell: pwsh - run: | - hatch run pytest test/integ/windows_smb --no-cov -q -rs 2>&1 | - Select-String -Pattern 'SKIPPED|passed|failed' + # numprocesses=0 overrides the auto in addopts: each xdist worker would build + # its own share and they would race for the one free drive letter. + hatch run pytest test/integ/windows_smb -v -rs --no-cov --numprocesses=0 -p no:randomly diff --git a/test/integ/windows_smb/test_unc_path_containment.py b/test/integ/windows_smb/test_unc_path_containment.py index 069436de5..51b4ab97f 100644 --- a/test/integ/windows_smb/test_unc_path_containment.py +++ b/test/integ/windows_smb/test_unc_path_containment.py @@ -10,6 +10,9 @@ verdicts are checked against a real redirector. Requires Windows and administrator rights; see .github/workflows/windows_smb_test.yml. +Where the share is meant to exist, set ``DEADLINE_SMB_TESTS_REQUIRED=1`` so an +environment that cannot host one fails instead of skipping. + Regression coverage for https://github.com/aws-deadline/deadline-cloud/issues/1321. """ @@ -21,7 +24,7 @@ import sys import uuid from pathlib import Path -from typing import Iterator +from typing import Iterator, NoReturn import pytest @@ -34,6 +37,21 @@ from deadline.client.job_bundle.loader import validate_directory_symlink_containment from deadline.client.exceptions import DeadlineOperationError + +def _smb_required() -> bool: + """Whether this environment promised a share. Read at call time, not cached, so the + tests can pin both branches.""" + return os.environ.get("DEADLINE_SMB_TESTS_REQUIRED") == "1" + + +if _smb_required() and sys.platform != "win32": + # A collection error, since a platform skip is a skip like any other here: every test + # would be marked skipped and pytest would still exit 0. + raise RuntimeError( + "DEADLINE_SMB_TESTS_REQUIRED is set, but SMB shares require Windows and this is " + f"{sys.platform}, so none of these tests can run." + ) + pytestmark = [ pytest.mark.integ, pytest.mark.skipif(sys.platform != "win32", reason="SMB shares require Windows."), @@ -44,6 +62,19 @@ def _run(*args: str) -> subprocess.CompletedProcess: return subprocess.run(args, capture_output=True, text=True, check=False) +def _unavailable(reason: str) -> NoReturn: + """Skip where a share cannot be hosted; fail where one was promised. + + pytest exits 0 when every test skips, so a module-scoped fixture skip would let a + run that built no share report success while asserting nothing about SMB. These are + the only tests that check the verdicts against a real redirector, so silence here + reads as coverage that does not exist. + """ + if _smb_required(): + pytest.fail(f"SMB coverage is required in this environment but unavailable: {reason}") + pytest.skip(reason) + + @pytest.fixture(scope="module") def smb_share(tmp_path_factory) -> Iterator[tuple[str, Path]]: """Share a local directory over SMB and yield ``(unc_root, local_path)``. @@ -56,7 +87,7 @@ def smb_share(tmp_path_factory) -> Iterator[tuple[str, Path]]: created = _run("net", "share", f"{share_name}={local_path}", "/GRANT:Everyone,FULL") if created.returncode != 0: - pytest.skip( + _unavailable( f"could not create an SMB share: {created.stdout.strip()} {created.stderr.strip()}" ) @@ -67,12 +98,24 @@ def smb_share(tmp_path_factory) -> Iterator[tuple[str, Path]]: # Fail fast and clearly if the redirector cannot reach the new share, rather # than letting every assertion below fail with a confusing error. if not os.path.isdir(unc_root): - pytest.skip(f"SMB share {unc_root} is not reachable from this host") + _unavailable(f"SMB share {unc_root} is not reachable from this host") yield unc_root, local_path finally: _run("net", "share", share_name, "/DELETE", "/Y") +def test_unavailable_fails_where_the_share_is_required(monkeypatch): + """Pin the enforcement, since a regression in it would be invisible: it turns the + whole file back into a skip, which is what a pass looks like.""" + monkeypatch.delenv("DEADLINE_SMB_TESTS_REQUIRED", raising=False) + with pytest.raises(pytest.skip.Exception): + _unavailable("no share here") + + monkeypatch.setenv("DEADLINE_SMB_TESTS_REQUIRED", "1") + with pytest.raises(pytest.fail.Exception): + _unavailable("no share here") + + def test_host_level_root_contains_share_contents(smb_share): """The reported bug: a '\\\\server' root must contain files on its shares.""" unc_root, local_path = smb_share @@ -172,7 +215,7 @@ def test_symlink_escaping_the_share_is_rejected(smb_share): try: os.symlink(outside, link) except OSError as exc: # pragma: no cover - depends on runner privileges - pytest.skip(f"cannot create a symlink on this share: {exc}") + _unavailable(f"cannot create a symlink on this share: {exc}") with pytest.raises(DeadlineOperationError): validate_directory_symlink_containment(str(bundle)) @@ -193,7 +236,7 @@ def test_mapped_drive_resolves_and_compares(smb_share): drive = letter break if drive is None: - pytest.skip("no free drive letter to map the share onto") + _unavailable("no free drive letter to map the share onto") try: asset = Path(drive + "\\") / "mapped_probe.txt" From 0bcc8bf2a70b6e034ac401cfa2053bf3467d2b2c Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Wed, 19 Aug 2026 09:25:54 -0700 Subject: [PATCH 10/28] fix: use component containment for the archive extraction guard The zip-slip guard #1181 added compares os.path.commonpath against the destination, which the TID251 ban this PR introduces rejects -- and for the reason the ban exists. commonpath raises on a share-root destination: ntpath.commonpath([r"\\host\share", r"\\host\share\template.yaml"]) ValueError: Can't mix absolute and relative paths The guard read that as an escape, so with the destination on a share root it rejected every entry of every archive. A redirected profile puts the bundle cache there, so this was reachable rather than theoretical. is_path_contained answers the same question across path spaces: a different drive or UNC host is not contained, so the branch that caught ValueError to reject them is no longer needed. It also compares components rather than strings, so a sibling sharing a prefix stays outside. Tests: the share-root extraction and its escape on a real share, and a sibling-prefix rejection in the unit suite. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- src/deadline/client/job_bundle/_repository.py | 10 +++--- .../windows_smb/test_unc_path_containment.py | 31 +++++++++++++++++++ .../job_bundle/test_repository.py | 15 ++++++++- 3 files changed, 49 insertions(+), 7 deletions(-) diff --git a/src/deadline/client/job_bundle/_repository.py b/src/deadline/client/job_bundle/_repository.py index 07262f3d8..1fd36eb53 100644 --- a/src/deadline/client/job_bundle/_repository.py +++ b/src/deadline/client/job_bundle/_repository.py @@ -27,6 +27,7 @@ from botocore.exceptions import ClientError +from .._path_utils import is_path_contained from ..config import config_file from ..config.config_file import get_cache_directory from ..exceptions import DeadlineOperationError @@ -162,12 +163,9 @@ def _safe_zip_extract( if os.path.isabs(member) or member.startswith(("/", "\\")): raise ValueError(f"Archive contains absolute path: {member}") target = os.path.realpath(os.path.join(dest, member)) - try: - common = os.path.commonpath([dest, target]) - except ValueError: - # On Windows, different drives have no common path - raise ValueError(f"Archive entry would extract outside target directory: {member}") - if common != dest: + # Unrelated path spaces -- a different drive, a different UNC host -- are not + # contained, so they are rejected here rather than raising from the comparison. + if not is_path_contained(target, dest): raise ValueError(f"Archive entry would extract outside target directory: {member}") _check_archive_extraction_safety(zf, dest) diff --git a/test/integ/windows_smb/test_unc_path_containment.py b/test/integ/windows_smb/test_unc_path_containment.py index 51b4ab97f..59268c8cc 100644 --- a/test/integ/windows_smb/test_unc_path_containment.py +++ b/test/integ/windows_smb/test_unc_path_containment.py @@ -23,6 +23,7 @@ import subprocess import sys import uuid +import zipfile from pathlib import Path from typing import Iterator, NoReturn @@ -34,6 +35,7 @@ _filter_redundant_known_paths, _is_known_path, ) +from deadline.client.job_bundle._repository import _safe_zip_extract from deadline.client.job_bundle.loader import validate_directory_symlink_containment from deadline.client.exceptions import DeadlineOperationError @@ -221,6 +223,35 @@ def test_symlink_escaping_the_share_is_rejected(smb_share): validate_directory_symlink_containment(str(bundle)) +def test_archive_extracts_at_a_share_root_and_still_rejects_an_escape(smb_share): + """A bundle archive must extract onto a share root, and still not out of one. + + ``\\\\server\\share`` against its own files is the pair ``commonpath`` answered with + ``ValueError``, which the extraction guard read as an escape -- so every entry of + every archive was rejected when the destination was a share root. + """ + unc_root, _ = smb_share + + archive = Path(unc_root) / "extract_probe.ojd" + with zipfile.ZipFile(str(archive), "w") as zf: + zf.writestr("extract_probe.yaml", "specificationVersion: jobtemplate-2023-09\n") + zf.writestr("extract_probe/scene.c4d", "scene") + + # The destination is the share root itself, which is the spelling that raised. + with zipfile.ZipFile(str(archive), "r") as zf: + _safe_zip_extract(zf, unc_root) + assert (Path(unc_root) / "extract_probe.yaml").is_file() + assert (Path(unc_root) / "extract_probe" / "scene.c4d").is_file() + + # '..' from a share root lands on the host, a different path space, not inside it. + escaping = Path(unc_root) / "escaping.ojd" + with zipfile.ZipFile(str(escaping), "w") as zf: + zf.writestr("../escaped.txt", "nope") + with zipfile.ZipFile(str(escaping), "r") as zf: + with pytest.raises(ValueError, match="outside target directory"): + _safe_zip_extract(zf, unc_root) + + def test_mapped_drive_resolves_and_compares(smb_share): """A mapped drive letter is a distinct path space from the UNC path it points at. diff --git a/test/unit/deadline_client/job_bundle/test_repository.py b/test/unit/deadline_client/job_bundle/test_repository.py index d4983ebf7..6de3ddfee 100644 --- a/test/unit/deadline_client/job_bundle/test_repository.py +++ b/test/unit/deadline_client/job_bundle/test_repository.py @@ -18,6 +18,7 @@ from unittest.mock import MagicMock, patch from botocore.exceptions import ClientError +from deadline.client._path_utils import is_path_contained from deadline.client.exceptions import DeadlineOperationError from deadline.client.job_bundle._repository import ( LocalBundleRepository, @@ -697,6 +698,18 @@ def test_rejects_parent_directory_traversal(self, tmp_path): with pytest.raises(ValueError, match="outside target directory"): _safe_zip_extract(zf, str(dest)) + def test_rejects_sibling_sharing_a_string_prefix(self, tmp_path): + """A string prefix is not a directory prefix: 'out-evil' is outside 'out'.""" + archive = tmp_path / "bad.zip" + with zipfile.ZipFile(str(archive), "w") as zf: + zf.writestr("../out-evil/payload", "malicious") + + dest = tmp_path / "out" + dest.mkdir() + with zipfile.ZipFile(str(archive), "r") as zf: + with pytest.raises(ValueError, match="outside target directory"): + _safe_zip_extract(zf, str(dest)) + def test_allows_normal_archive(self, tmp_path): archive = tmp_path / "good.zip" with zipfile.ZipFile(str(archive), "w") as zf: @@ -1894,7 +1907,7 @@ def _assert_under_root(self, key: str): # Must be a *strict* descendant of the cache root, never the root itself # (which is what "/.." used to normalize to). assert resolved != root - assert os.path.commonpath([resolved, root]) == root + assert is_path_contained(resolved, root) assert ".." not in key.replace("\\", "/").split("/") @pytest.mark.parametrize( From baa5b9d23e1b0ed2d97bd113c522f4f93e34ab12 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Wed, 19 Aug 2026 15:21:59 -0700 Subject: [PATCH 11/28] test: pin the share-root '..' clamp instead of a false escape Windows clamps '..' at a share root, so an archive entry of '../escaped.txt' extracted into \\server\share resolves back to \\server\share -- contained, and the guard correctly does not raise. Assert the clamp where the destination is a share root, and move the escape case onto a subdirectory, the only destination on a share that '..' can leave. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- .../windows_smb/test_unc_path_containment.py | 35 ++++++++++++++++--- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/test/integ/windows_smb/test_unc_path_containment.py b/test/integ/windows_smb/test_unc_path_containment.py index 59268c8cc..9b8cbcff1 100644 --- a/test/integ/windows_smb/test_unc_path_containment.py +++ b/test/integ/windows_smb/test_unc_path_containment.py @@ -223,8 +223,8 @@ def test_symlink_escaping_the_share_is_rejected(smb_share): validate_directory_symlink_containment(str(bundle)) -def test_archive_extracts_at_a_share_root_and_still_rejects_an_escape(smb_share): - """A bundle archive must extract onto a share root, and still not out of one. +def test_archive_extracts_at_a_share_root(smb_share): + """A bundle archive must extract onto a share root. ``\\\\server\\share`` against its own files is the pair ``commonpath`` answered with ``ValueError``, which the extraction guard read as an escape -- so every entry of @@ -243,13 +243,40 @@ def test_archive_extracts_at_a_share_root_and_still_rejects_an_escape(smb_share) assert (Path(unc_root) / "extract_probe.yaml").is_file() assert (Path(unc_root) / "extract_probe" / "scene.c4d").is_file() - # '..' from a share root lands on the host, a different path space, not inside it. + +def test_pardir_at_a_share_root_stays_on_the_share(smb_share): + """A share root is a path root: '..' from it is clamped, not a step onto the host. + + The lexical tests assert this of ``ntpath``; here the real redirector agrees, which + is why a share-root destination needs no escape case of its own -- there is nowhere + for an archive entry to climb to. + """ + unc_root, _ = smb_share + share_root = os.path.realpath(unc_root) + + climbed = os.path.realpath(os.path.join(unc_root, "..", "escaped.txt")) + assert climbed.lower() == os.path.join(share_root, "escaped.txt").lower(), climbed + assert is_path_contained(climbed, share_root) + + +def test_archive_escaping_a_directory_on_a_share_is_rejected(smb_share): + """An entry climbing out of its destination must be rejected on a share too. + + The destination is a subdirectory, the only place on a share where '..' resolves + anywhere else -- from the share root Windows clamps it. + """ + unc_root, _ = smb_share + + dest = Path(unc_root) / "escape_dest" + dest.mkdir(parents=True, exist_ok=True) + escaping = Path(unc_root) / "escaping.ojd" with zipfile.ZipFile(str(escaping), "w") as zf: zf.writestr("../escaped.txt", "nope") with zipfile.ZipFile(str(escaping), "r") as zf: with pytest.raises(ValueError, match="outside target directory"): - _safe_zip_extract(zf, unc_root) + _safe_zip_extract(zf, str(dest)) + assert not (Path(unc_root) / "escaped.txt").exists() def test_mapped_drive_resolves_and_compares(smb_share): From 02191cf26fe63aaead7918d72063487403479b1e Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 20 Aug 2026 08:10:36 -0700 Subject: [PATCH 12/28] fix: make the archive extraction guard testable off Windows is_path_contained's path_module default binds os.path at import time, so the one call site that relied on it could not be pointed at ntpath the way every other call site can. The guard's UNC behavior was therefore reachable only from the SMB integ suite, which no pull request runs -- and reverting the fix it belongs to failed nothing in test/unit. Pass path_module explicitly, as the other call sites do, and cover the share-root destination, the '..' clamp at a share root, an escape from a directory on a share, and a drive-relative entry that discards the destination entirely. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- src/deadline/client/job_bundle/_repository.py | 4 +- .../job_bundle/test_repository.py | 97 +++++++++++++++++++ 2 files changed, 100 insertions(+), 1 deletion(-) diff --git a/src/deadline/client/job_bundle/_repository.py b/src/deadline/client/job_bundle/_repository.py index 1fd36eb53..4a024463f 100644 --- a/src/deadline/client/job_bundle/_repository.py +++ b/src/deadline/client/job_bundle/_repository.py @@ -165,7 +165,9 @@ def _safe_zip_extract( target = os.path.realpath(os.path.join(dest, member)) # Unrelated path spaces -- a different drive, a different UNC host -- are not # contained, so they are rejected here rather than raising from the comparison. - if not is_path_contained(target, dest): + # path_module is passed explicitly, and read at call time, so tests can patch it + # for another platform. + if not is_path_contained(target, dest, path_module=os.path): raise ValueError(f"Archive entry would extract outside target directory: {member}") _check_archive_extraction_safety(zf, dest) diff --git a/test/unit/deadline_client/job_bundle/test_repository.py b/test/unit/deadline_client/job_bundle/test_repository.py index 6de3ddfee..afa67f901 100644 --- a/test/unit/deadline_client/job_bundle/test_repository.py +++ b/test/unit/deadline_client/job_bundle/test_repository.py @@ -7,10 +7,13 @@ import io import json import math +import ntpath import os import sys import zipfile +from contextlib import contextmanager from pathlib import Path +from types import SimpleNamespace import pytest import yaml @@ -20,6 +23,7 @@ from deadline.client._path_utils import is_path_contained from deadline.client.exceptions import DeadlineOperationError +from deadline.client.job_bundle import _repository from deadline.client.job_bundle._repository import ( LocalBundleRepository, MAX_ARCHIVE_ENTRIES, @@ -725,6 +729,99 @@ def test_allows_normal_archive(self, tmp_path): assert (dest / "subdir" / "file.txt").exists() +class TestSafeZipExtractWindowsPaths: + """ + Windows path semantics for the extraction guard, exercised through a simulated + ntpath filesystem so the cases run on every platform. + + A destination at a UNC share root is the pair os.path.commonpath rejected outright + ('\\\\host\\share' vs '\\\\host\\share\\template.yaml' -> "Can't mix absolute and + relative paths"), which the guard read as an escape -- so every entry of every + archive was rejected there. + """ + + @contextmanager + def _simulated_windows_extract(self, zf): + """Run the guard against ntpath, with the extraction itself stubbed out. + + The destinations here do not exist on the host running the test, so the archive + is never written; only the containment verdict is under test. + """ + + class _WindowsPath: + def __getattr__(self, name): + return getattr(ntpath, name) + + @staticmethod + def realpath(path): + return ntpath.normpath(path) + + with ( + patch.object(_repository.os, "path", _WindowsPath()), + patch.object( + _repository.shutil, "disk_usage", lambda path: SimpleNamespace(free=1 << 40) + ), + patch.object(zf, "extractall"), + ): + yield + + def test_allows_a_unc_share_root_destination(self, tmp_path): + """The reported bug: entries of a bundle archive extracted onto a share root.""" + archive = tmp_path / "good.zip" + with zipfile.ZipFile(str(archive), "w") as zf: + zf.writestr("template.yaml", "name: Test\n") + zf.writestr("subdir/file.txt", "hello") + + with zipfile.ZipFile(str(archive), "r") as zf: + with self._simulated_windows_extract(zf): + _safe_zip_extract(zf, r"\\host\share") + + def test_pardir_at_a_share_root_is_clamped_not_an_escape(self, tmp_path): + """A share root is a path root, so '..' from it is clamped and stays inside. + + The entry lands on the share root rather than climbing to the host, so the guard + has nothing to reject. Pinned because the opposite is the intuitive reading. + """ + archive = tmp_path / "pardir.zip" + with zipfile.ZipFile(str(archive), "w") as zf: + zf.writestr("../escaped.txt", "nope") + + assert ntpath.normpath(ntpath.join(r"\\host\share", "../escaped.txt")) == ( + r"\\host\share\escaped.txt" + ) + with zipfile.ZipFile(str(archive), "r") as zf: + with self._simulated_windows_extract(zf): + _safe_zip_extract(zf, r"\\host\share") + + def test_rejects_an_escape_from_a_directory_on_a_share(self, tmp_path): + """A subdirectory of a share is the destination '..' can leave.""" + archive = tmp_path / "escape.zip" + with zipfile.ZipFile(str(archive), "w") as zf: + zf.writestr("../escaped.txt", "nope") + + with zipfile.ZipFile(str(archive), "r") as zf: + with self._simulated_windows_extract(zf): + with pytest.raises(ValueError, match="outside target directory"): + _safe_zip_extract(zf, r"\\host\share\bundle") + + def test_rejects_a_drive_relative_entry(self, tmp_path): + """'D:evil' is neither absolute nor rooted, yet it discards the destination. + + ntpath.join('\\\\host\\share', 'D:evil') is 'D:evil', so the entry lands in a + different path space entirely -- the containment check is the only thing between + it and a write outside the destination. + """ + archive = tmp_path / "drive.zip" + with zipfile.ZipFile(str(archive), "w") as zf: + zf.writestr("D:evil", "nope") + + assert not ntpath.isabs("D:evil"), "would be caught by the absolute-path check" + with zipfile.ZipFile(str(archive), "r") as zf: + with self._simulated_windows_extract(zf): + with pytest.raises(ValueError, match="outside target directory"): + _safe_zip_extract(zf, r"\\host\share") + + class TestSanitizeBundleName: def test_slashes_replaced(self): assert sanitize_bundle_name("path/to/bundle") == "path_to_bundle" From d1c5bf7d9ae4e0da9a897ef31b89a3b911cad88a Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 20 Aug 2026 08:10:36 -0700 Subject: [PATCH 13/28] refactor: extract the hook PATH-value guard to make it testable The guard sat inline in create_job_from_job_bundle with no seam, so nothing exercised it under Windows semantics on any platform or interpreter: restoring pre-3.11 isabs semantics there passed all 3580 unit tests. Its sibling call site, which takes path_module, kills the same mutation with 14 failures. Moving it to a module-level function with an injectable path_module pins both directions: a hook emitting '\\host\share' is accepted (pre-3.11 isabs read that as relative), and one emitting '\scene.ma' or 'C:scene.ma' is rejected (isabs accepted the first through 3.12). Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- src/deadline/client/api/_submit_job_bundle.py | 61 +++++++++------ .../deadline_client/job_bundle/test_hooks.py | 74 +++++++++++++++++++ 2 files changed, 114 insertions(+), 21 deletions(-) diff --git a/src/deadline/client/api/_submit_job_bundle.py b/src/deadline/client/api/_submit_job_bundle.py index 69c48d486..8023fb650 100644 --- a/src/deadline/client/api/_submit_job_bundle.py +++ b/src/deadline/client/api/_submit_job_bundle.py @@ -14,7 +14,7 @@ import textwrap from configparser import ConfigParser from typing import Any, Callable, Dict, List, Optional, Tuple, Iterable -from collections.abc import Collection +from collections.abc import Collection, Mapping from pathlib import Path import shlex from datetime import datetime @@ -98,6 +98,42 @@ def _is_known_path(path: Path | str, known_roots: Iterable[Path | str]) -> bool: return is_any_path_contained(path, known_roots, path_module=os.path) +def _reject_relative_hook_path_values( + hook_stdout_parameters: Mapping[str, Any], + job_bundle_parameters: Iterable[Mapping[str, Any]], + *, + path_module: Any = os.path, +) -> None: + """Raise if a hook emitted a PATH value that is not absolute. + + A hook's stdout parameters are layered as job_parameters overrides, which follow CLI + ``--parameter`` semantics: a relative PATH resolves against the current working + directory. A hook does not run from -- and does not control -- the submitting shell's + cwd, so a relative PATH from a hook is ambiguous (unlike an on-disk + parameter_values.yaml rewrite, which resolves against the bundle dir). + + Not ``os.path.isabs``: before Python 3.11 it reads a UNC path naming a share as + relative, rejecting a valid value on the very setup #1321 reports. + """ + bundle_parameter_types = { + p.get("name"): p.get("type") for p in job_bundle_parameters if "name" in p + } + for name, value in hook_stdout_parameters.items(): + if ( + bundle_parameter_types.get(name) == "PATH" + and isinstance(value, str) + and value != "" + and not is_absolute_path(value, path_module=path_module) + ): + raise DeadlineOperationError( + f"Pre-submission hook emitted a relative PATH value for parameter " + f"'{name}': '{value}'. Hooks must emit absolute paths for PATH " + f"parameters on stdout, since a hook does not run from the submitting " + f"working directory. Use an absolute path (e.g. join with " + f"DEADLINE_JOB_BUNDLE_DIR) or rewrite parameter_values.yaml on disk." + ) + + def _summarize_asset_paths( input_paths: Collection[Path | str], output_paths: Collection[Path | str], name: str ) -> list[str]: @@ -811,26 +847,9 @@ def _path_parameter_known_paths(resolved_parameters): # submitting shell's cwd, so a relative PATH from a hook is ambiguous (unlike an # on-disk parameter_values.yaml rewrite, which resolves against the bundle dir). # Reject relative PATH values here and require hooks to emit absolute paths. - bundle_parameter_types = { - p.get("name"): p.get("type") for p in job_bundle_parameters if "name" in p - } - for name, value in hook_stdout_parameters.items(): - if ( - bundle_parameter_types.get(name) == "PATH" - and isinstance(value, str) - and value != "" - # Not os.path.isabs: before Python 3.11 it reads a UNC path naming a - # share as relative, rejecting a valid value on the very setup #1321 - # reports. - and not is_absolute_path(value, path_module=os.path) - ): - raise DeadlineOperationError( - f"Pre-submission hook emitted a relative PATH value for parameter " - f"'{name}': '{value}'. Hooks must emit absolute paths for PATH " - f"parameters on stdout, since a hook does not run from the submitting " - f"working directory. Use an absolute path (e.g. join with " - f"DEADLINE_JOB_BUNDLE_DIR) or rewrite parameter_values.yaml on disk." - ) + _reject_relative_hook_path_values( + hook_stdout_parameters, job_bundle_parameters, path_module=os.path + ) hook_parameter_overrides = [ {"name": name, "value": value} for name, value in hook_stdout_parameters.items() diff --git a/test/unit/deadline_client/job_bundle/test_hooks.py b/test/unit/deadline_client/job_bundle/test_hooks.py index ecde69e1a..6840c544f 100644 --- a/test/unit/deadline_client/job_bundle/test_hooks.py +++ b/test/unit/deadline_client/job_bundle/test_hooks.py @@ -3,7 +3,9 @@ """Tests for submission hooks functionality.""" import json +import ntpath import os +import posixpath import signal import subprocess import sys @@ -17,6 +19,7 @@ from typing import List from deadline.client import api, config +from deadline.client.api._submit_job_bundle import _reject_relative_hook_path_values from deadline.client.exceptions import DeadlineOperationError from deadline.client.job_bundle._hooks import ( HookConfiguration, @@ -2231,3 +2234,74 @@ def test_invalid_env_dir_warns(self, tmp_path): assert sources == [] assert any("is not a valid directory" in w for w in warnings) + + +class TestRejectRelativeHookPathValues: + """ + Windows path semantics for the hook PATH-value guard, exercised by injecting the path + module so the cases run on every platform. + + ntpath.isabs cannot be the reference in either direction: before Python 3.11 it read a + UNC path naming a share as relative, which would reject a valid hook value on the very + setup #1321 reports, and it accepted a rooted, driveless path through 3.12. + """ + + PATH_PARAM = [{"name": "ScenePath", "type": "PATH"}] + + @pytest.mark.parametrize( + "value", + [ + r"\\host\share", + r"\\host\share\scene.ma", + r"\\host", + r"C:\scene.ma", + r"\\?\UNC\host\share\scene.ma", + ], + ) + def test_absolute_windows_value_is_accepted(self, value): + _reject_relative_hook_path_values({"ScenePath": value}, self.PATH_PARAM, path_module=ntpath) + + @pytest.mark.parametrize( + "value", + [ + r"relative\scene.ma", + "scene.ma", + # Rooted but driveless: 'scene.ma' at the root of whichever drive the process + # happens to be on, which is the cwd dependence the guard exists to reject. + r"\scene.ma", + # Drive-relative: the cwd on drive C:. + "C:scene.ma", + ], + ) + def test_relative_windows_value_is_rejected(self, value): + with pytest.raises(DeadlineOperationError, match="relative PATH"): + _reject_relative_hook_path_values( + {"ScenePath": value}, self.PATH_PARAM, path_module=ntpath + ) + + def test_posix_values(self): + _reject_relative_hook_path_values( + {"ScenePath": "/mnt/share/scene.ma"}, self.PATH_PARAM, path_module=posixpath + ) + with pytest.raises(DeadlineOperationError, match="relative PATH"): + _reject_relative_hook_path_values( + {"ScenePath": "relative/scene.ma"}, self.PATH_PARAM, path_module=posixpath + ) + + @pytest.mark.parametrize( + "parameters, definitions", + [ + # Only PATH parameters are checked. + ({"ScenePath": r"relative\scene.ma"}, [{"name": "ScenePath", "type": "STRING"}]), + # A parameter the bundle does not define is not a PATH value. + ({"Other": r"relative\scene.ma"}, PATH_PARAM), + # An empty value is left to the normal parameter handling. + ({"ScenePath": ""}, PATH_PARAM), + # A non-string value cannot be a path. + ({"ScenePath": 42}, PATH_PARAM), + # A definition with no name is skipped rather than raising. + ({"ScenePath": r"relative\scene.ma"}, [{"type": "PATH"}]), + ], + ) + def test_values_outside_the_guard_are_left_alone(self, parameters, definitions): + _reject_relative_hook_path_values(parameters, definitions, path_module=ntpath) From fa4f7a93ac8cc32100e6bfc3944b8ffa1d90c384 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 20 Aug 2026 08:10:37 -0700 Subject: [PATCH 14/28] test: cover the absolute PATH-default check on Windows spellings The existing cases all used a relative default, so the absolute check above the containment check never ran under the injected ntpath -- pre-3.11 isabs semantics passed the whole suite. A share-root default is the spelling that diverges, and match= is load-bearing because the containment check raises the same type. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- .../job_bundle/test_job_parameters.py | 43 +++++++++++++++++-- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/test/unit/deadline_client/job_bundle/test_job_parameters.py b/test/unit/deadline_client/job_bundle/test_job_parameters.py index f44b86280..fbca509ab 100644 --- a/test/unit/deadline_client/job_bundle/test_job_parameters.py +++ b/test/unit/deadline_client/job_bundle/test_job_parameters.py @@ -10,6 +10,7 @@ import ntpath from contextlib import contextmanager from copy import deepcopy +from typing import Any from unittest.mock import patch import pytest @@ -720,7 +721,7 @@ class TestPathDefaultContainmentWindowsPaths: } @contextmanager - def _simulated_windows_bundle(self, bundle_dir, resolves_to=None): + def _simulated_windows_bundle(self, bundle_dir, resolves_to=None, default=None): resolves_to = resolves_to or {} class _WindowsPath: @@ -734,7 +735,12 @@ def realpath(path): def read_yaml_or_json_object(bundle_dir, filename, required): # Deep-copied because read_job_bundle_parameters sets 'value' on the # parameter definitions in place. - return deepcopy(self.TEMPLATE) if filename == "template" else None + if filename != "template": + return None + template: Any = deepcopy(self.TEMPLATE) + if default is not None: + template["parameterDefinitions"][0]["default"] = default + return template with ( patch.object(parameters.os, "path", _WindowsPath()), @@ -763,7 +769,10 @@ def test_default_resolving_outside_unc_share_is_rejected(self): bundle_dir, {r"\\host\share\bundle\output": r"\\host\other\secret"}, ): - with pytest.raises(exceptions.DeadlineOperationError): + with pytest.raises( + exceptions.DeadlineOperationError, + match="specifies files outside of Job Bundle directory", + ): parameters.read_job_bundle_parameters(bundle_dir) def test_default_resolving_from_drive_bundle_onto_unc_share_is_rejected(self): @@ -772,5 +781,31 @@ def test_default_resolving_from_drive_bundle_onto_unc_share_is_rejected(self): bundle_dir, {r"C:\bundle\output": r"\\host\share\secret"}, ): - with pytest.raises(exceptions.DeadlineOperationError): + with pytest.raises( + exceptions.DeadlineOperationError, + match="specifies files outside of Job Bundle directory", + ): + parameters.read_job_bundle_parameters(bundle_dir) + + @pytest.mark.parametrize( + "default", + [ + # A share root is the spelling ntpath.splitdrive left with no tail before + # 3.11, so isabs read it as relative and it fell through to the containment + # check, which rejected it for the wrong reason. + r"\\host\share", + r"\\host", + r"\\host\share\output", + r"C:\output", + ], + ) + def test_absolute_default_is_rejected_as_absolute(self, default): + """An absolute default must be reported as absolute, not as escaping the bundle. + + ``match`` is load-bearing: both branches raise the same exception type, so without + it this passes on the misattributed error. + """ + bundle_dir = r"\\host\share\bundle" + with self._simulated_windows_bundle(bundle_dir, default=default): + with pytest.raises(exceptions.DeadlineOperationError, match="is absolute"): parameters.read_job_bundle_parameters(bundle_dir) From 9a65ce9725e01c16800650b3e8738258c1eaa734 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 20 Aug 2026 08:10:37 -0700 Subject: [PATCH 15/28] test: close the gaps an audit of these tests found Coverage: - the anti-climb backstop had one covering case; add the prefixed and device spellings whose '..' survives normalization before 3.11 (verified on 3.9). - the known-root filter is now asserted through a pre-3.11 normpath, the versions where a collapsed host-level root matches none of its own shares. - is_absolute_path is pinned against a path module whose isabs is deliberately wrong, since 3.13+ stdlib agrees with it and cannot fail the delegating form. - '..' in a root, degenerate empty paths, and the documented order tie-break. - the pathlib oracle now carries the extended-length spellings, and names folding as the second sanctioned disagreement rather than claiming one. Correctness: - five pytest.raises calls gain match=; the functions raise that type from unrelated preconditions, so a fixture drift would pass them silently. - the common_ancestor property test asserts a floor; it skips empty answers, so returning nothing for everything passed it vacuously. - drop a patch of abspath that the filter never calls, and three per-file ruff exemptions for a rule none of those files trips. - normpath leaves '..' inside '\\?\' alone before 3.11, not 3.10 (measured), and the splitroot shim's triples are not comparable across that boundary. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- pyproject.toml | 6 +- test/unit/deadline_client/_legacy_ntpath.py | 42 ++++++ .../cli/test_cli_bundle_submit_known_paths.py | 28 +++- .../job_bundle/test_job_bundle_loader.py | 12 +- .../unit/deadline_client/test_path_summary.py | 6 + test/unit/deadline_client/test_path_utils.py | 130 ++++++++++++------ 6 files changed, 173 insertions(+), 51 deletions(-) create mode 100644 test/unit/deadline_client/_legacy_ntpath.py diff --git a/pyproject.toml b/pyproject.toml index 5f472ce0a..e803a85a6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -147,11 +147,7 @@ ignore = ["E501"] "os.path.commonprefix".msg = "Use deadline.client._path_utils.is_path_contained or deadline.client._path_summary.common_ancestor; commonprefix is a string-prefix match, not a path-component match." [tool.ruff.lint.per-file-ignores] -# The sanctioned wrappers, and the one place allowed to reach for what they replace. -"src/deadline/client/_path_utils.py" = ["TID251"] -# These compare the wrappers against the stdlib behavior they replace. -"test/unit/deadline_client/test_path_utils.py" = ["TID251"] -"test/unit/deadline_client/test_path_summary.py" = ["TID251"] +# Builds an expected value with what the wrappers replace, to pin the difference. "test/unit/deadline_client/api/test_job_bundle_submission_asset_refs.py" = ["TID251"] [tool.ruff.lint.isort] diff --git a/test/unit/deadline_client/_legacy_ntpath.py b/test/unit/deadline_client/_legacy_ntpath.py new file mode 100644 index 000000000..918581231 --- /dev/null +++ b/test/unit/deadline_client/_legacy_ntpath.py @@ -0,0 +1,42 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + +"""A faithful pre-3.11 ``ntpath`` for tests, so those code paths run on any interpreter. + +Shared because more than one module needs it: the containment helpers are tested directly, +and the known-asset-root filter has to be tested through the same lens to prove it does not +lose a host-level UNC root on the interpreters where ``normpath`` collapses one. +""" + +import ntpath + + +class PreThreeElevenNtpath: + """``ntpath`` as it behaved before Python 3.11 for a UNC path that names no share. + + Both ``normpath`` and ``splitdrive`` stripped such a path down to a rooted, driveless + one. Injecting this exercises that branch on any interpreter, rather than only on the + 3.9 and 3.10 jobs -- the same reason the tests inject ``ntpath`` to begin with. + """ + + # Forces the _splitroot backport, which is what those versions had. + splitroot = None + + @staticmethod + def _is_shareless_unc(text: str) -> bool: + return text.startswith("\\\\") and "\\" not in text[2:] + + @staticmethod + def normpath(text: str) -> str: + result = ntpath.normpath(text) + if PreThreeElevenNtpath._is_shareless_unc(result): + return result[1:] + return result + + @staticmethod + def splitdrive(text: str): + if PreThreeElevenNtpath._is_shareless_unc(text): + return "", text + return ntpath.splitdrive(text) + + def __getattr__(self, name): + return getattr(ntpath, name) diff --git a/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py b/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py index a645c02e1..134c45458 100644 --- a/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py +++ b/test/unit/deadline_client/cli/test_cli_bundle_submit_known_paths.py @@ -15,6 +15,7 @@ from click.testing import CliRunner import pytest +from .._legacy_ntpath import PreThreeElevenNtpath from deadline.client import config from deadline.client.cli import main from deadline.client.api import _submit_job_bundle as sjb @@ -211,14 +212,37 @@ def test_is_known_path_windows_semantics(path, roots, expected): ([r"C:\proj", r"c:\PROJ\sub"], [r"C:\proj"]), # Drive-letter roots stay separate from UNC roots. ([r"C:\proj", r"\\host\share"], [r"C:\proj", r"\\host\share"]), + # Ties keep input order, so of two spellings of one location the caller's first -- + # highest precedence -- is the one retained. Every case above differs in depth, so + # this is what pins the documented tie-break. + ([r"C:\Proj", r"c:\proj"], [r"C:\Proj"]), + ([r"c:\proj", r"C:\Proj"], [r"c:\proj"]), + # The retained entry is the *normalized* spelling of the first input, so a + # trailing separator on it does not survive. + (["\\\\host\\", r"\\host"], [r"\\host"]), ], ) def test_filter_redundant_known_paths_windows_semantics(input, expected): - # abspath is left native so the already-absolute inputs pass through unchanged. - with patch.object(sjb.os.path, "abspath", lambda p: p), patch.object(sjb.os, "path", ntpath): + with patch.object(sjb.os, "path", ntpath): assert _filter_redundant_known_paths(input) == expected +def test_filter_redundant_known_paths_survives_pre_3_11_normpath(): + """A host-level UNC root must still subsume its shares on the interpreters where + ``normpath`` collapses the leading pair. + + ``os.path.normpath(r"\\host")`` returned ``\host`` before 3.11, moving the root out + of the UNC space so it matched none of its own shares. The filter normalizes with the + UNC-aware helper instead; injected here so the 3.9 and 3.10 behavior is asserted on + every interpreter rather than only on those matrix legs. + """ + legacy = PreThreeElevenNtpath() + assert legacy.normpath(r"\\host") == r"\host", "proxy no longer reproduces the old behavior" + with patch.object(sjb.os, "path", legacy): + assert _filter_redundant_known_paths([r"\\host", r"\\host\share"]) == [r"\\host"] + assert _filter_redundant_known_paths([r"\\host\share", r"\\host"]) == [r"\\host"] + + def test_filter_redundant_known_paths_expands_user_paths(): """ A '~'-prefixed root has to be expanded to match an absolute candidate. Such a root diff --git a/test/unit/deadline_client/job_bundle/test_job_bundle_loader.py b/test/unit/deadline_client/job_bundle/test_job_bundle_loader.py index 08f5e3bce..d53814e98 100644 --- a/test/unit/deadline_client/job_bundle/test_job_bundle_loader.py +++ b/test/unit/deadline_client/job_bundle/test_job_bundle_loader.py @@ -302,7 +302,9 @@ def test_symlink_escaping_unc_share_root_is_rejected(self): ["escape.yaml"], {r"\\host\share\escape.yaml": r"\\host\other\secret.yaml"}, ): - with pytest.raises(DeadlineOperationError): + with pytest.raises( + DeadlineOperationError, match="resolves outside of the resolved bundle directory" + ): validate_directory_symlink_containment(bundle_dir) def test_symlink_from_drive_bundle_onto_unc_share_is_rejected(self): @@ -312,7 +314,9 @@ def test_symlink_from_drive_bundle_onto_unc_share_is_rejected(self): ["escape.yaml"], {r"C:\bundle\escape.yaml": r"\\host\share\secret.yaml"}, ): - with pytest.raises(DeadlineOperationError): + with pytest.raises( + DeadlineOperationError, match="resolves outside of the resolved bundle directory" + ): validate_directory_symlink_containment(bundle_dir) def test_symlink_to_sibling_prefix_directory_is_rejected(self): @@ -322,7 +326,9 @@ def test_symlink_to_sibling_prefix_directory_is_rejected(self): ["escape.yaml"], {r"C:\bundle\escape.yaml": r"C:\bundle-secret\secret.yaml"}, ): - with pytest.raises(DeadlineOperationError): + with pytest.raises( + DeadlineOperationError, match="resolves outside of the resolved bundle directory" + ): validate_directory_symlink_containment(bundle_dir) diff --git a/test/unit/deadline_client/test_path_summary.py b/test/unit/deadline_client/test_path_summary.py index 514577e7f..4529f354f 100644 --- a/test/unit/deadline_client/test_path_summary.py +++ b/test/unit/deadline_client/test_path_summary.py @@ -38,14 +38,20 @@ def test_common_ancestor_contains_its_inputs(path_module): if path_module is ntpath else ["/a/b", "/a/c", "//a/d", "../a/b", "../a/c", "../../a/b", "rel/f", "rel/g"] ) + asserted = 0 for first in paths: for second in paths: ancestor = common_ancestor([first, second], path_module=path_module) if not ancestor: continue + asserted += 1 assert is_path_contained(first, ancestor, path_module=path_module), (first, ancestor) assert is_path_contained(second, ancestor, path_module=path_module), (second, ancestor) + # Most pairs here share no ancestor, and an empty answer is skipped above, so without a + # floor a regression that returned nothing for everything would pass this vacuously. + assert asserted >= len(paths), asserted + @pytest.mark.parametrize( "paths, path_module, expected", diff --git a/test/unit/deadline_client/test_path_utils.py b/test/unit/deadline_client/test_path_utils.py index 0cfd4bb3b..12d7646b1 100644 --- a/test/unit/deadline_client/test_path_utils.py +++ b/test/unit/deadline_client/test_path_utils.py @@ -17,6 +17,7 @@ import pytest +from ._legacy_ntpath import PreThreeElevenNtpath from deadline.client._path_utils import ( _splitroot, is_absolute_path, @@ -76,6 +77,13 @@ # A '..' that normpath cannot resolve (there is no share to clamp against) # fails closed rather than being read as a component named '..'. (r"\\host\..\other\share\f", r"\\host", False), + # An unresolvable '..' in the *root* belongs to the root, so a candidate is + # contained only if it repeats it. A known-asset root reaches here spelled as the + # config file gave it -- normalized, not resolved -- so this pair is reachable. + (r"\\host\..\other\f", r"\\host\..", True), + (r"\\host\other\f", r"\\host\..", False), + # A '..' the root *can* resolve is resolved, as anywhere else. + (r"C:\etc\f", r"C:\t\..\etc", True), # Mismatched drives are simply not contained; no exception. (r"D:\trusted\project\file", r"C:\trusted\project", False), (r"\\host\share\file", r"C:\trusted\project", False), @@ -114,9 +122,9 @@ (r"\\host\share\file", "//", False), (r"\\host\share\file", "\\\\?\\UNC\\", False), (r"\\host", "\\\\", False), - # The anchor is still reflexive, and a root naming an actual server still works. + # The anchor is still reflexive. A root naming an actual server still works; that + # is the first case in this table. ("\\\\", "\\\\", True), - (r"\\host\share\file", r"\\host", True), # 'C:' means the cwd on drive C:, so it contains drive-relative paths but not # the drive root's absolute contents. (r"C:\Windows", "C:", False), @@ -128,6 +136,17 @@ (r"C:\secret", r"\\.\C:", False), (r"\\?\Volume{abc}\trusted\f", r"Volume{abc}\trusted", False), (r"\\?\Volume{abc}\trusted\f", r"\\?\Volume{abc}\trusted", True), + # Two prefixes of one volume are still separate spaces where neither folds to a + # plain spelling. Unlike the relative-root case above, both sides are absolute + # here, so the verdict comes from the prefix and not from that mismatch. + (r"\\?\Volume{abc}\t\f", r"\\.\Volume{abc}\t", False), + # A '..' that survives normalization must not be read as a component named '..'. + # Which inputs those are depends on the interpreter -- before 3.11 normpath + # returned an extended-length path untouched, so its '..' reached the comparison + # (verified on 3.9: components are [..., 't', '..', 'evil', 'f']) -- and the + # anti-climb backstop is what makes the verdict the same on every version. + (r"\\?\Volume{abc}\t\..\evil\f", r"\\?\Volume{abc}\t", False), + (r"\\.\C:\a\..\evil", r"\\.\C:\a", False), # '\\?\C:' folds to the drive-relative 'C:' space and '\\?\C:\' to the drive root, # so each behaves as the plain spelling it denotes -- including keeping those two # spaces apart, which is why the last two disagree. @@ -239,41 +258,9 @@ def test_host_level_unc_root_containment_is_version_independent(root, contained) assert is_path_contained(r"\\host\share\f", root, path_module=ntpath) is contained -class _PreThreeElevenNtpath: - """``ntpath`` as it behaved before Python 3.11 for a UNC path that names no share. - - Both ``normpath`` and ``splitdrive`` stripped such a path down to a rooted, driveless - one. Injecting this exercises that branch on any interpreter, rather than only on the - 3.9 and 3.10 jobs -- the same reason the rest of this file injects ``ntpath``. - """ - - # Forces the _splitroot backport, which is what those versions had. - splitroot = None - - @staticmethod - def _is_shareless_unc(text: str) -> bool: - return text.startswith("\\\\") and "\\" not in text[2:] - - @staticmethod - def normpath(text: str) -> str: - result = ntpath.normpath(text) - if _PreThreeElevenNtpath._is_shareless_unc(result): - return result[1:] - return result - - @staticmethod - def splitdrive(text: str): - if _PreThreeElevenNtpath._is_shareless_unc(text): - return "", text - return ntpath.splitdrive(text) - - def __getattr__(self, name): - return getattr(ntpath, name) - - def test_host_level_unc_root_survives_pre_3_11_normpath(): """A host-level root stays in the UNC space even when normpath collapses its anchor.""" - legacy: Any = _PreThreeElevenNtpath() + legacy: Any = PreThreeElevenNtpath() # Confirm the proxy actually reproduces the old behavior, so this cannot pass vacuously. assert legacy.normpath("\\\\host") == "\\host" assert legacy.splitdrive("\\\\host") == ("", "\\\\host") @@ -327,7 +314,7 @@ def _prefixed_form(path: str) -> str: def test_extended_length_prefix_resolves_dot_segments_uniformly(): - """normpath leaves '..' alone inside a '\\\\?\\' path before 3.10 and collapses it after. + """normpath leaves '..' alone inside a '\\\\?\\' path before 3.11 and collapses it after. Folding to the plain spelling first makes the components the same on every supported interpreter, so containment does not depend on the running Python. @@ -348,6 +335,14 @@ def test_splitroot_backport_matches_stdlib(path_module): Python 3.12 added ``splitroot``; this project supports 3.9, so on older interpreters the shim is what distinguishes one path space from another. Hiding ``splitroot`` exercises the shim on any interpreter. + + The comparison only runs where the stdlib has an oracle. It is deliberately not + replaced by frozen triples for the older interpreters: the shim reads the running + ``splitdrive``, which itself changed in 3.11, so the correct pre-3.11 triples differ + from these (measured on 3.9: ``\\\\srv`` splits as ``("", "\\", "\\srv")``, not + ``("\\\\srv", "", "")``). What must hold on those versions is the downstream verdict, + which :func:`test_host_level_unc_root_survives_pre_3_11_normpath` and the filter's + legacy-proxy test assert directly. """ if not hasattr(path_module, "splitroot"): pytest.skip("stdlib splitroot unavailable, nothing to compare against") @@ -397,11 +392,14 @@ def __getattr__(self, name): def test_agrees_with_pathlib_except_for_unc_hosts(path_module): """Differential check against ``PurePath.is_relative_to`` as an independent oracle. - pathlib folds a UNC server and share into one atom, so it cannot see a host-level root - as an ancestor of its shares -- that gap is issue #1321 and the only sanctioned - disagreement. Elsewhere pathlib is the reference. It does not resolve '..', so the - corpus avoids inputs needing normalization; this supplements the explicit cases above - rather than replacing them. + Two disagreements are sanctioned, and in one direction only -- we may be more + permissive than pathlib, never the reverse. First, pathlib folds a UNC server and share + into one atom, so it cannot see a host-level root as an ancestor of its shares: that + gap is issue #1321. Second, pathlib keeps an extended-length prefix as part of the + drive, so it cannot see that '\\\\?\\C:\\a' denotes the same location as 'C:\\a'. + Elsewhere pathlib is the reference. It does not resolve '..', so the corpus avoids + inputs needing normalization; this supplements the explicit cases above rather than + replacing them. """ if path_module is ntpath: flavour: Any = PureWindowsPath @@ -428,6 +426,9 @@ def test_agrees_with_pathlib_except_for_unc_hosts(path_module): "C:foo", r"\\.\C:\a", r"\\?\Volume{abc}\a", + r"\\?\C:\a", + r"\\?\C:\a\b", + r"\\?\UNC\srv\sh\a", ] else: flavour = PurePosixPath @@ -443,6 +444,11 @@ def test_agrees_with_pathlib_except_for_unc_hosts(path_module): # more permissive than pathlib here, never elsewhere and never in reverse. assert path_module is ntpath, (candidate, root, ours, pathlibs) assert ours is True and pathlibs is False, (candidate, root, ours, pathlibs) + if candidate.startswith("\\\\?\\") or root.startswith("\\\\?\\"): + # Folding. The explicit table above pins which prefixed spellings fold to + # what; pathlib cannot be the oracle for it, so all this can check is the + # direction asserted above. + continue # The root must name an actual server. The bare '\\\\' anchor names none, so it # is not a sanctioned disagreement -- pathlib is right to contain nothing there. assert flavour(root).drive.startswith("\\\\"), (candidate, root) @@ -451,6 +457,48 @@ def test_agrees_with_pathlib_except_for_unc_hosts(path_module): assert not flavour(root).parts[1:], (candidate, root) +def test_is_absolute_path_does_not_delegate_to_the_stdlib(): + """``isabs`` cannot be the reference, so the helper must not consult it. + + ``ntpath.isabs`` disagrees with itself across the supported range -- it read + ``\\\\host\\share`` as relative before 3.11 and accepted ``\\x`` through 3.12 -- so a + delegating implementation would answer differently depending on the interpreter. On + 3.13+ the stdlib happens to agree with the helper, which is why this injects a path + module whose ``isabs`` is deliberately wrong in both directions rather than relying on + the version matrix to notice. + """ + + class _WrongIsabs: + @staticmethod + def isabs(path): + return not path.startswith("\\\\") + + def __getattr__(self, name): + return getattr(ntpath, name) + + wrong: Any = _WrongIsabs() + assert wrong.isabs(r"\\host\share") is False, "proxy no longer disagrees with the helper" + assert is_absolute_path(r"\\host\share", path_module=wrong) is True + assert is_absolute_path(r"\x", path_module=wrong) is False + assert is_absolute_path("C:foo", path_module=wrong) is False + assert is_absolute_path(r"C:\a", path_module=wrong) is True + + +def test_containment_of_degenerate_paths(): + """The empty string normalizes to the current directory, so it is its own space. + + It is a real input -- ``--known-asset-path ""``, MCP JSON, and a PATH parameter whose + allowedValues suppressed absolutization all produce one -- and the callers drop it on + truthiness before it reaches here. This pins the verdict for any that do not, so an + empty root can never be read as an ancestor of an absolute path. + """ + for module in (ntpath, posixpath): + assert path_components("", path_module=module) == ["."] + assert is_path_contained("", "", path_module=module) is True + assert is_path_contained("/a", "", path_module=module) is False + assert is_path_contained("", "/", path_module=module) is False + + def test_is_any_path_contained(): assert is_any_path_contained("/a/f", ["/b", "/a"]) is True assert is_any_path_contained("/c/f", ["/b", "/a"]) is False From cc49598de41bd8bcf82de95974946db6f53d13f4 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 20 Aug 2026 08:10:37 -0700 Subject: [PATCH 16/28] ci: gate pull requests on the SMB suite, and stop pretending elsewhere Nothing on a pull request could reach test/integ/windows_smb: the reusable build runs `hatch run test`, whose script hard-codes test/unit and test/cli_e2e. So the only tests that check these verdicts against a real redirector ran after merge, and a wrong assertion in them stayed green through review. Run it on pull requests touching the paths it covers. Meanwhile the mainline integ jobs collected this directory and skipped all of it -- no share, no DEADLINE_SMB_TESTS_REQUIRED -- reading as coverage they did not have; exclude it so the dedicated workflow owns it. Replace the Developer Mode step, which cannot grant CPython the symlink privilege, with the SymlinkEvaluation setting the escape test actually depends on. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- .github/workflows/windows_smb_test.yml | 32 ++++++++++++++++++-------- hatch.toml | 4 +++- 2 files changed, 25 insertions(+), 11 deletions(-) diff --git a/.github/workflows/windows_smb_test.yml b/.github/workflows/windows_smb_test.yml index d7933cd3b..f79222d31 100644 --- a/.github/workflows/windows_smb_test.yml +++ b/.github/workflows/windows_smb_test.yml @@ -3,13 +3,23 @@ name: Windows SMB Path Test # Validates UNC path containment against a real SMB share, which lexical ntpath modeling # cannot do. Regression coverage for issue #1321. # -# 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. It runs post-merge -# instead, matching dcm_integration_tests.yml -- if it only ran on manual dispatch a -# regression in UNC handling would ship, since test/integ is outside testpaths and so no -# other job reaches it, and the unit tests are lexical by their own docstring. +# Not part of Code Quality, which runs the reusable Python build: that invokes +# `hatch run test`, whose script hard-codes test/unit and test/cli_e2e, so no job it runs +# can reach test/integ. This workflow owns these tests, and it has to gate pull requests +# to be worth anything -- a wrong assertion here is invisible to every other check, so +# post-merge-only means it lands green and fails on mainline. GitHub-hosted windows +# runners are already elevated, so `net share` works; the cost is the Windows env install +# on each run, which is why the path filter keeps it off unrelated pull requests. on: workflow_dispatch: + pull_request: + paths: + - 'src/deadline/client/_path_utils.py' + - 'src/deadline/client/_path_summary.py' + - 'src/deadline/client/api/_submit_job_bundle.py' + - 'src/deadline/client/job_bundle/**' + - 'test/integ/windows_smb/**' + - '.github/workflows/windows_smb_test.yml' push: branches: [mainline] workflow_call: @@ -46,11 +56,13 @@ jobs: Get-Service LanmanServer, LanmanWorkstation | Format-Table -AutoSize Start-Service LanmanServer Start-Service LanmanWorkstation - # Developer Mode lets a non-elevated process create symlinks; the escape - # test needs one and fails below if it cannot get one. - $key = 'HKLM:\SOFTWARE\Microsoft\Windows\CurrentVersion\AppModelUnlock' - New-Item -Path $key -Force | Out-Null - Set-ItemProperty -Path $key -Name AllowDevelopmentWithoutDevLicense -Value 1 -Type DWord + # The symlink the escape test needs comes from the runner being elevated: + # CreateSymbolicLinkW needs SeCreateSymbolicLinkPrivilege, and CPython does not + # pass the flag that would let Developer Mode substitute for it. The test fails + # loudly (not skips) if the privilege is ever missing. + # Remote-to-local symlink evaluation must be on, or realpath leaves the link + # unresolved and the escape reads as contained. + fsutil behavior set SymlinkEvaluation R2L:1 R2R:1 - name: Install run: | diff --git a/hatch.toml b/hatch.toml index b058d0e92..97ed33b42 100644 --- a/hatch.toml +++ b/hatch.toml @@ -28,7 +28,9 @@ pre-install-commands = [ ] [envs.integ.scripts] -test = "pytest --xfail-tb --no-cov -vvv --numprocesses=1 {args:test/integ}" +# windows_smb is excluded: it needs a hosted SMB share, so it skips silently here and +# reads as coverage this run does not have. windows_smb_test.yml owns it. +test = "pytest --xfail-tb --no-cov -vvv --numprocesses=1 {args:test/integ --ignore=test/integ/windows_smb}" proxy-test = "sudo -E env PATH=$PATH bash scripts/run_proxy_integ_tests.sh {args:test/integ/cli/}" [envs.ui] From 7db87fdcc0b1e39e57a33b6e6af150600149da20 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 20 Aug 2026 08:10:37 -0700 Subject: [PATCH 17/28] test: stop the SMB cases from choosing their own assertions The mapped-drive test branched on realpath's output, so a containment regression in the drive-letter space would steer it into the branch that does not check that space, and the surviving assertion duplicated an earlier test. Pin the rewrite instead, then assert both spellings, including the limitation that a mapped-drive root does not cover files that resolve to UNC form. The symlink escape targeted the local C: spelling of a file inside the share, so the rejection came from comparing path spaces rather than from leaving the bundle. Target a sibling through the share, pin that realpath resolves the link at all, and match the message. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- .../windows_smb/test_unc_path_containment.py | 41 ++++++++++++++----- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/test/integ/windows_smb/test_unc_path_containment.py b/test/integ/windows_smb/test_unc_path_containment.py index 9b8cbcff1..11e08227a 100644 --- a/test/integ/windows_smb/test_unc_path_containment.py +++ b/test/integ/windows_smb/test_unc_path_containment.py @@ -200,17 +200,22 @@ def test_bundle_on_share_passes_symlink_containment(smb_share): validate_directory_symlink_containment(str(root_bundle)) -def test_symlink_escaping_the_share_is_rejected(smb_share): +def test_symlink_escaping_the_bundle_on_a_share_is_rejected(smb_share): """A symlink out of a bundle on a share must still be caught. This is the security direction: the lexical tests assert it, but only a real filesystem exercises the ``realpath`` resolution the guard depends on. + + The target is a sibling of the bundle spelled through the share, so the verdict + comes from leaving the bundle -- not from the local ``C:`` spelling of the same + file being a different path space, which would be caught by drive comparison + alone and would leave the climb itself untested. """ - unc_root, local_path = smb_share + unc_root, _ = smb_share bundle = Path(unc_root) / "escape_bundle" bundle.mkdir(parents=True, exist_ok=True) - outside = local_path / "outside_secret.txt" + outside = Path(unc_root) / "outside_secret.txt" outside.write_text("secret", encoding="utf8") link = bundle / "escape.txt" @@ -219,7 +224,12 @@ def test_symlink_escaping_the_share_is_rejected(smb_share): except OSError as exc: # pragma: no cover - depends on runner privileges _unavailable(f"cannot create a symlink on this share: {exc}") - with pytest.raises(DeadlineOperationError): + # Pin the resolution the guard depends on: an unresolved link would be read as + # inside the bundle, and the escape would pass unnoticed. + assert os.path.realpath(link).lower() == os.path.realpath(outside).lower() + with pytest.raises( + DeadlineOperationError, match="resolves outside of the resolved bundle directory" + ): validate_directory_symlink_containment(str(bundle)) @@ -300,14 +310,23 @@ def test_mapped_drive_resolves_and_compares(smb_share): asset = Path(drive + "\\") / "mapped_probe.txt" asset.write_text("mapped", encoding="utf8") + # Pinned rather than branched on: the redirector rewrites the mapping back to + # UNC form, and every assertion below follows from that. Branching on it would + # let a containment regression steer the test into the case it does not check. resolved = os.path.realpath(asset) - # Whatever spelling realpath returns, it must be contained by the matching - # root and not by the other path space. - if resolved.startswith("\\\\"): - assert is_path_contained(resolved, host_root) - else: - assert is_path_contained(resolved, drive + "\\") - assert not is_path_contained(resolved, host_root) + assert resolved.startswith("\\\\"), ( + f"realpath kept the drive-letter spelling ({resolved}); the assertions below " + "assume it resolves to UNC form" + ) + assert is_path_contained(resolved, unc_root) + assert is_path_contained(resolved, host_root) + + # The two spellings are separate path spaces, so a mapped-drive root does not + # cover the same files once they resolve to UNC form, and vice versa. That is a + # real limitation for a studio that configures 'Z:\proj' as a known asset root. + assert not is_path_contained(resolved, drive + "\\") + assert is_path_contained(str(asset), drive + "\\") + assert not is_path_contained(str(asset), host_root) finally: _run("net", "use", drive, "/DELETE", "/Y") From c113ca1abd0ad72e7a8687cdc6c5c689eb0fad38 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 20 Aug 2026 09:38:51 -0700 Subject: [PATCH 18/28] fix: resolve path_module at call time, not in the signature A default argument binds os.path when the module is imported, so a caller that omitted path_module could not be redirected by a test patching os.path -- which is why the archive guard's UNC behavior was unreachable from test/unit until it started passing one explicitly. Resolving None in the body makes the omission harmless: every call site is patchable whether or not it passes one. Also corrects three claims these modules made: - the docstring said every function takes an explicit path_module; it takes an optional one. - the commonpath explanation was written as universal but describes 3.9/3.10. From 3.11 splitdrive does report a drive for '\\server' and the exception changes message; it raises on every supported version, which is the part that matters. - the component trie's first key is the path-space anchor ('/'), not ''. The security argument in that same docstring depends on it. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- src/deadline/client/_path_summary.py | 6 +++- src/deadline/client/_path_utils.py | 32 ++++++++++++------- src/deadline/client/api/_submit_job_bundle.py | 2 +- 3 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/deadline/client/_path_summary.py b/src/deadline/client/_path_summary.py index f15126f80..f0aa8c1a2 100644 --- a/src/deadline/client/_path_summary.py +++ b/src/deadline/client/_path_summary.py @@ -37,7 +37,7 @@ def _leading_pardir_count(parts: list[str]) -> int: return count -def common_ancestor(paths: Sequence[Any], *, path_module: Any = os.path) -> str: +def common_ancestor(paths: Sequence[Any], *, path_module: Any = None) -> str: """Return the deepest directory containing every path in ``paths``. This is ``os.path.commonpath`` without the exceptions: paths in unrelated spaces return @@ -45,6 +45,10 @@ def common_ancestor(paths: Sequence[Any], *, path_module: Any = os.path) -> str: shares of one server. The result keeps the first path's spelling and, like ``commonpath``, is purely lexical. """ + # Resolved here, not in the signature: a default argument binds os.path when this + # module is imported, which would silently ignore a test's patch of it and make a + # caller that omits it untestable on another platform. + path_module = path_module or os.path if not paths: return "" diff --git a/src/deadline/client/_path_utils.py b/src/deadline/client/_path_utils.py index 531672059..414ba8142 100644 --- a/src/deadline/client/_path_utils.py +++ b/src/deadline/client/_path_utils.py @@ -4,13 +4,17 @@ Path containment helpers that understand Windows UNC paths. ``os.path.commonpath`` raises ``ValueError`` rather than comparing a host-level UNC path -(``\\\\server``) with a path under one of its shares: ``splitdrive`` reports no drive for -the former and ``\\\\server\\share`` for the latter. It raises the same way for two shares -on one host. Callers that read that exception as "not contained" reject valid paths. +(``\\\\server``) with a path under one of its shares, and raises the same way for two shares +on one host. Which message it raises depends on the interpreter -- before 3.11 +``splitdrive`` reported no drive for ``\\\\server``, so the pair read as mixing absolute +and relative; from 3.11 it reports one and they read as different drives -- but it raises +on every supported version. Callers that read that exception as "not contained" reject +valid paths. These helpers compare paths component by component instead, so a UNC host is an ordinary -ancestor of its shares. Every function takes an explicit ``path_module`` -(``ntpath``/``posixpath``), so Windows semantics stay testable on non-Windows hosts. +ancestor of its shares. Every function takes an optional ``path_module`` +(``ntpath``/``posixpath``), resolved to ``os.path`` at call time rather than in the +signature, so Windows semantics stay testable on non-Windows hosts from any call site. Comparisons are lexical -- pass ``realpath`` output in if symlinks must be resolved -- and never raise. Anything unresolvable fails closed, since callers use containment to decide @@ -146,7 +150,7 @@ def _split_anchored(path: Any, path_module: Any, normalize_case: bool) -> tuple[ def path_components( path: Any, *, - path_module: Any = os.path, + path_module: Any = None, normalize_case: bool = True, ) -> list[str]: """Split ``path`` into the components used for ancestor comparisons. @@ -162,11 +166,15 @@ def path_components( ``normalize_case`` lowercases components on Windows to match the filesystem. """ + # Resolved here, not in the signature: a default argument binds os.path when this + # module is imported, which would silently ignore a test's patch of it and make a + # caller that omits it untestable on another platform. + path_module = path_module or os.path anchor, parts = _split_anchored(path, path_module, normalize_case) return ([anchor] if anchor else []) + parts -def is_absolute_path(path: Any, *, path_module: Any = os.path) -> bool: +def is_absolute_path(path: Any, *, path_module: Any = None) -> bool: """Return True iff ``path`` names a location without consulting the working directory. ``path_module.isabs`` cannot be used before Python 3.11: it tests what ``splitdrive`` @@ -185,6 +193,7 @@ def is_absolute_path(path: Any, *, path_module: Any = os.path) -> bool: the reference in either direction. Answering from the anchor keeps the verdict the same on every version. """ + path_module = path_module or os.path anchor, _ = _split_anchored(path, path_module, normalize_case=True) if not anchor: return False @@ -193,7 +202,7 @@ def is_absolute_path(path: Any, *, path_module: Any = os.path) -> bool: return not _denotes_drive(anchor) and anchor != path_module.sep -def is_bare_unc_anchor(path: Any, *, path_module: Any = os.path) -> bool: +def is_bare_unc_anchor(path: Any, *, path_module: Any = None) -> bool: """True iff ``path`` is the bare ``\\\\`` marker, which names no server. It is fully qualified yet identifies no location, so it contains nothing -- @@ -207,7 +216,7 @@ def is_bare_unc_anchor(path: Any, *, path_module: Any = os.path) -> bool: return path_components(path, path_module=path_module) == [_UNC_ANCHOR] -def normalized_path(path: Any, *, path_module: Any = os.path) -> str: +def normalized_path(path: Any, *, path_module: Any = None) -> str: """Return ``path`` with ``..``, ``.``, repeated separators and separator style resolved. ``path_module.normpath`` with the version differences handled: before Python 3.11 it @@ -215,6 +224,7 @@ def normalized_path(path: Any, *, path_module: Any = os.path) -> str: moving a host-level root out of the UNC space so it matches none of its own shares. Case is preserved, unlike the components used for comparison. """ + path_module = path_module or os.path anchor, parts = _split_anchored(path, path_module, normalize_case=False) return anchor + path_module.sep.join(parts) @@ -223,7 +233,7 @@ def is_path_contained( path: Any, root: Any, *, - path_module: Any = os.path, + path_module: Any = None, ) -> bool: """Return True iff ``path`` equals or is a descendant of ``root``. @@ -248,7 +258,7 @@ def is_any_path_contained( path: Any, roots: Iterable[Any], *, - path_module: Any = os.path, + path_module: Any = None, ) -> bool: """Return True iff ``path`` is contained by any root in ``roots``.""" return any(is_path_contained(path, root, path_module=path_module) for root in roots) diff --git a/src/deadline/client/api/_submit_job_bundle.py b/src/deadline/client/api/_submit_job_bundle.py index 8023fb650..298b51a44 100644 --- a/src/deadline/client/api/_submit_job_bundle.py +++ b/src/deadline/client/api/_submit_job_bundle.py @@ -330,7 +330,7 @@ def _filter_redundant_known_paths(known_asset_paths: Iterable[str]) -> list[str] 1. Sort the paths from fewest to most components, so any prefix of a path has to happen before that path. 2. For each path, split it into parts (i.e. '/mnt/prod/project' becomes - ['', 'mnt', 'prod', 'project']), and then insert it part by part into + ['/', 'mnt', 'prod', 'project']), and then insert it part by part into a nested dict called dir_tree organized as a TRIE. The value True in the TRIE indicates that a path with that as its final part is in the list. 3. While inserting a path into the TRIE, detect whether another path already From 2fc4e6fe6ea038b7312a97d521de95a942ecbe91 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 20 Aug 2026 09:38:51 -0700 Subject: [PATCH 19/28] test: make platform-gated cases assert everywhere, and two raises specific `if sys.platform == "win32": assert ...` with no else passes while executing nothing on the other two CI platforms -- half of sanitize_bundle_name's behavior, including its traversal guard, went unverified on linux and macos. It reads sys.platform at call time, so patching it pins both verdicts anywhere. The pre-existing symlink containment test used a bare pytest.raises for the guard this branch rewired; that function raises the same type for "path is not a directory", so fixture drift would pass it without reaching the check. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- .../job_bundle/test_job_bundle_loader.py | 8 +++- .../job_bundle/test_repository.py | 46 ++++++++++++------- 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/test/unit/deadline_client/job_bundle/test_job_bundle_loader.py b/test/unit/deadline_client/job_bundle/test_job_bundle_loader.py index d53814e98..92d3277ce 100644 --- a/test/unit/deadline_client/job_bundle/test_job_bundle_loader.py +++ b/test/unit/deadline_client/job_bundle/test_job_bundle_loader.py @@ -236,12 +236,16 @@ def test_validate_directory_symlink_containment_fail(tmpdir): symlink_dir = test_root.join("symlink_dir") os.symlink(target_dir, test_root.join("symlink_dir"), target_is_directory=True) - with pytest.raises(DeadlineOperationError): + with pytest.raises( + DeadlineOperationError, match="resolves outside of the resolved bundle directory" + ): validate_directory_symlink_containment(str(test_root)) os.unlink(symlink_dir) os.symlink(target_file, test_root.join("symlink_file.txt")) - with pytest.raises(DeadlineOperationError): + with pytest.raises( + DeadlineOperationError, match="resolves outside of the resolved bundle directory" + ): validate_directory_symlink_containment(str(test_root)) diff --git a/test/unit/deadline_client/job_bundle/test_repository.py b/test/unit/deadline_client/job_bundle/test_repository.py index afa67f901..7840657a2 100644 --- a/test/unit/deadline_client/job_bundle/test_repository.py +++ b/test/unit/deadline_client/job_bundle/test_repository.py @@ -9,7 +9,6 @@ import math import ntpath import os -import sys import zipfile from contextlib import contextmanager from pathlib import Path @@ -826,21 +825,36 @@ class TestSanitizeBundleName: def test_slashes_replaced(self): assert sanitize_bundle_name("path/to/bundle") == "path_to_bundle" - def test_backslashes_replaced_on_windows(self): - if sys.platform == "win32": - assert sanitize_bundle_name("path\\to\\bundle") == "path_to_bundle" - - def test_backslashes_preserved_on_posix(self): - if sys.platform != "win32": - assert sanitize_bundle_name("path\\to\\bundle") == "path\\to\\bundle" - - def test_windows_illegal_chars_replaced_on_windows(self): - if sys.platform == "win32": - assert sanitize_bundle_name("file:name*with?bad") == "file_name_with_bad_chars_" - - def test_colons_preserved_on_posix(self): - if sys.platform != "win32": - assert sanitize_bundle_name("my:bundle") == "my:bundle" + @pytest.mark.parametrize( + "platform, name, expected", + [ + # Only what is illegal on the running OS is replaced, so each verdict holds on + # one platform and not the other. The function reads sys.platform at call time, + # so patching it pins both on every host -- written as a bare `if + # sys.platform ...` these asserted nothing on two of the three CI platforms. + ("win32", "path\\to\\bundle", "path_to_bundle"), + ("linux", "path\\to\\bundle", "path\\to\\bundle"), + ("win32", "file:name*with?bad", "file_name_with_bad_chars_"), + ("linux", "file:name*with?bad", "file:name*with?bad"), + ("win32", 'a"b|c', "a_b_c"), + ("linux", "my:bundle", "my:bundle"), + ("win32", "my:bundle", "my_bundle"), + # Traversal: a separator that is illegal here is replaced, which flattens the + # name to one component and leaves nothing to climb with. + ("win32", "..\\..\\evil", ".._.._evil"), + ("linux", "../../evil", ".._.._evil"), + ], + ) + def test_platform_specific_sanitization(self, platform, name, expected): + with patch.object(_repository.sys, "platform", platform): + assert sanitize_bundle_name(name) == expected + + def test_traversal_components_are_rejected_where_the_separator_is_legal(self): + """A backslash is an ordinary filename character on POSIX, so it survives + sanitization and the '..' components it separates have to be rejected outright.""" + with patch.object(_repository.sys, "platform", "linux"): + with pytest.raises(ValueError, match="empty or unsafe"): + sanitize_bundle_name("..\\..\\evil") def test_empty_after_sanitization_raises(self): with pytest.raises(ValueError, match="empty or unsafe"): From 412a31782b9a4cd28f267620013ccee21e4372f8 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 20 Aug 2026 09:38:51 -0700 Subject: [PATCH 20/28] test: give the pathlib oracle a disagreement floor Every agreeing pair takes a `continue` and all assertions sit after it, so the test passed having executed zero assertions -- including when is_path_contained is replaced by a pathlib-delegating implementation, which is the exact regression it exists to catch. Count both sanctioned classes and assert the corpus produces them, mirroring the floor its sibling in test_path_summary.py already has. Also fixes an inline comment still claiming one sanctioned disagreement where the docstring above it now says two. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- test/unit/deadline_client/test_path_utils.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/test/unit/deadline_client/test_path_utils.py b/test/unit/deadline_client/test_path_utils.py index 12d7646b1..b759016d3 100644 --- a/test/unit/deadline_client/test_path_utils.py +++ b/test/unit/deadline_client/test_path_utils.py @@ -434,12 +434,14 @@ def test_agrees_with_pathlib_except_for_unc_hosts(path_module): flavour = PurePosixPath corpus = ["/", "/a", "/a/b", "/a-secret", "rel", "rel/f"] + unc_disagreements = 0 + fold_disagreements = 0 for candidate, root in itertools.permutations(corpus, 2): ours = is_path_contained(candidate, root, path_module=path_module) pathlibs = flavour(candidate).is_relative_to(flavour(root)) if ours == pathlibs: continue - # The only sanctioned disagreement: a UNC root that pathlib cannot see as an + # The first sanctioned disagreement: a UNC root that pathlib cannot see as an # ancestor because it folds the server and share into one atom. We may only be # more permissive than pathlib here, never elsewhere and never in reverse. assert path_module is ntpath, (candidate, root, ours, pathlibs) @@ -448,7 +450,9 @@ def test_agrees_with_pathlib_except_for_unc_hosts(path_module): # Folding. The explicit table above pins which prefixed spellings fold to # what; pathlib cannot be the oracle for it, so all this can check is the # direction asserted above. + fold_disagreements += 1 continue + unc_disagreements += 1 # The root must name an actual server. The bare '\\\\' anchor names none, so it # is not a sanctioned disagreement -- pathlib is right to contain nothing there. assert flavour(root).drive.startswith("\\\\"), (candidate, root) @@ -456,6 +460,17 @@ def test_agrees_with_pathlib_except_for_unc_hosts(path_module): assert flavour(candidate).drive.startswith("\\\\"), (candidate, root) assert not flavour(root).parts[1:], (candidate, root) + # Every agreeing pair takes a `continue`, so without a floor this passes having + # asserted nothing -- including if the module regressed to pathlib's own semantics, + # which is the bug being fixed. The counts are what the corpus produces today. + if path_module is ntpath: + assert unc_disagreements >= 3, unc_disagreements + assert fold_disagreements >= 9, fold_disagreements + else: + # pathlib and these helpers agree everywhere on POSIX; the value of this leg is + # that it stays that way. + assert (unc_disagreements, fold_disagreements) == (0, 0) + def test_is_absolute_path_does_not_delegate_to_the_stdlib(): """``isabs`` cannot be the reference, so the helper must not consult it. From 5b6b64e38ca02c5df1e0733546e6ce0e074ed731 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 20 Aug 2026 09:39:42 -0700 Subject: [PATCH 21/28] fix: accept a host-level UNC download root _assert_valid_path validated a root arriving over the JSON protocol with Path.is_absolute, which reads '\\host' as relative before Python 3.13 and absolute from 3.13 -- so the same host-level UNC path #1321 reports for containment was rejected outright as a download root on four of the six supported versions. It had no tests: both references to it in the suite patch it out. Use the version-independent helper, with the path module injected as the sibling call sites do, and cover both directions including the rooted-driveless and drive-relative spellings that must stay rejected. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- src/deadline/client/cli/_groups/job_group.py | 14 +++--- test/unit/deadline_client/cli/test_cli_job.py | 44 +++++++++++++++++++ 2 files changed, 53 insertions(+), 5 deletions(-) diff --git a/src/deadline/client/cli/_groups/job_group.py b/src/deadline/client/cli/_groups/job_group.py index d3c3a8ef9..da2d74fcd 100644 --- a/src/deadline/client/cli/_groups/job_group.py +++ b/src/deadline/client/cli/_groups/job_group.py @@ -42,6 +42,7 @@ from ... import api from ...config import config_file from ..._path_summary import common_ancestor +from ..._path_utils import is_absolute_path from ...exceptions import DeadlineOperationError, DeadlineOperationTimedOut from .._common import ( _OUTPUT_FORMAT_HELP, @@ -551,7 +552,7 @@ def _prompt_for_os_mismatch_roots( new_root = _get_value_from_json_line( json_string, JSON_MSG_TYPE_PATHCONFIRM, expected_size=1 )[0] - _assert_valid_path(new_root) + _assert_valid_path(new_root, path_module=os.path) downloader.set_root_path(asset_root, os.path.expanduser(new_root)) return downloader.get_paths_by_root() @@ -602,7 +603,7 @@ def _prompt_to_confirm_roots( json_string, JSON_MSG_TYPE_PATHCONFIRM, expected_size=len(asset_roots) ) for index, confirmed_root in enumerate(confirmed_asset_roots): - _assert_valid_path(confirmed_root) + _assert_valid_path(confirmed_root, path_module=os.path) downloader.set_root_path(asset_roots[index], str(Path(confirmed_root))) paths_by_root = downloader.get_paths_by_root() if on_roots_changed: @@ -1020,12 +1021,15 @@ def _get_value_from_json_line( raise ValueError(f"Invalid JSON line '{json_line}': {e}") -def _assert_valid_path(path: str) -> None: +def _assert_valid_path(path: str, *, path_module: Any = None) -> None: """ Validates that the path has the format of the OS currently running. + + Not ``Path.is_absolute``, which reads a host-level UNC path as relative before + Python 3.13 and so rejects '\\\\host' as a download root on four of the six + supported versions -- the same disagreement #1321 reports for containment. """ - path_obj = Path(path) - if not path_obj.is_absolute(): + if not is_absolute_path(path, path_module=path_module): raise ValueError(f"Path {path} is not an absolute path.") diff --git a/test/unit/deadline_client/cli/test_cli_job.py b/test/unit/deadline_client/cli/test_cli_job.py index d7ba97057..98b6c8a60 100644 --- a/test/unit/deadline_client/cli/test_cli_job.py +++ b/test/unit/deadline_client/cli/test_cli_job.py @@ -9,6 +9,7 @@ import json import ntpath import os +import posixpath from typing import Dict, List import pytest from pathlib import Path @@ -1556,6 +1557,49 @@ def test_cli_job_download_output_with_different_asset_root_path_format_than_job( mock_expanduser.assert_any_call("~") +class TestAssertValidPath: + """The download-root validation applied to paths arriving over the JSON protocol. + + Not Path.is_absolute: PureWindowsPath(r"\\host").is_absolute() is False before 3.13 + and True from 3.13, so a host-level UNC download root was rejected on four of the six + supported versions. Injecting the path module pins the verdict on every platform. + """ + + @pytest.mark.parametrize( + "path", + [ + r"\\host\share\out", + r"\\host\share", + # A host-level root: the spelling Path.is_absolute disagrees with itself on. + r"\\host", + r"C:\out", + "C:\\", + ], + ) + def test_absolute_windows_path_is_accepted(self, path): + job_group._assert_valid_path(path, path_module=ntpath) + + @pytest.mark.parametrize( + "path", + [ + r"relative\out", + "out", + # Rooted but driveless, and drive-relative: both resolve against the cwd. + r"\out", + "C:out", + "", + ], + ) + def test_non_absolute_windows_path_is_rejected(self, path): + with pytest.raises(ValueError, match="is not an absolute path"): + job_group._assert_valid_path(path, path_module=ntpath) + + def test_posix_paths(self): + job_group._assert_valid_path("/mnt/share/out", path_module=posixpath) + with pytest.raises(ValueError, match="is not an absolute path"): + job_group._assert_valid_path("relative/out", path_module=posixpath) + + class TestJsonLineHelpers: """Tests for JSON line helper functions.""" From c62f7fc0053e542e460b94a1f01a23d30aa44592 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 20 Aug 2026 09:40:40 -0700 Subject: [PATCH 22/28] chore: ban commonprefix on the path modules the code actually passes around The ban's own comment says ntpath and posixpath are included because this codebase passes explicit path modules, then applied that to commonpath only -- so ntpath.commonprefix, the string-prefix match that reports '\\host\share2' as inside '\\host\share', was allowed. Verified both spellings now flag. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- pyproject.toml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index e803a85a6..26fe4772a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -145,9 +145,15 @@ ignore = ["E501"] # commonprefix compares strings, not path components, so it reports '\\host\share2' as # sharing a prefix with '\\host\share'. It is never the right containment primitive. "os.path.commonprefix".msg = "Use deadline.client._path_utils.is_path_contained or deadline.client._path_summary.common_ancestor; commonprefix is a string-prefix match, not a path-component match." +"ntpath.commonprefix".msg = "Use deadline.client._path_utils.is_path_contained or deadline.client._path_summary.common_ancestor; commonprefix is a string-prefix match, not a path-component match." +"posixpath.commonprefix".msg = "Use deadline.client._path_utils.is_path_contained or deadline.client._path_summary.common_ancestor; commonprefix is a string-prefix match, not a path-component match." +# The rule resolves names statically, so it cannot see `path_module.commonpath(...)` -- the +# spelling this codebase actually uses. It catches the direct calls; the helpers are what +# keep the indirect ones honest. [tool.ruff.lint.per-file-ignores] -# Builds an expected value with what the wrappers replace, to pin the difference. +# Mirrors the root-path grouping done inside deadline.job_attachments, which is a separate +# package and not ours to change, so the expected value has to be built the same way. "test/unit/deadline_client/api/test_job_bundle_submission_asset_refs.py" = ["TID251"] [tool.ruff.lint.isort] From c4857bd007c11bd52631bc7a83e14a16183415e7 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 20 Aug 2026 09:52:13 -0700 Subject: [PATCH 23/28] test: wait for the message, not for the thread that sends it test_monitor_login_keeps_its_own_message failed on the macos 3.11 leg of this branch, and reproduced locally under the full suite. It is not this branch's code -- the file is untouched here -- but it blocks the required check, and the race is in the test's own helper. The helper waited on an Event the background thread sets straight after calling on_pending_authorization, then read dialog.text(). The message crosses to the GUI thread on a queued signal, so that Event says nothing about whether the dialog has applied it; under load the cancel click lands first and the assertion sees the default 'Logging you in...'. Wait for the text itself, bounded so a message that never arrives still fails. Verified by injecting the delay the loaded runner produces (Event set before the message is sent): the old predicate fails with exactly the CI error, the new one passes. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- .../ui/dialogs/test_deadline_login_dialog.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/test/unit/deadline_client/ui/dialogs/test_deadline_login_dialog.py b/test/unit/deadline_client/ui/dialogs/test_deadline_login_dialog.py index 0616dc59d..887221ed0 100644 --- a/test/unit/deadline_client/ui/dialogs/test_deadline_login_dialog.py +++ b/test/unit/deadline_client/ui/dialogs/test_deadline_login_dialog.py @@ -137,11 +137,9 @@ def _message_for(qtbot, credentials_source) -> str: Runs a login that only fires `on_pending_authorization` with the given source, then blocks until cancelled, and returns the text the dialog settled on. """ - notified = threading.Event() def login(on_pending_authorization, on_cancellation_check, config=None): on_pending_authorization(credentials_source=credentials_source) - notified.set() while not on_cancellation_check(): pass return "unused-because-canceled" @@ -149,12 +147,16 @@ def login(on_pending_authorization, on_cancellation_check, config=None): with patch(_API_LOGIN, side_effect=login): dialog = DeadlineLoginDialog(parent=None, close_on_success=True) qtbot.addWidget(dialog) - - def click_cancel(): - # The message is set via a queued signal, so wait for the callback to - # have fired before tearing the dialog down. - if not notified.is_set(): - QTimer.singleShot(10, click_cancel) + initial_text = dialog.text() + + def click_cancel(attempts: int = 0): + # Wait for the text itself, not for the callback to have fired: the message + # crosses to this thread on a queued signal, so the background thread having + # called on_pending_authorization does not mean the dialog has applied it. + # Bounded, so a message that never arrives fails the assertion below rather + # than hanging the test. + if dialog.text() == initial_text and attempts < 200: + QTimer.singleShot(10, lambda: click_cancel(attempts + 1)) return dialog.button(QMessageBox.StandardButton.Cancel).click() From efc6150cc2f108ac281f64116a8c92ad8ba4a6b2 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 20 Aug 2026 10:21:01 -0700 Subject: [PATCH 24/28] fix: collapse '~' by path component, not by string prefix The picker widgets rewrote a chosen path to the '~' spelling when it startswith() the home directory, then sliced it by the home directory's length. With a home of /Users/bob, choosing /Users/bobby/projects/scene.ma displayed '~/ar/projects/scene.ma', and /Users/bob2/projects/x came back as '/projects/x', because join() drops the '~' when what follows is rooted. The config dialog writes that text straight into job_history_dir and job_bundle_default_directory, so the wrong directory is persisted; the widget then expands the '~' again and the two no longer agree. Use is_path_contained and relpath, in one helper rather than the two copies the two widgets carried. Covered in both path spaces, including the sibling prefixes that must come back untouched and the case-insensitivity Windows needs; the previous implementation fails 8 of the new cases. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- .../client/ui/widgets/path_widgets.py | 34 +++++-- .../ui/widgets/test_path_widgets.py | 88 +++++++++++++++++++ 2 files changed, 113 insertions(+), 9 deletions(-) create mode 100644 test/unit/deadline_client/ui/widgets/test_path_widgets.py diff --git a/src/deadline/client/ui/widgets/path_widgets.py b/src/deadline/client/ui/widgets/path_widgets.py index 7e5a702e0..579f43ee3 100644 --- a/src/deadline/client/ui/widgets/path_widgets.py +++ b/src/deadline/client/ui/widgets/path_widgets.py @@ -3,7 +3,7 @@ __all__ = ["DirectoryPickerWidget", "InputFilePickerWidget", "OutputFilePickerWidget"] import os -from typing import Optional +from typing import Any, Optional from qtpy.QtCore import Signal from qtpy.QtWidgets import ( # pylint: disable=import-error; type: ignore @@ -14,9 +14,31 @@ QWidget, ) +from ..._path_utils import is_path_contained from .._utils import block_signals, tr +def _collapse_user_dir(path: str, *, path_module: Any = None) -> str: + """Rewrite a path inside the user's home directory to the ``~`` spelling. + + Containment is by component, not by string prefix: with a home directory of + ``C:\\Users\\bob``, ``C:\\Users\\bobby\\scene.ma`` is not inside it. Slicing by the home + directory's length kept such a path and ate the first character of what followed, and + where the remainder then started with a separator ``join("~", ...)`` discarded the + ``~`` and returned an unrelated absolute path. The config dialog writes this text + straight into ``job_history_dir`` and ``job_bundle_default_directory``, so a wrong + answer here is persisted. + """ + path_module = path_module or os.path + home_dir = path_module.expanduser("~") + if not is_path_contained(path, home_dir, path_module=path_module): + return path + relative = path_module.relpath(path, home_dir) + if relative == path_module.curdir: + return "~" + return path_module.join("~", relative) + + class _FileWidget(QWidget): # Emitted when the file changes path_changed = Signal(str) @@ -62,10 +84,7 @@ def setText(self, filename): if filename: filename = os.path.normpath(filename) if self.collapse_user_dir: - # If it's in the home directory, change to the ~ syntax - home_dir = os.path.expanduser("~") - if filename.startswith(home_dir): - filename = os.path.join("~", filename[len(home_dir) + 1 :]) + filename = _collapse_user_dir(filename, path_module=os.path) with block_signals(self.filename_edit): self.filename_edit.setText(filename) @@ -236,10 +255,7 @@ def setText(self, directory): if directory: directory = os.path.normpath(directory) if self.collapse_user_dir: - # If it's in the home directory, collapse to the ~ syntax - home_dir = os.path.expanduser("~") - if directory.startswith(home_dir): - directory = os.path.join("~", directory[len(home_dir) + 1 :]) + directory = _collapse_user_dir(directory, path_module=os.path) with block_signals(self.directory_edit): self.directory_edit.setText(directory) diff --git a/test/unit/deadline_client/ui/widgets/test_path_widgets.py b/test/unit/deadline_client/ui/widgets/test_path_widgets.py new file mode 100644 index 000000000..8e6dfc73a --- /dev/null +++ b/test/unit/deadline_client/ui/widgets/test_path_widgets.py @@ -0,0 +1,88 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + +"""Tests for the path picker widgets' home-directory collapsing.""" + +import ntpath +import posixpath + +import pytest + +# importorskip, not a try/except: it binds the module when Qt is available and skips the +# file when it is not, where the except branch would leave the name unbound and every case +# would fail with NameError instead. +_path_widgets = pytest.importorskip("deadline.client.ui.widgets.path_widgets") +_collapse_user_dir = _path_widgets._collapse_user_dir + + +def _home_module(module, home): + """``module`` with ``expanduser`` pinned to ``home``, so the cases do not depend on + the home directory of whoever runs them.""" + + class _PinnedHome: + @staticmethod + def expanduser(path): + return home if path == "~" else path + + def __getattr__(self, name): + return getattr(module, name) + + return _PinnedHome() + + +class TestCollapseUserDir: + """ + The collapsed text is written straight into settings by the config dialog + (job_history_dir, job_bundle_default_directory), so a path that is not really inside + the home directory must come back untouched rather than rewritten. + """ + + @pytest.mark.parametrize( + "path, expected", + [ + (r"C:\Users\bob\projects\scene.ma", r"~\projects\scene.ma"), + (r"C:\Users\bob\scene.ma", r"~\scene.ma"), + (r"C:\Users\bob", "~"), + # Case-insensitive, like the filesystem. + (r"c:\users\BOB\projects\scene.ma", r"~\projects\scene.ma"), + # A sibling that merely shares a string prefix is not inside the home + # directory. Slicing by the home directory's length rewrote the first of + # these to '~\r\projects\scene.ma' and the second to '\projects\scene.ma', + # because join() drops the '~' when what follows is rooted. + (r"C:\Users\bobby\projects\scene.ma", r"C:\Users\bobby\projects\scene.ma"), + (r"C:\Users\bob2\projects\scene.ma", r"C:\Users\bob2\projects\scene.ma"), + # Elsewhere entirely, including another path space. + (r"D:\projects\scene.ma", r"D:\projects\scene.ma"), + (r"\\host\share\scene.ma", r"\\host\share\scene.ma"), + (r"C:\Users", r"C:\Users"), + ], + ) + def test_windows(self, path, expected): + assert ( + _collapse_user_dir(path, path_module=_home_module(ntpath, r"C:\Users\bob")) == expected + ) + + @pytest.mark.parametrize( + "path, expected", + [ + ("/home/bob/projects/scene.ma", "~/projects/scene.ma"), + ("/home/bob", "~"), + ("/home/bobby/projects/scene.ma", "/home/bobby/projects/scene.ma"), + ("/home/bob2/projects/scene.ma", "/home/bob2/projects/scene.ma"), + ("/mnt/share/scene.ma", "/mnt/share/scene.ma"), + ("/home", "/home"), + # POSIX paths are case-sensitive, so a case variant is a different directory. + ("/home/BOB/projects/scene.ma", "/home/BOB/projects/scene.ma"), + ], + ) + def test_posix(self, path, expected): + assert ( + _collapse_user_dir(path, path_module=_home_module(posixpath, "/home/bob")) == expected + ) + + def test_round_trips_through_expanduser(self): + """The collapsed text is expanded again when the widget is read back, so the two + have to agree -- that is what makes a wrong collapse persist as a wrong path.""" + module = _home_module(posixpath, "/home/bob") + for path in ("/home/bob/projects/scene.ma", "/home/bobby/projects/scene.ma"): + collapsed = _collapse_user_dir(path, path_module=module) + assert posixpath.expanduser(collapsed.replace("~", "/home/bob", 1)) == path From df07688b1b287523293776f4aad78d22c2aad235 Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Thu, 20 Aug 2026 14:59:43 -0700 Subject: [PATCH 25/28] ci: run the SMB suite on every pull request and before a release Two changes so this can actually block a merge: The paths filter is gone. GitHub reports a required check that was filtered out as pending rather than passed, so keeping the filter would block every merge it skipped. The cost is the Windows env install on each run, about two minutes. The release workflow now calls it, between the unit tests and PreRelease, using the tag input the workflow_call trigger already declared for exactly that and which nothing used -- so the tag being published was never validated against a real redirector. No secrets: the job creates a loopback share and needs none. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- .github/workflows/release_publish.yml | 8 ++++++++ .github/workflows/windows_smb_test.yml | 16 +++++++--------- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/.github/workflows/release_publish.yml b/.github/workflows/release_publish.yml index 34044b272..a2ff15475 100644 --- a/.github/workflows/release_publish.yml +++ b/.github/workflows/release_publish.yml @@ -100,6 +100,13 @@ jobs: with: tag: ${{ needs.TagRelease.outputs.tag }} + WindowsSMBPathTest: + name: Windows SMB Path Test + needs: [TagRelease, UnitTests] + uses: ./.github/workflows/windows_smb_test.yml + with: + tag: ${{ needs.TagRelease.outputs.tag }} + PreRelease: needs: - TagRelease @@ -109,6 +116,7 @@ jobs: - LinuxDCMIntegrationTest - WindowsDCMIntegrationTest - MacOSDCMIntegrationTest + - WindowsSMBPathTest uses: aws-deadline/.github/.github/workflows/reusable_prerelease.yml@mainline permissions: id-token: write diff --git a/.github/workflows/windows_smb_test.yml b/.github/workflows/windows_smb_test.yml index f79222d31..79645ca33 100644 --- a/.github/workflows/windows_smb_test.yml +++ b/.github/workflows/windows_smb_test.yml @@ -8,18 +8,16 @@ name: Windows SMB Path Test # can reach test/integ. This workflow owns these tests, and it has to gate pull requests # to be worth anything -- a wrong assertion here is invisible to every other check, so # post-merge-only means it lands green and fails on mainline. GitHub-hosted windows -# runners are already elevated, so `net share` works; the cost is the Windows env install -# on each run, which is why the path filter keeps it off unrelated pull requests. +# runners are already elevated, so `net share` works. +# +# It runs on every pull request, with no paths filter, because it is a required check: +# GitHub reports a filtered-out required check as pending forever, which would block every +# merge it skipped. The cost is the Windows env install (~2 min) per run. It also runs +# before a release publishes, since a tag nobody validated is how UNC handling would ship +# broken. on: workflow_dispatch: pull_request: - paths: - - 'src/deadline/client/_path_utils.py' - - 'src/deadline/client/_path_summary.py' - - 'src/deadline/client/api/_submit_job_bundle.py' - - 'src/deadline/client/job_bundle/**' - - 'test/integ/windows_smb/**' - - '.github/workflows/windows_smb_test.yml' push: branches: [mainline] workflow_call: From adba15ccdc4d792bff57996a32b17488fd2cf2aa Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Fri, 21 Aug 2026 08:51:55 -0700 Subject: [PATCH 26/28] test: cover five behaviors that survived mutation An audit reverted each change one at a time and found six mutants no test noticed. Five are closed here, each verified to fail the new test: - The call-time path_module resolution -- the most heavily commented decision in this branch -- was untested from every angle: every production call site passes the argument explicitly, and every test that omits it does so without patching os.path, so binding it at import again broke nothing. One test, patching os.path and omitting the argument at all six helpers, kills that. - parameters.py had no sibling-string-prefix case, so 'C:\bundle-secret' passed a naive startswith. Its two sibling guards, the symlink check and the archive guard, each have this case. - The picker widgets' calls to the collapse helper were deletable with the suite still green: the helper was tested, the wiring was not, and the wiring is what persists a wrong directory into settings. - The download summary's UNC case pinned only a cosmetic separator. Two shares of one host is the pair commonpath answers with ValueError, which nothing caught, so summarizing such a download aborted the command. - The JSON-protocol branch of the OS-mismatch remap validated nothing under test; it is where a root arrives from a machine rather than a person. The sixth is not a defect: passing path_module=os.path explicitly at a call site is unobservable now that the default resolves at call time, so it stays a convention that documents intent. Every line this branch adds to src is now covered on this interpreter. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- .github/workflows/windows_smb_test.yml | 12 ++-- test/unit/deadline_client/cli/test_cli_job.py | 56 +++++++++++++++++++ .../job_bundle/test_job_parameters.py | 15 +++++ test/unit/deadline_client/test_path_utils.py | 21 +++++++ .../ui/widgets/test_path_widgets.py | 52 +++++++++++++++++ 5 files changed, 151 insertions(+), 5 deletions(-) diff --git a/.github/workflows/windows_smb_test.yml b/.github/workflows/windows_smb_test.yml index 79645ca33..947f179cd 100644 --- a/.github/workflows/windows_smb_test.yml +++ b/.github/workflows/windows_smb_test.yml @@ -10,11 +10,13 @@ name: Windows SMB Path Test # post-merge-only means it lands green and fails on mainline. GitHub-hosted windows # runners are already elevated, so `net share` works. # -# It runs on every pull request, with no paths filter, because it is a required check: -# GitHub reports a filtered-out required check as pending forever, which would block every -# merge it skipped. The cost is the Windows env install (~2 min) per run. It also runs -# before a release publishes, since a tag nobody validated is how UNC handling would ship -# broken. +# It runs on every pull request with no paths filter, so the check always reports and can +# be made a required one: GitHub leaves a filtered-out required check pending forever, so +# the filter would have to go first. It is not in mainline's required checks yet -- adding +# it there before this workflow reaches mainline would block every open pull request, since +# their merge refs would not contain it. The cost is the Windows env install, about two +# minutes per run. It also runs before a release publishes, since a tag nobody validated +# against a real redirector is how broken UNC handling would ship. on: workflow_dispatch: pull_request: diff --git a/test/unit/deadline_client/cli/test_cli_job.py b/test/unit/deadline_client/cli/test_cli_job.py index 98b6c8a60..de6422eb9 100644 --- a/test/unit/deadline_client/cli/test_cli_job.py +++ b/test/unit/deadline_client/cli/test_cli_job.py @@ -933,6 +933,13 @@ def test_get_summary_of_files_to_download_message_windows( {r"\\host\share": ["only.png"]}, "\nSummary of files to download:\n \\\\host\\share\\only.png (1 file)\n", ), + # Two shares of one host share only the host. This is the pair os.path.commonpath + # answers with ValueError("Paths don't have the same drive"), which nothing here + # caught -- so summarizing a download spanning two shares aborted the command. + ( + {r"\\host": ["s1/a.png", "s2/b.png"]}, + "\nSummary of files to download:\n \\\\host (2 files)\n", + ), ], ) def test_get_summary_of_files_to_download_message_unc_paths( @@ -1600,6 +1607,55 @@ def test_posix_paths(self): job_group._assert_valid_path("relative/out", path_module=posixpath) +class TestPromptForOsMismatchRoots: + """The JSON-protocol branch of the OS-mismatch remap. + + This is where a root arrives from a machine rather than a person -- the GUI and any + automation driving the CLI -- so it is the branch that most needs the download root it + is handed to be validated. Only the interactive branch's sibling was covered. + """ + + @staticmethod + def _remap(new_root, host_format="posix", root_format="windows"): + downloader = MagicMock() + downloader.get_paths_by_root.return_value = {new_root: ["a.png"]} + root = "/renders" if host_format == "posix" else r"C:\renders" + json_line = json.dumps({"messageType": "pathconfirm", "value": [new_root]}) + with ( + patch.object( + job_group.PathFormat, "get_host_path_format_string", return_value=host_format + ), + patch.object(job_group.click, "prompt", return_value=json_line), + patch.object(job_group.click, "echo"), + ): + result = job_group._prompt_for_os_mismatch_roots( + downloader, + {root: ["a.png"]}, + {root: root_format}, + is_json_format=True, + ) + return downloader, result + + def test_absolute_root_is_accepted_and_set(self): + downloader, result = self._remap("/mnt/share/renders") + downloader.set_root_path.assert_called_once_with("/renders", "/mnt/share/renders") + assert result == {"/mnt/share/renders": ["a.png"]} + + @pytest.mark.parametrize("new_root", ["relative/renders", "renders"]) + def test_relative_root_is_rejected(self, new_root): + """A relative root would resolve against the CLI's working directory, which the + caller on the other end of the protocol does not control.""" + with pytest.raises(ValueError, match="is not an absolute path"): + self._remap(new_root) + + def test_a_root_with_no_format_is_an_error(self): + downloader = MagicMock() + with pytest.raises(DeadlineOperationError, match="No root path format found"): + job_group._prompt_for_os_mismatch_roots( + downloader, {"/renders": ["a.png"]}, {}, is_json_format=True + ) + + class TestJsonLineHelpers: """Tests for JSON line helper functions.""" diff --git a/test/unit/deadline_client/job_bundle/test_job_parameters.py b/test/unit/deadline_client/job_bundle/test_job_parameters.py index fbca509ab..1174fe94b 100644 --- a/test/unit/deadline_client/job_bundle/test_job_parameters.py +++ b/test/unit/deadline_client/job_bundle/test_job_parameters.py @@ -787,6 +787,21 @@ def test_default_resolving_from_drive_bundle_onto_unc_share_is_rejected(self): ): parameters.read_job_bundle_parameters(bundle_dir) + def test_default_resolving_into_sibling_prefix_directory_is_rejected(self): + """A string prefix is not a directory prefix: 'C:\\bundle-secret' is outside + 'C:\\bundle'. Its two sibling guards -- the symlink check and the archive guard -- + each have this case; without it a naive startswith passes here.""" + bundle_dir = r"C:\bundle" + with self._simulated_windows_bundle( + bundle_dir, + {r"C:\bundle\output": r"C:\bundle-secret\output"}, + ): + with pytest.raises( + exceptions.DeadlineOperationError, + match="specifies files outside of Job Bundle directory", + ): + parameters.read_job_bundle_parameters(bundle_dir) + @pytest.mark.parametrize( "default", [ diff --git a/test/unit/deadline_client/test_path_utils.py b/test/unit/deadline_client/test_path_utils.py index b759016d3..5e539d63a 100644 --- a/test/unit/deadline_client/test_path_utils.py +++ b/test/unit/deadline_client/test_path_utils.py @@ -10,6 +10,7 @@ import itertools import ntpath +import os import posixpath import sys from pathlib import PurePosixPath, PureWindowsPath @@ -17,7 +18,10 @@ import pytest +from unittest.mock import patch + from ._legacy_ntpath import PreThreeElevenNtpath +from deadline.client._path_summary import common_ancestor from deadline.client._path_utils import ( _splitroot, is_absolute_path, @@ -499,6 +503,23 @@ def __getattr__(self, name): assert is_absolute_path(r"C:\a", path_module=wrong) is True +def test_path_module_is_resolved_at_call_time(): + """A caller that omits ``path_module`` must still follow the running platform. + + A signature default would bind ``os.path`` when this module is imported, so patching + it here would have no effect -- which is how the archive extraction guard came to have + no Windows coverage at all. Every assertion below omits the argument, so it fails if + the resolution moves back into a signature. + """ + with patch.object(os, "path", ntpath): + assert path_components(r"\\host\share") == ["\\\\", "host", "share"] + assert is_absolute_path(r"\\host\share") is True + assert normalized_path("\\\\host\\") == r"\\host" + assert is_path_contained(r"\\host\share\f", r"\\host") is True + assert is_bare_unc_anchor("\\\\") is True + assert common_ancestor([r"\\host\s1\a", r"\\host\s2\b"]) == r"\\host" + + def test_containment_of_degenerate_paths(): """The empty string normalizes to the current directory, so it is its own space. diff --git a/test/unit/deadline_client/ui/widgets/test_path_widgets.py b/test/unit/deadline_client/ui/widgets/test_path_widgets.py index 8e6dfc73a..1a9c72398 100644 --- a/test/unit/deadline_client/ui/widgets/test_path_widgets.py +++ b/test/unit/deadline_client/ui/widgets/test_path_widgets.py @@ -3,6 +3,7 @@ """Tests for the path picker widgets' home-directory collapsing.""" import ntpath +import os import posixpath import pytest @@ -12,6 +13,8 @@ # would fail with NameError instead. _path_widgets = pytest.importorskip("deadline.client.ui.widgets.path_widgets") _collapse_user_dir = _path_widgets._collapse_user_dir +DirectoryPickerWidget = _path_widgets.DirectoryPickerWidget +InputFilePickerWidget = _path_widgets.InputFilePickerWidget def _home_module(module, home): @@ -86,3 +89,52 @@ def test_round_trips_through_expanduser(self): for path in ("/home/bob/projects/scene.ma", "/home/bobby/projects/scene.ma"): collapsed = _collapse_user_dir(path, path_module=module) assert posixpath.expanduser(collapsed.replace("~", "/home/bob", 1)) == path + + +class TestWidgetsCollapseOnSetText: + """ + The helper being right is not enough: the widgets have to call it. Both of these pass + with the calls deleted if only the helper is tested, and the collapsed text is what the + config dialog persists. + """ + + def test_directory_picker_collapses_a_path_in_the_home_directory(self, qtbot): + home = os.path.expanduser("~") + widget = DirectoryPickerWidget( + initial_directory="", directory_label="Test Dir", collapse_user_dir=True + ) + qtbot.addWidget(widget) + + widget.setText(os.path.join(home, "projects")) + assert widget.text() == os.path.join("~", "projects") + + # A sibling that merely shares the prefix must survive untouched. + widget.setText(os.path.join(home + "2", "projects")) + assert widget.text() == os.path.join(home + "2", "projects") + + def test_file_picker_collapses_a_path_in_the_home_directory(self, qtbot): + home = os.path.expanduser("~") + widget = InputFilePickerWidget( + initial_filename="", + file_label="Test File", + filter="All Files (*)", + selected_filter="All Files (*)", + collapse_user_dir=True, + ) + qtbot.addWidget(widget) + + widget.setText(os.path.join(home, "scene.ma")) + assert widget.text() == os.path.join("~", "scene.ma") + + widget.setText(os.path.join(home + "2", "scene.ma")) + assert widget.text() == os.path.join(home + "2", "scene.ma") + + def test_collapsing_is_opt_in(self, qtbot): + """The default leaves the path alone, which is what the pickers that store an + absolute path depend on.""" + home = os.path.expanduser("~") + widget = DirectoryPickerWidget(initial_directory="", directory_label="Test Dir") + qtbot.addWidget(widget) + + widget.setText(os.path.join(home, "projects")) + assert widget.text() == os.path.join(home, "projects") From fa6e95a90cdb76738c98b4945efc9c1fc7ad05ea Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Wed, 26 Aug 2026 10:12:02 -0700 Subject: [PATCH 27/28] test: spell the accepted download root for the host The JSON-protocol remap validates the root it is handed against os.path, the module of the machine that will do the downloading. The new test supplied '/mnt/share/renders' regardless of platform, which is rooted but driveless on Windows -- it resolves against whichever drive the process happens to be on, so it is not absolute there and the validator rejected it. The suite was green on Linux and macOS and failed on all six Windows jobs. Pick the spelling from os.name so the case asserts what it means -- an absolute root is accepted and set -- on every host. TestAssertValidPath already covers both spellings everywhere by injecting the path module, so the platform-specific verdicts stay pinned off Windows too. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- test/unit/deadline_client/cli/test_cli_job.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/test/unit/deadline_client/cli/test_cli_job.py b/test/unit/deadline_client/cli/test_cli_job.py index de6422eb9..71e732882 100644 --- a/test/unit/deadline_client/cli/test_cli_job.py +++ b/test/unit/deadline_client/cli/test_cli_job.py @@ -1615,6 +1615,12 @@ class TestPromptForOsMismatchRoots: is handed to be validated. Only the interactive branch's sibling was covered. """ + # The root is validated against the host's own path module, so it has to be spelled for + # the platform the test runs on: '/mnt/share/renders' is rooted but driveless on + # Windows, which resolves against the current drive and so is not absolute there. + # TestAssertValidPath covers both spellings on every platform by injecting the module. + HOST_ABSOLUTE_ROOT = r"C:\mnt\share\renders" if os.name == "nt" else "/mnt/share/renders" + @staticmethod def _remap(new_root, host_format="posix", root_format="windows"): downloader = MagicMock() @@ -1637,9 +1643,9 @@ def _remap(new_root, host_format="posix", root_format="windows"): return downloader, result def test_absolute_root_is_accepted_and_set(self): - downloader, result = self._remap("/mnt/share/renders") - downloader.set_root_path.assert_called_once_with("/renders", "/mnt/share/renders") - assert result == {"/mnt/share/renders": ["a.png"]} + downloader, result = self._remap(self.HOST_ABSOLUTE_ROOT) + downloader.set_root_path.assert_called_once_with("/renders", self.HOST_ABSOLUTE_ROOT) + assert result == {self.HOST_ABSOLUTE_ROOT: ["a.png"]} @pytest.mark.parametrize("new_root", ["relative/renders", "renders"]) def test_relative_root_is_rejected(self, new_root): From a2bd9d72e3884c294315f3c952af511671303cef Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Wed, 26 Aug 2026 11:12:28 -0700 Subject: [PATCH 28/28] chore: narrow the click.Path prompt results to str click 8.5.0 propagates a ParamType's declared result type through click.prompt, so the two prompts typed with click.Path now hand back 'str | bytes | os.PathLike[str]' -- the union click.Path declares to cover its path_type option. os.path.expanduser widens that to 'str | bytes' and Path() rejects bytes outright, so mypy fails on both call sites. With path_type unset, click.Path.coerce_path_result returns the prompt string unchanged, so str is the only type either prompt can produce; cast says so. No runtime behavior changes. This is not specific to this branch -- mainline fails the same two lines under click 8.5.0, which is unpinned and resolves fresh whenever the Hatch environment cache misses. That is why every job in the matrix went red at once, after the same commit linted clean on click 8.4.2. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- src/deadline/client/cli/_groups/job_group.py | 27 ++++++++++++++------ 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/src/deadline/client/cli/_groups/job_group.py b/src/deadline/client/cli/_groups/job_group.py index da2d74fcd..02de62172 100644 --- a/src/deadline/client/cli/_groups/job_group.py +++ b/src/deadline/client/cli/_groups/job_group.py @@ -12,7 +12,7 @@ import os import re import sys -from typing import Callable, Optional, Union +from typing import Callable, Optional, Union, cast import datetime from typing import Any import textwrap @@ -543,9 +543,16 @@ def _prompt_for_os_mismatch_roots( if PathFormat.get_host_path_format_string() != root_path_format: click.echo(_get_mismatch_os_root_warning(asset_root, root_path_format, is_json_format)) if not is_json_format: - new_root = click.prompt( - "> Please enter a new root path", - type=click.Path(exists=False), + # click.Path annotates its result 'str | bytes | os.PathLike[str]' to cover + # its path_type option; with path_type unset it returns the prompt string + # unchanged. Narrowing here keeps the union out of expanduser and Path, + # neither of which accepts bytes. + new_root = cast( + str, + click.prompt( + "> Please enter a new root path", + type=click.Path(exists=False), + ), ) else: json_string = click.prompt("", prompt_suffix="", type=str) @@ -585,10 +592,14 @@ def _prompt_to_confirm_roots( return None elif user_choice != "y": index_to_change = int(user_choice) - new_root = click.prompt( - "> Please enter the new root directory path, or press Enter to keep it unchanged", - type=click.Path(exists=False), - default=asset_roots[index_to_change], + # Narrowed for the same reason as the sibling prompt above. + new_root = cast( + str, + click.prompt( + "> Please enter the new root directory path, or press Enter to keep it unchanged", + type=click.Path(exists=False), + default=asset_roots[index_to_change], + ), ) downloader.set_root_path(asset_roots[index_to_change], str(Path(new_root))) paths_by_root = downloader.get_paths_by_root()