Clear the review blockers on the merged wilm work
All checks were successful
CI / naming (push) Successful in 3s
CI / frontend (push) Successful in 13s
CI / migrations-mysql (push) Successful in 42s
CI / backend (push) Successful in 4m18s

A multi-agent review of the merged branch produced 29 findings that survived
adversarial verification. This clears the blockers and the four worth fixing
before it reaches main. The design held up well: path-traversal defence on all
four serving routes is clean, the agent's SHA-256 check fails closed, enqueue
authority is proportionate and audited, and no site literals or credential leaks
in the new C#.

THE TESTS NEVER RAN

pytest.ini sets testpaths = tests, so plugins/endpoints/tests/ was collected ZERO
times. Fifty-five lines of delivery assertions were dead code, and run directly
five of them failed. plugins/printedparts/tests/ had been invisible for longer.

That is the worst shape a gap can take: the suite is green, the coverage is
imaginary, and the collector auth on a download route could be deleted without
anything going red.

Both files moved under tests/. tests/test_no_hidden_tests.py now fails if a test
file appears anywhere pytest will not look; it caught a staged copy inside the
installer bundle on its first run, and was verified by planting a violation.

The five failures were stale expectations, and they split by cause. His: the
applications panel made six panels where his test said five, and 'applications'
belongs in KNOWN_PAYLOAD_KEYS now that it is persisted - his own test contradicted
his own feature. Ours: we renamed the events card to activity, moved the
fleet-wide cards to endpoints.fleet, and made the list route a bare route('')
which Flask rejects unless the blueprint is mounted under a prefix.

FOUR FIXES

The snapshot table's key was a 32-bit autoincrement on a table rewritten
wholesale every check-in, so it climbs at (endpoints x apps) per report and
exhausts on MySQL. Widened while the table is unreleased. Widening it broke every
insert on SQLite, which only autoincrements a column typed exactly INTEGER
PRIMARY KEY, so it carries a dialect variant - and nothing caught either fault,
because no test inserted into that table at all. One does now.

Ingest truncated displayname but not displayversion or publisher. On strict MySQL
an over-long registry value fails the whole check-in transaction, so one machine
with a malformed Uninstall entry goes silent entirely. SQLite does not enforce
lengths, so the suite would never have shown it.

update_app's version: branch pinned AppVersion.sha256, but that route resolves the
version to its APPLICATION and serves application.installpath - so the digest
described an artifact the route never sends, and where it was null the agent had
nothing to verify and failed open. It now pins the file actually served.

The same branch ended the delivery URL at the id, so an MSI arrived with no
extension and could not be routed through msiexec. It carries the filename now.

Agent packaging moved off require_role('admin') to a new endpoints.agentpackage
permission. The generated ZIP carries a live collector key, so this is a heavier
grant than deploying to one endpoint - and a role cannot be given to one person
for one job the way a permission can.

Full suite 2231 passed (2198 before, plus the 22 that never ran and 11 new),
frontend 314.
This commit is contained in:
cproudlock
2026-08-30 17:17:13 -04:00
parent 7dc1f0bee8
commit 5f27f81e88
15 changed files with 1256 additions and 27 deletions

View File

@@ -78,6 +78,20 @@ def _as_json(value):
return json.dumps(value, separators=(',', ':'), sort_keys=True)
def _clamp(value, length):
"""A string bounded to its column width, or None.
Ingest writes whatever an endpoint reports. A registry value longer than its
column is not a data-quality nuisance on MySQL: in strict mode it raises
Data too long and fails the whole check-in transaction, so one machine with
a malformed Uninstall entry stops reporting telemetry and events entirely.
"""
if value is None:
return None
text = str(value).strip()
return text[:length] or None
class EndpointsPlugin(BasePlugin):
"""Endpoint RMM telemetry ingest plus read/dashboard surface."""
@@ -153,6 +167,17 @@ class EndpointsPlugin(BasePlugin):
# heavier grant than a routine service/process toggle.
('endpoints.deploy',
'Run staged scripts and push application installers to endpoints', 'endpoints'),
# Packaging the agent is a heavier grant than deploying to one, and
# it is not the same job either: the generated ZIP carries a live
# collector key stamped into its install script, so whoever holds
# this can mint a distributable artefact containing the fleet-wide
# command-channel credential. It was require_role('admin'), which
# this codebase otherwise reserves for the few genuinely
# role-shaped checks - and a role cannot be granted to one person
# for one job the way a permission can.
('endpoints.agentpackage',
'Generate and download an agent or adoption-tool package '
'(contains a live collector key)', 'endpoints'),
# Collector service scope for the endpoint agent. A managed token
# scoped to ONLY this authorizes the endpoints collector and nothing
# else, so rotating/revoking it stops endpoint-agent traffic without
@@ -641,11 +666,18 @@ class EndpointsPlugin(BasePlugin):
if not displayname:
warnings.append('skipped application with no displayname')
continue
# TRUNCATE EVERY STRING, not just the name. On MySQL in strict
# mode an over-long value raises Data too long and takes down the
# WHOLE check-in transaction, so one machine with a malformed
# Uninstall registry entry stops reporting entirely - telemetry,
# events and all - until somebody uninstalls the offender. SQLite
# does not enforce declared lengths, so no test here would ever see
# it. displayname was already clamped; these two were not.
db.session.add(EndpointApplication(
assetid=assetid,
displayname=displayname[:255],
displayversion=(item.get('displayversion') or None),
publisher=(item.get('publisher') or None),
displayversion=_clamp(item.get('displayversion'), 128),
publisher=_clamp(item.get('publisher'), 255),
))
stored += 1
return stored