From 3fc561a1f71edf1c2bae695cafa03909d24a5ca3 Mon Sep 17 00:00:00 2001
From: Andrew White <andyw@pixeltrix.co.uk>
Date: Sun, 20 May 2012 16:44:42 +0100
Subject: [PATCH] Return 400 Bad Request for URL paths with invalid encoding.

Passing path parameters with invalid encoding is likely to trigger errors
further on like `ArgumentError (invalid byte sequence in UTF-8)`. This will
result in a 500 error whereas the better error to return is a 400 error which
allows exception notification libraries to filter it out if they wish.

Closes #4450
---
 actionpack/CHANGELOG.md                       |  2 ++
 .../action_dispatch/routing/redirection.rb    |  9 ++++++
 .../lib/action_dispatch/routing/route_set.rb  |  9 ++++++
 actionpack/test/dispatch/routing_test.rb      | 31 +++++++++++++++++++
 4 files changed, 51 insertions(+)

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 5d4ae16c09..94b3490b80 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,5 +1,7 @@
 ## Rails 4.0.0 (unreleased) ##
 
+*   URL path parameters with invalid encoding now raise ActionController::BadRequest. *Andrew White*
+
 *   Malformed query and request parameter hashes now raise ActionController::BadRequest. *Andrew White*
 
 *   Add `divider` option to `grouped_options_for_select` to generate a separator
diff --git a/actionpack/lib/action_dispatch/routing/redirection.rb b/actionpack/lib/action_dispatch/routing/redirection.rb
index b3823bb496..205ff44b1c 100644
--- a/actionpack/lib/action_dispatch/routing/redirection.rb
+++ b/actionpack/lib/action_dispatch/routing/redirection.rb
@@ -2,6 +2,7 @@
 require 'active_support/core_ext/uri'
 require 'active_support/core_ext/array/extract_options'
 require 'rack/utils'
+require 'action_controller/metal/exceptions'
 
 module ActionDispatch
   module Routing
@@ -16,6 +17,14 @@ def initialize(status, block)
       def call(env)
         req = Request.new(env)
 
+        # If any of the path parameters has a invalid encoding then
+        # raise since it's likely to trigger errors further on.
+        req.symbolized_path_parameters.each do |key, value|
+          unless value.valid_encoding?
+            raise ActionController::BadRequest, "Invalid parameter: #{key} => #{value}"
+          end
+        end
+
         uri = URI.parse(path(req.symbolized_path_parameters, req))
         uri.scheme ||= req.scheme
         uri.host   ||= req.host
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index 0ae668d42a..1d0a67d0d2 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -26,6 +26,15 @@ def initialize(options={})
 
         def call(env)
           params = env[PARAMETERS_KEY]
+
+          # If any of the path parameters has a invalid encoding then
+          # raise since it's likely to trigger errors further on.
+          params.each do |key, value|
+            unless value.valid_encoding?
+              raise ActionController::BadRequest, "Invalid parameter: #{key} => #{value}"
+            end
+          end
+
           prepare_params!(params)
 
           # Just raise undefined constant errors if a controller was specified as default.
diff --git a/actionpack/test/dispatch/routing_test.rb b/actionpack/test/dispatch/routing_test.rb
index 1a8f40037f..00d09282ca 100644
--- a/actionpack/test/dispatch/routing_test.rb
+++ b/actionpack/test/dispatch/routing_test.rb
@@ -2697,3 +2697,34 @@ def app; Routes end
     assert_response :success
   end
 end
+
+class TestInvalidUrls < ActionDispatch::IntegrationTest
+  class FooController < ActionController::Base
+    def show
+      render :text => "foo#show"
+    end
+  end
+
+  test "invalid UTF-8 encoding returns a 400 Bad Request" do
+    with_routing do |set|
+      set.draw do
+        get "/bar/:id", :to => redirect("/foo/show/%{id}")
+        get "/foo/show(/:id)", :to => "test_invalid_urls/foo#show"
+        get "/foo(/:action(/:id))", :to => "test_invalid_urls/foo"
+        get "/:controller(/:action(/:id))"
+      end
+
+      get "/%E2%EF%BF%BD%A6"
+      assert_response :bad_request
+
+      get "/foo/%E2%EF%BF%BD%A6"
+      assert_response :bad_request
+
+      get "/foo/show/%E2%EF%BF%BD%A6"
+      assert_response :bad_request
+
+      get "/bar/%E2%EF%BF%BD%A6"
+      assert_response :bad_request
+    end
+  end
+end
\ No newline at end of file
-- 
GitLab