From 8bb580cc56ed6987a3a0e29771c2ca536eef1db8 Mon Sep 17 00:00:00 2001 From: Aaron Bockelie Date: Mon, 13 Jul 2026 17:46:50 -0500 Subject: [PATCH] fix: .mcpignore semantics, TLS 1.3 startup, rename extension (#250, #252, #253) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bundles the three reported bugs plus an unreported under-blocking bug found while writing the tests for #250, and adds the coverage tooling that would have caught all four. #250 — .mcpignore negation was inverted The parser stripped the leading '!' before Minimatch saw it, so .negate was always false and the branch reading it was dead code. '!folder/*' compiled into a positive exclusion: negation did not merely fail, it inverted. Under-blocking (found while testing the above, not reported) Patterns were handed to Minimatch raw, so two gitignore rules were never implemented: a slash-less pattern matches at any depth ('*.secret' must hide 'a/b/creds.secret'), and a directory match covers its contents ('private/' must hide 'private/notes.md'). No consumer of isExcluded() checks ancestors — every caller passes a full file path — so nothing compensated downstream. The plugin's own shipped .mcpignore template promises both behaviours, so a user following it believed private files were hidden while they were being served to the model. Patterns are now translated into the globs that implement gitignore semantics, and negation is interpreted here rather than by Minimatch. #252 — min TLS 1.3 crashed the server secureProtocol mapped 1.3 to 'TLSv1_3_method', which OpenSSL never shipped, so context creation threw "Unknown method: TLSv1_3_method". Switched to minVersion, which also fixes a second latent bug: secureProtocol PINS one version, so "minimum TLS 1.2" was silently refusing TLS 1.3 clients. The setting now behaves as the floor it is labelled. #253 — rename dropped the extension 'newName' was concatenated onto the source directory verbatim, so renaming 'note.md' to 'renamed' produced an extension-less file that drops out of markdown views. The source extension is now carried over when newName omits one, before the overwrite guard so it checks the right path. Coverage + test contract All four bugs shipped through code paths with zero coverage. Adds `make coverage` / `coverage-map` / `coverage-gate`, a ratcheting coverageThreshold (security boundary held to a far higher floor than global, since a hole there leaks vault content), and tests/test-contract.test.ts enforcing that a green run means something: no .only, no .skip, every file asserts, no tautologies. The contract immediately caught an `expect(true).toBe(true)` placeholder in the path-validator suite, now replaced with real assertions. BREAKING (.mcpignore, both intended): - The double-'!' workaround for #250 now correctly parses as a double negation (net exclude) and will stop working. - A bare '*' now excludes at every depth, per gitignore, rather than top-level only. This is what makes the '*' + '!keep/**' whitelist idiom work. --- .github/workflows/test.yml | 19 +-- Makefile | 14 +- eslint.config.mts | 3 + jest.config.js | 31 +++- package.json | 1 + scripts/coverage-map.mjs | 95 ++++++++++++ src/security/mcp-ignore-manager.ts | 131 +++++++++------- src/semantic/operations/vault.ts | 18 ++- src/tools/semantic-tools.ts | 2 +- src/utils/certificate-manager.ts | 5 +- tests/__mocks__/obsidian.ts | 15 ++ tests/security/mcp-ignore-manager.test.ts | 176 ++++++++++++++++++++++ tests/security/path-validator.test.ts | 37 ++++- tests/security/tls-min-version.test.ts | 120 +++++++++++++++ tests/test-contract.test.ts | 109 ++++++++++++++ tests/vault-rename-extension.test.ts | 102 +++++++++++++ 16 files changed, 800 insertions(+), 78 deletions(-) create mode 100644 scripts/coverage-map.mjs create mode 100644 tests/security/mcp-ignore-manager.test.ts create mode 100644 tests/security/tls-min-version.test.ts create mode 100644 tests/test-contract.test.ts create mode 100644 tests/vault-rename-extension.test.ts diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index b1ae4c7..eb607ad 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -29,14 +29,15 @@ jobs: - name: Build run: npm run build - - name: Run tests - run: npm test - + # Runs the suite and enforces the coverage floors in jest.config.js. + # Fails the build on backslide; raise a floor when real coverage clears it. + - name: Run tests + coverage gate + run: npm run test:coverage + + - name: Coverage map + if: always() + run: node scripts/coverage-map.mjs + - name: Run linter run: npm run lint - continue-on-error: true # Don't fail on lint warnings for now - - # Optional: Add code coverage - # - name: Upload coverage reports - # uses: codecov/codecov-action@v3 - # if: matrix.node-version == '20.x' \ No newline at end of file + continue-on-error: true # Don't fail on lint warnings for now \ No newline at end of file diff --git a/Makefile b/Makefile index 05db6f2..0bffe9e 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: help build dev test lint lint-fix check clean install \ +.PHONY: help build dev test coverage coverage-map coverage-gate lint lint-fix check clean install \ release-patch release-minor release-major release publish promote sync-version mcpb scorecard set-description MIN_OBSIDIAN := 1.6.6 @@ -29,7 +29,17 @@ lint-fix: ## Run ESLint with auto-fix test: ## Run test suite npm test -check: build lint test ## Run all quality gates (build + lint + test) +coverage: ## Run tests with coverage + print the per-file map (html report in coverage/) + npm run test:coverage + @node scripts/coverage-map.mjs + +coverage-map: ## Print the per-file coverage map from the last run (no re-run) + @node scripts/coverage-map.mjs + +coverage-gate: ## Enforce the coverage floors in jest.config.js (exit 1 on backslide) + npm run test:coverage + +check: build lint coverage-gate ## Run all quality gates (build + lint + tests + coverage floors) # --- Release --- # Full cycle: bump → sync → update versions.json → commit → push → trigger workflow diff --git a/eslint.config.mts b/eslint.config.mts index f1e765e..90dbf4f 100644 --- a/eslint.config.mts +++ b/eslint.config.mts @@ -60,6 +60,9 @@ export default tseslint.config( "node_modules", "dist", "tests", + // Generated by `make coverage` — istanbul's HTML report ships its own vendored + // JS, which is not project code and has no tsconfig entry. + "coverage", "test-*.js", "*.config.js", "*.config.mjs", diff --git a/jest.config.js b/jest.config.js index 1c04bd9..ac52a8d 100644 --- a/jest.config.js +++ b/jest.config.js @@ -15,7 +15,36 @@ module.exports = { '!src/main.ts' // Exclude main plugin entry point from coverage ], coverageDirectory: 'coverage', - coverageReporters: ['text', 'lcov', 'html'], + coverageReporters: ['text', 'lcov', 'html', 'json-summary'], + + // Ratchet, not a target. Each floor sits just under the measured value at the time + // it was set, so it blocks backslide without blocking work. Raise a floor when real + // coverage clears it — never lower one to make a run go green. + // + // Jest subtracts glob-matched paths from the global pool, so `global` below is the + // residual (everything except the security/ and validation/ globs), which is why it + // reads lower than the headline number `make coverage` prints. + // + // Directory keys (trailing slash) are aggregate floors across the files beneath them. + // A glob key would instead apply the floor to each file individually — a much harsher + // gate that today's numbers cannot clear. Tightening these to per-file globs is the + // natural next ratchet once the weak files (secure-obsidian-api, mcp-ignore-manager) + // come up. + // + // Measured 2026-07-13 (baseline: statements 38.57 overall). + coverageThreshold: { + // Security boundary: .mcpignore matching, path validation, TLS. Held highest — + // a silent regression here exposes vault content rather than merely breaking a call. + './src/security/': { + statements: 75, branches: 66, functions: 65, lines: 76 + }, + './src/validation/': { + statements: 82, branches: 79, functions: 95, lines: 83 + }, + global: { + statements: 35, branches: 28, functions: 36, lines: 35 + } + }, setupFilesAfterEnv: ['/tests/setup.ts'], moduleNameMapper: { '^obsidian$': '/tests/__mocks__/obsidian.ts' diff --git a/package.json b/package.json index 413ac52..1b35467 100644 --- a/package.json +++ b/package.json @@ -9,6 +9,7 @@ "sync-version": "node sync-version.mjs", "version": "node sync-version.mjs && git add manifest.json", "test": "node --no-warnings node_modules/.bin/jest", + "test:coverage": "node --no-warnings node_modules/.bin/jest --coverage", "lint": "eslint .", "lint:fix": "eslint . --fix" }, diff --git a/scripts/coverage-map.mjs b/scripts/coverage-map.mjs new file mode 100644 index 0000000..ff26a26 --- /dev/null +++ b/scripts/coverage-map.mjs @@ -0,0 +1,95 @@ +#!/usr/bin/env node +/** + * Coverage map — turns coverage/coverage-summary.json into something actionable. + * + * A single global percentage tells you nothing about *where* to spend the next test. + * This ranks files two ways: + * 1. Risk tier — security/boundary code first, because a coverage hole there leaks + * vault content rather than merely breaking a call. + * 2. Absolute gap — uncovered statements, so effort goes where it moves the number. + * + * Run `make coverage` first (this reads the summary it writes). + * `--json` emits one machine-readable line for diffing across releases. + */ +import { readFileSync } from 'fs'; +import { resolve } from 'path'; + +const SUMMARY = resolve('coverage/coverage-summary.json'); + +// Tiers are ordered: the first matching pattern wins. +const TIERS = [ + { name: 'security boundary', re: /^src\/(security|validation)\// }, + { name: 'core api + routing', re: /^src\/(semantic|utils\/obsidian-api|tools|mcp-server)/ }, + { name: 'supporting', re: /.*/ } +]; + +let summary; +try { + summary = JSON.parse(readFileSync(SUMMARY, 'utf8')); +} catch { + console.error(`No coverage summary at ${SUMMARY}.\nRun \`make coverage\` first.`); + process.exit(2); +} + +const cwd = process.cwd() + '/'; +const files = Object.entries(summary) + .filter(([f]) => f !== 'total') + .map(([f, m]) => { + const file = f.replace(cwd, ''); + return { + file, + tier: TIERS.find(t => t.re.test(file)).name, + pct: m.statements.pct, + covered: m.statements.covered, + total: m.statements.total, + gap: m.statements.total - m.statements.covered + }; + }); + +const total = summary.total; + +if (process.argv.includes('--json')) { + console.log(JSON.stringify({ + total: { + statements: total.statements.pct, + branches: total.branches.pct, + functions: total.functions.pct, + lines: total.lines.pct + }, + files: files.map(({ file, tier, pct, gap }) => ({ file, tier, pct, gap })) + })); + process.exit(0); +} + +const bar = pct => { + const filled = Math.round(pct / 5); + return '█'.repeat(filled) + '·'.repeat(20 - filled); +}; + +console.log('\nCOVERAGE MAP\n'); +console.log( + ` overall statements ${total.statements.pct}% branches ${total.branches.pct}% ` + + `functions ${total.functions.pct}% lines ${total.lines.pct}%\n` +); + +for (const { name } of TIERS) { + const tierFiles = files.filter(f => f.tier === name).sort((a, b) => a.pct - b.pct); + if (!tierFiles.length) continue; + + const covered = tierFiles.reduce((n, f) => n + f.covered, 0); + const stmts = tierFiles.reduce((n, f) => n + f.total, 0); + const tierPct = stmts ? (100 * covered / stmts).toFixed(1) : '100.0'; + + console.log(` ${name.toUpperCase()} — ${tierPct}% of ${stmts} statements`); + for (const f of tierFiles) { + const pct = String(f.pct.toFixed(1)).padStart(5); + console.log(` ${bar(f.pct)} ${pct}% ${String(f.gap).padStart(4)} uncov ${f.file}`); + } + console.log(); +} + +console.log(' BIGGEST GAPS (uncovered statements)'); +for (const f of [...files].sort((a, b) => b.gap - a.gap).slice(0, 10)) { + console.log(` ${String(f.gap).padStart(4)} ${String(f.pct.toFixed(1)).padStart(5)}% ${f.file}`); +} +console.log('\n Floors live in jest.config.js (coverageThreshold). `make coverage-gate` enforces them.\n'); diff --git a/src/security/mcp-ignore-manager.ts b/src/security/mcp-ignore-manager.ts index ad92a19..df9d39d 100644 --- a/src/security/mcp-ignore-manager.ts +++ b/src/security/mcp-ignore-manager.ts @@ -2,9 +2,19 @@ import { App } from 'obsidian'; import { Minimatch } from 'minimatch'; import { Debug } from '../utils/debug'; +/** + * One .mcpignore line, compiled. `matchers` are the globs that together implement the + * source pattern's gitignore semantics (see expandPattern); the rule matches a path if + * any of them does. + */ +interface IgnoreRule { + negate: boolean; + matchers: Minimatch[]; +} + /** * MCPIgnoreManager - Handles .mcpignore file-based path exclusions - * + * * Uses .gitignore-style patterns to exclude files and directories from MCP operations. * Patterns are stored in .mcpignore at the vault root (like .gitignore) */ @@ -12,7 +22,7 @@ export class MCPIgnoreManager { private app: App; private ignorePath: string; private patterns: string[] = []; - private matchers: Minimatch[] = []; + private rules: IgnoreRule[] = []; private isEnabled: boolean = false; private lastModified: number = 0; @@ -61,49 +71,85 @@ export class MCPIgnoreManager { } catch { // File doesn't exist or can't be read - no exclusions this.patterns = []; - this.matchers = []; + this.rules = []; this.lastModified = 0; Debug.log('MCPIgnore: No .mcpignore file found, no exclusions active'); } } + /** + * Translate one .gitignore pattern into the minimatch globs that implement it. + * + * Minimatch is a glob matcher, not a gitignore matcher, so handing it a raw pattern + * silently under-blocks. Two gitignore rules have to be expanded explicitly: + * + * - A pattern with no internal '/' matches at ANY depth ('*.secret' hides + * 'a/b/creds.secret'), whereas the bare glob only matches the top level. + * - A directory match also covers everything beneath it ('private/' hides + * 'private/notes.md'). Nothing downstream re-checks ancestors — every caller of + * isExcluded() passes a full file path — so the contents must be matched here or + * they are not excluded at all. + * + * Returns the globs for the pattern; a path matches the pattern if it matches any. + */ + private expandPattern(pattern: string): string[] { + let p = pattern; + + // 'dir/' is directory-only in gitignore. We cannot stat here, so we treat the name + // as matching whether it is a file or a folder; the contents glob below is what + // actually does the work for directories. + if (p.endsWith('/')) { + p = p.slice(0, -1); + } + + // A leading '/' anchors to the vault root; an internal '/' anchors too. Only a + // pattern with no separator at all is depth-agnostic. + const rootAnchored = p.startsWith('/'); + if (rootAnchored) { + p = p.replace(/^\/+/, ''); + } + + const base = rootAnchored || p.includes('/') ? p : `**/${p}`; + + // The second glob excludes the contents when `base` names a directory. It is inert + // for a plain file, since no path can be nested under one. + return [base, `${base}/**`]; + } + /** * Parse .gitignore-style content into patterns */ private parseIgnoreContent(content: string): void { - const lines = content.split('\n'); const validPatterns: string[] = []; - const matchers: Minimatch[] = []; + const rules: IgnoreRule[] = []; - for (const line of lines) { + for (const line of content.split('\n')) { const trimmed = line.trim(); - + // Skip empty lines and comments if (!trimmed || trimmed.startsWith('#')) { continue; } - // Handle negation patterns (!) - let pattern = trimmed; - let negate = false; - if (pattern.startsWith('!')) { - negate = true; - pattern = pattern.substring(1); + // Negation is handled here rather than by Minimatch: the pattern is rewritten + // below, so the '!' would not survive to reach it anyway. + const negate = trimmed.startsWith('!'); + const body = negate ? trimmed.slice(1) : trimmed; + if (!body) { + continue; } try { - // Create minimatch instance with gitignore-compatible options - const matcher = new Minimatch(pattern, { + const matchers = this.expandPattern(body).map(glob => new Minimatch(glob, { dot: true, // Match files starting with . nobrace: false, // Enable {a,b} expansion noglobstar: false, // Enable ** patterns noext: false, // Enable extended matching - nonegate: false, // Allow negation - flipNegate: negate // Handle ! prefix - }); + nonegate: true // '!' is ours to interpret, not Minimatch's + })); validPatterns.push(trimmed); - matchers.push(matcher); + rules.push({ negate, matchers }); } catch (error) { const message = error instanceof Error ? error.message : String(error); Debug.log(`MCPIgnore: Invalid pattern "${trimmed}": ${message}`); @@ -111,7 +157,7 @@ export class MCPIgnoreManager { } this.patterns = validPatterns; - this.matchers = matchers; + this.rules = rules; } /** @@ -120,52 +166,23 @@ export class MCPIgnoreManager { * @returns true if path should be excluded */ isExcluded(path: string): boolean { - Debug.log(`🔍 MCPIgnore.isExcluded called with path: "${path}"`); - - if (!this.isEnabled || this.matchers.length === 0) { - Debug.log(`🔍 MCPIgnore: disabled or no matchers (enabled: ${this.isEnabled}, matchers: ${this.matchers.length})`); + if (!this.isEnabled || this.rules.length === 0) { return false; } // Normalize path (remove leading slash, use forward slashes) const normalizedPath = path.replace(/^\/+/, '').replace(/\\/g, '/'); - Debug.log(`🔍 MCPIgnore: normalized path "${path}" -> "${normalizedPath}"`); - + + // Last matching rule wins, so a later negation can re-include an earlier exclusion. let excluded = false; - - // Process patterns in order - later patterns can override earlier ones - Debug.log(`🔍 MCPIgnore: checking ${this.matchers.length} patterns against "${normalizedPath}"`); - for (let i = 0; i < this.matchers.length; i++) { - const matcher = this.matchers[i]; - let isMatch = matcher.match(normalizedPath); - - // .gitignore directory patterns: "dir/" should match "dir" and "dir/anything" - if (!isMatch && matcher.pattern.endsWith('/')) { - // Try matching without the trailing slash for the directory itself - const dirPattern = matcher.pattern.slice(0, -1); - const dirMatcher = new Minimatch(dirPattern, matcher.options); - isMatch = dirMatcher.match(normalizedPath); - Debug.log(`🔍 MCPIgnore: directory pattern fallback "${dirPattern}" -> match: ${isMatch}`); - } - - Debug.log(`🔍 MCPIgnore: pattern ${i+1}: "${matcher.pattern}" (negate: ${matcher.negate}) -> match: ${isMatch}`); - - if (matcher.negate) { - // Negation pattern - include if it matches - if (isMatch) { - excluded = false; - Debug.log(`🔍 MCPIgnore: negation pattern matched, setting excluded = false`); - } - } else { - // Normal pattern - exclude if it matches - if (isMatch) { - excluded = true; - Debug.log(`🔍 MCPIgnore: normal pattern matched, setting excluded = true`); - } + + for (const rule of this.rules) { + if (rule.matchers.some(matcher => matcher.match(normalizedPath))) { + excluded = !rule.negate; } } - Debug.log(`🔍 MCPIgnore: final result for "${normalizedPath}": excluded = ${excluded}`); + Debug.log(`🔍 MCPIgnore: "${normalizedPath}" excluded = ${excluded}`); return excluded; } diff --git a/src/semantic/operations/vault.ts b/src/semantic/operations/vault.ts index b0a0052..0434306 100644 --- a/src/semantic/operations/vault.ts +++ b/src/semantic/operations/vault.ts @@ -12,6 +12,16 @@ import { ValidationException } from '../../validation/input-validator'; import { RouterContext } from './router-context'; import { Params, paramStr, paramNum, paramBool, requireParamStr } from './shared'; +/** + * Extension of a vault path, including the leading dot ('' when there is none). + * A leading dot is not an extension: '.gitignore' has none. + */ +function extensionOf(path: string): string { + const base = path.substring(path.lastIndexOf('/') + 1); + const dot = base.lastIndexOf('.'); + return dot > 0 ? base.substring(dot) : ''; +} + export async function executeVaultOperation(ctx: RouterContext, action: string, params: Params): Promise { switch (action) { case 'list': { @@ -335,7 +345,13 @@ export async function executeVaultOperation(ctx: RouterContext, action: string, // Extract directory from current path const lastSlash = path.lastIndexOf('/'); const dir = lastSlash >= 0 ? path.substring(0, lastSlash) : ''; - const newPath = dir ? `${dir}/${newName}` : newName; + + // Carry the source extension over when newName omits one, so renaming + // 'note.md' to 'renamed' yields 'renamed.md' rather than an extension-less + // file that drops out of markdown views (#253). An explicit extension in + // newName is honoured as-is, so no double extension is appended. + const resolvedName = extensionOf(newName) ? newName : `${newName}${extensionOf(path)}`; + const newPath = dir ? `${dir}/${resolvedName}` : resolvedName; // Check if destination already exists try { diff --git a/src/tools/semantic-tools.ts b/src/tools/semantic-tools.ts index e446b32..2b53c53 100644 --- a/src/tools/semantic-tools.ts +++ b/src/tools/semantic-tools.ts @@ -462,7 +462,7 @@ function getParametersForOperation(operation: string): Record { }, newName: { type: 'string', - description: 'New filename for rename operation (without path)' + description: 'New filename for rename operation (without path). If the extension is omitted, the source file\'s extension is preserved (renaming "note.md" to "renamed" yields "renamed.md")' }, overwrite: { type: 'boolean', diff --git a/src/utils/certificate-manager.ts b/src/utils/certificate-manager.ts index aec9e1c..4d7f7d0 100644 --- a/src/utils/certificate-manager.ts +++ b/src/utils/certificate-manager.ts @@ -299,7 +299,10 @@ export class CertificateManager { ca, passphrase: config.passphrase, rejectUnauthorized: config.rejectUnauthorized !== false, - secureProtocol: config.minTLSVersion === 'TLSv1.3' ? 'TLSv1_3_method' : 'TLSv1_2_method' + // minVersion sets a floor. The legacy `secureProtocol` method-string API pins one + // exact version, and OpenSSL never shipped a TLSv1_3_method, so selecting TLS 1.3 + // through it threw "Unknown method: TLSv1_3_method" at context creation. + minVersion: config.minTLSVersion ?? 'TLSv1.2' }; Debug.log('🔒 Creating HTTPS server with certificate'); diff --git a/tests/__mocks__/obsidian.ts b/tests/__mocks__/obsidian.ts index c6cb151..9000fb5 100644 --- a/tests/__mocks__/obsidian.ts +++ b/tests/__mocks__/obsidian.ts @@ -25,6 +25,9 @@ export class App { } export class Vault { + adapter: any = new DataAdapter(); + configDir: string = '.obsidian'; + getName(): string { return 'test-vault'; } @@ -48,6 +51,18 @@ export class Vault { } } +export class DataAdapter {} + +export class FileSystemAdapter extends DataAdapter { + getBasePath(): string { + return '/test-vault'; + } +} + +export class Notice { + constructor(public message?: string) {} +} + export class Workspace { getActiveFile(): any { return { diff --git a/tests/security/mcp-ignore-manager.test.ts b/tests/security/mcp-ignore-manager.test.ts new file mode 100644 index 0000000..165250b --- /dev/null +++ b/tests/security/mcp-ignore-manager.test.ts @@ -0,0 +1,176 @@ +/** + * MCPIgnoreManager pattern parsing + matching. + * + * These tests drive the real parser and the real Minimatch pipeline. The only thing + * stubbed is the vault adapter — an I/O boundary that supplies the .mcpignore bytes. + * Stubbing isExcluded() itself (as tests/graph-ignore-exclusion.test.ts does, for its + * own narrower purpose) is what let #250 ship: the matcher was inverted and no test + * could see it. + */ +import { App } from 'obsidian'; +import { MCPIgnoreManager } from '../../src/security/mcp-ignore-manager'; + +async function managerWith(content: string): Promise { + const app = { + vault: { + adapter: { + stat: async () => ({ mtime: 1, ctime: 1, size: content.length, type: 'file' as const }), + read: async () => content + } + } + } as unknown as App; + + const manager = new MCPIgnoreManager(app); + manager.setEnabled(true); + await manager.loadIgnoreFile(); + return manager; +} + +describe('MCPIgnoreManager', () => { + describe('negation (#250)', () => { + it('should un-exclude a whitelisted subtree when negation follows a broad exclude', async () => { + // The exact .mcpignore from the bug report. + const manager = await managerWith(['*', '!folder/*'].join('\n')); + + expect(manager.isExcluded('folder/note.md')).toBe(false); + }); + + it('should still exclude paths the negation does not cover', async () => { + const manager = await managerWith(['*', '!folder/*'].join('\n')); + + expect(manager.isExcluded('top-level.md')).toBe(true); + }); + + it('should not treat a negated pattern as a positive exclusion', async () => { + // Regression guard for the root cause: the leading '!' was stripped before + // Minimatch saw it, leaving negate=false, which turned '!folder/*' into an + // exclusion of folder/* — the precise inverse of what the user asked for. + const manager = await managerWith('!folder/*'); + + expect(manager.isExcluded('folder/note.md')).toBe(false); + }); + + it('should apply last-match-wins so a later exclude overrides an earlier negation', async () => { + const manager = await managerWith(['!folder/*', 'folder/*'].join('\n')); + + expect(manager.isExcluded('folder/note.md')).toBe(true); + }); + + it('should re-exclude on double negation, per gitignore semantics', async () => { + // '!!x' is two negations = a positive exclude. Prior to the #250 fix, one '!' + // was eaten by the manual strip and the other reached Minimatch, so '!!x' was + // the documented workaround for negation being broken. That workaround now + // correctly inverts back to an exclusion. + const manager = await managerWith(['*', '!!folder/*'].join('\n')); + + expect(manager.isExcluded('folder/note.md')).toBe(true); + }); + }); + + describe('pattern matching', () => { + it('should exclude a top-level file matching a wildcard extension pattern', async () => { + const manager = await managerWith('*.secret'); + + expect(manager.isExcluded('creds.secret')).toBe(true); + }); + + it('should not exclude a non-matching file', async () => { + const manager = await managerWith('*.secret'); + + expect(manager.isExcluded('notes.md')).toBe(false); + }); + + // A slash-less pattern matches at any depth in gitignore. Handing '*.secret' to + // Minimatch raw only matched the top level, so a user following the plugin's own + // shipped template ("*.secret # All files ending with .secret in any directory") + // believed nested secrets were hidden while they were still being served. + it('should exclude a nested file matching a slash-less pattern', async () => { + const manager = await managerWith('*.secret'); + + expect(manager.isExcluded('nested/deeply/creds.secret')).toBe(true); + }); + + it('should exclude the directory node itself for a trailing-slash pattern', async () => { + const manager = await managerWith('private/'); + + expect(manager.isExcluded('private')).toBe(true); + }); + + // 'private/' must imply 'private/**'. No consumer of isExcluded() checks ancestors — + // every caller passes a full file path — so if the contents are not matched here, + // they are not excluded anywhere. + it('should exclude the contents of a trailing-slash directory pattern', async () => { + const manager = await managerWith('private/'); + + expect(manager.isExcluded('private/notes.md')).toBe(true); + expect(manager.isExcluded('private/deeply/nested/notes.md')).toBe(true); + }); + + it('should exclude a nested directory matching a slash-less directory pattern', async () => { + const manager = await managerWith('private/'); + + expect(manager.isExcluded('work/private/notes.md')).toBe(true); + }); + + it('should anchor a pattern that contains a slash to the vault root', async () => { + const manager = await managerWith('work/private/'); + + expect(manager.isExcluded('work/private/notes.md')).toBe(true); + expect(manager.isExcluded('other/work/private/notes.md')).toBe(false); + }); + + it('should anchor a leading-slash pattern to the vault root only', async () => { + const manager = await managerWith('/private/'); + + expect(manager.isExcluded('private/notes.md')).toBe(true); + expect(manager.isExcluded('work/private/notes.md')).toBe(false); + }); + + it('should normalize leading slashes and backslashes before matching', async () => { + const manager = await managerWith('private/'); + + expect(manager.isExcluded('/private/notes.md')).toBe(true); + expect(manager.isExcluded('private\\notes.md')).toBe(true); + }); + + // BEHAVIOUR CHANGE: a bare '*' has no slash, so gitignore matches it at every depth + // — it excludes the whole vault, which is what makes the '*' + '!keep/' whitelist + // idiom work. Previously '*' only matched top-level entries. + it('should treat a bare * as excluding every depth', async () => { + const manager = await managerWith('*'); + + expect(manager.isExcluded('top.md')).toBe(true); + expect(manager.isExcluded('folder/note.md')).toBe(true); + expect(manager.isExcluded('a/b/c/deep.md')).toBe(true); + }); + + it('should support the whitelist idiom: exclude everything, re-include one folder', async () => { + const manager = await managerWith(['*', '!folder/**'].join('\n')); + + expect(manager.isExcluded('folder/note.md')).toBe(false); + expect(manager.isExcluded('folder/deeply/nested/note.md')).toBe(false); + expect(manager.isExcluded('elsewhere/note.md')).toBe(true); + }); + }); + + describe('parsing', () => { + it('should ignore comments and blank lines', async () => { + const manager = await managerWith(['# a comment', '', ' ', '*.secret'].join('\n')); + + expect(manager.getPatterns()).toEqual(['*.secret']); + }); + + it('should retain the leading ! in the reported pattern list', async () => { + const manager = await managerWith(['*', '!folder/*'].join('\n')); + + expect(manager.getPatterns()).toEqual(['*', '!folder/*']); + }); + + it('should exclude nothing when disabled', async () => { + const manager = await managerWith('**'); + manager.setEnabled(false); + + expect(manager.isExcluded('folder/note.md')).toBe(false); + }); + }); +}); diff --git a/tests/security/path-validator.test.ts b/tests/security/path-validator.test.ts index 73c0175..f60819f 100644 --- a/tests/security/path-validator.test.ts +++ b/tests/security/path-validator.test.ts @@ -1,4 +1,4 @@ -import { SecurePathValidator, SecurityError } from '../../src/security/path-validator'; +import { SecurePathValidator, TypeSafePathValidator, SecurityError } from '../../src/security/path-validator'; import { App, normalizePath } from 'obsidian'; import * as path from 'path'; import * as fs from 'fs'; @@ -259,10 +259,35 @@ describe('SecurePathValidator', () => { }); describe('TypeSafePathValidator', () => { - // TypeScript compile-time tests would go here - // These mainly test that the branded types work correctly - test('exists for type safety', () => { - // This is mainly a compile-time feature - expect(true).toBe(true); + // The branded ValidatedPath return type is compile-time only, but the class is a real + // SecurePathValidator subclass and must still enforce the boundary at runtime — that + // part is testable, and is what actually protects the vault. + let validator: TypeSafePathValidator; + const mockBaseDir = '/home/user/vault'; + + beforeEach(() => { + jest.clearAllMocks(); + + (fs.existsSync as jest.Mock).mockReturnValue(false); + (path.normalize as jest.Mock).mockImplementation((p: string) => p); + (path.resolve as jest.Mock).mockImplementation((base: string, userPath: string) => `${base}/${userPath}`); + (path.relative as jest.Mock).mockImplementation((from: string, to: string) => + to.startsWith(from) ? to.substring(from.length + 1) : to + ); + + const mockApp = { vault: { adapter: { basePath: mockBaseDir } } }; + validator = new TypeSafePathValidator(mockApp as unknown as App); + }); + + test('returns the validated path for a legitimate input', () => { + expect(validator.validatePath('notes/daily.md')).toBe('notes/daily.md'); + }); + + test('still rejects directory traversal inherited from SecurePathValidator', () => { + expect(() => validator.validatePath('../../etc/passwd')).toThrow(SecurityError); + }); + + test('still rejects empty paths', () => { + expect(() => validator.validatePath('')).toThrow(SecurityError); }); }); \ No newline at end of file diff --git a/tests/security/tls-min-version.test.ts b/tests/security/tls-min-version.test.ts new file mode 100644 index 0000000..7b7a726 --- /dev/null +++ b/tests/security/tls-min-version.test.ts @@ -0,0 +1,120 @@ +/** + * TLS minimum-version negotiation (#252). + * + * Behavioural, not source-grep: the server is really started and a real TLS client + * really handshakes against it. That is what distinguishes "the setting no longer + * crashes" from "the setting does what it says". The previous implementation used the + * legacy `secureProtocol` method-string API, which (a) has no TLSv1_3_method, so + * selecting TLS 1.3 threw at context creation, and (b) PINS one exact version rather + * than setting a floor — so "minimum TLS 1.2" also silently refused TLS 1.3 clients. + */ +import { mkdtempSync, writeFileSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { connect as tlsConnect } from 'tls'; +import { AddressInfo } from 'net'; +import { Server as HttpsServer } from 'https'; +import { Application } from 'express'; +import { App } from 'obsidian'; +import { CertificateManager, CertificateConfig } from '../../src/utils/certificate-manager'; + +const handler = ((_req: unknown, res: { end: (b: string) => void }) => res.end('ok')) as unknown as Application; + +let manager: CertificateManager; +let certPath: string; +let keyPath: string; + +beforeAll(() => { + manager = new CertificateManager(new App() as unknown as App); + const { cert, key } = manager.generateSelfSignedCertificate('localhost'); + + const dir = mkdtempSync(join(tmpdir(), 'mcp-tls-')); + certPath = join(dir, 'cert.pem'); + keyPath = join(dir, 'key.pem'); + writeFileSync(certPath, cert); + writeFileSync(keyPath, key); +}); + +function configFor(minTLSVersion: CertificateConfig['minTLSVersion']): CertificateConfig { + return { enabled: true, certPath, keyPath, minTLSVersion }; +} + +/** Start the server on an ephemeral port and resolve the port actually bound. */ +async function listen(server: HttpsServer): Promise { + await new Promise(resolve => server.listen(0, '127.0.0.1', resolve)); + return (server.address() as AddressInfo).port; +} + +/** Handshake against the server, resolving the negotiated protocol or an error. */ +async function handshake( + port: number, + clientOpts: { minVersion?: string; maxVersion?: string } = {} +): Promise<{ ok: true; protocol: string } | { ok: false; error: string }> { + return new Promise(resolve => { + const socket = tlsConnect( + { port, host: '127.0.0.1', rejectUnauthorized: false, ...clientOpts } as never, + () => { + const protocol = socket.getProtocol() ?? 'unknown'; + socket.end(); + resolve({ ok: true, protocol }); + } + ); + socket.on('error', (err: Error) => resolve({ ok: false, error: err.message })); + }); +} + +describe('CertificateManager TLS minimum version (#252)', () => { + it('should start an HTTPS server when the minimum is TLS 1.3', async () => { + // Pre-fix this threw "Unknown method: TLSv1_3_method" and the server never bound. + const server = manager.createServer(handler, configFor('TLSv1.3'), 0) as HttpsServer; + const port = await listen(server); + + const result = await handshake(port); + + expect(result).toEqual({ ok: true, protocol: 'TLSv1.3' }); + server.close(); + }); + + it('should refuse a client capped below the TLS 1.3 minimum', async () => { + const server = manager.createServer(handler, configFor('TLSv1.3'), 0) as HttpsServer; + const port = await listen(server); + + const result = await handshake(port, { maxVersion: 'TLSv1.2' }); + + expect(result.ok).toBe(false); + server.close(); + }); + + it('should treat TLS 1.2 as a floor, not a pin, and still allow TLS 1.3', async () => { + // The pre-fix `secureProtocol: 'TLSv1_2_method'` pinned the server to exactly + // TLS 1.2, so a modern client negotiated 1.2 even though 1.3 was available. The + // setting is labelled a *minimum*; this asserts it behaves like one. + const server = manager.createServer(handler, configFor('TLSv1.2'), 0) as HttpsServer; + const port = await listen(server); + + const result = await handshake(port); + + expect(result).toEqual({ ok: true, protocol: 'TLSv1.3' }); + server.close(); + }); + + it('should still accept a TLS 1.2 client when the minimum is TLS 1.2', async () => { + const server = manager.createServer(handler, configFor('TLSv1.2'), 0) as HttpsServer; + const port = await listen(server); + + const result = await handshake(port, { maxVersion: 'TLSv1.2' }); + + expect(result).toEqual({ ok: true, protocol: 'TLSv1.2' }); + server.close(); + }); + + it('should default to a TLS 1.2 floor when no minimum is configured', async () => { + const server = manager.createServer(handler, { enabled: true, certPath, keyPath }, 0) as HttpsServer; + const port = await listen(server); + + const result = await handshake(port, { maxVersion: 'TLSv1.1' }); + + expect(result.ok).toBe(false); + server.close(); + }); +}); diff --git a/tests/test-contract.test.ts b/tests/test-contract.test.ts new file mode 100644 index 0000000..959fe79 --- /dev/null +++ b/tests/test-contract.test.ts @@ -0,0 +1,109 @@ +/** + * The test contract — rules the test suite itself must satisfy. + * + * A test suite is only worth its green checkmark if a passing run actually means the + * code works. Every rule below closes a way a suite can be green while proving nothing. + * This file is the enforcement; it lints the corpus the way ESLint lints src/. + * + * (Source-grep guards are an established pattern here — see tls-cert-verification.test.ts. + * This avoids adding eslint-plugin-jest, which the CLAUDE.md supply-chain hold would + * otherwise delay by 7 days.) + * + * Rules: + * 1. No focused tests — `.only` greens a run while silently skipping everything else. + * 2. No skipped tests — a skip is an assertion you have stopped making. Known bugs + * use `it.failing`, which fails if the bug is ever fixed and + * therefore self-corrects; a `.skip` rots silently forever. + * 3. Every file asserts — a test with no expect() passes by merely not throwing. + * 4. No lone expect(true).toBe(true) style tautologies. + * 5. Coverage floors exist — the ratchet in jest.config.js must not be deleted. + */ +import { readFileSync, readdirSync, statSync } from 'fs'; +import { join } from 'path'; + +const TESTS_DIR = join(__dirname); + +function testFiles(dir: string): string[] { + return readdirSync(dir).flatMap(entry => { + const full = join(dir, entry); + if (statSync(full).isDirectory()) return testFiles(full); + return full.endsWith('.test.ts') ? [full] : []; + }); +} + +const FILES = testFiles(TESTS_DIR).map(path => ({ + path, + rel: path.replace(join(__dirname, '..') + '/', ''), + // Strip block and line comments so prose about `.only` (like this file's own header) + // is not mistaken for the real thing. + source: readFileSync(path, 'utf8') + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/(^|[^:])\/\/.*$/gm, '$1') +})); + +describe('test contract', () => { + it('should have found the test corpus', () => { + // Guards the guard: if the walker breaks, every rule below would vacuously pass. + expect(FILES.length).toBeGreaterThan(20); + }); + + it('should contain no focused tests', () => { + const offenders = FILES + .filter(f => /\b(?:it|test|describe)\.only\b|\bfit\(|\bfdescribe\(/.test(f.source)) + .map(f => f.rel); + + expect(offenders).toEqual([]); + }); + + it('should contain no skipped tests', () => { + const offenders = FILES + .filter(f => /\b(?:it|test|describe)\.skip\b|\bxit\(|\bxdescribe\(/.test(f.source)) + .map(f => f.rel); + + expect(offenders).toEqual([]); + }); + + it('should assert something in every test file', () => { + const offenders = FILES + .filter(f => !/\bexpect\(/.test(f.source)) + .map(f => f.rel); + + expect(offenders).toEqual([]); + }); + + it('should contain no tautological assertions', () => { + const offenders = FILES + .filter(f => /expect\(\s*(true|false|1)\s*\)\s*\.\s*toBe\(\s*(true|false|1)\s*\)/.test(f.source)) + .map(f => f.rel); + + expect(offenders).toEqual([]); + }); +}); + +describe('coverage ratchet', () => { + // eslint-disable-next-line @typescript-eslint/no-require-imports -- jest.config.js is CJS + const config = require('../jest.config.js'); + + it('should define coverage thresholds', () => { + expect(config.coverageThreshold).toBeDefined(); + }); + + it('should hold the security boundary to a higher floor than the global one', () => { + const security = config.coverageThreshold['./src/security/']; + const global = config.coverageThreshold.global; + + expect(security.statements).toBeGreaterThan(global.statements); + expect(security.branches).toBeGreaterThan(global.branches); + }); + + it('should keep every floor above zero', () => { + const floors = Object.values(config.coverageThreshold) as Record[]; + + expect(floors.length).toBeGreaterThan(0); + for (const floor of floors) { + for (const value of Object.values(floor)) { + expect(value).toBeGreaterThan(0); + } + } + }); +}); diff --git a/tests/vault-rename-extension.test.ts b/tests/vault-rename-extension.test.ts new file mode 100644 index 0000000..e136c8e --- /dev/null +++ b/tests/vault-rename-extension.test.ts @@ -0,0 +1,102 @@ +/** + * vault rename — extension preservation (#253). + * + * Drives the real SemanticRouter -> executeVaultOperation path. Only the vault I/O + * boundary is stubbed (ObsidianAPI.getFile, app.fileManager.renameFile), so the path + * construction under test is the shipped one. The rename action had no behavioural + * test at all before this, which is why the dropped extension shipped. + */ +import { SemanticRouter } from '../src/semantic/router'; +import { ObsidianAPI } from '../src/utils/obsidian-api'; +import { App, TFile } from 'obsidian'; + +interface RenameResult { + success: boolean; + oldPath: string; + newPath: string; +} + +class MockObsidianAPI extends ObsidianAPI { + constructor(private existing: Set) { + super({} as App); + } + + async getFile(path: string): Promise { + if (!this.existing.has(path)) { + throw new Error(`File not found: ${path}`); + } + return { path, content: 'body' } as never; + } +} + +/** + * Fake Obsidian app that records the destination handed to fileManager.renameFile — + * the actual side effect a rename produces on the vault. + */ +function fakeApp(existing: Set, renamed: string[]): App { + return { + vault: { + getAbstractFileByPath: (path: string) => + existing.has(path) ? ({ path, extension: 'md' } as unknown as TFile) : null + }, + fileManager: { + renameFile: async (_file: TFile, newPath: string) => { + renamed.push(newPath); + } + } + } as unknown as App; +} + +async function rename(source: string, newName: string): Promise<{ result: RenameResult; renamed: string[] }> { + const existing = new Set([source]); + const renamed: string[] = []; + const router = new SemanticRouter(new MockObsidianAPI(existing), fakeApp(existing, renamed)); + + const response = await router.route({ + operation: 'vault', + action: 'rename', + params: { path: source, newName } + }); + + return { result: response.result as unknown as RenameResult, renamed }; +} + +describe('vault rename — extension handling (#253)', () => { + it('should preserve the source extension when newName omits one', async () => { + const { result, renamed } = await rename('work/my-note.md', 'my-renamed'); + + expect(result.newPath).toBe('work/my-renamed.md'); + expect(renamed).toEqual(['work/my-renamed.md']); + }); + + it('should not double up the extension when newName already has one', async () => { + const { result, renamed } = await rename('work/my-note.md', 'my-renamed.md'); + + expect(result.newPath).toBe('work/my-renamed.md'); + expect(renamed).toEqual(['work/my-renamed.md']); + }); + + it('should preserve a non-markdown source extension', async () => { + const { result } = await rename('assets/diagram.png', 'architecture'); + + expect(result.newPath).toBe('assets/architecture.png'); + }); + + it('should honour an explicit different extension in newName', async () => { + const { result } = await rename('work/my-note.md', 'my-renamed.txt'); + + expect(result.newPath).toBe('work/my-renamed.txt'); + }); + + it('should preserve the extension for a file at the vault root', async () => { + const { result } = await rename('note.md', 'renamed'); + + expect(result.newPath).toBe('renamed.md'); + }); + + it('should leave an extension-less source extension-less', async () => { + const { result } = await rename('work/LICENSE', 'COPYING'); + + expect(result.newPath).toBe('work/COPYING'); + }); +});