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'); + }); +});