Issue 14605043: Remove ES5/3 artifacts from taming membrane (Closed)
|
Created:
12 years, 5 months ago by ihab.awad Modified:
12 years, 4 months ago Reviewers:
kpreid_google CC:
google-caja-discuss_googlegroups.com Base URL:
http://google-caja.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionRemove ES5/3 artifacts from taming membrane
Patch Set 1 #
Total comments: 20
Patch Set 2 : Remove ES5/3 artifacts from taming membrane #
Total comments: 8
Patch Set 3 : Remove ES5/3 artifacts from taming membrane #
MessagesTotal messages: 9
https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/apitaming/... File src/com/google/caja/apitaming/google.visualization.policyFactory.js (right): https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/apitaming/... src/com/google/caja/apitaming/google.visualization.policyFactory.js:419: return [ args[0].apply(Object.freeze({}), []) ]; Revert this if you agree with my other comments. https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/ses... File src/com/google/caja/plugin/ses-frame-group.js (right): https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/ses... src/com/google/caja/plugin/ses-frame-group.js:114: Object.freeze({USELESS: 'USELESS'}), I think this should use a singleton USELESS, because constructing a frozen object is more expensive than we want for a single function call. The rationale explained elsewhere should also be explained here. https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... File src/com/google/caja/plugin/taming-membrane.js (right): https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... src/com/google/caja/plugin/taming-membrane.js:40: function directConstructor(obj) { Maybe document this function, or at least what BASE_OBJECT_CONSTRUCTOR means? https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... src/com/google/caja/plugin/taming-membrane.js:94: // then check for enumerability? This looks equivalent to Object.keys. https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... src/com/google/caja/plugin/taming-membrane.js:448: // CAUTION: It is ESSENTIAL that we pass USELESS, not (void 0), when Suggest moving this comment to the definition(s) of USELESS. https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... src/com/google/caja/plugin/taming-membrane.js:465: var t = function (_) { should be function(var_args) (no space, that particular arg name), yes? https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... File src/com/google/caja/plugin/taming-schema.js (left): https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... src/com/google/caja/plugin/taming-schema.js:111: if (privilegedAccess.isDefinedInCajaFrame(f)) { Why is this constraint no longer needed? https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... File src/com/google/caja/plugin/taming-schema.js (right): https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... src/com/google/caja/plugin/taming-schema.js:202: return p(self, advice.apply((void 0), [f, self, args])); Does this (and the 2 below) even need to be apply instead of a plain function call? https://codereview.appspot.com/14605043/diff/1/tests/com/google/caja/plugin/t... File tests/com/google/caja/plugin/test-defensible-objects.js (right): https://codereview.appspot.com/14605043/diff/1/tests/com/google/caja/plugin/t... tests/com/google/caja/plugin/test-defensible-objects.js:38: assertEquals('USELESS', this.USELESS); Ew. I'd rather see there be a singleton USELESS. User code might find a need to reliably recognize it; in fact, if any such uses already exist, removing caja.USELESS is a breaking change.
Sign in to reply to this message.
https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/apitaming/... File src/com/google/caja/apitaming/google.visualization.policyFactory.js (right): https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/apitaming/... src/com/google/caja/apitaming/google.visualization.policyFactory.js:419: return [ args[0].apply(Object.freeze({}), []) ]; On 2013/10/11 01:16:27, kpreid_google wrote: > Revert this if you agree with my other comments. Done. https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/ses... File src/com/google/caja/plugin/ses-frame-group.js (right): https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/ses... src/com/google/caja/plugin/ses-frame-group.js:114: Object.freeze({USELESS: 'USELESS'}), On 2013/10/11 01:16:27, kpreid_google wrote: > I think this should use a singleton USELESS, because constructing a frozen > object is more expensive than we want for a single function call. > > The rationale explained elsewhere should also be explained here. Done. https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... File src/com/google/caja/plugin/taming-membrane.js (right): https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... src/com/google/caja/plugin/taming-membrane.js:40: function directConstructor(obj) { On 2013/10/11 01:16:27, kpreid_google wrote: > Maybe document this function, or at least what BASE_OBJECT_CONSTRUCTOR means? Done. https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... src/com/google/caja/plugin/taming-membrane.js:94: // then check for enumerability? On 2013/10/11 01:16:27, kpreid_google wrote: > This looks equivalent to Object.keys. Done. https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... src/com/google/caja/plugin/taming-membrane.js:448: // CAUTION: It is ESSENTIAL that we pass USELESS, not (void 0), when On 2013/10/11 01:16:27, kpreid_google wrote: > Suggest moving this comment to the definition(s) of USELESS. Done. https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... src/com/google/caja/plugin/taming-membrane.js:465: var t = function (_) { On 2013/10/11 01:16:27, kpreid_google wrote: > should be function(var_args) (no space, that particular arg name), yes? Not part of this CL and pervasive. https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... File src/com/google/caja/plugin/taming-schema.js (left): https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... src/com/google/caja/plugin/taming-schema.js:111: if (privilegedAccess.isDefinedInCajaFrame(f)) { On 2013/10/11 01:16:27, kpreid_google wrote: > Why is this constraint no longer needed? There is now no foundational difference between objects anywhere, and we may choose to use Caja in a mode where we bundle the taming and guest code into the same frame. https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... File src/com/google/caja/plugin/taming-schema.js (right): https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... src/com/google/caja/plugin/taming-schema.js:202: return p(self, advice.apply((void 0), [f, self, args])); On 2013/10/11 01:16:27, kpreid_google wrote: > Does this (and the 2 below) even need to be apply instead of a plain function > call? Done. https://codereview.appspot.com/14605043/diff/1/tests/com/google/caja/plugin/t... File tests/com/google/caja/plugin/test-defensible-objects.js (right): https://codereview.appspot.com/14605043/diff/1/tests/com/google/caja/plugin/t... tests/com/google/caja/plugin/test-defensible-objects.js:38: assertEquals('USELESS', this.USELESS); Sigh. I started this CL hoping to Bobbitt the whole idea of USELESS once and for good. I failed. I then figured, well at least we should not have this thing that we pass around all over tarnation. Looking back at the carnage, I guess that was a ... um ... USELESS goal. :P
Sign in to reply to this message.
https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... File src/com/google/caja/plugin/taming-schema.js (left): https://codereview.appspot.com/14605043/diff/1/src/com/google/caja/plugin/tam... src/com/google/caja/plugin/taming-schema.js:111: if (privilegedAccess.isDefinedInCajaFrame(f)) { On 2013/10/11 20:39:55, ihab.awad wrote: > On 2013/10/11 01:16:27, kpreid_google wrote: > > Why is this constraint no longer needed? > > There is now no foundational difference between objects anywhere, and we may > choose to use Caja in a mode where we bundle the taming and guest code into > the same frame. We already do: post-ES5/3, there are never distinct taming and guest frames. And that is irrelevant, because this restriction is about host vs. Caja frames, not taming vs. guest frames. I am leery of removing this restriction because it might create confusion about whether something is a 'native' cap-styled object vs. one in need of taming. I would prefer to consider it separately. Maybe add a TODO or issue to review the idea? https://codereview.appspot.com/14605043/diff/1/tests/com/google/caja/plugin/t... File tests/com/google/caja/plugin/test-defensible-objects.js (right): https://codereview.appspot.com/14605043/diff/1/tests/com/google/caja/plugin/t... tests/com/google/caja/plugin/test-defensible-objects.js:38: assertEquals('USELESS', this.USELESS); On 2013/10/11 20:39:55, ihab.awad wrote: > I started this CL hoping to Bobbitt the whole idea of USELESS once and for > good. I failed. I then figured, well at least we should not have this > thing that we pass around all over tarnation. Looking back at the carnage, > I guess that was a ... um ... USELESS goal. :P I agree with all of the above. https://codereview.appspot.com/14605043/diff/9001/src/com/google/caja/plugin/... File src/com/google/caja/plugin/ses-frame-group.js (right): https://codereview.appspot.com/14605043/diff/9001/src/com/google/caja/plugin/... src/com/google/caja/plugin/ses-frame-group.js:41: // we called it with (void 0), which would be a serious vulnerability. "_Would_ be a serious vulnerability" is both too strong and unclear about what the problem actually is. There is only a vulnerability if the function is exophoric (and ends up doing something non-harmless to its this). https://codereview.appspot.com/14605043/diff/9001/src/com/google/caja/plugin/... src/com/google/caja/plugin/ses-frame-group.js:42: var USELESS = Object.freeze({ USELESS: 'USELESS' }); Not relevant per se, but I think it'd be nice to give this a toString. ES5/3's USELESS had one. https://codereview.appspot.com/14605043/diff/9001/src/com/google/caja/plugin/... File src/com/google/caja/plugin/taming-membrane.js (right): https://codereview.appspot.com/14605043/diff/9001/src/com/google/caja/plugin/... src/com/google/caja/plugin/taming-membrane.js:22: function TamingMembrane(privilegedAccess, schema) { I note that the name privilegedAccess is somewhat less fitting than it used to be (but allFrames and weakMapPermitHostObjects are still a big deal). No need to change the name now, but something to think about. https://codereview.appspot.com/14605043/diff/9001/src/com/google/caja/plugin/... File src/com/google/caja/plugin/taming-schema.js (left): https://codereview.appspot.com/14605043/diff/9001/src/com/google/caja/plugin/... src/com/google/caja/plugin/taming-schema.js:111: if (privilegedAccess.isDefinedInCajaFrame(f)) { As noted on previous version, please leave this in for now.
Sign in to reply to this message.
https://codereview.appspot.com/14605043/diff/9001/src/com/google/caja/plugin/... File src/com/google/caja/plugin/ses-frame-group.js (right): https://codereview.appspot.com/14605043/diff/9001/src/com/google/caja/plugin/... src/com/google/caja/plugin/ses-frame-group.js:41: // we called it with (void 0), which would be a serious vulnerability. On 2013/10/11 21:54:53, kpreid_google wrote: > "_Would_ be a serious vulnerability" is both too strong and unclear about what > the problem actually is. There is only a vulnerability if the function is > exophoric (and ends up doing something non-harmless to its this). Done. https://codereview.appspot.com/14605043/diff/9001/src/com/google/caja/plugin/... src/com/google/caja/plugin/ses-frame-group.js:42: var USELESS = Object.freeze({ USELESS: 'USELESS' }); On 2013/10/11 21:54:53, kpreid_google wrote: > Not relevant per se, but I think it'd be nice to give this a toString. ES5/3's > USELESS had one. Done. https://codereview.appspot.com/14605043/diff/9001/src/com/google/caja/plugin/... File src/com/google/caja/plugin/taming-membrane.js (right): https://codereview.appspot.com/14605043/diff/9001/src/com/google/caja/plugin/... src/com/google/caja/plugin/taming-membrane.js:22: function TamingMembrane(privilegedAccess, schema) { On 2013/10/11 21:54:53, kpreid_google wrote: > I note that the name privilegedAccess is somewhat less fitting than it used to > be (but allFrames and weakMapPermitHostObjects are still a big deal). No need to > change the name now, but something to think about. Done. https://codereview.appspot.com/14605043/diff/9001/src/com/google/caja/plugin/... File src/com/google/caja/plugin/taming-schema.js (left): https://codereview.appspot.com/14605043/diff/9001/src/com/google/caja/plugin/... src/com/google/caja/plugin/taming-schema.js:111: if (privilegedAccess.isDefinedInCajaFrame(f)) { On 2013/10/11 21:54:53, kpreid_google wrote: > As noted on previous version, please leave this in for now. Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
