Skip to content

Use Expiring Auth JWTs#4337

Open
nicballesteros wants to merge 12 commits into
umami-software:devfrom
nicballesteros:specify-auth-token-expiry
Open

Use Expiring Auth JWTs#4337
nicballesteros wants to merge 12 commits into
umami-software:devfrom
nicballesteros:specify-auth-token-expiry

Conversation

@nicballesteros

@nicballesteros nicballesteros commented Jun 15, 2026

Copy link
Copy Markdown

Use Expiring Auth JWTs

I wanted to add the ability to set an expiry on the JWTs created by the auth system. Using never dying JWTs are not ideal for security reasons.

Why

For example: if I sell my laptop and don't clear my browser history or storage, someone could log into my analytics dashboard since the JWT is still valid a year later. This way there is a time limit on how long someone can use the system before they need to reauthenticate. The default is 30 days.

Refreshing tokens

When a request fails due to an expired token, the system automatically refreshes the Access Token. When refresh tokens are enabled in the ENV VARs, the system starts saving the access token in browser memory and saves the refresh token in the localStorage.

Environment Variables

3 new environment variables were added:

  • AUTH_REFRESH_TOKEN_ENABLED -> 'true' | 'false'
  • AUTH_ACCESS_TOKEN_EXPIRY -> drop in for: jsonwebtoken -> sign() -> expiresIn property
  • AUTH_REFRESH_TOKEN_EXPIRY_DAYS -> number of days the refresh token will work.

If AUTH_REFRESH_TOKEN_ENABLED is set to false, the system should act as if the changes in this PR do not exist.

jsonwebtoken docs: https://www.npmjs.com/package/jsonwebtoken

New API Endpoint

POST /api/auth/refresh
Body:

{
  refreshToken: ${refreshToken},
}

Response:

{
  token: ${newAccessToken}
  refreshToken: ${newRefreshToken}
  user: { } // same as /login
}

"Session Tracking"

Database tracks UserAuthSession. This is the source of truth for Refresh Tokens. Refresh Tokens can be revoked in the UserAuthSession. Refresh Tokens are hashed for security.

Fixes

If you have any questions or concerns, leave a comment and I'll edit this description or respond.


View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.

@vercel

vercel Bot commented Jun 15, 2026

Copy link
Copy Markdown

@nicballesteros is attempting to deploy a commit to the Umami Software Team on Vercel.

A member of the Team first needs to authorize it.

Comment thread src/lib/fetch.ts
return res.data.token;
}

throw new Error('Failed token refresh');

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Need help here. Not sure what I should do when refreshing fails.

@nicballesteros nicballesteros marked this pull request as ready for review June 15, 2026 19:38
@greptile-apps

greptile-apps Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces expiring JWTs with a refresh-token rotation scheme: a new UserAuthSession table stores hashed refresh tokens, a /api/auth/refresh endpoint rotates them, and the client automatically retries failed requests after refreshing. Three new env vars (AUTH_REFRESH_TOKEN_ENABLED, AUTH_ACCESS_TOKEN_EXPIRY, AUTH_REFRESH_TOKEN_EXPIRY_DAYS) gate the feature.

  • New DB table & migration (user_auth_session) stores hashed refresh tokens with expiry and revocation timestamps.
  • New /api/auth/refresh endpoint validates the stored session and rotates both the access token and refresh token.
  • Client-side token storage is intended to keep the access token in memory and the refresh token in localStorage, with automatic 401-triggered refresh in fetch.ts.

Confidence Score: 2/5

The PR introduces several correctness gaps that affect core auth flows: logout does not actually clear client tokens, the in-memory access-token model silently falls back to localStorage in the browser, and revoking an unknown refresh token crashes the server.

Three distinct defects in changed code paths: (1) revokeRefreshToken dereferences a potentially-null DB result without a guard, crashing the logout handler on invalid tokens; (2) removeClientAuthToken/removeClientRefreshToken are called from a server Route Handler where they are no-ops, so logout never clears browser-side tokens; (3) refreshTokensEnabled() reads a non-NEXT_PUBLIC_ env var and always returns false in client bundles, meaning the access token is always stored in localStorage rather than memory, defeating the stated security improvement.

src/lib/auth.ts (null-dereference in revokeRefreshToken), src/app/api/auth/logout/route.ts (client functions called server-side), src/lib/client.ts (env-var visibility), src/lib/fetch.ts (unbounded recursion on token refresh)

Important Files Changed

Filename Overview
src/lib/auth.ts Adds saveRefreshToken and revokeRefreshToken; revokeRefreshToken dereferences session.id without a null guard, crashing on unknown/revoked tokens.
src/app/api/auth/logout/route.ts Imports and calls client-side removeClientAuthToken/removeClientRefreshToken from a server Route Handler — these calls are no-ops on the server, so client tokens are never cleared on logout.
src/lib/client.ts Token storage mode gated on refreshTokensEnabled() which reads a server-only env var — always returns false in the browser, so the access token always lands in localStorage instead of memory.
src/lib/fetch.ts Adds automatic token refresh on 401; request() calling refreshTokens() which calls request() creates a potential infinite recursion without a depth guard.
src/lib/jwt.ts Adds refresh/expiry helpers; getRefreshExpiry uses Number.isNaN(expiryDays) on a string (always false), so invalid env var values silently produce NaN expiry dates.
src/app/api/auth/refresh/route.ts New refresh endpoint; correctly validates session existence, revocation, and expiry before rotating tokens. Blocked by the NaN-expiry issue upstream in getRefreshExpiry.
src/app/api/auth/login/route.ts Extends login to issue and persist refresh tokens when the feature is enabled; logic is correct but depends on getRefreshExpiry which has the NaN bug.
prisma/schema.prisma Adds UserAuthSession model with appropriate fields and indexes for the refresh token store.
src/queries/prisma/userAuthSession.ts Clean data-access helpers for UserAuthSession; no issues found.

Sequence Diagram

sequenceDiagram
    participant Browser
    participant LoginAPI as POST /api/auth/login
    participant RefreshAPI as POST /api/auth/refresh
    participant LogoutAPI as POST /api/auth/logout
    participant DB as UserAuthSession (DB)

    Browser->>LoginAPI: "POST {username, password}"
    LoginAPI->>DB: create UserAuthSession (refreshHash, expiresAt)
    LoginAPI-->>Browser: "{token (short-lived), refreshToken}"
    Note over Browser: token → memory (intended)<br/>refreshToken → localStorage

    Browser->>RefreshAPI: "POST {refreshToken} [on 401 expired-token]"
    RefreshAPI->>DB: lookup session by hash(refreshToken)
    DB-->>RefreshAPI: session (or null)
    RefreshAPI->>DB: update session (new refreshHash, expiresAt)
    RefreshAPI-->>Browser: "{token, refreshToken, user}"
    Note over Browser: Retries original request with new token

    Browser->>LogoutAPI: "POST {refreshToken}"
    LogoutAPI->>DB: set revokedAt on session
    LogoutAPI-->>Browser: 200 OK
    Note over Browser: Client tokens NOT cleared<br/>(server-side calls are no-ops)
Loading

Reviews (1): Last reviewed commit: "Remove unused comment" | Re-trigger Greptile

Comment thread src/lib/auth.ts
Comment on lines +116 to +119
export async function revokeRefreshToken(refreshToken: string) {
const session = await getUserAuthSessionByRefreshHash(hash(refreshToken));

await updateUserAuthSession(session.id, { revokedAt: new Date() });

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.

P1 Null dereference on missing session

getUserAuthSessionByRefreshHash uses findUnique, which returns null when no matching row exists. Accessing session.id on the next line will throw TypeError: Cannot read properties of null (reading 'id'), crashing the logout endpoint whenever an unrecognised or already-revoked refresh token is submitted.

Comment thread src/app/api/auth/logout/route.ts Outdated
Comment on lines +32 to +33
removeClientAuthToken();
removeClientRefreshToken();

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.

P1 Client-side functions called from a server route handler

removeClientAuthToken and removeClientRefreshToken manipulate browser storage and a module-level in-memory variable. When invoked inside a Next.js Route Handler (server-side Node.js), typeof window === 'undefined' so the storage calls are silently skipped, and the accessToken module variable being reset applies to shared server memory — not to any individual user's browser state. The logout endpoint therefore never actually clears the client's tokens.

Comment thread src/lib/client.ts
import { getItem, removeItem, setItem } from '@/lib/storage';
import { AUTH_TOKEN } from './constants';
import { AUTH_TOKEN, REFRESH_TOKEN } from './constants';
import { refreshTokensEnabled } from './jwt';

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.

P1 AUTH_REFRESH_TOKEN_ENABLED is invisible to client-side code

refreshTokensEnabled() reads process.env.AUTH_REFRESH_TOKEN_ENABLED. In Next.js, only variables prefixed with NEXT_PUBLIC_ are inlined into the browser bundle; all others are undefined at runtime in client code. Because undefined === 'true' is false, refreshTokensEnabled() always returns false in the browser. As a result, setClientAuthToken always stores the access token in localStorage rather than memory, and getClientAuthToken always reads from localStorage, completely defeating the intended in-memory-only security model for access tokens.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I tested this running in docker. I'd like some guidance here on how I should name these variables.

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.

The fix is straightforward: rename AUTH_REFRESH_TOKEN_ENABLED to NEXT_PUBLIC_AUTH_REFRESH_TOKEN_ENABLED so Next.js inlines it into the client bundle.

Here's the specific change needed in jwt.ts:

Suggested change
import { refreshTokensEnabled } from './jwt';
export function refreshTokensEnabled() {
return process.env.NEXT_PUBLIC_AUTH_REFRESH_TOKEN_ENABLED === 'true';
}

The server-side functions getAccessExpiry() and getRefreshExpiry() can continue reading process.env.AUTH_REFRESH_TOKEN_ENABLED (without the prefix) since they only run on the server — but refreshTokensEnabled() is called from client.ts which runs in the browser, so it needs the NEXT_PUBLIC_ prefix.

You'd also update your .env / Docker env to use the new name:

NEXT_PUBLIC_AUTH_REFRESH_TOKEN_ENABLED=true
AUTH_ACCESS_TOKEN_EXPIRY=15m
AUTH_REFRESH_TOKEN_EXPIRY_DAYS=30

Note that NEXT_PUBLIC_ variables are visible in the browser bundle, so this is fine for a feature flag like this. The actual secrets (AUTH_SECRET, token values) stay server-only — this just exposes a boolean toggle.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

Comment thread src/lib/fetch.ts Outdated
Comment on lines +38 to +44
if (res.status === 401 && data?.error?.code === 'expired-token') {
const token = await refreshTokens();
return request(method, url, body, {
...headers,
authorization: `Bearer ${token}`
});
}

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.

P1 Unbounded recursive retry on token refresh

When request() receives 401 + expired-token, it calls refreshTokens(), which itself calls request() to hit /auth/refresh. If the refresh endpoint ever returns 401 + expired-token (e.g., due to a misconfiguration or a future code path), this will recurse infinitely and stack-overflow. A guard — such as a skipRefresh flag passed through options — should prevent the refresh call from triggering another refresh.

Comment thread src/lib/jwt.ts Outdated
Comment on lines +56 to +58
if (refreshTokensEnabled() && !Number.isNaN(expiryDays)) {
return Number(expiryDays);
}

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.

P2 Number.isNaN checks whether a value is the NaN number — it does not test whether a string is non-numeric. Number.isNaN('abc') returns false, so the guard never fires, and Number('abc') silently returns NaN. Any invalid AUTH_REFRESH_TOKEN_EXPIRY_DAYS value will produce new Date(NaN) — an Invalid Date — for every session's expiresAt, which the expiry check session.expiresAt < new Date() will silently mishandle.

Suggested change
if (refreshTokensEnabled() && !Number.isNaN(expiryDays)) {
return Number(expiryDays);
}
const parsed = Number(expiryDays);
if (refreshTokensEnabled() && !Number.isNaN(parsed)) {
return parsed;
}

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant