diff --git a/backend/common/tests/test_prod_storage_settings.py b/backend/common/tests/test_prod_storage_settings.py new file mode 100644 index 00000000..e1d0067f --- /dev/null +++ b/backend/common/tests/test_prod_storage_settings.py @@ -0,0 +1,117 @@ +"""``crm/server_settings.py`` keeps prod uploads on S3, private, and unmixed. + +The test suite runs under ``ENV_TYPE=dev``, so this module is never imported by +anything else here. That is precisely why it needs its own test: every defect it +has carried reached production undetected, because nothing in CI loads the file +and the symptoms all appear somewhere other than where the mistake is. + +Four have already been live on bottlecrm.io: + +* ``DEFAULT_FILE_STORAGE`` was removed in Django 5.1, so on Django 6 it read + back fine from ``settings`` while ``default_storage`` quietly resolved to + ``FileSystemStorage`` and wrote to a root-owned ``/media/``. Uploads had never + once worked in production. +* ``AWS_S3_CUSTOM_DOMAIN`` made ``url()`` return unsigned ``https://...`` + links, which resolve for anyone holding them, across tenants, forever. +* boto3's default addressing signs the global S3 host, and S3 answers it with a + 307 to the regional host. Following the redirect changes the Host the + signature covered, so every download came back 403 SignatureDoesNotMatch. +* django-storages defaults ``AWS_S3_FILE_OVERWRITE`` to ``True``, the opposite + of ``FileSystemStorage``. None of the ``upload_to`` values in + ``common/models.py`` carry an org or a unique component, so two orgs uploading + the same filename shared a key and the second replaced the first's bytes while + the first's ``Attachment`` row kept pointing at it. RLS cannot see that: the + row is correctly scoped and untouched, only the object behind the key changed. + +Each assertion below is one of those, so a regression fails here rather than in +production. +""" + +from __future__ import annotations + +import importlib +import os +from unittest import mock + +import pytest + +BASE_ENV = { + "AWS_BUCKET_NAME": "test-bucket", + "AWS_ACCESS_KEY_ID": "test-key-id", + "AWS_SECRET_ACCESS_KEY": "test-secret-value", + "AWS_SES_REGION_NAME": "ap-south-1", + "AWS_SES_REGION_ENDPOINT": "email.ap-south-1.amazonaws.com", + # Present but empty, which is what disables the Sentry SDK. The init call is + # patched out below as well, so importing this module reports nothing. + "SENTRY_DSN": "", +} + + +def load(**overrides): + """Import ``crm.server_settings`` fresh under a controlled environment.""" + env = {**BASE_ENV, **overrides} + with mock.patch.dict(os.environ, env, clear=False): + with mock.patch("sentry_sdk.init"): + module = importlib.import_module("crm.server_settings") + return importlib.reload(module) + + +def test_default_storage_is_s3_via_storages_dict(): + """STORAGES is what Django reads; DEFAULT_FILE_STORAGE is inert on 5.1+.""" + settings = load() + assert settings.STORAGES["default"]["BACKEND"] == ( + "storages.backends.s3boto3.S3Boto3Storage" + ) + # Django does not merge STORAGES with its defaults, so omitting this key + # makes every static file lookup raise InvalidStorageError. + assert "staticfiles" in settings.STORAGES + + +def test_uploads_do_not_overwrite_each_other(): + """Two orgs uploading the same filename must not share one object.""" + settings = load() + assert settings.AWS_S3_FILE_OVERWRITE is False + + +def test_urls_are_signed_not_public(): + """No custom domain means url() presigns, so the bucket stays private.""" + settings = load() + assert not hasattr(settings, "AWS_S3_CUSTOM_DOMAIN") + + +def test_addressing_style_is_virtual(): + """Anything else signs the global host and 403s after the 307 redirect.""" + settings = load() + assert settings.AWS_S3_ADDRESSING_STYLE == "virtual" + + +def test_region_is_set_and_defaults_to_the_ses_region(): + """SigV4 signs the region in, so a wrong guess produces rejected URLs.""" + assert load().AWS_S3_REGION_NAME == "ap-south-1" + assert load(AWS_S3_REGION_NAME="eu-west-1").AWS_S3_REGION_NAME == "eu-west-1" + + +def test_uploads_are_prefixed_so_they_are_reachable(): + """Without the prefix, uploads land at the bucket root and 404 when served.""" + settings = load() + assert settings.AWS_LOCATION == "media" + + +@pytest.mark.parametrize("blank", ["", " "]) +def test_blank_bucket_name_fails_at_startup(blank): + """Empty is the case that hurts: os.environ[...] alone does not catch it. + + With no guard the module still evaluates end to end, uploads address a + bucket named "", and the deployment looks configured until a user attaches + a file. That is how this shipped. + """ + with pytest.raises(ValueError, match="AWS_BUCKET_NAME is empty"): + load(AWS_BUCKET_NAME=blank) + + +def test_missing_bucket_name_fails_at_startup(): + with mock.patch.dict(os.environ, BASE_ENV, clear=False): + os.environ.pop("AWS_BUCKET_NAME", None) + with mock.patch("sentry_sdk.init"): + with pytest.raises(KeyError): + importlib.reload(importlib.import_module("crm.server_settings")) diff --git a/backend/crm/server_settings.py b/backend/crm/server_settings.py index 9e268575..cf85ef2d 100644 --- a/backend/crm/server_settings.py +++ b/backend/crm/server_settings.py @@ -6,19 +6,120 @@ from sentry_sdk.integrations.django import DjangoIntegration DEBUG = False AWS_STORAGE_BUCKET_NAME = AWS_BUCKET_NAME = os.environ["AWS_BUCKET_NAME"] + +# `os.environ[...]` catches the variable being absent but not its being present +# and empty, and empty is the case that hurts. With `AWS_BUCKET_NAME=`, every +# line below still evaluates: S3_DOMAIN becomes ".s3.amazonaws.com", MEDIA_URL +# becomes "//.s3.amazonaws.com/media/", and uploads go to a bucket named "". +# Nothing raises, so the deployment looks configured and fails only when a user +# tries to attach a file. Found exactly that way on bottlecrm.io, where the AWS +# credentials turned out to be an SES-only IAM user and no bucket had ever +# existed. Fail at startup instead, in the same shape as the FRONTEND_URL guard +# in settings.py. +if not AWS_BUCKET_NAME.strip(): + raise ValueError( + "AWS_BUCKET_NAME is empty. ENV_TYPE=prod stores uploads in S3, so this " + "must name a real bucket, and the AWS credentials must be allowed to " + "write to it. Set it, or run with ENV_TYPE=dev to store uploads on " + "local disk under MEDIA_ROOT." + ) + AWS_ACCESS_KEY_ID = os.environ["AWS_ACCESS_KEY_ID"] AWS_SECRET_ACCESS_KEY = os.environ["AWS_SECRET_ACCESS_KEY"] -S3_DOMAIN = AWS_S3_CUSTOM_DOMAIN = str(AWS_BUCKET_NAME) + ".s3.amazonaws.com" AWS_SES_REGION_NAME = os.environ["AWS_SES_REGION_NAME"] AWS_SES_REGION_ENDPOINT = os.environ["AWS_SES_REGION_ENDPOINT"] +# No AWS_S3_CUSTOM_DOMAIN, deliberately. +# +# Setting it makes django-storages return plain `https:///` URLs +# with no signature, which only resolve if the bucket is public-read. For a +# multi-tenant CRM that means every uploaded attachment is fetchable by anyone +# holding the URL, across tenants, with no authorization check anywhere. Leaving +# it unset makes `storage.url()` return a presigned URL that expires, so the +# bucket can keep Block Public Access fully on. +# +# S3_DOMAIN is kept because other settings and templates refer to it, but it is +# no longer wired into how URLs are generated. +S3_DOMAIN = str(AWS_BUCKET_NAME) + ".s3.amazonaws.com" + +# Required, and easy to miss. Presigned URLs are SigV4, which signs the region +# into the signature, so boto3 guessing wrong produces URLs the bucket rejects. +# Defaults to the SES region because a single-region deployment is the common +# case; override when the bucket lives somewhere else. +AWS_S3_REGION_NAME = os.environ.get("AWS_S3_REGION_NAME", AWS_SES_REGION_NAME) + +# Required for any bucket outside us-east-1, and the failure is not obvious. +# +# boto3's default addressing builds presigned URLs against the global host, +# `.s3.amazonaws.com`, even when the client itself is pointed at the +# regional endpoint. S3 answers that host with a 307 redirect to +# `.s3..amazonaws.com` and preserves the query string. The +# signature covers the Host header, so following the redirect presents a +# signature computed over the old host to the new one, and the request comes +# back 403 SignatureDoesNotMatch. Every download breaks; uploads keep working, +# because those go straight through the regional client and never redirect. +# +# Verified against the real bucket: default style returns 307 then 403, this +# setting returns 200. It is not a workaround for a wrong region, and the error +# does not name the region, which is what makes it slow to diagnose. +AWS_S3_ADDRESSING_STYLE = "virtual" + AWS_S3_OBJECT_PARAMETERS = { "CacheControl": "max-age=86400", } -DEFAULT_FILE_STORAGE = "storages.backends.s3boto3.S3Boto3Storage" DEFAULT_S3_PATH = "media" +# STORAGES, not DEFAULT_FILE_STORAGE. +# +# `DEFAULT_FILE_STORAGE` was deprecated in Django 4.2 and REMOVED in 5.1. On a +# modern Django it is an ordinary unused name: the setting is still readable via +# `settings.DEFAULT_FILE_STORAGE`, so it looks configured, while `default_storage` +# quietly resolves from `STORAGES` instead and lands on the built-in +# `FileSystemStorage`. Every upload then tries to write to MEDIA_ROOT below, +# which is `/media/` at the filesystem root, owned by root, and fails with +# `PermissionError: [Errno 13] Permission denied: '/media/attachments'`. +# +# That is not a hypothetical: it was live on bottlecrm.io for days before anyone +# noticed, because the failure is a 500 on upload rather than anything visible at +# startup, and `settings.DEFAULT_FILE_STORAGE` reads back exactly as intended. +# +# Both keys are required. Django does not merge this setting with its defaults, +# so omitting "staticfiles" makes every static file lookup raise +# InvalidStorageError. +STORAGES = { + "default": {"BACKEND": "storages.backends.s3boto3.S3Boto3Storage"}, + "staticfiles": {"BACKEND": "django.contrib.staticfiles.storage.StaticFilesStorage"}, +} + +# The key prefix inside the bucket. Without it, django-storages writes to the +# bucket root while MEDIA_URL below points at /media/, so uploads would succeed +# and then 404 when served, which is a worse failure than the one above because +# nothing errors. +AWS_LOCATION = DEFAULT_S3_PATH + +# Cross-tenant overwrite. django-storages defaults this to True, which is the +# opposite of what FileSystemStorage does, so moving to S3 changes the semantics +# of every upload path in the project without touching any of them. +# +# None of the three upload_to values in common/models.py carry an org or a +# unique component: `org_logos/`, `CommentFiles`, and `attachments/%Y/%m/` all +# derive the key from the user's filename alone. With overwriting on, one org +# uploading `contract.pdf` lands on the same key as another org's `contract.pdf` +# in the same month, replaces its bytes, and leaves the first org's Attachment +# row pointing at the second org's file. RLS cannot see this: the database row +# is untouched and correctly scoped, and only the object behind the key changed. +# +# Setting it False restores Django's own behaviour, which is to append a random +# suffix until the name is free. That keeps dev on local disk and prod on S3 +# behaving alike, and it is also what stops a re-upload from accumulating a +# billable noncurrent version on a versioned bucket. +AWS_S3_FILE_OVERWRITE = False + +# Unused while STORAGES routes to S3, which addresses paths by key rather than +# by filesystem location. Kept so that a deployment falling back to +# FileSystemStorage has a defined destination rather than writing relative to +# the working directory. MEDIA_ROOT = f"/{DEFAULT_S3_PATH}/" MEDIA_URL = f"//{S3_DOMAIN}/{DEFAULT_S3_PATH}/" # STATIC_URL = "https://%s/" % (S3_DOMAIN) diff --git a/backend/pyproject.toml b/backend/pyproject.toml index c2816c96..0bbd0d89 100644 --- a/backend/pyproject.toml +++ b/backend/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "hatchling.build" [project] name = "django-crm" -version = "1.9.0" +version = "1.9.1" # The one-line summary PyPI shows under the project title and in search results, # so it carries the terms people actually search for ("open source CRM", # "self-hosted CRM", "Django CRM") and names the modules rather than the diff --git a/backend/uv.lock b/backend/uv.lock index 918eefb6..5b5bbddc 100644 --- a/backend/uv.lock +++ b/backend/uv.lock @@ -583,7 +583,7 @@ wheels = [ [[package]] name = "django-crm" -version = "1.9.0" +version = "1.9.1" source = { editable = "." } dependencies = [ { name = "cairocffi" },