Act on the review of the wilm import
Some checks failed
CI / naming (push) Successful in 3s
CI / frontend (push) Successful in 13s
CI / backend (push) Failing after 5m8s
CI / migrations-mysql (push) Successful in 45s

Six findings from a four-agent review of the imported work, plus the contract
bump the import owed. Nothing here changes what the imported features do.

The contract version was not bumped for a change that widens what a plugin may
declare. The asset-panel vocabulary gained two tab groups (serviceprocess,
reports), a datetime column format, a map.body key and a tabs section that
renders as a table. 0.29.0 bumped for exactly this class of change when the
optional group was added, so this is 0.31.0 with a ledger row. All four are
optional and resolve server-side when absent, so a plugin declaring none of them
is unaffected.

Site Backups now answers honestly where it has nothing behind it. The overlay is
a compose profile: a separate container writes sets onto a shared volume and the
api container only reads them. A Windows/IIS install runs no such container and
the default root resolves to a drive-letter path nothing creates, so Back Up Now
either failed on PermissionError or reported a run queued that would never
happen, under a banner reading "No verified backup yet" - which describes a
backup that failed, not a feature that was never installed. The three write
routes refuse with the same 404 shape the configuration-policy routes use for a
disabled feature, the list and status routes report whether a store exists, and
the page says so instead of offering buttons.

Two listAll callers still read a response that is not there. listAll resolves to
a plain array through fetchAllPages, so response.data.data throws: PC Types
settings rendered empty, and in Configuration Policies the throw happened inside
the shared try, so PC types, applications, the action catalog and monitors were
all left unassigned by one bad read.

The agent package upload cannot succeed on IIS as shipped. MAX_CONTENT_LENGTH
went to 256 MB to admit the ~69 MB tray binary, but IIS request filtering caps a
body at 30,000,000 bytes and answers 413 before waitress sees the request, so
web.config needs the matching requestLimits.

The backup image could not be built by anyone outside a TLS-intercepting
network. Dockerfile.backup did a plain COPY of a gitignored corporate root CA,
and a COPY of a missing file fails the build. The trailing glob makes it a no-op
when the cert is absent.

endpointstates.domain was the one new string column written without a clamp.
Under STRICT_TRANS_TABLES an over-long value raises Data too long and aborts the
whole check-in, so a single malformed report would stop a machine reporting
telemetry and events entirely.

Not fixed here, and left with upstream: the restore path finalizes its schema
upgrade only from a browser poll loop, so closing the tab mid-restore leaves the
data behind the code; it restores instance files from the newest snapshot rather
than the set that was asked for; and it takes no safety dump before the
destructive load. All three are in the Docker-only overlay, which this site does
not run, and none can be validated here.

Scope: product
This commit is contained in:
cproudlock
2026-09-06 16:47:47 -04:00
parent 6e36df841e
commit 83190422c9
10 changed files with 99 additions and 7 deletions

View File

@@ -101,6 +101,17 @@
</rewrite>
SHOPDB-CLIENTIP-END -->
<security>
<requestFiltering>
<!-- IIS request filtering caps a request body at 30,000,000 bytes by
default and answers 413 before waitress sees it. The agent package
upload accepts 128 MB (MAX_CONTENT_LENGTH is 256 MB), and the tray
binary alone is ~69 MB, so the IIS cap has to be raised to match or
every agent upload fails on Windows while working under Docker. -->
<requestLimits maxAllowedContentLength="268435456" />
</requestFiltering>
</security>
</system.webServer>
<!--

View File

@@ -49,6 +49,7 @@ Recorded in the comment block in `shopdb/__init__.py`:
| 0.27.0 | Added `Script` so a plugin can read the ShopDB-hosted script library that backs endpoint `run_script` delivery without importing core model internals. The endpoints plugin resolves a delivered script by name and serves its body by digest to the agent | additive surface (minor) |
| 0.29.0 | Added an optional `group` to `get_asset_panels`, naming which detail-page tab a panel belongs to: `details` (the default) / `monitors` / `software` / `activity`. The detail pages are tabbed, and a page that placed panels by knowing their ids would defeat the hook. Additive: an absent or unrecognised group resolves to `details` server-side, so every existing plugin keeps working with no change | additive surface (minor) |
| 0.28.0 | Added `client_ip` and `strip_port` so a plugin does not hand-roll caller-IP extraction. Two plugins had, and both read the first `X-Forwarded-For` hop, which the caller supplies - so a rate limit keyed on it was defeated by rotating the header. The helper resolves from `remote_addr`, which ProxyFix has already filled from the header when `TRUSTED_PROXY_COUNT` says a proxy is really in front | additive surface (minor) |
| 0.31.0 | The asset-panel vocabulary a plugin may declare grew four optional things: two more tab groups, `serviceprocess` and `reports`, beside the 0.29.0 set; a `format: 'datetime'` on a column or field, so a panel stops shipping pre-formatted date strings the browser cannot re-localise; a `map.body` key, so a keyvalue field can name where its value lives in a nested payload; and a `tabs` section that carries `columns`/`rows`/`empty` and renders as a table, so one card can hold two tables that share no key (volume capacity and physical drive health are the case). All optional and all resolved server-side when absent, so a plugin declaring none of them is unaffected | additive surface (minor) |
The source comment block documents 0.3.0, 0.4.0, 0.6.0, 0.7.0, 0.9.0, 0.10.0,
0.11.0, 0.16.0, 0.18.0 and 0.19.0; 0.12.0 through 0.15.0 and 0.17.0 are recorded

View File

@@ -12,7 +12,7 @@ never by editing this file.
| series | value | governed by |
|---|---|---|
| product `__version__` | `0.12.0` | ADR-007 |
| plugin contract `__contract_version__` | `0.30.0` | ADR-002 |
| plugin contract `__contract_version__` | `0.31.0` | ADR-002 |
They move independently. A contract bump is not a release.

View File

@@ -4,9 +4,18 @@
<h2>Backups</h2>
</div>
<div v-if="!isConfigured" class="not-configured">
<strong>Site Backups is not configured on this deployment.</strong>
<span>
The feature needs the backup overlay (a separate container writing sets
onto a shared volume) and a readable backups root. A Windows/IIS install
does not run it, so nothing here would have a store behind it.
</span>
</div>
<!-- Confidence banner: the single most reassuring thing an admin wants to
see - is there a recent, verified backup? Everything else is secondary. -->
<div class="confidence-banner" :class="bannerClass">
<div v-if="isConfigured" class="confidence-banner" :class="bannerClass">
<span class="banner-icon">
<component :is="bannerIcon" :size="22" />
</span>
@@ -302,6 +311,9 @@ const sets = ref([])
const loading = ref(true)
const isBusy = ref(false)
const lastGoodAt = ref(null)
// Assume configured until the list says otherwise, so a slow first load
// does not flash the not-configured notice on a working install.
const isConfigured = ref(true)
const restoreTarget = ref(null)
// Live restore progress. restoreStatus mirrors the api /status.restore payload;
@@ -405,6 +417,10 @@ async function load() {
const response = await api.get('/shopdbbackups')
sets.value = response.data.data || []
lastGoodAt.value = response.data.meta?.lastgoodat || null
// A deployment without the backup overlay has no store behind any of
// this. Say so instead of showing 'No verified backup yet', which reads
// as a backup that failed rather than a feature that is not installed.
isConfigured.value = response.data.meta?.configured !== false
} catch (error) {
toast.error('Could not load backups.')
} finally {
@@ -647,6 +663,17 @@ onUnmounted(() => {
.banner-none { border-color: #c53030; }
.banner-none .banner-icon { color: #e53e3e; }
.not-configured {
display: flex;
flex-direction: column;
gap: 0.35rem;
padding: 1rem 1.25rem;
margin-bottom: 1rem;
border: 1px solid var(--border-color, #d8dbe0);
border-radius: 8px;
background: var(--surface-muted, #f6f7f9);
}
.explain {
margin-bottom: 1.25rem;
border: 1px solid var(--border);

View File

@@ -174,7 +174,9 @@ async function loadData() {
}
if (types.status === 'fulfilled') {
pctypes.value = types.value.data.data || []
// Plain array, same as applications below. Reading .data.data threw
// inside this try, so apps, actions and monitors never got assigned.
pctypes.value = types.value || []
} else {
failures.push('PC types')
console.error('Error loading PC types:', types.reason)

View File

@@ -107,8 +107,10 @@ onMounted(() => loadData())
async function loadData() {
loading.value = true
try {
// listAll resolves to a plain array (fetchAllPages), not an axios
// response. Reading .data.data threw and left the page empty.
const response = await computersApi.types.listAll({ active: false })
pcTypes.value = response.data.data || []
pcTypes.value = response || []
} catch (err) {
console.error('Error loading PC types:', err)
} finally {

View File

@@ -1121,7 +1121,10 @@ class EndpointsPlugin(BasePlugin):
if 'osinfo' in payload:
state.osinfojson = _as_json(payload.get('osinfo'))
if 'domain' in payload:
state.domain = payload.get('domain')
# Clamped like every other String column here: an over-long value
# aborts the whole collector transaction under STRICT_TRANS_TABLES,
# so one bad agent report would drop a machine's entire telemetry.
state.domain = _clamp(payload.get('domain'), 255)
if 'pendingreboot' in payload:
state.pendingreboot = payload.get('pendingreboot')
if 'diskhealth' in payload:

View File

@@ -14,7 +14,13 @@ FROM mysql:8.4
# bzip2/python3/alembic or a base image that carries them) so the release image
# needs no outbound calls and no embedded cert. The cert is gitignored (*.pem)
# and must be staged at scripts/backup/zscaler-root-ca.pem before building.
COPY scripts/backup/zscaler-root-ca.pem /etc/pki/ca-trust/source/anchors/zscaler-root-ca.pem
#
# The trailing * makes the COPY optional: a plain COPY of a gitignored file
# fails the build outright, so nobody outside a TLS-intercepting network -
# another site, or our own CI - could build this image at all. Absent cert,
# the glob matches nothing, the layer is a no-op and the fetches below go out
# over the public trust store.
COPY scripts/backup/zscaler-root-ca.pem* /etc/pki/ca-trust/source/anchors/
RUN update-ca-trust
# restic from the distro is often stale; pull the pinned release binary so the

View File

@@ -110,7 +110,12 @@ from .plugins import plugin_manager
# internals. The journal records the primitive mutations a bulk import makes
# (update a cell, insert a row) tied to a batch id, and a generic undo replays
# them in reverse. Additive names on the surface, minor bump.
__contract_version__ = '0.30.0'
#
# 0.31.0 widens the asset-panel vocabulary a plugin may declare: two more
# tab groups (serviceprocess, reports), a datetime column format, a map body
# key, and a tabs section that renders as a table. All optional, so a plugin
# that declares none of them is unaffected. Additive, minor bump.
__contract_version__ = '0.31.0'
# Product release version (see ADR-007). The product version and the
# plugin-contract version above are distinct series with independent

View File

@@ -85,6 +85,30 @@ def _backups_root():
return os.environ.get(BACKUPS_ROOT_ENV, DEFAULT_BACKUPS_ROOT)
def _backups_configured():
"""Whether this deployment has a backup store behind the feature.
The overlay is a compose profile: a third container writes the sets onto a
shared volume, and the api container only ever reads them and drops a
sentinel. A Windows/IIS install has no such container, and the default root
resolves there to a drive-letter path nothing creates - so every button
either 500s on PermissionError or reports 'queued' for a run that will
never happen. Answer the caller honestly instead.
"""
return os.path.isdir(_backups_root())
def _unavailable_response():
# 404 + NOT_FOUND is the established shape for "this feature is not
# enabled here" (the configuration-policy routes answer the same way), and
# the UI already tells a 404 apart from a real failure.
return error_response(
ErrorCodes.NOT_FOUND,
'Site Backups is not configured on this deployment. It needs the '
'backup overlay and a readable backups root (%s).' % BACKUPS_ROOT_ENV,
http_code=404)
def _read_json(path):
"""Load a JSON file, or None when it is absent or unreadable."""
try:
@@ -339,6 +363,7 @@ def list_backups():
'retention': RETENTION,
'lastgoodat': (laststgood or {}).get('takenat'),
'count': len(sets),
'configured': _backups_configured(),
}
return success_response(data=sets, meta=meta)
@@ -373,6 +398,9 @@ def trigger_backup():
up. Returning isrunning=false with a clear message keeps the UI honest
rather than pretending the run started inside this process.
"""
if not _backups_configured():
return _unavailable_response()
root = _backups_root()
try:
os.makedirs(root, exist_ok=True)
@@ -401,6 +429,9 @@ def restore_backup(snapshotid):
The actual restore is performed by scripts/backup/restore.sh in the backup
container; here we validate and record intent.
"""
if not _backups_configured():
return _unavailable_response()
root = _backups_root()
setdir = os.path.join(root, os.path.basename(snapshotid))
if not os.path.isdir(setdir):
@@ -451,6 +482,9 @@ def finalize_restore():
Idempotent and safe to call when nothing is pending: if there is no
.upgrade-requested sentinel it reports there was nothing to finalize.
"""
if not _backups_configured():
return _unavailable_response()
root = _backups_root()
sentinel = os.path.join(root, '.upgrade-requested')
try:
@@ -496,6 +530,7 @@ def get_status():
value is None when that kind of run has never been requested.
"""
return success_response(data={
'configured': _backups_configured(),
'backup': _read_status('backup'),
'restore': _read_status('restore'),
})