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(s3store): pass the contentType or filetype metadata field to S3 if supplied #1217

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

Conversation

mackinleysmith
Copy link

After this PR, if the client passes a contentType value in the metadata object, the resulting file on S3 will be created with the value passed.

This change causes tusd for Go to match the NodeJS version's behavior, discussed here: https://stackoverflow.com/questions/74148196/how-to-resolve-application-octet-stream-in-s3-using-tus-node-tusd-uppy-or-net.

@Acconut
Copy link
Member

Acconut commented Nov 21, 2024

Thank you for the PR! tusd currently uses the filetype metadata field to set the Content-Type and Content-Disposition headers in GET responses:

// filterContentType returns the values for the Content-Type and
// Content-Disposition headers for a given upload. These values should be used
// in responses for GET requests to ensure that only non-malicious file types
// are shown directly in the browser. It will extract the file name and type
// from the "fileame" and "filetype".
// See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition
func filterContentType(info FileInfo) (contentType string, contentDisposition string) {
filetype := info.MetaData["filetype"]
if reMimeType.MatchString(filetype) {
// If the filetype from metadata is well formed, we forward use this
// for the Content-Type header. However, only whitelisted mime types
// will be allowed to be shown inline in the browser
contentType = filetype
if _, isWhitelisted := mimeInlineBrowserWhitelist[filetype]; isWhitelisted {
contentDisposition = "inline"
} else {
contentDisposition = "attachment"
}
} else {
// If the filetype from the metadata is not well formed, we use a
// default type and force the browser to download the content.
contentType = "application/octet-stream"
contentDisposition = "attachment"
}
// Add a filename to Content-Disposition if one is available in the metadata
if filename, ok := info.MetaData["filename"]; ok {
contentDisposition += ";filename=" + strconv.Quote(filename)
}
return contentType, contentDisposition
}

The contentType field is not yet used in tusd. Although it would be handy to be compatible with tus-node-server here, I think it's more important to be consistent in tusd itself. Would you mind using the filetype metadata field for setting the Content-Type on S3 as well?

@mackinleysmith
Copy link
Author

@Acconut absolutely!! I'll add handling for the filetype field later today. Thank you for reviewing!

@mackinleysmith
Copy link
Author

@Acconut I have updated this PR with handling for the filetype metadata field and added another test. Thanks for the suggestion.

@mackinleysmith mackinleysmith changed the title feat(s3store): pass the contentType metadata field to S3 if supplied feat(s3store): pass the contentType or filetype metadata field to S3 if supplied Nov 21, 2024
@Acconut
Copy link
Member

Acconut commented Nov 22, 2024

Thanks for the update! Apologies if my comment was not clear enough but I think we should only use filetype for obtaining a media type and not use contentType at all. Compatibility with tus-node-server would be nice, but then tusd uses different metadata fields for different purposes, which is not good.

@mackinleysmith
Copy link
Author

Ok @Acconut, while I personally would prefer that publicly posted solutions for one Tus implementation would work against them all, my preference to get something merged that handles my current need to overwrite the file in S3 post-upload is greater. I will make that adjustment so that contentType is not used.

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