The python-provisioning job failed on ubuntu:22.04 with every apt lookup coming
back empty — software-properties-common not found, so the deadsnakes fallback
could not even be attempted and provision_python fell through to the source
build.
The installer was not broken. main()'s order is
identify_system -> choose_pkg_manager -> ensure_bootstrap_tools -> ... -> provision_python
and ensure_bootstrap_tools is what refreshes the package index — its own comment
says minimal images "ship with empty package indexes". This test called
provision_python without it, so on a fresh container the apt lists were empty
and the FIRST `apt-get install` had no candidate for anything. A test that calls
production functions out of order invents failures; "fixing" install.sh to
satisfy it would have made the real installer worse.
Why it was not caught locally: my verification ran `apt-get update` before the
test, while the CI step is `command -v bash || { apt-get update && install bash; }`
— Ubuntu already ships bash, so that update never runs. I had verified a machine
state CI does not have. Re-verified by running the CI command VERBATIM.
All eight matrix images, exact CI invocation, 7 passed / 0 failed each:
ubuntu:22.04 -> 3.12 (deadsnakes) debian:12 -> 3.11
ubuntu:24.04 -> 3.12 debian:13 -> 3.13
rockylinux:9 -> 3.12 almalinux:9 -> 3.12
fedora:40 -> 3.12 opensuse/leap:15.5 -> 3.11
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
write_config mints its secrets with `openssl rand -hex 32` and a python3
one-liner. The cross-distro containers install ONLY bash — ubuntu:24.04 and
debian:12 ship neither binary — so under `set -e` the failed command
substitution aborted write_config before it wrote a line, and three assertions
read an empty .env:
✘ stock install produced: []
✘ an exposed backend port still trusted X-Forwarded-For: []
✘ external proxy did not set 2 hops: []
The three T44 cases that passed are exactly the ones taking write_config's
existing-.env early return, which never reaches key generation — that split is
what identified the cause.
My bug, and the avoidable kind: T44 was the one part of this suite that leaned
on real binaries, in a file whose whole premise is that it stubs every external
tool. It passed locally only because a dev box happens to carry openssl and
python on PATH.
Stub both inside the three affected subshells. Shell functions take precedence
over PATH lookup, including through `"${PYTHON_BIN:-python3}"`, so no PATH
juggling is needed. The stub values are literals rather than built with
seq/tr/head — those are precisely the tools a stripped base image turns out not
to have, which is the bug being fixed.
Verified in the exact failing image: ubuntu:24.04 goes from 121 passed / 3
failed to 124 passed / 0 failed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`apt-get install -y` answers apt's own prompts but NOT debconf's. tzdata,
keyboard-configuration and friends still open an interactive dialog and block
forever. pkg_add captures apt's output, so the installer hangs in complete
silence — no prompt, no progress, nothing to diagnose.
Observed, not theorised: ubuntu:22.04 sat for 60 minutes on 39 bytes of output,
stuck in tzdata.postinst -> debconf frontend. Real Debian/Ubuntu servers are
exposed to the same stall; a container is just where it showed up. Under
`curl | bash` it is worse still, because stdin is the installer script itself,
so debconf can consume installer source as its answers.
DEBIAN_FRONTEND was set NOWHERE in install.sh, scripts/lib/pkg.sh or update.sh.
Every other manager was already guarded (dnf/yum -y, zypper --non-interactive,
pacman --noconfirm) — apt was the only unguarded one, and pkg.sh's own
docstring already claimed the whole function was non-interactive.
In pkg.sh it goes in ARGV as `env DEBIAN_FRONTEND=noninteractive …`, not an
export: _pkg_sudo runs plain `sudo "$@"` and sudo's env_reset would drop an
exported variable before apt saw it. --force-confold/--force-confdef cover the
other prompt class (modified conffiles), which DEBIAN_FRONTEND does not
suppress.
Tests: pkg_install must be non-interactive for every manager, and pkg_add's apt
branch is guarded statically. Three existing assertions pinned the exact apt
argv (`install -y <pkg>`) and now match the package instead — a flag added to
pkg_add should not fail tests whose subject is which package got installed.
Verified across the whole distro matrix: ubuntu:22.04 went from a 60-minute
hang to 7 passed in minutes; debian:13/12, ubuntu:24.04, fedora:40,
rockylinux:9 and opensuse/leap:15.5 all 7 passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Seven distro images were green throughout issue #99 because every one of them
runs the unit suites against STUBS. That is the right trade for logic, but it
left the one thing that talks to real package repos completely untested — so a
new Debian release dropping the Python we ask for could only ever be found by a
user.
scripts/test/test_provision_python.sh runs identify_system, choose_pkg_manager
and provision_python for real, then proves the result is usable: in range, can
create a virtualenv, has working pip, and imports ssl/sqlite3/ctypes. It needs
no systemd, nginx or ports, so it fits the plain containers the matrix already
pulls. Reaching the source build counts as a FAILURE, not a pass: a distro that
gets there is one we have not really added support for.
Wired up as a new `python-provisioning` job over the same images, and debian:13
added to both matrices. Adding trixie to the existing matrix alone would have
been decorative — those legs would have gone green on a distro that cannot
install.
Proven in both directions before being relied on: against the pre-fix tree it
reproduced #99 exactly on debian:13 (the reporter's apt errors, the fall
through to a source build, and the abort message), while debian:12 passed 7/7.
Also fixed the suite pinning the bug. T2 asserted `ver_in_range 3.13` must be
FALSE and T1's stub used 3.13 as its "too new" case, so the gate could not be
widened without the tests failing. Both moved to 3.14, plus a direct guard on
PYTHON_MAX — otherwise a regression to 3.12 could be "fixed" by editing the
assertion to match. 122 passed, 0 failed.
full-stack-test.ps1 documents why debian13 is NOT in the VM harness: no trixie
box publishes a Hyper-V provider (generic/debian13 does not exist,
debian/trixie64 is libvirt-only, bento/debian-13 has no hyperv) and Multipass
is Ubuntu-only. Named there with what remains uncovered on trixie — the full
systemd + nginx + API end-to-end — rather than left as a silent gap.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Issue #99. Three separate bugs, and the apt errors the reporter pasted were the
least of them.
1. The gate. Debian 13 (trixie) ships ONLY python3.13 — no 3.11, no 3.12 — so
PYTHON_MAX=3.12 could never be satisfied from its repos. Ceiling raised to
3.13; locate_python now probes python3.13 first. Ubuntu 26.04 lands in the
same place.
2. The Debian branch asked for versions by name (python3.11, else python3.12),
which is exactly why trixie broke it. It now installs the distro's DEFAULT
python3 first — that tracks the release, bookworm 3.11, trixie 3.13 — and
falls back to explicit versions only for releases whose default is out of
range. Hardcoding 3.13 instead would have relocated the same trap to Debian
14. The probe is locate_python, not `command -v`: a binary existing says
nothing about it being in range or venv-capable.
3. The source-build fallback could never have worked. It ran ./configure and
make while NOTHING in install.sh ever installed a compiler — only the -dev
headers — so on any clean VPS it failed immediately, and every step was
piped to `tail -1` so the real error was discarded. That, not the apt
errors, is what actually ended the reporter's install.
Now: build-essential / gcc+make installed for the right family, an explicit
"no compiler" halt with the command to fix it, each step status-checked with
the log tail printed on failure, and the built interpreter verified to have
ssl, sqlite3 and ctypes (a source build silently omits modules whose headers
were missing, and both failures surface far from here). Dropped
--enable-optimizations: PGO triples a build that is already the last resort,
for runtime speed a control panel never notices.
Verified against real distro repos, not stubs — provision_python run for real
in each container: debian:13 now picks python3.13 (7 passed, was 2 failed and
falling through to the source build), debian:12 python3.11, ubuntu:24.04 and
fedora:40 python3.12, all 7 passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Required for Debian 13 (issue #99), which ships only python3.13. SQLAlchemy
2.0.23 installs there and then dies at import:
AssertionError: Class <class 'sqlalchemy.sql.elements.SQLCoreOperations'>
directly inherits TypingOnly but has additional attributes
{'__static_attributes__', '__firstlineno__'}
3.13 adds those two class attributes and SQLAlchemy's TypingOnly guard rejected
anything it did not expect. Fixed upstream in 2.0.31.
Measured rather than assumed, because this pin sits under every query in the
app. Full backend suite on Python 3.11 with 2.0.52: 3542 passed, 48 skipped —
byte-identical to the same suite on 2.0.23. Run against a throwaway overlay so
the comparison was pins-only, nothing else moved.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SystemService._is_iana_timezone and CronService._same_application were both
declared as
@classmethod
@staticmethod
def _method(...)
which is meaningless and only ever worked because Python 3.9 let classmethod
chain to another descriptor. That chaining was deprecated in 3.11 and REMOVED
in 3.13, so cls stops being absorbed and leaks in as the first argument:
TypeError: SystemService._is_iana_timezone() takes 1 positional argument
but 2 were given
Latent on the 3.11 we ship today, fatal on 3.13. It takes out server-time
display, every cron-to-application operation (suspend/resume/clear/jobs_for)
and, through those, application soft delete and restore.
Plain @staticmethod is correct on every version — calling a staticmethod
through `cls.` has always been fine — so no call site changes.
These were the ONLY two occurrences in the tree. Found while establishing
whether the stack runs on Python 3.13 for Debian 13 (issue #99): they were the
entire delta between 3.13 and 3.11, 8 failures across 4 files. With them fixed
those files are 45 passed on both interpreters.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
install.sh wrote no TRUST_PROXY_HEADERS line at all, so the code default (False)
applied while every host install serves the panel through its own nginx. Per-IP
rate limits, the login brute-force throttle and every audit-log entry therefore
keyed on 127.0.0.1, which quietly weakened all three -- one abusive client shared
a bucket with everyone else. .env.example claimed true, so the two disagreed and
it looked configured.
Trusting X-Forwarded-For is only safe when nothing can reach gunicorn without
traversing a proxy we control, so the switch follows that condition rather than
assuming it. On a host install it holds: render_service_unit binds gunicorn to
127.0.0.1 and bootstrap_firewall opens SSH + 80/443 + PANEL_PORT and never the
backend port. backend_bind_host() is now the single source of truth for both the
systemd unit and the trust decision so the two cannot drift, and
backend_is_loopback_only() gates the setting.
- stock install: TRUST_PROXY_HEADERS=true, TRUSTED_PROXY_HOPS=1
- SERVERKIT_EXTERNAL_PROXY=1: true with 2 hops, unchanged
- SERVERKIT_BIND_HOST not loopback: false plus a warning. The port is directly
reachable there, so the header is attacker-chosen, and a forged IP is worse
than a wrong one because it defeats the throttles that consume it.
Re-runs backfill it into installs that predate this, via _env_default_key, which
fills a gap but never overrides an uncommented value -- flipping someone's
deliberate false back on would be the worst kind of surprise.
.env.example moves to false on purpose: that file is the Docker/manual path and
docker-compose.yml publishes 5000 on all interfaces, so true was the unsafe
default there. Host installs never read it. SECURITY.md claimed "the shipped
nginx deploy sets it", which was untrue until now.
T44 covers all six behaviours, each mutation-checked by line number: disabling
the branch, inverting it to trust an exposed port, and disabling the backfill
each fail exactly one assertion and no others.
Shell suite 121 passed. Backend suite 3542 passed, 48 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Issue #96: an operator fronting the panel with Caddy and a smallstep private CA
hit a redirect loop, and there was no documented path to follow. The
architecture already tolerated this deployment -- the fix was a config file we
already ship -- but nothing said so anywhere.
Adds an INSTALLATION.md section covering both topologies (external proxy ->
ServerKit nginx, full features, HOPS=2; external proxy -> the app directly,
panel-only, HOPS=1), why the loop happens, how to count TRUSTED_PROXY_HOPS,
Caddy (with acme_ca for a private CA), Traefik, Nginx Proxy Manager and plain
nginx snippets, what a private CA does and does not require (the panel never
sees it; a remote agent's host must trust the root), and a symptom-to-cause
table for the loop, stale HSTS, the rejected websocket and wrong client IPs.
Also corrects the Docker documentation, which had drifted badly:
- README (en/es/pt/zh-CN), INSTALLATION and DEPLOYMENT all told people to run
`docker compose up -d` and reach the panel at http://localhost. It starts the
all-in-one container on :5000, and a containerised panel cannot manage its own
host -- now stated up front rather than discovered.
- DEPLOYMENT is a host-install guide that told operators to run
`docker compose build && up` to fix a panel, which would build a second panel
beside theirs, and carried a certbot cron copying certs and restarting a
container that does not exist on a host install.
- CLAUDE.md, AGENTS.md, CONTRIBUTING.md and LOCAL_DEVELOPMENT.md (x3) pointed at
a docker-compose.dev.yml that has never existed; the snippet offering to
create it referenced a frontend/Dockerfile.dev that does not exist either.
Every anchor introduced here was checked to resolve.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
Make "someone else terminates TLS in front of the panel" a supported install
shape instead of something you reverse-engineer from this script (issue #96).
SERVERKIT_EXTERNAL_PROXY=1 implies SERVERKIT_SKIP_SSL=1, so the plain-HTTP nginx
site is selected and there is no 301 for the external proxy to follow back into
itself; it also writes TRUST_PROXY_HEADERS=true and TRUSTED_PROXY_HOPS=2 (their
proxy appends the real client, our nginx appends their proxy) and asks for
SERVERKIT_PUBLIC_URL, deriving it from PANEL_DOMAIN when it can and warning
rather than blocking when there is no tty. Without that URL the Socket.IO
handshake is rejected on origin and the UI loads but never live-updates.
It applies on re-runs too, via a new _env_set_key helper. "Convert my install to
sit behind Caddy" is a re-run and lands in the existing-.env branch, which
previously only refreshed the ssl mode and profile -- so the documented
conversion path would have silently done nothing. A plain re-run never removes
proxy settings someone configured by hand.
SERVERKIT_CONFIG names a KEY=VALUE file of defaults for the same variables. It
is PARSED, not sourced: sourcing would let file assignments overwrite the
environment, inverting the documented precedence, and would execute anything
else in the file as root. Applying keys one at a time and skipping names already
set gives the stated semantics safely.
scripts/test/test_install.sh T43 covers all of it, including that a command
placed in the config file does not run, that the converted .env flips
SERVERKIT_SSL_MODE to insecure (config resolves the env var before the file, so
writing /etc/serverkit/ssl-mode alone would leave HSTS on), and that a default
install is untouched. T12 gained guards for the compose and Dockerfile
regressions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
serverkit.conf and serverkit-insecure.conf both declare
`listen 80 default_server`, and both end by including
/etc/nginx/serverkit-conf.d/*.conf. write_config dropped canonical-domain.conf
into that directory with its own `listen 80 default_server` whenever
PANEL_DOMAIN was set, and nginx rejects that outright:
[emerg] a duplicate default server for 0.0.0.0:80 in
/etc/nginx/serverkit-conf.d/canonical-domain.conf:2
Reproduced against real nginx in BOTH ssl modes. main() runs configure_nginx ->
write_config -> launch_services, and launch_services starts nginx after the bad
file exists, so the failure lands during the install -- and install.sh runs
`nginx -t` nowhere, so nothing said why. Nothing ever removed the file either.
Use a regex server_name instead. nginx resolves exact names, then wildcards,
then regexes, and only then the default server, so this block still wins for a
bare-IP Host while every other host falls through to the panel. That also drops
the `if`, which nginx documents as best avoided.
Verified end to end against real nginx: `nginx -t` passes; Host: 203.0.113.9 ->
301 to the panel domain; Host: panel.example.com -> 200 panel; any other host ->
200 panel.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two independent bugs, both surfaced by issue #96.
The image died on every run with "sqlite3.OperationalError: unable to open
database file": config.py defaults DATABASE_URL to
sqlite:////app/instance/serverkit.db, the Dockerfile never created
/app/instance, and SQLAlchemy will not create a parent directory. Creating it
before the USER switch also fixes named-volume ownership -- Docker seeds an
empty volume from the image directory, permissions included, but creates it
root-owned when the path is absent from the image.
Separately, every docker-compose.yml service sat behind the legacy-frontend
profile, so `docker compose up -d` started nothing at all while the README and
install guide both gave that command as the Docker install. Following the
profile hint yielded the legacy nginx container alone -- a static file server
plus a proxy to a backend on the host -- serving an empty bind mount, which is
the 403 the reporter saw. Default `up -d` now runs the all-in-one image on
:5000 with a named volume; the legacy service stays profiled and says in a
comment that it needs a host install.
DATABASE_URL is pinned rather than interpolated. .env.example shipped a
*relative* sqlite path, which resolves against the container WORKDIR instead of
the volume and is destroyed by the next `compose down`; anyone who copied that
.env would inherit the trap. SERVERKIT_DATABASE_URL is the documented override.
Verified against a real build: boots healthy, serves the SPA, survives a
container recreate on the volume with 143 tables intact, and comes up with no
.env present at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Found by the full-stack VM harness, which 3524 unit tests and a 7-image
container matrix had all missed. On a real Ubuntu 24.04 install the log read
"Opening HTTP/HTTPS (80, 443) via nftables" and not one line of
bootstrap_firewall appeared: it returned at its "a firewall is already active"
guard, so the box got its two ports opened and no default-deny policy at all.
The headline feature of that work was a no-op on exactly the population it was
written for, and it looked green the whole way.
The guard asked `[ "$(firewall_detect)" != "none" ]`. firewall_detect answers a
different question — "which backend do I open a port THROUGH" — and returns
`nftables` whenever the nft binary exists and can list a ruleset, including an
empty one. That is true of every modern Linux, so the guard reported every box
as already protected. Verified directly against a stock-image fixture: same
inputs, `firewall_detect` -> nftables, inbound-default-deny -> unprotected.
firewall_inbound_default_deny() asks what the bootstrap actually cares about:
does this host already deny inbound by default? Presence of rules is
deliberately NOT the test -- Docker installs plenty of nft and iptables chains
and protects nothing. A default-deny policy on the input hook is: firewalld
running, ufw active *with* "deny (incoming)", an nft base chain hooked to input
with policy drop or reject, or an iptables INPUT policy that is not ACCEPT.
Unknown answers 1 (unprotected), because the caller's response is to offer
protection and every caller re-checks before acting.
firewall_detect is left alone -- configure_firewall legitimately wants a
backend name for a binary that exists -- and both functions now carry a comment
saying which question they answer, since that is what was conflated.
Six cases pin the behaviour, five of them shapes the old guard got wrong: an
empty nft ruleset must run the bootstrap, Docker's chains must not read as
protection, an active-but-allow-inbound ufw must be fixed rather than
respected, a real nft policy drop must be left alone, and the two predicates
must disagree on a stock image -- that last one asserted at the predicate level
so the distinction survives a refactor of either function. make_stub_stdout is
new because an exit-code stub cannot express "the tool ran fine and reported
nothing", which is the whole stock-image case.
Green on all seven CI images (265/265/265/275/275/275/263, 0 failed).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three entries for what shipped across this round, written from the operator's
point of view and without internal plan numbers per the file's convention: a
failed deploy no longer stopping the live site, the health doctor covering the
whole fleet with repair buttons that work, and the extension trust view that is
candid about which permissions the panel can and cannot verify.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 55 tasks 6 and 7, UI half — one Permissions dialog per installed
extension, following the existing config-dialog pattern on the same page.
The design problem here is entirely about not overclaiming, so the three
permission states render as three separate sentences rather than one vague
row shared by all of them. A capability the SDK actually mediates and has seen
used says so with a count. One it mediates and has NOT seen says the gate
recorded nothing and names both readings — not needed yet, or declared more
than it uses. One it cannot mediate renders no count, no zero, and no "unused"
badge at all: it says the panel exposes no gated helper, that an extension
wanting it imports the host module directly, and that the question cannot be
answered here.
Refused calls get their own section under their own heading rather than
sitting among the declared rows, because a blocked attempt did not run and
must never be mistaken for a use. Where a non-observable capability does carry
gate-recorded calls — a plugin can call require() voluntarily — the count is
labelled a floor rather than a total.
Two caveats are stated once rather than implied: observations live in memory
in one backend process and are cleared on restart, and declared permissions
are a consent signal rather than a sandbox, since extension code runs inside
the panel process with the panel's privileges.
The requirements section names the env var from the API response rather than a
hardcoded literal, distinguishes "not installed" from "installed at install
time", and covers the case where the opt-in was switched on after the file was
written — the panel only installs during install or update, so it says to
reinstall. There is deliberately no install button; the surface is read-only
and says so.
Timestamps are pinned to UTC before formatting: the backend stamps them with
datetime.utcnow().isoformat() and no zone suffix, which JS would otherwise
read as local time and skew every relative label by the browser's offset.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 55 Phase 4. EXTENSIONS.md said `permissions` is "**enforced** by the SDK
gate: require_permission(slug, "docker") raises unless docker is declared".
That sentence is true about the function and misleading about the system: the
gate only fires where something calls it, and nothing routes Docker through it.
The SDK exposes no docker, shell, filesystem or network helper at all, and hands
out `db` as raw SQLAlchemy, so an extension that wants any of them imports the
host module directly and no in-process check is involved. Declaring `docker`
does not stop an extension using Docker, and not declaring it does not either.
Both documents now state which capabilities are actually mediated -- today
`agent.command:<action>` and nothing else, because the SDK is the only route to
an agent -- in a per-capability table, and say plainly what declaring does
instead: it tells the operator at install time what the extension claims to
need, and lets them refuse.
SECURITY.md gains an Extension Trust Model section next to the agent one. It
leads with the part that matters most and was never written down: an extension
is in-process code running with the panel's privileges against the panel's
database, nothing sandboxes it, and no permission string changes that. Signing
and sha256 pinning are described for what they are -- integrity of what you
installed, not containment of what it does -- along with the unsigned-prompts,
bad-signature-fails split, and the pip opt-in.
It also explains why the new detail page reports "cannot be observed" rather
than a zero count for unenforceable permissions: absence of evidence is not
evidence of absence, and a "never used" badge there would imply a boundary that
does not exist.
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>
Completes plan 72 A.1's "surface the job id so the UI can link to it". The
repair dispatcher hands back a job_id for repairs that run in the background,
but the panel answered every success the same way: "Repaired 1 item". For a
restore drill that is not true yet -- the job has been queued and will run for
minutes -- so the toast claimed an outcome that had not happened, on the one
check family whose job is telling you whether your backups can be trusted.
Results carrying a job_id now say the repair was started and finishes in the
background, with an action that goes to the Jobs page. Inline repairs are
unchanged.
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>
Two entries for what shipped across this round: the installer now leaves a
stock Debian/Ubuntu box with a default-deny firewall and an SSH jail instead of
a warning, and the certificate path stops failing for reasons the panel could
already see -- certbot found outside $PATH, the nginx plugin checked before
issuance, stray AAAA records explained, one remembered ACME contact, and a
doctor that notices an nginx serving a config it cannot parse.
Written from the operator's point of view and without internal plan numbers,
per the file's convention.
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 10, disposition DROPPED -- not a defect. `serverkit apps
stop/start/logs/delete` were tried on a recorded review and failed because
that surface was never built: the apps group has only `list`. Click already
reports the unknown subcommand clearly, so nothing is broken; what was missing
is any hint about where those verbs do live.
The group help now says it outright, at the exact point of contact. It also
names the trap that most likely caused the confusion: `serverkit services`
*does* start and restart things, but those are system services like nginx and
docker, not applications.
Building CLI app lifecycle would be a feature rather than a fix, so it is not
in this plan's scope.
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 items 4 and 6.
The ServiceDetail ⋯-menu "Open in Browser" entry still pointed at
http://localhost:<port>. It predates the serviceUrl.js work and was missed
when the title and port links were converted, so on any remote panel -- the
normal case -- it opened a link to the operator's own machine. It now uses the
same openUrl the neighbouring links already use, which also means it correctly
disappears when a domain fronts the service and the domain link is the one to
follow.
The email install toast threw the server's answer away: the API returns
per-component results with each component's stderr, and the page rendered a
bare "Install failed" for all of it. The failing components and their reasons
are now dug out of err.data.results and rendered inline under the hero, where
they stay after the toast has gone; the toast keeps the top-line reason plus
the first failure so it remains one line. All failing components are listed,
not just the first -- install_all only returns early on postfix and dovecot,
so dkim and spamassassin can both fall through and fail in the same call.
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>
Juan Denis