security(CORE-1130): clear session data on logout transition - #3398
Conversation
WEBVIEW_USER_LOGGED_IN was dispatched by the webview preload bridge on every login/logout, but only the reducer consumed it (updating server.userLoggedIn in Redux state). No main-process side effect cleared the logged-out server's session storage/cookies/cache, so login data for a server the user explicitly logged out of remained on disk and in memory. Add handleUserLoggedOutDataClearing() in servers/cache.ts, wired in main.ts next to the existing handleClearCacheDialog(). It listens on WEBVIEW_USER_LOGGED_IN and reuses the existing clearWebviewStorageDeletingLoginData() primitive (previously only reachable from the manual "Clear Cache" dialog). Transition guard: the action fires userLoggedIn=false on webview attach/startup before the user has ever logged in, not just on an actual logout. Reacting to every false payload would wipe storage and reload the webview on every app launch — a regression, not a fix. A per-server-URL Map tracks the previously observed userLoggedIn value (independent of the Redux reducer, which already overwrites server.userLoggedIn by the time listeners run) so the clear only fires on a genuine logged-in(true) -> logged-out(false) transition. Initial false, false->true, and true->true are all no-ops. Scope: this only clears webview session storage/cookies/cache for the server that logged out - it intentionally does not touch app-level data (download history, persisted preferences) or other servers' sessions, since a multi-server client can have one server logged out while staying logged into another. Test: src/servers/main/cache.spec.ts (named to match jest's main-process testMatch, which only discovers src/*/main/**/*.spec.ts or src/**/main.spec.ts - a flat src/servers/cache.main.spec.ts is not picked up). Covers: logged-in->logged-out triggers the clear; initial false at startup does not; logged-out->logged-in does not; missing webContents is a safe no-op.
WalkthroughThis PR adds a handler that clears guest webview storage (cache, storage data, reload) when a server's user transitions from logged-in to logged-out, tracked per server URL via a module-level map, wires it into app startup, and adds corresponding Jest tests. ChangesLogout Data Clearing
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Store
participant CacheModule as handleUserLoggedOutDataClearing
participant WebContentsResolver as getWebContentsByServerUrl
participant GuestWebContents
Store->>CacheModule: WEBVIEW_USER_LOGGED_IN action (url, userLoggedIn)
CacheModule->>CacheModule: compare previous vs new login state
alt logged-in to logged-out transition
CacheModule->>WebContentsResolver: getWebContentsByServerUrl(url)
WebContentsResolver-->>CacheModule: guestWebContents
CacheModule->>GuestWebContents: clearWebviewStorageDeletingLoginData(guestWebContents)
end
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/servers/main/cache.spec.ts`:
- Around line 54-57: The cache.spec.ts setup only clears mocks, but the
module-level previousUserLoggedInByUrl state in cache.ts persists across tests
and can hide the initial-state path. Update the tests to load the cache module
with fresh module state per case, using jest.isolateModules around the imports
that define handleUserLoggedOutDataClearing, so previousUserLoggedInByUrl starts
empty for each test and the “initial logged-out state” scenario truly exercises
the undefined transition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9cb4c9b-5d10-4c85-a6ef-6aaf83904f39
📒 Files selected for processing (3)
src/main.tssrc/servers/cache.tssrc/servers/main/cache.spec.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use TypeScript for all new code unless explicitly told otherwise
Use optional chaining with fallbacks for platform-specific APIs instead of mocking when possible. Example:const uid = process.getuid?.() ?? 1000;
Files:
src/servers/main/cache.spec.tssrc/main.tssrc/servers/cache.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,ts}: MANDATORY: Use Fuselage components for all UI work. Only create custom components when Fuselage doesn't provide what's needed
Import UI components from@rocket.chat/fuselageand checkTheme.d.tsfor valid color tokens
Use React functional components with hooks
Use PascalCase for component file names
Files:
src/servers/main/cache.spec.tssrc/main.tssrc/servers/cache.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfile naming for Renderer process tests
Files:
src/servers/main/cache.spec.ts
**/*.{spec.ts,main.spec.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Only mock platform-specific APIs when defensive coding isn't possible. Linux-only APIs requiring mocks:
process.getuid(),process.getgid(),process.geteuid(),process.getegid()
Files:
src/servers/main/cache.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Redux actions must follow FSA (Flux Standard Action) pattern
Avoid unnecessary comments — write self-documenting code through clear naming
Always verify libraries by checking official docs and.d.tsfiles innode_modules/. Never assume props, tokens, or APIs work without verification
Avoid subjective descriptors ('smart', 'excellent', 'dumb') in documentation and comments
Use measurable descriptions in code documentation: 'reduced memory usage', 'improved by X%' instead of subjective claims
NEVER invent metrics — don't include estimated time spent or speculated user counts. Only include numbers from actual logs, error messages, or documented sources
Files:
src/servers/main/cache.spec.tssrc/main.tssrc/servers/cache.ts
🔇 Additional comments (2)
src/servers/cache.ts (1)
7-10: LGTM!Also applies to: 57-79
src/main.ts (1)
41-44: LGTM!Also applies to: 161-161
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| handleUserLoggedOutDataClearing(); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Reset previousUserLoggedInByUrl between tests for proper isolation.
jest.clearAllMocks() resets mock call history but does not reset the module-level previousUserLoggedInByUrl Map in cache.ts. After test 1 dispatches true then false, the map retains { url: false }. Test 2 ("does not clear on the initial logged-out state at startup/attach") then runs with wasLoggedIn = false instead of undefined — it passes, but doesn't exercise the actual initial-state path described in its name.
If the transition condition were ever changed to distinguish undefined from false, this test would give false confidence. Use jest.isolateModules to get a fresh module registry per test:
🧪 Proposed fix for test isolation
beforeEach(() => {
jest.clearAllMocks();
- handleUserLoggedOutDataClearing();
+ jest.isolateModules(() => {
+ // eslint-disable-next-line `@typescript-eslint/no-var-requires`
+ const { handleUserLoggedOutDataClearing } = require('../cache');
+ handleUserLoggedOutDataClearing();
+ });
});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/servers/main/cache.spec.ts` around lines 54 - 57, The cache.spec.ts setup
only clears mocks, but the module-level previousUserLoggedInByUrl state in
cache.ts persists across tests and can hide the initial-state path. Update the
tests to load the cache module with fresh module state per case, using
jest.isolateModules around the imports that define
handleUserLoggedOutDataClearing, so previousUserLoggedInByUrl starts empty for
each test and the “initial logged-out state” scenario truly exercises the
undefined transition.
1 participant
Summary
src/injected.ts→WEBVIEW_USER_LOGGED_INaction) but had no main-process listener reacting to it — this PR wires it up. No webapp-side (Rocket.Chat repo) change needed.handleUserLoggedOutDataClearing()insrc/servers/cache.ts: tracks each server's prioruserLoggedInstate and only clears on a genuinetrue → falsetransition — not on the initialfalseemitted at webview attach/startup, which would otherwise wipe storage and reload before the user ever logs in.Test plan
npx tsc --noEmitcleanyarn eslintcleansrc/servers/main/cache.spec.ts(4 cases, independently re-run and verified): logged-in→logged-out triggers clear; initialfalseat startup does NOT trigger (critical regression guard); logged-out→logged-in does NOT trigger; missing webContents is a safe no-opSummary by CodeRabbit
New Features
Bug Fixes