Hide potentially sensitive ActiveJob params from logs #17817
Conversation
| @@ -42,37 +42,45 @@ def set_logger(logger) | |||
|
|
|||
|
|
|||
| def test_uses_active_job_as_tag | |||
| HelloJob.perform_later "Cristian" | |||
| HelloJob.perform_later nil | |||
There was a problem hiding this comment.
Just a cleanup. The argument value had no bearing on the actual test, so it seemed better to pass the minimal thing to get it to pass and remove the distractions.
There was a problem hiding this comment.
There was no distraction. Please leave it as it was is considered cosmetic change.
* Show GlobalID instead of full object .inspect output
|
I don't like the [FILTERED] approach. It should just show the GID URI instead of doing #inspect on the object itself. |
|
@dhh what about basic types that have no GID? Based on the assumption that it's easier to add logs than take away, it didn't seem right to leave those params without the ability to filter them. |
|
I don’t see enough of a compelling reason that you should be passing secrets as basic types to the jobs where logging should be filtering like this. Route it through a GID-compatible layer like AR or leave it as an exercise for the programmer.
|
|
Updated with those changes. I still think there is a totally fine use case for passing arbitrary (including sensitive) parameters to ActiveJob - if you're using a backend based on Redis or a MQ, they're probably significantly faster than actually inserting or updating rows in your primary database, so you can delay as much of that as possible until later without slowing down a web request. |
| @@ -84,8 +84,21 @@ def queue_name(event) | |||
| event.payload[:adapter].name.demodulize.remove('Adapter') + "(#{event.payload[:job].queue_name})" | |||
| end | |||
|
|
|||
| def global_id_or_inspected(argument) | |||
| if argument.is_a?(GlobalID::Identification) | |||
| argument.to_global_id.to_s | |||
There was a problem hiding this comment.
Why call #to_global_id again if you just checked that the argument already is?
There was a problem hiding this comment.
It's checking if the argument is a model that mixes in GlobalID::Identification, not if it's a GlobalID object. Calling .to_s on a regular AR object just results in an unhelpful #<Person:0x007fe1717a8370>.
There was a problem hiding this comment.
Ah, right. We should just use #try instead.
GlobalID objects are logged by their URI, not #inspect on the object, to prevent logging private data
GlobalID objects are logged by their URI, not #inspect on the object, to prevent logging private data
Latest iteration of #17746.