Articles icon indicating copy to clipboard operation
Articles copied to clipboard

ArticlesPlugin OnPageNotFound can lead to resource exhaustion

Open Josh-G opened this issue 6 years ago • 1 comments

Bug report

Summary

If an articles container is inaccessible to the current user, and they visit an articles listing which triggers the ArticlesRouter in the OnPageNotFound event handler, an internal forward loop consumes excessive CPU and memory.

The articles listing generates a OnPageNotFound event, causing ArticlesRouter::route to be called, which has a few branches that end with a sendForward call (example). As the sendForward call can once again end with an OnPageNotFound, the ArticlesRouter::route function is triggered again and a loop is formed.

This hit us quite hard as Googlebot began trying to index these pages which led to resource exhaustion: image

We resolved this in the short term by adding a global (😞) $_articlesHasRouted to the ArticlesPlugin:

        global $_articlesHasRouted;
        if ($_articlesHasRouted) {
            return;
        }
        $_articlesHasRouted = true;

To resolve, we could first attempt to get the resource inside ArticlesRouter::route before passing it to sendForward to prevent the loop from starting. Another solution was to track the last resource that was forwarded as a static in ArticlesRouter and to add a conditional to prevent forwarding to the same resource as the last route() call forwarded to.

I'm not familiar with MODX, however I can file a PR if some guidance can be given on the preferred solution.

Step to reproduce

  1. Create an ArticlesContainer that is not publicly accessible (via resource groups), e.g., http://localhost/blog
  2. Log out
  3. Browse to http://localhost/blog/2019/01/01/broken

Observed behavior

The page times out with excessive CPU and memory usage.

Expected behavior

I should have been redirected to the error page.

Environment

Articles: 1.7.13-pl MODX: 2.6.5-pl

Josh-G avatar Aug 20 '19 15:08 Josh-G

In a future version I want to remove the current ArticlesRouter and replace it with a better solution .

For this particular issue I have to dig a bit in the code to figure out what the best solution is.

JoshuaLuckers avatar Aug 20 '19 16:08 JoshuaLuckers