From af7a262ef0066b8f0808d7a0da88cd495420e25c Mon Sep 17 00:00:00 2001
From: Danny Coates <dannycoates@gmail.com>
Date: Thu, 31 May 2018 14:06:25 -0700
Subject: [PATCH] refactored upload away from multipart forms to binary data

---
 app/api.js               |  7 ++----
 package-lock.json        | 41 ++++++----------------------------
 package.json             |  1 -
 server/limiter.js        | 20 +++++++++++++++++
 server/routes/index.js   |  8 +------
 server/routes/upload.js  | 48 +++++++++++++++++-----------------------
 server/storage/fs.js     |  5 ++---
 server/storage/s3.js     | 17 +++-----------
 test/backend/s3-tests.js |  2 +-
 9 files changed, 56 insertions(+), 93 deletions(-)
 create mode 100644 server/limiter.js

diff --git a/app/api.js b/app/api.js
index e314657d..42d7d512 100644
--- a/app/api.js
+++ b/app/api.js
@@ -121,10 +121,7 @@ export function uploadFile(
       });
     })
   };
-  const dataView = new DataView(encrypted);
-  const blob = new Blob([dataView], { type: 'application/octet-stream' });
-  const fd = new FormData();
-  fd.append('data', blob);
+  const blob = new Blob([encrypted], { type: 'application/octet-stream' });
   xhr.upload.addEventListener('progress', function(event) {
     if (event.lengthComputable) {
       onprogress([event.loaded, event.total]);
@@ -133,7 +130,7 @@ export function uploadFile(
   xhr.open('post', '/api/upload', true);
   xhr.setRequestHeader('X-File-Metadata', arrayToB64(new Uint8Array(metadata)));
   xhr.setRequestHeader('Authorization', `send-v1 ${verifierB64}`);
-  xhr.send(fd);
+  xhr.send(blob);
   return upload;
 }
 
diff --git a/package-lock.json b/package-lock.json
index 1a4e734f..509909c1 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -1742,15 +1742,6 @@
       "integrity": "sha1-hZgoeOIbmOHGZCXgPQF0eI9Wnug=",
       "dev": true
     },
-    "busboy": {
-      "version": "0.2.14",
-      "resolved": "https://registry.npmjs.org/busboy/-/busboy-0.2.14.tgz",
-      "integrity": "sha1-bCpiLvz0fFe7vh4qnDetNseSVFM=",
-      "requires": {
-        "dicer": "0.2.5",
-        "readable-stream": "1.1.14"
-      }
-    },
     "bytes": {
       "version": "3.0.0",
       "resolved": "https://registry.npmjs.org/bytes/-/bytes-3.0.0.tgz",
@@ -2493,14 +2484,6 @@
         }
       }
     },
-    "connect-busboy": {
-      "version": "0.0.2",
-      "resolved": "https://registry.npmjs.org/connect-busboy/-/connect-busboy-0.0.2.tgz",
-      "integrity": "sha1-rFyclmchcYheV2xmsr/ZXTuxEJc=",
-      "requires": {
-        "busboy": "0.2.14"
-      }
-    },
     "connect-history-api-fallback": {
       "version": "1.5.0",
       "resolved": "https://registry.npmjs.org/connect-history-api-fallback/-/connect-history-api-fallback-1.5.0.tgz",
@@ -2630,7 +2613,8 @@
     "core-util-is": {
       "version": "1.0.2",
       "resolved": "https://registry.npmjs.org/core-util-is/-/core-util-is-1.0.2.tgz",
-      "integrity": "sha1-tf1UIgqivFq1eqtxQMlAdUUDwac="
+      "integrity": "sha1-tf1UIgqivFq1eqtxQMlAdUUDwac=",
+      "dev": true
     },
     "cosmiconfig": {
       "version": "4.0.0",
@@ -3346,15 +3330,6 @@
       "integrity": "sha1-ogM8CcyOFY03dI+951B4Mr1s4Sc=",
       "dev": true
     },
-    "dicer": {
-      "version": "0.2.5",
-      "resolved": "https://registry.npmjs.org/dicer/-/dicer-0.2.5.tgz",
-      "integrity": "sha1-WZbAhrszIYyBLAkL3cCc0S+stw8=",
-      "requires": {
-        "readable-stream": "1.1.14",
-        "streamsearch": "0.1.2"
-      }
-    },
     "diff": {
       "version": "3.3.1",
       "resolved": "https://registry.npmjs.org/diff/-/diff-3.3.1.tgz",
@@ -16177,6 +16152,7 @@
       "version": "1.1.14",
       "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-1.1.14.tgz",
       "integrity": "sha1-fPTFTvZI44EwhMY23SB54WbAgdk=",
+      "dev": true,
       "requires": {
         "core-util-is": "1.0.2",
         "inherits": "2.0.3",
@@ -16187,7 +16163,8 @@
         "isarray": {
           "version": "0.0.1",
           "resolved": "https://registry.npmjs.org/isarray/-/isarray-0.0.1.tgz",
-          "integrity": "sha1-ihis/Kmo9Bd+Cav8YDiTmwXR7t8="
+          "integrity": "sha1-ihis/Kmo9Bd+Cav8YDiTmwXR7t8=",
+          "dev": true
         }
       }
     },
@@ -17690,11 +17667,6 @@
         "any-observable": "0.2.0"
       }
     },
-    "streamsearch": {
-      "version": "0.1.2",
-      "resolved": "https://registry.npmjs.org/streamsearch/-/streamsearch-0.1.2.tgz",
-      "integrity": "sha1-gIudDlb8Jz2Am6VzOOkpkZoanxo="
-    },
     "strftime": {
       "version": "0.10.0",
       "resolved": "https://registry.npmjs.org/strftime/-/strftime-0.10.0.tgz",
@@ -17753,7 +17725,8 @@
     "string_decoder": {
       "version": "0.10.31",
       "resolved": "https://registry.npmjs.org/string_decoder/-/string_decoder-0.10.31.tgz",
-      "integrity": "sha1-YuIDvEF2bGwoyfyEMB2rHFMQ+pQ="
+      "integrity": "sha1-YuIDvEF2bGwoyfyEMB2rHFMQ+pQ=",
+      "dev": true
     },
     "stringify-entities": {
       "version": "1.3.1",
diff --git a/package.json b/package.json
index 11e39818..90dca69e 100644
--- a/package.json
+++ b/package.json
@@ -120,7 +120,6 @@
     "babel-polyfill": "^6.26.0",
     "choo": "^6.10.0",
     "cldr-core": "^32.0.0",
-    "connect-busboy": "0.0.2",
     "convict": "^4.0.1",
     "express": "^4.16.2",
     "fluent": "^0.6.3",
diff --git a/server/limiter.js b/server/limiter.js
new file mode 100644
index 00000000..a2d5cb37
--- /dev/null
+++ b/server/limiter.js
@@ -0,0 +1,20 @@
+const { Transform } = require('stream');
+
+class Limiter extends Transform {
+  constructor(limit) {
+    super();
+    this.limit = limit;
+    this.length = 0;
+  }
+
+  _transform(chunk, encoding, callback) {
+    this.length += chunk.length;
+    this.push(chunk);
+    if (this.length > this.limit) {
+      return callback(new Error('limit'));
+    }
+    callback();
+  }
+}
+
+module.exports = Limiter;
diff --git a/server/routes/index.js b/server/routes/index.js
index aabbd2fd..4b7d6f31 100644
--- a/server/routes/index.js
+++ b/server/routes/index.js
@@ -1,5 +1,4 @@
 const express = require('express');
-const busboy = require('connect-busboy');
 const helmet = require('helmet');
 const storage = require('../storage');
 const config = require('../config');
@@ -10,11 +9,6 @@ const pages = require('./pages');
 
 const IS_DEV = config.env === 'development';
 const ID_REGEX = '([0-9a-fA-F]{10})';
-const uploader = busboy({
-  limits: {
-    fileSize: config.max_file_size
-  }
-});
 
 module.exports = function(app) {
   app.use(helmet());
@@ -62,7 +56,7 @@ module.exports = function(app) {
   app.get(`/api/download/:id${ID_REGEX}`, auth, require('./download'));
   app.get(`/api/exists/:id${ID_REGEX}`, require('./exists'));
   app.get(`/api/metadata/:id${ID_REGEX}`, auth, require('./metadata'));
-  app.post('/api/upload', uploader, require('./upload'));
+  app.post('/api/upload', require('./upload'));
   app.post(`/api/delete/:id${ID_REGEX}`, owner, require('./delete'));
   app.post(`/api/password/:id${ID_REGEX}`, owner, require('./password'));
   app.post(`/api/params/:id${ID_REGEX}`, owner, require('./params'));
diff --git a/server/routes/upload.js b/server/routes/upload.js
index e566bdc8..2b2bd617 100644
--- a/server/routes/upload.js
+++ b/server/routes/upload.js
@@ -2,10 +2,11 @@ const crypto = require('crypto');
 const storage = require('../storage');
 const config = require('../config');
 const mozlog = require('../log');
+const Limiter = require('../limiter');
 
 const log = mozlog('send.upload');
 
-module.exports = function(req, res) {
+module.exports = async function(req, res) {
   const newId = crypto.randomBytes(5).toString('hex');
   const metadata = req.header('X-File-Metadata');
   const auth = req.header('Authorization');
@@ -19,33 +20,24 @@ module.exports = function(req, res) {
     auth: auth.split(' ')[1],
     nonce: crypto.randomBytes(16).toString('base64')
   };
-  req.pipe(req.busboy);
 
-  req.busboy.on('file', async (fieldname, file) => {
-    try {
-      await storage.set(newId, file, meta);
-      const protocol = config.env === 'production' ? 'https' : req.protocol;
-      const url = `${protocol}://${req.get('host')}/download/${newId}/`;
-      res.set('WWW-Authenticate', `send-v1 ${meta.nonce}`);
-      res.json({
-        url,
-        owner: meta.owner,
-        id: newId
-      });
-    } catch (e) {
-      log.error('upload', e);
-      if (e.message === 'limit') {
-        return res.sendStatus(413);
-      }
-      res.sendStatus(500);
+  try {
+    const limiter = new Limiter(config.max_file_size);
+    const fileStream = req.pipe(limiter);
+    await storage.set(newId, fileStream, meta);
+    const protocol = config.env === 'production' ? 'https' : req.protocol;
+    const url = `${protocol}://${req.get('host')}/download/${newId}/`;
+    res.set('WWW-Authenticate', `send-v1 ${meta.nonce}`);
+    res.json({
+      url,
+      owner: meta.owner,
+      id: newId
+    });
+  } catch (e) {
+    if (e.message === 'limit') {
+      return res.sendStatus(413);
     }
-  });
-
-  req.on('close', async err => {
-    try {
-      await storage.del(newId);
-    } catch (e) {
-      log.info('DeleteError:', newId);
-    }
-  });
+    log.error('upload', e);
+    res.sendStatus(500);
+  }
 };
diff --git a/server/storage/fs.js b/server/storage/fs.js
index 54fcd53a..aa6da744 100644
--- a/server/storage/fs.js
+++ b/server/storage/fs.js
@@ -26,9 +26,8 @@ class FSStorage {
       const filepath = path.join(this.dir, id);
       const fstream = fs.createWriteStream(filepath);
       file.pipe(fstream);
-      file.on('limit', () => {
-        file.unpipe(fstream);
-        fstream.destroy(new Error('limit'));
+      file.on('error', err => {
+        fstream.destroy(err);
       });
       fstream.on('error', err => {
         fs.unlinkSync(filepath);
diff --git a/server/storage/s3.js b/server/storage/s3.js
index b77f91b3..bb2b0100 100644
--- a/server/storage/s3.js
+++ b/server/storage/s3.js
@@ -18,25 +18,14 @@ class S3Storage {
     return s3.getObject({ Bucket: this.bucket, Key: id }).createReadStream();
   }
 
-  async set(id, file) {
-    let hitLimit = false;
+  set(id, file) {
     const upload = s3.upload({
       Bucket: this.bucket,
       Key: id,
       Body: file
     });
-    file.on('limit', () => {
-      hitLimit = true;
-      upload.abort();
-    });
-    try {
-      await upload.promise();
-    } catch (e) {
-      if (hitLimit) {
-        throw new Error('limit');
-      }
-      throw e;
-    }
+    file.on('error', () => upload.abort());
+    return upload.promise();
   }
 
   del(id) {
diff --git a/test/backend/s3-tests.js b/test/backend/s3-tests.js
index 68e55f36..997b7c34 100644
--- a/test/backend/s3-tests.js
+++ b/test/backend/s3-tests.js
@@ -98,7 +98,7 @@ describe('S3Storage', function() {
         on: (ev, fn) => fn()
       };
       const abort = sinon.stub();
-      const err = new Error();
+      const err = new Error('limit');
       s3Stub.upload = sinon.stub().returns({
         promise: () => Promise.reject(err),
         abort