commit 2fed610569530a0e274b1e3aec19d589a38e7e26
parent 5d61bbe0c19fe0888a48d60414e1dbbddc14c105
Author: Atte Kojo <atte.kojo@reaktor.fi>
Date: Fri, 21 Feb 2014 13:04:45 +0200
Fix attachment filename handling
Attachment#write_to_disk created its temporary file with a shell-escaped name.
Passing the path to this file to write_to_disk's block parameter and returning
it created a double-escape problem, which rendered attachments with e.g. spaces
in the filename unusable.
Fixed by removing the use of the original filename from the tempfile name. Also
removed the warnings about shell escaping the temporary file name.
Diffstat:
1 file changed, 18 insertions(+), 22 deletions(-)
diff --git a/lib/sup/message_chunks.rb b/lib/sup/message_chunks.rb
@@ -60,8 +60,6 @@ end
module Redwood
module Chunk
class Attachment
- ## please see note in write_to_disk on important usage
- ## of quotes to avoid remote command injection.
HookManager.register "mime-decode", <<EOS
Decodes a MIME attachment into text form. The text will be displayed
directly in Sup. For attachments that you wish to use a separate program
@@ -79,8 +77,6 @@ Return value:
EOS
- ## please see note in write_to_disk on important usage
- ## of quotes to avoid remote command injection.
HookManager.register "mime-view", <<EOS
Views a non-text MIME attachment. This hook allows you to run
third-party programs for attachments that require such a thing (e.g.
@@ -132,8 +128,6 @@ EOS
when /^text\/plain\b/
@raw_content
else
- ## please see note in write_to_disk on important usage
- ## of quotes to avoid remote command injection.
HookManager.run "mime-decode", :content_type => @content_type,
:filename => lambda { write_to_disk },
:charset => encoded_content.charset,
@@ -171,8 +165,6 @@ EOS
def initial_state; :open end
def viewable?; @lines.nil? end
def view_default! path
- ## please see note in write_to_disk on important usage
- ## of quotes to avoid remote command injection.
case RbConfig::CONFIG['arch']
when /darwin/
cmd = "open #{path}"
@@ -185,28 +177,32 @@ EOS
end
def view!
- ## please see note in write_to_disk on important usage
- ## of quotes to avoid remote command injection.
- write_to_disk do |file|
-
- @@view_tempfiles.push file # make sure the tempfile is not garbage collected before sup stops
-
+ write_to_disk do |path|
ret = HookManager.run "mime-view", :content_type => @content_type,
- :filename => file.path
- ret || view_default!(file.path)
+ :filename => path
+ ret || view_default!(path)
end
end
- ## note that the path returned from write_to_disk is
- ## Shellwords.escaped and is intended to be used without single
- ## or double quotes. the use of either opens sup up for remote
- ## code injection through the file name.
def write_to_disk
begin
- file = Tempfile.new(["sup", Shellwords.escape(@filename.gsub("/", "_")) || "sup-attachment"])
+ # Add the original extension to the generated tempfile name only if the
+ # extension is "safe" (won't be interpreted by the shell). Since
+ # Tempfile.new always generates safe file names this should prevent
+ # attacking the user with funny attachment file names.
+ tempname = if (File.extname @filename) =~ /^\.[[:alnum:]]+$/ then
+ ["sup-attachment", File.extname(@filename)]
+ else
+ "sup-attachment"
+ end
+
+ file = Tempfile.new(tempname)
file.print @raw_content
file.flush
- yield file if block_given?
+
+ @@view_tempfiles.push file # make sure the tempfile is not garbage collected before sup stops
+
+ yield file.path if block_given?
return file.path
ensure
file.close