Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#46689 closed enhancement (wontfix)

Site Health: Your site is set to log errors to a potentially public file

Reported by: knutsp's profile knutsp Owned by:
Milestone: Priority: normal
Severity: normal Version: 5.2
Component: Site Health Keywords: site-health dev-feedback
Focuses: Cc:

Description

The value, WP_DEBUG_LOG, has been added to this websites configuration file. This means any errors on the site will be written to a file which is potentially available to normal users.

The value of WP_DEBUG_LOG in my case is a file path in the user root, above the public_html folder and should be safe as anything else there.

I suggest, in case it's not false to begin with, then

if (WP_DEBUG_LOG !== true)

to check if WP_DEBUG_LOG, treated as a path, is above ABSPATH and this folder does not contain an index.php file, it's ignored and considered safe, otherwise it fails with this warning.

The last condition will ensure there is no (PHP based) webapp, like WordPress, based in that folder, which could happen when the actual site in a subfolder of another WordPress installation, as in my case here.

Attachments (2)

46689.diff (752 bytes) - added by xkon 5 years ago.
string_wp_debug_log.jpg (148.8 KB) - added by xkon 5 years ago.

Download all attachments as: .zip

Change History (13)

#1 @knutsp
5 years ago

  • Keywords site-health added

#2 @earnjam
5 years ago

I'm not sure we can be positive it isn't public just by checking that its location is above ABSPATH and doesn't contain index.php.

You can run WordPress in a subdirectory (as deeply nested as you want) of a public-facing directory. That root directory (or any public directory above ABSPATH) may not include an index.php file.

#3 follow-up: @xkon
5 years ago

I don't know if there's a concrete way to check if the file is accessible or not and be 100% certain. But that's why the wording of the notice is like that as well I guess. In my opinion the users must know that the file might be accessible depending on the overall setup of their system, so I'd vote to leave this as is.

#4 @Clorith
5 years ago

Yes, the wording is intentional for that reason, we should change the debug data to output the value of WP_DEBUG_LOG if it's not a boolean though.

@xkon
5 years ago

#5 @xkon
5 years ago

  • Keywords has-patch has-screenshots dev-feedback added

46689.diff adds an extra check and displays the value of WP_DEBUG_LOG as well in WordPress Constants section.

Not really sure if we can stretch the ternary there like that so I'd like an opinion. We can always move the check elsewhere for better readability if needed :) .

#6 in reply to: ↑ 3 @knutsp
5 years ago

  • Type changed from defect (bug) to enhancement

Replying to xkon:

I don't know if there's a concrete way to check if the file is accessible or not and be 100% certain. But that's why the wording of the notice is like that as well I guess. In my opinion the users must know that the file might be accessible depending on the overall setup of their system, so I'd vote to leave this as is.

No way to be sure, as I know of.

But I don't like false positives, especially not under "Critical issues" and labeled "Security". Ideally this should be under "Recommended improvements" in the case I suggested, as it then is quite unlikely the log file is public. Every possible, potential issue cannot be detected anyway. It's about making it better, avoid setting off the alarm when probably quite ok.

Related: #46692 (and where the patch should go)

#7 @xkon
5 years ago

  • Keywords has-patch has-screenshots removed

Please ignore the 46689.diff (and screenshot) here. It will be added to the #46692 as it's the correct ticket so we can keep this discussion clear and within scope.

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

#8 @Clorith
5 years ago

In all fairness, any kind of error logging should not be running on production, except for in limited periods of time when there's a known issue you are trying to track. In light of this, it should be left as is.

#9 @Clorith
5 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

#46692 covers showing the value of WP_DEBUG_LOG if it's a string.

Leaving a warning if you're logging output to a file is still a good approach, see my reasoning for this in comment:8

Based on this, I'm closing this off as wontfix at this time.

#10 @spacedmonkey
5 years ago

  • Component changed from Administration to Site Health

#11 @Clorith
3 years ago

#53689 was marked as a duplicate.

Note: See TracTickets for help on using tickets.