I'm holding off on reviewing the files in the draft and experimental directories since I don't know what they're for. If they're demo then they're exempt from review.
I haven't yet started on domado.js but i am going to start working through that next. http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/demos/playground/es53.html File src/com/google/caja/demos/playground/es53.html (right): http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/demos/playground/es53.html#newcode57 src/com/google/caja/demos/playground/es53.html:57: function (n) {return n;}, div, imports.document); What does using the identity function for makeDOMAccessible mean? Does it mean we're not membraning? http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guards.js File src/com/google/caja/es5-guards.js (right): http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guards.js#newcode20 src/com/google/caja/es5-guards.js:20: * TODO(kpreid): Straighten out exactly what this exports Please explain the relationship of this file to es53.js which still seems to contain trademarks and sealer unsealer pairs. http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guards.js#newcode30 src/com/google/caja/es5-guards.js:30: // SES. Mike Stay, can you comment on this? http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guards.js#newcode32 src/com/google/caja/es5-guards.js:32: var gCajaVM; Should gCajaVM be a read-only global? http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guards.js#newcode102 src/com/google/caja/es5-guards.js:102: var failFunc = opt_failFunc || function (x) { return x; }; Should this failFunc be frozen? http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guards.js#newcode105 src/com/google/caja/es5-guards.js:105: // token.UNCATCHABLE___ = true; // TODO(kpreid): can't do in ES5? MarkM, can you comment on this? Is this meant to stop it from being intercepted via try { ... } catch (ex) {} and do { try { ... } finally { break; } } while (0); I chatted with Mark and he says it is not a goal to support uncatchable exceptions in SES. I am filing an issue to audit all uses of callWithEjector in the current codebase to see whether any security properties depend on ejectors being reliable in the face of exception interception. It may be that the efficient runtime JS lexer proposed earlier on the list will allow uncatchable exceptions via a token-level-only transformation. http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guards.js#newcode182 src/com/google/caja/es5-guards.js:182: } Should SealerUnsealerPair fail if called without new? if (!(this instanceof SealerUnsealerPair)) { throw new TypeError(); } As written, it looks like this leaks the authority to freeze the global object to someone who has no access to Object.freeze. http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/bridal.js File src/com/google/caja/plugin/bridal.js (right): http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/bridal.js#newcode27 src/com/google/caja/plugin/bridal.js:27: var bridalMaker = function (makeDOMAccessible, document) { Can you add comments to help maintainers understand when makeDOMAccessible should be called and when it shouldn't. If there is documentation elsewhere on this question, a pointer is sufficient. http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/bridal.js#newcode770 src/com/google/caja/plugin/bridal.js:770: doc.body.appendChild(s); Is this supposed to happen synchronously? http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/caja.js File src/com/google/caja/plugin/caja.js (right): http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/caja.js#newcode341 src/com/google/caja/plugin/caja.js:341: * code. How are we going to verify that this function is only called on objects that are not given to untrusted code? This seems to require static inside-membrane determinations. http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/caja.js#newcode345 src/com/google/caja/plugin/caja.js:345: * wrappers. (Domado is not.) I don't understand this. Does it mean that bridal makes fewer assumptions, and might receive a makeDOMAccessible parameter that unwraps, whereas html-emitter always receives one that returns its argument. http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/caja.js#newcode358 src/com/google/caja/plugin/caja.js:358: p = '' + p; Ok. Coerces p to a string. http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/caja.js#newcode359 src/com/google/caja/plugin/caja.js:359: if (('' + (+p)) !== p && !(/__$/).test(p)) { Tests that p is not a canonicalized numeric property and does not end in __. Why is the canonicalized property significant? Why does passing in '1' result in a runtime error, but passing in '1.0' does not? http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/caja.js#newcode365 src/com/google/caja/plugin/caja.js:365: throw new TypeError('Not a function: ' + p); Misleading when p === '1.0' || p == 'x__' http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/caja.js#newcode382 src/com/google/caja/plugin/caja.js:382: } // else let primitives go normally Ok. Nothing that supports typeof [] === 'array' is in the A-grade set. http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/capture-domado.js File src/com/google/caja/plugin/capture-domado.js (right): http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/capture-domado.js#newcode34 src/com/google/caja/plugin/capture-domado.js:34: ["log", "warn", "error", "debug"].forEach(function (v) { Is this a minimal set that you need for debugging? Why these methods and not group/ungroup? http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/html-emitter.js File src/com/google/caja/plugin/html-emitter.js (right): http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/html-emitter.js#newcode30 src/com/google/caja/plugin/html-emitter.js:30: /** Please document @param makeDOMAccessible http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/html-emitter.js#newcode32 src/com/google/caja/plugin/html-emitter.js:32: * @param opt_domadoTools the tools of the domado instance that will receive a Tools is an obscure term that does nothing to convey the contracts it or its clients have to meet. Is there a set of interfaces that the HTML DOM spec defines that tools have to implement? http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/html-emitter.js#newcode41 src/com/google/caja/plugin/html-emitter.js:41: makeDOMAccessible(base); Return value discarded here and below. In the only place I've seen it used thus far, it was the identity function, not a no-op and bridal.js uses the return value. What happens if makeDOMAccessible returns a value other than its input? http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/html-emitter.js#newcode63 src/com/google/caja/plugin/html-emitter.js:63: // name conflicts. Maybe add a note that prerequisite passes such as SafeHTMLMaker are responsible for ensuring that IDs that end with __ are allowed. Also, the prohibition is on 2 underscores, not 3. http://codereview.appspot.com/4535118/
