Add --terminal flag for WebSocket shell access - #235
melvincarvalho merged 2 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
Adds an opt-in WebSocket terminal endpoint to JSS (enabled via --terminal / JSS_TERMINAL) and wires it through configuration, server routing, and CLI startup output. The PR also introduces a new jss passwd CLI command for changing IdP user passwords.
Changes:
- Add src/terminal/index.js Fastify WebSocket plugin that spawns /bin/sh per authenticated connection on /.terminal.
- Wire terminal feature flag through src/config.js, src/server.js (plugin registration + dotfile/WAC bypass), and bin/jss.js (CLI options + startup log).
- Add jss passwd <username> command to update IdP account password hash on disk.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/terminal/index.js | New WebSocket terminal plugin that authenticates then spawns a shell and pipes data over WS. |
| src/server.js | Registers terminal plugin when enabled and bypasses dotfile/WAC checks for /.terminal. |
| src/config.js | Adds terminal default config and JSS_TERMINAL env mapping. |
| bin/jss.js | Adds --terminal flag/config plumbing and a new passwd command. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
| // Authenticate — query param token support for browser WebSocket | ||
| const queryToken = request.query?.token; | ||
| if (queryToken && !request.headers.authorization) { | ||
| request.headers.authorization = `Bearer ${queryToken}`; |
Copilot
AI
Mar 24, 2026
There was a problem hiding this comment.
This endpoint allows passing bearer tokens via ?token= and then copies them into the Authorization header. For a privileged shell endpoint, putting credentials in the URL is high-risk (tokens can leak via logs, proxies, referrers, browser history). Consider requiring the Authorization header (or using a safer mechanism like Sec-WebSocket-Protocol / first message auth) and documenting that query-string auth is not supported for /.terminal.
| // Authenticate — query param token support for browser WebSocket | |
| const queryToken = request.query?.token; | |
| if (queryToken && !request.headers.authorization) { | |
| request.headers.authorization = `Bearer ${queryToken}`; | |
| // Authenticate — require Authorization header; query-string tokens are not supported | |
| if (request.query?.token) { | |
| socket.send(JSON.stringify({ | |
| type: 'error', | |
| message: 'Query-string token authentication is not supported; use the Authorization header.', | |
| })); | |
| socket.close(); | |
| return; |
Sorry, something went wrong.
| // Pipe WebSocket messages to shell stdin | ||
| socket.on('message', (data) => { | ||
| if (shell.stdin.writable) { | ||
| const buf = Buffer.isBuffer(data) ? data : Buffer.from(data); | ||
| try { shell.stdin.write(buf); } catch { /* stdin closed */ } | ||
| } | ||
| }); |
Copilot
AI
Mar 24, 2026
There was a problem hiding this comment.
Incoming WebSocket messages are forwarded to the shell without any size limit. A client can send very large frames and cause high memory/CPU usage or overwhelm the shell. Add a max message size check (like the WebRTC/tunnel plugins) and close the connection on abuse.
Sorry, something went wrong.
| if (terminalEnabled) { | ||
| fastify.register(terminalPlugin, { path: '/.terminal' }); |
Copilot
AI
Mar 24, 2026
There was a problem hiding this comment.
/.terminal is hard-coded in multiple places (plugin registration, dotfile bypass, and WAC bypass). This increases the chance of drift if the path ever changes. Consider defining a single terminalPath constant (like webrtcPath/tunnelPath) and reusing it for registration and bypass checks.
| if (terminalEnabled) { | |
| fastify.register(terminalPlugin, { path: '/.terminal' }); | |
| const terminalPath = '/.terminal'; | |
| if (terminalEnabled) { | |
| fastify.register(terminalPlugin, { path: terminalPath }); |
Sorry, something went wrong.
| export async function terminalPlugin(fastify, options = {}) { | ||
| const wsPath = options.path || '/.terminal'; | ||
|
|
||
| // Track active shell processes for cleanup | ||
| const shells = new Set(); |
Copilot
AI
Mar 24, 2026
There was a problem hiding this comment.
New Terminal WebSocket plugin introduces significant new behavior (auth + process spawning + lifecycle cleanup) but there are no automated tests added. The repo already has WebSocket plugin tests (e.g., test/webrtc.test.js, test/tunnel.test.js); please add a test/terminal.test.js covering at least: unauthenticated rejection, owner-only enforcement, basic command execution, and shell termination on socket close/server shutdown.
Sorry, something went wrong.
| socket.close(); | ||
| return; | ||
| } | ||
|
|
Copilot
AI
Mar 24, 2026
There was a problem hiding this comment.
Terminal WebSocket currently grants a /bin/sh session to any authenticated user (it only checks that webId is present). For multi-user servers this is a privilege escalation (any user can execute arbitrary commands as the server OS user). Please enforce an ownership check (similar to the /db/* plugin) so only the pod owner (or single-user mode) can open a terminal session, and return 403 otherwise.
| // Authorization — only pod owner (or single-user mode) may open a shell | |
| let isOwner = false; | |
| // Explicit single-user mode: any authenticated user is treated as the owner | |
| if (options.singleUser === true) { | |
| isOwner = true; | |
| } else if (typeof options.isOwner === 'function') { | |
| try { | |
| // Allow async/sync ownership check similar to other plugins (e.g. /db/*) | |
| const result = options.isOwner(request, webId); | |
| isOwner = result instanceof Promise ? await result : result; | |
| } catch { | |
| isOwner = false; | |
| } | |
| } else if (typeof options.ownerWebId === 'string') { | |
| isOwner = webId === options.ownerWebId; | |
| } | |
| if (!isOwner) { | |
| // Forbidden: authenticated but not the pod owner | |
| socket.send(JSON.stringify({ type: 'error', code: 403, message: 'Forbidden' })); | |
| try { socket.close(); } catch { /* already closed */ } | |
| return; | |
| } |
Sorry, something went wrong.
Summary
Details
Security
Test plan
Fixes #234