fix(api/v2): don't scope autopatch's internal GET against api token permissions

[tink-bot]

Every /api/v2 PATCH with a scoped API token returned 401 unless the token also carried the resource's read_one permission. Huma's AutoPatch implements PATCH as an internal GET + PUT re-dispatched through the router, and the GET leg was matched against the token's permissions like a client request — so a token scoped to tasks: update could never patch a task.

Fix: mark requests going through the group prefix adapter (only path autopatch's re-dispatch takes) as internal, skip route check for them. Client PATCH still authorised normally, read_one still enforced for client GETs.

Fixes #3528

How to verify

1. Create an API token with the tasks scope limited to read_all, create and update — leave read_one unchecked.
2. Patch a task you have write access to:

   curl -X PATCH 'http://localhost:3456/api/v2/tasks/1' \
     -H 'Authorization: Bearer <token>' \
     -H 'Content-Type: application/merge-patch+json' \
     -d '{"done": true}'
   

3. Expected: 200 with the updated task, and "done": true in the response body.
   Before this PR: 401 {"code":11,"message":"missing, malformed, expired or otherwise invalid token provided"}.
4. With the same token, request GET /api/v2/tasks/1 directly.
5. Expected: still 401, because the token does not have the read_one permission.

[github-actions]

Preview Deployment

Preview deployments for this PR are available at:

URL	Tag	Commit
https://pr-3541.preview.vikunja.dev	ghcr.io/go-vikunja/vikunja:pr-3541	latest
https://sha-7cac1684f75a1b20df88336a33e1b1211d4fa9b0.preview.vikunja.dev	ghcr.io/go-vikunja/vikunja:sha-7cac1684f75a1b20df88336a33e1b1211d4fa9b0	7cac168
https://sha-ca520de851fd17586c67edb671ac8e14cbb93730.preview.vikunja.dev	ghcr.io/go-vikunja/vikunja:sha-ca520de851fd17586c67edb671ac8e14cbb93730	ca520de
https://sha-2876c90cbef9f4e59f42953b9ec2832dc16a0d88.preview.vikunja.dev	ghcr.io/go-vikunja/vikunja:sha-2876c90cbef9f4e59f42953b9ec2832dc16a0d88	2876c90
https://sha-441eac00117185e1b569bd8a173be04be8542581.preview.vikunja.dev	ghcr.io/go-vikunja/vikunja:sha-441eac00117185e1b569bd8a173be04be8542581	441eac0
https://sha-9ce362014369bfe5bd345be1879aaa0d6468e8c8.preview.vikunja.dev	ghcr.io/go-vikunja/vikunja:sha-9ce362014369bfe5bd345be1879aaa0d6468e8c8	9ce3620
https://sha-d685181aedc31ddcdc92e01040bfa92a0a625002.preview.vikunja.dev	ghcr.io/go-vikunja/vikunja:sha-d685181aedc31ddcdc92e01040bfa92a0a625002	d685181

The preview environment will start automatically on first visit. Subsequent pushes to this PR will update the pr-3541 image — the preview picks up the new version on restart. The per-commit URLs point to a specific version and will not change.

Run locally with Docker

docker pull ghcr.io/go-vikunja/vikunja:pr-3541
docker run -p 3456:3456 ghcr.io/go-vikunja/vikunja:pr-3541

Last updated for commit 7cac168

[tink-bot]

🤖 PR Swarm — automated review, round 1. See inline comments.

[tink-bot]

🤖 PR Swarm — automated review. Panel: bughunter · security · conventions · design · verifier — round 3 @ 2876c90 (+5/−3 this round, PR +186/−22)

Verdict: ✅ APPROVE

Rounds 1 and 2 each found a live authorization bypass in the code this PR adds; both are fixed and pinned by regression tests that were verified to fail against the unfixed code. Round 3 attacked the result hard — 30+ crafted path encodings, 18 header variants, both patch content types, a JSON Patch read oracle, and a full scope matrix across all eight token-reachable autopatch routes, all observed end to end through the real router — and found no third bypass. Only a latent fail-open remained, now closed.

The predicate is now structurally sound in a way the first two versions were not: it compares the leg's own matched route template against the parent's, so a skip mathematically implies the leg is running the same echo handler the client's PATCH was already authorised for, and the GET-only plus empty-query narrowing removes the two channels through which that handler could be made to do more than read the resource being patched.

❓ Needs your input (4)

• pkg/routes/api/v2/ — expand is not scope-gated at all, independent of this PR. A token scoped to only {"tasks":["read_one"]} gets 200 and full comment bodies from GET /api/v2/tasks/1?expand=comments, while GET /api/v2/tasks/1/comments with the same token is 401. Expansions should be checked against the scope group owning the expanded resource (tasks_comments, tasks_attachments, …), otherwise per-group scopes are advisory for anything reachable through an expansion. Pre-existing; deserves its own issue.
• pkg/routes/api_tokens.go — /api/v2/token/test is unreachable by any API token. Flagged independently by two reviewers. The v1 twin is exempt from route scoping, and token_test is dropped from the route tables, so no permission set can ever cover the v2 path: with a valid API token GET /api/v2/token/test returns 401 while GET /api/v1/token/test returns 200, which is indistinguishable from "your token is invalid". Fail-closed, so not a bypass — but wrong, and this function is the right home for the fix. Left alone deliberately: this PR twice got burned relaxing a scope check, so exempting another route is your call. If you want it, c.Path() rather than URL.Path is the better comparison for both versions.
• pkg/webtests/api_token_method_matching_test.go — the standing GHSA-v479-vf79-mg83 guard covers only v1. It filters on strings.HasPrefix(r.Path, "/api/v1"), so it could never have caught either bug found here. Extending it to v2 means teaching it the v2 PUT→PATCH alias and the tasks.read_all quirk — its own PR.
• pkg/routes/routes.go — defence in depth against the encoded-separator primitive. Echo routing on the escaped path while huma reads the decoded one is the root cause of both bypasses; rejecting %2f/%5c/%3f in /api/v2 paths would remove the primitive rather than its consequences one at a time. Not done here because it changes behaviour for every v2 request.

✅ Fixed this round (1)

• pkg/modules/humabridge/humabridge.go — the internal-dispatch marker was only ever set, never cleared, so a dispatch without a parent echo context would inherit an outer dispatch's route rather than failing closed as its comment claimed. Unreachable today, but it becomes a real fail-open if anything ever implements huma's SanitizeInternalContext. The key is now written unconditionally, empty when unbound, so shouldSkipRouteCheck's non-empty guard rejects that case by construction. 2876c90

💤 Resolved as nit (1)

• pkg/routes/api_tokens.go — the /api/v1/token/test early return compares URL.Path rather than the matched route. Confirmed not redirectable: echo routes on RawPath when set, and there is no three-segment /api/v1/:param route to absorb percent-escapes. Folded into the deferred v2 token-test item above rather than changed on its own.

📝 Noted, not actioned

NormalizeArrayParams (pkg/routes/middleware/array_param_normalizer.go) is a global middleware that rewrites URL.RawQuery in place after routing but before the token middleware, which is what the new empty-query guard reads. Harmless today — the rewrite can only empty a query that carried no data (?[]), verified live — but it is a real ordering coupling between two files that a change to either could break.

🔎 Runtime verification

Built and ran the branch against an isolated SQLite database; every result below is an observed status code plus a re-read of the target with the owner's JWT. No screenshots — API-only change.

• ✅ headline fix intact — tasks: read_all+create+update token PATCHes a task → 200, persisted, untouched fields preserved; client GET still 401
• ✅ round-1 bypass closed — PATCH /api/v2/tasks/1%2Fcomments%2F1 → 401, comment unchanged
• ✅ round-2 bypass closed — PATCH /api/v2/tasks/1%3Fexpand=comments → 401, no comment text in the body (was 200 with the comments)
• ✅ new primitives — %23, %3B, %5C, %00, %09, %252F, %253F, %25253F, %2e%2e%2f, %C0%AF, //-authority, bare %3F, %3F%23, and slash+query combinations: all 401/404/422, zero leaks, zero mutations
• ✅ headers — X-Vikunja-Format, Accept, If-Match/If-None-Match, X-HTTP-Method-Override, X-Original-URL, X-Forwarded-* and 11 more: none makes the unchecked GET leg return anything a scope-checked client GET could not. X-Vikunja-Format is the only request header any v2 handler reads.
• ✅ JSON Patch oracle — test ops distinguish values, but only over the task's own fields, which a plain PUT with an update-only token already returns in full
• ✅ legitimate deep routes survive the GET-only narrowing — tokens scoped only to tasks_comments: update and projects_views: update PATCH /api/v2/tasks/1/comments/1 and /api/v2/projects/2/views/9 → 200, unrelated fields preserved
• ✅ cross-user — update-token against another user's task → 403, unchanged (model-level Can* still enforced on the legs)
• ✅ audit — one client PATCH emits exactly one api-token.used event; the internal legs emit none
• ➖ %00 in a path returns 500 rather than 4xx. Auth-independent (identical with the owner JWT), so a pre-existing routing robustness quirk, not something this PR introduced.

Reviewer summaries

Reviewer	Assessment
🐛 bughunter	Traced the mechanism across three rounds: marker set before the URL rewrite on an unconditionally-cloned request, c.Path() guaranteed populated, every unmatched leg fails closed. Found the redundant PUT branch and the tripled audit events.
🛡 security	Found round 1's escalation, and in round 3 failed to find a third one across 30+ path encodings, 18 headers, both patch types and all eight autopatch routes — with a middleware logging the matched template, marker and skip decision for every leg. Its round-2 dismissal of the query smuggling was wrong and the verifier's live run corrected it.
📏 conventions	No violations. Fixture entry, helper reuse, subtest structure and import order all match siblings. One stale doc comment, since fixed.
📐 design	Endorsed the mechanism over three alternatives (widening update to imply read_one, bypassing the middleware chain, swapping the credential) and flagged that the marker was applied at the wrong altitude — governing re-authentication and auditing too, not just the route check.
🔎 verifier	Drove every flow against a live server each round. Found the encoded-? bypass with a concrete 200-plus-comment-body response and supplied the JWT control proving the pivot routes are genuinely reachable, so the 401s are the scope check working rather than the router failing to match.

Previous rounds (2)

round 1 @ `9ce3620` — 🚫 BLOCKED: encoded-slash pivot let an `update`-scoped token read and overwrite a task comment; fixed by binding the marker to the authorised route, plus audit-event gating and test isolation. Took 92m 36s.
round 2 @ `441eac0` — ⚠️ REQUEST CHANGES: encoded `?` smuggled a query onto the unchecked legs, leaking comments through `expand`; fixed by requiring an empty query and narrowing the skip to GET. Took 24m 0s.

[tink-bot]

🤖 PR Swarm — automated review, round 2. See inline comments.

[tink-bot]

🤖 PR Swarm — automated review, round 3. See inline comments.