fix(management)+fix(cli): neutralize CSV formula injection + align export columns; deadline + Ctrl-C on audit export streaming (arch-review 2026-08-19 P6-7 + P6-8)
P6-7 (CWE-1236): AuditEndpoints CSV export quoted cells on comma/quote/CR/LF but never neutralized a leading =, +, -, @, TAB, or CR — inbound-API-caller text landing in Target/ErrorMessage/RequestSummary/ResponseSummary/Extra could execute as a formula/DDE trigger when opened in Excel. AppendField now prefixes a literal single quote on free-text columns only (never the GUID/enum/numeric/date/bool columns the exporter formats itself), applied before RFC 4180 quoting. The CSV was also missing ExecutionId/ ParentExecutionId/SourceNode that the JSONL export already carried; both are now 24 columns in the same AuditExportRow order. A second, independently maintained CSV writer (CentralUI's AuditLogExportService, feeding the Audit Log page's bulk-export button) had the identical gap and got the identical fix + column alignment. P6-8: `audit export` was the one CLI HTTP path with no deadline at all — a residual gap from the ManagementHttpClient timeout-honesty fix, which made HttpClient.Timeout infinite and the per-call token the single deadline everywhere except this streamed GET. RunExportAsync now takes a headers- received deadline (CancelAfter, reusing ManagementHttpClient. DefaultConnectTimeout) around the initial GetAsync, an inactivity deadline around the streaming copy (rearmed on every successful read, so large exports are bounded by silence, not total size), and an external CancellationToken wired to Console.CancelKeyPress in AuditCommands so Ctrl-C cancels promptly. A stalled/faulted stream now produces a friendly error noting the partial output file instead of hanging or dumping a stack trace.
This commit is contained in:
Joseph Doherty
@@ -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<AuditEvent>)new[] { evt },
|
||||
(IReadOnlyList<AuditEvent>)Array.Empty<AuditEvent>(),
|
||||
});
|
||||
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()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user