Skip to content

Commit fd2d3d7

Browse files
fix(security): harden session creation input validation (#104)
## Summary Adds type validation for `args` and `initialCommand` parameters in session creation to address CodeQL alert #14 (code injection). ### Changes - **routes.js**: Validate `args` is an array of strings and `initialCommand` is a string at the API layer - **sessions.js**: Defense-in-depth type checks before `pty.spawn()` - **routes.test.js**: 3 new tests covering invalid `args` (non-array, mixed types) and non-string `initialCommand` ### Context CodeQL flags user-provided values flowing into `pty.spawn()` as code injection. While this is by design (TermBeam is a terminal emulator), adding type validation prevents unexpected object types from reaching the PTY layer. Closes #98 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 8f15c66 commit fd2d3d7

3 files changed

Lines changed: 96 additions & 8 deletions

File tree

src/routes.js

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,17 @@ const pageRateLimit = rateLimit({
1515
max: 120,
1616
standardHeaders: true,
1717
legacyHeaders: false,
18-
handler: (_req, res) => res.status(429).json({ error: 'Too many requests, please try again later.' }),
18+
handler: (_req, res) =>
19+
res.status(429).json({ error: 'Too many requests, please try again later.' }),
1920
});
2021

2122
const apiRateLimit = rateLimit({
2223
windowMs: 1 * 60 * 1000,
2324
max: 120,
2425
standardHeaders: true,
2526
legacyHeaders: false,
26-
handler: (_req, res) => res.status(429).json({ error: 'Too many requests, please try again later.' }),
27+
handler: (_req, res) =>
28+
res.status(429).json({ error: 'Too many requests, please try again later.' }),
2729
});
2830

2931
const IMAGE_SIGNATURES = [
@@ -51,7 +53,6 @@ function validateMagicBytes(buffer, contentType) {
5153
}
5254

5355
function setupRoutes(app, { auth, sessions, config, state }) {
54-
5556
// Serve static files (manifest.json, sw.js, icons, etc.)
5657
app.use(express.static(PUBLIC_DIR, { index: false }));
5758

@@ -143,6 +144,20 @@ function setupRoutes(app, { auth, sessions, config, state }) {
143144
}
144145
}
145146

147+
// Validate args field — must be an array of strings
148+
if (shellArgs !== undefined) {
149+
if (!Array.isArray(shellArgs) || !shellArgs.every((a) => typeof a === 'string')) {
150+
return res.status(400).json({ error: 'args must be an array of strings' });
151+
}
152+
}
153+
154+
// Validate initialCommand field — must be a string
155+
if (initialCommand !== undefined && initialCommand !== null) {
156+
if (typeof initialCommand !== 'string') {
157+
return res.status(400).json({ error: 'initialCommand must be a string' });
158+
}
159+
}
160+
146161
// Validate cwd field
147162
if (cwd) {
148163
if (!path.isAbsolute(cwd)) {
@@ -164,7 +179,7 @@ function setupRoutes(app, { auth, sessions, config, state }) {
164179
shell: shell || config.defaultShell,
165180
args: shellArgs || [],
166181
cwd: cwd ? path.resolve(cwd) : config.cwd,
167-
initialCommand: initialCommand || null,
182+
initialCommand: initialCommand ?? null,
168183
color: color || null,
169184
cols: typeof cols === 'number' && cols > 0 && cols <= 500 ? Math.floor(cols) : undefined,
170185
rows: typeof rows === 'number' && rows > 0 && rows <= 200 ? Math.floor(rows) : undefined,

src/sessions.js

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ const pty = require('node-pty');
66
const log = require('./logger');
77
const { getGitInfo } = require('./git');
88

9-
109
function _getProcessCwd(pid) {
1110
try {
1211
if (process.platform === 'linux') {
@@ -111,12 +110,23 @@ class SessionManager {
111110
rows = 30,
112111
}) {
113112
// Defense-in-depth: reject shells with dangerous characters or relative paths
114-
if (typeof shell !== 'string' || !shell ||
115-
/[;&|`$(){}\[\]!#~]/.test(shell) ||
116-
(!path.isAbsolute(shell) && !shell.match(/^[a-zA-Z0-9._-]+(\.exe)?$/))) {
113+
if (
114+
typeof shell !== 'string' ||
115+
!shell ||
116+
/[;&|`$(){}\[\]!#~]/.test(shell) ||
117+
(!path.isAbsolute(shell) && !shell.match(/^[a-zA-Z0-9._-]+(\.exe)?$/))
118+
) {
117119
throw new Error('Invalid shell');
118120
}
119121

122+
// Defense-in-depth: validate args and initialCommand types
123+
if (!Array.isArray(args) || !args.every((a) => typeof a === 'string')) {
124+
throw new Error('args must be an array of strings');
125+
}
126+
if (initialCommand !== null && typeof initialCommand !== 'string') {
127+
throw new Error('initialCommand must be a string');
128+
}
129+
120130
const id = crypto.randomBytes(16).toString('hex');
121131
if (!color) {
122132
color = SESSION_COLORS[this.sessions.size % SESSION_COLORS.length];

test/routes.test.js

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -505,6 +505,69 @@ describe('Routes', () => {
505505
assert.strictEqual(data.error, 'cwd does not exist');
506506
});
507507

508+
it('should reject non-array args with 400', async () => {
509+
if (!inst) inst = await startServer();
510+
const body = JSON.stringify({ args: 'not-an-array' });
511+
const res = await httpRequest(
512+
{
513+
hostname: '127.0.0.1',
514+
port: inst.port,
515+
path: '/api/sessions',
516+
method: 'POST',
517+
headers: {
518+
'Content-Type': 'application/json',
519+
'Content-Length': Buffer.byteLength(body),
520+
},
521+
},
522+
body,
523+
);
524+
assert.strictEqual(res.statusCode, 400);
525+
const data = JSON.parse(res.data);
526+
assert.strictEqual(data.error, 'args must be an array of strings');
527+
});
528+
529+
it('should reject args with non-string elements with 400', async () => {
530+
if (!inst) inst = await startServer();
531+
const body = JSON.stringify({ args: ['valid', 123] });
532+
const res = await httpRequest(
533+
{
534+
hostname: '127.0.0.1',
535+
port: inst.port,
536+
path: '/api/sessions',
537+
method: 'POST',
538+
headers: {
539+
'Content-Type': 'application/json',
540+
'Content-Length': Buffer.byteLength(body),
541+
},
542+
},
543+
body,
544+
);
545+
assert.strictEqual(res.statusCode, 400);
546+
const data = JSON.parse(res.data);
547+
assert.strictEqual(data.error, 'args must be an array of strings');
548+
});
549+
550+
it('should reject non-string initialCommand with 400', async () => {
551+
if (!inst) inst = await startServer();
552+
const body = JSON.stringify({ initialCommand: 12345 });
553+
const res = await httpRequest(
554+
{
555+
hostname: '127.0.0.1',
556+
port: inst.port,
557+
path: '/api/sessions',
558+
method: 'POST',
559+
headers: {
560+
'Content-Type': 'application/json',
561+
'Content-Length': Buffer.byteLength(body),
562+
},
563+
},
564+
body,
565+
);
566+
assert.strictEqual(res.statusCode, 400);
567+
const data = JSON.parse(res.data);
568+
assert.strictEqual(data.error, 'initialCommand must be a string');
569+
});
570+
508571
it('should create session with valid data', async () => {
509572
if (!inst) inst = await startServer();
510573
const body = JSON.stringify({ name: 'Valid Session' });

0 commit comments

Comments
 (0)