From cc00343f1b4ae763f3403f5678a9fdebb4c73665 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 19 Jul 2026 20:25:47 +0200 Subject: [PATCH] WW-5650 Obtain a fresh JSON reader/writer per request in JSONInterceptor (#1782) * WW-5650 revert StrutsJSONReader to plain single-use instance fields * WW-5650 revert StrutsJSONWriter to plain single-use instance fields * WW-5650 obtain a fresh JSONUtil per request in JSONInterceptor * WW-5650 resolve JSONUtil lazily only on JSON request paths Move getJSONUtil() into the JSON and JSON-RPC branches of intercept() so requests with a non-JSON content type no longer construct and discard an unused JSONUtil/reader/writer graph. Also trim a stray trailing blank line in StrutsJSONWriter. Co-Authored-By: Claude Opus 4.8 * WW-5650 test(json): assert JSONWriter bean stays prototype-scoped Guards the response-side invariant from WW-5644: StrutsJSONWriter now uses plain instance fields and is not thread-safe, so cross-request safety relies solely on the writer bean being prototype-scoped. Assert distinct instances per container lookup so a future switch to singleton scope fails the build. Addresses review feedback on #1782 without adding a getWriter() accessor purely for tests. Co-Authored-By: Claude Opus 4.8 --------- Co-authored-by: Claude Opus 4.8 --- .../apache/struts2/json/JSONInterceptor.java | 19 ++- .../apache/struts2/json/StrutsJSONReader.java | 134 ++++++++---------- .../apache/struts2/json/StrutsJSONWriter.java | 113 ++++++--------- .../struts2/json/JSONInterceptorTest.java | 51 ++++--- .../struts2/json/StrutsJSONReaderTest.java | 80 ----------- .../struts2/json/StrutsJSONWriterTest.java | 74 ---------- 6 files changed, 149 insertions(+), 322 deletions(-) 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 b1c172388..804542399 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 @@ -22,6 +22,7 @@ import org.apache.struts2.action.Action; import org.apache.struts2.action.ParameterNameAware; import org.apache.struts2.action.ParameterValueAware; import org.apache.struts2.ActionInvocation; +import org.apache.struts2.inject.Container; import org.apache.struts2.inject.Inject; import org.apache.struts2.interceptor.AbstractInterceptor; import org.apache.struts2.interceptor.parameter.ParameterAuthorizer; @@ -77,7 +78,7 @@ public class JSONInterceptor extends AbstractInterceptor { private String jsonContentType = "application/json"; private String jsonRpcContentType = "application/json-rpc"; - private JSONUtil jsonUtil; + private Container container; private ParameterAuthorizer parameterAuthorizer; private ExcludedPatternsChecker excludedPatterns; private AcceptedPatternsChecker acceptedPatterns; @@ -111,7 +112,8 @@ public class JSONInterceptor extends AbstractInterceptor { if (jsonContentType.equalsIgnoreCase(requestContentType)) { // load JSON object - applyLimitsToReader(); + JSONUtil jsonUtil = getJSONUtil(); + applyLimitsToReader(jsonUtil); Object obj = jsonUtil.deserializeInput(request.getReader(), maxLength); // JSON array (this.root cannot be null in this case) @@ -154,10 +156,11 @@ public class JSONInterceptor extends AbstractInterceptor { throw new JSONException("Unable to deserialize JSON object from request"); } } else if (jsonRpcContentType.equalsIgnoreCase(requestContentType)) { + JSONUtil jsonUtil = getJSONUtil(); Object result; if (this.enableSMD) { // load JSON object - applyLimitsToReader(); + applyLimitsToReader(jsonUtil); Object obj = jsonUtil.deserializeInput(request.getReader(), maxLength); if (obj instanceof Map) { @@ -208,7 +211,7 @@ public class JSONInterceptor extends AbstractInterceptor { return invocation.invoke(); } - private void applyLimitsToReader() { + private void applyLimitsToReader(JSONUtil jsonUtil) { JSONReader reader = jsonUtil.getReader(); reader.setMaxElements(maxElements); reader.setMaxDepth(maxDepth); @@ -744,8 +747,12 @@ public class JSONInterceptor extends AbstractInterceptor { } @Inject - public void setJsonUtil(JSONUtil jsonUtil) { - this.jsonUtil = jsonUtil; + public void setContainer(Container container) { + this.container = container; + } + + protected JSONUtil getJSONUtil() { + return container.getInstance(JSONUtil.class); } @Inject diff --git a/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONReader.java b/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONReader.java index 96593d2b9..a1399a461 100644 --- a/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONReader.java +++ b/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONReader.java @@ -30,12 +30,9 @@ import java.util.Map; * Deserializes an object from a JSON string with configurable limits * to prevent denial-of-service attacks via malicious payloads. *

- * *

- * A single StrutsJSONReader instance is shared across all concurrent requests handled by a given - * JSONInterceptor (it is injected once, not created per request), so the cursor, token buffer and - * nesting depth of an in-progress parse are kept in a {@link ThreadLocal}, not instance fields -- - * otherwise two concurrent {@link #read(String)} calls would corrupt each other's parse state. + * This reader keeps per-parse state in instance fields and is not thread-safe; + * obtain a fresh instance per parse (the container serves it as a prototype bean). See WW-5650. *

*/ public class StrutsJSONReader implements JSONReader { @@ -54,20 +51,16 @@ public class StrutsJSONReader implements JSONReader { 't', '\t' ); - private static final class ParseState { - private CharacterIterator it; - private char c; - private Object token; - private final StringBuilder buf = new StringBuilder(); - private int depth; - } - - private static final ThreadLocal PARSE_STATE = new ThreadLocal<>(); + private CharacterIterator it; + private char c; + private Object token; + private final StringBuilder buf = new StringBuilder(); private int maxElements = DEFAULT_MAX_ELEMENTS; private int maxDepth = DEFAULT_MAX_DEPTH; private int maxStringLength = DEFAULT_MAX_STRING_LENGTH; private int maxKeyLength = DEFAULT_MAX_KEY_LENGTH; + private int depth; @Override public void setMaxElements(int maxElements) { @@ -90,99 +83,92 @@ public class StrutsJSONReader implements JSONReader { } protected char next() { - ParseState state = PARSE_STATE.get(); - state.c = state.it.next(); + this.c = this.it.next(); - return state.c; + return this.c; } protected void skipWhiteSpace() { - while (Character.isWhitespace(PARSE_STATE.get().c)) { + while (Character.isWhitespace(this.c)) { this.next(); } } @Override public Object read(String string) throws JSONException { - ParseState state = new ParseState(); - state.it = new StringCharacterIterator(string); - state.c = state.it.first(); - PARSE_STATE.set(state); - try { - return this.read(); - } finally { - PARSE_STATE.remove(); - } + this.it = new StringCharacterIterator(string); + this.c = this.it.first(); + this.depth = 0; + + return this.read(); } protected Object read() throws JSONException { - ParseState state = PARSE_STATE.get(); Object ret; this.skipWhiteSpace(); - if (state.c == '"') { + if (this.c == '"') { this.next(); ret = this.string('"'); - } else if (state.c == '\'') { + } else if (this.c == '\'') { this.next(); ret = this.string('\''); - } else if (state.c == '[') { + } else if (this.c == '[') { this.next(); ret = this.array(); - } else if (state.c == ']') { + } else if (this.c == ']') { ret = ARRAY_END; this.next(); - } else if (state.c == ',') { + } else if (this.c == ',') { ret = COMMA; this.next(); - } else if (state.c == '{') { + } else if (this.c == '{') { this.next(); ret = this.object(); - } else if (state.c == '}') { + } else if (this.c == '}') { ret = OBJECT_END; this.next(); - } else if (state.c == ':') { + } else if (this.c == ':') { ret = COLON; this.next(); - } else if ((state.c == 't') && (this.next() == 'r') && (this.next() == 'u') && (this.next() == 'e')) { + } else if ((this.c == 't') && (this.next() == 'r') && (this.next() == 'u') && (this.next() == 'e')) { ret = Boolean.TRUE; this.next(); - } else if ((state.c == 'f') && (this.next() == 'a') && (this.next() == 'l') && (this.next() == 's') + } else if ((this.c == 'f') && (this.next() == 'a') && (this.next() == 'l') && (this.next() == 's') && (this.next() == 'e')) { ret = Boolean.FALSE; this.next(); - } else if ((state.c == 'n') && (this.next() == 'u') && (this.next() == 'l') && (this.next() == 'l')) { + } else if ((this.c == 'n') && (this.next() == 'u') && (this.next() == 'l') && (this.next() == 'l')) { ret = null; this.next(); - } else if (Character.isDigit(state.c) || (state.c == '-')) { + } else if (Character.isDigit(this.c) || (this.c == '-')) { ret = this.number(); } else { throw buildInvalidInputException(); } - state.token = ret; + this.token = ret; return ret; } protected Map object() throws JSONException { - ParseState state = PARSE_STATE.get(); - if (state.depth >= this.maxDepth) { + if (this.depth >= this.maxDepth) { throw new JSONException("JSON object nesting exceeds maximum allowed depth (" + this.maxDepth + "). Use " + JSONConstants.JSON_MAX_DEPTH + " to increase the limit."); } - state.depth++; + this.depth++; try { Map ret = new HashMap<>(); Object next = this.read(); if (next != OBJECT_END) { String key = (String) next; validateKeyLength(key); - while (state.token != OBJECT_END) { + while (this.token != OBJECT_END) { this.read(); // should be a colon - if (state.token != OBJECT_END) { + if (this.token != OBJECT_END) { if (ret.size() >= this.maxElements) { throw new JSONException("JSON object exceeds maximum allowed elements (" + this.maxElements + "). Use " + JSONConstants.JSON_MAX_ELEMENTS + " to increase the limit."); @@ -205,7 +191,7 @@ public class StrutsJSONReader implements JSONReader { return ret; } finally { - state.depth--; + this.depth--; } } @@ -217,22 +203,21 @@ public class StrutsJSONReader implements JSONReader { } protected JSONException buildInvalidInputException() { - return new JSONException("Input string is not well formed JSON (invalid char " + PARSE_STATE.get().c + ")"); + return new JSONException("Input string is not well formed JSON (invalid char " + this.c + ")"); } protected List array() throws JSONException { - ParseState state = PARSE_STATE.get(); - if (state.depth >= this.maxDepth) { + if (this.depth >= this.maxDepth) { throw new JSONException("JSON array nesting exceeds maximum allowed depth (" + this.maxDepth + "). Use " + JSONConstants.JSON_MAX_DEPTH + " to increase the limit."); } - state.depth++; + this.depth++; try { List ret = new ArrayList<>(); Object value = this.read(); - while (state.token != ARRAY_END) { + while (this.token != ARRAY_END) { if (ret.size() >= this.maxElements) { throw new JSONException("JSON array exceeds maximum allowed elements (" + this.maxElements + "). Use " + JSONConstants.JSON_MAX_ELEMENTS + " to increase the limit."); @@ -249,32 +234,31 @@ public class StrutsJSONReader implements JSONReader { return ret; } finally { - state.depth--; + this.depth--; } } protected Object number() throws JSONException { - ParseState state = PARSE_STATE.get(); - state.buf.setLength(0); + this.buf.setLength(0); boolean toDouble = false; - if (state.c == '-') { + if (this.c == '-') { this.add(); } this.addDigits(); - if (state.c == '.') { + if (this.c == '.') { toDouble = true; this.add(); this.addDigits(); } - if ((state.c == 'e') || (state.c == 'E')) { + if ((this.c == 'e') || (this.c == 'E')) { toDouble = true; this.add(); - if ((state.c == '+') || (state.c == '-')) { + if ((this.c == '+') || (this.c == '-')) { this.add(); } @@ -283,13 +267,13 @@ public class StrutsJSONReader implements JSONReader { if (toDouble) { try { - return Double.parseDouble(state.buf.toString()); + return Double.parseDouble(this.buf.toString()); } catch (NumberFormatException e) { throw buildInvalidInputException(); } } else { try { - return Long.parseLong(state.buf.toString()); + return Long.parseLong(this.buf.toString()); } catch (NumberFormatException e) { throw buildInvalidInputException(); } @@ -297,17 +281,16 @@ public class StrutsJSONReader implements JSONReader { } protected Object string(char quote) throws JSONException { - ParseState state = PARSE_STATE.get(); - state.buf.setLength(0); + this.buf.setLength(0); - while ((state.c != quote) && (state.c != CharacterIterator.DONE)) { - if (state.c == '\\') { + while ((this.c != quote) && (this.c != CharacterIterator.DONE)) { + if (this.c == '\\') { this.next(); - if (state.c == 'u') { + if (this.c == 'u') { this.add(this.unicode()); } else { - Character value = escapes.get(state.c); + Character value = escapes.get(this.c); if (value != null) { this.add(value); @@ -316,7 +299,7 @@ public class StrutsJSONReader implements JSONReader { } else { this.add(); } - if (state.buf.length() > this.maxStringLength) { + if (this.buf.length() > this.maxStringLength) { throw new JSONException("JSON string exceeds maximum allowed length (" + this.maxStringLength + "). Use " + JSONConstants.JSON_MAX_STRING_LENGTH + " to increase the limit."); } @@ -324,33 +307,32 @@ public class StrutsJSONReader implements JSONReader { this.next(); - return state.buf.toString(); + return this.buf.toString(); } protected void add(char cc) { - PARSE_STATE.get().buf.append(cc); + this.buf.append(cc); this.next(); } protected void add() { - this.add(PARSE_STATE.get().c); + this.add(this.c); } protected void addDigits() { - while (Character.isDigit(PARSE_STATE.get().c)) { + while (Character.isDigit(this.c)) { this.add(); } } protected char unicode() { - ParseState state = PARSE_STATE.get(); int value = 0; for (int i = 0; i < 4; ++i) { value = switch (this.next()) { - case '0', '1', '2', '3', '4', '5', '6', '7', '8', '9' -> (value << 4) + (state.c - '0'); - case 'a', 'b', 'c', 'd', 'e', 'f' -> (value << 4) + (state.c - 'W'); - case 'A', 'B', 'C', 'D', 'E', 'F' -> (value << 4) + (state.c - '7'); + case '0', '1', '2', '3', '4', '5', '6', '7', '8', '9' -> (value << 4) + (this.c - '0'); + case 'a', 'b', 'c', 'd', 'e', 'f' -> (value << 4) + (this.c - 'W'); + case 'A', 'B', 'C', 'D', 'E', 'F' -> (value << 4) + (this.c - '7'); default -> value; }; } diff --git a/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONWriter.java b/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONWriter.java index 59fadd9b3..b7f540b0b 100644 --- a/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONWriter.java +++ b/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONWriter.java @@ -66,13 +66,9 @@ import java.util.regex.Pattern; * Serializes an object into JavaScript Object Notation (JSON). If cyclic * references are detected they will be nulled out. *

- * *

- * A single StrutsJSONWriter instance is shared across all concurrent responses handled by a given - * JSONResult/JSONInterceptor (it is injected once, not created per request), so the output buffer, - * cycle-detection stack and other per-write state are kept in a {@link ThreadLocal}, not instance - * fields -- otherwise two concurrent {@link #write(Object)} calls would corrupt each other's output, - * including returning one request's serialized data as another, unrelated request's response. + * This writer keeps per-write state in instance fields and is not thread-safe; + * obtain a fresh instance per write (the container serves it as a prototype bean). See WW-5650. *

*/ public class StrutsJSONWriter implements JSONWriter { @@ -92,22 +88,17 @@ public class StrutsJSONWriter implements JSONWriter { BEAN_INFO_CACHE.clear(); } - private static final class WriteState { - private final StringBuilder buf = new StringBuilder(); - private final Deque stack = new ArrayDeque<>(); - private Object root; - private boolean buildExpr = true; - private String exprStack = ""; - private Collection excludeProperties; - private Collection includeProperties; - private boolean excludeNullProperties; - } - - private static final ThreadLocal WRITE_STATE = new ThreadLocal<>(); - + private final StringBuilder buf = new StringBuilder(); + private final Deque stack = new ArrayDeque<>(); private boolean ignoreHierarchy = true; + private Object root; + private boolean buildExpr = true; + private String exprStack = ""; + private Collection excludeProperties; + private Collection includeProperties; private DateFormat dateFormat; private boolean enumAsBean = ENUM_AS_BEAN_DEFAULT; + private boolean excludeNullProperties; private boolean excludeProxyProperties; private ProxyService proxyService; @@ -142,20 +133,18 @@ public class StrutsJSONWriter implements JSONWriter { @Override public String write(Object object, Collection excludeProperties, Collection includeProperties, boolean excludeNullProperties) throws JSONException { - WriteState state = new WriteState(); - state.excludeNullProperties = excludeNullProperties; - state.root = object; - state.buildExpr = ((excludeProperties != null) && !excludeProperties.isEmpty()) + this.excludeNullProperties = excludeNullProperties; + this.buf.setLength(0); + this.stack.clear(); + this.root = object; + this.exprStack = ""; + this.buildExpr = ((excludeProperties != null) && !excludeProperties.isEmpty()) || ((includeProperties != null) && !includeProperties.isEmpty()); - state.excludeProperties = excludeProperties; - state.includeProperties = includeProperties; - WRITE_STATE.set(state); - try { - this.value(object, null); - return state.buf.toString(); - } finally { - WRITE_STATE.remove(); - } + this.excludeProperties = excludeProperties; + this.includeProperties = includeProperties; + this.value(object, null); + + return this.buf.toString(); } /** @@ -171,7 +160,7 @@ public class StrutsJSONWriter implements JSONWriter { return; } - if (WRITE_STATE.get().stack.contains(object)) { + if (this.stack.contains(object)) { Class clazz = object.getClass(); // cyclic reference @@ -196,8 +185,7 @@ public class StrutsJSONWriter implements JSONWriter { * @throws JSONException in case of error during serialize */ protected void process(Object object, Method method) throws JSONException { - WriteState state = WRITE_STATE.get(); - state.stack.push(object); + this.stack.push(object); if (object instanceof Class) { this.string(object); @@ -229,7 +217,7 @@ public class StrutsJSONWriter implements JSONWriter { processCustom(object, method); } - state.stack.pop(); + this.stack.pop(); } /** @@ -252,13 +240,12 @@ public class StrutsJSONWriter implements JSONWriter { protected void bean(Object object) throws JSONException { this.add("{"); - WriteState state = WRITE_STATE.get(); BeanInfo info; try { Class clazz = excludeProxyProperties ? proxyService.ultimateTargetClass(object) : object.getClass(); - info = ((object == state.root) && this.ignoreHierarchy) + info = ((object == this.root) && this.ignoreHierarchy) ? getBeanInfoIgnoreHierarchy(clazz) : getBeanInfo(clazz); @@ -284,7 +271,7 @@ public class StrutsJSONWriter implements JSONWriter { continue; } String expr = null; - if (state.buildExpr) { + if (this.buildExpr) { expr = this.expandExpr(name); if (this.shouldExcludeProperty(expr)) { continue; @@ -299,7 +286,7 @@ public class StrutsJSONWriter implements JSONWriter { boolean propertyPrinted = this.add(name, value, accessor, hasData); hasData = hasData || propertyPrinted; - if (state.buildExpr) { + if (this.buildExpr) { this.setExprStack(expr); } } @@ -414,28 +401,25 @@ public class StrutsJSONWriter implements JSONWriter { } protected String expandExpr(int i) { - return WRITE_STATE.get().exprStack + "[" + i + "]"; + return this.exprStack + "[" + i + "]"; } protected String expandExpr(String property) { - String exprStack = WRITE_STATE.get().exprStack; - if (exprStack.isEmpty()) { + if (this.exprStack.isEmpty()) { return property; } - return exprStack + "." + property; + return this.exprStack + "." + property; } protected String setExprStack(String expr) { - WriteState state = WRITE_STATE.get(); - String s = state.exprStack; - state.exprStack = expr; + String s = this.exprStack; + this.exprStack = expr; return s; } protected boolean shouldExcludeProperty(String expr) { - WriteState state = WRITE_STATE.get(); - if (state.excludeProperties != null) { - for (Pattern pattern : state.excludeProperties) { + if (this.excludeProperties != null) { + for (Pattern pattern : this.excludeProperties) { if (pattern.matcher(expr).matches()) { if (LOG.isDebugEnabled()) { LOG.debug("Ignoring property because of exclude rule: " + expr); @@ -445,8 +429,8 @@ public class StrutsJSONWriter implements JSONWriter { } } - if (state.includeProperties != null) { - for (Pattern pattern : state.includeProperties) { + if (this.includeProperties != null) { + for (Pattern pattern : this.includeProperties) { if (pattern.matcher(expr).matches()) { return false; } @@ -463,7 +447,7 @@ public class StrutsJSONWriter implements JSONWriter { * Add name/value pair to buffer */ protected boolean add(String name, Object value, Method method, boolean hasData) throws JSONException { - if (WRITE_STATE.get().excludeNullProperties && value == null) { + if (excludeNullProperties && value == null) { return false; } if (hasData) { @@ -482,25 +466,24 @@ public class StrutsJSONWriter implements JSONWriter { protected void map(Map map, Method method) throws JSONException { this.add("{"); - WriteState state = WRITE_STATE.get(); Iterator it = map.entrySet().iterator(); boolean warnedNonString = false; // one report per map boolean hasData = false; while (it.hasNext()) { Map.Entry entry = (Map.Entry) it.next(); - if (state.excludeNullProperties && entry.getValue() == null) { + if (excludeNullProperties && entry.getValue() == null) { continue; } Object key = entry.getKey(); if (key == null) { - LOG.error("Cannot build expression for null key in {}", state.exprStack); + LOG.error("Cannot build expression for null key in {}", exprStack); continue; } String expr = null; - if (state.buildExpr) { + if (this.buildExpr) { expr = this.expandExpr(key.toString()); if (this.shouldExcludeProperty(expr)) { continue; @@ -520,7 +503,7 @@ public class StrutsJSONWriter implements JSONWriter { this.value(key.toString(), method); this.add(":"); this.value(entry.getValue(), method); - if (state.buildExpr) { + if (this.buildExpr) { this.setExprStack(expr); } } @@ -587,11 +570,10 @@ public class StrutsJSONWriter implements JSONWriter { protected void array(Iterator it, Method method) throws JSONException { this.add("["); - WriteState state = WRITE_STATE.get(); boolean hasData = false; for (int i = 0; it.hasNext(); i++) { String expr = null; - if (state.buildExpr) { + if (this.buildExpr) { expr = this.expandExpr(i); if (this.shouldExcludeProperty(expr)) { it.next(); @@ -604,7 +586,7 @@ public class StrutsJSONWriter implements JSONWriter { } hasData = true; this.value(it.next(), method); - if (state.buildExpr) { + if (this.buildExpr) { this.setExprStack(expr); } } @@ -618,13 +600,12 @@ public class StrutsJSONWriter implements JSONWriter { protected void array(Object object, Method method) throws JSONException { this.add("["); - WriteState state = WRITE_STATE.get(); int length = Array.getLength(object); boolean hasData = false; for (int i = 0; i < length; ++i) { String expr = null; - if (state.buildExpr) { + if (this.buildExpr) { expr = this.expandExpr(i); if (this.shouldExcludeProperty(expr)) { continue; @@ -636,7 +617,7 @@ public class StrutsJSONWriter implements JSONWriter { } hasData = true; this.value(Array.get(object, i), method); - if (state.buildExpr) { + if (this.buildExpr) { this.setExprStack(expr); } } @@ -692,14 +673,14 @@ public class StrutsJSONWriter implements JSONWriter { * Add object to buffer */ protected void add(Object obj) { - WRITE_STATE.get().buf.append(obj); + this.buf.append(obj); } /* * Add char to buffer */ protected void add(char c) { - WRITE_STATE.get().buf.append(c); + this.buf.append(c); } /** 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 eaf428600..4aa27563a 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 @@ -44,10 +44,7 @@ public class JSONInterceptorTest extends StrutsTestCase { private JSONInterceptor createInterceptor() { JSONInterceptor interceptor = new JSONInterceptor(); - JSONUtil jsonUtil = new JSONUtil(); - jsonUtil.setReader(new StrutsJSONReader()); - jsonUtil.setWriter(new StrutsJSONWriter()); - interceptor.setJsonUtil(jsonUtil); + interceptor.setContainer(container); // Default: allow all parameters (simulates requireAnnotations=false) interceptor.setParameterAuthorizer((parameterName, target, action) -> true); interceptor.setExcludedPatterns(new org.apache.struts2.security.DefaultExcludedPatternsChecker()); @@ -567,10 +564,7 @@ public class JSONInterceptorTest extends StrutsTestCase { this.request.addHeader("Content-Type", "application/json"); JSONInterceptor interceptor = new JSONInterceptor(); - JSONUtil jsonUtil = new JSONUtil(); - jsonUtil.setReader(new StrutsJSONReader()); - jsonUtil.setWriter(new StrutsJSONWriter()); - interceptor.setJsonUtil(jsonUtil); + interceptor.setContainer(container); // Only authorize "foo", reject "bar" interceptor.setParameterAuthorizer((parameterName, target, action) -> "foo".equals(parameterName)); TestAction action = new TestAction(); @@ -589,10 +583,7 @@ public class JSONInterceptorTest extends StrutsTestCase { // Simulate a custom JSON reader producing a Map with a non-String key. // The authorizer should skip the entry rather than throw ClassCastException. JSONInterceptor interceptor = new JSONInterceptor(); - JSONUtil jsonUtil = new JSONUtil(); - jsonUtil.setReader(new StrutsJSONReader()); - jsonUtil.setWriter(new StrutsJSONWriter()); - interceptor.setJsonUtil(jsonUtil); + interceptor.setContainer(container); interceptor.setParameterAuthorizer((parameterName, target, action) -> true); java.util.Map mixedKeyMap = new java.util.LinkedHashMap<>(); @@ -658,10 +649,7 @@ public class JSONInterceptorTest extends StrutsTestCase { this.request.addHeader("Content-Type", "application/json"); JSONInterceptor interceptor = new JSONInterceptor(); - JSONUtil jsonUtil = new JSONUtil(); - jsonUtil.setReader(new StrutsJSONReader()); - jsonUtil.setWriter(new StrutsJSONWriter()); - interceptor.setJsonUtil(jsonUtil); + interceptor.setContainer(container); // Authorize "bean" (top-level) and "bean.stringField" (nested) but reject "bean.intField" interceptor.setParameterAuthorizer((parameterName, target, action) -> "bean".equals(parameterName) || "bean.stringField".equals(parameterName)); @@ -688,10 +676,7 @@ public class JSONInterceptorTest extends StrutsTestCase { this.request.addHeader("Content-Type", "application/json"); JSONInterceptor interceptor = new JSONInterceptor(); - JSONUtil jsonUtil = new JSONUtil(); - jsonUtil.setReader(new StrutsJSONReader()); - jsonUtil.setWriter(new StrutsJSONWriter()); - interceptor.setJsonUtil(jsonUtil); + interceptor.setContainer(container); interceptor.setRoot("bean"); // Reject all parameters — simulates strict requireAnnotations interceptor.setParameterAuthorizer((parameterName, target, action) -> false); @@ -930,6 +915,32 @@ public class JSONInterceptorTest extends StrutsTestCase { assertEquals("b", action.getBar()); } + public void testObtainsFreshJSONUtilAndReaderPerInvocation() { + JSONInterceptor interceptor = new JSONInterceptor(); + interceptor.setContainer(container); // StrutsTestCase-provided container + + JSONUtil first = interceptor.getJSONUtil(); + JSONUtil second = interceptor.getJSONUtil(); + + assertNotSame("interceptor must obtain a fresh JSONUtil per request", first, second); + assertNotSame("each fresh JSONUtil must carry its own reader (no shared parse state)", + first.getReader(), second.getReader()); + } + + /** + * StrutsJSONWriter keeps per-serialization state in plain instance fields and is not + * thread-safe, so the response-side protection from WW-5644 now relies entirely on the writer + * bean being prototype-scoped (a fresh writer per request/result). Guard that scope directly: + * if it were ever switched to singleton, cross-request response state would leak again. + */ + public void testObtainsFreshWriterPerAcquisition() { + JSONWriter first = container.getInstance(JSONWriter.class); + JSONWriter second = container.getInstance(JSONWriter.class); + + assertNotSame("JSONWriter bean must be prototype-scoped so serialize state is never shared across responses", + first, second); + } + public void testIncludePropertiesAppliedToNestedInputWhenEnabled() throws Exception { this.request.setContent("{\"bean\": {\"stringField\": \"keep\", \"intField\": 42}}".getBytes()); this.request.addHeader("Content-Type", "application/json"); diff --git a/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java b/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java index 827711c61..d8af5e21f 100644 --- a/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java +++ b/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONReaderTest.java @@ -22,11 +22,6 @@ import org.junit.Test; import java.util.List; import java.util.Map; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; @@ -162,79 +157,4 @@ public class StrutsJSONReaderTest { assertTrue(ex.getMessage().contains("maximum allowed depth")); } - /** - * A single StrutsJSONReader instance is injected once into JSONUtil/JSONInterceptor and reused - * across every concurrent request handled by that interceptor, so read() must be safe to call - * concurrently from multiple threads on the same instance without one call's parse state - * (cursor, token buffer, depth counter) leaking into another's -- both as a maxDepth bypass and - * as one request's data appearing inside a different, concurrently-parsed request's result. - * - *

Uses a higher thread count than the minimum needed to demonstrate the bug: with only two - * threads on a machine with many cores, the OS scheduler has no need to preempt either thread - * mid-call, so the race window is rarely hit and the test can pass even against the unpatched - * code. Sixteen threads contending for the same instance reproduces both symptoms reliably.

- */ - @Test - public void testConcurrentReuseDoesNotBypassMaxDepthOrLeakDataAcrossParses() throws Exception { - int maxDepth = 5; - var reader = new StrutsJSONReader(); - reader.setMaxDepth(maxDepth); - - String overDepthPayload = nestedObject(maxDepth + 1); - int threadCount = 16; - int iterations = 10_000; - AtomicInteger overDepthAccepted = new AtomicInteger(); - AtomicInteger crossThreadLeaks = new AtomicInteger(); - CountDownLatch start = new CountDownLatch(1); - ExecutorService pool = Executors.newFixedThreadPool(threadCount); - - for (int t = 0; t < threadCount; t++) { - String marker = "MARKER_" + t + "_9f8e7d6c5b4a3210"; - String ownPayload = "{\"account\":\"user" + t + "\",\"secret\":\"" + marker + "\"}"; - pool.submit(() -> { - await(start); - for (int i = 0; i < iterations; i++) { - try { - Object result = reader.read(ownPayload); - if (!(result instanceof Map) || !marker.equals(((Map) result).get("secret"))) { - // this thread's own parse doesn't even contain its own data: the shared - // cursor/buffer was corrupted or overwritten by another thread - crossThreadLeaks.incrementAndGet(); - } - } catch (JSONException ignored) { - // a failed parse under contention is not itself the thing measured here - } - try { - reader.read(overDepthPayload); - overDepthAccepted.incrementAndGet(); - } catch (JSONException expected) { - // must always be rejected: depth is one more than maxDepth - } - } - }); - } - - start.countDown(); - pool.shutdown(); - assertTrue(pool.awaitTermination(120, TimeUnit.SECONDS)); - - assertEquals("an over-depth payload must never be accepted, even when the reader instance " + - "is shared with concurrent unrelated parses", 0, overDepthAccepted.get()); - assertEquals("a concurrently-parsed request's data must never appear in place of this " + - "thread's own parsed result", 0, crossThreadLeaks.get()); - } - - private static void await(CountDownLatch latch) { - try { - latch.await(); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } - } - - private static String nestedObject(int depth) { - StringBuilder sb = new StringBuilder(); - sb.append("{\"a\":".repeat(depth)).append("1").append("}".repeat(depth)); - return sb.toString(); - } } diff --git a/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONWriterTest.java b/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONWriterTest.java index 2239c5f71..acc52dcaa 100644 --- a/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONWriterTest.java +++ b/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONWriterTest.java @@ -39,11 +39,6 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.TimeZone; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -320,73 +315,4 @@ public class StrutsJSONWriterTest { assertTrue(json.contains("\"calendar\":\"2012-12-23T10:10:10\"")); } - public static class NamedBean { - private final String owner; - private final String secret; - - public NamedBean(String owner, String secret) { - this.owner = owner; - this.secret = secret; - } - - public String getOwner() { return owner; } - public String getSecret() { return secret; } - } - - /** - * A single StrutsJSONWriter instance is injected once into JSONUtil/JSONResult and reused - * across every concurrent response handled by that result, so write() must be safe to call - * concurrently from multiple threads on the same instance without one call's output buffer - * leaking into -- or being overwritten by -- another call's output. - * - *

Uses a higher thread count than the minimum needed to demonstrate the bug: with only two - * threads on a machine with many cores, the OS scheduler has no need to preempt either thread - * mid-call, so the race window is rarely hit and the test can pass even against the unpatched - * code. Sixteen threads contending for the same instance reproduces the corruption reliably.

- */ - @Test - public void testConcurrentReuseDoesNotSwapResponsesAcrossWrites() throws Exception { - JSONWriter sharedWriter = new StrutsJSONWriter(); - int threadCount = 16; - int iterations = 20_000; - AtomicInteger corrupted = new AtomicInteger(); - CountDownLatch start = new CountDownLatch(1); - ExecutorService pool = Executors.newFixedThreadPool(threadCount); - - for (int t = 0; t < threadCount; t++) { - String secret = "SECRET_" + t + "_9f8e7d6c5b4a3210"; - NamedBean own = new NamedBean("user" + t, secret); - pool.submit(() -> { - await(start); - for (int i = 0; i < iterations; i++) { - try { - String response = sharedWriter.write(own); - if (!response.contains(secret)) { - // this thread's own response doesn't even contain its own data: - // the shared buffer was corrupted or overwritten by another thread - corrupted.incrementAndGet(); - } - } catch (JSONException ignored) { - // a failed serialize under contention is not itself the thing measured here - } - } - }); - } - - start.countDown(); - pool.shutdown(); - assertTrue(pool.awaitTermination(120, TimeUnit.SECONDS)); - - assertEquals("a concurrently-written response must never overwrite or corrupt this " + - "thread's own response", 0, corrupted.get()); - } - - private static void await(CountDownLatch latch) { - try { - latch.await(); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } - } - }