fix: recognize host-level UNC roots in path containment checks

.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'

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"]
 

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])

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.

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"