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

feat: add private method to get expected content type #210

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

anonymousRecords
Copy link

What kind of change does this PR introduce?

Related to issue: supabase/supabase-js#1248

This PR expands MIME type support in the _getExpectedContentType function and adds MIME type validation during upload.

What is the current behavior?

createSignedUploadUrl does not validate MIME types.

What is the new behavior?

  • Added more MIME types to the _getExpectedContentType function for broader file type support.
  • Implemented a check to compare the expected MIME type with the actual Content-Type during upload, throwing an error if they don't match.

Additional context

  • The selection of additional MIME types was done arbitrarily. If support for more file types is needed, I'm open to suggestions and can expand the list accordingly.
  • Currently, the function throws a StorageError when the content type doesn't match. I'm seeking feedback on whether using console.warn for a warning instead would be more appropriate.

Comment on lines +226 to +230
if (headers['content-type'] !== expectedContentType) {
throw new StorageError(
`Content-type mismatch. Expected: "${expectedContentType}", but received: "${headers['content-type']}" for file "${path}"`
)
}
Copy link

@saqibameen saqibameen Sep 29, 2024

Choose a reason for hiding this comment

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

Wouldn't this lead to unwanted errors because expectedContentType is defaulting to application/octet-stream?

E.g., for .xls file, the mime type is application/vnd.ms-excel, but because of how _getExpectedContentType(...) works, it will return application/octet-stream and headers['content-type'] will be application/vnd.ms-excel

Copy link
Author

Choose a reason for hiding this comment

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

I've modified the code to address the issue you pointed out. The _getExpectedContentType function has been updated as follows:

  • For known extensions, it continues to return the corresponding MIME type as before.
  • For unknown extensions, it now returns an empty string ('') instead of 'application/octet-stream'.

This change prevents unnecessary errors for unknown file types like .xls, and allows the system to use the content-type provided by the user. Could you please review this change and confirm if it adequately addresses the concern you raised?

Copy link

@saqibameen saqibameen Oct 1, 2024

Choose a reason for hiding this comment

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

Thank you! It does address the concern that I raised. However, I feel this method is not scalable and is only limited to handling the specified types e.g., .png, .jpg, .jpeg, .gif, .pdf, and .md. This seems very limiting.

Solutions that I'd expect:

  1. Get the mime type of the file using something like mime-types pkg. So it is not limited to the specified types. Alternatively, we can ask users to pass it while creating signedUploadUrl. Then just match the type when user uploads.
  2. Even better, detect the mime type from the incoming buffer, just like we do on frontend file.type. Not sure if that is possible, then match it.

Just thinking out loud here. Would be happy to contribute in any way.

Copy link
Author

Choose a reason for hiding this comment

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

I have modified the code to use the suggested mime-types package for content type verification, allowing support for a wider range of file types.

While considering the option of detecting MIME types directly from file buffers, I decided to adopt the mime-types package approach for now, as accurately detecting all file types from buffers could be challenging.

If any ideas come up for implementing buffer-based MIME type detection in the future, I'll certainly explore that approach as well. Any additional thoughts or suggestions are welcome 😊

Choose a reason for hiding this comment

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

This looks good to me. Thank you.

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