From eb6ac5052ea763349a34fcfab8b890fd212cc646 Mon Sep 17 00:00:00 2001 From: Jay Kuri Date: Wed, 9 Oct 2019 23:34:50 -0600 Subject: [PATCH 1/8] Add 'followsymlinks' option which, when set to false, rejects paths that contain symlinks --- index.js | 28 +++++++++++++++++++++++++++ test/fixtures/members | 1 + test/fixtures/symroot | 1 + test/test.js | 44 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+) create mode 120000 test/fixtures/members create mode 120000 test/fixtures/symroot diff --git a/index.js b/index.js index b7d3984..2f14a3a 100644 --- a/index.js +++ b/index.js @@ -17,6 +17,7 @@ var encodeUrl = require('encodeurl') var escapeHtml = require('escape-html') var parseUrl = require('parseurl') var resolve = require('path').resolve +var fs = require('fs') var send = require('send') var url = require('url') @@ -50,6 +51,16 @@ function serveStatic (root, options) { // fall-though var fallthrough = opts.fallthrough !== false + // handle symlinks + var realroot + var followsymlinks = true + + // only set followsymlinks to false if it was explicitly set + if (opts.followsymlinks === false) { + followsymlinks = false + realroot = fs.realpathSync(root) + } + // default redirect var redirect = opts.redirect !== false @@ -86,12 +97,29 @@ function serveStatic (root, options) { var forwardError = !fallthrough var originalUrl = parseUrl.original(req) var path = parseUrl(req).pathname + var fullpath, realpath // make sure redirect occurs at mount if (path === '/' && originalUrl.pathname.substr(-1) !== '/') { path = '' } + if (followsymlinks == false) { + fullpath = realroot + path + realpath = fs.realpathSync(realroot+path) + // if the full path and the real path are not the same, + // then there is a symlink somewhere along the way + if (fullpath != realpath) { + if (fallthrough) { + return next() + } + + // forbidden on symlinks + this.error(403) + return + } + } + // create send stream var stream = send(req, path, opts) diff --git a/test/fixtures/members b/test/fixtures/members new file mode 120000 index 0000000..aa7fdfe --- /dev/null +++ b/test/fixtures/members @@ -0,0 +1 @@ +users/ \ No newline at end of file diff --git a/test/fixtures/symroot b/test/fixtures/symroot new file mode 120000 index 0000000..6a04314 --- /dev/null +++ b/test/fixtures/symroot @@ -0,0 +1 @@ +./ \ No newline at end of file diff --git a/test/test.js b/test/test.js index 94a6c6a..b88384d 100644 --- a/test/test.js +++ b/test/test.js @@ -761,6 +761,50 @@ describe('serveStatic()', function () { }) }) + describe('when followsymlinks is false', function () { + var server + before(function () { + server = createServer(fixtures, { followsymlinks: false }); + }) + + it('accessing a real file works', function (done) { + request(server) + .get('/users/tobi.txt') + .expect(200, 'ferret', done) + }) + + it('should 403 on a symlink', function (done) { + request(server) + .get('/members/tobi.txt') + .expect(404, done) + }) + + it('should fail on nested root symlink', function (done) { + request(server) + .get('/symroot/users/tobi.txt') + .expect(404, done) + }) + + }) + + describe('when followsymlinks is false and root had symlinks', function () { + var server + before(function () { + server = createServer(fixtures + "/symroot", { followsymlinks: false }); + }) + + it('accessing a real file works', function (done) { + request(server) + .get('/users/tobi.txt') + .expect(200, 'ferret', done) + }) + + it('should 403 on a symlink', function (done) { + request(server) + .get('/members/tobi.txt') + .expect(404, done) + }) + }) describe('when responding non-2xx or 304', function () { var server before(function () { From 84dddc9202ccd41ee633ba756e566a1ee07d24dd Mon Sep 17 00:00:00 2001 From: Jay Kuri Date: Thu, 10 Oct 2019 07:34:05 -0600 Subject: [PATCH 2/8] Fix tests on fallthrough behavior, linting issues and add docs for followymlinks --- README.md | 9 +++++++++ index.js | 14 ++++++++------ test/test.js | 29 ++++++++++++++++++++++++----- 3 files changed, 41 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 7cce428..268e535 100644 --- a/README.md +++ b/README.md @@ -92,6 +92,15 @@ all methods. The default value is `true`. +##### followsymlinks + +Determines how serve-static will handle how files or paths containing symlinks +are handled. Setting `followsymlinks` to `false` will cause serve-static to +reject requests for files that have a symlink in their path. + +The default value is `true`. + + ##### immutable Enable or disable the `immutable` directive in the `Cache-Control` response diff --git a/index.js b/index.js index 2f14a3a..75a1da9 100644 --- a/index.js +++ b/index.js @@ -57,8 +57,8 @@ function serveStatic (root, options) { // only set followsymlinks to false if it was explicitly set if (opts.followsymlinks === false) { - followsymlinks = false - realroot = fs.realpathSync(root) + followsymlinks = false + realroot = fs.realpathSync(root) } // default redirect @@ -104,10 +104,10 @@ function serveStatic (root, options) { path = '' } - if (followsymlinks == false) { + if (followsymlinks === false) { fullpath = realroot + path - realpath = fs.realpathSync(realroot+path) - // if the full path and the real path are not the same, + realpath = fs.realpathSync(realroot + path) + // if the full path and the real path are not the same, // then there is a symlink somewhere along the way if (fullpath != realpath) { if (fallthrough) { @@ -115,7 +115,9 @@ function serveStatic (root, options) { } // forbidden on symlinks - this.error(403) + res.statusCode = 403 + res.setHeader('Content-Length', '0') + res.end() return } } diff --git a/test/test.js b/test/test.js index b88384d..426d8a4 100644 --- a/test/test.js +++ b/test/test.js @@ -764,7 +764,7 @@ describe('serveStatic()', function () { describe('when followsymlinks is false', function () { var server before(function () { - server = createServer(fixtures, { followsymlinks: false }); + server = createServer(fixtures, { followsymlinks: false, fallthrough: false }) }) it('accessing a real file works', function (done) { @@ -776,21 +776,20 @@ describe('serveStatic()', function () { it('should 403 on a symlink', function (done) { request(server) .get('/members/tobi.txt') - .expect(404, done) + .expect(403, done) }) it('should fail on nested root symlink', function (done) { request(server) .get('/symroot/users/tobi.txt') - .expect(404, done) + .expect(403, done) }) - }) describe('when followsymlinks is false and root had symlinks', function () { var server before(function () { - server = createServer(fixtures + "/symroot", { followsymlinks: false }); + server = createServer(fixtures + '/symroot', { followsymlinks: false, fallthrough: false }) }) it('accessing a real file works', function (done) { @@ -800,11 +799,31 @@ describe('serveStatic()', function () { }) it('should 403 on a symlink', function (done) { + request(server) + .get('/members/tobi.txt') + .expect(403, done) + }) + }) + + describe('when followsymlinks is false and fallthrough is true', function () { + var server + before(function () { + server = createServer(fixtures, { followsymlinks: false, fallthrough: true }) + }) + + it('accessing a real file works', function (done) { + request(server) + .get('/users/tobi.txt') + .expect(200, 'ferret', done) + }) + + it('should 404 on a symlink', function (done) { request(server) .get('/members/tobi.txt') .expect(404, done) }) }) + describe('when responding non-2xx or 304', function () { var server before(function () { From 6cc983321b9b035f2b39a1ba4509efe05a8e5cfc Mon Sep 17 00:00:00 2001 From: Jay Kuri Date: Thu, 10 Oct 2019 07:51:07 -0600 Subject: [PATCH 3/8] skip followsymlink tests if symlinks don't work on target system --- test/test.js | 107 +++++++++++++++++++++++++++------------------------ 1 file changed, 57 insertions(+), 50 deletions(-) diff --git a/test/test.js b/test/test.js index 426d8a4..b7bad30 100644 --- a/test/test.js +++ b/test/test.js @@ -3,6 +3,7 @@ var assert = require('assert') var Buffer = require('safe-buffer').Buffer var http = require('http') var path = require('path') +var fs = require('fs') var request = require('supertest') var serveStatic = require('..') @@ -10,6 +11,10 @@ var fixtures = path.join(__dirname, '/fixtures') var relative = path.relative(process.cwd(), fixtures) var skipRelative = ~relative.indexOf('..') || path.resolve(relative) === relative +var skipSymlinks = true +try { + skipSymlinks = fs.realpathSync(fixtures + '/users/tobi.txt') !== fs.realpathSync(fixtures + '/members/tobi.txt') +} catch (e) {} describe('serveStatic()', function () { describe('basic operations', function () { @@ -759,68 +764,70 @@ describe('serveStatic()', function () { .get('/todo/') .expect(404, done) }) - }) + }); - describe('when followsymlinks is false', function () { - var server - before(function () { - server = createServer(fixtures, { followsymlinks: false, fallthrough: false }) - }) + (skipSymlinks ? describe.skip : describe)('symlink tests', function () { + describe('when followsymlinks is false', function () { + var server + before(function () { + server = createServer(fixtures, { followsymlinks: false, fallthrough: false }) + }) - it('accessing a real file works', function (done) { - request(server) - .get('/users/tobi.txt') - .expect(200, 'ferret', done) - }) + it('accessing a real file works', function (done) { + request(server) + .get('/users/tobi.txt') + .expect(200, 'ferret', done) + }) - it('should 403 on a symlink', function (done) { - request(server) - .get('/members/tobi.txt') - .expect(403, done) - }) + it('should 403 on a symlink', function (done) { + request(server) + .get('/members/tobi.txt') + .expect(403, done) + }) - it('should fail on nested root symlink', function (done) { - request(server) - .get('/symroot/users/tobi.txt') - .expect(403, done) + it('should fail on nested root symlink', function (done) { + request(server) + .get('/symroot/users/tobi.txt') + .expect(403, done) + }) }) - }) - describe('when followsymlinks is false and root had symlinks', function () { - var server - before(function () { - server = createServer(fixtures + '/symroot', { followsymlinks: false, fallthrough: false }) - }) + describe('when followsymlinks is false and root had symlinks', function () { + var server + before(function () { + server = createServer(fixtures + '/symroot', { followsymlinks: false, fallthrough: false }) + }) - it('accessing a real file works', function (done) { - request(server) - .get('/users/tobi.txt') - .expect(200, 'ferret', done) - }) + it('accessing a real file works', function (done) { + request(server) + .get('/users/tobi.txt') + .expect(200, 'ferret', done) + }) - it('should 403 on a symlink', function (done) { - request(server) - .get('/members/tobi.txt') - .expect(403, done) + it('should 403 on a symlink', function (done) { + request(server) + .get('/members/tobi.txt') + .expect(403, done) + }) }) - }) - describe('when followsymlinks is false and fallthrough is true', function () { - var server - before(function () { - server = createServer(fixtures, { followsymlinks: false, fallthrough: true }) - }) + describe('when followsymlinks is false and fallthrough is true', function () { + var server + before(function () { + server = createServer(fixtures, { followsymlinks: false, fallthrough: true }) + }) - it('accessing a real file works', function (done) { - request(server) - .get('/users/tobi.txt') - .expect(200, 'ferret', done) - }) + it('accessing a real file works', function (done) { + request(server) + .get('/users/tobi.txt') + .expect(200, 'ferret', done) + }) - it('should 404 on a symlink', function (done) { - request(server) - .get('/members/tobi.txt') - .expect(404, done) + it('should 404 on a symlink', function (done) { + request(server) + .get('/members/tobi.txt') + .expect(404, done) + }) }) }) From 24869e20085e6041bb58ddd3940268377fc9c2ef Mon Sep 17 00:00:00 2001 From: Jay Kuri Date: Thu, 10 Oct 2019 07:56:53 -0600 Subject: [PATCH 4/8] Fix outstanding lint issues --- index.js | 2 +- test/test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/index.js b/index.js index 75a1da9..bb0c5a0 100644 --- a/index.js +++ b/index.js @@ -109,7 +109,7 @@ function serveStatic (root, options) { realpath = fs.realpathSync(realroot + path) // if the full path and the real path are not the same, // then there is a symlink somewhere along the way - if (fullpath != realpath) { + if (fullpath !== realpath) { if (fallthrough) { return next() } diff --git a/test/test.js b/test/test.js index b7bad30..4616ff4 100644 --- a/test/test.js +++ b/test/test.js @@ -13,7 +13,7 @@ var relative = path.relative(process.cwd(), fixtures) var skipRelative = ~relative.indexOf('..') || path.resolve(relative) === relative var skipSymlinks = true try { - skipSymlinks = fs.realpathSync(fixtures + '/users/tobi.txt') !== fs.realpathSync(fixtures + '/members/tobi.txt') + skipSymlinks = fs.realpathSync(fixtures + '/users/tobi.txt') !== fs.realpathSync(fixtures + '/members/tobi.txt') } catch (e) {} describe('serveStatic()', function () { From 1282b49cf1b3f1585470da15ebf3d618fa5d0ab0 Mon Sep 17 00:00:00 2001 From: Jay Kuri Date: Mon, 21 Oct 2019 18:21:34 -0600 Subject: [PATCH 5/8] Update to pass O_NOFOLLOW to send when followsymlinks == false --- README.md | 5 +++++ index.js | 16 ++++++++++------ test/fixtures/users/william.txt | 1 + test/test.js | 16 ++++++++++++++-- 4 files changed, 30 insertions(+), 8 deletions(-) create mode 120000 test/fixtures/users/william.txt diff --git a/README.md b/README.md index 268e535..1d01a63 100644 --- a/README.md +++ b/README.md @@ -100,6 +100,11 @@ reject requests for files that have a symlink in their path. The default value is `true`. +Note that setting `followsymlinks` to `false` also causes the the module +to resolve any symbolic links in the root path during startup. This means +that if your root path does contain symlinks, changes to those symlinks after +application startup will not be noticed. + ##### immutable diff --git a/index.js b/index.js index bb0c5a0..2b76598 100644 --- a/index.js +++ b/index.js @@ -55,12 +55,6 @@ function serveStatic (root, options) { var realroot var followsymlinks = true - // only set followsymlinks to false if it was explicitly set - if (opts.followsymlinks === false) { - followsymlinks = false - realroot = fs.realpathSync(root) - } - // default redirect var redirect = opts.redirect !== false @@ -75,6 +69,16 @@ function serveStatic (root, options) { opts.maxage = opts.maxage || opts.maxAge || 0 opts.root = resolve(root) + // only set followsymlinks to false if it was explicitly set + if (opts.followsymlinks === false) { + followsymlinks = false + realroot = fs.realpathSync(root) + opts.flags = fs.constants.O_NOFOLLOW | fs.constants.O_RDONLY + // if followsymlinks is disabled, we need the fully resolved + // (un-symlink'd) root to start + opts.root = realroot + } + // construct directory listener var onDirectory = redirect ? createRedirectDirectoryListener() diff --git a/test/fixtures/users/william.txt b/test/fixtures/users/william.txt new file mode 120000 index 0000000..5540581 --- /dev/null +++ b/test/fixtures/users/william.txt @@ -0,0 +1 @@ +tobi.txt \ No newline at end of file diff --git a/test/test.js b/test/test.js index 4616ff4..58add7d 100644 --- a/test/test.js +++ b/test/test.js @@ -779,12 +779,18 @@ describe('serveStatic()', function () { .expect(200, 'ferret', done) }) - it('should 403 on a symlink', function (done) { + it('should 403 on a symlink in the path', function (done) { request(server) .get('/members/tobi.txt') .expect(403, done) }) + it('should 403 on a symlink as the file', function (done) { + request(server) + .get('/users/william.txt') + .expect(403, done) + }) + it('should fail on nested root symlink', function (done) { request(server) .get('/symroot/users/tobi.txt') @@ -804,11 +810,17 @@ describe('serveStatic()', function () { .expect(200, 'ferret', done) }) - it('should 403 on a symlink', function (done) { + it('should 403 on a symlink in the path', function (done) { request(server) .get('/members/tobi.txt') .expect(403, done) }) + + it('should 403 on a symlink as the file', function (done) { + request(server) + .get('/users/william.txt') + .expect(403, done) + }) }) describe('when followsymlinks is false and fallthrough is true', function () { From 6ca2674e4ba7cfd28b75c264f2e7f5503eb250dc Mon Sep 17 00:00:00 2001 From: Jay Kuri Date: Mon, 21 Oct 2019 18:37:21 -0600 Subject: [PATCH 6/8] Fix bug where accessing nonexistent file when followsymlinks=false could cause a hang. --- index.js | 6 +++++- test/test.js | 6 ++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/index.js b/index.js index 2b76598..e48ca4b 100644 --- a/index.js +++ b/index.js @@ -110,7 +110,11 @@ function serveStatic (root, options) { if (followsymlinks === false) { fullpath = realroot + path - realpath = fs.realpathSync(realroot + path) + try { + realpath = fs.realpathSync(realroot + path) + } catch (e) { + realpath = undefined + } // if the full path and the real path are not the same, // then there is a symlink somewhere along the way if (fullpath !== realpath) { diff --git a/test/test.js b/test/test.js index 58add7d..790f05f 100644 --- a/test/test.js +++ b/test/test.js @@ -779,6 +779,12 @@ describe('serveStatic()', function () { .expect(200, 'ferret', done) }) + it('should 403 on nonexistant file', function (done) { + request(server) + .get('/users/bob.txt') + .expect(403, done) + }) + it('should 403 on a symlink in the path', function (done) { request(server) .get('/members/tobi.txt') From 2759c857f85f8451ebea96700ca0574c9055888a Mon Sep 17 00:00:00 2001 From: Jay Kuri Date: Mon, 21 Oct 2019 19:00:20 -0600 Subject: [PATCH 7/8] Change to use require('constants') for old versions of node --- index.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/index.js b/index.js index e48ca4b..7a86952 100644 --- a/index.js +++ b/index.js @@ -18,6 +18,7 @@ var escapeHtml = require('escape-html') var parseUrl = require('parseurl') var resolve = require('path').resolve var fs = require('fs') +var constants = require('constants') var send = require('send') var url = require('url') @@ -73,7 +74,7 @@ function serveStatic (root, options) { if (opts.followsymlinks === false) { followsymlinks = false realroot = fs.realpathSync(root) - opts.flags = fs.constants.O_NOFOLLOW | fs.constants.O_RDONLY + opts.flags = constants.O_RDONLY | constants.O_NOFOLLOW // if followsymlinks is disabled, we need the fully resolved // (un-symlink'd) root to start opts.root = realroot From 79563ea50a4f0479ecea917924284d01721c9262 Mon Sep 17 00:00:00 2001 From: Jay Kuri Date: Mon, 21 Oct 2019 19:05:46 -0600 Subject: [PATCH 8/8] Use fs.constants if available, require('constants') on old versions of node. --- index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.js b/index.js index 7a86952..8dfb143 100644 --- a/index.js +++ b/index.js @@ -18,7 +18,7 @@ var escapeHtml = require('escape-html') var parseUrl = require('parseurl') var resolve = require('path').resolve var fs = require('fs') -var constants = require('constants') +var constants = fs.constants || require('constants') // eslint-disable-line node/no-deprecated-api var send = require('send') var url = require('url')