fix(cli): address pid file creation issues (#2124)

This commit is contained in:
Alex Rudenko
2026-05-26 11:28:46 +02:00
committed by GitHub
parent 35c9c7f154
commit 1b51a520f2
2 changed files with 204 additions and 5 deletions
+78 -5
View File
@@ -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);
+126
View File
@@ -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
}
});
});