Apache/struts
1
0
Fork 0
mirror of https://github.com/apache/struts.git synced 2026-08-05 22:56:59 +00:00
Code Issues Packages Projects Releases Wiki Activity

WW-4858 Evaluate JSON name allowlist at leaf keys only (#1784)

Browse Source 
* WW-4858 fix(json): evaluate name allowlist at leaf keys only

The JSON population filter walked the object tree and applied every name
check at every node before recursing. Accepted name patterns and the
ParameterNameAware callback target the full dotted binding path, so gating
an intermediate node (e.g. "bean") against a leaf-specific rule dropped the
entire subtree before the leaf ("bean.stringField") was ever evaluated —
diverging from ParametersInterceptor, which only evaluates complete leaf
names. For arrays it also meant the accepted allowlist judged the container
name instead of the element path.

Split the per-key gate: length, excluded patterns, @StrutsParameter
authorization and property filters stay per-node (exclusion is prefix-safe
and authorization is intentionally hierarchical); accepted patterns and
ParameterNameAware move to leaf keys only, including scalar array elements
at their indexed path ("items[0]"). This reproduces the flat-path semantics
exactly. Excluded/include-property behavior is unchanged.

Tests: nested-object leaf populates under a leaf-targeting accepted pattern
and a ParameterNameAware action that rejects the intermediate node; accepted
patterns now apply to the array element path; nested include-property
filtering still works.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* WW-4858 fix(json): apply per-node checks to scalar array elements

Scalar list elements were gated only by the leaf name-allowlist and value
checks, skipping the per-node checks (length, excluded patterns,
@StrutsParameter authorization, property filters). That left the JSON path
more permissive than ParametersInterceptor, which evaluates all of these
against the full indexed name "items[0]".

Apply isAcceptableNode(elementPrefix, ...) to scalar list elements so an
element is gated exactly as the flat path gates "items[0]". Note this makes
scalar-list @StrutsParameter authorization use the element path (depth 1,
read method) rather than only the container (depth 0), matching the flat
path.

Tests: excluded name pattern and @StrutsParameter authorization now apply at
the list element path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* WW-4858 refactor(json): extract keyTypeName helper to lower cognitive complexity

Move the non-String-key logging ternary out of filterUnacceptableKeysRecursive
into a keyTypeName helper. Pure extraction, no behavior change; drops the
method's cognitive complexity from 17 to 14, under Sonar's S3776 threshold.

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
2026-07-21 10:59:38 +02:00
committed by GitHub
parent f2c1f50da2
commit 6d27132926
3 changed files with 149 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files
plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java
+48 -15
View File
@@ -233,17 +233,24 @@ public class JSONInterceptor extends AbstractInterceptor {
// Defensive: a custom JSONReader could produce non-String keys. Skip — we cannot
// construct a parameter path for filtering, and JSONPopulator wouldn't bind these anyway.
LOG.debug("Skipping JSON entry with non-String key [{}] of type [{}] under prefix [{}]",
entry.getKey(), entry.getKey() == null ? "null" : entry.getKey().getClass().getName(), prefix);
entry.getKey(), keyTypeName(entry.getKey()), prefix);
continue;
}
String fullPath = prefix.isEmpty() ? key : prefix + "." + key;
if (!isAcceptableKey(fullPath, target, action)) {
Object value = entry.getValue();
boolean leaf = !(value instanceof Map) && !(value instanceof java.util.List);
// Per-node checks (length, excluded patterns, @StrutsParameter authorization, property
// filters) apply to every node. Name-allowlist checks (accepted patterns,
// ParameterNameAware) apply only at leaf keys, so an intermediate node is never gated by
// a pattern written for a leaf path — mirroring ParametersInterceptor, which only ever
// evaluates complete leaf names.
if (!isAcceptableNode(fullPath, target, action) || (leaf && !isAcceptableLeafName(fullPath, action))) {
it.remove();
continue;
}
Object value = entry.getValue();
if (value instanceof Map) {
filterUnacceptableKeysRecursive((Map) value, fullPath, target, action);
} else if (value instanceof java.util.List) {
@@ -254,6 +261,10 @@ public class JSONInterceptor extends AbstractInterceptor {
}
}
private static String keyTypeName(Object key) {
return key == null ? "null" : key.getClass().getName();
}
@SuppressWarnings("rawtypes")
private void filterUnacceptableList(java.util.List list, String prefix, Object target, Object action) {
// Use prefix+"[0]" so list element properties pick up one extra '[' in their path,
@@ -266,15 +277,25 @@ public class JSONInterceptor extends AbstractInterceptor {
filterUnacceptableKeysRecursive((Map) item, elementPrefix, target, action);
} else if (item instanceof java.util.List) {
filterUnacceptableList((java.util.List) item, elementPrefix, target, action);
// Scalar list elements are value-checked only; their parent key already passed name/authorization checks.
} else if (!isAcceptableValue(elementPrefix, item, action)) {
// Scalar list elements are full leaf binding targets: apply the per-node checks, the leaf
// name-allowlist checks and the value checks at the element path (e.g. "items[0]"), so a
// scalar element is gated exactly as ParametersInterceptor gates the flat name "items[0]".
} else if (!isAcceptableNode(elementPrefix, target, action)
|| !isAcceptableLeafName(elementPrefix, action)
|| !isAcceptableValue(elementPrefix, item, action)) {
it.remove();
}
}
}
@SuppressWarnings("rawtypes")
private boolean isAcceptableKey(String fullPath, Object target, Object action) {
/**
* Checks applied to every node of the JSON tree (both intermediate objects/arrays and leaves):
* key length, excluded name patterns, {@code @StrutsParameter} authorization, and — when
* enabled — the interceptor's own property filters. Excluded patterns are prefix-safe and
* authorization is intentionally hierarchical (a parent must be authorized for a child to be
* reachable), so these are correct to evaluate at intermediate nodes.
*/
private boolean isAcceptableNode(String fullPath, Object target, Object action) {
if (fullPath.length() > paramNameMaxLength) {
LOG.warn("JSON body parameter [{}] is too long, allowed length is [{}]; rejected", fullPath, paramNameMaxLength);
return false;
@@ -283,19 +304,11 @@ public class JSONInterceptor extends AbstractInterceptor {
LOG.warn("JSON body parameter [{}] matches an excluded pattern; rejected", fullPath);
return false;
}
if (acceptedPatterns != null && !acceptedPatterns.isAccepted(fullPath).isAccepted()) {
LOG.warn("JSON body parameter [{}] does not match any accepted pattern; rejected", fullPath);
return false;
}
if (!parameterAuthorizer.isAuthorized(fullPath, target, action)) {
LOG.warn("JSON body parameter [{}] rejected by @StrutsParameter authorization on [{}]",
fullPath, target.getClass().getName());
return false;
}
if (action instanceof ParameterNameAware nameAware && !nameAware.acceptableParameterName(fullPath)) {
LOG.debug("JSON body parameter [{}] rejected by ParameterNameAware action", fullPath);
return false;
}
if (applyPropertyFiltersToInput && !isAcceptedByPropertyFilters(fullPath)) {
LOG.debug("JSON body parameter [{}] rejected by excludeProperties/includeProperties on input", fullPath);
return false;
@@ -303,6 +316,26 @@ public class JSONInterceptor extends AbstractInterceptor {
return true;
}
/**
* Name-allowlist checks applied only at leaf keys (scalar values, including scalar array
* elements): accepted name patterns and the {@link ParameterNameAware} action callback. These
* are deliberately not applied to intermediate nodes: their patterns/decisions target the full
* dotted binding path, and gating an intermediate node against a leaf-specific rule would drop
* the whole subtree before the leaf is ever evaluated. This matches ParametersInterceptor, which
* only ever evaluates complete leaf names.
*/
private boolean isAcceptableLeafName(String fullPath, Object action) {
if (acceptedPatterns != null && !acceptedPatterns.isAccepted(fullPath).isAccepted()) {
LOG.warn("JSON body parameter [{}] does not match any accepted pattern; rejected", fullPath);
return false;
}
if (action instanceof ParameterNameAware nameAware && !nameAware.acceptableParameterName(fullPath)) {
LOG.debug("JSON body parameter [{}] rejected by ParameterNameAware action", fullPath);
return false;
}
return true;
}
private boolean isAcceptedByPropertyFilters(String fullPath) {
if (excludeProperties != null) {
for (Pattern pattern : excludeProperties) {
plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java
+89 -5
View File
@@ -762,11 +762,11 @@ public class JSONInterceptorTest extends StrutsTestCase {
JSONInterceptor interceptor = createInterceptor();
org.apache.struts2.security.DefaultAcceptedPatternsChecker accepted =
new org.apache.struts2.security.DefaultAcceptedPatternsChecker();
// Accept the intermediate node "bean" and the nested leaf "bean.stringField", but not
// "bean.intField". The intermediate node must itself match an accepted pattern, otherwise
// the whole subtree is dropped before the leaf is ever visited (accepted name patterns are
// raw full-match regexes with no hierarchy expansion, unlike include patterns).
accepted.setAcceptedPatterns("bean(\\.stringField)?");
// Leaf-targeting pattern: matches only the nested leaf "bean.stringField", not the
// intermediate node "bean". Accepted patterns are evaluated at leaf keys, so the
// intermediate node is not gated and the matching leaf still populates while the
// non-matching sibling "bean.intField" is dropped.
accepted.setAcceptedPatterns("bean\\.stringField");
interceptor.setAcceptedPatterns(accepted);
TestAction action = new TestAction();
@@ -780,6 +780,90 @@ public class JSONInterceptorTest extends StrutsTestCase {
assertEquals(0, action.getBean().getIntField());
}
public void testAcceptedNamePatternAppliesToListElementPath() throws Exception {
this.request.setContent("{\"list\": [\"x\", \"y\"]}".getBytes());
this.request.addHeader("Content-Type", "application/json");
JSONInterceptor interceptor = createInterceptor();
org.apache.struts2.security.DefaultAcceptedPatternsChecker accepted =
new org.apache.struts2.security.DefaultAcceptedPatternsChecker();
// Matches the container name "list" but not the element path "list[0]". Because accepted
// patterns are evaluated at the leaf (the scalar element path), the elements are rejected
// even though the container name matches — mirroring the flat ParametersInterceptor path,
// which would evaluate "list[0]" and reject it.
accepted.setAcceptedPatterns("list");
interceptor.setAcceptedPatterns(accepted);
TestAction action = new TestAction();
this.invocation.setAction(action);
this.invocation.getStack().push(action);
interceptor.intercept(this.invocation);
assertTrue(action.getList() == null || action.getList().isEmpty());
}
public void testExcludedNamePatternAppliesToListElementPath() throws Exception {
this.request.setContent("{\"list\": [\"x\", \"y\"]}".getBytes());
this.request.addHeader("Content-Type", "application/json");
JSONInterceptor interceptor = createInterceptor();
org.apache.struts2.security.DefaultExcludedPatternsChecker excluded =
new org.apache.struts2.security.DefaultExcludedPatternsChecker();
// Excludes the element path "list[0]" but not the container "list". Scalar list elements are
// full leaf binding targets, so the per-node checks (here, excluded patterns) are evaluated
// at the element path — mirroring the flat ParametersInterceptor path, which would exclude
// "list[0]".
excluded.setExcludedPatterns("list\\[0\\]");
interceptor.setExcludedPatterns(excluded);
TestAction action = new TestAction();
this.invocation.setAction(action);
this.invocation.getStack().push(action);
interceptor.intercept(this.invocation);
assertTrue(action.getList() == null || action.getList().isEmpty());
}
public void testAuthorizationAppliesToListElementPath() throws Exception {
this.request.setContent("{\"list\": [\"x\", \"y\"]}".getBytes());
this.request.addHeader("Content-Type", "application/json");
JSONInterceptor interceptor = createInterceptor();
// Authorize the container "list" but reject the element path "list[0]". Scalar list elements
// now pass through @StrutsParameter authorization at their element path, so the elements are
// rejected even though the container is authorized.
interceptor.setParameterAuthorizer((parameterName, target, action) -> !"list[0]".equals(parameterName));
TestAction action = new TestAction();
this.invocation.setAction(action);
this.invocation.getStack().push(action);
interceptor.intercept(this.invocation);
assertTrue(action.getList() == null || action.getList().isEmpty());
}
public void testParameterNameAwareDoesNotRejectIntermediateNode() throws Exception {
this.request.setContent("{\"bean\": {\"stringField\": \"keep\", \"intField\": 42}}".getBytes());
this.request.addHeader("Content-Type", "application/json");
JSONInterceptor interceptor = createInterceptor();
// The action's acceptableParameterName rejects the intermediate node "bean" but accepts the
// nested leaves. ParameterNameAware is evaluated at leaf keys, so the subtree is not dropped.
ParameterAwareTestAction action = new ParameterAwareTestAction();
this.invocation.setAction(action);
this.invocation.getStack().push(action);
interceptor.intercept(this.invocation);
assertNotNull(action.getBean());
assertEquals("keep", action.getBean().getStringField());
assertEquals(42, action.getBean().getIntField());
}
public void testExcludedValuePatternRejectsListElement() throws Exception {
this.request.setContent("{\"list\": [\"good\", \"badvalue\"]}".getBytes());
this.request.addHeader("Content-Type", "application/json");
plugins/json/src/test/java/org/apache/struts2/json/ParameterAwareTestAction.java
+12 -1
View File
@@ -26,6 +26,7 @@ public class ParameterAwareTestAction implements ParameterNameAware, ParameterVa
private String foo;
private String bar;
private String baz;
private Bean bean;
public String getFoo() {
return foo;
@@ -51,9 +52,19 @@ public class ParameterAwareTestAction implements ParameterNameAware, ParameterVa
this.baz = baz;
}
public Bean getBean() {
return bean;
}
public void setBean(Bean bean) {
this.bean = bean;
}
@Override
public boolean acceptableParameterName(String parameterName) {
return !"bar".equals(parameterName);
// Reject the flat key "bar" and the intermediate node "bean"; nested leaves such as
// "bean.stringField" are accepted so tests can assert intermediate nodes are not gated.
return !"bar".equals(parameterName) && !"bean".equals(parameterName);
}
@Override