fix(auth): send refresh token cookie to /api/v2 refresh endpoint by tink-bot · Pull Request #3651 · go-vikunja/vikunja

tink-bot · 2026-08-29T14:20:57Z

Refresh cookie path-scoped to /api/v1/user/token/refresh only. Browser never sent it to /api/v2/user/token/refresh, so v2 refresh always 401. Now one cookie per API version path. Frontend switched to v2 refresh.

How to verify

Log in via the web UI.
In the browser dev tools, check the cookies on the login response.
Expected: two vikunja_refresh_token cookies, one with path /api/v1/user/token/refresh and one with /api/v2/user/token/refresh.
Wait for the access token to expire (or delete token from localStorage and reload).
Expected: a POST /api/v2/user/token/refresh request returns 200 with a new token and the user stays logged in.
Before this PR: the v2 request returned 401 No refresh token provided.

github-actions · 2026-08-29T14:26:31Z

Preview Deployment

Preview deployments for this PR are available at:

URL	Tag	Commit
https://pr-3651.preview.vikunja.dev	`ghcr.io/go-vikunja/vikunja:pr-3651`	latest
https://sha-4b6dfd44c2b866cfc94ac6696b6786f2c34b106d.preview.vikunja.dev	`ghcr.io/go-vikunja/vikunja:sha-4b6dfd44c2b866cfc94ac6696b6786f2c34b106d`	`4b6dfd4`
https://sha-6cd17e8d5156d74904c9ff94222733c76bc59025.preview.vikunja.dev	`ghcr.io/go-vikunja/vikunja:sha-6cd17e8d5156d74904c9ff94222733c76bc59025`	`6cd17e8`
https://sha-22f0a85fc1e1fe3de6de8f6f5bfe06bca0d2d9c0.preview.vikunja.dev	`ghcr.io/go-vikunja/vikunja:sha-22f0a85fc1e1fe3de6de8f6f5bfe06bca0d2d9c0`	`22f0a85`
https://sha-6de67eebb1d0c622eb7350084dab6f2fa3360eb5.preview.vikunja.dev	`ghcr.io/go-vikunja/vikunja:sha-6de67eebb1d0c622eb7350084dab6f2fa3360eb5`	`6de67ee`
https://sha-7fae29b7a68528cd9265c365d632e30b434ca875.preview.vikunja.dev	`ghcr.io/go-vikunja/vikunja:sha-7fae29b7a68528cd9265c365d632e30b434ca875`	`7fae29b`

The preview environment will start automatically on first visit. Subsequent pushes to this PR will update the pr-3651 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-3651
docker run -p 3456:3456 ghcr.io/go-vikunja/vikunja:pr-3651

Last updated for commit 4b6dfd4

tink-bot

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

tink-bot · 2026-08-29T15:04:48Z

 func registerAPIRoutesV2(e *echo.Echo, a *echo.Group, wsRateLimit echo.MiddlewareFunc) {
 	a.Use(noStoreCacheControl())
 	a.Use(SetupTokenMiddleware())
+	a.Use(rateLimitV2TokenRefresh())


[security] 🟠 HIGH — ❓ needs your input

Pre-existing, but this PR is the first to route production traffic at the v2 auth surface: none of the public v2 auth routes (/api/v2/login, /register, /user/password/token, /user/password/reset, /shares/:share/auth, /oauth/token) have the unconditional per-IP floor that v1 applies via unauthRateLimit() (10/min/IP). /api/v2/login is an unthrottled password oracle on a default install. This round only fixed the refresh path to stay in scope. Should the same path-scoped pattern be extended to all v2 unauthenticated paths in this PR, or as a follow-up?

do a follow up PR that adds the same rate limit pattern for unauthenticated v2 routes as v1

On it — building the follow-up now on feat-v2-unauth-rate-limit: unauthRateLimit() path-scoped onto /api/v2/register, /user/password/token, /user/password/reset, /user/confirm, /login, /auth/openid/:provider/callback, /shares/:share/auth, plus the refresh limiter on /api/v2/oauth/token to match v1's tr group. It also folds in one shared unauthRateLimit() instance — v1 currently builds it twice (ws upgrade and the ur group), so its own budget is split the same way the refresh limiter's was.

Stacking it on this branch rather than main, since it reuses pathScoped from here; I'll retarget it to main once this merges. PR link to follow.

Follow-up is up: #3663 (stacked on this branch, retarget to main once this merges).

tink-bot · 2026-08-29T15:04:54Z

🤖 PR Swarm — automated review. Panel: bughunter · security · conventions · design · verifier — round 3 (final) @ 6cd17e8 (+218/−114 this round, PR +438/−114)

Verdict: ✅ APPROVE

Three rounds, each reviewing the previous round's fixes. Round 3 caught a regression round 2 introduced and paid down the design debt behind it; nothing actionable is left. One question below is yours to answer, not a blocker.

❓ Needs your input (1)

frontend/src/helpers/auth.ts:145 — the v1 refresh fallback exists only for browsers that logged in before this release. It fires on any non-429 v2 failure, so a genuinely dead session costs two refresh POSTs while it lives. Keep it and delete next release (current state), or drop it and ship the backend cookie change one release ahead of the frontend switch? Everything else is settled.

✅ Fixed this round (7)

pkg/modules/auth/auth.go:439 — regression from round 2: the loser of two concurrent refreshes cleared the cookie the winner had just set, logging both tabs out. Only an expired session or disabled account clears now — 42b5670
pkg/models/error.go:2359 — RefreshSession now returns typed domain errors instead of *echo.HTTPError from a transport-agnostic module; v2's shim for that leak is deleted — e2e3217
frontend/src/helpers/fetcher.ts:54 — the 429 short-circuit was undone by the interceptor's blind 1s retry — f696982
frontend/src/helpers/auth.test.ts:164 — three identical tests for one branch collapsed; leftover postCallCount removed — 542be90
pkg/webtests/admin_test.go:78 — v2 admin license gate was uncovered after the pathScoped refactor — 2643691
pkg/webtests/token_refresh_rate_limit_test.go:80 — two overlapping subtests merged into one interleaved v1/v2 test — 6901d3b
pkg/routes/routes.go:386 — documented that pathScoped matches the route template, not the request URL — 271890e

💤 Resolved as nit (5)

CORS * reflection with credentials — raised with the maintainer, accepted as is.
v2 429 bodies use Echo's error shape, not the RFC7807 envelope — pre-existing for all Echo-level middleware on v2; frontend uses the status only.
v2 refresh is double-covered by the group-wide limiter (v1's sub-group escapes it) — cosmetic unless ratelimit.enabled=true; the per-IP floor is identical on both.
A token whose user was deleted isn't classified unusable, so its cookie lingers — deliberate under the new rule; the client logs out anyway.
refreshCookie returns the first of two same-named cookies; address pr-swarm finding trailers in commit bodies — not worth a history rewrite.

🔎 Runtime verification

Local build (real frontend embedded), fresh user, jwtttlshort=5. Round 3 re-drove every round-2 change against 22f0a85.

✅ shared refresh limiter — v1 walked X-RateLimit-Remaining 59→30, the next v2 request continued at 29, request 61 429'd on either version. One 60/min per-IP budget, not 60 each.
✅ limiter scoping — 20 calls to /api/v2/projects and /api/v2/user consumed zero refresh budget; authenticated endpoints returned 200 while refresh was 429.
✅ cookie clearing — bogus, replayed and expired-session tokens each → 401 + exactly two Max-Age=0 cookies; no cookie → 401 + zero Set-Cookie; a non-401 failure (user deleted under a live session) → 404 with the cookie untouched, proving the gate is live.
✅ v2 admin gate after the pathScoped refactor — all nine /api/v2/admin/* ops 404 for an authenticated non-admin with no side effects, identical to v1.
✅ upgrade path in a real browser — v2 401 → v1 200 → session intact → later v2 refresh 200.
✅ network-level v2 failure (ERR_CONNECTION_REFUSED, no response object) also falls back to v1 and keeps the session.
✅ 429 exclusion — with the budget exhausted the app issued only v2 429s and no v1 fallback, recovering when the window reset.
➖ Round 3's own commits (typed errors, rotation-race gate, fetcher 429 skip) are covered by webtests and unit tests, not re-driven at runtime.

Reviewer summaries

Reviewer	Assessment
🐛 bughunter	Round-2 additions structurally sound (`c.Path()` safe, shared limiter has no shared mutable state, new tests non-vacuous); found the rotation-race clear.
🛡 security	Traced `c.Path()` through Echo's router and confirmed the admin gate is stricter than before; independently found the same rotation-race logout.
📏 conventions	Clean — comments are genuine whys, tests placed correctly, commit series coherent.
📐 design	Named the root cause behind the 401-sniffing (`RefreshSession` returning transport errors) and the churn residue in tests.
🔎 verifier	Nine flows verified with evidence, including that the admin gate survived the refactor and the limiter budget is genuinely shared.

Previous rounds (2)

round 1 @ 6de67ee — 🚫 BLOCKED: frontend unit suite red (missing apiV2Url mock export), mass logout on upgrade, v2 refresh unthrottled, stale e2e spies. 10 fixed, 5 nit-resolved, 2 deferred.

round 2 @ 22f0a85 — ⚠️ REQUEST CHANGES: clearing the cookie on any error (transient DB failures included), doubled rate-limit budget from a per-version limiter instance, fallback too narrow. 7 fixed, 5 nit-resolved, 1 deferred.

tink-bot

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

tink-bot · 2026-08-29T16:12:35Z

+				}
+				if (loggedOutSinceStart()) {
+					return
+				}


[design] 🟡 MEDIUM — ❓ needs your input

The fallback fires on every 401, and v2 returns the same 401 for "no cookie" (the migration case) and "token expired/replayed" (the everyday case). So a genuinely dead session costs two refresh POSTs — four with the interceptor's own 1s retry — permanently, not just during the upgrade window. Options: (a) leave as is and delete the fallback next release; (b) give the missing-cookie branch a distinct error code and fall back only on that; (c) drop the fallback and instead ship the backend cookie change one release ahead of the frontend switch. I went with (a) plus the broadened condition above, since it self-heals in a single release — but (c) is the zero-extra-code path if you'd rather stage it.

tink-bot

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

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.

It returned *echo.HTTPError from a transport-agnostic module, which v2 had to shim back into a huma error. Domain errors render on both APIs and let callers tell the failure modes apart. 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.

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

address pr-swarm finding: the admin gate and the refresh limiter hand-rolled the same shape. Matching now uses c.Path() like the other route lookups.

Browsers logged in before this release only hold the refresh cookie at the v1 path, and some deployments cannot reach v2 at all. One v1 refresh re-seeds both cookies, so sessions survive the upgrade. Drop the fallback once pre-v2 clients have cycled out.

The interceptor's retry sent a second request into the same exhausted window, which the limiter counts and which extends the block. address pr-swarm finding

address pr-swarm finding: spy still matched the v1 URL

…pers

address pr-swarm finding: the pathScoped refactor routes both admin gate halves, but only the instance-admin half was covered on v2.

github-actions Bot added area/api-v2 Huma-backed /api/v2 API surface area/auth Login, OIDC, LDAP, SAML, TOTP/2FA, PKCE, SCIM, sessions, password labels Aug 29, 2026

tink-bot added the pr-swarm/started PR Swarm run is currently active on this PR label Aug 29, 2026

tink-bot commented Aug 29, 2026

View reviewed changes

tink-bot added pr-swarm/needs-input PR Swarm finished, deferred items are waiting on maintainer input and removed pr-swarm/started PR Swarm run is currently active on this PR labels Aug 29, 2026

tink-bot mentioned this pull request Aug 29, 2026

fix(routes): rate limit the unauthenticated /api/v2 auth routes #3663

Merged

kolaente added 11 commits August 29, 2026 20:32

refactor(auth): give RefreshSession typed domain errors

5fb773f

It returned *echo.HTTPError from a transport-agnostic module, which v2 had to shim back into a huma error. Domain errors render on both APIs and let callers tell the failure modes apart. address pr-swarm finding

fix(routes): rate limit /api/v2/user/token/refresh

f290bf9

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

refactor(routes): extract pathScoped middleware helper

d7bcb8b

address pr-swarm finding: the admin gate and the refresh limiter hand-rolled the same shape. Matching now uses c.Path() like the other route lookups.

fix(frontend): do not retry a rate-limited token refresh

37da52f

The interceptor's retry sent a second request into the same exhausted window, which the limiter counts and which extends the block. address pr-swarm finding

test(e2e): spy on v2 refresh endpoint

9547742

address pr-swarm finding: spy still matched the v1 URL

test: assert both refresh cookie paths and consolidate the cookie hel…

e71a392

…pers

test: cover the v2 admin license gate

3cce67a

address pr-swarm finding: the pathScoped refactor routes both admin gate halves, but only the instance-admin half was covered on v2.

test: merge overlapping refresh rate limit subtests

4b6dfd4

tink-bot force-pushed the feat-refresh-cookie-v2 branch from 6cd17e8 to 4b6dfd4 Compare August 29, 2026 18:38

kolaente merged commit 4dbdc64 into main Aug 29, 2026
80 of 81 checks passed

kolaente deleted the feat-refresh-cookie-v2 branch August 29, 2026 19:47

Uh oh!

Uh oh!

Conversation

tink-bot commented Aug 29, 2026

How to verify

Uh oh!

github-actions Bot commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Preview Deployment

Uh oh!

tink-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tink-bot Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

kolaente Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

tink-bot Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

tink-bot Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tink-bot commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Verdict: ✅ APPROVE

❓ Needs your input (1)

✅ Fixed this round (7)

💤 Resolved as nit (5)

🔎 Runtime verification

Reviewer summaries

Uh oh!

tink-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tink-bot Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

kolaente Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

tink-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Aug 29, 2026 •

edited

Loading

tink-bot commented Aug 29, 2026 •

edited

Loading