Stop scripts reporting success after doing nothing

A run_script command was judged solely on PowerShell's exit code, and
PowerShell treats most errors as non-terminating: it writes the error,
runs the next line, and still exits zero. The account-provisioning
script reported succeeded having provisioned nothing, and because no
output was captured there was no evidence to contradict the verdict.

Scripts now carry an errormode. Strict prepends $ErrorActionPreference
= 'Stop' so the first error ends the run and the exit code means what
it says. It is opt-in per script rather than global because bodies
authored against the permissive semantics rely on them - tolerating a
failing cleanup, probing with a cmdlet that errors when a thing is
absent, looping past a bad item - and would start aborting midway,
leaving endpoints half-configured. The default is unchanged, so every
existing script keeps running exactly as before.

The prologue is applied after digest verification, so the SHA-256 still
validates the bytes the server delivered.

stdout/stderr is captured on both paths: redirected streams for SYSTEM,
a transcript file under the ShopFloor profile for user-session runs,
since CreateProcessAsUser cannot inherit the service's pipes across the
session boundary. Output is always logged locally and returned to the
server only on failure, so routine output stays on the machine that
produced it instead of filling command history.

Also adds the optional per-script working directory, which the same
surfaces needed, and a collapsible explainer on the scripts page - the
dropdown is not self-explanatory, and an operator who does not know
about non-terminating errors has no reason to ever leave the default.

Agent 1.14.0. An older agent ignoring errormode keeps today's behaviour.

tests/test_core/test_script_errormode.py

@@ -0,0 +1,138 @@
+"""The per-script error mode.
+
+PowerShell treats most errors as non-terminating: it writes the error, runs the
+next line, and still exits zero. The agent judges a run by that exit code, so a
+script could report success having accomplished none of its work - which is
+exactly what happened with the account-provisioning script that "succeeded"
+without provisioning anything.
+
+Strict mode fixes the verdict by making the first error terminating. It is
+deliberately NOT the default, because it changes how an existing body behaves
+partway through: a script that tolerates a failing cleanup, probes with a cmdlet
+that errors when something is absent, or loops past bad items would start
+aborting midway and leave the endpoint half-configured. These tests pin the
+permissive default hard, since that is the property protecting every script
+authored before this field existed.
+"""
+import pytest
+from werkzeug.security import generate_password_hash
+
+from shopdb.core.models import Permission
+
+
+SCRIPT_GRANT = ('endpoints.runscript', 'Run staged scripts', 'endpoints')
+
+
+@pytest.fixture
+def headers(client, db):
+    from shopdb.core.models import User
+
+    Permission.seed_entries([SCRIPT_GRANT])
+    db.session.commit()
+
+    user = User(
+        username='modeauthor',
+        email='modeauthor@test.local',
+        passwordhash=generate_password_hash('testpass'),
+    )
+    user.directpermissions.append(
+        Permission.query.filter_by(name='endpoints.runscript').one())
+    db.session.add(user)
+    db.session.commit()
+
+    login = client.post('/api/auth/login',
+                        json={'username': 'modeauthor', 'password': 'testpass'})
+    assert login.status_code == 200, login.get_json()
+    return {'Authorization': f"Bearer {login.get_json()['data']['access_token']}"}
+
+
+def _create(client, headers, name, **extra):
+    payload = {'scriptname': name}
+    payload.update(extra)
+    return client.post('/api/scripts', headers=headers, json=payload)
+
+
+def test_a_script_without_a_mode_stays_permissive(client, headers):
+    """The load-bearing test. Every script written before this field existed
+    was authored against permissive semantics; defaulting to strict would
+    change how all of them run without anyone touching them."""
+    created = _create(client, headers, 'no-mode')
+
+    assert created.status_code == 201, created.get_json()
+    assert created.get_json()['data']['errormode'] == 'default'
+
+
+def test_strict_is_stored_and_returned(client, headers):
+    created = _create(client, headers, 'strict-one', errormode='strict')
+
+    assert created.status_code == 201, created.get_json()
+    assert created.get_json()['data']['errormode'] == 'strict'
+
+
+@pytest.mark.parametrize('bad', ['stop', 'true', 'yes', 'none', ''])
+def test_an_unrecognised_mode_is_rejected(client, headers, bad):
+    """Rejected rather than coerced. Silently falling back to permissive would
+    let a caller believe a script fails loudly when it does not, which is the
+    precise failure this field exists to remove."""
+    created = _create(client, headers, f'bad-{bad or "blank"}', errormode=bad)
+
+    assert created.status_code == 400, created.get_json()
+    assert 'errormode' in created.get_json()['data']['error']['message']
+
+
+def test_casing_is_normalised_rather_than_refused(client, headers):
+    """A caller typing Strict means strict. Case is a keyboard artefact, not a
+    different intention, so it is normalised - unlike the values above, which
+    are genuinely different words and could mean something the author did not
+    intend."""
+    created = _create(client, headers, 'cased', errormode='Strict')
+
+    assert created.status_code == 201, created.get_json()
+    assert created.get_json()['data']['errormode'] == 'strict'
+
+
+def test_a_script_can_be_moved_back_to_permissive(client, headers):
+    """Strict has to be reversible without recreating the script: the way you
+    discover a body cannot tolerate it is by watching it abort in production."""
+    scriptid = _create(client, headers, 'reversible',
+                       errormode='strict').get_json()['data']['scriptid']
+
+    updated = client.put(f'/api/scripts/{scriptid}', headers=headers,
+                         json={'errormode': 'default'})
+
+    assert updated.status_code == 200, updated.get_json()
+    assert updated.get_json()['data']['errormode'] == 'default'
+
+
+def test_the_delivery_descriptor_always_carries_the_field(client, db, headers):
+    """Always present, even at the default, so an agent that received it and
+    ignored it is distinguishable from one that never saw it."""
+    from plugins.endpoints.api.routes import _resolve_run_script
+    from shopdb.core.models.script_library import Script
+
+    script = Script(scriptname='delivered-strict', filename='script-1.ps1',
+                    sha256='a' * 64, errormode='strict')
+    db.session.add(script)
+    db.session.commit()
+
+    descriptor, error = _resolve_run_script(f'script:{script.scriptid}')
+
+    assert error is None, error
+    assert descriptor['errormode'] == 'strict'
+
+
+def test_the_descriptor_defaults_rather_than_omitting(client, db, headers):
+    """An absent key would leave the agent guessing. It sends the permissive
+    value explicitly so both ends agree on what unset means."""
+    from plugins.endpoints.api.routes import _resolve_run_script
+    from shopdb.core.models.script_library import Script
+
+    script = Script(scriptname='delivered-default', filename='script-2.ps1',
+                    sha256='b' * 64)
+    db.session.add(script)
+    db.session.commit()
+
+    descriptor, error = _resolve_run_script(f'script:{script.scriptid}')
+
+    assert error is None, error
+    assert descriptor['errormode'] == 'default'

tests/test_core/test_script_workingdirectory.py

@@ -0,0 +1,149 @@
+"""The optional per-script working directory.
+
+A delivered script runs from the agent's staging directory. That is correct for
+a self-contained script and wrong for one that shells out to a corporate tool
+already installed on the endpoint by Intune and invokes it by relative path -
+sfld_autologon needs Autologon.exe beside it. This field pins the folder.
+
+The validation here is a SHAPE check, not a privilege boundary: the body is
+already arbitrary SYSTEM PowerShell that could Set-Location anywhere. These
+tests pin that distinction so the field is not later mistaken for a security
+control, and pin the default so old scripts keep behaving exactly as before.
+"""
+import pytest
+from werkzeug.security import generate_password_hash
+
+from shopdb.core.models import Permission
+
+
+SCRIPT_GRANT = ('endpoints.runscript', 'Run staged scripts', 'endpoints')
+
+
+@pytest.fixture
+def headers(client, db):
+    from shopdb.core.models import User
+
+    Permission.seed_entries([SCRIPT_GRANT])
+    db.session.commit()
+
+    user = User(
+        username='scriptauthor',
+        email='scriptauthor@test.local',
+        passwordhash=generate_password_hash('testpass'),
+    )
+    user.directpermissions.append(
+        Permission.query.filter_by(name='endpoints.runscript').one())
+    db.session.add(user)
+    db.session.commit()
+
+    login = client.post('/api/auth/login',
+                        json={'username': 'scriptauthor', 'password': 'testpass'})
+    assert login.status_code == 200, login.get_json()
+    return {'Authorization': f"Bearer {login.get_json()['data']['access_token']}"}
+
+
+def _create(client, headers, name, **extra):
+    payload = {'scriptname': name}
+    payload.update(extra)
+    return client.post('/api/scripts', headers=headers, json=payload)
+
+
+def test_a_script_without_a_folder_stays_unpinned(client, headers):
+    """The default must remain "the agent decides", or every pre-existing
+    script silently acquires a folder it was never authored for."""
+    created = _create(client, headers, 'no-folder')
+
+    assert created.status_code == 201, created.get_json()
+    assert not created.get_json()['data']['workingdirectory']
+
+
+def test_a_folder_is_stored_and_returned(client, headers):
+    created = _create(client, headers, 'autologon',
+                      workingdirectory=r'C:\Program Files\Sysinternals')
+
+    assert created.status_code == 201, created.get_json()
+    assert (created.get_json()['data']['workingdirectory']
+            == r'C:\Program Files\Sysinternals')
+
+
+def test_a_trailing_separator_is_trimmed(client, headers):
+    created = _create(client, headers, 'trailing',
+                      workingdirectory='C:\\Tools\\')
+
+    assert created.get_json()['data']['workingdirectory'] == r'C:\Tools'
+
+
+def test_a_drive_root_keeps_its_separator(client, headers):
+    r"""C:\ must not degrade to C:. On Windows "C:" is drive-RELATIVE and
+    resolves to wherever the process last was on that drive, which is the exact
+    opposite of pinning a folder - a silent wrong answer, not an error."""
+    created = _create(client, headers, 'driveroot', workingdirectory='C:\\')
+
+    assert created.get_json()['data']['workingdirectory'] == 'C:\\'
+
+
+def test_forward_slashes_are_normalized(client, headers):
+    created = _create(client, headers, 'slashes',
+                      workingdirectory='C:/Tools/Corporate')
+
+    assert created.get_json()['data']['workingdirectory'] == r'C:\Tools\Corporate'
+
+
+@pytest.mark.parametrize('bad', [
+    r'Tools\Corporate',          # relative: relative to WHAT?
+    r'\\fileserver\tools',       # UNC: machine-account access to a file server
+    r'C:\Tools\..\..\Windows',   # traversal: obscures the real target
+])
+def test_a_malformed_folder_is_rejected_with_an_explanation(client, headers, bad):
+    """Rejected at the API, where the message can say why, rather than on the
+    endpoint where the failure would name a missing tool instead."""
+    created = _create(client, headers, f'bad-{abs(hash(bad))}',
+                      workingdirectory=bad)
+
+    assert created.status_code == 400, created.get_json()
+    assert 'workingdirectory' in created.get_json()['data']['error']['message']
+
+
+def test_a_folder_can_be_cleared_back_to_the_default(client, headers):
+    """Reverting to staging must be reachable; otherwise a mistyped pin is
+    permanent short of deleting and re-uploading the script."""
+    scriptid = _create(client, headers, 'clearable',
+                       workingdirectory=r'C:\Tools').get_json()['data']['scriptid']
+
+    updated = client.put(f'/api/scripts/{scriptid}', headers=headers,
+                         json={'workingdirectory': ''})
+
+    assert updated.status_code == 200, updated.get_json()
+    assert not updated.get_json()['data']['workingdirectory']
+
+
+def test_the_delivery_descriptor_always_carries_the_field(client, db, headers):
+    """Always present, even when empty, so an agent that received it and
+    ignored it is distinguishable from one that never saw it."""
+    from plugins.endpoints.api.routes import _resolve_run_script
+    from shopdb.core.models.script_library import Script
+
+    script = Script(scriptname='delivered', filename='script-1.ps1',
+                    sha256='a' * 64, workingdirectory=r'C:\Tools')
+    db.session.add(script)
+    db.session.commit()
+
+    descriptor, error = _resolve_run_script(f'script:{script.scriptid}')
+
+    assert error is None, error
+    assert descriptor['workingdirectory'] == r'C:\Tools'
+
+
+def test_the_descriptor_sends_an_empty_string_when_unpinned(client, db, headers):
+    from plugins.endpoints.api.routes import _resolve_run_script
+    from shopdb.core.models.script_library import Script
+
+    script = Script(scriptname='unpinned', filename='script-2.ps1',
+                    sha256='b' * 64)
+    db.session.add(script)
+    db.session.commit()
+
+    descriptor, error = _resolve_run_script(f'script:{script.scriptid}')
+
+    assert error is None, error
+    assert descriptor['workingdirectory'] == ''