From 69ab91ae9396f0101afd13871f179a7f779d3178 Mon Sep 17 00:00:00 2001
From: Lukasz Sarnacki <lukesarnacki@gmail.com>
Date: Thu, 23 Jan 2014 16:31:52 +0100
Subject: Log which keys were set to nil in deep_munge

deep_munge solves CVE-2013-0155 security vulnerability, but its
behaviour is definately confuisng. This commit adds logging to deep_munge.
It logs keys for which values were set to nil.

Also mentions in guides were added.
---
 actionpack/CHANGELOG.md                            |  8 ++++++++
 actionpack/lib/action_controller/log_subscriber.rb |  9 +++++++++
 actionpack/lib/action_dispatch/request/utils.rb    | 13 +++++++++----
 3 files changed, 26 insertions(+), 4 deletions(-)

(limited to 'actionpack')

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 23bb01d678..fc05ae3cec 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -11,6 +11,14 @@
 
     *Maurizio De Santis*
 
+*   Log which keys were affected by deep munge.
+
+    Deep munge solves CVE-2013-0155 security vulnerability, but its
+    behaviour is definately confusing, so now at least information
+    about for which keys values were set to nil is visible in logs.
+
+    *Łukasz Sarnacki*
+
 *   Automatically convert dashes to underscores for shorthand routes, e.g:
 
         get '/our-work/latest'
diff --git a/actionpack/lib/action_controller/log_subscriber.rb b/actionpack/lib/action_controller/log_subscriber.rb
index 9279d8bcea..823a1050b5 100644
--- a/actionpack/lib/action_controller/log_subscriber.rb
+++ b/actionpack/lib/action_controller/log_subscriber.rb
@@ -53,6 +53,15 @@ module ActionController
       debug("Unpermitted parameters: #{unpermitted_keys.join(", ")}")
     end
 
+    def deep_munge(event)
+      message = "Value for params[:#{event.payload[:keys].join('][:')}] was set"\
+                "to nil, because it was one of [], [null] or [null, null, ...]."\
+                "Go to http://guides.rubyonrails.org/security.html#unsafe-query-generation"\
+                "for more information."\
+
+      debug(message)
+    end
+
     %w(write_fragment read_fragment exist_fragment?
        expire_fragment expire_page write_page).each do |method|
       class_eval <<-METHOD, __FILE__, __LINE__ + 1
diff --git a/actionpack/lib/action_dispatch/request/utils.rb b/actionpack/lib/action_dispatch/request/utils.rb
index a6dca9741c..9d4f1aa3c5 100644
--- a/actionpack/lib/action_dispatch/request/utils.rb
+++ b/actionpack/lib/action_dispatch/request/utils.rb
@@ -7,18 +7,23 @@ module ActionDispatch
 
       class << self
         # Remove nils from the params hash
-        def deep_munge(hash)
+        def deep_munge(hash, keys = [])
           return hash unless perform_deep_munge
 
           hash.each do |k, v|
+            keys << k
             case v
             when Array
-              v.grep(Hash) { |x| deep_munge(x) }
+              v.grep(Hash) { |x| deep_munge(x, keys) }
               v.compact!
-              hash[k] = nil if v.empty?
+              if v.empty?
+                hash[k] = nil
+                ActiveSupport::Notifications.instrument("deep_munge.action_controller", keys: keys)
+              end
             when Hash
-              deep_munge(v)
+              deep_munge(v, keys)
             end
+            keys.pop
           end
 
           hash
-- 
cgit v1.2.3