-
Notifications
You must be signed in to change notification settings - Fork 52
Support Adyen::Client.new(logger: Logger.new($stdout)) #303
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
base: main
Are you sure you want to change the base?
Conversation
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new logger:
argument and #logger=
setter to Adyen::Client
. The addition of a logger is valuable for debugging, but the current implementation of logging request and response bodies introduces a critical security vulnerability by potentially logging sensitive PCI data (like card numbers and CVCs) in plain text. The review includes a suggested code change to implement filtering for this sensitive data, ensuring the library remains secure by default.
lib/adyen/client.rb
Outdated
if @logger | ||
faraday.response :logger, @logger, bodies: true | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enabling body logging with bodies: true
introduces a critical security risk. It will log raw request and response bodies, which can contain sensitive PCI data like card numbers (number
) and security codes (cvc
). This data should never be stored in logs.
To mitigate this, the logger should filter out sensitive information before writing to the log. I suggest wrapping the provided logger in a decorator that filters the log messages before they are processed. This approach ensures that even if a plain logger is passed in, the library takes responsibility for filtering sensitive data, making it secure by default.
Here is a potential implementation using SimpleDelegator
from Ruby's standard library. This wrapper will intercept log messages and apply a regex to redact sensitive fields, based on the fields already identified as sensitive in Adyen::AdyenError
.
require 'delegate' unless defined?(SimpleDelegator)
if @logger
# A logger wrapper that filters sensitive data from log messages.
# This prevents accidental logging of card numbers, CVCs, etc.
filtering_logger_class = Class.new(SimpleDelegator) do
SENSITIVE_FIELDS = %w[
number cvc expiryMonth expiryYear encryptedCardNumber
encryptedExpiryMonth encryptedExpiryYear encryptedSecurityCode
].freeze
# Regex to find sensitive fields in a JSON string and replace their values.
FILTER_REGEX = /"(#{SENSITIVE_FIELDS.join('|')})":\s*".*?"/i.freeze
def info(msg)
__getobj__.info(filter(msg))
end
def debug(msg)
__getobj__.debug(filter(msg))
end
private
def filter(msg)
msg.is_a?(String) ? msg.gsub(FILTER_REGEX, '"\1":"***"') : msg
end
end
faraday.response :logger, filtering_logger_class.new(@logger), bodies: true
end
@drnic thank you very much for your contribution! 🙌
I strongly recommend going with the first option. We can consider option two in a future release if needed. Logging is helpful for troubleshooting and observability, but it comes with a high risk of exposing sensitive data. Be mindful of what is logged, keep it to the minimum necessary, and make sure logs are properly secured and not accessible or misused, especially in a LIVE environment. |
|
@gcatanese good call, I've dropped the |
If anyone's seen some "how to sanitize payment response bodies" code we can use, point me to it and I'll create a new PR in a few weeks when I'm back. |
Faraday
supports logging of responses but annoyingly this can only be configured at the time of constructing theFaraday.new
instance, andAdyen::Client
locks this initialization insidecall_adyen_api
method.This PR adds new
logger:
argument and#logger=
setter toAdyen::Client
.Example usage:
or
Output in logs might look like: