Skip to content

Add uploading and converting directories#12

Open
strawberry-raccoon wants to merge 1 commit intoswe-productivity:mainfrom
strawberry-raccoon:add-folders
Open

Add uploading and converting directories#12
strawberry-raccoon wants to merge 1 commit intoswe-productivity:mainfrom
strawberry-raccoon:add-folders

Conversation

@strawberry-raccoon
Copy link

Closes #8

@C4illin C4illin self-assigned this Feb 17, 2026
Copy link
Collaborator

@C4illin C4illin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
});

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And more probably more things with some other special characters https://www.npmjs.com/package/sanitize-filename

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we make sure to normalize the path first we can at least get rid of the dots in the middle problem

Copy link
Collaborator

@C4illin C4illin Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even when allowing it, I didn't manage to escape the correct folder in deleteFile (which doesn't use normalize)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Folders & subfolders

2 participants