Implement file writing in the privileged chrome://media-app context.

AbstractFile.overwriteOriginal is implemented in the guest context to
send IPC to the privileged host to achieve the file write. To ensure
the correct file is written, a token is passed on IPC load requests.

Only the file that the privileged context determines is "current" may
be written to by the unprivileged guest.

Testing
 - Remove receiver_api.js and depend on receiver.js directly for
   type checking.
 - Mock out the native file system on the host side.
 - Add a test-only IPC to trigger AbstractFile.overwriteOriginal().

Bug: b/146580738
Change-Id: Ie68213e3e1412a2d1fd63d8039d71858f5b4c740
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2090992
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Bugs Nash <bugsnash@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748428}

chromeos/components/media_app_ui/BUILD.gn

@@ -69,7 +69,6 @@ source_set("browser_test_support") {
     "test/driver.js",
     "test/driver_api.js",
     "test/guest_query_receiver.js",
-    "test/receiver_api.js",
   ]
 }
 
@@ -99,27 +98,22 @@ js_type_check("closure_compile_tests") {
     ":test_driver_api_js",
     ":test_driver_js",
     ":test_guest_query_receiver_js",
-    ":test_receiver_api_js",
   ]
 }
 
 js_library("test_driver_api_js") {
   testonly = true
+  externs_list = [ "resources/js/web_app_file_handling.externs.js" ]
   sources = [ "test/driver_api.js" ]
 }
 
-js_library("test_receiver_api_js") {
-  testonly = true
-  sources = [ "test/receiver_api.js" ]
-}
-
 js_library("test_guest_query_receiver_js") {
   testonly = true
   sources = [ "test/guest_query_receiver.js" ]
   deps = [
     ":test_driver_api_js",
-    ":test_receiver_api_js",
     "//chromeos/components/media_app_ui/resources/js:message_pipe",
+    "//chromeos/components/media_app_ui/resources/js:receiver",
   ]
 }
 

chromeos/components/media_app_ui/resources/js/BUILD.gn

@@ -48,6 +48,7 @@ js_library("launch") {
   ]
   deps = [
     ":message_pipe",
+    ":message_types",
     "//chromeos/components/media_app_ui:mojo_bindings_js_library_for_compile",
   ]
 }
@@ -55,9 +56,15 @@ js_library("launch") {
 js_library("message_pipe") {
 }
 
+js_library("message_types") {
+}
+
 js_library("receiver") {
   externs_list = [ "media_app.externs.js" ]
-  deps = [ ":message_pipe" ]
+  deps = [
+    ":message_pipe",
+    ":message_types",
+  ]
 }
 
 js_library("mojo_api_bootstrap") {

chromeos/components/media_app_ui/resources/js/launch.js

@@ -5,7 +5,7 @@
 /**
  * Array of entries available in the current directory.
  *
- * @type {Array<!File>}
+ * @type {Array<{file: !File, handle: !FileSystemFileHandle}>}
  */
 const currentFiles = [];
 
@@ -16,40 +16,67 @@ const currentFiles = [];
  */
 let entryIndex = -1;
 
+/**
+ * Token that identifies the file that is currently writable. Incremented each
+ * time a new file is given focus.
+ * @type {number}
+ */
+let fileToken = 0;
+
+/**
+ * The file currently writable.
+ * @type {?FileSystemFileHandle}
+ */
+let currentlyWritableFileHandle = null;
+
 /** A pipe through which we can send messages to the guest frame. */
 const guestMessagePipe = new MessagePipe('chrome://media-app-guest');
 
 guestMessagePipe.registerHandler('openFeedbackDialog', () => {
   let response = media_app.handler.openFeedbackDialog();
   if (response === null) {
-    response = {errorMessage: 'Null response recieved'};
+    response = {errorMessage: 'Null response received'};
   }
   return response;
 });
 
+guestMessagePipe.registerHandler(Message.OVERWRITE_FILE, async (message) => {
+  const overwrite = /** @type{OverwriteFileMessage} */ (message);
+  if (!currentlyWritableFileHandle || overwrite.token != fileToken) {
+    throw new Error('File not current.');
+  }
+  const writer = await currentlyWritableFileHandle.createWriter();
+  await writer.write(0, overwrite.blob);
+  await writer.truncate(overwrite.blob.size);
+  await writer.close();
+});
+
 /**
  * Loads a file in the guest.
  *
- * @param {File} file
+ * @param {?File} file
+ * @param {!FileSystemFileHandle} handle
  */
-function loadFile(file) {
-  guestMessagePipe.sendMessage('file', {'file': file});
+function loadFile(file, handle) {
+  const token = ++fileToken;
+  currentlyWritableFileHandle = handle;
+  guestMessagePipe.sendMessage(Message.LOAD_FILE, {token, file});
 }
 
 /**
  * Loads a file from a handle received via the fileHandling API.
  *
- * @param {FileSystemHandle} handle
+ * @param {?FileSystemHandle} handle
  * @return {Promise<?File>}
  */
 async function loadFileFromHandle(handle) {
-  if (!handle.isFile) {
+  if (!handle || !handle.isFile) {
     return null;
   }
 
-  const fileHandle = /** @type{FileSystemFileHandle} */ (handle);
+  const fileHandle = /** @type{!FileSystemFileHandle} */ (handle);
   const file = await fileHandle.getFile();
-  loadFile(file);
+  loadFile(file, fileHandle);
   return file;
 }
 
@@ -74,10 +101,10 @@ async function setCurrentDirectory(directory, focusFile) {
 
     // Only allow traversal of matching mime types.
     if (file.type === focusFile.type) {
-      currentFiles.push(file);
+      currentFiles.push({file, handle: fileHandle});
     }
   }
-  entryIndex = currentFiles.findIndex(i => i.name == focusFile.name);
+  entryIndex = currentFiles.findIndex(i => i.file.name == focusFile.name);
 }
 
 /**
@@ -93,7 +120,8 @@ async function advance(direction) {
   if (entryIndex < 0) {
     entryIndex += currentFiles.length;
   }
-  loadFile(currentFiles[entryIndex]);
+  const entry = currentFiles[entryIndex];
+  loadFile(entry.file, entry.handle);
 }
 
 document.getElementById('prev-container')

chromeos/components/media_app_ui/resources/js/media_app_app_scripts.js

@@ -5,4 +5,5 @@
 /** @fileoverview Concatenation of the JS files we use in app.html. */
 
 // <include src="message_pipe.js">
-// <include src="receiver.js">
\ No newline at end of file
+// <include src="message_types.js">
+// <include src="receiver.js">

chromeos/components/media_app_ui/resources/js/media_app_index_scripts.js

@@ -5,4 +5,5 @@
 /** @fileoverview Concatenation of the JS files we use in index.html. */
 
 // <include src="message_pipe.js">
-// <include src="launch.js">
\ No newline at end of file
+// <include src="message_types.js">
+// <include src="launch.js">

chromeos/components/media_app_ui/resources/js/message_pipe.js

@@ -32,6 +32,14 @@ class MessageData {
   }
 }
 
+/**
+ * The Object placed in MessageData.message (and thrown by the Promise returned
+ * by sendMessage) if an exception is caught on the receiving end.
+ *
+ * @typedef {{message: string}}
+ */
+let GenericErrorResponse;
+
 /**
  * The type of a message handler function which gets called when the message
  * pipe receives a message.
@@ -162,6 +170,14 @@ class MessagePipe {
      */
     this.rethrowErrors = rethrowErrors;
 
+    /**
+     * Client error logger. Mockable for tests that check for errors. This is
+     * only used to log errors generated from handlers. Logging occurs on both
+     * sides of the message pipe if rethrowErrors is set, otherwise only on
+     * the side that sent the message.
+     */
+    this.logClientError = (/** * */ object) =>
+        console.error(JSON.stringify(object));
 
     /**
      * Maps a message type to a message handler, a function which takes in
@@ -266,6 +282,7 @@ class MessagePipe {
     if (messageType === RESPONSE_TYPE) {
       resolver.resolve(message);
     } else if (messageType === ERROR_TYPE) {
+      this.logClientError(message);
       resolver.reject(message);
     } else {
       console.error(`Response for message ${
@@ -296,10 +313,12 @@ class MessagePipe {
     try {
       response = await this.messageHandlers_.get(messageType)(message);
     } catch (/** @type {!Error} */ err) {
+      // If an error happened capture the error and send it back.
       error = err;
       sawError = true;
-      // If an error happened capture the error and send it back.
-      response = {message: error.message || ''};
+      /** @type{GenericErrorResponse} */
+      const errorRespose = {message: error.message || ''};
+      response = errorRespose;
     }
 
     this.postToTarget_(error ? ERROR_TYPE : RESPONSE_TYPE, response, messageId);
@@ -307,6 +326,7 @@ class MessagePipe {
     if (sawError && this.rethrowErrors) {
       // Rethrow the error so the current frame has visibility on its handler
       // failures.
+      this.logClientError(error);
       throw error;
     }
   }
@@ -342,10 +362,11 @@ class MessagePipe {
 
     if (!this.messageHandlers_.has(type)) {
       // If there is no listener for this event send a error message to source.
-      this.postToTarget_(
-          ERROR_TYPE,
-          {message: `No handler registered for message type '${type}'`},
-          messageId);
+      /** @type {GenericErrorResponse} */
+      const errorResponse = {
+        message: `No handler registered for message type '${type}'`
+      };
+      this.postToTarget_(ERROR_TYPE, errorResponse, messageId);
       return;
     }
 

chromeos/components/media_app_ui/test/receiver_api.js → chromeos/components/media_app_ui/resources/js/message_types.js

@@ -2,5 +2,22 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-/** @type {!MessagePipe} */
-var parentMessagePipe;
\ No newline at end of file
+/**
+ * @fileoverview
+ * Message definitions passed over the MediaApp privileged/unprivileged pipe.
+ */
+
+/**
+ * Enum for message types.
+ * @enum {string}
+ */
+const Message = {
+  LOAD_FILE: 'load-file',
+  OVERWRITE_FILE: 'overwrite-file',
+};
+
+/** @typedef {{token: number, file: !File}} */
+let OpenFileMessage;
+
+/** @typedef {{token: number, blob: !Blob}} */
+let OverwriteFileMessage;

chromeos/components/media_app_ui/resources/js/receiver.js

@@ -2,6 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+/** A pipe through which we can send messages to the parent frame. */
+const parentMessagePipe = new MessagePipe('chrome://media-app', window.parent);
+
 /** @implements mediaApp.AbstractFileList */
 class SingleArrayBufferFileList {
   /** @param {!mediaApp.AbstractFile} file */
@@ -15,14 +18,52 @@ class SingleArrayBufferFileList {
   }
 }
 
-/** A pipe through which we can send messages to the parent frame. */
-const parentMessagePipe = new MessagePipe('chrome://media-app', window.parent);
+/**
+ * A file received from the privileged context.
+ * @implements {mediaApp.AbstractFile}
+ */
+class ReceivedFile {
+  /**
+   * @param {!File} file The received file.
+   * @param {number} token A token that identifies the file.
+   */
+  constructor(file, token) {
+    this.blob = file;
+    this.name = file.name;
+    this.size = file.size;
+    this.mimeType = file.type;
+    this.token = token;
+  }
 
-parentMessagePipe.registerHandler('file', (message) => {
-  const fileMessage = /** @type mediaApp.MessageEventData */ (message);
-  if (fileMessage.file) {
-    loadFile(fileMessage.file);
+  /**
+   * @override
+   * @param{!Blob} blob
+   */
+  async overwriteOriginal(blob) {
+    /** @type{OverwriteFileMessage} */
+    const message = {token: this.token, blob: blob};
+    const reply =
+        parentMessagePipe.sendMessage(Message.OVERWRITE_FILE, message);
+    try {
+      await reply;
+    } catch (/** @type{GenericErrorResponse} */ errorResponse) {
+      if (errorResponse.message === 'File not current.') {
+        const domError = new DOMError();
+        domError.name = 'NotAllowedError';
+        throw domError;
+      }
+      throw errorResponse;
+    }
+    // Note the following are skipped if an exception is thrown above.
+    this.blob = blob;
+    this.size = blob.size;
+    this.mimeType = blob.type;
   }
+}
+
+parentMessagePipe.registerHandler(Message.LOAD_FILE, (message) => {
+  const fileMessage = /** @type{!OpenFileMessage} */ (message);
+  loadFile(fileMessage.token, fileMessage.file);
 })
 
 /**
@@ -49,15 +90,13 @@ function getApp() {
 
 /**
  * Loads files associated with a message received from the host.
+ * @param {number} token
  * @param {!File} file
+ * @return {!Promise<!ReceivedFile>} The received file (for testing).
  */
-async function loadFile(file) {
-  const fileList = new SingleArrayBufferFileList({
-    blob: file,
-    size: file.size,
-    mimeType: file.type,
-    name: file.name,
-  });
+async function loadFile(token, file) {
+  const receivedFile = new ReceivedFile(file, token);
+  const fileList = new SingleArrayBufferFileList(receivedFile);
 
   const app = getApp();
   if (app) {
@@ -65,6 +104,7 @@ async function loadFile(file) {
   } else {
     window.customLaunchData = {files: fileList};
   }
+  return receivedFile;
 }
 
 /**

chromeos/components/media_app_ui/resources/js/web_app_file_handling.externs.js

@@ -13,14 +13,19 @@ class FileSystemWriter {
   /**
    * @param {number} position
    * @param {BufferSource|Blob|string} data
+   * @return {Promise<undefined>}
    */
   async write(position, data) {}
 
   /**
    * @param {number} size
+   * @return {Promise<undefined>}
    */
   async truncate(size) {}
 
+  /**
+   * @return {Promise<undefined>}
+   */
   async close() {}
 }
 

chromeos/components/media_app_ui/test/driver.js

@@ -24,3 +24,60 @@ class GuestDriver {
     return result.testQueryResult;
   }
 }
+
+/** @implements FileSystemWriter */
+class FakeFileWriter {
+  constructor(/** !Blob= */ data = new Blob()) {
+    this.data = data;
+
+    /** @type{function(!Blob)} */
+    this.resolveClose;
+
+    this.closePromise = new Promise((/** function(!Blob) */ resolve) => {
+      this.resolveClose = resolve;
+    });
+  }
+  /** @override */
+  async write(position, data) {
+    this.data = new Blob([
+      this.data.slice(0, position),
+      data,
+      this.data.slice(position + data.size),
+    ]);
+  }
+  /** @override */
+  async truncate(size) {
+    this.data = this.data.slice(0, size);
+  }
+  /** @override */
+  async close() {
+    this.resolveClose(this.data);
+  }
+}
+
+/** @implements FileSystemFileHandle  */
+class FakeFileSystemFileHandle {
+  constructor() {
+    this.isFile = true;
+    this.isDirectory = false;
+    this.name = 'fakefile';
+
+    /** @type{?FakeFileWriter} */
+    this.lastWriter;
+  }
+
+  /** @override */
+  queryPermission(descriptor) {}
+  /** @override */
+  requestPermission(descriptor) {}
+  /** @override */
+  createWriter(options) {
+    this.lastWriter = new FakeFileWriter();
+    return Promise.resolve(this.lastWriter);
+  }
+  /** @override */
+  getFile() {
+    console.error('getFile() not implemented');
+    return null;
+  }
+}

chromeos/components/media_app_ui/test/driver_api.js

@@ -10,9 +10,11 @@ var TestMessageResponseData;
  *     testQuery: string,
  *     pathToRoot: (Array<string>|undefined),
  *     property: (string|undefined),
- *     requestFullscreen: (boolean|undefined)}}
+ *     requestFullscreen: (boolean|undefined),
+ *     overwriteLastFile: (string|undefined)
+ * }}
  */
 var TestMessageQueryData;
 
 /** @type {MessagePipe} */
-var guestMessagePipe;
\ No newline at end of file
+var guestMessagePipe;

chromeos/components/media_app_ui/test/guest_query_receiver.js

@@ -2,6 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+/**
+ * The last file loaded into the guest, updated via a spy on loadFile().
+ * @type{?Promise<!ReceivedFile>}
+ */
+let lastReceivedFile = null;
+
 /**
  * Repeatedly runs a query selector until it finds an element.
  *
@@ -44,18 +50,26 @@ async function waitForNode(query, opt_path) {
  * @return {!Promise<TestMessageResponseData>}
  */
 async function runTestQuery(data) {
-  const element = await waitForNode(data.testQuery, data.pathToRoot || []);
-  let result = element.tagName;
+  let result = 'no result';
+  if (data.testQuery) {
+    const element = await waitForNode(data.testQuery, data.pathToRoot || []);
+    result = element.tagName;
 
-  if (data.property) {
-    result = JSON.stringify(element[data.property]);
-  } else if (data.requestFullscreen) {
-    try {
-      await element.requestFullscreen();
-      result = 'hooray';
-    } catch (/** @type{TypeError} */ typeError) {
-      result = typeError.message;
+    if (data.property) {
+      result = JSON.stringify(element[data.property]);
+    } else if (data.requestFullscreen) {
+      try {
+        await element.requestFullscreen();
+        result = 'hooray';
+      } catch (/** @type{TypeError} */ typeError) {
+        result = typeError.message;
+      }
     }
+  } else if (data.overwriteLastFile) {
+    const testBlob = new Blob([data.overwriteLastFile]);
+    const ensureLoaded = await lastReceivedFile;
+    await ensureLoaded.overwriteOriginal(testBlob);
+    result = 'overwriteOriginal resolved';
   }
 
   return {testQueryResult: result};
@@ -75,6 +89,17 @@ window.addEventListener('DOMContentLoaded', () => {
   parentMessagePipe.registerHandler('bad-handler', () => {
     throw Error('This is an error');
   });
+
+  // Log errors, rather than send them to console.error.
+  parentMessagePipe.logClientError = error =>
+      console.log(JSON.stringify(error));
+
+  // Install spies.
+  const realLoadFile = loadFile;
+  loadFile = async (/** number */ token, /** !File */ file) => {
+    lastReceivedFile = realLoadFile(token, file);
+    return lastReceivedFile;
+  }
 });
 
 //# sourceURL=guest_query_receiver.js

chromeos/components/media_app_ui/test/media_app_ui_browsertest.js

@@ -97,7 +97,7 @@ TEST_F('MediaAppUIBrowserTest', 'GuestCanLoad', async () => {
 });
 
 TEST_F('MediaAppUIBrowserTest', 'LoadFile', async () => {
-  loadFile(await createTestImageFile());
+  loadFile(await createTestImageFile(), new FakeFileSystemFileHandle());
   const result =
       await driver.waitForElementInGuest('img[src^="blob:"]', 'naturalWidth');
 
@@ -124,7 +124,9 @@ TEST_F('MediaAppUIBrowserTest', 'CanFullscreenVideo', async () => {
 
   // Load a zero-byte video. It won't load, but the video element should be
   // added to the DOM (and it can still be fullscreened).
-  loadFile(new File([], 'zero_byte_video.webm', {type: 'video/webm'}));
+  loadFile(
+      new File([], 'zero_byte_video.webm', {type: 'video/webm'}),
+      new FakeFileSystemFileHandle());
 
   const SELECTOR = 'video';
   const tagName = await driver.waitForElementInGuest(
@@ -142,7 +144,9 @@ TEST_F('MediaAppUIBrowserTest', 'CanFullscreenVideo', async () => {
 
 // Tests that we receive an error if our message is unhandled.
 TEST_F('MediaAppUIBrowserTest', 'ReceivesNoHandlerError', async () => {
+  guestMessagePipe.logClientError = error => console.log(JSON.stringify(error));
   let errorMessage = '';
+
   try {
     await guestMessagePipe.sendMessage('unknown-message', null);
   } catch (error) {
@@ -157,7 +161,9 @@ TEST_F('MediaAppUIBrowserTest', 'ReceivesNoHandlerError', async () => {
 
 // Tests that we receive an error if the handler fails.
 TEST_F('MediaAppUIBrowserTest', 'ReceivesProxiedError', async () => {
+  guestMessagePipe.logClientError = error => console.log(JSON.stringify(error));
   let errorMessage = '';
+
   try {
     await guestMessagePipe.sendMessage('bad-handler', null);
   } catch (error) {
@@ -167,3 +173,22 @@ TEST_F('MediaAppUIBrowserTest', 'ReceivesProxiedError', async () => {
   assertEquals(errorMessage, 'This is an error');
   testDone();
 });
+
+// Tests the IPC behind the implementation of ReceivedFile.overwriteOriginal()
+// in the untrusted context. Ensures it correctly updates the file handle owned
+// by the privileged context.
+TEST_F('MediaAppUIBrowserTest', 'OverwriteOriginalIPC', async () => {
+  const handle = new FakeFileSystemFileHandle();
+  loadFile(await createTestImageFile(), handle);
+
+  // Write should not be called initially.
+  assertEquals(undefined, handle.lastWriter);
+
+  const message = {overwriteLastFile: 'Foo'};
+  const testResponse = await guestMessagePipe.sendMessage('test', message);
+  const writeResult = await handle.lastWriter.closePromise;
+
+  assertEquals(testResponse.testQueryResult, 'overwriteOriginal resolved');
+  assertEquals(await writeResult.text(), 'Foo');
+  testDone();
+});