"GET %2Ffavicon.ico HTTP/1.1" in log

[znz]

How to reproduce:

1. rails new foo (I use rails 4.0.0)
2. edit Gemfile and enable unicorn gem
3. run bundle exec unicorn
4. open http://localhost:8080/ with browser.

then I found "GET %2Ffavicon.ico HTTP/1.1" in log messages.
I expected "GET /favicon.ico HTTP/1.1".

In actionpack-4.0.0/lib/action_dispatch/middleware/static.rb,
match = @file_handler.match?(path) returns escaped path,
and overwrite env["PATH_INFO"].
It seems to affect logger.

[steveklabnik]

I can confirm that this happens with Unicorn and Puma. It doesn't even seem to show up with bin/rails s, though....

[valk]

With Unicorn I could reproduce the bug. However it shows up on the first request with curl http://localhost:8080. Afterwards, only after server restart it shows up again (but not always, looks like Unicorn is somehow caching the logger output...)

[arthurnn]

So the issue in here is not on unicorn or puma. We only can see the symptoms of it happening on those because they enable the Rack::CommonLogger middleware, see unicorn https://github.com/defunkt/unicorn/blob/master/lib/unicorn.rb#L67 . IMO, the problem is on rack, see my PR on rack that should fix this.

[jeremy]

This is a bug in AD::Static. It's wrongly escaping the PATH_INFO:

• Set here https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/static.rb#L112-L113
• After matching file path and escaping it here https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/static.rb#L34

[jeremy]

Caused by d07b2f3

Which appears to be a workaround for incorrectly unescaping PATH_INFO using Rack::Utils.unescape instead of unescape_path (or using URI).

This was addressed the following day in ceb288b, so perhaps the original commit can be corrected now.

[jeremy]

Indeed, Rack::File is still unescaping PATH_INFO incorrectly.

From 2012: rack/rack#265 (comment)

[jeremy]

Rack

diff --git a/lib/rack/file.rb b/lib/rack/file.rb
index c8f8d0d..e1bf12b 100644
--- a/lib/rack/file.rb
+++ b/lib/rack/file.rb
@@ -38,7 +38,8 @@ module Rack
         return fail(405, "Method Not Allowed", {'Allow' => ALLOW_HEADER})
       end

-      path_info = Utils.unescape(env["PATH_INFO"])
+      # FIXME: unescape as URI path, not as query component
+      path_info = env['rack.file.unescaped_path_info'] || Utils.unescape(env["PATH_INFO"])
       clean_path_info = Utils.clean_path_info(path_info)

       @path = F.join(@root, clean_path_info)

Rails

diff --git a/actionpack/lib/action_dispatch/middleware/static.rb b/actionpack/lib/action_dispatch/middleware/static.rb
index e66c21e..03ce68f 100644
--- a/actionpack/lib/action_dispatch/middleware/static.rb
+++ b/actionpack/lib/action_dispatch/middleware/static.rb
@@ -21,14 +21,11 @@ module ActionDispatch
     end

     def match?(path)
-      path = URI.parser.unescape(path)
+      path = Journey::Router::Utils.unescape_uri(path)
       return false unless path.valid_encoding?

       paths = [path, "#{path}#{ext}", "#{path}/index#{ext}"]
-
-      if match = paths.detect {|p| File.file?(File.join(@root, p)) }
-        return ::Rack::Utils.escape(match)
-      end
+      paths.detect {|p| File.file?(File.join(@root, p)) }
     end

     def call(env)
@@ -36,7 +33,8 @@ module ActionDispatch
       gzip_path = gzip_file_path(path)

       if gzip_path && gzip_encoding_accepted?(env)
-        env['PATH_INFO']            = gzip_path
+        env["rack.file.unescaped_path_info"] = gzip_path
+        env["PATH_INFO"] = Journey::Router::Utils.escape_path(gzip_path)
         status, headers, body       = @file_server.call(env)
         headers['Content-Encoding'] = 'gzip'
         headers['Content-Type']     = content_type(path)
@@ -67,7 +65,7 @@ module ActionDispatch
       def gzip_file_path(path)
         can_gzip_mime = content_type(path) =~ /\A(?:text\/|application\/javascript)/
         gzip_path     = "#{path}.gz"
-        if can_gzip_mime && File.exist?(File.join(@root, ::Rack::Utils.unescape(gzip_path)))
+        if can_gzip_mime && File.exist?(File.join(@root, Journey::Router::Utils.unescape_uri(gzip_path)))
           gzip_path
         else
           false
@@ -95,7 +93,8 @@ module ActionDispatch
       when 'GET', 'HEAD'
         path = env['PATH_INFO'].chomp('/')
         if match = @file_handler.match?(path)
-          env["PATH_INFO"] = match
+          env["rack.file.unescaped_path_info"] = match
+          env["PATH_INFO"] = Journey::Router::Utils.escape_path(match)
           return @file_handler.call(env)
         end
       end

[rails-bot]

This issue has been automatically marked as stale because it has not been commented on for at least
three months.

The resources of the Rails team are limited, and so we are asking for your help.

If you can still reproduce this error on the 4-2-stable, 4-1-stable branches or on master,
please reply with all of the information you have about it in order to keep the issue open.

Thank you for all your contributions.

[tenderlove]

@jeremy the ruby-lang bug you linked to in rack/rack#265 is broken. Are you able to find the right URL? Since Rack master is 2.0, I'd like to fix this The Right Way™

[matthewd]

@tenderlove see http://blade.nagaokaut.ac.jp/cgi-bin/scat.rb/ruby/ruby-core/24014 and surrounds.

AFAICS, it's a bug report complaining that URI.encode works the way @jeremy is wants (+ means +), while the reporter expected it to work the way Rack currently does (+ means ), and @shyouhei pointing out that the current behaviour matches the RFC.

That matches my understanding too:

Within the query string, the plus sign is reserved as shorthand notation for a space. Therefore, real plus signs must be encoded.

(emphasis mine, but the heading's hardly subtle in the original)

RFC 3986 ascribes no special interpretation to +. In fact, it carries an example to the contrary: in tel:+1-816-555-1212, + clearly means +.

[tenderlove]

@matthewd So we should really only be dealing with + encoding in the query string, and not the path.

Shouldn't we just change Rack to use URI unescape vs form unescape? It looks like Rack is using form unescape rather than URI escape:

irb(main):008:0> Rack::Utils.unescape("/foo+bar")
=> "/foo bar"
irb(main):009:0> URI.decode_www_form_component("/foo+bar")
=> "/foo bar"
irb(main):010:0> URI::Parser.new.unescape "/foo+bar"
=> "/foo+bar"
irb(main):011:0> 

As a side note, this comment seems dubious / wrong.

It seems like we have an escape and an unescape (for query parameters and form components) and an escape_path for path components, but no unescape_path. Does this patch seem reasonable?

diff --git a/lib/rack/file.rb b/lib/rack/file.rb
index 1efa15a..5b755f5 100644
--- a/lib/rack/file.rb
+++ b/lib/rack/file.rb
@@ -30,7 +30,7 @@ module Rack
         return fail(405, "Method Not Allowed", {'Allow' => ALLOW_HEADER})
       end

-      path_info = Utils.unescape request.path_info
+      path_info = Utils.unescape_path request.path_info
       clean_path_info = Utils.clean_path_info(path_info)

       path = ::File.join(@root, clean_path_info)
diff --git a/lib/rack/utils.rb b/lib/rack/utils.rb
index 184bf5a..8911445 100644
--- a/lib/rack/utils.rb
+++ b/lib/rack/utils.rb
@@ -19,6 +19,8 @@ module Rack
     COMMON_SEP = QueryParser::COMMON_SEP
     KeySpaceConstrainedParams = QueryParser::Params

+    URI_PARSER = URI::Parser.new
+
     class << self
       attr_accessor :default_query_parser
     end
@@ -35,10 +37,17 @@ module Rack
     # Like URI escaping, but with %20 instead of +. Strictly speaking this is
     # true URI escaping.
     def escape_path(s)
-      escape(s).gsub('+', '%20')
+      URI_PARSER.escape s
     end
     module_function :escape_path

+    # Unescapes the **path** component of a URI.  See Rack::Utils.unescape for
+    # unescaping query parameters or form components.
+    def unescape_path(s)
+      URI_PARSER.unescape s
+    end
+    module_function :unescape_path
+
     # Unescapes a URI escaped string with +encoding+. +encoding+ will be the
     # target encoding of the string returned, and it defaults to UTF-8
     def unescape(s, encoding = Encoding::UTF_8)

/cc @jeremy

[tenderlove]

Fixed in e25fdad