Conversation
lib/providers/filesystem/index.js
Outdated
| var self = this; | ||
| if (!validateName(containerName, cb)) return; | ||
| var dir = path.join(this.root, containerName); | ||
| this.getProperPath(dir, this.root); |
There was a problem hiding this comment.
We already have a method to detect invalid container and file names, see validateName. Please fix this existing method instead of adding a new one.
lib/providers/filesystem/index.js
Outdated
| FileSystemProvider.prototype.getProperPath = function(dirPath, rootPath) { | ||
| if (dirPath.indexOf(rootPath) !== 0) { | ||
| var errmsg = ('Illegal character'); | ||
| throw new Error(errmsg); |
There was a problem hiding this comment.
Mixing throw err and callback(err) is confusing for consumers of this module/API. Most provider methods use a callback to signal errors, therefore API consumers don't expect the method to throw an error. Since "illegal character error" is sort of an expected situation, it shouldn't crash the app on unhandled errors.
(It does not matter much here, since getProperPath will go away in favour of existing validateName function, I am writing this comment to educate you for the future.)
|
@bajtos PTAL. Thanks. |
lib/providers/filesystem/index.js
Outdated
|
|
||
| function validateName(name, cb) { | ||
| if (!name) { | ||
| if (!name || name === '..') { |
There was a problem hiding this comment.
You should check if name.indexOf('..') !== -1 instead of name === '..'.
lib/providers/filesystem/index.js
Outdated
|
|
||
| function validateName(name, cb) { | ||
| if (!name) { | ||
| if (!name || name.indexOf('..') !== -1) { |
There was a problem hiding this comment.
This is incorrect. .. as part of a path component is valid:
/tmp/foo/from..to
/tmp/..valid-but-badlynamed
/tmp/.../dont-actually-use-paths-like-even-though-you-can
The last 2 examples above would be hidden by UNIX dot-file logic, but that's more about ls defaults than anything else.
There was a problem hiding this comment.
Fix path joining security issue
Is not a useful commit message. It tells people there is a security issue, and they can see the issue from the PR, so if vagueness is supposed to be protective, I don't think it suceeds. The commit message should say what it does:
Disallow paths that contain
..path elements
perhaps
lib/providers/filesystem/index.js
Outdated
| } | ||
|
|
||
| var namePattern = new RegExp('[^' + path.sep + '/]+'); | ||
| var badPath = /(^|[\\\/])\.\.([\\\/]|$)/; |
There was a problem hiding this comment.
IMO, security code should be clear on what it is doing. A comment would be good. Its looking for any of
.., ../blah, blah/.., or blah/../blah, right? A better name would be containsDotDotPaths().
test/upload-download.test.js
Outdated
| }); | ||
| }); | ||
|
|
||
| it('fails to upload using bad file path', function(done) { |
There was a problem hiding this comment.
need 4 tests, I think, one for each circumstance, and I guess the "not bad" paths are already covered by the tests?
loay
force-pushed
the
proper-path
branch
2 times, most recently
from
9df1f77 to
64dd54e
Compare
| }); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
don't your eslint rules forbid this kind of thing? Its not related to this PR, maybe you want to save this change for a PR to improve (or add?) the eslint config.
There was a problem hiding this comment.
That behavior happens once in a while from the editor. I had no control on it. Not sure why.
I will fix it in a different PR.
|
Thank you @sam-github for helping with the review! The patch looks great now 👍 |
5 participants
connect to https://github.com/strongloop-internal/scrum-apex/issues/32