Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

website: Allowed CSRF Update #4029

Merged
merged 6 commits into from
Dec 9, 2024
Merged

website: Allowed CSRF Update #4029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7054518
test again
OchiengPaul442 Dec 9, 2024
d953b81
test
OchiengPaul442 Dec 9, 2024
720a199
test
OchiengPaul442 Dec 9, 2024
c1c87c5
updates
OchiengPaul442 Dec 9, 2024
220a54a
update
OchiengPaul442 Dec 9, 2024
717df35
update
OchiengPaul442 Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 80 additions & 10 deletions src/website/core/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@

import os
import sys
from pathlib import Path
Expand Down Expand Up @@ -55,7 +54,8 @@ def require_env_var(env_var: str) -> str:
SECRET_KEY = require_env_var('SECRET_KEY')
DEBUG = get_env_bool('DEBUG', default=False)

ALLOWED_HOSTS = parse_env_list("ALLOWED_HOSTS")
# ALLOWED_HOSTS = parse_env_list("ALLOWED_HOSTS")
ALLOWED_HOSTS = ['*']

Comment on lines +57 to +58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
# ALLOWED_HOSTS = parse_env_list("ALLOWED_HOSTS")
ALLOWED_HOSTS = ['*']
ALLOWED_HOSTS = parse_env_list("ALLOWED_HOSTS")

# ---------------------------------------------------------
# Application Definitions
Expand Down Expand Up @@ -100,7 +100,7 @@ def require_env_var(env_var: str) -> str:
# Middleware
# ---------------------------------------------------------
MIDDLEWARE = [
'corsheaders.middleware.CorsMiddleware',
'corsheaders.middleware.CorsMiddleware', # Must be first
'django.middleware.security.SecurityMiddleware',
'whitenoise.middleware.WhiteNoiseMiddleware',
'django.contrib.sessions.middleware.SessionMiddleware',
Expand All @@ -114,11 +114,75 @@ def require_env_var(env_var: str) -> str:
# ---------------------------------------------------------
# CORS and CSRF Configuration
# ---------------------------------------------------------
CORS_ORIGIN_ALLOW_ALL = 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")

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:
# Restrict CORS origins in production
Comment on lines +117 to +130
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

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:

  1. Using a specific list of development origins instead of CORS_ORIGIN_ALLOW_ALL
  2. 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

CORS_ORIGIN_ALLOW_ALL = False
CORS_ALLOWED_ORIGINS = [
"https://staging-dot-airqo-frontend.appspot.com",
"https://staging.airqo.net",
"https://airqo.net",
"https://airqo.africa",
"https://airqo.org",
"https://airqo.mak.ac.ug",
"http://127.0.0.1:8000",
"http://localhost:3000",
"https://staging-platform.airqo.net",
"https://staging-analytics.airqo.net",
"https://analytics.airqo.net",
"https://platform.airqo.net",
]
CORS_ORIGIN_REGEX_WHITELIST = [
# Matches subdomains under airqo.net, airqo.africa, airqo.org, airqo.io
r"^https://[a-zA-Z0-9_\-]+\.airqo\.(net|africa|org|io)$",
# Matches airqo.africa, airqo.org, and airqo.mak.ac.ug
r"^https://airqo\.(africa|org|mak\.ac\.ug)$",
# Matches staging-dot-airqo-frontend.appspot.com
r"^https://staging-dot-airqo-frontend\.appspot\.com$",
r"^https://staging-platform\.airqo\.net$", # Matches staging-platform.airqo.net
# Matches staging-analytics.airqo.net
r"^https://staging-analytics\.airqo\.net$",
r"^https://analytics\.airqo\.net$", # Matches analytics.airqo.net
r"^https://platform\.airqo\.net$", # Matches platform.airqo.net
# Matches any subpath under https://platform.airqo.net/website/admin
r"^https://platform\.airqo\.net/website/admin.*$",
# Matches any subpath under https://staging-platform.airqo.net/website/admin
r"^https://staging-platform\.airqo\.net/website/admin.*$",
]

# Trust specific origins for CSRF protection in production
# CSRF_TRUSTED_ORIGINS = parse_env_list("CSRF_TRUSTED_ORIGINS")
CSRF_TRUSTED_ORIGINS = [
"https://staging-dot-airqo-frontend.appspot.com",
"https://staging.airqo.net",
"https://airqo.net",
"https://airqo.africa",
"https://airqo.org",
"https://airqo.mak.ac.ug",
"http://127.0.0.1:8000",
"http://localhost:3000",
"https://*.cloudshell.dev",
"https://staging-platform.airqo.net",
"https://staging-analytics.airqo.net",
"https://analytics.airqo.net",
"https://platform.airqo.net",
"https://website-trigger-3-website-preview-w7kzhvlewq-ew.a.run.app",
]


# Security settings
CSRF_COOKIE_SECURE = not DEBUG
SESSION_COOKIE_SECURE = not DEBUG

Expand Down Expand Up @@ -190,7 +254,6 @@ def require_env_var(env_var: str) -> str:
STATICFILES_DIRS = [BASE_DIR / 'static']
STATICFILES_STORAGE = 'whitenoise.storage.CompressedManifestStaticFilesStorage'


if DEBUG:
# Local file storage for development
MEDIA_URL = '/media/'
Expand Down Expand Up @@ -231,7 +294,11 @@ def require_env_var(env_var: str) -> str:
# ---------------------------------------------------------
# File Upload Limits
# ---------------------------------------------------------
MAX_UPLOAD_SIZE = 10 * 1024 * 1024 # 10MB
# 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
DATA_UPLOAD_MAX_MEMORY_SIZE = MAX_UPLOAD_SIZE
Comment on lines +297 to +301
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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_SIZE
  • src/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.py with 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

FILE_UPLOAD_MAX_MEMORY_SIZE = MAX_UPLOAD_SIZE

Expand All @@ -254,6 +321,9 @@ def require_env_var(env_var: str) -> str:
},
}

# ---------------------------------------------------------
# Quill Editor Configuration
# ---------------------------------------------------------
QUILL_CONFIGS = {
'default': {
'theme': 'snow',
Expand Down
1 change: 1 addition & 0 deletions src/website/entrypoint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

🛠️ Refactor suggestion

Consider using the dynamic worker configuration

The commented line with ${GUNICORN_WORKERS:-3} represents a better configuration approach as it:

  1. Provides flexibility to adjust worker count through environment variables
  2. Maintains a sensible default of 3 workers if the variable isn't set
  3. 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 info

The 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

Loading