Opened 13 months ago
Last modified 2 months ago
#62722 new defect (bug)
Fix all ABSPATH direct access errors
| Reported by: |
|
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)
Change History (22)
#4
@
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:
tools/release/sync-stable-blocks.jsfile that has also been modified and doesn't seem correct that mod.src/wp-includes/wp-diff.phpdouble checksrc/wp-includes/nav-menu-template.phpdouble checksrc/wp-includes/ms-settings.phpdouble checksrc/wp-includes/ms-blogs.phpdouble checksrc/wp-includes/functions.phpdouble checksrc/wp-includes/default-widgets.phpdouble checksrc/wp-includes/class-wp-http.phpdouble checksrc/wp-includes/class-wp-customize-setting.phpdouble checksrc/wp-includes/class-wp-customize-panel.phpdouble checksrc/wp-includes/class-wp-customize-control.phpdouble checksrc/wp-includes/class-IXR.phpdouble check
I'm not sure if saying which were right would have been shorter ๐ด
This ticket was mentioned in โPR #8932 on โWordPress/wordpress-develop by โ@bor0.
8 months ago
#5
Trac issue: https://core.trac.wordpress.org/ticket/62722
#6
follow-up:
โย 7
@
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
#7
in reply to:
โย 6
;
follow-up:
โย 8
@
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
toolschange 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
@
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();
}
#9
follow-up:
โย 10
@
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:
โย 11
@
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-packagesintroduce 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.jsline 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 "ABSPATH" 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
@
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
@
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
@
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
@
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.
The attached file looks to address all the ABSPATH issues. Note that there are other issues (such as
add_actionundefined, 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 :)