Refresh token POC #2966
Refresh token POC #2966
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| response = await makeRequest(); | ||
| status = response.status; | ||
| statusText = response.statusText; | ||
| } |
There was a problem hiding this comment.
In the past I've implemented a concept called "interceptors" almost like middleware or maybe more similar to hooks. Basically an abstraction of what this is doing.
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
|
@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}
| status = response.status; | ||
| statusText = response.statusText; | ||
| } | ||
| // If refresh failed, let the error flow to handleError() which will redirect to login |
There was a problem hiding this comment.
Race condition vulnerability: If multiple concurrent requests fail with 401/403, they will all attempt to refresh tokens simultaneously. This could lead to:
- Multiple refresh requests being sent in parallel
- Race conditions updating the auth store
- Potential token conflicts
Recommendation: Implement a single-flight pattern to ensure only one refresh operation happens at a time:
let refreshPromise: Promise<boolean> | null = null;
export const refreshTokens = async (): Promise<boolean> => {
if (refreshPromise) return refreshPromise;
refreshPromise = (async () => {
try {
// ... existing refresh logic
} finally {
refreshPromise = null;
}
})();
return refreshPromise;
};
| return nil | ||
| } | ||
|
|
||
| func validateJWTExpiration(tokenString string) error { |
There was a problem hiding this comment.
Security: JWT signature not validated: This function parses JWT tokens without verifying their signatures (ParseUnverified). While the comment says it's just checking expiration, an attacker could craft a token with a future expiration date and bypass this check.
Impact: An attacker who can modify tokens could extend their validity indefinitely.
Recommendation: Either:
- Validate the JWT signature properly using the OIDC provider's public keys, or
- If you only need expiration checking, clearly document the security implications and ensure this is acceptable for your threat model
Note: Line 175 shows you're validating tokens from the Authorization header, but this validation is not cryptographically sound.
| } | ||
|
|
||
| // Use the refresh token to obtain a new token set | ||
| ts := oauthCfg.TokenSource(ctx, &oauth2.Token{RefreshToken: refreshCookie.Value}) |
There was a problem hiding this comment.
Security: Refresh token validation missing: The refresh token from the cookie is used directly without any validation:
- No check if the token belongs to the current session
- No CSRF protection for the refresh endpoint
- No rate limiting mentioned
Recommendations:
- Consider binding refresh tokens to specific sessions/devices to prevent token theft
- Add CSRF protection since this endpoint accepts cookies (similar to other authenticated endpoints)
- Implement rate limiting to prevent brute-force attacks on stolen refresh tokens
- Consider adding logging for security monitoring
| deviceFlow: { enabled: true }, | ||
| revocation: { enabled: true }, | ||
| }, | ||
| jwks: { |
There was a problem hiding this comment.
Security concern: Hardcoded private keys in source code: The JWKS contains private keys (RSA d parameter and EC d parameter) that should never be committed to a repository.
Impact: Anyone with access to the repository can impersonate your OIDC server and forge tokens.
Recommendations:
- If this is only for local development/testing, add a prominent comment explaining this and ensure it's never used in production
- Generate keys at runtime during server startup
- Consider using environment variables or secure key management
- Add documentation warning about the security implications
Given this appears to be a test OIDC server (based on file path), please ensure:
- This configuration is never used outside of local development
- Production deployments use a properly configured OIDC provider
| refreshCookie := &http.Cookie{ | ||
| Name: "refresh", | ||
| Value: rt, | ||
| MaxAge: int((30 * 24 * time.Hour).Seconds()), |
There was a problem hiding this comment.
Cookie security consideration: The refresh token cookie has a 30-day expiration, which is significantly longer than the 1-day refresh token TTL configured in the OIDC server (utilities/oidc-server/support/configuration.ts:9).
Potential issue: The cookie will persist for 30 days even though the refresh token itself expires after 1 day, leading to confusing auth failures.
Recommendation: Align the cookie MaxAge with the refresh token TTL, or make it slightly shorter to ensure the cookie expires before or when the token does.
- 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}
| @@ -11246,12 +11574,12 @@ packages: | |||
| fsevents: 2.3.2 | |||
| dev: true | |||
|
|
|||
| /[email protected]: | |||
| resolution: {integrity: sha512-aFi5B0WovBHTEvpM3DzXTUaeN6eN0qWnTkKx4NQaH4Wvcmc153PdaY2UBdSYKaGYw+UyWXSVyxDUg5DoPEttjw==} | |||
| /[email protected]: | |||
There was a problem hiding this comment.
High severity vulnerability introduced by a package you're using:
Line 11577 lists a dependency (playwright) with a known High severity vulnerability. Fixing requires upgrading or replacing the dependency.
ℹ️ Why this matters
Affected versions of playwright are vulnerable to Improper Verification of Cryptographic Signature. The macOS browser reinstall scripts in Playwright use curl -k to fetch installer packages without any SSL certificate validation, allowing a man-in-the-middle attacker to serve a trojaned browser installer that's run with elevated privileges, resulting in full system compromise.
To resolve this comment:
Upgrade this dependency to at least version 1.55.1 at pnpm-lock.yaml.
💬 Ignore this finding
To ignore this, reply with:
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
You can view more details on this finding in the Semgrep AppSec Platform here.
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?