This project is archived and is in readonly mode.

Filter sensitive `QUERY_STRING` parameters in the log

#6244

On a year before, I've submitted a patch which move Action Controller's filter_parameter_logging into ActionDispatch::Http::ParametersFilter.filter_parameters. (https://github.com/rails/rails/commit/bd4f21f) My intention of doing that was actually the fact that I want to move those logics out from Action Controller to be able to access it somewhere else.

So, this patch is actually a sequel of that previous patch.

In real world, I'm seeing a lot of applications which involves sending some kind of secret information such as api_key or token as a QUERY_STRING, or GET parameter more precisely. While setting config.filter_parameters already filtered out the parameters hash in the log, it didn't filtered out the request's path.

Consider this log, given I configured config.filter_parameters += [:password, :secret]:

Started GET "/posts?secret=abcd&user%5Bname%5D=sikachu" for 127.0.0.1 at Sat Jan 01 22:29:18 +0700 2011
  Processing by PostsController#index as HTML
  Parameters: {"user"=>{"name"=>"sikachu"}, "secret"=>"[FILTERED]"}
  Post Load (0.5ms)  SELECT "posts".* FROM "posts" 
Rendered posts/index.html.erb within layouts/application (3.2ms)
Completed 200 OK in 24ms (Views: 17.7ms | ActiveRecord: 0.5ms)

You can see that on the first line it's exposing the value of secret. So, in this patch I just take that QUERY_STRING and filtered them out, resulting in:

Started GET "/posts?secret=[FILTERED]&user%5Bname%5D=sikachu" for 127.0.0.1 at Sun Jan 02 00:06:41 +0700 2011
  Processing by PostsController#index as HTML
  Parameters: {"user"=>{"name"=>"sikachu"}, "secret"=>"[FILTERED]"}
  Post Load (0.4ms)  SELECT "posts".* FROM "posts" 
Rendered posts/index.html.erb within layouts/application (0.9ms)
Completed 200 OK in 11ms (Views: 6.7ms | ActiveRecord: 0.4ms)

I think this would make your log file safer, if your server got compromised and someone trying to extract something out of your log file.

However, there's one known issue: This approach has been done by reconstructing the QUERY_STRING from request.GET hash. This would result in shifting of parameter position in Ruby version < 1.9, as it doesn't sort hash by insertion order. I don't know if it's crucial or not, but IMO not leaking secret information into log file is more crucial and people would move to 1.9.x one day anyway.

Please review it, and let me know if it's acceptable. Thank you :)

Reported by Prem Sichanugrist (sikachu) · January 1st, 2011 @ 05:13 PM

Activity

Xavier Noria
Looks good, I'll try to apply it today.

February 4th, 2011 @ 09:21 AM
Xavier Noria
Wonder if it is worth mentioning in actionpack/lib/action_dispatch/http/parameters_filter.rb that filtering is performed both in the logged parameters hash, and the query string. The current wording isn't clear about this (kind of implies only the hash, which is the current implementation).

February 4th, 2011 @ 09:27 AM
Prem Sichanugrist (sikachu)
Yes, I think the documentation overthere should be updated as well. I'll update it and push to docrails :)

Thank you.

February 4th, 2011 @ 09:30 AM
Xavier Noria
Good :).

It is better to update the patch, though. A complete patch has code, tests, and docs, as needed. If a test was missing we'd update the patch. If some docs revision is needed, it is better that it comes with the patch as well.

docrails is more for quick fixes and writing guides, it does not replace the need for self-contained commits to master :).

February 4th, 2011 @ 09:37 AM
Prem Sichanugrist (sikachu)
Aha! I got your point. Let me get the patch cooking then ;)

February 4th, 2011 @ 09:38 AM
Matt Jones
To play devil's advocate for a bit, isn't passing secrets in GET parameters generally a bad idea anyways? For instance, they still end up in the Apache logs if you're running with Passenger and would presumably end up in the logs of any proxy servers the request passes through.

February 4th, 2011 @ 06:15 PM
Dan Pickett
I'm in agreement that it's bad practice, but I've definitely seen use cases where third parties require a GET with some silly things in the query string (Push type systems that send a unique identifier in a query string via a GET). While we can't guarantee it's not logged elsewhere, I still think it's good practice to have the option to have it omitted in the Rails logs.

I'm +1 on this once Prem's doc patch is attached.

February 4th, 2011 @ 10:38 PM
Prem Sichanugrist (sikachu)
Here's the new patch with documentation.
- 0001-filter-sensitive-query_string-parameters-in-the-log.patch 3.81 KB
February 6th, 2011 @ 04:17 AM
Xavier Noria
I've played a bit with this feature.

I think it is worthwhile but see a few details that could still be improved:
1. If the user sends a genuine string "[FILTERED]" (encoded as %5BFILTERED%5D) as value or part of the value of an unfiltered parameter, the square brackets will be decoded in the logged query string, while they shouldn't.
2. At least in 1.8 the reconstructed query string in the log does not necessarily match the actual query string, because it does not necessarily preserve the order of the parameters (and in some of my tests it really doesn't).
3. If a parameter in the query string is repeated, say the "x" in "x=y&a=b&x=z", only one occurrence is shown in the reconstructed query string in the log. So it will also differ from the real query string.
Prem, do you think you could work on those ones?
February 8th, 2011 @ 11:18 PM
Prem Sichanugrist (sikachu)
Nice catch! Yes, it seems like I've overlooked something in my patch. Will update it soon. :)

February 9th, 2011 @ 02:25 AM
Prem Sichanugrist (sikachu)
- Assigned user changed from Prem Sichanugrist (sikachu) to Xavier Noria
I've update the patch to fixes those issues above, and add those cases as the test.

I've included patch for master and 3-0-stable. The only difference is the CHANGELOG entry.
- filter_get_master.patch 5.07 KB
- filter_get_3-0-stable.patch 5.04 KB
March 9th, 2011 @ 03:29 PM
Xavier Noria
Looks good to me. I have been working a bit on the patches:
- Semicolon as a separator was untested
- The edge case /foo?secret (no equal sign) was not handled properly, it was logged as /foo?secret=[FILTERED]
Will push them soon.

Thanks Prem!
March 10th, 2011 @ 11:18 PM
Repository
- State changed from open to committed
(from [434e451221cd3d19e4575e26799ee53954347a03]) Filter sensitive query string parameters in the log [#6244 Filter sensitive `QUERY_STRING` parameters in the log state:committed]

This provides more safety to applications that put secret information in the query string, such as API keys or SSO tokens.

Signed-off-by: Xavier Noria fxn@hashref.com
https://github.com/rails/rails/commit/434e451221cd3d19e4575e26799ee...

March 10th, 2011 @ 11:34 PM
Repository

(from [68802d0fbe9d20ef8c5f6626d4b3279bd3a42d3e]) Filter sensitive query string parameters in the log [#6244 Filter sensitive `QUERY_STRING` parameters in the log state:committed]

This provides more safety to applications that put secret information in the query string, such as API keys or SSO tokens.

Signed-off-by: Xavier Noria fxn@hashref.com
https://github.com/rails/rails/commit/68802d0fbe9d20ef8c5f6626d4b32...

March 10th, 2011 @ 11:34 PM
Prem Sichanugrist (sikachu)
Thank you for updating my patch and push them for me. ;)

Jeez, seems like I keep overlooking something.

March 11th, 2011 @ 02:09 AM
bingbing
gucci watches

March 26th, 2011 @ 01:31 AM