From 551b23a80c314e6ad70dbc8f806bdc43af594b99 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 8 Feb 2026 08:26:10 +0000 Subject: [PATCH] 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 | 46 +++++++++++--- app/api/dropbox.py | 103 ++++++++---------------------- app/api/google_drive.py | 103 ++++++++---------------------- app/api/onedrive.py | 104 ++++++++----------------------- app/tasks/upload_with_rclone.py | 34 +++++----- app/utils/oauth_helper.py | 107 ++++++++++++++++++++++++++++++++ 6 files changed, 240 insertions(+), 257 deletions(-) create mode 100644 app/utils/oauth_helper.py diff --git a/app/api/common.py b/app/api/common.py index fc1e949f..591bac88 100644 --- a/app/api/common.py +++ b/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 """ + # Build the base directory + if subfolder: + base_dir = Path(settings.workdir) / subfolder + else: + base_dir = Path(settings.workdir) + + # Resolve the file path if not os.path.isabs(file_path): - if subfolder: - file_path = os.path.join(settings.workdir, subfolder, file_path) - else: - file_path = os.path.join(settings.workdir, file_path) - return 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) diff --git a/app/api/dropbox.py b/app/api/dropbox.py index dc31c4c9..b1b2a5f0 100644 --- a/app/api/dropbox.py +++ b/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,84 +32,30 @@ 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" - - payload = { - 'client_id': client_id, - 'client_secret': client_secret, - 'code': code, - 'redirect_uri': redirect_uri, - 'grant_type': 'authorization_code' - } - - # Log request details (excluding secret) - safe_payload = payload.copy() - safe_payload['client_secret'] = '[REDACTED]' - safe_payload['code'] = f"{code[:5]}...{code[-5:]}" if len(code) > 10 else '[REDACTED]' - logger.info(f"Token exchange request payload: {safe_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"], - "access_token": token_data["access_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)}" - ) + # Prepare the token request + token_url = "https://api.dropboxapi.com/oauth2/token" + + payload = { + 'client_id': client_id, + 'client_secret': client_secret, + 'code': code, + 'redirect_uri': redirect_uri, + 'grant_type': 'authorization_code' + } + + # Use shared OAuth helper (handles secure logging and error handling) + token_data = exchange_oauth_token( + provider_name="Dropbox", + token_url=token_url, + payload=payload + ) + + # Return just what's needed by the frontend + return { + "refresh_token": token_data["refresh_token"], + "access_token": token_data["access_token"], + "expires_in": token_data.get("expires_in", 14400) + } @router.post("/dropbox/update-settings") @require_login diff --git a/app/api/google_drive.py b/app/api/google_drive.py index b80a1062..d6626896 100644 --- a/app/api/google_drive.py +++ b/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,84 +32,30 @@ 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" - - payload = { - 'client_id': client_id, - 'client_secret': client_secret, - 'code': code, - 'redirect_uri': redirect_uri, - 'grant_type': 'authorization_code' - } - - # Log request details (excluding secret) - safe_payload = payload.copy() - safe_payload['client_secret'] = '[REDACTED]' - safe_payload['code'] = f"{code[:5]}...{code[-5:]}" if len(code) > 10 else '[REDACTED]' - logger.info(f"Token exchange request payload: {safe_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"], - "access_token": token_data["access_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)}" - ) + # Prepare the token request + token_url = "https://oauth2.googleapis.com/token" + + payload = { + 'client_id': client_id, + 'client_secret': client_secret, + 'code': code, + 'redirect_uri': redirect_uri, + 'grant_type': 'authorization_code' + } + + # Use shared OAuth helper (handles secure logging and error handling) + token_data = exchange_oauth_token( + provider_name="Google Drive", + token_url=token_url, + payload=payload + ) + + # Return just what's needed by the frontend + return { + "refresh_token": token_data["refresh_token"], + "access_token": token_data["access_token"], + "expires_in": token_data.get("expires_in", 3600) + } @router.post("/google-drive/update-settings") @require_login diff --git a/app/api/onedrive.py b/app/api/onedrive.py index a52eda9d..1f160b12 100644 --- a/app/api/onedrive.py +++ b/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,85 +32,30 @@ 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, - 'scope': 'https://graph.microsoft.com/.default offline_access', - 'code': code, - 'redirect_uri': redirect_uri, - 'grant_type': 'authorization_code', - 'client_secret': client_secret - } - - # Log request details (excluding secret) - safe_payload = payload.copy() - safe_payload['client_secret'] = '[REDACTED]' - safe_payload['code'] = f"{code[:5]}...{code[-5:]}" if len(code) > 10 else '[REDACTED]' - logger.info(f"Token exchange request payload: {safe_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)}" - ) + # Prepare the token request + token_url = f"https://login.microsoftonline.com/{tenant_id}/oauth2/v2.0/token" + + payload = { + 'client_id': client_id, + 'scope': 'https://graph.microsoft.com/.default offline_access', + 'code': code, + 'redirect_uri': redirect_uri, + 'grant_type': 'authorization_code', + 'client_secret': client_secret + } + + # Use shared OAuth helper (handles secure logging and error handling) + token_data = exchange_oauth_token( + provider_name="OneDrive", + token_url=token_url, + payload=payload + ) + + # Return just what's needed by the frontend + return { + "refresh_token": token_data["refresh_token"], + "expires_in": token_data.get("expires_in", 3600) + } @router.get("/onedrive/test-token") @require_login diff --git a/app/tasks/upload_with_rclone.py b/app/tasks/upload_with_rclone.py index fc8d9352..7cf2c245 100644 --- a/app/tasks/upload_with_rclone.py +++ b/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 diff --git a/app/utils/oauth_helper.py b/app/utils/oauth_helper.py new file mode 100644 index 00000000..aef6853b --- /dev/null +++ b/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)}" + )