Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Comparing changes

Choose two branches to see what's changed or to start a new pull request. If you need to, you can also compare across forks.

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also compare across forks.
base fork: arunagw/rails
...
head fork: arunagw/rails
Checking mergeability… Don't worry, you can still create the pull request.
  • 10 commits
  • 22 files changed
  • 0 commit comments
  • 6 contributors
Commits on May 20, 2012
@pixeltrix pixeltrix Correct order of expected and actual arguments 972376a
@pixeltrix pixeltrix Raise ActionController::BadRequest for malformed parameter hashes.
Currently Rack raises a TypeError when it encounters a malformed or
ambiguous hash like `foo[]=bar&foo[4]=bar`. Rather than pass this
through to the application this commit captures the exception and
re-raises it using a new ActionController::BadRequest exception.

The new ActionController::BadRequest exception returns a 400 error
instead of the 500 error that would've been returned by the original
TypeError. This allows exception notification libraries to ignore
these errors if so desired.

Closes #3051
66eb3f0
@pixeltrix pixeltrix 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
3fc561a
@pixeltrix pixeltrix Escape the extension when normalizing the action cache path.
Although no recognized formats use non-ASCII characters, sometimes they
can be included in the :format parameter because of invalid URLS. To
prevent encoding incompatibility errors we need to escape them before
passing the path to URI.unescape.

Closes #4379
79b38c3
@drogus drogus Revert "Merge pull request #5702 from oscardelben/patch-4"
This reverts commit cae1ca7, reversing
changes made to da97cf0.

These changes break the build, it needs more investigation.
08a5b10
@ivankukobko ivankukobko fixed typo in word finiding 45d059b
@carlosantoniodasilva carlosantoniodasilva Merge pull request #6408 from ivankukobko/master
Fixed typo in AR test name
46fcce9
@rafaelfranca rafaelfranca Fix CHANGELOG order and add a brief description of the changes in the
Action Pack in the upgrading guide. [ci skip]
a78ee05
@josevalim josevalim Merge pull request #6407 from pinetops/565c1b0a0772ac6cf91c77e9285806…
…f7b028614c

Template concurrency fixes
Conflicts:

	actionpack/lib/action_view/template.rb
74c4e7b
@drogus drogus Fix generators to help with ambiguous `ApplicationController` issue
In development mode, dependencies are loaded dynamically at runtime,
using `const_missing`. Because of that, when one of the constants is
already loaded and `const_missing` is not triggered, user can end up
with unexpected results.

Given such file in an Engine:

```ruby
module Blog
  class PostsController < ApplicationController
  end
end
```

If you load it first, before loading any application files, it will
correctly load `Blog::ApplicationController`, because second line will
hit `const_missing`. However if you load `ApplicationController` first,
the constant will be loaded already, `const_missing` hook will not be
fired and in result `PostsController` will inherit from
`ApplicationController` instead of `Blog::ApplicationController`.

Since it can't be fixed in `AS::Dependencies`, the easiest fix is to
just explicitly load application controller.

closes #6413
7c95be5
Showing with 189 additions and 30 deletions.
  1. +7 −3 actionpack/CHANGELOG.md
  2. +3 −2 actionpack/lib/action_controller/caching/actions.rb
  3. +4 −1 actionpack/lib/action_controller/metal/exceptions.rb
  4. +10 −3 actionpack/lib/action_dispatch/http/request.rb
  5. +2 −1  actionpack/lib/action_dispatch/middleware/exception_wrapper.rb
  6. +9 −0 actionpack/lib/action_dispatch/routing/redirection.rb
  7. +9 −0 actionpack/lib/action_dispatch/routing/route_set.rb
  8. +1 −1  actionpack/lib/action_dispatch/testing/assertions/response.rb
  9. +22 −10 actionpack/lib/action_view/template.rb
  10. +30 −1 actionpack/test/controller/caching_test.rb
  11. +6 −0 actionpack/test/dispatch/debug_exceptions_test.rb
  12. +11 −0 actionpack/test/dispatch/request/query_string_parsing_test.rb
  13. +11 −0 actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb
  14. +1 −1  actionpack/test/dispatch/request_test.rb
  15. +31 −0 actionpack/test/dispatch/routing_test.rb
  16. +3 −1 activemodel/lib/active_model/attribute_methods.rb
  17. +1 −1  activerecord/test/cases/relations_test.rb
  18. +4 −0 guides/source/upgrading_ruby_on_rails.textile
  19. +4 −0 railties/lib/rails/generators/named_base.rb
  20. +4 −0 railties/lib/rails/generators/rails/controller/templates/controller.rb
  21. +4 −0 railties/lib/rails/generators/rails/scaffold_controller/templates/controller.rb
  22. +12 −5 railties/test/generators/namespaced_generators_test.rb
View
10 actionpack/CHANGELOG.md
@@ -1,5 +1,12 @@
## Rails 4.0.0 (unreleased) ##
+* `assert_generates`, `assert_recognizes`, and `assert_routing` all raise
+ `Assertion` instead of `RoutingError` *David Chelimsky*
+
+* 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
`optgroup` automatically, and deprecate `prompt` as third argument, in favor
of using an options hash. *Nicholas Greenfield*
@@ -186,9 +193,6 @@
* `ActionView::Helpers::TextHelper#highlight` now defaults to the
HTML5 `mark` element. *Brian Cardarella*
-* `assert_generates`, `assert_recognizes`, and `assert_routing` all raise
- `Assertion` instead of `RoutingError` *David Chelimsky*
-
## Rails 3.2.3 (March 30, 2012) ##
View
5 actionpack/lib/action_controller/caching/actions.rb
@@ -47,7 +47,7 @@ module Caching
# And you can also use <tt>:if</tt> (or <tt>:unless</tt>) to pass a
# proc that specifies when the action should be cached.
#
- # As of Rails 3.0, you can also pass <tt>:expires_in</tt> with a time
+ # As of Rails 3.0, you can also pass <tt>:expires_in</tt> with a time
# interval (in seconds) to schedule expiration of the cached item.
#
# The following example depicts some of the points made above:
@@ -178,8 +178,9 @@ def initialize(controller, options = {}, infer_extension = true)
private
def normalize!(path)
+ ext = URI.parser.escape(extension) if extension
path << 'index' if path[-1] == ?/
- path << ".#{extension}" if extension and !path.split('?', 2).first.ends_with?(".#{extension}")
+ path << ".#{ext}" if extension and !path.split('?', 2).first.ends_with?(".#{ext}")
URI.parser.unescape(path)
end
end
View
5 actionpack/lib/action_controller/metal/exceptions.rb
@@ -2,6 +2,9 @@ module ActionController
class ActionControllerError < StandardError #:nodoc:
end
+ class BadRequest < ActionControllerError #:nodoc:
+ end
+
class RenderError < ActionControllerError #:nodoc:
end
@@ -38,7 +41,7 @@ def initialize(message = nil)
class UnknownHttpMethod < ActionControllerError #:nodoc:
end
-
+
class UnknownFormat < ActionControllerError #:nodoc:
end
end
View
13 actionpack/lib/action_dispatch/http/request.rb
@@ -231,17 +231,24 @@ def session_options=(options)
# Override Rack's GET method to support indifferent access
def GET
- @env["action_dispatch.request.query_parameters"] ||= (normalize_parameters(super) || {})
+ begin
+ @env["action_dispatch.request.query_parameters"] ||= (normalize_parameters(super) || {})
+ rescue TypeError => e
+ raise ActionController::BadRequest, "Invalid query parameters: #{e.message}"
+ end
end
alias :query_parameters :GET
# Override Rack's POST method to support indifferent access
def POST
- @env["action_dispatch.request.request_parameters"] ||= (normalize_parameters(super) || {})
+ begin
+ @env["action_dispatch.request.request_parameters"] ||= (normalize_parameters(super) || {})
+ rescue TypeError => e
+ raise ActionController::BadRequest, "Invalid request parameters: #{e.message}"
+ end
end
alias :request_parameters :POST
-
# Returns the authorization header regardless of whether it was specified directly or through one of the
# proxy alternatives.
def authorization
View
3  actionpack/lib/action_dispatch/middleware/exception_wrapper.rb
@@ -12,7 +12,8 @@ class ExceptionWrapper
'ActionController::MethodNotAllowed' => :method_not_allowed,
'ActionController::NotImplemented' => :not_implemented,
'ActionController::UnknownFormat' => :not_acceptable,
- 'ActionController::InvalidAuthenticityToken' => :unprocessable_entity
+ 'ActionController::InvalidAuthenticityToken' => :unprocessable_entity,
+ 'ActionController::BadRequest' => :bad_request
)
cattr_accessor :rescue_templates
View
9 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
View
9 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.
View
2  actionpack/lib/action_dispatch/testing/assertions/response.rb
@@ -28,7 +28,7 @@ def assert_response(type, message = nil)
assert @response.send("#{type}?"), message
else
code = Rack::Utils::SYMBOL_TO_STATUS_CODE[type]
- assert_equal @response.response_code, code, message
+ assert_equal code, @response.response_code, message
end
else
assert_equal type, @response.response_code, message
View
32 actionpack/lib/action_view/template.rb
@@ -1,6 +1,7 @@
require 'active_support/core_ext/object/blank'
require 'active_support/core_ext/object/try'
require 'active_support/core_ext/kernel/singleton_class'
+require 'thread'
module ActionView
# = Action View Template
@@ -122,6 +123,7 @@ def initialize(source, identifier, handler, details)
@virtual_path = details[:virtual_path]
@updated_at = details[:updated_at] || Time.now
@formats = Array(format).map { |f| f.is_a?(Mime::Type) ? f.ref : f }
+ @compile_mutex = Mutex.new
end
# Returns if the underlying handler supports streaming. If so,
@@ -223,18 +225,28 @@ def encode!
def compile!(view) #:nodoc:
return if @compiled
- if view.is_a?(ActionView::CompiledTemplates)
- mod = ActionView::CompiledTemplates
- else
- mod = view.singleton_class
- end
+ # Templates can be used concurrently in threaded environments
+ # so compilation and any instance variable modification must
+ # be synchronized
+ @compile_mutex.synchronize do
+ # Any thread holding this lock will be compiling the template needed
+ # by the threads waiting. So re-check the @compiled flag to avoid
+ # re-compilation
+ return if @compiled
+
+ if view.is_a?(ActionView::CompiledTemplates)
+ mod = ActionView::CompiledTemplates
+ else
+ mod = view.singleton_class
+ end
- compile(view, mod)
+ compile(view, mod)
- # Just discard the source if we have a virtual path. This
- # means we can get the template back.
- @source = nil if @virtual_path
- @compiled = true
+ # Just discard the source if we have a virtual path. This
+ # means we can get the template back.
+ @source = nil if @virtual_path
+ @compiled = true
+ end
end
# Among other things, this method is responsible for properly setting
View
31 actionpack/test/controller/caching_test.rb
@@ -223,6 +223,7 @@ def page_cached?(action)
class ActionCachingTestController < CachingController
rescue_from(Exception) { head 500 }
+ rescue_from(ActionController::UnknownFormat) { head :not_acceptable }
if defined? ActiveRecord
rescue_from(ActiveRecord::RecordNotFound) { head :not_found }
end
@@ -230,7 +231,7 @@ class ActionCachingTestController < CachingController
# Eliminate uninitialized ivar warning
before_filter { @title = nil }
- caches_action :index, :redirected, :forbidden, :if => Proc.new { |c| !c.request.format.json? }, :expires_in => 1.hour
+ caches_action :index, :redirected, :forbidden, :if => Proc.new { |c| c.request.format && !c.request.format.json? }, :expires_in => 1.hour
caches_action :show, :cache_path => 'http://test.host/custom/show'
caches_action :edit, :cache_path => Proc.new { |c| c.params[:id] ? "http://test.host/#{c.params[:id]};edit" : "http://test.host/edit" }
caches_action :with_layout
@@ -239,6 +240,7 @@ class ActionCachingTestController < CachingController
caches_action :with_layout_proc_param, :layout => Proc.new { |c| c.params[:layout] }
caches_action :record_not_found, :four_oh_four, :simple_runtime_error
caches_action :streaming
+ caches_action :invalid
layout 'talk_from_action'
@@ -303,6 +305,14 @@ def expire_with_url_string
def streaming
render :text => "streaming", :stream => true
end
+
+ def invalid
+ @cache_this = MockTime.now.to_f.to_s
+
+ respond_to do |format|
+ format.json{ render :json => @cache_this }
+ end
+ end
end
class MockTime < Time
@@ -690,6 +700,25 @@ def test_action_caching_plus_streaming
assert fragment_exist?('hostname.com/action_caching_test/streaming')
end
+ def test_invalid_format_returns_not_acceptable
+ get :invalid, :format => "json"
+ assert_response :success
+ cached_time = content_to_cache
+ assert_equal cached_time, @response.body
+
+ assert fragment_exist?("hostname.com/action_caching_test/invalid.json")
+
+ get :invalid, :format => "json"
+ assert_response :success
+ assert_equal cached_time, @response.body
+
+ get :invalid, :format => "xml"
+ assert_response :not_acceptable
+
+ get :invalid, :format => "\xC3\x83"
+ assert_response :not_acceptable
+ end
+
private
def content_to_cache
assigns(:cache_this)
View
6 actionpack/test/dispatch/debug_exceptions_test.rb
@@ -35,6 +35,8 @@ def call(env)
raise ActionController::InvalidAuthenticityToken
when "/not_found_original_exception"
raise ActionView::Template::Error.new('template', AbstractController::ActionNotFound.new)
+ when "/bad_request"
+ raise ActionController::BadRequest
else
raise "puke!"
end
@@ -88,6 +90,10 @@ def call(env)
get "/method_not_allowed", {}, {'action_dispatch.show_exceptions' => true}
assert_response 405
assert_match(/ActionController::MethodNotAllowed/, body)
+
+ get "/bad_request", {}, {'action_dispatch.show_exceptions' => true}
+ assert_response 400
+ assert_match(/ActionController::BadRequest/, body)
end
test "does not show filtered parameters" do
View
11 actionpack/test/dispatch/request/query_string_parsing_test.rb
@@ -105,6 +105,17 @@ def teardown
)
end
+ test "ambiguous query string returns a bad request" do
+ with_routing do |set|
+ set.draw do
+ get ':action', :to => ::QueryStringParsingTest::TestController
+ end
+
+ get "/parse", nil, "QUERY_STRING" => "foo[]=bar&foo[4]=bar"
+ assert_response :bad_request
+ end
+ end
+
private
def assert_parses(expected, actual)
with_routing do |set|
View
11 actionpack/test/dispatch/request/url_encoded_params_parsing_test.rb
@@ -126,6 +126,17 @@ def teardown
assert_parses expected, query
end
+ test "ambiguous params returns a bad request" do
+ with_routing do |set|
+ set.draw do
+ post ':action', :to => ::UrlEncodedParamsParsingTest::TestController
+ end
+
+ post "/parse", "foo[]=bar&foo[4]=bar"
+ assert_response :bad_request
+ end
+ end
+
private
def with_test_routing
with_routing do |set|
View
2  actionpack/test/dispatch/request_test.rb
@@ -561,7 +561,7 @@ def url_for(options = {})
begin
request = stub_request(mock_rack_env)
request.parameters
- rescue TypeError
+ rescue ActionController::BadRequest
# rack will raise a TypeError when parsing this query string
end
assert_equal({}, request.parameters)
View
31 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
View
4 activemodel/lib/active_model/attribute_methods.rb
@@ -102,6 +102,7 @@ module ClassMethods
# person.name # => nil
def attribute_method_prefix(*prefixes)
self.attribute_method_matchers += prefixes.map { |prefix| AttributeMethodMatcher.new :prefix => prefix }
+ undefine_attribute_methods
end
# Declares a method available for all attributes with the given suffix.
@@ -138,6 +139,7 @@ def attribute_method_prefix(*prefixes)
# person.name_short? # => true
def attribute_method_suffix(*suffixes)
self.attribute_method_matchers += suffixes.map { |suffix| AttributeMethodMatcher.new :suffix => suffix }
+ undefine_attribute_methods
end
# Declares a method available for all attributes with the given prefix
@@ -175,6 +177,7 @@ def attribute_method_suffix(*suffixes)
# person.name # => 'Gemma'
def attribute_method_affix(*affixes)
self.attribute_method_matchers += affixes.map { |affix| AttributeMethodMatcher.new :prefix => affix[:prefix], :suffix => affix[:suffix] }
+ undefine_attribute_methods
end
@@ -222,7 +225,6 @@ def alias_attribute(new_name, old_name)
# end
# end
def define_attribute_methods(*attr_names)
- undefine_attribute_methods
attr_names.flatten.each { |attr_name| define_attribute_method(attr_name) }
end
View
2  activerecord/test/cases/relations_test.rb
@@ -133,7 +133,7 @@ def test_reload
assert topics.loaded?
end
- def test_finiding_with_subquery
+ def test_finding_with_subquery
relation = Topic.where(:approved => true)
assert_equal relation.to_a, Topic.select('*').from(relation).to_a
assert_equal relation.to_a, Topic.select('subquery.*').from(relation).to_a
View
4 guides/source/upgrading_ruby_on_rails.textile
@@ -42,6 +42,10 @@ h4(#active_model4_0). ActiveModel
Rails 4.0 has changed how errors attach with the ConfirmationValidator. Now when confirmation validations fail the error will be attached to <tt>:#{attribute}_confirmation</tt> instead of <tt>attribute</tt>.
+h4(#action_pack4_0). ActionPack
+
+Rails 4.0 changed how <tt>assert_generates</tt>, <tt>assert_recognizes</tt>, and <tt>assert_routing</tt> work. Now all these assertions raise <tt>Assertion</tt> instead of <tt>RoutingError</tt>.
+
h3. Upgrading from Rails 3.1 to Rails 3.2
If your application is currently on any version of Rails older than 3.1.x, you should upgrade to Rails 3.1 before attempting an update to Rails 3.2.
View
4 railties/lib/rails/generators/named_base.rb
@@ -79,6 +79,10 @@ def regular_class_path
@class_path
end
+ def namespaced_file_path
+ @namespaced_file_path ||= namespaced_class_path.join("/")
+ end
+
def namespaced_class_path
@namespaced_class_path ||= begin
namespace_path = namespace.name.split("::").map {|m| m.underscore }
View
4 railties/lib/rails/generators/rails/controller/templates/controller.rb
@@ -1,3 +1,7 @@
+<% if namespaced? -%>
+require "<%= namespaced_file_path %>/application_controller"
+<% end -%>
+
<% module_namespacing do -%>
class <%= class_name %>Controller < ApplicationController
<% actions.each do |action| -%>
View
4 railties/lib/rails/generators/rails/scaffold_controller/templates/controller.rb
@@ -1,3 +1,7 @@
+<% if namespaced? -%>
+require "<%= namespaced_file_path %>/application_controller"
+<% end -%>
+
<% module_namespacing do -%>
class <%= controller_class_name %>Controller < ApplicationController
# GET <%= route_url %>
View
17 railties/test/generators/namespaced_generators_test.rb
@@ -20,8 +20,14 @@ class NamespacedControllerGeneratorTest < NamespacedGeneratorTestCase
def test_namespaced_controller_skeleton_is_created
run_generator
- assert_file "app/controllers/test_app/account_controller.rb", /module TestApp/, / class AccountController < ApplicationController/
- assert_file "test/functional/test_app/account_controller_test.rb", /module TestApp/, / class AccountControllerTest/
+ assert_file "app/controllers/test_app/account_controller.rb",
+ /require "test_app\/application_controller"/,
+ /module TestApp/,
+ / class AccountController < ApplicationController/
+
+ assert_file "test/functional/test_app/account_controller_test.rb",
+ /module TestApp/,
+ / class AccountControllerTest/
end
def test_skipping_namespace
@@ -227,9 +233,10 @@ def test_scaffold_on_invoke
end
# Controller
- assert_file "app/controllers/test_app/product_lines_controller.rb" do |content|
- assert_match(/module TestApp\n class ProductLinesController < ApplicationController/, content)
- end
+ assert_file "app/controllers/test_app/product_lines_controller.rb",
+ /require "test_app\/application_controller"/,
+ /module TestApp/,
+ /class ProductLinesController < ApplicationController/
assert_file "test/functional/test_app/product_lines_controller_test.rb",
/module TestApp\n class ProductLinesControllerTest < ActionController::TestCase/

No commit comments for this range

Something went wrong with that request. Please try again.