Fix WebDAV/CalDAV/CardDAV bugs, drop username from URLs, harden concurrency

- Root handler now only serves the welcome page for GET/HEAD; all other methods (e.g. OPTIONS, PROPFIND) return 405 with an Allow header instead of always returning 200, fixing client capability probes and PROPFIND misbehavior. - Mount /files/ properly and cache one xwebdav.Handler per authenticated user so its LockSystem persists across requests instead of being recreated per-request (which broke LOCK/UNLOCK). - Remove the username segment from all DAV URLs (/cal/, /card/, /files/ are now identical for every account; the acting user is always resolved via Basic Auth, never the path). - Reintroduce a fixed literal "home" path segment (/cal/home/, /card/home/) to preserve the URL segment depth that go-webdav's caldav/carddav server relies on to classify resources (principal vs. home-set vs. collection vs. object). Removing the username had collapsed this depth, silently misclassifying requests and returning empty <multistatus> responses (DAVx5 "no resources found"). - Replace the store's single global mutex with per-user sharded locks so different users' requests no longer serialize against each other. - Add auth.NewContext test helper, WebDAV handler tests (per-user isolation, lock persistence across requests), and a concurrent multi-user store test. - Update README and copilot-instructions to document the new URL scheme and the go-webdav path-depth classification quirk. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2026-08-18 12:49:57 +02:00
parent 7a11b5bbbf
commit b4644bc590
11 changed files with 465 additions and 100 deletions
@@ -5,15 +5,28 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
+	"sync"

 	"github.com/yourusername/caldav-server/internal/auth"
 	"github.com/yourusername/caldav-server/internal/config"
 	xwebdav "golang.org/x/net/webdav"
 )

-// NewHandler returns an http.Handler that provides standard WebDAV file access
-// per-user under dataDir/files/<username>/.
+// NewHandler returns an http.Handler that provides standard WebDAV file access,
+// mounted at the fixed URL /files/ for every user and rooted at
+// dataDir/files/<username>/ on disk. The URL is the same for all users —
+// which user's directory is served is resolved from the Basic Auth identity
+// in the request context, not from the URL.
+//
+// A dedicated xwebdav.Handler (with its own persistent LockSystem) is created
+// once per user and cached, so LOCK/UNLOCK state survives across requests
+// instead of being reset on every call.
 func NewHandler(cfg *config.Config, dataDir string, logger *slog.Logger) http.Handler {
+	var (
+		mu       sync.Mutex
+		handlers = make(map[string]http.Handler)
+	)
+
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		p := auth.FromContext(r.Context())
 		if p == nil {
@@ -21,29 +34,39 @@ func NewHandler(cfg *config.Config, dataDir string, logger *slog.Logger) http.Ha
 			return
 		}

-		userDir := filepath.Join(dataDir, "files", p.Username)
-		logger.Debug(userDir)
-		if err := os.MkdirAll(userDir, 0o755); err != nil {
-			logger.Error("creating user WebDAV dir", "user", p.Username, "error", err)
-			http.Error(w, "internal server error", http.StatusInternalServerError)
-			return
-		}
+		mu.Lock()
+		h, ok := handlers[p.Username]
+		if !ok {
+			username := p.Username
+			userDir := filepath.Join(dataDir, "files", username)
+			if err := os.MkdirAll(userDir, 0o755); err != nil {
+				mu.Unlock()
+				logger.Error("creating user WebDAV dir", "user", username, "error", err)
+				http.Error(w, "internal server error", http.StatusInternalServerError)
+				return
+			}

-		// Each user gets their own isolated WebDAV handler so paths don't bleed.
-		h := &xwebdav.Handler{
-			FileSystem: xwebdav.Dir(userDir),
-			LockSystem: xwebdav.NewMemLS(),
-			Logger: func(r *http.Request, err error) {
-				if err != nil {
-					logger.Warn("WebDAV error",
-						"user", p.Username,
-						"method", r.Method,
-						"path", r.URL.Path,
-						"error", err)
-				}
-			},
-			Prefix: "/", //fmt.Sprintf("/files/%s", p.Username),
+			// Each user gets their own isolated WebDAV handler (and lock
+			// system) so paths and locks don't bleed between users, even
+			// though they all share the same "/files/" URL.
+			h = &xwebdav.Handler{
+				FileSystem: xwebdav.Dir(userDir),
+				LockSystem: xwebdav.NewMemLS(),
+				Logger: func(r *http.Request, err error) {
+					if err != nil {
+						logger.Warn("WebDAV error",
+							"user", username,
+							"method", r.Method,
+							"path", r.URL.Path,
+							"error", err)
+					}
+				},
+				Prefix: "/files",
+			}
+			handlers[username] = h
 		}
+		mu.Unlock()
+
 		h.ServeHTTP(w, r)
 	})
 }
@@ -0,0 +1,100 @@
+package filewebdav_test
+
+import (
+	"io"
+	"log/slog"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"strings"
+	"testing"
+
+	"github.com/yourusername/caldav-server/internal/auth"
+	filewebdav "github.com/yourusername/caldav-server/internal/webdav"
+)
+
+func testLogger() *slog.Logger {
+	return slog.New(slog.NewTextHandler(io.Discard, nil))
+}
+
+func doAs(t *testing.T, h http.Handler, user, method, path string, body string) *httptest.ResponseRecorder {
+	t.Helper()
+	req := httptest.NewRequest(method, path, strings.NewReader(body))
+	ctx := auth.NewContext(req.Context(), &auth.Principal{Username: user})
+	req = req.WithContext(ctx)
+	rec := httptest.NewRecorder()
+	h.ServeHTTP(rec, req)
+	return rec
+}
+
+// TestPerUserIsolationAndPrefix verifies that all users share the same
+// "/files/" URL, but each is served from (and can only see) their own
+// directory on disk, resolved from the Basic Auth identity.
+func TestPerUserIsolationAndPrefix(t *testing.T) {
+	dir := t.TempDir()
+	h := filewebdav.NewHandler(nil, dir, testLogger())
+
+	putRec := doAs(t, h, "alice", http.MethodPut, "/files/note.txt", "hello alice")
+	if putRec.Code != http.StatusCreated && putRec.Code != http.StatusNoContent {
+		t.Fatalf("PUT as alice: unexpected status %d: %s", putRec.Code, putRec.Body.String())
+	}
+
+	getRec := doAs(t, h, "alice", http.MethodGet, "/files/note.txt", "")
+	if getRec.Code != http.StatusOK {
+		t.Fatalf("GET as alice: unexpected status %d", getRec.Code)
+	}
+	if getRec.Body.String() != "hello alice" {
+		t.Fatalf("unexpected body: %q", getRec.Body.String())
+	}
+
+	// bob hits the exact same URL, but must not see alice's file — his own
+	// directory on disk is empty.
+	bobRec := doAs(t, h, "bob", http.MethodGet, "/files/note.txt", "")
+	if bobRec.Code != http.StatusNotFound {
+		t.Fatalf("expected bob to get 404 for alice's file, got %d", bobRec.Code)
+	}
+
+	// Confirm the file physically landed under dataDir/files/alice/, not
+	// nested under an extra files/files/... path.
+	if _, err := os.Stat(dir + "/files/alice/note.txt"); err != nil {
+		t.Fatalf("expected file at dataDir/files/alice/note.txt: %v", err)
+	}
+}
+
+// TestLockPersistsAcrossRequests ensures the LockSystem used by the handler
+// is not recreated (and thus reset) on every request.
+func TestLockPersistsAcrossRequests(t *testing.T) {
+	dir := t.TempDir()
+	h := filewebdav.NewHandler(nil, dir, testLogger())
+
+	// Create the file first.
+	doAs(t, h, "alice", http.MethodPut, "/files/locked.txt", "v1")
+
+	lockBody := `<?xml version="1.0" encoding="utf-8" ?>
+<D:lockinfo xmlns:D="DAV:">
+  <D:lockscope><D:exclusive/></D:lockscope>
+  <D:locktype><D:write/></D:locktype>
+  <D:owner><D:href>test</D:href></D:owner>
+</D:lockinfo>`
+
+	lockRec := doAs(t, h, "alice", "LOCK", "/files/locked.txt", lockBody)
+	if lockRec.Code != http.StatusOK {
+		t.Fatalf("LOCK: unexpected status %d: %s", lockRec.Code, lockRec.Body.String())
+	}
+	locktoken := lockRec.Header().Get("Lock-Token")
+	if locktoken == "" {
+		t.Fatal("expected Lock-Token header in LOCK response")
+	}
+
+	// A second, unrelated request must still see the lock as active,
+	// proving the LockSystem instance was reused rather than reset.
+	req := httptest.NewRequest(http.MethodPut, "/files/locked.txt", strings.NewReader("v2 without token"))
+	ctx := auth.NewContext(req.Context(), &auth.Principal{Username: "alice"})
+	req = req.WithContext(ctx)
+	rec := httptest.NewRecorder()
+	h.ServeHTTP(rec, req)
+
+	if rec.Code != http.StatusLocked {
+		t.Fatalf("expected 423 Locked for PUT without lock token, got %d: %s", rec.Code, rec.Body.String())
+	}
+}