- Add deletedAt field to IProject interface and ProjectModel
- Add restore() and getDeletedProjects() to ProjectService
- Add restoreProject() and fetchDeletedProjects() to project store
- Create ProjectsBin.vue page showing deleted projects with restore
- Add Bin link to sidebar navigation
- Update delete modal text to softer language (move to bin)
- Add undo toast action after deleting a project
- Add route for /projects/bin
- Add DeletedAt field to Project model with XORM soft-delete tag
- Replace hard delete with soft-delete in Project.Delete()
- Recursively soft-delete all descendant projects via CTE
- Add PermanentDelete() for actual cascade deletion (used by purge job
and user deletion)
- Add RestoreProject() to restore soft-deleted projects and descendants
- Add GetDeletedProjects() to list soft-deleted projects for a user
- Add background purge cron job (hourly) for projects past 30-day
retention
- Update user deletion to use PermanentDelete instead of soft-delete
349e6a590 renamed ListCreatedNotification's name from list.created to
project.created without rewriting the notifications.name column, so instances
upgraded from 0.19 or earlier still hold list.created rows carrying a full
project payload. Nothing can hydrate them, so nothing can scope them either,
and the read paths pass unrecognised names through unfiltered — leaking the
project's title, description, identifier, colour and owner to users since
removed from it.
Retaining rows no type can render only preserves that leak, so they go.
log.Warningf formatted a web.Auth with %v on the create, read, update and
delete denial paths. That holds a *user.User, which has a Password field
carrying the bcrypt hash and no String method, so every denied request wrote
a password hash into the log at warning level. Log the user id instead.
The capability interface carrying a notification's project was optional, and
ProjectIDOf defaulted a missing implementation to 0 — which means
account-scoped, which means always visible. So a new project-scoped type whose
author forgot the method would have leaked task titles, project names and
comment bodies to users with no access to the project, with no compile error
and no test failure.
Register now takes a factory returning PersistedNotification, which requires
the method. Registering is what makes a notification persist, so a stored row
that cannot be permission-checked no longer compiles. The three account-scoped
types say so by returning 0 explicitly instead of by omission.
Notification rows outlived access. A subscription survives a project being
unshared, so every notification already written for a revoked user stayed
readable — comment bodies, task titles, project names, deletion notices. The
read paths filtered on notifiable_id alone, with no permission check anywhere.
#3325 stopped the sender writing new ones; this is the other half.
The project a notification is about is persisted on the row when it is written
and the read paths filter on it in SQL, so LIMIT, OFFSET and total are all
computed on the filtered set. Notification types declare their project through
a capability interface in pkg/notifications, the same way they already declare
SubjectID, ThreadID and ToTitle — which is what lets the package below
pkg/models stay ignorant of what a project is.
project_id 0 means account-scoped and always visible, a positive value is
checked against the projects the caller can read, and -1 marks a project-scoped
row whose project could not be determined, so it is visible to nobody. Filtering
reuses the existing accessibleProjectIDsSubquery, so the page query and the
count cannot drift apart. A migration backfills existing rows from their stored
payloads, resolving through soft-deleted tasks so task.deleted rows still land
on their project.
Covers every read path: the v1 and v2 list endpoints, mark-as-read (which
echoes the payload back), the Atom feed, and the websocket push — the last of
which is load-bearing, since a row is still written for a revoked subscriber.
Deliberately no instance-admin bypass: notifications are always the caller's
own, and being an admin says nothing about whether they should still read a
comment out of a project they were removed from.
Subscriptions outlive access: nothing purges them when a project is
unshared, and access can change with no revocation event at all, so a
user who can no longer open a task kept receiving its comment bodies,
assignment details and deletion notices by mail and in the feed.
Filter subscribers by current read permission when the subscription is
fetched, so every listener is covered by one check. Rows are kept rather
than deleted - a subscription is user intent and resumes if access does.
GetSubscriptionsForDeletedTask keeps its own lookup because a
soft-deleted task cannot be resolved back to its project, but it now
reuses the same filter with the project id it already holds.
sortParentsBeforeChildren marked a task as placed only after recursing
into its parent, so a parentId cycle in an uploaded TickTick export made
place() recurse forever. That is a Go stack overflow, a runtime fatal
error the recover middleware cannot catch, so a two-line CSV from any
authenticated user took down the whole process.
Track a tri-state per task and mark it before recursing, which breaks
the cycle. Acyclic input is unaffected.
The filter preprocessing replaced " in ", " not in " and " like " with their
fexpr sigils using blind whole-string replacements, corrupting any value that
happened to contain those words: `title like 'stuff in progress'` became
`title ~ 'stuff ?= progress'`, so the filter matched the wrong tasks or failed
to parse with no hint as to why.
Walk the filter instead and skip over quoted runs, matching fexpr's own
scanner: both ' and " open a string and a backslash escapes the next
character. An unclosed quote is treated as an ordinary character so bare
values with an apostrophe keep working. " not in " is still matched before
" in " so the longer operator wins.
ProjectInfo re-permits the target attribute via ADD_ATTR, so a link in a
project description could open a page that keeps a live window.opener
handle to the Vikunja tab and navigate it elsewhere.
Force rel via an afterSanitizeAttributes hook rather than allowing it
through ADD_ATTR, so a description supplying its own rel cannot drop
noopener.
GET /api/v2/health passed the raw error from health.Check() into
huma.Error500InternalServerError, which serialized it into the response
body. The endpoint is public, so a routine DB or Redis outage exposed
internal hostnames, private IPs, ports, the DB technology and DB usernames
to anyone who asked:
{"title":"Internal Server Error","status":500,
"detail":"Internal server error",
"errors":[{"message":"dial tcp 127.0.0.1:6390: connect: connection refused"}]}
Log the cause with context instead and return a bare generic 500.
The 5xx sanitizer lived in the NewErrorWithContext override, but huma's
Error5xx* helpers (Error500InternalServerError and friends) call NewError
directly, so anything built through them kept the raw cause in the
problem+json `errors[]`. Huma's dispatch loop then writes an already-built
StatusError as-is, so the framework could not re-sanitize it either.
Move the strip into NewError so every 5xx passes through it by construction
and no future handler can reintroduce the leak. NewErrorWithContext is now
left at huma's default, which delegates to NewError — keeping both would log
the same cause twice.
4xx errors keep their details, including ErrorDetailer locations and the
Vikunja `code`/`i18n_params` fields. Huma's registration-time schema probe
calls NewError(0, ""), which is below the threshold and unaffected.
Around 38 events are registered for audit logging, including every admin
action, but a full personal data export left no trace at all. It is
dispatched from both v1 and v2, so one registration covers both.
The license servers are hardcoded, but the check client followed redirects
without any policy and dialed without the SSRF guard, so a hijacked or
poisoned license host could forward the license key to an internal address.
Redirects are refused outright rather than capped: the check is a POST to a
fixed JSON API that never redirects.
avatar.gravatarbaseurl is operator-configurable, so the request destination was
never a fixed constant - pkg/utils/avatar.go already uses the SSRF-safe client
for the same job. The previous 5s timeout is kept as a context deadline so the
configured (30s by default) client timeout does not apply here.
doGet built its own http.Client, unlike its siblings in the same file which
already use utils.NewSSRFSafeHTTPClient(). The previous 10s timeout is kept as
a context deadline so the configured (30s by default) client timeout does not
apply here.
The webhook target URL is user-configured, so a hostile target can answer
a delivery with a 4xx/5xx carrying an arbitrarily large body. That body
was read whole into memory and written whole to the log.
Cap the read at 4KiB, which is plenty for a diagnostic log line, matching
the LimitReader already used for the license server response.
v1 only registers /login when local or LDAP auth is enabled, but v2
registered it unconditionally. With auth.local.enabled=false a
pre-existing local password still authenticated on /api/v2/login.
/logout stays unconditional - it terminates any session, OIDC included.
The job pushes with the SSH deploy key configured by actions/checkout's
ssh-key input, so GITHUB_TOKEN is never used for writes. Scope it down to
contents: read.
GetTokenFromTokenString sliced token[len(token)-8:] without checking the
length, so any string with the "tk_" prefix but shorter than 8 characters
panicked with "slice bounds out of range".
The helper is reachable with attacker-controlled input from three
unauthenticated call sites: the main API bearer-token middleware
(/api/v1 and /api/v2) and CalDAV basic auth on /dav/ and
/.well-known/caldav. Each request was turned into a 500 by the global
panic recovery, at the cost of a full stack unwind, an error-level log
line and a Sentry event. On the main API the token middleware also runs
before the rate limiter, and the CalDAV routes are not rate limited at
all.
The same guard already existed in the feeds auth path; fixing it at the
shared choke point closes all sites at once. Real tokens are the prefix
plus 40 hex characters, so no legitimate token is affected — a too-short
token now gets the same rejection as any other invalid one.
The listener runs after the deleting transaction committed, so the task
is already soft-deleted when it looks up who to notify. Every task
subscription lookup filters `t.deleted_at IS NULL`, so the subscriber
list came back empty and nobody was notified - not even users with full
access.
The `IsErrTaskDoesNotExist` fallback to project subscribers never
covered for this: the lookup returns an empty slice with a nil error, so
the branch could not fire. Removed rather than repaired - with
soft-deleted tasks included, the CTE resolves project and parent-project
subscriptions on its own, which is strictly more than the fallback did.
Soft-deleted tasks are opt-in, so the reminder crons keep ignoring them.
Permissions come from the project, since the task can no longer carry
them - a subscriber who lost access still gets nothing.
kolaente
renovate[bot]andkolaente
Frederick [Bot]
TinkandGitHub