http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/domado.js File src/com/google/caja/plugin/domado.js (right):
http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/domado.js#newcode60 src/com/google/caja/plugin/domado.js:60: var PLACEHOLDER_theTools; // TODO(kpreid): This is a placeholder for something to replace the pluginId mechanism, making it into a global singleton instead. 'tools' is the replacement for ___ properties on 'imports'. Isn't tools also akin to a view? html-emitter accesses tools.document. http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/domado.js#newcode182 src/com/google/caja/plugin/domado.js:182: * the right thing is. "overriden" -> "overridden"? Does "non-writable property cannot be overridden" imply (propName in object.prototype && !Object.hasOwnProperty.call(object, propName)) before the attempt to set? http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/domado.js#newcode192 src/com/google/caja/plugin/domado.js:192: enumerable: domitaModules.getPropertyDescriptor(object, propName) propName might be coerced to two different strings above if propName can come from an untrusted source. Maybe do propName += ''; at the start. Does getPropertyDescriptor not return a nullish value when propName is blank? Should properties default to non-enumerable if not overriding? http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/domado.js#newcode229 src/com/google/caja/plugin/domado.js:229: * objects). Mark, can you please review this? http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/domado.js#newcode318 src/com/google/caja/plugin/domado.js:318: * @param {Object} target The tame node to forward methods to. Ok. After discussion, there is a 3-layered onion. The feral node is the core, a middle tame DOM node which reinterprets messages meant for the feral node, and an outer layer that implements expandos for any messages that cannot be handled by the middle tier. So the target is both tame and an inner layer. http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/domado.js#newcode320 src/com/google/caja/plugin/domado.js:320: * @param {Object} key The object to index the expando properties under. key seems an odd name here. http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/domado.js#newcode330 src/com/google/caja/plugin/domado.js:330: return this; Is this necessary? http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/plugin/domado.js#newcode352 src/com/google/caja/plugin/domado.js:352: }; Restart here. http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js File src/com/google/caja/plugin/domado.js (right): http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode50 src/com/google/caja/plugin/domado.js:50: * @overrides domitaModules This is a really impressive piece of work. Are there ways we could use naming conventions to make it easier for reviewers/maintainers to triage inputs, e.g. into (untrusted value, tame value, statically unclear provenance). That's something that is out of scope for this CL, but thought I'd raise the idea. http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode160 src/com/google/caja/plugin/domado.js:160: if (i === props.length) throw StopIteration; I know this is commented out, but is it a memory leak to not null out elements of props that are < i? Probably only an issue if large strings are used as keys with long-lived iterators. http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode204 src/com/google/caja/plugin/domado.js:204: function (aCallback) { Do the callbacks tend to receive an argument? Is it worth doing an arity check? Do we need to check that the argument is callable by untrusted code or is that not ES53-ish? Is eval a valid callback? http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode226 src/com/google/caja/plugin/domado.js:226: * ability for one object to access its the private state of other similar "its the" -> "the" Is this sibling amplification similar to the way that an instance of a class C in java can access the privates of other instances of a class C? http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode282 src/com/google/caja/plugin/domado.js:282: guard.coerce(object); // borrow failure Does this always fail, or will this return undefined instead of an empty object sometimes? http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode348 src/com/google/caja/plugin/domado.js:348: ExpandoProxyHandler.prototype.getOwnPropertyDescriptor = function (name) { Are these name parameters guaranteed to be coerced to strings or numbers before reaching the proxy handler? http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode515 src/com/google/caja/plugin/domado.js:515: var TameXHRConf = new Confidence(); Missing typename? http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode520 src/com/google/caja/plugin/domado.js:520: TameXHRConf.confide(this); If someone forgets a `new` then we might confide in the global object. Maybe a simple instanceof check at the top of constructors that confide in this would add a layer of protection. http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode521 src/com/google/caja/plugin/domado.js:521: var xhr = p(this).feral = Ok, so p(this) gets the private of this and that has to be done to the object that backs the proxy, not the proxy object. http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode530 src/com/google/caja/plugin/domado.js:530: } This self idiom seems to occur several times with subtle variations. Can we reduce the differences so someone who has to find and change uses of this idiom has an easier time operating on all of them? http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode652 src/com/google/caja/plugin/domado.js:652: var evt = { target: this }; Do we use this, not the proxy because an event handler wrapper dispatches to the proxy if proxy self != this? http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode833 src/com/google/caja/plugin/domado.js:833: function definePropertiesAwesomely(object, propDescs) { Is this part of Bill and Ted's excellent reflection? Does this threads the property name through to things like myNode.style.backgroundColor where the handler can be generic over all CSS property names if it just figure out which CSS property is being set? http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode850 src/com/google/caja/plugin/domado.js:850: return extGet.call(this, prop); What is `this` here and at 855? http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode892 src/com/google/caja/plugin/domado.js:892: return array.push.apply(array, rest); This seems to be trusting array.push which is OK I guess if it is only called on listener lists we control, but if you don't need the negative index handling, isn't it easier to just do array.splice(from, to - from) or the negative index handling equivalent : array.splice((from + array.length) % array.length, (to + array.length) % array.length - (from + array.length) % array.length)? http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode912 src/com/google/caja/plugin/domado.js:912: var h = String(htmlFragment || ''); If htmlFragment is 0 or NaN this might cause strangeness. Probably a problem with the original code, so I don't think it's a huge problem, but maybe String(htmlFragment == null ? '' : htmlFragment) where the == is intentionally not ===. http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode922 src/com/google/caja/plugin/domado.js:922: : html.escapeAttrib(String(htmlFragment || '')); Same problem as above. http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode1061 src/com/google/caja/plugin/domado.js:1061: * map is never used with more than one version of setTimeout. Cool. http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode1266 src/com/google/caja/plugin/domado.js:1266: * object is known as a "domicile". Thanks for the docs. http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode1279 src/com/google/caja/plugin/domado.js:1279: var window = bridalMaker.getWindow(pseudoBodyNode); Ah, so this uses the trick with the inserted script node. http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode1282 src/com/google/caja/plugin/domado.js:1282: domicile.isProcessingEvent = false; Ok, so this is used to elevate permission when the user is interacting with an element. http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode1286 src/com/google/caja/plugin/domado.js:1286: } Maybe move this check to the top. The closer it is to either succeeding or ding nothing to document and window, the better. http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode1345 src/com/google/caja/plugin/domado.js:1345: // unfrozen so that assignments to expando fields work. ok, so this is just an extra safety net that is available when proxy nets are available. http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode1635 src/com/google/caja/plugin/domado.js:1635: * predicate restricting the value) have the value the original node "and" before "have the value"? Maybe make it clear that the predicate is not applied on read. http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode1665 src/com/google/caja/plugin/domado.js:1665: extendedAccessors: true, Is this necessary? http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode1781 src/com/google/caja/plugin/domado.js:1781: makeDOMAccessible(node); Should this move below the sanity check below? http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode1836 src/com/google/caja/plugin/domado.js:1836: makeDOMAccessible(node); Should this move below the sanity check and early out below? http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode2389 src/com/google/caja/plugin/domado.js:2389: }); Ok. So bridal.dispatch event here gets the ferals, and is responsible for all the membrane maintenance. http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode2507 src/com/google/caja/plugin/domado.js:2507: get: nodeMethod(function () { return np(this).geometryDelegate.clientWidth; }) Can these be defined in terms of one awesome property getter that gets the property name? http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode2703 src/com/google/caja/plugin/domado.js:2703: innerText: textAccessor, Ok. Since this only applies to text nodes, the difference between innerText and nodeValue can be ignored. http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode3123 src/com/google/caja/plugin/domado.js:3123: var construct = record.construct || function (self) { return self; }; || identity? http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode3146 src/com/google/caja/plugin/domado.js:3146: }); Cool http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode3146 src/com/google/caja/plugin/domado.js:3146: }); Cool http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode3927 src/com/google/caja/plugin/domado.js:3927: src: NP.filter(false, identity, true, identity) How does the identity setter with an attr getter work here? This turns into a call to setAttribute("src") which presumably invokes the URL policy? Where is the policy code that prevents adding text nodes to a script element? http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode4052 src/com/google/caja/plugin/domado.js:4052: function fromInt(x) { return '' + (x | 0); } // coerce null and false to 0 So if x is NaN, it is coerced to 0, and if it is "NaN" it is coerced to NaN. Strange, but probably fine in practice, (and quite possibly my fault). http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode5178 src/com/google/caja/plugin/domado.js:5178: //'HTMLScriptElement', Why are these commented out? http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode5224 src/com/google/caja/plugin/domado.js:5224: event = makeDOMAccessible(event || bridalMaker.getWindow(thisNode).event); ok http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode5251 src/com/google/caja/plugin/domado.js:5251: fn = domicile.imports.window[handler]; Much nicer. http://codereview.appspot.com/4535118/diff/44001/src/com/google/caja/plugin/domado.js#newcode5267 src/com/google/caja/plugin/domado.js:5267: //} I do not know yet. I think it is good to leave this catch block around though to guard against IE discarding finally blocks. http://codereview.appspot.com/4535118/
