Oops! I thought I had published these comments long ago!
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);
On 2011/07/06 20:48:27, MikeSamuel wrote:
What does using the identity function for makeDOMAccessible mean?
Does it mean
we're not membraning?
It means that there is no ES5/3 taming membrane that we need to work
despite.
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
On 2011/07/06 20:48:27, MikeSamuel wrote:
Please explain the relationship of this file to es53.js which still
seems to
contain trademarks and sealer unsealer pairs.
This file is only loaded when es53.js is not. In the final version, this
will be turned into an optional component of SES, such that cajaVM
(possibly renamed 'ses' per discussion with MarkM) has a larger subset
identical on both platforms (ES5/3 and SES).
Perhaps that should be a separate changeset coming first?
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;
On 2011/07/06 20:48:27, MikeSamuel wrote:
Should gCajaVM be a read-only global?
See above; I hope to fold it into the real cajaVM.
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; };
On 2011/07/06 20:48:27, MikeSamuel wrote:
Should this failFunc be frozen?
It is only ever called, not passed, so freezing would not make a
difference.
http://codereview.appspot.com/4535118/diff/21003/src/com/google/caja/es5-guards.js#newcode182
src/com/google/caja/es5-guards.js:182: }
On 2011/07/06 20:48:27, MikeSamuel wrote:
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.
Done. This may just go away seeing as Confidence is nearly always a
better abstraction -- except insofar as this file should have feature
parity with es53.js's cajaVM.
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) {
On 2011/07/06 20:48:27, MikeSamuel wrote:
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.
Done.
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);
On 2011/07/06 20:48:27, MikeSamuel wrote:
Is this supposed to happen synchronously?
This code is unmodified, just moved (line 227 on the left).
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.
On 2011/07/06 20:48:27, MikeSamuel wrote:
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.
This function is only ever called on host DOM objects which aren't safe
to give to untrusted code anyway.
The alternative which Ihab and I discussed was to have a "perform
arbitrary operations on arbitrary objects" authority given to
Domado/Bridal/etc.; we decided this, "allow arbitrary operations to be
performed on this designated object", was safer.
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.)
On 2011/07/06 20:48:27, MikeSamuel wrote:
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.
Yes. There is no current example of a situation where makeDOMAccessible
would need to wrap or unwrap, but it was easy to let bridal support
that, so I did.
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)) {
On 2011/07/06 20:48:27, MikeSamuel wrote:
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?
This code is intended to be identical to es53.js's (!isNumericName(p) &&
!endsWith__.test(p)). I have no knowledge of why it should be one way or
the other.
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) {
On 2011/07/06 20:48:27, MikeSamuel wrote:
Is this a minimal set that you need for debugging? Why these methods
and not
group/ungroup?
It's a minimal set for debugging and presenting unusual events to the
end developer, and also the set of methods which have identical
interfaces to the one I needed.
Note that IIRC this is unsafe in that passing guest objects into the
Firebug console can cause invocation of their toString. For hostile uses
this needs to coerce all its arguments to string. I've made a note to
work on that.
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#newcode32
src/com/google/caja/plugin/html-emitter.js:32: * @param opt_domadoTools
the tools of the domado instance that will receive a
On 2011/07/06 20:48:27, MikeSamuel wrote:
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?
Tools is Domado-specific authorities to the document which used to be
accessible via privileged access to foo___ properties (i.e.
amplification) on the tameDocument. It actually only needs signalLoaded
and sanitizeAttrs.
I welcome suggestions of more generic interfaces that work given that
HtmlEmitter is now unprivileged.
On the other hand, tools is now the object that is returned by
attachDocumentStub2. It should probably therefore have a more
explanatory name. I'd call it a 'Domado instance' except that there is a
Domado() constructor already, which returns, not 'tools', but the object
of which attachDocumentStub2 is an instance. (I should also rename
attachDocumentStub2; the '2' is a relic.)
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);
On 2011/07/06 20:48:27, MikeSamuel wrote:
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.
In the actual membrane case (caja.js sets it up), makeDOMAccessible is a
side-effecting identity function.
What happens if makeDOMAccessible returns a value other than its
input?
bridal.js will function in that case; HtmlEmitter and Domado won't.
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.
On 2011/07/06 20:48:27, MikeSamuel wrote:
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.
Name conflicts by themselves are sufficient reason; I've made the
comment more generic.
http://codereview.appspot.com/4535118/
