diff --git a/src/main/ssh/ssh-config-include-completeness.test.ts b/src/main/ssh/ssh-config-include-completeness.test.ts index 0371fdfb81b..1ab81465a1d 100644 --- a/src/main/ssh/ssh-config-include-completeness.test.ts +++ b/src/main/ssh/ssh-config-include-completeness.test.ts @@ -30,6 +30,7 @@ const lockedPaths = new Set() const canRestrictFileAccess = process.platform !== 'win32' && (process.getuid?.() ?? 0) !== 0 afterEach(() => { + vi.restoreAllMocks() vi.useRealTimers() vi.unstubAllEnvs() invalidateSshConfigAliasClaimCache() @@ -151,6 +152,68 @@ describe('SSH config Include completeness', () => { expect(expandSshConfigIncludes(configPath).fullyExpanded).toBe(true) }) + + /** + * A ~/.ssh holding hundreds of keys and control sockets is ordinary. Reporting it as unopenable + * turned the alias claim permanently uncertain, which silently disabled endpoint restoration. + */ + it('proves a glob complete under a readable directory with hundreds of entries', () => { + const home = makeTemporaryHome() + // The glob's literal parent is ~/.ssh itself, so its entries are what the scan has to walk. + const configPath = writeFile(home, '.ssh/config', 'Include 50-*\n') + writeFile(home, '.ssh/50-wildcard', 'Host *\n ForwardAgent yes\n') + for (let index = 0; index < 302; index += 1) { + writeFile(home, `.ssh/id_key_${index}`, '') + } + + expect(expandSshConfigIncludes(configPath).fullyExpanded).toBe(true) + expect(sshConfigMayClaimAlias('prod', loadUserSshConfigAliasClaims())).toBe(false) + }) + + it('proves a nested glob complete when its literal parent holds hundreds of entries', () => { + const home = makeTemporaryHome() + const configPath = writeFile(home, '.ssh/config', 'Include sub*/config\n') + writeFile(home, '.ssh/sub-work/config', 'Host *\n ForwardAgent yes\n') + for (let index = 0; index < 302; index += 1) { + writeFile(home, `.ssh/id_key_${index}`, '') + } + + expect(expandSshConfigIncludes(configPath).fullyExpanded).toBe(true) + }) +}) + +describe('SSH config Include warnings', () => { + const SECRET_SEGMENT = 's3cr3t-vault' + + it('names the pattern, not the substituted directory, when ${VAR} resolved a path', () => { + const home = makeTemporaryHome() + const configPath = writeFile(home, '.ssh/config', 'Include ${ORCA_TEST_SSH_VAULT}/50-prod\n') + mkdirSync(join(home, '.ssh', SECRET_SEGMENT, '50-prod'), { + recursive: true + }) + vi.stubEnv('ORCA_TEST_SSH_VAULT', join(home, '.ssh', SECRET_SEGMENT)) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + expandSshConfigIncludes(configPath) + + expect(warn).toHaveBeenCalled() + expect(warn.mock.calls.flat().join('\n')).not.toContain(SECRET_SEGMENT) + }) + + it.runIf(canRestrictFileAccess)( + 'keeps the substituted directory out of a glob completeness warning', + () => { + const home = makeTemporaryHome() + const configPath = writeFile(home, '.ssh/config', 'Include ${ORCA_TEST_SSH_VAULT}/*/config\n') + writeFile(home, `.ssh/${SECRET_SEGMENT}/personal/config`, 'Host personal\n') + lockPath(join(home, '.ssh', SECRET_SEGMENT, 'personal')) + vi.stubEnv('ORCA_TEST_SSH_VAULT', join(home, '.ssh', SECRET_SEGMENT)) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + expect(expandSshConfigIncludes(configPath).fullyExpanded).toBe(false) + expect(warn.mock.calls.flat().join('\n')).not.toContain(SECRET_SEGMENT) + } + ) }) describe('SSH config alias claim completeness', () => { diff --git a/src/main/ssh/ssh-config-include-expander.ts b/src/main/ssh/ssh-config-include-expander.ts index 26d45e35ed7..ba58f2a76a2 100644 --- a/src/main/ssh/ssh-config-include-expander.ts +++ b/src/main/ssh/ssh-config-include-expander.ts @@ -1,7 +1,13 @@ import { globSync, readFileSync, realpathSync, statSync } from 'node:fs' import { homedir, hostname } from 'node:os' import { isDefinitiveAbsence } from '../../shared/definitive-filesystem-absence' -import { findGlobExpansionUncertainty, hasGlobPattern } from './ssh-config-include-glob-readability' +import { + createGlobReadabilityProofs, + findGlobExpansionUncertainty, + hasGlobPattern, + type GlobExpansionUncertainty, + type GlobReadabilityProofs +} from './ssh-config-include-glob-readability' import { expandEnvironmentVariables, expandIncludeTokens, @@ -21,6 +27,19 @@ type SshConfigExpansion = { type IncludeExpansionContext = IncludePathContext & { cache: Map fullyExpanded: boolean + /** Shared across every Include so one ~/.ssh is enumerated once per expansion, not once per glob. */ + globProofs: GlobReadabilityProofs +} + +type ResolvedIncludePaths = { + paths: string[] + /** + * What to log in place of a resolved path, or `null` when the resolved path is safe to log. + * + * `${VAR}` expansion can substitute a directory the user keeps secret, and these warnings land in + * local logs. The pattern as written identifies the offending Include line just as well. + */ + redactedLabel: string | null } const MAX_INCLUDE_GLOB_MATCHES = 256 @@ -34,6 +53,7 @@ export function expandSshConfigIncludes(configPath: string): SshConfigExpansion const context: IncludeExpansionContext = { cache: new Map(), fullyExpanded: true, + globProofs: createGlobReadabilityProofs(), home, pathApi, rootDir: pathApi.dirname(configPath), @@ -46,22 +66,41 @@ export function expandSshConfigIncludes(configPath: string): SshConfigExpansion return { content: lines.join('\n'), fullyExpanded: context.fullyExpanded } } -function markIncomplete(context: IncludeExpansionContext, target: string): void { +function markIncomplete(context: IncludeExpansionContext, target: string, detail?: string): void { context.fullyExpanded = false - console.warn(`[ssh] Could not expand SSH config Include "${target}"; hosts may be missing`) + const because = detail ? ` (${detail})` : '' + console.warn( + `[ssh] Could not expand SSH config Include "${target}"${because}; hosts may be missing` + ) } +/** An unreadable directory is the user's to fix; an unproven one is only bigger than our budget. */ +function describeGlobUncertainty( + uncertainty: GlobExpansionUncertainty, + redactedLabel: string | null +): string { + const where = redactedLabel === null ? `"${uncertainty.target}"` : 'a directory it matches' + return uncertainty.reason === 'unreadable' + ? `could not enumerate ${where}` + : `${where} is too large to scan for completeness` +} + +/** + * `logTarget` is what warnings about this file name; it differs from `filePath` only when the path + * came out of `${VAR}` expansion and so must not reach a log. + */ function expandSshConfigFile( filePath: string, context: IncludeExpansionContext, - activeStack: string[] + activeStack: string[], + logTarget: string = filePath ): string[] { - const canonicalPath = getCanonicalPath(filePath, context) + const canonicalPath = getCanonicalPath(filePath, context, logTarget) if (!canonicalPath || activeStack.includes(canonicalPath)) { return [] } - const rawContent = readCachedFile(canonicalPath, context) + const rawContent = readCachedFile(canonicalPath, context, logTarget) if (rawContent === null) { return [] } @@ -77,8 +116,17 @@ function expandSshConfigFile( } for (const includeArg of includeArgs) { - for (const matchedPath of resolveIncludePaths(includeArg, context)) { - appendExpandedLines(expandedLines, expandSshConfigFile(matchedPath, context, nextStack)) + const resolved = resolveIncludePaths(includeArg, context) + for (const matchedPath of resolved.paths) { + appendExpandedLines( + expandedLines, + expandSshConfigFile( + matchedPath, + context, + nextStack, + resolved.redactedLabel ?? matchedPath + ) + ) } } } @@ -94,13 +142,17 @@ function appendExpandedLines(target: string[], lines: readonly string[]): void { } } -function readCachedFile(filePath: string, context: IncludeExpansionContext): string | null { +function readCachedFile( + filePath: string, + context: IncludeExpansionContext, + logTarget: string +): string | null { const cached = context.cache.get(filePath) if (cached !== undefined) { return cached } - if (!isReadableRegularFile(filePath, context)) { + if (!isReadableRegularFile(filePath, context, logTarget)) { return null } @@ -110,7 +162,7 @@ function readCachedFile(filePath: string, context: IncludeExpansionContext): str return content } catch (error) { if (!isDefinitiveAbsence(error)) { - markIncomplete(context, filePath) + markIncomplete(context, logTarget) } return null } @@ -172,17 +224,24 @@ function splitQuotedArguments(input: string): string[] { return args } -function resolveIncludePaths(pattern: string, context: IncludeExpansionContext): string[] { +function resolveIncludePaths( + pattern: string, + context: IncludeExpansionContext +): ResolvedIncludePaths { const withEnv = expandEnvironmentVariables(pattern) if (withEnv === null) { - markIncomplete(context, pattern) - return [] + markIncomplete(context, pattern, 'unset environment variable') + return { paths: [], redactedLabel: pattern } } + // Only a substitution that actually fired can carry a secret into a resolved path. + const redactedLabel = withEnv === pattern ? null : pattern + const logTarget = (resolved: string): string => redactedLabel ?? resolved + const withTokens = expandIncludeTokens(withEnv, context) if (withTokens === null) { - markIncomplete(context, pattern) - return [] + markIncomplete(context, pattern, 'token needs a connection target') + return { paths: [], redactedLabel } } const absolutePattern = resolveIncludePatternPath(withTokens, context) @@ -191,22 +250,31 @@ function resolveIncludePaths(pattern: string, context: IncludeExpansionContext): const matches = globSync(absolutePattern).sort((left, right) => left.localeCompare(right)) if (matches.length > MAX_INCLUDE_GLOB_MATCHES) { console.warn( - `[ssh] Include pattern "${absolutePattern}" matched ${matches.length} files; processing first ${MAX_INCLUDE_GLOB_MATCHES}` + `[ssh] Include pattern "${logTarget(absolutePattern)}" matched ${matches.length} files; processing first ${MAX_INCLUDE_GLOB_MATCHES}` ) context.fullyExpanded = false - return matches.slice(0, MAX_INCLUDE_GLOB_MATCHES) + return { + paths: matches.slice(0, MAX_INCLUDE_GLOB_MATCHES), + redactedLabel + } } // Unconditional, not only on an empty result: a partial expansion is exactly as unproven, and // it is the half that goes on to feed a confident alias claim. - const uncertainTarget = findGlobExpansionUncertainty(absolutePattern, context.pathApi) - if (uncertainTarget) { - markIncomplete(context, uncertainTarget) + const uncertainty = findGlobExpansionUncertainty(absolutePattern, context.pathApi, { + proofs: context.globProofs + }) + if (uncertainty) { + markIncomplete( + context, + logTarget(absolutePattern), + describeGlobUncertainty(uncertainty, redactedLabel) + ) } - return matches + return { paths: matches, redactedLabel } } catch { // A glob that threw walked a directory it could not read; it never proved the set is empty. - markIncomplete(context, absolutePattern) - return [] + markIncomplete(context, logTarget(absolutePattern), 'glob traversal failed') + return { paths: [], redactedLabel } } } @@ -214,36 +282,44 @@ function resolveIncludePaths(pattern: string, context: IncludeExpansionContext): // Include living behind an unreadable parent directory as if the user had never written it. try { statSync(absolutePattern) - return [absolutePattern] + return { paths: [absolutePattern], redactedLabel } } catch (error) { if (!isDefinitiveAbsence(error)) { - markIncomplete(context, absolutePattern) + markIncomplete(context, logTarget(absolutePattern)) } - return [] + return { paths: [], redactedLabel } } } -function getCanonicalPath(filePath: string, context: IncludeExpansionContext): string | null { +function getCanonicalPath( + filePath: string, + context: IncludeExpansionContext, + logTarget: string +): string | null { try { return realpathSync.native(filePath) } catch (error) { if (!isDefinitiveAbsence(error)) { - markIncomplete(context, filePath) + markIncomplete(context, logTarget) } return null } } -function isReadableRegularFile(filePath: string, context: IncludeExpansionContext): boolean { +function isReadableRegularFile( + filePath: string, + context: IncludeExpansionContext, + logTarget: string +): boolean { try { const stats = statSync(filePath) if (!stats.isFile()) { - console.warn(`[ssh] Skipping SSH config include "${filePath}": not a regular file`) + console.warn(`[ssh] Skipping SSH config include "${logTarget}": not a regular file`) return false } if (stats.size > MAX_INCLUDE_FILE_BYTES) { console.warn( - `[ssh] Skipping SSH config include "${filePath}": size ${stats.size} exceeds ${MAX_INCLUDE_FILE_BYTES} bytes` + `[ssh] Skipping SSH config include "${logTarget}": size ${stats.size} exceeds ${MAX_INCLUDE_FILE_BYTES} bytes` ) context.fullyExpanded = false return false @@ -251,7 +327,7 @@ function isReadableRegularFile(filePath: string, context: IncludeExpansionContex return true } catch (error) { if (!isDefinitiveAbsence(error)) { - markIncomplete(context, filePath) + markIncomplete(context, logTarget) } return false } diff --git a/src/main/ssh/ssh-config-include-glob-literal-parent.test.ts b/src/main/ssh/ssh-config-include-glob-literal-parent.test.ts index 38f29c336d2..a274748b451 100644 --- a/src/main/ssh/ssh-config-include-glob-literal-parent.test.ts +++ b/src/main/ssh/ssh-config-include-glob-literal-parent.test.ts @@ -3,6 +3,7 @@ import { tmpdir } from 'node:os' import { join, posix, win32 } from 'node:path' import { afterEach, describe, expect, it, vi } from 'vitest' import { + createGlobReadabilityProofs, findGlobExpansionUncertainty, getLiteralGlobParent } from './ssh-config-include-glob-readability' @@ -58,18 +59,42 @@ describe('findGlobExpansionUncertainty', () => { } const pattern = join(root, '*', 'config') - // Large enough for the literal parent's three entries, so the glob traversal is what runs out. - expect(findGlobExpansionUncertainty(pattern, posix, 4)).toBe(pattern) + // Only the literal parent's own open fits, so the glob traversal is what runs out. + expect(findGlobExpansionUncertainty(pattern, posix, { maxPaths: 2 })).toEqual({ + reason: 'unproven-within-budget', + target: pattern + }) }) - it('charges directory entries to the same budget', () => { + /** + * Entries get their own budget because they are batched reads inside an already-open directory. + * Charging them against the path budget reported a readable ~/.ssh holding a few hundred keys and + * control sockets as unopenable, which permanently disabled the alias claim it feeds. + */ + it('proves a directory holding far more entries than the path budget complete', () => { + const root = mkdtempSync(join(tmpdir(), 'orca-ssh-glob-wide-')) + temporaryDirectories.push(root) + const matched = join(root, 'sub') + mkdirSync(matched) + writeFileSync(join(matched, 'config'), '') + for (let index = 0; index < 302; index += 1) { + writeFileSync(join(root, `id_key_${index}`), '') + } + + expect(findGlobExpansionUncertainty(join(root, 'sub*', 'config'), posix)).toBeNull() + }) + + it('reports a directory past the entry budget as unproven rather than unreadable', () => { const root = mkdtempSync(join(tmpdir(), 'orca-ssh-glob-entries-')) temporaryDirectories.push(root) for (const name of ['one', 'two', 'three']) { writeFileSync(join(root, name), '') } - expect(findGlobExpansionUncertainty(join(root, '*'), posix, 2)).toBe(root) + expect(findGlobExpansionUncertainty(join(root, '*'), posix, { maxEntries: 2 })).toEqual({ + reason: 'unproven-within-budget', + target: root + }) }) it('proves a bounded readable glob complete', () => { @@ -79,7 +104,11 @@ describe('findGlobExpansionUncertainty', () => { mkdirSync(directory) writeFileSync(join(directory, 'config'), '') - expect(findGlobExpansionUncertainty(join(root, '*', 'config'), posix, 32)).toBeNull() + expect( + findGlobExpansionUncertainty(join(root, '*', 'config'), posix, { + maxPaths: 32 + }) + ).toBeNull() }) it('keeps enumeration failures uncertain after the directory opens', () => { @@ -89,6 +118,26 @@ describe('findGlobExpansionUncertainty', () => { throw Object.assign(new Error('enumeration failed'), { code: 'EIO' }) }) - expect(findGlobExpansionUncertainty(join(root, '*'), posix)).toBe(root) + expect(findGlobExpansionUncertainty(join(root, '*'), posix)).toEqual({ + reason: 'unreadable', + target: root + }) + }) + + it('walks a directory once per expansion, so sibling Includes share the proof', () => { + const root = mkdtempSync(join(tmpdir(), 'orca-ssh-glob-proofs-')) + temporaryDirectories.push(root) + writeFileSync(join(root, 'a-config'), '') + writeFileSync(join(root, 'b-config'), '') + const proofs = createGlobReadabilityProofs() + + expect(findGlobExpansionUncertainty(join(root, 'a*'), posix, { proofs })).toBeNull() + + // Any re-enumeration now fails, so a second null can only have come from the memo. + vi.spyOn(Dir.prototype, 'readSync').mockImplementation(() => { + throw Object.assign(new Error('enumeration failed'), { code: 'EIO' }) + }) + expect(findGlobExpansionUncertainty(join(root, 'b*'), posix, { proofs })).toBeNull() + expect(findGlobExpansionUncertainty(join(root, 'b*'), posix)).not.toBeNull() }) }) diff --git a/src/main/ssh/ssh-config-include-glob-readability.ts b/src/main/ssh/ssh-config-include-glob-readability.ts index 6ac1e7aabe7..591e6a6f4c8 100644 --- a/src/main/ssh/ssh-config-include-glob-readability.ts +++ b/src/main/ssh/ssh-config-include-glob-readability.ts @@ -4,9 +4,52 @@ import type { PathApi } from './ssh-config-include-path-resolution' const GLOB_METACHARACTER = /[*?[]/ // Alias checks run on the main process, so uncertainty is safer than an unbounded proof scan. +// Paths are the expensive axis: each one is an open on a filesystem that may be a network mount. const MAX_GLOB_READABILITY_PATHS = 256 +// Entries are batched reads inside a directory that is already open, so they get their own, far +// larger allowance. A ~/.ssh holding hundreds of keys and control sockets is ordinary, and charging +// its entries against the path budget reported a perfectly readable directory as unopenable. +const MAX_GLOB_READABILITY_ENTRIES = 16_384 const GLOB_READABILITY_LIMIT_REACHED = Symbol('glob-readability-limit-reached') +/** + * Why a glob could not be proven complete. Both block a confident alias claim, but they are not the + * same problem and must not be reported as each other: one is a permission or I/O fault the user can + * fix, the other is a readable tree that is simply too big to walk on the main process. + */ +export type GlobUncertaintyReason = 'unreadable' | 'unproven-within-budget' + +export type GlobExpansionUncertainty = { + /** The directory that could not be walked, or the pattern when its own traversal ran out. */ + target: string + reason: GlobUncertaintyReason +} + +/** + * Per-expansion memo of directories already walked, keyed by path, so sibling Includes under one + * parent (`Include conf.d/a*` plus `Include conf.d/b*`) enumerate that parent once. + * + * Only definitive verdicts are stored. Budget exhaustion is not one: it depends on what the calling + * scan had left, so another scan may still prove the same directory. + */ +export type GlobReadabilityProofs = Map + +export type GlobReadabilityScanOptions = { + maxEntries?: number + maxPaths?: number + proofs?: GlobReadabilityProofs +} + +type ScanBudget = { + entries: number + paths: number + proofs: GlobReadabilityProofs +} + +export function createGlobReadabilityProofs(): GlobReadabilityProofs { + return new Map() +} + export function hasGlobPattern(input: string): boolean { return GLOB_METACHARACTER.test(input) } @@ -20,8 +63,7 @@ export function getLiteralGlobParent(pattern: string, pathApi: PathApi): string } /** - * The path that prevents proving a glob complete, or `null` when every traversed directory was - * enumerated. A proof that exceeds its path budget returns the pattern itself as the uncertainty. + * What prevents proving a glob complete, or `null` when every traversed directory was enumerated. * * `globSync` reports what it could see and never reports what it could not: an unreadable directory * yields fewer matches, not an error. Walk each globbed directory level so partial matches are not @@ -30,22 +72,26 @@ export function getLiteralGlobParent(pattern: string, pathApi: PathApi): string export function findGlobExpansionUncertainty( pattern: string, pathApi: PathApi, - maxPaths = MAX_GLOB_READABILITY_PATHS -): string | null { - const budget = { remaining: maxPaths } - const unopenableParent = findUnopenableDirectory(getLiteralGlobParent(pattern, pathApi), budget) - if (unopenableParent) { - return unopenableParent + options: GlobReadabilityScanOptions = {} +): GlobExpansionUncertainty | null { + const budget: ScanBudget = { + entries: options.maxEntries ?? MAX_GLOB_READABILITY_ENTRIES, + paths: options.maxPaths ?? MAX_GLOB_READABILITY_PATHS, + proofs: options.proofs ?? createGlobReadabilityProofs() + } + const unwalkableParent = findUnwalkableDirectory(getLiteralGlobParent(pattern, pathApi), budget) + if (unwalkableParent) { + return unwalkableParent } for (const prefix of getGlobDirectoryPrefixes(pattern, pathApi)) { const directories = globWithinReadabilityLimit(prefix, budget) if (directories === null) { - return pattern + return { reason: 'unproven-within-budget', target: pattern } } for (const directory of directories) { - const unopenable = findUnopenableDirectory(directory, budget) - if (unopenable) { - return unopenable + const unwalkable = findUnwalkableDirectory(directory, budget) + if (unwalkable) { + return unwalkable } } } @@ -53,38 +99,51 @@ export function findGlobExpansionUncertainty( } /** Missing directories and paths below regular files are definitive empty matches. */ -function findUnopenableDirectory(directory: string, budget: { remaining: number }): string | null { +function findUnwalkableDirectory( + directory: string, + budget: ScanBudget +): GlobExpansionUncertainty | null { + if (budget.proofs.has(directory)) { + return budget.proofs.get(directory) ?? null + } + if (budget.paths <= 0) { + return { reason: 'unproven-within-budget', target: directory } + } + budget.paths -= 1 + try { // Read every entry: some network filesystems open successfully but fail during enumeration. const handle = opendirSync(directory) try { while (handle.readSync() !== null) { // Exhaust the directory so a late enumeration error cannot look like a complete glob. - budget.remaining -= 1 - if (budget.remaining < 0) { - // Entries share the scan budget: an unbounded directory stays unproven, not blocking. - return directory + budget.entries -= 1 + if (budget.entries < 0) { + // Readable so far, just bigger than one scan may walk: unproven, and not memoisable. + return { reason: 'unproven-within-budget', target: directory } } } } finally { handle.closeSync() } + budget.proofs.set(directory, null) return null } catch (error) { - return isDefinitiveAbsence(error) ? null : directory + const verdict: GlobExpansionUncertainty | null = isDefinitiveAbsence(error) + ? null + : { reason: 'unreadable', target: directory } + budget.proofs.set(directory, verdict) + return verdict } } /** Stop a proof scan before a broad glob can synchronously walk an unbounded tree. */ -function globWithinReadabilityLimit( - pattern: string, - budget: { remaining: number } -): string[] | null { +function globWithinReadabilityLimit(pattern: string, budget: ScanBudget): string[] | null { try { return globSync(pattern, { exclude: () => { - budget.remaining -= 1 - if (budget.remaining < 0) { + budget.paths -= 1 + if (budget.paths < 0) { throw GLOB_READABILITY_LIMIT_REACHED } return false