New Login Flow vs code#12
Conversation
|
🐕 Corgea found the following SCA issues in the codebase:
Showing 8 out of 9 findings. See full results |
There was a problem hiding this comment.
Greptile Summary
This PR implements a comprehensive OAuth-based authentication system for the Corgea VS Code extension, replacing the previous manual API key entry method. The changes introduce three authentication methods: standard OAuth login, enterprise OAuth with custom scope support, and fallback API key login.
The core authentication flow is handled by significant updates to AuthService, which now manages OAuth state, generates callback URIs using the extension context, and processes OAuth callbacks through a new URI navigation event handler. The service supports both public Corgea instances and enterprise deployments with custom domain scopes (e.g., company-name.corgea.app).
The UI components have been completely redesigned with a new multi-state authentication container that displays different interfaces based on the authentication method selected. The AuthContainer component now handles OAuth loading states, enterprise form validation with regex pattern matching, and provides proper error handling and user feedback.
New VS Code commands have been registered (corgea.oauthLogin, corgea.submitEnterpriseLogin, corgea.loginWithApiKey, corgea.cancelOAuth) along with URI handling capabilities to process OAuth callbacks. The extension's package.json has been updated with the onUri activation event and URI handler configuration to support the OAuth callback flow.
The authentication state is managed through the existing VulnerabilitiesContext, which has been extended with OAuth loading states and enterprise form visibility tracking. The webview provider has been updated to handle the new authentication message types and communicate loading states back to the UI.
A new exchangeOAuthCode method in APIManager handles the OAuth code-to-token exchange, though it bypasses the existing client infrastructure for simplicity. The extension's initialization logic has been modified to conditionally initialize services that require the extension context.
Confidence score: 2/5
- This PR contains critical security issues that make it unsafe to merge, particularly the hardcoded Sentry DSN in production code
- Score lowered due to security vulnerabilities, potential type safety issues with service casting, and OAuth implementation concerns
- Pay close attention to
ErrorHandlingManager.ts(hardcoded secrets),extension.ts(type safety bypass), andauthService.ts(OAuth state management)
Context used:
Style Guide - Corgea Docs (link)
10 files reviewed, 8 comments
| if ((service as any).initialize) { | ||
| (service as any).initialize(context); | ||
| } |
There was a problem hiding this comment.
style: Using (service as any) bypasses TypeScript's type safety. Consider defining a common interface with an optional initialize method.
| public static async exchangeOAuthCode(code: string, corgeaUrl: string): Promise<{status: string, user_token?: string}> { | ||
| try { | ||
| const response = await axios.get(`${corgeaUrl}/api/${this.apiVersion}/authorize`, { | ||
| params: { code }, | ||
| timeout: 10000 | ||
| }); | ||
|
|
||
| return response.data; | ||
| } catch (error: any) { | ||
| DebugManager.log(`OAuth code exchange failed: ${error.message}`); | ||
| throw new Error(`Failed to exchange OAuth code: ${error.response?.data?.error || error.message}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
style: This method bypasses the established getBaseClient() pattern used throughout the class. Consider using the existing infrastructure for consistency, even with throwOnMissingToken: false to handle the unauthenticated nature of this endpoint.
| .loading-spinner { | ||
| width: 24px; | ||
| height: 24px; | ||
| border: 3px solid var(--vscode-input-border); | ||
| border-top: 3px solid var(--vscode-button-background); | ||
| border-radius: 50%; | ||
| animation: spin 1s linear infinite; | ||
| margin-bottom: 0.5rem; | ||
| } | ||
|
|
||
| @keyframes spin { | ||
| 0% { transform: rotate(0deg); } | ||
| 100% { transform: rotate(360deg); } | ||
| } |
There was a problem hiding this comment.
style: Custom spinner implementation duplicates existing LoadingSpinner component functionality. Consider reusing the existing component for consistency.
| @@ -1,17 +1,98 @@ | ||
| import React from 'react'; | ||
| import { useVulnerabilities } from '../../context/VulnerabilitiesContext'; | ||
| import LoadingSpinner from '../LoadingSpinner/LoadingSpinner'; |
There was a problem hiding this comment.
style: LoadingSpinner component imported but not used - using custom div instead
| import LoadingSpinner from '../LoadingSpinner/LoadingSpinner'; | |
| <LoadingSpinner /> |
| {scopeError && <div className="form-error">{scopeError}</div>} | ||
| </div> | ||
| <div className="form-buttons"> | ||
| <button type="button" className="btn btn-secondary" onClick={handleCancel}> |
There was a problem hiding this comment.
syntax: Missing indentation - should align with other button at line 76
Context Used: Style Guide - Corgea Docs (link)
src/services/index.ts
Outdated
| import VulnerabilityHighlightingService from "./vulnerabilityHighlightingService"; | ||
| import scanningService from "./scanningService"; | ||
|
|
||
There was a problem hiding this comment.
style: Unnecessary trailing whitespace should be removed
Context Used: Style Guide - Corgea Docs (link)
| login: () => { | ||
| vscode.postMessage({ type: 'login' }); | ||
| vscode.postMessage({ type: 'oauthLogin' }); | ||
| }, | ||
| oauthLogin: () => { | ||
| vscode.postMessage({ type: 'oauthLogin' }); |
There was a problem hiding this comment.
style: both login and oauthLogin now do identical operations - the login method could simply call oauthLogin() instead of duplicating the message posting
| const checkInterval = setInterval(() => { | ||
| if (!AuthService._isOAuthInProgress) { | ||
| clearInterval(checkInterval); | ||
| resolve(); | ||
| } | ||
| }, 1000); | ||
|
|
||
| // Set a timeout for the authentication | ||
| setTimeout(() => { | ||
| if (AuthService._isOAuthInProgress) { | ||
| clearInterval(checkInterval); | ||
| AuthService._isOAuthInProgress = false; | ||
| AuthService._oauthCancelToken?.dispose(); | ||
| AuthService._oauthCancelToken = null; | ||
| reject(new Error("Authentication timed out. Please try again.")); | ||
| } | ||
| }, 300000); // 5 minutes timeout |
There was a problem hiding this comment.
logic: The polling mechanism creates potential memory leaks - the interval continues running even after timeout, and multiple intervals could accumulate if authentication is retried quickly
|
@Ibrahimrahhal looks good overall. Is the greptile comment correct on the security issue? Why did we hardcode the DSN? |
14996be to
4604d43
Compare
There was a problem hiding this comment.
Greptile Summary
This PR implements a comprehensive new OAuth-based authentication flow for the Corgea VS Code extension, replacing the previous simple API key authentication. The changes introduce three distinct authentication methods: OAuth login (now the default), enterprise login with custom scope configuration, and traditional API key login for backward compatibility.
The implementation spans the entire authentication stack:
- Frontend UI (
AuthContainer): Now supports three states - OAuth loading with cancellation, enterprise form with scope validation, and default login options with separate buttons for OAuth and enterprise authentication - State Management (
VulnerabilitiesContext): Extended to handle OAuth loading states, enterprise form visibility, and new authentication action types while maintaining backward compatibility - Backend Services (
AuthService): Completely rewritten to support OAuth 2.0 authorization code flow with proper state management, callback handling, and enterprise authentication for custom Corgea deployments - API Integration (
apiManager): AddedexchangeOAuthCodemethod to handle OAuth token exchange - Extension Integration: Updated command registration in
package.jsonand message handling invulnerabilitiesWebviewProviderto support the new authentication flows - Styling (
AuthContainer.css): Significantly expanded to accommodate multiple authentication methods, loading states, form controls, and proper VS Code theme integration
The OAuth flow follows standard practices: users are redirected to Corgea's authorization server via browser, authenticate there, and are redirected back to VS Code with an authorization code that gets exchanged for an API token. Enterprise users can specify custom scopes/domains before initiating OAuth. The system includes proper error handling, user feedback through VS Code notifications, and cancellation mechanisms for improved UX.
Confidence score: 3/5
- This PR introduces complex authentication logic with potential runtime issues from type casting and polling mechanisms
- Score reflects the complexity of OAuth implementation and several architectural concerns that could cause problems
- Pay close attention to
src/services/authService.tsfor memory leak risks andsrc/extension.tsfor type safety bypasses
Context used:
Style Guide - Corgea Docs (link)
10 files reviewed, no comments
asadeddin
left a comment
There was a problem hiding this comment.
Fix login to not be 2 buttons
| @@ -1,17 +1,104 @@ | ||
| import React from 'react'; | ||
| import { useVulnerabilities } from '../../context/VulnerabilitiesContext'; | ||
| import LoadingSpinner from '../LoadingSpinner/LoadingSpinner'; |
There was a problem hiding this comment.
Assignment to Variable without Use ( ⚪ Low ) - The variable's value is assigned but never used, making it a dead store. View in Corgea ↗
More Details
🎟️Issue Explanation: The import "LoadingSpinner" is declared but never used in the code, making it a dead store. This means the variable occupies memory without purpose.
- Dead stores increase code clutter, reducing maintainability and readability of the file.
- Unused imports can slow down build tools and increase bundle size, impacting performance.
- It confuses engineers by implying "LoadingSpinner" is needed, adding cognitive load.
We could not generate a fix for this.
| const [showRightIndicator, setShowRightIndicator] = useState(false); | ||
|
|
||
| const scanningMode = state.isInScanningMode; | ||
| const handleTabClick = (tab: 'code' | 'sca' | 'scanning') => { |
There was a problem hiding this comment.
Exposure of Resource to Wrong Sphere ( 🟠 Medium ) - The product exposes a resource to the wrong control sphere, providing unintended actors with inappropriate access to the resource. View in Corgea ↗
More Details
🎟️Issue Explanation: The code permits switching tabs based on "state.ideScanningEnabled", but doesn't restrict access control elsewhere, potentially exposing the "scanning" tab's resource to unauthorized actors.
- Improper access control on "scanning" tab means unintended users could interact with sensitive data or features.
- Using UI state guard ("state.ideScanningEnabled") alone doesn't prevent backend or deeper access issues.
- This can cause maintenance challenges as team members may assume UI restrictions imply security, leading to hidden vulnerabilities.
We could not generate a fix for this.
|
🐕 Corgea found the following new SCA issues in the codebase:
Showing 1 out of 1 findings. See full results |
| dsn: "DSN_PLACEHOLDER", | ||
| // Setting this option to true will send default PII data to Sentry. |
There was a problem hiding this comment.
Assignment to Variable without Use ( 🟠 Medium ) - The variable's value is assigned but never used, making it a dead store. View in Corgea ↗
More Details
🎟️Issue Explanation: The code assigns the string "DSN_PLACEHOLDER" to the variable "dsn" but never uses its value anywhere. This results in a dead store, which is unnecessary code execution and memory usage.
- Dead stores in "dsn: "DSN_PLACEHOLDER"" increase code clutter, reducing readability for developers.
- Unused assignments consume memory and processing without benefits, impacting performance subtly.
- Accumulation of dead code can confuse engineers on the actual program flow, slowing debugging and maintenance.
🪄Fix Explanation: Replacing the hardcoded DSN with an environment variable improves flexibility and security by removing sensitive data from the source code and enabling configuration without code changes.
"dsn: "DSN_PLACEHOLDER"" was replaced with "dsn: process.env.SENTRY_DSN || """ to externalize configuration.
This avoids exposing sensitive credentials in the codebase, enhancing security and maintainability.
Using "process.env.SENTRY_DSN || """ ensures the app gracefully handles missing environment variables by defaulting to an empty string.
This approach supports different deployment environments without modifying code, improving consistency and flexibility.
💡Important Instructions: Ensure the environment variable
SENTRY_DSN is properly set in all deployment environments for Sentry integration to work as expected.
| dsn: "DSN_PLACEHOLDER", | |
| // Setting this option to true will send default PII data to Sentry. | |
| dsn: process.env.SENTRY_DSN || "", |
|
🐕 Corgea found the following new SCA issues in the codebase:
Showing 1 out of 1 findings. See full results |
Recording.2025-08-29.163654.mp4