feat: secure copilot token generation via server action #1143
Conversation
- Add getCopilotTokenAction server action to fetch temp tokens from manage API - Add useCopilotToken hook with auto-refresh before expiry - Update CopilotChat to use server action instead of exposing bypass secret Requires INKEEP_COPILOT_API_KEY env var on manage-ui server
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Claude finished @amikofalvy's task —— View job 🔍 TypeScript Staff Engineer Code ReviewI've completed a comprehensive architectural review of the secure copilot token implementation. This PR represents a significant security improvement by replacing the exposed bypass secret with a proper JWT-based token system. ✅ Excellent Security ImprovementsThe core architectural change is sound and addresses the fundamental security issue:
🎯 Architecture & PatternsStrengths
Areas for Consideration🔒 Security & Error HandlingHIGH PRIORITY - Error Information Leakage ( let errorMessage = 'Failed to fetch copilot token';
try {
const errorData = await response.json();
errorMessage = errorData?.error?.message || errorData?.message || errorMessage;
} catch {
// Ignore JSON parse errors
}Issue: This exposes potentially sensitive API error details to the client. Consider sanitizing error messages or using generic errors for security. Recommendation: // Log the actual error server-side for debugging
console.error('Copilot token API error:', errorData);
// Return generic error to client
return {
success: false,
error: 'Authentication service temporarily unavailable',
code: 'api_error',
};⚡ Performance & Race ConditionsMEDIUM PRIORITY - Multiple Simultaneous Requests (
Recommendation: Add request deduplication: const [isRefreshing, setIsRefreshing] = useState(false);
const fetchToken = useCallback(async () => {
if (isRefreshing) return; // Prevent concurrent requests
try {
setIsRefreshing(true);
setIsLoading(true);
// ... existing logic
} finally {
setIsRefreshing(false);
setIsLoading(false);
}
}, [isRefreshing]);MEDIUM PRIORITY - Timer Cleanup Edge Case ( Recommendation: useEffect(() => {
if (!expiresAt) return;
const expiryTime = new Date(expiresAt).getTime();
const now = Date.now();
const timeUntilExpiry = expiryTime - now;
// Don't set timer if token is already expired or expires very soon
if (timeUntilExpiry <= 5 * 60 * 1000) {
fetchToken();
return;
}
const refreshTime = timeUntilExpiry - 5 * 60 * 1000;
const timer = setTimeout(() => {
console.log('Auto-refreshing copilot token before expiry...');
fetchToken();
}, refreshTime);
return () => clearTimeout(timer);
}, [expiresAt, fetchToken]);🏗️ Type Safety & ArchitectureLOW PRIORITY - Type Duplication Recommendation: Import from the shared types file: // copilot-token.ts
import type { ActionResult } from './types';
// Remove duplicate type definitionLOW PRIORITY - API Response Type Safety ( Recommendation: Consider using a schema validation library (like zod) or at minimum add runtime checks: const data = await response.json();
if (!data.apiKey || !data.expiresAt) {
throw new Error('Invalid API response format');
}🔧 Configuration & Error BoundariesMEDIUM PRIORITY - Environment Variable Validation Current: if (!copilotTenantId || !copilotProjectId || !copilotAgentId) {
return {
success: false,
error: 'Copilot tenant, project, or agent ID is not configured',
code: 'configuration_error',
};
}Recommendation: More specific error messages for easier debugging: const missingVars = [];
if (!copilotTenantId) missingVars.push('PUBLIC_INKEEP_COPILOT_TENANT_ID');
if (!copilotProjectId) missingVars.push('PUBLIC_INKEEP_COPILOT_PROJECT_ID');
if (!copilotAgentId) missingVars.push('PUBLIC_INKEEP_COPILOT_AGENT_ID');
if (missingVars.length > 0) {
return {
success: false,
error: `Missing environment variables: ${missingVars.join(', ')}`,
code: 'configuration_error',
};
}🎯 Minor Improvements
📋 Missing Considerations
🏁 Overall AssessmentThis is a well-architected security improvement that follows established patterns and significantly enhances the security posture. The implementation is clean, consistent, and addresses the core security concern effectively. Recommended Priority for Fixes:
The changes demonstrate solid TypeScript practices and good understanding of React hooks patterns. The security benefits far outweigh the minor areas for improvement identified above. |
- Add exponential backoff retry (3 attempts) for transient errors - Skip retries for configuration errors - Show loading state with retry count indicator - Show error alert with 'Try Again' button when all retries fail - Track mounted state to prevent state updates on unmounted component
|
This pull request has been automatically marked as stale because it has not had recent activity. If this PR is still relevant:
Thank you for your contributions! |
Summary
Replaces the exposed bypass secret in CopilotChat with a secure server action that fetches temporary JWT tokens, with proper retry behavior and error handling.
Changes
New Files
agents-manage-ui/src/lib/actions/copilot-token.ts- Server action that calls manage API's playground token endpointagents-manage-ui/src/hooks/use-copilot-token.ts- Client hook with:Modified Files
agents-manage-ui/src/components/agent/copilot/copilot-chat.tsx- Uses new hook with proper error states:Visual States
Configuration
Requires new environment variable on manage-ui server:
Security Benefits