Merge pull request #837 from mozilla/refactor-upload

refactored upload away from multipart forms to binary data

app/api.js

@@ -121,10 +121,7 @@ export function uploadFile(
       });
     })
   };
-  const dataView = new DataView(encrypted);
+  const blob = new Blob([encrypted], { type: 'application/octet-stream' });
-  const blob = new Blob([dataView], { type: 'application/octet-stream' });
-  const fd = new FormData();
-  fd.append('data', blob);
   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;
 }
 

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",

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",

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;

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'));

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 {
-    try {
+    const limiter = new Limiter(config.max_file_size);
-      await storage.set(newId, file, meta);
+    const fileStream = req.pipe(limiter);
-      const protocol = config.env === 'production' ? 'https' : req.protocol;
+    await storage.set(newId, fileStream, meta);
-      const url = `${protocol}://${req.get('host')}/download/${newId}/`;
+    const protocol = config.env === 'production' ? 'https' : req.protocol;
-      res.set('WWW-Authenticate', `send-v1 ${meta.nonce}`);
+    const url = `${protocol}://${req.get('host')}/download/${newId}/`;
-      res.json({
+    res.set('WWW-Authenticate', `send-v1 ${meta.nonce}`);
-        url,
+    res.json({
-        owner: meta.owner,
+      url,
-        id: newId
+      owner: meta.owner,
-      });
+      id: newId
-    } catch (e) {
+    });
-      log.error('upload', e);
+  } catch (e) {
-      if (e.message === 'limit') {
+    if (e.message === 'limit') {
-        return res.sendStatus(413);
+      return res.sendStatus(413);
-      }
-      res.sendStatus(500);
     }
-  });
+    log.error('upload', e);
-
+    res.sendStatus(500);
-  req.on('close', async err => {
+  }
-    try {
-      await storage.del(newId);
-    } catch (e) {
-      log.info('DeleteError:', newId);
-    }
-  });
 };

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.on('error', err => {
-        file.unpipe(fstream);
+        fstream.destroy(err);
-        fstream.destroy(new Error('limit'));
       });
       fstream.on('error', err => {
         fs.unlinkSync(filepath);

server/storage/s3.js

@@ -18,25 +18,14 @@ class S3Storage {
     return s3.getObject({ Bucket: this.bucket, Key: id }).createReadStream();
   }
 
-  async set(id, file) {
+  set(id, file) {
-    let hitLimit = false;
     const upload = s3.upload({
       Bucket: this.bucket,
       Key: id,
       Body: file
     });
-    file.on('limit', () => {
+    file.on('error', () => upload.abort());
-      hitLimit = true;
+    return upload.promise();
-      upload.abort();
-    });
-    try {
-      await upload.promise();
-    } catch (e) {
-      if (hitLimit) {
-        throw new Error('limit');
-      }
-      throw e;
-    }
   }
 
   del(id) {

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