From 9bf2961d139bffd2fe066addb60d13aac45e23b7 Mon Sep 17 00:00:00 2001 From: didericis Date: Mon, 27 Jul 2026 10:25:14 -0400 Subject: [PATCH] fix: name an absolute path in the sudo re-run hint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The firecracker setup message told users to run sudo bot-bottle backend setup --backend=firecracker which fails for exactly the users who followed the documented install. sudo replaces PATH with sudoers' secure_path — /usr/local/sbin:/usr/local/bin: /usr/sbin:/usr/bin:/sbin:/bin on Debian and Ubuntu — which deliberately excludes user-writable directories. Both supported install paths land in one: pipx uses ~/.local/bin, and install.sh's venv fallback symlinks there. So the hint works for anyone who installed system-wide and breaks with "command not found" for everyone else, which is how it survives a read-through. Add bot_bottle/invocation.py: self_path() resolves the running entry point to an absolute path, and sudo_command() builds the copy-pasteable form. The one sudo recommendation in the tree now uses it. Non-sudo hints keep the bare `bot-bottle`, which is correct — the user reached them by running it. self_path() falls back to the bare name when argv[0] cannot be resolved (a `python -m` style invocation), because a slightly wrong hint beats a traceback raised while reporting some unrelated problem. Tested behaviourally rather than by scanning source: the first version of the test grepped the module and failed on the comment explaining why the bare form is wrong. It now drives _setup_systemd() as non-root and asserts what is actually printed. Verified the guard bites by restoring the bare form and watching it fail. Not verified end to end: this message only prints on the systemd path, so it is unreachable on macOS, where the rest of this work was tested. --- bot_bottle/backend/firecracker/setup.py | 7 +- bot_bottle/invocation.py | 48 ++++++++++++ tests/unit/test_invocation.py | 98 +++++++++++++++++++++++++ 3 files changed, 151 insertions(+), 2 deletions(-) create mode 100644 bot_bottle/invocation.py create mode 100644 tests/unit/test_invocation.py diff --git a/bot_bottle/backend/firecracker/setup.py b/bot_bottle/backend/firecracker/setup.py index f7245783..51cd483d 100644 --- a/bot_bottle/backend/firecracker/setup.py +++ b/bot_bottle/backend/firecracker/setup.py @@ -20,6 +20,7 @@ import subprocess import sys from pathlib import Path +from ... import invocation from ... import resources from . import netpool from . import util @@ -179,9 +180,11 @@ def _setup_systemd() -> None: f"sudo systemctl daemon-reload\n" f"sudo systemctl enable --now {netpool.SYSTEMD_UNIT}\n" ) + # Absolute path, not `sudo bot-bottle`: sudo's secure_path drops + # ~/.local/bin, where both pipx and install.sh put the entry point. sys.stderr.write( - f"\n(Or re-run this as root to install it directly: " - f"sudo bot-bottle backend setup --backend=firecracker)\n" + f"\n(Or re-run this as root to install it directly:\n" + f" {invocation.sudo_command('backend', 'setup', '--backend=firecracker')})\n" ) diff --git a/bot_bottle/invocation.py b/bot_bottle/invocation.py new file mode 100644 index 00000000..97de780b --- /dev/null +++ b/bot_bottle/invocation.py @@ -0,0 +1,48 @@ +"""How to tell a user to re-run this CLI. + +`bot-bottle …` is the right thing to print for anything the user runs as +themselves — it is on their PATH, since that is how they got here. + +Under `sudo` it is not. sudo replaces PATH with sudoers' `secure_path` +(`/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin` on Debian and +Ubuntu, similar elsewhere), which deliberately excludes user-writable +directories. Both supported install paths put the entry point in one of those: +pipx uses `~/.local/bin`, and `install.sh`'s venv fallback symlinks there too. +So `sudo bot-bottle …` fails with "command not found" for exactly the users who +followed the documented install, while working for anyone who happened to +install system-wide — which is why it survives review so easily. + +Naming the absolute path sidesteps secure_path entirely. +""" + +from __future__ import annotations + +import os +import shutil +import sys + + +def self_path() -> str: + """Absolute path to this CLI's entry point. + + Falls back to the bare name when the entry point cannot be resolved (an + unusual invocation such as `python -m`), because a slightly wrong hint is + better than a traceback while reporting an unrelated problem. + """ + argv0 = sys.argv[0] or "bot-bottle" + resolved = shutil.which(argv0) or argv0 + if not os.path.isabs(resolved): + if os.path.exists(resolved): + resolved = os.path.abspath(resolved) + else: + return "bot-bottle" + return resolved + + +def sudo_command(*args: str) -> str: + """A copy-pasteable `sudo …` invocation of this CLI. + + >>> sudo_command("backend", "setup", "--backend=firecracker") + 'sudo /home/u/.local/bin/bot-bottle backend setup --backend=firecracker' + """ + return " ".join(["sudo", self_path(), *args]) diff --git a/tests/unit/test_invocation.py b/tests/unit/test_invocation.py new file mode 100644 index 00000000..0c345a37 --- /dev/null +++ b/tests/unit/test_invocation.py @@ -0,0 +1,98 @@ +"""Unit: how the CLI tells users to re-run it under sudo. + +`sudo bot-bottle …` is wrong for the users who followed the documented +install: sudo's secure_path excludes ~/.local/bin, where both pipx and +install.sh put the entry point. These lock in the absolute-path form. +""" + +from __future__ import annotations + +import contextlib +import io +import os +import tempfile +import unittest +from pathlib import Path +from unittest import mock + +from bot_bottle import invocation + + +class TestSelfPath(unittest.TestCase): + def test_absolute_argv0_is_used_as_is(self): + with mock.patch.object(invocation.sys, "argv", ["/opt/venv/bin/bot-bottle"]): + self.assertEqual("/opt/venv/bin/bot-bottle", invocation.self_path()) + + def test_bare_name_is_resolved_through_path(self): + # The case that matters: invoked as `bot-bottle`, installed in a + # directory sudo would drop. + with tempfile.TemporaryDirectory() as d: + entry = Path(d, "bot-bottle") + entry.write_text("#!/bin/sh\n") + entry.chmod(0o755) + with mock.patch.object(invocation.sys, "argv", ["bot-bottle"]), \ + mock.patch.dict(os.environ, {"PATH": d}): + self.assertEqual(str(entry), invocation.self_path()) + + def test_relative_path_is_made_absolute(self): + with tempfile.TemporaryDirectory() as d: + entry = Path(d, "bot-bottle") + entry.write_text("#!/bin/sh\n") + entry.chmod(0o755) + cwd = os.getcwd() + try: + os.chdir(d) + with mock.patch.object(invocation.sys, "argv", ["./bot-bottle"]): + self.assertTrue(os.path.isabs(invocation.self_path())) + finally: + os.chdir(cwd) + + def test_unresolvable_entry_point_falls_back_to_the_name(self): + # `python -m`-style invocation, or an argv[0] that no longer exists. + # A slightly wrong hint beats a traceback raised while reporting some + # unrelated problem. + with mock.patch.object(invocation.sys, "argv", ["/nonexistent/gone"]), \ + mock.patch.object(invocation.shutil, "which", return_value=None): + self.assertEqual("/nonexistent/gone", invocation.self_path()) + with mock.patch.object(invocation.sys, "argv", [""]), \ + mock.patch.object(invocation.shutil, "which", return_value=None): + self.assertEqual("bot-bottle", invocation.self_path()) + + +class TestSudoCommand(unittest.TestCase): + def test_names_an_absolute_path_not_the_bare_command(self): + with mock.patch.object(invocation, "self_path", + return_value="/home/u/.local/bin/bot-bottle"): + cmd = invocation.sudo_command("backend", "setup", "--backend=firecracker") + self.assertEqual( + "sudo /home/u/.local/bin/bot-bottle backend setup --backend=firecracker", + cmd, + ) + # The regression this exists to prevent. + self.assertNotIn("sudo bot-bottle", cmd) + + +class TestFirecrackerSetupUsesIt(unittest.TestCase): + def test_root_reinvocation_hint_names_an_absolute_path(self): + # The message that prompted all this. Drive the real code path rather + # than scanning the source, which would also match the comment + # explaining why the bare form is wrong. + from bot_bottle.backend.firecracker import setup as fc_setup + + err, out = io.StringIO(), io.StringIO() + with mock.patch.object(fc_setup.os, "geteuid", return_value=501), \ + mock.patch.object(fc_setup.invocation, "self_path", + return_value="/home/u/.local/bin/bot-bottle"), \ + contextlib.redirect_stderr(err), contextlib.redirect_stdout(out): + fc_setup._setup_systemd() + + printed = err.getvalue() + self.assertIn( + "sudo /home/u/.local/bin/bot-bottle backend setup --backend=firecracker", + printed, + ) + self.assertNotIn("sudo bot-bottle", printed) + + +if __name__ == "__main__": + unittest.main()