fix: keep server-managed names out of reach of DAV clients (#38)

README.es.md

@@ -72,6 +72,13 @@ volumen de datos, layout "multifilesystem" de Radicale
   Los metadatos de colección viven en los `.Radicale.props` propios de
   Radicale (mismas claves, incluido el string separado por comas del
   supported-calendar-component-set).
+- **Los nombres que gestiona el servidor están reservados**: un nombre que
+  empieza por punto (el `.Radicale.props` propio de cada colección, los ficheros
+  `.tmp-*` de las escrituras atómicas) o que acaba en tilde no existe para los
+  clientes. Un `PUT` sobre ese nombre responde 400 y `DELETE`/`GET` responden
+  404, así que ningún cliente puede romper una colección escribiendo encima de
+  sus metadatos o borrándolos. Radicale aplica la misma regla con
+  `pathutils.is_safe_filesystem_path_component`.
 - **La identidad es una cabecera**: se confía ciegamente en
   `X-Remote-User`, así que el puerto nunca debe ser alcanzable sin el proxy
   autenticador delante. En modo `-dav-root` el proxy elimina los prefijos

README.md

@@ -67,6 +67,12 @@ data volume, Radicale "multifilesystem" layout
   indistinguishable from one written by Radicale - in both directions.
   Collection metadata lives in Radicale's own `.Radicale.props` files (same
   keys, including the comma separated supported-calendar-component-set).
+- **Server-managed names are reserved**: a name with a leading dot (a
+  collection's own `.Radicale.props`, the `.tmp-*` files of atomic writes) or a
+  trailing tilde does not exist as far as clients are concerned. `PUT` on such a
+  name answers 400 and `DELETE`/`GET` answer 404, so no client can break a
+  collection by writing over, or deleting, its metadata. Radicale enforces the
+  same rule through `pathutils.is_safe_filesystem_path_component`.
 - **Identity is a header**: `X-Remote-User` is trusted blindly, so the port
   must never be reachable without the authenticating proxy in front. In
   `-dav-root` mode the proxy strips the `/caldav`/`/carddav` prefixes and

cmd/server/dav_integration_test.go

@@ -1076,3 +1076,66 @@ func TestSpaFallbackStillWorks(t *testing.T) {
 		t.Errorf("spa fallback body = %q", body)
 	}
 }
+
+// TestServerManagedNamesAreUnreachable covers issue #38: no DAV request may
+// touch the ".Radicale.props" metadata file of a collection, because losing it
+// makes the whole collection disappear from the tree (404) with no way back.
+func TestServerManagedNamesAreUnreachable(t *testing.T) {
+	srv, dir := newTestServer(t)
+
+	resp, body := do(t, "PROPFIND", srv.URL+"/caldav/alice/", propfindBody, map[string]string{"Depth": "1"})
+	if resp.StatusCode != http.StatusMultiStatus {
+		t.Fatalf("PROPFIND home = %d, want 207: %s", resp.StatusCode, body)
+	}
+	props := filepath.Join(dir, "collection-root", "alice", "def-calendar", store.PropsFile)
+	before, err := os.ReadFile(props)
+	if err != nil {
+		t.Fatalf("metadata file missing before the requests: %v", err)
+	}
+
+	// A valid stream whose UID matches the reserved name must still be refused.
+	evil := strings.Replace(testICS, "UID:e1", "UID:"+store.PropsFile, 1)
+	resp, body = do(t, "PUT", srv.URL+"/caldav/alice/def-calendar/"+store.PropsFile, evil, map[string]string{"Content-Type": "text/calendar"})
+	if resp.StatusCode != http.StatusBadRequest {
+		t.Errorf("PUT on the metadata file = %d, want 400: %s", resp.StatusCode, body)
+	}
+
+	resp, body = do(t, "DELETE", srv.URL+"/caldav/alice/def-calendar/"+store.PropsFile, "", nil)
+	if resp.StatusCode != http.StatusNotFound {
+		t.Errorf("DELETE on the metadata file = %d, want 404: %s", resp.StatusCode, body)
+	}
+
+	resp, body = do(t, "GET", srv.URL+"/caldav/alice/def-calendar/"+store.PropsFile, "", nil)
+	if resp.StatusCode != http.StatusNotFound {
+		t.Errorf("GET on the metadata file = %d, want 404: %s", resp.StatusCode, body)
+	}
+
+	// Hidden names are reserved for every object, not only for the metadata file.
+	resp, body = do(t, "PUT", srv.URL+"/caldav/alice/def-calendar/.hidden.ics", testICS, map[string]string{"Content-Type": "text/calendar"})
+	if resp.StatusCode != http.StatusBadRequest {
+		t.Errorf("PUT on a hidden name = %d, want 400: %s", resp.StatusCode, body)
+	}
+
+	// A collection cannot take a reserved name either.
+	resp, body = do(t, "MKCALENDAR", srv.URL+"/caldav/alice/.reserved/", "", nil)
+	if resp.StatusCode != http.StatusBadRequest {
+		t.Errorf("MKCALENDAR with a reserved name = %d, want 400: %s", resp.StatusCode, body)
+	}
+
+	// The metadata file survived, the collection still serves and the listing
+	// does not expose the reserved name.
+	after, err := os.ReadFile(props)
+	if err != nil {
+		t.Fatalf("metadata file is gone after the requests: %v", err)
+	}
+	if string(before) != string(after) {
+		t.Error("metadata file was modified")
+	}
+	resp, body = do(t, "PROPFIND", srv.URL+"/caldav/alice/def-calendar/", propfindBody, map[string]string{"Depth": "1"})
+	if resp.StatusCode != http.StatusMultiStatus {
+		t.Errorf("PROPFIND on the collection after the requests = %d, want 207: %s", resp.StatusCode, body)
+	}
+	if strings.Contains(body, store.PropsFile) {
+		t.Error("the listing exposes the metadata file")
+	}
+}

internal/dav/dav.go

@@ -114,6 +114,15 @@ func (h *Handler) resolve(r *http.Request) (*resource, error) {
 	if segs[0] != user {
 		return nil, errForbidden
 	}
+	// Names the server reserves for itself (the ".Radicale.props" metadata file
+	// of every collection and the ".tmp-*" files of atomic writes) do not exist
+	// as far as clients are concerned, which is how Radicale behaves too
+	// (pathutils.is_safe_filesystem_path_component in its filesystem storage).
+	for _, s := range segs[1:] {
+		if !store.ValidName(s) {
+			return nil, errNotFound
+		}
+	}
 	switch len(segs) {
 	case 1:
 	case 2:
@@ -146,6 +155,9 @@ func (h *Handler) resolveForCreate(r *http.Request) (*resource, error) {
 	if len(segs) != 2 || segs[0] != user {
 		return nil, &davError{code: http.StatusForbidden, msg: "dav: collections can only be created under the user home set"}
 	}
+	if !store.ValidName(segs[1]) {
+		return nil, errReservedName
+	}
 	if _, err := h.Store.GetCollection(user, segs[1]); err == nil {
 		return nil, &davError{code: http.StatusMethodNotAllowed, msg: "dav: collection already exists"}
 	}
@@ -269,6 +281,10 @@ func (e *davError) Error() string { return e.msg }
 var (
 	errForbidden = &davError{code: http.StatusForbidden, msg: "dav: access to another user's data is not allowed"}
 	errNotFound  = &davError{code: http.StatusNotFound, msg: "dav: resource not found"}
+	// errReservedName is the answer to a request that tries to create or write a
+	// name the server keeps for itself (leading dot, trailing tilde). Radicale
+	// answers a PUT with such an href with 400 as well.
+	errReservedName = &davError{code: http.StatusBadRequest, msg: "dav: names starting with a dot or ending with a tilde are reserved by the server"}
 )
 
 func writeError(w http.ResponseWriter, err error) {

internal/dav/objects.go

@@ -41,6 +41,9 @@ func (h *Handler) resolveForWrite(r *http.Request) (*resource, error) {
 	if len(segs) != 3 || segs[0] != user {
 		return nil, &davError{code: http.StatusForbidden, msg: "dav: objects live at /<user>/<collection>/<item>"}
 	}
+	if !store.ValidName(segs[1]) || !store.ValidName(segs[2]) {
+		return nil, errReservedName
+	}
 	c, err := h.Store.GetCollection(user, segs[1])
 	if err != nil || c.Tag != h.Kind.collectionTag() {
 		return nil, &davError{code: http.StatusConflict, msg: "dav: the parent collection does not exist"}

internal/store/store.go

@@ -137,6 +137,30 @@ func cleanSeg(s string) (string, error) {
 	return s, nil
 }
 
+// cleanName is cleanSeg plus the rule that keeps server-managed files out of
+// the reach of clients: names with a leading dot (the ".Radicale.props"
+// metadata file of every collection and the ".tmp-*" files used by atomic
+// writes) and names with a trailing tilde are reserved. Radicale applies the
+// same rule through pathutils.is_safe_filesystem_path_component before it
+// touches the filesystem, which is what stops a client from deleting or
+// overwriting a collection's own metadata.
+func cleanName(s string) (string, error) {
+	if _, err := cleanSeg(s); err != nil {
+		return "", err
+	}
+	if strings.HasPrefix(s, ".") || strings.HasSuffix(s, "~") {
+		return "", fmt.Errorf("store: reserved path segment %q", s)
+	}
+	return s, nil
+}
+
+// ValidName reports whether name is usable as a collection or object name on
+// the wire.
+func ValidName(name string) bool {
+	_, err := cleanName(name)
+	return err == nil
+}
+
 func (s *Store) userDir(user string) (string, error) {
 	u, err := cleanSeg(user)
 	if err != nil {
@@ -262,7 +286,7 @@ func (s *Store) ListCollections(user string) ([]Collection, error) {
 
 // GetCollection returns one collection by id.
 func (s *Store) GetCollection(user, id string) (Collection, error) {
-	if _, err := cleanSeg(id); err != nil {
+	if _, err := cleanName(id); err != nil {
 		return Collection{}, err
 	}
 	dir, err := s.userDir(user)
@@ -274,7 +298,7 @@ func (s *Store) GetCollection(user, id string) (Collection, error) {
 
 // CreateCollection creates a collection directory with props.
 func (s *Store) CreateCollection(user string, c Collection) error {
-	if _, err := cleanSeg(c.ID); err != nil {
+	if _, err := cleanName(c.ID); err != nil {
 		return err
 	}
 	dir, err := s.userDir(user)
@@ -296,7 +320,7 @@ func (s *Store) CreateCollection(user string, c Collection) error {
 
 // DeleteCollection removes a collection directory entirely.
 func (s *Store) DeleteCollection(user, id string) error {
-	if _, err := cleanSeg(id); err != nil {
+	if _, err := cleanName(id); err != nil {
 		return err
 	}
 	dir, err := s.userDir(user)
@@ -311,6 +335,9 @@ func (s *Store) DeleteCollection(user, id string) error {
 // Fixed keys are applied to their struct fields, everything else to Extra.
 // The write is atomic (same mechanism as items).
 func (s *Store) UpdateCollectionProps(user, id string, set map[string]string, remove []string) error {
+	if _, err := cleanName(id); err != nil {
+		return err
+	}
 	dir, err := s.userDir(user)
 	if err != nil {
 		return err
@@ -350,7 +377,7 @@ func (s *Store) UpdateCollectionProps(user, id string, set map[string]string, re
 }
 
 func (s *Store) collDir(user, coll string) (string, error) {
-	if _, err := cleanSeg(coll); err != nil {
+	if _, err := cleanName(coll); err != nil {
 		return "", err
 	}
 	dir, err := s.userDir(user)
@@ -372,7 +399,7 @@ func (s *Store) ListItems(user, coll string) ([]string, error) {
 	}
 	var out []string
 	for _, e := range entries {
-		if e.IsDir() || e.Name() == PropsFile || strings.HasPrefix(e.Name(), ".tmp-") {
+		if e.IsDir() || !ValidName(e.Name()) {
 			continue
 		}
 		out = append(out, e.Name())
@@ -382,7 +409,7 @@ func (s *Store) ListItems(user, coll string) ([]string, error) {
 
 // ReadItem returns the raw content of an item.
 func (s *Store) ReadItem(user, coll, name string) ([]byte, error) {
-	if _, err := cleanSeg(name); err != nil {
+	if _, err := cleanName(name); err != nil {
 		return nil, err
 	}
 	dir, err := s.collDir(user, coll)
@@ -407,7 +434,7 @@ func (s *Store) ItemETag(user, coll, name string) (string, error) {
 
 // WriteItem writes an item atomically and returns its new ETag (sha256).
 func (s *Store) WriteItem(user, coll, name string, data []byte) (string, error) {
-	if _, err := cleanSeg(name); err != nil {
+	if _, err := cleanName(name); err != nil {
 		return "", err
 	}
 	dir, err := s.collDir(user, coll)
@@ -425,7 +452,7 @@ func (s *Store) WriteItem(user, coll, name string, data []byte) (string, error)
 
 // DeleteItem removes an item. Returns os.ErrNotExist if missing.
 func (s *Store) DeleteItem(user, coll, name string) error {
-	if _, err := cleanSeg(name); err != nil {
+	if _, err := cleanName(name); err != nil {
 		return err
 	}
 	dir, err := s.collDir(user, coll)
@@ -437,7 +464,7 @@ func (s *Store) DeleteItem(user, coll, name string) error {
 
 // ItemModTime returns the modification time of an item.
 func (s *Store) ItemModTime(user, coll, name string) (time.Time, error) {
-	if _, err := cleanSeg(name); err != nil {
+	if _, err := cleanName(name); err != nil {
 		return time.Time{}, err
 	}
 	dir, err := s.collDir(user, coll)

internal/store/store_test.go

@@ -232,3 +232,69 @@ func TestCreateCollectionRejectsExisting(t *testing.T) {
 		t.Error("collection still readable after delete")
 	}
 }
+
+// TestReservedNamesAreRefused mirrors Radicale's
+// pathutils.is_safe_filesystem_path_component: a collection's own metadata file
+// and the files an atomic write uses must never be reachable through the store
+// API, whichever call is used.
+func TestReservedNamesAreRefused(t *testing.T) {
+	root := t.TempDir()
+	s := New(root)
+	if err := s.EnsureUser("alice"); err != nil {
+		t.Fatal(err)
+	}
+
+	for _, name := range []string{PropsFile, ".hidden.ics", ".Radicale.cache", "trailing~"} {
+		if ValidName(name) {
+			t.Errorf("ValidName(%q) = true, want false", name)
+		}
+		if _, err := s.WriteItem("alice", "def-calendar", name, []byte("x")); err == nil {
+			t.Errorf("WriteItem(%q) succeeded, want an error", name)
+		}
+		if _, err := s.ReadItem("alice", "def-calendar", name); err == nil {
+			t.Errorf("ReadItem(%q) succeeded, want an error", name)
+		}
+		if err := s.DeleteItem("alice", "def-calendar", name); err == nil {
+			t.Errorf("DeleteItem(%q) succeeded, want an error", name)
+		}
+		if _, err := s.GetCollection("alice", name); err == nil {
+			t.Errorf("GetCollection(%q) succeeded, want an error", name)
+		}
+		if err := s.CreateCollection("alice", Collection{ID: name, Tag: "VCALENDAR"}); err == nil {
+			t.Errorf("CreateCollection(%q) succeeded, want an error", name)
+		}
+	}
+
+	// The metadata file of a live collection survives a delete attempt.
+	props := filepath.Join(root, "collection-root", "alice", "def-calendar", PropsFile)
+	before, err := os.ReadFile(props)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := s.DeleteItem("alice", "def-calendar", PropsFile); err == nil {
+		t.Fatal("DeleteItem on the metadata file reported success")
+	}
+	after, err := os.ReadFile(props)
+	if err != nil {
+		t.Fatalf("metadata file is gone after DeleteItem: %v", err)
+	}
+	if string(before) != string(after) {
+		t.Error("metadata file changed")
+	}
+
+	// Hidden and reserved names are not listed, ordinary ones are.
+	if _, err := s.WriteItem("alice", "def-calendar", "e1.ics", []byte("x")); err != nil {
+		t.Fatal(err)
+	}
+	hidden := filepath.Join(root, "collection-root", "alice", "def-calendar", ".hidden.ics")
+	if err := os.WriteFile(hidden, []byte("x"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	items, err := s.ListItems("alice", "def-calendar")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(items) != 1 || items[0] != "e1.ics" {
+		t.Errorf("ListItems = %v, want [e1.ics]", items)
+	}
+}