From 6d271329262bd82db732ee3b4facd053b8407dc0 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Tue, 21 Jul 2026 10:59:38 +0200 Subject: [PATCH] WW-4858 Evaluate JSON name allowlist at leaf keys only (#1784) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * 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 * 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 --------- Co-authored-by: Claude Opus 4.8 --- .../apache/struts2/json/JSONInterceptor.java | 63 ++++++++++--- .../struts2/json/JSONInterceptorTest.java | 94 ++++++++++++++++++- .../json/ParameterAwareTestAction.java | 13 ++- 3 files changed, 149 insertions(+), 21 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 804542399..9386a618d 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 @@ -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) { 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 4aa27563a..32853534a 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 @@ -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"); diff --git a/plugins/json/src/test/java/org/apache/struts2/json/ParameterAwareTestAction.java b/plugins/json/src/test/java/org/apache/struts2/json/ParameterAwareTestAction.java index d71287cd9..eeb953ac2 100644 --- a/plugins/json/src/test/java/org/apache/struts2/json/ParameterAwareTestAction.java +++ b/plugins/json/src/test/java/org/apache/struts2/json/ParameterAwareTestAction.java @@ -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