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 cc00343f1 WW-5650 Obtain a fresh JSON reader/writer per request in
JSONInterceptor (#1782)
cc00343f1 is described below
commit cc00343f1b4ae763f3403f5678a9fdebb4c73665
Author: Lukasz Lenart <[email protected]>
AuthorDate: Sun Jul 19 20:25:47 2026 +0200
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 <[email protected]>
* 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 <[email protected]>
---------
Co-authored-by: Claude Opus 4.8 <[email protected]>
---
.../org/apache/struts2/json/JSONInterceptor.java | 19 ++-
.../org/apache/struts2/json/StrutsJSONReader.java | 134 +++++++++------------
.../org/apache/struts2/json/StrutsJSONWriter.java | 113 ++++++++---------
.../apache/struts2/json/JSONInterceptorTest.java | 51 +++++---
.../apache/struts2/json/StrutsJSONReaderTest.java | 80 ------------
.../apache/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.
* </p>
- *
* <p>
- * 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 <strong>not
thread-safe</strong>;
+ * obtain a fresh instance per parse (the container serves it as a prototype
bean). See WW-5650.
* </p>
*/
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<ParseState> 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<String, Object> 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<String, Object> 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<Object> 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<Object> 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.
* </p>
- *
* <p>
- * 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 <strong>not
thread-safe</strong>;
+ * obtain a fresh instance per write (the container serves it as a prototype
bean). See WW-5650.
* </p>
*/
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<Object> stack = new ArrayDeque<>();
- private Object root;
- private boolean buildExpr = true;
- private String exprStack = "";
- private Collection<Pattern> excludeProperties;
- private Collection<Pattern> includeProperties;
- private boolean excludeNullProperties;
- }
-
- private static final ThreadLocal<WriteState> WRITE_STATE = new
ThreadLocal<>();
-
+ private final StringBuilder buf = new StringBuilder();
+ private final Deque<Object> stack = new ArrayDeque<>();
private boolean ignoreHierarchy = true;
+ private Object root;
+ private boolean buildExpr = true;
+ private String exprStack = "";
+ private Collection<Pattern> excludeProperties;
+ private Collection<Pattern> 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<Pattern> excludeProperties,
Collection<Pattern> 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<Object, Object> 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.
- *
- * <p>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.</p>
- */
- @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.
- *
- * <p>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.</p>
- */
- @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();
- }
- }
-
}
