Disallow non-element params to service getters by dreamofabear · Pull Request #16328 · ampproject/amphtml

dreamofabear · 2018-06-25T20:18:00Z

Partial for #16322. Goal is to prevent bugs like #16043 in the future.

Fix a few instances of window.document being passed to service getters.
Change all service getter methods to accept !Element instead of !Node to prevent future bugs.
Create getServiceForDocDeprecated(nodeOrDoc, id) to keep a few remaining usages (startup services) for follow-up fix.

/to @jridgewell @danielrozenberg /cc @jpettitt @aghassemi

jridgewell

Why can't we just have it accept a Document?

jridgewell · 2018-06-25T21:24:00Z

src/service.js

    // If a node is passed, try to resolve via this node.
    const win = toWin(/** @type {!Document} */ (
-      nodeOrDoc.ownerDocument || nodeOrDoc).defaultView);
+      elementOrAmpDoc.ownerDocument || elementOrAmpDoc).defaultView);


This change is incorrect. Element does not have a defaultView.

Oh man, nice catch. Thanks.

jridgewell · 2018-06-25T21:24:20Z

src/element-service.js

+  if (elementOrAmpDoc.nodeType) {
    const win = toWin(/** @type {!Document} */ (
-      nodeOrDoc.ownerDocument || nodeOrDoc).defaultView);
+      elementOrAmpDoc.ownerDocument || elementOrAmpDoc).defaultView);


jridgewell · 2018-06-25T21:25:17Z

src/service/navigation.js

   */
  parseUrl_(url) {
-    return Services.urlForDoc(this.rootNode_).parse(url);
+    return Services.urlForDoc(this.ampdoc).parse(url);


I don't think this change is correct. rootNode is in the document scope, while ampdoc is the root scope.

I considered that, but I don't think local scope is important for URL parsing. Line 168 already grabs the URL service from this.ampdoc.

For FIEs, it's critical. FIE have a <base> element that changes where their relative URLs resolve from.

TIL, good call. I think I was too aggressive here -- reverted APIs for FIE-scope services.

dreamofabear · 2018-06-25T22:44:18Z

Why can't we just have it accept a Document?

It would only work for single docs since shadow AMPs don't have a Document, and I want to prevent misuse. Part of the problem is Document vs. AmpDoc conflation e.g. fooForDoc is really misleading.

dreamofabear · 2018-06-25T22:45:11Z

/cc @dvoytenko FYI

danielrozenberg · 2018-06-26T15:35:32Z

ads/google/a4a/utils.js

 /**
 * `nodeOrDoc` must be passed for correct behavior in shadow AMP (PWA) case.
 * @param {!Window} win
- * @param {!Node|!../../../src/service/ampdoc-impl.AmpDoc} nodeOrDoc


!../../../src/service/ampdoc-impl.AmpDoc makes me 😢

jridgewell · 2018-06-26T17:37:23Z

src/service/navigation.js

   */
  parseUrl_(url) {
-    return Services.urlForDoc(this.rootNode_).parse(url);
+    return Services.urlForDoc(this.ampdoc).parse(url);


For FIEs, it's critical. FIE have a <base> element that changes where their relative URLs resolve from.

dreamofabear · 2018-06-27T20:38:54Z

@jridgewell Friendly ping.

jridgewell · 2018-06-27T20:45:51Z

src/service.js

+ * Unlike most service getters, passing `Node` is necessary for some FIE-scope
+ * services since sometimes we only have the FIE Document for context.
+ *
+ * @param {!Node|!./service/ampdoc-impl.AmpDoc} nodeOrAmpDoc


Could we change this to !Element|!ShadowRoot|!AmpDoc? That we we fix the core bug here (passing in a Document) but still allow ShadowRoots?

The problem isn't shadow roots, it's FIE.

In PWA context, callers typically can pass in the AmpDoc or elements in the shadow DOM. But in FIE, sometimes we only have access to the iframe's contentWindow. This is because of how FIE services are set up:

amphtml/src/service/extensions-impl.js

Lines 661 to 669 in b6d49e4

function adoptStandardServicesForEmbed(childWin) {

// The order of service adoptations is important.

// TODO(dvoytenko): Refactor service registration if this set becomes

// to pass the "embeddable" flag if this set becomes too unwieldy.

adoptServiceForEmbed(childWin, 'url');

adoptServiceForEmbed(childWin, 'action');

adoptServiceForEmbed(childWin, 'standard-actions');

adoptServiceForEmbed(childWin, 'navigation');

}

Maybe we can refactor this but it'll be a large change. Another idea is to have privileged and non-privileged getters, one that takes !Node and another that takes !Element.

jridgewell · 2018-06-27T20:46:37Z

src/service.js

+ * Unlike most service getters, passing `Node` is necessary for some FIE-scope
+ * services since sometimes we only have the FIE Document for context.
+ *
 * @param {!Node|!./service/ampdoc-impl.AmpDoc} nodeOrDoc


Could we change this to !Element|!ShadowRoot|!AmpDoc? That we we fix the core bug here (passing in a Document) but still allow ShadowRoots?

William Chou added 6 commits June 25, 2018 13:20

Add assertion for doc-level service getters.

493f1ea

Audit and fix most service getters.

bb0bb11

Replace optional param with getServiceForDocDeprecated().

d844741

Update comments and fix lint.

76e06ad

Convert remaining service getters.

fa63d2e

Fix lint.

f6216f5

dreamofabear requested review from danielrozenberg and jridgewell June 25, 2018 20:18

googlebot added the cla: yes label Jun 25, 2018

jridgewell requested changes Jun 25, 2018

View reviewed changes

Elements don't have a defaultView.

c1afbb0

danielrozenberg approved these changes Jun 26, 2018

View reviewed changes

jridgewell requested changes Jun 26, 2018

View reviewed changes

William Chou added 2 commits June 26, 2018 15:32

Fix tests by allowing Node in FIE-scope getters.

ee87a69

Fix types.

5f1d6ab

dreamofabear force-pushed the pwa-services branch from f1f8ed8 to 5f1d6ab Compare June 26, 2018 21:31

Merge branch 'master' into pwa-services

0df8f2c

jridgewell reviewed Jun 27, 2018

View reviewed changes

jridgewell approved these changes Jun 29, 2018

View reviewed changes

dreamofabear merged commit 7172638 into ampproject:master Jun 29, 2018

dreamofabear deleted the pwa-services branch June 29, 2018 21:31

dreamofabear mentioned this pull request Nov 15, 2018

Fix element services and AMP.setState in FIE #19344

Merged

3 tasks

dreamofabear mentioned this pull request Nov 28, 2018

Remove getServiceForDocDeprecated() #19490

Merged

ampprojectbot mentioned this pull request May 7, 2020

🚨 Error: Cannot read property '__AMP_TOP' of null #28264

Closed

ampprojectbot mentioned this pull request Aug 5, 2020

🚨 Error: No ampdoc found for [object HTMLIFrameElement] #29693

Closed

ampprojectbot mentioned this pull request Dec 30, 2020

🚨 Error: null is not an object (evaluating 'a.__AMP_TOP') #31767

Closed

This was referenced Dec 30, 2020

🚨 Error: null is not an object (evaluating 'a.__AMP_TOP') #31768

Closed

🚨 Error: null is not an object (evaluating 'a.__AMP_TOP') #31769

Closed

🚨 Error: null is not an object (evaluating 'a.__AMP_TOP') #31770

Closed

ampprojectbot mentioned this pull request Jan 28, 2021

🚨 Error: null is not an object (evaluating 'a.__AMP_TOP') ampproject/error-reporting#50

Open

	function adoptStandardServicesForEmbed(childWin) {
	// The order of service adoptations is important.
	// TODO(dvoytenko): Refactor service registration if this set becomes
	// to pass the "embeddable" flag if this set becomes too unwieldy.
	adoptServiceForEmbed(childWin, 'url');
	adoptServiceForEmbed(childWin, 'action');
	adoptServiceForEmbed(childWin, 'standard-actions');
	adoptServiceForEmbed(childWin, 'navigation');
	}

Conversation

dreamofabear commented Jun 25, 2018

Uh oh!

jridgewell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dreamofabear commented Jun 25, 2018

Uh oh!

dreamofabear commented Jun 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dreamofabear commented Jun 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants