diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditCommands.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditCommands.cs index fdfc6968..112550ea 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditCommands.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditCommands.cs @@ -218,16 +218,32 @@ public static class AuditCommands Actor = result.GetValue(actorOption), }; + // Ctrl-C is the one way to abort an in-flight export other than the + // headers/inactivity deadlines below — mirrors the CancellationTokenSource + // owned by DebugCommands' streaming debug-view command (arch-review + // 2026-08-19 P6-8: this was the one CLI HTTP path with NO deadline at all). + using var cts = new CancellationTokenSource(); + ConsoleCancelEventHandler onCancel = (_, e) => + { + e.Cancel = true; + cts.Cancel(); + }; + Console.CancelKeyPress += onCancel; + try { using var client = new ManagementHttpClient(connection.Url!, connection.Username!, connection.Password!); - return await AuditExportHelpers.RunExportAsync(client, args, Console.Out, DateTimeOffset.UtcNow); + return await AuditExportHelpers.RunExportAsync(client, args, Console.Out, DateTimeOffset.UtcNow, cts.Token); } catch (FormatException ex) { OutputFormatter.WriteError(ex.Message, "INVALID_ARGUMENT"); return 1; } + finally + { + Console.CancelKeyPress -= onCancel; + } }); return cmd; } diff --git a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditExportHelpers.cs b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditExportHelpers.cs index 72e76a3b..08dbf343 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditExportHelpers.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CLI/Commands/AuditExportHelpers.cs @@ -106,10 +106,34 @@ public static class AuditExportHelpers return "?" + string.Join("&", parts); } + /// + /// Deadline for receiving the export response's headers (i.e. the server has + /// started responding), measured from the start of the request. This is the + /// one bound does NOT + /// carry on its own — 's HttpClient.Timeout + /// is deliberately (see its doc comment) + /// and bounds only the TCP/TLS + /// connect phase, not a connected-but-silent server. Reuses + /// as the standard CLI + /// per-call default (arch-review 2026-08-19 P6-8). + /// + public static readonly TimeSpan DefaultHeadersTimeout = ManagementHttpClient.DefaultConnectTimeout; + + /// + /// Inactivity deadline for the streaming body copy: reset on every chunk + /// successfully read, so a large export keeps streaming indefinitely as long + /// as bytes keep arriving, but a stream that goes silent mid-transfer (a + /// hung/crashed server holding the connection open) is bounded rather than + /// hanging the CLI forever — the residual gap the timeout-honesty fix + /// (ManagementHttpClient.DefaultConnectTimeout doc comment) left in this + /// one path (arch-review 2026-08-19 P6-8). + /// + public static readonly TimeSpan DefaultInactivityTimeout = ManagementHttpClient.DefaultConnectTimeout; + /// /// Executes the export: GETs /api/audit/export and copies the response body /// stream directly to . The body is never fully - /// buffered — streams in fixed-size chunks. + /// buffered — the copy streams in fixed-size chunks via . /// A 501 Not Implemented (parquet not yet supported server-side) prints the /// server message and returns a non-zero exit code. /// @@ -117,16 +141,43 @@ public static class AuditExportHelpers /// The export arguments containing filters and output file path. /// Text writer for command output messages. /// The current time for resolving relative time specifications. + /// + /// External cancellation (e.g. the CLI's Ctrl-C handler) — cancelling this + /// stops the export promptly at either the headers wait or the next stream + /// read, with a "cancelled" (not timed-out) message. + /// + /// + /// Override for — test seam. + /// + /// + /// Override for — test seam. + /// /// 0 on success, 1 on general error, or 2 on authorization failure. public static async Task RunExportAsync( - ManagementHttpClient client, AuditExportArgs args, TextWriter output, DateTimeOffset now) + ManagementHttpClient client, AuditExportArgs args, TextWriter output, DateTimeOffset now, + CancellationToken cancellationToken = default, + TimeSpan? headersTimeout = null, + TimeSpan? inactivityTimeout = null) { var qs = BuildQueryString(args, now); + using var headersCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + headersCts.CancelAfter(headersTimeout ?? DefaultHeadersTimeout); + HttpResponseMessage response; try { - response = await client.SendGetStreamAsync("api/audit/export" + qs, CancellationToken.None); + response = await client.SendGetStreamAsync("api/audit/export" + qs, headersCts.Token); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + OutputFormatter.WriteError("Export cancelled.", "CANCELLED"); + return 1; + } + catch (OperationCanceledException) + { + OutputFormatter.WriteError("Request timed out waiting for the server to respond.", "TIMEOUT"); + return 1; } catch (HttpRequestException ex) { @@ -138,7 +189,7 @@ public static class AuditExportHelpers { if (response.StatusCode == HttpStatusCode.NotImplemented) { - var message = await response.Content.ReadAsStringAsync(); + var message = await response.Content.ReadAsStringAsync(cancellationToken); OutputFormatter.WriteError( string.IsNullOrWhiteSpace(message) ? "Export format not implemented by the server." @@ -149,7 +200,7 @@ public static class AuditExportHelpers if (!response.IsSuccessStatusCode) { - var message = await response.Content.ReadAsStringAsync(); + var message = await response.Content.ReadAsStringAsync(cancellationToken); // Honour the documented "authorization failure → exit 2" // contract on the REST audit surface as well. HTTP 403 is the // primary signal; the server may also surface UNAUTHORIZED / @@ -164,17 +215,69 @@ public static class AuditExportHelpers return isAuthFailure ? 2 : 1; } - await using var source = await response.Content.ReadAsStreamAsync(); + await using var source = await response.Content.ReadAsStreamAsync(cancellationToken); await using var destination = new FileStream( args.Output, FileMode.Create, FileAccess.Write, FileShare.None, bufferSize: 81920, useAsync: true); - await source.CopyToAsync(destination); + + try + { + await CopyWithInactivityTimeoutAsync( + source, destination, inactivityTimeout ?? DefaultInactivityTimeout, cancellationToken); + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + OutputFormatter.WriteError($"Export cancelled; partial file written to {args.Output}.", "CANCELLED"); + return 1; + } + catch (OperationCanceledException) + { + OutputFormatter.WriteError( + "Export stalled — no data received from the server for " + + $"{(inactivityTimeout ?? DefaultInactivityTimeout).TotalSeconds:0}s; " + + $"partial file written to {args.Output}.", + "TIMEOUT"); + return 1; + } + catch (Exception ex) when (ex is IOException or HttpRequestException) + { + OutputFormatter.WriteError( + $"Export failed while streaming the response: {ex.Message}; partial file written to {args.Output}.", + "STREAM_FAILED"); + return 1; + } } output.WriteLine($"Exported audit log to {args.Output}"); return 0; } + /// + /// Copies to in fixed-size + /// chunks, rearming an inactivity deadline before every read so the copy is bounded + /// only by silence, not by total size — a multi-hundred-MB export keeps streaming as + /// long as bytes keep arriving. (external, e.g. + /// Ctrl-C) and the inactivity deadline share one linked token; callers distinguish the + /// two causes via cancellationToken.IsCancellationRequested after catching + /// . + /// + private static async Task CopyWithInactivityTimeoutAsync( + Stream source, Stream destination, TimeSpan inactivityTimeout, CancellationToken cancellationToken) + { + using var readCts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + var buffer = new byte[81920]; + while (true) + { + readCts.CancelAfter(inactivityTimeout); + var bytesRead = await source.ReadAsync(buffer.AsMemory(), readCts.Token); + if (bytesRead == 0) + { + break; + } + await destination.WriteAsync(buffer.AsMemory(0, bytesRead), cancellationToken); + } + } + /// /// Best-effort parse of the server's JSON error envelope ({ "error": ..., "code": ... }) /// to extract the code field. Returns null if the body is empty, not valid JSON, or diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/AuditLogExportService.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/AuditLogExportService.cs index c046ce89..ab1c917e 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/AuditLogExportService.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/AuditLogExportService.cs @@ -20,8 +20,9 @@ namespace ZB.MOM.WW.ScadaBridge.CentralUI.Services; /// Output is capped at a caller-supplied maxRows ceiling; when the cap /// is hit the service appends a # Capped at … rows. Use the CLI for larger /// exports. footer line so an operator can tell a truncated download from -/// a complete one. The header row contains the 21 columns of -/// in declaration order. +/// a complete one. The header row contains the same 24 columns, in the same +/// order, as the ManagementService CLI's CSV export (arch-review 2026-08-19 +/// P6-7 — the two CSV writers were found to have drifted and are kept aligned). /// /// public interface IAuditLogExportService @@ -92,7 +93,7 @@ public sealed class AuditLogExportService : IAuditLogExportService leaveOpen: true); writer.NewLine = "\r\n"; // RFC 4180 - // Header — 21 columns in AuditEvent declaration order. + // Header — 24 columns in AuditEventView declaration order. await writer.WriteLineAsync(Header); int written = 0; @@ -160,13 +161,14 @@ public sealed class AuditLogExportService : IAuditLogExportService } // ───────────────────────────────────────────────────────────────────── - // CSV helpers + // CSV helpers — 24 columns, kept aligned with the ManagementService CLI's + // CSV export (arch-review 2026-08-19 P6-7) // ───────────────────────────────────────────────────────────────────── - /// The 21 column names in declaration order. + /// The 24 column names, in declaration order. internal const string Header = - "EventId,OccurredAtUtc,IngestedAtUtc,Channel,Kind,CorrelationId," + - "SourceSiteId,SourceInstanceId,SourceScript,Actor,Target,Status," + + "EventId,OccurredAtUtc,IngestedAtUtc,Channel,Kind,CorrelationId,ExecutionId,ParentExecutionId," + + "SourceSiteId,SourceNode,SourceInstanceId,SourceScript,Actor,Target,Status," + "HttpStatus,DurationMs,ErrorMessage,ErrorDetail,RequestSummary," + "ResponseSummary,PayloadTruncated,Extra,ForwardState"; @@ -174,7 +176,12 @@ public sealed class AuditLogExportService : IAuditLogExportService /// Serialises one as a CSV row (no trailing newline). /// Each nullable column renders as the empty string when null; non-null /// scalars use invariant culture so an export taken on one locale parses - /// cleanly on another. + /// cleanly on another. Free-text columns are run through + /// before RFC 4180 quoting so a + /// leading =/+/-/@/TAB/CR can't be interpreted as + /// a formula or DDE trigger by a spreadsheet application (CWE-1236; + /// arch-review 2026-08-19 P6-7). GUID/enum/numeric/date/bool columns this + /// formatter generates itself are never neutralized. /// /// The audit event view to format as a CSV row. /// An RFC 4180 CSV row string (no trailing newline) for the supplied event. @@ -187,20 +194,23 @@ public sealed class AuditLogExportService : IAuditLogExportService AppendField(sb, evt.Channel.ToString()); AppendField(sb, evt.Kind.ToString()); AppendField(sb, evt.CorrelationId?.ToString()); - AppendField(sb, evt.SourceSiteId); - AppendField(sb, evt.SourceInstanceId); - AppendField(sb, evt.SourceScript); - AppendField(sb, evt.Actor); - AppendField(sb, evt.Target); + AppendField(sb, evt.ExecutionId?.ToString()); + AppendField(sb, evt.ParentExecutionId?.ToString()); + AppendField(sb, evt.SourceSiteId, freeText: true); + AppendField(sb, evt.SourceNode, freeText: true); + AppendField(sb, evt.SourceInstanceId, freeText: true); + AppendField(sb, evt.SourceScript, freeText: true); + AppendField(sb, evt.Actor, freeText: true); + AppendField(sb, evt.Target, freeText: true); AppendField(sb, evt.Status.ToString()); AppendField(sb, evt.HttpStatus?.ToString(CultureInfo.InvariantCulture)); AppendField(sb, evt.DurationMs?.ToString(CultureInfo.InvariantCulture)); - AppendField(sb, evt.ErrorMessage); - AppendField(sb, evt.ErrorDetail); - AppendField(sb, evt.RequestSummary); - AppendField(sb, evt.ResponseSummary); + AppendField(sb, evt.ErrorMessage, freeText: true); + AppendField(sb, evt.ErrorDetail, freeText: true); + AppendField(sb, evt.RequestSummary, freeText: true); + AppendField(sb, evt.ResponseSummary, freeText: true); AppendField(sb, evt.PayloadTruncated.ToString()); - AppendField(sb, evt.Extra); + AppendField(sb, evt.Extra, freeText: true); AppendField(sb, evt.ForwardState?.ToString()); return sb.ToString(); } @@ -211,7 +221,7 @@ public sealed class AuditLogExportService : IAuditLogExportService private static string FormatDate(DateTime value) => value.ToString("O", CultureInfo.InvariantCulture); - private static void AppendField(StringBuilder sb, string? value, bool first = false) + private static void AppendField(StringBuilder sb, string? value, bool first = false, bool freeText = false) { if (!first) sb.Append(','); if (string.IsNullOrEmpty(value)) @@ -219,6 +229,14 @@ public sealed class AuditLogExportService : IAuditLogExportService return; } + // OWASP CSV-injection neutralization (CWE-1236): prefix a literal single + // quote before RFC 4180 quoting so a free-text cell starting with a + // formula/DDE trigger character renders as inert text in Excel. + if (freeText && NeedsFormulaNeutralization(value)) + { + value = "'" + value; + } + // RFC 4180: quote on comma / quote / CR / LF; double-up embedded quotes. bool needsQuoting = value.IndexOfAny(s_quoteTriggers) >= 0; if (!needsQuoting) @@ -242,5 +260,16 @@ public sealed class AuditLogExportService : IAuditLogExportService sb.Append('"'); } + /// + /// True when starts with a character a spreadsheet + /// application would treat as a formula/DDE-launch trigger — the OWASP CSV + /// Injection standard set: =, +, -, @, TAB, CR. + /// + private static bool NeedsFormulaNeutralization(string value) + { + var c = value[0]; + return c == '=' || c == '+' || c == '-' || c == '@' || c == '\t' || c == '\r'; + } + private static readonly char[] s_quoteTriggers = { ',', '"', '\r', '\n' }; } diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/AuditEndpoints.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/AuditEndpoints.cs index cd086194..ac7f3c12 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/AuditEndpoints.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/AuditEndpoints.cs @@ -760,17 +760,30 @@ public static class AuditEndpoints } // ───────────────────────────────────────────────────────────────────── - // CSV helpers — 21 columns in AuditEvent declaration order + // CSV helpers — 24 columns, same order/naming as the JSONL export + // (AuditExportRow property declaration order; arch-review 2026-08-19 P6-7) // ───────────────────────────────────────────────────────────────────── public const string CsvHeader = - "EventId,OccurredAtUtc,IngestedAtUtc,Channel,Kind,CorrelationId," + - "SourceSiteId,SourceInstanceId,SourceScript,Actor,Target,Status," + + "EventId,OccurredAtUtc,IngestedAtUtc,Channel,Kind,CorrelationId,ExecutionId,ParentExecutionId," + + "SourceSiteId,SourceNode,SourceInstanceId,SourceScript,Actor,Target,Status," + "HttpStatus,DurationMs,ErrorMessage,ErrorDetail,RequestSummary," + "ResponseSummary,PayloadTruncated,Extra,ForwardState"; /// /// Formats a single as an RFC 4180 CSV row matching . + /// Free-text columns (anything that can carry inbound-API-caller-controlled or + /// site-script-controlled text — , + /// , , + /// , , + /// , , + /// , , + /// , ) are run + /// through before RFC 4180 quoting so a leading + /// =/+/-/@/TAB/CR can't be interpreted as a formula or DDE + /// trigger by a spreadsheet application (CWE-1236). GUID/enum/numeric/date/bool columns the + /// exporter formats itself are never neutralized — none of them can start with a trigger + /// character, and neutralizing a signed numeric column would corrupt it. /// /// The audit event to format. /// An RFC 4180 CSV row string (no trailing newline) for the supplied event. @@ -783,20 +796,23 @@ public static class AuditEndpoints AppendField(sb, evt.Channel.ToString()); AppendField(sb, evt.Kind.ToString()); AppendField(sb, evt.CorrelationId?.ToString()); - AppendField(sb, evt.SourceSiteId); - AppendField(sb, evt.SourceInstanceId); - AppendField(sb, evt.SourceScript); - AppendField(sb, evt.Actor); - AppendField(sb, evt.Target); + AppendField(sb, evt.ExecutionId?.ToString()); + AppendField(sb, evt.ParentExecutionId?.ToString()); + AppendField(sb, evt.SourceSiteId, freeText: true); + AppendField(sb, evt.SourceNode, freeText: true); + AppendField(sb, evt.SourceInstanceId, freeText: true); + AppendField(sb, evt.SourceScript, freeText: true); + AppendField(sb, evt.Actor, freeText: true); + AppendField(sb, evt.Target, freeText: true); AppendField(sb, evt.Status.ToString()); AppendField(sb, evt.HttpStatus?.ToString(CultureInfo.InvariantCulture)); AppendField(sb, evt.DurationMs?.ToString(CultureInfo.InvariantCulture)); - AppendField(sb, evt.ErrorMessage); - AppendField(sb, evt.ErrorDetail); - AppendField(sb, evt.RequestSummary); - AppendField(sb, evt.ResponseSummary); + AppendField(sb, evt.ErrorMessage, freeText: true); + AppendField(sb, evt.ErrorDetail, freeText: true); + AppendField(sb, evt.RequestSummary, freeText: true); + AppendField(sb, evt.ResponseSummary, freeText: true); AppendField(sb, evt.PayloadTruncated.ToString()); - AppendField(sb, evt.Extra); + AppendField(sb, evt.Extra, freeText: true); AppendField(sb, evt.ForwardState?.ToString()); return sb.ToString(); } @@ -807,7 +823,7 @@ public static class AuditEndpoints private static string FormatDate(DateTime value) => value.ToString("O", CultureInfo.InvariantCulture); - private static void AppendField(StringBuilder sb, string? value, bool first = false) + private static void AppendField(StringBuilder sb, string? value, bool first = false, bool freeText = false) { if (!first) sb.Append(','); if (string.IsNullOrEmpty(value)) @@ -815,6 +831,16 @@ public static class AuditEndpoints return; } + // OWASP CSV-injection neutralization (CWE-1236): a free-text cell whose + // first character would be interpreted by a spreadsheet application as a + // formula/DDE trigger is prefixed with a literal single quote. Applied + // BEFORE RFC 4180 quoting, and only to free-text columns — never to the + // GUID/enum/numeric/date/bool columns this formatter generates itself. + if (freeText && NeedsFormulaNeutralization(value)) + { + value = "'" + value; + } + // RFC 4180: quote on comma / quote / CR / LF; double-up embedded quotes. if (value.IndexOfAny(s_quoteTriggers) < 0) { @@ -837,5 +863,16 @@ public static class AuditEndpoints sb.Append('"'); } + /// + /// True when starts with a character a spreadsheet + /// application would treat as a formula/DDE-launch trigger — the OWASP CSV + /// Injection standard set: =, +, -, @, TAB, CR. + /// + private static bool NeedsFormulaNeutralization(string value) + { + var c = value[0]; + return c == '=' || c == '+' || c == '-' || c == '@' || c == '\t' || c == '\r'; + } + private static readonly char[] s_quoteTriggers = { ',', '"', '\r', '\n' }; } diff --git a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/Commands/AuditExportCommandTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/Commands/AuditExportCommandTests.cs index 3d23316a..fc97dc64 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/Commands/AuditExportCommandTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/Commands/AuditExportCommandTests.cs @@ -1,3 +1,4 @@ +using System.Diagnostics; using System.Net; using System.Text; using System.Web; @@ -376,4 +377,256 @@ public class AuditExportCommandTests public override void SetLength(long value) => throw new NotSupportedException(); public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); } + + // ───────────────────────────────────────────────────────────────────── + // Deadline + Ctrl-C cancellation (arch-review 2026-08-19 P6-8: audit export + // was the one CLI HTTP path with NO deadline — a hung server hung the CLI + // forever). Every test below bounds its own wait well under what the old, + // deadline-less implementation would have done (indefinite hang) — a + // regression back to that state fails these on a timeout, proving the fix. + // ───────────────────────────────────────────────────────────────────── + + /// A handler that never completes the request until its fires — simulates a server that accepted the connection but never answers. + private sealed class HangingHandler : HttpMessageHandler + { + public int CallCount { get; private set; } + + protected override async Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + CallCount++; + await Task.Delay(Timeout.InfiniteTimeSpan, cancellationToken); + throw new InvalidOperationException("unreachable — Task.Delay(Infinite) only returns via cancellation"); + } + } + + /// + /// A stream that serves one small chunk, then goes silent — any subsequent + /// blocks until the + /// caller's token cancels. Simulates a connection that stalls mid-transfer. + /// + private sealed class StallingStream : Stream + { + private readonly byte[] _firstChunk; + private bool _servedFirstChunk; + + public StallingStream(byte[] firstChunk) => _firstChunk = firstChunk; + + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => throw new NotSupportedException(); + public override long Position { get => throw new NotSupportedException(); set => throw new NotSupportedException(); } + + public override int Read(byte[] buffer, int offset, int count) => + ReadAsync(buffer.AsMemory(offset, count)).AsTask().GetAwaiter().GetResult(); + + public override async ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) + { + if (!_servedFirstChunk) + { + _servedFirstChunk = true; + var n = Math.Min(buffer.Length, _firstChunk.Length); + _firstChunk.AsSpan(0, n).CopyTo(buffer.Span); + return n; + } + + await Task.Delay(Timeout.InfiniteTimeSpan, cancellationToken); + return 0; // unreachable — Task.Delay(Infinite) only returns via cancellation + } + + public override void Flush() { } + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + public override void SetLength(long value) => throw new NotSupportedException(); + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + } + + /// A stream that serves one small chunk, then throws — simulates a connection reset mid-transfer. + private sealed class FaultingStream : Stream + { + private readonly byte[] _firstChunk; + private bool _servedFirstChunk; + + public FaultingStream(byte[] firstChunk) => _firstChunk = firstChunk; + + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => throw new NotSupportedException(); + public override long Position { get => throw new NotSupportedException(); set => throw new NotSupportedException(); } + + public override int Read(byte[] buffer, int offset, int count) => + ReadAsync(buffer.AsMemory(offset, count)).AsTask().GetAwaiter().GetResult(); + + public override ValueTask ReadAsync(Memory buffer, CancellationToken cancellationToken = default) + { + if (!_servedFirstChunk) + { + _servedFirstChunk = true; + var n = Math.Min(buffer.Length, _firstChunk.Length); + _firstChunk.AsSpan(0, n).CopyTo(buffer.Span); + return new ValueTask(n); + } + + throw new IOException("connection reset by peer (simulated)"); + } + + public override void Flush() { } + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + public override void SetLength(long value) => throw new NotSupportedException(); + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + } + + /// Redirects for the duration of the callback and returns what was written. + private static async Task CaptureStderrAsync(Func action) + { + var original = Console.Error; + var capture = new StringWriter(); + Console.SetError(capture); + try + { + await action(); + } + finally + { + Console.SetError(original); + } + return capture.ToString(); + } + + [Fact] + public async Task RunExport_ServerNeverSendsHeaders_TimesOutWithFriendlyError() + { + var path = Path.Combine(Path.GetTempPath(), $"audit-export-hang-{Guid.NewGuid():N}.csv"); + var handler = new HangingHandler(); + var client = new ManagementHttpClient(new HttpClient(handler), "http://localhost:9001", "u", "p"); + var output = new StringWriter(); + var sw = Stopwatch.StartNew(); + + var exit = 0; + var stderr = await CaptureStderrAsync(async () => + { + exit = await AuditExportHelpers.RunExportAsync( + client, + new AuditExportArgs { Since = "1h", Until = "0h", Format = "csv", Output = path }, + output, DateTimeOffset.UtcNow, + headersTimeout: TimeSpan.FromMilliseconds(200)); + }); + sw.Stop(); + + Assert.Equal(1, exit); + // Bounded well under the old infinite-hang behaviour. + Assert.True(sw.Elapsed < TimeSpan.FromSeconds(5), $"took {sw.Elapsed}"); + Assert.Contains("TIMEOUT", stderr); + Assert.False(File.Exists(path)); + } + + [Fact] + public async Task RunExport_StreamStallsMidTransfer_InactivityTimeoutFires() + { + var path = Path.Combine(Path.GetTempPath(), $"audit-export-stall-{Guid.NewGuid():N}.csv"); + try + { + var firstChunk = Encoding.UTF8.GetBytes("EventId,OccurredAtUtc\n"); + var handler = new BodyHandler(HttpStatusCode.OK, + () => new StreamContent(new StallingStream(firstChunk))); + var client = new ManagementHttpClient(new HttpClient(handler), "http://localhost:9001", "u", "p"); + var output = new StringWriter(); + var sw = Stopwatch.StartNew(); + + var exit = 0; + var stderr = await CaptureStderrAsync(async () => + { + exit = await AuditExportHelpers.RunExportAsync( + client, + new AuditExportArgs { Since = "1h", Until = "0h", Format = "csv", Output = path }, + output, DateTimeOffset.UtcNow, + inactivityTimeout: TimeSpan.FromMilliseconds(200)); + }); + sw.Stop(); + + Assert.Equal(1, exit); + Assert.True(sw.Elapsed < TimeSpan.FromSeconds(5), $"took {sw.Elapsed}"); + Assert.Contains("TIMEOUT", stderr); + // Clean message, not a raw stack trace, and it notes the partial file. + Assert.DoesNotContain(" at ", stderr); + Assert.Contains(path, stderr); + // The first chunk that DID arrive before the stall must have been + // flushed to disk — this is a partial, not empty, export. + Assert.True(File.Exists(path)); + Assert.Equal(firstChunk, await File.ReadAllBytesAsync(path)); + } + finally + { + if (File.Exists(path)) File.Delete(path); + } + } + + [Fact] + public async Task RunExport_MidStreamIOException_ProducesCleanMessageNotStackTrace() + { + var path = Path.Combine(Path.GetTempPath(), $"audit-export-fault-{Guid.NewGuid():N}.csv"); + try + { + var firstChunk = Encoding.UTF8.GetBytes("EventId,OccurredAtUtc\n"); + var handler = new BodyHandler(HttpStatusCode.OK, + () => new StreamContent(new FaultingStream(firstChunk))); + var client = new ManagementHttpClient(new HttpClient(handler), "http://localhost:9001", "u", "p"); + var output = new StringWriter(); + + var exit = 0; + var stderr = await CaptureStderrAsync(async () => + { + exit = await AuditExportHelpers.RunExportAsync( + client, + new AuditExportArgs { Since = "1h", Until = "0h", Format = "csv", Output = path }, + output, DateTimeOffset.UtcNow); + }); + + Assert.Equal(1, exit); + Assert.Contains("STREAM_FAILED", stderr); + Assert.Contains("connection reset by peer", stderr); + Assert.Contains(path, stderr); + Assert.DoesNotContain(" at ", stderr); + Assert.DoesNotContain(nameof(FaultingStream), stderr); + } + finally + { + if (File.Exists(path)) File.Delete(path); + } + } + + [Fact] + public async Task RunExport_CtrlCToken_CancelsPromptlyWithCleanMessage() + { + var path = Path.Combine(Path.GetTempPath(), $"audit-export-ctrlc-{Guid.NewGuid():N}.csv"); + var handler = new HangingHandler(); + var client = new ManagementHttpClient(new HttpClient(handler), "http://localhost:9001", "u", "p"); + var output = new StringWriter(); + using var cts = new CancellationTokenSource(); + var sw = Stopwatch.StartNew(); + + var exit = 0; + var stderr = await CaptureStderrAsync(async () => + { + var task = AuditExportHelpers.RunExportAsync( + client, + new AuditExportArgs { Since = "1h", Until = "0h", Format = "csv", Output = path }, + output, DateTimeOffset.UtcNow, + cancellationToken: cts.Token, + // Deliberately much longer than the Ctrl-C delay below, so a pass + // here proves cancellation — not the timeout — ended the call. + headersTimeout: TimeSpan.FromSeconds(30)); + + await Task.Delay(50); + cts.Cancel(); + exit = await task; + }); + sw.Stop(); + + Assert.Equal(1, exit); + Assert.True(sw.Elapsed < TimeSpan.FromSeconds(5), $"took {sw.Elapsed}"); + Assert.Contains("CANCELLED", stderr); + Assert.False(File.Exists(path)); + } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Services/AuditLogExportServiceTests.cs b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Services/AuditLogExportServiceTests.cs index c78633b0..72c1c76e 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Services/AuditLogExportServiceTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Services/AuditLogExportServiceTests.cs @@ -68,14 +68,14 @@ public class AuditLogExportServiceTests // 1 header + 5 rows + trailing empty token from final \r\n = 7 entries. Assert.Equal(7, lines.Length); - Assert.StartsWith("EventId,OccurredAtUtc,IngestedAtUtc,Channel,Kind,CorrelationId,SourceSiteId,", lines[0]); + Assert.StartsWith("EventId,OccurredAtUtc,IngestedAtUtc,Channel,Kind,CorrelationId,ExecutionId,ParentExecutionId,SourceSiteId,SourceNode,", lines[0]); Assert.StartsWith("11111111-1111-1111-1111-111111111111,", lines[1]); Assert.StartsWith("55555555-5555-5555-5555-555555555555,", lines[5]); Assert.Equal(string.Empty, lines[6]); } [Fact] - public async Task ExportAsync_HeaderHasAll21Columns_InSpecOrder() + public async Task ExportAsync_HeaderHasAll24Columns_InSpecOrder() { var repo = Substitute.For(); repo.QueryAsync(Arg.Any(), Arg.Any(), Arg.Any()) @@ -89,11 +89,14 @@ public class AuditLogExportServiceTests var header = csv.Split("\r\n")[0]; var columns = header.Split(','); - Assert.Equal(21, columns.Length); + // arch-review 2026-08-19 P6-7: ExecutionId/ParentExecutionId/SourceNode + // were missing from this CSV (JSON/JSONL exports already carried them). + Assert.Equal(24, columns.Length); Assert.Equal(new[] { "EventId", "OccurredAtUtc", "IngestedAtUtc", "Channel", "Kind", - "CorrelationId", "SourceSiteId", "SourceInstanceId", "SourceScript", + "CorrelationId", "ExecutionId", "ParentExecutionId", + "SourceSiteId", "SourceNode", "SourceInstanceId", "SourceScript", "Actor", "Target", "Status", "HttpStatus", "DurationMs", "ErrorMessage", "ErrorDetail", "RequestSummary", "ResponseSummary", "PayloadTruncated", "Extra", "ForwardState", @@ -136,6 +139,77 @@ public class AuditLogExportServiceTests Assert.Contains("\"boom\r\nthen again\"", csv); } + [Fact] + public async Task ExportAsync_FreeTextLeadingFormulaCharacters_ArePrefixedWithQuote() + { + // arch-review 2026-08-19 P6-7 (CWE-1236): a free-text cell starting with + // =, +, -, @, or TAB must be prefixed with a literal single quote so a + // spreadsheet application renders it as inert text, not a formula/DDE + // launch. Target, ErrorMessage, ResponseSummary, and Extra are + // inbound-API-caller-controlled and are the primary attack surface. + var row = ScadaBridgeAuditEventFactory.Create( + channel: AuditChannel.ApiOutbound, + kind: AuditKind.ApiCall, + status: AuditStatus.Delivered, + eventId: Guid.Parse("cccccccc-cccc-cccc-cccc-cccccccccccc"), + occurredAtUtc: new DateTime(2026, 5, 20, 12, 0, 0, DateTimeKind.Utc), + target: "=HYPERLINK(\"http://evil\",\"click\")", + errorMessage: "+cmd|'/c calc'!A1", + responseSummary: "-2+3", + extra: "@SUM(A1:A9)"); + var repo = Substitute.For(); + repo.QueryAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns( + Task.FromResult>(new[] { row }), + Task.FromResult>(Array.Empty())); + + var sut = new AuditLogExportService(repo); + using var ms = new MemoryStream(); + await sut.ExportAsync(new AuditLogQueryFilter(), maxRows: 10, ms, CancellationToken.None); + + var csv = Encoding.UTF8.GetString(ms.ToArray()); + var dataLine = csv.Split("\r\n")[1]; + + Assert.Contains("'=HYPERLINK(\"\"http://evil\"\",\"\"click\"\")", dataLine); + Assert.Contains("'+cmd|'/c calc'!A1", dataLine); + Assert.Contains(",'-2+3,", dataLine); + Assert.Contains(",'@SUM(A1:A9)", dataLine); + } + + [Fact] + public async Task ExportAsync_NumericAndDateColumns_NeverPrefixed() + { + // HttpStatus/DurationMs/OccurredAtUtc are formatted by the exporter + // itself (never free text) and must render unmangled even when a + // negative DurationMs would otherwise look like a formula-neutralized + // "-" leader if the neutralization were mistakenly applied there. + var row = ScadaBridgeAuditEventFactory.Create( + channel: AuditChannel.ApiOutbound, + kind: AuditKind.ApiCall, + status: AuditStatus.Delivered, + eventId: Guid.Parse("dddddddd-dddd-dddd-dddd-dddddddddddd"), + occurredAtUtc: new DateTime(2026, 5, 20, 12, 0, 0, DateTimeKind.Utc), + httpStatus: 200, + durationMs: -5); + var repo = Substitute.For(); + repo.QueryAsync(Arg.Any(), Arg.Any(), Arg.Any()) + .Returns( + Task.FromResult>(new[] { row }), + Task.FromResult>(Array.Empty())); + + var sut = new AuditLogExportService(repo); + using var ms = new MemoryStream(); + await sut.ExportAsync(new AuditLogQueryFilter(), maxRows: 10, ms, CancellationToken.None); + + var csv = Encoding.UTF8.GetString(ms.ToArray()); + var dataLine = csv.Split("\r\n")[1]; + var fields = dataLine.Split(','); + + Assert.Equal("200", fields[15]); // HttpStatus, unprefixed + Assert.Equal("-5", fields[16]); // DurationMs, unprefixed despite leading '-' + Assert.StartsWith("2026-05-20", fields[1]); // OccurredAtUtc, unprefixed + } + [Fact] public async Task ExportAsync_NullField_WrittenAsEmpty() { @@ -161,19 +235,23 @@ public class AuditLogExportServiceTests var fields = dataLine.Split(','); // EventId(0), OccurredAtUtc(1), IngestedAtUtc(2), Channel(3), Kind(4), - // CorrelationId(5), SourceSiteId(6), SourceInstanceId(7), SourceScript(8), - // Actor(9), Target(10), Status(11), HttpStatus(12), DurationMs(13), - // ErrorMessage(14), ErrorDetail(15), RequestSummary(16), ResponseSummary(17), - // PayloadTruncated(18), Extra(19), ForwardState(20) - Assert.Equal(21, fields.Length); + // CorrelationId(5), ExecutionId(6), ParentExecutionId(7), SourceSiteId(8), + // SourceNode(9), SourceInstanceId(10), SourceScript(11), Actor(12), + // Target(13), Status(14), HttpStatus(15), DurationMs(16), ErrorMessage(17), + // ErrorDetail(18), RequestSummary(19), ResponseSummary(20), + // PayloadTruncated(21), Extra(22), ForwardState(23) + Assert.Equal(24, fields.Length); Assert.Equal("bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", fields[0]); Assert.Equal(string.Empty, fields[2]); // IngestedAtUtc null Assert.Equal(string.Empty, fields[5]); // CorrelationId null - Assert.Equal(string.Empty, fields[6]); // SourceSiteId null - Assert.Equal(string.Empty, fields[12]); // HttpStatus null - Assert.Equal(string.Empty, fields[14]); // ErrorMessage null - Assert.Equal("False", fields[18]); // PayloadTruncated - Assert.Equal(string.Empty, fields[20]); // ForwardState null + Assert.Equal(string.Empty, fields[6]); // ExecutionId null + Assert.Equal(string.Empty, fields[7]); // ParentExecutionId null + Assert.Equal(string.Empty, fields[8]); // SourceSiteId null + Assert.Equal(string.Empty, fields[9]); // SourceNode null + Assert.Equal(string.Empty, fields[15]); // HttpStatus null + Assert.Equal(string.Empty, fields[17]); // ErrorMessage null + Assert.Equal("False", fields[21]); // PayloadTruncated + Assert.Equal(string.Empty, fields[23]); // ForwardState null } [Fact] diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/AuditEndpointsTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/AuditEndpointsTests.cs index 20f6f5d3..49069e87 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/AuditEndpointsTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/AuditEndpointsTests.cs @@ -315,6 +315,172 @@ public class AuditEndpointsTests } } + // ───────────────────────────────────────────────────────────────────── + // CSV export — formula-injection neutralization + column alignment + // (arch-review 2026-08-19 P6-7) + // ───────────────────────────────────────────────────────────────────── + + [Fact] + public void CsvHeader_Has24Columns_SameOrderAsAuditExportRowProperties() + { + // The JSONL export serializes AuditExportRow directly, so its field + // order IS the AuditExportRow property-declaration order; the CSV + // header must match it exactly (finding: the CSV was missing + // ExecutionId/ParentExecutionId/SourceNode that JSONL already carried). + var columns = AuditEndpoints.CsvHeader.Split(','); + + Assert.Equal(24, columns.Length); + Assert.Equal(new[] + { + "EventId", "OccurredAtUtc", "IngestedAtUtc", "Channel", "Kind", + "CorrelationId", "ExecutionId", "ParentExecutionId", + "SourceSiteId", "SourceNode", "SourceInstanceId", "SourceScript", + "Actor", "Target", "Status", "HttpStatus", "DurationMs", + "ErrorMessage", "ErrorDetail", "RequestSummary", "ResponseSummary", + "PayloadTruncated", "Extra", "ForwardState", + }, columns); + } + + [Fact] + public void FormatCsvRow_IncludesExecutionParentExecutionAndSourceNode() + { + var evt = ScadaBridgeAuditEventFactory.Create( + channel: AuditChannel.ApiOutbound, + kind: AuditKind.ApiCall, + status: AuditStatus.Delivered, + eventId: Guid.Parse("11111111-1111-1111-1111-111111111111"), + occurredAtUtc: new DateTime(2026, 5, 20, 12, 0, 0, DateTimeKind.Utc), + sourceNode: "site-a-node-a", + executionId: Guid.Parse("22222222-2222-2222-2222-222222222222"), + parentExecutionId: Guid.Parse("33333333-3333-3333-3333-333333333333")); + + var row = AuditEndpoints.FormatCsvRow(AuditExportRow.From(evt)); + var fields = row.Split(','); + + Assert.Equal(24, fields.Length); + Assert.Equal("22222222-2222-2222-2222-222222222222", fields[6]); // ExecutionId + Assert.Equal("33333333-3333-3333-3333-333333333333", fields[7]); // ParentExecutionId + Assert.Equal("site-a-node-a", fields[9]); // SourceNode + } + + [Theory] + [InlineData("=HYPERLINK(\"http://evil\",\"click\")", "'=HYPERLINK(")] + [InlineData("+cmd|'/c calc'!A1", "'+cmd|")] + [InlineData("-2+3", "'-2+3")] + [InlineData("@SUM(1,1)", "'@SUM(1,1)")] + [InlineData("\tsneaky", "'\tsneaky")] + public async Task Export_Csv_FreeTextLeadingFormulaCharacter_IsPrefixedWithQuote(string malicious, string expectedPrefix) + { + // CWE-1236: Target is inbound-API-caller-controlled free text. A raw + // leading =, +, -, @, or TAB must never reach the CSV cell unescaped. + var evt = ScadaBridgeAuditEventFactory.Create( + channel: AuditChannel.ApiInbound, + kind: AuditKind.InboundRequest, + status: AuditStatus.Delivered, + eventId: Guid.Parse("44444444-4444-4444-4444-444444444444"), + occurredAtUtc: new DateTime(2026, 5, 20, 12, 0, 0, DateTimeKind.Utc), + target: malicious); + + var (client, _, host) = await BuildHostAsync( + roles: new[] { "Administrator" }, + queryPages: new[] + { + (IReadOnlyList)new[] { evt }, + (IReadOnlyList)Array.Empty(), + }); + using (host) + { + var response = await client.SendAsync(Get("/api/audit/export?format=csv")); + var body = await response.Content.ReadAsStringAsync(); + + Assert.Contains(expectedPrefix, body); + // The raw, un-neutralized value must never appear on its own — + // every occurrence must be the neutralized ('-prefixed) form. + Assert.DoesNotContain("," + malicious + ",", body); + } + } + + [Fact] + public void FormatCsvRow_ErrorMessageResponseSummaryAndExtra_AreNeutralized() + { + // Covers the finding's named secondary carriers beyond Target. + var evt = ScadaBridgeAuditEventFactory.Create( + channel: AuditChannel.DbOutbound, + kind: AuditKind.DbWrite, + status: AuditStatus.Failed, + eventId: Guid.Parse("55555555-5555-5555-5555-555555555555"), + occurredAtUtc: new DateTime(2026, 5, 20, 12, 0, 0, DateTimeKind.Utc), + errorMessage: "=cmd()", + responseSummary: "+1+1", + extra: "@evil"); + + var row = AuditEndpoints.FormatCsvRow(AuditExportRow.From(evt)); + var fields = row.Split(','); + + Assert.Equal("'=cmd()", fields[17]); // ErrorMessage + Assert.Equal("'+1+1", fields[20]); // ResponseSummary + Assert.Equal("'@evil", fields[22]); // Extra + } + + [Fact] + public void FormatCsvRow_NegativeDurationAndDateColumns_NeverNeutralized() + { + // DurationMs/OccurredAtUtc are formatted by the exporter itself, never + // free text — a negative duration's leading '-' must render as-is, not + // be mistaken for a formula trigger and quote-prefixed. + var evt = ScadaBridgeAuditEventFactory.Create( + channel: AuditChannel.ApiOutbound, + kind: AuditKind.ApiCall, + status: AuditStatus.Delivered, + eventId: Guid.Parse("66666666-6666-6666-6666-666666666666"), + occurredAtUtc: new DateTime(2026, 5, 20, 12, 0, 0, DateTimeKind.Utc), + httpStatus: 200, + durationMs: -7); + + var row = AuditEndpoints.FormatCsvRow(AuditExportRow.From(evt)); + var fields = row.Split(','); + + Assert.Equal("-7", fields[16]); // DurationMs, unprefixed + Assert.StartsWith("2026-05-20", fields[1]); // OccurredAtUtc, unprefixed + Assert.Equal("200", fields[15]); // HttpStatus, unprefixed + } + + [Fact] + public void FormatCsvRow_BenignFreeTextField_IsNotPrefixed() + { + var evt = ScadaBridgeAuditEventFactory.Create( + channel: AuditChannel.ApiOutbound, + kind: AuditKind.ApiCall, + status: AuditStatus.Delivered, + eventId: Guid.Parse("77777777-7777-7777-7777-777777777777"), + occurredAtUtc: new DateTime(2026, 5, 20, 12, 0, 0, DateTimeKind.Utc), + target: "plant-a-external-system"); + + var row = AuditEndpoints.FormatCsvRow(AuditExportRow.From(evt)); + var fields = row.Split(','); + + Assert.Equal("plant-a-external-system", fields[13]); // Target + } + + [Fact] + public void CsvHeader_ColumnsMatchAuditExportRowPropertyOrder() + { + // The JSONL export serializes AuditExportRow verbatim (property + // declaration order = JSON field order once camelCased); assert the + // CSV header names + order are identical to that property surface, so + // a column can never again silently drift between the two formats. + // (Reflection, not a live JSONL round-trip: JsonOptions drops null + // properties on write — e.g. ForwardState is always null on the + // central read path — which would otherwise hide a real column from + // a JSON-based comparison.) + var propertyNames = typeof(AuditExportRow) + .GetProperties() + .Select(p => p.Name) + .ToArray(); + + Assert.Equal(propertyNames, AuditEndpoints.CsvHeader.Split(',')); + } + [Fact] public async Task Export_Parquet_Returns501() {