From c48a0d2e8de177e1775a730f1ad7334a048e051e Mon Sep 17 00:00:00 2001 From: Jeff Thomas Date: Sun, 3 Nov 2024 15:51:24 +0100 Subject: [PATCH 1/5] Add improved validation in StringMatchFilter for null/empty text --- .../log4j/core/filter/StringMatchFilter.java | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/StringMatchFilter.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/StringMatchFilter.java index 96a91c18304..31de4258661 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/StringMatchFilter.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/StringMatchFilter.java @@ -25,6 +25,8 @@ import org.apache.logging.log4j.core.config.plugins.Plugin; import org.apache.logging.log4j.core.config.plugins.PluginBuilderAttribute; import org.apache.logging.log4j.core.config.plugins.PluginBuilderFactory; +import org.apache.logging.log4j.core.config.plugins.validation.constraints.Required; +import org.apache.logging.log4j.core.util.Assert; import org.apache.logging.log4j.message.Message; import org.apache.logging.log4j.util.PerformanceSensitive; @@ -41,7 +43,7 @@ public final class StringMatchFilter extends AbstractFilter { private StringMatchFilter(final String text, final Result onMatch, final Result onMismatch) { super(onMatch, onMismatch); - this.text = text; + this.text = Assert.requireNonEmpty(text, "text"); } @Override @@ -235,8 +237,10 @@ public static StringMatchFilter.Builder newBuilder() { public static class Builder extends AbstractFilterBuilder implements org.apache.logging.log4j.core.util.Builder { + @PluginBuilderAttribute - private String text = ""; + @Required(message = "No text provided for StringMatchFilter") + private String text; /** * Sets the text to search in event messages. @@ -244,12 +248,15 @@ public static class Builder extends AbstractFilterBuilder Date: Sun, 3 Nov 2024 15:53:23 +0100 Subject: [PATCH 2/5] Changelog for #3153 --- .../.2.x.x/3153_fix_StringMatchFilter_guardNPE.xml | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 src/changelog/.2.x.x/3153_fix_StringMatchFilter_guardNPE.xml diff --git a/src/changelog/.2.x.x/3153_fix_StringMatchFilter_guardNPE.xml b/src/changelog/.2.x.x/3153_fix_StringMatchFilter_guardNPE.xml new file mode 100644 index 00000000000..d84642c7183 --- /dev/null +++ b/src/changelog/.2.x.x/3153_fix_StringMatchFilter_guardNPE.xml @@ -0,0 +1,8 @@ + + + + Add improved validation to StringMatchFilter for null/empty text. GitHub issue #3153. + From b36f5335ea30faee05c8442727d8c7fb9453642c Mon Sep 17 00:00:00 2001 From: Jeff Thomas Date: Sun, 3 Nov 2024 20:49:04 +0100 Subject: [PATCH 3/5] Added StringMatchFilterTest with test XMLs --- .../core/filter/StringMatchFilterTest.java | 70 +++++++++++++++++++ .../log4j2-stringmatchfilter-3153-nok.xml | 20 ++++++ .../log4j2-stringmatchfilter-3153-ok.xml | 20 ++++++ 3 files changed, 110 insertions(+) create mode 100644 log4j-core-test/src/test/java/org/apache/logging/log4j/core/filter/StringMatchFilterTest.java create mode 100644 log4j-core-test/src/test/resources/log4j2-stringmatchfilter-3153-nok.xml create mode 100644 log4j-core-test/src/test/resources/log4j2-stringmatchfilter-3153-ok.xml diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/filter/StringMatchFilterTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/filter/StringMatchFilterTest.java new file mode 100644 index 00000000000..31a4089ffe1 --- /dev/null +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/filter/StringMatchFilterTest.java @@ -0,0 +1,70 @@ +package org.apache.logging.log4j.core.filter; + +import org.apache.logging.log4j.core.Filter; +import org.apache.logging.log4j.core.config.Configuration; +import org.apache.logging.log4j.core.test.junit.LoggerContextSource; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; + +/** + * Unit test for {@link StringMatchFilter}. + */ +public class StringMatchFilterTest { + + /** + * Test that if no match-string is set on the builder, the '{@link StringMatchFilter.Builder#build()}' returns + * {@code null}. + */ + @Test + public void testFilterBuilderFailsWithNullText() { + Assertions.assertNull(StringMatchFilter.newBuilder().build()); + } + + /** + * Test that if a {@code null} string is set as a match-pattern, an {@code IllegalArgumentExeption} is thrown. + */ + @Test + void testFilterBuilderFailsWithExceptionOnNullText() { + Assertions.assertThrows(IllegalArgumentException.class, () -> StringMatchFilter.newBuilder().setMatchString(null)); + } + + /** + * Test that if an empty ({@code ""}) string is set as a match-pattern, an {@code IllegalArgumentException} is thrown. + */ + @Test + void testFilterBuilderFailsWithExceptionOnEmptyText() { + Assertions.assertThrows(IllegalArgumentException.class, () -> StringMatchFilter.newBuilder().setMatchString("")); + } + + /** + * Test that if a {@link StringMatchFilter} is specified with a 'text' attribute it is correctly instantiated. + * + * @param configuration the configuration + */ + @Test + @LoggerContextSource("log4j2-stringmatchfilter-3153-ok.xml") + void testConfigurationWithTextPOS(final Configuration configuration) { + final Filter filter = configuration.getFilter(); + assertNotNull(filter, "The filter should not be null."); + assertInstanceOf(StringMatchFilter.class, filter, "Expected a StringMatchFilter, but got: " + filter.getClass()); + assertEquals("FooBar", filter.toString()); + } + + /** + * Test that if a {@link StringMatchFilter} is specified without a 'text' attribute it is not instantiated. + * + * @param configuration the configuration + */ + @Test + @LoggerContextSource("log4j2-stringmatchfilter-3153-nok.xml") + void testConfigurationWithTextNEG(final Configuration configuration) { + final Filter filter = configuration.getFilter(); + assertNull(filter, "The filter should be null."); + } + +} diff --git a/log4j-core-test/src/test/resources/log4j2-stringmatchfilter-3153-nok.xml b/log4j-core-test/src/test/resources/log4j2-stringmatchfilter-3153-nok.xml new file mode 100644 index 00000000000..8f3755a2201 --- /dev/null +++ b/log4j-core-test/src/test/resources/log4j2-stringmatchfilter-3153-nok.xml @@ -0,0 +1,20 @@ + + + + + diff --git a/log4j-core-test/src/test/resources/log4j2-stringmatchfilter-3153-ok.xml b/log4j-core-test/src/test/resources/log4j2-stringmatchfilter-3153-ok.xml new file mode 100644 index 00000000000..1ac8c144af7 --- /dev/null +++ b/log4j-core-test/src/test/resources/log4j2-stringmatchfilter-3153-ok.xml @@ -0,0 +1,20 @@ + + + + + From bfcd23236acf1e27232956c0d610a636a8d59e75 Mon Sep 17 00:00:00 2001 From: Jeff Thomas Date: Sun, 3 Nov 2024 20:58:36 +0100 Subject: [PATCH 4/5] Added license to StringMatchFilterTest.java --- .../log4j/core/filter/StringMatchFilterTest.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/filter/StringMatchFilterTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/filter/StringMatchFilterTest.java index 31a4089ffe1..ea35a369803 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/filter/StringMatchFilterTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/filter/StringMatchFilterTest.java @@ -1,3 +1,19 @@ +/* + * 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. + */ package org.apache.logging.log4j.core.filter; import org.apache.logging.log4j.core.Filter; From 66260960caa6067c85e879ae7795f1b7a5e68f14 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Sun, 3 Nov 2024 20:59:59 +0100 Subject: [PATCH 5/5] Fix formatting --- .../core/filter/StringMatchFilterTest.java | 106 +++++++++--------- 1 file changed, 54 insertions(+), 52 deletions(-) diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/filter/StringMatchFilterTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/filter/StringMatchFilterTest.java index ea35a369803..0972279b07a 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/filter/StringMatchFilterTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/filter/StringMatchFilterTest.java @@ -16,71 +16,73 @@ */ package org.apache.logging.log4j.core.filter; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; + import org.apache.logging.log4j.core.Filter; import org.apache.logging.log4j.core.config.Configuration; import org.apache.logging.log4j.core.test.junit.LoggerContextSource; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertInstanceOf; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertNull; - /** * Unit test for {@link StringMatchFilter}. */ public class StringMatchFilterTest { - /** - * Test that if no match-string is set on the builder, the '{@link StringMatchFilter.Builder#build()}' returns - * {@code null}. - */ - @Test - public void testFilterBuilderFailsWithNullText() { - Assertions.assertNull(StringMatchFilter.newBuilder().build()); - } - - /** - * Test that if a {@code null} string is set as a match-pattern, an {@code IllegalArgumentExeption} is thrown. - */ - @Test - void testFilterBuilderFailsWithExceptionOnNullText() { - Assertions.assertThrows(IllegalArgumentException.class, () -> StringMatchFilter.newBuilder().setMatchString(null)); - } + /** + * Test that if no match-string is set on the builder, the '{@link StringMatchFilter.Builder#build()}' returns + * {@code null}. + */ + @Test + public void testFilterBuilderFailsWithNullText() { + Assertions.assertNull(StringMatchFilter.newBuilder().build()); + } - /** - * Test that if an empty ({@code ""}) string is set as a match-pattern, an {@code IllegalArgumentException} is thrown. - */ - @Test - void testFilterBuilderFailsWithExceptionOnEmptyText() { - Assertions.assertThrows(IllegalArgumentException.class, () -> StringMatchFilter.newBuilder().setMatchString("")); - } + /** + * Test that if a {@code null} string is set as a match-pattern, an {@code IllegalArgumentExeption} is thrown. + */ + @Test + void testFilterBuilderFailsWithExceptionOnNullText() { + Assertions.assertThrows(IllegalArgumentException.class, () -> StringMatchFilter.newBuilder() + .setMatchString(null)); + } - /** - * Test that if a {@link StringMatchFilter} is specified with a 'text' attribute it is correctly instantiated. - * - * @param configuration the configuration - */ - @Test - @LoggerContextSource("log4j2-stringmatchfilter-3153-ok.xml") - void testConfigurationWithTextPOS(final Configuration configuration) { - final Filter filter = configuration.getFilter(); - assertNotNull(filter, "The filter should not be null."); - assertInstanceOf(StringMatchFilter.class, filter, "Expected a StringMatchFilter, but got: " + filter.getClass()); - assertEquals("FooBar", filter.toString()); - } + /** + * Test that if an empty ({@code ""}) string is set as a match-pattern, an {@code IllegalArgumentException} is thrown. + */ + @Test + void testFilterBuilderFailsWithExceptionOnEmptyText() { + Assertions.assertThrows(IllegalArgumentException.class, () -> StringMatchFilter.newBuilder() + .setMatchString("")); + } - /** - * Test that if a {@link StringMatchFilter} is specified without a 'text' attribute it is not instantiated. - * - * @param configuration the configuration - */ - @Test - @LoggerContextSource("log4j2-stringmatchfilter-3153-nok.xml") - void testConfigurationWithTextNEG(final Configuration configuration) { - final Filter filter = configuration.getFilter(); - assertNull(filter, "The filter should be null."); - } + /** + * Test that if a {@link StringMatchFilter} is specified with a 'text' attribute it is correctly instantiated. + * + * @param configuration the configuration + */ + @Test + @LoggerContextSource("log4j2-stringmatchfilter-3153-ok.xml") + void testConfigurationWithTextPOS(final Configuration configuration) { + final Filter filter = configuration.getFilter(); + assertNotNull(filter, "The filter should not be null."); + assertInstanceOf( + StringMatchFilter.class, filter, "Expected a StringMatchFilter, but got: " + filter.getClass()); + assertEquals("FooBar", filter.toString()); + } + /** + * Test that if a {@link StringMatchFilter} is specified without a 'text' attribute it is not instantiated. + * + * @param configuration the configuration + */ + @Test + @LoggerContextSource("log4j2-stringmatchfilter-3153-nok.xml") + void testConfigurationWithTextNEG(final Configuration configuration) { + final Filter filter = configuration.getFilter(); + assertNull(filter, "The filter should be null."); + } }