From 39487c5ebf3de97c7aafbaf146a86771846e072e Mon Sep 17 00:00:00 2001 From: Sanjay Santhanam <51058514+Sanjays2402@users.noreply.github.com> Date: Sat, 4 Jul 2026 01:57:27 -0700 Subject: [PATCH] serve webdav: fix MOVE overwrite failing without Overwrite header Per RFC 4918 section 10.6, when the Overwrite header is omitted from a COPY or MOVE request the resource MUST treat the request as if Overwrite: T had been sent. The upstream golang.org/x/net/webdav library mishandles this for MOVE by checking == "T" instead of != "F", so an absent header is treated as Overwrite: F and the request fails with 412 Precondition Failed. Normalise the header to T in the rclone WebDAV server before delegating to the upstream handler when the client did not send one. This restores RFC-compliant default behaviour and can be removed once the upstream fix in golang/go#66059 lands and the golang.org/x/net dependency is bumped. Fixes #9496 (cherry picked from commit cfb9a10a3d684e544abfbf03d440b739e3b30dc0) --- cmd/serve/webdav/webdav.go | 6 +++ cmd/serve/webdav/webdav_test.go | 86 +++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+) diff --git a/cmd/serve/webdav/webdav.go b/cmd/serve/webdav/webdav.go index a674a6955..b5079f115 100644 --- a/cmd/serve/webdav/webdav.go +++ b/cmd/serve/webdav/webdav.go @@ -391,6 +391,12 @@ func (w *WebDAV) ServeHTTP(rw http.ResponseWriter, r *http.Request) { w.serveDir(rw, r, remote) return } + // Work around bug in x/net/webdav which handles Overwrite incorrectly + // See: https://github.com/golang/go/issues/66059 + // Remove when the above bug is fixed. + if (r.Method == "COPY" || r.Method == "MOVE") && r.Header.Get("Overwrite") == "" { + r.Header.Set("Overwrite", "T") + } // Add URL Prefix back to path since webdavhandler needs to // return absolute references. r.URL.Path = w.opt.HTTP.BaseURL + r.URL.Path diff --git a/cmd/serve/webdav/webdav_test.go b/cmd/serve/webdav/webdav_test.go index 4066f6b76..f7bb29f50 100644 --- a/cmd/serve/webdav/webdav_test.go +++ b/cmd/serve/webdav/webdav_test.go @@ -271,3 +271,89 @@ func TestRc(t *testing.T) { "vfs_cache_mode": "off", }) } + +// startWritableServer starts a webdav server backed by a fresh temp +// directory and returns the server URL. It is used by the Overwrite tests +// which need to exercise mutating verbs such as MKCOL and MOVE. +func startWritableServer(t *testing.T) string { + t.Helper() + + f, err := fs.NewFs(context.Background(), t.TempDir()) + require.NoError(t, err) + + opt := Opt + opt.HTTP.ListenAddr = []string{testBindAddress} + + w, err := newWebDAV(context.Background(), f, &opt, &vfscommon.Opt, &proxy.Opt) + require.NoError(t, err) + go func() { + require.NoError(t, w.Serve()) + }() + t.Cleanup(func() { + assert.NoError(t, w.Shutdown()) + }) + + return w.server.URLs()[0] +} + +func mkcol(t *testing.T, baseURL, path string) { + t.Helper() + req, err := http.NewRequest("MKCOL", baseURL+path, nil) + require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + _ = resp.Body.Close() + require.Equal(t, http.StatusCreated, resp.StatusCode, "MKCOL %s", path) +} + +// TestMoveDefaultsToOverwrite is a regression test for +// https://github.com/rclone/rclone/issues/9496 +// +// RFC 4918 section 10.6 requires that when the Overwrite header is omitted +// from a COPY or MOVE request, the resource MUST behave as if Overwrite: T +// had been sent. The upstream golang.org/x/net/webdav library mis-handles +// the MOVE case (see https://github.com/golang/go/issues/66059), so rclone +// normalises the header before delegating so the default matches the RFC. +func TestMoveDefaultsToOverwrite(t *testing.T) { + testURL := startWritableServer(t) + + mkcol(t, testURL, "dir1") + mkcol(t, testURL, "dir2") + + // MOVE without Overwrite header: per RFC 4918 the default is T, so the + // existing destination must be replaced and the server must return 2xx. + req, err := http.NewRequest("MOVE", testURL+"dir2", nil) + require.NoError(t, err) + req.Header.Set("Destination", testURL+"dir1") + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + + assert.NotEqual(t, http.StatusPreconditionFailed, resp.StatusCode, + "MOVE without Overwrite header must not return 412; RFC 4918 default is Overwrite: T") + assert.True(t, resp.StatusCode >= 200 && resp.StatusCode < 300, + "expected 2xx, got %d", resp.StatusCode) +} + +// TestMoveOverwriteFalseStillRejects ensures the rclone normalisation only +// fills in a missing Overwrite header and never overrides an explicit +// Overwrite: F sent by the client. +func TestMoveOverwriteFalseStillRejects(t *testing.T) { + testURL := startWritableServer(t) + + mkcol(t, testURL, "dir1") + mkcol(t, testURL, "dir2") + + req, err := http.NewRequest("MOVE", testURL+"dir2", nil) + require.NoError(t, err) + req.Header.Set("Destination", testURL+"dir1") + req.Header.Set("Overwrite", "F") + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + + assert.Equal(t, http.StatusPreconditionFailed, resp.StatusCode, + "MOVE with explicit Overwrite: F must still return 412 when destination exists") +}