-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Conversation
There was a problem hiding this 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.
This comment was marked as abuse.
This comment was marked as abuse.
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... |
and probably never will be |
My comment for evidence that this should be merged can be found in the framework upstream pr joomla-framework/session#55 (comment) |
There was a problem hiding this 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.
@arnepluhar may you please do a test a mark it sucessful? https://issues.joomla.org/tracker/joomla-cms/35509 |
I have tested this item ✅ successfully on e8d6784 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. |
@HLeithner sorry. I have tested this item successfully on e8d6784 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35509. |
I have tested this item ✅ successfully on e8d6784 This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35509. |
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35509. |
Wow not it's a commit message....
Github is configured to allow maintainers merge only if all tests are successful. Thanks all. |
This comment was marked as abuse.
This comment was marked as abuse.
@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. |
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. |
@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. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/35509. |
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. |
@arnepluhar everything is good, no trouble For administrators it looks like this on merge: 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. |
we probably need to improve the JTracker to count reviewers as a human test |
I often review code but I dont always test everything so I dont submit a tested successfuly |
This comment was marked as abuse.
This comment was marked as abuse.
…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.
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. use Joomla\Session\Validator\AddressValidator;
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? |
This comment was marked as abuse.
This comment was marked as abuse.
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. |
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. |
This comment was marked as abuse.
This comment was marked as abuse.
Should it be an expected behavior that using any enabled cache option does not destroy the token on logout? I do not believe it. |
This comment was marked as abuse.
This comment was marked as abuse.
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 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]>
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.