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.
This commit is contained in:
Neil
2026-09-04 00:52:34 -07:00
parent 74951ac126
commit bd1bb356ff
4 changed files with 115 additions and 13 deletions
+16 -2
View File
@@ -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
)
+23 -8
View File
@@ -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[] {
@@ -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')
@@ -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<void> {
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
)
}