Skip to content
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

Sanitize filename before upload to prevent issues with spaces and special characters in filenames. #109

Merged
merged 2 commits into from
Dec 1, 2022

Conversation

larskarbo
Copy link
Contributor

@larskarbo larskarbo commented Nov 29, 2022

I noticed that uploads with spaces in the filename gave invalid urls.

shot-dGIGz1p0

The problem seems to be that req.body.filename in s3-upload.ts for some reason has filename spaces encoded with %20 instead of a space.

And when you upload to s3 with %20 in the url you have to add %2520 to get the actual url.

Returned by library: https://XXX/filename%20with-one-space.png
Actual working url (from aws console): https://XXX/filename%2520with-one-space.png

This PR does two things:

  1. Remove encodeURIComponent of the filename on the client side. Send the real, unencoded filename to the back-end.
  2. On the back-end, sanitize the filename and remove all characters not supported by s3.

@vercel
Copy link

vercel bot commented Nov 29, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
next-s3-upload ✅ Ready (Inspect) Visit Preview Nov 30, 2022 at 8:59PM (UTC)

@larskarbo larskarbo changed the title Escape %20 before uploading to prevent issues with spaces in filenames. Escape %20 before uploading to prevent issues with spaces in filenames. Nov 29, 2022
@ryanto
Copy link
Owner

ryanto commented Nov 30, 2022

Nice catch!

I wonder if it would make things easier if we didn't encode the filename here: https://github.com/ryanto/next-s3-upload/blob/master/packages/next-s3-upload/src/hooks/use-s3-upload.tsx#L99

That way it would be sent over with spaces, and then those spaces get replaced with -. No need for dealing with the %20's

What do you think?

@larskarbo
Copy link
Contributor Author

Actually, there are probably more characters that should be escaped, not only space.

Looking at the docs, there is a limited set of valid characters.

shot-wr0LwiNb

This library https://github.com/Advanon/sanitize-s3-objectkey seems stale but does what we need here.

So I think it's a good idea to skip the encodeURIComponent (like you suggested), but while we're at it, maybe consider sanitizing more heavily before uploading?

@ryanto
Copy link
Owner

ryanto commented Nov 30, 2022

100% Agree. 👍

I'd like to avoid having a dependency on that library if possible, also don't think we need the latin character remapping.

What do you think about something like this? https://codesandbox.io/s/laughing-murdock-w1xusx, trying to keep the sanitization as simple as possible here.

@larskarbo larskarbo changed the title Escape %20 before uploading to prevent issues with spaces in filenames. Sanitize filename before upload to prevent issues with spaces and special characters in filenames. Nov 30, 2022
@larskarbo
Copy link
Contributor Author

That's sleek @ryanto! I updated the PR with that solution. Let me know if you think it looks all right.

I think it makes sense to give the un-sanitized filename to the options.key function.

Tested locally with space and special chars. Works like a charm!

@ryanto
Copy link
Owner

ryanto commented Dec 1, 2022

Looks great!

@ryanto ryanto merged commit b66bced into ryanto:master Dec 1, 2022
@ryanto
Copy link
Owner

ryanto commented Dec 1, 2022

Ok published at 0.2.7

Thanks again for adding this!

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.

2 participants