From bc091ded1c4fe850a8d45b1d1774141bbfacc350 Mon Sep 17 00:00:00 2001 From: eimexdev Date: Wed, 21 Oct 2026 20:17:11 -0700 Subject: [PATCH 01/12] Stage the macOS payload where only root can change it The package installed its payload into /private/tmp/OpenBeatInstaller, which any local user could create or swap before the root postinstall copied the runtime from it. Stage it under /Library/Application Support/OpenBeat/Installer instead, with root ownership and no group or world write bits. The postinstall refuses to run unless that folder and every folder above it is a real folder owned by root and writable only by root, and removes it when done. The postinstall no longer aborts under set -u when USER is unset, and it stops with a clear error instead of installing for root or a system account when it cannot find the user to install for. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 1 + docs/development.md | 2 + scripts/build_installers.py | 18 ++++- scripts/macos_postinstall.sh | 89 +++++++++++++++++++++--- tests/test_build_installers.py | 11 +++ tests/test_installer_updates.py | 118 ++++++++++++++++++++++++++++++++ 6 files changed, 230 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e47eac..d9fb755 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ All notable OpenBeat changes should be summarized here for users. GitHub Release ## Unreleased - Installer updates now prepare and verify the new files before replacing the installed ones. If an update fails or is interrupted, the previous installation is kept or restored by running the installer again. +- The macOS installer stages its files in a folder only root can change, instead of a shared temporary folder, and stops with a clear error if it cannot tell which user to install for. - Release builds use one resolved commit for testing, building, and tagging, and pinned build dependencies. - When beat analysis or click-track rendering fails, OpenBeat now shows the helper's error instead of a missing-file error, and no longer imports an older click track as if it were new. - On Windows, OpenBeat should now work when the Windows user name or the media path contains non-English characters, such as `José` or `Música`. Subtitle export to such folders is not fixed yet. diff --git a/docs/development.md b/docs/development.md index b920759..3e58519 100644 --- a/docs/development.md +++ b/docs/development.md @@ -53,6 +53,8 @@ Replace `--upgrade` with `--upgrade-package ` to move a single package, or Both installers stage the new `Modules/OpenBeat` and `Scripts/Utility/OpenBeat` folders next to the installed ones as `.OpenBeat.new`, verify them against SHA-256 manifests written by `build_installers.py`, and only then rename the installed folders to `.OpenBeat.old` and the staged folders into place. Any failure renames `.OpenBeat.old` back, and the next run restores a `.OpenBeat.old` folder left by an interrupted update before it stages anything. The macOS package does this in `scripts/macos_postinstall.sh` and the Windows installer in `scripts/staged_install.py`; `tests/test_installer_updates.py` runs the same failed-update scenarios against both. +The macOS package installs its payload into `/Library/Application Support/OpenBeat/Installer`, and the postinstall script, which runs as root, copies it from there into the Resolve folder of the logged-in user. The script refuses to run unless that folder and every folder above it belong to root and only root can write to them, so another user on the Mac cannot place or swap the files it copies, and it deletes the folder afterwards. With nobody logged in, for example `sudo installer -pkg ... -target /` over SSH, it installs for the user who ran `sudo`; it stops with an error rather than install for root or a system account. + ## Run Tests Install Lua 5.1 or a later Lua release, then run `python -m unittest discover -s tests -v`. The suite tests timecode conversion, source selection, marker replacement and rollback, and subtitle timing without an open Resolve project. It also sends generated audio through the real CLI and checks both click-track modes. diff --git a/scripts/build_installers.py b/scripts/build_installers.py index 31cee9a..1882bb8 100644 --- a/scripts/build_installers.py +++ b/scripts/build_installers.py @@ -13,6 +13,10 @@ # Installed folders, relative to the Resolve Fusion support folder. The installers # (staged_install.py and macos_postinstall.sh) replace exactly these folders. PAYLOAD_DIRS = ("Modules/OpenBeat", "Scripts/Utility/OpenBeat") +# Where the macOS package installs its payload before the postinstall script copies +# it into the user's Resolve folder. It must match OPENBEAT_STAGING in +# macos_postinstall.sh, which refuses to run unless root alone can write to it. +MACOS_STAGING = "/Library/Application Support/OpenBeat/Installer" def run(cmd: list[str], cwd: Path | None = None) -> None: @@ -122,11 +126,21 @@ def build_payload(payload_root: Path, cli_binary: Path, platform: str) -> None: write_manifest(payload_root / relative) +def restrict_modes(root: Path) -> None: + """Make every folder and file writable only by its owner, which the package sets to root.""" + for path in [root, *root.rglob("*")]: + if path.is_symlink(): + raise ValueError(f"The macOS package payload cannot contain symlinks: {path}") + executable = path.is_dir() or path.stat().st_mode & 0o111 + path.chmod(0o755 if executable else 0o644) + + def create_macos_pkg(cli_binary: Path, version: str) -> Path: with tempfile.TemporaryDirectory(prefix="openbeat-macos-installer-") as tmp: tmp_path = Path(tmp) root_dir = tmp_path / "root" build_payload(root_dir / "payload", cli_binary, "macos") + restrict_modes(root_dir) scripts_dir = tmp_path / "scripts" scripts_dir.mkdir(parents=True, exist_ok=True) @@ -144,10 +158,12 @@ def create_macos_pkg(cli_binary: Path, version: str) -> Path: version, "--root", str(root_dir), + "--ownership", + "recommended", "--scripts", str(scripts_dir), "--install-location", - "/private/tmp/OpenBeatInstaller", + MACOS_STAGING, str(pkg_path), ] ) diff --git a/scripts/macos_postinstall.sh b/scripts/macos_postinstall.sh index a6ed439..c86d1ef 100755 --- a/scripts/macos_postinstall.sh +++ b/scripts/macos_postinstall.sh @@ -14,13 +14,24 @@ # and discard the .OpenBeat.old folders. While that marker exists the update is # complete, so a later run deletes leftover folders instead of restoring them. # +# The package installs the payload under OPENBEAT_STAGING on the target volume. +# That folder and every folder above it must belong to root and be writable only +# by root, so no other user can place or swap files the script then copies as +# root. The script refuses to run otherwise and removes the folder when done. +# # The installer runs openbeat_main. The other functions take explicit paths so # tests can source this file and run them against temporary folders. macOS runs # this with bash 3.2, so avoid arrays and newer bash features. OPENBEAT_TARGETS="Modules/OpenBeat Scripts/Utility/OpenBeat" +# Relative to the target volume; build_installers.py passes it to pkgbuild as +# --install-location. +OPENBEAT_STAGING="Library/Application Support/OpenBeat/Installer" +# Owner required for the staging folders. Tests override it after sourcing. +OPENBEAT_TRUSTED_UID=0 OPENBEAT_ROOT="" OPENBEAT_CREATED="" +OPENBEAT_USER="" openbeat_log() { echo "OpenBeat: $*" @@ -205,20 +216,82 @@ openbeat_install() { return "$status" } +# Succeeds if $1/$2 and every folder between it and volume $1 is a real folder +# owned by OPENBEAT_TRUSTED_UID that no other user can write to. +openbeat_check_staging() { + local path="${1%/}" remaining="$2" component owner_mode owner mode + while [ -n "$remaining" ]; do + component="${remaining%%/*}" + if [ "$component" = "$remaining" ]; then + remaining="" + else + remaining="${remaining#*/}" + fi + [ -n "$component" ] || continue + path="$path/$component" + if [ -L "$path" ] || [ ! -d "$path" ]; then + openbeat_log "Refusing to install: $path is not a folder" + return 1 + fi + owner_mode="$(stat -f '%u %Lp' "$path")" || owner_mode="" + owner="${owner_mode%% *}" + mode="${owner_mode#* }" + case "$mode" in + "" | *[!0-7]*) mode=777 ;; + esac + if [ "$owner" != "$OPENBEAT_TRUSTED_UID" ] || [ $((8#$mode & 8#22)) -ne 0 ]; then + openbeat_log "Refusing to install: $path must belong to root and be writable only by root" + return 1 + fi + done +} + +# Sets OPENBEAT_USER to the user to install for: the console user, or for a +# command-line install with nobody logged in, the user who ran sudo. Fails for +# root and system accounts rather than installing into their home folders. +openbeat_target_user() { + local user uid + OPENBEAT_USER="" + user="$(stat -f %Su /dev/console 2>/dev/null)" || user="" + if [ -z "$user" ] || [ "$user" = "root" ] || [ "$user" = "loginwindow" ]; then + user="${SUDO_USER:-${USER:-}}" + fi + uid="" + if [ -n "$user" ] && [ "$user" != "root" ]; then + uid="$(id -u "$user" 2>/dev/null)" || uid="" + fi + case "$uid" in + "" | *[!0-9]*) uid=0 ;; + esac + if [ "$uid" -lt 500 ]; then + openbeat_log "Could not tell which user to install OpenBeat for. Log in as the user who runs DaVinci Resolve and run the installer again." + return 1 + fi + OPENBEAT_USER="$user" +} + openbeat_main() { set -euo pipefail - local installer_root="$3/private/tmp/OpenBeatInstaller" - local console_user user_home resolve_root status=0 - console_user="$(stat -f %Su /dev/console)" - if [[ -z "${console_user}" || "${console_user}" == "root" ]]; then - console_user="${SUDO_USER:-$USER}" + local volume="${3:-/}" staging user_home resolve_root status=0 + staging="${volume%/}/$OPENBEAT_STAGING" + openbeat_check_staging "$volume" "$OPENBEAT_STAGING" || return 1 + + if ! openbeat_target_user; then + rm -rf "$staging" + return 1 + fi + if ! user_home="$(dscl . -read /Users/"$OPENBEAT_USER" NFSHomeDirectory | awk '{print $2}')" || + [ -z "$user_home" ] || [ "$user_home" = "/" ] || [ "$user_home" = "/var/root" ] || [ ! -d "$user_home" ]; then + openbeat_log "Could not find the home folder of $OPENBEAT_USER" + rm -rf "$staging" + return 1 fi - user_home="$(dscl . -read /Users/"${console_user}" NFSHomeDirectory | awk '{print $2}')" resolve_root="$user_home/Library/Application Support/Blackmagic Design/DaVinci Resolve/Fusion" - openbeat_install "$installer_root/payload" "$resolve_root" "$console_user:staff" || status=$? - rm -rf "$installer_root" + openbeat_install "$staging/payload" "$resolve_root" "$OPENBEAT_USER:staff" || status=$? + rm -rf "$staging" + rmdir "${staging%/*}" 2>/dev/null || true return "$status" } diff --git a/tests/test_build_installers.py b/tests/test_build_installers.py index fe2f73c..4405903 100644 --- a/tests/test_build_installers.py +++ b/tests/test_build_installers.py @@ -48,6 +48,11 @@ def test_macos_package_runs_the_staged_postinstall_script(self) -> None: def fake_pkgbuild(cmd: list[str], cwd: Path | None = None) -> None: package_root = Path(cmd[cmd.index("--root") + 1]) scripts = Path(cmd[cmd.index("--scripts") + 1]) + self.assertEqual(cmd[cmd.index("--install-location") + 1], build_installers.MACOS_STAGING) + self.assertEqual(cmd[cmd.index("--ownership") + 1], "recommended") + for path in [package_root, *package_root.rglob("*")]: + self.assertEqual(path.stat().st_mode & 0o022, 0, path) + self.assertEqual((package_root / "payload" / "Modules" / "OpenBeat" / "bin" / "openbeat").stat().st_mode & 0o777, 0o755) postinstall = scripts / "postinstall" self.assertEqual(postinstall.read_bytes(), (build_installers.ROOT / "scripts" / "macos_postinstall.sh").read_bytes()) self.assertTrue(postinstall.stat().st_mode & 0o111) @@ -63,6 +68,12 @@ def fake_pkgbuild(cmd: list[str], cwd: Path | None = None) -> None: result = build_installers.create_macos_pkg(binary, "0.0.2") self.assertEqual(result.read_bytes(), b"package") + def test_macos_package_stages_where_the_postinstall_script_checks(self) -> None: + self.assertTrue(build_installers.MACOS_STAGING.startswith("/Library/")) + postinstall = (build_installers.ROOT / "scripts" / "macos_postinstall.sh").read_text() + self.assertIn(f'OPENBEAT_STAGING="{build_installers.MACOS_STAGING.lstrip("/")}"', postinstall) + self.assertNotIn("/tmp", postinstall) + def test_windows_cli_runtime_is_onedir_and_windowed(self) -> None: with TemporaryDirectory() as tmpdir: dist_root = Path(tmpdir) / "dist" diff --git a/tests/test_installer_updates.py b/tests/test_installer_updates.py index aa41bbd..e682708 100644 --- a/tests/test_installer_updates.py +++ b/tests/test_installer_updates.py @@ -7,6 +7,7 @@ import errno import importlib.util import os +import pwd import shutil import subprocess import sys @@ -53,6 +54,21 @@ time.sleep(300) """ +# Stand-ins for the macOS commands openbeat_main uses to find the user and inspect the staging folder. +BASH_MACOS = r""" +stat() { + if [ "$1" = "-f" ] && [ "$2" = "%Su" ]; then + [ -n "${CONSOLE_USER:-}" ] && printf '%s\n' "$CONSOLE_USER" + elif [ "$1" = "-f" ] && [ "$2" = "%u %Lp" ]; then + command stat -c '%u %a' "$3" + else + command stat "$@" + fi +} +dscl() { printf 'NFSHomeDirectory: %s\n' "$FAKE_HOME"; } +chown() { echo "chown $2" >&2; } +""" + KILL_DRIVER = """ import os, sys from pathlib import Path @@ -280,6 +296,108 @@ def test_sourcing_the_script_does_not_run_the_installer(self) -> None: self.assertEqual(result.stdout.strip(), "sourced", result.stderr) +@unittest.skipUnless(BASH and shutil.which("shasum"), "bash and shasum are required to run the macOS postinstall script") +@unittest.skipUnless(os.getuid() >= 500, "the postinstall script refuses to install for system accounts") +class MacOSPostinstallMainTests(unittest.TestCase): + """The package entry point: where it takes the payload from and whom it installs for.""" + + def setUp(self) -> None: + temporary = TemporaryDirectory(prefix="openbeat-postinstall-") + self.addCleanup(temporary.cleanup) + self.work = Path(temporary.name) + self.user = pwd.getpwuid(os.getuid()).pw_name + self.home = self.work / "home" + self.home.mkdir() + self.volume = self.work / "volume" + self.staging = self.volume.joinpath(*build_installers.MACOS_STAGING.strip("/").split("/")) + runtime = self.work / "openbeat" + runtime.write_bytes(b"new runtime") + build_installers.build_payload(self.staging / "payload", runtime, "macos") + path = self.staging + while path != self.volume: + path.chmod(0o755) + path = path.parent + + def run_main(self, trusted_uid: int | None = None, **environment: str) -> subprocess.CompletedProcess[str]: + env = {key: value for key, value in os.environ.items() if key not in ("USER", "SUDO_USER")} + env.update({"CONSOLE_USER": self.user, "FAKE_HOME": str(self.home)}, **environment) + uid = os.getuid() if trusted_uid is None else trusted_uid + command = f'set -u; source "$1"; {BASH_MACOS}\nOPENBEAT_TRUSTED_UID={uid}\nopenbeat_main "$2/OpenBeat.pkg" "$3" "$2"' + return subprocess.run( + [BASH, "-c", command, "postinstall", POSTINSTALL, self.volume, build_installers.MACOS_STAGING], + env=env, + text=True, + capture_output=True, + timeout=300, + ) + + @property + def modules(self) -> Path: + return self.home / "Library/Application Support/Blackmagic Design/DaVinci Resolve/Fusion/Modules/OpenBeat" + + def assert_refused(self, result: subprocess.CompletedProcess[str], message: str) -> None: + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn(message, result.stdout) + self.assertNotIn("unbound variable", result.stderr) + self.assertFalse((self.home / "Library").exists()) + + def test_installs_for_console_user_and_removes_staged_payload(self) -> None: + result = self.run_main() + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertEqual((self.modules / "bin" / "openbeat").read_bytes(), b"new runtime") + self.assertIn(f"chown {self.user}:staff", result.stderr) + self.assertFalse(self.staging.exists()) + self.assertFalse(self.staging.parent.exists()) + self.assertTrue(self.staging.parent.parent.is_dir()) + + def test_command_line_install_with_nobody_logged_in_uses_sudo_user(self) -> None: + result = self.run_main(CONSOLE_USER="root", SUDO_USER=self.user) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertTrue((self.modules / "bin" / "openbeat").is_file()) + + def test_unknown_user_fails_clearly_without_installing_for_root(self) -> None: + for console_user in ("root", "loginwindow", ""): + with self.subTest(console_user=console_user): + self.setUp() + result = self.run_main(CONSOLE_USER=console_user) + self.assert_refused(result, "Could not tell which user to install OpenBeat for") + self.assertFalse(self.staging.exists()) + + def test_system_account_is_not_an_install_target(self) -> None: + system_user = next((entry.pw_name for entry in pwd.getpwall() if 0 < entry.pw_uid < 500), None) + if system_user is None: + self.skipTest("no system account to test with") + result = self.run_main(CONSOLE_USER="root", SUDO_USER=system_user) + self.assert_refused(result, "Could not tell which user to install OpenBeat for") + + def test_refuses_staging_folders_other_users_can_write(self) -> None: + for folder, mode in ((self.staging, 0o775), (self.staging, 0o757), (self.volume / "Library", 0o777)): + with self.subTest(folder=folder, mode=oct(mode)): + folder.chmod(mode) + result = self.run_main() + folder.chmod(0o755) + self.assert_refused(result, f"Refusing to install: {folder} must belong to root") + # The payload is left alone rather than deleted as root. + self.assertTrue((self.staging / "payload").is_dir()) + + def test_refuses_staging_owned_by_another_user(self) -> None: + result = self.run_main(trusted_uid=0) + self.assert_refused(result, "must belong to root and be writable only by root") + self.assertTrue(self.staging.is_dir()) + + def test_refuses_symlinked_staging_folder(self) -> None: + elsewhere = self.work / "elsewhere" + self.staging.rename(elsewhere) + self.staging.symlink_to(elsewhere, target_is_directory=True) + result = self.run_main() + self.assert_refused(result, f"Refusing to install: {self.staging} is not a folder") + self.assertTrue((elsewhere / "payload").is_dir()) + + def test_refuses_missing_staging_folder(self) -> None: + shutil.rmtree(self.staging) + self.assert_refused(self.run_main(), "is not a folder") + + class WindowsStagedInstallUpdateTests(UpdateScenarios, unittest.TestCase): platform = "windows" runtime = "bin/openbeat/openbeat.exe" From dd43e6edf01ceed5e7ec4a70a7f5051997d5f2f8 Mon Sep 17 00:00:00 2001 From: eimexdev Date: Wed, 21 Oct 2026 21:44:08 -0700 Subject: [PATCH 02/12] Install into the user's folder as that user The postinstall verified files in the user's Resolve folder and then changed them as root, so a process running as the user could swap in a symlink after verification and have root make a protected file readable. Root now only checks the staging folder and picks the user; copying, verification, and replacement run as that user through sudo. Co-Authored-By: Claude Opus 5.5 --- scripts/macos_postinstall.sh | 36 ++++++++++++++++++++++++++++----- tests/test_installer_updates.py | 19 ++++++++++++----- 2 files changed, 45 insertions(+), 10 deletions(-) diff --git a/scripts/macos_postinstall.sh b/scripts/macos_postinstall.sh index c86d1ef..6f68053 100755 --- a/scripts/macos_postinstall.sh +++ b/scripts/macos_postinstall.sh @@ -155,7 +155,7 @@ openbeat_write_config() { } openbeat_install_steps() { - local payload="$1" root="$2" owner="$3" relative parent staged + local payload="$1" root="$2" relative parent staged if ! openbeat_recover "$root"; then openbeat_log "A previous update was interrupted and could not be completed or undone automatically" @@ -175,8 +175,7 @@ openbeat_install_steps() { staged="$root/Modules/.OpenBeat.new" if ! { chmod 755 "$staged/bin/openbeat" && - openbeat_write_config "$staged" "$root/Modules/OpenBeat/bin/openbeat" && - chown -R "$owner" "$staged" "$root/Scripts/Utility/.OpenBeat.new"; }; then + openbeat_write_config "$staged" "$root/Modules/OpenBeat/bin/openbeat"; }; then openbeat_log "Could not configure the new files; the existing installation was not changed" openbeat_restore "$root" return 1 @@ -204,7 +203,8 @@ openbeat_install_steps() { return 0 } -# Installs payload folder $1 into Resolve Fusion folder $2, owned by $3 (user:group). +# Installs payload folder $1 into Resolve Fusion folder $2, with the permissions of +# whoever runs it. openbeat_main runs it as the target user (see openbeat_as_user). openbeat_install() { local status=0 OPENBEAT_ROOT="$2" @@ -216,6 +216,32 @@ openbeat_install() { return "$status" } +# What openbeat_install needs, passed to the shell openbeat_install_as_user starts. +OPENBEAT_INSTALL_FUNCTIONS="openbeat_log openbeat_exists openbeat_rename openbeat_verify openbeat_restore + openbeat_finish openbeat_recover openbeat_abort openbeat_swap openbeat_write_config + openbeat_install_steps openbeat_install" + +# Runs command $2... as user $1. Tests replace this function. +openbeat_as_user() { + local user="$1" + shift + /usr/bin/sudo -u "$user" -H "$@" +} + +# Installs payload $2 into Fusion root $3 as user $1. The Fusion folder is in the +# user's home folder, where their own processes can swap files and symlinks at any +# time, so root must not copy, verify, or change permissions there: a swapped path +# could make root change or reveal a file the user cannot. +openbeat_install_as_user() { + local definitions + # shellcheck disable=SC2086 # The list is split into function names on purpose. + definitions="$(declare -f $OPENBEAT_INSTALL_FUNCTIONS)" || return 1 + openbeat_as_user "$1" /bin/bash -c "set -euo pipefail +OPENBEAT_TARGETS='$OPENBEAT_TARGETS' +$definitions +openbeat_install \"\$1\" \"\$2\"" openbeat-postinstall "$2" "$3" +} + # Succeeds if $1/$2 and every folder between it and volume $1 is a real folder # owned by OPENBEAT_TRUSTED_UID that no other user can write to. openbeat_check_staging() { @@ -289,7 +315,7 @@ openbeat_main() { fi resolve_root="$user_home/Library/Application Support/Blackmagic Design/DaVinci Resolve/Fusion" - openbeat_install "$staging/payload" "$resolve_root" "$OPENBEAT_USER:staff" || status=$? + openbeat_install_as_user "$OPENBEAT_USER" "$staging/payload" "$resolve_root" || status=$? rm -rf "$staging" rmdir "${staging%/*}" 2>/dev/null || true return "$status" diff --git a/tests/test_installer_updates.py b/tests/test_installer_updates.py index e682708..bfdbff8 100644 --- a/tests/test_installer_updates.py +++ b/tests/test_installer_updates.py @@ -66,7 +66,7 @@ fi } dscl() { printf 'NFSHomeDirectory: %s\n' "$FAKE_HOME"; } -chown() { echo "chown $2" >&2; } +openbeat_as_user() { echo "as user $1" >&2; shift; "$@"; } """ KILL_DRIVER = """ @@ -258,10 +258,9 @@ def run_script(self, payload: Path, root: Path, **faults: str) -> subprocess.Com environment = {**os.environ, "COPY_FAULT": faults.pop("copy", "")} for name, path in faults.items(): environment[name.upper()] = str(path) - owner = f"{os.getuid()}:{os.getgid()}" - command = f'set -u; source "$1"; {BASH_FAULTS}\nopenbeat_install "$2" "$3" "$4"' + command = f'set -u; source "$1"; {BASH_FAULTS}\nopenbeat_install "$2" "$3"' return subprocess.run( - [BASH, "-c", command, "postinstall", POSTINSTALL, payload, root, owner], + [BASH, "-c", command, "postinstall", POSTINSTALL, payload, root], env=environment, text=True, capture_output=True, @@ -345,11 +344,21 @@ def test_installs_for_console_user_and_removes_staged_payload(self) -> None: result = self.run_main() self.assertEqual(result.returncode, 0, result.stdout + result.stderr) self.assertEqual((self.modules / "bin" / "openbeat").read_bytes(), b"new runtime") - self.assertIn(f"chown {self.user}:staff", result.stderr) + self.assertIn(f"as user {self.user}", result.stderr) self.assertFalse(self.staging.exists()) self.assertFalse(self.staging.parent.exists()) self.assertTrue(self.staging.parent.parent.is_dir()) + def test_files_in_the_users_folder_are_only_touched_as_the_user(self) -> None: + # Root may only check the staging folder and pick the user; openbeat_install runs + # in the shell started as that user, which has no access to root's functions. + result = self.run_main() + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + script = POSTINSTALL.read_text() + main = script[script.index("openbeat_main() {"):] + self.assertNotIn("openbeat_install ", main) + self.assertNotRegex(main, r"\b(chmod|chown|cp|mv)\b") + def test_command_line_install_with_nobody_logged_in_uses_sudo_user(self) -> None: result = self.run_main(CONSOLE_USER="root", SUDO_USER=self.user) self.assertEqual(result.returncode, 0, result.stdout + result.stderr) From 296a6cde93cc2a4b4a7a67eec96cdc16d121d6ad Mon Sep 17 00:00:00 2001 From: eimexdev Date: Wed, 21 Oct 2026 21:46:04 -0700 Subject: [PATCH 03/12] Describe the user-level macOS install in development notes Co-Authored-By: Claude Opus 5.5 --- docs/development.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/development.md b/docs/development.md index 3e58519..4398104 100644 --- a/docs/development.md +++ b/docs/development.md @@ -53,7 +53,7 @@ Replace `--upgrade` with `--upgrade-package ` to move a single package, or Both installers stage the new `Modules/OpenBeat` and `Scripts/Utility/OpenBeat` folders next to the installed ones as `.OpenBeat.new`, verify them against SHA-256 manifests written by `build_installers.py`, and only then rename the installed folders to `.OpenBeat.old` and the staged folders into place. Any failure renames `.OpenBeat.old` back, and the next run restores a `.OpenBeat.old` folder left by an interrupted update before it stages anything. The macOS package does this in `scripts/macos_postinstall.sh` and the Windows installer in `scripts/staged_install.py`; `tests/test_installer_updates.py` runs the same failed-update scenarios against both. -The macOS package installs its payload into `/Library/Application Support/OpenBeat/Installer`, and the postinstall script, which runs as root, copies it from there into the Resolve folder of the logged-in user. The script refuses to run unless that folder and every folder above it belong to root and only root can write to them, so another user on the Mac cannot place or swap the files it copies, and it deletes the folder afterwards. With nobody logged in, for example `sudo installer -pkg ... -target /` over SSH, it installs for the user who ran `sudo`; it stops with an error rather than install for root or a system account. +The macOS package installs its payload into `/Library/Application Support/OpenBeat/Installer`, and the postinstall script, which runs as root, checks that folder and works out the logged-in user, then copies the payload into that user's Resolve folder as the user (through `sudo -u`), so nothing in their home folder is changed with root's permissions. The script refuses to run unless that folder and every folder above it belong to root and only root can write to them, so another user on the Mac cannot place or swap the files it copies, and it deletes the folder afterwards. With nobody logged in, for example `sudo installer -pkg ... -target /` over SSH, it installs for the user who ran `sudo`; it stops with an error rather than install for root or a system account. ## Run Tests From 2d29a1fa9828d486579811be9073208c8182fdea Mon Sep 17 00:00:00 2001 From: eimexdev Date: Wed, 21 Oct 2026 21:53:18 -0700 Subject: [PATCH 04/12] Handle older root-owned folders and unusual home paths OpenBeat 0.0.2 created missing Resolve folders as root, which the user-level install cannot write into; give those back to the user with chown -h. Read home folders with spaces from dscl, including its wrapped form, and escape the runtime path in the generated Lua config. Co-Authored-By: Claude Opus 5.5 --- scripts/macos_postinstall.sh | 37 +++++++++++++++++++++++-- tests/test_installer_updates.py | 49 ++++++++++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 4 deletions(-) diff --git a/scripts/macos_postinstall.sh b/scripts/macos_postinstall.sh index 6f68053..3d3b770 100755 --- a/scripts/macos_postinstall.sh +++ b/scripts/macos_postinstall.sh @@ -151,7 +151,11 @@ openbeat_swap() { } openbeat_write_config() { - printf 'return {\n python_bin = "%s",\n}\n' "$2" > "$1/OpenBeatConfig.local.lua" + local path + # Escape the path for a Lua string. sed, because bash 3.2 and 5 treat backslashes + # in ${var//pattern/replacement} differently. + path="$(printf '%s' "$2" | sed -e 's/\\/\\\\/g' -e 's/"/\\"/g')" || return 1 + printf 'return {\n python_bin = "%s",\n}\n' "$path" > "$1/OpenBeatConfig.local.lua" } openbeat_install_steps() { @@ -242,6 +246,28 @@ $definitions openbeat_install \"\$1\" \"\$2\"" openbeat-postinstall "$2" "$3" } +# OpenBeat 0.0.2 and earlier created missing Resolve folders as root. Give any such +# folder, between home folder $1 and the Fusion subfolders OpenBeat installs into, +# back to user $2 so the install can run as that user. chown -h never follows a +# symlink, so a folder swapped for one changes only the link. +openbeat_reclaim_folders() { + local folder uid + for folder in "Library/Application Support/Blackmagic Design" \ + "Library/Application Support/Blackmagic Design/DaVinci Resolve" \ + "Library/Application Support/Blackmagic Design/DaVinci Resolve/Fusion" \ + "Library/Application Support/Blackmagic Design/DaVinci Resolve/Fusion/Modules" \ + "Library/Application Support/Blackmagic Design/DaVinci Resolve/Fusion/Scripts" \ + "Library/Application Support/Blackmagic Design/DaVinci Resolve/Fusion/Scripts/Utility"; do + if [ -d "$1/$folder" ] && [ ! -L "$1/$folder" ]; then + uid="$(stat -f %u "$1/$folder")" || return 1 + if [ "$uid" = 0 ] && ! chown -h "$2:staff" "$1/$folder"; then + openbeat_log "Could not give $1/$folder back to $2" + return 1 + fi + fi + done +} + # Succeeds if $1/$2 and every folder between it and volume $1 is a real folder # owned by OPENBEAT_TRUSTED_UID that no other user can write to. openbeat_check_staging() { @@ -307,7 +333,9 @@ openbeat_main() { rm -rf "$staging" return 1 fi - if ! user_home="$(dscl . -read /Users/"$OPENBEAT_USER" NFSHomeDirectory | awk '{print $2}')" || + # dscl prints a value with spaces on the line after the attribute name. + if ! user_home="$(dscl . -read /Users/"$OPENBEAT_USER" NFSHomeDirectory | + awk 'NR == 1 { sub(/^NFSHomeDirectory:/, "") } { sub(/^ /, ""); printf "%s", $0 }')" || [ -z "$user_home" ] || [ "$user_home" = "/" ] || [ "$user_home" = "/var/root" ] || [ ! -d "$user_home" ]; then openbeat_log "Could not find the home folder of $OPENBEAT_USER" rm -rf "$staging" @@ -315,7 +343,10 @@ openbeat_main() { fi resolve_root="$user_home/Library/Application Support/Blackmagic Design/DaVinci Resolve/Fusion" - openbeat_install_as_user "$OPENBEAT_USER" "$staging/payload" "$resolve_root" || status=$? + openbeat_reclaim_folders "$user_home" "$OPENBEAT_USER" || status=$? + if [ "$status" -eq 0 ]; then + openbeat_install_as_user "$OPENBEAT_USER" "$staging/payload" "$resolve_root" || status=$? + fi rm -rf "$staging" rmdir "${staging%/*}" 2>/dev/null || true return "$status" diff --git a/tests/test_installer_updates.py b/tests/test_installer_updates.py index bfdbff8..5b71bb8 100644 --- a/tests/test_installer_updates.py +++ b/tests/test_installer_updates.py @@ -61,11 +61,23 @@ [ -n "${CONSOLE_USER:-}" ] && printf '%s\n' "$CONSOLE_USER" elif [ "$1" = "-f" ] && [ "$2" = "%u %Lp" ]; then command stat -c '%u %a' "$3" + elif [ "$1" = "-f" ] && [ "$2" = "%u" ]; then + case " ${FAKE_ROOT_OWNED:-} " in + *" $3 "*) echo 0 ;; + *) command stat -c '%u' "$3" ;; + esac else command stat "$@" fi } -dscl() { printf 'NFSHomeDirectory: %s\n' "$FAKE_HOME"; } +dscl() { + if [ -n "${FAKE_DSCL_WRAPPED:-}" ]; then + printf 'NFSHomeDirectory:\n %s\n' "$FAKE_HOME" + else + printf 'NFSHomeDirectory: %s\n' "$FAKE_HOME" + fi +} +chown() { echo "chown $*" >&2; } openbeat_as_user() { echo "as user $1" >&2; shift; "$@"; } """ @@ -359,6 +371,41 @@ def test_files_in_the_users_folder_are_only_touched_as_the_user(self) -> None: self.assertNotIn("openbeat_install ", main) self.assertNotRegex(main, r"\b(chmod|chown|cp|mv)\b") + def test_root_owned_resolve_folders_from_older_installers_are_given_back(self) -> None: + fusion = self.home / "Library/Application Support/Blackmagic Design/DaVinci Resolve/Fusion" + (fusion / "Scripts" / "Utility").mkdir(parents=True) + (fusion / "Modules").mkdir() + result = self.run_main(FAKE_ROOT_OWNED=f"{fusion / 'Modules'} {fusion / 'Scripts' / 'Utility'}") + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertIn(f"chown -h {self.user}:staff {fusion / 'Modules'}\n", result.stderr) + self.assertIn(f"chown -h {self.user}:staff {fusion / 'Scripts' / 'Utility'}\n", result.stderr) + self.assertEqual(result.stderr.count("chown"), 2) + self.assertTrue((self.modules / "bin" / "openbeat").is_file()) + + def test_home_folder_with_spaces_and_quotes(self) -> None: + for wrapped in ("", "1"): + with self.subTest(wrapped=bool(wrapped)): + home = self.work / f'User Homes{wrapped}' / 'al "ice"\\x' + home.mkdir(parents=True) + self.home = home + if wrapped: + self.setUp() + self.home = home + result = self.run_main(FAKE_HOME=str(home), FAKE_DSCL_WRAPPED=wrapped) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + config = (self.modules / "OpenBeatConfig.local.lua").read_text() + runtime = str(self.modules / "bin" / "openbeat") + self.assertIn(runtime.replace("\\", "\\\\").replace('"', '\\"'), config) + if shutil.which("lua5.1"): + # The config must load in Lua and give back the exact path. + check = self.work / "check_config.lua" + check.write_text( + f"local c = dofile({str(self.modules / 'OpenBeatConfig.local.lua')!r})\n" + f"assert(c.python_bin == {runtime!r}, c.python_bin)\n" + ) + lua = subprocess.run(["lua5.1", str(check)], text=True, capture_output=True) + self.assertEqual(lua.returncode, 0, lua.stdout + lua.stderr) + def test_command_line_install_with_nobody_logged_in_uses_sudo_user(self) -> None: result = self.run_main(CONSOLE_USER="root", SUDO_USER=self.user) self.assertEqual(result.returncode, 0, result.stdout + result.stderr) From 91052b81974be7d817fb782261ecd1a7db8ea6b5 Mon Sep 17 00:00:00 2001 From: eimexdev Date: Wed, 21 Oct 2026 22:02:16 -0700 Subject: [PATCH 05/12] Never follow the user's symlinks when reclaiming folders Walk into each Resolve folder with cd, checking it is a real folder and the one checked by device and inode, then chown -h relative to it. A symlinked Blackmagic Design folder, or one swapped in mid-walk, can no longer lead root to give system folders to the user. Co-Authored-By: Claude Opus 5.5 --- scripts/macos_postinstall.sh | 62 ++++++++++++++++++++++----------- tests/test_installer_updates.py | 27 +++++++++++--- 2 files changed, 64 insertions(+), 25 deletions(-) diff --git a/scripts/macos_postinstall.sh b/scripts/macos_postinstall.sh index 3d3b770..2fa3ca4 100755 --- a/scripts/macos_postinstall.sh +++ b/scripts/macos_postinstall.sh @@ -246,28 +246,50 @@ $definitions openbeat_install \"\$1\" \"\$2\"" openbeat-postinstall "$2" "$3" } -# OpenBeat 0.0.2 and earlier created missing Resolve folders as root. Give any such -# folder, between home folder $1 and the Fusion subfolders OpenBeat installs into, -# back to user $2 so the install can run as that user. chown -h never follows a -# symlink, so a folder swapped for one changes only the link. -openbeat_reclaim_folders() { - local folder uid - for folder in "Library/Application Support/Blackmagic Design" \ - "Library/Application Support/Blackmagic Design/DaVinci Resolve" \ - "Library/Application Support/Blackmagic Design/DaVinci Resolve/Fusion" \ - "Library/Application Support/Blackmagic Design/DaVinci Resolve/Fusion/Modules" \ - "Library/Application Support/Blackmagic Design/DaVinci Resolve/Fusion/Scripts" \ - "Library/Application Support/Blackmagic Design/DaVinci Resolve/Fusion/Scripts/Utility"; do - if [ -d "$1/$folder" ] && [ ! -L "$1/$folder" ]; then - uid="$(stat -f %u "$1/$folder")" || return 1 - if [ "$uid" = 0 ] && ! chown -h "$2:staff" "$1/$folder"; then - openbeat_log "Could not give $1/$folder back to $2" - return 1 - fi - fi - done +# Enters folder $1 of the current folder, refusing a symlink and checking the folder +# entered is the one checked, so a folder swapped in between is never followed. +openbeat_enter() { + local before + [ -d "$1" ] && [ ! -L "$1" ] || return 1 + before="$(stat -f '%d:%i' "$1")" || return 1 + cd -P -- "$1" || return 1 + [ "$(stat -f '%d:%i' .)" = "$before" ] +} + +# Gives folder $1 of the current folder to user $2 if it is a root-owned real folder. +# chown -h changes only the link if the folder is swapped for one in between. +openbeat_reclaim() { + local uid + [ -d "$1" ] && [ ! -L "$1" ] || return 0 + uid="$(stat -f %u "$1")" || return 1 + if [ "$uid" = 0 ] && ! chown -h "$2:staff" "./$1"; then + openbeat_log "Could not give $PWD/$1 back to $2" + return 1 + fi } +# OpenBeat 0.0.2 and earlier created missing Resolve folders as root, which the +# install, running as the user, cannot write into. Gives those folders, from +# Blackmagic Design down to the Fusion folders OpenBeat installs into, back to user +# $2 of home folder $1. It walks into each folder with openbeat_enter rather than +# using paths through the user's home folder, which they can change at any time, +# and stops at a symlink or missing folder: nothing root-owned from an older +# OpenBeat lies beyond one. +openbeat_reclaim_folders() ( + cd -P -- "$1" || exit 1 + openbeat_enter "Library" && openbeat_enter "Application Support" || exit 0 + openbeat_reclaim "Blackmagic Design" "$2" || exit 1 + openbeat_enter "Blackmagic Design" || exit 0 + openbeat_reclaim "DaVinci Resolve" "$2" || exit 1 + openbeat_enter "DaVinci Resolve" || exit 0 + openbeat_reclaim "Fusion" "$2" || exit 1 + openbeat_enter "Fusion" || exit 0 + openbeat_reclaim "Modules" "$2" || exit 1 + openbeat_reclaim "Scripts" "$2" || exit 1 + openbeat_enter "Scripts" || exit 0 + openbeat_reclaim "Utility" "$2" || exit 1 +) + # Succeeds if $1/$2 and every folder between it and volume $1 is a real folder # owned by OPENBEAT_TRUSTED_UID that no other user can write to. openbeat_check_staging() { diff --git a/tests/test_installer_updates.py b/tests/test_installer_updates.py index 5b71bb8..bd1bd5b 100644 --- a/tests/test_installer_updates.py +++ b/tests/test_installer_updates.py @@ -62,10 +62,14 @@ elif [ "$1" = "-f" ] && [ "$2" = "%u %Lp" ]; then command stat -c '%u %a' "$3" elif [ "$1" = "-f" ] && [ "$2" = "%u" ]; then - case " ${FAKE_ROOT_OWNED:-} " in - *" $3 "*) echo 0 ;; + # Resolve symlinked parent folders, as the real stat does when it opens the path. + set -- "$1" "$2" "$(cd -P -- "$(dirname -- "$3")" && pwd -P)/$(basename -- "$3")" + case "|${FAKE_ROOT_OWNED:-}|" in + *"|$3|"*) echo 0 ;; *) command stat -c '%u' "$3" ;; esac + elif [ "$1" = "-f" ] && [ "$2" = "%d:%i" ]; then + command stat -c '%d:%i' "$3" else command stat "$@" fi @@ -375,13 +379,26 @@ def test_root_owned_resolve_folders_from_older_installers_are_given_back(self) - fusion = self.home / "Library/Application Support/Blackmagic Design/DaVinci Resolve/Fusion" (fusion / "Scripts" / "Utility").mkdir(parents=True) (fusion / "Modules").mkdir() - result = self.run_main(FAKE_ROOT_OWNED=f"{fusion / 'Modules'} {fusion / 'Scripts' / 'Utility'}") + result = self.run_main(FAKE_ROOT_OWNED=f"{fusion / 'Modules'}|{fusion / 'Scripts' / 'Utility'}") self.assertEqual(result.returncode, 0, result.stdout + result.stderr) - self.assertIn(f"chown -h {self.user}:staff {fusion / 'Modules'}\n", result.stderr) - self.assertIn(f"chown -h {self.user}:staff {fusion / 'Scripts' / 'Utility'}\n", result.stderr) + self.assertIn(f"chown -h {self.user}:staff ./Modules\n", result.stderr) + self.assertIn(f"chown -h {self.user}:staff ./Utility\n", result.stderr) self.assertEqual(result.stderr.count("chown"), 2) self.assertTrue((self.modules / "bin" / "openbeat").is_file()) + def test_symlinked_resolve_folder_is_not_followed_as_root(self) -> None: + # The user links Blackmagic Design to a folder they cannot change, here standing + # in for the system-wide one, whose root-owned folders must stay root's. + system = self.work / "system" / "Blackmagic Design" + (system / "DaVinci Resolve" / "Fusion" / "Modules").mkdir(parents=True) + support = self.home / "Library" / "Application Support" + support.mkdir(parents=True) + (support / "Blackmagic Design").symlink_to(system, target_is_directory=True) + fusion = system / "DaVinci Resolve" / "Fusion" + result = self.run_main(FAKE_ROOT_OWNED=f"{system}|{system / 'DaVinci Resolve'}|{fusion}|{fusion / 'Modules'}") + self.assertNotIn("chown", result.stderr) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + def test_home_folder_with_spaces_and_quotes(self) -> None: for wrapped in ("", "1"): with self.subTest(wrapped=bool(wrapped)): From ecd81a02eb8aafec2ea093ce01b8a974c01485a3 Mon Sep 17 00:00:00 2001 From: eimexdev Date: Wed, 21 Oct 2026 22:09:17 -0700 Subject: [PATCH 06/12] Check the target volume itself before trusting staging Validate the volume root as well as the folders below it, so on a volume other users can write to, no checked folder can be swapped afterwards. Import pwd only where it exists, so the test suite also loads on Windows. Co-Authored-By: Claude Opus 5.5 --- scripts/macos_postinstall.sh | 43 ++++++++++++++++++++------------- tests/test_installer_updates.py | 16 ++++++++++-- 2 files changed, 40 insertions(+), 19 deletions(-) diff --git a/scripts/macos_postinstall.sh b/scripts/macos_postinstall.sh index 2fa3ca4..8b18163 100755 --- a/scripts/macos_postinstall.sh +++ b/scripts/macos_postinstall.sh @@ -290,10 +290,32 @@ openbeat_reclaim_folders() ( openbeat_reclaim "Utility" "$2" || exit 1 ) -# Succeeds if $1/$2 and every folder between it and volume $1 is a real folder -# owned by OPENBEAT_TRUSTED_UID that no other user can write to. +# Succeeds if folder $1 is a real folder owned by OPENBEAT_TRUSTED_UID that no other +# user can write to, so nobody else can rename or replace anything in it. +openbeat_check_trusted() { + local owner_mode owner mode + if [ -L "$1" ] || [ ! -d "$1" ]; then + openbeat_log "Refusing to install: $1 is not a folder" + return 1 + fi + owner_mode="$(stat -f '%u %Lp' "$1")" || owner_mode="" + owner="${owner_mode%% *}" + mode="${owner_mode#* }" + case "$mode" in + "" | *[!0-7]*) mode=777 ;; + esac + if [ "$owner" != "$OPENBEAT_TRUSTED_UID" ] || [ $((8#$mode & 8#22)) -ne 0 ]; then + openbeat_log "Refusing to install: $1 must belong to root and be writable only by root" + return 1 + fi +} + +# Succeeds if volume $1, $1/$2, and every folder between them pass +# openbeat_check_trusted. Checking the volume too means no folder on the path can be +# swapped after it is checked. openbeat_check_staging() { - local path="${1%/}" remaining="$2" component owner_mode owner mode + local path="${1%/}" remaining="$2" component + openbeat_check_trusted "${path:-/}" || return 1 while [ -n "$remaining" ]; do component="${remaining%%/*}" if [ "$component" = "$remaining" ]; then @@ -303,20 +325,7 @@ openbeat_check_staging() { fi [ -n "$component" ] || continue path="$path/$component" - if [ -L "$path" ] || [ ! -d "$path" ]; then - openbeat_log "Refusing to install: $path is not a folder" - return 1 - fi - owner_mode="$(stat -f '%u %Lp' "$path")" || owner_mode="" - owner="${owner_mode%% *}" - mode="${owner_mode#* }" - case "$mode" in - "" | *[!0-7]*) mode=777 ;; - esac - if [ "$owner" != "$OPENBEAT_TRUSTED_UID" ] || [ $((8#$mode & 8#22)) -ne 0 ]; then - openbeat_log "Refusing to install: $path must belong to root and be writable only by root" - return 1 - fi + openbeat_check_trusted "$path" || return 1 done } diff --git a/tests/test_installer_updates.py b/tests/test_installer_updates.py index bd1bd5b..784f923 100644 --- a/tests/test_installer_updates.py +++ b/tests/test_installer_updates.py @@ -7,7 +7,6 @@ import errno import importlib.util import os -import pwd import shutil import subprocess import sys @@ -18,6 +17,11 @@ from scripts import build_installers, staged_install +try: + import pwd +except ImportError: # Windows; the macOS postinstall tests are skipped there. + pwd = None + ROOT = Path(__file__).resolve().parents[1] POSTINSTALL = ROOT / "scripts" / "macos_postinstall.sh" @@ -312,7 +316,7 @@ def test_sourcing_the_script_does_not_run_the_installer(self) -> None: @unittest.skipUnless(BASH and shutil.which("shasum"), "bash and shasum are required to run the macOS postinstall script") -@unittest.skipUnless(os.getuid() >= 500, "the postinstall script refuses to install for system accounts") +@unittest.skipUnless(pwd is not None and os.getuid() >= 500, "needs a POSIX user account that is not a system account") class MacOSPostinstallMainTests(unittest.TestCase): """The package entry point: where it takes the payload from and whom it installs for.""" @@ -332,6 +336,7 @@ def setUp(self) -> None: while path != self.volume: path.chmod(0o755) path = path.parent + self.volume.chmod(0o755) def run_main(self, trusted_uid: int | None = None, **environment: str) -> subprocess.CompletedProcess[str]: env = {key: value for key, value in os.environ.items() if key not in ("USER", "SUDO_USER")} @@ -453,6 +458,13 @@ def test_refuses_staging_folders_other_users_can_write(self) -> None: # The payload is left alone rather than deleted as root. self.assertTrue((self.staging / "payload").is_dir()) + def test_refuses_volume_root_other_users_can_write(self) -> None: + self.volume.chmod(0o777) + self.addCleanup(self.volume.chmod, 0o755) + self.assert_refused(self.run_main(), f"Refusing to install: {self.volume} must belong to root") + # The payload is left alone rather than deleted as root. + self.assertTrue(self.staging.is_dir()) + def test_refuses_staging_owned_by_another_user(self) -> None: result = self.run_main(trusted_uid=0) self.assert_refused(result, "must belong to root and be writable only by root") From d71ce1d5093edd0100e26bbe5b8bd64e12f3ba7f Mon Sep 17 00:00:00 2001 From: eimexdev Date: Wed, 21 Oct 2026 22:15:47 -0700 Subject: [PATCH 07/12] Check the home folder before walking into it as root Enter the home folder like the folders below it: a real folder, the one checked by device and inode, and owned by the target user. Otherwise skip reclaiming; the install itself still runs as the user. Co-Authored-By: Claude Opus 5.5 --- scripts/macos_postinstall.sh | 20 +++++++++++++++----- tests/test_installer_updates.py | 22 +++++++++++++++++++++- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/scripts/macos_postinstall.sh b/scripts/macos_postinstall.sh index 8b18163..2fc16b8 100755 --- a/scripts/macos_postinstall.sh +++ b/scripts/macos_postinstall.sh @@ -246,14 +246,16 @@ $definitions openbeat_install \"\$1\" \"\$2\"" openbeat-postinstall "$2" "$3" } -# Enters folder $1 of the current folder, refusing a symlink and checking the folder -# entered is the one checked, so a folder swapped in between is never followed. +# Enters folder $1, refusing a symlink and checking the folder entered is the one +# checked, so a folder swapped in between is never followed. With $2, the folder +# must also belong to that user ID. openbeat_enter() { local before [ -d "$1" ] && [ ! -L "$1" ] || return 1 before="$(stat -f '%d:%i' "$1")" || return 1 cd -P -- "$1" || return 1 - [ "$(stat -f '%d:%i' .)" = "$before" ] + [ "$(stat -f '%d:%i' .)" = "$before" ] || return 1 + [ -z "${2:-}" ] || [ "$(stat -f %u .)" = "$2" ] } # Gives folder $1 of the current folder to user $2 if it is a root-owned real folder. @@ -271,12 +273,20 @@ openbeat_reclaim() { # OpenBeat 0.0.2 and earlier created missing Resolve folders as root, which the # install, running as the user, cannot write into. Gives those folders, from # Blackmagic Design down to the Fusion folders OpenBeat installs into, back to user -# $2 of home folder $1. It walks into each folder with openbeat_enter rather than +# $2 of home folder $1. It walks into each folder, the home folder included, with +# openbeat_enter rather than # using paths through the user's home folder, which they can change at any time, # and stops at a symlink or missing folder: nothing root-owned from an older # OpenBeat lies beyond one. openbeat_reclaim_folders() ( - cd -P -- "$1" || exit 1 + local uid + uid="$(id -u "$2")" || exit 1 + # The home folder may sit in a folder the user can change, so it gets the same + # checks, and must belong to the user; otherwise there is nothing to reclaim. + if ! openbeat_enter "$1" "$uid"; then + openbeat_log "Not checking folder ownership: $1 is not a folder that belongs to $2" + exit 0 + fi openbeat_enter "Library" && openbeat_enter "Application Support" || exit 0 openbeat_reclaim "Blackmagic Design" "$2" || exit 1 openbeat_enter "Blackmagic Design" || exit 0 diff --git a/tests/test_installer_updates.py b/tests/test_installer_updates.py index 784f923..88da72a 100644 --- a/tests/test_installer_updates.py +++ b/tests/test_installer_updates.py @@ -67,7 +67,11 @@ command stat -c '%u %a' "$3" elif [ "$1" = "-f" ] && [ "$2" = "%u" ]; then # Resolve symlinked parent folders, as the real stat does when it opens the path. - set -- "$1" "$2" "$(cd -P -- "$(dirname -- "$3")" && pwd -P)/$(basename -- "$3")" + if [ "$3" = "." ]; then + set -- "$1" "$2" "$(pwd -P)" + else + set -- "$1" "$2" "$(cd -P -- "$(dirname -- "$3")" && pwd -P)/$(basename -- "$3")" + fi case "|${FAKE_ROOT_OWNED:-}|" in *"|$3|"*) echo 0 ;; *) command stat -c '%u' "$3" ;; @@ -391,6 +395,22 @@ def test_root_owned_resolve_folders_from_older_installers_are_given_back(self) - self.assertEqual(result.stderr.count("chown"), 2) self.assertTrue((self.modules / "bin" / "openbeat").is_file()) + def test_home_folder_that_is_a_symlink_or_not_the_users_is_not_entered_as_root(self) -> None: + for linked in (True, False): + with self.subTest(linked=linked): + self.setUp() + # Stands in for / or another account's folder, with root-owned Resolve folders. + system = self.work / "system" + (system / "Library" / "Application Support" / "Blackmagic Design").mkdir(parents=True) + home = system + if linked: + home = self.work / "linked home" + home.symlink_to(system, target_is_directory=True) + root_owned = f"{system}|{system / 'Library' / 'Application Support' / 'Blackmagic Design'}" + result = self.run_main(FAKE_HOME=str(home), FAKE_ROOT_OWNED=root_owned) + self.assertNotIn("chown", result.stderr) + self.assertIn("Not checking folder ownership", result.stdout) + def test_symlinked_resolve_folder_is_not_followed_as_root(self) -> None: # The user links Blackmagic Design to a folder they cannot change, here standing # in for the system-wide one, whose root-owned folders must stay root's. From 1159e98a8b7f8a1560b01d82b5a5b2d9d3c54696 Mon Sep 17 00:00:00 2001 From: eimexdev Date: Wed, 21 Oct 2026 22:21:19 -0700 Subject: [PATCH 08/12] Reject staging folders an ACL lets other users change The mode bits miss macOS access control entries, so also refuse a staging folder or ancestor with an allow entry granting write, delete, or chown. Deny entries, which / and /Library carry, are fine. Co-Authored-By: Claude Opus 5.5 --- scripts/macos_postinstall.sh | 18 ++++++++++++++++-- tests/test_installer_updates.py | 13 +++++++++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/scripts/macos_postinstall.sh b/scripts/macos_postinstall.sh index 2fc16b8..6f33680 100755 --- a/scripts/macos_postinstall.sh +++ b/scripts/macos_postinstall.sh @@ -300,10 +300,18 @@ openbeat_reclaim_folders() ( openbeat_reclaim "Utility" "$2" || exit 1 ) +# Prints the access control entries of $1, one per line. Tests replace this function. +openbeat_acl_entries() { + local listing + listing="$(ls -lde "$1")" || return 1 + printf '%s\n' "$listing" | sed 1d +} + # Succeeds if folder $1 is a real folder owned by OPENBEAT_TRUSTED_UID that no other -# user can write to, so nobody else can rename or replace anything in it. +# user can write to, through its mode or an access control entry, so nobody else can +# rename or replace anything in it. openbeat_check_trusted() { - local owner_mode owner mode + local owner_mode owner mode acl if [ -L "$1" ] || [ ! -d "$1" ]; then openbeat_log "Refusing to install: $1 is not a folder" return 1 @@ -318,6 +326,12 @@ openbeat_check_trusted() { openbeat_log "Refusing to install: $1 must belong to root and be writable only by root" return 1 fi + # macOS folders such as / and /Library carry only deny entries. + if ! acl="$(openbeat_acl_entries "$1")" || + printf '%s\n' "$acl" | grep -Eq ' allow .*(write|append|add_file|add_subdirectory|delete|chown)'; then + openbeat_log "Refusing to install: an access control entry lets other users change $1" + return 1 + fi } # Succeeds if volume $1, $1/$2, and every folder between them pass diff --git a/tests/test_installer_updates.py b/tests/test_installer_updates.py index 88da72a..8f16e11 100644 --- a/tests/test_installer_updates.py +++ b/tests/test_installer_updates.py @@ -91,6 +91,12 @@ } chown() { echo "chown $*" >&2; } openbeat_as_user() { echo "as user $1" >&2; shift; "$@"; } +openbeat_acl_entries() { + echo " 0: group:everyone deny delete" + case "|${FAKE_ACL_WRITABLE:-}|" in + *"|$1|"*) echo " 1: user:mallory allow add_file,add_subdirectory,delete_child" ;; + esac +} """ KILL_DRIVER = """ @@ -485,6 +491,13 @@ def test_refuses_volume_root_other_users_can_write(self) -> None: # The payload is left alone rather than deleted as root. self.assertTrue(self.staging.is_dir()) + def test_refuses_folders_an_access_control_entry_lets_others_change(self) -> None: + for folder in (self.volume, self.staging.parent): + with self.subTest(folder=folder): + result = self.run_main(FAKE_ACL_WRITABLE=str(folder)) + self.assert_refused(result, f"an access control entry lets other users change {folder}") + self.assertTrue(self.staging.is_dir()) + def test_refuses_staging_owned_by_another_user(self) -> None: result = self.run_main(trusted_uid=0) self.assert_refused(result, "must belong to root and be writable only by root") From 4d09a8f565a5f4810f97eae9f2a3629bd07c7308 Mon Sep 17 00:00:00 2001 From: eimexdev Date: Wed, 21 Oct 2026 22:29:50 -0700 Subject: [PATCH 09/12] Refuse staged files the payload manifest does not list The macOS staging folder is a fixed path, so files left by an earlier, interrupted package could be copied along with a newer one. Verification now also requires the staged folder to hold exactly the manifest's files. Co-Authored-By: Claude Opus 5.5 --- scripts/macos_postinstall.sh | 10 +++++++++- tests/test_installer_updates.py | 10 ++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/scripts/macos_postinstall.sh b/scripts/macos_postinstall.sh index 6f33680..31650ee 100755 --- a/scripts/macos_postinstall.sh +++ b/scripts/macos_postinstall.sh @@ -52,7 +52,7 @@ openbeat_rename() { # Checks that directory $1 holds every file listed in manifest $2 with the recorded digest. openbeat_verify() { - local directory="$1" manifest="$2" line digest name result + local directory="$1" manifest="$2" line digest name result listed present if [ ! -s "$manifest" ]; then openbeat_log "The installer payload manifest $manifest is missing or empty" return 1 @@ -77,6 +77,14 @@ openbeat_verify() { printf '%s\n' "$result" | grep -v ': OK$' return 1 fi + # The staging folder is a fixed path, so files left there by an earlier package + # could be copied along; install nothing the manifest does not list. + listed="$(sed 's/^[0-9a-f]* //' "$manifest" | LC_ALL=C sort)" + present="$(cd "$directory" && find . ! -type d | sed 's|^\./||' | LC_ALL=C sort)" || return 1 + if [ "$listed" != "$present" ]; then + openbeat_log "The prepared files in $directory include files the installer payload does not list" + return 1 + fi } # Puts every .OpenBeat.old folder under Fusion root $1 back in place and removes diff --git a/tests/test_installer_updates.py b/tests/test_installer_updates.py index 8f16e11..96fee25 100644 --- a/tests/test_installer_updates.py +++ b/tests/test_installer_updates.py @@ -304,6 +304,16 @@ def install(self, payload: Path, root: Path, **faults: str) -> bool: self.assertIn("fault: ", result.stderr, result.stdout + result.stderr) return result.returncode == 0 + def test_files_left_in_the_staging_folder_by_an_older_package_are_refused(self) -> None: + before = self.make_previous_install() + stale = self.work / "stale" + shutil.copytree(self.payload, stale) + (stale / "Scripts" / "Utility" / "OpenBeat" / "Removed Action.lua").write_text("-- old package\n") + result = self.run_script(stale, self.root) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn("include files the installer payload does not list", result.stdout) + self.assertEqual(tree(self.root), before) + def test_termination_during_replacement_restores_previous_installation(self) -> None: before = self.make_previous_install() result = self.run_script(self.payload, self.root, term_move_to=self.root / "Scripts" / "Utility" / "OpenBeat") From dcdec28d13608a13acf63c2449c565ea724e53e4 Mon Sep 17 00:00:00 2001 From: eimexdev Date: Thu, 22 Oct 2026 00:37:40 -0700 Subject: [PATCH 10/12] Stop changing folder owners as root; ask the user instead Root cannot tell which root-owned folders in a home folder an earlier OpenBeat made, and the user can rename folders there, so reclaiming them by name could hand the user an unrelated root-owned folder. Drop the reclaim step. The install, running as the user, now checks up front that it can write to the Resolve folders and otherwise stops with the chown command to run. Co-Authored-By: Claude Opus 5.5 --- docs/install.md | 2 + scripts/macos_postinstall.sh | 76 ++++++++------------------------- tests/test_installer_updates.py | 68 ++++++----------------------- 3 files changed, 33 insertions(+), 113 deletions(-) diff --git a/docs/install.md b/docs/install.md index a082db3..6e7220c 100644 --- a/docs/install.md +++ b/docs/install.md @@ -46,6 +46,8 @@ Install the newer package over the old one, then restart Resolve. The installer The installer copies the new files next to your current installation and checks them before replacing anything. If copying, checking, or replacing fails, for example because the disk is full or Resolve still has OpenBeat files open, your current installation is kept and the installer reports that it did not complete. Close Resolve and run the installer again. On macOS, Installer's log (**Window > Installer Log**) shows the reason. +On macOS, OpenBeat 0.0.1 could leave some Resolve folders owned by root if Resolve had never been opened. If the installer log says OpenBeat cannot write to a folder, run the `sudo chown` command it shows in Terminal, then run the installer again. + If an update is cut off part way through, for example by a power failure, run the installer again. It first puts back the previous installation it had set aside, then repeats the update. It also removes any `.OpenBeat.new`, `.OpenBeat.old`, or `.OpenBeat.discard` folders left next to the OpenBeat folders. ## Uninstall diff --git a/scripts/macos_postinstall.sh b/scripts/macos_postinstall.sh index 31650ee..96fdb5b 100755 --- a/scripts/macos_postinstall.sh +++ b/scripts/macos_postinstall.sh @@ -166,9 +166,24 @@ openbeat_write_config() { printf 'return {\n python_bin = "%s",\n}\n' "$path" > "$1/OpenBeatConfig.local.lua" } +# Fails, explaining the fix, if a Resolve folder OpenBeat installs into exists but +# the installing user cannot write to it. OpenBeat 0.0.1 created missing ones as +# root. Root does not change their owner: it cannot tell which root-owned folders in +# a home folder OpenBeat made, and the user can rename folders there. +openbeat_check_writable() { + local root="$1" folder + for folder in "$root" "$root/Modules" "$root/Scripts" "$root/Scripts/Utility"; do + if [ -e "$folder" ] && [ ! -w "$folder" ]; then + openbeat_log "OpenBeat cannot write to $folder, which may belong to root after an earlier OpenBeat installer. In Terminal, run: sudo chown \"$(id -un)\" \"$folder\" and then run this installer again." + return 1 + fi + done +} + openbeat_install_steps() { local payload="$1" root="$2" relative parent staged + openbeat_check_writable "$root" || return 1 if ! openbeat_recover "$root"; then openbeat_log "A previous update was interrupted and could not be completed or undone automatically" return 1 @@ -231,7 +246,7 @@ openbeat_install() { # What openbeat_install needs, passed to the shell openbeat_install_as_user starts. OPENBEAT_INSTALL_FUNCTIONS="openbeat_log openbeat_exists openbeat_rename openbeat_verify openbeat_restore openbeat_finish openbeat_recover openbeat_abort openbeat_swap openbeat_write_config - openbeat_install_steps openbeat_install" + openbeat_check_writable openbeat_install_steps openbeat_install" # Runs command $2... as user $1. Tests replace this function. openbeat_as_user() { @@ -254,60 +269,6 @@ $definitions openbeat_install \"\$1\" \"\$2\"" openbeat-postinstall "$2" "$3" } -# Enters folder $1, refusing a symlink and checking the folder entered is the one -# checked, so a folder swapped in between is never followed. With $2, the folder -# must also belong to that user ID. -openbeat_enter() { - local before - [ -d "$1" ] && [ ! -L "$1" ] || return 1 - before="$(stat -f '%d:%i' "$1")" || return 1 - cd -P -- "$1" || return 1 - [ "$(stat -f '%d:%i' .)" = "$before" ] || return 1 - [ -z "${2:-}" ] || [ "$(stat -f %u .)" = "$2" ] -} - -# Gives folder $1 of the current folder to user $2 if it is a root-owned real folder. -# chown -h changes only the link if the folder is swapped for one in between. -openbeat_reclaim() { - local uid - [ -d "$1" ] && [ ! -L "$1" ] || return 0 - uid="$(stat -f %u "$1")" || return 1 - if [ "$uid" = 0 ] && ! chown -h "$2:staff" "./$1"; then - openbeat_log "Could not give $PWD/$1 back to $2" - return 1 - fi -} - -# OpenBeat 0.0.2 and earlier created missing Resolve folders as root, which the -# install, running as the user, cannot write into. Gives those folders, from -# Blackmagic Design down to the Fusion folders OpenBeat installs into, back to user -# $2 of home folder $1. It walks into each folder, the home folder included, with -# openbeat_enter rather than -# using paths through the user's home folder, which they can change at any time, -# and stops at a symlink or missing folder: nothing root-owned from an older -# OpenBeat lies beyond one. -openbeat_reclaim_folders() ( - local uid - uid="$(id -u "$2")" || exit 1 - # The home folder may sit in a folder the user can change, so it gets the same - # checks, and must belong to the user; otherwise there is nothing to reclaim. - if ! openbeat_enter "$1" "$uid"; then - openbeat_log "Not checking folder ownership: $1 is not a folder that belongs to $2" - exit 0 - fi - openbeat_enter "Library" && openbeat_enter "Application Support" || exit 0 - openbeat_reclaim "Blackmagic Design" "$2" || exit 1 - openbeat_enter "Blackmagic Design" || exit 0 - openbeat_reclaim "DaVinci Resolve" "$2" || exit 1 - openbeat_enter "DaVinci Resolve" || exit 0 - openbeat_reclaim "Fusion" "$2" || exit 1 - openbeat_enter "Fusion" || exit 0 - openbeat_reclaim "Modules" "$2" || exit 1 - openbeat_reclaim "Scripts" "$2" || exit 1 - openbeat_enter "Scripts" || exit 0 - openbeat_reclaim "Utility" "$2" || exit 1 -) - # Prints the access control entries of $1, one per line. Tests replace this function. openbeat_acl_entries() { local listing @@ -406,10 +367,7 @@ openbeat_main() { fi resolve_root="$user_home/Library/Application Support/Blackmagic Design/DaVinci Resolve/Fusion" - openbeat_reclaim_folders "$user_home" "$OPENBEAT_USER" || status=$? - if [ "$status" -eq 0 ]; then - openbeat_install_as_user "$OPENBEAT_USER" "$staging/payload" "$resolve_root" || status=$? - fi + openbeat_install_as_user "$OPENBEAT_USER" "$staging/payload" "$resolve_root" || status=$? rm -rf "$staging" rmdir "${staging%/*}" 2>/dev/null || true return "$status" diff --git a/tests/test_installer_updates.py b/tests/test_installer_updates.py index 96fee25..b084a2d 100644 --- a/tests/test_installer_updates.py +++ b/tests/test_installer_updates.py @@ -65,19 +65,6 @@ [ -n "${CONSOLE_USER:-}" ] && printf '%s\n' "$CONSOLE_USER" elif [ "$1" = "-f" ] && [ "$2" = "%u %Lp" ]; then command stat -c '%u %a' "$3" - elif [ "$1" = "-f" ] && [ "$2" = "%u" ]; then - # Resolve symlinked parent folders, as the real stat does when it opens the path. - if [ "$3" = "." ]; then - set -- "$1" "$2" "$(pwd -P)" - else - set -- "$1" "$2" "$(cd -P -- "$(dirname -- "$3")" && pwd -P)/$(basename -- "$3")" - fi - case "|${FAKE_ROOT_OWNED:-}|" in - *"|$3|"*) echo 0 ;; - *) command stat -c '%u' "$3" ;; - esac - elif [ "$1" = "-f" ] && [ "$2" = "%d:%i" ]; then - command stat -c '%d:%i' "$3" else command stat "$@" fi @@ -89,7 +76,6 @@ printf 'NFSHomeDirectory: %s\n' "$FAKE_HOME" fi } -chown() { echo "chown $*" >&2; } openbeat_as_user() { echo "as user $1" >&2; shift; "$@"; } openbeat_acl_entries() { echo " 0: group:everyone deny delete" @@ -314,6 +300,20 @@ def test_files_left_in_the_staging_folder_by_an_older_package_are_refused(self) self.assertIn("include files the installer payload does not list", result.stdout) self.assertEqual(tree(self.root), before) + def test_resolve_folder_the_user_cannot_write_stops_with_the_fix(self) -> None: + if os.geteuid() == 0: + self.skipTest("root can write to any folder") + before = self.make_previous_install() + modules = self.root / "Modules" + modules.chmod(0o555) + self.addCleanup(modules.chmod, 0o755) + result = self.run_script(self.payload, self.root) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn(f"OpenBeat cannot write to {modules}", result.stdout) + self.assertIn(f'sudo chown "{os.environ.get("USER") or pwd.getpwuid(os.getuid()).pw_name}" "{modules}"', result.stdout) + modules.chmod(0o755) + self.assertEqual(tree(self.root), before) + def test_termination_during_replacement_restores_previous_installation(self) -> None: before = self.make_previous_install() result = self.run_script(self.payload, self.root, term_move_to=self.root / "Scripts" / "Utility" / "OpenBeat") @@ -400,46 +400,6 @@ def test_files_in_the_users_folder_are_only_touched_as_the_user(self) -> None: self.assertNotIn("openbeat_install ", main) self.assertNotRegex(main, r"\b(chmod|chown|cp|mv)\b") - def test_root_owned_resolve_folders_from_older_installers_are_given_back(self) -> None: - fusion = self.home / "Library/Application Support/Blackmagic Design/DaVinci Resolve/Fusion" - (fusion / "Scripts" / "Utility").mkdir(parents=True) - (fusion / "Modules").mkdir() - result = self.run_main(FAKE_ROOT_OWNED=f"{fusion / 'Modules'}|{fusion / 'Scripts' / 'Utility'}") - self.assertEqual(result.returncode, 0, result.stdout + result.stderr) - self.assertIn(f"chown -h {self.user}:staff ./Modules\n", result.stderr) - self.assertIn(f"chown -h {self.user}:staff ./Utility\n", result.stderr) - self.assertEqual(result.stderr.count("chown"), 2) - self.assertTrue((self.modules / "bin" / "openbeat").is_file()) - - def test_home_folder_that_is_a_symlink_or_not_the_users_is_not_entered_as_root(self) -> None: - for linked in (True, False): - with self.subTest(linked=linked): - self.setUp() - # Stands in for / or another account's folder, with root-owned Resolve folders. - system = self.work / "system" - (system / "Library" / "Application Support" / "Blackmagic Design").mkdir(parents=True) - home = system - if linked: - home = self.work / "linked home" - home.symlink_to(system, target_is_directory=True) - root_owned = f"{system}|{system / 'Library' / 'Application Support' / 'Blackmagic Design'}" - result = self.run_main(FAKE_HOME=str(home), FAKE_ROOT_OWNED=root_owned) - self.assertNotIn("chown", result.stderr) - self.assertIn("Not checking folder ownership", result.stdout) - - def test_symlinked_resolve_folder_is_not_followed_as_root(self) -> None: - # The user links Blackmagic Design to a folder they cannot change, here standing - # in for the system-wide one, whose root-owned folders must stay root's. - system = self.work / "system" / "Blackmagic Design" - (system / "DaVinci Resolve" / "Fusion" / "Modules").mkdir(parents=True) - support = self.home / "Library" / "Application Support" - support.mkdir(parents=True) - (support / "Blackmagic Design").symlink_to(system, target_is_directory=True) - fusion = system / "DaVinci Resolve" / "Fusion" - result = self.run_main(FAKE_ROOT_OWNED=f"{system}|{system / 'DaVinci Resolve'}|{fusion}|{fusion / 'Modules'}") - self.assertNotIn("chown", result.stderr) - self.assertEqual(result.returncode, 0, result.stdout + result.stderr) - def test_home_folder_with_spaces_and_quotes(self) -> None: for wrapped in ("", "1"): with self.subTest(wrapped=bool(wrapped)): From dac5acb73e6044f89564abd9a3654e0a90dbc431 Mon Sep 17 00:00:00 2001 From: eimexdev Date: Thu, 22 Oct 2026 00:43:11 -0700 Subject: [PATCH 11/12] List every unwritable Resolve folder in one fix command Co-Authored-By: Claude Opus 5.5 --- scripts/macos_postinstall.sh | 9 ++++++--- tests/test_installer_updates.py | 15 +++++++++------ 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/scripts/macos_postinstall.sh b/scripts/macos_postinstall.sh index 96fdb5b..f7d9bf7 100755 --- a/scripts/macos_postinstall.sh +++ b/scripts/macos_postinstall.sh @@ -171,13 +171,16 @@ openbeat_write_config() { # root. Root does not change their owner: it cannot tell which root-owned folders in # a home folder OpenBeat made, and the user can rename folders there. openbeat_check_writable() { - local root="$1" folder + local root="$1" folder blocked="" for folder in "$root" "$root/Modules" "$root/Scripts" "$root/Scripts/Utility"; do if [ -e "$folder" ] && [ ! -w "$folder" ]; then - openbeat_log "OpenBeat cannot write to $folder, which may belong to root after an earlier OpenBeat installer. In Terminal, run: sudo chown \"$(id -un)\" \"$folder\" and then run this installer again." - return 1 + blocked="$blocked \"$folder\"" fi done + if [ -n "$blocked" ]; then + openbeat_log "OpenBeat cannot write to some Resolve folders, which may belong to root after an earlier OpenBeat installer. In Terminal, run: sudo chown \"$(id -un)\"$blocked and then run this installer again." + return 1 + fi } openbeat_install_steps() { diff --git a/tests/test_installer_updates.py b/tests/test_installer_updates.py index b084a2d..6dcf55f 100644 --- a/tests/test_installer_updates.py +++ b/tests/test_installer_updates.py @@ -304,14 +304,17 @@ def test_resolve_folder_the_user_cannot_write_stops_with_the_fix(self) -> None: if os.geteuid() == 0: self.skipTest("root can write to any folder") before = self.make_previous_install() - modules = self.root / "Modules" - modules.chmod(0o555) - self.addCleanup(modules.chmod, 0o755) + modules, utility = self.root / "Modules", self.root / "Scripts" / "Utility" + for folder in (modules, utility): + folder.chmod(0o555) + self.addCleanup(folder.chmod, 0o755) result = self.run_script(self.payload, self.root) self.assertEqual(result.returncode, 1, result.stdout + result.stderr) - self.assertIn(f"OpenBeat cannot write to {modules}", result.stdout) - self.assertIn(f'sudo chown "{os.environ.get("USER") or pwd.getpwuid(os.getuid()).pw_name}" "{modules}"', result.stdout) - modules.chmod(0o755) + # One command fixes every folder, so a single extra run is enough. + user = pwd.getpwuid(os.getuid()).pw_name + self.assertIn(f'sudo chown "{user}" "{modules}" "{utility}" and then', result.stdout) + for folder in (modules, utility): + folder.chmod(0o755) self.assertEqual(tree(self.root), before) def test_termination_during_replacement_restores_previous_installation(self) -> None: From 7ad12f232809e3acb4691c8ed27d62896046a892 Mon Sep 17 00:00:00 2001 From: eimexdev Date: Thu, 22 Oct 2026 00:53:34 -0700 Subject: [PATCH 12/12] Shell-quote the paths in the suggested chown command Co-Authored-By: Claude Opus 5.5 --- scripts/macos_postinstall.sh | 11 ++++++++--- tests/test_installer_updates.py | 25 ++++++++++++++++++++++++- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/scripts/macos_postinstall.sh b/scripts/macos_postinstall.sh index f7d9bf7..fb6eed4 100755 --- a/scripts/macos_postinstall.sh +++ b/scripts/macos_postinstall.sh @@ -170,15 +170,20 @@ openbeat_write_config() { # the installing user cannot write to it. OpenBeat 0.0.1 created missing ones as # root. Root does not change their owner: it cannot tell which root-owned folders in # a home folder OpenBeat made, and the user can rename folders there. +# Prints $1 single-quoted for pasting into a shell, whatever characters it contains. +openbeat_shell_quote() { + printf "'%s'" "$(printf '%s' "$1" | sed "s/'/'\\\\''/g")" +} + openbeat_check_writable() { local root="$1" folder blocked="" for folder in "$root" "$root/Modules" "$root/Scripts" "$root/Scripts/Utility"; do if [ -e "$folder" ] && [ ! -w "$folder" ]; then - blocked="$blocked \"$folder\"" + blocked="$blocked $(openbeat_shell_quote "$folder")" fi done if [ -n "$blocked" ]; then - openbeat_log "OpenBeat cannot write to some Resolve folders, which may belong to root after an earlier OpenBeat installer. In Terminal, run: sudo chown \"$(id -un)\"$blocked and then run this installer again." + openbeat_log "OpenBeat cannot write to some Resolve folders, which may belong to root after an earlier OpenBeat installer. In Terminal, run: sudo chown $(openbeat_shell_quote "$(id -un)")$blocked and then run this installer again." return 1 fi } @@ -249,7 +254,7 @@ openbeat_install() { # What openbeat_install needs, passed to the shell openbeat_install_as_user starts. OPENBEAT_INSTALL_FUNCTIONS="openbeat_log openbeat_exists openbeat_rename openbeat_verify openbeat_restore openbeat_finish openbeat_recover openbeat_abort openbeat_swap openbeat_write_config - openbeat_check_writable openbeat_install_steps openbeat_install" + openbeat_shell_quote openbeat_check_writable openbeat_install_steps openbeat_install" # Runs command $2... as user $1. Tests replace this function. openbeat_as_user() { diff --git a/tests/test_installer_updates.py b/tests/test_installer_updates.py index 6dcf55f..e77d307 100644 --- a/tests/test_installer_updates.py +++ b/tests/test_installer_updates.py @@ -312,11 +312,34 @@ def test_resolve_folder_the_user_cannot_write_stops_with_the_fix(self) -> None: self.assertEqual(result.returncode, 1, result.stdout + result.stderr) # One command fixes every folder, so a single extra run is enough. user = pwd.getpwuid(os.getuid()).pw_name - self.assertIn(f'sudo chown "{user}" "{modules}" "{utility}" and then', result.stdout) + self.assertEqual(self.suggested_chown(result.stdout), [user, str(modules), str(utility)]) for folder in (modules, utility): folder.chmod(0o755) self.assertEqual(tree(self.root), before) + def test_suggested_chown_command_survives_shell_characters_in_paths(self) -> None: + if os.geteuid() == 0: + self.skipTest("root can write to any folder") + root = self.work / "it's \"odd\" $HOME `id`" / "Fusion" + (root / "Modules").mkdir(parents=True) + (root / "Modules").chmod(0o555) + self.addCleanup((root / "Modules").chmod, 0o755) + result = self.run_script(self.payload, root) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertEqual(self.suggested_chown(result.stdout)[1:], [str(root / "Modules")]) + + def suggested_chown(self, output: str) -> list[str]: + """The arguments the logged sudo command passes to chown, as a shell would parse them.""" + command = output.split("In Terminal, run: ", 1)[1].split(" and then run this installer again.", 1)[0] + self.assertTrue(command.startswith("sudo chown "), command) + parsed = subprocess.run( + [BASH, "-c", 'sudo() { shift; printf "%s\\0" "$@"; }\n' + command], + capture_output=True, + text=True, + check=True, + ) + return parsed.stdout.split("\0")[:-1] + def test_termination_during_replacement_restores_previous_installation(self) -> None: before = self.make_previous_install() result = self.run_script(self.payload, self.root, term_move_to=self.root / "Scripts" / "Utility" / "OpenBeat")