Merge pull request #47 from kanongil/confine-file-path

Add "confine" option that confines file responses
hapijs · May 9, 2016 · d53a0ec · d53a0ec
2 parents fef2232 + 7251728
commit d53a0ec
Show file tree

Hide file tree

Showing 6 changed files with 140 additions and 44 deletions.
diff --git a/README.md b/README.md
@@ -124,10 +124,6 @@ server.ext('onPostHandler', function (request, reply) {
 });
 ```
 
-Note that paths for files served using the `reply.file()` handler are **NOT** guarded against
-access outside the `files.relativeTo` directory, so be careful to guard against malevolent user
-input.
-
 ## Usage
 
 After registration, this plugin adds a new method to the `reply` object and exposes the `'file'`
@@ -150,6 +146,10 @@ type based on filename extension.:
 
   - `path` - the file path.
   - `options` - optional settings:
+      - `confine` - serve file relative to this directory and returns `403 Forbidden` if the
+        `path` resolves outside the `confine` directory.
+        Defaults to `true` which uses the `relativeTo` route option as the `confine`.
+        Set to `false` to disable this security feature.
       - `filename` - an optional filename to specify if sending a 'Content-Disposition' header,
         defaults to the basename of `path`
       - `mode` - specifies whether to include the 'Content-Disposition' header with the response.
@@ -182,6 +182,10 @@ Generates a static file endpoint for serving a single file. `file` can be set to
     file path.
   - an object with one or more of the following options:
       - `path` - a path string or function as described above (required).
+      - `confine` - serve file relative to this directory and returns `403 Forbidden` if the
+        `path` resolves outside the `confine` directory.
+        Defaults to `true` which uses the `relativeTo` route option as the `confine`.
+        Set to `false` to disable this security feature.
       - `filename` - an optional filename to specify if sending a 'Content-Disposition'
         header, defaults to the basename of `path`
       - `mode` - specifies whether to include the 'Content-Disposition' header with the

diff --git a/lib/directory.js b/lib/directory.js
@@ -77,16 +77,7 @@ exports.handler = function (route, options) {
 
         // Append parameter
 
-        let selection = null;
-        const lastParam = request.paramsArray[request.paramsArray.length - 1];
-        if (lastParam) {
-            if (lastParam.indexOf('..') !== -1) {
-                return reply(Boom.forbidden());
-            }
-
-            selection = lastParam;
-        }
-
+        const selection = request.paramsArray[request.paramsArray.length - 1];
         if (selection &&
             !settings.showHidden &&
             internals.isFileHidden(selection)) {
@@ -98,11 +89,17 @@ exports.handler = function (route, options) {
 
         const resource = request.path;
         const hasTrailingSlash = resource.endsWith('/');
-        const fileOptions = { lookupCompressed: settings.lookupCompressed, etagMethod: settings.etagMethod };
+        const fileOptions = {
+            confine: null,
+            lookupCompressed: settings.lookupCompressed,
+            etagMethod: settings.etagMethod
+        };
+
+        Items.serial(paths, (baseDir, nextPath) => {
 
-        Items.serial(paths, (path, nextPath) => {
+            fileOptions.confine = baseDir;
 
-            path = Path.join(path, selection || '');
+            let path = selection || '';
 
             File.load(path, request, fileOptions, (response) => {
 
@@ -186,7 +183,7 @@ exports.handler = function (route, options) {
                         return reply(Boom.forbidden());
                     }
 
-                    return internals.generateListing(path, resource, selection, hasTrailingSlash, settings, request, reply);
+                    return internals.generateListing(Path.join(baseDir, path), resource, selection, hasTrailingSlash, settings, request, reply);
                 });
             });
         },
@@ -234,7 +231,7 @@ internals.generateListing = function (path, resource, selection, hasTrailingSlas
 
 internals.isFileHidden = function (path) {
 
-    return /(^|[\\\/])\.([^\\\/]|[\\\/]?$)/.test(path);           // Starts with a '.' or contains '/.' or '\.', and not followed by a '/' or '\' or end
+    return /(^|[\\\/])\.([^.\\\/]|\.[^\\\/])/.test(path);           // Starts with a '.' or contains '/.' or '\.', which is not followed by a '/' or '\' or '.'
 };
 
 

diff --git a/lib/file.js b/lib/file.js
@@ -21,6 +21,7 @@ internals.schema = Joi.alternatives([
     Joi.func(),
     Joi.object({
         path: Joi.alternatives(Joi.string(), Joi.func()).required(),
+        confine: Joi.alternatives(Joi.string(), Joi.boolean()).default(true),
         filename: Joi.string(),
         mode: Joi.string().valid('attachment', 'inline').allow(false),
         lookupCompressed: Joi.boolean(),
@@ -33,7 +34,8 @@ internals.schema = Joi.alternatives([
 exports.handler = function (route, options) {
 
     let settings = Joi.attempt(options, internals.schema, 'Invalid file handler options (' + route.path + ')');
-    settings = (typeof options !== 'object' ? { path: options } : settings);
+    settings = (typeof options !== 'object' ? { path: options, confine: '.' } : settings);
+    settings.confine = settings.confine === true ? '.' : settings.confine;
     Hoek.assert(typeof settings.path !== 'string' || settings.path[settings.path.length - 1] !== '/', 'File path cannot end with a \'/\':', route.path);
 
     const handler = (request, reply) => {
@@ -55,11 +57,23 @@ exports.load = function (path, request, options, callback) {
 
 exports.response = function (path, options, request, _preloaded) {
 
-    options = options || {};
     Hoek.assert(!options.mode || ['attachment', 'inline'].indexOf(options.mode) !== -1, 'options.mode must be either false, attachment, or inline');
 
+    if (options.confine) {
+        const confineDir = Path.resolve(request.route.settings.files.relativeTo, options.confine);
+        path = Path.isAbsolute(path) ? Path.normalize(path) : Path.join(confineDir, path);
+
+        // Verify that resolved path is within confineDir
+        if (path.lastIndexOf(confineDir, 0) !== 0) {
+            path = null;
+        }
+    }
+    else {
+        path = Path.isAbsolute(path) ? Path.normalize(path) : Path.join(request.route.settings.files.relativeTo, path);
+    }
+
     const source = {
-        path: Path.normalize(Path.isAbsolute(path) ? path : Path.join(request.route.settings.files.relativeTo, path)),
+        path: path,
         settings: options,
         stat: null,
         fd: null
@@ -74,6 +88,14 @@ exports.response = function (path, options, request, _preloaded) {
 internals.prepare = function (response, callback) {
 
     const path = response.source.path;
+
+    if (path === null) {
+        return process.nextTick(() => {
+
+            return callback(Boom.forbidden(null, 'EACCES'));
+        });
+    }
+
     internals.openStat(path, 'r', (err, fd, stat) => {
 
         if (err) {

diff --git a/lib/index.js b/lib/index.js
@@ -29,6 +29,13 @@ exports.register = function (server, options, next) {
 
     server.decorate('reply', 'file', function (path, responseOptions) {
 
+        // Set correct confine value
+
+        responseOptions = responseOptions || {};
+        if (typeof responseOptions.confine === 'undefined' || responseOptions.confine === true) {
+            responseOptions.confine = '.';
+        }
+
         return this.response(File.response(path, responseOptions, this.request));
     });
 

diff --git a/test/file.js b/test/file.js
@@ -45,7 +45,7 @@ describe('file', () => {
             const server = provisionServer({ routes: { files: { relativeTo: __dirname } } });
             const handler = (request, reply) => {
 
-                reply.file('../package.json').code(499);
+                reply.file('package.json', { confine: '../' }).code(499);
             };
 
             server.route({ method: 'GET', path: '/file', handler: handler });
@@ -66,7 +66,7 @@ describe('file', () => {
             const server = provisionServer();
             const handler = (request, reply) => {
 
-                reply.file('../package.json');
+                reply.file('../package.json', { confine: false });
             };
 
             server.route({ method: 'GET', path: '/file', handler: handler, config: { files: { relativeTo: __dirname } } });
@@ -180,7 +180,7 @@ describe('file', () => {
             const server = provisionServer({ routes: { files: { relativeTo: __dirname } } });
             const handler = (request, reply) => {
 
-                reply.file(Path.join(__dirname, '..', 'package.json'), { mode: 'attachment' });
+                reply.file(Path.join(__dirname, '..', 'package.json'), { confine: '..', mode: 'attachment' });
             };
 
             server.route({ method: 'GET', path: '/file', handler: handler });
@@ -201,7 +201,7 @@ describe('file', () => {
             const server = provisionServer({ routes: { files: { relativeTo: __dirname } } });
             const handler = (request, reply) => {
 
-                reply.file(Path.join(__dirname, '..', 'package.json'), { mode: 'attachment', filename: 'attachment.json' });
+                reply.file(Path.join(__dirname, '..', 'package.json'), { confine: '..', mode: 'attachment', filename: 'attachment.json' });
             };
 
             server.route({ method: 'GET', path: '/file', handler: handler });
@@ -222,7 +222,7 @@ describe('file', () => {
             const server = provisionServer({ routes: { files: { relativeTo: __dirname } } });
             const handler = (request, reply) => {
 
-                reply.file(Path.join(__dirname, '..', 'package.json'), { mode: 'inline' });
+                reply.file(Path.join(__dirname, '..', 'package.json'), { confine: '..', mode: 'inline' });
             };
 
             server.route({ method: 'GET', path: '/file', handler: handler });
@@ -243,7 +243,7 @@ describe('file', () => {
             const server = provisionServer({ routes: { files: { relativeTo: __dirname } } });
             const handler = (request, reply) => {
 
-                reply.file(Path.join(__dirname, '..', 'package.json'), { mode: 'inline', filename: 'attachment.json' });
+                reply.file(Path.join(__dirname, '..', 'package.json'), { confine: '..', mode: 'inline', filename: 'attachment.json' });
             };
 
             server.route({ method: 'GET', path: '/file', handler: handler });
@@ -287,7 +287,7 @@ describe('file', () => {
 
         it('returns a file using the built-in handler config', (done) => {
 
-            const server = provisionServer({ routes: { files: { relativeTo: __dirname } } });
+            const server = provisionServer({ routes: { files: { relativeTo: Path.join(__dirname, '..') } } });
             server.route({ method: 'GET', path: '/staticfile', handler: { file: Path.join(__dirname, '..', 'package.json') } });
 
             server.inject('/staticfile', (res) => {
@@ -304,10 +304,10 @@ describe('file', () => {
 
             const filenameFn = (request) => {
 
-                return '../lib/' + request.params.file;
+                return './lib/' + request.params.file;
             };
 
-            const server = provisionServer({ routes: { files: { relativeTo: __dirname } } });
+            const server = provisionServer({ routes: { files: { relativeTo: Path.join(__dirname, '..') } } });
             server.route({ method: 'GET', path: '/filefn/{file}', handler: { file: filenameFn } });
 
             server.inject('/filefn/index.js', (res) => {
@@ -325,7 +325,7 @@ describe('file', () => {
             const server = provisionServer({ routes: { files: { relativeTo: '.' } } });
             const relativeHandler = (request, reply) => {
 
-                reply.file('./package.json');
+                reply.file('./package.json', { confine: true });
             };
 
             server.route({ method: 'GET', path: '/relativefile', handler: relativeHandler });
@@ -342,8 +342,8 @@ describe('file', () => {
 
         it('returns a file using the built-in handler config (relative path)', (done) => {
 
-            const server = provisionServer({ routes: { files: { relativeTo: __dirname } } });
-            server.route({ method: 'GET', path: '/relativestaticfile', handler: { file: '../package.json' } });
+            const server = provisionServer({ routes: { files: { relativeTo: Path.join(__dirname, '..') } } });
+            server.route({ method: 'GET', path: '/relativestaticfile', handler: { file: './package.json' } });
 
             server.inject('/relativestaticfile', (res) => {
 
@@ -373,7 +373,7 @@ describe('file', () => {
             const server = provisionServer({ routes: { files: { relativeTo: __dirname } } });
             const handler = (request, reply) => {
 
-                reply.file('../LICENSE').type('application/example');
+                reply.file('../LICENSE', { confine: false }).type('application/example');
             };
 
             server.route({ method: 'GET', path: '/file', handler: handler });
@@ -870,7 +870,7 @@ describe('file', () => {
             const server = provisionServer({ routes: { files: { relativeTo: __dirname } } });
             const handler = (request, reply) => {
 
-                reply.file(Path.join(__dirname, '..', 'package.json'));
+                reply.file(Path.join(__dirname, '..', 'package.json'), { confine: '..' });
             };
 
             server.route({ method: 'GET', path: '/file', handler: handler });
@@ -912,7 +912,7 @@ describe('file', () => {
             const server = provisionServer({ routes: { files: { relativeTo: __dirname } } });
             const handler = (request, reply) => {
 
-                reply.file(Path.join(__dirname, '..', 'package.json'));
+                reply.file(Path.join(__dirname, '..', 'package.json'), { confine: '..' });
             };
 
             server.route({ method: 'GET', path: '/file', handler: handler });
@@ -1025,7 +1025,7 @@ describe('file', () => {
             Fs.writeFileSync(filename, 'data');
 
             const server = provisionServer();
-            server.route({ method: 'GET', path: '/', handler: { file: filename } });
+            server.route({ method: 'GET', path: '/', handler: { file: { path: filename, confine: false } } });
             server.ext('onPreResponse', (request, reply) => {
 
                 Fs.unlinkSync(filename);
@@ -1045,7 +1045,7 @@ describe('file', () => {
             Fs.writeFileSync(filename, 'data');
 
             const server = provisionServer();
-            server.route({ method: 'GET', path: '/', handler: { file: filename } });
+            server.route({ method: 'GET', path: '/', handler: { file: { path: filename, confine: false } } });
             server.ext('onPreResponse', (request, reply) => {
 
                 const tempfile = filename + '~';
@@ -1133,7 +1133,7 @@ describe('file', () => {
 
 
             const server = provisionServer();
-            server.route({ method: 'GET', path: '/', handler: { file: filename } });
+            server.route({ method: 'GET', path: '/', handler: { file: { path: filename, confine: false } } });
 
             server.inject('/', (res) => {
 
@@ -1156,7 +1156,7 @@ describe('file', () => {
             };
 
             const server = provisionServer();
-            server.route({ method: 'GET', path: '/', handler: { file: filename } });
+            server.route({ method: 'GET', path: '/', handler: { file: { path: filename, confine: false } } });
 
             server.inject('/', (res) => {