Bug: Nginx/PHP-FSM environments subject to infinite recursions (internal redirects)

[nathanbrauer]

We've come across a strange occurrence on Nginx/PHP-FSM environments where a combination of the most up to date silverstripe-trailing-slash and sharedraftcontent versions lead to infinite internal redirects, causing "exceeds memory" fatal errors (or "exceeds time limit" fatal errors, when configured for max memory). The issue did not impact Apache configurations running the identical code base.

Before diving in, I reached out to @wilr (who contributed to #144) for context and suggested direction.

I started just dumping all the data as it was executed from TrailingSlashRedirector.php and comparing the outputs between Nginx/php-fsm (not working) and Apache (working) and eventually found the discrepancy....

Left is Nginx (incorrect), right is Apache (correct). Note that on the left (nginx), the second attempt makes a lookup request for the preview link, when it should have already converted that link to the full slug at the point (as apache correctly did, on the right).

Ultimately, we isolated it to ShareDraftController where it sets up a clean environment internally, sets the URL via $variables['_SERVER']['REQUEST_URI'] = $url;, and then saves it to the environment via Environment::setVariables($variables);

I printed Environment::getEnv('REQUEST_URI'); before and again after Environment::setVariables($variables); is executed, and sure enough...

Left is Nginx (incorrect), right is Apache (correct).

Solution

Two alternate solutions - both accomplish the same thing in different ways. I'm not as deeply familiar with the Environment class and the generally acceptable usages so I'm submitting both for consideration (pick one), though, @wilr and I agree that Option B seems a bit clearer and there might be unknown repercussions to unilaterally overwriting static::$env as happens Option A.

1. Fixes #213: Nginx/PHP-FSM environments subject to infinite recursions (internal redirects) - Option A #211
2. Fixes #213: Nginx/PHP-FSM environments subject to infinite recursions (internal redirects) - Option B #212

The end result is the same for both implementations:

It really just comes down to whether we use only Environment::setVariables (Option A) or a combination of Environment::setVariables and Environment::setEnv (Option B).

I have to wonder if perhaps Environment::setVariables is not being used correctly in the first place? Because I noticed the source code for Environment::setVariables sets static::$env variables in a VERY different way compared to Environment::setEnv:

What doesn't work....

I did attempt to replace Environment::setVariables entirely with only Environment::setEnv calls -- it broke both environments. So, somehow $GLOBALS are a necessary component.

Context / related:

1. FIX Don't rely on _SERVER values for redirect axllent/silverstripe-trailing-slash#14
2. Preview links broken in SS 4.8 #143
3. FIX resolve previewing pages which redirect (#143) #144 - (related code)
4. Bugfix: Infinite recursion sanity checks with more helpful error messages #214

---

@muskie9 @jsirish @oddnoc @phil-quinn @wilr

PRs

• Bugfix: Infinite recursion sanity checks with more helpful error messages #214
• Fixes #213: Nginx/PHP-FSM environments subject to infinite recursions (internal redirects) - Option A #211
  OR
• Fixes #213: Nginx/PHP-FSM environments subject to infinite recursions (internal redirects) - Option B #212

[GuySartorelli]

Hi,
Can you please not @ me in new issues in the future? I'll come across it naturally. Cheers.

[GuySartorelli]

Separate out each problem's solution as its own PR

The first thing I note is that you're solving two separate problems (and have even clearly marked them as separate problems) - since there are two ways to solve the second problem but only one way to solve the first, can you please split out your solution to the first problem (i.e. the recursion protection for L161 of ShareDraftController) into its own PR?

That way we can review and merge that PR on its own merits, and it will be much simpler to compare the two remaining PRs which only include the different solutions for problem 2.

The approach for problem 2

I haven't taken the time to fully understand how the share draft module works, but manipulating (or even reading the PHP globals at this layer just feels like a bad way to handle things. It feels to me like this URL rewriting should be handled in middleware, or by getting and setting values from a HTTPRequest object. Especially since we've already identified the correct Page record before getRenderedPageByURL is ever called, I really don't understand why we need to be touching globals at that point.

[nathanbrauer]

• Separate out each problem's solution as its own PR - Done!

1. Bugfix: Infinite recursion sanity checks with more helpful error messages #214
2. Fixes #213: Nginx/PHP-FSM environments subject to infinite recursions (internal redirects) - Option A #211
3. Fixes #213: Nginx/PHP-FSM environments subject to infinite recursions (internal redirects) - Option B #212

The approach for problem 2 (now just "this issue")

It's important to note that the Environment class, which is part of silverstripe/framework, is where $GLOBALS are set/read from -- not in this module directly.

As for exactly how/why the request is built the way it is, that's something I'll need to defer to @wilr (or another of the original developers).

[nathanbrauer]

Checking in, here. :)

My thoughts on the involvement of $GLOBALS.... As it currently stands, both v2 and v3 rely on $GLOBALS when they use Environment::setVariables. As such, I think the best course of action is to accept this bugfix so those currently impacted (including several of our clients) can benefit immediately in v2/v3 as a patch release, and then work on refactoring this module so as not to rely on the globals in a minor (depending on how it's built) release on v3.

[GuySartorelli]

Thanks for reporting and fixing this!