From a45ede0d204ae59db987fab4e12374788435bb04 Mon Sep 17 00:00:00 2001 From: Justin Ridgewell Date: Thu, 13 May 2021 18:14:15 -0400 Subject: [PATCH] Forbid private properties inside BaseElement This fixes a cross binary issue with private properties being mangled to, ie., `lc` inside `BaseElement` base class. The issue is that a subclass extending it in another binary (eg, `amp-a4a`) can reuse that mangled name for something entirely different, because Closure isn't aware of what the base class is at that point. --- .eslintrc.js | 6 ++ build-system/eslint-rules/no-private-props.js | 66 +++++++++++++++++++ .../eslint-rules/private-prop-names.js | 6 +- src/base-element.js | 57 ++++++++-------- 4 files changed, 107 insertions(+), 28 deletions(-) create mode 100644 build-system/eslint-rules/no-private-props.js diff --git a/.eslintrc.js b/.eslintrc.js index 2ccf88faa677..f9025edff4ef 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -351,5 +351,11 @@ module.exports = { 'google-camelcase/google-camelcase': 0, }, }, + { + 'files': ['src/base-element.js'], + 'rules': { + 'local/no-private-props': 2, + }, + }, ], }; diff --git a/build-system/eslint-rules/no-private-props.js b/build-system/eslint-rules/no-private-props.js new file mode 100644 index 000000000000..301a8e9f97a5 --- /dev/null +++ b/build-system/eslint-rules/no-private-props.js @@ -0,0 +1,66 @@ +/** + * Copyright 2016 The AMP HTML Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS-IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +'use strict'; + +/** + * Ensures private properties are not used in the file. If they are, they must + * be quoted. + * + * @return {!Object} + */ +module.exports = { + meta: { + fixable: 'code', + }, + + create(context) { + return { + MemberExpression(node) { + if (node.computed || !node.property.name.endsWith('_')) { + return; + } + + context.report({ + node, + message: + 'Unqouted private properties are not allowed in BaseElement. Please use quotes', + fix(fixer) { + const {object} = node; + return fixer.replaceTextRange( + [object.end, node.end], + `['${node.property.name}']` + ); + }, + }); + }, + + MethodDefinition(node) { + if (node.computed || !node.key.name.endsWith('_')) { + return; + } + + context.report({ + node, + message: + 'Unqouted private methods are not allowed in BaseElement. Please use quotes', + fix(fixer) { + return fixer.replaceText(node.key, `['${node.key.name}']`); + }, + }); + }, + }; + }, +}; diff --git a/build-system/eslint-rules/private-prop-names.js b/build-system/eslint-rules/private-prop-names.js index 6a3838c51064..7fbf9da26614 100644 --- a/build-system/eslint-rules/private-prop-names.js +++ b/build-system/eslint-rules/private-prop-names.js @@ -55,7 +55,7 @@ module.exports = function (context) { MethodDefinition: function (node) { if ( hasPrivateAnnotation(context.getCommentsBefore(node)) && - !hasTrailingUnderscore(node.key.name) + !hasTrailingUnderscore(node.key.name || node.key.value) ) { context.report({ node, @@ -68,7 +68,9 @@ module.exports = function (context) { node.parent.type == 'ExpressionStatement' && hasPrivateAnnotation(context.getCommentsBefore(node.parent)) && isThisMemberExpression(node.left) && - !hasTrailingUnderscore(node.left.property.name) + !hasTrailingUnderscore( + node.left.property.name || node.left.property.value + ) ) { context.report({ node, diff --git a/src/base-element.js b/src/base-element.js index a5183a995777..0c868c8a7c04 100644 --- a/src/base-element.js +++ b/src/base-element.js @@ -229,6 +229,10 @@ export class BaseElement { constructor(element) { /** @public @const {!Element} */ this.element = element; + + /** @public @const {!Window} */ + this.win = toWin(element.ownerDocument.defaultView); + /* \ \ / \ / / / \ | _ \ | \ | | | | | \ | | / ____| \ \/ \/ / / ^ \ | |_) | | \| | | | | \| | | | __ @@ -236,16 +240,12 @@ export class BaseElement { \ /\ / / _____ \ | |\ \----.| |\ | | | | |\ | | |__| | \__/ \__/ /__/ \__\ | _| `._____||__| \__| |__| |__| \__| \______| - Any private property for BaseElement should be declared in - build-system/externs/amp.multipass.extern.js. This is so closure compiler - doesn't reuse the same symbol it would use in the core compilation unit for - the private property in the extensions compilation unit's private - properties. + Any private property for BaseElement MUST be wrapped with quotes. We cannot + allow Closure Compiler to mangle privates in this class, becasue it can + reuse the same mangled name for a different property in, ie., amp-youtube's + BaseElement subclass (which lives in a different binary). */ - /** @public @const {!Window} */ - this.win = toWin(element.ownerDocument.defaultView); - /** * Maps action name to struct containing the action handler and minimum * trust required to invoke the handler. @@ -253,10 +253,10 @@ export class BaseElement { * handler: function(!./service/action-impl.ActionInvocation), * minTrust: ActionTrust, * }>} */ - this.actionMap_ = null; + this['actionMap_'] = null; /** @private {?string} */ - this.defaultActionAlias_ = null; + this['defaultActionAlias_'] = null; } /** @@ -272,7 +272,7 @@ export class BaseElement { * @return {?string} */ getDefaultActionAlias() { - return this.defaultActionAlias_; + return this['defaultActionAlias_']; } /** @@ -678,13 +678,6 @@ export class BaseElement { return loadPromise(element); } - /** @private */ - initActionMap_() { - if (!this.actionMap_) { - this.actionMap_ = this.win.Object.create(null); - } - } - /** * Registers the action handler for the method with the specified name. * @@ -697,8 +690,8 @@ export class BaseElement { * @public */ registerAction(alias, handler, minTrust = ActionTrust.DEFAULT) { - this.initActionMap_(); - this.actionMap_[alias] = {handler, minTrust}; + initActionMap(this); + this['actionMap_'][alias] = {handler, minTrust}; } /** @@ -714,12 +707,12 @@ export class BaseElement { minTrust = ActionTrust.DEFAULT ) { devAssert( - !this.defaultActionAlias_, + !this['defaultActionAlias_'], 'Default action "%s" already registered.', - this.defaultActionAlias_ + this['defaultActionAlias_'] ); this.registerAction(alias, handler, minTrust); - this.defaultActionAlias_ = alias; + this['defaultActionAlias_'] = alias; } /** @@ -737,10 +730,10 @@ export class BaseElement { let {method} = invocation; // If the default action has an alias, the handler will be stored under it. if (method === DEFAULT_ACTION) { - method = this.defaultActionAlias_ || method; + method = this['defaultActionAlias_'] || method; } - this.initActionMap_(); - const holder = this.actionMap_[method]; + initActionMap(this); + const holder = this['actionMap_'][method]; const {tagName} = this.element; userAssert(holder, `Method not found: ${method} in ${tagName}`); const {handler, minTrust} = holder; @@ -1121,3 +1114,15 @@ export class BaseElement { return this; } } + +/** + * This would usually be a private method on BaseElement class, but we cannot + * use privates here. So, it's manually devirtualized into a regular function. + * + * @param {typeof BaseElement} baseElement + */ +function initActionMap(baseElement) { + if (!baseElement['actionMap_']) { + baseElement['actionMap_'] = baseElement.win.Object.create(null); + } +}