? ? Pending

User tests: Successful: Unsuccessful:

avatar PhilETaylor
PhilETaylor
8 Sep 2021

Code Review awaiting PLT decisions.

@wilsonge @nibra @HLeithner

Part of joomla-framework/session#55 that fixes issues where REMOTE_ADDR changes, which then forces a user to be logged out.

Possible solution for joomla-framework/session#54 and thus #35244 that fixes #35244 (comment) CloudFlare (and other reverse proxy's that set REMOTE_ADDR as their IP and not the clients IP (which is perfectly valid for a proxy to do that, but causes session logout issues as REMOTE_ADDR frequently changes.

Votes

# of Users Experiencing Issue
2/2
Average Importance Score
5.00

avatar PhilETaylor PhilETaylor - open - 8 Sep 2021
avatar PhilETaylor PhilETaylor - change - 8 Sep 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 8 Sep 2021
Category Libraries
avatar PhilETaylor PhilETaylor - change - 8 Sep 2021
Title
[4] Remove validation of the session by IP address
[4][CloudFlare Issues] Remove validation of the session by IP address
avatar PhilETaylor PhilETaylor - edited - 8 Sep 2021
avatar PhilETaylor PhilETaylor - change - 8 Sep 2021
The description was changed
avatar PhilETaylor PhilETaylor - edited - 8 Sep 2021
avatar PhilETaylor PhilETaylor - change - 8 Sep 2021
The description was changed
avatar PhilETaylor PhilETaylor - edited - 8 Sep 2021
avatar PhilETaylor PhilETaylor - change - 8 Sep 2021
Labels Added: ?
avatar PhilETaylor PhilETaylor - change - 8 Sep 2021
The description was changed
avatar PhilETaylor PhilETaylor - edited - 8 Sep 2021
avatar PhilETaylor PhilETaylor - change - 8 Sep 2021
The description was changed
avatar PhilETaylor PhilETaylor - edited - 8 Sep 2021
avatar PhilETaylor
PhilETaylor - comment - 9 Sep 2021

My stalker is back here too

Screenshot 2021-09-09 at 15 00 07

avatar nikosdion
nikosdion - comment - 9 Sep 2021

I agree with this PR and I have to add that it's necessary regardless of whether a site is behind a proxy / load balancer / CDN.

My 4G connections hops between different IPs every few minutes or whenever my phone switches between cell towers. This happens far more frequently than you think. My bedroom is covered by one cel tower, my office (at the other end of the house) by another. Line of sight to both from each respective window. Because of weather conditions I may even be stationary but my phone switches between cell towers. This means I can never stay connected logged in to my Joomla 4 blog when I am on 4G, e.g. when the VDSL connection goes down.

Speaking of my VDSL, my ISP gives a DHCP lease of 3600 seconds and changes my IP every hour, presumably as a means to dissuade running a server from the VDSL connection. Needless to say, this causes my session to reset on each DHCP lease anniversary, i.e. once every hour.

Tying sessions to IP addresses is not something that should be done by default. It's entered around the assumption that your IP will not change throughout a session's lifetime which is unrealistic out here in the real world. It works on an Intranet most of the time (assuming the DHCP leases are long enough) or when you are given the option to pay for a static IP but doesn't quite work with mobile connections and most cabled ISPs. Considering that sessions are a fundamental requirement for providing services to our sites' visitors having these validators is an anti–feature.

Removing these validators is, therefore, a good idea. If people get iffy about it we could always have Yet Another Global Configuration Option (disabled by default) to enable them. And, please, let's not pretend that this project particularly cares about Global Configuration options proliferation when a feature that should have been a plugin (block FLoC, which is a technology that's not even deployed yet) warrants its own Global Configuration option...

avatar brianteeman
brianteeman - comment - 9 Sep 2021

a feature that should have been a plugin (block FLoC, which is a technology that's not even deployed yet)

and probably never will be

avatar HLeithner HLeithner - change - 9 Sep 2021
The description was changed
avatar HLeithner HLeithner - edited - 9 Sep 2021
avatar HLeithner
HLeithner - comment - 9 Sep 2021

My comment for evidence that this should be merged can be found in the framework upstream pr joomla-framework/session#55 (comment)

avatar HLeithner
HLeithner - comment - 22 Sep 2021

@arnepluhar may you please do a test a mark it sucessful? https://issues.joomla.org/tracker/joomla-cms/35509

avatar nikosdion nikosdion - test_item - 24 Sep 2021 - Tested successfully
avatar nikosdion
nikosdion - comment - 24 Sep 2021

I have tested this item successfully on e8d6784

I tested using a Joomla 4.0.3 site behind CloudFlare.

IMHO the commend of @arnepluhar in #35509 should also count as a successful test. He did it as a GitHub code review. The rules about what constitutes a valid test should expand to GitHub code reviews; these didn't even exist when the rules were decided upon back in the day and they make FAR MORE SENSE than comments filed on a PR!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35509.

avatar arnepluhar
arnepluhar - comment - 24 Sep 2021

@HLeithner sorry.
I actually did the same like @nikosdion

I have tested this item successfully on e8d6784
I tested using two Joomla 4.0.3 sites behind CloudFlare.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35509.

avatar arnepluhar
arnepluhar - comment - 24 Sep 2021

@HLeithner sorry.
I actually did the same like @nikosdion

I have tested this item successfully on e8d6784
I tested using two Joomla 4.0.3 sites behind CloudFlare.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35509.

avatar arnepluhar arnepluhar - test_item - 24 Sep 2021 - Tested successfully
avatar arnepluhar
arnepluhar - comment - 24 Sep 2021

I have tested this item successfully on e8d6784

I have tested this item successfully on e8d6784
I tested using two Joomla 4.0.3 sites on different domains behind CloudFlare.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35509.

avatar alikon alikon - change - 24 Sep 2021
Status Pending Ready to Commit
avatar alikon
alikon - comment - 24 Sep 2021

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35509.

avatar HLeithner HLeithner - change - 24 Sep 2021
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-09-24 07:02:06
Closed_By HLeithner
Labels Added: ?
avatar HLeithner HLeithner - close - 24 Sep 2021
avatar HLeithner HLeithner - merge - 24 Sep 2021
avatar HLeithner
HLeithner - comment - 24 Sep 2021

Wow not it's a commit message....

IMHO the commend of @arnepluhar in #35509 should also count as a successful test. He did it as a GitHub code review. The rules about what constitutes a valid test should expand to GitHub code reviews; these didn't even exist when the rules were decided upon back in the day and they make FAR MORE SENSE than comments filed on a PR!

Github is configured to allow maintainers merge only if all tests are successful.

Thanks all.

avatar PhilETaylor
PhilETaylor - comment - 24 Sep 2021

Github is configured to allow maintainers merge only if all tests are successful.

And yet maintainers merge all the time without any testing!

avatar nikosdion
nikosdion - comment - 24 Sep 2021

@HLeithner GitHub tells me that he approved the changes which is the same message I see when we do GitHub code review on our own repositories ??‍♂️ I don't have any permissions on this repo so all I can tell you is what I see on the public GitHub interface.

avatar HLeithner
HLeithner - comment - 24 Sep 2021

Since you know it's build long before github has such features. The github repository is configured that all tests have to be green that a "normal" cms-maintainer can merge pull request. RL and Org admins can override this.

avatar arnepluhar
arnepluhar - comment - 24 Sep 2021

@nikosdion and @HLeithner … My behavior should not be the reason for any trouble here. I was not aware of issues.joomla.org before I was mentioned here. Before I "approved" the change in Github, as I thought this was the way to submit test results, but my comment on that was not exhaustive anyway.
Maybe somebody can clarify the difference between test and approval, if there is any and link the test procedure from time to time directly to the issue-discussion. Doing so, people who are facing the referenced issues are maybe more likely able to submit test results comprehensively.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35509.

avatar richard67
richard67 - comment - 24 Sep 2021

Approval = result of code reviews in most cases, and a test in the issue tracker is normally the result of a real test following provided testing instructions.

We require 2 human real tests in any case, regardless if there are approvals or not.

avatar HLeithner
HLeithner - comment - 24 Sep 2021

@arnepluhar everything is good, no trouble

For administrators it looks like this on merge:
image

Maintainers can not merge this PR as it doesn't have all required green check marks^^

issues.joomla.org is an application used to provide the "HumanTestResults" and has some alternative features, it's more or less an alternative frontend for github with some extras.

avatar alikon
alikon - comment - 24 Sep 2021

we probably need to improve the JTracker to count reviewers as a human test

avatar brianteeman
brianteeman - comment - 24 Sep 2021

I often review code but I dont always test everything so I dont submit a tested successfuly

avatar PhilETaylor
PhilETaylor - comment - 24 Sep 2021

If people are interested in tidying up the last part of this whole CloudFlare/IP behind proxy issue, please can you go and test #35524 so that it can be marked as tested and merged, then we can all forget about this until the next time.

avatar Stuartemk
Stuartemk - comment - 28 Oct 2021

Hello, I waited to update to 4.0.4 to activate cloudflare and avoid problems with the sessions, however the problem still exists, just a little different, the session is not destroyed and the user maintains its normal section, BUT the problem is that If the user closes his session and immediately tries to log in again, he throws the detestable invalid token, even waiting for a few minutes! The only option is for the user to open another url of the same domain, let's say he opens another article and from the form he enters again, but from the main path of the domain he cannot enter through the invalid token.
I have enabled the back of the balancer and I even checked and version 4.0.4 already has the lines removed

use Joomla\Session\Validator\AddressValidator;
use Joomla\Session\Validator\ForwardedValidator;

	$session->addValidator(new AddressValidator($input, $session));
	$session->addValidator(new ForwardedValidator($input, $session));

The sessions are managed by the database, although despite the documentation it has never been clear to me if it would be better to manage it by files

avatar HLeithner
HLeithner - comment - 28 Oct 2021

Hello, I waited to update to 4.0.4 to activate cloudflare and avoid problems with the sessions, however the problem still exists, just a little different, the session is not destroyed and the user maintains its normal section, BUT the problem is that If the user closes his session and immediately tries to log in again, he throws the detestable invalid token, even waiting for a few minutes! The only option is for the user to open another url of the same domain, let's say he opens another article and from the form he enters again, but from the main path of the domain he cannot enter through the invalid token. I have enabled the back of the balancer and I even checked and version 4.0.4 already has the lines removed

use Joomla\Session\Validator\AddressValidator; use Joomla\Session\Validator\ForwardedValidator;

	$session->addValidator(new AddressValidator($input, $session));
	$session->addValidator(new ForwardedValidator($input, $session));

The sessions are managed by the database, although despite the documentation it has never been clear to me if it would be better to manage it by files

@Stuartemk could you please make a new issue, because it seems not to be the ip validation and if I understand you correct. Joomla doesn't destroy the session on logout?

avatar PhilETaylor
PhilETaylor - comment - 28 Oct 2021

That sounds more like normal GET request caching - the whole point of CloudFlare. The page where your login form is, is probably cached aggressively by CloudFlare.

If you have a dynamic site (I.e, stuff changes (like a security token) or you have forms and logins (protected by a security token) then you cannot cache the GET requests with CloudFlare and you need to correctly set up CloudFlare for your use.

avatar Stuartemk
Stuartemk - comment - 5 Nov 2021

Hello, I waited to update to 4.0.4 to activate cloudflare and avoid problems with the sessions, however the problem still exists, just a little different, the session is not destroyed and the user maintains its normal section, BUT the problem is that If the user closes his session and immediately tries to log in again, he throws the detestable invalid token, even waiting for a few minutes! The only option is for the user to open another url of the same domain, let's say he opens another article and from the form he enters again, but from the main path of the domain he cannot enter through the invalid token. I have enabled the back of the balancer and I even checked and version 4.0.4 already has the lines removed
use Joomla\Session\Validator\AddressValidator; use Joomla\Session\Validator\ForwardedValidator;

	$session->addValidator(new AddressValidator($input, $session));
	$session->addValidator(new ForwardedValidator($input, $session));

The sessions are managed by the database, although despite the documentation it has never been clear to me if it would be better to manage it by files

@Stuartemk could you please make a new issue, because it seems not to be the ip validation and if I understand you correct. Joomla doesn't destroy the session on logout?

Hello, it seems to me that I already found the problem but not the solution, thank you very much for the answer, it provided a clue where to look.

avatar Stuartemk
Stuartemk - comment - 5 Nov 2021

That sounds more like normal GET request caching - the whole point of CloudFlare. The page where your login form is, is probably cached aggressively by CloudFlare.

If you have a dynamic site (I.e, stuff changes (like a security token) or you have forms and logins (protected by a security token) then you cannot cache the GET requests with CloudFlare and you need to correctly set up CloudFlare for your use.

Thanks for answering, but not in this case it is not a Cloudflare configuration problem, it is a Joomla problem, more specifically about the cache, I will open an issue giving the details.

avatar PhilETaylor
PhilETaylor - comment - 5 Nov 2021

specifically about the cache

Well yeah, if you have page cache plugin enabled then again, this would be the expected behaviour.

avatar Stuartemk
Stuartemk - comment - 5 Nov 2021

específicamente sobre el caché

Bueno, sí, si tiene habilitado el complemento de caché de página, este sería el comportamiento esperado.

Should it be an expected behavior that using any enabled cache option does not destroy the token on logout? I do not believe it.

avatar PhilETaylor
PhilETaylor - comment - 5 Nov 2021

It is expected behaviour that the "System - Page Cache" plugin CACHES THE WHOLE PAGE, including any token in the HTML and for this reason you should not enable that plugin, but enable the caching in Joomla Global Configuration that DOES take tokens into account.

avatar Stuartemk
Stuartemk - comment - 5 Nov 2021

It is expected behaviour that the "System - Page Cache" plugin CACHES THE WHOLE PAGE, including any token in the HTML and for this reason you should not enable that plugin, but enable the caching in Joomla Global Configuration that DOES take tokens into account.

What you are saying is that it is an expected behavior if the cache plugin is enabled because it captures the tokens, and that enabling the cache in the general configuration does not capture the tokens, so there would be no problem when enabling the cache in the general configuration. with the cache plugin disabled, but the error occurs in all possible configurations where at least one of the two caches is enabled global configuration or plugin or both.

Even though REDIS is managing the sessions #35971 (comment)

Add a Comment

Login with GitHub to post a comment