Fix terminal plugin bugs - #238
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts the --terminal WebSocket shell feature to improve runtime compatibility with newer @fastify/websocket handler shapes, improve interactive shell UX, and change authentication behavior when the server is started in public mode.
Changes:
- Use
connection.socket || connectionto support both old and new@fastify/websocketconnection handler shapes. - Spawn an interactive bash (
/bin/bash -i) instead of/bin/sh. - Normalize PTY output newlines by translating
\nto\r\nbefore sending over WebSocket. - Pass
publicfromserver.jsinto the terminal plugin and skip auth whenpublicis enabled.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/terminal/index.js | Updates WebSocket socket selection, authentication gating, shell spawning, and output newline normalization. |
| src/server.js | Passes public option into terminal plugin registration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| shell.stdout.on('data', (data) => { | ||
| if (socket.readyState === 1) { | ||
| try { socket.send(data); } catch { /* socket closed */ } | ||
| try { socket.send(data.toString().replace(/\n/g, '\r\n')); } catch { /* socket closed */ } | ||
| } |
There was a problem hiding this comment.
Replacing every \n with \r\n will turn existing CRLF sequences into \r\r\n, which can cause extra blank lines in terminals that already emit CRLF. Use a pattern that normalizes both LF and CRLF (e.g., replace \r?\n), or otherwise avoid doubling carriage returns.
| shell.stderr.on('data', (data) => { | ||
| if (socket.readyState === 1) { | ||
| try { socket.send(data); } catch { /* socket closed */ } | ||
| try { socket.send(data.toString().replace(/\n/g, '\r\n')); } catch { /* socket closed */ } | ||
| } |
There was a problem hiding this comment.
Same CRLF-normalization issue on stderr: converting all \n to \r\n can double existing CRLF (\r\r\n). Normalize \r?\n instead to avoid introducing extra carriage returns.
|
|
||
| // Spawn shell | ||
| const shell = spawn('/bin/sh', [], { | ||
| const shell = spawn('/bin/bash', ['-i'], { |
There was a problem hiding this comment.
Hard-coding /bin/bash reduces portability (e.g., minimal/distroless images or Alpine may not have bash at that path) and may cause the terminal feature to fail at runtime. Consider resolving the shell from process.env.SHELL, using bash via PATH, or falling back to /bin/sh when bash isn't available.
| const shell = spawn('/bin/bash', ['-i'], { | |
| const shellCommand = process.env.SHELL || 'bash'; | |
| const shell = spawn(shellCommand, ['-i'], { |
| if (!webId && !options.public) { | ||
| socket.send(JSON.stringify({ type: 'error', message: 'Authentication required' })); | ||
| socket.close(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
There are existing WebSocket plugin tests (e.g. tunnel/webrtc), but no automated coverage around the terminal plugin’s auth behavior. Given the changed semantics (public vs authenticated), add tests to ensure unauthenticated connections are rejected by default and only allowed under an explicit opt-in, and that newline normalization behaves as expected.
| if (!webId && !options.public) { | ||
| socket.send(JSON.stringify({ type: 'error', message: 'Authentication required' })); | ||
| socket.close(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Tying terminal authentication to options.public effectively makes /.terminal an unauthenticated remote shell whenever the server runs in public mode. Since public is intended to bypass WAC for resource access, reusing it here is a major security footgun. Consider requiring a dedicated explicit opt-in (e.g. terminalPublic / allowUnauthenticatedTerminal) and/or restricting unauthenticated access to loopback only, while keeping auth required by default even in public mode.
| // Register terminal (WebSocket shell) if enabled | ||
| if (terminalEnabled) { | ||
| fastify.register(terminalPlugin, { path: '/.terminal' }); | ||
| fastify.register(terminalPlugin, { path: '/.terminal', public: options.public || false }); |
There was a problem hiding this comment.
Passing public: options.public into the terminal plugin means starting the server in public mode will expose an unauthenticated WebSocket shell. This is likely too dangerous for a flag that is meant for public resource access; consider a separate CLI/server option to explicitly enable unauthenticated terminal access (or omit this option entirely).
| fastify.register(terminalPlugin, { path: '/.terminal', public: options.public || false }); | |
| fastify.register(terminalPlugin, { path: '/.terminal' }); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const shellCommand = process.env.SHELL || 'bash'; | ||
| const shell = spawn(shellCommand, ['-i'], { | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| env: { ...process.env, TERM: 'xterm-256color' }, | ||
| }); |
There was a problem hiding this comment.
shellCommand falls back to 'bash', which can fail on systems where bash isn’t installed (e.g., minimal images) and is a behavior change from the previous /bin/sh. Consider falling back to /bin/sh (or /bin/bash if present) when process.env.SHELL is unset or invalid, and/or allow an explicit config option/env var to choose the shell.
Summary
connection.socket || connectionfallback for @fastify/websocket compatibility across versions/bin/bash -i(interactive) instead of/bin/shfor proper prompt and shell features\nto\r\non stdout/stderr before sending to WebSocket, fixing terminal renderingoptions.publicis true; passpublicoption from server.js to the terminal pluginFixes #237
Test plan
--terminal --publicand connect via WebSocket — should get a shell without auth--terminal(no--public) without auth token — should get "Authentication required" error