fix(security): reduce code duplication and fix security issues in OAuth and file handling

- Extract common OAuth token exchange logic to shared utility (oauth_helper.py)
- Remove sensitive data logging (client_secret, authorization codes)
- Add path traversal validation in resolve_file_path()
- Add input validation for rclone destination parameter
- Replace bare Exception catches with specific exception types
- Use RuntimeError instead of generic Exception for better error handling

Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com>

app/api/common.py

@@ -3,8 +3,9 @@ Common utilities for API routes
 """
 import logging
 import os
+from pathlib import Path
 from sqlalchemy.orm import Session
-from fastapi import Depends
+from fastapi import Depends, HTTPException, status
 
 from app.database import SessionLocal
 from app.config import settings
@@ -22,15 +23,44 @@ def get_db():
 
 def resolve_file_path(file_path: str, subfolder: str = None) -> str:
     """
-    Resolves a file path to an absolute path.
+    Resolves a file path to an absolute path with path traversal protection.
     If the path is not absolute, it will be joined with the workdir path.
     Optionally, can include a subfolder like 'processed'.
     
-    Returns the absolute file path.
+    Security: Validates that the resolved path stays within the workdir
+    to prevent path traversal attacks (e.g., ../../etc/passwd).
+    
+    Args:
+        file_path: The file path to resolve
+        subfolder: Optional subfolder within workdir
+        
+    Returns:
+        The validated absolute file path
+        
+    Raises:
+        HTTPException: If the path attempts to escape the workdir
     """
-    if not os.path.isabs(file_path):
+    # Build the base directory
     if subfolder:
-            file_path = os.path.join(settings.workdir, subfolder, file_path)
+        base_dir = Path(settings.workdir) / subfolder
     else:
-            file_path = os.path.join(settings.workdir, file_path)
+        base_dir = Path(settings.workdir)
-    return file_path
+    
+    # Resolve the file path
+    if not os.path.isabs(file_path):
+        resolved_path = (base_dir / file_path).resolve()
+    else:
+        resolved_path = Path(file_path).resolve()
+    
+    # Ensure the resolved path is within the base directory (path traversal protection)
+    try:
+        resolved_path.relative_to(base_dir.resolve())
+    except ValueError:
+        # Path is outside the base directory - potential path traversal attack
+        logger.warning(f"Path traversal attempt detected: {file_path} -> {resolved_path}")
+        raise HTTPException(
+            status_code=status.HTTP_400_BAD_REQUEST,
+            detail="Invalid file path: path traversal not allowed"
+        )
+    
+    return str(resolved_path)

app/api/dropbox.py

@@ -11,6 +11,7 @@ from typing import Optional
 
 from app.auth import require_login
 from app.config import settings
+from app.utils.oauth_helper import exchange_oauth_token
 
 # Set up logging
 logger = logging.getLogger(__name__)
@@ -31,9 +32,6 @@ async def exchange_dropbox_token(
     Exchange an authorization code for a refresh token from Dropbox.
     This is done on the server to avoid exposing client secret in the browser.
     """
-    try:
-        logger.info("Starting Dropbox token exchange process")
-        
     # Prepare the token request
     token_url = "https://api.dropboxapi.com/oauth2/token"
     
@@ -45,54 +43,13 @@ async def exchange_dropbox_token(
         'grant_type': 'authorization_code'
     }
     
-        # Log request details (excluding secret)
+    # Use shared OAuth helper (handles secure logging and error handling)
-        safe_payload = payload.copy()
+    token_data = exchange_oauth_token(
-        safe_payload['client_secret'] = '[REDACTED]' 
+        provider_name="Dropbox",
-        safe_payload['code'] = f"{code[:5]}...{code[-5:]}" if len(code) > 10 else '[REDACTED]'
+        token_url=token_url,
-        logger.info(f"Token exchange request payload: {safe_payload}")
+        payload=payload
-        
-        # Make the token request
-        logger.info("Sending POST request to Dropbox for token exchange")
-        response = requests.post(token_url, data=payload, timeout=settings.http_request_timeout)
-        
-        # Check if the request was successful
-        logger.info(f"Token exchange response status: {response.status_code}")
-        
-        if response.status_code != 200:
-            # Log the error response for debugging
-            try:
-                error_json = response.json()
-                logger.error(f"Token exchange failed with status {response.status_code}: {error_json}")
-                error_detail = error_json
-            except Exception as json_err:
-                logger.error(f"Failed to parse error response as JSON: {str(json_err)}")
-                logger.error(f"Raw response content: {response.content[:500]}")  # Limit log size
-                error_detail = {"error": "Unknown error", "raw_content_snippet": str(response.content[:100])}
-            
-            raise HTTPException(
-                status_code=status.HTTP_400_BAD_REQUEST,
-                detail=f"Token exchange failed: {error_detail}"
     )
     
-        # Return the token response
-        token_data = response.json()
-        
-        # Validate the token response
-        if "refresh_token" not in token_data:
-            logger.error(f"Dropbox returned success but no refresh token found in response: {token_data.keys()}")
-            raise HTTPException(
-                status_code=status.HTTP_502_BAD_GATEWAY,
-                detail="Dropbox OAuth server returned success but no refresh token was included"
-            )
-        
-        # Calculate token length for logging
-        refresh_token_length = len(token_data.get("refresh_token", ""))
-        access_token_length = len(token_data.get("access_token", ""))
-        
-        logger.info(f"Successfully exchanged authorization code for Dropbox tokens. "
-                   f"Refresh token length: {refresh_token_length}, "
-                   f"Access token length: {access_token_length}")
-        
     # Return just what's needed by the frontend
     return {
         "refresh_token": token_data["refresh_token"],
@@ -100,16 +57,6 @@ async def exchange_dropbox_token(
         "expires_in": token_data.get("expires_in", 14400)
     }
 
-    except HTTPException:
-        # Re-raise HTTP exceptions as they already have appropriate status codes
-        raise
-    except Exception as e:
-        logger.exception(f"Unexpected error during Dropbox token exchange: {str(e)}")
-        raise HTTPException(
-            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
-            detail=f"Failed to exchange token: {str(e)}"
-        )
-
 @router.post("/dropbox/update-settings")
 @require_login
 async def update_dropbox_settings(

app/api/google_drive.py

@@ -11,6 +11,7 @@ from datetime import datetime, timedelta
 
 from app.auth import require_login
 from app.config import settings
+from app.utils.oauth_helper import exchange_oauth_token
 
 # Set up logging
 logger = logging.getLogger(__name__)
@@ -31,9 +32,6 @@ async def exchange_google_drive_token(
     Exchange an authorization code for refresh and access tokens from Google.
     This is done on the server to avoid exposing client secret in the browser.
     """
-    try:
-        logger.info("Starting Google Drive token exchange process")
-        
     # Prepare the token request
     token_url = "https://oauth2.googleapis.com/token"
     
@@ -45,54 +43,13 @@ async def exchange_google_drive_token(
         'grant_type': 'authorization_code'
     }
     
-        # Log request details (excluding secret)
+    # Use shared OAuth helper (handles secure logging and error handling)
-        safe_payload = payload.copy()
+    token_data = exchange_oauth_token(
-        safe_payload['client_secret'] = '[REDACTED]' 
+        provider_name="Google Drive",
-        safe_payload['code'] = f"{code[:5]}...{code[-5:]}" if len(code) > 10 else '[REDACTED]'
+        token_url=token_url,
-        logger.info(f"Token exchange request payload: {safe_payload}")
+        payload=payload
-        
-        # Make the token request
-        logger.info("Sending POST request to Google for token exchange")
-        response = requests.post(token_url, data=payload, timeout=settings.http_request_timeout)
-        
-        # Check if the request was successful
-        logger.info(f"Token exchange response status: {response.status_code}")
-        
-        if response.status_code != 200:
-            # Log the error response for debugging
-            try:
-                error_json = response.json()
-                logger.error(f"Token exchange failed with status {response.status_code}: {error_json}")
-                error_detail = error_json
-            except Exception as json_err:
-                logger.error(f"Failed to parse error response as JSON: {str(json_err)}")
-                logger.error(f"Raw response content: {response.content[:500]}")  # Limit log size
-                error_detail = {"error": "Unknown error", "raw_content_snippet": str(response.content[:100])}
-            
-            raise HTTPException(
-                status_code=status.HTTP_400_BAD_REQUEST,
-                detail=f"Token exchange failed: {error_detail}"
     )
     
-        # Return the token response
-        token_data = response.json()
-        
-        # Validate the token response
-        if "refresh_token" not in token_data:
-            logger.error(f"Google returned success but no refresh token found in response: {token_data.keys()}")
-            raise HTTPException(
-                status_code=status.HTTP_502_BAD_GATEWAY,
-                detail="Google OAuth server returned success but no refresh token was included"
-            )
-        
-        # Calculate token length for logging
-        refresh_token_length = len(token_data.get("refresh_token", ""))
-        access_token_length = len(token_data.get("access_token", ""))
-        
-        logger.info(f"Successfully exchanged authorization code for Google Drive tokens. "
-                   f"Refresh token length: {refresh_token_length}, "
-                   f"Access token length: {access_token_length}")
-        
     # Return just what's needed by the frontend
     return {
         "refresh_token": token_data["refresh_token"],
@@ -100,16 +57,6 @@ async def exchange_google_drive_token(
         "expires_in": token_data.get("expires_in", 3600)
     }
 
-    except HTTPException:
-        # Re-raise HTTP exceptions as they already have appropriate status codes
-        raise
-    except Exception as e:
-        logger.exception(f"Unexpected error during Google Drive token exchange: {str(e)}")
-        raise HTTPException(
-            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
-            detail=f"Failed to exchange token: {str(e)}"
-        )
-
 @router.post("/google-drive/update-settings")
 @require_login
 async def update_google_drive_settings(

app/api/onedrive.py

@@ -11,6 +11,7 @@ from typing import Optional
 
 from app.auth import require_login
 from app.config import settings
+from app.utils.oauth_helper import exchange_oauth_token
 
 # Set up logging
 logger = logging.getLogger(__name__)
@@ -31,12 +32,8 @@ async def exchange_onedrive_token(
     Exchange an authorization code for a refresh token.
     This is done on the server to avoid exposing client secret in the browser.
     """
-    try:
-        logger.info(f"Starting OneDrive token exchange process with tenant_id: {tenant_id}")
-        
     # Prepare the token request
     token_url = f"https://login.microsoftonline.com/{tenant_id}/oauth2/v2.0/token"
-        logger.info(f"Using token URL: {token_url}")
     
     payload = {
         'client_id': client_id,
@@ -47,70 +44,19 @@ async def exchange_onedrive_token(
         'client_secret': client_secret
     }
     
-        # Log request details (excluding secret)
+    # Use shared OAuth helper (handles secure logging and error handling)
-        safe_payload = payload.copy()
+    token_data = exchange_oauth_token(
-        safe_payload['client_secret'] = '[REDACTED]' 
+        provider_name="OneDrive",
-        safe_payload['code'] = f"{code[:5]}...{code[-5:]}" if len(code) > 10 else '[REDACTED]'
+        token_url=token_url,
-        logger.info(f"Token exchange request payload: {safe_payload}")
+        payload=payload
-        
-        # Make the token request
-        logger.info("Sending POST request to Microsoft for token exchange")
-        response = requests.post(token_url, data=payload, timeout=settings.http_request_timeout)
-        
-        # Check if the request was successful
-        logger.info(f"Token exchange response status: {response.status_code}")
-        
-        if response.status_code != 200:
-            # Log the error response for debugging
-            try:
-                error_json = response.json()
-                logger.error(f"Token exchange failed with status {response.status_code}: {error_json}")
-                error_detail = error_json
-            except Exception as json_err:
-                logger.error(f"Failed to parse error response as JSON: {str(json_err)}")
-                logger.error(f"Raw response content: {response.content[:500]}")  # Limit log size
-                error_detail = {"error": "Unknown error", "raw_content_snippet": str(response.content[:100])}
-            
-            raise HTTPException(
-                status_code=status.HTTP_400_BAD_REQUEST,
-                detail=f"Token exchange failed: {error_detail}"
     )
     
-        # Return the token response
-        token_data = response.json()
-        
-        # Validate the token response
-        if "refresh_token" not in token_data:
-            logger.error(f"Microsoft returned success but no refresh token found in response: {token_data.keys()}")
-            raise HTTPException(
-                status_code=status.HTTP_502_BAD_GATEWAY,
-                detail="Microsoft OAuth server returned success but no refresh token was included"
-            )
-        
-        # Calculate token length for logging
-        refresh_token_length = len(token_data.get("refresh_token", ""))
-        access_token_length = len(token_data.get("access_token", ""))
-        
-        logger.info(f"Successfully exchanged authorization code for OneDrive tokens. "
-                   f"Refresh token length: {refresh_token_length}, "
-                   f"Access token length: {access_token_length}")
-        
     # Return just what's needed by the frontend
     return {
         "refresh_token": token_data["refresh_token"],
         "expires_in": token_data.get("expires_in", 3600)
     }
 
-    except HTTPException:
-        # Re-raise HTTP exceptions as they already have appropriate status codes
-        raise
-    except Exception as e:
-        logger.exception(f"Unexpected error during OneDrive token exchange: {str(e)}")
-        raise HTTPException(
-            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
-            detail=f"Failed to exchange token: {str(e)}"
-        )
-
 @router.get("/onedrive/test-token")
 @require_login
 async def test_onedrive_token(request: Request):

app/tasks/upload_with_rclone.py

@@ -28,6 +28,17 @@ def upload_with_rclone(file_path: str, destination: str):
     # Extract filename
     filename = os.path.basename(file_path)
     
+    # Validate destination format to prevent command injection
+    if ":" not in destination:
+        raise ValueError(f"Invalid destination format: {destination}. Expected format: remote:path")
+    
+    # Split and validate destination components
+    remote, remote_path = destination.split(":", 1)
+    
+    # Validate remote name (alphanumeric, underscore, hyphen only)
+    if not remote or not all(c.isalnum() or c in ('_', '-') for c in remote):
+        raise ValueError(f"Invalid remote name: {remote}")
+    
     # Check if rclone is installed and config exists
     rclone_config_path = os.path.join(settings.workdir, "rclone.conf")
     if not os.path.exists(rclone_config_path):
@@ -36,12 +47,6 @@ def upload_with_rclone(file_path: str, destination: str):
         raise ValueError(error_msg)
     
     try:
-        # Split destination into remote and path
-        if ":" not in destination:
-            raise ValueError(f"Invalid destination format: {destination}. Expected format: remote:path")
-        
-        remote, remote_path = destination.split(":", 1)
-        
         # Ensure the remote path exists (create folders if needed)
         mkdir_cmd = [
             "rclone", 
@@ -77,7 +82,8 @@ def upload_with_rclone(file_path: str, destination: str):
                 ]
                 link_result = subprocess.run(link_cmd, capture_output=True, text=True)
                 public_url = link_result.stdout.strip() if link_result.returncode == 0 else None
-            except Exception:
+            except (subprocess.SubprocessError, OSError) as e:
+                logger.warning(f"Failed to get public link for {filename}: {str(e)}")
                 public_url = None
             
             logger.info(f"Successfully uploaded {filename} to {destination}")
@@ -90,17 +96,17 @@ def upload_with_rclone(file_path: str, destination: str):
         else:
             error_msg = f"Failed to upload {filename} to {destination}: {result.stderr}"
             logger.error(error_msg)
-            raise Exception(error_msg)
+            raise RuntimeError(error_msg)
             
     except subprocess.CalledProcessError as e:
         error_msg = f"Rclone error: {e.stderr.decode('utf-8') if hasattr(e.stderr, 'decode') else e.stderr}"
         logger.error(error_msg)
-        raise Exception(error_msg)
+        raise RuntimeError(error_msg) from e
     
-    except Exception as e:
+    except (OSError, ValueError) as e:
         error_msg = f"Error uploading {filename} to {destination}: {str(e)}"
         logger.error(error_msg)
-        raise Exception(error_msg)
+        raise RuntimeError(error_msg) from e
 
 
 @celery.task(base=BaseTaskWithRetry)
@@ -161,9 +167,9 @@ def send_to_all_rclone_destinations(file_path: str):
         else:
             error_msg = f"Failed to list rclone remotes: {result.stderr}"
             logger.error(error_msg)
-            raise Exception(error_msg)
+            raise RuntimeError(error_msg)
             
-    except Exception as e:
+    except (subprocess.SubprocessError, OSError) as e:
         error_msg = f"Error setting up rclone uploads for {filename}: {str(e)}"
         logger.error(error_msg)
-        raise Exception(error_msg)
+        raise RuntimeError(error_msg) from e

app/utils/oauth_helper.py

@@ -0,0 +1,107 @@
+"""
+OAuth helper utilities for token exchange operations.
+Shared across multiple OAuth providers to reduce code duplication.
+"""
+import logging
+from typing import Dict, Any, Optional
+import requests
+from fastapi import HTTPException, status
+
+from app.config import settings
+
+logger = logging.getLogger(__name__)
+
+
+def exchange_oauth_token(
+    provider_name: str,
+    token_url: str,
+    payload: Dict[str, str],
+    timeout: int = None
+) -> Dict[str, Any]:
+    """
+    Exchange an authorization code for tokens from an OAuth provider.
+    
+    This function handles the common OAuth token exchange flow across multiple providers
+    (OneDrive, Google Drive, Dropbox) with proper error handling and secure logging.
+    
+    Args:
+        provider_name: Name of the OAuth provider (for logging)
+        token_url: OAuth token endpoint URL
+        payload: Request payload containing client credentials and auth code
+        timeout: Request timeout in seconds (defaults to settings.http_request_timeout)
+        
+    Returns:
+        Dict containing the token response from the provider
+        
+    Raises:
+        HTTPException: If token exchange fails or response is invalid
+    """
+    if timeout is None:
+        timeout = settings.http_request_timeout
+    
+    try:
+        logger.info(f"Starting {provider_name} token exchange process")
+        
+        # SECURITY: Never log sensitive data - only log non-sensitive metadata
+        safe_info = {
+            "provider": provider_name,
+            "token_url": token_url,
+            "grant_type": payload.get("grant_type", "unknown"),
+        }
+        logger.info(f"Token exchange request: {safe_info}")
+        
+        # Make the token request
+        logger.info(f"Sending POST request to {provider_name} for token exchange")
+        response = requests.post(token_url, data=payload, timeout=timeout)
+        
+        # Check if the request was successful
+        logger.info(f"Token exchange response status: {response.status_code}")
+        
+        if response.status_code != 200:
+            # Log the error response for debugging (without sensitive data)
+            try:
+                error_json = response.json()
+                # Extract only error type, not full details which may contain sensitive info
+                error_type = error_json.get("error", "unknown_error")
+                logger.error(f"Token exchange failed with status {response.status_code}: {error_type}")
+                error_detail = {"error": error_type, "error_description": error_json.get("error_description", "")}
+            except Exception as json_err:
+                logger.error(f"Failed to parse error response as JSON: {str(json_err)}")
+                error_detail = {"error": "Unknown error", "status_code": response.status_code}
+            
+            raise HTTPException(
+                status_code=status.HTTP_400_BAD_REQUEST,
+                detail=f"Token exchange failed: {error_detail}"
+            )
+            
+        # Parse the token response
+        token_data = response.json()
+        
+        # Validate the token response
+        if "refresh_token" not in token_data:
+            logger.error(f"{provider_name} returned success but no refresh_token found in response")
+            raise HTTPException(
+                status_code=status.HTTP_502_BAD_GATEWAY,
+                detail=f"{provider_name} OAuth server returned success but no refresh token was included"
+            )
+        
+        # Log success with non-sensitive metadata only
+        logger.info(f"Successfully exchanged authorization code for {provider_name} tokens")
+        
+        return token_data
+        
+    except HTTPException:
+        # Re-raise HTTP exceptions as they already have appropriate status codes
+        raise
+    except requests.exceptions.RequestException as e:
+        logger.exception(f"Network error during {provider_name} token exchange: {str(e)}")
+        raise HTTPException(
+            status_code=status.HTTP_503_SERVICE_UNAVAILABLE,
+            detail=f"Failed to connect to {provider_name} OAuth service: {str(e)}"
+        )
+    except Exception as e:
+        logger.exception(f"Unexpected error during {provider_name} token exchange: {str(e)}")
+        raise HTTPException(
+            status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
+            detail=f"Failed to exchange token: {str(e)}"
+        )