[APICHANGE] Fix inconsistent defaults of SES mitigation resolveOptions. (issue 257910043 by kpreid@google.com)

[re...@codereview-hr.appspotmail.com]

Reviewers: MarkM,

Description:
Previously, if the mitigation options were absent (null or undefined
value), the parseFunctionBody option would be taken as true, but if
an options record were provided without the parseFunctionBody property,
it would be taken as false.

Resolved this inconsistency in the direction of parseFunctionBody
defaulting to false.

Please review this at https://codereview.appspot.com/257910043/

Affected files (+18, -24 lines):
M src/com/google/caja/ses/startSES.js


Index: src/com/google/caja/ses/startSES.js
diff --git a/src/com/google/caja/ses/startSES.js
b/src/com/google/caja/ses/startSES.js
index
4bbcca8332ca64ab17ad3f9cf42aef3033d99881..1c35690d74b4768706875879626b555afd965404
100644
--- a/src/com/google/caja/ses/startSES.js
+++ b/src/com/google/caja/ses/startSES.js
@@ -305,33 +305,27 @@ ses.startSES = function(global,
* options and their effects.
*/
function resolveOptions(opt_mitigateOpts) {
+ if (opt_mitigateOpts === undefined || opt_mitigateOpts === null) {
+ opt_mitigateOpts = {};
+ }
+
function resolve(opt, defaultOption) {
- return (opt_mitigateOpts && opt in opt_mitigateOpts) ?
- opt_mitigateOpts[opt] : defaultOption;
+ return opt in opt_mitigateOpts ? opt_mitigateOpts[opt] :
defaultOption;
}
+
var options = {};
- if (opt_mitigateOpts === undefined || opt_mitigateOpts === null) {
- options.maskReferenceError = true;
- options.parseFunctionBody = true;
- options.sourceUrl = void 0;
-
- options.rewriteTopLevelVars = true;
- options.rewriteTopLevelFuncs = true;
- options.rewriteFunctionCalls = true;
- options.rewriteTypeOf = false;
- options.forceParseAndRender = false;
- } else {
- options.maskReferenceError = resolve('maskReferenceError', true);
- options.parseFunctionBody = resolve('parseFunctionBody', false);
- options.sourceUrl = resolve('sourceUrl', void 0);
-
- options.rewriteTopLevelVars = resolve('rewriteTopLevelVars', true);
- options.rewriteTopLevelFuncs = resolve('rewriteTopLevelFuncs', true);
- options.rewriteFunctionCalls = resolve('rewriteFunctionCalls', true);
- options.rewriteTypeOf = resolve('rewriteTypeOf',
- !options.maskReferenceError);
- options.forceParseAndRender = resolve('forceParseAndRender', false);
- }
+
+ options.maskReferenceError = resolve('maskReferenceError', true);
+ options.parseFunctionBody = resolve('parseFunctionBody', false);
+ options.sourceUrl = resolve('sourceUrl', void 0);
+
+ options.rewriteTopLevelVars = resolve('rewriteTopLevelVars', true);
+ options.rewriteTopLevelFuncs = resolve('rewriteTopLevelFuncs', true);
+ options.rewriteFunctionCalls = resolve('rewriteFunctionCalls', true);
+ options.rewriteTypeOf = resolve('rewriteTypeOf',
+ !options.maskReferenceError);
+ options.forceParseAndRender = resolve('forceParseAndRender', false);
+
return options;
}
ses.resolveOptions = resolveOptions;

[eri...@gmail.com]

LGTM


https://codereview.appspot.com/257910043/diff/1/src/com/google/caja/ses/startSES.js
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/257910043/diff/1/src/com/google/caja/ses/startSES.js#newcode308
src/com/google/caja/ses/startSES.js:308: if (opt_mitigateOpts ===
undefined || opt_mitigateOpts === null) {
We have (or at least I have) generally said "void 0" rather than
"undefined". Both "void" and "null" are keywords, so we never need to
worry about them being shadowed. (And "void 0" is actually shorter ;).)

https://codereview.appspot.com/257910043/

[kpr...@google.com]

https://codereview.appspot.com/257910043/diff/1/src/com/google/caja/ses/startSES.js
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/257910043/diff/1/src/com/google/caja/ses/startSES.js#newcode308
src/com/google/caja/ses/startSES.js:308: if (opt_mitigateOpts ===
undefined || opt_mitigateOpts === null) {

On 2015/07/23 19:17:37, MarkM wrote:
> We have (or at least I have) generally said "void 0" rather than
"undefined".
> Both "void" and "null" are keywords, so we never need to worry about
them being
> shadowed. (And "void 0" is actually shorter ;).)

I object in principle to this because we are not actually being fully
defensive against shadowing and this practice incorrectly suggests we
are attempting it.

However, changed for consistency.

https://codereview.appspot.com/257910043/

[eri...@gmail.com]

https://codereview.appspot.com/257910043/diff/20001/src/com/google/caja/lang/html/html5-attributes-whitelist.json
File src/com/google/caja/lang/html/html5-attributes-whitelist.json
(left):

https://codereview.appspot.com/257910043/diff/20001/src/com/google/caja/lang/html/html5-attributes-whitelist.json#oldcode67
src/com/google/caja/lang/html/html5-attributes-whitelist.json:67:
"METER::OPTIMUM",
Why does this line disappear in patch set 2?

https://codereview.appspot.com/257910043/diff/20001/tests/com/google/caja/plugin/test-scan-guest.js
File tests/com/google/caja/plugin/test-scan-guest.js (right):

https://codereview.appspot.com/257910043/diff/20001/tests/com/google/caja/plugin/test-scan-guest.js#newcode1351
tests/com/google/caja/plugin/test-scan-guest.js:1351: var ctor =
window[name];
Did you intend to revert this line?

https://codereview.appspot.com/257910043/

[kpr...@google.com]

Fixed apparent reverts.

(this is why we need a new workflow script...)

https://codereview.appspot.com/257910043/

[eri...@gmail.com]

Back to LGTM

https://codereview.appspot.com/257910043/