Repository navigation
Conversation
lukaszlenart
left a comment
lukaszlenart
left a comment
There was a problem hiding this comment.
Thanks for the patch, the shape of the fix is right and the plugin suite is green locally on this branch (JSONUtilTest, JSONInterceptorTest, JSONResultTest).
A few changes before this can go in:
- the new warning line should normalise the rejected value like the other request-derived log lines in
JSONInterceptoralready do (normalizeSpace, see WW-5825); JSONInterceptorTestneeds the same rejection + control pair for the SMD arm asJSONResultTestnow has for the result arm;- keep the Javadoc and test comments functional, they currently narrate what a bad value could do; the helper's contract (dotted ASCII identifier, 128 chars max) is all the reader needs;
- the commit headline is still
WW-XXXX, please useWW-5845.
| if ((callbackName != null) && (callbackName.length() > 0)) | ||
| if ((callbackName != null) && (callbackName.length() > 0)) { | ||
| if (!JSONUtil.isValidJsonpCallbackName(callbackName)) { | ||
| LOG.warn("Rejecting JSONP callback parameter '{}' -- not a valid (dotted) " |
There was a problem hiding this comment.
Please pass normalizeSpace(callbackName) here, as the other request-derived values logged in this class are (lines 302 and 306). An invalid callback name is by definition unrestricted text, so the raw value does not belong in the log line as-is. Same at the JSONResult call site.
| String callbackName = request.getParameter(callbackParameter); | ||
| if (StringUtils.isNotEmpty(callbackName)) { | ||
| if (!JSONUtil.isValidJsonpCallbackName(callbackName)) { | ||
| LOG.warn("Rejecting JSONP callback parameter '{}' -- not a valid (dotted) " |
There was a problem hiding this comment.
Same as in JSONInterceptor: normalizeSpace(callbackName). org.apache.commons.lang3.StringUtils is already imported here.
| return StringUtils.contains(request.getHeader("Accept-Encoding"), "gzip"); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Please reduce this to the contract: the accepted shape (dotted ASCII identifier, [A-Za-z_$][A-Za-z0-9_$]*, segments joined by .), the length cap, and @since. The two {@link}s to the call sites and the paragraph about what an unchecked value could do belong in the ticket, not in the Javadoc.
| && JSONP_CALLBACK_PATTERN.matcher(callbackName).matches(); | ||
| } | ||
|
|
||
| private static final Pattern JSONP_CALLBACK_PATTERN = |
There was a problem hiding this comment.
Move the constant above the method that uses it, next to the other static final fields in this class.
| assertEquals("application/json;charset=UTF-8", response.getContentType()); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
The test name says what it checks; the comment block above it and the two inline comments can go. One line naming testJSONP() as the control case is enough if you want to keep that pointer.
| assertEquals("{\"listOfLists\":[[\"1\",\"2\"],[\"3\",\"4\"],[\"5\",\"6\"],[\"7\",\"8\"],[\"9\",\"0\"]]}", actual); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Same here, drop the comment block. Also please add the SMD-arm equivalent to JSONInterceptorTest: a request with callbackParameter set and a non-identifier value, asserting the response body is the bare JSON, next to a control request with exec that is wrapped. That path is not covered by JSONResultTest.
…e response in it
JSONResult.addCallbackIfApplicable and JSONInterceptor.addCallbackIfApplicable
built the JSONP wrapper as callbackName + "(" + json + ")", where
callbackName is the raw, unvalidated callbackParameter query value -- no
character restriction. Since the response is meant to be loaded as
<script src=...>, an unvalidated value there is more than a
differently-named callback function.
Adds a shared JSONUtil.isValidJsonpCallbackName() check
(^[A-Za-z_$][A-Za-z0-9_$]*(\.[A-Za-z_$][A-Za-z0-9_$]*)*$, length capped
at 128) at both call sites. An invalid callback name falls back to the
unwrapped JSON response, logged at WARN.
Tests: JSONUtilTest#testIsValidJsonpCallbackName,
JSONResultTest#testJSONPCallbackMustBeValidIdentifier (new, confirmed to
fail without the fix with a genuine assertion mismatch). Existing
JSONResultTest, JSONUtilTest, JSONInterceptorTest unaffected. Full
plugins/json suite: 179/0 failures.
cec1e97 to
428db20
Compare
… SMD arm, trim comments - log the rejected callback name through normalizeSpace in JSONInterceptor and JSONResult, like the other request-derived values in these classes - JSONInterceptorTest: add a rejection and a control test for the SMD arm - reduce the JSONUtil Javadoc and the new test comments to the contract - move the callback pattern next to the other static final fields
|
Addressed in 89ea169:
The commit headline was already switched to |
Fixes WW-5845
JSONResult.addCallbackIfApplicableandJSONInterceptor.addCallbackIfApplicablebuilt the JSONP wrapper ascallbackName + "(" + json + ")"from the raw, unvalidatedcallbackParameterquery value. Since the response is meant to be loaded as<script src=...>, an unvalidated value there is more than a differently-named callback function.Adds a shared
JSONUtil.isValidJsonpCallbackName()check (dotted ASCII identifier, max 128 characters) at both call sites. An invalid callback name leaves the JSON response unwrapped and is logged at WARN.Verified:
JSONUtilTest#testIsValidJsonpCallbackName,JSONResultTest#testJSONPCallbackMustBeValidIdentifier(new). Fullplugins/json: 179/0 failures.