Comments cited plan sections as "(§G3)" or "(§8 unification)", which
point into the same git-ignored plans the plan-number citations did.
References next to a real document (docs/REDESIGN_MAP.md §3) stay. Code
and runtime strings are unchanged (AST and token comparison).
The worst cases the audit named: docstrings and header comments that
told how three old copies drifted, or quoted subprocess counts that
were stale the day they were written, instead of stating the contract.
Each now says what the function does and keeps one line of why. Also
drops the always-zero n_cloud counter that existed only to keep a boot
log line's or-chain intact.
Comments, docstrings and a few strings cited plan numbers ("plan 29
#9", "plan 86 §B2") from a git-ignored folder, so they pointed at
nothing; one reached users in a bottleneck hint. Code and frontend
strings are unchanged apart from those citations (checked by AST and
token comparison). A vhost still carrying the old asset comment reads
as undrifted, and a guard test keeps new citations out.
Custom build commands, Python app commands, repo deploy scripts and
registry template scripts ran with os.environ.copy(), handing code from
a repository or registry the JWT signing key, the session key, the
database URL and every SERVERKIT_* credential. app_process_env() builds
their environment without those.
A gzip.open() handle passed as a subprocess stream hands the child the
raw fd, so mysqldump/pg_dump wrote plain SQL inside a gzip header and
restores piped compressed bytes into mysql/psql. Backups, policies and
pre-deploy snapshots all produced files that could not be restored.
run_to_gzip/run_from_gzip pump the bytes through Python instead, with
stderr in a temp file so a noisy child can't deadlock the pipe. The two
hand-rolled db_sync dump pipes move onto them too.
Plan 75 §G1, the remaining 33 files. Every raw subprocess call site in
backend/app that can go through a door now does. What is left is what
should be:
8 app/utils/system.py the doors themselves
8 database_service mysqldump|gzip, gunzip|mysql, pg_dump|gzip,
gunzip|psql — a pipe is two processes
7 build/docker/db_sync/ftp streaming output, gzip pipes
1 log_service tail -f
2 serverkit-analytics sibling repo; see below
Extensions were migrated in BOTH trees. builtin-extensions/ is the source
and backend/app/plugins/ the live copy, compared by normalised hash;
test_builtin_extension_drift stays green. serverkit-email has both sides
tracked, serverkit-ftp and serverkit-cloud-provision only the source.
serverkit-analytics (2 calls) is deliberately untouched: it has no source
in builtin-extensions/ at all, and backend/app/plugins/serverkit-analytics
is a gitignored materialised copy. Editing it would be a change that
cannot be committed and is overwritten on the next sync — that fix belongs
in the serverkit-analytics repo.
Exit-code semantics are preserved wherever they carry meaning, with a
comment at each site, because they are the thing a mechanical migration
destroys: clamscan 1 = INFECTED, grype 1 = vulnerabilities FOUND, yara
1 = matches, cron_runner 127 = never launched. Those read
result['returncode'], never result['success'].
run_checked gained two things real call sites needed: it yields when the
caller directs the streams itself (writing straight to a log file), rather
than adding capture_output on top and raising ValueError; and text/errors
pass through, so site_importers/ssh keeps decoding with errors='replace'
instead of gaining a UnicodeDecodeError on a bad byte.
A regression this migration introduced, caught by the suite and fixed:
install_profile_service._docker_usable() is deliberately TRI-STATE —
None means the probe could not run, because inside LXC the docker client
is usually present while the daemon never started, and callers "must not
treat a broken probe as proof of absence". Writing it as
run_checked(...)['success'] turned an exec failure into False, i.e. "docker
is not usable" — a fact nobody established. That is the §A violation this
plan exists to remove, reintroduced by the migration meant to prevent it.
test_probe_honesty_doctor caught it. Every other run_checked(...)['success']
caller was then audited; the rest are genuinely two-state, and their
originals also returned False on a missing binary.
Seven more slips of the same shape were caught before running by an AST
check for `x = run_checked(...)` followed by `x.stdout/.stderr/.returncode`
— a dict read as an object, which raises only on the failure path, so
happy-path tests stay green.
api_controller_boundary_baseline.json: pure deletion. Routing api/docker.py
through DockerService removed the last 6 subprocess crossings, so the
`subprocess` category is gone from the API layer entirely.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 75 §G1, second migration. docker_service was the heaviest file in the
census: 49 raw subprocess call sites -> 2. Census 258 -> 211.
DockerService.run now SITS ON run_checked instead of re-implementing it, so
G3's runner and G1's door are one thing rather than two. Adds
DockerService.run_compose() for the compose subcommands, whose argv head is
whatever _get_compose_cmd() resolved (`docker compose` v2 vs `docker-compose`
v1) and therefore cannot go through run().
The 2 survivors are the genuine streaming Popen sites — the live container
log stream and the compose-up transcript. A captured call cannot do those.
run_checked gained two things this migration needed:
- `stderr` (the raw stream) as a separate key from `error` (the verdict, None
on success). git and `docker exec` write to stderr and still succeed, so
folding the stream into the verdict would invent failures. exec_command
needs the stream; every other caller wants the verdict.
- merge_stderr=True, for `docker logs` and `compose logs`. A container writes
its log to both streams; the `result.stdout + result.stderr` concatenation
this replaces loses the interleaving, so a crash tail landed above the lines
that led to it.
TIMEOUTS ARE UNCHANGED, deliberately. run_checked defaults to 60s and every
docker call in this file is unbounded today; inheriting the default would cap
`compose up --build`, `docker pull` and `docker build` at a minute and turn
working deploys into timeouts. Every migrated site keeps exactly what it has,
with timeout=None passed explicitly at the long ones and six tests pinning it
(mutation-checked: removing it from pull_image fails the test). Bounding
docker's long operations is a real improvement and a deliberate behaviour
change — its own commit, not a side effect of this one.
Two regressions caught by the suite and fixed here:
- Dropping the try/except around the compose-version probe let an exception
escape into the caller, surfacing as `compose up` exit_code -1 instead of a
v1 fallback. run_checked converts the exec failures it knows about, but this
probe runs before everything else in the compose path and must not be able
to raise at all. Guard restored, with a test.
- test_docker_protection hand-rolled a CompletedProcess stand-in with only
returncode and stdout. Once list_containers went through run_checked,
reading .stderr off it raised AttributeError, the caller's blanket except
swallowed it, and the test saw an EMPTY CONTAINER LIST rather than an error.
Retargeted onto the §G7 kit. A private stub is only correct until something
reads the attribute it forgot.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 75 §G1 — the door and the ratchet. The 33-file migration is NOT
done; see below.
run_checked(cmd, *, privileged=False, timeout=60, input=None). Around
backend/app's raw subprocess calls sit 220 copies of capture_output/text,
56 hand-rolled TimeoutExpired handlers, 19 FileNotFoundError handlers and
1,164 literal {'success': False, 'error': ...} dicts. Each is a place an
exec failure becomes a false fact.
It keeps the distinction those handlers lose: returncode is None when the
command never ran. Exit 1 means the command answered "no"; no exit code
at all means nobody answered. A caller rendering "not installed" can
finally tell them apart — plan 74's outage in one field. Unprivileged
calls get the same argv[0] resolution as privileged ones, because the
sbin outage is not something that only happens when you ask for root.
The ratchet is tests/raw_subprocess_census.py + RAW_SUBPROCESS_CEILING,
wired into the existing "Audit Raw Subprocess Patterns" CI job and
enforced by test_raw_subprocess_ratchet.py. The grep steps already there
check eight hand-listed files for three known-bad patterns, so a new file
was invisible to the audit entirely; the census counts every call site
under backend/app. It is AST-based because a grep for subprocess.run also
matches comments, docstrings and `except subprocess.SubprocessError`, and
a miscounting ratchet reads green either way. test_the_ceiling_is_not_stale
fails if the ceiling drifts more than 10 above reality — a stale ceiling
silently re-authorises everything a migration removed.
First migration: cron_service, 13 raw calls -> 3. Its `crontab -l` read
and Popen-with-stdin `crontab -` install were written out five times each
and are now two helpers over run_checked. Census 267 -> 258.
The other 32 files are untouched: docker_service (49), database_service
(24), git_service (18), security_service (14), python_service (13) are
the bulk. The door plus the ratchet are what make that incremental
instead of one unreviewable diff.
test_utils_system.py loads system.py under its own importlib spec, so
@patch('app.utils.system.<name>') binds to whichever module object is in
sys.modules at patch time — not necessarily the one run_checked closes
over. shutil/subprocess are singletons so the file's existing patches are
safe; these are the first tests to patch a module-level function of
system.py, and they use patch.object(_module, ...) instead.
BASELINE_COUNT 3383 -> 4081. Most of that is pre-existing drift (the
ratchet only fails on a drop, so an un-bumped floor never complains), not
tests added here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 75 §G2 + §G4, together because they are the same physical files and
G4 is built on G2's helper.
§G2 — write_privileged_file(). The write-a-root-owned-file sequence was
pasted 15+ times in two competing forms: run_privileged(['tee', path],
input=...) and a raw subprocess.run(['sudo', 'tee', ...]). The second
hardcodes sudo instead of asking _needs_sudo(), so it fails on a panel
already running as root in a container with no sudo binary — plan 74's
outage shape. All core-service tee sites now go through one helper that
owns the sudo decision, the result shape, and honest failure: a missing
tee or a timeout reports the failure instead of being swallowed (§A).
§G4 — NginxService.write_vhost()/read_vhost(). environment_domain_service
re-implemented write → symlink → nginx -t → reload with raw sudo calls,
and nginx_advanced_service wrote the vhost with a plain unprivileged
open() — PermissionError against a root-owned sites-available, with no
config test and no reload. Both now go through NginxService.
The rollback is the part no copy had: a vhost that fails nginx -t used to
stay on disk and symlinked, so the next unrelated reload (a cert renewal,
another site going live) failed and the blame landed on whatever
triggered it. write_vhost restores the previous content, or removes a
file that did not exist before, and never reloads a config that failed
the test.
SITES_AVAILABLE/SITES_ENABLED were independently declared in four files,
so an NGINX_CONF_DIR override — the env var NginxService honours, and what
tests redirect — silently did not apply to the other three. One
declaration now.
Mutation-checked: disabling the rollback fails two tests.
Also lands unit_is_active() in app/utils/system.py; its adopters are in
the following commit (§G6).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The result translation was re-implemented per service with subtly
divergent failure shapes — stderr under "error" in postfix, under
"message" in php and nginx. ServiceControl.result_dict owns it, with
the key and fallback parametrized so converting a site is parity, not
a contract change. One deliberate improvement: an empty stderr now
yields a real fallback message instead of a None "message".
The audit corrected the plan's premise along the way: no dovecot/mail
×5 wrapper family exists; the genuine pass-throughs were
postfix.restart and the php fpm pair. nginx keeps its config test,
fail2ban its fallback, postfix.reload its own mechanism — genuine
per-service logic stays.
Plan 75 §A2. Four dishonest shapes across doctor/drift/fleet, all
converting a failed probe into a determined-looking answer:
- except: return []/None/False whose empty result is indistinguishable
from a determined negative: a broken Domain query rendered green
'No public site domains to check'; a whole doctor section (setup,
extensions) vanished silently on failure; an uninspectable app was
dropped from the drift sweep, reading as 'in sync' by omission.
- Resolver errors impersonating NXDOMAIN: any getaddrinfo failure
reported 'does not resolve', offered repair, and paged admins about
domains 'newly stopped resolving' during what was a panel-side DNS
outage. Only EAI_NONAME/EAI_NODATA is an unresolved domain now.
- Unknown -> False -> 'Not running': a missing systemctl (container,
non-systemd init) read as a repairable fail; a failed docker probe
read as a green 'Not installed — Minimal profile'; a fleet agent
that returned no state for a unit persisted fail 'Not running.'
- Fleet default-to-ok: missing status persisted and counted as ok
(STATUS_ERROR existed and was never emitted); a failed/timed-out
sweep left the server's last all-ok rows standing as current health
— a synthetic error row now records that this run couldn't check.
Two existing tests changed: one pinned the dishonest behavior itself
(is_active False on missing systemctl), one faked NXDOMAIN with a
single-arg gaierror (errno=None) — fixture corrected to the real
two-arg form, expectation unchanged.
plan 75 §B1. Two helpers did overlapping jobs with different semantics, and the
one that says nothing about privilege sat one autocomplete from the one that
does. nginx_advanced_service reached for it, so `nginx -t` ran unprivileged and
by bare name — it could neither read root-owned config nor find /usr/sbin/nginx,
and reported every config as invalid.
Renamed rather than folded. Folding into run_privileged(privileged=False) would
change sudo semantics at all six call sites — `sudo -n dig` fails where plain
dig worked, if sudoers does not permit it — and the failure this fixes was
choosing the wrong helper, not the helper's behaviour. The rename removes the
ambiguity with zero behavioural risk; the return type (dict, not
CompletedProcess) stays as callers expect.
`run_unprivileged(['nginx', '-t'])` reads as obviously wrong. `run_command` did
not. The old name was ambiguous in a second way: PythonService has an unrelated
method of the same name, which keeps it.
Six call sites, three test modules. Two patch targets were written as strings
('app.utils.system.run_command', setattr(mod, 'run_command')) so a word-boundary
rename skipped them and they would have failed at runtime, not import.
No alias: an alias leaves the second door open, which is the thing being closed.
run_command was never exported through plugins_sdk, so there is no public SDK
surface to break; a third-party extension importing it gets a loud ImportError
rather than silent misbehaviour. A guard test asserts the name stays gone.
417 tests green across monitor, ssl, dns, nginx, firewall and the guards.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ufw fix repaired every run_privileged caller centrally, but three sites
reached subprocess by other routes and were broken the same way. All are
"$PATH omits sbin + the panel runs as root, so nothing prepends sudo to
resolve it".
- run_command() got the same argv[0] resolution as privileged_cmd. It does no
privilege escalation, so nothing else would ever find these commands.
- nginx_advanced_service.test_config() ran `nginx -t` (/usr/sbin/nginx) via
run_command, so it raised FileNotFoundError and reported every config
invalid. It also returned valid=True unconditionally, ignoring the exit code
entirely — so when it DID run, a broken config passed. Now run_privileged and
the returncode is checked. reload_nginx() likewise reports a failed reload
instead of swallowing it.
- dovecot_service.get_status() detected dovecot with `which`, which searches
$PATH only and false-negatives on /usr/sbin/dovecot, then read the version by
bare name under a shared except. Now is_command_available() + a separate try,
so a version read that fails cannot unsay "installed". Patched in both the
builtin-extensions source and the app/plugins live copy.
- SecurityService.get_lynis_status() inferred presence solely from exec'ing
`lynis --version`; presence and version are now separate questions.
Adds test_sbin_command_guard.py so this cannot come back: an AST scan fails CI
on any raw subprocess call using a known sbin-resident command by bare name
(it caught the dovecot site), plus per-command proof that both helpers resolve
under the exact PATH the panel unit ships.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UFW installed cleanly, dpkg said ii, the install notification said so — and
/security/firewall still rendered "No Firewall Installed". fail2ban worked on
the same box, which is the clue: fail2ban-client lives in /usr/bin, ufw in
/usr/sbin, and the panel unit ships
PATH=<venv>/bin:/usr/local/bin:/usr/bin:/bin
with no sbin. subprocess resolves argv[0] through $PATH alone, so `ufw status`
raised FileNotFoundError. _check_ufw wrapped both questions in one try/except,
so that exception discarded an `installed` already computed as True and
returned False. Measured on the affected host:
is_command_available('ufw') = True
PackageManager.is_installed('ufw') = True
get_status() -> ufw {'installed': False}, any_installed False
FirewallTab.jsx gates its empty state on any_installed. The Enable button was
broken identically — same bare-name exec.
Three fixes, each independent:
- resolve_command() gives argv[0] an absolute path, but ONLY when $PATH cannot
resolve it, so working commands pass through byte-identical.
- probe failure can no longer unsay "installed" (same split for firewalld).
- the unit template gains /usr/local/sbin, /usr/sbin and /sbin. update.sh
re-renders the unit, so existing installs pick this up on upgrade.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 73 item 1. SSLService hardcoded /usr/bin/certbot, so the install method
certbot itself recommends -- snap, which lands in /snap/bin -- produced a
FileNotFoundError the panel surfaced as an opaque failure. is_command_available
had the same blind spot: it probed /usr/bin, /usr/sbin and the two local dirs
but never /snap/bin, which is routinely absent from a service's $PATH.
resolve_certbot_bin() now tries the CERTBOT_BIN override, then shutil.which,
then the explicit candidate list including the snap path. Positive results are
cached (an install location does not move mid-process) but negative ones are
not, so installing certbot is picked up without a panel restart;
install_certbot clears the cache outright. CERTBOT_BIN as a class attribute is
gone -- it captured the value at import time, which is exactly why an override
set later never took effect -- replaced by certbot_bin() plus CERTBOT_FALLBACK
so a command built on a host with no certbot still fails cleanly.
The second half is the failure the reviewer actually hit on stream: with
certbot present but its nginx plugin missing, `certbot --nginx` dies deep
inside certbot with "the nginx plugin is not working / binary doesn't exist",
and we passed that through raw. obtain_certificate now preflights -- nginx on
the box, then `certbot plugins` for an enabled `* nginx` -- and returns an
actionable error naming python3-certbot-nginx before certbot is ever invoked
for issuance. A failing plugins probe falls back to the distro package check
rather than blocking issuance on a host where the plugin is clearly present.
AdvancedSSLService's wildcard and multi-domain commands go through the same
resolution instead of the bare name.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
210 run_privileged() call sites passed no timeout. With output captured
and no terminal attached, a command that never returns takes its caller
with it — silently and forever. That is exactly how the metadata guard's
iptables probe wedged backend startup; the sudo -n fix removed that one
trigger, but the class of bug was still one hung command away.
run_privileged now applies a default timeout when the caller does not
name one, so the fix lands on all 210 sites at once rather than being
reapplied by hand and forgotten on the 211th. TimeoutExpired is a far
better outcome than a wedged worker, and callers that genuinely need
longer already pass their own; `timeout=None` opts out deliberately.
The four direct read-only probes in this module (dpkg -s, rpm -q,
systemctl is-active/is-enabled) bypass that helper, so they get their own
much tighter ceiling — they answer immediately or not at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
privileged_cmd prepended a bare `sudo`. Nothing here runs attached to a
terminal and run_privileged captures output, so on a non-root host a
password prompt is both invisible and unanswerable — sudo blocks forever
and takes its caller with it. This hung backend startup outright: the
metadata guard's `iptables -C DOCKER-USER` probe sat unanswered and the
server never bound its port.
Emit `sudo -n` so the call fails immediately with a non-zero exit, which
every caller already handles. Production runs as root (_needs_sudo() is
false) which is why this stayed hidden, but any non-root install without
passwordless sudo would hang on boot.
An explicitly-sudo command passed in by a caller is still forwarded
untouched; the regression test pins non-interactive sudo across all four
invocation shapes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Allow authenticated (non-admin) users to fetch API key usage by replacing @admin_required with @auth_required in api_analytics.py and importing auth_required. Add optional user support to privileged_cmd and run_privileged in app/utils/system.py so callers can run commands as a specific user (sudo -u). Update wordpress_service to use the new user keyword when invoking privileged commands. Fix tests/conftest.py to put the backend root (parent directory) on sys.path so tests import correctly.
Introduce safer system interactions and improved logging fallbacks across services. Replace direct sudo/subprocess calls with run_privileged/privileged_cmd, add command availability checks (is_command_available) and Linux guards for user management. Enhance LogService with fallback readers (python file I/O, grep fallback, syslog and Windows Event Log support) and standardize responses via sourced_result. Update frontend Terminal to display log source and adjust UI when journalctl is unavailable. Misc: delegate ProcessService log retrieval to LogService, add PackageManager usage in PHP/SSL installs, and add warning logging in DeploymentService when diff generation fails.
Backend: Replace direct sudo/subprocess.run calls with run_privileged and privileged_cmd, add _needs_sudo/privileged_cmd logic and is_command_available checks, and return clearer errors for missing binaries (journalctl/grep/tail/truncate/logrotate/systemctl/service). Add FileNotFoundError handling and use privileged_cmd for Popen.
Frontend: UI and style refinements across Downloads, Security and Terminal pages — simplified Linux icon, added release date and banner actions, redesigned scanner UI into interactive scan cards with custom path and toolbar, handle unavailable journalctl with an empty state, update SVG icons. Styles (.less): switch mono font to JetBrains Mono, adjust spacing/gaps, sidebar text truncation, revamp downloads and security card styles, and improve terminal log fonts, wrapping and processes table layout.
Refactor services to use centralized system helpers (ServiceControl, PackageManager, run_privileged, is_command_available) instead of ad-hoc subprocess/os calls. Updated deployment, firewall, ftp, nginx, php, python, security, and ssl services to call the new utilities and added error/timeout handling improvements and package manager abstraction. Added backend/app/utils/system.py implementing the helpers and backend/tests/test_utils_system.py with unit tests. This centralizes privileged command execution and service/package management, simplifying tests and platform differences.
Juan Denis