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)