diff --git a/changelog.d/unreleased/3442.security.md b/changelog.d/unreleased/3442.security.md new file mode 100644 index 0000000000..6072239523 --- /dev/null +++ b/changelog.d/unreleased/3442.security.md @@ -0,0 +1,16 @@ +--- +category: security +issues: + - 3442 +affected: + - tools/CodeIndex.TestTelemetry/Program.cs + - tests/CodeIndex.Tests/TestTelemetryTests.cs +--- + +## English + +- **Test telemetry TRX processing is now bounded (#3442)** — the telemetry helper now caps `--top`, TRX directory and entry traversal, TRX file discovery, TRX file size, XML parser work, and retained result collections so unusually large test result trees cannot force unbounded processing. It also skips non-regular `.trx` filesystem entries and discards malformed TRX files atomically instead of contributing partial pass/fail counts. + +## 日本語 + +- **Test telemetry の TRX 処理に上限を設けました (#3442)** — telemetry helper は `--top`、TRX ディレクトリ / エントリ走査、TRX ファイル探索、TRX ファイルサイズ、XML parser の処理量、保持する結果コレクションを制限し、過大なテスト結果ツリーで無制限の処理が発生しないようにしました。通常ファイルではない `.trx` filesystem entry をスキップし、壊れた TRX ファイルは pass/fail 件数へ部分的に反映せずファイル単位で破棄します。 diff --git a/changelog.d/unreleased/3443.security.md b/changelog.d/unreleased/3443.security.md new file mode 100644 index 0000000000..baba748a7f --- /dev/null +++ b/changelog.d/unreleased/3443.security.md @@ -0,0 +1,16 @@ +--- +category: security +issues: + - 3443 +affected: + - tools/CodeIndex.TestTelemetry/Program.cs + - tests/CodeIndex.Tests/TestTelemetryTests.cs +--- + +## English + +- **Test telemetry TRX warnings now avoid leaking local paths and XML parser details (#3443)** — parse and inspection warnings now use relative TRX paths when possible and stable reason codes instead of raw exception messages. + +## 日本語 + +- **Test telemetry の TRX warning がローカルパスや XML parser 詳細を漏らさないようになりました (#3443)** — parse / inspection warning は可能な限り相対 TRX パスと安定した reason code を使い、生の例外メッセージを出力しないようにしました。 diff --git a/tests/CodeIndex.Tests/TestTelemetryTests.cs b/tests/CodeIndex.Tests/TestTelemetryTests.cs index 8afe6d6853..6fb1d35d56 100644 --- a/tests/CodeIndex.Tests/TestTelemetryTests.cs +++ b/tests/CodeIndex.Tests/TestTelemetryTests.cs @@ -75,4 +75,228 @@ public void Load_MissingDirectoryReturnsWarningInsteadOfFailingCiSummary() Assert.Single(summary.Warnings); Assert.Contains("Results directory not found", summary.Warnings[0], StringComparison.Ordinal); } + + [Fact] + public void Load_RejectsTopValuesAboveTelemetryCap() + { + var exception = Assert.Throws(() => + TrxTelemetry.Load(Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N")), TrxTelemetry.MaxTop + 1)); + + Assert.Contains($"between 1 and {TrxTelemetry.MaxTop}", exception.Message, StringComparison.Ordinal); + } + + [Fact] + public void Load_CapsTrxDiscovery() + { + var projectRoot = TestProjectHelper.CreateTempProject("cdidx_trx_telemetry_cap"); + try + { + var resultsDirectory = Path.Combine(projectRoot, "TestResults"); + Directory.CreateDirectory(resultsDirectory); + + for (var i = 0; i < TrxTelemetry.MaxTrxFiles + 1; i++) + { + File.WriteAllText(Path.Combine(resultsDirectory, $"results-{i:D4}.trx"), MinimalTrx($"Test{i:D4}")); + } + + var summary = TrxTelemetry.Load(resultsDirectory, top: 1); + + Assert.Equal(TrxTelemetry.MaxTrxFiles, summary.TrxFileCount); + Assert.Equal(TrxTelemetry.MaxTrxFiles, summary.Total); + Assert.Contains(summary.Warnings, warning => + warning.Contains("TRX file cap reached", StringComparison.Ordinal)); + } + finally + { + TestProjectHelper.DeleteDirectory(projectRoot); + } + } + + [Fact] + public void Load_CapsTrxDirectoryTraversal() + { + var projectRoot = TestProjectHelper.CreateTempProject("cdidx_trx_telemetry_directory_cap"); + try + { + var resultsDirectory = Path.Combine(projectRoot, "TestResults"); + Directory.CreateDirectory(resultsDirectory); + + for (var i = 0; i < TrxTelemetry.MaxTraversalDirectories + 1; i++) + { + Directory.CreateDirectory(Path.Combine(resultsDirectory, $"dir-{i:D4}")); + } + + var summary = TrxTelemetry.Load(resultsDirectory, top: 1); + + Assert.Equal(0, summary.TrxFileCount); + Assert.Equal(0, summary.Total); + Assert.Contains(summary.Warnings, warning => + warning.Contains("directory traversal cap", StringComparison.Ordinal)); + } + finally + { + TestProjectHelper.DeleteDirectory(projectRoot); + } + } + + [Fact] + public void Load_SkipsTrxFilesAboveSizeCap() + { + var projectRoot = TestProjectHelper.CreateTempProject("cdidx_trx_telemetry_size"); + try + { + var resultsDirectory = Path.Combine(projectRoot, "TestResults"); + var nestedDirectory = Path.Combine(resultsDirectory, "nested"); + Directory.CreateDirectory(nestedDirectory); + var largeTrx = Path.Combine(nestedDirectory, "too-large.trx"); + using (var stream = File.Create(largeTrx)) + { + stream.SetLength(TrxTelemetry.MaxTrxFileBytes + 1); + } + + var summary = TrxTelemetry.Load(resultsDirectory, top: 1); + + Assert.Equal(1, summary.TrxFileCount); + Assert.Equal(0, summary.Total); + var warning = Assert.Single(summary.Warnings); + Assert.Contains("byte cap", warning, StringComparison.Ordinal); + Assert.Contains("nested/too-large.trx", warning, StringComparison.Ordinal); + Assert.DoesNotContain(projectRoot, warning, StringComparison.Ordinal); + } + finally + { + TestProjectHelper.DeleteDirectory(projectRoot); + } + } + + [Fact] + public void Load_SkipsUnixFifoTrxEntries() + { + if (OperatingSystem.IsWindows()) + return; + + var projectRoot = TestProjectHelper.CreateTempProject("cdidx_trx_telemetry_fifo"); + try + { + var resultsDirectory = Path.Combine(projectRoot, "TestResults"); + Directory.CreateDirectory(resultsDirectory); + var fifoPath = Path.Combine(resultsDirectory, "pipe.trx"); + if (Mkfifo(fifoPath, Convert.ToUInt32("600", 8)) != 0) + throw new IOException($"mkfifo failed with errno {System.Runtime.InteropServices.Marshal.GetLastWin32Error()}."); + + var summary = TrxTelemetry.Load(resultsDirectory, top: 1); + + Assert.Equal(0, summary.TrxFileCount); + Assert.Equal(0, summary.Total); + } + finally + { + TestProjectHelper.DeleteDirectory(projectRoot); + } + } + + [Fact] + public void Load_RejectsTrxDtds() + { + var projectRoot = TestProjectHelper.CreateTempProject("cdidx_trx_telemetry_dtd"); + try + { + var resultsDirectory = Path.Combine(projectRoot, "TestResults"); + Directory.CreateDirectory(resultsDirectory); + File.WriteAllText(Path.Combine(resultsDirectory, "with-dtd.trx"), """ + + ]> + + + + + + """); + + var summary = TrxTelemetry.Load(resultsDirectory, top: 1); + + Assert.Equal(0, summary.Total); + Assert.Contains(summary.Warnings, warning => + string.Equals(warning, "Could not parse with-dtd.trx: invalid_xml", StringComparison.Ordinal)); + } + finally + { + TestProjectHelper.DeleteDirectory(projectRoot); + } + } + + [Fact] + public void Load_DiscardsPartialResultsFromMalformedTrx() + { + var projectRoot = TestProjectHelper.CreateTempProject("cdidx_trx_telemetry_partial_xml"); + try + { + var resultsDirectory = Path.Combine(projectRoot, "TestResults"); + Directory.CreateDirectory(resultsDirectory); + File.WriteAllText(Path.Combine(resultsDirectory, "partial.trx"), """ + + + + + + + """); + + var summary = TrxTelemetry.Load(resultsDirectory, top: 1); + + Assert.Equal(0, summary.Total); + Assert.Equal(0, summary.Passed); + Assert.Empty(summary.Slowest); + Assert.Contains(summary.Warnings, warning => + string.Equals(warning, "Could not parse partial.trx: invalid_xml", StringComparison.Ordinal)); + } + finally + { + TestProjectHelper.DeleteDirectory(projectRoot); + } + } + + [Fact] + public void Load_SanitizesInvalidXmlWarnings() + { + var projectRoot = TestProjectHelper.CreateTempProject("cdidx_trx_telemetry_xml_warning"); + try + { + var resultsDirectory = Path.Combine(projectRoot, "TestResults"); + var nestedDirectory = Path.Combine(resultsDirectory, "nested"); + Directory.CreateDirectory(nestedDirectory); + File.WriteAllText(Path.Combine(nestedDirectory, "broken.trx"), """ + + + + + + """); + + var summary = TrxTelemetry.Load(resultsDirectory, top: 1); + + var warning = Assert.Single(summary.Warnings); + Assert.Equal("Could not parse nested/broken.trx: invalid_xml", warning); + Assert.DoesNotContain(projectRoot, warning, StringComparison.Ordinal); + Assert.DoesNotContain("Name cannot begin", warning, StringComparison.OrdinalIgnoreCase); + Assert.DoesNotContain("position", warning, StringComparison.OrdinalIgnoreCase); + } + finally + { + TestProjectHelper.DeleteDirectory(projectRoot); + } + } + + private static string MinimalTrx(string testName) => $$""" + + + + + + + """; + + [System.Runtime.InteropServices.DllImport("libc", EntryPoint = "mkfifo", SetLastError = true)] + private static extern int Mkfifo(string path, uint mode); } diff --git a/tools/CodeIndex.TestTelemetry/Program.cs b/tools/CodeIndex.TestTelemetry/Program.cs index 360c12a185..5280eba1d9 100644 --- a/tools/CodeIndex.TestTelemetry/Program.cs +++ b/tools/CodeIndex.TestTelemetry/Program.cs @@ -1,5 +1,5 @@ using System.Globalization; -using System.Xml.Linq; +using System.Xml; namespace CodeIndex.TestTelemetry; @@ -60,8 +60,12 @@ private static SummarizeOptions ParseSummarizeOptions(string[] args) if (i + 1 >= args.Length) throw new TelemetryException("Missing value for --top."); - if (!int.TryParse(args[++i], NumberStyles.None, CultureInfo.InvariantCulture, out top) || top <= 0) - throw new TelemetryException("--top must be a positive integer."); + if (!int.TryParse(args[++i], NumberStyles.None, CultureInfo.InvariantCulture, out top) || + top <= 0 || + top > TrxTelemetry.MaxTop) + { + throw new TelemetryException($"--top must be between 1 and {TrxTelemetry.MaxTop}."); + } continue; } @@ -77,10 +81,16 @@ private sealed record SummarizeOptions(string ResultsDirectory, int Top); public static class TrxTelemetry { + public const int MaxTop = 100; + public const int MaxTrxFiles = 256; + public const int MaxTraversalDirectories = 256; + public const int MaxTraversalEntries = 4096; + public const long MaxTrxFileBytes = 16 * 1024 * 1024; + public static TrxTelemetrySummary Load(string resultsDirectory, int top) { - if (top <= 0) - throw new TelemetryException("Top count must be positive."); + if (top <= 0 || top > MaxTop) + throw new TelemetryException($"Top count must be between 1 and {MaxTop}."); if (!Directory.Exists(resultsDirectory)) { @@ -97,64 +107,238 @@ public static TrxTelemetrySummary Load(string resultsDirectory, int top) Warnings: [$"Results directory not found: {resultsDirectory}"]); } - var trxFiles = Directory - .EnumerateFiles(resultsDirectory, "*.trx", SearchOption.AllDirectories) - .OrderBy(path => path, StringComparer.Ordinal) - .ToList(); - - var tests = new List(); var warnings = new List(); + var trxFiles = EnumerateTrxFiles(resultsDirectory, warnings); + var results = new TrxResultAccumulator(top); foreach (var path in trxFiles) { + if (!CanReadTrxFile(resultsDirectory, path, warnings)) + continue; + try { - tests.AddRange(ReadResults(path)); + var fileResults = new TrxResultAccumulator(top); + foreach (var result in ReadResults(path)) + { + fileResults.Add(result); + } + + results.Merge(fileResults); } - catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or System.Xml.XmlException) + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or XmlException) { - warnings.Add($"Could not parse {path}: {ex.Message}"); + warnings.Add($"Could not parse {FormatTrxPath(resultsDirectory, path)}: {GetWarningReason(ex)}"); } } - var passed = tests.Count(result => IsOutcome(result, "Passed")); - var failed = tests.Count(IsFailureOutcome); - var skipped = tests.Count(result => IsOutcome(result, "NotExecuted") || IsOutcome(result, "Skipped")); - var other = tests.Count - passed - failed - skipped; - - var slowest = tests - .OrderByDescending(result => result.Duration) - .ThenBy(result => result.TestName, StringComparer.Ordinal) - .Take(top) - .ToList(); - - var failures = tests - .Where(IsFailureOutcome) - .OrderByDescending(result => result.Duration) - .ThenBy(result => result.TestName, StringComparer.Ordinal) - .Take(top) - .ToList(); - return new TrxTelemetrySummary( ResultsDirectory: resultsDirectory, TrxFileCount: trxFiles.Count, - Total: tests.Count, - Passed: passed, - Failed: failed, - Skipped: skipped, - Other: other, - Slowest: slowest, - Failures: failures, + Total: results.Total, + Passed: results.Passed, + Failed: results.Failed, + Skipped: results.Skipped, + Other: results.Other, + Slowest: results.Slowest, + Failures: results.Failures, Warnings: warnings); } + private static List EnumerateTrxFiles(string resultsDirectory, List warnings) + { + var trxFiles = new List(MaxTrxFiles); + var pendingDirectories = new Queue(); + pendingDirectories.Enqueue(resultsDirectory); + var visitedDirectories = 0; + var visitedEntries = 0; + + while (pendingDirectories.Count > 0) + { + if (visitedDirectories >= MaxTraversalDirectories) + { + warnings.Add($"TRX directory traversal cap reached: visited first {MaxTraversalDirectories} directories."); + break; + } + + var directory = pendingDirectories.Dequeue(); + visitedDirectories++; + + foreach (var entry in EnumerateDirectoryEntries(directory, warnings)) + { + visitedEntries++; + if (visitedEntries > MaxTraversalEntries) + { + warnings.Add($"TRX entry traversal cap reached: visited first {MaxTraversalEntries} entries."); + trxFiles.Sort(StringComparer.Ordinal); + return trxFiles; + } + + if (!TryGetAttributes(entry, warnings, out var attributes)) + continue; + + if ((attributes & FileAttributes.ReparsePoint) != 0) + continue; + + if ((attributes & FileAttributes.Directory) != 0) + { + pendingDirectories.Enqueue(entry); + continue; + } + + if (!IsRegularFile(entry, attributes, warnings)) + continue; + + if (!string.Equals(Path.GetExtension(entry), ".trx", StringComparison.OrdinalIgnoreCase)) + continue; + + if (trxFiles.Count >= MaxTrxFiles) + { + warnings.Add($"TRX file cap reached: using first {MaxTrxFiles} files."); + trxFiles.Sort(StringComparer.Ordinal); + return trxFiles; + } + + trxFiles.Add(entry); + } + } + + trxFiles.Sort(StringComparer.Ordinal); + return trxFiles; + } + + private static IEnumerable EnumerateDirectoryEntries(string directory, List warnings) + { + IEnumerator? enumerator; + try + { + enumerator = Directory.EnumerateFileSystemEntries(directory).GetEnumerator(); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + warnings.Add($"Could not enumerate TRX directory: {GetWarningReason(ex)}"); + yield break; + } + + using (enumerator) + { + while (true) + { + string entry; + try + { + if (!enumerator.MoveNext()) + yield break; + + entry = enumerator.Current; + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + warnings.Add($"Could not enumerate TRX directory: {GetWarningReason(ex)}"); + yield break; + } + + yield return entry; + } + } + } + + private static bool IsRegularFile(string path, FileAttributes attributes, List warnings) + { + if ((attributes & (FileAttributes.Directory | FileAttributes.ReparsePoint | FileAttributes.Device)) != 0) + return false; + + if (OperatingSystem.IsWindows()) + return true; + + if (!UnixFileStatus.TryGetFileMode(path, out var mode)) + { + warnings.Add("Could not inspect TRX traversal entry: file_type_unavailable"); + return false; + } + + return (mode & UnixFileStatus.FileTypeMask) == UnixFileStatus.RegularFile; + } + + private static bool TryGetAttributes(string path, List warnings, out FileAttributes attributes) + { + try + { + attributes = File.GetAttributes(path); + return true; + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + warnings.Add($"Could not inspect TRX traversal entry: {GetWarningReason(ex)}"); + attributes = default; + return false; + } + } + + private static bool CanReadTrxFile(string resultsDirectory, string path, List warnings) + { + try + { + var file = new FileInfo(path); + if (file.Length > MaxTrxFileBytes) + { + warnings.Add($"TRX file exceeds {MaxTrxFileBytes} byte cap: {FormatTrxPath(resultsDirectory, path)}"); + return false; + } + + return true; + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + warnings.Add($"Could not inspect {FormatTrxPath(resultsDirectory, path)}: {GetWarningReason(ex)}"); + return false; + } + } + + private static string FormatTrxPath(string resultsDirectory, string path) + { + try + { + var relativePath = Path.GetRelativePath(resultsDirectory, path); + if (!IsParentTraversal(relativePath) && !Path.IsPathFullyQualified(relativePath)) + return NormalizePath(relativePath); + } + catch (Exception ex) when (ex is ArgumentException or NotSupportedException) + { + } + + var fileName = Path.GetFileName(path); + return string.IsNullOrWhiteSpace(fileName) ? "" : fileName; + } + + private static bool IsParentTraversal(string path) => + path == ".." || + path.StartsWith("../", StringComparison.Ordinal) || + path.StartsWith(@"..\", StringComparison.Ordinal); + + private static string NormalizePath(string path) => + path.Replace(Path.DirectorySeparatorChar, '/').Replace(Path.AltDirectorySeparatorChar, '/'); + + private static string GetWarningReason(Exception ex) => ex switch + { + XmlException => "invalid_xml", + UnauthorizedAccessException => "access_denied", + IOException => "io_error", + _ => "unknown_error" + }; + private static IEnumerable ReadResults(string path) { - var document = XDocument.Load(path, LoadOptions.None); - foreach (var element in document.Descendants().Where(element => element.Name.LocalName == "UnitTestResult")) + using var stream = File.OpenRead(path); + using var reader = XmlReader.Create(stream, CreateXmlReaderSettings()); + + while (reader.Read()) { - var testName = (string?)element.Attribute("testName"); - var outcome = (string?)element.Attribute("outcome"); + if (reader.NodeType != XmlNodeType.Element || reader.LocalName != "UnitTestResult") + continue; + + var testName = reader.GetAttribute("testName"); + var outcome = reader.GetAttribute("outcome"); if (string.IsNullOrWhiteSpace(testName) || string.IsNullOrWhiteSpace(outcome)) continue; @@ -162,7 +346,160 @@ private static IEnumerable ReadResults(string path) yield return new TrxTestResult( TestName: testName.Trim(), Outcome: outcome.Trim(), - Duration: ParseDuration((string?)element.Attribute("duration"))); + Duration: ParseDuration(reader.GetAttribute("duration"))); + } + } + + private static XmlReaderSettings CreateXmlReaderSettings() => new() + { + DtdProcessing = DtdProcessing.Prohibit, + IgnoreComments = true, + IgnoreProcessingInstructions = true, + MaxCharactersFromEntities = 0, + MaxCharactersInDocument = MaxTrxFileBytes, + XmlResolver = null + }; + + private static void AddTopResult(List results, TrxTestResult result, int limit) + { + results.Add(result); + results.Sort(CompareByDurationDescendingThenName); + + if (results.Count > limit) + results.RemoveAt(limit); + } + + private static int CompareByDurationDescendingThenName(TrxTestResult left, TrxTestResult right) + { + var duration = right.Duration.CompareTo(left.Duration); + return duration != 0 + ? duration + : string.Compare(left.TestName, right.TestName, StringComparison.Ordinal); + } + + private sealed class TrxResultAccumulator + { + private readonly int _top; + + public TrxResultAccumulator(int top) + { + _top = top; + Slowest = new List(Math.Min(top, 16)); + Failures = new List(Math.Min(top, 16)); + } + + public int Total { get; private set; } + + public int Passed { get; private set; } + + public int Failed { get; private set; } + + public int Skipped { get; private set; } + + public int Other => Total - Passed - Failed - Skipped; + + public List Slowest { get; } + + public List Failures { get; } + + public void Add(TrxTestResult result) + { + Total++; + + if (IsOutcome(result, "Passed")) + { + Passed++; + } + else if (IsFailureOutcome(result)) + { + Failed++; + AddTopResult(Failures, result, _top); + } + else if (IsOutcome(result, "NotExecuted") || IsOutcome(result, "Skipped")) + { + Skipped++; + } + + AddTopResult(Slowest, result, _top); + } + + public void Merge(TrxResultAccumulator other) + { + Total += other.Total; + Passed += other.Passed; + Failed += other.Failed; + Skipped += other.Skipped; + + foreach (var result in other.Slowest) + { + AddTopResult(Slowest, result, _top); + } + + foreach (var result in other.Failures) + { + AddTopResult(Failures, result, _top); + } + } + } + + private static class UnixFileStatus + { + internal const int FileTypeMask = 0xF000; + internal const int RegularFile = 0x8000; + + internal static bool TryGetFileMode(string filePath, out int mode) + { + mode = 0; + try + { + if (NativeMethods.Stat(filePath, out var status) != 0) + return false; + + mode = status.Mode; + return true; + } + catch (DllNotFoundException) + { + return false; + } + catch (EntryPointNotFoundException) + { + return false; + } + } + + [System.Runtime.InteropServices.StructLayout(System.Runtime.InteropServices.LayoutKind.Sequential)] + private struct FileStatus + { + internal FileStatusFlags Flags; + internal int Mode; + internal uint Uid; + internal uint Gid; + internal long Size; + internal long ATime; + internal long ATimeNsec; + internal long MTime; + internal long MTimeNsec; + internal long CTime; + internal long CTimeNsec; + internal long BirthTime; + internal long BirthTimeNsec; + internal long Dev; + internal long RDev; + internal long Ino; + internal uint UserFlags; + } + + [System.Flags] + private enum FileStatusFlags : uint + { + None = 0, + } + + private static class NativeMethods + { + [System.Runtime.InteropServices.DllImport("libSystem.Native", EntryPoint = "SystemNative_Stat", CharSet = System.Runtime.InteropServices.CharSet.Ansi)] + internal static extern int Stat(string path, out FileStatus output); } }