diff --git a/eslint-rules/no-config-imports.js b/eslint-rules/no-config-imports.js new file mode 100644 index 000000000..908dd5ae7 --- /dev/null +++ b/eslint-rules/no-config-imports.js @@ -0,0 +1,71 @@ +"use strict"; +import path from "path"; + +// The file from which we wish to discourage importing values +const configFilePath = path.resolve(import.meta.dirname, "../src/common/config.js"); + +// Files that are allowed to import value exports from config.ts +const allowedConfigValueImportFiles = [ + // Main entry point that injects the config + "src/index.ts", + // Config resource definition that works with the some config values + "src/resources/common/config.ts", +]; + +// Ref: https://eslint.org/docs/latest/extend/custom-rules +export default { + meta: { + type: "problem", + docs: { + description: + "Disallows value imports from config.ts, with a few exceptions, to enforce dependency injection of the config.", + recommended: true, + }, + fixable: null, + messages: { + noConfigImports: + "Value imports from config.ts are not allowed. Use dependency injection instead. Only type imports are permitted.", + }, + }, + create(context) { + const currentFilePath = path.resolve(context.getFilename()); + + const isCurrentFileAllowedToImport = allowedConfigValueImportFiles.some((allowedFile) => { + const resolvedAllowedFile = path.resolve(allowedFile); + return currentFilePath === resolvedAllowedFile; + }); + + if (isCurrentFileAllowedToImport) { + return {}; + } + + return { + ImportDeclaration(node) { + const importPath = node.source.value; + + // If the path is not relative, very likely its targeting a + // node_module so we skip it. And also if the entire import is + // marked with a type keyword. + if (typeof importPath !== "string" || !importPath.startsWith(".") || node.importKind === "type") { + return; + } + + const currentDir = path.dirname(currentFilePath); + const resolvedImportPath = path.resolve(currentDir, importPath); + + if (resolvedImportPath === configFilePath) { + const hasValueImportFromConfig = node.specifiers.some((specifier) => { + return specifier.importKind !== "type"; + }); + + if (hasValueImportFromConfig) { + context.report({ + node, + messageId: "noConfigImports", + }); + } + } + }, + }; + }, +}; diff --git a/eslint-rules/no-config-imports.test.js b/eslint-rules/no-config-imports.test.js new file mode 100644 index 000000000..d94e952db --- /dev/null +++ b/eslint-rules/no-config-imports.test.js @@ -0,0 +1,69 @@ +import path from "path"; +import { RuleTester } from "eslint"; +import { describe, it } from "vitest"; +import tsParser from "@typescript-eslint/parser"; +import rule from "./no-config-imports.js"; + +const ROOT = process.cwd(); +const resolve = (p) => path.resolve(ROOT, p); + +const ruleTester = new RuleTester({ + languageOptions: { + parser: tsParser, + parserOptions: { ecmaVersion: 2022, sourceType: "module" }, + }, +}); + +describe("no-config-imports", () => { + it("should not report any violations", () => { + ruleTester.run("no-config-imports", rule, { + valid: [ + { + filename: resolve("src/some/module.ts"), + code: 'import type { UserConfig } from "../common/config.js";\n', + }, + { + filename: resolve("src/some/module.ts"), + code: 'import { something } from "../common/logger.js";\n', + }, + { + filename: resolve("src/some/module.ts"), + code: 'import type * as Cfg from "../common/config.js";\n', + }, + { + filename: resolve("src/index.ts"), + code: 'import { driverOptions } from "../common/config.js";\n', + }, + ], + invalid: [], + }); + }); + + it("should report rule violations", () => { + ruleTester.run("no-config-imports", rule, { + valid: [], + invalid: [ + { + filename: resolve("src/another/module.ts"), + code: 'import { driverOptions } from "../common/config.js";\n', + errors: [{ messageId: "noConfigImports" }], + }, + { + filename: resolve("src/another/module.ts"), + code: 'import configDefault from "../common/config.js";\n', + errors: [{ messageId: "noConfigImports" }], + }, + { + filename: resolve("src/another/module.ts"), + code: 'import * as cfg from "../common/config.js";\n', + errors: [{ messageId: "noConfigImports" }], + }, + { + filename: resolve("src/another/module.ts"), + code: 'import { type UserConfig, driverOptions } from "../common/config.js";\n', + errors: [{ messageId: "noConfigImports" }], + }, + ], + }); + }); +}); diff --git a/eslint.config.js b/eslint.config.js index f6755d129..ce643a09c 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -1,9 +1,11 @@ +import path from "path"; import { defineConfig, globalIgnores } from "eslint/config"; import js from "@eslint/js"; import globals from "globals"; import tseslint from "typescript-eslint"; import eslintPluginPrettierRecommended from "eslint-plugin-prettier/recommended"; import vitestPlugin from "@vitest/eslint-plugin"; +import noConfigImports from "./eslint-rules/no-config-imports.js"; const testFiles = ["tests/**/*.test.ts", "tests/**/*.ts"]; @@ -62,6 +64,19 @@ export default defineConfig([ "@typescript-eslint/explicit-function-return-type": "error", }, }, + { + files: ["src/**/*.ts"], + plugins: { + "no-config-imports": { + rules: { + "no-config-imports": noConfigImports, + }, + }, + }, + rules: { + "no-config-imports/no-config-imports": "error", + }, + }, globalIgnores([ "node_modules", "dist", @@ -72,6 +87,7 @@ export default defineConfig([ "vitest.config.ts", "src/types/*.d.ts", "tests/integration/fixtures/", + "eslint-rules", ]), eslintPluginPrettierRecommended, ]); diff --git a/package.json b/package.json index c5927d242..e943a8277 100644 --- a/package.json +++ b/package.json @@ -52,7 +52,7 @@ "fix:lint": "eslint . --fix", "reformat": "prettier --write .", "generate": "./scripts/generate.sh", - "test": "vitest --project unit-and-integration --coverage", + "test": "vitest --project eslint-rules --project unit-and-integration --coverage", "pretest:accuracy": "npm run build", "test:accuracy": "sh ./scripts/accuracy/runAccuracyTests.sh" }, diff --git a/src/index.ts b/src/index.ts index 29f525dc3..31f393461 100644 --- a/src/index.ts +++ b/src/index.ts @@ -36,7 +36,7 @@ function enableFipsIfRequested(): void { enableFipsIfRequested(); import { ConsoleLogger, LogId } from "./common/logger.js"; -import { config } from "./common/config.js"; +import { config, driverOptions } from "./common/config.js"; import crypto from "crypto"; import { packageInfo } from "./common/packageInfo.js"; import { StdioRunner } from "./transports/stdio.js"; @@ -49,7 +49,10 @@ async function main(): Promise { assertHelpMode(); assertVersionMode(); - const transportRunner = config.transport === "stdio" ? new StdioRunner(config) : new StreamableHttpRunner(config); + const transportRunner = + config.transport === "stdio" + ? new StdioRunner(config, driverOptions) + : new StreamableHttpRunner(config, driverOptions); const shutdown = (): void => { transportRunner.logger.info({ id: LogId.serverCloseRequested, diff --git a/src/transports/base.ts b/src/transports/base.ts index f58fbf2ed..17a0ff5e7 100644 --- a/src/transports/base.ts +++ b/src/transports/base.ts @@ -1,5 +1,4 @@ -import type { UserConfig } from "../common/config.js"; -import { driverOptions } from "../common/config.js"; +import type { DriverOptions, UserConfig } from "../common/config.js"; import { packageInfo } from "../common/packageInfo.js"; import { Server } from "../server.js"; import { Session } from "../common/session.js"; @@ -15,7 +14,10 @@ export abstract class TransportRunnerBase { public logger: LoggerBase; public deviceId: DeviceId; - protected constructor(protected readonly userConfig: UserConfig) { + protected constructor( + protected readonly userConfig: UserConfig, + private readonly driverOptions: DriverOptions + ) { const loggers: LoggerBase[] = []; if (this.userConfig.loggers.includes("stderr")) { loggers.push(new ConsoleLogger()); @@ -35,37 +37,37 @@ export abstract class TransportRunnerBase { this.deviceId = DeviceId.create(this.logger); } - protected setupServer(userConfig: UserConfig): Server { + protected setupServer(): Server { const mcpServer = new McpServer({ name: packageInfo.mcpServerName, version: packageInfo.version, }); const loggers = [this.logger]; - if (userConfig.loggers.includes("mcp")) { + if (this.userConfig.loggers.includes("mcp")) { loggers.push(new McpLogger(mcpServer)); } const logger = new CompositeLogger(...loggers); - const exportsManager = ExportsManager.init(userConfig, logger); - const connectionManager = new ConnectionManager(userConfig, driverOptions, logger, this.deviceId); + const exportsManager = ExportsManager.init(this.userConfig, logger); + const connectionManager = new ConnectionManager(this.userConfig, this.driverOptions, logger, this.deviceId); const session = new Session({ - apiBaseUrl: userConfig.apiBaseUrl, - apiClientId: userConfig.apiClientId, - apiClientSecret: userConfig.apiClientSecret, + apiBaseUrl: this.userConfig.apiBaseUrl, + apiClientId: this.userConfig.apiClientId, + apiClientSecret: this.userConfig.apiClientSecret, logger, exportsManager, connectionManager, }); - const telemetry = Telemetry.create(session, userConfig, this.deviceId); + const telemetry = Telemetry.create(session, this.userConfig, this.deviceId); return new Server({ mcpServer, session, telemetry, - userConfig, + userConfig: this.userConfig, }); } diff --git a/src/transports/stdio.ts b/src/transports/stdio.ts index 3668c4ee8..d0619da6c 100644 --- a/src/transports/stdio.ts +++ b/src/transports/stdio.ts @@ -5,7 +5,7 @@ import type { JSONRPCMessage } from "@modelcontextprotocol/sdk/types.js"; import { JSONRPCMessageSchema } from "@modelcontextprotocol/sdk/types.js"; import { EJSON } from "bson"; import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js"; -import type { UserConfig } from "../common/config.js"; +import type { DriverOptions, UserConfig } from "../common/config.js"; // This is almost a copy of ReadBuffer from @modelcontextprotocol/sdk // but it uses EJSON.parse instead of JSON.parse to handle BSON types @@ -54,13 +54,13 @@ export function createStdioTransport(): StdioServerTransport { export class StdioRunner extends TransportRunnerBase { private server: Server | undefined; - constructor(userConfig: UserConfig) { - super(userConfig); + constructor(userConfig: UserConfig, driverOptions: DriverOptions) { + super(userConfig, driverOptions); } async start(): Promise { try { - this.server = this.setupServer(this.userConfig); + this.server = this.setupServer(); const transport = createStdioTransport(); diff --git a/src/transports/streamableHttp.ts b/src/transports/streamableHttp.ts index b15a201f2..f50019ef4 100644 --- a/src/transports/streamableHttp.ts +++ b/src/transports/streamableHttp.ts @@ -3,7 +3,7 @@ import type http from "http"; import { StreamableHTTPServerTransport } from "@modelcontextprotocol/sdk/server/streamableHttp.js"; import { isInitializeRequest } from "@modelcontextprotocol/sdk/types.js"; import { TransportRunnerBase } from "./base.js"; -import type { UserConfig } from "../common/config.js"; +import type { DriverOptions, UserConfig } from "../common/config.js"; import { LogId } from "../common/logger.js"; import { randomUUID } from "crypto"; import { SessionStore } from "../common/sessionStore.js"; @@ -18,8 +18,8 @@ export class StreamableHttpRunner extends TransportRunnerBase { private httpServer: http.Server | undefined; private sessionStore!: SessionStore; - constructor(userConfig: UserConfig) { - super(userConfig); + constructor(userConfig: UserConfig, driverOptions: DriverOptions) { + super(userConfig, driverOptions); } async start(): Promise { @@ -89,7 +89,7 @@ export class StreamableHttpRunner extends TransportRunnerBase { return; } - const server = this.setupServer(this.userConfig); + const server = this.setupServer(); const transport = new StreamableHTTPServerTransport({ sessionIdGenerator: (): string => randomUUID().toString(), onsessioninitialized: (sessionId): void => { diff --git a/tests/integration/transports/streamableHttp.test.ts b/tests/integration/transports/streamableHttp.test.ts index d5b6e0be7..396879b1b 100644 --- a/tests/integration/transports/streamableHttp.test.ts +++ b/tests/integration/transports/streamableHttp.test.ts @@ -2,7 +2,7 @@ import { StreamableHttpRunner } from "../../../src/transports/streamableHttp.js" import { Client } from "@modelcontextprotocol/sdk/client/index.js"; import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"; import { describe, expect, it, beforeAll, afterAll } from "vitest"; -import { config } from "../../../src/common/config.js"; +import { config, driverOptions } from "../../../src/common/config.js"; describe("StreamableHttpRunner", () => { let runner: StreamableHttpRunner; @@ -14,7 +14,7 @@ describe("StreamableHttpRunner", () => { oldLoggers = config.loggers; config.telemetry = "disabled"; config.loggers = ["stderr"]; - runner = new StreamableHttpRunner(config); + runner = new StreamableHttpRunner(config, driverOptions); await runner.start(); }); diff --git a/vitest.config.ts b/vitest.config.ts index 239650acb..903a174af 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -36,6 +36,13 @@ export default defineConfig({ include: ["**/accuracy/*.test.ts"], }, }, + { + extends: true, + test: { + name: "eslint-rules", + include: ["eslint-rules/*.test.js"], + }, + }, ], }, });