Notes below.... On Fri, Aug 21, 2009 at 8:50 PM, Jasvir Nagra <[email protected]> wrote:
> 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. The point is that this entire code path is skipped if window.JSON is defined by the browser. Will the native window.JSON functionality in recent browsers output the additional Caja properties? > > > > > > > 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. Oops. Fixing. > > > > > > > > > > > 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 > > > > > >
