stop appenders losing or leaking what they write

src/changelog/3.5.0/324-config-debug-secret-values.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="324" link="https://github.com/apache/logging-log4net/pull/324"/>
+  <description format="asciidoc">
+    stop the configurator echoing secret configuration values. A parameter whose name reads as a
+ secret, such as `password` or `connectionString`, is now logged as `*****`, the connection string
+ keeping only the keywords that name the server. This covers internal debugging, the documented
+ first troubleshooting step, and the four setter-failure messages, which are written even without
+ it (audit da18b6fd-f042, fixed by @FreeAndNil)
+  </description>
+</entry>

src/changelog/3.5.0/324-pickup-dir-dot-terminator.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="changed">
+  <issue id="324" link="https://github.com/apache/logging-log4net/pull/324"/>
+  <description format="asciidoc">
+    `SmtpPickupDirAppender` no longer writes a lone dot at the end of the file. The pickup service
+ reads the file to its end, so the terminator was never needed, and it made a logged line that is
+ only a dot look like the end of the mail. Body lines are still written verbatim rather than dot
+ stuffed, so an agent that expects SMTP DATA framing is not a supported consumer (audit
+ da18b6fd-f037, implemented by @FreeAndNil)
+  </description>
+</entry>

src/changelog/3.5.0/324-udp-oversize-truncation.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="324" link="https://github.com/apache/logging-log4net/pull/324"/>
+  <description format="asciidoc">
+    stop `UdpAppender` and `RemoteSyslogAppender` losing an event the socket refuses to send. An
+ event over the new `MaxDatagramSize` (default 65507, configurable down to 512) is truncated on a
+ character boundary and marked `...[truncated]`, instead of being dropped with only the first loss
+ reported. The limit is the IPv4 maximum and cannot be raised, an IPv6 packet would carry 20 bytes
+ more (audit da18b6fd-f015, fixed by @FreeAndNil)
+  </description>
+</entry>

src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs

@@ -67,6 +67,21 @@ public void NonAsciiContentIsEscapedAndNotDeleted()
       Is.EqualTo(@"<14>TestDomain: INFO  - Sch\u00f6nwetter \u4f60\u597d"));
   }
 
+  /// <summary>
+  /// A datagram over the limit is rejected by the socket, which used to cost the whole record.
+  /// </summary>
+  [Test]
+  public void AnOversizeRecordIsTruncatedInsteadOfLost()
+  {
+    List<byte[]> sentBytes = [];
+    LogLog.ExecuteWithoutEmittingInternalMessages(
+      () => sentBytes = ExecuteAppend(new string('a', 1000), maxDatagramSize: 512));
+
+    Assert.That(sentBytes, Has.Count.EqualTo(1));
+    Assert.That(sentBytes[0], Has.Length.EqualTo(512));
+    Assert.That(Encoding.ASCII.GetString(sentBytes[0]), Does.EndWith("...[truncated]"));
+  }
+
   /// <summary>A control character other than CR or LF was dropped as well.</summary>
   [Test]
   public void OtherControlCharactersAreEscaped()
@@ -275,7 +290,8 @@ public void IdentityWithoutControlCharactersIsUnchangedAndNotReported()
 
   private static List<byte[]> ExecuteAppend(string message,
     SyslogNewLineHandling newLineHandling = default,
-    string? identity = null)
+    string? identity = null,
+    int? maxDatagramSize = null)
   {
     System.Net.IPAddress ipAddress = new([127, 0, 0, 1]);
     RemoteAppender appender = new()
@@ -285,6 +301,10 @@ private static List<byte[]> ExecuteAppend(string message,
       NewLineHandling = newLineHandling,
       Identity = identity is null ? null : new PatternLayout(identity)
     };
+    if (maxDatagramSize is int size)
+    {
+      appender.MaxDatagramSize = size;
+    }
     appender.ActivateOptions();
     LoggingEvent loggingEvent = new(new()
     {

src/log4net.Tests/Appender/SmtpPickupDirAppenderTest.cs

@@ -149,6 +149,29 @@ public void ContentThatCannotBeEncodedDoesNotDestroyTheBatch()
     Assert.That(sh.Message, Is.EqualTo(string.Empty), "Unexpected error message");
   }
 
+  /// <summary>
+  /// The appender used to close the file with a lone dot, so a logged line that is only a dot was
+  /// indistinguishable from it and ended the mail early for an agent honouring the terminator.
+  /// </summary>
+  [Test]
+  public void ALoggedDotIsTheOnlyDotLineInTheFile()
+  {
+    using AutoTempFolder pickupDir = new();
+    SilentErrorHandler sh = new();
+    SmtpPickupDirAppender appender = CreateSmtpPickupDirAppender(pickupDir.Path, sh);
+    ILogger log = CreateLogger(appender);
+
+    log.Log(GetType(), Level.Info, ".", null);
+    log.Log(GetType(), Level.Info, "the event after it", null);
+    DestroyLogger();
+
+    string[] lines = File.ReadAllLines(Directory.GetFiles(pickupDir.Path)[0]);
+
+    Assert.That(Array.FindAll(lines, line => line == "."), Has.Length.EqualTo(1));
+    Assert.That(lines, Does.Contain("the event after it"));
+    Assert.That(sh.Message, Is.EqualTo(string.Empty), "Unexpected error message");
+  }
+
   /// <summary>
   /// Tests if the sent message contained the date header.
   /// </summary>

src/log4net.Tests/Appender/UdpAppenderTest.cs

@@ -0,0 +1,121 @@
+#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.Linq;
+using System.Reflection;
+using System.Text;
+using log4net.Appender;
+using log4net.Tests.Appender.Internal;
+using NUnit.Framework;
+
+namespace log4net.Tests.Appender;
+
+/// <summary>
+/// Tests for <see cref="UdpAppender"/>
+/// </summary>
+[TestFixture]
+public sealed class UdpAppenderTest
+{
+  private const string Marker = "...[truncated]";
+
+  /// <summary>Calls the internal encoder the way <c>Append</c> does.</summary>
+  private static byte[] Encode(UdpAppender appender, string message)
+    => (byte[])typeof(UdpAppender)
+      .GetMethod("GetDatagramBytes", BindingFlags.Instance | BindingFlags.NonPublic)!
+      .Invoke(appender, [message])!;
+
+  /// <summary>The maximum UDP payload of an IPv4 packet, measured as the first size the socket rejects minus one.</summary>
+  [Test]
+  public void MaxDatagramSizeDefaultsTo65507()
+    => Assert.That(new UdpAppender().MaxDatagramSize, Is.EqualTo(65507));
+
+  /// <summary>Below 512 nothing useful fits, above 65507 an IPv4 socket rejects the datagram.</summary>
+  [TestCase(511)]
+  [TestCase(65508)]
+  public void MaxDatagramSizeRejectsValuesOutsideTheRange(int value)
+    => Assert.That(() => new UdpAppender().MaxDatagramSize = value,
+      Throws.TypeOf<ArgumentOutOfRangeException>());
+
+  /// <summary>An event that fits is sent exactly as the layout rendered it.</summary>
+  [Test]
+  public void AMessageThatFitsIsEncodedUnchanged()
+  {
+    UdpAppender appender = new() { Encoding = Encoding.UTF8 };
+
+    Assert.That(Encode(appender, "hello"), Is.EqualTo(Encoding.UTF8.GetBytes("hello")));
+  }
+
+  /// <summary>An event of exactly the limit still fits, so nothing is cut and nothing reported.</summary>
+  [Test]
+  public void AMessageOfExactlyTheLimitIsEncodedUnchanged()
+  {
+    UdpAppender appender = new() { Encoding = Encoding.ASCII, MaxDatagramSize = 512 };
+    RecordingErrorHandler errorHandler = new();
+    appender.ErrorHandler = errorHandler;
+
+    byte[] datagram = Encode(appender, new string('a', 512));
+
+    Assert.That(datagram, Is.EqualTo(Encoding.ASCII.GetBytes(new string('a', 512))));
+    Assert.That(errorHandler.Messages, Is.Empty);
+  }
+
+  /// <summary>
+  /// An oversize datagram used to be rejected by the socket, which cost the whole event.
+  /// </summary>
+  [Test]
+  public void AnOversizeMessageIsTruncatedInsteadOfLost()
+  {
+    UdpAppender appender = new() { Encoding = Encoding.ASCII, MaxDatagramSize = 512 };
+    RecordingErrorHandler errorHandler = new();
+    appender.ErrorHandler = errorHandler;
+
+    byte[] datagram = Encode(appender, new string('a', 1000));
+
+    Assert.That(datagram, Has.Length.EqualTo(512));
+    Assert.That(Encoding.ASCII.GetString(datagram), Does.EndWith(Marker));
+    Assert.That(errorHandler.Messages, Has.Count.EqualTo(1));
+  }
+
+  /// <summary>A cut inside a character would encode as the replacement character.</summary>
+  [Test]
+  public void TruncationDoesNotSplitAMultiByteCharacter()
+  {
+    UdpAppender appender = new() { Encoding = Encoding.UTF8, MaxDatagramSize = 513 };
+    appender.ErrorHandler = new RecordingErrorHandler();
+
+    string decoded = Encoding.UTF8.GetString(Encode(appender, new string('\u00e4', 1000)));
+
+    // 513 bytes hold the 14 byte marker and 249 of the two byte characters.
+    Assert.That(decoded, Is.EqualTo(new string('\u00e4', 249) + Marker));
+  }
+
+  /// <summary>The same for a cut between the two halves of a surrogate pair.</summary>
+  [Test]
+  public void TruncationDoesNotSplitASurrogatePair()
+  {
+    UdpAppender appender = new() { Encoding = Encoding.UTF8, MaxDatagramSize = 515 };
+    appender.ErrorHandler = new RecordingErrorHandler();
+
+    string decoded = Encoding.UTF8.GetString(Encode(appender, string.Concat(Enumerable.Repeat("\U0001f600", 1000))));
+
+    Assert.That(decoded, Does.EndWith(Marker));
+    Assert.That(decoded, !Contains.Substring("\ufffd").Using(StringComparison.Ordinal));
+  }
+}

src/log4net.Tests/Config/RedactTest.cs

@@ -0,0 +1,134 @@
+#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.Collections.Generic;
+using System.Xml;
+using log4net.Config;
+using log4net.Repository;
+using log4net.Util;
+using NUnit.Framework;
+
+namespace log4net.Tests.Config;
+
+/// <summary>
+/// Tests that the configurator keeps secrets out of its own messages.
+/// </summary>
+[TestFixture]
+[NonParallelizable]
+public sealed class RedactTest
+{
+  private const string Password = "Sup3rS3cretPw";
+  private const string ConnectionString = "Server=db.example.invalid;User Id=sa;Password=" + Password;
+
+  /// <summary>
+  /// Internal debugging is the documented first troubleshooting step, and it used to echo every
+  /// configured value, secrets included.
+  /// </summary>
+  [Test]
+  public void SecretValuesAreNotEchoedByInternalDebugging()
+  {
+    string reported = Configure($"""
+      <log4net>
+        <appender name="SmtpAppender" type="log4net.Appender.SmtpAppender">
+          <to value="dest@example.invalid" />
+          <password value="{Password}" />
+          <layout type="log4net.Layout.PatternLayout" />
+        </appender>
+        <root>
+          <appender-ref ref="SmtpAppender" />
+        </root>
+      </log4net>
+      """);
+
+    Assert.That(reported, Does.Contain("Setting Property [Password]"), "internal debugging was off");
+    Assert.That(reported, !Contains.Substring(Password).Using(StringComparison.Ordinal));
+    // A value that is not a secret is still echoed, so the output stays useful.
+    Assert.That(reported, Does.Contain("dest@example.invalid"));
+  }
+
+  /// <summary>A connection string keeps the server it names, so it can still be checked.</summary>
+  [Test]
+  public void AConnectionStringKeepsOnlyTheKeywordsThatNameTheServer()
+  {
+    string reported = Configure($"""
+      <log4net>
+        <appender name="AdoNetAppender" type="log4net.Appender.AdoNetAppender">
+          <connectionString value="{ConnectionString}" />
+          <layout type="log4net.Layout.PatternLayout" />
+        </appender>
+        <root>
+          <appender-ref ref="AdoNetAppender" />
+        </root>
+      </log4net>
+      """);
+
+    Assert.That(reported, !Contains.Substring(Password).Using(StringComparison.Ordinal));
+    Assert.That(reported, Does.Contain("db.example.invalid"));
+  }
+
+  /// <summary>
+  /// The provider type and the lookup key are the first things read when configuration fails, so
+  /// neither may be hidden by the connection string rule.
+  /// </summary>
+  [Test]
+  public void AParameterOfTheConnectionFamilyThatIsNoSecretSurvives()
+  {
+    string reported = Configure("""
+      <log4net>
+        <appender name="AdoNetAppender" type="log4net.Appender.AdoNetAppender">
+          <connectionType value="System.Data.SqlClient.SqlConnection, System.Data" />
+          <connectionStringName value="myDatabase" />
+          <layout type="log4net.Layout.PatternLayout" />
+        </appender>
+        <root>
+          <appender-ref ref="AdoNetAppender" />
+        </root>
+      </log4net>
+      """);
+
+    Assert.That(reported, Does.Contain("System.Data.SqlClient.SqlConnection, System.Data"));
+    Assert.That(reported, Does.Contain("myDatabase"));
+  }
+
+  /// <summary>Configures a repository from <paramref name="xml"/> and returns what log4net reported.</summary>
+  private static string Configure(string xml)
+  {
+    List<LogLog> messages = [];
+    bool internalDebugging = LogLog.InternalDebugging;
+    try
+    {
+      LogLog.InternalDebugging = true;
+      LogLog.ExecuteWithoutEmittingInternalMessages(() =>
+      {
+        using LogLog.LogReceivedAdapter _ = new(messages);
+        XmlDocument document = new();
+        document.LoadXml(xml);
+        ILoggerRepository repository = LogManager.CreateRepository(Guid.NewGuid().ToString());
+        XmlConfigurator.Configure(repository, (XmlElement)document.DocumentElement!);
+      });
+    }
+    finally
+    {
+      LogLog.InternalDebugging = internalDebugging;
+    }
+
+    return string.Join(Environment.NewLine, messages.ConvertAll(message => message.Message));
+  }
+}