Skip to content

Loading…

fix invalid characters in multipart uploads #515

Merged
merged 1 commit into from

6 participants

@mraidel

Fixes invalid characters in multipart uploads by removing all characters which are not valid. Otherwise uploads with invalid characters raise an exception (ArgumentError: invalid byte sequence in UTF-8)

@mraidel

travis build fails because of missing bacon gem for rubinius so it has nothing to do with the pull request

@oscardelben oscardelben commented on an outdated diff
lib/rack/multipart/parser.rb
@@ -137,6 +137,9 @@ def get_filename(head)
if filename && filename.scan(/%.?.?/).all? { |s| s =~ /%[0-9a-fA-F]{2}/ }
filename = Utils.unescape(filename)
end
+ if filename && String.instance_methods.include?(:valid_encoding?)

You can use respond_to, but can you comment on what happens on versions of Ruby prior to 1.9?

@mraidel
mraidel added a note

on ruby 1.8.7 this does nothing which is fine because the problem only exists since multibyte support in ruby 1.9

@rkh Official Rack repositories member
rkh added a note

You can use respond_to

or method_defined?

@mraidel
mraidel added a note

yes, method_defined? sounds good. respond_to? would work here too but it doesn't feel right to me because we would test on another instance of string, not the one we are using the method on (as we are using chars which returns new instances of String). Have pushed a fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@etehtsea

What is the problem with merging this PR?

@rkh
Official Rack repositories member
rkh commented

No one reviewed it yet.

@jeremyevans

I had the same problem on one of my sites. I tested this commit and it fixes the error. However, I think the implementation is suboptimal from a performance standpoint. It should only do the work of filtering the string if it is not already in a valid encoding, since checking a string for a valid encoding is very fast, rebuilding is slow, and the vast majority of filenames are already in a valid encoding. To the new if statement, you should append && !filename.valid_encoding?. This should be be about 2.5x faster for a single byte filename, 7x faster for 10 byte filename, 35x faster for a 100 byte filename, and close to 300x faster for a 1KB filename.

With that change I think this should go in.

@spastorino
Official Rack repositories member

:+1: to what @jeremyevans said. So @mraidel can you do that modification and squash your commits?

@spastorino
Official Rack repositories member

And also your test https://travis-ci.org/rack/rack/jobs/4745776 fail in Ruby 2.0

@spastorino
Official Rack repositories member

@mraidel You will need to properly encode the String in the test

@mraidel

@jeremyevans @spastorino I have included both recommended changes and squashed the commits!

@spastorino
Official Rack repositories member

@raggi @rkh :+1: good to merge

@rkh rkh commented on the diff
lib/rack/multipart/parser.rb
@@ -137,6 +137,9 @@ def get_filename(head)
if filename && filename.scan(/%.?.?/).all? { |s| s =~ /%[0-9a-fA-F]{2}/ }
filename = Utils.unescape(filename)
end
+ if filename && String.method_defined?(:valid_encoding?) && !filename.valid_encoding?
+ filename = filename.chars.select { |char| char.valid_encoding? }.join
+ end
@rkh Official Rack repositories member
rkh added a note

On 2.1, we could use String#scrub, btw.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rkh rkh merged commit a811f12 into rack:master

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 20, 2013
  1. @mraidel
Showing with 23 additions and 0 deletions.
  1. +3 −0 lib/rack/multipart/parser.rb
  2. +6 −0 test/multipart/invalid_character
  3. +14 −0 test/spec_multipart.rb
View
3 lib/rack/multipart/parser.rb
@@ -137,6 +137,9 @@ def get_filename(head)
if filename && filename.scan(/%.?.?/).all? { |s| s =~ /%[0-9a-fA-F]{2}/ }
filename = Utils.unescape(filename)
end
+ if filename && String.method_defined?(:valid_encoding?) && !filename.valid_encoding?
+ filename = filename.chars.select { |char| char.valid_encoding? }.join
+ end
@rkh Official Rack repositories member
rkh added a note

On 2.1, we could use String#scrub, btw.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
if filename && filename !~ /\\[^\\"]/
filename = filename.gsub(/\\(.)/, '\1')
end
View
6 test/multipart/invalid_character
@@ -0,0 +1,6 @@
+--AaB03x
+Content-Disposition: form-data; name="files"; filename="invalid�.txt"
+Content-Type: text/plain
+
+contents
+--AaB03x--
View
14 test/spec_multipart.rb
@@ -166,6 +166,20 @@ def rd.length
params["files"][:tempfile].read.should.equal "contents"
end
+ should "parse multipart upload with filename with invalid characters" do
+ env = Rack::MockRequest.env_for("/", multipart_fixture(:invalid_character))
+ params = Rack::Multipart.parse_multipart(env)
+ params["files"][:type].should.equal "text/plain"
+ params["files"][:filename].should.match(/invalid/)
+ head = "Content-Disposition: form-data; " +
+ "name=\"files\"; filename=\"invalid\xC3.txt\"\r\n" +
+ "Content-Type: text/plain\r\n"
+ head = head.force_encoding("ASCII-8BIT") if head.respond_to?(:force_encoding)
+ params["files"][:head].should.equal head
+ params["files"][:name].should.equal "files"
+ params["files"][:tempfile].read.should.equal "contents"
+ end
+
should "not include file params if no file was selected" do
env = Rack::MockRequest.env_for("/", multipart_fixture(:none))
params = Rack::Multipart.parse_multipart(env)
Something went wrong with that request. Please try again.