Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#47985 closed enhancement (fixed)

Site Health: log errors to public file

Reported by: afragen's profile afragen Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.4 Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: has-patch commit
Focuses: Cc:

Description

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.

Attachments (4)

47985.diff (415 bytes) - added by afragen 5 years ago.
test error_log file path to ABSPATH
47985.2.diff (421 bytes) - added by afragen 5 years ago.
improved logic
47985.3.diff (632 bytes) - added by afragen 5 years ago.
more specific patch refresh
screenshot_02.png (84.6 KB) - added by afragen 5 years ago.
screenshot with patch applied. WP_DEBUG is true and WP_DEBUG_LOG is true

Download all attachments as: .zip

Change History (33)

@afragen
5 years ago

test error_log file path to ABSPATH

This ticket was mentioned in Slack in #core by afragen. View the logs.


5 years ago

#2 @afragen
5 years ago

  • Milestone changed from Awaiting Review to 5.3

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


5 years ago

This ticket was mentioned in Slack in #core by afragen. View the logs.


5 years ago

#5 @afragen
5 years ago

  • Keywords dev-feedback added

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

#6 @Clorith
5 years ago

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.

#7 @afragen
5 years ago

@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
5 years ago

improved logic

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


5 years ago

This ticket was mentioned in Slack in #hosting-community by miss_jwo. View the logs.


5 years ago

This ticket was mentioned in Slack in #core by afragen. View the logs.


5 years ago

#11 @kirasong
5 years ago

@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.

#12 @afragen
5 years ago

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

#13 @afragen
5 years ago

  • Milestone changed from 5.3 to 5.4

This ticket was mentioned in Slack in #hosting-community by jadonn. View the logs.


5 years ago

This ticket was mentioned in Slack in #hosting-community by mike. View the logs.


5 years ago

#16 @kirasong
5 years ago

@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
5 years ago

more specific patch refresh

@afragen
5 years ago

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

#17 @afragen
5 years ago

@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.

Version 0, edited 5 years ago by afragen (next)

This ticket was mentioned in Slack in #hosting-community by amykamala. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-site-health by afragen. View the logs.


5 years ago

#20 @Clorith
5 years ago

#48961 was marked as a duplicate.

#21 @Clorith
5 years ago

  • Version set to 5.2

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.

#22 follow-up: @kubiq
5 years ago

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

#23 in reply to: ↑ 22 @Clorith
5 years ago

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.

#24 @kubiq
5 years ago

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 ;)

#25 @afragen
5 years ago

@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.

Last edited 5 years ago by afragen (previous) (diff)

#26 @xkon
5 years ago

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!

This ticket was mentioned in Slack in #core-site-health by clorith. View the logs.


5 years ago

#28 @Clorith
5 years ago

  • Keywords commit added; dev-feedback removed

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 :)

#29 @SergeyBiryukov
5 years ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

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.

Note: See TracTickets for help on using tickets.