Skip to content
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

Do not suppress errors that occur when creating logs #1194

Closed
BAGELreflex opened this issue Sep 2, 2019 · 2 comments

Comments

@BAGELreflex
Copy link

commented Sep 2, 2019

Describe the bug
If a permission issue occurs when attempting to create log files then the user simply receives an Unhandled Exception warning. The System Administrator is left in the dark because there are no errors in the PHP logs, where an unhandled exception would normally appear, and there are no SimpleSAMLphp logs because the log file or directory may not exist or be writable. This permission issue could be due to a multitude of reasons such as invalid user, invalid permissions on the directory, invalid permissions on a parent directory, invalid permissions on the file, SELinux, directory not existing, etc.

These errors have been intentionally suppressed by using the @.

https://github.com/simplesamlphp/simplesamlphp/blob/master/lib/SimpleSAML/Logger/FileLoggingHandler.php#L60-L71

To Reproduce
Steps to reproduce the behavior:

  1. Create a directory on the filesystem which the apache (or www-data) user would not have permission to create a file in. Doing chown root:root /path/to/file && chmod 550 /path/to/file should emulate this setup.
  2. Configure the loggingdir in config.php to be /path/to/file.
  3. Receive Unhandled Exception with no further details outlining what went wrong or why.
  4. Be very confused because you have no error logs to determine what's wrong.

Expected behavior
This would ideally log to the standard PHP log and allow better handling by way of try/catch around the calls to file_exists, is_writeable and touch, however these functions do not actually throw errors. Instead they simply log to stdout and exit.

Screenshots or logs

if (file_exists($this->logFile)) {
    if (!is_writeable($this->logFile)) {
        throw new \Exception("Could not write to logfile: ".$this->logFile);
    }
} else {
    if (!touch($this->logFile)) {
        throw new \Exception(
            "Could not create logfile: ".$this->logFile.
            " The logging directory is not writable for the web server user."
        );
    }
}

Output:

[Mon Sep 02 16:50:04.039268 2019] [php7:warn] [pid 10853] [client 10.1.113.31:14349] PHP Warning:  touch(): Unable to create file /var/simplesamlphp/log/simplesamlphp.log because Permission denied in /var/simplesamlphp/lib/SimpleSAML/Logger/FileLoggingHandler.php on line 59

This at least gives the administrator information to go on. I'm not sure why legitimate errors would be being suppressed by design.

Additional context

[astclair@dev-auth ~]$ php --version
PHP 7.3.9 (cli) (built: Aug 27 2019 22:52:39) ( NTS )
Copyright (c) 1997-2018 The PHP Group
Zend Engine v3.3.9, Copyright (c) 1998-2018 Zend Technologies
[astclair@dev-auth ~]$ cat /etc/*-release
CentOS Linux release 7.6.1810 (Core)
NAME="CentOS Linux"
VERSION="7 (Core)"
ID="centos"
ID_LIKE="rhel fedora"
VERSION_ID="7"
PRETTY_NAME="CentOS Linux 7 (Core)"
ANSI_COLOR="0;31"
CPE_NAME="cpe:/o:centos:centos:7"
HOME_URL="https://www.centos.org/"
BUG_REPORT_URL="https://bugs.centos.org/"

CENTOS_MANTISBT_PROJECT="CentOS-7"
CENTOS_MANTISBT_PROJECT_VERSION="7"
REDHAT_SUPPORT_PRODUCT="centos"
REDHAT_SUPPORT_PRODUCT_VERSION="7"

CentOS Linux release 7.6.1810 (Core)
CentOS Linux release 7.6.1810 (Core)
@tvdijen tvdijen added the enhancement label Sep 2, 2019
@jaimeperez

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Hi @BAGELreflex!

I might be missing something, but I think the error you get in the browser is pretty much self-explanatory:

image

Could it be that you had disabled showing errors? The showerrors configuration option is enabled by default in the configuration template. If you didn't get any details with the exception message, I'm certain you had showerrors set to false, which is definitely not the best way to figure out what's wrong 😉

My point is that in a way, it's hard to say silencing errors with the @ operator is wrong, when you are also silencing them in the configuration. I see your point and I agree that we should make it easier to find out what's going on in a situation like this, but I'm not sure removing the @ operator to allow those functions print errors is the right way. I think I'd prefer either bail out and log the error with error_log() right before throwing the exception, or detect this situation in the Logger class and dump logs to the web server's error log. But I see little benefit in your approach...

@BAGELreflex

This comment has been minimized.

Copy link
Author

commented Sep 3, 2019

Ah, you are correct. I have implemented my own template for error.php and it is not displaying the $this->data['error']['exceptionTrace'] conditionally based on the showerrors config option.

However, my original argument still seems valid. I believe the showerrors configuration option is intended to show/hide the errors from the UI, not from standard error logs. I can't imagine a scenario where it would be intended to not log a breaking error like this. SimpleSAMLphp doesn't function if it cannot write to its log.

I have just commented out my custom theme and set showerrors to false, while retaining my proposed code changes of removing the @. The UI now does not display the error details and the following error is logged to the standard webserver log (apache in my case, at /var/log/httpd/ssl_error_log)L

[Tue Sep 03 12:49:02.202916 2019] [php7:warn] [pid 12426] [client 10.1.113.31:3324] PHP Warning:  touch(): Unable to create file /var/simplesamlphp/log/simplesamlphp.log because Permission denied in /var/simplesamlphp/lib/SimpleSAML/Logger/FileLoggingHandler.php on line 59

This gives the developer/administrator visibility of the issue at a location where they would normally be looking to debug this problem, IMO. It also removes the need to specifically call error_log(), and I don't know that you can specifically detect this situation in the Logger class because of how the functions being used work. Both file_exists() and is_writeable() specifically document that "Upon failure, an E_WARNING is emitted.". The touch() documentation does not state that, but my testing shows that it functions the same. So, when they fail, they immediately emit the E_WARNING which will write to stderr immediately.

@jaimeperez jaimeperez added this to the 1.18 milestone Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.