Whole-model validation recursed into Author/CreatedBy, so a user with an
unvalidated language setting could permanently break label, team and comment
updates for everyone touching their rows.
Partial updates went to models that write a fixed Cols list, so
tasks_update {done:true} blanked description, priority and colour. Update
now reads the row first, like v2 AutoPatch. Also: integral floats accepted
for integer args, validation errors bounded to 200 runes per field, list
envelope renamed to match apiv2.Paginated, users_search shares it.
The assignee tests passed vacuously: the claimed count-query bug doesn't
exist and team 1 never had access to project 1. Unknown-tool and
invalid-argument cases now assert the branch go-sdk actually takes.
- page/per_page default and clamp like the REST handler; page < 1 dropped
the LIMIT clause entirely
- config-gated resources are unreachable through do_action
- create/update run the model's valid: tag rules
- read_all returns {items, result_count, total_items, page, per_page} and
strips emails from user rows
The users and projects_users_search groups don't exist; the routes collect
as other:users and projects:users_search, so PermissionsAreValid rejected
any real token and the tool was unreachable. Also strip emails on the
project-scoped search path.
Stateful streamable-HTTP cached the initialize request's context, so every
later call on a session ran as whoever opened it — any mcp:access token plus
a leaked Mcp-Session-Id gave full impersonation, and revoked tokens kept
working through their session. Stateless mode rebuilds the tool set from the
token on each request; resources register at startup instead of lazily.
Catalog-tier resources stay out of tools/list; agents discover them via
find_action (scope-filtered, schemas on demand) and invoke them through
do_action, which funnels into the same Dispatch path — schema validation
and the per-call scope re-check apply identically.
Wave 1: task labels, task relations (subtasks), team members, project
user/team shares, project views. Deliberately absent: api tokens,
webhooks, link shares, buckets and positions (v1 token scopes don't map
onto (group, op) permissions), saved filters (nested filter object).
Adds IdentityFields for records not addressed by their id (team members
go by team + username) and treats readOnly+param fields as arguments
(REST reads them from the URL; MCP has no URL).
tasks_read_all accepts filter/sort_by/order_by (from TaskCollection's
query-tagged fields) plus the generic search/page/per_page arguments;
project_id is optional so agents can list across projects. Fixture token
11 gains the tasks.read_all scope it now exercises.
copyByJSONTag previously skipped any IsZero value, which made it
impossible for tasks_update / projects_update to flip done from true
to false, reset priority/percent_done to 0, or unarchive a project.
A non-nil pointer src is now the unambiguous "caller supplied this"
signal: dereferenced values are written through even when zero, while
value-typed src fields keep the partial-update semantics. The
affected wrapper fields (Done, IsArchived, IsFavorite, Priority,
PercentDone, RepeatAfter, RepeatMode, BucketID,
CoverImageAttachmentID, ParentProjectID, Position) move to pointer
types so the JSON Schema still marks them optional.
Registers tasks, labels, teams, task_comments and task_assignees through
the MCP tool surface, completing the v1 resource list from the plan:
* tasks : create / read_one / update / delete (read_all omitted;
models.Task.ReadAll is a stub — TaskCollection is OOS)
* labels : full CRUD
* teams : full CRUD
* tasks_comments : full CRUD, install-time gated on
config.ServiceEnableTaskComments
* tasks_assignees : create / read_all / delete only (REST exposes no
read_one or update)
Per-resource input wrappers carry the path-param fields (task_id,
user_id) explicitly so MCP callers can provide them as JSON args.
installToolsForToken fans out to one installer per resource; the
generics-bound addTool keeps per-(resource, op) call sites at compile
time. The api_tokens.yml fixture extends token 11 to cover the new
scopes; token count stays at 5 for user 1 so existing token-listing
tests are unaffected.
Integration tests per resource cover tools/list visibility, at least
one successful create or read_all, and a permission denial scenario.
Filter MCP tool visibility and invocation by the requesting API token's
(group, permission) scopes. tools/list now returns only the tools the
token's APIPermissions authorise; tools/call additionally re-checks the
scope in the dispatcher as defence-in-depth, so a session created with
one token cannot be reused to invoke tools that token never had access to.
The per-session filter runs at session-init via the StreamableHTTPHandler
getServer factory (which the SDK calls once per session, before caching
the *mcp.Server). The dispatcher check runs on every tools/call and
returns ErrScopeDenied, which the AddTool wrapper renders as an IsError
tool result.
Wires the projects resource into the MCP server end-to-end. The five
project tools (create, read_one, read_all, update, delete) are now
visible in tools/list and dispatch through handler.Do* like the REST
layer.
- Add ProjectCreateInput / ProjectUpdateInput in inputs.go with
jsonschema tags covering only the writable fields the model honours
(title, description, identifier, hex_color, parent_project_id,
position, is_archived, is_favorite); computed fields like Owner and
MaxPermission are intentionally absent so the SDK-reflected schema
stays narrow.
- Add resources.go with a sync.Once-guarded RegisterResources(), and an
installTools helper that registers tools per (resource, op) on the
*mcp.Server via a generic addTool[In inputAdapter] helper. The
handler maps domain failures (permission denials, missing rows,
validation) to IsError tool results per the SDK convention.
- Add DispatchTyped in dispatcher.go so the AddTool handler can hand a
pre-unmarshalled wrapper to the dispatcher without a JSON
round-trip. The existing Dispatch (raw JSON path) delegates to a
shared dispatchPrepared.
- Wire RegisterResources() + installTools() into newServer() so each
new MCP session inherits the static tool set.
- Add fixture token 11 (mcp:access + projects:*) for the full-scope
integration tests; bump TestAPIToken_ReadAll's expected count.
- Refresh TestMCP_ToolsListEmpty into
TestMCP_ToolsListReturnsRegisteredResources, asserting the five
projects_* tools are present (Task 6 will introduce scope-based
filtering of this list).
- Add pkg/webtests/mcp_projects_test.go covering tools/list,
create/read_one/read_all/update/delete happy paths, schema-validation
failure on missing required title, permission denial on a forbidden
project, and nonexistent-id lookup.
Mount /api/v1/mcp (and /api/v1/mcp/*) inside the authenticated route
group. Reject JWT-authed requests with 401 (token-only policy), reject
API tokens without the mcp:access scope with 403, and propagate the
authed *user.User + *models.APIToken to r.Context() via typed keys so
downstream tool handlers can pull them out without depending on Echo.
The MCP protocol — JSON-RPC framing, Mcp-Session-Id management, SSE
streaming — is delegated to github.com/modelcontextprotocol/go-sdk
v1.6.1. tools/list returns {"tools": []} since no tools are registered
yet.
Adds the mcp scope group with a single access permission so it shows up
in GET /api/v1/routes (and therefore in the frontend token form).
Adds APIToken.HasMCPAccess() mirroring the caldav/feeds helpers.
The MCP endpoint will use POST, GET, and DELETE on the same path for the
streamable-HTTP transport, which CanDoAPIRoute's exact (method, path)
match cannot gate. The token middleware therefore skips the route check
for /api/v1/mcp and any sub-path; the actual authorization is delegated
to an inline HasMCPAccess() call in the MCP handler (added in the next
task).
Fixtures gain two MCP tokens for user 1: one mcp-only and one with
mcp:access plus projects read scopes for the per-tool scope filter tests.
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
GET /user (v1 and v2) returns pending_email; DELETE /user/settings/email
cancels a pending change and POST /user/settings/email/resend re-sends the
confirmation link. The v2 user settings handlers now share runUserAction
for the auth/session/commit boilerplate.
API tokens got 401 on `GET /api/v2/token/test` no matter their scope: v1
twin exempt from route scoping, v2 not, and `token_test` dropped from
route tables so no permission set could ever cover it. Indistinguishable
from "token invalid". Fail-closed, so not a bypass, but wrong. Also
switch comparison to `c.Path()` for both versions.
## How to verify
1. Create an API token with any scope (e.g. `tasks: read_all`).
2. Run `curl -H "Authorization: Bearer tk_..."
http://localhost:3456/api/v2/token/test`
3. **Expected:** `200` with `{"message":"ok"}`, same as
`/api/v1/token/test`.
**Before this PR:** `401`.
---------
Co-authored-by: kolaente <k@knt.li>
Co-authored-by: kolaente <hello@kolaente.de>
The refresh-token and OAuth token endpoints shared the 10 requests/minute
floor that protects login, register and password reset. Access tokens live
for 10 minutes, so every client renews its session several times an hour --
and behind a reverse proxy `service.ipextractionmethod` defaults to `direct`,
which keys all of them to the proxy's address. Routine renewals therefore
used up the login budget, and users got "Too many requests" when signing in.
This hit the desktop app hardest: it exchanges its authorization code on the
very same endpoint it refreshes tokens on.
Renewal now has a separate limiter with its own `ratelimit.tokenrefreshlimit`
setting, defaulting to 60 requests/minute. Both limits stay enforced.
The default-project creation triggered by the user-created event wrote the
stale in-memory user back with a full UpdateUser, racing registration and
sometimes resetting the status from "email confirmation required" back to
active. Only update the default_project_id column and keep the returned
user's status in sync with what was persisted.
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.
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>
CalDAV PUT called unrestricted Task.Update, so it wiped repeat_after/repeat_mode,
percent_done and dates the parsed VTODO didn't carry, plus assignees, reminders,
favorites and relations. Parse now reports which fields the VTODO actually spoke
to and UpdateResource overlays only those onto the stored task, re-read inside the
update transaction. A UID can match several tasks, so the write targets the one the
permission check covered.
Fixes#544Fixes#1422
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
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.
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.
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.
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.
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.
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.
kolaente
