Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/net/webdav: MOVE without an Overwrite header does not default to T #66059

Open
samcrang opened this issue Mar 1, 2024 · 0 comments
Open

x/net/webdav: MOVE without an Overwrite header does not default to T #66059

samcrang opened this issue Mar 1, 2024 · 0 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@samcrang
Copy link

samcrang commented Mar 1, 2024

Go version

go version go1.22.0 darwin/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN='/Users/sam/src/go/bin'
GOCACHE='/Users/sam/Library/Caches/go-build'
GOENV='/Users/sam/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/sam/src/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/sam/src/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/Cellar/go/1.22.0/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/Cellar/go/1.22.0/libexec/pkg/tool/darwin_amd64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/Users/sam/src/spike/go-webdav/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/c8/njvpx3cn1k7g0zjkf50rk6200000gn/T/go-build779784624=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

I have a simple WebDAV server:

package main

import (
	"net/http"

	"golang.org/x/net/webdav"
)

func main() {
	http.Handle("/", &webdav.Handler{
		FileSystem: webdav.Dir("."),
		LockSystem: webdav.NewMemLS(),
	})
	http.ListenAndServe(":8080", nil)
}

I'm using the builtin macOS WebDAV client (from Finder.app: Go > Network). I then connect to the WebDAV and try to move a file to a destination that already exists (ie overwriting the destination file with the source file).

What did you see happen?

$  pwd
/Volumes/127.0.0.1
$  touch foo bar
$  ls
bar     foo     go.mod  go.sum  main.go
$  mv foo bar
mv: rename foo to bar: Invalid argument

What did you expect to see?

It should overwrite the destination file with the source file and remove the source file. There should be no error.

I did some investigation and discovered that the macOS WebDAV client doesn't appear to set the Overwrite header when doing a MOVE operation. I grabbed the traffic with WireShark:

MOVE /foo HTTP/1.1
Host: 127.0.0.1:8080
Destination: http://127.0.0.1:8080/bar
Accept: */*
Content-Length: 0
Connection: keep-alive
User-Agent: WebDAVFS/3.0.0 (03008000) Darwin/23.3.0 (x86_64)

HTTP/1.1 412 Precondition Failed
Date: Fri, 01 Mar 2024 15:43:51 GMT
Content-Length: 19
Content-Type: text/plain; charset=utf-8

Precondition Failed

According to the WebDAV spec:

If the overwrite header is not included in a COPY or MOVE request, then the resource must treat the request as if it has an overwrite header of value "T".

However, the current implementation doesn't seem to do this. Instead it returns a 412 because it seems to assume the Overwrite header will always exist.

I think one solution would be to replace r.Header.Get("Overwrite") == "T" with r.Header.Get("Overwrite") != "F". I forked x/net and it seemed to work for me—if you're happy with this as a solution I'm happy to raise a PR.

However, it's also worth noting that the specification says:

If a COPY or MOVE is not performed due to the value of the Overwrite header, the method must fail with a 412 (Precondition Failed) status code. The server must do authorization checks before checking this or any conditional header.

I interpret this to mean "any value for Overwrite other that T or F should be rejected". I guess this means that the use of r.Header.Get("Overwrite") != "F" would also be technically incorrect too but it'd seem unlikely to cause any problems(?).

Also as an aside the "Overwite should default to T" behavior appears in the WebDAV modules for Apache and nginx.

@gopherbot gopherbot added this to the Unreleased milestone Mar 1, 2024
@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants