From a44c32047f30849aae9d021f1a7a4129539ebaef Mon Sep 17 00:00:00 2001 From: Junhyeok Lee Date: Tue, 27 May 2025 01:37:23 +0900 Subject: [PATCH 1/2] Refactor script identification and enforce explicit names in global Scripts container This commit introduces a clearer distinction between user-defined script names and internal script identifiers, and enforces explicit naming for global scripts. - AbstractScript now provides 'getName()' for user-set names and 'getId()' for an internal Id. - ScriptsPlugin throws a 'ConfigurationException' if global scripts lack a non-blank name via 'getName()'. - ScriptManager and its users were updated to utilize 'script.getId()' for internal referencing. - Added unit tests for 'AbstractScript' and 'ScriptsPlugin'. - Updated 'org.apache.logging.log4j.core.script' package-info to version 2.25.0. - Added a changelog entry for issue #3176. --- .../log4j/core/config/ScriptsPluginTest.java | 79 +++++++++++++++++++ .../log4j/core/script/AbstractScriptTest.java | 70 ++++++++++++++++ .../core/appender/ScriptAppenderSelector.java | 4 +- .../rolling/action/ScriptCondition.java | 6 +- .../log4j/core/appender/routing/Routes.java | 2 +- .../appender/routing/RoutingAppender.java | 2 +- .../log4j/core/config/ScriptsPlugin.java | 16 +++- .../core/config/arbiters/ScriptArbiter.java | 6 +- .../log4j/core/filter/ScriptFilter.java | 14 ++-- .../core/layout/ScriptPatternSelector.java | 6 +- .../log4j/core/script/AbstractScript.java | 9 ++- .../logging/log4j/core/script/ScriptFile.java | 4 +- .../log4j/core/script/ScriptManager.java | 28 +++---- .../logging/log4j/core/script/ScriptRef.java | 6 +- .../log4j/core/script/package-info.java | 2 +- ...3176_validate_scripts_in_ScriptsPlugin.xml | 13 +++ 16 files changed, 225 insertions(+), 42 deletions(-) create mode 100644 log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ScriptsPluginTest.java create mode 100644 log4j-core-test/src/test/java/org/apache/logging/log4j/core/script/AbstractScriptTest.java create mode 100644 src/changelog/.2.x.x/3176_validate_scripts_in_ScriptsPlugin.xml diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ScriptsPluginTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ScriptsPluginTest.java new file mode 100644 index 00000000000..4bf53f52c9f --- /dev/null +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ScriptsPluginTest.java @@ -0,0 +1,79 @@ +/* + * 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.config; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.apache.logging.log4j.core.script.AbstractScript; +import org.junit.jupiter.api.Test; + +class ScriptsPluginTest { + + @Test + public void testCreateScriptsNullInput() { + assertNull(ScriptsPlugin.createScripts(null), "Should return null"); + } + + @Test + public void testCreateScriptsEmptyInput() { + AbstractScript[] emptyArray = new AbstractScript[0]; + assertSame(emptyArray, ScriptsPlugin.createScripts(emptyArray), "Should return empty array"); + } + + @Test + public void testCreateScriptsAllExplicitNames() { + AbstractScript script1 = new MockScript("script1", "JavaScript", "text"); + AbstractScript script2 = new MockScript("script2", "Groovy", "text"); + AbstractScript[] input = {script1, script2}; + AbstractScript[] result = ScriptsPlugin.createScripts(input); + assertEquals(2, result.length, "Should return 2 scripts"); + assertArrayEquals(input, result, "Should contain all valid scripts"); + } + + @Test + public void testCreateScriptsImplicitName() { + AbstractScript script = new MockScript(null, "JavaScript", "text"); + AbstractScript[] input = {script}; + assertThrows(ConfigurationException.class, () -> ScriptsPlugin.createScripts(input)); + } + + @Test + public void testCreateScriptsBlankName() { + AbstractScript script = new MockScript(" ", "JavaScript", "text"); + AbstractScript[] input = {script}; + assertThrows(ConfigurationException.class, () -> ScriptsPlugin.createScripts(input)); + } + + @Test + public void testCreateScriptsMixedExplicitAndImplicitNames() { + AbstractScript explicitScript = new MockScript("script", "Python", "text"); + AbstractScript implicitScript = new MockScript(null, "JavaScript", "text"); + AbstractScript[] input = {explicitScript, implicitScript}; + assertThrows(ConfigurationException.class, () -> ScriptsPlugin.createScripts(input)); + } + + private class MockScript extends AbstractScript { + + public MockScript(String name, String language, String scriptText) { + super(name, language, scriptText); + } + } +} diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/script/AbstractScriptTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/script/AbstractScriptTest.java new file mode 100644 index 00000000000..bdd5b081178 --- /dev/null +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/script/AbstractScriptTest.java @@ -0,0 +1,70 @@ +/* + * 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.script; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +import org.junit.jupiter.api.Test; + +class AbstractScriptTest { + + @Test + public void testConstructorAndGettersWithExplicitName() { + final String lang = "JavaScript"; + final String text = "text"; + final String name = "script"; + final AbstractScript script = new MockScript(name, lang, text); + + assertEquals(lang, script.getLanguage(), "Language should match"); + assertEquals(text, script.getScriptText(), "Script text should match"); + assertEquals(name, script.getName(), "Name should match the provided name"); + assertEquals(name, script.getId(), "Id should match the provided name"); + } + + @Test + public void testConstructorAndGettersWithImplicitName() { + final String lang = "JavaScript"; + final String text = "text"; + final AbstractScript script = new MockScript(null, lang, text); + + assertEquals(lang, script.getLanguage(), "Language should match"); + assertEquals(text, script.getScriptText(), "Script text should match"); + assertNull(script.getName(), "Name should be null"); + assertEquals(script.toString(), script.getId(), "Id should match its toString()"); + } + + @Test + public void testConstructorAndGettersWithBlankName() { + final String lang = "JavaScript"; + final String text = "text"; + final String name = " "; + final AbstractScript script = new MockScript(name, lang, text); + + assertEquals(lang, script.getLanguage(), "Language should match"); + assertEquals(text, script.getScriptText(), "Script text should match"); + assertEquals(name, script.getName(), "Name should be blank"); + assertEquals(script.toString(), script.getId(), "Id should match its toString()"); + } + + private class MockScript extends AbstractScript { + + public MockScript(String name, String language, String scriptText) { + super(name, language, scriptText); + } + } +} diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/ScriptAppenderSelector.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/ScriptAppenderSelector.java index 200fa47ec7c..efbd93e433d 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/ScriptAppenderSelector.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/ScriptAppenderSelector.java @@ -94,9 +94,9 @@ public Appender build() { "ScriptAppenderSelector '{}' executing {} '{}': {}", name, script.getLanguage(), - script.getName(), + script.getId(), script.getScriptText()); - final Object object = scriptManager.execute(script.getName(), bindings); + final Object object = scriptManager.execute(script.getId(), bindings); final String actualAppenderName = Objects.toString(object, null); LOGGER.debug("ScriptAppenderSelector '{}' selected '{}'", name, actualAppenderName); return appenderSet.createAppender(actualAppenderName, name); diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/ScriptCondition.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/ScriptCondition.java index 3519980c2ad..b87eff7ccc3 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/ScriptCondition.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/rolling/action/ScriptCondition.java @@ -73,7 +73,7 @@ public List selectFilesToDelete( bindings.put("configuration", configuration); bindings.put("substitutor", configuration.getStrSubstitutor()); bindings.put("statusLogger", LOGGER); - final Object object = configuration.getScriptManager().execute(script.getName(), bindings); + final Object object = configuration.getScriptManager().execute(script.getId(), bindings); return (List) object; } @@ -110,8 +110,8 @@ public static ScriptCondition createCondition( return null; } if (script instanceof ScriptRef) { - if (configuration.getScriptManager().getScript(script.getName()) == null) { - LOGGER.error("ScriptCondition: No script with name {} has been declared.", script.getName()); + if (configuration.getScriptManager().getScript(script.getId()) == null) { + LOGGER.error("ScriptCondition: No script with name {} has been declared.", script.getId()); return null; } } else { diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/routing/Routes.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/routing/Routes.java index cb5a452ef27..9ca37c15cb4 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/routing/Routes.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/routing/Routes.java @@ -180,7 +180,7 @@ public String getPattern(final LogEvent event, final ConcurrentMap validScripts = new ArrayList<>(scripts.length); + for (final AbstractScript script : scripts) { + if (Strings.isBlank(script.getName())) { + throw new ConfigurationException("A script defined in lacks an explicit 'name' attribute"); + } else { + validScripts.add(script); + } + } + return validScripts.toArray(new AbstractScript[0]); } } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/arbiters/ScriptArbiter.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/arbiters/ScriptArbiter.java index d4c96da171b..042b36a8146 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/arbiters/ScriptArbiter.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/arbiters/ScriptArbiter.java @@ -57,7 +57,7 @@ public boolean isCondition() { final SimpleBindings bindings = new SimpleBindings(); bindings.putAll(configuration.getProperties()); bindings.put("substitutor", configuration.getStrSubstitutor()); - final Object object = configuration.getScriptManager().execute(script.getName(), bindings); + final Object object = configuration.getScriptManager().execute(script.getId(), bindings); return Boolean.parseBoolean(object.toString()); } @@ -114,8 +114,8 @@ public ScriptArbiter build() { return null; } if (script instanceof ScriptRef) { - if (configuration.getScriptManager().getScript(script.getName()) == null) { - LOGGER.error("No script with name {} has been declared.", script.getName()); + if (configuration.getScriptManager().getScript(script.getId()) == null) { + LOGGER.error("No script with name {} has been declared.", script.getId()); return null; } } else { diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/ScriptFilter.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/ScriptFilter.java index 066dc7aa137..cec83951e39 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/ScriptFilter.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/filter/ScriptFilter.java @@ -69,7 +69,7 @@ public Result filter( bindings.put("throwable", null); bindings.putAll(configuration.getProperties()); bindings.put("substitutor", configuration.getStrSubstitutor()); - final Object object = configuration.getScriptManager().execute(script.getName(), bindings); + final Object object = configuration.getScriptManager().execute(script.getId(), bindings); return object == null || !Boolean.TRUE.equals(object) ? onMismatch : onMatch; } @@ -85,7 +85,7 @@ public Result filter( bindings.put("throwable", t); bindings.putAll(configuration.getProperties()); bindings.put("substitutor", configuration.getStrSubstitutor()); - final Object object = configuration.getScriptManager().execute(script.getName(), bindings); + final Object object = configuration.getScriptManager().execute(script.getId(), bindings); return object == null || !Boolean.TRUE.equals(object) ? onMismatch : onMatch; } @@ -101,7 +101,7 @@ public Result filter( bindings.put("throwable", t); bindings.putAll(configuration.getProperties()); bindings.put("substitutor", configuration.getStrSubstitutor()); - final Object object = configuration.getScriptManager().execute(script.getName(), bindings); + final Object object = configuration.getScriptManager().execute(script.getId(), bindings); return object == null || !Boolean.TRUE.equals(object) ? onMismatch : onMatch; } @@ -111,13 +111,13 @@ public Result filter(final LogEvent event) { bindings.put("logEvent", event); bindings.putAll(configuration.getProperties()); bindings.put("substitutor", configuration.getStrSubstitutor()); - final Object object = configuration.getScriptManager().execute(script.getName(), bindings); + final Object object = configuration.getScriptManager().execute(script.getId(), bindings); return object == null || !Boolean.TRUE.equals(object) ? onMismatch : onMatch; } @Override public String toString() { - return script.getName(); + return script.getId(); } /** @@ -146,8 +146,8 @@ public static ScriptFilter createFilter( return null; } if (script instanceof ScriptRef) { - if (configuration.getScriptManager().getScript(script.getName()) == null) { - logger.error("No script with name {} has been declared.", script.getName()); + if (configuration.getScriptManager().getScript(script.getId()) == null) { + logger.error("No script with name {} has been declared.", script.getId()); return null; } } else { diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/ScriptPatternSelector.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/ScriptPatternSelector.java index 92648e24d4b..f1e4d240f31 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/ScriptPatternSelector.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/layout/ScriptPatternSelector.java @@ -89,8 +89,8 @@ public ScriptPatternSelector build() { return null; } if (script instanceof ScriptRef) { - if (configuration.getScriptManager().getScript(script.getName()) == null) { - LOGGER.error("No script with name {} has been declared.", script.getName()); + if (configuration.getScriptManager().getScript(script.getId()) == null) { + LOGGER.error("No script with name {} has been declared.", script.getId()); return null; } } else { @@ -262,7 +262,7 @@ public PatternFormatter[] getFormatters(final LogEvent event) { bindings.putAll(configuration.getProperties()); bindings.put("substitutor", configuration.getStrSubstitutor()); bindings.put("logEvent", event); - final Object object = configuration.getScriptManager().execute(script.getName(), bindings); + final Object object = configuration.getScriptManager().execute(script.getId(), bindings); if (object == null) { return defaultFormatters; } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/script/AbstractScript.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/script/AbstractScript.java index d43ab2a452a..2ae427dcb64 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/script/AbstractScript.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/script/AbstractScript.java @@ -18,6 +18,7 @@ import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.status.StatusLogger; +import org.apache.logging.log4j.util.Strings; /** * Container for the language and body of a script. @@ -30,11 +31,13 @@ public abstract class AbstractScript { private final String language; private final String scriptText; private final String name; + private final String id; public AbstractScript(final String name, final String language, final String scriptText) { this.language = language; this.scriptText = scriptText; - this.name = name == null ? this.toString() : name; + this.name = name; + this.id = Strings.isBlank(name) ? this.toString() : name; } public String getLanguage() { @@ -48,4 +51,8 @@ public String getScriptText() { public String getName() { return this.name; } + + public String getId() { + return this.id; + } } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/script/ScriptFile.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/script/ScriptFile.java index 5ff0b27d34e..8f55ab48cba 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/script/ScriptFile.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/script/ScriptFile.java @@ -123,8 +123,8 @@ public static ScriptFile createScript( @Override public String toString() { final StringBuilder sb = new StringBuilder(); - if (!(getName().equals(filePath.toString()))) { - sb.append("name=").append(getName()).append(", "); + if (!(getId().equals(filePath.toString()))) { + sb.append("name=").append(getId()).append(", "); } sb.append("path=").append(filePath); if (getLanguage() != null) { diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/script/ScriptManager.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/script/ScriptManager.java index 5d87f925ae7..1409800bea0 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/script/ScriptManager.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/script/ScriptManager.java @@ -147,9 +147,9 @@ public boolean addScript(final AbstractScript script) { return false; } if (engine.getFactory().getParameter(KEY_THREADING) == null) { - scriptRunners.put(script.getName(), new ThreadLocalScriptRunner(script)); + scriptRunners.put(script.getId(), new ThreadLocalScriptRunner(script)); } else { - scriptRunners.put(script.getName(), new MainScriptRunner(engine, script)); + scriptRunners.put(script.getId(), new MainScriptRunner(engine, script)); } if (script instanceof ScriptFile) { @@ -162,7 +162,7 @@ public boolean addScript(final AbstractScript script) { } else { logger.error( "Unable to add script {}, {} has not been configured as an allowed language", - script.getName(), + script.getId(), script.getLanguage()); return false; } @@ -173,8 +173,8 @@ public Bindings createBindings(final AbstractScript script) { return getScriptRunner(script).createBindings(); } - public AbstractScript getScript(final String name) { - final ScriptRunner runner = scriptRunners.get(name); + public AbstractScript getScript(final String id) { + final ScriptRunner runner = scriptRunners.get(id); return runner != null ? runner.getScript() : null; } @@ -188,16 +188,16 @@ public void fileModified(final File file) { final ScriptEngine engine = runner.getScriptEngine(); final AbstractScript script = runner.getScript(); if (engine.getFactory().getParameter(KEY_THREADING) == null) { - scriptRunners.put(script.getName(), new ThreadLocalScriptRunner(script)); + scriptRunners.put(script.getId(), new ThreadLocalScriptRunner(script)); } else { - scriptRunners.put(script.getName(), new MainScriptRunner(engine, script)); + scriptRunners.put(script.getId(), new MainScriptRunner(engine, script)); } } - public Object execute(final String name, final Bindings bindings) { - final ScriptRunner scriptRunner = scriptRunners.get(name); + public Object execute(final String id, final Bindings bindings) { + final ScriptRunner scriptRunner = scriptRunners.get(id); if (scriptRunner == null) { - logger.warn("No script named {} could be found", name); + logger.warn("No script named {} could be found", id); return null; } return AccessController.doPrivileged((PrivilegedAction) () -> scriptRunner.execute(bindings)); @@ -224,7 +224,7 @@ public MainScriptRunner(final ScriptEngine scriptEngine, final AbstractScript sc this.scriptEngine = scriptEngine; CompiledScript compiled = null; if (scriptEngine instanceof Compilable) { - logger.debug("Script {} is compilable", script.getName()); + logger.debug("Script {} is compilable", script.getId()); compiled = AccessController.doPrivileged((PrivilegedAction) () -> { try { return ((Compilable) scriptEngine).compile(script.getScriptText()); @@ -252,14 +252,14 @@ public Object execute(final Bindings bindings) { try { return compiledScript.eval(bindings); } catch (final ScriptException ex) { - logger.error("Error running script " + script.getName(), ex); + logger.error("Error running script " + script.getId(), ex); return null; } } try { return scriptEngine.eval(script.getScriptText(), bindings); } catch (final ScriptException ex) { - logger.error("Error running script " + script.getName(), ex); + logger.error("Error running script " + script.getId(), ex); return null; } } @@ -302,6 +302,6 @@ public ScriptEngine getScriptEngine() { } private ScriptRunner getScriptRunner(final AbstractScript script) { - return scriptRunners.get(script.getName()); + return scriptRunners.get(script.getId()); } } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/script/ScriptRef.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/script/ScriptRef.java index a809f25e9d7..d9bb3df31ae 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/script/ScriptRef.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/script/ScriptRef.java @@ -38,13 +38,13 @@ public ScriptRef(final String name, final ScriptManager scriptManager) { @Override public String getLanguage() { - final AbstractScript script = this.scriptManager.getScript(getName()); + final AbstractScript script = this.scriptManager.getScript(getId()); return script != null ? script.getLanguage() : null; } @Override public String getScriptText() { - final AbstractScript script = this.scriptManager.getScript(getName()); + final AbstractScript script = this.scriptManager.getScript(getId()); return script != null ? script.getScriptText() : null; } @@ -62,6 +62,6 @@ public static ScriptRef createReference( @Override public String toString() { - return "ref=" + getName(); + return "ref=" + getId(); } } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/script/package-info.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/script/package-info.java index d973435b64c..bb9872749ea 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/script/package-info.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/script/package-info.java @@ -18,7 +18,7 @@ * Log4j 2 Script support. */ @Export -@Version("2.20.2") +@Version("2.25.0") package org.apache.logging.log4j.core.script; import org.osgi.annotation.bundle.Export; diff --git a/src/changelog/.2.x.x/3176_validate_scripts_in_ScriptsPlugin.xml b/src/changelog/.2.x.x/3176_validate_scripts_in_ScriptsPlugin.xml new file mode 100644 index 00000000000..0246396937d --- /dev/null +++ b/src/changelog/.2.x.x/3176_validate_scripts_in_ScriptsPlugin.xml @@ -0,0 +1,13 @@ + + + + + Ensured scripts in global `Scripts container` have explicit names by throwing a `ConfigurationException` for unnamed ones. + Refined script identification with `AbstractScript.getId()` and clarified `AbstractScript.getName()`. + + From 8ac8aa299a44f13489c6c045d7d249cf6f917831 Mon Sep 17 00:00:00 2001 From: Junhyeok Lee Date: Mon, 16 Jun 2025 21:40:02 +0900 Subject: [PATCH 2/2] Refine script handling and improve runtime safety - Improve null-safety in ScriptsPlugin by throwing an exception for null input. - Replace unsafe toString() call in AbstractScript constructor with a safer internal ID generation. - Adjust test cases to align with code changes and project conventions. - Update OSGi @Version annotation to 2.26.0 for the next release cycle. --- .../log4j/core/config/ScriptsPluginTest.java | 15 +++++++-------- .../log4j/core/script/AbstractScriptTest.java | 11 ++++++----- .../logging/log4j/core/config/ScriptsPlugin.java | 6 +++++- .../logging/log4j/core/script/AbstractScript.java | 3 ++- .../logging/log4j/core/script/package-info.java | 2 +- 5 files changed, 21 insertions(+), 16 deletions(-) diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ScriptsPluginTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ScriptsPluginTest.java index 4bf53f52c9f..0ea1acd6ef7 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ScriptsPluginTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ScriptsPluginTest.java @@ -18,7 +18,6 @@ import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -28,18 +27,18 @@ class ScriptsPluginTest { @Test - public void testCreateScriptsNullInput() { - assertNull(ScriptsPlugin.createScripts(null), "Should return null"); + void testCreateScriptsNullInput() { + assertThrows(NullPointerException.class, () -> ScriptsPlugin.createScripts(null)); } @Test - public void testCreateScriptsEmptyInput() { + void testCreateScriptsEmptyInput() { AbstractScript[] emptyArray = new AbstractScript[0]; assertSame(emptyArray, ScriptsPlugin.createScripts(emptyArray), "Should return empty array"); } @Test - public void testCreateScriptsAllExplicitNames() { + void testCreateScriptsAllExplicitNames() { AbstractScript script1 = new MockScript("script1", "JavaScript", "text"); AbstractScript script2 = new MockScript("script2", "Groovy", "text"); AbstractScript[] input = {script1, script2}; @@ -49,21 +48,21 @@ public void testCreateScriptsAllExplicitNames() { } @Test - public void testCreateScriptsImplicitName() { + void testCreateScriptsImplicitName() { AbstractScript script = new MockScript(null, "JavaScript", "text"); AbstractScript[] input = {script}; assertThrows(ConfigurationException.class, () -> ScriptsPlugin.createScripts(input)); } @Test - public void testCreateScriptsBlankName() { + void testCreateScriptsBlankName() { AbstractScript script = new MockScript(" ", "JavaScript", "text"); AbstractScript[] input = {script}; assertThrows(ConfigurationException.class, () -> ScriptsPlugin.createScripts(input)); } @Test - public void testCreateScriptsMixedExplicitAndImplicitNames() { + void testCreateScriptsMixedExplicitAndImplicitNames() { AbstractScript explicitScript = new MockScript("script", "Python", "text"); AbstractScript implicitScript = new MockScript(null, "JavaScript", "text"); AbstractScript[] input = {explicitScript, implicitScript}; diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/script/AbstractScriptTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/script/AbstractScriptTest.java index bdd5b081178..d4cbef56064 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/script/AbstractScriptTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/script/AbstractScriptTest.java @@ -17,6 +17,7 @@ package org.apache.logging.log4j.core.script; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import org.junit.jupiter.api.Test; @@ -24,7 +25,7 @@ class AbstractScriptTest { @Test - public void testConstructorAndGettersWithExplicitName() { + void testConstructorAndGettersWithExplicitName() { final String lang = "JavaScript"; final String text = "text"; final String name = "script"; @@ -37,7 +38,7 @@ public void testConstructorAndGettersWithExplicitName() { } @Test - public void testConstructorAndGettersWithImplicitName() { + void testConstructorAndGettersWithImplicitName() { final String lang = "JavaScript"; final String text = "text"; final AbstractScript script = new MockScript(null, lang, text); @@ -45,11 +46,11 @@ public void testConstructorAndGettersWithImplicitName() { assertEquals(lang, script.getLanguage(), "Language should match"); assertEquals(text, script.getScriptText(), "Script text should match"); assertNull(script.getName(), "Name should be null"); - assertEquals(script.toString(), script.getId(), "Id should match its toString()"); + assertNotNull(script.getId(), "Id should not be null"); } @Test - public void testConstructorAndGettersWithBlankName() { + void testConstructorAndGettersWithBlankName() { final String lang = "JavaScript"; final String text = "text"; final String name = " "; @@ -58,7 +59,7 @@ public void testConstructorAndGettersWithBlankName() { assertEquals(lang, script.getLanguage(), "Language should match"); assertEquals(text, script.getScriptText(), "Script text should match"); assertEquals(name, script.getName(), "Name should be blank"); - assertEquals(script.toString(), script.getId(), "Id should match its toString()"); + assertNotNull(script.getId(), "Id should not be null"); } private class MockScript extends AbstractScript { diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ScriptsPlugin.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ScriptsPlugin.java index fcf0f48979d..4ad19b6fb0a 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ScriptsPlugin.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/config/ScriptsPlugin.java @@ -18,16 +18,19 @@ import java.util.ArrayList; import java.util.List; +import java.util.Objects; import org.apache.logging.log4j.core.Core; import org.apache.logging.log4j.core.config.plugins.Plugin; import org.apache.logging.log4j.core.config.plugins.PluginElement; import org.apache.logging.log4j.core.config.plugins.PluginFactory; import org.apache.logging.log4j.core.script.AbstractScript; import org.apache.logging.log4j.util.Strings; +import org.jspecify.annotations.NullMarked; /** * A container of Scripts. */ +@NullMarked @Plugin(name = "Scripts", category = Core.CATEGORY_NAME) public final class ScriptsPlugin { @@ -40,7 +43,8 @@ private ScriptsPlugin() {} */ @PluginFactory public static AbstractScript[] createScripts(@PluginElement("Scripts") final AbstractScript[] scripts) { - if (scripts == null || scripts.length == 0) { + Objects.requireNonNull(scripts, "Scripts array cannot be null"); + if (scripts.length == 0) { return scripts; } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/script/AbstractScript.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/script/AbstractScript.java index 2ae427dcb64..1242d59ac3d 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/script/AbstractScript.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/script/AbstractScript.java @@ -16,6 +16,7 @@ */ package org.apache.logging.log4j.core.script; +import java.util.Objects; import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.status.StatusLogger; import org.apache.logging.log4j.util.Strings; @@ -37,7 +38,7 @@ public AbstractScript(final String name, final String language, final String scr this.language = language; this.scriptText = scriptText; this.name = name; - this.id = Strings.isBlank(name) ? this.toString() : name; + this.id = Strings.isBlank(name) ? Integer.toHexString(Objects.hashCode(this)) : name; } public String getLanguage() { diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/script/package-info.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/script/package-info.java index bb9872749ea..1374fe6673b 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/script/package-info.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/script/package-info.java @@ -18,7 +18,7 @@ * Log4j 2 Script support. */ @Export -@Version("2.25.0") +@Version("2.26.0") package org.apache.logging.log4j.core.script; import org.osgi.annotation.bundle.Export;