Explicitly specify requireAmpResponseSourceOrigin=false if missing #6724
Conversation
| 'requireAmpResponseSourceOrigin is deprecated, use ampCors instead'); | ||
| } | ||
|
|
||
| if (init.requireAmpResponseSourceOrigin === undefined) { |
There was a problem hiding this comment.
For backward compat during version skew, you will need to make this change first and then make the extension changes one release later.
There was a problem hiding this comment.
Aren't all extensions rolling out together with runtime?
There was a problem hiding this comment.
But they may be cached at different times on the reader's computer when browsing a pub's site.
There was a problem hiding this comment.
If that's a concern, then any changes on runtime and extension can have problem.
There was a problem hiding this comment.
Yes, you must be conscious about it. But renaming APIs in core definitely causes such issues.
We try to avoid skew in as many places as we can, but it can absolutely happen.
There was a problem hiding this comment.
OK. I've reverted my 2nd commit. Now this PR simply make all the calls explicit about the requireAmpResponseSourceOrigin param. Once this get in prod, we can then have another PR to default it to true.
PTAL.
lannka
force-pushed
the
deprecate-amp-response-source-origin
branch
from
c61e141
to
756db35
Compare
lannka
changed the title
| @@ -84,7 +84,9 @@ export class AmpAdCustom extends AMP.BaseElement { | |||
| // If this promise has no URL yet, create one for it. | |||
| if (!(fullUrl in ampCustomadXhrPromises)) { | |||
| // Here is a promise that will return the data for this URL | |||
| ampCustomadXhrPromises[fullUrl] = xhrFor(this.win).fetchJson(fullUrl); | |||
| ampCustomadXhrPromises[fullUrl] = xhrFor(this.win).fetchJson(fullUrl, { | |||
There was a problem hiding this comment.
This is young enough. We must require this here.
There was a problem hiding this comment.
changing it directly might potentially break @xgretsch . I'll do it separately.
There was a problem hiding this comment.
Yes, please discuss with them.
There was a problem hiding this comment.
I'm on leave and can't check it on my development system. But just looking at things: if I've understood this correctly, this simply enforces that this call is NOT requiring the special AMP-Access-Control-Allow-Source-Origin header. Since there's no damage to us if someone else shows our ads, I don't have a problem with this. If you let me know when this is going into canary, I'll check it then.
There was a problem hiding this comment.
@xgretsch this does not change everything. requireAmpResponseSourceOrigin defaults to false, this PR just made it explicit.
For security reason, we do have plan to change it to true though, which will need a change at your server side.
I'll open a separate issue to track that.
There was a problem hiding this comment.
Let me know when you've put in a separate issue. In the mean time, I've added the following header to the source code of my ad server:
AMP-Access-Control-Allow-Source-Origin: *
What I would really like to know is this: do you have any documents explaining the kind of attack that this feature is designed to protect against? Because in this case, where the response from the server is never interpreted as a script, I can't immediately think of one...
There was a problem hiding this comment.
@lannka Thanks for the reference. Everything in there makes complete sense for a state-changing request, where it's a great idea to discourage requests from the wrong place. For a request which is not state-changing, like a set of ad fetches, I still cannot for the life of me see why one would bother.
There was a problem hiding this comment.
I see your point. But overall we're building a platform for anyone to use, not for a single use case. So being cautious and make a high bar starting from the beginning is not a bad idea.
For #6716
This is the 1st step to deprecate field requireAmpResponseSourceOrigin.
To make sure no version skew between runtime and extensions, we do
/cc @keithwrightbos