Update list of filtered parameters - #2735
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new :auth filter is overly broad (can filter non-sensitive author* params) and the local re-raise should preserve backtraces to avoid harming debugging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR expands sensitive-parameter filtering for exception notifications/logging and adds an integration test to verify that secrets (notably authorization headers and common credentials) are scrubbed from exception emails. It also adjusts exception handling/configuration to ensure exception notifications can be exercised in non-production environments for testing.
Changes:
- Add an integration test ensuring exception notification emails filter credentials while allowing non-sensitive params through.
- Update controller error handling to make behavior testable by always registering the rescue handler and re-raising in local-request mode.
- Update exception notification setup/logging and extend filtered parameter list to include authorization-related data.
File summaries
| File | Description |
|---|---|
| test/integration/exception_notification_test.rb | Adds an integration test asserting sensitive request data is filtered from exception notification emails. |
| lib/seek/errors/exception_forwarder.rb | Fixes a log message typo and adjusts exception-notifier delivery error handling. |
| lib/seek/errors/controller_error_handling.rb | Makes exception rescue behavior consistent across environments while re-raising locally. |
| lib/seek/config.rb | Allows exception notification configuration when enabled (no longer production-only), supporting test usage. |
| config/initializers/filter_parameter_logging.rb | Extends the global parameter filter list to cover authorization-related keys. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def render_application_error(exception) | ||
| raise exception if Rails.application.config.consider_all_requests_local | ||
| logger.error "ERROR - #{exception.class.name} (#{exception.message})" |
There was a problem hiding this comment.
Not sure this is true
Also add a test (and tweak exception handling logic to make it possible to test)