From 879462d717351fe5d276ddec5246bdec31b41661 Mon Sep 17 00:00:00 2001 From: kolaente Date: Thu, 9 Apr 2026 17:17:09 +0200 Subject: [PATCH] fix(caldav): enforce URL project match in GetResourcesByList Multiget REPORT requests would happily return tasks from projects different from the one in the href, even though GetTasksByUIDs now filters by access. Drop any returned task whose real project_id does not match the project ID parsed from the href path segment. Hardening for GHSA-48ch-p4gq-x46x. --- pkg/routes/caldav/listStorageProvider.go | 29 ++++++++++-- pkg/routes/caldav/listStorageProvider_test.go | 47 +++++++++++++++++++ pkg/webtests/caldav_test.go | 36 ++++++++++++++ 3 files changed, 108 insertions(+), 4 deletions(-) diff --git a/pkg/routes/caldav/listStorageProvider.go b/pkg/routes/caldav/listStorageProvider.go index ab0ccad31..5544d3ec7 100644 --- a/pkg/routes/caldav/listStorageProvider.go +++ b/pkg/routes/caldav/listStorageProvider.go @@ -175,16 +175,32 @@ func principalPathForUser(username string) string { // GetResourcesByList fetches a list of resources from a slice of paths func (vcls *VikunjaCaldavProjectStorage) GetResourcesByList(rpaths []string) (resources []data.Resource, err error) { - // Parse the set of resourcepaths into usable uids - // A path looks like this: /dav/projects/10/a6eb526d5748a5c499da202fe74f36ed1aea2aef.ics - // So we split the url in parts, take the last one and strip the ".ics" at the end + // Path format: /dav/projects/{projectID}/{uid}.ics. + // Remember the href's project ID per uid so the consistency check below + // can drop tasks requested via the wrong project (GHSA-48ch-p4gq-x46x). var uids []string + uidURLProjects := map[string]int64{} for _, path := range rpaths { parts := strings.Split(path, "/") if len(parts) < 5 { continue } - uids = append(uids, strings.TrimSuffix(parts[4], ".ics")) + // Skip malformed hrefs: without these guards an empty/non-numeric + // project would bypass the consistency check, and an empty UID + // would query for tasks with empty/NULL uids. + if !strings.HasSuffix(parts[4], ".ics") { + continue + } + uid := strings.TrimSuffix(parts[4], ".ics") + if uid == "" { + continue + } + urlProjectID, perr := strconv.ParseInt(parts[3], 10, 64) + if perr != nil { + continue + } + uids = append(uids, uid) + uidURLProjects[uid] = urlProjectID } if len(uids) == 0 { @@ -207,6 +223,11 @@ func (vcls *VikunjaCaldavProjectStorage) GetResourcesByList(rpaths []string) (re } for _, t := range tasks { + // Closes the URL-path leak for calendar-multiget REPORTs + // (GHSA-48ch-p4gq-x46x). + if urlProjectID, ok := uidURLProjects[t.UID]; ok && urlProjectID != t.ProjectID { + continue + } rr := VikunjaProjectResourceAdapter{ task: t, } diff --git a/pkg/routes/caldav/listStorageProvider_test.go b/pkg/routes/caldav/listStorageProvider_test.go index caa92fec7..477b58e20 100644 --- a/pkg/routes/caldav/listStorageProvider_test.go +++ b/pkg/routes/caldav/listStorageProvider_test.go @@ -486,3 +486,50 @@ END:VCALENDAR` assert.Equal(t, "uid-caldav-test-child-task-2", formerSiblingSubTask.UID) }) } + +// TestGetResourcesByList_URLProjectConsistency covers GHSA-48ch-p4gq-x46x. +func TestGetResourcesByList_URLProjectConsistency(t *testing.T) { + u := &user.User{ + ID: 15, + Username: "user15", + } + + t.Run("drops task when href project does not match task project", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + + storage := &VikunjaCaldavProjectStorage{user: u} + + // uid-caldav-test lives in project 36 (fixtures/tasks.yml id 40). + resources, err := storage.GetResourcesByList([]string{ + "/dav/projects/38/uid-caldav-test.ics", + }) + require.NoError(t, err) + assert.Empty(t, resources, "task must not be returned when href project mismatches") + }) + + t.Run("returns task when href project matches task project", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + + storage := &VikunjaCaldavProjectStorage{user: u} + + resources, err := storage.GetResourcesByList([]string{ + "/dav/projects/36/uid-caldav-test.ics", + }) + require.NoError(t, err) + require.Len(t, resources, 1) + }) + + t.Run("drops task for unauthorized user", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + + // user 6 has no access to project 36 + outsider := &user.User{ID: 6, Username: "user6"} + storage := &VikunjaCaldavProjectStorage{user: outsider} + + resources, err := storage.GetResourcesByList([]string{ + "/dav/projects/36/uid-caldav-test.ics", + }) + require.NoError(t, err) + assert.Empty(t, resources, "unauthorized user must not receive any tasks") + }) +} diff --git a/pkg/webtests/caldav_test.go b/pkg/webtests/caldav_test.go index f338db71f..900772d0c 100644 --- a/pkg/webtests/caldav_test.go +++ b/pkg/webtests/caldav_test.go @@ -107,6 +107,42 @@ END:VCALENDAR` assert.NotContains(t, rec.Body.String(), "Title Caldav Test") assert.Equal(t, http.StatusNotFound, rec.Result().StatusCode) }) + t.Run("Multiget rejects UIDs from other projects", func(t *testing.T) { + e, _ := setupTestEnv() + + // href claims project 38, but uid-caldav-test lives in project 36. + reportBody := ` + + + + + + /dav/projects/38/uid-caldav-test.ics +` + + rec, err := newCaldavTestRequestWithUser(t, e, "REPORT", caldav.ProjectHandler, &testuser15, reportBody, nil, map[string]string{"project": "38"}) + require.NoError(t, err) + assert.NotContains(t, rec.Body.String(), "Title Caldav Test") + assert.NotContains(t, rec.Body.String(), "Description Caldav Test") + }) + t.Run("Multiget cross-user UID read is rejected", func(t *testing.T) { + e, _ := setupTestEnv() + + reportBody := ` + + + + + + /dav/projects/36/uid-caldav-test.ics +` + + // testuser6 cannot access project 36 where uid-caldav-test lives. + rec, err := newCaldavTestRequestWithUser(t, e, "REPORT", caldav.ProjectHandler, &testuser6, reportBody, nil, map[string]string{"project": "36"}) + require.NoError(t, err) + assert.NotContains(t, rec.Body.String(), "Title Caldav Test") + assert.NotContains(t, rec.Body.String(), "Description Caldav Test") + }) } func TestCaldavDiscovery(t *testing.T) {