From 99970410933700062743a172b7616310db5e21bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Thu, 10 Nov 2016 14:20:38 +0100 Subject: [PATCH 1/2] Clean up "download()" implementation Reduce nesting, remove repetition. --- lib/storage-handler.js | 94 +++++++++++++++++++++--------------------- 1 file changed, 47 insertions(+), 47 deletions(-) diff --git a/lib/storage-handler.js b/lib/storage-handler.js index 4063bf6..96080ea 100644 --- a/lib/storage-handler.js +++ b/lib/storage-handler.js @@ -233,57 +233,57 @@ exports.download = function(provider, req, res, container, file, cb) { var range = null; - if (req) { - if (req.headers) { - range = req.headers.range || ''; + if (!req) { + // TODO(rfeng/bajtos) We should let the caller now about the problem! + return; + } + + if (req.headers) { + range = req.headers.range || ''; + } + + if (!range) { + return download(params); + } + + provider.getFile(params.container, params.remote, function(err, stats) { + if (err) { + return handleError(res, err); } - if (range) { - provider.getFile(params.container, params.remote, function(err, stats) { - if (err) { - handleError(res, err); - } else { - var total = stats.size; + setupPartialDownload(params, stats, res); + download(params); + }); - var parts = range.replace(/bytes=/, '').split('-'); - var partialstart = parts[0]; - var partialend = parts[1]; + function download(params) { + var reader = provider.download(params); - params.start = parseInt(partialstart, 10); - params.end = partialend ? parseInt(partialend, 10) : total - 1; + res.type(fileName); - var chunksize = (params.end - params.start) + 1; - - res.status(206); - res.set('Content-Range', 'bytes ' + params.start + '-' + params.end + '/' + total); - res.set('Accept-Ranges', 'bytes'); - res.set('Content-Length', chunksize); - - var reader = provider.download(params); - - res.type(fileName); - - reader.pipe(res); - reader.on('error', function(err) { - handleError(res, err); - }); - reader.on('end', function() { - cb(); - }); - } - }); - } else { - var reader = provider.download(params); - - res.type(fileName); - - reader.pipe(res); - reader.on('error', function(err) { - handleError(res, err); - }); - reader.on('end', function() { - cb(); - }); - } + reader.pipe(res); + reader.on('error', function onReaderError(err) { + handleError(res, err); + }); + reader.on('end', function onReaderEnd() { + cb(); + }); } }; + +function setupPartialDownload(params, stats, res) { + var total = stats.size; + + var parts = range.replace(/bytes=/, '').split('-'); + var partialstart = parts[0]; + var partialend = parts[1]; + + params.start = parseInt(partialstart, 10); + params.end = partialend ? parseInt(partialend, 10) : total - 1; + + var chunksize = (params.end - params.start) + 1; + + res.status(206); + res.set('Content-Range', 'bytes ' + params.start + '-' + params.end + '/' + total); + res.set('Accept-Ranges', 'bytes'); + res.set('Content-Length', chunksize); +}; From 6cb5a294c501b802904c85009e6a80593d1d2e69 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Thu, 10 Nov 2016 14:31:45 +0100 Subject: [PATCH 2/2] Continue middleware chain after download error Stop sending constructing the error response directly inside the storage component and send the error back to the application to handle it. As a result: - `File.afterRemoteError` hook is trigered now - App-provided error handling strategy is used to build error responses, e.g. using the production mode of strong-error-handler to hide sensitive information. --- lib/storage-handler.js | 28 ++++++++++++++++------------ test/upload-download.test.js | 18 ++++++++++++++++++ 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/lib/storage-handler.js b/lib/storage-handler.js index 96080ea..52733a2 100644 --- a/lib/storage-handler.js +++ b/lib/storage-handler.js @@ -204,16 +204,6 @@ exports.upload = function(provider, req, res, options, cb) { }); }; -function handleError(res, err) { - if (err.code === 'ENOENT') { - res.type('application/json'); - res.status(404).send({error: err}); - return; - } - res.type('application/json'); - res.status(500).send({error: err}); -} - /** * Handle download from a container/file. * @param {Object} provider The storage service provider @@ -248,7 +238,7 @@ exports.download = function(provider, req, res, container, file, cb) { provider.getFile(params.container, params.remote, function(err, stats) { if (err) { - return handleError(res, err); + return cb(processError(err, params.remote)); } setupPartialDownload(params, stats, res); @@ -261,11 +251,15 @@ exports.download = function(provider, req, res, container, file, cb) { res.type(fileName); reader.pipe(res); + reader.on('error', function onReaderError(err) { - handleError(res, err); + cb(processError(err, params.remote)); + cb = function() {}; // avoid double-callback }); + reader.on('end', function onReaderEnd() { cb(); + cb = function() {}; // avoid double-callback }); } }; @@ -287,3 +281,13 @@ function setupPartialDownload(params, stats, res) { res.set('Accept-Ranges', 'bytes'); res.set('Content-Length', chunksize); }; + +function processError(err, fileName) { + if (err.code === 'ENOENT') { + err.statusCode = err.status = 404; + // Hide the original message reported e.g. by FS provider, as it may + // contain sensitive information. + err.message = 'File not found: ' + fileName; + } + return err; +} diff --git a/test/upload-download.test.js b/test/upload-download.test.js index a7d3728..7bc16e6 100644 --- a/test/upload-download.test.js +++ b/test/upload-download.test.js @@ -314,6 +314,24 @@ describe('storage service', function() { }); }); + it('should run a function after a download failed', function(done) { + var hookCalled = false; + var Container = app.models.Container; + + Container.afterRemoteError('download', function(ctx, cb) { + hookCalled = true; + cb(); + }); + + request('http://localhost:' + app.get('port')) + .get('/containers/album1/download/does-not-exist') + .expect(404, function(err, res) { + if (err) return done(err); + assert(hookCalled, 'afterRemoteEror hook was not called'); + done(); + }); + }); + it('should delete a file', function(done) { request('http://localhost:' + app.get('port')) .del('/containers/album1/files/test.jpg')