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 a2002c9aba..53d16dea96 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 @@ -541,8 +541,13 @@ private boolean checkSMDMethodSignature(Method method, String name, int paramete protected String addCallbackIfApplicable(HttpServletRequest request, String json) { if ((callbackParameter != null) && (callbackParameter.length() > 0)) { String callbackName = request.getParameter(callbackParameter); - if ((callbackName != null) && (callbackName.length() > 0)) + if ((callbackName != null) && (callbackName.length() > 0)) { + if (!JSONUtil.isValidJsonpCallbackName(callbackName)) { + LOG.warn("JSONP callback parameter [{}] is not a valid identifier; rejected", normalizeSpace(callbackName)); + return json; + } json = callbackName + "(" + json + ")"; + } } return json; } diff --git a/plugins/json/src/main/java/org/apache/struts2/json/JSONResult.java b/plugins/json/src/main/java/org/apache/struts2/json/JSONResult.java index f021a61ecf..6c17e82e8f 100644 --- a/plugins/json/src/main/java/org/apache/struts2/json/JSONResult.java +++ b/plugins/json/src/main/java/org/apache/struts2/json/JSONResult.java @@ -282,6 +282,10 @@ protected String addCallbackIfApplicable(HttpServletRequest request, String json if ((callbackParameter != null) && (callbackParameter.length() > 0)) { String callbackName = request.getParameter(callbackParameter); if (StringUtils.isNotEmpty(callbackName)) { + if (!JSONUtil.isValidJsonpCallbackName(callbackName)) { + LOG.warn("JSONP callback parameter [{}] is not a valid identifier; rejected", StringUtils.normalizeSpace(callbackName)); + return json; + } json = callbackName + "(" + json + ")"; } } diff --git a/plugins/json/src/main/java/org/apache/struts2/json/JSONUtil.java b/plugins/json/src/main/java/org/apache/struts2/json/JSONUtil.java index e1c20f7b1c..c0c8ff0069 100644 --- a/plugins/json/src/main/java/org/apache/struts2/json/JSONUtil.java +++ b/plugins/json/src/main/java/org/apache/struts2/json/JSONUtil.java @@ -62,6 +62,9 @@ public class JSONUtil { /** Chunk size used to read input incrementally while applying the length limit. */ private static final int READ_CHUNK_SIZE = 8192; + private static final Pattern JSONP_CALLBACK_PATTERN = + Pattern.compile("[A-Za-z_$][A-Za-z0-9_$]*(\\.[A-Za-z_$][A-Za-z0-9_$]*)*"); + private JSONReader reader; private JSONWriter writer; @@ -565,6 +568,19 @@ public static boolean isGzipInRequest(HttpServletRequest request) { return StringUtils.contains(request.getHeader("Accept-Encoding"), "gzip"); } + /** + * Checks a JSONP callback name: a dotted ASCII identifier ({@code [A-Za-z_$][A-Za-z0-9_$]*}, + * segments joined by {@code .}) of at most 128 characters. + * + * @param callbackName the requested callback name + * @return true if {@code callbackName} has that shape + * @since 7.5.0 + */ + public static boolean isValidJsonpCallbackName(String callbackName) { + return callbackName != null && callbackName.length() <= 128 + && JSONP_CALLBACK_PATTERN.matcher(callbackName).matches(); + } + public static final String REGEXP_PATTERN = "regexp"; public static final String WILDCARD_PATTERN = "wildcard"; /* package */ static final String SPLIT_PATTERN = "split"; 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 e7508fee41..1dcfeafb02 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 @@ -240,6 +240,35 @@ public void testSMDPrimitivesNoResult() throws Exception { assertEquals("application/json;charset=UTF-8", response.getContentType()); } + public void testSMDInvalidJSONPCallbackLeavesJsonUnwrapped() throws Exception { + String json = executeSMDWithCallback("cb);STRUTS_PROBE_MARKER;("); + + assertEquals(TestUtils.normalize(JSONResultTest.class.getResource("smd-11.txt")), TestUtils.normalize(json, true)); + assertFalse(json.contains("STRUTS_PROBE_MARKER")); + } + + public void testSMDValidJSONPCallbackWrapsJson() throws Exception { + String json = executeSMDWithCallback("exec"); + + assertTrue(json.startsWith("exec(") && json.endsWith(")")); + assertEquals(TestUtils.normalize(JSONResultTest.class.getResource("smd-11.txt")), + TestUtils.normalize(json.substring("exec(".length(), json.length() - 1), true)); + } + + private String executeSMDWithCallback(String callback) throws Exception { + setRequestContent("smd-6.txt"); + this.request.addHeader("Content-Type", "application/json-rpc"); + this.request.addParameter("callback", callback); + + JSONInterceptor interceptor = createInterceptor(); + interceptor.setEnableSMD(true); + interceptor.setCallbackParameter("callback"); + this.invocation.setAction(new SMDActionTest1()); + + interceptor.intercept(this.invocation); + return response.getContentAsString(); + } + public void testSMDReturnObject() throws Exception { // request setRequestContent("smd-10.txt"); diff --git a/plugins/json/src/test/java/org/apache/struts2/json/JSONResultTest.java b/plugins/json/src/test/java/org/apache/struts2/json/JSONResultTest.java index a5c938f0aa..7f9c8c5368 100644 --- a/plugins/json/src/test/java/org/apache/struts2/json/JSONResultTest.java +++ b/plugins/json/src/test/java/org/apache/struts2/json/JSONResultTest.java @@ -539,6 +539,25 @@ public void testJSONP() throws Exception { assertEquals("application/json;charset=UTF-8", response.getContentType()); } + // control case: testJSONP() + public void testJSONPCallbackMustBeValidIdentifier() throws Exception { + JSONResult result = new JSONResult(); + result.setCallbackParameter("callback"); + JSONUtil jsonUtil = new JSONUtil(); + jsonUtil.setWriter(new StrutsJSONWriter()); + result.setJsonUtil(jsonUtil); + + String notAnIdentifier = "cb);STRUTS_PROBE_MARKER;("; + request.addParameter("callback", notAnIdentifier); + + executeTest2Action(result); + String json = response.getContentAsString(); + + assertFalse("Non-identifier callback value must not be concatenated into the response", + json.contains(notAnIdentifier)); + assertFalse(json.contains("STRUTS_PROBE_MARKER")); + } + public void testNoCache() throws Exception { JSONResult result = new JSONResult(); result.setNoCache(true); diff --git a/plugins/json/src/test/java/org/apache/struts2/json/JSONUtilTest.java b/plugins/json/src/test/java/org/apache/struts2/json/JSONUtilTest.java index b2dcf8bff6..d9dca84d9e 100644 --- a/plugins/json/src/test/java/org/apache/struts2/json/JSONUtilTest.java +++ b/plugins/json/src/test/java/org/apache/struts2/json/JSONUtilTest.java @@ -79,4 +79,17 @@ public void testSerializeListOfList() throws Exception { assertEquals("{\"listOfLists\":[[\"1\",\"2\"],[\"3\",\"4\"],[\"5\",\"6\"],[\"7\",\"8\"],[\"9\",\"0\"]]}", actual); } + public void testIsValidJsonpCallbackName() { + assertTrue(JSONUtil.isValidJsonpCallbackName("exec")); + assertTrue(JSONUtil.isValidJsonpCallbackName("_cb$1")); + assertTrue(JSONUtil.isValidJsonpCallbackName("jQuery123.ajaxCallback")); + + assertFalse(JSONUtil.isValidJsonpCallbackName("cb);STRUTS_PROBE_MARKER;(")); + assertFalse(JSONUtil.isValidJsonpCallbackName("cb\nSTRUTS_PROBE_MARKER")); + assertFalse(JSONUtil.isValidJsonpCallbackName("")); + assertFalse(JSONUtil.isValidJsonpCallbackName("")); + assertFalse(JSONUtil.isValidJsonpCallbackName(null)); + assertFalse(JSONUtil.isValidJsonpCallbackName("a".repeat(129))); + } + }