bump

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://<bucket>...``
+  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"))

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://<domain>/<key>` 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,
+# `<bucket>.s3.amazonaws.com`, even when the client itself is pointed at the
+# regional endpoint. S3 answers that host with a 307 redirect to
+# `<bucket>.s3.<region>.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)

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

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" },