From 1283c77518b9483110bc74dd098e92baf031e89a Mon Sep 17 00:00:00 2001
From: Daniel Gasienica <daniel@signal.org>
Date: Tue, 13 Mar 2018 17:42:12 -0400
Subject: [PATCH] Switch from hashed to random attachment file names

Using hashes, we get the benefit of deduplication but if a user receives two
messages with the same attachment, deleting one would delete it for both since
they are only stored once. To avoid the complexity of tracking number of
references, we simply generate random file names similar to iMessage on MacOS
(?) and Signal Android.
---
 app/types/attachment/write_attachment_data.js | 22 +++++-----------
 .../attachment/write_attachment_data_test.js  | 26 ++++++++-----------
 2 files changed, 17 insertions(+), 31 deletions(-)

diff --git a/app/types/attachment/write_attachment_data.js b/app/types/attachment/write_attachment_data.js
index 8f6f6787243..5634457d9f1 100644
--- a/app/types/attachment/write_attachment_data.js
+++ b/app/types/attachment/write_attachment_data.js
@@ -1,7 +1,6 @@
 const crypto = require('crypto');
 const FSE = require('fs-extra');
 const isArrayBuffer = require('lodash/isArrayBuffer');
-const isBuffer = require('lodash/isBuffer');
 const isString = require('lodash/isString');
 const Path = require('path');
 
@@ -17,29 +16,20 @@ exports.writeAttachmentData = (root) => {
     }
 
     const buffer = new Buffer(arrayBuffer);
-    const path = Path.join(root, exports._getAttachmentPath(buffer));
+    const path = Path.join(root, exports._getAttachmentPath());
     await FSE.ensureFile(path);
     await FSE.writeFile(path, buffer);
     return path;
   };
 };
 
-exports._getAttachmentName = (buffer) => {
-  if (!isBuffer(buffer)) {
-    throw new TypeError('`buffer` must be a buffer');
-  }
-
-  const hash = crypto.createHash('sha256');
-  hash.update(buffer);
-  return hash.digest('hex');
+exports._getAttachmentName = () => {
+  const buffer = crypto.randomBytes(32);
+  return buffer.toString('hex');
 };
 
-exports._getAttachmentPath = (buffer) => {
-  if (!isBuffer(buffer)) {
-    throw new TypeError('`buffer` must be a buffer');
-  }
-
-  const name = exports._getAttachmentName(buffer);
+exports._getAttachmentPath = () => {
+  const name = exports._getAttachmentName();
   const prefix = name.slice(0, 3);
   return Path.join(prefix, name);
 };
diff --git a/test/app/types/attachment/write_attachment_data_test.js b/test/app/types/attachment/write_attachment_data_test.js
index a3b40602013..61a992d8832 100644
--- a/test/app/types/attachment/write_attachment_data_test.js
+++ b/test/app/types/attachment/write_attachment_data_test.js
@@ -2,7 +2,7 @@ const FSE = require('fs-extra');
 const isEqual = require('lodash/isEqual');
 const Path = require('path');
 const stringToArrayBuffer = require('string-to-arraybuffer');
-const tempy = require('tempy');
+const tmp = require('tmp');
 const { assert } = require('chai');
 
 const {
@@ -12,11 +12,15 @@ const {
 } = require('../../../../app/types/attachment/write_attachment_data');
 
 
+const PREFIX_LENGTH = 3;
+const NUM_SEPARATORS = 1;
+const NAME_LENGTH = 64;
+const PATH_LENGTH = PREFIX_LENGTH + NUM_SEPARATORS + NAME_LENGTH;
+
 describe('writeAttachmentData', () => {
   let TEMPORARY_DIRECTORY = null;
   before(() => {
-    // Sync!
-    TEMPORARY_DIRECTORY = tempy.directory();
+    TEMPORARY_DIRECTORY = tmp.dirSync().name;
   });
 
   after(async () => {
@@ -26,33 +30,25 @@ describe('writeAttachmentData', () => {
   it('should write file to disk and return path', async () => {
     const input = stringToArrayBuffer('test string');
     const tempDirectory = Path.join(TEMPORARY_DIRECTORY, 'writeAttachmentData');
-    const expectedPath = Path.join(
-      tempDirectory,
-      'd55/d5579c46dfcc7f18207013e65b44e4cb4e2c2298f4ac457ba8f82743f31e930b'
-    );
 
     const outputPath = await writeAttachmentData(tempDirectory)(input);
     const output = await FSE.readFile(outputPath);
 
-    assert.strictEqual(outputPath, expectedPath);
+    assert.lengthOf(Path.relative(tempDirectory, outputPath), PATH_LENGTH);
 
     const inputBuffer = Buffer.from(input);
     assert.isTrue(isEqual(inputBuffer, output));
   });
 
   describe('_getAttachmentName', () => {
-    it('should return correct name', () => {
-      const input = Buffer.from('test string', 'utf8');
-      const expected = 'd5579c46dfcc7f18207013e65b44e4cb4e2c2298f4ac457ba8f82743f31e930b';
-      assert.strictEqual(_getAttachmentName(input), expected);
+    it('should return random file name with correct length', () => {
+      assert.lengthOf(_getAttachmentName(), NAME_LENGTH);
     });
   });
 
   describe('_getAttachmentPath', () => {
     it('should return correct path', () => {
-      const input = Buffer.from('test string', 'utf8');
-      const expected = 'd55/d5579c46dfcc7f18207013e65b44e4cb4e2c2298f4ac457ba8f82743f31e930b';
-      assert.strictEqual(_getAttachmentPath(input), expected);
+      assert.lengthOf(_getAttachmentPath(), PATH_LENGTH);
     });
   });
 });