Issue 198470043: SES no longer uses objToString.call(specimen) as a reliable brand test. (Closed)
|
Created:
9 years, 2 months ago by MarkM Modified:
9 years, 2 months ago CC:
google-caja-discuss_googlegroups.com Base URL:
http://google-caja.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionAs of ES6, Object.prototype.toString.call(specimen) is no longer a
reliable brand test. However,
https://esdiscuss.org/topic/tostringtag-spoofing-for-null-and-undefined#content-59
demonstrates another technique that is still reliable. For those
places containing old-style brand tests, either fix them or remove
them.
NEEDS_DUMMY_SETTER and ERRORS_HAVE_INVISIBLE_PROPERTIES were using
doing brand testing in their repairs. But since both of these were
repaired prior to 2014, I removed the repairs instead. The tests are
still in there, so we will still fail safe on ancient browsers that we
don't support, but may newly fail to repair them,
Patch Set 1 #Patch Set 2 : SES no longer uses objToString.call(specimen) as a reliable brand test. #
Total comments: 21
Patch Set 3 : SES no longer uses objToString.call(specimen) as a reliable brand test. #Patch Set 4 : SES no longer uses objToString.call(specimen) as a reliable brand test. #Patch Set 5 : SES no longer uses objToString.call(specimen) as a reliable brand test. #Patch Set 6 : SES no longer uses objToString.call(specimen) as a reliable brand test. #Patch Set 7 : SES no longer uses objToString.call(specimen) as a reliable brand test. #MessagesTotal messages: 13
As of ES6, Object.prototype.toString.call(specimen) is no longer a reliable brand test. However, https://esdiscuss.org/topic/tostringtag-spoofing-for-null-and-undefined#conte... demonstrates another technique that is still reliable. For those places containing old-style brand tests, either fix them or remove them. NEEDS_DUMMY_SETTER and ERRORS_HAVE_INVISIBLE_PROPERTIES were using doing brand testing in their repairs. But since both of these were repaired prior to 2014, I removed the repairs instead. The tests are still in there, so we will still fail safe on ancient browsers that we don't support, but may newly fail to repair them,
Sign in to reply to this message.
Note that this repairs only the brand tests in SES. Remaining brand tests elsewhere in Caja still need to be repaired.
Sign in to reply to this message.
As of ES6, Object.prototype.toString.call(specimen) is no longer a reliable brand test. However, https://esdiscuss.org/topic/tostringtag-spoofing-for-null-and-undefined#conte... demonstrates another technique that is still reliable. For those places containing old-style brand tests, either fix them or remove them. NEEDS_DUMMY_SETTER and ERRORS_HAVE_INVISIBLE_PROPERTIES were using doing brand testing in their repairs. But since both of these were repaired prior to 2014, I removed the repairs instead. The tests are still in there, so we will still fail safe on ancient browsers that we don't support, but may newly fail to repair them,
Sign in to reply to this message.
https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:488: * Using Allen's trick from This is an explaining-the-implementation comment more than a doc comment. I think having an actual doc comment specifying the semantics of the parameters would be worthwhile (noting things like that constr may be undefined). https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:491: * makeBrandTester runs at SES initialization time, but the Clarify: s/runs/is only called/. Also, this comment suggests that makeBrandTester is _not_ safe when called after SES initialization is complete. That would be surprising because it implies mutability. If it is not the case, why are you calling it out specially here? I'm interested because the taming membrane, for example, might like to reuse this utility for different builtins (e.g. typed arrays). https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:495: function makeBrandTester(constr, methodName, args, opt_example) { For consistency with the rest of the codebase and (I think) common usage elsewhere, I suggest using “ctor” instead of “constr”. https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:499: return function(specimen) { return false; } Suggest giving this function a descriptive name for the sake of debugging, like: function nonexistentTypeBrandTester(specimen) { ... https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:512: strictForEachFn([null, undefined, true, 1, 'x', {}], function(v) { for style consistency, void 0 instead of undefined? I'd like to see some hairier pretend-to-be-the-object test cases, like 1. a Proxy whose target would pass (but you may not want to depend on Proxy yet). 2. {valueOf() { return anActualInstance; }} https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:565: * "WeakMap", or, in ES6 terminology, whether it has a [[WeakMapData]] This description is arguably nonsense, because (assuming I have my facts straight) ES5 doesn't have WeakMap and ES6 doesn't have [[Class]], so [[Class]] never took on the value WeakMap in any specification. https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:570: global.WeakMap ? new WeakMap() : void 0); Horizontal alignment like this is undesirable, because it breaks under find-and-replace renaming. Put "makeBrandTester(" on the previous line instead. https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:1500: /** Either don't pretend this is a doc comment, or put one on each of the four functions. https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:3194: * <p>This only works when {@code constr} is the constructor of While we're touching this, I'd like to s/constr/ctor/ as discussed above. https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:4178: id: 'NUMBER_PROTO_IS_NUMBER', What specific browsers have these bugs, or is this purely a conformance check for the sake of accurate brands?
Sign in to reply to this message.
As of ES6, Object.prototype.toString.call(specimen) is no longer a reliable brand test. However, https://esdiscuss.org/topic/tostringtag-spoofing-for-null-and-undefined#conte... demonstrates another technique that is still reliable. For those places containing old-style brand tests, either fix them or remove them. NEEDS_DUMMY_SETTER and ERRORS_HAVE_INVISIBLE_PROPERTIES were using doing brand testing in their repairs. But since both of these were repaired prior to 2014, I removed the repairs instead. The tests are still in there, so we will still fail safe on ancient browsers that we don't support, but may newly fail to repair them,
Sign in to reply to this message.
PTAL https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:488: * Using Allen's trick from On 2015/02/13 21:30:37, kpreid_google wrote: > This is an explaining-the-implementation comment more than a doc comment. > > I think having an actual doc comment specifying the semantics of the parameters > would be worthwhile (noting things like that constr may be undefined). Done. https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:491: * makeBrandTester runs at SES initialization time, but the On 2015/02/13 21:30:37, kpreid_google wrote: > Clarify: s/runs/is only called/. > > Also, this comment suggests that makeBrandTester is _not_ safe when called after > SES initialization is complete. That would be surprising because it implies > mutability. If it is not the case, why are you calling it out specially here? > > I'm interested because the taming membrane, for example, might like to reuse > this utility for different builtins (e.g. typed arrays). Done. I put the qualifier in because, at the time, I only wrote it so that the brandTester was defensive. I hadn't worried about whether makeBrandTester was defensive when it ran. However, reading it now, I don't see any problem. Nevertheless, even if the taming membrane uses it, it should probably do all the needed calls to makeBrandTester at setup time anyway, before untrusted code runs. https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:495: function makeBrandTester(constr, methodName, args, opt_example) { On 2015/02/13 21:30:37, kpreid_google wrote: > For consistency with the rest of the codebase and (I think) common usage > elsewhere, I suggest using “ctor” instead of “constr”. Done. https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:499: return function(specimen) { return false; } On 2015/02/13 21:30:37, kpreid_google wrote: > Suggest giving this function a descriptive name for the sake of debugging, like: > > function nonexistentTypeBrandTester(specimen) { ... Done. https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:512: strictForEachFn([null, undefined, true, 1, 'x', {}], function(v) { On 2015/02/13 21:30:37, kpreid_google wrote: > for style consistency, void 0 instead of undefined? > > I'd like to see some hairier pretend-to-be-the-object test cases, like > > 1. a Proxy whose target would pass (but you may not want to depend on Proxy > yet). > > 2. {valueOf() { return anActualInstance; }} Extremely good idea! The proxy case fails on FF 35.0.1 which we must still support, so I added extra logic to survive when that self-test fails, and added comments explaining why this is currently safe. All other browsers I tested so far -- Chrome, FF Nightly, Safari, and Opera pass fine. I won't be able to test IE until tomorrow, but I will test it before submitting. So, with these caveats, Done. PTAL. https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:565: * "WeakMap", or, in ES6 terminology, whether it has a [[WeakMapData]] On 2015/02/13 21:30:37, kpreid_google wrote: > This description is arguably nonsense, because (assuming I have my facts > straight) ES5 doesn't have WeakMap and ES6 doesn't have [[Class]], so [[Class]] > never took on the value WeakMap in any specification. Correct. Done. https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:570: global.WeakMap ? new WeakMap() : void 0); On 2015/02/13 21:30:37, kpreid_google wrote: > Horizontal alignment like this is undesirable, because it breaks under > find-and-replace renaming. > Put "makeBrandTester(" on the previous line instead. Done. https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:1500: /** On 2015/02/13 21:30:37, kpreid_google wrote: > Either don't pretend this is a doc comment, or put one on each of the four > functions. Done. https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:3194: * <p>This only works when {@code constr} is the constructor of On 2015/02/13 21:30:37, kpreid_google wrote: > While we're touching this, I'd like to s/constr/ctor/ as discussed above. Done. https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:4178: id: 'NUMBER_PROTO_IS_NUMBER', On 2015/02/13 21:30:37, kpreid_google wrote: > What specific browsers have these bugs, or is this purely a conformance check > for the sake of accurate brands? As of this writing, at least Chrome, FF, Safari, and Opera violate these. I haven't yet tested IE.
Sign in to reply to this message.
As of ES6, Object.prototype.toString.call(specimen) is no longer a reliable brand test. However, https://esdiscuss.org/topic/tostringtag-spoofing-for-null-and-undefined#conte... demonstrates another technique that is still reliable. For those places containing old-style brand tests, either fix them or remove them. NEEDS_DUMMY_SETTER and ERRORS_HAVE_INVISIBLE_PROPERTIES were using doing brand testing in their repairs. But since both of these were repaired prior to 2014, I removed the repairs instead. The tests are still in there, so we will still fail safe on ancient browsers that we don't support, but may newly fail to repair them,
Sign in to reply to this message.
https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... File src/com/google/caja/ses/repairES5.js (right): https://codereview.appspot.com/198470043/diff/20001/src/com/google/caja/ses/r... src/com/google/caja/ses/repairES5.js:512: strictForEachFn([null, undefined, true, 1, 'x', {}], function(v) { On 2015/02/14 00:38:46, MarkM wrote: > On 2015/02/13 21:30:37, kpreid_google wrote: > > for style consistency, void 0 instead of undefined? > > > > I'd like to see some hairier pretend-to-be-the-object test cases, like > > > > 1. a Proxy whose target would pass (but you may not want to depend on Proxy > > yet). > > > > 2. {valueOf() { return anActualInstance; }} > > Extremely good idea! The proxy case fails on FF 35.0.1 which we must still > support, so I added extra logic to survive when that self-test fails, and added > comments explaining why this is currently safe. All other browsers I tested so > far -- Chrome, FF Nightly, Safari, and Opera pass fine. I won't be able to test > IE until tomorrow, but I will test it before submitting. > > So, with these caveats, Done. PTAL. Works fine on IE11. New snapshot for other updated comments re IE11 testing.
Sign in to reply to this message.
As of ES6, Object.prototype.toString.call(specimen) is no longer a reliable brand test. However, https://esdiscuss.org/topic/tostringtag-spoofing-for-null-and-undefined#conte... demonstrates another technique that is still reliable. For those places containing old-style brand tests, either fix them or remove them. NEEDS_DUMMY_SETTER and ERRORS_HAVE_INVISIBLE_PROPERTIES were using doing brand testing in their repairs. But since both of these were repaired prior to 2014, I removed the repairs instead. The tests are still in there, so we will still fail safe on ancient browsers that we don't support, but may newly fail to repair them,
Sign in to reply to this message.
Reported issues relevant to this CL. Updated CL to include links to those issue reports.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
As of ES6, Object.prototype.toString.call(specimen) is no longer a reliable brand test. However, https://esdiscuss.org/topic/tostringtag-spoofing-for-null-and-undefined#conte... demonstrates another technique that is still reliable. For those places containing old-style brand tests, either fix them or remove them. NEEDS_DUMMY_SETTER and ERRORS_HAVE_INVISIBLE_PROPERTIES were using doing brand testing in their repairs. But since both of these were repaired prior to 2014, I removed the repairs instead. The tests are still in there, so we will still fail safe on ancient browsers that we don't support, but may newly fail to repair them,
Sign in to reply to this message.
|
