From 6fedae9c7149ceda814c55f55c40a87115dce3e9 Mon Sep 17 00:00:00 2001 From: Ken Tobias <634380+l1a@users.noreply.github.com> Date: Mon, 24 Aug 2026 15:42:58 -0700 Subject: [PATCH] Refuse a path where a command name belongs install_completions.py takes a command NAME, but the output path is `directory / pattern.format(bin=binary)` and pathlib discards the left operand when the right side is absolute. Passing a path -- which the flag `--from-path` invites -- therefore wrote the completion script over the binary it was asked to read. Found in rusticprofile, where it destroyed a working binary on a host taking hourly backups. This copy was identical apart from line endings. `--from-path` runs [binary], so an absolute path works for the READ and only breaks the write: generation succeeds, then destroys its own input, exit 0. Now refused before any file is written. Watched failing: neutering the guard fails --self-test and fails just standard-check, which just check depends on. etr's own recipes pass bare names and were never at risk. Also repairs this file's worktree copy, which was CRLF while the index was LF (stale from before .gitattributes). Template v3. Assisted-By: Claude Opus 5 --- Cargo.lock | 2 +- Cargo.toml | 2 +- NOTES.md | 33 +++++++++++++++++++++ scripts/install_completions.py | 52 ++++++++++++++++++++++++++++++++-- 4 files changed, 85 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2389358..fdd30c0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -599,7 +599,7 @@ dependencies = [ [[package]] name = "etr" -version = "0.8.0" +version = "0.8.1" dependencies = [ "clap", "clap_complete", diff --git a/Cargo.toml b/Cargo.toml index 4d8fab8..e637535 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "etr" -version = "0.8.0" +version = "0.8.1" edition = "2024" description = "A Rust implementation of Eternal Terminal (et)" license = "GPL-3.0-only" diff --git a/NOTES.md b/NOTES.md index ab90361..fd3d82c 100644 --- a/NOTES.md +++ b/NOTES.md @@ -9,6 +9,39 @@ the link drops. This project uses **QUIC** (via the `quinn` crate) for the tran layer, which provides reliable, ordered, multiplexed streams with congestion control and TLS 1.3 built-in. +## Current state: v0.8.1 — the completions helper could overwrite the binary it was asked to read + +Tooling only; no runtime change (145 tests, unchanged). +`scripts/install_completions.py` → **template v3**. + +- **The defect, found in `rusticprofile` and propagated here because the file is vendored + byte-identically.** There it destroyed a working binary on a host taking hourly backups — a + 3.6 MB executable replaced by a 21 KB bash completion script. etr's copy carried the same bug. +- **The mechanism.** `binaries` are command *names*, and the output path is + `directory / pattern.format(bin=binary)` — but **`Path("/dest") / "/abs/path"` discards the left + operand**. An absolute argument therefore relocates every write out of the completion directory + and onto the path itself, which under `--from-path` is the installed binary. +- **It fails in the worst available order.** `--from-path` runs `[binary]`, so an absolute path + *works for the read* and only breaks the write: generation succeeds and then destroys its own + input, exit 0, nothing printed. And the flag is **called `--from-path`**, which invites precisely + the argument that breaks it — so documenting it would not have prevented it. +- **Fixed by making it unexpressible.** `reject_path_like()` refuses any argument containing a path + separator or resolving absolute, **before any file is written**, and names the correct form + (`install_completions.py etr --from-path`) in the error. +- **Watched failing in both places it is enforced**: neutering the condition fails `--self-test` + (*"rejects an absolute path — expected True, got False"*) and fails `just standard-check`, which + `just check` depends on. Re-running the original accident against a stand-in file now leaves it + **byte-identical** instead of clobbered. A fourth self-test case pins the *property* — joining a + directory with an absolute string yields the absolute string — so the check survives a rewrite of + the guard. +- **etr's own recipes were never at risk**: `install`, `install-tag` and `standard-check` pass + `{{BINS}}`, i.e. bare names. Checked rather than assumed. The exposure is anyone invoking the + helper directly, which is how it happened. +- *Incidentally repaired:* the worktree copy of this file was **CRLF while the index was LF** + (`git ls-files --eol` → `i/lf w/crlf`), stale from before `.gitattributes` landed. Writing the + canonical LF file brings the two back into agreement, which is why this is a 50-line diff rather + than a whole-file rewrite. + ## Current state: v0.8.0 — `-4`/`-6` address-family preference New in v0.8.0 (client + server feature; 112 → 145 tests, one new e2e recipe). diff --git a/scripts/install_completions.py b/scripts/install_completions.py index beb0312..1f1e094 100644 --- a/scripts/install_completions.py +++ b/scripts/install_completions.py @@ -3,7 +3,7 @@ # Copyright (C) 2026 l1a """Install shell completions for one or more binaries. Canonical across repos. -TEMPLATE v2 — vendored verbatim in rusticprofile, retch and etr. Change it here, +TEMPLATE v3 — vendored verbatim in rusticprofile, retch and etr. Change it here, bump TEMPLATE_VERSION, and propagate in each repo's own PR. `just standard-check` runs `--self-test` below, so the behavioural invariants are asserted rather than compared as text: three separate repositories cannot diff each other's files, but @@ -56,7 +56,7 @@ import sys from pathlib import Path -TEMPLATE_VERSION = 2 +TEMPLATE_VERSION = 3 def completion_dirs(env, home): @@ -102,6 +102,34 @@ def zsh_reads(directory): return str(directory) in res.stdout.splitlines() +def reject_path_like(binary): + """Refuse a path where a command NAME belongs. Invariant 4. + + `out = directory / pattern.format(bin=binary)` — and `Path("/dest") / "/abs/path"` + DISCARDS the left operand. So an absolute `binary` silently relocates the write out of + the completion directory and onto the path itself, which for `--from-path` is the + installed binary: the helper overwrites the very executable it was asked to read. + + Measured 2026-08-24: `install_completions.py ~/.cargo/bin/rusticprofile --from-path` + replaced a 3.6 MB binary with a 21 KB bash completion script, on a host taking hourly + backups. It fails in the worst possible way — `--from-path` runs `[binary]`, so an + absolute path WORKS for the read and only breaks the write. Generation succeeds, then + destroys its own input, and the flag is *called* `--from-path`, which invites exactly + the argument that breaks it. + + Made unexpressible rather than documented, on the precedent of refusing a snapshot-set + name beginning with `-`: a note in a docstring would not have stopped it, because the + person passing the path has already read the flag name and concluded it wants one. + """ + if os.sep in binary or (os.altsep and os.altsep in binary) or Path(binary).is_absolute(): + raise RuntimeError( + f"`{binary}` looks like a path; this takes a command NAME.\n" + f" Use: install_completions.py {Path(binary).name} --from-path\n" + " (--from-path means 'run the binary as resolved on PATH', not " + "'here is a path to the binary'.)" + ) + + def generate(binary, shell, out_path, repo_root, from_path=False): """Write one completion file, or raise. Invariant 3: a failure is not survivable. @@ -173,6 +201,23 @@ def check(name, got, want): # All six shells present, so a silently dropped one cannot pass. check("shell count", len(unix), 6) + # Invariant 4: a path where a NAME belongs must be refused, because pathlib would + # otherwise discard the destination directory and write over the binary itself. + def refuses(arg): + try: + reject_path_like(arg) + return False + except RuntimeError: + return True + + check("rejects an absolute path", refuses(str(Path.home() / ".cargo/bin/rusticprofile")), True) + check("rejects a relative path", refuses(f"bin{os.sep}rusticprofile"), True) + check("accepts a bare name", refuses("rusticprofile"), False) + # The failure it prevents, stated as the property rather than the mechanism: joining a + # directory with an absolute string must never be how an output path is chosen. + check("pathlib really does discard the left operand", + str(Path("/dest/dir") / "/abs/path"), str(Path("/abs/path"))) + if failures: print(f"self-test FAILED (template v{TEMPLATE_VERSION}):", file=sys.stderr) print("\n".join(failures), file=sys.stderr) @@ -197,6 +242,9 @@ def main(argv): repo_root = Path(__file__).resolve().parent.parent dirs = completion_dirs(os.environ, Path.home()) + for binary in binaries: + reject_path_like(binary) # invariant 4 — before ANY file is written + for binary in binaries: for shell, (directory, pattern) in dirs.items(): out = directory / pattern.format(bin=binary)