-
-
Notifications
You must be signed in to change notification settings - Fork 230
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add 'followsymlinks' option which allows rejecting symlinks #127
Open
jayk
wants to merge
8
commits into
expressjs:master
Choose a base branch
from
jayk:nosymlinks
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
eb6ac50
Add 'followsymlinks' option which, when set to false, rejects paths t…
jayk 84dddc9
Fix tests on fallthrough behavior, linting issues and add docs for fo…
jayk 6cc9833
skip followsymlink tests if symlinks don't work on target system
jayk 24869e2
Fix outstanding lint issues
jayk 1282b49
Update to pass O_NOFOLLOW to send when followsymlinks == false
jayk 6ca2674
Fix bug where accessing nonexistent file when followsymlinks=false co…
jayk 2759c85
Change to use require('constants') for old versions of node
jayk 79563ea
Use fs.constants if available, require('constants') on old versions o…
jayk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
users/ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
./ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
tobi.txt |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,13 +3,18 @@ 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('..') | ||
|
||
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,6 +764,89 @@ describe('serveStatic()', function () { | |
.get('/todo/') | ||
.expect(404, done) | ||
}) | ||
}); | ||
|
||
(skipSymlinks ? describe.skip : describe)('symlink tests', function () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Clever! Never seen this done before but makes sense :) |
||
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('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') | ||
.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') | ||
.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 }) | ||
}) | ||
|
||
it('accessing a real file works', function (done) { | ||
request(server) | ||
.get('/users/tobi.txt') | ||
.expect(200, 'ferret', 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 () { | ||
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 () { | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is happening in the even loop of a request, we should probably use the async of this, not sync so it doesn't block unnecessarily