diff --git a/get_test.go b/get_test.go index 7739fb1..ef7f208 100644 --- a/get_test.go +++ b/get_test.go @@ -36,6 +36,7 @@ func TestGet(t *testing.T) { Size: 20, MetaData: map[string]string{ "filename": "file.jpg\"evil", + "filetype": "image/jpeg", }, }, nil), store.EXPECT().GetReader("yes").Return(reader, nil), @@ -56,8 +57,8 @@ func TestGet(t *testing.T) { URL: "yes", ResHeader: map[string]string{ "Content-Length": "5", - "Content-Type": "application/octet-stream", - "Content-Disposition": `attachment;filename="file.jpg\"evil"`, + "Content-Type": "image/jpeg", + "Content-Disposition": `inline;filename="file.jpg\"evil"`, }, Code: http.StatusOK, ResBody: "hello", @@ -103,4 +104,55 @@ func TestGet(t *testing.T) { Code: http.StatusNotImplemented, }).Run(http.HandlerFunc(handler.GetFile), t) }) + + SubTest(t, "InvalidFileType", func(t *testing.T, store *MockFullDataStore) { + store.EXPECT().GetInfo("yes").Return(FileInfo{ + Offset: 0, + MetaData: map[string]string{ + "filetype": "non-a-valid-mime-type", + }, + }, nil) + + handler, _ := NewHandler(Config{ + DataStore: store, + }) + + (&httpTest{ + Method: "GET", + URL: "yes", + ResHeader: map[string]string{ + "Content-Length": "0", + "Content-Type": "application/octet-stream", + "Content-Disposition": `attachment`, + }, + Code: http.StatusNoContent, + ResBody: "", + }).Run(handler, t) + }) + + SubTest(t, "NotWhitelistedFileType", func(t *testing.T, store *MockFullDataStore) { + store.EXPECT().GetInfo("yes").Return(FileInfo{ + Offset: 0, + MetaData: map[string]string{ + "filetype": "text/html", + "filename": "invoice.html", + }, + }, nil) + + handler, _ := NewHandler(Config{ + DataStore: store, + }) + + (&httpTest{ + Method: "GET", + URL: "yes", + ResHeader: map[string]string{ + "Content-Length": "0", + "Content-Type": "text/html", + "Content-Disposition": `attachment;filename="invoice.html"`, + }, + Code: http.StatusNoContent, + ResBody: "", + }).Run(handler, t) + }) } diff --git a/unrouted_handler.go b/unrouted_handler.go index 8c6e286..d36d73d 100644 --- a/unrouted_handler.go +++ b/unrouted_handler.go @@ -19,6 +19,7 @@ var ( reExtractFileID = regexp.MustCompile(`([^/]+)\/?$`) reForwardedHost = regexp.MustCompile(`host=([^,]+)`) reForwardedProto = regexp.MustCompile(`proto=(https?)`) + reMimeType = regexp.MustCompile(`^[a-z]+\/[a-z\-]+$`) ) // HTTPError represents an error with an additional status code attached @@ -542,19 +543,10 @@ func (handler *UnroutedHandler) GetFile(w http.ResponseWriter, r *http.Request) // Set headers before sending responses w.Header().Set("Content-Length", strconv.FormatInt(info.Offset, 10)) - w.Header().Set("Content-Type", "application/octet-stream") - // Force browsers to download the file instead of displaying it inline. - // Otherwise someone could upload malicious HTML which would then be - // executed if a victim visits this URL. - // See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition - // TODO: Consider lifting this limitation and allow e.g. images to be shown - // TODO: Consider pulling Content-Type from meta data - if filename, ok := info.MetaData["filename"]; ok { - w.Header().Set("Content-Disposition", "attachment;filename="+strconv.Quote(filename)) - } else { - w.Header().Set("Content-Disposition", "attachment") - } + contentType, contentDisposition := filterContentType(info) + w.Header().Set("Content-Type", contentType) + w.Header().Set("Content-Disposition", contentDisposition) // If no data has been uploaded yet, respond with an empty "204 No Content" status. if info.Offset == 0 { @@ -577,6 +569,67 @@ func (handler *UnroutedHandler) GetFile(w http.ResponseWriter, r *http.Request) } } +// mimeInlineBrowserWhitelist is a map containing MIME types which should be +// allowed to be rendered by browser inline, instead of being forced to be +// downloadd. For example, HTML or SVG files are not allowed, since they may +// contain malicious JavaScript. In a similiar fashion PDF is not on this list +// as their parsers commonly contain vulnerabilities which can be exploited. +// The values of this map does not convei any meaning and are therefore just +// empty structs. +var mimeInlineBrowserWhitelist = map[string]struct{}{ + "text/plain": struct{}{}, + + "image/png": struct{}{}, + "image/jpeg": struct{}{}, + "image/gif": struct{}{}, + "image/bmp": struct{}{}, + "image/webp": struct{}{}, + + "audio/wave": struct{}{}, + "audio/wav": struct{}{}, + "audio/x-wav": struct{}{}, + "audio/x-pn-wav": struct{}{}, + "audio/webm": struct{}{}, + "video/webm": struct{}{}, + "audio/ogg": struct{}{}, + "video/ogg ": struct{}{}, + "application/ogg": struct{}{}, +} + +// 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 +} + // DelFile terminates an upload permanently. func (handler *UnroutedHandler) DelFile(w http.ResponseWriter, r *http.Request) { // Abort the request handling if the required interface is not implemented