fix(cli): address pid file creation issues (#2124)
This commit is contained in:
+78
-5
@@ -6,8 +6,9 @@
|
||||
* SPDX-License-Identifier: Apache-2.0
|
||||
*/
|
||||
|
||||
import fs from 'node:fs';
|
||||
import fs, {constants, openSync, writeSync, closeSync} from 'node:fs';
|
||||
import {createServer, type Server} from 'node:net';
|
||||
import os from 'node:os';
|
||||
import path from 'node:path';
|
||||
import process from 'node:process';
|
||||
|
||||
@@ -36,10 +37,82 @@ if (isDaemonRunning(sessionId)) {
|
||||
process.exit(1);
|
||||
}
|
||||
const pidFilePath = getPidFilePath(sessionId);
|
||||
fs.mkdirSync(path.dirname(pidFilePath), {
|
||||
recursive: true,
|
||||
});
|
||||
fs.writeFileSync(pidFilePath, process.pid.toString());
|
||||
const pidDir = path.dirname(pidFilePath);
|
||||
const currentUserUid = os.userInfo().uid;
|
||||
|
||||
try {
|
||||
fs.mkdirSync(pidDir, {recursive: true});
|
||||
if (os.platform() !== 'win32') {
|
||||
// POSIX specific checks
|
||||
try {
|
||||
const stats = fs.statSync(pidDir);
|
||||
|
||||
// 1. Check Ownership: Ensure the directory is owned by the current user.
|
||||
if (stats.uid !== currentUserUid) {
|
||||
console.error(
|
||||
`[MCP Daemon] Critical error: PID directory ${pidDir} is not owned by the current user (Expected: ${currentUserUid}, Found: ${stats.uid}). Possible tampering.`,
|
||||
);
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
// 2. Check Permissions: Ensure the directory is not group or world-writable.
|
||||
// Mode is a number, e.g., 0o700. We check if bits for group/world write are set.
|
||||
const mode = stats.mode;
|
||||
if (mode & constants.S_IWGRP || mode & constants.S_IWOTH) {
|
||||
console.error(
|
||||
`[MCP Daemon] Critical error: PID directory ${pidDir} has insecure permissions (Mode: ${mode.toString(8)}). It should not be writable by group or others.`,
|
||||
);
|
||||
process.exit(1);
|
||||
}
|
||||
} catch (statErr) {
|
||||
console.error(
|
||||
`[MCP Daemon] Critical error stating PID directory ${pidDir}:`,
|
||||
statErr,
|
||||
);
|
||||
process.exit(1);
|
||||
}
|
||||
}
|
||||
} catch (err) {
|
||||
console.error(
|
||||
`[MCP Daemon] Critical error creating/validating PID directory: ${pidDir}`,
|
||||
err,
|
||||
);
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
let fd = -1;
|
||||
try {
|
||||
// Open the file with flags to:
|
||||
// - O_WRONLY: Write-only
|
||||
// - O_CREAT: Create if it doesn't exist
|
||||
// - O_TRUNC: Truncate to zero length if it exists
|
||||
// - O_NOFOLLOW: DO NOT follow symlinks.
|
||||
// - 0o600: Permissions: read/write for owner, no permissions for others.
|
||||
fd = openSync(
|
||||
pidFilePath,
|
||||
constants.O_WRONLY |
|
||||
constants.O_CREAT |
|
||||
constants.O_TRUNC |
|
||||
constants.O_NOFOLLOW,
|
||||
0o600,
|
||||
);
|
||||
writeSync(fd, process.pid.toString());
|
||||
} catch (err) {
|
||||
console.error(
|
||||
`[MCP Daemon] Critical error writing PID file: ${pidFilePath}`,
|
||||
err,
|
||||
);
|
||||
// If openSync fails due to O_NOFOLLOW on a symlink, the error will be caught here.
|
||||
process.exit(1);
|
||||
} finally {
|
||||
if (fd !== -1) {
|
||||
try {
|
||||
closeSync(fd);
|
||||
} catch (err) {
|
||||
console.error(`[MCP Daemon] Error closing PID file: ${pidFilePath}`, err);
|
||||
}
|
||||
}
|
||||
}
|
||||
logger(`Writing ${process.pid.toString()} to ${pidFilePath}`);
|
||||
|
||||
const socketPath = getSocketPath(sessionId);
|
||||
|
||||
@@ -0,0 +1,126 @@
|
||||
/**
|
||||
* @license
|
||||
* Copyright 2026 Google LLC
|
||||
* SPDX-License-Identifier: Apache-2.0
|
||||
*/
|
||||
|
||||
import assert from 'node:assert';
|
||||
import {spawn} from 'node:child_process';
|
||||
import crypto from 'node:crypto';
|
||||
import fs from 'node:fs';
|
||||
import path from 'node:path';
|
||||
import process from 'node:process';
|
||||
import {describe, it, afterEach, beforeEach} from 'node:test';
|
||||
|
||||
import {
|
||||
DAEMON_SCRIPT_PATH,
|
||||
getPidFilePath,
|
||||
IS_WINDOWS,
|
||||
} from '../../src/daemon/utils.js';
|
||||
|
||||
describe('daemon security checks', () => {
|
||||
let sessionId: string;
|
||||
|
||||
beforeEach(() => {
|
||||
sessionId = crypto.randomUUID();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
const pidFilePath = getPidFilePath(sessionId);
|
||||
const pidDir = path.dirname(pidFilePath);
|
||||
try {
|
||||
fs.unlinkSync(pidFilePath);
|
||||
} catch {
|
||||
// ignore
|
||||
}
|
||||
try {
|
||||
fs.rmdirSync(pidDir);
|
||||
} catch {
|
||||
// ignore
|
||||
}
|
||||
});
|
||||
|
||||
it('should not follow symlinks and fail to write PID file', async () => {
|
||||
if (IS_WINDOWS) {
|
||||
return;
|
||||
}
|
||||
const pidFilePath = getPidFilePath(sessionId);
|
||||
const pidDir = path.dirname(pidFilePath);
|
||||
|
||||
// Ensure directory exists with safe permissions
|
||||
fs.mkdirSync(pidDir, {recursive: true});
|
||||
fs.chmodSync(pidDir, 0o700);
|
||||
|
||||
// Create a target file that we do NOT want to be overwritten
|
||||
const targetPath = path.join(pidDir, 'target_file.txt');
|
||||
fs.writeFileSync(targetPath, 'original content', 'utf-8');
|
||||
|
||||
// Create a symlink at pidFilePath pointing to targetPath
|
||||
fs.symlinkSync(targetPath, pidFilePath);
|
||||
|
||||
// Try to spawn the daemon
|
||||
const child = spawn(process.execPath, [DAEMON_SCRIPT_PATH], {
|
||||
env: {...process.env, CHROME_DEVTOOLS_MCP_SESSION_ID: sessionId},
|
||||
});
|
||||
|
||||
const exitCode = await new Promise<number | null>(resolve => {
|
||||
child.on('exit', code => {
|
||||
resolve(code);
|
||||
});
|
||||
});
|
||||
|
||||
// Daemon should have exited with error code 1
|
||||
assert.strictEqual(exitCode, 1);
|
||||
|
||||
// Target file content should remain unchanged ("original content")
|
||||
const content = fs.readFileSync(targetPath, 'utf-8');
|
||||
assert.strictEqual(content, 'original content');
|
||||
|
||||
// Clean up target file and symlink
|
||||
try {
|
||||
fs.unlinkSync(pidFilePath);
|
||||
} catch {
|
||||
// ignore
|
||||
}
|
||||
try {
|
||||
fs.unlinkSync(targetPath);
|
||||
} catch {
|
||||
// ignore
|
||||
}
|
||||
});
|
||||
|
||||
it('should fail if directory has insecure permissions (group/world writable)', async () => {
|
||||
if (IS_WINDOWS) {
|
||||
return;
|
||||
}
|
||||
const pidFilePath = getPidFilePath(sessionId);
|
||||
const pidDir = path.dirname(pidFilePath);
|
||||
|
||||
// Ensure directory exists
|
||||
fs.mkdirSync(pidDir, {recursive: true});
|
||||
|
||||
// Change permissions to 0o777 (group and world writable)
|
||||
fs.chmodSync(pidDir, 0o777);
|
||||
|
||||
// Try to spawn the daemon
|
||||
const child = spawn(process.execPath, [DAEMON_SCRIPT_PATH], {
|
||||
env: {...process.env, CHROME_DEVTOOLS_MCP_SESSION_ID: sessionId},
|
||||
});
|
||||
|
||||
const exitCode = await new Promise<number | null>(resolve => {
|
||||
child.on('exit', code => {
|
||||
resolve(code);
|
||||
});
|
||||
});
|
||||
|
||||
// Daemon should have exited with error code 1
|
||||
assert.strictEqual(exitCode, 1);
|
||||
|
||||
// Restore permissions so cleanup can run successfully
|
||||
try {
|
||||
fs.chmodSync(pidDir, 0o700);
|
||||
} catch {
|
||||
// ignore
|
||||
}
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user