escaped params silently removing files

[danleyden]

I observed, when using Sinatra (this may be found with other things - I don't know), that the escaped params handler is removing (setting to nil) any parameters that are not any of String, Array or Hash.

This is an issue when POSTing multipart forms containing files, which are added to the params as a hash, with one of the items within that hash being a TempFile, pointing to the temporary file containing the uploaded file's contents.

This results in the application being provided with a hash containing what should be the tempfile as nil, and so the uploaded content not being available to the application.

The change ee53d6a appears to have broken this functionality, but I am not convinced that the desirable solution would be to revert that commit.

The important change (in lib/rack/protection/escaped_params.rb) appears to be the change from the default behaviour of raising an ArgumentError when the type is unknown to just returning nil (below):

       def escape(object)
         case object
         when Hash   then escape_hash(object)
         when Array  then object.map { |o| escape(o) }
         when String then escape_string(object)
-        else raise ArgumentError, "cannot escape #{object.inspect}"
+        else nil
         end
       end

I believe that the correct behaviour desired for the use case that commit refers to should actually be to return nil if (and only if) object is nil, leaving the default case as it was before raising the ArgumentError.

That behaviour would, if I am reading the code correctly, have the negative effect of causing the escaping to fail in the case that any parameter is not a Hash, Array, String or nil, resulting in the whole of the params being fed eventually in to the application unescaped.

In my use case, it would be preferable to pass this unknown item (the tempfile) through, but any other parameters should be escaped. I could add an explicit handler for TempFile to support this specific case, but I fear that the behaviour of silently dropping parameters is (the line else nil) is very painful to debug.

The best solution I could come up with is:

      def escape(object)
        case object
        when Hash   then escape_hash(object)
        when Array  then object.map { |o| escape(o) }
        when String then escape_string(object)
+       when TempFile then object
+       when nil then nil
+       else raise ArgumentError, "cannot escape #{object.inspect}"
-       else nil
        end
      end

In this case the behaviour is to handle TempFiles explicitly (and not escape them), to handle nil values explicitly (again, not escape them) but in the case of something unhandled to raise an Error.

I suspect that this may well be undesirable for many people as it is a change in behaviour.

An alternative may be to support passing in a block that could be executed in the case of an unknown value / type to allow the developer to handle the cases explicitly for their app. I'm not sure what that would look like, though.

Am I missing something?

[rkh]

Completely agree on the Tempfile handling. I think the else branch with nil might be better though, maybe we should log a warning.

[adrianbn]

This is an interesting issue. From a security standpoint, I believe that the correct behaviour for TempFile objects should be:

• Sanitize/escape the file name and any other received metadata.
• Provide a hook for the user to define what sanitization (if any) is performed on the file contents. By default, no sanitization should occur on the file contents.
• Document the behaviour and advise users to provide a function to sanitize file contents before manipulating those unless they know what they are doing.

[Albert-IV]

Is there any update on this? Right now it seems like it isn't possible to accept file uploads if you use Rack::Protection, which seems like a kinda big issue, at least coming from the view that the module shouldn't break valid use-cases.

The PR that fixes this has a failing Travis test viewable here which had an error for the latest version of JRuby. Any insight on how to handle this for the Average Joe, outside of making a fork and maintaining it?