On Fri, Aug 21, 2009 at 3:00 PM, Kevin Brown <[email protected]> wrote:
> On Fri, Aug 21, 2009 at 11:20 AM, <[email protected]> wrote: > > > > > http://codereview.appspot.com/104067/diff/4005/4008 > > File features/src/main/javascript/features/core/json.js (right): > > > > http://codereview.appspot.com/104067/diff/4005/4008#newcode147 > > Line 147: if (k.match('___$')) > > this restriction wont be respected if window.JSON is defined. See above. > > While this isnt strictly a security issue it it will put content you > > dont want in the JSON output. Can you test this on Firefox 3.5/ Safari > > 4? > This change only needs to execute if the for..in loop is taken - its needed because loading caja unavoidably adds additional enumerable properties on Object.prototype. Its an "innocent" change in the sense that if caja is not loaded, it doesn't do anything. Until ES5, there's no way to mark a property as being unenumerable - in the interim, all for..in loops need to be changed in this way either manually or by applying the InnocentCodeRewriter as part of the compilation process. > > I don't think this will be a major issue, but if it is we probably need to > tame window.JSON directly anyway. I think > window.JSON.stringify(___.copy(obj)); will do the trick. > > > > > > > > http://codereview.appspot.com/104067/diff/4005/4006 > > File > > > > > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/CajaContentRewriter.java > > (right): > > > > http://codereview.appspot.com/104067/diff/4005/4006#newcode85 > > Line 85: if (uri.getScheme().matches("^https?$")) { > > no need for regex, case insensitive direct comparison is sufficient. > Thank you for changing it in the patch that was applied. Unfortunately I think you missed the second case. This is testing if the uri scheme is either "http" or "https" - your modification to the patch only does a direct comparison for https. > > > > > http://codereview.appspot.com/104067/diff/4005/4006#newcode87 > > Line 87: } else if ("javascript".equals(uri.getScheme())) { > > should be case-insensitive. > > > > > > http://codereview.appspot.com/104067 > > >
