mirror of
https://github.com/apache/struts.git
synced 2026-08-05 14:47:09 +00:00
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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Lukasz Lenart
@@ -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
|
||||
|
||||
@@ -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;
|
||||
};
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user