fix: Set content type for file downloads in viewer handler#570
fix: Set content type for file downloads in viewer handler#570rusq merged 3 commits intorusq:masterfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issue #569 by setting the Content-Type header for file downloads to support files without extensions, such as Canvas files. The change ensures that http.ServeFileFS serves these files correctly.
Key Changes
- Added Content-Type header set to "application/octet-stream" in the file handler to ensure proper serving of extensionless files
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/viewer/handlers.go
Outdated
| // Setting content type to application/octet-stream to support any files without extensions (e.g. Canvas files) | ||
| w.Header().Set("Content-Type", "application/octet-stream") |
There was a problem hiding this comment.
The Content-Type header is being set before request validation. If the validation fails (lines 217-220), the response will include this header even though it's a 404 error response. This could be confusing and is not semantically correct.
Consider moving the header setting to after validation and before calling http.ServeFileFS:
if id == "" || filename == "" || isInvalid(filename) || isInvalid(id) {
http.NotFound(w, r)
return
}
lg := v.lg.With("in", "fileHandler", "id", id, "filename", filename)
fsys := v.src.Files().FS()
path, err := v.src.Files().File(id, filename)
if err != nil {
if errors.Is(err, fs.ErrNotExist) {
http.NotFound(w, r)
return
}
lg.ErrorContext(ctx, "File", "error", err)
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
// Setting content type to application/octet-stream to support any files without extensions (e.g. Canvas files)
w.Header().Set("Content-Type", "application/octet-stream")
http.ServeFileFS(w, r, fsys, path)This ensures the Content-Type header is only set for successful file serving.
|
Hey @lbeckman314, thanks a lot for this! Don't worry about the tmp/guest failing test - it used to be a test that used [unpublished] files on my machine. It is not executed in the CI, and doesn't relate to this change, of course. |
Overview
Minor fix that resolves #569 by setting Content Header for all files to allow for successful serving of non-extension files (e.g Canvas files).
Tip
See Issue Comment for steps run and new behavior.
Limitations⚠️
1. Canvas Link
Canvases are only included in the
__uploadsdirectory in the dump file if a link to that canvas is included in the channel messages:Note
Without this link, the Canvas will be not included in the final dump file.
2. Files in the Canvas
Files uploaded to the Canvas are not downloaded and are only represented as
File URLlinks in the Canvas dump:__uploads/<Channel ID>/Example_Canvas:3. Test Failure
Caution
All tests passing except for Test_exportV3 (need to investigate further...):