AgentSite now ships a declarative serverkit.yaml, so live inspection of that
repo returns strategy=serverkit. The template's declared hints — the labeled
fallback for when the public repo cannot be reached — still claimed
docker_compose and did not list the file at all, so the fallback described a
different deployment than the real one.
The six provider keys were also all marked required. build_template_hints
passes `required` straight through to the wizard, which stars each one, so the
install screen told people they needed six API keys. AgentSite is
model-agnostic and needs one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The integration module loaded firewall_service via importlib and
registered it in sys.modules under the real name — at collection time,
before any test ran. In a full-suite run, every @patch targeting
app.services.firewall_service then landed on the shim copy while the
firewall test modules held the real one: mocks silently not firing,
32 firewall tests red only in full-suite context, green in every
subset. In the full suite (Flask installed) import the real modules;
the container legs keep the shim, now under a non-colliding name.
Two assertions in TestProbeFailureIsolation pinned exactly what 2f002c59
fixed: "genuinely unknown, reported as off". Unknown is None + error
now; the determined-inactive case gains the error: None key.
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.
The guard fired on real-exec integration suites (the §D matrix): under
an sbin-ful PATH, run_privileged passes a bare name through — correct
behavior, since the same call absolutizes under the unit's sbin-less
PATH. The guard's model ("dies under the unit") cannot see that
resolution from inside a dev PATH. It now stays silent when the exec
came through app/utils/system.py — the one door it exists to route
traffic through — and still fires on a raw call one frame away. The
§D integration module's opt-out is removed; the guard stays active
there, and the capdrop matrix leg passes with it on.
Plan 75 §A5. The frontend could always render unknown (a real 'unknown'
state, an n/a chip) — the backend starved it of the signal:
- Doctor's cert check read Domain.ssl_expires_at, a column nothing in
production writes — structurally incapable of anything but a green
'No certificates tracked.' Empty now warns: expiry is not monitored.
- openssl -checkend exits non-zero for 'expires soon' AND for
'unreadable' — an unreadable cert reported needs_renewal=True, and
the error path vanished the cert from the expiring list entirely.
Unreadable now returns an error result, and the status endpoint
surfaces check_errors instead of dropping them.
- A failed monitor cert probe bumped cert_checked_at while keeping the
old cert_expires_at — a green 'valid / N days' chip from weeks-old
data next to a fresh checked timestamp. The stale value is cleared;
the UI falls back to n/a until a probe succeeds.
- get_expiry_alerts dropped unprobeable certs by omission; they now
appear with severity 'unknown'. get_cert_health reported grade 'F'
for a DNS timeout; probe failure now yields grade None + error.
- A failed certbot inventory reported '0 certificates, nothing
expiring' with HTTP 200; list_certificates_report carries the
errors and the status endpoint exposes them as list_errors.
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.
Two dishonesty directions, one rule (plan 75 §A3/A4): a probe answers a
value or None, never 0 when undetermined — and a real 0 must survive.
Live paths normalized every missing reading to 0 while the history path
stored NULL for the same probe: agent counters an LXC agent can't
determine rendered as a perfectly idle box; load average on unsupported
platforms read 0.00 (and silenced the load alert); CPU freq read 0 MHz;
an empty history window summarized as '0.0 avg'. The mobile summary
called a method that does not exist and rendered 0/0/0 permanently.
One unavailable network counter raised uncaught and froze the entire
live broadcast.
Reverse direction: falsy traps (x if x else None) collapsed a genuinely
idle 0.0 into 'no data' in rollups, averages, and the history model.
Fleet: KPI tiles read '0.0 ms latency' / '100% success' with no data;
never-probed servers exported serverkit_server_up 0 to external
alertmanagers — the up-series now emits only for determined
connectivity. get_all_metrics is per-section fault-tolerant so one
failed probe can't blank the other six.
Five services shipped byte-identical save_config copies and as many the
same get_config skeleton — the next fix (an atomic write, a better
error) would have had to land in every one. load_json_config /
save_json_config own the mechanics; the path, the default, and any
behaviour around the write stay per-service.
Byte-compatible with the copies: plain open(..., 'w'), no locking, no
atomic rename — adding those here would be a behaviour change smuggled
in with a consolidation; if they're ever wanted they now land once.
Divergent services (template cache invalidation, storage secret
merging, email_relay's DB backing, build_service's leaner return dict)
keep their own methods. monitoring_service converts separately — it is
mid-edit on another branch of this plan.
The static sbin guard reads literal argv lists — 164 of 288 raw
subprocess sites (57%). The other 43% (variables, f-strings,
concatenation) is where plan 74's outage class can still hide: a call
that works in dev (PATH has sbin) and dies under the panel unit (PATH
has none).
An autouse conftest fixture now routes every subprocess.Popen the suite
makes through GuardedPopen, which sees the actual argv at call time and
refuses a bare name that resolves under the current PATH but not under
one with the sbin dirs removed. A binary simply absent on the test
machine still raises the honest FileNotFoundError — the guard judges
PATH shape, never machine contents. The refusal is a BaseException:
this bug class ships because blanket except handlers convert exec
failures into false facts, so an AssertionError would be swallowed by
exactly the handlers it hunts.
Coverage is 'whatever the suite exercises' — honest, measurable, and
growing for free with every new test. Full suite green with the guard
live (3791 passed; 2 pre-existing load flakes pass in isolation).
Every model with a JSON-in-Text column hand-rolled the same try/except
json.loads pair — get_details/get_context/get_headers/get_payload and a
dozen more, each a place the next fix would have to land N times. The
mixin owns the mechanics (empty-is-default, corrupt-is-default so a
hand-edited row never 500s an endpoint, optional shape check,
falsy-on-write); the per-model parts — accessor name, column, default —
stay per-model.
Conversion is behaviour-preserving: identical fallbacks, identical
on-write empty forms (NULL vs '[]' vs '{}'). Deliberately NOT converted:
accessors that let json.loads raise on corrupt data (collapsing those
would be a behaviour change, not a consolidation), and inline parses in
to_dict (per-entity serialization choice). A handful of 'except
Exception' reads narrow to the dominant (JSONDecodeError, TypeError) —
the only exceptions json.loads raises for str input.
The static scan sees literal argv lists only — 164 of 288 raw subprocess
sites (57%), and only the hardcoded names in SBIN_COMMANDS. A green run
was easy to read as 'no bare sbin calls exist'; it never meant that. Say
so in the module header and in the failure message itself, and point at
the runtime layer (popen_guard) that watches actual argv.
_backup_unverified_check returned ok for any verify_level that was not
literally 'none', so a checksum mismatch — positive evidence a backup may
not restore — rendered as 'Latest backup verified (listed).', and an
unreadable archive read as merely 'not verified yet'. verify_run_tier1
was already honest; the summary boundary was not, the same shape as the
drill badge. fail is now reserved for verification that ran and came
back bad, warn for 'couldn't check', and ok is positively earned.
plan 75 §A1 — and it was a live bug, not the hardening pass I expected.
_drill_badge() returned 'ok' for any status that was not literally 'failed'. A
drill skipped for lack of scratch space records 'skipped_no_space' AND stamps
last_drill_at, so it fell through every branch and Doctor reported:
"A recent restore drill proved this backup restores."
about a drill that never executed. It also self-perpetuated: each skip
refreshed last_drill_at, so a policy permanently short of scratch space could
never even go stale. It would report proven forever, having proven nothing —
and "short of scratch space" is the normal state of a full disk, which is
exactly when you most need the backup.
The drill service was careful (its own comment reads "loud skip, never a silent
pass") and the frontend renders an amber pill. The honesty was lost in the one
function that summarises it, which is the one Doctor reads.
Fixes:
- 'ok' must now be positively earned: only last_drill_status == 'success'
reaches it. Adds 'skipped' and 'unknown' states, so an unrecognised future
status cannot reintroduce this by falling through.
- staleness is evaluated only for a drill that actually succeeded, since it is
a property of proof rather than of attempts.
- Doctor renders both new states as warn with what was and was not proven.
- the skip path now notifies. BackupAlertService.on_drill_result already mapped
skipped_no_space to a failed outcome, but only the exception path ever called
it, so that branch was unreachable and a skipped drill told nobody. A log
line is not a notification.
Mutation-checked: with the fix reverted, the 5 bug-targeting tests fail and the
9 pinning existing behaviour still pass. 307 green across backup/doctor/restore
(a 422 in test_raw_infra_authz did not reproduce on re-run and is absent from
the baseline selection — the documented intermittent, not this change).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
plan 75 §C. Bundled extensions exist twice on disk: builtin-extensions/<ext>/
backend is the source, backend/app/plugins/<ext> is what the loader imports. A
fix applied to one and not the other ships a repo that reads correct and a panel
that runs the old code.
Not hypothetical — the plan-74 dovecot fix had to be made twice, caught only by
remembering to look.
Written while drift is zero (21 file pairs), so this locks in a clean invariant
rather than repairing a dirty one. It only stays this cheap while that is true.
Compares by CRLF-normalised hash, since the repo has mixed line endings, and
only pairs present on BOTH sides: cloudflare-ops and localkit ship source with
no materialised live copy and serverkit-git uses a different layout, all
legitimate. Most live copies are gitignored, so a fresh clone sees fewer pairs
than a dev machine (serverkit-email's predates the ignore rule and is tracked).
test_drift_check_is_not_vacuous stops that from quietly reducing the
parametrised check to zero cases while still reporting green.
Also fails when a file exists on only one side — drift before anyone edits it.
Both assertions mutation-checked: appending a byte to the live dovecot copy
fails the hash test, adding a one-sided file fails the membership test, and the
tree is green again once reverted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The enable() and remove_rule() guards reason about ports. These two paths never
touch the SSH rule and lock the operator out anyway:
block_ip(<your own address>) the rule is untouched; your source
address is rejected before reaching it
set_default_zone(<zone without ssh>) firewalld re-homes every interface with
no explicit zone; the rules did not
change, the zone around them did
check_ip_block() refuses when the target covers a live SSH peer, the caller's
own HTTP client address, or loopback — nginx forwards to 127.0.0.1, so blocking
it takes the panel down with the session. CIDRs are matched with `ipaddress`,
because 73.244.0.0/16 is the same lockout wearing a wider mask.
Peers come from `ss -Htn state established`, filtered to the sshd ports; `who`
is empty for sessions with no utmp entry. Verified against the live host, whose
only established SSH connection was 146.190.213.37:22 <- 73.244.95.52:59085:
active_ssh_peers() -> {'73.244.95.52'}
block 73.244.95.52 -> refused
block 73.244.0.0/16 -> refused (covers the same peer)
block 127.0.0.1 -> refused (loopback)
block 198.51.100.7 -> allowed
caller_ip is read in the API layer from request.remote_addr and passed in, so
the service stays free of request context.
check_default_zone() asks whether the target zone permits ssh via
--list-services / --list-ports before switching. A failed `ss` probe yields no
peers rather than fabricating conflicts, so a broken probe cannot block every
request. Consistent with the existing guards: undeterminable SSH port refuses,
an unenforcing firewall is a no-op, force=true is the operator accepting it.
/block-ip and /zones/default answer 409 with the conflicting addresses.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
enable() guards the moment the firewall comes up; this guards every moment
after it. With ufw active and denying by default, deleting the rule that admits
SSH severs the very session issuing the delete, and there is no way back in.
Deleting by NUMBER is the sharp edge: the caller passes an opaque index, so the
only way to know what is about to go is to resolve it against
`ufw status numbered` first. Removal by port spec is checked too.
Coverage-aware rather than a blanket block — a duplicate SSH rule may still be
deleted, because another allow rule keeps the port open. Application profiles
(`[1] OpenSSH`) are resolved through `ufw app info`, verified against a real
host: _ports_in('OpenSSH') -> {22}, and '22/tcp (v6)' -> {22} rather than
parsing the address-family suffix as port 6.
deny_port() is implemented as remove_rule(), so it inherits the guard —
denying the SSH port and deleting its allow rule are the same outcome.
Consistent with the enable() guard: an undeterminable SSH port refuses instead
of assuming 22, an inactive firewall is a no-op (nothing is being enforced and
enable() preflights separately), DEFAULT_INPUT_POLICY=ACCEPT short-circuits,
and force=true is the operator accepting the risk.
firewalld: removing service=ssh or the SSH port is checked against
--list-services / --list-ports so other coverage still permits the removal.
DELETE /rules and POST /deny-port answer 409 with the offending rule;
POST /rules/removal-preflight lets the UI warn before the click.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ufw enable` normally prompts "Command may disrupt existing ssh connections.
Proceed?". The service must use `--force enable` because nothing here runs on a
terminal — and --force suppresses exactly that prompt. So the panel had removed
ufw's only safeguard and offered a one-click lockout of a remote box.
Found on a live host: ufw installed and inactive with one staged rule,
`ufw allow 25/tcp`, and sshd on 22. ufw defaults to deny-incoming, so enabling
would have severed the only way back in. Verified against that host:
ssh_ports() = [22]
ufw allowed ports = [25]
enable('ufw') -> blocked_by ssh_lockout
check_ssh_lockout() reads the live port from `sshd -T` (authoritative: resolves
Includes, Match blocks and compiled-in defaults) and compares it against
`ufw show added`, the rules that will exist the moment it comes up. Named
application profiles are resolved via `ufw app info` rather than assumed.
An undeterminable SSH port REFUSES rather than falling back to 22 — guessing in
the permissive direction is the lockout this exists to prevent. A DEFAULT_INPUT
_POLICY of ACCEPT short-circuits it, since nothing can be locked out then.
enable(force=True) is the operator accepting the risk; never set by default.
POST /firewall/enable answers 409 (well-formed request, conflicting state) with
the offending port and remediation. GET /firewall/ssh-preflight lets the UI warn
before the click instead of reporting the lockout after it.
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>
A VPS resized from 1 vCPU/1 GB to 4 vCPU/8 GB with a 60 GB volume attached,
and the panel never mentioned any of it. Not a stale cache: a resize forces a
power-off, so the process and its in-memory spec cache die and the next boot
reads the truth. The gap is that nothing ever persisted the PREVIOUS numbers,
so there was nothing to compare against — and the reboot that causes the
change is what destroys the only copy of the baseline.
Adds host_snapshots: capture specs + per-filesystem inventory at boot, diff
against the last row, and emit host.specs_changed / host.storage_advisory on a
real delta. Advisories fire on state transitions only, so an unfixed fstab
entry does not nag on every restart.
Repairs a divergence this exposed: resource_tier_service measured the first of
/var/lib/serverkit,/var/lib/docker,/ while capacity_service hardcoded /. Both
now use host_inventory_service.data_path(), and local headroom reports which
mountpoint it measured.
Notifications render in the existing bell UI, so no frontend change is needed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test_internal_error_handler_is_registered read app.error_handler_spec, a Flask
internal whose shape has moved between releases, and proved only that something
was registered. A handler can be registered and still return HTML or log
nothing, so the test could not fail for either way the guarantee actually breaks.
Now driven through app.handle_exception(), Flask's own entry point for an
unhandled exception, asserting the response is JSON and that the request was
logged. Not done by registering a route that raises: the `app` fixture is
function-scoped but wraps a session-scoped Flask object, so the route would leak
into every later test and collide on re-registration. PROPAGATE_EXCEPTIONS is
forced off for the call because TESTING=True turns it on, which re-raises before
any handler runs.
The log assertion matches this handler's own wording rather than just the path.
Flask's log_exception() already emits "Exception on /path [GET]" before any
handler runs, so a path-only assertion passes even with the handler's logging
deleted — confirmed by mutation, which is how the first version of this test was
caught being vacuous. Both mutations (return HTML, drop the log) now fail, on
different assertions.
Raised by a reviewer on the PR.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A `bundled: true` entry promises an extension that ships inside the panel, so it
carries no source/sha256 -- which means every other check in this file skips it.
test_matches_published_index only proves the catalog agrees with the registry,
not that either agrees with the code, so a bundled entry could name a version
the panel does not ship, or an extension it does not ship at all, and nothing
would notice.
Checks each bundled entry against builtin-extensions/<slug>/plugin.json. Offline
and deterministic, so unlike the published-index comparison it fails on the PR
that introduces the mismatch rather than needing the network.
Only asserted in this direction: a builtin deliberately absent from the catalog
is a product decision, not rot.
Verified non-vacuous by mutation -- claiming a wrong version and naming a
non-existent bundled slug each fail, on different assertions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tracing issue #101 turned up that a panel failing to serve its own agent
installer left no diagnostic trail at all. The backend had exactly one error
handler -- the 404/SPA one -- no 500 or HTTPException handler, and nothing
anywhere configured logging, while only 2 of 100 app/api modules ever call
logger.error.
Adds:
- an errorhandler(500) that logs the unhandled exception with its method and
path and returns JSON. Flask routes here only when it is not propagating, so
pytest and the dev server still re-raise with a full traceback.
- an errorhandler(HTTPException) so framework-generated errors under /api/
(405 from the router, 413, abort()) return JSON instead of Werkzeug's HTML
page to clients that are parsing JSON. Non-API paths keep the HTML page, and
the more specific 404/500 handlers still win for those codes.
- _configure_logging(): a stderr handler with a timestamp/level/logger-name
formatter, level from LOG_LEVEL. Services across the codebase log through
logging.getLogger(__name__); with no root handler those records fell through
to logging's last-resort handler -- WARNING and above only, unformatted, INFO
dropped on the floor.
Two things the container run caught while writing this: Flask's default handler
has to come off app.logger or every record prints twice (once as %(module)s via
Flask's handler, once as %(name)s via ours, both reaching stderr through
propagation), and the level must not key off DEBUG alone -- TestingConfig sets
DEBUG=True, which would have moved the whole test suite to DEBUG logging as a
side effect. Both are pinned by tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three changes to the endpoints that serve the fleet-agent installers.
A missing installer answered 404. That is what sent issue #101's reporter
looking for a wrong URL -- they filed it as "incorrect paths in setup" -- when
the file had simply never been shipped into the image. The status code produced
the misdiagnosis. It is now 503, the response names the path that was searched,
and the same path is written to the panel log, because the response body does
not survive the `curl -f` the enrollment one-liner uses: curl discards it and
prints only "The requested URL returned error: 404". For an endpoint consumed by
curl, the status code and the server log are the only diagnostics that reach
anyone.
The panel's release lookup pointed at jhd3197/ServerKit and filtered for
`agent-v*` tags, which exist nowhere since the agent moved to its own repo, so
/api/v1/servers/agent/version returned 503 to every agent polling for an update.
It now uses AGENT_GITHUB_REPO (env SERVERKIT_AGENT_GITHUB_REPO) and a ^v(\d…)
tag filter that skips the agent repo's malformed "v" release.
_load_installer() is shared by the .sh and .ps1 routes so they cannot drift, and
SERVERKIT_SCRIPTS_DIR overrides the resolved directory for unusual layouts.
New tests cover the endpoints, the release lookup, and -- the one that would
have caught the original bug -- a packaging invariant that parses the
Dockerfile's COPY sources and asserts every installer the panel serves is
actually copied into the image. No endpoint test could have caught it: pytest
runs against the source tree, where the file always exists.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
app/data/registry_index.json is what the Marketplace falls back to when the
live registry is unreachable, and what the test suite reads. Seven of its ten
entries carried a sha256 that does not match the artifact their own source URL
resolves to, at the same version: serverkit-gpu's zip hashes 3872d71f..., the
bundled copy claimed c861dafa.... A panel that fell back to this file would
download the correct, untampered release and reject it as tampered, on exactly
the panels already cut off from the network. It also sat nine entries and one
version behind (WordPress 1.0.0 against a published 1.0.1).
Nothing caught it because nothing checked it: the file was hand-maintained,
had no generator, and no test referenced it. A wrong-but-well-formed digest is
indistinguishable from a right one without fetching the artifact or the
published index.
Regenerated as a straight mirror of serverkit-extensions/index.json, and the
note now says it is a mirror and must not be hand-edited. The new test compares
the pinned (version, source, sha256) tuples against the published index and
skips only when the index is unreachable, so offline is not a failure but a
reachable mismatch is. Verified it fails against the old file, naming all seven
digests and the WordPress version, and passes against the regenerated one.
Also corrects the signed-releases roadmap line I marked done earlier today: the
verification path and the consent UX shipped, but no published release actually
carries a signature and the registry schema did not even document the field, so
every first-party install still takes the unsigned-consent path.
Issue #96's redirect loop came from the nginx site we install, not the app --
but nothing pinned "the app never redirects to HTTPS", so the docs relied on a
property no test kept true.
Covers: no redirect to https from the curated entry routes, the same under
X-Forwarded-Proto: https (the header any force-HTTPS middleware would key on),
and a sweep of every no-argument GET rule in the URL map so new blueprints are
covered without anyone remembering to add them; SERVERKIT_PUBLIC_URL auto-allowed
as an origin, trailing slash normalised, additive to CORS_ORIGINS and never '*';
that value reaching Socket.IO with async_mode='threading'; and the handshake
accepting the public origin while rejecting a stranger.
Mutation-checked rather than merely green: injecting a before_request HTTPS
redirect fails the sweep on all 352 routes it walks, and the handshake's 400
flips to 200 when the same origin is allowlisted.
ProxyFix on/off and HSTS gating are deliberately not duplicated here -- they are
already covered by test_trusted_client_ip.py and test_security_headers.py.
The config fixture loads config.py as a throwaway module rather than calling
importlib.reload. reload looks equivalent and is a trap: it rebinds
config.TestingConfig to a new class object while app/__init__.py still holds the
original config dict from its `from config import config`, so tests that
monkeypatch TestingConfig patch the new class while create_app reads the old one
and ProxyFix silently never turns on. That broke 9 tests in
test_trusted_client_ip.py and test_login_bruteforce.py, in a full run only.
Full suite: 3542 passed, 48 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 55 task 6, backend half. Installing a plugin's requirements runs pip with
the backend's privileges, and a setup.py hook is arbitrary code, so it is
opt-in and off by default -- correct, and the part that was missing is that
nothing ever said so. The skip was a logger.warning: the requirements.txt was
written next to the extension and the panel showed nothing, so an extension
could sit there running with imports it does not have and the operator had no
way to find out short of reading the install log.
pending_requirements() answers it: whether a file is pending, its contents, its
path, the parsed package lines, and the exact env var that changes the
behaviour. GET /plugins/<id>/requirements exposes it, admin-only.
Strictly read-only. Nothing here installs anything -- the plan is explicit that
this surface introduces no auto-install behaviour change -- and a test pins
that by failing if _install_requirements is reached.
The env check the installer inlined is now plugin_pip_enabled(), shared by both
sides. If the surface and the installer ever disagreed about what "enabled"
means, the page would describe a behaviour the installer does not have.
Two honest edges: the content is capped at 64KB before being echoed into a
browser, and pip_enabled is reported alongside pending, because a file written
while the opt-in was off survives turning it on -- the surface reports both
facts rather than guessing which applies.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 55 task 7, backend half. A manifest's `permissions` array was
declaration-only: the install dialog showed it and nothing ever compared it
against what the extension went on to do.
The capability gate now records every call through it -- including the
refusals, which are the interesting ones -- so `usage_report(slug)` can answer
declared-versus-observed per extension. Observations are in-memory and
per-process by design: the question is "what has this extension done since the
panel started", and persisting it would outlive the evidence and start reading
like an audit trail, which this is not. `has()` deliberately does not record; it
is a question, not a use, and counting it would fabricate activity for callers
that only checked.
The larger half of this change is admitting what the panel cannot see. Task 7
was specified as hooks at "the shell/docker/db helper entry points", and those
do not exist: the SDK exposes no shell, docker, filesystem or network helper at
all, and hands out `db` as raw SQLAlchemy. An extension wanting any of them
imports the host module directly, where no in-process gate is involved. The one
genuinely gated capability is `agent.command:<action>`, because the SDK is the
only way to reach an agent.
So observability is data, not prose: OBSERVABLE_PREFIXES marks what the gate
truly mediates, and the report marks every other declared permission
`observable: false` rather than showing it as "never used". Absence of evidence
is not evidence of absence here, and a UI claiming otherwise would be inventing
enforcement the panel does not have -- exactly the honesty the plan's D2
decision asks for.
GET /plugins/<id>/permissions returns the report, admin-only: it describes what
third-party code has been reaching for.
The UI surface and the docs follow.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 72 item B.1. Every redeploy path reached its point of no return -- a
compose down, or a stop+remove -- before anything had checked that the new
version could actually run. An invalid compose file, an image that no longer
pulls, or a port somebody else took stopped the site and left it stopped,
nginx 502ing until a human intervened. The fix is not blue/green (that is
B.2/B.3, deliberately not attempted here). It is ordering.
deploy_preflight_service runs everything checkable while the old containers
keep serving: docker compose config --quiet -- wiring the long-existing
validate_compose_file, whose only caller until now was a manual API endpoint
-- then image resolution to completion, then host port conflicts. Nothing in
it stops, removes, recreates or rewrites anything; its only side effects are
pulls and builds, which are additive. Each caller aborts on a blocking
finding, and the log line says the running deployment was left untouched,
because that is the whole user-visible payoff.
Three paths were unsafe and are fixed; each now carries an explicit marker at
the line past which the live deployment is gone.
The single-container path had the sharpest version of the bug: the registry
pull sat BELOW the stop and remove, so a registry hiccup left the app with no
container at all. The build already ran earlier, so the pull is the only
pre-traffic requirement, and it now happens first. The template update
renders the new compose to a sibling dotfile and validates and pulls THAT, so
the live docker-compose.yml is untouched until validation passes; same
directory, so relative bind mounts and .env resolve identically. The git
webhook default builds before it stops, and its later up --build is a cache
hit.
Everything is fail-open, which matters more than the checks themselves: when
compose config cannot be introspected the dependent checks are skipped, and a
pull failure is downgraded to advisory when every referenced image is already
present locally, so a locally-tagged image that was never pushed keeps
deploying exactly as before. A preflight must never abort a deploy the old
code would have run.
The port check needed reframing rather than reuse. The probe itself is
ManifestApplyService._port_bound, called through the class so the existing
test seam serves both surfaces, but a bare probe flags the deploy's own live
container and would abort every ordinary redeploy. It only fires for ports the
new config wants that the running project does not already publish, and skips
entirely when ownership cannot be determined. It is deliberately not applied
to the single-container path, where app.port is held by the app's own
container when one exists and there is nothing live to lose when one does not.
Paths already safe were left alone rather than given ceremony: the
zero_downtime restart builds first and never downs, template install has no
live container, and the upload/rollback/image-update paths already pull or
build before compose up.
Also fixes a pre-existing bug found in update_app: install_info was assigned
only inside a try but read unconditionally later, so a missing or corrupt
.serverkit-template.json raised NameError *after* compose_down and left the app
stopped with a new compose that was never started -- the exact outage this
item targets.
No existing test asserted the old unsafe ordering, so nothing had to be
rewritten. The new tests prove the ordering directly: on a validation failure
compose_down is never called, the template's compose file is byte-for-byte
unchanged, and stop_container/remove_container are never reached on a pull
failure.
Not verified: no real Docker was exercised -- every docker seam is stubbed, so
build/pull exit behaviour on a mixed project is reasoned-about, not observed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 72 item A.2. FleetDoctorService, FleetRepairService, the FleetDoctorResult
model, migration 063 and their tests were all complete and reachable from
nothing: no routes, register_jobs() never called, no seeded schedule. The
frontend was further along than the backend -- getFleetDoctorReport() and
runFleetSweep() have been calling /doctor/fleet and /doctor/fleet/run, two
endpoints that did not exist, so the multi-server health story the agent
platform advertises 404'd on contact.
Nothing in those services was redesigned. The one addition is a read-side
aggregator: they could run a sweep and persist rows but had no way to hand
back a per-server report, so fleet_report() composes one. It lists every
registered server rather than only connected ones, which is what makes an
unreachable box a reported state -- last known rows plus connected: false --
instead of a hole in the report. Persisted rows key on check_key while the
UI's field-driven renderer keys on key, so the report emits both.
Three admin-only routes, matching api/doctor.py's gating exactly. The sweep
returns 202 with a job id and never runs on the request thread: it fans out
across agents over the network, and the agent gateway registry is
single-worker and in-memory. Repair is a thin passthrough to FleetRepairService,
which already owns the allowlist, capability gate and audit trail; refusals
come back as per-item results inside a 200, so one offline server cannot fail
a batch. The batch is bounded at 25 so a single request cannot fan an
unbounded number of remote mutations at that single worker.
The schedule is daily like the host doctor but starts 30 minutes behind it,
so the fleet fan-out contends with neither the local sweep nor the boot-time
agent reconnect storm.
The frontend gets a Fleet Doctor section rather than losing its dead methods.
The per-check article was extracted into one component now shared by the host
and fleet lists, so fleet findings render through the same
repairable/repair_ref path instead of a parallel implementation; the confirm
modal says plainly that a fleet repair acts on a remote server through its
agent and is audited.
test_jobs' builtin schedule count moves 21 -> 22 for the new schedule. The
builtin.* kind count is untouched -- this kind is doctor.fleet.run.
Note for anyone reading the suite: tests/test_fleet_doctor_dns_rotation.py is
skipped at module level and proves nothing here. Its rotating-DNS-window API
did not survive the recovery rebuild; that is pre-existing and unrelated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to 07dbffbe, which took the wrong branch. That commit noticed
verify_run needs a remote copy and responded by withholding the Repair button
from local-only backups. It also broke test_doctor_backup_proof_matrix, which
asserts every unverified policy gets a repair_ref -- and that test was right.
Most installs have no offsite storage configured, so hiding the button there
means the check that tells you whether your backups can be trusted offers a
fix to almost nobody.
There were always two verification ladders and only one was wired. A run with
an offsite copy is verified against the provider by verify_run. A local-only
run is verified in place by backup_verify_service.verify_run_tier1 -- read the
archive, checksum it against the stored manifest, set the run's level to
listed or hashed. That function was complete, tested and reachable from
nothing: it had zero callers in application code. The repair now picks the
ladder from what the run actually has, so the button works either way and the
check goes back to offering it unconditionally.
Tier 1 mutates the run's verify ladder without committing, so the commit is
the repair's -- otherwise the level is discarded and the next doctor sweep
reports the same backup unverified again. A test pins that.
Both local failure modes are reported as failures rather than fixes, matching
how the remote mismatch is already handled: level 'none' means the archive
could not be read at all, and a 'listed' level carrying a verify_error means
it is readable but is not the archive the manifest describes. Either is worth
knowing before a restore depends on it, and neither is a repair.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 72 item A.1. The backup_drill_stale and backup_unverified checks both
emitted repairable: True, but DoctorService.repair() only knew drift, service,
dns and extension. Every Repair button those checks rendered came back
"Unknown repair kind: backup_drill" -- a button that failed 100% of the time,
on the check family whose entire job is telling you whether your backups can
be trusted.
The dispatcher now handles both, and they are deliberately asymmetric because
the underlying services are. request_drill enqueues a backup.drill.run job, so
that answer carries the job id for the console to link to. verify_run does the
size and checksum comparison inline and returns a verdict, so there is no job
id -- claiming one would send the console looking for a job that does not
exist. The plan assumed both enqueued; only the drill does.
Verification finding a BAD copy is reported as a failure, not a fix. It is a
worse outcome than "unverified" and calling it a successful repair would be a
lie told by a green checkmark. Both branches turn their services' ordinary
refusals -- a drill already in flight, no successful backup to drill, backup
not found -- into plain error text rather than letting them escape, and a
policy_id echoed back for a since-deleted policy reads as gone instead of
raising.
The second half is the same defect from the other side. verify_run compares
against the REMOTE copy and raises when there is none, so a local-only backup
was rendering a Repair button that could never succeed. That check now only
offers the button when there is something to verify against, and otherwise
says what would actually fix it: send the policy offsite, or run a restore
drill.
Drills started this way are attributed to 'doctor' rather than 'manual', so
the drill history says where they came from -- the field is free-form and only
displayed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fallout from the plan 73 item 1 preflight, caught by a full-suite run. Both
CAA issuance tests mock run_privileged to simulate a successful certbot, but
obtain_certificate now probes the real host for an nginx binary and an enabled
certbot nginx plugin before it builds the command. Neither exists on a dev box
or a CI runner, so the preflight short-circuited and the certbot run the tests
are actually about never happened.
They opt out of the preflight explicitly, the way test_ssl_certbot_resolution
does for the cases that are not about it. The preflight itself is unchanged --
refusing to invoke `certbot --nginx` on a host with no nginx is the point of
that item, and it keeps its own coverage.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 73 item 8. The installer opened 80/443 on whatever firewall it found and
shrugged -- "assuming ports 80/443 are already open" -- when there wasn't one,
which is the state of a stock Ubuntu/Debian VPS. That box then ran ServerKit
with every port a deploy maps onto the host (8001-8999, plus database
containers on their stock ports) facing the internet. fail2ban only shipped
with the "full" capacity profile, so the typical small VPS got neither.
The dangerous half is enabling default-deny without allowing the port sshd
actually answers on, over the SSH session running the installer. detect_ssh_ports
therefore never guesses: it asks `sshd -T` (the effective config, which resolves
the Include that Ubuntu 22.04+ ships), then sshd_config plus its drop-ins with
comments stripped, then a live `ss`. Multiple Port lines all come back. 22 is
used only when a config exists and names no port -- sshd's compiled-in default
-- and if nothing answers at all the function fails and no firewall is
installed. An exposed box is recoverable; a locked-out one usually is not.
That interlock had a real bug in its first draft, caught before it shipped and
now pinned by a named regression test: most boxes have no sshd_config.d/, the
unmatched glob makes cat exit non-zero, and under `set -o pipefail` that wiped
a correctly parsed answer and fell through to the 22 default -- returning 22 on
a box whose sshd is on 2222, which is precisely the lockout this function
exists to prevent.
Ordering is the other safety property and is asserted: every allow rule (all
detected SSH ports, 80, 443, and PANEL_PORT when non-standard) lands before
`default deny incoming`, and the policy before `--force enable`. The bootstrap
refuses to act inside a container, when a firewall is already active (it is the
operator's -- opening ports on it is fine, rewriting its default policy is not),
under SERVERKIT_SKIP_FIREWALL=1, and when a previous run bootstrapped but the
firewall is off now, since the operator turned it off deliberately. Non-Debian
installs are byte-identical to before: RHEL-family boots with firewalld already
active, and a firewalld default-deny bootstrap for boxes where it is inactive
is marked as an explicit follow-up rather than guessed at.
fail2ban moves to the standard profile with an sshd jail that watches the same
ports detect_ssh_ports found -- a jail pinned to :22 on a box listening on :2222
bans nobody -- and declares `backend = systemd` when there is no auth.log to
tail, without which the jail refuses to start and fail2ban looks installed while
protecting nothing. certbot stays full-only.
setup-health gains firewall (critical) and fail2ban (recommended) items. The
split is deliberate: this file grades critical by silent breakage, and an
unfirewalled box qualifies -- the operator set it in motion by deploying,
nothing in the UI says the port is public, and the failure mode is compromise
rather than an error. A missing fail2ban thins defence in depth but breaks
nothing silently, since the panel already throttles logins per IP. Both are
gated on actually owning the host -- Linux, /etc/serverkit present, not
containerised -- so a dev checkout or CI runner drops them entirely instead of
reporting a fake ok that would inflate the score.
SetupHealthWidget was orphaned; so, it turns out, was its stylesheet, which
main.scss never imported. Both are wired up, as a plain admin-only card above
the board rather than a placeable widget: an unfirewalled box has to be visible
without the operator first opting into showing it. It collapses to one line
when everything is clean.
Implementing this inverts a shipped test that asserted provision_hardening was
a no-op on the standard profile; it is rewritten to the new contract rather
than deleted, with a minimal-profile case added alongside.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 73 item 5. The Domains SSL modal asked for a contact email on every
single certificate, and the backend hard-required one per call. The app-detail
Settings tab did remember it -- in localStorage, so the memory was per browser
and invisible to the other surface. The two never agreed, and neither could
help the paths with no form at all: domain attach passed `admin@<host>` when
it had nothing else, an address that usually does not exist, which is where
every expiry notice for those certificates went to die.
There is now a single panel-wide ACME contact in system settings.
get_acme_contact() resolves an explicit address first and falls back to the
stored one; remember_acme_contact() writes it back whenever a request carries
an explicit address. Both are best-effort on the settings side by design: a
settings failure must not break issuance, and failing to remember an address
must not turn a successful certificate into an error. An empty input never
clears a stored contact, since the automatic paths pass whatever they were
given and that is frequently nothing.
Wired into every issuance entry point -- the domain SSL enable route, the
generic certificate route, and domain attach, which now consults the stored
contact before falling back to the admin@<host> guess. The address is only
remembered after issuance actually succeeds; remembering one Let's Encrypt
rejected would be worse than remembering none.
Prefill needed its own endpoint. The settings routes are all admin-gated, but
issuing a certificate for an app you can edit is not an admin action, so
GET /ssl/acme-contact is jwt_required only -- it returns an address the
operator chose for notices, not a credential. The Domains modal and the
Settings tab both prefill from it, and the Settings tab keeps its localStorage
seed so a browser that already has the value shows it without a flash.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 73 item 7. Let's Encrypt prefers IPv6 whenever a AAAA record exists, so a
domain whose A record points at this server perfectly still fails HTTP-01
issuance when its AAAA points somewhere else. That shape looked healthy
everywhere in the panel, which is what made it expensive: the operator saw a
verified domain, a green doctor report, and a certificate error naming nothing
either of them had mentioned.
Both DNS surfaces were blind to it for different reasons. The doctor's sweep
returned ok as soon as the server IP was *among* the resolved addresses, so
the extra IPv6 answer never registered -- and that branch is reached precisely
when the A record is correct, i.e. exactly the case worth reporting.
verify_domain used socket.gethostbyname, which is IPv4-only and cannot see a
AAAA record at all.
Both now compare every resolved address against this server: the configured
public IP plus the host's own interface addresses, so a correct dual-stack
setup stays quiet and only a genuine stray is named. Comparison is by parsed
value rather than by spelling, since an expanded 2001:0db8:... form and its
compressed equivalent are the same address, and psutil's zone ids (fe80::1%eth0)
are stripped first. Interface enumeration happens once per doctor sweep
instead of once per domain.
The doctor reports this as a warn, not a fail: the site itself is reachable
and it is a stray record rather than a broken one. Both messages say what to
do -- remove the AAAA record or point it at this server's IPv6 address --
rather than only naming the symptom.
verify_domain keeps its existing response shape. ip_address still carries a
single IPv4 address for the callers that predate this, with the full set added
alongside as ip_addresses, plus aaaa_conflict and a warning string. The
Domains page needed the last part to matter: it printed a green "Domain
verified!" toast for anything that resolved, so a conflict the API now
reports would have gone straight back to being invisible.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 73 item 9. Three blind spots with one theme: the panel trusted a signal
that did not mean what the UI claimed it meant.
The doctor's service sweep only asked `systemctl is-active`. nginx keeps
reporting active while serving a config it can no longer parse, so a box that
was one `nginx -t` away from explaining itself got a clean bill of health. A
new nginx.config check runs that test and fails with the file and line nginx
named. It is deliberately not repairable: config repair is operator territory
and a wrong guess takes every site on the host down. A missing nginx binary
warns rather than fails, since a profile without nginx is legitimate.
NginxService.restart now tests the config first, the way reload always has.
The asymmetry is what made this worth fixing: a reload against a broken config
fails safe -- nginx rejects it and keeps serving the old one -- while a restart
stops nginx before starting it, so the same broken config becomes a full
outage with nginx dead until someone fixes it by hand. Testing first turns
that outage back into an error message.
get_services_status substring-matched psutil process names while the
start/stop/restart buttons went through systemctl, and the two disagreed in
both directions. `mysql` matched the process `mysqld`, so a MariaDB box whose
unit is `mariadb` showed a running `mysql` row whose buttons systemctl could
not honour; `docker` matched `dockerd` and `redis` matched `redis-server`,
listing one daemon twice and three times; and any unrelated process whose name
merely contained a service name reported that service up. It now asks systemd
-- the same authority the control path uses -- so the list cannot claim a state
the buttons are unable to act on.
That query is one `systemctl show` for the whole list rather than one call per
unit, because this backs a polled list. Units the host does not have are
dropped instead of shown as stopped: MONITORED_SERVICES carries several
spellings of each daemon so whichever one a distro uses is found, and the
misses were noise. Aliases resolving to the same unit collapse to one row via
the Id property. If systemctl is absent, times out, exits non-zero, or returns
a block count that does not match the units asked about, the old process-name
heuristic still answers rather than the list going silently empty -- and it
remains the path for non-Linux dev boxes, which have no systemd.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 73 item 3. The quick-access rail links the local target at /etc/nginx,
but /etc was never in ALLOWED_ROOTS, so the shortcut returned 403 for every
user including admins -- a dead button, and our own regression: it shipped
after the review that flagged the surrounding SSL story.
/etc/nginx is now an allowed root, but browse-only. The panel generates those
vhosts itself, so a hand edit is silently overwritten on the next domain
change, and a writable /etc/nginx would hand anyone holding files.write a
root-owned config that the panel itself reloads -- a much larger grant than
"let me look at the effective config", which is all the shortcut is for.
READ_ONLY_ROOTS is a second tier under ALLOWED_ROOTS rather than a special
case for this one path: is_path_writable() = allowed AND not read-only, and
every mutating entry point now gates on it instead of on is_path_allowed, so
adding a future root to the list is enough to make the whole write surface
refuse it. Both comparisons resolve with realpath first, so a symlink planted
in a writable root cannot launder a write into the subtree.
copy is deliberately asymmetric -- copying a vhost out to keep only reads the
source, so only the destination is gated -- while move removes its source and
therefore checks both ends. The upload endpoint repeats the check because it
writes to disk directly instead of going through FileService's methods.
Only this subtree is opened, not /etc: /etc/passwd and /etc/shadow stay
denied, and the panel-internal PROTECTED_ROOTS exclusion is untouched
(GHSA-rm3m-9mvw-68fh).
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>
.gitattributes pins the whole repo to `text=auto eol=lf`, but plugin_service
wrote the tracked manifests through Python's text mode, which translates \n to
\r\n on Windows. Any install on a Windows dev box therefore rewrote
frontend/src/plugins/*/plugin.json with CRLF, and the backend suite -- which
installs builtin extensions -- left four modified manifests behind on every
run. The diff showed no content change, which made it look like unrelated
churn and made it easy to sweep the files into an unrelated commit.
This is the same failure the comment above the write already documents: the
trailing-newline half was found and fixed earlier, the line-ending half was
missed. Both halves now live in the same explanation.
_regenerate_frontend_manifest's write gets the same treatment for consistency,
though nothing covers it -- it is skipped under the testing config so it cannot
be reached from a test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review catch on #97. The chat connection test failure log interpolated
DeliveryResult.error verbatim. _deliver_webhook builds that from str(exc) on a
network error, and requests embeds the full URL in its exception text -- so the
line could carry a Discord/Slack/Teams webhook token or a Telegram bot token,
all of which live in the URL *path*. to_dict() masks `destination` even from an
admin, and /api/logs serves the log files back to the panel, so the log line
leaked past the API's own masking.
Dropping the reason would have restored the earlier problem of a failed test
button leaving no diagnostics anywhere, so it is redacted instead: URLs keep
scheme and host and lose the path, and bare bot tokens are substituted. The
host is what makes the line worth keeping and is not a secret.
Also normalise ChatWebhookConnection.raw_credentials() to {} when the stored
JSON is well-formed but not an object. A hand-edited or corrupt row could hold
`[]`, which every caller then treated as a mapping -- credentials() called
.items() on it, so to_dict() and therefore the list endpoint raised before the
write path was even reachable. Guarding at the source covers all callers and
matches the existing categories() pattern.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The floor had drifted. It was last set to 3234 at 9fb0a6ca; the rounds since
then added tests without bumping it, so it sat ~117 below what CI actually
collected. The ratchet only fails on a *drop*, so the gap was silent -- and a
floor that far below reality stops catching the accidental deletions it exists
for.
Collection is deterministic between a dev box and CI here, so the local count
is the right value: every file under backend/tests/ is tracked (only
__pycache__ is ignored), and the one filesystem-globbing parametrization,
test_extension_manifests.py::_all_manifest_paths, walks builtin-extensions/
and frontend/src/plugins/ where all 16 manifests are tracked. Locally
installed extensions change which tests *skip*, never how many are collected.
Note when re-running this: check_test_count.py collects with a bare `pytest`,
while backend-ci.yml deliberately runs `pytest tests` because a bare collect
also walks locally deployed apps under backend/dev-data/. Both report 3379
today, but only because dev-data errors out of collection rather than adding
to it -- don't rely on that holding.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up to PR #94. ChatWebhookService.update() decrypted every stored
credential and re-encrypted the lot on any credential-bearing request.
decrypt_secret_safe() returns its input unchanged when a value will not
decrypt, so a wrong-key or not-yet-migrated row got wrapped a second time
and became unrecoverable -- and it happened to fields the request never
named. It now patches the ciphertext dict directly and encrypts only what
it assigns, so untouched credentials survive byte-identical.
Also tolerate an unchanged is_default in a round-tripped body (to_dict()
always emits it, so GET -> edit -> PUT rejected an untouched field), and
record the delivery failure reason in the server log. The reason stays out
of the HTTP response: transport errors embed the full webhook URL and any
Telegram bot token.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
