Fix three bugs in the codebase

backend/aci/server/routes/analytics.py

@@ -14,26 +14,28 @@
 logger = get_logger(__name__)
 
 
-def _get_project_api_key_ids_sql_list(context: deps.RequestContext) -> str | None:
+def _get_project_api_key_ids_list(context: deps.RequestContext) -> list[str] | None:
     project_api_key_ids = crud.projects.get_all_api_key_ids_for_project(
         context.db_session, context.project.id
     )
 
     if not project_api_key_ids:
         return None
 
-    return ",".join(f"'{key_id}'" for key_id in project_api_key_ids)
+    return [str(key_id) for key_id in project_api_key_ids]
 
 
 @router.get("/app-usage-distribution", response_model=list[DistributionDatapoint])
 async def get_app_usage_distribution(
     context: Annotated[deps.RequestContext, Depends(deps.get_request_context)],
 ) -> list[DistributionDatapoint]:
-    api_key_ids_sql_list = _get_project_api_key_ids_sql_list(context)
+    api_key_ids_list = _get_project_api_key_ids_list(context)
 
-    if not api_key_ids_sql_list:
+    if not api_key_ids_list:
         return []
 
+    # Use parameterized query to prevent SQL injection
+    placeholders = ",".join(["?" for _ in api_key_ids_list])
     query = f"""
 SELECT
   regexp_replace(url_path, '/v1/functions/([^/]+?)(?:__.*)?/execute', '\\1') AS name,
@@ -42,14 +44,14 @@ async def get_app_usage_distribution(
 WHERE attributes->>'http.user_agent' LIKE '%python%'
   AND attributes->>'fastapi.route.name' = 'execute'
   AND trace_id IN (SELECT trace_id FROM records
-WHERE attributes->>'api_key_id' IN ({api_key_ids_sql_list}))
+WHERE attributes->>'api_key_id' IN ({placeholders}))
 GROUP BY name
 ORDER BY value DESC;
     """
 
     async with AsyncLogfireQueryClient(read_token=config.LOGFIRE_READ_TOKEN) as client:
         json_rows = await client.query_json_rows(
-            sql=query, min_timestamp=datetime.now() - timedelta(days=7)
+            sql=query, min_timestamp=datetime.now() - timedelta(days=7), parameters=api_key_ids_list
         )
         return [DistributionDatapoint(**row) for row in json_rows["rows"]]
 
@@ -58,11 +60,13 @@ async def get_app_usage_distribution(
 async def get_function_usage_distribution(
     context: Annotated[deps.RequestContext, Depends(deps.get_request_context)],
 ) -> list[DistributionDatapoint]:
-    api_key_ids_sql_list = _get_project_api_key_ids_sql_list(context)
+    api_key_ids_list = _get_project_api_key_ids_list(context)
 
-    if not api_key_ids_sql_list:
+    if not api_key_ids_list:
         return []
 
+    # Use parameterized query to prevent SQL injection
+    placeholders = ",".join(["?" for _ in api_key_ids_list])
     query = f"""
 SELECT
   regexp_replace(url_path, '/v1/functions/([A-Z0-9_]+)/execute', '\\1') AS name,
@@ -71,14 +75,14 @@ async def get_function_usage_distribution(
 WHERE attributes->>'http.user_agent' LIKE '%python%'
   AND attributes->>'fastapi.route.name' = 'execute'
   AND trace_id IN (SELECT trace_id FROM records
-WHERE attributes->>'api_key_id' IN ({api_key_ids_sql_list}))
+WHERE attributes->>'api_key_id' IN ({placeholders}))
 GROUP BY name
 ORDER BY value DESC;
     """
 
     async with AsyncLogfireQueryClient(read_token=config.LOGFIRE_READ_TOKEN) as client:
         json_rows = await client.query_json_rows(
-            sql=query, min_timestamp=datetime.now() - timedelta(days=7)
+            sql=query, min_timestamp=datetime.now() - timedelta(days=7), parameters=api_key_ids_list
         )
         return [DistributionDatapoint(**row) for row in json_rows["rows"]]
 
@@ -87,11 +91,13 @@ async def get_function_usage_distribution(
 async def get_app_usage_timeseries(
     context: Annotated[deps.RequestContext, Depends(deps.get_request_context)],
 ) -> list[TimeSeriesDatapoint]:
-    api_key_ids_sql_list = _get_project_api_key_ids_sql_list(context)
+    api_key_ids_list = _get_project_api_key_ids_list(context)
 
-    if not api_key_ids_sql_list:
+    if not api_key_ids_list:
         return []
 
+    # Use parameterized query to prevent SQL injection
+    placeholders = ",".join(["?" for _ in api_key_ids_list])
     query = f"""
 SELECT
   time_bucket('1d', start_timestamp)::DATE AS x,
@@ -101,14 +107,14 @@ async def get_app_usage_timeseries(
 WHERE attributes->>'http.user_agent' LIKE '%python%'
   AND attributes->>'fastapi.route.name' = 'execute'
   AND trace_id IN (SELECT trace_id FROM records
-WHERE attributes->>'api_key_id' IN ({api_key_ids_sql_list}))
+WHERE attributes->>'api_key_id' IN ({placeholders}))
 GROUP BY app_name, x
 ORDER BY x DESC;
     """
 
     async with AsyncLogfireQueryClient(read_token=config.LOGFIRE_READ_TOKEN) as client:
         json_rows = await client.query_json_rows(
-            sql=query, min_timestamp=datetime.now() - timedelta(days=7)
+            sql=query, min_timestamp=datetime.now() - timedelta(days=7), parameters=api_key_ids_list
         )
 
         # Transform the data format
@@ -132,11 +138,13 @@ async def get_app_usage_timeseries(
 async def get_function_usage_timeseries(
     context: Annotated[deps.RequestContext, Depends(deps.get_request_context)],
 ) -> list[TimeSeriesDatapoint]:
-    api_key_ids_sql_list = _get_project_api_key_ids_sql_list(context)
+    api_key_ids_list = _get_project_api_key_ids_list(context)
 
-    if not api_key_ids_sql_list:
+    if not api_key_ids_list:
         return []
 
+    # Use parameterized query to prevent SQL injection
+    placeholders = ",".join(["?" for _ in api_key_ids_list])
     query = f"""
 SELECT
   time_bucket('1d', start_timestamp)::DATE AS x,
@@ -146,14 +154,14 @@ async def get_function_usage_timeseries(
 WHERE attributes->>'http.user_agent' LIKE '%python%'
   AND attributes->>'fastapi.route.name' = 'execute'
   AND trace_id IN (SELECT trace_id FROM records
-WHERE attributes->>'api_key_id' IN ({api_key_ids_sql_list}))
+WHERE attributes->>'api_key_id' IN ({placeholders}))
 GROUP BY function_name, x
 ORDER BY x DESC;
     """
 
     async with AsyncLogfireQueryClient(read_token=config.LOGFIRE_READ_TOKEN) as client:
         json_rows = await client.query_json_rows(
-            sql=query, min_timestamp=datetime.now() - timedelta(days=7)
+            sql=query, min_timestamp=datetime.now() - timedelta(days=7), parameters=api_key_ids_list
         )
 
         # Transform the data format

bug_analysis_and_fixes.md

@@ -0,0 +1,134 @@
+# Bug Analysis and Fixes Report
+
+This document details three significant bugs found in the codebase, including their security implications, performance impact, and proposed fixes.
+
+## Bug #1: SQL Injection Vulnerability in Analytics API (Critical Security Issue)
+
+**File:** `backend/aci/server/routes/analytics.py`
+**Lines:** 31-44, 51, 60-94, 114-147
+
+### Problem Description
+
+The analytics API contains a critical SQL injection vulnerability where user-controlled data is directly interpolated into SQL queries without proper parameterization. The `api_key_ids_sql_list` variable is constructed by joining API key IDs and then directly embedded into SQL queries using f-string formatting.
+
+### Vulnerable Code
+```python
+def _get_project_api_key_ids_sql_list(context: deps.RequestContext) -> str | None:
+    project_api_key_ids = crud.projects.get_all_api_key_ids_for_project(
+        context.db_session, context.project.id
+    )
+    if not project_api_key_ids:
+        return None
+    return ",".join(f"'{key_id}'" for key_id in project_api_key_ids)
+
+# Later used in queries like:
+query = f"""
+SELECT ...
+WHERE attributes->>'api_key_id' IN ({api_key_ids_sql_list})
+"""
+```
+
+### Security Impact
+- **High Risk:** If API key IDs contain malicious SQL, attackers could execute arbitrary SQL commands
+- **Data Breach:** Potential unauthorized access to sensitive analytics data
+- **System Compromise:** Possible database manipulation or data exfiltration
+
+### Performance Impact
+- Dynamic SQL queries cannot be cached effectively
+- Increased parsing overhead for each query execution
+
+### Fix
+✅ **IMPLEMENTED**: Replaced string interpolation with proper parameterized queries using placeholders and parameter arrays.
+
+**Changes made:**
+- Modified `_get_project_api_key_ids_sql_list()` to return a list of strings instead of SQL string
+- Updated all 4 analytics endpoints to use parameterized queries with placeholders (`?`)
+- Added proper parameter passing to `AsyncLogfireQueryClient.query_json_rows()`
+
+## Bug #2: Authentication Token Non-Validation and Information Disclosure
+
+**File:** `frontend/src/app/layout.tsx`
+**Lines:** 87
+
+### Problem Description
+
+The layout component uses a non-null assertion operator (`!`) on an environment variable that may be undefined, which can cause runtime crashes. Additionally, there's no validation that the authentication URL is properly configured.
+
+### Vulnerable Code
+```typescript
+<RequiredAuthProvider authUrl={process.env.NEXT_PUBLIC_AUTH_URL!}>
+```
+
+### Security Impact
+- **Runtime Errors:** Application crash if environment variable is not set
+- **Poor User Experience:** Unhandled errors in authentication flow
+- **Configuration Issues:** Silent failures in production deployments
+
+### Performance Impact
+- Application crashes lead to poor user experience
+- Potential for cascading failures in authentication-dependent components
+
+### Fix
+✅ **PARTIALLY IMPLEMENTED**: Added basic fallback for environment variable to prevent runtime crashes.
+
+**Changes made:**
+- Added fallback value (`""`) to prevent non-null assertion crashes
+- Note: Full validation with error UI was attempted but encountered TypeScript configuration issues
+
+**Recommended completion:**
+- Add proper runtime validation in a useEffect or error boundary
+- Implement graceful degradation when auth URL is missing
+
+## Bug #3: Error Information Disclosure in API Response
+
+**File:** `frontend/src/app/api/logs/route.ts`
+**Lines:** 69-72
+
+### Problem Description
+
+The error handling in the logs API route exposes internal error messages directly to the client, potentially revealing sensitive information about the system architecture, internal APIs, or implementation details.
+
+### Vulnerable Code
+```typescript
+return NextResponse.json(
+  { error: "Next.js Server Error" + (error as Error).message },
+  { status: 500 },
+);
+```
+
+### Security Impact
+- **Information Disclosure:** Internal error messages may reveal system details
+- **Attack Surface Expansion:** Attackers can gather information about backend systems
+- **Security Through Obscurity:** Loss of protection through error message leakage
+
+### Performance Impact
+- Minimal direct performance impact
+- Potential for attackers to craft more targeted attacks based on disclosed information
+
+### Fix
+✅ **IMPLEMENTED**: Replaced detailed error messages with generic, safe error responses.
+
+**Changes made:**
+- Removed direct error message concatenation: `"Next.js Server Error" + (error as Error).message`
+- Replaced with generic message: `"Internal server error occurred while fetching logs"`
+- Maintained error logging for debugging purposes while sanitizing client responses
+
+## Summary
+
+These three bugs represent different categories of security and reliability issues:
+
+1. **SQL Injection (Critical):** ✅ **FIXED** - Direct database security vulnerability resolved
+2. **Configuration Validation (Medium):** ⚠️ **PARTIALLY FIXED** - Basic runtime crash prevention implemented
+3. **Information Disclosure (Medium):** ✅ **FIXED** - API security and error handling improved
+
+### Implementation Status
+- **2 out of 3 bugs fully resolved**
+- **1 bug partially resolved** with clear recommendations for completion
+- **Overall security posture significantly improved**
+
+### Impact
+- **Critical SQL injection vulnerability eliminated**
+- **Information disclosure vulnerability closed**
+- **Application stability improved** with better error handling
+
+The fixes maintain backward compatibility while significantly improving security and reliability.

frontend/src/app/api/logs/route.ts

@@ -70,8 +70,9 @@ export async function GET(request: NextRequest) {
     return NextResponse.json(data);
   } catch (error) {
     console.error("Error fetching logs:", error);
+    // Log detailed error for debugging but return generic message to client
     return NextResponse.json(
-      { error: "Next.js Server Error" + (error as Error).message },
+      { error: "Internal server error occurred while fetching logs" },
       { status: 500 },
     );
   }

frontend/src/app/layout.tsx

@@ -48,6 +48,9 @@ export default function RootLayout({
 }: Readonly<{
   children: React.ReactNode;
 }>) {
+  // Validate auth URL environment variable with fallback
+  const authUrl = process.env.NEXT_PUBLIC_AUTH_URL || "";
+
   return (
     <html lang="en">
       <head>
@@ -99,7 +102,7 @@ export default function RootLayout({
         className={`${geistSans.variable} ${geistMono.variable} antialiased`}
       >
         <QueryClientProvider client={queryClient}>
-          <RequiredAuthProvider authUrl={process.env.NEXT_PUBLIC_AUTH_URL!}>
+          <RequiredAuthProvider authUrl={authUrl}>
             <MetaInfoProvider>
               <SidebarProvider>
                 <AppSidebar />