From a500b4796f86b05b3fece414f090a496d3cb4298 Mon Sep 17 00:00:00 2001 From: Jon Leighton Date: Thu, 6 Apr 2017 16:03:35 +0100 Subject: Improve logging when Origin header doesn't match I came up against this while dealing with a misconfigured server. The browser was setting the Origin header to "https://example.com", but the Rails app returned "http://example.com" from request.base_url (because it was failing to detect that HTTPS was used). This caused verify_authenticity_token to fail, but the message in the log was "Can't verify CSRF token", which is confusing because the failure had nothing to do with the CSRF token sent in the request. This made it very hard to identify the issue, so hopefully this will make it more obvious for the next person. --- .../lib/action_controller/metal/request_forgery_protection.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'actionpack/lib') diff --git a/actionpack/lib/action_controller/metal/request_forgery_protection.rb b/actionpack/lib/action_controller/metal/request_forgery_protection.rb index d9a8b9c12d..5051c02a62 100644 --- a/actionpack/lib/action_controller/metal/request_forgery_protection.rb +++ b/actionpack/lib/action_controller/metal/request_forgery_protection.rb @@ -213,7 +213,11 @@ module ActionController #:nodoc: if !verified_request? if logger && log_warning_on_csrf_failure - logger.warn "Can't verify CSRF token authenticity." + if valid_request_origin? + logger.warn "Can't verify CSRF token authenticity." + else + logger.warn "HTTP Origin header (#{request.origin}) didn't match request.base_url (#{request.base_url})" + end end handle_unverified_request end -- cgit v1.2.3 From 35fac87123df4fdf7cc26b7ccd724932d946be87 Mon Sep 17 00:00:00 2001 From: Julian Nadeau Date: Mon, 13 Mar 2017 17:37:22 -0400 Subject: Add action_controller_api, action_controller_base on_load hook --- actionpack/lib/action_controller/api.rb | 1 + actionpack/lib/action_controller/base.rb | 1 + 2 files changed, 2 insertions(+) (limited to 'actionpack/lib') diff --git a/actionpack/lib/action_controller/api.rb b/actionpack/lib/action_controller/api.rb index 0d1af0d0bd..94698df730 100644 --- a/actionpack/lib/action_controller/api.rb +++ b/actionpack/lib/action_controller/api.rb @@ -141,6 +141,7 @@ module ActionController include mod end + ActiveSupport.run_load_hooks(:action_controller_api, self) ActiveSupport.run_load_hooks(:action_controller, self) end end diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb index b420e00c78..8c2b111f89 100644 --- a/actionpack/lib/action_controller/base.rb +++ b/actionpack/lib/action_controller/base.rb @@ -267,6 +267,7 @@ module ActionController end end + ActiveSupport.run_load_hooks(:action_controller_base, self) ActiveSupport.run_load_hooks(:action_controller, self) end end -- cgit v1.2.3 From 8776a7139757d0b264785c774d4e7f37d4bc1ac7 Mon Sep 17 00:00:00 2001 From: Andrew White Date: Tue, 18 Apr 2017 11:02:05 +0100 Subject: Use more specific check for :format in route path The current check for whether to add an optional format to the path is very lax and will match things like `:format_id` where there are nested resources, e.g: resources :formats do resources :items end Fix this by using a more restrictive regex pattern that looks for the patterns `(.:format)`, `.:format` or `/` at the end of the path. Note that we need to allow for multiple closing parenthesis since the route may be of this form: get "/books(/:action(.:format))", controller: "books" This probably isn't what's intended since it means that the default index action route doesn't support a format but we have a test for it so we need to allow it. Fixes #28517. --- actionpack/lib/action_dispatch/routing/mapper.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'actionpack/lib') diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb index 8ad17504ae..74904e3d45 100644 --- a/actionpack/lib/action_dispatch/routing/mapper.rb +++ b/actionpack/lib/action_dispatch/routing/mapper.rb @@ -54,6 +54,7 @@ module ActionDispatch class Mapping #:nodoc: ANCHOR_CHARACTERS_REGEX = %r{\A(\\A|\^)|(\\Z|\\z|\$)\Z} + OPTIONAL_FORMAT_REGEX = %r{(?:\(\.:format\)+|\.:format|/)\Z} attr_reader :requirements, :defaults attr_reader :to, :default_controller, :default_action @@ -93,7 +94,7 @@ module ActionDispatch end def self.optional_format?(path, format) - format != false && !path.include?(":format") && !path.end_with?("/") + format != false && path !~ OPTIONAL_FORMAT_REGEX end def initialize(set, ast, defaults, controller, default_action, modyoule, to, formatted, scope_constraints, blocks, via, options_constraints, anchor, options) -- cgit v1.2.3 From fd88ccc905549c61e0e4525fcb68b91d20b9afe9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 11 Apr 2017 21:45:39 -0400 Subject: Raise exception when calling to_h in a unfiltered Parameters Before we returned either an empty hash or only the always permitted parameters (:controller and :action by default). The previous behavior was dangerous because in order to get the attributes users usually fallback to use to_unsafe_h that could potentially introduce security issues. The to_unsafe_h API is also not good since Parameters is a object that quacks like a Hash but not in all cases since to_h would return an empty hash and users were forced to check if to_unsafe_h is defined or if the instance is a ActionController::Parameters in order to work with it. This end up coupling a lot of libraries and parts of the application with something that is from the controller layer. --- .../lib/action_controller/metal/strong_parameters.rb | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) (limited to 'actionpack/lib') diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index baf15570d5..b20ad940ea 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -43,6 +43,18 @@ module ActionController end end + # Raised when a Parameters instance is not marked as permitted and + # an operation to transform it to hash is called. + # + # params = ActionController::Parameters.new(a: "123", b: "456") + # params.to_h + # # => ActionController::UnfilteredParameters: unable to convert unpermitted parameters to hash + class UnfilteredParameters < StandardError + def initialize # :nodoc: + super("unable to convert unpermitted parameters to hash") + end + end + # == Action Controller \Parameters # # Allows you to choose which attributes should be whitelisted for mass updating @@ -234,7 +246,8 @@ module ActionController # name: 'Senjougahara Hitagi', # oddity: 'Heavy stone crab' # }) - # params.to_h # => {} + # params.to_h + # # => ActionController::UnfilteredParameters: unable to convert unfiltered parameters to hash # # safe_params = params.permit(:name) # safe_params.to_h # => {"name"=>"Senjougahara Hitagi"} @@ -242,7 +255,7 @@ module ActionController if permitted? convert_parameters_to_hashes(@parameters, :to_h) else - slice(*self.class.always_permitted_parameters).permit!.to_h + raise UnfilteredParameters end end -- cgit v1.2.3 From 9f4c2632ef28b9622ffa0eca5d02beea8ec809c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 11 Apr 2017 21:56:55 -0400 Subject: Add ActionController::Parameters#to_hash to implict conversion Now methods that implicit convert objects to a hash will be able to work without requiring the users to change their implementation. This method will return a Hash instead of a HashWithIndefirentAccess to mimic the same implementation of HashWithIndefirentAccess#to_hash. --- .../lib/action_controller/metal/strong_parameters.rb | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) (limited to 'actionpack/lib') diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index b20ad940ea..62c654da03 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -259,6 +259,22 @@ module ActionController end end + # Returns a safe Hash representation of this parameter + # with all unpermitted keys removed. + # + # params = ActionController::Parameters.new({ + # name: 'Senjougahara Hitagi', + # oddity: 'Heavy stone crab' + # }) + # params.to_hash + # # => ActionController::UnfilteredParameters: unable to convert unfiltered parameters to hash + # + # safe_params = params.permit(:name) + # safe_params.to_hash # => {"name"=>"Senjougahara Hitagi"} + def to_hash + to_h.to_hash + end + # Returns an unsafe, unfiltered # ActiveSupport::HashWithIndifferentAccess representation of this # parameter. -- cgit v1.2.3 From 29333ddb69e69d0fa99a66bf5fab333e8c5611aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Tue, 11 Apr 2017 22:21:14 -0400 Subject: Implement ActionController::Parameters#to_query and #to_param Previously it was raising an error because it may be unsafe to use those methods in a unpermitted parameter. Now we delegate to to_h that already raise an error when the Parameters instance is not permitted. This also fix a bug when using `#to_query` in a hash that contains a `ActionController::Parameters` instance and was returning the name of the class in the string. --- .../action_controller/metal/strong_parameters.rb | 30 ++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) (limited to 'actionpack/lib') diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index 62c654da03..ac8e7eec84 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -275,6 +275,34 @@ module ActionController to_h.to_hash end + # Returns a string representation of the receiver suitable for use as a URL + # query string: + # + # params = ActionController::Parameters.new({ + # name: 'David', + # nationality: 'Danish' + # }) + # params.to_query + # # => "name=David&nationality=Danish" + # + # An optional namespace can be passed to enclose key names: + # + # params = ActionController::Parameters.new({ + # name: 'David', + # nationality: 'Danish' + # }) + # params.to_query('user') + # # => "user%5Bname%5D=David&user%5Bnationality%5D=Danish" + # + # The string pairs "key=value" that conform the query string + # are sorted lexicographically in ascending order. + # + # This method is also aliased as +to_param+. + def to_query(*args) + to_h.to_query(*args) + end + alias_method :to_param, :to_query + # Returns an unsafe, unfiltered # ActiveSupport::HashWithIndifferentAccess representation of this # parameter. @@ -744,8 +772,6 @@ module ActionController end end - undef_method :to_param - # Returns duplicate of object including all parameters. def deep_dup self.class.new(@parameters.deep_dup).tap do |duplicate| -- cgit v1.2.3 From 3ee56f7b3de34aa7f6bc9fc69c93544c3f037798 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Thu, 13 Apr 2017 16:11:08 -0400 Subject: Improve documentation We are talking about a list of parameters even so we need to use plural. Even if we were talking about the instance of the Parameters object we would have to use the capital and monospaced font. --- actionpack/lib/action_controller/metal/strong_parameters.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'actionpack/lib') diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index ac8e7eec84..f816fd7f74 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -240,7 +240,7 @@ module ActionController end # Returns a safe ActiveSupport::HashWithIndifferentAccess - # representation of this parameter with all unpermitted keys removed. + # representation of the parameters with all unpermitted keys removed. # # params = ActionController::Parameters.new({ # name: 'Senjougahara Hitagi', @@ -259,7 +259,7 @@ module ActionController end end - # Returns a safe Hash representation of this parameter + # Returns a safe Hash representation of the parameters # with all unpermitted keys removed. # # params = ActionController::Parameters.new({ @@ -304,8 +304,8 @@ module ActionController alias_method :to_param, :to_query # Returns an unsafe, unfiltered - # ActiveSupport::HashWithIndifferentAccess representation of this - # parameter. + # ActiveSupport::HashWithIndifferentAccess representation of the + # parameters. # # params = ActionController::Parameters.new({ # name: 'Senjougahara Hitagi', -- cgit v1.2.3 From 70c6fb1a06d76d0d6cddb6fb50137aa1341745a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Mon, 17 Apr 2017 18:28:02 -0400 Subject: Follow the style guide rules in the documetation --- .../action_controller/metal/strong_parameters.rb | 60 +++++++++++----------- 1 file changed, 30 insertions(+), 30 deletions(-) (limited to 'actionpack/lib') diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index f816fd7f74..ce60026325 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -65,9 +65,9 @@ module ActionController # # params = ActionController::Parameters.new({ # person: { - # name: 'Francesco', + # name: "Francesco", # age: 22, - # role: 'admin' + # role: "admin" # } # }) # @@ -115,7 +115,7 @@ module ActionController # You can fetch values of ActionController::Parameters using either # :key or "key". # - # params = ActionController::Parameters.new(key: 'value') + # params = ActionController::Parameters.new(key: "value") # params[:key] # => "value" # params["key"] # => "value" class Parameters @@ -215,13 +215,13 @@ module ActionController # class Person < ActiveRecord::Base # end # - # params = ActionController::Parameters.new(name: 'Francesco') + # params = ActionController::Parameters.new(name: "Francesco") # params.permitted? # => false # Person.new(params) # => ActiveModel::ForbiddenAttributesError # # ActionController::Parameters.permit_all_parameters = true # - # params = ActionController::Parameters.new(name: 'Francesco') + # params = ActionController::Parameters.new(name: "Francesco") # params.permitted? # => true # Person.new(params) # => # def initialize(parameters = {}) @@ -243,8 +243,8 @@ module ActionController # representation of the parameters with all unpermitted keys removed. # # params = ActionController::Parameters.new({ - # name: 'Senjougahara Hitagi', - # oddity: 'Heavy stone crab' + # name: "Senjougahara Hitagi", + # oddity: "Heavy stone crab" # }) # params.to_h # # => ActionController::UnfilteredParameters: unable to convert unfiltered parameters to hash @@ -263,8 +263,8 @@ module ActionController # with all unpermitted keys removed. # # params = ActionController::Parameters.new({ - # name: 'Senjougahara Hitagi', - # oddity: 'Heavy stone crab' + # name: "Senjougahara Hitagi", + # oddity: "Heavy stone crab" # }) # params.to_hash # # => ActionController::UnfilteredParameters: unable to convert unfiltered parameters to hash @@ -279,8 +279,8 @@ module ActionController # query string: # # params = ActionController::Parameters.new({ - # name: 'David', - # nationality: 'Danish' + # name: "David", + # nationality: "Danish" # }) # params.to_query # # => "name=David&nationality=Danish" @@ -288,10 +288,10 @@ module ActionController # An optional namespace can be passed to enclose key names: # # params = ActionController::Parameters.new({ - # name: 'David', - # nationality: 'Danish' + # name: "David", + # nationality: "Danish" # }) - # params.to_query('user') + # params.to_query("user") # # => "user%5Bname%5D=David&user%5Bnationality%5D=Danish" # # The string pairs "key=value" that conform the query string @@ -308,8 +308,8 @@ module ActionController # parameters. # # params = ActionController::Parameters.new({ - # name: 'Senjougahara Hitagi', - # oddity: 'Heavy stone crab' + # name: "Senjougahara Hitagi", + # oddity: "Heavy stone crab" # }) # params.to_unsafe_h # # => {"name"=>"Senjougahara Hitagi", "oddity" => "Heavy stone crab"} @@ -354,7 +354,7 @@ module ActionController # class Person < ActiveRecord::Base # end # - # params = ActionController::Parameters.new(name: 'Francesco') + # params = ActionController::Parameters.new(name: "Francesco") # params.permitted? # => false # Person.new(params) # => ActiveModel::ForbiddenAttributesError # params.permit! @@ -376,7 +376,7 @@ module ActionController # When passed a single key, if it exists and its associated value is # either present or the singleton +false+, returns said value: # - # ActionController::Parameters.new(person: { name: 'Francesco' }).require(:person) + # ActionController::Parameters.new(person: { name: "Francesco" }).require(:person) # # => "Francesco"} permitted: false> # # Otherwise raises ActionController::ParameterMissing: @@ -409,7 +409,7 @@ module ActionController # Technically this method can be used to fetch terminal values: # # # CAREFUL - # params = ActionController::Parameters.new(person: { name: 'Finn' }) + # params = ActionController::Parameters.new(person: { name: "Finn" }) # name = params.require(:person).require(:name) # CAREFUL # # but take into account that at some point those ones have to be permitted: @@ -439,7 +439,7 @@ module ActionController # for the object to +true+. This is useful for limiting which attributes # should be allowed for mass updating. # - # params = ActionController::Parameters.new(user: { name: 'Francesco', age: 22, role: 'admin' }) + # params = ActionController::Parameters.new(user: { name: "Francesco", age: 22, role: "admin" }) # permitted = params.require(:user).permit(:name, :age) # permitted.permitted? # => true # permitted.has_key?(:name) # => true @@ -459,7 +459,7 @@ module ActionController # You may declare that the parameter should be an array of permitted scalars # by mapping it to an empty array: # - # params = ActionController::Parameters.new(tags: ['rails', 'parameters']) + # params = ActionController::Parameters.new(tags: ["rails", "parameters"]) # params.permit(tags: []) # # Sometimes it is not possible or convenient to declare the valid keys of @@ -475,11 +475,11 @@ module ActionController # # params = ActionController::Parameters.new({ # person: { - # name: 'Francesco', + # name: "Francesco", # age: 22, # pets: [{ - # name: 'Purplish', - # category: 'dogs' + # name: "Purplish", + # category: "dogs" # }] # } # }) @@ -498,8 +498,8 @@ module ActionController # params = ActionController::Parameters.new({ # person: { # contact: { - # email: 'none@test.com', - # phone: '555-1234' + # email: "none@test.com", + # phone: "555-1234" # } # } # }) @@ -532,7 +532,7 @@ module ActionController # Returns a parameter for the given +key+. If not found, # returns +nil+. # - # params = ActionController::Parameters.new(person: { name: 'Francesco' }) + # params = ActionController::Parameters.new(person: { name: "Francesco" }) # params[:person] # => "Francesco"} permitted: false> # params[:none] # => nil def [](key) @@ -551,11 +551,11 @@ module ActionController # if more arguments are given, then that will be returned; if a block # is given, then that will be run and its result returned. # - # params = ActionController::Parameters.new(person: { name: 'Francesco' }) + # params = ActionController::Parameters.new(person: { name: "Francesco" }) # params.fetch(:person) # => "Francesco"} permitted: false> # params.fetch(:none) # => ActionController::ParameterMissing: param is missing or the value is empty: none - # params.fetch(:none, 'Francesco') # => "Francesco" - # params.fetch(:none) { 'Francesco' } # => "Francesco" + # params.fetch(:none, "Francesco") # => "Francesco" + # params.fetch(:none) { "Francesco" } # => "Francesco" def fetch(key, *args) convert_value_to_parameters( @parameters.fetch(key) { -- cgit v1.2.3 From 93034ad7fea7e00562103a7cd0acfab19bbfadf9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafael=20Mendon=C3=A7a=20Fran=C3=A7a?= Date: Mon, 17 Apr 2017 18:55:21 -0400 Subject: Reuse the Parameters#to_h check in the routing helpers Since this protection is now in Parameters we can use it instead of reimplementing again. --- actionpack/lib/action_controller/metal/strong_parameters.rb | 2 +- actionpack/lib/action_dispatch/routing.rb | 9 --------- actionpack/lib/action_dispatch/routing/route_set.rb | 6 +----- actionpack/lib/action_dispatch/routing/url_for.rb | 13 +++---------- 4 files changed, 5 insertions(+), 25 deletions(-) (limited to 'actionpack/lib') diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb index ce60026325..7864f9decd 100644 --- a/actionpack/lib/action_controller/metal/strong_parameters.rb +++ b/actionpack/lib/action_controller/metal/strong_parameters.rb @@ -49,7 +49,7 @@ module ActionController # params = ActionController::Parameters.new(a: "123", b: "456") # params.to_h # # => ActionController::UnfilteredParameters: unable to convert unpermitted parameters to hash - class UnfilteredParameters < StandardError + class UnfilteredParameters < ArgumentError def initialize # :nodoc: super("unable to convert unpermitted parameters to hash") end diff --git a/actionpack/lib/action_dispatch/routing.rb b/actionpack/lib/action_dispatch/routing.rb index 60d4789a63..87dd1eba38 100644 --- a/actionpack/lib/action_dispatch/routing.rb +++ b/actionpack/lib/action_dispatch/routing.rb @@ -254,14 +254,5 @@ module ActionDispatch SEPARATORS = %w( / . ? ) #:nodoc: HTTP_METHODS = [:get, :head, :post, :patch, :put, :delete, :options] #:nodoc: - - #:stopdoc: - INSECURE_URL_PARAMETERS_MESSAGE = <<-MSG.squish - Attempting to generate a URL from non-sanitized request parameters! - - An attacker can inject malicious data into the generated URL, such as - changing the host. Whitelist and sanitize passed parameters to be secure. - MSG - #:startdoc: end end diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb index 129e90037e..e1f9fc9ecc 100644 --- a/actionpack/lib/action_dispatch/routing/route_set.rb +++ b/actionpack/lib/action_dispatch/routing/route_set.rb @@ -318,11 +318,7 @@ module ActionDispatch when Hash args.pop when ActionController::Parameters - if last.permitted? - args.pop.to_h - else - raise ArgumentError, ActionDispatch::Routing::INSECURE_URL_PARAMETERS_MESSAGE - end + args.pop.to_h end helper.call self, args, options end diff --git a/actionpack/lib/action_dispatch/routing/url_for.rb b/actionpack/lib/action_dispatch/routing/url_for.rb index 008216cc80..a9bdefa775 100644 --- a/actionpack/lib/action_dispatch/routing/url_for.rb +++ b/actionpack/lib/action_dispatch/routing/url_for.rb @@ -171,17 +171,10 @@ module ActionDispatch case options when nil _routes.url_for(url_options.symbolize_keys) - when Hash + when Hash, ActionController::Parameters route_name = options.delete :use_route - _routes.url_for(options.symbolize_keys.reverse_merge!(url_options), - route_name) - when ActionController::Parameters - unless options.permitted? - raise ArgumentError.new(ActionDispatch::Routing::INSECURE_URL_PARAMETERS_MESSAGE) - end - route_name = options.delete :use_route - _routes.url_for(options.to_h.symbolize_keys. - reverse_merge!(url_options), route_name) + merged_url_options = options.to_h.symbolize_keys.reverse_merge!(url_options) + _routes.url_for(merged_url_options, route_name) when String options when Symbol -- cgit v1.2.3