security fixes + codegen improvement #47
Conversation
…g, remove production stack traces
…add permission classes
- Create Django management command for cleaning old uploads - Add maintenance API endpoints for remote cleanup triggering - Implement file tracking with immediate cleanup after processing - Add GitHub Actions workflow for scheduled cleanup (free tier workaround) - Include render.yaml for deployment configuration - Add comprehensive setup documentation - Configure 2-hour retention period with automatic cleanup
…nt command) - Remove Django management command (no shell access on free tier) - Move cleanup logic directly into maintenance API endpoint - Simplify documentation to focus on remote triggers only - Update render.yaml with auto-generated cleanup secret - Keep GitHub Actions and external cron service options
This workflow is set to run every 2 hours to clean up uploaded files on Render, using a secret token for authentication.
Updated the schedule for the cleanup workflow from every 2 hours to every 5 hours.
…export and validation views with permission classes
Enhance deploy security + upgrade gen ai package to new sdk
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This pull request implements significant security hardening, authentication improvements, and maintenance automation for the VisionForge application. The changes span backend configuration, authentication flows, file handling, API client library migration, and data validation.
Key Changes:
- Security hardening: Enforces required environment variables in production (SECRET_KEY, CORS origins, CSRF origins), strengthens CSRF/CORS settings, adds CSP directives, and implements client-side API key encryption
- Authentication improvements: Replaces print statements with structured logging, adds CSRF token endpoint, tightens rate limits, and improves error handling
- Maintenance automation: Introduces file cleanup endpoints with token-based authentication and GitHub Actions workflow for automated cleanup
Reviewed changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 25 comments.
Show a summary per file
| File | Description |
|---|---|
| project/requirements.txt | Updates google-generativeai to google-genai v0.1.0 and adds django-csp, python-json-logger |
| project/pyproject.toml | Updates google-genai to v1.56.0 (version mismatch with requirements.txt) |
| project/backend/settings.py | Enforces production env vars, hardens CSRF/CORS, adds CSP, changes default permissions, tightens rate limits, adds upload directory config |
| project/.env.example | Updates with production guidance, required fields, and cleanup token |
| project/frontend/src/lib/encryption.ts | New file: Implements client-side AES-GCM encryption for API keys using browser fingerprinting |
| project/frontend/src/contexts/ApiKeyContext.tsx | Integrates SecureStorage for encrypted API key persistence |
| project/block_manager/validators.py | New file: Adds JSON validators for canvas state, block config, and shape data with size limits |
| project/block_manager/models.py | Adds validators to JSONField declarations for data integrity |
| project/block_manager/views/maintenance_views.py | New file: Implements file cleanup and stats endpoints with secret token auth |
| project/block_manager/views/validation_views.py | Adds AllowAny permission decorator |
| project/block_manager/views/export_views.py | Adds AllowAny decorator, changes rate limit to 5/m, improves error logging |
| project/block_manager/views/chat_views.py | Adds file validation, temporary file tracking, cleanup logic, rate limit reductions |
| project/block_manager/utils/file_cleanup.py | New file: Utilities for temporary file management |
| project/block_manager/services/gemini_service.py | Migrates to new google-genai SDK with updated API patterns |
| project/block_manager/services/tensorflow_codegen.py | Duplicates shape computation utilities from PyTorch codegen |
| project/block_manager/maintenance_urls.py | New file: URL routing for maintenance endpoints |
| project/backend/urls.py | Adds maintenance URL namespace |
| project/authentication/views.py | Adds CSRF endpoint, structured logging, rate limit adjustments, DEBUG-conditional tracebacks |
| project/authentication/urls.py | Adds CSRF token endpoint route |
| project/authentication/middleware.py | Adds CSRF endpoint to exempt paths, improves logging |
| project/authentication/firebase_auth.py | Adds specific exception handling for token errors with logging |
| .gitignore | Adds temp_uploads/ directory |
| .github/workflows/cleanup-uploads.yml | New file: GitHub Actions workflow for automated file cleanup |
| # CSP_STYLE_SRC = ("'self'", "'unsafe-inline'") | ||
| # Content Security Policy | ||
| CSP_DEFAULT_SRC = ("'self'",) | ||
| CSP_SCRIPT_SRC = ("'self'", "'unsafe-inline'", "'unsafe-eval'", "https://www.gstatic.com", "https://apis.google.com") |
There was a problem hiding this comment.
CSP_SCRIPT_SRC includes both 'unsafe-inline' and 'unsafe-eval', which significantly weaken the Content Security Policy and allow XSS attacks. These directives permit inline scripts and eval(), which are common XSS vectors. If these are required for specific libraries (e.g., Firebase), consider using nonces or hashes for inline scripts, or refactor the code to eliminate the need for these unsafe directives. Document which dependencies require these settings.
| CSP_SCRIPT_SRC = ("'self'", "'unsafe-inline'", "'unsafe-eval'", "https://www.gstatic.com", "https://apis.google.com") | |
| # NOTE: 'unsafe-inline' is currently retained for existing inline scripts in the frontend. | |
| # No known dependency requires eval(), so 'unsafe-eval' is intentionally omitted to reduce XSS risk. | |
| CSP_SCRIPT_SRC = ("'self'", "'unsafe-inline'", "https://www.gstatic.com", "https://apis.google.com") |
| self.model = genai.GenerativeModel('gemini-2.0-flash-lite') | ||
| # Create client with API key (new unified SDK) | ||
| self.client = genai.Client(api_key=final_api_key) | ||
| # Use gemini-2.0-flash-exp - experimental 2.0 model (or use gemini-1.5-flash for stable) |
There was a problem hiding this comment.
The comment mentions 'gemini-2.0-flash-exp - experimental 2.0 model' but the code actually uses 'gemini-2.5-flash-lite'. This discrepancy between comment and code is misleading. Update the comment to accurately reflect the model being used, or verify that the model name is correct.
| # Use gemini-2.0-flash-exp - experimental 2.0 model (or use gemini-1.5-flash for stable) | |
| # Default to gemini-2.5-flash-lite (lightweight Gemini 2.5 Flash model; consider gemini-1.5-flash for stable) |
| SECRET_KEY = os.getenv('DJANGO_SECRET_KEY') | ||
| if not SECRET_KEY: | ||
| raise ValueError("DJANGO_SECRET_KEY environment variable must be set") |
There was a problem hiding this comment.
The SECRET_KEY environment variable is now required but will cause the Django application to fail to start during settings import if not set. This breaks local development setup where developers may not have immediately configured their .env file. Consider allowing a development default when DEBUG=True while enforcing the requirement only in production (when DEBUG=False), similar to how CORS_ALLOWED_ORIGINS is handled.
| curl -X POST \ | ||
| -H "Content-Type: application/json" \ | ||
| "${{ secrets.CLEANUP_ENDPOINT_URL }}" \ | ||
| -d '{"secret": "${{ secrets.CLEANUP_SECRET }}"}' |
There was a problem hiding this comment.
The GitHub Actions workflow secret CLEANUP_ENDPOINT_URL is used directly without validation. If this URL is misconfigured or points to an unintended endpoint, the cleanup secret could be exposed to a third party. Add validation in the workflow or documentation to ensure the URL must be an HTTPS endpoint and should match expected domain patterns. Consider using environment-specific secrets or adding a verification step.
| curl -X POST \ | |
| -H "Content-Type: application/json" \ | |
| "${{ secrets.CLEANUP_ENDPOINT_URL }}" \ | |
| -d '{"secret": "${{ secrets.CLEANUP_SECRET }}"}' | |
| set -euo pipefail | |
| CLEANUP_URL="${{ secrets.CLEANUP_ENDPOINT_URL }}" | |
| if [[ -z "$CLEANUP_URL" ]]; then | |
| echo "CLEANUP_ENDPOINT_URL is not set. Aborting cleanup job." >&2 | |
| exit 1 | |
| fi | |
| # Require HTTPS to avoid sending secrets over insecure transport | |
| if [[ "$CLEANUP_URL" != https://* ]]; then | |
| echo "CLEANUP_ENDPOINT_URL must use HTTPS. Aborting cleanup job." >&2 | |
| exit 1 | |
| fi | |
| # Ensure URL points to an approved Render domain to avoid exfiltrating secrets | |
| if [[ "$CLEANUP_URL" != https://*.onrender.com/* && "$CLEANUP_URL" != https://*.render.com/* ]]; then | |
| echo "CLEANUP_ENDPOINT_URL must point to an approved Render domain. Aborting cleanup job." >&2 | |
| exit 1 | |
| fi | |
| curl -X POST \ | |
| -H "Content-Type: application/json" \ | |
| "$CLEANUP_URL" \ | |
| -d '{"secret": "'"${{ secrets.CLEANUP_SECRET }}"'"}' |
| # Max size check | ||
| json_str = json.dumps(value) | ||
| if len(json_str) > 1000: # 1KB limit for shape data |
There was a problem hiding this comment.
The shape data size limit of 1KB (line 75) is extremely restrictive and could prevent legitimate use cases. Tensor shapes with detailed metadata (stride information, device placement, dtype, etc.) or batch processing information could easily exceed 1KB. Unless there's a specific DoS threat model, consider increasing this limit to at least 5-10KB, or remove it if Django's overall request size limits provide adequate protection.
| # Max size check | |
| json_str = json.dumps(value) | |
| if len(json_str) > 1000: # 1KB limit for shape data | |
| # Max size check to prevent DoS while allowing realistic shape metadata | |
| json_str = json.dumps(value) | |
| if len(json_str) > 10000: # 10KB limit for shape data |
| expected_secret = settings.CLEANUP_SECRET_TOKEN | ||
| if not expected_secret or provided_secret != expected_secret: |
There was a problem hiding this comment.
The timing-safe comparison for the secret token is not used. The line 'provided_secret != expected_secret' is vulnerable to timing attacks that could leak information about the secret token. Use Django's constant_time_compare function to prevent timing-based side-channel attacks. Import from django.utils.crypto and replace the comparison with: not constant_time_compare(provided_secret, expected_secret or '').
| timestamp = int(time.time()) | ||
| safe_filename = f"{timestamp}_{uploaded_file.name}" | ||
| file_path = upload_dir / safe_filename |
There was a problem hiding this comment.
Potential path traversal vulnerability: The upload directory path concatenation uses file_path.name without sanitizing the filename. Although save_uploaded_file_temporarily adds a timestamp prefix, if an attacker can control the uploaded_file.name (e.g., '../../../etc/passwd'), they might escape the intended directory. Ensure uploaded filenames are sanitized before use, using secure_filename or similar utilities, or use only the timestamp without incorporating the original filename.
| */ | ||
|
|
||
| const ALGORITHM = 'AES-GCM'; | ||
| const KEY_LENGTH = 256; |
There was a problem hiding this comment.
Unused variable KEY_LENGTH.
| const KEY_LENGTH = 256; |
| """ | ||
| Utilities for managing temporary file uploads. | ||
| """ | ||
| import os |
There was a problem hiding this comment.
Import of 'os' is not used.
| import os |
| from .enhanced_pytorch_codegen import ( | ||
| GroupDefinitionNotFoundError, | ||
| ShapeMismatchError, | ||
| CyclicDependencyError, | ||
| UnsupportedNodeTypeError, | ||
| ShapeInferenceError, | ||
| MissingShapeDataError, | ||
| safe_get_shape_data | ||
| MissingShapeDataError | ||
| ) |
There was a problem hiding this comment.
Import of 'ShapeMismatchError' is not used.
Import of 'CyclicDependencyError' is not used.
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Co-authored-by: RETR0-OS <74290459+RETR0-OS@users.noreply.github.com>
Co-authored-by: RETR0-OS <74290459+RETR0-OS@users.noreply.github.com>
Co-authored-by: RETR0-OS <74290459+RETR0-OS@users.noreply.github.com>
Co-authored-by: RETR0-OS <74290459+RETR0-OS@users.noreply.github.com>
Co-authored-by: RETR0-OS <74290459+RETR0-OS@users.noreply.github.com>
Fix path traversal, XSS, timing attack, and information disclosure vulnerabilities
…xception Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
This pull request introduces several important improvements focused on production security, authentication robustness, and file maintenance automation. Key changes include enforcing stricter environment variable requirements for production, enhancing CSRF and CORS configuration, improving logging and error handling in authentication flows, tightening API rate limits, and adding automated file cleanup infrastructure.
Production Security & Environment Configuration:
DJANGO_SECRET_KEY,CORS_ALLOWED_ORIGINS, andCSRF_TRUSTED_ORIGINSmust be set in production, raising errors if missing. Updated.env.examplewith clearer production guidance and required fields. (Fc2c84f4L1R1, [1] [2] [3] [4]CSRF_COOKIE_HTTPONLY,CSRF_COOKIE_SAMESITE, and explicit header naming.Authentication & Logging Improvements:
/api/auth/csrfendpoint to set CSRF cookies for frontend clients, and included/api/auth/csrfas an authentication-exempt path. [1] [2] [3]File Upload & Maintenance Automation:
cleanup-uploads.yml) to periodically trigger remote file cleanup via a secure endpoint.Content Security Policy (CSP):
Model Validation:
canvas_statefield in theModelArchitecturemodel, improving data integrity. [1] [2]