From af7a262ef0066b8f0808d7a0da88cd495420e25c Mon Sep 17 00:00:00 2001 From: Danny Coates 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