fix(caldav): encode task uids in hrefs instead of interpolating them raw (#3560)

Task uids are client chosen — the api only generates a uuid when the
field is empty (`pkg/models/tasks.go`), and the CalDAV parser stores
whatever the inbound VTODO carried (`pkg/caldav/parsing.go`). That value
went into hrefs verbatim, which lets a uid forge a path or inject
markup.

Split out of #3551 so the path-forgery half gets its own review.

## What goes wrong today

**Path forgery.** A uid containing a slash makes the href appear to live
in another collection:

```
UID:evil/../../../projects/5/y  →  <D:href>/dav/projects/36/evil/../../../projects/5/y.ics</D:href>
                                    path.Clean → /dav/projects/5/y.ics
```

Anyone who can create a task in a project the victim can see can plant
one; it then renders inside the victim's collection listing pointing
elsewhere. Same class as GHSA-48ch-p4gq-x46x.

**XML injection.** `sync_collection.go` wraps hrefs in `xmlEscape`, but
PROPFIND and calendar-multiget hand `Resource.Path` to caldav-go's
`ixml.HrefTag` → `ixml.Tag`, a plain `Sprintf`. `ixml.EscapeText` sits
in the same file and is only used for prop content. A uid with `<`, `>`
or `&` therefore lands raw in the multistatus body.

Input validation cannot close either: RFC 5545 §3.3.11 puts `<`
(`%x3C`), `>` (`%x3E`) and `&` (`%x26`) all inside `TSAFE-CHAR`, so they
are legal in a TEXT value.

## The fix

Percent-encode the uid to RFC 3986 `unreserved` when building an href.
`url.PathEscape` is not enough — it leaves sub-delims alone, so `&`
survives:

```
url.PathEscape("a&b<c>d")  →  "a&b%3Cc%3Ed"
```

Encoding conservatively means no XML metacharacter reaches the document,
so no change to the vendored library is needed. Existing uuid uids are
entirely `unreserved`, so their hrefs are byte-for-byte unchanged and no
client resyncs.

Then decode on the way back in — neither side did:

- echo v5 hands back the still-encoded path segment (`c.Param("task")` →
`evil%2F..%2F%3Cx%3E`), so `TaskHandler` needed the decode.
- `GetResourcesByList` parses hrefs out of the REPORT body and never
decoded, so encoded hrefs the server itself emitted would silently stop
matching and tasks would vanish from multiget responses.

One more, found while checking the other call sites: caldav-go builds
`Resource.Path` from `request.URL.Path`, which Go has already decoded,
and writes it into XML unescaped. So a PROPFIND against the encoded href
echoed the forged form right back:

```
PROPFIND /dav/projects/36/evil%2F..%2F..%2F..%2Fprojects%2F5%2Fpwned%3Cx%3E%26y.ics
  →  <D:href>/dav/projects/5/pwned<x>&y.ics</D:href>
```

Task requests now carry a canonical href that the storage returns
instead of echoing the client's path.

## Not fixed here

Principal hrefs interpolate the username unescaped, and
`pkg/user/user_create.go` only rejects spaces and the link-share
pattern. `isOwnPrincipalPath` limits this to the authenticated user's
own username, so it never crosses a user boundary — separate concern,
not uid-related.

## How to verify

1. Create a task over CalDAV whose VTODO carries
`UID:evil/../../../projects/<other project id>/pwned<x>&y` in a project
you own.
2. Run `curl -u <user>:<caldav-token> -X PROPFIND -H 'Depth: 1'
https://<instance>/dav/projects/<that project id>/`
3. **Expected:** the `<D:href>` for that task is percent-encoded, stays
under `/dav/projects/<that project id>/`, and the response body parses
as XML.
**Before this PR:** the href resolves to the other project's collection
and the body is malformed XML.

1. `GET` that percent-encoded href.
2. **Expected:** 200 with the task's VTODO — the uid round-trips.

1. Send a `calendar-multiget` REPORT listing that same href.
2. **Expected:** 207 containing the task.

1. Take any pre-existing task with a normal uuid uid and PROPFIND its
collection.
2. **Expected:** its href is unchanged from before this PR — encoding is
a no-op for uuids, so no client is forced to resync.

---------

Co-authored-by: kolaente <k@knt.li>

pkg/routes/caldav/handler.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/url"
 	"reflect"
 	"strconv"
 	"strings"
@@ -181,8 +182,14 @@ func TaskHandler(c *echo.Context) error {
 		return echo.NewHTTPError(http.StatusInternalServerError, "Internal server error").Wrap(err)
 	}
 
-	// Get the task uid
-	taskUID := strings.TrimSuffix(c.Param("task"), ".ics")
+	// Whether c.Param("task") arrives decoded depends on router config and on which
+	// bytes were escaped; the escaped path is unambiguous, so decode that instead.
+	esc := c.Request().URL.EscapedPath()
+	seg := esc[strings.LastIndex(esc, "/")+1:]
+	taskUID, err := url.PathUnescape(strings.TrimSuffix(seg, ".ics"))
+	if err != nil {
+		return c.String(http.StatusNotFound, "Task not found")
+	}
 
 	storage := &VikunjaCaldavProjectStorage{
 		project: project,

pkg/routes/caldav/href_encoding_test.go

@@ -0,0 +1,178 @@
+// Vikunja is a to-do list application to facilitate your life.
+// Copyright 2018-present Vikunja and contributors. All rights reserved.
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program.  If not, see <https://www.gnu.org/licenses/>.
+
+package caldav
+
+// Client-chosen task UIDs end up in CalDAV hrefs, so they must not be able to
+// forge a path or inject XML markup - and the server must still resolve every
+// href it hands out, whichever way the router decoded the request.
+
+import (
+	"encoding/xml"
+	"net/http"
+	"net/http/httptest"
+	"strconv"
+	"strings"
+	"testing"
+
+	"code.vikunja.io/api/pkg/db"
+	"code.vikunja.io/api/pkg/models"
+
+	"github.com/labstack/echo/v5"
+	"github.com/samedi/caldav-go/ixml"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Project 36 in the fixtures belongs to caldavFilterUser and holds the caldav tasks.
+const caldavHrefProjectID = 36
+
+// A UID that tries both path forgery and XML injection at once.
+const hostileUID = `evil/../../../projects/5/pwned<x>&y`
+
+const propfindGetetag = `<?xml version="1.0"?><D:propfind xmlns:D="DAV:"><D:prop><D:getetag/></D:prop></D:propfind>`
+
+func insertTaskWithUID(t *testing.T, uid string, index int64) {
+	t.Helper()
+
+	s := db.NewSession()
+	defer s.Close()
+
+	task := &models.Task{
+		Title:       "Task with uid " + uid,
+		UID:         uid,
+		ProjectID:   caldavHrefProjectID,
+		Index:       index,
+		CreatedByID: caldavFilterUser.ID,
+	}
+	_, err := s.Insert(task)
+	require.NoError(t, err)
+	require.NoError(t, s.Commit())
+}
+
+// Production builds its echo with UnescapePathParamValues (pkg/routes/routes.go), so a
+// test on echo's default config exercises a decode path production never takes.
+func newTaskRouter(unescapePathParamValues bool) *echo.Echo {
+	e := echo.NewWithConfig(echo.Config{
+		Router: echo.NewRouter(echo.RouterConfig{UnescapePathParamValues: unescapePathParamValues}),
+	})
+	e.Any("/dav/projects/:project/:task", func(c *echo.Context) error {
+		c.Set("userBasicAuth", caldavFilterUser)
+		return TaskHandler(c)
+	})
+	return e
+}
+
+func serve(e *echo.Echo, req *http.Request) *httptest.ResponseRecorder {
+	rec := httptest.NewRecorder()
+	e.ServeHTTP(rec, req)
+	return rec
+}
+
+var uidRoundTripCases = []struct {
+	name string
+	uid  string
+	href string
+	// A second task whose uid a double-decode of uid resolves to.
+	decoyUID string
+}{
+	{name: "uuid", uid: "550e8400-e29b-41d4-a716-446655440000", href: "/dav/projects/36/550e8400-e29b-41d4-a716-446655440000.ics"},
+	// RFC 5545 recommends the addr-spec form, so encoding it would resync every client.
+	{name: "addr-spec", uid: "task-1@host.example", href: "/dav/projects/36/task-1@host.example.ics"},
+	{name: "literal percent", uid: "50%off-sale", href: "/dav/projects/36/50%25off-sale.ics"},
+	{name: "percent escape lookalike", uid: "a%41b", href: "/dav/projects/36/a%2541b.ics", decoyUID: "aAb"},
+	{name: "space", uid: "sale 2026 spring", href: "/dav/projects/36/sale%202026%20spring.ics"},
+	{name: "utf-8", uid: "täsk-Ω", href: "/dav/projects/36/t%C3%A4sk-%CE%A9.ics"},
+	{name: "path forgery and xml injection", uid: hostileUID, href: `/dav/projects/36/evil%2F..%2F..%2F..%2Fprojects%2F5%2Fpwned%3Cx%3E%26y.ics`},
+}
+
+func TestTaskUIDHrefRoundTrip(t *testing.T) {
+	for _, tc := range uidRoundTripCases {
+		t.Run(tc.name, func(t *testing.T) {
+			href := taskURL(caldavHrefProjectID, &models.Task{UID: tc.uid})
+			require.Equal(t, tc.href, href)
+
+			// ixml.HrefTag does no escaping, so an unencoded href produces broken XML.
+			var parsed struct {
+				Href string `xml:",chardata"`
+			}
+			require.NoError(t, xml.Unmarshal([]byte(ixml.HrefTag(href)), &parsed))
+			assert.Equal(t, href, parsed.Href)
+
+			setup := func(t *testing.T) {
+				db.LoadAndAssertFixtures(t)
+				insertTaskWithUID(t, tc.uid, 98)
+				if tc.decoyUID != "" {
+					insertTaskWithUID(t, tc.decoyUID, 99)
+				}
+			}
+
+			t.Run("multiget resolves the href", func(t *testing.T) {
+				setup(t)
+
+				resources, err := storageFor(caldavFilterUser, caldavHrefProjectID).GetResourcesByList([]string{href})
+				require.NoError(t, err)
+				require.Len(t, resources, 1, "the server must resolve the href it emitted")
+				assert.Equal(t, href, resources[0].Path)
+
+				content, found := resources[0].GetContentData()
+				require.True(t, found)
+				assert.Contains(t, content, "UID:"+tc.uid)
+			})
+
+			for _, unescapePathParamValues := range []bool{false, true} {
+				t.Run("router unescape_path_param_values="+strconv.FormatBool(unescapePathParamValues), func(t *testing.T) {
+					setup(t)
+					e := newTaskRouter(unescapePathParamValues)
+
+					rec := serve(e, httptest.NewRequest(http.MethodGet, href, nil))
+					require.Equal(t, http.StatusOK, rec.Code)
+					assert.Contains(t, rec.Body.String(), "UID:"+tc.uid)
+					if tc.decoyUID != "" {
+						assert.NotContains(t, rec.Body.String(), "UID:"+tc.decoyUID, "the uid must not be decoded twice")
+					}
+
+					req := httptest.NewRequest("PROPFIND", href, strings.NewReader(propfindGetetag))
+					req.Header.Set("Depth", "0")
+					rec = serve(e, req)
+					require.Equal(t, http.StatusMultiStatus, rec.Code)
+
+					// caldav-go builds the href from the decoded request path, so an
+					// echoed one would be forged or unescaped.
+					var multistatus struct {
+						Responses []struct {
+							Href string `xml:"href"`
+						} `xml:"response"`
+					}
+					require.NoError(t, xml.Unmarshal(rec.Body.Bytes(), &multistatus))
+					require.Len(t, multistatus.Responses, 1)
+					assert.Equal(t, href, multistatus.Responses[0].Href)
+				})
+			}
+		})
+	}
+}
+
+func TestGetResourcesByList_MalformedEscape(t *testing.T) {
+	db.LoadAndAssertFixtures(t)
+
+	insertTaskWithUID(t, "uid-caldav-%zz", 98)
+
+	resources, err := storageFor(caldavFilterUser, caldavHrefProjectID).
+		GetResourcesByList([]string{"/dav/projects/36/uid-caldav-%zz.ics"})
+	require.NoError(t, err)
+	assert.Empty(t, resources, "an href that is not valid percent-encoding is not an href we emitted")
+}

pkg/routes/caldav/listStorageProvider.go

@@ -19,6 +19,7 @@ package caldav
 import (
 	"context"
 	"fmt"
+	"net/url"
 	"slices"
 	"strconv"
 	"strings"
@@ -100,7 +101,7 @@ func (vcls *VikunjaCaldavProjectStorage) GetResources(rpath string, withChildren
 		if err != nil {
 			return nil, err
 		}
-		r := data.NewResource(rpath, &rr)
+		r := data.NewResource(vcls.hrefFor(rpath), &rr)
 		r.Name = vcls.project.Title
 
 		// If the request is withChildren (Depth: 1), we need to return all tasks of the project
@@ -200,8 +201,9 @@ func (vcls *VikunjaCaldavProjectStorage) GetResourcesByList(rpaths []string) (re
 		if !strings.HasSuffix(parts[4], ".ics") {
 			continue
 		}
-		uid := strings.TrimSuffix(parts[4], ".ics")
-		if uid == "" {
+		// echo does not decode path params either, so hrefs travel percent-encoded end to end.
+		uid, uerr := url.PathUnescape(strings.TrimSuffix(parts[4], ".ics"))
+		if uerr != nil || uid == "" {
 			continue
 		}
 		urlProjectID, perr := strconv.ParseInt(parts[3], 10, 64)
@@ -308,7 +310,7 @@ func (vcls *VikunjaCaldavProjectStorage) GetResourcesByFilters(rpath string, _ *
 	if err != nil {
 		return nil, err
 	}
-	r := data.NewResource(rpath, &rr)
+	r := data.NewResource(vcls.hrefFor(rpath), &rr)
 	r.Name = vcls.project.Title
 	return []data.Resource{r}, nil
 	// For now, filtering is disabled.
@@ -317,7 +319,21 @@ func (vcls *VikunjaCaldavProjectStorage) GetResourcesByFilters(rpath string, _ *
 
 // WebDAV requires every Depth: 1 child to live under the collection's own URI.
 func taskURL(collectionProjectID int64, task *models.Task) string {
-	return ProjectBasePath + "/" + strconv.FormatInt(collectionProjectID, 10) + `/` + task.UID + `.ics`
+	return ProjectBasePath + "/" + strconv.FormatInt(collectionProjectID, 10) + `/` + encodeURIPathSegment(task.UID) + `.ics`
+}
+
+// url.PathEscape leaves & alone, and caldav-go writes hrefs into XML unescaped.
+func encodeURIPathSegment(segment string) string {
+	return strings.ReplaceAll(url.PathEscape(segment), "&", "%26")
+}
+
+// caldav-go builds rpath from the decoded request path, so echoing it back for a
+// task resource would undo the encoding taskURL applied.
+func (vcls *VikunjaCaldavProjectStorage) hrefFor(rpath string) string {
+	if strings.HasSuffix(rpath, ".ics") && vcls.project != nil && vcls.task != nil && vcls.task.UID != "" {
+		return taskURL(vcls.project.ID, vcls.task)
+	}
+	return rpath
 }
 
 // Project.CanWrite reports archival through its error return; caldav-go has no case for
@@ -501,7 +517,7 @@ func (vcls *VikunjaCaldavProjectStorage) GetResource(rpath string) (*data.Resour
 			project: vcls.project,
 			task:    vcls.task,
 		}
-		r := data.NewResource(rpath, &rr)
+		r := data.NewResource(vcls.hrefFor(rpath), &rr)
 		return &r, true, nil
 	}
 

pkg/webtests/integrations.go

@@ -187,6 +187,7 @@ func newCaldavTestRequestWithUser(t *testing.T, e *echo.Echo, method string, han
 	var c *echo.Context
 	c, rec = createRequest(e, method, payload, queryParams, urlParams)
 	c.Request().Header.Set(echo.HeaderContentType, echo.MIMETextPlain)
+	setCaldavRequestPath(c, urlParams)
 
 	result, _ := caldav.BasicAuth(c, user.Username, "12345678")
 	if !result {
@@ -197,6 +198,28 @@ func newCaldavTestRequestWithUser(t *testing.T, e *echo.Echo, method string, han
 	return
 }
 
+// createRequest sets path values out of band, but the caldav handlers and caldav-go
+// both read the resource off the request path, so the fixture has to carry a real one.
+func setCaldavRequestPath(c *echo.Context, urlParams map[string]string) {
+	project, has := urlParams["project"]
+	if !has {
+		return
+	}
+
+	path := caldav.ProjectBasePath + "/" + project
+	if task, has := urlParams["task"]; has {
+		path += "/" + url.PathEscape(task) + ".ics"
+	}
+
+	u, err := url.Parse(path)
+	if err != nil {
+		panic(err)
+	}
+	c.Request().URL.Path = u.Path
+	c.Request().URL.RawPath = u.RawPath
+	c.Request().RequestURI = path
+}
+
 func assertHandlerErrorCode(t *testing.T, err error, expectedErrorCode int) {
 	if err == nil {
 		t.Error("Error is nil")