Add filename to Content-Disposition header for DAM downloads - #6338
Add filename to Content-Disposition header for DAM downloads#6338VPS-Obi wants to merge 1 commit into
Conversation
The download endpoints sent a bare `attachment`, so browsers fell back to the URL for the name of the saved file. That URL only contains the file name without its extension, leaving downloads without one. Use `res.attachment()` instead of building the header by hand: file names are not slugified when a file is renamed, so they can contain quotes or non-latin1 characters, which need escaping and RFC 5987 encoding. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CxUhjh9AAnB6AHvGopkdCu
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@nsams do we need the |
| } | ||
|
|
||
| res.setHeader("Content-Disposition", "attachment"); | ||
| res.attachment(file.name); |
There was a problem hiding this comment.
this does also set mime type (see https://github.com/expressjs/express/blob/bed501c695a61886399ee622875f3be933c716d8/lib/response.js#L609), based on the file name. (which you noted in the description)
I'm not sure we should do that; doesn't it set the header twice? (two values)
There was a problem hiding this comment.
Tried this locally, it doesnt:
Alternatively, we could use the content-disposition package directly. I'd prefer to not roll our own encoding/escaping.
There was a problem hiding this comment.
can you verify using curl, so we can see the raw output?
Alternatively, we could use the content-disposition package directly.
yes, maybe better. is that what express uses internally?
yes, together with inline that might work. Needs proper testing in all browser we support... |
3 participants
The header was previously set to
attachmentwithout a file name, so browsers derived the name of the downloaded file from the URL, which contains the file name without its extension.Uses Express' res.attachment instead of building the header by hand, which does proper escaping and RFC 5987 encoding. Note: this also sets the
Content-Type-header based on the file extension, but this is overridden in streamFile.https://claude.ai/code/session_01CxUhjh9AAnB6AHvGopkdCu