Skip to content

Fix terminal plugin bugs #238

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
melvincarvalho merged 2 commits into from
Mar 24, 2026
Merged

Fix terminal plugin bugs #238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0fe5ea3
Fix terminal: socket ref, PTY newlines, bash -i, public mode. Fixes #237
melvincarvalho Mar 24, 2026
3a83b96
Fix CRLF normalization and use env SHELL with fallback
melvincarvalho Mar 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ export function createServer(options = {}) {

// Register terminal (WebSocket shell) if enabled
if (terminalEnabled) {
fastify.register(terminalPlugin, { path: '/.terminal' });
fastify.register(terminalPlugin, { path: '/.terminal', public: options.public || false });

Copilot AI Mar 24, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
fastify.register(terminalPlugin, { path: '/.terminal', public: options.public || false });
fastify.register(terminalPlugin, { path: '/.terminal' });

Copilot uses AI. Check for mistakes.
}

// Register tunnel proxy if enabled
Expand Down
11 changes: 6 additions & 5 deletions src/terminal/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ export async function terminalPlugin(fastify, options = {}) {
});

fastify.get(wsPath, { websocket: true }, async (connection, request) => {
const socket = connection.socket;
const socket = connection.socket || connection;

// Authenticate — query param token support for browser WebSocket
const queryToken = request.query?.token;
Expand All @@ -57,14 +57,15 @@ export async function terminalPlugin(fastify, options = {}) {
}
const { webId } = await getWebIdFromRequestAsync(request);

if (!webId) {
if (!webId && !options.public) {
socket.send(JSON.stringify({ type: 'error', message: 'Authentication required' }));
socket.close();
return;
}
Comment on lines +60 to 64

Copilot AI Mar 24, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +60 to 64

Copilot AI Mar 24, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

// Spawn shell
const shell = spawn('/bin/sh', [], {
const shellCommand = process.env.SHELL || 'bash';
const shell = spawn(shellCommand, ['-i'], {
stdio: ['pipe', 'pipe', 'pipe'],
env: { ...process.env, TERM: 'xterm-256color' },
});
Comment on lines +67 to 71

Copilot AI Mar 24, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Expand All @@ -74,14 +75,14 @@ export async function terminalPlugin(fastify, options = {}) {
// Pipe shell stdout to WebSocket
shell.stdout.on('data', (data) => {
if (socket.readyState === 1) {
try { socket.send(data); } catch { /* socket closed */ }
try { socket.send(data.toString().replace(/\r?\n/g, '\r\n')); } catch { /* socket closed */ }
}
Comment on lines 76 to 79

Copilot AI Mar 24, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
});

// Pipe shell stderr to WebSocket
shell.stderr.on('data', (data) => {
if (socket.readyState === 1) {
try { socket.send(data); } catch { /* socket closed */ }
try { socket.send(data.toString().replace(/\r?\n/g, '\r\n')); } catch { /* socket closed */ }
}
Comment on lines 83 to 86

Copilot AI Mar 24, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
});

Expand Down
Loading