LIComputerGuy/media-downloader
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
main
media-downloader/docs/archive/CODE_REVIEW_2025-11-09.md
Todd 0d7b2b1aab Initial commit 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2026-03-29 22:42:55 -04:00

20 KiB
Raw Permalink Blame History

Media Downloader - Comprehensive Code Review

Executive Summary

The Media Downloader application is a sophisticated multi-platform media download system with ~30,775 lines of Python and TypeScript code. It integrates Instagram, TikTok, Forums, Snapchat, and other platforms with a web-based management interface. Overall architecture is well-designed with proper separation of concerns, but there are several security, performance, and code quality issues that need attention.

Overall Assessment: B+ (Good with room for improvement in specific areas)

1. ARCHITECTURE & DESIGN PATTERNS

Strengths

  1. Unified Database Architecture (/opt/media-downloader/modules/unified_database.py)

    • Excellent consolidation of multiple platform databases into single unified DB
    • Connection pooling implemented correctly (lines 21-92)
    • Proper use of context managers for resource management
    • Well-designed adapter pattern for platform-specific compatibility (lines 1707-2080)

  2. Module Organization

    • Clean separation: downloaders, database, UI, utilities
    • Each platform has dedicated module (fastdl, tiktok, instagram, snapchat, etc.)
    • Settings manager provides centralized configuration

  3. Authentication Layer

    • Proper use of JWT tokens with bcrypt password hashing
    • Rate limiting on login attempts (5 attempts, 15-min lockout)
    • Support for 2FA (TOTP, Passkeys, Duo)

Issues

  1. Tight Coupling in Main Application

    • Location: /opt/media-downloader/media-downloader.py (lines 1-100)
    • Issue: Core class imports 20+ modules directly, making it tightly coupled
    • Impact: Hard to test individual components; difficult to extend
    • Recommendation: Create dependency injection container or factory pattern

  2. Incomplete Separation of Concerns

    • Location: /opt/media-downloader/modules/fastdl_module.py (lines 35-70)
    • Issue: Browser automation logic mixed with download logic
    • Recommendation: Extract Playwright interactions into separate browser manager class

  3. Missing Interface Definitions

    • No clear contracts between modules
    • Recommendation: Add type hints and Protocol classes for module boundaries

2. SECURITY ISSUES

Critical Issues

  1. Token Exposure in URLs

    • Location: /opt/media-downloader/web/frontend/src/lib/api.ts (lines 558-568)
    • Issue: Authentication tokens passed as query parameters for media preview/thumbnails
    getMediaThumbnailUrl(filePath: string, mediaType: 'image' | 'video') {
    const token = localStorage.getItem('auth_token')
    const tokenParam = token ? `&token=${encodeURIComponent(token)}` : ''
    return `${API_BASE}/media/thumbnail?file_path=${encodeURIComponent(filePath)}&media_type=${mediaType}${tokenParam}`
}
    • Risk: Tokens visible in browser history, server logs, referrer headers
    • Fix: Use Authorization header instead; implement server-side session validation for media endpoints

  2. Weak File Path Validation

    • Location: /opt/media-downloader/web/backend/api.py (likely in file handling endpoints)
    • Issue: File paths received from frontend may not be properly validated
    • Risk: Path traversal attacks (../ sequences)
    • Fix: 
      from pathlib import Path
def validate_file_path(file_path: str, allowed_base: Path) -> Path:
    real_path = Path(file_path).resolve()
    if not str(real_path).startswith(str(allowed_base)):
        raise ValueError("Path traversal detected")
    return real_path

  3. Missing CSRF Protection

    • Location: /opt/media-downloader/web/backend/api.py (lines 318-320)
    • Issue: SessionMiddleware added but no CSRF tokens implemented
    • Impact: POST/PUT/DELETE requests vulnerable to CSRF
    • Fix: Add CSRF middleware (starlette-csrf)

High Priority Issues

  1. Subprocess Usage Without Validation

    • Location: /opt/media-downloader/modules/tiktok_module.py (lines 294, 422, 440)
    • Issue: Uses subprocess.run() for yt-dlp commands
    result = subprocess.run(cmd, capture_output=True, text=True, cwd=output_dir)
    • Risk: If username or other params are unsanitized, could lead to command injection
    • Fix: Use list form of subprocess.run (which is safer) and validate all inputs

  2. SQL Injection Protection Issues

    • Location: /opt/media-downloader/modules/unified_database.py (lines 576-577)
    • Issue: Uses LIKE patterns with string formatting:
    pattern1 = f'%"media_id": "{media_id}"%'  # Potential SQL injection if media_id not sanitized
    • Current State: Properly uses parameterized queries, but patterns could be safer
    • Recommendation: Add explicit input validation before using in LIKE patterns

  3. Credentials in Environment & Files

    • Location: /opt/media-downloader/.jwt_secret, /opt/media-downloader/.env
    • Issue: Sensitive files with improper permissions
    • Fix:
      • Ensure .jwt_secret is mode 0600 (already done in auth_manager.py line 38)
      • .env should not be committed to git
      • Consider using vault/secrets manager for production

  4. No Input Validation on Config Updates

    • Location: /opt/media-downloader/web/backend/api.py (lines 349-351)
    • Issue: Config updates from frontend lack validation
    • Impact: Could set invalid/malicious values
    • Fix: Add Pydantic validators for all config fields

  5. Missing Rate Limiting on API Endpoints

    • Location: /opt/media-downloader/web/backend/api.py (lines 322-325)
    • Issue: Rate limiter configured but not applied to routes
    • Fix: Add @limiter.limit() decorators on endpoints, especially:
      • Media downloads
      • Configuration updates
      • Scheduler triggers

Medium Priority Issues

  1. Insufficient Error Message Sanitization

    • Location: Various modules show detailed error messages in logs
    • Risk: Error messages may expose internal paths/configuration
    • Fix: Return generic messages to clients, detailed logs server-side only

  2. Missing Security Headers

    • Location: /opt/media-downloader/web/backend/api.py (app creation)
    • Missing: Content-Security-Policy, X-Frame-Options, X-Content-Type-Options
    • Fix: Add security headers middleware

3. PERFORMANCE OPTIMIZATIONS

Database Performance

  1. Connection Pool Configuration ✓ GOOD

    • /opt/media-downloader/modules/unified_database.py (lines 21-45)
    • Pool size of 5 (default), configurable to 20 for API
    • WAL mode enabled for better concurrency
    • Good index strategy (lines 338-377)

  2. Query Optimization Issues

    a) N+1 Problem in Face Recognition

    • Location: /opt/media-downloader/modules/face_recognition_module.py
    • Issue: Likely fetches file list, then queries metadata for each file
    • Recommendation: Join operations or batch queries

    b) Missing Indexes

    • Location: /opt/media-downloader/modules/unified_database.py (lines 338-377)
    • Current Indexes: ✓ Platform, source, status, dates (good)
    • Missing:
      • Composite index on (file_hash, platform) for deduplication checks
      • Index on metadata field (though JSON search is problematic)

    c) JSON Metadata Searches

    • Location: /opt/media-downloader/modules/unified_database.py (lines 576-590)
    • Issue: Uses LIKE on JSON metadata field - very inefficient
    cursor.execute('''SELECT ... WHERE metadata LIKE ? OR metadata LIKE ?''', 
               (f'%"media_id": "{media_id}"%', f'%"media_id"%{media_id}%'))
    • Impact: Full table scans on large datasets
    • Fix: Use JSON_EXTRACT() for JSON queries (if database supports) or extract media_id to separate column

  3. File I/O Bottlenecks

    a) Hash Calculation on Every Download

    • Location: /opt/media-downloader/modules/unified_database.py (lines 437-461)
    • Issue: SHA256 hash computed for every file download
    • Fix: Cache hashes, compute asynchronously, or skip for non-deduplicated files

    b) Synchronous File Operations in Async Context

    • Location: /opt/media-downloader/web/backend/api.py (likely file operations)
    • Issue: Could block event loop
    • Fix: Use aiofiles or asyncio.to_thread() for file I/O

  4. Image Processing Performance

    • Location: /opt/media-downloader/modules/face_recognition_module.py
    • Issue: Face recognition runs on main thread, blocks other operations
    • Current: Semaphore limits to 1 concurrent (good)
    • Suggestion: Make async, use process pool for CPU-bound face detection

  5. Caching Opportunities

    • Missing: Result caching for frequently accessed data
    • Recommendation: Add Redis/in-memory caching for:
      • Platform stats (cache 5 minutes)
      • Download filters (cache 15 minutes)
      • System health (cache 1 minute)

Frontend Performance

  1. No Pagination Implementation Found

    • Location: /opt/media-downloader/web/frontend/src/lib/api.ts (lines 225-289)
    • Issue: API supports pagination but unclear if UI implements infinite scroll
    • Recommendation: Implement virtual scrolling for large media galleries

  2. Unoptimized Asset Loading

    • Location: Built assets in /opt/media-downloader/web/backend/static/assets/
    • Issue: Multiple .js chunks loaded (index-*.js variations suggest no optimization)
    • Recommendation: Check Vite build config for code splitting optimization

4. CODE QUALITY

Code Duplication

  1. Adapter Pattern Duplication

    • Location: /opt/media-downloader/modules/unified_database.py (lines 1708-2080)
    • Issue: Multiple adapter classes (FastDLDatabaseAdapter, TikTokDatabaseAdapter, etc.) with similar structure
    • Lines Affected: ~372 lines of repetitive code
    • Fix: Create generic adapter base class with template method pattern

  2. Download Manager Pattern Repeated

    • Location: Each platform module has similar download logic
    • Recommendation: Extract to common base class

  3. Cookie/Session Management Duplicated

    • Location: fastdl_module, imginn_module, toolzu_module, snapchat_module
    • Recommendation: Create shared CookieManager utility

Error Handling

  1. Bare Exception Handlers

    • Locations:
      • /opt/media-downloader/modules/fastdl_module.py (line 100+)
      • /opt/media-downloader/media-downloader.py (lines 2084-2085)
    except:  # Too broad!
    break
    • Risk: Suppresses unexpected errors
    • Fix: Catch specific exceptions

  2. Missing Error Recovery

    • Location: /opt/media-downloader/modules/forum_downloader.py (lines 83+)
    • Issue: ForumDownloader has minimal retry logic
    • Recommendation: Add exponential backoff with jitter

  3. Logging Inconsistency

    • Location: Throughout codebase
    • Issue: Mix of logger.info(), print(), and log() callbacks
    • Fix: Standardize on logger module everywhere

Complexity Issues

  1. Long Functions

    • Location: /opt/media-downloader/media-downloader.py
    • Issue: Main class likely has 200+ line methods
    • Recommendation: Break into smaller, testable methods

  2. Complex Conditional Logic

    • Location: 2FA implementation in auth_manager.py
    • Issue: Multiple nested if/elif chains for 2FA method selection
    • Fix: Strategy pattern with 2FA providers

Missing Type Hints

  1. Inconsistent Type Coverage
    • Status: Backend has some type hints, but inconsistent
    • Examples:
      • /opt/media-downloader/modules/download_manager.py: ✓ Good type hints
      • /opt/media-downloader/modules/fastdl_module.py: ✗ Minimal type hints
    • Recommendation: Use mypy --strict on entire codebase

5. FEATURE OPPORTUNITIES

User Experience

  1. Download Scheduling Enhancements

    • Current: Basic interval-based scheduling
    • Suggestion: Add cron expression support
    • Effort: Medium

  2. Batch Operations

    • Current: Single file operations
    • Suggestion: Queue system for batch config changes
    • Effort: Medium

  3. Search & Filters

    • Current: Basic platform/source filters
    • Suggestions:
      • Date range picker UI
      • File size filters
      • Content type hierarchy
    • Effort: Low

  4. Advanced Metadata Editing

    • Current: Read-only metadata display
    • Suggestion: Edit post dates, tags, descriptions
    • Effort: Medium

  5. Duplicate Detection Improvements

    • Current: File hash based
    • Suggestion: Perceptual hashing for images (detect same photo at different resolutions)
    • Effort: High

Integration Features

  1. Webhook Support

    • Use Case: Trigger downloads from external services
    • Effort: Medium

  2. API Key Authentication

    • Current: JWT only
    • Suggestion: Support API keys for programmatic access
    • Effort: Low

  3. Export/Import Functionality

    • Suggestion: Export download history, settings to JSON/CSV
    • Effort: Low

Platform Support

  1. Additional Platforms
    • Missing: LinkedIn, Pinterest, X/Twitter, Reddit
    • Effort: High per platform

6. BUG RISKS

Race Conditions

  1. Database Write Conflicts

    • Location: /opt/media-downloader/modules/unified_database.py (lines 728-793)
    • Issue: Multiple processes writing simultaneously could hit database locks
    • Current Mitigation: WAL mode, write locks, retries (good!)
    • Enhancement: Add distributed lock if scaling to multiple servers

  2. Face Recognition Concurrent Access

    • Location: /opt/media-downloader/web/backend/api.py (line 225)
    • Issue: Face recognition limited to 1 concurrent via semaphore
    • Status: ✓ Protected
    • Note: But blocking may cause timeouts if many requests queue

  3. Cookie/Session File Access

    • Location: /opt/media-downloader/modules/fastdl_module.py (line 77)
    • Issue: Multiple downloader instances reading/writing cookies.json simultaneously
    • Risk: File corruption or lost updates
    • Fix: Add file locking

Memory Leaks

  1. Unclosed File Handles

    • Location: /opt/media-downloader/modules/download_manager.py (streams)
    • Review: Check all file operations use context managers
    • Status: Need to verify

  2. WebSocket Connection Leaks

    • Location: /opt/media-downloader/web/backend/api.py (lines 334-348)
    • Issue: ConnectionManager stores WebSocket refs
    • Risk: Disconnected clients not properly cleaned up
    • Fix: Add timeout/heartbeat for stale connections

  3. Large Image Processing

    • Location: Image thumbnail generation
    • Risk: In-memory image processing could OOM with large files
    • Recommendation: Stream processing or size limits

Data Integrity

  1. Incomplete Download Tracking

    • Location: /opt/media-downloader/modules/download_manager.py (DownloadResult)
    • Issue: If database insert fails after successful download, file orphaned
    • Fix: Transactional approach - record first, then download

  2. Timestamp Modification

    • Location: /opt/media-downloader/media-downloader.py (lines 2033-2035)
    • Issue: Using os.utime() may fail silently
    os.utime(dest_file, (ts, ts))  # No error handling
    • Fix: Check return value and log failures

  3. Partial Recycle Bin Operations

    • Location: /opt/media-downloader/modules/unified_database.py (lines 1472-1533)
    • Issue: If file move fails but DB updates success, inconsistent state
    • Fix: Rollback DB changes if file move fails

7. SPECIFIC CODE ISSUES

Path Handling

  1. Hardcoded Paths

    • Location:
      • /opt/media-downloader/modules/unified_database.py line 1432: /opt/immich/recycle
      • Various modules hardcode /opt/media-downloader
    • Issue: Not portable, breaks if deployed elsewhere
    • Fix: Use environment variables with fallbacks

  2. Path Validation Missing

    • Location: Media file serving endpoints
    • Issue: No symlink attack prevention
    • Fix: Use Path.resolve() and verify within allowed directory

Settings Management

  1. Settings Validation
    • Location: /opt/media-downloader/modules/settings_manager.py
    • Issue: No schema validation for settings
    • Recommendation: Use Pydantic models for all settings

API Design

  1. Inconsistent Response Formats

    • Issue: Some endpoints return {success, data}, others just data
    • Recommendation: Standardize on single response envelope

  2. Missing API Documentation

    • Suggestion: Add OpenAPI/Swagger documentation
    • Benefit: Self-documenting API, auto-generated client SDKs

RECOMMENDATIONS PRIORITY LIST

IMMEDIATE (Week 1)

  1. Remove tokens from URL queries - Use Authorization header only
  2. Add CSRF protection - Use starlette-csrf
  3. Fix bare except clauses - Catch specific exceptions
  4. Add file path validation - Prevent directory traversal
  5. Add security headers - CSP, X-Frame-Options, etc.

SHORT TERM (Week 2-4)

  1. Implement rate limiting on routes - Protect all write operations
  2. Fix JSON search performance - Use proper JSON queries or separate columns
  3. Add input validation on config - Validate all settings updates
  4. Extract adapter duplications - Create generic base adapter
  5. Standardize logging - Remove print(), use logger everywhere
  6. Add type hints - Run mypy on entire codebase

MEDIUM TERM (Month 2)

  1. Implement caching layer - Redis/in-memory for hot data
  2. Add async file I/O - Use aiofiles for media operations
  3. Extract browser logic - Separate Playwright concerns
  4. Add WebSocket heartbeat - Prevent connection leaks
  5. Implement distributed locking - If scaling to multiple instances

LONG TERM (Month 3+)

  1. Add perceptual hashing - Better duplicate detection
  2. Implement API key auth - Support programmatic access
  3. Add webhook support - External service integration
  4. Refactor main class - Implement dependency injection

TESTING RECOMMENDATIONS

Current State

  • Test directory exists (/opt/media-downloader/tests/) with 10 test files
  • Status: Need to verify test coverage

Recommendations

  1. Add unit tests for core database operations
  2. Add integration tests for download pipeline
  3. Add security tests (SQL injection, path traversal, CSRF)
  4. Add load tests for concurrent downloads
  5. Add UI tests for critical flows (login, config, downloads)

DEPLOYMENT RECOMMENDATIONS

  1. Environment Configuration

    • Move all hardcoded paths to environment variables
    • Document all required env vars
    • Use .env.example template

  2. Database

    • Regular backups of media_downloader.db
    • Monitor database file size
    • Implement retention policies for old records

  3. Security

    • Use strong JWT secret (already implemented, good)
    • Enable HTTPS only in production
    • Implement rate limiting on all API endpoints
    • Regular security audits

  4. Monitoring

    • Add health check endpoint monitoring
    • Set up alerts for database locks
    • Monitor disk space for media/recycle bin
    • Log critical errors to centralized system

  5. Scaling

    • Current design assumes single instance
    • For multi-instance: implement distributed locking, session sharing
    • Consider message queue for download jobs (Redis/RabbitMQ)

CONCLUSION

The Media Downloader application is well-architected with good separation of concerns, proper database design, and thoughtful authentication implementation. The main areas for improvement are:

  1. Security: Primarily around token handling, path validation, and CSRF protection
  2. Performance: Database query optimization, especially JSON searches and file I/O
  3. Code Quality: Reducing duplication, standardizing error handling and logging
  4. Testing: Expanding test coverage, especially for security-critical paths

With the recommended fixes prioritized by the provided list, the application can achieve production-grade quality suitable for enterprise deployment.

Overall Code Grade: B+ (Good with specific improvements needed)

Reference in New Issue View Git Blame Copy Permalink