Filter redirects #8404

Merged
merged 1 commit into from Dec 5, 2012

Projects

None yet

4 participants

@freegenie

Given the latest discussion on rails-core google group, rather than just discussing this one I'm providing working code. I think Rails is missing a way to filter redirect logs. It makes sense to me, given redirect logs could hold sensible informations the same way POST params do.
The typical example is when you redirect to S3 signed URLs, which is my case. People is sending out logs to third party services for analysis, this feature will at least give them a way to have more control over what they send.

I was thinking about a way to make a gem out of it, but I seems that the current implementation does not leave room for anything to plug other than a monkeypatch. Advices on how to make a gem for this are of welcome of course.

@steveklabnik steveklabnik and 1 other commented on an outdated diff Dec 3, 2012
actionpack/lib/action_controller/redirect_log_filter.rb
@@ -0,0 +1,36 @@
+module ActionController
+ module RedirectLogFilter #:nodoc:
+ extend ActiveSupport::Concern
+
+ FILTERED = '[FILTERED]'.freeze
@steveklabnik
steveklabnik Dec 3, 2012

Is there any particular reason to freeze this string, or is this just defensive?

@freegenie
freegenie Dec 3, 2012

The only reason is I saw it's frozen in actionpack/lib/action_dispatch/http/parameter_filter.rb, so I did it just to be consistent.

@steveklabnik
steveklabnik Dec 3, 2012

That's reasonable.

@steveklabnik steveklabnik and 1 other commented on an outdated diff Dec 3, 2012
actionpack/lib/action_controller/redirect_log_filter.rb
@@ -0,0 +1,36 @@
+module ActionController
+ module RedirectLogFilter #:nodoc:
+ extend ActiveSupport::Concern
+
+ FILTERED = '[FILTERED]'.freeze
+
+ included do
+ config_accessor :filter_redirect_logs
+ extend ClassMethods
@steveklabnik
steveklabnik Dec 3, 2012

Is this needed, since this is a Concern?

@pixeltrix
pixeltrix Dec 3, 2012

No it isn't - @freegenie please remove this line

@steveklabnik
Ruby on Rails member

I like it. :)

@pixeltrix
Ruby on Rails member

I'm assuming that the use case for this is to filter sensitive data out of log files like secure download urls? If so we need some documentation and an example to show this and also a CHANGELOG entry.

@pixeltrix
Ruby on Rails member

Also squash the commits - thanks!

@freegenie

fixed and squashed. Looking to add some docs and CHANGELOG entry.

@freegenie

CHANGELOG and docs added.

@pixeltrix
Ruby on Rails member

Was there a particular reason you put this under ActionController, rather than ActionDispatch like the one for params?

@Trevoke

This feature is required to use Intuit's API - when we redirect to Intuit, the oauth_token shows in the log, and in order to fulfill their security questionnaires, no token can appear in the logs. I'm looking forward to this being added to Rails.

@freegenie

@pixeltrix thanks for pointing this out. I admit that implementing more closely to what we have in params_filter is a very attractive idea. After all they share similar security concerns, and maybe they should also share some common behaviour in terms of code. That would be more consistent, and less surprising for the user. I realized it while editing the guide. But at the same time this is a tiny change, doing it the other way would probably require to go touch a lot of code, expecially if we want to abstract something out from FilterParameters. @steveklabnik what do you think about this? Would it be better to propose this change in an action_dispatch fashon?

@steveklabnik
Ruby on Rails member

If the one for params is in ActionDispatch, it makes sense to put this one there as well. I didn't realize that it was there in the first place; I'm still learning my way around some parts of the codebase, and @pixeltrix knows what he's talking about. :)

@freegenie

Well then, I'm going to rework this a little.

@freegenie

I updated this branch with a second attempt. I kept the previous commit and reverted it just to provide some context for future readers. I'll squash eventually if it gets approved. This time I used the same approach as the one used by filter_parameters. Paths of the two are quite different though, since one is in the scope of the request, while the other one belongs to the reponse.

@pixeltrix pixeltrix commented on an outdated diff Dec 5, 2012
guides/source/action_controller_overview.md
@@ -751,8 +751,12 @@ Now the user can request to get a PDF version of a client just by adding ".pdf"
GET /clients/1.pdf
```
-Parameter Filtering
--------------------
+Log Filtering
+-------------
+
+Rails keeps a log file for each environment in the `log` folder. These are extremely useful when debugging what's actually going on in your application, but in a live application you may not want every bit of information to be stored in the log file.
+
+### Parameters Filtering
Rails keeps a log file for each environment in the `log` folder. These are extremely useful when debugging what's actually going on in your application, but in a live application you may not want every bit of information to be stored in the log file. You can filter certain request parameters from your log files by appending them to `config.filter_parameters` in the application configuration. These parameters will be marked [FILTERED] in the log.
@pixeltrix
pixeltrix Dec 5, 2012

This sentence is repeated.

@pixeltrix
Ruby on Rails member

@freegenie it's looking much better - can you fix the guide and squash the commits and then I'll merge it, thanks!

@freegenie

@pixeltrix Done! Many thanks to you for your precious review.

@pixeltrix pixeltrix merged commit e905639 into rails:master Dec 5, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment