Skip to content

fix: rewrite path resolution logic #21

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

Merged
merged 5 commits into from
Dec 9, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ ignore
node_modules
.DS_Store
package-lock.json
.idea
tests
7 changes: 7 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,13 @@ Contributions are welcome.

Use prettier for formatting

## Testing

Run unit tests with
```sh
$ yarn run test
```

Testing - try:

```sh
Expand Down
65 changes: 14 additions & 51 deletions angular-http-server.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ var https = require("https");
var http = require("http");
var opn = require("opn");

const getFilePathFromUrl = require('./lib/get-file-path-from-url');


const NO_PATH_FILE_ERROR_MESSAGE =
"Error: index.html could not be found in the specified path ";
Expand All @@ -29,12 +31,12 @@ if (argv.config) {
} else {
config = getConfig;
}

// supplement argv with config, but CLI args take precedence
argv = Object.assign({}, config, argv);
}

const useHttps = argv.ssl || argv.https;
const basePath = argv.path ? path.resolve(argv.path) : process.cwd();

// As a part of the startup - check to make sure we can access index.html
returnDistFile(true);
Expand Down Expand Up @@ -97,13 +99,7 @@ function requestListener(req, res) {
}
}

// Request is for a page instead
// Only interested in the part before any query params
var url = req.url.split("?")[0];
// Attaches path prefix with --path option
var possibleFilename = resolveUrl(url.slice(1)) || "dummy";

var safeFullFileName = safeFileName(possibleFilename);
const safeFullFileName = getFilePathFromUrl(req.url, basePath);

fs.stat(safeFullFileName, function(err, stats) {
var fileBuffer;
Expand Down Expand Up @@ -138,45 +134,19 @@ function getPort() {

function returnDistFile(displayFileMessages = false) {
var distPath;
var argvPath = argv.path;

if (argvPath) {
try {
if (displayFileMessages) {
log("Path specified: %s", argvPath);
}
distPath = path.join(argvPath, "index.html");
if (displayFileMessages) {
log("Using %s", distPath);
}
return fs.readFileSync(distPath);
} catch (e) {
console.warn(NO_PATH_FILE_ERROR_MESSAGE + "%s", argvPath);
process.exit(1);
}
} else {

try {
if (displayFileMessages) {
log("Info: Path not specified using the working directory.");
log("Serving from path: %s", basePath);
}
distPath = "index.html";
try {
return fs.readFileSync(distPath);
} catch (e) {
console.warn(NO_ROOT_FILE_ERROR_MESSAGE);
process.exit(1);
distPath = path.join(basePath, 'index.html');
if (displayFileMessages) {
log("Using default file: %s", distPath);
}
}
}

function resolveUrl(filename) {
// basic santizing to prevent attempts to read files outside of directory set
if (filename.includes("..")) {
return null;
}
if (filename && argv.path) {
return path.join(argv.path, filename);
} else {
return filename;
return fs.readFileSync(distPath);
} catch (e) {
console.warn(NO_PATH_FILE_ERROR_MESSAGE + "%s", basePath);
process.exit(1);
}
}

Expand All @@ -185,10 +155,3 @@ function log() {
console.log.apply(console, arguments);
}
}

// Prevents a file path being provided that uses '..'
function safeFileName(possibleFilename) {
let tmp = path.normalize(possibleFilename).replace(/^(\.\.[\/\\])+/, "");
// Insert "." to ensure file is read relatively (Security)
return path.join(".", tmp);
}
37 changes: 37 additions & 0 deletions lib/get-file-path-from-url.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
const path = require('path');

/**
* Safely get the path for a file in the project directory, or reject by returning "dummy"
*
* @param {string} url
* @param {string} basePath - absolute path to base directory
* @param {object} [pathLib=path] - path library, override for testing
* @return {string} - will return 'dummy' if the path is bad
*/
function getFilePathFromUrl(url, basePath, { pathLib = path } = {}) {
if (!basePath) {
throw new Error('basePath must be specified');
}
if (!pathLib.isAbsolute(basePath)) {
throw new Error(`${basePath} is invalid - must be absolute`);
}

const relativePath = url.split('?')[0];

if (relativePath.indexOf('../') > -1) {
// any path attempting to traverse up the directory should be rejected
return 'dummy';
}

const absolutePath = pathLib.join(basePath, relativePath);
if (
!absolutePath.startsWith(basePath) || // if the path has broken out of the basePath, it should be rejected
absolutePath.endsWith(pathLib.sep) // only files (not folders) can be served
) {
return 'dummy';
}

return absolutePath;
}

module.exports = getFilePathFromUrl;
81 changes: 81 additions & 0 deletions lib/get-file-path-from-url.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
const path = require('path');

const getFilePathFromUrl = require('./get-file-path-from-url');

describe('getFilePathFromUrl', () => {
const VALID_PATHS = [
// normal inputs
'/index.html',
'/beach.js?_=1236363',
'/static/beach.jpg',
'/VERSION',
// common false-positives
'/./index.html',
'/a.weird.folder/file.css',
'/a..weirder..folder/file.js',
'/.valid/test.zip',
'/..valid/test.zip',
'/......a/dots.jpg',
];
const INVALID_PATHS = [
// directories
'/',
'/../',
'/static/',
// traversal within base path
'/../output/index.html',
'/static/../index.html',
// traversal out of base path
'/..',
'/../static/file.png',
'/../../../../sys/passwd', // https://hackerone.com/reports/309120
];

function testInvalidBasePaths(pathLib, relativePaths) {
const input = VALID_PATHS[0];

it('should throw an error if the basePath is undefined', () => {
expect(() => getFilePathFromUrl(input, undefined, { pathLib })).to.throw('basePath must be specified');
});

context('where basePath is not absolute', () => {
relativePaths.forEach((basePath) => {
it(`should throw an error if basePath = ${basePath}`, () => {
expect(() => getFilePathFromUrl(input, basePath, { pathLib })).to.throw('must be absolute');
});
});
});
}

function testValidBasePaths(pathLib, basePaths) {
basePaths.forEach((basePath) => {
context(`where basePath = ${basePath}`, () => {
function createTest(input, expected) {
it(`should return "${expected}" given input "${input}"`, () => {
expect(getFilePathFromUrl(input, basePath, { pathLib })).to.equal(expected);
});
}

VALID_PATHS.forEach((testPath) => {
const expected = pathLib.join(basePath, testPath).split('?')[0];
createTest(testPath, expected);
});
INVALID_PATHS.forEach((testPath) => createTest(testPath, 'dummy'));
});
});
}

context('on a Windows system', () => {
const pathLib = path.win32;

testInvalidBasePaths(pathLib, ['dist\\output', '.\\', '.', '.\\serve']);
testValidBasePaths(pathLib, ['C:\\', 'C:\\project', 'C:\\project\\serve\\output']);
});

context('on a POSIX system', () => {
const pathLib = path.posix;

testInvalidBasePaths(pathLib, ['dist/output', './', '.', './serve']);
testValidBasePaths(pathLib, ['/', '/home', '/home/user/project/serve/output']);
});
});
6 changes: 5 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"routing"
],
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"
"test": "mocha"
},
"contributors": [
{
Expand All @@ -39,5 +39,9 @@
"minimist": "^1.2.0",
"opn": "^5.3.0",
"pem": "^1.9.7"
},
"devDependencies": {
"chai": "^4.2.0",
"mocha": "^5.2.0"
}
}
2 changes: 2 additions & 0 deletions test/mocha.opts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
--require chai/register-expect
lib/**/*.spec.js
Loading