On Mon, Aug 24, 2009 at 9:46 PM, Louis Ryan<[email protected]> wrote: > 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?
Ok - I understand now. Cajoled gadgets see a different window.JSON which uses json_sans_eval. The code being modified here in features/src/main/javascript/features/core/json.js is only used by tamed libraries and don't need 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 >> > > >> > >> >
