Merge origin/wilm into wj

Brings in agent upgrade and log collection features:
- One-click agent self-upgrade from endpoint detail page
- Auto-register staged agent builds as pushable applications
- Log collection and script output capture
- Endpoint log collections plugin integration

Resolved conflict in ShopDbEndpointAgent.csproj by taking wilm version 1.16.0

agent/ShopDbEndpointAgent.Core/CommandExecutor.cs

@@ -1,6 +1,8 @@
 using System.Diagnostics;
+using System.Globalization;
 using System.Security.Cryptography;
 using System.ServiceProcess;
+using System.Text;
 using Microsoft.Extensions.Logging;
 
 namespace ShopDbEndpointAgent.Core;
@@ -167,6 +169,11 @@ public sealed class CommandExecutor
             case "network_connections":
                 return NetworkConnections();
 
+            // Read a plaintext log off this machine on demand. Read-only and
+            // bounded: see CollectLog for why each refusal is there.
+            case "collect_log":
+                return RequireTarget(action, target) ?? CollectLog(target);
+
             default:
                 logger.LogWarning("Rejected unknown command action {Action}", action);
                 return new CommandResult
@@ -241,6 +248,248 @@ public sealed class CommandExecutor
         }
     }
 
+    // ---- Log collection ----
+
+    // Extensions this agent will read. An allowlist, not a blocklist: the
+    // question is not "is this file dangerous" but "will this render as text",
+    // and only a closed set answers that. Mirrors LOG_PATH_EXTENSIONS on the
+    // server; that copy gives a fast error at enqueue, THIS copy is the one
+    // guarding the disk and must never be removed on the assumption the server
+    // already checked.
+    private static readonly HashSet<string> AllowedLogExtensions =
+        new(StringComparer.OrdinalIgnoreCase)
+        {
+            // The extension the server also omits (see LOG_PATH_EXTENSIONS) is
+            // omitted here too. The two copies must agree, or the server
+            // accepts a path the agent then refuses and the operator gets a
+            // failure with no actionable cause.
+            ".log", ".txt", ".xml", ".json", ".csv", ".ini", ".conf",
+            ".yml", ".yaml", ".trace", ".out", ".err", ".md",
+        };
+
+    // Ceiling on what crosses the wire. Logs grow without bound; a multi-GB
+    // file would exhaust memory here and be refused by the server's payload
+    // limit anyway, returning nothing. Sending the last 2 MB is far more useful
+    // than failing, because the end of a log is where the recent failure is.
+    private const int MaxLogBytes = 2 * 1024 * 1024;
+
+    // Read a plaintext log file and return its tail.
+    //
+    // Strictly read-only: opens for reading, never creates, writes or deletes.
+    // FileShare.ReadWrite|Delete is essential rather than lax - the interesting
+    // logs are the ones a running application currently holds open, and a
+    // stricter share mode would fail on exactly those with a sharing violation.
+    private CommandResult CollectLog(string target)
+    {
+        var path = (target ?? string.Empty).Trim();
+
+        try
+        {
+            // Re-validate independently of the server. The agent runs as SYSTEM
+            // and this path arrived over the network; trusting an upstream check
+            // would make the whole guard depend on a component this process
+            // cannot verify.
+            var rejection = ValidateLogPath(ref path);
+            if (rejection != null)
+            {
+                logger.LogWarning("collect_log refused {Path}: {Reason}", path, rejection);
+                return new CommandResult { Succeeded = false, Message = rejection };
+            }
+
+            var info = new FileInfo(path);
+            if (!info.Exists)
+            {
+                return new CommandResult
+                {
+                    Succeeded = false,
+                    Message = $"No file found at {path}",
+                };
+            }
+
+            var totalBytes = info.Length;
+            var truncated = totalBytes > MaxLogBytes;
+
+            string content;
+            using (var stream = new FileStream(
+                path, FileMode.Open, FileAccess.Read,
+                FileShare.ReadWrite | FileShare.Delete))
+            {
+                if (truncated)
+                {
+                    // Seek to the tail. This can land mid-line, so the partial
+                    // first line is dropped below rather than shown as a
+                    // fragment the reader would misread as a real entry.
+                    stream.Seek(-MaxLogBytes, SeekOrigin.End);
+                }
+
+                using var reader = new StreamReader(stream, Encoding.UTF8, true);
+                content = reader.ReadToEnd();
+            }
+
+            if (truncated)
+            {
+                var firstBreak = content.IndexOf('\n');
+                if (firstBreak >= 0 && firstBreak < content.Length - 1)
+                    content = content[(firstBreak + 1)..];
+            }
+
+            // A file whose bytes are not text will have produced replacement
+            // characters above. Returning that would fill the operator's pane
+            // with noise, so it is refused with a reason instead.
+            if (LooksBinary(content))
+            {
+                return new CommandResult
+                {
+                    Succeeded = false,
+                    Message =
+                        $"{path} does not appear to be a plaintext file; " +
+                        "only text logs can be collected",
+                };
+            }
+
+            var lineCount = CountLines(content);
+
+            logger.LogInformation(
+                "collect_log read {Path} ({Bytes} bytes on disk, truncated: {Truncated})",
+                path, totalBytes, truncated);
+
+            return new CommandResult
+            {
+                Succeeded = true,
+                Message = truncated
+                    ? $"Collected the last {content.Length:N0} characters of {info.Name} "
+                      + $"({totalBytes:N0} bytes on disk)"
+                    : $"Collected {info.Name} ({totalBytes:N0} bytes)",
+                Detail = new Dictionary<string, object>
+                {
+                    // The RESOLVED path, which may differ from what was typed
+                    // once environment variables are expanded.
+                    ["path"] = info.FullName,
+                    ["content"] = content,
+                    ["filesizebytes"] = totalBytes,
+                    ["linecount"] = lineCount,
+                    ["truncated"] = truncated,
+                    ["modifiedat"] = info.LastWriteTimeUtc.ToString(
+                        "o", CultureInfo.InvariantCulture),
+                },
+            };
+        }
+        catch (UnauthorizedAccessException exception)
+        {
+            // Called out separately because the remedy is different from a
+            // generic failure: the file exists but the agent cannot read it.
+            logger.LogWarning(exception, "collect_log denied for {Path}", path);
+            return new CommandResult
+            {
+                Succeeded = false,
+                Message = $"Access denied reading {path}",
+            };
+        }
+        catch (IOException exception)
+        {
+            logger.LogWarning(exception, "collect_log io failure for {Path}", path);
+            return new CommandResult
+            {
+                Succeeded = false,
+                Message = $"Could not read {path}: {exception.Message}",
+            };
+        }
+        catch (Exception exception)
+        {
+            logger.LogWarning(exception, "collect_log failed for {Path}", path);
+            return new CommandResult
+            {
+                Succeeded = false,
+                Message = $"Could not collect {path}: {exception.Message}",
+            };
+        }
+    }
+
+    // Shape validation, run before anything is opened. Returns a reason to
+    // refuse, or null to proceed. Expands environment variables first so the
+    // checks apply to the path that will ACTUALLY be opened - validating the
+    // literal string and then expanding it would let %VAR% smuggle past every
+    // rule here.
+    private static string? ValidateLogPath(ref string path)
+    {
+        if (string.IsNullOrWhiteSpace(path))
+            return "A log file path is required";
+
+        path = Environment.ExpandEnvironmentVariables(path).Trim();
+        var normalized = path.Replace('/', '\\');
+
+        if (normalized.StartsWith(@"\\", StringComparison.Ordinal))
+            return "UNC/network paths are not supported";
+
+        // Reject '..' before calling GetFullPath. Normalising first would
+        // resolve the traversal and hide it, so the check has to see the raw
+        // segments.
+        if (normalized.Split('\\').Any(part => part == ".."))
+            return "Path must not contain \"..\" segments";
+
+        if (normalized.Contains('*') || normalized.Contains('?'))
+            return "Wildcards are not supported; name a single log file";
+
+        if (!Path.IsPathRooted(normalized) || normalized.Length < 3
+            || normalized[1] != ':')
+        {
+            return "Enter a full local path including the drive letter";
+        }
+
+        var extension = Path.GetExtension(normalized);
+        if (string.IsNullOrEmpty(extension)
+            || !AllowedLogExtensions.Contains(extension))
+        {
+            return "Only plaintext log files can be collected "
+                 + $"(allowed: {string.Join(", ", AllowedLogExtensions.OrderBy(e => e))})";
+        }
+
+        // A directory reached through a path that passes every check above is
+        // still not a file, and Directory.Exists is cheap insurance.
+        if (Directory.Exists(normalized))
+            return "That path is a directory, not a log file";
+
+        path = Path.GetFullPath(normalized);
+        return null;
+    }
+
+    // Heuristic: a NUL byte, or many replacement characters, means the decode
+    // produced garbage rather than text. Sampling the head is enough - a binary
+    // file betrays itself immediately, and scanning megabytes to be sure would
+    // cost more than it is worth.
+    private static bool LooksBinary(string content)
+    {
+        if (string.IsNullOrEmpty(content))
+            return false;
+
+        var sample = Math.Min(content.Length, 8000);
+        var replacements = 0;
+        for (var index = 0; index < sample; index++)
+        {
+            var character = content[index];
+            if (character == '\0')
+                return true;
+            if (character == '\uFFFD')
+                replacements++;
+        }
+
+        return replacements > sample / 100;
+    }
+
+    private static int CountLines(string content)
+    {
+        if (string.IsNullOrEmpty(content))
+            return 0;
+
+        var lines = 1;
+        foreach (var character in content)
+        {
+            if (character == '\n')
+                lines++;
+        }
+        return lines;
+    }
+
     private Dictionary<int, string> GetProcessNameSnapshot()
     {
         var names = new Dictionary<int, string>();
@@ -1016,7 +1265,7 @@ public sealed class CommandExecutor
     // 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).
+    // doing nothing useful. The server copy is success-gated (see ShouldReturnOutput).
     private void LogScriptOutput(string name, string output, int? exitCode)
     {
         if (string.IsNullOrWhiteSpace(output))
@@ -1166,7 +1415,7 @@ public sealed class CommandExecutor
                 ? $"Script {name} completed (system context)"
                 : $"Script {name} exited with code {exit}",
             Detail = ScriptDetail(name, script, scriptPath, exit, workingDirectory,
-                exit == 0 ? null : output),
+                ShouldReturnOutput(script, succeeded: exit == 0) ? output : null),
         };
     }
 
@@ -1282,10 +1531,22 @@ public sealed class CommandExecutor
                     ? $"Script {name} completed (user context)"
                     : $"Script {name} exited with code {exit}",
             Detail = ScriptDetail(name, script, scriptPath, exit, workingDirectory,
-                ok ? null : output),
+                ShouldReturnOutput(script, succeeded: ok) ? output : null),
         };
     }
 
+    // Whether this run's output should travel back to the server.
+    //
+    // A FAILED run always returns it: the whole point of the field is making a
+    // failure diagnosable from the UI rather than by remoting to the box. A
+    // SUCCESSFUL run returns it only when the script's author opted in, because
+    // output carries whatever the body printed - hostnames, share paths, a
+    // token echoed while debugging - and a routine green run is not worth that
+    // exposure by default. A claim from a server too old to send the field
+    // leaves CaptureOutput false, so the behaviour is unchanged.
+    private static bool ShouldReturnOutput(ScriptDeliveryClaim? script, bool succeeded)
+        => !succeeded || (script?.CaptureOutput ?? false);
+
     private static Dictionary<string, object> ScriptDetail(
         string name, ScriptDeliveryClaim? script, string? scriptPath, int? exitCode,
         string? workingDirectory = null, string? output = null)
@@ -1313,12 +1574,12 @@ public sealed class CommandExecutor
             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.
+        // Always present on failure; on success only when the script opted in
+        // via captureoutput (see ShouldReturnOutput). Script output can carry
+        // whatever the body happened to print - hostnames, paths, tokens an
+        // author echoed while debugging - so a successful run stays silent
+        // unless someone decided otherwise after reading the body. The endpoint
+        // log keeps the full copy either way.
         if (!string.IsNullOrWhiteSpace(output))
             detail["output"] = output!;
         return detail;

agent/ShopDbEndpointAgent.Core/Contracts.cs

@@ -197,6 +197,13 @@ public sealed class AppDeliveryClaim
 // 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.
+// CaptureOutput decides whether a SUCCESSFUL run returns its output to the
+// server. Failures always return it, because a failure has to be diagnosable
+// without walking to the machine. Success is opt-in per script: output carries
+// whatever the body printed, and that is not worth persisting server-side for a
+// routine green run. The endpoint log keeps the full copy regardless, so this
+// only governs what crosses the wire. Absent means false, which is the same
+// behaviour as before the field existed.
 public sealed class ScriptDeliveryClaim
 {
     [JsonPropertyName("scriptid")] public int ScriptId { get; set; }
@@ -206,6 +213,7 @@ public sealed class ScriptDeliveryClaim
     [JsonPropertyName("runcontext")] public string? RunContext { get; set; }
     [JsonPropertyName("workingdirectory")] public string? WorkingDirectory { get; set; }
     [JsonPropertyName("errormode")] public string? ErrorMode { get; set; }
+    [JsonPropertyName("captureoutput")] public bool CaptureOutput { get; set; }
 }
 
 // Envelope for the claim long-poll: { status, data: { endpointcommandid, action } }.

agent/ShopDbEndpointAgent/ShopDbEndpointAgent.csproj

@@ -186,38 +186,54 @@
 		 member of that group, so Start-ScheduledTask returned access denied and
 		 the task only ever ran on its own trigger. Requires tray 1.4.0 - the tray
 		 is the side that listens, so an older tray simply never answers. -->
-	<!-- 1.13.1: coordinated release with tray 1.6.0 (fleet alert styling update).
-	     No agent code changes, but deployed together for version consistency. -->
-	<!-- 1.13.2: coordinated release with tray 1.6.3 (auto-close when inactive, crash fix).
-	     No agent code changes, but deployed together for version consistency. -->
-	<!-- 1.13.3: coordinated release with tray 1.6.4 (deadlock fix).
-	     No agent code changes, but deployed together for version consistency. -->
-	<!-- 1.13.4: coordinated release with tray 1.6.5 (resource leak fixes).
-	     No agent code changes, but deployed together for version consistency. -->
-	<!-- 1.13.5: coordinated release with tray 1.6.6 (no tray changes).
-	     No agent code changes, but deployed together for version consistency. -->
-	<!-- 1.14.0: truthful script verdicts and script output capture. 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 on, and
-		 still exits zero. An account-provisioning script therefore reported
-		 succeeded having provisioned nothing, and because no output was captured
-		 there was no evidence to contradict it. The claim now carries errormode
-		 (AGENT-CONTRACT.md 7.4); strict prepends $ErrorActionPreference='Stop'
-		 AFTER digest verification so the hash still validates delivered bytes.
-		 Default stays permissive so bodies authored against the old semantics
-		 keep working. 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 our pipes
-		 across the session boundary - always logged locally and returned to the
-		 server only on failure. Minor because the claim gains a field and an
-		 older agent ignoring it keeps today's behaviour. No tray change - tray
-		 stays 1.4.0. -->
-	<!-- 1.14.1: coordinated release with tray 1.6.7 (merged wj 1.14.1).
-	     Merge wilm 1.14.0 changes with wj tray coordination releases. -->
-	<!-- 1.14.2: merge wilm fleet alert scope fixes and operator report enhancements.
-	     Brings in wilm's alertscope targeting and _alert_targets_endpoint function,
-	     along with wj's operator report email improvements (FQDN, machine relationship). -->
-	<Version>1.14.2</Version>
+<!-- 1.13.1: coordinated release with tray 1.6.0 (fleet alert styling update).
+	 No agent code changes, but deployed together for version consistency. -->
+<!-- 1.13.2: coordinated release with tray 1.6.3 (auto-close when inactive, crash fix).
+	 No agent code changes, but deployed together for version consistency. -->
+<!-- 1.13.3: coordinated release with tray 1.6.4 (deadlock fix).
+	 No agent code changes, but deployed together for version consistency. -->
+<!-- 1.13.4: coordinated release with tray 1.6.5 (resource leak fixes).
+	 No agent code changes, but deployed together for version consistency. -->
+<!-- 1.14.0: truthful script verdicts and script output capture. 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 on, and
+	 still exits zero. An account-provisioning script therefore reported
+	 succeeded having provisioned nothing, and because no output was captured
+	 there was no evidence to contradict it. The claim now carries errormode
+	 (AGENT-CONTRACT.md 7.4); strict prepends $ErrorActionPreference='Stop'
+	 AFTER digest verification so the hash still validates delivered bytes.
+	 Default stays permissive so bodies authored against the old semantics
+	 keep working. 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 our pipes
+	 across the session boundary - always logged locally and returned to the
+	 server only on failure. Minor because the claim gains a field and an
+	 older agent ignoring it keeps today's behaviour. Supersedes the 1.13.1
+	 to 1.13.4 tray-coordination bumps merged from wj, which carried no agent
+	 code changes. Tray is at 1.6.5 from wj. -->
+	<!-- 1.15.0: successful runs can now return their output to the server, gated
+	 by a per-script captureoutput flag on the claim. Failures still always
+	 return output; success previously never did, which left a green badge as
+	 the only evidence a run produced and no way to tell "did the work" from
+	 "PowerShell exited cleanly". Opt-in rather than always-on because output
+	 carries whatever the body printed, and that is not worth persisting
+	 server-side for every routine run. Minor because the claim gains a field
+	 that defaults false: an agent or server too old to know it keeps today's
+	 withholding behaviour, which is the safe direction to fail. -->
+	<!-- 1.16.0: adds collect_log, which reads a plaintext log file from a
+	 caller-supplied path and returns its contents for the Log Collector tab.
+	 Strictly read-only: opened for reading with FileShare.ReadWrite|Delete,
+	 because the logs worth collecting are the ones a running application
+	 currently holds open and a stricter share mode fails on exactly those.
+	 Bounded rather than unlimited - only the last 2 MB is returned, and only
+	 for an allowlisted set of text extensions; a binary or oversized file is
+	 refused rather than mangled into an unreadable pane. The path is
+	 re-validated here even though the server already checked it, since this
+	 process runs as SYSTEM and the path arrived over the network: environment
+	 variables are expanded BEFORE the checks so %VAR% cannot smuggle a
+	 traversal or UNC path past them. Minor because it is a new action name -
+	 an older agent asked for it fails closed on the unknown-action default. -->
+<Version>1.16.0</Version>
 	<InvariantGlobalization>true</InvariantGlobalization>
 	<RuntimeIdentifier>win-x64</RuntimeIdentifier>
 	<SelfContained>true</SelfContained>