Fix Windows path detection in cleanPathWithBase - #644
Open
owenthereal wants to merge 1 commit into
Open
Conversation
Use filepath.IsAbs on the original path before converting to forward slashes. This correctly handles Windows paths like "C:\foo" which filepath.IsAbs recognizes but path.IsAbs (after ToSlash conversion to "C:/foo") does not.
Collaborator
|
Remote paths to windows servers require absolute paths to be encoded as This is not a bug. |
Collaborator
|
I keep thinking I’m missing something here. I’m thinking I need this explained better. This code is only called from within the |
Author
|
The premise that "this code is only called from within the
On Windows, a Go program calling either of these with a native path — e.g. That happens because Agreed that the wire form for SFTP-to-Windows is Happy to add a Windows test that exercises this if that would help. |
Collaborator
|
Yeah, a Windows-specific test would be great. 🤔 It looks like you’re right, there are a few API positions where we’re calling this on a string that users might naturally expect to use a local filepath, rather than a POSIX path. It might be a good idea to instead split these uses into separate |
optimalone
added a commit
to owenthereal/upterm
that referenced
this pull request
Sep 9, 2026
upterm carried two go.mod replace directives, and together they make `go install github.com/owenthereal/upterm/cmd/upterm@latest` fail: Go builds ...@latest in module-aware mode and refuses a main module whose go.mod contains replace directives (https://go.dev/ref/mod#go-install). This is issue #511. This removes the sftp one; the x/crypto replace remains and still blocks `go install` on its own. The fork existed to carry pkg/sftp#644, which changes cleanPathWithBase to test filepath.IsAbs on the original path instead of path.IsAbs on the slash-converted one. That patch is wrong, and the bug it works around is ours. cleanPathWithBase operates on SFTP wire paths, which are POSIX on every platform: a Windows server exposes C:\dir to clients as /C:/dir. path.IsAbs is not failing to be cross-platform there, it implements the POSIX rule everywhere, which is what a wire path needs. filepath.IsAbs is platform-aware, and on Windows requires a volume name, so it reports false for every path without a drive letter. Substituting it lets the server's own filesystem conventions decide how remote paths are read. Running Go's volumeNameLen and IsAbs from internal/filepathlite/path_windows.go against both versions: base p upstream with #644 / C:\a /C:/a C:/a /C:/Users/foo /C:/Users/foo/test.txt /C:/Users/foo/test.txt /C:/Users/foo/C:/Users/foo/test.txt /start /abs/path /abs/path /start/abs/path The first breaks pkg/sftp's own TestCleanPath. The second is the doubling #644 sets out to fix, reintroduced for the canonical wire form. The third re-roots every absolute remote path under the session's start directory, which is exactly how upterm runs its SFTP server. The real trigger was on the client side. Client.Open sends the path verbatim -- client.go has no filepath reference at all -- and these tests passed filepath.Join(t.TempDir(), ...), so on Windows the wire carried a native C:\... path rather than the /C:/... form OpenSSH's clients use. The server then joined it onto the start directory and the request landed on /C:/Users/runneradmin/C:/Users/runneradmin/.../download-test.txt. Tests now encode paths the way the protocol expects, so the fork is no longer needed and upterm requires github.com/pkg/sftp directly. The two are the same code: v1.13.10 is commit 939b203, and the fork's base is two commits ahead of it, both an x/crypto bump. The functional SFTP tests cover this end to end and run unskipped on the Windows job, but only implicitly; a regression would surface as a confusing permission or not-found error from a doubled path, layers away from the cause. TestRemotePathIsPOSIXAbsolute states the requirement instead, which is platform-independent even though the bug is not: whatever is handed to the sftp client must satisfy path.IsAbs. Not verified on Windows locally. The table above is Go's Windows path logic executed on darwin, not a Windows test run. The Test (Windows) job covers ftests, so CI is the check that matters.
optimalone
added a commit
to owenthereal/upterm
that referenced
this pull request
Sep 9, 2026
upterm carried two go.mod replace directives, and together they make `go install github.com/owenthereal/upterm/cmd/upterm@latest` fail: Go builds ...@latest in module-aware mode and refuses a main module whose go.mod contains replace directives (https://go.dev/ref/mod#go-install). This is issue #511. This removes the sftp one; the x/crypto replace remains and still blocks `go install` on its own. The fork existed to carry pkg/sftp#644, which changes cleanPathWithBase to test filepath.IsAbs on the original path instead of path.IsAbs on the slash-converted one. That patch is wrong, and the bug it works around is ours. cleanPathWithBase operates on SFTP wire paths, which are POSIX on every platform: a Windows server exposes C:\dir to clients as /C:/dir. path.IsAbs is not failing to be cross-platform there, it implements the POSIX rule everywhere, which is what a wire path needs. filepath.IsAbs is platform-aware, and on Windows requires a volume name, so it reports false for every path without a drive letter. Substituting it lets the server's own filesystem conventions decide how remote paths are read. Running Go's volumeNameLen and IsAbs from internal/filepathlite/path_windows.go against both versions: base p upstream with #644 / C:\a /C:/a C:/a /C:/Users/foo /C:/Users/foo/test.txt /C:/Users/foo/test.txt /C:/Users/foo/C:/Users/foo/test.txt /start /abs/path /abs/path /start/abs/path The first breaks pkg/sftp's own TestCleanPath. The second is the doubling #644 sets out to fix, reintroduced for the canonical wire form. The third re-roots every absolute remote path under the session's start directory, which is exactly how upterm runs its SFTP server. The real trigger was on the client side. Client.Open sends the path verbatim -- client.go has no filepath reference at all -- and these tests passed filepath.Join(t.TempDir(), ...), so on Windows the wire carried a native C:\... path rather than the /C:/... form OpenSSH's clients use. The server then joined it onto the start directory and the request landed on /C:/Users/runneradmin/C:/Users/runneradmin/.../download-test.txt. Tests now encode paths the way the protocol expects, so the fork is no longer needed and upterm requires github.com/pkg/sftp directly. The two are the same code: v1.13.10 is commit 939b203, and the fork's base is two commits ahead of it, both an x/crypto bump. The functional SFTP tests cover this end to end and run unskipped on the Windows job, but only implicitly; a regression would surface as a confusing permission or not-found error from a doubled path, layers away from the cause. TestRemotePathIsPOSIXAbsolute states the requirement instead, which is platform-independent even though the bug is not: whatever is handed to the sftp client must satisfy path.IsAbs. Not verified on Windows locally. The table above is Go's Windows path logic executed on darwin, not a Windows test run. The Test (Windows) job covers ftests, so CI is the check that matters.
owenthereal
pushed a commit
to owenthereal/upterm
that referenced
this pull request
Sep 9, 2026
…tion tests, exit-status and charm.land/ssh fixes (#523) * Add channel-proxy characterization tests, and a trustworthy baseline The functional suite is the safety net for replacing the relay's front door with a stock x/crypto proxy. Today the relay forwards decrypted SSH *packets*, so channel numbering, windows, request ordering and close ordering survive untouched. A stock proxy terminates the channel layer on each side and re-originates every channel and every request, at which point each of those becomes a way to lose data silently. ftests/proxy_test.go pins eight of them across ssh/ws and single/multi node: exit-status delivered before the channel closes, a forced command's termination reported at all, five megabytes each way through sftp, mid-session window-change, channel requests sent before the shell starts, an unknown request refused rather than fatal, eight concurrent guests, and a session idle across several keepalive rounds. Three hazards are deliberately absent, with reasons recorded in the file, because upterm's guest surface cannot reach them and a test that cannot fail is worse than no test: stderr as a separate stream, stdin half-close, and a guest whose key the host rejects. They belong to the forwarder's own unit tests. testHostClientCallback also gets a trustworthy wait. It failed roughly one full-suite run in five, which is corrosive to a rewrite that uses the suite as its safety net. Measured, the client-left callback fires ~600us after Client.Close() returns, so the 2s budget it used had three orders of magnitude of headroom and the failure was not a slow machine. The test was using wall-clock time as a proxy for causality on an event it had already caused. It now allows 10s, dumps goroutine stacks on timeout so a recurrence is diagnosable, and buffers the event channels: the emitter delivers from a goroutine that holds an emitter-wide lock while it blocks on the listener, so one callback stuck on an unbuffered send stalls every later event on that host. * Report a forced command's exit code to the guest reliably HandleSession read the session's exit status off run.Group.Run, which returns whichever actor finished first. When a forced command exits, two actors unblock at the same instant: the wait, which carries the status, and the output copy, which sees the pty's EOF and returns nil. The guest therefore got the command's real code or a clean 0 depending on scheduling. Measured with --force-command 'exit 42': 42 in one of the four functional-test topologies and 0 in the other three, same run. The actor that waits on the command now records the status itself. run.Group.Run drains every actor before returning, so reading it afterwards is ordered. Windows never reported the code at all. pty.Wait returned fmt.Errorf("exit status %d"), which no caller could parse, so every failing forced command surfaced as 1. It now returns a typed *ExitError. A status is used only when the process exited under its own control. exec reports -1 for a signalled process, which is what happens when the pty is closed underneath a still-running command during teardown, and an SSH exit status is marshalled as a uint32, so passing that on would send the guest 4294967295. Also stop falling through after rejecting a session with no pty. Exit closes the channel, so the rest of the handler was attaching a dead session to the shared output writer, starting a keepalive ticker and a window-change loop against it, and ending with a second Exit that could only fail. * Move to charm.land/ssh v0.4.3 The library's import path moved to charm.land/ssh, a vanity path on the same GitHub repository. It is not archived and has not been absorbed into charmbracelet/wish, which is built on top of it. upterm was pinned to v0.0.0-20250826160808-ebfa259c7309, a pseudo-version from 2025-08-26, so the tree was a year behind on the library that terminates SSH for every host, missing two merged data-race fixes (SetOption, handshakeDeadline). Catching up needs no API changes: a path swap in five files and go.mod. It does not fix the sess.pty race, which is still present in v0.4.3 and is reported upstream as charmbracelet/ssh#58 with a fix in #59. testProxyWindowChange works around it in the meantime by ordering the two accesses. * Drop the sftp replace directive, and send canonical SFTP paths upterm carried two go.mod replace directives, and together they make `go install github.com/owenthereal/upterm/cmd/upterm@latest` fail: Go builds ...@latest in module-aware mode and refuses a main module whose go.mod contains replace directives (https://go.dev/ref/mod#go-install). This is issue #511. This removes the sftp one; the x/crypto replace remains and still blocks `go install` on its own. The fork existed to carry pkg/sftp#644, which changes cleanPathWithBase to test filepath.IsAbs on the original path instead of path.IsAbs on the slash-converted one. That patch is wrong, and the bug it works around is ours. cleanPathWithBase operates on SFTP wire paths, which are POSIX on every platform: a Windows server exposes C:\dir to clients as /C:/dir. path.IsAbs is not failing to be cross-platform there, it implements the POSIX rule everywhere, which is what a wire path needs. filepath.IsAbs is platform-aware, and on Windows requires a volume name, so it reports false for every path without a drive letter. Substituting it lets the server's own filesystem conventions decide how remote paths are read. Running Go's volumeNameLen and IsAbs from internal/filepathlite/path_windows.go against both versions: base p upstream with #644 / C:\a /C:/a C:/a /C:/Users/foo /C:/Users/foo/test.txt /C:/Users/foo/test.txt /C:/Users/foo/C:/Users/foo/test.txt /start /abs/path /abs/path /start/abs/path The first breaks pkg/sftp's own TestCleanPath. The second is the doubling #644 sets out to fix, reintroduced for the canonical wire form. The third re-roots every absolute remote path under the session's start directory, which is exactly how upterm runs its SFTP server. The real trigger was on the client side. Client.Open sends the path verbatim -- client.go has no filepath reference at all -- and these tests passed filepath.Join(t.TempDir(), ...), so on Windows the wire carried a native C:\... path rather than the /C:/... form OpenSSH's clients use. The server then joined it onto the start directory and the request landed on /C:/Users/runneradmin/C:/Users/runneradmin/.../download-test.txt. Tests now encode paths the way the protocol expects, so the fork is no longer needed and upterm requires github.com/pkg/sftp directly. The two are the same code: v1.13.10 is commit 939b203, and the fork's base is two commits ahead of it, both an x/crypto bump. The functional SFTP tests cover this end to end and run unskipped on the Windows job, but only implicitly; a regression would surface as a confusing permission or not-found error from a doubled path, layers away from the cause. TestRemotePathIsPOSIXAbsolute states the requirement instead, which is platform-independent even though the bug is not: whatever is handed to the sftp client must satisfy path.IsAbs. Not verified on Windows locally. The table above is Go's Windows path logic executed on darwin, not a Windows test run. The Test (Windows) job covers ftests, so CI is the check that matters. * Stop one guest from wedging or killing the whole session The relay's functional suite has an intermittent failure where the host's client-left callback never fires. It was recorded as a timing flake; it is not. Two defects in MultiWriter, the fan-out that feeds every attached guest and the host's own terminal, both reachable in production. Write held writeMu across the writes. A guest that stops reading its SSH channel blocks there once the window fills, so Append and Remove waited on it. HandleSession removes its writer as it returns and emits the client-left event behind that, so the event was never sent. Write now snapshots the writers under the lock and writes with it released. Write also returned the first writer's error, which aborted the io.Copy feeding it from the pty. That ended the command and tore down the host, so a guest losing its connection at the wrong moment took the whole session with it, and the goroutine dump from the Windows job shows exactly that: the host gone while the test still waited. Errors also abandoned the rest of the slice, so a broken guest silenced everyone attached after it. A failing writer is now dropped and the fan-out continues; the producer always sees success. Remove counted down to index 1, so whichever writer sat at index 0 could never be removed. Latent, because index 0 is the host's own stdout. Two things found alongside, in the same I/O plumbing: contextReader handed the caller's buffer to a goroutine that outlives an abandoned Read, so a read completing after its Read call had returned wrote into a buffer io.Copy had already reused. It reads into its own buffer now and copies out only on the path that is still listening. It also closed the result channel on the way out, so a Read racing cancellation could return (0, nil), which io.Copy treats as neither data nor EOF and spins on. Its test asserted the abandoned read never runs, which is not true and cannot be: the read is unstoppable, only unheard. Calling t.Error from it panicked the binary once the subtest had finished, which is why ok github.com/owenthereal/upterm/io 90.473s was already red on master. testClientCallbacks never drained the guest's output, which is what manufactured the back-pressure that exposed all of this. Not fixed here: the fan-out is still serial, so a guest that has stopped reading holds everyone up until its write returns. That needs per-writer buffering and is tracked separately. * Accept native Windows paths from SFTP clients, and drop dead tilde handling Dropping the sftp fork restored the protocol's path handling, which cost something: the fork accepted native Windows paths, and upstream does not. SFTP paths are POSIX on every platform, so an absolute path on a Windows host travels as /C:/dir/file. A guest looking at a Windows machine reasonably types C:\dir\file instead, which the protocol reads as relative, so the server resolves it against the session's start directory and the request arrives as /C:/Users/me/C:/dir/file. Before this the file transfer failed with "The filename, directory name, or volume label syntax is incorrect", naming a path the guest never typed and with no hint that the fix is to spell it /C:/... instead. Windows filenames cannot contain ':', so a drive letter anywhere but the front can only have arrived that way, and the last one is the path the guest meant. That makes the repair exact rather than a guess. It is applied on Windows only: ':' is legal in a POSIX filename, so on a Linux host a guest asking for C:\foo really is naming a file under the start directory, and repairing it there would break a correct case. Tilde handling is removed rather than fixed. Tilde expansion is not part of SFTP, and the branch could never run: the library resolves every request against the start directory before a handler sees it, so the path is always absolute by then and never still begins with "~". Its tests passed by calling resolvePath directly, which is how unreachable code came to look like a working feature. Relative paths already resolve to the home directory, so "notes.txt" covers what "~/notes.txt" was reaching for. The repair is a pure function so it is covered on every platform rather than only on the Windows job, and testSFTPNativeWindowsPath exercises the whole path through a real sftp client where it can actually run. * Address review of the guest-isolation and Windows path fixes Five findings from review of #523, all in code this branch introduced. MultiWriter split one lock into two rather than dropping it. Releasing writeMu before the fan-out fixed Remove blocking behind a stuck guest, but it also let two producers into the same writer at once, which races any writer that is not concurrency safe and interleaves halves of two writes. writeMu now serializes the fan-out and a separate membersMu guards the slice, and membersMu is never held across a write, so Append and Remove still never wait on a guest. Append refuses a writer Remove could not match. Removal compares interface values, and comparing two of the same non-comparable dynamic type panics. Write removes the writers that failed it, so that panic would land in the host's pty copy and take the process down on an ordinary disconnect. This is not hypothetical here: TerminalQueryFilter holds slices and is only safe because it is attached by pointer. The stuck-writer test signalled from the calling goroutine, so Remove could run before the writer had blocked and the test could pass without exercising the bug at all. It now waits until the fan-out is inside the writer. Against master's MultiWriter it fails, as it should. undoubleDriveLetter read "/C:/dir/f:meta" as a drive. That is NTFS alternate-data-stream syntax for the "meta" stream of the file "f", and rewriting it silently retargeted the request at drive F:. A drive must now be a whole path component: "/X:" followed by "/" or by end of string. Windows exit statuses stay unsigned. GetExitCodeProcess returns a uint32 and the released windows/386 build has a 32-bit int, so a status with the high bit set -- an unhandled exception like 0xC0000005, or "exit -1" -- became negative, and exitCode read that as a process that never exited under its own control and reported a flat 1. ExitError.Code is a uint32 now, and the "did not exit" check is left to exec, which is the only source of the -1 it was written for. Each new test fails against the code it covers and passes after, except the 32-bit exit status, which cannot fail on a 64-bit build; there it documents the invariant that windows/386 depends on. One finding is declined. Codex reports that removing tilde handling breaks "~/dest" for Rename, Symlink and Link because Request.Target is left verbatim. It is not: pkg/sftp resolves Target through cleanPathWithBase for exactly those three methods (request.go:202, 206, 209), so a target arrives absolute and can no more begin with "~" than Filepath can. * Ask whether the writer value is comparable, not its type The check added for unremovable writers asked reflect.Type.Comparable, which answers a different question than the one Remove needs. A struct wrapping an io.Writer is a comparable type -- an interface field is one -- so the check passed it, and comparing two of them still panicked when the writers inside were funcs. That is not a contrived shape: a decorator holding an io.Writer is what TerminalQueryFilter is, and it is only safe because the host attaches it by pointer. reflect.Value.Comparable walks into the interface and answers for what is actually in it, and guarantees the comparison will not panic when it says yes. Because the value we validate is the value we store -- the interface holds a copy -- there is no window between the check and the comparison. Table-driven now, over both bare and wrapped forms of every non-comparable kind, plus the case that keeps the check from over-rejecting: the same wrapper around a pointer is attached, written to, and removed by identity. The two wrapped cases fail against the type-based check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix
cleanPathWithBaseto usefilepath.IsAbson the original path before converting to forward slashes. This correctly handles Windows paths likeC:\foowhichfilepath.IsAbsrecognizes butpath.IsAbs(afterToSlashconversion toC:/foo) does not.Problem
When using
WithStartDirectoryon Windows, absolute paths likeC:\Users\fooare incorrectly treated as relative paths because:filepath.ToSlash(filepath.Clean("C:\\Users\\foo"))→C:/Users/foopath.IsAbs("C:/Users/foo")→false(only checks for/prefix)Solution
Check
filepath.IsAbs(p)on the original path before theToSlashconversion.filepath.IsAbsis platform-aware and correctly identifies Windows absolute paths.Related
This fix resolves Windows SFTP path handling issues in owenthereal/upterm#440