search

sup-heliotrope / sup

A curses threads-with-tags style email client (mailing list: supmua@googlegroups.com)
http://sup-heliotrope.github.io
GNU General Public License v2.0
900 stars 97 forks source link

fix attachment escaping #528

Closed ninewise closed 4 years ago

ninewise commented 7 years ago

Closes #505

gauteh commented 7 years ago

Make sure this doesn't open for this kind of attack: https://github.com/sup-heliotrope/sup/wiki/Security-%23SBU1 - it does not immediately seem like a problem.

ninewise commented 7 years ago

I was looking around a bit, and apparently I reversed some earlier commit to allow saving attachments with slashes. Fixed that.

mklinik commented 6 years ago

chunk.safe_filename is the attachment's shell-escaped filename where slashes are replaced by underscores. You only need to shell-escape a filename if you pass it to a shell-like command interpreter, as in chunk.view_default. There, the filename is eventually passed to system. But when the filename is used directly to open the file, we don't need to escape it. I think it is a bug to use chunk.safe_filename for saving the attachment. Using just chunk.filename should be fine.

I think @NoctuaNivalis' commits can safely be merged.

danc86 commented 4 years ago

This fix is good, but for some reason when I restart the Travis build it insists on using an old, broken build config instead of merging with develop (where the build is now fixed). I'm not sure what's going on there.

So I've squashed these two commits and re-posted them with a clearer commit message in PR #558. I'm closing this PR in favour of the other one.

Thanks very much for your contribution!

ninewise commented 4 years ago

I didn't even remember this, but thank you for merging this into master.