Use original filename in content-disposition header

clbn commented 3 years ago

For some file formats, the server stored files to S3 without the original extension in content-disposition, so the files downloaded without the extension.

Before:

content-disposition: attachment; filename="IMG_8126"; filename*=utf-8''IMG_8126

After:

content-disposition: attachment; filename="IMG_8126.MOV"; filename*=utf-8''IMG_8126.MOV

clbn commented 3 years ago

It seems like simple change but I wasn't sure if it's good enough, or if it introduces side effects I don't see.

After some debugging, I spotted the culprit in this line:

const dispositionName = parsePath(this.fileName).name + parsePath(destPath).ext;

It used the original filename for name part, but destPath for ext part, and the latter (built using getFilename()) only consists of attachment ID when it comes to non-whitelisted file formats, so there's no extension for those.

Making changes in getFilename() looked like a lot, so my solution here is to just replace destPath with this.fileName:

const dispositionName = parsePath(this.fileName).name + parsePath(this.fileName).ext;

Which was then refactored to what you can see in the commit:

const { name, ext } = parsePath(this.fileName);  
const dispositionName = name + ext;

Now, it seems it can be further simplified to:

const dispositionName = this.fileName;

But there might be security concerns, I guess? Please let me know if you think it's actually safe to do it this way.

n1313 commented 3 years ago

As long as I can't upload an *.html file with <html><script>alert(1)</script> inside and get it executed in my browser by clicking on the attachment link, I'm OK with this. I think content-disposition: attachment; takes care of this, so adding a correct file extension shouldn't be a problem.

davidmz commented 3 years ago

Хорошо бы тестов добавить, для порядка. Там для S3 сейчас есть некоторый mock, см. тут например.

clbn commented 3 years ago

@n1313 Yes, attachment should work fine for that. My question though is if this code:

const { name, ext } = parsePath(this.fileName);  
const dispositionName = name + ext;

provides any additional safeguards in comparison to this:

const dispositionName = this.fileName;

(Meaning, if some tricks with the filename are possible on the upload, that would compromise security when the readers download the file on their devices. And if the former code somehow guards from that imaginary threat.)

clbn commented 3 years ago

@davidmz Хорошо, добавлю.

FreeFeed / freefeed-server

Use original filename in content-disposition header #540