This is an automated email from the ASF dual-hosted git repository.
lukaszlenart pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/struts.git
The following commit(s) were added to refs/heads/main by this push:
new 54a1ca3ff WW-5752 Normalize request-derived values in log messages
(#1982)
54a1ca3ff is described below
commit 54a1ca3ff61c40f0c9e26dacf81c0823ec4809fa
Author: ⳕⲛτⲉⲅⲥⲉⳏτⲟⲅ 🕵🏻 <[email protected]>
AuthorDate: Fri Oct 2 11:23:42 2026 +0530
WW-5752 Normalize request-derived values in log messages (#1982)
* WW-5752 Normalize request-derived values in log messages
Request-derived strings were logged unchanged, so a decoded line break in a
URL path, parameter name, locale or JSON key produced forged log lines. Apply
normalizeSpace, as TokenHelper, the multipart classes and ParametersInterceptor
already do.
* WW-5752 Normalize the token name in the missing-token debug message
Review follow-up: validToken() still logged the request-supplied token name
unchanged.
---------
Co-authored-by: Gouri Sankar A <[email protected]>
---
.../struts2/action/DefaultCspReportAction.java | 4 +-
.../dispatcher/mapper/DefaultActionMapper.java | 8 +--
.../dispatcher/mapper/RestfulActionMapper.java | 4 +-
.../parameter/ParametersInterceptor.java | 20 +++----
.../struts2/locale/DefaultLocaleProvider.java | 5 +-
.../java/org/apache/struts2/util/TokenHelper.java | 4 +-
.../apache/struts2/action/CspReportActionTest.java | 12 +++++
.../dispatcher/mapper/DefaultActionMapperTest.java | 17 ++++++
.../dispatcher/mapper/RestfulActionMapperTest.java | 12 +++++
.../parameter/ParametersInterceptorTest.java | 22 ++++++++
.../struts2/locale/DefaultLocaleProviderTest.java | 16 ++++++
.../java/org/apache/struts2/test/LogCapture.java | 62 ++++++++++++++++++++++
.../org/apache/struts2/util/TokenHelperTest.java | 39 ++++++++++++++
.../org/apache/struts2/json/JSONInterceptor.java | 20 +++----
.../apache/struts2/json/JSONInterceptorTest.java | 49 +++++++++++++++++
15 files changed, 266 insertions(+), 28 deletions(-)
diff --git
a/core/src/main/java/org/apache/struts2/action/DefaultCspReportAction.java
b/core/src/main/java/org/apache/struts2/action/DefaultCspReportAction.java
index 3bae9f0ee..9b71c15ad 100644
--- a/core/src/main/java/org/apache/struts2/action/DefaultCspReportAction.java
+++ b/core/src/main/java/org/apache/struts2/action/DefaultCspReportAction.java
@@ -21,6 +21,8 @@ package org.apache.struts2.action;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
+import static org.apache.commons.lang3.StringUtils.normalizeSpace;
+
/**
* The default implementation of {@link CspReportAction} that simply logs the
JSON object
* that contains the details of the CSP violation.
@@ -33,6 +35,6 @@ public class DefaultCspReportAction extends CspReportAction {
@Override
void processReport(String jsonCspReport) {
- LOG.error(jsonCspReport);
+ LOG.error(normalizeSpace(jsonCspReport));
}
}
diff --git
a/core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java
b/core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java
index 6e0f57c8b..60d31be68 100644
---
a/core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java
+++
b/core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java
@@ -42,6 +42,8 @@ import java.util.Map;
import java.util.Set;
import java.util.regex.Pattern;
+import static org.apache.commons.lang3.StringUtils.normalizeSpace;
+
/**
* <!-- START SNIPPET: javadoc -->
* <p>
@@ -410,7 +412,7 @@ public class DefaultActionMapper implements ActionMapper {
} else {
LOG.warn(
"{} did not match allowed namespace names {} - default
namespace {} will be used!",
- rawNamespace, allowedNamespaceNames, defaultNamespaceName
+ normalizeSpace(rawNamespace), allowedNamespaceNames,
defaultNamespaceName
);
return defaultNamespaceName;
}
@@ -426,7 +428,7 @@ public class DefaultActionMapper implements ActionMapper {
if (allowedActionNames.matcher(rawActionName).matches()) {
return rawActionName;
} else {
- LOG.warn("{} did not match allowed action names {} - default
action {} will be used!", rawActionName, allowedActionNames, defaultActionName);
+ LOG.warn("{} did not match allowed action names {} - default
action {} will be used!", normalizeSpace(rawActionName), allowedActionNames,
defaultActionName);
return defaultActionName;
}
}
@@ -441,7 +443,7 @@ public class DefaultActionMapper implements ActionMapper {
if (allowedMethodNames.matcher(rawMethodName).matches()) {
return rawMethodName;
} else {
- LOG.warn("{} did not match allowed method names {} - default
method {} will be used!", rawMethodName, allowedMethodNames, defaultMethodName);
+ LOG.warn("{} did not match allowed method names {} - default
method {} will be used!", normalizeSpace(rawMethodName), allowedMethodNames,
defaultMethodName);
return defaultMethodName;
}
}
diff --git
a/core/src/main/java/org/apache/struts2/dispatcher/mapper/RestfulActionMapper.java
b/core/src/main/java/org/apache/struts2/dispatcher/mapper/RestfulActionMapper.java
index a9a929592..f38ecfecf 100644
---
a/core/src/main/java/org/apache/struts2/dispatcher/mapper/RestfulActionMapper.java
+++
b/core/src/main/java/org/apache/struts2/dispatcher/mapper/RestfulActionMapper.java
@@ -32,6 +32,8 @@ import java.util.Map;
import java.util.StringTokenizer;
import java.util.regex.Pattern;
+import static org.apache.commons.lang3.StringUtils.normalizeSpace;
+
/**
* Simple Restfull Action Mapper to support REST application
* See docs for more information
@@ -133,7 +135,7 @@ public class RestfulActionMapper implements ActionMapper {
if (allowedActionNames.matcher(rawActionName).matches()) {
return rawActionName;
} else {
- LOG.warn("{} did not match allowed action names {} - default
action {} will be used!", rawActionName, allowedActionNames, defaultActionName);
+ LOG.warn("{} did not match allowed action names {} - default
action {} will be used!", normalizeSpace(rawActionName), allowedActionNames,
defaultActionName);
return defaultActionName;
}
}
diff --git
a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java
b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java
index 22d575eab..254e471cc 100644
---
a/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java
+++
b/core/src/main/java/org/apache/struts2/interceptor/parameter/ParametersInterceptor.java
@@ -634,9 +634,9 @@ public class ParametersInterceptor extends
MethodFilterInterceptor {
LOG.warn("Parameter [{}] is too long, allowed length is [{}].
Use Interceptor Parameter Overriding " +
"to override the limit, see more at\n" +
"https://struts.apache.org/core-developers/interceptors.html#interceptor-parameter-overriding";,
- name, paramNameMaxLength);
+ normalizeSpace(name), paramNameMaxLength);
} else {
- LOG.warn("Parameter [{}] is too long, allowed length is [{}]",
name, paramNameMaxLength);
+ LOG.warn("Parameter [{}] is too long, allowed length is [{}]",
normalizeSpace(name), paramNameMaxLength);
}
}
return matchLength;
@@ -648,9 +648,9 @@ public class ParametersInterceptor extends
MethodFilterInterceptor {
if (devMode) {
LOG.warn("Parameter [{}] didn't match accepted pattern [{}]!
See Accepted / Excluded patterns at\n" +
"https://struts.apache.org/security/#accepted--excluded-patterns";,
- paramName, result.getAcceptedPattern());
+ normalizeSpace(paramName),
result.getAcceptedPattern());
} else {
- LOG.debug("Parameter [{}] didn't match accepted pattern
[{}]!", paramName, result.getAcceptedPattern());
+ LOG.debug("Parameter [{}] didn't match accepted pattern
[{}]!", normalizeSpace(paramName), result.getAcceptedPattern());
}
return false;
}
@@ -663,9 +663,9 @@ public class ParametersInterceptor extends
MethodFilterInterceptor {
if (devMode) {
LOG.warn("Parameter [{}] matches excluded pattern [{}]! See
Accepted / Excluded patterns at\n" +
"https://struts.apache.org/security/#accepted--excluded-patterns";,
- paramName, result.getExcludedPattern());
+ normalizeSpace(paramName),
result.getExcludedPattern());
} else {
- LOG.debug("Parameter [{}] matches excluded pattern [{}]!",
paramName, result.getExcludedPattern());
+ LOG.debug("Parameter [{}] matches excluded pattern [{}]!",
normalizeSpace(paramName), result.getExcludedPattern());
}
return true;
}
@@ -682,9 +682,9 @@ public class ParametersInterceptor extends
MethodFilterInterceptor {
if (devMode) {
LOG.warn("Parameter value [{}] matches excluded pattern
[{}]! See Accepting/Excluding parameter values at\n" +
"https://struts.apache.org/core-developers/parameters-interceptor#excluding-parameter-values";,
- value, excludedValuePatterns);
+ normalizeSpace(value), excludedValuePatterns);
} else {
- LOG.debug("Parameter value [{}] matches excluded pattern
[{}]", value, excludedValuePattern);
+ LOG.debug("Parameter value [{}] matches excluded pattern
[{}]", normalizeSpace(value), excludedValuePattern);
}
return true;
}
@@ -705,9 +705,9 @@ public class ParametersInterceptor extends
MethodFilterInterceptor {
if (devMode) {
LOG.warn("Parameter value [{}] didn't match accepted pattern [{}]!
See Accepting/Excluding parameter values at\n" +
"https://struts.apache.org/core-developers/parameters-interceptor#excluding-parameter-values";,
- value, acceptedValuePatterns);
+ normalizeSpace(value), acceptedValuePatterns);
} else {
- LOG.debug("Parameter value [{}] was not accepted!", value);
+ LOG.debug("Parameter value [{}] was not accepted!",
normalizeSpace(value));
}
return false;
}
diff --git
a/core/src/main/java/org/apache/struts2/locale/DefaultLocaleProvider.java
b/core/src/main/java/org/apache/struts2/locale/DefaultLocaleProvider.java
index 2cf266fea..3301a8ca6 100644
--- a/core/src/main/java/org/apache/struts2/locale/DefaultLocaleProvider.java
+++ b/core/src/main/java/org/apache/struts2/locale/DefaultLocaleProvider.java
@@ -22,11 +22,12 @@ import org.apache.commons.lang3.LocaleUtils;
import org.apache.commons.lang3.StringUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
-import org.apache.logging.log4j.message.ParameterizedMessage;
import org.apache.struts2.ActionContext;
import java.util.Locale;
+import static org.apache.commons.lang3.StringUtils.normalizeSpace;
+
/**
* Default implementation of {@link LocaleProvider}
*/
@@ -62,7 +63,7 @@ public class DefaultLocaleProvider implements LocaleProvider {
try {
locale = LocaleUtils.toLocale(StringUtils.trimToNull(localeStr));
} catch (IllegalArgumentException e) {
- LOG.warn(new ParameterizedMessage("Cannot convert [{}] to proper
locale", localeStr), e);
+ LOG.warn("Cannot convert [{}] to proper locale",
normalizeSpace(localeStr));
}
return locale;
}
diff --git a/core/src/main/java/org/apache/struts2/util/TokenHelper.java
b/core/src/main/java/org/apache/struts2/util/TokenHelper.java
index 42ba4cc60..8bc349e91 100644
--- a/core/src/main/java/org/apache/struts2/util/TokenHelper.java
+++ b/core/src/main/java/org/apache/struts2/util/TokenHelper.java
@@ -133,7 +133,7 @@ public class TokenHelper {
Parameter parameter = params.get(tokenName);
if (!parameter.isDefined()) {
- LOG.warn("Could not find token mapped to token name: {}",
tokenName);
+ LOG.warn("Could not find token mapped to token name: {}",
normalizeSpace(tokenName));
return null;
}
return parameter.getValue();
@@ -177,7 +177,7 @@ public class TokenHelper {
String token = getToken(tokenName);
if (token == null) {
- LOG.debug("No token found for token name {} -> Invalid token ",
tokenName);
+ LOG.debug("No token found for token name {} -> Invalid token ",
normalizeSpace(tokenName));
return false;
}
diff --git
a/core/src/test/java/org/apache/struts2/action/CspReportActionTest.java
b/core/src/test/java/org/apache/struts2/action/CspReportActionTest.java
index 6485454fb..cbfaeeb20 100644
--- a/core/src/test/java/org/apache/struts2/action/CspReportActionTest.java
+++ b/core/src/test/java/org/apache/struts2/action/CspReportActionTest.java
@@ -19,6 +19,7 @@
package org.apache.struts2.action;
import org.apache.struts2.XWorkTestCase;
+import org.apache.struts2.test.LogCapture;
import org.apache.struts2.interceptor.csp.CspSettings;
import org.springframework.http.HttpMethod;
import org.springframework.mock.web.MockHttpServletRequest;
@@ -48,6 +49,17 @@ public class CspReportActionTest extends XWorkTestCase {
}
}
+ public void testDefaultActionDoesNotInjectLineBreaksIntoLog() {
+ try (LogCapture logs = new LogCapture(DefaultCspReportAction.class)) {
+ new
DefaultCspReportAction().processReport("{\"csp-report\":{}}\n12:00:00 ERROR
forged");
+
+ assertEquals(1, logs.messages().size());
+ String message = logs.messages().get(0);
+ assertTrue(message, message.contains("12:00:00 ERROR forged"));
+ assertFalse(message, message.contains("\n") || message.contains("\r"));
+ }
+ }
+
public void testWhenNoContentLength_thenReportNotProcessed() {
TestCspReportAction cspReportAction = new TestCspReportAction();
MockHttpServletRequest request = new MockHttpServletRequest("POST",
"/requestUri");
diff --git
a/core/src/test/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapperTest.java
b/core/src/test/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapperTest.java
index c047ec433..a03319c3a 100644
---
a/core/src/test/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapperTest.java
+++
b/core/src/test/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapperTest.java
@@ -25,6 +25,7 @@ import java.util.Map;
import org.apache.struts2.ServletActionContext;
import org.apache.struts2.StrutsInternalTestCase;
+import org.apache.struts2.test.LogCapture;
import org.apache.struts2.views.jsp.StrutsMockHttpServletRequest;
import org.springframework.mock.web.MockHttpServletRequest;
@@ -865,6 +866,22 @@ public class DefaultActionMapperTest extends
StrutsInternalTestCase {
assertEquals("/test_namespace/namespace.test/",
mapper.cleanupActionName(namespace));
}
+ public void testRejectedNamesDoNotInjectLineBreaksIntoLog() {
+ DefaultActionMapper mapper = new DefaultActionMapper();
+
+ try (LogCapture logs = new LogCapture(DefaultActionMapper.class)) {
+ mapper.cleanupNamespaceName("/ns\n12:00:00 ERROR forged");
+ mapper.cleanupActionName("action\r\n12:00:00 ERROR forged");
+ mapper.cleanupMethodName("method\n12:00:00 ERROR forged");
+
+ assertEquals(3, logs.messages().size());
+ for (String message : logs.messages()) {
+ assertTrue(message, message.contains("12:00:00 ERROR forged"));
+ assertFalse(message, message.contains("\n") ||
message.contains("\r"));
+ }
+ }
+ }
+
public void testAllowedActionNames() {
DefaultActionMapper mapper = new DefaultActionMapper();
diff --git
a/core/src/test/java/org/apache/struts2/dispatcher/mapper/RestfulActionMapperTest.java
b/core/src/test/java/org/apache/struts2/dispatcher/mapper/RestfulActionMapperTest.java
index 79efccfa0..aa1652f24 100644
---
a/core/src/test/java/org/apache/struts2/dispatcher/mapper/RestfulActionMapperTest.java
+++
b/core/src/test/java/org/apache/struts2/dispatcher/mapper/RestfulActionMapperTest.java
@@ -25,6 +25,7 @@ import java.util.Map;
import junit.framework.TestCase;
import org.apache.struts2.StrutsInternalTestCase;
+import org.apache.struts2.test.LogCapture;
import org.apache.struts2.views.jsp.StrutsMockHttpServletRequest;
/**
@@ -121,6 +122,17 @@ public class RestfulActionMapperTest extends
StrutsInternalTestCase {
assertEquals("my-app.action", am.getName());
}
+ public void testRejectedActionNameDoesNotInjectLineBreaksIntoLog() {
+ try (LogCapture logs = new LogCapture(RestfulActionMapper.class)) {
+ mapper.cleanupActionName("action\r\n12:00:00 ERROR forged");
+
+ assertEquals(1, logs.messages().size());
+ String message = logs.messages().get(0);
+ assertTrue(message, message.contains("12:00:00 ERROR forged"));
+ assertFalse(message, message.contains("\n") ||
message.contains("\r"));
+ }
+ }
+
protected void setUp() throws Exception {
super.setUp();
mapper = new RestfulActionMapper();
diff --git
a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java
b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java
index de17ba7cb..9b0d52b6b 100644
---
a/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java
+++
b/core/src/test/java/org/apache/struts2/interceptor/parameter/ParametersInterceptorTest.java
@@ -44,6 +44,7 @@ import org.apache.struts2.ognl.accessor.CompoundRootAccessor;
import org.apache.struts2.ognl.accessor.RootAccessor;
import org.apache.struts2.util.ValueStack;
import org.apache.struts2.util.ValueStackFactory;
+import org.apache.struts2.test.LogCapture;
import org.apache.struts2.util.reflection.ReflectionContextState;
import ognl.OgnlContext;
import org.apache.struts2.action.NoParameters;
@@ -1009,6 +1010,27 @@ public class ParametersInterceptorTest extends
XWorkTestCase {
return actual;
}
+ public void
testRejectedParameterNamesAndValuesDoNotInjectLineBreaksIntoLog() {
+ for (String devMode : new String[]{"false", "true"}) {
+ ParametersInterceptor pi = createParametersInterceptor();
+ pi.setDevMode(devMode);
+ pi.setParamNameMaxLength(10);
+ pi.setAcceptedValuePatterns("fooValue");
+ pi.setExcludedValuePatterns("barValue");
+
+ try (LogCapture logs = new
LogCapture(ParametersInterceptor.class)) {
+ pi.isAcceptableName("tooLongName\n12:00:00 ERROR forged");
+ pi.isAcceptableName("a\n12:00:00");
+ pi.isAcceptableValue("name", "bazValue\n12:00:00 ERROR
forged");
+
+ assertFalse(logs.messages().isEmpty());
+ for (String message : logs.messages()) {
+ assertFalse("devMode=" + devMode + ": " + message,
message.contains("\n12:00:00"));
+ }
+ }
+ }
+ }
+
private ParametersInterceptor createParametersInterceptor() {
ParametersInterceptor pi = new ParametersInterceptor();
container.inject(pi);
diff --git
a/core/src/test/java/org/apache/struts2/locale/DefaultLocaleProviderTest.java
b/core/src/test/java/org/apache/struts2/locale/DefaultLocaleProviderTest.java
index b58211fec..958a73274 100644
---
a/core/src/test/java/org/apache/struts2/locale/DefaultLocaleProviderTest.java
+++
b/core/src/test/java/org/apache/struts2/locale/DefaultLocaleProviderTest.java
@@ -19,6 +19,7 @@
package org.apache.struts2.locale;
import org.apache.struts2.ActionContext;
+import org.apache.struts2.test.LogCapture;
import org.junit.AfterClass;
import org.junit.Before;
import org.junit.BeforeClass;
@@ -50,6 +51,21 @@ public class DefaultLocaleProviderTest {
ActionContext.clear();
}
+ @Test
+ public void toLocaleRejectedValueDoesNotInjectLineBreaksIntoLog() {
+ try (LogCapture logs = new LogCapture(DefaultLocaleProvider.class)) {
+ // when
+ Locale actual = provider.toLocale("en\n12:00:00 ERROR forged");
+
+ // then
+ assertNull(actual);
+ assertEquals(1, logs.messages().size());
+ String message = logs.messages().get(0);
+ assertTrue(message, message.contains("12:00:00 ERROR forged"));
+ assertFalse(message, message.contains("\n") ||
message.contains("\r"));
+ }
+ }
+
@Test
public void getLocale() {
// given
diff --git a/core/src/test/java/org/apache/struts2/test/LogCapture.java
b/core/src/test/java/org/apache/struts2/test/LogCapture.java
new file mode 100644
index 000000000..58d021594
--- /dev/null
+++ b/core/src/test/java/org/apache/struts2/test/LogCapture.java
@@ -0,0 +1,62 @@
+/*
+ * 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.struts2.test;
+
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.Logger;
+import org.apache.logging.log4j.core.appender.AbstractAppender;
+import org.apache.logging.log4j.core.config.Property;
+
+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
+
+/**
+ * Collects what a class logs while open, as the formatted message followed by
the exception, if any. Only levels
+ * enabled by the test log configuration are seen.
+ */
+public class LogCapture implements AutoCloseable {
+
+ private final Logger logger;
+ private final AbstractAppender appender;
+ private final List<String> messages = new CopyOnWriteArrayList<>();
+
+ public LogCapture(Class<?> source) {
+ logger = (Logger) LogManager.getLogger(source);
+ appender = new AbstractAppender("LogCapture-" + System.nanoTime(),
null, null, false, Property.EMPTY_ARRAY) {
+ @Override
+ public void append(LogEvent event) {
+ String message = event.getMessage().getFormattedMessage();
+ messages.add(event.getThrown() == null ? message : message + "
" + event.getThrown());
+ }
+ };
+ appender.start();
+ logger.addAppender(appender);
+ }
+
+ public List<String> messages() {
+ return messages;
+ }
+
+ @Override
+ public void close() {
+ logger.removeAppender(appender);
+ appender.stop();
+ }
+}
diff --git a/core/src/test/java/org/apache/struts2/util/TokenHelperTest.java
b/core/src/test/java/org/apache/struts2/util/TokenHelperTest.java
index 07add2a07..85fcf3dcc 100644
--- a/core/src/test/java/org/apache/struts2/util/TokenHelperTest.java
+++ b/core/src/test/java/org/apache/struts2/util/TokenHelperTest.java
@@ -18,9 +18,14 @@
*/
package org.apache.struts2.util;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.core.Logger;
+import org.apache.logging.log4j.core.config.Configurator;
import org.apache.struts2.ActionContext;
import junit.framework.TestCase;
import org.apache.struts2.dispatcher.HttpParameters;
+import org.apache.struts2.test.LogCapture;
import java.util.HashMap;
import java.util.Map;
@@ -61,6 +66,40 @@ public class TokenHelperTest extends TestCase {
assertEquals(token, session.get(sessionTokenName));
}
+ public void testMissingTokenNameDoesNotInjectLineBreaksIntoLog() {
+ ActionContext.getContext().withParameters(HttpParameters.create(new
HashMap<String, String[]>()).build());
+
+ try (LogCapture logs = new LogCapture(TokenHelper.class)) {
+ assertNull(TokenHelper.getToken("token\n12:00:00 ERROR forged"));
+
+ assertEquals(1, logs.messages().size());
+ String message = logs.messages().get(0);
+ assertTrue(message, message.contains("12:00:00 ERROR forged"));
+ assertFalse(message, message.contains("\n") ||
message.contains("\r"));
+ }
+ }
+
+ public void
testMissingTokenForTokenNameDoesNotInjectLineBreaksIntoDebugLog() {
+ Map<String, String[]> params = new HashMap<>();
+ params.put(TokenHelper.TOKEN_NAME_FIELD, new String[]{"token\n12:00:00
ERROR forged"});
+
ActionContext.getContext().withParameters(HttpParameters.create(params).build());
+
+ Logger logger = (Logger) LogManager.getLogger(TokenHelper.class);
+ Level previousLevel = logger.getLevel();
+ Configurator.setLevel(logger.getName(), Level.DEBUG);
+ try (LogCapture logs = new LogCapture(TokenHelper.class)) {
+ assertFalse(TokenHelper.validToken());
+
+ assertFalse(logs.messages().isEmpty());
+ for (String message : logs.messages()) {
+ assertFalse(message, message.contains("\n") ||
message.contains("\r"));
+ }
+ assertTrue(logs.messages().stream().anyMatch(m -> m.contains("No
token found for token name")));
+ } finally {
+ Configurator.setLevel(logger.getName(), previousLevel);
+ }
+ }
+
public void testValidToken() {
String tokenName = "validTokenTest";
String token = TokenHelper.setToken(tokenName);
diff --git
a/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java
b/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java
index 9386a618d..a2002c9ab 100644
--- a/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java
+++ b/plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java
@@ -51,6 +51,8 @@ import java.lang.reflect.Type;
import java.util.*;
import java.util.regex.Pattern;
+import static org.apache.commons.lang3.StringUtils.normalizeSpace;
+
/**
* Populates an action from a JSON string
*/
@@ -297,20 +299,20 @@ public class JSONInterceptor extends AbstractInterceptor {
*/
private boolean isAcceptableNode(String fullPath, Object target, Object
action) {
if (fullPath.length() > paramNameMaxLength) {
- LOG.warn("JSON body parameter [{}] is too long, allowed length is
[{}]; rejected", fullPath, paramNameMaxLength);
+ LOG.warn("JSON body parameter [{}] is too long, allowed length is
[{}]; rejected", normalizeSpace(fullPath), paramNameMaxLength);
return false;
}
if (excludedPatterns != null &&
excludedPatterns.isExcluded(fullPath).isExcluded()) {
- LOG.warn("JSON body parameter [{}] matches an excluded pattern;
rejected", fullPath);
+ LOG.warn("JSON body parameter [{}] matches an excluded pattern;
rejected", normalizeSpace(fullPath));
return false;
}
if (!parameterAuthorizer.isAuthorized(fullPath, target, action)) {
LOG.warn("JSON body parameter [{}] rejected by @StrutsParameter
authorization on [{}]",
- fullPath, target.getClass().getName());
+ normalizeSpace(fullPath), target.getClass().getName());
return false;
}
if (applyPropertyFiltersToInput &&
!isAcceptedByPropertyFilters(fullPath)) {
- LOG.debug("JSON body parameter [{}] rejected by
excludeProperties/includeProperties on input", fullPath);
+ LOG.debug("JSON body parameter [{}] rejected by
excludeProperties/includeProperties on input", normalizeSpace(fullPath));
return false;
}
return true;
@@ -326,11 +328,11 @@ public class JSONInterceptor extends AbstractInterceptor {
*/
private boolean isAcceptableLeafName(String fullPath, Object action) {
if (acceptedPatterns != null &&
!acceptedPatterns.isAccepted(fullPath).isAccepted()) {
- LOG.warn("JSON body parameter [{}] does not match any accepted
pattern; rejected", fullPath);
+ LOG.warn("JSON body parameter [{}] does not match any accepted
pattern; rejected", normalizeSpace(fullPath));
return false;
}
if (action instanceof ParameterNameAware nameAware &&
!nameAware.acceptableParameterName(fullPath)) {
- LOG.debug("JSON body parameter [{}] rejected by ParameterNameAware
action", fullPath);
+ LOG.debug("JSON body parameter [{}] rejected by ParameterNameAware
action", normalizeSpace(fullPath));
return false;
}
return true;
@@ -358,18 +360,18 @@ public class JSONInterceptor extends AbstractInterceptor {
private boolean isAcceptableValue(String fullPath, Object value, Object
action) {
String stringValue = value == null ? null : String.valueOf(value);
if (action instanceof ParameterValueAware valueAware &&
!valueAware.acceptableParameterValue(stringValue)) {
- LOG.debug("JSON body value for parameter [{}] rejected by
ParameterValueAware action", fullPath);
+ LOG.debug("JSON body value for parameter [{}] rejected by
ParameterValueAware action", normalizeSpace(fullPath));
return false;
}
if (stringValue == null || stringValue.isEmpty()) {
return true;
}
if (isValueExcluded(stringValue)) {
- LOG.warn("JSON body value [{}] for parameter [{}] matches an
excluded value pattern; rejected", stringValue, fullPath);
+ LOG.warn("JSON body value [{}] for parameter [{}] matches an
excluded value pattern; rejected", normalizeSpace(stringValue),
normalizeSpace(fullPath));
return false;
}
if (!isValueAccepted(stringValue)) {
- LOG.warn("JSON body value [{}] for parameter [{}] does not match
any accepted value pattern; rejected", stringValue, fullPath);
+ LOG.warn("JSON body value [{}] for parameter [{}] does not match
any accepted value pattern; rejected", normalizeSpace(stringValue),
normalizeSpace(fullPath));
return false;
}
return true;
diff --git
a/plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java
b/plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java
index 32853534a..e7508fee4 100644
---
a/plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java
+++
b/plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java
@@ -18,6 +18,13 @@
*/
package org.apache.struts2.json;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.Logger;
+import org.apache.logging.log4j.core.appender.AbstractAppender;
+import org.apache.logging.log4j.core.config.Configurator;
+import org.apache.logging.log4j.core.config.Property;
import org.apache.struts2.ActionContext;
import org.apache.struts2.mock.MockActionInvocation;
import org.apache.struts2.util.ValueStack;
@@ -31,6 +38,7 @@ import org.springframework.mock.web.MockServletContext;
import java.util.Calendar;
import java.util.List;
import java.util.Map;
+import java.util.concurrent.CopyOnWriteArrayList;
public class JSONInterceptorTest extends StrutsTestCase {
private MockActionInvocationEx invocation;
@@ -714,6 +722,47 @@ public class JSONInterceptorTest extends StrutsTestCase {
assertNull(action.getBar());
}
+ public void testRejectedKeysAndValuesDoNotInjectLineBreaksIntoLog() throws
Exception {
+ this.request.setContent("{\"foo\":\"a\\n12:00:00 ERROR forged\",
\"b\\n12:00:00 ERROR forged\":\"b\"}".getBytes());
+ this.request.addHeader("Content-Type", "application/json");
+
+ JSONInterceptor interceptor = createInterceptor();
+ org.apache.struts2.security.DefaultAcceptedPatternsChecker accepted =
+ new
org.apache.struts2.security.DefaultAcceptedPatternsChecker();
+ interceptor.setAcceptedPatterns(accepted);
+ interceptor.setAcceptedValuePatterns("allowed");
+ TestAction action = new TestAction();
+
+ this.invocation.setAction(action);
+ this.invocation.getStack().push(action);
+
+ List<String> messages = new CopyOnWriteArrayList<>();
+ Logger logger = (Logger) LogManager.getLogger(JSONInterceptor.class);
+ AbstractAppender appender = new
AbstractAppender("JSONInterceptorTest", null, null, false,
Property.EMPTY_ARRAY) {
+ @Override
+ public void append(LogEvent event) {
+ messages.add(event.getMessage().getFormattedMessage());
+ }
+ };
+ appender.start();
+ logger.addAppender(appender);
+ Level previousLevel = logger.getLevel();
+ Configurator.setLevel(logger.getName(), Level.WARN);
+ try {
+ interceptor.intercept(this.invocation);
+ } finally {
+ Configurator.setLevel(logger.getName(), previousLevel);
+ logger.removeAppender(appender);
+ appender.stop();
+ }
+
+ assertFalse(messages.isEmpty());
+ for (String message : messages) {
+ assertFalse(message, message.contains("\n") ||
message.contains("\r"));
+ }
+ assertTrue(messages.stream().anyMatch(m -> m.contains("12:00:00 ERROR
forged")));
+ }
+
public void testAcceptedNamePatternRejectsKey() throws Exception {
this.request.setContent("{\"foo\":\"a\", \"bar\":\"b\"}".getBytes());
this.request.addHeader("Content-Type", "application/json");
