Some minor comments on this patch, other than that LGTM. http://codereview.appspot.com/186167/diff/1/4 File bundles/servlets/get/src/main/java/org/apache/sling/servlets/get/impl/JsonQueryServlet.java (right): http://codereview.appspot.com/186167/diff/1/4#newcode199 ...
14 years, 3 months ago
(2010-01-16 09:43:21 UTC)
#2
Some minor comments on this patch, other than that LGTM.
http://codereview.appspot.com/186167/diff/1/4
File
bundles/servlets/get/src/main/java/org/apache/sling/servlets/get/impl/JsonQueryServlet.java
(right):
http://codereview.appspot.com/186167/diff/1/4#newcode199
bundles/servlets/get/src/main/java/org/apache/sling/servlets/get/impl/JsonQueryServlet.java:199:
this segment of the patch looks like a white space change?
http://codereview.appspot.com/186167/diff/1/5
File
bundles/servlets/get/src/main/java/org/apache/sling/servlets/get/impl/RedirectServlet.java
(left):
http://codereview.appspot.com/186167/diff/1/5#oldcode55
bundles/servlets/get/src/main/java/org/apache/sling/servlets/get/impl/RedirectServlet.java:55:
* @scr.component immediate="true" metatype="no"
was there a reason to remove metatype ?
http://codereview.appspot.com/186167/diff/1/2
File
bundles/servlets/get/src/main/java/org/apache/sling/servlets/get/impl/helpers/JsonRendererServlet.java
(right):
http://codereview.appspot.com/186167/diff/1/2#newcode110
bundles/servlets/get/src/main/java/org/apache/sling/servlets/get/impl/helpers/JsonRendererServlet.java:110:
allowedLevel = Integer.parseInt(e.getMessage());
since you have a specific exception why not have an int in the exception rather
than parsing ?
I noticed this was still marked as draft and not send out. http://codereview.appspot.com/186167/diff/1/4 File bundles/servlets/get/src/main/java/org/apache/sling/servlets/get/impl/JsonQueryServlet.java ...
14 years, 3 months ago
(2010-01-18 10:11:23 UTC)
#3
I noticed this was still marked as draft and not send out.
http://codereview.appspot.com/186167/diff/1/4
File
bundles/servlets/get/src/main/java/org/apache/sling/servlets/get/impl/JsonQueryServlet.java
(right):
http://codereview.appspot.com/186167/diff/1/4#newcode199
bundles/servlets/get/src/main/java/org/apache/sling/servlets/get/impl/JsonQueryServlet.java:199:
On 2010/01/16 09:43:21, ianboston wrote:
> this segment of the patch looks like a white space change?
Yes, my bad, this can be ignored.
http://codereview.appspot.com/186167/diff/1/5
File
bundles/servlets/get/src/main/java/org/apache/sling/servlets/get/impl/RedirectServlet.java
(left):
http://codereview.appspot.com/186167/diff/1/5#oldcode55
bundles/servlets/get/src/main/java/org/apache/sling/servlets/get/impl/RedirectServlet.java:55:
* @scr.component immediate="true" metatype="no"
Yes, I added an OSGi property. If I didn't remove it, the scr plugin would not
recognize it.
On 2010/01/16 09:43:21, ianboston wrote:
> was there a reason to remove metatype ?
http://codereview.appspot.com/186167/diff/1/2
File
bundles/servlets/get/src/main/java/org/apache/sling/servlets/get/impl/helpers/JsonRendererServlet.java
(right):
http://codereview.appspot.com/186167/diff/1/2#newcode110
bundles/servlets/get/src/main/java/org/apache/sling/servlets/get/impl/helpers/JsonRendererServlet.java:110:
allowedLevel = Integer.parseInt(e.getMessage());
You're right, I should have modified the exception as well.
On 2010/01/16 09:43:21, ianboston wrote:
> since you have a specific exception why not have an int in the exception
rather
> than parsing ?
Issue 186167: JsonResourceWriter fix for possible DOS attack (3)
Created 14 years, 3 months ago by simong
Modified 9 years, 4 months ago
Reviewers: ianboston
Base URL: https://svn.apache.org/repos/asf/sling/trunk/
Comments: 6