Refresh token POC #2966
Refresh token POC #2966
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@claude review this PR |
|
Claude finished @Alex-Tideman's task —— View job Code Review Complete ✅I've reviewed this PR implementing OAuth2 token refresh functionality. Here's my comprehensive assessment: SummaryThis PR implements automatic token refresh for OAuth2 authentication, preventing session interruptions when access tokens expire. The implementation includes:
Critical Issues 🔴1. JWT Signature Not Validated (server/server/auth/auth.go:108)The Risk: High - Authentication bypass vulnerability 2. Race Condition in Token Refresh (src/lib/utilities/request-from-api.ts:112-123)Multiple concurrent requests failing with 401 will all trigger refresh attempts simultaneously, potentially causing:
Risk: Medium - Could cause auth failures under load 3. Hardcoded Private Keys (utilities/oidc-server/support/configuration.ts:56)JWKS contains private keys committed to the repository. While this appears to be for testing only, it needs clear documentation to prevent production misuse. Risk: High if used in production, Low if properly documented as test-only Security Concerns 🟡4. Refresh Token Endpoint Lacks Protections (server/server/route/auth.go:163)The
5. 403 Retry Logic (src/lib/utilities/request-from-api.ts:112)The code retries on both 401 (Unauthorized) and 403 (Forbidden). However, 403 typically indicates insufficient permissions, not expired tokens. Refreshing won't fix permission issues. 6. Cookie/Token TTL Mismatch (server/server/auth/auth.go:94)The refresh token cookie expires after 30 days, but the OIDC server's refresh token expires after 1 day. This mismatch will cause confusing auth failures. Code Quality Issues 🟡7. Silent Error Handling (src/lib/utilities/auth-refresh.ts:27)The refresh function silently returns 8. Missing Interceptor AbstractionAs noted by @rossnelson, the request retry logic could benefit from an interceptor pattern for better abstraction and reusability. Positive Aspects ✅
RecommendationsBefore Merging:
Future Improvements:
Test CoverageThe PR includes testing documentation but I cannot verify:
Recommendation: Ensure automated tests cover the happy path and failure scenarios. Files ReviewedFrontend:
Backend:
Test Infrastructure:
Overall AssessmentThis is a solid foundation for token refresh functionality, but should not be merged without addressing the critical security issues, particularly JWT signature validation and race conditions. The test OIDC server is well-implemented but needs clear documentation about its test-only nature. Recommended Action: Request changes to address critical issues before merging. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Implement single-flight pattern to prevent race conditions during concurrent token refresh attempts - Remove 403 from retry logic - only retry on 401 (expired token), not 403 (forbidden/permissions) - Add comprehensive error logging throughout refresh flow for better debugging - Add security warning to OIDC test server configuration file Addresses feedback from PR #2966
![semgrep-managed-scans[bot]](https://avatars.githubusercontent.com/u/56493103?s=60&v=4){kind=small}
feat(oidc-server): integrate standalone OIDC server plugin - Added a new Vite plugin `vite-plugin-oidc-server` to manage the lifecycle of the OIDC server during development. - Replaced the old OIDC server implementation with a modularized version under `utilities/oidc-server`. - Updated `vite.config.ts` to include the new OIDC server plugin. - Enhanced `vite-plugin-temporal-server` and `vite-plugin-ui-server` to support the `with-auth` mode. - Added new dependencies: `desm`, `helmet`, `lodash`, and `nanoid`. - Removed deprecated OIDC server scripts and tests. - Updated `development.yaml` to use the new OIDC server configuration. feat(auth): enhance OIDC server configuration and UI server setup - Introduced `getConfig` function for dynamic OIDC server configuration. - Added `ValidEnv` type and updated `createUIServer` to support `with-auth`. - Updated `development.yaml` and added `with-auth.yaml` for auth configs. - Removed redundant URL validation in `auth.go`. - Added `ejs` dependency and updated `pnpm-lock.yaml`. - Refactored OIDC server initialization and removed unused code. - Introduced `start-oidc-server.ts` script for standalone OIDC server. These changes improve modularity, simplify configuration, and enhance auth-related functionality. feat: improve OIDC and UI server styling and logging - Refactored OIDC server plugin to use a centralized `log` function with colored output for better readability. - Updated UI server plugin to display Temporal UI server port logs. - Enhanced OIDC server views with Tailwind CSS and Google Fonts for modern styling. - Improved interaction and login views with responsive and accessible design. - Removed unnecessary `form-action` directive in OIDC server's CSP configuration to allow form submissions. - Made `ui-server` process quiet during startup for cleaner logs.
Add automatic token refresh functionality to prevent session interruptions when access tokens expire. The UI now seamlessly refreshes tokens using HttpOnly refresh token cookies without requiring user re-authentication. Backend changes: - Add JWT expiration validation in auth middleware - Implement /auth/refresh endpoint for token exchange - Configure OIDC provider to issue refresh tokens - Add offline_access scope to OAuth client config - Set refresh token as HttpOnly cookie (30 day TTL) Frontend changes: - Add automatic refresh on 401 responses - Retry failed requests after successful refresh - Fix SSO redirect to use API server origin - Simplify error handling for auth failures OIDC configuration: - Add refresh_token grant type to client - Add offline_access to supported scopes - Implement issueRefreshToken callback - Configure token TTLs (access: 60s, refresh: 1 day) The refresh flow works as follows: 1. JWT expires after 60 seconds (configurable) 2. API request receives 401 Unauthorized 3. Frontend calls /auth/refresh with HttpOnly cookie 4. Server exchanges refresh token for new access/ID tokens 5. New tokens set in cookies, original request retries 6. User session continues without interruption
- Implement single-flight pattern to prevent race conditions during concurrent token refresh attempts - Remove 403 from retry logic - only retry on 401 (expired token), not 403 (forbidden/permissions) - Add comprehensive error logging throughout refresh flow for better debugging - Add security warning to OIDC test server configuration file Addresses feedback from PR #2966
Replaces the commented-out same-host validation with a more flexible approach that validates returnUrl against configured CORS origins. This prevents open redirect vulnerabilities while supporting cross-origin setups where UI and API are on different hosts.
Update the test OIDC server login form to require a valid email address. The full email is used as the user's email claim, and the prefix is used as the display name.
Update call site to match new function signature after rebase.
rossnelson
force-pushed
the
refresh-token
branch
from
bb7719c to
9c58ded
Compare
|
Replace ParseUnverified with proper OIDC token verification using the provider's JWKS. This addresses the security concern where JWT signatures were not being validated, allowing potentially crafted tokens to bypass expiration checks. - Add package-level tokenVerifier in auth package - Initialize verifier from OIDC provider during route setup - Remove unused golang-jwt dependency
- Add undefined to return type for requestFromAPI - Use Record<string, string> for header manipulation instead of HeadersInit - Fix globalThis type access with proper casting - Accept undefined options in withSecurityOptions
Update tests to expect routeForSsoRedirect instead of routeForLoginPage to match the OAuth/OIDC authentication flow.
![temporal-cicd[bot]](https://avatars.githubusercontent.com/in/357945?s=60&v=4){kind=small}
|
@claude review this PR |
|
Claude finished @rossnelson's task in 4m 21s —— View job PR Review: Refresh Token POC ✅SummaryThis PR implements OAuth2 token refresh functionality to prevent session interruptions when access tokens expire. After reviewing the code, I can confirm that several critical security issues from the previous review have been addressed, but there are still TypeScript strict mode errors that should be resolved before merging. ✅ Fixed Issues from Previous Review1. JWT Signature Validation ✓Previous: JWT signatures were not validated (using func validateJWT(ctx context.Context, tokenString string) error {
if tokenVerifier == nil {
log.Println("[JWT Validation] No verifier configured, skipping validation")
return nil
}
_, err := tokenVerifier.Verify(ctx, tokenString)
// ...
}✅ RESOLVED - Now using proper JWT signature validation 2. Race Condition in Token Refresh ✓Previous: Multiple concurrent 401 errors could trigger parallel refresh attempts let refreshPromise: Promise<boolean> | null = null;
export const refreshTokens = async (): Promise<boolean> => {
if (refreshPromise) {
return refreshPromise; // Reuse existing promise
}
refreshPromise = (async () => {
// ... refresh logic
})();
return refreshPromise;
};✅ RESOLVED - Only one refresh happens at a time 3. Test-Only OIDC Server Documentation ✓Previous: Hardcoded private keys lacked clear warnings // ⚠️ FOR LOCAL DEVELOPMENT AND TESTING ONLY - Contains hardcoded secrets✅ RESOLVED - Clear documentation added 4. Error Logging ✓Previous: Silent failures without logging console.warn(`[Auth] Token refresh failed with status ${res.status}: ${res.statusText}`);
console.info('[Auth] Token refresh successful');
console.error('[Auth] Token refresh network error:', error);✅ RESOLVED - Detailed logging added 5. 403 Retry Logic ✓Previous: Code retried on both 401 and 403 🔴 Critical Issues to FixTypeScript Strict Mode Errors (6 errors in 4 files)The danger bot reports 6 TypeScript errors that will cause CI to fail:
I've added inline comments with specific fix suggestions for the oidc-server errors. Action Required: Fix these TypeScript errors before merging. 🟡 Recommendations1. Cookie/Token TTL Mismatch
The cookie will persist for 30 days but the token expires after 1 day, causing confusing auth failures. See inline comment for details. Recommendation: Align both to the same value or document why they differ. 2. 403 Error Handling BehaviorThe code correctly removed 403 from retry logic in Question: Should 403 (Forbidden) trigger SSO redirect? Typically 403 means "authenticated but not authorized" and shouldn't require re-login. See inline comment for details. 3. Security Hardening for ProductionWhile not critical for a POC, consider these for production:
4. Interceptor PatternConsider Ross's suggestion to implement an interceptor/middleware pattern for better abstraction of the retry logic. ✅ Positive Aspects
Test CoverageThe PR includes excellent testing documentation. Manual testing steps are clear and comprehensive. Consider adding:
Overall AssessmentThis is a well-implemented token refresh solution. The critical security issues from the previous review have been properly addressed:
Remaining work:
Recommended Action: Fix TypeScript errors, then this is ready to merge. The security foundation is sound. Inline comments posted on:
(Note: I couldn't post inline comments on src/lib/utilities/request-from-api.ts:80 and src/lib/utilities/route-for.ts:129 as those lines weren't part of the PR diff, but the TypeScript errors are documented in the danger bot comment above) |
| type Response, | ||
| urlencoded, | ||
| } from 'express'; | ||
| import isEmpty from 'lodash/isEmpty.js'; |
There was a problem hiding this comment.
TypeScript strict mode errors: Missing type declarations for lodash/isEmpty.js and oidc-provider.
Since this is test/dev infrastructure, consider one of these approaches:
Option 1 - Install types:
pnpm add -D @types/lodash @types/oidc-providerOption 2 - Suppress errors for test utilities:
// @ts-expect-error - No types available for oidc-provider
import type Provider from 'oidc-provider';
// @ts-expect-error - No types available for oidc-provider
import { errors } from 'oidc-provider';Option 3 - Skip lib check for utilities:
Add to tsconfig.json:
{
"compilerOptions": {
"skipLibCheck": true
}
}Since this is a local development OIDC server (not production code), Option 2 or 3 would be acceptable.
| refreshCookie := &http.Cookie{ | ||
| Name: "refresh", | ||
| Value: rt, | ||
| MaxAge: int((30 * 24 * time.Hour).Seconds()), |
There was a problem hiding this comment.
Cookie/Token TTL mismatch: The refresh token cookie has a 30-day expiration, but the OIDC server's refresh token expires after 1 day (see utilities/oidc-server/support/configuration.ts:7).
Issue: The cookie will persist for 30 days even though the token itself expires after 1 day, leading to confusing auth failures.
Recommendation:
- Align the cookie MaxAge with the refresh token TTL (1 day for testing)
- For production, this should match your OIDC provider's refresh token TTL
- Or document why they differ if this is intentional
MaxAge: int((24 * time.Hour).Seconds()), // Match refresh token TTL
| window.location.assign(routeForSsoRedirect()); | ||
| } | ||
|
|
||
| if (isForbidden(error) && isBrowser) { |
There was a problem hiding this comment.
Question about 403 behavior: Should 403 (Forbidden) errors redirect to SSO login?
Typically:
- 401 (Unauthorized) = Not authenticated → Redirect to login ✓
- 403 (Forbidden) = Authenticated but not authorized → Show permission denied message
Refreshing tokens won't fix a 403 error because it indicates the user lacks permissions, not that their token is expired.
Consider: Should this show an error message instead of redirecting? Or is your auth system using 403 for expired sessions?
If 403 should redirect to login, please document why. Otherwise, consider:
if (isForbidden(error) && isBrowser) {
toasts.push({
variant: 'error',
message: 'You do not have permission to access this resource'
});
return; // Don't redirect
}Note: request-from-api.ts correctly removed 403 from the retry logic (line 112 now only checks for 401).
Description & motivation 💭
Screenshots (if applicable) 📸
Design Considerations 🎨
Testing 🧪
How was this tested 👻
Steps for others to test: 🚶🏽♂️🚶🏽♀️
Checklists
Draft Checklist
Merge Checklist
Issue(s) closed
Docs
Any docs updates needed?