Sanitize event data before they are sent to async job. #936
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi guys, is there a possibility to address this issue? Your proposed solution in the readme for the async job: config.async = lambda { |event| SentryJob.perform_later(event) }
class SentryJob < ActiveJob::Base
queue_as :default
def perform(event)
Raven.send_event(event)
end
endleads to writing the sensitive informations (passwords, Authorizatino headers, tokens, etc) to the log in readable form in probably hundreds maybe even thousands of apps (according to |
|
@edariedl But all the processors would run twice then? |
|
@HazAT Thank you for the reply. You are right, processor would run twice in my proposed solution. They would run for the first time before the data will be sent to the async job and second time in the background job before the data are sent to the sentry. But it shouldn't be a problem second run is not necessary but it also does not do any harm. Active job is alway logging all of the parameters when the job is Enqueued and again when the job is started. Steps to simulate the problemI created a new rails 6.0.2 application, added sentry gem and one scaffold controller with parameters class UsersController < ApplicationController
before_action :set_user, only: [:show, :edit, :update, :destroy]
def index
@users = User.all
end
def new
@user = User.new
end
def create
@user = User.new(user_params)
raise "Something bad happend" # <------------ Exception
if @user.save
redirect_to @user, notice: 'User was successfully created.'
else
render :new
end
end
private
def set_user
@user = User.find(params[:id])
end
def user_params
params.require(:user).permit(:name, :password)
end
endSentryJob class SentryJob < ApplicationJob
def perform(event)
Raven.send_event(event)
end
endSentry configuration: Raven.configure do |config|
environments = %w(production development)
# dns is just some random characters
config.dsn = "https://adfvuopasdnfjbasajfsfdsd:adfvuopasdnfjbasajfsfdsd@sentry.io/112576"
config.environments = environments
config.current_environment = Rails.env
config.sanitize_fields = Rails.application.config.filter_parameters.map(&:to_s)
config.async = lambda { |event|
SentryJob.perform_later(event.to_hash)
}
config.silence_ready = true
endFiltered params initializer: # Configure sensitive parameters which will be filtered from the log file.
Rails.application.config.filter_parameters += [:password]I started the rails server and performed the following request to the This is ActiveJob part of the log: ActiveJob log is super long so here is just the relevant part: As you can see, the password is in the log in the plain text even if it definitely shouldn't be. |
HazAT
approved these changes
Dec 17, 2019
|
Awesome, thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All of the error parameters should be sanitized before they are passed to the async job.
When sentry is configured to send data asynchronously, all the data passed to the async job are not sanitized. This can cause potential security problems. If you are using background job processing to send data to sentry, unsanitized parameters are send to the background job and sometimes they are logged to the application log.
Eg. ActiveJob is logging all of the data captured by sentry, currently including passwords, tokens, cookies, Authorization headers, etc.. Sanitization is done during background job processing just before data are sent to Sentry.
Changes proposed by this pull request should fix it.