fix: harden supported-versions validation, recovery, and exception scoping - #3405
Conversation
Audited the supported-versions subsystem end to end after the
uniqueId-scope escalation and fixed the confirmed defects:
- Wrap the cache/builtin fallback validation in a helper with try/catch
so a malformed cached payload can no longer reject the update before
the error state is dispatched, which left fetchState stuck at
'loading' and suppressed the UnsupportedServer block gate.
- Catch rejections at all four fire-and-forget validation call sites.
- Fall back to the persisted workspace uniqueID when the fresh fetch
fails so the cloud lookup is not skipped for previously-known servers.
- Switch uniqueID/version/gitCommitHash reducer cases from upsert to
update so a late identity dispatch cannot resurrect a server deleted
while a validation was in flight.
- Guard getExpirationMessageTranslated against payloads whose i18n
dictionary lacks both the user language and 'en'; the missing guard
crashed the async check and silently suppressed the expiring-
workspace warning dialog.
- Revalidate all servers on powerMonitor 'resume' (the window 'online'
event does not fire when waking with the same network connected).
- Use the real currentView state in SupportedVersionDialog's effect
dependencies; the previous dependency was the imported reducer
function, which never changes, so the dialog never re-checked on
view switches.
- Add a 'Check again' button to the unsupported-workspace screen so
users can re-trigger validation without restarting the app.
- Fix logRequestError printing a literal ${description}.
Also relax the exception scope check for UNKNOWN local identity: an
unfetchable workspace uniqueID (e.g. settings.public restricted by
enterprise API ACLs) no longer disqualifies a domain-matched exception;
a PROVEN uniqueID mismatch still rejects. The gate is client-side UX
enforcement rather than a security boundary, and wrongly blocking a
legitimate workspace is the worse failure mode.
Server, cloud, and cache supported-versions payloads are fetched from or for the server being validated, so their exceptions block cannot belong to another tenant. For these sources a domain/uniqueId scope mismatch is now logged as a diagnostic warning instead of disqualifying the exception. The bundled builtin payload is the only source that could carry another deployment's exceptions and keeps the strict scope requirement. This removes the remaining paths where a valid, unexpired exception could be rejected over unverifiable or drifted identity data (restricted settings.public API, rotated workspace uniqueID, stale persisted state).
WalkthroughChangesSupported version and server state
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant UnsupportedServer
participant ipcRenderer
participant ipcMain
participant SupportedVersions
participant Redux
UnsupportedServer->>ipcRenderer: invoke refresh-supported-versions with serverUrl
ipcRenderer->>ipcMain: forward refresh request
ipcMain->>SupportedVersions: update supported versions
SupportedVersions->>Redux: dispatch supported-version verdict
Redux->>UnsupportedServer: render updated state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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
🧹 Nitpick comments (1)
src/servers/supportedVersions/main.main.spec.ts (1)
2544-2556: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider testing the resume handler's behavior, not just its registration.
The test verifies
powerMonitor.on('resume', ...)is called, but doesn't invoke the handler to confirm it triggersupdateSupportedVersionsDatafor all servers. Invoking the registered callback and asserting the refresh calls would strengthen coverage against regressions where the handler body changes but the registration stays the same.♻️ Proposed enhancement: invoke and verify the resume handler
it('registers a powerMonitor resume handler when checkSupportedVersionServers runs', async () => { const electronMock = jest.requireMock('electron'); checkSupportedVersionServers(); expect(electronMock.powerMonitor.on).toHaveBeenCalledWith( 'resume', expect.any(Function) ); + + // Invoke the registered handler and verify revalidation + const resumeHandler = electronMock.powerMonitor.on.mock.calls.find( ([event]) => event === 'resume' )?.[1]; expect(resumeHandler).toBeDefined(); + + const mockServers = [ createMockServer({ url: 'https://a.rocket.chat/' }), createMockServer({ url: 'https://b.rocket.chat/' }), ]; selectMock.mockReturnValue(mockServers); + + resumeHandler(); + // Allow microtasks to flush + await Promise.resolve(); + + // Both servers should trigger a refresh + expect(selectMock).toHaveBeenCalled(); });🤖 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/supportedVersions/main.main.spec.ts` around lines 2544 - 2556, The power resume test in checkSupportedVersionServers only verifies that powerMonitor.on registers a resume handler, but it does not validate the handler’s behavior. Update the spec around checkSupportedVersionServers to capture the resume callback passed to electronMock.powerMonitor.on, invoke it, and assert that updateSupportedVersionsData is called for all servers (and any related refresh path) so the test covers the actual revalidation logic rather than just registration.
🤖 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/supportedVersions/main.ts`:
- Around line 550-591: `validateFallbackAndDispatch` always falls through to
`WEBVIEW_SERVER_SUPPORTED_VERSIONS_ERROR`, so a successful fallback validation
gets overwritten from success to error. Add an early return after the successful
`WEBVIEW_SERVER_IS_SUPPORTED_VERSION` and `dispatchSupportedVersionsUpdated`
calls, matching the guard used in the server and cloud paths, so the error
dispatch only happens in the catch path or when stale.
---
Nitpick comments:
In `@src/servers/supportedVersions/main.main.spec.ts`:
- Around line 2544-2556: The power resume test in checkSupportedVersionServers
only verifies that powerMonitor.on registers a resume handler, but it does not
validate the handler’s behavior. Update the spec around
checkSupportedVersionServers to capture the resume callback passed to
electronMock.powerMonitor.on, invoke it, and assert that
updateSupportedVersionsData is called for all servers (and any related refresh
path) so the test covers the actual revalidation logic rather than just
registration.
🪄 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: 9505ac66-6426-4f40-9b68-f1a1ffe33cc3
📒 Files selected for processing (10)
src/i18n/en.i18n.jsonsrc/servers/reducers.tssrc/servers/reducers/__tests__/servers.spec.tssrc/servers/supportedVersions/main.main.spec.tssrc/servers/supportedVersions/main.tssrc/ui/components/ServersView/ServerPane.tsxsrc/ui/components/ServersView/UnsupportedServer.spec.tsxsrc/ui/components/ServersView/UnsupportedServer.tsxsrc/ui/components/SupportedVersionDialog/index.spec.tsxsrc/ui/components/SupportedVersionDialog/index.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript for all new code unless explicitly told otherwise.
Use Fuselage components for all UI work; only create custom components when Fuselage does not provide what is needed.
Import UI components from@rocket.chat/fuselage.
CheckTheme.d.tsfor valid color tokens before using Fuselage theme colors.
Use optional chaining with fallbacks for platform-specific APIs, especially Linux-only process APIs likeprocess.getuid(),process.getgid(),process.geteuid(), andprocess.getegid().
Only mock platform-specific APIs when defensive coding is not possible.
Use TypeScript strict mode.
Use React functional components with hooks.
Redux actions must follow the Flux Standard Action (FSA) pattern.
Use camelCase for file names and PascalCase for components.
**/*.{ts,tsx}: Use TypeScript for new code unless explicitly told otherwise.
Use Fuselage components from@rocket.chat/fuselagefor UI work unless the design requires something Fuselage does not provide.
CheckTheme.d.tsfor valid color tokens before using Fuselage colors.
Verify library props, APIs, and tokens against official docs or local.d.tsfiles instead of assuming.
Use React functional components with hooks.
Redux actions follow FSA shape.
Use camelCase for file names and PascalCase for components.
Prefer clear names over unnecessary comments.
Prefer editing existing files over creating new abstractions unless the new abstraction removes real complexity or matches an existing pattern.
Files:
src/ui/components/ServersView/ServerPane.tsxsrc/ui/components/ServersView/UnsupportedServer.spec.tsxsrc/servers/reducers/__tests__/servers.spec.tssrc/ui/components/SupportedVersionDialog/index.tsxsrc/ui/components/SupportedVersionDialog/index.spec.tsxsrc/servers/reducers.tssrc/ui/components/ServersView/UnsupportedServer.tsxsrc/servers/supportedVersions/main.main.spec.tssrc/servers/supportedVersions/main.ts
**/*.spec.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsxfor renderer process tests.
Files:
src/ui/components/ServersView/UnsupportedServer.spec.tsxsrc/ui/components/SupportedVersionDialog/index.spec.tsx
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs use
*.spec.ts/*.spec.tsx.
Files:
src/ui/components/ServersView/UnsupportedServer.spec.tsxsrc/servers/reducers/__tests__/servers.spec.tssrc/ui/components/SupportedVersionDialog/index.spec.tsxsrc/servers/supportedVersions/main.main.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfor renderer process tests.
Files:
src/servers/reducers/__tests__/servers.spec.tssrc/servers/supportedVersions/main.main.spec.ts
**/*.main.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.main.spec.tsfor main process tests.Main-process specs use
*.main.spec.ts.
Files:
src/servers/supportedVersions/main.main.spec.ts
src/*/*/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Renderer specs must live in a Jest-matched nested path, such as
src/<module>/<subdir>/*.spec.ts(x); flatsrc/<module>/*.spec.tsfiles are not discovered by the currenttestMatch.
Files:
src/servers/supportedVersions/main.main.spec.ts
🧠 Learnings (1)
📚 Learning: 2026-06-26T18:14:15.295Z
Learnt from: jeanfbrito
Repo: RocketChat/Rocket.Chat.Electron PR: 3358
File: src/i18n/it-IT.i18n.json:39-42
Timestamp: 2026-06-26T18:14:15.295Z
Learning: In the i18n JSON files, the translation key `minimizeOnClose.disabledHint` is intentionally displayed when `isTrayIconEnabled` is true and the minimize-on-close toggle is disabled. The hint text should therefore instruct the user to disable the tray icon to make the setting available. During reviews, don’t “correct” this translation for seeming mismatches with the toggle state—first confirm it matches the component’s intended behavior; only update the wording if the underlying product logic/UX requirement changes.
Applied to files:
src/i18n/en.i18n.json
🔇 Additional comments (18)
src/servers/reducers.ts (1)
162-165: LGTM!Also applies to: 184-188, 220-223
src/servers/reducers/__tests__/servers.spec.ts (1)
195-205: LGTM!Also applies to: 241-251, 319-329
src/servers/supportedVersions/main.ts (6)
345-383: LGTM!
302-304: LGTM!
85-85: LGTM!
5-5: LGTM!
710-718: LGTM!
801-841: LGTM!src/servers/supportedVersions/main.main.spec.ts (4)
478-517: LGTM!Also applies to: 1976-2006, 2027-2063, 2089-2116, 2114-2141
2449-2499: LGTM!
2501-2526: LGTM!
2527-2543: LGTM!src/ui/components/ServersView/UnsupportedServer.tsx (1)
15-27: LGTM!Also applies to: 37-40, 76-78
src/ui/components/ServersView/ServerPane.tsx (1)
237-237: LGTM!src/i18n/en.i18n.json (1)
492-493: LGTM!src/ui/components/ServersView/UnsupportedServer.spec.tsx (1)
1-96: LGTM!src/ui/components/SupportedVersionDialog/index.tsx (1)
18-18: LGTM!Also applies to: 28-38, 142-144
src/ui/components/SupportedVersionDialog/index.spec.tsx (1)
1-9: LGTM!Also applies to: 171-203
| // Validates the fallback (cache/builtin) payload and dispatches the verdict. | ||
| // isServerVersionSupported can throw on a malformed cached/builtin payload | ||
| // (e.g. `versions` not an array). Left unguarded, that throw would reject | ||
| // updateSupportedVersionsData before WEBVIEW_SERVER_SUPPORTED_VERSIONS_ERROR | ||
| // is dispatched, leaving supportedVersionsFetchState stuck at 'loading' — | ||
| // which suppresses the UnsupportedServer block gate (fail-open). On failure, | ||
| // only the error state is dispatched; the prior verdict is left untouched. | ||
| const validateFallbackAndDispatch = async ( | ||
| server: Server, | ||
| serverUrl: string, | ||
| fallbackVersions: SupportedVersions, | ||
| fallbackSource: 'cloud' | 'builtin', | ||
| freshCommitHash: string | undefined, | ||
| isStale: () => boolean | ||
| ): Promise<void> => { | ||
| try { | ||
| const fallbackSupported = await isServerVersionSupported( | ||
| server, | ||
| fallbackVersions, | ||
| freshCommitHash, | ||
| fallbackSource | ||
| ); | ||
| if (isStale()) return; | ||
| dispatch({ | ||
| type: WEBVIEW_SERVER_IS_SUPPORTED_VERSION, | ||
| payload: { | ||
| url: server.url, | ||
| isSupportedVersion: fallbackSupported.supported, | ||
| }, | ||
| }); | ||
| dispatchSupportedVersionsUpdated(server.url, fallbackVersions, { | ||
| source: fallbackSource, | ||
| }); | ||
| } catch (error) { | ||
| console.error('Error validating fallback supported versions:', error); | ||
| } | ||
| if (isStale()) return; | ||
| dispatch({ | ||
| type: WEBVIEW_SERVER_SUPPORTED_VERSIONS_ERROR, | ||
| payload: { url: serverUrl }, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Critical: validateFallbackAndDispatch dispatches error state on the success path, overriding supportedVersionsFetchState: 'success' with 'error'.
After a successful validation, the try block dispatches WEBVIEW_SERVER_IS_SUPPORTED_VERSION and WEBVIEW_SERVER_SUPPORTED_VERSIONS_UPDATED (which sets supportedVersionsFetchState: 'success'), but there is no return before the post-try-catch WEBVIEW_SERVER_SUPPORTED_VERSIONS_ERROR dispatch, which overrides the fetch state to 'error'.
Both the server path (line 682) and cloud path (line 747) correctly return after dispatching their verdict. The fallback path is the only one missing this guard. The function's own comment states "On failure, only the error state is dispatched; the prior verdict is left untouched," but the implementation contradicts this for the success path.
The existing FIX-1 test only covers the failure path (malformed payload), so this bug goes undetected.
🐛 Proposed fix: add `return` after success-path dispatches
dispatchSupportedVersionsUpdated(server.url, fallbackVersions, {
source: fallbackSource,
});
+ return;
} catch (error) {
console.error('Error validating fallback supported versions:', error);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Validates the fallback (cache/builtin) payload and dispatches the verdict. | |
| // isServerVersionSupported can throw on a malformed cached/builtin payload | |
| // (e.g. `versions` not an array). Left unguarded, that throw would reject | |
| // updateSupportedVersionsData before WEBVIEW_SERVER_SUPPORTED_VERSIONS_ERROR | |
| // is dispatched, leaving supportedVersionsFetchState stuck at 'loading' — | |
| // which suppresses the UnsupportedServer block gate (fail-open). On failure, | |
| // only the error state is dispatched; the prior verdict is left untouched. | |
| const validateFallbackAndDispatch = async ( | |
| server: Server, | |
| serverUrl: string, | |
| fallbackVersions: SupportedVersions, | |
| fallbackSource: 'cloud' | 'builtin', | |
| freshCommitHash: string | undefined, | |
| isStale: () => boolean | |
| ): Promise<void> => { | |
| try { | |
| const fallbackSupported = await isServerVersionSupported( | |
| server, | |
| fallbackVersions, | |
| freshCommitHash, | |
| fallbackSource | |
| ); | |
| if (isStale()) return; | |
| dispatch({ | |
| type: WEBVIEW_SERVER_IS_SUPPORTED_VERSION, | |
| payload: { | |
| url: server.url, | |
| isSupportedVersion: fallbackSupported.supported, | |
| }, | |
| }); | |
| dispatchSupportedVersionsUpdated(server.url, fallbackVersions, { | |
| source: fallbackSource, | |
| }); | |
| } catch (error) { | |
| console.error('Error validating fallback supported versions:', error); | |
| } | |
| if (isStale()) return; | |
| dispatch({ | |
| type: WEBVIEW_SERVER_SUPPORTED_VERSIONS_ERROR, | |
| payload: { url: serverUrl }, | |
| }); | |
| }; | |
| // Validates the fallback (cache/builtin) payload and dispatches the verdict. | |
| // isServerVersionSupported can throw on a malformed cached/builtin payload | |
| // (e.g. `versions` not an array). Left unguarded, that throw would reject | |
| // updateSupportedVersionsData before WEBVIEW_SERVER_SUPPORTED_VERSIONS_ERROR | |
| // is dispatched, leaving supportedVersionsFetchState stuck at 'loading' — | |
| // which suppresses the UnsupportedServer block gate (fail-open). On failure, | |
| // only the error state is dispatched; the prior verdict is left untouched. | |
| const validateFallbackAndDispatch = async ( | |
| server: Server, | |
| serverUrl: string, | |
| fallbackVersions: SupportedVersions, | |
| fallbackSource: 'cloud' | 'builtin', | |
| freshCommitHash: string | undefined, | |
| isStale: () => boolean | |
| ): Promise<void> => { | |
| try { | |
| const fallbackSupported = await isServerVersionSupported( | |
| server, | |
| fallbackVersions, | |
| freshCommitHash, | |
| fallbackSource | |
| ); | |
| if (isStale()) return; | |
| dispatch({ | |
| type: WEBVIEW_SERVER_IS_SUPPORTED_VERSION, | |
| payload: { | |
| url: server.url, | |
| isSupportedVersion: fallbackSupported.supported, | |
| }, | |
| }); | |
| dispatchSupportedVersionsUpdated(server.url, fallbackVersions, { | |
| source: fallbackSource, | |
| }); | |
| return; | |
| } catch (error) { | |
| console.error('Error validating fallback supported versions:', error); | |
| } | |
| if (isStale()) return; | |
| dispatch({ | |
| type: WEBVIEW_SERVER_SUPPORTED_VERSIONS_ERROR, | |
| payload: { url: serverUrl }, | |
| }); | |
| }; |
🤖 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/supportedVersions/main.ts` around lines 550 - 591,
`validateFallbackAndDispatch` always falls through to
`WEBVIEW_SERVER_SUPPORTED_VERSIONS_ERROR`, so a successful fallback validation
gets overwritten from success to error. Add an early return after the successful
`WEBVIEW_SERVER_IS_SUPPORTED_VERSION` and `dispatchSupportedVersionsUpdated`
calls, matching the guard used in the server and cloud paths, so the error
dispatch only happens in the catch path or when stale.
A missing or unparseable enforcementStartDate previously produced an Invalid Date that failed the future-date comparison and fell through to the unsupported verdict — blocking the workspace based on incomplete payload data. Blocking now requires a valid, past enforcement date: uncertain data keeps the server usable until a payload with a valid enforcement date proves enforcement is active.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
1 participant
Stacked on #3404 — retarget to
masterafter it merges.What
Follow-up hardening pass over the supported-versions subsystem after the exception-scope investigation in #3404. Two independent audits (main-process flow and renderer surfaces) produced the findings below; every fix ships with a regression test.
Fixes
Validation robustness (main process)
updateSupportedVersionsDatabefore the error state was dispatched, leavingsupportedVersionsFetchStatestuck at'loading'— which suppresses the UnsupportedServer block gate. The error state is now always dispatched and the prior verdict is preserved.refresh-supported-versionsIPC handler).powerMonitor'resume'— the window'online'event does not fire when waking with the same network still connected.Exception scoping
settings.publicrestricted by API access controls) is never treated as a scope mismatch.exceptions.domaincomparison was already made case-insensitive in fix: honor uniqueId-scoped support exceptions on server-signed path #3404.State integrity
WEBVIEW_SERVER_UNIQUE_ID_UPDATED,WEBVIEW_SERVER_VERSION_UPDATED, andWEBVIEW_GIT_COMMIT_HASH_CHANGEDreducer cases switched fromupserttoupdate: a late identity dispatch for a server deleted while a validation was in flight can no longer resurrect a ghost server entry.Renderer
SupportedVersionDialogeffect dependencies used the importedcurrentViewreducer function (never changes) instead of the state value, so the dialog never re-checked when switching server views. Now selects the real state.getExpirationMessageTranslatedagainst payloads whose i18n dictionary lacks both the user language anden; the unguarded lookup crashed the async check and silently suppressed the expiring-workspace warning dialog.unsupportedServer.checkAgainen key; other locales fall back).Enforcement certainty
enforcementStartDateno longer blocks the workspace: an Invalid Date previously failed the future-date comparison and fell through to the unsupported verdict on incomplete payload data. Blocking now requires a valid, past enforcement date.Cosmetic
logRequestErrorprinted a literal${description}(single-quoted string).Out of scope, tracked for follow-up
enforcementStartDate⇒ supported (global grace masks a lapsed exception) — needs product confirmation.uniqueId/commit/full patch version in/api/info, which real unauthenticated servers do not send.Tests
tsc --noEmitandyarn lintclean.Summary by CodeRabbit
New Features
Bug Fixes