Paging @HLeithner @nikosdion @wilsonge

Forked from #35244 (comment)

This PR also cleans up now that the upstream default is correctly set to $allowIpOverrides = false which was not the case before Joomla 4 had the upstream updated.

This is gonna be a rather long technical explanation / history lesson / proposed fix, so brace yourselves.

It all starts with how the Joomla session management works.

The session service is provided by the libraries/src/Service/Provider/Session.php file.

The correct session service for the installation, web, administrator or cli (console) application is aliased in each application's app.php file, e.g. the includes/app.php file for the public site a.k.a. front–end application.

This is used in src/Service/Provider/Application.php to inject the correct session object into the application object.

At this point the session object is there but the session is inactive; Joomla has not checked whether there is a session or whether this is valid.

If you dive into the Joomla\CMS\Session\Session class which what all these sessions objects use you will see that it extends the Joomla! Framework's Joomla\Session\Session class. In there you will see that the session is actually started, resumed or invalidated the first time we try to access its contents.

This happens very early in the Joomla application object initialisation in CMSApplication itself when it calls doExecute. This method implementation in all three web application objects (site, administrator and api) start by calling Factory::getUser().

If you follow that rabbit hole you will see that it immediately tries to load the user key from the session. The get method on the session calls the session's start method which in turn calls the validate method. If that method returns false the session is destroyed and restarted.

Now we get to the devil while lies in the (implementation) details.

Remember libraries/src/Service/Provider/Session.php which sets up the sessions? Its buildSession method adds the Joomla\Session\Validator\AddressValidator validator to the validation stack. As @PhilETaylor pointed out, this uses $_SERVER['REMOTE_ADDR'] through the Input object:
$remoteAddr = $this->input->server->getString('REMOTE_ADDR', '');
Since this file belongs to the Joomla! Framework which is its own separate entity we can't possibly change it to tie it into Joomla's “Behind load balancer” configuration setting. At best we could have it go through the Joomla\Utilities\IPHelper utility class (which is initialised with Joomla's ”Behind load balancer” setting in each application's framework.php file) which is a bit problematic because it creates a hard dependency on that package which is not necessary outside of Joomla. That's what Phil did in joomla-framework/session#55

Even though it's merged, there's no guarantee this change would make into Joomla 4 since it requires a. the framework to publish a new version of the session package and b. Joomla 4 to update the dependency to joomla/session.

Moreover, this approach does not address the elephant in the room: third party code could go either through the Joomla input object or directly through $_SERVER to get REMOTE_ADDR which still contains the wrong IP address. While this is only incidental to the problem at hand it can still cause a security issue.

It's easy to say that Joomla extensions MUST always go through IpHelper but this ignores some very simple facts: i. Joomla 3 is still supported until August 17th, 2023, doesn't have the IpHelper and most 3PDs still need to support it; ii. this was the way people were doing things in Joomla 3 so unless they have already bitten by an IP issue on J3 they won't know to change their code in Joomla 4; and iii. if a third party library is used it might actually use $_SERVER['REMOTE_ADDR'] with the developer being none the wiser. We can't plausibly tell 3PDs (or even core code!) to change third party code.

Speaking of which Joomla 4 includes two third party libraries which use REMOTE_ADDR already, maximebf/debugbar and voku/portable-utf8. LOL!

Neither does just removing the validators, as Phil did in #35509, address this devil in the implementation details. BTW, I fully agree with that PR because the IP address will change randomly. My 4G connection jumps IPs every few minutes or when I go between cell towers. My VDSL connection jumps IPs every one hour to dissuade people from hosting their own servers at home or at the office. But I digress.

The only two ways to deal with $_SERVER['REMOTE_ADDR'] are A. have the host configure their web server to trust an HTTP header claiming to have the correct client address or B. overwrite $_SERVER['REMOTE_ADDR'] when “Behind load balancer” is enabled.

The former is a pipe dream and a security nightmare rolled into one. Hosts will rightfully decline doing that since it's your problem, not theirs. It's also a security nightmare because you should only trust this kind of header when the actual REMOTE_ADDR belongs to a trusted source, something that Joomla doesn't do at all right now. It also cannot be fixed by third party code since the session initialisation which needs the correct IP address happens long before onAfterIntiialize, the very first event third party code can attach itself to.

The latter is possible, with the asterisk of whether you actually trust the source of the IP address, and in fact Joomla was so kind as to copy the workaroundIPIssues method from FOF into the IpHelper class... but never call it anywhere.

Considering that the ”Behind load balancer” option is only going to be turned on when the user _really trusts _ the source of the X-Forwarded-For HTTP header I would propose that we call IPHelper::workaroundIPIssues() right after calling IpHelper::setAllowIpOverrides(true); in the following files:

• administrator/includes/framework.php
• includes/framework.php
• api/includes/framework.php

Remember, this fix is incidental to what we are discussing here and not enough on its own to fix the session problem. However, the existing proposed solutions are not enough either. Likewise, the “Behind load balancer” setting is necessary to fixing these issues but not enough on its own right. You need all of the above to deal with all issues which may arise from having a site behind a load balancer, a proxy (yes, this includes Apache behind NginX!) or a CDN.

It'd also be cool to have an exclusive allow list of IPs the user trusts to provide a valid X-Forwarded-For header as an optional security hardening tool (meaning: if the list is empty play dumb and accept it from everywhere, thank you very much) but that's another PR for another day, right?