The guard on Webhook.CanRead is unreachable: no route exposes a read-one
webhook, and DoReadAll never calls CanRead. Two paths were left open:
- the v2 user-webhook list passes a.GetID() into Webhook.UserID, which is
negative for a link share, so the w.UserID > 0 branch and its link share
check were skipped and the request fell through to the project branch with
project id 0, returning 404 instead of 403.
- the project branch never rejected link shares at all, so any holder of a
public share link could list the project's webhooks. target_url is a bearer
secret for Slack, Discord, Teams and Zapier.
Guard both by rejecting link shares at the top of ReadAll.
web.Auth is satisfied by both *user.User and *LinkSharing, so returning the
raw positive share.ID made a share with id N indistinguishable from the user
with id N at every permission check comparing against a users.id column.
The rest of the codebase already keys shares negatively
(getUsersOrLinkSharesFromIDs, toUser), so this makes GetID consistent with
that contract instead of an exception to it.
Switching an existing view to kanban left bucket_configuration_mode at
none, so the tasks endpoint returned a flat task list which the
frontend rendered as empty bucket columns.
The mode is now normalized on create and update: a kanban view without
a mode becomes manual, a non-kanban view loses its mode, and an update
which omits the mode keeps the stored one together with its bucket
configuration. Becoming a manual kanban view seeds the default buckets
and backfills task_buckets rows for tasks which have none in that view,
so tasks created while the view was of a different kind stay visible.
Bucket ids from the request are validated against the view: an id of a
bucket which is gone resets to zero instead of locking the view, an id
belonging to another view is rejected, and non-kanban views no longer
write those columns at all so a round trip can restore them.
The backfill selects only task ids, scopes saved filter views to the
projects their owner can see, batches its inserts and ignores conflicts
with concurrently placed tasks.
Fixes https://github.com/go-vikunja/vikunja/issues/3386
POST /api/v2/projects/{project}/tasks/bulk creates up to 100 tasks in
one atomic request. The URL project wins over any body project_id, and
rich-text descriptions honor the format query param like single create.
Bucket limits are only enforced for explicitly provided bucket ids.
The route files under the tasks API-token group as create_bulk,
matching the v1 bulk naming, so a token scoped to tasks can use it.
The resource-heavy saved-filter evaluation moves from TaskCreatedEvent
to the batch event: filters, views and the fallback timezone are loaded
once per batch, then every task is evaluated in one pass. Buckets and
positions are inserted per task so a mid-loop position recalculation
sees earlier members' rows.
Creates up to 100 tasks in one project atomically. One project write
check covers the batch; any invalid task rolls the whole batch back
with error 4031 naming the offending payload index, and an
invalid batch size returns 4030. The 1..100 bound is declared as
minItems/maxItems in the schema (pinned to the constant by a test) with
the model check as backstop for non-HTTP callers. Payload positions are
zeroed — positions are always calculated server-side.
createTask becomes a wrapper around createTasks, which creates a whole
batch in one pass: project, creator, views and default buckets are
looked up once, indexes are assigned from a single max-index query
(preset indexes kept when free, collisions get the next free one), and
explicitly provided buckets are resolved once per distinct bucket —
verified to belong to the target project and checked against their
limit with the batch's own members counted. The row insert stays per
task because multi-row inserts don't reliably return autoincrement ids
on all supported databases.
Validation errors always carry the payload index; the single-create
wrapper unwraps them so existing callers keep their raw error types.
A new TasksBatchCreatedEvent fires once per batch (single create is a
batch of one) for listeners which can process all new tasks in one
pass; per-task TaskCreatedEvent semantics stay unchanged for webhooks,
mentions and audit.
Split for reviewability: builds together with the follow-up commit
adding the BulkTaskCreation model and error types.
calculateNewPositionsForTasks places a whole creation batch on top of a
view with a single lowest-position query: evenly spaced below the
current lowest, payload order preserved, no collisions by construction.
When the spacing trips a full recalculation, the batch's own freshly
inserted rows are snapshot-scoped and removed again afterwards so the
queued top positions land while rows written earlier in the creation
survive.
It replaces the two previous implementations of the same idea:
calculateNewPositionForTask is now a one-element wrapper and the
saved-filter healing path calls it directly. The empty-view default
falls back to payload-order spacing when task indexes repeat or are
zero, since index-derived defaults collide in saved-filter views
spanning projects.
Users with access to parent project could remove labels from tasks in
child projects but not add them back — 403 "Tried to create while not
having the permissions for it". Label attach check used direct shares
only; task write permission and label picker both walk project
hierarchy. Fix: label access now uses same recursive subquery.
Reported: https://community.vikunja.io/t/permissions-on-labels/4460
## How to verify
1. As user A, create a parent project with a child project, and add a
task in the child project.
2. Share the parent project with a team that has write access and
contains user B.
3. As user A, add a label to the task in the child project.
4. As user B, open that task, remove the label, then try to add it back.
5. **Expected:** the label can be added again; the API returns 201.
**Before this PR:** step 4 failed with 403 even though user B could edit
the task and remove the label.
Co-authored-by: kolaente <k@knt.li>
updateDone reschedules due/start/end dates and resets the description checklist
after colsToUpdate was frozen from the caller's field list, so a restricted update
computed the next occurrence and threw it away. Affects BulkTask.Update, the
remaining caller that passes a field list.
TeamProject.ReadAll used db.ILIKE for the main query but a
case-sensitive LIKE for the count query, so on postgres the total
count could disagree with the returned rows. Use db.ILIKE for both.
SubscriptionEntityType is an int in Go but its custom MarshalJSON emits
"project"/"task" strings. Huma reflected it as an integer schema, so
AutoPatch's GET-to-PUT echo failed request validation with
"subscription.entity: expected integer" whenever the requesting user had
a task or project subscription, blocking all v2 task and project PATCH
writes for subscribed users.
Add a Schema override reflecting it as a string enum, same pattern as
ProjectViewKind and BucketConfigurationModeKind.
Fixes#3316
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.
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.
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 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.
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.
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.
The unshadowed promoted method made LinkSharing's value method set non-empty, which breaks Huma's $schema wrapper for response bodies embedding it (reflect.StructOf, go#15924).
The fuzzy mirror test is ParadeDB-only and fails without the previous
commit (child returned as a second root). The multi-parent test pins the
NOT EXISTS semantics from #3197: one in-scope and one out-of-scope
parent must still nest the child.
Part of #2954
The parent root-membership mirror always used the ILIKE fallback, so on
ParadeDB a parent matching the search only via fuzzy/token matching kept
its child as a duplicate root, and a substring-only ILIKE match could
hide a child under a parent absent from the results. The assumed
limitation no longer holds: pg_search plans ||| against the aliased
parent_tasks inside the correlated NOT EXISTS as a regular BM25 scan
(verified on 0.24.1), and score-free predicates never triggered the
"unsupported query shape" error on older versions either.
Part of #2954
Five PUT-routed "create" endpoints document @Success 200, but they all
dispatch through the shared CreateWeb handler (pkg/web/handler/create.go),
which always responds 201 Created. Every other CreateWeb-routed endpoint
(labels, teams, filters, task relations/comments/assignees/duplicate,
task-labels) already correctly documents 201 - these five are stragglers
with the same stale annotation.
Affected endpoints:
- PUT /tokens
- PUT /projects/{id}/views/{view}/buckets
- PUT /projects/{id}/webhooks
- PUT /{kind}/{id}/reactions
- PUT /projects/{project}/views
Doc-only change, no behavior change. Regenerated pkg/swagger/* via
`swag init -g ./pkg/routes/routes.go --parseDependency -d . -o ./pkg/swagger`
(the command mage generate:swagger-docs runs) so the generated docs match
the annotations.
Parametrised error translations (invalid api token permission, invalid
timezone, invalid claim data) rendered their placeholders as empty text
because the backend only baked the values into the English message and
the frontend translated the error code without any interpolation params.
Carry the values as i18n_params on error responses, keyed by the
placeholder names of the frontend's error translations, on both v1 and
the v2 problem+json body, and pass them through when translating the
toast. A 14002 toast now reads e.g. "The permission time_entries of
group tasks is invalid." instead of "The permission of group is
invalid."
kolaente
Mauandkolaente
maximilizeandGitHub
Pierre Christenandkolaente