Skip to content

fix(auth): rotate refresh tokens with single-use enforcement + enable rotation#3083

Open
xilosada wants to merge 6 commits into
auth-token-semanticsfrom
auth-refresh-rotation
Open

fix(auth): rotate refresh tokens with single-use enforcement + enable rotation#3083
xilosada wants to merge 6 commits into
auth-token-semanticsfrom
auth-refresh-rotation

Conversation

@xilosada

@xilosada xilosada commented Jun 30, 2026

Copy link
Copy Markdown
Member

Stacked on #3082 (base branch: auth-token-semantics). Review/merge after it.

What

Makes refresh tokens single-use and turns on signing-secret rotation. Closes security-review findings 11 (root refresh tokens never rotated, no reuse detection) and 13 (secret-rotation task defined but never started).

Before → after

Before After
Refresh replay Root refresh is stateless: the same 30-day refresh token can be exchanged unlimited times; a stolen refresh token = 30 days of silent access. Every exchange consumes the token's jti into a storage-backed denylist atomically, before minting (process-wide consume lock → two concurrent requests with the same token yield exactly one success). A replay is treated as theft: the token family's live key is revoked and the client gets 401 + x-auth-error: token_reuse.
Rotated client keys Client keys rotate ids on refresh, but a replayed old refresh token could only be matched against the deleted id — family revocation silently failed and the live key survived. Each rotation records old id → new id; reuse handling chases the chain (bounded at 32 hops) and revokes the live key.
Denylist growth Entries carry the token's own expiry; a throttled (1/h) sweep runs in a spawned background task off the refresh hot path and reaps expired denylist + rotation-map entries.
Secret rotation start_rotation_task exists but has zero callers → the JWT signing secret is permanent (finding 13). Spawned at startup (both standalone mero-auth and merod-embedded paths). Safe only because #3079 makes verification accept the unexpired backup secret — hence the stack order.

Review-feedback fixes (meroreviewer inline, all 5 addressed)

  1. Denylist write after return → the jti is now claimed before minting; a storage failure aborts with nothing issued (safe retry), a post-claim mint failure burns the token (fail closed) instead of ever leaving a minted-but-unrecorded pair.
  2. Family revocation fails for rotated client keyssystem:refresh:rotated:{old_id} mapping + revoke_token_family chain resolution.
  3. TOCTOU between reuse-check and record → both are one critical section under a process-wide consume_refresh_lock (storage has no CAS; the persistent backend is single-process RocksDB, so a process lock is sufficient).
  4. Sweep on the hot pathtokio::spawned, throttled internally.
  5. "token_reuse".parse().unwrap()HeaderValue::from_static.

How to test

cargo test -p mero-auth

Targeted tests:

  • refresh_rotates_and_consumed_token_is_reuse_rejected — exchange, replay → TokenReuse, family revoked.
  • concurrent_refresh_of_same_token_yields_exactly_one_success — the TOCTOU regression test.
  • replayed_client_refresh_after_rotation_revokes_live_key — the rotated-client-key revocation regression test.
  • fresh_refresh_token_is_not_flagged_as_reuse — no false positives.

Manual: POST /auth/refresh twice with the same refresh token → second responds 401 with x-auth-error: token_reuse, and the first exchange's new tokens stop working (family revoked).

Compatibility

…tion

- each refresh rotates the refresh token; a consumed refresh token is recorded
  in a storage-backed denylist and may not be reused (reuse revokes the token
  family), surfaced via an x-auth-error: token_reuse response signal
- a throttled, process-wide sweep bounds the denylist size
- spawn the signing-secret rotation task at startup, safe now that verification
  accepts an unexpired backup secret

@meroreviewer meroreviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 MeroReviewer

Reviewed by 1 agents | Quality score: 24% | Review time: 69.9s

🟡 3 warnings, 💡 1 suggestions, 📝 1 nitpicks. See inline comments.


🤖 Generated by MeroReviewer | Review ID: review-60d2d926

Comment thread crates/auth/src/auth/token/jwt.rs
Comment thread crates/auth/src/auth/token/jwt.rs
Comment thread crates/auth/src/auth/token/jwt.rs
Comment thread crates/auth/src/auth/token/jwt.rs
Comment thread crates/auth/src/api/handlers/auth.rs Outdated
@meroreviewer

meroreviewer Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Documentation Review

The following documentation may need updates based on the changes in this PR:

  • 🟡 AGENTS.md: AGENTS.md exists but was not updated — consider updating it to reflect the architecture changes in this PR.
  • 🟡 CONTRIBUTING.md: CONTRIBUTING.md exists but was not updated — consider updating it to reflect the architecture changes in this PR.
  • 🟡 architecture/crates/auth.html: Files matching crates/auth/** were changed but architecture/crates/auth.html was not updated (per source_to_docs_mapping).
  • 🟡 docs/: Static HTML docs in docs/ may need updating — architecture-impacting changes detected. On merge, update-docs will scan this directory and open a PR if any pages need to change.

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@github-actions github-actions Bot added the Stale label Jul 7, 2026
@frdomovic frdomovic changed the title auth: rotate refresh tokens with single-use enforcement + enable rotation fix(auth): rotate refresh tokens with single-use enforcement + enable rotation Jul 13, 2026
@frdomovic

Copy link
Copy Markdown
Member

Triage of the meroreviewer[bot] review findings

Reviewed all five as part of closing out the security-review pass. All are valid; grouping by priority. (Titles fixed across #3079#3083 to pass the conventional-commit pr-lint check.)

Must-fix before merge (they defeat the single-use guarantee this PR adds):

  1. Denylist write after the pair is returned (jwt.rs, ~record_consumed_refresh) — if record_consumed_refresh fails, ? returns an error to the caller after the new pair was minted; the client retries with the original refresh token, which was never denylisted → single-use defeated. Prefer write-first, return-second (record the consumed jti before generating the new pair) so a storage error fails the exchange before any token is issued, rather than best-effort log-and-continue (which silently weakens the guarantee).

  2. TOCTOU between is_refresh_consumed and record_consumed_refresh — two concurrent requests with the same refresh token both pass the check and both mint pairs. Needs a set-if-not-exists / compare-and-set at the storage layer, or a per-jti in-process lock held across check+write. This and (1) are really one change: an atomic "claim this jti or fail" primitive, executed before minting.

  3. Family revocation misses rotated client keysrevoke_client_tokens looks up by claims.sub, but a client key whose id was deleted on a prior rotation returns None and the revoke is swallowed, so a replayed rotated client token isn't revoked. Carry a stable family_id on every client key (or persist old→new key-id in the denylist entry) and revoke by family, not by the now-stale sub.

Should-fix:

  1. Sweep on the refresh hot-pathmaybe_sweep_consumed_refresh().await runs inline; a large/slow backend adds latency to the refresh that wins the sweep slot. tokio::spawn it (fire-and-forget) so the response isn't blocked.

Nit:

  1. "token_reuse".parse().unwrap()HeaderValue::from_static("token_reuse") (infallible, matches the surrounding style).

I've left these for the PR author to implement rather than pushing to the branch, since (1)–(3) are one interdependent design decision around the denylist's atomicity/family model. Happy to open a follow-up commit if you'd like. Full context in security-review-resolution.md (items 11 & 13).

@cursor

cursor Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

- claim the refresh jti atomically (consume lock) BEFORE minting: closes
  the TOCTOU between the consumed-check and the denylist write, and fixes
  the record-after-return failure mode where a storage error let the
  caller retry a not-yet-denylisted token after a pair was already minted
- family revocation now chases the client key-rotation chain
  (system:refresh:rotated:{old} -> new) so a replayed rotated refresh
  token revokes the LIVE key instead of silently failing on the deleted id
- key lookup no longer short-circuits reuse detection when the key id was
  rotated away
- denylist/rotation-map sweep runs in a spawned background task off the
  refresh hot path
- X-Auth-Error uses HeaderValue::from_static

New tests: concurrent_refresh_of_same_token_yields_exactly_one_success,
replayed_client_refresh_after_rotation_revokes_live_key

@meroreviewer meroreviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 90% | Review time: 644.2s


⚠️ Review Incomplete

1 of 2 agent(s) did not finish (patterns-reviewer) and no findings were produced. Treat this PR as not yet reviewed, not as approved.


🤖 Generated by AI Code Reviewer | Review ID: review-be013ee1

…n grace

Two ways this PR could brick a node:

1. Reuse-revocation hit ROOT keys. A user_password login mints its pair
   against the root key, so a replayed root refresh token revoked the
   node's root key. verify_credentials then rejects it while list_keys
   still reports it, so the bootstrap path cannot re-fire either: the
   node is permanently unauthenticatable. Two tabs sharing one token
   bundle (the desktop SSO model) are enough to trigger it, on a FRESH
   node. Reuse on a root key is now refused WITHOUT revoking the key —
   the single-use denylist already defeats the replay, and revoking a
   root key stays an explicit admin action (mirroring delete_key_handler,
   which refuses to revoke the last active root key).

2. Rotation grace was shorter than the refresh-token lifetime. Storage
   keeps ONE previous secret, so a token must verify while its signing
   secret is at most one generation old. With 24h rotation / 48h grace
   and 30-day refresh tokens, enabling rotation for the first time would
   have invalidated every refresh token within 48h, forcing a re-login
   every day or two. Defaults are now 30d rotation / 60d grace, which
   satisfy rotation_interval >= token lifetime and grace >= interval +
   lifetime.

New test: root_key_survives_refresh_reuse. Updated
refresh_rotates_and_consumed_token_is_reuse_rejected, whose old
assertion encoded the bricking behavior.
@frdomovic

Copy link
Copy Markdown
Member

⚠️ Two node-bricking defects found and fixed (1ea46f2)

1. Reuse-revocation revoked ROOT keys → permanently unauthenticatable node

A user_password login mints its pair against the root key, so sub on a root refresh token is the root key id. On reuse, revoke_token_familyrevoke_client_tokenskey.revoke() applied to any key type, root included. After that: verify_credentials rejects the invalid key, but list_keys still reports it, so the bootstrap path can't re-fire either — the node can never be authenticated again. It also bypassed the guard the HTTP layer already has (delete_key_handler refuses to revoke the last active root key).

This needed no upgrade of an old node and no attacker: two tabs/webviews sharing one token bundle (exactly the desktop SSO model, which passes the refresh token to every app window) is enough. A fresh node would brick itself the first time that happened.

Fix: reuse on a root key is now refused (TokenReuse) without revoking the key. The single-use denylist already defeats the replay; revoking a root key stays an explicit, authenticated admin action. Client keys still get their family revoked as before. Regression test: root_key_survives_refresh_reuse (asserts the root key stays valid and the legitimately-rotated token keeps working). The old assertion in refresh_rotates_and_consumed_token_is_reuse_rejected encoded the bricking behavior and was updated.

2. Rotation grace (48h) was shorter than the refresh-token lifetime (30d)

This PR enables start_rotation_task for the first time. Storage keeps exactly one previous secret, so a token must verify while its signing secret is at most one generation old. With 24h rotation / 48h grace and 30-day refresh tokens, every refresh token would have stopped verifying within 48h of the first rotation — a forced re-login every day or two, across every deployment, silently.

Fix: defaults are now 30d rotation / 60d grace, satisfying rotation_interval >= longest token lifetime and grace_period >= rotation_interval + lifetime. Rationale documented on SecretRotationConfig::default.

Suite: 108 passed. Branch rebased on the updated base (which is current with master).

Still required before this can merge

mero-js must survive single-use rotation across instances/tabs — PR incoming (single-flight + cross-tab lock + x-auth-error: token_reuse handling). Apps that re-inject a stale token bundle from the SSO hash (mero-chat, tauri-app, pixart/calendar/design) need fixes too; tauri-app hands its refresh token to every app webview, which creates uncoordinated rotators by construction.

frdomovic added a commit to calimero-network/mero-pixart that referenced this pull request Jul 14, 2026
Refresh tokens are single-use (calimero-network/core#3083): every
POST /auth/refresh consumes the presented refresh token and mints a new one.
Re-presenting a consumed refresh token is treated as theft — 401
`x-auth-error: token_reuse` — and the node revokes the entire token family,
hard-logging-out every holder.

`persistTauriHashAuth()` unconditionally overwrote the stored `mero-tokens`
bundle from the SSO hash on every load. The desktop re-opens MeroPixArt with
the bundle it minted at ITS login, so the hash routinely carries a bundle older
than the stored one — mero-js may have rotated several times since. Clobbering
it re-presents a refresh token the node has already consumed, and the next
refresh kills the session.

Seed only when nothing is stored, the node changed, or the hash bundle is
genuinely newer (utils/authTokens.ts, with tests) — mirroring the same helper
in mero-chat.

Depends on calimero-network/core#3083 and calimero-network/mero-js#67.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
frdomovic added a commit to calimero-network/tauri-app that referenced this pull request Jul 14, 2026
Refresh tokens are single-use (calimero-network/core#3083): every
POST /auth/refresh consumes the presented token and mints a new one, and
re-presenting a consumed token is treated as theft — the node revokes the
whole token family and replies 401 x-auth-error: token_reuse, logging out
every holder.

The desktop broke that rule twice:

1. Across webviews. openAppFrontend put the real refresh token in every app
   window's SSO URL hash. Each webview is a separate origin with its own
   localStorage and its own MeroJs, so N holders each rotated the same
   single-use token: the first consumed it, the next tripped reuse detection,
   and the family was revoked. Re-opening an app also re-injected the
   desktop's (possibly stale) token over the app's live one.

2. Within the desktop. Several MeroJs instances exist (the lazy apiClient
   default, createClientAsync, plus StrictMode double-mounting), each
   deduping refreshes only among its own requests. A burst of 401s fired six
   concurrent refresh POSTs carrying the same token — one succeeded, the rest
   were reuse.

The desktop is now the single holder and single rotator. App windows get the
access token plus an opaque BROKERED_REFRESH_TOKEN sentinel, never the real
refresh token; the injected proxy script intercepts their POST /auth/refresh
and routes it to the new broker_token_refresh Tauri command, which the desktop
answers from its own token store. All rotations — desktop and app alike —
funnel through one single-flight that re-reads the store before POSTing, so a
consumed token can never be replayed. Apps need no changes: an unmodified
mero-js/mero-react app still just calls /auth/refresh.

Also fixes the e2e fixture: the refresh route was mocked at **/auth/refresh-token,
which matches nothing (the real path is /auth/refresh), so the refresh path was
never exercised. The mock now models single-use rotation — a new pair per call,
token_reuse + family revocation on replay — and it caught bug 2 above.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A browser hides every response header from JS unless the server names it
in Access-Control-Expose-Headers. The standalone mero-auth router set
allow_origin/methods/headers for an explicit origin allow-list but never
called expose_headers, so a cross-origin client saw a bare 401: it could
not auto-refresh (SDKs only refresh on x-auth-error: token_expired) and,
with this PR's single-use enforcement, could not recognise a revoked
family (token_reuse) either — it would retry forever.

CorsLayer::permissive() (the allow-all branch) already exposes
everything, which is why this went unnoticed; mero-server has carried the
same list, and tests pinning it, since an earlier production incident.

Test: explicit_origin_cors_exposes_x_auth_error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants