Re: JsonResourceWriter fix for possible DOS attack (3) (issue186167)

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: JsonResourceWriter fix for possible DOS attack (3) (issue186167)

Ian Boston-3
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 ?

http://codereview.appspot.com/186167
Reply | Threaded
Open this post in threaded view
|

Re: JsonResourceWriter fix for possible DOS attack (3) (issue186167)

Simon Gaeremynck
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 ?

http://codereview.appspot.com/186167/show