Prevent the ANSI appender and %aspnet-request dropping events

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
@@ -229,9 +235,15 @@ What that leaves for this file is where the answers live in the code:
   at the site with a link to the model rather than changing the code. `XmlConfigurator` and
   `XmlHierarchyConfigurator` carry these for the configuration-is-trusted paths, and
   `SystemStringFormat` for the format string.
-- `LocalSyslogAppender.EscapeNulCharacters` and `RemoteSyslogAppender.ValidateIdentity` are the two
+- `log4net.Appender.Internal.ContentEscape` and `RemoteSyslogAppender.ValidateIdentity` are the two
   sides of the content and structural-identifier rule: content is escaped and never rejected, a
   malformed identifier is reported rather than quietly repaired.
+- **A sink that cannot carry a character escapes it visibly, and never drops the character, the
+  rest of the record, or the event.** The escapes already in use are `\0` for NUL, `\r` and `\n`
+  for newlines, and `\uXXXX` for anything else, in `ContentEscape` and in
+  `RemoteSyslogAppender.AppendMessage`. Put new ones in `ContentEscape` rather than in the
+  appender: four appenders have needed the same two so far. Escaping before a length limit, not
+  after, since an escape is longer than what it replaces.
 - Deliberate secure-default choices belong in the changelog with their opt-out named, so that an
   upgrade surprise is searchable. See the entries for `SendTimeoutMillis`, `MatchTimeoutMillis` and
   `LockTimeoutMillis`.

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)
   </description>
 </entry>

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)
   </description>
 </entry>

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)
   </description>
 </entry>

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)
   </description>
 </entry>

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)
   </description>
 </entry>

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)
   </description>
 </entry>

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)
   </description>
 </entry>

src/changelog/3.5.0/315-eventlog-nul.xml

@@ -0,0 +1,15 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="315" link="https://github.com/apache/logging-log4net/pull/315"/>
+  <description format="asciidoc">
+    escape NUL characters in `EventLogAppender` content. `ReportEventW` takes a null terminated
+ string, so a NUL in logged content ended the stored record there and silently dropped whatever
+ 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, fixed by @FreeAndNil)
+  </description>
+</entry>

src/changelog/3.5.0/315-eventlog-size-budget.xml

@@ -0,0 +1,17 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="315" link="https://github.com/apache/logging-log4net/pull/315"/>
+  <description format="asciidoc">
+    stop `EventLogAppender` truncating to a size the event log then discards. The limit is a whole
+ record budget that the log name, the source and the machine name are spent from, so the fixed
+ 31837 was above the real ceiling: measured on Windows 11 build 26200, a record is stored while
+ `message + logName + applicationName` stays within 31736 characters, and one character beyond
+ 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, fixed by @FreeAndNil)
+  </description>
+</entry>

src/changelog/3.5.0/315-local-syslog-newlines.xml

@@ -0,0 +1,15 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="315" link="https://github.com/apache/logging-log4net/pull/315"/>
+  <description format="asciidoc">
+    escape the newlines in logged content in `LocalSyslogAppender`, which passed them to
+ `syslog(3)` unchanged. A daemon that writes the message through to a line oriented log then
+ 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, fixed by @FreeAndNil)
+  </description>
+</entry>

src/changelog/3.5.0/315-outputdebugstring-nul.xml

@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="315" link="https://github.com/apache/logging-log4net/pull/315"/>
+  <description format="asciidoc">
+    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, fixed by @FreeAndNil)
+  </description>
+</entry>

src/changelog/3.5.0/315-pickup-dir-unencodable-content.xml

@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="315" link="https://github.com/apache/logging-log4net/pull/315"/>
+  <description format="asciidoc">
+    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, fixed by @FreeAndNil)
+  </description>
+</entry>

src/changelog/3.5.0/315-syslog-newline-handling-type.xml

@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="changed">
+  <issue id="315" link="https://github.com/apache/logging-log4net/pull/315"/>
+  <description format="asciidoc">
+    move `SyslogNewLineHandling` out of `RemoteSyslogAppender` to `log4net.Appender`, now that
+ `LocalSyslogAppender` uses it too. Configuration files are unaffected, they bind the value by
+ name, but code naming `RemoteSyslogAppender.SyslogNewLineHandling` has to drop the prefix
+ (implemented by @FreeAndNil)
+  </description>
+</entry>

src/changelog/3.5.0/315-syslog-non-ascii.xml

@@ -0,0 +1,14 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="315" link="https://github.com/apache/logging-log4net/pull/315"/>
+  <description format="asciidoc">
+    escape the characters `RemoteSyslogAppender` cannot send instead of deleting them. RFC 3164
+ allows only the visible ASCII characters and space, and everything else was dropped silently, so
+ `Sch&#246;nwetter &#20320;&#22909;` 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, fixed by @FreeAndNil)
+  </description>
+</entry>

src/changelog/3.5.0/315-telnet-unencodable-content.xml

@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="315" link="https://github.com/apache/logging-log4net/pull/315"/>
+  <description format="asciidoc">
+    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, fixed by @FreeAndNil)
+  </description>
+</entry>

src/changelog/3.5.0/316-ansi-empty-render.xml

@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="316" link="https://github.com/apache/logging-log4net/pull/316"/>
+  <description format="asciidoc">
+    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)
+  </description>
+</entry>

src/changelog/3.5.0/316-aspnet-request-event-loss.xml

@@ -0,0 +1,13 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="316" link="https://github.com/apache/logging-log4net/pull/316"/>
+  <description format="asciidoc">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 `&lt;script&gt;` 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)</description>
+</entry>

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;
+
+/// <summary>
+/// Tests for <see cref="AnsiColorTerminalAppender"/>, which places the terminal reset codes
+/// before any trailing line break so the colour ends with the text.
+/// </summary>
+[TestFixture]
+[NonParallelizable]
+public class AnsiColorTerminalAppenderTest
+{
+  /// <summary>Matches the appender's private PostEventCodes.</summary>
+  private const string Reset = "\x1b[0m";
+
+  /// <summary>The reset codes belong before the line break, whichever one it is.</summary>
+  // 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));
+  }
+
+  /// <summary>Collects what the appender reports, so a dropped event is visible.</summary>
+  private sealed class RecordingErrorHandler : IErrorHandler
+  {
+    /// <summary>Everything reported so far.</summary>
+    internal string Message { get; private set; } = string.Empty;
+
+    /// <inheritdoc/>
+    public void Error(string message) => Message += message + '\n';
+
+    /// <inheritdoc/>
+    public void Error(string message, Exception e) => Message += message + '\n';
+
+    /// <inheritdoc/>
+    public void Error(string message, Exception? e, ErrorCode errorCode) => Message += message + '\n';
+  }
+}