Use Expiring Auth JWTs#4337
Conversation
…ars are set correctly (see comments). When a JWT expires, use /api/auth/refresh to get a new set of tokens. Should be backwards compatible.
|
@nicballesteros is attempting to deploy a commit to the Umami Software Team on Vercel. A member of the Team first needs to authorize it. |
| return res.data.token; | ||
| } | ||
|
|
||
| throw new Error('Failed token refresh'); |
There was a problem hiding this comment.
Need help here. Not sure what I should do when refreshing fails.
Greptile SummaryThis PR introduces expiring JWTs with a refresh-token rotation scheme: a new
Confidence Score: 2/5The 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) 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
Sequence DiagramsequenceDiagram
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)
Reviews (1): Last reviewed commit: "Remove unused comment" | Re-trigger Greptile |
| export async function revokeRefreshToken(refreshToken: string) { | ||
| const session = await getUserAuthSessionByRefreshHash(hash(refreshToken)); | ||
|
|
||
| await updateUserAuthSession(session.id, { revokedAt: new Date() }); |
There was a problem hiding this comment.
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.
| removeClientAuthToken(); | ||
| removeClientRefreshToken(); |
There was a problem hiding this comment.
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.
| import { getItem, removeItem, setItem } from '@/lib/storage'; | ||
| import { AUTH_TOKEN } from './constants'; | ||
| import { AUTH_TOKEN, REFRESH_TOKEN } from './constants'; | ||
| import { refreshTokensEnabled } from './jwt'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I tested this running in docker. I'd like some guidance here on how I should name these variables.
There was a problem hiding this comment.
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:
| 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.
| if (res.status === 401 && data?.error?.code === 'expired-token') { | ||
| const token = await refreshTokens(); | ||
| return request(method, url, body, { | ||
| ...headers, | ||
| authorization: `Bearer ${token}` | ||
| }); | ||
| } |
There was a problem hiding this comment.
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.
| if (refreshTokensEnabled() && !Number.isNaN(expiryDays)) { | ||
| return Number(expiryDays); | ||
| } |
There was a problem hiding this comment.
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.
| if (refreshTokensEnabled() && !Number.isNaN(expiryDays)) { | |
| return Number(expiryDays); | |
| } | |
| const parsed = Number(expiryDays); | |
| if (refreshTokensEnabled() && !Number.isNaN(parsed)) { | |
| return parsed; | |
| } |
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:
sign()->expiresInpropertyIf 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/refreshBody:
Response:
"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.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.