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")