Plan 76 milestone A, the third row. 23 handlers decided admin access inline,
all in two files, in two shapes.
18 were the mid-route gate this milestone already built a door for:
current_user_id = get_jwt_identity()
user = User.query.get(current_user_id)
if user.role != 'admin':
return jsonify({'error': 'Admin access required'}), 403
which is four lines to say require_admin_user(), and carried the two-line
identity lookup with it. Migrating them closes both rows at once: the identity
census drops 87 -> 69 and the controller-boundary baseline loses 18 persistence
crossings (528 -> 510), with no new entries.
It also fixes a latent 500: where the check omitted the `not user or` guard, a
token naming a deleted user hit `None.role` instead of answering 403.
The other 5 are `not user.is_admin and app.user_id != user.id` — admin OR
owner, which is a different question. Those keep their shape and only converge
the truth source from `role != 'admin'` to `is_admin`; routing them through
can_access_app would silently widen access, since that also honours resource
grants.
The migration initially left `current_user_id` referenced but unbound in four
places. Two were caught by tests; the other two (create_manual_app,
upload_app_archive) have no coverage and were caught by an AST scan for names
used-but-never-assigned. Worth remembering that a mechanical removal needs the
scan, not just the suite.
Gate: -k "system or apps or app_ or host_snapshot or manual_app or upload",
304 passed / 13 skipped; all seven ratchets green (52 passed).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a7d5a0b9 replaced hand-shaped 500s in docker.py and notifications.py with
unexpected_response(), which rolls the session back itself, so those two routes
no longer call db.session.rollback() directly. Controller-boundary persistence
crossings 519 -> 517; no new entries.
Caught by the full per-file sweep rather than the focused gate, which did not
include this ratchet.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone C, drift/bug row 3 — the ratchet only. 23 sites choose an
HTTP status by searching an error message:
status = 403 if 'denied' in result.get('error', '').lower() else 400
That is what `{'success': False, 'error': '<prose>'}` forces on a caller: the
service knew the failure was a denial, discarded it, and the route rebuilds it
from the sentence. The status is then coupled to the wording — rephrasing
"Access denied: path not in allowed directories" to "You do not have access to
this path" turns a 403 into a 400 and nothing fails, not a test, not a lint
rule.
The migration is NOT done here on purpose. Converging these means services
raising typed errors instead of returning result dicts, and file_service alone
(12 of the 23 sites) has eight callers. Converting it to raise while its other
callers still expect a dict leaves two contracts, which is the disease rather
than the cure — that is milestone C's envelope decision, sequenced after A and
B by the plan's own execution order.
So the ratchet lands first, per second-wave rule 1, to stop the pattern
spreading in the meantime. Ceiling 23. Counts a conditional choosing between
two HTTP status codes whose test inspects string content (`in`, `.lower()`,
`.startswith()`); a status chosen from a typed exception, an enum, or a boolean
is not counted, since that is the thing being asked for. Verified non-vacuous:
an injected site fails it and it passes again on revert.
Gate: tests/test_status_sniffing_ratchet.py, 8 passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone B, drift/bug row 4. 43 API handlers answered HTTP 500 from
their own `except Exception`, so the global 500 handler never ran — and that
handler was the ONLY caller of error_log_service.record_error(source='backend').
Every one of those crashes was absent from /monitoring/errors, and the page read
empty precisely because someone had "handled" them.
35 of the 43 also returned str(exc) to the caller. Two returned the full
traceback (templates.py, which also print()ed it), and several logged with a
locally-minted uuid4 'ref' that correlated with nothing but a log line.
The recording side of the 500 handler moves to app/error_reporting.py, so it is
importable rather than reachable only by not-catching:
record_unexpected(exc) -> logs, rolls the session back, records, returns
the request id
unexpected_response(exc) -> the above plus the documented crash body
The 500 handler now calls the same pair, so there is one recorder rather than
one recorder and 43 routes that quietly opted out of it.
Caller-visible change on those endpoints: the body is now the documented
{'error': 'Internal server error', 'status': 500, 'code': 'internal_error',
'request_id': ...} instead of the exception text. That is the point — an
unexpected exception's text is written for whoever reads the log and routinely
carries connection strings, absolute paths, and SQL. The request id is the part
a caller can act on, and it matches the row in the error log. No frontend code
read the 'ref' or 'trace' keys this drops (checked).
Two sites keep their own wording and call record_unexpected instead: sso.py's
login failures should not read as a generic crash on the login page.
docker.py's bulk endpoint keeps its per-item `results` — the caller's half-done
work survives, only the exception text is dropped.
Ratchet: tests/unreported_crash_census.py + its test, held at **0**, not a
legacy baseline to shrink. It counts blanket handlers that answer 500 without
calling either reporter, and is scoped to app/api — a service catching an
exception and returning a value is making a domain decision, not declaring an
HTTP crash. Verified non-vacuous: an injected handler fails it, and it passes
again on revert.
Measured and deliberately still open: 56 API handlers swallow a crash and
answer 200. A failure reported as success is worse, but converging it is
milestone C's response-envelope work, not this row.
Gate: tests/test_unexpected_error_reporting.py (11, including the two that pin
why the old shape lost the crash), test_unreported_crash_ratchet.py (9), and
the 66 test files covering every touched module: 1105 passed, 7 skipped, 1
pre-existing Windows-only failure (test_fleet_doctor's socket.gaierror).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fe02ce8e removed queue_bus.py's private _current_user(), which was the only
code using the model. The automatic dead-import sweep kept it because the word
"User" also appears in a docstring two lines into _ensure_group_accessible.
Gate: -k queue_bus, 26 passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone A, drift/bug row 2 — scheduled as a fix, not a refactor,
because it is a behaviour difference shipping today.
rbac.get_current_user() is the only lookup that knows about both auth
mechanisms: it returns g.api_key_user for an X-API-Key request and falls back
to the JWT. 73 call sites answered "who is calling" with
`User.query.get(get_jwt_identity())` instead, behind 19 near-identical private
helpers (12 named get_current_user(), shadowing the real door).
The audit that scheduled this row predicted those return None for API-key
callers. Measured, it is worse: auth_required() deliberately skips
verify_jwt_in_request() once the key middleware has authenticated, so there is
no JWT context and get_jwt_identity() *raises*. Nine handlers reached through
an API-key-capable policy decorator therefore returned 500 to every API-key
caller:
bandwidth.get_app_bandwidth @viewer_required
servers.set_server_workspace @developer_required
servers.create_server @developer_required
sso.get_sso_config / update_provider_config / update_general_settings
source_connections.update_{github,gitlab,bitbucket}_admin_config
source_connections.complete_github_app_manifest
databases.execute_{mysql,pg,sqlite,docker}_query (see below)
Three service helpers had the same lookup inside a blanket `except Exception`,
which converted the raise into a silent user_id=None — the write landed and
the audit trail lost the actor (shared_resource_service, cf_ops_change_service,
secrets_webhooks).
The four databases.execute_*_query sites were a second admin check *inside* an
already-@admin_required route: dead code that always passed for JWT callers and
crashed for API-key ones. They now call require_admin_user() rather than
re-deriving the answer.
Also folds in a sixth _require_admin() found in the cloudflare-ops extension
source (29 gates, the same inverted contract as jobs.py), and fixes
serverkit-gui's blueprint at its sibling-repo source rather than in this repo's
live copy.
Ratchet (second-wave rule 1 — a migration without one regrows):
tests/identity_door_census.py counts both bypass shapes by AST, direct and the
two-line `uid = get_jwt_identity(); User.query.get(uid)` variant. The ceiling is
87, all of them on @jwt_required()-only routes where reading the JWT is correct
today; the ratchet's third assertion holds the API-key-capable population at
exactly zero, so migrating one of those routes to a policy decorator without
fixing its lookup now fails. Verified non-vacuous: an injected bypass fails both
assertions and passes again on revert.
The first wave's controller-boundary baseline is regenerated — this removes 74
persistence crossings (593 -> 519), a debt reduction with no new entries.
Gate: tests/test_api_key_identity.py (7, each red before the fix),
test_identity_door_ratchet.py (10), test_api_controller_boundaries.py (4), plus
the 65 test files covering every touched module: 1024 passed, 10 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone A, drift/bug row 3. Five API modules had grown a private
_require_admin() with *three mutually incompatible* return contracts:
jobs.py -> returns the user; None means DENIED
dns_cutover.py -> returns None on success; a response tuple means denied
waf.py -> same as dns_cutover
manifests.py -> (user, None) on success, (None, response) on denial
themes.py -> same as manifests
jobs.py's is the exact inverse of dns_cutover.py's, so a guard copied between
the two reads identically at the call site and fails open. Nothing in the code
told you which contract you were looking at.
Replace all five with rbac.require_admin_user(), which raises a typed
PermissionDeniedError the global ApplicationError handler already renders. A
gate that raises has no return contract to get backwards.
Two convergences ride along, both toward the stricter shared behaviour:
- manifests.py gated on `user.role != 'admin'` where the others used
`is_admin`. These are the same predicate today (is_admin is defined as
role == ROLE_ADMIN), so no route changes status, but there is now one
truth source rather than two.
- none of the five checked is_active, so a deactivated admin still passed
them; @admin_required has always checked it. The shared door checks it, so
these 25 call sites now reject deactivated admins like every other
admin route.
The unknown-user case keeps its 403 'Admin access required' rather than
becoming a 401/404: a non-admin caller should not learn from the status code
whether the account behind the token still exists.
Gate: tests/test_jobs.py, test_themes.py, test_waf.py, test_dns_cutover_api.py
(82 passed) and -k manifest (152 passed).
Co-Authored-By: Claude Opus 5 (1M context) <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 §G3. Three services kept private `_docker()` wrappers around the
same `subprocess.run(['docker', ...])` call, and the three had drifted
into three different return contracts:
- db_admin_sso_service returned a dict and mapped FileNotFoundError to
"Docker not found";
- db_config_tuner_service returned a dict that collapsed every failure
into str(e), so a caller could not tell "install docker" from "the
daemon is wedged";
- image_update_service returned a raw CompletedProcess, so a caller
reading result['success'] against it got a TypeError, not an answer.
DockerService.run(args, timeout=60) is now that call, with one shape —
{'success', 'output', 'error'} — that keeps the three distinctions §A
cares about apart: docker said no, docker is absent, docker did not answer
in time. DockerService.available() asks the daemon rather than the binary,
because "CLI installed, daemon down" is the state the callers check for.
The three named seams stay (each service's tests stub them); only the
bodies go. image_update_service's two digest probes move to the dict
shape, and drop the blanket try/except that was there to absorb the
CompletedProcess path's exceptions.
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>
Plan 75 §G7. The suite's hand-rolled stubs were entrenching the very
duplication the plan removes: _FakeProc pasted byte-identical in four
modules, ~25 fake_run closures re-implementing argv dispatch, and zero
tests patching the wrappers — every stub targeted raw subprocess inside
one service module, so each new service test recreated the raw seam.
tests/subprocess_stub.py is that seam. Tests script argv → result; when
G1 moves services onto run_checked() the retarget happens here once
instead of in twelve files.
Two properties are deliberate:
- An unscripted command raises instead of inventing returncode=0. It is a
BaseException for the same reason SbinPathError is — services wrap
subprocess in `except Exception`, so an AssertionError would be eaten
by exactly the handlers the kit exists to test.
- Stubbed execs are still sbin-guarded. Patching subprocess removes the
§B2 runtime guard, so the kit re-applies it; popen_guard grows a public
guard_argv() as the one door for that check.
Matching normalises away a sudo prefix and argv[0] absolutisation, so one
scripted rule matches on a root box, a sudo box and a dev box alike.
Mutation-checked: removing the guard call fails the guard test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
reportClientError sent no Authorization header, so every source='frontend' row
was anonymous no matter who was signed in -- which made ErrorLog.user_id,
to_dict()['username'] and the "User #N" row in pages/Errors.jsx dead end to
end, and the route's own docstring ("a valid JWT attaches user_id") false.
Attaching the header by hand does not violate the deliberate design here: the
comment forbids this.request() -- its 401 handling, refresh loop and throwing
-- not a header. The raw fetch and the swallow-everything contract are intact,
and Content-Type already made this a non-simple CORS request, so no new
preflight is added.
The review's stated rationale was wrong and the corrected version is in the
comment: verify_jwt_in_request(optional=True) tolerates a MISSING token only.
In Flask-JWT-Extended 4.6.0 it catches NoAuthorizationError alone, so an
expired token raises ExpiredSignatureError and garbage raises DecodeError. A
stale token is harmless here only because the route wraps that call in
try/except and falls back to an anonymous report -- tightening that except
would turn every expired session's crash report into a 500.
Three backend tests pin the contract the header relies on: valid token
attributes the row, no token stays anonymous, junk token stays anonymous
rather than 500ing. The frontend side that sends it is unproven -- the panel
has no test runner for pages.
Known ceiling: dedup bumps count on an existing unresolved row without
backfilling user_id, so attribution is first-writer-wins per fingerprint.
Reported by Copilot on the error-tracker PR.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The 500 handler shares the app-context-scoped db.session with the view that
just failed, which breaks two ways.
The one that matters: when the crash IS a database error, that session is
already in a failed transaction, so record_error's very first query raises
PendingRollbackError -- swallowed by its own never-raise contract. Database-
caused 500s were therefore the one class of crash the error tracker could never
show, and they are the class most worth seeing. 72 API views commit with no
try/except anywhere in the function, so this is not a narrow path.
The second: record_error's commit() flushes whatever ORM state the crashed view
left pending, persisting a half-applied write on the way out.
Rolling back first fixes both. Flask-SQLAlchemy's teardown rolls this session
back moments later anyway, so a clean request is unaffected.
Reviewed as an alternative to a partial unique index on the dedup fingerprint,
which was rejected: record_error swallows everything and returns None, so an
IntegrityError would DROP the error report rather than dedupe it; set_resolved
commits unguarded, so un-resolving an old row sharing a fingerprint would 500 a
legitimate admin action; and CREATE UNIQUE INDEX would abort the migration on
exactly the installs that already accumulated duplicates. The race is real but
its blast radius is duplicate rows in an admin list -- nothing reads
fingerprint for uniqueness, and the lookup is .first(), not .one().
Both new tests fail with the rollback removed (mutation-checked by line
number). The first draft of the DB-error test did NOT: a hand-built
IntegrityError never poisons the session, so it has to be a real failed flush.
Race reported as High by Copilot on the error-tracker PR.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ServiceControl.is_active raises FileNotFoundError when systemctl is absent --
deliberately, so a host without systemd reads as "could not check" rather than
"not running". DovecotService.get_status called it unguarded, so
GET /api/v1/email/status returned a 500 on every such host. Email.jsx catches
that and renders "Mail server not installed" with an Install button, on a box
already running Postfix and Dovecot.
Guarding only the is_active line -- the literal review comment -- leaves the
second escape route open one statement later: both wrappers carry
timeout=PROBE_TIMEOUT, so a systemd that stops answering raises TimeoutExpired
straight through is_enabled, which swallows only the missing binary. Hence
(subprocess.SubprocessError, FileNotFoundError), covering both probes -- the
same pair dkim, spamassassin, ftp and postfix already catch.
running/enabled report None on an unreadable probe. Fabricating False here is
exactly the dishonesty this round removed from the firewall and SSL services.
email_service keeps its top-level `running` boolean with bool(), since
`True and None` is None and that field is documented as a boolean; the honest
tri-state stays in the per-component dicts.
Mutation-checked by line number: the narrow `except FileNotFoundError` fails 2
of the 4 new tests, a guard catching neither fails 3, and the healthy path
passes throughout.
Reported by Copilot on the plan-75 PR (is_active only).
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>
Matches the registry copy. The bundle is the offline floor, so leaving the
placeholder glyph here would show a different icon to any panel reading it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
AgentSite now ships a declarative serverkit.yaml, so live inspection of that
repo returns strategy=serverkit. The template's declared hints — the labeled
fallback for when the public repo cannot be reached — still claimed
docker_compose and did not list the file at all, so the fallback described a
different deployment than the real one.
The six provider keys were also all marked required. build_template_hints
passes `required` straight through to the wizard, which stars each one, so the
install screen told people they needed six API keys. AgentSite is
model-agnostic and needs one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The integration module loaded firewall_service via importlib and
registered it in sys.modules under the real name — at collection time,
before any test ran. In a full-suite run, every @patch targeting
app.services.firewall_service then landed on the shim copy while the
firewall test modules held the real one: mocks silently not firing,
32 firewall tests red only in full-suite context, green in every
subset. In the full suite (Flask installed) import the real modules;
the container legs keep the shim, now under a non-colliding name.
Two assertions in TestProbeFailureIsolation pinned exactly what 2f002c59
fixed: "genuinely unknown, reported as off". Unknown is None + error
now; the determined-inactive case gains the error: None key.
The result translation was re-implemented per service with subtly
divergent failure shapes — stderr under "error" in postfix, under
"message" in php and nginx. ServiceControl.result_dict owns it, with
the key and fallback parametrized so converting a site is parity, not
a contract change. One deliberate improvement: an empty stderr now
yields a real fallback message instead of a None "message".
The audit corrected the plan's premise along the way: no dovecot/mail
×5 wrapper family exists; the genuine pass-throughs were
postfix.restart and the php fpm pair. nginx keeps its config test,
fail2ban its fallback, postfix.reload its own mechanism — genuine
per-service logic stays.
The guard fired on real-exec integration suites (the §D matrix): under
an sbin-ful PATH, run_privileged passes a bare name through — correct
behavior, since the same call absolutizes under the unit's sbin-less
PATH. The guard's model ("dies under the unit") cannot see that
resolution from inside a dev PATH. It now stays silent when the exec
came through app/utils/system.py — the one door it exists to route
traffic through — and still fires on a raw call one frame away. The
§D integration module's opt-out is removed; the guard stays active
there, and the capdrop matrix leg passes with it on.
The §D matrix pinned this live: in a capability-restricted environment
(unprivileged LXC — ufw exists, is on PATH, and fails on dropped
CAP_NET_ADMIN) ufw status exits non-zero, and _check_ufw returned
active=False with no error signal — "probe failed" indistinguishable
from "firewall off", the same dishonesty class as plan 74's "not
installed", one rung down the ladder.
active/running are now tri-state: rc=0 parses a determined answer;
non-zero with a dbus/not-running signature is a determined
not-running (verified on Rocky 9: rc=36); anything else is None +
error. Not-installed stays a determined False. any_active propagates
the None, and the firewall tab renders an amber Status Unknown
instead of collapsing it to Inactive.
Plan 75 §A5. The frontend could always render unknown (a real 'unknown'
state, an n/a chip) — the backend starved it of the signal:
- Doctor's cert check read Domain.ssl_expires_at, a column nothing in
production writes — structurally incapable of anything but a green
'No certificates tracked.' Empty now warns: expiry is not monitored.
- openssl -checkend exits non-zero for 'expires soon' AND for
'unreadable' — an unreadable cert reported needs_renewal=True, and
the error path vanished the cert from the expiring list entirely.
Unreadable now returns an error result, and the status endpoint
surfaces check_errors instead of dropping them.
- A failed monitor cert probe bumped cert_checked_at while keeping the
old cert_expires_at — a green 'valid / N days' chip from weeks-old
data next to a fresh checked timestamp. The stale value is cleared;
the UI falls back to n/a until a probe succeeds.
- get_expiry_alerts dropped unprobeable certs by omission; they now
appear with severity 'unknown'. get_cert_health reported grade 'F'
for a DNS timeout; probe failure now yields grade None + error.
- A failed certbot inventory reported '0 certificates, nothing
expiring' with HTTP 200; list_certificates_report carries the
errors and the status endpoint exposes them as list_errors.
Plan 75 §A2. Four dishonest shapes across doctor/drift/fleet, all
converting a failed probe into a determined-looking answer:
- except: return []/None/False whose empty result is indistinguishable
from a determined negative: a broken Domain query rendered green
'No public site domains to check'; a whole doctor section (setup,
extensions) vanished silently on failure; an uninspectable app was
dropped from the drift sweep, reading as 'in sync' by omission.
- Resolver errors impersonating NXDOMAIN: any getaddrinfo failure
reported 'does not resolve', offered repair, and paged admins about
domains 'newly stopped resolving' during what was a panel-side DNS
outage. Only EAI_NONAME/EAI_NODATA is an unresolved domain now.
- Unknown -> False -> 'Not running': a missing systemctl (container,
non-systemd init) read as a repairable fail; a failed docker probe
read as a green 'Not installed — Minimal profile'; a fleet agent
that returned no state for a unit persisted fail 'Not running.'
- Fleet default-to-ok: missing status persisted and counted as ok
(STATUS_ERROR existed and was never emitted); a failed/timed-out
sweep left the server's last all-ok rows standing as current health
— a synthetic error row now records that this run couldn't check.
Two existing tests changed: one pinned the dishonest behavior itself
(is_active False on missing systemctl), one faked NXDOMAIN with a
single-arg gaierror (errno=None) — fixture corrected to the real
two-arg form, expectation unchanged.
Two changes in one file (kept together to avoid hunk surgery):
- probe honesty (plan 75 §A3): os.getloadavg failure produced
(0, 0, 0) — a fake zero load that also silenced the load threshold
check. Now (None, None, None) and the threshold check is skipped
when undetermined.
- consolidation (plan 75 §F4): get/save_config join the ConfigStore
conversion (a3fe9bf5), the last of the five byte-identical copies.
Two dishonesty directions, one rule (plan 75 §A3/A4): a probe answers a
value or None, never 0 when undetermined — and a real 0 must survive.
Live paths normalized every missing reading to 0 while the history path
stored NULL for the same probe: agent counters an LXC agent can't
determine rendered as a perfectly idle box; load average on unsupported
platforms read 0.00 (and silenced the load alert); CPU freq read 0 MHz;
an empty history window summarized as '0.0 avg'. The mobile summary
called a method that does not exist and rendered 0/0/0 permanently.
One unavailable network counter raised uncaught and froze the entire
live broadcast.
Reverse direction: falsy traps (x if x else None) collapsed a genuinely
idle 0.0 into 'no data' in rollups, averages, and the history model.
Fleet: KPI tiles read '0.0 ms latency' / '100% success' with no data;
never-probed servers exported serverkit_server_up 0 to external
alertmanagers — the up-series now emits only for determined
connectivity. get_all_metrics is per-section fault-tolerant so one
failed probe can't blank the other six.
Five services shipped byte-identical save_config copies and as many the
same get_config skeleton — the next fix (an atomic write, a better
error) would have had to land in every one. load_json_config /
save_json_config own the mechanics; the path, the default, and any
behaviour around the write stay per-service.
Byte-compatible with the copies: plain open(..., 'w'), no locking, no
atomic rename — adding those here would be a behaviour change smuggled
in with a consolidation; if they're ever wanted they now land once.
Divergent services (template cache invalidation, storage secret
merging, email_relay's DB backing, build_service's leaner return dict)
keep their own methods. monitoring_service converts separately — it is
mid-edit on another branch of this plan.
The static sbin guard reads literal argv lists — 164 of 288 raw
subprocess sites (57%). The other 43% (variables, f-strings,
concatenation) is where plan 74's outage class can still hide: a call
that works in dev (PATH has sbin) and dies under the panel unit (PATH
has none).
An autouse conftest fixture now routes every subprocess.Popen the suite
makes through GuardedPopen, which sees the actual argv at call time and
refuses a bare name that resolves under the current PATH but not under
one with the sbin dirs removed. A binary simply absent on the test
machine still raises the honest FileNotFoundError — the guard judges
PATH shape, never machine contents. The refusal is a BaseException:
this bug class ships because blanket except handlers convert exec
failures into false facts, so an AssertionError would be swallowed by
exactly the handlers it hunts.
Coverage is 'whatever the suite exercises' — honest, measurable, and
growing for free with every new test. Full suite green with the guard
live (3791 passed; 2 pre-existing load flakes pass in isolation).
Every model with a JSON-in-Text column hand-rolled the same try/except
json.loads pair — get_details/get_context/get_headers/get_payload and a
dozen more, each a place the next fix would have to land N times. The
mixin owns the mechanics (empty-is-default, corrupt-is-default so a
hand-edited row never 500s an endpoint, optional shape check,
falsy-on-write); the per-model parts — accessor name, column, default —
stay per-model.
Conversion is behaviour-preserving: identical fallbacks, identical
on-write empty forms (NULL vs '[]' vs '{}'). Deliberately NOT converted:
accessors that let json.loads raise on corrupt data (collapsing those
would be a behaviour change, not a consolidation), and inline parses in
to_dict (per-entity serialization choice). A handful of 'except
Exception' reads narrow to the dominant (JSONDecodeError, TypeError) —
the only exceptions json.loads raises for str input.
The static scan sees literal argv lists only — 164 of 288 raw subprocess
sites (57%), and only the hardcoded names in SBIN_COMMANDS. A green run
was easy to read as 'no bare sbin calls exist'; it never meant that. Say
so in the module header and in the failure message itself, and point at
the runtime layer (popen_guard) that watches actual argv.
_backup_unverified_check returned ok for any verify_level that was not
literally 'none', so a checksum mismatch — positive evidence a backup may
not restore — rendered as 'Latest backup verified (listed).', and an
unreadable archive read as merely 'not verified yet'. verify_run_tier1
was already honest; the summary boundary was not, the same shape as the
drill badge. fail is now reserved for verification that ran and came
back bad, warn for 'couldn't check', and ok is positively earned.
plan 75 §A1 — and it was a live bug, not the hardening pass I expected.
_drill_badge() returned 'ok' for any status that was not literally 'failed'. A
drill skipped for lack of scratch space records 'skipped_no_space' AND stamps
last_drill_at, so it fell through every branch and Doctor reported:
"A recent restore drill proved this backup restores."
about a drill that never executed. It also self-perpetuated: each skip
refreshed last_drill_at, so a policy permanently short of scratch space could
never even go stale. It would report proven forever, having proven nothing —
and "short of scratch space" is the normal state of a full disk, which is
exactly when you most need the backup.
The drill service was careful (its own comment reads "loud skip, never a silent
pass") and the frontend renders an amber pill. The honesty was lost in the one
function that summarises it, which is the one Doctor reads.
Fixes:
- 'ok' must now be positively earned: only last_drill_status == 'success'
reaches it. Adds 'skipped' and 'unknown' states, so an unrecognised future
status cannot reintroduce this by falling through.
- staleness is evaluated only for a drill that actually succeeded, since it is
a property of proof rather than of attempts.
- Doctor renders both new states as warn with what was and was not proven.
- the skip path now notifies. BackupAlertService.on_drill_result already mapped
skipped_no_space to a failed outcome, but only the exception path ever called
it, so that branch was unreachable and a skipped drill told nobody. A log
line is not a notification.
Mutation-checked: with the fix reverted, the 5 bug-targeting tests fail and the
9 pinning existing behaviour still pass. 307 green across backup/doctor/restore
(a 422 in test_raw_infra_authz did not reproduce on re-run and is absent from
the baseline selection — the documented intermittent, not this change).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
plan 75 §B1. Two helpers did overlapping jobs with different semantics, and the
one that says nothing about privilege sat one autocomplete from the one that
does. nginx_advanced_service reached for it, so `nginx -t` ran unprivileged and
by bare name — it could neither read root-owned config nor find /usr/sbin/nginx,
and reported every config as invalid.
Renamed rather than folded. Folding into run_privileged(privileged=False) would
change sudo semantics at all six call sites — `sudo -n dig` fails where plain
dig worked, if sudoers does not permit it — and the failure this fixes was
choosing the wrong helper, not the helper's behaviour. The rename removes the
ambiguity with zero behavioural risk; the return type (dict, not
CompletedProcess) stays as callers expect.
`run_unprivileged(['nginx', '-t'])` reads as obviously wrong. `run_command` did
not. The old name was ambiguous in a second way: PythonService has an unrelated
method of the same name, which keeps it.
Six call sites, three test modules. Two patch targets were written as strings
('app.utils.system.run_command', setattr(mod, 'run_command')) so a word-boundary
rename skipped them and they would have failed at runtime, not import.
No alias: an alias leaves the second door open, which is the thing being closed.
run_command was never exported through plugins_sdk, so there is no public SDK
surface to break; a third-party extension importing it gets a loud ImportError
rather than silent misbehaviour. A guard test asserts the name stays gone.
417 tests green across monitor, ssl, dns, nginx, firewall and the guards.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
plan 75 §C. Bundled extensions exist twice on disk: builtin-extensions/<ext>/
backend is the source, backend/app/plugins/<ext> is what the loader imports. A
fix applied to one and not the other ships a repo that reads correct and a panel
that runs the old code.
Not hypothetical — the plan-74 dovecot fix had to be made twice, caught only by
remembering to look.
Written while drift is zero (21 file pairs), so this locks in a clean invariant
rather than repairing a dirty one. It only stays this cheap while that is true.
Compares by CRLF-normalised hash, since the repo has mixed line endings, and
only pairs present on BOTH sides: cloudflare-ops and localkit ship source with
no materialised live copy and serverkit-git uses a different layout, all
legitimate. Most live copies are gitignored, so a fresh clone sees fewer pairs
than a dev machine (serverkit-email's predates the ignore rule and is tracked).
test_drift_check_is_not_vacuous stops that from quietly reducing the
parametrised check to zero cases while still reporting green.
Also fails when a file exists on only one side — drift before anyone edits it.
Both assertions mutation-checked: appending a byte to the live dovecot copy
fails the hash test, adding a one-sided file fails the membership test, and the
tree is green again once reverted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 enable() and remove_rule() guards reason about ports. These two paths never
touch the SSH rule and lock the operator out anyway:
block_ip(<your own address>) the rule is untouched; your source
address is rejected before reaching it
set_default_zone(<zone without ssh>) firewalld re-homes every interface with
no explicit zone; the rules did not
change, the zone around them did
check_ip_block() refuses when the target covers a live SSH peer, the caller's
own HTTP client address, or loopback — nginx forwards to 127.0.0.1, so blocking
it takes the panel down with the session. CIDRs are matched with `ipaddress`,
because 73.244.0.0/16 is the same lockout wearing a wider mask.
Peers come from `ss -Htn state established`, filtered to the sshd ports; `who`
is empty for sessions with no utmp entry. Verified against the live host, whose
only established SSH connection was 146.190.213.37:22 <- 73.244.95.52:59085:
active_ssh_peers() -> {'73.244.95.52'}
block 73.244.95.52 -> refused
block 73.244.0.0/16 -> refused (covers the same peer)
block 127.0.0.1 -> refused (loopback)
block 198.51.100.7 -> allowed
caller_ip is read in the API layer from request.remote_addr and passed in, so
the service stays free of request context.
check_default_zone() asks whether the target zone permits ssh via
--list-services / --list-ports before switching. A failed `ss` probe yields no
peers rather than fabricating conflicts, so a broken probe cannot block every
request. Consistent with the existing guards: undeterminable SSH port refuses,
an unenforcing firewall is a no-op, force=true is the operator accepting it.
/block-ip and /zones/default answer 409 with the conflicting addresses.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
