From 0a082473e36052b65c9591d53238531ca3a0e705 Mon Sep 17 00:00:00 2001 From: Simon de Almeida Date: Sun, 2 Apr 2017 12:15:42 -0400 Subject: [PATCH 1/6] [WIP] Fixing issue 13099 --- src/project/ProjectModel.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/project/ProjectModel.js b/src/project/ProjectModel.js index 809d175d73b..fbe33f50995 100644 --- a/src/project/ProjectModel.js +++ b/src/project/ProjectModel.js @@ -76,7 +76,8 @@ define(function (require, exports, module) { * also used to substitute the place holder of the error message. */ var _invalidChars; - + var _pattern = /([?\*\|\:\<\>\\]+|\/{2,}|\.{2,}|\.$)/i // TODO: This will replace _invalidChars + /** * @private * RegEx to validate if a filename is not allowed even if the system allows it. @@ -99,8 +100,7 @@ define(function (require, exports, module) { // Checks for valid Windows filenames: // See http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx return !( - new RegExp("[" + invalidChars + "]+").test(filename) || - _illegalFilenamesRegEx.test(filename) + filename.match(_pattern) || filename.match(_illegalFilenamesRegEx) ); } @@ -182,8 +182,8 @@ define(function (require, exports, module) { function doCreate(path, isFolder) { var d = new $.Deferred(); - var name = FileUtils.getBaseName(path); - if (!isValidFilename(name, _invalidChars)) { + // Check if full path is valid + if (!isValidFilename(path, _invalidChars)) { return d.reject(ERROR_INVALID_FILENAME).promise(); } From 342aae755d93cdb0c9a3c44cdae2a53f946ffafd Mon Sep 17 00:00:00 2001 From: Simon de Almeida Date: Sun, 2 Apr 2017 12:16:58 -0400 Subject: [PATCH 2/6] Fixed error --- src/project/ProjectModel.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/project/ProjectModel.js b/src/project/ProjectModel.js index fbe33f50995..7695b03d3ff 100644 --- a/src/project/ProjectModel.js +++ b/src/project/ProjectModel.js @@ -76,7 +76,7 @@ define(function (require, exports, module) { * also used to substitute the place holder of the error message. */ var _invalidChars; - var _pattern = /([?\*\|\:\<\>\\]+|\/{2,}|\.{2,}|\.$)/i // TODO: This will replace _invalidChars + var _pattern = /([?\*\|\:\<\>\\]+|\/{2,}|\.{2,}|\.$)/i; // TODO: This will replace _invalidChars /** * @private From d5a63d437c45d700cbc5eec215b130ce4a03e743 Mon Sep 17 00:00:00 2001 From: Simon de Almeida Date: Sun, 2 Apr 2017 12:36:59 -0400 Subject: [PATCH 3/6] Renamed variables --- src/project/ProjectModel.js | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/src/project/ProjectModel.js b/src/project/ProjectModel.js index 7695b03d3ff..bc48ba90743 100644 --- a/src/project/ProjectModel.js +++ b/src/project/ProjectModel.js @@ -75,8 +75,7 @@ define(function (require, exports, module) { * When a filename with one of these invalid characters are detected, then it is * also used to substitute the place holder of the error message. */ - var _invalidChars; - var _pattern = /([?\*\|\:\<\>\\]+|\/{2,}|\.{2,}|\.$)/i; // TODO: This will replace _invalidChars + var _invalidChars = /([?\*\|\:\<\>\\\"]+|\/{2,}|\.{2,}|\.$)/i; /** * @private @@ -88,6 +87,7 @@ define(function (require, exports, module) { /** * Returns true if this matches valid filename specifications. + * See http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx * * TODO: This likely belongs in FileUtils. * @@ -96,11 +96,10 @@ define(function (require, exports, module) { * @return {boolean} true if the filename is valid */ function isValidFilename(filename, invalidChars) { - // Validate file name - // Checks for valid Windows filenames: - // See http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx + // Fix issue adobe#13099 + // See https://github.com/adobe/brackets/issues/13099 return !( - filename.match(_pattern) || filename.match(_illegalFilenamesRegEx) + filename.match(invalidChars) || filename.match(_illegalFilenamesRegEx) ); } @@ -1345,21 +1344,12 @@ define(function (require, exports, module) { return welcomeProjects.indexOf(pathNoSlash) !== -1; } - // Init invalid characters string - if (brackets.platform === "mac") { - _invalidChars = "?*|:/"; - } else if (brackets.platform === "linux") { - _invalidChars = "?*|/"; - } else { - _invalidChars = "/?*:<>\\|\""; // invalid characters on Windows - } - exports._getWelcomeProjectPath = _getWelcomeProjectPath; exports._addWelcomeProjectPath = _addWelcomeProjectPath; exports._isWelcomeProjectPath = _isWelcomeProjectPath; exports._ensureTrailingSlash = _ensureTrailingSlash; exports._shouldShowName = _shouldShowName; - exports._invalidChars = _invalidChars; + exports._invalidChars = "? * | : / < > \\ | \" .."; exports.shouldShow = shouldShow; exports.defaultIgnoreGlobs = defaultIgnoreGlobs; From 4e44cd34642898a6b4b9adbceae4730f89b77f19 Mon Sep 17 00:00:00 2001 From: Simon de Almeida Date: Sun, 2 Apr 2017 12:39:22 -0400 Subject: [PATCH 4/6] Fixed typo --- src/project/ProjectModel.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/project/ProjectModel.js b/src/project/ProjectModel.js index bc48ba90743..ced2eec6384 100644 --- a/src/project/ProjectModel.js +++ b/src/project/ProjectModel.js @@ -76,7 +76,7 @@ define(function (require, exports, module) { * also used to substitute the place holder of the error message. */ var _invalidChars = /([?\*\|\:\<\>\\\"]+|\/{2,}|\.{2,}|\.$)/i; - + /** * @private * RegEx to validate if a filename is not allowed even if the system allows it. From 935028e3a025fe8759e5956a120129bd7ba90fcd Mon Sep 17 00:00:00 2001 From: Simon de Almeida Date: Sun, 2 Apr 2017 15:15:51 -0400 Subject: [PATCH 5/6] Added test cases --- src/project/ProjectModel.js | 42 +++++++++---- test/spec/ProjectModel-test.js | 106 +++++++++++++++++++++++++++++++-- 2 files changed, 129 insertions(+), 19 deletions(-) diff --git a/src/project/ProjectModel.js b/src/project/ProjectModel.js index ced2eec6384..38ea49e48ab 100644 --- a/src/project/ProjectModel.js +++ b/src/project/ProjectModel.js @@ -70,20 +70,16 @@ define(function (require, exports, module) { /** * @private - * A string containing all invalid characters for a specific platform. - * This will be used to construct a regular expression for checking invalid filenames. - * When a filename with one of these invalid characters are detected, then it is - * also used to substitute the place holder of the error message. + * RegEx to validate a file path. */ - var _invalidChars = /([?\*\|\:\<\>\\\"]+|\/{2,}|\.{2,}|\.$)/i; + var _invalidChars = /([?\*\|\<\>"]+|\/{2,}|\.{2,}|\.$)/i; /** * @private * RegEx to validate if a filename is not allowed even if the system allows it. * This is done to prevent cross-platform issues. */ - - var _illegalFilenamesRegEx = /^(\.+|com[1-9]|lpt[1-9]|nul|con|prn|aux|)$|\.+$/i; + var _illegalFilenamesRegEx = /((\b(com[0-9]+|lpt[0-9]+|nul|con|prn|aux)\b)|\.+$|\/+|\\+|\:)/i; /** * Returns true if this matches valid filename specifications. @@ -92,17 +88,29 @@ define(function (require, exports, module) { * TODO: This likely belongs in FileUtils. * * @param {string} filename to check - * @param {string} invalidChars List of characters that are disallowed * @return {boolean} true if the filename is valid */ - function isValidFilename(filename, invalidChars) { + function isValidFilename(filename) { // Fix issue adobe#13099 // See https://github.com/adobe/brackets/issues/13099 return !( - filename.match(invalidChars) || filename.match(_illegalFilenamesRegEx) + filename.match(_invalidChars)|| filename.match(_illegalFilenamesRegEx) + //filename.match(_invalidChars) || filename.match(_illegalFilenamesRegEx) ); } + /** + * Returns true if given path is valid. + * + * @param {string} path to check + * @return {boolean} true if the filename is valid + */ + function isValidPath(path) { + // Fix issue adobe#13099 + // See https://github.com/adobe/brackets/issues/13099 + return !(path.match(_invalidChars)); + } + /** * @private * @see #shouldShow @@ -180,9 +188,16 @@ define(function (require, exports, module) { */ function doCreate(path, isFolder) { var d = new $.Deferred(); + var filename = FileUtils.getBaseName(path); + + // Check if filename + if (!isValidFilename(filename)){ + return d.reject(ERROR_INVALID_FILENAME).promise(); + } - // Check if full path is valid - if (!isValidFilename(path, _invalidChars)) { + // Check if fullpath with filename is valid + // This check is used to circumvent directory jumps (Like ../..) + if (!isValidPath(path)) { return d.reject(ERROR_INVALID_FILENAME).promise(); } @@ -905,7 +920,7 @@ define(function (require, exports, module) { if (oldPath === newPath) { result.resolve(); - } else if (!isValidFilename(newName, _invalidChars)) { + } else if (!isValidFilename(newName)) { result.reject(ERROR_INVALID_FILENAME); } else { var entry = isFolder ? FileSystem.getDirectoryForPath(oldPath) : FileSystem.getFileForPath(oldPath); @@ -1354,6 +1369,7 @@ define(function (require, exports, module) { exports.shouldShow = shouldShow; exports.defaultIgnoreGlobs = defaultIgnoreGlobs; exports.isValidFilename = isValidFilename; + exports.isValidPath = isValidPath; exports.EVENT_CHANGE = EVENT_CHANGE; exports.EVENT_SHOULD_SELECT = EVENT_SHOULD_SELECT; exports.EVENT_SHOULD_FOCUS = EVENT_SHOULD_FOCUS; diff --git a/test/spec/ProjectModel-test.js b/test/spec/ProjectModel-test.js index 6e4b76c0b00..5c007f5e719 100644 --- a/test/spec/ProjectModel-test.js +++ b/test/spec/ProjectModel-test.js @@ -311,17 +311,111 @@ define(function (require, exports, module) { }); }); + describe("isValidPath", function () { + it("returns true for UNIX style file path", function () { + expect(ProjectModel.isValidPath("/tmp/src/test/")).toBe(true); + }); + + it("returns true for WINDOWS style file path", function () { + expect(ProjectModel.isValidPath("C:\\tmp\\src\\test\\")).toBe(true); + }); + + it("returns false for path that contains an invalid char \'..\'", function () { + expect(ProjectModel.isValidPath("../tmp/src/test/")).toBe(false); + }); + + it("returns false for path that contains an invalid char \'../..\'", function () { + expect(ProjectModel.isValidPath("../../tmp/src/test/")).toBe(false); + }); + + it("returns false for path that contains an invalid char \'..\\..\'", function () { + expect(ProjectModel.isValidPath("..\\..\\tmp\\src\\test\\")).toBe(false); + }); + }); + describe("isValidFilename", function () { - it("returns true for filenames with nothing invalid", function () { - expect(ProjectModel.isValidFilename("foo.txt", "*")).toBe(true); + it("returns true for simple filename", function () { + expect(ProjectModel.isValidFilename("foo.txt")).toBe(true); + }); + + it("returns true for filename that starts with a '\.\'", function () { + expect(ProjectModel.isValidFilename(".tmp")).toBe(true); + }); + + it("returns false for filenames that has ends with a \'.\'", function () { + expect(ProjectModel.isValidFilename("dummy.")).toBe(false); + }); + + it("returns false for filename that contains an invalid char \'?\'", function () { + expect(ProjectModel.isValidFilename("foo?txt")).toBe(false); + }); + + it("returns false for filename that contains an invalid char \'*\'", function () { + expect(ProjectModel.isValidFilename("foo\*txt")).toBe(false); + }); + + it("returns false for filename that contains an invalid char \'\|\'", function () { + expect(ProjectModel.isValidFilename("foo\|txt")).toBe(false); + }); + + it("returns false for filename that contains an invalid char \'\:\'", function () { + expect(ProjectModel.isValidFilename("foo\:txt")).toBe(false); + }); + + it("returns false for filename that contains an invalid char \'\<\'", function () { + expect(ProjectModel.isValidFilename("foo\\'", function () { + expect(ProjectModel.isValidFilename("foo\>txt")).toBe(false); + }); + + it("returns false for filename that contains an invalid char \'/\'", function () { + expect(ProjectModel.isValidFilename("directory/foo.txt")).toBe(false); + }); + + it("returns false for filename that contains an invalid char \'//\'", function () { + expect(ProjectModel.isValidFilename("directory//foo.txt")).toBe(false); }); - it("returns false for filenames that match the invalid characters", function () { - expect(ProjectModel.isValidFilename("foo*txt", "|*")).toBe(false); + it("returns false for filename that contains an invalid char \'\\\'", function () { + expect(ProjectModel.isValidFilename("directory\\foo.txt")).toBe(false); + }); + + it("returns false for filename that contains an invalid char \'\\\\\'", function () { + expect(ProjectModel.isValidFilename("directory\\\\foo.txt")).toBe(false); + }); + + it("returns false for filename that contains an invalid char \'..\'", function () { + expect(ProjectModel.isValidFilename("..foo")).toBe(false); + }); + + it("returns false for filename that contains an invalid char \'..\\..\'", function () { + expect(ProjectModel.isValidFilename("..\\foo")).toBe(false); + }); + + it("returns false for filenames that has invalid name \'com1\'", function () { + expect(ProjectModel.isValidFilename("com1")).toBe(false); + }); + + it("returns false for filenames that has invalid name \'lpt\'", function () { + expect(ProjectModel.isValidFilename("lpt1")).toBe(false); + }); + + it("returns false for filenames that has invalid name \'nul\'", function () { + expect(ProjectModel.isValidFilename("nul")).toBe(false); + }); + + it("returns false for filenames that has invalid name \'con\'", function () { + expect(ProjectModel.isValidFilename("con")).toBe(false); + }); + + it("returns false for filenames that has invalid name \'prn\'", function () { + expect(ProjectModel.isValidFilename("prn")).toBe(false); }); - it("returns false for filenames that match the internal list of disallowed names", function () { - expect(ProjectModel.isValidFilename("/test/prn")).toBe(false); + it("returns false for filenames that has invalid name \'aux\'", function () { + expect(ProjectModel.isValidFilename("aux")).toBe(false); }); }); From 07c2e47db47d252d4ed8f8c83ba611be3812bdf3 Mon Sep 17 00:00:00 2001 From: Simon de Almeida Date: Mon, 24 Apr 2017 17:38:32 -0400 Subject: [PATCH 6/6] Removed unused code. --- src/project/ProjectModel.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/project/ProjectModel.js b/src/project/ProjectModel.js index 38ea49e48ab..95ffbc26bdf 100644 --- a/src/project/ProjectModel.js +++ b/src/project/ProjectModel.js @@ -95,7 +95,6 @@ define(function (require, exports, module) { // See https://github.com/adobe/brackets/issues/13099 return !( filename.match(_invalidChars)|| filename.match(_illegalFilenamesRegEx) - //filename.match(_invalidChars) || filename.match(_illegalFilenamesRegEx) ); }