diff --git a/CLAUDE.md b/CLAUDE.md index bba164a2..0e7bcaa2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -149,6 +149,12 @@ almost always be doing. the assertion is about control characters, use `Contains.Substring(x).Using(StringComparison.Ordinal)`, negated with the `!` operator that `Constraint` defines, or assert the whole value with `Is.EqualTo`, which is ordinal. +- **Give a `[TestCase]` an explicit `TestName` when an argument holds a control character.** + Otherwise the whole fixture can become invisible to `dotnet test --filter`, silently: it is + listed by `--list-tests` and runs in a full pass, but every filter reports "No test matches". + Reproduced with `[TestCase("one", "\x1b[0m")]`; a single argument holding the same escape is + fine, so it takes two arguments and an escape character. `AnsiColorTerminalAppenderTest` names + all ten of its cases for that reason, and a filtered run there is 54 ms against 9 s for the suite. - Mark a test `[NonParallelizable]` when it mutates static state (`LogLog.InternalDebugging`, a static field on a test double, a process-wide native registration). - Wrap expected internal logging in `LogLog.ExecuteWithoutEmittingInternalMessages(...)` and capture diff --git a/src/changelog/3.5.0/313-redact-connection-string-allowlist.xml b/src/changelog/3.5.0/313-redact-connection-string-allowlist.xml index 0aec3b5d..e020b60a 100644 --- a/src/changelog/3.5.0/313-redact-connection-string-allowlist.xml +++ b/src/changelog/3.5.0/313-redact-connection-string-allowlist.xml @@ -8,6 +8,6 @@ keep secrets out of the `AdoNetAppender` message for a connection it could not open. Hiding password-bearing keywords missed `Extended Properties`, which nests a whole connection string, and keywords such as `AccessToken` (CWE-532). Only keywords naming the server and account are kept now - (audit da18b6fd-f028) + (audit da18b6fd-f028, fixed by @FreeAndNil) diff --git a/src/changelog/3.5.0/313-require-powershell-74.xml b/src/changelog/3.5.0/313-require-powershell-74.xml index 3222a5e4..e0ab2306 100644 --- a/src/changelog/3.5.0/313-require-powershell-74.xml +++ b/src/changelog/3.5.0/313-require-powershell-74.xml @@ -9,6 +9,6 @@ `$PSNativeCommandUseErrorActionPreference`, which exists only from 7.4, so under Windows PowerShell 5.1 a failing `gpg --verify` was ignored and `verify-release.ps1` reported success and exited 0. The scripts now refuse to start on an older host, and the review instructions install PowerShell 7 and - run the script with `pwsh` (audit 1231d72-f009) + run the script with `pwsh` (audit 1231d72-f009, implemented by @FreeAndNil) diff --git a/src/changelog/3.5.0/313-verify-release-keys-bypass.xml b/src/changelog/3.5.0/313-verify-release-keys-bypass.xml index e49e157c..215ad438 100644 --- a/src/changelog/3.5.0/313-verify-release-keys-bypass.xml +++ b/src/changelog/3.5.0/313-verify-release-keys-bypass.xml @@ -10,6 +10,6 @@ and was imported into the verification key ring, and artifacts signed by whoever placed it verified (CWE-347). The scripts now verify in a GnuPG home of their own, filled from a copy downloaded there, rather than with `--keyring`, which `gpg` ignores where `common.conf` sets `use-keyboxd`. - Present in 3.2.0 onward, since the script was added (audit da18b6fd-f003, reported by @swebb2066) + Present in 3.2.0 onward, since the script was added (audit da18b6fd-f003, reported by @swebb2066, fixed by @FreeAndNil) diff --git a/src/changelog/3.5.0/314-ext-mail-background-sender.xml b/src/changelog/3.5.0/314-ext-mail-background-sender.xml index f16ce22f..acbe9d09 100644 --- a/src/changelog/3.5.0/314-ext-mail-background-sender.xml +++ b/src/changelog/3.5.0/314-ext-mail-background-sender.xml @@ -10,6 +10,6 @@ for the SMTP server. The queue holds `sendQueueSize` mails (500) and a logging call waits at most `enqueueTimeoutMillis` (5000) for room in it. Failures are still reported to the error handler, but after the logging call has returned, and `Flush` now honours its timeout - (implemented by @FreeAndNil) + (audit da18b6fd-f004, implemented by @FreeAndNil) diff --git a/src/changelog/3.5.0/314-ext-mail-send-timeout.xml b/src/changelog/3.5.0/314-ext-mail-send-timeout.xml index 2a8a0469..8ec6b8f6 100644 --- a/src/changelog/3.5.0/314-ext-mail-send-timeout.xml +++ b/src/changelog/3.5.0/314-ext-mail-send-timeout.xml @@ -9,6 +9,6 @@ per operation by default, and the mail goes out while the appender lock is held, so an unresponsive server suspended every thread logging through the appender. `SendTimeoutMillis` is a deadline for the send as a whole, because a per operation timeout still allows a multiple of - itself overall (implemented by @FreeAndNil) + itself overall (audit da18b6fd-f004, implemented by @FreeAndNil) diff --git a/src/changelog/3.5.0/314-remote-syslog-background-sender.xml b/src/changelog/3.5.0/314-remote-syslog-background-sender.xml index 9ea954df..1894ef20 100644 --- a/src/changelog/3.5.0/314-remote-syslog-background-sender.xml +++ b/src/changelog/3.5.0/314-remote-syslog-background-sender.xml @@ -9,6 +9,6 @@ stopped accepting datagrams grew it until the process ran out of memory, and shutdown waited five seconds and then abandoned a drain that had no limit of its own. It now holds `sendQueueSize` datagrams (500), a logging call waits at most `enqueueTimeoutMillis` (5000) for room, losses are - counted and reported, and `Flush` honours its timeout (implemented by @FreeAndNil) + counted and reported, and `Flush` honours its timeout (audit da18b6fd-f034, implemented by @FreeAndNil) diff --git a/src/changelog/3.5.0/314-remote-syslog-socket-and-connect.xml b/src/changelog/3.5.0/314-remote-syslog-socket-and-connect.xml index fb2185c7..2785a792 100644 --- a/src/changelog/3.5.0/314-remote-syslog-socket-and-connect.xml +++ b/src/changelog/3.5.0/314-remote-syslog-socket-and-connect.xml @@ -9,6 +9,6 @@ connection its background pump owns, but also inherited one from `UdpAppender` that nothing ever used, which bound `localPort` twice. A pump that cannot connect now reports it as well, instead of ending unobserved and leaving every later event queued behind a sender that is gone - (implemented by @FreeAndNil) + (audit da18b6fd-f034, implemented by @FreeAndNil) diff --git a/src/changelog/3.5.0/315-eventlog-nul.xml b/src/changelog/3.5.0/315-eventlog-nul.xml index 2ac7a56f..117ad85f 100644 --- a/src/changelog/3.5.0/315-eventlog-nul.xml +++ b/src/changelog/3.5.0/315-eventlog-nul.xml @@ -10,6 +10,6 @@ the layout rendered after it, exception text and trailing fields included (CWE-158). `WriteEntry` raises nothing, so the record simply stored short. Measured on Windows 11 build 26200: of a 45 character message with a NUL at 23, the 23 character prefix was stored and the rest was gone - (audit da18b6fd-f007) + (audit da18b6fd-f007, fixed by @FreeAndNil) diff --git a/src/changelog/3.5.0/315-eventlog-size-budget.xml b/src/changelog/3.5.0/315-eventlog-size-budget.xml index faa41f34..5a38f539 100644 --- a/src/changelog/3.5.0/315-eventlog-size-budget.xml +++ b/src/changelog/3.5.0/315-eventlog-size-budget.xml @@ -12,6 +12,6 @@ that the service stores nothing and reports nothing. The whole event was lost rather than shortened, and `applicationName` defaults to the app domain name, so a consumer with a long assembly name lost more. The limit is now computed, and a truncation is reported through the - error handler, which is the only signal available (audit da18b6fd-f030) + error handler, which is the only signal available (audit da18b6fd-f030, fixed by @FreeAndNil) diff --git a/src/changelog/3.5.0/315-local-syslog-newlines.xml b/src/changelog/3.5.0/315-local-syslog-newlines.xml index 4c7a6a47..3b002e42 100644 --- a/src/changelog/3.5.0/315-local-syslog-newlines.xml +++ b/src/changelog/3.5.0/315-local-syslog-newlines.xml @@ -10,6 +10,6 @@ records everything after the newline as its own entry, so content could forge an authentic looking record (CWE-117). `NewLineHandling` mirrors the option of the same name on `RemoteSyslogAppender`, which already escaped by default; set it to `Keep` for the previous - behaviour (audit da18b6fd-f008) + behaviour (audit da18b6fd-f008, fixed by @FreeAndNil) diff --git a/src/changelog/3.5.0/315-outputdebugstring-nul.xml b/src/changelog/3.5.0/315-outputdebugstring-nul.xml index b2adef68..0ed4c455 100644 --- a/src/changelog/3.5.0/315-outputdebugstring-nul.xml +++ b/src/changelog/3.5.0/315-outputdebugstring-nul.xml @@ -8,6 +8,6 @@ escape NUL characters in `OutputDebugStringAppender` content. `OutputDebugStringW` takes a null terminated string, so a NUL in logged content ended the record there and silently dropped whatever the layout rendered after it, exception text and trailing fields included (CWE-158). The escape - `LocalSyslogAppender` already applied is now shared between the two (audit da18b6fd-f009) + `LocalSyslogAppender` already applied is now shared between the two (audit da18b6fd-f009, fixed by @FreeAndNil) diff --git a/src/changelog/3.5.0/315-pickup-dir-unencodable-content.xml b/src/changelog/3.5.0/315-pickup-dir-unencodable-content.xml index 8f952bf1..7953e8bc 100644 --- a/src/changelog/3.5.0/315-pickup-dir-unencodable-content.xml +++ b/src/changelog/3.5.0/315-pickup-dir-unencodable-content.xml @@ -8,6 +8,6 @@ stop one logging event destroying a whole `SmtpPickupDirAppender` batch. `File.CreateText` throws on content it cannot encode, such as an unpaired surrogate, which abandoned every buffered event and left a truncated mail in the pickup directory for the service to send. Such content is - now written as a `\uXXXX` escape (audit da18b6fd-f011) + now written as a `\uXXXX` escape (audit da18b6fd-f011, fixed by @FreeAndNil) diff --git a/src/changelog/3.5.0/315-syslog-non-ascii.xml b/src/changelog/3.5.0/315-syslog-non-ascii.xml index e8020168..edcf04df 100644 --- a/src/changelog/3.5.0/315-syslog-non-ascii.xml +++ b/src/changelog/3.5.0/315-syslog-non-ascii.xml @@ -9,6 +9,6 @@ allows only the visible ASCII characters and space, and everything else was dropped silently, so `Schönwetter 你好` reached the collector as `Schnwetter ` and a tab disappeared from between its neighbours. Such characters are now written as a `\uXXXX` escape, which keeps - the record inside the allowed range and readable (audit da18b6fd-f035) + the record inside the allowed range and readable (audit da18b6fd-f035, fixed by @FreeAndNil) diff --git a/src/changelog/3.5.0/315-telnet-unencodable-content.xml b/src/changelog/3.5.0/315-telnet-unencodable-content.xml index ad96651f..d1b44771 100644 --- a/src/changelog/3.5.0/315-telnet-unencodable-content.xml +++ b/src/changelog/3.5.0/315-telnet-unencodable-content.xml @@ -8,6 +8,6 @@ stop one logging event disconnecting every `TelnetAppender` client. The default writer encoding throws on content it cannot encode, such as an unpaired surrogate, and `Send` reads any failure as a client that hung up. Unpaired surrogates are now written as a `\uXXXX` escape, as elsewhere - (audit da18b6fd-f013) + (audit da18b6fd-f013, fixed by @FreeAndNil) diff --git a/src/changelog/3.5.0/316-ansi-empty-render.xml b/src/changelog/3.5.0/316-ansi-empty-render.xml new file mode 100644 index 00000000..1f32068d --- /dev/null +++ b/src/changelog/3.5.0/316-ansi-empty-render.xml @@ -0,0 +1,13 @@ + + + + + stop `AnsiColorTerminalAppender` dropping an event that renders to nothing. The branch meant + for a single character read the first one without checking there was one, so an empty render + threw and the event was lost. The reset codes are now placed by one computed offset, which has no + special case to get wrong (audit da18b6fd-f029, fixed by @FreeAndNil) + + diff --git a/src/changelog/3.5.0/316-aspnet-request-event-loss.xml b/src/changelog/3.5.0/316-aspnet-request-event-loss.xml new file mode 100644 index 00000000..39dd62fc --- /dev/null +++ b/src/changelog/3.5.0/316-aspnet-request-event-loss.xml @@ -0,0 +1,13 @@ + + + + Stop `%aspnet-request` losing the whole event for a request that fails + ASP.NET request validation. Reading `HttpRequest.Params` validates the query string, form and + cookies on first access, so a request carrying `<script>` threw inside the layout and the + appender discarded the event: a sender could suppress the log record of their own request. The + converter now reads through `HttpRequest.Unvalidated`, which keeps the content instead of + dropping it (audit da18b6fd-f019, fixed by @FreeAndNil) + diff --git a/src/changelog/3.5.0/317-filter-ordinal-substring.xml b/src/changelog/3.5.0/317-filter-ordinal-substring.xml new file mode 100644 index 00000000..46f85331 --- /dev/null +++ b/src/changelog/3.5.0/317-filter-ordinal-substring.xml @@ -0,0 +1,12 @@ + + + + Compare `StringToMatch` ordinally in `StringMatchFilter` and `PropertyFilter`. + The substring search was culture sensitive, and a linguistic search skips ignorable characters, so + content holding a NUL, a soft hyphen or a zero-width space between the letters of the configured + text still matched it, and the decision varied with the host culture. The filter now decides the + same way a reader of the log would (audit da18b6fd-f018, fixed by @FreeAndNil) + diff --git a/src/changelog/3.5.0/317-filter-timeout-decision.xml b/src/changelog/3.5.0/317-filter-timeout-decision.xml new file mode 100644 index 00000000..1e6e88c2 --- /dev/null +++ b/src/changelog/3.5.0/317-filter-timeout-decision.xml @@ -0,0 +1,13 @@ + + + + `TimeoutDecision` on `StringMatchFilter` and `PropertyFilter`, deciding an + event whose regular expression match was abandoned. An abandoned match was treated as a non-match, + but the content decides whether the deadline is reached, so in an `AcceptOnMatch` allowlist + followed by a deny-all it let content suppress its own record. The default stays `Neutral`; + `Accept` makes such a chain fail towards logging (audit da18b6fd-f017, implemented by + @FreeAndNil) + diff --git a/src/changelog/3.5.0/317-filter-timeout-default.xml b/src/changelog/3.5.0/317-filter-timeout-default.xml new file mode 100644 index 00000000..aed27a53 --- /dev/null +++ b/src/changelog/3.5.0/317-filter-timeout-default.xml @@ -0,0 +1,12 @@ + + + + The default `MatchTimeoutMillis` on `StringMatchFilter` and + `PropertyFilter` is now 50 instead of 1000. The match runs while the appender lock is held, so the + deadline is the bound on how long one crafted event can stall everything logging through the + appender. A legitimate match over an event takes a fraction of 50ms; raise the property if a + pattern genuinely needs longer (audit da18b6fd-f041, implemented by @FreeAndNil) + diff --git a/src/log4net.Tests/Appender/AnsiColorTerminalAppenderTest.cs b/src/log4net.Tests/Appender/AnsiColorTerminalAppenderTest.cs new file mode 100644 index 00000000..222dce26 --- /dev/null +++ b/src/log4net.Tests/Appender/AnsiColorTerminalAppenderTest.cs @@ -0,0 +1,104 @@ +#region Apache License +// +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to you under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#endregion + +using System; +using System.IO; + +using log4net.Appender; +using log4net.Core; +using log4net.Layout; + +using NUnit.Framework; + +namespace log4net.Tests.Appender; + +/// +/// Tests for , which places the terminal reset codes +/// before any trailing line break so the colour ends with the text. +/// +[TestFixture] +[NonParallelizable] +public class AnsiColorTerminalAppenderTest +{ + /// Matches the appender's private PostEventCodes. + private const string Reset = "\x1b[0m"; + + /// The reset codes belong before the line break, whichever one it is. + // Explicit names: two arguments where one holds an escape character make the whole fixture + // invisible to "dotnet test --filter", reproduced with [TestCase("one", "\x1b[0m")]. + [TestCase("", Reset, TestName = "AnEmptyRender")] + [TestCase("x", "x" + Reset, TestName = "ASingleCharacter")] + [TestCase("\n", Reset + "\n", TestName = "NothingButALineFeed")] + [TestCase("text", "text" + Reset, TestName = "NoTrailingLineBreak")] + [TestCase("\r", Reset + "\r", TestName = "NothingButACarriageReturn")] + [TestCase("text\n", "text" + Reset + "\n", TestName = "TrailingLineFeed")] + [TestCase("text\r", "text" + Reset + "\r", TestName = "TrailingCarriageReturn")] + [TestCase("text\r\n", "text" + Reset + "\r\n", TestName = "TrailingCarriageReturnLineFeed")] + [TestCase("text\n\r", "text" + Reset + "\n\r", TestName = "TrailingLineFeedCarriageReturn")] + [TestCase("text\n\n", "text\n" + Reset + "\n", TestName = "TrailingDoubledLineFeedCountsAsOne")] + public void TheResetCodesGoBeforeATrailingLineBreak(string message, string expected) + { + RecordingErrorHandler errorHandler = new(); + // Level.Info has no colour mapping configured, so nothing is prepended and the rendered + // message is exactly what was logged, down to the empty string. + AnsiColorTerminalAppender appender = new() + { + Layout = new PatternLayout("%message"), + ErrorHandler = errorHandler + }; + appender.ActivateOptions(); + + TextWriter previous = Console.Out; + using StringWriter captured = new(); + try + { + Console.SetOut(captured); + // DoAppend is overloaded on LoggingEvent and LoggingEvent[], so this new cannot be short. + appender.DoAppend(new LoggingEvent(new() + { + Level = Level.Info, + Message = message, + LoggerName = nameof(AnsiColorTerminalAppenderTest) + })); + } + finally + { + Console.SetOut(previous); + } + + Assert.That(errorHandler.Message, Is.Empty, "the event must not be dropped"); + Assert.That(captured.ToString(), Is.EqualTo(expected)); + } + + /// Collects what the appender reports, so a dropped event is visible. + private sealed class RecordingErrorHandler : IErrorHandler + { + /// Everything reported so far. + internal string Message { get; private set; } = string.Empty; + + /// + public void Error(string message) => Message += message + '\n'; + + /// + public void Error(string message, Exception e) => Message += message + '\n'; + + /// + public void Error(string message, Exception? e, ErrorCode errorCode) => Message += message + '\n'; + } +} diff --git a/src/log4net.Tests/Filter/PropertyFilterTest.cs b/src/log4net.Tests/Filter/PropertyFilterTest.cs new file mode 100644 index 00000000..c77b447b --- /dev/null +++ b/src/log4net.Tests/Filter/PropertyFilterTest.cs @@ -0,0 +1,119 @@ +#region Apache License +// +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to you under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#endregion + +using System; + +using log4net.Core; +using log4net.Filter; +using log4net.Repository; +using log4net.Util; + +using NUnit.Framework; + +namespace log4net.Tests.Filter; + +/// +/// Tests for , which decides a property value through the same +/// matching as . +/// +[TestFixture] +public class PropertyFilterTest +{ + private const string Key = "user"; + + /// + /// A pattern that backtracks, with a value that makes it do so. + /// + private const string CatastrophicPattern = "^(a+)+$"; + + private const string CraftedValue = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa!"; + + /// + /// A property value is content too, so an ignorable character must not force a match here + /// either. + /// + [TestCase("\0", TestName = "ANulDoesNotForceAPropertyMatch")] + [TestCase("­", TestName = "ASoftHyphenDoesNotForceAPropertyMatch")] + public void AnIgnorableCharacterDoesNotForceAPropertyMatch(string ignorable) + { + PropertyFilter filter = new() { Key = Key, StringToMatch = "admin", AcceptOnMatch = true }; + filter.ActivateOptions(); + + Assert.That(filter.Decide(CreateEvent($"adm{ignorable}in")), Is.EqualTo(FilterDecision.Neutral)); + } + + /// + /// The exact value still decides, so the ordinal comparison has not broken the filter. + /// + [Test] + public void AnExactPropertyValueStillMatches() + { + PropertyFilter filter = new() { Key = Key, StringToMatch = "admin", AcceptOnMatch = false }; + filter.ActivateOptions(); + + Assert.That(filter.Decide(CreateEvent("admin")), Is.EqualTo(FilterDecision.Deny)); + Assert.That(filter.Decide(CreateEvent("guest")), Is.EqualTo(FilterDecision.Neutral)); + } + + /// + /// An abandoned match is decided the same way as in the base filter. + /// + [Test] + public void AnAbandonedPropertyMatchHonoursTimeoutDecision() + { + PropertyFilter filter = new() + { + Key = Key, + RegexToMatch = CatastrophicPattern, + MatchTimeoutMillis = 50, + TimeoutDecision = FilterDecision.Accept + }; + filter.ActivateOptions(); + + FilterDecision decision = FilterDecision.Neutral; + LogLog.ExecuteWithoutEmittingInternalMessages(() => decision = filter.Decide(CreateEvent(CraftedValue))); + + Assert.That(decision, Is.EqualTo(FilterDecision.Accept)); + } + + /// + /// Without a key there is nothing to look up, so the chain continues. + /// + [Test] + public void AFilterWithoutAKeyIsNeutral() + { + PropertyFilter filter = new() { StringToMatch = "admin" }; + filter.ActivateOptions(); + + Assert.That(filter.Decide(CreateEvent("admin")), Is.EqualTo(FilterDecision.Neutral)); + } + + /// + /// Builds an event carrying under . The filter renders + /// the property through the repository, so the event needs one. + /// + private static LoggingEvent CreateEvent(string value) + { + ILoggerRepository repository = LogManager.CreateRepository(Guid.NewGuid().ToString()); + LoggingEventData data = new() { Level = Level.Info, Message = "TestMessage", LoggerName = "TestLogger" }; + LoggingEvent loggingEvent = new(null, repository, data); + loggingEvent.Properties[Key] = value; + return loggingEvent; + } +} diff --git a/src/log4net.Tests/Filter/StringMatchFilterTest.cs b/src/log4net.Tests/Filter/StringMatchFilterTest.cs index ff3eced6..641d58fa 100644 --- a/src/log4net.Tests/Filter/StringMatchFilterTest.cs +++ b/src/log4net.Tests/Filter/StringMatchFilterTest.cs @@ -50,7 +50,7 @@ public class StringMatchFilterTest [Test] public void AMatchThatBacktracksIsAbandoned() { - StringMatchFilter filter = new() { RegexToMatch = CatastrophicPattern, MatchTimeoutMillis = 100 }; + StringMatchFilter filter = new() { RegexToMatch = CatastrophicPattern, MatchTimeoutMillis = 50 }; filter.ActivateOptions(); Stopwatch stopwatch = Stopwatch.StartNew(); @@ -70,7 +70,7 @@ public void AMatchThatBacktracksIsAbandoned() [NonParallelizable] public void AnAbandonedMatchIsReportedOnce() { - StringMatchFilter filter = new() { RegexToMatch = CatastrophicPattern, MatchTimeoutMillis = 100 }; + StringMatchFilter filter = new() { RegexToMatch = CatastrophicPattern, MatchTimeoutMillis = 50 }; filter.ActivateOptions(); List messages = []; @@ -101,12 +101,72 @@ public void AMatchingPatternStillDecides() } /// - /// The deadline has to be finite by default, so that an expensive pattern cannot hold the - /// appender lock indefinitely without the operator having opted into that. + /// The deadline bounds how long one crafted event holds the appender lock, so it has to be + /// finite by default and short enough to matter. /// [Test] public void MatchTimeoutMillisDefaultsToAFiniteValue() - => Assert.That(new StringMatchFilter().MatchTimeoutMillis, Is.EqualTo(1000)); + => Assert.That(new StringMatchFilter().MatchTimeoutMillis, Is.EqualTo(50)); + + /// + /// An abandoned match leaves the chain to decide unless the operator says otherwise, so the + /// default has to stay Neutral. + /// + [Test] + public void TimeoutDecisionDefaultsToNeutral() + => Assert.That(new StringMatchFilter().TimeoutDecision, Is.EqualTo(FilterDecision.Neutral)); + + /// + /// The content decides whether the deadline is reached, so an allowlist that treats an abandoned + /// match as a non-match lets content suppress its own record. TimeoutDecision is how such a + /// chain fails towards logging instead. + /// + [TestCase(FilterDecision.Deny, TestName = "AnAbandonedMatchIsDecidedByTimeoutDecisionDeny")] + [TestCase(FilterDecision.Accept, TestName = "AnAbandonedMatchIsDecidedByTimeoutDecisionAccept")] + [TestCase(FilterDecision.Neutral, TestName = "AnAbandonedMatchIsDecidedByTimeoutDecisionNeutral")] + public void AnAbandonedMatchIsDecidedByTimeoutDecision(FilterDecision timeoutDecision) + { + StringMatchFilter filter = new() + { + RegexToMatch = CatastrophicPattern, + MatchTimeoutMillis = 50, + TimeoutDecision = timeoutDecision + }; + filter.ActivateOptions(); + + FilterDecision decision = FilterDecision.Accept; + LogLog.ExecuteWithoutEmittingInternalMessages(() => decision = filter.Decide(CreateEvent(CraftedMessage))); + + Assert.That(decision, Is.EqualTo(timeoutDecision)); + } + + /// + /// A linguistic search skips ignorable characters, so content could otherwise force a substring + /// match that no ordinal reader of the same log would make. + /// + [TestCase("\0", TestName = "ANulDoesNotForceASubstringMatch")] + [TestCase("­", TestName = "ASoftHyphenDoesNotForceASubstringMatch")] + [TestCase("​", TestName = "AZeroWidthSpaceDoesNotForceASubstringMatch")] + public void AnIgnorableCharacterDoesNotForceASubstringMatch(string ignorable) + { + StringMatchFilter filter = new() { StringToMatch = "password", AcceptOnMatch = true }; + filter.ActivateOptions(); + + Assert.That(filter.Decide(CreateEvent($"pass{ignorable}word")), Is.EqualTo(FilterDecision.Neutral)); + } + + /// + /// The ordinal comparison must not break the match it exists to protect. + /// + [Test] + public void AnExactSubstringStillMatches() + { + StringMatchFilter filter = new() { StringToMatch = "password", AcceptOnMatch = true }; + filter.ActivateOptions(); + + Assert.That(filter.Decide(CreateEvent("a password here")), Is.EqualTo(FilterDecision.Accept)); + Assert.That(filter.Decide(CreateEvent("nothing here")), Is.EqualTo(FilterDecision.Neutral)); + } /// /// 0 is the documented opt-out that restores unbounded matching; a negative deadline has no diff --git a/src/log4net.Tests/Layout/Pattern/AspNetRequestPatternConverterTest.cs b/src/log4net.Tests/Layout/Pattern/AspNetRequestPatternConverterTest.cs new file mode 100644 index 00000000..115e164e --- /dev/null +++ b/src/log4net.Tests/Layout/Pattern/AspNetRequestPatternConverterTest.cs @@ -0,0 +1,147 @@ +#region Apache License +// +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to you under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#endregion + +// netstandard has no System.Web +#if NET462_OR_GREATER + +using System; +using System.IO; +using System.Web; + +using log4net.Config; +using log4net.Layout; +using log4net.Repository; +using log4net.Tests.Appender; +using log4net.Util; + +using NUnit.Framework; + +namespace log4net.Tests.Layout.Pattern; + +/// +/// Tests that %aspnet-request survives content ASP.NET request validation rejects. +/// +[TestFixture] +public sealed class AspNetRequestPatternConverterTest +{ + private const string Payload = "