Sidekiq exception reporting should automatically filter job arguments based on Rails config?

[tekin]

Hi,

We recently had an issue with our SMTP provider which resulted in ActionMailer emails that were scheduled to be delivered later (Sidekiq processing jobs queued by ActiveJob) to fail. The resulting exception notifications in AppSignal included the job args as the parameters, which in this case included sensitive data (email addresses and security tokens). My question is: should the Sidekiq integration automatically sanitize the job arguments using the configured filters in Rails.application.config.filter_parameters in the same way the Rails Instrumentation uses filtered_parameters params in request contexts?

As an example, it's common to do something like this to send an authentication email to a user:

class VerificationMailer < ApplicationMailer
  default to: -> { params.fetch(:email) }

  def start
    @token = params.fetch(:token)
    mail
  end
end

VerificationMailer.with(
  email: 'recipient@example.com',
  token: 'secure-123'
).start.deliver_later

If this failed to be delivered in a background job, the parameters sent to AppSignal would look like:

[
  "VerificationMailer",
  "start",
  "deliver_now",
  {
    "_aj_ruby2_keywords": [
      "params",
      "args"
    ],
    "args": [],
    "params": {
      "_aj_symbol_keys": [
        "email",
        "token"
      ],
      "email_address": "recipient@example.com",
      "token": "secure-123"
    }
  }
]

I appreciate it's possible to configure filter_parameters directly in AppSignal, but can't help but feel default filtering based on the filtering already configured in Rails would be a good baseline and prevent potential leaking of sensitive data.

[tombruijn]

Hi @tekin, the way we fetch the request parameters in Rails we immediately get the filtered result; we don't read from the Rails filter_parameters config to amend to ours, so that's why they're not filtered by default for every gem. Rails and Sidekiq are different gems. In the same way that Sidekiq's logs don't filter out any sensitive data based on the Rails settings, we don't either.

At the moment, I don't think I want to fetch the Rails config to get the filter_parameters option when we filter all parameters, in case some apps don't want the Rails config to apply to all parameters. We'd be filtering the same keys twice for Rails requests as well.

The way we configure AppSignal for Rails also means we can't read the configuration at boot because we start before any Rails initializers are run. We could change that, we already have an option for it. Then apps could amend the Rails config easily without duplicating the filter list.

# config/initializers/filter_parameter_logging.rb

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

Appsignal.configure do |config|
  # THIS DOES NOT WORK WITHOUT `config.appsignal.start_at = :after_initialize`
  config.filter_parameters += Rails.application.config.filter_parameters
end

Let me think about it and discuss it with the team.

[tekin]

Hi @tombruijn.

That all makes sense. For me I think it would make sense when both Rails and Sidekiq are present for the Sidekiq parameters to get filtered using the same list as configured in Rails to avoid this unexpected gotcha and ship with safer defaults. I appreciate that under some circumstances users may not want that to happen (although I can't help but feel it must be quite rare).

One thing that may be worth doing is updating the docs to make it clearer that background jobs do not automatically get the same filtering as Rails requests. Someone reading the section on request filtering might not get the nuance that this won't also apply to thieir background jobs.

We've got around the issue configuring filter parameters and application boot this way:

Rails.application.config.after_initialize do
  Appsignal.configure do |config|
    config.filter_parameters += Rails.application.config.filter_parameters
  end
end

[tombruijn]

We've got around the issue configuring filter parameters and application boot this way:

This may work on Ruby gem version 3, but will not work on Ruby gem version 4. Even when Rails is configured with config.appsignal.start_at = :after_initialize that Appsignal.configure block is not called because AppSignal is configured either before or right after the initialize process.

You'll see this message in the appsignal.log file. That's what I've found in all combinations I've tried.

AppSignal is already started. Ignoring Appsignal.configure call.

Best is to move the code to an initializer and remove the Rails.application.config.after_initialize do block around it.

One thing that may be worth doing is [...]

We'll have a look at how we can improve this :)

[tekin]

This may work on Ruby gem version 3, but will not work on Ruby gem version 4. Even when Rails is configured with config.appsignal.start_at = :after_initialize that Appsignal.configure block is not called because AppSignal is configured either before or right after the initialize process.

You'll see this message in the appsignal.log file. That's what I've found in all combinations I've tried.

AppSignal is already started. Ignoring Appsignal.configure call.

OK, thanks for pointing this out @tombruijn. I think we'll revert to duplicating the filtered params in the YAML config as losing boot error reporting would be a shame.

[tombruijn]

I think we'll revert to duplicating the filtered params in the YAML config as losing boot error reporting would be a shame.

Ok!

Thinking about it, I don't see a way to load the Rails filter_parameters config into the AppSignal config, in such a way that it can be modified from an initializer, if we don't start AppSignal after the initializers have run. Like this:

Appsignal.configure do |config|
  config.filter_parameters -= [:token]
end

And I don't want to apply it to all parameters/arguments reported, from other gems too.

So I can see a future version of the Ruby gem change the start moment to after Rails has started to allow for configuration from the Rails initializers.

losing boot error reporting would be a shame.

I understand this is a useful feature, but I also don't think the way it's done now is the best way, from within the application process itself. There's no guarantee that AppSignal started before the app has an error during its boot. Ideally, we have some kind of fully configured process wrapper that captures any process exit failure and reports that. I know @unflxw has brought up this idea before, and maybe this CC can inspire her :)

[unflxw]

Wrapper process idea aside, the way we solve this problem in the Elixir integration, s/Rails/Phoenix, is by doing that part of the filtering on the integration side, not on the agent side: https://github.com/appsignal/appsignal-elixir/blob/72e07ed42c61e91097d319c47b250837070c0c06/lib/appsignal/utils/map_filter.ex#L5

[tombruijn]

In the Ruby gem we ask Rails to filter the params for us. This because you can also pass in lambdas to filter parameters, which we do not support.

The params_method option in the Rails middleware configures the AbstractMiddleware, which tells the ApplyRackRequest internal helper to read from that request method on the Rails request class.