Merge pull request #186 from jeffora/fix-error-handling

Fix #185: Validation failures crash server

lib/providers/filesystem/index.js

@@ -13,6 +13,7 @@ var g = require('strong-globalize')();
 
 var fs = require('fs'),
   path = require('path'),
+  stream = require('stream'),
   async = require('async'),
   File = require('./file').File,
   Container = require('./container').Container;
@@ -58,12 +59,23 @@ function validateName(name, cb) {
     cb && process.nextTick(cb.bind(null,
       new Error(g.f('{{FileSystemProvider}}: Invalid name: %s', name))));
     if (!cb) {
-      console.error(g.f('{{FileSystemProvider}}: Invalid name: ', name));
+      console.error(g.f('{{FileSystemProvider}}: Invalid name: %s', name));
     }
     return false;
   }
 }
 
+function streamError(errStream, err, cb) {
+  process.nextTick(function() {
+    errStream.emit('error', err);
+    cb && cb(null, err);
+  });
+  return errStream;
+}
+
+var writeStreamError = streamError.bind(null, new stream.Writable());
+var readStreamError = streamError.bind(null, new stream.Readable());
+
 /*!
  * Populate the metadata from file stat into props
  * @param {fs.Stats} stat The file stat instance
@@ -168,9 +180,19 @@ FileSystemProvider.prototype.getContainer = function(containerName, cb) {
 // File related functions
 FileSystemProvider.prototype.upload = function(options, cb) {
   var container = options.container;
-  if (!validateName(container, cb)) return;
+  if (!validateName(container)) {
+    return writeStreamError(
+      new Error(g.f('{{FileSystemProvider}}: Invalid name: %s', container)),
+      cb
+    );
+  }
   var file = options.remote;
-  if (!validateName(file, cb)) return;
+  if (!validateName(file)) {
+    return writeStreamError(
+      new Error(g.f('{{FileSystemProvider}}: Invalid name: %s', file)),
+      cb
+    );
+  }
   var filePath = path.join(this.root, container, file);
 
   var fileOpts = {flags: options.flags || 'w+',
@@ -188,15 +210,25 @@ FileSystemProvider.prototype.upload = function(options, cb) {
     });
     return stream;
   } catch (e) {
-    cb && cb(e);
+    return writeStreamError(e, cb);
   }
 };
 
 FileSystemProvider.prototype.download = function(options, cb) {
   var container = options.container;
-  if (!validateName(container, cb)) return;
+  if (!validateName(container, cb)) {
+    return readStreamError(
+      new Error(g.f('{{FileSystemProvider}}: Invalid name: %s', container)),
+      cb
+    );
+  }
   var file = options.remote;
-  if (!validateName(file, cb)) return;
+  if (!validateName(file, cb)) {
+    return readStreamError(
+      new Error(g.f('{{FileSystemProvider}}: Invalid name: %s', file)),
+      cb
+    );
+  }
 
   var filePath = path.join(this.root, container, file);
 
@@ -211,7 +243,7 @@ FileSystemProvider.prototype.download = function(options, cb) {
   try {
     return fs.createReadStream(filePath, fileOpts);
   } catch (e) {
-    cb && cb(e);
+    return readStreamError(e, cb);
   }
 };
 

test/files/.gitignore

@@ -1 +1,2 @@
+a-f1_downloaded.txt
 f1_downloaded.txt

test/fs.test.js

@@ -110,6 +110,21 @@ describe('FileSystem based storage provider', function() {
       writer.on('error', done);
     });
 
+    it('should fail to upload a file with invalid characters', function(done) {
+      var writer = client.upload({container: 'c1', remote: 'a/f1.txt'});
+      fs.createReadStream(path.join(__dirname, 'files/f1.txt')).pipe(writer);
+      var cb = done;
+      var clearCb = function() {};
+      writer.on('error', function() {
+        cb();
+        cb = clearCb;
+      });
+      writer.on('finish', function() {
+        cb(new Error('Should have finished with error callback'));
+        cb = clearCb;
+      });
+    });
+
     it('should download a file', function(done) {
       var reader = client.download({
         container: 'c1',
@@ -120,6 +135,21 @@ describe('FileSystem based storage provider', function() {
       reader.on('error', done);
     });
 
+    it('should fail to download a file with invalid characters', function(done) {
+      var reader = client.download({container: 'c1', remote: 'a/f1.txt'});
+      reader.pipe(fs.createWriteStream(path.join(__dirname, 'files/a-f1_downloaded.txt')));
+      var cb = done;
+      var clearCb = function() {};
+      reader.on('error', function() {
+        cb();
+        cb = clearCb;
+      });
+      reader.on('end', function() {
+        cb(new Error('Expected error: Invalid name'));
+        cb = clearCb;
+      });
+    });
+
     it('should get files for a container', function(done) {
       client.getFiles('c1', function(err, files) {
         assert(!err);
@@ -161,4 +191,3 @@ describe('FileSystem based storage provider', function() {
     });
   });
 });
-