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)