proxy: Use Content-Disposition to generate meaningful filenames#8
proxy: Use Content-Disposition to generate meaningful filenames#8noctux wants to merge 1 commit into
Conversation
| let (id_str, content_disposition) = match id_str.split_once('/') { | ||
| Some((id, filename)) => ( | ||
| percent_decode(id), | ||
| format!("Content-Disposition: attachment; filename=\"{}\"\r\n", percent_decode(filename).replace(['/', '\\', '"', '\''], "_")) |
There was a problem hiding this comment.
Probably not a big deal in practice, but still worth closing. The caption comes from whoever sent the message, rides in through the url, and lands in this header unescaped. media_line cleans a couple chars then urlencoding::encode, so a CR/LF in the caption becomes %0D%0A, and percent_decode here turns it back. The replace only covers slash/backslash/quote/apostrophe, so the CR/LF makes it into the Content-Disposition line.
Say a sender captions an attachment hi\r\nX-Injected: yes. media_line encodes that to .../attach/$evt:srv/hi%0D%0AX-Injected%3A%20yes, and fetching it gives:
Content-Disposition: attachment; filename="hi
X-Injected: yes"
so they've added their own header. Push it to %0D%0A%0D%0A and they close the header block and write a body.
Stripping control chars before formatting is enough. RFC 6266 filename*=UTF-8''... is the tidier fix and also handles non-ascii captions, which aren't valid in a quoted filename anyway.
| attach_base: &str, | ||
| ) -> String { | ||
| let cleaned_caption = caption.replace(['/', '\\', '"'], "_"); | ||
| let suffix = encode(&cleaned_caption); |
There was a problem hiding this comment.
Couple of small things. If the caption's empty the url ends up as /attach// and you get filename="" at the other end, so maybe skip the suffix when it's empty. Also the set here (slash/backslash/quote) is missing the apostrophe the proxy side strips - bound to drift apart, might be nicer as one shared helper.
2 participants
This PR reconfigures the url-generation to include the caption of attachments into the
/attach/-urls.This then allows to set a suitable Content-Disposition Header so that downloaded attachments are not named by the eventid, but instead offer semantic sames.
N.B.: This PR reuses the existing percent_decode function, but this PR uses
urlencoding::encodefor the reverse route. As matrix-sdk already pullsurlencoding, it might be a sensible choice to instead reuseurlencoding::decodeinstead of handrolling that decoding unless there were good reasons to handroll that one.