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

filter form from logs #516

Merged
merged 3 commits into from
Nov 15, 2016
Merged

filter form from logs #516

merged 3 commits into from
Nov 15, 2016

Conversation

lihanli
Copy link
Contributor

@lihanli lihanli commented Nov 14, 2016

closes department-of-veterans-affairs/platform-team#171

Copy link
Contributor

@omgitsbillryan omgitsbillryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the catch-all with form. I think logic belongs in /vets-api/config/initializers/filter_parameter_logging.rb

@@ -47,5 +47,7 @@ class Application < Rails::Application
end

config.middleware.use 'OliveBranch::Middleware'

config.filter_parameters << :form
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to make this more specific? We may have forms in the future that don't need to be filtered. It looks like in Rails 5 you can specify the parent as well, maybe someone has written a port of that?
rails/rails#13897

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a worst-case, we could just filter :education_benefits_claim which sits one level above :form

Copy link
Contributor

@omgitsbillryan omgitsbillryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lihanli lihanli merged commit 1e968d8 into master Nov 15, 2016
@lihanli lihanli deleted the filter-pii-from-logs branch November 15, 2016 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants