From fd180991991382ef366522330b5ec7cc4f5055f8 Mon Sep 17 00:00:00 2001 From: "benjamin.baker" Date: Wed, 9 Sep 2026 06:37:02 -0400 Subject: [PATCH] Gate the script library on the grant that pushes scripts The script library was gated on applications.*, so anyone holding the software-delivery grant could rewrite the bytes of a script that a separately trusted operator later pushed to a bay. The push was gated on endpoints.runscript; the CONTENT of what got pushed was not. An installer is a vendor artefact, but a script here is arbitrary PowerShell this site wrote that the agent runs as SYSTEM, so the write and the push must require the same trust. All six script routes now require endpoints.runscript, including the body download - reading the bytes is not a lesser act than listing them. Core names the permission as a string rather than importing it from the endpoints plugin so the module stays importable with the plugin absent. This tightens access and there is deliberately no backfill: the operator scope reaches applications.create/edit, so operator-scope holders lose the library. Granting them endpoints.runscript on upgrade would undo the change and hand every operator remote SYSTEM code execution, since the same grant gates the push. A test pins that no prebuilt scope confers it. Adds the System Scripts settings page and card. The route requires the permission rather than admin, because the grant is meant for non-admin operators and the sibling admin gate would lock them out of the library the grant exists for. The settings icon map gains terminal and key; the agent-key card had been asking for key since it shipped and silently drawing a puzzle piece. The library had no test coverage before this. --- frontend/src/api/index.js | 7 +- frontend/src/composables/settingsCatalog.js | 6 +- plugins/endpoints/frontend/routes.js | 14 + .../frontend/views/SystemScriptsSettings.vue | 252 ++++++++++++++++++ .../views/systemScriptsSettings.spec.js | 50 ++++ plugins/endpoints/plugin.py | 15 ++ shopdb/core/api/scripts.py | 44 ++- tests/test_core/test_script_library_gate.py | 160 +++++++++++ 8 files changed, 533 insertions(+), 15 deletions(-) create mode 100644 plugins/endpoints/frontend/views/SystemScriptsSettings.vue create mode 100644 plugins/endpoints/frontend/views/systemScriptsSettings.spec.js create mode 100644 tests/test_core/test_script_library_gate.py diff --git a/frontend/src/api/index.js b/frontend/src/api/index.js index 29c39600..30fe4c8a 100644 --- a/frontend/src/api/index.js +++ b/frontend/src/api/index.js @@ -827,8 +827,11 @@ export const applicationsApi = { } // ShopDB-hosted script library. Backs run_script delivery: the agent fetches a -// script body by digest with its collector credential and runs it in the -// script's configured run context (system or user). +// script body by digest with its collector credential and runs it as SYSTEM. +// The UI does not offer a run context - scripts exist to trigger an already +// registered scheduled task, and the agent service has no user session to +// borrow. Anything needing a user's own token must be that scheduled task's +// problem, not this library's. export const scriptsApi = { list(params = {}) { return api.get('/scripts', { params }) diff --git a/frontend/src/composables/settingsCatalog.js b/frontend/src/composables/settingsCatalog.js index 0634c7a4..eae99a18 100644 --- a/frontend/src/composables/settingsCatalog.js +++ b/frontend/src/composables/settingsCatalog.js @@ -4,7 +4,7 @@ // (landing) read the merged catalog so plugins add settings cards with no core // edit. Fetched once into a module-level ref and shared across callers. import { ref, computed } from 'vue' -import { Ruler, Wrench, Cog, Package, Settings, Puzzle, SlidersHorizontal, Palette, Bell, Network, Printer, Droplets } from 'lucide-vue-next' +import { Ruler, Wrench, Cog, Package, Settings, Puzzle, SlidersHorizontal, Palette, Bell, Network, Printer, Droplets, Terminal, KeyRound } from 'lucide-vue-next' import api from '../api' import router from '../router' import { settingsGroups } from '../views/settings/settingsNav' @@ -24,6 +24,10 @@ const iconMap = { network: Network, printer: Printer, droplets: Droplets, + terminal: Terminal, + // The endpoints agent-key card has asked for 'key' since it shipped and has + // been silently drawing a puzzle piece the whole time. + key: KeyRound, } function resolveIcon(key) { diff --git a/plugins/endpoints/frontend/routes.js b/plugins/endpoints/frontend/routes.js index 93119886..f573103e 100644 --- a/plugins/endpoints/frontend/routes.js +++ b/plugins/endpoints/frontend/routes.js @@ -23,6 +23,20 @@ export default [ name: 'endpoints-monitors-settings', component: () => import('./views/MonitorsSettings.vue'), meta: { requiresAuth: true, requiresAdmin: true, plugin: 'endpoints' } + }, + // requiresPermission, not requiresAdmin: endpoints.runscript is grantable to + // non-admin operators, and the sibling pages' admin gate would lock the + // holders of the grant out of the library they are trusted to manage. The + // API enforces the same permission, so this only keeps the menu honest. + { + path: 'settings/endpoints/scripts', + name: 'endpoints-scripts-settings', + component: () => import('./views/SystemScriptsSettings.vue'), + meta: { + requiresAuth: true, + requiresPermission: 'endpoints.runscript', + plugin: 'endpoints' + } } ] diff --git a/plugins/endpoints/frontend/views/SystemScriptsSettings.vue b/plugins/endpoints/frontend/views/SystemScriptsSettings.vue new file mode 100644 index 00000000..33ef0cba --- /dev/null +++ b/plugins/endpoints/frontend/views/SystemScriptsSettings.vue @@ -0,0 +1,252 @@ + + + + + diff --git a/plugins/endpoints/frontend/views/systemScriptsSettings.spec.js b/plugins/endpoints/frontend/views/systemScriptsSettings.spec.js new file mode 100644 index 00000000..38ce5877 --- /dev/null +++ b/plugins/endpoints/frontend/views/systemScriptsSettings.spec.js @@ -0,0 +1,50 @@ +/** + * System Scripts settings page. + * + * Two things are worth pinning here and nothing else is: + * + * 1. The page must NOT offer a run context. Scripts run as SYSTEM because the + * agent is a service with no user session; exposing a picker would let + * someone author a script that reads as "run as the logged-on user" and + * silently does not. + * 2. The SYSTEM warning must be present on both the list and the add/edit + * form. Someone pasting a script into the modal may never have read the + * page copy above it. + * + * Asserted against the file text, matching configurationPolicyEditable.spec.js. + */ +import { describe, it, expect } from 'vitest' +import { readFileSync } from 'fs' +import { join, dirname } from 'path' +import { fileURLToPath } from 'url' + +const here = dirname(fileURLToPath(import.meta.url)) +const source = readFileSync(join(here, 'SystemScriptsSettings.vue'), 'utf8') +const routes = readFileSync(join(here, '..', 'routes.js'), 'utf8') + +describe('SystemScriptsSettings', () => { + it('warns that scripts run as SYSTEM on the list page and in the form', () => { + const warnings = source.match(/runs? as SYSTEM/g) || [] + expect(warnings.length).toBeGreaterThanOrEqual(2) + }) + + it('does not offer a run context picker', () => { + expect(source).not.toMatch(/runcontext/i) + }) + + it('does not send a runcontext in its save payload', () => { + const payload = source.slice(source.indexOf('const payload = {')) + expect(payload.slice(0, 200)).not.toMatch(/runcontext/i) + }) + + it('flags a script with no body as unrunnable', () => { + expect(source).toMatch(/hasbody/) + expect(source).toMatch(/cannot run/i) + }) + + it('is gated on the standalone script permission, not admin', () => { + const route = routes.slice(routes.indexOf("'settings/endpoints/scripts'")) + expect(route).toMatch(/requiresPermission: 'endpoints\.runscript'/) + expect(route.slice(0, 300)).not.toMatch(/requiresAdmin/) + }) +}) diff --git a/plugins/endpoints/plugin.py b/plugins/endpoints/plugin.py index 5c69e98c..aa05540f 100644 --- a/plugins/endpoints/plugin.py +++ b/plugins/endpoints/plugin.py @@ -320,6 +320,21 @@ class EndpointsPlugin(BasePlugin): 'agent, and configure what it reports.', 'position': 27, }, + # Its own card rather than a section of the agent key page: the + # audience differs. Key management is admin-only, while the script + # library is reachable by any endpoints.runscript holder, and + # burying it under an admin page would hide it from exactly the + # operators the grant exists for. + { + 'group': 'Endpoints', + 'to': '/settings/endpoints/scripts', + 'icon': 'terminal', + 'title': 'System Scripts', + 'description': 'PowerShell the agent can run on an endpoint. ' + 'Runs as SYSTEM; used to trigger scheduled ' + 'tasks already registered on the endpoint.', + 'position': 28, + }, ] def get_navigation_items(self) -> List[Dict]: diff --git a/shopdb/core/api/scripts.py b/shopdb/core/api/scripts.py index 8b606d41..2c7fc903 100644 --- a/shopdb/core/api/scripts.py +++ b/shopdb/core/api/scripts.py @@ -6,8 +6,23 @@ its body by digest on push (the same model as application installer delivery). The body never crosses the command wire and is never pre-staged on the endpoint; the agent downloads it with its collector credential and verifies sha256. -Gated on the applications.* permissions - scripts are software delivery like -apps, so they ride the same operator grant rather than a separate permission. +Gated on endpoints.runscript, the same standalone grant that authorizes pushing +a script. These are NOT applications: an application installer is a vendor +artefact, while a script here is arbitrary PowerShell this site wrote that the +agent runs as SYSTEM. Gating library management on applications.* meant anyone +with the software-delivery grant could rewrite the bytes of a script a trusted +operator later pushes, which is a privilege escalation wearing a content- +management hat. The write and the push now require the same trust. + +This TIGHTENS access, and there is deliberately NO backfill migration. The +operator scope grants applications.create/edit (they are edit verbs), so +operator-scope holders could reach this library before and cannot now. Do not +"fix" that by granting endpoints.runscript to applications.edit holders on +upgrade: it is in SCOPE_NEVER_GRANT precisely because it is a per-user checkbox +decision, and the same grant also authorizes the remote PUSH, so a backfill +would hand every operator SYSTEM code execution on the fleet. Admins are +unaffected via the role bypass; anyone else who genuinely needs the library +gets the checkbox in the user editor. """ import os @@ -32,6 +47,11 @@ SCRIPT_EXTENSIONS = {'.ps1'} SCRIPT_URL_PREFIX = '/api/scripts/body/' # A delivery script is small; cap well under the installer limit. MAX_SCRIPT_BYTES = 1 * 1024 * 1024 +# Owned by the endpoints plugin, which is where the risk is described in full +# ("ARBITRARY CODE, executes as SYSTEM"). Core names it as a string rather than +# importing it so this module stays importable with the plugin absent - the +# grant simply cannot be held by anyone if nothing seeds it. +SCRIPT_PERMISSION = 'endpoints.runscript' def _script_dir(): @@ -42,7 +62,7 @@ scripts_bp = Blueprint('scripts', __name__) @scripts_bp.route('', methods=['GET']) -@require_permission('applications.view') +@require_permission(SCRIPT_PERMISSION) def list_scripts(): """List scripts (active by default).""" query = Script.query @@ -55,7 +75,7 @@ def list_scripts(): @scripts_bp.route('/', methods=['GET']) -@require_permission('applications.view') +@require_permission(SCRIPT_PERMISSION) def get_script(script_id: int): script = db.session.get(Script, script_id) if not script: @@ -74,7 +94,7 @@ def _validate_runcontext(value): @scripts_bp.route('', methods=['POST']) -@require_permission('applications.create') +@require_permission(SCRIPT_PERMISSION) def create_script(): """Create a script catalog entry (body uploaded separately).""" data = request.get_json(silent=True) or {} @@ -103,7 +123,7 @@ def create_script(): @scripts_bp.route('/', methods=['PUT']) -@require_permission('applications.edit') +@require_permission(SCRIPT_PERMISSION) def update_script(script_id: int): script = db.session.get(Script, script_id) if not script: @@ -135,7 +155,7 @@ def update_script(script_id: int): @scripts_bp.route('/', methods=['DELETE']) -@require_permission('applications.delete') +@require_permission(SCRIPT_PERMISSION) def delete_script(script_id: int): script = db.session.get(Script, script_id) if not script: @@ -149,7 +169,7 @@ def delete_script(script_id: int): @scripts_bp.route('//body', methods=['POST']) -@require_permission('applications.edit') +@require_permission(SCRIPT_PERMISSION) def upload_script_body(script_id: int): """Upload (or replace) the .ps1 body for a script. @@ -196,15 +216,15 @@ def upload_script_body(script_id: int): @scripts_bp.route('/body/', methods=['GET']) -@require_permission('applications.download') +@require_permission(SCRIPT_PERMISSION) def download_script_body(filename: str): """Operator-facing script body download (JWT/RBAC). The endpoint agent uses the collector-authenticated plugin route instead; this one is for humans reviewing what a script does. - Gated on applications.download, not applications.view: this serves the - SYSTEM-context PowerShell a site pushes to its bays, and a guest may be - granted the metadata permission. + Same grant as the rest of the library: this serves the SYSTEM-context + PowerShell a site pushes to its bays, so reading it is not a lesser act + than listing it. """ return send_upload(_script_dir(), filename, as_attachment=True) diff --git a/tests/test_core/test_script_library_gate.py b/tests/test_core/test_script_library_gate.py new file mode 100644 index 00000000..e65e4e3e --- /dev/null +++ b/tests/test_core/test_script_library_gate.py @@ -0,0 +1,160 @@ +"""The script library is gated on its own grant, not on applications.*. + +A script here is not an application. An installer is a vendor artefact; a script +is arbitrary PowerShell this site wrote that the agent runs as SYSTEM. While the +library was gated on applications.*, anyone holding the software-delivery grant +could rewrite the bytes of a script a trusted operator later pushed - the push +was gated on endpoints.runscript, but the CONTENT of what got pushed was not. +That is privilege escalation wearing a content-management hat. + +These tests pin the gate itself. The library had NO coverage before this. +""" +import io + +import pytest +from werkzeug.security import generate_password_hash + +from shopdb.core.models import Permission + + +APPLICATIONS_GRANTS = [ + ('applications.view', 'View applications', 'applications'), + ('applications.create', 'Create applications', 'applications'), + ('applications.edit', 'Edit applications', 'applications'), + ('applications.delete', 'Delete applications', 'applications'), + ('applications.download', 'Download installers', 'applications'), +] +SCRIPT_GRANT = ('endpoints.runscript', 'Run staged scripts', 'endpoints') + + +@pytest.fixture +def seeded(db): + Permission.seed_entries(APPLICATIONS_GRANTS + [SCRIPT_GRANT]) + db.session.commit() + + +def _user_with(client, db, username, grants): + """Create a user holding exactly the named permissions, return its headers. + + Direct permissions, no role: a role named 'admin' would short-circuit the + gate and prove nothing about the permission under test. + """ + from shopdb.core.models import Permission, User + + user = User( + username=username, + email=f'{username}@test.local', + passwordhash=generate_password_hash('testpass'), + ) + for name in grants: + permission = Permission.query.filter_by(name=name).one() + user.directpermissions.append(permission) + db.session.add(user) + db.session.commit() + + login = client.post('/api/auth/login', + json={'username': username, 'password': 'testpass'}) + assert login.status_code == 200, login.get_json() + token = login.get_json()['data']['access_token'] + return {'Authorization': f'Bearer {token}'} + + +def test_the_applications_grant_does_not_open_the_script_library( + client, db, seeded): + """The whole point: every applications.* grant, and still no access.""" + headers = _user_with(client, db, 'appsonly', + [name for name, _d, _c in APPLICATIONS_GRANTS]) + + assert client.get('/api/scripts', headers=headers).status_code == 403 + assert client.post('/api/scripts', headers=headers, + json={'scriptname': 'x'}).status_code == 403 + + +def test_the_runscript_grant_opens_the_script_library(client, db, seeded): + headers = _user_with(client, db, 'pusher', ['endpoints.runscript']) + + assert client.get('/api/scripts', headers=headers).status_code == 200 + created = client.post('/api/scripts', headers=headers, + json={'scriptname': 'map-drives'}) + assert created.status_code == 201, created.get_json() + + +def test_writing_a_script_body_needs_the_same_grant(client, db, seeded): + """Uploading the bytes is the dangerous half; it must not be cheaper to + reach than the push that runs them.""" + author = _user_with(client, db, 'author', ['endpoints.runscript']) + created = client.post('/api/scripts', headers=author, + json={'scriptname': 'payload'}) + scriptid = created.get_json()['data']['scriptid'] + + appsuser = _user_with(client, db, 'appsuser2', + [name for name, _d, _c in APPLICATIONS_GRANTS]) + body = (io.BytesIO(b"Write-Output 'hi'\r\n"), 'payload.ps1') + denied = client.post(f'/api/scripts/{scriptid}/body', headers=appsuser, + data={'file': body}, + content_type='multipart/form-data') + assert denied.status_code == 403 + + +def test_reading_a_script_body_needs_the_same_grant(client, db, seeded): + """Reading is not a lesser act: the body is the SYSTEM-context PowerShell + itself, so it must not be readable by a weaker grant than writing it.""" + appsuser = _user_with(client, db, 'appsuser3', + [name for name, _d, _c in APPLICATIONS_GRANTS]) + response = client.get('/api/scripts/body/script-1.ps1', headers=appsuser) + assert response.status_code == 403 + + +def test_upload_pins_the_digest_of_the_stored_bytes(client, db, seeded): + """The agent refuses a body whose sha256 does not match, so a wrong digest + here is a script that can never run. Untested until now.""" + import hashlib + + headers = _user_with(client, db, 'digest', ['endpoints.runscript']) + created = client.post('/api/scripts', headers=headers, + json={'scriptname': 'digest-check'}) + scriptid = created.get_json()['data']['scriptid'] + + payload = b"Write-Output 'pinned'\r\n" + uploaded = client.post( + f'/api/scripts/{scriptid}/body', headers=headers, + data={'file': (io.BytesIO(payload), 'digest-check.ps1')}, + content_type='multipart/form-data') + assert uploaded.status_code == 200, uploaded.get_json() + + data = uploaded.get_json()['data'] + assert data['sha256'] == hashlib.sha256(payload).hexdigest() + assert data['hasbody'] is True + + +def test_a_duplicate_script_name_is_refused(client, db, seeded): + """The accidental-overwrite guard the library was specified to have.""" + headers = _user_with(client, db, 'dupes', ['endpoints.runscript']) + first = client.post('/api/scripts', headers=headers, + json={'scriptname': 'same-name'}) + assert first.status_code == 201 + second = client.post('/api/scripts', headers=headers, + json={'scriptname': 'same-name'}) + assert second.status_code == 400 + assert 'already exists' in second.get_json()['data']['error']['message'] + + +def test_no_prebuilt_scope_confers_the_script_permission(): + """Pins the decision NOT to backfill the tightened gate. + + Moving the library onto endpoints.runscript takes it away from operator + scope holders, who reach applications.create/edit. The tempting upgrade fix + is to grant endpoints.runscript to whoever holds applications.edit - that + would undo the whole change AND hand every operator remote SYSTEM code + execution, because the same grant gates the push. If this test fails, + someone has widened a scope; do not relax it. + """ + from shopdb.core.models.user import ( + SCOPE_NEVER_GRANT, + scope_permission_names, + ) + from shopdb.core.api.scripts import SCRIPT_PERMISSION + + assert SCRIPT_PERMISSION in SCOPE_NEVER_GRANT + for scope in ('reporter', 'technician', 'operator'): + assert SCRIPT_PERMISSION not in scope_permission_names(scope)