CyberSource / cybersource-rest-client-php

PHP client library for the CyberSource REST API
30 stars 66 forks source link

Monolog Attempts to write files when "disabled". #98

Closed TechN8 closed 2 years ago

TechN8 commented 2 years ago

On our server, even when setting enableLogging(false) in the merchant config, we sometimes see attempts to write to MonoLog files.

It looks like the constructor for MerchantConfiguration sets it's own static logger before the logConfig has been set so this one is always using default settings.

    /**
     * Constructor
     */
    public function __construct()
    {
        $this->tempFolderPath = sys_get_temp_dir();
        $this->logConfig = new LogConfiguration();

        if (self::$logger === null) {
            self::$logger = (new LogFactory())->getLogger(\CyberSource\Utilities\Helpers\ClassHelper::getClassName(get_class()), $this->logConfig);
        }
    }

So when we do

$logConfiguration = new LogConfiguration();
$logConfiguration->enableLogging(false);
$merchantConfig->setLogConfiguration($logConfiguration);

It's too late to stop all log messages.

UnexpectedValueException: There is no existing directory at 
 "/mnt/www/html/xxx/vendor/cybersource/rest-client-php/lib/Logging/../../../../../Log"
 and it could not be created: Permission denied in Monolog\Handler\StreamHandler->createDir()
 (line 216 of /mnt/www/html/xxx/vendor/monolog/monolog/src/Monolog/Handler/StreamHandler.php)

Our filesystem is read-only. How can we totally disable Monolog?

omerida commented 2 years ago

I've run into this as well and there's no direct way to configure a different PSR compliant instance (even though one would work). I resorted to using the Reflection API to set $logger early and bypass the hard coded usage:

For example:

/**
 * Get an instance of the requested class with null logger configured for it.
 * 
 * @template T
 * @param class-string<T> $className 
 * @param mixed $params
 * @return T
 */
private static function getCSInstance(string $className, ...$params) : mixed {
    // the parent class declares logger as private and insists
    // on creating a file based logger even if you disable logging.
    $logger = new Logger('null');
    $logger->pushHandler(new NullHandler());

    $property = new \ReflectionProperty($className, "logger");
    $property->setAccessible(true);
    $property->setValue($logger);

    // continue setup as normal
    return new $className(...$params);
}

When I need a CyberSource client or class, I instantiate it like:

$client = getCSInstance(\CyberSource\ApiClient::class, $config, $merchantConfig);
gnongsie commented 2 years ago

Hi @TechN8,

Thank you for bringing this up. I have checked this out and I am not able to reproduce this issue.

I agree that the constructor for the MerchantConfiguration class creates a static logger internally before it processes the LogConfiguration object from the application. But the constructor creates a new instance of the LogConfiguration class for itself.

    /**
     * Constructor
     */
    public function __construct()
    {
        $this->tempFolderPath = sys_get_temp_dir();
        $this->logConfig = new LogConfiguration();   <--- This line create a new instance of LogConfiguration

        if (self::$logger === null) {
            self::$logger = (new LogFactory())->getLogger(\CyberSource\Utilities\Helpers\ClassHelper::getClassName(get_class()), $this->logConfig);
        }
    }

If you take a look at the constructor of the LogConfiguration class, you will see that logging is automatically disabled unless explicitly enabled.

    /**
     * Constructor
     */
    public function __construct()
    {
        $this->enableLogging = false;   <--- Logging is disabled
        $this->debugLogFile = __DIR__ . DIRECTORY_SEPARATOR . ".." . DIRECTORY_SEPARATOR . ".." . DIRECTORY_SEPARATOR . ".." . DIRECTORY_SEPARATOR . ".." . DIRECTORY_SEPARATOR . ".." . DIRECTORY_SEPARATOR . "Log" . DIRECTORY_SEPARATOR . "debug.log";
        $this->errorLogFile = __DIR__ . DIRECTORY_SEPARATOR . ".." . DIRECTORY_SEPARATOR . ".." . DIRECTORY_SEPARATOR . ".." . DIRECTORY_SEPARATOR . ".." . DIRECTORY_SEPARATOR . ".." . DIRECTORY_SEPARATOR . "Log" . DIRECTORY_SEPARATOR . "error.log";
        $this->logDateFormat = "Y-m-d\TH:i:s";
        $this->logFormat = "[%datetime%] [%level_name%] [%channel%] : %message%\n";
        $this->logMaxFiles = 3;
        $this->logLevel = "info";
        $this->enableMasking = false;
    }

In effect, this line is not causing any logging statements by default.

In order to assist you better, can you provide the following information :

  1. Version of the SDK being used
  2. Error message caused by this issue, where you see it is trying to write to log
  3. Any additional lines of code that you may have pertaining to MerchantConfiguration objects or LogConfiguration objects or using the LogFactory class
omerida commented 2 years ago

@gnosie the enableLogging property does not affect the creation of the $logger object because of the code you highlighted:

        if (self::$logger === null) {
            self::$logger = (new LogFactory())->getLogger(\CyberSource\Utilities\Helpers\ClassHelper::getClassName(get_class()), $this->logConfig);
        }

Regardless of what vlue you set enableLogging to, when the __construct method of any class is first called, that property will be null and a new logger instance will be created. All (most?) of the API classes expect that self::$logger has a valid Monolog instance that they can use regardless of the value of enableLogging.

If enableLogging is false, you can setup a null lugger without breaking other code. The best place to do this is likely LogFactory.php::createNewLogger():

private function createNewLogger($loggerName, $logConfig) {

       if ($logConfig->enableLogging == false) {
            $logger = new Logger('null');
            $logger->pushHandler(new \Monolog\Handler\NullHandler());
                return $logger;
        }

        // logging is enabled, proceed as before
        $dateFormat = $logConfig->getLogDateFormat();
        $logFormat = $logConfig->getLogFormat();

        // For DEBUGGING LOG FILE
        $logFile = $logConfig->getDebugLogFile();
        $maxFiles = $logConfig->getLogMaxFiles();
        if (null !== $logConfig->getLogLevel()) { $logLevel = $logConfig->getLogLevel(); } else { $logLevel = Logger::INFO; }
        $formatter = new LineFormatter($logFormat, $dateFormat, true, true);
        $debugHandler = new RotatingFileHandler($logFile, $maxFiles, $logLevel);
        $debugHandler->setFormatter($formatter);

        // For ERROR LOG FILE
        $errorLogFile = $logConfig->getErrorLogFile();
        $errorMaxFiles = $logConfig->getLogMaxFiles();
        $errorLogLevel = Logger::ERROR;
        $errorFormatter = new LineFormatter($logFormat, $dateFormat, true, true);
        $errorHandler = new RotatingFileHandler($errorLogFile, $errorMaxFiles, $errorLogLevel);
        $errorHandler->setFormatter($errorFormatter);

        $logger = new Logger($loggerName);
        $logger->pushHandler($errorHandler);

        if ($logConfig->isLoggingEnabled()) {
            $logger->pushHandler($debugHandler);
        }

        return $logger;
    }
omerida commented 2 years ago

@gnongsie Ideally, the code that uses the cybersource SDK client should be able to pass in ANY Monolog logger for the SDK to use. It should not assume that all application can/will log to the filesystem

omerida commented 2 years ago

Finally, you can replicate that logging isn't disabled in at least one spot with the latest SDK

  1. Using latest SDK, configure it with logging disabled and set to write somewhere it does not have permissions (invalid path or without write privileges).
  2. Instantiate the TransactionsAPI
  3. Try to fetch transaction details for an ID that does not exist.
  4. Response should be a 404 with an error that logging isn't available via an UnexpectedValueException
TechN8 commented 2 years ago

Thanks Oscar! This is similar to what I observed.

gnongsie commented 2 years ago

Thank you @omerida. This is immensely helpful.

I see that the issue is with the error logger. If you see the following lines, you will notice that the error logger is not covered by the enableLogging property.

        $logger = new Logger($loggerName);
        $logger->pushHandler($errorHandler);  <--- Specifically this line

        if ($logConfig->isLoggingEnabled()) {
            $logger->pushHandler($debugHandler);
        }

For context here, I believe this line was done with the intention that the application developer would want to see if an error occurred and why it occurs. It did not occur to me that there are some systems where you cannot write into the filesystem.

Rest assured, we will have this fix released in the next version.

rsachan8 commented 2 years ago

Fixed in [0.0.32] version.