Add uploading and converting directories#12
Add uploading and converting directories#12strawberry-raccoon wants to merge 1 commit intoswe-productivity:mainfrom
Conversation
C4illin
left a comment
There was a problem hiding this comment.
It works very well! I am just a little unsure about how you can now exploit paths, but I haven't managed to do any exploit. What do you think?
| @@ -0,0 +1,34 @@ | |||
| import { expect, test, describe } from "bun:test"; | |||
| import { isSafePath } from "../src/helpers/validatePath"; | |||
There was a problem hiding this comment.
You can still do things like:
test("should disallow dots in the middle", () => {
const path = "dir/../cat.jpg";
const result = isSafePath("./job/", `./job/${path}`);
expect(result).toEqual(false);
});There was a problem hiding this comment.
And more probably more things with some other special characters https://www.npmjs.com/package/sanitize-filename
There was a problem hiding this comment.
If we make sure to normalize the path first we can at least get rid of the dots in the middle problem
There was a problem hiding this comment.
Even when allowing it, I didn't manage to escape the correct folder in deleteFile (which doesn't use normalize)
There was a problem hiding this comment.
You can still do things like
Is there an actual issue here though? The key part is can you escape outside of ./ and while you can traverse around that directory, as long as you can't touch ./../ it's safe.
And more probably more things with some other special characters
Since we exclusively use Bun's path parsing, we shouldn't run into any issues. Even if they use a weird character, Bun will either consider it outside of the expected directory and therefore it will be rejected as unsafe or it will be inside the directory and therefore unsafe. Vulnerabilities would arise from parser differentials like passing the paths to other programs (which is handled with the converters) or using a different library to parse the paths where Bun would think it's safe but the other library wouldn't, but since we're consistent I'm not aware of any issues that could arise from special characters.
If we make sure to normalize the path first we can at least get rid of the dots in the middle problem
Is there a problem? As far as I'm aware you couldn't get any janky looking paths from normal usage as the JS APIs won't give e.g. /../ so only people modifying the API requests would see paths with e.g. /../ in, and even if you do that, it just means the URLs for when you try to download something looks weirder than you expect, but it's still safe. We could normalize fileName and therefore newFileName so the paths in the DB are nicer but it doesn't seem too dissimilar to "I can inspect element and delete the page which prevents me from using the website" (although if it is possible to run into issues that aren't clearly from intentionally trying to break it then it's more of an issue as regular users could also experience it).
I didn't manage to escape the correct folder in
deleteFile(which doesn't use normalize)
The security of isSafePath() shouldn't rely on normalize() as long as the paths stay with Bun functions so this shouldn't become an issue.
Closes #8