diff --git a/CHANGELOG.md b/CHANGELOG.md index f5c5488..ec93124 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,31 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.2.0-alpha.1] + +### Added + +- **Per-instance data directory.** New `--data-dir ` flag and `SPE_DATA_DIR` + environment variable select where the provisioning `state.json` and MSAL token + cache are stored (precedence: flag > env > default `~/.spe-mcp`). Point each + server instance at a unique directory to run multiple instances (e.g. two + tenants, or a published build alongside a local build) without clobbering + shared state. Applies uniformly to `start`, `auth`, and `logout`. The default + path is unchanged and byte-identical to prior releases. + +### Security + +- **Fail-closed credential/state file handling.** The data directory and token + cache files are now validated fail-closed: a symlinked, foreign-owned, or + group/other-accessible directory is refused (POSIX `0o700`); an off-`%USERPROFILE%` + Windows override is given an owner-only DACL or refused. Reads and writes use + `O_NOFOLLOW` + `fstat` fd verification and `fchmod` the descriptor (never the + path) to defeat symlink/TOCTOU swaps. A caller-supplied `--data-dir` must be an + absolute (or `~/`-relative) path; CWD-relative paths are rejected so credentials + can never be written into a working directory. On an insecure/unverifiable + target, refresh-token persistence is skipped (forcing a fresh interactive + sign-in) rather than writing a token to an unsafe location. + ## [0.1.0] Initial release. diff --git a/README.md b/README.md index a3eec69..309472f 100644 --- a/README.md +++ b/README.md @@ -181,6 +181,7 @@ The server accepts configuration via CLI flags or environment variables: | `--tenant-id` | `SPE_TENANT_ID` | Entra ID Tenant ID | | `--read-only` | `SPE_READ_ONLY` | Advertise/allow only read/list/get/search tools; reject mutating calls | | `--tools` | `SPE_TOOLS` | Restrict exposed tools to a profile (`readOnly`, `docsOnly`, `provisioning`, `content`, `admin`) or a comma-separated tool list | +| `--data-dir` | `SPE_DATA_DIR` | Directory for the token cache + provisioning state (default `~/.spe-mcp`). Point each instance at a unique **absolute** path (or `~/...`; CWD-relative paths are rejected) to run multiple servers without clobbering state | > The CLI flag wins when both a flag and its env var are set. Run > `spe-mcp start --help` to see the authoritative option list and descriptions. @@ -293,7 +294,31 @@ For most developers nothing extra is needed: create the owning app with the `pro ### Token Storage -Tokens are cached under `~/.spe-mcp/` in per-identity files named `token-cache...json` (a legacy `token-cache.json` may also exist). Each file contains MSAL's serialized token cache (refresh tokens, account info). On macOS/Linux the cache directory is created `0700` and the cache files `0600` (owner read/write only); on Windows the files are protected by the per-user profile ACL. +Tokens are cached under the **data directory** (default `~/.spe-mcp/`, or a `--data-dir` / `SPE_DATA_DIR` override) in per-identity files named `token-cache...json` (a legacy `token-cache.json` may also exist). Each file contains MSAL's serialized token cache (refresh tokens, account info). On macOS/Linux the cache directory is created `0700` and the cache files `0600` (owner read/write only), and the server fails closed if the directory is a symlink, owned by another user, or group/other-accessible; on Windows the files are protected by the per-user profile ACL (an off-profile `--data-dir` override is given an owner-only DACL, or refused). + +### Running multiple instances (isolating state) + +The data directory holds a single provisioning `state.json` plus the token cache, so two servers pointed at the **same** directory can clobber each other's state. To run more than one instance (e.g. two tenants, or a published build alongside a local build), give each its own `--data-dir` / `SPE_DATA_DIR`: + +```jsonc +// .vscode/mcp.json — two isolated instances +{ + "servers": { + "spe-tenantA": { + "command": "npx", + "args": ["-y", "@microsoft/spe-mcp-server", "start"], + "env": { "SPE_DATA_DIR": "~/.spe-mcp-tenantA", "SPE_TENANT_ID": "" } + }, + "spe-tenantB": { + "command": "npx", + "args": ["-y", "@microsoft/spe-mcp-server", "start"], + "env": { "SPE_DATA_DIR": "~/.spe-mcp-tenantB", "SPE_TENANT_ID": "" } + } + } +} +``` + +The path must be **absolute** (a leading `~/` is expanded against your home directory); CWD-relative paths are rejected so credentials can never be written into a working directory. The same value must be used for `start`, `auth`, and `logout` of a given instance — set it once via `SPE_DATA_DIR` (as above) and all three commands agree. ### Full Local Auth Reset diff --git a/package.json b/package.json index a7987f6..f58f0f7 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@microsoft/spe-mcp-server", - "version": "0.1.0-alpha.1", + "version": "0.2.0-alpha.1", "description": "SharePoint Embedded MCP Server — manage container types, containers, and content via any MCP client", "keywords": [ "mcp", diff --git a/src/auth.ts b/src/auth.ts index c2148e4..6ab5c70 100644 --- a/src/auth.ts +++ b/src/auth.ts @@ -15,10 +15,10 @@ * prior tenant from interfering on a tenant switch. */ -import { existsSync, readFileSync } from "node:fs"; -import { homedir } from "node:os"; -import { basename, join } from "node:path"; -import { ensureSecureDir, writeSecureFile } from "./secure-fs.js"; +import { existsSync } from "node:fs"; +import { basename } from "node:path"; +import { getCacheDir, getCacheFile, getLegacyCacheFile } from "./paths.js"; +import { ensureSecureDir, readSecureFile, writeSecureFile } from "./secure-fs.js"; import { type AccountInfo, type AuthenticationResult, @@ -271,15 +271,9 @@ function logError(message: string, data?: unknown): void { // different tenants never co-mingle, which is the root cause of the // tenant-switch silent-auth failures and wrong-tenant-token hazard. -const CACHE_DIR = join(homedir(), ".spe-mcp"); -// Legacy single-file cache used before per-tenant partitioning. Kept only so -// that clearCachedToken() can clean it up; it is never read on the hot path. -const LEGACY_CACHE_FILE = join(CACHE_DIR, "token-cache.json"); - -/** Make a value safe to embed in a filename (GUIDs are already safe; be defensive). */ -function sanitizeForFilename(value: string): string { - return value.replace(/[^a-zA-Z0-9._-]/g, "_"); -} +// The token-cache directory + partitioned file paths come from the resolve-once +// seam in paths.ts, so they honor a --data-dir / SPE_DATA_DIR override. Legacy +// single-file cache (pre-partitioning) is only cleaned up by clearCachedToken(). /** * Derive the token-cache file path for a given auth config. Partitioned by @@ -291,11 +285,9 @@ function sanitizeForFilename(value: string): string { */ export function getCacheFilePath(config: AuthConfig | null = authConfig): string { if (config?.tenantId) { - const tenant = sanitizeForFilename(config.tenantId); - const client = config.clientId ? sanitizeForFilename(config.clientId) : "default"; - return join(CACHE_DIR, `token-cache.${tenant}.${client}.json`); + return getCacheFile(config.tenantId, config.clientId ?? "default"); } - return LEGACY_CACHE_FILE; + return getLegacyCacheFile(); } /** @@ -316,8 +308,10 @@ const fileCachePlugin: ICachePlugin = { beforeCacheAccess: async (cacheContext) => { const cacheFile = getCacheFilePath(); try { - if (existsSync(cacheFile)) { - const cached = readFileSync(cacheFile, "utf-8"); + // O_NOFOLLOW + owner check: a planted symlink is refused (throws) rather + // than followed; a missing cache returns null and forces a fresh sign-in. + const cached = readSecureFile(cacheFile); + if (cached !== null) { cacheContext.tokenCache.deserialize(cached); log(`Token cache loaded from file (${basename(cacheFile)})`); } @@ -329,7 +323,7 @@ const fileCachePlugin: ICachePlugin = { if (cacheContext.cacheHasChanged) { const cacheFile = getCacheFilePath(); try { - ensureSecureDir(CACHE_DIR); + ensureSecureDir(getCacheDir()); const serialized = cacheContext.tokenCache.serialize(); // SEC-003: token cache holds refresh tokens — owner-only (0o600). writeSecureFile(cacheFile, serialized); @@ -896,7 +890,7 @@ export async function clearCachedToken(): Promise { try { log("Clearing cached tokens..."); const { unlinkSync } = await import("node:fs"); - const filesToRemove = new Set([getCacheFilePath(), LEGACY_CACHE_FILE]); + const filesToRemove = new Set([getCacheFilePath(), getLegacyCacheFile()]); for (const file of filesToRemove) { try { if (existsSync(file)) { diff --git a/src/cli.ts b/src/cli.ts index 5768cd4..0e3e73d 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -28,6 +28,27 @@ function isTruthyEnv(value: string | undefined): boolean { return ["1", "true", "yes", "on"].includes(value.trim().toLowerCase()); } +/** Shared `--data-dir` option description (start / auth / logout). */ +const DATA_DIR_OPTION = + "Directory for the token cache + provisioning state (default ~/.spe-mcp). " + + "Point each instance at a unique path to run multiple servers without clobbering state. " + + "Must be absolute (or ~/...). Can also be set via SPE_DATA_DIR."; + +/** + * Resolve the data directory from `--data-dir` (falling back to SPE_DATA_DIR), + * record it as the process-wide override BEFORE any state/auth module reads the + * location, propagate it via SPE_DATA_DIR for defense-in-depth, and log the + * resolved path to stderr only (path-only; stdout is the MCP JSON-RPC channel). + * Throws AppError on an invalid (e.g. CWD-relative) path — caught by each + * command's try/catch so the failure is loud, not silent. + */ +async function applyDataDir(dataDir?: string): Promise { + const { setDataDirOverride } = await import("./paths.js"); + const resolved = setDataDirOverride(dataDir || process.env.SPE_DATA_DIR); + process.env.SPE_DATA_DIR = resolved; + console.error(`[SPE MCP Server] Data directory: ${resolved}`); +} + const program = new Command(); program @@ -54,8 +75,12 @@ program "--tools ", "Restrict exposed tools: a built-in profile (readOnly, docsOnly, provisioning, content, admin) or a comma-separated list of tool names. Can also be set via SPE_TOOLS.", ) - .action(async (options: { clientId?: string; tenantId?: string; readOnly?: boolean; tools?: string }) => { + .option("--data-dir ", DATA_DIR_OPTION) + .action(async (options: { clientId?: string; tenantId?: string; readOnly?: boolean; tools?: string; dataDir?: string }) => { try { + // Resolve + record the data dir FIRST, before importing ./index.js (which + // pulls in state.ts/auth.ts) so every entry point resolves the same dir. + await applyDataDir(options.dataDir); const clientId = options.clientId || process.env.SPE_CLIENT_ID; const tenantId = options.tenantId || process.env.SPE_TENANT_ID; // Read-only: CLI flag wins; otherwise a truthy SPE_READ_ONLY env value. @@ -85,8 +110,12 @@ program .option("--client-id ", "Entra ID Application (Client) ID. Can also be set via SPE_CLIENT_ID env var.") .option("--tenant-id ", "Entra ID Tenant ID. Can also be set via SPE_TENANT_ID env var.") .option("--reset", "Clear any cached tokens for this tenant before authenticating (useful when switching tenants).") - .action(async (options: { clientId?: string; tenantId?: string; reset?: boolean }) => { + .option("--data-dir ", DATA_DIR_OPTION) + .action(async (options: { clientId?: string; tenantId?: string; reset?: boolean; dataDir?: string }) => { try { + // Resolve + record the data dir FIRST so auth caches tokens to the SAME + // directory `start` will later read from (else silent "not authenticated"). + await applyDataDir(options.dataDir); const clientId = options.clientId || process.env.SPE_CLIENT_ID; const tenantId = options.tenantId || process.env.SPE_TENANT_ID; @@ -120,8 +149,12 @@ program program .command("logout") .description("Clear cached authentication tokens") - .action(async () => { + .option("--data-dir ", DATA_DIR_OPTION) + .action(async (options: { dataDir?: string }) => { try { + // Resolve + record the data dir FIRST so logout clears tokens from the + // SAME directory the matching `auth`/`start` used. + await applyDataDir(options.dataDir); const { clearCachedToken } = await import("./auth.js"); await clearCachedToken(); console.log("Logged out. Cached tokens have been cleared."); diff --git a/src/paths.test.ts b/src/paths.test.ts new file mode 100644 index 0000000..91d5e7d --- /dev/null +++ b/src/paths.test.ts @@ -0,0 +1,134 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { mkdtempSync, rmSync } from "node:fs"; +import { homedir, tmpdir } from "node:os"; +import { join, normalize } from "node:path"; +import { + resolveDataDir, + setDataDirOverride, + getDataDir, + getStateFile, + getCacheDir, + getCacheFile, + getLegacyCacheFile, + sanitizeForFilename, + __testing, +} from "./paths.js"; + +const DEFAULT = normalize(join(homedir(), ".spe-mcp")); + +describe("paths — data directory resolution", () => { + const savedEnv = process.env.SPE_DATA_DIR; + + beforeEach(() => { + // Each test starts from a clean slate: no memoized dir, no env override. + __testing.reset(); + delete process.env.SPE_DATA_DIR; + }); + + afterEach(() => { + __testing.reset(); + if (savedEnv === undefined) delete process.env.SPE_DATA_DIR; + else process.env.SPE_DATA_DIR = savedEnv; + }); + + it("defaults to ~/.spe-mcp (byte-identical to the legacy hardcoded path)", () => { + expect(getDataDir()).toBe(DEFAULT); + expect(getStateFile()).toBe(join(DEFAULT, "state.json")); + expect(getCacheDir()).toBe(DEFAULT); + }); + + it("resolveDataDir(undefined/empty/whitespace) returns the default", () => { + expect(resolveDataDir()).toBe(DEFAULT); + expect(resolveDataDir("")).toBe(DEFAULT); + expect(resolveDataDir(" ")).toBe(DEFAULT); + }); + + it("honors an explicit override (flag) via setDataDirOverride", () => { + const dir = mkdtempSync(join(tmpdir(), "spe-mcp-paths-A-")); + try { + const resolved = setDataDirOverride(dir); + expect(resolved).toBe(normalize(dir)); + expect(getDataDir()).toBe(normalize(dir)); + expect(getStateFile()).toBe(join(normalize(dir), "state.json")); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("honors SPE_DATA_DIR env when no explicit override is set", () => { + const dir = mkdtempSync(join(tmpdir(), "spe-mcp-paths-E-")); + try { + process.env.SPE_DATA_DIR = dir; + expect(getDataDir()).toBe(normalize(dir)); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("gives the explicit override precedence over the env var (flag > env)", () => { + const envDir = mkdtempSync(join(tmpdir(), "spe-mcp-paths-env-")); + const flagDir = mkdtempSync(join(tmpdir(), "spe-mcp-paths-flag-")); + try { + process.env.SPE_DATA_DIR = envDir; + // Mirror the CLI's `options.dataDir || process.env.SPE_DATA_DIR`. + setDataDirOverride(flagDir || process.env.SPE_DATA_DIR); + expect(getDataDir()).toBe(normalize(flagDir)); + } finally { + rmSync(envDir, { recursive: true, force: true }); + rmSync(flagDir, { recursive: true, force: true }); + } + }); + + it("expands a leading ~ against the home directory", () => { + expect(resolveDataDir("~")).toBe(normalize(homedir())); + expect(resolveDataDir("~/spe-alt")).toBe(normalize(join(homedir(), "spe-alt"))); + }); + + it("rejects a CWD-relative path so secrets never land in the working directory", () => { + expect(() => resolveDataDir("relative/dir")).toThrow(/absolute/i); + expect(() => resolveDataDir("./foo")).toThrow(/absolute/i); + expect(() => resolveDataDir("../foo")).toThrow(/absolute/i); + }); + + it("re-resolves lazily after a later override (no import-time freeze)", () => { + const a = mkdtempSync(join(tmpdir(), "spe-mcp-paths-lazy-a-")); + const b = mkdtempSync(join(tmpdir(), "spe-mcp-paths-lazy-b-")); + try { + setDataDirOverride(a); + expect(getDataDir()).toBe(normalize(a)); + // A subsequent override wins and getters re-resolve to it. + setDataDirOverride(b); + expect(getDataDir()).toBe(normalize(b)); + } finally { + rmSync(a, { recursive: true, force: true }); + rmSync(b, { recursive: true, force: true }); + } + }); + + it("partitions the token cache by tenant and client within the data dir", () => { + const dir = mkdtempSync(join(tmpdir(), "spe-mcp-paths-cache-")); + try { + setDataDirOverride(dir); + const a = getCacheFile("tenant-A", "client-1"); + const b = getCacheFile("tenant-B", "client-1"); + const c = getCacheFile("tenant-A", "client-2"); + expect(a).not.toBe(b); + expect(a).not.toBe(c); + expect(a).toContain("tenant-A"); + expect(a.startsWith(normalize(dir))).toBe(true); + expect(getLegacyCacheFile()).toBe(join(normalize(dir), "token-cache.json")); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("sanitizeForFilename replaces path-unsafe characters (keeps [A-Za-z0-9._-])", () => { + expect(sanitizeForFilename("abc-123_DEF.xyz")).toBe("abc-123_DEF.xyz"); + expect(sanitizeForFilename("a/b\\c:d*e")).toBe("a_b_c_d_e"); + // A traversal-looking value cannot introduce separators into the filename. + expect(sanitizeForFilename("../../etc/passwd")).toBe(".._.._etc_passwd"); + }); +}); diff --git a/src/paths.ts b/src/paths.ts new file mode 100644 index 0000000..fa042ac --- /dev/null +++ b/src/paths.ts @@ -0,0 +1,188 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +/** + * Per-instance data-directory resolution — the SINGLE source of truth for where + * the SPE MCP Server keeps its provisioning state (`state.json`) and MSAL token + * cache (`token-cache.*.json`). + * + * Historically both `state.ts` and `auth.ts` hard-coded `~/.spe-mcp` in a + * module-level `const` evaluated at import time. That froze the location before + * any CLI flag / env var could be parsed, and it meant two MCP server instances + * sharing a home directory would clobber each other's single, unpartitioned + * `state.json`. This module replaces those consts with a lazy, memoized resolver + * so a caller can select the directory per-instance via `--data-dir` / + * `SPE_DATA_DIR`. + * + * Precedence (highest first): explicit override (`setDataDirOverride`, set by the + * CLI from `--data-dir` or `SPE_DATA_DIR`) > `SPE_DATA_DIR` env > default + * `~/.spe-mcp`. The default is byte-for-byte identical to the previous behavior. + * + * IMPORTANT — import-order safety: nothing here captures the resolved path in a + * module-load-time constant. Resolution happens lazily on the first getter call + * (or when the CLI calls `setDataDirOverride`), so the flag/env parsed in the CLI + * action always wins even though `state.ts`/`auth.ts` are imported first. + * + * Cross-platform: every path is built with `node:path` + `os.homedir()`, so it + * resolves correctly on Windows (`%USERPROFILE%\.spe-mcp`) and POSIX + * (`~/.spe-mcp`) alike. The override is treated as UNTRUSTED input: it must be + * absolute after explicit `~` expansion, it is normalized, and a CWD-relative + * path is rejected outright (we never resolve against `process.cwd()`). + */ + +import { homedir } from "node:os"; +import { isAbsolute, join, normalize, sep } from "node:path"; +import { AppError } from "./errors.js"; + +/** Directory name kept under the home directory by default. */ +const DEFAULT_DIR_NAME = ".spe-mcp"; + +/** + * The default data directory: `~/.spe-mcp`. Computed via a function (not a + * module-level const) so tests can exercise a changed `homedir()` and so nothing + * is frozen at import time. + */ +function defaultDataDir(): string { + return join(homedir(), DEFAULT_DIR_NAME); +} + +/** + * Memoized resolved data directory. `null` means "not yet resolved" — the next + * `getDataDir()` will resolve it (from an override set via `setDataDirOverride`, + * else `SPE_DATA_DIR`, else the default). `setDataDirOverride` overwrites it so + * a later override re-resolves lazily on demand. + */ +let memoizedDataDir: string | null = null; + +/** + * Expand a leading `~` against the HOME directory only. `~` alone → home; + * `~/foo` or `~\foo` → `/foo`. A `~user` form is intentionally NOT + * expanded (we don't resolve other users' homes) and will fall through to the + * absolute-path check, which rejects it. + */ +function expandTilde(input: string): string { + if (input === "~") return homedir(); + if (input.startsWith("~/") || input.startsWith("~\\")) { + return join(homedir(), input.slice(2)); + } + return input; +} + +/** Strip a single trailing path separator so the default compares byte-identically. */ +function stripTrailingSep(p: string): string { + if (p.length > 1 && p.endsWith(sep)) return p.slice(0, -1); + return p; +} + +/** + * Resolve a raw data-directory value (from a flag, env var, or nothing) to an + * absolute, normalized path. + * + * - Empty / whitespace / undefined → the default `~/.spe-mcp`. + * - A leading `~` is expanded against `homedir()`. + * - The result MUST be absolute. A CWD-relative path (e.g. `foo`, `./foo`, + * `../foo`) is REJECTED — we never resolve against `process.cwd()`, because an + * attacker-influenced working directory must not be able to redirect where + * refresh tokens are written. + * + * Exported for unit testing and reuse by the CLI. + */ +export function resolveDataDir(input?: string): string { + const raw = (input ?? "").trim(); + if (raw === "") { + return stripTrailingSep(normalize(defaultDataDir())); + } + const expanded = expandTilde(raw); + if (!isAbsolute(expanded)) { + throw new AppError( + "INVALID_DATA_DIR", + `Data directory must be an absolute path (got '${raw}'). Use an absolute path or a '~/...'-relative path; CWD-relative paths are rejected so the token store cannot be redirected by the working directory.`, + { + safeMessage: + "Data directory must be an absolute path (or '~/...'); CWD-relative paths are rejected.", + }, + ); + } + return stripTrailingSep(normalize(expanded)); +} + +/** + * Record an explicit data-directory override (highest precedence). The CLI calls + * this once per invocation from `--data-dir` (falling back to `SPE_DATA_DIR`) + * BEFORE `state.ts`/`auth.ts` first read the directory through the seam. Returns + * the resolved absolute path so the caller can also propagate it (e.g. by + * setting `process.env.SPE_DATA_DIR`) and log it. + * + * This overwrites the memoized value, so a subsequent `getDataDir()` re-resolves + * to the new location (lazy re-resolution). + */ +export function setDataDirOverride(input?: string): string { + memoizedDataDir = resolveDataDir(input); + return memoizedDataDir; +} + +/** + * The resolved, absolute data directory for this process. Lazily resolved and + * memoized on first use: an override set via `setDataDirOverride` wins; otherwise + * `SPE_DATA_DIR` is honored; otherwise the default `~/.spe-mcp` is used. + */ +export function getDataDir(): string { + if (memoizedDataDir === null) { + memoizedDataDir = resolveDataDir(process.env.SPE_DATA_DIR); + } + return memoizedDataDir; +} + +/** Absolute path to the provisioning state file (`/state.json`). */ +export function getStateFile(): string { + return join(getDataDir(), "state.json"); +} + +/** + * The token-cache directory. This is the SAME directory as the data dir — the + * cache and state co-locate under `~/.spe-mcp` — but it is exposed under its own + * name so `auth.ts` reads intent-revealing code. + */ +export function getCacheDir(): string { + return getDataDir(); +} + +/** Make a value safe to embed in a filename (GUIDs are already safe; be defensive). */ +export function sanitizeForFilename(value: string): string { + return value.replace(/[^a-zA-Z0-9._-]/g, "_"); +} + +/** + * Token-cache file path partitioned by tenant + client: + * `/token-cache...json`. Partitioning guarantees + * accounts from different tenants (or client apps) never co-mingle. + */ +export function getCacheFile(tenant: string, client: string): string { + return join( + getDataDir(), + `token-cache.${sanitizeForFilename(tenant)}.${sanitizeForFilename(client)}.json`, + ); +} + +/** + * Legacy single-file token cache (`/token-cache.json`) used before + * per-tenant partitioning. Kept only so logout can clean it up; never read on the + * hot path. + */ +export function getLegacyCacheFile(): string { + return join(getDataDir(), "token-cache.json"); +} + +/** + * Test-only hooks. Not part of the public API. Used to reset the memoized state + * between unit tests so env-var precedence and lazy re-resolution can be asserted + * deterministically. + */ +export const __testing = { + /** Clear the memoized data dir so the next getter re-resolves from env/default. */ + reset(): void { + memoizedDataDir = null; + }, + /** The default data directory (`~/.spe-mcp`), for golden-path assertions. */ + defaultDataDir, +}; diff --git a/src/secure-fs.test.ts b/src/secure-fs.test.ts index dadacdd..e06825e 100644 --- a/src/secure-fs.test.ts +++ b/src/secure-fs.test.ts @@ -2,10 +2,19 @@ // Licensed under the MIT license. import { describe, it, expect, beforeEach, afterEach } from "vitest"; -import { mkdtempSync, rmSync, statSync, existsSync, writeFileSync } from "node:fs"; +import { + mkdtempSync, + mkdirSync, + readFileSync, + rmSync, + statSync, + existsSync, + symlinkSync, + writeFileSync, +} from "node:fs"; import { tmpdir, platform } from "node:os"; import { join } from "node:path"; -import { ensureSecureDir, writeSecureFile } from "./secure-fs.js"; +import { ensureSecureDir, writeSecureFile, readSecureFile } from "./secure-fs.js"; // POSIX permission bits under test, named for readability (see secure-fs.ts). // 0o700 = rwx------ (owner-only, directories) 0o600 = rw------- (owner-only, files) @@ -74,3 +83,55 @@ describe("secure-fs (SEC-003 owner-only credential/state files)", () => { expect(statSync(file).mode & PERMISSION_MASK).toBe(OWNER_RW); }); }); + +describe("secure-fs — fail-closed hardening (symlink / TOCTOU / perms)", () => { + let dir: string; + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), "spe-mcp-securefs-h-")); + }); + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + }); + + it("readSecureFile returns null for a missing file and round-trips content (cross-platform)", () => { + const file = join(dir, "cache.json"); + expect(readSecureFile(file)).toBeNull(); + writeSecureFile(file, "hello"); + expect(readSecureFile(file)).toBe("hello"); + }); + + it.runIf(isPosix)("writeSecureFile refuses to follow a symlinked target (O_NOFOLLOW)", () => { + const real = join(dir, "outside.json"); + writeFileSync(real, "original", { mode: 0o600 }); + const link = join(dir, "link.json"); + symlinkSync(real, link); + expect(() => writeSecureFile(link, "attacker")).toThrow(); + // The real file must NOT have been overwritten through the symlink. + expect(readFileSync(real, "utf-8")).toBe("original"); + }); + + it.runIf(isPosix)("readSecureFile refuses to follow a symlinked cache file", () => { + const real = join(dir, "secret.json"); + writeFileSync(real, "secret", { mode: 0o600 }); + const link = join(dir, "cache-link.json"); + symlinkSync(real, link); + expect(() => readSecureFile(link)).toThrow(); + }); + + it.runIf(isPosix)("ensureSecureDir refuses a symlinked directory", () => { + const realDir = join(dir, "real"); + mkdirSync(realDir, { mode: 0o700 }); + const linkDir = join(dir, "link"); + symlinkSync(realDir, linkDir); + expect(() => ensureSecureDir(linkDir)).toThrow(); + }); + + it.runIf(isPosix)("ensureSecureDir repairs a group/other-accessible directory to 0o700", () => { + const sub = join(dir, "loose"); + mkdirSync(sub, { mode: 0o755 }); + ensureSecureDir(sub); // owner can repair -> must not throw + expect(statSync(sub).mode & PERMISSION_MASK).toBe(0o700); + }); +}); diff --git a/src/secure-fs.ts b/src/secure-fs.ts index 4dc407d..8255658 100644 --- a/src/secure-fs.ts +++ b/src/secure-fs.ts @@ -4,19 +4,52 @@ /** * Restrictive filesystem helpers for credential / state material (SEC-003). * - * The token cache (MSAL refresh tokens) and provisioning state live under - * `~/.spe-mcp/`. On POSIX, default umask yields world-readable `0644` files in - * a `0755` directory — so on a shared host another local user could read the - * refresh token. We therefore create the directory `0o700` and write files - * `0o600`, and best-effort `chmod` any pre-existing files/dir to repair perms - * created before this hardening landed. + * The token cache (MSAL refresh tokens) and provisioning state live under the + * resolved data directory (default `~/.spe-mcp/`, or a `--data-dir` / + * `SPE_DATA_DIR` override). On POSIX, default umask yields world-readable + * `0644` files in a `0755` directory — so on a shared host another local user + * could read the refresh token. We therefore create the directory `0o700` and + * write files `0o600`. + * + * Historically the data directory was ALWAYS the user-owned `~/.spe-mcp`, so a + * fail-open, symlink-following implementation was safe. Now that the directory + * is caller-supplied (potentially from untrusted workspace config), that path + * crosses a trust boundary and these helpers are hardened to FAIL CLOSED: + * - `ensureSecureDir` refuses a directory that is a symlink, not owned by the + * current user, or accessible to group/other (POSIX). On Windows an override + * outside `%USERPROFILE%` gets an owner-only DACL applied via `icacls`, or is + * refused. + * - `writeSecureFile` / `readSecureFile` open the final component with + * `O_NOFOLLOW` and verify the resulting fd with `fstat` (regular file, owner) + * BEFORE writing/reading, and `chmod` the fd — never the path — to defeat + * symlink/TOCTOU swaps. + * A refusal throws, so callers that persist secrets (the MSAL cache writer) + * simply skip persistence and force a fresh interactive sign-in rather than + * writing a refresh token to an insecure location. * * On Windows the POSIX mode bits are largely ignored by the FS; protection - * comes from the per-user profile ACL on `%USERPROFILE%\.spe-mcp`. The chmod - * calls are wrapped so they never throw on platforms that don't support them. + * comes from the profile ACL (default path) or the icacls-applied owner-only + * DACL (off-profile override). */ -import { chmodSync, existsSync, mkdirSync, writeFileSync } from "node:fs"; +import { execFileSync } from "node:child_process"; +import { + chmodSync, + closeSync, + constants as fsConstants, + existsSync, + fchmodSync, + fstatSync, + ftruncateSync, + lstatSync, + mkdirSync, + openSync, + readFileSync, + writeFileSync, +} from "node:fs"; +import { homedir } from "node:os"; +import { resolve, sep } from "node:path"; +import { AppError } from "./errors.js"; /** * POSIX permission modes as octal literals (the leading `0o` is octal in JS). @@ -38,35 +71,196 @@ const OWNER_RW = 0o600; // rw------- (files) */ const IS_POSIX = process.platform !== "win32"; -/** Create a directory (recursively) with owner-only permissions. */ +/** + * `O_NOFOLLOW` makes `open` fail with `ELOOP` if the final path component is a + * symlink, instead of following it to an attacker-chosen target. It is a POSIX + * flag; on Windows Node leaves it `undefined`, so we coalesce to `0` (no-op) and + * rely on the directory ACL there instead. + */ +const O_NOFOLLOW = (fsConstants.O_NOFOLLOW as number | undefined) ?? 0; + +/** + * Per-process memo of directories already validated as secure. Avoids repeating + * the stat / icacls work on the hot path (every state + cache write). The + * per-write fd checks in {@link writeSecureFile} still run every time, so this + * only caches the directory-level decision, not the file-level TOCTOU defense. + */ +const validatedDirs = new Set(); + +function insecureDir(dir: string, reason: string): AppError { + return new AppError("INSECURE_DATA_DIR", `Refusing to use data directory '${dir}': ${reason}.`, { + safeMessage: `Refusing to use an insecure data directory: ${reason}.`, + suggestion: + "Point --data-dir / SPE_DATA_DIR at a directory you own with owner-only permissions (a fresh directory under your home directory is safest).", + }); +} + +function insecureFile(path: string, reason: string): AppError { + return new AppError("INSECURE_CACHE_FILE", `Refusing to use file '${path}': ${reason}.`, { + safeMessage: `Refusing to use an insecure credential/state file: ${reason}.`, + }); +} + +/** True when `dir` resolves to the home directory or something beneath it. */ +function isUnderHome(dir: string): boolean { + const home = resolve(homedir()); + const d = resolve(dir); + return d === home || d.startsWith(home + sep); +} + +/** + * Windows: apply an owner-only DACL to an off-profile override directory, or + * throw. `/inheritance:r` strips inherited ACEs; `/grant:r :(OI)(CI)F` + * replaces the user's ACE with full control inherited by files + subdirs. + * + * icacls is invoked by absolute path (not a bare name) so a planted + * `icacls.exe` on PATH / in the CWD cannot be run in its place. + * + * KNOWN LIMITATION (tracked as a follow-up under Feature AB#3116729): this does + * NOT remove pre-existing *explicit* ACEs and does not verify the directory + * owner (Node has no cheap owner read on Windows). An attacker who can + * pre-create the exact override path with a permissive explicit ACE is not + * fully mitigated here. The default `~/.spe-mcp` (under %USERPROFILE%) is + * unaffected — it inherits the per-user profile ACL and never reaches this path. + */ +function secureWindowsDirAclOrThrow(dir: string): void { + const user = process.env.USERDOMAIN + ? `${process.env.USERDOMAIN}\\${process.env.USERNAME}` + : process.env.USERNAME; + if (!user) { + throw insecureDir(dir, "the current Windows user could not be determined to set an owner-only ACL"); + } + const icacls = process.env.SystemRoot + ? `${process.env.SystemRoot}\\System32\\icacls.exe` + : "C:\\Windows\\System32\\icacls.exe"; + try { + execFileSync(icacls, [dir, "/inheritance:r", "/grant:r", `${user}:(OI)(CI)F`], { + stdio: "ignore", + }); + } catch { + throw insecureDir(dir, "an owner-only ACL could not be applied to this off-profile path"); + } +} + +/** + * Create a directory (recursively) with owner-only permissions and FAIL CLOSED + * if it cannot be verified as owner-only. Safe to call repeatedly (memoized). + */ export function ensureSecureDir(dir: string): void { + const key = resolve(dir); + if (validatedDirs.has(key)) return; + if (!existsSync(dir)) { // `mode` is honored on POSIX at creation time; ignored (harmless) on Windows. mkdirSync(dir, { recursive: true, mode: OWNER_RWX }); - return; } - if (!IS_POSIX) return; // Windows: ACL governs; nothing to repair. - // POSIX: repair perms on a dir that may predate this hardening (best-effort). - try { - chmodSync(dir, OWNER_RWX); - } catch { - /* best-effort repair (e.g. not the owner) — leave existing perms as-is */ + + // Fail-closed validation. lstat ONLY the final component (not the whole + // chain) so legitimately symlinked parents (e.g. macOS /var -> /private/var, + // or a symlinked home) don't trip the check. + const st = lstatSync(key); + if (st.isSymbolicLink()) throw insecureDir(dir, "it is a symlink"); + if (!st.isDirectory()) throw insecureDir(dir, "it is not a directory"); + + if (IS_POSIX) { + // Repair perms that may predate this hardening, then re-verify. If we are + // not the owner, chmod throws EPERM and the ownership check below rejects. + try { + chmodSync(key, OWNER_RWX); + } catch { + /* fall through to the ownership/mode check, which will reject */ + } + const uid = process.getuid?.(); + if (uid !== undefined && st.uid !== uid) { + throw insecureDir(dir, "it is owned by another user"); + } + const mode = lstatSync(key).mode & 0o777; + if (mode & 0o077 && !isUnderHome(dir)) { + // Group/other-accessible. For an explicit off-home override (the + // untrusted-input case) this is fail-closed. For the user's own home tree + // (the default ~/.spe-mcp) we stay best-effort: a mode-ignoring filesystem + // (WSL DrvFs, some NFS/CIFS) must not turn the default path into a hard + // failure — ownership + symlink checks above still apply there. + throw insecureDir(dir, "it is accessible to group or other (expected 0o700)"); + } + } else if (!isUnderHome(dir)) { + // Windows override outside %USERPROFILE% has no inherited profile ACL. + secureWindowsDirAclOrThrow(dir); } + + validatedDirs.add(key); } /** - * Write a file with owner-only (0o600) permissions. `mode` on writeFileSync is - * only honored when the file is *created* (and only on POSIX), so on POSIX we - * also chmod to repair an existing file that may have been written - * world-readable previously. On Windows the chmod is skipped and the profile - * ACL provides the protection. + * Write a file with owner-only (0o600) permissions, opening with `O_NOFOLLOW` + * and verifying the fd (regular file, owner) before writing. Repairs a + * pre-existing world-readable file via `fchmod` on the fd (never the path). + * Throws (fail-closed) if the target is a symlink or owned by another user. */ export function writeSecureFile(path: string, data: string): void { - writeFileSync(path, data, { encoding: "utf-8", mode: OWNER_RW }); - if (!IS_POSIX) return; // Windows: ACL governs; chmod would be a no-op. + // No O_TRUNC: we truncate only AFTER verifying the fd below, so a + // foreign-owned/symlinked target is never emptied before the refusal throws. + const flags = fsConstants.O_WRONLY | fsConstants.O_CREAT | O_NOFOLLOW; + let fd: number; try { - chmodSync(path, OWNER_RW); - } catch { - /* best-effort repair (e.g. not the owner) — leave existing perms as-is */ + fd = openSync(path, flags, OWNER_RW); + } catch (err) { + if ((err as NodeJS.ErrnoException)?.code === "ELOOP") { + throw insecureFile(path, "it is a symlink"); + } + throw err; + } + try { + if (IS_POSIX) { + const st = fstatSync(fd); + if (!st.isFile()) throw insecureFile(path, "it is not a regular file"); + const uid = process.getuid?.(); + if (uid !== undefined && st.uid !== uid) { + throw insecureFile(path, "it is owned by another user"); + } + // chmod the fd (never the path) so a swap between check and change can't + // redirect us. `mode` on open only applies when creating a NEW file, so + // this also repairs a pre-existing world-readable file. + fchmodSync(fd, OWNER_RW); + } + // Truncate only now (post-verification), then write. writeFileSync(fd, …) + // loops until every byte is flushed, handling short writes / EINTR that a + // single writeSync could leave partially written. + ftruncateSync(fd, 0); + writeFileSync(fd, data, "utf-8"); + } finally { + closeSync(fd); + } +} + +/** + * Read a credential/state file, opening with `O_NOFOLLOW` and verifying the fd + * (regular file, owner) before reading. Returns `null` when the file does not + * exist; throws (fail-closed) if the final component is a symlink or is owned + * by another user, so a planted symlink is never read through. + */ +export function readSecureFile(path: string): string | null { + if (!existsSync(path)) return null; + let fd: number; + try { + fd = openSync(path, fsConstants.O_RDONLY | O_NOFOLLOW); + } catch (err) { + const code = (err as NodeJS.ErrnoException)?.code; + if (code === "ELOOP") throw insecureFile(path, "it is a symlink"); + if (code === "ENOENT") return null; + throw err; + } + try { + if (IS_POSIX) { + const st = fstatSync(fd); + if (!st.isFile()) throw insecureFile(path, "it is not a regular file"); + const uid = process.getuid?.(); + if (uid !== undefined && st.uid !== uid) { + throw insecureFile(path, "it is owned by another user"); + } + } + return readFileSync(fd, { encoding: "utf-8" }); + } finally { + closeSync(fd); } } diff --git a/src/state.test.ts b/src/state.test.ts new file mode 100644 index 0000000..0277143 --- /dev/null +++ b/src/state.test.ts @@ -0,0 +1,86 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { homedir, tmpdir } from "node:os"; +import { join, normalize } from "node:path"; +import { readState, writeState, clearState } from "./state.js"; +import { setDataDirOverride, getStateFile, __testing } from "./paths.js"; + +describe("state — per-instance isolation via the data-dir seam", () => { + const savedEnv = process.env.SPE_DATA_DIR; + let dirA: string; + let dirB: string; + + beforeEach(() => { + __testing.reset(); + delete process.env.SPE_DATA_DIR; + dirA = mkdtempSync(join(tmpdir(), "spe-mcp-state-A-")); + dirB = mkdtempSync(join(tmpdir(), "spe-mcp-state-B-")); + }); + + afterEach(() => { + __testing.reset(); + if (savedEnv === undefined) delete process.env.SPE_DATA_DIR; + else process.env.SPE_DATA_DIR = savedEnv; + rmSync(dirA, { recursive: true, force: true }); + rmSync(dirB, { recursive: true, force: true }); + }); + + it("writes state under the resolved data dir, not ~/.spe-mcp", () => { + setDataDirOverride(dirA); + writeState({ tenantId: "tenant-A" }); + expect(getStateFile()).toBe(join(normalize(dirA), "state.json")); + expect(readState().tenantId).toBe("tenant-A"); + }); + + it("keeps two instances isolated: writing dir B leaves dir A byte-identical", () => { + // Instance A writes its state. + setDataDirOverride(dirA); + writeState({ tenantId: "tenant-A", appId: "app-A" }); + const stateFileA = getStateFile(); + const bytesA = readFileSync(stateFileA); + + // Instance B (different data dir) writes DIFFERENT state. + __testing.reset(); + setDataDirOverride(dirB); + writeState({ tenantId: "tenant-B", appId: "app-B" }); + + // A's file is unchanged — no cross-instance clobber. + const bytesA2 = readFileSync(stateFileA); + expect(bytesA2.equals(bytesA)).toBe(true); + + // And each dir reflects only its own writes. + __testing.reset(); + setDataDirOverride(dirA); + expect(readState().tenantId).toBe("tenant-A"); + __testing.reset(); + setDataDirOverride(dirB); + expect(readState().tenantId).toBe("tenant-B"); + }); + + it("clearState removes only the resolving instance's state file", () => { + setDataDirOverride(dirA); + writeState({ tenantId: "tenant-A" }); + __testing.reset(); + setDataDirOverride(dirB); + writeState({ tenantId: "tenant-B" }); + + // Clear A; B must survive. + __testing.reset(); + setDataDirOverride(dirA); + clearState(); + expect(readState()).toEqual({}); + + __testing.reset(); + setDataDirOverride(dirB); + expect(readState().tenantId).toBe("tenant-B"); + }); + + it("golden default: with no override, state resolves to ~/.spe-mcp/state.json", () => { + // No setDataDirOverride, no env — the default path is byte-identical to the + // pre-feature hardcoded location. + expect(getStateFile()).toBe(join(normalize(join(homedir(), ".spe-mcp")), "state.json")); + }); +}); diff --git a/src/state.ts b/src/state.ts index fd90973..d01bbc9 100644 --- a/src/state.ts +++ b/src/state.ts @@ -9,20 +9,18 @@ * flow is resumable/idempotent and `status_get` can report what exists. This is * the MCP analogue of the full-setup skill's `.env.spe`. * - * Cross-platform: there are no shell-command invocations here; every path is - * built with `node:path.join` + `os.homedir()`, so it resolves correctly on - * Windows (`%USERPROFILE%\.spe-mcp`) and POSIX (`~/.spe-mcp`) alike. + * Cross-platform: there are no shell-command invocations here; the data + * directory and state-file paths come from the resolve-once seam in `paths.ts` + * (built with `node:path` + `os.homedir()`), so they resolve correctly on + * Windows (`%USERPROFILE%\.spe-mcp`) and POSIX (`~/.spe-mcp`) alike, and honor a + * `--data-dir` / `SPE_DATA_DIR` override. */ -import { existsSync, readFileSync, rmSync } from "node:fs"; -import { homedir } from "node:os"; -import { join } from "node:path"; -import { ensureSecureDir, writeSecureFile } from "./secure-fs.js"; +import { existsSync, rmSync } from "node:fs"; +import { getDataDir, getStateFile } from "./paths.js"; +import { ensureSecureDir, readSecureFile, writeSecureFile } from "./secure-fs.js"; import type { BillingClassification, OwnerScope } from "./types.js"; -const STATE_DIR = join(homedir(), ".spe-mcp"); -const STATE_FILE = join(STATE_DIR, "state.json"); - export interface ProvisioningState { tenantId?: string; /** Owning Entra app client (application) ID. */ @@ -74,28 +72,33 @@ export interface ProvisioningState { export function readState(): ProvisioningState { try { - if (existsSync(STATE_FILE)) { - return JSON.parse(readFileSync(STATE_FILE, "utf-8")) as ProvisioningState; + // O_NOFOLLOW + owner check (readSecureFile): a symlinked or foreign-owned + // state.json is refused (throws → treated as empty) rather than followed, + // consistent with the writeState hardening. Returns null when absent. + const raw = readSecureFile(getStateFile()); + if (raw !== null) { + return JSON.parse(raw) as ProvisioningState; } } catch { - /* ignore corrupt state — treat as empty */ + /* ignore corrupt or insecure state — treat as empty */ } return {}; } export function writeState(patch: Partial): ProvisioningState { const next = { ...readState(), ...patch }; - ensureSecureDir(STATE_DIR); + ensureSecureDir(getDataDir()); // SEC-003: state can hold tenant/app/subscription IDs — owner-only (0o600). - writeSecureFile(STATE_FILE, JSON.stringify(next, null, 2)); + writeSecureFile(getStateFile(), JSON.stringify(next, null, 2)); return next; } /** Delete the persisted provisioning state (used by cleanup). */ export function clearState(): void { try { - if (existsSync(STATE_FILE)) { - rmSync(STATE_FILE); + const stateFile = getStateFile(); + if (existsSync(stateFile)) { + rmSync(stateFile); } } catch { /* ignore */