Skip to content

Conversation

HeinrichvonStein
Copy link

@HeinrichvonStein HeinrichvonStein commented Jan 29, 2025

This PR addresses security concerns raised by @rkistner in #44 (comment)

The main update is using Supabase Edge Functions to obtain a signed URL that can be used by the client to upload and download files.

Comment on lines 89 to 90
const { S3Region, S3BucketName, fileName, mediaType, expiresIn } = await req.json();
if (!S3Region || !S3BucketName || !fileName || !mediaType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Region and bucket name should be configured on the server (edge function), and not the client. Only fileName and mediaType should be sent from the client.

The same comments apply for all the calls (upload, download and delete).

Even the filename has potential security implications, such as allowing an user to overwrite another user's files by passing in the same filename. So often we use an approach such as generating a random prefix/folder for the file. It's probably okay to exclude that from the demo, since the user can read or delete any file anyway, but maybe add a note that the developer should look into that for production.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the feedback @rkistner. Will make the relevant changes.

Didn't think about the use-case of a user being able to overwrite someone else's files but wouldn't the sync rules negate that possibility as only the user's data would be available to them?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm imaging a case where you send me a link to an image in Slack. I then have the filename, which is enough to delete or overwrite it. Or depending on how the app is structured, it may be possible to guess/brute force what other filenames could be.

Either way, to handle those properly requires an entire access control system in the edge function, which I think is out of scope for this guide.

Copy link
Author

Choose a reason for hiding this comment

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

Ah that makes sense 👍

Copy link
Contributor

@rkistner rkistner left a comment

Choose a reason for hiding this comment

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

Happy overall, just had some minor comments

@michaelbarnes michaelbarnes merged commit e6a8486 into docs Jan 30, 2025
2 checks passed
@michaelbarnes michaelbarnes deleted the fix/aws-s3-tutorial branch January 30, 2025 16:33
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.

3 participants