Make logout function more robust to prevent session issues #11359
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
|
sureshanaparti
requested review from
DaanHoogland,
Pearl1594,
Copilot and
shwstppr
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the logout function to prevent session issues by implementing more comprehensive cookie cleanup. The change addresses incomplete session termination by ensuring cookies are removed across multiple paths and domains.
Key Changes
- Replaces basic cookie removal with systematic cleanup across multiple paths (
/and/client) - Adds domain-specific cookie removal for current hostname and its subdomain variant
- Implements nested loops to ensure all cookie combinations are properly cleared
ui/src/store/modules/user.js
Outdated
| }).finally(() => { | ||
| const paths = ['/', '/client'] | ||
| const hostname = window.location.hostname | ||
| const domains = [undefined, hostname, `.${hostname}`] |
There was a problem hiding this comment.
The domain .${hostname} may create an invalid domain format. For example, if hostname is 'localhost' or an IP address like '127.0.0.1', the resulting domain '.localhost' or '.127.0.0.1' would be invalid. Consider validating the hostname format before adding the dot prefix, or handle cases where the hostname might not be a valid domain.
| const domains = [undefined, hostname, `.${hostname}`] | |
| const isValidDomain = (name) => /^[a-zA-Z0-9.-]+$/.test(name) && !/^(\d{1,3}\.){3}\d{1,3}$/.test(name) && name !== 'localhost' | |
| const domains = [undefined, hostname] | |
| if (isValidDomain(hostname)) { | |
| domains.push(`.${hostname}`) | |
| } |
ui/src/store/modules/user.js
Outdated
| Object.keys(Cookies.get()).forEach(cookieName => { | ||
| Cookies.remove(cookieName) | ||
| Cookies.remove(cookieName, { path: '/client' }) | ||
| paths.forEach(path => { | ||
| domains.forEach(domain => { | ||
| const options = { path } | ||
| if (domain) options.domain = domain | ||
| Cookies.remove(cookieName, options) | ||
| }) |
There was a problem hiding this comment.
Using nested forEach loops results in O(nmk) complexity where n is the number of cookies, m is the number of paths, and k is the number of domains. This could be inefficient if there are many cookies. Consider batching the operations or using a more efficient approach for cookie removal.
|
@sureshanaparti a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress. |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #11359 +/- ##
=============================================
- Coverage 17.17% 3.64% -13.53%
=============================================
Files 5869 441 -5428
Lines 521732 36822 -484910
Branches 63511 6736 -56775
=============================================
- Hits 89611 1344 -88267
+ Misses 422064 35318 -386746
+ Partials 10057 160 -9897
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
UI build: ✔️ |
AndrewHaZG
force-pushed
the
fix-cookie-logout-issue-11078
branch
from
5c44461 to
6589235
Compare
|
@AndrewHaZG , any reason you closed your new commit? |
4 participants
Description
Fixes #11078
This PR extends the logout function to improve cookie cleanup by removing cookies for multiple paths and domains to ensure complete session termination.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?