🐛Restrict checking cached ampDoc reference to AMP elements.

[sparhami]

This fixes a bug where the compiled (renamed) property for ampDoc_
could conflict with a form field name. Rather than blacklist form,
whitelist amp- elements in case anything else has this behavior (like
window and Element ids).

Fixes #24995

[jpettitt]

Woohoo PR number 25000

[dreamofabear]

Interesting bug, thanks for investigating and fixing. 👍

The way we fixed a similar DOM clobbering issue was to use property names that the validator bans in form[name], [id], etc. I.e.

const cached = node['__amp_doc']; // __amp_* is an invalid value for form[name].

We can try extending the window-property-name.js eslint rule to try to catch these generally, though it may have precision problems.

[sparhami]

Interesting bug, thanks for investigating and fixing.

The way we fixed a similar DOM clobbering issue was to use property names that the validator bans in form[name], [id], etc. I.e.

const cached = node['__amp_doc']; // __amp_* is an invalid value for form[name].

We can try extending the window-property-name.js eslint rule to try to catch these generally, though it may have precision problems.

I considered using a string constant, but the problem here is that we are relying on the fact that we are relying on this matching the ampDoc_ property named here:

amphtml/src/custom-element.js

Lines 171 to 175 in b6bd628

	/**
	* Ampdoc can only be looked up when an element is attached.
	* @private {?./service/ampdoc-impl.AmpDoc}
	*/
	this.ampdoc_ = null;

Since this is set on an HTMLElement (since it is a custom element), and we are accessing it from a Node (a super class of HTMLElement), Closure Compiler will ensure consistent property renaming for the set of this property and the read elsewhere.

Perhaps calling it a cached value is not quite correct, since it isn't cached after a lookup. Rather, it is saved when the element is upgraded as we already know which AmpDoc it belongs to.

[dreamofabear]

Another alternative is to avoid accessing private ivars.

const cached = typeof node.getAmpDoc === 'function' ? node.getAmpDoc() : null;

Would need a change to dodge the devAssert.

[sparhami]

Another alternative is to avoid accessing private ivars.

const cached = typeof node.getAmpDoc === 'function' ? node.getAmpDoc() : null;

Would need a change to dodge the devAssert.

I'm not sure how much of a problem this is, since getAmpDoc has a devAssert requiring the node be connected, if specified. This means that it should also have an ampDoc_. The one case where this doesn't hold is the getAmpDocIfAvailable entry point (which only has one usage), which explicitly does not require the node to be connected,

[sparhami]

Changed to use getAmpDoc, but also guarded with everAttached, so we should never hit the devAssert where ampDoc_ is null.