fix(security): apply CodeRabbit security feedback (#38)
Browse files* fix(security): apply CodeRabbit security feedback
Addresses 3 actionable items from CodeRabbit review:
1. CORS regex anchors (CRITICAL): Added ^ and $ anchors to prevent
subdomain spoofing attacks like .hf.space.evil.com
2. ErrorBoundary production leak: Error details now only shown in
development mode (import.meta.env.DEV) or when explicitly enabled
via showDetails prop
3. Type safety: Split BackendJobStatus from JobStatus to clearly
distinguish API response types (never include 'waking_up') from
frontend-only states (which do include 'waking_up')
* fix(security): complete CodeRabbit feedback implementation
Additional fixes based on CodeRabbit's full review:
1. componentDidCatch logging (security): Now gates full error+stack
logging behind import.meta.env.DEV. Production only logs minimal
"ErrorBoundary caught an error" message without exposing internals.
2. CORS single source of truth (DRY): Removed regex entirely in favor
of exact-match allow_origins list. Added HF_SPACE_FRONTEND constant
used in single place. This eliminates:
- Regex security surface area
- Dual-source drift risk (list vs regex)
- Potential misconfiguration
Both changes validated from first principles against CodeRabbit claims.
|
@@ -3,6 +3,8 @@ import { Component, type ReactNode } from "react";
|
|
| 3 |
interface Props {
|
| 4 |
children: ReactNode;
|
| 5 |
fallback?: ReactNode;
|
|
|
|
|
|
|
| 6 |
}
|
| 7 |
|
| 8 |
interface State {
|
|
@@ -32,8 +34,13 @@ export class ErrorBoundary extends Component<Props, State> {
|
|
| 32 |
}
|
| 33 |
|
| 34 |
componentDidCatch(error: Error, errorInfo: React.ErrorInfo): void {
|
| 35 |
-
//
|
| 36 |
-
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 37 |
}
|
| 38 |
|
| 39 |
handleRetry = (): void => {
|
|
@@ -53,16 +60,18 @@ export class ErrorBoundary extends Component<Props, State> {
|
|
| 53 |
<p className="text-gray-300">
|
| 54 |
An unexpected error occurred while rendering the application.
|
| 55 |
</p>
|
| 56 |
-
{
|
| 57 |
-
|
| 58 |
-
|
| 59 |
-
|
| 60 |
-
|
| 61 |
-
|
| 62 |
-
|
| 63 |
-
|
| 64 |
-
|
| 65 |
-
|
|
|
|
|
|
|
| 66 |
<button
|
| 67 |
onClick={this.handleRetry}
|
| 68 |
className="bg-blue-600 hover:bg-blue-700 text-white font-medium py-2 px-4 rounded-lg transition-colors"
|
|
|
|
| 3 |
interface Props {
|
| 4 |
children: ReactNode;
|
| 5 |
fallback?: ReactNode;
|
| 6 |
+
/** Show error details even in production (defaults to false) */
|
| 7 |
+
showDetails?: boolean;
|
| 8 |
}
|
| 9 |
|
| 10 |
interface State {
|
|
|
|
| 34 |
}
|
| 35 |
|
| 36 |
componentDidCatch(error: Error, errorInfo: React.ErrorInfo): void {
|
| 37 |
+
// Only log full error details in development (security: don't leak stack traces in prod)
|
| 38 |
+
if (import.meta.env.DEV) {
|
| 39 |
+
console.error("ErrorBoundary caught error:", error, errorInfo);
|
| 40 |
+
} else {
|
| 41 |
+
// Minimal production logging - no stack traces or internal details
|
| 42 |
+
console.error("ErrorBoundary caught an error");
|
| 43 |
+
}
|
| 44 |
}
|
| 45 |
|
| 46 |
handleRetry = (): void => {
|
|
|
|
| 60 |
<p className="text-gray-300">
|
| 61 |
An unexpected error occurred while rendering the application.
|
| 62 |
</p>
|
| 63 |
+
{/* Only show error details in development or if explicitly enabled (security) */}
|
| 64 |
+
{this.state.error &&
|
| 65 |
+
(import.meta.env.DEV || this.props.showDetails) && (
|
| 66 |
+
<details className="text-left">
|
| 67 |
+
<summary className="text-gray-400 cursor-pointer hover:text-gray-300">
|
| 68 |
+
Error details
|
| 69 |
+
</summary>
|
| 70 |
+
<pre className="mt-2 p-2 bg-gray-900 rounded text-xs text-red-300 overflow-auto max-h-32">
|
| 71 |
+
{this.state.error.message}
|
| 72 |
+
</pre>
|
| 73 |
+
</details>
|
| 74 |
+
)}
|
| 75 |
<button
|
| 76 |
onClick={this.handleRetry}
|
| 77 |
className="bg-blue-600 hover:bg-blue-700 text-white font-medium py-2 px-4 rounded-lg transition-colors"
|
|
@@ -30,25 +30,23 @@ export interface SegmentResponse {
|
|
| 30 |
warning?: string | null;
|
| 31 |
}
|
| 32 |
|
| 33 |
-
// Job Status Types
|
| 34 |
-
export type
|
| 35 |
-
|
| 36 |
-
|
| 37 |
-
|
| 38 |
-
| "failed"
|
| 39 |
-
| "waking_up";
|
| 40 |
|
| 41 |
// Response from POST /api/segment (job creation)
|
| 42 |
export interface CreateJobResponse {
|
| 43 |
jobId: string;
|
| 44 |
-
status:
|
| 45 |
message: string;
|
| 46 |
}
|
| 47 |
|
| 48 |
// Response from GET /api/jobs/{jobId} (status polling)
|
| 49 |
export interface JobStatusResponse {
|
| 50 |
jobId: string;
|
| 51 |
-
status:
|
| 52 |
progress: number;
|
| 53 |
progressMessage: string;
|
| 54 |
elapsedSeconds?: number;
|
|
|
|
| 30 |
warning?: string | null;
|
| 31 |
}
|
| 32 |
|
| 33 |
+
// Backend Job Status Types (returned by API - never includes 'waking_up')
|
| 34 |
+
export type BackendJobStatus = "pending" | "running" | "completed" | "failed";
|
| 35 |
+
|
| 36 |
+
// Frontend Job Status Types (includes client-side states like 'waking_up')
|
| 37 |
+
export type JobStatus = BackendJobStatus | "waking_up";
|
|
|
|
|
|
|
| 38 |
|
| 39 |
// Response from POST /api/segment (job creation)
|
| 40 |
export interface CreateJobResponse {
|
| 41 |
jobId: string;
|
| 42 |
+
status: BackendJobStatus; // Backend never returns 'waking_up'
|
| 43 |
message: string;
|
| 44 |
}
|
| 45 |
|
| 46 |
// Response from GET /api/jobs/{jobId} (status polling)
|
| 47 |
export interface JobStatusResponse {
|
| 48 |
jobId: string;
|
| 49 |
+
status: BackendJobStatus; // Backend never returns 'waking_up'
|
| 50 |
progress: number;
|
| 51 |
progressMessage: string;
|
| 52 |
elapsedSeconds?: number;
|
|
@@ -97,24 +97,30 @@ class CORPMiddleware(BaseHTTPMiddleware):
|
|
| 97 |
return response
|
| 98 |
|
| 99 |
|
| 100 |
-
# CORS configuration
|
| 101 |
-
|
| 102 |
-
|
|
|
|
|
|
|
| 103 |
"http://localhost:5173", # Vite dev server
|
| 104 |
"http://localhost:3000", # Alternative local port
|
|
|
|
| 105 |
]
|
| 106 |
-
|
|
|
|
|
|
|
|
|
|
| 107 |
CORS_ORIGINS.append(FRONTEND_ORIGIN)
|
| 108 |
|
| 109 |
# Add CORP middleware first (for COEP compatibility)
|
| 110 |
app.add_middleware(CORPMiddleware)
|
| 111 |
|
| 112 |
# Add CORS middleware with strict security settings
|
|
|
|
|
|
|
| 113 |
app.add_middleware(
|
| 114 |
CORSMiddleware,
|
| 115 |
allow_origins=CORS_ORIGINS,
|
| 116 |
-
# Anchored regex: only allow our specific HF Space (security fix for BUG-002)
|
| 117 |
-
allow_origin_regex=r"https://vibecodermcswaggins-stroke-viewer-frontend\.hf\.space",
|
| 118 |
allow_credentials=False, # Not needed - no cookies/auth
|
| 119 |
allow_methods=["GET", "POST"], # Only methods we use
|
| 120 |
allow_headers=["Content-Type"], # Only headers we need
|
|
|
|
| 97 |
return response
|
| 98 |
|
| 99 |
|
| 100 |
+
# CORS configuration - Single source of truth (no regex needed for exact origins)
|
| 101 |
+
# Production HF Space frontend origin
|
| 102 |
+
HF_SPACE_FRONTEND = "https://vibecodermcswaggins-stroke-viewer-frontend.hf.space"
|
| 103 |
+
|
| 104 |
+
CORS_ORIGINS: list[str] = [
|
| 105 |
"http://localhost:5173", # Vite dev server
|
| 106 |
"http://localhost:3000", # Alternative local port
|
| 107 |
+
HF_SPACE_FRONTEND, # Production HF Space frontend
|
| 108 |
]
|
| 109 |
+
|
| 110 |
+
# Allow override via environment variable (for custom deployments)
|
| 111 |
+
FRONTEND_ORIGIN = os.environ.get("FRONTEND_ORIGIN", "")
|
| 112 |
+
if FRONTEND_ORIGIN and FRONTEND_ORIGIN not in CORS_ORIGINS:
|
| 113 |
CORS_ORIGINS.append(FRONTEND_ORIGIN)
|
| 114 |
|
| 115 |
# Add CORP middleware first (for COEP compatibility)
|
| 116 |
app.add_middleware(CORPMiddleware)
|
| 117 |
|
| 118 |
# Add CORS middleware with strict security settings
|
| 119 |
+
# Note: Using allow_origins list for exact matching (no regex needed)
|
| 120 |
+
# This eliminates regex security concerns while maintaining single source of truth
|
| 121 |
app.add_middleware(
|
| 122 |
CORSMiddleware,
|
| 123 |
allow_origins=CORS_ORIGINS,
|
|
|
|
|
|
|
| 124 |
allow_credentials=False, # Not needed - no cookies/auth
|
| 125 |
allow_methods=["GET", "POST"], # Only methods we use
|
| 126 |
allow_headers=["Content-Type"], # Only headers we need
|