Moves closure sha384 into a new extension amp-crypto-polyfill for lazy load #7006

lannka · 2017-01-12T06:14:42Z

This PR introduces our first "lib-like" extension amp-crypto-polyfill.
Part of #4898

/cc @keithwrightbos This should unblock your refactoring in #6837

…y load.

jridgewell · 2017-01-12T19:37:19Z

extensions/amp-analytics/0.1/crypto-impl.js

+                  dev().error(TAG, FALLBACK_MSG, e);
+                }
+                return this.loadPolyfill_()
+                    .then(polyfill => polyfill.sha384(input));


Can we call this.sha384(input) instead? The correct polyfill method is already codified above.

that will not work. we need to call loadPolyfill here otherwise this.polyfillPromise_ is null forever

Yah, but the then block doesn't need to know about polyfill#sha384.

return this.loadPolyfill_().then(this.sha384(input));

jridgewell · 2017-01-12T19:38:33Z

extensions/amp-analytics/0.1/crypto-impl.js

+              });
+    } catch (e) {
+      dev().error(TAG, FALLBACK_MSG, e);
+      return this.loadPolyfill_().then(polyfill => polyfill.sha384(input));


jridgewell · 2017-01-12T19:39:15Z

extensions/amp-analytics/0.1/crypto-impl.js

+    if (this.polyfillPromise_) {
+      return this.polyfillPromise_;
+    }
+    return this.polyfillPromise_ = extensionsFor(this.win_)


Need to add the polyfillPromise_ property in the constructor, regardless of subtle_.

jridgewell · 2017-01-12T19:39:43Z

extensions/amp-analytics/0.1/crypto-impl.js

@@ -70,9 +83,7 @@ export class Crypto {
   * @throws {!Error} when input string contains chars out of range [0,255]
   */
  sha384Base64(input) {
-    return this.sha384(input).then(buffer => {
-      return lib.base64(buffer);


Can we please drop the base64 stuff from the library?! 😋

oh ye, will do

done. but no size change.

Really? Does sha384 internally depend on it?

Ohh, we might have to update https://github.com/ampproject/amphtml/blob/fa5c6cc7a9a78802bc816b6a986543420d90917f/third_party/closure-library/compile.sh, too.

yep, i've updated it already. did you see anything else i can strip off?

Oh perfect, not sure how I missed that.

dvoytenko · 2017-01-12T21:25:52Z

extensions/amp-crypto-polyfill/0.1/amp-crypto-polyfill.js

+import * as lib from '../../../third_party/closure-library/sha384-generated';
+
+// Register doc-service factory.
+AMP.registerServiceForDoc(


Should we just do window-singleton service instead? It really always window-scoped, right? Just as WebCrypto itself...

Small correction:
installCryptoPolyfill(window);

->

installCryptoPolyfill(AMP.win);

dvoytenko · 2017-01-12T21:27:42Z

extensions/amp-analytics/0.1/crypto-impl.js

-      }
+    // polyfill is (being) loaded,
+    // means native Crypto API is not available or failed before.
+    if (this.polyfillPromise_) {


This var should be defined in the constructor.

dvoytenko · 2017-01-12T21:27:51Z

extensions/amp-analytics/0.1/crypto-impl.js

@@ -28,6 +30,13 @@ export class Crypto {
  constructor(win) {
    /** @private @const {?webCrypto.SubtleCrypto} */
    this.subtle_ = getSubtle(win);
+
+    /** @private {!Window} */


Move on top.

dvoytenko · 2017-01-12T21:27:58Z

extensions/amp-analytics/0.1/crypto-impl.js

@@ -28,6 +30,13 @@ export class Crypto {
  constructor(win) {


dvoytenko · 2017-01-12T21:28:47Z

extensions/amp-analytics/0.1/crypto-impl.js

 import {fromClass} from '../../../src/service';
 import {dev} from '../../../src/log';
+import {getExistingServiceForWindow} from '../../../src/service';


So, this module now just needs to go to the src/, right?

Also, please add tests.

@keithwrightbos has a pending PR to do that. Tests updated.

dvoytenko · 2017-01-12T21:30:40Z

extensions/amp-analytics/0.1/crypto-impl.js

+    }
+    try {
+      return this.subtle_.digest({name: 'SHA-384'},
+              input instanceof Uint8Array ? input : stringToBytes(input))


Should this be the other way: if (typeof input == 'string') ? stringToBytes(input) : input?

dvoytenko · 2017-01-12T21:30:55Z

extensions/amp-analytics/0.1/crypto-impl.js

+    try {
+      return this.subtle_.digest({name: 'SHA-384'},
+              input instanceof Uint8Array ? input : stringToBytes(input))
+          // [].slice.call(Unit8Array) is a shim for Array.from(Unit8Array)


dvoytenko · 2017-01-12T21:33:03Z

extensions/amp-analytics/0.1/crypto-impl.js

+    }
+    return this.polyfillPromise_ = extensionsFor(this.win_)
+        .loadExtension('amp-crypto-polyfill')
+        .then(() => getExistingServiceForWindow(this.win_, 'crypto-polyfill'));


You already lookup window-scoped service, so it should definitely be window-scoped in the polyfill itself.

dvoytenko · 2017-01-12T21:34:22Z

extensions/amp-analytics/0.1/crypto-impl.js

+
+  /**
+   * Loads Crypto polyfill library.
+   * @returns {!Promise}


This should be !Promise<????> since you are using response of this promise everywhere.

Not seeing changes here. Did you push?

Also, it should be @return (no "s")

obviously i lied. done now.

lannka

Comments addressed. PTAL

lannka · 2017-01-13T04:34:36Z

extensions/amp-analytics/0.1/crypto-impl.js

 import {fromClass} from '../../../src/service';
 import {dev} from '../../../src/log';
+import {getExistingServiceForWindow} from '../../../src/service';


@keithwrightbos has a pending PR to do that. Tests updated.

lannka · 2017-01-13T04:35:07Z

extensions/amp-analytics/0.1/crypto-impl.js

@@ -28,6 +30,13 @@ export class Crypto {
  constructor(win) {


lannka · 2017-01-13T04:35:23Z

extensions/amp-analytics/0.1/crypto-impl.js

@@ -28,6 +30,13 @@ export class Crypto {
  constructor(win) {
    /** @private @const {?webCrypto.SubtleCrypto} */
    this.subtle_ = getSubtle(win);
+
+    /** @private {!Window} */


lannka · 2017-01-13T04:35:28Z

extensions/amp-analytics/0.1/crypto-impl.js

-      }
+    // polyfill is (being) loaded,
+    // means native Crypto API is not available or failed before.
+    if (this.polyfillPromise_) {


lannka · 2017-01-13T04:36:06Z

extensions/amp-analytics/0.1/crypto-impl.js

+    try {
+      return this.subtle_.digest({name: 'SHA-384'},
+              input instanceof Uint8Array ? input : stringToBytes(input))
+          // [].slice.call(Unit8Array) is a shim for Array.from(Unit8Array)


lannka · 2017-01-13T04:36:19Z

extensions/amp-analytics/0.1/crypto-impl.js

+
+  /**
+   * Loads Crypto polyfill library.
+   * @returns {!Promise}


lannka · 2017-01-13T04:36:24Z

extensions/amp-analytics/0.1/crypto-impl.js

+    if (this.polyfillPromise_) {
+      return this.polyfillPromise_;
+    }
+    return this.polyfillPromise_ = extensionsFor(this.win_)


lannka · 2017-01-13T04:36:31Z

extensions/amp-analytics/0.1/crypto-impl.js

+    }
+    return this.polyfillPromise_ = extensionsFor(this.win_)
+        .loadExtension('amp-crypto-polyfill')
+        .then(() => getExistingServiceForWindow(this.win_, 'crypto-polyfill'));


lannka · 2017-01-13T04:36:37Z

extensions/amp-crypto-polyfill/0.1/amp-crypto-polyfill.js

+import * as lib from '../../../third_party/closure-library/sha384-generated';
+
+// Register doc-service factory.
+AMP.registerServiceForDoc(


dvoytenko · 2017-01-13T18:31:18Z

extensions/amp-analytics/0.1/crypto-impl.js

  }

  /**
   * Returns the SHA-384 hash of the input string in a number array.
   * Input string cannot contain chars out of range [0,255].
   * @param {string|!Uint8Array} input
-   * @returns {!Promise<!Array<number>>}
+   * @returns {!Promise<!Uint8Array>}


dvoytenko · 2017-01-13T18:33:40Z

extensions/amp-analytics/0.1/crypto-impl.js

-      } catch (e) {
-        dev().error(TAG, FALLBACK_MSG, e);
-      }
+    if (!(input instanceof Uint8Array)) {


We are usually careful with instanceof b/c of different window contexts. E.g. window.UInt8Array vs embedWin.UInt8Array. Is this not a concern here?

/cc @jridgewell

Friendly Frames are the only thing that can violate that, right? We can do a !input.buffer check instead.

Correct. Only friendly iframes.

hi gentlemen, what is this?
Uint8Array can be different across windows in same browser?

Each window has its own instance of the generic classes, take String for instance. Now, when we create an iframe, it gets its own window and its own String. If we compare, they are not equivalent:

window.String === iframe.contentWindow.String; // => false

Since instanceof looks for object equality in the prototype chain, this also breaks across iframe boundaries:

iframe.contentWindow.uint8instance instanceof Uint8Array; // => false uint8instance instanceof iframe.contentWindow.Uint8Array; // => false

wow. thanks!
will change to typeof input === "string"

dvoytenko · 2017-01-13T18:36:26Z

@lannka Couple more comments, but otherwise LGTM.

* master: (310 commits) Update csa.md to remove non-required parameters (ampproject#6902) Add notes about requesting ads ATF and link to demo (ampproject#7037) Remove whitelist for lightbox scrollable validator (ampproject#7034) Delegate submit events until amp-form is loaded (ampproject#6929) Moves closure sha384 into a new extension amp-crypto-polyfill for lazy load (ampproject#7006) Refactor observables in viewer-impl into a map object (ampproject#7004) resizing of margins (ampproject#6824) Use URL replacer from embed for pixel (ampproject#7029) adds support for Gemius analytics (ampproject#6558) Avoid duplicating server-layout (ampproject#7021) Laterpay validator config (ampproject#6974) Validator rollup (ampproject#7023) skeleton for amp-tabs (ampproject#7003) Upgrade post-css and related packages to latest (ampproject#7020) handle unload (ampproject#7001) viewer-integr.js -> amp-viewer-integration (ampproject#6989) dev().info()->dev().fine() (ampproject#7017) Turned on experiment flag (ampproject#6774) Unlaunch ios-embed-wrapper for iOS8 to avoid scroll freezing issues (ampproject#7018) Add some A4A ad request parameters (ampproject#6643) ...

…y load (ampproject#7006) * Moves closure sha384 into a new extension amp-crypto-polyfill for lazy load. * fix * Update tests. Remove base64 polyfill from lib. * Fix presubmit * Address comments * Fix test.

Moves closure sha384 into a new extension amp-crypto-polyfill for laz…

ca82f7f

…y load.

lannka added the WIP label Jan 12, 2017

lannka assigned dvoytenko Jan 12, 2017

fix

dd07338

jridgewell reviewed Jan 12, 2017

View reviewed changes

dvoytenko suggested changes Jan 12, 2017

View reviewed changes

Update tests. Remove base64 polyfill from lib.

217c101

lannka commented Jan 13, 2017

View reviewed changes

lannka removed the WIP label Jan 13, 2017

Fix presubmit

c2398e0

dvoytenko reviewed Jan 13, 2017

View reviewed changes

dvoytenko approved these changes Jan 13, 2017

View reviewed changes

lannka added 2 commits January 13, 2017 12:04

Address comments

a879a13

Fix test.

fee7e71

lannka merged commit 17136cf into ampproject:master Jan 13, 2017

lannka deleted the crypto-polyfill branch January 13, 2017 21:49

erwinmombay mentioned this pull request Jan 18, 2017

Meeting Notes 2017-01-17 #7094

Closed

lannka mentioned this pull request Feb 10, 2017

Lazy load closure sha384 lib when browser native Crypto API is not available #3888

Closed

Moves closure sha384 into a new extension amp-crypto-polyfill for lazy load #7006

Moves closure sha384 into a new extension amp-crypto-polyfill for lazy load #7006

Conversation

lannka commented Jan 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lannka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell Jan 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Jan 13, 2017

lannka commented Jan 12, 2017 •

edited

jridgewell Jan 13, 2017 •

edited