Don't use Fast Fetch with AdSense or DoubleClick on non-Google AMP caches #10872
Conversation
| mockWin.location = parseUrl( | ||
| 'https://amp.cloudflare.com/some/path/to/content.html'); | ||
| sandbox.stub( | ||
| urls, 'cdnProxyRegex', |
There was a problem hiding this comment.
this works in practice but shouldn't be done as modules are not really objects and can't be stubbed
There was a problem hiding this comment.
urls isn't a module, it's just an object.
| return supportsNativeCrypto && | ||
| (isProxyOrigin(win.location) || getMode(win).localDev || | ||
| (googleCdnProxyRegex.test(win.location.origin) || getMode(win).localDev || |
There was a problem hiding this comment.
I'm not seeing how this is functionally different from what isProxyOrigin does, i.e., it's basically
return urls.cdnProxyRegex.test(url.origin); where urls.cdnProxyRegex is the same as what you've added above (unless overridden).
There was a problem hiding this comment.
"Unless overridden" is exactly the scenario where the problem manifests. urls.cdnProxyRegex can be overridden in AMP_CONFIG, and Cloudflare does so.
There was a problem hiding this comment.
To see for yourself, search for the following string in https://amp.cloudflare.com/v0.js: "cdnProxyRegex":/^https:\/\/([a-zA-Z0-9_-]+\.)?amp\.cloudflare\.com/,
There was a problem hiding this comment.
Nit: this will match https://cdn.ampproject.org.badguys.com/etc - I suggest adding a trailing slash to googleCdnProxyRegex.
There was a problem hiding this comment.
Good catch. We can't add a trailing slash though, that won't match an origin; we need a trailing $. Also, that regex was copy-pasted from isProxyOrigin so other code has the same bug.
…ches (#10872) * Don't use Fast Fetch with AdSense or DoubleClick on non-Google AMP caches * Fix forbidden dependency and add test * Fix googleCdnProxyRegex
…ches (#10872) * Don't use Fast Fetch with AdSense or DoubleClick on non-Google AMP caches * Fix forbidden dependency and add test * Fix googleCdnProxyRegex
|
@erwinmombay @lannka Was this also patched into Canary (1502234866079)? |
…ches (#10872) * Don't use Fast Fetch with AdSense or DoubleClick on non-Google AMP caches * Fix forbidden dependency and add test * Fix googleCdnProxyRegex
Fixes #10871.