diff --git a/docs/superpowers/plans/2026-07-08-WW-4858-json-parameter-filtering.md b/docs/superpowers/plans/2026-07-08-WW-4858-json-parameter-filtering.md new file mode 100644 index 0000000000..b312092383 --- /dev/null +++ b/docs/superpowers/plans/2026-07-08-WW-4858-json-parameter-filtering.md @@ -0,0 +1,838 @@ +# WW-4858 JSON Parameter Filtering Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Make `JSONInterceptor`'s stack-population path enforce the same name/value acceptability controls as `ParametersInterceptor`, while keeping pure-reflection population (no OGNL). + +**Architecture:** Generalize the existing single tree-walk `filterUnauthorizedKeysRecursive()` into `filterUnacceptableKeysRecursive()`. Each new check hooks in at the same per-key visit point, using dotted paths (`address.city`, `items[0].name`). The two name-pattern checkers are injected as the same global singletons `ParametersInterceptor` uses; other controls are interceptor-local settings. `ParametersInterceptor` is not modified. + +**Tech Stack:** Java, Maven multi-module (`plugins/json` depends on `core`), JSON plugin. Tests are JUnit 4 style via `StrutsTestCase` (methods named `testXxx`, `setUp()` override, `assertEquals`/`assertNull`/`assertNotNull`). + +## Global Constraints + +- Commit messages MUST be prefixed with `WW-4858` and follow `WW-4858 (json): `. +- Every commit ends with the footer line: `Co-Authored-By: Claude Opus 4.8 `. +- Work happens on branch `WW-4858-json-parameter-filtering` (already created). Never commit to `main`. +- Test class is JUnit 4 style (`extends StrutsTestCase`); do NOT add JUnit 5 `@Test` annotations. Name tests `testXxx`. +- Run a single test with: `mvn test -pl plugins/json -am -DskipAssembly -Dtest=JSONInterceptorTest#` +- Pure-reflection population must remain unchanged for accepted input; no OGNL evaluation may be introduced. +- The two pattern checkers (`excludedPatterns`, `acceptedPatterns`) are null-guarded in production code: a `null` checker means "skip this check". Production wires them via `@Inject`; the guard exists so existing tests that build a bare `new JSONInterceptor()` still pass. + +--- + +## File Structure + +- Modify: `plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java` — inject checkers, add settings, generalize the recursive filter to apply all name/value checks. +- Modify: `plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java` — wire default checkers into `createInterceptor()`, update one reflective test to the renamed method, add one test per gap. +- Create: `plugins/json/src/test/java/org/apache/struts2/json/ParameterAwareTestAction.java` — test fixture implementing `ParameterNameAware` + `ParameterValueAware`. + +--- + +## Task 1: Inject checkers + generalize filter to enforce excluded/accepted name patterns + +**Files:** +- Modify: `plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java` +- Modify: `plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java` + +**Interfaces:** +- Consumes: `ExcludedPatternsChecker.isExcluded(String).isExcluded()`, `AcceptedPatternsChecker.isAccepted(String).isAccepted()`, existing `ParameterAuthorizer.isAuthorized(String, Object, Object)`. +- Produces: renamed methods `filterUnacceptableKeys(Map, Object, Object)`, `filterUnacceptableKeysRecursive(Map, String, Object, Object)`, `filterUnacceptableList(List, String, Object, Object)`; new method `boolean isAcceptableKey(String fullPath, Object target, Object action)`; injected fields `excludedPatterns`, `acceptedPatterns`. + +- [ ] **Step 1: Write the failing tests** + +Add to `JSONInterceptorTest.java` (before the final `setUp()` override): + +```java + public void testExcludedNamePatternRejectsKey() throws Exception { + this.request.setContent("{\"foo\":\"a\", \"bar\":\"b\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + org.apache.struts2.security.DefaultExcludedPatternsChecker excluded = + new org.apache.struts2.security.DefaultExcludedPatternsChecker(); + excluded.setExcludedPatterns("bar"); + interceptor.setExcludedPatterns(excluded); + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertEquals("a", action.getFoo()); + assertNull(action.getBar()); + } + + public void testAcceptedNamePatternRejectsKey() throws Exception { + this.request.setContent("{\"foo\":\"a\", \"bar\":\"b\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + org.apache.struts2.security.DefaultAcceptedPatternsChecker accepted = + new org.apache.struts2.security.DefaultAcceptedPatternsChecker(); + accepted.setAcceptedPatterns("foo"); + interceptor.setAcceptedPatterns(accepted); + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertEquals("a", action.getFoo()); + assertNull(action.getBar()); + } +``` + +Update `createInterceptor()` to wire default checkers so the happy path exercises real checkers. Replace the existing method body with: + +```java + private JSONInterceptor createInterceptor() { + JSONInterceptor interceptor = new JSONInterceptor(); + JSONUtil jsonUtil = new JSONUtil(); + jsonUtil.setReader(new StrutsJSONReader()); + jsonUtil.setWriter(new StrutsJSONWriter()); + interceptor.setJsonUtil(jsonUtil); + // Default: allow all parameters (simulates requireAnnotations=false) + interceptor.setParameterAuthorizer((parameterName, target, action) -> true); + interceptor.setExcludedPatterns(new org.apache.struts2.security.DefaultExcludedPatternsChecker()); + interceptor.setAcceptedPatterns(new org.apache.struts2.security.DefaultAcceptedPatternsChecker()); + return interceptor; + } +``` + +Update the reflective lookup in `testNonStringKeysAreSkippedByAuthorizationFilter` from the old method name to the new one: + +```java + java.lang.reflect.Method method = JSONInterceptor.class.getDeclaredMethod( + "filterUnacceptableKeys", java.util.Map.class, Object.class, Object.class); +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `mvn test -pl plugins/json -am -DskipAssembly -Dtest=JSONInterceptorTest#testExcludedNamePatternRejectsKey+testAcceptedNamePatternRejectsKey` +Expected: FAIL — `setExcludedPatterns`/`setAcceptedPatterns` do not exist on `JSONInterceptor` (compile error). + +- [ ] **Step 3: Implement in `JSONInterceptor.java`** + +Add imports after the existing `org.apache.struts2.interceptor.parameter.ParameterAuthorizer` import: + +```java +import org.apache.struts2.security.AcceptedPatternsChecker; +import org.apache.struts2.security.ExcludedPatternsChecker; +``` + +Add fields after the existing `private ParameterAuthorizer parameterAuthorizer;` field: + +```java + private ExcludedPatternsChecker excludedPatterns; + private AcceptedPatternsChecker acceptedPatterns; +``` + +Add injected setters next to `setParameterAuthorizer`: + +```java + @Inject + public void setExcludedPatterns(ExcludedPatternsChecker excludedPatterns) { + this.excludedPatterns = excludedPatterns; + } + + @Inject + public void setAcceptedPatterns(AcceptedPatternsChecker acceptedPatterns) { + this.acceptedPatterns = acceptedPatterns; + } +``` + +Rename the call site at the `filterUnauthorizedKeys(json, rootObject, invocation.getAction());` line to: + +```java + filterUnacceptableKeys(json, rootObject, invocation.getAction()); +``` + +Replace the three methods `filterUnauthorizedKeys`, `filterUnauthorizedKeysRecursive`, `filterUnauthorizedList` with the generalized versions below (the leaf-value branch is a no-op placeholder here and is filled in by Task 3/4): + +```java + @SuppressWarnings("rawtypes") + private void filterUnacceptableKeys(Map json, Object target, Object action) { + filterUnacceptableKeysRecursive(json, "", target, action); + } + + @SuppressWarnings("rawtypes") + private void filterUnacceptableKeysRecursive(Map json, String prefix, Object target, Object action) { + Iterator it = json.entrySet().iterator(); + while (it.hasNext()) { + Map.Entry entry = it.next(); + if (!(entry.getKey() instanceof String key)) { + // 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); + continue; + } + String fullPath = prefix.isEmpty() ? key : prefix + "." + key; + + if (!isAcceptableKey(fullPath, target, 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) { + filterUnacceptableList((java.util.List) value, fullPath, target, action); + } + } + } + + @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, + // matching the indexed-path semantics of ParametersInterceptor (e.g. "items[0].key"). + String elementPrefix = prefix + "[0]"; + for (Object item : list) { + if (item instanceof Map) { + filterUnacceptableKeysRecursive((Map) item, elementPrefix, target, action); + } else if (item instanceof java.util.List) { + filterUnacceptableList((java.util.List) item, elementPrefix, target, action); + } + } + } + + @SuppressWarnings("rawtypes") + private boolean isAcceptableKey(String fullPath, Object target, Object action) { + if (excludedPatterns != null && excludedPatterns.isExcluded(fullPath).isExcluded()) { + 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; + } + return true; + } +``` + +- [ ] **Step 4: Run the full test class to verify pass and no regressions** + +Run: `mvn test -pl plugins/json -am -DskipAssembly -Dtest=JSONInterceptorTest` +Expected: PASS (all tests, including the existing authorization tests, green). + +- [ ] **Step 5: Commit** + +```bash +git add plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java +git commit -m "WW-4858 feat(json): enforce excluded/accepted name patterns on JSON population + +Co-Authored-By: Claude Opus 4.8 " +``` + +--- + +## Task 2: Enforce param-name max length + +**Files:** +- Modify: `plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java` +- Modify: `plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java` + +**Interfaces:** +- Consumes: `isAcceptableKey` (Task 1). +- Produces: field `int paramNameMaxLength = 100`; setter `void setParamNameMaxLength(int)`. + +- [ ] **Step 1: Write the failing test** + +Add to `JSONInterceptorTest.java`: + +```java + public void testParamNameMaxLengthRejectsLongKey() throws Exception { + this.request.setContent("{\"foo\":\"a\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + interceptor.setParamNameMaxLength(2); // "foo" is length 3, over the limit + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertNull(action.getFoo()); + } +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `mvn test -pl plugins/json -am -DskipAssembly -Dtest=JSONInterceptorTest#testParamNameMaxLengthRejectsLongKey` +Expected: FAIL — `setParamNameMaxLength` does not exist (compile error). + +- [ ] **Step 3: Implement in `JSONInterceptor.java`** + +Add field next to `excludedPatterns`/`acceptedPatterns`: + +```java + private int paramNameMaxLength = 100; +``` + +Add setter next to the checker setters: + +```java + /** + * If the dotted JSON key path exceeds the configured maximum length it will not be accepted. + * + * @param paramNameMaxLength maximum length of a JSON key path + */ + public void setParamNameMaxLength(int paramNameMaxLength) { + this.paramNameMaxLength = paramNameMaxLength; + } +``` + +Add the length check as the FIRST check inside `isAcceptableKey`, before the excluded-pattern check: + +```java + if (fullPath.length() > paramNameMaxLength) { + LOG.warn("JSON body parameter [{}] is too long, allowed length is [{}]; rejected", fullPath, paramNameMaxLength); + return false; + } +``` + +- [ ] **Step 4: Run test to verify it passes** + +Run: `mvn test -pl plugins/json -am -DskipAssembly -Dtest=JSONInterceptorTest#testParamNameMaxLengthRejectsLongKey` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java +git commit -m "WW-4858 feat(json): enforce param-name max length on JSON population + +Co-Authored-By: Claude Opus 4.8 " +``` + +--- + +## Task 3: Honor ParameterNameAware and ParameterValueAware + +**Files:** +- Create: `plugins/json/src/test/java/org/apache/struts2/json/ParameterAwareTestAction.java` +- Modify: `plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java` +- Modify: `plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java` + +**Interfaces:** +- Consumes: `isAcceptableKey` (Task 1), `filterUnacceptableKeysRecursive`/`filterUnacceptableList` (Task 1), `org.apache.struts2.action.ParameterNameAware.acceptableParameterName(String)`, `org.apache.struts2.action.ParameterValueAware.acceptableParameterValue(String)`. +- Produces: new method `boolean isAcceptableValue(String fullPath, Object value, Object action)`; leaf-value handling wired into `filterUnacceptableKeysRecursive` and `filterUnacceptableList`. + +- [ ] **Step 1: Write the failing tests** + +Create `plugins/json/src/test/java/org/apache/struts2/json/ParameterAwareTestAction.java`: + +```java +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.json; + +import org.apache.struts2.action.ParameterNameAware; +import org.apache.struts2.action.ParameterValueAware; + +public class ParameterAwareTestAction implements ParameterNameAware, ParameterValueAware { + + private String foo; + private String bar; + private String baz; + + public String getFoo() { + return foo; + } + + public void setFoo(String foo) { + this.foo = foo; + } + + public String getBar() { + return bar; + } + + public void setBar(String bar) { + this.bar = bar; + } + + public String getBaz() { + return baz; + } + + public void setBaz(String baz) { + this.baz = baz; + } + + @Override + public boolean acceptableParameterName(String parameterName) { + return !"bar".equals(parameterName); + } + + @Override + public boolean acceptableParameterValue(String parameterValue) { + return !"blocked".equals(parameterValue); + } +} +``` + +Add to `JSONInterceptorTest.java`: + +```java + public void testParameterNameAwareRejectsKey() throws Exception { + this.request.setContent("{\"foo\":\"a\", \"bar\":\"b\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + ParameterAwareTestAction action = new ParameterAwareTestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertEquals("a", action.getFoo()); + assertNull(action.getBar()); + } + + public void testParameterValueAwareRejectsValue() throws Exception { + this.request.setContent("{\"foo\":\"blocked\", \"baz\":\"good\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + ParameterAwareTestAction action = new ParameterAwareTestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertNull(action.getFoo()); + assertEquals("good", action.getBaz()); + } +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `mvn test -pl plugins/json -am -DskipAssembly -Dtest=JSONInterceptorTest#testParameterNameAwareRejectsKey+testParameterValueAwareRejectsValue` +Expected: `testParameterNameAwareRejectsKey` FAILS (bar is populated because `ParameterNameAware` is not honored); `testParameterValueAwareRejectsValue` FAILS (foo is populated because `ParameterValueAware` is not honored). + +- [ ] **Step 3: Implement in `JSONInterceptor.java`** + +Add import next to the security-checker imports: + +```java +import org.apache.struts2.action.ParameterNameAware; +import org.apache.struts2.action.ParameterValueAware; +``` + +Add the `ParameterNameAware` check to `isAcceptableKey`, as the LAST check before `return true;`: + +```java + if (action instanceof ParameterNameAware nameAware && !nameAware.acceptableParameterName(fullPath)) { + LOG.debug("JSON body parameter [{}] rejected by ParameterNameAware action", fullPath); + return false; + } +``` + +Add the new leaf-value method after `isAcceptableKey`: + +```java + private boolean isAcceptableValue(String fullPath, Object value, Object action) { + String stringValue = value == null ? null : String.valueOf(value); + if (action instanceof ParameterValueAware valueAware && !valueAware.acceptableParameterValue(stringValue)) { + LOG.debug("JSON body value for parameter [{}] rejected by ParameterValueAware action", fullPath); + return false; + } + return true; + } +``` + +Wire leaf-value handling into `filterUnacceptableKeysRecursive` by adding an `else` branch to the value dispatch (after the `List` branch): + +```java + if (value instanceof Map) { + filterUnacceptableKeysRecursive((Map) value, fullPath, target, action); + } else if (value instanceof java.util.List) { + filterUnacceptableList((java.util.List) value, fullPath, target, action); + } else if (!isAcceptableValue(fullPath, value, action)) { + it.remove(); + } +``` + +Wire scalar-element value handling into `filterUnacceptableList` by converting the for-loop to an iterator and adding a scalar branch: + +```java + @SuppressWarnings("rawtypes") + private void filterUnacceptableList(java.util.List list, String prefix, Object target, Object action) { + String elementPrefix = prefix + "[0]"; + Iterator it = list.iterator(); + while (it.hasNext()) { + Object item = it.next(); + if (item instanceof Map) { + filterUnacceptableKeysRecursive((Map) item, elementPrefix, target, action); + } else if (item instanceof java.util.List) { + filterUnacceptableList((java.util.List) item, elementPrefix, target, action); + } else if (!isAcceptableValue(elementPrefix, item, action)) { + it.remove(); + } + } + } +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `mvn test -pl plugins/json -am -DskipAssembly -Dtest=JSONInterceptorTest` +Expected: PASS (whole class green). + +- [ ] **Step 5: Commit** + +```bash +git add plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java plugins/json/src/test/java/org/apache/struts2/json/ParameterAwareTestAction.java +git commit -m "WW-4858 feat(json): honor ParameterNameAware and ParameterValueAware on JSON population + +Co-Authored-By: Claude Opus 4.8 " +``` + +--- + +## Task 4: Opt-in excluded/accepted value patterns + +**Files:** +- Modify: `plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java` +- Modify: `plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java` + +**Interfaces:** +- Consumes: `isAcceptableValue` (Task 3). +- Produces: fields `Set excludedValuePatterns`, `Set acceptedValuePatterns`; setters `void setExcludedValuePatterns(String)`, `void setAcceptedValuePatterns(String)`. + +- [ ] **Step 1: Write the failing tests** + +Add to `JSONInterceptorTest.java`: + +```java + public void testExcludedValuePatternRejectsValue() throws Exception { + this.request.setContent("{\"foo\":\"badvalue\", \"bar\":\"okvalue\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + interceptor.setExcludedValuePatterns("badvalue"); + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertNull(action.getFoo()); + assertEquals("okvalue", action.getBar()); + } + + public void testAcceptedValuePatternRejectsValue() throws Exception { + this.request.setContent("{\"foo\":\"allowed\", \"bar\":\"other\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + interceptor.setAcceptedValuePatterns("allowed"); + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertEquals("allowed", action.getFoo()); + assertNull(action.getBar()); + } +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `mvn test -pl plugins/json -am -DskipAssembly -Dtest=JSONInterceptorTest#testExcludedValuePatternRejectsValue+testAcceptedValuePatternRejectsValue` +Expected: FAIL — `setExcludedValuePatterns`/`setAcceptedValuePatterns` do not exist (compile error). + +- [ ] **Step 3: Implement in `JSONInterceptor.java`** + +Add import next to the other imports: + +```java +import org.apache.struts2.util.TextParseUtil; +``` + +Add fields next to `paramNameMaxLength`: + +```java + private Set excludedValuePatterns; + private Set acceptedValuePatterns; +``` + +Add setters + a private compile helper next to `setParamNameMaxLength`: + +```java + /** + * Sets a comma-delimited list of regular expressions to match JSON leaf values + * that should be removed. Opt-in: no patterns configured means no value filtering. + * + * @param commaDelim comma-delimited regular expressions + */ + public void setExcludedValuePatterns(String commaDelim) { + this.excludedValuePatterns = compileValuePatterns(commaDelim); + } + + /** + * Sets a comma-delimited list of regular expressions; when set, only JSON leaf values + * matching one of them are accepted. Opt-in: no patterns configured means no value filtering. + * + * @param commaDelim comma-delimited regular expressions + */ + public void setAcceptedValuePatterns(String commaDelim) { + this.acceptedValuePatterns = compileValuePatterns(commaDelim); + } + + private static Set compileValuePatterns(String commaDelim) { + Set raw = TextParseUtil.commaDelimitedStringToSet(commaDelim); + Set compiled = new HashSet<>(raw.size()); + for (String pattern : raw) { + compiled.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); + } + return Collections.unmodifiableSet(compiled); + } +``` + +Extend `isAcceptableValue` — after the `ParameterValueAware` check and before `return true;`, add the value-pattern logic (mirrors `ParametersInterceptor`: null/empty always acceptable): + +```java + if (stringValue == null || stringValue.isEmpty()) { + return true; + } + if (isValueExcluded(stringValue)) { + LOG.warn("JSON body value [{}] for parameter [{}] matches an excluded value pattern; rejected", stringValue, fullPath); + return false; + } + if (!isValueAccepted(stringValue)) { + LOG.warn("JSON body value [{}] for parameter [{}] does not match any accepted value pattern; rejected", stringValue, fullPath); + return false; + } +``` + +Add the two predicate helpers after `isAcceptableValue`: + +```java + private boolean isValueExcluded(String value) { + if (excludedValuePatterns == null || excludedValuePatterns.isEmpty()) { + return false; + } + for (Pattern pattern : excludedValuePatterns) { + if (pattern.matcher(value).matches()) { + return true; + } + } + return false; + } + + private boolean isValueAccepted(String value) { + if (acceptedValuePatterns == null || acceptedValuePatterns.isEmpty()) { + return true; + } + for (Pattern pattern : acceptedValuePatterns) { + if (pattern.matcher(value).matches()) { + return true; + } + } + return false; + } +``` + +Note: `Set`, `HashSet`, `Collections`, and `Pattern` are already available (`import java.util.*;` and `import java.util.regex.Pattern;` at the top of the file). + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `mvn test -pl plugins/json -am -DskipAssembly -Dtest=JSONInterceptorTest` +Expected: PASS (whole class green). + +- [ ] **Step 5: Commit** + +```bash +git add plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java +git commit -m "WW-4858 feat(json): add opt-in excluded/accepted value patterns on JSON population + +Co-Authored-By: Claude Opus 4.8 " +``` + +--- + +## Task 5: Opt-in applying excludeProperties/includeProperties to input + +**Files:** +- Modify: `plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java` +- Modify: `plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java` + +**Interfaces:** +- Consumes: `isAcceptableKey` (Task 1); existing fields `excludeProperties` and `includeProperties` (both `List`). +- Produces: field `boolean applyPropertyFiltersToInput = false`; setter `void setApplyPropertyFiltersToInput(boolean)`; private method `boolean isAcceptedByPropertyFilters(String)`. + +- [ ] **Step 1: Write the failing test** + +Add to `JSONInterceptorTest.java`: + +```java + public void testExcludePropertiesAppliedToInputWhenEnabled() throws Exception { + this.request.setContent("{\"foo\":\"a\", \"bar\":\"b\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + interceptor.setApplyPropertyFiltersToInput(true); + interceptor.setExcludeProperties("foo"); + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertNull(action.getFoo()); + assertEquals("b", action.getBar()); + } + + public void testExcludePropertiesNotAppliedToInputByDefault() throws Exception { + this.request.setContent("{\"foo\":\"a\", \"bar\":\"b\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + interceptor.setExcludeProperties("foo"); // configured, but flag off + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertEquals("a", action.getFoo()); + assertEquals("b", action.getBar()); + } +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `mvn test -pl plugins/json -am -DskipAssembly -Dtest=JSONInterceptorTest#testExcludePropertiesAppliedToInputWhenEnabled+testExcludePropertiesNotAppliedToInputByDefault` +Expected: `testExcludePropertiesAppliedToInputWhenEnabled` FAILS to compile — `setApplyPropertyFiltersToInput` does not exist. + +- [ ] **Step 3: Implement in `JSONInterceptor.java`** + +Add field next to `excludedValuePatterns`/`acceptedValuePatterns`: + +```java + private boolean applyPropertyFiltersToInput = false; +``` + +Add setter next to the value-pattern setters: + +```java + /** + * When enabled, the interceptor's {@code excludeProperties}/{@code includeProperties} patterns — + * otherwise used only for serialization output — also gate which JSON keys are populated on input. + * Opt-in; defaults to {@code false} to preserve existing behavior. + * + * @param applyPropertyFiltersToInput true to apply property filters to deserialization + */ + public void setApplyPropertyFiltersToInput(boolean applyPropertyFiltersToInput) { + this.applyPropertyFiltersToInput = applyPropertyFiltersToInput; + } +``` + +Add the property-filter check to `isAcceptableKey`, immediately before `return true;` (after the `ParameterNameAware` check): + +```java + if (applyPropertyFiltersToInput && !isAcceptedByPropertyFilters(fullPath)) { + LOG.debug("JSON body parameter [{}] rejected by excludeProperties/includeProperties on input", fullPath); + return false; + } +``` + +Add the helper after `isAcceptableKey`: + +```java + private boolean isAcceptedByPropertyFilters(String fullPath) { + if (excludeProperties != null) { + for (Pattern pattern : excludeProperties) { + if (pattern.matcher(fullPath).matches()) { + return false; + } + } + } + if (includeProperties != null && !includeProperties.isEmpty()) { + for (Pattern pattern : includeProperties) { + if (pattern.matcher(fullPath).matches()) { + return true; + } + } + return false; + } + return true; + } +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `mvn test -pl plugins/json -am -DskipAssembly -Dtest=JSONInterceptorTest` +Expected: PASS (whole class green). + +- [ ] **Step 5: Commit** + +```bash +git add plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java +git commit -m "WW-4858 feat(json): opt-in applying excludeProperties/includeProperties to JSON input + +Co-Authored-By: Claude Opus 4.8 " +``` + +--- + +## Task 6: Full JSON-plugin regression verification + +**Files:** none (verification only). + +- [ ] **Step 1: Run the full JSON plugin test suite** + +Run: `mvn test -pl plugins/json -am -DskipAssembly` +Expected: BUILD SUCCESS, all JSON plugin tests pass. If any pre-existing population test now fails, treat it as a regression in the traversal changes and fix before proceeding. + +- [ ] **Step 2: Confirm no OGNL was introduced** + +Run: `git diff main -- plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java` +Expected: the diff shows only reflection/pattern-based filtering; no calls to `stack.setValue`, `stack.findValue` for population, or OGNL evaluation were added to the population path. + +- [ ] **Step 3: Verify the branch is clean and ready for PR** + +Run: `git status` and `git log --oneline main..HEAD` +Expected: working tree clean; commits for Tasks 1–5 present, each prefixed `WW-4858`. + +--- + +## Self-Review Notes + +- **Spec coverage:** excluded/accepted name patterns → Task 1; param-name length → Task 2; `ParameterNameAware`/`ParameterValueAware` → Task 3; opt-in value patterns → Task 4; opt-in `excludeProperties`/`includeProperties` on input → Task 5; JSON-RPC left untouched (only the `Content-Type: application/json` `populateObject` path is filtered) — verified by not modifying the RPC branch; list-element scalar value-checking with individual removal → Task 3 (`filterUnacceptableList` iterator). Full regression → Task 6. +- **Rollout posture:** security/app-owned checks (name patterns, length, authorization, `*Aware`) are always-on; value patterns (Task 4) and property filters on input (Task 5) are opt-in and default off. +- **Type consistency:** `isAcceptableKey(String, Object, Object)`, `isAcceptableValue(String, Object, Object)`, and the renamed `filterUnacceptable*` methods are defined in Task 1/Task 3 and reused with identical signatures in later tasks. diff --git a/docs/superpowers/specs/2026-07-08-WW-4858-json-parameter-filtering-design.md b/docs/superpowers/specs/2026-07-08-WW-4858-json-parameter-filtering-design.md new file mode 100644 index 0000000000..e337fc1191 --- /dev/null +++ b/docs/superpowers/specs/2026-07-08-WW-4858-json-parameter-filtering-design.md @@ -0,0 +1,145 @@ +# WW-4858 — Honor parameter filtering during JSON population + +- **Jira:** [WW-4858](https://issues.apache.org/jira/browse/WW-4858) +- **Date:** 2026-07-08 +- **Status:** Design approved +- **Component:** Plugin - JSON + +## Problem + +`JSONInterceptor` populates action/model properties directly through `JSONPopulator` +(pure Java reflection over bean setters). This path bypasses most of the name/value +acceptability controls that `ParametersInterceptor` applies to ordinary HTTP +parameters. WW-5624 already retrofitted `@StrutsParameter` authorization onto the JSON +path (via the shared `ParameterAuthorizer` and `filterUnauthorizedKeys()`), but the +remaining consistency gaps persist. + +Gaps relative to the normal parameter path (pure-reflection population, ignoring the +OGNL-specific allowlisting which is intentionally *not* shared): + +| Control | Normal path | JSON path today | +|---|---|---| +| `@StrutsParameter` authorization | yes | yes (WW-5624) | +| Excluded name patterns (`ExcludedPatternsChecker`) | yes | **no** | +| Accepted name patterns (`AcceptedPatternsChecker`) | yes | **no** | +| Param-name max length (default 100) | yes | **no** | +| `ParameterNameAware` callback | yes | **no** | +| `ParameterValueAware` callback | yes | **no** | +| Excluded/accepted value patterns | yes | **no** | +| Interceptor's own `excludeProperties`/`includeProperties` | output only | not on input | + +## Goal & Non-Goals + +**Goal:** Make the JSON-object population path enforce the same name/value acceptability +controls as `ParametersInterceptor`, while **keeping pure-reflection population**. OGNL +name evaluation is deliberately *not* introduced — pure reflection is a security feature +(it sidesteps the historical Struts OGNL-injection vector), and this change preserves it. + +**Non-goals:** + +- The JSON-RPC path (`Content-Type: application/json-rpc`, SMD method invocation) is out + of scope. It binds deserialized values to *method arguments*, not to stack properties, + so the parameter-filtering controls do not apply. +- No changes to `ParametersInterceptor` (the framework's most security-sensitive class). +- No conversion of JSON to an HTTP parameter map, and no delegation to + `ParametersInterceptor` (the ticket's original 2017 proposal). That would re-introduce + OGNL name evaluation on JSON input and is explicitly rejected. + +## Approach + +Generalize the existing single tree-walk in `JSONInterceptor`, +`filterUnauthorizedKeysRecursive()` → `filterUnacceptableKeysRecursive()`. That walk +already computes dotted paths (`address.city`, `items[0].name`) using the same naming +semantics `ParametersInterceptor` uses, and already removes rejected keys in place. Every +new check hooks in at that same visit point — one traversal, no second representation of +the JSON tree. + +### New injected dependencies + +Injected into `JSONInterceptor` — the *same* global singletons `ParametersInterceptor` +uses, so the name denylist/allowlist configuration is shared automatically via the global +Struts constants (no duplicate configuration): + +- `ExcludedPatternsChecker excludedPatterns` +- `AcceptedPatternsChecker acceptedPatterns` + +### Per-node checks (unified traversal) + +For each JSON key at `fullPath`, reject (remove the key) if any of these fail: + +**Always-on (security / app-owned):** + +- Name length `fullPath.length() <= paramNameMaxLength` +- Not excluded: `!excludedPatterns.isExcluded(fullPath).isExcluded()` +- Accepted: `acceptedPatterns.isAccepted(fullPath).isAccepted()` +- `@StrutsParameter` authorization — **existing behavior, unchanged** +- `ParameterNameAware.acceptableParameterName(fullPath)` when the action implements it + +**Opt-in:** + +- Interceptor's own `excludeProperties` / `includeProperties` matched against `fullPath`, + gated by a new `applyPropertyFiltersToInput` flag (default `false`). + +For each **leaf scalar value** (including scalar elements inside arrays): + +**Always-on:** + +- `ParameterValueAware.acceptableParameterValue(value)` when the action implements it + +**Opt-in:** + +- Excluded/accepted **value** patterns via new `setExcludedValuePatterns` / + `setAcceptedValuePatterns` (mirroring `ParametersInterceptor`). "No patterns + configured" means no value filtering, so the opt-in is implicit — configuring the + patterns turns the filtering on. Values are stringified with `String.valueOf`; + `null`/empty values are treated as acceptable, matching the normal path. + +Maps and Lists recurse as today. List handling is extended to value-check scalar +elements: a scalar element whose value fails a value check is dropped from the list +(individual element removal, not whole-array rejection — approved trade-off). Rejections +log at warn level in devMode and debug level otherwise, mirroring `ParametersInterceptor`. + +### New configuration surface (all on `JSONInterceptor`) + +| Setting | Default | Effect | +|---|---|---| +| `paramNameMaxLength` | 100 | Max length of a dotted JSON key path | +| `excludedValuePatterns` / `acceptedValuePatterns` | unset | Opt-in value filtering | +| `applyPropertyFiltersToInput` | false | Apply `excludeProperties`/`includeProperties` to input | + +Default `struts-plugin.xml` wiring is unchanged: always-on security filters active, +behavioral filters off by default. + +## Rollout / Backward Compatibility + +Posture: **security-on, behavior opt-in.** + +- Always-on additions (name denylist/allowlist, name length, plus the app-owned + `ParameterNameAware`/`ParameterValueAware` callbacks) only *tighten* acceptance and + align JSON with the framework's baseline security controls. +- Changes that could break existing permissive JSON apps — value patterns and applying + `excludeProperties`/`includeProperties` to input — are opt-in and default off. + +## Testing + +Extend `JSONInterceptorTest` (matching that class's existing framework and conventions) +with one focused test per gap: + +- Excluded name pattern rejects a key +- Accepted name pattern gating +- Over-length key rejected +- `ParameterNameAware` rejection honored +- `ParameterValueAware` rejection honored +- Opt-in value patterns (excluded and accepted) +- Opt-in `applyPropertyFiltersToInput` applies `excludeProperties`/`includeProperties` + +Plus coverage for nested-object paths and list-element paths. Verify existing population +tests pass unchanged — accepted input must populate identically to today. + +## Risk Notes + +- Pure-reflection population is unchanged for accepted input; no OGNL surface is added. +- Always-on additions only tighten acceptance; app-breaking behavior changes are opt-in. +- `ParametersInterceptor` is not modified. +- The important shared logic (the pattern checkers) is reused via the existing global + singletons, so JSON and the normal path share one source of truth for name patterns. 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 d0acfd4ce0..b1c1723888 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 @@ -19,10 +19,15 @@ package org.apache.struts2.json; 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.Inject; import org.apache.struts2.interceptor.AbstractInterceptor; import org.apache.struts2.interceptor.parameter.ParameterAuthorizer; +import org.apache.struts2.security.AcceptedPatternsChecker; +import org.apache.struts2.security.ExcludedPatternsChecker; +import org.apache.struts2.util.TextParseUtil; import org.apache.struts2.util.ValueStack; import org.apache.struts2.util.WildcardUtil; import org.apache.commons.lang3.BooleanUtils; @@ -74,11 +79,17 @@ public class JSONInterceptor extends AbstractInterceptor { private JSONUtil jsonUtil; private ParameterAuthorizer parameterAuthorizer; + private ExcludedPatternsChecker excludedPatterns; + private AcceptedPatternsChecker acceptedPatterns; private int maxElements = JSONReader.DEFAULT_MAX_ELEMENTS; private int maxDepth = JSONReader.DEFAULT_MAX_DEPTH; private int maxLength = 2_097_152; // 2MB private int maxStringLength = JSONReader.DEFAULT_MAX_STRING_LENGTH; private int maxKeyLength = JSONReader.DEFAULT_MAX_KEY_LENGTH; + private int paramNameMaxLength = 100; + private Set excludedValuePatterns; + private Set acceptedValuePatterns; + private boolean applyPropertyFiltersToInput = false; @SuppressWarnings("unchecked") public String intercept(ActionInvocation invocation) throws Exception { @@ -133,8 +144,8 @@ public String intercept(ActionInvocation invocation) throws Exception { if (rootObject == null) // model overrides action rootObject = invocation.getStack().peek(); - // enforce @StrutsParameter authorization on JSON body keys - filterUnauthorizedKeys(json, rootObject, invocation.getAction()); + // enforce name/value acceptability (patterns, length, *Aware) and @StrutsParameter authorization on JSON body keys + filterUnacceptableKeys(json, rootObject, invocation.getAction()); // populate fields populator.populateObject(rootObject, json); @@ -206,57 +217,152 @@ private void applyLimitsToReader() { } @SuppressWarnings("rawtypes") - private void filterUnauthorizedKeys(Map json, Object target, Object action) { - filterUnauthorizedKeysRecursive(json, "", target, action); + private void filterUnacceptableKeys(Map json, Object target, Object action) { + filterUnacceptableKeysRecursive(json, "", target, action); } @SuppressWarnings("rawtypes") - private void filterUnauthorizedKeysRecursive(Map json, String prefix, Object target, Object action) { + private void filterUnacceptableKeysRecursive(Map json, String prefix, Object target, Object action) { Iterator it = json.entrySet().iterator(); while (it.hasNext()) { Map.Entry entry = it.next(); if (!(entry.getKey() instanceof String key)) { // Defensive: a custom JSONReader could produce non-String keys. Skip — we cannot - // construct a parameter path for authorization, and JSONPopulator wouldn't bind - // these to bean properties anyway. + // 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); continue; } String fullPath = prefix.isEmpty() ? key : prefix + "." + key; - if (!parameterAuthorizer.isAuthorized(fullPath, target, action)) { - LOG.warn("JSON body parameter [{}] rejected by @StrutsParameter authorization on [{}]", - fullPath, target.getClass().getName()); + if (!isAcceptableKey(fullPath, target, action)) { it.remove(); continue; } - // Recurse into nested Maps (JSON objects) to enforce depth-aware authorization Object value = entry.getValue(); if (value instanceof Map) { - filterUnauthorizedKeysRecursive((Map) value, fullPath, target, action); + filterUnacceptableKeysRecursive((Map) value, fullPath, target, action); } else if (value instanceof java.util.List) { - filterUnauthorizedList((java.util.List) value, fullPath, target, action); + filterUnacceptableList((java.util.List) value, fullPath, target, action); + } else if (!isAcceptableValue(fullPath, value, action)) { + it.remove(); } } } @SuppressWarnings("rawtypes") - private void filterUnauthorizedList(java.util.List list, String prefix, Object target, Object action) { - // Use prefix+"[0]" so that list element properties pick up one extra '[' in their path, - // matching the indexed-path semantics of ParametersInterceptor (e.g. "items[0].key" → depth 2). + 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, + // matching the indexed-path semantics of ParametersInterceptor (e.g. "items[0].key"). String elementPrefix = prefix + "[0]"; - for (Object item : list) { + Iterator it = list.iterator(); + while (it.hasNext()) { + Object item = it.next(); if (item instanceof Map) { - filterUnauthorizedKeysRecursive((Map) item, elementPrefix, target, action); + filterUnacceptableKeysRecursive((Map) item, elementPrefix, target, action); } else if (item instanceof java.util.List) { - // Handle nested lists (e.g. List>) by recursing with the same elementPrefix - filterUnauthorizedList((java.util.List) item, elementPrefix, target, action); + 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)) { + it.remove(); } } } + @SuppressWarnings("rawtypes") + private boolean isAcceptableKey(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; + } + if (excludedPatterns != null && excludedPatterns.isExcluded(fullPath).isExcluded()) { + 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; + } + return true; + } + + private boolean isAcceptedByPropertyFilters(String fullPath) { + if (excludeProperties != null) { + for (Pattern pattern : excludeProperties) { + if (pattern.matcher(fullPath).matches()) { + return false; + } + } + } + if (includeProperties != null && !includeProperties.isEmpty()) { + for (Pattern pattern : includeProperties) { + if (pattern.matcher(fullPath).matches()) { + return true; + } + } + return false; + } + return true; + } + + private boolean isAcceptableValue(String fullPath, Object value, Object action) { + String stringValue = value == null ? null : String.valueOf(value); + if (action instanceof ParameterValueAware valueAware && !valueAware.acceptableParameterValue(stringValue)) { + LOG.debug("JSON body value for parameter [{}] rejected by ParameterValueAware action", fullPath); + return false; + } + if (stringValue == null || stringValue.isEmpty()) { + return true; + } + if (isValueExcluded(stringValue)) { + LOG.warn("JSON body value [{}] for parameter [{}] matches an excluded value pattern; rejected", stringValue, fullPath); + return false; + } + if (!isValueAccepted(stringValue)) { + LOG.warn("JSON body value [{}] for parameter [{}] does not match any accepted value pattern; rejected", stringValue, fullPath); + return false; + } + return true; + } + + private boolean isValueExcluded(String value) { + if (excludedValuePatterns == null || excludedValuePatterns.isEmpty()) { + return false; + } + for (Pattern pattern : excludedValuePatterns) { + if (pattern.matcher(value).matches()) { + return true; + } + } + return false; + } + + private boolean isValueAccepted(String value) { + if (acceptedValuePatterns == null || acceptedValuePatterns.isEmpty()) { + return true; + } + for (Pattern pattern : acceptedValuePatterns) { + if (pattern.matcher(value).matches()) { + return true; + } + } + return false; + } + protected String readContentType(HttpServletRequest request) { String contentType = request.getHeader("Content-Type"); LOG.debug("Content Type from request: {}", contentType); @@ -647,6 +753,65 @@ public void setParameterAuthorizer(ParameterAuthorizer parameterAuthorizer) { this.parameterAuthorizer = parameterAuthorizer; } + @Inject + public void setExcludedPatterns(ExcludedPatternsChecker excludedPatterns) { + this.excludedPatterns = excludedPatterns; + } + + @Inject + public void setAcceptedPatterns(AcceptedPatternsChecker acceptedPatterns) { + this.acceptedPatterns = acceptedPatterns; + } + + /** + * If the dotted JSON key path exceeds the configured maximum length it will not be accepted. + * + * @param paramNameMaxLength maximum length of a JSON key path + */ + public void setParamNameMaxLength(int paramNameMaxLength) { + this.paramNameMaxLength = paramNameMaxLength; + } + + /** + * Sets a comma-delimited list of regular expressions to match JSON leaf values + * that should be removed. Opt-in: no patterns configured means no value filtering. + * + * @param commaDelim comma-delimited regular expressions + */ + public void setExcludedValuePatterns(String commaDelim) { + this.excludedValuePatterns = compileValuePatterns(commaDelim); + } + + /** + * Sets a comma-delimited list of regular expressions; when set, only JSON leaf values + * matching one of them are accepted. Opt-in: no patterns configured means no value filtering. + * + * @param commaDelim comma-delimited regular expressions + */ + public void setAcceptedValuePatterns(String commaDelim) { + this.acceptedValuePatterns = compileValuePatterns(commaDelim); + } + + /** + * When enabled, the interceptor's {@code excludeProperties}/{@code includeProperties} patterns — + * otherwise used only for serialization output — also gate which JSON keys are populated on input. + * Opt-in; defaults to {@code false} to preserve existing behavior. + * + * @param applyPropertyFiltersToInput true to apply property filters to deserialization + */ + public void setApplyPropertyFiltersToInput(boolean applyPropertyFiltersToInput) { + this.applyPropertyFiltersToInput = applyPropertyFiltersToInput; + } + + private static Set compileValuePatterns(String commaDelim) { + Set raw = TextParseUtil.commaDelimitedStringToSet(commaDelim); + Set compiled = new HashSet<>(raw.size()); + for (String pattern : raw) { + compiled.add(Pattern.compile(pattern, Pattern.CASE_INSENSITIVE)); + } + return Collections.unmodifiableSet(compiled); + } + @Inject(value = JSONConstants.JSON_MAX_ELEMENTS, required = false) public void setMaxElements(String maxElements) { this.maxElements = Integer.parseInt(maxElements); 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 ece2f1272d..e1aed361ba 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 @@ -50,6 +50,8 @@ private JSONInterceptor createInterceptor() { interceptor.setJsonUtil(jsonUtil); // Default: allow all parameters (simulates requireAnnotations=false) interceptor.setParameterAuthorizer((parameterName, target, action) -> true); + interceptor.setExcludedPatterns(new org.apache.struts2.security.DefaultExcludedPatternsChecker()); + interceptor.setAcceptedPatterns(new org.apache.struts2.security.DefaultAcceptedPatternsChecker()); return interceptor; } @@ -598,7 +600,7 @@ public void testNonStringKeysAreSkippedByAuthorizationFilter() throws Exception mixedKeyMap.put(42, "shouldBeSkipped"); // Integer key, not String java.lang.reflect.Method method = JSONInterceptor.class.getDeclaredMethod( - "filterUnauthorizedKeys", java.util.Map.class, Object.class, Object.class); + "filterUnacceptableKeys", java.util.Map.class, Object.class, Object.class); method.setAccessible(true); // Should not throw ClassCastException @@ -707,6 +709,202 @@ public void testNonActionRootObjectStillChecked() throws Exception { assertEquals(0, bean.getIntField()); } + public void testExcludedNamePatternRejectsKey() throws Exception { + this.request.setContent("{\"foo\":\"a\", \"bar\":\"b\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + org.apache.struts2.security.DefaultExcludedPatternsChecker excluded = + new org.apache.struts2.security.DefaultExcludedPatternsChecker(); + excluded.setExcludedPatterns("bar"); + interceptor.setExcludedPatterns(excluded); + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertEquals("a", action.getFoo()); + assertNull(action.getBar()); + } + + public void testAcceptedNamePatternRejectsKey() throws Exception { + this.request.setContent("{\"foo\":\"a\", \"bar\":\"b\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + org.apache.struts2.security.DefaultAcceptedPatternsChecker accepted = + new org.apache.struts2.security.DefaultAcceptedPatternsChecker(); + accepted.setAcceptedPatterns("foo"); + interceptor.setAcceptedPatterns(accepted); + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertEquals("a", action.getFoo()); + assertNull(action.getBar()); + } + + public void testExcludedNamePatternRejectsNestedKey() throws Exception { + this.request.setContent("{\"bean\": {\"stringField\": \"keep\", \"intField\": 42}}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + org.apache.struts2.security.DefaultExcludedPatternsChecker excluded = + new org.apache.struts2.security.DefaultExcludedPatternsChecker(); + excluded.setExcludedPatterns("bean\\.intField"); + interceptor.setExcludedPatterns(excluded); + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertNotNull(action.getBean()); + assertEquals("keep", action.getBean().getStringField()); + assertEquals(0, action.getBean().getIntField()); + } + + public void testExcludedValuePatternRejectsListElement() throws Exception { + this.request.setContent("{\"list\": [\"good\", \"badvalue\"]}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + interceptor.setExcludedValuePatterns("badvalue"); + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertNotNull(action.getList()); + assertEquals(1, action.getList().size()); + assertEquals("good", action.getList().get(0)); + } + + public void testParamNameMaxLengthRejectsLongKey() throws Exception { + this.request.setContent("{\"foo\":\"a\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + interceptor.setParamNameMaxLength(2); // "foo" is length 3, over the limit + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertNull(action.getFoo()); + } + + public void testParameterNameAwareRejectsKey() throws Exception { + this.request.setContent("{\"foo\":\"a\", \"bar\":\"b\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + ParameterAwareTestAction action = new ParameterAwareTestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertEquals("a", action.getFoo()); + assertNull(action.getBar()); + } + + public void testParameterValueAwareRejectsValue() throws Exception { + this.request.setContent("{\"foo\":\"blocked\", \"baz\":\"good\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + ParameterAwareTestAction action = new ParameterAwareTestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertNull(action.getFoo()); + assertEquals("good", action.getBaz()); + } + + public void testExcludedValuePatternRejectsValue() throws Exception { + this.request.setContent("{\"foo\":\"badvalue\", \"bar\":\"okvalue\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + interceptor.setExcludedValuePatterns("badvalue"); + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertNull(action.getFoo()); + assertEquals("okvalue", action.getBar()); + } + + public void testAcceptedValuePatternRejectsValue() throws Exception { + this.request.setContent("{\"foo\":\"allowed\", \"bar\":\"other\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + interceptor.setAcceptedValuePatterns("allowed"); + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertEquals("allowed", action.getFoo()); + assertNull(action.getBar()); + } + + public void testExcludePropertiesAppliedToInputWhenEnabled() throws Exception { + this.request.setContent("{\"foo\":\"a\", \"bar\":\"b\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + interceptor.setApplyPropertyFiltersToInput(true); + interceptor.setExcludeProperties("foo"); + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertNull(action.getFoo()); + assertEquals("b", action.getBar()); + } + + public void testExcludePropertiesNotAppliedToInputByDefault() throws Exception { + this.request.setContent("{\"foo\":\"a\", \"bar\":\"b\"}".getBytes()); + this.request.addHeader("Content-Type", "application/json"); + + JSONInterceptor interceptor = createInterceptor(); + interceptor.setExcludeProperties("foo"); // configured, but flag off + TestAction action = new TestAction(); + + this.invocation.setAction(action); + this.invocation.getStack().push(action); + + interceptor.intercept(this.invocation); + + assertEquals("a", action.getFoo()); + assertEquals("b", action.getBar()); + } + @Override protected void setUp() throws Exception { super.setUp(); 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 new file mode 100644 index 0000000000..d71287cd93 --- /dev/null +++ b/plugins/json/src/test/java/org/apache/struts2/json/ParameterAwareTestAction.java @@ -0,0 +1,63 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.json; + +import org.apache.struts2.action.ParameterNameAware; +import org.apache.struts2.action.ParameterValueAware; + +public class ParameterAwareTestAction implements ParameterNameAware, ParameterValueAware { + + private String foo; + private String bar; + private String baz; + + public String getFoo() { + return foo; + } + + public void setFoo(String foo) { + this.foo = foo; + } + + public String getBar() { + return bar; + } + + public void setBar(String bar) { + this.bar = bar; + } + + public String getBaz() { + return baz; + } + + public void setBaz(String baz) { + this.baz = baz; + } + + @Override + public boolean acceptableParameterName(String parameterName) { + return !"bar".equals(parameterName); + } + + @Override + public boolean acceptableParameterValue(String parameterValue) { + return !"blocked".equals(parameterValue); + } +}