security(CORE-1122): macOS secure keyboard entry for sensitive inputs#3314
security(CORE-1122): macOS secure keyboard entry for sensitive inputs#3314jeanfbrito wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
WalkthroughAdds macOS-only secure keyboard entry: a new IPC channel and main-process setup to toggle Electron's secure keyboard entry, renderer wiring in the Outlook credentials dialog to invoke it on password focus/blur and window focus, plus platform-specific tests and cleanup integration. Changes
Sequence DiagramsequenceDiagram
participant User
participant Dialog as OutlookCredentialsDialog
participant RendererIPC as Renderer IPC (invoke)
participant Main as Main Process (ipcMain handler)
participant ElectronApp as Electron.app
User->>Dialog: focus password field
Dialog->>RendererIPC: invoke('secure-keyboard-entry/set', true)
RendererIPC->>Main: deliver 'secure-keyboard-entry/set' true
Main->>ElectronApp: app.setSecureKeyboardEntryEnabled(true)
User->>Dialog: blur password field
Dialog->>RendererIPC: invoke('secure-keyboard-entry/set', false)
RendererIPC->>Main: deliver 'secure-keyboard-entry/set' false
Main->>ElectronApp: app.setSecureKeyboardEntryEnabled(false)
Note over Dialog,Main: On window focus, Dialog may re-invoke when password still active
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 5/8 reviews remaining, refill in 19 minutes and 57 seconds.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 the current code and only fix it if needed.
Inline comments:
In `@src/ui/components/OutlookCredentialsDialog/index.tsx`:
- Around line 157-161: The PasswordInput onFocus/onBlur currently calls
invoke('secure-keyboard-entry/set', ...) unconditionally and overrides
react-hook-form's blur handler; wrap the IPC calls in a platform guard (only
call invoke when process.platform === 'darwin' or when
setupSecureKeyboardEntry() is active) and preserve react-hook-form handlers by
calling the form's onBlur returned from register inside your onBlur wrapper
(e.g., call the registered onBlur first/last) and similarly preserve any
registered onFocus if present; update the PasswordInput usage that spreads
{...register('password', { required: true })} to capture the returned
onBlur/onFocus and call those along with the guarded
invoke('secure-keyboard-entry/set', ...) so non-macOS platforms do not trigger a
rejected IPC and react-hook-form touched state remains correct.
🪄 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: a43a301b-5d3d-4a78-8126-2195d2cb7d01
📒 Files selected for processing (5)
src/ipc/channels.tssrc/ui/components/OutlookCredentialsDialog/index.tsxsrc/ui/main/rootWindow.tssrc/ui/main/secureKeyboardEntry.main.spec.tssrc/ui/main/secureKeyboardEntry.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript for all new code in this codebase unless explicitly told otherwise
Use Fuselage components from@rocket.chat/fuselagefor all UI work — only create custom components when Fuselage doesn't provide the needed functionality
Check Theme.d.ts for valid color tokens when working with Fuselage components
Use optional chaining with fallbacks for platform-specific APIs instead of mocks (e.g., process.getuid?.() ?? 1000) to ensure code works across all platforms without requiring mocks
TypeScript code must use strict mode
Use React functional components with hooks instead of class components
Redux actions must follow the FSA (Flux Standard Action) pattern
Use camelCase for file naming
Use PascalCase for component file names (React components)
Write self-documenting code through clear naming — avoid unnecessary comments
Files:
src/ipc/channels.tssrc/ui/components/OutlookCredentialsDialog/index.tsxsrc/ui/main/secureKeyboardEntry.tssrc/ui/main/secureKeyboardEntry.main.spec.tssrc/ui/main/rootWindow.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use *.spec.ts file naming convention for Renderer process tests
Files:
src/ui/main/secureKeyboardEntry.main.spec.ts
**/*.main.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use *.main.spec.ts file naming convention for Main process tests
Files:
src/ui/main/secureKeyboardEntry.main.spec.ts
**/*.{spec,main.spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{spec,main.spec}.ts: Tests must run and pass on Windows, macOS, and Linux CI environments — always verify cross-platform compatibility
Only mock Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) when defensive coding with optional chaining isn't possible
Files:
src/ui/main/secureKeyboardEntry.main.spec.ts
🧠 Learnings (2)
📚 Learning: 2026-04-06T18:34:44.375Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-06T18:34:44.375Z
Learning: Applies to **/*.{spec,main.spec}.ts : Only mock Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) when defensive coding with optional chaining isn't possible
Applied to files:
src/ui/main/secureKeyboardEntry.main.spec.ts
📚 Learning: 2026-04-06T18:34:44.375Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-06T18:34:44.375Z
Learning: Applies to **/*.{spec,main.spec}.ts : Tests must run and pass on Windows, macOS, and Linux CI environments — always verify cross-platform compatibility
Applied to files:
src/ui/main/secureKeyboardEntry.main.spec.ts
🔇 Additional comments (6)
src/ui/components/OutlookCredentialsDialog/index.tsx (1)
24-24: Import wiring looks good.
invokeis correctly pulled from the typed renderer IPC layer for this UI flow.src/ui/main/rootWindow.ts (2)
252-253: Lifecycle setup/cleanup is correctly wired.Registering once in
setupRootWindow()and removing onbefore-quitis a clean ownership model for this IPC handler.Also applies to: 531-531
348-352: Good macOS reset safeguards on focus loss and close.Disabling secure keyboard entry on both blur and close reduces risk of it staying enabled longer than intended.
Also applies to: 355-357
src/ipc/channels.ts (1)
139-139: Channel typing is clean and consistent.The boolean argument signature matches both renderer invocation and main-process handler expectations.
src/ui/main/secureKeyboardEntry.ts (1)
5-18: Implementation is compact and maintainable.The darwin gate plus returned cleanup function keeps the main-process integration straightforward and easy to reason about.
src/ui/main/secureKeyboardEntry.main.spec.ts (1)
15-96: Solid platform-branch coverage.The spec validates darwin behavior and non-darwin no-op behavior clearly, including cleanup on macOS.
| <PasswordInput | ||
| {...register('password', { required: true })} | ||
| onFocus={() => invoke('secure-keyboard-entry/set', true)} | ||
| onBlur={() => invoke('secure-keyboard-entry/set', false)} | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify darwin-only handler registration vs unguarded renderer invocation,
# and verify explicit onBlur override after register spread.
rg -n "secure-keyboard-entry/set|process.platform !== 'darwin'|register\('password'|onBlur=" \
src/ui/components/OutlookCredentialsDialog/index.tsx \
src/ui/main/secureKeyboardEntry.tsRepository: RocketChat/Rocket.Chat.Electron
Length of output: 603
🏁 Script executed:
cat -n src/ui/main/secureKeyboardEntry.tsRepository: RocketChat/Rocket.Chat.Electron
Length of output: 592
🏁 Script executed:
head -200 src/ui/components/OutlookCredentialsDialog/index.tsx | tail -100Repository: RocketChat/Rocket.Chat.Electron
Length of output: 3135
🏁 Script executed:
# Check for any other guards or error handling around invoke calls
rg -B5 -A5 "invoke\('secure-keyboard-entry" src/ui/components/OutlookCredentialsDialog/index.tsxRepository: RocketChat/Rocket.Chat.Electron
Length of output: 561
Guard IPC invocation by platform and preserve react-hook-form's blur handler
Lines 159–160 invoke the IPC channel unconditionally, but the handler is only registered on macOS (see setupSecureKeyboardEntry() in src/ui/main/secureKeyboardEntry.ts). On Linux and Windows, this will reject. Additionally, the onBlur override replaces react-hook-form's blur handler entirely, breaking the form's touched state tracking.
🔧 Proposed fix
+ const passwordField = register('password', { required: true });
+
+ const setSecureKeyboardEntry = (enabled: boolean): void => {
+ if (process.platform !== 'darwin') {
+ return;
+ }
+ void invoke('secure-keyboard-entry/set', enabled).catch(() => undefined);
+ };
+
<PasswordInput
- {...register('password', { required: true })}
+ {...passwordField}
- onFocus={() => invoke('secure-keyboard-entry/set', true)}
- onBlur={() => invoke('secure-keyboard-entry/set', false)}
+ onFocus={() => setSecureKeyboardEntry(true)}
+ onBlur={(event) => {
+ passwordField.onBlur(event);
+ setSecureKeyboardEntry(false);
+ }}
/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/components/OutlookCredentialsDialog/index.tsx` around lines 157 - 161,
The PasswordInput onFocus/onBlur currently calls
invoke('secure-keyboard-entry/set', ...) unconditionally and overrides
react-hook-form's blur handler; wrap the IPC calls in a platform guard (only
call invoke when process.platform === 'darwin' or when
setupSecureKeyboardEntry() is active) and preserve react-hook-form handlers by
calling the form's onBlur returned from register inside your onBlur wrapper
(e.g., call the registered onBlur first/last) and similarly preserve any
registered onFocus if present; update the PasswordInput usage that spreads
{...register('password', { required: true })} to capture the returned
onBlur/onFocus and call those along with the guarded
invoke('secure-keyboard-entry/set', ...) so non-macOS platforms do not trigger a
rejected IPC and react-hook-form touched state remains correct.
…spec On non-darwin platforms the secure-keyboard-entry IPC handler is never registered; guard the onFocus/onBlur invoke calls in OutlookCredentialsDialog with a process.platform === 'darwin' check to prevent unhandled rejections. Add jest.mock for secureKeyboardEntry in rootWindow.spec.ts so the darwin close-handler path does not call ipcMain.handle on an undefined mock, and stub app.setSecureKeyboardEntryEnabled so the macOS close test passes.
Codex P2 findings addressed (commit c80bd6a)Finding 1 — renderer call site unguarded on non-darwin Finding 2 — rootWindow.spec.ts breaks on darwin path
Verification: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ui/main/rootWindow.spec.ts`:
- Around line 42-44: The mock for setupSecureKeyboardEntry should return a
callable cleanup spy instead of a plain no-op so the test can assert cleanup was
invoked; update the jest.mock implementation (mock of setupSecureKeyboardEntry)
to return jest.fn() as the cleanup function and expose that spy to the spec,
then in the test call setupRootWindow() and trigger the shutdown path (e.g.,
invoke the before-quit handler) and assert the returned cleanup spy was called,
ensuring the IPC handler/unregister logic in setupRootWindow is exercised and
observed.
🪄 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: 646a3a5c-8593-4f95-b72c-08ab5a163965
📒 Files selected for processing (2)
src/ui/components/OutlookCredentialsDialog/index.tsxsrc/ui/main/rootWindow.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ui/components/OutlookCredentialsDialog/index.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check (windows-latest)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: Analyze (javascript)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript for all new code in this codebase unless explicitly told otherwise
Use Fuselage components from@rocket.chat/fuselagefor all UI work — only create custom components when Fuselage doesn't provide the needed functionality
Check Theme.d.ts for valid color tokens when working with Fuselage components
Use optional chaining with fallbacks for platform-specific APIs instead of mocks (e.g., process.getuid?.() ?? 1000) to ensure code works across all platforms without requiring mocks
TypeScript code must use strict mode
Use React functional components with hooks instead of class components
Redux actions must follow the FSA (Flux Standard Action) pattern
Use camelCase for file naming
Use PascalCase for component file names (React components)
Write self-documenting code through clear naming — avoid unnecessary comments
Files:
src/ui/main/rootWindow.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use *.spec.ts file naming convention for Renderer process tests
Files:
src/ui/main/rootWindow.spec.ts
**/*.{spec,main.spec}.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{spec,main.spec}.ts: Tests must run and pass on Windows, macOS, and Linux CI environments — always verify cross-platform compatibility
Only mock Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) when defensive coding with optional chaining isn't possible
Files:
src/ui/main/rootWindow.spec.ts
🧠 Learnings (2)
📚 Learning: 2026-04-06T18:34:44.375Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-06T18:34:44.375Z
Learning: Applies to **/*.{spec,main.spec}.ts : Only mock Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) when defensive coding with optional chaining isn't possible
Applied to files:
src/ui/main/rootWindow.spec.ts
📚 Learning: 2026-04-06T18:34:44.375Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-06T18:34:44.375Z
Learning: Applies to **/*.{ts,tsx} : Use optional chaining with fallbacks for platform-specific APIs instead of mocks (e.g., process.getuid?.() ?? 1000) to ensure code works across all platforms without requiring mocks
Applied to files:
src/ui/main/rootWindow.spec.ts
🔇 Additional comments (1)
src/ui/main/rootWindow.spec.ts (1)
9-9: Good addition: the Electron mock now covers the new macOS API.This keeps the
rootWindowclose-path test from failing on theapp.setSecureKeyboardEntryEnabled(...)call.
| jest.mock('./secureKeyboardEntry', () => ({ | ||
| setupSecureKeyboardEntry: jest.fn(() => () => undefined), | ||
| })); |
There was a problem hiding this comment.
Expose the cleanup function in the secure-keyboard mock.
Returning a plain no-op hides regressions in the before-quit cleanup path. The spec should be able to observe that setupRootWindow() unregisters the IPC handler on shutdown.
Suggested test mock
jest.mock('./secureKeyboardEntry', () => ({
- setupSecureKeyboardEntry: jest.fn(() => () => undefined),
+ setupSecureKeyboardEntry: jest.fn(() => {
+ const removeSecureKeyboardHandler = jest.fn();
+ return removeSecureKeyboardHandler;
+ }),
}));📝 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.
| jest.mock('./secureKeyboardEntry', () => ({ | |
| setupSecureKeyboardEntry: jest.fn(() => () => undefined), | |
| })); | |
| jest.mock('./secureKeyboardEntry', () => ({ | |
| setupSecureKeyboardEntry: jest.fn(() => { | |
| const removeSecureKeyboardHandler = jest.fn(); | |
| return removeSecureKeyboardHandler; | |
| }), | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ui/main/rootWindow.spec.ts` around lines 42 - 44, The mock for
setupSecureKeyboardEntry should return a callable cleanup spy instead of a plain
no-op so the test can assert cleanup was invoked; update the jest.mock
implementation (mock of setupSecureKeyboardEntry) to return jest.fn() as the
cleanup function and expose that spy to the spec, then in the test call
setupRootWindow() and trigger the shutdown path (e.g., invoke the before-quit
handler) and assert the returned cleanup spy was called, ensuring the IPC
handler/unregister logic in setupRootWindow is exercised and observed.
…le password field active When the BrowserWindow loses and regains OS focus while the password input is the active DOM element, the input's onFocus does not re-fire (DOM focus never left it). Track password-field active state in a ref and listen to the window 'focus' event on darwin to re-invoke secure-keyboard-entry/set(true) when needed. Adds renderer spec covering focus, blur, window-refocus, and cross-platform (linux/win32) guard paths.
Fix applied: re-enable secure keyboard entry on window refocus (Fix 1)Scenario addressed: user focuses the password field (secure entry ON), switches to another app (window blur fires → secure entry forced OFF), switches back. The password input is still the active DOM element but its Chosen fix: Fix 1 — track DOM-side state and re-enable on window focus. What changed
Verification
Commit: |
Adds cleanup for the case where OutlookCredentialsDialog is dismissed via Esc, overlay click, or state-driven hide without password onBlur firing. Two new useEffect hooks: one watching isVisible (covers state-driven close), one returning a cleanup on unmount. Both check passwordFieldActiveRef and call secure-keyboard-entry/set false only when needed, darwin-only. Extends index.spec.tsx with 7 new test cases covering unmount and state-change close paths on darwin, linux, and win32.
Fix: secure keyboard entry cleanup on dialog close pathsCommit: fix(CORE-1122): disable secure keyboard entry on dialog close paths ProblemWhen Fix (
|
1 participant
What changed
secure-keyboard-entry/setIPC channel (src/ipc/channels.ts) with payloadboolean.src/ui/main/secureKeyboardEntry.ts: registers anipcMain.handlefor that channel on macOS only, callingapp.setSecureKeyboardEntryEnabled(value). No-op on linux/win32.src/ui/main/rootWindow.ts: wiressetupSecureKeyboardEntry()intosetupRootWindow(); adds ablurlistener and a reset at the top of theclosehandler (both darwin-gated) to force secure entry off when the window loses focus or closes, preventing a stuck-on state.src/ui/components/OutlookCredentialsDialog/index.tsx: the password field'sonFocusinvokessecure-keyboard-entry/setwithtrue;onBlurinvokes it withfalse.Why
Q4-2022 security audit (CORE-1122) identified that the app does not protect credential entry with macOS Secure Keyboard Entry. Apple's guidance is to enable it only while a sensitive field is focused and disable it immediately after — leaving it globally enabled interferes with input methods and accessibility tools.
Known limitation
Server-side login (the Rocket.Chat login form) runs inside a webview. Injecting JS into third-party webviews to hook focus/blur is out of scope for this ticket and will be addressed in a follow-up ticket.
Test plan
yarn lint— passesnpx tsc --noEmit— cleanyarn test --testPathPattern="secureKeyboardEntry"— 9/9 tests passsetSecureKeyboardEntryEnabled(true/false)called, cleanup removes handlerSummary by CodeRabbit
New Features
Tests