From b4644bc590361841d3dadac2a3fb066e2b0fb837 Mon Sep 17 00:00:00 2001 From: arnef Date: Tue, 18 Aug 2026 12:49:57 +0200 Subject: [PATCH] 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 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> --- .github/copilot-instructions.md | 108 ++++++++++++++++++++++++++++++++ README.md | 22 +++++-- cmd/server/main.go | 27 +++++--- internal/auth/middleware.go | 7 +++ internal/caldav/backend.go | 47 +++++++++----- internal/carddav/backend.go | 44 ++++++++----- internal/config/config.go | 20 +++--- internal/store/store.go | 67 ++++++++++++++------ internal/store/store_test.go | 54 ++++++++++++++++ internal/webdav/handler.go | 69 +++++++++++++------- internal/webdav/handler_test.go | 100 +++++++++++++++++++++++++++++ 11 files changed, 465 insertions(+), 100 deletions(-) create mode 100644 .github/copilot-instructions.md create mode 100644 internal/webdav/handler_test.go diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 0000000..71b866f --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,108 @@ +# Copilot Instructions for nidus + +A self-hosted CalDAV, CardDAV, and WebDAV server written in Go, backed by a +filesystem store. HTTP Basic Auth (bcrypt) with per-user isolated collections. + +## Build, test, lint + +```bash +make build # go build -o bin/davserver ./cmd/server +make run # build + ./bin/davserver -config config.yaml +make test # go test ./... -v -race +make lint # golangci-lint run ./... +make tidy # go mod tidy +go test ./internal/store/ -run TestStoreRoundTrip -v # single test +``` + +There is currently only one test file: `internal/store/store_test.go`. + +## Architecture + +- `cmd/server/main.go` — entrypoint. Loads config, builds the `slog.Logger`, + constructs the `store.Store`, pre-creates each user's configured + calendars/address-books as collections, wires up `auth.Middleware`, and + builds the `http.ServeMux` (`buildMux`). Routes: `/cal/`, `/card/`, + `/files/`, `/.well-known/{caldav,carddav}`, `/healthz` (unauthenticated), + and `/` (welcome page on GET/HEAD only; any other method — e.g. a WebDAV + client pointed at the wrong URL — gets `405` instead of a misleading + `200`). +- `internal/config` — YAML config loading (`config.Load`), defaults + (`applyDefaults`), and validation (`validate`). `Config.Users` is a + `map[string]UserConfig` keyed by username; each user has a bcrypt + `Password`, `Calendars`, and `AddressBooks` lists that seed collection + names. +- `internal/auth` — HTTP Basic Auth middleware (`auth.Middleware.Wrap`). + Validates credentials against `cfg.Users` via bcrypt, then stores a + `*Principal{Username, DisplayName, Email}` in the request context. + Downstream code retrieves it with `auth.FromContext(ctx)` — every backend + method needs this and returns `webdav.NewHTTPError(http.StatusUnauthorized, ...)` + if it's nil. `auth.NewContext(ctx, p)` is the test-only inverse, used to + build authenticated contexts without a real Basic Auth handshake. +- `internal/store` — the single source of truth for all persisted data. + A thin filesystem KV abstraction: `///`. + All collection/object names pass through `sanitize()` (via + `filepath.Base` + strip `..`) to prevent path traversal — preserve this + when adding new store methods. Writes use temp-file + rename for atomicity + (`PutObject`). Locking is sharded per-user (`lockFor(user)`, a + `map[string]*sync.RWMutex` guarded by its own mutex) rather than one + global lock, so different users' requests don't serialize against each + other. +- `internal/caldav` and `internal/carddav` — implement the + `caldav.Backend`/`carddav.Backend` interfaces from `github.com/emersion/go-webdav` + on top of `store.Store`. Calendars are stored as collections prefixed + `cal-` and address books as `card-` (see `ListCalendars`, + `parseCalPath`). **The URL scheme has no username segment**, but DOES + have a fixed literal `home` segment: `/cal/` (principal), `/cal/home/` + (calendar-home-set), `/cal/home//` (calendar), + `/cal/home//` (object) — and equivalently + `/card/`, `/card/home/`, `/card/home//`, + `/card/home//` for carddav. These are identical for + every user; the acting user always comes from `auth.FromContext(ctx)`, + never from the path. **The `home` segment is load-bearing, not + cosmetic**: go-webdav's `caldav`/`carddav` server (in the + `github.com/emersion/go-webdav` dependency, not our code) classifies + each request purely by counting URL path segments relative to the + handler's `Prefix` (which we leave `""`) — 1 segment = user principal, 2 + = home-set, 3 = calendar/address book, 4 = object. If the segment counts + don't line up (e.g. removing `home` would make `/cal/` and + `/cal//` collapse to 1 and 2 segments, misclassifying the + calendar collection itself as the home-set), PROPFIND requests silently + return an empty `` (200/207, zero `` elements) — + no error, just nothing found, which breaks client auto-discovery (e.g. + DAVx5 reporting "no resources found"). Keep this in mind when touching + `parseCalPath`/`parseObjPath`/`parseBookPath`, + `calHomePath`/`cardHomePath`, or `CurrentUserPrincipal` — always + preserve the exact segment depth at each level. Query methods + (`QueryCalendarObjects`) currently list all objects and filter in-memory + via `caldav.Filter` — fine for small collections, not optimized for + scale. +- `internal/webdav` — plain-file WebDAV via `golang.org/x/net/webdav`, + mounted at the single fixed URL `/files/` for all users (no username in + the path either). `NewHandler` caches one `*xwebdav.Handler` per + authenticated username (keyed off `auth.FromContext`), each rooted at + `/files//` on disk with its own persistent + `LockSystem` — the handler (and its lock table) must be created once and + reused, not per-request, or LOCK/UNLOCK state resets on every call. +- `tools/hashpwd` — standalone CLI (`go run ./tools/hashpwd `) to + generate bcrypt hashes for `config.yaml`. + +## Conventions + +- **No username in any DAV URL** (`/cal/`, `/card/`, `/files/` are the same + for every account) — the acting user is always resolved from the Basic + Auth identity (`auth.FromContext`), never parsed out of the request path. + Don't reintroduce a `` path segment when adding routes/paths. +- Path parsing in the CalDAV/CardDAV backends assumes fixed URL segment + positions (e.g. `cal/home//`) split on `/` — see + `parseCalPath`/`parseObjPath`/`parseBookPath`. The `home` segment must + stay exactly one fixed literal segment (see note above on go-webdav's + segment-count-based resource classification) — don't remove it or add/ + remove segments elsewhere without re-checking all four resource-type + depths still line up. New path-based operations should follow the same + segment-index approach for consistency. +- Store errors are sentinel values (`store.ErrNotFound`, `store.ErrConflict`) + checked with `errors.Is`/direct comparison; backends translate them into + `webdav.NewHTTPError` with the appropriate HTTP status. +- Logging uses `log/slog` structured fields (e.g. `logger.Warn("...", "user", u, "error", err)`), passed down explicitly to every constructor (`NewBackend`, `NewHandler`, `NewMiddleware`) rather than a global logger. +- Config module path is `github.com/yourusername/caldav-server` (go.mod name + predates the `nidus` repo rename) — import paths still use this, not `nidus`. diff --git a/README.md b/README.md index d2ce9aa..150c9b4 100644 --- a/README.md +++ b/README.md @@ -87,7 +87,7 @@ docker run -p 8080:8080 \ - **Server**: `http://yourserver:8080` - **Username**: `alice` - **Password**: your plaintext password -3. The app will auto-discover calendars at `/cal/alice/`. +3. The app will auto-discover calendars at `/cal/`. Same flow for **CardDAV** with Contacts app. @@ -110,13 +110,23 @@ Use the GNOME Online Accounts panel: |------|-------------| | `/.well-known/caldav` | Redirects to `/cal/` | | `/.well-known/carddav` | Redirects to `/card/` | -| `/cal//` | CalDAV home | -| `/cal///` | Calendar collection | -| `/card//` | CardDAV home | -| `/card///` | Address book collection | -| `/files//` | WebDAV file storage | +| `/cal/` | CalDAV principal (same URL for every user; resolved via Basic Auth) | +| `/cal/home/` | Calendar home-set (lists the user's calendars) | +| `/cal/home//` | Calendar collection | +| `/card/` | CardDAV principal (same URL for every user; resolved via Basic Auth) | +| `/card/home/` | Address book home-set (lists the user's address books) | +| `/card/home//` | Address book collection | +| `/files/` | WebDAV file storage (same URL for every user; resolved via Basic Auth) | | `/healthz` | Health check (unauthenticated) | +> **Note:** the `home` segment is a fixed literal (not a username or real +> resource) — it exists only to give the calendar/address-book home-set the +> path depth that the underlying CalDAV/CardDAV library expects when +> classifying resources by URL. Clients should never need to construct +> these URLs by hand; they're discovered automatically via +> `.well-known` + `current-user-principal` + `calendar-home-set` / +> `addressbook-home-set` properties. + --- ## TLS / Reverse proxy diff --git a/cmd/server/main.go b/cmd/server/main.go index 2156059..8776638 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -139,16 +139,25 @@ func buildMux( // CalDAV, CardDAV, and file WebDAV — all behind Basic Auth mux.Handle("/cal/", authMw.Wrap(calHandler)) mux.Handle("/card/", authMw.Wrap(cardHandler)) - // mux.Handle("/files/", authMw.Wrap(fileHandler)) + mux.Handle("/files/", authMw.Wrap(fileHandler)) - // Root — simple HTML welcome page + // Root — simple HTML welcome page (unauthenticated) mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/" && r.Method == "GET" { - w.Header().Set("Content-Type", "text/html; charset=utf-8") - fmt.Fprintf(w, welcomePage, cfg.Server.BaseURL, cfg.Server.BaseURL, cfg.Server.BaseURL, cfg.Server.BaseURL, cfg.Server.BaseURL) + if r.URL.Path != "/" { + http.NotFound(w, r) return } - authMw.Wrap(fileHandler).ServeHTTP(w, r) + // Only plain GET/HEAD get the welcome page. Any other method + // (PROPFIND, LOCK, PUT, ...) hitting "/" means a client is pointed + // at the wrong URL — reject it explicitly instead of returning a + // misleading 200 OK, which breaks WebDAV clients expecting 207. + if r.Method != http.MethodGet && r.Method != http.MethodHead { + w.Header().Set("Allow", "GET, HEAD") + http.Error(w, "not a WebDAV collection; use /cal/, /card/ or /files/", http.StatusMethodNotAllowed) + return + } + w.Header().Set("Content-Type", "text/html; charset=utf-8") + fmt.Fprintf(w, welcomePage, cfg.Server.BaseURL, cfg.Server.BaseURL, cfg.Server.BaseURL, cfg.Server.BaseURL, cfg.Server.BaseURL) }) return mux @@ -218,9 +227,9 @@ const welcomePage = `

This server provides CalDAV, CardDAV, and WebDAV access.

Endpoints

    -
  • CalDAV — %s/cal/<username>/
    • -
  • CardDAV — %s/card/<username>/
    • -
  • WebDAV files — %s/files/<username>/
    • +
  • CalDAV — %s/cal/
    • +
  • CardDAV — %s/card/
    • +
  • WebDAV files — %s/files/

Auto-discovery

    diff --git a/internal/auth/middleware.go b/internal/auth/middleware.go index 7c8e41d..858e8d5 100644 --- a/internal/auth/middleware.go +++ b/internal/auth/middleware.go @@ -95,6 +95,13 @@ func FromContext(ctx context.Context) *Principal { return p } +// NewContext returns a copy of ctx carrying p, retrievable via FromContext. +// This is primarily useful for tests of downstream packages that need an +// authenticated context without going through the Basic Auth handshake. +func NewContext(ctx context.Context, p *Principal) context.Context { + return context.WithValue(ctx, userContextKey, p) +} + var errUnauthorized = &authError{msg: "invalid credentials"} type authError struct{ msg string } diff --git a/internal/caldav/backend.go b/internal/caldav/backend.go index e0b8f3d..320b935 100644 --- a/internal/caldav/backend.go +++ b/internal/caldav/backend.go @@ -38,19 +38,23 @@ func NewHandler(cfg *config.Config, st *store.Store, logger *slog.Logger) http.H // -------- caldav.Backend interface -------- func (b *Backend) CurrentUserPrincipal(ctx context.Context) (string, error) { - p := auth.FromContext(ctx) - if p == nil { + if auth.FromContext(ctx) == nil { return "", webdav.NewHTTPError(http.StatusUnauthorized, fmt.Errorf("not authenticated")) } - return principalPath(p.Username), nil + // Must resolve to a path served by this same handler (i.e. under /cal/) + // at exactly one path segment of depth, since go-webdav's caldav server + // classifies resources purely by path depth relative to Handler.Prefix: + // depth 1 = principal, depth 2 = home-set, depth 3 = calendar, depth 4 = + // calendar object. A /principals// path would never be reached + // (nothing is mounted there) and would break discovery. + return calPrincipalPath(), nil } func (b *Backend) CalendarHomeSetPath(ctx context.Context) (string, error) { - p := auth.FromContext(ctx) - if p == nil { + if auth.FromContext(ctx) == nil { return "", webdav.NewHTTPError(http.StatusUnauthorized, fmt.Errorf("not authenticated")) } - return calHomePath(p.Username), nil + return calHomePath(), nil } func (b *Backend) ListCalendars(ctx context.Context) ([]caldav.Calendar, error) { @@ -132,7 +136,7 @@ func (b *Backend) ListCalendarObjects(ctx context.Context, calPath string, req * if err != nil { continue } - obj, err := b.decodeObject(calObjectPath(user, calName, id), data) + obj, err := b.decodeObject(calObjectPath(calName, id), data) if err != nil { b.logger.Warn("decoding calendar object", "id", id, "error", err) continue @@ -204,7 +208,7 @@ func (b *Backend) DeleteCalendarObject(ctx context.Context, objPath string) erro func (b *Backend) calendarMeta(user, name string) caldav.Calendar { return caldav.Calendar{ - Path: calHomePath(user) + name + "/", + Path: calHomePath() + name + "/", Name: name, Description: fmt.Sprintf("%s's %s calendar", user, name), SupportedComponentSet: []string{"VEVENT", "VTODO", "VJOURNAL"}, @@ -236,7 +240,7 @@ func (b *Backend) parseCalPath(ctx context.Context, calPath string) (user, calNa } user = p.Username parts := strings.Split(strings.Trim(calPath, "/"), "/") - // expected: cal/// + // expected: cal/home// if len(parts) < 3 { return "", "", webdav.NewHTTPError(http.StatusBadRequest, fmt.Errorf("invalid calendar path")) } @@ -251,7 +255,7 @@ func (b *Backend) parseObjPath(ctx context.Context, objPath string) (user, calNa } user = p.Username parts := strings.Split(strings.Trim(objPath, "/"), "/") - // expected: cal/// + // expected: cal/home// if len(parts) < 4 { return "", "", "", webdav.NewHTTPError(http.StatusBadRequest, fmt.Errorf("invalid object path")) } @@ -260,16 +264,27 @@ func (b *Backend) parseObjPath(ctx context.Context, objPath string) (user, calNa return user, calName, objID, nil } -func principalPath(user string) string { - return fmt.Sprintf("/principals/%s/", user) +// calPrincipalPath, calHomePath, and calObjectPath are the same for every +// user: authorization is resolved from the Basic Auth identity, not from +// the URL, so no username segment is needed in the path. +// +// The fixed "home" segment (in place of a username) is required, not +// cosmetic: go-webdav's caldav server classifies a request purely by how +// many path segments it has relative to the handler's mount point — 1 +// segment is treated as the principal, 2 as the calendar-home-set, 3 as a +// calendar, 4 as a calendar object. Removing that segment entirely would +// make the home-set and calendar paths misclassified as principal/home-set +// respectively, breaking discovery (empty PROPFIND responses). +func calPrincipalPath() string { + return "/cal/" } -func calHomePath(user string) string { - return fmt.Sprintf("/cal/%s/", user) +func calHomePath() string { + return "/cal/home/" } -func calObjectPath(user, calName, objID string) string { - return fmt.Sprintf("/cal/%s/%s/%s", user, calName, objID) +func calObjectPath(calName, objID string) string { + return fmt.Sprintf("/cal/home/%s/%s", calName, objID) } func hashBytes(data []byte) uint64 { diff --git a/internal/carddav/backend.go b/internal/carddav/backend.go index cd0f81d..c310f60 100644 --- a/internal/carddav/backend.go +++ b/internal/carddav/backend.go @@ -38,19 +38,19 @@ func NewHandler(cfg *config.Config, st *store.Store, logger *slog.Logger) http.H // -------- carddav.Backend interface -------- func (b *Backend) CurrentUserPrincipal(ctx context.Context) (string, error) { - p := auth.FromContext(ctx) - if p == nil { + if auth.FromContext(ctx) == nil { return "", webdav.NewHTTPError(http.StatusUnauthorized, fmt.Errorf("not authenticated")) } - return principalPath(p.Username), nil + // Must resolve to a path served by this same handler (i.e. under + // /card/) at exactly one path segment of depth — see cardPrincipalPath. + return cardPrincipalPath(), nil } func (b *Backend) AddressBookHomeSetPath(ctx context.Context) (string, error) { - p := auth.FromContext(ctx) - if p == nil { + if auth.FromContext(ctx) == nil { return "", webdav.NewHTTPError(http.StatusUnauthorized, fmt.Errorf("not authenticated")) } - return cardHomePath(p.Username), nil + return cardHomePath(), nil } func (b *Backend) ListAddressBooks(ctx context.Context) ([]carddav.AddressBook, error) { @@ -132,7 +132,7 @@ func (b *Backend) ListAddressObjects(ctx context.Context, bookPath string, req * if err != nil { continue } - obj, err := b.decodeObject(cardObjectPath(user, bookName, id), data) + obj, err := b.decodeObject(cardObjectPath(bookName, id), data) if err != nil { b.logger.Warn("decoding vcard object", "id", id, "error", err) continue @@ -202,7 +202,7 @@ func (b *Backend) DeleteAddressObject(ctx context.Context, objPath string) error func (b *Backend) bookMeta(user, name string) carddav.AddressBook { return carddav.AddressBook{ - Path: cardHomePath(user) + name + "/", + Path: cardHomePath() + name + "/", Name: name, Description: fmt.Sprintf("%s's %s address book", user, name), MaxResourceSize: 10 * 1024 * 1024, @@ -234,7 +234,7 @@ func (b *Backend) parseBookPath(ctx context.Context, bookPath string) (user, boo } user = p.Username parts := strings.Split(strings.Trim(bookPath, "/"), "/") - // expected: card/// + // expected: card/home// if len(parts) < 3 { return "", "", webdav.NewHTTPError(http.StatusBadRequest, fmt.Errorf("invalid address book path")) } @@ -249,7 +249,7 @@ func (b *Backend) parseObjPath(ctx context.Context, objPath string) (user, bookN } user = p.Username parts := strings.Split(strings.Trim(objPath, "/"), "/") - // expected: card/// + // expected: card/home// if len(parts) < 4 { return "", "", "", webdav.NewHTTPError(http.StatusBadRequest, fmt.Errorf("invalid object path")) } @@ -258,16 +258,28 @@ func (b *Backend) parseObjPath(ctx context.Context, objPath string) (user, bookN return user, bookName, objID, nil } -func principalPath(user string) string { - return fmt.Sprintf("/principals/%s/", user) +// cardPrincipalPath, cardHomePath, and cardObjectPath are the same for +// every user: authorization is resolved from the Basic Auth identity, not +// from the URL, so no username segment is needed in the path. +// +// The fixed "home" segment (in place of a username) is required, not +// cosmetic: go-webdav's carddav server classifies a request purely by how +// many path segments it has relative to the handler's mount point — 1 +// segment is treated as the principal, 2 as the addressbook-home-set, 3 as +// an address book, 4 as an address object. Removing that segment entirely +// would make the home-set and address-book paths misclassified as +// principal/home-set respectively, breaking discovery (empty PROPFIND +// responses). +func cardPrincipalPath() string { + return "/card/" } -func cardHomePath(user string) string { - return fmt.Sprintf("/card/%s/", user) +func cardHomePath() string { + return "/card/home/" } -func cardObjectPath(user, bookName, objID string) string { - return fmt.Sprintf("/card/%s/%s/%s", user, bookName, objID) +func cardObjectPath(bookName, objID string) string { + return fmt.Sprintf("/card/home/%s/%s", bookName, objID) } func hashBytes(data []byte) uint64 { diff --git a/internal/config/config.go b/internal/config/config.go index d343c91..26ee678 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -9,12 +9,12 @@ import ( // Config is the top-level server configuration. type Config struct { - Server ServerConfig `yaml:"server"` - Auth AuthConfig `yaml:"auth"` - Storage StorageConfig `yaml:"storage"` - Users map[string]UserConfig `yaml:"users"` - TLS TLSConfig `yaml:"tls"` - Logging LoggingConfig `yaml:"logging"` + Server ServerConfig `yaml:"server"` + Auth AuthConfig `yaml:"auth"` + Storage StorageConfig `yaml:"storage"` + Users map[string]UserConfig `yaml:"users"` + TLS TLSConfig `yaml:"tls"` + Logging LoggingConfig `yaml:"logging"` } type ServerConfig struct { @@ -36,10 +36,10 @@ type StorageConfig struct { type UserConfig struct { // bcrypt-hashed password (use `htpasswd -nB `) - Password string `yaml:"password"` - DisplayName string `yaml:"display_name"` - Email string `yaml:"email"` - Calendars []string `yaml:"calendars"` + Password string `yaml:"password"` + DisplayName string `yaml:"display_name"` + Email string `yaml:"email"` + Calendars []string `yaml:"calendars"` AddressBooks []string `yaml:"address_books"` } diff --git a/internal/store/store.go b/internal/store/store.go index 1f7e121..dd0c511 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -17,9 +17,15 @@ var ErrConflict = errors.New("conflict") // Store is a filesystem-backed key/value store for DAV objects. // Each "collection" maps to a directory; each "object" maps to a file. +// +// Locking is sharded per-user (rather than one global mutex) so that +// concurrent requests from different users don't serialize against each +// other; operations within a single user's data still block one another. type Store struct { rootDir string - mu sync.RWMutex + + locksMu sync.Mutex + locks map[string]*sync.RWMutex } // NewStore creates or opens a Store rooted at rootDir. @@ -27,7 +33,19 @@ func NewStore(rootDir string) (*Store, error) { if err := os.MkdirAll(rootDir, 0o755); err != nil { return nil, fmt.Errorf("creating store root %q: %w", rootDir, err) } - return &Store{rootDir: rootDir}, nil + return &Store{rootDir: rootDir, locks: make(map[string]*sync.RWMutex)}, nil +} + +// lockFor returns the per-user lock, creating it on first use. +func (s *Store) lockFor(user string) *sync.RWMutex { + s.locksMu.Lock() + defer s.locksMu.Unlock() + l, ok := s.locks[user] + if !ok { + l = &sync.RWMutex{} + s.locks[user] = l + } + return l } // collectionPath returns the filesystem path for a collection. @@ -42,16 +60,18 @@ func (s *Store) objectPath(user, collection, objectID string) string { // EnsureCollection creates the collection directory if it does not exist. func (s *Store) EnsureCollection(user, collection string) error { - s.mu.Lock() - defer s.mu.Unlock() + l := s.lockFor(user) + l.Lock() + defer l.Unlock() dir := s.collectionPath(user, collection) return os.MkdirAll(dir, 0o755) } // ListCollections returns all collection names for a user. func (s *Store) ListCollections(user string) ([]string, error) { - s.mu.RLock() - defer s.mu.RUnlock() + l := s.lockFor(user) + l.RLock() + defer l.RUnlock() userDir := filepath.Join(s.rootDir, sanitize(user)) entries, err := os.ReadDir(userDir) @@ -73,8 +93,9 @@ func (s *Store) ListCollections(user string) ([]string, error) { // GetCollection returns metadata about a collection. func (s *Store) GetCollection(user, collection string) (os.FileInfo, error) { - s.mu.RLock() - defer s.mu.RUnlock() + l := s.lockFor(user) + l.RLock() + defer l.RUnlock() info, err := os.Stat(s.collectionPath(user, collection)) if errors.Is(err, os.ErrNotExist) { return nil, ErrNotFound @@ -84,8 +105,9 @@ func (s *Store) GetCollection(user, collection string) (os.FileInfo, error) { // PutObject writes data to an object, creating or replacing it. func (s *Store) PutObject(user, collection, objectID string, data []byte) error { - s.mu.Lock() - defer s.mu.Unlock() + l := s.lockFor(user) + l.Lock() + defer l.Unlock() dir := s.collectionPath(user, collection) if err := os.MkdirAll(dir, 0o755); err != nil { @@ -103,8 +125,9 @@ func (s *Store) PutObject(user, collection, objectID string, data []byte) error // GetObject reads an object's raw bytes. func (s *Store) GetObject(user, collection, objectID string) ([]byte, error) { - s.mu.RLock() - defer s.mu.RUnlock() + l := s.lockFor(user) + l.RLock() + defer l.RUnlock() data, err := os.ReadFile(s.objectPath(user, collection, objectID)) if errors.Is(err, os.ErrNotExist) { @@ -115,8 +138,9 @@ func (s *Store) GetObject(user, collection, objectID string) ([]byte, error) { // DeleteObject removes an object. func (s *Store) DeleteObject(user, collection, objectID string) error { - s.mu.Lock() - defer s.mu.Unlock() + l := s.lockFor(user) + l.Lock() + defer l.Unlock() err := os.Remove(s.objectPath(user, collection, objectID)) if errors.Is(err, os.ErrNotExist) { @@ -127,8 +151,9 @@ func (s *Store) DeleteObject(user, collection, objectID string) error { // ListObjects returns all object filenames in a collection. func (s *Store) ListObjects(user, collection string) ([]string, error) { - s.mu.RLock() - defer s.mu.RUnlock() + l := s.lockFor(user) + l.RLock() + defer l.RUnlock() dir := s.collectionPath(user, collection) entries, err := os.ReadDir(dir) @@ -150,8 +175,9 @@ func (s *Store) ListObjects(user, collection string) ([]string, error) { // StatObject returns FileInfo for an object. func (s *Store) StatObject(user, collection, objectID string) (os.FileInfo, error) { - s.mu.RLock() - defer s.mu.RUnlock() + l := s.lockFor(user) + l.RLock() + defer l.RUnlock() info, err := os.Stat(s.objectPath(user, collection, objectID)) if errors.Is(err, os.ErrNotExist) { @@ -162,8 +188,9 @@ func (s *Store) StatObject(user, collection, objectID string) (os.FileInfo, erro // DeleteCollection removes an entire collection directory. func (s *Store) DeleteCollection(user, collection string) error { - s.mu.Lock() - defer s.mu.Unlock() + l := s.lockFor(user) + l.Lock() + defer l.Unlock() err := os.RemoveAll(s.collectionPath(user, collection)) return err } diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 2c18985..c423d8f 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -1,7 +1,9 @@ package store_test import ( + "fmt" "os" + "sync" "testing" "github.com/yourusername/caldav-server/internal/store" @@ -77,3 +79,55 @@ func TestSanitizePath(t *testing.T) { t.Fatal("path traversal succeeded — security issue!") } } + +// TestConcurrentMultiUserAccess exercises the store from several users +// concurrently to make sure the per-user locking not only avoids data races +// (checked by -race) but also doesn't serialize unrelated users' operations +// incorrectly (e.g. deadlocks or cross-user data corruption). +func TestConcurrentMultiUserAccess(t *testing.T) { + dir := t.TempDir() + st, err := store.NewStore(dir) + if err != nil { + t.Fatalf("NewStore: %v", err) + } + + const users = 8 + const objectsPerUser = 20 + + var wg sync.WaitGroup + for u := 0; u < users; u++ { + user := fmt.Sprintf("user%d", u) + wg.Add(1) + go func(user string) { + defer wg.Done() + for i := 0; i < objectsPerUser; i++ { + id := fmt.Sprintf("obj-%d.ics", i) + data := []byte(fmt.Sprintf("DATA-%s-%d", user, i)) + if err := st.PutObject(user, "cal-personal", id, data); err != nil { + t.Errorf("PutObject(%s, %d): %v", user, i, err) + return + } + got, err := st.GetObject(user, "cal-personal", id) + if err != nil { + t.Errorf("GetObject(%s, %d): %v", user, i, err) + return + } + if string(got) != string(data) { + t.Errorf("cross-user data corruption for %s obj %d: got %q want %q", user, i, got, data) + } + } + }(user) + } + wg.Wait() + + for u := 0; u < users; u++ { + user := fmt.Sprintf("user%d", u) + ids, err := st.ListObjects(user, "cal-personal") + if err != nil { + t.Fatalf("ListObjects(%s): %v", user, err) + } + if len(ids) != objectsPerUser { + t.Errorf("user %s: expected %d objects, got %d", user, objectsPerUser, len(ids)) + } + } +} diff --git a/internal/webdav/handler.go b/internal/webdav/handler.go index 58e22a4..4e8bee0 100644 --- a/internal/webdav/handler.go +++ b/internal/webdav/handler.go @@ -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//. +// 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// 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) }) } diff --git a/internal/webdav/handler_test.go b/internal/webdav/handler_test.go new file mode 100644 index 0000000..d46b0be --- /dev/null +++ b/internal/webdav/handler_test.go @@ -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 := ` + + + + test +` + + 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()) + } +}