Allow for fetch to not include __amp_source_origin so resources can be cached across pages by keithwrightbos · Pull Request #6637 · ampproject/amphtml

keithwrightbos · 2016-12-13T21:13:38Z

Issue #6494: ensure that __amp_source_origin is not included on fetchJSON CORS requests for AMP validation public key fetches so that they can be cached across publisher pages.

cramforce · 2016-12-13T21:20:26Z

extensions/amp-a4a/0.1/amp-a4a.js

      if (url) {
-        return xhrFor(this.win).fetchJson(url, {mode: 'cors', method: 'GET'})
+        return xhrFor(this.win).fetchJson(
+          url, {mode: 'cors', method: 'GET', disableAmpSourceOrigin: true})


Reformat object literal to multiple lines. Should this be also credentials: 'omit'?

cramforce · 2016-12-13T21:20:57Z

extensions/amp-a4a/0.1/amp-a4a.js

      if (url) {
-        return xhrFor(this.win).fetchJson(url, {mode: 'cors', method: 'GET'})
+        return xhrFor(this.win).fetchJson(
+          url, {mode: 'cors', method: 'GET', disableAmpSourceOrigin: true})


Add docs for why the disableAmpSourceOrigin is set.

cramforce · 2016-12-13T21:21:36Z

src/service/xhr-impl.js

-    input = this.getCorsUrl(this.win, input);
+  fetchAmpCors_(input, init = {}) {
+    // Do not append __amp_source_origin
+    if (!(!init.requireAmpResponseSourceOrigin &&


Wow, please clean up the boolean logic a little bit :)

Sorry about that! Done

cramforce · 2016-12-13T21:22:47Z

src/service/xhr-impl.js

@@ -36,6 +39,7 @@ import {isArray, isObject, isFormData} from '../types';
 *   headers: (!Object|undefined),
 *   method: (string|undefined),
 *   requireAmpResponseSourceOrigin: (boolean|undefined)


Please make requireAmpResponseSourceOrigin explicit on the caller side.

…_wo_origin

keithwrightbos · 2016-12-14T15:14:40Z

@cramforce PTAL

lannka · 2016-12-14T16:13:47Z

extensions/amp-a4a/0.1/test/test-amp-a4a.js

+        mode: 'cors',
+        method: 'GET',
+        requireAmpResponseSourceOrigin: false,
+        disableAmpSourceOrigin: true,


should we call it ampCors: false?
Overall, disabling AMP CORS is the purpose, which requires 2 things:

Not sending the AMP_SOURCE_ORIGIN.

Not check response source origin.

I was trying to be as explicit as possible thus the word "disable". Technically this is still a CORS request, it just doesn't include the source origin parameter so ampCors would seem somewhat confusing to me. Will defer to @cramforce

ampCors is more than CORS. The extra things it does are the 2 I listed. And I don't see use case for:

requireAmpResponseSourceOrigin: true, disableAmpSourceOrigin: true,

On the other side, requireAmpResponseSourceOrigin only exists for backward compatibility. I hope some day we can get rid of it. So any new changes should not really depend on it .

Modified to use assert as Malte suggested

I still think these 2 boolean names are confusing.
We should have the new boolean as ampCors, which defaults to true. In case it's false, it will override requireAmpResponseSourceOrigin to false.

As I mentioned, the requireAmpResponseSourceOrigin boolean exists only for backward compatibility. No new code should use it as false. They should instead go with ampCors: false.

@cramforce

Convinced. Lets change it.

lannka · 2016-12-14T16:14:53Z

test/functional/test-xhr.js

+        });
+
+        it('should not include __amp_source_origin if disableAmpSourceOrigin ' +
+            'true & requireAmpResponseSourceOrigin true', () => {


is it a valid use case?

Again this is about being explicit. Should somewhat inadvertently specify both but in conflict, the requireAmpResponseSourceOrigin should take precedent (which this verifies).

I think that should just be covered by dev().assert

cramforce

Lets roll with this. One comment.

cramforce · 2016-12-14T18:37:02Z

test/functional/test-xhr.js

+        });
+
+        it('should not include __amp_source_origin if disableAmpSourceOrigin ' +
+            'true & requireAmpResponseSourceOrigin true', () => {


I think that should just be covered by dev().assert

keithwrightbos · 2016-12-14T18:41:47Z

@lannka @cramforce please merge. Thanks

…_wo_origin

keithwrightbos · 2016-12-14T21:11:34Z

@lannka PTAL and merge if ok

…_wo_origin

keithwrightbos · 2016-12-15T15:31:45Z

@lannka PTAL. Changed to use ampCors. Please merge if travis is green. Thx

lannka · 2016-12-15T18:31:55Z

src/service/xhr-impl.js

    // (requireAmpResponseSourceOrigin overrides disable).
    dev().assert(
-      !(init.disableAmpSourceOrigin && init.requireAmpResponseSourceOrigin),
+      !(init.ampCors && init.requireAmpResponseSourceOrigin),


in case ampCors == false, let's just override the value of requireAmpResponseSourceOrigin to false.

…_wo_origin

keithwrightbos · 2016-12-15T18:53:25Z

@lannka PTAL, changed so ampCors = false forces requireAmpResponseSourceOrigin to false

…e cached across pages (ampproject#6637) * Initial commit * Fix merge issues * PR responses * PR review * PR responses * PR responses

dandv · 2017-06-09T20:47:44Z

Is disableAmpSourceOrigin documented somewhere?

mkaraula · 2017-08-22T14:53:52Z

@dandv No its not.

AndrewKGuan · 2019-10-10T23:35:04Z

Hi,
can we use this flag to turn off __amp_source_origin on fetches now? If yes, please provide an example how to use it.

keithwrightbos added 2 commits December 13, 2016 16:06

Initial commit

ab944ea

Fix merge issues

ea784a4

keithwrightbos added the WG: monetization label Dec 13, 2016

keithwrightbos assigned cramforce and lannka Dec 13, 2016

cramforce requested changes Dec 13, 2016

View reviewed changes

keithwrightbos changed the title ~~A4a fetch wo origin~~ Allow for fetch to not include __amp_source_origin so resources can be cached across pages Dec 14, 2016

keithwrightbos added 2 commits December 14, 2016 09:42

Merge branch 'master' of github.com:ampproject/amphtml into a4a_fetch…

e5326e1

…_wo_origin

PR responses

00e877e

lannka suggested changes Dec 14, 2016

View reviewed changes

cramforce approved these changes Dec 14, 2016

View reviewed changes

keithwrightbos added 2 commits December 14, 2016 13:46

Merge branch 'master' of github.com:ampproject/amphtml into a4a_fetch…

6dd9571

…_wo_origin

PR review

e550cea

keithwrightbos added 2 commits December 15, 2016 10:08

Merge branch 'master' of github.com:ampproject/amphtml into a4a_fetch…

a86b285

…_wo_origin

PR responses

36987c4

lannka reviewed Dec 15, 2016

View reviewed changes

keithwrightbos added 2 commits December 15, 2016 13:46

Merge branch 'master' of github.com:ampproject/amphtml into a4a_fetch…

f44b346

…_wo_origin

PR responses

19c755a

lannka approved these changes Dec 15, 2016

View reviewed changes

lannka merged commit a36160a into ampproject:master Dec 15, 2016

lannka mentioned this pull request Dec 16, 2016

Deprecate XHR requireAmpResponseSourceOrigin param #6716

Closed

Conversation

keithwrightbos commented Dec 13, 2016

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!

keithwrightbos commented Dec 14, 2016

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cramforce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithwrightbos commented Dec 14, 2016

Uh oh!

keithwrightbos commented Dec 14, 2016

Uh oh!

keithwrightbos commented Dec 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keithwrightbos commented Dec 15, 2016

Uh oh!

dandv commented Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkaraula commented Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewKGuan commented Oct 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dandv commented Jun 9, 2017 •

edited

Loading

mkaraula commented Aug 22, 2017 •

edited

Loading