- 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.
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.
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.
Asserting only "not 500" let the test pass on a 404, 401 or 403, so it
would have kept passing if the public v2 endpoints broke another way.
Assert 200 per path plus the remaining budget counting down across them,
which is what proves both unauthenticated requests share one ip key.
The default branch of the kind switch logged the misconfiguration and
then continued with an empty key, putting every request of the whole
instance into one shared bucket. Fall back to per-ip limiting instead,
so a typo in ratelimit.kind degrades to the "ip" behaviour.
With ratelimit.kind at its default value of "user", the rate limit
middleware logged the error from GetAuthFromClaims and then dereferenced
the nil web.Auth anyway. Every unauthenticated /api/v2 request produces
exactly that state, since v2 attaches the limiter to the single group
serving its public routes too - so enabling rate limiting turned
/api/v2/info, /api/v2/health and /api/v2/login into 500s. v1 is
unaffected because it splits its unauthenticated routes into their own
ip-keyed subgroups before the "user" limiter is attached.
Fall back to keying by IP, matching the "ip" kind and v1's
unauthenticated groups. Authenticated requests are unchanged.
service.secret had an unconditional random default, and viper's IsSet
reports defaults as set. The deprecation branch therefore always took the
"both keys are set" path and never copied service.jwtsecret over, so
instances configured with the old key silently got a fresh random secret
on every start — flaky 401s across replicas.
The random fallback is now generated after the config file and env are
read, so an empty service.secret unambiguously means "not configured".
Recreates model-declared indexes that are missing, matching by column
set so converted databases with differently-named equivalent indexes
are left alone. Fails with an actionable message if duplicate values
block recreating a unique index.
Fixes#3244
xorm's Sync drops every index and unique constraint the synced struct
doesn't declare (#3244). Adds a partialSync helper (no drops) and
converts all 65 existing-table struct syncs to it — the four v2.4.0
migrations that wiped users/tasks indexes on every upgrade, plus every
historical one back to 2019, each a latent index wipe and a pgloader
2BP01 abort point for installs upgrading from versions predating it.
xorm connects lazily, so nothing touched the database until the ParadeDB probe
ran, and a connection failure surfaced as "could not check for paradedb
extension" followed by "Schema check failed: could not determine the current
schema" - which is how #3287 came in labelled as a schema bug. Ping right after
the engine is built and let the existing "Could not connect to db" callers report
it. The ParadeDB probe moves up alongside it so it no longer runs first.
Route-level tests on both api versions, since the pseudo-projects only get
their parent through the read path. The v2 read body embeds models.Project and
Huma's $schema wrapper copies its fields into a generated struct, so a custom
MarshalJSON on the model would not have applied there — assert the raw JSON
instead of the struct.
781ffac19 (GHSA-44v6-7fxq-vgf4) turned ParentProjectID into a *int64 with
`omitempty` so a write can tell an omitted field from an explicit 0 (detach,
admin only). That also dropped the key from every response where the pointer is
nil: the Favorites and saved-filter pseudo-projects, which are built in Go and
never set it, plus rows whose nullable parent_project_id column was never
backfilled. Clients parse the field as a plain int, so the whole project list
fails to decode (go-vikunja/app#295).
Keep the pointer and its three-way write semantics, but never hand out a nil:
normalize DB rows in AfterLoad and set the field where the pseudo-projects are
built.
Closesgo-vikunja/app#295
Both the listener and the cron bailed out completely as soon as a single saved
filter errored, so no filter view in the instance got updated. Skip the failing
filter instead. Genuine db errors still bubble up so the retry middleware can
do its job.
Evaluating a saved filter needs the list of projects its owner has access to,
which getRawProjectsForUser refuses to return for disabled or locked accounts.
Since the listener walks every manual kanban filter view in the instance, a
single filter of a disabled user made the whole handler fail with "Account is
disabled" - for every task update, for everyone. The recalculation which would
un-crowd the positions is the very thing that failed, so the state never healed.
Every task creation looks for rows with the same position in the view to
resolve conflicts. Without an index on position that scans all positions of
the view, so creating n tasks in one project costs O(n²) row reads - the
second half of the slow import in #3297.
Tasks imported without a position are inserted in front of the lowest one by
halving it. That reaches the minimum spacing every few dozen inserts, which
recalculates every position in the view and makes a large import O(n²) - a
10k task TickTick export with an empty Order column never finished.
Numbering the tasks up front skips the halving entirely and keeps the order
the export was written in.
Closes#3297
Registers the full v2 API and fails on Huma's 'unable to create schema link' stderr warning, catching any future response body type Huma cannot wrap. Fixes#3272.
kolaente
Frederick [Bot]
TinkandGitHub