Bug 1642855 - remove flowID from plaintext send-tab payload. r=markh

Differential Revision: https://phabricator.services.mozilla.com/D80793

services/fxaccounts/FxAccountsCommands.js

@@ -233,9 +233,11 @@ class SendTab {
         const targetData = Object.assign({ flowID, streamID }, data);
         const bytes = encoder.encode(JSON.stringify(targetData));
         const encrypted = await this._encrypt(bytes, device);
-        // TODO: remove flowID from the payload.
-        const payload = { encrypted, flowID };
-        await this._commands.invoke(COMMAND_SENDTAB, device, payload); // FxA needs an object.
+        // FxA expects an object as the payload, but we only have a single encrypted string; wrap it.
+        // If you add any plaintext items to this payload, please carefully consider the privacy implications
+        // of revealing that data to the FxA server.
+        const payload = { encrypted };
+        await this._commands.invoke(COMMAND_SENDTAB, device, payload);
         this._fxai.telemetry.recordEvent(
           "command-sent",
           COMMAND_SENDTAB_TAIL,
@@ -264,7 +266,7 @@ class SendTab {
   }
 
   // Handle incoming send tab payload, called by FxAccountsCommands.
-  async handle(senderID, { encrypted, flowID: deprecatedFlowID }) {
+  async handle(senderID, { encrypted }) {
     const bytes = await this._decrypt(encrypted);
     const decoder = new TextDecoder("utf8");
     const data = JSON.parse(decoder.decode(bytes));
@@ -280,7 +282,7 @@ class SendTab {
       "command-received",
       COMMAND_SENDTAB_TAIL,
       this._fxai.telemetry.sanitizeDeviceId(senderID),
-      { flowID: flowID || deprecatedFlowID, streamID }
+      { flowID, streamID }
     );
 
     return {

services/fxaccounts/tests/xpcshell/test_commands.js

@@ -186,8 +186,9 @@ add_task(async function test_sendtab_receive() {
 
   for (let { cmd, device, payload } of commands._invokes) {
     Assert.equal(cmd, COMMAND_SENDTAB);
-    // test we do the right thing with the "duplicated" flow ID.
-    Assert.equal(payload.flowID, "1");
+    // Older Firefoxes would send a plaintext flowID in the top-level payload.
+    // Test that we sensibly ignore it.
+    Assert.ok(!payload.hasOwnProperty("flowID"));
     // change it - ensure we still get what we expect in telemetry later.
     payload.flowID = "ignore-me";
     Assert.deepEqual(await sendTab.handle(device.id, payload), {
@@ -213,7 +214,7 @@ add_task(async function test_sendtab_receive() {
 });
 
 // Test that a client which only sends the flowID in the envelope and not in the
-// encrypted body still gets recorded correctly.
+// encrypted body gets recorded without the flowID.
 add_task(async function test_sendtab_receive_old_client() {
   const fxai = FxaInternalMock();
   const sendTab = new SendTab(null, fxai);
@@ -234,7 +235,7 @@ add_task(async function test_sendtab_receive_old_client() {
       value: "sender-id-san",
       // deepEqual doesn't ignore undefined, but our telemetry code and
       // JSON.stringify() do...
-      extra: { flowID: "flow-id", streamID: undefined },
+      extra: { flowID: undefined, streamID: undefined },
     },
   ]);
 });