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 b1c1723888..25ff796a36 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 @@ -230,17 +230,24 @@ private void filterUnacceptableKeysRecursive(Map json, String prefix, Object tar // 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) { @@ -251,6 +258,10 @@ private void filterUnacceptableKeysRecursive(Map json, String prefix, Object tar } } + 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, @@ -263,15 +274,25 @@ private void filterUnacceptableList(java.util.List list, String prefix, Object t 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; @@ -280,19 +301,11 @@ private boolean isAcceptableKey(String fullPath, Object target, Object action) { 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; @@ -300,6 +313,26 @@ private boolean isAcceptableKey(String fullPath, Object target, Object action) { 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 eaf4286002..91c13202b7 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 @@ -777,11 +777,11 @@ public void testAcceptedNamePatternRejectsNestedKey() throws Exception { 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(); @@ -795,6 +795,90 @@ public void testAcceptedNamePatternRejectsNestedKey() throws Exception { 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 d71287cd93..eeb953ac2e 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 void setBaz(String baz) { 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