Close the four smaller findings from the wilm import review

None blocked the merge; all four were mechanisms that did not mechanise.

_RUNNING_STATES existed twice, in api/routes.py and services/reconciler.py, with
a comment asking a human to keep them in lockstep. The panel and the reconciler
read the same agent snapshot, so a drift between the copies would paint a
service green and flag it as drift on the same page. One definition now, owned
by the reconciler and imported by routes - api -> services is the direction the
plugin layout already runs in, and the reconciler's import of routes is deferred,
so there is no cycle.

The operator-report upload checked its 10 MB cap AFTER upload.read() had already
pulled the whole body into memory, so the check could not prevent the thing it
was there to prevent. content_length is now an early out and the read itself is
bounded to cap+1, which is enough to know the cap was exceeded without taking a
body that lied about its length. The multipart envelope carries the other form
fields too, so the early out allows 64 KB of slack rather than rejecting an
image that is just under.

The same handler forced the extension to .png and the mimetype to image/png
whatever the bytes were, then served the file back under that type. The upload
headers (CSP sandbox + nosniff) make the document case inert, so this was never
the hole it looks like, but nothing is gained by storing a file we already know
is not what it claims. The PNG signature is now checked.

The lightbox bound Escape and the arrow keys to a div and never focused it, so
every one of those bindings was dead until the user happened to click the
backdrop. It takes focus on open and hands it back to the thumbnail on close.

On the two upload tests, honestly: the PNG one was confirmed to fail with the
check removed. The oversize one passes either way, because the old code did
reject an oversized image - it just did so after reading all of it. That test
pins the response, not the fix. Whether the read is bounded is not observable
through the response, so nothing here proves it; the change stands on reading
the handler.

Suite 2311 passing, frontend 316 passing, build clean.

Scope: product

frontend/src/components/PluginAssetPanels.vue

@@ -186,10 +186,11 @@
        page rather than a single card. -->
   <div
     v-if="lightbox.panel"
+    ref="lightboxEl"
     class="pap-lightbox"
     role="dialog"
     aria-modal="true"
-    tabindex="0"
+    tabindex="-1"
     @click.self="closeReport"
     @keydown.esc="closeReport"
     @keydown.left="cycleReport(-1)"
@@ -235,7 +236,7 @@
 </template>
 
 <script setup>
-import { ref, computed, watch, onUnmounted } from 'vue'
+import { ref, computed, watch, onUnmounted, nextTick } from 'vue'
 import api from '../api'
 import { colorStyle } from '@/utils/colorStyle'
 import {
@@ -291,12 +292,26 @@ const openReportRow = computed(() => {
   return rows(lightbox.value.panel)[lightbox.value.index] || null
 })
 
+// The overlay owns Escape and the arrow keys, and a keydown handler on a div
+// only fires while that div has focus. Nothing moved focus to it, so every
+// keyboard binding on the lightbox was dead until the user happened to click
+// the backdrop. Focus it once it has rendered, and hand focus back to the
+// thumbnail that opened it on close, so keyboard users are not dropped at the
+// top of the document.
+const lightboxEl = ref(null)
+let reopenFocus = null
+
 function openReport(panel, index) {
+  reopenFocus = typeof document !== 'undefined' ? document.activeElement : null
   lightbox.value = { panel, index }
+  nextTick(() => lightboxEl.value?.focus())
 }
 
 function closeReport() {
   lightbox.value = { panel: null, index: 0 }
+  const returnTo = reopenFocus
+  reopenFocus = null
+  nextTick(() => returnTo?.focus?.())
 }
 
 function cycleReport(delta) {

plugins/endpoints/api/routes.py

@@ -39,11 +39,12 @@ def _naive_utcnow():
     return datetime.now(timezone.utc).replace(tzinfo=None)
 
 
-# A watched service or process reports a STATE STRING, not a boolean, and the
+# What counts as RUNNING is defined once, by the reconciler, and imported here.
-# vocabulary varies by source. Both the services panel and the fleet health
+# The panel and the reconciler read the same agent snapshot, so a disagreement
-# rollup have to agree on what counts as running, so the rule lives here once -
+# would paint a service green and flag it as drift on the same page. Importing
-# reading it wrong makes every service look stopped.
+# api -> services is the direction the plugin layout already runs in; the
-_RUNNING_STATES = ('running', 'started', 'up', 'ok', 'active')
+# reconciler's own import of this module is deferred, so there is no cycle.
+from ..services.reconciler import RUNNING_STATES as _RUNNING_STATES
 
 
 def _service_running(entry):
@@ -1781,6 +1782,13 @@ OPERATOR_REPORT_RETENTION_DEFAULT_DAYS = 7
 # Screenshots are small PNGs from a virtual-screen grab. Cap the upload so a
 # malformed or hostile agent cannot fill the disk with one request.
 OPERATOR_REPORT_MAX_IMAGE_BYTES = 10 * 1024 * 1024
+# The multipart envelope carries the other form fields and the part boundaries
+# too, so content_length is legitimately larger than the image. Allow for that
+# rather than rejecting a screenshot that is just under the cap.
+_OPERATOR_REPORT_FORM_SLACK = 64 * 1024
+# PNG signature. The route stores and serves the file as image/png, so this is
+# the one thing it accepts.
+_PNG_MAGIC = b'\x89PNG\r\n\x1a\n'
 
 
 def _operator_report_dir():
@@ -1837,7 +1845,6 @@ def upload_operator_report():
     holds only metadata plus the stored filename. One retention sweep runs
     opportunistically so the feed stays bounded.
     """
-    from werkzeug.utils import secure_filename
     import uuid
 
     error = _collector_auth_or_error()
@@ -1862,19 +1869,38 @@ def upload_operator_report():
     imagemimetype = None
     imagebytesize = None
 
+    # Refuse an oversized body BEFORE reading it. The cap used to be checked
+    # after upload.read(), which pulls the whole thing into memory first - the
+    # check could not prevent what it was there to prevent. content_length is
+    # caller-supplied so it is a cheap early out, not the enforcement; the read
+    # below is still bounded.
+    declared = request.content_length or 0
+    if declared > OPERATOR_REPORT_MAX_IMAGE_BYTES + _OPERATOR_REPORT_FORM_SLACK:
+        return error_response(
+            ErrorCodes.VALIDATION_ERROR,
+            'screenshot exceeds the {} byte limit'.format(
+                OPERATOR_REPORT_MAX_IMAGE_BYTES))
+
     upload = request.files.get('screenshot')
     if upload is not None and upload.filename:
-        data = upload.read()
+        # Read one byte past the cap: enough to know it was exceeded, without
+        # taking the whole of a body that lied about its length.
+        data = upload.read(OPERATOR_REPORT_MAX_IMAGE_BYTES + 1)
         if len(data) > OPERATOR_REPORT_MAX_IMAGE_BYTES:
             return error_response(
                 ErrorCodes.VALIDATION_ERROR,
                 'screenshot exceeds the {} byte limit'.format(
                     OPERATOR_REPORT_MAX_IMAGE_BYTES))
+        if data and not data.startswith(_PNG_MAGIC):
+            # The stored file is served back as image/png. Storing whatever
+            # arrived under that name meant the extension and the bytes could
+            # disagree; the upload headers make the document case inert, but
+            # nothing is gained by keeping a file we already know is not what it
+            # claims.
+            return error_response(ErrorCodes.VALIDATION_ERROR,
+                                  'screenshot must be a PNG')
         if data:
-            ext = os.path.splitext(secure_filename(upload.filename))[1].lower()
+            imagefilename = 'report-{}.png'.format(uuid.uuid4().hex)
-            if ext != '.png':
-                ext = '.png'
-            imagefilename = 'report-{}{}'.format(uuid.uuid4().hex, ext)
             imagemimetype = 'image/png'
             imagebytesize = len(data)
             report_dir = _operator_report_dir()

plugins/endpoints/services/reconciler.py

@@ -27,11 +27,16 @@ Two seams are kept deliberately narrow so this stays easy to change:
       can call it directly.
 """
 
-# The running-state vocabulary a watched process/service can report. Kept in
+# The running-state vocabulary a watched process/service can report. A watched
-# lockstep with plugins/endpoints/api/routes.py::_RUNNING_STATES so a service the
+# service reports a STATE STRING, not a boolean, and the vocabulary varies by
-# panel paints green is never flagged as drift here. Both read from the same
+# source, so what counts as running is a single rule rather than two.
-# agent snapshot; disagreeing would show a running app as out-of-desired-state.
+#
-_RUNNING_STATES = ('running', 'started', 'up', 'ok', 'active')
+# It lives here, and api/routes.py imports it, because the panel and this
+# reconciler MUST agree: a service the panel paints green that this flags as
+# drift is a contradiction shown to the same person on the same page. It was
+# two copies with a comment asking a human to keep them in lockstep, which is
+# not a mechanism.
+RUNNING_STATES = ('running', 'started', 'up', 'ok', 'active')
 
 
 def monitor_expects_running(monitor):
@@ -82,7 +87,7 @@ def _reported_running_keys(reported_services):
         else:
             name, raw, kind = str(entry), '', None
         name = name.strip().lower()
-        if not name or raw not in _RUNNING_STATES:
+        if not name or raw not in RUNNING_STATES:
             continue
         running.add((kind, name))
     return running

tests/test_plugins/test_endpoints_operator_reports.py

@@ -235,3 +235,51 @@ def test_retention_disabled_keeps_old_reports(client, db, app):
                 headers=_collector_headers(app))
 
     assert db.session.get(OperatorReport, old.operatorreportid) is not None
+
+
+# --- what the route refuses to store -----------------------------------------
+#
+# The stored file is served back as image/png, and the size cap is the only
+# thing bounding what an agent key can push into the instance directory. Both
+# guards used to be nominal: the cap was checked AFTER upload.read() had already
+# pulled the whole body into memory, and the extension was forced to .png
+# whatever the bytes were.
+
+def test_upload_refuses_bytes_that_are_not_a_png(client, db, app):
+    from plugins.endpoints.models import OperatorReport
+
+    _computer(db, 'OPREPORT-NOTPNG')
+    data = {
+        'hostname': 'OPREPORT-NOTPNG',
+        'details': 'not really a screenshot',
+        'screenshot': (io.BytesIO(b'MZ\x90\x00 this is a PE, not an image'),
+                       'shot.png'),
+    }
+    response = client.post(UPLOAD_URL, data=data,
+                           content_type='multipart/form-data',
+                           headers=_collector_headers(app))
+    assert response.status_code == 400
+    # Nothing was written: not the row, and not a file under the uploads dir.
+    assert OperatorReport.query.filter_by(
+        reportedhostname='OPREPORT-NOTPNG').first() is None
+
+
+def test_upload_refuses_an_image_over_the_cap(client, db, app):
+    from plugins.endpoints.api.routes import OPERATOR_REPORT_MAX_IMAGE_BYTES
+    from plugins.endpoints.models import OperatorReport
+
+    _computer(db, 'OPREPORT-HUGE')
+    # A real PNG header so the size check is what rejects it, not the signature.
+    oversized = _png_bytes() + b'\x00' * (OPERATOR_REPORT_MAX_IMAGE_BYTES + 1)
+    data = {
+        'hostname': 'OPREPORT-HUGE',
+        'details': 'oversized',
+        'screenshot': (io.BytesIO(oversized), 'shot.png'),
+    }
+    response = client.post(UPLOAD_URL, data=data,
+                           content_type='multipart/form-data',
+                           headers=_collector_headers(app))
+    assert response.status_code == 400
+    assert 'limit' in response.get_json()['data']['error']['message']
+    assert OperatorReport.query.filter_by(
+        reportedhostname='OPREPORT-HUGE').first() is None