website: Allowed CSRF Update #4029
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## staging #4029 +/- ##
========================================
Coverage 11.73% 11.73%
========================================
Files 114 114
Lines 15331 15331
Branches 319 319
========================================
Hits 1799 1799
Misses 13532 13532 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
src/website/core/settings.py (1)
Security configuration needs adjustment for production environment
The codebase analysis reveals several security concerns that need to be addressed:
DEBUG = Trueis hardcoded in settings.py, which makesCSRF_COOKIE_SECURE = not DEBUGandSESSION_COOKIE_SECURE = not DEBUGalways set to FalseCORS_ORIGIN_ALLOW_ALL = Trueis enabled while relying onCSRF_TRUSTED_ORIGINSfor protection- The middleware order is correct (CORS before CSRF), but the permissive CORS configuration combined with insecure cookies in development mode creates a security risk
To maintain security:
DEBUGshould be configured via environment variable instead of being hardcoded- Consider restricting CORS using
CORS_ALLOWED_ORIGINSinstead of allowing all origins- Ensure secure cookie settings in production by properly configuring DEBUG
🔗 Analysis chain
Line range hint
123-124: Verify CSRF protection with CORS changesThe combination of unrestricted CORS and these CSRF settings needs careful consideration:
CSRF_COOKIE_SECURE = not DEBUG SESSION_COOKIE_SECURE = not DEBUGWith
DEBUG = True, these cookies won't be secure, potentially exposing the application to CSRF attacks.Let's check for CSRF token usage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for CSRF token handling in frontend code rg "csrf|xsrf" --type-add 'vue:*.vue' -tvue -tjs -ttsLength of output: 55
Script:
#!/bin/bash # Let's do a broader search for CSRF-related code and configurations rg "csrf|xsrf" -i --type=python -A 3 # Also check for CORS settings as they're related to the security concern rg "CORS" -i --type=python -A 3 # Check Django middleware configuration rg "MIDDLEWARE.*=.*\[" -A 10 --type=python # Look for DEBUG setting rg "DEBUG.*=.*" --type=pythonLength of output: 271
Script:
#!/bin/bash # Let's try without file type specification for broader search # Search for CSRF-related configurations rg "csrf|xsrf" -i # Look for CORS settings rg "CORS" -i # Check middleware configurations rg "MIDDLEWARE.*=.*\[" -A 10 # Check DEBUG settings rg "DEBUG.*=.*" # Also check for Django security middleware rg "django.middleware.security" -A 3Length of output: 12281
| @@ -14,3 +14,4 @@ python manage.py collectstatic --noinput | |||
| # Start Gunicorn server to serve the Django application | |||
| echo "Starting Gunicorn server..." | |||
| exec gunicorn core.wsgi:application --bind 0.0.0.0:8000 --timeout 600 --log-level info | |||
| # exec gunicorn core.wsgi:application --bind 0.0.0.0:8000 --timeout 600 --workers ${GUNICORN_WORKERS:-3} --log-level info | |||
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Consider using the dynamic worker configuration
The commented line with ${GUNICORN_WORKERS:-3} represents a better configuration approach as it:
- Provides flexibility to adjust worker count through environment variables
- Maintains a sensible default of 3 workers if the variable isn't set
- Follows container best practices by allowing runtime configuration
-exec gunicorn core.wsgi:application --bind 0.0.0.0:8000 --timeout 600 --log-level info
+exec gunicorn core.wsgi:application --bind 0.0.0.0:8000 --timeout 600 --workers ${GUNICORN_WORKERS:-3} --log-level info🔗 Analysis chain
Clarify Gunicorn worker configuration intention
There appears to be ambiguity between the active and commented configurations for Gunicorn workers. The commented line offers dynamic scaling via ${GUNICORN_WORKERS:-3}, while the active line uses a fixed configuration.
Let's check if there are any environment variables set for workers in the repository:
Consider using the dynamic configuration to allow better scaling:
-exec gunicorn core.wsgi:application --bind 0.0.0.0:8000 --timeout 600 --log-level info
+exec gunicorn core.wsgi:application --bind 0.0.0.0:8000 --timeout 600 --workers ${GUNICORN_WORKERS:-3} --log-level infoThe file shows only one reference to GUNICORN_WORKERS, but we need to see the full context of the entrypoint.sh file to understand if this is a deliberate configuration choice or an oversight. This will help us determine if the commented line is meant to be a reference or if it's an actual alternative configuration that should be used.
Before making a final decision, we should check if there are any Docker or environment configuration files that might be setting the GUNICORN_WORKERS variable. This will help us understand if the commented configuration is actually being used elsewhere in the deployment setup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GUNICORN_WORKERS environment variable references
rg "GUNICORN_WORKERS" -l
Length of output: 50
Script:
#!/bin/bash
# Let's check the full content of entrypoint.sh to understand the context better
cat src/website/entrypoint.sh
Length of output: 642
Script:
#!/bin/bash
# Let's check if there are any Docker or environment configuration files that might set GUNICORN_WORKERS
fd -t f "docker-compose|\.env|Dockerfile" -X cat
Length of output: 23089
| # ALLOWED_HOSTS = parse_env_list("ALLOWED_HOSTS") | ||
| ALLOWED_HOSTS = ['*'] |
There was a problem hiding this comment.
Security Risk: Unrestricted ALLOWED_HOSTS
Using ['*'] for ALLOWED_HOSTS bypasses Django's host header validation, potentially enabling HTTP Host header attacks.
Restore the environment-based configuration:
-# ALLOWED_HOSTS = parse_env_list("ALLOWED_HOSTS")
-ALLOWED_HOSTS = ['*']
+ALLOWED_HOSTS = parse_env_list("ALLOWED_HOSTS")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # ALLOWED_HOSTS = parse_env_list("ALLOWED_HOSTS") | |
| ALLOWED_HOSTS = ['*'] | |
| ALLOWED_HOSTS = parse_env_list("ALLOWED_HOSTS") |
src/website/core/settings.py
Outdated
| # DEBUG = get_env_bool('DEBUG', default=False) | ||
| DEBUG = True |
There was a problem hiding this comment.
Critical: DEBUG must be False in production
Setting DEBUG = True in production is a severe security risk that could:
- Expose sensitive configuration details
- Display detailed error messages to end users
- Potentially leak source code
Restore the environment-based configuration:
-# DEBUG = get_env_bool('DEBUG', default=False)
-DEBUG = True
+DEBUG = get_env_bool('DEBUG', default=False)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # DEBUG = get_env_bool('DEBUG', default=False) | |
| DEBUG = True | |
| DEBUG = get_env_bool('DEBUG', default=False) |
src/website/core/settings.py
Outdated
| # CORS_ORIGIN_ALLOW_ALL = False | ||
| CORS_ORIGIN_ALLOW_ALL = True |
There was a problem hiding this comment.
💡 Codebase verification
Security Configuration Needs Attention
The codebase shows multiple concerning security configurations:
CORS_ORIGIN_ALLOW_ALL = Trueoverrides the more secure origin-specific settingsALLOWED_HOSTS = ['*']is set to allow all hostsDEBUG = Trueis enabled- Environment-based security configurations are present but effectively bypassed
These settings suggest a development configuration that shouldn't be used in production. While CORS_ALLOWED_ORIGINS, CORS_ORIGIN_REGEX_WHITELIST, and CSRF_TRUSTED_ORIGINS are properly configured to use environment variables, they're rendered ineffective by the CORS_ORIGIN_ALLOW_ALL = True setting.
Please update the configuration to:
DEBUG = get_env_bool('DEBUG', default=False)
ALLOWED_HOSTS = parse_env_list("ALLOWED_HOSTS")
CORS_ORIGIN_ALLOW_ALL = False🔗 Analysis chain
Security Risk: Unrestricted CORS
Setting CORS_ORIGIN_ALLOW_ALL = True allows any origin to make cross-origin requests to your API, potentially enabling CSRF attacks.
Let's check if there are any specific origins that need CORS access:
Restore the restricted CORS configuration:
-# CORS_ORIGIN_ALLOW_ALL = False
-CORS_ORIGIN_ALLOW_ALL = True
+CORS_ORIGIN_ALLOW_ALL = FalseThe previous results show that CORS is configured using environment variables. Let's understand how these are parsed and what default values are set to make a more informed decision about the security implications.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for frontend origins or CORS configurations
rg "CORS_ALLOWED_ORIGINS" -A 5
Length of output: 475
Script:
#!/bin/bash
# Let's check the parse_env_list function and environment variables setup
rg "parse_env_list" -B 5 -A 10
# Also check for any environment variable files
fd -g ".env*"
Length of output: 3533
| # Define a constant for maximum upload size | ||
| MAX_UPLOAD_SIZE_MB = 10 # Maximum upload size in MB | ||
| MAX_UPLOAD_SIZE = MAX_UPLOAD_SIZE_MB * 1024 * 1024 # Convert to bytes | ||
|
|
||
| # Apply the maximum upload size to Django settings |
There was a problem hiding this comment.
💡 Codebase verification
Upload size limits need alignment across the codebase
The verification revealed inconsistencies in upload size limits across different parts of the application:
src/website/core/settings.py: Sets 10MB as MAX_UPLOAD_SIZEsrc/website/utils/validators.py: Has two different limits:- 8MB limit for image validation
- 10MB limit for general file validation
src/website/utils/fields.py: Sets 10MB limit for image uploads
To maintain consistency and prevent user confusion:
- Align the 8MB image validation limit in
validators.pywith the 10MB limit used elsewhere - Consider extracting the size limit to a single configuration point to prevent future divergence
🔗 Analysis chain
Verify upload size limits across environments
The 10MB upload limit is a good security measure, but we should ensure it's consistent with frontend validations and server configurations.
Let's check for related configurations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for upload size configurations in frontend and nginx configs
rg -l "upload.*size|max.*size" --type-add 'conf:*.conf' -tconf -tjs -tts
Length of output: 74
Script:
#!/bin/bash
# Let's try a broader search for upload-related configurations
# Search in all file types since configurations could be in various places
rg -i "upload.*size|max.*size|file.*size" --type-add 'conf:*.conf' -A 2
# Also search for any upload-related frontend validations
rg -i "upload.*validation|file.*validation|maxFileSize|uploadSize" --type-add 'conf:*.conf' -A 2
# Check for nginx configurations specifically
fd -e conf -e nginx.conf . -x cat {}
# Look for environment-specific settings files
fd -e py -e env -e ini -e yaml -e json . | grep -i "settings\|config\|env"
Length of output: 10848
|
website changes in this PR available for preview here |
|
website changes in this PR available for preview here |
|
website changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/website/core/settings.py (3)
118-138: Architectural Concern: Security Settings Tied to DEBUGThe current implementation has several issues:
- Since DEBUG is hardcoded to True, the production security settings will never be applied
- Security configurations should not be tied to DEBUG status
- Development environments may need specific origins rather than allowing all
Consider this alternative approach:
# Use environment variables with secure defaults CORS_ORIGIN_ALLOW_ALL = get_env_bool('CORS_ALLOW_ALL', default=False) CORS_ALLOWED_ORIGINS = parse_env_list("CORS_ALLOWED_ORIGINS") CORS_ORIGIN_REGEX_WHITELIST = parse_env_list("CORS_ORIGIN_REGEX_WHITELIST") CSRF_TRUSTED_ORIGINS = parse_env_list("CSRF_TRUSTED_ORIGINS") # Security settings should always be enabled in production CSRF_COOKIE_SECURE = not DEBUG and not get_env_bool('DISABLE_SECURE_COOKIES', default=False) SESSION_COOKIE_SECURE = not DEBUG and not get_env_bool('DISABLE_SECURE_COOKIES', default=False)
250-254: Consider Type-Specific Upload LimitsWhile the upload size configuration is clearer now, consider implementing different limits for different file types:
# Define type-specific upload limits UPLOAD_LIMITS = { 'image': 5 * 1024 * 1024, # 5MB for images 'document': 10 * 1024 * 1024, # 10MB for documents 'video': 50 * 1024 * 1024, # 50MB for videos } # Default limit for general uploads MAX_UPLOAD_SIZE = UPLOAD_LIMITS['document'] # Use document limit as default DATA_UPLOAD_MAX_MEMORY_SIZE = MAX_UPLOAD_SIZE FILE_UPLOAD_MAX_MEMORY_SIZE = MAX_UPLOAD_SIZE
Line range hint
277-307: Security: Add Content Sanitization for Rich Text EditorThe Quill editor configuration allows rich content including images and videos, but lacks content sanitization settings.
Add sanitization settings to prevent XSS attacks:
QUILL_CONFIGS = { 'default': { # ... existing config ... 'sanitize': True, 'allowedTags': [ 'p', 'br', 'strong', 'em', 'u', 's', 'blockquote', 'ol', 'ul', 'li', 'a', 'img' ], 'allowedAttributes': { 'a': ['href', 'target'], 'img': ['src', 'alt'] } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/website/core/settings.py(5 hunks)
🔇 Additional comments (1)
src/website/core/settings.py (1)
55-59:
Critical Security Risk: DEBUG and ALLOWED_HOSTS
These changes reintroduce previously identified security vulnerabilities that could expose sensitive information and enable HTTP Host header attacks.
|
website changes in this PR available for preview here |
|
website changes in this PR available for preview here |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/website/core/settings.py(5 hunks)
🔇 Additional comments (4)
src/website/core/settings.py (4)
297-301: Verify Upload Size Consistency
The 10MB upload limit is clearly defined, but let's ensure it's consistent across:
- Frontend validation
- Nginx configuration
- Other Django apps
#!/bin/bash
# Search for upload size configurations
echo "Checking upload size configurations..."
rg -i "upload.*size|max.*size|file.*size" --type-add 'conf:*.conf' -tconf -tpy -tjs -tts
# Check nginx configuration
fd -e conf -e nginx.conf . -x cat {}57-58:
Security Risk: Unrestricted ALLOWED_HOSTS Setting
The current configuration bypasses Django's host header validation, potentially enabling HTTP Host header attacks. Please restore the environment-based configuration.
-# ALLOWED_HOSTS = parse_env_list("ALLOWED_HOSTS")
-ALLOWED_HOSTS = ['*']
+ALLOWED_HOSTS = parse_env_list("ALLOWED_HOSTS")131-182: Verify Production Origins
The production CORS and CSRF configurations are well-structured with specific origins. However, let's verify:
- All listed domains are still active and owned by your organization
- No deprecated or unused domains are included
✅ Verification successful
Let me try a different approach to verify the CORS and CSRF configurations by checking for any references to these domains in the codebase.
CORS and CSRF Origins Verified - Configuration is Valid
The codebase analysis confirms that all listed domains are actively used across the application:
- All domains are referenced in production Kubernetes configurations (
k8s/nginx/production/) - The domains are consistently used across multiple services (analytics, platform, api)
- The domains align with the organization's infrastructure as seen in deployment configs and service URLs
The CORS/CSRF configurations are properly structured with:
- Production domains (airqo.net, airqo.africa, airqo.org)
- Staging environments (staging-platform.airqo.net, staging-analytics.airqo.net)
- Development endpoints (localhost, 127.0.0.1)
- Institutional domain (airqo.mak.ac.ug)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Extract and verify all domains
echo "Checking domain ownership and status..."
domains=(
"staging-dot-airqo-frontend.appspot.com"
"staging.airqo.net"
"airqo.net"
"airqo.africa"
"airqo.org"
"airqo.mak.ac.ug"
"staging-platform.airqo.net"
"staging-analytics.airqo.net"
"analytics.airqo.net"
"platform.airqo.net"
)
for domain in "${domains[@]}"; do
echo "Checking $domain..."
# Perform DNS lookup and WHOIS query
dig +short "$domain"
whois "$domain" | grep -i "registrar\|organization"
done
Length of output: 4260
Script:
#!/bin/bash
# Search for domain references in the codebase
echo "Searching for domain references in code..."
# Look for any configuration files that might reference these domains
fd -e yml -e yaml -e json -e env -e conf -e config --exec grep -l "airqo\." {} \;
# Search for any hardcoded references to these domains in Python files
echo -e "\nChecking Python files for domain references..."
rg "airqo\.(net|africa|org|mak\.ac\.ug)" -t py
# Check for any deployment or infrastructure files
echo -e "\nChecking deployment configurations..."
fd -e yaml -e yml --exec grep -l "website-trigger-3-website-preview" {} \;
# Look for environment variable definitions
echo -e "\nChecking for environment variable configurations..."
fd -g "*env*" --exec grep -l "CORS_" {} \;
Length of output: 5836
324-326: Verify Quill Editor Security Settings
The Quill editor configuration allows image and video uploads. Please verify:
- File type restrictions are in place
- Upload size limits are enforced
- Content sanitization is implemented
✅ Verification successful
Let me gather more information about the Quill configuration and security settings.
Let me gather more information about content sanitization specifically for Quill editor.
Based on the comprehensive analysis of the codebase, I can now provide a final response regarding the Quill editor security concerns:
Quill Editor Security Measures are Properly Implemented
The codebase has robust security measures in place:
- File upload size limits are enforced (MAX_UPLOAD_SIZE_MB = 10MB)
- Content sanitization is implemented via delta_to_html.py which converts Quill content to sanitized HTML
- File uploads are handled through Django's secure file storage system with proper validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Quill-related security configurations
echo "Checking Quill security settings..."
rg -i "quill.*sanitize|quill.*security|quill.*upload" -A 5
Length of output: 513
Script:
#!/bin/bash
# Search for Quill configuration and related security settings
echo "Checking Quill configuration and security settings..."
# Look for Quill configuration
echo "=== Quill Configuration ==="
rg -i "quill.*config|quill.*options" -A 10
# Look for upload handlers and settings
echo -e "\n=== Upload Handlers ==="
rg -i "upload.*handler|file.*upload|media.*upload" -A 5
# Look for content sanitization
echo -e "\n=== Content Sanitization ==="
ast-grep --pattern 'def sanitize($$$)'
# Check settings.py for upload configurations
echo -e "\n=== Upload Settings ==="
rg "UPLOAD_|FILE_|MEDIA_" src/website/core/settings.py
Length of output: 26906
Script:
#!/bin/bash
# Search for Quill-related content handling and sanitization
echo "=== Quill Content Handling ==="
rg -i "quill.*content|quill.*sanitize|quill.*clean" -A 5
# Look for HTML sanitization implementations
echo -e "\n=== HTML Sanitization ==="
rg -i "sanitize.*html|clean.*html|bleach" -A 5
# Check for content validation
echo -e "\n=== Content Validation ==="
rg -i "validate.*content|content.*validation|content.*security" -A 5
Length of output: 42554
| if DEBUG: | ||
| # Allow all CORS origins during development | ||
| CORS_ORIGIN_ALLOW_ALL = True | ||
| CORS_ALLOWED_ORIGINS = [] | ||
| CORS_ORIGIN_REGEX_WHITELIST = [] | ||
|
|
||
| # Allow all CSRF origins during development | ||
| CSRF_TRUSTED_ORIGINS = [ | ||
| "https://website-trigger-3-website-preview-w7kzhvlewq-ew.a.run.app", | ||
| ] | ||
|
|
||
| # Optionally, you can add more relaxed settings | ||
| # For example, allow specific subdomains or ports if needed | ||
| else: |
There was a problem hiding this comment.
💡 Codebase verification
Remove Inactive Preview URL and Strengthen Development Security
The preview URL (website-trigger-3-website-preview-w7kzhvlewq-ew.a.run.app) returns a 404 error and appears in both development and production CSRF configurations. Additionally:
- Remove the inactive preview URL from CSRF_TRUSTED_ORIGINS in both DEBUG and production sections
- Replace CORS_ORIGIN_ALLOW_ALL = True with specific development origins:
CORS_ALLOWED_ORIGINS = [
"http://localhost:3000",
"http://127.0.0.1:3000"
]- The production configuration is properly secured with specific origins and regex patterns
🔗 Analysis chain
Development Mode Security Risk
The development configuration is overly permissive. Consider:
- Using a specific list of development origins instead of CORS_ORIGIN_ALLOW_ALL
- Limiting CSRF_TRUSTED_ORIGINS even in development
Let me search for any other preview URLs and CORS/CSRF configurations to get a complete picture of the security settings across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if the preview URL is still active and needed
curl -I "https://website-trigger-3-website-preview-w7kzhvlewq-ew.a.run.app"
Length of output: 890
Script:
#!/bin/bash
# Check for any other preview URLs in the codebase
rg -i "website.*preview.*\.run\.app" -A 2 -B 2
# Check for any other CORS/CSRF configurations
rg "CORS_|CSRF_" -A 2 -B 2
Length of output: 6237
Description
Summary by CodeRabbit
Configuration Changes
DEBUGsetting to a hardcoded value ofTrue.ALLOWED_HOSTSto allow all hosts.Script Enhancements
GUNICORN_WORKERSenvironment variable.