From fb277cff810b69f327ccf874c3073624aa2edc3a Mon Sep 17 00:00:00 2001 From: Jo-Be-Co Date: Mon, 23 Mar 2026 15:29:32 +0100 Subject: [PATCH] Faulty regex patterns will throw InvalidOperationException during evaluation. --- .../ConditionalTagCollection[TClass].cs | 110 +++++++++++------- .../ConditionalTagCollectionTests.cs | 97 +++++++++++++++ .../FileNamingTemplateTests.cs | 4 +- .../TemplatesTests.cs | 2 +- 4 files changed, 168 insertions(+), 45 deletions(-) create mode 100644 Source/_Tests/FileManager.Tests/ConditionalTagCollectionTests.cs diff --git a/Source/FileManager/NamingTemplate/ConditionalTagCollection[TClass].cs b/Source/FileManager/NamingTemplate/ConditionalTagCollection[TClass].cs index b474358d..39225698 100644 --- a/Source/FileManager/NamingTemplate/ConditionalTagCollection[TClass].cs +++ b/Source/FileManager/NamingTemplate/ConditionalTagCollection[TClass].cs @@ -65,10 +65,12 @@ public partial class ConditionalTagCollection(bool caseSensitive = true) private partial class ConditionalTag : TagBase, IClosingPropertyTag { + private static readonly TimeSpan RegexpCheckTimeout = TimeSpan.FromMilliseconds(100); + public override Regex NameMatcher { get; } public Regex NameCloseMatcher { get; } - private Func CreateConditionExpression { get; } + private Func CreateConditionExpression { get; } public ConditionalTag(ITemplateTag templateTag, RegexOptions options, Expression conditionExpression) : base(templateTag, conditionExpression) @@ -77,7 +79,7 @@ public partial class ConditionalTagCollection(bool caseSensitive = true) NameMatcher = new Regex($"^<(?!)?{tagNameRe}->", options); NameCloseMatcher = new Regex($"^<-{tagNameRe}>", options); - CreateConditionExpression = (_, _) => conditionExpression; + CreateConditionExpression = (_, _, _) => conditionExpression; } public ConditionalTag(ITemplateTag templateTag, RegexOptions options, ParameterExpression parameter, ValueProvider valueProvider, ConditionEvaluator conditionEvaluator) @@ -96,7 +98,7 @@ public partial class ConditionalTagCollection(bool caseSensitive = true) , options); NameCloseMatcher = new Regex($"^<-{templateTag.TagName}>", options); - CreateConditionExpression = (property, _) + CreateConditionExpression = (_, property, _) => ConditionEvaluatorCall(templateTag, parameter, valueProvider, property, conditionEvaluator); } @@ -124,9 +126,9 @@ public partial class ConditionalTagCollection(bool caseSensitive = true) , options); NameCloseMatcher = new Regex($"^<-{templateTag.TagName}>", options); - CreateConditionExpression = (property, checkString) => + CreateConditionExpression = (exactName, property, checkString) => { - var conditionEvaluator = GetPredicate(checkString); + var conditionEvaluator = GetPredicate(exactName, checkString); return ConditionEvaluatorCall(templateTag, parameter, valueProvider, property, conditionEvaluator); }; } @@ -152,7 +154,7 @@ public partial class ConditionalTagCollection(bool caseSensitive = true) CultureParameter); } - private static ConditionEvaluator GetPredicate(string? checkString) + private static ConditionEvaluator GetPredicate(string exactName, string? checkString) { if (checkString == null) return (v, _) => v switch @@ -168,11 +170,11 @@ public partial class ConditionalTagCollection(bool caseSensitive = true) var iVal = -1; var isNumericalOperator = match.Groups["num_op"].Success && int.TryParse(valStr, out iVal); - Func checkItem = match.Groups["op"].ValueSpan switch + var checkItem = match.Groups["op"].ValueSpan switch { "=" or "" => (v, culture) => VComparedToStr(v, culture, valStr) == 0, "!=" or "!" => (v, culture) => VComparedToStr(v, culture, valStr) != 0, - "~" => GetRegExpCheck(valStr), + "~" => GetRegExpCheck(exactName, valStr), "#=" => (v, _) => VAsInt(v) == iVal, "#!=" => (v, _) => VAsInt(v) != iVal, "#>=" or ">=" => (v, _) => VAsInt(v) >= iVal, @@ -202,55 +204,78 @@ public partial class ConditionalTagCollection(bool caseSensitive = true) private static int VComparedToStr(object? v, CultureInfo? culture, string valStr) { culture ??= CultureInfo.CurrentCulture; - return culture.CompareInfo.Compare(v?.ToString()?.Trim(), valStr, CompareOptions.IgnoreCase); + return culture.CompareInfo.Compare(v?.ToString(), valStr, CompareOptions.IgnoreCase); } /// /// Build a regular expression check. Uses culture-invariant matching for thread-safety and consistency. /// Applies a timeout to prevent regex patterns from causing excessive backtracking and blocking. + /// Throws InvalidOperationException if the regex pattern is invalid or evaluation times out. /// - /// The regex pattern to match + /// The full tag string for context in error messages + /// The regex pattern to match /// check function to validate an object - private static Func GetRegExpCheck(string valStr) + /// Thrown when regex parsing fails or when regex matching times out, indicating faulty user input + private static Func GetRegExpCheck(string exactName, string pattern) { + Regex regex; try { // Compile regex with timeout to prevent catastrophic backtracking - var regex = new Regex(valStr, + regex = new Regex(pattern, RegexOptions.IgnoreCase | RegexOptions.CultureInvariant | RegexOptions.Compiled, - TimeSpan.FromMilliseconds(100)); - - return (v, _) => - { - try - { - // CultureInfo parameter is intentionally ignored (discarded with _). - // RegexOptions.CultureInvariant ensures culture-independent matching for predictable behavior. - // This is preferred for template conditions because: - // 1. Thread-safety: Regex operations are isolated and don't depend on thread-local culture - // 2. Consistency: Template matches produce identical results regardless of system locale - // 3. Predictability: Rules don't unexpectedly change based on user's OS settings - // - // Culture-sensitive matching would be problematic in cases like: - // - Turkish locale: 'I' has different case folding (I ↔ ı vs. I ↔ i). Pattern "[i-z]" might match Turkish 'ı'. - // - German locale: ß might be treated as equivalent to 'ss' during case-insensitive matching. - // - Lithuanian locale: 'i' after 'ž' has an accent that affects sorting/matching. - // - // For naming templates, culture-invariant is the safer default. - return regex.IsMatch(v.ToString()?.Trim() ?? ""); - } - catch (RegexMatchTimeoutException) - { - // Return false if regex evaluation times out - return false; - } - }; + RegexpCheckTimeout); } - catch + catch (ArgumentException ex) { - // If regex compilation fails, return a predicate that always returns false - return (_, _) => false; + // If regex compilation fails, throw as faulty user input + var errorMessage = BuildErrorMessage(exactName, pattern, "Invalid regular expression pattern. Correct the pattern and escaping or remove that condition"); + throw new InvalidOperationException(errorMessage, ex); } + + return (v, _) => + { + try + { + // CultureInfo parameter is intentionally ignored (discarded with _). + // RegexOptions.CultureInvariant ensures culture-independent matching for predictable behavior. + // This is preferred for template conditions because: + // 1. Thread-safety: Regex operations are isolated and don't depend on thread-local culture + // 2. Consistency: Template matches produce identical results regardless of system locale + // 3. Predictability: Rules don't unexpectedly change based on user's OS settings + // + // Culture-sensitive matching would be problematic in cases like: + // - Turkish locale: 'I' has different case folding (I ↔ ı vs. I ↔ i). Pattern "[i-z]" might match Turkish 'ı'. + // - German locale: ß might be treated as equivalent to 'ss' during case-insensitive matching. + // - Lithuanian locale: 'i' after 'ž' has an accent that affects sorting/matching. + // + // For naming templates, culture-invariant is the safer default. + return regex.IsMatch(v.ToString()?.Trim() ?? ""); + } + catch (RegexMatchTimeoutException ex) + { + // Throw if regex evaluation times out, indicating faulty user input (e.g., catastrophic backtracking) + var errorMessage = BuildErrorMessage(exactName, pattern, "Regular expression pattern evaluation timed out. Use a simpler pattern or remove that condition"); + throw new InvalidOperationException(errorMessage, ex); + } + }; + } + + private static string BuildErrorMessage(string exactName, string pattern, string errorType) + { + const int maxMessageLen = 200; + + // Build full message with pattern + var fullMsg = $"{errorType}: {exactName} -> Pattern: {pattern}"; + + // Return full message if it's within the character limit + if (fullMsg.Length <= maxMessageLen) return fullMsg; + + // Keep the error type and as much pattern as possible + var maxPatternLen = maxMessageLen - errorType.Length - 23; // Account for ". Pattern starts with: " + var trimmedPattern = pattern.Length > maxPatternLen ? pattern[..(maxPatternLen - 3)] + "..." : pattern; + return $"{errorType}. Pattern starts with: {trimmedPattern}"; + } // without any special check, only the existence of the property is checked. Strings need to be non-empty. @@ -273,6 +298,7 @@ public partial class ConditionalTagCollection(bool caseSensitive = true) protected override Expression GetTagExpression(string exactName, Dictionary matchData, OutputType outputType) { var getBool = CreateConditionExpression( + exactName, matchData.GetValueOrDefault("property")?.Value, Unescape(matchData.GetValueOrDefault("check"))); return matchData["not"].Success ? Expression.Not(getBool) : getBool; diff --git a/Source/_Tests/FileManager.Tests/ConditionalTagCollectionTests.cs b/Source/_Tests/FileManager.Tests/ConditionalTagCollectionTests.cs new file mode 100644 index 00000000..2a127fd2 --- /dev/null +++ b/Source/_Tests/FileManager.Tests/ConditionalTagCollectionTests.cs @@ -0,0 +1,97 @@ +using System; +using System.Globalization; +using System.Reflection; +using FileManager.NamingTemplate; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace FileManager.Tests; + +[TestClass] +public class ConditionalTagCollectionTests +{ + private class TestObject + { + public string? Value { get; init; } + } + + private class TestTag : ITemplateTag + { + public string TagName => "testcond"; + } + + private readonly ConditionalTagCollection _conditionalTags = new() + { + { new TestTag(), TryGetValue } + }; + + private static object? TryGetValue(ITemplateTag _, TestObject obj, string condition, CultureInfo? culture) + => obj.Value; + + /// + /// Test that invalid regex patterns throw InvalidOperationException during evaluation. + /// Tests include malformed patterns and catastrophic backtracking scenarios. + /// + [TestMethod] + [DataRow("[abc", "test_value", DisplayName = "InvalidRegexPattern_UnmatchedBracket")] + [DataRow("(?'name)abc", "test_value", DisplayName = "InvalidRegexPattern_InvalidGroup")] + [DataRow("(a+)+b", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", DisplayName = "CatastrophicBacktracking_NestedQuantifiers")] + [DataRow("(a|aa|aaa|aaaa)*?b", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", DisplayName = "CatastrophicBacktracking_AlternationOverlap")] + [DataRow("(a+a+)+b", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", DisplayName = "CatastrophicBacktracking_RepeatedConcatenation")] + [DataRow("^(a+)+$", "aaaaaaaaaaaaaaaaaaaaaab", DisplayName = "CatastrophicBacktracking_AnchoredRepeated")] + [DataRow("(a*)*b", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", DisplayName = "CatastrophicBacktracking_StarStar")] + public void ConditionalTag_InvalidRegexPattern_ThrowsInvalidOperationException(string pattern, string testValue) + { + // Arrange: Invalid regex patterns that should throw InvalidOperationException during evaluation + var template = $"content<-testcond>"; + var namingTemplate = NamingTemplate.NamingTemplate.Parse(template, [_conditionalTags]); + + var testObj = new TestObject { Value = testValue }; + + // Act & Assert: Evaluate template with invalid regex, should throw InvalidOperationException + try + { + namingTemplate.Evaluate(testObj); + Assert.Fail($"Expected InvalidOperationException for pattern: {pattern}"); + } + catch (Exception ex) when (ex is InvalidOperationException or TargetInvocationException) + { + // Expected behavior - regex is invalid or caused timeout + } + } + + /// + /// Test that valid simple regex patterns parse successfully and don't throw during evaluation. + /// + [TestMethod] + public void ConditionalTag_ValidRegexPattern_ParsesSuccessfully() + { + // Arrange: Valid simple regex pattern with proper closing tag + var template = "content<-testcond>"; + + // Act: Parse should succeed without throwing exceptions + var namingTemplate = NamingTemplate.NamingTemplate.Parse(template, [_conditionalTags]); + + // Assert: Should parse successfully (may have warnings but no exceptions) + Assert.IsNotNull(namingTemplate); + } + + /// + /// Test that regex patterns with special characters don't cause issues. + /// + [TestMethod] + [DataRow("^test$", DisplayName = "RegexAnchors")] + [DataRow("test.*value", DisplayName = "RegexWildcard")] + [DataRow(@"[a-z\]+", DisplayName = "RegexCharacterClass")] + [DataRow("test|value", DisplayName = "RegexAlternation")] + public void ConditionalTag_ValidComplexRegexPatterns_ParseSuccessfully(string pattern) + { + // Arrange: Valid complex regex patterns with proper closing tags + var template = $"c<-testcond>"; + + // Act: Parse should succeed without throwing + var namingTemplate = NamingTemplate.NamingTemplate.Parse(template, [_conditionalTags]); + + // Assert: Should parse successfully without exceptions + Assert.IsNotNull(namingTemplate); + } +} \ No newline at end of file diff --git a/Source/_Tests/FileManager.Tests/FileNamingTemplateTests.cs b/Source/_Tests/FileManager.Tests/FileNamingTemplateTests.cs index 6556a048..e4d9d2a0 100644 --- a/Source/_Tests/FileManager.Tests/FileNamingTemplateTests.cs +++ b/Source/_Tests/FileManager.Tests/FileNamingTemplateTests.cs @@ -273,14 +273,14 @@ public class GetPortionFilename templateText.Should().Be(outStr); - string FormatInt(ITemplateTag templateTag, int value, string format, CultureInfo? culture) + string FormatInt(ITemplateTag templateTag, int value, string? format, CultureInfo? culture) { if (int.TryParse(format, out var numDecs)) return value.ToString($"D{numDecs}", culture); return value.ToString(culture); } - string FormatString(ITemplateTag templateTag, string? value, string format, CultureInfo? culture) + string FormatString(ITemplateTag templateTag, string? value, string? format, CultureInfo? culture) { return CommonFormatters.StringFormatter(templateTag, value, format, culture); } diff --git a/Source/_Tests/LibationFileManager.Tests/TemplatesTests.cs b/Source/_Tests/LibationFileManager.Tests/TemplatesTests.cs index 84b1945b..8db009ba 100644 --- a/Source/_Tests/LibationFileManager.Tests/TemplatesTests.cs +++ b/Source/_Tests/LibationFileManager.Tests/TemplatesTests.cs @@ -167,7 +167,7 @@ namespace TemplatesTests [DataRow("Kbps ", "128Kbps A STUDY IN SCARLET")] [DataRow("Kbps ", "128Kbps A Study In Scarlet")] [DataRow("Kbps ", "0128Kbps a study in scarlet")] - [DataRow(" Hz", "Aac[Lc]Mp3 044100Hz")] + [DataRow(" Hz", "Aac[Lc] 044100Hz")] [DataRow(" ", "AAC A STU")] [DataRow("Kbps Hz", "0128Kbps 044100Hz")] public void FormatTags(string template, string expected)