From d6163f54a41c66a84ea27f639fbfd542e39ea204 Mon Sep 17 00:00:00 2001 From: "benjamin.baker" Date: Wed, 9 Sep 2026 11:36:42 -0400 Subject: [PATCH] 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. --- .../CommandExecutor.cs | 282 +++++++++++++++++- agent/ShopDbEndpointAgent.Core/Contracts.cs | 17 ++ .../ShopDbEndpointAgent.csproj | 33 +- .../versions/7d48_script_workingdirectory.py | 41 +++ migrations/versions/7d49_script_errormode.py | 46 +++ plugins/computers/frontend/views/PCDetail.vue | 230 +++++++++++++- .../frontend/views/pcDetailRunScripts.spec.js | 85 ++++++ plugins/endpoints/AGENT-CONTRACT.md | 69 +++++ plugins/endpoints/api/routes.py | 14 +- .../frontend/views/SystemScriptsSettings.vue | 139 ++++++++- .../views/systemScriptsSettings.spec.js | 55 ++++ shopdb/core/api/scripts.py | 90 +++++- shopdb/core/models/__init__.py | 6 +- shopdb/core/models/script_library.py | 37 +++ tests/test_core/test_script_errormode.py | 138 +++++++++ .../test_core/test_script_workingdirectory.py | 149 +++++++++ 16 files changed, 1411 insertions(+), 20 deletions(-) create mode 100644 migrations/versions/7d48_script_workingdirectory.py create mode 100644 migrations/versions/7d49_script_errormode.py create mode 100644 plugins/computers/frontend/views/pcDetailRunScripts.spec.js create mode 100644 tests/test_core/test_script_errormode.py create mode 100644 tests/test_core/test_script_workingdirectory.py diff --git a/agent/ShopDbEndpointAgent.Core/CommandExecutor.cs b/agent/ShopDbEndpointAgent.Core/CommandExecutor.cs index 99277899..dffd7cd5 100644 --- a/agent/ShopDbEndpointAgent.Core/CommandExecutor.cs +++ b/agent/ShopDbEndpointAgent.Core/CommandExecutor.cs @@ -901,6 +901,7 @@ public sealed class CommandExecutor var runInUser = string.Equals( script.RunContext, "user", StringComparison.OrdinalIgnoreCase); + ApplyErrorMode(script, tempPath, name); return runInUser ? RunScriptInUserSession(name, script, tempPath) : await RunScriptAsSystem(name, script, tempPath, token); @@ -933,18 +934,203 @@ public sealed class CommandExecutor } } + // Best-effort. A missing transcript directory must not stop the script from + // running: the capture is diagnostics, and losing diagnostics is a smaller + // failure than refusing to do the work that was asked for. + private void TryEnsureTranscriptDirectory(string transcriptPath, string name) + { + try + { + var directory = Path.GetDirectoryName(transcriptPath); + if (!string.IsNullOrWhiteSpace(directory)) + Directory.CreateDirectory(directory!); + } + catch (Exception error) + { + logger.LogWarning( + error, + "run_script {Name}: could not prepare the output transcript folder; " + + "the script will still run but its output will not be captured", name); + } + } + + // Read the transcript back and delete it. Deleted whether or not the read + // worked: it sits in the operator's own profile where they could read it, + // and a script's output is not theirs to keep lying around after the agent + // has reported it. + private string ReadAndClearTranscript(string transcriptPath, string name) + { + var output = ""; + try + { + if (File.Exists(transcriptPath)) + output = CombineScriptOutput(File.ReadAllText(transcriptPath), null); + } + catch (Exception error) + { + logger.LogWarning( + error, "run_script {Name}: could not read the output transcript", name); + } + try + { + if (File.Exists(transcriptPath)) + File.Delete(transcriptPath); + } + catch (Exception error) + { + logger.LogDebug( + error, "run_script {Name}: transcript cleanup failed", name); + } + return output; + } + + // Cap on captured script output. Two different limits are at work: the + // endpoint log should stay readable, and the server payload rides an HTTP + // POST into a MEDIUMTEXT column. A runaway loop printing megabytes must not + // turn a failed script into a failed REPORT - losing the verdict entirely is + // worse than losing the tail of the evidence. Head is kept rather than tail + // because the FIRST error is the one that explains the failure; under strict + // mode nothing after it ran anyway. + private const int MaxCapturedOutputChars = 8000; + + // Merge the two streams into one operator-readable blob. stderr is labelled + // rather than concatenated blind, because "which stream said this" is the + // difference between a warning a script prints and the error that killed it. + private static string CombineScriptOutput(string? stdout, string? stderr) + { + var parts = new List(2); + if (!string.IsNullOrWhiteSpace(stdout)) + parts.Add(stdout!.TrimEnd()); + if (!string.IsNullOrWhiteSpace(stderr)) + parts.Add("[stderr] " + stderr!.TrimEnd()); + var combined = string.Join(Environment.NewLine, parts); + if (combined.Length <= MaxCapturedOutputChars) + return combined; + // Say so explicitly. Silently truncated evidence reads as a script that + // stopped talking, which is a different diagnosis than one that was cut. + return combined.Substring(0, MaxCapturedOutputChars) + + Environment.NewLine + + $"[truncated after {MaxCapturedOutputChars} characters]"; + } + + // The endpoint log gets the output ALWAYS, success or failure. This is the + // copy that stays on the box: it never leaves the endpoint, so it costs no + // exposure, and it is the only record when a script reports success while + // doing nothing useful. The server copy is failure-only (see ScriptDetail). + private void LogScriptOutput(string name, string output, int? exitCode) + { + if (string.IsNullOrWhiteSpace(output)) + { + logger.LogInformation( + "run_script {Name} produced no output (exit {Exit})", + name, exitCode?.ToString() ?? "unknown"); + return; + } + logger.LogInformation( + "run_script {Name} output (exit {Exit}):{NewLine}{Output}", + name, exitCode?.ToString() ?? "unknown", Environment.NewLine, output); + } + + // Prepended to a strict-mode body. Written on ONE line so every line number + // PowerShell reports still matches the body the author sees in the WebUI + // editor: a multi-line prologue would shift them and make the error text + // point at the wrong statement during exactly the debugging session that + // needed it. + private const string StrictModePrologue = + "$ErrorActionPreference = 'Stop'; $ProgressPreference = 'SilentlyContinue'"; + + // Turn a script's non-terminating errors fatal when the library says strict. + // + // MUST run AFTER the SHA-256 check. The digest pins the bytes the SERVER + // holds; prepending first would rewrite the file and fail its own integrity + // check. Verify what was delivered, then modify how it is interpreted. + // + // Best-effort by design: if the rewrite fails the script still runs under + // PowerShell's default rules. That is the behaviour it had before this + // existed, so a failed rewrite degrades to "as before" rather than blocking + // work, and the warning says the verdict is the weaker one. + private void ApplyErrorMode(ScriptDeliveryClaim script, string scriptPath, string name) + { + if (!string.Equals(script.ErrorMode, "strict", StringComparison.OrdinalIgnoreCase)) + return; + + try + { + var body = File.ReadAllText(scriptPath); + File.WriteAllText(scriptPath, StrictModePrologue + Environment.NewLine + body); + logger.LogInformation( + "run_script {Name}: strict error mode applied; a non-terminating " + + "error will now stop the script and fail the command", name); + } + catch (Exception error) + { + logger.LogWarning( + error, + "run_script {Name}: could not apply strict error mode; running under " + + "PowerShell defaults, so a non-terminating error will NOT fail the command", + name); + } + } + + // Where the script actually runs. Empty means the agent staging directory, + // which is right for a self-contained script and wrong for one that shells + // out to a corporate tool already installed on the endpoint (Sysinternals + // and similar, delivered by Intune) and invokes it by relative path. The + // server carries that folder per script. + // + // A NAMED DIRECTORY THAT IS ABSENT FAILS THE COMMAND. Falling back to + // staging would run SYSTEM PowerShell somewhere other than where the author + // said; the script would then fail further in with an error naming the + // missing tool rather than the missing folder, or half-succeed, which is + // worse. Fail closed and name the folder. + private static bool TryResolveWorkingDirectory( + ScriptDeliveryClaim? script, string scriptPath, + out string workingDirectory, out string error) + { + error = ""; + workingDirectory = Path.GetDirectoryName(scriptPath) ?? ""; + + var requested = script?.WorkingDirectory; + if (string.IsNullOrWhiteSpace(requested)) + return true; + + if (!Directory.Exists(requested)) + { + error = $"working directory not found on this endpoint: {requested}"; + return false; + } + + workingDirectory = requested!; + return true; + } + // System context: run through the agent's own service account and wait for // the exit code, the same as before. Fine for machine-scoped scripts that do // not need the interactive user's session resources. private async Task RunScriptAsSystem( string name, ScriptDeliveryClaim script, string scriptPath, CancellationToken token) { + if (!TryResolveWorkingDirectory( + script, scriptPath, out var workingDirectory, out var directoryError)) + { + logger.LogWarning( + "run_script for {Name} not started: {Error}", name, directoryError); + return new CommandResult + { + Succeeded = false, + Message = $"Script {name} not run: {directoryError}", + Detail = ScriptDetail(name, script, scriptPath, exitCode: null), + }; + } + var startInfo = new ProcessStartInfo { FileName = "powershell.exe", UseShellExecute = false, CreateNoWindow = true, - WorkingDirectory = Path.GetDirectoryName(scriptPath) ?? "", + WorkingDirectory = workingDirectory, + RedirectStandardOutput = true, + RedirectStandardError = true, }; startInfo.ArgumentList.Add("-NoProfile"); startInfo.ArgumentList.Add("-NonInteractive"); @@ -960,19 +1146,27 @@ public sealed class CommandExecutor { Succeeded = false, Message = $"Failed to start script {name}", - Detail = ScriptDetail(name, script, scriptPath, exitCode: null), + Detail = ScriptDetail(name, script, scriptPath, exitCode: null, workingDirectory), }; } await process.WaitForExitAsync(token); + // Read only AFTER exit, for the reason spelled out on the schtasks path: + // ReadToEnd blocks until the streams close, which IS process exit, so + // reading first would make the caller's cancellation unable to fire. + var stdout = await process.StandardOutput.ReadToEndAsync(token); + var stderr = await process.StandardError.ReadToEndAsync(token); var exit = process.ExitCode; + var output = CombineScriptOutput(stdout, stderr); + LogScriptOutput(name, output, exit); return new CommandResult { Succeeded = exit == 0, Message = exit == 0 ? $"Script {name} completed (system context)" : $"Script {name} exited with code {exit}", - Detail = ScriptDetail(name, script, scriptPath, exit), + Detail = ScriptDetail(name, script, scriptPath, exit, workingDirectory, + exit == 0 ? null : output), }; } @@ -988,13 +1182,57 @@ public sealed class CommandExecutor // hung script pinned the caller forever. private static readonly TimeSpan UserScriptWaitTimeout = TimeSpan.FromMinutes(5); + // Where a user-session script's output is captured to. + // + // The SYSTEM path can simply redirect the child's stdout/stderr handles. + // This path cannot: it goes through CreateProcessAsUser to cross a session + // boundary, and inheriting pipe handles across that boundary is exactly the + // part of the launcher that is delicate (session enumeration, token + // duplication). So PowerShell redirects to a file itself and the agent reads + // it back after exit. + // + // The file lands in the OPERATOR's profile, not the agent's ProgramData + // staging folder: staging is SYSTEM-owned and the signed-in operator cannot + // write there, which is the same constraint that put the tray's own log in + // the profile. Safe to rely on here because a user-context script only runs + // when someone is interactively signed in, and a signed-in account has a + // materialised profile by definition. + private static string UserTranscriptPath(int scriptId) + => Path.Combine( + FileLoggerProvider.UserProfileLogDirectory(), + $"shopdb-script-{scriptId}-output.txt"); + private CommandResult RunScriptInUserSession( string name, ScriptDeliveryClaim script, string scriptPath) { var quotedPath = "\"" + scriptPath + "\""; + // Redirect inside the user session rather than from the service. The + // child is launched through CreateProcessAsUser, so the agent holds no + // handles to its streams; -Command wraps the same -File invocation and + // sends both streams to a file the operator can write, which the agent + // reads back after exit. 2>&1 merges stderr into the same transcript + // because ordering between the two matters more than telling them apart + // when reconstructing what a script did. + var transcriptPath = UserTranscriptPath(script.ScriptId); + var quotedTranscript = "\"" + transcriptPath + "\""; + TryEnsureTranscriptDirectory(transcriptPath, name); var arguments = - "-NoProfile -NonInteractive -ExecutionPolicy Bypass -File " + quotedPath; - var workingDirectory = Path.GetDirectoryName(scriptPath) ?? ""; + "-NoProfile -NonInteractive -ExecutionPolicy Bypass -Command " + + "\"& { & powershell.exe -NoProfile -NonInteractive -ExecutionPolicy " + + "Bypass -File " + quotedPath.Replace("\"", "\\\"") + + " *> " + quotedTranscript.Replace("\"", "\\\"") + "; exit $LASTEXITCODE }\""; + if (!TryResolveWorkingDirectory( + script, scriptPath, out var workingDirectory, out var directoryError)) + { + logger.LogWarning( + "run_script for {Name} not started: {Error}", name, directoryError); + return new CommandResult + { + Succeeded = false, + Message = $"Script {name} not run: {directoryError}", + Detail = ScriptDetail(name, script, scriptPath, exitCode: null), + }; + } var outcome = InteractiveProcessLauncher.LaunchInActiveSession( "powershell.exe", arguments, workingDirectory, waitForExit: true, @@ -1008,7 +1246,8 @@ public sealed class CommandExecutor { Succeeded = false, Message = $"Script {name} could not run in the user session: {outcome.Error}", - Detail = ScriptDetail(name, script, scriptPath, exitCode: null), + Detail = ScriptDetail(name, script, scriptPath, exitCode: null, workingDirectory, + ReadAndClearTranscript(transcriptPath, name)), }; } @@ -1018,19 +1257,22 @@ public sealed class CommandExecutor // as unresolved rather than success, because we never saw an exit // code and cannot claim it worked. logger.LogWarning( - "run_script for {Name} still running after {Timeout}; stopped waiting", - name, UserScriptWaitTimeout); + "run_script for {Name} still running after {Timeout}; stopped waiting. " + + "Partial output remains at {Transcript} while it runs", + name, UserScriptWaitTimeout, transcriptPath); return new CommandResult { Succeeded = false, Message = $"Script {name} did not finish within " + $"{UserScriptWaitTimeout.TotalMinutes:0} minutes and is still running", - Detail = ScriptDetail(name, script, scriptPath, exitCode: null), + Detail = ScriptDetail(name, script, scriptPath, exitCode: null, workingDirectory), }; } var exit = outcome.ExitCode; var ok = exit is null || exit == 0; + var output = ReadAndClearTranscript(transcriptPath, name); + LogScriptOutput(name, output, exit); return new CommandResult { Succeeded = ok, @@ -1039,12 +1281,14 @@ public sealed class CommandExecutor : ok ? $"Script {name} completed (user context)" : $"Script {name} exited with code {exit}", - Detail = ScriptDetail(name, script, scriptPath, exit), + Detail = ScriptDetail(name, script, scriptPath, exit, workingDirectory, + ok ? null : output), }; } private static Dictionary ScriptDetail( - string name, ScriptDeliveryClaim? script, string? scriptPath, int? exitCode) + string name, ScriptDeliveryClaim? script, string? scriptPath, int? exitCode, + string? workingDirectory = null, string? output = null) { var detail = new Dictionary { @@ -1056,11 +1300,27 @@ public sealed class CommandExecutor detail["scriptid"] = script.ScriptId; if (!string.IsNullOrWhiteSpace(script.RunContext)) detail["runcontext"] = script.RunContext!; + if (!string.IsNullOrWhiteSpace(script.ErrorMode)) + detail["errormode"] = script.ErrorMode!; } if (scriptPath is not null) detail["path"] = scriptPath; + // The directory the script REALLY ran in, not the one that was asked + // for. An agent too old to know the field reports nothing here, which is + // how an operator tells "ignored" apart from "honoured" while agents of + // both vintages are in the field. + if (!string.IsNullOrWhiteSpace(workingDirectory)) + detail["workingdirectory"] = workingDirectory!; if (exitCode is not null) detail["exitcode"] = exitCode; + // FAILURE ONLY. Callers pass output only when the command failed, so a + // successful run's stdout never leaves the endpoint. Script output can + // carry whatever the body happened to print - hostnames, paths, tokens + // an author echoed while debugging - and there is no reason to accept + // that exposure for a run nobody needs to diagnose. The endpoint log + // keeps the full copy either way. + if (!string.IsNullOrWhiteSpace(output)) + detail["output"] = output!; return detail; } diff --git a/agent/ShopDbEndpointAgent.Core/Contracts.cs b/agent/ShopDbEndpointAgent.Core/Contracts.cs index 911ad397..a7a1e1c1 100644 --- a/agent/ShopDbEndpointAgent.Core/Contracts.cs +++ b/agent/ShopDbEndpointAgent.Core/Contracts.cs @@ -182,6 +182,21 @@ public sealed class AppDeliveryClaim // downloads with its collector credential; Sha256 pins the exact bytes; // RunContext is 'system' (the agent's own service account) or 'user' (the // interactive logged-on session), decided per script in the ShopDB library. +// WorkingDirectory is the optional folder to run the script IN. Empty means the +// agent's staging directory, which is what every script did before the field +// existed. It is there so a script can sit beside a corporate tool already +// installed on the endpoint and invoke it by relative path; it confers no +// rights, since the body is already arbitrary PowerShell that could change its +// own directory. +// ErrorMode is 'default' or 'strict'. Default is what PowerShell does unaided: +// a non-terminating error is written and the script continues, so powershell.exe +// still exits 0 and the command reports SUCCEEDED even when the body did none of +// its work. Strict makes the agent prepend $ErrorActionPreference = 'Stop' so the +// first error terminates and the exit code means what an operator reads it to +// mean. Per script, never global: strict changes execution semantics, and a body +// written to tolerate a failing cleanup or to probe with a cmdlet that errors on +// absence would start aborting halfway. An absent or unrecognised value is +// treated as default, so an old server that never sends the field is safe. public sealed class ScriptDeliveryClaim { [JsonPropertyName("scriptid")] public int ScriptId { get; set; } @@ -189,6 +204,8 @@ public sealed class ScriptDeliveryClaim [JsonPropertyName("installpath")] public string? InstallPath { get; set; } [JsonPropertyName("sha256")] public string? Sha256 { get; set; } [JsonPropertyName("runcontext")] public string? RunContext { get; set; } + [JsonPropertyName("workingdirectory")] public string? WorkingDirectory { get; set; } + [JsonPropertyName("errormode")] public string? ErrorMode { get; set; } } // Envelope for the claim long-poll: { status, data: { endpointcommandid, action } }. diff --git a/agent/ShopDbEndpointAgent/ShopDbEndpointAgent.csproj b/agent/ShopDbEndpointAgent/ShopDbEndpointAgent.csproj index e3ca12f1..f8678bf3 100644 --- a/agent/ShopDbEndpointAgent/ShopDbEndpointAgent.csproj +++ b/agent/ShopDbEndpointAgent/ShopDbEndpointAgent.csproj @@ -131,6 +131,21 @@ (AgentConfig.LaunchPaths, shipped defaults overridable per site). 1.1.0: generic, data-driven service_*/process_* actions carry their target over the wire, so new policies/apps need no agent rebuild. --> + - 1.12.0 + + 1.14.0 true win-x64 true diff --git a/migrations/versions/7d48_script_workingdirectory.py b/migrations/versions/7d48_script_workingdirectory.py new file mode 100644 index 00000000..4aef61e7 --- /dev/null +++ b/migrations/versions/7d48_script_workingdirectory.py @@ -0,0 +1,41 @@ +"""Add scripts workingdirectory + +Backs the optional "run in this folder" field on a library script. The agent +otherwise runs a delivered script from its own staging directory, which is +correct for a self-contained script and wrong for one that shells out to a +corporate tool already installed on the endpoint (Sysinternals and similar, +delivered by Intune) and referenced by relative path. + +NULLABLE with no default, so every script row that existed before this +migration stays valid and keeps today's behaviour exactly: empty means the +staging directory. The path is validated in shopdb/core/api/scripts.py where a +rejection can explain itself, not in the schema where it would only produce an +IntegrityError. + +This column grants no capability. A script body is already arbitrary PowerShell +run as SYSTEM and can change its own directory; the boundary is the +endpoints.runscript grant and the digest-pinned library. + +Revision ID: 7d48_script_workingdir +Revises: 7d47_endpoints_operatorreports +Create Date: 2026-09-09 + +""" +from alembic import op +import sqlalchemy as sa + + +revision = '7d48_script_workingdir' +down_revision = '7d47_endpoints_operatorreports' +branch_labels = None +depends_on = None + + +def upgrade(): + op.add_column('scripts', sa.Column( + 'workingdirectory', sa.String(length=255), nullable=True, + comment='Folder to run the script in; empty means the agent staging dir')) + + +def downgrade(): + op.drop_column('scripts', 'workingdirectory') diff --git a/migrations/versions/7d49_script_errormode.py b/migrations/versions/7d49_script_errormode.py new file mode 100644 index 00000000..a8da063b --- /dev/null +++ b/migrations/versions/7d49_script_errormode.py @@ -0,0 +1,46 @@ +"""Add scripts errormode + +Backs the per-script "how should a non-terminating error be treated" field. + +PowerShell invoked with -File exits 0 whenever it reaches the end of the body, +even if every meaningful line inside wrote an error. The agent derives the +command verdict from that exit code, so a script could report SUCCEEDED while +doing none of its work - which is exactly how an account-provisioning script +came back green with the account only partly built. + +NOT NULL with server_default 'default', so every script row that existed before +this migration keeps today's permissive behaviour exactly. Strict is opt-in per +script and never retroactive: turning it on globally would change the meaning of +bodies that were authored under the permissive rule and deliberately tolerate a +failing Remove-Item, probe with Get-LocalUser, or loop past a bad item. Those +would begin aborting partway through, which is worse than a false green because +it leaves the endpoint half-configured. + +The value is validated in shopdb/core/api/scripts.py where a rejection can +explain itself, not in the schema where it would only produce an IntegrityError. + +Revision ID: 7d49_script_errormode +Revises: 7d48_script_workingdir +Create Date: 2026-09-09 + +""" +from alembic import op +import sqlalchemy as sa + + +revision = '7d49_script_errormode' +down_revision = '7d48_script_workingdir' +branch_labels = None +depends_on = None + + +def upgrade(): + op.add_column('scripts', sa.Column( + 'errormode', sa.String(length=16), nullable=False, + server_default='default', + comment="'default' keeps PowerShell's permissive behaviour; " + "'strict' stops on the first error so the exit code is honest")) + + +def downgrade(): + op.drop_column('scripts', 'errormode') diff --git a/plugins/computers/frontend/views/PCDetail.vue b/plugins/computers/frontend/views/PCDetail.vue index 6a8deb72..7fe76e7c 100644 --- a/plugins/computers/frontend/views/PCDetail.vue +++ b/plugins/computers/frontend/views/PCDetail.vue @@ -512,6 +512,90 @@ + +
+
+

Run Scripts

+

+ Sends a script from the ShopDB script library to this computer. The + agent fetches the body by digest and runs it in the context the + script itself specifies. +

+ +

+ A script here runs as SYSTEM unless it is configured to run as the + logged-on user. It takes effect immediately on the live computer and + cannot be undone from here. +

+ +
Loading scripts...
+
+ No runnable scripts. A script needs an uploaded body before it can be + sent; add one under Settings then return here. +
+ +
+
+
Created {{ formatDate(computer.createddate) }} @@ -686,7 +770,7 @@ import { ref, computed, watch, onMounted, onUnmounted } from 'vue' import { colorStyle } from "@/utils/colorStyle" import { useRoute, useRouter } from 'vue-router' -import { computersApi, applicationsApi, endpointsApi } from '@/api' +import { computersApi, applicationsApi, endpointsApi, scriptsApi } from '@/api' import { withBase } from '@/utils/basePath' import { useWarrantyBadge } from '@/composables/warrantyBadge' import LocationMapTooltip from '@/components/LocationMapTooltip.vue' @@ -829,6 +913,16 @@ const hasCloudIdentity = computed(() => portalTemplatesLoaded.value && Boolean( // during setup, which reads canDeploy before this line would otherwise run. const canDeploy = computed(() => authStore.hasPermission('endpoints.corptools')) +// Gates the Run Scripts tab. Deliberately NOT canDeploy: endpoints.corptools +// starts a task the endpoint already holds and has allowlisted, while +// endpoints.runscript delivers arbitrary PowerShell that runs as SYSTEM unless +// the script pins a user context. Those were split out of the old +// endpoints.deploy bundle for exactly this reason (see +// ELEVATED_ACTION_PERMISSIONS in plugins/endpoints/commands.py), so hanging +// scripts off the Corporate Tools gate would put the bundle back together in +// the UI after the backend took it apart. +const canRunScript = computed(() => authStore.hasPermission('endpoints.runscript')) + // Operator report screenshots are an uncontrolled capture of whatever was on // the operator's screen, so they carry their own grant rather than riding // endpoints.view. Declared alongside canDeploy for the same reason: detailtabs @@ -868,6 +962,15 @@ const detailtabs = computed(() => { if (canDeploy.value && liveness.value) { tabs.push({ key: 'corptools', label: 'Corporate Tools' }) } + // Run Scripts is its own tab rather than a card in Corporate Tools because it + // is a different grant and a different trust model - a library of arbitrary + // PowerShell delivered by ShopDB, versus a closed set of tasks the endpoint + // already approved. Liveness-gated for the same reason as Corporate Tools: + // the command queues against a live agent, so offering it for a machine that + // is not checking in only produces a command nothing will collect. + if (canRunScript.value && liveness.value) { + tabs.push({ key: 'runscripts', label: 'Run Scripts' }) + } return tabs }) @@ -1377,6 +1480,47 @@ const corporateTools = [ const firingTask = ref('') +// Run Scripts state. The library is fetched rather than hardcoded (the opposite +// of corporateTools above) because these are site-authored scripts that change +// without an agent release. +const scriptlibrary = ref([]) +const scriptsloading = ref(false) +const scriptsloaded = ref(false) +const selectedscriptid = ref('') +const firingScript = ref(false) + +// Only scripts that can actually run. A row with no uploaded body is rejected +// at enqueue by _resolve_run_script, so listing it would offer a button whose +// only outcome is an error; the library page is where that gets fixed. +const runnablescripts = computed( + () => scriptlibrary.value.filter((script) => script.hasbody)) + +const selectedscript = computed( + () => runnablescripts.value.find( + (script) => String(script.scriptid) === String(selectedscriptid.value)) || null) + +// Lazy and permission-gated: an operator without endpoints.runscript never has +// the tab, and so never issues a request the server would refuse anyway. +async function loadScriptLibrary() { + if (scriptsloaded.value || scriptsloading.value) return + scriptsloading.value = true + try { + const response = await scriptsApi.list({ active: 'true' }) + scriptlibrary.value = response.data.data || [] + scriptsloaded.value = true + } catch (error) { + toast.error(apiError(error, 'Failed to load the script library')) + } finally { + scriptsloading.value = false + } +} + +// Fetch on first view of the tab rather than on page load: most visits to a PC +// never open this, and the library is irrelevant to every other tab. +watch(activegroup, (group) => { + if (group === 'runscripts' && canRunScript.value) loadScriptLibrary() +}) + // Elevated: queues a scheduledtask_run. The confirm gate mirrors // confirmRestart - these start real work on a live machine and some of them // change system state - and the server re-checks endpoints.corptools regardless. @@ -1412,6 +1556,41 @@ async function confirmRunTask(tool) { } } +// Elevated: queues a run_script. commands.py asks for this to be gated "the way +// system_restart is gated", so the confirmation is TYPED rather than an OK +// button - and unlike Corporate Tools there is no non-destructive branch, +// because every script here is arbitrary code by definition. The confirm names +// the context and the folder so the operator sees what the library decided, not +// just the script's name. +async function confirmRunScript() { + const script = selectedscript.value + if (!script) return + const label = computer.value?.assetnumber || computer.value?.name || 'this PC' + const context = script.runcontext === 'user' + ? 'the logged-on user' + : 'SYSTEM' + const folder = script.workingdirectory || 'the agent staging folder' + const typed = prompt(`Run "${script.scriptname}" on ${label}\n\n` + + `It runs as ${context}, in ${folder}.\n` + + 'It starts immediately and cannot be undone from here.' + + '\n\nType RUN to confirm.') + if (typed !== 'RUN') return + + firingScript.value = true + try { + await endpointsApi.enqueueCommand( + computer.value.assetid, 'run_script', `script:${script.scriptid}`) + // "Queued", not "finished": the agent collects this on its next check-in, + // and the script's own result comes back separately. Saying "ran" here + // would claim an outcome nothing has observed yet. + toast.success(`${script.scriptname} queued for ${label}`) + } catch (error) { + toast.error(apiError(error, `Failed to queue ${script.scriptname}`)) + } finally { + firingScript.value = false + } +} + // Retire, not delete: the server deactivates the asset, so the PC // leaves every list and report while its history and relationships stay put. // Reversible by an admin setting isactive back to true. @@ -1864,6 +2043,55 @@ async function confirmRetire() { font-size: 0.8125rem; font-weight: 500; } +/* Run Scripts reuses the Corporate Tools intro/warning styles above; only the + picker-specific pieces are new. */ +.runscripts-empty { + padding: 0.75rem 0; + color: var(--text-secondary); + font-size: 0.875rem; +} +.runscripts-detail { + margin-top: 0.75rem; + padding: 0.75rem; + border: 1px solid var(--border); + border-radius: 4px; +} +.runscripts-desc { + margin: 0 0 0.5rem; + color: var(--text-secondary); + font-size: 0.8125rem; +} +/* Two columns so the label and its value stay on one line: the folder is the + fact most likely to be skimmed, and wrapping it under its own label made it + read as body copy in review. */ +.runscripts-facts { + display: grid; + grid-template-columns: auto 1fr; + gap: 0.25rem 0.75rem; + margin: 0; + font-size: 0.8125rem; +} +.runscripts-facts dt { + color: var(--text-secondary); +} +.runscripts-facts dd { + margin: 0; + color: var(--text-primary); +} +/* Defined locally: .mono is not a global utility in this build, and a path + rendered in the body font is harder to read back against the endpoint. */ +.runscripts-facts dd.mono { + font-family: var(--font-mono, ui-monospace, SFMono-Regular, Menlo, monospace); + word-break: break-all; +} +.runscripts-note { + margin: 0.5rem 0 0; + color: var(--text-secondary); + font-size: 0.8125rem; +} +.runscripts-actions { + margin-top: 1rem; +} .triage-chip.critical .triage-dot { background: var(--danger); } .triage-chip.ok { color: var(--success-dark); } .triage-chip.warning { color: var(--warning-dark); } diff --git a/plugins/computers/frontend/views/pcDetailRunScripts.spec.js b/plugins/computers/frontend/views/pcDetailRunScripts.spec.js new file mode 100644 index 00000000..a8149dbe --- /dev/null +++ b/plugins/computers/frontend/views/pcDetailRunScripts.spec.js @@ -0,0 +1,85 @@ +/** + * Run Scripts tab on PC Details. + * + * The thing worth pinning here is the SEPARATION. run_script and + * scheduledtask_run map to different grants in plugins/endpoints/commands.py + * (endpoints.runscript and endpoints.corptools), split out of a single + * endpoints.deploy bundle precisely so that being able to trigger a corporate + * tool does not also mean being able to run arbitrary PowerShell as SYSTEM. + * + * That split lives in the backend. The UI can quietly undo it by hanging the + * script picker off the Corporate Tools tab, whose only gate is + * endpoints.corptools - and the failure would be invisible in review, because + * the page would look correct and the server would simply 403 the push. These + * tests fail if someone merges the two. + * + * Asserted against the file text, matching systemScriptsSettings.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, 'PCDetail.vue'), 'utf8') + +describe('PCDetail Run Scripts tab', () => { + it('gates the tab on endpoints.runscript, not the Corporate Tools grant', () => { + expect(source).toMatch( + /const canRunScript = computed\(\(\) => authStore\.hasPermission\('endpoints\.runscript'\)\)/) + }) + + it('pushes run_script from its own tab, not from the Corporate Tools block', () => { + // The corptools block must not gain a run_script call: that would put the + // action behind endpoints.corptools in the UI while the server still + // demands endpoints.runscript. + const corptools = source.slice( + source.indexOf("activegroup === 'corptools'"), + source.indexOf("activegroup === 'runscripts'")) + expect(corptools).not.toMatch(/run_script/) + expect(source).toMatch(/activegroup === 'runscripts'/) + }) + + it('enqueues run_script with a script-id target', () => { + expect(source).toMatch(/'run_script', `script:\$\{script\.scriptid\}`/) + }) + + it('requires a typed confirmation before running', () => { + // Every script on this tab is arbitrary code, so unlike Corporate Tools + // there is no safe branch that gets a plain OK button. + const handler = source.slice(source.indexOf('async function confirmRunScript')) + const body = handler.slice(0, handler.indexOf('\n}\n')) + expect(body).toMatch(/Type RUN to confirm/) + expect(body).toMatch(/typed !== 'RUN'/) + }) + + it('warns that a script runs as SYSTEM before it is sent', () => { + const tab = source.slice(source.indexOf("activegroup === 'runscripts'")) + expect(tab.slice(0, 2000)).toMatch(/runs as SYSTEM/) + }) + + it('shows the run context and working directory read-only', () => { + // Both are authoring decisions made in the library. Offering an override + // here would mean the script that ran is not the script that was reviewed, + // so they must be rendered as facts and never bound with v-model. + const tab = source.slice(source.indexOf("activegroup === 'runscripts'")) + const block = tab.slice(0, tab.indexOf('runscripts-actions')) + expect(block).toMatch(/selectedscript\.workingdirectory/) + expect(block).toMatch(/selectedscript\.runcontext/) + expect(block).not.toMatch(/v-model="[^"]*workingdirectory/) + expect(block).not.toMatch(/v-model="[^"]*runcontext/) + }) + + it('offers only scripts that have a body', () => { + // A bodyless script is rejected at enqueue by _resolve_run_script, so + // listing it would offer a button whose only outcome is an error. + expect(source).toMatch(/filter\(\(script\) => script\.hasbody\)/) + }) + + it('reports the push as queued rather than as finished', () => { + // The agent collects the command on its next check-in; claiming it ran + // would assert an outcome nothing has observed. + const handler = source.slice(source.indexOf('async function confirmRunScript')) + expect(handler.slice(0, handler.indexOf('\n}\n'))).toMatch(/queued for/) + }) +}) diff --git a/plugins/endpoints/AGENT-CONTRACT.md b/plugins/endpoints/AGENT-CONTRACT.md index 21dc42d4..c809de74 100644 --- a/plugins/endpoints/AGENT-CONTRACT.md +++ b/plugins/endpoints/AGENT-CONTRACT.md @@ -465,6 +465,9 @@ Allowlisted actions (server source of truth: `plugins/endpoints/commands.py`): connections with a `truncated` flag in `detail`, because an oversized payload rejected by the server returns nothing at all and a partial answer is more useful than a failed one. Standard reports carry listening ports only. +- `run_script` - run a reviewed PowerShell script from the script library. The + body is NOT carried on the command channel; the claim carries a descriptor and + the agent fetches the bytes separately (section 7.4). ### 7.1 Claim the next command (long-poll) @@ -509,3 +512,69 @@ Allowlisted actions (server source of truth: `plugins/endpoints/commands.py`): gated by user JWT + `endpoints.command`. Only one command may be in flight per endpoint (a second enqueue returns HTTP 409 until the first finalizes). +### 7.4 `run_script` claim descriptor + +A `run_script` claim carries a `script` object alongside the usual +`endpointcommandid` and `action`: + +```jsonc +{ + "scriptid": 12, + "scriptname": "sfld_autologon", + "installpath": "/api/endpoints/scriptbody/12", // fetch with X-API-Key + "sha256": "3f9a...", // 64 hex chars, lowercase + "runcontext": "system", // "system" | "user" + "workingdirectory": "C:\\Tools", // "" = agent staging folder + "errormode": "default" // "default" | "strict" +} +``` + +The agent MUST download `installpath`, verify the SHA-256 of the received bytes +against `sha256`, and refuse to run on mismatch. The body never travels on the +command channel so a queued command cannot be used to smuggle code past review. + +`workingdirectory` is the folder the script runs from. Empty means the agent's +staging directory, which is correct for a self-contained script and wrong for +one invoking a separately-installed tool by relative path. A pinned folder that +does not exist MUST fail the command rather than falling back, so the script +never runs somewhere unintended. + +`errormode` selects how the run is judged: + +- `default` - PowerShell's normal behaviour. Most errors are non-terminating: + the script writes the error, continues, and still exits zero. +- `strict` - the agent prepends `$ErrorActionPreference = 'Stop'` so the first + error terminates the run and the exit code reflects reality. + +`default` is the default precisely BECAUSE it is permissive. Every script +authored before this field existed relies on that behaviour, and some rely on it +deliberately - tolerating a failing cleanup, probing with a cmdlet that errors +when a thing is absent, looping past a bad item. Promoting `strict` globally +would make those bodies abort partway and leave endpoints half-configured, so it +is opt-in per script. + +Two consequences the agent MUST honour: + +- The strict prologue is applied AFTER digest verification, so the hash still + validates the bytes the server actually delivered. +- Because the prologue adds a line, an error reported on line N of a strict run + refers to line N-1 of the stored body. Surfaces showing the body should say so. + +An agent that does not recognise an `errormode` value MUST treat it as +`default`. The server always sends the field explicitly, even at the default, so +an agent that received it and ignored it stays distinguishable from one that +never saw it. + +#### Script output + +Script stdout/stderr is ALWAYS written to the local agent log on the endpoint. +It is returned to the server in `detail.output` only when the command FAILED. +Routine output stays on the machine that produced it: shipping every successful +run back would put arbitrary script output, including whatever an author printed +while debugging, into the server's command history for no diagnostic gain. +Output is truncated to 8000 characters and flagged when truncated, because an +oversized payload rejected by the server returns nothing at all. + +Timed-out runs do not carry output: the script is still running and still holds +the capture file open, so anything read at that moment would be misleading. + diff --git a/plugins/endpoints/api/routes.py b/plugins/endpoints/api/routes.py index 85c7107b..602e6fe7 100644 --- a/plugins/endpoints/api/routes.py +++ b/plugins/endpoints/api/routes.py @@ -2070,9 +2070,15 @@ def _resolve_run_script(target): """Resolve a run_script target to a script delivery descriptor. Accepts either "script:" or a bare script name. The descriptor - carries a collector-authenticated download URL, the body's SHA-256, and the - per-script run context (system or user). The agent fetches the body with - its collector credential and runs it; the body never crosses the wire. + carries a collector-authenticated download URL, the body's SHA-256, the + per-script run context (system or user), and the optional folder to run in. + The agent fetches the body with its credential and runs it; the body never + crosses the wire. + + workingdirectory is sent even when empty so the field is always present in + the claim: an agent that receives it and ignores it is then distinguishable + from one that never saw it, which matters while agents of both vintages are + in the field. errormode rides the same rule, and for the same reason. """ kind, _sep, raw = str(target or '').partition(':') script = None @@ -2100,6 +2106,8 @@ def _resolve_run_script(target): 'installpath': '/api/endpoints/scriptbody/{}'.format(script.scriptid), 'sha256': script.sha256, 'runcontext': script.runcontext, + 'workingdirectory': script.workingdirectory or '', + 'errormode': script.errormode or 'default', }, None diff --git a/plugins/endpoints/frontend/views/SystemScriptsSettings.vue b/plugins/endpoints/frontend/views/SystemScriptsSettings.vue index 33ef0cba..30793bb9 100644 --- a/plugins/endpoints/frontend/views/SystemScriptsSettings.vue +++ b/plugins/endpoints/frontend/views/SystemScriptsSettings.vue @@ -20,6 +20,66 @@ written to act "as the logged-on user" will not behave the way it reads.
+
+ How does this work? +
+

Delivery

+

+ The body is stored here and hashed. At push time the agent downloads it + with its own credential and checks the digest before running anything, + so the bytes that run are the bytes that were reviewed. The body never + travels on the command channel and is deleted from the endpoint after + the run. +

+ +

Runs in

+

+ System is the agent's own service account: no desktop, + no mapped drives, no signed-in person's credentials. + User runs in the interactive session instead, and only + works when somebody is actually signed in. +

+ +

Run in folder

+

+ The working directory for the run. Blank means the agent's staging + folder, which is right for a self-contained script. Set it when the + script calls a tool installed separately on the endpoint by relative + path. If the folder is missing the run fails outright rather than + running somewhere unintended. +

+ +

When a line fails

+

+ This is the one worth understanding. PowerShell treats most errors as + non-terminating: it writes the error, carries on to the next + line, and still exits zero. Because the command's verdict comes from + that exit code, Continue means a script can report + success having accomplished nothing. +

+

+ Stop makes the first error end the run, so the verdict + means what you would assume it means. It is not the default because it + changes how a script behaves: a body that tolerates a failing cleanup, + probes with a cmdlet that errors when something is absent, or loops + past bad items will start aborting partway through, leaving the + endpoint half-configured. Read the script first, then switch it on. +

+ +

Where the output goes

+

+ Script output is always written to the agent log on the endpoint. It is + sent back to ShopDB only when the run fails, so routine output stays on + the machine that produced it. Long output is truncated and marked as + such. +

+

+ In Stop mode the agent adds one line to the top of the script, so an + error reported on line N refers to line N-1 of the body shown here. +

+
+
+
Loading...