g0w6y opened a new pull request, #1776: URL: https://github.com/apache/struts/pull/1776
## Summary `JSONUtil` obtains its `JSONWriter` once via `@Inject` and reuses that same instance across every concurrent response handled by that `JSONResult`/`JSONInterceptor` configuration -- this is how every Struts interceptor's injected dependencies work, not something specific to this plugin. `StrutsJSONWriter` kept its output buffer, cyclic-reference-detection stack, root object, and expression-path state (`buf`, `stack`, `root`, `buildExpr`, `exprStack`, `excludeProperties`, `includeProperties`, `excludeNullProperties`) as **plain instance fields**, all reset in place at the start of `write()`: ```java this.buf.setLength(0); this.stack.clear(); this.root = object; ... this.value(object, null); return this.buf.toString(); ``` ## Root cause Two concurrent `write()` calls on the same writer instance race on that reset. `buf.setLength(0)` at the top of one call can wipe out another, unrelated, concurrently-in-progress call's already-appended output; both calls then keep appending their own serialized fragments into the same, shared `StringBuilder`. Because each call finishes by reading back whatever the buffer currently contains (`this.buf.toString()`), **one request's `write()` call can return a different, concurrently-served request's fully serialized response body** -- not just a corrupted mix, but the other request's complete JSON, verbatim. This is the same class of defect as #1775 (`StrutsJSONReader` sharing per-parse state across concurrent request bodies), but on the response side, which is exercised by the default `json` result type on every request with no opt-in configuration required. Verified with a live, quantified reproduction before writing the fix: two threads sharing one `StrutsJSONWriter` instance, one repeatedly serializing a "victim" object containing a unique secret marker, the other repeatedly serializing an unrelated "attacker" object and inspecting its own returned string. Out of 50,000 attempts, the attacker's own `write()` call returned the victim's complete secret value 180 times, plus a partial fragment of it 612 more times. ## Fix Move `buf`/`stack`/`root`/`buildExpr`/`exprStack`/`excludeProperties`/`includeProperties`/`excludeNullProperties` into a `WriteState` object confined to a `ThreadLocal`, created fresh in `write(...)` and cleared in a `finally` block. All method signatures and control flow are otherwise unchanged, so existing `StrutsJSONWriter` subclasses (`bean()`/`map()`/`array()`/`string()`/`add()`/etc. are all `protected` extension points) continue to work unmodified -- the per-write-state fields were already `private`, so no external code could have been touching them directly. `ignoreHierarchy`/`dateFormat`/`enumAsBean`/`excludeProxyProperties` stay as ordinary instance fields, since they're set to the same value on every call for a given writer configuration and are safe to share across threads. ## Test plan - [x] New regression test `testConcurrentReuseDoesNotSwapResponsesAcrossWrites` in `StrutsJSONWriterTest`: two threads share one writer instance, a "victim" thread repeatedly serializes an object containing a unique secret marker while an "attacker" thread concurrently serializes an unrelated object and inspects its own returned string. Asserts the attacker's response never contains the victim's secret. - [x] Full existing `StrutsJSONWriterTest` suite passes unchanged -- confirms no behavioral regression from the refactor. - [x] Full `struts2-json-plugin` module test suite passes (126/126). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
