From cd82ad4d5115e6d07011f95fd9dbc5b255aa1ca2 Mon Sep 17 00:00:00 2001 From: Tink Date: Wed, 19 Aug 2026 22:13:06 +0200 Subject: [PATCH] fix(caldav): encode task uids in hrefs instead of interpolating them raw (#3560) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 → /dav/projects/36/evil/../../../projects/5/y.ics 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&bd") → "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 → /dav/projects/5/pwned&y.ics ``` 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//pwned&y` in a project you own. 2. Run `curl -u : -X PROPFIND -H 'Depth: 1' https:///dav/projects//` 3. **Expected:** the `` for that task is percent-encoded, stays under `/dav/projects//`, 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 --- pkg/routes/caldav/handler.go | 11 +- pkg/routes/caldav/href_encoding_test.go | 178 +++++++++++++++++++++++ pkg/routes/caldav/listStorageProvider.go | 28 +++- pkg/webtests/integrations.go | 23 +++ 4 files changed, 232 insertions(+), 8 deletions(-) create mode 100644 pkg/routes/caldav/href_encoding_test.go diff --git a/pkg/routes/caldav/handler.go b/pkg/routes/caldav/handler.go index 6dbd6fab5..5386bf0d3 100644 --- a/pkg/routes/caldav/handler.go +++ b/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, diff --git a/pkg/routes/caldav/href_encoding_test.go b/pkg/routes/caldav/href_encoding_test.go new file mode 100644 index 000000000..c2c1d0f34 --- /dev/null +++ b/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 . + +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&y` + +const propfindGetetag = `` + +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") +} diff --git a/pkg/routes/caldav/listStorageProvider.go b/pkg/routes/caldav/listStorageProvider.go index b696cbf18..f439b0168 100644 --- a/pkg/routes/caldav/listStorageProvider.go +++ b/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 } diff --git a/pkg/webtests/integrations.go b/pkg/webtests/integrations.go index e3d5bc05e..ff7b7d652 100644 --- a/pkg/webtests/integrations.go +++ b/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")