From 27ba79b973f0a458e6d303ceeb73df06eeddf12c Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sat, 17 Oct 2026 15:39:27 +0000 Subject: [PATCH 1/4] adopt-project.py: the clone `plan` and `check` make is claimed with this run's own `mkdir` and removed only while it is still that directory, and a `--work-dir` or `source-readonly/` another user can rename in is refused first, so a swapped `source-readonly` no longer turns the removal onto another directory, two runs on one `--work-dir` no longer remove each other's clone, and a skipped removal is named (#803) On main, #749's removal asked the clone's real path and then removed the path, in two steps. `source-readonly`, swapped for a symlink between them, removed the directory of that name wherever the link led (#457's L1-99 (b)). A skipped removal said nothing (L1-99 (a)). Two runs given one `--work-dir` could both list one target, and the run whose clone failed removed the other's (L1-98). `check --source` removed a clone it had made at the plan's own `source.local_path` (L1-97). The paths were compared without `os.path.normcase` (L1-102). A file at `source-readonly` was a traceback (L1-65). Now: - `_work_root` refuses a `--work-dir` that another user owns, or that every user can write in without the sticky bit, before anything is made in it, as the new `adopt-work-dir-shared`. It asks with `os.lstat`, and never on Windows. - `Source.clone` asks `source-readonly/` before the NOTE. A symlink or a file is `adopt-work-dir-occupied`, one another user can rename in is `adopt-work-dir-shared`, and one this user cannot write in is `adopt-work-dir-unwritable`. A failed `mkdir` there is refused by name. - The clone's directory is claimed with an exclusive `mkdir` (0755) before `gh` runs, and any target already there, an empty one included, is refused. Only that claim is listed, with the identity (`st_dev`, `st_ino`) of the directory and of `source-readonly/`. - `_remove_a_tree` removes only while both identities still hold, never through a symlink. From Python 3.11 on Linux and macOS it removes relative to a descriptor of `source-readonly/` (`shutil.rmtree(dir_fd=)`). On 3.9, 3.10 and Windows it removes by path after the same checks. - Every skipped removal is named in a NOTE, and paths are compared with `os.path.normcase` on both sides. - `check --source` keeps the clone it made at the plan's own record. - `execute`, whose clone fails, removes the directory its own `mkdir` made, after the same checks, so a re-run is not refused at an empty directory. Tests: tests/test_adopt_clone_claimed.py is new, with 22 cases. Two of #749's tests change: the symlinked `source-readonly` is now refused before the clone, and the read-only unlink emulation also emulates Windows's removal by path. The help's code table names the new code, and docs/cli.md is rendered again. Lane: openreposhape-1 (openRepoShape-1) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 544 ++++++++++++++++++---- docs/cli.md | 27 +- tests/test_adopt_clone_claimed.py | 574 ++++++++++++++++++++++++ tests/test_adopt_source_clone_record.py | 19 +- tests/test_adopt_work_dir_refusal.py | 8 +- 5 files changed, 1080 insertions(+), 92 deletions(-) create mode 100644 tests/test_adopt_clone_claimed.py diff --git a/adopt-project.py b/adopt-project.py index 6e56e5df..ee4e7397 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -174,6 +174,7 @@ import argparse import contextlib import datetime as _dt +import errno import itertools import os import re @@ -184,6 +185,7 @@ import sys import tempfile from pathlib import Path, PurePosixPath +from typing import NamedTuple SHAPE_ROOT = Path(__file__).resolve().parent sys.path.insert(0, str(SHAPE_ROOT / "scripts")) @@ -1692,17 +1694,21 @@ def _occupied_clone_target(work_root: Path, path: Path, repository: str, first in the way out as its other early refusals say it; `execute` keeps the clone it makes, so the way out says that too.""" held = path.relative_to(work_root).as_posix() + # An empty directory is refused too, since #803: the run that made it + # may be cloning into it now (#457's L1-98). + empty = "" if _holds_something(path) else "an empty directory, " + why = ("a run clones only into a directory it has just made, so that it " + "never takes another run's for its own" if empty else + "`git clone` clones only into a directory that is new or empty") detail = (f"--work-dir {_spelled(str(work_root))} already holds " - f"{_spelled(held)}, where this run clones {repository}, and " - "`git clone` clones only into a directory that is new or empty") - another = ("a --work-dir that does not hold it, or none: without one, " - "each run makes a new one.") - remedy = f"Pass {another}" + f"{_spelled(held)}, {empty}where this run clones {repository}, " + f"and {why}") + remedy = f"Pass {ANOTHER_WORK_DIR}" if taken: detail = f"{taken}; and {detail}" remedy = (f"It is no longer a whole clone: delete {_spelled(held)} " "and run the command again, which clones it there anew; " - f"or pass {another}") + f"or pass {ANOTHER_WORK_DIR}") if done: remedy += (" An earlier `execute` given this --work-dir keeps the " "clone it made there: where that is what it holds, delete " @@ -1711,6 +1717,214 @@ def _occupied_clone_target(work_root: Path, path: Path, repository: str, f"Remediation: {done}{remedy}") +#: The refusal of a `--work-dir`, or of the `source-readonly/` in it, that +#: another user can rename or replace entries in (#803). +WORK_DIR_SHARED = "adopt-work-dir-shared" +#: The way out of a `--work-dir` that holds something in the clone's way. +ANOTHER_WORK_DIR = ("a --work-dir that does not hold it, or none: without " + "one, each run makes a new one.") +#: The way out of a `--work-dir` another user can rename entries in. +WORK_DIR_OWN = ("Pass a --work-dir this user owns and no other user can write " + "in, or none: without one, each run makes a new one.") +#: Whether `_remove_a_tree` removes a clone relative to a descriptor of the +#: `source-readonly/` it was made in, never through its path (#803): what +#: `shutil.rmtree(dir_fd=)` does from Python 3.11, where this platform's +#: `rmtree` walks by descriptor (`avoids_symlink_attacks`): Linux and macOS, +#: never Windows. +FD_REMOVAL = (sys.version_info >= (3, 11) + and shutil.rmtree.avoids_symlink_attacks + and hasattr(os, "O_DIRECTORY") and hasattr(os, "O_NOFOLLOW")) +#: Why a clone this run made is not removed where its path leads to another +#: directory now (`_NotTheClone`). +NOT_THE_CLONE = ("what that path leads to now is not the directory this run " + "made, so nothing is removed through it") + + +class _Claim(NamedTuple): + """The directory `Source.clone` made for a clone, with an exclusive + `mkdir`, and what `_remove_a_tree` holds its removal to (#803): its + path as the run spells it; its real path and its identity (`st_dev`, + `st_ino`), read right after the `mkdir`; the identity of the + `source-readonly/` it is in; and whether this run made that too.""" + path: Path + made_at: str + identity: tuple + parent_identity: tuple + made_parent: bool + + +class _NotTheClone(Exception): + """A clone's path that no longer leads to the directory this run made, + and why, in words (#803). Nothing is removed through it.""" + + +def _identity(status: os.stat_result) -> tuple: + return status.st_dev, status.st_ino + + +def _same_spelling(path: str, other: str) -> bool: + """Whether two real paths name one entry as this platform compares + names: `os.path.normcase` on both, so that on Windows the disk's case, + `Source-Readonly` as `ntpath.realpath` returns it, is the recorded + `source-readonly` (#803, #457's L1-102).""" + return os.path.normcase(path) == os.path.normcase(other) + + +def _owner(uid: int) -> str: + """The user `uid` names, in backticks, or the id where none does. POSIX + only, as `_shared_with_another_user` asks it.""" + import pwd + try: + return _spelled(pwd.getpwuid(uid).pw_name) + except KeyError: + return f"user id {uid}" + + +def _shared_with_another_user(status: os.stat_result) -> str | None: + """Why another user can rename or replace what is in the directory + `status` describes, an `os.lstat`, in words; None where nobody else can + (#803). Another user owns it; or every user can write in it, and its + sticky bit, which keeps each entry to its owner as in `/tmp`, is not + set. GROUP WRITE IS NOT ASKED: under `umask 002` every directory a user + makes is group-writable, for a group of that user alone. NOR IS ANYTHING + ON WINDOWS, where neither the owner nor the mode bits say it.""" + if not hasattr(os, "geteuid"): + return None + if status.st_uid != os.geteuid(): + return f"is owned by {_owner(status.st_uid)}, not by this user" + if status.st_mode & stat.S_IWOTH and not status.st_mode & stat.S_ISVTX: + return "can be written in by every user, without the sticky bit" + return None + + +def _shared_work_dir(detail: str, done: str, remedy: str) -> Refusal: + """`adopt-work-dir-shared` for `detail`; `done` is said first in the way + out, as `_work_dir_refusal` says it.""" + return Refusal(WORK_DIR_SHARED, detail, f"Remediation: {done}{remedy}") + + +def _what_is_there(held: Path, status: os.stat_result, leads: str) -> str: + """What is at `held`, a `source-readonly` that is no directory of the + work directory's own, in words: a symlink and what it holds, a file, or + a directory that leads elsewhere, as a Windows junction does.""" + if stat.S_ISLNK(status.st_mode): + with contextlib.suppress(OSError): + return f"a symlink to {_spelled(os.readlink(held))}" + return "a symlink" + if stat.S_ISREG(status.st_mode): + return "a file" + if stat.S_ISDIR(status.st_mode): + return f"a directory that leads to {_spelled(leads)}" + return "neither a file nor a directory" + + +def _source_readonly_refusal(work_root: Path, repository: str, + done: str) -> Refusal | None: + """Why `/source-readonly` cannot hold the clone of + `repository`, as the refusal; None where it can, or is not there yet + (#803). Asked before the NOTE, and before anything is made there. + + It must be a directory of the work directory's own: no symlink, which + leads the clone and its removal elsewhere (#457's L1-99), and no file, + which ended each command in a `FileExistsError` traceback (L1-65), both + `adopt-work-dir-occupied`. No other user may be able to rename in it + (`_shared_with_another_user`): in a work directory every user can write + in, the user who made it first owns the clone's parent, and could turn + the clone, or its removal, onto another directory. That is + `adopt-work-dir-shared`. And this user must be able to make the clone's + directory in it, or it is `adopt-work-dir-unwritable`.""" + held = work_root / SOURCE_READONLY + shown = _spelled(str(work_root)) + try: + status = os.lstat(held) + except FileNotFoundError: + return None + except OSError as exc: + return _work_dir_refusal( + f"--work-dir {shown} cannot hold `{SOURCE_READONLY}`, where this " + f"run clones {repository}: reading it failed " + f"({exc.strerror or exc})", done) + leads = os.path.realpath(held) + if not stat.S_ISDIR(status.st_mode) or not _same_spelling( + leads, os.path.join(os.path.realpath(work_root), SOURCE_READONLY)): + return Refusal( + "adopt-work-dir-occupied", + f"--work-dir {shown} already holds `{SOURCE_READONLY}`, " + f"{_what_is_there(held, status, leads)}, where this run clones " + f"{repository} into a directory of its own", + f"Remediation: {done}Pass {ANOTHER_WORK_DIR}") + shared = _shared_with_another_user(status) + if shared: + return _shared_work_dir( + f"--work-dir {shown} holds `{SOURCE_READONLY}`, which {shared}, " + "so another user could rename or replace the clone this run " + "makes in it", done, f"Pass {ANOTHER_WORK_DIR}") + if not os.access(held, os.W_OK | os.X_OK): + return _work_dir_refusal( + f"--work-dir {shown} holds `{SOURCE_READONLY}`, where this run " + f"clones {repository}, and this user cannot make a directory in " + "it", done) + return None + + +def _made_source_readonly(work_root: Path, parent: Path, repository: str, + done: str) -> bool: + """Make `parent`, `source-readonly/`, where nothing is there, and say + whether this run made it (#803). One another run made meanwhile is + asked as `_source_readonly_refusal` asks it, and any other failure is + `adopt-work-dir-unwritable`, by name, where it was a traceback (#457's + L1-65). Made 0755 whatever the umask: under `umask 0` it would be one + every user can rename in.""" + if os.path.lexists(parent): + return False + try: + os.mkdir(parent, 0o755) + return True + except FileExistsError: + refused = _source_readonly_refusal(work_root, repository, done) + except OSError as exc: + refused = _work_dir_refusal( + f"--work-dir {_spelled(str(work_root))} cannot hold " + f"`{SOURCE_READONLY}`, where this run clones {repository}: " + f"making it failed ({exc.strerror or exc})", done) + if refused: + raise refused + return False + + +def _claimed(work_root: Path, path: Path, repository: str, + done: str) -> _Claim: + """`path`, the directory `repository` is cloned into, made by THIS run + with an exclusive `mkdir` before `gh` runs (`git clone` clones into an + empty directory), and `source-readonly/` above it where it is not there: + the claim `_remove_a_tree` holds a removal to (#803). + + ON MAIN THE TARGET WAS LISTED WHERE IT DID NOT EXIST YET, and made by + `git clone`. So two runs given one `--work-dir` could both list it: the + one whose clone failed removed the other's, which that run's plan + recorded (#457's L1-98). Now the `mkdir` decides which run it is, and + one made meanwhile by another run is refused as + `adopt-work-dir-occupied`. Made 0755 whatever the umask.""" + made_parent = _made_source_readonly(work_root, path.parent, repository, + done) + try: + os.mkdir(path, 0o755) + except OSError as exc: + if made_parent: + with contextlib.suppress(OSError): + os.rmdir(path.parent) + if isinstance(exc, FileExistsError): + raise _occupied_clone_target(work_root, path, repository, + done=done) from None + raise _work_dir_refusal( + f"--work-dir {_spelled(str(work_root))} holds " + f"`{SOURCE_READONLY}`, where this run clones {repository}, and " + f"making `{path.name}` in it failed ({exc.strerror or exc})", + done) from None + return _Claim(path, os.path.realpath(path), _identity(os.lstat(path)), + _identity(os.lstat(path.parent)), made_parent) + + def _not_a_whole_clone(path: Path, commit: str) -> str | None: """Why the clone `plan` made at `path`, which exists, is no longer one the plan's source can be read from, in words; None where it is (#652). @@ -1790,8 +2004,9 @@ def clone(cls, repository: str, work_root: Path, (`_unwritable_work_dir`), or None. It is refused as `adopt-work-dir-unwritable` before any directory is made or any clone is said (#650): `plan` ended in a PermissionError traceback at - `source-readonly`. Then a target that already holds something is - refused as `adopt-work-dir-occupied` (#652): `git clone` refuses it, + `source-readonly`. Then a target that is already there is refused as + `adopt-work-dir-occupied` (#652, #803): `git clone` refuses one that + holds something, and the refusal was `source-unclonable` with a remedy about `gh auth status`. `taken` is what that refusal says first, when the occupant is the plan's own clone (`Plan.open_source`), and `done` what @@ -1799,29 +2014,39 @@ def clone(cls, repository: str, work_root: Path, that says why the plan's clone is made again, printed. `made` is the list a command keeps of the clones it made, for - `_leave_the_clones`: the target is put on it before `gh` runs, where - it did not exist, with the REAL path it has now, which is the only - path `_leave_the_clones` will remove, and whether this run made - `source-readonly/` too.""" + `_leave_the_clones`. THE TARGET IS CLAIMED BEFORE `gh` RUNS (#803): + `source-readonly/` must be a directory of the work directory's own + that no other user can rename in (`_source_readonly_refusal`), asked + with the writable check and before the NOTE; ANY target already + there is refused, an empty one included; and this run then makes it + with an exclusive `mkdir` (`_claimed`). Only that `_Claim` goes on + `made`, and `_remove_a_tree` removes only the directory it names. + `execute` lists none and keeps its clone, but where `gh` fails it + removes the directory its own `mkdir` made, after the same checks, + so that the run given the same --work-dir again is not refused at + an empty directory.""" path = work_root / SOURCE_READONLY / repository.split("/")[1] if unwritable: raise _work_dir_refusal(unwritable) - if _holds_something(path): + refused = _source_readonly_refusal(work_root, repository, done) + if refused: + raise refused + if os.path.lexists(path): raise _occupied_clone_target(work_root, path, repository, taken, done) if note: print(note) - made_parent = not os.path.lexists(path.parent) - if made is not None and not os.path.lexists(path): - made.append((path, os.path.join(os.path.realpath(work_root), - SOURCE_READONLY, path.name), - made_parent)) - path.parent.mkdir(parents=True, exist_ok=True) + claim = _claimed(work_root, path, repository, done) + if made is not None: + made.append(claim) print(f"cloning {repository} (read only) into {path}") try: run(["gh", "repo", "clone", repository, str(path), "--", "--quiet"]) except CommandFailed as exc: + if made is None: + with contextlib.suppress(OSError, _NotTheClone): + _remove_a_tree(claim) raise Refusal( "source-unclonable", f"could not clone {repository}: {exc.output.strip()[:400]}", @@ -4635,7 +4860,8 @@ def open_source(self, override: str | None, work_root: Path, existed, said the plan's repository is cloned again and read that directory instead.""" if override: - return Source.open(override, work_root, unwritable, made, done) + return self._kept_at_the_record( + Source.open(override, work_root, unwritable, made, done), made) local_path = self.source.get("local_path") if local_path: return self._recorded_source(str(local_path), work_root, @@ -4648,6 +4874,23 @@ def open_source(self, override: str | None, work_root: Path, return Source.clone(repository, work_root, unwritable, made, done=done) + def _kept_at_the_record(self, source: Source, + made: list | None) -> Source: + """`source`, read from `--source`, with the clone this run made of it + taken off `made` where it is at the plan's own `source.local_path` + (#803, #457's L1-97): it is the plan's source again, as the clone + `_recorded_source` makes there is. On main `check` removed it, and + the next run cloned it once more.""" + record = self.source.get("local_path") + if made and record: + try: + at = os.path.realpath(os.path.expanduser(str(record))) + except (OSError, ValueError): + return source + made[:] = [claim for claim in made + if not _same_spelling(claim.made_at, at)] + return source + def _recorded_repository(self) -> str | None: """The plan's `source.repository` as `_github_name` reads it, or None where it records none. One that names no `owner/repo` is refused, by @@ -12957,7 +13200,15 @@ def _work_root(args, done: str = "") -> Path: 1, and on a loop Python 3.12's `Path.resolve` raised RuntimeError first; `os.path.realpath` is the same reading without that raise. The code and the way out are #524's, for one this user cannot write in; - `done` is what the refusing command left as it was, said first.""" + `done` is what the refusing command left as it was, said first. + + A `--work-dir` ANOTHER USER CAN RENAME IN IS REFUSED NEXT, before + anything is made in it (#803): one another user owns, or one every user + can write in without the sticky bit (`_shared_with_another_user`), as + `adopt-work-dir-shared`. There another user could replace the clone + `plan` reads, or turn the clone's removal onto a directory of theirs + (#457's L1-99), or the legs `execute` builds. Asked with `os.lstat` of + the real path; never on Windows.""" if not getattr(args, "work_dir", None): root = Path(tempfile.mkdtemp(prefix="openreposhape-adopt-")) args.made_work_root = root @@ -12965,10 +13216,17 @@ def _work_root(args, done: str = "") -> Path: root = Path(os.path.realpath(args.work_dir)) try: root.mkdir(parents=True, exist_ok=True) + status = os.lstat(root) except OSError as exc: raise _work_dir_refusal( f"--work-dir {_spelled(str(root))} cannot be made: making it a " f"directory failed ({exc.strerror or exc})", done) from None + shared = _shared_with_another_user(status) + if shared: + raise _shared_work_dir( + f"--work-dir {_spelled(str(root))} {shared}, so another user " + "could rename or replace what this run makes in it", done, + WORK_DIR_OWN) return root @@ -13011,33 +13269,161 @@ def _removed_if_it_holds_no_file(root: Path) -> bool: return True -def _remove_a_tree(path: Path) -> None: - """`shutil.rmtree` of `path`. What git wrote READ-ONLY, a clone's packs - and loose objects, Windows refuses to unlink: a file or directory whose - unlink or rmdir fails gets its write bit ADDED, and is removed again. - Every other failure is raised as it was, and no mode is ever lowered - (#652's review, F4): the handler once set the mode to exactly - `S_IWRITE`, taking read and search away, and called whatever function - failed, `os.open` included, which ended in a TypeError traceback. A - symlink is never changed, since `chmod` would follow it. The handler's - keyword is `onexc` from Python 3.12, whose `onerror` warns.""" - def writable_then_removed(remove, target, error): - if remove not in (os.unlink, os.remove, os.rmdir) \ - or os.path.islink(target): - raise error if isinstance(error, BaseException) else error[1] - os.chmod(target, stat.S_IMODE(os.lstat(target).st_mode) - | stat.S_IWRITE) - remove(target) - - keyword = "onexc" if sys.version_info >= (3, 12) else "onerror" - shutil.rmtree(path, **{keyword: writable_then_removed}) +def _writable_then_removed(remove, target, error) -> None: + """`shutil.rmtree`'s handler where a clone is removed by path. What git + wrote READ-ONLY, a clone's packs and loose objects, Windows refuses to + unlink: a file or directory whose unlink or rmdir fails gets its write + bit ADDED, and is removed again. Every other failure is raised as it + was, and no mode is ever lowered (#652's review, F4): the handler once + set the mode to exactly `S_IWRITE`, taking read and search away, and + called whatever function failed, `os.open` included, which ended in a + TypeError traceback. A symlink is never changed, since `chmod` would + follow it.""" + if remove not in (os.unlink, os.remove, os.rmdir) \ + or os.path.islink(target): + raise error if isinstance(error, BaseException) else error[1] + os.chmod(target, stat.S_IMODE(os.lstat(target).st_mode) | stat.S_IWRITE) + remove(target) + + +#: The keyword `_writable_then_removed` is given as: `onexc` from Python +#: 3.12, whose `onerror` warns. +REMOVAL_HANDLER = "onexc" if sys.version_info >= (3, 12) else "onerror" + + +def _opened_directory(path: str) -> int | None: + """A descriptor of the directory at `path`, opened without following a + symlink there; None where nothing is there. A symlink or anything else + that is no directory is `_NotTheClone`.""" + try: + return os.open(path, os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW) + except FileNotFoundError: + return None + except OSError as exc: + if exc.errno in (errno.ELOOP, errno.ENOTDIR): + raise _NotTheClone(NOT_THE_CLONE) from None + raise + + +def _status_at(where: str, dir_fd: int | None) -> os.stat_result | None: + """`os.lstat` of `where`, in the directory `dir_fd` where one is given; + None where nothing is there. A path through something that is no + directory is `_NotTheClone`.""" + try: + return os.lstat(where, dir_fd=dir_fd) + except FileNotFoundError: + return None + except NotADirectoryError: + raise _NotTheClone(NOT_THE_CLONE) from None + + +def _held_to(status: os.stat_result, identity: tuple) -> None: + """`_NotTheClone` where `status`, an `lstat`, is not the directory + `identity` names: a symlink never is.""" + if not stat.S_ISDIR(status.st_mode) or _identity(status) != identity: + raise _NotTheClone(NOT_THE_CLONE) + + +def _removed_if_still(parent: str, identity: tuple) -> None: + """`os.rmdir` of `source-readonly/`, made by this run, where it is still + that directory and empty; and silently not, as on main (#652).""" + above, name = os.path.split(parent) + with contextlib.suppress(OSError, _NotTheClone): + held = _opened_directory(above) if FD_REMOVAL else None + where = name if held is not None else parent + try: + status = _status_at(where, held) + if status is not None: + _held_to(status, identity) + os.rmdir(where, dir_fd=held) + finally: + if held is not None: + os.close(held) + + +def _remove_through_a_descriptor(claim: _Claim) -> None: + """`_remove_a_tree` where `FD_REMOVAL` holds: `source-readonly/` opened + once, never through a symlink, and held to its identity; the clone's + directory asked and removed relative to that descriptor alone.""" + parent, name = os.path.split(claim.made_at) + held = _opened_directory(parent) + if held is None: + return + try: + _held_to(os.fstat(held), claim.parent_identity) + status = _status_at(name, held) + if status is not None: + _held_to(status, claim.identity) + shutil.rmtree(name, dir_fd=held) + finally: + os.close(held) + if claim.made_parent: + _removed_if_still(parent, claim.parent_identity) + + +def _remove_by_path(claim: _Claim) -> None: + """`_remove_a_tree` where no removal by descriptor is had: Python 3.9 + and 3.10, and Windows. The same checks, each with `os.lstat`, then + `shutil.rmtree` of the path, with `_writable_then_removed`.""" + parent = os.path.dirname(claim.made_at) + status = _status_at(parent, None) + if status is not None: + _held_to(status, claim.parent_identity) + status = _status_at(claim.made_at, None) + if status is not None: + _held_to(status, claim.identity) + shutil.rmtree(claim.made_at, + **{REMOVAL_HANDLER: _writable_then_removed}) + if claim.made_parent: + _removed_if_still(parent, claim.parent_identity) + + +def _remove_a_tree(claim: _Claim) -> None: + """Remove the directory `claim` names, and `source-readonly/` where + this run made that too, each only where it is still the directory this + run made (#803). Nothing at the path any more is nothing to remove; + anything else there is `_NotTheClone`, and nothing is removed through + it. + + ON MAIN THE REAL PATH WAS ASKED, AND THEN THE PATH REMOVED: a + `source-readonly` swapped for a symlink between the two removed the + directory of that name wherever the link led (#457's L1-99 (b)). Now + the identity of each directory, `st_dev` and `st_ino` read right after + this run's `mkdir`, is asked of what is there, never through a symlink. + Where `FD_REMOVAL` holds, the removal is relative to the descriptor of + the `source-readonly/` so asked, so nothing renamed after the check + redirects it. Elsewhere, by path after the same checks, only someone + who can rename in the work directory, in its `source-readonly/` or in a + directory above them could swap one between the check and the + removal; `_work_root` and `_source_readonly_refusal` refuse the first + two before anything is made there.""" + if FD_REMOVAL: + _remove_through_a_descriptor(claim) + else: + _remove_by_path(claim) + + +def _why_kept(claim: _Claim) -> str | None: + """Remove the clone `claim` names (`_remove_a_tree`), and say why it is + not removed, in the words its NOTE ends with; None where it is.""" + now = os.path.realpath(claim.path) + if not _same_spelling(now, claim.made_at): + return (f"is not removed: that path now leads to {_spelled(now)}, so " + "nothing is removed through it") + try: + _remove_a_tree(claim) + except _NotTheClone as kept: + return f"is not removed: {kept}" + except OSError as exc: + return (f"is kept: removing it failed ({exc.strerror or exc}). Delete " + "it once nothing in it is needed") + return None def _leave_the_clones(args) -> None: """Remove each clone of a source this command made that no plan records - (#652), however the command ended: `args.clones`, which `Source.clone` - fills for `plan` and `check`, each with the real path it had when it was - made and whether this run made `source-readonly/` too. + (#652), however the command ended: `args.clones`, the `_Claim` of each + directory `Source.clone` made for `plan` and `check`. ON MAIN EACH STAYED, and nothing said so: the clone `check --source ` made, the one `check` made again where the plan's was gone, @@ -13050,40 +13436,34 @@ def _leave_the_clones(args) -> None: WHAT IS NEVER REMOVED. A clone that a written plan records: once `plan` has written it (`args.plan_written`), whatever ends the run after, its report cut short by a closed pipe or Ctrl-C included (#652's review, - F2). `check` does not list a clone it made again at the plan's own - `local_path` (`Plan._recorded_source`). `execute` lists none: under - `--local-remote-dir` its clone IS the assembly the split is pushed into, - and the deletion command `adopt-branch-exists` prints runs in it; the - work directory it made is named on its last line (`_leave_the_work_root`). - A path is listed only where it did not exist before this run, so nothing - a person or an earlier run put there is. And a path is removed only - where its real path is still the one it had when it was made, so - nothing outside that work directory's own `source-readonly/` is: not - through a symlink or a junction there before the run, and not through - one put there during it, at the work directory or below (F1). - - A CLONE THAT CANNOT BE REMOVED IS NAMED, in a NOTE on stderr, and kept. - Nothing here raises: an exception the command raised reaches the - caller as it was, and the exit is the command's (F4).""" + F2). `check` does not list a clone it made at the plan's own + `local_path` (`Plan._recorded_source`, `Plan._kept_at_the_record`). + `execute` lists none: under `--local-remote-dir` its clone IS the + assembly the split is pushed into, and the deletion command + `adopt-branch-exists` prints runs in it; the work directory it made is + named on its last line (`_leave_the_work_root`). A directory is listed + only where this run's own `mkdir` made it (#803), so nothing a person, + an earlier run or another run put there is. And it is removed only + where its path still leads to it, compared as the platform compares + names (#803), and where it is still the directory this run made + (`_remove_a_tree`), so nothing outside the work directory's own + `source-readonly/` is: not through a symlink or a junction there + before the run, and not through one put there during it. + + A CLONE NOT REMOVED IS NAMED, in a NOTE on stderr: one that cannot be + removed, which is kept, and since #803 one whose path no longer leads + to it, where on main the skip said nothing (#457's L1-99 (a)). Nothing + here raises: an exception the command raised reaches the caller as it + was, and the exit is the command's (F4).""" if getattr(args, "plan_written", False): return - for path, made_at, made_parent in getattr(args, "clones", None) or (): - if os.path.realpath(path) != made_at: - continue - try: - if os.path.isdir(made_at): - _remove_a_tree(Path(made_at)) - except OSError as exc: + for claim in getattr(args, "clones", None) or (): + why = _why_kept(claim) + if why: _flush_stdout() with contextlib.suppress(OSError, UnicodeError): - print(f"NOTE the clone this run made, {_spelled(made_at)}, " - f"is kept: removing it failed ({exc.strerror or exc}). " - "Delete it once nothing in it is needed.", - file=sys.stderr) - continue - if made_parent: - with contextlib.suppress(OSError): - os.rmdir(os.path.dirname(made_at)) + print(f"NOTE the clone this run made, " + f"{_spelled(claim.made_at)}, {why}.", file=sys.stderr) def _leave_the_work_root(args, succeeded: bool) -> None: @@ -13179,14 +13559,22 @@ def _work_dir_help(does: str, refused: str, but: str = "") -> str: return (f"{does} Default: a new directory in the system temp folder, " "removed when the run ends if it holds no file. A --work-dir " "you give is made where it is missing, and nothing in it is " - f"removed{but}. {refused}") + f"removed{but}. {refused} {WORK_DIR_SHARED_HELP}") + + +#: What `_work_root` refuses in all three commands, said in each one's +#: `--work-dir` help (#803). +WORK_DIR_SHARED_HELP = ( + "Except on Windows, one that another user owns, or that every user can " + "write in without the sticky bit, is refused as `adopt-work-dir-shared` " + "before anything is made in it.") #: What a work directory that holds the clone's directory is refused as, in #: `plan`'s and `check`'s `--work-dir` help (#652). -CLONE_TARGET_HELD = ("One that already holds `source-readonly/`, where " - "a source is cloned, is refused as " - "`adopt-work-dir-occupied`.") +CLONE_TARGET_HELD = ("Where a source is cloned, one whose `source-readonly` " + "is a file or a symlink, or holds `` already, even " + "empty, is refused as `adopt-work-dir-occupied`.") #: `--source`'s help on `check` and `execute`: how the source is read with #: it and without it (#652). SOURCE_OVERRIDE_HELP = ( diff --git a/docs/cli.md b/docs/cli.md index ad2cfa9b..65695b16 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -534,9 +534,14 @@ options: but the clone, when `plan` writes no plan that records it. One that cannot be made, or that this user cannot write in when a source is cloned into it, is refused - as `adopt-work-dir-unwritable`. One that already holds - `source-readonly/`, where a source is cloned, is - refused as `adopt-work-dir-occupied`. + as `adopt-work-dir-unwritable`. Where a source is + cloned, one whose `source-readonly` is a file or a + symlink, or holds `` already, even empty, is + refused as `adopt-work-dir-occupied`. Except on + Windows, one that another user owns, or that every + user can write in without the sticky bit, is refused + as `adopt-work-dir-shared` before anything is made in + it. ``` #### `adopt-project.py check` @@ -576,9 +581,14 @@ options: it. One this user cannot write in is a finding, `adopt-work-dir-unwritable`, and is refused under that code where a source would be cloned into it or cannot - be opened, as one that cannot be made is. One that - already holds `source-readonly/`, where a source - is cloned, is refused as `adopt-work-dir-occupied`. + be opened, as one that cannot be made is. Where a + source is cloned, one whose `source-readonly` is a + file or a symlink, or holds `` already, even + empty, is refused as `adopt-work-dir-occupied`. Except + on Windows, one that another user owns, or that every + user can write in without the sticky bit, is refused + as `adopt-work-dir-shared` before anything is made in + it. ``` #### `adopt-project.py execute` @@ -622,7 +632,10 @@ options: refused as `adopt-work-dir-occupied`, and one that cannot be made, or that this user cannot write in, as `adopt-work-dir-unwritable`, before any leg repository - exists. + exists. Except on Windows, one that another user owns, + or that every user can write in without the sticky + bit, is refused as `adopt-work-dir-shared` before + anything is made in it. ``` ### `update-shape.py` diff --git a/tests/test_adopt_clone_claimed.py b/tests/test_adopt_clone_claimed.py new file mode 100644 index 00000000..e0c7cdf2 --- /dev/null +++ b/tests/test_adopt_clone_claimed.py @@ -0,0 +1,574 @@ +# SPDX-License-Identifier: Apache-2.0 +"""The clone `plan` and `check` make in the work directory is claimed by this +run, and removed only as what this run made (#803: L1-65, L1-97, L1-98, +L1-99 and L1-102 of #457). + +ON MAIN AT bf1f099b (2026-10-17), with a stand-in `gh`: +- L1-99 (b). `_leave_the_clones` asked the clone's real path, then + `_remove_a_tree` removed it by that path: `W/source-readonly`, swapped for + a symlink to `victim/` between the two, removed `victim/Thing`. +- L1-99 (a). A removal skipped because the path led elsewhere said nothing. +- L1-98. A run whose clone failed removed the clone another run on the same + `--work-dir` had made there since, and that run's plan recorded it. +- L1-97. `check --source ` removed its clone where it had made it + at the plan's own `source.local_path`. +- L1-102. The clone's recorded path and its real path were compared with + `!=`, which on Windows reads the disk's case on one side only. +- L1-65. A file at `W/source-readonly` ended `plan`, `check` and `execute` + in a `FileExistsError` traceback. +- A `--work-dir`, or a `source-readonly/` in it, that another user owns or + every user can write in was used as any other. + +NO NETWORK AND NO GITHUB: `gh repo clone` is a `git clone` of a repository +on disk (`gh_clones`), or a stub `gh` that fails (the race). Every directory +a test swaps in or moves is under its own `tmp_path`. +""" + +from __future__ import annotations + +import errno +import os +import stat +import subprocess +import sys +import time +from pathlib import Path + +import pytest + +from conftest import ADOPT, WINDOWS_SKIP, spelled +from test_adopt_source_clone_record import ( + MINE, OCCUPIED, SOURCE_READONLY, WORK, WORK_FLAG, clone_dir, on_github, + plan_args, run, symlinks_are_made, +) +from test_adopt_temp_work_roots import ( # noqa: F401 + CHECK, EXECUTE, LOCAL_REMOTE_DIR, ORG, PLAN, PLAN_FLAG, PLAN_VERB, PROJECT, + REMOTES, REPOSITORY, SOURCE_FLAG, adopter, planned_from_github, +) + +SHARED = "REFUSED adopt-work-dir-shared: " +UNWRITABLE = "REFUSED adopt-work-dir-unwritable: " +UNCLONABLE = "REFUSED source-unclonable: " +#: `execute`'s word, before its way out, that nothing exists yet. +NOTHING_PUSHED = "no leg repository was created and nothing was pushed. " +ANOTHER = ("Pass a --work-dir that does not hold it, or none: without one, " + "each run makes a new one.") +OWN = ("Pass a --work-dir this user owns and no other user can write in, or " + "none: without one, each run makes a new one.") +EVERY_USER = "can be written in by every user, without the sticky bit" +#: The NOTE for a clone the run made and did not remove, up to its path. +NOT_REMOVED = "NOTE the clone this run made, " +NOT_MADE_HERE = ("what that path leads to now is not the directory this run " + "made, so nothing is removed through it.") +PRECIOUS = "precious.txt" +SWAPS = ["symlink", "directory"] +posix_only = pytest.mark.skipif( + not hasattr(os, "geteuid"), + reason="Windows asks no owner and no mode of a directory") + + +def real(path: Path) -> Path: + return Path(os.path.realpath(path)) + + +def the_clone_of(work: Path) -> Path: + """`clone_dir`, as the NOTE spells the path this run made.""" + return real(work) / SOURCE_READONLY / PROJECT + + +def victim_holding(base: Path) -> Path: + """`base/victim`, holding `Thing/precious.txt`: a directory of the + person's own, outside every work directory.""" + victim = base / "victim" + (victim / PROJECT).mkdir(parents=True) + (victim / PROJECT / PRECIOUS).write_text(MINE, encoding="utf-8") + return victim + + +def swap(work: Path, aside: Path, victim: Path, how: str) -> None: + """`work/source-readonly` moved to `aside`, and in its place a symlink to + `victim`, or `victim` itself moved there.""" + os.rename(work / SOURCE_READONLY, aside) + if how == "symlink": + os.symlink(victim, work / SOURCE_READONLY, target_is_directory=True) + else: + os.rename(victim, work / SOURCE_READONLY) + + +def checked_with_source(planned, work: Path) -> list[str]: + return [CHECK, PLAN_FLAG, str(planned.plan), SOURCE_FLAG, REPOSITORY, + WORK_FLAG, str(work)] + + +def argv_for(command: str, planned, work: Path, base: Path) -> list[str]: + """`command` given `--source REPOSITORY --work-dir work`.""" + if command == PLAN_VERB: + return plan_args(REPOSITORY, base / "again.yaml", WORK_FLAG, str(work)) + argv = checked_with_source(planned, work) + if command == EXECUTE: + argv[0] = EXECUTE + argv += [LOCAL_REMOTE_DIR, str(base / REMOTES)] + return argv + + +# --- L1-99 (b): a swap at the removal removes nothing of anybody's ----------- + +@pytest.mark.parametrize("how", SWAPS) +def test_a_swap_as_the_removal_starts_removes_nothing_outside_the_clone( + adopter, monkeypatch, capsys, tmp_path, how): + """t111c's injection: `source-readonly` swapped at the start of + `_remove_a_tree`, for a symlink to `victim/` or for `victim/` itself. On + main `check` exited 0 and removed `victim/Thing` with the file in it. + Now the removal finds no longer the directory this run made, says so, + and removes nothing.""" + planned = planned_from_github(adopter, monkeypatch, tmp_path) + if how == "symlink": + symlinks_are_made(tmp_path) + work, aside = tmp_path / WORK, tmp_path / "aside" + victim = victim_holding(tmp_path) + precious = (victim if how == "symlink" else work / SOURCE_READONLY) \ + / PROJECT / PRECIOUS + made_at = the_clone_of(work) + removal = adopter._remove_a_tree + + def swapped_first(*args, **keywords): + swap(work, aside, victim, how) + return removal(*args, **keywords) + + monkeypatch.setattr(adopter, "_remove_a_tree", swapped_first) + code, _, err = run(adopter, capsys, *checked_with_source(planned, work)) + assert code == 0, err + assert precious.read_text(encoding="utf-8") == MINE + assert f"{NOT_REMOVED}{spelled(made_at)}, is not removed: " \ + f"{NOT_MADE_HERE}" in err.splitlines(), err + assert (aside / PROJECT / ".git").is_dir() + + +def test_a_swap_after_the_checks_removes_only_the_runs_own_clone( + adopter, monkeypatch, capsys, tmp_path): + """The window between the last check and the removal: `source-readonly` + swapped for a symlink to `victim/` as `shutil.rmtree` is called. The + removal is relative to the directory this run made, held open, so it + removes the run's own clone, moved aside, and nothing through the link. + Where the removal is by path (Python 3.9 and 3.10, and Windows) this + window stays open, and the test cannot close it.""" + if not getattr(adopter, "FD_REMOVAL", True): + pytest.skip("this Python removes a clone by its path: the window " + "between the check and the removal stays open") + planned = planned_from_github(adopter, monkeypatch, tmp_path) + symlinks_are_made(tmp_path) + work, aside = tmp_path / WORK, tmp_path / "aside" + victim = victim_holding(tmp_path) + removal = adopter.shutil.rmtree + + def swapped_first(path, *args, **keywords): + if os.path.basename(path) == PROJECT and not aside.exists(): + swap(work, aside, victim, "symlink") + return removal(path, *args, **keywords) + + monkeypatch.setattr(adopter.shutil, "rmtree", swapped_first) + code, _, err = run(adopter, capsys, *checked_with_source(planned, work)) + assert code == 0, err + assert (victim / PROJECT / PRECIOUS).read_text(encoding="utf-8") == MINE + assert not os.path.lexists(aside / PROJECT) + + +# --- L1-99 (a): a removal skipped is named ----------------------------------- + +def test_a_source_readonly_swapped_for_a_symlink_is_named_and_kept( + adopter, monkeypatch, capsys, tmp_path): + """`W/source-readonly` swapped for a symlink to `bigdisk/` once the + clone was read: the clone's path now leads elsewhere, so nothing is + removed through it, and a NOTE says so. On main the skip was a bare + `continue`, and stderr was empty.""" + planned = planned_from_github(adopter, monkeypatch, tmp_path) + symlinks_are_made(tmp_path) + work, aside = tmp_path / WORK, tmp_path / "aside" + bigdisk = tmp_path / "bigdisk" + bigdisk.mkdir() + made_at = the_clone_of(work) + leaving = adopter._leave_the_clones + + def swapped_first(args): + swap(work, aside, bigdisk, "symlink") + return leaving(args) + + monkeypatch.setattr(adopter, "_leave_the_clones", swapped_first) + code, _, err = run(adopter, capsys, *checked_with_source(planned, work)) + assert code == 0, err + assert (f"{NOT_REMOVED}{spelled(made_at)}, is not removed: that path now " + f"leads to {spelled(real(bigdisk) / PROJECT)}, so nothing is " + "removed through it.") in err.splitlines(), err + assert (aside / PROJECT / ".git").is_dir() + assert os.listdir(bigdisk) == [] + + +# --- L1-98: a run claims its clone's directory -------------------------------- + +#: A `gh` that says it started, then waits, at most a minute, for the test +#: to let it go, and fails: the run whose clone fails late. +LATE_GH = (b"#!/bin/sh\n" + b": > \"$W803_STARTED\"\n" + b"i=0\n" + b"while [ ! -e \"$W803_GO\" ] && [ $i -lt 600 ]; do\n" + b" sleep 0.1\n" + b" i=$((i + 1))\n" + b"done\n" + b"echo 'stub gh: could not clone' >&2\n" + b"exit 1\n") + + +def wait_for(path: Path, child: subprocess.Popen) -> None: + """Until `path` exists, a minute at most, while `child` runs.""" + for _ in range(600): + if path.exists() or child.poll() is not None: + break + time.sleep(0.1) + assert path.exists(), child.communicate() + + +@WINDOWS_SKIP # the stub `gh` is a `#!` script; Windows executes none +def test_a_run_never_takes_the_clone_another_run_on_its_work_dir_claimed( + adopter, monkeypatch, capsys, tmp_path): + """Run A, `check --source testorg/Thing --work-dir W`, is in `gh` when + run B, `plan --source testorg/Thing --work-dir W`, starts; then A's + clone fails. On main B cloned into the directory A was about to clone + into and wrote its plan, and A then removed B's clone. Now A made that + directory first, so B is refused it, and A removes only what it made.""" + planned = planned_from_github(adopter, monkeypatch, tmp_path) + work = tmp_path / WORK + stub = tmp_path / "late-bin" + stub.mkdir() + (stub / "gh").write_bytes(LATE_GH) + (stub / "gh").chmod(0o755) + (tmp_path / "gh-config").mkdir() + started, go = tmp_path / "started", tmp_path / "go" + env = {**os.environ, "W803_STARTED": str(started), "W803_GO": str(go), + "PATH": f"{stub}{os.pathsep}{os.environ['PATH']}", + "GH_TOKEN": "bogus-w803", "GH_CONFIG_DIR": str(tmp_path / "gh-config")} + first = subprocess.Popen( # noqa: S603 - this interpreter, this tool + [sys.executable, str(ADOPT), *checked_with_source(planned, work)], + cwd=str(tmp_path), env=env, stdin=subprocess.DEVNULL, + stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True) + try: + wait_for(started, first) + second = tmp_path / "second.yaml" + code, _, err = run(adopter, capsys, + *plan_args(REPOSITORY, second, WORK_FLAG, str(work))) + finally: + go.touch() + _, first_err = first.communicate(timeout=120) + assert code == 2, err + assert err.startswith( + f"{OCCUPIED}--work-dir {spelled(real(work))} already holds " + f"`{SOURCE_READONLY}/{PROJECT}`, an empty directory, "), err + assert not second.exists() + assert len(planned.calls) == 1 + assert first.returncode == 2, first_err + assert UNCLONABLE in first_err + assert not os.path.lexists(work / SOURCE_READONLY) + + +def test_an_empty_directory_where_the_clone_goes_is_refused_and_kept( + adopter, monkeypatch, capsys, tmp_path): + """A directory already there is never taken for this run's own: on main + `plan` cloned into an empty one and listed none of it.""" + calls = on_github(adopter, monkeypatch, tmp_path) + work = tmp_path / WORK + clone_dir(work).mkdir(parents=True) + code, _, err = run(adopter, capsys, *plan_args( + REPOSITORY, tmp_path / PLAN, WORK_FLAG, str(work))) + assert code == 2, err + assert err == ( + f"{OCCUPIED}--work-dir {spelled(real(work))} already holds " + f"`{SOURCE_READONLY}/{PROJECT}`, an empty directory, where this run " + f"clones {REPOSITORY}, and a run clones only into a directory it has " + "just made, so that it never takes another run's for its own\n" + f"Remediation: {ANOTHER}\n") + assert calls == [] + assert clone_dir(work).is_dir() + + +def test_execute_whose_clone_fails_removes_what_it_made_and_no_more( + adopter, monkeypatch, capsys, tmp_path): + """`execute` keeps the clone it makes, so it lists none. Its claim on + the clone's directory is made before `gh`, and a clone that fails leaves + that directory empty: it removes what its own `mkdir` made, so that the + same command run again is not refused at it. What the person keeps in + the work directory stays.""" + planned = planned_from_github(adopter, monkeypatch, tmp_path) + work = tmp_path / WORK + work.mkdir() + (work / "mine.txt").write_text(MINE, encoding="utf-8") + argv = [EXECUTE, PLAN_FLAG, str(planned.plan), SOURCE_FLAG, + f"{ORG}/Elsewhere", WORK_FLAG, str(work), LOCAL_REMOTE_DIR, + str(tmp_path / REMOTES)] + for _ in range(2): + code, _, err = run(adopter, capsys, *argv) + assert code == 2, err + assert err.startswith(UNCLONABLE), err + assert sorted(os.listdir(work)) == ["mine.txt"] + + +# --- L1-97: the plan's own record is kept ------------------------------------- + +def test_a_source_clone_made_at_the_plans_own_record_is_kept( + adopter, monkeypatch, capsys, tmp_path): + """The plan records `W/source-readonly/Thing`, moved away since. `check + --source testorg/Thing --work-dir W` clones there again, which is the + plan's source again: kept, as the same `check` without `--source` keeps + it. On main it was removed, and the next run cloned it once more.""" + calls = on_github(adopter, monkeypatch, tmp_path) + work, plan = tmp_path / WORK, tmp_path / PLAN + assert run(adopter, capsys, *plan_args(REPOSITORY, plan, WORK_FLAG, + str(work)))[0] == 0 + os.rename(clone_dir(work), tmp_path / "moved") + code, _, err = run(adopter, capsys, CHECK, PLAN_FLAG, str(plan), + SOURCE_FLAG, REPOSITORY, WORK_FLAG, str(work)) + assert code == 0, err + assert (clone_dir(work) / ".git").is_dir() + assert len(calls) == 2 + + +# --- L1-102: the clone's path is compared as the platform compares names ----- + +def test_the_clones_path_is_compared_whatever_its_case( + adopter, monkeypatch, capsys, tmp_path): + """On Windows `os.path.realpath` gives the disk's case, here + `Source-Readonly`, and the recorded path was spelled `source-readonly`: + on main the two differed, and the clone was kept, silently. Emulated + with `os.path` patched as `ntpath` answers, during the removal only.""" + planned = planned_from_github(adopter, monkeypatch, tmp_path) + work = tmp_path / WORK + made_at = the_clone_of(work) + leaving, realpath = adopter._leave_the_clones, os.path.realpath + + def as_windows_reads_it(args): + with monkeypatch.context() as patched: + patched.setattr(os.path, "normcase", str.lower) + patched.setattr(os.path, "realpath", lambda path, *rest, **kw: ( + realpath(path, *rest, **kw).replace( + SOURCE_READONLY, "Source-Readonly"))) + return leaving(args) + + monkeypatch.setattr(adopter, "_leave_the_clones", as_windows_reads_it) + code, _, err = run(adopter, capsys, *checked_with_source(planned, work)) + assert code == 0, err + assert NOT_REMOVED not in err + assert not os.path.lexists(made_at) + + +# --- L1-65 and the work directory's own `source-readonly` -------------------- + +@pytest.mark.parametrize("command", [PLAN_VERB, CHECK, EXECUTE]) +def test_a_file_at_source_readonly_is_refused_by_name_before_gh( + adopter, monkeypatch, capsys, tmp_path, command): + """On main each command ended in `FileExistsError: [Errno 17] File + exists: '/source-readonly'`, exit 1. Now it is refused by + name, before any `gh` call, and the file stays.""" + planned = planned_from_github(adopter, monkeypatch, tmp_path) + work = tmp_path / WORK + work.mkdir() + (work / SOURCE_READONLY).write_text(MINE, encoding="utf-8") + done = NOTHING_PUSHED if command == EXECUTE else "" + code, _, err = run(adopter, capsys, + *argv_for(command, planned, work, tmp_path)) + assert code == 2, err + assert err == ( + f"{OCCUPIED}--work-dir {spelled(real(work))} already holds " + f"`{SOURCE_READONLY}`, a file, where this run clones {REPOSITORY} " + f"into a directory of its own\nRemediation: {done}{ANOTHER}\n") + assert len(planned.calls) == 1 + assert (work / SOURCE_READONLY).read_text(encoding="utf-8") == MINE + + +INJECTED = { + "a-file-made-since": ( + "file", OCCUPIED, "already holds `source-readonly`, a file, where this " + f"run clones {REPOSITORY} into a directory of its own", ANOTHER), + "a-mkdir-refused": ( + "refused", UNWRITABLE, "cannot hold `source-readonly`, where this run " + f"clones {REPOSITORY}: making it failed (Permission denied)", + "Pass a --work-dir this user can write in, or none: without one, " + "each run makes a new one."), +} + + +@pytest.mark.parametrize(("what", "refused", "detail", "remedy"), + list(INJECTED.values()), ids=list(INJECTED)) +def test_a_source_readonly_that_cannot_be_made_is_refused_by_name( + adopter, monkeypatch, capsys, tmp_path, what, refused, detail, remedy): + """Asked before the NOTE, `source-readonly` was free; then its `mkdir` + failed: a file made there since, or the system refusing it. Each is + refused by name, before `gh`. On main each was a traceback.""" + calls = on_github(adopter, monkeypatch, tmp_path) + work = tmp_path / WORK + mkdir = os.mkdir + + def in_the_way(path, *args, **keywords): + if os.path.basename(path) == SOURCE_READONLY: + if what == "refused": + raise PermissionError(errno.EACCES, "Permission denied", path) + Path(path).write_text(MINE, encoding="utf-8") + return mkdir(path, *args, **keywords) + + monkeypatch.setattr(os, "mkdir", in_the_way) + code, _, err = run(adopter, capsys, *plan_args( + REPOSITORY, tmp_path / PLAN, WORK_FLAG, str(work))) + assert code == 2, err + assert err == (f"{refused}--work-dir {spelled(real(work))} {detail}\n" + f"Remediation: {remedy}\n") + assert calls == [] + + +# --- decision 1: a directory another user can rename in ---------------------- + +@posix_only +@pytest.mark.parametrize("command", [PLAN_VERB, CHECK, EXECUTE]) +def test_a_work_dir_every_user_can_write_in_is_refused_before_anything( + adopter, monkeypatch, capsys, tmp_path, command): + """`chmod 777`, no sticky bit: another user can rename anything in it, + the clone included. Refused before anything is made there, the write + probe included, and before `gh`. On main each command used it.""" + planned = planned_from_github(adopter, monkeypatch, tmp_path) + work = tmp_path / WORK + work.mkdir() + os.chmod(work, 0o777) + done = NOTHING_PUSHED if command == EXECUTE else "" + code, _, err = run(adopter, capsys, + *argv_for(command, planned, work, tmp_path)) + assert code == 2, err + assert err == ( + f"{SHARED}--work-dir {spelled(real(work))} {EVERY_USER}, so another " + "user could rename or replace what this run makes in it\n" + f"Remediation: {done}{OWN}\n") + assert os.listdir(work) == [] + assert len(planned.calls) == 1 + + +@posix_only +def test_a_source_readonly_every_user_can_write_in_is_refused( + adopter, monkeypatch, capsys, tmp_path): + planned = planned_from_github(adopter, monkeypatch, tmp_path) + work = tmp_path / WORK + (work / SOURCE_READONLY).mkdir(parents=True) + os.chmod(work / SOURCE_READONLY, 0o777) + code, _, err = run(adopter, capsys, *checked_with_source(planned, work)) + assert code == 2, err + assert err == ( + f"{SHARED}--work-dir {spelled(real(work))} holds `{SOURCE_READONLY}`, " + f"which {EVERY_USER}, so another user could rename or replace the " + f"clone this run makes in it\nRemediation: {ANOTHER}\n") + assert os.listdir(work / SOURCE_READONLY) == [] + assert len(planned.calls) == 1 + + +@posix_only +def test_the_sticky_bit_keeps_each_entry_to_its_owner( + adopter, monkeypatch, capsys, tmp_path): + """The control: a work directory and a `source-readonly` every user can + write in, each with the sticky bit, as `/tmp` is. Nobody but this user + can rename what this run makes there, so both are used, and the clone + removed.""" + planned = planned_from_github(adopter, monkeypatch, tmp_path) + work = tmp_path / WORK + (work / SOURCE_READONLY).mkdir(parents=True) + for path in (work, work / SOURCE_READONLY): + os.chmod(path, 0o1777) + code, _, err = run(adopter, capsys, *checked_with_source(planned, work)) + assert code == 0, err + assert os.listdir(work / SOURCE_READONLY) == [] + assert len(planned.calls) == 2 + + +def owned_by_another(adopter, monkeypatch, path: Path) -> int: + """`path` owned by another user: given away where this test runs as + root, and otherwise told so by `os.lstat` for that path alone. The + other user's id.""" + other = os.geteuid() + 1 if os.geteuid() else 65534 + if os.geteuid() == 0: + os.chown(path, other, -1) + return other + lstat, which = os.lstat, os.path.realpath(path) + + def as_another_owns_it(target, *args, **keywords): + status = lstat(target, *args, **keywords) + if keywords.get("dir_fd") is None and not args \ + and os.fspath(target) == which: + fields = list(status[:10]) + fields[4] = other + return os.stat_result(fields) + return status + + monkeypatch.setattr(os, "lstat", as_another_owns_it) + return other + + +@posix_only +def test_a_source_readonly_another_user_owns_is_refused( + adopter, monkeypatch, capsys, tmp_path): + """The other user who made `source-readonly/` first owns the clone's + parent, and can swap the clone or the removal. Refused by name, before + `gh`.""" + planned = planned_from_github(adopter, monkeypatch, tmp_path) + work = tmp_path / WORK + (work / SOURCE_READONLY).mkdir(parents=True) + owned_by_another(adopter, monkeypatch, work / SOURCE_READONLY) + code, _, err = run(adopter, capsys, *checked_with_source(planned, work)) + assert code == 2, err + assert err.startswith( + f"{SHARED}--work-dir {spelled(real(work))} holds `{SOURCE_READONLY}`, " + "which is owned by "), err + assert err.endswith( + ", not by this user, so another user could rename or replace the " + f"clone this run makes in it\nRemediation: {ANOTHER}\n"), err + assert len(planned.calls) == 1 + + +@posix_only +def test_a_work_dir_another_user_owns_is_refused( + adopter, monkeypatch, capsys, tmp_path): + planned = planned_from_github(adopter, monkeypatch, tmp_path) + work = tmp_path / WORK + work.mkdir() + owned_by_another(adopter, monkeypatch, work) + code, _, err = run(adopter, capsys, *checked_with_source(planned, work)) + assert code == 2, err + assert err.startswith( + f"{SHARED}--work-dir {spelled(real(work))} is owned by "), err + assert err.endswith( + ", not by this user, so another user could rename or replace what " + f"this run makes in it\nRemediation: {OWN}\n"), err + assert os.listdir(work) == [] + assert len(planned.calls) == 1 + + +@posix_only +def test_a_mode_this_run_makes_lets_no_other_user_write( + adopter, monkeypatch, capsys, tmp_path): + """`source-readonly` and the clone's directory are made 0755 whatever + the umask, so a run under `umask 0` makes none another user can rename + in. Asked as `then` reads it, right after the clone.""" + seen: dict = {} + calls = on_github(adopter, monkeypatch, tmp_path) + work = tmp_path / WORK + work.mkdir(mode=0o755) + real_run = adopter.run + + def and_look(args, *rest, **keywords): + done = real_run(args, *rest, **keywords) + if args[0] == "gh": + seen.update({path: stat.S_IMODE(os.lstat(path).st_mode) & 0o022 + for path in (work / SOURCE_READONLY, Path(args[4]))}) + return done + + monkeypatch.setattr(adopter, "run", and_look) + before = os.umask(0) + try: + code, _, err = run(adopter, capsys, *plan_args( + REPOSITORY, tmp_path / PLAN, WORK_FLAG, str(work))) + finally: + os.umask(before) + assert code == 0, err + assert len(calls) == 1 + assert set(seen.values()) == {0} diff --git a/tests/test_adopt_source_clone_record.py b/tests/test_adopt_source_clone_record.py index b9ee0800..ac082e04 100644 --- a/tests/test_adopt_source_clone_record.py +++ b/tests/test_adopt_source_clone_record.py @@ -535,7 +535,10 @@ def test_plan_into_a_work_dir_holding_its_clone_is_refused_by_name( def test_a_clone_reached_through_a_symlink_is_never_removed( adopter, monkeypatch, capsys, tmp_path): """Nothing outside the work directory's own `source-readonly/` goes: a - `source-readonly` that is a symlink leads somewhere else.""" + `source-readonly` that is a symlink leads somewhere else, so no clone + is made through it (#803). Until #803 the clone was made there and + kept, and nothing said so (#457's L1-99 (a)); the link's target is + named.""" planned = planned_from_github(adopter, monkeypatch, tmp_path) work, elsewhere = tmp_path / WORK, tmp_path / "elsewhere" work.mkdir() @@ -544,9 +547,15 @@ def test_a_clone_reached_through_a_symlink_is_never_removed( os.symlink(elsewhere, work / SOURCE_READONLY, target_is_directory=True) except OSError: pytest.skip("this machine makes no symlink") - assert run(adopter, capsys, CHECK, PLAN_FLAG, str(planned.plan), - SOURCE_FLAG, REPOSITORY, WORK_FLAG, str(work))[0] == 0 - assert (elsewhere / PROJECT / ".git").is_dir() + code, _, err = run(adopter, capsys, CHECK, PLAN_FLAG, str(planned.plan), + SOURCE_FLAG, REPOSITORY, WORK_FLAG, str(work)) + assert code == 2, err + assert err.startswith( + f"{OCCUPIED}--work-dir {spelled(os.path.realpath(work))} already " + f"holds `{SOURCE_READONLY}`, a symlink to {spelled(elsewhere)}, where " + f"this run clones {REPOSITORY} into a directory of its own\n"), err + assert len(planned.calls) == 1 + assert os.listdir(elsewhere) == [] def test_a_clone_is_removed_where_a_read_only_file_cannot_be_unlinked( @@ -566,6 +575,8 @@ def as_windows(path, *, dir_fd=None): monkeypatch.setattr(os, "unlink", as_windows) monkeypatch.setattr(os, "remove", as_windows) + # Nor, as on Windows, any removal by descriptor (#803). + monkeypatch.setattr(adopter, "FD_REMOVAL", False, raising=False) code, _, err = run(adopter, capsys, CHECK, PLAN_FLAG, str(planned.plan), SOURCE_FLAG, REPOSITORY) assert code == 0, err diff --git a/tests/test_adopt_work_dir_refusal.py b/tests/test_adopt_work_dir_refusal.py index d8c03807..856e4d17 100644 --- a/tests/test_adopt_work_dir_refusal.py +++ b/tests/test_adopt_work_dir_refusal.py @@ -59,9 +59,11 @@ MINE = "mine\n" #: Each command's codes for a `--work-dir`, which its help names. WORK_DIR_OCCUPIED = "adopt-work-dir-occupied" -WORK_DIR_CODES = {PLAN_VERB: (WORK_DIR_UNWRITABLE, WORK_DIR_OCCUPIED), - CHECK: (WORK_DIR_UNWRITABLE, WORK_DIR_OCCUPIED), - EXECUTE: (WORK_DIR_UNWRITABLE, WORK_DIR_OCCUPIED)} +#: One another user owns, or every user can write in (#803). +WORK_DIR_SHARED = "adopt-work-dir-shared" +WORK_DIR_CODES = { + command: (WORK_DIR_UNWRITABLE, WORK_DIR_OCCUPIED, WORK_DIR_SHARED) + for command in COMMANDS} NO_ORG = "adopt-no-org" #: How the NOTE about a recorded clone that is gone begins. GONE = "NOTE the plan's `source.local_path`, " From cf36411e963c33f637a04395fc24b22786315217 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sat, 17 Oct 2026 16:23:10 +0000 Subject: [PATCH 2/4] adopt-project.py: `Source.clone` makes a work directory that is missing above `source-readonly`, as #749 did, so a caller of `Source.open` that names a new one is no longer refused `adopt-work-dir-unwritable` (#803) The first commit made `source-readonly` with an exclusive `os.mkdir`, which needs the work directory to exist. `_work_root` makes it for every command, but tests/test_adopt_history_gitlinks.py calls `Source.open` directly with a work directory that does not exist, and both of its `org/repo` cases were refused. Only `source-readonly` itself and the clone's directory are made exclusively; the directories above are made as on main. The new test of a `source-readonly` this user cannot write in, which is refused before the NOTE, is added: the revert of that check failed nothing before. Lane: openreposhape-1 (openRepoShape-1) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 5 +++- tests/test_adopt_clone_claimed.py | 40 ++++++++++++++++++++++++++----- 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index ee4e7397..eda5f4d4 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -1874,10 +1874,13 @@ def _made_source_readonly(work_root: Path, parent: Path, repository: str, asked as `_source_readonly_refusal` asks it, and any other failure is `adopt-work-dir-unwritable`, by name, where it was a traceback (#457's L1-65). Made 0755 whatever the umask: under `umask 0` it would be one - every user can rename in.""" + every user can rename in. The work directory above it is made where it + is missing, as on main: `_work_root` makes it for every command, so + only a caller of `Source.open` that names another one meets that.""" if os.path.lexists(parent): return False try: + os.makedirs(work_root, exist_ok=True) os.mkdir(parent, 0o755) return True except FileExistsError: diff --git a/tests/test_adopt_clone_claimed.py b/tests/test_adopt_clone_claimed.py index e0c7cdf2..f6feb7a2 100644 --- a/tests/test_adopt_clone_claimed.py +++ b/tests/test_adopt_clone_claimed.py @@ -36,10 +36,10 @@ import pytest -from conftest import ADOPT, WINDOWS_SKIP, spelled -from test_adopt_source_clone_record import ( - MINE, OCCUPIED, SOURCE_READONLY, WORK, WORK_FLAG, clone_dir, on_github, - plan_args, run, symlinks_are_made, +from conftest import ADOPT, WINDOWS_SKIP, rmtree, spelled +from test_adopt_source_clone_record import ( # noqa: F401 + MINE, OCCUPIED, SOURCE_READONLY, WORK, WORK_FLAG, clone_dir, modes_hold, + on_github, plan_args, run, symlinks_are_made, ) from test_adopt_temp_work_roots import ( # noqa: F401 CHECK, EXECUTE, LOCAL_REMOTE_DIR, ORG, PLAN, PLAN_FLAG, PLAN_VERB, PROJECT, @@ -53,6 +53,8 @@ NOTHING_PUSHED = "no leg repository was created and nothing was pushed. " ANOTHER = ("Pass a --work-dir that does not hold it, or none: without one, " "each run makes a new one.") +WRITABLE = ("Pass a --work-dir this user can write in, or none: without one, " + "each run makes a new one.") OWN = ("Pass a --work-dir this user owns and no other user can write in, or " "none: without one, each run makes a new one.") EVERY_USER = "can be written in by every user, without the sticky bit" @@ -389,8 +391,7 @@ def test_a_file_at_source_readonly_is_refused_by_name_before_gh( "a-mkdir-refused": ( "refused", UNWRITABLE, "cannot hold `source-readonly`, where this run " f"clones {REPOSITORY}: making it failed (Permission denied)", - "Pass a --work-dir this user can write in, or none: without one, " - "each run makes a new one."), + WRITABLE), } @@ -421,6 +422,33 @@ def in_the_way(path, *args, **keywords): assert calls == [] +def test_a_source_readonly_this_user_cannot_write_in_is_refused_first( + adopter, monkeypatch, capsys, tmp_path, modes_hold): + """The plan's clone is gone from `W/source-readonly/`, which this user + can no longer write in (`chmod 555`). It is refused by name before the + NOTE that says the clone is made again, in #749's order: on main the + NOTE was printed, `gh` was called, and the run was refused + `source-unclonable`, with a remedy about `gh auth status`.""" + calls = on_github(adopter, monkeypatch, tmp_path) + work, plan = tmp_path / WORK, tmp_path / PLAN + assert run(adopter, capsys, *plan_args(REPOSITORY, plan, WORK_FLAG, + str(work)))[0] == 0 + rmtree(clone_dir(work)) + os.chmod(work / SOURCE_READONLY, 0o555) + try: + code, out, err = run(adopter, capsys, CHECK, PLAN_FLAG, str(plan), + WORK_FLAG, str(work)) + finally: + os.chmod(work / SOURCE_READONLY, 0o755) + assert code == 2, err + assert "NOTE the plan's" not in out + assert err == ( + f"{UNWRITABLE}--work-dir {spelled(real(work))} holds " + f"`{SOURCE_READONLY}`, where this run clones {REPOSITORY}, and this " + f"user cannot make a directory in it\nRemediation: {WRITABLE}\n") + assert len(calls) == 1 + + # --- decision 1: a directory another user can rename in ---------------------- @posix_only From bba7e4453d17a79165a53c0b4ba2f6095af876db Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sat, 17 Oct 2026 22:55:37 +0000 Subject: [PATCH 3/4] adopt-project.py: a `--work-dir` under a directory another user can rename in is refused before anything is made, root is trusted, `execute` never strands its claim, and every directory a run makes is 0755, so a work directory swapped from above no longer turns the removal onto another directory (#803) Fix round 1 on #843's review (r803, at cf36411e), on the coordinator's rulings: - Finding 1 (option a): `_work_root` asks each directory above the real `--work-dir`, from `/` down, before it makes any (`_shared_above`, `_refuse_a_shared_chain`), and again after. One another user can rename in is refused as `adopt-work-dir-shared`, naming it. This is OpenSSH's StrictModes rule for `authorized_keys`, but the sticky bit counts and group write is not asked. The real path is the only path the run uses from then on. - Finding 2: `execute` removes its claim however the clone ends (Ctrl-C, no `gh`), through the identity-checked removal, and raises what ended it. The empty-directory refusal also says to delete it where no run is using it. - Finding 3: a directory root owns is trusted, as StrictModes trusts it. - Finding 4: one constant, `A_FILE`, for "a file" (two lines outside the first commits change by that name), and `Plan._keep_the_clone_at_the_record` returns nothing. - Finding 5: `_made_0755` makes every missing directory 0755, and keeps #650's refusals. - Finding 6: the window test skips on the platform, and otherwise asserts `FD_REMOVAL`. - The help names the work directory, each directory above it and `source-readonly`; docs/cli.md is rendered again. Tests: tests/test_adopt_clone_claimed.py has 35 cases. Lane: openreposhape-1 (openRepoShape-1) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 192 +++++++++++++++----- docs/cli.md | 27 +-- tests/test_adopt_clone_claimed.py | 293 +++++++++++++++++++++++++++--- 3 files changed, 423 insertions(+), 89 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index eda5f4d4..c502e10f 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -1704,6 +1704,9 @@ def _occupied_clone_target(work_root: Path, path: Path, repository: str, f"{_spelled(held)}, {empty}where this run clones {repository}, " f"and {why}") remedy = f"Pass {ANOTHER_WORK_DIR}" + if empty: + remedy += (f" Or, where no run is using {_spelled(held)}, delete it: " + "a run that was killed can leave it empty.") if taken: detail = f"{taken}; and {detail}" remedy = (f"It is no longer a whole clone: delete {_spelled(held)} " @@ -1723,9 +1726,15 @@ def _occupied_clone_target(work_root: Path, path: Path, repository: str, #: The way out of a `--work-dir` that holds something in the clone's way. ANOTHER_WORK_DIR = ("a --work-dir that does not hold it, or none: without " "one, each run makes a new one.") -#: The way out of a `--work-dir` another user can rename entries in. -WORK_DIR_OWN = ("Pass a --work-dir this user owns and no other user can write " - "in, or none: without one, each run makes a new one.") +#: The way out of a `--work-dir` another user can rename entries in, or +#: rename it from a directory above it. +WORK_DIR_OWN = ("Pass a --work-dir this user owns, that no other user can " + "write in, and that has no directory above it another user " + "can rename in, or none: without one, each run makes a new " + "one.") +#: How a refusal names a file it found where it looked (#803: one spelling +#: for the three refusals that say it). +A_FILE = "a file" #: Whether `_remove_a_tree` removes a clone relative to a descriptor of the #: `source-readonly/` it was made in, never through its path (#803): what #: `shutil.rmtree(dir_fd=)` does from Python 3.11, where this platform's @@ -1783,20 +1792,88 @@ def _owner(uid: int) -> str: def _shared_with_another_user(status: os.stat_result) -> str | None: """Why another user can rename or replace what is in the directory `status` describes, an `os.lstat`, in words; None where nobody else can - (#803). Another user owns it; or every user can write in it, and its - sticky bit, which keeps each entry to its owner as in `/tmp`, is not - set. GROUP WRITE IS NOT ASKED: under `umask 002` every directory a user - makes is group-writable, for a group of that user alone. NOR IS ANYTHING - ON WINDOWS, where neither the owner nor the mode bits say it.""" + (#803). A user other than this one or root owns it; or every user can + write in it, and its sticky bit, which keeps each entry to its owner as + in `/tmp`, is not set. ROOT IS TRUSTED, as OpenSSH's StrictModes trusts + it: root can do anything here anyway, and a directory an earlier `sudo` + run left is then `adopt-work-dir-unwritable`, which is true, and not a + claim that another user could write in it. GROUP WRITE IS NOT ASKED: + under `umask 002` every directory a user makes is group-writable, for a + group of that user alone. NOR IS ANYTHING ON WINDOWS, where neither the + owner nor the mode bits say it.""" if not hasattr(os, "geteuid"): return None - if status.st_uid != os.geteuid(): - return f"is owned by {_owner(status.st_uid)}, not by this user" + if status.st_uid not in (os.geteuid(), 0): + return (f"is owned by {_owner(status.st_uid)}, not by this user or " + "root") if status.st_mode & stat.S_IWOTH and not status.st_mode & stat.S_ISVTX: return "can be written in by every user, without the sticky bit" return None +def _made_0755(path: Path, parents: bool = True) -> None: + """`path` made a directory where it is missing, and each missing + directory above it, as `Path.mkdir(parents=True, exist_ok=True)` makes + them and fails, but each with `os.mkdir(..., 0o755)` (#803): under + `umask 000` a work directory made 0777 would be refused as + `adopt-work-dir-shared` by the run that made it. The umask still narrows + it: under `umask 077` it is 0700. One another run made meanwhile is + taken, and is asked with the others above the work directory.""" + try: + os.mkdir(path, 0o755) + except FileNotFoundError: + if not parents or path.parent == path: + raise + _made_0755(path.parent) + _made_0755(path, parents=False) + except OSError: + if not path.is_dir(): + raise + + +def _shared_above(root: Path) -> tuple[Path, str] | None: + """The first directory above `root`, from `/` down, that another user + can rename in (`_shared_with_another_user`), and why; None where there is + none, asked down to the deepest one that exists (#803). `root` is the + real path `_work_root` read, so each of these is a directory, never a + symlink, and from then on the run uses `root` alone. + + THIS IS THE RULE OpenSSH's StrictModes HOLDS `authorized_keys` TO, but + for two things: the sticky bit counts, which is what keeps `/tmp` open, + and group write is not asked. A user who can rename in a directory above + the work directory can swap the work directory itself between this + run's claim and the identity it reads, so that the removal is held to + that user's directory (#843's review: a second process won 5 of 20 runs + on Python 3.12), and git's own cleanup of a failed clone removes by + path.""" + if not hasattr(os, "geteuid"): + return None + for above in reversed(root.parents): + try: + status = os.lstat(above) + except OSError: + return None + shared = _shared_with_another_user(status) + if shared: + return above, shared + return None + + +def _refuse_a_shared_chain(root: Path, done: str) -> None: + """`adopt-work-dir-shared` for a `--work-dir` that has a directory above + it another user can rename in (`_shared_above`), naming that directory + and why.""" + found = _shared_above(root) + if found: + above, shared = found + raise _shared_work_dir( + f"--work-dir {_spelled(str(root))} is under " + f"{_spelled(str(above))}, which {shared}, so another user could " + f"rename {_spelled(str(root))}, or a directory between them, and " + "turn this run's removal onto another directory", done, + WORK_DIR_OWN) + + def _shared_work_dir(detail: str, done: str, remedy: str) -> Refusal: """`adopt-work-dir-shared` for `detail`; `done` is said first in the way out, as `_work_dir_refusal` says it.""" @@ -1812,7 +1889,7 @@ def _what_is_there(held: Path, status: os.stat_result, leads: str) -> str: return f"a symlink to {_spelled(os.readlink(held))}" return "a symlink" if stat.S_ISREG(status.st_mode): - return "a file" + return A_FILE if stat.S_ISDIR(status.st_mode): return f"a directory that leads to {_spelled(leads)}" return "neither a file nor a directory" @@ -1880,7 +1957,7 @@ def _made_source_readonly(work_root: Path, parent: Path, repository: str, if os.path.lexists(parent): return False try: - os.makedirs(work_root, exist_ok=True) + _made_0755(work_root) os.mkdir(parent, 0o755) return True except FileExistsError: @@ -2024,10 +2101,13 @@ def clone(cls, repository: str, work_root: Path, there is refused, an empty one included; and this run then makes it with an exclusive `mkdir` (`_claimed`). Only that `_Claim` goes on `made`, and `_remove_a_tree` removes only the directory it names. - `execute` lists none and keeps its clone, but where `gh` fails it - removes the directory its own `mkdir` made, after the same checks, - so that the run given the same --work-dir again is not refused at - an empty directory.""" + `execute` lists none and keeps its clone, but where the clone ends + any other way than in a clone -- `gh` failing, Ctrl-C, no `gh` at + all -- it removes the directory its own `mkdir` made, after the same + checks, and then raises what ended it: so that the run given the + same --work-dir again is not refused at an empty directory. Only a + kill or a power loss can still leave one, and its refusal says to + delete it where no run is using it.""" path = work_root / SOURCE_READONLY / repository.split("/")[1] if unwritable: raise _work_dir_refusal(unwritable) @@ -2042,14 +2122,16 @@ def clone(cls, repository: str, work_root: Path, claim = _claimed(work_root, path, repository, done) if made is not None: made.append(claim) - print(f"cloning {repository} (read only) into {path}") try: + print(f"cloning {repository} (read only) into {path}") run(["gh", "repo", "clone", repository, str(path), "--", "--quiet"]) - except CommandFailed as exc: + except BaseException as exc: if made is None: with contextlib.suppress(OSError, _NotTheClone): _remove_a_tree(claim) + if not isinstance(exc, CommandFailed): + raise raise Refusal( "source-unclonable", f"could not clone {repository}: {exc.output.strip()[:400]}", @@ -4863,8 +4945,9 @@ def open_source(self, override: str | None, work_root: Path, existed, said the plan's repository is cloned again and read that directory instead.""" if override: - return self._kept_at_the_record( - Source.open(override, work_root, unwritable, made, done), made) + source = Source.open(override, work_root, unwritable, made, done) + self._keep_the_clone_at_the_record(made) + return source local_path = self.source.get("local_path") if local_path: return self._recorded_source(str(local_path), work_root, @@ -4877,22 +4960,21 @@ def open_source(self, override: str | None, work_root: Path, return Source.clone(repository, work_root, unwritable, made, done=done) - def _kept_at_the_record(self, source: Source, - made: list | None) -> Source: - """`source`, read from `--source`, with the clone this run made of it - taken off `made` where it is at the plan's own `source.local_path` - (#803, #457's L1-97): it is the plan's source again, as the clone - `_recorded_source` makes there is. On main `check` removed it, and - the next run cloned it once more.""" + def _keep_the_clone_at_the_record(self, made: list | None) -> None: + """Take off `made` the clone this run made from `--source` where it + is at the plan's own `source.local_path` (#803, #457's L1-97): it is + the plan's source again, as the clone `_recorded_source` makes there + is. On main `check` removed it, and the next run cloned it once + more.""" record = self.source.get("local_path") - if made and record: - try: - at = os.path.realpath(os.path.expanduser(str(record))) - except (OSError, ValueError): - return source - made[:] = [claim for claim in made - if not _same_spelling(claim.made_at, at)] - return source + if not (made and record): + return + try: + at = os.path.realpath(os.path.expanduser(str(record))) + except (OSError, ValueError): + return + made[:] = [claim for claim in made + if not _same_spelling(claim.made_at, at)] def _recorded_repository(self) -> str | None: """The plan's `source.repository` as `_github_name` reads it, or None @@ -8344,7 +8426,7 @@ def _over(self, walk: _Walk) -> str: landed = _spelled(walk.landed) is_dir = (os.path.isdir(walk.landed) if walk.outside else self._is_dir(walk.landed)) - what = "a directory" if is_dir else "a file" + what = "a directory" if is_dir else A_FILE return (f"it lands on {landed}, {what} that is already there, where " "the shape's copy cannot be written") @@ -10685,7 +10767,7 @@ def _index_entries(assembly: Path, for record in raw.split(b"\x00"): if record: head, _, name = record.partition(b"\t") - kind = INDEX_ENTRY_KINDS.get(int(head.split()[0], 8), "a file") + kind = INDEX_ENTRY_KINDS.get(int(head.split()[0], 8), A_FILE) entries.append((kind, name.decode("utf-8", "surrogateescape"))) return entries @@ -13210,20 +13292,27 @@ def _work_root(args, done: str = "") -> Path: can write in without the sticky bit (`_shared_with_another_user`), as `adopt-work-dir-shared`. There another user could replace the clone `plan` reads, or turn the clone's removal onto a directory of theirs - (#457's L1-99), or the legs `execute` builds. Asked with `os.lstat` of - the real path; never on Windows.""" + (#457's L1-99), or the legs `execute` builds. So is one under a + directory another user can rename in (`_shared_above`): asked of the + directories that exist BEFORE any is made, so that a refused run makes + nothing, and again after. Asked with `os.lstat` of the real path, which + is the one path the run uses from then on; never on Windows. Each + directory made here is 0755 whatever the umask (`_made_0755`). The + default work directory is not asked: `tempfile.mkdtemp` makes it 0700.""" if not getattr(args, "work_dir", None): root = Path(tempfile.mkdtemp(prefix="openreposhape-adopt-")) args.made_work_root = root return root root = Path(os.path.realpath(args.work_dir)) + _refuse_a_shared_chain(root, done) try: - root.mkdir(parents=True, exist_ok=True) + _made_0755(root) status = os.lstat(root) except OSError as exc: raise _work_dir_refusal( f"--work-dir {_spelled(str(root))} cannot be made: making it a " f"directory failed ({exc.strerror or exc})", done) from None + _refuse_a_shared_chain(root, done) shared = _shared_with_another_user(status) if shared: raise _shared_work_dir( @@ -13394,12 +13483,13 @@ def _remove_a_tree(claim: _Claim) -> None: the identity of each directory, `st_dev` and `st_ino` read right after this run's `mkdir`, is asked of what is there, never through a symlink. Where `FD_REMOVAL` holds, the removal is relative to the descriptor of - the `source-readonly/` so asked, so nothing renamed after the check - redirects it. Elsewhere, by path after the same checks, only someone - who can rename in the work directory, in its `source-readonly/` or in a - directory above them could swap one between the check and the - removal; `_work_root` and `_source_readonly_refusal` refuse the first - two before anything is made there.""" + the `source-readonly/` so asked, so no rename of `source-readonly/` or + above it after the check redirects it. Elsewhere it is by path, after + the same checks. Either way, only someone who can rename in the work + directory, in its `source-readonly/` or in a directory above them could + swap one, and `_work_root` and `_source_readonly_refusal` refuse all + three before anything is made there: but for group write, which is not + asked, and on Windows, where nothing is.""" if FD_REMOVAL: _remove_through_a_descriptor(claim) else: @@ -13440,7 +13530,8 @@ def _leave_the_clones(args) -> None: has written it (`args.plan_written`), whatever ends the run after, its report cut short by a closed pipe or Ctrl-C included (#652's review, F2). `check` does not list a clone it made at the plan's own - `local_path` (`Plan._recorded_source`, `Plan._kept_at_the_record`). + `local_path` (`Plan._recorded_source`, + `Plan._keep_the_clone_at_the_record`). `execute` lists none: under `--local-remote-dir` its clone IS the assembly the split is pushed into, and the deletion command `adopt-branch-exists` prints runs in it; the work directory it made is @@ -13568,9 +13659,10 @@ def _work_dir_help(does: str, refused: str, but: str = "") -> str: #: What `_work_root` refuses in all three commands, said in each one's #: `--work-dir` help (#803). WORK_DIR_SHARED_HELP = ( - "Except on Windows, one that another user owns, or that every user can " - "write in without the sticky bit, is refused as `adopt-work-dir-shared` " - "before anything is made in it.") + "Except on Windows, `adopt-work-dir-shared` refuses, before anything is " + "made, one that a user other than this one or root owns or every user " + "can write in without the sticky bit, one under such a directory, and " + "one whose `source-readonly` is one.") #: What a work directory that holds the clone's directory is refused as, in diff --git a/docs/cli.md b/docs/cli.md index 65695b16..81ac67f2 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -538,10 +538,11 @@ options: cloned, one whose `source-readonly` is a file or a symlink, or holds `` already, even empty, is refused as `adopt-work-dir-occupied`. Except on - Windows, one that another user owns, or that every - user can write in without the sticky bit, is refused - as `adopt-work-dir-shared` before anything is made in - it. + Windows, `adopt-work-dir-shared` refuses, before + anything is made, one that a user other than this one + or root owns or every user can write in without the + sticky bit, one under such a directory, and one whose + `source-readonly` is one. ``` #### `adopt-project.py check` @@ -585,10 +586,11 @@ options: source is cloned, one whose `source-readonly` is a file or a symlink, or holds `` already, even empty, is refused as `adopt-work-dir-occupied`. Except - on Windows, one that another user owns, or that every - user can write in without the sticky bit, is refused - as `adopt-work-dir-shared` before anything is made in - it. + on Windows, `adopt-work-dir-shared` refuses, before + anything is made, one that a user other than this one + or root owns or every user can write in without the + sticky bit, one under such a directory, and one whose + `source-readonly` is one. ``` #### `adopt-project.py execute` @@ -632,10 +634,11 @@ options: refused as `adopt-work-dir-occupied`, and one that cannot be made, or that this user cannot write in, as `adopt-work-dir-unwritable`, before any leg repository - exists. Except on Windows, one that another user owns, - or that every user can write in without the sticky - bit, is refused as `adopt-work-dir-shared` before - anything is made in it. + exists. Except on Windows, `adopt-work-dir-shared` + refuses, before anything is made, one that a user + other than this one or root owns or every user can + write in without the sticky bit, one under such a + directory, and one whose `source-readonly` is one. ``` ### `update-shape.py` diff --git a/tests/test_adopt_clone_claimed.py b/tests/test_adopt_clone_claimed.py index f6feb7a2..8fee0d31 100644 --- a/tests/test_adopt_clone_claimed.py +++ b/tests/test_adopt_clone_claimed.py @@ -28,6 +28,7 @@ import errno import os +import shutil import stat import subprocess import sys @@ -36,14 +37,15 @@ import pytest -from conftest import ADOPT, WINDOWS_SKIP, rmtree, spelled +from conftest import ADOPT, WINDOWS_SKIP, make_source_repo, rmtree, spelled from test_adopt_source_clone_record import ( # noqa: F401 MINE, OCCUPIED, SOURCE_READONLY, WORK, WORK_FLAG, clone_dir, modes_hold, on_github, plan_args, run, symlinks_are_made, ) from test_adopt_temp_work_roots import ( # noqa: F401 - CHECK, EXECUTE, LOCAL_REMOTE_DIR, ORG, PLAN, PLAN_FLAG, PLAN_VERB, PROJECT, - REMOTES, REPOSITORY, SOURCE_FLAG, adopter, planned_from_github, + CHECK, EXECUTE, LOCAL_REMOTE_DIR, ORG, OUT, PLAN, PLAN_FLAG, PLAN_VERB, + PLANNED, PROJECT, REMOTES, REPOSITORY, SMALL_TREE, SOURCE_FLAG, adopter, + planned_from_github, ) SHARED = "REFUSED adopt-work-dir-shared: " @@ -55,8 +57,13 @@ "each run makes a new one.") WRITABLE = ("Pass a --work-dir this user can write in, or none: without one, " "each run makes a new one.") -OWN = ("Pass a --work-dir this user owns and no other user can write in, or " +OWN = ("Pass a --work-dir this user owns, that no other user can write in, " + "and that has no directory above it another user can rename in, or " "none: without one, each run makes a new one.") +#: Why a directory above the work directory is refused, after its reason. +RENAMES_ABOVE = ("or a directory between them, and turn this run's removal " + "onto another directory") +NOT_THEIRS = ", not by this user or root, so another user could " EVERY_USER = "can be written in by every user, without the sticky bit" #: The NOTE for a clone the run made and did not remove, up to its path. NOT_REMOVED = "NOTE the clone this run made, " @@ -154,9 +161,11 @@ def test_a_swap_after_the_checks_removes_only_the_runs_own_clone( removes the run's own clone, moved aside, and nothing through the link. Where the removal is by path (Python 3.9 and 3.10, and Windows) this window stays open, and the test cannot close it.""" - if not getattr(adopter, "FD_REMOVAL", True): - pytest.skip("this Python removes a clone by its path: the window " + if sys.version_info < (3, 11) or os.name == "nt" \ + or not shutil.rmtree.avoids_symlink_attacks: + pytest.skip("this platform removes a clone by its path: the window " "between the check and the removal stays open") + assert adopter.FD_REMOVAL planned = planned_from_github(adopter, monkeypatch, tmp_path) symlinks_are_made(tmp_path) work, aside = tmp_path / WORK, tmp_path / "aside" @@ -286,7 +295,9 @@ def test_an_empty_directory_where_the_clone_goes_is_refused_and_kept( f"`{SOURCE_READONLY}/{PROJECT}`, an empty directory, where this run " f"clones {REPOSITORY}, and a run clones only into a directory it has " "just made, so that it never takes another run's for its own\n" - f"Remediation: {ANOTHER}\n") + f"Remediation: {ANOTHER} Or, where no run is using " + f"`{SOURCE_READONLY}/{PROJECT}`, delete it: a run that was killed can " + "leave it empty.\n") assert calls == [] assert clone_dir(work).is_dir() @@ -312,6 +323,42 @@ def test_execute_whose_clone_fails_removes_what_it_made_and_no_more( assert sorted(os.listdir(work)) == ["mine.txt"] +CUT_SHORT = { + "ctrl-c": KeyboardInterrupt, + "no-gh": lambda: FileNotFoundError(errno.ENOENT, "No such file or " + "directory", "gh"), +} + + +@pytest.mark.parametrize("cut", list(CUT_SHORT.values()), ids=list(CUT_SHORT)) +def test_execute_whose_clone_is_cut_short_leaves_no_claim( + adopter, monkeypatch, capsys, tmp_path, cut): + """Ctrl-C during the clone, or no `gh` on PATH: `execute` removes the + directory its own `mkdir` made and raises what ended it, so the same + command run again reaches `gh`. On #803's first head it ended only a + `CommandFailed` so, and the run again was refused at an empty + directory (#843's review, finding 2).""" + planned = planned_from_github(adopter, monkeypatch, tmp_path) + work = tmp_path / WORK + stand_in = adopter.run + + def cut_short(args, *rest, **keywords): + if args[0] == "gh": + raise cut() + return stand_in(args, *rest, **keywords) + + argv = [EXECUTE, PLAN_FLAG, str(planned.plan), SOURCE_FLAG, REPOSITORY, + WORK_FLAG, str(work), LOCAL_REMOTE_DIR, str(tmp_path / REMOTES)] + monkeypatch.setattr(adopter, "run", cut_short) + with pytest.raises(type(cut())): + adopter.main(argv) + assert not os.path.lexists(work / SOURCE_READONLY) + monkeypatch.setattr(adopter, "run", stand_in) + code, _, err = run(adopter, capsys, *argv) + assert "REFUSED adopt-unconfirmed: " in err, err + assert len(planned.calls) == 2 + + # --- L1-97: the plan's own record is kept ------------------------------------- def test_a_source_clone_made_at_the_plans_own_record_is_kept( @@ -494,14 +541,15 @@ def test_a_source_readonly_every_user_can_write_in_is_refused( @posix_only def test_the_sticky_bit_keeps_each_entry_to_its_owner( adopter, monkeypatch, capsys, tmp_path): - """The control: a work directory and a `source-readonly` every user can - write in, each with the sticky bit, as `/tmp` is. Nobody but this user - can rename what this run makes there, so both are used, and the clone + """The control: a work directory, the directory above it and its + `source-readonly`, each one every user can write in with the sticky + bit, as `/tmp` is, and each entry this user's. Nobody but this user + can rename what this run makes there, so all are used, and the clone removed.""" planned = planned_from_github(adopter, monkeypatch, tmp_path) - work = tmp_path / WORK + work = tmp_path / "sticky" / WORK (work / SOURCE_READONLY).mkdir(parents=True) - for path in (work, work / SOURCE_READONLY): + for path in (work.parent, work, work / SOURCE_READONLY): os.chmod(path, 0o1777) code, _, err = run(adopter, capsys, *checked_with_source(planned, work)) assert code == 0, err @@ -509,26 +557,34 @@ def test_the_sticky_bit_keeps_each_entry_to_its_owner( assert len(planned.calls) == 2 -def owned_by_another(adopter, monkeypatch, path: Path) -> int: - """`path` owned by another user: given away where this test runs as - root, and otherwise told so by `os.lstat` for that path alone. The - other user's id.""" - other = os.geteuid() + 1 if os.geteuid() else 65534 - if os.geteuid() == 0: - os.chown(path, other, -1) - return other +def told_of(monkeypatch, path: Path, uid: int, mode: int | None = None): + """`os.lstat` of `path` alone says `uid` owns it, and, where given, that + its mode is `mode`: an owner this test cannot make.""" lstat, which = os.lstat, os.path.realpath(path) - def as_another_owns_it(target, *args, **keywords): + def as_told(target, *args, **keywords): status = lstat(target, *args, **keywords) if keywords.get("dir_fd") is None and not args \ and os.fspath(target) == which: fields = list(status[:10]) - fields[4] = other + fields[4] = uid + if mode is not None: + fields[0] = stat.S_IFMT(status.st_mode) | mode return os.stat_result(fields) return status - monkeypatch.setattr(os, "lstat", as_another_owns_it) + monkeypatch.setattr(os, "lstat", as_told) + + +def owned_by_another(adopter, monkeypatch, path: Path) -> int: + """`path` owned by another user, neither this one nor root: given away + where this test runs as root, and otherwise told so by `os.lstat`. The + other user's id.""" + other = os.geteuid() + 1 if os.geteuid() else 65534 + if os.geteuid() == 0: + os.chown(path, other, -1) + else: + told_of(monkeypatch, path, other) return other @@ -548,8 +604,8 @@ def test_a_source_readonly_another_user_owns_is_refused( f"{SHARED}--work-dir {spelled(real(work))} holds `{SOURCE_READONLY}`, " "which is owned by "), err assert err.endswith( - ", not by this user, so another user could rename or replace the " - f"clone this run makes in it\nRemediation: {ANOTHER}\n"), err + f"{NOT_THEIRS}rename or replace the clone this run makes in it\n" + f"Remediation: {ANOTHER}\n"), err assert len(planned.calls) == 1 @@ -565,8 +621,8 @@ def test_a_work_dir_another_user_owns_is_refused( assert err.startswith( f"{SHARED}--work-dir {spelled(real(work))} is owned by "), err assert err.endswith( - ", not by this user, so another user could rename or replace what " - f"this run makes in it\nRemediation: {OWN}\n"), err + f"{NOT_THEIRS}rename or replace what this run makes in it\n" + f"Remediation: {OWN}\n"), err assert os.listdir(work) == [] assert len(planned.calls) == 1 @@ -600,3 +656,186 @@ def and_look(args, *rest, **keywords): assert code == 0, err assert len(calls) == 1 assert set(seen.values()) == {0} + + +# --- #843's review, finding 1: every directory above the work directory ------ + +def under(base: Path, mode: int) -> tuple[Path, Path]: + """`base/shared/mid/work`, not made, under `base/shared` at `mode` and + `base/shared/mid` at 0755: (the directory above, the work directory).""" + shared = base / "shared" + (shared / "mid").mkdir(parents=True) + os.chmod(shared, mode) + return shared, shared / "mid" / WORK + + +def renamed_above(work: Path, above: Path, why: str) -> str: + return (f"{SHARED}--work-dir {spelled(real(work))} is under " + f"{spelled(real(above))}, which {why}, so another user could " + f"rename {spelled(real(work))}, {RENAMES_ABOVE}") + + +@posix_only +@pytest.mark.parametrize("command", [PLAN_VERB, CHECK, EXECUTE]) +def test_a_directory_above_the_work_dir_every_user_can_write_in_is_refused( + adopter, monkeypatch, capsys, tmp_path, command): + """Two levels up, at 0777 with no sticky bit: another user could swap + the work directory between this run's claim and the identity it reads, + or under git's cleanup of a failed clone. Refused by name, with nothing + made, the missing work directory included. On #803's first head each + command used it.""" + planned = planned_from_github(adopter, monkeypatch, tmp_path) + shared, work = under(tmp_path, 0o777) + done = NOTHING_PUSHED if command == EXECUTE else "" + code, _, err = run(adopter, capsys, + *argv_for(command, planned, work, tmp_path)) + assert code == 2, err + assert err == (f"{renamed_above(work, shared, EVERY_USER)}\n" + f"Remediation: {done}{OWN}\n") + assert os.listdir(work.parent) == [] + assert len(planned.calls) == 1 + + +@posix_only +def test_a_directory_above_another_user_owns_is_refused( + adopter, monkeypatch, capsys, tmp_path): + planned = planned_from_github(adopter, monkeypatch, tmp_path) + shared, work = under(tmp_path, 0o755) + owned_by_another(adopter, monkeypatch, shared) + code, _, err = run(adopter, capsys, *checked_with_source(planned, work)) + assert code == 2, err + assert err.startswith(f"{SHARED}--work-dir {spelled(real(work))} is " + f"under {spelled(real(shared))}, which is owned " + "by "), err + assert err.endswith(f"{NOT_THEIRS}rename {spelled(real(work))}, " + f"{RENAMES_ABOVE}\nRemediation: {OWN}\n"), err + assert not os.path.lexists(work) + + +@posix_only +def test_a_root_owned_directory_above_the_work_dir_is_used( + adopter, monkeypatch, capsys, tmp_path): + """Root is trusted, as OpenSSH's StrictModes trusts it: `/` and + `/home` are root's. Emulated with `os.lstat` telling uid 0.""" + planned = planned_from_github(adopter, monkeypatch, tmp_path) + above, work = under(tmp_path, 0o755) + told_of(monkeypatch, above, 0) + code, _, err = run(adopter, capsys, *checked_with_source(planned, work)) + assert code == 0, err + assert os.listdir(work) == [] + + +@posix_only +def test_a_work_dir_given_through_a_link_in_a_shared_directory_is_its_target( + adopter, monkeypatch, capsys, tmp_path): + """The link sits in a directory every user can write in; its target is + in a chain nobody else can rename in. The run uses the target alone + from then on, so it is used, and the clone removed there.""" + planned = planned_from_github(adopter, monkeypatch, tmp_path) + symlinks_are_made(tmp_path) + shared, _ = under(tmp_path, 0o777) + target = tmp_path / "safe" / WORK + target.mkdir(parents=True) + os.symlink(target, shared / "link", target_is_directory=True) + code, _, err = run(adopter, capsys, + *checked_with_source(planned, shared / "link")) + assert code == 0, err + assert os.listdir(target) == [] + + +@posix_only +def test_a_work_dir_swapped_from_a_shared_directory_above_loses_nothing( + adopter, monkeypatch, capsys, tmp_path): + """#843's review, reproduced in process: right after the claim's + `mkdir`, the work directory is renamed and a symlink to `victimroot/` + put in its place, from the 0777 directory above it. On #803's first + head the claim recorded the victim's directory, and the removal + removed `victimroot/source-readonly/Thing` with its file. Now the run + is refused before anything is made, and the swap never happens.""" + planned = planned_from_github(adopter, monkeypatch, tmp_path) + symlinks_are_made(tmp_path) + shared, work = under(tmp_path, 0o777) + work.mkdir() + victimroot = tmp_path / "victimroot" + victim_holding(victimroot) + os.rename(victimroot / "victim", victimroot / SOURCE_READONLY) + precious = victimroot / SOURCE_READONLY / PROJECT / PRECIOUS + mkdir = os.mkdir + + def swapped_after(path, *args, **keywords): + mkdir(path, *args, **keywords) + if os.path.basename(path) == PROJECT \ + and not os.path.lexists(f"{work}.real"): + os.rename(work, f"{work}.real") + os.symlink(victimroot, work, target_is_directory=True) + + monkeypatch.setattr(os, "mkdir", swapped_after) + code, _, err = run(adopter, capsys, *checked_with_source(planned, work)) + assert code == 2, err + assert err.startswith(renamed_above(work, shared, EVERY_USER)), err + assert precious.read_text(encoding="utf-8") == MINE + assert not os.path.lexists(f"{work}.real") + + +# --- #843's review, finding 3: root is trusted --------------------------------- + +@posix_only +def test_a_sticky_work_dir_root_owns_is_used( + adopter, monkeypatch, capsys, tmp_path): + """`--work-dir /tmp`, emulated here: root's, at 1777. On #803's first + head it was refused as owned by `root`, though the sticky bit keeps + every entry to its owner.""" + planned = planned_from_github(adopter, monkeypatch, tmp_path) + work = tmp_path / "root-tmp" + work.mkdir() + os.chmod(work, 0o1777) + told_of(monkeypatch, work, 0) + code, _, err = run(adopter, capsys, *checked_with_source(planned, work)) + assert code == 0, err + assert os.listdir(work) == [] + + +@posix_only +def test_a_work_dir_root_owns_and_this_user_cannot_write_in_says_so( + adopter, monkeypatch, capsys, tmp_path, modes_hold): + """Root's, at 0755, as an earlier `sudo` run leaves one: the true + refusal, `adopt-work-dir-unwritable`, as on main. On #803's first head + it was `adopt-work-dir-shared`, which said another user could write + there.""" + calls = on_github(adopter, monkeypatch, tmp_path) + work = tmp_path / "root-made" + work.mkdir() + os.chmod(work, 0o555) + told_of(monkeypatch, work, 0, 0o755) + try: + code, _, err = run(adopter, capsys, *plan_args( + REPOSITORY, tmp_path / PLAN, WORK_FLAG, str(work))) + finally: + os.chmod(work, 0o755) + assert code == 2, err + assert err.startswith(f"{UNWRITABLE}--work-dir {spelled(real(work))} " + "cannot be written: "), err + assert calls == [] + + +# --- #843's review, finding 5: every directory a run makes is 0755 ------------ + +@posix_only +def test_a_work_dir_made_under_umask_000_is_made_0755_and_used(tmp_path): + """A missing, deep `--work-dir`, made by a `plan` under `umask 000`: + each directory it makes is 0755, so the run that made them uses them. + On #803's first head they were made 0777, and that run refused its own + work directory as `adopt-work-dir-shared`.""" + source = make_source_repo(tmp_path / PROJECT, tree=SMALL_TREE, edits=()) + made = tmp_path / "made" + work = made / "deep" / WORK + result = subprocess.run( # noqa: S603 - this interpreter, this tool + ["/bin/sh", "-c", 'umask 000 && exec "$0" "$@"', sys.executable, + str(ADOPT), PLAN_VERB, SOURCE_FLAG, str(source), *PLANNED, OUT, + str(tmp_path / PLAN), WORK_FLAG, str(work)], + stdin=subprocess.DEVNULL, capture_output=True, text=True, + check=False) + assert result.returncode == 0, result.stderr + modes = {path: stat.S_IMODE(os.lstat(path).st_mode) + for path in (made, made / "deep", work)} + assert set(modes.values()) == {0o755}, modes From 08f7a283675f9d25f43ab02fae645051951636b7 Mon Sep 17 00:00:00 2001 From: Brett Heap Date: Sun, 18 Oct 2026 02:22:34 +0000 Subject: [PATCH 4/4] adopt-project.py: a symlink loop or a file above the `--work-dir` keeps #650's `adopt-work-dir-unwritable`, and a refused directory above that is this user's names its sticky bit, so neither is called a directory every user can write in (#803) Fix round 2 on #843's re-verification (r803, at bba7e445): - N1: `_shared_above` stops at an entry above the work directory that is no directory: a file, or a symlink `realpath` could not follow, such as a loop. `_made_0755` then refuses it as #650 does, in main's words. A symlink reads as mode 0777, so a loop above was refused as `adopt-work-dir-shared`, and so was a file at 0666. - Where the directory above that is refused is this user's, its mode is the reason, and the way out also says: "Or, since is this user's, set its sticky bit (`chmod +t `): each entry in it then stays its owner's." Tests: tests/test_adopt_clone_claimed.py has 42 cases: the loop and the 0666 file above, in `plan`, `check` and `execute`; the sticky bit the refusal names, after which the same run is used; and the way out of a directory above another user owns names no sticky bit. Lane: openreposhape-1 (openRepoShape-1) Co-Authored-By: Claude Opus 5.5 --- adopt-project.py | 47 +++++++++++++++++--------- tests/test_adopt_clone_claimed.py | 55 ++++++++++++++++++++++++++++++- 2 files changed, 86 insertions(+), 16 deletions(-) diff --git a/adopt-project.py b/adopt-project.py index c502e10f..a89b4ae6 100755 --- a/adopt-project.py +++ b/adopt-project.py @@ -1831,12 +1831,18 @@ def _made_0755(path: Path, parents: bool = True) -> None: raise -def _shared_above(root: Path) -> tuple[Path, str] | None: +def _shared_above(root: Path) -> tuple[Path, str, bool] | None: """The first directory above `root`, from `/` down, that another user - can rename in (`_shared_with_another_user`), and why; None where there is - none, asked down to the deepest one that exists (#803). `root` is the - real path `_work_root` read, so each of these is a directory, never a - symlink, and from then on the run uses `root` alone. + can rename in (`_shared_with_another_user`), why, and whether it is this + user's own; None where there is none (#803). `root` is the real path + `_work_root` read, and from then on the run uses it alone. Each entry + above it is asked down to the deepest one that is a directory: where + one is missing, or is no directory -- a file, or a symlink `realpath` + could not follow, such as a loop -- the walk stops there, and + `_made_0755` refuses it as #650 does, `adopt-work-dir-unwritable`. A + symlink always reads as mode 0777, and a file another user owns would + otherwise be named as a directory they can rename in (#843's + re-verification, N1). THIS IS THE RULE OpenSSH's StrictModes HOLDS `authorized_keys` TO, but for two things: the sticky bit counts, which is what keeps `/tmp` open, @@ -1853,25 +1859,36 @@ def _shared_above(root: Path) -> tuple[Path, str] | None: status = os.lstat(above) except OSError: return None + if not stat.S_ISDIR(status.st_mode): + return None shared = _shared_with_another_user(status) if shared: - return above, shared + return above, shared, status.st_uid == os.geteuid() return None def _refuse_a_shared_chain(root: Path, done: str) -> None: """`adopt-work-dir-shared` for a `--work-dir` that has a directory above it another user can rename in (`_shared_above`), naming that directory - and why.""" + and why. Where that directory is this user's, its mode is the reason, + and the way out also names its sticky bit: set, it keeps each entry in + the directory its owner's, and takes nothing from this user (#843's + re-verification).""" found = _shared_above(root) - if found: - above, shared = found - raise _shared_work_dir( - f"--work-dir {_spelled(str(root))} is under " - f"{_spelled(str(above))}, which {shared}, so another user could " - f"rename {_spelled(str(root))}, or a directory between them, and " - "turn this run's removal onto another directory", done, - WORK_DIR_OWN) + if not found: + return + above, shared, mine = found + remedy = WORK_DIR_OWN + if mine: + remedy += (f" Or, since {_spelled(str(above))} is this user's, set " + "its sticky bit " + f"({_spelled('chmod +t ' + shlex.quote(str(above)))}): " + "each entry in it then stays its owner's.") + raise _shared_work_dir( + f"--work-dir {_spelled(str(root))} is under {_spelled(str(above))}, " + f"which {shared}, so another user could rename " + f"{_spelled(str(root))}, or a directory between them, and turn this " + "run's removal onto another directory", done, remedy) def _shared_work_dir(detail: str, done: str, remedy: str) -> Refusal: diff --git a/tests/test_adopt_clone_claimed.py b/tests/test_adopt_clone_claimed.py index 8fee0d31..697a7988 100644 --- a/tests/test_adopt_clone_claimed.py +++ b/tests/test_adopt_clone_claimed.py @@ -28,6 +28,7 @@ import errno import os +import shlex import shutil import stat import subprocess @@ -38,10 +39,12 @@ import pytest from conftest import ADOPT, WINDOWS_SKIP, make_source_repo, rmtree, spelled +from test_adopt_work_dir_refusal import refused_alike from test_adopt_source_clone_record import ( # noqa: F401 MINE, OCCUPIED, SOURCE_READONLY, WORK, WORK_FLAG, clone_dir, modes_hold, on_github, plan_args, run, symlinks_are_made, ) +from test_adopt_root_symlinks import planned as planned_locally from test_adopt_temp_work_roots import ( # noqa: F401 CHECK, EXECUTE, LOCAL_REMOTE_DIR, ORG, OUT, PLAN, PLAN_FLAG, PLAN_VERB, PLANNED, PROJECT, REMOTES, REPOSITORY, SMALL_TREE, SOURCE_FLAG, adopter, @@ -675,6 +678,14 @@ def renamed_above(work: Path, above: Path, why: str) -> str: f"rename {spelled(real(work))}, {RENAMES_ABOVE}") +def sticky_way_out(above: Path) -> str: + """What the way out adds where the directory above is this user's.""" + command = f"chmod +t {shlex.quote(str(real(above)))}" + return (f" Or, since {spelled(real(above))} is this user's, set its " + f"sticky bit ({spelled(command)}): each entry in it then stays " + "its owner's.") + + @posix_only @pytest.mark.parametrize("command", [PLAN_VERB, CHECK, EXECUTE]) def test_a_directory_above_the_work_dir_every_user_can_write_in_is_refused( @@ -691,11 +702,52 @@ def test_a_directory_above_the_work_dir_every_user_can_write_in_is_refused( *argv_for(command, planned, work, tmp_path)) assert code == 2, err assert err == (f"{renamed_above(work, shared, EVERY_USER)}\n" - f"Remediation: {done}{OWN}\n") + f"Remediation: {done}{OWN}{sticky_way_out(shared)}\n") assert os.listdir(work.parent) == [] assert len(planned.calls) == 1 +@posix_only +def test_the_sticky_bit_the_refusal_names_lets_the_same_run_through( + adopter, monkeypatch, capsys, tmp_path): + """The directory above is this user's, at 0777: the refusal names its + sticky bit, and once it is set, the same run is used, and its clone + removed (#843's re-verification).""" + planned = planned_from_github(adopter, monkeypatch, tmp_path) + shared, work = under(tmp_path, 0o777) + argv = checked_with_source(planned, work) + code, _, err = run(adopter, capsys, *argv) + assert code == 2, err + assert sticky_way_out(shared) in err + os.chmod(shared, 0o1777) + code, _, err = run(adopter, capsys, *argv) + assert code == 0, err + assert os.listdir(work) == [] + + +@pytest.mark.parametrize("command", [PLAN_VERB, CHECK, EXECUTE]) +@pytest.mark.parametrize("above", ["a-symlink-loop", "a-file-at-0666"]) +def test_a_loop_or_a_file_above_the_work_dir_keeps_its_650_refusal( + tmp_path, command, above): + """`--work-dir /loop/W` (`loop -> loop`) and `/notes.txt/W`, a + file every user can write: no directory can be made there, and each + command refuses it as #650 does, in main's words. On `bba7e445` each was + `adopt-work-dir-shared`, naming the link or the file as a directory + every user can write in: a symlink always reads as mode 0777.""" + planned_locally(tmp_path, {}, {}, {}) + if above == "a-symlink-loop": + in_the_way = tmp_path / "loop" + try: + in_the_way.symlink_to(in_the_way.name) + except OSError: + pytest.skip("this machine cannot make a symlink") + else: + in_the_way = tmp_path / "notes.txt" + in_the_way.write_text(MINE, encoding="utf-8") + os.chmod(in_the_way, 0o666) + refused_alike(command, tmp_path, in_the_way / WORK) + + @posix_only def test_a_directory_above_another_user_owns_is_refused( adopter, monkeypatch, capsys, tmp_path): @@ -709,6 +761,7 @@ def test_a_directory_above_another_user_owns_is_refused( "by "), err assert err.endswith(f"{NOT_THEIRS}rename {spelled(real(work))}, " f"{RENAMES_ABOVE}\nRemediation: {OWN}\n"), err + assert "chmod +t" not in err assert not os.path.lexists(work)