Issue 4249052: First checkin of SES to Caja source tree. (Closed)
|
Created:
14 years, 2 months ago by MarkM Modified:
13 years, 12 months ago CC:
google-caja-discuss_googlegroups.com Base URL:
http://google-caja.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionTurns an ES5 environment (e.g., browser frame) into a SES environment
following object-capability rules.
Patch Set 1 #
Total comments: 6
Patch Set 2 : First checkin of SES to Caja source tree. #
Total comments: 75
Patch Set 3 : First checkin of SES to Caja source tree. #Patch Set 4 : First checkin of SES to Caja source tree. #Patch Set 5 : First checkin of SES to Caja source tree. #Patch Set 6 : First checkin of SES to Caja source tree. #Patch Set 7 : First checkin of SES to Caja source tree. #Patch Set 8 : First checkin of SES to Caja source tree. #Patch Set 9 : First checkin of SES to Caja source tree. #Patch Set 10 : First checkin of SES to Caja source tree. #Patch Set 11 : First checkin of SES to Caja source tree. #
Total comments: 66
Patch Set 12 : First checkin of SES to Caja source tree. #
Total comments: 2
Patch Set 13 : First checkin of SES to Caja source tree. #Patch Set 14 : First checkin of SES to Caja source tree. #Patch Set 15 : First checkin of SES to Caja source tree. #
MessagesTotal messages: 20
Some thoughts on the Unicode source issue http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/atLeastF... File src/com/google/caja/ses/atLeastFreeVarNames.js (right): http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/atLeastF... src/com/google/caja/ses/atLeastFreeVarNames.js:62: var SHOULD_MATCH_IDENTIFIER = (/(\w|\$)+/gm); A somewhat more liberal match would be /([$\w\u0100-\uFFFF]+)/gm This would cause some legal ES5 whitespace to be included as part of identifiers. We could exclude those in LIMIT_SRC above: U+2028, U+2029, U+FEFF and other characters with Unicode property Zs (U+2000 ~ U+200B, U+3000 according to unicode.js mentioned above). At that point, I think, this match will be more liberal than ES5, but only in that it matches some character sequences that ES5 would reject as not valid identifiers.
Sign in to reply to this message.
Two more findings. Also: UNIT TESTS?!?!?! This code needs unit tests! http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/atLeastF... File src/com/google/caja/ses/atLeastFreeVarNames.js (right): http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/atLeastF... src/com/google/caja/ses/atLeastFreeVarNames.js:46: if (!((/^[\u0000-\u007f]*$/m).test(programSrc))) { This doesn't appear to actually work! I have programSrc with characters beyond U+007f and it passes this test. I don't think you want /m -- since this makes ^ & $ match a line - and .test() can certainly find at least one line in my program source that is wholly ASCII. http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/startSES.js File src/com/google/caja/ses/startSES.js (right): http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/startSES... src/com/google/caja/ses/startSES.js:205: for (var name in from) { On Chrome 9, when this is called with root (see line 232), the properties are already non-enumerable. As such, this for loop fails to find any of the globals except CajaVM and WeakMap. The following code fixes this issue, though I'm not sure if I'm changing the expectations: var names = Object.getOwnPropertyNames(from); for (var i in names) { var name = names[i];
Sign in to reply to this message.
I think the TOLERATE_MISSING_DESCRIPTOR logic is wrong. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... File src/com/google/caja/ses/startSES.js (right): http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:207: if (!desc && TOLERATE_MISSING_DESCRIPTOR) { return; } Shouldn't this be !TOLERATE_MISSING_DESCRIPTOR? In otherwords: If there is no desc, and TOLERATE_MISSING_DESCRIPTOR is false... THEN we want to just.. exit? or just skip this one? If exit, then surely there should be an error message logged? if just skip this one, then the if logic needs to be rewritten. And, when TOLERATE_MISSING_DESCRIPTOR is true, don't we want to still defineProperty() in this case... but then this code will need to create a descriptor object, rather than attempting to set .enumerable and .configurable on null/undefined (or what ever desc is in this case).
Sign in to reply to this message.
In general, this is really cool! Some notes: * As noted throughout, there needs to be more description and clarification for various pieces. * I noted this in one spot, but generally, refactoring the files to put related functionality together would be good. Specifically, WeakMap.js contains a lot of general stuff that ought to be merged with the similar stuff in startSES.js. * It would be great if the number of global symbols exported by this set of files as a whole were limited to the barest minimum. It's ok if we at least have a *plan* for doing this, e.g., by concatenation with some boilerplate and putting a function wrapper around everything. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... File src/com/google/caja/ses/WeakMap.js (right): http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:25: * retain maps indexed by that key and (crucially) a map does not ... retain *values* indexed ... http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:46: * <p>Will also install some other elements of ES5 as needed to bring It would be nice if these were in separate files. It turns out that the minimal WeakMap-related stuff here is pretty compact. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:60: 'which implements Object.getOwnPropertyName'); ... getOwnPropertyName*s* ... http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:67: var classProp = Object.prototype.toString; Unused. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:69: var real = {}; I would find a name like "realObject" or "RealObject" more descriptive. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:79: enumerable: false, Why not enumerable? http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:82: if (base === Object) { I found the stealth special-casing here a bit confusing; is there another way to do the same thing? If not then never mind, but just a thought.... http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:119: var UnsafeRegExp = RegExp; What happens with regex literals in SES code? Surely they are not instances of FakeRegExp? http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:135: global.RegExp = FakeRegExp; Code like this also occurs in startSES.js; the monkey patching of globals should really be all consolidated in one place. If there's stuff to support a particular feature, I think it would be clearer to note there ("to support WeakMaps" or whatever). http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:166: delete funcBound.prototype; Why? http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/atLea... File src/com/google/caja/ses/atLeastFreeVarNames.js (right): http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/atLea... src/com/google/caja/ses/atLeastFreeVarNames.js:36: // "use strict"; // not here because of an unreported Caja bug What bug? http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/atLea... src/com/google/caja/ses/atLeastFreeVarNames.js:77: throw new EvalError('Apparent identifier "ident___" not permitted'); I think there's a vuln here. Since SHOULD_MATCH_IDENTIFIER is shared, if you throw at the i-th identifier for one invocation of the function, then the next invocation will fail to look at the first (i) identifiers. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... File src/com/google/caja/ses/startSES.js (right): http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:1: // Copyright (C) 2009 Google Inc. Copyrights in files seem suspect, throughout. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:67: // "use strict"; // not here because of an unreported Caja bug Please report & insert reference. :) http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:92: var FREEZE_EARLY = Array.prototype.forEach; To aid readability, FREEZE_EARLY should at least be an array. Otherwise, it reads like it's a boolean flag, leading to confusion about the rvalue. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:110: var TOLERATE_MISSING_DESCRIPTOR = true; Instead of defining a general-looking flag, would it be possible to special-case the specific item ("callee") instead, since it's the only known occurrence of the problem? http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:125: if (REQUIRE_STRICT && function(){return this;}()) { Nit: spaces inside function body, etc. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:182: verifyStrictProgram(exprSrc + ';'); I think these tests need some more description. For this one, I assume that you are verifying that exprSrc is either (a) a list of Statements; (b) a list of Statements with the final semicolon missing; or (c) an Expression. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:183: verifyStrictProgram('( ' + exprSrc + '\n);'); Now you verify that it is an Expression, since it can parse correctly surrounded by the parentheses. But what's the newline for? http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:193: * properties of {@code env} becomes an accessor of {@code s/an// http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:195: * use their {@code this} value, then the original and any copied How are you sure the getters and setters you plan to encounter don't use their "this"? A note justifying the assumption might be helpful. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:227: * scope. Each decision violates some assumptions. Could you note what the violated assumptions are for each case? http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:254: enumerable: true Why is enumerability important? This is only used to satisfy free variable references on demand, right? http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:274: * the platform such as an parser API that provides an AST. s/an/a/ http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:313: ' );\n' + Why only newlines at the end of syntactic groups, even when they are not strictly needed? http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:444: global.eval = fakeEval; Should explain in a comment (as you did to me) why corresponding properties of the *real* global object need to be modified as well, given that SES code is always run in a virtualized "global" scope. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:527: }); Per our discussion, please add a comment describing that this is to make the property immutable such that future read()s get the same value -- and that the code below relies on this fact. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:565: * <p>To ensure that each subsequent traverse obtains the same s/traverse/traversal/ http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:575: if (oldPermit) { This looks like mostly a diagnostic -- to see if you are getting to the same value by distinct paths. Perhaps rewrite so it's clear that this is what's happening, and log a warning or something.... To me, just "oldPermit" doesn't explain what you're trying to get at. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:581: if (permit[name] !== 'skip') { I'm confused about this. If permit[name] === '*' or (true), then the register() call 2 lines below will be a no-op anyway due to the value !== Object(value) check at the top. This check seems like it can be dropped. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:590: * We initialize the whiteTable only so that we can process "*" and This is not a description of function isPermitted() ... I'm not sure this comment belongs here. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:597: if (permit[name]) { return permit[name]; } If permit[name] is an Object, you return that anyway -- but simply as a synonym for true (i.e., expecting it to be evaluated for truthiness only). This caused me confusion regarding the expected range of function isPermitted(). Perhaps refactor to clarify? Or at least document the range of this function in the comments? Also, isPermitted() implies it returns *only* a boolean value, whereas this function returns something more funky than that.... Anyway: as I understand it, the range of isPermitted() is: { /* ... */ } /* i.e. truthy */ true '*' 'skip' false For clarity, I think the {} and '*' cases should be explicitly returned as true. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:624: function clean(value, n) { The identifiers 'n' and 'path' are confusing to me ... perhaps 'path' and 'childPath' or something? http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:685: reportDiagnosis("Can't neuter", complaint); Prefer consistent single quotes for strings. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/white... File src/com/google/caja/ses/whitelist.js (right): http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/white... src/com/google/caja/ses/whitelist.js:42: * </ul> Per our discussion: the 3 common cases, [ true, "*", {} ], should be described by an example, and the "skip" case should be explained as a temporary escape hatch that we hope will go away by the time this stuff is productionized. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/white... src/com/google/caja/ses/whitelist.js:42: * </ul> It seems impossible for an entry to have the meanings of both "*" (inherit the whitelisting of this property) and a record (whitelist the value of this property according to the following criteria). It seems therefore that there exist whitelists which cannot be expressed by this schema. Have we just not encountered them? Yet? Or is this by design?
Sign in to reply to this message.
http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... File src/com/google/caja/ses/startSES.js (right): http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:67: // "use strict"; // not here because of an unreported Caja bug On 2011/03/16 21:17:11, ihab.awad wrote: > Please report & insert reference. :) http://code.google.com/p/google-caja/issues/detail?id=1349
Sign in to reply to this message.
Even better! Thanks Mike! :) -- Ihab On Mon, Apr 25, 2011 at 6:25 PM, <mikesamuel@gmail.com> wrote: > > http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... > File src/com/google/caja/ses/startSES.js (right): > > http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... > src/com/google/caja/ses/startSES.js:67: // Â "use strict"; // not here > because of an unreported Caja bug > On 2011/03/16 21:17:11, ihab.awad wrote: >> >> Please report & insert reference. :) > > http://code.google.com/p/google-caja/issues/detail?id=1349 > > http://codereview.appspot.com/4249052/ > -- Ihab A.B. Awad, Palo Alto, CA
Sign in to reply to this message.
Not yet ready for next review. Soon. http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/startSES.js File src/com/google/caja/ses/startSES.js (right): http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/startSES... src/com/google/caja/ses/startSES.js:205: for (var name in from) { On 2011/03/04 23:12:19, mzero wrote: > On Chrome 9, when this is called with root (see line 232), the properties are > already non-enumerable. As such, this for loop fails to find any of the globals > except CajaVM and WeakMap. > > The following code fixes this issue, though I'm not sure if I'm changing the > expectations: > > var names = Object.getOwnPropertyNames(from); > for (var i in names) { > var name = names[i]; Done. And other related issues regarding enumerability rethought. Please have a look. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:1: // Copyright (C) 2009 Google Inc. On 2011/03/16 21:17:11, ihab.awad wrote: > Copyrights in files seem suspect, throughout. Done. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:67: // "use strict"; // not here because of an unreported Caja bug On 2011/03/16 21:17:11, ihab.awad wrote: > Please report & insert reference. :) Reported. Fixed by mikesamuel. Uncommented. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:92: var FREEZE_EARLY = Array.prototype.forEach; On 2011/03/16 21:17:11, ihab.awad wrote: > To aid readability, FREEZE_EARLY should at least be an array. Otherwise, it > reads like it's a boolean flag, leading to confusion about the rvalue. Done. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:110: var TOLERATE_MISSING_DESCRIPTOR = true; On 2011/03/16 21:17:11, ihab.awad wrote: > Instead of defining a general-looking flag, would it be possible to special-case > the specific item ("callee") instead, since it's the only known occurrence of > the problem? Done. Also moved the handling into WeakMap.js, where it is used to install a fixed Object.getOwnPropertyNames without this bug. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:125: if (REQUIRE_STRICT && function(){return this;}()) { On 2011/03/16 21:17:11, ihab.awad wrote: > Nit: spaces inside function body, etc. Done. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:207: if (desc) { On 2011/03/07 23:26:40, mzero wrote: > Shouldn't this be !TOLERATE_MISSING_DESCRIPTOR? In otherwords: If there is no > desc, and TOLERATE_MISSING_DESCRIPTOR is false... THEN we want to just.. exit? > or just skip this one? > > If exit, then surely there should be an error message logged? > if just skip this one, then the if logic needs to be rewritten. > > And, when TOLERATE_MISSING_DESCRIPTOR is true, don't we want to still > defineProperty() in this case... but then this code will need to create a > descriptor object, rather than attempting to set .enumerable and .configurable > on null/undefined (or what ever desc is in this case). This issue has been moved to WeakMap.js, renamed TOLERATE_MISSING_CALLEE_DESCRIPTOR, and been reworked to fix Object.getOwnPropertyNames. Please have a look. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:274: * practical as a library without some non-standard support from On 2011/03/16 21:17:11, ihab.awad wrote: > s/an/a/ Done. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:313: } On 2011/03/16 21:17:11, ihab.awad wrote: > Why only newlines at the end of syntactic groups, even when they are not > strictly needed? Readability in case it shows up in a debugger. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:565: } On 2011/03/16 21:17:11, ihab.awad wrote: > s/traverse/traversal/ Done. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:575: function isPermitted(base, name) { On 2011/03/16 21:17:11, ihab.awad wrote: > This looks like mostly a diagnostic -- to see if you are getting to the same > value by distinct paths. Perhaps rewrite so it's clear that this is what's > happening, and log a warning or something.... To me, just "oldPermit" doesn't > explain what you're trying to get at. Done. Made it a fatal error. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:685: On 2011/03/16 21:17:11, ihab.awad wrote: > Prefer consistent single quotes for strings. Done.
Sign in to reply to this message.
still not ready. closer. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... File src/com/google/caja/ses/WeakMap.js (right): http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:25: * retain maps indexed by that key and (crucially) a map does not On 2011/03/16 21:17:11, ihab.awad wrote: > ... retain *values* indexed ... No, it does retain a value so long as both the map and the corresponding key is retained. The interesting thing here is that a WeakMap, and even this emulation, does not retain the *keys* of the WeakMap. If key K is used as an index in WeakMap W, then K can be collected even when W is not garbage. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:60: 'which implements Object.getOwnPropertyName'); On 2011/03/16 21:17:11, ihab.awad wrote: > ... getOwnPropertyName*s* ... Done. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:67: var classProp = Object.prototype.toString; On 2011/03/16 21:17:11, ihab.awad wrote: > Unused. Done. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:69: var real = {}; On 2011/03/16 21:17:11, ihab.awad wrote: > I would find a name like "realObject" or "RealObject" more descriptive. Done. Called it originalProps, since it is the properties of this object that matter, not the object itself. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:79: enumerable: false, On 2011/03/16 21:17:11, ihab.awad wrote: > Why not enumerable? Because this is used to install the equivalent of the Chapter 15 built in methods, which are defined as non-enumerable. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:82: if (base === Object) { On 2011/03/16 21:17:11, ihab.awad wrote: > I found the stealth special-casing here a bit confusing; is there another way to > do the same thing? If not then never mind, but just a thought.... Added a TODO to investigate. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:119: var UnsafeRegExp = RegExp; On 2011/03/16 21:17:11, ihab.awad wrote: > What happens with regex literals in SES code? Surely they are not instances of > FakeRegExp? Actually they are, since X instanceof Y just tests whether X inherits from Y.prototype. Since FakeRegExp.prototype === UnsafeRegExp.prototype, everything's cool. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:135: global.RegExp = FakeRegExp; On 2011/03/16 21:17:11, ihab.awad wrote: > Code like this also occurs in startSES.js; the monkey patching of globals should > really be all consolidated in one place. If there's stuff to support a > particular feature, I think it would be clearer to note there ("to support > WeakMaps" or whatever). As the comment above says, we're monkey patching away the global RegExp constructor because of https://bugzilla.mozilla.org/show_bug.cgi?id=591846 . In startSES.js, we use a similar pattern to monkey patch away the Function constructor, but for a completely separate reason -- because we assume that it is stds conformant, making it dangerous. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM... src/com/google/caja/ses/WeakMap.js:166: delete funcBound.prototype; On 2011/03/16 21:17:11, ihab.awad wrote: > Why? According to the Note at the end of 15.3.4.5: "Function objects created using Function.prototype.bind do not have a prototype property [...]" http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/atLea... File src/com/google/caja/ses/atLeastFreeVarNames.js (right): http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/atLea... src/com/google/caja/ses/atLeastFreeVarNames.js:36: // "use strict"; // not here because of an unreported Caja bug On 2011/03/16 21:17:11, ihab.awad wrote: > What bug? Done. Fixed because of mikesamuel's fix to http://code.google.com/p/google-caja/issues/detail?id=1349
Sign in to reply to this message.
Still not ready for review.
Separated WeakMap.js into es5shim.js and WeakMap.js according to Ihab's
suggestion below.
Caught and fixed a bug where I was using 'in' when I should have been doing an
own property check. (The bug was causing deletion of, for example,
URIError.prototype.constructor because the permit returned by isPermitted
inherits from Object.prototype, so ('constructor' in permit) was accidentally
true.)
Made work with the new WeakMap API available on FF6.0a1.
Found security hole in their API, documented in WeakMap.js, and started
correspondence with Mozilla about hazard.
When on platform with that security hole (only FF6.0a1 for now), wrap in a
mostly compatible safe wrapper which repairs the problem.
Made WeakMap emulation in WeakMap.js mirror that repaired API.
http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM...
File src/com/google/caja/ses/WeakMap.js (right):
http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/WeakM...
src/com/google/caja/ses/WeakMap.js:46: * <p>Will also install some other
elements of ES5 as needed to bring
On 2011/03/16 21:17:11, ihab.awad wrote:
> It would be nice if these were in separate files. It turns out that the
minimal
> WeakMap-related stuff here is pretty compact.
Done. All the non-WeakMap-specific logic has been moved into an earlier
es5shim.js.
Sign in to reply to this message.
Still not ready. Soon. http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/atLeastF... File src/com/google/caja/ses/atLeastFreeVarNames.js (right): http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/atLeastF... src/com/google/caja/ses/atLeastFreeVarNames.js:46: if (!((/^[\u0000-\u007f]*$/m).test(programSrc))) { On 2011/03/04 23:12:19, mzero wrote: > This doesn't appear to actually work! I have programSrc with characters beyond > U+007f and it passes this test. I don't think you want /m -- since this makes ^ > & $ match a line - and .test() can certainly find at least one line in my > program source that is wholly ASCII. Done. http://codereview.appspot.com/4249052/diff/1/src/com/google/caja/ses/atLeastF... src/com/google/caja/ses/atLeastFreeVarNames.js:62: var SHOULD_MATCH_IDENTIFIER = (/(\w|\$)+/gm); On 2011/03/04 22:03:03, mzero wrote: > A somewhat more liberal match would be > /([$\w\u0100-\uFFFF]+)/gm > > This would cause some legal ES5 whitespace to be included as part of > identifiers. > We could exclude those in LIMIT_SRC above: U+2028, U+2029, U+FEFF and other > characters with Unicode property Zs (U+2000 ~ U+200B, U+3000 according to > unicode.js mentioned above). > > At that point, I think, this match will be more liberal than ES5, but only in > that it matches some character sequences that ES5 would reject as not valid > identifiers. Having repaired the ascii-only test, I'm going to postpone further unicode issues for now. I added a TODO. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/atLea... src/com/google/caja/ses/atLeastFreeVarNames.js:77: throw new EvalError('Apparent identifier "ident___" not permitted'); On 2011/03/16 21:17:11, ihab.awad wrote: > I think there's a vuln here. Since SHOULD_MATCH_IDENTIFIER is shared, if you > throw at the i-th identifier for one invocation of the function, then the next > invocation will fail to look at the first (i) identifiers. Done. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... File src/com/google/caja/ses/startSES.js (right): http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:182: verifyStrictProgram(exprSrc + ';'); On 2011/03/16 21:17:11, ihab.awad wrote: > I think these tests need some more description. For this one, I assume that you > are verifying that exprSrc is either (a) a list of Statements; (b) a list of > Statements with the final semicolon missing; or (c) an Expression. Done. Check out the new comment. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:183: verifyStrictProgram('( ' + exprSrc + '\n);'); On 2011/03/16 21:17:11, ihab.awad wrote: > Now you verify that it is an Expression, since it can parse correctly surrounded > by the parentheses. But what's the newline for? Done. Check out the new comment. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:193: * properties of {@code env} becomes an accessor of {@code On 2011/03/16 21:17:11, ihab.awad wrote: > s/an// Done. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/white... File src/com/google/caja/ses/whitelist.js (right): http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/white... src/com/google/caja/ses/whitelist.js:42: * </ul> On 2011/03/16 21:17:11, ihab.awad wrote: > Per our discussion: the 3 common cases, [ true, "*", {} ], should be described > by an example, and the "skip" case should be explained as a temporary escape > hatch that we hope will go away by the time this stuff is productionized. Done. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/white... src/com/google/caja/ses/whitelist.js:42: * </ul> On 2011/03/16 21:17:11, ihab.awad wrote: > It seems impossible for an entry to have the meanings of both "*" (inherit the > whitelisting of this property) and a record (whitelist the value of this > property according to the following criteria). It seems therefore that there > exist whitelists which cannot be expressed by this schema. Have we just not > encountered them? Yet? Or is this by design? Done.
Sign in to reply to this message.
Finally ready for the next round of review. Have at it. Thx. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... File src/com/google/caja/ses/startSES.js (right): http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:195: * use their {@code this} value, then the original and any copied On 2011/03/16 21:17:11, ihab.awad wrote: > How are you sure the getters and setters you plan to encounter don't use their > "this"? A note justifying the assumption might be helpful. Done. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:227: * scope. Each decision violates some assumptions. On 2011/03/16 21:17:11, ihab.awad wrote: > Could you note what the violated assumptions are for each case? Done. On reflection, I don't see any assumptions violated by the way it is now, so I deleted the TODO note. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:254: enumerable: true On 2011/03/16 21:17:11, ihab.awad wrote: > Why is enumerability important? This is only used to satisfy free variable > references on demand, right? Done. Correct. It doesn't matter so I removed it. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:444: global.eval = fakeEval; On 2011/03/16 21:17:11, ihab.awad wrote: > Should explain in a comment (as you did to me) why corresponding properties of > the *real* global object need to be modified as well, given that SES code is > always run in a virtualized "global" scope. Done. See the doc-comment on eval. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:527: }); On 2011/03/16 21:17:11, ihab.awad wrote: > Per our discussion, please add a comment describing that this is to make the > property immutable such that future read()s get the same value -- and that the > code below relies on this fact. Done. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:581: if (permit[name] !== 'skip') { On 2011/03/16 21:17:11, ihab.awad wrote: > I'm confused about this. If permit[name] === '*' or (true), then the register() > call 2 lines below will be a no-op anyway due to the value !== Object(value) > check at the top. This check seems like it can be dropped. In the "skip" case, we must also avoid the read with its implied property freeze, since some skipped properties will refuse. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:590: * We initialize the whiteTable only so that we can process "*" and On 2011/03/16 21:17:11, ihab.awad wrote: > This is not a description of function isPermitted() ... I'm not sure this > comment belongs here. Done. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:597: if (permit[name]) { return permit[name]; } On 2011/03/16 21:17:11, ihab.awad wrote: > If permit[name] is an Object, you return that anyway -- but simply as a synonym > for true (i.e., expecting it to be evaluated for truthiness only). This caused > me confusion regarding the expected range of function isPermitted(). Perhaps > refactor to clarify? Or at least document the range of this function in the > comments? > > Also, isPermitted() implies it returns *only* a boolean value, whereas this > function returns something more funky than that.... > > Anyway: as I understand it, the range of isPermitted() is: > > { /* ... */ } /* i.e. truthy */ > true > '*' > 'skip' > false > > For clarity, I think the {} and '*' cases should be explicitly returned as true. Done. Changed the comment and the name to clarify. http://codereview.appspot.com/4249052/diff/2002/src/com/google/caja/ses/start... src/com/google/caja/ses/startSES.js:624: function clean(value, n) { On 2011/03/16 21:17:11, ihab.awad wrote: > The identifiers 'n' and 'path' are confusing to me ... perhaps 'path' and > 'childPath' or something? Done. Changed 'n' to 'prefix'
Sign in to reply to this message.
Fixed two manifestations of the same security vulnerability, as now documented on PATCH_MUTABLE_FROZEN_DATE_PROTO and PATCH_MUTABLE_FROZEN_WEAKMAP_PROTO in WeakMap.js.
Sign in to reply to this message.
Improved diagnostic when the new checks that prevent modifications of internal prototype checks fail. Relayered order of initSES to be clearer. Shouldn't actually make any operational difference.
Sign in to reply to this message.
http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... File src/com/google/caja/ses/startSES.js (right): http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:294: * <p>TODO(erights): Switch to Crock's alternate suggestion of I cannot reproduce execution. The problem I was thinking of was around E4X. On FF which, Function('<{"a" + "b"}>') will do expression optimization but so far I am unable to produce something that causes actual execution.
Sign in to reply to this message.
Also found a better kludge to work around some undiagnosed bugs. See METHODS_TO_WRAP_ON_V8 in es5shim.js, which replaces the EARLY_FREEZES and TOLERATE_FAILED_FREEZE kludges that had been in startSES.js. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... File src/com/google/caja/ses/startSES.js (right): http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:294: * <p>TODO(erights): Switch to Crock's alternate suggestion of On 2011/05/10 23:26:38, jasvir wrote: > I cannot reproduce execution. The problem I was thinking of was around E4X. On > FF which, Function('<{"a" + "b"}>') will do expression optimization but so far I > am unable to produce something that causes actual execution. Done. Switched from Ankur's trick to Crock's trick, which testing reveals to be more than 100x faster on V8.
Sign in to reply to this message.
[+ankur, +douglas] New code implementing Crock's trick at http://codereview.appspot.com/4249052/diff/56001/src/com/google/caja/ses/star... Thanks to both of you! On Fri, May 13, 2011 at 11:24 AM, <erights@gmail.com> wrote: [...] > > > http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... > File src/com/google/caja/ses/startSES.js (right): > > > http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... > src/com/google/caja/ses/startSES.js:294: * <p>TODO(erights): Switch to > Crock's alternate suggestion of > On 2011/05/10 23:26:38, jasvir wrote: > >> I cannot reproduce execution. The problem I was thinking of was >> > around E4X. On > >> FF which, Function('<{"a" + "b"}>') will do expression optimization >> > but so far I > >> am unable to produce something that causes actual execution. >> > > Done. Switched from Ankur's trick to Crock's trick, which testing > reveals to be more than 100x faster on V8. > > > http://codereview.appspot.com/4249052/ > -- Cheers, --MarkM
Sign in to reply to this message.
LGTM++ http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak... File src/com/google/caja/ses/WeakMap.js (right): http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak... src/com/google/caja/ses/WeakMap.js:44: * FF6.0a1 is the presence on non enumerable {@code get___, has___, ... "of non" ... http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak... src/com/google/caja/ses/WeakMap.js:107: * MUST either have different identities, or at least one of their ... "or both of their" ... -- right? http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak... src/com/google/caja/ses/WeakMap.js:162: * randomness provided by {@code Math.random}. This not ... "This is not" ... http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak... src/com/google/caja/ses/WeakMap.js:218: identifyFirst(Object, 'freeze'); This should not induce a change in the CL, but it just occurred to me that a common application pattern may be to create a whole bunch of small "value" objects and freeze them -- like (x, y) coordinates or whatever. We are making that nontrivially more expensive here. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak... src/com/google/caja/ses/WeakMap.js:323: var last = ids.length -1; Space after minus operator. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak... src/com/google/caja/ses/WeakMap.js:332: delete vals[last]; delete does not shorten the array length; did you mean to use ids.splice(last)? http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/atLe... File src/com/google/caja/ses/atLeastFreeVarNames.js (right): http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/atLe... src/com/google/caja/ses/atLeastFreeVarNames.js:86: while ((a = regexp.exec(programSrc))) { Extra parens around expr. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... File src/com/google/caja/ses/es5shim.js (right): http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:15: var RegExp; Why declare this here? (Was not present in older revs.) http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:142: var classTester = Object.prototype.toString; I found the renaming confusing. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:144: var getParent = Object.getPrototypeOf; I found the renaming confusing. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:168: switch (arguments.length) { Please indent case-es 1 step. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:237: * Make a function suitable for using as a forEach argument on a Describing in terms of foreach is a bit confusing ... maybe just describe in terms of what it returns? Not a big deal either way. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:246: * objects of this [[Class]], only the prototypes directly inherits ... "only the prototype" ... http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:267: classString + '.prototype'); Indentation. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:280: // Note: coordinate this list with maintanence of whitelist.js ... "maintenance" ... http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... File src/com/google/caja/ses/startSES.js (right): http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:90: * <li>Since {@code Q.post} can be used for asynchronously involing ... "invoking" ... http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:118: * evaluation operations) takes care to uphold object-capability My code says: alert('I pledge allegiance to the Grannovetter diagram,\nAnd to the Introduction for which it stands.') Does that mean it upholds object-capability rules? If not, what must my code do? http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:138: * <tt>global</tt>. These properties are also duplicated onto a s/ a$// http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:152: * potentially offensive code but not supporting defensive Perhaps clarify that this implies one trust domain per frame? Not a big deal either way. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:273: /** Repair phase -- monkey patch safe replacements */ This comment misled me. Actually, what's starting in the below anonymous function is the construction of the guts of "global.cajaVM".... http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:387: (function(name) { 2nd level of lambda unnecessary, right? http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:467: var requirePattern = (/^(?:\w*\s*(?:\w|\$|\.)*\s*=\s*)?\s*require\s*\(\s*['"]((?:\w|\$|\.|\/)+)['"]\s*\)$/m); Is there an extra "\s*"? Harmless anyway but wouldn't this also work? ...\s*=)?\s*require... http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:647: })(); ... and here is where we're done building "global.cajaVM". http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:700: writable: true, Why writable? http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:710: writable: true, Why writable? http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:737: fail('primordial reachable through multiple paths'); Seems benign; why is that a problem? Just because it would make your attempt to limit access to it via the whitelist more fragile? http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:750: * Should the property names {@code name} be whitelisted on the ... "named" ... http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:766: if (permit && hop.call(permit, name)) { Why not just use permit[name] as before? Why the "hop" call? http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:820: for (var i = 0, len = EARLY_FREEZES.length; i < len; i++) { Variables "i" and "len" leak to a large scope; maybe put in closure? http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:855: } Else throw, as an extra safety feature? http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/whit... File src/com/google/caja/ses/whitelist.js (right): http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/whit... src/com/google/caja/ses/whitelist.js:30: * "Object"} may have and how each of such property, if present, ... "each such property" ... http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/whit... src/com/google/caja/ses/whitelist.js:313: // Note: coordinate this list with maintanence of es5shim.js ... "maintenance" ... http://codereview.appspot.com/4249052/diff/56001/src/com/google/caja/ses/es5s... File src/com/google/caja/ses/es5shim.js (right): http://codereview.appspot.com/4249052/diff/56001/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:165: * not frozen. Why would Confined-ES5 change things?
Sign in to reply to this message.
http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak... File src/com/google/caja/ses/WeakMap.js (right): http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak... src/com/google/caja/ses/WeakMap.js:44: * FF6.0a1 is the presence on non enumerable {@code get___, has___, On 2011/05/13 21:49:08, ihab.awad wrote: > ... "of non" ... Done. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak... src/com/google/caja/ses/WeakMap.js:107: * MUST either have different identities, or at least one of their On 2011/05/13 21:49:08, ihab.awad wrote: > ... "or both of their" ... -- right? No. If only one of their identities is NO_IDENT, then they are still not egal. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak... src/com/google/caja/ses/WeakMap.js:162: * randomness provided by {@code Math.random}. This not On 2011/05/13 21:49:08, ihab.awad wrote: > ... "This is not" ... Done. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak... src/com/google/caja/ses/WeakMap.js:218: identifyFirst(Object, 'freeze'); On 2011/05/13 21:49:08, ihab.awad wrote: > This should not induce a change in the CL, but it just occurred to me that a > common application pattern may be to create a whole bunch of small "value" > objects and freeze them -- like (x, y) coordinates or whatever. We are making > that nontrivially more expensive here. Yes. Without built-in WeakMaps, I don't see that we have any other choice. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak... src/com/google/caja/ses/WeakMap.js:323: var last = ids.length -1; On 2011/05/13 21:49:08, ihab.awad wrote: > Space after minus operator. Done. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/Weak... src/com/google/caja/ses/WeakMap.js:332: delete vals[last]; On 2011/05/13 21:49:08, ihab.awad wrote: > delete does not shorten the array length; did you mean to use ids.splice(last)? Done. Wow. Somehow it had escaped my notice that delete on the end does not update length. I thought it did. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/atLe... File src/com/google/caja/ses/atLeastFreeVarNames.js (right): http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/atLe... src/com/google/caja/ses/atLeastFreeVarNames.js:86: while ((a = regexp.exec(programSrc))) { On 2011/05/13 21:49:08, ihab.awad wrote: > Extra parens around expr. That's on purpose, as a widespread convention saying "yes, even though it's a conditional expression, I really did mean assignment rather than an equality test." I think this convention started in C but I've seen it at least in Java as well. I think I've also seen linting tools that recognize it -- though not yet for JavaScript AFAIK. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... File src/com/google/caja/ses/es5shim.js (right): http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:15: var RegExp; On 2011/05/13 21:49:08, ihab.awad wrote: > Why declare this here? (Was not present in older revs.) Just to be clearer that this is a global that might be assigned to by this module. The older code was doing "global.RegExp" and so needed to bind "global" in this scope to the global object, which it no longer does. Of course, neither is actually necessary, but a simple local assignment to an identifier doesn't stand out sufficiently as a potentially global assignment. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:142: var classTester = Object.prototype.toString; On 2011/05/13 21:49:08, ihab.awad wrote: > I found the renaming confusing. Done. Renamed to objToString. Clearer about what it is, but slightly less clear about what it is for, http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:144: var getParent = Object.getPrototypeOf; On 2011/05/13 21:49:08, ihab.awad wrote: > I found the renaming confusing. Done. Renamed to getPrototypeOf http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:168: switch (arguments.length) { On 2011/05/13 21:49:08, ihab.awad wrote: > Please indent case-es 1 step. Done. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:237: * Make a function suitable for using as a forEach argument on a On 2011/05/13 21:49:08, ihab.awad wrote: > Describing in terms of foreach is a bit confusing ... maybe just describe in > terms of what it returns? Not a big deal either way. Done. "Make a function..." -> "Return a function..." http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:246: * objects of this [[Class]], only the prototypes directly inherits On 2011/05/13 21:49:08, ihab.awad wrote: > ... "only the prototype" ... I meant the plural to emphasize the cross-frame issue. However, "inherits" should therefore be "inherit" which is now fixed. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:267: classString + '.prototype'); On 2011/05/13 21:49:08, ihab.awad wrote: > Indentation. Done. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:280: // Note: coordinate this list with maintanence of whitelist.js On 2011/05/13 21:49:08, ihab.awad wrote: > ... "maintenance" ... Done. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... File src/com/google/caja/ses/startSES.js (right): http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:90: * <li>Since {@code Q.post} can be used for asynchronously involing On 2011/05/13 21:49:08, ihab.awad wrote: > ... "invoking" ... Done. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:118: * evaluation operations) takes care to uphold object-capability On 2011/05/13 21:49:08, ihab.awad wrote: > My code says: > > alert('I pledge allegiance to the Grannovetter diagram,\nAnd to the Introduction > for which it stands.') > > Does that mean it upholds object-capability rules? If not, what must my code do? I'm not sure what to say, so I added a TODO reminder to explain this. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:138: * <tt>global</tt>. These properties are also duplicated onto a On 2011/05/13 21:49:08, ihab.awad wrote: > s/ a$// Done. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:152: * potentially offensive code but not supporting defensive On 2011/05/13 21:49:08, ihab.awad wrote: > Perhaps clarify that this implies one trust domain per frame? Not a big deal > either way. Done. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:273: /** Repair phase -- monkey patch safe replacements */ On 2011/05/13 21:49:08, ihab.awad wrote: > This comment misled me. Actually, what's starting in the below anonymous > function is the construction of the guts of "global.cajaVM".... Done. Confusing comment removed. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:387: (function(name) { On 2011/05/13 21:49:08, ihab.awad wrote: > 2nd level of lambda unnecessary, right? Done. Good catch. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:467: var requirePattern = (/^(?:\w*\s*(?:\w|\$|\.)*\s*=\s*)?\s*require\s*\(\s*['"]((?:\w|\$|\.|\/)+)['"]\s*\)$/m); On 2011/05/13 21:49:08, ihab.awad wrote: > Is there an extra "\s*"? Harmless anyway but wouldn't this also work? > > ...\s*=)?\s*require... I don't know. Since it's harmless, I'm skipping this issue for now but adding a TODO. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:647: })(); On 2011/05/13 21:49:08, ihab.awad wrote: > ... and here is where we're done building "global.cajaVM". Yup. Nothing done here since offending comment removed above. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:700: writable: true, On 2011/05/13 21:49:08, ihab.awad wrote: > Why writable? A baby step towards preparing for confined-ES5. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:710: writable: true, On 2011/05/13 21:49:08, ihab.awad wrote: > Why writable? A baby step towards preparing for confined-ES5. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:737: fail('primordial reachable through multiple paths'); On 2011/05/13 21:49:08, ihab.awad wrote: > Seems benign; why is that a problem? Just because it would make your attempt to > limit access to it via the whitelist more fragile? If this happens, it means that the heap doesn't have the structure I assumed it had when I constructed the whitelist, so fail early and safe rather than proceed under possibly invalid assumptions. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:750: * Should the property names {@code name} be whitelisted on the On 2011/05/13 21:49:08, ihab.awad wrote: > ... "named" ... Done. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:766: if (permit && hop.call(permit, name)) { On 2011/05/13 21:49:08, ihab.awad wrote: > Why not just use permit[name] as before? Why the "hop" call? Because the permits inherit from object.prototype, so, for example, permit['constructor'] will be truthy even for a permit that does not say anything about a 'constructor' property. I actually ran into precisely that bug on that earlier version. This is a great example of JS hazards that even experienced JS programmers will miss, as both of us have now done. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:820: for (var i = 0, len = EARLY_FREEZES.length; i < len; i++) { On 2011/05/13 21:49:08, ihab.awad wrote: > Variables "i" and "len" leak to a large scope; maybe put in closure? Done. No longer a problem since the kludge migration from startSES.js to es5shim.js removed the need for this loop. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/star... src/com/google/caja/ses/startSES.js:855: } On 2011/05/13 21:49:08, ihab.awad wrote: > Else throw, as an extra safety feature? Done. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/whit... File src/com/google/caja/ses/whitelist.js (right): http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/whit... src/com/google/caja/ses/whitelist.js:30: * "Object"} may have and how each of such property, if present, On 2011/05/13 21:49:08, ihab.awad wrote: > ... "each such property" ... Done. http://codereview.appspot.com/4249052/diff/37002/src/com/google/caja/ses/whit... src/com/google/caja/ses/whitelist.js:313: // Note: coordinate this list with maintanence of es5shim.js On 2011/05/13 21:49:08, ihab.awad wrote: > ... "maintenance" ... Done. http://codereview.appspot.com/4249052/diff/56001/src/com/google/caja/ses/es5s... File src/com/google/caja/ses/es5shim.js (right): http://codereview.appspot.com/4249052/diff/56001/src/com/google/caja/ses/es5s... src/com/google/caja/ses/es5shim.js:165: * not frozen. On 2011/05/13 21:49:08, ihab.awad wrote: > Why would Confined-ES5 change things? It might allow the untrusted code to monkey patch its own Function.prototype.apply, in which case "return original.apply(this, arguments);" is no longer a transparent emulation of the internal "original.[[Call]](this, arguments)". Also, the munkey-patched apply obtains access to original, which should be denied to untrusted code.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
