Make WordPress Core

Opened 13 months ago

Last modified 2 months ago

#62722 new defect (bug)

Fix all ABSPATH direct access errors

Reported by: bor0's profile bor0 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch has-test-info close
Focuses: Cc:

Description

We host WooCommerce.com, and our logs are flooded with ABSPATH errors due to possibly bots accessing random URLs.

I see that this was already reported in #61314, #61286, #61277, #61912, #55936. The aim of this ticket is to resolve all "undefined ABSPATH" related issues.

Here is the easiest way to get all ABSPATH issues:

$ wp core download
Downloading WordPress 6.7.1 (en_US)...
md5 hash verified: fae7bae13a158496ab884b6cdb0c5c03
Success: WordPress downloaded.
$ wp config create --dbname=wordpress --dbuser=root
Success: Generated 'wp-config.php' file.
$ wp db create
Success: Database created.
$ wp core install --url=localhost:8080 --title="WordPress" --admin_user=bor0 --admin_password=asdf --admin_email=boro.sitnikovski@automattic.com
Success: WordPress installed successfully.
$ > ~/dev/log/error_log # empty error log
$ find . -name '*.php' | sed 's|^\./||' | xargs -I {} echo "http://localhost:8080/{}" > urls.txt # generate urls
$ xargs -P 10 -n 1 curl -s -o /dev/null < urls.txt # visit each url
$ grep ABSPATH ~/dev/log/error_log | grep -o '/[^ ]*.php' | uniq
/opt/homebrew/var/www/wp-settings.php
/opt/homebrew/var/www/wp-admin/includes/class-wp-privacy-data-export-requests-list-table.php
/opt/homebrew/var/www/wp-admin/includes/class-wp-upgrader.php
/opt/homebrew/var/www/wp-admin/includes/nav-menu.php
/opt/homebrew/var/www/wp-admin/includes/class-wp-privacy-data-removal-requests-list-table.php
/opt/homebrew/var/www/wp-admin/includes/template.php
/opt/homebrew/var/www/wp-includes/functions.php
/opt/homebrew/var/www/wp-includes/blocks/require-dynamic-blocks.php
/opt/homebrew/var/www/wp-includes/class-wp-customize-setting.php
/opt/homebrew/var/www/wp-includes/class-wp-customize-panel.php
/opt/homebrew/var/www/wp-includes/class-simplepie.php
/opt/homebrew/var/www/wp-includes/cache.php
/opt/homebrew/var/www/wp-includes/class-IXR.php
/opt/homebrew/var/www/wp-includes/meta.php
/opt/homebrew/var/www/wp-includes/ms-blogs.php
/opt/homebrew/var/www/wp-includes/Requests/library/Requests.php
/opt/homebrew/var/www/wp-includes/wp-diff.php
/opt/homebrew/var/www/wp-includes/class-wp-customize-section.php
/opt/homebrew/var/www/wp-includes/class-wp-customize-control.php
/opt/homebrew/var/www/wp-includes/nav-menu-template.php
/opt/homebrew/var/www/wp-includes/default-widgets.php
/opt/homebrew/var/www/wp-includes/class-wp-http.php
/opt/homebrew/var/www/wp-includes/ms-settings.php
/opt/homebrew/var/www/wp-includes/script-loader.php

Props @raicem

Attachments (3)

62722.patchโ€‹ (11.4 KB) - added by bor0 13 months ago.
62722.2.patchโ€‹ (6.7 KB) - added by bor0 8 months ago.
62722.3.patchโ€‹ (6.7 KB) - added by bor0 8 months ago.

Download all attachments as: .zip

Change History (22)

@bor0
13 months ago

#1 @bor0
13 months ago

  • Keywords has-patch dev-feedback added

The attached file looks to address all the ABSPATH issues. Note that there are other issues (such as add_action undefined, etc.) but I'd propose addressing those as separate trac issues.

$ grep ABSPATH ~/dev/log/error_log | grep -o '/[^ ]*.php' | uniq
/opt/homebrew/var/www/wp-settings.php
/opt/homebrew/var/www/wp-admin/includes/class-wp-privacy-data-export-requests-list-table.php
/opt/homebrew/var/www/wp-admin/includes/class-wp-upgrader.php
/opt/homebrew/var/www/wp-admin/includes/nav-menu.php
/opt/homebrew/var/www/wp-admin/includes/class-wp-privacy-data-removal-requests-list-table.php
/opt/homebrew/var/www/wp-admin/includes/template.php
/opt/homebrew/var/www/wp-includes/functions.php
/opt/homebrew/var/www/wp-includes/blocks/require-dynamic-blocks.php
/opt/homebrew/var/www/wp-includes/class-wp-customize-setting.php
/opt/homebrew/var/www/wp-includes/class-wp-customize-panel.php
/opt/homebrew/var/www/wp-includes/class-simplepie.php
/opt/homebrew/var/www/wp-includes/cache.php
/opt/homebrew/var/www/wp-includes/class-IXR.php
/opt/homebrew/var/www/wp-includes/meta.php
/opt/homebrew/var/www/wp-includes/ms-blogs.php
/opt/homebrew/var/www/wp-includes/Requests/library/Requests.php
/opt/homebrew/var/www/wp-includes/wp-diff.php
/opt/homebrew/var/www/wp-includes/class-wp-customize-section.php
/opt/homebrew/var/www/wp-includes/class-wp-customize-control.php
/opt/homebrew/var/www/wp-includes/nav-menu-template.php
/opt/homebrew/var/www/wp-includes/default-widgets.php
/opt/homebrew/var/www/wp-includes/class-wp-http.php
/opt/homebrew/var/www/wp-includes/ms-settings.php
/opt/homebrew/var/www/wp-includes/script-loader.php
$ patch -p0 < ~/Desktop/62722.patch 
patching file 'wp-admin/includes/class-wp-privacy-data-export-requests-list-table.php'
patching file 'wp-admin/includes/class-wp-privacy-data-removal-requests-list-table.php'
patching file 'wp-admin/includes/class-wp-upgrader.php'
patching file 'wp-admin/includes/nav-menu.php'
patching file 'wp-admin/includes/template.php'
patching file 'wp-includes/Requests/library/Requests.php'
patching file 'wp-includes/blocks/require-dynamic-blocks.php'
patching file 'wp-includes/cache.php'
patching file 'wp-includes/class-IXR.php'
patching file 'wp-includes/class-simplepie.php'
patching file 'wp-includes/class-wp-customize-control.php'
patching file 'wp-includes/class-wp-customize-panel.php'
patching file 'wp-includes/class-wp-customize-section.php'
patching file 'wp-includes/class-wp-customize-setting.php'
patching file 'wp-includes/class-wp-http.php'
patching file 'wp-includes/default-widgets.php'
patching file 'wp-includes/functions.php'
patching file 'wp-includes/meta.php'
patching file 'wp-includes/ms-blogs.php'
patching file 'wp-includes/ms-settings.php'
patching file 'wp-includes/nav-menu-template.php'
patching file 'wp-includes/script-loader.php'
patching file 'wp-includes/wp-diff.php'
patching file wp-settings.php
$ > ~/dev/log/error_log # empty error log
$ find . -name '*.php' | sed 's|^\./||' | xargs -I {} echo "http://localhost:8080/{}" > urls.txt # generate urls
$ xargs -P 10 -n 1 curl -s -o /dev/null < urls.txt # visit each url
$ grep ABSPATH ~/dev/log/error_log | grep -o '/[^ ]*.php' | uniq
$

That is, after applying the patch, no ABSPATH errors are reported.

@SergeyBiryukov I'd like to get your attention on this ticket - since you're blazingly fast at helping out :)

#2 @SirLouen
9 months ago

#63303 was marked as a duplicate.

#4 @SirLouen
8 months ago

  • Keywords changes-requested added; dev-feedback removed

@bor0 can you create a GH PR for this?

Also, here some files to review:

  1. tools/release/sync-stable-blocks.js file that has also been modified and doesn't seem correct that mod.
  2. src/wp-includes/wp-diff.php double check
  3. src/wp-includes/nav-menu-template.php double check
  4. src/wp-includes/ms-settings.php double check
  5. src/wp-includes/ms-blogs.php double check
  6. src/wp-includes/functions.php double check
  7. src/wp-includes/default-widgets.php double check
  8. src/wp-includes/class-wp-http.php double check
  9. src/wp-includes/class-wp-customize-setting.php double check
  10. src/wp-includes/class-wp-customize-panel.php double check
  11. src/wp-includes/class-wp-customize-control.php double check
  12. src/wp-includes/class-IXR.php double check

I'm not sure if saying which were right would have been shorter ๐Ÿด

#6 follow-up: @bor0
8 months ago

Attaching new patch. @SirLouen I updated the files, saw that the check was already added in some. Also, I still think we need that tools change because it's generating a php file

@bor0
8 months ago

#7 in reply to: โ†‘ย 6 ; follow-up: @SirLouen
8 months ago

Replying to bor0:

Attaching new patch. @SirLouen I updated the files, saw that the check was already added in some. Also, I still think we need that tools change because it's generating a php file

Do you know exactly how this is generated?

Because I see that you have also commented require-dynamic-blocks.php which in theory is the file autogenerated by tools/release/sync-gutenberg-packages.js

If you can find out which are the steps to generate this file, I think it would great from a testing perspective (to see if it's actually generating this code in the right place)

#8 in reply to: โ†‘ย 7 @bor0
8 months ago

If you can find out which are the steps to generate this file, I think it would great from a testing perspective (to see if it's actually generating this code in the right place)

bor0:~/dev/wordpress-develop$ npm install
...
bor0:~/dev/wordpress-develop$ npm run sync-gutenberg-packages
...
bor0:~/dev/wordpress-develop$ git status src/wp-includes/blocks/*-blocks.php
On branch 62722
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	modified:   src/wp-includes/blocks/require-dynamic-blocks.php
	modified:   src/wp-includes/blocks/require-static-blocks.php

I noticed (and had forgotten) about this, so updating another tiny patch:

--- a/src/wp-includes/blocks/require-dynamic-blocks.php
+++ b/src/wp-includes/blocks/require-dynamic-blocks.php
@@ -4,7 +4,7 @@
 
 // Don't load directly.
 if ( ! defined( 'ABSPATH' ) ) {
-       die( '-1' );
+       exit();
 }

@bor0
8 months ago

#9 follow-up: @SirLouen
8 months ago

@bor0 ok! I clearly see it now. I wondered what were those '<?php` in that JS file. Now it makes sense.

But as I told you, if these two files are being automatically generated, it doesnt make sense that you include them in this patch right?. I assume that they will be included among many other changes that sync-gutenberg-packages introduce in the code.

Finally, in tools/release/sync-stable-blocks.js line 39 you are adding a new line, but I think there you were intending to add the anti-load code.

#10 in reply to: โ†‘ย 9 ; follow-up: @bor0
7 months ago

But as I told you, if these two files are being automatically generated, it doesnt make sense that you include them in this patch right?. I assume that they will be included among many other changes that sync-gutenberg-packages introduce in the code.

Correct, but I still don't know at which point in the release process they get automatically generated, so I thought I'd include that as part of the patch since it's not changing any functionality. But if you feel strongly about it, I'm happy to exclude.

Finally, in tools/release/sync-stable-blocks.js line 39 you are adding a new line, but I think there you were intending to add the anti-load code.

The static file doesn't generate the error, this is why I decided to exclude it:

bor0:~$ curl "http://localhost:8080/wp-includes/blocks/require-dynamic-blocks.php"
<br />
<b>Fatal error</b>:  Uncaught Error: Undefined constant &quot;ABSPATH&quot; in /opt/homebrew/var/www/wp-includes/blocks/require-dynamic-blocks.php:5
Stack trace:
#0 {main}
  thrown in <b>/opt/homebrew/var/www/wp-includes/blocks/require-dynamic-blocks.php</b> on line <b>5</b><br />
bor0:~$ curl "http://localhost:8080/wp-includes/blocks/require-static-blocks.php"

#11 in reply to: โ†‘ย 10 @SirLouen
7 months ago

Replying to bor0:

Correct, but I still don't know at which point in the release process they get automatically generated, so I thought I'd include that as part of the patch since it's not changing any functionality. But if you feel strongly about it, I'm happy to exclude.

Let me ask, and I will get back to here when we have an answer. I think we got it almost there.

The static file doesn't generate the error, this is why I decided to exclude it:

Got it ๐Ÿ‘Œ

#12 @SirLouen
7 months ago

  • Keywords has-test-info added; changes-requested removed

A little extra update: I've been playing around a little with this

Taking your sequence, I've built this single command, which can display the results (using wordpress-develop environment)

(find . -path './wp-content' -prune -false -o -name '*.php' -print | sed 's|^\./||' | sort -u | xargs -P 10 -I {} curl -s -o /dev/null "http://localhost:8889/{}"; grep ABSPATH wp-content/debug.log | grep -o '/[^ ]*.php' | sort -u)

Moreover, I see that the number of files being reported on debug.log with errors on execution, is practically infinite (not just the ABSPATH ones).

And I wonder if something like this can be included in CI checks. Because it's interesting how your original results differ (issued only 6 months ago), differ from nowadays results. What we are doing here is bread for today, hunger for tomorrow.

And probably this kind of checks could be expanded to virtually all potentially failing files in WP.

#13 @SirLouen
7 months ago

So I got an answer from @desrosj in #core-build-test-tools

All changes to built files that are included in version control should be included in the PR and eventual commit. Otherwise the build will likely fail, or something will be broke.

I've opened a related ticket #63554 because I can't really see which advantages there are solving specifically all these files and not the other 450 (less because there are many includes, but a good amount). I'm not 100% sure which is the position of the developer collective about this.

Personally, I would go into sorting all of them in batches.

I want to cc @johnbillion because I know he is sensitive about these topics. Maybe he can throw some light on this topic.

Anyway, just in case we don't get a perspective on this, I'm going to schedule this for my futures scrubs.

This ticket was mentioned in โ€‹Slack in #core by sirlouen. โ€‹View the logs.


7 months ago

This ticket was mentioned in โ€‹Slack in #core by sirlouen. โ€‹View the logs.


7 months ago

This ticket was mentioned in โ€‹Slack in #core by audrasjb. โ€‹View the logs.


7 months ago

#17 @SirLouen
7 months ago

  • Keywords close added

As per today, dev-chat it seems that direct file access` errors, are a not a relevant topic on WP

@jorbin said that its a problem of server-side configuration

And it was also commented that unless they pose a clear risk at some point, that should be solved, they don't seem to be priority.

So I was expecting to have this sorted at some point, but it appears that this has become a close candidate.

Last edited 7 months ago by SirLouen (previous) (diff)

#18 @ocean90
2 months ago

#64287 was marked as a duplicate.

#19 @ocean90
2 months ago

#63930 was marked as a duplicate.

Note: See TracTickets for help on using tickets.