From bd1bb356ff2c1a5b140902f7f6f69a2f3a10f71f Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 4 Sep 2026 00:52:34 -0700 Subject: [PATCH] fix(ssh): keep a path sftp cannot spell from becoming a verdict about the host Audit of isSftpUnavailableError, prompted by the '-l' gap having the same shape: a per-operation condition being written into a per-host cache that holds for 30 minutes. It had a second instance, and this one was mine. UnsupportedSftpPathError was classified as 'this host cannot do sftp', but it is thrown for a UNC or relative destination and for any path sftp's batch lexer cannot quote -- including a *local* filename containing a newline, which POSIX clients allow. One such file would have routed every later Windows write to that host down the defective PowerShell 5.1 path for the rest of the cache window. The host verdict is now only the errors that really are host-scoped: a refused subsystem, a client that will not start, and an untranslatable argument list. A path refusal falls back for that one write and leaves the cache alone, in both the file-write and directory-creation paths. Revert-tested. Removing the operation-scoped catch fails all three new tests, whether or not the predicate is also widened. Widening the predicate alone does not fail them, correctly: with the catch in place the predicate no longer gates that path, so keeping it narrow is defence-in-depth rather than the live mechanism. Flag audit at the same time: -F, -o, -T, -S, -p, -i, -J, -l and -- are now the complete set buildSshArgs can emit, and all are handled. --- src/main/ssh/system-ssh-file-transfer.ts | 18 +++++++- src/main/ssh/system-ssh-sftp-transfer.ts | 31 +++++++++---- .../ssh/system-ssh-windows-upload.test.ts | 45 +++++++++++++++++++ .../ssh/system-ssh-windows-write-strategy.ts | 34 ++++++++++++-- 4 files changed, 115 insertions(+), 13 deletions(-) diff --git a/src/main/ssh/system-ssh-file-transfer.ts b/src/main/ssh/system-ssh-file-transfer.ts index 052ac5a79a3..506fa6ec384 100644 --- a/src/main/ssh/system-ssh-file-transfer.ts +++ b/src/main/ssh/system-ssh-file-transfer.ts @@ -27,7 +27,11 @@ import { WINDOWS_STDIN_WRITE_TIMEOUT_MS, writeBufferViaSystemSsh } from './system-ssh-file-binary-transfer' -import { isSftpUnavailableError, makeDirectoriesViaSftp } from './system-ssh-sftp-transfer' +import { + isSftpPathUnsupportedError, + isSftpUnavailableError, + makeDirectoriesViaSftp +} from './system-ssh-sftp-transfer' import { getWindowsRemoteWriteCapabilities } from './system-ssh-windows-write-capabilities' type SystemSshOperationOptions = SystemSshBuildArgsOptions & { @@ -189,7 +193,17 @@ async function createWindowsUploadDirectories( throwIfAborted(options.signal) await getWindowsRemoteWriteCapabilities(target).runWithFallback( 'sftp-subsystem', - () => makeDirectoriesViaSftp(target, pending, options), + async () => { + try { + await makeDirectoriesViaSftp(target, pending, options) + } catch (error) { + // A directory sftp cannot address is this batch's problem, not the host's verdict. + if (!isSftpPathUnsupportedError(error)) { + throw error + } + await createWindowsUploadDirectoriesViaPowerShell(target, payload, options) + } + }, () => createWindowsUploadDirectoriesViaPowerShell(target, payload, options), isSftpUnavailableError ) diff --git a/src/main/ssh/system-ssh-sftp-transfer.ts b/src/main/ssh/system-ssh-sftp-transfer.ts index 2e5c6f6dcb1..c50375fa8eb 100644 --- a/src/main/ssh/system-ssh-sftp-transfer.ts +++ b/src/main/ssh/system-ssh-sftp-transfer.ts @@ -25,16 +25,31 @@ export class SftpSubsystemUnavailableError extends Error { } /** - * True for the errors that mean "this host cannot do sftp", as opposed to "this transfer failed". - * Deliberately narrow: a permission denial or a missing directory is a real failure that must - * surface, not a reason to retry the whole upload down a slower path. + * True for the errors that mean "this host cannot serve sftp at all". + * + * Host-scoped, and therefore the only errors safe to remember: a capability cache keyed by host + * turns anything it accepts into a verdict about every later write to that host. Deliberately + * narrow — a permission denial or a missing directory is a real failure that must surface, not a + * reason to retry the whole upload down a slower path. */ export function isSftpUnavailableError(error: unknown): boolean { - return ( - error instanceof SftpSubsystemUnavailableError || - error instanceof SftpArgTranslationError || - error instanceof UnsupportedSftpPathError - ) + return error instanceof SftpSubsystemUnavailableError || error instanceof SftpArgTranslationError +} + +/** + * True when *this path* cannot be spelled for sftp, which says nothing about the host. + * + * Kept apart from the host verdict on purpose. A UNC destination, or a local file whose name + * contains a newline, is a property of one operation; caching it would degrade every subsequent + * write to that host for the cache's whole retry window on the strength of one odd filename. + */ +export function isSftpPathUnsupportedError(error: unknown): boolean { + return error instanceof UnsupportedSftpPathError +} + +/** Neither kind of refusal moves a byte, so a staged file cannot exist to sweep. */ +export function isSftpRefusalBeforeStaging(error: unknown): boolean { + return isSftpUnavailableError(error) || isSftpPathUnsupportedError(error) } function systemSftpCandidates(sshPath: string | null, platform: NodeJS.Platform): string[] { diff --git a/src/main/ssh/system-ssh-windows-upload.test.ts b/src/main/ssh/system-ssh-windows-upload.test.ts index 6ba8d777507..dbfb053cb89 100644 --- a/src/main/ssh/system-ssh-windows-upload.test.ts +++ b/src/main/ssh/system-ssh-windows-upload.test.ts @@ -436,6 +436,51 @@ describe('Windows upload over sftp', () => { expect(fileWrites()).toHaveLength(0) }) + it('does not let one unaddressable path become a verdict about the host', async () => { + writeFileSync(join(localDir, 'relay.js'), 'x') + + // A UNC destination has no settled mapping in sftp's drive-rooted namespace, so this write + // falls back — but the host still serves sftp perfectly well for every other path. + await uploadFileViaSystemSsh( + target, + join(localDir, 'relay.js'), + '//fileserver/share/relay.js', + { hostPlatform } + ) + + expect(fileWrites().length).toBeGreaterThan(0) + expect(sftpBatches).toHaveLength(0) + // The 30-minute capability cache is keyed by host; caching this would send every later write + // to the same machine down the defective path on the strength of one odd destination. + expect(getWindowsRemoteWriteCapabilities(target).shouldTry('sftp-subsystem')).toBe(true) + }) + + it('keeps using sftp for the next file after one path it could not spell', async () => { + writeFileSync(join(localDir, 'relay.js'), 'x') + + await uploadFileViaSystemSsh(target, join(localDir, 'relay.js'), '//fileserver/share/a.js', { + hostPlatform + }) + await uploadFileViaSystemSsh(target, join(localDir, 'relay.js'), `${remoteRoot}/b.js`, { + hostPlatform + }) + + expect(putLines()).toHaveLength(1) + expect(putDestination(putLines()[0]!)).toContain('/C:/Users/dev/.orca-remote/b.js') + }) + + it('does not let a local filename sftp cannot quote become a verdict either', async () => { + // POSIX clients allow a newline in a filename, and sftp's batch lexer would read it as the end + // of one command and the start of another. + const awkward = join(localDir, 'two\nlines.js') + writeFileSync(awkward, 'x') + + await uploadFileViaSystemSsh(target, awkward, `${remoteRoot}/relay.js`, { hostPlatform }) + + expect(fileWrites().length).toBeGreaterThan(0) + expect(getWindowsRemoteWriteCapabilities(target).shouldTry('sftp-subsystem')).toBe(true) + }) + it('translates the ssh argument list rather than passing it to a client that reads it differently', async () => { writeFileSync(join(localDir, 'relay.js'), 'x') diff --git a/src/main/ssh/system-ssh-windows-write-strategy.ts b/src/main/ssh/system-ssh-windows-write-strategy.ts index 7df9f800925..f2cdca12516 100644 --- a/src/main/ssh/system-ssh-windows-write-strategy.ts +++ b/src/main/ssh/system-ssh-windows-write-strategy.ts @@ -6,7 +6,12 @@ import { throwIfAborted, waitForChannelClose } from './system-ssh-operation-lifecycle' -import { isSftpUnavailableError, runSftpBatch } from './system-ssh-sftp-transfer' +import { + isSftpPathUnsupportedError, + isSftpRefusalBeforeStaging, + isSftpUnavailableError, + runSftpBatch +} from './system-ssh-sftp-transfer' import { quoteSftpBatchArgument, toSftpRemotePath } from './system-ssh-sftp-path' import { getWindowsRemoteWriteCapabilities } from './system-ssh-windows-write-capabilities' import { @@ -108,7 +113,30 @@ async function stageThenPublish( } } -function writeViaSftp( +/** + * A path sftp cannot address falls back for this write alone, without touching the host verdict. + * + * The distinction matters because the capability cache is keyed by host and holds for half an hour: + * routing one UNC destination, or one local filename containing a newline, into + * `rememberUnsupported` would send every later write to that host down the defective path too. + */ +async function writeViaSftp( + target: SshTarget, + remotePath: string, + source: WindowsWriteSource, + options: WindowsWriteOptions +): Promise { + try { + await attemptSftpWrite(target, remotePath, source, options) + } catch (error) { + if (!isSftpPathUnsupportedError(error)) { + throw error + } + await writeViaRemoteStdin(target, remotePath, source, options) + } +} + +function attemptSftpWrite( target: SshTarget, remotePath: string, source: WindowsWriteSource, @@ -133,7 +161,7 @@ function writeViaSftp( options ) ), - isSftpUnavailableError + isSftpRefusalBeforeStaging ) }