Add refresh-token auth flow and harden HTML injection paths#425
Add refresh-token auth flow and harden HTML injection paths#425ghengeveld wants to merge 2 commits intonextfrom
Conversation
Store auth as a validated token blob with persisted session metadata, send session headers on token exchanges, and refresh access tokens reactively with timeout/retry handling to avoid forced re-login during active use. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## next #425 +/- ##
========================================
+ Coverage 6.80% 8.05% +1.24%
========================================
Files 166 166
Lines 14216 14384 +168
Branches 311 345 +34
========================================
+ Hits 968 1158 +190
+ Misses 13168 13150 -18
+ Partials 80 76 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
valentinpalkovic
left a comment
valentinpalkovic
left a comment
There was a problem hiding this comment.
CCG review (Codex + Gemini + Claude synthesis)
Solid security upgrade overall. PKCE + auth-code is a clear win and the single-flight refresh pattern is the right shape. Findings cluster around (a) refreshed token never reaching React state, (b) refresh-error propagation, (c) isRetryableRefreshError defaulting to retry on terminal OAuth errors. No Critical. 3 High that should land before merge.
Reviewer convergence
| Finding | Codex | Gemini | Claude |
|---|---|---|---|
| H1 Stale React token after refresh | ✅ | — | ✅ |
H2 Premature setAuthenticatedSession |
✅ | — | ✅ |
| H3 Swallowed refresh error | ✅ | partial | ✅ |
M1 isRetryableRefreshError default |
— | ✅ | ✅ |
M2 refresh_token required by schema |
✅ | — | ✅ |
| M3 Logout/refresh race | ✅ | — | ✅ |
| M5 No legacy storage migration | ✅ | — | partial |
| M6 Cross-tab refresh not single-flight | ✅ | — | ✅ |
| M7 Silent failure, no UX feedback | partial | ✅ | — |
| L2 Lint gaps (outerHTML, document.write, srcdoc) | ✅ | — | — |
L3 globalThis vs window inconsistency |
— | ✅ | ✅ |
L4 <React.Fragment /> regression |
— | ✅ | — |
No conflicts between advisors.
Action checklist (priority order)
- H3 — rethrow refresh errors so
authExchange.refreshAuthsees failure. - H1 — propagate refreshed access token to React state /
useAccessTokensubscribers. - H2 — defer
setAuthenticatedSessionuntil project-creation flow completes. - M1 — fix
isRetryableRefreshErrorfallthrough. - M3 — generation counter to discard refresh result after logout.
- M2 — make
refresh_tokenoptional inTokenResponseSchema(RFC 6749 §6). - M7 — surface session-expired notification.
- M5 — document forced re-login on upgrade or migrate legacy values.
- M6 — decide cross-tab strategy (BroadcastChannel /
storageevent) or document. - L2 / L3 / L4 — quick cleanups.
- Tests for H1 / M3 / state-mismatch on error callback / single-flight concurrent 401 / refresh-without-rotation.
Further related findings outside this PR's diff (already in next): OAuth state not validated on the error sub-branch in Verify.tsx's grant handler, and state forwarded with optional validation in manager.tsx. Per RFC 6749 §10.12 state must be validated on all authorization responses. Worth following up.
Detailed inline comments below.
| sessionId: currentAuth.sessionId, | ||
| signal: abortController.signal, | ||
| }); | ||
| setCurrentAuth(nextAuth); |
There was a problem hiding this comment.
[High] Refreshed access token never reaches React state — builds run with stale token.
attemptTokenRefresh updates the module-level currentAuth / currentToken and localStorage, but useAccessToken only updates via setTokenState inside updateToken (lines 94-100). The refresh path bypasses that callback entirely.
useBuildEvents reads accessToken from React state and emits it on START_BUILD (src/utils/useBuildEvents.ts:11,36). After a silent refresh, the React tree (and therefore startBuild → preset.ts START_BUILD handler) keeps the expired token. So a 401-triggered refresh succeeds for GraphQL but the next build still fires with the stale token.
Fix options:
- expose a small subscribe/notify on the module and have
useAccessTokensubscribe; - read
currentTokenlazily insidestartBuildinstead of capturing via closure / props; - push token change through
useAddonStatesouseAccessTokenre-renders.
Whichever you pick, add a regression test: simulate refresh → assert React-side accessToken updated.
(Codex + Claude)
| return true; | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
[Medium] isRetryableRefreshError defaults to true for non-status-coded errors → retries terminal OAuth errors.
if (error instanceof Error) {
if (statusMatch) return statusCode >= 500; // 4xx → false ✓
if (error.name === 'AbortError') return true;
// falls through ↓
}
return true; // ← non-status Errors and non-Errors retryErrors thrown by decodeTokenResponse (e.g. invalid_grant, invalid_request) carry no (NNN) token in the message. They fall through and retry once — wasted call against an already-revoked refresh token, plus extra latency before logout.
const isRetryableRefreshError = (error: unknown) => {
if (error instanceof Error) {
const statusMatch = error.message.match(/\((\d{3})\)/);
if (statusMatch) {
const statusCode = Number(statusMatch[1]);
return statusCode >= 500;
}
if (error.name === 'AbortError') {
return true;
}
return false;
}
return false;
};(Gemini + Claude)
| throw error; | ||
| } | ||
| })() | ||
| .catch(() => { |
There was a problem hiding this comment.
[High] Refresh failures are swallowed — authExchange.refreshAuth (line 204) sees success even when the session was cleared.
The outer .catch(() => { console.warn(...); clearCurrentAuth(); }) resolves the refreshPromise cleanly. urql's authExchange.refreshAuth awaits this and treats it as a successful refresh, then retries the original op (now without auth) instead of surfacing the auth error.
Fix: rethrow inside the catch (or remove the catch entirely and let it propagate, with clearCurrentAuth() in a finally). Pair with M7 (UX toast) so the user understands why they were just signed out.
.catch((error) => {
clearCurrentAuth();
throw error;
})(Codex + Claude)
| } | ||
| })() | ||
| .catch(() => { | ||
| console.warn('Session expired. Please sign in again.'); |
There was a problem hiding this comment.
[Medium] No UI feedback on silent refresh failure.
A console.warn is the only signal when a session expires server-side or the refresh token has been revoked. The user sees stale UI / failing requests until something else triggers an auth-gated render.
Suggest routing through useErrorNotification or emitting a session-expired toast on the addon channel when clearCurrentAuth is called from the refresh path (vs an explicit logout). That's also the natural pairing for the rethrow in H3.
(Gemini)
| }; | ||
|
|
||
| const clearCurrentAuth = () => { | ||
| refreshAbortController?.abort(); |
There was a problem hiding this comment.
[Medium] Logout ↔ in-flight refresh race can resurrect auth state.
clearCurrentAuth aborts the controller and nulls module state, but if attemptTokenRefresh's fetch already resolved (response in flight, abort no-ops), the awaited continuation still calls setCurrentAuth(nextAuth) (line 142) and re-populates auth post-logout.
Guard with a generation counter:
let authGen = 0;
const clearCurrentAuth = () => { authGen++; /* ...rest */ };
const attemptTokenRefresh = async () => {
const myGen = authGen;
const nextAuth = await refreshAccessToken({...});
if (myGen !== authGen) return; // logged out mid-flight
setCurrentAuth(nextAuth);
};(Codex + Claude)
|
|
||
| const TokenResponseSchema = z.object({ | ||
| access_token: z.string().min(1), | ||
| refresh_token: z.string().min(1), |
There was a problem hiding this comment.
[Medium] refresh_token is required, but RFC 6749 §6 allows the server to omit it on refresh responses.
const TokenResponseSchema = z.object({
access_token: z.string().min(1),
refresh_token: z.string().min(1), // ← required
...
});If Chromatic's token endpoint ever returns an access-only response (i.e. doesn't rotate refresh tokens on every refresh), the schema fails and the user is logged out. Make refresh_token optional and reuse the previous refreshToken when omitted:
const TokenResponseSchema = z.object({
access_token: z.string().min(1),
refresh_token: z.string().min(1).optional(),
error: z.string().optional(),
error_description: z.string().optional(),
});Then in refreshAccessToken: refreshToken: data.refresh_token ?? refreshToken.
(Codex + Claude)
| refreshToken: z.string().min(1), | ||
| tokenEndpoint: z.string().url(), | ||
| sessionId: z.string().min(1), | ||
| }); |
There was a problem hiding this comment.
[Medium] No backward-compat for users with the previous raw-token storage format.
The schema requires version: literal(1) and a JSON object. Existing users with a plain JWT string in ACCESS_TOKEN_KEY will hit the parse failure path → clearCurrentAuth() → forced re-login on upgrade.
If re-login is acceptable, please call it out in CHANGELOG and release notes (one-time hit but visible to users). Otherwise add a small migration — detect non-JSON value, treat as expired, drop it cleanly.
(Codex)
| }); | ||
| if (!token) throw new Error('Failed to fetch an access token'); | ||
| accessToken.current = token; | ||
| setAuthenticatedSession(token); |
There was a problem hiding this comment.
[High] Refresh token persisted before the auth flow is actually complete.
setAuthenticatedSession(token) here writes the refresh token to localStorage immediately after token exchange, but the comment three lines down still says "don't store it yet." For users with no projects, control branches into the newProjectUrl dialog (line ~113) without ever calling setAccessToken. If the user closes the dialog at that point, a long-lived refresh token sits in localStorage while the Panel-level auth state is still empty.
Fix: defer setAuthenticatedSession to the same point as setAccessToken(accessToken.current) (lines 104 / 124). Or, alternatively, clear auth on Verify-screen unmount unless setAccessToken was called.
(Codex + Claude)
| selector: "NewExpression[callee.name='Function']", | ||
| message: 'Function constructor is banned', | ||
| }, | ||
| ], |
There was a problem hiding this comment.
[Low] Sink ban is incomplete as a guardrail.
Great addition overall. Common sinks not yet covered:
outerHTMLassignmentdocument.write/document.writeln- iframe
srcdocJSXAttribute Range.createContextualFragment- string-form
setTimeout/setInterval
Suggested additions:
{ selector: "AssignmentExpression[left.property.name='outerHTML']", message: 'outerHTML assignment is banned' },
{ selector: "CallExpression[callee.object.name='document'][callee.property.name=/^writeln?$/]", message: 'document.write/writeln is banned' },
{ selector: "JSXAttribute[name.name='srcdoc']", message: 'iframe srcdoc is banned' },
{ selector: "CallExpression[callee.property.name='createContextualFragment']", message: 'createContextualFragment is banned' },
{ selector: "CallExpression[callee.name=/^setTimeout|setInterval$/][arguments.0.type='Literal']", message: 'String-form setTimeout/setInterval is banned' },(Codex)
| }, [targetSelector]); | ||
|
|
||
| return <></>; | ||
| return <React.Fragment />; |
There was a problem hiding this comment.
[Low] <React.Fragment /> is inconsistent with the rest of the repo.
Elsewhere in the codebase (e.g. VisualTests.tsx:323) we use <></>. Suggest reverting this hunk to keep the style uniform.
(Gemini)
2 participants
Summary
accessToken,refreshToken,tokenEndpoint,sessionId) stored under the existing storage keyauthExchangewith in-tab single-flight behavior, 10s timeout, one retry for transient/5xx failures, and session clearing on terminal auth failuredangerouslySetInnerHTML/innerHTML-style sinks in ESLint and replacinginnerHTMLusage inPulsatingEffectwithtextContentTest plan
yarn lintyarn typecheckyarn vitest run src/utils/graphQLClient.test.tsMade with Cursor