Cloudflare ad network example by oliy · Pull Request #6882 · ampproject/amphtml

oliy · 2017-01-05T01:02:49Z

For issue #6881

This adds example code for implementing an ad network that uses the Cloudflare A4A signing service.

keithwrightbos

I'd prefer to wait to review this until #6884 has been merged.

dknecht · 2017-01-05T14:24:39Z

Can we still get them in same release?

keithwrightbos · 2017-01-05T14:36:00Z

Very likely, next canary cut should be Wednesday of next week.

…

On Thu, Jan 5, 2017 at 9:24 AM dknecht ***@***.***> wrote: Can we still get them in same release? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#6882 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABr0Lzj0eC252RjT9NGOvNahDr4n_3V8ks5rPP0qgaJpZM4LbPPe> .

jridgewell · 2017-01-05T18:41:15Z

ads/_a4a-config.js

 export const a4aRegistry = map({
  'adsense': adsenseIsA4AEnabled,
  'doubleclick': doubleclickIsA4AEnabled,
+  'cloudflare': cloudflareIsA4AEnabled,


This should only be enabled in dev mode, right?

We will be having some production demos so would like to keep enabled.

oliy · 2017-01-05T20:54:26Z

I got the lint and type check errors out of the way, but I see a dist timeout. Is that an intermittent error?

keithwrightbos · 2017-01-06T18:44:25Z

I noticed that #6884 was closed. Are you planning to use this PR to include the triplelift A4A implementation? If so, can you address my review comments in the original PR?

oliy · 2017-01-09T04:00:51Z

I think I've addressed all your comments from the triple lift implementation. This won't include the triple lift implementation, but hopefully, it will inform them of changes necessary to have a clean submission. I'll be adding in tests this coming week.

keithwrightbos · 2017-01-09T14:59:27Z

extensions/amp-ad-network-cloudflare-impl/0.1/amp-ad-network-cloudflare-impl.js

+import {dev} from '../../../src/log';
+
+
+// Header that will contain Cloudflare generated signature


Correct format for this comment would be as follows. Given that this can be private, please modify name to end in underscore.
`/**

Header that will contain Cloudflare generated signature.

@type {string}

@Private
*/
const AMP_SIGNATURE_HEADER_ = 'X-AmpAdSignature';`

keithwrightbos · 2017-01-09T14:59:56Z

extensions/amp-ad-network-cloudflare-impl/0.1/amp-ad-network-cloudflare-impl.js

+   * @param {!Element} element
+   */
+  /*
+  constructor(element) {


I wanted this file to serve as an example starting point for other implementations. Since many of them actually will need initialization, I've included most of the boilerplate here for guidance/convenience and given instructions to remove this in actual implementations. If you have a strong preference, I'm fine with removing this.

Is this blocking the merge? If so, I will remove this.

I agree with @keithwrightbos . let's remove. we can make the fake-impl as a template.

keithwrightbos · 2017-01-09T15:02:28Z

extensions/amp-ad-network-cloudflare-impl/0.1/amp-ad-network-cloudflare-impl.js

+    //   const rect = this.getIntersectionElementLayoutBox();
+    // can extract the layout size and be used to pass layout hints to the server.
+
+    // Be sure that this doesn't allow the generation of ANY url, since it could


How/when do you plan to add checks?

keithwrightbos · 2017-01-09T15:05:14Z

extensions/amp-ad-network-cloudflare-impl/0.1/amp-ad-network-cloudflare-impl.js

+   * Extract creative and signature from a Cloudflare signed response.
+   *
+   * Note: Invalid A4A content will NOT have a signature, which will automatically
+   *   cause the A4A processing to fall through to the 3p ad flow.


Note that lack of signature does not actually cause the 3p ad flow to be used where a xdomain iframe is created in which custom JS executes to generate an ad request. Instead, it will take the creative given and render it directly within a cross domain frame using either network cache (if Android) or safeframe (if iOS).

keithwrightbos · 2017-01-09T15:06:01Z

extensions/amp-ad-network-cloudflare-impl/0.1/cloudflare-a4a-config.js

+  // check for an explicit attribute to enable A4A processing.
+  // This could be just true, if all ads from this network are A4A, or any
+  // other logic based upon configuration, client settings, etc.
+  return !!element.getAttribute('data-a4a');


Could add check for src attribute existence

oliy · 2017-01-21T03:39:29Z

I've added tests and made the most recently requested changes. Let me know if you'd like anything else. I'd like this to be a clean template for future implementers to start with to integrate Cloudflare ad signing.

dknecht · 2017-01-21T14:10:29Z

cc / @keithwrightbos

keithwrightbos · 2017-01-23T19:54:36Z

Can you explain why it is necessary to create a completely separate extension from the fake-impl? Why not just allow for fake-impl to extract its signing services via an element attribute?

tdrl · 2017-01-23T20:07:52Z

Sorry I'm late to this conversation. Just so I'm clear: Is Cloudflare providing an ad network service? Or only a signing service for other ad networks (e.g., TripleLift)?

tdrl · 2017-01-23T20:10:22Z

extensions/amp-ad-network-cloudflare-impl/0.1/amp-ad-network-cloudflare-impl.js

+    // be sure that this only generates urls to valid hosts, since it could
+    // allow cookie leaks, etc.
+
+    return CLOUDFLARE_BASE_URL + this.element.getAttribute('src');


Since the 'src' attribute is publisher-controlled, is there a possibility for hostile content injection here? Would it be feasible to uriEncode the src?

added encoding.

dknecht · 2017-01-23T20:31:41Z

@tdrl @keithwrightbos We are providing a signing service, but would like to be able to do a full working demo to help make educating potential users easier. We will use for PSA on our blog's amp content.

keithwrightbos · 2017-01-23T21:44:04Z

And you feel that modifying fake as suggested would be insufficient? I'm concerned about making several extensions that will never actually be used. @jridgewell your thoughts?

jridgewell · 2017-01-23T21:48:50Z

This is more a question for @lannka or @cramforce.

lannka · 2017-01-23T21:56:32Z

build-system/server.js

 */
 var BBPromise = require('bluebird');
-var app = require('express')();
+var express = require('express');


is express used?

I had worked around needing this, so this change is no longer needed. I'm removing it.

lannka · 2017-01-23T21:59:10Z

extensions/amp-ad-network-cloudflare-impl/0.1/amp-ad-network-cloudflare-impl.js

+   * @param {!Element} element
+   */
+  /*
+  constructor(element) {


I agree with @keithwrightbos . let's remove. we can make the fake-impl as a template.

dknecht · 2017-01-23T22:16:14Z

I don't believe the fake-impl can be used in production.

cramforce · 2017-01-23T22:52:43Z

I think having such demo code makes sense at this stage to help mature the content. Lets proceed with regular review.

dknecht · 2017-01-24T22:59:42Z

@keithwrightbos are we good to go? We are talking with @jasti about ways to remove in the future.

lannka · 2017-01-24T23:06:53Z

extensions/amp-ad-network-cloudflare-impl/0.1/test/test-amp-ad-network-cloudflare-impl.js

+
+import {AmpAdNetworkCloudflareImpl} from '../amp-ad-network-cloudflare-impl';
+import {
+  AmpAdUIHandler, // eslint-disable-line no-unused-vars


lannka · 2017-01-24T23:10:07Z

extensions/amp-ad-network-cloudflare-impl/0.1/amp-ad-network-cloudflare-impl.js

+    // be sure that this only generates urls to valid hosts, since it could
+    // allow cookie leaks, etc.
+
+    return CLOUDFLARE_BASE_URL + encodeURI(this.element.getAttribute('src'));


encodeURIComponent

Needs to be encodeURI, since this is meant to be the continuation of the URL, to support path references, etc.

lannka · 2017-01-24T23:12:19Z

extensions/amp-ad-network-cloudflare-impl/0.1/amp-ad-network-cloudflare-impl.js

+ * @type {string}
+ * @private
+ */
+//const CLOUDFLARE_BASE_URL = 'http://www.mycloudflaredomain.com/_a4a';


can we remove this and merge JsDoc into the other one?

lannka · 2017-01-24T23:12:49Z

extensions/amp-ad-network-cloudflare-impl/0.1/amp-ad-network-cloudflare-impl.js

+ * @private
+ */
+const CLOUDFLARE_BASE_URL =
+  '/extensions/amp-ad-network-cloudflare-impl/0.1/data';


we can use simple string like: /cloudflare/a4a-data

This follows the pattern from the fake impl to locate test data, so this represents the path to the test data folder.

lannka · 2017-01-24T23:20:34Z

build-system/server.js

+ * Handle CORS headers
+ */
+app.use(cloudflareDir, function fakeCors(req, res, next) {
+    res.setHeader("Access-Control-Allow-Methods", "GET,OPTIONS");


can we reuse the existing method assertCors?

lannka · 2017-01-24T23:22:45Z

build-system/server.js

+/**
+ * Handle fake data
+ */
+app.get(cloudflareDir + '/*', function(req, res) {


can we reuse the existing code at line 543?

you do:

app.get([doubleclickPath, cloudflarePath], function(req, res) { }

This one is different, since it has different header handling than the fake one.

is X-AmpAdSignature the only difference ? we can add it to that method.

lannka · 2017-01-24T23:24:43Z

build-system/server.js

+/**
+ * Handle CORS preflight
+ */
+app.options(cloudflareDir + '/*', function(req, res) {


are we able to merge this into the previous CORS handler?

oliy · 2017-01-25T15:44:59Z

The test failure only occurred after rebasing on master. I'm pretty sure we didn't do anything that would have changed that test result. Also, it looks like timing could affect the test results.

cramforce · 2017-01-25T15:46:37Z

Restarted the test. You can always force-push a new git-sha to get a new run. Apologies for the flaky test.

lannka · 2017-01-25T19:05:01Z

build-system/server.js

+  assertCors(req, res, ['GET', 'OPTIONS'], ['X-AmpAdSignature']);
+
+  if (req.method=='OPTIONS') {
+    res.status(204);


nit: res.status(204).end()

oliy · 2017-01-25T23:46:09Z

@lannka I think I implemented all changes from your review.

* Local debug server changes to fake Cloudflare signing. * Changes to support a new A4A ad network with Cloudflare Signing

jasti assigned lannka and keithwrightbos Jan 5, 2017

keithwrightbos reviewed Jan 5, 2017

View reviewed changes

oliy force-pushed the cloudflare/cf-ad-network branch from 2a5aa26 to d2347d0 Compare January 5, 2017 17:53

jridgewell reviewed Jan 5, 2017

View reviewed changes

oliy force-pushed the cloudflare/cf-ad-network branch from d2347d0 to 026eae1 Compare January 5, 2017 19:44

oliy force-pushed the cloudflare/cf-ad-network branch from 026eae1 to a6ad0c1 Compare January 6, 2017 15:28

keithwrightbos requested changes Jan 9, 2017

View reviewed changes

oliy force-pushed the cloudflare/cf-ad-network branch 3 times, most recently from 41689cc to a500b3d Compare January 21, 2017 02:25

tdrl reviewed Jan 23, 2017

View reviewed changes

lannka reviewed Jan 23, 2017

View reviewed changes

oliy force-pushed the cloudflare/cf-ad-network branch 2 times, most recently from 5e123aa to 2b4316e Compare January 24, 2017 01:05

lannka suggested changes Jan 24, 2017

View reviewed changes

oliy force-pushed the cloudflare/cf-ad-network branch from 2b4316e to 7f695ab Compare January 25, 2017 00:55

keithwrightbos approved these changes Jan 25, 2017

View reviewed changes

lannka reviewed Jan 25, 2017

View reviewed changes

oliy force-pushed the cloudflare/cf-ad-network branch from 7f695ab to 1a71d1e Compare January 25, 2017 19:14

oliy added 2 commits January 25, 2017 17:21

Local debug server changes to fake Cloudflare signing.

24ba0b6

Changes to support a new A4A ad network with Cloudflare Signing

6d41ed3

oliy force-pushed the cloudflare/cf-ad-network branch from 1a71d1e to 6d41ed3 Compare January 25, 2017 23:22

lannka approved these changes Jan 26, 2017

View reviewed changes

lannka merged commit 298920c into ampproject:master Jan 26, 2017

jridgewell pushed a commit to jridgewell/amphtml that referenced this pull request Jan 31, 2017

Cloudflare ad network example (ampproject#6882)

d5b6461

* Local debug server changes to fake Cloudflare signing. * Changes to support a new A4A ad network with Cloudflare Signing

mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017

Cloudflare ad network example (ampproject#6882)

9304ead

* Local debug server changes to fake Cloudflare signing. * Changes to support a new A4A ad network with Cloudflare Signing

		import {dev} from '../../../src/log';


		// Header that will contain Cloudflare generated signature

Conversation

oliy commented Jan 5, 2017

Uh oh!

keithwrightbos left a comment

Choose a reason for hiding this comment

Uh oh!

dknecht commented Jan 5, 2017

Uh oh!

keithwrightbos commented Jan 5, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliy commented Jan 5, 2017

Uh oh!

keithwrightbos commented Jan 6, 2017

Uh oh!

oliy commented Jan 9, 2017

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!

oliy commented Jan 21, 2017

Uh oh!

dknecht commented Jan 21, 2017

Uh oh!

keithwrightbos commented Jan 23, 2017

Uh oh!

tdrl commented Jan 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dknecht commented Jan 23, 2017

Uh oh!

keithwrightbos commented Jan 23, 2017

Uh oh!

jridgewell commented Jan 23, 2017

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!

dknecht commented Jan 23, 2017

Uh oh!

cramforce commented Jan 23, 2017

Uh oh!

dknecht commented Jan 24, 2017

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!