Adopt ArrayBuffer to pass sync encryption keys

Encryption keys can contain non-utf8 binary content and are better
represented by ArrayBuffer being adopted in the API.

The patch also reorders the parameter order for future expansion
options.

Bug: 1000146
Change-Id: I7d1da9320c7e5c512f657deb5b211d3cf46d06d7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1919935
Commit-Queue: David Trainor <dtrainor@chromium.org>
Auto-Submit: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#718313}

chrome/browser/sync/sync_encryption_keys_tab_helper.cc

@@ -40,6 +40,22 @@ bool ShouldExposeMojoApi(content::NavigationHandle* navigation_handle) {
   return url::Origin::Create(navigation_handle->GetURL()) == GetAllowedOrigin();
 }
 
+// TODO(crbug.com/1027676): Migrate away from std::string and adopt some safer
+// type (std::vector<uint8_t> or newly-introduced class) to represent encryption
+// keys.
+std::string BytesAsString(const std::vector<uint8_t>& bytes) {
+  return std::string(reinterpret_cast<const char*>(bytes.data()), bytes.size());
+}
+
+std::vector<std::string> EncryptionKeysAsStrings(
+    const std::vector<std::vector<uint8_t>>& encryption_keys) {
+  std::vector<std::string> encryption_keys_as_strings;
+  for (const auto& encryption_key : encryption_keys) {
+    encryption_keys_as_strings.push_back(BytesAsString(encryption_key));
+  }
+  return encryption_keys_as_strings;
+}
+
 }  // namespace
 
 class SyncEncryptionKeysTabHelper::EncryptionKeyApi
@@ -53,14 +69,15 @@ class SyncEncryptionKeysTabHelper::EncryptionKeyApi
   }
 
   // chrome::mojom::SyncEncryptionKeysExtension:
-  void SetEncryptionKeys(const std::vector<std::string>& encryption_keys,
-                         const std::string& gaia_id,
-                         SetEncryptionKeysCallback callback) override {
+  void SetEncryptionKeys(
+      const std::vector<std::vector<uint8_t>>& encryption_keys,
+      const std::string& gaia_id,
+      SetEncryptionKeysCallback callback) override {
     CHECK_EQ(bindings_.GetCurrentTargetFrame()->GetLastCommittedOrigin(),
              GetAllowedOrigin());
 
     sync_service_->GetUserSettings()->AddTrustedVaultDecryptionKeys(
-        gaia_id, encryption_keys);
+        gaia_id, EncryptionKeysAsStrings(encryption_keys));
     std::move(callback).Run();
   }
 

chrome/common/sync_encryption_keys_extension.mojom

@@ -11,5 +11,5 @@ interface SyncEncryptionKeysExtension {
   // Provides sync encryption keys to the browser process. Returns true if no
   // further encryption keys are needed (which includes the case, among others,
   // of encryption keys not being needed even prior to this call).
-  SetEncryptionKeys(array<string> encryption_keys, string gaia_id) => ();
+  SetEncryptionKeys(array<array<uint8>> encryption_keys, string gaia_id) => ();
 };

chrome/renderer/sync_encryption_keys_extension.cc

@@ -30,6 +30,26 @@ const url::Origin& GetAllowedOrigin() {
   return *origin;
 }
 
+// This function is intended to convert a binary blob representing an encryption
+// key and provided by the web via a Javascript ArrayBuffer.
+std::vector<uint8_t> ArrayBufferAsBytes(
+    const v8::Local<v8::ArrayBuffer>& array_buffer) {
+  auto backing_store = array_buffer->GetBackingStore();
+  const uint8_t* start =
+      reinterpret_cast<const uint8_t*>(backing_store->Data());
+  const size_t length = backing_store->ByteLength();
+  return std::vector<uint8_t>(start, start + length);
+}
+
+std::vector<std::vector<uint8_t>> EncryptionKeysAsBytes(
+    const std::vector<v8::Local<v8::ArrayBuffer>>& encryption_keys) {
+  std::vector<std::vector<uint8_t>> encryption_keys_as_bytes;
+  for (const v8::Local<v8::ArrayBuffer>& encryption_key : encryption_keys) {
+    encryption_keys_as_bytes.push_back(ArrayBufferAsBytes(encryption_key));
+  }
+  return encryption_keys_as_bytes;
+}
+
 }  // namespace
 
 // static
@@ -93,31 +113,40 @@ void SyncEncryptionKeysExtension::Install() {
 void SyncEncryptionKeysExtension::SetSyncEncryptionKeys(gin::Arguments* args) {
   DCHECK(render_frame());
 
+  // This function as exposed to the web has the following signature:
+  //   setSyncEncryptionKeys(callback, gaia_id, encryption_keys)
+  //
+  // Where:
+  //   callback: Allows caller to get notified upon completion.
+  //   gaia_id: String representing the user's server-provided ID.
+  //   encryption_keys: Array where each element is an ArrayBuffer representing
+  //                    an encryption key (binary blob).
+
   v8::HandleScope handle_scope(args->isolate());
 
-  std::vector<std::string> encryption_keys;
-  if (!args->GetNext(&encryption_keys)) {
-    DLOG(ERROR) << "Not array of strings";
+  v8::Local<v8::Function> callback;
+  if (!args->GetNext(&callback)) {
+    DLOG(ERROR) << "No callback";
     args->ThrowError();
     return;
   }
 
-  if (encryption_keys.empty()) {
-    DLOG(ERROR) << "Array of strings empty";
+  std::string gaia_id;
+  if (!args->GetNext(&gaia_id)) {
+    DLOG(ERROR) << "No account ID";
     args->ThrowError();
     return;
   }
 
-  std::string gaia_id;
-  if (!args->GetNext(&gaia_id)) {
-    DLOG(ERROR) << "No account ID";
+  std::vector<v8::Local<v8::ArrayBuffer>> encryption_keys;
+  if (!args->GetNext(&encryption_keys)) {
+    DLOG(ERROR) << "Not array of strings";
     args->ThrowError();
     return;
   }
 
-  v8::Local<v8::Function> callback;
-  if (!args->GetNext(&callback)) {
-    DLOG(ERROR) << "No callback";
+  if (encryption_keys.empty()) {
+    DLOG(ERROR) << "Array of strings empty";
     args->ThrowError();
     return;
   }
@@ -130,7 +159,7 @@ void SyncEncryptionKeysExtension::SetSyncEncryptionKeys(gin::Arguments* args) {
   }
 
   remote_->SetEncryptionKeys(
-      encryption_keys, gaia_id,
+      EncryptionKeysAsBytes(encryption_keys), gaia_id,
       base::BindOnce(&SyncEncryptionKeysExtension::RunCompletionCallback,
                      weak_ptr_factory_.GetWeakPtr(),
                      std::move(global_callback)));

chrome/test/data/sync/encryption_keys_retrieval.html

 <html>
 <head>
 <script>
+// This particular test page uses ASCII keys for simplicity. Real callers are
+// not expected to deal with strings and converting them via charCodeAt().
+// TODO(crbug.com/978430): Pass an actual binary (non-ASCII) key.
+function asciiToArrayBuffer(s) {
+  var buffer = new ArrayBuffer(s.length);
+  var view = new Uint8Array(buffer);
+  for (var i = 0; i < s.length; i++) {
+    view[i] = s.charCodeAt(i);
+  }
+  return buffer;
+}
+
 window.onload = function() {
   if (typeof chrome.setSyncEncryptionKeys == "undefined") {
     document.title = "UNDEFINED";
   } else {
-    chrome.setSyncEncryptionKeys([location.hash.substring(1)],
+    chrome.setSyncEncryptionKeys(() => { document.title = "OK"; },
                                  location.search.substring(1),
-                                 () => { document.title = "OK"; });
+                                 [asciiToArrayBuffer(location.hash.substring(1))]);
   }
 }
 </script>