Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Verified after the bumps: full suite green, `npm ci` reproducible from the lockfile, and the real server answers `initialize` and `tools/list` correctly over STDIO.

### Fixed
- **Windows/Claude Desktop: every SSH command failed with exit 255 (fixes #10)**: Claude Desktop launches the extension with a stripped, allow-listed environment that omits `%ProgramData%` and `%ALLUSERSPROFILE%`. Win32-OpenSSH resolves `%ProgramData%` at startup to locate its global config (`%ProgramData%\ssh\`) and exits 255 **before producing any output** when it is unset — so every spawned `ssh`/`scp` failed with empty stdout/stderr while the identical command succeeded from a normal shell. Both spawn paths were affected: key-auth hosts get no env override and inherit `process.env` implicitly, and password hosts spread a `process.env` that is itself missing the variable. Both variables are now normalized once at module load, Windows only. Reported by @Krolikfarm with an environment bisection, independently reproduced by @pa-bmundt, and contributed by @pa-bmundt.
- The fallback derives from `%SystemDrive%` instead of hardcoding `C:`, so a Windows install on another drive still gets a valid path (`SystemDrive` is one of the variables Claude Desktop does pass through), and tolerates a trailing separator.
- **Multi-alias hosts (fixes #12)**: A host declared under several aliases (`Host docker-lxc hlab`) was unreachable under *any* of its names. `ssh-config@5` returns a plain string for a single-token value but an array of token objects (`{val, separator, quoted}`) once a directive carries more than one token; `extractHostsFromConfig` stored that array in `alias` verbatim, so every strict comparison downstream (`_assertKnownHostAlias`, `getHostInfo`, `getPasswordForHost`, `getAllKnownHosts`) compared a string against an array and never matched. The host was listed by `listKnownHosts` but rejected by the known-host gate before `ssh` was ever spawned. ssh-config values are now normalized once at parse time: `alias` keeps the first alias (output shape unchanged), a new `aliases` field carries the full list, and matching goes through a shared `hostMatchesAlias()` helper. Contributed by @badigit.
- **Wildcard blocks with negations**: `Host * !bastion` was emitted as a connectable host if it carried a `HostName`. The old `section.value !== '*'` check could not match a multi-token value, which is an array. Blocks consisting only of wildcards and negated patterns are now skipped as the defaults blocks they are.
- **Multi-token directives**: `ProxyCommand`, `SendEnv`, `IPQoS` and friends were surfaced in `listKnownHosts` output as arrays of token objects instead of readable strings. They are now flattened.
Expand Down
19 changes: 19 additions & 0 deletions server.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,25 @@ const execFileAsync = promisify(execFile);

const isWindows = process.platform === 'win32';

// Windows + Claude Desktop: extensions are launched with a stripped, allow-listed
// environment that omits %ProgramData% and %ALLUSERSPROFILE%. Win32-OpenSSH needs
// %ProgramData% to locate its global config (%ProgramData%\ssh\) at startup and
// exits 255 with no output before it produces anything when the variable is unset,
// which makes every spawned ssh/scp fail. Normalize the vars once at module load so
// every spawned child inherits them. See issue #10.
if (isWindows) {
if (!process.env.ProgramData) {
// Derive the last-resort default from %SystemDrive% rather than hardcoding
// C:, so a Windows install on another drive still gets a valid path.
// SystemDrive is part of the environment Claude Desktop does pass through.
const systemDrive = (process.env.SystemDrive || 'C:').replace(/[\\/]+$/, '');
process.env.ProgramData = process.env.ALLUSERSPROFILE || `${systemDrive}\\ProgramData`;
}
if (!process.env.ALLUSERSPROFILE) {
process.env.ALLUSERSPROFILE = process.env.ProgramData;
}
}

// Resolve an executable's absolute path on Windows by walking PATH and PATHEXT.
// This lets us call spawn() with shell:false on Windows — without it we would
// need shell:true to find ssh.exe/scp.exe via PATH, which would route every
Expand Down
132 changes: 130 additions & 2 deletions server.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,19 @@ import { SSHConfigParser, SSHClient, main, SSH_BIN, SCP_BIN } from './server.mjs
// Without this, ~14 tests silently assert POSIX-only behaviour (chmod 600
// checks, the /bin/sh askpass helper, `detached`, a bare 'ssh' argv[0]) and fail
// when the suite runs on Windows.
// Variables server.mjs writes to process.env at import time (the Windows
// ProgramData normalization). They are always saved and restored, whether or not
// a test overrides them — otherwise the first Windows-flavoured import leaks its
// mutation into every later test and makes those branches look covered when
// nothing asserted them.
const ENV_MUTATED_AT_IMPORT = ['ProgramData', 'ALLUSERSPROFILE'];

async function loadServerAs(platform, envOverrides = {}) {
const realPlatform = Object.getOwnPropertyDescriptor(process, 'platform');
const realEnv = {};

for (const key of ENV_MUTATED_AT_IMPORT) realEnv[key] = process.env[key];

for (const [key, value] of Object.entries(envOverrides)) {
realEnv[key] = process.env[key];
if (value === undefined) delete process.env[key];
Expand All @@ -51,10 +60,16 @@ async function loadServerAs(platform, envOverrides = {}) {
// resetModules re-runs the vi.mock factory, so these are new spies — not the
// ones bound by the static import above.
const fs = await import('fs/promises');
return { ...server, fs };
// Snapshot the variables the module writes at import time. The finally block
// below restores process.env immediately, so a test that wants to assert on
// the normalization has to read it from here.
const envAfterImport = Object.fromEntries(
ENV_MUTATED_AT_IMPORT.map(key => [key, process.env[key]])
);
return { ...server, fs, envAfterImport };
} finally {
Object.defineProperty(process, 'platform', realPlatform);
for (const key of Object.keys(envOverrides)) {
for (const key of new Set([...ENV_MUTATED_AT_IMPORT, ...Object.keys(envOverrides)])) {
if (realEnv[key] === undefined) delete process.env[key];
else process.env[key] = realEnv[key];
}
Expand Down Expand Up @@ -1852,3 +1867,116 @@ describe('output truncation', () => {
expect(markers).toHaveLength(1);
});
});

// =============================================================================
// Windows ProgramData normalization (issue #10)
//
// Claude Desktop launches the extension with a stripped, allow-listed
// environment that omits %ProgramData%. Win32-OpenSSH resolves it at startup to
// find its global config (%ProgramData%\ssh\) and exits 255 with no output when
// it is unset, so every spawned ssh/scp fails while the same command works from
// a normal shell. server.mjs restores the variable at import time; these tests
// pin that behaviour, including that it stays out of the way on POSIX.
// =============================================================================

describe('Windows ProgramData normalization', () => {
it('should default ProgramData and ALLUSERSPROFILE when both are missing', async () => {
const win = await loadServerAs('win32', {
ProgramData: undefined,
ALLUSERSPROFILE: undefined,
SystemDrive: 'C:',
});

expect(win.envAfterImport.ProgramData).toBe('C:\\ProgramData');
expect(win.envAfterImport.ALLUSERSPROFILE).toBe('C:\\ProgramData');
});

it('should derive the default from %SystemDrive% rather than hardcoding C:', async () => {
const win = await loadServerAs('win32', {
ProgramData: undefined,
ALLUSERSPROFILE: undefined,
SystemDrive: 'D:',
});

expect(win.envAfterImport.ProgramData).toBe('D:\\ProgramData');
});

it('should tolerate a %SystemDrive% that carries a trailing separator', async () => {
const win = await loadServerAs('win32', {
ProgramData: undefined,
ALLUSERSPROFILE: undefined,
SystemDrive: 'E:\\',
});

expect(win.envAfterImport.ProgramData).toBe('E:\\ProgramData');
});

it('should fall back to C: when %SystemDrive% is missing too', async () => {
const win = await loadServerAs('win32', {
ProgramData: undefined,
ALLUSERSPROFILE: undefined,
SystemDrive: undefined,
});

expect(win.envAfterImport.ProgramData).toBe('C:\\ProgramData');
});

it('should prefer an existing ALLUSERSPROFILE over the drive-based default', async () => {
const win = await loadServerAs('win32', {
ProgramData: undefined,
ALLUSERSPROFILE: 'X:\\CustomProgramData',
});

expect(win.envAfterImport.ProgramData).toBe('X:\\CustomProgramData');
});

it('should leave an already-set ProgramData untouched and backfill ALLUSERSPROFILE', async () => {
const win = await loadServerAs('win32', {
ProgramData: 'Q:\\Existing',
ALLUSERSPROFILE: undefined,
});

expect(win.envAfterImport.ProgramData).toBe('Q:\\Existing');
expect(win.envAfterImport.ALLUSERSPROFILE).toBe('Q:\\Existing');
});

it('should not touch either variable when both are already set', async () => {
const win = await loadServerAs('win32', {
ProgramData: 'Q:\\Existing',
ALLUSERSPROFILE: 'R:\\Other',
});

expect(win.envAfterImport.ProgramData).toBe('Q:\\Existing');
expect(win.envAfterImport.ALLUSERSPROFILE).toBe('R:\\Other');
});

it('should not invent the variables on POSIX', async () => {
const posix = await loadServerAs('linux', {
ProgramData: undefined,
ALLUSERSPROFILE: undefined,
});

expect(posix.envAfterImport.ProgramData).toBeUndefined();
expect(posix.envAfterImport.ALLUSERSPROFILE).toBeUndefined();
});

it('should reach the spawned ssh process through the inherited environment', async () => {
// The whole point of issue #10: key-auth hosts get no env override, so the
// child inherits process.env and must find ProgramData there.
const win = await loadServerAs('win32', {
ProgramData: undefined,
ALLUSERSPROFILE: undefined,
SystemDrive: 'C:',
});
const client = new win.SSHClient();
win.fs.readFile.mockResolvedValue(`Host test\n HostName 1.2.3.4\n`);
client._spawn = createMockSpawn({ stdout: 'ok\n', code: 0 });

await client.runRemoteCommand('test', 'echo ok');

// No password -> no explicit env, so the child inherits the parent's, which
// the import-time normalization has already repaired.
expect(client._spawn.mock.calls[0][2].env).toBeUndefined();
expect(win.envAfterImport.ProgramData).toBe('C:\\ProgramData');
});
});
Loading