docker exec -e MYSQL_PWD=<pw>, mysqldump -p<pw> and friends put the
root DB password in argv, readable by every local account through ps
or /proc. Add secret_env() and hand the password to the child's
environment (bare -e NAME for docker exec), and drop it from the
mysqladmin readiness ping, which never needed it.
Both files used a truncating open() and their loaders read damage as
empty, so a full disk mid-write reset off-site backups to local and
the next cron add replaced every job's metadata with the new job.
Route both through the config store's temp+replace write; an
unreadable file is moved aside instead of being overwritten.
Comments cited plan sections as "(§G3)" or "(§8 unification)", which
point into the same git-ignored plans the plan-number citations did.
References next to a real document (docs/REDESIGN_MAP.md §3) stay. Code
and runtime strings are unchanged (AST and token comparison).
The worst cases the audit named: docstrings and header comments that
told how three old copies drifted, or quoted subprocess counts that
were stale the day they were written, instead of stating the contract.
Each now says what the function does and keeps one line of why. Also
drops the always-zero n_cloud counter that existed only to keep a boot
log line's or-chain intact.
Each was referenced only by its own definition across the panel, its
tests, scripts, builtin extensions and every sibling extension repo
(~350 lines). Imports they alone used go with them.
Comments, docstrings and a few strings cited plan numbers ("plan 29
#9", "plan 86 §B2") from a git-ignored folder, so they pointed at
nothing; one reached users in a bottleneck hint. Code and frontend
strings are unchanged apart from those citations (checked by AST and
token comparison). A vhost still carrying the old asset comment reads
as undrifted, and a guard test keeps new citations out.
A corrupt JSON config read as the defaults and the next save, which
writes back what was loaded, destroyed the only copy of the operator's
settings; it is now moved aside first. Every save also used the same
fixed temp name, so two concurrent writers could rename a half-written
file into place; each write now gets its own temp file and writers to
one path are serialized.
Custom build commands, Python app commands, repo deploy scripts and
registry template scripts ran with os.environ.copy(), handing code from
a repository or registry the JWT signing key, the session key, the
database URL and every SERVERKIT_* credential. app_process_env() builds
their environment without those.
Event subscriptions, the webhook gateway's forward_url and the
Discord/Slack/generic alert hooks posted to whatever URL a user typed,
from inside the network: the panel's own API on loopback, LAN hosts,
cloud metadata. Subscriptions followed redirects and stored the reply,
and the gateway forward can be triggered without logging in.
safe_http.post refuses non-public addresses, connects to the address it
checked so DNS can't swap it afterwards, never follows redirects and
caps the body it keeps. SERVERKIT_ALLOW_PRIVATE_WEBHOOKS=1 restores
internal targets for operators who need them.
A gzip.open() handle passed as a subprocess stream hands the child the
raw fd, so mysqldump/pg_dump wrote plain SQL inside a gzip header and
restores piped compressed bytes into mysql/psql. Backups, policies and
pre-deploy snapshots all produced files that could not be restored.
run_to_gzip/run_from_gzip pump the bytes through Python instead, with
stderr in a temp file so a noisy child can't deadlock the pipe. The two
hand-rolled db_sync dump pipes move onto them too.
Extensions hand-roll their own server selects, domain inputs and copy
boxes because core never exported its versions. Exporting them keeps
an extension page looking and validating exactly like a core page.
Extensions can now contribute walkthroughs via contributions.walkthroughs
in plugin.json. The backend validates definitions against a shared
contract (route, signal, manual, and check completion types), transports
them through the dynamic contribution envelope, and persists custom
walkthroughs from the Studio. Includes the JSON Schema, SDK version gate,
and extension scaffold support.
A deleted app deliberately keeps reserving its name (Plan 70: a restore
must never collide), but every clash site said only "already exists" —
pointing the operator at an app that is in no list (GH report: delete an
app, then a same-name deploy fails with no way forward).
app_name_clash_error() now distinguishes a tombstone from a live row and
names the release path (restore or purge from Settings -> Recycle Bin);
all five clash sites go through it. Live-row wording is unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WP-Phase-4 pattern, in-tree Tier C: pages/Email.jsx, its SCSS partial and a
new self-contained /email API client move into
builtin-extensions/serverkit-email/frontend/; the extension index.jsx now
imports locally (no more core re-export shim) and side-effect-imports the
SCSS so it compiles through the extension's module graph. main.scss drops
the _email partial.
Core keeps only the email-adjacent surfaces core Settings renders: the
DNS-provider methods (core dns_providers blueprint) and the relay tile
methods. serverkit-sdk grows the generic list-page chrome the relocation
needed (DataTableFooter/ListToolbar/SearchField/SortChipBar, ds/grid saved-
view chrome, PageLayout, useTableSort, useColumnVisibility) - SDK 1.4.0,
backend mirror bumped in lock-step; vendor shim regenerated.
Frontend build green, lint 0 errors, i18n check no orphans.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- LocaleContext: reset ready on each language change so non-default
locale bundles trigger a rerender once active
- auth PUT /me: raise typed ValidationError (auth.unsupported_language)
instead of a hand-shaped error body; error-shape ceiling 1145 -> 1144
- i18n.py docstring: point at the real parity test
(test_user_language_preference.py)
Follow-up audit of the extension surface, across all of them rather than one.
THE COMPATIBILITY HOLE. Two places documented an install-time `sdk_version`
check as existing -- app/utils/sdk.py's docstring ("checked at install (manifest
lint) and at load") and the runtime loader's comment, which named
`_assert_manifest_sdk_compatible` -- but `sdk_version_satisfies` had ZERO
production callers; only its own unit test. Consequences split by extension
kind: a runtime ESM extension installed cleanly and failed later at the load
gate, while a builtin/in-repo extension never passes that gate at all, so its
range went unchecked everywhere. The function now exists and runs at install,
beside the min/max_panel_version check, and refuses with what the operator
needs to act: what the extension wants, what this panel offers. Fail-open on a
missing range, matching the load gate's 'grace'. Proved both directions.
This matters now because SDK 1.3.0 added the localization surface and made
i18next/react-i18next shared singletons: an extension that externalizes them
needs a panel at 1.3.0+. Pinning `^1.3.0` is what turns a white screen on an
older panel into a readable message. The bump is additive, so anything pinned
to ^1.2.0 keeps installing and loading. Documented as a version table.
THE AUDIT. Measured, not assumed -- both censuses now take `--root` so they can
run against any extension repo (and refuse `--update` there, since the ceilings
belong to the panel):
2,105 untranslated literals across 13 extension repos (WordPress alone 1,099
in 44 files), plus 78 direct toLocale*/Intl calls that follow the BROWSER's
locale (WordPress 56). None of it was visible to any check before, because
every census stopped at frontend/src.
serverkit-gui -- the one builtin with real UI -- is converted as the worked
example, importing useTranslation/useFormat from 'serverkit-sdk' exactly as a
third-party extension would. That also closes its own toLocaleTimeString.
Ceiling 8055 -> 8034, intl door 124 -> 123.
The probe now asserts the claim the SDK surface rests on: an extension
resolving i18next through the shared vendor gets the HOST's INITIALISED
instance, and an extension-style t() returns Spanish. A second instance would
be uninitialised and every t() in that extension would render English forever
with nothing reporting it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
SDK 1.2.0 -> 1.3.0 (additive).
Auditing the extension surface for plan 79 found the localization gap I
expected and two pre-existing bugs I did not.
The gap: extensions had no way to translate anything. The SDK now re-exports
the HOST's translation surface -- useTranslation, Trans, t, useLocale,
useLabel, useFormat and the intl door -- and i18next/react-i18next join the
shared vendor singletons. That sharing is the load-bearing part: an extension
bundling its own i18next would get a separate, uninitialised instance, so
every t() in it would render its English default forever and nothing would
report it. Exactly the "two React copies" hazard vendorShare already documents,
and exactly the failure mode this plan hit twice in core. They also join
dedupe/optimizeDeps for the same reason React is there.
The bugs, both caused by the same duplication:
- The import map listed `react` and `react-router-dom`, but no shim existed
for either. A runtime extension importing bare `react` 404'd at load.
- The serverkit-sdk shim re-exported 26 of the SDK's 71 exports. An extension
importing Button, Modal, EmptyState, formatBytes or timeAgo from the SDK got
`undefined`. The shim's own comment claimed parity was "asserted by the
frontend build + backend test_sdk_contract"; nothing asserted it.
Three files independently restated the same module list -- the import map, the
namespace capture, and one hand-written shim per module -- so they drifted.
Now vendorManifest.js is the list, generate-vendor-shims.mjs derives every
shim's exports (from the real package's namespace, or from the SDK entry's own
export statements -- never typed by hand), the import map is built from the
same manifest, and `--check` fails the build on drift. 26 -> 85 exports on the
SDK shim, 6 -> 8 shims, all present.
Manifest contributions gain an optional `labelKey` beside `label` for nav, tabs
and command-palette entries, resolved at render by the same translateLabel core
uses -- data cannot call t(), and a label resolved at import would translate
once and never follow a switch. Omitting it renders `label` verbatim, which is
what every existing extension does, so nothing breaks.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 79 A1/A2/A4 + B1/B2.
Delivery deliberately avoids the usual http-backend fetch. `en` is bundled;
other locales are dynamic imports Vite emits as chunks from the app's own
static origin. The sign-in and setup screens render before any API call
succeeds, so a fetched locale either flashes English or -- when the backend
is the thing that is broken -- never arrives, on the one screen whose job is
to say so. It also keeps air-gapped and subpath installs working with no
second path to 404.
Precedence (user row -> localStorage -> panel default -> navigator -> en)
mirrors ThemeContext's, including the split provider: LocaleProvider sits
outermost so a toast raised during theme init is already translatable, and
LocaleSync bridges auth state down, exactly as ThemeSync does.
users.language is nullable on purpose -- "follow the panel default" is a
different state from an explicit choice of English, so existing rows are not
backfilled. The backend restates the supported list to validate writes; a
parity test fails the build if it drifts from the frontend manifest, which
stays canonical.
Two bugs the tests caught, both real rather than test artifacts:
- normalize_language('ES') returned None. The shape gate was case-sensitive,
so two of the three forms browsers actually send were rejected.
- PUT /me is throttled 3/minute for credential edits, and language rides the
same route. Switching language three times returned 429 -- and because the
client applies the change locally first, the preference silently failed to
persist and reverted on the next sign-in elsewhere, defeating the column.
Cosmetic-only payloads are now exempt from that limit; credential edits
stay throttled (proved both ways) and the app-wide 100/minute still holds.
eslint moves to ecmaVersion 'latest' because import attributes are ES2025 and
Node's test runner requires them for the manifest imports.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fetch -> TTL -> last-good -> bundled pipeline that four services
re-implemented (and that produced the bundled-index and proxy-rewrite
incidents) now lives once in utils/remote_index. The extension registry
and theme registry become thin normalizers over it; the registry gains
the failure-retry throttling the theme copy had already hardened to
(last-good under full TTL, bundled held only error_ttl). Deliberate
non-adopters, documented: template_service (keyed per-repo cache with
targeted invalidation) and security_feed (DB-durable last-good for a
daily job). Engine unit tests pin the semantics; registry/theme/signing
suites stay green (test seeds updated to the shared cache seam).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
One shape for process polling loops: Event-based stop (immediate, not
sleep-bounded), lock-guarded idempotent start/stop, optional app-context
per tick, real logger error handling (the sockets loops reported errors
via print() — unreadable in prod), and a run_while predicate for
subscriber-scoped loops. The metrics and container-status broadcasters
convert; thread_ownership now carries ONE entry for the helper's spawn
site instead of a row per hand-rolled thread.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
config.py's parsers move to app/utils/env.py and are imported back; the
three incompatible truthiness variants collapse to one TRUTHY tuple.
Fixes the measured bug: SERVERKIT_ALLOW_PRIVATE_DOWNLOADS=on silently
read false in plugin_service (its tuple lacked 'on'); same for the pip
gate, SERVERKIT_STAGING, and SERVERKIT_DEMO_MODE. Ratchet: no new
os.environ truthiness tuples outside utils/env.py.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
utils/sensitive_data_filter.SENSITIVE_KEY_PARTS is now the union of the
four diverging lists (response filter, audit_service, middleware/audit —
previously the only sink covering totp/otp/csrf/session — telemetry);
all sinks delegate to the shared is_sensitive_key(), and mask_payload()
gives log sinks a scope-free recursive masker. Safe-key exceptions keep
identifier-shaped fields (private_url, certificates, totp_enabled, …)
readable. Ratchet: no SENSITIVE list literals outside the one door.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Plan 75 §G1, the remaining 33 files. Every raw subprocess call site in
backend/app that can go through a door now does. What is left is what
should be:
8 app/utils/system.py the doors themselves
8 database_service mysqldump|gzip, gunzip|mysql, pg_dump|gzip,
gunzip|psql — a pipe is two processes
7 build/docker/db_sync/ftp streaming output, gzip pipes
1 log_service tail -f
2 serverkit-analytics sibling repo; see below
Extensions were migrated in BOTH trees. builtin-extensions/ is the source
and backend/app/plugins/ the live copy, compared by normalised hash;
test_builtin_extension_drift stays green. serverkit-email has both sides
tracked, serverkit-ftp and serverkit-cloud-provision only the source.
serverkit-analytics (2 calls) is deliberately untouched: it has no source
in builtin-extensions/ at all, and backend/app/plugins/serverkit-analytics
is a gitignored materialised copy. Editing it would be a change that
cannot be committed and is overwritten on the next sync — that fix belongs
in the serverkit-analytics repo.
Exit-code semantics are preserved wherever they carry meaning, with a
comment at each site, because they are the thing a mechanical migration
destroys: clamscan 1 = INFECTED, grype 1 = vulnerabilities FOUND, yara
1 = matches, cron_runner 127 = never launched. Those read
result['returncode'], never result['success'].
run_checked gained two things real call sites needed: it yields when the
caller directs the streams itself (writing straight to a log file), rather
than adding capture_output on top and raising ValueError; and text/errors
pass through, so site_importers/ssh keeps decoding with errors='replace'
instead of gaining a UnicodeDecodeError on a bad byte.
A regression this migration introduced, caught by the suite and fixed:
install_profile_service._docker_usable() is deliberately TRI-STATE —
None means the probe could not run, because inside LXC the docker client
is usually present while the daemon never started, and callers "must not
treat a broken probe as proof of absence". Writing it as
run_checked(...)['success'] turned an exec failure into False, i.e. "docker
is not usable" — a fact nobody established. That is the §A violation this
plan exists to remove, reintroduced by the migration meant to prevent it.
test_probe_honesty_doctor caught it. Every other run_checked(...)['success']
caller was then audited; the rest are genuinely two-state, and their
originals also returned False on a missing binary.
Seven more slips of the same shape were caught before running by an AST
check for `x = run_checked(...)` followed by `x.stdout/.stderr/.returncode`
— a dict read as an object, which raises only on the failure path, so
happy-path tests stay green.
api_controller_boundary_baseline.json: pure deletion. Routing api/docker.py
through DockerService removed the last 6 subprocess crossings, so the
`subprocess` category is gone from the API layer entirely.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 75 §G1, second migration. docker_service was the heaviest file in the
census: 49 raw subprocess call sites -> 2. Census 258 -> 211.
DockerService.run now SITS ON run_checked instead of re-implementing it, so
G3's runner and G1's door are one thing rather than two. Adds
DockerService.run_compose() for the compose subcommands, whose argv head is
whatever _get_compose_cmd() resolved (`docker compose` v2 vs `docker-compose`
v1) and therefore cannot go through run().
The 2 survivors are the genuine streaming Popen sites — the live container
log stream and the compose-up transcript. A captured call cannot do those.
run_checked gained two things this migration needed:
- `stderr` (the raw stream) as a separate key from `error` (the verdict, None
on success). git and `docker exec` write to stderr and still succeed, so
folding the stream into the verdict would invent failures. exec_command
needs the stream; every other caller wants the verdict.
- merge_stderr=True, for `docker logs` and `compose logs`. A container writes
its log to both streams; the `result.stdout + result.stderr` concatenation
this replaces loses the interleaving, so a crash tail landed above the lines
that led to it.
TIMEOUTS ARE UNCHANGED, deliberately. run_checked defaults to 60s and every
docker call in this file is unbounded today; inheriting the default would cap
`compose up --build`, `docker pull` and `docker build` at a minute and turn
working deploys into timeouts. Every migrated site keeps exactly what it has,
with timeout=None passed explicitly at the long ones and six tests pinning it
(mutation-checked: removing it from pull_image fails the test). Bounding
docker's long operations is a real improvement and a deliberate behaviour
change — its own commit, not a side effect of this one.
Two regressions caught by the suite and fixed here:
- Dropping the try/except around the compose-version probe let an exception
escape into the caller, surfacing as `compose up` exit_code -1 instead of a
v1 fallback. run_checked converts the exec failures it knows about, but this
probe runs before everything else in the compose path and must not be able
to raise at all. Guard restored, with a test.
- test_docker_protection hand-rolled a CompletedProcess stand-in with only
returncode and stdout. Once list_containers went through run_checked,
reading .stderr off it raised AttributeError, the caller's blanket except
swallowed it, and the test saw an EMPTY CONTAINER LIST rather than an error.
Retargeted onto the §G7 kit. A private stub is only correct until something
reads the attribute it forgot.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 75 §G1 — the door and the ratchet. The 33-file migration is NOT
done; see below.
run_checked(cmd, *, privileged=False, timeout=60, input=None). Around
backend/app's raw subprocess calls sit 220 copies of capture_output/text,
56 hand-rolled TimeoutExpired handlers, 19 FileNotFoundError handlers and
1,164 literal {'success': False, 'error': ...} dicts. Each is a place an
exec failure becomes a false fact.
It keeps the distinction those handlers lose: returncode is None when the
command never ran. Exit 1 means the command answered "no"; no exit code
at all means nobody answered. A caller rendering "not installed" can
finally tell them apart — plan 74's outage in one field. Unprivileged
calls get the same argv[0] resolution as privileged ones, because the
sbin outage is not something that only happens when you ask for root.
The ratchet is tests/raw_subprocess_census.py + RAW_SUBPROCESS_CEILING,
wired into the existing "Audit Raw Subprocess Patterns" CI job and
enforced by test_raw_subprocess_ratchet.py. The grep steps already there
check eight hand-listed files for three known-bad patterns, so a new file
was invisible to the audit entirely; the census counts every call site
under backend/app. It is AST-based because a grep for subprocess.run also
matches comments, docstrings and `except subprocess.SubprocessError`, and
a miscounting ratchet reads green either way. test_the_ceiling_is_not_stale
fails if the ceiling drifts more than 10 above reality — a stale ceiling
silently re-authorises everything a migration removed.
First migration: cron_service, 13 raw calls -> 3. Its `crontab -l` read
and Popen-with-stdin `crontab -` install were written out five times each
and are now two helpers over run_checked. Census 267 -> 258.
The other 32 files are untouched: docker_service (49), database_service
(24), git_service (18), security_service (14), python_service (13) are
the bulk. The door plus the ratchet are what make that incremental
instead of one unreviewable diff.
test_utils_system.py loads system.py under its own importlib spec, so
@patch('app.utils.system.<name>') binds to whichever module object is in
sys.modules at patch time — not necessarily the one run_checked closes
over. shutil/subprocess are singletons so the file's existing patches are
safe; these are the first tests to patch a module-level function of
system.py, and they use patch.object(_module, ...) instead.
BASELINE_COUNT 3383 -> 4081. Most of that is pre-existing drift (the
ratchet only fails on a drop, so an un-bumped floor never complains), not
tests added here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 75 §G5 + §G6.
§G5 — app/services/db_exec.py. engine × host dispatch (mysql/postgres
crossed with native/docker) was written three times. Only
managed_db_user_service delegated fully to DatabaseService's executors;
the other two grew private copies that had already drifted —
db_process_service's docker-postgres branch built its own psql argv and
never set ON_ERROR_STOP=1, so psql exited 0 for a statement the server
had rejected and the caller was told it worked.
db_exec owns the dispatch and nothing else; the four executors stay in
DatabaseService. The three _exec_sql seams remain (tests stub them) and
now delegate. An unrecognised engine is reported, never quietly routed to
mysql, where it would surface as a syntax error blamed on the caller.
docker_mysql_execute/docker_pg_execute gain machine_readable, which is
the single flag db_config_tuner_service was keeping a whole private copy
of the dispatcher to add. The MYSQL_PWD env block, pasted six times in
database_service, becomes _mysql_env(): it carries a real rule — the
password goes in the environment, never on the argv — and a rule in six
copies only holds in the copies someone remembered.
§G6 — adopting doors Round 1 built:
- config_store's write is now atomic (tmp + os.replace). The §F4
consolidation deliberately kept the copies' truncating write and noted
atomicity would land here once; this is that once. A write that dies
mid-dump leaves a truncated file, and every load path treats a corrupt
config as "use the default" — a config silently reset.
- uptime_service._save_history was `except Exception: pass` over that
truncating write, so a full disk destroyed the history and reported
nothing while the panel kept rendering a figure from a file that had
stopped updating. It now returns and logs the failure.
- unit_is_active(): `systemctl is-active` was re-inlined in four services
and every copy collapsed "could not ask" into "not running". The shared
probe is tri-state; None is not False.
- slug: preview_service and saved_view_service re-implemented slugify's
regex, and saved_view re-implemented unique_slug's collision loop too.
unique_slug gains start= so saved_view keeps generating the handles it
always has.
Checked rather than assumed: the plan lists file_integrity_service as
silently losing data on a full disk. It does not — it already used tmp +
os.replace and does not swallow the error. It is the pattern followed
here, not a victim; only uptime_service had the silent loss.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 75 §G2 + §G4, together because they are the same physical files and
G4 is built on G2's helper.
§G2 — write_privileged_file(). The write-a-root-owned-file sequence was
pasted 15+ times in two competing forms: run_privileged(['tee', path],
input=...) and a raw subprocess.run(['sudo', 'tee', ...]). The second
hardcodes sudo instead of asking _needs_sudo(), so it fails on a panel
already running as root in a container with no sudo binary — plan 74's
outage shape. All core-service tee sites now go through one helper that
owns the sudo decision, the result shape, and honest failure: a missing
tee or a timeout reports the failure instead of being swallowed (§A).
§G4 — NginxService.write_vhost()/read_vhost(). environment_domain_service
re-implemented write → symlink → nginx -t → reload with raw sudo calls,
and nginx_advanced_service wrote the vhost with a plain unprivileged
open() — PermissionError against a root-owned sites-available, with no
config test and no reload. Both now go through NginxService.
The rollback is the part no copy had: a vhost that fails nginx -t used to
stay on disk and symlinked, so the next unrelated reload (a cert renewal,
another site going live) failed and the blame landed on whatever
triggered it. write_vhost restores the previous content, or removes a
file that did not exist before, and never reloads a config that failed
the test.
SITES_AVAILABLE/SITES_ENABLED were independently declared in four files,
so an NGINX_CONF_DIR override — the env var NginxService honours, and what
tests redirect — silently did not apply to the other three. One
declaration now.
Mutation-checked: disabling the rollback fails two tests.
Also lands unit_is_active() in app/utils/system.py; its adopters are in
the following commit (§G6).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`return default` gave every caller the same object the caller passed in, and
callers do mutate what they get back (git_service, build_service and
monitoring_service all assign into it). No caller is actually bitten today --
all six construct a fresh literal at the call site -- so this hardens the
shared helper rather than repairing a live leak. That matters because
config_store is the one door the remaining hand-rolled copies are meant to
converge on, and the next adopter is the one that would pass a module-level
default.
Deep, not shallow: several defaults nest mutable dicts (backup_service's
notifications, security_service's scheduled_scan) that a shallow copy would
still alias. The test mutates through both a top-level list and a nested dict
for that reason -- it fails against `return default` AND against
`copy.copy(default)`.
Reported by Copilot on the plan-75 PR.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The result translation was re-implemented per service with subtly
divergent failure shapes — stderr under "error" in postfix, under
"message" in php and nginx. ServiceControl.result_dict owns it, with
the key and fallback parametrized so converting a site is parity, not
a contract change. One deliberate improvement: an empty stderr now
yields a real fallback message instead of a None "message".
The audit corrected the plan's premise along the way: no dovecot/mail
×5 wrapper family exists; the genuine pass-throughs were
postfix.restart and the php fpm pair. nginx keeps its config test,
fail2ban its fallback, postfix.reload its own mechanism — genuine
per-service logic stays.
Plan 75 §A2. Four dishonest shapes across doctor/drift/fleet, all
converting a failed probe into a determined-looking answer:
- except: return []/None/False whose empty result is indistinguishable
from a determined negative: a broken Domain query rendered green
'No public site domains to check'; a whole doctor section (setup,
extensions) vanished silently on failure; an uninspectable app was
dropped from the drift sweep, reading as 'in sync' by omission.
- Resolver errors impersonating NXDOMAIN: any getaddrinfo failure
reported 'does not resolve', offered repair, and paged admins about
domains 'newly stopped resolving' during what was a panel-side DNS
outage. Only EAI_NONAME/EAI_NODATA is an unresolved domain now.
- Unknown -> False -> 'Not running': a missing systemctl (container,
non-systemd init) read as a repairable fail; a failed docker probe
read as a green 'Not installed — Minimal profile'; a fleet agent
that returned no state for a unit persisted fail 'Not running.'
- Fleet default-to-ok: missing status persisted and counted as ok
(STATUS_ERROR existed and was never emitted); a failed/timed-out
sweep left the server's last all-ok rows standing as current health
— a synthetic error row now records that this run couldn't check.
Two existing tests changed: one pinned the dishonest behavior itself
(is_active False on missing systemctl), one faked NXDOMAIN with a
single-arg gaierror (errno=None) — fixture corrected to the real
two-arg form, expectation unchanged.
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.
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 §F1. A duplication audit over 4103 functions found 73 structurally
identical groups; the sharpest instance was self-inflicted.
capacity_service.format_size renders bytes for sentences ('needs about 512 MB'):
'unknown' for None, adaptive precision. host_inventory_service carried a
byte-for-byte copy of it, added in the plan-74 round, written to avoid an import
cycle that cannot exist — app/utils/formatting imports nothing and is already
used by 9 modules.
Moved it to that leaf module. capacity_service re-exports the name so existing
callers keep working; host_inventory_service imports it.
The audit's headline was misleading and worth recording: of the seven other
_format_size methods, six are one-line adapters to the canonical format_bytes
and system_service.get_size is another. Only models/db_snapshot.py holds a real
hand-rolled reimplementation. "Four competing implementations" was a tool
artifact; reading the code gives one hand-rolled copy, one prose variant, and
one duplicate of that variant.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ufw fix repaired every run_privileged caller centrally, but three sites
reached subprocess by other routes and were broken the same way. All are
"$PATH omits sbin + the panel runs as root, so nothing prepends sudo to
resolve it".
- run_command() got the same argv[0] resolution as privileged_cmd. It does no
privilege escalation, so nothing else would ever find these commands.
- nginx_advanced_service.test_config() ran `nginx -t` (/usr/sbin/nginx) via
run_command, so it raised FileNotFoundError and reported every config
invalid. It also returned valid=True unconditionally, ignoring the exit code
entirely — so when it DID run, a broken config passed. Now run_privileged and
the returncode is checked. reload_nginx() likewise reports a failed reload
instead of swallowing it.
- dovecot_service.get_status() detected dovecot with `which`, which searches
$PATH only and false-negatives on /usr/sbin/dovecot, then read the version by
bare name under a shared except. Now is_command_available() + a separate try,
so a version read that fails cannot unsay "installed". Patched in both the
builtin-extensions source and the app/plugins live copy.
- SecurityService.get_lynis_status() inferred presence solely from exec'ing
`lynis --version`; presence and version are now separate questions.
Adds test_sbin_command_guard.py so this cannot come back: an AST scan fails CI
on any raw subprocess call using a known sbin-resident command by bare name
(it caught the dovecot site), plus per-command proof that both helpers resolve
under the exact PATH the panel unit ships.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UFW installed cleanly, dpkg said ii, the install notification said so — and
/security/firewall still rendered "No Firewall Installed". fail2ban worked on
the same box, which is the clue: fail2ban-client lives in /usr/bin, ufw in
/usr/sbin, and the panel unit ships
PATH=<venv>/bin:/usr/local/bin:/usr/bin:/bin
with no sbin. subprocess resolves argv[0] through $PATH alone, so `ufw status`
raised FileNotFoundError. _check_ufw wrapped both questions in one try/except,
so that exception discarded an `installed` already computed as True and
returned False. Measured on the affected host:
is_command_available('ufw') = True
PackageManager.is_installed('ufw') = True
get_status() -> ufw {'installed': False}, any_installed False
FirewallTab.jsx gates its empty state on any_installed. The Enable button was
broken identically — same bare-name exec.
Three fixes, each independent:
- resolve_command() gives argv[0] an absolute path, but ONLY when $PATH cannot
resolve it, so working commands pass through byte-identical.
- probe failure can no longer unsay "installed" (same split for firewalld).
- the unit template gains /usr/local/sbin, /usr/sbin and /sbin. update.sh
re-renders the unit, so existing installs pick this up on upgrade.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 73 item 1. SSLService hardcoded /usr/bin/certbot, so the install method
certbot itself recommends -- snap, which lands in /snap/bin -- produced a
FileNotFoundError the panel surfaced as an opaque failure. is_command_available
had the same blind spot: it probed /usr/bin, /usr/sbin and the two local dirs
but never /snap/bin, which is routinely absent from a service's $PATH.
resolve_certbot_bin() now tries the CERTBOT_BIN override, then shutil.which,
then the explicit candidate list including the snap path. Positive results are
cached (an install location does not move mid-process) but negative ones are
not, so installing certbot is picked up without a panel restart;
install_certbot clears the cache outright. CERTBOT_BIN as a class attribute is
gone -- it captured the value at import time, which is exactly why an override
set later never took effect -- replaced by certbot_bin() plus CERTBOT_FALLBACK
so a command built on a host with no certbot still fails cleanly.
The second half is the failure the reviewer actually hit on stream: with
certbot present but its nginx plugin missing, `certbot --nginx` dies deep
inside certbot with "the nginx plugin is not working / binary doesn't exist",
and we passed that through raw. obtain_certificate now preflights -- nginx on
the box, then `certbot plugins` for an enabled `* nginx` -- and returns an
actionable error naming python3-certbot-nginx before certbot is ever invoked
for issuance. A failing plugins probe falls back to the distro package check
rather than blocking issuance on a host where the plugin is clearly present.
AdvancedSSLService's wildcard and multi-domain commands go through the same
resolution instead of the bare name.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An authenticated user of any role could inject arguments into git
subprocesses running as the backend user (root under systemd):
- repo_url was passed unvalidated to git ls-remote/clone, enabling the
ext:: transport (host command execution on git < 2.52 defaults) and
file:// local-repository disclosure (all git versions).
- app_path on POST /deploy/clone was passed without a '--' terminator,
enabling --upload-pack=<cmd> option injection independent of git
version and protocol.*.allow policy.
- branch flowed into 'git fetch origin <branch>' as an unterminated
refspec.
Fix (new app/utils/git_security.py applied at every affected sink):
- repo_url scheme allowlist (https/http/git/ssh + scp-like SSH);
rejects ext::/file://, local paths, leading '-', whitespace.
- '-c protocol.ext.allow=never -c protocol.file.allow=never' pinned on
every remote-touching git call, plus a restrictive GIT_ALLOW_PROTOCOL
in the subprocess environment.
- '--' positional terminator before URL/path/refspec arguments —
co-load-bearing with protocol pinning, which alone does not stop
option injection such as --upload-pack=<cmd>.
- branch/ref validation (leading '-', ref-format metacharacters) for
clone --branch, fetch refspecs and stored deploy configs.
- POST /deploy/clone now confines app_path to the managed apps root,
mirroring how /apps/from-repository derives it server-side.
Adds 31 regression tests; bumps version to 1.7.86.
Reported-by: CaptBoykin
Co-authored-by: Tyler Boykin <25166954+CaptBoykin@users.noreply.github.com>
Runtime-ESM extensions (starting with serverkit-wordpress) need slot
mounting and error containment from the host; additive, versioned in
lockstep frontend/backend.
`contributions.widgets` mounts a component at a host-chosen slot. That is the
wrong shape for the new dashboard, where the user decides what goes on a board
and where. So this adds `dashboard_widgets` alongside it: a registry of
placeable widget TYPES that surface in the widget library, each instance
positioned, sized and configured by the person looking at it.
The two are easy to confuse, so the docstring, the manifest spec and
EXTENSIONS.md all now say plainly which is which, and a test asserts a
dashboard widget never leaks into the fixed-slot list.
SDK_VERSION 1.0.0 -> 1.1.0 (added surface, older extensions unaffected),
bumped in lock-step across the JS export and the backend mirror.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
210 run_privileged() call sites passed no timeout. With output captured
and no terminal attached, a command that never returns takes its caller
with it — silently and forever. That is exactly how the metadata guard's
iptables probe wedged backend startup; the sudo -n fix removed that one
trigger, but the class of bug was still one hung command away.
run_privileged now applies a default timeout when the caller does not
name one, so the fix lands on all 210 sites at once rather than being
reapplied by hand and forgotten on the 211th. TimeoutExpired is a far
better outcome than a wedged worker, and callers that genuinely need
longer already pass their own; `timeout=None` opts out deliberately.
The four direct read-only probes in this module (dpkg -s, rpm -q,
systemctl is-active/is-enabled) bypass that helper, so they get their own
much tighter ceiling — they answer immediately or not at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
privileged_cmd prepended a bare `sudo`. Nothing here runs attached to a
terminal and run_privileged captures output, so on a non-root host a
password prompt is both invisible and unanswerable — sudo blocks forever
and takes its caller with it. This hung backend startup outright: the
metadata guard's `iptables -C DOCKER-USER` probe sat unanswered and the
server never bound its port.
Emit `sudo -n` so the call fails immediately with a non-zero exit, which
every caller already handles. Production runs as root (_needs_sudo() is
false) which is why this stayed hidden, but any non-root install without
passwordless sudo would hang on boot.
An explicitly-sudo command passed in by a caller is still forwarded
untouched; the regression test pins non-interactive sudo across all four
invocation shapes.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rate-limit buckets, login lockout and audit-log source IPs hand-parsed the
leftmost X-Forwarded-For token — a client-controlled value — letting an
attacker rotate it to dodge per-IP limits or forge audit trails.
- config: add TRUST_PROXY_HEADERS / TRUSTED_PROXY_HOPS (+ _env_bool/_env_int)
- create_app: wrap in gated Werkzeug ProxyFix (rightmost trusted hops) before
the limiter/handlers so every remote_addr consumer sees the real client
- new app/utils/client_ip.py::get_client_ip() — the single trusted seam
- replace all six leftmost-XFF parsers (rate_limit, audit middleware,
api_key_auth, api_analytics, audit_service, ddns) with get_client_ip()
- deploy: root .env.example ships TRUST_PROXY_HEADERS=true (behind nginx);
backend/.env.example documents it off for dev
- tests: test_trusted_client_ip.py (7) incl. real flask-limiter bucket isolation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
/system/version now returns install_dir (new get_install_dir() in the
shared version resolver: SERVERKIT_INSTALL_DIR override -> own tree ->
/opt/serverkit default; abspath not realpath so blue/green boxes report
the stable symlink). The File Manager re-points its 'Stack' quick-access
shortcut from the response, so custom-directory installs jump to the
real tree; the static default remains the fallback while loading or on
older backends. 2 new resolver tests + install_dir asserted in the
endpoint test.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The About page's /system/version (and check-update's current-version
read) hardcoded /opt/serverkit first, duplicated in three places:
- app/utils/version.py is now the single resolver: explicit
SERVERKIT_INSTALL_DIR override -> the running code's own tree ->
/opt/serverkit last (a box moved to a custom dir may still have a
stale tree there); system.py's two inline copies now call it
- the systemd unit template renders SERVERKIT_INSTALL_DIR from the
installer's SERVERKIT_DIR (deliberately not the bare SERVERKIT_DIR
env name, which app/paths.py already uses for the /var/serverkit
data root); flows to existing installs via the updater's re-render
- crypto's .env discovery gains the same install-dir hint
- Docker image now ships VERSION (/app/VERSION) — containers showed
the 1.0.0 fallback because no candidate path existed in the image
- 4 proving tests in backend/tests/test_panel_version.py
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Both reverse proxies bind 80/443, so pointing the same domain at host Nginx and
a Dockerized proxy stack is an operational footgun. This tags each app with the
plane it expects and surfaces disagreements instead of leaving them implicit.
- Each app carries an `ingress_plane` ('nginx' | 'proxy_stack'); NULL reads as
the host-Nginx default. New pure helpers in app/utils/ingress.py: only
container-based services are proxy-stack eligible, and a requested proxy_stack
is forced back to nginx for PHP/WordPress/static/Python apps. to_dict() also
exposes `ingress_proxy_eligible`.
- ProxyStackService.ingress_audit(server) reports which of a server's apps
disagree with its active proxy mode; fleet_overview now includes per-server
app_count + mismatch_count. New GET /servers/<id>/proxy/ingress-audit.
- Create wizard offers the proxy stack only for Docker/Compose services and
keeps the default on Nginx; app detail shows an Ingress badge; the fleet proxy
dashboard flags mismatch counts; the server proxy panel warns and lists the
specific mismatched apps with reasons.
Migration 035 adds applications.ingress_plane. Backend suite green (incl. 8 new
ingress tests); frontend builds clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Juan Denis