Site Health: log errors to public file

[afragen]

As mentioned in #47046 just because WP_DEBUG_LOG is true doesn't mean the file is publicly viewable.

Obviously viewable files are in the ABSPATH usually in wp-content somewhere. A simple test against ABSPATH could be used to move the error from critical to recommended. It still shows as a potential problem but possibly not as severe as an average user might infer.

[afragen]

test error_log file path to ABSPATH

[afragen]

The patch is pretty straightforward. What it really needs is a #committer to see if the logic is sound.

[Clorith]

There's no way to reliably know if a path is accessible to others in any way, so I stand by my previous opinion that if logging is enabled, it should alert the user, they are the only ones that can know if it's available or not.

[afragen]

@Clorith my thought was that if the log is not in the normal flow of the WordPress structure then the notification should be less severe.

If the log isn’t in the normal WordPress flow it would be much more difficult to randomly find it.

[afragen]

improved logic

[kirasong]

@miss_jwo reached out in #hosting_community for feedback -- putting that in here as well, so it's easier to find:

I think it’s a good idea to alert folks if they have logging enabled, whether or not the log is public, since a log filling up disk is a real concern, if they’re not handling rotation and such.
It’s possible that it’ll slow down the site due to writes, too.

[afragen]

@mikeschroder it does register an alert. It simply changes the alert from critical to recommended.

[kirasong]

@afragen Is there a place where I could see what the severity change would look like in practice? What would it mean for users to change it?

[afragen]

more specific patch refresh

[afragen]

screenshot with patch applied. WP_DEBUG is true and WP_DEBUG_LOG is true

[afragen]

@mikeschroder I discovered that the previous patch didn't work as expected. I've updated the patch to be as specific as possible. The screenshot is what happens when the patch is applied and WP_DEBUG and WP_DEBUG_LOG are both true if the log is outside of ABSPATH.

Otherwise the error shows as critical.

To test you would need to add something like the following line above the patch.
ini_set( 'error_log', '/tmp/debug.log' );

[Clorith]

#48961 was marked as a duplicate.

[Clorith]

With the changes made in 5.3 on how we weight issues, a potentially public logfile that could contain any manner of details would not be as discoverable if you lower the severity to a recommendation.

This should still be considered potentially critical in my opinion, and whomever has set up the logging would then either be an average user that disables it, or a more technical user that knows the file isn't a problem while using it, and that removes it once done and that severe warning serves as a reminder (us technical folks forget things too after all).

In short, I'm still not convinced we should reduce the severity of (potentially) public logs.

[kubiq]

We should check it with CURL if that file is really public.
For example, in all my WP installations I have enabled WP_DEBUG_LOG and I placed there also .htaccess file with this content:

RemoveHandler .log
RemoveType .log

<Files *.log>
	order deny,allow
	deny from all
</Files>

So debug.log is definitely not public, it can be accessed only by FTP, but there is still this "critical issue" and some clients are worried about it.

Workaround should be like firstly write something to that log, so we make sure, file exists:

<?php error_log('Testing debug.log'); ?>

Then just use one of PHP functions to return response headers, like file_get_contents or CURL or others... and check if the response code is 200

[Clorith]

Replying to kubiq:

We should check it with CURL if that file is really public.

This makes the presumption that the file is always in a fixed location, but r44453 made it so you can change the path and filename via the WP_DEBUG_LOG constant, meaning the URL may end up being unknown as well.

I'd also draw attention to comment:11 which mentions that leaving a debug logfile may end up eating hosting space that the user does not account for or expect, and is outside of log rotation.

[kubiq]

Well, you can check if WP_DEBUG_LOG == 1 and then do test with default url, else you can translate absolute path from WP_DEBUG_LOG to URL, maybe some try-catch and comparison with ABSPATH, etc... there is always a way how to do it, but alerting user a critical issue for public log file that is absolutely not public doesn't seems right to me.
I can try to rewrite WP site health tests if you want, if it will help ;)

[afragen]

@kubiq the patch on this ticket checks to see if debug.log is in ABSPATH and if not changes the warning from critical to recommended. I thought this would be a simple solution as anything outside of ABSPATH should NOT be found in a normal WP URL.

[xkon]

Can we discuss this "the other way around" if possible?

I'm not sure if I'm missing anything from previous discussions but, why do we want to be flagging WP_DEBUG_LOG as critical in any case, I don't quite understand.

For it to be enabled, it means that somebody altered the constant so that kind of puts them in a place that they are already "aware" of it being enabled (I'm not talking about plugin tampering I'm talking about the default behavior). On a case of plugin tampering, again most likely there would be a user action leading to that.

The public/non-public view doesn't make a major difference in my eyes at least since they might also have DISPLAY on, so essentially all the errors could be in the wild either way. In this sense for this to be a critical issue then other constants should be raised as critical flags also.

I can only agree with the part that a log might be potentially getting bigger and bigger and eventually cause issues regarding space. But that should be a fairly straightforward filesize check that we could implement on anything that is defined as a log ( either the debug.log or a custom ).

From my point of view, this issue should've been under recommended improvements and not mentioned as a Critical one.

Also if we do want to continue having this as a Critical issue then we might want to update ​https://wordpress.org/support/article/debugging-in-wordpress/ as well since there's nothing mentioned there regarding possible security implications or file size and other concerns :).

Again sorry if I'm missing anything from previous chats either on other tickets or on slack!

[Clorith]

Those who have enabled it and would _need_ this warning are the users told to enable it by someone helping them troubleshoot (or trying to troubleshoot on their own based on various guides), without fully understanding what leaving this on may involve, once a problem is resolved which may end in warnings being shown, it's easy to forget that you enabled debug logging in the first place.

We need to keep in mind that the majority of users are likely not the tech savvy ones that understand what information may or may not end up in such a log location, and with them in mind it should still remain critical to not leave potentially private information in a location which may or may not be exposed.

If the user enabling it knew to change the location of the log output, that's great, but I would then also presume they are slightly more technically inclined than many others, and know to ignore the warning for the short period in which they are troubleshooting for.

So then...

All of that said, I am a sensible person, let's go with 47985.3.diff​ as an iteration at this time and see how that ends up being received :)

[SergeyBiryukov]

In 47235:

Site Health: Move the warning about WP_DEBUG_LOG being publicly accessible from "critical issues" to "recommended improvements" if the error log is outside of the WordPress directory.

Props afragen, Clorith, miss_jwo, mikeschroder, kubiq, xkon.
Fixes #47985.