Both refresh endpoints answered a missing cookie with a code-less 401, so
clients couldn't tell it apart from a rotated-away token, which is worth one
retry. ErrNoRefreshToken gives it its own code.
Refs #4023
ListParams defaulted per_page to 50 and allowed up to 1000 regardless of the instance setting. per_page now defaults to and is silently capped at service.maxitemsperpage, like /api/v1, and the response envelope reports the effective value.
A file import ran inside the upload request. A big export takes minutes,
far longer than a reverse proxy holds a request open, so the client got a
502 while the import kept running and kept the user's migration slot, and
there was no way to cancel or retry it.
The upload request now only validates what is cheap to validate, claims
the slot, spools the upload into a per-process temp directory and queues a
FileMigrationRequestedEvent. FileMigrationListener runs the import through
the lifecycle it now shares with the OAuth migrations: stale event skip,
panic recovery, heartbeat, outcome and notifications. The vikunja-file
migrator rejects a non-zip or structurally broken export before queueing;
ticktick, wekan and csv would have to parse the whole upload, so they fail
in the job and report through the status and failure mail. The csv
migrator receives its column mapping as options carried with the event.
Registered after apiv2.RegisterAll so the tool index sees the AutoPatch
operations. Routes under the prefix skip the token route check: the
endpoint checks mcp:access itself and every loopback call is checked on
its own route.
The v1 name leaked into the v2 OpenAPI document and everything derived from
it. Every other /tasks/... route already uses {task}. The expand-scope check
is keyed by echo path template, so its entry moves too, and a webtest now
fails if any of its keys stops naming a registered route.
A by-index address that no longer resolves falls back to the alias table
and answers 307 with the task's current address and Cache-Control:
no-store. The fallback requires read access to the project in the URL and
to the target task, using the same 403 body as DoReadOne.
encoding/json reports a document that ends mid-value as io.ErrUnexpectedEOF
rather than a *json.SyntaxError, so those still reached the error handler as a
500.
A WeKan JSON export saved with a UTF-8 BOM made encoding/json return a raw
*json.SyntaxError from the migrator, which the shared error handler turned into
a 500 and forwarded to Sentry. Map decode failures of the uploaded file to a
400 in RunFileMigration (shared by v1 and v2), and skip a leading BOM so an
otherwise valid export imports instead of being rejected.
Fixes API-CLOUD-4B
The overview's task query filters `project_id IN (...) AND done = false` and
sorts by due_date. `done` is not selective, so with an index on
(done, due_date) the planner walks it in due_date order and heap-probes every
open task in the database just to test project_id.
Leading with project_id lets the scan seek to the user's projects and top-N
sort a small set instead.
The old index is dropped rather than kept: its only other consumer is the daily
overdue-reminder cron, whose selective predicate is the due_date range that
IDX_tasks_due_date already covers.
ProjectID moves above Done in the Task struct because xorm orders composite
index columns by struct field order, which also shifts project_id earlier in
the serialized JSON — hence the churn in the task collection test's expected
response bodies.
Preflight over the declared uncompressed sizes (overflow-safe), a file
count cap and a per-user storage quota bound the import before
anything is read; the actual decompressed bytes are still counted
against the same budget while reading. Attachments and backgrounds
now stream through a lazy provider: one zip entry is decompressed
into a private temp file at a time. A failed import deletes the file
blobs it wrote, which were orphaned for every migrator before.
Decoding now validates image dimensions before allocating pixels
(imaging.Fit replaces unbounded Resize so neither side can blow up),
backgrounds decode once and reuse the image for the blur hash, and
DownloadImage is bounded at the configured max file size. Small sources
now keep their native size (Fit does not upscale); the frontend scales
via CSS.
The v2 admin API has every user action (create, promote, status,
password, delete) but not the list itself — the admin users view still
had to call v1 for `GET /admin/users`. This blocks moving that view onto
a generated v2 client.
The list query and its audit event move from the v1 route layer into
`models.ListUsersAsAdmin` (next to the other `*AsAdmin` actions); v1
delegates to it, v2 registers `admin-users-list` on top.
## How to verify
1. As an instance admin on a licensed instance, run `curl -H
"Authorization: Bearer $TOKEN"
https://instance/api/v2/admin/users?q=user2`
2. **Expected:** a paginated envelope (`items`, `total`) of users
matching "user2" in username or email, including the admin-only
`is_admin` and `status` fields, and an `admin.users.listed` entry in the
audit log.
3. As a non-admin user, request the same URL.
4. **Expected:** 404.
**Before this PR:** `/api/v2/admin/users` returned 404 for everyone;
only v1 had the list.
---------
Co-authored-by: kolaente <k@knt.li>
GET /labels ran the label list as `labels LEFT JOIN label_tasks ... WHERE
label_tasks.task_id IN (SELECT id FROM tasks WHERE project_id IN (<every
accessible project>)) GROUP BY labels.id`, fanning every label out over its
label_tasks rows only to collapse them again, and then repeated the whole
thing as a `count(DISTINCT labels.id)`.
The label list is now a plain select over labels whose task branch is an
uncorrelated `labels.id IN (SELECT label_id FROM label_tasks INNER JOIN
tasks ...)` subquery. The count query only runs when the returned page is
full, since a short page means the offset plus what we got is already the
total. The same applies to the per-task query when it is unpaginated, which
is how task lists load their labels.
The old query resolved its projects through getRawProjectsForUser, which
leaves out archived projects, so a label used only on tasks in an archived
project was missing from the list even though CanRead - and therefore
GET /labels/{id} - happily returned it. Reading the accessible projects
from the session access memo drops that accident and makes the list agree
with the permission check.
Covers unsubscribing a task that has both its own subscription and a subscribed
project, unsubscribing after lifting an opt-out, an opt-out outliving the
subscription it was made against, being assigned not lifting one, and removing
an own row after losing access leaving no opt-out behind.
Assertions pin the stored rows rather than only how a subscription resolves, so
a stray or duplicated row fails, and the read-access case now needs the
permission check to pass - it previously stayed green without it. The v2 test
decodes the task instead of matching substrings against the whole body.
Deleting a subscription that resolves through a parent project used to delete
nothing and fail the permission check, leaving the user subscribed with no way
out.
Delete now follows one rule - leave the entity resolving to unsubscribed. It
drops every row the user holds for the entity, re-resolves, and stores an
opt-out only when an ancestor is uncovered, so an own row shadowing a parent can
no longer resurrect the subscription either. The opt-out is written only for an
entity the caller can still read, while removing your own row stays possible
after losing access. Subscribing again lifts the opt-out and refreshes its
timestamp.
Muted is kept out of request bodies: json:"-" does not cover echo's XML binder,
which matches exported field names.
Both helpers compared a.GetID() against users.id, but *LinkSharing also
satisfies web.Auth and its GetID() is a link_shares.id - a different id
namespace. Passing one would have emitted users.id = <share id> and lent
that share another user's identity, which is the confusion behind
GHSA-vvcv-vpph-h844. Guarded at every call site, but the helpers moved
out of models and into user, so the guard drifted away from the code
needing it. Taking *User makes the case unrepresentable.
Also fail closed on a zero id: humans store bot_owner_id = 0, so an
unguarded zero would have matched all of them.
The doc claimed JWT and CalDAV auth carry no BotOwnerID, but bots can
reach neither path, and claimed bots-cannot-own-bots as an invariant when
the guard is inert for JWT principals. Reworded to chain-safety by
construction, which holds regardless.
The label access condition covered owner-to-bot and bot-to-owner, so two bots
of the same human could not use each other's labels: bot A seeded a label and
bot B got a 403 on it.
Replacing it with SameBotIdentityCond resolves every caller to an identity root
- a bot's owner, or a human themselves - and matches everyone under that root,
which covers all three directions in a single condition. Write access is
unchanged: only the creator and the human owner can update or delete a label.
Search now matches inherited projects, so the old counts are wrong: ILIKE returns
11 for Test1 where it returned 6, and ParadeDB's fuzzy match returns whatever it
returns — a number nobody can verify without ParadeDB running.
Assert the property the change is about instead. Nothing outside the user's
accessible set may come back, and the ids the substring match pins must be there.
v1 refresh sits behind tokenRefreshRateLimit, a per-IP floor that applies
even with the global limiter off. The v2 endpoint had no floor at all, and
the frontend now uses it.
address pr-swarm finding
Only an expired session or a disabled account invalidates the cookie. A transient failure must not clear it, and neither must a token rotated away by a concurrent refresh: that would delete the cookie the winning refresh just set.
The cookie was path-scoped to /api/v1/user/token/refresh only, so browsers never sent it to the v2 endpoint and every v2 refresh returned 401. One cookie per API version path now, with the route literals shared as consts.
Permission's zero value is PermissionRead, so Project.MaxPermission serialized
as 0 — a real permission meaning read — on every response path that never
resolved it. GET /api/v1/projects/:project claimed read-only access on a
project you own while the x-max-permission header correctly said 2.
Typing the field *Permission makes that unrepresentable: nil marshals to null,
so "not computed" is the default rather than something each call site has to
remember. That also fixes the paths which serialize a project outside the CRUD
pipeline and had the same lie — the admin project list, the background
handlers, duplicated_project on both API versions, and the admin owner-reassign
route — and retires v2's two explicit resets, which the field type now covers.
Webhook payloads for project.created and project.updated change from 0 to null
along with it.
Fixes#3574
Label with zero task attachments unreachable by anyone but creator, so
label seeded by human unusable by that human's own bot — every attach
403s forever until someone else attaches it once.
Refs #3592
`hasAccessToLabel` grants non-creators access only through existing
`label_tasks` row on readable task. Bot owners already inherit access to
labels their bots created (c9c2c58c1); this adds missing reverse
direction so both sides of bot/owner pair share label access.
Human-to-human access unchanged: another user's labels stay invisible
until they show up on a task you can read — the restriction
GHSA-hj5c-mhh2-g7jq introduced. Bots inherit read/attach only; renaming
and deleting the owner's labels still requires being the owner.
### Scope: does not close#3592
Deliberately `Refs`, not `Fixes`. The issue also reports the
human-teammate case — someone you shared a project with still cannot
attach a label that has never been used. That stays broken here, because
the fix the issue proposes for it (allow attach whenever the caller can
write the target task) would let any user attach any label id to a task
they own and read the title back, re-opening GHSA-hj5c-mhh2-g7jq in a
new shape. Whether labels should become project-scoped or explicitly
shareable is a design question worth its own issue — leaving #3592 open
to track it.
Note: the commit trailer still reads `Fixes #3592`, so adjust the
message on squash-merge if you want the issue left open.
### Operator note: widened bot/owner scope
A bot token can now enumerate every label its owner has created via `GET
/api/v2/labels`, including labels only used on projects the bot was
never given access to, and can attach one to a task in any project it
can write (which then makes that label visible to that project's
members). Read-only — rename and delete still require being the owner.
Worth knowing if you hand bot tokens to third-party integrations.
## How to verify
1. As a human user, create a label and do not attach it to any task:
`POST /api/v2/labels {"title":"seeded"}` → note the returned id `N`.
2. Create a bot owned by that user (`PUT /api/v1/user/bots`), issue an
API token owned by the bot with the `tasks_labels` create scope, and
give the bot access to a project.
3. As the bot, create a task in that project, then attach the label:
`POST /api/v2/tasks/{taskID}/labels {"label_id": N}`.
4. **Expected:** the request returns 201 and the label appears on the
task. `GET /api/v2/labels` as the bot also lists label `N`.
**Before this PR:** step 3 returned 403 on every attempt, and label `N`
was missing from the bot's label listing, until some other user attached
the label to a task first.
5. As a second, unrelated user (not the bot's owner), attach the same
label to a task you can write. **Expected:** still 403 — the label
remains private to its creator until it is visible through a shared
task.
6. As the bot, try to rename and delete label `N` (`PUT` and `DELETE
/api/v2/labels/N`). **Expected:** 403 on both.
---------
Co-authored-by: kolaente <k@knt.li>
Buckets, webhooks, backgrounds and task duplication checked write access
through Project.CanUpdate on a bare stub, whose zero IsArchived matched the
un-archive carve-out. With is_archived now set on descendants too, that
made whole archived subtrees writable. Use CanWrite instead.
Enforced in UpdateProject, the single write path, instead of CanUpdate,
which short-circuits for instance admins and is bypassed by direct
callers. Callers get a dedicated ErrParentProjectIsArchived (3016)
telling them to un-archive the parent first.
is_archived is written down the whole subtree on archive/unarchive and
backfilled for pre-existing rows, so CheckIsArchived, ReadOne and the
project list CTE no longer need to derive it from ancestors. The list
CTE previously only saw ancestors the user could access, so list and
single reads could disagree. task_overdue_reminder already trusted the
column and is now correct for old data too.
Credentials migrators are verified synchronously before the async migration is
queued (CredentialsChecker). Routes live on v2 only; the listener learns the
migrator via a route-free RegisterMigratorForEvents, which the oauth v2
registrar uses as well. Shared status/migrate registration moved to
migration_shared.go.
Task uids are client chosen — the api only generates a uuid when the
field is empty (`pkg/models/tasks.go`), and the CalDAV parser stores
whatever the inbound VTODO carried (`pkg/caldav/parsing.go`). That value
went into hrefs verbatim, which lets a uid forge a path or inject
markup.
Split out of #3551 so the path-forgery half gets its own review.
## What goes wrong today
**Path forgery.** A uid containing a slash makes the href appear to live
in another collection:
```
UID:evil/../../../projects/5/y → <D:href>/dav/projects/36/evil/../../../projects/5/y.ics</D:href>
path.Clean → /dav/projects/5/y.ics
```
Anyone who can create a task in a project the victim can see can plant
one; it then renders inside the victim's collection listing pointing
elsewhere. Same class as GHSA-48ch-p4gq-x46x.
**XML injection.** `sync_collection.go` wraps hrefs in `xmlEscape`, but
PROPFIND and calendar-multiget hand `Resource.Path` to caldav-go's
`ixml.HrefTag` → `ixml.Tag`, a plain `Sprintf`. `ixml.EscapeText` sits
in the same file and is only used for prop content. A uid with `<`, `>`
or `&` therefore lands raw in the multistatus body.
Input validation cannot close either: RFC 5545 §3.3.11 puts `<`
(`%x3C`), `>` (`%x3E`) and `&` (`%x26`) all inside `TSAFE-CHAR`, so they
are legal in a TEXT value.
## The fix
Percent-encode the uid to RFC 3986 `unreserved` when building an href.
`url.PathEscape` is not enough — it leaves sub-delims alone, so `&`
survives:
```
url.PathEscape("a&b<c>d") → "a&b%3Cc%3Ed"
```
Encoding conservatively means no XML metacharacter reaches the document,
so no change to the vendored library is needed. Existing uuid uids are
entirely `unreserved`, so their hrefs are byte-for-byte unchanged and no
client resyncs.
Then decode on the way back in — neither side did:
- echo v5 hands back the still-encoded path segment (`c.Param("task")` →
`evil%2F..%2F%3Cx%3E`), so `TaskHandler` needed the decode.
- `GetResourcesByList` parses hrefs out of the REPORT body and never
decoded, so encoded hrefs the server itself emitted would silently stop
matching and tasks would vanish from multiget responses.
One more, found while checking the other call sites: caldav-go builds
`Resource.Path` from `request.URL.Path`, which Go has already decoded,
and writes it into XML unescaped. So a PROPFIND against the encoded href
echoed the forged form right back:
```
PROPFIND /dav/projects/36/evil%2F..%2F..%2F..%2Fprojects%2F5%2Fpwned%3Cx%3E%26y.ics
→ <D:href>/dav/projects/5/pwned<x>&y.ics</D:href>
```
Task requests now carry a canonical href that the storage returns
instead of echoing the client's path.
## Not fixed here
Principal hrefs interpolate the username unescaped, and
`pkg/user/user_create.go` only rejects spaces and the link-share
pattern. `isOwnPrincipalPath` limits this to the authenticated user's
own username, so it never crosses a user boundary — separate concern,
not uid-related.
## How to verify
1. Create a task over CalDAV whose VTODO carries
`UID:evil/../../../projects/<other project id>/pwned<x>&y` in a project
you own.
2. Run `curl -u <user>:<caldav-token> -X PROPFIND -H 'Depth: 1'
https://<instance>/dav/projects/<that project id>/`
3. **Expected:** the `<D:href>` for that task is percent-encoded, stays
under `/dav/projects/<that project id>/`, and the response body parses
as XML.
**Before this PR:** the href resolves to the other project's collection
and the body is malformed XML.
1. `GET` that percent-encoded href.
2. **Expected:** 200 with the task's VTODO — the uid round-trips.
1. Send a `calendar-multiget` REPORT listing that same href.
2. **Expected:** 207 containing the task.
1. Take any pre-existing task with a normal uuid uid and PROPFIND its
collection.
2. **Expected:** its href is unchanged from before this PR — encoding is
a no-op for uuids, so no client is forced to resync.
---------
Co-authored-by: kolaente <k@knt.li>
Huma's AutoPatch implements PATCH as an internal GET + PUT re-dispatched through
the router, so both legs re-enter the API token middleware. Scoping the GET leg
like a client request made every /api/v2 PATCH additionally demand the
resource's read_one permission, so a token scoped to tasks: update could never
patch a task.
Mark the re-dispatched requests with the route the client request was matched
against, and skip the scope check only for a GET leg that carries no query
string and resolves to that exact route. Anything looser is exploitable: echo
routes on the raw path while autopatch re-dispatches the decoded one, so an
encoded slash steers the unchecked leg onto a deeper route and an encoded
question mark smuggles a query onto it.
Fixes#3528
kolaenteandTink
Melroy van den BergandGitHub