From the /my-review of PR #288 (finding 4, medium). Not fixed on issue-286 — the branch was scoped to the install-path blockers.
src/chief/install/account.py:99-105 rejects only the literal home path, /, and relative paths. Verified against the real function:
'~/..' -> /home/will/.. => None (allowed)
'/home' -> /home => None (allowed)
'/Users' -> /Users => None (allowed)
'/etc' -> /etc => None (allowed)
So the "home directory root is never offered" claim in docs/SECURITY.md, docs/OPERATIONS.md and PRD #286 is defeated by typing ~/.., and chgrp -R chief /Users && chmod -R g+rwX /Users is two keystrokes away — a recursive, irreversible group-ownership rewrite of every account on the box.
There is also no existence check, so a typo'd directory makes chgrp exit non-zero and _run aborts the plan after the account and tree steps already landed.
Related: src/chief/install/dedicated_ask.py:84-87 does not validate the account name, which flows into argv and into Path("/Users") / user — default_home("darwin", "/etc") returns /etc, which would become useradd --home-dir /etc.
Fix
path = Path(token).expanduser().resolve(); refuse if path == Path("/"), if home.is_relative_to(path), or if not path.is_dir().
- Gate the account name on
^[a-z_][a-z0-9_-]{0,31}$.
account.py is at 195/200 — put the name check in dedicated_ask.py (96 lines) and keep grant_reason to the same line count by replacing its two-branch body.
Test
Extend tests/test_install.py::test_granted_directories_are_group_permissions_and_never_the_home_root with ~/.., /home, /Users and a nonexistent path.
From the
/my-reviewof PR #288 (finding 4, medium). Not fixed onissue-286— the branch was scoped to the install-path blockers.src/chief/install/account.py:99-105rejects only the literalhomepath,/, and relative paths. Verified against the real function:So the "home directory root is never offered" claim in
docs/SECURITY.md,docs/OPERATIONS.mdand PRD #286 is defeated by typing~/.., andchgrp -R chief /Users && chmod -R g+rwX /Usersis two keystrokes away — a recursive, irreversible group-ownership rewrite of every account on the box.There is also no existence check, so a typo'd directory makes
chgrpexit non-zero and_runaborts the plan after the account and tree steps already landed.Related:
src/chief/install/dedicated_ask.py:84-87does not validate the account name, which flows into argv and intoPath("/Users") / user—default_home("darwin", "/etc")returns/etc, which would becomeuseradd --home-dir /etc.Fix
path = Path(token).expanduser().resolve(); refuse ifpath == Path("/"), ifhome.is_relative_to(path), or ifnot path.is_dir().^[a-z_][a-z0-9_-]{0,31}$.account.pyis at 195/200 — put the name check indedicated_ask.py(96 lines) and keepgrant_reasonto the same line count by replacing its two-branch body.Test
Extend
tests/test_install.py::test_granted_directories_are_group_permissions_and_never_the_home_rootwith~/..,/home,/Usersand a nonexistent path.