Fall back to filter_parameters when filter_redirect is not set for redirect log lines #14055

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
Contributor

trevorturk commented Feb 14, 2014

See also #8404 and 86e3aaa

I noticed that log lines for redirects were not filtered as I expected. I did a bit of searching and I see that we have a newish config option related to this that I wasn't aware of. It's a great option, but I propose that we fallback to params filtering when this new option is not used.

I worked up a crude implementation (steals code from filter_parameters and doesn't include tests) to illustrate the point. I collected some example log lines that show the current/expected behavior.

I'm happy to improve this PR if there's consensus that this is generally a good idea.

Thanks!

-- Current behavior --

Rails.application.config.filter_parameters += [:password]

Started GET "/" for 127.0.0.1 at 2014-02-13 19:58:50 -0600
Processing by ApplicationController#one as HTML
Redirected to http://localhost:3000/two?password=secret
Completed 302 Found in 15ms (ActiveRecord: 0.0ms)

Started GET "/two?password=[FILTERED]" for 127.0.0.1 at 2014-02-13 19:58:50 -0600
Processing by ApplicationController#two as HTML
Parameters: {"password"=>"[FILTERED]"}
Completed 200 OK in 0ms (ActiveRecord: 0.0ms)

I expected the log line "Redirected to http://localhost:3000/two?password=secret" to be filtered.

-- Expected behavior --

Started GET "/" for 127.0.0.1 at 2014-02-13 19:58:50 -0600
Processing by ApplicationController#one as HTML
Redirected to http://localhost:3000/two?password=[FILTERED]
Completed 302 Found in 20ms (ActiveRecord: 0.0ms)

I expected the log line to read: "Redirected to http://localhost:3000/two?password=[FILTERED]"

-- Behavior with the newly introduced redirect filter --

Rails.application.config.filter_redirect << 'password'

Started GET "/" for 127.0.0.1 at 2014-02-13 19:58:50 -0600
Processing by ApplicationController#one as HTML
Redirected to [FILTERED]
Completed 302 Found in 20ms (ActiveRecord: 0.0ms)

With the new redirect filter option in use, the entire URL is filtered: "Redirected to [FILTERED]"

/cc @freegenie -- would love your feedback!

Contributor

freegenie commented Apr 11, 2014

@trevorturk if I understood correctly you are trying to solve two problems:

  1. filter_parameters should address params in redirect strings too
  2. when filtering redirects, hide just the fitlered params not the whole string

About the 1st, I don't know, I'm not sure it's worth. Querystirng in a redirect is not strictly a hash of parameters from Rails' point of view. It's likely to not point to your app at all.

About the 2nd, it sounds totally reasonable.

+ def parameter_filter
+ ParameterFilter.new request.env.fetch("action_dispatch.parameter_filter") {
+ return NULL_PARAM_FILTER
+ }
@jeremy

jeremy Apr 11, 2014

Owner
  • Non-local return within a block leads to confusing control flow. Can check for the param filter in the env first, then instantiate the ParameterFilter.
  • Security by default. If the param filter is missing from the env, filter all params.
+ KV_RE = '[^&;=]+'
+ PAIR_RE = %r{(#{KV_RE})=(#{KV_RE})}
+ def parameter_filtered_location
+ location.gsub(PAIR_RE) do |_|
@jeremy

jeremy Apr 11, 2014

Owner

This catches a=b in the URI path. Meant to check the query string alone?

Would prob be safer to parse the URI, filter the query params, then reconstruct it.

@rafaelfranca rafaelfranca added this to the 4.2.0 milestone Apr 11, 2014

@robin850 robin850 added the security label Apr 12, 2014

@rafaelfranca rafaelfranca modified the milestones: 4.2.0, 5.0.0 Aug 18, 2014

Member

sikachu commented Dec 13, 2015

Closing this PR in favor of #21045. Let's move the discussion there.

@sikachu sikachu closed this Dec 13, 2015

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment