Force cloudflare amp-ad to use absolute src. #7556
Conversation
1c0d825 to
cce9325
Compare
| } | ||
|
|
||
| const src = el.getAttribute('src'); | ||
| if (src.indexOf(network.base) != 0) { |
| } | ||
|
|
||
| const src = el.getAttribute('src'); | ||
| if (src.indexOf(network.base) != 0) { |
There was a problem hiding this comment.
Keeping with the title, we should also assertAbsoluteHttpOrHttpsUrl .
| let url = el.getAttribute('src'); | ||
|
|
||
| // optionally convert to a4a endpoint | ||
| if (a4a && url.indexOf('/_a4a/') != base.length) { |
There was a problem hiding this comment.
url.substr(base.length, 6) != '/_a4a'
cce9325 to
c67030b
Compare
| } | ||
|
|
||
| const src = el.getAttribute('src'); | ||
| if (!src.startsWith(network.base)) { |
There was a problem hiding this comment.
String#startsWith is not available in all environments. You'll need to import the helper.
There was a problem hiding this comment.
Switched to the string.js version. Incidentally, the original implementation was pretty much the same, but I guess the string.js version is a bit clearer in intent.
| } | ||
|
|
||
| const src = el.getAttribute('src'); | ||
| if (!src.startsWith(network.base)) { |
There was a problem hiding this comment.
please use startsWith from string.js. i guess the native one is not widely supported
c67030b to
cdaf1c7
Compare
| && el.hasAttribute('src') | ||
| && el.hasAttribute('data-cf-network') | ||
| && NETWORKS[el.getAttribute('data-cf-network')] != null; | ||
| if (!(this.isAmpAdElement() && el.hasAttribute('src') |
There was a problem hiding this comment.
can you add brief js doc what are we checking ?
|
I am not sure longterm if this is best idea because then we are having publisher put specific url on page vs just attributes. I guess can always add other option in the future. |
|
|
||
| it('should handle both data parameters and templated parameters', () => { | ||
| el.setAttribute('src', '/ad?width=SLOT_WIDTH&height=SLOT_HEIGHT'); | ||
| el.setAttribute('src', 'https://firebolt.cloudflaredemo.com' + |
There was a problem hiding this comment.
Are there some tests that it correctly fails if the src does not start with the Cloudflare base URL? (Or, at least, is not a fully qualified URL?)
Also, there should be tests for the 'optionally convert to a4a endpoint' logic.
There was a problem hiding this comment.
Added additional test.
|
@oliy can we switch adzerk to be engine.betazerk.com ? |
cdaf1c7 to
9d7ac83
Compare
|
I also addressed @dknecht concerns with supporting default src in vendors and changing adzerk to betazerk. |
|
@lannka Can we merge this? Would like to get them in before Thursday canary. |
|
@jridgewell just a heads up. Can we please merge this for the canary release tomorrow? Thanks! |
|
@jridgewell Would it be possible for this to get patched into current canary and then pushed with next production release. We have publishers ready to use and collect a4a metrics that we were hoping to share for ampconf. |
The cloudflare extension wasn't properly ensuring that the src attribute was an absolute URL. This changes the implementation to still check for vendor specific URLs, but ensures full URLs.
Fixes #7543