Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `inspection_masks` to make values of sensitive database columns w… #33756

Merged
merged 1 commit into from Sep 7, 2018

Conversation

@piecehealth
Copy link
Contributor

@piecehealth piecehealth commented Aug 30, 2018

…on't be exposed while call #inspect.

  • Why:
    Some sensitive data will be exposed in log accidentally, e.g.
account = Account.find params[:id]
payload = { account: @account }
logger.info "payload will be #{ payload }"

All the information of account will be exposed in log.

  • Solution:
    Add a class attribute inspection_masks to specify which values of columns shouldn't be exposed.
class Account < ActiveRecord::Base
  self.inspection_masks = [:credit_card_number]
end

Account.last.insepct # => #<Account id: 123, credit_card_number: [FILTERED] ...>

Summary

Provide a general description of the code changes in your pull
request... were there any bugs you had fixed? If so, mention them. If
these bugs have open GitHub issues, be sure to tag them here as well,
to keep the conversation linked together.

Other Information

If there's anything else that's important and relevant to your pull
request, mention that information here. This could include
benchmarks, or other information.

If you are updating any of the CHANGELOG files or are asked to update the
CHANGELOG files by reviewers, please add the CHANGELOG entry at the top of the file.

Finally, if your pull request affects documentation or any non-code
changes, guidelines for those changes are available
here

Thanks for contributing to Rails!

@piecehealth piecehealth force-pushed the piecehealth:pr branch Aug 30, 2018
activerecord/lib/active_record/core.rb Outdated
@@ -8,6 +8,8 @@ module ActiveRecord
module Core
extend ActiveSupport::Concern

FILTERED = "[FILTERED]".freeze # :nodoc:

This comment has been minimized.

@rafaelfranca

rafaelfranca Aug 31, 2018
Member

Don't need to freeze this string.

@piecehealth
Copy link
Contributor Author

@piecehealth piecehealth commented Sep 1, 2018

Hi @rafaelfranca , I have removed freeze per your comment, please take a look, thanks.

@eileencodes
Copy link
Member

@eileencodes eileencodes commented Sep 1, 2018

This is great! I'm wondering though, would it be better to piggyback on the config.filter_parameters option and by default always filter those in inspect? I'd imagine you wouldn't want credit card number in inspect or in the logs, so it makes sense to me to use the same list. Thoughts?

@eileencodes eileencodes added this to the 6.0.0 milestone Sep 1, 2018
@piecehealth
Copy link
Contributor Author

@piecehealth piecehealth commented Sep 2, 2018

Hi @eileencodes , thanks for your advise, I checked in a new commit based on your comment.

I don't know how to write test about railtie part, but I do test on my local 😉

@piecehealth piecehealth closed this Sep 2, 2018
@piecehealth piecehealth reopened this Sep 2, 2018
@piecehealth piecehealth closed this Sep 2, 2018
@piecehealth
Copy link
Contributor Author

@piecehealth piecehealth commented Sep 2, 2018

trigger CI

@piecehealth piecehealth reopened this Sep 2, 2018
@eileencodes
Copy link
Member

@eileencodes eileencodes commented Sep 2, 2018

@piecehealth you should be able to add a test to railties/test/application/configuration_test.rb that asserts that inspection_masks are equivalent to filter_parameters.

I'm not sure if inspection_masks is my favorite phrasing. I'm thinking it should match the filter_parameters and be called filter_attributes. I need a second opinion though. @rafaelfranca what do you think?

@matthewd
Copy link
Member

@matthewd matthewd commented Sep 3, 2018

I'm not sure if inspection_masks is my favorite phrasing. I'm thinking it should match the filter_parameters and be called filter_attributes.

I agree it'd be better to have them sound more related. My only worry about filter_attributes in particular would be that it could also describe ignored_columns... OTOH, the same could be said about filter_parameters vs strong params. ¯\_(ツ)_/¯

We definitely also need to cover pretty_print below, which is basically another more complicated inspect method. But beyond that, the other place we show attribute values is SQL logs; we can't do much about it if prepared statements is off, but at least when it's on, we could suppress the values.

Finally, on a behavioural note: I suggest we do not mask the value if it's nil (or maybe blank?).

@piecehealth piecehealth force-pushed the piecehealth:pr branch Sep 3, 2018
@piecehealth
Copy link
Contributor Author

@piecehealth piecehealth commented Sep 3, 2018

Thanks @eileencodes @matthewd for your review, I have made some changes according your comments.

  • Change class variable name: inspection_masks -> filter_attributes.
  • Add test case for railtie.
  • Don't mask nil value.
  • Also support pretty_print.

By the way, it's really hard for non English speaker to name variable 😢

activerecord/lib/active_record/core.rb Outdated
@@ -487,12 +493,17 @@ def connection_handler

# Returns the contents of the record as a nicely formatted string.
def inspect
filter_attributes = self.filter_attributes.map(&:to_s)

This comment has been minimized.

@eugeneius

eugeneius Sep 3, 2018
Member

Since this will have include? called on it for each attribute in the model, I think we should convert it to a set (include? is linear time for an array, but constant time for a set).

It might also be worth doing the conversion on assignment, as we do for ignored_columns:

def ignored_columns=(columns)
@ignored_columns = columns.map(&:to_s)
end

activerecord/lib/active_record/railtie.rb Outdated
@@ -235,5 +235,13 @@ class Railtie < Rails::Railtie # :nodoc:
end
end
end

initializer "active_record.set_filter_attributes" do
config.after_initialize do

This comment has been minimized.

@eugeneius

eugeneius Sep 3, 2018
Member

This is unnecessary, and in fact it causes a problem: if the user sets filter_attributes in an initializer, their configuration will be overwritten by filter_parameters and lost.

It might be worth adding a test for this case.

This comment has been minimized.

@piecehealth

piecehealth Sep 3, 2018
Author Contributor

Thanks for your valuable advice, I just wonder is it necessary for user to set filter_attributes in an initializer? Since user could set filter_attributes in ApplicationRecord e.g.

class ApplicationRecord < ActiveRecord::Base
  self.abstract_class = true
  self.filter_attributes = [:credit_card_number]
end

Otherwise, I should add a new instance variable @filter_attributes in Rails::Application::Configuration, please correct me if there is any mistake, thanks 😉

This comment has been minimized.

@eugeneius

eugeneius Sep 3, 2018
Member

Any option on ActiveRecord::Base can be configured by setting a key on Rails.application.config.active_record. I was thinking of someone adding an initializer that mirrors the one for filter_parameters, since they're so similar:

https://github.com/rails/rails/blob/e6ba30efbf107954db2707410d78d77680d69995/railties/lib/rails/generators/rails/app/templates/config/initializers/filter_parameter_logging.rb.tt

activerecord/lib/active_record/railtie.rb Outdated
initializer "active_record.set_filter_attributes" do
config.after_initialize do
ActiveSupport.on_load(:active_record) do
ActiveRecord::Base.filter_attributes = Rails.application.config.filter_parameters

This comment has been minimized.

@eugeneius

eugeneius Sep 3, 2018
Member

These hooks are run in the context of ActiveRecord::Base, and it's idiomatic to use self instead of referring to the constant explicitly, e.g:

initializer "active_record.initialize_database" do
ActiveSupport.on_load(:active_record) do
self.configurations = Rails.application.config.database_configuration

@piecehealth piecehealth force-pushed the piecehealth:pr branch 3 times, most recently Sep 3, 2018
Copy link
Member

@rafaelfranca rafaelfranca left a comment

Awesome! Very good patch. Can you squash your commit?

…ensitive value of database column when call `#inspect`

* Why
Some sensitive data will be exposed in log accidentally by calling `#inspect`, e.g.

```ruby
@account = Account.find params[:id]
payload = { account: @account }
logger.info "payload will be #{ payload }"
```

All the information of `@account` will be exposed in log.

* Solution
Add a class attribute filter_attributes to specify which values of columns shouldn't be exposed.
This attribute equals to `Rails.application.config.filter_parameters` by default.

```ruby
Rails.application.config.filter_parameters += [:credit_card_number]
Account.last.insepct # => #<Account id: 123, credit_card_number: [FILTERED] ...>
```
@piecehealth piecehealth force-pushed the piecehealth:pr branch to 180dcd1 Sep 7, 2018
@piecehealth
Copy link
Contributor Author

@piecehealth piecehealth commented Sep 7, 2018

@rafaelfranca @eileencodes thanks for your time, I have squashed my commits and the CI is passed 😃

@eileencodes eileencodes merged commit 2a470d7 into rails:master Sep 7, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@piecehealth piecehealth deleted the piecehealth:pr branch Sep 7, 2018
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 10, 2018
Add mention that `config.filter_parameters` also filters out sensitive
values of database columns when call `#inspect` since rails#33756.
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 10, 2018
- Move
  ```
  filter_attributes = self.filter_attributes.map(&:to_s).to_set

  filter_attributes.include?(attribute_name) && !read_attribute(attribute_name).nil?
  ```
  to private method.

- Fix tests in `activerecord/test/cases/filter_attributes_test.rb`
  - Ensure that `teardown` sets `ActiveRecord::Base.filter_attributes` to
    previous state.

  - Ensure that `Admin::Account.filter_attributes` is set to previous
    state in the "filter_attributes could be overwritten by models" test.

Follow up rails#33756
@bogdanvlviv bogdanvlviv mentioned this pull request Sep 10, 2018
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 11, 2018
Add mention that `config.filter_parameters` also filters out sensitive
values of database columns when call `#inspect` since rails#33756.
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 11, 2018
- Move
  ```
  filter_attributes = self.filter_attributes.map(&:to_s).to_set

  filter_attributes.include?(attribute_name) && !read_attribute(attribute_name).nil?
  ```
  to private method.

- Fix tests in `activerecord/test/cases/filter_attributes_test.rb`
  - Ensure that `teardown` sets `ActiveRecord::Base.filter_attributes` to
    previous state.

  - Ensure that `Admin::Account.filter_attributes` is set to previous
    state in the "filter_attributes could be overwritten by models" test.

Follow up rails#33756
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 12, 2018
Add mention that `config.filter_parameters` also filters out sensitive
values of database columns when call `#inspect` since rails#33756.
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Sep 12, 2018
- Move
  ```
  filter_attributes = self.filter_attributes.map(&:to_s).to_set

  filter_attributes.include?(attribute_name) && !read_attribute(attribute_name).nil?
  ```
  to private method.

- Fix tests in `activerecord/test/cases/filter_attributes_test.rb`
  - Ensure that `teardown` sets `ActiveRecord::Base.filter_attributes` to
    previous state.

  - Ensure that `Admin::Account.filter_attributes` is set to previous
    state in the "filter_attributes could be overwritten by models" test.

Follow up rails#33756
ruby-bench-server pushed a commit to tgxworld/rails that referenced this pull request Sep 12, 2018
Follow up rails#33756
@eliotsykes
Copy link
Contributor

@eliotsykes eliotsykes commented Sep 17, 2018

Pleased to see this, lovely addition 👍

Currently it will behave inconsistently for apps configured with regex or callable filter_parameters.

For those apps, affected request parameters successfully filtered in the logs will not be filtered in #inspect.

Related examples from actionpack/lib/action_dispatch/http/filter_parameters.rb docs:

    # Allows you to specify sensitive parameters which will be replaced from
    # the request log by looking in the query string of the request and all
    # sub-hashes of the params hash to filter. Filtering only certain sub-keys
    # from a hash is possible by using the dot notation: 'credit_card.number'.
    # If a block is given, each key and value of the params hash and all
    # sub-hashes are passed to it, where the value or the key can be replaced using
    # String#replace or similar methods.
    #
    #   env["action_dispatch.parameter_filter"] = [:password]
    #   => replaces the value to all keys matching /password/i with "[FILTERED]"
    #
    #   env["action_dispatch.parameter_filter"] = [:foo, "bar"]
    #   => replaces the value to all keys matching /foo|bar/i with "[FILTERED]"
    #
    #   env["action_dispatch.parameter_filter"] = [ "credit_card.code" ]
    #   => replaces { credit_card: {code: "xxxx"} } with "[FILTERED]", does not
    #   change { file: { code: "xxxx"} }
    #
    #   env["action_dispatch.parameter_filter"] = -> (k, v) do
    #     v.reverse! if k =~ /secret/i
    #   end
    #   => reverses the value to all keys matching /secret/i
@reckerswartz
Copy link

@reckerswartz reckerswartz commented Oct 3, 2018

can this be used or improved for
the issue on devise to prevent password reset tokens leak?
https://github.com/plataformatec/devise#password-reset-tokens-and-rails-logs

yskkin added a commit to yskkin/rails that referenced this pull request Oct 17, 2018
AR instance support `filter_parameters` since rails#33756.
Though Regex or Proc is valid as `filter_parameters`,
they are not supported as AR#inspect.

I also add :mask option to `ActiveSupport::ParameterFilter#new` to
implement this.
yskkin added a commit to yskkin/rails that referenced this pull request Oct 19, 2018
AR instance support `filter_parameters` since rails#33756.
Though Regex or Proc is valid as `filter_parameters`,
they are not supported as AR#inspect.

I also add :mask option and #filter_params to
`ActiveSupport::ParameterFilter#new` to implement this.
suketa added a commit to suketa/rails_sandbox that referenced this pull request May 21, 2019
Configuration item config.filter_parameters could also filter out
sensitive values of database columns when call #inspect
rails/rails#33756
suketa added a commit to suketa/rails_sandbox that referenced this pull request May 22, 2019
Configuration item config.filter_parameters could also filter out
sensitive values of database columns when call #inspect
rails/rails#33756
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.