Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4][CloudFlare Issues] Remove validation of the session by IP address #35509

Merged
merged 2 commits into from
Sep 24, 2021
Merged

[4][CloudFlare Issues] Remove validation of the session by IP address #35509

merged 2 commits into from
Sep 24, 2021

Conversation

PhilETaylor
Copy link
Contributor

@PhilETaylor PhilETaylor commented Sep 8, 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.

@PhilETaylor PhilETaylor changed the title [4] Remove validation of the session by IP address [4][CloudFlare Issues] Remove validation of the session by IP address Sep 8, 2021
Copy link
Member

@HLeithner HLeithner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also fixes changing IPs for end user connections.

@PhilETaylor

This comment was marked as abuse.

@nikosdion
Copy link
Contributor

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...

@brianteeman
Copy link
Contributor

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

@HLeithner
Copy link
Member

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

Copy link

@arnepluhar arnepluhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Session drops via CloudFlare stopped after implementing this PR. The session should be secured without the reliance of an IP address, anyway.

@HLeithner
Copy link
Member

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

@nikosdion
Copy link
Contributor

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.

@arnepluhar
Copy link

@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.

@arnepluhar
Copy link

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.

@alikon
Copy link
Contributor

alikon commented Sep 24, 2021

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 24, 2021
@HLeithner HLeithner merged commit e198c08 into joomla:4.0-dev Sep 24, 2021
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 24, 2021
@HLeithner
Copy link
Member

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.

@PhilETaylor

This comment was marked as abuse.

@nikosdion
Copy link
Contributor

@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.

@HLeithner
Copy link
Member

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.

@arnepluhar
Copy link

@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.

@richard67
Copy link
Member

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.

@HLeithner
Copy link
Member

@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.

@alikon
Copy link
Contributor

alikon commented Sep 24, 2021

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

@brianteeman
Copy link
Contributor

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

@PhilETaylor

This comment was marked as abuse.

@zero-24 zero-24 added this to the Joomla 4.0.4 milestone Sep 24, 2021
brianteeman pushed a commit to brianteeman/joomla-cms that referenced this pull request Oct 3, 2021
…joomla#35509)

> IMHO the commend of @arnepluhar in joomla#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.
@Stuartemk
Copy link

Stuartemk commented Oct 28, 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

@HLeithner
Copy link
Member

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?

@PhilETaylor

This comment was marked as abuse.

@Stuartemk
Copy link

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.

@Stuartemk
Copy link

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.

@PhilETaylor

This comment was marked as abuse.

@Stuartemk
Copy link

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.

@PhilETaylor

This comment was marked as abuse.

@Stuartemk
Copy link

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)

bembelimen added a commit that referenced this pull request Nov 21, 2021
* Add sensible inline help to Global Configuration

* Button to show/hide inline help

Example in backend com_config

* Add back the error handling

* Show how this can be done in the frontend, too

* Update build/media_source/system/js/inlinehelp.es6.js

Co-authored-by: Brian Teeman <[email protected]>

* Update build/media_source/system/js/inlinehelp.es6.js

Co-authored-by: Dimitris Grammatikogiannis <[email protected]>

* Address inconsistency of control and description DIV id's

The controls have IDs like `jform_example`.

The description DIVs had IDs like `jform[example]-desc`.

This is horribly inconsistent and a pain to work with.

Converted the description DIV IDs to the more consistent
form `jform_example-desc`.

* Toggle the aria-describedby attribute to follow the visibility of the description

* Use simpler English

* Make this into a proper BUTTON element and end–align it

* Update administrator/language/en-GB/joomla.ini

Co-authored-by: Quy <[email protected]>

* Update language string

* Update language/en-GB/com_config.ini

Co-authored-by: Brian Teeman <[email protected]>

* Clarify SEF URLs

* Apply suggestions from code review

DocBlock updates

Co-authored-by: Phil E. Taylor <[email protected]>

* [4.x] Tinymce updates (#35605)

See https://www.tiny.cloud/docs/changelog/ for details

This includes 5.9.0, 5.9.1 and 5.9.2

* typo in filename (#35613)

* Remove JHELP_ strings for com_config and move the ref_key to the config.xml of the respective component. (#35479)

Co-authored-by: Thomas Hunziker <[email protected]>

* [4.0] Fix modal data attributes (#35626)

* Update main.php

* Update Bootstrap.php

* [4.0] Remove JHELP_ strings for com_categories (#35534)

* Category custom fields marked as 'subform' act as if they are not (#35547)

* Subform–only fields cause saving a category to fail.

Part I: “Only Use In Subform” doesn't do anything

* Subform–only fields cause saving a category to fail.

Part II. PlgSystemFields::onContentPrepareForm doesn't actually work for com_categories

Close gh-35543

* Subform–only fields cause saving a category to fail.

Part II. PlgSystemFields::onContentPrepareForm doesn't actually work for com_categories

This should only apply on com_content categories, not
third party categories.

* Invert the if–block order

Co-authored-by: Richard Fath <[email protected]>

* Subform–only fields cause saving a category to fail.

Part II. PlgSystemFields::onContentPrepareForm doesn't actually work for com_categories

Okay, it could actually apply for any category,
not just com_content.

* Docblock

Co-authored-by: Richard Fath <[email protected]>

* [4.0] Replace JHELP_ String with hardcoded ref-key (#35429)

* Replace JHELP_ String with hardcoded ref-key

* Simplifying helpTOC.php so it doesn't need to reverse lookup the JHELP_ strings.

* Remove loading of joomla.ini and messing with language class property

* alpha-sort help strings

* Next batch

* Last batch

* Ooops

Co-authored-by: Thomas Hunziker <[email protected]>

* [4][CloudFlare Issues] Remove validation of the session by IP address (#35509)

> 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.

* Language Keys may have a : in it. (#35659)

* Button to show/hide inline help

Example in backend com_config

* Whoops, wrong merge option

* Lang string change

* Apply language string suggestion

* Remove empty line on language file,

* Add sensible inline help to Global Configuration

* Button to show/hide inline help

Example in backend com_config

* Add back the error handling

* Show how this can be done in the frontend, too

* Update build/media_source/system/js/inlinehelp.es6.js

Co-authored-by: Brian Teeman <[email protected]>

* Update build/media_source/system/js/inlinehelp.es6.js

Co-authored-by: Dimitris Grammatikogiannis <[email protected]>

* Address inconsistency of control and description DIV id's

The controls have IDs like `jform_example`.

The description DIVs had IDs like `jform[example]-desc`.

This is horribly inconsistent and a pain to work with.

Converted the description DIV IDs to the more consistent
form `jform_example-desc`.

* Toggle the aria-describedby attribute to follow the visibility of the description

* Use simpler English

* Make this into a proper BUTTON element and end–align it

* Update administrator/language/en-GB/joomla.ini

Co-authored-by: Quy <[email protected]>

* Update language string

* Update language/en-GB/com_config.ini

Co-authored-by: Brian Teeman <[email protected]>

* Clarify SEF URLs

* Apply suggestions from code review

DocBlock updates

Co-authored-by: Phil E. Taylor <[email protected]>

* Button to show/hide inline help

Example in backend com_config

* Whoops, wrong merge option

* Lang string change

* Apply language string suggestion

* Remove empty line on language file,

Co-authored-by: Brian Teeman <[email protected]>
Co-authored-by: Dimitris Grammatikogiannis <[email protected]>
Co-authored-by: Quy <[email protected]>
Co-authored-by: Phil E. Taylor <[email protected]>
Co-authored-by: Stefan Wendhausen <[email protected]>
Co-authored-by: Thomas Hunziker <[email protected]>
Co-authored-by: Thomas Hunziker <[email protected]>
Co-authored-by: Richard Fath <[email protected]>
Co-authored-by: Benjamin Trenkle <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[J4.0] Joomla 4 Login and Cloudflare Free - Sessions Getting Destroyed Prematurely
10 participants