serve sftp: fix file corruption when a client resumes an upload

The SFTP serve write handler always opened files with O_TRUNC,
ignoring the flags requested in the SFTP OPEN packet. Some clients
(notably WinSCP's "Process in Background", which resumes an upload on
a second connection) re-open the partially written file without the
truncate flag and continue writing from the offset they had reached,
relying on the existing data being preserved. Forcing O_TRUNC zeroed
that prefix, so the start of the uploaded file ended up as a block of
zero bytes.

This fix respects the requested open flags instead so a resume open
without truncate keeps the already written data intact.

See: https://forum.rclone.org/t/rclone-serve-sftp-winscp-background-mode-uploading-causes-file-corruption/53841

cmd/serve/sftp/handler.go

@@ -38,7 +38,22 @@ func (v vfsHandler) Fileread(r *sftp.Request) (io.ReaderAt, error) {
 }
 
 func (v vfsHandler) Filewrite(r *sftp.Request) (io.WriterAt, error) {
-	file, err := v.OpenFile(r.Filepath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0777)
+	// Respect the flags requested in the SFTP OPEN packet
+	p := r.Pflags()
+	flags := os.O_WRONLY
+	if p.Append {
+		flags |= os.O_APPEND
+	}
+	if p.Creat {
+		flags |= os.O_CREATE
+	}
+	if p.Trunc {
+		flags |= os.O_TRUNC
+	}
+	if p.Excl {
+		flags |= os.O_EXCL
+	}
+	file, err := v.OpenFile(r.Filepath, flags, 0777)
 	if err != nil {
 		return nil, err
 	}

cmd/serve/sftp/handler_test.go

@@ -0,0 +1,153 @@
+// Test the SFTP serve handler against a real server and client.
+//
+// We skip tests on platforms with troublesome character mappings
+
+//go:build !windows && !darwin && !plan9
+
+package sftp
+
+import (
+	"context"
+	"io"
+	"os"
+	"strings"
+	"testing"
+
+	"github.com/pkg/sftp"
+	_ "github.com/rclone/rclone/backend/local"
+	"github.com/rclone/rclone/cmd/serve/proxy"
+	"github.com/rclone/rclone/fs"
+	"github.com/rclone/rclone/vfs/vfscommon"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/crypto/ssh"
+)
+
+// startTestServer starts an sftp server serving a temporary local directory
+// with the given VFS options and returns a connected sftp client.
+func startTestServer(t *testing.T, vfsOpt *vfscommon.Options) *sftp.Client {
+	ctx := context.Background()
+
+	f, err := fs.NewFs(ctx, t.TempDir())
+	require.NoError(t, err)
+
+	opt := Opt
+	opt.ListenAddr = testBindAddress
+	opt.User = testUser
+	opt.Pass = testPass
+
+	w, err := newServer(ctx, f, &opt, vfsOpt, &proxy.Opt)
+	require.NoError(t, err)
+	go func() {
+		_ = w.Serve()
+	}()
+	t.Cleanup(func() {
+		assert.NoError(t, w.Shutdown())
+	})
+
+	clientConfig := &ssh.ClientConfig{
+		User:            testUser,
+		Auth:            []ssh.AuthMethod{ssh.Password(testPass)},
+		HostKeyCallback: ssh.InsecureIgnoreHostKey(),
+	}
+	conn, err := ssh.Dial("tcp", w.Addr().String(), clientConfig)
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		_ = conn.Close()
+	})
+
+	client, err := sftp.NewClient(conn)
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		_ = client.Close()
+	})
+
+	return client
+}
+
+// Test that re-opening a file for write without the truncate flag preserves
+// the data already written, rather than zeroing the start of the file.
+//
+// This reproduces a corruption seen with WinSCP "Process in Background", which
+// resumes an upload on a second connection by re-opening the partial file
+// without SSH_FXF_TRUNC and writing from the offset it had reached.
+func TestFilewriteResumeNoTruncate(t *testing.T) {
+	vfsOpt := vfscommon.Opt
+	vfsOpt.CacheMode = vfscommon.CacheModeWrites
+	client := startTestServer(t, &vfsOpt)
+
+	const (
+		fileName = "file.bin"
+		total    = 1024 * 1024 // 1 MiB
+		split    = 700 * 1024  // where the upload is "backgrounded"
+	)
+
+	// Deterministic non-zero contents so a zeroed hole is easy to spot
+	contents := make([]byte, total)
+	for i := range contents {
+		contents[i] = byte(i%251 + 1)
+	}
+
+	// First connection: write the first part with truncate, as a fresh upload
+	f, err := client.OpenFile(fileName, os.O_WRONLY|os.O_CREATE|os.O_TRUNC)
+	require.NoError(t, err)
+	_, err = f.Write(contents[:split])
+	require.NoError(t, err)
+	require.NoError(t, f.Close())
+
+	// Resume: re-open WITHOUT truncate and write the remainder at its offset
+	f, err = client.OpenFile(fileName, os.O_WRONLY|os.O_CREATE)
+	require.NoError(t, err)
+	_, err = f.WriteAt(contents[split:], int64(split))
+	require.NoError(t, err)
+	require.NoError(t, f.Close())
+
+	// Read the file back and check it is intact - no zeroed prefix
+	rd, err := client.Open(fileName)
+	require.NoError(t, err)
+	got, err := io.ReadAll(rd)
+	require.NoError(t, err)
+	require.NoError(t, rd.Close())
+
+	require.Equal(t, total, len(got), "wrong file size")
+	assert.Equal(t, contents, got, "file contents corrupted on resumed upload")
+}
+
+// Test that opening a file for write with the truncate flag does truncate any
+// existing data, which is the normal overwrite case for most clients.
+func TestFilewriteTruncate(t *testing.T) {
+	vfsOpt := vfscommon.Opt
+	vfsOpt.CacheMode = vfscommon.CacheModeWrites
+	client := startTestServer(t, &vfsOpt)
+
+	const fileName = "file.bin"
+
+	// Write some initial long contents
+	require.NoError(t, writeFile(client, fileName, strings.Repeat("A", 1024)))
+
+	// Overwrite with shorter contents using truncate
+	const newContents = "hello"
+	require.NoError(t, writeFile(client, fileName, newContents))
+
+	rd, err := client.Open(fileName)
+	require.NoError(t, err)
+	got, err := io.ReadAll(rd)
+	require.NoError(t, err)
+	require.NoError(t, rd.Close())
+
+	assert.Equal(t, newContents, string(got))
+}
+
+// writeFile writes contents to fileName via the client truncating any existing
+// data, the way a normal upload does.
+func writeFile(client *sftp.Client, fileName, contents string) error {
+	f, err := client.OpenFile(fileName, os.O_WRONLY|os.O_CREATE|os.O_TRUNC)
+	if err != nil {
+		return err
+	}
+	if _, err := f.Write([]byte(contents)); err != nil {
+		_ = f.Close()
+		return err
+	}
+	return f.Close()
+}