From 1a214a5a9e669733fa8d5999c48ff64aaa392c9b Mon Sep 17 00:00:00 2001 From: moesnow <11678347+moesnow@users.noreply.github.com> Date: Sat, 24 Oct 2026 03:20:58 +0800 Subject: [PATCH] fix(mcp): secure live read-only queries and retain tool hints --- packages/mcp-server/README.md | 4 +- packages/mcp-server/src/legacy-schema.test.ts | 68 +++++++- packages/mcp-server/src/live-safety.test.ts | 163 ++++++++++++++++++ packages/mcp-server/src/server.test.ts | 32 +++- packages/mcp-server/src/server.ts | 46 ++--- .../mcp-server/src/session-lifecycle.test.ts | 106 ++++++++++++ packages/mcp-server/src/sql-guard.ts | 27 +++ .../node-runtime/src/database-manager.test.ts | 113 +++++++++++- packages/node-runtime/src/database-manager.ts | 128 ++++++++++---- .../tools/src/definitions/sessions.test.ts | 29 ++++ packages/tools/src/definitions/sessions.ts | 16 +- 11 files changed, 668 insertions(+), 64 deletions(-) create mode 100644 packages/mcp-server/src/live-safety.test.ts create mode 100644 packages/mcp-server/src/session-lifecycle.test.ts create mode 100644 packages/mcp-server/src/sql-guard.ts create mode 100644 packages/tools/src/definitions/sessions.test.ts diff --git a/packages/mcp-server/README.md b/packages/mcp-server/README.md index d23d096c..28bb8ce1 100644 --- a/packages/mcp-server/README.md +++ b/packages/mcp-server/README.md @@ -51,7 +51,9 @@ If you install the package globally, you can also use: If you configured a custom ChatLab data directory in `~/.chatlab/config.toml`, the MCP server will use that configuration automatically. -The server is read-only. It opens existing ChatLab session databases and does not modify chat data. +MCP tools use native read-only SQLite connections and short read transactions. They do not modify logical chat records, database schema, or the data-directory compatibility marker, and they never automatically migrate sessions. A session with an older schema must first be upgraded explicitly by a writable ChatLab runtime. + +**Live SQLite caveat:** WAL-mode reads may create/update `-wal` or `-shm` coordination files beside the database. This mode does **not** guarantee an unchanged filesystem directory, nor universal support for read-only mounts or databases requiring recovery. MCP query tools declare `readOnlyHint: true` and `destructiveHint: false` because they do not intentionally change _logical chat data_, perform migrations, or update compatibility metadata. Whether SQLite's auxiliary WAL/SHM coordination is compatible with the MCP annotation's broader "does not modify its environment" wording is a product/contract question for the upstream maintainers; the annotation is not a filesystem-sandbox guarantee. Queries do not create full temporary copies of chat databases. ## Available Capabilities diff --git a/packages/mcp-server/src/legacy-schema.test.ts b/packages/mcp-server/src/legacy-schema.test.ts index 2730769e..29f512ba 100644 --- a/packages/mcp-server/src/legacy-schema.test.ts +++ b/packages/mcp-server/src/legacy-schema.test.ts @@ -6,7 +6,7 @@ import test from 'node:test' import { Client } from '@modelcontextprotocol/sdk/client/index.js' import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js' import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js' -import { CURRENT_SCHEMA_VERSION } from '@openchatlab/core' +import { CHAT_DB_TABLES, CURRENT_SCHEMA_VERSION } from '@openchatlab/core' import { DatabaseManager, NodePathProvider, readDataDirCompatibilityMeta } from '@openchatlab/node-runtime' import { startMcpServer } from './server' import { openTestSqliteDatabase } from '../../../tests/helpers/sqlite.mts' @@ -218,3 +218,69 @@ test('MCP tools and resources refuse invalid SQLite schema versions before SQL d await client.close() } }) + +test('real MCP tools and resources read checkpointed WAL without logical data/marker mutation', async (t) => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'chatlab-mcp-wal-live-')) + t.after(() => fs.rmSync(root, { recursive: true, force: true })) + const dataDir = path.join(root, 'data') + const dbDir = path.join(dataDir, 'databases') + fs.mkdirSync(dbDir, { recursive: true }) + const dbPath = path.join(dbDir, 'wal.db') + const fixture = openTestSqliteDatabase(dbPath) + try { + fixture.pragma('journal_mode = WAL') + fixture.exec(CHAT_DB_TABLES) + fixture.exec(` + INSERT INTO meta (name, platform, type, imported_at) VALUES ('WAL live', 'qq', 'group', 1000); + INSERT INTO member (platform_id, account_name) VALUES ('alice', 'Alice'); + INSERT INTO message (sender_id, ts, type, content) VALUES (1, 1000, 0, 'WAL query works'); + `) + } finally { + fixture.close() + } + const dbManager = new DatabaseManager(new NodePathProvider(dataDir), { + runtime: { version: '0.37.6', kind: 'mcp' }, + }) + t.after(() => dbManager.closeAll()) + dbManager.raiseCurrentChatDbCompatibilityGate() + const original = fs.readFileSync(dbPath) + const metaPath = path.join(dataDir, '.chatlab-meta.json') + const marker = fs.readFileSync(metaPath) + + const client = new Client({ name: 'wal-live-test', version: '1.0.0' }) + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair() + const connect = McpServer.prototype.connect + t.mock.method(McpServer.prototype, 'connect', function (this: McpServer) { + t.after(() => this.close()) + return connect.call(this, serverTransport) + }) + await startMcpServer({ version: '0.37.6', dbManager }) + await client.connect(clientTransport) + try { + for (const [name, args] of [ + ['chatlab_list_sessions', {}], + ['chatlab_get_session_info', { session_id: 'wal' }], + ['chatlab_get_recent_messages', { session_id: 'wal' }], + ['chatlab_execute_sql', { session_id: 'wal', sql: 'SELECT content FROM message' }], + ] as const) { + const result = await client.callTool({ name, arguments: { ...args, format: 'json' } }) + assert.equal(result.isError, undefined, `${name}: ${JSON.stringify(result.content)}`) + } + for (const uri of ['chatlab://sessions', 'chatlab://sessions/wal/meta', 'chatlab://sessions/wal/schema']) { + const resource = await client.readResource({ uri }) + assert.ok(resource.contents.length > 0, uri) + } + // Native readonly WAL readers may create coordination sidecars, but may + // not modify logical chat records, DB schema or compatibility metadata. + assert.deepEqual(fs.readFileSync(dbPath), original) + assert.deepEqual(fs.readFileSync(metaPath), marker) + const readonlyDb = openTestSqliteDatabase(dbPath, { readonly: true }) + try { + assert.deepEqual(readonlyDb.prepare('SELECT content FROM message').all(), [{ content: 'WAL query works' }]) + } finally { + readonlyDb.close() + } + } finally { + await client.close() + } +}) diff --git a/packages/mcp-server/src/live-safety.test.ts b/packages/mcp-server/src/live-safety.test.ts new file mode 100644 index 00000000..a4c84a02 --- /dev/null +++ b/packages/mcp-server/src/live-safety.test.ts @@ -0,0 +1,163 @@ +import assert from 'node:assert/strict' +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' +import test from 'node:test' +import Database from 'better-sqlite3' +import { Client } from '@modelcontextprotocol/sdk/client/index.js' +import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js' +import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js' +import { CHAT_DB_TABLES } from '@openchatlab/core' +import { DatabaseManager, NodePathProvider } from '@openchatlab/node-runtime' +import { startMcpServer } from './server' + +/** Full MCP protocol tests for the two trust boundaries identified in PR review. */ +test('MCP confines session IDs to real ChatLab databases and rejects SQL prepare side effects', async (t) => { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'chatlab-mcp-safety-')) + t.after(() => fs.rmSync(root, { recursive: true, force: true })) + const dataDir = path.join(root, 'data') + const dbDir = path.join(dataDir, 'databases') + fs.mkdirSync(dbDir, { recursive: true }) + const dbPath = path.join(dbDir, 'valid.db') + const db = new Database(dbPath) + try { + db.pragma('journal_mode=WAL') + db.exec(CHAT_DB_TABLES) + db.exec(` + INSERT INTO meta(name, platform, type, imported_at) + VALUES('Valid chat', 'qq', 'group', 1000); + INSERT INTO member(platform_id, account_name) VALUES('p1', 'Alice'); + INSERT INTO message(sender_id, ts, type, content) VALUES(1, 1000, 0, 'allowed chat content'); + `) + } finally { + db.close() + } + + const privateDbPath = path.join(dataDir, 'private.db') + const privateDb = new Database(privateDbPath) + try { + privateDb.exec("CREATE TABLE secrets(value TEXT); INSERT INTO secrets VALUES('SYNTHETIC_OUTSIDE_SESSION_SECRET');") + } finally { + privateDb.close() + } + const foreignDbPath = path.join(dbDir, 'foreign.db') + const foreignDb = new Database(foreignDbPath) + try { + foreignDb.exec("CREATE TABLE secrets(value TEXT); INSERT INTO secrets VALUES('NON_CHATLAB_SECRET');") + } finally { + foreignDb.close() + } + + const manager = new DatabaseManager(new NodePathProvider(dataDir), { + runtime: { version: '0.37.6', kind: 'mcp' }, + }) + manager.raiseCurrentChatDbCompatibilityGate() + t.after(() => manager.closeAll()) + assert.deepEqual(manager.listSessionIdsMcpReadonly(), ['valid']) + // File symlinks require elevated privilege on some Windows test runners. + // Exercise the symlink boundary on POSIX; Windows-specific path checks still run. + if (process.platform !== 'win32') fs.symlinkSync(privateDbPath, path.join(dbDir, 'linked.db')) + const markerPath = path.join(dataDir, '.chatlab-meta.json') + const beforeMarker = fs.readFileSync(markerPath) + const beforeDb = fs.readFileSync(dbPath) + + const client = new Client({ name: 'mcp-safety-client', version: '1.0.0' }) + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair() + const connect = McpServer.prototype.connect + t.mock.method(McpServer.prototype, 'connect', function (this: McpServer) { + t.after(() => this.close()) + return connect.call(this, serverTransport) + }) + await startMcpServer({ version: '0.37.6', dbManager: manager }) + await client.connect(clientTransport) + try { + for (const sessionId of [ + '../private', + '..\\private', + 'C:\\outside', + '/tmp/outside', + 'name:stream', + 'bad\u0000id', + '.', + '..', + 'linked', + 'foreign', + ]) { + const result = await client.callTool({ + name: 'chatlab_execute_sql', + arguments: { session_id: sessionId, sql: 'SELECT * FROM secrets', format: 'json' }, + }) + assert.equal(result.isError, true, `${JSON.stringify(sessionId)} escaped session boundary`) + assert.doesNotMatch(JSON.stringify(result.content), /SYNTHETIC_OUTSIDE_SESSION_SECRET|NON_CHATLAB_SECRET/) + } + for (const uri of [ + 'chatlab://sessions/..%2Fprivate/meta', + 'chatlab://sessions/..%5Cprivate/schema', + 'chatlab://sessions/linked/meta', + 'chatlab://sessions/foreign/schema', + ]) { + let body: string + try { + const result = await client.readResource({ uri }) + body = JSON.stringify(result.contents) + } catch (error) { + body = String(error) + } + assert.doesNotMatch(body, /SYNTHETIC_OUTSIDE_SESSION_SECRET|NON_CHATLAB_SECRET/, uri) + assert.match(body, /Session not found|Invalid MCP session|Not a ChatLab|not found|Invalid Request/i, uri) + } + + const check = async (sql: string) => + client.callTool({ name: 'chatlab_execute_sql', arguments: { session_id: 'valid', sql, format: 'json' } }) + // Query a second SQLite connection to prove that prepare() itself did not + // change the process-global temp_store_directory setting. + const probe = new Database(':memory:') + const beforeTempDirectory = probe.pragma('temp_store_directory') + try { + const pragma = await check(`PRAGMA temp_store_directory='${root.replace(/'/g, "''")}' -- LIMIT`) + assert.equal(pragma.isError, true) + assert.match(JSON.stringify(pragma.content), /Only SELECT or WITH/i) + assert.deepEqual(probe.pragma('temp_store_directory'), beforeTempDirectory) + } finally { + probe.close() + } + for (const sql of [ + 'ATTACH DATABASE ":memory:" AS extra', + 'EXPLAIN PRAGMA temp_store_directory', + 'BEGIN', + '/* comment */ PRAGMA writable_schema=ON', + ]) { + const result = await check(sql) + assert.equal(result.isError, true, sql) + assert.match(JSON.stringify(result.content), /Only SELECT or WITH/i, sql) + } + // WITH is allowed lexically, but SQLite's native readonly flag must + // still reject DML hidden behind the CTE entry point. + const withWrite = await check( + "WITH c(x) AS (SELECT 1) UPDATE message SET content='TAMPERED' WHERE id IN (SELECT x FROM c)" + ) + assert.match(JSON.stringify(withWrite.content), /read-only statements/i) + + const validSql = [ + 'SELECT content FROM message', + '-- comment\n SELECT content FROM message', + '/* leading */\nWITH c(x) AS (SELECT 7) SELECT x FROM c', + ] + for (const sql of validSql) { + const result = await check(sql) + assert.equal(result.isError, undefined, `${sql}: ${JSON.stringify(result.content)}`) + } + assert.deepEqual(fs.readFileSync(dbPath), beforeDb) + assert.deepEqual(fs.readFileSync(markerPath), beforeMarker) + const other = new Database(privateDbPath, { readonly: true }) + try { + assert.deepEqual(other.prepare('SELECT value FROM secrets').all(), [ + { value: 'SYNTHETIC_OUTSIDE_SESSION_SECRET' }, + ]) + } finally { + other.close() + } + } finally { + await client.close() + } +}) diff --git a/packages/mcp-server/src/server.test.ts b/packages/mcp-server/src/server.test.ts index 7255312f..8275f592 100644 --- a/packages/mcp-server/src/server.test.ts +++ b/packages/mcp-server/src/server.test.ts @@ -17,7 +17,7 @@ import { } from '@openchatlab/node-runtime' import { registerTools } from './server' -test('advertises every ChatLab MCP query tool as read-only', async () => { +test('advertises logical ChatLab MCP queries as read-only', async () => { const server = new McpServer({ name: 'chatlab-test', version: '0.0.0' }) const client = new Client({ name: 'chatlab-test-client', version: '0.0.0' }) const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair() @@ -49,6 +49,36 @@ test('advertises every ChatLab MCP query tool as read-only', async () => { } }) +test('rejects unsafe raw MCP SQL before opening a database', async () => { + const server = new McpServer({ name: 'chatlab-sql-precheck', version: '0.0.0' }) + const client = new Client({ name: 'chatlab-sql-precheck-client', version: '0.0.0' }) + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair() + let databaseOpenCalls = 0 + + try { + registerTools(server, { + listSessionIdsMcpReadonly: () => [], + openMcpReadonly: () => { + databaseOpenCalls++ + throw new Error('Unsafe SQL should never open a database') + }, + }) + await server.connect(serverTransport) + await client.connect(clientTransport) + + const result = await client.callTool({ + name: 'chatlab_execute_sql', + arguments: { session_id: 'missing', sql: "PRAGMA temp_store_directory='/tmp'" }, + }) + assert.equal(result.isError, true) + assert.match(JSON.stringify(result.content), /Only SELECT or WITH queries/i) + assert.equal(databaseOpenCalls, 0) + } finally { + await client.close() + await server.close() + } +}) + test('MCP rejects schema v5 before dispatch without migrating or raising the compatibility gate', async (t) => { const root = fs.mkdtempSync(path.join(os.tmpdir(), 'chatlab-mcp-readonly-')) t.after(() => fs.rmSync(root, { recursive: true, force: true })) diff --git a/packages/mcp-server/src/server.ts b/packages/mcp-server/src/server.ts index c1f46ad5..68255835 100644 --- a/packages/mcp-server/src/server.ts +++ b/packages/mcp-server/src/server.ts @@ -9,15 +9,19 @@ import { McpServer, ResourceTemplate } from '@modelcontextprotocol/sdk/server/mc import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js' import { z } from 'zod' import { getSessionMeta, getSessionOverview, getDatabaseSchema } from '@openchatlab/core' -import type { DatabaseAdapter } from '@openchatlab/core' import { MCP_TOOL_REGISTRY, CoreDataProvider, getLocalizedToolMetadata } from '@openchatlab/tools' import type { SessionListContext } from '@openchatlab/tools/src/definitions/sessions' import type { McpDatabaseManager, McpServerOptions } from './types' import { jsonSchemaToZod } from './schema' import { formatToolResultAsText } from './format' +import { assertMcpSqlReadOnly } from './sql-guard' const MCP_TOOL_PREFIX = 'chatlab_' +// These tools only read logical chat data, never migrate sessions or modify +// the compatibility marker. SQLite's ordinary WAL/SHM coordination can still +// change auxiliary filesystem files; that contract distinction is documented +// explicitly and left for upstream maintainer approval. const READ_ONLY_TOOL_ANNOTATIONS = { readOnlyHint: true, destructiveHint: false, @@ -53,28 +57,19 @@ export function registerTools(server: McpServer, dbManager: McpDatabaseManager, { description, inputSchema: zodShape, annotations: READ_ONLY_TOOL_ANNOTATIONS }, async (params) => { const format = params.format as string | undefined - // The shared list handler opens multiple databases; close every connection - // after it returns, including when a query throws. - const openedDbs: DatabaseAdapter[] = [] - try { - const context: SessionListContext = { - db: null as any, - sessionId: '', - listSessionIds: () => dbManager.listSessionIdsMcpReadonly(), - openDb: (id) => { - const db = dbManager.openMcpReadonly(id) - if (db) openedDbs.push(db) - return db - }, - } - const toolParams = { ...params } as Record - delete toolParams.format - const result = await tool.handler(toolParams, context) - const text = applyFormat(result.content, format, tool.name) - return { content: [{ type: 'text' as const, text }] } - } finally { - for (const db of openedDbs) db.close() + const context: SessionListContext = { + db: null as any, + sessionId: '', + listSessionIds: () => dbManager.listSessionIdsMcpReadonly(), + openDb: (id) => dbManager.openMcpReadonly(id), + // Close each independent session transaction before opening the next. + closeDb: (db) => db.close(), } + const toolParams = { ...params } as Record + delete toolParams.format + const result = await tool.handler(toolParams, context) + const text = applyFormat(result.content, format, tool.name) + return { content: [{ type: 'text' as const, text }] } } ) continue @@ -92,6 +87,12 @@ export function registerTools(server: McpServer, dbManager: McpDatabaseManager, async (params) => { const sessionId = params.session_id as string const format = params.format as string | undefined + // Reject SQL that can have prepare-time side effects before even opening + // the SQLite database, which might itself create WAL/SHM sidecars. + const rawSql = (params as Record).sql + if ((tool.name === 'execute_sql' || tool.name === 'render_chart') && typeof rawSql === 'string') { + assertMcpSqlReadOnly(rawSql) + } const db = dbManager.openMcpReadonly(sessionId) if (!db) { return { @@ -104,7 +105,6 @@ export function registerTools(server: McpServer, dbManager: McpDatabaseManager, const toolParams = { ...params } as Record delete toolParams.session_id delete toolParams.format - const result = await tool.handler(toolParams, { db, sessionId, dataProvider: new CoreDataProvider(db) }) const text = applyFormat(result.content, format, tool.name) return { content: [{ type: 'text' as const, text }] } diff --git a/packages/mcp-server/src/session-lifecycle.test.ts b/packages/mcp-server/src/session-lifecycle.test.ts new file mode 100644 index 00000000..4cf7a50e --- /dev/null +++ b/packages/mcp-server/src/session-lifecycle.test.ts @@ -0,0 +1,106 @@ +import assert from 'node:assert/strict' +import fs from 'node:fs' +import path from 'node:path' +import test from 'node:test' +import { Client } from '@modelcontextprotocol/sdk/client/index.js' +import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory.js' +import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js' +import { CHAT_DB_TABLES } from '@openchatlab/core' +import type { DatabaseAdapter } from '@openchatlab/core' +import { DatabaseManager, NodePathProvider } from '@openchatlab/node-runtime' +import { startMcpServer } from './server' +import { openTestSqliteDatabase } from '../../../tests/helpers/sqlite.mts' + +test('MCP list_sessions closes each read transaction before opening the next one', async (t) => { + const root = fs.mkdtempSync(path.join(process.cwd(), '.chatlab-mcp-list-lifecycle-')) + t.after(() => fs.rmSync(root, { recursive: true, force: true })) + const dataDir = path.join(root, 'data') + const dbDir = path.join(dataDir, 'databases') + fs.mkdirSync(dbDir, { recursive: true }) + + const firstPath = path.join(dbDir, 'first.db') + const fixture = openTestSqliteDatabase(firstPath) + try { + fixture.pragma('journal_mode = WAL') + fixture.exec(CHAT_DB_TABLES) + fixture.exec(` + INSERT INTO meta (name, platform, type, imported_at) + VALUES ('Session One', 'qq', 'group', 1000); + `) + } finally { + fixture.close() + } + const secondPath = path.join(dbDir, 'second.db') + fs.copyFileSync(firstPath, secondPath) + const second = openTestSqliteDatabase(secondPath) + try { + second.exec("UPDATE meta SET name = 'Session Two'") + } finally { + second.close() + } + + const pathProvider = new NodePathProvider(dataDir) + const manager = new DatabaseManager(pathProvider, { runtime: { version: '0.37.6', kind: 'mcp' } }) + t.after(() => manager.closeAll()) + manager.raiseCurrentChatDbCompatibilityGate() + const markerPath = path.join(dataDir, '.chatlab-meta.json') + const beforeMarker = fs.readFileSync(markerPath) + + let activeConnections = 0 + let peakConnections = 0 + let sessionsOpened = 0 + let sessionsClosed = 0 + const trackedManager = { + listSessionIdsMcpReadonly: () => manager.listSessionIdsMcpReadonly(), + openMcpReadonly(sessionId: string): DatabaseAdapter | null { + const db = manager.openMcpReadonly(sessionId) + if (!db) return null + activeConnections++ + sessionsOpened++ + peakConnections = Math.max(peakConnections, activeConnections) + return { + readonly: db.readonly, + exec: (sql) => db.exec(sql), + prepare: (sql) => db.prepare(sql), + transaction: (fn) => db.transaction(fn), + pragma: (pragma) => db.pragma(pragma), + close() { + db.close() + activeConnections-- + sessionsClosed++ + }, + } + }, + } + + const client = new Client({ name: 'session-list-lifetime-test-client', version: '1.0.0' }) + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair() + const connect = McpServer.prototype.connect + t.mock.method(McpServer.prototype, 'connect', function (this: McpServer) { + t.after(() => this.close()) + return connect.call(this, serverTransport) + }) + + await startMcpServer({ version: '0.37.6', dbManager: trackedManager }) + await client.connect(clientTransport) + try { + const result = await client.callTool({ name: 'chatlab_list_sessions', arguments: { format: 'json' } }) + assert.equal(result.isError, undefined, JSON.stringify(result.content)) + const parsed = JSON.parse((result.content as Array<{ text: string }>)[0].text) + assert.equal(parsed.total, 2) + assert.deepEqual(parsed.sessions.map((s: { id: string }) => s.id).sort(), ['first', 'second']) + assert.equal(peakConnections, 1, 'a second session connection opened before the previous one was closed') + assert.equal(activeConnections, 0) + assert.equal(sessionsOpened, 2) + assert.equal(sessionsClosed, 2) + + const resource = await client.readResource({ uri: 'chatlab://sessions' }) + assert.match(JSON.stringify(resource.contents), /Session One/) + assert.match(JSON.stringify(resource.contents), /Session Two/) + assert.ok(fs.readdirSync(dbDir).includes('first.db')) + assert.ok(fs.readdirSync(dbDir).includes('second.db')) + assert.deepEqual(fs.readFileSync(markerPath), beforeMarker) + } finally { + await client.close() + } +}) diff --git a/packages/mcp-server/src/sql-guard.ts b/packages/mcp-server/src/sql-guard.ts new file mode 100644 index 00000000..ec3bf9bf --- /dev/null +++ b/packages/mcp-server/src/sql-guard.ts @@ -0,0 +1,27 @@ +/** + * MCP-only guard for user-provided SQL. SQLite can change global settings + * during prepare() (e.g. some PRAGMAs), before stmt.readonly can be checked. + * This lexical precheck is deliberately narrow, not a replacement for SQLite's + * native single-statement/read-only verification in the shared SQL executor. + */ +export function assertMcpSqlReadOnly(sql: string): void { + let pos = 0 + while (pos < sql.length) { + while (pos < sql.length && /\s/.test(sql[pos])) pos++ + if (sql.startsWith('--', pos)) { + const end = sql.indexOf('\n', pos + 2) + pos = end < 0 ? sql.length : end + 1 + continue + } + if (sql.startsWith('/*', pos)) { + const end = sql.indexOf('*/', pos + 2) + if (end < 0) throw new Error('Only SELECT or WITH queries are allowed in MCP') + pos = end + 2 + continue + } + break + } + if (!/^(SELECT|WITH)\b/i.test(sql.slice(pos))) { + throw new Error('Only SELECT or WITH queries are allowed in MCP') + } +} diff --git a/packages/node-runtime/src/database-manager.test.ts b/packages/node-runtime/src/database-manager.test.ts index 2a444c7c..4ba7c13d 100644 --- a/packages/node-runtime/src/database-manager.test.ts +++ b/packages/node-runtime/src/database-manager.test.ts @@ -4,7 +4,7 @@ import os from 'node:os' import path from 'node:path' import test from 'node:test' import Database from 'better-sqlite3' -import type { PathProvider } from '@openchatlab/core' +import type { DatabaseAdapter, PathProvider } from '@openchatlab/core' import { CHAT_DB_SCHEMA, CURRENT_SCHEMA_VERSION, getSessionInfo } from '@openchatlab/core' import { DataDirCompatibilityError, readDataDirCompatibilityMeta } from './data-dir-compat' import { DatabaseManager, listDatabaseCandidateIds } from './database-manager' @@ -1477,3 +1477,114 @@ test('MCP schema preflight rejects invalid SQLite version values without affecti } } }) + +test('MCP live WAL reads use one committed SQLite snapshot across multiple SELECTs', () => { + const root = makeTempDir() + const provider = createPathProvider(root) + fs.mkdirSync(provider.getDatabaseDir(), { recursive: true }) + const dbPath = path.join(provider.getDatabaseDir(), 'concurrent.db') + const writer = new Database(dbPath, { nativeBinding }) + const manager = new DatabaseManager(provider, { + nativeBinding, + runtime: { version: '0.37.6', kind: 'mcp' }, + }) + try { + writer.pragma('journal_mode=WAL') + writer.exec(CHAT_DB_SCHEMA) + writer.exec(` + INSERT INTO meta(name, platform, type, imported_at) VALUES('Concurrent', 'qq', 'group', 1000); + INSERT INTO member(platform_id, account_name) VALUES('p1', 'Alice'); + INSERT INTO message(sender_id, ts, type, content) VALUES(1, 1000, 0, 'committed earlier'); + `) + manager.raiseCurrentChatDbCompatibilityGate() + const markerPath = path.join(provider.getUserDataDir(), '.chatlab-meta.json') + const beforeMarker = fs.readFileSync(markerPath) + const count = (db: DatabaseAdapter) => (db.prepare('SELECT count(*) as n FROM message').get() as { n: number }).n + const first = manager.openMcpReadonly('concurrent') + assert.ok(first) + try { + assert.equal(first.readonly, true) + assert.equal(count(first), 1) + writer.exec("INSERT INTO message(sender_id, ts, type, content) VALUES(1, 1001, 0, 'committed later')") + // The version check and both queries must see the same transaction view. + assert.equal(count(first), 1) + const second = manager.openMcpReadonly('concurrent') + assert.ok(second) + try { + assert.equal(count(second), 2) + } finally { + second.close() + } + assert.equal(count(first), 1) + } finally { + first.close() + } + assert.equal((writer.pragma('wal_checkpoint(TRUNCATE)') as Array<{ busy: number }>)[0].busy, 0) + assert.deepEqual(fs.readFileSync(markerPath), beforeMarker) + } finally { + manager.closeAll() + writer.close() + } +}) + +test('MCP does not return uncommitted changes from a rollback-journal writer', () => { + const root = makeTempDir() + const provider = createPathProvider(root) + fs.mkdirSync(provider.getDatabaseDir(), { recursive: true }) + const dbPath = path.join(provider.getDatabaseDir(), 'rollback.db') + const writer = new Database(dbPath, { nativeBinding }) + const manager = new DatabaseManager(provider, { + nativeBinding, + runtime: { version: '0.37.6', kind: 'mcp' }, + }) + try { + writer.exec(CHAT_DB_SCHEMA) + writer.exec(` + INSERT INTO meta(name, platform, type, imported_at) VALUES('Rollback', 'qq', 'group', 1000); + INSERT INTO member(platform_id, account_name) VALUES('p1', 'Alice'); + `) + writer.pragma('journal_mode=DELETE') + writer.pragma('cache_size=10') + writer.pragma('cache_spill=ON') + const insert = writer.prepare('INSERT INTO message(sender_id, ts, type, content) VALUES(1, ?, 0, ?)') + writer.transaction(() => { + for (let i = 0; i < 90; i++) insert.run(i, 'committed '.padEnd(1600, 'a')) + })() + manager.raiseCurrentChatDbCompatibilityGate() + writer.exec('BEGIN IMMEDIATE') + try { + writer.exec("UPDATE message SET content = 'UNCOMMITTED ' || content") + assert.equal(fs.existsSync(dbPath + '-journal'), true) + try { + const reader = manager.openMcpReadonly('rollback') + if (reader) { + try { + const uncommitted = reader + .prepare("SELECT count(*) AS n FROM message WHERE content LIKE 'UNCOMMITTED%'") + .get() as { n: number } + assert.equal(uncommitted.n, 0, 'a live MCP reader must never expose uncommitted rollback pages') + } finally { + reader.close() + } + } + } catch (error) { + assert.match(String(error), /BUSY|locked/i, 'only a SQLite lock error is acceptable') + } + } finally { + writer.exec('ROLLBACK') + } + const clean = manager.openMcpReadonly('rollback') + assert.ok(clean) + try { + assert.equal( + (clean.prepare("SELECT count(*) AS n FROM message WHERE content LIKE 'UNCOMMITTED%'").get() as { n: number }).n, + 0 + ) + } finally { + clean.close() + } + } finally { + manager.closeAll() + writer.close() + } +}) diff --git a/packages/node-runtime/src/database-manager.ts b/packages/node-runtime/src/database-manager.ts index 99e923ec..85c57a2c 100644 --- a/packages/node-runtime/src/database-manager.ts +++ b/packages/node-runtime/src/database-manager.ts @@ -64,6 +64,14 @@ export function listDatabaseCandidateIds(databaseDir: string): string[] { .map((fileName) => fileName.slice(0, -3)) } +/** A non-ChatLab SQLite file may be skipped during discovery, not opened directly. */ +class NotChatLabSessionError extends Error { + constructor(sessionId: string) { + super(`Not a ChatLab session database: ${sessionId}`) + this.name = 'NotChatLabSessionError' + } +} + export class DatabaseManager { private cache = new Map() private nativeBinding?: string @@ -144,41 +152,89 @@ export class DatabaseManager { } /** - * MCP-only strict read access. Never migrate or repair data. Older schema - * versions require an explicit upgrade through a writable ChatLab runtime. + * Validate MCP session paths without tightening the shared CLI/desktop APIs. + * Reject out-of-scope files, directory traversal, symlinks and non-files. + */ + private resolveMcpSessionPath(sessionId: string): string | null { + if ( + !sessionId || + sessionId === '.' || + sessionId === '..' || + /[\\/:]/.test(sessionId) || + [...sessionId].some((character) => character.charCodeAt(0) < 32) || + /[. ]$/.test(sessionId) || + path.isAbsolute(sessionId) || + path.win32.isAbsolute(sessionId) + ) { + throw new Error(`Invalid MCP session ID: ${JSON.stringify(sessionId)}`) + } + + const databaseDir = this.pathProvider.getDatabaseDir() + const dbPath = path.join(databaseDir, `${sessionId}.db`) + let entry: fs.Stats + try { + entry = fs.lstatSync(dbPath) + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return null + throw error + } + if (!entry.isFile() || entry.isSymbolicLink()) { + throw new Error(`Invalid MCP session database path: ${sessionId}`) + } + + // A configured database root may itself be a symlink. Resolve both sides + // and verify the real target is the expected direct child of that root. + const root = fs.realpathSync(databaseDir) + const actual = fs.realpathSync(dbPath) + if (path.relative(root, actual) !== `${sessionId}.db`) { + throw new Error(`MCP session database is outside the configured directory: ${sessionId}`) + } + return dbPath + } + + /** + * MCP-only live SQLite read. No migrations or marker updates, and no + * full-file snapshot: the native read transaction isolates committed data. + * Normal WAL coordination may create/update -wal or -shm sidecars. */ openMcpReadonly(sessionId: string): DatabaseAdapter | null { - const db = this.openReadonly(sessionId) - if (!db) return null + this.assertCompatible() + const dbPath = this.resolveMcpSessionPath(sessionId) + if (!dbPath) return null + const db = openBetterSqliteDatabase(dbPath, { readonly: true, nativeBinding: this.nativeBinding }) try { - if (isChatSessionDb(db)) { - const schemaVersion = getSchemaVersion(db) - // SQLite INTEGER-affinity columns may actually contain TEXT, REAL or - // BLOB values. A type assertion cannot protect numeric comparisons - // from coercing invalid values (e.g. nonnumeric text to NaN). - if (!Number.isSafeInteger(schemaVersion) || schemaVersion < 0) { - throw new Error( - `ChatLab session ${sessionId} has an invalid schema version. ` + - 'Cannot safely serve this database through read-only MCP tools.' - ) - } - // The runtime cannot understand a future schema or safely repair its - // compatibility metadata; report the version mismatch before the gate. - if (schemaVersion > CURRENT_SCHEMA_VERSION) { - throw new Error( - `ChatLab session ${sessionId} uses schema v${schemaVersion}, but read-only MCP tools require ` + - `schema v${CURRENT_SCHEMA_VERSION}. Upgrade the ChatLab runtime to one that supports this newer schema.` - ) - } + // BEGIN is deferred; the first schema query fixes the SQLite read view. + // Closing this one-request connection releases the read transaction. + db.exec('BEGIN') + if (!isChatSessionDb(db)) { + throw new NotChatLabSessionError(sessionId) + } + const schemaVersion = getSchemaVersion(db) + // SQLite INTEGER-affinity columns may actually contain TEXT, REAL or + // BLOB values. A type assertion cannot protect numeric comparisons + // from coercing invalid values (e.g. nonnumeric text to NaN). + if (!Number.isSafeInteger(schemaVersion) || schemaVersion < 0) { + throw new Error( + `ChatLab session ${sessionId} has an invalid schema version. ` + + 'Cannot safely serve this database through read-only MCP tools.' + ) + } + // The runtime cannot understand a future schema or safely repair its + // compatibility metadata; report the version mismatch before the gate. + if (schemaVersion > CURRENT_SCHEMA_VERSION) { + throw new Error( + `ChatLab session ${sessionId} uses schema v${schemaVersion}, but read-only MCP tools require ` + + `schema v${CURRENT_SCHEMA_VERSION}. Upgrade the ChatLab runtime to one that supports this newer schema.` + ) + } - this.assertReadonlyCompatibilityGate(schemaVersion) - if (schemaVersion < CURRENT_SCHEMA_VERSION) { - throw new Error( - `ChatLab session ${sessionId} uses schema v${schemaVersion}, but read-only MCP tools require ` + - `schema v${CURRENT_SCHEMA_VERSION}. Open the session with the current writable ChatLab runtime to migrate it first.` - ) - } + this.assertReadonlyCompatibilityGate(schemaVersion) + if (schemaVersion < CURRENT_SCHEMA_VERSION) { + throw new Error( + `ChatLab session ${sessionId} uses schema v${schemaVersion}, but read-only MCP tools require ` + + `schema v${CURRENT_SCHEMA_VERSION}. Open the session with the current writable ChatLab runtime to migrate it first.` + ) } return db } catch (error) { @@ -287,10 +343,18 @@ export class DatabaseManager { this.assertCompatible() return listDatabaseCandidateIds(this.pathProvider.getDatabaseDir()).filter((sessionId) => { - const db = this.openMcpReadonly(sessionId) + let db: DatabaseAdapter | null + try { + db = this.openMcpReadonly(sessionId) + } catch (error) { + // Discovery may ignore proven non-ChatLab databases, but must not + // silently swallow damaged databases or schema/gate/permission errors. + if (error instanceof NotChatLabSessionError) return false + throw error + } if (!db) return false try { - return isChatSessionDb(db) + return true // openMcpReadonly has already validated its type and schema. } finally { db.close() } diff --git a/packages/tools/src/definitions/sessions.test.ts b/packages/tools/src/definitions/sessions.test.ts new file mode 100644 index 00000000..c7b261a2 --- /dev/null +++ b/packages/tools/src/definitions/sessions.test.ts @@ -0,0 +1,29 @@ +import assert from 'node:assert/strict' +import test from 'node:test' +import type { DatabaseAdapter } from '@openchatlab/core' +import { sessionsListTool, type SessionListContext } from './sessions' + +test('list_sessions closes each per-session database even when metadata query throws', () => { + let closeCount = 0 + const fakeDb: DatabaseAdapter = { + readonly: true, + prepare: () => { + throw new Error('synthetic database query failure') + }, + pragma: () => [], + exec: () => undefined, + transaction: (fn) => fn(), + close: () => { + closeCount++ + }, + } + const context: SessionListContext = { + db: fakeDb, + sessionId: '', + listSessionIds: () => ['one'], + openDb: () => fakeDb, + closeDb: (db) => db.close(), + } + assert.throws(() => sessionsListTool.handler({}, context), /synthetic database query failure/) + assert.equal(closeCount, 1) +}) diff --git a/packages/tools/src/definitions/sessions.ts b/packages/tools/src/definitions/sessions.ts index a17e58f1..27ab201e 100644 --- a/packages/tools/src/definitions/sessions.ts +++ b/packages/tools/src/definitions/sessions.ts @@ -22,6 +22,8 @@ const inputSchema: JsonSchema = { export interface SessionListContext extends ToolExecutionContext { listSessionIds: () => string[] openDb: (sessionId: string) => DatabaseAdapter | null + /** Optional lifecycle hook for uncached per-session connections (e.g. MCP read transactions). */ + closeDb?: (db: DatabaseAdapter) => void } function handler(_params: Record, context: SessionListContext): ToolResult { @@ -32,11 +34,15 @@ function handler(_params: Record, context: SessionListContext): .map((id) => { const db = context.openDb(id) if (!db) return null - const meta = getSessionMeta(db) - if (!meta) return null - if (keyword && !meta.name.toLowerCase().includes(keyword)) return null - const overview = getSessionOverview(db) - return { id, ...meta, ...overview } + try { + const meta = getSessionMeta(db) + if (!meta) return null + if (keyword && !meta.name.toLowerCase().includes(keyword)) return null + const overview = getSessionOverview(db) + return { id, ...meta, ...overview } + } finally { + context.closeDb?.(db) + } }) .filter(Boolean)