search

TangibleInc / template-system

A template system for WordPress with content type loops and conditions
https://docs.loopsandlogic.com/reference/template-system/
8 stars 3 forks source link

Framework: API module - Prevent recursive call #114

Closed nicolas-jaussaud closed 5 months ago

nicolas-jaussaud commented 5 months ago

Hi Eliot!

I recently noticed an issues with some endpoints that returns a 503 error on sites where the framework is used

The issue can be reproduced by creating or updating a new page on a WordPress site

The requests made on the initial page load will return a 503 error:

Screenshot from 2024-05-07 12-02-02

All those requests pass _locale as a GET parameter, and the issue seems to only happen when this parameter is set

In the error logs, there is the following error for each request:

PHP Fatal error:  Uncaught Error: Xdebug has detected a possible infinite loop, and aborted your script with a stack depth of '512' frames in /var/www/html/tangiblelive/wp-includes/class-wp-object-cache.php:14

From the associated stack trace it seems that there is indeed an infinite loop, caused by the determine_current_user filter we set in routes.php (here), which will trigger the determine_current_user filter again (by calling validate_token, here):

# [...]
#429 /var/www/html/tangiblelive/wp-content/plugins/tangible-live/vendor/tangible/framework/api/rest/routes.php(268): __()
#430 /var/www/html/tangiblelive/wp-content/plugins/tangible-live/vendor/tangible/framework/api/rest/routes.php(228): class@anonymous->validate_token()
#431 /var/www/html/tangiblelive/wp-includes/class-wp-hook.php(324): class@anonymous->determine_current_user()
#432 /var/www/html/tangiblelive/wp-includes/plugin.php(205): WP_Hook->apply_filters()
#433 /var/www/html/tangiblelive/wp-includes/user.php(3632): apply_filters()
#434 /var/www/html/tangiblelive/wp-includes/pluggable.php(70): _wp_get_current_user()
#435 /var/www/html/tangiblelive/wp-includes/l10n.php(98): wp_get_current_user()
#436 /var/www/html/tangiblelive/wp-includes/l10n.php(152): get_user_locale()
#437 /var/www/html/tangiblelive/wp-includes/l10n.php(1353): determine_locale()
#438 /var/www/html/tangiblelive/wp-includes/l10n.php(1384): _load_textdomain_just_in_time()
#439 /var/www/html/tangiblelive/wp-includes/l10n.php(194): get_translations_for_domain()
#440 /var/www/html/tangiblelive/wp-includes/l10n.php(306): translate()
#441 /var/www/html/tangiblelive/wp-content/plugins/tangible-live/vendor/tangible/framework/api/rest/routes.php(268): __()
#442 /var/www/html/tangiblelive/wp-content/plugins/tangible-live/vendor/tangible/framework/api/rest/routes.php(228): class@anonymous->validate_token()
#443 /var/www/html/tangiblelive/wp-includes/class-wp-hook.php(324): class@anonymous->determine_current_user()
#444 /var/www/html/tangiblelive/wp-includes/plugin.php(205): WP_Hook->apply_filters()
# [...]

The issue seems to come from the WP_Error returned here inside validate_token

Because the error message is localized, WordPress will try to look for the current user local and will end-up calling _wp_get_current_user, which will trigger the determine_current_user filter and call our method again

The reason it only affect requests that pass _locale as a GET parameter seems to be because of this condition in determine_locale()

This pull request should prevent this loop by removing our filter before calling validate_token

eliot-akira commented 5 months ago

Thanks for the pull request!

Because the error message is localized, WordPress will try to look for the current user local and will end-up calling _wp_get_current_user, which will trigger the determine_current_user filter and call our method again.

I see, that's a tricky bug - I appreciate you tracking down the cause and solving it. And thanks for the helpful issue description and the code comment to explain the solution. This part of the API module is adapted from a fairly well-used library, so I'm surprised no one has caught it before.

eliot-akira commented 5 months ago

OK, I refactored the module to remove the unnecessary use of filters and localization of error messages. (https://github.com/TangibleInc/template-system/commit/e0553d2105b68df2a4e728c1dfeb2e46ab830e07) That simplifies things so there's no possibility of this issue happening.

I've pushed the Framework module to its repo with npm run subrepo push framework. You can run composer update to pull the newest changes.

nicolas-jaussaud commented 5 months ago

That was fast, thank you!