From 82dcceca55882614855ebdbbf59ec25f4e7716bd Mon Sep 17 00:00:00 2001 From: kolaente Date: Wed, 4 Nov 2026 14:34:02 +0100 Subject: [PATCH] fix(caldav): invalidate sync tokens after permanent deletion Implement the approved last_purged_at decision with an advancing purge generation so same-second purges invalidate old tokens and full resync tokens remain usable. --- pkg/migration/20261104142611.go | 45 +++++++++++ pkg/migration/20261104142611_test.go | 92 ++++++++++++++++++++++ pkg/models/project.go | 2 + pkg/models/project_purge.go | 44 +++++++++++ pkg/models/project_purge_test.go | 91 +++++++++++++++++++++ pkg/models/task_trash.go | 5 ++ pkg/routes/caldav/listStorageProvider.go | 6 +- pkg/routes/caldav/sync_collection.go | 34 +++++--- pkg/routes/caldav/sync_collection_test.go | 60 ++++++++++++++ pkg/webtests/caldav_test.go | 96 +++++++++++++++++++++++ 10 files changed, 464 insertions(+), 11 deletions(-) create mode 100644 pkg/migration/20261104142611.go create mode 100644 pkg/migration/20261104142611_test.go create mode 100644 pkg/models/project_purge.go create mode 100644 pkg/models/project_purge_test.go create mode 100644 pkg/routes/caldav/sync_collection_test.go diff --git a/pkg/migration/20261104142611.go b/pkg/migration/20261104142611.go new file mode 100644 index 000000000..48b1df09c --- /dev/null +++ b/pkg/migration/20261104142611.go @@ -0,0 +1,45 @@ +// 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 migration + +import ( + "time" + + "src.techknowlogick.com/xormigrate" + "xorm.io/xorm" +) + +type projects20261104142611 struct { + LastPurgedAt time.Time `xorm:"null"` +} + +func (projects20261104142611) TableName() string { + return "projects" +} + +func init() { + migrations = append(migrations, &xormigrate.Migration{ + ID: "20261104142611", + Description: "Add project purge marker for CalDAV sync tokens", + Migrate: func(tx *xorm.Engine) error { + return partialSync(tx, projects20261104142611{}) + }, + Rollback: func(tx *xorm.Engine) error { + return nil + }, + }) +} diff --git a/pkg/migration/20261104142611_test.go b/pkg/migration/20261104142611_test.go new file mode 100644 index 000000000..c2025d441 --- /dev/null +++ b/pkg/migration/20261104142611_test.go @@ -0,0 +1,92 @@ +// 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 migration + +import ( + "testing" + + "code.vikunja.io/api/pkg/db" + + "github.com/stretchr/testify/require" + "xorm.io/xorm" + "xorm.io/xorm/schemas" +) + +type projectsBefore20261104142611 struct { + ID int64 `xorm:"bigint autoincr not null unique pk"` + OwnerID int64 `xorm:"bigint INDEX not null"` + Title string `xorm:"varchar(250) not null"` +} + +func (projectsBefore20261104142611) TableName() string { + return "projects" +} + +func projectTable20261104142611(t *testing.T, x *xorm.Engine) *schemas.Table { + t.Helper() + tables, err := x.DBMetas() + require.NoError(t, err) + for _, table := range tables { + if table.Name == "projects" { + return table + } + } + t.Fatal("projects table not found") + return nil +} + +func TestAddProjectPurgeMarker20261104142611(t *testing.T) { + x, err := db.CreateTestEngine() + require.NoError(t, err) + table := projectsBefore20261104142611{} + require.NoError(t, x.DropTables(table)) + t.Cleanup(func() { + require.NoError(t, x.DropTables(table)) + }) + require.NoError(t, x.Sync2(table)) + existing := projectsBefore20261104142611{ + ID: 1, + OwnerID: 42, + Title: "Existing project", + } + _, err = x.Insert(&existing) + require.NoError(t, err) + before := projectTable20261104142611(t, x) + require.NotEmpty(t, before.Indexes) + for _, m := range migrations { + if m.ID == "20261104142611" { + require.NoError(t, m.Migrate(x)) + break + } + } + after := projectTable20261104142611(t, x) + require.NotNil(t, after.GetColumn("last_purged_at")) + for _, column := range before.ColumnsSeq() { + require.NotNilf(t, after.GetColumn(column), "migration dropped column %s", column) + } + for name, index := range before.Indexes { + preserved, found := after.Indexes[name] + require.Truef(t, found, "migration dropped index %s", name) + require.Equal(t, index.Type, preserved.Type) + require.Equal(t, index.Cols, preserved.Cols) + } + got := projectsBefore20261104142611{} + found, err := x.ID(existing.ID).Get(&got) + require.NoError(t, err) + require.True(t, found) + require.Equal(t, existing, got) +} diff --git a/pkg/models/project.go b/pkg/models/project.go index 7e06b9e83..c9b5fff09 100644 --- a/pkg/models/project.go +++ b/pkg/models/project.go @@ -86,6 +86,8 @@ type Project struct { // A timestamp when this project was last updated. You cannot change this value. Updated time.Time `xorm:"updated not null" json:"updated" readOnly:"true" doc:"A timestamp when this project was last updated. You cannot change this value."` + LastPurgedAt time.Time `xorm:"null" json:"-"` + web.CRUDable `xorm:"-" json:"-"` web.Permissions `xorm:"-" json:"-"` } diff --git a/pkg/models/project_purge.go b/pkg/models/project_purge.go new file mode 100644 index 000000000..7da3ec709 --- /dev/null +++ b/pkg/models/project_purge.go @@ -0,0 +1,44 @@ +// 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 models + +import ( + "time" + + "xorm.io/xorm" +) + +func advanceProjectPurgeMarker(s *xorm.Session, projectID int64) error { + project := &Project{} + found, err := lockingSession(s).ID(projectID).Get(project) + if err != nil { + return err + } + if !found { + return ErrProjectDoesNotExist{ID: projectID} + } + + marker := time.Now().UTC().Truncate(time.Second) + // Whole seconds keep purge generations distinct across database precisions. + if !marker.After(project.LastPurgedAt) { + marker = project.LastPurgedAt.Add(time.Second) + } + _, err = s.ID(projectID).Cols("last_purged_at").NoAutoTime().Update(&Project{ + LastPurgedAt: marker, + }) + return err +} diff --git a/pkg/models/project_purge_test.go b/pkg/models/project_purge_test.go new file mode 100644 index 000000000..6a1c1ad06 --- /dev/null +++ b/pkg/models/project_purge_test.go @@ -0,0 +1,91 @@ +// 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 models + +import ( + "testing" + "time" + + "code.vikunja.io/api/pkg/db" + "code.vikunja.io/api/pkg/user" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "xorm.io/xorm" +) + +func projectPurgeMarkerForTest(t *testing.T, s *xorm.Session, projectID int64) time.Time { + t.Helper() + marker := struct { + LastPurgedAt time.Time `xorm:"null"` + }{} + found, err := s.Table("projects").Where("id = ?", projectID).Get(&marker) + require.NoError(t, err) + require.True(t, found) + return marker.LastPurgedAt +} + +func TestTaskPurgeSyncMarker(t *testing.T) { + u := &user.User{ID: 1} + t.Run("advances even when another purge has a newer deletion timestamp", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + future := time.Now().UTC().Add(time.Hour).Truncate(time.Second) + s := db.NewSession() + defer s.Close() + _, err := s.Table("projects").Where("id = ?", 1).Update(map[string]any{ + "last_purged_at": future, + }) + require.NoError(t, err) + require.NoError(t, s.Commit()) + s.Close() + + require.NoError(t, deletePermanentlyForTest(t, 51, u)) + s = db.NewSession() + defer s.Close() + assert.True(t, projectPurgeMarkerForTest(t, s, 1).Equal(future.Add(time.Second))) + assert.True(t, projectPurgeMarkerForTest(t, s, 2).IsZero()) + }) + + t.Run("rolls back together with the purged task", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + task := &Task{ID: 51} + can, err := task.CanDeletePermanently(s, u) + require.NoError(t, err) + require.True(t, can) + require.NoError(t, task.DeletePermanently(s, u)) + assert.False(t, projectPurgeMarkerForTest(t, s, 1).IsZero()) + require.NoError(t, s.Rollback()) + s.Close() + + s = db.NewSession() + defer s.Close() + assert.True(t, projectPurgeMarkerForTest(t, s, 1).IsZero()) + db.AssertExists(t, "tasks", map[string]any{ + "id": 51, + }, false) + }) + + t.Run("retention cleanup keeps the marker unchanged", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + deleteExpiredTasks(time.Now()) + s := db.NewSession() + defer s.Close() + assert.True(t, projectPurgeMarkerForTest(t, s, 1).IsZero()) + }) +} diff --git a/pkg/models/task_trash.go b/pkg/models/task_trash.go index 89cfb0040..a1d1869dc 100644 --- a/pkg/models/task_trash.go +++ b/pkg/models/task_trash.go @@ -146,6 +146,11 @@ func purgeTrashedTask(s *xorm.Session, t *Task, doer *user.User) error { if err := hardDeleteTask(s, t); err != nil { return err } + if doer != nil { + if err := advanceProjectPurgeMarker(s, t.ProjectID); err != nil { + return err + } + } events.DispatchOnCommit(s, &TaskPurgedEvent{ Task: t, Doer: doer, diff --git a/pkg/routes/caldav/listStorageProvider.go b/pkg/routes/caldav/listStorageProvider.go index 61586246c..19922c211 100644 --- a/pkg/routes/caldav/listStorageProvider.go +++ b/pkg/routes/caldav/listStorageProvider.go @@ -1144,7 +1144,11 @@ func (vlra *VikunjaProjectResourceAdapter) CalculateEtag() string { } } - return `"` + strconv.FormatInt(vlra.project.ID, 10) + `-` + strconv.FormatInt(latest.Unix(), 10) + `"` + etag := strconv.FormatInt(vlra.project.ID, 10) + "-" + strconv.FormatInt(latest.Unix(), 10) + if !vlra.project.LastPurgedAt.IsZero() { + etag += ";purge=" + strconv.FormatInt(vlra.project.LastPurgedAt.Unix(), 10) + } + return `"` + etag + `"` } // GetContent returns the content string of a resource (a task in our case) diff --git a/pkg/routes/caldav/sync_collection.go b/pkg/routes/caldav/sync_collection.go index 97eb92113..3fe793dbe 100644 --- a/pkg/routes/caldav/sync_collection.go +++ b/pkg/routes/caldav/sync_collection.go @@ -37,9 +37,7 @@ const ( syncTokenPrefix = "data:," ) -// parseSyncToken extracts the project ID and timestamp from a sync token -// of the form data:,"PROJECT_ID-UNIX_TIMESTAMP". -func parseSyncToken(token string) (projectID int64, ts time.Time, ok bool) { +func parseSyncToken(token string) (projectID int64, ts time.Time, purgeGeneration int64, ok bool) { // Quotes may sit outside or inside the data:, prefix. token = strings.TrimPrefix(token, `"`) token = strings.TrimSuffix(token, `"`) @@ -47,14 +45,23 @@ func parseSyncToken(token string) (projectID int64, ts time.Time, ok bool) { token = strings.TrimPrefix(token, `"`) token = strings.TrimSuffix(token, `"`) + if timestamp, generation, found := strings.Cut(token, ";purge="); found { + var err error + purgeGeneration, err = strconv.ParseInt(generation, 10, 64) + if err != nil || purgeGeneration <= 0 { + return 0, time.Time{}, 0, false + } + token = timestamp + } + if token == "" { - return 0, time.Time{}, false + return 0, time.Time{}, 0, false } // Split on the last '-' so a negative project ID can't break parsing. idx := strings.LastIndex(token, "-") if idx < 0 { - return 0, time.Time{}, false + return 0, time.Time{}, 0, false } projectIDStr := token[:idx] @@ -62,15 +69,15 @@ func parseSyncToken(token string) (projectID int64, ts time.Time, ok bool) { pid, err := strconv.ParseInt(projectIDStr, 10, 64) if err != nil { - return 0, time.Time{}, false + return 0, time.Time{}, 0, false } unixSec, err := strconv.ParseInt(tsStr, 10, 64) if err != nil { - return 0, time.Time{}, false + return 0, time.Time{}, 0, false } - return pid, time.Unix(unixSec, 0).UTC(), true + return pid, time.Unix(unixSec, 0).UTC(), purgeGeneration, true } // extractSyncTokenFromBody pulls the text content from the @@ -152,7 +159,6 @@ func handleSyncCollectionReport(c *echo.Context, body string, storage *VikunjaCa return echo.NewHTTPError(http.StatusInternalServerError, "internal server error").Wrap(err) } - // The etag is "PROJECT_ID-UNIX_TS"; the new sync token reuses it. newEtag := rr.CalculateEtag() newEtagClean := strings.Trim(newEtag, `"`) newToken := syncTokenPrefix + `"` + newEtagClean + `"` @@ -164,7 +170,7 @@ func handleSyncCollectionReport(c *echo.Context, body string, storage *VikunjaCa return writeSyncResponse(c, rr, rr.projectTasks, nil, newToken, includeCalendarData) } - tokenProjectID, tokenTS, ok := parseSyncToken(rawToken) + tokenProjectID, tokenTS, tokenPurgeGeneration, ok := parseSyncToken(rawToken) if !ok { log.Warningf("[CALDAV sync-collection] project=%d could not parse token %q → 403 valid-sync-token", projectID, rawToken) return writeForbiddenValidSyncToken(c) @@ -175,6 +181,14 @@ func handleSyncCollectionReport(c *echo.Context, body string, storage *VikunjaCa return writeForbiddenValidSyncToken(c) } + purgeGeneration := int64(0) + if !rr.project.LastPurgedAt.IsZero() { + purgeGeneration = rr.project.LastPurgedAt.Unix() + } + if tokenPurgeGeneration != purgeGeneration { + return writeForbiddenValidSyncToken(c) + } + // Tokens older than the soft-delete retention window cannot see deletions // whose tasks were already purged — force a full resync instead of // silently serving an incomplete delta. diff --git a/pkg/routes/caldav/sync_collection_test.go b/pkg/routes/caldav/sync_collection_test.go new file mode 100644 index 000000000..c46e8be26 --- /dev/null +++ b/pkg/routes/caldav/sync_collection_test.go @@ -0,0 +1,60 @@ +// 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 + +import ( + "testing" + "time" + + "code.vikunja.io/api/pkg/models" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSyncTokenPurgeGeneration(t *testing.T) { + updated := time.Unix(1_700_000_000, 0).UTC() + purged := updated.Add(time.Hour) + rr := VikunjaProjectResourceAdapter{ + project: &models.ProjectWithTasksAndBuckets{ + Project: models.Project{ + ID: 36, + Updated: updated, + LastPurgedAt: purged, + }, + }, + } + token := syncTokenPrefix + rr.CalculateEtag() + projectID, timestamp, generation, ok := parseSyncToken(token) + require.True(t, ok) + assert.Equal(t, int64(36), projectID) + assert.Equal(t, updated, timestamp) + assert.Equal(t, purged.Unix(), generation) + + _, _, generation, ok = parseSyncToken(`data:,"36-1700000000"`) + require.True(t, ok) + assert.Zero(t, generation) + + for _, token := range []string{ + `data:,"36-1700000000;purge=abc"`, + `data:,"36-1700000000;purge=0"`, + `data:,"36-1700000000;purge=-1"`, + } { + _, _, _, ok := parseSyncToken(token) + assert.False(t, ok) + } +} diff --git a/pkg/webtests/caldav_test.go b/pkg/webtests/caldav_test.go index 61f006350..09ef69063 100644 --- a/pkg/webtests/caldav_test.go +++ b/pkg/webtests/caldav_test.go @@ -1468,6 +1468,102 @@ END:VCALENDAR` }) } +func TestCaldavSyncCollectionAfterPurge(t *testing.T) { + deletedAt := time.Now().UTC().Add(-time.Hour).Truncate(time.Second) + purge := func(t *testing.T, taskID int64) { + t.Helper() + s := db.NewSession() + defer s.Close() + _, err := s.Unscoped().ID(taskID).Cols("deleted_at").NoAutoTime().Update(&models.Task{ + DeletedAt: deletedAt, + }) + require.NoError(t, err) + task := &models.Task{ID: taskID} + can, err := task.CanDeletePermanently(s, &testuser15) + require.NoError(t, err) + require.True(t, can) + require.NoError(t, task.DeletePermanently(s, &testuser15)) + _, err = s.ID(int64(36)).Cols("updated").NoAutoTime().Update(&models.Project{ + Updated: deletedAt, + }) + require.NoError(t, err) + require.NoError(t, s.Commit()) + } + syncReport := func(t *testing.T, e *echo.Echo, token string) *httptest.ResponseRecorder { + t.Helper() + body := `` + token + + `1` + rec, err := newCaldavTestRequestWithUser( + t, e, "REPORT", caldav.ProjectHandler, &testuser15, body, nil, + map[string]string{ + "project": "36", + }, + ) + require.NoError(t, err) + return rec + } + fullSync := func(t *testing.T, e *echo.Echo) string { + t.Helper() + rec := syncReport(t, e, "") + require.Equal(t, http.StatusMultiStatus, rec.Code) + response := struct { + SyncToken string `xml:"sync-token"` + }{} + require.NoError(t, xml.Unmarshal(rec.Body.Bytes(), &response)) + require.NotEmpty(t, response.SyncToken) + assert.NotContains(t, rec.Body.String(), "uid-caldav-test.ics") + return response.SyncToken + } + + t.Run("purge invalidates older and same-second tokens", func(t *testing.T) { + e, _ := setupTestEnv() + purge(t, 40) + for _, offset := range []time.Duration{ + -time.Second, + 0, + } { + token := fmt.Sprintf(`data:,"36-%d"`, deletedAt.Add(offset).Unix()) + rec := syncReport(t, e, token) + assert.Equal(t, http.StatusForbidden, rec.Code) + assert.Contains(t, rec.Body.String(), "valid-sync-token") + } + }) + + t.Run("full resync produces a usable token", func(t *testing.T) { + e, _ := setupTestEnv() + purge(t, 40) + token := fullSync(t, e) + rec := syncReport(t, e, token) + assert.Equal(t, http.StatusMultiStatus, rec.Code) + }) + + t.Run("purge generation leaves same-second live changes visible", func(t *testing.T) { + e, _ := setupTestEnv() + purge(t, 40) + token := fullSync(t, e) + s := db.NewSession() + _, err := s.ID(int64(41)).Cols("updated").NoAutoTime().Update(&models.Task{ + Updated: deletedAt, + }) + require.NoError(t, err) + require.NoError(t, s.Commit()) + s.Close() + rec := syncReport(t, e, token) + assert.Equal(t, http.StatusMultiStatus, rec.Code) + assert.Contains(t, rec.Body.String(), "uid-caldav-test-parent-task.ics") + }) + + t.Run("a second purge in the same second invalidates the resync token", func(t *testing.T) { + e, _ := setupTestEnv() + purge(t, 40) + token := fullSync(t, e) + purge(t, 41) + rec := syncReport(t, e, token) + assert.Equal(t, http.StatusForbidden, rec.Code) + assert.Contains(t, rec.Body.String(), "valid-sync-token") + }) +} + // TestCaldavPartialUpdate: properties a CalDAV PUT doesn't carry must survive the update. func TestCaldavPartialUpdate(t *testing.T) { const vtodoHeader = `BEGIN:VCALENDAR