feat(terminal): super_user PTY-over-WebSocket prototype (Studio terminal)#1755
Draft
dawsontoth wants to merge 3 commits into
Draft
feat(terminal): super_user PTY-over-WebSocket prototype (Studio terminal)#1755dawsontoth wants to merge 3 commits into
dawsontoth wants to merge 3 commits into
Conversation
Prototype (for team review) of an interactive terminal exposed from a Harper instance, so Studio can render an xterm.js shell running inside the container. The authenticated WebSocket channel it establishes is the shared substrate the planned live-log-stream and inspector-bridge features reuse. Design (per review): served on the OPERATIONS API port (same security boundary as the rest of the operations surface, not the app data port), with FIRST-MESSAGE auth (credential in the first WS frame, not the handshake). - components/terminal: new built-in. registerTerminalWebSocket() attaches a `/terminal` upgrade handler to the operations Fastify server's node server (dedicated ws.WebSocketServer, subprotocol `harper-terminal`). Accepts the socket unauthenticated, then validates the first frame (operation token via validateOperationToken, or basic via server.authenticateUser), asserts super_user, and spawns a PTY (node-pty, optional native dep, dynamically imported). Opcode-framed wire protocol, 5s auth timeout, idle timeout, per-thread session cap, notify-level open/close/deny audit logging. - server/operationsServer.ts: call registerTerminalWebSocket in buildServer (next to the MCP profile), gated on terminal.enabled. - config-root.schema.json: opt-in `terminal` block (enabled: false default). - package.json: node-pty as an optionalDependency. No change to security/auth.ts — first-message auth is handled entirely in the component. Off by default; direct-connection only (Fabric Connect proxy buffers streams). See components/terminal/README.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a prototype for a built-in interactive terminal component, exposing a super_user-only PTY over a WebSocket on the operations API port. The feedback highlights critical security and stability improvements, including the addition of an authenticating flag to prevent concurrent authentication requests from leaking PTY processes, passing the missing request object to authenticateUser to avoid runtime errors, and configuring a maxPayload limit on the WebSocket server to mitigate potential Denial of Service (DoS) vulnerabilities.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
Reviewed; no blockers found. Prior blocker (closed socket during auth awaits — leaked |
- Prevent auth re-entrancy / double-PTY spawn: synchronous `authenticating`
flag set before the first await, plus reserve the session slot synchronously
(check + liveSessions++ with no await between, rolled back on PTY-start
failure) so concurrent connections can't overshoot maxSessions.
- Pass `req` through to `server.authenticateUser` (3-arg signature).
- Add `maxPayload` (1 MiB) and a `wss.on('error')` handler (unhandled 'error'
on the ws.Server would crash the process).
- Guard authenticated frames after PTY exit: bail if closed/no term, and wrap
`term.write` in try/catch → teardown (avoids unhandled rejection during the
WS close handshake).
- Sync package-lock.json for node-pty (fixes `npm ci` in CI).
- Add node-pty to dependencies.md; correct config schema wording to
"operations API port".
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add `if (closed) return` after `await authenticate()` and after `await loadPty()` (rolling back the reserved slot in the latter). Without these, a socket that closes mid-await — e.g. a second rapid auth frame that safeCloses the connection while the first authenticate() is still in flight — would still reach liveSessions++/pty.spawn() on a dead socket, leaking the session counter and orphaning the PTY. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prototype / draft — for team discussion. Not for merge as-is.
Adds a super_user-only interactive terminal (PTY over WebSocket) exposed from a Harper instance, so Studio can render an
xterm.jsshell running inside the container. This is the stepping-stone channel from the "Claude in the container" exploration; the authenticated WebSocket it establishes is the substrate the planned live-log-stream and Node-inspector-bridge features would reuse.Companion Studio PR: HarperFast/studio#1467
Design decisions (settled with the team)
registerTerminalWebSocketinbuildServer, right next to the MCP operations profile), gated onterminal.enabled. Keeps the terminal on the same security boundary as the rest of the operations surface, not the application data port.security/auth.ts.What's here
components/terminal/— the component + README (wire protocol, close codes, security notes).server/operationsServer.ts— wires it in, gated onterminal.enabled.config-root.schema.json— opt-interminalblock (default off).package.json—node-ptyas anoptionalDependency.Enable
Status / caveats
terminalblock present andenabled: true).components/terminal/README.md.main@d33f0970f;mainhas since advanced. Rebase before merge — expect touch-points inserver/operationsServer.ts,config-root.schema.json, andpackage.json.Verified locally end-to-end in the real Studio UI (see companion PR).
🤖 Generated with Claude Code