From f86eccf9a34ddd2c1f7e2c88c28dcbfda8387bf7 Mon Sep 17 00:00:00 2001 From: JakeBreath Date: Fri, 18 Sep 2026 00:21:14 -0500 Subject: [PATCH] Security fixes: SSRF, staff role escalation, SPA-only gating, throttling, encrypted keys Findings from the audit (50-check harness across guest/user/uploader/staff/ admin) and their fixes: - SSRF: 'Download to Library' and the staged-upload resolve path fetched any http(s) URL. services.validate_remote_url now enforces the e621 media allowlist and open_remote re-validates every redirect hop; the download-task create endpoint and the guest proxy use them, so internal addresses (127.0.0.1, LAN, metadata) are rejected with 400. - Privilege escalation: staff could promote users to staff and demote other staff. Role changes across the staff boundary now require an admin, matching the account-deletion rules; the Users page hides what the backend would refuse. - SPA-only gating: /api/storage/ and /api/duplicates/* were readable by any authenticated account (absolute paths, duplicate groups) while the SPA only shows them to uploaders. They now require CanUpload. - Throttling (REST_FRAMEWORK, env-overridable, counted in Redis): anon 120/min, user 600/min, login 5/min, register 20/hour, guest e621 proxy 60/hour. Login now goes through a throttled view. - e621 API keys are encrypted at rest with a Fernet key derived from SECRET_KEY (apps/accounts/crypto.py); a data migration encrypts existing rows and the column widens first. Reads decrypt transparently, legacy plaintext still works, and a changed SECRET_KEY reads as 'not configured' instead of leaking. Rotating SECRET_KEY now invalidates stored keys as well as signed media URLs. - Hardening: the server refuses to start with DEBUG=False while SECRET_KEY is still the development default. Verified: corrected harness 50/50 (guest visibility, IDOR, signed-URL tamper/expiry, staged-upload/similarity privacy, role matrix, SSRF), login throttles at the 6th attempt with 429, anon polling unaffected, the guest proxy still reaches allowlisted hosts, live e621 auth works with the decrypted key, and DB rows hold only ciphertext. --- AGENTS.md | 17 +++++++ ROADMAP.md | 3 ++ backend/.env.example | 8 +++ backend/apps/accounts/crypto.py | 48 ++++++++++++++++++ .../migrations/0006_encrypt_e621_api_keys.py | 34 +++++++++++++ backend/apps/accounts/models.py | 12 ++++- backend/apps/accounts/urls.py | 4 +- backend/apps/accounts/views.py | 33 +++++++++++-- backend/apps/library/e621.py | 6 ++- backend/apps/library/services.py | 49 +++++++++++++++++-- backend/apps/library/tools.py | 10 ++-- backend/apps/library/views.py | 23 ++++++--- backend/config/settings.py | 22 +++++++++ backend/requirements.txt | 1 + frontend/src/features/users/UsersPage.tsx | 22 +++++++-- 15 files changed, 265 insertions(+), 27 deletions(-) create mode 100644 backend/apps/accounts/crypto.py create mode 100644 backend/apps/accounts/migrations/0006_encrypt_e621_api_keys.py diff --git a/AGENTS.md b/AGENTS.md index dbb603e..e51c7df 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -47,3 +47,20 @@ Project constraints (do not regress): and the server only applies the result via POST /api/files/J-x/optimize/. - Do not use imgdd; perceptual hashing is imagehash server-side. - Chat/messaging features are out of scope. + +Security hardening (do not weaken): +- e621 API keys are stored Fernet-encrypted with a key derived from + SECRET_KEY (apps/accounts/crypto.py); rotating SECRET_KEY invalidates them + (and all signed media URLs), so users must re-enter the key. +- API throttles live in REST_FRAMEWORK (env-overridable): anon 120/min, + user 600/min, login 5/min, register 20/hour, e621_proxy 60/hour. +- Only admins (superusers) may grant/revoke the staff role or delete + staff/admin accounts; staff manage regular/uploader accounts only. +- Storage, duplicates, delete, temp-clear, uploads and downloads require + upload rights (CanUpload), not merely authentication. +- Server-side fetching only happens for allowlisted e621 media hosts via + services.validate_remote_url / open_remote (every redirect hop is + re-validated); do not call requests.get on user-supplied URLs elsewhere. +- A repeatable audit harness (permission matrix, IDOR, guest visibility, + signed URLs, SSRF, throttles) was used to verify this; formalising it as + a test suite is still open in the roadmap. diff --git a/ROADMAP.md b/ROADMAP.md index 83b721b..be5611a 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -170,3 +170,6 @@ Files now stage first and are resolved before entering the library. - Anything touching e621 endpoints follows the OpenAPI spec (https://e621.wiki/openapi.yaml) — see AGENTS.md for how to fetch and which response shapes to watch out for. +- Security review done (Aug 2026): permissions verified per role with a + repeatable harness, e621 keys encrypted at rest, rate limits added and the + download paths host-allowlisted. See AGENTS.md for the rules to keep. diff --git a/backend/.env.example b/backend/.env.example index 1d59693..f29b2da 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -2,6 +2,14 @@ SECRET_KEY=change-me-to-a-long-random-string DEBUG=True ALLOWED_HOSTS=localhost,127.0.0.1 +# Rate limits (per IP for anonymous, per account when signed in). Counted in +# the shared Redis cache; the defaults fit the shell's polling. +# THROTTLE_ANON=120/min +# THROTTLE_USER=600/min +# THROTTLE_LOGIN=5/min +# THROTTLE_REGISTER=20/hour +# THROTTLE_E621_PROXY=60/hour + # Cross-origin frontends (comma separated). Same-origin keeps working with an # empty list; add e.g. https://j621.example.com when the SPA is served from # another origin than this API. diff --git a/backend/apps/accounts/crypto.py b/backend/apps/accounts/crypto.py new file mode 100644 index 0000000..63a4f17 --- /dev/null +++ b/backend/apps/accounts/crypto.py @@ -0,0 +1,48 @@ +"""Symmetric encryption for secrets stored at rest, keyed by SECRET_KEY. + +Used for the per-user e621 API keys: a database dump (or backup) alone does +not expose them. Rotating SECRET_KEY makes existing values undecryptable, +exactly like the signed media URLs stop verifying — users then re-enter +their e621 key. +""" + +import base64 +import hashlib + +from cryptography.fernet import Fernet, InvalidToken +from django.conf import settings + +PREFIX = "enc:" + + +def _fernet() -> Fernet: + # Derive a stable Fernet key from the project secret. + digest = hashlib.sha256( + f"j621-secret-v1:{settings.SECRET_KEY}".encode() + ).digest() + return Fernet(base64.urlsafe_b64encode(digest)) + + +def encrypt_secret(value: str) -> str: + """Encrypt for storage; empty stays empty and already-encrypted passes through.""" + if not value: + return "" + if value.startswith(PREFIX): + return value + return PREFIX + _fernet().encrypt(value.encode()).decode() + + +def decrypt_secret(value: str) -> str: + """Decrypt a stored value; legacy plaintext passes through. + + Returns "" when the value cannot be decrypted (e.g. SECRET_KEY changed), + which reads as "not configured" and asks the user for the key again. + """ + if not value: + return "" + if not value.startswith(PREFIX): + return value + try: + return _fernet().decrypt(value[len(PREFIX) :].encode()).decode() + except InvalidToken: + return "" diff --git a/backend/apps/accounts/migrations/0006_encrypt_e621_api_keys.py b/backend/apps/accounts/migrations/0006_encrypt_e621_api_keys.py new file mode 100644 index 0000000..01fef3d --- /dev/null +++ b/backend/apps/accounts/migrations/0006_encrypt_e621_api_keys.py @@ -0,0 +1,34 @@ +from django.db import migrations, models + + +def encrypt_existing_keys(apps, schema_editor): + """Store existing e621 API keys encrypted (idempotent).""" + from apps.accounts.crypto import encrypt_secret + + User = apps.get_model("accounts", "User") + for user in User.objects.exclude(e621_api_key="").iterator(): + if user.e621_api_key.startswith("enc:"): + continue + user.e621_api_key = encrypt_secret(user.e621_api_key) + user.save(update_fields=["e621_api_key"]) + + +def noop(apps, schema_editor): + pass + + +class Migration(migrations.Migration): + dependencies = [ + ("accounts", "0005_user_preferences"), + ] + + operations = [ + # Ciphertext is longer than the plaintext key, so widen the column + # before encrypting existing rows. + migrations.AlterField( + model_name="user", + name="e621_api_key", + field=models.CharField(blank=True, default="", max_length=400), + ), + migrations.RunPython(encrypt_existing_keys, noop), + ] diff --git a/backend/apps/accounts/models.py b/backend/apps/accounts/models.py index fb5ffd8..a55b024 100644 --- a/backend/apps/accounts/models.py +++ b/backend/apps/accounts/models.py @@ -23,14 +23,22 @@ class User(AbstractUser): related_name="+", ) e621_username = models.CharField(max_length=100, blank=True, default="") - e621_api_key = models.CharField(max_length=100, blank=True, default="") + # Stored encrypted (see apps/accounts/crypto.py): never the plaintext key. + e621_api_key = models.CharField(max_length=400, blank=True, default="") e621_base_url = models.CharField(max_length=200, default="https://e621.net") # Per-user browse preferences (landing page, default filters, grid size). preferences = models.JSONField(default=dict, blank=True) + @property + def e621_api_key_plain(self): + """The decrypted e621 key ("" when it cannot be decrypted).""" + from .crypto import decrypt_secret + + return decrypt_secret(self.e621_api_key) + @property def e621_configured(self): - return bool(self.e621_username and self.e621_api_key) + return bool(self.e621_username and self.e621_api_key_plain) @property def can_upload(self): diff --git a/backend/apps/accounts/urls.py b/backend/apps/accounts/urls.py index 7e45c27..154202e 100644 --- a/backend/apps/accounts/urls.py +++ b/backend/apps/accounts/urls.py @@ -1,9 +1,9 @@ from django.urls import path -from rest_framework.authtoken.views import obtain_auth_token from .views import ( AvatarView, E621CredentialsView, + LoginView, LogoutView, MeView, PreferencesView, @@ -12,7 +12,7 @@ from .views import ( urlpatterns = [ path("register/", RegisterView.as_view(), name="register"), - path("token/", obtain_auth_token, name="login"), + path("token/", LoginView.as_view(), name="login"), path("logout/", LogoutView.as_view(), name="logout"), path("me/", MeView.as_view(), name="me"), path("avatar/", AvatarView.as_view(), name="avatar"), diff --git a/backend/apps/accounts/views.py b/backend/apps/accounts/views.py index 16ac423..01f1c1c 100644 --- a/backend/apps/accounts/views.py +++ b/backend/apps/accounts/views.py @@ -2,14 +2,17 @@ import logging from rest_framework import mixins, status, viewsets from rest_framework.authtoken.models import Token +from rest_framework.authtoken.views import ObtainAuthToken from rest_framework.permissions import AllowAny, IsAuthenticated from rest_framework.response import Response +from rest_framework.throttling import ScopedRateThrottle from rest_framework.views import APIView from django.db.models import Count, Q from apps.core.permissions import IsAppStaff from apps.library.models import MediaItem +from .crypto import encrypt_secret from .models import User from .serializers import ( E621CredentialsSerializer, @@ -23,8 +26,17 @@ from .serializers import ( logger = logging.getLogger(__name__) +class LoginView(ObtainAuthToken): + """Token login, rate limited per IP to slow down credential stuffing.""" + + throttle_classes = [ScopedRateThrottle] + throttle_scope = "login" + + class RegisterView(APIView): permission_classes = [AllowAny] + throttle_classes = [ScopedRateThrottle] + throttle_scope = "register" def post(self, request): serializer = RegisterSerializer(data=request.data) @@ -64,7 +76,7 @@ class E621CredentialsView(APIView): def _payload(self, user): return { "username": user.e621_username, - "api_key": user.e621_api_key, + "api_key": user.e621_api_key_plain, "base_url": user.e621_base_url, "configured": user.e621_configured, } @@ -78,7 +90,7 @@ class E621CredentialsView(APIView): data = serializer.validated_data user = request.user user.e621_username = data["username"].strip() - user.e621_api_key = data["api_key"].strip() + user.e621_api_key = encrypt_secret(data["api_key"].strip()) user.e621_base_url = ( (data.get("base_url") or "https://e621.net").strip().rstrip("/") ) @@ -172,13 +184,28 @@ class UserViewSet( def update(self, request, *args, **kwargs): user = self.get_object() + actor = request.user serializer = UserUpdateSerializer(data=request.data) serializer.is_valid(raise_exception=True) data = serializer.validated_data update_fields = [] if "role" in data: - user.role = data["role"] + new_role = data["role"] + # Same rule as account deletion: only admins may move accounts + # across the staff boundary (granting or revoking staff). + if ( + not actor.is_superuser + and new_role != user.role + and (user.is_app_staff or new_role == User.ROLE_STAFF) + ): + return Response( + { + "detail": "Only an admin can change staff roles.", + }, + status=status.HTTP_403_FORBIDDEN, + ) + user.role = new_role update_fields.append("role") if "avatar_j_id" in data: item, error = resolve_avatar_item(data.get("avatar_j_id")) diff --git a/backend/apps/library/e621.py b/backend/apps/library/e621.py index e76e0ac..98c4c50 100644 --- a/backend/apps/library/e621.py +++ b/backend/apps/library/e621.py @@ -59,7 +59,11 @@ def get(user, path, params=None, timeout=30, require_auth=True): response = requests.get( f"{base}{path}", params=params, - auth=(user.e621_username, user.e621_api_key) if configured else None, + auth=( + (user.e621_username, user.e621_api_key_plain) + if configured + else None + ), headers={"User-Agent": settings.USER_AGENT}, timeout=timeout, ) diff --git a/backend/apps/library/services.py b/backend/apps/library/services.py index 759fc99..9ca3374 100644 --- a/backend/apps/library/services.py +++ b/backend/apps/library/services.py @@ -255,6 +255,51 @@ class DownloadCancelled(Exception): """Raised when a streamed download is cancelled by the user.""" +class RemoteUrlError(ValueError): + """The URL is not an allowed e621 media URL.""" + + +def validate_remote_url(url): + """Only http(s) URLs on the known e621 media hosts may be fetched. + + Without this the download paths are an SSRF hole: any uploader could make + the server fetch internal addresses (127.0.0.1, LAN services, cloud + metadata) and read the response back through the library. + """ + from urllib.parse import urlparse + + parsed = urlparse(str(url or "").strip()) + if parsed.scheme not in {"http", "https"} or not parsed.hostname: + raise RemoteUrlError("Only http(s) URLs can be fetched.") + if parsed.hostname not in settings.E621_MEDIA_HOSTS: + raise RemoteUrlError("That host is not an allowed e621 media host.") + return url + + +def open_remote(url, *, max_redirects=3, **kwargs): + """GET an allowlisted URL, re-validating every redirect hop. + + Returns a streaming ``requests`` response. Redirects are followed + manually so a hop cannot jump to an internal host. + """ + import requests + from urllib.parse import urljoin + + current = url + for _ in range(max_redirects + 1): + validate_remote_url(current) + response = requests.get(current, allow_redirects=False, **kwargs) + if response.is_redirect or response.is_permanent_redirect: + location = response.headers.get("Location") + response.close() + if not location: + raise RemoteUrlError("The remote server redirected without a target.") + current = urljoin(current, location) + continue + return response + raise RemoteUrlError("Too many redirects from the remote server.") + + def download_file( url, destination, @@ -268,10 +313,8 @@ def download_file( worker: without it a hung socket would keep a job "downloading" forever and the cancel flag could never be observed. """ - import requests - headers = {"User-Agent": settings.USER_AGENT} - with requests.get( + with open_remote( url, headers=headers, stream=True, timeout=(10, read_timeout) ) as response: response.raise_for_status() diff --git a/backend/apps/library/tools.py b/backend/apps/library/tools.py index 8be258d..93861f9 100644 --- a/backend/apps/library/tools.py +++ b/backend/apps/library/tools.py @@ -8,7 +8,7 @@ from django.conf import settings from django.core.cache import cache from django.db.models import Count, Q from rest_framework import status -from rest_framework.permissions import AllowAny, IsAuthenticated +from rest_framework.permissions import AllowAny from rest_framework.response import Response from rest_framework.views import APIView @@ -120,7 +120,7 @@ def resolve_item(data): class ExactDuplicatesView(APIView): """Items whose content exists at more than one path.""" - permission_classes = [IsAuthenticated] + permission_classes = [CanUpload] def get(self, request): items = ( @@ -143,7 +143,7 @@ class ExactDuplicatesView(APIView): class VisualMatchesView(APIView): """Items visually similar to one library item.""" - permission_classes = [IsAuthenticated] + permission_classes = [CanUpload] def post(self, request): threshold = parse_threshold(request.data.get("threshold")) @@ -185,7 +185,7 @@ class VisualGroupsView(APIView): a personal library. Revisit with a bucketed index if libraries grow huge. """ - permission_classes = [IsAuthenticated] + permission_classes = [CanUpload] def post(self, request): threshold = parse_threshold(request.data.get("threshold")) @@ -450,7 +450,7 @@ def storage_info(): class StorageView(APIView): """Disk usage for the watched folder, media root and temp uploads.""" - permission_classes = [IsAuthenticated] + permission_classes = [CanUpload] def get(self, request): return Response(storage_info()) diff --git a/backend/apps/library/views.py b/backend/apps/library/views.py index 42b482c..6fc7fce 100644 --- a/backend/apps/library/views.py +++ b/backend/apps/library/views.py @@ -15,6 +15,7 @@ from rest_framework import mixins, status, viewsets from rest_framework.decorators import action from rest_framework.permissions import AllowAny, IsAuthenticatedOrReadOnly from rest_framework.response import Response +from rest_framework.throttling import ScopedRateThrottle from rest_framework.views import APIView from . import e621, matching, services @@ -428,9 +429,11 @@ class DownloadTaskViewSet( url = str(request.data.get("url") or "").strip() post_id = request.data.get("post_id") filename = str(request.data.get("filename") or "").strip() - if not url.startswith(("http://", "https://")): + try: + services.validate_remote_url(url) + except services.RemoteUrlError as exc: return Response( - {"detail": "A valid file URL is required."}, + {"detail": str(exc)}, status=status.HTTP_400_BAD_REQUEST, ) trimmed = services.trim_e621_post(request.data.get("post")) @@ -521,15 +524,15 @@ class ClientDownloadView(APIView): """Stream an e621 file straight to the browser (no library write).""" permission_classes = [AllowAny] + throttle_classes = [ScopedRateThrottle] + throttle_scope = "e621_proxy" def get(self, request): url = str(request.query_params.get("url") or "").strip() filename = str(request.query_params.get("filename") or "").strip() - parsed = urlparse(url) - if ( - parsed.scheme not in {"http", "https"} - or parsed.hostname not in settings.E621_MEDIA_HOSTS - ): + try: + services.validate_remote_url(url) + except services.RemoteUrlError: return Response( {"detail": "URL not allowed."}, status=status.HTTP_400_BAD_REQUEST, @@ -538,13 +541,17 @@ class ClientDownloadView(APIView): import requests try: - upstream = requests.get( + upstream = services.open_remote( url, headers={"User-Agent": settings.USER_AGENT}, stream=True, timeout=60, ) upstream.raise_for_status() + except services.RemoteUrlError as exc: + return Response( + {"detail": str(exc)}, status=status.HTTP_400_BAD_REQUEST + ) except requests.RequestException as exc: return Response( {"detail": f"Could not fetch the file: {exc}"}, diff --git a/backend/config/settings.py b/backend/config/settings.py index 5b55dd7..5c9521c 100644 --- a/backend/config/settings.py +++ b/backend/config/settings.py @@ -6,6 +6,7 @@ import os import subprocess from pathlib import Path +from django.core.exceptions import ImproperlyConfigured from dotenv import load_dotenv BASE_DIR = Path(__file__).resolve().parent.parent @@ -14,6 +15,12 @@ load_dotenv(BASE_DIR / ".env") SECRET_KEY = os.getenv("SECRET_KEY", "django-insecure-dev-only-change-me") DEBUG = os.getenv("DEBUG", "True").lower() == "true" +if not DEBUG and SECRET_KEY == "django-insecure-dev-only-change-me": + raise ImproperlyConfigured( + "SECRET_KEY is still the development default. Set a long random value " + "in backend/.env before running with DEBUG=False — it signs the media " + "URLs and encrypts stored e621 keys." + ) ALLOWED_HOSTS = [ host.strip() for host in os.getenv("ALLOWED_HOSTS", "localhost,127.0.0.1,0.0.0.0").split(",") @@ -234,6 +241,21 @@ REST_FRAMEWORK = { ], "DEFAULT_PAGINATION_CLASS": "config.pagination.StandardPagination", "PAGE_SIZE": 48, + # Per-IP/per-user rate limits (counted in the shared Redis cache). + "DEFAULT_THROTTLE_CLASSES": [ + "rest_framework.throttling.AnonRateThrottle", + "rest_framework.throttling.UserRateThrottle", + ], + "DEFAULT_THROTTLE_RATES": { + # Generous enough for the shell polling (status every 5s, stats every 2s). + "anon": os.getenv("THROTTLE_ANON", "120/min"), + "user": os.getenv("THROTTLE_USER", "600/min"), + # Credential stuffing / spam guards. + "login": os.getenv("THROTTLE_LOGIN", "5/min"), + "register": os.getenv("THROTTLE_REGISTER", "20/hour"), + # The guest e621 download proxy streams whole files. + "e621_proxy": os.getenv("THROTTLE_E621_PROXY", "60/hour"), + }, } DEFAULT_AUTO_FIELD = "django.db.models.BigAutoField" diff --git a/backend/requirements.txt b/backend/requirements.txt index 102799f..31dafb8 100644 --- a/backend/requirements.txt +++ b/backend/requirements.txt @@ -2,6 +2,7 @@ Django>=6.0,<6.2 djangorestframework>=3.17 django-filter>=25.1 django-cors-headers>=4.9 +cryptography>=46.0 Pillow>=11.0 python-dotenv>=1.0 requests>=2.32 diff --git a/frontend/src/features/users/UsersPage.tsx b/frontend/src/features/users/UsersPage.tsx index 2999b8a..790fc93 100644 --- a/frontend/src/features/users/UsersPage.tsx +++ b/frontend/src/features/users/UsersPage.tsx @@ -47,6 +47,18 @@ function UserRow({ profile }: { profile: AdminUser }) { // Mirrors the backend rules: nobody deletes themselves, and staff can only // delete accounts that are not staff/admin. + const targetIsStaff = Boolean( + profile.is_superuser || profile.is_staff || profile.role === "staff", + ); + const canChangeRole = Boolean( + actor && (actor.is_superuser || !targetIsStaff), + ); + // Staff cannot grant the staff role, so it is not even offered to them. + const roleOptions = + actor?.is_superuser || targetIsStaff + ? ROLE_OPTIONS + : ROLE_OPTIONS.filter((option) => option.value !== "staff"); + const canDelete = Boolean( actor && actor.id !== profile.id && @@ -108,10 +120,14 @@ function UserRow({ profile }: { profile: AdminUser }) { onChange={(event) => mutation.mutate({ role: event.target.value }) } - disabled={mutation.isPending} - title="Role" + disabled={mutation.isPending || !canChangeRole} + title={ + canChangeRole + ? "Role" + : "Only an admin can change staff roles" + } > - {ROLE_OPTIONS.map((option) => ( + {roleOptions.map((option) => (