Unpick one site's conventions from the shipped bundle, and bind the launch-path

check to the file it actually runs

Two clusters the five-agent review found. Both are about a product that another
plant has to be able to install.

ADR-015: the shipped monitor bundle was the originating plant's fleet
convention frozen into product seed.

  The business policy was seeded as "RPCA Shopfloor App Monitors". RPCA is that
  plant's facility designator, the row is source='shipped', and the API refused
  to rename it - so every adopting site would carry another plant's label at the
  top of its policy list forever. 0007 now seeds the neutral name, and 0010
  renames it on installs that already stamped 0007, guarded so it cannot collide
  with a policy a site made itself. Downgrade is one-way on purpose: putting
  another site's designator back is not a state worth reaching.

  Which PC type the bundle attaches to was the literal 'Shopfloor PC'. A plant
  whose type is "Shop Floor Workstation" silently got no shipped monitors at
  all, and one using that name for something else got all of them. It is a
  setting now, defaulting to the seeded name so nothing changes here, and a
  blank value attaches the bundle to nothing - the right neutral answer for a
  site that does not want it.

  Shipped rows refused every edit, including isactive and launchpath. A plant
  whose eDNC is on D:, or that runs no UDC, could not correct the path or switch
  the monitor off, and then carried permanent false drift for it. ADR-015 allows
  seed data BECAUSE a site can edit it afterwards. Paths and the active flag are
  now editable; kind and displayname stay fixed, because that pair is the
  identity 0007's bundle matches on.

  Also corrected two false claims in 0007's docstring: the policy tables belong
  to the computers plugin, not the core baseline, and a guarded no-op does NOT
  reconcile later - Alembic stamps the revision either way, so the bundle would
  simply never seed.

LaunchPathPolicy was purely lexical. GetFullPath is string normalization that
never touches the disk, so the root check constrained the NAME while File.Exists
and CreateProcess followed reparse points to somewhere else. A junction under a
writable directory in an approved root - creating one needs no privilege, only
write access to the parent, and vendor trees under Program Files are not
reliably locked down - turned WebUI monitor-edit rights into code execution as
the console operator. It now resolves the real target, leaf and directory chain
both, and re-checks THAT against the roots, rejecting if resolution fails.

Alternate data streams passed the whole policy on net8.0: an interior colon no
longer throws in GetFullPath, so "readme.txt:payload.exe" reported an .exe
extension, passed the root check, and existed. Rejected explicitly now.

Six tests added for the shipped-row and pctype behaviour. The Core project
compiles clean. The launch-path change has NOT been exercised against real
junctions, ADS or 8.3 names - that needs the win11 VM, and per the reviewer it
wants a planted-violation suite there rather than my say-so.

Still open from the review: monitor revocation does not take effect until agent
restart, ActionPicker is keyboard-inoperable, the lightbox has no focus trap,
one attached monitor grants generic any-target process control, and screenshots
are retained unencrypted in the user profile.

Scope: product
This commit is contained in:
cproudlock
2026-09-02 15:44:17 -04:00
parent 88a683dad3
commit e98e15259e
8 changed files with 291 additions and 21 deletions

View File

@@ -63,6 +63,19 @@ public sealed class LaunchPathPolicy
return false; return false;
} }
// Reject an alternate data stream before anything else looks at the
// path. On .NET Framework an interior colon made GetFullPath throw; on
// net8.0 it does not, so "readme.txt:payload.exe" survives, reports an
// .exe extension, passes the root check, and File.Exists returns true
// for the stream. CreateProcess will run it. Write access to any
// existing file under an approved root would otherwise be enough.
var lastSegment = Path.GetFileName(resolved);
if (lastSegment.Contains(':'))
{
reason = "path names an alternate data stream";
return false;
}
var extension = Path.GetExtension(resolved); var extension = Path.GetExtension(resolved);
if (!ApprovedExtensions.Contains(extension, StringComparer.OrdinalIgnoreCase)) if (!ApprovedExtensions.Contains(extension, StringComparer.OrdinalIgnoreCase))
{ {
@@ -82,10 +95,81 @@ public sealed class LaunchPathPolicy
return false; return false;
} }
fullPath = resolved; // Everything above is LEXICAL: GetFullPath is pure string normalization
// and never touches the disk, so the root check so far constrains the
// name, not the file. File.Exists follows reparse points, which means a
// junction anywhere under an approved root pointed the check at one
// place and the execution at another. Creating a junction needs no
// privilege, only write access to the parent - and vendor install trees
// under Program Files are not reliably locked down.
//
// So resolve what the path actually refers to and re-check THAT. A
// resolve failure rejects: this is the last gate before the agent runs
// the file.
string finalPath;
try
{
finalPath = ResolveFinalPath(resolved);
}
catch (Exception ex)
{
reason = $"cannot resolve the real path: {ex.Message}";
return false;
}
if (!IsUnderApprovedRoot(finalPath))
{
reason = "path resolves outside every approved launch root";
return false;
}
fullPath = finalPath;
return true; return true;
} }
// The path with every symlink and junction on it followed, so the caller
// checks the file that will actually execute. The leaf is resolved first,
// then the directory chain above it - a junction is usually placed on a
// DIRECTORY, so resolving only the leaf would miss the case this exists to
// catch.
private static string ResolveFinalPath(string resolved)
{
var name = System.IO.File.ResolveLinkTarget(resolved, returnFinalTarget: true)?.FullName;
if (!string.IsNullOrWhiteSpace(name))
{
// ResolveLinkTarget resolves the file itself; run it back through
// GetFullPath so the comparison sees the same shape as the roots.
return Path.GetFullPath(name);
}
// Not a link. Its directory chain still might be, so resolve that.
var directory = Path.GetDirectoryName(resolved);
if (string.IsNullOrEmpty(directory))
return resolved;
var realDirectory = ResolveDirectoryChain(directory);
return Path.GetFullPath(Path.Combine(realDirectory, Path.GetFileName(resolved)));
}
// Walk up the directory chain resolving any junction or symlink on it. A
// junction is usually placed on a DIRECTORY, not on the executable, so
// resolving only the leaf would miss the case this exists to catch.
private static string ResolveDirectoryChain(string directory)
{
var resolvedDirectory = System.IO.Directory
.ResolveLinkTarget(directory, returnFinalTarget: true)?.FullName;
if (!string.IsNullOrWhiteSpace(resolvedDirectory))
return resolvedDirectory;
var parent = Path.GetDirectoryName(directory);
if (string.IsNullOrEmpty(parent) || parent == directory)
return directory;
var realParent = ResolveDirectoryChain(parent);
return realParent == parent
? directory
: Path.Combine(realParent, Path.GetFileName(directory));
}
private bool IsUnderApprovedRoot(string resolved) private bool IsUnderApprovedRoot(string resolved)
{ {
foreach (var root in approvedRoots) foreach (var root in approvedRoots)

View File

@@ -27,7 +27,7 @@ with `flask plugin upgrade-all`. Both are needed on a deploy.
| backups | `backups0003clearlastseen` | | backups | `backups0003clearlastseen` |
| computers | `computers0004policies` | | computers | `computers0004policies` |
| employees | `employees0002photo` | | employees | `employees0002photo` |
| endpoints | `endpoints0009opreports` | | endpoints | `endpoints0010renamepolicy` |
| geenforce | `geenforce0004minlib` | | geenforce | `geenforce0004minlib` |
| guides | `guides0001baseline` | | guides | `guides0001baseline` |
| knowledgebase | `knowledgebase0001anchor` | | knowledgebase | `knowledgebase0001anchor` |
@@ -90,6 +90,6 @@ Manifest-less directories under `plugins/` are core frontend surface and always
## Size ## Size
- test functions defined: **1619** (parametrised cases collect higher) - test functions defined: **1624** (parametrised cases collect higher)
- documented API paths: **353** (`docs/openapi.json`, regenerate with `scripts/gen_openapi.py`) - documented API paths: **353** (`docs/openapi.json`, regenerate with `scripts/gen_openapi.py`)

View File

@@ -21,16 +21,30 @@ from flask_jwt_extended import current_user
from shopdb.api import ( from shopdb.api import (
db, success_response, error_response, ErrorCodes, require_permission, db, success_response, error_response, ErrorCodes, require_permission,
require_role, require_role, Setting,
) )
from ..models import EndpointMonitor from ..models import EndpointMonitor
from .routes import endpoints_bp from .routes import endpoints_bp
# Shipped monitors default to this PC type until they are authored as policies. # Which PC type the shipped monitors attach to, as a SETTING rather than a
# literal (ADR-015). PC-type vocabulary is per-site seed data: a plant whose
# type is called "Shop Floor Workstation" got no shipped monitors at all when
# this was hardcoded, and a plant that happens to use the name for something
# else got all of them. The default matches the seeded type so nothing changes
# for a site that uses it; a blank setting attaches the shipped bundle to
# nothing, which is the right neutral behaviour for a site that does not want
# it rather than a silent mismatch.
SHIPPED_PCTYPE_SETTING = 'plugin.endpoints.shippedmonitorpctype'
SHIPPED_DEFAULT_PCTYPE = 'Shopfloor PC' SHIPPED_DEFAULT_PCTYPE = 'Shopfloor PC'
def shipped_pctype():
"""The PC type the shipped monitor bundle attaches to at this site."""
configured = Setting.get(SHIPPED_PCTYPE_SETTING, default=SHIPPED_DEFAULT_PCTYPE)
return (configured or '').strip()
_VALID_KINDS = ('process', 'service') _VALID_KINDS = ('process', 'service')
@@ -93,9 +107,6 @@ def update_monitor(monitor_id):
if monitor is None: if monitor is None:
return error_response(ErrorCodes.NOT_FOUND, 'Monitor not found', return error_response(ErrorCodes.NOT_FOUND, 'Monitor not found',
http_code=404) http_code=404)
if monitor.source != 'local':
return error_response(ErrorCodes.FORBIDDEN,
'Shipped monitors cannot be edited', http_code=403)
body = request.get_json(silent=True) or {} body = request.get_json(silent=True) or {}
kind = (body.get('kind') or monitor.kind or '').strip().lower() kind = (body.get('kind') or monitor.kind or '').strip().lower()
@@ -110,8 +121,27 @@ def update_monitor(monitor_id):
if problem: if problem:
return error_response(ErrorCodes.VALIDATION_ERROR, problem, http_code=400) return error_response(ErrorCodes.VALIDATION_ERROR, problem, http_code=400)
monitor.kind = kind # A shipped row is product-owned IDENTITY, not a product-owned opinion about
monitor.displayname = (body.get('displayname', monitor.displayname) or '').strip() # one site's disk layout. It used to refuse every edit, which meant a plant
# whose eDNC lives on D:, or that runs no UDC at all, could neither correct
# the launch path nor switch the monitor off - and then got permanent false
# drift warnings for it. ADR-015 allows seed data precisely because a site
# can edit it afterwards.
#
# So: kind and displayname are the identity 0007's policy bundle matches on
# and stay fixed on a shipped row; the paths and the active flag, which are
# what actually differ between plants, are editable.
if monitor.source != 'local':
for field in ('kind', 'displayname'):
if field in body and (body.get(field) or '').strip() != (
getattr(monitor, field) or ''):
return error_response(
ErrorCodes.FORBIDDEN,
'A shipped monitor keeps its kind and display name; its '
'paths and active flag can be changed', http_code=403)
else:
monitor.kind = kind
monitor.displayname = (body.get('displayname', monitor.displayname) or '').strip()
monitor.processname = (body.get('processname', monitor.processname) or '').strip() or None monitor.processname = (body.get('processname', monitor.processname) or '').strip() or None
monitor.servicename = (body.get('servicename', monitor.servicename) or '').strip() or None monitor.servicename = (body.get('servicename', monitor.servicename) or '').strip() or None
monitor.launchpath = (body.get('launchpath', monitor.launchpath) or '').strip() or None monitor.launchpath = (body.get('launchpath', monitor.launchpath) or '').strip() or None
@@ -142,13 +172,14 @@ def delete_monitor(monitor_id):
def _resolve_pctype_monitors(pctype): def _resolve_pctype_monitors(pctype):
"""Monitor definitions a PC type should watch: shipped defaults + policy. """Monitor definitions a PC type should watch: shipped defaults + policy.
Shipped monitors are attached to SHIPPED_DEFAULT_PCTYPE as a code rule; Shipped monitors attach to the PC type named by the shipped-pctype setting;
policy-attached monitors come from the computers policy resolver by PC type. policy-attached monitors come from the computers policy resolver by PC type.
Returns a list of EndpointMonitor rows (active only), de-duped by id. Returns a list of EndpointMonitor rows (active only), de-duped by id.
""" """
selected = {} selected = {}
if pctype == SHIPPED_DEFAULT_PCTYPE: shipped_for = shipped_pctype()
if shipped_for and pctype == shipped_for:
for monitor in EndpointMonitor.query.filter_by( for monitor in EndpointMonitor.query.filter_by(
source='shipped', isactive=True).all(): source='shipped', isactive=True).all():
selected[monitor.endpointmonitorid] = monitor selected[monitor.endpointmonitorid] = monitor

View File

@@ -20,17 +20,27 @@ CRUD API refuses to edit or delete them):
trying to START the process when a launch path is mapped - never something trying to START the process when a launch path is mapped - never something
the product ships switched on for every endpoint. the product ships switched on for every endpoint.
BusinessPolicy "RPCA Shopfloor App Monitors" BusinessPolicy "Shopfloor App Monitors"
the site-level wrapper, carrying the Configuration Policy as its one item. the site-level wrapper, carrying the Configuration Policy as its one item.
It was named for the originating plant when this arrived. A shipped row is
product-owned and every adopting site sees it, so the name cannot be one
site's designator (ADR-015). Migration 0010 renames it on installs that
already stamped this one.
Monitors are looked up by identity (kind, displayname), never by a hardcoded id, Monitors are looked up by identity (kind, displayname), never by a hardcoded id,
so the bundle survives id drift across installs. so the bundle survives id drift across installs.
Lives in the endpoints chain, after 0006, because the monitor rows it references Lives in the endpoints chain, after 0006, because the monitor rows it references
are seeded there. The computers policy tables are created by the core baseline are seeded there. The policy tables belong to the COMPUTERS plugin and are
before per-plugin chains run; this migration still guards on their presence and created by its own post-cutover chain, not by the core baseline; endpoints
no-ops cleanly if they are absent, and is idempotent on the policy/business-policy declares a hard dependency on computers, which is what orders the two.
names so re-running never duplicates the bundle.
It guards on those tables being present and no-ops if they are not, and it is
idempotent on the policy names so re-running never duplicates the bundle. Be
clear about what the guard costs, though: Alembic stamps the revision either
way, so if it ever no-ops the bundle is simply never seeded and nothing comes
back to finish the job. That is acceptable only because the dependency makes
the absent case unreachable in a supported install.
""" """
from alembic import op from alembic import op
@@ -47,7 +57,7 @@ depends_on = None
CONFIG_POLICY_NAME = 'Shopfloor App Monitors' CONFIG_POLICY_NAME = 'Shopfloor App Monitors'
BUSINESS_POLICY_NAME = 'RPCA Shopfloor App Monitors' BUSINESS_POLICY_NAME = 'Shopfloor App Monitors'
# The shipped monitors this bundle attaches, by identity (kind, displayname). # The shipped monitors this bundle attaches, by identity (kind, displayname).
_SHIPPED_MONITORS = ( _SHIPPED_MONITORS = (

View File

@@ -0,0 +1,60 @@
"""Rename the shipped business policy off one site's designator.
0007 seeded it as "RPCA Shopfloor App Monitors". RPCA is the originating
plant's facility designator, and the row is product-owned: source='shipped',
seeded into every install, and the CRUD API refuses to rename it. So every
adopting site would carry another plant's label at the top of its policy list
with no way to change it, which is exactly what ADR-015 exists to prevent.
0007 now seeds the neutral name, which covers a fresh install. This migration
covers the installs that already stamped 0007 with the old one - the
originating site, and anyone who upgraded before this landed.
Renames only when the old name is present AND the new one is not, so it is
idempotent and cannot collide with a site that already has a policy of its own
under the new name. Downgrade is deliberately one-way: putting another site's
designator back into a shipped row is not a state worth being able to reach.
Revision ID: endpoints0010renamepolicy
Revises: endpoints0009opreports
"""
from alembic import op
import sqlalchemy as sa
revision = 'endpoints0010renamepolicy'
down_revision = 'endpoints0009opreports'
branch_labels = None
depends_on = None
OLD_NAME = 'RPCA Shopfloor App Monitors'
NEW_NAME = 'Shopfloor App Monitors'
def _has_table(bind, name):
return sa.inspect(bind).has_table(name)
def upgrade():
bind = op.get_bind()
# The policy tables belong to the computers plugin. Same guard as 0007: if
# they are not there, there is nothing seeded to rename.
if not _has_table(bind, 'businesspolicies'):
return
exists = bind.execute(
sa.text('SELECT COUNT(*) FROM businesspolicies WHERE name = :name'),
{'name': NEW_NAME}).scalar()
if exists:
return
bind.execute(
sa.text('UPDATE businesspolicies SET name = :new WHERE name = :old'),
{'new': NEW_NAME, 'old': OLD_NAME})
def downgrade():
# One-way on purpose. See the module docstring.
pass

View File

@@ -97,7 +97,9 @@ EXPECTED_HEAD_REVISION['warranty'] = 'warranty0002proof'
# seeds the reusable monitor catalog, 0007 ships the shopfloor monitor policy # seeds the reusable monitor catalog, 0007 ships the shopfloor monitor policy
# bundle, 0008 adds the warn-only reconciler drift cache column, and 0009 adds # bundle, 0008 adds the warn-only reconciler drift cache column, and 0009 adds
# the operatorreports table for tray issue reports. # the operatorreports table for tray issue reports.
EXPECTED_HEAD_REVISION['endpoints'] = 'endpoints0009opreports' # 0010 renames the shipped business policy off the originating site's
# designator on installs that already stamped 0007 (ADR-015).
EXPECTED_HEAD_REVISION['endpoints'] = 'endpoints0010renamepolicy'
# Plugins built after the cutover: their 0001 baseline really creates tables the # Plugins built after the cutover: their 0001 baseline really creates tables the
# core chain never owned. # core chain never owned.

View File

@@ -197,3 +197,86 @@ def test_resolved_monitors_unknown_hostname_is_404(client, db, app):
response = client.get(RESOLVED_URL, query_string={'hostname': 'NOSUCHBOX'}, response = client.get(RESOLVED_URL, query_string={'hostname': 'NOSUCHBOX'},
headers=_collector_headers(app)) headers=_collector_headers(app))
assert response.status_code == 404 assert response.status_code == 404
# --- shipped rows are product-owned IDENTITY, not a fixed opinion about disks --
#
# The shipped bundle used to be uneditable in every field and attached to a
# hardcoded PC-type name. A plant whose eDNC sits on another drive, or whose
# type is called something else, could neither correct it nor turn it off, and
# then carried permanent false drift for it. ADR-015 allows seed data because a
# site can edit it afterwards.
def _shipped(db, displayname='eDNC', **kwargs):
from plugins.endpoints.models import EndpointMonitor
monitor = EndpointMonitor(
kind=kwargs.pop('kind', 'process'),
displayname=displayname,
processname=kwargs.pop('processname', 'DncMain'),
launchpath=kwargs.pop('launchpath', r'C:\Program Files\Dnc\bin\DncMain.exe'),
source='shipped', isactive=True, **kwargs)
db.session.add(monitor)
db.session.flush()
return monitor
def test_a_site_can_correct_a_shipped_monitors_launch_path(
client, db, auth_headers):
monitor = _shipped(db)
response = client.put(
'/api/endpoints/monitors/%d' % monitor.endpointmonitorid,
json={'launchpath': r'D:\Dnc\bin\DncMain.exe'}, headers=auth_headers)
assert response.status_code == 200
assert response.get_json()['data']['launchpath'] == r'D:\Dnc\bin\DncMain.exe'
def test_a_site_can_switch_off_a_shipped_monitor_it_does_not_run(
client, db, auth_headers):
monitor = _shipped(db, displayname='Universal Data Collector',
processname='UDC')
response = client.put(
'/api/endpoints/monitors/%d' % monitor.endpointmonitorid,
json={'isactive': False}, headers=auth_headers)
assert response.status_code == 200
assert response.get_json()['data']['isactive'] is False
def test_a_shipped_monitor_keeps_the_identity_the_policy_matches_on(
client, db, auth_headers):
"""0007 attaches its bundle by (kind, displayname), so those stay put."""
monitor = _shipped(db)
response = client.put(
'/api/endpoints/monitors/%d' % monitor.endpointmonitorid,
json={'displayname': 'Renamed'}, headers=auth_headers)
assert response.status_code == 403
assert monitor.displayname == 'eDNC'
# --- which PC type the shipped bundle attaches to is a setting ---------------
def test_shipped_monitors_follow_the_configured_pctype(client, db, app):
from plugins.endpoints.api.monitors import (
SHIPPED_PCTYPE_SETTING, _resolve_pctype_monitors)
_shipped(db)
Setting.set(SHIPPED_PCTYPE_SETTING, 'Shop Floor Workstation')
db.session.commit()
# The plant's own name for the type resolves the bundle...
assert [m.displayname for m in
_resolve_pctype_monitors('Shop Floor Workstation')] == ['eDNC']
# ...and the name this shipped with no longer does.
assert _resolve_pctype_monitors('Shopfloor PC') == []
def test_a_blank_shipped_pctype_attaches_the_bundle_to_nothing(client, db, app):
"""The neutral behaviour for a site that does not want the bundle: off,
rather than silently attached to whatever happens to match."""
from plugins.endpoints.api.monitors import (
SHIPPED_PCTYPE_SETTING, _resolve_pctype_monitors)
_shipped(db)
Setting.set(SHIPPED_PCTYPE_SETTING, '')
db.session.commit()
assert _resolve_pctype_monitors('Shopfloor PC') == []

View File

@@ -4,7 +4,7 @@ Migration endpoints0007shippedpolicy offloads the eDNC/UDC/MTConnect monitors
from the agent's hardcoded appsettings.json into a product-owned policy bundle: from the agent's hardcoded appsettings.json into a product-owned policy bundle:
a shipped ConfigurationPolicy "Shopfloor App Monitors" (global scope, one a shipped ConfigurationPolicy "Shopfloor App Monitors" (global scope, one
desired-state monitor action per shipped monitor) wrapped in a shipped desired-state monitor action per shipped monitor) wrapped in a shipped
BusinessPolicy "RPCA Shopfloor App Monitors". BusinessPolicy "Shopfloor App Monitors".
Run the endpoints chain against a fresh engine, alongside the computers policy Run the endpoints chain against a fresh engine, alongside the computers policy
tables the bundle writes into, and assert the bundle exists, is source='shipped' tables the bundle writes into, and assert the bundle exists, is source='shipped'
@@ -25,7 +25,7 @@ _ENDPOINTS_VERSIONS = _ROOT / 'plugins' / 'endpoints' / 'migrations' / 'versions
_COMPUTERS_VERSIONS = _ROOT / 'plugins' / 'computers' / 'migrations' / 'versions' _COMPUTERS_VERSIONS = _ROOT / 'plugins' / 'computers' / 'migrations' / 'versions'
CONFIG_POLICY_NAME = 'Shopfloor App Monitors' CONFIG_POLICY_NAME = 'Shopfloor App Monitors'
BUSINESS_POLICY_NAME = 'RPCA Shopfloor App Monitors' BUSINESS_POLICY_NAME = 'Shopfloor App Monitors'
def _load(path, prefix): def _load(path, prefix):