Report when XHR are issued before viewer is visible #9350

jridgewell · 2017-05-15T22:12:31Z

dvoytenko · 2017-05-15T23:38:44Z

src/service/xhr-impl.js

+      this.ampdoc_ = ampdocServiceFor(win).getAmpDoc();
+    } catch (e) {
+      this.ampdoc_ = null;
+      dev().error('XHR', e, 'XHR service failed to find ampdoc');


The probability of failure in PWA case here is about 100% :).

I think we just need to restrict this reporting/warning to single-doc mode. The real issue is only in this mode anyway. See AmpDocService.isSingleDoc()

That's what I was thinking. Though I don't agree that it's only needed in single-doc mode. Is there a way to get the parent amp-doc while in PWA?

The only way to pull the parent ampdoc is by passing around the context node (element). The reason why I think PWA mode is less relevant b/c this is a privacy feature, since iframes can pull documents from different origins. PWA on the other hand is built to serve "own" same-origin documents. If PWA decides to request cross-origin docs, it still needs to support CORS, etc - which essentially nullify our protection here.

Thanks for the explanation, done!

dvoytenko · 2017-05-15T23:39:43Z

src/service/xhr-impl.js

+            input);
+      }
+    } else {
+      dev().error('XHR', 'attempted to fetch %s without knowing if viewer is' +


dvoytenko · 2017-05-15T23:40:45Z

src/service/xhr-impl.js

+      // TODO(@jridgewell, #9048): We can alternatively _always_ wait on
+      // #whenFirstVisible.
+      if (!viewerForDoc(this.ampdoc_).hasBeenVisible()) {
+        dev().error('XHR', 'attempted to fetch %s before viewer was visible',


To start with, we should sample it at 1% and then go higher. Otherwise we take a risk of over-spamming our pantheon which is quota-limited. Once confirmed low volume - we can go to 100%.

Isn't this already sampled by the error reporting server?

Well, we don't want to overwhelm that service either. But basically, error transformer can only sample randomly regardless of error. So if we think this could have too big a rate, we should do it here.

dreamofabear · 2017-05-16T05:00:06Z

src/service/xhr-impl.js

@@ -94,6 +96,11 @@ export class Xhr {
  constructor(win) {
    /** @const {!Window} */
    this.win = win;
+
+    const ampdocService = ampdocServiceFor(win);
+    this.ampdoc_ = ampdocService.isSingleDoc() ?


Nit: ampdocSingle_

+1 and jsdoc

dreamofabear · 2017-05-16T05:03:11Z

src/service/xhr-impl.js

+    if (this.ampdoc_) {
+      // TODO(@jridgewell, #9048): We can alternatively _always_ wait on
+      // #whenFirstVisible.
+      if (Math.random() < .01 && !viewerForDoc(this.ampdoc_).hasBeenVisible()) {


Seems like this will also trigger for legitimate pre-render XHRs, e.g. same-origin.

That's intended for now.

What's the purpose of reporting those? Won't it just muddy the signal?

#9350 (comment)

To follow up on this from above. I definitely agree with @choumx - this signal will get super noisy and even with 1% error rate it would be overwhelming. Sorry for missing it at first. We can already define which requests are "good" or "bad" . The only requests we need to report here are URLs with url.origin != window.location.origin. Same-origin requests are explicitly allowed and have 0 risk of any kind associated with them since we just pulled the document itself from that origin.

Eventually, I believe, we'll use this condition and throw an actual error == deny the request in prerender with different origin.

dreamofabear · 2017-05-16T05:05:26Z

src/service/xhr-impl.js

@@ -106,6 +113,15 @@ export class Xhr {
   * @private
   */
  fetch_(input, init) {
+    if (this.ampdoc_) {
+      // TODO(@jridgewell, #9048): We can alternatively _always_ wait on
+      // #whenFirstVisible.


Could be nice as a conservative mitigation technique. As is, this PR won't prevent the next privacy issue.

No, just reports it. I think once we understand all the valid use cases, we can move to this.

We definitely can't always wait on first-visible, not w/o additional rules. We can try to define these rules right now - it's generally possible. But starting with reporting is likely better. I'll make a comment below.

dreamofabear · 2017-05-17T21:07:40Z

src/service/xhr-impl.js

@@ -154,7 +171,7 @@ export class Xhr {
    }
    // For some same origin requests, add AMP-Same-Origin: true header to allow
    // publishers to validate that this request came from their own origin.
-    const currentOrigin = parseUrl(this.win.location.href).origin;
+    const currentOrigin = this.win.location.origin;


Safe to use? MDN is unclear.

jridgewell · 2017-05-18T19:09:25Z

Ping.

dreamofabear · 2017-05-18T21:25:29Z

src/service/xhr-impl.js

+        return false;
+      },
+    };
+  });


This looks weird. Can we do without?

It'll complain that it can't find the service.

The hidden side effect would cause subsequent ampdoc registrations to silently fail. Perhaps we can either allow injection of the ampdoc instance or just don't call that code in test mode.

The rest LGTM.

I'd prefer to resolve this. I'm ok with not calling this code in the test code as well. Otherwise this will be asking for a trouble.

Ditto: otherwise LGTM.

If we have to, we can even test for the presence of the service and if it out if not.

dreamofabear · 2017-05-19T20:43:53Z

src/service/xhr-impl.js

+        return false;
+      },
+    };
+  });


The hidden side effect would cause subsequent ampdoc registrations to silently fail. Perhaps we can either allow injection of the ampdoc instance or just don't call that code in test mode.

The rest LGTM.

dvoytenko

LGTM, but one quick confirmation: this won't have any issues with our workers, will it?

jridgewell · 2017-05-23T19:50:10Z

Workers?

dvoytenko · 2017-05-23T22:32:09Z

Web workers or service workers? Do we reuse xhr-impl for any of those?

jridgewell · 2017-05-23T22:58:18Z

Oh no, they don't use any of this.

dreamofabear · 2017-05-23T23:02:01Z

Well, the web worker is fetched via fetchText() but that happens after viewer is visible.

jridgewell requested review from dvoytenko and dreamofabear May 15, 2017 22:12

googlebot added the cla: yes label May 15, 2017

dvoytenko suggested changes May 15, 2017

View reviewed changes

dreamofabear reviewed May 16, 2017

View reviewed changes

jridgewell force-pushed the xhr-viewer-visible branch 2 times, most recently from d8fddf2 to 9b89509 Compare May 17, 2017 17:40

dreamofabear reviewed May 17, 2017

View reviewed changes

jridgewell force-pushed the xhr-viewer-visible branch from 0164f2a to bf2d429 Compare May 17, 2017 23:18

dreamofabear reviewed May 18, 2017

View reviewed changes

dreamofabear approved these changes May 19, 2017

View reviewed changes

jridgewell mentioned this pull request May 23, 2017

Weekly Status 2017-05-22 #9464

Closed

dvoytenko approved these changes May 23, 2017

View reviewed changes

jridgewell force-pushed the xhr-viewer-visible branch from 01e978b to d227ba0 Compare May 23, 2017 19:51

jridgewell added 8 commits May 23, 2017 15:51

Report when XHR are issued before viewer is visible

a519e8a

Lint

aa03a0c

Review comments

bb328c4

Do not log same-origin requests

ca8a72f

Fix tests

3f60c6a

Lint

cc04a02

Fix tests

e4235d6

Undo location.origin changes

1bcd42e

jridgewell force-pushed the xhr-viewer-visible branch from d227ba0 to 370f95c Compare May 23, 2017 22:57

Do not log in test mode

370f95c

jridgewell merged commit a70dcf8 into ampproject:master May 25, 2017

jridgewell deleted the xhr-viewer-visible branch May 25, 2017 21:03

ampprojectbot added the PR use: In Beta / Experimental label Jun 7, 2017

jridgewell added this to Privacy Leaks Caught in Make Malte, Dima, Justin Happy Apr 17, 2018

This was referenced Apr 17, 2018

Make XHR service only do requests after first visible by default #14685

Closed

XHR fetch when view first becomes visible #14837

Merged

Report when XHR are issued before viewer is visible #9350

Report when XHR are issued before viewer is visible #9350

Conversation

jridgewell commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko left a comment

Choose a reason for hiding this comment

jridgewell commented May 23, 2017

dvoytenko commented May 23, 2017

jridgewell commented May 23, 2017

dreamofabear commented May 23, 2017