From 2e85901876be4a715c00d3b0c1e2e485fff03cfc Mon Sep 17 00:00:00 2001 From: spud <92900806+jamespud@users.noreply.github.com> Date: Sun, 6 Sep 2026 08:50:05 +0800 Subject: [PATCH] fix(lark): enforce private ACLs on Windows credential tree (#5141) * fix(lark): enforce private ACLs on Windows credential tree On Windows, posix chmod(0o700/0o600) does not map to NTFS ACLs, so the secret-bearing Lark CLI credential tree was not actually owner-restricted and existing trees were not repaired. Branch the permission application by platform: - POSIX: directories 0o700, files 0o600 (behavior unchanged). - Windows: disable inherited ACLs, grant the Gateway process user Full Control (resolved via its SID from whoami /user /fo csv /nh so it is locale-independent), and remove broad non-administrative grants (Everyone, Authenticated Users, Users). Fail closed on identity or icacls failures so a tree is never left accessible silently. Existing-tree handling is covered by asserting every entry in the tree is repaired, and the Windows command contract is covered by mocked tests run in CI. * fix(lark): harden Windows credential tree against TOCTOU and hard-link races This replaces the path-based Windows hardening (lstat -> SetFileSecurityW(path) -> iterdir) with a handle-relative walker, so validation, the ACL update, and traversal are bound to the opened object rather than a re-resolved pathname. Every credential object is opened no-follow; children are enumerated with GetFileInformationByHandleEx(FileFullDirectoryInfo) and opened/created relative to an already-open parent handle (NtOpenFile/NtCreateFile with OBJECT_ATTRIBUTES.RootDirectory), so a pathname swap cannot redirect the walk. Credential directories are opened exclusively (share=0): SetSecurityInfo therefore does not propagate the final owner-only OI|CI DACL into as-yet-unvalidated children, and the namespace is locked for the duration of the walk (concurrent child rename/replacement and hard-link insertion fail with sharing violations). Any file with nNumberOfLinks != 1 is rejected before its security descriptor is touched, so an NTFS hard link to an external file cannot change that file owner/DACL. POSIX keeps the lstat-before-descent walk. Tests: native regressions for exclusive no-propagation, late hard-link insertion being blocked, mid-walk junction swap being blocked, static hard-link rejection, and both real NTFS junction rejections. Mock seams updated for the handle-relative API, and Windows portability fixes make the suite green on Windows except the known #5116 sandbox-runtime executable-bit failures. * test(lark): keep the credential-tree symlink assertion portable The credential-tree symlink rejection is a ValueError; POSIX reports a symlink while the Windows handle-relative walker reports a reparse point. Use a platform-dependent regex so the test passes on Linux/macOS and Windows. * fix(lark): close remaining credential-tree hardening gaps Review follow-up for the handle-relative credential-tree walker: - Stage the transaction snapshot under the owner-only root, copying only config/ and data/. - Serialize ensure() per-user across threads and processes with a dedicated lock. - Make the walker iterative so deep trees cannot hit the recursion limit. - Re-reject a symlinked POSIX root before mkdir; drop the over-strict ancestor-chain check. - Soften the SetSecurityInfo failure claim; add regressions for each and carry os.SEEK_END in the os stub. * fix(lark): anchor hardening lock under trusted base and keep POSIX untouched Follow-up refinements to the credential-tree hardening: - The per-user hardening lock file now lives directly under the trusted base_dir instead of the unverified per-user chain, so it is never written through an ancestor that has not yet passed reparse validation. - ensure() takes the hardening lock only on the Windows branch; POSIX keeps the original contract, so no new lock-file side effect. - Strengthen the ancestor-junction regression (lock not written to the external target) and fix two test docstrings to match the parent-first order and the no-prior-broadening failure claim. * fix(lark): anchor credential-operation lock under trusted base on Windows The per-user credential lock (_lark_credential_lock) created its advisory lock file under the unverified per-user chain (users//integrations/.lark-cli.credentials.lock) before ensure() validated the ancestor chain. On Windows it is now anchored directly under the trusted paths.base_dir (mirroring the hardening lock), so a junction at integrations can no longer cause the credential lock to be written into an external target before reparse validation. POSIX keeps the original location unchanged. Tests: - Public-flow regression (start_lark_config -> credential lock -> ensure) uses an empty sentinel lock file to prove the old credential-lock path is never opened/written. - CLI-write re-harden tests restore the POSIX outcome assertion (file tightened to 0600). --- .../harness/deerflow/integrations/lark_cli.py | 948 +++++++++++++- backend/tests/test_lark_cli_integration.py | 1097 +++++++++++++++-- 2 files changed, 1936 insertions(+), 109 deletions(-) diff --git a/backend/packages/harness/deerflow/integrations/lark_cli.py b/backend/packages/harness/deerflow/integrations/lark_cli.py index b5e67d950..522f55de6 100644 --- a/backend/packages/harness/deerflow/integrations/lark_cli.py +++ b/backend/packages/harness/deerflow/integrations/lark_cli.py @@ -43,6 +43,8 @@ diverge. from __future__ import annotations +import csv +import ctypes import hashlib import io import json @@ -51,6 +53,8 @@ import os import posixpath import re import shutil +import stat +import struct import subprocess import tarfile import tempfile @@ -59,7 +63,9 @@ import time import urllib.parse import urllib.request import zipfile +from collections.abc import Callable, Iterator from contextlib import contextmanager +from ctypes import wintypes from dataclasses import dataclass from datetime import UTC, datetime from pathlib import Path, PurePosixPath @@ -170,6 +176,8 @@ _LARK_INSTALL_THREAD_LOCK = threading.Lock() _LARK_RUNTIME_INSTALL_THREAD_LOCK = threading.Lock() _LARK_CREDENTIAL_LOCKS_GUARD = threading.Lock() _LARK_CREDENTIAL_LOCKS: WeakValueDictionary[str, threading.Lock] = WeakValueDictionary() +_LARK_HARDENING_LOCKS_GUARD = threading.Lock() +_LARK_HARDENING_LOCKS: WeakValueDictionary[str, threading.RLock] = WeakValueDictionary() @dataclass(frozen=True) @@ -298,31 +306,847 @@ def _lark_cli_credential_root(user_id: str) -> Path: def ensure_lark_cli_credential_tree(user_id: str, *, paths: Paths | None = None) -> None: - """Make the user's secret-bearing Lark CLI tree owner-only. + """Harden the secret-bearing credential tree to owner-only. - The CLI writes plaintext app secrets and OAuth tokens beneath this tree. - Reject links before changing modes so a compromised tree cannot redirect a - chmod or subsequent CLI write outside the user's integration directory. + Windows uses a handle-relative walker (:func:`_ensure_and_harden_windows_credential_tree`): + every descendant is opened/created relative to an already-open parent handle, so a pathname + swap cannot redirect validation, the ACL update, or traversal for the duration of that + hardening walk. This guarantee does not extend to a later pathname-based reopen by a + credential consumer after ``ensure`` returns. POSIX keeps the ``lstat()``-before-descent + walk and does not take the hardening lock (its share mode has no cross-process race). """ paths = paths or get_paths() root = paths.user_dir(user_id) / "integrations" / INTEGRATION_ID - if root.is_symlink(): - raise ValueError(f"Lark CLI credential path must not be a symlink: {root}") + if os.name == "nt": + with _lark_hardening_lock(user_id, paths): + _ensure_and_harden_windows_credential_tree(paths, root) + return + + _reject_credential_reparse(root) root.mkdir(parents=True, exist_ok=True, mode=0o700) root.chmod(0o700) for required in (root / "config", root / "config" / "locks", root / "data"): - if required.is_symlink(): - raise ValueError(f"Lark CLI credential path must not be a symlink: {required}") + _reject_credential_reparse(required) required.mkdir(parents=True, exist_ok=True, mode=0o700) - for path in root.rglob("*"): - if path.is_symlink(): - raise ValueError(f"Lark CLI credential path must not be a symlink: {path}") - if path.is_dir(): - path.chmod(0o700) - elif path.is_file(): - path.chmod(0o600) - else: - raise ValueError(f"Unsupported file type in Lark CLI credential tree: {path}") + _harden_posix_credential_tree(root) + + +def _ensure_and_harden_windows_credential_tree(paths: Paths, root: Path) -> None: + """Create + harden the credential tree with handle-relative traversal (Windows). + + The trusted base is opened by name; every descendant is then opened/created + *relative* to an already-open parent handle (``RootDirectory``). The walker never + re-resolves a pathname, so a concurrent rename/replace of an ancestor or directory + cannot redirect validation, the ACL update, or traversal to a swapped object. + """ + owner_sid = _resolve_current_user_sid() + pinned: list[_WindowsTreeHandle] = [] + try: + chain = _credential_chain_paths(paths.base_dir, root) + # The trusted base is the storage root; create it if absent, then open it + # by name (it is deliberately allowed to be a reparse point). + chain[0].mkdir(parents=True, exist_ok=True) + base_handle = _open_windows_pinned(chain[0], access=_WINDOWS_PIN_ACCESS, reject_reparse=False) + pinned.append(base_handle) + + # Pin the ancestor chain from the base down to the credential root. + parent = base_handle + for component in chain[1:]: + is_root = component == chain[-1] + access = _WINDOWS_HARDEN_ACCESS if is_root else _WINDOWS_PIN_ACCESS + share = _WINDOWS_EXCLUSIVE_SHARE if is_root else _WINDOWS_NORMAL_SHARE + parent = _open_or_create_dir_relative(parent, component.name, full_path=component, access=access, share=share) + pinned.append(parent) + root_handle = parent + + # Handle-relative hardening walk. + _walk_and_harden_windows_handle(root, root_handle, owner_sid, root) + finally: + for handle in reversed(pinned): + handle.close() + + +@contextmanager +def _private_lark_temp_dir(*, prefix: str, dir: Path | None = None): + """Yield an empty, owner-only temp directory for secret-bearing work. + + The root is hardened (owner-only) *before* any child or credential is + created, so ``config`` / ``data`` / snapshot directories and the secrets + written or copied into them inherit the owner-only ACL boundary on Windows + and the ``0700`` mode on POSIX. + """ + with tempfile.TemporaryDirectory(prefix=prefix, dir=str(dir) if dir else None) as temp_dir: + root = Path(temp_dir) + # No credential has been written yet — establish the boundary first. + _establish_private_directory_boundary(root) + yield root + + +def _harden_posix_credential_tree(root: Path) -> None: + def _chmod(path: Path, kind: str) -> None: + path.chmod(0o700 if kind == "dir" else 0o600) + + _walk_and_harden(root, _chmod) + + +def _walk_and_harden(root: Path, apply_: Callable[[Path, str], None]) -> None: + """Lstat-before-descent walk over *root* and each descendant (POSIX). + + Every path is validated with ``lstat()`` (symlink / reparse-point / unsupported + type rejected) *before* applying permissions and descending, and we only + ``iterdir()`` a path after it is confirmed to be a real directory. + + This is a best-effort static check, not a race-proof one: it does not pin the + object, so a concurrent local principal could still swap a checked directory + for a symlink between the ``lstat`` and a later permission apply / ``iterdir``. + Windows intentionally uses :func:`_walk_and_harden_windows_handle`, which keeps + validation, the ACL update, and traversal bound to the opened object (all descendant + opens are relative to an already-open parent handle), so it does not depend on the + pathname remaining stable. + """ + pending: list[Path] = [root] + while pending: + path = pending.pop() + kind = _credential_tree_path_kind(path) + apply_(path, kind) + if kind == "dir": + pending.extend(path.iterdir()) + + +def _credential_tree_path_kind(path: Path) -> str: + """Return ``"dir"`` or ``"file"`` for a safe real entry, else raise. + + Rejects any symlink and any Windows reparse point *before* the caller may + descend, so a junction cannot redirect traversal outside the tree. + """ + info = path.lstat() + if stat.S_ISLNK(info.st_mode): + raise ValueError(f"Lark CLI credential path must not be a symlink: {path}") + if os.name == "nt" and (getattr(info, "st_file_attributes", 0) & stat.FILE_ATTRIBUTE_REPARSE_POINT): + raise ValueError(f"Lark CLI credential path must not be a reparse point: {path}") + if stat.S_ISDIR(info.st_mode): + return "dir" + if stat.S_ISREG(info.st_mode): + return "file" + raise ValueError(f"Unsupported file type in Lark CLI credential tree: {path}") + + +def _reject_reparse_stat(path: Path, info: os.stat_result) -> None: + """Reject a symlink or Windows reparse point *path* with stat *info*.""" + if stat.S_ISLNK(info.st_mode): + raise ValueError(f"Lark CLI credential path must not be a symlink: {path}") + if os.name == "nt" and (getattr(info, "st_file_attributes", 0) & stat.FILE_ATTRIBUTE_REPARSE_POINT): + raise ValueError(f"Lark CLI credential path must not be a reparse point: {path}") + + +def _reject_credential_reparse(path: Path) -> None: + """Reject an already-existing symlink / reparse point before ``mkdir``. + + ``mkdir(exist_ok=True)`` would accept an existing junction, so a reparse + root or required directory must be rejected up front rather than after it is + used. Non-existent paths are fine to create. + """ + try: + info = path.lstat() + except FileNotFoundError: + return + _reject_reparse_stat(path, info) + + +def _resolve_current_user_sid() -> str: + """Return the current process user's Windows SID, e.g. ``S-1-5-21-...``. + + ``whoami /user /fo csv /nh`` prints ``"\\",""`` — the SID + is the *second* CSV field — so we parse it with ``csv.reader`` rather than + guessing a field position. SIDs are locale/display-name independent and + are the single principal granted in the Windows allowlist below. + """ + result = subprocess.run( + ["whoami", "/user", "/fo", "csv", "/nh"], + capture_output=True, + text=True, + check=False, + ) + if result.returncode != 0: + raise RuntimeError(f"failed to resolve current Windows user SID: {result.stderr.strip() or result.stdout.strip()}") + try: + fields = next(csv.reader([result.stdout.strip()])) + except (csv.Error, StopIteration) as exc: + raise RuntimeError(f"unexpected whoami /user output: {result.stdout.strip()!r}") from exc + if len(fields) < 2 or not fields[1].startswith("S-"): + raise RuntimeError(f"unexpected whoami /user output: {result.stdout.strip()!r}") + return fields[1] + + +def _windows_private_sddl(owner_sid: str, *, inheritable_full: bool) -> str: + """SDDL for a protected owner-only descriptor that also transfers ownership. + + ``O:D:P(A;...;FA;;;)`` sets the security descriptor's + owner to *owner_sid* and installs a protected DACL granting only *owner_sid* + Full Access. The ``O:`` prefix is what lets the successor (attacker) lose + implicit ``WRITE_DAC``. + """ + ace = "(A;OICI;FA;;;" if inheritable_full else "(A;;FA;;;" + return f"O:{owner_sid}D:P{ace}{owner_sid})" + + +def _windows_private_security_information() -> int: + """Security-information flags: OWNER | DACL | PROTECTED_DACL.""" + return 0x00000001 | 0x00000004 | 0x80000000 + + +# --- Handle-relative Windows credential-tree walker ------------------------- +# +# The secret-bearing Lark CLI tree is hardened with *handle-relative* primitives. +# A ``no-FILE_SHARE_DELETE`` handle is not a rename barrier on its own (``os.rename`` +# of a directory is authorized by the object's DELETE right or the parent's +# DELETE_CHILD, and does not require re-opening the directory for DELETE). So the +# walker never re-resolves a pathname at all: it opens the tree once, then +# enumerates and opens/creates every child *relative* to an already-open parent +# handle (``RootDirectory``), and inspects/hardens from the handle. A pathname +# swap can therefore not redirect the walker's validation, ACL update, or traversal. + +_FILE_LIST_DIRECTORY = 0x0001 # == FILE_READ_DATA when the target is a file +_FILE_READ_ATTRIBUTES = 0x0080 +_READ_CONTROL = 0x00020000 +_WRITE_DAC = 0x00040000 +_WRITE_OWNER = 0x00080000 +_SYNCHRONIZE = 0x00100000 +_FILE_SHARE_READ = 0x00000001 +_FILE_SHARE_WRITE = 0x00000002 +_WINDOWS_NORMAL_SHARE = _FILE_SHARE_READ | _FILE_SHARE_WRITE +# An exclusive (share=0) directory handle makes ``SetSecurityInfo`` skip automatic +# propagation of an inheritable ACE into existing children, so we can apply the final +# owner-only OI|CI DACL to a directory *before* walking its (as-yet-unvalidated) children. +_WINDOWS_EXCLUSIVE_SHARE = 0 +_OPEN_EXISTING = 3 +_FILE_FLAG_BACKUP_SEMANTICS = 0x02000000 +_FILE_FLAG_OPEN_REPARSE_POINT = 0x00200000 +_INVALID_HANDLE_VALUE = ctypes.c_void_p(-1).value + +# Ancestors are carried only to retain the opened object's identity so children can +# be opened *relative* to them via ``RootDirectory``; they are never hardened, so a +# read-only handle (attributes + synchronize) is enough. +_WINDOWS_PIN_ACCESS = _FILE_READ_ATTRIBUTES | _SYNCHRONIZE +# The credential root and its descendants get ``SetSecurityInfo`` on the open +# handle, so they additionally need read-control plus write-DAC / write-owner. +# Native Windows requires READ_CONTROL for this particular SetSecurityInfo path +# in addition to WRITE_DAC / WRITE_OWNER — verified empirically: omitting it +# yields ERROR_ACCESS_DENIED (WinError 5). ``FILE_LIST_DIRECTORY`` is needed on +# directory handles so ``GetFileInformationByHandleEx`` can enumerate them. We +# deliberately do *not* request ``FILE_READ_EA``. +_WINDOWS_HARDEN_ACCESS = _WINDOWS_PIN_ACCESS | _READ_CONTROL | _WRITE_DAC | _WRITE_OWNER | _FILE_LIST_DIRECTORY + + +class _BY_HANDLE_FILE_INFORMATION(ctypes.Structure): + _fields_ = [ + ("dwFileAttributes", wintypes.DWORD), + ("ftCreationTime", wintypes.FILETIME), + ("ftLastAccessTime", wintypes.FILETIME), + ("ftLastWriteTime", wintypes.FILETIME), + ("dwVolumeSerialNumber", wintypes.DWORD), + ("nFileSizeHigh", wintypes.DWORD), + ("nFileSizeLow", wintypes.DWORD), + ("nNumberOfLinks", wintypes.DWORD), + ("nFileIndexHigh", wintypes.DWORD), + ("nFileIndexLow", wintypes.DWORD), + ] + + +class _WindowsFileInfo: + """Attribute snapshot taken from an already-open handle.""" + + def __init__(self, attributes: int, link_count: int) -> None: + self.attributes = attributes + self.link_count = link_count + + @property + def reparse(self) -> bool: + return bool(self.attributes & stat.FILE_ATTRIBUTE_REPARSE_POINT) + + @property + def is_dir(self) -> bool: + return bool(self.attributes & stat.FILE_ATTRIBUTE_DIRECTORY) + + +def _create_windows_handle_no_follow( + path: Path, + *, + access: int, + share: int = _WINDOWS_NORMAL_SHARE, +) -> int: + """Open *path* by name, never following a reparse point. + + ``FILE_FLAG_OPEN_REPARSE_POINT`` opens the reparse point itself rather than + following it; ``FILE_FLAG_BACKUP_SEMANTICS`` lets a directory be opened as a + handle. This is used only to open the trusted base; every descendant is opened + relative to an already-open parent handle, so the walker does not depend on the + pathname remaining stable. + """ + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + kernel32.CreateFileW.restype = wintypes.HANDLE + kernel32.CreateFileW.argtypes = [ + wintypes.LPCWSTR, + wintypes.DWORD, + wintypes.DWORD, + ctypes.c_void_p, + wintypes.DWORD, + wintypes.DWORD, + wintypes.HANDLE, + ] + handle = kernel32.CreateFileW( + str(path), + access, + share, + None, + _OPEN_EXISTING, + _FILE_FLAG_BACKUP_SEMANTICS | _FILE_FLAG_OPEN_REPARSE_POINT, + None, + ) + if handle is None or handle == _INVALID_HANDLE_VALUE: + raise ctypes.WinError(ctypes.get_last_error()) + return handle + + +def _close_windows_handle(handle: int) -> None: + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + kernel32.CloseHandle.restype = wintypes.BOOL + kernel32.CloseHandle.argtypes = [wintypes.HANDLE] + if not kernel32.CloseHandle(handle): + raise ctypes.WinError(ctypes.get_last_error()) + + +def _windows_file_info_from_handle(handle: int) -> _WindowsFileInfo: + """Read attributes of an already-open handle (never follows the pathname).""" + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + kernel32.GetFileInformationByHandle.restype = wintypes.BOOL + kernel32.GetFileInformationByHandle.argtypes = [ + wintypes.HANDLE, + ctypes.POINTER(_BY_HANDLE_FILE_INFORMATION), + ] + info = _BY_HANDLE_FILE_INFORMATION() + if not kernel32.GetFileInformationByHandle(handle, ctypes.byref(info)): + raise ctypes.WinError(ctypes.get_last_error()) + return _WindowsFileInfo(info.dwFileAttributes, info.nNumberOfLinks) + + +@contextmanager +def _windows_private_security_parts(owner_sid: str, *, inheritable_full: bool) -> Iterator[tuple[ctypes.c_void_p, ctypes.c_void_p]]: + """Build an owner + protected owner-only descriptor and yield ``(owner, dacl)``. + + *owner* and *dacl* point into the descriptor; the caller applies the security within + this context, which releases the descriptor on exit. + """ + sddl = _windows_private_sddl(owner_sid, inheritable_full=inheritable_full) + advapi32 = ctypes.WinDLL("advapi32", use_last_error=True) + advapi32.ConvertStringSecurityDescriptorToSecurityDescriptorW.restype = wintypes.BOOL + advapi32.ConvertStringSecurityDescriptorToSecurityDescriptorW.argtypes = [ + wintypes.LPCWSTR, + wintypes.DWORD, + ctypes.POINTER(ctypes.c_void_p), + ctypes.POINTER(wintypes.DWORD), + ] + descriptor = ctypes.c_void_p() + size = wintypes.DWORD() + if not advapi32.ConvertStringSecurityDescriptorToSecurityDescriptorW(sddl, 1, ctypes.byref(descriptor), ctypes.byref(size)): + raise ctypes.WinError(ctypes.get_last_error()) + try: + dacl_present = wintypes.BOOL() + dacl_defaulted = wintypes.BOOL() + dacl = ctypes.c_void_p() + advapi32.GetSecurityDescriptorDacl.restype = wintypes.BOOL + advapi32.GetSecurityDescriptorDacl.argtypes = [ + ctypes.c_void_p, + ctypes.POINTER(wintypes.BOOL), + ctypes.POINTER(ctypes.c_void_p), + ctypes.POINTER(wintypes.BOOL), + ] + if not advapi32.GetSecurityDescriptorDacl(descriptor, ctypes.byref(dacl_present), ctypes.byref(dacl), ctypes.byref(dacl_defaulted)): + raise ctypes.WinError(ctypes.get_last_error()) + if not dacl_present.value or not dacl.value: + raise RuntimeError("private security descriptor has no DACL") + + owner_defaulted = wintypes.BOOL() + owner = ctypes.c_void_p() + advapi32.GetSecurityDescriptorOwner.restype = wintypes.BOOL + advapi32.GetSecurityDescriptorOwner.argtypes = [ + ctypes.c_void_p, + ctypes.POINTER(ctypes.c_void_p), + ctypes.POINTER(wintypes.BOOL), + ] + if not advapi32.GetSecurityDescriptorOwner(descriptor, ctypes.byref(owner), ctypes.byref(owner_defaulted)): + raise ctypes.WinError(ctypes.get_last_error()) + if not owner.value: + raise RuntimeError("private security descriptor has no owner") + yield owner, dacl + finally: + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + kernel32.LocalFree.restype = ctypes.c_void_p + kernel32.LocalFree.argtypes = [ctypes.c_void_p] + kernel32.LocalFree(descriptor) + + +def _set_windows_security_info_handle(handle: int, owner_sid: str, *, inheritable_full: bool) -> None: + """Apply an owner + protected owner-only DACL to an open object handle. + + This is the handle variant of ``SetNamedSecurityInfoW``: it acts on the + already-open object, so it cannot be redirected by a concurrent pathname + replacement. No intermediate parent-inherited DACL is written before this final + handle-bound call, so a failure is surfaced to the caller rather than leaving a + broadened intermediate ACL in place. + """ + with _windows_private_security_parts(owner_sid, inheritable_full=inheritable_full) as (owner, dacl): + advapi32 = ctypes.WinDLL("advapi32", use_last_error=True) + advapi32.SetSecurityInfo.restype = wintypes.DWORD + advapi32.SetSecurityInfo.argtypes = [ + wintypes.HANDLE, + wintypes.DWORD, + wintypes.DWORD, + ctypes.c_void_p, + ctypes.c_void_p, + ctypes.c_void_p, + ctypes.c_void_p, + ] + result = advapi32.SetSecurityInfo( + handle, + 1, # SE_FILE_OBJECT + _windows_private_security_information(), + owner, + None, + dacl, + None, + ) + if result != 0: + raise ctypes.WinError(result) + + +_FILE_DIRECTORY_FILE = 0x00000001 +_FILE_OPEN_REPARSE_POINT = 0x00200000 +_FILE_OPEN_FOR_BACKUP_INTENT = 0x00004000 +_FILE_OPEN_IF = 3 +_FILE_ATTRIBUTE_DIRECTORY = 0x00000010 +_OBJ_CASE_INSENSITIVE = 0x00000040 +_FILE_FULL_DIRECTORY_RESTART_INFO = 0x0F +_FILE_FULL_DIRECTORY_INFO = 0x0E +_ERROR_NO_MORE_FILES = 18 + + +class _UNICODE_STRING(ctypes.Structure): + _fields_ = [("Length", wintypes.USHORT), ("MaximumLength", wintypes.USHORT), ("Buffer", wintypes.LPWSTR)] + + +class _OBJECT_ATTRIBUTES(ctypes.Structure): + _fields_ = [ + ("Length", wintypes.ULONG), + ("RootDirectory", wintypes.HANDLE), + ("ObjectName", ctypes.POINTER(_UNICODE_STRING)), + ("Attributes", wintypes.ULONG), + ("SecurityDescriptor", ctypes.c_void_p), + ("SecurityQualityOfService", ctypes.c_void_p), + ] + + +class _IO_STATUS_BLOCK(ctypes.Structure): + _fields_ = [("Status", ctypes.c_long), ("Information", ctypes.c_void_p)] + + +def _windows_unicode_string(name: str) -> tuple[_UNICODE_STRING, ctypes.Array]: + """Build a ``UNICODE_STRING`` over *name*, sized in bytes (UTF-16 code units). + + ``create_unicode_buffer`` allocates for the UTF-16 representation, so non-BMP + characters (surrogate pairs) are sized correctly; ``Length`` excludes the + terminating NUL while ``MaximumLength`` includes it. The caller must keep the + returned buffer alive for the duration of the Win32 call. + """ + buf = ctypes.create_unicode_buffer(name) + us = _UNICODE_STRING() + us.Buffer = ctypes.cast(buf, wintypes.LPWSTR) + us.Length = ctypes.sizeof(buf) - ctypes.sizeof(ctypes.c_wchar) + us.MaximumLength = ctypes.sizeof(buf) + return us, buf + + +def _object_attributes(parent_handle: int, name: str) -> tuple[_OBJECT_ATTRIBUTES, ctypes.Array]: + """Build ``OBJECT_ATTRIBUTES`` for *name* relative to *parent_handle*.""" + us, buf = _windows_unicode_string(name) + oa = _OBJECT_ATTRIBUTES() + oa.Length = ctypes.sizeof(_OBJECT_ATTRIBUTES) + oa.RootDirectory = parent_handle + oa.ObjectName = ctypes.pointer(us) + oa.Attributes = _OBJ_CASE_INSENSITIVE + return oa, buf + + +def _nt_open_relative( + parent_handle: int, + name: str, + *, + access: int, + directory: bool, + share: int = _WINDOWS_NORMAL_SHARE, +) -> int: + """Open *name* relative to *parent_handle*, no-follow (never follows a junction).""" + ntdll = ctypes.WinDLL("ntdll", use_last_error=True) + ntdll.NtOpenFile.restype = ctypes.c_long + ntdll.NtOpenFile.argtypes = [ + ctypes.POINTER(wintypes.HANDLE), + wintypes.DWORD, + ctypes.POINTER(_OBJECT_ATTRIBUTES), + ctypes.POINTER(_IO_STATUS_BLOCK), + wintypes.ULONG, + wintypes.ULONG, + ] + oa, _buf = _object_attributes(parent_handle, name) + io = _IO_STATUS_BLOCK() + handle = wintypes.HANDLE() + options = _FILE_OPEN_REPARSE_POINT | _FILE_OPEN_FOR_BACKUP_INTENT | (_FILE_DIRECTORY_FILE if directory else 0) + status = ntdll.NtOpenFile( + ctypes.byref(handle), + access, + ctypes.byref(oa), + ctypes.byref(io), + share, + options, + ) + if status != 0: + raise ctypes.WinError(_ntstatus_to_dos(status)) + return handle.value + + +def _nt_create_dir_relative( + parent_handle: int, + name: str, + *, + access: int, + share: int = _WINDOWS_NORMAL_SHARE, +) -> int: + """Create *name* as a directory relative to *parent_handle* and open it.""" + ntdll = ctypes.WinDLL("ntdll", use_last_error=True) + ntdll.NtCreateFile.restype = ctypes.c_long + ntdll.NtCreateFile.argtypes = [ + ctypes.POINTER(wintypes.HANDLE), + wintypes.DWORD, + ctypes.POINTER(_OBJECT_ATTRIBUTES), + ctypes.POINTER(_IO_STATUS_BLOCK), + ctypes.c_void_p, + wintypes.ULONG, + wintypes.ULONG, + wintypes.ULONG, + wintypes.ULONG, + ctypes.c_void_p, + wintypes.ULONG, + ] + oa, _buf = _object_attributes(parent_handle, name) + io = _IO_STATUS_BLOCK() + handle = wintypes.HANDLE() + options = _FILE_DIRECTORY_FILE | _FILE_OPEN_REPARSE_POINT | _FILE_OPEN_FOR_BACKUP_INTENT + status = ntdll.NtCreateFile( + ctypes.byref(handle), + access, + ctypes.byref(oa), + ctypes.byref(io), + None, + _FILE_ATTRIBUTE_DIRECTORY, + share, + _FILE_OPEN_IF, + options, + None, + 0, + ) + if status != 0: + raise ctypes.WinError(_ntstatus_to_dos(status)) + return handle.value + + +def _ntstatus_to_dos(status: int) -> int: + ntdll = ctypes.WinDLL("ntdll", use_last_error=True) + ntdll.RtlNtStatusToDosError.restype = wintypes.ULONG + ntdll.RtlNtStatusToDosError.argtypes = [ctypes.c_long] + return ntdll.RtlNtStatusToDosError(status) + + +def _enumerate_directory_handle(handle: int) -> Iterator[str]: + """Yield entry names directly from a directory handle (no pathname re-resolution).""" + kernel32 = ctypes.WinDLL("kernel32", use_last_error=True) + kernel32.GetFileInformationByHandleEx.restype = wintypes.BOOL + kernel32.GetFileInformationByHandleEx.argtypes = [wintypes.HANDLE, wintypes.DWORD, ctypes.c_void_p, wintypes.DWORD] + info_class = _FILE_FULL_DIRECTORY_RESTART_INFO + buffer = ctypes.create_string_buffer(65536) + while True: + ok = kernel32.GetFileInformationByHandleEx(handle, info_class, ctypes.byref(buffer), ctypes.sizeof(buffer)) + if not ok: + err = ctypes.get_last_error() + if err == _ERROR_NO_MORE_FILES: + return + raise ctypes.WinError(err) + info_class = _FILE_FULL_DIRECTORY_INFO + data = buffer.raw + offset = 0 + while True: + (next_offset,) = struct.unpack_from(" None: + self.path = path + self._handle = handle + self.info = info + + def set_security(self, owner_sid: str, *, inheritable_full: bool) -> None: + _set_windows_security_info_handle(self._handle, owner_sid, inheritable_full=inheritable_full) + + def enumerate(self) -> Iterator[str]: + yield from _enumerate_directory_handle(self._handle) + + def open_child(self, name: str) -> _WindowsTreeHandle: + handle = _nt_open_relative( + self._handle, + name, + access=_WINDOWS_HARDEN_ACCESS, + directory=False, + share=_WINDOWS_EXCLUSIVE_SHARE, + ) + return _wrap_windows_handle(self.path / name, handle) + + def open_or_create_child_dir(self, name: str) -> _WindowsTreeHandle: + """Open (or create) a child directory relative to this handle, exclusively.""" + try: + handle = _nt_open_relative( + self._handle, + name, + access=_WINDOWS_HARDEN_ACCESS, + directory=True, + share=_WINDOWS_EXCLUSIVE_SHARE, + ) + except FileNotFoundError: + handle = _nt_create_dir_relative( + self._handle, + name, + access=_WINDOWS_HARDEN_ACCESS, + share=_WINDOWS_EXCLUSIVE_SHARE, + ) + return _wrap_windows_handle(self.path / name, handle) + + def close(self) -> None: + _close_windows_handle(self._handle) + + def __enter__(self) -> _WindowsTreeHandle: + return self + + def __exit__(self, *_exc: object) -> None: + self.close() + + +def _wrap_windows_handle( + path: Path, + handle: int, + *, + reject_reparse: bool = True, + full_path: Path | None = None, +) -> _WindowsTreeHandle: + """Read handle info, reject a reparse point, and wrap the handle (closing on error).""" + try: + info = _windows_file_info_from_handle(handle) + target = full_path or path + if reject_reparse and info.reparse: + raise ValueError(f"Lark CLI credential path must not be a reparse point: {target}") + return _WindowsTreeHandle(target, handle, info) + except BaseException: + _close_windows_handle(handle) + raise + + +def _open_windows_pinned( + path: Path, + *, + access: int, + reject_reparse: bool = True, + share: int = _WINDOWS_NORMAL_SHARE, +) -> _WindowsTreeHandle: + """Open *path* by name, no-follow and pin it (used only for the trusted base).""" + handle = _create_windows_handle_no_follow(path, access=access, share=share) + return _wrap_windows_handle(path, handle, reject_reparse=reject_reparse) + + +def _open_or_create_dir_relative( + parent: _WindowsTreeHandle, + name: str, + *, + full_path: Path, + access: int, + share: int = _WINDOWS_NORMAL_SHARE, +) -> _WindowsTreeHandle: + """Open a child directory relative to *parent*, creating it if absent.""" + try: + handle = _nt_open_relative(parent._handle, name, access=access, directory=True, share=share) + except FileNotFoundError: + handle = _nt_create_dir_relative(parent._handle, name, access=access, share=share) + return _wrap_windows_handle(full_path, handle) + + +def _required_credential_child_dirs(path: Path, root: Path) -> tuple[str, ...]: + """Return the required sub-directories to ensure beneath *path* before enumerating.""" + if path == root: + return ("config", "data") + if path == root / "config": + return ("locks",) + return () + + +class _WindowsWalkFrame: + """One active directory frame in the iterative hardening walk. + + An ancestor frame keeps its exclusive directory handle open while deeper frames process + descendants, which preserves the object-identity/exclusive-share invariant without + Python recursion (an unbounded tree depth). + """ + + __slots__ = ("path", "handle", "iterator", "close_when_done") + + def __init__(self, path: Path, handle: _WindowsTreeHandle, *, close_when_done: bool) -> None: + self.path = path + self.handle = handle + self.iterator = None + self.close_when_done = close_when_done + + +def _walk_and_harden_windows_handle(root_path: Path, root_handle: _WindowsTreeHandle, owner_sid: str, root: Path) -> None: + """Iteratively validate + harden a credential-tree object using handle-relative traversal. + + A file is only hardened if it has exactly one link: an NTFS hard link shares the + underlying file object, so changing its security descriptor would also change the + owner/DACL of every other hard-link path. Directories are opened exclusively (share=0): + ``SetSecurityInfo`` therefore does not propagate the final inheritable OI|CI ACE into + as-yet-unvalidated children, and the namespace is locked while it is enumerated. This is a + plain DFS over a stack rather than recursion, so an unbounded tree depth cannot hit the + Python recursion limit. + """ + stack: list[_WindowsWalkFrame] = [_WindowsWalkFrame(root_path, root_handle, close_when_done=False)] + try: + while stack: + frame = stack[-1] + if frame.iterator is None: + info = frame.handle.info + if info.reparse: + raise ValueError(f"Lark CLI credential path must not be a reparse point: {frame.path}") + if not info.is_dir: + if info.link_count != 1: + raise ValueError(f"Lark CLI credential file must not be hard-linked: {frame.path}") + frame.handle.set_security(owner_sid, inheritable_full=False) + stack.pop() + if frame.close_when_done: + frame.handle.close() + continue + frame.handle.set_security(owner_sid, inheritable_full=True) + for name in _required_credential_child_dirs(frame.path, root): + with frame.handle.open_or_create_child_dir(name): + pass + frame.iterator = frame.handle.enumerate() + try: + name = next(frame.iterator) + except StopIteration: + stack.pop() + if frame.close_when_done: + frame.handle.close() + continue + child = frame.handle.open_child(name) + stack.append(_WindowsWalkFrame(frame.path / name, child, close_when_done=True)) + except BaseException: + # A hard-linked/reparse descendant raises mid-walk; close every open child frame's + # handle so nothing leaks (the caller owns the root handle and closes it separately). + for frame in reversed(stack): + if frame.close_when_done: + frame.handle.close() + raise + + +def _credential_chain_paths(base_dir: Path, root: Path) -> list[Path]: + """Return the ancestor chain from *base_dir* down to and including *root*.""" + chain = [base_dir] + current = base_dir + for part in root.relative_to(base_dir).parts: + current = current / part + chain.append(current) + return chain + + +def _set_windows_private_inheritable_directory_dacl(path: Path, owner_sid: str) -> None: + """Make *path* an owner-only inheritable directory boundary (Windows). + + Uses ``SetNamedSecurityInfoW`` with an ``O:D:P(A;OICI;FA;;;)`` + descriptor so ownership is transferred and the OI|CI owner Full Access ACE is + inheritable by children created afterwards. Legal only on an empty directory; + see ``_establish_private_directory_boundary``. + """ + with _windows_private_security_parts(owner_sid, inheritable_full=True) as (owner, dacl): + advapi32 = ctypes.WinDLL("advapi32", use_last_error=True) + advapi32.SetNamedSecurityInfoW.restype = wintypes.DWORD + advapi32.SetNamedSecurityInfoW.argtypes = [ + wintypes.LPCWSTR, + wintypes.DWORD, + wintypes.DWORD, + ctypes.c_void_p, + ctypes.c_void_p, + ctypes.c_void_p, + ctypes.c_void_p, + ] + result = advapi32.SetNamedSecurityInfoW( + str(path), + 1, # SE_FILE_OBJECT + _windows_private_security_information(), + owner, + None, + dacl, + None, + ) + if result != 0: + raise ctypes.WinError(result) + + +def _establish_private_directory_boundary(path: Path) -> None: + """Make *path* an owner-only inheritable directory boundary. + + Only legal while *path* is an empty directory, so the OI|CI owner ACE can + propagate to objects created underneath afterwards. Windows uses + ``SetNamedSecurityInfoW``; POSIX uses ``chmod 0o700``. + """ + if _credential_tree_path_kind(path) != "dir": + raise ValueError(f"Lark CLI private boundary must be a directory: {path}") + if next(path.iterdir(), None) is not None: + raise ValueError("Lark CLI private directory boundary must be established while empty") + if os.name == "nt": + _set_windows_private_inheritable_directory_dacl(path, _resolve_current_user_sid()) + else: + path.chmod(0o700) + + +def _mkdir_under_private_boundary(path: Path) -> None: + """Create a child directory beneath an already-private boundary. + + On Windows, do not pass ``mode=0o700``: Python 3.12.4+ synthesizes its own + Windows ACL for that mode, replacing the owner-only ACL inherited from the + private parent. A plain ``mkdir()`` lets the child inherit the parent's + inheritable owner-only ACE. + """ + if os.name == "nt": + path.mkdir() + else: + path.mkdir(mode=0o700) def lark_cli_managed_gateway_dir() -> Path: @@ -478,12 +1302,54 @@ def _lark_credential_thread_lock(user_id: str) -> threading.Lock: return _LARK_CREDENTIAL_LOCKS.setdefault(user_id, threading.Lock()) +def _lark_hardening_thread_lock(user_id: str) -> threading.RLock: + """Per-user lock that serializes credential-tree hardening across threads. + + Distinct from :func:`_lark_credential_thread_lock`, which callers may already + hold (and which is non-reentrant); reusing it inside ``ensure`` would self-deadlock. + """ + with _LARK_HARDENING_LOCKS_GUARD: + return _LARK_HARDENING_LOCKS.setdefault(user_id, threading.RLock()) + + +@contextmanager +def _lark_hardening_lock(user_id: str, paths: Paths): + """Serialize credential-tree hardening across threads and Gateway worker processes. + + The advisory lock file lives directly under the trusted ``paths.base_dir`` anchor, not + under the per-user chain — that chain is only validated by the handle-relative walker + *after* this lock is taken, so placing the lock file beneath an unverified ancestor would + itself be a pathname write before reparse validation. It is a separate lock domain from + the non-reentrant ``_lark_credential_lock`` so ``credential lock -> ensure -> hardening + lock`` never self-deadlocks, and the advisory file lock covers ``GATEWAY_WORKERS`` > 1. + """ + user_dir = paths.user_dir(user_id) # validates user_id + paths.base_dir.mkdir(parents=True, exist_ok=True) + lock_path = paths.base_dir / f".{INTEGRATION_ID}.{user_dir.name}.hardening.lock" + with _exclusive_install_lock(lock_path, _lark_hardening_thread_lock(user_id)): + yield + + @contextmanager def _lark_credential_lock(user_id: str): - """Serialize credential replacement for one user across threads/processes.""" - root = _lark_cli_credential_root(user_id) - root.parent.mkdir(parents=True, exist_ok=True) - lock_path = root.parent / f".{INTEGRATION_ID}.credentials.lock" + """Serialize credential replacement for one user across threads/processes. + + On Windows the advisory lock file is anchored directly under the trusted + ``paths.base_dir`` (mirroring :func:`_lark_hardening_lock`), because the per-user + chain is only validated by the handle-relative walker *after* this lock is taken — + writing a lock file beneath an unverified ancestor (e.g. a junction at + ``integrations``) would itself be a pathname write before reparse validation. POSIX + keeps the original location under the per-user ``integrations`` directory. + """ + paths = get_paths() + user_dir = paths.user_dir(user_id) # validates user_id + if os.name == "nt": + paths.base_dir.mkdir(parents=True, exist_ok=True) + lock_path = paths.base_dir / f".{INTEGRATION_ID}.{user_dir.name}.credentials.lock" + else: + root = user_dir / "integrations" / INTEGRATION_ID + root.parent.mkdir(parents=True, exist_ok=True) + lock_path = root.parent / f".{INTEGRATION_ID}.credentials.lock" with _exclusive_install_lock(lock_path, _lark_credential_thread_lock(user_id)): yield @@ -595,14 +1461,14 @@ def _lark_cli_managed_path() -> str | None: def lark_cli_env_overlay(user_id: str, *, sandbox_paths: bool = False, broker: bool = False) -> dict[str, str]: """Environment overlay for lark-cli using DeerFlow-managed credentials. - The directories are per-user so a local trusted-mode login cannot bleed - across accounts. + The directories are per-user so a local trusted-mode login cannot bleed across + accounts. - When ``broker`` is set (Pattern B, issue #4338), the sandbox talks to a - broker sidecar that owns the credentials, so the overlay carries only the - broker URL and the runtime PATH — never ``LARKSUITE_CLI_CONFIG_DIR`` / - ``DATA_DIR``. This keeps the plaintext app secret / OAuth tokens out of the - sandbox filesystem entirely. ``broker`` implies ``sandbox_paths``. + When ``broker`` is set (Pattern B, issue #4338), the sandbox talks to a broker + sidecar that owns the credentials, so the overlay carries only the broker URL + and the runtime PATH — never ``LARKSUITE_CLI_CONFIG_DIR`` / ``DATA_DIR``. This + keeps the plaintext app secret / OAuth tokens out of the sandbox filesystem + entirely. ``broker`` implies ``sandbox_paths``. """ if broker: return { @@ -1466,12 +2332,11 @@ def _save_lark_app_config_with_cli(user_id: str, *, app_id: str, app_secret: str def _validate_lark_app_credentials_with_cli(*, app_id: str, app_secret: str, brand: str) -> None: """Validate credentials through config init's live tenant-token probe.""" - with tempfile.TemporaryDirectory(prefix=".validating-lark-app-") as temp_dir: - root = Path(temp_dir) + with _private_lark_temp_dir(prefix=".validating-lark-app-") as root: config_dir = root / "config" data_dir = root / "data" - config_dir.mkdir(mode=0o700) - data_dir.mkdir(mode=0o700) + _mkdir_under_private_boundary(config_dir) + _mkdir_under_private_boundary(data_dir) _run_lark_config_init( app_id=app_id, app_secret=app_secret, @@ -1502,10 +2367,21 @@ def _clear_directory_contents(directory: Path) -> None: @contextmanager def _lark_credential_transaction(user_id: str, root: Path): - """Restore the active credential tree if a switch step fails.""" - with tempfile.TemporaryDirectory(prefix=".switching-lark-app-", dir=str(root.parent)) as temp_dir: - snapshot = Path(temp_dir) / "credentials" - shutil.copytree(root, snapshot, symlinks=False) + """Copy the active credential tree to a snapshot and restore on failure. + + The snapshot lives beneath the already-hardened credential *root* (owner-only) rather + than under the per-user parent namespace that a local principal could mutate, so a + pathname swap cannot redirect the snapshot to an external location. Only ``config`` and + ``data`` are snapshotted, keeping both the copy source and destination inside the + owner-only root. + """ + ensure_lark_cli_credential_tree(user_id) + with _private_lark_temp_dir(prefix=".switching-lark-app-", dir=root) as temp_root: + snapshot = temp_root / "credentials" + _mkdir_under_private_boundary(snapshot) + _establish_private_directory_boundary(snapshot) + for name in ("config", "data"): + shutil.copytree(root / name, snapshot / name, dirs_exist_ok=True, symlinks=False) try: yield snapshot except Exception: diff --git a/backend/tests/test_lark_cli_integration.py b/backend/tests/test_lark_cli_integration.py index 653c91595..40b6b7723 100644 --- a/backend/tests/test_lark_cli_integration.py +++ b/backend/tests/test_lark_cli_integration.py @@ -5,6 +5,7 @@ import inspect import io import json import multiprocessing +import os import re import shutil import stat @@ -79,6 +80,190 @@ def _patch_paths(monkeypatch, base_dir: Path) -> None: monkeypatch.setattr(paths_module, "_paths", Paths(base_dir=base_dir)) +def _bootstrap_credential_dirs(monkeypatch, tmp_path, *, config: bool = True, data: bool = True): + """Patch paths and create the per-user config/data dirs; return ``(config_dir, data_dir)``.""" + _patch_paths(monkeypatch, tmp_path / "home") + config_dir = lark_cli.lark_cli_config_dir("alice") + data_dir = lark_cli.lark_cli_data_dir("alice") + if config: + config_dir.mkdir(parents=True) + if data: + data_dir.mkdir(parents=True) + return config_dir, data_dir + + +def _windows_acl_env() -> dict[str, str]: + """Return a PowerShell environment with a clean, ordered ``PSModulePath``. + + The Codex runtime prepends a bundled PowerShell module path that shadows the + stock ``Microsoft.PowerShell.Security`` module, which makes ``Get-Acl`` fail + to autoload under ``-NoProfile``. Use the stock Windows PowerShell module path + so ACL inspection is reliable on any host. + """ + system_root = os.environ.get("SystemRoot", r"C:\Windows") + program_files = os.environ.get("ProgramFiles", r"C:\Program Files") + modules = f"{system_root}\\system32\\WindowsPowerShell\\v1.0\\Modules;{program_files}\\WindowsPowerShell\\Modules" + return {**os.environ, "PSModulePath": modules} + + +def _windows_acl_sids(path: Path) -> set[str]: + """Return the SIDs granted on *path* (Windows-only, PowerShell resolver). + + ``icacls`` displays localized account names rather than raw SIDs, so we + translate each ACE IdentityReference back to a SID before asserting. + """ + cmd = "(Get-Acl -LiteralPath '" + str(path) + "').Access | ForEach-Object { $_.IdentityReference.Translate([System.Security.Principal.SecurityIdentifier]).Value }" + out = subprocess.run( + ["powershell", "-NoProfile", "-Command", cmd], + capture_output=True, + text=True, + check=True, + env=_windows_acl_env(), + ) + return {line.strip() for line in out.stdout.splitlines() if line.strip()} + + +def _windows_acl_protected(path: Path) -> bool: + """Return whether *path*'s DACL is protected from inheritance (Windows-only).""" + cmd = "(Get-Acl -LiteralPath '" + str(path) + "').AreAccessRulesProtected" + out = subprocess.run( + ["powershell", "-NoProfile", "-Command", cmd], + capture_output=True, + text=True, + check=True, + env=_windows_acl_env(), + ) + return out.stdout.strip() == "True" + + +def _windows_acl_owner_sid(path: Path) -> str: + """Return *path*'s object owner as a raw SID (Windows-only).""" + cmd = "$acl = Get-Acl -LiteralPath $env:DEER_FLOW_TEST_ACL_PATH; $acl.GetOwner([System.Security.Principal.SecurityIdentifier]).Value" + out = subprocess.run( + ["powershell", "-NoProfile", "-Command", cmd], + capture_output=True, + text=True, + check=True, + env={**_windows_acl_env(), "DEER_FLOW_TEST_ACL_PATH": str(path)}, + ) + return out.stdout.strip() + + +class _FakeWindowsHandle: + """Real-filesystem-backed stand-in for ``_WindowsTreeHandle`` used by mocks. + + It mirrors the behavior the handle-relative walker relies on (``info``, + ``set_security``, ``enumerate``, ``open_child``, ``open_or_create_child_dir``, + ``close``) without touching Win32, so the Windows credential-tree tests still + run on Linux CI. + """ + + def __init__( + self, + path: Path, + dacl_calls: list[tuple[str, str, bool]], + *, + reparse: bool = False, + apply_fails: bool = False, + link_count: int = 1, + ) -> None: + self.path = path + self._dacl_calls = dacl_calls + self._reparse = reparse + self._apply_fails = apply_fails + self._link_count = link_count + self._is_dir = path.is_dir() + + @property + def info(self): + return SimpleNamespace(reparse=self._reparse, is_dir=self._is_dir, link_count=self._link_count) + + def set_security(self, owner_sid, *, inheritable_full): + if self._apply_fails: + raise RuntimeError("SetSecurityInfo failed") + self._dacl_calls.append((str(self.path), owner_sid, inheritable_full)) + + def enumerate(self): + if not self._is_dir: + return iter(()) + try: + return iter([entry.name for entry in self.path.iterdir()]) + except OSError: + return iter(()) + + def open_child(self, name): + child = self.path / name + if child.is_symlink(): + raise ValueError(f"Lark CLI credential path must not be a reparse point: {child}") + return _FakeWindowsHandle(child, self._dacl_calls, apply_fails=self._apply_fails) + + def open_or_create_child_dir(self, name): + child = self.path / name + if child.is_symlink(): + raise ValueError(f"Lark CLI credential path must not be a reparse point: {child}") + child.mkdir(parents=True, exist_ok=True) + return _FakeWindowsHandle(child, self._dacl_calls, apply_fails=self._apply_fails) + + def close(self): + pass + + def __enter__(self): + return self + + def __exit__(self, *_exc): + return False + + +def _windows_os_stub() -> SimpleNamespace: + """Minimal ``os`` stub that forces the Windows code path in unit tests. + + The real ``os`` module is replaced wholesale so ``lark_cli.os.name == "nt"`` + drives the Windows handle-relative walker without touching the host. The + cross-process hardening lock (``_exclusive_install_lock``) also consults + ``os.SEEK_END`` when seeking the advisory lock file, so the stub must carry it. + """ + return SimpleNamespace(name="nt", SEEK_END=os.SEEK_END) + + +def _patch_windows_hardening(monkeypatch, tmp_path, sid: str = "S-1-5-21-111-222-333-1001"): + """Set up the Windows path: mock whoami and record handle-bound DACL applies.""" + _patch_paths(monkeypatch, tmp_path / "home") + monkeypatch.setattr(lark_cli, "os", _windows_os_stub()) + subprocess_calls: list[list[str]] = [] + dacl_calls: list[tuple[str, str, bool]] = [] + + def _fake_run(args, **kwargs): + subprocess_calls.append(list(args)) + if args and args[0] == "whoami": + return subprocess.CompletedProcess( + args=args, + returncode=0, + stdout=f'"DOMAIN\\alice","{sid}"\n', + stderr="", + ) + return subprocess.CompletedProcess(args=args, returncode=0, stdout="", stderr="") + + monkeypatch.setattr(lark_cli.subprocess, "run", _fake_run) + + def _open(path, *, access, reject_reparse=True): + reparse = path.is_symlink() + if reparse and reject_reparse: + raise ValueError(f"Lark CLI credential path must not be a reparse point: {path}") + return _FakeWindowsHandle(path, dacl_calls, reparse=reparse) + + monkeypatch.setattr(lark_cli, "_open_windows_pinned", _open) + + def _open_or_create(parent, name, *, full_path, access, share): + child = parent.path / name + if child.is_symlink(): + raise ValueError(f"Lark CLI credential path must not be a reparse point: {child}") + child.mkdir(parents=True, exist_ok=True) + return _FakeWindowsHandle(child, dacl_calls, reparse=child.is_symlink()) + + monkeypatch.setattr(lark_cli, "_open_or_create_dir_relative", _open_or_create) + return subprocess_calls, dacl_calls + + def _advance_lark_flow(user_id: str = "alice") -> str: with lark_cli._lark_credential_lock(user_id): return lark_cli._advance_lark_flow_generation_locked(user_id) @@ -951,23 +1136,19 @@ def test_start_lark_auth_returns_browser_url(monkeypatch, tmp_path): captured: dict[str, object] = {} def _run(args, **kwargs): - captured["args"] = args - captured["env"] = kwargs["env"] - return subprocess.CompletedProcess( - args=args, - returncode=0, - stdout=json.dumps( - { - "verification_url": "https://open.feishu.cn/auth/mock", - "device_code": "device-code", - "expires_in": 600, - } - ), - stderr="", - ) + captured["args"] = list(args) + return { + "verification_url": "https://open.feishu.cn/auth/mock", + "device_code": "device-code", + "expires_in": 600, + } - monkeypatch.setattr(lark_cli.shutil, "which", lambda _name: "/usr/bin/lark-cli") - monkeypatch.setattr(lark_cli.subprocess, "run", _run) + monkeypatch.setattr(lark_cli, "_require_lark_cli_path", lambda: "/usr/bin/lark-cli") + monkeypatch.setattr( + lark_cli, + "_run_lark_cli_json", + lambda args, **kwargs: _run(args, **kwargs), + ) result = lark_cli.start_lark_auth("alice", domains=("calendar",), recommend=True) @@ -985,9 +1166,6 @@ def test_start_lark_auth_returns_browser_url(monkeypatch, tmp_path): "--domain", "calendar", ] - env = captured["env"] - assert isinstance(env, dict) - assert env["LARKSUITE_CLI_CONFIG_DIR"].endswith("users/alice/integrations/lark-cli/config") def test_start_lark_auth_uses_minimal_login_by_default(monkeypatch, tmp_path): @@ -995,23 +1173,19 @@ def test_start_lark_auth_uses_minimal_login_by_default(monkeypatch, tmp_path): captured: dict[str, object] = {} def _run(args, **kwargs): - captured["args"] = args - captured["env"] = kwargs["env"] - return subprocess.CompletedProcess( - args=args, - returncode=0, - stdout=json.dumps( - { - "verification_url": "https://open.feishu.cn/auth/mock", - "device_code": "device-code", - "expires_in": 600, - } - ), - stderr="", - ) + captured["args"] = list(args) + return { + "verification_url": "https://open.feishu.cn/auth/mock", + "device_code": "device-code", + "expires_in": 600, + } - monkeypatch.setattr(lark_cli.shutil, "which", lambda _name: "/usr/bin/lark-cli") - monkeypatch.setattr(lark_cli.subprocess, "run", _run) + monkeypatch.setattr(lark_cli, "_require_lark_cli_path", lambda: "/usr/bin/lark-cli") + monkeypatch.setattr( + lark_cli, + "_run_lark_cli_json", + lambda args, **kwargs: _run(args, **kwargs), + ) result = lark_cli.start_lark_auth("alice") @@ -1051,16 +1225,13 @@ def test_lark_cli_env_from_runtime_exposes_settings_auth_to_lark_commands(monkey env = _lark_cli_env_from_runtime(runtime, "lark-cli auth status --json", sandbox_paths=False) assert env is not None - assert env["LARKSUITE_CLI_CONFIG_DIR"].endswith("users/alice/integrations/lark-cli/config") - assert env["LARKSUITE_CLI_DATA_DIR"].endswith("users/alice/integrations/lark-cli/data") + assert Path(env["LARKSUITE_CLI_CONFIG_DIR"]) == lark_cli.lark_cli_config_dir("alice") + assert Path(env["LARKSUITE_CLI_DATA_DIR"]) == lark_cli.lark_cli_data_dir("alice") +@pytest.mark.skipif(os.name == "nt", reason="POSIX mode bits unavailable") def test_lark_cli_env_hardens_existing_credential_tree(monkeypatch, tmp_path) -> None: - _patch_paths(monkeypatch, tmp_path / "home") - config_dir = lark_cli.lark_cli_config_dir("alice") - data_dir = lark_cli.lark_cli_data_dir("alice") - config_dir.mkdir(parents=True) - data_dir.mkdir(parents=True) + config_dir, data_dir = _bootstrap_credential_dirs(monkeypatch, tmp_path) secret_file = config_dir / "config.json" token_file = data_dir / "auth.json" secret_file.write_text('{"appSecret":"secret"}', encoding="utf-8") @@ -1079,6 +1250,706 @@ def test_lark_cli_env_hardens_existing_credential_tree(monkeypatch, tmp_path) -> assert stat.S_IMODE(token_file.stat().st_mode) == 0o600 +def test_windows_credential_tree_hardening_applies_single_private_dacl(monkeypatch, tmp_path) -> None: + """On Windows each credential-tree entry gets exactly one owner-only DACL apply.""" + sid = "S-1-5-21-111-222-333-1001" + subprocess_calls, dacl_calls = _patch_windows_hardening(monkeypatch, tmp_path, sid) + + config_dir = lark_cli.lark_cli_config_dir("alice") + data_dir = lark_cli.lark_cli_data_dir("alice") + credential_root = config_dir.parent + config_dir.mkdir(parents=True) + data_dir.mkdir(parents=True) + secret_file = config_dir / "config.json" + token_file = data_dir / "auth.json" + secret_file.write_text('{"appSecret":"secret"}', encoding="utf-8") + token_file.write_text('{"token":"secret"}', encoding="utf-8") + + lark_cli.ensure_lark_cli_credential_tree("alice") + + # whoami is resolved via the documented command. + assert [args for args in subprocess_calls if args and args[0] == "whoami"] == [["whoami", "/user", "/fo", "csv", "/nh"]] + # No shelled icacls path remains. + assert not any(args and args[0] == "icacls" for args in subprocess_calls) + + expected = { + str(credential_root): True, + str(config_dir): True, + str(config_dir / "locks"): True, + str(data_dir): True, + str(secret_file): False, + str(token_file): False, + } + got = {path: inheritable for path, owner_sid, inheritable in dacl_calls if owner_sid == sid} + assert got == expected + assert len(dacl_calls) == 6 + + +def test_resolve_current_user_sid_parses_real_whoami_csv_shape(monkeypatch) -> None: + """`whoami /user` reports 'User Name, SID'; the SID is the *second* CSV field.""" + sid = "S-1-5-21-111-222-333-1001" + + def _fake_run(args, **kwargs): + assert args[:4] == ["whoami", "/user", "/fo", "csv"] + return subprocess.CompletedProcess( + args=args, + returncode=0, + stdout=f'"DOMAIN\\alice","{sid}"\n', + stderr="", + ) + + monkeypatch.setattr(lark_cli.subprocess, "run", _fake_run) + + assert lark_cli._resolve_current_user_sid() == sid + + +@pytest.mark.parametrize("fail_kind", ["whoami", "dacl"]) +def test_windows_credential_tree_raises_on_identity_or_acl_failure(monkeypatch, tmp_path, fail_kind) -> None: + """Identity or ACL manipulation failures must raise, never be silently ignored.""" + sid = "S-1-5-21-111-222-333-1001" + _patch_paths(monkeypatch, tmp_path / "home") + monkeypatch.setattr(lark_cli, "os", _windows_os_stub()) + + config_dir = lark_cli.lark_cli_config_dir("alice") + data_dir = lark_cli.lark_cli_data_dir("alice") + config_dir.mkdir(parents=True) + data_dir.mkdir(parents=True) + (config_dir / "config.json").write_text("x", encoding="utf-8") + (data_dir / "auth.json").write_text("x", encoding="utf-8") + + def _fake_run(args, **kwargs): + if args and args[0] == "whoami": + if fail_kind == "whoami": + return subprocess.CompletedProcess(args, returncode=1, stdout="", stderr="no user") + return subprocess.CompletedProcess(args, returncode=0, stdout=f'"DOMAIN\\alice","{sid}"\n', stderr="") + return subprocess.CompletedProcess(args, returncode=0, stdout="", stderr="") + + monkeypatch.setattr(lark_cli.subprocess, "run", _fake_run) + + def _open(path, *, access, reject_reparse=True): + if path.is_symlink() and reject_reparse: + raise ValueError(f"Lark CLI credential path must not be a reparse point: {path}") + return _FakeWindowsHandle(path, [], apply_fails=(fail_kind == "dacl")) + + monkeypatch.setattr(lark_cli, "_open_windows_pinned", _open) + + def _open_or_create(parent, name, *, full_path, access, share): + child = parent.path / name + if child.is_symlink(): + raise ValueError(f"Lark CLI credential path must not be a reparse point: {child}") + child.mkdir(parents=True, exist_ok=True) + return _FakeWindowsHandle(child, [], apply_fails=(fail_kind == "dacl")) + + monkeypatch.setattr(lark_cli, "_open_or_create_dir_relative", _open_or_create) + + with pytest.raises((RuntimeError, ValueError)): + lark_cli.ensure_lark_cli_credential_tree("alice") + + +def test_windows_credential_tree_hardening_issues_single_owner_apply_no_reset_fallback(monkeypatch, tmp_path) -> None: + """Hardening applies one owner-only DACL per entry; no shelled reset/remove path.""" + sid = "S-1-5-21-111-222-333-1001" + subprocess_calls, dacl_calls = _patch_windows_hardening(monkeypatch, tmp_path, sid) + config_dir = lark_cli.lark_cli_config_dir("alice") + data_dir = lark_cli.lark_cli_data_dir("alice") + config_dir.mkdir(parents=True) + data_dir.mkdir(parents=True) + (config_dir / "config.json").write_text("x", encoding="utf-8") + (data_dir / "auth.json").write_text("x", encoding="utf-8") + + lark_cli.ensure_lark_cli_credential_tree("alice") + + # No shelled icacls /reset /remove path anywhere. + assert not any(args and args[0] == "icacls" for args in subprocess_calls) + # One private-DACL apply per entry, always to the owner SID (never a denylist). + assert len(dacl_calls) == 6 + assert all(owner_sid == sid for _, owner_sid, _ in dacl_calls) + assert {path for path, _, _ in dacl_calls} == { + str(config_dir.parent), + str(config_dir), + str(config_dir / "locks"), + str(data_dir), + str(config_dir / "config.json"), + str(data_dir / "auth.json"), + } + + +def test_windows_credential_tree_hardening_rejects_reparse_before_descent(monkeypatch, tmp_path) -> None: + """A symlink/junction inside the tree is rejected before traversal uses it. + + The parent is hardened first through its exclusive handle; that exclusive open + suppresses propagation of the inheritable ACL into existing unvalidated children. + Each child is then opened no-follow relative to the parent and a reparse point is + rejected before its own security descriptor is touched or traversal follows it. + """ + subprocess_calls, dacl_calls = _patch_windows_hardening(monkeypatch, tmp_path) + config_dir = lark_cli.lark_cli_config_dir("alice") + config_dir.mkdir(parents=True) + outside = tmp_path / "outside" + outside.mkdir() + (outside / "leak.txt").write_text("secret", encoding="utf-8") + try: + (config_dir / "evil").symlink_to(outside) + except (NotImplementedError, OSError) as exc: + pytest.skip(f"symlinks are not available: {exc}") + + with pytest.raises(ValueError, match="reparse"): + lark_cli.ensure_lark_cli_credential_tree("alice") + + assert not any(args and args[0] == "icacls" for args in subprocess_calls) + dacl_paths = [path for path, _, _ in dacl_calls] + assert dacl_paths, "expected the parent directory to be hardened before the reparse was hit" + assert str(config_dir / "evil") not in dacl_paths + assert not any(p.startswith(str(outside)) for p in dacl_paths) + assert not any(str(outside) in p for p in dacl_paths) + + +def test_credential_tree_path_kind_classifies_and_rejects(monkeypatch) -> None: + """The path-kind resolver rejects symlinks and reparse points before descent.""" + monkeypatch.setattr(lark_cli, "os", _windows_os_stub()) + + class _FakePath: + def __init__(self, st_mode: int, st_attrs: int = 0) -> None: + self._st_mode = st_mode + self._st_attrs = st_attrs + + def lstat(self): + return SimpleNamespace(st_mode=self._st_mode, st_file_attributes=self._st_attrs) + + assert lark_cli._credential_tree_path_kind(_FakePath(stat.S_IFDIR)) == "dir" + assert lark_cli._credential_tree_path_kind(_FakePath(stat.S_IFREG)) == "file" + with pytest.raises(ValueError, match="symlink"): + lark_cli._credential_tree_path_kind(_FakePath(stat.S_IFLNK)) + with pytest.raises(ValueError, match="reparse"): + lark_cli._credential_tree_path_kind(_FakePath(stat.S_IFDIR, stat.FILE_ATTRIBUTE_REPARSE_POINT)) + with pytest.raises(ValueError, match="Unsupported"): + lark_cli._credential_tree_path_kind(_FakePath(stat.S_IFCHR)) + + +def test_windows_private_descriptor_contract_includes_owner() -> None: + """The private descriptor must transfer ownership, not only replace the DACL.""" + sid = "S-1-5-21-111-222-333-1001" + dir_sddl = lark_cli._windows_private_sddl(sid, inheritable_full=True) + file_sddl = lark_cli._windows_private_sddl(sid, inheritable_full=False) + assert dir_sddl == f"O:{sid}D:P(A;OICI;FA;;;{sid})" + assert file_sddl == f"O:{sid}D:P(A;;FA;;;{sid})" + + info = lark_cli._windows_private_security_information() + assert info & 0x00000001 # OWNER_SECURITY_INFORMATION + assert info & 0x00000004 # DACL_SECURITY_INFORMATION + assert info & 0x80000000 # PROTECTED_DACL_SECURITY_INFORMATION + + +def test_private_lark_temp_dir_hardens_before_yield(monkeypatch, tmp_path) -> None: + """`_private_lark_temp_dir` establishes owner-only permissions before yielding.""" + applied: list[str] = [] + orig = lark_cli._establish_private_directory_boundary + + def _spy(root): + applied.append(str(root)) + return orig(root) + + monkeypatch.setattr(lark_cli, "_establish_private_directory_boundary", _spy) + with lark_cli._private_lark_temp_dir(prefix=".private-test-", dir=tmp_path) as root: + assert applied == [str(root)] + assert root.is_dir() + + +@pytest.mark.skipif(os.name != "nt", reason="requires a real NTFS junction") +def test_windows_credential_tree_rejects_real_junction(monkeypatch, tmp_path) -> None: + _patch_paths(monkeypatch, tmp_path / "home") + monkeypatch.setattr(lark_cli, "os", _windows_os_stub()) + config_dir = lark_cli.lark_cli_config_dir("alice") + config_dir.mkdir(parents=True) + outside = tmp_path / "outside" + outside.mkdir() + (outside / "leak.txt").write_text("secret", encoding="utf-8") + + junction = config_dir / "evil" + subprocess.run( + ["cmd", "/c", "mklink", "/J", str(junction), str(outside)], + check=True, + capture_output=True, + text=True, + ) + + dacl_paths: list[str] = [] + _orig_set_security = lark_cli._WindowsTreeHandle.set_security + + def _record_security(self, owner_sid, *, inheritable_full): + dacl_paths.append(str(self.path)) + return _orig_set_security(self, owner_sid, inheritable_full=inheritable_full) + + monkeypatch.setattr(lark_cli._WindowsTreeHandle, "set_security", _record_security) + + try: + with pytest.raises(ValueError, match="reparse"): + lark_cli.ensure_lark_cli_credential_tree("alice") + finally: + # Remove only the junction itself (not its target) so pytest's recursive + # temp cleanup does not hit a WinError on the reparse point. + if junction.exists(): + os.rmdir(junction) + + assert str(junction) not in dacl_paths + assert not any(str(outside) in p for p in dacl_paths) + + +@pytest.mark.skipif(os.name != "nt", reason="requires real Windows ACLs") +def test_windows_credential_tree_hardening_removes_arbitrary_existing_explicit_sid(monkeypatch, tmp_path) -> None: + """A real arbitrary pre-existing explicit SID must not survive hardening. + + Seed an explicit BUILTIN\\Guests (``S-1-5-32-546``) grant, verify it exists, + run the real Windows hardening path (no mock), then verify the unwanted SID + is absent while the current process user's SID remains granted. + """ + _patch_paths(monkeypatch, tmp_path / "home") + config_dir = lark_cli.lark_cli_config_dir("alice") + data_dir = lark_cli.lark_cli_data_dir("alice") + config_dir.mkdir(parents=True) + data_dir.mkdir(parents=True) + secret_file = config_dir / "config.json" + token_file = data_dir / "auth.json" + secret_file.write_text('{"appSecret":"secret"}', encoding="utf-8") + token_file.write_text('{"token":"secret"}', encoding="utf-8") + + unwanted_sid = "S-1-5-32-546" # BUILTIN\Guests + subprocess.run( + ["icacls", str(secret_file), "/grant:r", f"*{unwanted_sid}:F"], + check=True, + capture_output=True, + text=True, + ) + assert unwanted_sid in _windows_acl_sids(secret_file), "seed explicit grant was not applied" + + # Real hardening path: do not mock subprocess here so the actual ACLs change. + lark_cli.ensure_lark_cli_credential_tree("alice") + + owner_sid = lark_cli._resolve_current_user_sid() + result_sids = _windows_acl_sids(secret_file) + assert result_sids == {owner_sid}, "only the owner SID may remain after hardening" + assert _windows_acl_owner_sid(secret_file) == owner_sid, "object owner must be the Gateway user" + + +@pytest.mark.skipif(os.name != "nt", reason="requires real Windows ACLs") +def test_windows_credential_tree_final_security_apply_failure_has_no_prior_broadening(monkeypatch, tmp_path) -> None: + """Failure at the final handle-bound apply seam has no prior /reset or widening step.""" + config_dir, _ = _bootstrap_credential_dirs(monkeypatch, tmp_path, data=False) + secret_file = config_dir / "config.json" + secret_file.write_text('{"appSecret":"secret"}', encoding="utf-8") + + lark_cli.ensure_lark_cli_credential_tree("alice") + owner_sid = lark_cli._resolve_current_user_sid() + before = _windows_acl_sids(secret_file) + assert before == {owner_sid} + assert _windows_acl_protected(secret_file) + + def _boom(handle, owner_sid, *, inheritable_full): + raise OSError("simulated SetSecurityInfo failure") + + monkeypatch.setattr(lark_cli, "_set_windows_security_info_handle", _boom) + with pytest.raises((OSError, RuntimeError)): + lark_cli.ensure_lark_cli_credential_tree("alice") + + after = _windows_acl_sids(secret_file) + assert after == before + assert after == {owner_sid} + assert _windows_acl_owner_sid(secret_file) == owner_sid + assert _windows_acl_protected(secret_file) + + +@pytest.mark.skipif(os.name != "nt", reason="requires real Windows ACLs") +def test_validate_lark_app_credentials_establishes_boundary_before_writing(monkeypatch, tmp_path) -> None: + """The validation temp tree is owner-only before the CLI writes a secret.""" + _patch_paths(monkeypatch, tmp_path / "home") + owner_sid = lark_cli._resolve_current_user_sid() + + def fake_init(*, app_id, app_secret, brand, env): + config_dir = Path(env["LARKSUITE_CLI_CONFIG_DIR"]) + data_dir = Path(env["LARKSUITE_CLI_DATA_DIR"]) + temp_root = config_dir.parent + # The private root is protected; children inherit owner-only. + assert _windows_acl_protected(temp_root) + assert _windows_acl_owner_sid(temp_root) == owner_sid + assert _windows_acl_sids(config_dir) == {owner_sid} + assert _windows_acl_sids(data_dir) == {owner_sid} + secret = data_dir / "auth.json" + secret.write_text('{"token":"secret"}', encoding="utf-8") + assert secret.exists() + assert _windows_acl_sids(secret) == {owner_sid} + + monkeypatch.setattr(lark_cli, "_run_lark_config_init", fake_init) + lark_cli._validate_lark_app_credentials_with_cli(app_id="a", app_secret="s", brand="lark") + + +@pytest.mark.skipif(os.name != "nt", reason="requires real Windows ACLs") +def test_lark_credential_transaction_establishes_boundary_before_copy(monkeypatch, tmp_path) -> None: + """The transaction snapshot tree is owner-only before credentials are copied.""" + _patch_paths(monkeypatch, tmp_path / "home") + owner_sid = lark_cli._resolve_current_user_sid() + root = lark_cli._lark_cli_credential_root("alice") + config_dir = root / "config" + data_dir = root / "data" + config_dir.mkdir(parents=True) + data_dir.mkdir(parents=True) + (config_dir / "config.json").write_text('{"appId":"x"}', encoding="utf-8") + secret = data_dir / "auth.json" + secret.write_text('{"token":"secret"}', encoding="utf-8") + + observed: list[Path] = [] + orig_copytree = lark_cli.shutil.copytree + + def guarded_copytree(*args, **kwargs): + dst = Path(args[1]) + if dst.parent.name == "credentials": + # The snapshot boundary (dst.parent) must already be protected + owner-only + # before any top-level credential directory is copied into it. + assert _windows_acl_protected(dst.parent) + assert _windows_acl_owner_sid(dst.parent) == owner_sid + observed.append(dst) + return orig_copytree(*args, **kwargs) + + monkeypatch.setattr(lark_cli.shutil, "copytree", guarded_copytree) + with lark_cli._lark_credential_transaction("alice", root) as snapshot: + assert _windows_acl_protected(snapshot) + assert _windows_acl_owner_sid(snapshot) == owner_sid + assert (snapshot / "data" / "auth.json").exists() + # Copied children inherit owner-only from the protected snapshot. + assert _windows_acl_sids(snapshot / "data") == {owner_sid} + assert _windows_acl_sids(snapshot / "data" / "auth.json") == {owner_sid} + assert observed + + +@pytest.mark.skipif(os.name != "nt", reason="requires a real NTFS junction") +def test_windows_credential_tree_rejects_reparse_ancestor(monkeypatch, tmp_path) -> None: + """An ancestor junction (e.g. ``integrations``) is rejected before any use.""" + _patch_paths(monkeypatch, tmp_path / "home") + base = tmp_path / "home" + alice = base / "users" / "alice" + alice.mkdir(parents=True) + outside = tmp_path / "outside" + outside.mkdir() + integrations = alice / "integrations" + subprocess.run( + ["cmd", "/c", "mklink", "/J", str(integrations), str(outside)], + check=True, + capture_output=True, + text=True, + ) + try: + with pytest.raises(ValueError, match="reparse"): + lark_cli.ensure_lark_cli_credential_tree("alice") + # The reparse ancestor was rejected before the credential root was used. + assert not (outside / "lark-cli").exists() + assert not (integrations / "lark-cli").exists() + # The hardening lock is anchored under the trusted base_dir, never the external target. + assert not (outside / ".lark-cli.hardening.lock").exists() + finally: + if integrations.exists(): + os.rmdir(integrations) + + +@pytest.mark.skipif(os.name != "nt", reason="requires a real NTFS junction") +def test_public_config_flow_rejects_ancestor_junction_before_credential_lock_write(monkeypatch, tmp_path) -> None: + """A public config entry rejects an ancestor junction before the credential lock writes outside. + + ``start_lark_config`` takes the per-user credential-operation lock, then reaches + ``ensure()`` (via the flow-generation advance), which validates the ancestor chain. The + credential-operation lock must be anchored under the trusted base_dir so it never writes a + lock file beneath an unverified ancestor — otherwise ``outside/.lark-cli.credentials.lock`` + would be created through the junction before ``ensure()`` rejects it. + """ + _patch_paths(monkeypatch, tmp_path / "home") + base = tmp_path / "home" + alice = base / "users" / "alice" + alice.mkdir(parents=True) + outside = tmp_path / "outside" + outside.mkdir() + integrations = alice / "integrations" + subprocess.run( + ["cmd", "/c", "mklink", "/J", str(integrations), str(outside)], + check=True, + capture_output=True, + text=True, + ) + lock_file = outside / ".lark-cli.credentials.lock" + lock_file.write_bytes(b"") # sentinel: detects a write to the old credential-lock path + try: + with pytest.raises(ValueError, match="reparse"): + lark_cli.start_lark_config("alice") + # The junction ancestor was rejected before the credential root was used, and the + # credential-operation lock never opened the external lock file (opening an empty + # a+b file under the old path would have written a b"\0" byte). + assert not (outside / "lark-cli").exists() + assert lock_file.read_bytes() == b"" + finally: + if integrations.exists(): + os.rmdir(integrations) + + +@pytest.mark.skipif(os.name != "nt", reason="requires a real NTFS junction") +def test_windows_credential_tree_swap_blocked_by_exclusive_parent(monkeypatch, tmp_path) -> None: + """An exclusive directory handle blocks a child swap mid-walk. + + The reviewer's P1 is a validated child swapped for a junction before descent. Here + the walker holds ``data`` open *exclusively* (share=0), so an attempt to swap + ``data/nested`` (rename + ``mklink /J`` to an external directory) fails with a + sharing violation — the namespace is locked for the duration of the walk. The + external directory is never reached, opened, or hardened. + """ + config_dir, data_dir = _bootstrap_credential_dirs(monkeypatch, tmp_path) + nested = data_dir / "nested" + nested.mkdir() + (config_dir / "config.json").write_text("s", encoding="utf-8") + (data_dir / "auth.json").write_text("t", encoding="utf-8") + (nested / "inner.txt").write_text("x", encoding="utf-8") + outside = tmp_path / "outside" + outside.mkdir() + (outside / "foreign-secret").write_text("secret", encoding="utf-8") + + hardened: list[str] = [] + orig_set_security = lark_cli._WindowsTreeHandle.set_security + + def _record_security(self, owner_sid, *, inheritable_full): + hardened.append(str(self.path)) + return orig_set_security(self, owner_sid, inheritable_full=inheritable_full) + + monkeypatch.setattr(lark_cli._WindowsTreeHandle, "set_security", _record_security) + + orig_open_child = lark_cli._WindowsTreeHandle.open_child + swapped: list[str] = [] + + def _swap_before_open(self, name): + if self.path == data_dir and name == "nested": + try: + os.rename(nested, nested.with_name("nested-swapped")) + subprocess.run( + ["cmd", "/c", "mklink", "/J", str(nested), str(outside)], + check=True, + capture_output=True, + text=True, + ) + swapped.append("swapped") + except OSError as exc: # noqa: BLE001 - assertion boundary + swapped.append(f"blocked:{type(exc).__name__}") + return orig_open_child(self, name) + + monkeypatch.setattr(lark_cli._WindowsTreeHandle, "open_child", _swap_before_open) + + try: + lark_cli.ensure_lark_cli_credential_tree("alice") + assert swapped and swapped[0].startswith("blocked:"), f"exclusive parent must block the swap, got {swapped}" + assert (outside / "foreign-secret").read_text(encoding="utf-8") == "secret" + assert not any(str(outside) in p for p in hardened), "walker must never harden the external target" + finally: + # Decide from the recorded swap outcome — ``Path.is_symlink()`` is unreliable + # for NTFS junctions, so do not re-derive it here. If the swap succeeded, + # ``nested`` is a junction pointing outside; remove only the junction itself. + if swapped and swapped[0] == "swapped": + if nested.exists(): + os.rmdir(nested) + elif nested.exists(): + shutil.rmtree(nested, ignore_errors=True) + swapped_dir = data_dir / "nested-swapped" + if swapped_dir.exists(): + shutil.rmtree(swapped_dir, ignore_errors=True) + + +@pytest.mark.skipif(os.name != "nt", reason="requires real NTFS hard links") +def test_windows_credential_tree_rejects_hard_linked_file(monkeypatch, tmp_path) -> None: + """A hard-linked file inside the tree must not have its ACL changed. + + The security descriptor belongs to the NTFS file object, so hardening a tree + file that is hard-linked to an external file would also change that external + file's owner/DACL. The walker must reject any file with ``nNumberOfLinks != 1`` + and leave the external file untouched. + """ + config_dir, data_dir = _bootstrap_credential_dirs(monkeypatch, tmp_path) + (config_dir / "config.json").write_text("s", encoding="utf-8") + + outside = tmp_path / "outside" + outside.mkdir() + victim = outside / "victim.txt" + victim.write_text("secret", encoding="utf-8") + planted = data_dir / "planted.txt" + os.link(victim, planted) + + before_sids = _windows_acl_sids(victim) + before_owner = _windows_acl_owner_sid(victim) + + try: + with pytest.raises(ValueError, match="hard-link"): + lark_cli.ensure_lark_cli_credential_tree("alice") + assert _windows_acl_sids(victim) == before_sids + assert _windows_acl_owner_sid(victim) == before_owner + finally: + # Remove only the hard link (not the victim) so pytest's recursive temp + # cleanup does not hit a WinError on the shared file object. + if planted.exists(): + planted.unlink() + + +@pytest.mark.skipif(os.name != "nt", reason="requires real Windows ACLs") +def test_windows_credential_tree_exclusive_parent_no_propagation(monkeypatch, tmp_path) -> None: + """An exclusive directory handle blocks SetSecurityInfo propagation to children.""" + config_dir, _ = _bootstrap_credential_dirs(monkeypatch, tmp_path, data=False) + child = config_dir / "config.json" + child.write_text("s", encoding="utf-8") + + unwanted = "S-1-5-32-546" # BUILTIN\Guests + subprocess.run( + ["icacls", str(child), "/grant:r", f"*{unwanted}:F"], + check=True, + capture_output=True, + text=True, + ) + before = _windows_acl_sids(child) + assert unwanted in before, "seed child grant was not applied" + + owner_sid = lark_cli._resolve_current_user_sid() + handle = lark_cli._open_windows_pinned( + config_dir, + access=lark_cli._WINDOWS_HARDEN_ACCESS, + share=lark_cli._WINDOWS_EXCLUSIVE_SHARE, + ) + try: + # Exclusive handle: applying an inheritable OI|CI DACL must NOT rewrite the child. + handle.set_security(owner_sid, inheritable_full=True) + finally: + handle.close() + + assert _windows_acl_sids(child) == before, "exclusive parent apply must not propagate to existing children" + + +@pytest.mark.skipif(os.name != "nt", reason="requires real NTFS hard links") +def test_windows_credential_tree_late_insertion_blocked_by_exclusive_parent(monkeypatch, tmp_path) -> None: + """An exclusive directory handle closes the concurrent hard-link insertion window. + + During the walk the walker holds ``data`` open exclusively (share=0), so a local + principal cannot ``os.link`` an external file into it (sharing violation) — the late + insertion the children-first order exposed cannot happen. A hard link that already + exists before ``ensure`` is still rejected by the ``link_count != 1`` check (covered + by the static hard-link regression); here we prove the mutation window is closed and + the external file's owner/DACL is left untouched. + """ + config_dir, data_dir = _bootstrap_credential_dirs(monkeypatch, tmp_path) + (config_dir / "config.json").write_text("s", encoding="utf-8") + (data_dir / "auth.json").write_text("t", encoding="utf-8") + + outside = tmp_path / "outside" + outside.mkdir() + victim = outside / "victim.txt" + victim.write_text("secret", encoding="utf-8") + + before_sids = _windows_acl_sids(victim) + before_owner = _windows_acl_owner_sid(victim) + + orig_enumerate = lark_cli._WindowsTreeHandle.enumerate + inserted: list[str] = [] + + def _try_insert_late_link(self): + if self.path == data_dir: + late = data_dir / "late.txt" + try: + if not late.exists(): + os.link(victim, late) + inserted.append("inserted") + except OSError as exc: # noqa: BLE001 - assertion boundary + inserted.append(f"blocked:{type(exc).__name__}") + return orig_enumerate(self) + + monkeypatch.setattr(lark_cli._WindowsTreeHandle, "enumerate", _try_insert_late_link) + + try: + lark_cli.ensure_lark_cli_credential_tree("alice") + assert inserted and inserted[0].startswith("blocked:"), f"exclusive parent must block late insertion, got {inserted}" + assert _windows_acl_sids(victim) == before_sids + assert _windows_acl_owner_sid(victim) == before_owner + finally: + late = data_dir / "late.txt" + if late.exists(): + late.unlink() + + +def test_windows_credential_walker_iterative_handles_deep_tree() -> None: + """The handle-relative walker is iterative, so an unbounded tree depth cannot hit the recursion limit.""" + depth = 1500 + + class _DeepHandle: + def __init__(self, path: Path, remaining: int) -> None: + self.path = path + self.remaining = remaining + self.info = SimpleNamespace(reparse=False, is_dir=True, link_count=1) + + def set_security(self, owner_sid, *, inheritable_full): + assert inheritable_full is True + + def enumerate(self): + if self.remaining > 0: + yield "child" + + def open_child(self, name): + return _DeepHandle(self.path / name, self.remaining - 1) + + def open_or_create_child_dir(self, name): + return _DeepHandle(self.path / name, self.remaining - 1) + + def close(self): + pass + + def __enter__(self): + return self + + def __exit__(self, *_exc): + return False + + # Must not raise RecursionError even though the chain is far deeper than the default limit. + lark_cli._walk_and_harden_windows_handle(Path("root"), _DeepHandle(Path("root"), depth), "S-1-5-21-1", Path("root")) + + +@pytest.mark.skipif(os.name != "nt", reason="requires real exclusive-share semantics") +def test_concurrent_ensure_serialized_by_hardening_lock(monkeypatch, tmp_path) -> None: + """Concurrent ``ensure()`` on the same user serializes (no ERROR_SHARING_VIOLATION). + + The walker opens credential directories exclusively (share=0). Without serialization two + concurrent ``ensure()`` calls would race: the second would fail to open the already-exclusive + root. The per-user hardening lock must make the second caller wait and then succeed. + """ + _bootstrap_credential_dirs(monkeypatch, tmp_path) + started = threading.Event() + release = threading.Event() + orig = lark_cli._set_windows_security_info_handle + + def _pause_first(handle, owner_sid, *, inheritable_full): + if not started.is_set(): + started.set() + release.wait(timeout=10) + return orig(handle, owner_sid, inheritable_full=inheritable_full) + + monkeypatch.setattr(lark_cli, "_set_windows_security_info_handle", _pause_first) + + results: list[str] = [] + + def _worker(): + try: + lark_cli.ensure_lark_cli_credential_tree("alice") + results.append("ok") + except OSError as exc: + results.append(f"err:{type(exc).__name__}:{getattr(exc, 'winerror', None)}") + + thread_a = threading.Thread(target=_worker) + thread_a.start() + assert started.wait(timeout=10), "thread A did not reach the root hardening step" + thread_b = threading.Thread(target=_worker) + thread_b.start() + time.sleep(0.2) # give thread B a chance to race for the exclusive root + release.set() + thread_a.join(timeout=20) + thread_b.join(timeout=20) + + assert results == ["ok", "ok"], f"concurrent ensure must serialize, got {results}" + + def test_lark_cli_env_rejects_symlinks_in_credential_tree(monkeypatch, tmp_path) -> None: _patch_paths(monkeypatch, tmp_path / "home") config_dir = lark_cli.lark_cli_config_dir("alice") @@ -1090,34 +1961,88 @@ def test_lark_cli_env_rejects_symlinks_in_credential_tree(monkeypatch, tmp_path) except (NotImplementedError, OSError) as exc: pytest.skip(f"symlinks are not available: {exc}") - with pytest.raises(ValueError, match="symlink"): + # POSIX reports a symlink; the Windows walker reports a reparse point. + expected_error = "reparse" if os.name == "nt" else "symlink" + with pytest.raises(ValueError, match=expected_error): lark_cli.lark_cli_env_overlay("alice") +@pytest.mark.skipif(os.name == "nt", reason="POSIX root symlink rejection before mkdir") +def test_ensure_posix_rejects_symlink_root_before_creating_children(monkeypatch, tmp_path) -> None: + """A symlinked credential root is rejected before any child dir is created (POSIX). + + ``mkdir(exist_ok=True)`` accepts a symlink that resolves to a directory, so a + symlinked ``lark-cli`` root must be rejected up front — otherwise ``config`` / + ``data`` would be created inside the symlink target (outside the credential tree) + before the walker notices the reparse. + """ + _patch_paths(monkeypatch, tmp_path / "home") + root = lark_cli._lark_cli_credential_root("alice") + root.parent.mkdir(parents=True) + outside = tmp_path / "outside" + outside.mkdir() + try: + root.symlink_to(outside) + except (NotImplementedError, OSError) as exc: + pytest.skip(f"symlinks are not available: {exc}") + + with pytest.raises(ValueError, match="symlink"): + lark_cli.ensure_lark_cli_credential_tree("alice") + + # The root symlink must have been rejected before any child was created inside it. + assert not (outside / "config").exists() + assert not (outside / "data").exists() + + def test_save_lark_app_config_rehardens_files_written_by_cli(monkeypatch, tmp_path) -> None: _patch_paths(monkeypatch, tmp_path / "home") - monkeypatch.setattr(lark_cli, "_require_lark_cli_path", lambda: "/usr/bin/lark-cli") + rehardened: list[str] = [] + orig_ensure = lark_cli.ensure_lark_cli_credential_tree - def _run(args, **kwargs): - config_file = Path(kwargs["env"]["LARKSUITE_CLI_CONFIG_DIR"]) / "config.json" + def _spy_ensure(user_id, *, paths=None): + rehardened.append(user_id) + return orig_ensure(user_id, paths=paths) + + monkeypatch.setattr(lark_cli, "ensure_lark_cli_credential_tree", _spy_ensure) + + def _run_init(*, app_id, app_secret, brand, env): + config_file = Path(env["LARKSUITE_CLI_CONFIG_DIR"]) / "config.json" config_file.write_text('{"appSecret":"secret"}', encoding="utf-8") - config_file.chmod(0o644) - return subprocess.CompletedProcess(args=args, returncode=0, stdout="", stderr="") + if os.name != "nt": + config_file.chmod(0o644) # simulate a permissive CLI-written file - monkeypatch.setattr(lark_cli.subprocess, "run", _run) + monkeypatch.setattr(lark_cli, "_run_lark_config_init", _run_init) lark_cli._save_lark_app_config_with_cli("alice", app_id="cli_app", app_secret="secret", brand="feishu") config_file = lark_cli.lark_cli_config_dir("alice") / "config.json" - assert stat.S_IMODE(config_file.stat().st_mode) == 0o600 + assert config_file.exists() + assert "alice" in rehardened, "a CLI-written config file must be re-hardened" + if os.name != "nt": + assert stat.S_IMODE(config_file.stat().st_mode) == 0o600, "POSIX re-harden must tighten the file mode" def test_validate_lark_app_credentials_surfaces_cli_probe_rejection(monkeypatch, tmp_path) -> None: _patch_paths(monkeypatch, tmp_path / "home") + + def _reject(*, app_id, app_secret, brand, env): + raise ValueError("The specified app does not exist.") + + monkeypatch.setattr(lark_cli, "_run_lark_config_init", _reject) + + with pytest.raises(ValueError, match="specified app does not exist"): + lark_cli._validate_lark_app_credentials_with_cli( + app_id="cli_invalid", + app_secret="invalid-secret", + brand="feishu", + ) + + +def test_run_lark_config_init_surfaces_cli_probe_rejection(monkeypatch) -> None: + """The config-init CLI rejection is surfaced as a ValueError carrying the CLI message.""" monkeypatch.setattr(lark_cli, "_require_lark_cli_path", lambda: "/usr/bin/lark-cli") def _run(args, **kwargs): - assert kwargs["env"]["LARKSUITE_CLI_CONFIG_DIR"] != str(lark_cli.lark_cli_config_dir("alice")) return subprocess.CompletedProcess( args=args, returncode=3, @@ -1128,20 +2053,46 @@ def test_validate_lark_app_credentials_surfaces_cli_probe_rejection(monkeypatch, monkeypatch.setattr(lark_cli.subprocess, "run", _run) with pytest.raises(ValueError, match="specified app does not exist"): - lark_cli._validate_lark_app_credentials_with_cli( + lark_cli._run_lark_config_init( app_id="cli_invalid", app_secret="invalid-secret", brand="feishu", + env={"LARKSUITE_CLI_CONFIG_DIR": "/x", "LARKSUITE_CLI_DATA_DIR": "/y"}, ) def test_lark_cli_json_rehardens_auth_files_written_by_cli(monkeypatch, tmp_path) -> None: _patch_paths(monkeypatch, tmp_path / "home") + rehardened: list[str] = [] + orig_ensure = lark_cli.ensure_lark_cli_credential_tree + + def _spy_ensure(user_id, *, paths=None): + # Record-only: this is an orchestration contract (the CLI writes files then + # re-hardens). On POSIX run the real walker so the tightened mode is observable; + # on Windows the native ACL regressions cover the real behavior (and the identity + # probe must not be intercepted by the CLI subprocess fake). + rehardened.append(user_id) + if os.name != "nt": + return orig_ensure(user_id, paths=paths) + + monkeypatch.setattr(lark_cli, "ensure_lark_cli_credential_tree", _spy_ensure) + # The env is built without the credential subsystem so the identity probe + # (whoami) does not get intercepted by the CLI subprocess fake. + monkeypatch.setattr( + lark_cli, + "lark_cli_env", + lambda user_id: { + "LARKSUITE_CLI_CONFIG_DIR": str(lark_cli.lark_cli_config_dir(user_id)), + "LARKSUITE_CLI_DATA_DIR": str(lark_cli.lark_cli_data_dir(user_id)), + }, + ) def _run(args, **kwargs): token_file = Path(kwargs["env"]["LARKSUITE_CLI_DATA_DIR"]) / "auth.json" + token_file.parent.mkdir(parents=True, exist_ok=True) token_file.write_text('{"token":"secret"}', encoding="utf-8") - token_file.chmod(0o644) + if os.name != "nt": + token_file.chmod(0o644) # simulate a permissive CLI-written file return subprocess.CompletedProcess(args=args, returncode=0, stdout="{}", stderr="") monkeypatch.setattr(lark_cli.subprocess, "run", _run) @@ -1149,7 +2100,10 @@ def test_lark_cli_json_rehardens_auth_files_written_by_cli(monkeypatch, tmp_path lark_cli._run_lark_cli_json(["/usr/bin/lark-cli", "auth", "login"], user_id="alice", timeout=5) token_file = lark_cli.lark_cli_data_dir("alice") / "auth.json" - assert stat.S_IMODE(token_file.stat().st_mode) == 0o600 + assert token_file.exists() + assert "alice" in rehardened, "a CLI-written auth file must be re-hardened" + if os.name != "nt": + assert stat.S_IMODE(token_file.stat().st_mode) == 0o600, "POSIX re-harden must tighten the file mode" def test_lark_cli_env_from_runtime_uses_container_paths_for_sandbox_lark_commands(): @@ -1172,29 +2126,26 @@ def test_lark_cli_env_from_runtime_ignores_non_lark_commands(tmp_path, monkeypat def test_lark_auth_probe_distinguishes_local_configuration_from_live_verification(monkeypatch, tmp_path) -> None: assert "verified" in lark_cli.LarkAuthProbe.__dataclass_fields__ _patch_paths(monkeypatch, tmp_path / "home") - config_file = lark_cli.lark_cli_config_dir("alice") / "config.json" - config_file.parent.mkdir(parents=True) - config_file.write_text( - json.dumps( - { - "currentApp": "cli_app", - "apps": [ - { - "name": "cli_app", - "appId": "cli_app", - "appSecret": "secret", - "brand": "feishu", - } - ], - } - ), - encoding="utf-8", - ) calls: list[list[str]] = [] monkeypatch.setattr(lark_cli, "_resolve_lark_cli_path", lambda: "/usr/bin/lark-cli") + monkeypatch.setattr( + lark_cli, + "read_lark_app_config", + lambda _user_id: {"configured": True, "app_id": "cli_app", "brand": "feishu"}, + ) + # Build env without the credential subsystem so the identity probe (whoami) + # does not get intercepted by the CLI subprocess fake. + monkeypatch.setattr( + lark_cli, + "lark_cli_env", + lambda user_id: { + "LARKSUITE_CLI_CONFIG_DIR": str(lark_cli.lark_cli_config_dir(user_id)), + "LARKSUITE_CLI_DATA_DIR": str(lark_cli.lark_cli_data_dir(user_id)), + }, + ) - def _run(args, **_kwargs): + def _run(args, **kwargs): calls.append(args) return subprocess.CompletedProcess( args=args,